fix(cli): keep in-flight tool calls pending during live session replay - #9705
Conversation
github-actions
Bot
added
the
review/self-reported
|
Re-run on the replaced branch — the PR is now a single focused fix instead of the withdrawn flush attempt, so this supersedes the earlier gate pass. Template: complete ✓ — all sections present, including the reviewer test plan and the bilingual detail block. Problem: observed bug, not theoretical hardening. #9704 documents a live session restore finalizing a still-running tool call as the permanent "Tool result missing from saved history" failure. The author reproduced it, and the maintainer's local verification round (2026-08-24) reproduced it independently A/B: the base build shows the phantom finalize, this head does not. For a race window, that is about as good as the evidence gets. Direction: aligned. Session-restore correctness on the daemon/ACP surface is core product, and the reader-side framing is right — the earlier writer-side flush attempt was withdrawn with a sound analysis (the scheduler records nothing until the tool batch reaches a terminal state, so there was nothing to flush). The reference CLI's changelog shows a steady stream of resume/restore fixes — no exact match for this window, but the area is clearly relevant. Size: 55 production lines (acpAgent.ts +42, history-replay-page.ts +10, history-replayer.ts +2/−1) vs 274 test lines, 0 generated/schema. Not core module paths ( Approach: minimal, and as far as the description plus diff structure show, the right shape — gate only the ungated Risk: Stage 1e matches — every changed file sits under Moving on to code review. 🔍 中文说明在已替换的分支上重跑——本 PR 现在是一个聚焦的单次修复，不再是已撤回的 flush 尝试，本评论取代之前的门禁评估。 **模板：**完整 ✓——各小节齐全，包含评审测试计划与中文对照。 **问题：**已观测到的 bug，不是理论性加固。#9704 记录了 live session restore 把仍在运行的 tool call 固化为永久 "Tool result missing from saved history" 失败。作者有复现，维护者本地验证轮（2026-08-24）也独立做了 A/B 复现：base 构建出现幻影 finalize，本 head 不再出现。对一个竞态窗口来说，这已经是几乎最好的证据。 **方向：**对齐。daemon/ACP 面上的 session restore 正确性属于核心产品，且读取侧的定位是对的——早先写入侧的 flush 尝试已带着合理分析撤回（scheduler 在 tool batch 到达终态前不记录任何内容，所以没有可 flush 的东西）。参考 CLI 的 changelog 里 resume/restore 修复一直不少——没有与本窗口完全对应的条目，但该领域显然相关。 **规模：**生产代码 55 行（acpAgent.ts +42、history-replay-page.ts +10、history-replayer.ts +2/−1），测试 274 行，生成/schema 0 行。未触及核心模块路径（ **方案：**最小化，且从描述加 diff 结构看形态正确——只对无门禁的 **风险：**Stage 1e 命中——所有改动文件都在 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first: the failure is on the reader side — bulk replay finalizes the trailing unmatched call unconditionally — so I would make finalization conditional on the ungated The PR matches that proposal essentially one-to-one, and the details hold up at head
No critical findings, no AGENTS.md violations. One observation, non-blocking and by design: a turn that settles between the two samples keeps the call pending for one extra FlowsequenceDiagram
participant P1 as Client
participant P2 as QwenAgent
participant P3 as Session
participant P4 as HistoryReplayer
P1->>P2: qwen/session/loadUpdates
P2->>P3: sample isTurnIdle() before the transcript read
P2->>P3: read transcript via loadSession
P2->>P3: sample isTurnIdle() again at replay time
P2->>P4: replay with finalizeDangling = idleBeforeRead AND idleAtReplay
P4-->>P1: trailing call stays pending, live stream delivers the result
Files changed (6 of 6)
Test evidence — this PR's own CI on
|
| Check | Conclusion |
|---|---|
Test (ubuntu-latest, Node 22.x) |
✅ success |
Classify PR |
✅ success |
Dependency CVE audit |
✅ success |
Secret scan (TruffleHog) |
✅ success |
precheck-pr / precheck |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
Real daemon E2E / Java 11 |
✅ success |
Test (macos-latest, Node 22.x) |
⏭️ skipped |
Test (windows-latest, Node 22.x) |
⏭️ skipped |
Integration Tests (CLI, No Sandbox) |
⏭️ skipped |
Beyond CI: maintainer @wenshao's local verification round (2026-08-24, comment in this thread) reports a real end-to-end A/B on this exact head — base reproduces the phantom finalize, head keeps the call pending with the result arriving via the live stream, a trial merge into current main matches head, and a mutation matrix shows all three new guard tests fail with the guard removed (zero survivors). That is the maintainer's evidence, attributed as such — this review did not re-run it.
Not verified here: Windows/Linux behavior (fork-PR CI skips; author's table says macOS only), and goal/cron/notification turns driven live — they are pinned at predicate level plus unit tests only.
Sandboxed verification would settle the remaining gap if wanted before merge: @qwen-code /verify for an independent A/B re-run of the in-flight finalize claim (the author has write access, so this is the standard lane) — the central claim itself is already substantiated by the maintainer's A/B round and the guard-pinning tests, so this is a belt-and-braces line, not a missing prerequisite.
中文说明
代码审查
**先给独立方案：**失败在读取侧——批量 replay 无条件 finalize 尾部未匹配的 call——所以我会只在无门禁的 qwen/session/loadUpdates 路径上，以"本进程中该 session 是否仍有活跃 turn"为条件控制 finalize。单次采样不够（turn 可能在 transcript 读取期间启动或结束），因此采样两次：读取前一次、replay 时一次。有门禁的 live restore 路径必须保持无条件：其 close gate 已 drain 掉 turn 并阻止新 turn，在那里采样结果结构性地"非 idle"，会把真正被遗弃的 call 永远卡在 pending。
PR 与该方案基本一一对应，且在 head a55295ab 上细节成立：
- **三个生产调用点都处理正确。**有门禁的 live restore 显式传
finalizeDangling: true——正确：withLiveSessionRestore依次assertCanStartTurn→beginClose()→ 带SESSION_DRAIN_TIMEOUT_MS的 drain，drain 超时会在 replay 前拒绝，此处的尾部未匹配 call 确实是被遗弃的。冷恢复分支（从投影新建 session）不传、走默认——那是已披露、由 Follow-up: live session load during a long-running tool hits the 30s restore drain timeout (#9704 remaining window) #9773/Follow-up: prompt terminal ledger review suggestions (PR #9426 round 3) #9483 跟踪的剩余窗口，不是遗漏。只有无门禁的loadUpdates读取加守卫。 - 谓词覆盖与描述一致。
isTurnIdle()即!closing && !#hasActiveTurn()，#hasActiveTurn()覆盖pendingPrompt、goal、cron、notification turn（含 history mutation 与 completion 处理）——客户端 prompt 与全部自主 turn 家族都在守卫内，而closing项正是让有门禁路径结构性为 false 的原因。 - 默认行为处处保留。
HistoryReplayer.replay里?? true，collectHistoryReplayUpdates仅在显式设置时透传——transcript 分页及其他 replay 消费方行为不变。session-transcript 分页路径本就有一个类似的两点守卫（!activePromptBeforeRead && !activePromptCalls.has(...)），本改动是沿用既有模式而非新造。 - **两次采样各自被单独钉住。**settle-during-read 与 start-during-read 测试把
isTurnIdle的翻转锚定在 mock 的loadSession内部（读取边界上），因此任何把采样点挪过读取操作的突变都无法存活。默认 finalize 与门禁路径的显式true也都有断言。debug 诊断用模板字符串（debugLogger 不做 printf 替换——注释说明了原因，测试钉住展开后的字符串）。
无关键问题，无 AGENTS.md 违规。一个非阻塞的、设计内的观察：在两次采样之间 settle 的 turn 会让该 call 多保持一轮 pending——朝安全方向保守，且测试有意钉住该行为。
测试证据
无人值守运行：本审查不执行 PR 代码；以上证据为通过 API 获取的 PR 自身 CI，另加注明的维护者本地轮。该 head 上所有 check 已完成——无失败。Linux 单测套件绿。macOS/Windows 单测与无沙箱集成通道按 fork PR 惯例跳过——即下方点名的平台缺口。
CI 之外：维护者 @wenshao 的本地验证轮（2026-08-24，见本线程评论）报告了针对该 head 的真实端到端 A/B——base 复现幻影 finalize，head 保持 pending 且结果经 live stream 到达，试合并与 head 一致，突变矩阵显示移除守卫后三个新测试全红（零幸存）。那是维护者的证据，如实注明——本审查未重跑。
此处未验证：Windows/Linux 行为（fork PR CI 跳过；作者表格为仅 macOS），以及 goal/cron/notification turn 的真实驱动——目前仅在谓词层与单测钉住。
如需在合并前补齐剩余缺口，可用沙箱验证：@qwen-code /verify 独立重跑 in-flight finalize 声明的 A/B（作者有写权限，走标准通道）——核心声明本身已由维护者 A/B 轮与钉住守卫的测试证实，故该行是双保险，不是缺失的前置条件。
— Qwen Code · qwen3.8-max
Reviewed at a55295ab895f762e02ac1c60d4db7f57b0bb34f2 · re-run with @qwen-code /triage
|
Confidence: 4/5 — clean across every stage; the one honest reservation is structural (fork-PR CI skips the macOS/Windows suites and the integration lane), which no review lane on this side can close. Stepping back: this PR is what the process looks like when it works. The first attempt (writer-side flush) was reviewed, reproduced against, and withdrawn by the author with a correct analysis instead of being defended. The replacement does the minimal right thing on the reader side — a two-sample turn-idle guard on exactly the one ungated path, explicit finalization on the gated path where the guard would be wrong, defaults preserved everywhere else, and the remaining windows (cold restore, drain timeout) honestly disclosed and tracked in #9773/#9483 rather than silently scoped in. My independent proposal before reading the diff was the same shape, and I did not find a simpler path on reading it. The tests pin the change rather than decorating it: each of the two samples fails individually under mutation (anchored at the read boundary, not keyed by call order), the default behavior is pinned, and the gated-path exception is pinned. A maintainer then verified the actual behavior end-to-end on this exact commit — A/B against base, trial merge into current main, mutation matrix with zero survivors — and approved. CI on the head is fully green with nothing pending. Noted, non-blocking: the Windows/Linux unit suites and the sandboxless integration lane are skipped for fork PRs, and the autonomous turn families are verified at predicate level plus unit tests rather than driven live. Neither is resolvable from this lane, and neither changes what the diff does. Approving, pinned to the reviewed commit. ✅ 中文说明置信度：4/5——各阶段都干净；唯一诚实的保留是结构性的（fork PR 的 CI 跳过 macOS/Windows 套件与无沙箱集成通道），这一侧的任何审查通道都无法补上。 退一步看：这个 PR 展示了流程应有的样子。第一次尝试（写入侧 flush）被审查、被复现反驳，作者带着正确的分析撤回，而不是为它辩护。替代方案在读取侧做了最小且正确的事——只在唯一无门禁的路径上加两点采样的 turn-idle 守卫；在有门禁的路径上显式 finalize（那里加守卫反而是错的）；其余各处保留默认；剩余窗口（冷恢复、drain 超时）如实披露并交给 #9773/#9483 跟踪，而不是悄悄扩大范围。我在读 diff 之前的独立方案就是同一形态，读完也没有找到更简的路径。 测试钉住的是改动本身而非装饰：两次采样各自在突变下失败（锚定在读取边界，而不是按调用次序取键），默认行为有断言，门禁路径的例外也有断言。随后一位维护者在该 commit 上端到端验证了真实行为——对 base 的 A/B、对当前 main 的试合并、零幸存的突变矩阵——并批准。head 上的 CI 全绿且无待完成项。 记录在案、不阻塞：fork PR 跳过 Windows/Linux 单测与无沙箱集成通道；自主 turn 家族是在谓词层加单测验证，而非真实驱动。两者都不是本通道能解决的，也不改变这个 diff 的实际作用。 批准，锚定在被审查的 commit 上。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs some rethinking — the lease can't serve as a drain probe here. Full evidence in my Stage 2 comment above, but the short version:
SessionWriterLease.acquire()on a held lock throwsSessionWriterConflictError(a sibling ofSessionWriterUnavailableError), so the retry loop never catches it — in the #9704 scenario (live session + concurrent load) the load now throws "This session is already open in another Qwen process." instead of returning a degraded transcript.- The writer's lease is held for the whole session lifetime (acquired in
activateChatRecording, released only incloseOnce), never on flush — so even with the right error class, the drain can only time out and read the same possibly-incomplete file. - With the experimental lease enabled, the projection read already happens after the restoring agent's own lease acquire, so the second acquire conflicts with this process's own live lock.
- With the lease disabled (default), the drain is a no-op.
@yiliang114 lighter directions that could work are in the Stage 2 notes (replay tolerance for a trailing in-flight call, serving live-session loads from memory, or a real flush-barrier primitive). Happy to re-run if any step of the lease analysis is wrong. 🙏
🩺 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 |
9960b15 to
600392d
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）为单个提交。 |
eaf15f7 to
3479e49
Compare
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical — the branch is stacked on #9690 and the described fix is not what the diff contains
Verified at head 3479e493.
Where / trigger — the branch fix/issue-9704-writer-drain-on-cold-restore was cut from #9690's fix/issue-8993-git-prerequisite branch. 11 of the 12 commits (and the bulk of the diff against main: extension/github.ts, extension/archive-safety.ts, extensionManager.ts, the extension docs and tests) belong to #9690. The only commit belonging to this PR is 3479e493 itself — a single await this.chatRecordingService?.flush() added after this.recordToolResults(completedCalls) in coreToolScheduler.ts (~line 6076) plus the matching test mock.
Impact — merged as-is, this PR would pull #9690's extension work in under this PR's title/attribution, and any later change to #9690 creates a conflict/duplication mess. Additionally, the PR body describes SessionService.waitForWriterDrain() coordinating the cold-restore read path with the writer queue — that code does not exist anywhere in the diff (grep for waitForWriterDrain returns nothing). The actual change is writer-side (flush at tool-batch completion in the scheduler), which is a different mechanism than the one described, so reviewers and the linked issue (#9704) are being told the wrong story about what fixes it. Note also that the earlier automated CHANGES_REQUESTED review targeted the previous lease-probe iteration (SHA 9960b153, since force-pushed) and does not apply to the current head; the current flush approach has had no review pass yet.
Fix direction —
- Rebase the branch onto
mainand keep only the flush commit (3479e493's changes), so the diff contains just thecoreToolScheduler.ts+ test lines; if the intent was to stack on #9690, say so explicitly in the PR description and make the merge order dependency clear instead. - Rewrite the PR body to describe the actual implementation: flushing the recording queue after each completed tool batch narrows the #9704 window where results are queued but not yet persisted; state honestly which windows remain (e.g. a cold restore mid-tool-execution still sees the in-flight call without a result, which is expected for a running turn) and why the restore-path
waitForWriterDrainidea was dropped. - On the merits for the re-review round: justify the per-batch
flush()I/O cost on tool-heavy turns, and confirm the-precording path drains through this same scheduler site.
Critical —— 分支叠在 #9690 之上，且描述的修复与 diff 实际内容不符
已在 head 3479e493 核实。
位置 / 触发条件 —— 分支 fix/issue-9704-writer-drain-on-cold-restore 是从 #9690 的 fix/issue-8993-git-prerequisite 分支上切出的。12 个提交中有 11 个（以及对 main 的 diff 主体：extension/github.ts、extension/archive-safety.ts、extensionManager.ts、扩展文档与测试）属于 #9690。本 PR 自己的提交只有 3479e493——在 coreToolScheduler.ts（约 6076 行）this.recordToolResults(completedCalls) 之后加了一行 await this.chatRecordingService?.flush()，外加对应测试 mock。
影响 —— 按现状合并会把 #9690 的扩展改动以本 PR 的标题/归属带进来；#9690 后续任何改动都会造成冲突或重复。此外，PR 描述写的是 SessionService.waitForWriterDrain() 在冷恢复读取路径上与写队列协调——该代码在 diff 中完全不存在（全 diff 检索 waitForWriterDrain 为零）。实际改动是写入侧的（调度器在工具批完成时 flush），与描述的机制不同，等于给评审者和关联 issue（#9704）讲了错误的修复故事。另请注意：此前自动评审的 CHANGES_REQUESTED 针对的是被 force-push 替换掉的旧版 lease 探测实现（SHA 9960b153），不适用于当前 head；当前 flush 方案尚未经过任何完整评审。
修复方向 ——
- 将分支 rebase 到
main，只保留 flush 提交（3479e493的改动），使 diff 仅含coreToolScheduler.ts+ 测试两行；如果本意就是叠加在 #9690 之上，请在 PR 描述中明确说明依赖关系与合并顺序，而不是维持现状。 - 重写 PR 描述，描述真实实现：在每个工具批完成后 flush 录制队列，收窄 #9704 中"结果已入队但未落盘"的窗口；如实说明剩余窗口（例如冷恢复发生在工具执行中时仍会看到在途调用无结果，这对运行中的轮次是预期行为），以及为何放弃在恢复路径做
waitForWriterDrain。 - 重新评审时需说明：每个工具批都
flush()在工具密集轮次的 I/O 开销是否可接受；并确认-p模式的录制路径确实经由该调度器位点落盘。
3479e49 to
8235433
Compare
yiliang114
changed the title
Addressed all review feedback@qwen-code-ci-bot's lease analysis (stale)The reviewed code ( @qqqys's three items
CIOld commit CI had 1 ubuntu test failure — the new commit needs a fresh CI run. @qqqys could you re-review when you have a chance? |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical — the flush line breaks 8 pre-existing scheduler tests (reproduced deterministically on current main + this one line)
First: the previous round's findings are resolved — the branch is rebased onto main (diff is exactly the two lines), the title and body now describe the real change, and the remaining windows are disclosed. Thank you.
However, CI on the new head 82354333 is red, and the failures are caused by this PR's one production line, verified as follows:
Where — packages/core/src/core/coreToolScheduler.ts (~line 6079): await this.chatRecordingService?.flush() inserted after this.recordToolResults(completedCalls) in the batch-completion path.
Evidence — CI run 32549296900, Test (ubuntu-latest, Node 22.x): coreToolScheduler.test.ts 8 failed / 372, plus a 15s timeout in nonInteractiveToolExecutor.test.ts. I reproduced this locally on current upstream main (7703d1c3):
- main alone: the failing tests pass;
- main + only this one line applied: the exact same 8 tests fail (
keeps a tool-produced timeout as an error after a later parent abort,aborts immediately when the parent signal is already aborted before scheduling,keeps PostToolUse additionalContext intact after truncating oversized output,appends PostToolUse additionalContext AFTER truncation...,deterministically bounds tool outputs when a batch exceeds the budget,applies canonical legacy tool names to the deny-list fallback,should cancel a tool call if the signal is aborted before confirmation,telemetry spans > pre-aborted signal: terminalizes before validation or execution) — full-file run matches CI 8/8; - removing only this line again: they pass.
Why this is not a test-only artifact — in most of these tests chatRecordingService is undefined, so the ?. chain short-circuits and no method is ever called: it is the extra await suspension itself that changes the outcome. All the failing scenarios are the scheduler's abort/deny/truncation/budget completion paths, and onAllToolCallsComplete ends up never being called. The same suspension happens in production on exactly those paths, so this is a behavioral regression risk for real sessions, not merely a mock gap. (The "372 tests pass" in the PR body does not hold on CI's Linux runners; the local macOS run apparently masked it.)
Fix direction —
- Find out why one extra microtask suspension in the completion path drops
onAllToolCallsCompleteon the abort/deny/truncation/budget paths — there is very likely a latent completion/queue-drain race this await exposes; if so, fix that race rather than working around it. - If the flush genuinely cannot sit on the turn-critical path, move it off: e.g. kick it off without awaiting in the completion sequence (the whole point of the fix is persistence eventually before a cold restore, not synchronously before
onAllToolCallsComplete), or flush at a later safe point (turn end / before the session is releasable). - Either way, the full
coreToolScheduler.test.tsandnonInteractiveToolExecutor.test.tssuites must be green in CI before this can merge.
Critical —— flush 这一行破坏了 8 个既有调度器测试（已在当前 main + 仅此一行上确定性复现）
首先：上一轮的意见已全部落实——分支已 rebase 到 main（diff 恰为两行）、标题与正文已描述真实改动、剩余窗口也已如实披露，感谢。
但新 head 82354333 的 CI 红，且失败由本 PR 的唯一生产行导致，验证过程如下：
位置 —— packages/core/src/core/coreToolScheduler.ts（约 6079 行）：在批完成路径 this.recordToolResults(completedCalls) 之后插入的 await this.chatRecordingService?.flush()。
证据 —— CI run 32549296900，Test (ubuntu-latest, Node 22.x)：coreToolScheduler.test.ts 8 失败 / 372，另有 nonInteractiveToolExecutor.test.ts 15 秒超时。我在当前 upstream main（7703d1c3）上本地复现：
- 仅 main：失败用例全部通过；
- main + 仅加这一行：同样 8 个用例失败（
keeps a tool-produced timeout as an error after a later parent abort、aborts immediately when the parent signal is already aborted before scheduling、keeps PostToolUse additionalContext intact after truncating oversized output、appends PostToolUse additionalContext AFTER truncation...、deterministically bounds tool outputs when a batch exceeds the budget、applies canonical legacy tool names to the deny-list fallback、should cancel a tool call if the signal is aborted before confirmation、telemetry spans > pre-aborted signal: terminalizes before validation or execution）——整文件运行与 CI 的 8 个失败完全一致； - 仅去掉这一行：全部恢复通过。
为什么这不是纯测试问题 —— 其中多数测试里 chatRecordingService 为 undefined，?. 链直接短路、根本没有方法被调用：改变结果的是这个 await 挂起本身。失败场景全部是调度器的 abort/拒绝/截断/预算完成路径，且 onAllToolCallsComplete 最终从未被调用。同样的挂起在生产环境的这些路径上也会发生，因此这是真实会话的行为回归风险，而不只是 mock 缺口。（PR 正文中"372 tests pass"在 CI 的 Linux runner 上不成立；本地 macOS 运行显然掩盖了该问题。）
修复方向 ——
- 查明为何完成路径上多一个微任务挂起就会在 abort/拒绝/截断/预算路径丢掉
onAllToolCallsComplete——很可能存在一个被该 await 暴露的完成/队列排空竞态；若是，请修复竞态本身而不是绕过它。 - 如果 flush 确实不能放在轮次关键路径上，请移出：例如在完成序列中不等待地发起（本修复的目标是"冷恢复前最终落盘"，而非"
onAllToolCallsComplete前同步落盘"），或改在更晚的安全点（轮次结束 / 会话可释放前）执行。 - 无论哪种方式，
coreToolScheduler.test.ts与nonInteractiveToolExecutor.test.ts全套必须在 CI 上转绿才能合并。
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.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Test Plan (not a blocker): 372 tests pass — this review observed 20766, 1679, 22938, 1641, 495, 4063, 595 passed.
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
I reproduced both the reported timing window and the current PR regressions, then withdrew the implementation with additive commits (no force-push). During a deliberately pending real scheduler tool call, no result had reached the recorder ( The previous head also failed 8 of 372 scheduler tests and timed out 1 of 10 non-interactive executor tests because the extra await could prevent the completion callback. After reverting the two-line implementation, both files pass in full (372/372 and 10/10), and the repository build and typecheck pass. The PR now has no net code diff and no longer claims to fix #9704. The remaining issue needs ownership-aware cold-restore mediation: prove and attach to a live writer when one exists, or distinguish it from a dead writer using durable identity. A blanket “trailing means running” replay rule would misreport genuinely interrupted sessions. |
yiliang114
changed the title
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 18 passed · 0 failed · 18 total Flakiness gate: not applicable — no runnable changed test files (0 out-of-scope file(s) noted in the log) 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：18 通过 · 0 失败 · 18 总计 抖动门：不适用 — no runnable changed test files (0 out-of-scope file(s) noted in the log) Verification reportPR #9705 Deep Verification —
|
| Cell | Oracle | Result |
|---|---|---|
git diff --exit-code HEAD^1..HEAD (effective diff into main) |
exit code 0, empty output | exit 0, 0 files changed |
| Merge tree OID vs base-tip tree OID | OID equality (same OID ⇒ byte-identical content) | a4d7b6ea… == a4d7b6ea… — identical tree objects |
HEAD^2 vs snapshot headRefOid |
OID equality | c6779602f7… == c6779602f7… |
PR head's coreToolScheduler.ts vs base's |
git diff HEAD^2 HEAD^1 -- <file> empty |
empty — the PR head's scheduler file is byte-identical to base |
The PR head's own tree (3854416f…) differs from the current base tree only in files that landed on main after this branch forked (the qwen-autofix workflow updates, the #9623 review/convergence work, and docs/design/9152-…md); the merge keeps base's content for all of them, which is why the merge tree equals the base tree exactly. Witness: 01-net-diff-zero-tree-identity.png.
Cell table — head gates (claimed in the PR body's "Verification after the revert")
| Arm | Suite | Body's claim | Measured | Exit |
|---|---|---|---|---|
| HEAD | coreToolScheduler.test.ts |
372 passed | 372 passed | 0 |
| HEAD | nonInteractiveToolExecutor.test.ts |
10 passed | 10 passed | 0 |
| HEAD (rerun, both suites) | — | — | 382 passed | 0 |
| MUTANT (reconstructed flush) | coreToolScheduler.test.ts |
(8 failed, per body) | 9 failed | 363 passed | 1 |
| MUTANT (reconstructed flush) | nonInteractiveToolExecutor.test.ts |
1 timeout | 1 failed — Test timed out in 15000ms | 9 passed |
1 |
| MUTANT (rerun, both suites) | — | — | 10 failed | 372 passed, failure set byte-identical to first mutant run | 1 |
Witnesses: 02-ab-head-green-mutant-red.png (cell table), 03-mutant-failure-signature.png (raw vitest output). Raw logs: logs/head-*.log, logs/mutant-*.log, logs/*-rerun.log.
Mutant construction. Commit 823543336bde… (the flush fix being withdrawn) is below the depth-2 shallow boundary and not fetchable here, so the mutant is a reconstruction from its commit message, applied verbatim: insert await this.chatRecordingService?.flush(); immediately after this.recordToolResults(completedCalls); in the scheduler's completion path (coreToolScheduler.ts:6079). The harness asserts the anchor is unique and the result is exactly +1/−0 (flush-mutation.mjs apply), and the restore asserts the index tree hash returns to a4d7b6ea… (flush-mutation.mjs restore).
Failure mechanism (matches the body's account, one unified cause). All 7 no-recorder failures are ordering failures: these tests await scheduler.schedule(...) and immediately assert onAllToolCallsComplete was called — an ordering that holds on the clean tree (the callback fires at or before schedule() resolves) and breaks with one extra suspension inserted between recordToolResults and the callback (await this.chatRecordingService?.flush() evaluates to await undefined when no recorder is wired, but still defers the callback by a hop, letting schedule()'s resolution land first). In the two tests that wire a partial recorder mock ({ recordToolResult }, no flush member), the call additionally throws TypeError: ...flush is not a function; that rejection is swallowed by the fire-and-forget caller's .catch(error => debugLogger.warn(...)) (coreToolScheduler.ts:1715 pattern; debugLogger is a mocked no-op in tests), so completion is suppressed silently — no error surfaces in the output, the only observable signature is vi.waitFor expiring at ~1 s ("expected 'spy' to be called at least once"). The executor timeout is the same hazard end-to-end: executeToolCall's promise never resolves because completion throws before resolve() — exactly the body's "missing or rejected flush() prevented completion". Note the swallowed-error shape: in production the reason for such a stall would survive only in a debugLogger.warn that most deployments never read — the quiet variant of the defect. Both mutant runs produced byte-identical failure sets, so the red arm is deterministic on this runner, not flake.
Corrections
None to prior review rounds. One measurement note on the PR's own text is recorded under Findings (info).
Findings
- (info) Body's scheduler-failure count is 8; measured is 9. The withdrawal narrative says the flush "introduced a control-flow regression: … eight scheduler test failures and one non-interactive timeout." Reconstructing the change produced nine scheduler failures plus the one executor timeout, deterministically across two runs. Direction, mechanism, and magnitude are confirmed; the off-by-one most plausibly reflects the exact original hunk shape (not inspectable here — the commit is absent at the shallow boundary) or the CI conditions of the original run. No action needed for this PR (it merges zero changes); recorded so the follow-up on bug(core): tool result write delay causes transient 'Tool result missing from saved history' on concurrent session load #9704 does not inherit the wrong count.
Not covered
- Per-commit attribution. Only 1 of the snapshot's 3 commits (
c677960head) is reachable locally;8235433(flush) andce68c79(revert) are below the depth-2 shallow boundary (git cat-file -efails for both;.git/shallowlists both roots;git rev-list HEAD^1..HEAD^2returns 1, the classic shallow-boundary truncation). The aggregateHEAD^1..HEADdiff is what was verified (empty), and the flush commit's effect was verified via reconstruction, not via the original bytes. - Byte-exact equivalence of the reconstruction to the original flush commit — impossible while
8235433is unreachable; the reconstruction follows the commit message verbatim and the body's "before the completion callback" placement description. - Repo-wide test suite, typecheck, lint — not re-run. The net diff is zero (identical tree objects), so no workspace outside the two exercised suites can behave differently from the base tip that the PR's own CI already built and tested; re-running would measure the base, not this PR.
- bug(core): tool result write delay causes transient 'Tool result missing from saved history' on concurrent session load #9704 itself (cold restore observing a dangling tool call without its
tool_result). The PR explicitly does not fix it and describes the correct-fix boundary (ownership evidence for cold restore; relates to Follow-up: prompt terminal ledger review suggestions (PR #9426 round 3) #9483). Verifying that underlying bug is out of scope for a withdrawal. - The flush mutation's effect on other core suites (e.g.
goal-turn-integration.test.ts, which attaches a recorder withoutflushdirectly) — relevant only to the withdrawn commit, not to this merge.
Methodology
Environment: the CI verify container (node:22-bookworm) at refs/pull/9705/merge, depth 2, with npm ci + npm run build already run at HEAD; no GitHub token, no network. All evidence was produced locally: git plumbing for the tree-identity proof (print-net-diff-proof.sh); vitest runs of the two named suites from packages/core with --coverage.enabled=false (an initial parallel run collided on vitest's shared coverage temp dir and crashed before collecting any tests — rerun sequentially, which is what the logs contain); a mutation harness (flush-mutation.mjs) that applies/restores the one-line flush with anchor-uniqueness and tree-hash assertions; print-ab.mjs renders the cell table from the captured logs (and run-suites.mjs remains as a JSON-mode rerun helper); and wiring-check.sh for the body's recorder-wiring claim (all three production CoreToolScheduler construction sites — useReactToolScheduler.ts, agent-core.ts, nonInteractiveToolExecutor.ts via its sole production caller nonInteractiveCli.ts with recordToolResult: false — wire no recorder, 5/5 assertions pass). Snapshot baseRefOid (7703d1c3…) differs from HEAD^1 because main advanced and the merge ref was regenerated; per the merge-ref contract the effective diff is measured against HEAD^1, and 7703d1c3… was additionally used to confirm the head-vs-base asymmetry is main-side drift only. Evidence images were rendered with scripts/verify-capture.mjs. Raw per-cell logs live in logs/; harnesses in the artifact root.
Flakiness gate log
verdict: n/a
summary: no runnable changed test files (0 out-of-scope file(s) noted in the log)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Closing — this PR no longer contains any changes. The recorder flush attempt was reverted on the branch, leaving a net-zero diff, and the PR body now serves as a withdrawal note. The withdrawal analysis is sound:
The underlying problem in #9704 remains reproducible on the latest Thanks @yiliang114 for the honest withdrawal and the thorough root-cause write-up. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 5": local execution of the new vitest suites (worktree has no node_modules/dist; CI unit-test check still pending at review time).
— qwen3.8-max via Qwen Code /review (v0.22.0)
github-actions
Bot
removed
the
review/self-reported
|
Tracking note for the remaining live-load window: the 30s restore-drain timeout during long-running tools is deliberate, contract-tested behavior ( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 5": running the four new acpAgent.test.ts cases to confirm they pass — the review worktree has no node_modules or built dist, and install+build for this monorepo ex….
Convergence: round 3 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 5 (4 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.ts (findings in rounds 1, 2; 5 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
— qwen3.8-max via Qwen Code /review (v0.22.0)
Loading a live session while a prompt is still running replayed the transcript with dangling-call finalization hard-coded on, so a tool call whose result had not been persisted yet was surfaced as the permanent 'Tool result missing from saved history' failure even though the tool was still executing (QwenLM#9704). Thread finalizeDangling through the bulk replay path and apply the same active-prompt guard the transcript paging path already uses: while a prompt is active in this process, a trailing unmatched call stays pending and its result arrives through the live stream. Cold restores of non-live sessions keep finalizing as before.
…rompts Review round on f806ae7 found the activePromptCalls predicate cannot see the daemon's autonomous turns (goal continuations, cron, background notifications), which bypass the client-prompt handler entirely, and that the live-load drain makes parts of the guard unreachable. Sample Session.isTurnIdle() — the same predicate family the live restore drain already waits on — before the transcript read and again at replay time, through a single shared helper used by both restore surfaces. Add settle-during-read regression tests at both sites; a mutant dropping the before-read sample now fails them. Also disclose the remaining windows honestly: the live-load drain still times out loads during long-running tools, and cold restores of non-live sessions keep finalizing pending ownership evidence (QwenLM#9483), so the PR no longer claims to fully close QwenLM#9704.
Round-3 review found the turn-activity guard blinded by the restore's own close gate: withLiveSessionRestore holds beginClose() across the replay, and isTurnIdle() is structurally false while closing=true, so every live load — including fully idle sessions whose trailing call is genuinely abandoned — skipped finalization and replayed the call as running forever. The gate already drains active turns and blocks new ones (a drain timeout rejects before any replay), so the gated live loadSession path now finalizes unconditionally. The two-sample isTurnIdle() guard stays on the ungated qwen/session/loadUpdates surface, with a debug line at the decision point and new tests covering the replay-time sample, the non-live default, and the gate contract.
7bef9cc to
b1047ad
Compare
|
Round-3 response (head b1047ad, rebased onto latest main with the
Local verification: acpAgent.test.ts + acp-integration/session suites 1547 passed, typecheck/ESLint/Prettier clean. |
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.
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 1a": executing the PR's new vitest cases ( acpAgent.test.ts , history-replay-page.test.ts , history-replayer.test.ts ) — vitest startup failed with ERR_MODULE_NOT_….
Convergence: round 4 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.ts (findings in round 3; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1a"：executing the PR's new vitest cases ( acpAgent.test.ts , history-replay-page.test.ts , history-replayer.test.ts ) — vitest startup failed with ERR_MODULE_NOT_…。
收敛情况：第 4 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/acpAgent.ts（第 3 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
createDebugLogger's formatArgs performs no util.format substitution — it stringifies and space-joins its arguments — so the printf-style %s placeholders shipped as literal markers with unlabeled values appended. Interpolate the values via a template literal (the convention at every other debugLogger call site in this file) and pin the single-interpolated-string shape in the loadUpdates test.
|
Closeout (head R1-3 (Fixes-trailer auto-close, root 3836893437) — verified addressed at head, no code change: the PR body carries "Narrows #9704 … This PR does not close the issue entirely … the issue stays open" and Linked Issues reads "Relates to #9704 and #9483. Follow-up for the live in-memory load shape: #9773" — the drop-trailer/disclose-narrowing option the finding offered. Replied + resolved. Predicate blind to autonomous turns (root 3836893438) — verified fixed at head ( Close gate blinding the replay-time sample (root 3837860972) — verified fixed at head ( printf-style Verification: Threads: 4 replied + resolved (roots 3836893437/3836893438/3837860972/3839035670); 7 unresolved remain (5 Suggestions + 2 test-shape Suggestions from this round) for the next cap round. Fork PR: no |
|
Closeout — Suggestions cap round on Of the 7 standing Suggestions: 1 fixed, 5 verified answered at head, 1 escalated. Fixed — test anchoring (3839035672) in Verified answered at head (evidence replies, resolved):
Escalated, deliberately unresolved (3839035673): real at head — Verification: |
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.
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 1a": end-to-end vitest run of the changed/new tests — attempted but not completed: the shared worktree had no built dist/ and a concurrent process was actively bui….
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.ts:5000 — [review] Restore guard's isTurnIdle() predicate conflates closing/mutation state with turn activity on the ungated loadUpdates path
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1a"：end-to-end vitest run of the changed/new tests — attempted but not completed: the shared worktree had no built dist/ and a concurrent process was actively bui…。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
PR Verification Report — maintainer local round (2026-08-24)Verdict: 中文摘要结论： A/B 核心证明（真实端到端）：真实 CLI 以 ACP 模式启动，loopback 假 OpenAI 模型，真实
未覆盖：goal/cron/notification 自主 turn 的 E2E 级驱动（已核实 Central claim A/B (real end-to-end, no mocks inside the unit under test)Harness: each arm's real CLI booted
Arm assertion totals (expected outcomes encoded per arm): base 11/11, head 12/12, merge 12/12. Evidence (terminal captures of the live runs):
Mutation / vacuity matrix — zero survivors (head worktree,
|
| Mutation | Result |
|---|---|
| baseline | 463 passed |
| M1 guard removed (= base behaviour) | 3 failed — all three new guard tests |
M2 before-read isTurnIdle sample dropped |
1 failed — exactly the settle-during-read test |
| M3 replay-time sample dropped | 1 failed — exactly the start-during-read test |
M4 gated live-restore path finalizeDangling: false |
4 failed — gated-path test + 3 goal-restore tests on that path |
| M5 positive control: debug interpolation → printf style | 1 failed — the interpolated-string assertion (control in the same mutated file) |
| restored | file clean |
Both samples of the two-sample guard are individually load-bearing; the guard as a whole is load-bearing; the E2E behaviour and the unit tests agree.
Gates
- head worktree, the 3 changed test files: 538/538 passed;
npm run typecheck(all workspaces) pass - trial-merge worktree, same files: 533/533 passed (count differs —
mainevolved these suites; both green); typecheck pass - trial merge into current
main: conflict-free
Findings
None blocking. Two observations: (1) the two-sample guard is conservative by construction — a turn starting or settling inside the transcript-read window keeps the call pending for one extra round, which the tests pin deliberately; (2) in the kill/restore cells the abandoned-call finalize message is "The tool had already completed; its output was discarded." (a different, more accurate wording than cell A's) — behaviour identical across arms, wording noted for completeness.
Not covered
Daemon autonomous turns (goal/cron/notification) verified at predicate level (#hasActiveTurn() covers all five turn families, unit tests pin the guard) but not driven live through a real cron/goal turn; the qwen serve/desktop WS channel layer (unchanged code); base-arm vitest (nothing to attribute — head/merge fully green); Windows/Linux.
Full report, harnesses (harnesses/e2e-loadupdates.mjs, mutation-matrix.sh), and raw logs available from this verification round on request.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 573 passed · 0 failed · 573 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：573 通过 · 0 失败 · 573 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #9705 Deep Verification —
|
| # | Previous finding | Severity | Status at new head a55295ab |
|---|---|---|---|
| 1 | Body claimed 8 scheduler failures from the withdrawn flush; measured 9 + 1 timeout | info | Superseded. The flush commit is no longer in this PR (the body itself records it as withdrawn). Its target code is absent from the new head, so there is nothing to re-measure; the finding applied to a version of the PR that no longer exists. |
| — | Previous verdict merge-ready for a zero-byte diff |
— | Superseded. That verdict applied to the withdrawal; this round verifies the replacement fix end to end. |
中文摘要
- 结论：
merge-ready，573/573 脚本化断言通过，fail: 0。这是跟进轮：上一轮验证的是本 PR 的旧版本（净零字节改动的撤回），PR 已重写为真正的修复，本轮对新 diff 从零重新验证。 - A/B 结论：核心声明成立且是承载性的。无 mock harness 用真实的
HistoryReplayer（编译产物）回放一个尾部 tool call 缺失结果的真实 transcript fixture：head 在finalizeDangling:false时让该 call 保持 pending（无 "Tool result missing from saved history" 占位、getPendingToolCalls()仍含该 call），而两个 base 对照臂（hunk 回退 dist + base 树真实重编译）在相同调用下都无条件 finalize（见01/02/03图，三臂各 7/7）。默认路径两臂行为一致，无回归。 - 测试承载性：8 个变异体全部按预期杀死对应测试（M1–M6 行为性，M8 钉住 round-3 的 gate 内决策；M7 为冗余防御，行为上是 no-op，仅被 mock 边界形状断言钉住），正控制落在与被变异文件相同的套件里，失败消息均为预期的断言不匹配（见
04图）。三个改动测试套件在 head 上 75 + 467 全绿。 - 发现：无阻塞项。1 条 low（预先存在）：分页读取面
qwen/status/session/transcript仍在用只对客户端 prompt 敏感的activePromptCalls守卫——正是本 PR 在 loadUpdates 上废弃的盲区谓词，goal/cron/notification turn 期间同样会误固化在途 call；PR 正文的"剩余窗口"清单未提及该面。2 条 info（预先存在）：导出路径（/export与 daemon export）中途导出时会把在途 call 标成误导文案的占位；以及 M7 行的冗余防御分类说明。 - 未覆盖：逐 commit 归因（depth-2 浅克隆，6 个提交仅 head 可达）；带真实模型 turn 的活体 daemon E2E（决策逻辑改由被变异证明非空转的 PR 单测 + 无 mock 的 replayer 层验证，复现的是 wire 形状而非端到端触发）；PR 明确声明不修的 30s drain 超时与冷恢复固化（Follow-up: live session load during a long-running tool hits the 30s restore drain timeout (#9704 remaining window) #9773、Follow-up: prompt terminal ledger review suggestions (PR #9426 round 3) #9483）；仓库级门禁（由 CI 车道覆盖）。
Central claim and A/B
Central claim (PR body): while a session has an active turn in this process, the ungated qwen/session/loadUpdates replay keeps a trailing unmatched tool call pending instead of finalizing it as the permanent "Tool result missing from saved history" failure; idle and non-live loads finalize as before; the gated live loadSession/resumeSession restore finalizes unconditionally because its close gate already drained turns.
Mechanism verified. HistoryReplayer.replayPage calls machine.finalize() (which emits the tool_call_update with status: 'failed' and the missing-result text) only when options.finalizeDangling === true (history-replayer.ts:171). Base hard-coded true inside replay(); the PR threads the option through replay() (?? true), collectHistoryReplayUpdates (forwarding), and computes it in acpAgent.ts:
- loadUpdates (ungated):
finalizeDanglingForRestore()=turnIdleBeforeRead && isTurnIdle()@replay, with the before-read sample taken prior to the transcript read (acpAgent.ts:10973,:11018); - gated live restore: unconditional
true(acpAgent.ts:4691) — underwithLiveSessionRestore,beginClose()is held across drain→read→replay (acpAgent.ts:3861-3883), andisTurnIdle()is!this.closing && !#hasActiveTurn()(Session.ts:3227), i.e. structurally false under the gate; a drain timeout rejects before any replay.
#hasActiveTurn() covers pendingPrompt and goalProcessing, cronProcessing/cronAbortController/cronCompletion, notificationProcessing/notificationAbortController/notificationCompletion (Session.ts:3290) — the autonomous-turn coverage the body claims, which the earlier activePromptCalls predicate lacked.
Cell table — Level-1 A/B (mock-free, real compiled HistoryReplayer + real transcript-replay machine + real dangling-call fixture)
| Cell | Arm | Oracle (emitted SessionUpdate stream) | Result |
|---|---|---|---|
R1 replay() default |
head | tool_call_update[failed] present, pending empty |
✅ finalized (7/7 arm) |
R2 replay(…, {finalizeDangling:false}) |
head | no failed update; getPendingToolCalls() keeps call-dangle |
✅ kept pending |
| R2 same call | base (hunk-reverted dist) | tool_call_update[failed] emitted anyway |
✅ finalized anyway |
| R2 same call | base (real base-tree rebuild) | tool_call_update[failed] emitted anyway |
✅ finalized anyway |
P1 collectHistoryReplayUpdates default |
both | identical finalize behavior | ✅ no default-path regression |
P2 collectHistoryReplayUpdates({finalizeDangling:false}) |
head / base | head: no finalize · base: finalizes | ✅ flip |
P3 explicit true |
both | finalize | ✅ identical |
Head arm 7/7, hunk-reverted base 7/7, real base-tree build 7/7. Witnesses: 01-ab-head-keeps-call-pending.png, 02-ab-base-finalizes-regardless.png, 03-ab-base-tree-real-build.png. Raw logs: logs/ab-head.log, logs/ab-base-revert.log, logs/ab-base-tree.log.
The hunk-reverted control and the real base build were cross-validated against each other: the base-tree's compiled history-replayer.js carries finalizeDangling: true at the exact line the control reverts, and both base arms produced byte-identical verdicts on all 7 cells.
Cell table — Level-2 decision logic + gates (PR's own suites, proven load-bearing below)
| Gate / suite at head | Result |
|---|---|
history-replayer.test.ts + history-replay-page.test.ts |
75 passed |
acpAgent.test.ts (incl. the 4 new decision tests) |
467 passed |
| Mutant runs (M1–M8) | every predicted red materialized; tree restored clean after each |
One subtlety measured, not a finding: the guard's settle-during-read branch (active before read, idle at replay ⇒ no finalize) is the conservative direction — if the turn settled before the replay, the write-barrier-guarded read normally already contains the result (nothing dangling), and in the residual race the call self-resolves on the next loadUpdates poll (now idle ⇒ finalize) or via the live stream for attached clients. No permanent-spinner path found.
Corrections
None. Commit 5's premise was independently confirmed: createDebugLogger's formatArgs maps args to strings/util.inspect and space-joins them with no util.format substitution (packages/core/src/utils/debugLogger.ts:82), so printf-style placeholders would have shipped as literal markers — the template-literal diagnostic is correct, and the test's single-string pin is load-bearing (see M4/M5 below).
Findings
- (low, pre-existing) The sibling paging surface keeps the stale guard this PR retires.
qwen/status/session/transcript(acpAgent.ts:8012, the paging reader) computesfinalizeDangling: !activePromptBeforeRead && !this.activePromptCalls.has(sessionId)(acpAgent.ts:8087,:8107).activePromptCallsis populated only in the clientprompt()handler (acpAgent.ts:5521-5526) — goal/cron/notification turns never register there (they are tracked inSession.#hasActiveTurnstate, see above). So during an autonomous turn, paging the transcript (a backward page, or the last forward page —history-replay-page.ts:390) finalizes an in-flight trailing call into the same permanent placeholder this PR removes fromloadUpdates: the same root cause, one surface over. This is pre-existing at the base tip (the PR does not touch that path) and non-blocking for this PR, which explicitly narrows rather than closes bug(core): tool result write delay causes transient 'Tool result missing from saved history' on concurrent session load #9704 — but the body's disclosed remaining-window list (the 30 s drain timeout → Follow-up: live session load during a long-running tool hits the 30s restore drain timeout (#9704 remaining window) #9773, cold restore → Follow-up: prompt terminal ledger review suggestions (PR #9426 round 3) #9483) does not name this surface, so it should not be read as covered by those issues. Suggested follow-up (not measured as a patch here, per scope): thread the same two-sampleisTurnIdle()guard throughreplayTranscriptRecordPage's caller. - (info, pre-existing) Export surfaces finalize mid-turn with misleading wording. Both export callers replay with the default (
collectSessionData→replayer.replay(messages),ui/utils/export/collect.ts:694): the TUI/exportcommand (exportCommand.ts:313) and the daemon export endpoint (serve/server/session-export.ts:94, which reads the transcript from disk). Exporting while a turn is in flight stamps the trailing live call as failed with "the previous run likely ended before this tool completed" — wrong wording for a live session. Static artifact, resolved by re-exporting; pre-existing at base; listed for completeness of the dangling-finalize sweep, not as a PR defect. - (info) M7 matrix row is redundant defence, not a coverage gap. The gated path's explicit
finalizeDangling: trueis behaviorally a no-op (replay()defaultsundefinedtotrue), and 4 tests pin the explicit shape at the mock boundary (1 new gated test + 3 pre-existing goal-bootstrap tests). The guard that actually matters there is M8's: re-introducing turn-activity sampling under the close gate is killed by the new gated test alone. Recorded so a future simplifier doesn't delete the explicit flag and its comment on the strength of "no behavioral difference".
No blockers. All findings are pre-existing shapes or classification notes; none is caused by this diff.
Not covered
- Per-commit attribution. The snapshot lists 6 commits; only
a55295ab(HEAD^2) is reachable at the depth-2 boundary (.git/shallowlists both roots;git rev-list HEAD^1..HEAD^2returns 1 — the classic shallow truncation). The aggregateHEAD^1..HEADdiff is what was verified. - Live daemon E2E with a real in-flight model turn. Requires a model backend this container does not have. The decision logic was instead verified through the PR's own unit tests — proven non-vacuous by 8 mutants — and the emitted-update mechanism mock-free at the replayer layer. This reproduces the wire shape of bug(core): tool result write delay causes transient 'Tool result missing from saved history' on concurrent session load #9704 (the guard decision and the exact update stream), not the end-to-end trigger (a real restore racing a real tool).
- The disclosed remaining windows: the 30 s active-turn drain timeout on gated live loads, and cold-restore finalization of non-live sessions (Follow-up: live session load during a long-running tool hits the 30s restore drain timeout (#9704 remaining window) #9773, Follow-up: prompt terminal ledger review suggestions (PR #9426 round 3) #9483). The base arm proves the default/cold finalization behavior is unchanged by construction.
- Repo-wide gates (full test suite, lint, typecheck) — the PR's own CI lane covers them; only the 3 changed suites were run here (all green, plus 10 mutant cells). No flake observed across the repeated runs of the changed suites.
- Windows/macOS-specific behavior — the change is platform-neutral TS in the ACP session layer.
Methodology
Environment: the CI verify container (node:22-bookworm, node v22.23.2) at refs/pull/9705/merge, depth 2, npm ci + npm run build pre-run at HEAD; no GitHub token, no network. Snapshot baseRefOid (be891657…) differs from HEAD^1 because main advanced and the merge ref was regenerated; the effective diff is measured against HEAD^1 per the merge-ref contract, and HEAD^2 equals the snapshot headRefOid. Harnesses live in the artifact root and were driven against compiled dist/ output: replay-ab.mjs (Level-1 A/B; takes --dist + --expect head|base, asserts on the emitted SessionUpdate stream and getPendingToolCalls()); mutate.mjs (unique-anchor-asserted source mutants, restored via git checkout -- after each cell with a clean-tree verification); print-matrix.mjs (matrix renderer). Base control: (a) a scratch copy of the head dist with exactly the two changed hunks reverted (anchor-uniqueness asserted, one occurrence each), and (b) a real base worktree (git worktree add tmp/base-tree HEAD^1) rebuilt with the repo's typescript (tsc --build; two unrelated environment errors in test-only/generated files did not affect the emitted modules, verified by content). Internal workspace deps resolve from the base tree into the head tree's node_modules (asserted via fs.realpathSync) — a clean control because the PR's diff touches no file outside packages/cli/src/acp-integration (git diff --stat HEAD^1..HEAD -- packages/core packages/acp-bridge is empty). Gates: npx vitest run <changed files> --coverage.enabled=false from packages/cli. Evidence images rendered with scripts/verify-capture.mjs; raw per-cell logs in logs/.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts
file packages/cli/src/acp-integration/session/history-replay-page.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/history-replay-page.test.ts
file packages/cli/src/acp-integration/session/history-replayer.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/history-replayer.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/acp-integration/acpAgent.test.ts: PPPPP
packages/cli/src/acp-integration/session/history-replay-page.test.ts: PPPPP
packages/cli/src/acp-integration/session/history-replayer.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/history-replay-page.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/history-replayer.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/history-replay-page.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/history-replayer.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/history-replay-page.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/history-replayer.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/session/history-replay-page.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/session/history-replayer.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/session/history-replay-page.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/session/history-replayer.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.22.2. |
4 participants
What this PR does
Narrows #9704: session-restore replays no longer finalize a trailing tool call as the permanent "Tool result missing from saved history" failure while the session still has an active turn in this process, covering client prompts and the daemon's autonomous goal, cron, and notification turns. The call stays pending and its result arrives through the live stream. This PR does not close the issue entirely; the remaining windows are disclosed below and the issue stays open.
This replaces the earlier recorder-flush attempt, which was withdrawn: the scheduler only records results after a tool batch reaches a terminal state, so during the reported window there was nothing to flush. The failure was on the reader side, not the writer side.
Why it's needed
The bulk session-restore replay finalized dangling tool calls unconditionally. During a live in-flight turn, that made the reader convert a still-running tool call into a permanent missing-result placeholder before the real result could be streamed.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: a live restore could show a permanent "Tool result missing from saved history" placeholder for a tool call that was still running. After: the replay keeps that call pending while the session has an active turn, then the live stream supplies the result.
Tested on
Environment (optional)
Local Node workspace tests and repository build/typecheck/lint/format checks.
Risk & Scope
Linked Issues
Relates to #9704 and #9483. Follow-up for the live in-memory load shape: #9773.
中文说明
本 PR 做了什么
收窄 #9704：当当前进程里 session 仍有活跃 turn 时，session restore replay 不再把尾部未匹配的 tool call 固化为永久的 "Tool result missing from saved history" 失败，覆盖客户端 prompt 以及 daemon 的 goal、cron、notification 自主 turn。该 tool call 会保持 pending，并通过 live stream 收到后续结果。本 PR 不完全关闭该 issue；剩余窗口在下方说明，issue 保持打开。
这替代了早先撤回的 recorder flush 尝试：scheduler 只有在 tool batch 进入终态后才记录结果，所以报告窗口里没有可 flush 的内容。失败点在读取侧，不在写入侧。
为什么需要
批量 session restore replay 过去会无条件 finalize dangling tool call。在 live in-flight turn 中，这会让读取侧在真实结果流回来前，把仍在运行的 tool call 转成永久 missing-result placeholder。
评审测试计划
如何验证
证据（Before & After）
Before：live restore 可能把仍在运行的 tool call 显示成永久 "Tool result missing from saved history" placeholder。After：session 有活跃 turn 时 replay 会让该 call 保持 pending，随后由 live stream 补上结果。
测试环境
环境（可选）
本地 Node workspace 测试以及仓库 build/typecheck/lint/format 检查。
风险与范围
关联 Issue
Relates to #9704 and #9483。live in-memory load 形态的 follow-up：#9773。