Stage 1: Gate (re-run #3)

Fresh triage against commit 096b217d.

Template looks good ✓ — all required headings present, bilingual, test plan with OS matrix.

Problem: observed and well-documented. V1 artifact APIs were live-session-only — daemon restart or historical session load lost all artifact metadata. This is a genuine gap following up on #5895.

Direction: aligned with the roadmap. Metadata persistence across daemon restarts and session load/replay is a natural next step after V1 live-session artifacts. Content retention was correctly split into a separate future PR (#6346). The design doc now explicitly defers sidecar cache, pin/save, and content archive to future iterations.

Size: approximately 3,627 production logic lines across core paths (packages/core/src/services/*, packages/acp-bridge/src/*, packages/cli/src/serve/*, packages/sdk-typescript/src/*), 3,957 test lines, and an 893-line design doc. Since this is a feat-type PR touching core infrastructure, it's not subject to the hard block. The large PR advisory (1000+ lines) applies — this is a substantial feature, but well-organized across clear module boundaries.

Approach: the scope feels well-calibrated. The JSONL-only approach (no sidecar cache) keeps the implementation focused. The design doc comprehensively maps to the implementation. The ~30 hardening commits since the maintainer's design review have addressed all 8 critical/suggestion comments:

• contentRef validation: normalizeContentRef() validates kind/contentId pattern/sha256/sizeBytes; legacy contentRef is stripped on restore, not exposed as openable content
• Sidecar validity: N/A — sidecar deferred to future work (§4.1)
• Durable-first semantics: eviction and unpin write durable remove events; explicit DELETE uses live-first with best-effort tombstone and warning (§5.3)
• file:// URL restore rule: file: allowed only for trusted published artifacts, validated via normalizeArtifactUrl(raw, allowFile) (§5.5)
• Client POST retention: clients can request ephemeral or restorable; pinned is correctly rejected (§6.2)
• Race-safe workspace access: O_NOFOLLOW open, pre/post-open stat comparison (isSameFile), stream-based hashing (§7.2)
• URL/metadata privacy: hasSecretLikeUrlComponent and credential rejection on URLs; prototype key filtering and secret-key denylist on metadata (§7.3)
• Live cap vs persisted cap alignment: both aligned at 200; durable eviction writes eviction remove events; restore prunes via same rules (§8.1)

One note: the stray .qwen/skills/triage/references/pr-workflow.md change (1 line) is unrelated — minor, but still worth splitting out.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Stage 2: Code Review + Test (re-run #3)

Code Review

Independent proposal: for daemon session artifact metadata persistence, I would add JSONL system records (session_artifact_event / session_artifact_snapshot) to the existing chat recording service, a core persistence types/normalization/rebuild module, extend the SessionArtifactStore with a persistence hook, integrate restore into the daemon-side bridge createSessionEntry, and add SDK/REST types for the new optional fields. Workspace integrity would use stat-gated sha256 checks.

The PR's approach matches this closely. The implementation is split cleanly between packages/core (types, normalization, rebuild, chat recording integration) and packages/acp-bridge (store persistence hook, workspace integrity, fork remapping, cross-client authorization). The operation queue serializes mutations. State clone/restore provides atomicity. Fork remapping correctly recalculates artifact IDs and downgrades pinned → restorable.

The ~30 hardening commits since the maintainer's design review addressed all 8 critical comments:

• contentRef validation: normalizeContentRef() enforces kind=managed_copy, contentId hex-64-hex-16 pattern, sha256 hex-64 pattern, non-negative sizeBytes. Legacy contentRef is stripped during restore — never exposed as openable content.
• Durable-first semantics: eviction writes durable remove event with reason: "eviction". Explicit DELETE is live-first with best-effort tombstone — tombstone failure returns warning. remove() rolls back live state on persistence failure when persistenceStrict.
• file:// URL restore rule: normalizeArtifactUrl(raw, allowFile) gates file: protocol on trusted publisher status. Restore path passes allowFile = artifact.storage === 'published' && !isFileArtifactUrl(artifact.url).
• Client POST retention: normalizeRetention accepts ephemeral or restorable, rejects pinned with validation error.
• Race-safe workspace access: getWorkspaceStatus opens with O_RDONLY | O_NOFOLLOW, compares pre-open lstat with post-open fstat via isSameFile(dev, ino), hashes via stream, and detects symlink escapes.
• URL/metadata privacy: hasSecretLikeUrlComponent scans query params and fragment for token/secret/password patterns. URL userinfo is rejected. Metadata prototype keys (__proto__, constructor, prototype) are filtered. Secret-like metadata values are checked.
• Live cap alignment: both live and persisted caps are 200. Restore calls evictOverflow which writes eviction remove events for durable artifacts.

Critical findings: none.

Reuse check: the persistence module reuses the existing ChatRecordingService infrastructure for JSONL writes, the exported stableSessionArtifactId, and the existing operation queue pattern in SessionArtifactStore. No unnecessary parallel utilities.

Convention check: ESM, strict TypeScript, Prettier formatting, kebab-case file names, tests co-located. All consistent with project conventions.

Test Results

All targeted test suites pass:

✓ packages/core/session-artifact-persistence.test.ts  —  20/20 passed (16ms)
✓ packages/acp-bridge/sessionArtifacts.test.ts        —  83/83 passed (100ms)
✓ packages/acp-bridge/bridge.test.ts                  — 345/345 passed (3432ms)
✓ packages/core/sessionService.test.ts                —  93/93 passed (110ms)
✓ packages/core/chatRecordingService.test.ts          —  38/38 passed (41ms)
✓ packages/sdk-typescript/acpRouteTable.test.ts       — 101/101 passed (15ms)
✓ packages/cli/acpAgent.test.ts                       — 198/198 passed (8639ms)
✓ packages/cli/server.test.ts                         — 603/603 passed (10793ms)

Total: 1,481 tests, all passing.

Typecheck clean for both packages/core and packages/acp-bridge — no errors.

Real-Scenario Testing

This PR adds internal daemon infrastructure (artifact metadata persistence via JSONL records). The changes are not user-visible in the CLI output — there's no TUI change to capture with tmux. The 1,481 unit tests comprehensively cover the persistence semantics: durable events, snapshots, tombstones, sticky ephemeral overrides, workspace integrity (available/missing/changed), cross-client authorization, URL secret filtering, prototype metadata protection, fork remapping, snapshot backoff, restore failure rollback, and eviction event durability.

— Qwen Code · qwen3.7-max

Stage 3: Reflection + Verdict (re-run #3)

Stepping back to look at the whole picture.

This PR has been through an extraordinary review arc — a maintainer design review that found 8 substantive issues (7 critical, 1 suggestion) on the design doc, followed by ~30 hardening commits that comprehensively addressed every one of them. The design doc was rewritten to match the actual codebase architecture and now serves as a detailed contract between the implementation and the expected behavior.

My independent proposal from Stage 2a aligns closely with what the PR implements. The approach is sound — JSONL system records for journal/snapshot integrated into the existing chat recording service, core normalization module with thorough input validation (prototype key filtering, symlink containment, metadata budget enforcement, content ID pattern validation, secret-like URL component detection), persistence hook on the store with operation queue serialization, proper fork remapping, and correct daemon-side restore seeding at createSessionEntry. I don't see a materially simpler path that would cover the same ground.

The code quality is high: clean separation between core types and store integration, defense-in-depth input validation, atomic state transitions with clone/restore for error recovery, exponential backoff for snapshot failures, O_NOFOLLOW + fstat revalidation for race-safe workspace access, and proper eviction event durability for cap alignment.

1,481 tests pass across 8 test suites, typecheck is clean for both core and acp-bridge. The test coverage is deep — tombstones, sticky ephemeral overrides, eviction events, fork remapping, corrupt record handling, cross-client authorization boundaries, prototype pollution filtering, workspace integrity three-state checks, snapshot backoff, restore failure rollback, and URL secret filtering.

The main reservation remains scope: ~3,627 production lines across 4 packages. But the changes are well-organized, each file has a clear purpose, and the design doc provides the architectural map. Content retention was correctly deferred to a future PR, which shows good scope discipline.

One concern worth noting: the maintainer's CHANGES_REQUESTED review from July 3 has not been explicitly dismissed or responded to in review comments. The author addressed the feedback through code changes, but a direct response to the maintainer's review thread would help close the loop. This is a process note, not a code concern — all 8 issues are addressed in the implementation.

This is a well-executed feature addition that solves a real problem in the daemon session model. The maintainer's design review feedback has been comprehensively addressed through both code and design doc updates. Ready to ship. ✅

— Qwen Code · qwen3.7-max

Design review — checked against the V1 implementation on main

Thorough doc, and most of its V1 claims verified accurate against the code (live-store cap 200, 4096-byte metadata limit, workspace status batch 20 + TTL, status: available|missing, artifact_changed event name, operationQueue serialization, upsertMany()/remove(), ChatRecord.subtype/systemPayload extensibility, /branch copying the active record chain with per-record sessionId rewrite, the chatRecording setting gate). However, three claims about the existing code are wrong — two of them on load-bearing parts of the design (the restore integration point and the sidecar location) — plus five spec-internal issues. Line links point to this PR's file.

Where the doc mis-describes current code

1. §5.3 step 6 / §9 Milestone C — restore seeding targets the wrong process (L283, L615)
The doc seeds SessionArtifactStore from AcpAgent.createAndStoreSession(). No class AcpAgent exists (it's class QwenAgent, packages/cli/src/acp-integration/acpAgent.ts:2572), but the substantive problem is the process: the only production new SessionArtifactStore(...) is daemon-side — packages/acp-bridge/src/bridge.ts:2268 inside createSessionEntry — and GET /session/:id/artifacts reads exactly that store (packages/cli/src/serve/routes/session.ts:526 → bridge.getSessionArtifacts). The ACP child process where createAndStoreSession runs has zero references to SessionArtifactStore, so a seed implemented as written populates nothing the HTTP API can see. The existing child→daemon pathway (qwen/notify/session/artifact-event) can't substitute as-is either: handleArtifactEvent stamps source:'hook' and treats input as fresh ingestion (V2 fields like restoreState wouldn't round-trip), and it drops events until the SessionEntry is registered — which only happens after loadSession resolves. Suggest specifying the seed at the bridge (createSessionEntry / restore completion), or an explicit post-registration snapshot notification.

2. §4.3 — sidecar path is modeled on the legacy chats location (L176)
~/.qwen/tmp/<project_hash>/chats/<sessionId>.artifacts.json assumes chat JSONLs live under tmp/<hash>/chats/. They don't: the writer is packages/core/src/services/chatRecordingService.ts:627 → storage.getProjectDir() + 'chats', and getProjectDir() is ~/.qwen/projects/<sanitized-cwd> (packages/core/src/config/storage.ts:313-320, keyed by sanitized cwd, not a hash); tmp/<hash> is getProjectTempDir() (tool-results etc.). The tmp/<project_id>/chats/ strings survive only in stale comments (sessionService.ts:269, chatRecordingService.ts:476, sessionStorageUtils.ts:22) — the doc appears to have copied one. Consequence: sidecars would be written/validated in a directory the session loader never reads, and §4.3's "move together with the chat JSONL on archive" premise breaks. The projectHash cache key (L186/L200) inherits the same outdated model.

3. §5.4 — the http/https-only restore rule blocks every locally-published artifact (L293)
V1 deliberately allows file: URLs for trusted published artifacts (packages/acp-bridge/src/sessionArtifacts.ts:1241 normalizeArtifactUrl(raw, allowFile); call site :392 passes trustedPublisher), the default publisher returns file:// URLs (packages/core/src/tools/artifact/local-publisher.ts:40 pathToFileURL(...); local is the default), and url is the only locator for storage:'published' (validateLocator, sessionArtifacts.ts:1017-1027). Since §2.2 defaults published artifacts to restorable, every locally-published artifact would restore blocked/unopenable under §5.4 as written. Compounding: §4.2 forbids persisting trustedPublisher, so the restore path has no signal left to re-allow file: even after publisher-manifest re-validation — and §7.3's "不保存 host 绝对路径" is itself in tension with persisting file:///... URLs. Needs an explicit rule: allow file: when publisher-manifest trust re-validates, or rewrite published URLs to daemon-served ones.

Spec-internal issues

4. §7.1/§6.5 — session_owner and "用户确认" aren't mintable in the daemon's actual auth model (L452)
The daemon's HTTP trust boundary is one shared bearer token (run-qwen-serve.ts:1019; the artifact REST routes gate on global mutate({strict:true}) only). The one per-session primitive that exists, conn.ownsSession, can't ground session_owner: it's connection-lifetime (empty after daemon restart — exactly the persistence scenario), self-mintable by any token holder via session/load (sibling co-ownership is by design), and absent on the REST surface where §6.3's pin endpoint is specified. There is also no mechanism to hold a client-initiated REST mutation pending human confirmation (the permission-vote flow only covers agent-initiated tool calls during a prompt turn). As specified, the owner tier collapses to "holds the token". The doc should define how session_owner is established and persisted across restarts (a durable per-session capability minted at creation? an explicit downgrade to token-holder?) and what 用户确认 means mechanically for headless clients.

5. §6.2 + Milestone D vs §2.2 — the POST retention whitelist excludes the mandated ephemeral opt-out (L367, L621)
§6.2 "client 不能请求 pinned，只能请求 restorable" and Milestone D "POST /session/:id/artifacts 支持 retention: "restorable"" both enumerate {restorable} as the whitelist — but §2.2 (L56) requires background clients to explicitly request retention: "ephemeral" on that same endpoint (plus an SDK ephemeral helper), and §2.3 (L74) requires honoring explicit retention. An implementer deriving request validation from §6.2/Milestone D rejects exactly the request §2.2 mandates. Fix both spots to "ephemeral 或 restorable".

6. §6.4 — unpin's downgrade parameter is undefined, and unpin→ephemeral resurrects on next load (L411)
The target ("restorable 或 ephemeral，由请求参数决定") has no body/query schema (contrast §6.3's {"mode": ...}). Worse, by the doc's own rules — §5.1 journals only restorable/pinned changes, §8.1 "ephemeral 本来不写 journal", §5.3 replay applies only tombstones, and §4.2 requires tombstones only for deletion (which §6.4 says unpin is not: "不删除 metadata") — an unpin-to-ephemeral leaves the artifact's prior pinned upsert as its last journaled state, so it comes back on the next session load, contradicting §2.2 "ephemeral…不恢复". §4.2's reason:'unpin' enum value is consumed by the default (→restorable) upsert and assigns nothing to the ephemeral variant; §10 tests only the DELETE-tombstone case. Define the parameter, require a tombstone (or an explicit restore-skip rule) for the ephemeral target, and add a test.

7. §8.3 — TTL'd pinned content is excluded from the only expiry GC rule (L540)
§6.3's ttlDays produces pinned content with expiresAt (§3.1: "pinned 默认不设置" = settable), and §8.2 explicitly says pinned content is auto-deleted "除非用户设置 TTL" — but §8.3's only expiry-based rule is "删除超过 expiresAt 的 non-pinned content", which excludes pinned by its own wording, and the other rules all require a tombstone or session delete. Net: ttlDays is a dead parameter and expired pinned content accumulates until QUOTA_EXCEEDED. Add a GC rule for expired pinned content (or route TTL through unpin semantics).

8. §4.3/§4.4 — the sidecar buys almost nothing by the doc's own analysis, and its two sequence fields are redundant (L174, L205)
§8.5(4) concedes SessionService.loadSession() already parses the entire JSONL and can extract artifact records "在同一轮 parse 中顺手提取" (that is exactly how file_history_snapshot restore ships today, via readAllRecords), and fallback replay is already bounded by the 100-mutation/256 KB snapshot threshold; §4.4 concedes "sidecar 只解决性能，不承载协议正确性". Against that near-zero win, the spec spends: atomic writes, 6-field staleness validation, corruption fallback, archive/unarchive/delete co-movement, orphan-sidecar GC (§8.3), crash-consistency clauses (§8.4), and 3 dedicated tests (§10). Consider shipping V2 JSONL-only and adding the cache later if session load is measured slow. Separately, sequence vs sourceArtifactSequence: the doc admits "正常情况下两者相同" and no scenario in the doc makes them diverge (rebuild and compaction recompute both to the same value); moreover, validating sourceArtifactSequence against the JSONL's actual latest sequence requires parsing the JSONL — which defeats the cache. One field suffices (or drop the sidecar and the question disappears).

Minor

• §3.1 (L100) "缺省按 V1 兼容处理为 live-only" vs the restorable defaults in §2.2/§2.3/§6.1: these coexist only if L100 is reader-side (interpreting V1-era records) while §2.2 governs ingest — the doc never states that scoping, and never requires V2 writers to materialize retention in §4.2 journal records (which reuse the same optional shape), so a field-less V2-written record would legitimately be read as live-only and dropped from restore. One clarifying sentence fixes it.
• §4.5/§8.1: content gets a per-project cap but persisted metadata has no cross-session bound, and each 100-mutation snapshot appends another ≤1 MB copy into the same JSONL — the replay-cost mitigation grows the very file §4.5 admits "可能继续增长". Worth one sentence stating the accepted policy (metadata rides the chat-transcript lifecycle) or adding a journal-bytes ceiling.

Also for the record: doc language, directory placement, and formatting are all consistent with existing docs/design/ conventions, and the file passes prettier.

Addressed the review feedback in commits 9b3a00ca1 and c376e1ff8.

Summary:

• Fixed the restore integration point: the design now seeds SessionArtifactStore on the daemon-side bridge (createSessionEntry / restore completion), not in the ACP child process.
• Removed sidecar from the V2 design. V2 is now JSONL-only; sidecar is explicitly deferred until measured load cost requires a separate cache design. This makes the stale sidecar path/cache-validation comments no longer applicable.
• Preserved the V1 published-artifact exception: ordinary external_url stays http/https-only, while trusted manifest-validated published artifacts may restore file: locators when they resolve to daemon-managed storage.
• Reworked auth language to match the current bearer-token route model; no unmintable durable session_owner is assumed in V2.
• Allowed Client POST to request ephemeral or restorable, rejecting only pinned on that endpoint.
• Defined unpin body semantics and required tombstone/restore-skip behavior for unpin-to-ephemeral.
• Added durable-first/rollback semantics for DELETE, eviction, unpin-to-ephemeral, and restore_pruned tombstones.
• Added contentRef ingest/restore validation through daemon-managed manifests with size/hash checks.
• Added race-safe content-copy requirements, secret-bearing URL/metadata handling, V1-to-V2 live backfill, aligned persisted metadata cap with live cap, fork pinned-content downgrade, expired pinned-content GC, JSONL sequence/snapshot/tombstone invariants, observability, and expanded test coverage.

Validation run locally:

npx prettier --check docs/design/daemon-session-artifacts/session-artifacts-persistence-v2-design.md
git diff --check
awk '/```/{c++} END{if (c%2==0) print "fences_ok", c; else print "fences_bad", c}' docs/design/daemon-session-artifacts/session-artifacts-persistence-v2-design.md

All review threads have been replied to and resolved. reviewDecision may remain CHANGES_REQUESTED until a reviewer re-reviews.

Synced #6259 with latest origin/main in 7440b0806 using a normal merge commit.

Merge notes:

• origin/main advanced to 880b06ed4.
• The only manual conflict was packages/sdk-typescript/scripts/build.js, where both sides updated the daemon browser bundle size budget. I kept both SDK surface notes and set the combined budget to 140KB.
• No artifact persistence semantics, recovery behavior, security boundary, or API contract conflict required manual design judgment.

Validation run locally:

• npx prettier --check docs/design/daemon-session-artifacts/session-artifacts-persistence-v2-design.md packages/sdk-typescript/scripts/build.js
• git diff --check
• cd packages/sdk-typescript && npm run build
• cd packages/core && npx vitest run src/services/session-artifact-persistence.test.ts src/services/sessionService.test.ts
• cd packages/acp-bridge && npx vitest run src/sessionArtifacts.test.ts src/bridge.test.ts -t "artifact snapshots|periodic snapshots|rewind|tombstone"
• cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts

Pushed normally to the fork branch and upstream helper branch; no force push.