feat(daemon): add session artifact content retention#6346
Conversation
|
This is the corrected upstream stacked PR for content retention. It is stacked on #6259 and should be reviewed as the delta from The fork-only PR chiga0#3 was opened in the wrong repository and is superseded by this PR. Local validation before opening this PR passed: acp-bridge/sdk/cli typecheck, acp-bridge build, targeted SDK/CLI/ACP/acp-bridge vitest runs, prettier check, and |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, reviewer test plan included. Problem: This is a feature addition, not a bug fix — the "problem existence" check doesn't apply in the usual sense. The content-retention layer is a planned extension of the session artifact system described in the design doc ( Direction: Aligned. Session artifact content retention (pin/unpin, managed copies, fsck/gc) is a natural next step on top of the metadata persistence foundation in #6259. The Approach: The scope is substantial (~1,890 lines of non-test source across three packages) but focused on a coherent feature. The content store is well-designed: atomic writes, SHA-256 integrity, workspace path traversal protection (symlink/hardlink/TOCTOU checks), quota enforcement (50MB per file, 256MB total), lease-based GC protection. Tests are comprehensive and match the source scope (~1,340 lines). The stacking on #6259 is the right call to keep reviews manageable. One concern: Base PR #6259 currently has Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必填部分齐全，双语，包含 reviewer test plan。 问题： 这是功能新增而非 bug fix，"问题存在"检查不适用常规标准。content-retention 层是 session artifact 系统的计划扩展，设计文档中有描述。用例明确：用户需要保留的 workspace 文件内容，带 managed copy、quota 和清理。 方向： 对齐。session artifact content retention（pin/unpin、managed copy、fsck/gc）是 #6259 metadata persistence 基础上的自然扩展。 方案： 范围较大（~1,890 行非测试源码，跨三个 package），但聚焦在同一个功能上。content store 设计良好：原子写入、SHA-256 完整性校验、workspace 路径穿越保护、quota enforcement、lease-based GC 保护。测试覆盖面广。stacked PR 方式正确。 一个关注点： 基础 PR #6259 当前状态为 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading diff): For content retention on top of metadata persistence, I'd add a Comparison with PR: The implementation closely matches this proposal. The content store is well-structured — atomic writes via temp file + fsync + rename, SHA-256 hashing during copy (not a separate pass), and a manifest-based content directory layout. Security is thorough: realpath traversal check before open, O_NOFOLLOW on open, post-open stat verification (TOCTOU guard), hardlink detection, symlink escape rejection, and size limit enforcement during streaming copy. Findings: No critical blockers. A few observations:
Real-Scenario TestingNot performed. This PR adds daemon API endpoints (pin/unpin/fsck/gc) that require a running The unit test coverage is thorough (~1,340 lines of new test code) and covers the key scenarios: pin/unpin lifecycle, TTL expiry, quota enforcement, symlink/hardlink rejection, GC behavior, fsck integrity, lease protection, concurrent pin serialization, and REST/ACP transport. 中文说明代码审查独立提案： 在 metadata persistence 之上添加 content retention，我会在 acp-bridge 中添加 与 PR 对比： 实现与提案高度一致。content store 结构良好——原子写入、copy 过程中同步 hash、基于 manifest 的目录布局。安全性全面：realpath 穿越检查、O_NOFOLLOW、open 后 stat 验证（TOCTOU 防护）、hardlink 检测、symlink 逃逸拒绝、流式 copy 中大小限制。 发现： 无关键阻塞问题。
真实场景测试未执行。 此 PR 添加 daemon API 端点，需要运行的 — Qwen Code · qwen3.7-max |
ReflectionStepping back: this is a well-executed stacked PR that adds a content-retention layer exactly as described in the design doc. The implementation matches what I'd propose independently — content store with atomic writes, SHA-256 integrity, quota, session-scoped GC, and comprehensive security checks against workspace path traversal attacks. The test suite is thorough and covers the important edge cases (TTL expiry, concurrent pins, symlink escape, hardlink rejection, quota overflow, lease protection). The code doesn't feel over-engineered. Each piece serves the stated goal: the manifest format enables future fsck, the lease mechanism prevents GC-during-read races, the write queue prevents concurrent quota corruption. Nothing speculative. What I'd flag for the maintainer:
The author clearly understands the codebase conventions — validation patterns match existing routes, error handling follows established patterns, capability gating is consistent with Verdict: Code is ready to merge once #6259 lands. Escalating to the maintainer for the cross-package scope and stacked-PR dependency awareness. 中文说明总结这是一个执行良好的 stacked PR，按设计文档添加了 content-retention 层。实现与我的独立提案一致——content store 带原子写入、SHA-256 完整性、quota、session-scoped GC、全面的 workspace 路径穿越防护。测试覆盖面广。 代码没有过度工程化。每个部分都服务于目标：manifest 格式支持未来 fsck，lease 机制防止 GC 读竞争，写入队列防止并发 quota 损坏。无投机性代码。 需 maintainer 关注的点：
代码在 #6259 合并后即可 ready to ship。转交 maintainer 关注跨 package 范围和 stacked PR 依赖。 — Qwen Code · qwen3.7-max |
# Conflicts: # packages/acp-bridge/src/bridge.ts
|
Design decision needed for content-retention review: a #6259 review thread flagged that Please decide the intended quota policy before implementation: separate |
|
Updated the stacked content-retention branch in Validation run after the stack update:
Note: an initial parallel run of two CLI vitest commands hit the shared coverage temp directory; rerunning |
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/cli/src/serve/acp-http/dispatch.ts:453 |
ACP dispatch pick* functions extract mode, ttlDays, clientRetained, retention, deleteContent using as casts with zero runtime validation. REST routes validate types, but JSON-RPC doesn't. |
Add typeof checks in pickSessionArtifactPinRequest, pickSessionArtifactUnpinRequest, pickSessionArtifactRemoveRequest mirroring the REST parsers, or extract a shared validator. |
packages/acp-bridge/src/bridge.ts:3408 |
Session close awaits gcArtifactContent synchronously. If content store is large or disk is slow, close blocks — cascading during multi-session shutdown and risking SIGKILL from orchestrators. |
Make GC fire-and-forget during close. Content will be cleaned up on next session's GC or daemon restart. |
packages/acp-bridge/src/sessionArtifactContentStore.ts:21 |
Global quota (256 MB) shared across all sessions, but gc() only removes content for the calling session. One heavy session can starve all others with no cross-session reclaim path. |
Scope quota per-session, or add an orphan GC pass that removes entries whose sessionId doesn't match any live session. |
packages/acp-bridge/src/bridge.ts:4737 |
removeSessionArtifact with deleteContent: false only skips the eager GC call. The next GC trigger (session close, rewind, or explicit gcSessionArtifacts) still deletes the orphaned content since no artifact references it. |
Either add a preservation marker that gc() respects, or rename/document the option to clarify it only defers cleanup. |
packages/sdk-typescript/src/daemon/DaemonClient.ts:3212 |
New methods use encodeURIComponent(...) directly instead of the local urlEncode alias used by 60+ existing call sites in the same file. |
Replace encodeURIComponent(...) with urlEncode(...) to match the local convention. |
| Test coverage (5 paths) | Untested: (1) pinSessionArtifact with mode: 'content' on non-workspace artifact, (2) removeSessionArtifact with { deleteContent: false } asserting GC is skipped, (3) bridge-level fsckSessionArtifacts/gcSessionArtifacts happy paths, (4) verifyContentRef integration during session restore, (5) gcArtifactContent invocation during session close/rewind. |
Add bridge.test.ts cases for each path to verify the wiring between bridge, artifact store, and content store. |
— qwen3.7-max via Qwen Code /review
|
Processed the latest suggestion summary in
I did not change quota semantics or |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
|
No new Suggestion-level findings this round — all prior suggestions have been addressed or superseded. |
|
Updated this stacked content-retention PR by merging the refreshed #6259 base in Validation after the merge:
Note: an earlier parallel |
|
Updated this stacked content-retention PR by merging the refreshed #6259 base in Validation after the merge:
Also confirmed the content-retention restore path wires |
|
Follow-up for the latest content-retention review: pushed What changed:
Validation:
|
…/session-artifacts-content-retention
|
Synced this stacked content-retention PR with the refreshed #6259 base in merge commit Validation after the merge:
|
| await gcArtifactContent(entry); | ||
| } catch (error) { | ||
| if (removeOptions.deleteContent === true || artifact.contentRef) { | ||
| warnings.push('content_delete_preserved'); |
There was a problem hiding this comment.
[Suggestion] The same warning string 'content_delete_preserved' is pushed from three distinct failure contexts: removeSessionArtifact GC failure (line 4747), pinSessionArtifact metadata-mode GC failure (line 4796), and unpinSessionArtifact GC failure (line 4877). The stderr logs differentiate (remove_gc_failed, pin_metadata_gc_failed, unpin_gc_failed), but clients receiving the warning array cannot distinguish which operation's GC failed.
Use distinct warning strings per context, e.g. 'remove_content_preserved', 'pin_metadata_content_preserved', 'unpin_content_preserved', so clients can provide actionable diagnostics.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, I am leaving this one unresolved because changing content_delete_preserved into operation-specific warning strings is a client-visible API contract change. The current stderr actions already distinguish remove vs pin-metadata vs unpin failures; changing the warning vocabulary should get maintainer/design confirmation first.
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
1 similar comment
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
1 similar comment
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #6346 is draft. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| // `extNotification` from the (about-to-be-killed) child can't | ||
| // seed the early-event buffer for a subsequent load/resume of | ||
| // the same persisted id. | ||
| ci?.client.markSessionClosed(sessionId); |
There was a problem hiding this comment.
[Suggestion] killEmptyChannelAfterCleanup introduces a new fast-path that immediately kills the channel when resolvedChannelIdleTimeoutMs() <= 0 && hasNoChannelWork(ci), bypassing reapPendingEmptyChannel + startIdleTimer. This is a behavioral change to channel lifecycle that (1) is not mentioned in the design doc, (2) has no dedicated test coverage, and (3) is inconsistent with closeSessionImpl, which always uses reapPendingEmptyChannel without this fast-path. This optimization is outside the PR's stated scope of content retention — consider calling it out in the PR description and adding test coverage, or splitting it into a separate commit.
— qwen3.7-max via Qwen Code /review
| @@ -3436,4 +3494,649 @@ describe('SessionArtifactStore', () => { | |||
| ], | |||
There was a problem hiding this comment.
[Suggestion] The design doc at session-artifacts-persistence-v2-design.md (~line 925) explicitly requires testing "TOCTOU swap" during pin. The openRegularWorkspaceFile function implements post-open checks (realpath re-validation, sameFile inode comparison, nlink check, size re-check), but no test simulates file replacement between fs.open and the post-open stat. The pre-open checks (hardlink, oversized, symlink escape) are tested, but the post-open TOCTOU protection — the most security-sensitive part — has zero coverage. Consider adding a test that swaps the file (via rename or atomic replace) between the open call and the post-open verification.
— qwen3.7-max via Qwen Code /review
| @@ -949,9 +955,9 @@ V2 新增的失败路径必须有 structured logs，格式沿用： | |||
| - SDK 旧 client 忽略 optional fields 后仍能展示 V1 artifacts。 | |||
| - V2 -> V1 rollback compatibility：旧 daemon 必须能解析或忽略 unknown `system` subtype，不得导致 session load 崩溃；回滚后 artifact persistence 不恢复是可接受降级。如果当前最低支持版本不能保证这一点，V2 writer 必须 capability-gate 到支持 unknown system record 的版本之后。 | |||
| - rollback preflight：最低支持旧 daemon 版本加载包含 V2 event/snapshot 的 JSONL；如果未来加入 fork marker，再扩展 rollback fixture。 | |||
There was a problem hiding this comment.
[Suggestion] The design doc states that GET /artifacts performs a TTL scan that downgrades expired pinned content and triggers best-effort session-scoped GC. However, getSessionArtifacts in bridge.ts only calls entry.artifacts.list(), which refreshes workspace statuses but never calls pruneExpiredPins() or gcArtifactContent(). The new test "does not prune or GC expired pins during artifact GET" explicitly asserts the expired artifact retains retention: 'pinned' and gcSpy was not called — directly contradicting the design doc. Either update the design doc to mark this as a future behavior, or implement the TTL scan during GET as documented.
— qwen3.7-max via Qwen Code /review
| '/session/:id/artifacts/:artifactId/pin', | ||
| mutate({ strict: true }), | ||
| withMutableSession( | ||
| 'POST /session/:id/artifacts/:artifactId/pin', |
There was a problem hiding this comment.
[Suggestion] The if (!artifactId) guard is dead code. Express matches :artifactId as a required route segment — a request without it 404s at route selection, and the parameter cannot be empty once the route is matched. The equivalent check in dispatch.ts is correct (JSON-RPC params can genuinely be absent), but this REST-side branch is unreachable. Consider removing it to avoid confusion, or at minimum add a brief comment.
— qwen3.7-max via Qwen Code /review
| .json(await bridge.fsckSessionArtifacts(sessionId, { clientId })); | ||
| } catch (err) { | ||
| sendBridgeError(res, err, { | ||
| route: 'GET /session/:id/artifacts/fsck', |
There was a problem hiding this comment.
[Suggestion] The new fsck, gc, pin, and unpin routes call sendBridgeError on thrown errors, but no test exercises the bridge-error surface for these endpoints. Specifically: SessionNotFoundError from fsck/gc (bridge throws when byId.get(sessionId) misses), and SessionArtifactAuthorizationError from pin/unpin (via denyCrossClientMutation). The sendBridgeError error-mapping path (authorization → 403, not-found → 404) is only as trustworthy as the tests that exercise each route through it. Add at minimum one SessionNotFoundError test for fsck and one authorization error test for pin.
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
This is the content-retention layer stacked on #6259. It adds pinned artifact content retention on top of the metadata and restart-recovery foundation: retained content can be pinned and unpinned, read through daemon APIs, addressed by content references and hashes, and maintained with quota and cleanup behavior.
Why it's needed
The base PR keeps artifact metadata durable across daemon restart without committing to long-lived file content. This follow-up isolates the heavier review surface so reviewers can focus on filesystem safety, retained-content API behavior, quota enforcement, cleanup behavior, and REST/ACP/SDK compatibility without mixing it with metadata rebuild semantics.
Reviewer Test Plan
How to verify
Review this PR as a stacked diff against
feat/session-artifacts-persistence-v2-design, not againstmain. Confirm that metadata persistence remains in #6259 and this PR only adds retained-content behavior. Verify pin/unpin behavior, retained content reads, quota handling, cleanup behavior, and client/API exposure.Evidence (Before & After)
N/A for UI. Local validation passed with:
cd packages/acp-bridge && npm run typecheckcd packages/sdk-typescript && npm run typecheckcd packages/acp-bridge && npm run buildcd packages/cli && npm run typecheckcd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts test/unit/DaemonSessionClient.test.ts test/unit/acpRouteTable.test.tscd packages/cli && npx vitest run src/serve/server.test.ts -t "session artifact routes" --coverage.enabled=falsecd packages/cli && npx vitest run src/serve/acp-http/transport.test.ts -t "_qwen/session/artifacts" --coverage.enabled=falsecd packages/acp-bridge && npx vitest run src/bridge.test.ts src/sessionArtifacts.test.tsnpx prettier --check packages/acp-bridge/src/bridge.test.tsgit diff --checkTested on
Environment (optional)
Local package typecheck/build/vitest validation on the stacked branch.
Risk & Scope
Linked Issues
Stacked on #6259.
中文说明
What this PR does
这是堆叠在 #6259 之上的 content-retention 层。它在 metadata 和 daemon restart recovery 基础上增加被 pin 的 artifact content 保留能力：内容可以 pin/unpin，可以通过 daemon API 读取，可以用 content reference/hash 定位，并且有 quota 和清理行为。
Why it's needed
基础 PR 只保证 artifact metadata 在 daemon 重启后可恢复，不承诺长期保存文件内容。这个后续 PR 把更重的 review 面单独隔离出来，让 reviewer 可以专注检查文件系统安全、retained-content API 行为、quota enforcement、清理行为以及 REST/ACP/SDK 兼容性，而不用和 metadata rebuild 语义混在一起。
Reviewer Test Plan
How to verify
请按相对
feat/session-artifacts-persistence-v2-design的 stacked diff review，而不是按相对main的完整 diff review。确认 metadata persistence 仍在 #6259 中，本 PR 只增加 retained-content 行为。重点验证 pin/unpin、retained content read、quota、清理行为，以及 client/API 暴露。Evidence (Before & After)
UI 不适用。本地验证已通过上面列出的 typecheck、build、vitest、prettier 和 diff check 命令。
Tested on
macOS 已本地验证；Windows/Linux 未本地验证，依赖 CI 覆盖。
Environment (optional)
在 stacked branch 上运行本地 package typecheck/build/vitest 验证。
Risk & Scope
Linked Issues
堆叠在 #6259 之上。