feat(daemon): persist session artifacts across restarts#6259
Conversation
|
Thanks for the design doc! Template looks good ✓ Direction: Clean follow-up to #5895 (merged today). Lives in the existing A few things worth surfacing before we get to line-level review:
Approach: Scope feels right for a design doc — 671 lines covering data model, storage, API, security, quota/GC, milestones, and test plan. No runtime code, no drive-by refactors, no unrelated edits. The document is deliberately opinionated (e.g., "don't fetch remote URLs," "don't treat One question worth thinking about: §2.2 defines three retention tiers ( Moving on to document review. 🔍 中文说明感谢设计文档！ 模板完整 ✓ 方向： 作为 #5895（今天刚合并）的后续，方向清晰。文档放在已有的 在进入逐行 review 之前，有几点值得提前点名：
方案： 作为设计文档，范围合理——671 行覆盖数据模型、存储、API、安全、配额/GC、里程碑、测试计划。没有运行时代码，没有顺手重构，没有无关改动。文档刻意给出明确立场（例如“不抓取远端 URL”“不把 一个问题值得再确认：§2.2 定义三层 retention（ 进入文档审查 🔍 — Qwen Code · qwen3.7-max |
Code reviewIndependent proposal (before reading): For a V2 persistence layer on top of V1's live session artifacts, I'd expect — append-only event log as source of truth (JSONL fits the existing chat recording naturally), a fast-load cache for session startup latency, explicit retention tiers so users opt into content persistence rather than having it happen by default, restore-time revalidation that doesn't trust on-disk state, fork/branch handling that remaps IDs, and quota/GC before any content API ships. The doc hits all of those. Comparison with the diff: The document is well-structured and covers the design space thoroughly. The two-layer storage (JSONL source of truth + sidecar cache), three-tier retention ( Findings:
None of these are blockers — they're all polish-level gaps in an otherwise thorough design. TestingDocs-only PR — no runtime changes, no user-visible behavior to drive in tmux. Verification is limited to: Markdown fences are balanced (verified by reading the rendered output above). The diff contains only the new design document, no stray edits. 中文说明代码审查独立方案（读 diff 之前）： 在 V1 live session artifacts 之上做 V2 持久化，我会期待——append-only event log 作为 source of truth（JSONL 天然适配已有 chat recording）、一个用于 session 启动延迟的快速加载缓存、显式 retention 分层让用户主动选择内容持久化而不是默认发生、恢复时重新校验而不信任磁盘状态、fork/branch 处理要重写 ID、content API 上线前先有 quota/GC。文档覆盖了所有这些。 与 diff 对比： 文档结构良好，覆盖设计空间充分。两层存储（JSONL source of truth + sidecar cache）、三层 retention（ 发现：
都不是阻塞问题——都是整体扎实设计之上的润色级缺口。 测试仅文档 PR——无运行时变更，无用户可见行为可用于 tmux 驱动。验证范围仅限于： Markdown 代码块闭合正常（已通过上方渲染输出确认）。diff 只包含新增设计文档，没有顺手改动。 — Qwen Code · qwen3.7-max |
|
Reflection on the whole PR. Stepping back — this is a design document, not code, so the bar is different. The question isn't "does it work" but "will future implementers ship something coherent from this spec." On that measure, it's strong. The document's best quality is how often it says "no." §11's explicit out-of-scope list, §7.1's refusal to use My independent proposal (Stage 2a) landed in roughly the same place as the PR: JSONL as source of truth, sidecar as cache, explicit retention tiers, restore-time revalidation. The PR actually goes further than I would have in a couple of spots — the The Stage 2 findings are all polish: a missing The two Stage 1 flags I raised still stand as questions for the author, not blockers:
On the approval-guardrail check: title is Verdict: Approving. The design is opinionated in the right places, the scope is tight for a spec doc, and the open questions I raised are the kind you answer during implementation rather than block on now. The author can address the Stage 2 polish items in a follow-up commit or leave them for the implementer to pick up — either is fine. 中文说明对整体 PR 的反思。 退一步看——这是设计文档，不是代码，所以评判标准不同。问题不是"它能不能用"，而是"未来实现者能不能根据这份 spec 交付出一致的东西"。在这个标准下，它是扎实的。 文档最大的优点在于它频繁地说"不"。§11 显式的 out-of-scope 列表、§7.1 拒绝把 我的独立方案（Stage 2a）和 PR 落在大致相同的位置：JSONL 作为 source of truth、sidecar 作为缓存、显式 retention 分层、恢复时重新校验。PR 在几个地方实际上比我预想的走得更远—— Stage 2 的发现都是润色级：缺少的 我在 Stage 1 提出的两个问题仍然作为给作者的问题存在，不是阻塞：
Approval guardrail 检查：标题是 结论： Approve。设计在该有立场的地方有立场，作为 spec 文档 scope 紧凑，我提出的开放问题是那种在实现过程中回答而不是现在就阻塞的类型。作者可以在后续 commit 里处理 Stage 2 的润色项，或者留给实现者自己去拿——都可以。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Design review — checked against the V1 implementation on
|
|
Addressed the review feedback in commits Summary:
Validation run locally: All review threads have been replied to and resolved. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
代码审查总结
审查了完整的设计文档，以下是发现的问题。其中 F1 为 Critical 级别——V2 设计移除了 V1 现有的 per-client delete 保护机制，但未引入替代方案。
高置信度发现 (已作为 inline comment 提交)
| # | 严重度 | 主题 | 位置 |
|---|---|---|---|
| F1 | Critical | V2 移除 V1 per-client delete 保护 | Section 6.4 |
| F4 | Suggestion | deleteContent 授权标准未定义 | Section 6.4 |
| F5 | Suggestion | GC 孤儿 grace period 未指定 | Section 8.3 |
| F6 | Suggestion | 缺少诊断/修复工具规范 | Section 8.6 |
| F7 | Suggestion | V2→V1 回滚未覆盖测试 | Section 10 |
| F9 | Suggestion | ttlDays 无上限约束 | Section 6.2 |
| F11 | Suggestion | 工作区重定位导致 artifacts 静默失效 | Section 3.3 |
| F15 | Suggestion | 裁剪顺序使用「例如」导致非规范 | Section 8.1 |
| F3 | Suggestion | "restore-skip 记录"未在 schema 定义 | Section 5.3 |
终端可见 (低置信度)
- F2: GC TOCTOU 竞态（跨 session pin）
- F8: expiresAt 在 restore 路径未检查
- F12: workspace_reference 是死枚举变体
- F14: Fork snapshot 记录处理未定义
— qwen3.7-max via Qwen Code /review
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Session Artifacts Persistence v2 · 设计审计
本文档在 v1 内存 store 之上加持久化（journal upsert/remove event + snapshot + tombstone，靠 active-branch replay 恢复）。方向合理，但核心的「tombstone + active-branch replay」持久化模型存在多处会导致「删除项复活 / 持久化项静默消失」的正确性缺口，以及一处安全回归。高强度审计（24 agent，17 条确认）后按你的维度归纳如下，逐条 inline。
① 可行性 / 可实施性 🔴
- restore 依赖 active-branch，但
reconstructHistory()只走 leaf 链（sessionService.ts:897）——/rewind 后 turn N 的 artifact journal 记录落到废弃分支被丢弃：删除 tombstone 被丢而旧 upsert 存活 → 删除项复活;或持久化项静默消失。代码里file_history_snapshot已为此加了显式 「top up」（sessionService.ts:1402-1426），v2 artifact 记录需要等价机制但设计未写。 - restore-prune tombstone 无 durable 落盘路径：prune 发生在
loadSession()（§5.4 step 1），而 store + operationQueue 到 step 6 才创建 → 首次 GET 与后续 replay 结果不一致。 - ACP 进程边界未打通（PLAUSIBLE）：snapshot 在
qwen --acpchild 产出，但 daemon 侧 store 只能经loadSession的 ACPLoadSessionResponseRPC 拿到；Milestone C 只提到扩展 core 返回类型，未点名扩展 ACP wire 契约 → daemon 重启后 GET 返回空、v2 承诺失效。
② 副作用 / 安全回归 🔴
- creator-only delete guard 每次重启失效：creator principal 故意不落盘（L210），restore 后 L496 分支必然命中 → 降级为「任意 session-mutate 持有者可删」，v1 的越权边界被静默移除（不是边界 case，是每次重启）。
- v1 内存 eviction 现写永久 tombstone：v1 §7.1 eviction 只为限内存，v2 却给它写 durable remove tombstone → 一个仅因 source 超软配额被逐出的
restorableartifact 被永久删除、永不恢复，即便持久化配额（§8.1 的 200）还有空位 → 数据丢失。 - live-first vs tombstone-first 自相矛盾（§5.2 vs §5.3）：满 200 时 add 内触发 eviction，§5.2 先改内存、§5.3 要求先写 tombstone;tombstone 写失败时内存已改无法回滚 → 崩溃后复活、可能超 200。
- unpin→ephemeral 被后续 upsert 复活：unpin 保留 live + 写 tombstone，但工具常重发同 identity → tombstone 之后又来 upsert → 下次 load 复活持久化，静默撤销用户「不要持久化」意图。
- DELETE 耦合 durable tombstone → 可用性回归：v1 DELETE 是纯内存、保证成功;v2 journal 不可写（磁盘满/只读）时 DELETE 报错且 live 不变 → 用户无法移除误登记或敏感 artifact。
- 内部字段丢失影响 eviction 公平性：
clientRetained/insertSeq不入 journal（§4.2），restore 后重置为 false/新序号 → 重启后一波新 artifact 触顶会裁掉用户点过 keep 的项。
③ 性能消耗 🟠
tombstonedIds无界增长：每个 snapshot「必须携带 tombstonedIds」但无过期/compaction → 频繁创建删除的长 session 让 snapshot 撑爆 100-400KB / 4MB working-set 预算（§4.4）→journal_budget_exceeded、新 artifact 降级 ephemeral，即便 live artifact 远不足 200。
④ 是否有更好的替代方案
上面的复活/顺序/膨胀问题大多源于「把 artifact 事件塞进 chat-record 树、靠 active-branch replay 恢复」。可考虑：
- artifact 用独立的 per-session append-only log + 定期 compaction（不挂 chat-record 树），避开 /rewind 分支丢弃问题，tombstone 在 compaction 时 GC，天然解决 #1/#7;
- snapshot 为权威、event 只做增量并在每次 snapshot 后截断（snapshot-authoritative），tombstonedIds 只需覆盖「最近一次 snapshot 之后」的删除,不再无界;
- eviction 与持久化解耦：内存 eviction 只影响 live 视图、不写 tombstone;持久化用独立 GC/配额 —— 直接消除 #4。
建议在文档「Alternatives / 取舍」一节显式对比现方案与上述，尤其为何选择耦合 chat-record 树。
⑤ 生命周期 / GC
- tombstone 无 compaction（#7）、restorable 配额与内存 eviction 语义纠缠（#4）——GC 未闭环。session 结束后持久化数据的保留/过期策略也未见明确 TTL。
⑥ v1→v2 迁移 / 兼容
- 未见 schema 版本化与 v1（无持久化）→ v2 的升级路径说明;restore 对缺 sha256/缺内部字段的历史记录的兼容处理需补。
总评
尚不足以作为实现依据。 方向与数据模型基本成立，但 tombstone/replay 持久化模型有 3 类必须先解决的问题：(A) active-branch replay 与 /rewind 的复活/丢失（#1、#2、#5），(B) 安全回归 creator guard 失效（#3）+ eviction 永久删除 restorable（#4），(C) tombstonedIds 无界（#7）。建议先在文档层面：明确 restore 的 top-up/权威快照策略、把 eviction 与持久化 tombstone 解耦、给 creator principal 一个可验证的持久化/降级契约、给 tombstone 定 compaction，并补一节替代方案对比与 v1→v2 迁移。逐条见下方 inline。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Round-2 pass at e90c6c4 — I re-verified every earlier "Fixed in …" reply against the current text and against the V1 implementation on main; the prior findings are genuinely resolved. The new findings below are mostly architectural: the journal design models the chat JSONL as a linear log the daemon can append to, but the real transcript is a parentUuid tree written exclusively by the ACP child process — that assumption breaks the write path, rewind, and fork semantics (first three comments). The rest cover pinned-vs-eviction, the V1→V2 backfill, the delete-authorization model, and two spec-consistency items.
Two minor textual mismatches, no dedicated comment: §2.3 L77 / §5.5 L321 still describe restore as recovering the persisted published locator, while §7.2 L520 requires file: locators to be regenerated from the manifest and never taken from old JSONL — the older sections should say the persisted locator is opaque and the openable locator is manifest-derived. And the three content-quota surfaces disagree in naming/scope: §8.2 defines artifact/session/project tiers, §6.1 exposes maxPinnedArtifactBytes/maxPinnedSessionBytes (no project knob), §7.3's settings offer maxArtifactBytes/maxTotalBytes (1 GB matches the project tier but the name doesn't say so, and there is no session-level setting).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
Scope: 1 file, +843 lines — design document for session artifacts persistence V2
Findings: 19 verified (13 high-confidence, 6 low-confidence), after deduplication against 15+ existing review comments.
Overall this is a thorough and well-structured design document. The three-tier retention model, snapshot/tombstone invariants, and restore validation are all well thought out. The findings below focus on specification gaps that could cause ambiguity during implementation.
Critical (7)
- Line 220 —
clientRetainedused for sorting/eviction but no API/SDK/ingest field sets it - Line 219 —
PersistedSessionArtifactinherits runtime fields (restoreState,persistenceWarning) that would replay stale values - Line 636 — GC TTL downgrade has no journal event
reasonenum value - Line 454 — Pin/Unpin/Delete endpoints lack response body structure and HTTP status codes
- Line 749 — V1→V2 backfill described in §2.1 but no milestone in §9 delivers it
- Line 280 —
expiresAtnot listed as daemon-only at ingest — client could inject it - Line 105 —
persistenceWarning.messageis free-form with no content constraint
Important (8)
8–19. See inline comments for details.
Positive
- Clear three-tier retention lifecycle with well-defined invariants
- Strong restore validation with defense-in-depth
- Tombstone/snapshot consistency model is thorough
- Privacy and security considerations are comprehensive
- Pragmatic milestone decomposition with capability gating
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Qwen Code Review — PR #6259
V2 session artifacts 持久化设计文档整体非常详尽，覆盖了 metadata restore、content retention、crash consistency、安全边界、配额和 GC 等关键维度。以下是 review 发现的需要关注的问题。
感谢作者的深入设计工作 🙏
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
自动 Review 结果
本 PR 是一份详尽的 V2 持久化设计文档（1009 行）。整体架构清晰、安全边界考虑充分。以下是本轮 review 发现的新问题，不包含已有 18 条已修复评论的内容。
汇总： 5 Critical / 6 Suggestion
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #6259
Model: qwen3.7-max | Mode: comment
Summary
This PR adds a comprehensive V2 design doc for daemon session artifact persistence. Overall the document is thorough and well-structured, covering a broad set of edge cases (fork semantics, crash consistency, authorization, quota/GC). Below are findings from automated review.
Findings delivered: 9 (3 Critical, 6 Suggestion)
- Critical issues focus on restore correctness (snapshot ordering, status ambiguity) and observability gaps (operation queue stall).
- Suggestions cover unbounded growth, degenerate ordering, API schema gaps, and cleanup protocols.
This review was generated automatically by Qwen Code.
|
Follow-up on the workspace I added a small stat fast path in Current behavior:
So the normal GET/list refresh path is now stat-only for unchanged files, while still detecting ordinary same-size rewrites through the hash fallback. This remains metadata-only and does not reintroduce content retention into #6259. Validation:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Automated Review — PR #6259
Build: ✅ passes | Tests: ✅ 76/76 pass (12 core + 64 acp-bridge) | ESLint: ✅ clean
Overall this is a well-structured PR with solid normalization and defensive coding in the persistence layer. The normalizePersistedArtifact and normalizeContentRef validators are thorough, and the event-sourcing replay model in rebuildSessionArtifactSnapshot is clean.
Inline comments below cover 4 critical findings and several suggestions. The most impactful are the cross-client authorization gap in upsertMany and the fork remap that silently drops removal events.
|
Handled the current unresolved #6259 review set in What changed:
Validation run locally:
I am now going through the remaining unresolved review threads and resolving the stale/scope-split ones with thread-level notes. |
聚焦审计（head
|
|
Handled the latest focused audit from #6259 (comment) in What changed:
Validation run:
Latest review-thread check after the push reports |
Qwen Code Review — PR #6259Summary: V2 daemon session artifact persistence — JSONL journal, snapshot compaction, managed content store with pin/unpin, tombstones, and restore-on-restart. Well-structured with strong security hygiene (restore-time revalidation, workspace containment, URL sanitization, quota enforcement). Build & tests: All pass — 🔴 Critical1. Rewind with failed artifact snapshot rebuild silently wipes all durable artifacts
2. sessionArtifactsPersist handler: 4 of 5 validation branches untested
🟡 Suggestions3. Cross-package type drift: duplicate aliases + SDK omissions + type assertions
4. pin() accepts
5. unpin() unconditionally deletes
6. Fork remap preserves orphaned
7. stickyEphemeralOverride bypassable with explicit
8. Eviction clears
9. persistChanges mutates change objects before persistence
10. Partial restore doesn't rollback; no summary count in warnings
11.
12.
13.
🔵 Needs Human Review
Verdict: Request changes — The rewind data-loss path (finding 1) can permanently destroy durable artifact metadata on a single failed rebuild. Generated by qwen-code-review-plugin via QoderWork |
|
Fixed the Ubuntu CI failures in What changed:
Validation run locally:
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review of PR #6259 — feat(daemon): persist session artifacts across restarts
3 Critical issues found:
- bridge.ts:5936 —
restore()called with potentiallyundefinedartifactSnapshot +preserveLiveEphemeral: truewipes all durable artifacts on rewind (see inline comment) - sessionArtifacts.ts:2075 (
normalizeMetadatafunction) — Prototype pollution via__proto__key not filtered beforenormalized[key] = valueassignment. Fix: addif (key === '__proto__' || key === 'constructor' || key === 'prototype') continue;in the loop. Also apply tosession-artifact-persistence.tsnormalizeMetadata. - session-artifact-persistence.ts:58 —
PersistedSessionArtifactlacksclientId, causing cross-client authorization bypass after session restore (see inline comment)
Build passes ✅, core tests pass ✅. acp-bridge tests failed to load due to pre-existing @xterm/headless env issue (not PR-related).
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/acp-bridge/src/bridge.ts:4570 |
removeSessionArtifact calls await entry.artifacts.get(artifactId) before remove(). The get() triggers refreshWorkspaceStatus (file stat + potential sha256 hash) for workspace artifacts. remove() already handles nonexistent artifacts gracefully by returning empty changes. |
Remove the get() pre-check, or add a synchronous has() method that doesn't trigger workspace status refresh. |
packages/acp-bridge/src/bridge.ts:2645 |
artifactReseedChanges labels rewind-removed artifacts with reason: 'explicit', which misrepresents the actual removal cause to SDK clients. Rewind-induced removals are not explicit user deletions. |
Add a 'rewind' reason to SessionArtifactRemovalReason, or use 'eviction' as the closest existing semantic. |
Needs Human Review
These findings were flagged by review agents but not confirmed with high confidence — they may be intentional design decisions:
packages/acp-bridge/src/sessionArtifacts.ts:951— Duringrestore(), every workspace artifact triggershashFile()unconditionally (noexpectedparameter). Session restore latency scales with total workspace file sizes. May be intentional for integrity verification.packages/acp-bridge/src/sessionArtifacts.ts:285-290— In strict mode,restoreStaterolls back in-memory state but not persisted JSONL records written bypersistChanges. Unlikely scenario (requiresmaybeRecordSnapshotto throw afterrecordEventsucceeds).packages/acp-bridge/src/sessionArtifacts.ts:2161-2178— TOCTOU race betweenfs.realpath()containment check andhashFile()ingetWorkspaceStatus. Limited threat model for local CLI (attacker needs workspace write access).packages/acp-bridge/src/sessionArtifacts.ts:540-555— Restore rollback only triggers when ALL artifacts fail (restoredCount === 0). Partial failures keep inconsistent state. Current behavior is a reasonable compromise.
— qwen3.7-max via Qwen Code /review
|
Triage for #6259 (comment) on current head:
Validation for the new commit:
|
聚焦审计（head
|
…-v2-design' into feat/session-artifacts-persistence-v2-design
|
Handled the focused audit from #6259 (comment) in the pushed head What changed:
Validation:
Pushed normally to both |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-Review at HEAD d4d8691a
12 new non-merge commits since last review at 44e2d3dd. Major changes: workspace content change detection (sha256 + mtime), pinned retention removed entirely, enhanced tombstone tracking, preserveLiveEphemeral restore option, fork-side artifact record inclusion, rewind artifact reconciliation.
Status of Previous Findings
| Finding | Status |
|---|---|
Major: TOCTOU in pinWorkspaceFile |
N/A — content retention removed in 729dff74 |
Minor: stickyEphemeralIds unbounded |
Addressed — bounded by maxArtifacts (200) via snapshot filter |
New Findings
Major: Rewind synthesizes empty snapshot on loadSession returning undefined (acpAgent.ts, rewind handler)
When loadSession returns undefined (session empty, not found, or transient I/O failure), the rewind handler falls through to a synthetic empty snapshot:
artifactSnapshot = sessionData?.artifactSnapshot ?? {
v: SESSION_ARTIFACT_PERSISTENCE_VERSION,
sessionId, sequence: 0, artifacts: [], tombstonedIds: [], stickyEphemeralIds: [], warnings: [],
};This calls store.restore(emptySnapshot, { preserveLiveEphemeral: true }), which clears all durable artifacts. If undefined was caused by a transient issue (file I/O race during rewind), durable artifacts are silently lost. The adjacent catch block (artifactSnapshotUnavailable path) correctly avoids this by not calling restore().
Fix: Treat sessionData === undefined similarly to the catch path — set artifactSnapshotUnavailable rather than synthesizing an empty snapshot. Or at minimum, only synthesize when the caller can confirm the session is genuinely empty (not a load failure).
Minor: remove() no longer rolls back on persistence failure (sessionArtifacts.ts, ~line 454)
The old remove() captured before = this.cloneState() before deleting and called persistChanges(changes, strict=true) when persistence was available, rolling back on failure. The new code passes persistenceStrict = false and has no cloneState(). If persistence fails, the artifact is unconditionally removed from the in-memory store but the removal is not durable — creating a divergence between live state and the journal.
The design doc (§5.3) specifies durable-first for explicit DELETE. If remove() is only called for eviction (not explicit delete), this is acceptable since eviction is live-only by design. But if explicit delete flows through this path, the rollback safety net is lost.
Minor: mtime-based hash skip can miss content changes (sessionArtifacts.ts, commit 37fb6ccc)
The optimized getWorkspaceStatus short-circuits sha256 computation when size + mtimeMs are both unchanged. This heuristic is violated by git checkout/git restore (restores content while preserving mtime depending on config), cp -p/rsync -a, and coarse-resolution filesystems. A workspace artifact may report 'available' when the file content has actually changed.
This is a reasonable performance tradeoff for the common case (IDE saves, build outputs always change mtime). Consider documenting it as a best-effort heuristic and/or adding ctimeMs (inode change time) as an additional gate.
Minor: lastRestoreWarnings persist indefinitely in list() output (sessionArtifacts.ts, ~line 248)
lastRestoreWarnings is only cleared by another restore() call. Every list() invocation between restores includes the same stale warnings with no mechanism to acknowledge or dismiss them. Consider clearing warnings after the first list() that surfaces them, or adding a version/timestamp to the warnings.
Verified Non-Issues
- Fork artifact parentUuid: Confirmed correct. Forked conversation records reuse source UUIDs, and artifact records' parentUuid references source-session parents that ARE on the active branch (already validated by
includeActiveSideArtifactRecords). No orphaning risk. rememberTombstoneSet ordering: Confirmed correct. JS Setdelete+adddoes move the element to the end of insertion order per ECMAScript spec. The LRU eviction viaSet.values().next().valuecorrectly evicts the oldest entry.- Sticky ephemeral override: Still correct.
applyStickyEphemeralOverrideproperly checksretentionExplicitand sticky IDs are correctly loaded from snapshots during restore. normalizePersistedArtifactallowlist: Still correct with proper prototype pollution guards (__proto__,constructor,prototype).updateActiveTail: falsefor artifact records: Correctly decouples artifact writes from the conversation tail pointer.
Additional Audit Coverage
- Workspace content change detection: Fast-path (size + mtime) → slow-path (sha256) is a sound tiered approach. The
changedstatus correctly maps back toavailablein persistence (preventing daemon-local status leakage). preserveLiveEphemeralrestore: Correctly re-injects ephemeral artifacts after snapshot restore with tombstone/ID collision checks. Skip-re-validation is acceptable since ephemeral artifacts are short-lived.remapSessionArtifactPayloadForFork: Correctly handles both snapshot and event payloads, dropstombstonedIds/stickyEphemeralIds, downgrades pinned→restorable, deletescontentRef.- ChatRecordingService
updateActiveTailparameter: Clean addition that correctly prevents artifact system records from becoming conversation endpoints.
Verdict
COMMENT. One Major finding (rewind empty-snapshot synthesis) should be addressed to prevent potential data loss on transient load failures. Three Minor findings are noted for consideration. The overall implementation continues to improve — the workspace change detection, enhanced tombstone tracking, and fork-side artifact inclusion are all well-executed.
This review was generated by QoderWork AI
聚焦审计（head
|
|
Handled #6259 (comment) in What changed:
I kept this fix intentionally narrow for #6259: no content-retention changes, no API contract changes, and no history rewrite. Validation:
Pushed normally to both |
| type RebuiltSessionArtifactSnapshot, | ||
| type SessionArtifactEventRecordPayload, | ||
| type SessionArtifactSnapshotRecordPayload, | ||
| } from '@qwen-code/qwen-code-core'; |
There was a problem hiding this comment.
[Critical] Importing from @qwen-code/qwen-code-core barrel pulls in shellExecutionService.ts → @xterm/headless, which resolves to undefined under vitest ESM transform from the acp-bridge package. This causes a suite collection error — 0 tests run across all 42+ existing and new tests in this file. This is a regression from main.
| } from '@qwen-code/qwen-code-core'; | |
| // Option 1: Mock @xterm/headless in vitest setup | |
| vi.mock('@xterm/headless', () => ({ default: { Terminal: class {} } })); |
Or create a focused sub-entry in @qwen-code/qwen-code-core so acp-bridge can import artifact types without loading the full barrel.
— qwen3.7-max via Qwen Code /review
| artifactSnapshot !== undefined && | ||
| artifactSnapshotUnavailable === undefined; | ||
| const artifactRestoreWarnings = | ||
| artifactSnapshotUnavailable !== undefined |
There was a problem hiding this comment.
[Critical] Two rewind edge cases cause silent data loss:
-
Undefined snapshot wipes artifacts: When both
artifactSnapshotandartifactSnapshotUnavailableareundefined(version skew),restore(undefined, { preserveLiveEphemeral: true })clearsthis.artifactsand only re-adds ephemeral artifacts. All restorable/pinned artifacts are silently wiped. -
JSONL divergence: When
artifactSnapshotUnavailableis set,restore()is skipped (in-memory retains pre-rewind state), but the JSONL has already been truncated by rewind. On daemon restart, the JSONL rebuild produces fewer artifacts → permanent data loss.
| artifactSnapshotUnavailable !== undefined | |
| const artifactRestoreWarnings = | |
| artifactSnapshotUnavailable !== undefined | |
| ? [ | |
| `artifact snapshot rebuild unavailable during rewind: ${artifactSnapshotUnavailable}`, | |
| ] | |
| : artifactSnapshot !== undefined | |
| ? await entry.artifacts.restore(artifactSnapshot, { | |
| preserveLiveEphemeral: true, | |
| }) | |
| : []; | |
| // Force a snapshot when JSONL divergence risk exists | |
| if (artifactSnapshotUnavailable !== undefined) { | |
| await entry.artifacts.recordSnapshot(); | |
| } |
— qwen3.7-max via Qwen Code /review
| } | ||
| if ( | ||
| (snapshot?.artifacts.length ?? 0) > 0 && | ||
| restoredCount === 0 && |
There was a problem hiding this comment.
[Critical] restore() clears this.artifacts unconditionally (line 503), then re-adds artifacts one by one. The rollback guard only fires when restoredCount === 0 — i.e., all artifacts must fail. If 4 of 5 restore successfully and 1 fails, the rollback is skipped and the 1 artifact is permanently lost with only a stderr warning. Compounding this, the rewind handler unconditionally calls recordSnapshot() after partial restore, cementing the degraded state into the JSONL.
| restoredCount === 0 && | |
| if ( | |
| (snapshot?.artifacts.length ?? 0) > 0 && | |
| restoredCount < (snapshot?.artifacts.length ?? 0) && | |
| warnings.length > warningCountBeforeRestore | |
| ) { | |
| warnings.push( | |
| `restored ${restoredCount}/${snapshot?.artifacts.length} artifacts; ${ | |
| (snapshot?.artifacts.length ?? 0) - restoredCount | |
| } failed validation`, | |
| ); | |
| if (restoredCount === 0) { | |
| this.restoreState(previousState); | |
| } | |
| } |
Also gate recordSnapshot in the rewind handler on restoreOk.
— qwen3.7-max via Qwen Code /review
| } | ||
| if (kind === 'event') { | ||
| await recording.recordSessionArtifactEvent( | ||
| payload as SessionArtifactEventRecordPayload, |
There was a problem hiding this comment.
[Critical] The payload is cast to SessionArtifactEventRecordPayload / SessionArtifactSnapshotRecordPayload using as without validating required fields (v, sessionId, sequence, recordedAt, changes/artifacts). Malformed records are silently persisted to the JSONL and later skipped during rebuild, causing data loss on session restore.
| payload as SessionArtifactEventRecordPayload, | |
| // Add structural validation before persisting | |
| if ( | |
| typeof (payload as Record<string, unknown>).v !== 'number' || | |
| typeof (payload as Record<string, unknown>).sessionId !== 'string' || | |
| typeof (payload as Record<string, unknown>).sequence !== 'number' | |
| ) { | |
| throw new Error('sessionArtifactsPersist: payload missing required fields'); | |
| } |
— qwen3.7-max via Qwen Code /review
|
|
||
| const event = normalizeEventPayload(payload, []); | ||
| if (!event) return payload; | ||
| const remappedArtifactIds = new Map<string, string>(); |
There was a problem hiding this comment.
[Critical] remappedArtifactIds is scoped per-call to remapSessionArtifactPayloadForFork and doesn't survive across event records. When an artifact is created in event record N and removed in record N+1, the remove in N+1 can't find the original id in its empty map and falls back to a synthetic id. During rebuildSessionArtifactSnapshot, artifacts.delete(syntheticId) is a no-op (the artifact lives under the remapped id), so the artifact is resurrected in the forked session.
Suggested fix: Build the remappedArtifactIds mapping across all event records before remapping them. Either a two-pass approach (first pass collects all originalId → remappedId mappings, second pass remaps removals using the complete map), or accumulate the map in the caller (forkSession).
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
Implements V2 daemon session artifact persistence as a follow-up to #5895. The PR now restores restorable artifact metadata across daemon restart/session replay, adds durable tombstone and snapshot handling, supports explicit pin/unpin content retention through a managed content store, and keeps the original live-session V1 behavior for ephemeral artifacts.
It also hardens the persistence boundary: managed content is stored under content-addressed IDs with manifest validation, sha256 verification, quota enforcement, atomic manifest writes, best-effort GC, malformed contentRef rejection during restore, and cleanup on session close. Fork/load behavior intentionally keeps pinned content from leaking into remapped sessions.
Why it's needed
V1 artifact APIs exposed only live-session state, so page refresh could recover from a live daemon but daemon restart or historical load could not restore artifact metadata. V2 gives clients a stable metadata restore path while keeping content retention explicit and bounded by security, quota, hash, manifest, and GC rules.
Reviewer Test Plan
How to verify
Review that ephemeral artifacts still remain live-only, restorable artifacts survive JSONL rebuild/snapshot restore, explicit remove tombstones prevent deleted artifacts from coming back, pin/unpin transitions preserve the intended retention state, content retention is only available for valid workspace files, malformed persisted content references are ignored, and close-session cleanup does not leave managed content behind.
Evidence (Before & After)
Before: artifact state was daemon-memory-only and the PR contained only a design document. After: daemon sessions persist artifact metadata/events/snapshots and can retain explicitly pinned workspace content with quota, hashing, manifests, and garbage collection.
Tested on
Environment (optional)
Validated locally with
cd packages/acp-bridge && npx vitest run src/sessionArtifacts.test.ts,cd packages/core && npx vitest run src/services/session-artifact-persistence.test.ts,cd packages/acp-bridge && npm run typecheck,cd packages/core && npm run typecheck,npx prettier --check docs/design/daemon-session-artifacts/session-artifacts-persistence-v2-design.md packages/acp-bridge/src/sessionArtifactContentStore.ts packages/acp-bridge/src/sessionArtifacts.ts packages/acp-bridge/src/sessionArtifacts.test.ts packages/acp-bridge/src/bridge.ts packages/core/src/services/session-artifact-persistence.ts packages/core/src/services/session-artifact-persistence.test.ts,git diff --check, andnpm run build.Risk & Scope
Linked Issues
Follow-up to #5895.
中文说明
What this PR does
这个 PR 在 #5895 之后实现 V2 daemon session artifact 持久化。现在会在 daemon 重启或 session replay 后恢复 restorable artifact metadata，记录 durable tombstone 和 snapshot，支持通过 managed content store 显式 pin/unpin 内容保留，同时保持 ephemeral artifact 的 V1 live-session 语义。
实现也补齐了安全和稳定性边界：managed content 使用 content-addressed ID、manifest 校验、sha256 校验、quota、原子 manifest 写入、best-effort GC、恢复时丢弃非法 contentRef，并在 close session 时清理托管内容。fork/load 场景不会把 pinned content 泄露到 remap 后的新 session。
Why it's needed
V1 artifact API 只有 live-session 状态：页面刷新且 daemon 还活着时能恢复，但 daemon 重启或历史 load 后不能恢复 artifact metadata。V2 提供稳定的 metadata restore，同时把 content retention 设计成显式、受安全边界和配额控制的能力。
Reviewer Test Plan
How to verify
请重点检查：ephemeral artifact 是否仍然只存在于 live session；restorable artifact 是否能通过 JSONL rebuild/snapshot restore 恢复；显式删除的 tombstone 是否阻止旧 artifact 回来；pin/unpin 状态转换是否符合预期；content retention 是否只接受合法 workspace 文件；非法持久化 contentRef 是否会被忽略；close session 是否会清理 managed content。
Evidence (Before & After)
Before：artifact state 只在 daemon 内存中，且 PR 只有设计文档。After：daemon session 会持久化 artifact metadata/events/snapshots，并能在 quota、hash、manifest、GC 约束下保留显式 pinned workspace content。
Tested on
Environment (optional)
本地已验证：
cd packages/acp-bridge && npx vitest run src/sessionArtifacts.test.ts、cd packages/core && npx vitest run src/services/session-artifact-persistence.test.ts、cd packages/acp-bridge && npm run typecheck、cd packages/core && npm run typecheck、npx prettier --check docs/design/daemon-session-artifacts/session-artifacts-persistence-v2-design.md packages/acp-bridge/src/sessionArtifactContentStore.ts packages/acp-bridge/src/sessionArtifacts.ts packages/acp-bridge/src/sessionArtifacts.test.ts packages/acp-bridge/src/bridge.ts packages/core/src/services/session-artifact-persistence.ts packages/core/src/services/session-artifact-persistence.test.ts、git diff --check、npm run build。Risk & Scope
Linked Issues
#5895 的后续实现。