feat(serve): persist prompt terminal ledger for cold-load reconciliation - #9426
Conversation
|
Thanks for the PR — this is a well-framed one, design doc included. Template: complete ✓ Problem: the gap is real and verifiable in code rather than theoretical: turn terminals are synthesized in memory and published over SSE only, cold-load replay re-emits chunk-class events through the agent subprocess and never a terminal, so an orchestrator mediating prompts by id genuinely cannot resolve a prompt that was in flight when the daemon died. No linked issue or field report (the PR says N/A) — this reads as serve-contract completion rather than an observed user bug, and the load-bearing assumption is that external orchestrators actually rely on the "terminal for exactly this promptId" contract across daemon restarts. Direction: aligned — daemon reliability/observability, additive and backward-compatible response shape, privacy-conscious record format. Flagging per policy: this touches the serve HTTP response contract, the session storage layout, and Size: ~529 production-logic lines (bridge 112, serve ledger module 157, serve routes 37, run-qwen-serve 23, core Approach: the reuse ladder is solid — the existing turn-interruption classifier, the existing shutdown terminal flush, the existing sidecar convention ( Risk: no high-risk-path matches from the revert-history signal. Review depth: full Stage 2 with CI evidence, given the core-path touch. Moving on to code review. 🔍 中文说明感谢贡献——这个 PR 写得很清晰,还附带了设计文档。 模板: 完整 ✓ 问题: 这个缺口是真实存在且可以从代码中验证的,不是理论性的:turn 终态事件只在内存中合成并通过 SSE 发布;冷加载回放只通过 agent 子进程重新发出 chunk 类事件,从不发出终态事件。因此按 promptId 中介 prompt 的外部编排器,确实无法解析 daemon 挂掉时仍在执行中的 prompt。没有关联 issue 或实际使用中的报告(PR 中写 N/A)——这更像是 serve 契约的补全,而不是观测到的用户 bug;其关键假设是:外部编排器确实依赖"恰好这个 promptId 的终态事件"这一契约跨越 daemon 重启。 方向: 对齐——daemon 可靠性/可观测性方向,响应结构向后兼容(纯增量、可选字段),记录格式注意了隐私边界。按策略提示:此 PR 触及 serve HTTP 响应契约、会话存储布局以及 规模: 约 529 行生产逻辑代码(bridge 112、serve ledger 模块 157、serve 路由 37、run-qwen-serve 23、core 方案: 复用做得扎实——复用了现有的 turn-interruption 分类器、现有的 shutdown 终态 flush、现有的 sidecar 约定( 风险: revert 历史信号的高风险路径无命中。鉴于触及 core 路径,审查深度:完整 Stage 2 + CI 证据。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed against What holds up well. The layering is right: the bridge only writes, through an injected One real correctness question — attribution with a queued backlog. The One inherited limitation worth surfacing in the design doc. Minor notes, non-blocking. (1) The design doc credits sequenceDiagram
participant P1 as Orchestrator
participant P2 as Serve load route
participant P3 as Bridge
participant P4 as Ledger sidecar
participant P5 as Transcript
P1->>P2: send prompt, daemon alive
P2->>P3: sendPrompt
P3->>P4: append in_flight at admission
P3->>P4: append terminal at settle or flush
Note over P3: daemon dies with a prompt in flight
P1->>P2: POST session load, cold
P2->>P3: loadSession
P3-->>P2: restored, not attached, no active prompt
P2->>P4: read records, find dangling ids
P2->>P5: classify transcript tail
P5-->>P2: clean or interrupted
P2->>P4: append reconstructed verdict
P2-->>P1: response with promptTerminals
Files changed (15)
Testing evidence — the PR's own CIThis is an unattended CI run, so per policy nothing was built or executed here; the evidence below is the PR's own CI on the reviewed commit, fetched through the API. No red checks at review time, but the primary suites had not landed yet — the Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Not verified: the live daemon kill/restart flow itself — this pass is static review plus the PR's own CI. The unit suite pins the designed attribution behavior, including the multi-dangling case questioned above, so a green suite cannot settle whether that behavior is the right one. Sandboxed verification would settle this: 中文说明代码审查针对提交 扎实的部分。 分层正确:bridge 只负责写入,通过注入的 一个真实的正确性问题——排队积压时的归属。 一个值得在设计文档中披露的继承性限制。 次要备注(不阻塞)。(1)设计文档把单飞对账归功于 测试证据。 本次为无人值守 CI 运行,按策略未构建或执行任何 PR 代码;证据来自 API 拉取的该提交自身 CI。审查时无红色检查,但主要套件尚未出结果—— — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — a well-engineered feature whose main path I trust, but two unresolved edge-case correctness questions and the core-path size escalation mean a maintainer should make the call, not the gate. Stepping back: my independent proposal for this problem was "reuse the turn-interruption classifier on the transcript tail at cold load and persist the verdict in a sidecar" — the PR matches it and goes one step further in a way that earns its cost: the live admission/terminal ledger is what preserves the cause (shutdown vs channel loss vs kill) and the admission evidence that makes any attribution defensible at all; transcript-only reconstruction could not do that. The discipline is what I want to see in this area: fail-closed intent everywhere, no prompt content in records, byte-identical response shape for pre-ledger sessions, synchronous appends so the shutdown flush lands before process exit, and roughly two lines of test for every line of production code. If I had to maintain this in six months, the module boundaries (bridge writes through an injected seam, serve layer owns layout and reads) would make it easy to reason about. My reservations are the two from the review, and I don't want to paper over them: the queued-backlog attribution can produce a CI was still in flight at review time (primary ubuntu suite and Serve A/B pending; macOS/Windows matrix skipped on this commit), which alone would defer an approval — but the verdict here is escalation, not deferred approval, so no approve-on-green marker is attached. ⏸️ Deferring to the core code owners — @wenshao @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC (per CODEOWNERS for 中文说明置信度:3/5 —— 主路径可信、工程质量扎实的功能,但存在两个未解决的边界场景正确性问题,且触及 core 的规模已触发升级策略,应由维护者拍板,而不是由门禁决定。 回顾一下:我对这个问题的独立方案是"冷加载时复用 turn-interruption 分类器判断转录尾部,并把判定持久化到 sidecar"——这个 PR 与之一致,并且多走了一步且这一步物有所值:实时的准入/终态账本保留了原因(shutdown、通道丢失还是 kill)以及使归属判断站得住脚的准入证据;仅靠转录重构做不到这一点。这个领域该有的纪律都在:处处 fail-closed 的意图、记录中不含任何 prompt 内容、旧会话响应结构逐字节不变、同步追加保证 shutdown flush 在进程退出前落盘、测试代码约为生产代码的两倍。如果六个月后由我维护,模块边界(bridge 通过注入接口写入、serve 层拥有布局与读取)会让它易于推理。 我的保留意见就是审查中的那两点,不想掩饰:排队积压时的归属可能给一个从未运行的 prompt 产生 审查时 CI 仍在进行(ubuntu 主套件与 Serve A/B 待定;macOS/Windows 矩阵在该提交上为 skipped)——仅此一点就足以推迟批准;但这里的结论是升级人工处理,而非推迟批准,因此不附带 approve-on-green 标记。 ⏸️ 转交 core 代码负责人 —— @wenshao @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC(依据 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.13)
doudouOUC
left a comment
There was a problem hiding this comment.
Review of PR #9426 — feat(serve): persist prompt terminal ledger for cold-load reconciliation
Reviewed commit: 943eaf5bbfa021d8f39b61dd7227da0dc7b4b16a
Overall assessment
No new blockers found. The PR is well-engineered: the design doc is thorough, the fail-closed invariants are correctly implemented, the test coverage is comprehensive, and the cross-file integration is sound.
This is a re-review after the previous round found 6 Criticals and 11 Suggestions. All 6 Criticals were addressed by the author in commit 943eaf5bbf with the described fixes, and the review agents have verified the fixes against the current code. The 11 Suggestions were also addressed.
Previous blockers re-check
All 6 previous Criticals have been verified as fixed:
-
Cold-load reconciliation / false completed (comment 3807690282) — FIXED. The attribution guard now skips settled in_flight records, and the temporal evidence check (
lastWriteMs >= targetAdmission.at) prevents attributing an earlier turn's tail to a queued-but-never-started prompt. -
id-less functionCall tail (comment 3807690287) — FIXED.
tailHoldsAnyFunctionCallinspects the lastapiHistoryentry for ANY functionCall part (id or not), and upgrades the verdict tointerrupted/daemon_lost. -
Settled queued prompt guards (comment 3807690303) — FIXED. The guard now skips the
in_flightrecords of prompts that later settled (a terminal exists for their id) and requires the last unsettledin_flightto be the target's own admission. -
Test asserts wrong fail-closed invariant (comment 3807690308) — FIXED. The test now expects the fail-closed outcome (nothing appended) for the ambiguous pair, and 'newest gets the verdict' coverage moved to the attribution-valid interleaving.
-
Ledger not deleted on session removal (comment 3807690318) — FIXED.
removeSessionFilesnow deletes both ledger states viaremovePromptLedgers. -
Ledger ingested as transcript by insight scanner (comment 3807690327) — FIXED. Both
DataProcessor.scanChatFilesandusageHistoryService.rebuildFromSessionJsonlnow exclude*.ledger.jsonl.
New finding
Suggestion: The usageHistoryService.ts filter change (.ledger.jsonl exclusion) has no test coverage. The existing rebuildFromSessionJsonl tests would pass even if the filter were removed. A future refactor could silently remove it, causing the ledger sidecar to be parsed as a transcript. Consider adding a test in usageHistoryService.test.ts where a .ledger.jsonl file is placed alongside a real session .jsonl and asserting only the session file is included.
Build & Test Status
Build: packages/core (✅) and packages/acp-bridge (✅) compiled successfully. packages/audio-capture failed due to a pre-existing environment issue (missing Python for node-gyp on Windows), unrelated to this PR. Test suite could not run due to the build chain being blocked by the audio-capture dependency.
Conclusion
No critical or blocking issues found. The feature is well-structured, the fail-closed invariants are correct, and the test coverage is thorough. One minor suggestion regarding test coverage for the usageHistoryService.ts filter.
|
Self-audit finding — fixed in 7a10362: hot-path synchronous full-ledger read While auditing this PR I found a performance issue on the per-request hot path. Problem. Fix (7a10362):
Tests: 3 new unit tests for the tail-window contract (fits-in-window, torn-first-line drop, aligned-boundary drop) plus an end-to-end serve test driving a ~290 KiB ledger through |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": verify the transcript persistence path for addHistory 'd reminder entries ( drainPendingAddedMcpToolsReminder / skill & agent reminder drains) — the ceiling s…; chunk 8: none — though note I could not execute the new unit tests (no node_modules / dist in this worktree); verification of them is by static walkthrough only..
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/run-qwen-serve.ts:4354 — [review] Production sink wiring has no test at any levelpackages/acp-bridge/src/bridge.ts:2023 — [probe] kind: 'cancelled' projection branch is untested (mutation survives)packages/cli/src/serve/prompt-terminal-ledger.ts:72 — [probe] Reconcile's unbounded synchronous full-ledger read on every eligible cold loadpackages/cli/src/serve/routes/session.ts:3221 — [probe] Coalesced load waiter reads the ledger before the owner's reconcile appendspackages/acp-bridge/src/bridge.ts:2012 — [probe] Unbounded agent-supplied stopReason persisted into the ledger
中文说明
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 3)":verify the transcript persistence path for addHistory 'd reminder entries ( drainPendingAddedMcpToolsReminder / skill & agent reminder drains) — the ceiling s…;chunk 8:none — though note I could not execute the new unit tests (no node_modules / dist in this worktree); verification of them is by static walkthrough only.。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
chiga0
left a comment
There was a problem hiding this comment.
Review — cold-load prompt terminal ledger
Note on form: prose instead of inline comments. GitHub rejected the inline
anchors with422 Line could not be resolved. All seven lines validate as in-hunk
against this PR's ownfilespatch, and the identical method posted cleanly to
#9391 minutes earlier — the difference is that this PR ismergeable: CONFLICTING,
so there is no computable merge diff to anchor against. Rebasing should restore
inline review. File and line references below are exact.
Tier: Deep — new on-disk format, writer/reader split across packages, and records already written cannot be un-written by reverting the commit.
Blocking, before anything else
This branch does not typecheck, and CI never noticed. readRecentPromptTerminals returns PromptLedgerTerminalRecord[] | undefined (prompt-terminal-ledger.ts:190-193), and prompt-terminal-ledger.test.ts:508 indexes it unguarded — expect(terminals[63]). :514's terminals[0]?.at optional-chains the element, so it still requires terminals to be defined, and toHaveLength(64) does not narrow for TS.
Why CI is silent: mergeable: CONFLICTING, so no merge commit can be built. Seventeen checks that ran on #9391 are simply absent here — Test (ubuntu-latest), Integration Tests, Secret scan, Dependency CVE audit, the whole matrix. The six green checks are all review orchestration (authorize, label, review-pr, precheck-pr, delay-automatic-review, Remind on force-push). A green rollup here is an absence of evidence, not a pass.
Findings
2 blockers · 3 major · 2 minor. Both blockers are in reconcileDanglingPromptTerminals but have different root causes, and each was settled by execution.
Blocker 1 · prompt-terminal-ledger.ts:163
[Blocker] The temporal fence uses queue-admission time, so a prompt that never ran can be attributed another prompt's tail
targetAdmission.at is queuedAt = Date.now() taken at the top of sendPrompt (bridge.ts:8118) — that is enqueue time, not start-of-execution time. Any prompt queued behind a running one therefore carries an at that predates the running prompt's transcript writes, so this guard passes on a tail the queued prompt never produced.
Failure scenario: t0 p1 admitted and runs · t0+1s p2 admitted → queued · p1 writes its turn (last record t0+4s) · t0+5s p1 completes · t0+6s daemon killed. p2 never executed. Cold load → dangling=[p2]; attribution guard passes (p1 is settled); this check passes (t0+4s ≥ t0+1s); p1's clean tail is classified → {promptId:"p2", terminal:"completed", stopReason:"reconstructed_from_transcript"} is appended durably. The orchestrator this feature exists for is told a prompt that never ran completed successfully, and the verdict is idempotent, so every later load re-serves it.
Witness (vitest, this head, real SessionService fixture; probe removed and tree verified clean):
PROBE ledger after reconcile: [ …p1 in_flight, p2 in_flight, p1 completed,
{"v":1,"promptId":"p2","terminal":"completed",
"stopReason":"reconstructed_from_transcript","at":1787130027111} ]
A second shape yields {"terminal":"interrupted","code":"daemon_lost"} for the same non-executed prompt.
This contradicts the design doc's Non-Goal "No reconstruction for queued-but-never-started prompts" and the fail-closed invariant "a wrong terminal is never synthesized". The existing test stays fail-closed when the last transcript write predates the admission only covers the case where the dangling prompt is the sole ledger entry — one preceding settled prompt defeats it.
Direction: fence on a start-of-execution timestamp (persist at from the pending_prompt_started transition, or add startedAt to the in_flight record); or require that no prior settled prompt's terminal at postdates the target's admission.
Blocker 2 · prompt-terminal-ledger.ts:168
[Blocker] The attribution guard and the verdict read two different views of the same transcript
The guard above reads resumed.conversation.messages — the raw ChatRecord list. The verdict reads buildApiHistoryFromConversation(...), which (session-api-history.ts:57,75-84) drops records with no message, drops type:'system' records, and — on a subtype:'chat_compression' record — replaces the entire accumulated history with payload.compressedHistory. So "the transcript's last write postdates the admission" is asserted about a record the verdict is not computed from.
Failure scenario: prompt P admitted and running; model emits an id'd functionCall, tool returns. Mid-turn auto-compaction writes a system/chat_compression record. Daemon is OOM-killed before the post-compaction round produces a message-bearing record. Cold load: dangling=[P], attribution passes, temporal guard passes (raw tail is the compaction record) — but the api-history tail is now compressedHistory, whose last entry is {role:'model',parts:[{text:'Got it. Thanks for the additional context!'}]} (postCompactAttachments.ts:849-851). detectTurnInterruption → {kind:'none'} → a durable completed for a prompt that got no model token after admission.
Witness (real session-api-history.ts + turn-interruption.ts at this head, node --experimental-strip-types):
temporal guard : lastWriteMs >= admission.at -> true (raw tail = system/chat_compression)
api history len : 2 (4 raw records in)
detectTurnInterruption -> {"kind":"none"}
=> ledger record : {terminal:'completed', stopReason:'reconstructed_from_transcript'}
Corroborating the code's own intent: turn-interruption.ts:33-36 states "A model text tail that was truncated mid-stream is indistinguishable from a clean finish … so it classifies as none here." The classifier's only other consumer (session-recovery.ts:168-181) maps none → {kind:'clean', canContinue:false} — "nothing to continue", not "the turn succeeded". This is the first caller to promote none into a positive success terminal.
The route test's fixture uses at: 1 (session-prompt-terminals.test.ts:174), so the temporal guard passes for any transcript timestamp and no test distinguishes the two views.
Major 1 · prompt-terminal-ledger.ts:270
[Major] promptTerminals is emitted by the daemon but never declared on the published SDK type, so the feature's stated consumer cannot read it
DaemonClient.loadSession returns (await res.json()) as DaemonRestoredSession (DaemonClient.ts:3073), and DaemonRestoredSession (sdk-typescript/src/daemon/types.ts:987) has no promptTerminals member. DaemonSession (types.ts:926-959) ends at branch? with no index signature. Every other daemon-added load-response field was declared there — replayDegraded (types.ts:1028), historyAnchorRecordId, eventEpoch — and each also got plumbing in DaemonSessionClient and an entry in docs/developers/qwen-serve-protocol.md. This field got none of the three.
Failure scenario: the PR's rationale is "external orchestrators that mediate prompts by id". Such an orchestrator on @qwen-code/sdk-typescript writes restored.promptTerminals and the build fails; it must as any or hand-redeclare, and there is no exported type for the record shape either (PromptLedgerTerminalRecord lives in @qwen-code/acp-bridge/promptLedger, outside the SDK surface). The value survives at runtime — DaemonSessionClient.load rest-spreads and omitSkillDetailsFromReplayArrays is spread-based — so this is a typed-surface gap, not data loss.
Witness: tsc 5.9.2 --strict against the real types.ts at this head → error TS2339: Property 'promptTerminals' does not exist on type 'DaemonRestoredSession'. Also git grep promptTerminals over the head tree: 12 hits, all under packages/cli/src/serve/** plus the design doc; zero in packages/sdk-typescript/, packages/webui/, or the protocol doc.
(Two agents reviewing this PR independently reached this finding.)
Major 2 · prompt-terminal-ledger.ts:183
[Major] The reconciled verdict is invisible to the pre-existing per-prompt terminal API, which answers 404 for the same promptId
The premise that terminal facts "were never persisted" holds for the SSE event, not the fact: chatRecordingService.ts:2670 already appends a system/turn_result record carrying promptId, state, stopReason, error{code,message}, startedAt, endedAt; acpAgent.ts:424 findSettledTurnResult scans it back after restart; and GET /session/:id/turns/:promptId (routes/session.ts:6288-6326 → bridge.ts:10653) already answers per-promptId queries from it across restarts.
Failure scenario: daemon dies mid-prompt P → restart → cold POST /session/:id/load reports promptTerminals:[{promptId:P, terminal:"interrupted", code:"daemon_lost"}]. Same daemon, same moment, GET /session/:id/turns/P: findLiveTurnStatus empty, enrichedTerminalPromptIds empty, transcript scan finds no turn_result for P → getSessionTurnStatus returns undefined → 404 prompt_not_found. Two endpoints, one promptId, contradictory answers; an orchestrator polling the documented per-prompt route gets no benefit from this PR.
Secondary cost: terminal:'interrupted' has no counterpart in TurnResultRecordPayload.state, and coercePromptLedgerRecord applies none of the length bounds isTurnResultRecordPayload applies to the same fields — two validators for one concept.
Witness: not run (needs a live daemon plus a real ACP child). Mechanism traced end-to-end: chatRecordingService.ts:2670 → acpAgent.ts:424 → bridge.ts:10653-10745 → routes/session.ts:6288-6326 (404 branch) vs prompt-terminal-ledger.ts:141-160.
Worth deciding explicitly whether the sidecar should be the second source of truth, or whether the reconstructed verdict belongs in turn_result.
Major 3 · routes/session.ts:3229
[Major] All seven reconciliation bail-out paths are silent, including the fail-closed veto — while the same PR's other two failure sinks both log
reconcileDanglingPromptTerminals has five bare returns (unreadable ledger · >1 dangling · attribution veto · loadSession threw · temporal veto) and two catch {} (read, append); this route wraps the call in a third. None logs.
Sibling inconsistency inside this diff: the bridge's ledger-append failure logs (bridge.ts:1984-1990, writeStderrLine), and the archive/unarchive ledger-move failures log (sessionService.ts:1746-1751, 1826-1831, this.warn). This route already has daemonLog in scope and uses it two hunks earlier (daemonLog?.warn('worktree sidecar path failed containment', …), session.ts:3346).
Failure scenario: an orchestrator reports a prompt permanently unknown. The operator has no way to tell which of five causes applied — unreadable ledger, two dangling prompts, attribution veto, loadSession threw, or nothing was dangling. The single most interesting state the design describes, the fail-closed veto, is the one with no diagnostic. The test plan says "only a stderr warning records the ledger failure", which is true of the write path and false of the whole reconciliation path.
Witness: not run (needs a live daemon); established by reading the three sinks in this diff side by side. A single daemonLog?.debug per veto reason would close it.
Minor 1 · prompt-terminal-ledger.ts:79
[Minor] Reconcile reads and parses the entire ledger on the same request whose response read this PR just windowed to 256 KiB
The head commit is "perf(serve): read only the ledger tail for load-response promptTerminals", and readRecentPromptTerminals passes tailBytes: RECENT_TERMINALS_TAIL_BYTES (256 KiB). This read passes no window, so every cold POST /session/:id/load does a synchronous readFileSync + per-line JSON.parse of the whole file on the event loop. The PR states there is no truncation or compaction, and the writer appends 2 records per prompt forever, so it is unbounded.
Concrete cost (probe, since removed): 25 000 prompts → 50 001 records, 5 525 068 bytes; full read 19.6 ms blocking vs tail read 1.0 ms. Linear and unbounded. moveLedgerSidecar also reads the whole file on an archive collision.
Note the full read is correct for dangling detection — a tail window could hide an in_flight record and make a dangling prompt look absent, which is the fail-closed direction — so the fix belongs on the writer (a cap, compaction, or checkpoint record), not a window here. Two open questions the design doc could answer: what bounds this file, and what happens when it stops fitting the 256 KiB response window.
Minor 2 · prompt-terminal-ledger.ts:92
[Minor] This fail-closed bail-out has no test that can fail — deleting the line leaves the suite green
Mutation: delete this line. prompt-terminal-ledger.test.ts + routes/session-prompt-terminals.test.ts → 24 passed (24).
The test named appends nothing when several prompts are dangling passes for a different reason: with 2+ unsettled ids, targetAdmission is always the newest in_flight record while target is dangling[0] (the oldest), so the attribution guard returns first. The listed invariant "Multiple dangling prompts never receive a synthesized terminal" is therefore asserted by a test that cannot distinguish the two guards.
For contrast, the other three guards are genuinely pinned — same suite, same matrix:
| mutation | result |
|---|---|
drop this dangling.length > 1 bail-out |
survived (24/24 green) |
| drop temporal-evidence check | killed |
| drop attribution guard | killed |
drop id-less functionCall upgrade |
killed |
Actionable form: a fixture with two dangling ids where the oldest is also the last unsettled in_flight record — or accept the line is redundant and drop it, rather than documenting it as a load-bearing invariant.
Verification performed
npm ci clean · new reconciliation suite 19 passed, route suite 24 passed · 3 behavioural probes (both blockers) · 4-mutant matrix on the reconciler guards — 3 killed, 1 survived · 1 perf probe (25k-prompt ledger) · tsc --strict against the real SDK types. Working tree verified byte-identical to head afterwards.
Checked and found clean — so you can tell coverage from silence
- Sidecar filename collision: enumerated every
chats/reader.sessionService(5 sites),core/memory/manager.ts:345,vscode-ide-companion/qwenSessionReader.ts:111all gate onSESSION_FILE_PATTERN, which<uuid>.ledger.jsonlfails. The only two loose.jsonlfilters —DataProcessor.ts:1004,usageHistoryService.ts:377— are exactly the two this PR patched. No gap found; the drive-by fixes are correct and complete. - Writer ↔ reader symmetry: field sets, the four
terminalvalues, torn-tail seal,dropFirstLinewindow — symmetric. Caps internally consistent (PROMPT_TERMINALS_RESPONSE_LIMIT=64↔ 256 KiB tail). codeforwarding chainflushPromptTerminals→err:{code}→publishPromptTerminal→promptLedgerTerminalRecord→normalizeTurnResultError().code→ ledger: intact.- Lifecycle: delete (
removePromptLedgers, both archive states) mirrorsremoveWorktreeSidecarsat both call sites; branch/copy deliberately copies the transcript only. No resurrection path found there. - Privacy: the ledger writes only
v/promptId/state|terminal/code/stopReason/at— no prompt text, no tool payloads, no paths. The claim holds. (appendFileSynccreates at0o666 & ~umaskwhile transcripts are explicitly0o600, so "inherits the directory's permissions" is loose — not filed, since the content is ids and enums.) - Write-point completeness: one
SessionEntrysite, onepublishPromptTerminalfunnel with six callers — no path admits or settles a prompt without hitting the ledger.promptLedgeris in theexportsmap, the barrel, and the vitest alias.
Recorded, not filed as defects
v: 1has no forward-compat path.coercePromptLedgerRecordrejectsrecord['v'] !== 1outright, so a futurev: 2record is silently dropped rather than preserved-as-unknown. A mixed-version situation where a prompt'sin_flightis v1 and its terminal v2 makes that prompt look dangling to the older reader — feeding it straight into blocker 1. No v2 writer exists yet, so this is a question: isva hard gate or a compat hint?- Unarchive can invert ledger order.
moveLedgerSidecarappends source after destination; in the unarchive direction that puts the older segment after the newer one, and bothdanglingInFlightPromptIdsandrecentPromptTerminalRecordsdecide supersession positionally while every record carriesat. Reachability needs a prior warn-only move failure — a state the method's own docblock anticipates.
Not covered
mergeable: CONFLICTING — not reviewed against a merged tree. No live-daemon run (blocker-2's 404 divergence and the silent-veto finding are traced, not observed). No mutation matrix against the bridge write points. Windows/Linux path behaviour untested by anyone, and there is no CI to cover it.
Reviewed with AI assistance.
cd42cc8 to
13ac019
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the core (prompt-ledger.ts, prompt-terminal-ledger.ts, sessionService reconciliation).
Approve. The reconciliation is fail-closed where it matters: unreadable ledger → no-op; only a SINGLE dangling in-flight prompt is reconciled (multiple → no-op); degraded transcript → no-op. The ledger sink appends via the session service's ledger path.
0 unresolved threads.
ytahdn
left a comment
There was a problem hiding this comment.
审查总结 / Review Summary
变更概览 / Overview: 19 files, +2724/−3. Adds an append-only prompt terminal ledger sidecar (<sessionId>.ledger.jsonl) per session, recording prompt admission (in_flight) and terminal outcome (completed / cancelled / error / interrupted). On cold-load reconciliation, classifies the transcript tail of a dangling in-flight prompt and persists a verdict so external orchestrators can resolve prompts that were in flight when the daemon died.
🎉 做得好 / Highlights
- 严密的 fail-closed 设计 / Rigorous fail-closed design: Every step that cannot attribute the tail with confidence returns without appending — the prompt stays
unknown. Multi-dangling guard, attribution guard, compression fence, temporal evidence (projection-consistent withSessionApiHistoryAccumulator), FIFO evidence, and the id-less functionCall guard each close a concrete wrong-terminal class. No wrong terminal is ever synthesized. - 分层清晰 / Clean layering:
acp-bridgegains no new core coupling — the ledger sink is injected (PromptLedgerSink), the bridge module stays dependency-free beyondnode:fs, and all writes are best-effort (never throw). Serve-layer reconciliation may import core. - torn-write 处理 / Torn-write handling:
sealTornTailSyncdetects a missing trailing\n(crash mid-append) and seals it before the next record, preventing two records from fusing into one unreadable line. - 设计文档 / Design doc: The design document thoroughly explains each guard with concrete failure scenarios and examples (
[A if, B if, B cancelled],[if p1, if p2, term p1]), making the algorithm auditable without reading the code.
交叉验证 ci-bot Critical 发现 / Cross-validation of ci-bot Critical findings
All 5 Critical findings from the ci-bot review (R1-1 through R1-6) were verified at head 13ac0194e1 and found to be false positives or compensated:
- R1-1 (clean-tail verdict attributed to wrong prompt): Not a bug —
dangling.length > 1bails out, so exactly one target exists when the verdict fires. No oldest/newest ambiguity. - R1-2 (
detectTurnInterruptionreturnsnonefor mid-tool-run kill): Compensated at the call site —tailHoldsAnyFunctionCallupgrades the verdict tointerruptedfor any functionCall tail (with or without id). Test-pinned:'marks a dangling prompt interrupted on an id-less functionCall tail'. - R1-3 (attribution guard misclassifies settled QUEUED prompt): Not reproduced — the guard correctly skips settled prompts'
in_flightrecords. - R1-4 (test asserts opposite of fail-closed): Not reproduced — no test treats a corrupt ledger as valid; all tests reinforce fail-closed.
- R1-5 (ledger not deleted on session delete): Not reproduced —
removePromptLedgersis called at both deletion paths (lines 1654, 1669). - R1-6 (
.ledger.jsonlconfused with transcripts): Not reproduced —SESSION_FILE_PATTERN(/^[0-9a-fA-F-]{32,36}\.jsonl$/) does not match.ledger.jsonl.
🟡 重要问题 / Important (non-blocking)
- I1 — Reconciled
at使用 wall-clock 而非证据时间 / Reconciledatuses reconciliation time instead of evidence time (prompt-terminal-ledger.ts:184,191): Both theinterruptedandcompletedreconciled records useat: Date.now()— the time of reconciliation, not the time the daemon actually died.lastVisibleWriteMs(lines 132–148, already in scope) is a much closer approximation of the actual interruption time. Failure scenario: daemon dies at 10:00, session loaded at 11:00 → reconciled record saysat: 11:00. If a new prompt was admitted at 10:05 and terminated at 10:10, the reconciledat: 11:00sorts after it, breaking temporal ordering. Suggested fix:at: Math.max(lastVisibleWriteMs, targetAdmission.at).
💡 建议 / Suggestions
- S1 —
moveLedgerSidecarmerge path (sessionService.ts:832-839): If the process crashes betweenappendFileSync(line 837) andunlinkSync(line 839), the next archive/unarchive re-appends the same source records, creating duplicates. The window is narrow (two sync calls) and the reader tolerates duplicates (last-write-wins for dangling detection, 64-record cap for terminals), but an idempotent merge (e.g., compare last record before appending) would be cleaner. - S2 — Reconcile gate (
routes/session.ts:3217-3222): The condition!restored.attached && !restored.hasActivePromptskips reconciliation when a client is attached but has no active prompt. A reconnecting client that doesn't know about the previous prompt would miss its terminal. Consider whetherhasActivePromptalone should be the gate.
结论 / Verdict
✅ 批准 / Approve — A carefully designed, well-layered addition with correct fail-closed invariants throughout. The ci-bot's Critical findings are all resolved at the current head. The Important item (reconciled timestamp) is a correctness polish that improves temporal ordering but does not produce wrong verdicts. The five suggestion-level items from ci-bot (R1-7 through R1-16) are non-blocking observations.
doudouOUC
left a comment
There was a problem hiding this comment.
Review — prompt terminal ledger (cold-load reconciliation)
Reviewed commit: 13ac0194e1adb057547196d11437f513e566da12 (verified as current HEAD at review time). Critical: 0 — APPROVE.
I checked out this exact head in an isolated worktree, installed, built, typechecked, and ran the affected suites rather than reading the diff alone. Every claim below was verified against the code at this commit.
First: clearing the standing blocker
The round-4 review posted a hard blocker — "This branch does not typecheck, and CI never noticed" — citing prompt-terminal-ledger.test.ts:508 indexing readRecentPromptTerminals()' possibly-undefined return as terminals[63]. That no longer stands at this head, on both halves of the claim:
- There is no
terminals[indexing anywhere inprompt-terminal-ledger.test.tsany more. npm run build→ exit 0,npm run typecheck→ exit 0, 0 TS errors across all workspaces.
The blocker was also written while the PR was CONFLICTING, which is why 17 checks were absent. The branch is now MERGEABLE and the full matrix ran green against this head, including Test (ubuntu-latest, Node 22.x), Serve A/B (no response drift vs. base), and Real daemon E2E.
One thing that claim got right and is worth recording: npm run typecheck is genuinely not part of the CI Test job (only preflight runs it). Type-only regressions in test files can land undetected. That is a CI gap, not a defect in this PR — this branch typechecks clean.
Local verification
| Suite | Result |
|---|---|
cli prompt-terminal-ledger + session-prompt-terminals + DataProcessor |
79 passed |
acp-bridge prompt-ledger + bridge-prompt-ledger |
20 passed |
core sessionService |
159 passed |
core usageHistoryService |
30 passed |
npm run build / npm run typecheck |
exit 0 / exit 0, 0 errors |
288 tests green. (First attempt failed on unrelated Ink type errors — my own --ignore-scripts install skipping patch-package, not the branch.)
What I verified, and why it holds
1. The sidecar cannot leak into transcript enumeration. This was my main concern: <id>.ledger.jsonl lives in the same chats/ dir as transcripts and ends in .jsonl, and the PR only patched two enumerators. I grepped every .jsonl reader repo-wide and adjudicated all of them:
usageHistoryService.rebuildFromSessionJsonlandDataProcessor.scanChatFiles— the two patched here; both now exclude.ledger.jsonl, andDataProcessor.test.tspins it with a ledger file present in the fixture.sessionServicereaddirSync(chatsDir)× 5 (lines 719, 1195, 1357, 2359, 2456) — four gate onSESSION_FILE_PATTERN, one on exact${sessionId}.jsonlequality. None can match.memory/manager.ts:345defaultSessionScanner— gates on its ownSESSION_FILE_PATTERN. Safe.
No enumerator was missed. And no collision is constructible: SESSION_FILE_PATTERN is /^[0-9a-fA-F-]{32,36}\.jsonl$/, so a session id can contain neither . nor l/g/r, and SessionService throws SessionWriterUnavailableError for ids that fail it — a session whose transcript is named <x>.ledger.jsonl cannot exist.
2. The fail-closed attribution invariant genuinely holds. I walked each guard in reconcileDanglingPromptTerminals and confirmed two properties the guards silently depend on:
- Clock domain.
queuedAt(bridge.ts:8258) and the terminalatare bothDate.now()wall-clock epoch ms, so comparing them againstDate.parse(record.timestamp)from the transcript is sound. A monotonic-clock source here would have broken every temporal guard at once. - Projection parity. The evidence loop's filters (
type === 'system'skip, compression-reset detection,!record.message || subtype === 'realtime_message'skip) mirrorSessionApiHistoryAccumulator/appendApiHistoryRecordexactly, solastVisibleWriteMsis measured on the same projection the verdict runs on.isCompressionResetRecordcorrectly mirrorsisApiHistoryCompressionCandidate.
I could not construct a reachable path that synthesizes completed for a prompt that never ran. The queued-backlog case flagged in triage is now closed by the FIFO guard: for [A in_flight, B in_flight, A completed] the tail predates A's terminal, so lastVisibleWriteMs < lastOtherTerminalAt vetoes attribution to B — and the test asserts the ledger stays at 3 records rather than merely asserting "no crash".
3. Reconciliation runs at most once per cold restore — verified, not assumed. archiveCoordinator.runSharedMany is a shared lock, so I expected concurrent cold loads to double-reconcile. It cannot happen: a coalesced waiter takes the inFlightRestores branch and returns attached: true (bridge.ts ~6790), so the route gate !restored.attached excludes it, and the owner alone reconciles. The does not reconcile an attached load test pins that gate. Worth knowing this invariant rests on the waiter's attached: true, since a future change there would break it silently.
4. No dead switches. Every added field has a real producer and consumer: tailBytes (set by readRecentPromptTerminals), promptLedger sink (injected at all three bridge construction sites, live-conversation deliberately excluded), getPromptLedgerPath, promptTerminals (attached only for action === 'load').
5. Lifecycle and format details. removePromptLedgers is wired at exactly the two sites as the pre-existing removeWorktreeSidecars, so deletion is symmetric with the established sidecar convention. moveLedgerSidecar's append-merge on an existing destination is correct and newline-sealed. The terminal append sits immediately after the terminalPublished latch inside publishPromptTerminal, the single funnel for all four flush paths, so it is exactly-once per prompt. The tailBytes window offset is always positive (guarded by size > tailBytes), and a file shrinking between statSync and readSync degrades to fewer records — i.e. fail-closed, never a throw.
6. In-scope and house style. The DataProcessor / usageHistoryService / vitest.config.ts edits are not drive-bys — they are required by the sidecar's placement. prompt-ledger.ts / prompt-terminal-ledger.ts are kebab-case, ESM, no any, tests collocated. The @qwen-code/acp-bridge/promptLedger subpath follows the existing processRegistry → process-registry.ts precedent (export map + vitest alias; typecheck resolves via exports after build), which I confirmed by the clean typecheck.
Non-blocking note
The design doc says concurrent loads coalesce "so reconciliation runs at most once per cold restore." The conclusion is correct, but the mechanism is the waiter's attached: true gating the route check, not the reconcile call itself being coalesced. Worth a one-line precision fix whenever this file is next touched — not worth a round on its own.
Ledger growth is unbounded by design (two lines per prompt, no compaction), which the PR states as an explicit non-goal. Fine as scoped.
中文说明
审查提交:13ac0194e1ad(已确认为当前 HEAD)。Critical: 0 — 批准。
我在隔离 worktree 中检出该提交,完成安装、构建、类型检查并实际运行了相关测试,而非仅阅读 diff。
先处理遗留阻断项。 第 4 轮 review 提出的硬阻断「该分支无法通过类型检查」在当前 HEAD 已不成立:测试文件中已不存在被引用的 terminals[ 无保护索引;npm run build 与 npm run typecheck 均退出码 0、0 个 TS 错误。该结论是在 PR 处于 CONFLICTING(导致 17 个检查缺失)时写下的;分支现为 MERGEABLE,完整矩阵在本 HEAD 全绿。不过该结论有一点是对的并值得记录:CI 的 Test job 确实不运行 npm run typecheck(仅 preflight 会跑),这是 CI 缺口而非本 PR 缺陷。
本地验证: cli 79 + acp-bridge 20 + core sessionService 159 + usageHistoryService 30 = 288 测试全绿;build / typecheck 均 0 错误。
逐项核验结论:
- sidecar 不会污染 transcript 枚举(我的首要关注点)。我全仓 grep 了所有
.jsonl读取点并逐一判定:本 PR 修补的 2 处、sessionService的 5 处readdirSync(chatsDir)(4 处走SESSION_FILE_PATTERN、1 处走精确等值比较)、以及memory/manager.ts:345。没有遗漏的枚举点。 且文件名冲突不可构造:session id 既不能含.也不能含l/g/r,不合规 id 会抛SessionWriterUnavailableError。 - fail-closed 归因不变式成立。 我额外验证了两个守卫隐含依赖的前提:
queuedAt与终态at均为Date.now()墙钟毫秒,与Date.parse(transcript.timestamp)同一时钟域;证据循环的过滤条件与SessionApiHistoryAccumulator完全一致(投影一致性)。triage 提出的排队积压场景已被 FIFO 守卫关闭,且测试断言账本记录数保持不变,而非仅断言「不崩」。 - 冷加载收口每次至多执行一次——经验证而非假定。
runSharedMany是共享锁,本应允许并发;但被合并的等待者走inFlightRestores分支并返回attached: true,被路由的!restored.attached排除,故仅 owner 执行收口。 - 无死开关:
tailBytes、promptLedger(3 个注入点,live-conversation 有意排除)、getPromptLedgerPath、promptTerminals均有真实生产读写点。 - 生命周期与格式:
removePromptLedgers与既有removeWorktreeSidecars调用点 1:1 对称;moveLedgerSidecar追加合并与换行封边正确;终态写入紧随terminalPublished闩锁,覆盖全部四条冲刷路径且恰好一次;tail window 偏移恒为正,文件缩小时退化为 fail-closed 而非抛错。 - 范围与风格:
DataProcessor/usageHistoryService/vitest.config.ts的改动是 sidecar 落位的必要配套,非顺手改动;命名、ESM、无any、测试同置均合规;子路径导出沿用既有processRegistry先例。
非阻塞备注: 设计文档称并发加载「收口至多执行一次」,结论正确,但真实机制是等待者的 attached: true 触发路由守卫,而非收口调用本身被合并——建议下次触碰该文件时顺手修正一行措辞,不值得单独一轮。账本无界增长已在文档中明确列为非目标,按当前范围可接受。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 9: execute session-prompt-terminals.test.ts to confirm it passes (worktree has no node_modules and no built workspace dist/ ; npm ci + full build exceeded thi….
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):chunk 9:execute session-prompt-terminals.test.ts to confirm it passes (worktree has no node_modules and no built workspace dist/ ; npm ci + full build exceeded thi…。
— qwen3.8-max via Qwen Code /review (v0.21.14)
f8e1a58
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/services/sessionService.ts:1825 — [review] The unarchive-direction ledger-move failure has no test, while the archive direction has one ('packages/acp-bridge/src/bridgeOptions.ts:47 — [review] PromptLedgerSink.appendSync's void return type does not enforce the synchronous contract its JSpackages/cli/src/serve/routes/session.ts:3228 — [review] The route-level invariant 'reconciliation failure must never fail the load' is load-bearing andpackages/core/src/services/sessionService.ts:606 — [review] All ledger reads (reconcile, readRecentPromptTerminals, the sink) resolve only the ACTIVE chatspackages/acp-bridge/src/prompt-ledger.test.ts:185 — [review] The size <= tailBytes equality boundary in readPromptLedgerRecords has no test pinning it — the
中文说明
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 12: none — no checks were cut short..
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/prompt-terminal-ledger.test.ts:724 — [probe] system-record exclusion test fixture predates admission; the mutant counting system records survivespackages/core/src/config/config.ts:5050 — [review] /cd artifact migration omits the .ledger.jsonl sidecar (evidence permanently split from transcript)packages/acp-bridge/src/bridge.ts:2067 — [probe] cancelled-kind projection branch untested; mutant survives 1613+38 testspackages/cli/src/serve/routes/session.ts:3223 — [probe] 'reconciliation failure must never fail the load' contract untested; catch removal ships greendocs/design/2026-08-19-prompt-terminal-ledger-design.md:111 — [review] privacy field enumeration omits the persisted tailUuidpackages/cli/src/serve/prompt-terminal-ledger.test.ts:160 — [probe] realtime_message exclusion unpinned; removing both exclusions ships green and synthesizes a wrong terminalpackages/cli/src/serve/prompt-terminal-ledger.test.ts:704 — [probe] compression guard >= equality boundary unpinned; flip to > ships greenpackages/cli/src/serve/prompt-terminal-ledger.test.ts:1016 — [probe] readTranscriptTailUuid >64 KiB windowed branch untested (impl currently correct)docs/design/2026-08-19-prompt-terminal-ledger-design.md:94 — [review] 'trailing 64 terminal records' promise vs 256 KiB window truncation undocumentedpackages/acp-bridge/src/prompt-ledger.test.ts:206 — [probe] size === tailBytes boundary unpinned; <= to < mutant ships greenpackages/cli/src/serve/prompt-terminal-ledger.test.ts:669 — [probe] TOCTOU race test's ordering premise rests on module-global recordSeq; isolated run inverts itpackages/cli/src/serve/prompt-terminal-ledger.test.ts:908 — [probe] RECENT_TERMINALS_TAIL_BYTES has no pinned lower bound; 256x shrink ships greenpackages/cli/src/serve/prompt-terminal-ledger.ts:192 — [probe] session-artifact record captured as dispatch marker permanently vetoes reconciliationpackages/core/src/services/sessionService.ts:592 — [probe] /dream + auto-consolidation grep --include=*.jsonl ingests ledger sidecars as transcriptsdocs/design/2026-08-19-prompt-terminal-ledger-design.md:78 — [probe] spec's visible-write predicate omits the realtime_message exclusion both scans applypackages/acp-bridge/src/prompt-ledger.test.ts:113 — [probe] no test reads a ledger ending in a torn fragment without appending firstpackages/cli/src/serve/prompt-terminal-ledger.test.ts:688 — [probe] compression gate pinned only in the veto direction; positive probes missingpackages/cli/src/serve/prompt-terminal-ledger.test.ts:446 — [probe] passing-marker + downstream-veto combination untested; short-circuit mutant ships greenpackages/cli/src/serve/prompt-terminal-ledger.test.ts:864 — [probe] readRecentPromptTerminals catch-all (sole barrier vs failed load) unpinnedpackages/core/src/services/sessionService.test.ts:1985 — [probe] transcript-first archive ordering unpinned; hoisted ledger move survives all tests- …and 5 more (see the run report)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):chunk 12:none — no checks were cut short.。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 25 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 2: live execution of bridge-prompt-ledger.test.ts (no node_modules in worktree or main checkout; monorepo-wide npm ci + build declined as disproportionate fo…; "agent 1c": none — no check was cut short..
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/services/sessionService.test.ts:1878 — [probe] Ledger-removal test covers only the archived branch; the active-branch removePromptLedgers call site is unpinnedpackages/acp-bridge/src/prompt-ledger.test.ts:29 — [probe] The documented throw-on-I/O-failure contract of appendPromptLedgerRecord is pinned by no testpackages/cli/src/serve/prompt-terminal-ledger.test.ts:1016 — [probe] The 64 KiB transcript-tail window in readTranscriptTailUuid is pinned by no testpackages/core/src/services/sessionService.ts:1672 — [probe] A write-ahead ledger with no transcript head survives session deletion permanentlypackages/cli/src/serve/prompt-terminal-ledger.test.ts:313 — [probe] The multi-dangling fail-closed guard is unpinned; a re-admission shape then synthesizes a wrong terminalpackages/acp-bridge/src/prompt-ledger.test.ts:279 — [probe] danglingInFlightPromptIds' latest-wins supersession contract is pinned by no testpackages/acp-bridge/src/prompt-ledger.test.ts:127 — [probe] The version-strictness guard of coercePromptLedgerRecord is pinned by no test
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):chunk 2:live execution of bridge-prompt-ledger.test.ts (no node_modules in worktree or main checkout; monorepo-wide npm ci + build declined as disproportionate fo…;"agent 1c":none — no check was cut short.。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 7 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
7c4ef89 to
7598852
Compare
ytahdn
left a comment
There was a problem hiding this comment.
Round 7 — Approve with a documented known risk
Reviewed head 7598852f (incremental over the round-6 ledger; the ledger feature code is unchanged from the round-7-reviewed tree — the new head is a rebase onto current main).
Verified fixed this round:
- R6-1 (ledger sidecar permissions):
appendPromptLedgerRecordnow creates the sidecar withmode: 0o600(owner-only, matching the transcript's convention) instead of the umask default; pinned by'creates the ledger owner-only, not umask-default'. - All previously raised blocker classes (rounds 1–6) are closed in code and pinned by tests: multi-dangling fail-closed reconciliation, attribution guard for settled queued prompts, dispatch marker, projection-consistent temporal evidence with
<=clock-equality veto, compression checkpoint, unconditional deadline fence, id-less tool-call interrupted upgrade, TOCTOU fence before append,.ledger.jsonlexclusion in both transcript scanners, and ledger removal on session deletion.
Known residual risk, accepted and tracked:
- R2-2 (wrong-terminal attribution surface): the dispatch marker binds the transcript tail by ordering, not ownership — transcript records carry no writer identity, so the two documented entrances remain fail-open: (1) a recordless predecessor whose best-effort ledger appends were swallowed keeps writing past its queued successor's marker; (2) a ledger-less cross-client writer (interactive CLI resuming a serve-created session) writes transcript records invisible to every guard, which a cold load can then attribute to the dangling prompt. Entrance (2) needs no compound failure. This is a known limitation of the current design, documented in the design doc's "Residual attribution risk" section; the structural fix (writer identity on transcript records) is tracked in #9483 and this PR does not claim to close it.
Approve on the understanding that the residual attribution risk above is a documented, tracked limitation rather than an open defect in the guarded single-writer path, and that #9483 is the committed follow-up.
中文说明
第 7 轮 — 批准(附已记录的已知风险)
已审查 head 7598852f(相对第 6 轮 ledger 的增量;ledger 功能代码与第 7 轮审查的树一致——新 head 是对当前 main 的 rebase)。
本轮核实已修复:
- R6-1(ledger sidecar 权限):
appendPromptLedgerRecord现在以mode: 0o600创建(仅属主,与转录约定一致),不再使用 umask 默认值;由'creates the ledger owner-only, not umask-default'测试固化。 - 此前各轮(1–6)提出的所有阻断类问题均已修复并被测试固化:多悬空 fail-closed 对账、已结算排队 prompt 的归因守卫、dispatch marker、与投影一致的时序证据(含
<=时钟相等否决)、压缩检查点、无条件 deadline 围栏、无 id 工具调用的 interrupted 升级、追加前 TOCTOU 围栏、两处转录扫描器排除.ledger.jsonl、会话删除时清理 ledger。
已知残余风险,已接受并跟踪:
- R2-2(错误终态归因面): dispatch marker 绑定的是转录尾部的顺序而非所有权——转录记录不携带写者身份,因此两个已记录的入口仍然 fail-open:(1) 无记录前驱(其 best-effort 账本追加被吞)在其排队后继的 marker 之后持续写入;(2) 无账本写者(交互式 CLI resume serve 创建的会话)写入对全部守卫不可见的转录记录,冷加载可将其归因给悬空 prompt。入口 (2) 无需复合故障即可触发。这是当前设计的已知局限,已在设计文档 "Residual attribution risk" 一节记录;结构性修复(转录记录携带写者身份)由 #9483 跟踪,本 PR 不声称关闭它。
在"上述残余归因风险是已记录、已跟踪的局限,而非受守卫的单写者路径上的未修复缺陷,且 #9483 是承诺的跟进项"这一前提下予以批准。
Turn terminal events (turn_complete / turn_error) were synthesized by the ACP bridge and published over SSE only, so a prompt that was in flight when the daemon died could never be resolved after a restart: the cold load replay emits transcript chunks and carries no terminal evidence, leaving promptId-keyed orchestrators stuck on "unknown". Each session now owns an append-only sidecar ledger next to its transcript. The bridge appends one in_flight record at prompt admission and one terminal record at the single publishPromptTerminal exit (covering the close/kill/channel-crash/daemon-shutdown flushes) through an injected synchronous sink. Ledger writes are best-effort and never block prompt execution or teardown, and records carry only ids, states, and timestamps — no prompt text, user content, or paths. On a cold session load the serve layer reconciles prompts left dangling by a dead daemon: it classifies the transcript tail with the existing turn-interruption detector and appends a completed (stop reason reconstructed_from_transcript) or interrupted (code daemon_lost) verdict, guarded by an attribution check so an unattributable tail stays unknown (fail-closed). The load response gains an optional promptTerminals field with the trailing 64 terminal records, omitted entirely when the ledger holds no terminal evidence, and archive/unarchive move the sidecar alongside the transcript so evidence survives storage lifecycle. Design: docs/design/2026-08-19-prompt-terminal-ledger-design.md
…omplete sidecar lifecycle Address review findings on the prompt terminal ledger: - reconcile: fail closed on multiple dangling prompts (no synthesized terminal for the newest either); attribute the oldest dangling prompt only when the attribution guard skips settled admissions (fixes the [A if, B if, B cancelled] misattribution veto), the transcript's last write postdates the admission (temporal evidence), and a clean verdict is upgraded to interrupted when the model tail holds any functionCall part, id or not (id-less tool-call guard covering the detectTurnInterruption wire-pairing blind spot) - lifecycle: removeSessionFiles deletes the ledger in both archive states; archive/unarchive move it through a single getPromptLedgerPathForState helper with merge semantics when the destination already exists (append-and-unlink instead of a permanent split); move warnings carry full source and destination paths in both directions - scans: DataProcessor.scanChatFiles and usageHistoryService.rebuildFromSessionJsonl exclude .ledger.jsonl sidecars (the ledger is not a transcript) - writer: appendPromptLedgerRecord seals a torn tail before appending so a torn fragment cannot fuse with (and destroy) the next record - tests: pin the new behavior across multi-dangling fail-closed, settled-then-queued attribution, valid interleave migration, temporal veto, id-less tool-call guard, sidecar lifecycle (move/merge/warn-only delete), torn-tail sealing, queued-admission flush on shutdown, active-prompt and resume load contracts, and ledger exclusion from insight scans - docs: sync the design doc's reconciliation algorithm, lifecycle, and fail-closed invariants
readRecentPromptTerminals ran on every POST /session/:id/load (including attached hot loads) and synchronously read and JSON-parsed the entire ledger — a multi-megabyte event-loop stall for long sessions on the per-request hot path. Add a tailBytes option to readPromptLedgerRecords that reads a trailing byte window (the first window line is always dropped: the window start can tear a line in half). The load path now reads a 256 KiB window, which holds hundreds of ~150-byte records against the 64-terminal response cap; sessions whose ledger outgrows the window return a best-effort trailing subset, which the response contract already allows.
…onciliation Strengthen the reconcile attribution evidence per review round 2: measure the temporal evidence on the same api-history projection the verdict uses, fail closed on a compression checkpoint written after the target's admission, and require the visible tail to postdate every other prompt's settled terminal (FIFO evidence). Also fix a TS18048 narrowing gap in the window test, make the seal test assert the raw file layout, and restructure the window test so the call-site tailBytes wiring is actually observable.
… it into a type-only import
…and deadline-overlapped turns
… attribution risk
…pression by position Round-7 review Criticals: - appendPromptLedgerRecord created the sidecar with umask-default permissions (0o644) while the adjacent transcript is owner-only; the ledger now follows the 0o600 convention at creation time. - Marker-bearing admissions fence post-admission compression by marker position instead of wall clock, so a backward clock step cannot hide a compression reset that voids the evidence chain. - Design doc: the residual-risk claim is corrected — the dispatch marker binds ordering, not ownership; the two ownership classes that survive it (recordless predecessor with continued writes, ledger-less cross-client writer) are documented, pending writer identity on transcript records (QwenLM#9483).
7598852 to
75cd5bc
Compare
ytahdn
left a comment
There was a problem hiding this comment.
Re-approve after merging main
Head advanced 7598852f → 75cd5bcd by merging current main; the PR's own code is unchanged — the ledger feature files (prompt-ledger.ts, prompt-terminal-ledger.ts, sessionService.ts, routes/session.ts) are byte-identical to the round-7-approved tree, and the only delta is a main-side test fix in session-archive.test.ts.
Round-7 conclusion carries over unchanged: R6-1 (ledger sidecar created 0o600) and all rounds 1–6 blocker classes are fixed and pinned by tests; the R2-2 wrong-terminal attribution residual (cross-client ledger-less writer; ordering-bound marker, not ownership) remains a documented, tracked limitation — accepted on this approval, follow-up tracked in #9483.
中文说明
合并 main 后重新批准
head 由 7598852f 前进到 75cd5bcd(合并了当前 main);PR 自身代码未变——ledger 功能文件(prompt-ledger.ts、prompt-terminal-ledger.ts、sessionService.ts、routes/session.ts)与第 7 轮批准的树逐字节一致,唯一差异是 main 侧的 session-archive.test.ts 测试修正。
第 7 轮结论原样沿用:R6-1(ledger 以 0o600 创建)及第 1–6 轮全部阻断类均已修复并被测试固化;R2-2 错误终态归因残余(跨客户端无账本写者;marker 绑定顺序而非所有权)仍是已记录、已跟踪的局限——本次批准基于此接受,跟进项见 #9483。
doudouOUC
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "You are review agent test-matrix — Test coverage matrix…": run-qwen-serve.ts wiring is not unit-tested. The createPromptLedgerSink function itself is tested (path layout, append, readTranscriptTailUuid), but the spe….
中文说明
未探索到全部深度(达到工具调用预算):"You are review agent test-matrix — Test coverage matrix…":run-qwen-serve.ts wiring is not unit-tested. The createPromptLedgerSink function itself is tested (path layout, append, readTranscriptTailUuid), but the spe…。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Conflict in sessionService.ts: main's prompt-ledger lifecycle (QwenLM#9426) landed in the same spots as the PR-sidecar lifecycle — both are kept: removeSessionFiles removes both, archive/unarchive move both.
|
Released in v0.21.15. |
What this PR does
Each session now keeps a small append-only ledger of prompt lifecycle outcomes, stored as a sidecar file next to the session transcript. When a prompt is accepted for execution, an in-flight record is written; when its formal terminal outcome publishes — normal completion, cancellation, error, or any of the shutdown/kill/channel-loss flushes — a terminal record is written. Records contain only a version, the prompt id, the state or terminal kind, a machine-readable cause code, the agent stop reason when present, and a timestamp; no prompt text, user content, tool input/output, or file paths are ever written, so the ledger inherits the transcript directory's privacy boundary. Writes are best-effort and synchronous: a ledger failure is logged and never blocks prompt execution or shutdown flushing.
On a cold session load (the path taken after a daemon restart, when no live entry owns the session and no prompt is currently active), the serve layer closes the loop for prompts the dead daemon left in flight: it classifies the tail of the session transcript with the existing turn-interruption detector and appends a reconstructed verdict — completed when the tail is clean, interrupted with a daemon-lost cause when the tail shows an interrupted prompt or turn. An attribution guard ensures the visible transcript tail is only ever mapped to the most recently admitted dangling prompt; anything that cannot be attributed with confidence stays unknown, and a wrong terminal is never synthesized (fail-closed). Queued-but-never-started prompts also stay unknown by design, since they produced no transcript content.
The load response gains an optional promptTerminals field carrying the trailing 64 terminal records (including freshly reconstructed verdicts). The field is omitted entirely when there is no terminal evidence, so old clients and pre-ledger sessions see the exact previous response shape. Archiving and unarchiving a session moves the sidecar alongside its transcript, keeping the evidence through the storage lifecycle.
Why it's needed
The daemon's turn terminal events (turn_complete / turn_error) are synthesized in memory and published over SSE; they were never persisted. After a daemon restart, a cold load replays the transcript through the agent subprocess, which only emits chunk-style updates — never terminal events. External orchestrators that mediate prompts by id therefore could never resolve a prompt that was in flight when the daemon died: the contract "a terminal event for exactly this promptId" was unsatisfiable, and the only safe answer was unknown. This PR makes terminal facts survive restarts and reconstructs a trustworthy verdict for the dangling prompt, without adding any new state machine: it reuses the existing shutdown terminal flush and the existing transcript-tail interruption classifier, and layers a tiny append-only sidecar on top.
Design doc: docs/design/2026-08-19-prompt-terminal-ledger-design.md
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A (daemon-side persistence and HTTP response shape; no user-visible TUI change. Unit and route-level tests cover the write points, reconciliation branches, fail-closed paths, and response field presence/omission.)
Tested on
Environment (optional)
N/A — package-level unit tests plus root build and typecheck only.
Risk & Scope
Linked Issues
N/A
中文说明
这个 PR 做了什么
每个会话现在在会话转录文件旁边维护一个小型只追加的 prompt 生命周期结果账本(sidecar 文件)。当一个 prompt 被接受执行时写入一条 in-flight 记录;当它的正式终态发布时——正常完成、取消、错误,以及关停/终止/通道断开等各种冲刷路径——写入一条终态记录。记录只包含版本号、prompt id、状态或终态类型、机器可读的原因码、存在时的 agent 停止原因、以及时间戳;绝不写入 prompt 文本、用户内容、工具输入输出或文件路径,因此账本完全继承转录目录的隐私边界。写入是尽力而为且同步的:账本失败只记录日志,绝不阻塞 prompt 执行或关停冲刷。
在会话冷加载时(守护进程重启后、没有存活入口持有会话且没有活跃 prompt 的路径),serve 层为被死亡守护进程遗留的 in-flight prompt 收口:它用既有的回合中断检测器对会话转录尾部分类,并追加一条重建的判定——尾部干净时判定为 completed,尾部显示中断的 prompt 或回合时判定为 interrupted 并携带 daemon-lost 原因。一个归因守卫确保可见的转录尾部只会映射到最近一次准入的悬空 prompt;任何无法确信归因的情况都保持 unknown,绝不合成错误的终态(fail-closed)。排队但从未启动的 prompt 按设计同样保持 unknown,因为它们没有产生任何转录内容。
加载响应新增可选的 promptTerminals 字段,携带尾部最多 64 条终态记录(包括刚重建的判定)。没有终态证据时该字段整体省略,因此旧客户端和账本存在之前的会话看到的是与之前完全一致的响应形状。归档与取消归档会随转录一起搬动 sidecar,证据在整个存储生命周期中保留。
为什么需要
守护进程的回合终态事件(turn_complete / turn_error)在内存中合成并经 SSE 发布,从未被持久化。守护进程重启后,冷加载通过 agent 子进程重放转录,只会发出分块类更新——不会有终态事件。因此按 id 介导 prompt 的外部编排者在守护进程死亡时永远无法解析当时正在执行的 prompt:「恰好属于这个 promptId 的终态事件」这一契约无法满足,唯一安全的答案是 unknown。本 PR 让终态事实在重启后存活,并为悬空 prompt 重建可信的判定,同时不引入任何新状态机:它复用既有的关停终态冲刷和既有的转录尾部中断分类器,只在其上叠加一个极小的只追加 sidecar。
设计文档:docs/design/2026-08-19-prompt-terminal-ledger-design.md
审阅者测试计划
如何验证
证据(前后对比)
N/A(守护进程侧持久化与 HTTP 响应形状变化;无用户可见的 TUI 变化。单测与路由级测试覆盖写点、归因分支、fail-closed 路径以及响应字段的存在/省略。)
测试平台
环境(可选)
N/A —— 仅包级单测加根目录 build 与 typecheck。
风险与范围
关联 Issue
N/A