feat(serve): add pollable turn-status endpoints for daemon sessions - #8682
feat(serve): add pollable turn-status endpoints for daemon sessions#8682BenGuanRan wants to merge 36 commits into
Conversation
github-actions
Bot
added
the
review/self-reported
|
@/tmp/e2e-comment-fixed.md |
Add GET /session/:id/turns/current and GET /session/:id/turns/:promptId so external callers can poll a turn's lifecycle state (queued / running / completed / cancelled / error) and result instead of holding the SSE stream for the whole turn lifetime. - Live state comes from the bridge's pending prompt queue; settled outcomes from persisted turn_result transcript records, so results survive daemon restarts and the daemon keeps no per-turn memory - Each prompt captures its own recording and settles exactly that one, so overlapping turns (DAEMON-003 deadline overlap) can never misattribute one turn's outcome to another promptId - Enforces the same client authorization as POST /session/:id/prompt Refs QwenLM#8680
82df1ca to
e16c23e
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Thanks for the PR — and for pairing it with the design doc and the linked issue. Template looks good ✓ Problem: real, not theoretical. Linked issue #8680 was triaged as a P2 feature request under Direction: aligned — read-only, additive, and on the daemon integration surface (IM bridges, automation, editor integrations) that is actively growing. Supporting signal: Claude Code's CHANGELOG (2.1.211) records the same need — "Improved background agent result reporting — Claude now reports the status of still-running agents…" — so pollable turn/agent state for non-attached consumers is a direction both products are converging on. Size: core paths are touched ( Approach: the outline is the right shape — reuse the existing transcript append path for settled outcomes (no new daemon memory, survives restarts), live queue wins over settled records, same Risk: Moving on to code review. 🔍 中文说明感谢贡献——设计文档与关联 issue 都准备齐全。 模板完整 ✓ 问题：真实存在，不是理论性问题。关联 issue #8680 已完成 triage，为 P2 功能请求，归属 方向：对齐——只读、增量式，且正位于 daemon 活跃发展的集成面（IM 桥接、自动化、编辑器集成）。参考信号：Claude Code 的 CHANGELOG（2.1.211）记录了同样的需求——"Improved background agent result reporting — Claude now reports the status of still-running agents…"——即为未挂载的消费方提供可轮询的轮次/代理状态，是两个产品都在收敛的方向。 规模：触及核心路径（ 方案：总体形态正确——复用现有 transcript 追加路径落盘终态（不新增 daemon 内存、重启后仍可查）、实时队列优先于已落定记录、与 prompt 提交共用 风险： 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed against the squashed head (the force-push dropped the unrelated Feishu design doc — the diff is now one thing). Independent baseline first: for this problem I would have built exactly the shape this PR implements — routes next to
Things that looked fragile but check out against the base code:
No correctness or security blockers found. One non-blocking observation: sequenceDiagram
participant P1 as Client
participant P2 as serve session routes
participant P3 as AcpSessionBridge
participant P4 as owning ACP child
participant P5 as SessionTranscriptReader
P1->>P2: GET turn status
P2->>P3: getSessionTurnStatus (authorize client id)
P3->>P3: check live pending list, live wins
alt no live entry
P3->>P4: ext-method session turn_status
P4->>P4: flush recorder, best-effort
P4->>P5: bounded backward scan, 10 pages x 500
P5-->>P4: turn_result record or null
P4-->>P3: settled record or null
end
P3-->>P2: live or settled status
P2-->>P1: 200 status, or 404 prompt_not_found
Files changed (17 of 17 shown)
Test evidence — the PR's own CIThe reviewed commit is a fresh force-push: the PR's own CI workflow runs have not started on it yet (on fork PRs the first run can wait for maintainer approval). The only checks present on the commit are bot orchestration jobs. The table below reflects that snapshot and will be rewritten in place by the finalize workflow once CI settles — treat any approval decision as gated on that.
The author posted an E2E report in this thread covering all state transitions against a live daemon with a mock provider — that is the author's self-reported result (macOS only), not independently re-run here; the Sandboxed verification would settle this: 中文说明代码审查先说独立基线：如果由我来实现，方案与本 PR 几乎完全一致——路由与
看似脆弱但对照基础代码确认无问题的点：
未发现正确性或安全性阻塞项。一个非阻塞观察：Session.ts 的 （时序图见英文部分：Client → serve 路由 → bridge 实时队列优先 → 否则经 ext-method 由 ACP 子进程 flush 后有界回溯扫描 transcript。） 测试证据被审提交是一次新的 force-push：PR 自身的 CI 工作流尚未在该提交上启动（fork PR 首次运行可能需维护者批准），提交上目前只有 bot 编排类检查。上方 CI 表格即该快照，finalize 工作流会在 CI 落定后就地改写——任何批准决定都应以之为前提。 作者在线程中贴出了一份对真实 daemon（mock provider）覆盖全部状态转换的 E2E 报告——这是作者自述结果（仅 macOS），此处未独立复跑； 沙箱验证可以定论： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review across every stage, but the fork-PR core-surface size needs a maintainer's sign-off per Stage 0 policy, and CI has not yet run on the reviewed commit. Stepping back: this is a fork PR that is easier to review than to fault. The architecture is exactly what I would have proposed independently — live queue first, settled outcomes through the existing transcript append path, reads served by the owning child — and it handles the edge I would have missed on a first pass: the DAEMON-003 deadline overlap where a successor is admitted before the predecessor settles, with a real regression test proving each record lands on its own promptId. The cancel-classification fix (key on the abort reason, not the error shape) is a genuine bug the author's own E2E caught, fixed at the root with a test. The diff is minimal now that the unrelated design doc is gone, the failure semantics are documented honestly (removals are not pollable; recording is best-effort), and the test volume (~1087 lines) outweighs the production logic (~605 lines). If I had to maintain this in six months, I would thank the author. Why it is still not an approval:
⏸️ Deferring to the core owners (@wenshao @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC) — no blocking issues found in the code; what is needed is the core-module maintainer sign-off the two-tier rule prescribes, green CI on the reviewed commit, and ideally a 中文说明置信度：3/5 —— 各阶段审查都很干净，但按 Stage 0 政策，fork PR 触及核心面的规模需要维护者签核，且被审提交上 CI 尚未运行。 退一步看：这是一个"挑不出毛病、只有好审"的 fork PR。架构与我独立提出的方案完全一致——实时队列优先、终态走既有 transcript 追加路径、读取由持有会话的子进程提供——并且处理了我第一遍会漏掉的边界：DAEMON-003 截止时间重叠（后继在前序落定前被准入），且有真实的回归测试证明各自记录落在各自 promptId 上。取消分类修复（按 abort reason 而非错误形状分类）是作者 E2E 自己抓到的真实 bug，在根上修复并配了测试。无关设计文档移除后 diff 已是最小集；失败语义诚实落档（移除不可轮询、记录 best-effort）；测试量（约 1087 行）超过生产逻辑（约 605 行）。半年后接手维护，只会感谢作者。 仍不批准的原因：
⏸️ 转交核心 owner（@wenshao @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC）——代码中未发现阻塞问题；需要的是两级规则要求的核心模块维护者签核、被审提交上的绿色 CI，以及最好跑一次 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
中文说明
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
— qwen3.8-max via Qwen Code /review (v0.21.7)
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution for PR #8682 (turn-status polling)Root causeMain diverged far past this PR's base. Four main-side features collided with the PR at the same insertion points: Goal v3 ( Textual or semanticFour files were textual only (both sides added disjoint members at the same spot; resolution = keep both): One conflict was semantic: await this.assertCanStartTurn();
if (this.liveScreenContextTool || this.liveTaskTools.length > 0 || this.liveSpeakToUserTool) {
await this.#syncLiveToolDeclarations();
}
if (this.closing) {
throw RequestError.invalidParams(undefined, 'Session is closing');
}
const turnRecording = this.#beginTurnRecording(params, invocationContext);What is load-bearing
What I could not verify (needs follow-up)
中文说明根因：main 已远超本 PR 基线，四处相撞：Goal v3 记录、会话级 runtime MCP 扩展方法、Live Voice 在 文本 vs 语义：四个文件为纯文本冲突（双方同位置添加不相交成员，均保留）： 关键点： 需后续处理： |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "session_turn_status" |
— Qwen Code · serve A/B
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
[Critical] R1-2 (round-1 Critical, re-checked at this commit, still stands): #accumulateTurnResultText attributes every agent_message_chunk to whichever recording occupies the shared #turnRecording slot, with no ownership check and no _meta.rewritten filter. With opt-in messageRewrite, a late re-emitted rewrite lands in a successor turn's persisted record (cross-turn misattribution: GET /turns/ returns A's text), and every rewritten turn accumulates original+rewritten text. Probe-confirmed in round 1; no fix in this round's diff. Already on the PR at Session.ts:4868.
[Critical] R1-3 (round-1 Critical, re-checked at this commit, still stands): the live filter pendingPromptList.filter((p) => !p.removed) misses deadline-terminated entries — a queued prompt whose DAEMON-003 deadline expired stays on the list with terminalPublished: true (nothing splices it until its FIFO node runs, blocked behind a wedged predecessor), so polling reports queued indefinitely after SSE already published turn_error{code:'prompt_deadline_exceeded'}, then flips to permanent 404 once the wedge clears. Probe-confirmed in round 1; no fix in this round's diff. Already on the PR at bridge.ts:7879.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
[Critical] R1-2 (round-1 Critical, re-checked at this commit, still stands): #accumulateTurnResultText attributes every agent_message_chunk to whichever recording occupies the shared #turnRecording slot, with no ownership check and no _meta.rewritten filter. With opt-in messageRewrite, a late re-emitted rewrite lands in a successor turn's persisted record (cross-turn misattribution: GET /turns/ returns A's text), and every rewritten turn accumulates original+rewritten text. Probe-confirmed in round 1; no fix in this round's diff. Already on the PR at Session.ts:4868.
[Critical] R1-3 (round-1 Critical, re-checked at this commit, still stands): the live filter pendingPromptList.filter((p) => !p.removed) misses deadline-terminated entries — a queued prompt whose DAEMON-003 deadline expired stays on the list with terminalPublished: true (nothing splices it until its FIFO node runs, blocked behind a wedged predecessor), so polling reports queued indefinitely after SSE already published turn_error{code:'prompt_deadline_exceeded'}, then flips to permanent 404 once the wedge clears. Probe-confirmed in round 1; no fix in this round's diff. Already on the PR at bridge.ts:7879.
— qwen3.8-max via Qwen Code /review (v0.21.7)
…20a9 # Conflicts: # integration-tests/cli/qwen-serve-streaming.test.ts
Targeted blocker verification at
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): This PR adds a daemon (qwen-serve) turn-status polling en...: none — all planned checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; chunk 4: I did not execute the integration suite or a standalone typecheck for these files (heavy daemon tests requiring a bundled build); verification was static.; chunk 4: did not execute the integration suite or a standalone typecheck for these files — these are long daemon tests requiring a bundled build; verification above is s….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
未探索到全部深度（达到工具调用预算）：This PR adds a daemon (qwen-serve) turn-status polling en...：none — all planned checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.；chunk 4：I did not execute the integration suite or a standalone typecheck for these files (heavy daemon tests requiring a bundled build); verification was static.；chunk 4：did not execute the integration suite or a standalone typecheck for these files — these are long daemon tests requiring a bundled build; verification above is s…。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| ).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('keeps overlapping turn records attributed to their own promptIds', async () => { |
There was a problem hiding this comment.
[Suggestion] R6-34: The DAEMON-003 overlap test never streams a predecessor chunk AFTER the successor's admission, so the recording-publication timing guard (Session.ts ~3181-3188 — publish only after the predecessor settles; the comment names exactly this regression) is unpinned. Probe-verified: a mutant publishing the successor's recording at admission keeps all three existing overlap tests green; opening the real window (predecessor chunk delivery held in flight across the successor's admission) flips it — the predecessor's record loses resultText entirely. — Concrete cost: a refactor moving publication earlier (into admission) redirects the predecessor's still-streaming chunks into the successor's record in production (capture reads the current #turnRecording slot), corrupting both persisted turn_result records with wrong promptId attribution — while this test stays green because firstStream yields its only chunk before the successor is admitted and never yields again. Fix: gate a second predecessor yield on a promise resolved after the successor is admitted (and the admission flush) but before releaseFirst(); assert that chunk lands in the first record's resultText while the second record's does not contain it.
中文说明
DAEMON-003 重叠测试从不在后继准入之后流式输出前驱 chunk，因此记录发布时机守卫（Session.ts 约 3181-3188——只在前驱落定后发布；注释明确点名的正是这个回归）未被钉住。探针验证：把后继记录在准入时就发布的变异能让现有三个重叠测试全绿；打开真实窗口（前驱 chunk 投递在后继准入期间悬置）后变异翻转——前驱记录完全丢失 resultText。具体代价：若重构把发布提前（到准入时），生产中前驱仍在流式的 chunk 会被重定向进后继的记录（捕获读取当前 #turnRecording 槽位），两条持久化 turn_result 记录的 promptId 归因都被破坏——而本测试仍全绿，因为 firstStream 在后继准入前就输出了唯一的 chunk 且之后不再输出。修复：把第二次前驱输出 gate 在一个 promise 上，该 promise 在后继准入（及准入 flush）之后、releaseFirst() 之前 resolve；断言该 chunk 落入第一条记录的 resultText，且第二条记录不包含它。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(mockSendUpdate).toHaveBeenCalledTimes(1); | ||
| expect(rewriter.commitOutput).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R6-35: No test pins the AFTER-delivery generation/abort re-check that gates commitOutput (MessageRewriteMiddleware.ts ~208): the abort test aborts before delivery starts (pre-delivery guard only), and every delivery mock in the file resolves synchronously, so the inter-guard window never opens. Probe-verified: deleting the second check keeps 22/22 tests green while a discard during in-flight delivery commits the discarded turn's rewrite. — Concrete cost: in production sendUpdate is an awaited client/channel write with unbounded latency; when discardTurn() lands while a rewrite's delivery is in flight, the second re-check is the only thing preventing the discarded turn's rewrite from entering outputHistory — which feeds the next turn's rewrite prompt as '上一轮改写结果' context ('the user has already seen it. Don't repeat...'), suppressing or skewing later user-visible progress updates. Fix: add a deferred-delivery test — sendUpdate returns a pending promise for the rewritten emission; flush, resolve the rewrite, let the continuation pass the first guard and enter delivery, then discardTurn(); resolve the deferred delivery; assert commitOutput was NOT called. Also add commitOutput: vi.fn() to the drain test's replacement mock so its path doesn't throw-swallow.
中文说明
没有任何测试钉住门控 commitOutput 的投递后生成代/中止复查（MessageRewriteMiddleware.ts 约 208 行）：中止测试在投递开始前就中止（只覆盖投递前守卫），文件中所有投递 mock 都同步 resolve，因此两个守卫之间的窗口从未被打开。探针验证：删除第二个检查，22/22 测试仍全绿，而投递在途时的 discard 会把被丢弃 turn 的改写提交进上下文。具体代价：生产中 sendUpdate 是延迟无界的被 await 客户端/渠道写入；当 discardTurn() 落在某改写投递在途时，第二个复查是阻止被丢弃 turn 的改写进入 outputHistory 的唯一屏障——而 outputHistory 会作为"上一轮改写结果"上下文喂给下一 turn 的改写提示（"用户已经看过了，不要重复……"），从而抑制或扭曲后续用户可见的进展更新。修复：补充延迟投递测试——sendUpdate 对改写产出返回挂起的 promise；flush、resolve 改写、让续行通过第一个守卫进入投递，然后 discardTurn()；再 resolve 延迟投递；断言 commitOutput 未被调用。同时给 drain 测试的替换 mock 加上 commitOutput: vi.fn()，避免其路径抛异常被吞。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const status = await runtime.bridge.getSessionTurnStatus( | ||
| sessionId, | ||
| clientId !== undefined ? { clientId } : undefined, | ||
| promptId, |
There was a problem hiding this comment.
[Suggestion] R6-36: Both new turn-status GET routes read persisted turn_result records without the archiveCoordinator shared lock that the comparable persisted-read route takes: GET /session/:id/transcript (~2759) wraps runtime resolution + bridge read in archiveCoordinator.runSharedMany([sessionId], ...) precisely so archive transitions wait; the new routes are plain resolveLiveSessionRuntime + bridge call. — Concrete cost: a poll for an evicted settled turn (the persisted path — the case the route exists for) passes resolveLiveSessionRuntime while the session is live, then awaits the child RPC (flush + bounded transcript scan); concurrently POST /sessions/archive or DELETE /session/:id runs closeSession + file move/delete inside runExclusiveMany. The bridge degrades to the overlay only when terminalBeforeRead exists — exactly not the case here — so the poll surfaces an error instead of a settled answer; the window grows with transcript size, and long-lived sessions are the ones most likely to be archived. Fix: wrap each route's bridge call in archiveCoordinator.runSharedMany([sessionId], async () => ...) (moving runtime resolution inside), mirroring the transcript route.
中文说明
两个新的 turn-status GET 路由读取持久化 turn_result 记录时没有取同类持久化读取路由所取的 archiveCoordinator 共享锁：GET /session/:id/transcript（约 2759 行）把运行时解析 + bridge 读取包在 archiveCoordinator.runSharedMany([sessionId], ...) 中，正是为了让归档转换等待；新路由只是裸的 resolveLiveSessionRuntime + bridge 调用。具体代价：对被逐出 overlay 的已落定 turn 的轮询（持久化路径——该路由存在的场景）在会话存活时通过 resolveLiveSessionRuntime，然后 await 子进程 RPC（flush + 有界转录扫描）；与此同时 POST /sessions/archive 或 DELETE /session/:id 在 runExclusiveMany 内执行 closeSession + 文件移动/删除。bridge 只在存在 terminalBeforeRead 时降级到 overlay——此处恰恰没有——因此轮询会暴露错误而非落定答案；窗口随转录大小增长，而长寿会话恰恰最可能被归档。修复：把每个路由的 bridge 调用包进 archiveCoordinator.runSharedMany([sessionId], async () => ...)（把运行时解析移入其中），与 transcript 路由保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| }); | ||
| }); | ||
|
|
||
| describe('isTurnResultRecordPayload', () => { |
There was a problem hiding this comment.
[Suggestion] R6-37: isTurnResultRecordPayload tests never exercise the error-state branch: every validator case uses state:'completed'; the CLI transcript-scan tests also only use completed records; Session.test.ts asserts error-state writes against a mocked recorder that never runs the validator. Probe-verified: a mutant rejecting state:'error' payloads keeps the full chatRecordingService suite, all acpAgent turn_status tests, and all Session 'records an error' tests green. — Concrete cost: both consumers then break: the read path (acpAgent.ts findSettledTurnResult skips invalid records) drops persisted error records — after a daemon restart GET turn-status returns 404 prompt_not_found for a turn that formally failed; and the strict write path (sessionTurnResultRecord) rejects the bridge's persisted error terminals with invalidParams — bridge-authoritative error outcomes are never durably recorded at all. (Mitigation: the integration test 'keeps a dispatched deadline error across daemon restart' exercises the strict error path end-to-end, so a wholesale-rejection mutant is not entirely unpinned at the integration tier.) Fix: add validator cases for the error branch: { promptId, state: 'error', endedAt, error: { message: 'boom' } } → true; state:'error' with no error object → false; state:'completed' carrying an error object → false; optionally one CLI scan test returning an error-state record end-to-end.
中文说明
isTurnResultRecordPayload 的测试从未覆盖 error 状态分支：校验器的所有用例都用 state:'completed'；CLI 转录扫描测试也只使用 completed 记录；Session.test.ts 对 error 状态写入的断言走的是从不运行校验器的 mock 记录器。探针验证：拒绝 state:'error' 载荷的变异下，chatRecordingService 全套件、全部 acpAgent turn_status 测试、全部 Session 'records an error' 测试仍全绿。具体代价：两个消费方都会坏：读取路径（acpAgent.ts findSettledTurnResult 跳过非法记录）会丢弃持久化的 error 记录——守护进程重启后，对一个正式失败的 turn 的 GET turn-status 返回 404 prompt_not_found；严格写入路径（sessionTurnResultRecord）会以 invalidParams 拒绝 bridge 持久化的 error 终态——bridge 权威的 error 结果完全无法持久记录。（缓解：集成测试 'keeps a dispatched deadline error across daemon restart' 端到端走过了严格 error 路径，因此整体拒绝的变异在集成层并非完全无约束。）修复：为 error 分支补充校验器用例：{ promptId, state: 'error', endedAt, error: { message: 'boom' } } → true；state:'error' 无 error 对象 → false；state:'completed' 携带 error 对象 → false；可选再加一个端到端返回 error 状态记录的 CLI 扫描测试。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| turnResult: terminalStatusRecord(status), | ||
| }) | ||
| .then(() => undefined); | ||
| pendingEntry.terminalPersistence.catch(() => {}); |
There was a problem hiding this comment.
[Suggestion] R6-39: Persistence failures for undispatched-prompt turn records are swallowed by an empty .catch(() => {}) with no diagnostic, unlike every other failure path in this function (writeServeDebugLine at 1386/8614/8636/8830/9533). flushPromptTerminals (~1453-1471) additionally never awaits the write at all during teardown. — Concrete cost: if sessionTurnResultRecord persistently fails for queued-prompt terminals (child build lacks the method, degraded channel, recording disabled), the overlay masks it while the session lives; after daemon restart, polling for those turns silently misses history — with zero log trail to diagnose why persisted turn_result records are missing despite formal terminals having been published.
| pendingEntry.terminalPersistence.catch(() => {}); | |
| pendingEntry.terminalPersistence.catch((err) => { | |
| writeServeDebugLine( | |
| `turn-result persistence failed for prompt ${pendingEntry.promptId} (session ${entry.sessionId}): ${extractErrorMessage(err)}`, | |
| ); | |
| }); |
中文说明
未派发 prompt 的 turn 记录持久化失败被空 .catch(() => {}) 吞掉、无任何诊断，与本函数其他所有失败路径不同（1386/8614/8636/8830/9533 处均有 writeServeDebugLine）。flushPromptTerminals（约 1453-1471）在清理期间还完全不 await 该写入。具体代价：若 sessionTurnResultRecord 对排队 prompt 的终态持续失败（子进程构建缺少该方法、通道降级、录制被禁用），会话存活期间 overlay 会掩盖问题；守护进程重启后，对这些 turn 的轮询会静默缺失历史——没有任何日志线索可解释为何正式终态已发布、持久化的 turn_result 记录却缺失。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Post-fix verification for
The exact historical lookup contract remains bounded by the documented system scan window of 10 pages x 500 transcript records. |
…20a9 # Conflicts: # docs/developers/qwen-serve-protocol.md # packages/acp-bridge/src/bridge.ts # packages/cli/src/serve/server/telemetry-catalog.test.ts # packages/cli/src/serve/server/telemetry.test.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; 变更概要：此 PR（feat(serve): add pollable turn-status endpoints...: none — all planned checks completed within budget.; chunk 20: none — all checks I wanted to run completed within budget (~9 tool calls).; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; chunk 8: compiler typecheck of packages/acp-bridge — neither the worktree nor the parent checkout has node_modules installed ( npx tsc unavailable); type-compatibil…, and 10 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；变更概要：此 PR（feat(serve): add pollable turn-status endpoints...：none — all planned checks completed within budget.；chunk 20：none — all checks I wanted to run completed within budget (~9 tool calls).；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；chunk 8：compiler typecheck of packages/acp-bridge — neither the worktree nor the parent checkout has node_modules installed ( npx tsc unavailable); type-compatibil…，另有 10 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (text.length <= TURN_RESULT_TEXT_MAX_CHARS) { | ||
| return { text, truncated: false }; | ||
| } | ||
| return { text: text.slice(0, TURN_RESULT_TEXT_MAX_CHARS), truncated: true }; |
There was a problem hiding this comment.
[Suggestion] R6-26, still standing at this commit: truncateTurnText is implemented twice with identical semantics — here and in packages/acp-bridge/src/bridge.ts — and both copies slice UTF-16 code units, so a boundary cut can split an astral character (emoji) and leave a lone surrogate at the end of the recorded promptText/resultText. — Concrete cost: duplication — a cap or semantics change must be made in lockstep in two packages; and (verified in node) a prompt/result over 32,768 chars whose boundary character is half of a surrogate pair yields a turn_result record ending in an unpaired surrogate — malformed Unicode served through the polling endpoint and rejected by strict UTF-8/JSON consumers. Fix: share one helper (core already exports the cap) and back off one code unit when the cut lands on a high surrogate.
中文说明
R6-26 在该提交上仍然存在：truncateTurnText 有两份语义完全相同的实现——此处与 packages/acp-bridge/src/bridge.ts——且都按 UTF-16 码元切片，边界处可能把一个天文字符（emoji）切成两半，使录制的 promptText/resultText 末尾残留孤立代理项。具体代价：一是重复——上限或语义变更必须在两个包同步修改；二是（已在 node 中验证）超过 32,768 字符且边界字符恰为代理对一半的 prompt/结果，会产生以未配对代理项结尾的 turn_result 记录——轮询端点向外提供畸形 Unicode，严格的 UTF-8/JSON 消费者会直接拒绝。修复：共用一个辅助函数（core 已导出上限常量），并在切点落在高代理项时回退一个码元。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| await this.#deliverUpdate(update); | ||
| } | ||
|
|
||
| async #deliverUpdate( |
There was a problem hiding this comment.
[Suggestion] R6-13, still standing at this commit: #deliverUpdate's captureResultText = true parameter is a dead switch — declared and read (if (captureResultText)) but set by no caller. Exactly three call sites exist (:2702 rewriter callback, :5251 sendUpdate, :6004 #emitAgentDiagnosticMessage); none passes a third argument, so the branch is always taken. — Concrete cost: an unreachable branch plus a knob that invites a future caller to silently skip turn-result capture without anyone noticing during review (the skip looks identical to the default path from the call site), contrary to the repo's Simplicity-First rule. Drop the parameter and the guard.
中文说明
R6-13 在该提交上仍然存在：#deliverUpdate 的 captureResultText = true 参数是死开关——声明并读取（if (captureResultText)）但没有任何调用方设置过它。全部三个调用点（:2702 rewriter 回调、:5251 sendUpdate、:6004 #emitAgentDiagnosticMessage）都不传第三个参数，分支恒为真。具体代价：一个不可达分支，加上一个会诱导未来调用方静默跳过 turn-result 捕获的旋钮（从调用点看与默认路径无法区分），违背本仓库的 Simplicity-First 原则。建议删除该参数与守卫。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| commitOutput(rewritten: string): void { | ||
| if (this.contextTurns <= 0) return; |
There was a problem hiding this comment.
[Suggestion] R6-35, still standing at this commit: nothing pins the after-delivery re-check that gates commitOutput, and the re-check itself is over-broad — a rewrite already delivered to the client is silently never committed when the rewrite timeout fires during the delivery await. — Failure scenario (probe-confirmed with ordinary latencies: timeout 200ms, rewrite 150ms, delivery 120ms): the rewritten update WAS delivered but commitOutput was called 0 times; the probe flips green with a generation-only post-delivery guard. Pre-PR the history push happened unconditionally inside rewrite() before emission, so delivered-but-unrecorded could not occur. Cost: the next turn's rewrite prompt lacks the "Previous rewrite output" entry the system prompt instructs to build on → the rewriter may repeat content the user already saw. After await sendUpdate(...) succeeds, gate the commit on generation only.
中文说明
R6-35 在该提交上仍然存在：没有任何测试固化 commitOutput 之前的「送达后复查」，且该复查本身过宽——当改写超时恰好在等待送达期间触发时，已经送达给客户端的改写会被静默地永不提交。失败场景（以普通时延探针实证：超时 200ms、改写 150ms、送达 120ms）：改写更新确实送达了，但 commitOutput 被调用 0 次；把送达后的守卫改为仅检查 generation 后探针翻绿。本 PR 之前历史推送在 rewrite() 内部、发送之前无条件执行，不存在「已送达但未记录」的状态。代价：下一 turn 的改写提示词缺少系统提示要求据此避免重复的「上一轮改写结果」条目 → 改写器可能重复用户已经看过的内容。建议在 await sendUpdate(...) 成功后仅以 generation 作为提交守卫。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (this.contextTurns <= 0) return; | ||
| this.outputHistory.push(rewritten); |
There was a problem hiding this comment.
[Suggestion] The relocated storage guard was inverted from if (this.contextTurns > 0) { push } to if (this.contextTurns <= 0) return; — for non-numeric contextTurns values the two conditions are both false, so values the old code never stored for are now pushed and never trimmed. — Failure scenario (probe-confirmed): loadRewriteConfig casts the raw messageRewrite settings JSON with no validation, so a typo like contextTurns: 'All' survives the constructor (=== 'all' is case-sensitive, ?? 1 keeps the string); with 'All', outputHistory grew on every commitOutput (expected 0, received 3) while never being read (the read guard > 0 is false for the same value) — unbounded retention of dead data for long-lived daemon sessions, breaking the deleted comment's invariant ("0 means the history is never read, so store nothing"). Normalize contextTurns once in the constructor, or mirror the read condition exactly (if (!(this.contextTurns > 0)) return;).
中文说明
迁移后的存储守卫由 if (this.contextTurns > 0) { push } 反转为 if (this.contextTurns <= 0) return;——对非数字的 contextTurns 取值，两个条件都为假，于是旧代码从不存储的取值现在会被不断 push 且永不裁剪。失败场景（探针实证）：loadRewriteConfig 对原始 messageRewrite 设置 JSON 直接强转、无任何校验，contextTurns: 'All' 这样的笔误会原样进入构造函数（=== 'all' 区分大小写，?? 1 保留字符串）；取 'All' 时每次 commitOutput 都会使 outputHistory 增长（期望 0，实际 3），而该历史从不被读取（读取守卫 > 0 对同一取值为假）——长寿命 daemon 会话中死数据无界驻留，破坏了被删除注释所述不变量（「0 表示历史从不被读取，因此不存储」）。建议在构造函数中一次性归一化 contextTurns，或让存储守卫与读取条件完全一致（if (!(this.contextTurns > 0)) return;）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| }); | ||
| }); | ||
|
|
||
| describe('normalizeTurnResultError', () => { |
There was a problem hiding this comment.
[Suggestion] The normalizeTurnResultError tests cover only the throwing-getter fallback and the string-length bounds; the numeric-.code coercion branch (chatRecordingService.ts:621-623) is asserted nowhere. — Failure scenario (probe-confirmed): deleting the numeric branch ships 76/76 green, and the proposed one-line test fails against the deletion (expected undefined to be '-32603'). Numeric JSON-RPC codes are a real input class in this PR's own code — the sibling extractErrorCode has the identical coercion AND its own numeric test; both production call sites (bridge.ts:1330, Session.ts:5477) feed untrusted error objects, so a dropped branch silently persists turn_result records with error.code absent, degrading code-keyed consumers. Add expect(normalizeTurnResultError({ message: 'boom', code: -32603 }).code).toBe('-32603');.
中文说明
normalizeTurnResultError 的测试只覆盖了 getter 抛错的回退与字符串长度边界；数字 .code 强转分支（chatRecordingService.ts:621-623）没有任何断言。失败场景（探针实证）：删除该数字分支全绿（76/76），而按建议补上的一行测试在删除后失败（expected undefined to be '-32603'）。数字 JSON-RPC 码在本 PR 自己的代码里就是真实输入类别——兄弟工具 extractErrorCode 有同样的强转且自带数字用例；两个生产调用点（bridge.ts:1330、Session.ts:5477）都喂入不可信错误对象，删除该分支会静默持久化缺失 error.code 的 turn_result 记录，使按 code 分支的消费者退化。建议补充 expect(normalizeTurnResultError({ message: 'boom', code: -32603 }).code).toBe('-32603');。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge summary — PR #8682 ← origin/mainRoot cause
Textual, not semanticBoth sides made independent additions to one import list; neither modified the same logic. Resolution keeps both sides: SESSION_ARTIFACT_PERSISTENCE_VERSION,
SESSION_TRANSCRIPT_MAX_LIMIT, // from main (#8933)
TURN_RESULT_CODE_TEXT_TRUNCATED, // from this PR
TURN_RESULT_TEXT_MAX_CHARS,
normalizeTurnResultError,
TrustGateError,All four symbols are confirmed used in the merged file and exported by What is load-bearing
Semantic overlaps reviewed (auto-merged): #8884 widened Could not verifyNo build/typecheck/tests were run, per this workflow. Only the conflicted file was edited. The auto-merged 中文说明根因： 纯文本冲突，非语义冲突：双方只是各自在同一导入列表中追加符号。解决方式是两边全部保留（见上方代码块）。已确认四个符号在合并后的文件中均被使用，且 core 包均有导出。 关键点：四个导入缺一不可——PR 的三个符号用于有界 turn-result 捕获（bridge.ts 约 L1293/1334/1606）， 已复查的语义交集（自动合并、无冲突标记）：#8884 给 未能验证：本流程不运行构建/类型检查/测试，且只允许修改冲突文件。自动合并的 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: pinning the exact vitest expect.poll default timeout value in the installed node_modules (4 greps, inconclusive; judged immaterial per point 5 above).; You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget., and 8 more.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：pinning the exact vitest expect.poll default timeout value in the installed node_modules (4 greps, inconclusive; judged immaterial per point 5 above).；You are review agent reverse-audit — Reverse audit agen...：none — all checks above were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.，另有 8 条。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| publishPromptTerminal( | ||
| entry, | ||
| pendingEntry, | ||
| { kind: 'error', err }, | ||
| { persistIfDispatched: true }, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] For a dispatched prompt that errors AFTER the child already settled its own turn_result, this persistIfDispatched write appends a second record that shadows the child's richer one. The child settles record A (with the partial resultText captured by #settleTurnRecording('error', …)) then throws; this path publishes with persistIfDispatched: true, appending record B (same promptId/state/error, no resultText) after A via the same writer. findSettledTurnResult scans tail-first and returns B; matchesPersistedTerminal matches (identical normalized error) and deletes the overlay — so polling permanently loses the partial resultText that exists in the transcript, and every such turn leaves a duplicate turn_result pair. — Concrete cost: reachable on model-stream failures after partial streaming and on live-end-cleanup failures after a complete answer; it defeats the design doc's own enrichment promise ("persisted prompt/result text enriches the bridge terminal"). The safety-net case this write exists for is transport failure with a dead child that wrote nothing. Fix direction: take the bridge-side write for dispatched errors only when the child's record cannot have landed (e.g. transport-class errors), or have the scan/merge prefer the earliest record per promptId so the child's content-bearing record survives.
中文说明
[Suggestion] child 已落定自己的 turn_result 之后才出错的 dispatched prompt，会由此处的 persistIfDispatched 写入追加第二条记录并遮蔽 child 那条更完整的记录：child 落定记录 A（含 #settleTurnRecording('error', …) 捕获的部分 resultText）后抛出；此路径以 persistIfDispatched: true 发布，经同一写入器在 A 之后追加记录 B（相同 promptId/state/error、无 resultText）。findSettledTurnResult 从尾部扫描返回 B；matchesPersistedTerminal 匹配（相同的归一化 error）并删除 overlay——于是轮询永久丢失 transcript 中已存在的部分 resultText，每个这样的 turn 都会留下一对重复的 turn_result。— 具体代价：部分流式之后的模型流失败、完整回答之后的 live-end-cleanup 失败均可触发；违背设计文档自己的富化承诺（「持久化的 prompt/result 文本会富化 bridge 终态」）。这条写入真正服务的兜底场景是 transport 失败且 child 已死、什么都没写。修复方向：仅在 child 记录不可能落盘时（如 transport 类错误）才做 bridge 侧写入；或让扫描/合并对同一 promptId 优先取最早的记录，使 child 的含内容记录存活。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| typeof terminalError.code !== 'string' || | ||
| terminalError.code.length === 0 |
There was a problem hiding this comment.
[Suggestion] Runtime validation requires a non-empty string terminalError.code, but the shared contract type PromptCancelRequest['terminalError'] (= TurnResultErrorPayload from chatRecordingService.ts) declares code?: string as optional, and the design doc states "a required message plus an optional code" — a type-valid, doc-conformant {message}-only request is rejected wholesale with invalidParams BEFORE the abort, so the targeted prompt keeps executing. — Failure scenario: latent today (the only current sender — the bridge's deadline path — always sets code: 'prompt_deadline_exceeded'), but any future/SDK caller coding against the exported bridgeTypes contract sends terminalError with only message; the child throws invalidParams instead of cancelling, the daemon gets an error instead of {cancelled: true}, and the prompt runs to completion. Fix: reconcile contract and validation — give the terminal error a dedicated shape with code: string required, or drop the code requirement and only attach code when present. Related (different site, ledger R7-21, still standing): the sessionTurnResultRecord handler at acpAgent.ts:10651-10656 pairs RPC code -32021 with errorKind: 'session_writer_unavailable', contradicting SESSION_WRITER_RPC_CODES (-32021 = session_writer_lost, -32023 = session_writer_unavailable).
中文说明
[Suggestion] 运行时校验要求 terminalError.code 为非空字符串，但共享契约类型 PromptCancelRequest['terminalError']（即 chatRecordingService.ts 的 TurnResultErrorPayload）将 code?: string 声明为可选，设计文档也写明「message 必填、code 可选」——一个类型合法且符合文档的仅含 {message} 的请求会在 abort 之前被整体以 invalidParams 拒绝，目标 prompt 继续执行。— 失败场景：当前为潜在问题（现有唯一发送方——bridge 的 deadline 路径——总是设置 code: 'prompt_deadline_exceeded'），但任何按导出的 bridgeTypes 契约编码的未来/SDK 调用方只带 message 发送 terminalError 时，child 会抛 invalidParams 而不是执行取消，daemon 收到错误而非 {cancelled: true}，prompt 执行到底。修复：使契约与校验一致——为 terminal error 定义 code: string 必填的专用结构，或去掉 code 要求、仅在存在时附带。另（不同位置，台账 R7-21，仍未解决）：acpAgent.ts:10651-10656 的 sessionTurnResultRecord handler 把 RPC 码 -32021 与 errorKind: 'session_writer_unavailable' 配对，与 SESSION_WRITER_RPC_CODES（-32021 = session_writer_lost、-32023 = session_writer_unavailable）矛盾。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // from the bridge's pending list; settled outcomes (completed / cancelled | ||
| // / error) from the bridge terminal overlay and persisted `turn_result` | ||
| // transcript records. | ||
| app.get('/session/:id/turns/current', (req, res) => { |
There was a problem hiding this comment.
[Suggestion] The two new turn-status routes have no multi-workspace owner-routing tests; a wiring mistake would ship green. Both routes resolve via resolveLiveSessionRuntime, and this PR's protocol-doc change adds them to the multi-workspace owner-routed list with 403/404/500 semantics — but packages/cli/src/serve/multi-workspace-sessions.test.ts adds no coverage for them, while every sibling owner-routed route (GET/DELETE /session/:id/pending-prompts, mid-turn-messages including untrusted_workspace at :2553 and ambiguous_session_owner at :2622) has dedicated owner-routing tests there. — Failure scenario: on a daemon with multi_workspace_sessions, if these routes were wired to the wrong runtime resolver (or lose the owner-routing wiring in a future refactor), polling a session owned by a non-primary runtime 404s or reads the wrong runtime and nothing goes red: server.test.ts exercises the routes only against a single-runtime fake bridge, and the integration tests run a single-workspace daemon. Fix: add the two routes to multi-workspace-sessions.test.ts alongside the pending-prompts cases — secondary-runtime owner resolves, missing session → 404, duplicate/ambiguous owner fails closed.
中文说明
[Suggestion] 两个新的 turn-status 路由缺少多工作区 owner-routing 测试，接线错误可以在全绿中合入。两个路由都通过 resolveLiveSessionRuntime 解析，本 PR 的协议文档改动也已把它们列入多工作区 owner-routed 列表（含 403/404/500 语义）——但 packages/cli/src/serve/multi-workspace-sessions.test.ts 未为它们添加任何覆盖，而同样走 owner-routing 的兄弟路由（GET/DELETE /session/:id/pending-prompts、mid-turn-messages，含 :2553 的 untrusted_workspace 与 :2622 的 ambiguous_session_owner）在那里都有专门的 owner-routing 测试。— 失败场景：在启用 multi_workspace_sessions 的 daemon 上，如果这两个路由接错了 runtime 解析器（或未来重构丢失 owner-routing 接线），轮询非主 runtime 拥有的会话会 404 或读错 runtime，而没有任何测试变红：server.test.ts 只用单 runtime 假 bridge 测试这两个路由，集成测试也只运行单工作区 daemon。修复：在 multi-workspace-sessions.test.ts 中仿照 pending-prompts 用例补充这两个路由——次 runtime owner 正常解析、会话不存在 → 404、重复/歧义 owner fail-closed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| } | ||
| } | ||
|
|
||
| commitOutput(rewritten: string): void { |
There was a problem hiding this comment.
[Suggestion] The diff moved the outputHistory mutation out of rewrite() into caller-driven commitOutput() and migrated every history-dependent test to the rewriteAndCommit helper — EXCEPT 'should not accumulate failed rewrites in history' (LlmRewriter.test.ts:337-356), which still calls plain rewrite() and is now vacuous: rewrite() can no longer mutate history, so its assertion passes unconditionally for the regression class its name describes. No middleware-level test fills the gap (nothing tests rewrite → null ⇒ commitOutput not called, which guards the middleware's if (!rewritten) return; line). — Failure scenario: a future regression committing falsy rewrite output (dropping the middleware's if (!rewritten) return;, or calling commitOutput unconditionally — commitOutput has no emptiness guard and pushes '') pollutes outputHistory; subsequent rewrite prompts carry an empty "previous rewrite output" block, and per the rewrite prompt's context-continuity rule the rewriter can suppress real user-facing content — with the vacuous test and the whole suite staying green. Fix: retarget the orphaned test — e.g. a middleware test where rewriter.rewrite resolves null and expect(rewriter.commitOutput).not.toHaveBeenCalled().
中文说明
[Suggestion] 本 diff 把 outputHistory 的写入从 rewrite() 移入调用方驱动的 commitOutput()，并把所有依赖历史的测试迁移到 rewriteAndCommit 辅助函数——唯独漏掉 'should not accumulate failed rewrites in history'（LlmRewriter.test.ts:337-356）：它仍然调用裸 rewrite()，如今已是空转测试——rewrite() 不再可能修改历史，因此对测试名所描述的回归类，该断言无条件通过。中间件层面也没有测试补位（没有任何测试验证 rewrite → null ⇒ commitOutput 不被调用，而这正是中间件 if (!rewritten) return; 一行的防线）。— 失败场景：未来某个回归提交了空的改写输出（删掉中间件的 if (!rewritten) return;，或无条件调用 commitOutput——commitOutput 没有空值防护，会 push ''），outputHistory 被污染；后续改写 prompt 会携带空的「上一轮改写结果」块，按改写 prompt 的上下文连续性规则，改写器可能压制真正面向用户的内容——而空转测试与整个测试套件全绿。修复：改造该孤儿测试——例如新增中间件测试：rewriter.rewrite resolve null 时 expect(rewriter.commitOutput).not.toHaveBeenCalled()。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const attempts = strictTerminalPersistences.get(entry); | ||
| if (!attempts) return; |
There was a problem hiding this comment.
[Suggestion] shutdown() skips the persistence await that close/kill guarantee. flushPromptTerminals(e, 'daemon_shutdown', …) (~bridge.ts:11294) starts a strict sessionTurnResultRecord persistence for every queued (never-dispatched) prompt, but nothing awaits those attempts before Promise.allSettled(channels.map(ci => ci.channel.kill())) kills the child the write is addressed to — while closeSession/killSession block on exactly these attempts via cancelQueuedPromptsBeforeTeardown (this code), tested by the queuedPersistenceIdx < childCloseIdx ordering assertions. — Failure scenario (probe-confirmed at this commit): daemon shutdown/restart while a prompt is queued: the record call reaches the child, kill starts while the write is unsettled, the bridge logs persist_failed error=agent channel closed mid-request, and shutdown resolves with the record never written — the client observed turn_error{code:'daemon_shutdown'} over SSE, but after restart + session/load, polling that promptId returns prompt_not_found/idle, contradicting the design doc's "settled outcomes survive daemon restarts" (the close/kill tests in this very block pin that guarantee; shutdown lacks it). Flip-verified: awaiting the attempts before kill gates kill until the write completes. Fix: in shutdown(), after flushPromptTerminals, collect the entries' pending strictTerminalPersistences attempts and await them with a bounded budget (e.g. Promise.allSettled raced against a short timer) before channel.kill(); add a test asserting a queued prompt's record lands across bridge.shutdown(). If the skip is deliberate best-effort, state that in the design doc's persistence section.
中文说明
[Suggestion] shutdown() 跳过了 close/kill 所保证的持久化等待。flushPromptTerminals(e, 'daemon_shutdown', …)（约 bridge.ts:11294）会为每个排队（未 dispatch）prompt 启动 strict sessionTurnResultRecord 持久化，但在 Promise.allSettled(channels.map(ci => ci.channel.kill())) 杀死写入的目标 child 之前，没有任何代码等待这些尝试——而 closeSession/killSession 正是通过 cancelQueuedPromptsBeforeTeardown（即本段代码）阻塞等待这些尝试的，并有 queuedPersistenceIdx < childCloseIdx 顺序断言测试。— 失败场景（已在该提交上探针证实）：有 prompt 排队时 daemon 关闭/重启：记录调用到达 child，kill 在写入未落定时开始，bridge 记录 persist_failed error=agent channel closed mid-request，shutdown 在记录从未写入的情况下 resolve——客户端曾通过 SSE 看到 turn_error{code:'daemon_shutdown'}，但重启 + session/load 后轮询该 promptId 得到 prompt_not_found/idle，与设计文档「已落定结果在 daemon 重启后存活」矛盾（本代码块对应的 close/kill 测试固化了该保证，shutdown 缺失）。翻转验证：在 kill 之前等待这些尝试后，kill 被门控直到写入完成。修复：在 shutdown() 的 flushPromptTerminals 之后，收集各 entry 的 strictTerminalPersistences 待决尝试，以有界预算等待（如 Promise.allSettled 与短计时器竞速）再 channel.kill()；补充测试断言排队 prompt 的记录能跨越 bridge.shutdown() 落盘。若该跳过是有意的 best-effort，请在设计文档的持久化章节写明。
— qwen3.8-max via Qwen Code /review (v0.21.10)
…20a9 # Conflicts: # packages/cli/src/acp-integration/session/Session.ts
|
Updated the PR to This round fixed two PR-introduced lifecycle regressions:
Verification on the pushed head:
A full local |
Scope correction: bounded turn lookup, not an exactly-once job-result storeBefore pushing the final scope correction, I want to make the contract explicit. The requirement of this PR is: within the configured transcript/scan retention bound, a caller can query a turn by The intended guarantees are:
The previous round drifted beyond that contract: it added a second daemon-side transcript writer after the ACP child had already exited, including writer-lease acquisition, transcript scanning, append/release work, shutdown waiting, and restore barriers. That was scope corrosion rather than a requirement of promptId lookup. It also caused concrete lifecycle regressions: synchronous shutdown callbacks were delayed, and restore admission could yield before reserving a caller-supplied session id. I have therefore prepared a scope-narrowing change that removes that direct crash/shutdown backfill and its recovery barriers, while retaining normal turn recording, bounded promptId lookup, live crash terminal delivery, and explicit close/kill persistence. Local verification of the narrowed contract is green:
I will commit and push this scope correction next. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 5: none — did not run the test suite itself (test execution was not required to validate the hunks; correctness was verified against the implementation source).; chunk 15: could not execute the new tests — neither the review worktree nor the parent checkout has node_modules installed, and a monorepo dependency install exceeded t…; This PR adds a daemon turn-status polling endpoint to qwe...: none — all planned checks completed within budget.; chunk 12: the tail of the 'strictly records a bridge-owned pre-dispatch turn result' test body (beyond runAcpAgent(mockConfig, ) falls in the next chunk; its setup por…; This PR adds a daemon turn-status polling endpoint to qwe...: full integration-tests/cli/qwen-serve-streaming.test.ts real-daemon E2E not executed (requires bundle + fake-server boot); the component/bridge-level equivale…, and 4 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
[Critical] R9-16 (still-standing blocker 3758881655, crash/shutdown leg): teardown terminals for undispatched prompts are not durable on crash/shutdown — flushPromptTerminals publishes queued prompts' cancelled terminals into the volatile overlay only (the child is dead, so the strict child-side write cannot complete), and HEAD commit 6b44312 removed the daemon-side persistTerminalResultsAfterChannelExit backfill, so after restart those promptIds return 404 prompt_not_found permanently — while the design doc asserts 'Settled outcomes survive daemon restarts' with no crash/shutdown carve-out. The close/kill leg of the original blocker is fixed; only the crash/shutdown leg stands. Unanchorable to a diff hunk.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 5：none — did not run the test suite itself (test execution was not required to validate the hunks; correctness was verified against the implementation source).；chunk 15：could not execute the new tests — neither the review worktree nor the parent checkout has node_modules installed, and a monorepo dependency install exceeded t…；This PR adds a daemon turn-status polling endpoint to qwe...：none — all planned checks completed within budget.；chunk 12：the tail of the 'strictly records a bridge-owned pre-dispatch turn result' test body (beyond runAcpAgent(mockConfig, ) falls in the next chunk; its setup por…；This PR adds a daemon turn-status polling endpoint to qwe...：full integration-tests/cli/qwen-serve-streaming.test.ts real-daemon E2E not executed (requires bundle + fake-server boot); the component/bridge-level equivale…，另有 4 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
[Critical] R9-16 (still-standing blocker 3758881655, crash/shutdown leg): teardown terminals for undispatched prompts are not durable on crash/shutdown — flushPromptTerminals publishes queued prompts' cancelled terminals into the volatile overlay only (the child is dead, so the strict child-side write cannot complete), and HEAD commit 6b44312 removed the daemon-side persistTerminalResultsAfterChannelExit backfill, so after restart those promptIds return 404 prompt_not_found permanently — while the design doc asserts 'Settled outcomes survive daemon restarts' with no crash/shutdown carve-out. The close/kill leg of the original blocker is fixed; only the crash/shutdown leg stands. Unanchorable to a diff hunk.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await expect | ||
| .poll(() => turnStatus(session.sessionId, running.promptId)) | ||
| .toMatchObject({ |
There was a problem hiding this comment.
[Suggestion] R9-2: This expect.poll omits the timeout option and runs with vitest's 1000 ms default poll budget, while every other poll in this file passes { timeout: 30_000 } / { timeout: 60_000 }.
Failure scenario: measured against this repo's vitest 3.2.4: a poll with no timeout whose condition becomes true at ~1.5s fails at 1008ms ("Matcher did not succeed in time"); adding { timeout: 5000 } makes the identical condition pass — the default budget is 1000ms and integration-tests/vitest.config.ts sets no override. This poll waits for the genuinely async accepted→running daemon transition under pool: 'forks'/maxForks: 4 where each fork spawns its own daemon + fake server; a single >1s event-loop stall fails the poll — a spurious red retry: 2 only masks (and triples runtime when it fires).
| await expect | |
| .poll(() => turnStatus(session.sessionId, running.promptId)) | |
| .toMatchObject({ | |
| await expect | |
| .poll(() => turnStatus(session.sessionId, running.promptId), { timeout: 30_000 }) | |
| .toMatchObject({ |
中文说明
这个 expect.poll 没有传 timeout 选项，将使用 vitest 默认的 1000 毫秒轮询预算，而本文件其他所有 poll 都传了 { timeout: 30_000 } / { timeout: 60_000 }。
失败场景：在本仓库的 vitest 3.2.4 上实测：未传 timeout 的 poll 在条件约 1.5 秒后才为真时，会在 1008ms 失败（"Matcher did not succeed in time"）；加上 { timeout: 5000 } 后同样条件通过——默认预算为 1000ms，且 integration-tests/vitest.config.ts 没有覆盖配置。这个 poll 等待的是真正的异步 accepted→running daemon 状态转换，运行环境是 pool: 'forks'/maxForks: 4，每个 fork 各自启动 daemon + 假服务器；一次超过 1 秒的事件循环停顿就会让 poll 失败——retry: 2 只能掩盖偶发红灯（且触发时运行时间变为三倍）。同一模式的另外两处（本测试文件 1368、1381 行附近）同理。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await expect | ||
| .poll(() => turnStatus(session.sessionId, queued.promptId)) | ||
| .toMatchObject({ | ||
| promptId: queued.promptId, | ||
| state: 'queued', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R9-2: This expect.poll omits the timeout option and runs with vitest's 1000 ms default poll budget, while every other poll in this file passes { timeout: 30_000 } / { timeout: 60_000 }.
Failure scenario: measured against this repo's vitest 3.2.4: the default poll budget is 1000ms (integration-tests/vitest.config.ts sets no override); a poll whose condition becomes true after ~1s fails with "Matcher did not succeed in time" — a spurious red retry: 2 only masks. This poll's queued state is set closer to the HTTP response than the running case, so the practical risk here is lower, but the 1s budget is still inconsistent with the rest of the suite.
| await expect | |
| .poll(() => turnStatus(session.sessionId, queued.promptId)) | |
| .toMatchObject({ | |
| promptId: queued.promptId, | |
| state: 'queued', | |
| }); | |
| await expect | |
| .poll(() => turnStatus(session.sessionId, queued.promptId), { timeout: 30_000 }) | |
| .toMatchObject({ | |
| promptId: queued.promptId, | |
| state: 'queued', | |
| }); |
中文说明
这个 expect.poll 没有传 timeout 选项，将使用 vitest 默认的 1000 毫秒轮询预算，而本文件其他所有 poll 都传了 { timeout: 30_000 } / { timeout: 60_000 }。
失败场景：在本仓库的 vitest 3.2.4 上实测，默认轮询预算为 1000ms（integration-tests/vitest.config.ts 无覆盖配置）；条件在约 1 秒后才为真的 poll 会以 "Matcher did not succeed in time" 失败——retry: 2 只能掩盖偶发红灯。这个 poll 的 queued 状态比 running 场景更早在 HTTP 响应附近设置，实际风险较低，但 1 秒预算仍与套件其余部分不一致。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| 'agent_launch_prompt', | ||
| 'file_history_snapshot', | ||
| 'session_source', | ||
| 'turn_result', |
There was a problem hiding this comment.
[Suggestion] R9-8: No test in this diff gates the 'turn_result' KNOWN_RECORD_SUBTYPES registration — reverting that line keeps all 79 session-transcript-reader tests green (measured by the test-efficacy probe).
Failure scenario: if a future change deletes or typos the 'turn_result' entry, records with subtype turn_result flowing through the transcript-records validation path start emitting unknown_record_or_part diagnostics, and nothing in this diff's tests fails — the registration ships ungated. The new rewind-branch test actually exercises filtering in unchanged session-transcript-reader.ts code.
Suggested fix: add a test asserting turn_result records pass subtype validation without an unknown_record_or_part diagnostic, colocated with transcript-records.ts.
中文说明
本 diff 中没有任何测试为 'turn_result' 的 KNOWN_RECORD_SUBTYPES 注册把关——把这一行还原后，全部 79 个 session-transcript-reader 测试仍为绿（测试有效性探针实测）。
失败场景：如果未来某次改动删除或拼错了 'turn_result' 条目，经过 transcript-records 校验路径的 turn_result 子类型记录将开始产生 unknown_record_or_part 诊断，而本 diff 的测试没有任何一个会失败——该注册在无把关的情况下上线。新增的 rewind 分支测试实际上覆盖的是未改动的 session-transcript-reader.ts 中的过滤逻辑。
建议修复：在 transcript-records.ts 旁新增测试，断言 turn_result 记录通过子类型校验且不产生 unknown_record_or_part 诊断。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| } catch (error) { | ||
| if (terminalBeforeRead) return terminalBeforeRead; | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R9-11: getSessionTurnStatus's failed-lookup path falls back to the pre-await overlay snapshot (terminalBeforeRead) and never re-reads the overlay, unlike the success path which deliberately re-reads after the await (the plan's Task 2 re-read requirement).
Failure scenario: probe-verified on both route shapes at this commit: a /turns/current poll on an idle session, a prompt admitted and settled while the sessionTurnStatus ext-method is in flight, then the lookup fails (transport close / timeout). First poll → 500 (agent transport closed) even though the overlay holds the just-published terminal; an immediate retry succeeds. A poller treating non-404 errors as fatal reports a spurious failure. The design doc promises the overlay "remain[s] available while the daemon session is resident" but never argues for ignoring terminals published during a failed lookup.
| } catch (error) { | |
| if (terminalBeforeRead) return terminalBeforeRead; | |
| throw error; | |
| } | |
| } catch (error) { | |
| const terminalAfterRead = | |
| promptId !== undefined | |
| ? entry.terminalTurnStatuses.get(promptId) | |
| : latestTerminalTurnStatus(entry); | |
| const fallback = terminalAfterRead ?? terminalBeforeRead; | |
| if (fallback) return fallback; | |
| throw error; | |
| } |
中文说明
getSessionTurnStatus 的查询失败路径回退到 await 之前捕获的 overlay 快照（terminalBeforeRead），从不重新读取 overlay；而成功路径在 await 之后特意重读（即计划中 Task 2 的"await 后重读"要求）。
失败场景：在本提交上以探针实测（两种路由形态均复现）：对 idle 会话发起 /turns/current 轮询，期间一个 prompt 被接纳并落定，同时 sessionTurnStatus ext-method 正在飞行中，随后查询失败（transport 关闭/超时）。第一次轮询 → 500（agent transport closed），尽管 overlay 中已持有刚发布的终态；立即重试即可成功。把非 404 错误视为致命的轮询方会报告一次假失败。设计文档承诺 overlay "在 daemon 会话驻留期间保持可用"，但从未论证应忽略失败查询期间发布的终态。（建议代码块中的变量名以当前作用域为准，必要时微调。）
— qwen3.8-max via Qwen Code /review (v0.21.10)
| terminalError === null || | ||
| typeof terminalError.message !== 'string' || | ||
| terminalError.message.length === 0 || | ||
| typeof terminalError.code !== 'string' || |
There was a problem hiding this comment.
[Suggestion] R8-6: Runtime validation requires a non-empty string terminalError.code, but the shared contract keeps code optional — a producer honoring the documented contract is rejected with invalidParams (round-8 finding, still stands).
Failure scenario: the PROMPT_CANCEL_METHOD validation rejects any terminal error without a non-empty string code, while PromptCancelRequest declares terminalError?: TurnResultErrorPayload with code?: string, and the design doc says "a required message plus an optional code" — a future producer (or the SDK) honoring the contract fails validation. Related mismatch still standing: the sessionTurnResultRecord handler throws RequestError(-32021, ..., { errorKind: 'session_writer_unavailable' }), but SESSION_WRITER_RPC_CODES maps -32021 → session_writer_lost and -32023 → session_writer_unavailable (session-writer-lease.ts:69-73) — the numeric code and the kind disagree.
Suggested fix: make validation match the contract (accept a missing/empty code), or tighten the contract and design doc to require code; and align the -32021 errorKind with SESSION_WRITER_RPC_CODES.
中文说明
运行时校验要求 terminalError.code 为非空字符串，但共享契约中 code 是可选的——遵循文档契约的生产者会被以 invalidParams 拒绝（第 8 轮发现，仍然存在）。
失败场景：PROMPT_CANCEL_METHOD 校验会拒绝任何缺少非空字符串 code 的终态错误，而 PromptCancelRequest 声明的是 terminalError?: TurnResultErrorPayload（其中 code?: string），设计文档也写着"必需的 message 加可选的 code"——未来遵循契约的生产者（或 SDK）会校验失败。仍然成立的相关不匹配：sessionTurnResultRecord 处理器抛出 RequestError(-32021, ..., { errorKind: 'session_writer_unavailable' })，但 SESSION_WRITER_RPC_CODES 的映射是 -32021 → session_writer_lost、-32023 → session_writer_unavailable（session-writer-lease.ts:69-73）——数字码与 kind 不一致。
建议修复：让校验与契约一致（接受缺失/空 code），或者收紧契约与设计文档、改为要求 code；并让 -32021 的 errorKind 与 SESSION_WRITER_RPC_CODES 对齐。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| }); | ||
|
|
||
| it('lets a removed running prompt deadline release the FIFO when cancel wedges', async () => { | ||
| const handle = wedgeChannel(); |
There was a problem hiding this comment.
[Suggestion] R6-23: The "when cancel wedges" test never wedges the cancel — wedgeChannel() sets no cancelImpl, so the fake's cancel resolves immediately and the cancelForwardDeadline leg of forwardRunningPromptCancel is never load-bearing (round-6 finding, no author reply, still stands).
Failure scenario: lets a removed running prompt deadline release the FIFO when cancel wedges relies on the cancel-forward never resolving; but the fake's cancel resolves at once, so the scenario the test name promises is not exercised — a regression in the wedged-cancel path ships green.
Suggested fix: give the fake a wedged cancel for this test (e.g. cancelImpl: () => new Promise(() => {})) so the cancel-forward genuinely hangs and the deadline release is what unblocks the FIFO.
中文说明
"when cancel wedges" 测试实际上从未让 cancel 卡住——wedgeChannel() 没有设置 cancelImpl，fake 的 cancel 会立即 resolve，forwardRunningPromptCancel 的 cancelForwardDeadline 分支从未真正承重（第 6 轮发现，无作者回复，仍然存在）。
失败场景：lets a removed running prompt deadline release the FIFO when cancel wedges 依赖 cancel 转发永不 resolve；但 fake 的 cancel 立即 resolve，测试名称承诺的场景并未被真正执行——wedged-cancel 路径上的回归会在绿灯下合入。
建议修复：为本测试给 fake 一个卡住的 cancel（如 cancelImpl: () => new Promise(() => {})），让 cancel 转发真正挂起、由 deadline 释放来解锁 FIFO。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const followupGate = new Promise<void>((resolve) => { | ||
| releaseFollowup = resolve; | ||
| }); | ||
| it('falls back to persisted turn_result records once settled', async () => { |
There was a problem hiding this comment.
[Suggestion] R9-12: Settled-fall-through while a newer turn is live is unpinned — no getSessionTurnStatus test settles one prompt while another keeps pendingPromptList non-empty (still-standing blocker 3736679273; author deferred as non-blocking).
Failure scenario: falls back to persisted turn_result records once settled and the sibling overlay tests query only after sendPrompt settles (live list empty). A live.length > 0 → return undefined mutant in the settled-fall-through path ships green: a client polling a settled promptId while a newer prompt runs would get undefined/404 instead of the settled outcome, and no test notices.
Suggested fix: add a test: settle prompt A, keep prompt B pending (blocking FIFO), and assert getSessionTurnStatus(sessionId, context, 'A') still returns A's settled status.
中文说明
"已有更新 turn 存活时对已落定 turn 的回退"没有被测试钉住——没有任何 getSessionTurnStatus 测试在一个 prompt 已落定而另一个 prompt 仍使 pendingPromptList 非空时查询（仍成立的阻断项 3736679273；作者曾按非阻断处理）。
失败场景：falls back to persisted turn_result records once settled 与相邻的 overlay 测试都只在 sendPrompt 落定后（live 列表为空时）查询。在已落定回退路径上植入 live.length > 0 → return undefined 的突变仍能为绿：客户端在更新的 prompt 运行期间轮询一个已落定的 promptId 时，会得到 undefined/404 而不是已落定结果，且没有测试能发现。
建议修复：新增测试：落定 prompt A，让 prompt B 保持排队（阻塞 FIFO），断言 getSessionTurnStatus(sessionId, context, 'A') 仍返回 A 的已落定状态。
— qwen3.8-max via Qwen Code /review (v0.21.10)
…rced A forced kill could be preempted when strict queued-terminal persistence failed, leaving the child running, and an errored dispatched turn could append a thin bridge error record over the child's persisted main answer. Force kill now converges after a persistence failure while close keeps the strict retry contract, and dispatched terminals are written only by the owning child. Docs now state the bounded durability boundary: persistence across restarts requires successful recording, and rewind with recording disabled does not retain historical promptIds.
|
Pushed 334af79 addressing the remaining blocking findings, with the durability boundary stated explicitly instead of adding more recovery machinery. R9-16 (crash/shutdown leg): resolved by documentation boundary, by design. The crash/shutdown leg is real but is intentionally not fixed with a daemon-side backfill writer. This endpoint is a bounded turn-status lookup — the requirement is that an admitted promptId can poll its main answer while the daemon session is resident, plus across normal restarts when recording succeeded — not an exactly-once result store. Restoring R9-1 and R8-5: fixed in code (see thread replies): forced kill converges after a persistence failure while close keeps the strict retry contract, and dispatched terminals are written only by the owning child so a lean bridge record can no longer shadow the persisted main answer. R6-3: documented boundary (see thread reply): with recording disabled or failed, historical promptIds are not retained across rewind; retaining them would need a persistent turn-index/branch ledger, which is out of scope here. Verification on the new head (local, real bundled daemon): acp-bridge 596/596; Session + acpAgent 1027/1027; typecheck across all workspaces; build + bundle; targeted E2E "turn result polling" 3/3 (promptId returns the final main answer, tool-boundary filtering, truncation code) and "turn result blocker regressions" 6/6 (dispatched deadline error across daemon restart, queued cancellation + prior completion across restart, retained results vs rewound turn, rewrite diagnostics, dropped late rewrite, provider failures as pollable errors). CI is running on the new head; the earlier Ubuntu failure was a transient actionlint download error ( Per the ~5-review-rounds guideline, the remaining unresolved Suggestions (R6-x and follow-ups, ~68 threads) are deliberately not addressed in this PR to stop diff bloat; they are recorded here so nothing is silently dropped, and can be picked up in a follow-up issue if maintainers want them. |
|
Scope correction and latest Critical disposition (334af79)\n\nThe implementation is intentionally bounded to promptId-based turn lookup and the user's final main answer. It is not an exactly-once task-result store and does not guarantee that an unpersisted result survives every crash/storage failure.\n\nAccepted and fixed:\n- Force kill is no longer blocked when strict queued-terminal persistence fails. Close remains fail-closed and retryable.\n- Dispatched error results are persisted only by the owning child, preventing a later lean bridge record from shadowing the child's richer resultText.\n\nVerified boundary, documented rather than expanded:\n- Persisted turn results survive normal daemon restart only when session recording successfully appends them.\n- The live terminal overlay is process-local.\n- Unexpected child exit before append, daemon hard stop, disabled recording, and permanent storage failure have no cross-restart guarantee.\n- Rewind follows the persisted active transcript branch. With recording disabled/failed, historical promptIds are not retained across rewind. Adding selective retention here would duplicate branch-membership state and could expose abandoned-branch results.\n\nFresh verification on exact head 334af79:\n- bridge focused regressions: 7 passed\n- Session turn-result/Goal tests: 58 passed\n- ACP turn-result tests: 10 passed\n- real daemon turn-status/restart E2E: 5 passed\n- npm run build, npm run typecheck, npm run bundle, npm run lint: passed\n\nAfter the prior crash-backfill removal and this scope clarification, I am not taking remaining Suggestions that expand durability or add recovery/state machinery in this PR. |
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] R6-3 (bridge.ts:10554) — rewind clears the terminal overlay; with recording disabled/failed, historical promptIds are not retained across rewind. The mechanism still exists in code at this commit, now explicitly carved out in docs/design/daemon-turn-status-endpoint.md ('With recording disabled or failed, historical promptIds are therefore not retained across rewind'); whether the documented boundary is an acceptable resolution is a maintainer scope decision this re-check cannot settle from code.
- [Critical] R9-16 (body-level) — teardown terminals for undispatched prompts are not durable on crash/shutdown (flushPromptTerminals publishes into the volatile overlay only). Still true in code at this commit; the author resolved it in 334af79 as an explicitly documented boundary ('unexpected child exit before its record is appended, daemon hard stop, disabled recording, or permanent storage failure does not gain a cross-restart durability guarantee from the bridge'); accepting that boundary versus adding recovery machinery is a maintainer scope decision this re-check cannot settle from code.
Not explored to full depth (tool budget reached): "变更概要：为 daemon（qwen serve）新增可轮询的 turn 状态端点（按 promptId 查询与…": none material — the one check I did not finish (child recording-service queue ordering relative to a rewind truncation) is disclosed inside finding 2's confiden…; "You are review agent reverse-audit — Reverse audit agent…": none — all checks completed within budget.; chunk 10: none — all checks I started completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget., and 11 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 2 条（原文未翻译，列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："变更概要：为 daemon（qwen serve）新增可轮询的 turn 状态端点（按 promptId 查询与…"：none material — the one check I did not finish (child recording-service queue ordering relative to a rewind truncation) is disclosed inside finding 2's confiden…；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks completed within budget.；chunk 10：none — all checks I started completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.，另有 11 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| !pendingEntry.dispatched && | ||
| pendingEntry.terminalPersistence === undefined | ||
| ) { | ||
| startStrictTerminalPersistence( |
There was a problem hiding this comment.
[Critical] Graceful shutdown loses queued-prompt turn_result records: bridge.shutdown() (the SIGTERM/SIGINT path) calls flushPromptTerminals for every session, which starts strict sessionTurnResultRecord persistence for undispatched queued prompts via this gate — but then kills the channels without ever awaiting strictTerminalPersistences. closeSession/killSession deliberately drain exactly these writes via cancelQueuedPromptsBeforeTeardown (bounded by terminalPersistenceTimeoutMs); shutdown does not.
Failure scenario: SIGTERM arrives while a session has queued prompts → the strict-write RPCs are issued and the child is killed in the same shutdown sequence, so the appends routinely lose the race (probe-verified in this review: shutdown resolved in ~31 ms, before a 150 ms simulated writer finished; the record RPC never reached the agent). After a normal graceful restart those promptIds return 404 prompt_not_found although SSE subscribers were just told the turn ended. The documented loss boundary (design doc) lists only unexpected child exit / hard stop / disabled recording / storage failure — graceful shutdown is the canonical "normal daemon restart" and is not in that list.
Suggested fix: in shutdown, after flushPromptTerminals for each entry, await the entry's outstanding strictTerminalPersistences attempts (bounded by the existing terminalPersistenceTimeoutMs), mirroring cancelQueuedPromptsBeforeTeardown. (Dispatched/running prompts at shutdown are a second leg of this problem — no durable record is even started for them because of the !dispatched gate — worth handling together.)
中文说明
[Critical] 优雅关停会丢失排队 prompt 的 turn_result 记录：bridge.shutdown()（SIGTERM/SIGINT 路径）对每个会话调用 flushPromptTerminals，该调用会经此处的门控为未派发的排队 prompt 启动严格的 sessionTurnResultRecord 持久化——但随后直接杀死 channel，从不等待 strictTerminalPersistences。closeSession/killSession 都会通过 cancelQueuedPromptsBeforeTeardown（以 terminalPersistenceTimeoutMs 为上限）刻意排空这些写入；shutdown 没有。
失败场景：会话中还有排队 prompt 时收到 SIGTERM → 严格写入 RPC 刚发出，child 就在同一次关停流程中被杀死，append 几乎必然输掉竞争（本次评审探针证实：shutdown 约 31 ms 即返回，早于 150 ms 的模拟写入器完成；记录 RPC 根本没到达 agent）。正常优雅重启后，这些 promptId 返回 404 prompt_not_found，而 SSE 订阅方刚刚才收到该 turn 的终态。设计文档列出的丢失边界仅包括 child 意外退出 / 硬停 / 未开记录 / 存储永久失败——优雅关停是「正常重启」的典型形态，却不在该列表中。
建议修复：在 shutdown 中对每个条目 flushPromptTerminals 之后，等待该条目未完成的 strictTerminalPersistences（以现有 terminalPersistenceTimeoutMs 为上限），与 cancelQueuedPromptsBeforeTeardown 保持一致。（shutdown 时处于 dispatched/running 状态的 prompt 是该问题的第二条腿——由于 !dispatched 门控，根本不会为它们启动任何持久化——值得一并处理。）
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const terminalError = getTerminalPromptAbortError(pendingSend.signal); | ||
| settlement = | ||
| result.stopReason === 'cancelled' && terminalError !== undefined | ||
| ? { state: 'error', error: terminalError } |
There was a problem hiding this comment.
[Suggestion] A terminal abort (deadline) arriving after the model loop already returned cancelled settles the persisted turn_result as error here, but the prompt RPC still resolves {stopReason:'cancelled'} — the loop-top/post-tool abort checks test only .aborted, not the reason, and didPromptFail is never set on this branch. Probe-verified in this review: the divergence reproduces deterministically with a deadline abort fired between tool runs, and the implied fix flips it.
Failure scenario: the same deadline event yields three different RPC surfaces depending on timing (before execution → throws the structured terminal error; mid-stream provider throw → rejects 'Request was aborted.'; between iterations → resolves cancelled), while the pollable record says error{prompt_deadline_exceeded} in all three. This between-iterations variant is untested, so a refactor of either surface can silently widen the split between what the RPC caller and the polling client believe about the same turn.
Suggested fix: if the split is intentional (RPC = transport outcome, record = semantic state), add a regression test for the deadline-fires-between-iterations case asserting both resolves.toEqual({ stopReason: 'cancelled' }) and the error record; otherwise set promptFailure = terminalError; didPromptFail = true; here so this branch throws like the other terminal paths.
中文说明
[Suggestion] 当模型循环已返回 cancelled 之后才到达的终态中止（deadline）会在此处把持久化的 turn_result 落定为 error，但 prompt RPC 仍然 resolve {stopReason:'cancelled'}——循环顶部/工具后的中止检查只测 .aborted、不测原因，且该分支从不设置 didPromptFail。本次评审已用探针证实：在两次工具运行之间触发 deadline 中止可确定性地复现该分歧，且隐含修复可使其翻转。
失败场景：同一个 deadline 事件随时序不同会产生三种不同的 RPC 表面（执行前 → 抛出结构化终态错误；流中 provider 抛出 → reject 'Request was aborted.'；两次迭代之间 → resolve cancelled），而可轮询记录在三种情形下都是 error{prompt_deadline_exceeded}。这个「两次迭代之间」的变体没有测试，因此对任一表面的重构都可能悄悄拉大 RPC 调用方与轮询客户端对同一 turn 的认知分歧。
建议修复：若该分裂是有意的（RPC = 传输结果，记录 = 语义状态），为「deadline 落在两次迭代之间」补充回归测试，同时断言 resolves.toEqual({ stopReason: 'cancelled' }) 与 error 记录；否则在此处设置 promptFailure = terminalError; didPromptFail = true;，使该分支像其他终态路径一样抛出。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| - A bridge deadline error remains authoritative if the agent later persists | ||
| a generic cancelled record; persisted `resultText` may still enrich the | ||
| error response. |
There was a problem hiding this comment.
[Suggestion] This bullet asserts deadline-error authority unqualified, but authority holds only while the overlay entry is live: for dispatched prompts the bridge-side deadline terminal exists only in the 64-entry overlay (strict persistence is gated !dispatched), and the fallback connection.cancel used when PROMPT_CANCEL_METHOD is unsupported carries no terminalError — so the agent can settle a generic cancelled record.
Failure scenario: after a daemon restart, or once 64 newer terminals evict the overlay entry, polling flips from error{prompt_deadline_exceeded} to state:'cancelled' for the same promptId. This contradicts the unqualified wording here, and a client or conformance test written from this bullet misclassifies the turn post-restart. (The sibling bullet at line 22 — the 404 condition — has the same overlay omission; it is already discussed in an existing thread.)
| - A bridge deadline error remains authoritative if the agent later persists | |
| a generic cancelled record; persisted `resultText` may still enrich the | |
| error response. | |
| - A bridge deadline error remains authoritative while the daemon is live and | |
| the terminal remains in the overlay; after a daemon restart or overlay | |
| eviction the persisted record alone is reported, so a generic cancelled | |
| record surfaces as `state: 'cancelled'`. Persisted `resultText` may still | |
| enrich the error response. |
中文说明
[Suggestion] 该条目无条件地断言 deadline 错误的权威性，但权威性只在 overlay 条目存活期间成立：对 dispatched prompt，bridge 侧的 deadline 终态只存在于 64 条目的 overlay 中（严格持久化以 !dispatched 为门控），且当 PROMPT_CANCEL_METHOD 不被支持时回退使用的 connection.cancel 不携带 terminalError——因此 agent 可能落定一条普通的 cancelled 记录。
失败场景：daemon 重启后，或 64 个更新终态把该 overlay 条目淘汰后，同一 promptId 的轮询结果会从 error{prompt_deadline_exceeded} 翻转为 state:'cancelled'。这与本条目的无条件表述矛盾，按本条目实现的客户端或一致性测试会在重启后误判该 turn。（第 22 行的同类条目——404 条件——同样遗漏了 overlay；已在现有讨论串中。）
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (generation !== this.generation || rewriteSignal.aborted) return; | ||
| this.rewriter.commitOutput(rewritten); |
There was a problem hiding this comment.
[Suggestion] The post-delivery commit gate conflates turn-discard with the rewrite AbortSignal.timeout(timeoutMs): a rewrite that resolves just inside the timeout whose delivery crosses the timeout boundary is delivered to the client yet never committed to outputHistory. Probe-verified in this review (timeoutMs 150, delivery ~300 ms): the chunk was delivered once, the signal aborted mid-delivery, and commitOutput was never called on the unmodified code; the fix below flips the probe, and both real suites stay 37/37 green with it (discard protection comes from discardTurn() bumping generation, which this guard keeps).
Failure scenario: with the default 30 s budget (a user-configured smaller timeoutMs widens the race), a rewrite resolving near the boundary and delivering across it reaches this guard with rewriteSignal.aborted === true; commitOutput is skipped although the user visibly received the rewritten chunk. The next turn's rewrite context ([上一轮改写结果]) then omits content the user already saw, so the "don't repeat" instruction has no record to work from and the rewriter repeats content. Pre-diff this edge did not exist (the history push happened inside rewrite() before delivery).
| if (generation !== this.generation || rewriteSignal.aborted) return; | |
| this.rewriter.commitOutput(rewritten); | |
| if (generation !== this.generation) return; | |
| this.rewriter.commitOutput(rewritten); |
中文说明
[Suggestion] 交付后的 commit 门控把「turn 丢弃」与改写自身的 AbortSignal.timeout(timeoutMs) 混为一谈：一个在超时前一刻完成、但交付过程跨越超时边界的改写，会被交付给客户端，却永远不会 commit 进 outputHistory。本次评审已用探针证实（timeoutMs 150、交付约 300 ms）：在未修改的代码上，改写块被交付了一次、信号在交付中途被超时中止、commitOutput 从未被调用；下面的修复可使探针翻转，且应用该修复后两个真实测试套件仍为 37/37 全绿（丢弃保护来自 discardTurn() 递增 generation，该门控予以保留）。
失败场景：在默认 30 s 预算下（用户配置更小的 timeoutMs 会放大该竞争），接近边界完成、交付却跨越边界的改写到达此门控时 rewriteSignal.aborted === true；尽管用户已实际看到改写内容，commitOutput 仍被跳过。下一 turn 的改写上下文（[上一轮改写结果]）因此缺失用户已看到的内容，「不要重复」指令失去依据，改写器会重复输出。此边界在改动前不存在（历史推入原先发生在 rewrite() 内部、交付之前）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| removePendingPromptCalls.push({ sessionId, promptId }); | ||
| return removePendingPromptImpl(sessionId, promptId); | ||
| }, |
There was a problem hiding this comment.
[Suggestion] This PR converted AcpSessionBridge.removePendingPrompt to Promise<{removed: boolean}> and converted the sibling fake method removeMidTurnMessage to async in this same fake — but the fake's removePendingPrompt still returns the bare sync value of the sync-typed removePendingPromptImpl, violating the interface it is checked against (probe-verified TS2322 through the package's own tsc --noEmit).
Concrete cost: it escapes CI only because src/serve/server.test.ts is on the tsconfig exclude list under TODO(5691) — the moment that TODO lands and the file is re-included, npm run typecheck fails on this line. Until then the sync-typed fake cannot model in-flight removals for interleaving tests (exactly the ordering the bridge tests now pin). The same unconverted sync member exists at multi-workspace-sessions.test.ts:853, masked by the as unknown as FakeBridge double-cast.
Suggested fix:
async removePendingPrompt(sessionId, promptId) {
removePendingPromptCalls.push({ sessionId, promptId });
return removePendingPromptImpl(sessionId, promptId);
},and widen removePendingPromptImpl to (...) => { removed: boolean } | Promise<{ removed: boolean }>, mirroring the removeMidTurnMessage conversion this diff already performed; apply the same to multi-workspace-sessions.test.ts.
中文说明
[Suggestion] 本 PR 把 AcpSessionBridge.removePendingPrompt 改为返回 Promise<{removed: boolean}>，并在同一个 fake 中把相邻的 removeMidTurnMessage 改成了 async——但 fake 的 removePendingPrompt 仍然返回同步类型的 removePendingPromptImpl 的裸同步值，违反其所实现的接口（已用包内 tsc --noEmit 探针证实 TS2322）。
具体代价：它之所以能逃过 CI，仅因为 src/serve/server.test.ts 在 TODO(5691) 名下位于 tsconfig 的 exclude 列表——一旦该 TODO 落地、文件被重新纳入，npm run typecheck 就会在这一行失败。在此之前，同步类型的 fake 无法为交错测试建模「移除进行中」的状态（而这正是 bridge 测试现在钉住的时序）。同样的未转换同步成员还存在于 multi-workspace-sessions.test.ts:853，被 as unknown as FakeBridge 双重断言掩盖。
建议修复：把该方法改为 async（见上方代码块），并把 removePendingPromptImpl 放宽为 (...) => { removed: boolean } | Promise<{ removed: boolean }>，与本 diff 已完成的 removeMidTurnMessage 转换保持一致；对 multi-workspace-sessions.test.ts 做同样处理。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
I opened #9080 as a clean replacement built from the latest The retained contract is intentionally narrow: live-Session polling by exact This does not claim permanent or exactly-once result storage, crash/shutdown transcript backfill, deleted-JSONL recovery, offline workspace scanning, strict close/kill persistence barriers, rewind indexing, or message-rewrite integration. #8682 established the problem and many useful correctness cases, but repeated review rounds expanded it into lifecycle durability, crash recovery, rewind bookkeeping, rewrite-pipeline changes, and recurring conflict resolution. Continuing to patch that branch made the implementation and review surface drift beyond the requested polling feature. #9080 restarts from the validated contract and carries forward only the correctness fixes required by that boundary. |
4 participants
What this PR does
Adds two read-only endpoints to the daemon HTTP API for polling a session turn's lifecycle state and result:
GET /session/:sessionId/turns/:promptIdreports the status of the turn submitted under that prompt id, andGET /session/:sessionId/turns/currentreports the session's current turn (the running prompt, else the queued FIFO head, else the most recent settled turn, or idle). Responses cover the full state machine —idle,queued,running,completed,cancelled,error— with the turn's prompt text, timing milestones, stop reason, an error payload for failed turns, and the result text the agent streamed during the turn (tool output and thoughts excluded). Live queue state always wins over settled history, and settled outcomes are read from the session's persisted transcript, so they survive daemon restarts without the daemon holding per-turn state in memory. Both endpoints enforce the same client authorization as the prompt submission route, and long prompt/result text is capped at 32 KiB with paired truncation flags so responses stay bounded. A design document describing the response contract, persistence model, and failure semantics is included.Why it's needed
External services that drive the daemon (IM bridges, automation, editor integrations) need to know when a submitted turn finished and what it produced. Today the only way to observe that is holding the daemon's long-lived event stream for the entire turn lifetime, which is fragile for short-lived or reconnecting consumers and makes fire-and-forget submission impossible: a client that posts a prompt and disconnects cannot later ask "did that turn finish, and what did it return?". A pollable turn status lets such clients submit and then poll for the terminal state — including user cancellation and errors — at their own pace, with no long-lived connection required.
Reviewer Test Plan
How to verify
qwen serve) and create a session.POST /session/:id/promptand note the returned prompt id.GET /session/:id/turns/:promptId(with the same client-id header used for submission): expectrunningwhile the turn executes, thencompletedwithstopReason,resultText, and timing fields. An unknown prompt id yields404 prompt_not_found; a foreign client id yields400 invalid_client_id.cancelledwith the partial result text preserved.packages/acp-bridgeandpackages/cli.Expected vs observed: every listed transition was observed live against a real model endpoint during E2E (report posted as a comment); the focused suites pass locally.
Evidence (Before & After)
N/A (non-UI change; HTTP behavior covered by the verification steps and the E2E report comment)
Tested on
Environment (optional)
Local daemon from the bundled CLI (
npm run build && npm run bundle) against a real model endpoint, plus focused vitest suites for the bridge / session / agent / server layers.Risk & Scope
idle/404 prompt_not_found(documented failure semantics) instead of erroring.Linked Issues
Fixes #8680
中文说明
这个 PR 做了什么
为 daemon HTTP API 新增两个只读接口，用于轮询会话 turn 的生命周期状态与结果：
GET /session/:sessionId/turns/:promptId查询指定 prompt id 对应 turn 的状态；GET /session/:sessionId/turns/current查询会话当前 turn（运行中的 prompt，否则是排队 FIFO 队首，再否则是最近一条已落定的 turn，或 idle）。响应覆盖完整状态机——idle、queued、running、completed、cancelled、error——并包含 turn 的 prompt 文本、各时间点、终止原因、失败 turn 的错误载荷，以及该 turn 期间 agent 流式输出的结果文本（不含工具输出与思考）。实时队列状态始终优先于已落定历史；已落定结果从会话持久化 transcript 读取，因此 daemon 重启后依然可查，且 daemon 内存中不保留任何 per-turn 状态。两个接口执行与 prompt 提交路由相同的客户端授权；超长 prompt/结果文本按 32 KiB 截断并附带成对的截断标志，保证响应有界。附带一份设计文档，描述响应契约、持久化模型与失败语义。为什么需要
驱动 daemon 的外部服务（IM 桥接、自动化、编辑器集成）需要知道提交的 turn 何时结束、产出了什么。目前唯一的观测方式是在整个 turn 生命周期内保持 daemon 的长连接事件流——这对短生命周期或会重连的消费方很脆弱，也使得"提交即走"的场景无法实现：客户端提交 prompt 后断开，就无法再问"那个 turn 结束了吗，结果是什么？"。可轮询的 turn 状态让这类客户端提交后按自己的节奏轮询终态——包括用户取消与错误——无需保持长连接。
评审者测试计划
如何验证
qwen serve）并创建会话。POST /session/:id/prompt提交 prompt，记录返回的 prompt id。GET /session/:id/turns/:promptId（带与提交相同的 client-id 头）：turn 执行中应得到running，随后是带stopReason、resultText与时间字段的completed。未知 prompt id 返回404 prompt_not_found；外来 client id 返回400 invalid_client_id。cancelled且保留部分结果文本。packages/acp-bridge与packages/cli中的 turn-status 套件。预期与实际：E2E 阶段所有列出的状态转换均在真实模型端点上实测观察到（报告以评论形式附上）；定向套件本地全部通过。
前后对比证据
N/A（非 UI 变更；HTTP 行为由上述验证步骤与 E2E 报告评论覆盖）
测试环境
运行环境（可选）
本地 daemon 使用打包后的 CLI（
npm run build && npm run bundle）对接真实模型端点，另跑 bridge / session / agent / server 各层的定向 vitest 套件。风险与范围
idle/404 prompt_not_found（文档化的失败语义）而非报错。关联 Issue
Fixes #8680