Conversation
Verification reportVerified commit: Before and afterThe ACP host reproduction uses a deterministic model/tool response that ends a Goal turn after the terminating tool result. The original baseline fails the clean-recovery assertion after normal
The independent host probe passed its one selected regression (1,010 unrelated cases skipped). The Goal turn still used exactly one model stream. Actual persistence and restoreA separate script exercised the built recorder, real temporary JSONL files, full-history restore, and optimized runtime projection. All seven cases passed:
Both loaders agreed in every case. API-history JSON was byte-for-byte identical before and after writing the structured end record. This script made zero model calls. Repository checks
The focused suites include settlement cancellation, write failure, subsequent retry, duplicate IDs, compression, memory-pressure cleanup, fork/rewind, and channel turns that still owe a final response. Focused-suite commandsRun each command from the indicated package after building the repository. # packages/core
npx vitest run src/core/llm-chat.test.ts src/core/turn-interruption.test.ts src/core/session-recovery.test.ts src/core/client.test.ts src/services/memoryPressureMonitor.test.ts src/services/chatRecordingService.test.ts src/services/session-api-history.test.ts src/services/session-transcript-reader.test.ts src/utils/transcript-records.test.ts src/utils/conversation-branches.test.ts
# packages/cli
npx vitest run src/acp-integration/session/Session.test.ts src/nonInteractive/session.test.ts src/nonInteractiveCli.test.ts src/serve/prompt-terminal-ledger.test.ts src/ui/hooks/use-llm-stream.test.tsxLimitsThis is host-level and real-persistence verification with an isolated CLI smoke, not browser or live-model E2E. Cache-hit rate was not measured. Windows/Linux execution and hosted CI are separate checks. The fix has not been deployed to production or applied retroactively to old transcripts. |
github-actions
Bot
added
the
review/self-reported
|
|
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
Thanks for the PR! Template looks good ✓ — every section filled in, a real Before/After, and a complete Chinese translation rather than a stub. Problem: observed, not theoretical. #11914 carries transcript timestamps from a real session ( Direction: aligned. Recovery telling a user their work was interrupted when the Goal actually completed is a wrong signal on the one path users are supposed to trust after a reload — and the conservative direction matters too, since a genuinely unanswered prompt still has to offer Continue. The reference agent's CHANGELOG has fixed this class of bug repeatedly: auto-resume re-running a turn that had already settled, resumed headless sessions losing a turn's replies, and resume changing how earlier context was re-sent so prompt-cache reuse suffered. So recovery classification is a maintained concern upstream, not a niche one — and that last entry is a good independent argument for this PR's "stays out of model history" constraint. Size: core paths are touched, so the breakdown matters — 284 production logic lines across 16 files, 804 test lines across 8 files, 0 generated/schema. That reconciles to the reported 1000 additions + 88 deletions. Under the 500-line core escalation threshold and under the 1000-line large-PR advisory, so no size escalation. A ~2.8:1 test-to-production ratio is the right shape for a change to recovery semantics. Approach: the scope holds up. I wrote down what I'd do before opening the diff — a durable side-band record written at settle time, keyed to the tool-result IDs it settles, re-validated by every history transform that could invalidate it — and that's what this does, covering more edge cases than I had listed. Every changed file sits on one of three links: write the record, carry it through a transform, or read it back on recovery. The Two questions rather than blockers:
Risk: Stage 1e matched. Also worth a maintainer's eye on sequencing: this is one of four Goal/hooks PRs opened the same day (#11922, #11924, #11927, #11904), so they may want to land in a particular order. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 各章节都填了，有真实的 Before/After，中文翻译也是完整的而不是占位。 问题： 是已观测到的 bug，不是理论性加固。#11914 带有真实会话的记录时间戳（03:25:38Z 方向： 对齐。Goal 实际已完成，恢复逻辑却告诉用户"上次请求中断"，这是在用户重载后最应当信任的路径上给出错误信号；同时保守方向同样重要——真正未回答的请求仍必须提供"继续执行"。参考实现的 CHANGELOG 反复修过这类问题：自动恢复重跑了已经结算的回合、恢复后的 headless 会话丢失某回合的回复、以及恢复改变了既有上下文的重发方式从而影响 prompt cache 命中。可见恢复分类在上游是持续维护的关注点，而非边缘问题——最后那一条也正好为本次"不进入模型历史"的设计约束提供了独立佐证。 规模： 触及核心路径，因此需要拆分统计——16 个文件共 284 行生产逻辑，8 个文件共 804 行测试，生成/schema 文件 0 行。合计与 PR 报告的 1000 增 / 88 删一致。低于核心 500 行升级阈值，也低于 1000 行大 PR 提示线，因此不触发规模升级。测试与生产代码约 2.8:1，对恢复语义类改动是合适的比例。 方案： 范围站得住。我在看 diff 之前先写下了自己的做法——在结算时刻写入一条旁路持久化记录，以其所结算的工具结果 ID 为键，并由每一个可能使其失效的历史变换重新校验——本 PR 正是这样做的，且覆盖的边界情况比我列的更多。所有改动文件都落在三条链路之一：写入记录、在变换中传递记录、恢复时读回记录。 两点是提问，不是阻塞项：
风险： Stage 1e 命中。 另外提请维护者注意合并顺序：这是同一天提交的四个 Goal/hooks 相关 PR 之一（#11922、#11924、#11927、#11904），可能需要按特定顺序合并。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo correctness blockers found. I read the production diff against the base tree and, because this touches core recovery semantics, walked the consumer surface rather than trusting the diff to be self-contained. The consumer map checks out. The riskiest part of this change is not the new record, it's that Both The failure semantics are conservative in both directions, which is the part I cared about most. An id is only accepted on replay when the immediately preceding material record is a Two things I confirmed are non-issues, because both looked wrong at first glance. The new Keeping Non-blocking observations:
How the boundary is written and read backsequenceDiagram
participant P1 as ACP host Session
participant P2 as ChatRecordingService
participant P3 as Transcript JSONL
participant P4 as History Accumulator
participant P5 as LlmChat
participant P6 as Turn Interruption
Note over P1: terminating tool result settles the Goal turn
P1->>P1: waitForPendingRewrites, capture ending tool call id
P1->>P2: recordGoalTurnEnd(toolCallId, permit)
P2->>P3: append strict system record
P1->>P5: setCompletedToolCallIds(existing plus id)
Note over P1,P5: skipped on cancel, on writer failure, or when not end_turn
P3-->>P4: reload, replay the active branch only
P4->>P4: require matching prior tool_result and a unique id
P4-->>P5: completedToolCallIds
P5->>P6: history plus completedToolCallIds
P6-->>P1: boundary at end, kind none, canContinue false
Files changed (24 of 24 shown)
Test evidenceThis is an unattended CI run, so per the gate rules I did not build or execute anything from this PR — the evidence below is the PR's own CI, read through the API for the reviewed commit. 80 check-runs, 0 failures, 0 cancellations. Lint & Static, Integration Tests (no-AK, No Sandbox), TUI parity snapshots, both Desktop Shell jobs, the OpenTUI no-flicker gate, the Real daemon E2E job and the full Java SDK matrix are green. The load-bearing check has not landed yet: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified, and why: the central claim is behavioural — that a real ACP Goal turn ending on a terminating tool result reloads clean with no Continue banner — and a green unit suite proves the tests pass, not that they pin the change. The author reports a compiled-code JSONL test over seven recovery/fork/rewind scenarios and an isolated built-CLI Sandboxed verification would settle this: 中文说明代码审查未发现正确性阻塞项。我对照基线代码树读了生产部分 diff；由于改动触及核心恢复语义，我把下游消费方完整走了一遍，而不是假定 diff 自身闭合。 消费方映射是完整的。 本次最危险的地方不是新增记录，而是 两处 失败语义在两个方向上都是保守的，这是我最关心的部分。 只有在满足以下全部条件时，回放侧才接受一个 id：紧邻的上一条实质记录是同一 goal/revision/turn 许可下的 有两处我确认不是问题，因为它们乍看都像 bug。 把 非阻塞观察：
测试证据这是无人值守的 CI 运行，因此按关卡规则我没有构建或执行本 PR 的任何代码——以下证据来自 PR 自身的 CI，通过 API 读取所审提交的结果。80 个 check-run，0 失败，0 取消。 Lint & Static、Integration Tests (no-AK, No Sandbox)、TUI parity snapshots、两个 Desktop Shell 任务、OpenTUI no-flicker gate、Real daemon E2E 以及完整的 Java SDK 矩阵均为绿色。 关键检查尚未出结果： 未验证的部分及原因：核心主张是行为性的——真实的 ACP Goal 回合在终止型工具结果处结束后，重载应当干净、不出现"继续执行"横幅——而绿色单元套件只能证明测试通过，不能证明测试钉住了这个改动。作者报告了一个使用编译产物和真实 JSONL 的测试覆盖七个恢复/fork/rewind 场景，以及一次隔离构建版 CLI 的 沙箱验证可以定论： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the design is right and the failure semantics are conservative in both directions; the nits are a mocked seam and a missing design doc, neither of which blocks. Stepping back: I tried to break this and couldn't. I went in expecting the interesting risk to be the new record type, and it isn't — a new The two things that looked like real bugs on first read both dissolved. The early return in On my own proposal: I sketched it before reading the diff and it was the same shape — durable side-band record keyed to the tool-result ids it settles, re-validated by every transform. They covered more ground than I had, notably that a boundary must survive compaction via the compression payload and be re-filtered against the compressed history, and that rewind has to be able to expose an earlier completed turn rather than only the latest. The What I'd want a maintainer to weigh, none of it blocking:
CI is still running, so no approval is posted in this run. A maintainer can close the behavioural gap in the meantime with 中文说明信心度：4/5 —— 设计正确，失败语义在两个方向上都保守；不足之处是一处被 mock 的接缝和缺少设计文档，两者都不构成阻塞。 退一步看：我试着找它的破绽，没找到。我原以为有意思的风险在新增记录类型上，其实不是——一个新的 初读时像真 bug 的两处都消解了。 关于我自己的方案：我在读 diff 前先写了草案，形状相同——以所结算的工具结果 id 为键的旁路持久化记录，并由每个变换重新校验。他们覆盖的范围比我更广，尤其是边界必须通过压缩载荷在压缩后存活、并针对压缩后的历史重新过滤，以及回退必须能暴露更早的已完成回合而不只是最近一个。 希望维护者权衡的几点，均不阻塞：
CI 仍在运行，因此本次不提交批准。 维护者在此期间可以用 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Traced the new boundary from where it is written to where it is read. No blocking issues found. The paths I checked and what makes each hold:
The record is only accepted when it can be trusted. SessionApiHistoryAccumulator.add demands a unique call/result pair (hasUniqueToolResult), a matching goal permit on both the goal_turn_end record and the immediately preceding material record, and a functionResponse with that id inside that record. Any reuse of the id later deletes the entry again (completedToolCallIds.delete on a later functionCall/functionResponse), which is the "rewritten tool result invalidates the boundary" rule. A failed appendRecordStrict skips the in-memory setCompletedToolCallIds too, so a write failure cannot leave a boundary that no transcript can back up.
completedToolCallBoundary is conservative in the right direction. boundaries.has(id) || history[i].role !== 'user' ? 0 : i + 1 parses as (A || B) ? 0 : i + 1, so a duplicated id or a response outside a user content collapses the candidate to 0 — that is, to today's behaviour — rather than to a wrong boundary. An empty map yields Math.max(0).
The early return is what fixes the reported symptom. if (boundary === history.length) return { kind: 'none' } is load-bearing: without it the trailing user content that carries the terminating tool result is exactly what the interrupted_prompt scan collects, which is the false "Continue" this PR removes. The new turn-interruption.test.ts case pins the clean case, the later unanswered input, a missing id, a reused id, and the interrupted_turn path unchanged.
Every history transform re-validates instead of carrying blindly. setHistory, truncateHistory, stripThoughtParts, stripOrphanedUserEntriesFromHistory, the compression record round trip and the hard-rescue rollback all pass the current ids back through setCompletedToolCallIds, so a boundary whose tool result did not survive is dropped. stripOrphanedUserEntriesFromHistory stops popping at the boundary, so it cannot eat the content the boundary rests on. client.ts seeds the ids after both restore paths and re-seeds them onto the new chat after compaction.
Including goal_turn_end in modelSet is necessary and adds nothing to the model history. Without it the accumulator would never see the record on a reload; the subtype branch returns before anything is appended, which is consistent with the "API-history JSON is byte-for-byte identical" evidence in the PR body. conversation-branches.ts and transcript-records.ts also register the subtype, so rewind/fork treat it as a neutral tail record rather than an unknown one.
A reachable-looking case that is not a defect. Dropping the mid_turn_user_message merge when the preceding user content holds a completed result can emit two adjacent user contents. I checked the Anthropic converter, which runs mergeConsecutiveUserMessages before the wire, so this does not produce a role-alternation failure, and keeping the mid-turn text out of the boundary content is what lets recovery offer it as unanswered input.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- DUP-1 LlmClient.setHistory boundary threading on wholesale restore — already discussed and ruled intentional (comment 5677051197)
- DUP-2 mocked completed-id seam in the Session Goal-turn test — already reported (comments 5677051197, 5677051436)
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): "agent reverse-audit (round 2)": whether a real session can produce a compressedHistory with functionCall count ≠ 1 and functionResponse count === 1 for a live-preserved boundary id — I t…; "agent reverse-audit (round 2)": whether skipPersistence (Session.ts:12196, restored- ask_user_question -only) can co-occur with a terminateTurn batch such that endingToolCallId = toolRun.….
Test Plan (not a blocker): 359 tests passed — this review observed 31597, 26476, 2113, 1028, 2040, 587, 8178 passed; 537 tests passed — this review observed 31597, 26476, 2113, 1028, 2040, 587, 8178 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：whether a real session can produce a compressedHistory with functionCall count ≠ 1 and functionResponse count === 1 for a live-preserved boundary id — I t…；"agent reverse-audit (round 2)"：whether skipPersistence (Session.ts:12196, restored- ask_user_question -only) can co-occur with a terminateTurn batch such that endingToolCallId = toolRun.…。
Test Plan（非阻断）：359 tests passed — this review observed 31597, 26476, 2113, 1028, 2040, 587, 8178 passed; 537 tests passed — this review observed 31597, 26476, 2113, 1028, 2040, 587, 8178 passed。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| await recorder.recordGoalTurnEnd( | ||
| turn.endingToolCallId, | ||
| turn.permit, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-1: This strict append runs before runtime.finishTurn(turn.permit), so a transient failure of what is optional recovery metadata costs the turn commit that is essential. If the goal_turn_end append fails — ENOSPC, EIO, or a writer lease taken over by a second process — enterWriteFailure latches the recorder and rethrows; the try/catch here swallows that throw and logs a warn, so execution continues into finishTurn. finishTurn journals through recordGoalState → appendRecordStrict, which opens with if (this.writeFailure) throw this.writeFailure;, so it throws too, and the settlement catch arm runs releaseTurn(turn.turnKey) with the default requeue. The turn that had already finished is then never committed: no goal_state record, and no token or noProgressTurns accounting, since both are applied inside finishTurn only after the journal write. The Goal stays active and churns permits — the requeued continuation's prompt() rejects at assertCanStartTurn() before any model request, #drainGoalQueueExclusive's catch settles it with modelStarted: false, and the cycle repeats until the user cancels. Moving this block behind the settlement try/catch costs only the boundary on a write failure, which is the conservative outcome the description claims.
For scale: this widens a hazard the file already documents rather than introducing a new failure mode — the merge base carries the same latched-write fallback for finishTurn's own strict write, and a transient failure there produces the identical lost settlement. That is why this is a Suggestion rather than a blocker, but the fix is a pure move.
Witness:
P0 (real ChatRecordingService, one transient writeLine rejection on the goal_turn_end append):
{"probe":"P0","writesWhenGoalTurnEndFailed":1,"recordGoalState":"THREW EIO once",
"flush":"THREW EIO once","writesAfterRecordGoalState":1,"writerTouchedBySettlementWrite":false}
P1 (latch modelled causally, unmodified PR):
{"probe":"P1","recordGoalTurnEndCalled":1,"finishTurnCalled":1,"finishTurnThrew":true,
"releaseTurnCalls":[["goal-runtime:turn-latch-probe"]],"dispatchCalled":0}
P1 with the block moved behind the settlement try/catch — the probe flips:
{"probe":"P1","recordGoalTurnEndCalled":1,"finishTurnCalled":1,"finishTurnThrew":false,
"releaseTurnCalls":[],"dispatchCalled":0}
Transcript ordering stays valid after the move: the intervening goal_state record is a system record the reader keeps out of modelSet, and in SessionApiHistoryAccumulator.add a non-goal_turn_end system record returns before touching lastMaterialRecord, so the previous?.type === 'tool_result' adjacency check still sees the terminating tool result.
The fix must not assume the recorder is healthy at settlement time: appendRecordStrict re-throws a latched failure forever (packages/core/src/services/chatRecordingService.ts:1449-1452), the latch is set by enterWriteFailure (chatRecordingService.ts:1326-1346), and finishTurn shares that path via recordGoalState (chatRecordingService.ts:1895, reached from goal-runtime.ts:1913). Extend the existing recordingFails: true case in Session.test.ts so the mocked runtime models the latch — make mockGoalRuntime.finishTurn reject once recordGoalTurnEnd has rejected — and assert expect(mockGoalRuntime.releaseTurn).not.toHaveBeenCalled(); that assertion is red today and green once the record moves behind settlement. Please then remove the move again and confirm the assertion reds.
中文说明
这个 strict append 位于 runtime.finishTurn(turn.permit) 之前，因此一次可选恢复元数据的瞬时写入失败，会连带牺牲掉真正必要的回合提交。若 goal_turn_end 追加失败（ENOSPC、EIO，或写入租约被第二个进程接管），enterWriteFailure 会把记录器置为锁定并重新抛出；此处的 try/catch 吞掉该异常只记一条 warn，于是执行继续进入 finishTurn。finishTurn 经 recordGoalState → appendRecordStrict 写日志，而后者开头就是 if (this.writeFailure) throw this.writeFailure;，因此它同样抛出，结算的 catch 分支随即以默认 requeue 调用 releaseTurn(turn.turnKey)。结果是：本已完成的回合永远不会被提交——没有 goal_state 记录，也没有 token 与 noProgressTurns 记账（两者都在 finishTurn 内部、日志写入之后才生效）。Goal 会保持 active 并不断空转许可——被重新排队的 continuation 在 prompt() 中于 assertCanStartTurn() 处被拒（尚未发出任何模型请求），#drainGoalQueueExclusive 的 catch 以 modelStarted: false 结算，如此循环直到用户取消。把这一整块移到结算 try/catch 之后，写入失败时只损失边界，这正是描述中所声称的保守结果。
关于严重程度：这是在扩大文件中已记录的风险，而非引入新的失败模式——merge base 对 finishTurn 自身的 strict 写入已有同样的锁定回退，那里的瞬时失败会产生完全相同的“提交丢失”。因此这是 Suggestion 而非阻塞项，但修复只是一次纯粹的代码移动。
移动后记录顺序依然成立：中间插入的 goal_state 是 system 记录，读取侧不会将其纳入 modelSet；而在 SessionApiHistoryAccumulator.add 中，非 goal_turn_end 的 system 记录会在触及 lastMaterialRecord 之前返回，所以 previous?.type === 'tool_result' 的相邻性检查仍能看到那个终止型工具结果。
修复时不能假定结算那一刻记录器是健康的：appendRecordStrict 会永久重抛锁定失败（packages/core/src/services/chatRecordingService.ts:1449-1452），锁定由 enterWriteFailure 设置（chatRecordingService.ts:1326-1346），而 finishTurn 经 recordGoalState 共用这条路径（chatRecordingService.ts:1895，由 goal-runtime.ts:1913 抵达）。请把 Session.test.ts 中已有的 recordingFails: true 用例扩展为让 mock runtime 模拟该锁定——在 recordGoalTurnEnd 拒绝之后让 mockGoalRuntime.finishTurn 也拒绝——并断言 expect(mockGoalRuntime.releaseTurn).not.toHaveBeenCalled()；该断言在今天为红，移动之后转绿。随后请再撤销这次移动，确认断言重新变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| result?.stopReason === 'end_turn' && | ||
| failureMessage === undefined && | ||
| !turn.controller.signal.aborted |
There was a problem hiding this comment.
[Suggestion] R1-2: The new cancellation test never makes !turn.controller.signal.aborted the deciding clause, so this guard can be deleted with the whole suite still green. In that test the abort lands before the terminating-tool exit computes its stop reason, so getAbortAwareEndTurnStopReason returns 'cancelled' and the preceding result?.stopReason === 'end_turn' clause already blocks the recording. The clause is load-bearing only for the narrower race it was presumably written for — a cancel landing after the stop reason was computed, for example during the settlement path's await ...flush() just above — and nothing covers that window.
The remap hazard is not hypothetical: this file holds 21 bare return { stopReason: 'end_turn' } exits against 14 abort-aware ones, so a future remap of the terminating-tool exit onto a bare one would persist a goal_turn_end boundary for a turn the user cancelled. On resume getRecoveryStatus() would report clean / canContinue: false, and the cancelled-but-unanswered prompt would be silently dropped instead of offered for continuation.
Witness:
mutation: delete `!turn.controller.signal.aborted` from the guard
npx vitest run src/acp-integration/session/Session.test.ts -t 'a tool that ends the turn'
-> Tests 17 passed | 998 skipped (1015) # the mutant survives, cancellation test included
static premises, quoted at the reviewed commit:
Session.ts:577-581 return signal.aborted ? 'cancelled' : 'end_turn';
Session.ts:7954 stopReason: getAbortAwareEndTurnStopReason(pendingSend.signal)
Session.ts:8720-8724 endingToolCallId assigned after `await this.messageRewriter?.waitForPendingRewrites()`
exit census in Session.ts: 21 bare `{ stopReason: 'end_turn' }` vs 14 abort-aware
Add a case that lets the terminating tool exit normally (so getAbortAwareEndTurnStopReason returns 'end_turn') and aborts the turn controller while the settlement's recording flush is in flight, then asserts that neither recordGoalTurnEnd nor setCompletedToolCallIds was called.
getAbortAwareEndTurnStopReason already folds an abort into the stop reason (return signal.aborted ? 'cancelled' : 'end_turn';, Session.ts:580), so the new test must abort after that call rather than before it, or it re-pins the stopReason clause and leaves this one still uncovered. That new case must go red when !turn.controller.signal.aborted is removed — the mutation that survives today.
中文说明
新增的取消测试从未让 !turn.controller.signal.aborted 成为决定性条件，因此删掉这个守卫，整个测试套件仍然是绿的。在该测试中，中止发生在终止型工具退出计算 stop reason 之前，所以 getAbortAwareEndTurnStopReason 返回 'cancelled'，前一个 result?.stopReason === 'end_turn' 条件已经拦住了记录写入。这个子句真正起作用的只有一个更窄的竞态——中止发生在 stop reason 计算之后，例如就在上方的结算路径 await ...flush() 期间——而这个窗口没有任何覆盖。
“被改写”的风险并非假设：本文件中有 21 处裸 return { stopReason: 'end_turn' } 退出，对应 14 处感知中止的退出。因此将来若把终止型工具退出改接到某个裸退出上，就会为一个用户已取消的回合持久化 goal_turn_end 边界。恢复时 getRecoveryStatus() 会报告 clean / canContinue: false，那条已取消但未获答复的输入会被静默丢弃，而不再被提供“继续执行”。
请补一个用例：让终止型工具正常退出（使 getAbortAwareEndTurnStopReason 返回 'end_turn'），并在结算的记录 flush 进行中中止 turn controller，然后断言 recordGoalTurnEnd 与 setCompletedToolCallIds 都未被调用。
注意 getAbortAwareEndTurnStopReason 已经把中止折进 stop reason（return signal.aborted ? 'cancelled' : 'end_turn';，Session.ts:580），所以新用例必须在该调用之后才中止，否则只是重新钉住了 stopReason 条件，本子句仍未被覆盖。该新用例在移除 !turn.controller.signal.aborted 时必须变红——也就是今天能够存活的那个变异。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| try { | ||
| const history = llmClient?.getHistoryShallow?.() ?? []; | ||
| const history = | ||
| llmClient?.getChat?.()?.getHistoryForRecovery?.() ?? |
There was a problem hiding this comment.
[Suggestion] R1-3: One of six sites where the new completedToolCallIds threading has no test that reddens when the threading at that site is removed, so a regression here ships green. getHistoryForRecovery appears in zero files matching packages/cli/src/**/*.test.ts{,x}, and no fake client in use-llm-stream.test.tsx exposes getChat, so this new first ?? rung always evaluates to undefined under test and all ~10 retry-debt tests exercise the getHistoryShallow fallback instead.
Because the call is optional-chained, a rename or removal of the core method degrades silently to the old full-history scan with nothing failing. The cost is that this hunk's whole purpose — making the CLI's orphaned-envelope scan stop at the same completed-tool-call boundary that core's stripOrphanedUserEntriesFromHistory now stops at — stays unpinned: an envelope sitting below the boundary would be classified as a trailing orphan and re-attached to the retry payload even though the core strip no longer pops it, duplicating teammate envelope text into the retried prompt.
Witness:
mechanical, at the reviewed commit:
rg -l 'getHistoryForRecovery' 'packages/cli/src/**/*.test.ts' '**/*.test.tsx' -> 0 files
only cli source reference: packages/cli/src/ui/hooks/use-llm-stream.ts:4520 (this hunk)
the cited retry-debt tests do exist and drive the fallback, e.g.
client.getHistoryShallow = vi.fn().mockReturnValue([ at 2044, 2142, 2259, 2327, 2373, 3115, ...
so reverting this rung is a semantic no-op in tests.
Add one case to the existing retry-debt describe whose fake client exposes getChat: () => ({ getHistoryForRecovery: () => historyFromBoundary }), where a pre-boundary user entry carries the envelope text, and assert the envelope is not re-attached — while keeping getHistoryShallow returning a list that would match, so the test fails if the chain regresses to the fallback.
The scan matches by byte equality (return JSON.stringify(a) === JSON.stringify(b);, use-llm-stream.ts:4483) while getHistoryForRecovery() returns copyContentContainer copies, so a hand-built fixture must round-trip identically or the test would pass by never matching. That new case must go red when this getHistoryForRecovery?.() rung is deleted and the scan falls back to getHistoryShallow.
中文说明
这是六处“新增 completedToolCallIds 传递没有被任何测试钉住”的站点之一：移除该处的传递，测试仍然全绿。getHistoryForRecovery 在 packages/cli/src/**/*.test.ts{,x} 中出现 0 次，且 use-llm-stream.test.tsx 中没有任何 fake client 暴露 getChat，因此这个新的第一级 ?? 在测试中永远求值为 undefined，约 10 个 retry-debt 用例实际走的都是 getHistoryShallow 回退分支。
由于该调用使用了可选链，核心方法一旦被重命名或移除，就会静默退回到旧的全历史扫描而不会有任何失败。代价是本 hunk 的全部意图——让 CLI 的孤儿 envelope 扫描停在 core 的 stripOrphanedUserEntriesFromHistory 现在所停的同一个已完成工具调用边界上——完全没有被钉住：位于边界之下的 envelope 会被判定为尾部孤儿并重新挂到重试载荷上，而 core 的 strip 已不再弹出它，于是队友 envelope 文本会在重试提示中重复出现。
请在已有的 retry-debt describe 中补一个用例：让 fake client 暴露 getChat: () => ({ getHistoryForRecovery: () => historyFromBoundary })，其中边界之前的 user 条目携带该 envelope 文本，并断言 envelope 没有被重新挂上；同时让 getHistoryShallow 返回一个会匹配的列表，这样一旦链路退化到回退分支，用例就会失败。
注意该扫描按字节相等匹配（return JSON.stringify(a) === JSON.stringify(b);，use-llm-stream.ts:4483），而 getHistoryForRecovery() 返回的是 copyContentContainer 副本，因此手工构造的 fixture 必须能逐字节往返一致，否则用例会因为“从不匹配”而假绿。删除这一级 getHistoryForRecovery?.() 使扫描退回 getHistoryShallow 时，该新用例必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| ); | ||
| await this.restoreLoadedSkillsFromHistory(resumedHistory); | ||
| const chat = this.getChat(); | ||
| chat.setCompletedToolCallIds(restored.completedToolCallIds); |
There was a problem hiding this comment.
[Suggestion] R1-4: One of six sites where the new completedToolCallIds threading has no test that reddens when the threading at that site is removed. This is the legacy getResumedSessionData resume branch; only the sibling getSessionRestoreRuntime projection branch at line 592 is pinned. client.test.ts has no goal_turn_end fixture and no getResumedSessionData case asserting getCompletedToolCallIds().
So qwen --resume <session> on a transcript whose tail is a recorded Goal turn end can lose its boundary with nothing failing: getHistoryForRecovery() returns the entire restored history instead of [], and the first stripOrphanedUserEntriesFromHistory() after a failed send pops the restored update_goal functionResponse — a role: 'user' entry — together with the new prompt, which is precisely the corruption the projection-path test at line 733 was added to prevent, on the other resume path. The same unpinned one-line plumbing appears at packages/cli/src/nonInteractive/session.ts:558 and packages/cli/src/nonInteractiveCli.ts:1144.
Witness:
mutation: delete `chat.setCompletedToolCallIds(restored.completedToolCallIds);`
npx vitest run src/core/client.test.ts -> 412 passed (412) # mutant survives the whole file
LIVENESS CONTROL: delete the sibling seeding at client.ts:593 instead
-> 1 failed | 411 passed
FAIL ... keeps the restored tool boundary through startup reminder refresh
expected [ { role: 'user', ...(1) }, ...(1) ] to deeply equal []
Mirror the new selective-restore test for this branch: mock getResumedSessionData() with a conversation containing the call / result / goal_turn_end triple, await client.initialize(), and assert client.getChat().getCompletedToolCallIds() equals ['ended'] and getHistoryForRecovery() is [].
packages/core/src/services/session-api-history.ts:216 returns ...(completedToolCallIds.length > 0 ? { completedToolCallIds } : {}), so a fixture that fails any of the accumulator's gates yields undefined rather than an empty-list signal — copy the working fixture at session-api-history.test.ts:73 rather than hand-rolling one, and keep the seeding after await this.startChat(...) since the setter filters against this.history. That new test must go red when this line is removed.
中文说明
这是六处“新增 completedToolCallIds 传递没有被任何测试钉住”的站点之一。此处是旧式 getResumedSessionData 恢复分支；只有第 592 行的同级 getSessionRestoreRuntime 投影分支被钉住。client.test.ts 中没有任何 goal_turn_end fixture，也没有任何 getResumedSessionData 用例断言 getCompletedToolCallIds()。
因此对一段尾部为已记录 Goal 回合结束的会话执行 qwen --resume <session>，边界可能丢失而没有任何测试失败：getHistoryForRecovery() 会返回整段恢复后的历史而不是 []，随后一次发送失败触发的 stripOrphanedUserEntriesFromHistory() 会把恢复出来的 update_goal functionResponse（一个 role: 'user' 条目）连同新提示一起弹出——这正是第 733 行投影路径用例被添加来防止的损坏，只不过发生在另一条恢复路径上。同样未被钉住的一行式传递还出现在 packages/cli/src/nonInteractive/session.ts:558 与 packages/cli/src/nonInteractiveCli.ts:1144。
请为本分支照搬新的选择性恢复用例：用包含 call / result / goal_turn_end 三元组的会话 mock getResumedSessionData()，await client.initialize()，然后断言 client.getChat().getCompletedToolCallIds() 等于 ['ended'] 且 getHistoryForRecovery() 为 []。
注意 packages/core/src/services/session-api-history.ts:216 返回的是 ...(completedToolCallIds.length > 0 ? { completedToolCallIds } : {})，因此任何未通过累加器校验的 fixture 得到的是 undefined 而不是空列表信号——请复制 session-api-history.test.ts:73 中可用的 fixture，不要手工拼装；同时播种必须保持在 await this.startChat(...) 之后，因为 setter 会基于 this.history 过滤。移除本行时，该新用例必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| }); | ||
| } | ||
| this.setHistory(newHistory); | ||
| this.setHistory(newHistory, this.completedToolCallIds); |
There was a problem hiding this comment.
[Suggestion] R1-5: One of six sites where the new completedToolCallIds threading has no test that reddens when the threading at that site is removed — and the most consequential one, since compression is the most frequent history rewrite in a long session. This location covers all three llm-chat sites (:2726, :2866, and the hard-rescue rollback at :3216-3219); they are really one gap. No test pins the boundary surviving a real compression: the only compression test that checks ids (client.test.ts:5238) replaces tryCompress with a mock that calls setHistory(compressedHistory, ['completed-call']) itself, so it pins the client-level re-seed rather than this file.
A session that ended a Goal turn compresses, getCompletedToolCallIds() returns [], so getRecoveryStatus() reports interrupted_prompt for a turn that finished, stripOrphanedUserEntriesFromHistory() pops the trailing update_goal functionResponse and leaves the model's functionCall unanswered on the next send, and the JSONL checkpoint no longer carries the ids — so a resume after compression cannot recover the boundary even though the reader side is tested. The hard-rescue rollback has the same shape: a prompt still too large after compression restores the pre-compression history but silently clears the boundary.
Witness:
mutation: drop the ids argument at all three sites (:2726, :2866, :3216-3219)
npx vitest run src/core/llm-chat.test.ts -> 520 passed (520) # mutant survives
LIVENESS CONTROL: make getHistoryForRecovery() return the unsliced history
-> 3 failed | 1 passed in the `completed tool boundary` block
Add a case to the completed tool boundary describe: seed chat.setHistory([call, result], ['ended']), mock ChatCompressionService.prototype.compress to return a newHistory that retains both the functionCall and its functionResponse, run chat.tryCompress(...), then assert chat.getCompletedToolCallIds() equals ['ended'], chat.getHistoryForRecovery() is [], and the recordChatCompression payload carries completedToolCallIds: ['ended']. Add the hard-rescue twin: force the post-compression send to be rejected and assert the ids survive the rollback.
setCompletedToolCallIds drops any id not uniquely answered in the new history — (id) => completedToolCallBoundary(this.history, [id]) > 0 (llm-chat.ts:2286) — so the fixture's compressed history must keep exactly one functionCall and one functionResponse for 'ended', or the assertion passes for the wrong reason. Both new cases must go red when the second argument is removed from the corresponding setHistory call.
中文说明
这是六处“新增 completedToolCallIds 传递没有被任何测试钉住”的站点之一，也是其中影响最大的一处，因为压缩是长会话中最频繁的历史重写。本条覆盖 llm-chat 的全部三处（:2726、:2866，以及 :3216-3219 的硬救援回滚）——它们实际上是同一个缺口。没有任何测试钉住“边界在真实压缩后存活”：唯一检查 ids 的压缩用例（client.test.ts:5238）把 tryCompress 替换成了一个自行调用 setHistory(compressedHistory, ['completed-call']) 的 mock，因此它钉住的是 client 层的重新播种，而不是本文件。
一个结束了 Goal 回合的会话在压缩后，getCompletedToolCallIds() 返回 []，于是 getRecoveryStatus() 会对一个已完成的回合报告 interrupted_prompt，stripOrphanedUserEntriesFromHistory() 会弹出尾部的 update_goal functionResponse，使模型的 functionCall 在下一次发送时无应答；同时 JSONL 检查点不再携带 ids——因此压缩后的 resume 无法恢复边界，尽管读取侧是有测试的。硬救援回滚是同样的形态：压缩后仍然过大的提示会恢复压缩前的历史，却静默清空边界。
请在 completed tool boundary describe 中补一个用例：先 chat.setHistory([call, result], ['ended'])，mock ChatCompressionService.prototype.compress 返回一个同时保留 functionCall 与其 functionResponse 的 newHistory，执行 chat.tryCompress(...)，然后断言 chat.getCompletedToolCallIds() 等于 ['ended']、chat.getHistoryForRecovery() 为 []，且 recordChatCompression 载荷携带 completedToolCallIds: ['ended']。再补硬救援的孪生用例：让压缩后的发送被拒绝，并断言 ids 在回滚后仍然存在。
注意 setCompletedToolCallIds 会丢弃在新历史中没有唯一应答的 id——(id) => completedToolCallBoundary(this.history, [id]) > 0（llm-chat.ts:2286）——因此 fixture 的压缩后历史必须为 'ended' 保留恰好一个 functionCall 和一个 functionResponse，否则断言会因为错误的原因而通过。从对应 setHistory 调用中移除第二个参数时，这两个新用例都必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| } | ||
| const apiHistory = buildApiHistoryFromConversation(resumed.conversation); | ||
| const { apiHistory, completedToolCallIds } = | ||
| buildSessionHistoryFromConversation(resumed.conversation); |
There was a problem hiding this comment.
[Suggestion] R1-6: One of six sites where the new completedToolCallIds threading has no test that reddens when the threading at that site is removed. reconcileDanglingPromptTerminals now feeds recorded boundary ids into detectTurnInterruption, but prompt-terminal-ledger.test.ts — 28 cases for this function — contains no goal_turn_end fixture and no completedToolCallIds reference, so the new classification is untested at this call site.
Daemon restart while a Goal turn's prompt terminal is still dangling, with a transcript ending in a recorded goal_turn_end: reverting this to buildApiHistoryFromConversation plus detectTurnInterruption(historyTail) leaves the suite green, and the behaviour that then ships is that the trailing user-role functionResponse entry is classified interrupted_prompt, so the ledger is stamped { terminal: 'interrupted', code: 'daemon_lost' } instead of { terminal: 'completed', stopReason: 'reconstructed_from_transcript' } — a Goal prompt that finished cleanly is reported to SSE clients as lost. This is the load-bearing path for the banner reported in the linked issue, so it is the one site here worth pinning first.
Witness:
mutation: revert to `detectTurnInterruption(historyTail)` (drop completedToolCallIds)
npx vitest run src/serve/prompt-terminal-ledger.test.ts -> 34 passed (34) # mutant survives
LIVENESS CONTROL: injected `throw new Error('PROBE-LIVENESS-CONTROL')` in the same
function surfaced repeatedly in that suite's output, proving the run executes the mutation
mechanical: `rg 'goal_turn_end|completedToolCallIds' prompt-terminal-ledger.test.ts` -> 0 hits
Add a ledger case whose transcript is assistant functionCall → tool_result functionResponse (same goalContext) → system/goal_turn_end with a matching permit, and assert the appended terminal record is completed.
The accumulator only honours a boundary when the immediately preceding material record is the tool_result carrying the same permit — previous?.type === 'tool_result' plus matching goalId / revision / turnId (packages/core/src/services/session-api-history.ts:110-126) — so the fixture's goal_turn_end record must directly follow its tool_result. That new case must go red when completedToolCallIds is dropped from the detectTurnInterruption(historyTail, ...) call.
中文说明
这是六处“新增 completedToolCallIds 传递没有被任何测试钉住”的站点之一。reconcileDanglingPromptTerminals 现在会把记录到的边界 ids 传给 detectTurnInterruption，但 prompt-terminal-ledger.test.ts（该函数有 28 个用例）中没有任何 goal_turn_end fixture，也没有任何 completedToolCallIds 引用，因此这个新的分类在该调用点未被测试。
场景：守护进程重启时某个 Goal 回合的 prompt terminal 仍悬空，且记录尾部是一条已写入的 goal_turn_end。把此处改回 buildApiHistoryFromConversation 加 detectTurnInterruption(historyTail)，套件仍然全绿；随之上线的行为是：尾部的 user 角色 functionResponse 条目被判定为 interrupted_prompt，账本被写入 { terminal: 'interrupted', code: 'daemon_lost' } 而不是 { terminal: 'completed', stopReason: 'reconstructed_from_transcript' }——一个干净结束的 Goal 提示会被当作“丢失”上报给 SSE 客户端。这正是所关联 issue 中横幅问题的关键路径，因此这几处里最值得优先钉住。
请补一个账本用例，其记录序列为：assistant functionCall → tool_result functionResponse（相同 goalContext）→ 携带匹配许可的 system/goal_turn_end，并断言追加的 terminal 记录为 completed。
注意累加器只有在紧邻的上一条实质记录是携带同一许可的 tool_result 时才承认边界——previous?.type === 'tool_result' 加上 goalId / revision / turnId 匹配（packages/core/src/services/session-api-history.ts:110-126）——因此 fixture 中的 goal_turn_end 记录必须紧跟在其 tool_result 之后。从 detectTurnInterruption(historyTail, ...) 调用中去掉 completedToolCallIds 时，该新用例必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| this.getChat().setHistory( | ||
| mcResult.history, | ||
| this.getChat().getCompletedToolCallIds(), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-7: One of six sites where the new completedToolCallIds threading has no test that reddens when the threading at that site is removed — and the highest-frequency one, since microcompactHistoryBeforeSend runs before every send. All 16 setHistory assertions inside describe('microcompaction FileReadCache invalidation') are toHaveBeenCalled() or not.toHaveBeenCalled(); the file's only three setHistory argument assertions are at 1959, 2005 and 5693. The 26 getCompletedToolCallIds: vi.fn().mockReturnValue(undefined) mock additions make those tests type-safe but value-blind, so nothing in the suite observes the second argument on this path.
Revert this hunk to setHistory(mcResult.history) and LlmChat.setHistory calls setCompletedToolCallIds(undefined), emptying the id set so completedToolCallBoundary returns 0 for the rest of the session — while every microcompaction test stays green. At runtime: a session resumed after a Goal turn end holds ['X']; the next prompt triggers microcompaction; the boundary is wiped; then stripOrphanedUserEntriesFromHistory on the retry path pops the settled user[functionResponse] together with the new prompt, because its loop guard is history.length > boundary with boundary === 0, and detectTurnInterruption re-classifies the settled turn as interrupted_prompt so continue re-sends a prompt the model already answered.
Witness:
BASELINE (intact): -t 'microcompaction FileReadCache invalidation' -> Tests 22 passed | 389 skipped (411)
MUTANT (this call -> `this.getChat().setHistory(mcResult.history);`):
describe -> Tests 22 passed | 389 skipped (411)
whole file -> Tests 411 passed (411) # survives every test in client.test.ts
LIVENESS + FIX FLIP (same mutant, with the suggested assertion applied to the test at 4120):
x microcompaction FileReadCache invalidation > disarms the fast-path for blanked files
instead of wiping the cache (issue #4239)
-> expected "spy" to be called with arguments: [ Any<Array>, [ 'mc-call-0' ] ]
Number of calls: 1
restore client.ts, keep the assertion -> Tests 22 passed | 389 skipped (411)
In one microcompaction test that already reaches the changed branch and uses makeReadFileResponses(6) — whose fixture already carries ids mc-call-0…5 at client.test.ts:4064 — replace getCompletedToolCallIds: vi.fn().mockReturnValue(undefined) with mockReturnValue(['mc-call-0']) and upgrade the existing expect(setHistory).toHaveBeenCalled() to expect(setHistory).toHaveBeenCalledWith(expect.any(Array), ['mc-call-0']).
If the witness is written against a real LlmChat instead of the mock, the chosen id must survive the completedToolCallBoundary(this.history, [id]) > 0 filter (llm-chat.ts:2284-2288), which yields a non-zero index only when the id appears exactly once inside a role: 'user' content — and microcompaction blanks old tool responses, so pick an id whose functionResponse the fixture keeps intact. The upgraded assertion must go red when the second argument here is removed; the flip above shows it does.
中文说明
这是六处“新增 completedToolCallIds 传递没有被任何测试钉住”的站点之一，也是频率最高的一处，因为 microcompactHistoryBeforeSend 在每次发送前都会运行。describe('microcompaction FileReadCache invalidation') 内全部 16 处 setHistory 断言都是 toHaveBeenCalled() 或 not.toHaveBeenCalled()；整个文件中仅有的三处 setHistory 参数断言位于 1959、2005 和 5693。新增的 26 处 getCompletedToolCallIds: vi.fn().mockReturnValue(undefined) mock 让这些测试在类型上安全、但在取值上失明，因此套件中没有任何东西观察该路径上的第二个参数。
把本 hunk 改回 setHistory(mcResult.history)，LlmChat.setHistory 就会调用 setCompletedToolCallIds(undefined)，清空 id 集合，使 completedToolCallBoundary 在会话余下时间返回 0——而所有微压缩测试仍然全绿。运行时表现为：一个在 Goal 回合结束后恢复的会话持有 ['X']；下一次提示触发微压缩；边界被清空；随后重试路径上的 stripOrphanedUserEntriesFromHistory 会把已结算的 user[functionResponse] 连同新提示一起弹出（因为其循环条件是 history.length > boundary，而此时 boundary === 0），并且 detectTurnInterruption 会把已结算回合重新判定为 interrupted_prompt，于是“继续”会重发一个模型已经回答过的提示。
请在一个已经走到 changed 分支、且使用 makeReadFileResponses(6) 的微压缩用例中（其 fixture 在 client.test.ts:4064 已带有 ids mc-call-0…5），把 getCompletedToolCallIds: vi.fn().mockReturnValue(undefined) 换成 mockReturnValue(['mc-call-0'])，并把已有的 expect(setHistory).toHaveBeenCalled() 升级为 expect(setHistory).toHaveBeenCalledWith(expect.any(Array), ['mc-call-0'])。
如果这个验证写成针对真实 LlmChat 而非 mock，所选 id 必须能通过 completedToolCallBoundary(this.history, [id]) > 0 过滤（llm-chat.ts:2284-2288）——只有当该 id 在某个 role: 'user' 内容中恰好出现一次时才会得到非零索引；而微压缩会清空旧的工具响应，所以要选一个其 functionResponse 被 fixture 完整保留的 id。移除此处第二个参数时，升级后的断言必须变红；上面的翻转结果已经证明它会。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| if (part.functionResponse?.id === toolCallId) results += 1; | ||
| } | ||
| } | ||
| return calls === 1 && results === 1; |
There was a problem hiding this comment.
[Suggestion] R1-8: One of six sites where the new boundary work has no test that reddens when it is removed — here a validator clause rather than a threading site. hasUniqueToolResult's calls === 1 condition is load-bearing for the shapes the accumulator admits, but no test in the new suite distinguishes it from its absence.
The clause is stricter than the live gate it mirrors: completedToolCallBoundary counts only part.functionResponse?.id and requires that count to be 1 in a role === 'user' content, never looking at functionCall parts, and the list persisted on the chat_compression record is exactly that live-filtered list. So the replay side re-filters with a rule the live side does not apply. That asymmetry could not be driven to a harmful outcome — the summary-compaction producer emits no user-role functionResponse at all, and provider-duplicate calls queue a second response record so calls === 2 arrives with results === 2 — and its direction is fail-closed, since discarding the id yields boundary = 0, precisely the pre-change classification. What is left is that the clause is unpinned, so a future edit to it passes every gate.
Witness:
mutation: `return calls === 1 && results === 1;` -> `return results === 1;`
src/services/session-api-history.test.ts -> Tests 8 passed (8)
src/services/session-transcript-reader.test.ts -> Tests 141 passed (141)
LIVENESS PROBE over the real builder + real completedToolCallBoundary (flips under the mutation):
INTACT: [calls2] replay=undefined live=["X"] DIVERGES=true
[calls0] replay=undefined live=["X"] DIVERGES=true
MUTANT: [calls2] replay=["X"] live=["X"] DIVERGES=false
[calls0] replay=["X"] live=["X"] DIVERGES=false
Add a case under describe('Goal turn end history metadata') that reaches the compression filter with a compressedHistory holding two functionCall parts for the boundary id and one functionResponse, asserting buildSessionHistoryFromConversation({ messages }).completedToolCallIds for whichever rule is chosen as canonical.
Whichever way the rule is settled, the two sides should share one predicate rather than maintaining a private second copy — export it from turn-interruption.ts and call it here. The live rule the replay side must agree with is boundaries.set(id, boundaries.has(id) || history[i].role !== 'user' ? 0 : i + 1) over const id = part.functionResponse?.id; (packages/core/src/core/turn-interruption.ts:57-64). The new case must go red when the chosen rule is removed from hasUniqueToolResult / completedToolCallBoundary — exactly the mutation the existing six cases survive today.
中文说明
这是六处“新增边界逻辑被移除后不会有任何测试变红”的站点之一——此处是一个校验子句，而非传递点。hasUniqueToolResult 的 calls === 1 条件对累加器所承认的形态是起作用的，但新套件中没有任何测试能把它与它不存在的情况区分开。
该子句比它所镜像的运行时闸门更严格：completedToolCallBoundary 只统计 part.functionResponse?.id，并要求它在某个 role === 'user' 内容中恰好出现 1 次，从不查看 functionCall 部分；而持久化到 chat_compression 记录上的列表正是这个经过运行时过滤的列表。因此回放侧用了一条运行时侧并不施加的规则再过滤一次。这种不对称无法被驱动到有害结果——摘要压缩的产出根本不含 user 角色的 functionResponse，而 provider 重复调用会追加第二条响应记录，于是 calls === 2 总是伴随 results === 2——并且其方向是失败关闭的：丢弃该 id 得到 boundary = 0，恰好就是改动前的分类。剩下的问题是该子句未被钉住，因此将来对它的修改能通过所有关卡。
请在 describe('Goal turn end history metadata') 下补一个用例，让压缩过滤分支接收到一个 compressedHistory，其中该边界 id 有两个 functionCall 部分和一个 functionResponse，并按最终选定的规范规则断言 buildSessionHistoryFromConversation({ messages }).completedToolCallIds。
无论规则最终如何统一，两侧都应共用同一个谓词，而不是各自维护一份私有副本——从 turn-interruption.ts 导出并在此调用。回放侧必须与之一致的运行时规则是 boundaries.set(id, boundaries.has(id) || history[i].role !== 'user' ? 0 : i + 1)，作用于 const id = part.functionResponse?.id;（packages/core/src/core/turn-interruption.ts:57-64）。当所选规则从 hasUniqueToolResult / completedToolCallBoundary 中移除时，该新用例必须变红——而这正是今天已有六个用例都能存活的那个变异。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| expect(setHistory.mock.calls[0][1]).toEqual(['goal-end']); | ||
| expect(compacted.at(-1)?.parts?.[0]?.functionResponse?.id).toBe( | ||
| 'goal-end', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-9: This is the only assertion that appears to pin "functionResponse.id survives memory-pressure compaction, so the Goal-turn boundary survives", and it reads back an entry microcompactHistory provably never touches. update_goal is not in COMPACTABLE_TOOLS and its response: {} carries no nested media, so collectCompactablePartRefs never marks that part and microcompactHistory returns the identical Content object via if (!partsToClean || !content.parts) return content;. compacted.at(-1) is therefore the same object literal this test pushed twenty lines earlier, and the assertion cannot fail for any reason connected to compaction.
So if the blanking branch were changed to construct functionResponse without spreading the original — dropping id — or a Goal tool were added to COMPACTABLE_TOOLS, then after any compact_history step setCompletedToolCallIds' filter would find no matching functionResponse.id and drop the id, getHistoryForRecovery() would return the whole history, and detectTurnInterruption would re-classify the finished Goal turn as interrupted_prompt with canContinue: true and re-submit the tool response to the model. This test stays green through all of it, and the unit suite for the blanking branch does not pin id retention either.
Witness:
mutation: microcompact.ts:733-738, spread `...stripNestedMedia(part.functionResponse)`
replaced by `name: part.functionResponse.name` (i.e. drop the id)
INTACT: PROBE blankedCount=2 blankedIds=["call_0","call_1"]
PROBE compacted.at(-1)={"role":"user","parts":[{"functionResponse":
{"id":"goal-end","name":"update_goal","response":{}}}]} sameObjectAsFixtureEntry=true
MUTANT: PROBE blankedCount=2 blankedIds=["<NO ID>","<NO ID>"] <- mutation is live, ids really are dropped
PROBE compacted.at(-1)={..."id":"goal-end"...} sameObjectAsFixtureEntry=true
<- the asserted entry is untouched, and is the identical object
memoryPressureMonitor.test.ts -> Tests 72 passed (72)
wider, same run: the mutant also survives
src/services/microcompaction/microcompact.test.ts -> Tests 82 passed (82)
Assert id retention on a blanked response rather than on the untouched update_goal one. The fixture already contains the right material: the seven read_file responses at lines 1389-1396 carry id: call_${i} and are compactable, so they are the entries that actually flow through the rewriting branch. Have the mock return getCompletedToolCallIds: () => ['call_0', 'goal-end'] so the setHistory.mock.calls[0][1] threading assertion also covers an id whose response was rewritten, and cover microcompact.test.ts too since the same mutant survives it.
COMPACTABLE_TOOLS at packages/core/src/services/microcompaction/microcompact.ts:40-51 does not contain update_goal, and microcompact.ts:699-700 returns the input Content untouched for unclassified parts — so any fixture intended to exercise blanking must use one of the ten listed tool names, not a Goal tool. The new assertion must go red under the mutation quoted above, which survives today.
中文说明
这是唯一一处看起来钉住了“functionResponse.id 能在内存压力压缩后存活，因此 Goal 回合边界也能存活”的断言，而它读回的是一个 microcompactHistory 可证明从未触及的条目。update_goal 不在 COMPACTABLE_TOOLS 中，其 response: {} 也不含嵌套媒体，因此 collectCompactablePartRefs 从不标记该部分，microcompactHistory 会经 if (!partsToClean || !content.parts) return content; 原样返回同一个 Content 对象。于是 compacted.at(-1) 就是本测试在二十行前推入的那个对象字面量，该断言不可能因为任何与压缩有关的原因而失败。
因此，如果清空分支被改成不使用展开构造 functionResponse（从而丢掉 id），或者某个 Goal 工具被加入 COMPACTABLE_TOOLS，那么在任何 compact_history 步骤之后，setCompletedToolCallIds 的过滤会找不到匹配的 functionResponse.id 而丢弃该 id，getHistoryForRecovery() 会返回整段历史，detectTurnInterruption 会把已完成的 Goal 回合重新判定为 interrupted_prompt 且 canContinue: true，并把工具响应重新提交给模型。而本测试在这一切之中始终为绿；清空分支自身的单元测试套件同样没有钉住 id 保留。
请把 id 保留的断言落在一个被清空过的响应上，而不是那个未被触及的 update_goal 上。fixture 中已有合适材料：1389-1396 行的七个 read_file 响应带有 id: call_${i} 且可被压缩，它们才是真正流经重写分支的条目。让 mock 返回 getCompletedToolCallIds: () => ['call_0', 'goal-end']，使 setHistory.mock.calls[0][1] 的传递断言也覆盖一个其响应被重写过的 id；同时也请覆盖 microcompact.test.ts，因为同一个变异在那里同样存活。
注意 packages/core/src/services/microcompaction/microcompact.ts:40-51 的 COMPACTABLE_TOOLS 不含 update_goal，且 microcompact.ts:699-700 对未分类部分会原样返回输入 Content——因此任何意图触发清空的 fixture 必须使用所列十个工具名之一，而不是 Goal 工具。在上面引用的变异下，新断言必须变红，而它今天是存活的。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const completedToolCallIds = accumulator | ||
| .getCompletedToolCallIds() | ||
| .filter((toolCallId) => hasUniqueToolResult(apiHistory, toolCallId)); |
There was a problem hiding this comment.
[Suggestion] R1-10: This exit re-filter duplicates an admission invariant SessionApiHistoryAccumulator.add() already maintains on every path, so no test can tell it apart from live code — and it silently absorbs a break in the primary guard. Every divergence route is already closed inside add(): a later record adding a second functionCall/functionResponse with the same id runs the delete loop at lines 154-161 on the same record that would grow the count, under the identical if (!record.message || record.subtype === 'realtime_message') return; guard that gates the append; a compression record replaces history and re-filters the payload's ids against the freshly assigned this.history at lines 135-141; and finish() returns this.history by reference unless stripThoughtsFromHistory is set, whose only true repo-wide is sessionService.test.ts:5007 while all three production callers pass no options.
The concrete cost is masking, not performance. Removing add()'s id-invalidation delete loop — a plausible future refactor — leaves the suite at 8/8 green, because this filter silently re-establishes the same result. The suite pins only the union of the two implementations, so neither guard is individually attributable, and the two copies of the admission rule can drift apart without anything noticing.
Witness:
unmodified PR: src/services/session-api-history.test.ts -> Tests 8 passed (8)
MUTANT A (lines 211-213 -> `const completedToolCallIds = accumulator.getCompletedToolCallIds();`):
session-api-history + session-transcript-reader -> Tests 149 passed (149)
src/services + src/core/session-recovery.test.ts -> Test Files 69 passed (69); Tests 3076 passed | 2 skipped
MUTANT B (add()'s id-invalidation delete loop removed, exit filter intact):
session-api-history.test.ts -> Tests 8 passed (8) <- the break is INVISIBLE
MUTANT A+B (both guards gone) — liveness control:
x invalidates a boundary when its tool id is reused later -> expected [ 'finish' ] to be undefined
x retains earlier boundaries and removes only a reused tool id
-> expected [ 'finish', 'finish-2' ] to deeply equal [ 'finish-2' ]
Tests 2 failed | 6 passed (8)
Pick one home for the rule. Either delete these three lines and return [...accumulator.getCompletedToolCallIds()] (keeping the length > 0 conditional spread), so add() is the single place the invariant is established and MUTANT B reddens — i.e. the surviving guard becomes pinned — or keep the filter and make it load-bearing by adding the only case it can act on: a stripThoughtsFromHistory: true call over a conversation whose ending functionResponse part carries thought: true, asserting the id is dropped.
LlmChat.setCompletedToolCallIds already re-filters whatever the restore hands it (packages/core/src/core/llm-chat.ts:2284-2288), so dropping the exit filter cannot leak an id that has no functionResponse in the seeded history. That gate counts only part.functionResponse?.id and does not require a matching functionCall, i.e. it is looser than hasUniqueToolResult; this change must not be used to widen or narrow that difference. Under the keep-and-pin option, the new stripThoughtsFromHistory: true case must go red when these three lines are removed.
中文说明
这个出口处的再过滤重复了 SessionApiHistoryAccumulator.add() 在每条路径上都已经维护的准入不变量，因此没有任何测试能把它与有效代码区分开——而且它会静默吸收主守卫被破坏的情况。所有可能产生分歧的路径在 add() 内部就已闭合：后续记录若为同一 id 追加第二个 functionCall/functionResponse，会在“使计数增长的那条记录”上运行 154-161 行的删除循环，且该循环受与追加相同的 if (!record.message || record.subtype === 'realtime_message') return; 守卫约束；压缩记录会替换历史，并在 135-141 行针对新赋值的 this.history 重新过滤载荷中的 ids；而 finish() 除非设置了 stripThoughtsFromHistory，否则按引用返回 this.history——全仓库唯一一处 true 在 sessionService.test.ts:5007，三个生产调用方都不传选项。
真正的代价是“掩盖”，不是性能。移除 add() 的 id 失效删除循环（一个相当可能的未来重构）后，套件仍然是 8/8 全绿，因为这个过滤器会静默地重新得到同样的结果。套件钉住的只是两份实现的并集，因此任一守卫都无法被单独归因，两份准入规则的副本可以在无人察觉的情况下逐渐分歧。
请为这条规则选定唯一归属。要么删除这三行并返回 [...accumulator.getCompletedToolCallIds()]（保留 length > 0 的条件展开），让 add() 成为该不变量唯一的建立处，此时 MUTANT B 会变红——也就是存活下来的那个守卫被钉住了；要么保留该过滤器并让它真正起作用，补上它唯一能发挥作用的用例：以 stripThoughtsFromHistory: true 调用，会话末尾的 functionResponse 部分带有 thought: true，并断言该 id 被丢弃。
注意 LlmChat.setCompletedToolCallIds 已经会对恢复交给它的任何内容再过滤一次（packages/core/src/core/llm-chat.ts:2284-2288），因此删除出口过滤不会泄漏一个在播种历史中没有 functionResponse 的 id。该闸门只统计 part.functionResponse?.id，并不要求存在匹配的 functionCall，也就是说它比 hasUniqueToolResult 更宽松；本次改动不应用来扩大或缩小这一差异。若选择“保留并钉住”，那么移除这三行时，新增的 stripThoughtsFromHistory: true 用例必须变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
4 participants
What this PR does
Persists a structured end record when ACP intentionally ends a Goal turn after a terminating tool result. Recovery uses these records to recognize completed turns and to retry only later unanswered input. The records stay outside model messages and do not require another model request.
Why it's needed
A Goal can deliberately end with a tool response so independent verification can proceed. Recovery currently interprets that user-role response as unfinished work and offers Continue after reload. A durable host decision distinguishes this case from an actual interruption without inserting instructions into the next model prompt.
Ended turns are associated with their Goal permit and normalized tool-result IDs. The active transcript branch supplies the recovery boundaries. Startup refresh, compaction, memory-pressure cleanup, forks, and rewind retain only boundaries whose tool results still exist. Keeping all retained boundaries allows rewind to expose an earlier completed turn. Cancellation before settlement, failed recording, ambiguous IDs, and incomplete history remain conservative.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: an ACP host regression on the original base returned
interrupted_prompt / canContinue: trueafter a normalend_turnand Goal iteration settlement. After: the same path returnsclean / canContinue: false; later unanswered input remains recoverable.Validation at
1dd94e73b8f2: root build, bundle, typecheck, and changed-file ESLint/Prettier passed. Core: 10 files / 1,359 tests passed. CLI: 5 files / 1,537 tests passed, 1 skipped. The original host regression passes independently. A compiled-code test using real JSONL recording passed seven recovery/fork/rewind scenarios through both loaders; API-history JSON was byte-for-byte identical before and after recording the end event. An isolated built-CLI/goalsmoke also passed. No live-model or cache-hit-rate result is claimed.Tested on
Environment (optional)
Node 22.17.0, isolated checkout and CLI runtime.
Risk & Scope
Linked Issues
Fixes #11914. Supersedes the closed #11915.
中文说明
变更内容
当 ACP 在终止型工具结果后主动结束 Goal 回合时，持久化一条结构化结束记录。恢复逻辑使用这些记录识别已完成回合，并只重试后续未回答的输入。记录不进入模型消息，也不需要额外模型请求。
修复原因
Goal 可以有意停在工具返回处，让独立校验继续进行。恢复逻辑目前将这种 user 角色的返回视为未完成工作，导致重载后错误显示“继续执行”。持久化宿主的结束决策，可以区分这种情况与真实中断，无需向下一次模型请求插入指令。
结束回合关联具体 Goal 回合标识和规范化后的工具结果 ID，活动记录分支提供恢复边界。启动上下文刷新、压缩、内存压力清理、fork 和 rewind 只保留对应工具结果仍存在的边界。保留全部有效边界，使回退后仍能识别更早的已完成回合。结算前取消、记录写入失败、ID 歧义或历史不完整时，继续采用保守判断。
审阅者测试计划
验证方法
修复前后证据
修复前：原始基线上的 ACP 宿主回归测试在正常
end_turn和 Goal 回合结算后，仍返回interrupted_prompt / canContinue: true。修复后：同一路径返回clean / canContinue: false；后续未回答的输入仍可恢复。在
1dd94e73b8f2上验证：根目录构建、bundle、类型检查及改动文件的 ESLint/Prettier 均通过。核心包 10 个文件、1,359 项测试通过；CLI 5 个文件、1,537 项测试通过，1 项跳过。原始宿主回归也独立通过。使用编译产物和真实 JSONL 写入的测试，在两种加载器上通过七个恢复、fork 和 rewind 场景；写入结束事件前后的 API 历史 JSON 逐字相同。隔离环境中的构建版 CLI/goal冒烟检查也通过。这些结果不代表真实模型或缓存命中率测量。测试平台
环境
Node 22.17.0；隔离的代码检出和 CLI 运行环境。
风险与范围
关联问题
修复 #11914，替代已关闭的 #11915。