fix(cli): finalize dangling turns and barrier live transcript reads - #11144
fix(cli): finalize dangling turns and barrier live transcript reads#11144kabishou11 wants to merge 7 commits into
Conversation
…enLM#9704) Concurrent session loads could read the JSONL transcript after flush() returned but before a queued recordToolResult write landed, so replay finalized the trailing call as permanently missing. Wrap live sessionTranscript disk reads in runWithWriteBarrier (matching loadUpdates and loadSession) and guard finalizeDangling with the shared isTurnIdle() sampler so in-flight turns stay pending.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the fix, @kabishou11 — but the description here is a single line and doesn't follow the PR template. Maintainers prioritize PRs with a clear reviewer test plan, so without it this one is likely to sit unreviewed.
Please rewrite the description using the template's headings:
- What this PR does / Why it's needed — the one-liner you have now belongs here, expanded into prose.
- Reviewer Test Plan → How to verify — this is the section that matters most for this particular change. The title and body describe a TOCTOU race between tool-result writes and live transcript reads, which is timing-dependent by nature, so reviewers need the concrete steps you used to trigger the transient
Tool result missing from saved historyon a concurrent session load, and how reliably it reproduces. - Evidence (Before & After) — the failure before the change, and the same run after it. If the race can't be hit on demand, say so and describe what you actually observed rather than leaving it blank.
- Tested on — the OS table, plus the environment you ran in.
- Risk & Scope — the main tradeoff of serializing these reads behind the writes, and anything you deliberately left unvalidated.
- Linked Issues —
Fixes #9704is already parsed correctly; keep it in this section. - The
<details>中文说明 block, translating all of the above.
No code review yet — this stops at the description gate. Once the template is filled in, re-run with @qwen-code /triage and it will pick up from there.
中文说明
感谢提交修复，@kabishou11 — 不过目前 PR 描述只有一行，没有按照 PR 模板 填写。维护者会优先处理带有清晰 reviewer test plan 的 PR，缺少它的话这个 PR 很可能一直没人 review。
请用模板的小标题重写描述：
- What this PR does / Why it's needed — 你现在这一行总结放在这里，并展开成完整说明。
- Reviewer Test Plan → How to verify — 这是本次改动最关键的一节。标题和正文描述的是 tool-result 写入与实时 transcript 读取之间的 TOCTOU 竞态，本质上是时序相关的，所以审阅者需要知道你用什么具体步骤触发了并发加载会话时偶发的
Tool result missing from saved history，以及复现的稳定性如何。 - Evidence (Before & After) — 改动前的失败现象，以及同样操作在改动后的结果。如果这个竞态无法按需触发，请说明情况并描述你实际观察到的现象，不要留空。
- Tested on — 操作系统表格，以及你运行的环境。
- Risk & Scope — 把这些读取串行化到写入之后的主要权衡，以及你有意未验证的部分。
- Linked Issues —
Fixes #9704已被正确解析，保留在这一节即可。 <details>里的 中文说明，对应翻译以上全部内容。
目前还没有做代码审查——流程停在描述这一关。补齐模板后，用 @qwen-code /triage 重新触发，会从这一步继续。
— Qwen Code · qwen3.8-max-2026-09-02
|
Updated the PR description to match the full template (What this PR does / Why / Reviewer Test Plan / Risk & Scope / Linked Issues + 中文说明). Ready for another look. |
|
@qwen-code /triage — PR description has been rewritten to the full template (including Reviewer Test Plan / Evidence / 中文说明). Please re-run from the description gate. |
|
Thanks for coming back and filling the template in properly, @kabishou11 — that unblocks the review. Picking up from the description gate. Template looks good ✓ — every heading is there, the OS table and Risk & Scope are filled in rather than stubbed, and the 中文说明 block is a real translation. Problem: observed, not theoretical. #9704 carries a concrete timeline from a Linux daemon (session Direction: aligned, and the site is the right one. I checked this rather than assuming it. Size: not applicable — Approach: the scope is right and reusing the existing helper instead of adding a parallel one is exactly what we want. One thing I would think about before the code review, and it is the same thing I raise as a blocker below: the base code only touched the recorder when Risk: Stage 1e matched. Moving on to code review. 🔍 中文说明感谢 @kabishou11 回来把模板补全 —— 这解除了 review 的阻塞。下面从描述这一关继续。 模板完整 ✓ —— 所有小标题都在，操作系统表格和 Risk & Scope 是认真填写的而不是占位，中文说明也是真实翻译。 问题：已观测，不是理论性的。#9704 提供了具体的时间线（Linux daemon，session 方向：一致，而且改动位置正确。这一点我做过核实而非假定。 规模：不适用 —— 方案：范围合理，复用已有 helper 而不是新增一个平行实现正是我们希望的。有一点希望在代码审查前先想清楚，也正是我在下面作为阻塞项提出的：base 代码只在 风险：Stage 1e 命中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Thanks for the thorough Stage 1 pass — agreed on the live path analysis ( Follow-up after Stage 2/3: I had argued for widening the barrier to every live disk read. Your point about |
Code reviewI read the title and the "Why it's needed" section first and wrote down what I would do before opening the diff: put the One blocker. The barrier is now unconditional, and that widens the failure surface onto the Web UI's pagination path. Base gated recorder interaction on direction: if (rawDirection === 'backward') {
await this.sessions.get(sessionId)?.getConfig().getChatRecordingService()?.flush();
}Your version wraps every read: const page =
recording !== undefined
? await recording.runWithWriteBarrier(readPersistedPage)
: await readPersistedPage();Those two are not interchangeable. The reason this matters more than it looks: the Web UI's "load older history" call never sends Two reachable states where a session is still live in
To be clear about what is not wrong here: the error surfacing is fine, I traced it. The drain semantics you actually need are preserved either way — Suggested fix, smallest first: keep the barrier on the latest read only, i.e. when there is no cursor/anchor ( Non-blocking, worth knowing:
What is good: the new ordering test is a genuine pin, not a tautology — it holds the barrier open with a gate promise and asserts Coverage gap: nothing exercises the new throw path. There is no test for a non-active recorder on a sequenceDiagram
participant P1 as Web UI chat pane
participant P2 as daemon session route
participant P3 as acp-bridge
participant P4 as QwenAgent sessionTranscript
participant P5 as ChatRecordingService
participant P6 as SessionTranscriptReader
participant P7 as transcript replay
P1->>P2: GET transcript page (cursor or beforeRecordId)
P2->>P3: getSessionTranscriptPage
P3->>P4: extMethod qwen/status/session/transcript
Note over P4: base skips the recorder entirely when direction is absent
P4->>P5: runWithWriteBarrier (PR, now unconditional)
alt recorder active
P5->>P6: readPage
P6-->>P5: page
P5-->>P4: page
P4->>P7: replay with finalizeDanglingForRestore
P7-->>P1: events, dangling calls left pending
else recorder closing or integrity failed
P5-->>P4: SessionWriterUnavailableError
P4-->>P1: 503 session writer unavailable
end
Files changed (2 of 2)
Testing evidenceThis is an unattended CI run, so I did not build or execute anything from this PR — the evidence below is the PR's own CI, read through the API for commit The headline is that this PR's CI has not run. All three
Everything green above is bot orchestration or precheck plumbing — none of it compiles the change or runs a test. So: not verified: unit tests, lint, and typecheck have not executed on this commit, and I want to be explicit that this is a gate, not a formality. Also not verified: the race itself. The author's Evidence section says N/A for before/after, and I am not treating the unit tests as a substitute — they assert barrier ordering against a mocked recorder, which is a real property but not the 104-second write-delay window #9704 describes. That is the author's own honest framing, not a claim I am adopting as evidence. Sandboxed verification would settle this: 中文说明代码审查我先只看了标题和「为什么需要」一节，在打开 diff 之前写下了自己会怎么做：把 有一个阻塞项。 barrier 现在是无条件的，这把失败面扩大到了 Web UI 的分页路径上。 base 是按 direction 来决定是否触碰 recorder 的：只在 之所以比看上去更严重：Web UI 的「加载更早历史」从不发送 会话仍在
需要说清楚哪里没有问题：错误的对外呈现是正常的，我追过了。 你真正需要的 drain 语义两种写法都保留了 —— 建议的修法，由小到大：把 barrier 只保留在最新读取上，即没有 cursor/anchor 时（ 非阻塞，但值得知道：
做得好的地方： 新增的顺序测试是真正的 pin，不是同义反复 —— 它用一个 gate promise 把 barrier 挂住，并在释放前断言 覆盖缺口： 没有任何用例覆盖新增的抛出路径。缺少「recorder 非 active 时对 测试证据这是一次无人值守的 CI 运行，所以我没有构建或执行本 PR 的任何代码 —— 以下证据来自 PR 自身的 CI，通过 API 读取 commit 要点是本 PR 的 CI 根本没有运行。 三个由 上表中所有绿色项都是 bot 编排或 precheck 类流水线 —— 没有一个会编译这次改动或运行测试。所以：未验证：本 commit 上的单测、lint、typecheck 均未执行，我要明确说明这是一道关卡而非形式。 同样未验证：竞态本身。作者的 Evidence 一节对 before/after 写的是 N/A，我也不把单测当作替代品 —— 它们断言的是针对 mock recorder 的 barrier 顺序，这是一个真实性质，但不是 #9704 描述的那个 104 秒写入延迟窗口。这是作者自己诚实的表述，不是我采纳为证据的主张。 沙箱验证可以定这件事： （时序图与文件表见英文部分，内容一致，不再重复。） — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 2/5 — right fix, right site, wrong scope on the barrier; and there is no CI evidence to lean on. Stepping back. This is not a volume PR and not a solution looking for a problem. #9704 is a well-evidenced bug with a real timeline, the changed site is genuinely the one that produces the symptom, and I verified that independently rather than taking the description's word for it — the path the existing What stops me is scope. Lifting the read into a closure was the natural way to pass it to the barrier, and in doing so the The part that pushes this from "nit" to "request changes" is that Risk & Scope does not mention it. "Slightly longer wait while a tool result is flushing" is an accurate description of the intended tradeoff and says nothing about a new failure mode. I would rather the author make that choice deliberately — barrier only the latest read, or barrier everything and fall back to a direct read when the recorder is not active — than have it arrive as a side effect of a refactor. If I were maintaining this in six months I would thank them for the helper reuse and curse the day history pagination started depending on writer health. Those are separable, which is exactly why it is worth one more round rather than a merge and a follow-up. Two things I am deliberately not holding against the PR: the lease-churn path where a read-only page can degrade a live recorder is real but gated behind an experimental, off-by-default flag and already exists on On CI: there is none. For the record, my earlier On pattern: the author opened this and #11145 twenty seconds apart. I evaluated this one on its own merits and it holds up substantively — the concern above is a specific, reachable regression I traced to file and line, not a judgment about volume. 中文说明Confidence: 2/5 —— 修法对、位置对，但 barrier 的作用范围不对；而且没有 CI 证据可依。 退一步看整体。这不是刷量 PR，也不是拿着方案找问题。#9704 是一个有真实时间线、证据充分的 bug，改动位置确实就是产生该症状的那一处，而且这一点是我独立核实的，不是采信描述的措辞 —— 已有 让我停下来的是范围。把读取提取成闭包是把它交给 barrier 的自然写法，而在这个过程中 把这一点从「小问题」推到「request changes」的，是 Risk & Scope 完全没有提到它。「tool 结果刷盘期间稍等更久」准确描述了预期的取舍，但对新增的失败模式只字未提。我更希望作者是有意识地做出这个选择 —— 只对最新读取加 barrier，或者对所有读取加 barrier 但在 recorder 非 active 时回退为直接读取 —— 而不是让它作为一次改写的副作用出现。 如果六个月后由我来维护这段代码，我会感谢他们的 helper 复用，也会诅咒历史分页开始依赖 writer 健康状态的那一天。这两件事是可以分开的，这也正值得再走一轮，而不是先合并再补follow-up。 有两点我刻意不作为本 PR 的扣分项：只读分页可能降级存活 recorder 的 lease-churn 路径确实存在，但它被一个实验性、默认关闭的开关挡住，且 关于 CI：没有。 另作说明：我此前对本 PR 的 关于模式：作者在本次与 #11145 之间相隔二十秒提交。我是按本 PR 自身的是非来评估的，它在实质上是站得住的 —— 上面那个顾虑是我追到具体文件与行号的一个可复现回归，而不是对提交数量的评判。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs one more round, @kabishou11 — the fix itself is right, but the barrier's scope is wrong. Full reasoning is in the Stage 2 and Stage 3 comments above; the short version:
Blocking. Base only touched the recorder when direction === 'backward'; this PR wraps every read in runWithWriteBarrier. Those are not interchangeable — flush() only drains operationTail, while the barrier also throws SessionWriterUnavailableError when the recorder is not active/acceptingWrites (chatRecordingService.ts:1626-1631). The Web UI's history pagination never sends direction (DaemonSessionProvider.tsx:4143 passes only cursor/beforeRecordId), so reads that never consulted writer health before now fail on it. Concretely: a session whose recorder hit a write failure stays live and readable by design, and scrolling back through it would start returning 503. Same for the managed-shutdown/handoff window between acpAgent.ts:3713 and :3733.
Either barrier only the latest read (no cursor/anchor — which keeps the #9704 fix intact, since the reported path is a backward read), or barrier everything and fall back to a direct read when the recorder is not active. Whichever you pick, please name the new failure mode under Risk & Scope — it currently lists only the extra latency.
Also blocking, and not yours to fix: CI has not run. Qwen Code CI, SDK Java and tui-parity are all at action_required, waiting on a maintainer to approve the fork's workflow runs. acp-integration is a high-risk path here, so there is no approving this without test, lint and typecheck evidence on the commit. A maintainer needs to release those runs.
Not blocking: a read-only page can now degrade a live recorder via the barrier's lease check (experimental flag, off by default, and the pattern already exists on main elsewhere); and serve/routes/session.ts:5050 still hand-rolls its own dangling-call sampling — worth a follow-up issue, it cannot share the helper across the process boundary.
To be clear about what is good here: the site is correct, and I checked that rather than assuming — qwen/session/loadUpdates, where finalizeDanglingForRestore was already wired in, has no production consumer, so the live path genuinely was still broken. Swapping activePromptCalls for isTurnIdle() is the actual fix, since the former cannot see autonomous goal/cron/notification turns. The new ordering test is a real pin, not a tautology.
This review supersedes my earlier one, which was the description gate — you have satisfied that.
中文说明
需要再走一轮，@kabishou11 —— 修复本身是对的，但 barrier 的作用范围不对。完整推理见上面的 Stage 2 与 Stage 3 评论，简述如下：
阻塞项。 base 只在 direction === 'backward' 时触碰 recorder；本 PR 把每一次读取都包进了 runWithWriteBarrier。两者不可互换 —— flush() 只 drain operationTail，而 barrier 还会在 recorder 非 active/acceptingWrites 时抛出 SessionWriterUnavailableError（chatRecordingService.ts:1626-1631）。Web UI 的历史分页从不发送 direction（DaemonSessionProvider.tsx:4143 只传 cursor/beforeRecordId），所以此前从不查询 writer 健康状态的读取现在会因它失败。具体来说：recorder 已经写入失败的会话按设计仍然存活可读，而向前翻它的历史会开始返回 503；acpAgent.ts:3713 与 :3733 之间的托管关闭/handoff 窗口同理。
要么只对最新读取加 barrier（无 cursor/anchor —— 这不影响 #9704 的修复，因为所报路径就是 backward 读取），要么对所有读取加 barrier 但在 recorder 非 active 时回退为直接读取。无论选哪个，请在 Risk & Scope 中写明这个新增的失败模式 —— 目前只列了额外延迟。
同样阻塞，但不需要你来解决： CI 没有运行。Qwen Code CI、SDK Java、tui-parity 全部停在 action_required，在等维护者批准 fork 的 workflow 运行。acp-integration 在此属于高风险路径，所以没有该 commit 上的测试、lint、typecheck 证据就无法 approve。需要维护者放行这些运行。
非阻塞： 只读分页现在可能通过 barrier 的 lease 检查降级一个存活 recorder（实验性开关，默认关闭，且 main 上别处已有同样模式）；以及 serve/routes/session.ts:5050 仍手写自己的 dangling-call 采样 —— 值得开一个后续 issue，它跨进程边界无法共享该 helper。
需要说清楚哪里做得好：位置是正确的，而且这一点是我核实过的而非假定 —— finalizeDanglingForRestore 当初接入的 qwen/session/loadUpdates 没有任何生产调用方，所以真正在用的路径确实仍是坏的。用 isTurnIdle() 换掉 activePromptCalls 才是真正的修复，因为前者能看见自主的 goal/cron/notification turn。新增的顺序测试是真正的 pin，不是同义反复。
本次 review 取代我此前那一次 —— 那一次是描述这一关，你已经满足了。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at ce54f416011f4915946d3a6e845321c55fc6b328
runWithWriteBarrier also refuses when the recorder is not active/acceptingWrites, unlike flush(). Keep the TOCTOU barrier on the backward/latest path and let cursor/anchor pagination read directly so Web UI history still works after a write failure or during handoff.
|
Thanks — addressed on Narrowed the write barrier to the latest/backward path only ( Risk & Scope now calls out that residual 503 mode on the latest/backward path, and why non-latest pages stay unbarred (possible remaining race on older pages only). Local checks: the barrier ordering test for backward/latest, plus a cursor/anchor test that skips the barrier even when the recorder would refuse writes, are green. |
|
Verified against current
Nothing blocking from a read-code pass. One micro-question if you want a follow-up nit: for a session that exists but returns no recording service, |
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.
Unresolved, please confirm:
- [Critical] CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678): 'CI has not run. Qwen Code CI, SDK Java and tui-parity are all at action_required... acp-integration is a high-risk path here, so ther…
Not explored to full depth (tool budget reached): "agent 3b": exact await ordering of closeSessionWriter() inside the normal Config.shutdown() path, and whether ChatRecordingService.finalize() (chatRecordingService.t…; "agent 3c": did not verify whether DaemonSessionTranscriptPageOptions / DaemonClient.getSessionTranscriptPage (packages/sdk-typescript/src/daemon/types.ts:1360, DaemonC…; "agent 6a": verifying whether getSessionSummary(...).hasActivePrompt (packages/cli/src/serve/routes/session.ts:4988-4990) already accounts for autonomous goal/cron/notifi….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 3b"：exact await ordering of closeSessionWriter() inside the normal Config.shutdown() path, and whether ChatRecordingService.finalize() (chatRecordingService.t…；"agent 3c"：did not verify whether DaemonSessionTranscriptPageOptions / DaemonClient.getSessionTranscriptPage (packages/sdk-typescript/src/daemon/types.ts:1360, DaemonC…；"agent 6a"：verifying whether getSessionSummary(...).hasActivePrompt (packages/cli/src/serve/routes/session.ts:4988-4990) already accounts for autonomous goal/cron/notifi…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // never consulted writer health; the barrier would 503 them | ||
| // after a write failure or during handoff. | ||
| const page = | ||
| recording !== undefined && rawDirection === 'backward' |
There was a problem hiding this comment.
[Critical] R1-1: [fails-closed] [regression] Swapping this path's flush() for runWithWriteBarrier turns a read-only latest-page request into a writer-health query. runWithWriteBarrier throws SessionWriterUnavailableError when !acceptingWrites || state !== 'active' (chatRecordingService.ts:1626-1631), while the flush() it replaced was only await this.operationTail; if (this.writeFailure) throw (:1606-1610). Narrowing the barrier to this branch did close the blocker the triage pass raised — cursor and anchor pages are genuinely exempt now, and the new test pins that — but the refusal travels further than the tradeoff Risk & Scope describes.
Risk & Scope accepts a residual 503 "after a write failure, or in the managed shutdown/handoff window" and calls it "the residual failure mode for the #9704 path only". That rationale does cover a direct ACP client's own latest-page read during a managed handoff. It does not cover three entrances that share this one root.
The first is the ordinary per-session close, which is neither of the two states the description names: recorder?.finalize(); await recorder?.flush(); await recorder?.close() at acpAgent.ts:4437-4455 runs before removeStoredSessionEntry deletes the entry at :4164, so the window spans recorder close plus a full config.shutdown(). That is every session/close, not just managed shutdown. The second is the daemon, which uses this exact call as a flush primitive and then throws the answer away: serve/routes/session.ts:5000-5017 calls bridge.flushSessionTranscript, which is {direction:'backward', limit:1} (bridge.ts:12229-12237), swallows only SessionNotFoundError, and reads the page from disk itself at :5018-5028. No #9704 latest-page read is happening for that consumer at all, so the 503 buys nothing and loses a request the daemon could have served. The third is GET /session/:id/transcript (session.ts:4774), which forwards direction at :4850-4857 and returns the child's page as the HTTP response with no daemon-local read to fall back to. In all three the transcript is on disk and readable, and base returned 200 with the page.
Worth correcting in the same pass: bridge.ts:12230's comment "The child flushes before every backward page" is now false — the child barriers, and can refuse — and that is the line a maintainer reads when deciding whether the daemon may ignore the error.
Witness:
PROBE-R1-1-window {"flushOutcome":"resolved",
"barrierOutcome":"rejected:SessionWriterUnavailableError:session_writer_unavailable:-32023"}
(real ChatRecordingService, no service mock; beginClose() is what Config.closeSessionWriter() calls)
PROBE-R1-1 BASE: {"outcome":"resolved","hasMore":false,"events":0,
"barrierCalls":0,"flushCalls":1,"readPageCalls":1}
PROBE-R1-1 PR: {"outcome":"rejected","message":"Session write ownership could not be verified.",
"code":-32023,"data":{"errorKind":"session_writer_unavailable"},
"barrierCalls":1,"flushCalls":0,"readPageCalls":0}
readPageCalls: 0 on the PR arm is the deciding number — the persisted page was never read, though it was on disk.
// Smallest fix consistent with the tradeoff you already documented: keep the barrier,
// but do not let a lifecycle-inactive recorder fail a read-only page.
const page =
recording !== undefined && rawDirection === 'backward'
? await recording.runWithWriteBarrier(readPersistedPage).catch((error) => {
// discriminate on recorder state / absence of writeFailure, NOT on instanceof
if (!isWriterLifecycleUnavailable(recording, error)) throw error;
return readPersistedPage();
})
: await readPersistedPage();
// Alternatively, fix it at the consumer that discards the page:
// serve/routes/session.ts:5012-5016 already swallows SessionNotFoundError from the
// pre-flush; swallow a writer-unavailable flush the same way and fall through to its
// own disk read at :5018-5028.The fallback must discriminate on recorder state or the absence of writeFailure, not on instanceof SessionWriterUnavailableError: chatRecordingService.ts:1627 (if (this.writeFailure) throw this.writeFailure;) is the barrier's first statement and base's flush() threw it too, and enterWriteFailure can store a SessionWriterUnavailableError as that failure (:1290-1300), so a type-keyed fallback would swallow a genuine integrity failure and serve a transcript the recorder has already declared untrustworthy. Please add a backward-page sibling of the new does not barrier cursor or record-anchor transcript pages test that rejects with a real SessionWriterUnavailableError and pins whichever semantics you settle on — today no test pins either side, so removing the behaviour you add would go unnoticed; run that mutation (drop the fallback, or drop the propagation) and confirm the new test reds.
中文说明
把这条路径的 flush() 换成 runWithWriteBarrier，会让一个只读的最新页请求变成对 writer 健康状态的查询。runWithWriteBarrier 在 !acceptingWrites || state !== 'active' 时抛出 SessionWriterUnavailableError（chatRecordingService.ts:1626-1631），而被它替换掉的 flush() 只是 await this.operationTail; if (this.writeFailure) throw（:1606-1610）。把 barrier 收窄到这一分支确实解决了 triage 提出的阻塞项 —— cursor 与 anchor 分页现在真正被豁免，新增测试也固定了这一点 —— 但这个拒绝语义的影响范围比 Risk & Scope 所描述的取舍要广。
Risk & Scope 接受了「写入失败之后，或托管关闭/handoff 窗口」的残余 503，并称其为「仅针对 #9704 路径的残余失败模式」。这个理由确实覆盖了直连 ACP 客户端在托管 handoff 期间自身的最新页读取，但没有覆盖共享同一处根因的三个入口。
第一个是普通的单会话关闭，它不是描述中提到的两种状态中的任何一种：acpAgent.ts:4437-4455 的 recorder?.finalize(); await recorder?.flush(); await recorder?.close() 在 removeStoredSessionEntry 于 :4164 删除条目之前执行，因此该窗口横跨 recorder 关闭加上一整个 config.shutdown()。那是每一次 session/close，而不只是托管关闭。第二个是 daemon，它把这个调用当作 flush 原语使用，随后又丢弃返回结果：serve/routes/session.ts:5000-5017 调用 bridge.flushSessionTranscript（即 {direction:'backward', limit:1}，见 bridge.ts:12229-12237），只吞掉 SessionNotFoundError，然后在 :5018-5028 自己从磁盘读取页面。对这个调用方而言根本不存在 #9704 的最新页读取，所以这个 503 换不到任何东西，却丢掉了一个 daemon 本可自行服务的请求。第三个是 GET /session/:id/transcript（session.ts:4774），它在 :4850-4857 转发 direction，并把子进程返回的页面直接作为 HTTP 响应，没有 daemon 本地读取可回退。这三种情况下 transcript 都在磁盘上且可读，而 base 返回 200 并带上了页面。
同一轮里值得顺手改正：bridge.ts:12230 的注释「The child flushes before every backward page」现在已不成立 —— 子进程是加 barrier，而且可能拒绝 —— 而这正是维护者判断 daemon 是否可以忽略该错误时会读到的那一行。
证据： 见上方 PROBE-R1-1 输出。PR 侧的 readPageCalls: 0 是决定性数字 —— 页面在磁盘上，却从未被读取。
修法： 与你已经记录的取舍一致的最小改法是保留 barrier，但不让一个生命周期上非 active 的 recorder 使只读页面失败（见上方代码块）；或者在丢弃该页面的调用方处修 —— serve/routes/session.ts:5012-5016 已经吞掉 pre-flush 的 SessionNotFoundError，用同样方式吞掉 writer-unavailable，然后落到它在 :5018-5028 自己的磁盘读取。
约束： 回退必须以 recorder 状态或 writeFailure 的缺失来判别，而不能用 instanceof SessionWriterUnavailableError：chatRecordingService.ts:1627（if (this.writeFailure) throw this.writeFailure;）是 barrier 的第一条语句，base 的 flush() 同样会抛它，而 enterWriteFailure 可能把一个 SessionWriterUnavailableError 存为该 failure（:1290-1300），所以按类型判别的回退会吞掉真正的完整性失败，把一个 recorder 已宣告不可信的 transcript 交给用户。
验收标准： 请为新测试 does not barrier cursor or record-anchor transcript pages 补一个 backward 页的兄弟用例，用真实的 SessionWriterUnavailableError 拒绝，并固定你最终选定的语义 —— 目前没有任何测试固定任一侧，所以你新增的行为被移除也不会被发现；请执行该变异（去掉回退，或去掉向外传播）并确认新测试变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| maxBytes: SESSION_TRANSCRIPT_MAX_PAGE_BYTES, | ||
| }); | ||
| }; | ||
| // Barrier only the latest tail (#9704). Cursor/anchor pages |
There was a problem hiding this comment.
[Suggestion] R1-2: This comment states the invariant as "barrier only the latest tail", but the code decides it from the request's rawDirection while the decision that actually needs the barrier is made downstream from the resolved page: history-replay-page.ts:389-390 applies finalizeDangling && (page.direction === 'backward' || !page.hasMore). The two predicates are not the same set, and the gap is reachable from a shipped route.
A request with no direction, no cursor and no anchor resolves inside the reader to direction: 'forward' from position 0 (session-transcript-reader.ts:3406-3411). For a session whose record count fits the page limit the window runs to EOF, so hasMore is false and finalization applies — while rawDirection === 'backward' is false, so the read never enters the barrier and never waits on operationTail. A recordToolResult append still queued is then missing from the page, and the trailing tool call is replayed as permanently missing. It fails silently, as data rather than as an error. GET /session/:id/transcript (serve/routes/session.ts:4774) really does forward this shape: :4850-4857 spreads ...(direction !== undefined ? { direction } : {}), so ?limit=100 with no direction reaches the child direction-less, and that route returns the child's page as the HTTP response. The request grammar is validated and bounded, so this is one named shape rather than an open-ended family — but the shapes that reach the live tail are decided in a different package from the predicate guarding them, so every future shape resolving to the tail needs its own hand-added clause here.
To be explicit about what this is not: base gated flush() on the identical rawDirection === 'backward' condition, and a probe measured base and PR byte-identical on this shape. So this is an incomplete fix, not a regression — the race survives on a shape the new gate does not cover, rather than being newly introduced.
Witness:
PROBE-R1-2 PR: {"readPageArgs":["12345678-...-1234567890ab",{"maxBytes":4194304}],
"barrierCalls":0,"flushCalls":0,
"replayFinalizeDangling":true,"replayPendingToolCalls":[]}
PROBE-R1-2 BASE: {"readPageArgs":["12345678-...-1234567890ab",{"maxBytes":4194304}],
"barrierCalls":0,"flushCalls":0,
"replayFinalizeDangling":true,"replayPendingToolCalls":[]}
PROBE-R1-2-reader {"records":3,"hasMore":false} # direction and nextCursorState absent (undefined)
replayFinalizeDangling is read off the third argument to the mocked HistoryReplayer.replayPage, i.e. after the real (unmocked) replayTranscriptRecordPage applied its own gating — the effective value, not the caller's.
// Key the barrier on the same fact the replay keys finalization on —
// "this request is not pinned to a frozen snapshot" — not on the raw direction param.
const unanchored =
rawCursor === undefined &&
rawBeforeRecordId === undefined &&
rawAtRecordId === undefined;
const page =
recording !== undefined && unanchored
? await recording.runWithWriteBarrier(readPersistedPage)
: await readPersistedPage();
// and reword the comment to name the replayer's rule as the reason.The predicate must cover at least the pages history-replay-page.ts:389-390 lets finalize — finalizeDangling: finalizeDangling && (page.direction === 'backward' || !page.hasMore), with pendingToolCalls: page.direction === 'backward' ? [] : state.pendingToolCalls on the line above expressing the same resolved-shape notion. Note this widens the barrier to the direction-less shape, so it interacts with R1-1: settle the refusal semantics there first, or this pulls one more shape into a path that can 503. Please add a case calling agent.extMethod(SERVE_STATUS_EXT_METHODS.sessionTranscript, { sessionId: VALID_SESSION_ID }) with no direction and asserting recording.runWithWriteBarrier was called once — it is red today, and the existing does not barrier cursor or record-anchor transcript pages test must stay green, which pins the exemption to the three anchored shapes only.
中文说明
这段注释把不变量表述为「只对最新尾部加 barrier」，但代码是根据请求的 rawDirection 来决定的，而真正需要 barrier 的那个决定是在下游根据已解析的页面做出的：history-replay-page.ts:389-390 应用的是 finalizeDangling && (page.direction === 'backward' || !page.hasMore)。两个谓词覆盖的集合并不相同，而这个缺口可以从一条已上线的路由到达。
一个既无 direction、也无 cursor 和 anchor 的请求，在 reader 内部会解析为 direction: 'forward'、position 0（session-transcript-reader.ts:3406-3411）。对于记录数不超过分页上限的会话，该窗口一直读到 EOF，于是 hasMore 为 false、finalize 生效 —— 而此时 rawDirection === 'backward' 为假，读取根本不进入 barrier，也不会等待 operationTail。仍排队中的 recordToolResult 写入因此不在页面里，尾部 tool call 被 replay 成永久缺失。它是静默失败的：表现为数据而非错误。GET /session/:id/transcript（serve/routes/session.ts:4774）确实会转发这种形状：:4850-4857 展开 ...(direction !== undefined ? { direction } : {})，所以不带 direction 的 ?limit=100 会以无 direction 的形式到达子进程，而该路由把子进程返回的页面直接作为 HTTP 响应。请求语法是经过校验且有界的，所以这是一个具名形状而非无界家族 —— 但「哪些形状会到达实时尾部」是在另一个 package 里决定的，与守护它的谓词不同处，因此今后每个解析到尾部的形状都需要在这里手工补一个分支。
需要说清楚这不是什么：base 的 flush() 也是以完全相同的 rawDirection === 'backward' 条件来 gate 的，探针测得该形状下 base 与 PR 逐字节相同。所以这是一次不完整的修复，而不是回归 —— 竞态在一个新 gate 未覆盖的形状上存活，而非被新引入。
证据： 见上方 PROBE-R1-2 输出；replayFinalizeDangling 取自 mocked HistoryReplayer.replayPage 的第三个参数，即在真实（未 mock 的）replayTranscriptRecordPage 应用其自身 gating 之后，是生效值而非调用方传入值。
修法： 让 barrier 依据 replay 判定 finalize 所依据的同一个事实 —— 「本请求未被钉在一个冻结快照上」—— 而不是原始 direction 参数（见上方代码块），并把注释改为以 replayer 的规则作为理由。
约束： 该谓词至少必须覆盖 history-replay-page.ts:389-390 允许 finalize 的那些页面 —— finalizeDangling: finalizeDangling && (page.direction === 'backward' || !page.hasMore)，其上一行 pendingToolCalls: page.direction === 'backward' ? [] : state.pendingToolCalls 表达的是同一个「已解析形状」概念。注意这会把 barrier 扩大到无 direction 的形状，因此与 R1-1 相互影响：请先确定那里的拒绝语义，否则这会把又一个形状拉进一条可能 503 的路径。
验收标准： 请新增一个用例，调用 agent.extMethod(SERVE_STATUS_EXT_METHODS.sessionTranscript, { sessionId: VALID_SESSION_ID })（不带 direction）并断言 recording.runWithWriteBarrier 被调用一次 —— 它今天是红的；同时现有的 does not barrier cursor or record-anchor transcript pages 必须保持绿，从而把豁免钉在三个带 anchor 的形状上。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // after a write failure or during handoff. | ||
| const page = | ||
| recording !== undefined && rawDirection === 'backward' | ||
| ? await recording.runWithWriteBarrier(readPersistedPage) |
There was a problem hiding this comment.
[Suggestion] R1-4: runWithWriteBarrier does not merely await the recorder's write queue the way flush() did — it joins it (this.operationTail = pending.then(...) at chatRecordingService.ts:1657-1662, with await operation() inside at :1641). So the whole disk read now sits inside the write serialization point that base only observed: fsp.stat, an index build, and up to SESSION_TRANSCRIPT_MAX_PAGE_BYTES (4 MB) of read and parse.
That is expensive more often than it looks, because the index cache misses on every append: the cache key includes snapshotSize = stats.size and lastUpdated derived from stats.mtimeMs (session-transcript-reader.ts:3369-3395, key built at :2244-2249), so any record written since the last read invalidates it, and a miss streams and parses bytes 0..stats.size to rebuild the whole-session index (forEachLineInSnapshot, :1704-1723). Measured on a real 1.09 MB / 3001-record JSONL, a single appended record takes the read from 0.29 ms to 29.79 ms. While that runs, each enqueueRecordWrite the live turn issues chains behind it (chatRecordingService.ts:1357-1379), and assertCanStartTurn() — itself runWithWriteBarrier(async () => undefined) (:1665-1674), awaited at prompt admission (Session.ts:3904-3913) — queues behind it too, so a read-only poll delays the user's next prompt. The cost scales with transcript size, and it is paid most often by bridge.flushSessionTranscript, which discards the page it pays for. The five pre-existing sibling barrier sites (acpAgent.ts:4224, :12654, :12658 and the rewind paths) do the same thing, but they are low-frequency load/restore/rewind rather than a per-poll latest page.
Witness:
PROBE-R1-4-cost {"records":3001,"fileBytesBeforeAppend":1087167,
"coldReadMs":39.31,"warmReadMs":0.29,"readAfterOneAppendMs":29.79,
"coldRecords":2,"warmRecords":2,"afterAppendRecords":1}
real SessionTranscriptReader over a real 1.09 MB / 3001-record JSONL;
readAfterOneAppendMs vs warmReadMs is the cache-invalidation claim, measured not inferred.
witness: not run — the wall-clock stall inflicted on a concurrent LIVE TURN was not
measured; that needs a real recorder plus a real turn, which the mock scaffolding cannot
produce. The read cost above is the measured bound.
// Cheapest: give the flush-only caller something narrower than a one-record backward
// page, so the frequent flushSessionTranscript request drains the tail without parsing
// the file — an early return in the handler before getCachedIndex/replayTranscriptRecordPage,
// or a dedicated flush ext method.
// Durable: make the index build incremental — extend the cached index from the previously
// indexed offset when fileIdentity is unchanged and the file only grew — so a barriered
// latest-page read costs O(appended bytes) instead of O(file).A cheaper flush path must not simply fall back to flush()'s body: runWithWriteBarrier is the only path that both drains operationTail and re-asserts the writer lease around the operation (await lease.assertOwnedAndUnchanged() before, and await lease?.assertOwnedAndUnchanged() after operation()), and that post-read assertion is what keeps a writer handoff during the read from being reported as a consistent page. If neither narrowing is in scope here, measuring the stall on a large session with a live turn and recording the tradeoff under Risk & Scope would be enough — that section currently names only the extra wait, not its scaling.
中文说明
runWithWriteBarrier 不像 flush() 那样只是等待 recorder 的写入队列 —— 它会加入该队列（chatRecordingService.ts:1657-1662 的 this.operationTail = pending.then(...)，其中 :1641 处 await operation()）。因此整个磁盘读取现在位于 base 只是观察的那个写入串行点内部：fsp.stat、一次索引构建，以及最多 SESSION_TRANSCRIPT_MAX_PAGE_BYTES（4 MB）的读取与解析。
这比看上去更频繁地昂贵，因为索引缓存在每次追加时都会 miss：缓存键包含 snapshotSize = stats.size 与由 stats.mtimeMs 推导出的 lastUpdated（session-transcript-reader.ts:3369-3395，键构造于 :2244-2249），所以自上次读取以来写入的任何记录都会使其失效，而一次 miss 会流式读取并解析 0..stats.size 的字节以重建整个会话索引（forEachLineInSnapshot，:1704-1723）。在真实的 1.09 MB / 3001 条记录 JSONL 上实测，仅追加一条记录就会让读取从 0.29 ms 变成 29.79 ms。在这段时间里，实时 turn 发出的每次 enqueueRecordWrite 都排在它后面（chatRecordingService.ts:1357-1379），assertCanStartTurn() 也排在它后面 —— 它本身就是 runWithWriteBarrier(async () => undefined)（:1665-1674），在 prompt 准入时被 await（Session.ts:3904-3913）—— 于是一次只读轮询会拖延用户的下一个 prompt。成本随 transcript 大小增长，而付出最多的调用方是 bridge.flushSessionTranscript，它把自己换来的页面丢弃。已有的五个同类 barrier 站点（acpAgent.ts:4224、:12654、:12658 以及 rewind 路径）做的是同一件事，但它们是低频的 load/restore/rewind，而不是每次轮询的最新页。
证据： 见上方 PROBE-R1-4-cost 实测数字；对并发实时 turn 造成的实际墙钟停顿未测量（需要真实 recorder 加真实 turn，mock 脚手架无法产生），上面的读取成本是已测得的下界。
修法： 最省的做法是给「只为 flush」的调用方一个比「一条记录的 backward 页」更窄的东西，让频繁的 flushSessionTranscript 请求 drain 尾部而不解析文件 —— 在 handler 中于 getCachedIndex/replayTranscriptRecordPage 之前提前返回，或提供一个专用的 flush ext method。更持久的做法是让索引构建增量化 —— 当 fileIdentity 未变且文件只是增长时，从上次已索引的偏移扩展缓存索引 —— 使加 barrier 的最新页读取成本从 O(文件) 降为 O(追加字节)。
约束： 更廉价的 flush 路径不能简单退回 flush() 的实现：runWithWriteBarrier 是唯一既 drain operationTail、又在操作前后重新断言 writer lease 的路径（操作前 await lease.assertOwnedAndUnchanged()，operation() 之后 await lease?.assertOwnedAndUnchanged()），而这次读取后的断言正是防止「读取期间发生 writer handoff 却被报告为一个一致页面」的机制。如果两种收窄都不在本 PR 范围内，那么在一个大会话加上实时 turn 的场景下实测该停顿、并把取舍写进 Risk & Scope 也足够 —— 目前那一节只提到额外等待，没有提到它的增长关系。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
|
||
| const prompt = agent.prompt({ sessionId: VALID_SESSION_ID, prompt: [] }); | ||
| await vi.waitFor(() => expect(lastSessionMock!.prompt).toHaveBeenCalled()); | ||
| lastSessionMock!.isTurnIdle.mockReturnValue(false); |
There was a problem hiding this comment.
[Suggestion] R1-9: This rewrite ports only one of the two directions the helper's contract needs. The loadUpdates route this PR copies finalizeDanglingForRestore from pins both — :17538 for a turn that settles during the read and :17581 for a turn that starts during it — but only the "settles" half was ported to qwen/status/session/transcript. The result is that the replay-time sample is unguarded on this route: the only transcript-route assertions are :17411 (expects false) and :17420 (expects true), and both are decided by turnIdleBeforeRead alone (false && x = false; true && true = true), so the liveSession argument at acpAgent.ts:9066 can be dropped without any test failing.
That is not a hypothetical mutation — it was run. Replacing the call with finalizeDanglingForRestore(undefined, turnIdleBeforeRead), equivalently finalizeDangling: turnIdleBeforeRead, survives the entire suite. In production that loses the second sample: a session that is idle when the Web UI polls the latest page and then starts a turn during the disk read — a client prompt, or a goal/cron/notification turn, all counted by Session.#hasActiveTurn() (session/Session.ts:4025-4039) — replays with finalizeDangling: true and emits a synthetic terminal update for a trailing tool call whose result the live stream is about to write. That is the false dangling tool result #9704 describes, reintroduced on this route with a green suite.
Witness:
Mutation run in a scratch tree, four runs of packages/cli/src/acp-integration/acpAgent.test.ts:
intact PR -> Tests 624 passed (624)
mutant finalizeDanglingForRestore(undefined, turnIdleBeforeRead)
-> Tests 624 passed (624) # MUTANT SURVIVES
proposed symmetric test added, mutant in -> x PROBE keeps a dangling transcript call
pending when a turn starts during the read
AssertionError:
- "finalizeDangling": false,
+ "finalizeDangling": true
proposed symmetric test added, mutant reverted -> Tests 625 passed (625)
The probe FLIPS, so both the gap and the fix below are real.
Adjacent mutation, for contrast: `const turnIdleBeforeRead = true;` makes this rewritten test
at :17378 go RED while the replay-time probe stays GREEN — the before-read sample IS pinned,
the replay-time one is not.
// Symmetric case, mirroring :17581 on the loadUpdates route:
it('keeps a dangling transcript call pending when a turn starts during the read', async () => {
// live session; isTurnIdle true before the call ...
lastSessionMock!.isTurnIdle.mockReturnValue(true);
readPage.mockImplementationOnce(async () => {
// ... and flipped to false INSIDE the read, anchoring the flip to the read boundary
lastSessionMock!.isTurnIdle.mockReturnValue(false);
await new Promise<void>((resolve) => setImmediate(resolve));
return page;
});
// assert mockHistoryReplayPage was last called with
// expect.objectContaining({ finalizeDangling: false })
});The new case belongs to the ungated status route only: acpAgent.ts:4620-4630 states the helper is not for the gated live-load path ("that restore runs under the close gate, which drains active turns, blocks new ones, and reports closing=true — so isTurnIdle() there is structurally false"), and loadSession's opposite expectation is pinned at :17467 with a different mock (mockHistoryReplay, not mockHistoryReplayPage) — it stayed green in the 625/625 run above, so the addition does not collide with it. Please confirm the acceptance criterion by mutation: with the new test in place, replace liveSession at acpAgent.ts:9066 with undefined and check the new test reds while :17379 still passes.
中文说明
这次改写只移植了 helper 契约所需的两个方向中的一个。本 PR 从中复制 finalizeDanglingForRestore 的 loadUpdates 路由两个方向都固定了 —— :17538 覆盖读取期间 turn 结束，:17581 覆盖读取期间 turn 开始 —— 但只有「结束」那一半被移植到 qwen/status/session/transcript。结果是这条路由上「replay 时刻的采样」无人守护：transcript 路由仅有的断言是 :17411（期望 false）与 :17420（期望 true），而两者都只由 turnIdleBeforeRead 决定（false && x = false；true && true = true），因此 acpAgent.ts:9066 的 liveSession 参数可以被去掉而没有任何测试失败。
这不是假想的变异 —— 它已被实际运行。把该调用替换为 finalizeDanglingForRestore(undefined, turnIdleBeforeRead)（等价于 finalizeDangling: turnIdleBeforeRead）能在整个测试套件中存活。在生产中这就丢失了第二次采样：一个在 Web UI 轮询最新页时处于空闲、随后在磁盘读取期间开始一个 turn 的会话 —— 客户端 prompt，或 goal/cron/notification turn，全部被 Session.#hasActiveTurn() 计入（session/Session.ts:4025-4039）—— 会以 finalizeDangling: true 进行 replay，并为一个尾部 tool call 发出合成的终态更新，而它的结果实时流即将写入。这正是 #9704 所描述的误报 dangling tool result，在套件全绿的情况下于这条路由上被重新引入。
证据： 见上方变异运行输出（四次运行，探针会翻转，说明缺口与修法都真实存在）。作为对照的相邻变异：const turnIdleBeforeRead = true; 会让 :17378 这个被改写的测试变红，而 replay 时刻的探针仍为绿 —— 读取前的采样是被固定的，replay 时刻的没有。
修法： 补一个对称用例，参照 loadUpdates 路由的 :17581（见上方代码块）：读取前 isTurnIdle 为 true，在 readPage.mockImplementationOnce 内部翻转为 false（把翻转锚定在读取边界上），然后断言 mockHistoryReplayPage 最后一次被调用时收到 expect.objectContaining({ finalizeDangling: false })。
约束： 新用例只属于未加 gate 的 status 路由。acpAgent.ts:4620-4630 说明该 helper 不用于加了 gate 的 live-load 路径（「该 restore 在 close gate 下运行，它会 drain 活跃 turn、阻止新 turn，并报告 closing=true —— 所以那里 isTurnIdle() 在结构上为 false」），而 loadSession 相反的期望固定在 :17467，且用的是不同的 mock（mockHistoryReplay 而非 mockHistoryReplayPage）—— 它在上面的 625/625 运行中保持绿色，所以新增用例不会与它冲突。
验收标准： 请用变异确认：加入新测试后，把 acpAgent.ts:9066 的 liveSession 替换为 undefined，确认新测试变红而 :17379 仍然通过。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| it('does not barrier cursor or record-anchor transcript pages', async () => { | ||
| const innerConfig = await setupSessionMocks(VALID_SESSION_ID); | ||
| const recording = innerConfig.getChatRecordingService(); | ||
| recording.runWithWriteBarrier.mockRejectedValue( |
There was a problem hiding this comment.
[Suggestion] R1-10: This rejecting mock is used only to prove the three anchored shapes never reach the barrier. Nothing pins what the backward/latest page does when the barrier refuses — which is the new failure mode this diff deliberately introduces on that path, and which the commit message states as intended.
So both resolutions of that question currently ship green. A later change that swallows the rejection and falls back to readPersistedPage() would silently reintroduce the #9704 interleaving this PR exists to fix; a change that lets the error escape unmapped would surface an internal error instead of -32023 / session_writer_unavailable. Neither is caught. The path is live rather than theoretical: refreshedReplayFieldsFor sends {direction: 'backward'} for the first page (bridge.ts:7424-7427), POST /session/:id/load always sends historyReplay: 'response' (serve/routes/session.ts:3947) and reaches it via bridge.ts:7750-7756, and the Web UI's own latest-page poll sends direction: 'backward' whenever it has no cursor (web-shell/client/App.tsx:5364-5370).
This is complementary to the Critical on acpAgent.ts:9057, not an alternative to it: whichever refusal semantics is settled on there needs pinning here. If the fallback is adopted, pin the fallback and that the barrier was attempted first; if the refusal is kept, pin -32023.
Witness:
Mutant run in a scratch tree:
mutant `await recording.runWithWriteBarrier(readPersistedPage).catch(() => readPersistedPage())`
-> Test Files 2 passed (2) / Tests 627 passed (627)
across acpAgent.test.ts + acpAgent.worktree.test.ts
(the only two test files that import ./acpAgent.js) # NOTHING PINS THE REFUSAL
PROBE-A direction:'backward' + a real SessionWriterUnavailableError
(from the mocked core class at acpAgent.test.ts:689-692)
-> passes: rejects.toMatchObject({ code: -32023,
data: { errorKind: 'session_writer_unavailable' } })
runWithWriteBarrier called once, readPage NOT called
PROBE-B plain new Error('recorder not accepting writes') <- the shape used at :17116
-> surfaces UNMAPPED: AssertionError: expected Error: recorder not accepting writes
to match object {...} with diff - "code": undefined
Real constants confirmed in core, not just in the mock:
SESSION_WRITER_RPC_CODES.session_writer_unavailable = -32023 (session-writer-lease.ts:165-170)
// Pin the refusal (or the fallback, if that is the semantics chosen for acpAgent.ts:9057):
it('refuses the latest transcript page when the recorder is not accepting writes', async () => {
recording.runWithWriteBarrier.mockRejectedValue(new SessionWriterUnavailableError());
await expect(
agent.extMethod(SERVE_STATUS_EXT_METHODS.sessionTranscript, {
sessionId: VALID_SESSION_ID,
direction: 'backward',
limit: 1,
}),
).rejects.toMatchObject({
code: -32023,
data: { errorKind: 'session_writer_unavailable' },
});
expect(readPage).not.toHaveBeenCalled();
});The rejected value must be a real SessionWriterUnavailableError carrying the matching rpcCode/errorKind pair — acpAgent.ts:740-742 (if (candidate['rpcCode'] !== SESSION_WRITER_RPC_CODES[typedKind]) { return undefined; }) means a plain new Error(...), the shape this line already uses, is not mapped by getSessionWriterError at all, and the assertion would fail for the wrong reason; PROBE-B above is that failure measured. Please confirm the new test discriminates by mutation: add the .catch(() => readPersistedPage()) fallback shown in the witness and check the test reds.
中文说明
这个会拒绝的 mock 只被用来证明三个带 anchor 的形状根本不会到达 barrier。没有任何测试固定「当 barrier 拒绝时，backward/最新页会怎样」—— 而这正是本 diff 在该路径上有意引入的新失败模式，commit message 也明确说明是有意的。
于是这个问题的两种答案目前都能全绿通过。之后若有改动吞掉该拒绝并回退到 readPersistedPage()，就会静默地重新引入本 PR 要修复的 #9704 交错；若有改动让该错误未被映射地逃逸，就会呈现为内部错误而不是 -32023 / session_writer_unavailable。两者都不会被发现。这条路径是真实可达而非理论性的：refreshedReplayFieldsFor 为第一页发送 {direction: 'backward'}（bridge.ts:7424-7427），POST /session/:id/load 总是发送 historyReplay: 'response'（serve/routes/session.ts:3947）并经 bridge.ts:7750-7756 到达这里，而 Web UI 自身的最新页轮询在没有 cursor 时总会发送 direction: 'backward'（web-shell/client/App.tsx:5364-5370）。
这与 acpAgent.ts:9057 上的 Critical 是互补关系，而不是替代关系：那里最终选定哪种拒绝语义，这里就需要把它固定下来。如果采用回退，就固定「回退发生、且 barrier 先被尝试过」；如果保留拒绝，就固定 -32023。
证据： 见上方变异与 PROBE-A/PROBE-B 实测输出。核心常量在 core 中确认（不只是 mock）：SESSION_WRITER_RPC_CODES.session_writer_unavailable = -32023（session-writer-lease.ts:165-170）。
修法： 见上方代码块，新增一个用例固定拒绝语义（或改为固定回退语义，取决于 acpAgent.ts:9057 的选择）。
约束： 被拒绝的值必须是携带匹配 rpcCode/errorKind 的真实 SessionWriterUnavailableError —— acpAgent.ts:740-742（if (candidate['rpcCode'] !== SESSION_WRITER_RPC_CODES[typedKind]) { return undefined; }）意味着一个普通的 new Error(...)（也就是本行现在使用的形状）根本不会被 getSessionWriterError 映射，断言会因为错误的原因失败；上方的 PROBE-B 就是这个失败的实测结果。
验收标准： 请用变异确认新测试具备判别力：加上证据中展示的 .catch(() => readPersistedPage()) 回退，确认该测试变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
Thanks @now-ing for the independent read — agreed on the shape (reuse On the micro-question: yes, Separately, triage R1-1 is right that a lifecycle-inactive recorder must not fail-closed a read-only latest page the way a bare barrier does today. I will keep the barrier for in-flight writes, but fall back to a direct disk read when the recorder is only lifecycle-unavailable (and not in |
…corder (QwenLM#9704) Keep the write barrier for in-flight tool-result writes. If the barrier refuses because the recorder is only lifecycle-unavailable and has no writeFailure, read the persisted page directly. A latched writeFailure still fails the read. Co-authored-by: Zhu Lei <kabishou11@users.noreply.github.com>
|
Addressed Critical R1-1 on Backward/latest reads still go through Also tightened the gate comment (request Local: full |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 barrier gate keyed on the request direction rather than the resolved page direction - still stands, already reported (comment 3943316415)
- R1-4 the barrier joins the write serialization point rather than merely awaiting it - still stands, already reported (comment 3943316417)
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": did not enumerate getTranscriptState / sameHardTranscriptState / reconcileTranscriptMetadata in session-writer-lease.ts to rule out whether the barrier's pr…; "agent reverse-audit (round 2)": did not execute the four added tests or the rewritten one (this worktree needs npm run build for the workspace dist/ prerequisites), so the oracle/vacuity a….
[Critical] CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678, and carried as unresolved by the round-1 review 5124611584) - STILL STANDS at the reviewed commit. Verified against the actions runs API for head 0d48550: Qwen Code CI, SDK Java and tui-parity are all action_required, i.e. the three pull_request-triggered workflows have never started on this commit and produce no check runs at all. The 12 check runs that do exist are bot orchestration and precheck plumbing (label, assign, authorize, precheck-pr, delay-automatic-review, review-config, Remind on force-push, PR self-report label, Assign PR owner, PR Force-Push Reminder) - none compiles the change or runs a test, and no lint signal exists on the commit in any form. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list, which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 affected workspaces, packages/cli tests are 1013 passed / 1 failed file of 1014 with the single failing file (src/config/settings.test.ts) also failing on the merge base (measured netNew: [], shared: ["src/config/settings.test.ts"], so pre-existing by measurement and not attributable to this diff), and acpAgent.test.ts passes 627/627 both inside the suite and in isolation - but that is this review's local run, not CI on the commit, and it does not include lint.
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：did not enumerate getTranscriptState / sameHardTranscriptState / reconcileTranscriptMetadata in session-writer-lease.ts to rule out whether the barrier's pr…；"agent reverse-audit (round 2)"：did not execute the four added tests or the rewritten one (this worktree needs npm run build for the workspace dist/ prerequisites), so the oracle/vacuity a…。
[Critical] CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678, and carried as unresolved by the round-1 review 5124611584) - STILL STANDS at the reviewed commit. Verified against the actions runs API for head 0d48550: Qwen Code CI, SDK Java and tui-parity are all action_required, i.e. the three pull_request-triggered workflows have never started on this commit and produce no check runs at all. The 12 check runs that do exist are bot orchestration and precheck plumbing (label, assign, authorize, precheck-pr, delay-automatic-review, review-config, Remind on force-push, PR self-report label, Assign PR owner, PR Force-Push Reminder) - none compiles the change or runs a test, and no lint signal exists on the commit in any form. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list, which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 affected workspaces, packages/cli tests are 1013 passed / 1 failed file of 1014 with the single failing file (src/config/settings.test.ts) also failing on the merge base (measured netNew: [], shared: ["src/config/settings.test.ts"], so pre-existing by measurement and not attributable to this diff), and acpAgent.test.ts passes 627/627 both inside the suite and in isolation - but that is this review's local run, not CI on the commit, and it does not include lint.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| throw error; | ||
| } | ||
| return readPersistedPage(); |
There was a problem hiding this comment.
[Critical] R1-1: (fix-induced) [certifies-falsely] [regression] The lifecycle fallback you added to answer the round-1 blocker reads the transcript without draining the recorder's write tail, so a backward/latest page can be served missing records that were already queued for writing — while reporting hasMore: false.
The line this hunk replaced drained first. Base called getChatRecordingService()?.flush() on exactly this branch, and flush() is await this.operationTail; if (this.writeFailure) throw this.writeFailure; (chatRecordingService.ts:1606-1610) — it waits regardless of recorder state. runWithWriteBarrier re-provides that drain only on the admitted path: it refuses synchronously, before touching operationTail, when !acceptingWrites || state !== 'active' (chatRecordingService.ts:1626-1631). So on the fallback branch nothing waits, and the comment above it ("The barrier still waits for in-flight writes") states the opposite of what this branch does.
The window is one core itself assumes exists. beginClose() sets acceptingWrites = false / state = 'closing' synchronously (chatRecordingService.ts:1698-1706) while closeOnce() is still inside await this.flush() draining what the just-aborted turn queued — close() is this.beginClose(options); const pending = this.closeOnce();, and closeOnce() opens with that flush. Two paths keep the session in this.sessions throughout: beginManagedShutdown fires an unawaited void session.cancelPendingPrompt() (acpAgent.ts:3716) and then config.closeSessionWriter({ handoff: true }) (:3734), deleting entries only at :4186; and removeStoredSessionEntry runs dispose() then await config.shutdown() before its delete (:4163-4186). A client polling qwen/status/session/transcript with direction: 'backward' in either window is refused by the barrier, the predicate returns true (no latched writeFailure, acceptingWrites === false), and readPersistedPage() scans the file immediately. The response omits the session's final assistant message or tool result while asserting hasMore: false, so the page is certified complete when it is not — and it is the client's last view of that session, since the entry disappears from the next snapshot.
Risk & Scope's own rationale does not answer this. It names daemon flushSessionTranscript as a consumer the fallback keeps working, but that consumer sends {direction:'backward', limit:1} purely as a drain and discards the child's page, reading the JSONL itself (bridge.ts:12229-12237, serve/routes/session.ts:5000-5028) — so the fallback keeps the request succeeding while dropping the one thing that caller buys. Worth correcting in the same pass: bridge.ts:12230's "The child flushes before every backward page" (raised in round 1) is now false on this branch as well.
Witness:
A/B probe. Real recorder shape, one queued write that only flush() drains, recorder
hand-set to acceptingWrites:false / state:'closing', barrier rejecting with the real
SessionWriterUnavailableError. BASE arm built via base-tree + revert-hunk.
PR : PROBE-R1-1 {"flushCalls":0,"readPageCalls":1,"replayedUuids":["call-record"],
"pageHasQueuedRecord":false,"reportedHasMore":false}
BASE : PROBE-R1-1 {"flushCalls":1,"readPageCalls":1,
"replayedUuids":["call-record","tool-result-record"],
"pageHasQueuedRecord":true,"reportedHasMore":false}
fix-flip (await recording.flush().catch(() => undefined) before the fallback read):
PR+fix: PROBE-R1-1 {"flushCalls":1,...,"pageHasQueuedRecord":true} <- matches BASE
x falls back to a direct latest transcript page when the recorder is only lifecycle-unavailable
-> expected "spy" to not be called at all, but actually been called 1 times
v propagates a latest transcript page refusal when the recorder has a writeFailure
pageHasQueuedRecord and flushCalls are the deciding numbers: base drained and served the queued record, the PR neither drained nor served it, and the fix-flip restores base exactly.
.catch(async (error: unknown) => {
if (!isWriterLifecycleUnavailable(recording)) {
throw error;
}
await recording.flush().catch(() => undefined);
return readPersistedPage();
})This is free in the states the fallback was added for — for an inactive/closed recorder flush() awaits an already-settled tail and returns. The drain must stay .catch-guarded: flush() rethrows a latched writeFailure (chatRecordingService.ts:1606-1610), and letting that escape turns the best-effort lifecycle read back into the hard -32023 session_writer_unavailable refusal the fallback exists to avoid, which is the behaviour your sibling new test pins (code: -32023, data.errorKind: 'session_writer_unavailable', readPage not called). operationTail is documented "Serializes appends and authoritative read barriers. Always settles." (:938), so awaiting it cannot hang the request.
Please invert expect(recording.flush).not.toHaveBeenCalled(); in falls back to a direct latest transcript page when the recorder is only lifecycle-unavailable — today it pins the absence of the drain, so removing a drain you add would go unnoticed. The mutation is measurable: with the fix in, that assertion reds (witness above). Ideally make it an ordering assertion of the shape your serializes live transcript reads behind in-flight tool-result writes case already uses — a flush mock that resolves only after a queued record becomes visible to readPage, then assert readPage observed it — so deleting the await recording.flush() again turns the test red.
中文说明
你为回应第一轮阻塞项而加的生命周期回退，在读取 transcript 时没有 drain recorder 的写入尾部，于是一个 backward/最新页可能在「已排队但尚未落盘」的记录缺失的情况下被返回 —— 同时还报告 hasMore: false。
这个 hunk 替换掉的那一行是先 drain 的。base 在这条分支上调用 getChatRecordingService()?.flush()，而 flush() 就是 await this.operationTail; if (this.writeFailure) throw this.writeFailure;（chatRecordingService.ts:1606-1610）—— 它无论 recorder 处于什么状态都会等待。runWithWriteBarrier 只在被准入的路径上重新提供了这个 drain：当 !acceptingWrites || state !== 'active' 时，它会在触碰 operationTail 之前同步抛出（chatRecordingService.ts:1626-1631）。所以在回退分支上没有任何东西在等待，而它上方的注释（「The barrier still waits for in-flight writes」）说的正是这条分支实际行为的反面。
这个窗口是 core 自己就假定存在的。beginClose() 会同步置 acceptingWrites = false / state = 'closing'（chatRecordingService.ts:1698-1706），而此时 closeOnce() 仍在 await this.flush() 里 drain 刚被中止的 turn 排队的记录 —— close() 就是 this.beginClose(options); const pending = this.closeOnce();，而 closeOnce() 以该 flush 开头。有两条路径在整个过程中都把会话保留在 this.sessions 里：beginManagedShutdown 先发出一个未 await 的 void session.cancelPendingPrompt()（acpAgent.ts:3716），随后调用 config.closeSessionWriter({ handoff: true })（:3734），而条目要到 :4186 才删除；removeStoredSessionEntry 则先 dispose()、再 await config.shutdown()，最后才删除（:4163-4186）。在这两个窗口中，一个带 direction: 'backward' 的 qwen/status/session/transcript 轮询会被 barrier 拒绝，谓词返回 true（没有锁定的 writeFailure，acceptingWrites === false），于是 readPersistedPage() 立即扫盘。响应会漏掉该会话最后一条 assistant 消息或 tool result，却断言 hasMore: false —— 页面被证明是完整的，而实际不是；而且这是客户端对该会话的最后一次视图，因为条目会从下一个快照中消失。
Risk & Scope 自己的理由并不能回应这一点。它把 daemon 的 flushSessionTranscript 列为回退保住可用的调用方，但那个调用方发送 {direction:'backward', limit:1} 纯粹是当作 drain 使用，并且会丢弃子进程返回的页面、自己读取 JSONL（bridge.ts:12229-12237、serve/routes/session.ts:5000-5028）—— 所以回退让请求继续成功，却丢掉了那个调用方唯一想买的东西。同一轮里值得顺手改正：bridge.ts:12230 的「The child flushes before every backward page」（第一轮已提出）在这条分支上现在也不成立了。
证据： 见上方 PROBE-R1-1 输出。pageHasQueuedRecord 与 flushCalls 是决定性数字：base 先 drain 并返回了那条排队的记录，PR 既没有 drain 也没有返回它，而 fix-flip 完全恢复了 base 的行为。
修法： 见上方代码块 —— 在回退读取之前 drain 尾部。
约束： 该 drain 必须保持 .catch 保护。flush() 会重抛已锁定的 writeFailure（chatRecordingService.ts:1606-1610），若让它逃逸，就会把这个尽力而为的生命周期读取重新变回回退本要避免的硬 -32023 session_writer_unavailable 拒绝 —— 而那正是你另一个新测试所固定的行为（code: -32023、data.errorKind: 'session_writer_unavailable'、readPage 未被调用）。operationTail 的文档写明「Serializes appends and authoritative read barriers. Always settles.」（:938），所以 await 它不会挂住请求。
验收标准： 请把 falls back to a direct latest transcript page when the recorder is only lifecycle-unavailable 中的 expect(recording.flush).not.toHaveBeenCalled(); 反转 —— 它今天固定的是「没有 drain」，所以你加上 drain 后再被移除也不会被发现。该变异是可测量的：加上修法后该断言会变红（见上方证据）。最好改成你的 serializes live transcript reads behind in-flight tool-result writes 已经使用的顺序断言形式 —— 让 flush mock 只在某条排队记录对 readPage 可见之后才 resolve，然后断言 readPage 观察到了它 —— 这样再次删掉 await recording.flush() 就会让测试变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| finalizeDangling: this.finalizeDanglingForRestore( | ||
| liveSession, | ||
| turnIdleBeforeRead, |
There was a problem hiding this comment.
[Suggestion] R2-1: Routing this ungated transcript read through finalizeDanglingForRestore folds Session.closing into the turn-activity sample, because isTurnIdle() is !this.closing && !this.#hasActiveTurn() (session/Session.ts:3927-3929). A poll served while another request holds that session's close gate — or after the session was disposed but before it left this.sessions — reports finalizeDangling: false and leaves an already-abandoned trailing tool call pending, where the sample it replaced (!activePromptBeforeRead && !this.activePromptCalls.has(sessionId)) finalized it.
Three windows reach this, all with the session still mapped and the status route still served (assertManagedSessionAdmission() is never called in the sessionTranscript case; its call sites are :13709/13912/14015/14267+). withLiveSessionRestore takes session.beginClose() at acpAgent.ts:4232 and holds it across waitForSessionDrain, bounded by SESSION_DRAIN_TIMEOUT_MS = 30_000 (:496), releasing only in finally — and once the drain succeeds there is provably no active turn, yet closing is still true. removeStoredSessionEntry disposes the session (Session.dispose() sets closing = true permanently, Session.ts:4147-4150) and keeps the entry across await config.shutdown(). And beginManagedShutdown calls session.beginCloseIfAvailable() at :3707-3709 and discards the release function, so closing stays true until finishManagedShutdown deletes the entries. In each, a qwen/status/session/transcript poll samples false twice and returns a trailing tool call as still-running for a session whose prompt was already cancelled or drained — a spinner no live stream can ever clear.
This is a Suggestion rather than a blocker because the window is bounded and the direction is conservative: closeStoredSession's finally { if (!removedFromStore) cancelClose(); } (:4503-4505) resets closing, removeStoredSessionEntry deletes unconditionally (:4186), and the route is non-live afterwards — so it self-corrects on the next ungated poll, and it never fabricates a tool result. But the helper's own docstring names exactly this hazard ("that restore runs under the close gate, which drains active turns, blocks new ones, and reports closing=true — so isTurnIdle() there is structurally false and would keep genuinely abandoned calls pending forever", acpAgent.ts:4640-4652) and warns off only the loadSession caller, not a third caller that can be served under that caller's gate. That docstring is also now the only place in the file recording this interaction, and it still names only qwen/session/loadUpdates — which has no production consumer in this repo (grep over non-test packages/** finds only the handler at :12667, the doc mentions, and serve/large-pipe-frame-observer.ts:197/227, a response-shape classifier that never sends the request). So a maintainer asking "when can the transcript route leave a call pending?" is pointed at one dead caller and away from the window this diff opened.
Witness:
Probe over the real route, live session with isTurnIdle() false and no active turn:
BASE : PROBE-R2-1 {"finalizeDangling":true}
PR : PROBE-R2-1 {"finalizeDangling":false}
PR : PROBE-R2-7 {"finalizeDangling":true} (no live session - the self-heal state;
both arms agree, which is what bounds it)
Docstring sub-claim: grep 'qwen/session/loadUpdates' over non-test packages/** ->
handler acpAgent.ts:12667; doc comments :4605, :4641; comment in
session/history-replay-page.ts:275; serve/large-pipe-frame-observer.ts:197/227
(isLoadUpdatesResult - classifies a response shape, calls nothing). No production caller.
Separate "a turn is active" from "the close gate is held" for this ungated route — Session.#hasActiveTurn() is private (Session.ts:4025), so this needs a new public accessor used here, or an option on finalizeDanglingForRestore that ignores closing when the caller is not the gated restore.
isTurnIdle() itself must not change: return !this.closing && !this.#hasActiveTurn(); (Session.ts:3927-3929) is also the busy-check that rejects turns at acpAgent.ts:12495 and Session.ts:4044, and isIdle() (Session.ts:3931-3933) builds on it. Any new accessor must keep #hasActiveTurn()'s coverage of historyMutationActive, pendingPromptCompletion and the goal/cron/notification flags (Session.ts:4025-4039) — that coverage is what makes this switch an improvement over activePromptCalls in the first place — and must not change the second caller at acpAgent.ts:12720 or the live loadSession path's unconditional finalize.
Please add a case in acpAgent.test.ts where the session reports closing-with-no-active-turn and assert mockHistoryReplayPage receives expect.objectContaining({ finalizeDangling: true }); it must go red if the closing term is not separated out. The two existing turn-race cases cannot pin this — they only toggle lastSessionMock.isTurnIdle, so they cannot distinguish "active turn" from "gate held" and stay green either way. That they cannot is itself the evidence the route needs the narrower signal. When you amend the docstring, extend it rather than replacing it with a shorter one.
中文说明
把这条未加 gate 的 transcript 读取接到 finalizeDanglingForRestore 上，会把 Session.closing 一起折进 turn 活跃度采样里，因为 isTurnIdle() 是 !this.closing && !this.#hasActiveTurn()（session/Session.ts:3927-3929）。当另一个请求正持有该会话的 close gate 时 —— 或会话已 dispose 但尚未从 this.sessions 移除时 —— 一次轮询会得到 finalizeDangling: false，于是一个早已被放弃的尾部 tool call 被留作 pending；而被它替换掉的采样（!activePromptBeforeRead && !this.activePromptCalls.has(sessionId)）在这种情况下会 finalize。
有三个窗口可以到达这里，且会话都仍在 map 中、status 路由仍被服务（sessionTranscript 分支从不调用 assertManagedSessionAdmission()，其调用点是 :13709/13912/14015/14267+）。withLiveSessionRestore 在 acpAgent.ts:4232 取得 session.beginClose()，并跨 waitForSessionDrain 持有它，上界为 SESSION_DRAIN_TIMEOUT_MS = 30_000（:496），只在 finally 中释放 —— 而 drain 成功之后可证明已无活跃 turn，但 closing 仍为 true。removeStoredSessionEntry 会 dispose 会话（Session.dispose() 永久置 closing = true，Session.ts:4147-4150），并在 await config.shutdown() 期间保留条目。beginManagedShutdown 则在 :3707-3709 调用 session.beginCloseIfAvailable() 并丢弃了释放函数，因此 closing 会一直保持 true，直到 finishManagedShutdown 删除条目。在这三种情况下，一次 qwen/status/session/transcript 轮询会两次采样到 false，从而把一个 prompt 已被取消或已 drain 的会话的尾部 tool call 返回为「仍在运行」—— 一个没有任何实时流能清除的 spinner。
这是 Suggestion 而非阻塞项，因为窗口是有界的、方向是保守的：closeStoredSession 的 finally { if (!removedFromStore) cancelClose(); }（:4503-4505）会重置 closing，removeStoredSessionEntry 无条件删除（:4186），之后该路由就是非 live 的 —— 所以它会在下一次未加 gate 的轮询时自我修正，而且它绝不会伪造 tool result。但 helper 自己的文档注释恰好点名了这个隐患（「该 restore 在 close gate 下运行，它会 drain 活跃 turn、阻止新 turn，并报告 closing=true —— 所以那里 isTurnIdle() 在结构上为 false，会让真正被放弃的调用永远挂起」，acpAgent.ts:4640-4652），却只警告了 loadSession 调用方，没有警告一个可能在该调用方的 gate 之下被服务的第三个调用方。而且该注释现在是文件中唯一记录这一交互的地方，它仍然只提到 qwen/session/loadUpdates —— 而该方法在本仓库中没有任何生产调用方（在非测试的 packages/** 上 grep 只找到 :12667 的 handler、文档提及，以及 serve/large-pipe-frame-observer.ts:197/227 这个只识别响应形状、从不发起请求的分类器）。于是当一个维护者问「transcript 路由什么时候会把调用留作 pending？」时，文档把他指向一个死调用方，而远离了本 diff 打开的这个窗口。
证据： 见上方 PROBE-R2-1 / PROBE-R2-7 输出，以及 qwen/session/loadUpdates 的 grep 结果。
修法： 为这条未加 gate 的路由把「turn 活跃」与「close gate 被持有」分开 —— Session.#hasActiveTurn() 是私有的（Session.ts:4025），所以这需要一个在此处使用的新公开访问器，或者给 finalizeDanglingForRestore 加一个选项，在调用方不是加 gate 的 restore 时忽略 closing。
约束： isTurnIdle() 本身不能改：return !this.closing && !this.#hasActiveTurn();（Session.ts:3927-3929）同时是 acpAgent.ts:12495 与 Session.ts:4044 处拒绝 turn 的忙碌检查，且 isIdle()（Session.ts:3931-3933）建立在它之上。任何新访问器都必须保留 #hasActiveTurn() 对 historyMutationActive、pendingPromptCompletion 以及 goal/cron/notification 标志的覆盖（Session.ts:4025-4039）—— 正是这份覆盖让这次替换相对 activePromptCalls 成为改进 —— 并且不得改变 acpAgent.ts:12720 的第二个调用方，也不得改变 live loadSession 路径的无条件 finalize。
验收标准： 请在 acpAgent.test.ts 中新增一个用例，让会话报告「closing 但无活跃 turn」，并断言 mockHistoryReplayPage 收到 expect.objectContaining({ finalizeDangling: true })；如果没有把 closing 这一项分离出来，它必须变红。现有的两个 turn 竞态用例无法固定这一点 —— 它们只切换 lastSessionMock.isTurnIdle，因此无法区分「turn 活跃」与「gate 被持有」，两种情况下都保持绿色。它们无法区分，本身就是这条路由需要更窄信号的证据。修改文档注释时请扩充它，而不是替换成更短的版本。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| function isWriterLifecycleUnavailable(recording: object): boolean { | ||
| const writer = recording as { | ||
| writeFailure?: unknown; |
There was a problem hiding this comment.
[Suggestion] R2-2: This predicate re-derives ChatRecordingService's own barrier admission check in another package by reading three TypeScript-private fields through an erasing recording: object cast, so nothing — compiler, core test, or CLI test — pins the coupling. writeFailure (chatRecordingService.ts:946), acceptingWrites (:940) and state (:926-931, the union 'inactive' | 'active' | 'closing' | 'closed' | 'integrity_failed', re-typed here as bare state?: string) are all private, and no public accessor exists: the only public state method on the class is hasWriteOwnership() (:1750), which reports lease ownership, not lifecycle or failure. This is the only production site in packages/cli reading them.
The failure mode is silent and it is exactly the bug this round set out to remove. A core rename, a narrowing of the state union, or a migration to ECMAScript #private fields — a pattern this codebase already uses (#hasActiveTurn, Session.ts:4025) — leaves the cast compiling and every read returning undefined. Then writer.writeFailure != null is false, writer.acceptingWrites === false is false (undefined !== false), and the state !== undefined guard makes the state arm false, so the predicate returns false for a genuinely closing recorder, the .catch rethrows, and every backward/latest transcript read during teardown, handoff, or against a not-yet-adopted ('inactive') recorder fails with -32023 session_writer_unavailable — behind a green tsc and a green suite. No test can catch that: the recorder double is a hand-built plain object carrying none of the three fields, with a pass-through barrier (acpAgent.test.ts:4468-4476), and both new tests fabricate the shape with Object.assign(recording, { acceptingWrites: false, state: 'closing' }) (:17184-17187) and { writeFailure, acceptingWrites: false, state: 'integrity_failed' } (:17232-17236). The suite pins three string field names, not the class — so the mutation you reported running ("dropping the lifecycle fallback goes red") does not cover a core-side rename.
There is a second, subtler exposure: the decision is sampled from mutable state after the rejection rather than from the rejection's reason, so it stays correct only while core keeps throwing writeFailure first (chatRecordingService.ts:1626-1629) — a pattern core already breaks elsewhere by wrapping: assertCanStartTurn throws new SessionWriterUnavailableError({ cause: this.writeFailure }) (:1667-1673). If the barrier ever adopts that wrap, a genuinely failed recorder is classified as a lifecycle miss and a transcript the recorder has declared untrustworthy is served as a success.
Witness:
Probe against a field-less recorder double modelling a post-rename / post-#private core.
(Declared: a MODEL of the migrated class, not a migrated core - the scratch tree links
core's built dist, so a core edit there would not be seen by a CLI probe.)
BASE : PROBE-R2-4 {"outcome":"served hasMore=false","readPageCalls":1}
PR : PROBE-R2-4 {"outcome":"refused {\"code\":-32023,\"data\":
{\"errorKind\":\"session_writer_unavailable\"}}","readPageCalls":0}
Facts verified at HEAD:
private state: 'inactive' | 'active' | 'closing' | 'closed' | 'integrity_failed'
(chatRecordingService.ts:926-931)
private acceptingWrites = false (:940)
private writeFailure: Error | undefined (:946)
grep isAcceptingWrites|getWriterState|writerHealth|isWriterAvailable
over packages/core/src (non-test) -> no matches
Put the decision where the state lives: a public predicate on ChatRecordingService beside hasWriteOwnership() — isWriterLifecycleUnavailable(): boolean { return this.writeFailure == null && (this.acceptingWrites === false || this.state !== 'active'); } — used by runWithWriteBarrier and appendRecordStrict as well, with acpAgent.ts calling recording.isWriterLifecycleUnavailable() and this local cast deleted, so a core rename becomes a compile error at the call site. If core must not be touched in this PR, the CLI-only alternative is to discriminate on the rejection's reason rather than on private state (have the barrier's lifecycle refusal carry a tag the latched-failure rethrow does not), or at minimum type the parameter with an exported interface the recorder implements so a rename is a compile error.
Note the gate this runs into: AGENTS.md's "Core Infrastructure Is Maintainer-Only" lists packages/core/src/** and packages/*/src/services/** as core modules facing a two-tier gate for external PRs, and this PR touches only packages/cli — so a core-side predicate needs maintainer buy-in, and a CLI-only fix must not re-widen the barrier, which round 1 rejected. Any relocated predicate must keep writeFailure winning over the lifecycle branch (runWithWriteBarrier rethrows this.writeFailure before its admission check, chatRecordingService.ts:1626-1629, and enterWriteFailure can latch a SessionWriterUnavailableError as that failure, :1293-1305), must keep returning lifecycle-unavailable for 'inactive' (lease adoption requires state === 'inactive', :1234-1248, so a live session whose lease has not been adopted yet must not start failing), and must not key on instanceof SessionWriterUnavailableError — per this diff's own comment at :759-763.
A CLI-only pin is available without crossing that gate: packages/cli/src/acp-integration/session/history-replayer.test.ts:968 already builds a real ChatRecordingService(configStub, undefined, false) inside packages/cli and drives recordToolResult + flush(). Add a case that returns a real recorder from getChatRecordingService(), drives beginClose() (and separately a latched write failure), and asserts the backward transcript read falls back to readPage in the first case and refuses with -32023 without calling readPage in the second. Renaming either field in core then turns one of those red, which the Object.assign-based tests cannot. If the predicate moves to core instead, add chatRecordingService.test.ts coverage asserting it is false while active, true after beginClose(), and false once a writeFailure has latched — that core test is what reds when the two copies drift.
中文说明
这个谓词在另一个 package 里重新推导了 ChatRecordingService 自己的 barrier 准入检查，方式是通过一个擦除类型的 recording: object 强转去读三个 TypeScript private 字段，因此没有任何东西 —— 编译器、core 测试或 CLI 测试 —— 固定住这层耦合。writeFailure（chatRecordingService.ts:946）、acceptingWrites（:940）与 state（:926-931，联合类型 'inactive' | 'active' | 'closing' | 'closed' | 'integrity_failed'，在这里被重新标注为裸 state?: string）都是私有的，而且不存在公开访问器：该类唯一的公开状态方法是 hasWriteOwnership()（:1750），它报告的是 lease 归属，而不是生命周期或失败。这是 packages/cli 中唯一读取它们的生产站点。
它的失效方式是静默的，而且恰好就是本轮要移除的那个 bug。core 侧的一次重命名、state 联合类型的收窄，或迁移到 ECMAScript #private 字段（本代码库已在使用的模式 —— #hasActiveTurn，Session.ts:4025），都会让这个强转继续通过编译，而每次读取都返回 undefined。于是 writer.writeFailure != null 为假、writer.acceptingWrites === false 为假（undefined !== false）、state !== undefined 这个守卫又让 state 分支为假，谓词对一个确实在 closing 的 recorder 返回 false，.catch 重抛，而在 teardown、handoff 期间，或面对一个尚未被 adopt（'inactive'）的 recorder 时，每一次 backward/最新 transcript 读取都会以 -32023 session_writer_unavailable 失败 —— 而 tsc 与测试套件全绿。没有测试能发现这一点：recorder 替身是一个手工构造的普通对象，三个字段一个都没有，barrier 是直通的（acpAgent.test.ts:4468-4476），而两个新测试用 Object.assign(recording, { acceptingWrites: false, state: 'closing' })（:17184-17187）和 { writeFailure, acceptingWrites: false, state: 'integrity_failed' }（:17232-17236）伪造出这个形状。套件固定的是三个字符串字段名，而不是那个类 —— 所以你报告跑过的那个变异（「去掉生命周期回退会变红」）并不覆盖 core 侧的重命名。
还有第二个更隐蔽的暴露面：这个判定是在拒绝之后从可变状态采样得到的，而不是从拒绝的原因得到的，所以它只在 core 继续先抛 writeFailure 时才正确（chatRecordingService.ts:1626-1629）—— 而 core 在别处已经用包装的方式打破了这一模式：assertCanStartTurn 抛的是 new SessionWriterUnavailableError({ cause: this.writeFailure })（:1667-1673）。如果 barrier 将来采用这种包装，一个真正失败的 recorder 就会被归类为生命周期缺失，于是一份 recorder 已宣告不可信的 transcript 会被当作成功返回。
证据： 见上方 PROBE-R2-4 输出（针对一个无字段 recorder 替身的实测，该替身是对重命名/#private 迁移后 core 类的模型，并非迁移后的 core —— scratch tree 链接的是 core 已构建的 dist，所以在其中改 core 不会被 CLI 探针看到），以及在 HEAD 处核实的字段声明与「无公开访问器」的 grep 结果。
修法： 把这个判定放到状态所在之处 —— 在 ChatRecordingService 上、hasWriteOwnership() 旁边加一个公开谓词：isWriterLifecycleUnavailable(): boolean { return this.writeFailure == null && (this.acceptingWrites === false || this.state !== 'active'); }，让 runWithWriteBarrier 与 appendRecordStrict 也使用它，acpAgent.ts 改为调用 recording.isWriterLifecycleUnavailable() 并删除本地强转，这样 core 侧重命名就会在调用点变成编译错误。如果本 PR 不能改 core，纯 CLI 的替代方案是根据拒绝的原因而不是私有状态来判别（让 barrier 的生命周期拒绝携带一个「锁定失败重抛」所没有的标记），或者至少把参数类型写成 recorder 实现的一个导出接口，使重命名成为编译错误。
约束： 注意这会撞上一道关卡 —— AGENTS.md 的「Core Infrastructure Is Maintainer-Only」把 packages/core/src/** 与 packages/*/src/services/** 列为核心模块，外部 PR 触碰它们要过两级门槛，而本 PR 只改了 packages/cli，所以 core 侧谓词需要维护者认可，而纯 CLI 的修法不得重新扩大 barrier 范围（第一轮已否决）。任何被搬移的谓词都必须保持 writeFailure 优先于生命周期分支（runWithWriteBarrier 在准入检查之前重抛 this.writeFailure，chatRecordingService.ts:1626-1629，而 enterWriteFailure 可能把一个 SessionWriterUnavailableError 锁定为该 failure，:1293-1305），必须对 'inactive' 继续返回「生命周期不可用」（lease adoption 要求 state === 'inactive'，:1234-1248，所以一个 lease 尚未被 adopt 的存活会话不能开始失败），并且不得以 instanceof SessionWriterUnavailableError 为依据 —— 这正是本 diff 自己在 :759-763 的注释所要求的。
验收标准： 不跨越那道关卡也有纯 CLI 的固定办法：packages/cli/src/acp-integration/session/history-replayer.test.ts:968 已经在 packages/cli 内部构造了一个真实的 ChatRecordingService(configStub, undefined, false) 并驱动 recordToolResult + flush()。请新增一个用例，让 getChatRecordingService() 返回一个真实 recorder，驱动 beginClose()（以及 separately 一个锁定的写入失败），断言 backward transcript 读取在第一种情况下回退到 readPage、在第二种情况下以 -32023 拒绝且不调用 readPage。这样 core 中任一字段被重命名都会让其中一个变红，而基于 Object.assign 的测试做不到。如果谓词改为搬进 core，则请在 chatRecordingService.test.ts 中补一个用例，断言它在 active 时为 false、beginClose() 之后为 true、writeFailure 锁定之后为 false —— 当两份副本发生漂移时，变红的正是那个 core 测试。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| ? await recording | ||
| .runWithWriteBarrier(readPersistedPage) | ||
| .catch((error: unknown) => { |
There was a problem hiding this comment.
[Suggestion] R2-3: This .catch is attached to the whole runWithWriteBarrier(readPersistedPage) promise, so it also catches a rejection thrown by the read itself, and it classifies from recorder state sampled after the failure rather than from the rejection's identity. runWithWriteBarrier runs the operation inside its queued callback (const result = await operation();, chatRecordingService.ts:1642) and rethrows a non-writer error unchanged (:1655-1657), so a reader error reaches this handler; if beginClose() landed while the read was in flight, the predicate now returns true and the whole page read is executed a second time outside the barrier.
Concretely: a direction:'backward' request enters the barrier while the recorder is active; teardown flips acceptingWrites = false / state = 'closing' during the read (close() calls beginClose() before closeOnce() awaits the tail, chatRecordingService.ts:1676-1712, and nothing serializes beginClose() behind an in-flight barriered read); readPage then rejects for its own reason — SessionTranscriptPageTooLargeError / SessionTranscriptTooLargeError / InvalidSessionTranscriptCursorError (plain Error subclasses, session-transcript-reader.ts:102-138, :1952) or a transient fs error such as EMFILE or EIO. The handler re-runs readPersistedPage(): a second SessionTranscriptReader, a second fsp.stat, and — because a buildIndex failure deletes the pending cache entry (session-transcript-reader.ts:2300-2308) — a second O(file) index rebuild over a page capped at SESSION_TRANSCRIPT_MAX_PAGE_BYTES (4 MiB). For a transient first error the retry can succeed, so the client silently receives a page from a retry path the barrier never sanctioned and the original error is never reported; for a deterministic one the second rejection surfaces, masking the first. Neither attempt is logged, so an incident report of "transcript intermittently fails or comes back short during shutdown" has no trace distinguishing the barriered read from the off-barrier retry.
Witness:
Probe using the DEFAULT barrier mock, which invokes the operation exactly as the real
one does; readPage flips the recorder to acceptingWrites:false / state:'closing'
before rejecting on its first call.
BASE : PROBE-R2-5 {"readPageCalls":1,"outcome":"rejected {\"message\":\"EMFILE: too many open files\"}"}
PR : PROBE-R2-5 {"readPageCalls":2,"outcome":"resolved hasMore=false"}
fix-flip (let readAttempted = false; set first in readPersistedPage;
if (readAttempted || !isWriterLifecycleUnavailable(recording)) throw error;):
PR+fix: PROBE-R2-5 {"readPageCalls":1,"outcome":"rejected EMFILE"} <- matches BASE
Tests 7 passed | 624 skipped - including both round-2 fallback tests, still green
This settles a disagreement inside the review: two lenses cleared this on the reasoning that an unrelated operation error rethrows "with the recorder still active", and the probe shows the recorder can flip in between.
let readAttempted = false;
const readPersistedPage = async () => {
readAttempted = true;
const reader = new SessionTranscriptReader(cwd);
return await reader.readPage(sessionId, { /* unchanged */ });
};
// ...
.catch((error: unknown) => {
if (readAttempted || !isWriterLifecycleUnavailable(recording)) {
throw error;
}
return readPersistedPage();
})Do not replace the flag with an error-type check: your own comment at acpAgent.ts:759-763 forbids keying on instanceof SessionWriterUnavailableError, because the barrier rethrows writeFailure first and that failure can itself be that class. A debugLogger.debug line naming the sessionId, the writer state and the original error when the fallback is taken would also make this path diagnosable, which it currently is not.
The discriminator is sound because the barrier refuses before invoking the operation — if (this.writeFailure) throw this.writeFailure; then if (!this.acceptingWrites || this.state !== 'active') { throw new SessionWriterUnavailableError(); } (chatRecordingService.ts:1627-1630) — so on a genuine lifecycle refusal there is no read to retry. The guard must not let the fallback run the read when a writeFailure is latched: your existing test asserts expect(recording.runWithWriteBarrier).toHaveBeenCalledOnce(); expect(readPage).not.toHaveBeenCalled(); and pins code: -32023 with data.errorKind: 'session_writer_unavailable'.
Please add a case in acpAgent.test.ts beside falls back to a direct latest transcript page when the recorder is only lifecycle-unavailable: recorder hand-set lifecycle-unavailable, runWithWriteBarrier mocked to invoke the operation (the pattern already used at :17066), and readPage rejecting with a plain reader error; assert expect(readPage).toHaveBeenCalledOnce() and that the request rejects. It is red today (readPage is called twice) and green with the guard — so removing the readAttempted check turns it red. Note the existing fallback test cannot catch this: it asserts expect(readPage).toHaveBeenCalledWith(...), which a double call still satisfies.
中文说明
这个 .catch 挂在整个 runWithWriteBarrier(readPersistedPage) promise 上，因此它同样会捕获读取自身抛出的拒绝；而且它是根据失败之后采样到的 recorder 状态来分类的，而不是根据拒绝本身的身份。runWithWriteBarrier 在其排队回调内部运行该操作（const result = await operation();，chatRecordingService.ts:1642），并且会原样重抛非 writer 类错误（:1655-1657），所以一个 reader 错误会到达这个 handler；如果 beginClose() 在读取进行中落地，谓词此时返回 true，于是整个页面读取会在 barrier 之外被再执行一次。
具体来说：一个 direction:'backward' 请求在 recorder 还是 active 时进入 barrier；teardown 在读取期间把 acceptingWrites 置为 false、state 置为 'closing'（close() 先调用 beginClose()，之后 closeOnce() 才 await 尾部，chatRecordingService.ts:1676-1712，而且没有任何机制把 beginClose() 串行化到一个进行中的 barrier 读取之后）；随后 readPage 因自身原因拒绝 —— SessionTranscriptPageTooLargeError / SessionTranscriptTooLargeError / InvalidSessionTranscriptCursorError（都是普通 Error 子类，session-transcript-reader.ts:102-138、:1952），或 EMFILE、EIO 这类瞬时 fs 错误。handler 于是重跑 readPersistedPage()：第二个 SessionTranscriptReader、第二次 fsp.stat，并且 —— 因为 buildIndex 失败会删除待定的缓存条目（session-transcript-reader.ts:2300-2308）—— 第二次对一个上限为 SESSION_TRANSCRIPT_MAX_PAGE_BYTES（4 MiB）的页面做 O(文件) 的索引重建。对瞬时的首个错误，重试可能成功，于是客户端会静默地拿到一个来自「barrier 从未批准的重试路径」的页面，而原始错误永不被上报；对确定性错误，第二次拒绝会浮现，掩盖第一次。两次尝试都没有日志，所以一份「shutdown 期间 transcript 偶发失败或偶发变短」的事故报告，没有任何痕迹能区分 barrier 内读取与 barrier 外重试。
证据： 见上方 PROBE-R2-5 输出。这解决了审查内部的一处分歧：有两个视角基于「无关的操作错误会在 recorder 仍为 active 时重抛」而排除了这一点，而探针表明 recorder 可以在这中间翻转。
修法： 见上方代码块 —— 让判别条件变成「barrier 在运行操作之前就拒绝了」，这才是这个回退存在的目的。
约束： 不要用错误类型检查替换这个标志：你自己在 acpAgent.ts:759-763 的注释禁止以 instanceof SessionWriterUnavailableError 为依据，因为 barrier 会先重抛 writeFailure，而那个 failure 本身可能就是该类。这个判别条件之所以成立，是因为 barrier 在调用操作之前就拒绝 —— if (this.writeFailure) throw this.writeFailure; 然后 if (!this.acceptingWrites || this.state !== 'active') { throw new SessionWriterUnavailableError(); }（chatRecordingService.ts:1627-1630）—— 所以在真正的生命周期拒绝时并没有一次读取需要重试。该守卫也不得在 writeFailure 已锁定时让回退去执行读取：你现有的测试断言 expect(recording.runWithWriteBarrier).toHaveBeenCalledOnce(); expect(readPage).not.toHaveBeenCalled();，并固定了 code: -32023 与 data.errorKind: 'session_writer_unavailable'。另外，在走回退时加一行 debugLogger.debug，写明 sessionId、writer 状态与原始错误，会让这条路径变得可诊断 —— 目前它不可诊断。
验收标准： 请在 acpAgent.test.ts 中、falls back to a direct latest transcript page when the recorder is only lifecycle-unavailable 旁边新增一个用例：把 recorder 手工置为生命周期不可用，把 runWithWriteBarrier mock 成调用该操作（:17066 已有这种写法），让 readPage 以一个普通 reader 错误拒绝；断言 expect(readPage).toHaveBeenCalledOnce() 且请求被拒绝。它今天是红的（readPage 被调用两次），加上守卫后变绿 —— 所以移除 readAttempted 检查会让它变红。注意现有的回退测试抓不到这一点：它断言的是 expect(readPage).toHaveBeenCalledWith(...)，而调用两次同样满足。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| // Barrier the request's backward/latest page (#9704). The | ||
| // request direction, not the resolved page direction, is the | ||
| // gate. Cursor/anchor pages never consulted writer health. |
There was a problem hiding this comment.
[Suggestion] R2-4: This comment, the PR title and "What this PR does" all credit the write barrier with the #9704 fix, but the barrier cannot affect the timeline that issue reports: the scheduler records tool results only after the tool batch completes, so during the execution window there is nothing queued on operationTail for the barrier to wait out. The mechanism that actually changes a replay outcome is the finalizeDangling / isTurnIdle() sampling switch just below it.
The issue's own evidence is a 104-second window: run_shell_command issued 16:45:53, POST /session/d30b943a/load answered 200 at 16:46:01 with "the function call was in the JSONL file but the tool_result record was not", and the tool_result finally written at 16:47:37. recordToolResults (packages/core/src/core/coreToolScheduler.ts:6277, reached via :6402-6427) is the scheduler's only recorder call site and sits after the batch's post-hooks, so at 16:46:01 operationTail held no tool-result write and runWithWriteBarrier resolved immediately — reading byte-for-byte what base's await ...flush() read. The issue thread's own triage reached the same conclusion independently: "The write-delay hypothesis is not supported by the code ... the scheduler only calls recordToolResults after the tools complete ... 'Flush before answering load' cannot fix this: during the execution window there is nothing to flush."
The cost is concrete rather than cosmetic. Everything this PR carries to serve that inert mechanism — isWriterLifecycleUnavailable (:759-779), the fallback branch, two of the new tests, and the residual 503 accepted under Risk & Scope — is the same surface that produced round 1's Critical and both of this round's confirmed defects on that branch. And a maintainer reading Fixes #9704 next to this comment attributes the fix to the wrong mechanism and closes the issue on that basis, while the reader-side gap the issue's triage named as the real root cause ("The replay machine has no concept of 'in-flight'") stays open, as do the cold-restore shapes it listed ("cross-runtime restore, post-restart load against an orphaned writer, entry reaped mid-turn") — those read through the ?? true defaults with no live Session, which base handled identically, so nothing in this diff changes them.
To be clear about what the barrier does buy, so the re-attribution is not a demotion: it covers the enqueue-to-drain window — a completed tool whose append is queued but not yet persisted — and the sibling sampling change genuinely extends coverage to the autonomous goal/cron/notification turns that base's activePromptCalls could not see. That second part is the fix.
Witness:
witness: not run - the nearest capability was an ab-drive pair against a real daemon
plus child running a >100 s tool, the only shape that reproduces #9704's timeline.
The deciding fact is static and does not need it:
coreToolScheduler.ts:6277 this.recordToolResults(completedCalls);
<- sits after the batch's post-hooks and
applyBatchOutputBudget
coreToolScheduler.ts:6402-6427 recordToolResults -> chatRecordingService.recordToolResult
<- the scheduler's ONLY recorder call site
So nothing is enqueued on operationTail during tool execution. The same probe harness
that settled R1-1 measures queued writes and there are none in that window; its BASE arm
(flushCalls:1 draining a queued record) is the positive control showing the harness can
see a queued write when one exists. The R1-1 A/B also bounds what the barrier adds over
base: base's flush() already provided the drain, so the barrier's only additions are the
lease fence and the new refusal modes.
Re-attribute the fix. State here and in the description that the barrier covers only the enqueue-to-drain window, and that the #9704 symptom is addressed by the finalizeDanglingForRestore / isTurnIdle() sampling. Then either narrow the Fixes #9704 claim to what the change actually closes and name the residual shapes it leaves (the cold-restore reads where no live Session answers), or keep the issue open for them. The alternative, if the drain is all this read needs, is to keep flush() on the backward path and drop runWithWriteBarrier plus isWriterLifecycleUnavailable — which would also retire the entire refusal class rather than patching it with a fallback; R1-1's A/B shows base's flush() already provided the ordering this read needs, so that is a real option rather than a regression, and it is worth weighing before the fix round widens this surface further.
Do not simply default the non-live case to false if you take the claim-narrowing route: the helper's contract requires finalizing genuinely dead runs — "so isTurnIdle() there is structurally false and would keep genuinely abandoned calls pending forever" (acpAgent.ts:4649-4652) — and the placeholder text is the correct terminal state for a run that really ended (packages/acp-bridge/src/transcript-replay.ts:32-34).
For the claim-and-description correction there is no guard or branch to pin, so no test is owed. If you take the keep-flush() variant instead, the pin that flips is reads the live transcript page through the owner write barrier in acpAgent.test.ts, which asserts recording?.runWithWriteBarrier was called once and recording?.flush was not; note also that serializes live transcript reads behind in-flight tool-result writes is the only test pinning the barrier's ordering and it uses an empty page (records: []), so no test today ties the barrier to the issue's dangling-call shape.
中文说明
这段注释、PR 标题以及「What this PR does」都把 #9704 的修复归功于写入 barrier，但 barrier 无法影响该 issue 所报告的时间线：调度器只在 tool 批次完成之后才记录 tool result，所以在执行窗口期间 operationTail 上根本没有任何排队的写入可供 barrier 等待。真正能改变 replay 结果的机制，是紧接其下的 finalizeDangling / isTurnIdle() 采样切换。
该 issue 自己的证据是一个 104 秒的窗口：16:45:53 发起 run_shell_command，16:46:01 POST /session/d30b943a/load 返回 200，此时「function call 已在 JSONL 文件中，但 tool_result 记录还没有」，而 tool_result 直到 16:47:37 才写入。recordToolResults（packages/core/src/core/coreToolScheduler.ts:6277，经 :6402-6427 到达）是调度器唯一的 recorder 调用点，且位于批次的 post-hooks 之后，所以在 16:46:01 时 operationTail 上没有任何 tool-result 写入，runWithWriteBarrier 会立即 resolve —— 读到的内容与 base 的 await ...flush() 逐字节相同。issue 线程里的 triage 也独立得出了同样结论：「写入延迟假说得不到代码支持……调度器只在 tools 完成之后才调用 recordToolResults……『在应答 load 之前 flush』修不了这个：在执行窗口期间没有东西可 flush。」
这个代价是具体的，而不是措辞问题。本 PR 为服务这个不起作用的机制所背负的一切 —— isWriterLifecycleUnavailable（:759-779）、回退分支、两个新测试，以及 Risk & Scope 中接受的残余 503 —— 正是产生第一轮 Critical 以及本轮该分支上两个已确认缺陷的那同一个面。而一位维护者在这段注释旁读到 Fixes #9704，就会把修复归因于错误的机制，并据此关闭 issue；与此同时，issue triage 指认为真正根因的读取侧缺口（「replay 机器没有 'in-flight' 的概念」）仍然敞开，它列出的冷恢复形状（「跨 runtime 恢复、重启后对孤儿 writer 的加载、条目在 turn 进行中被回收」）也一样 —— 那些读取会经由 ?? true 默认值、在没有 live Session 的情况下进行，而 base 的处理完全相同，所以本 diff 没有任何东西改变它们。
为了避免这次重新归因被读成贬低，需要说清楚 barrier 确实买到了什么：它覆盖的是「入队到 drain」的窗口 —— 一个已完成的 tool，其 append 已排队但尚未落盘 —— 而与之相邻的采样改动确实把覆盖扩展到了 base 的 activePromptCalls 看不见的自主 goal/cron/notification turn。后面这一部分才是修复。
证据： 未运行 —— 最接近的手段是针对一个真实 daemon 加子进程、运行一个 >100 秒 tool 的 ab-drive 对照组，那是唯一能复现 #9704 时间线的形状。但决定性事实是静态的，不需要它：coreToolScheduler.ts:6277 的 this.recordToolResults(completedCalls); 位于批次 post-hooks 与 applyBatchOutputBudget 之后，而 recordToolResults（:6402-6427 → chatRecordingService.recordToolResult）是调度器唯一的 recorder 调用点，所以 tool 执行期间没有任何东西入队到 operationTail。确定 R1-1 的同一套探针装置测量的正是排队写入，而该窗口内一个也没有；它的 BASE 臂（flushCalls:1，drain 了一条排队记录）是阳性对照，说明当确实存在排队写入时该装置能看见。R1-1 的 A/B 也界定了 barrier 相对 base 增加了什么：base 的 flush() 已经提供了 drain，所以 barrier 唯一新增的是 lease fence 与新的拒绝模式。
修法： 重新归因这个修复。在此处以及描述中说明 barrier 只覆盖「入队到 drain」的窗口，而 #9704 的症状是由 finalizeDanglingForRestore / isTurnIdle() 采样解决的。然后要么把 Fixes #9704 的声明收窄到该改动真正关闭的范围、并点名它留下的残余形状（没有 live Session 应答的冷恢复读取），要么为该 issue 保持敞开。另一个选择是：如果这条读取需要的只是 drain，那就在 backward 路径上保留 flush()，去掉 runWithWriteBarrier 与 isWriterLifecycleUnavailable —— 那样还会让整个拒绝类别消失，而不是用回退去打补丁；R1-1 的 A/B 表明 base 的 flush() 已经提供了这条读取所需的顺序，所以那是一个真实可选项而非回归，值得在修复轮进一步扩大这个面之前权衡。
约束： 如果你选择收窄声明这条路线，不要把非 live 的情况简单默认为 false：helper 的契约要求对真正死掉的 run 执行 finalize ——「所以那里 isTurnIdle() 在结构上为 false，会让真正被放弃的调用永远挂起」（acpAgent.ts:4649-4652）—— 而对于一个确实已结束的 run，占位文本就是正确的终态（packages/acp-bridge/src/transcript-replay.ts:32-34）。
验收标准： 对「修正声明与描述」这一项，没有需要固定的守卫或分支，因此不欠测试。如果你改走保留 flush() 的方案，会翻转的固定点是 acpAgent.test.ts 中的 reads the live transcript page through the owner write barrier，它断言 recording?.runWithWriteBarrier 被调用一次且 recording?.flush 未被调用；另请注意 serializes live transcript reads behind in-flight tool-result writes 是唯一固定 barrier 顺序的测试，而它用的是空页面（records: []），所以今天没有任何测试把 barrier 与该 issue 的 dangling-call 形状联系起来。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| releaseBarrier(); | ||
| await transcriptPromise; | ||
| expect(readPage).toHaveBeenCalledOnce(); |
There was a problem hiding this comment.
[Suggestion] R2-5: This test — serializes live transcript reads behind in-flight tool-result writes, the only one pinning the barriered read — pins that the barrier is entered before the read starts, but not that the read is the barrier's operation. A fence-then-read rewrite of the route survives the entire test file, so the suite cannot distinguish the shipped shape from one in which core's post-operation lease assertion no longer fences the read.
That distinction is load-bearing for two live items on this PR. The fence exists only while the read is the operation — const result = await operation(); await lease?.assertOwnedAndUnchanged(); return result; (chatRecordingService.ts:1640-1642) — so under the mutant a lease loss during the read is neither detected nor latched. And the still-open round-1 Suggestion about the read sitting inside the write serialization point asks you to consider moving it out; whichever way that goes, nothing today tells you the fence silently appeared or disappeared.
To be precise about what this test does pin, because it is a genuine pin and not a tautology: bypassing the barrier entirely (readPersistedPage() instead of runWithWriteBarrier(readPersistedPage)) never resolves the barrierEntered gate, so four tests red — including a 60 s timeout on this one. The gap is narrower than "the test is vacuous": it is silent on exactly one property, the one the two items above turn on.
Witness:
Six vitest runs of packages/cli/src/acp-integration/acpAgent.test.ts
in a scratch tree at 0d485501c9:
SHIPPED (baseline) -> Tests 627 passed (627)
FENCE-THEN-READ MUTANT, -t filtered -> Tests 1 passed | 626 skipped (627)
<- SURVIVES the named test
FENCE-THEN-READ MUTANT, full file -> Tests 627 passed (627)
<- SURVIVES all four barrier tests
MUTANT + proposed fix assertion -> x serializes live transcript reads
behind in-flight tool-result writes
AssertionError: expected undefined to
deeply equal ObjectContaining{...}
SHIPPED + proposed fix assertion -> Tests 627 passed (627) <- fix is safe
BYPASS MUTANT (positive control, full file) -> Tests 4 failed | 623 passed (627),
incl. "Test timed out in 60000ms."
The mutant applied to acpAgent.ts:9080-9090, .catch fallback semantics preserved:
? await recording
.runWithWriteBarrier(async () => undefined)
.then(() => readPersistedPage())
.catch((error: unknown) => { ...unchanged... })
baseline: new-surface - `git show 1b604721b0:...acpAgent.test.ts | grep -c 'serializes
live transcript reads behind in-flight tool-result writes'` -> 0; its predecessor
'flushes the live recording before reading the latest persisted page' -> 1.
All four barrier tests this diff adds (:17010, :17055, :17168, :17211) survive the mutant, because each asserts only barrier entry, call count, readPage arguments, or the refusal mapping — all of which the fence-then-read shape also satisfies.
| releaseBarrier(); | |
| await transcriptPromise; | |
| expect(readPage).toHaveBeenCalledOnce(); | |
| releaseBarrier(); | |
| await transcriptPromise; | |
| expect(readPage).toHaveBeenCalledOnce(); | |
| const barrierResult = await recording.runWithWriteBarrier.mock.results[0]! | |
| .value; | |
| expect(barrierResult).toEqual( | |
| expect.objectContaining({ sessionId: VALID_SESSION_ID }), | |
| ); |
Under the mutant the barrier resolves undefined and this reds; under the shipped code it resolves the page, so the assertion is safe to add as-is (measured both directions above). mock.results[0] is the transcript read's own call — the green shipped run confirms no earlier barrier call from session setup is captured.
The barrier resolves with the operation's own value and only after the post-operation lease assertion (chatRecordingService.ts:1640-1642), so the witness must assert on that resolved value — call-count assertions cannot discriminate — and must not assume the fence runs before the read.
Please confirm the addition discriminates by mutation: apply the fence-then-read shape above and check this test reds, then revert it and check the suite is green again. Consider also renaming the test to what it pins — that the read runs as the barrier's operation — rather than to tool-result writes it never models (no write is ever in flight in the mock's queue).
中文说明
这个测试 —— serializes live transcript reads behind in-flight tool-result writes，也是唯一固定「加 barrier 的读取」的那个 —— 固定的是「读取开始之前已进入 barrier」，而不是「读取就是 barrier 的操作」。把路由改写成「先 fence 再读取」的形式，整个测试文件都会照常通过，所以套件无法区分实际发布的形状与「core 的操作后 lease 断言不再为该读取做 fence」的形状。
这个区别对本 PR 上两个仍然存活的事项是承重的。fence 只在读取就是该操作时才存在 —— const result = await operation(); await lease?.assertOwnedAndUnchanged(); return result;（chatRecordingService.ts:1640-1642）—— 所以在该变异下，读取期间发生的 lease 丢失既不会被检测到，也不会被锁定。而第一轮那条关于「读取位于写入串行点内部」、目前仍敞开的 Suggestion，正是要你考虑把它移出来；无论朝哪个方向走，今天都没有任何东西会告诉你 fence 悄悄出现或消失了。
为了准确说明这个测试确实固定了什么（它是一个真实的 pin，不是同义反复）：完全绕过 barrier（用 readPersistedPage() 而不是 runWithWriteBarrier(readPersistedPage)）会让 barrierEntered 这个 gate 永不 resolve，于是四个测试变红 —— 包括这个测试的 60 秒超时。所以缺口比「测试是空的」要窄：它只在一个性质上保持沉默，而上面两项正好取决于那一个性质。
证据： 见上方六次 vitest 运行输出（scratch tree，0d485501c9）。「先 fence 再读取」的变异在 -t 过滤下与整文件运行下都存活；加上建议的断言后该变异变红（AssertionError: expected undefined to deeply equal ObjectContaining{...}），而在未修改的实现上加同一断言 627/627 全绿，说明该断言可安全直接加入；作为阳性对照，「绕过 barrier」的变异会让 4 个测试失败（含 60000ms 超时）。baseline: new-surface 由 merge base 确定：该测试标题在 1b604721b0 处 grep 计数为 0，其前身 flushes the live recording before reading the latest persisted page 为 1。
本 diff 新增的四个 barrier 测试（:17010、:17055、:17168、:17211）全部在该变异下存活，因为它们各自只断言 barrier 进入、调用次数、readPage 参数，或拒绝的映射 —— 而「先 fence 再读取」的形状同样满足这些。
修法： 见上方 suggestion 块 —— 增加一个把页面与 barrier 自身 resolve 值绑定起来的断言。在变异下 barrier resolve 出 undefined，该断言变红；在实际代码下它 resolve 出页面。
约束： barrier 是以操作自身的值 resolve 的，而且是在操作后的 lease 断言之后（chatRecordingService.ts:1640-1642），所以这个见证必须断言那个 resolved 值 —— 调用次数类断言无法区分 —— 并且不得假定 fence 在读取之前运行。
验收标准： 请用变异确认这个新增断言具备判别力：应用上方「先 fence 再读取」的形状，确认该测试变红；然后还原，确认套件重新全绿。也可以考虑把测试名改成它真正固定的性质 —— 读取作为 barrier 的操作运行 —— 而不是改成它从未建模的 tool-result 写入（mock 的队列中从未有写入在进行）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Latest/backward reads still go through the write barrier. When the recorder is only lifecycle-unavailable, flush the queued tail (catching a latched writeFailure so that path stays best-effort) before reading the persisted page. A locked writeFailure still fails the read. Ungated sessionTranscript now samples hasActiveTurn rather than isTurnIdle so a closing session with no active turn finalizes dangling calls. Read-operation failures no longer retry as a lifecycle fallback.
|
Addressed Round 2 Critical R1-1 on Lifecycle fallback now drains first: Also in this pass:
Risk & Scope updated. Fork CI is still |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- X3-1 lifecycle-fallback drain failure swallowed, with no test pinning either semantics - not posted: location overlap with comment 3944179483 (acpAgent.ts:9117)
- R2-4 comment/title/description still credit the write barrier with the issue-9704 fix - still stands, already reported (comment 3944179491)
- R1-2 barrier gate keyed on the request direction rather than the resolved page direction - still stands, already reported (comment 3943316415)
- R1-4 the barrier joins the write serialization point rather than merely awaiting it - still stands, already reported (comment 3943316417)
Not explored to full depth (tool budget reached): "agent 1b": did not read the daemon/web-shell consumer of the transcript route's events payload to confirm how a terminal success:false update for a callId that later…; "agent 1b": did not read chatRecordingService.ts:1225-1250 in full, so the reachability of the lease-required-but-unbound-while-active state in Finding 2 is unverified..
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.ts:776 — [review] the state !== 'active' disjunct in isWriterLifecycleUnavailable is inert and unpinnable; its only reachable future effect widens the un-barriered fallback
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 6 (6 new). Findings keep coming back to the same files: packages/cli/src/acp-integration/acpAgent.ts (findings in rounds 1, 2; 1 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. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678, carried as unresolved by reviews 5124611584 and 5125551684) - STILL STANDS at the reviewed commit. Re-verified this round against the actions runs API for head 37ebf3c: Qwen Code CI, SDK Java and tui-parity are all action_required, i.e. the three pull_request-triggered workflows have never started on this commit and produce no check runs at all. The check runs that do exist are bot orchestration and precheck plumbing (label, publish-resolution, ack-review-request, resolve-pr, delay-automatic-review, authorize, review-config, precheck-pr, Remind on force-push, assign, review-pr) - none compiles the change or runs a test, and no lint signal exists on the commit in any form. Note that a check-run-based classifier scores this commit all_pass on the four checks it can see, which is exactly why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list, which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing, and the author confirms in the thread that they are still pending. For completeness, this review produced its own evidence on the commit - the build is green across all 17 affected workspaces, packages/cli tests are 1013 passed / 1 failed file of 1014 with the single failing file (src/config/settings.test.ts) also failing on the merge base (measured netNew: [], shared: ["src/config/settings.test.ts"], so pre-existing by measurement and not attributable to this diff), and the two changed suites pass 1461/1461 in isolation - but that is this review's local run, not CI on the commit, and it does not include lint.
中文说明
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 1b"：did not read the daemon/web-shell consumer of the transcript route's events payload to confirm how a terminal success:false update for a callId that later…；"agent 1b"：did not read chatRecordingService.ts:1225-1250 in full, so the reachability of the lease-required-but-unbound-while-active state in Finding 2 is unverified.。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 6 条（其中 6 条首次提出）。发现反复回到同一批文件：packages/cli/src/acp-integration/acpAgent.ts（第 1、2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678, carried as unresolved by reviews 5124611584 and 5125551684) - STILL STANDS at the reviewed commit. Re-verified this round against the actions runs API for head 37ebf3c: Qwen Code CI, SDK Java and tui-parity are all action_required, i.e. the three pull_request-triggered workflows have never started on this commit and produce no check runs at all. The check runs that do exist are bot orchestration and precheck plumbing (label, publish-resolution, ack-review-request, resolve-pr, delay-automatic-review, authorize, review-config, precheck-pr, Remind on force-push, assign, review-pr) - none compiles the change or runs a test, and no lint signal exists on the commit in any form. Note that a check-run-based classifier scores this commit all_pass on the four checks it can see, which is exactly why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list, which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing, and the author confirms in the thread that they are still pending. For completeness, this review produced its own evidence on the commit - the build is green across all 17 affected workspaces, packages/cli tests are 1013 passed / 1 failed file of 1014 with the single failing file (src/config/settings.test.ts) also failing on the merge base (measured netNew: [], shared: ["src/config/settings.test.ts"], so pre-existing by measurement and not attributable to this diff), and the two changed suites pass 1461/1461 in isolation - but that is this review's local run, not CI on the commit, and it does not include lint.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| ?.getConfig() | ||
| .getChatRecordingService(); | ||
| const turnIdleBeforeRead = liveSession | ||
| ? !liveSession.hasActiveTurn() |
There was a problem hiding this comment.
[Critical] R3-1: [certifies-falsely] [regression] The finalizeDangling decision on this route now samples only Session.hasActiveTurn(), which cannot see a prompt call that is already registered but still waiting at Session admission — so a backward transcript read landing in that window terminally certifies a dangling tool call as failed, and the prompt then resumes or re-hangs that very same call.
A transcript that ends on a dangling tool call is the normal input for a continuation, not an exotic one: Session.ts:10818-10830 deliberately leaves a restored ask_user_question whose permission wait timed out "dangling on disk so a later load can re-hang it", and continueLastTurn()'s interrupted_turn path classifies exactly those (Session.ts:4917-4932). When the daemon dispatches that prompt, the agent registers it in activePromptCalls (acpAgent.ts:6180-6186) and awaits Session.prompt(), which blocks in assertCanStartTurn() (Session.ts:4582 — itself joining the recorder's operationTail), then assertManagedConversationBindingReady, then #syncLiveToolDeclarations() → await llmClient.setTools(), a network round trip on Live sessions, and installs pendingPrompt only at Session.ts:4644. Across that whole window #hasActiveTurn() is false. A concurrent qwen/status/session/transcript read with direction: 'backward' therefore samples turnIdleBeforeRead === true and idleAtReplay === true, so finalizeDangling is true; replayTranscriptRecordPage has no skipFinalizeCallIds parameter at all (history-replay-page.ts:364-377) and its gate is unconditionally true for a backward page, so history-replayer.ts:170-182 emits a terminal success: false carrying MISSING_TRANSCRIPT_TOOL_RESULT_MESSAGE for that callId. The prompt is then admitted and the live stream delivers the real or synthesized result for the same callId — the client is told the call failed, then watches it run.
This is reachable without the route being gated. The bridge fires the restore prompt fire-and-forget on attach (bridge.ts:7596-7651, call sites :8478/:8508/:8650/:8679) while GET /session/:id/transcript forwards direction (serve/routes/session.ts:4775-4880) and returns the child's page verbatim with no prompt gating — your own new comment at acpAgent.ts:4653-4656 records that this route "is also ungated". The window is widest exactly where a reader is most likely: on a Live or managed-binding session the awaits above are milliseconds to seconds, and in lease mode the shared operationTail structurally prolongs it, because a read whose barrier operation queues ahead of the prompt's assertCanStartTurn completes while the prompt still has several awaits before installing pendingPrompt. Base suppressed all of it: at the merge base this read computed !activePromptBeforeRead && !this.activePromptCalls.has(sessionId), false for the whole registered-call window, and base had a test pinning exactly that shape, which this PR rewrote.
Witness:
Probe in a scratch tree at HEAD, reconstructing the deleted base-test shape: agent.prompt()
registered and blocked inside Session.prompt() (the admission window), hasActiveTurn() false,
concurrent ext-method read with direction:'backward'. The split is deterministic by
construction - the probe holds the prompt blocked at admission, so it is not a flaky window.
HEAD {"finalizeDangling":true, "promptInFlightDuringRead":true,
"promptSettledAtSample":false, "hasActiveTurn":false}
HEAD + fix below {"finalizeDangling":false,"promptInFlightDuringRead":true,
"promptSettledAtSample":false, "hasActiveTurn":false}
Fix-constraint pins held with the fix applied:
-t "dangling" -> 8 passed | 623 skipped (631), including
'finalizes dangling transcript calls when the session is closing with no active turn'
Independent corroboration - the deleted base oracle restored verbatim at HEAD:
x AssertionError: expected last "spy" call to have been called with ObjectContaining{...}
- "finalizeDangling": false
+ "finalizeDangling": true
(1 failed | 630 skipped)
The fix spans two locations, so it is not a one-click suggestion. Sample the agent-level signal before the read, and AND it into the decision:
const promptCallBeforeRead = this.activePromptCalls.has(sessionId);
const turnIdleBeforeRead = liveSession
? !liveSession.hasActiveTurn()
: true;
// ...
finalizeDangling:
!promptCallBeforeRead &&
!this.activePromptCalls.has(sessionId) &&
this.finalizeDanglingForRestore(liveSession, turnIdleBeforeRead, {
ignoreClosing: true,
}),The signal must be ANDed in, never substituted for the ignoreClosing sample: the closing path has to keep finalizing, which Session.ts:3928 (return !this.closing && !this.#hasActiveTurn();), Session.test.ts:4133-4137 and your own new finalizes dangling transcript calls when the session is closing with no active turn all pin. acpAgent.ts:6222-6224 is also explicit that "Prompt calls still waiting at Session admission are tracked in activePromptCalls but have no session pendingPrompt yet, so cancelPendingPrompt cannot see them", so a fix must not assume Session state covers admission-waiting calls.
Please restore the deleted end-to-end oracle rather than adding another mock-flip variant: drive a real agent.prompt() gated inside lastSessionMock.prompt, issue the backward sessionTranscript read, and assert mockHistoryReplayPage received expect.objectContaining({ finalizeDangling: false }) — measured red at HEAD and green with the fix. Fold in the base test's own arm as well, releasing the prompt inside readPage: measured against a narrower fix that keeps only the post-read activePromptCalls term, the gated-throughout assertion passes while that arm still fails, so a half-fix would ship green over the same false certification. Removing either term must turn it red.
中文说明
这条路由上的 finalizeDangling 判定现在只采样 Session.hasActiveTurn()，而它看不见一个「已登记、但仍卡在 Session 准入阶段」的 prompt 调用 —— 于是落在这个窗口里的一次 backward transcript 读取，会把一个悬空的 tool call 终态化地判定为失败，而那个 prompt 随后正要恢复或重新挂起同一个调用。
以悬空 tool call 结尾的 transcript 是「继续对话」的正常输入，并不罕见：Session.ts:10818-10830 会刻意让一个权限等待超时的、被恢复的 ask_user_question「悬空留在磁盘上，以便后续加载可以重新挂起它」，而 continueLastTurn() 的 interrupted_turn 分支处理的正是这一类（Session.ts:4917-4932）。当 daemon 派发该 prompt 时，agent 会把它登记进 activePromptCalls（acpAgent.ts:6180-6186），随后 await Session.prompt()；后者会阻塞在 assertCanStartTurn()（Session.ts:4582 —— 它本身也要 join recorder 的 operationTail），接着是 assertManagedConversationBindingReady，再接着 #syncLiveToolDeclarations() → await llmClient.setTools()（Live 会话上是一次网络往返），直到 Session.ts:4644 才安装 pendingPrompt。在整个这段窗口里 #hasActiveTurn() 都是 false。于是一次并发的、带 direction: 'backward' 的 qwen/status/session/transcript 读取会采样到 turnIdleBeforeRead === true 与 idleAtReplay === true，finalizeDangling 为 true；replayTranscriptRecordPage 根本没有 skipFinalizeCallIds 参数（history-replay-page.ts:364-377），而它的门控对 backward 页恒为真，所以 history-replayer.ts:170-182 会为该 callId 发出一个携带 MISSING_TRANSCRIPT_TOOL_RESULT_MESSAGE 的终态 success: false。随后 prompt 被准入，实时流又为同一个 callId 送上真实或合成的结果 —— 客户端先被告知该调用失败，然后又看着它运行。
这条路由本身没有加 gate，所以该场景是可达的：bridge 在 attach 时以 fire-and-forget 方式发出恢复 prompt（bridge.ts:7596-7651，调用点 :8478/:8508/:8650/:8679），而 GET /session/:id/transcript 会转发 direction（serve/routes/session.ts:4775-4880）并把子进程返回的页面原样作为响应，完全没有 prompt 门控 —— 你新加的注释（acpAgent.ts:4653-4656）自己也写明这条路由「同样未加 gate」。这个窗口恰恰在读取最可能发生时最宽：在 Live 或 managed-binding 会话上，上面那些 await 是毫秒到秒级；而在 lease 模式下共享的 operationTail 会从结构上把它拉长 —— 一次 barrier 操作排在 prompt 的 assertCanStartTurn 之前的读取，会在该 prompt 距离安装 pendingPrompt 还差好几个 await 时就完成。base 把这一切都挡住了：在 merge base 上这次读取计算的是 !activePromptBeforeRead && !this.activePromptCalls.has(sessionId)，在整个「已登记调用」窗口内都为 false，而且 base 有一个测试正好固定了这个形状，本 PR 把它改写掉了。
证据： 见上方探针输出。HEAD 侧为 finalizeDangling: true，加上修法后为 false；该切分是构造性确定的（探针把 prompt 阻塞在准入处，因此不是偶发窗口）。独立佐证：把被删掉的 base 测试原样恢复后，在 HEAD 上以 - "finalizeDangling": false / + "finalizeDangling": true 失败。
修法： 该修法跨两个位置，因此不是一键 suggestion。在读取之前采样 agent 级信号，并把它 AND 进判定（见上方代码块）。
约束： 该信号必须是 AND 进去的，绝不能替换掉 ignoreClosing 采样：closing 路径必须继续执行 finalize，这一点由 Session.ts:3928（return !this.closing && !this.#hasActiveTurn();）、Session.test.ts:4133-4137 以及你新增的 finalizes dangling transcript calls when the session is closing with no active turn 共同固定。acpAgent.ts:6222-6224 也明确写着「仍在等待 Session 准入的 prompt 调用会被登记在 activePromptCalls 中，但此时还没有 session pendingPrompt，所以 cancelPendingPrompt 看不见它们」，因此修法不得假定 Session 状态能覆盖等待准入的调用。
验收标准： 请恢复被删掉的端到端 oracle，而不是再加一个翻 mock 的变体：驱动一个真实的、被 gate 在 lastSessionMock.prompt 内部的 agent.prompt()，发起 backward 的 sessionTranscript 读取，并断言 mockHistoryReplayPage 收到 expect.objectContaining({ finalizeDangling: false }) —— 实测在 HEAD 上为红、加上修法后为绿。同时请把 base 测试自己的那一支也合并进来，即在 readPage 内部释放该 prompt：实测表明，若只采用保留「读取后 activePromptCalls 项」的窄修法，「全程 gate」那条断言会通过，而这一支仍然失败 —— 也就是说半个修法会在同样的错误终态判定之上绿灯通过。移除其中任何一项都必须让测试变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| expect(session.hasActiveTurn()).toBe(false); | ||
| }); | ||
|
|
||
| it('reports no active turn while the close gate is held', () => { |
There was a problem hiding this comment.
[Suggestion] R3-2: The new public Session.hasActiveTurn() is only ever asserted on its false side, so a stub that always returns false survives the entire suite while silently collapsing both finalizeDangling samples in production.
A repo-wide grep over packages/ (excluding dist/) finds hasActiveTurn at its definition (Session.ts:3931-3932) and at three assertions in this file — :4129, :4133, :4136 — all of them expect(...).toBe(false). The two production read sites (acpAgent.ts:4667 and :9065) are exercised in acpAgent.test.ts only through a mocked session whose default is hasActiveTurn: vi.fn().mockReturnValue(false) (:4807), never the real method. So changing the body to return false; leaves every assertion green, while in production turnIdleBeforeRead = !liveSession.hasActiveTurn() and idleAtReplay = !(session?.hasActiveTurn() ?? false) both become unconditionally true and finalizeDanglingForRestore returns true for every backward transcript read — including one served mid-flight with an unrecorded tool call, which hands the client a synthesized placeholder for a live call. Always-false is the dangerous direction: it fails by certifying a live tool call as abandoned, which is the issue 9704 symptom this PR exists to remove.
Witness:
Mutation runs in a scratch tree at HEAD,
npx vitest run src/acp-integration/acpAgent.test.ts
src/acp-integration/session/Session.test.ts
BASELINE (intact) Test Files 2 passed (2)
Tests 1461 passed (1461), exit 0
M1 hasActiveTurn() { return false; } Test Files 2 passed (2)
Tests 1461 passed (1461), exit 0 <- SURVIVES
M2 hasActiveTurn() { return this.isTurnIdle(); }
Test Files 1 failed | 1 passed (2)
Tests 2 failed | 1459 passed <- killed
M1 + the assertion below Tests 1 failed | 830 passed (831) <- fix kills M1
the assertion below, intact source Tests 831 passed (831) <- true assertion
The sibling mutant return this.isTurnIdle(); is already killed by the case you added here, because :4129 and :4133 assert the hasActiveTurn/isTurnIdle pair — and that pair is exactly the discriminator between the two methods. The gap is specifically the always-false stub, which nothing distinguishes from the real method.
const releaseMutation = session.beginHistoryMutation();
expect(session.hasActiveTurn()).toBe(true);Add that inside rejects a prompt while an exclusive history mutation is active, immediately after beginHistoryMutation() and before releaseMutation(), keeping the existing toBe(false) after the release as the contrasting sample. The state is already there, so this needs no new fixture: historyMutationActive is one of #hasActiveTurn()'s terms (Session.ts:4032), which is why :4116-4117 already assert isIdle()/isTurnIdle() false at that point. A prompt-in-flight variant would additionally cover the pendingPrompt term.
isTurnIdle() is already asserted false in that same test, confirming the state really is active there. With the assertion in place, stubbing the method to return false; must turn it red — measured, it fails at Session.test.ts:4115 with "expected false to be true", and the intact source stays 831/831 green, so the assertion is safe to add as-is.
中文说明
新增的公开方法 Session.hasActiveTurn() 只在 false 这一侧被断言过，因此一个恒返回 false 的桩实现能在整个测试套件中存活，同时在生产环境里悄悄让两个 finalizeDangling 采样一起塌掉。
在 packages/（排除 dist/）上做全仓 grep，hasActiveTurn 只出现在它的定义处（Session.ts:3931-3932）以及本文件的三处断言 —— :4129、:4133、:4136 —— 全部是 expect(...).toBe(false)。两个生产读取点（acpAgent.ts:4667 与 :9065）在 acpAgent.test.ts 中只经由一个 mock 会话被触达，其默认值是 hasActiveTurn: vi.fn().mockReturnValue(false)（:4807），从未经由真实方法。因此把方法体改成 return false; 之后所有断言仍然全绿，而在生产中 turnIdleBeforeRead = !liveSession.hasActiveTurn() 与 idleAtReplay = !(session?.hasActiveTurn() ?? false) 会双双恒为 true，finalizeDanglingForRestore 于是对每一次 backward transcript 读取都返回 true —— 包括在 tool result 尚未落盘、turn 仍在进行时被服务的那一次，那会给客户端递上一个为「仍在进行的调用」合成的占位结果。「恒为 false」是危险的那个方向：它的失效方式是把一个仍在进行的 tool call 判定为已被放弃，而这正是本 PR 要消除的 issue 9704 症状。
证据： 见上方变异测试运行输出。基线 1461/1461 全绿；M1（return false;）同样 1461/1461 全绿 —— 存活；M2（return this.isTurnIdle();）2 failed，被你新增的用例杀掉；M1 加上下方断言后 1 failed（修法杀掉了 M1）；下方断言在未修改的实现上 831/831 全绿。
兄弟变异 return this.isTurnIdle(); 已经被你在这里新增的用例杀掉了，因为 :4129 与 :4133 断言的是 hasActiveTurn/isTurnIdle 这一对 —— 而这一对正是两个方法之间的判别点。缺口恰恰是那个恒为 false 的桩：没有任何东西能把它与真实方法区分开。
修法： 见上方代码块 —— 在 rejects a prompt while an exclusive history mutation is active 中，紧接 beginHistoryMutation() 之后、releaseMutation() 之前加入该断言，并保留释放之后已有的 toBe(false) 作为对照样本。所需状态已经存在，因此不需要新的 fixture：historyMutationActive 是 #hasActiveTurn() 的其中一项（Session.ts:4032），这也正是 :4116-4117 在该处已断言 isIdle()/isTurnIdle() 为 false 的原因。若再补一个 prompt 进行中的变体，还能额外覆盖 pendingPrompt 这一项。
验收标准： 同一个测试中已经断言了 isTurnIdle() 为 false，说明该处状态确实是活跃的。加上该断言后，把方法桩成 return false; 必须让它变红 —— 实测会在 Session.test.ts:4115 以 "expected false to be true" 失败，而未修改的实现保持 831/831 全绿，所以这条断言可以直接安全加入。
— qwen3.8-max via Qwen Code /review (v0.23.0)
…wenLM#9704) AND the agent-level activePromptCalls sample into the ungated backward sessionTranscript finalize decision so a prompt already registered but still waiting at Session admission is not certified as a failed dangling tool call. Closing sessions with no active turn still finalize.
eaef407 to
809f3f9
Compare
|
Addressed Critical R3-1 on Ungated backward Restored the end-to-end oracle: a real |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-2
Session.hasActiveTurn()asserted only on itsfalseside - still stands, already reported (comment 3944888112)
Not explored to full depth (tool budget reached): "agent 6a": I did not verify whether any real ACP client issues beforeRecordId or no- direction transcript requests against a live session (finding 1's shape-reachabilit…; "agent 6a": I did not run the new acpAgent.test.ts cases, so my reading of what each asserts is from the diff text alone..
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/acpAgent.ts:9125 — [review] the lifecycle fallback's own trail cannot distinguish a clean drain from a failed one, and no test pins either semantics (removing the .catch guard survives 631/631)
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678 and carried as unresolved by reviews 5124611584, 5125551684 and 5126242224) - STILL STANDS at the reviewed commit. Re-verified this round against the actions runs API for head 809f3f9: Qwen Code CI, SDK Java and tui-parity are all completed with conclusion action_required on event=pull_request - the three pull_request-triggered workflows have never started on this commit and contribute no check run at all. The 12 check runs that do exist are bot orchestration and precheck plumbing (review-pr, publish-resolution, resolve-pr, ack-review-request, delay-automatic-review, label x2, authorize, review-config, precheck-pr/precheck, assign, Remind on force-push); none compiles the change or runs a test, and no lint signal exists on the commit in any form. A check-run-based classifier scores this commit all_pass on the four checks it can see - this round's own presubmit did exactly that - which is why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list (acp-integration), which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 in-scope workspaces including packages/cli, and the two changed suites pass 1462 tests in isolation - but that is this review's local run, not CI on the commit, and it does not include lint. Witness: gh api repos/QwenLM/qwen-code/actions/runs?head_sha=809f3f972fbf2a59e37beab27a8ec00e91a6f22f -> Qwen Code CI | completed | action_required | event=pull_request, SDK Java | completed | action_required | event=pull_request, tui-parity | completed | action_required | event=pull_request, the remaining five runs being pull_request_target bot plumbing; gh api .../commits/809f3f97.../check-runs -> total_check_runs=12, all orchestration/precheck; gh pr view 11144 -> reviewDecision CHANGES_REQUESTED, statusCheckRollup carries no compile, test or lint entry.
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 6a"：I did not verify whether any real ACP client issues beforeRecordId or no- direction transcript requests against a live session (finding 1's shape-reachabilit…；"agent 6a"：I did not run the new acpAgent.test.ts cases, so my reading of what each asserts is from the diff text alone.。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678 and carried as unresolved by reviews 5124611584, 5125551684 and 5126242224) - STILL STANDS at the reviewed commit. Re-verified this round against the actions runs API for head 809f3f9: Qwen Code CI, SDK Java and tui-parity are all completed with conclusion action_required on event=pull_request - the three pull_request-triggered workflows have never started on this commit and contribute no check run at all. The 12 check runs that do exist are bot orchestration and precheck plumbing (review-pr, publish-resolution, resolve-pr, ack-review-request, delay-automatic-review, label x2, authorize, review-config, precheck-pr/precheck, assign, Remind on force-push); none compiles the change or runs a test, and no lint signal exists on the commit in any form. A check-run-based classifier scores this commit all_pass on the four checks it can see - this round's own presubmit did exactly that - which is why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list (acp-integration), which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 in-scope workspaces including packages/cli, and the two changed suites pass 1462 tests in isolation - but that is this review's local run, not CI on the commit, and it does not include lint. Witness: gh api repos/QwenLM/qwen-code/actions/runs?head_sha=809f3f972fbf2a59e37beab27a8ec00e91a6f22f -> Qwen Code CI | completed | action_required | event=pull_request, SDK Java | completed | action_required | event=pull_request, tui-parity | completed | action_required | event=pull_request, the remaining five runs being pull_request_target bot plumbing; gh api .../commits/809f3f97.../check-runs -> total_check_runs=12, all orchestration/precheck; gh pr view 11144 -> reviewDecision CHANGES_REQUESTED, statusCheckRollup carries no compile, test or lint entry.
— qwen3.8-max via Qwen Code /review (v0.23.0)
Add positive hasActiveTurn() assertions in Session.test.ts so an always-false stub cannot greenwash finalizeDangling sampling. Covers history mutation and in-flight prompt paths; keeps R3-1 activePromptCalls AND behavior unchanged.
|
Addressed R3-2 in 5bb348b: added positive Session.hasActiveTurn() assertions (true during active turn) on the history-mutation and in-flight prompt paths in Session.test.ts, so an always-false stub cannot greenwash finalizeDangling sampling. R3-1 activePromptCalls AND behavior is unchanged. |
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: reverse audit — stopped before round 4 by the review time budget.
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678 and carried as unresolved by reviews 5124611584, 5125551684, 5126242224 and 5128103181) - STILL STANDS at the reviewed commit 5bb348b. Re-verified this round against the actions runs API for this head: Qwen Code CI, SDK Java and tui-parity are all completed with conclusion action_required on event=pull_request - the three pull_request-triggered workflows have never started on this commit and contribute no check run at all. Every check run that does exist is bot orchestration or precheck plumbing (review-pr, publish-resolution, ack-review-request, resolve-pr, delay-automatic-review, authorize, review-config, precheck-pr/precheck, assign, Remind on force-push, label, PR self-report label, Assign PR owner, PR Force-Push Reminder); none compiles the change or runs a test, and no lint signal exists on the commit in any form. This round's own presubmit scored the commit all_pass on the three check runs it could see - which is precisely why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list (acp-integration), which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 in-scope workspaces including packages/cli, packages/cli typecheck is clean, the changed file passes eslint and prettier, and the full Session.test.ts passes 832/832 - but that is this review's local run, not CI on the commit, and it is not the repo's CI matrix. Witness: gh api repos/QwenLM/qwen-code/actions/runs?head_sha=5bb348bc64184d9516bc3a8b4a418c0233cec305 -> SDK Java | completed | action_required | event=pull_request, Qwen Code CI | completed | action_required | event=pull_request, tui-parity | completed | action_required | event=pull_request, the remaining runs being pull_request_target bot plumbing.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678 and carried as unresolved by reviews 5124611584, 5125551684, 5126242224 and 5128103181) - STILL STANDS at the reviewed commit 5bb348b. Re-verified this round against the actions runs API for this head: Qwen Code CI, SDK Java and tui-parity are all completed with conclusion action_required on event=pull_request - the three pull_request-triggered workflows have never started on this commit and contribute no check run at all. Every check run that does exist is bot orchestration or precheck plumbing (review-pr, publish-resolution, ack-review-request, resolve-pr, delay-automatic-review, authorize, review-config, precheck-pr/precheck, assign, Remind on force-push, label, PR self-report label, Assign PR owner, PR Force-Push Reminder); none compiles the change or runs a test, and no lint signal exists on the commit in any form. This round's own presubmit scored the commit all_pass on the three check runs it could see - which is precisely why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list (acp-integration), which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 in-scope workspaces including packages/cli, packages/cli typecheck is clean, the changed file passes eslint and prettier, and the full Session.test.ts passes 832/832 - but that is this review's local run, not CI on the commit, and it is not the repo's CI matrix. Witness: gh api repos/QwenLM/qwen-code/actions/runs?head_sha=5bb348bc64184d9516bc3a8b4a418c0233cec305 -> SDK Java | completed | action_required | event=pull_request, Qwen Code CI | completed | action_required | event=pull_request, tui-parity | completed | action_required | event=pull_request, the remaining runs being pull_request_target bot plumbing.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| sessionId: 'test-session-id', | ||
| prompt: [{ type: 'text', text: 'hello' }], | ||
| }); | ||
| await vi.waitFor(() => expect(session.hasActiveTurn()).toBe(true)); |
There was a problem hiding this comment.
[Suggestion] R3-2: (fix-induced) The positive hasActiveTurn() assertions added here do close R3-2 — an always-false stub is now killed, measured red at Session.test.ts:4116 and :4157. But they sample the new public wrapper in only three of its input states, so two independent narrowing mutants still survive the entire suite green, and each one drops a property the transcript read depends on.
hasActiveTurn() { return !this.closing && this.#hasActiveTurn(); } survives Session.test.ts 832/832 and acpAgent.test.ts 631/631 — the new close-gate test at :4136 holds the gate with no turn, where mutant and real code both return false, and acpAgent.test.ts only ever stubs lastSessionMock!.hasActiveTurn. Shipped, a transcript read served while restore holds the close gate over a still-draining turn (acpAgent.ts:4232-4237 takes beginClose() before waitForActiveTurnsToSettle) samples idle at acpAgent.ts:9073 and :4674, so finalizeDangling becomes true and the replay finalizes a trailing tool call whose result is about to land — collapsing ignoreClosing into exactly the behaviour acpAgent.ts:4653-4658 says it exists to avoid.
hasActiveTurn() { return Boolean(this.pendingPrompt || this.historyMutationActive); } survives 2070/2070 tests across all 39 src/acp-integration files, because the other eight terms of #hasActiveTurn() are pinned only through the private predicate — waitForActiveTurnsToSettle, rewindToTurn (Session.ts:4320) and restoreHistory (:4389) all call #hasActiveTurn(), so the guards at Session.test.ts:6364-6534 cannot see a public-wrapper narrowing. Shipped, a backward page served during an autonomous goal/cron/notification turn — which activePromptCalls does not cover, since it wraps only the agent-mediated session.prompt() call (acpAgent.ts:6181-6203) — computes finalizeDangling: true, contradicting the consumer's own contract at acpAgent.ts:4643-4646 ("a client prompt or an autonomous goal/cron/notification turn"). Either way the #9704 placeholder returns on the path this predicate was added for, with a green suite.
One added test kills both, reusing the send gate the in-flight test already builds:
it('reports an active turn from a non-prompt source under the close gate', async () => {
// drive an autonomous turn source through the internals-cast pattern at :4036-4040
// (notificationProcessing + notificationCompletion), or gate sendMessageStream and
// start prompt() the way the in-flight test below does
await vi.waitFor(() => expect(session.hasActiveTurn()).toBe(true));
const releaseClose = session.beginClose();
expect(session.hasActiveTurn()).toBe(true);
expect(session.isTurnIdle()).toBe(false);
releaseClose();
// clear the turn source, then assert the settled side
expect(session.hasActiveTurn()).toBe(false);
expect(session.isTurnIdle()).toBe(true);
});Witness:
mutant A hasActiveTurn(){ return !this.closing && this.#hasActiveTurn(); }
Session.test.ts Tests 832 passed (832) EXIT=0 <- survives
acpAgent.test.ts Tests 631 passed (631) EXIT=0 <- survives
mutant B hasActiveTurn(){ return Boolean(this.pendingPrompt || this.historyMutationActive); }
src/acp-integration Test Files 39 passed (39) / Tests 2070 passed (2070) <- survives
one combined probe (autonomous turn source + beginClose() held over the live turn):
mutant A + probe x AssertionError: expected false to be true
mutant B + probe x AssertionError: expected false to be true
intact + probe v 1 passed | 832 skipped (833)
R3-2's own mutant hasActiveTurn(){ return false; } against this commit:
x Session.test.ts:4116 x Session.test.ts:4157 <- killed by the delta, as intended
Keep the new expectation un-folded with respect to closing — hasActiveTurn() excludes it deliberately so that "a closing session with no active turn must still finalize abandoned trailing calls" (acpAgent.ts:4655-4658), which the close-gate test at :4133-4141 already pins; and build the combined state from an in-flight prompt or an autonomous turn rather than a history mutation, because beginClose() throws when already closing (Session.ts:4065-4070) and beginHistoryMutation() throws while closing (Session.ts:4044-4047), so the gate has to be taken once, after the turn is live.
The test that must pin this is the new one above: it has to go red on its expect(session.hasActiveTurn()).toBe(true) line under both mutants, each of which the six assertions added here tolerate — please run each mutation against it and confirm it reds.
中文说明
这里新增的 hasActiveTurn() 正向断言确实解决了 R3-2 —— 恒返回 false 的 stub 现在会被杀掉，实测在 Session.test.ts:4116 与 :4157 变红。但它们只在这个新公共 wrapper 的输入状态中采样了三种，因此仍有两个互相独立的「收窄」变异体能在整个测试套件全绿的情况下存活，而每一个都丢掉了 transcript 读取所依赖的一条性质。
hasActiveTurn() { return !this.closing && this.#hasActiveTurn(); } 在 Session.test.ts 832/832、acpAgent.test.ts 631/631 下存活 —— 新增的 close-gate 测试（:4136）是在没有活动 turn 的情况下持有 gate 的，此时变异体与真实代码都返回 false；而 acpAgent.test.ts 始终只是 stub lastSessionMock!.hasActiveTurn。一旦这样发布：restore 在仍有 turn 在 drain 时持有 close gate（acpAgent.ts:4232-4237 先 beginClose()，之后才 waitForActiveTurnsToSettle），此时被服务的一次 transcript 读取会在 acpAgent.ts:9073 与 :4674 采样到 idle，于是 finalizeDangling 变为 true，replay 会把一个结果马上就要落地的尾部 tool call 终态化 —— 这恰好把 ignoreClosing 塌缩成 acpAgent.ts:4653-4658 说明它要避免的那种行为。
hasActiveTurn() { return Boolean(this.pendingPrompt || this.historyMutationActive); } 在全部 39 个 src/acp-integration 文件的 2070/2070 个测试下存活，因为 #hasActiveTurn() 另外八个条件项只通过私有谓词被固定 —— waitForActiveTurnsToSettle、rewindToTurn（Session.ts:4320）与 restoreHistory（:4389）调用的都是 #hasActiveTurn()，所以 Session.test.ts:6364-6534 的那些守卫看不见对公共 wrapper 的收窄。一旦这样发布：在自主 goal/cron/notification turn 期间被服务的一次 backward 分页 —— activePromptCalls 覆盖不到它，因为该注册表只包住由 agent 中介的 session.prompt() 调用（acpAgent.ts:6181-6203）—— 会算出 finalizeDangling: true，与消费方自己在 acpAgent.ts:4643-4646 写明的契约（“客户端 prompt，或自主的 goal/cron/notification turn”）相矛盾。两种情况下，#9704 的占位提示都会在这个谓词被引入的那条路径上重新出现，而测试套件全绿。
补一个测试即可同时杀掉两者，复用 in-flight 测试已经搭好的 send gate（代码见上方英文部分，不重复）。
证据： 见上方变异实测输出。
新增的断言对 closing 必须保持「不折叠」—— hasActiveTurn() 有意不包含它，以便「一个正在关闭、但没有活动 turn 的会话仍必须把已放弃的尾部调用终态化」（acpAgent.ts:4655-4658），而这一点已由 :4133-4141 的 close-gate 测试固定；并且这个组合状态要用一个进行中的 prompt 或自主 turn 来构造，而不是 history mutation —— 因为 beginClose() 在已经 closing 时会抛错（Session.ts:4065-4070），beginHistoryMutation() 在 closing 期间也会抛错（Session.ts:4044-4047），所以 gate 只能在 turn 已经活动之后取一次。
验收标准： 需要固定这一点的就是上面这个新测试：在上述两个变异体下，它都必须在自己的 expect(session.hasActiveTurn()).toBe(true) 这一行变红，而这两个变异体是本次新增的六条断言都能容忍的 —— 请分别施加这两个变异并确认它变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Add a Session.test.ts case that holds beginClose() over a live notification turn and asserts hasActiveTurn() stays true. Kills narrowing mutants that fold in closing or drop non-prompt turn sources.
|
R3-2 strengthened at R2-6 CI-evidence is still blocked on maintainer workflow approval for a first-time contributor ( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-4 comment/title/description still credit the write barrier with the issue-9704 fix - still stands, already reported (comment 3944179491)
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/session/Session.test.ts:4172 — [review] R5-1: (fix-induced) the non-prompt-source test co-sets notificationProcessing and notificationCompletion, so three terms of #hasActiveTurn() (notificationCompletion, g…
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678 and carried as unresolved by reviews 5124611584, 5125551684, 5126242224, 5128103181 and 5133193655) - STILL STANDS at the reviewed commit 8e04553. Re-verified this round against the actions runs API for this head: Qwen Code CI, tui-parity and SDK Java are all completed with conclusion action_required on event=pull_request - the three pull_request-triggered workflows have never started on this commit and contribute no check run at all. The 11 check runs that do exist are bot orchestration and precheck plumbing (review-pr, publish-resolution, ack-review-request, resolve-pr, delay-automatic-review, authorize, review-config, precheck-pr/precheck, label, assign, Remind on force-push); none compiles the change or runs a test, and no lint signal exists on the commit in any form. This round's own presubmit again scored the commit all_pass on the 3 check runs it could see - which is precisely why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list (acp-integration), which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 in-scope workspaces including packages/cli, packages/cli typecheck is clean, and the full Session.test.ts passes 833/833 both in isolation and inside the workspace suite (the one red file in that suite, src/config/settings.test.ts, fails identically on the merge base: measured netNew: [], shared: ["src/config/settings.test.ts"], so pre-existing and not attributable to this diff) - but that is this review's local run, not CI on the commit, and it is not the repo's CI matrix. Witness: gh api repos/QwenLM/qwen-code/actions/runs?head_sha=8e045536a38c601584c6c55bb0ac972e1f2f3d3b -> Qwen Code CI | completed | action_required | pull_request, tui-parity | completed | action_required | pull_request, SDK Java | completed | action_required | pull_request, the remaining runs being pull_request_target bot plumbing; gh api .../commits/8e045536a3.../check-runs -> total_count=11, all orchestration/precheck; gh pr view 11144 -> reviewDecision CHANGES_REQUESTED.
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
[Critical] R2-6 CI-evidence blocker filed by @qwen-code-ci-bot (issue comment 5556889374, restated in review 5124108678 and carried as unresolved by reviews 5124611584, 5125551684, 5126242224, 5128103181 and 5133193655) - STILL STANDS at the reviewed commit 8e04553. Re-verified this round against the actions runs API for this head: Qwen Code CI, tui-parity and SDK Java are all completed with conclusion action_required on event=pull_request - the three pull_request-triggered workflows have never started on this commit and contribute no check run at all. The 11 check runs that do exist are bot orchestration and precheck plumbing (review-pr, publish-resolution, ack-review-request, resolve-pr, delay-automatic-review, authorize, review-config, precheck-pr/precheck, label, assign, Remind on force-push); none compiles the change or runs a test, and no lint signal exists on the commit in any form. This round's own presubmit again scored the commit all_pass on the 3 check runs it could see - which is precisely why the blocker keys on the workflow runs rather than on check runs: a workflow that never started contributes no check run to score. packages/cli/src/acp-integration/acpAgent.ts is on the repo's high-risk path list (acp-integration), which the triage pass states requires CI evidence before approval. This is a maintainer action rather than an author fix: the fork's workflow runs need releasing. For completeness, this review produced its own evidence on the commit - the build is green across all 17 in-scope workspaces including packages/cli, packages/cli typecheck is clean, and the full Session.test.ts passes 833/833 both in isolation and inside the workspace suite (the one red file in that suite, src/config/settings.test.ts, fails identically on the merge base: measured netNew: [], shared: ["src/config/settings.test.ts"], so pre-existing and not attributable to this diff) - but that is this review's local run, not CI on the commit, and it is not the repo's CI matrix. Witness: gh api repos/QwenLM/qwen-code/actions/runs?head_sha=8e045536a38c601584c6c55bb0ac972e1f2f3d3b -> Qwen Code CI | completed | action_required | pull_request, tui-parity | completed | action_required | pull_request, SDK Java | completed | action_required | pull_request, the remaining runs being pull_request_target bot plumbing; gh api .../commits/8e045536a3.../check-runs -> total_count=11, all orchestration/precheck; gh pr view 11144 -> reviewDecision CHANGES_REQUESTED.
— qwen3.8-max via Qwen Code /review (v0.23.0)
kabishou11
changed the title
|
Updated title + description for R2-4: the #9704 symptom fix is finalizeDangling / isTurnIdle()/hasActiveTurn() sampling; the write barrier is separate latest/backward read hardening and cannot alone close that issue window. R2-6 still needs a maintainer to approve the pending pull_request workflows (Qwen Code CI / tui-parity / SDK Java) on this fork PR. |
3 participants
What this PR does
Routes restore cleanup through the shared
finalizeDanglingForRestorehelper and samplesisTurnIdle()/hasActiveTurn()so concurrent session loads do not report a false dangling tool while a turn is still active — this is the mechanism that changes the #9704 replay outcome.Also barriers live
sessionTranscriptdisk reads behind in-flight tool-result writes on the ACP agent path for the latest/backward read (direction === 'backward'). Cursor,beforeRecordId, and snapshot/anchor pages still read the persisted transcript directly. The barrier alone cannot close the #9704 window (tool results are recorded only after the batch completes, sooperationTailis empty mid-execution).Why it's needed
During a live session, loading the transcript while a turn is still active can show: "Tool result missing from saved history; the previous run likely ended before this tool completed." The tool result is written shortly after, but the race makes the UI/CLI report a false dangling tool. Issue #9704 describes this TOCTOU on concurrent session load.
runWithWriteBarrieris not a drop-in for the oldflush(): it also throwsSessionWriterUnavailableErrorwhen the recorder is notactive/acceptingWrites. Web UI history pagination never sendsdirection(onlycursor/beforeRecordId), so those pages must not go through the barrier.Reviewer Test Plan
How to verify
beforeRecordIdafter a recording write failure or during managed shutdown/handoff: the page still returns. That path does not consult writer health.Unit coverage:
packages/cli/src/acp-integration/acpAgent.test.tscovers barrier ordering on the backward/latest path, and asserts cursor/anchor pages skip the barrier even when the recorder would refuse writes.Evidence (Before & After)
Before: concurrent load during an in-flight tool write can surface "Tool result missing from saved history" even though the result lands on disk soon after.
After: latest/backward live transcript reads are sequenced behind tool-result writes; cursor/anchor pagination stays a direct disk read. Unit tests assert both.
N/A for Before/After screenshots (non-TUI, race fix + unit tests).
Tested on
Environment (optional)
Unit tests via the package test runner for
packages/cli.Risk & Scope
Ungated
sessionTranscriptfinalize ANDs the agent-levelactivePromptCallssample (a prompt already registered but still waiting at Session admission) with the ignore-closing active-turn sample, so a close-gate hold still finalizes abandoned calls while an admission-waiting continuation is not certified as failed.Linked Issues
Fixes #9704
中文说明
这个 PR 做了什么
在 ACP agent 路径上，把实时
sessionTranscript磁盘读取挡在进行中的 tool-result 写入之后，并通过共享的finalizeDanglingForRestore做恢复清理，避免并发加载读到半写入的 tool turn。写入屏障只作用于最新/backward 读取（
direction === 'backward'）。带 cursor、beforeRecordId或 snapshot/anchor 的分页直接读已落盘的 transcript。为什么需要
实会话中若在 tool 仍在写结果时加载 transcript，会出现 "Tool result missing from saved history…"。结果稍后会写入，但这是竞态导致的误报。#9704 描述了该 TOCTOU。
runWithWriteBarrier不能直接替换原来的flush()：它还会在 recorder 非active/acceptingWrites时抛出SessionWriterUnavailableError。Web UI 历史分页从不发送direction（只传cursor/beforeRecordId），因此这些页不能走 barrier。评审人验证计划
如何验证
beforeRecordId向前翻历史仍应返回页面，该路径不查询 writer 健康状态。单测：
acpAgent.test.ts覆盖 backward/latest 的 barrier 顺序，并断言 cursor/anchor 分页在 recorder 拒绝写入时仍直接读取。证据（前后对比）
前：并发加载可误报缺失；后：最新/backward 读写有序，cursor/anchor 分页仍直接读盘。单测覆盖两者。非 TUI，截图 N/A。
测试平台
Linux ✅；macOS/Windows N/A（单测）。
风险与范围
writeFailure，先await recording.flush().catch(() => undefined)drain 排队尾部再读盘，避免关闭/handoff 窗口丢掉已排队记录；flush上的 latchedwriteFailure被吞掉以免把尽力而为路径打回硬拒绝。barrier 准入时已锁定的writeFailure仍使读取失败。cursor/beforeRecordId，直接读盘。sessionTranscriptfinalize 按 active-turn 采样（忽略Session.closing），避免 close gate 把已放弃的 tool call 永远留作 pending。关联 Issue
Fixes #9704