Repository navigation
fix(web-shell): keep mid-turn messages the daemon rejects at idle - #11289
Conversation
Verification report —
|
| Gate | Result |
|---|---|
npm run build |
exit 0 |
npm run bundle |
exit 0 |
npm run typecheck |
exit 0 |
packages/web-shell full suite |
279 files / 6420 tests passed |
packages/acp-bridge full suite |
35 files / 1959 tests passed |
packages/sdk-typescript daemon client |
415 tests passed |
packages/cli src/serve/server.test.ts |
1226 tests passed |
npx eslint / npx prettier --check on every changed file |
clean |
The four queued-prompt suites that carry this change: 223 tests passed.
Live daemon wire capture
A real serve process built from this commit (isolated QWEN_RUNTIME_DIR, random port, fresh thread session) refused three mid-turn inserts at idle — plain text, the same messageId repeated, and an image-only message. Each returned exactly {"accepted":false,"reason":"session_idle"} under a strict deep-equality assertion, and the mid-turn queue, the settled/promoted rings and the pending-prompt list were all empty afterwards, so the refusal transfers no ownership. The daemon logged rejected id "pr11289-idle-text": session idle and rejected id "pr11289-idle-image": session idle, then stopped cleanly on SIGTERM. Full transcript in the PR description.
The new tests are load-bearing (mutation-checked)
Each behaviour this PR adds was re-verified by breaking it and watching exactly one test go red:
| Mutation | Result |
|---|---|
| Removal guard inverted so an unavailable snapshot licenses removal | does not remove a cleared delayed fallback whose confirmation snapshot fails fails — removal issued once; the queued / running / started variants stay green |
| Image-only placeholder clause deleted from the matcher | binds an image-only idle fallback through the daemon placeholder text fails — message echoed and the queue row dropped |
| Duplicate-row filter deleted | drops the local duplicate when two identical idle fallbacks queue fails — 4 rows for 2 messages |
| Idle flag made unconditional instead of keyed on the stated reason | does not echo a reasonless rejection the daemon starts itself fails |
Before the last of those tests existed, that mutation left the whole suite green.
Review round
A balanced (medium) review round ran over this change: 10 finder dimensions, all 4 diff chunks covered, then 2 verification shards that ruled on every finding with executed evidence (probes and mutations, not readings). 16 findings — 3 Critical, 10 Suggestion, 3 Nice to have; 6 of them low confidence. 9 were fixed in this branch, including both high-confidence Criticals:
- the cleared-queue removal that could cancel a turn the daemon was already running when its confirmation snapshot failed or was superseded (now removal requires the snapshot to still list the prompt as queued);
- the protocol sentence that described
session_idleas a property of session state rather than of the refusal's cause, which would have led an integrator to read a missingreasonas "busy" or "old daemon".
Seven were skipped on the record, the two worth naming here:
- The daemon's pending-prompt removal is still unconditional. A removal decided from a snapshot that was accurate when read can in principle hit a prompt that started in the meantime. Closing that needs an opt-in conditional removal on the daemon side (bridge type, route and SDK), which is a separate change; this PR removes the client-side half by no longer removing on an unknown state. Worth a follow-up issue.
- Two blocks that discard an admitted prompt whose local row disappeared now exist side by side — the one this PR adds registers the id so a concurrent refresh cannot re-materialize the row, the pre-existing one does not. Unifying them changes the behaviour of a path this PR does not otherwise touch, so it was left alone; the pre-existing gap is transient (the next successful refresh drops a row the daemon no longer lists).
Not validated
- No browser was driven: the timing between the daemon's refusal and the browser's own streaming-state update is reproduced in the hook harness with stubbed SDK actions, not in a live Web Shell. No screenshots, no toast DOM, no real image decode.
- No two-client race was driven end to end, so an image-only follow-up competing with another client's image-only prompt in the same session is reasoned about rather than measured.
- The mutation-testing gate that would have measured hunk survival automatically returned
inconclusiveon every probe (its runner could not produce parseable JSON in the probe tree), so the mutation results above are hand-run. - Whole-repo
npm run lintaborts with a Node heap exhaustion on this machine (~4 GB); lint was run per changed file instead. - Windows and Linux untested locally — CI covers them.
packages/clihas 8 pre-existing failing test files on this machine (user-config and absolute-path leakage: provider list,~/.qwenpath comparisons, a timeout, status-code semantics). None is touched by this PR; the serve-routes suite that this PR does touch passes 1226/1226 in isolation.
中文摘要
以下全部在本 PR 提交 9add20f0ca 上、macOS 本地、对当前 main 执行干净 npm ci 之后运行；没有为了让结果变绿而修改任何源码或测试。
自动化门禁：npm run build、npm run bundle、npm run typecheck 均 exit 0；packages/web-shell 全套件 279 文件 / 6420 测试通过；packages/acp-bridge 35 文件 / 1959 通过；packages/sdk-typescript daemon client 415 通过；packages/cli 的 src/serve/server.test.ts 1226 通过；所有改动文件的 eslint 与 prettier --check 干净。承载本改动的 4 个队列测试文件共 223 个用例通过。
真实 daemon wire 抓取：用本提交构建的真实 serve 进程（独立 runtime 目录、随机端口、全新 thread 会话）在空闲态拒绝了三次 mid-turn 插入 —— 纯文本、同 messageId 重复、仅图片。三次都在严格深比较下等于 {"accepted":false,"reason":"session_idle"}，随后 mid-turn 队列、settled/promoted 环与 pending-prompt 列表全为空，说明拒绝不转移所有权。daemon 记录了两条 rejected id …: session idle，并以 SIGTERM 干净停止。完整记录见 PR 描述。
新增测试均经变异验证：把守卫改成"快照不可用也允许删除"、删掉图片占位文本分支、删掉重复行过滤、把空闲标记改成无条件 —— 每次都恰好有一个对应用例变红，其余保持绿。其中最后一项在这个测试出现之前，整个套件对该变异是绿的。
审查轮次：balanced（medium）轮次覆盖全部 4 个 diff chunk、10 个 finder 维度，再由 2 个验证分片用可执行证据（探针与变异，而非阅读）逐条裁决，共 16 条发现（3 Critical / 10 Suggestion / 3 Nice to have，其中 6 条低置信）。本分支修掉 9 条，含两条高置信 Critical：一是确认快照失败或被顶替时清空队列仍会取消 daemon 已在运行的回合（现在必须快照仍列为 queued 才删除）；二是协议文档把 session_idle 写成会话状态的属性而非拒绝原因，会让集成方把"缺少 reason"误读成"忙"或"老 daemon"。
7 条记录在案地跳过，其中两条值得单列：daemon 的 pending-prompt 删除仍是无条件的，彻底收口需要在 bridge 类型/路由/SDK 增加可选的条件删除，属另一个改动，本 PR 只去掉了客户端这一半；以及"丢弃已被接受但本地行已消失的提示词"现在有两处并存（本 PR 新增的那处会登记 id 以防并发刷新把行复活，既有那处不会），统一它会改变本 PR 未触及路径的行为，因此保留原状 —— 既有那处的漏洞是瞬态的。
未验证：没有驱动真实浏览器（拒绝与浏览器流式状态更新之间的时序是在 hook harness 中用桩复现的），无截图、无 toast DOM、无真实图片解码；没有端到端驱动双客户端竞态；自动化的 hunk 存活率门禁（test-efficacy）四个探针全部 inconclusive，因此上面的变异结果都是手工跑的；本机整仓 npm run lint 会 Node 堆溢出（约 4 GB），改为逐文件 lint；Windows / Linux 未在本地测试，交给 CI；packages/cli 在本机有 8 个既有失败文件（用户配置与绝对路径泄漏所致），均与本 PR 无关，本 PR 触及的 serve 路由套件单独运行 1226/1226 通过。
|
@qwen-code /takeover from 2 |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This window's round counter starts at 2 (the rounds this PR spent in review before takeover), so the Critical-only brake engages after 3 more change-producing round(s) instead of a full fresh 5. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本窗口轮次计数从 2 起算（即本 PR 托管前已进行的评审轮数），因此再经过 3 个产生改动的轮次即进入 Critical-only，而非重新计满 5 轮。移除 |
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review round addressed —
|
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix round — PR #11289All five findings from the round-2 review are addressed in Feedback pointsR2-1 (Critical, rc:3954525496) — text fallback can bind a co-client's prompt — FIXED
R2-2 (Critical, rc:3954525509) — sync-spliced row misread as a user clear — FIXEDThe branch now discriminates on the row's own provenance instead of its mere absence. R2-3 (Critical, rc:3954525517) — superseded confirmation forfeits the DELETE — FIXEDThe submit body now re-awaits until it holds a non-superseded result: R1-14 (Suggestion, rc:3954525523) — removal re-entry races a user delete/edit — FIXEDBoth halves of the guard are in place. (1) The branch returns early (settling R2-4 (Suggestion, rc:3954525532) — id-binding ignores settled/removing markers — FIXEDThe id-binding branch now computes Notes
Verification
中文说明Autofix 本轮处理 — PR #11289第 2 轮评审的全部 5 条发现已在 反馈条目R2-1（Critical，rc:3954525496）—— 文本回退可能绑到同会话其他客户端的 prompt —— 已修复
R2-2（Critical，rc:3954525509）—— 被 sync 摘除的行被误读为用户清空 —— 已修复该分支现在依据行自身的来源做区分，而不再仅凭"行不存在"。 R2-3（Critical，rc:3954525517）—— 被取代的确认快照使 DELETE 落空 —— 已修复提交体现在会重新等待，直到拿到非 superseded 的结果： R1-14（Suggestion，rc:3954525523）—— 删除重入与用户的删除/编辑竞争 —— 已修复守卫的两半都已落地。(1) 当 R2-4（Suggestion，rc:3954525532）—— 按 id 绑定不看 settled/removing 标记 —— 已修复按 id 绑定分支现在用 说明
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 5 selected review thread(s). · 已关闭全部选中的 5 条评审线程。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Address-review round — PR #11289Growth audit: Findings
No maintainer-decision items and no deferrals this round; the deferred non-Critical section and the review body's recorded probes were left untouched as instructed. No base-conflict resolution was requested. Mutation probes (applied, measured, reverted)
Verification
中文说明评审处理轮次 —— PR #11289增长审计结论： 发现处理
本轮无需维护者决定的事项，也没有延后项；按指示未触碰"延后的非 Critical"段落与评审正文记录的探针。未请求解决基线冲突。 变异探针（均已施加、实测、还原）
验证
Deferred non-Critical feedbackCritical-only mode is active: the PR's diff grew src 112 / test 702 net lines beyond this counting window's baseline (budgets: 400/400). The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback continues to flow unaffected during a growth-only engagement (the per-author batch budget applies only after 5 change-producing rounds). ( 中文说明已进入仅处理 Critical 的模式：本计数窗口内 diff 净增长已达 源码 112 / 测试 702 行（预算 400/400）。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。纯增长触发期间维护者反馈照常流动（按作者的批次预算仅在完成 5 个产生改动的轮次后生效）。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 8 selected review thread(s). · 已关闭全部选中的 8 条评审线程。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #11289Addressed all six Critical findings from the round-4 review (plus verified the earlier-round findings still hold). One commit: Round-4 findings (all Critical, all fixed)
Earlier-round findingsAll findings from rounds 1–3 were re-verified as still fixed at HEAD (code + their pinning tests: No merge conflicts ( Mutation probesEvery guard/branch added this round was verified by removing it and watching its witness fail:
All probes were reverted and the suites re-run to green. Verification
中文说明Autofix 本轮总结 — PR #11289处理了第 4 轮评审的全部 6 个 Critical 发现（并复核了前几轮发现仍然成立）。共一个提交： 第 4 轮发现（均为 Critical,均已修复）
前几轮发现第 1–3 轮的全部发现均已复核在 HEAD 上仍然修复（代码 + 各自锁定测试，包括: 无合并冲突（ 变异探针本轮新增的每个守卫/分支都通过"移除后观察见证测试失败"验证:
所有探针均已还原，套件重新跑绿。 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 23 selected review thread(s). · 已关闭全部选中的 23 条评审线程。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
⏸️ AutoFix paused by a review convergence signal: 3 consecutive automated review round(s) signalled machine-readable non-convergence (codes observed since the last maintainer response, or the window start if none: batch-fixes, root-cause-triage) — the reviewer keeps finding new defects at a rate that is not falling while the loop keeps widening the diff, so another automatic round is unlikely to converge this PR. The loop resumes once a maintainer responds on this PR (a review or comment counts, and steers the next rounds), and pauses again if the signal persists for 3 more round(s). Alternatives: split the recurring cluster into its own PR, batch the remaining fixes into one push, comment 中文说明⏸️ AutoFix 已因评审收敛信号暂停：3 轮自动评审连续发出机器可读的不收敛信号（自上次维护者响应以来观察到的信号码；若无响应则自窗口开始：batch-fixes, root-cause-triage）——评审仍在以不降的速率发现新缺陷，而循环在继续扩大 diff，再跑一轮自动修复难以收敛本 PR。维护者在本 PR 上作出回应后循环自动恢复（评论或评审均可，并将作为后续轮次的指引）；若信号再持续 3 轮会再次暂停。可选做法：把反复出问题的簇拆成独立 PR、把剩余修复攒成一批一次推送、评论 |
Verification report — real daemon + real browser, on Linux (
|
| browser bundle | daemon | |
|---|---|---|
| PR | packages/web-shell/dist built at 9f8993e6a4 |
node dist/cli.js serve built at 9f8993e6a4 |
| base | same, built at the merge base cfb173ec46 |
same, built at cfb173ec46 |
- A ~40-line OpenAI-compatible server stands in for the model, so turn timing is exact. A prompt containing
SLOWTURNnholds the turn open for n seconds. - A transparent HTTP proxy sits between the browser and each daemon. It forwards everything untouched, records the wire, and — this is the whole trick — withholds the SSE bytes from
event: turn_completeonward for a set time. That makes the race deterministic instead of a millisecond coin flip: the daemon is genuinely idle while the browser still renders the turn as running, which is exactly the ordering the PR describes. - Playwright drives the real Web Shell the daemon serves at
/(no mocks, nopage.route, noinstallMockDaemon). Every screenshot below is that browser.
Results
| # | Check | Result |
|---|---|---|
| 1 | reason: "session_idle" on the wire, text and image-only |
✅ PR states it, base omits it |
| 2 | Reported bug, end to end in a browser | ✅ fixed on PR, reproduced on base |
| 3 | Which half is load-bearing (2×2 cross matrix) | ✅ both — neither half alone fixes it |
| 4 | Queue row kept when the daemon queues the resubmission | ✅ row survives, editable and removable |
| 5 | A clear must not cancel a turn already running | ✅ no cancellation observed in any run |
| 6 | Standing Critical R5-1, against the running product | main |
| 7 | Standing Criticals R2-3, R6-3/R6-4 | ➖ not reproducible through the product; see below |
| 8 | Linux gates (PR lists Linux as untested) | ✅ all green; 4 unrelated failures, identical on base |
1. The wire
Against a live daemon, on an idle session:
# PR 9f8993e6a4
POST /session/:id/mid-turn-message → 200 {"accepted":false,"reason":"session_idle"}
POST /session/:id/mid-turn-message (image) → 200 {"accepted":false,"reason":"session_idle"}
# base cfb173ec46
POST /session/:id/mid-turn-message → 200 {"accepted":false}
POST /session/:id/mid-turn-message (image) → 200 {"accepted":false}
The '[image]' placeholder the client now binds on is not an assumption: a text-less image prompt really is rendered that way by the daemon at this commit —
GET /session/:id/pending-prompts → running '<other prompt text>'
queued '[image]' originatorClientId client_4566d30a-…
2. The reported bug, end to end
Turn started, turn_complete withheld from the browser, follow-up typed and sent inside the window.
Browser-observed wire on the PR arm:
POST /session/:id/prompt 202 (FIRST-TURN)
POST /session/:id/mid-turn-message 200 {"accepted":false,"reason":"session_idle"}
POST /session/:id/prompt 202 {"promptId":"7cdfa9c6-…"} ← resubmitted
GET /session/:id/pending-prompts 200 [{promptId:"7cdfa9c6-…", state:"running"}]
On the base arm the third line never happens: the browser raises Daemon rejected mid-turn message, the composer content is dropped, and the model never sees the message (the fake model logs one request, not two). This is the negative control — the harness discriminates the fix rather than just showing green.
3. Which half of the fix is load-bearing
The proxy can serve the static bundle from one daemon and route the API to the other, so the browser half and the daemon half can be mixed:
| browser bundle | daemon | outcome |
|---|---|---|
| base | base | ❌ toast, message lost |
| PR | base | ❌ toast, message lost |
| base | PR | ❌ toast, message lost |
| PR | PR | ✅ resubmitted, runs exactly once, no toast |
Two things fall out of this. The change is not decorative — neither half fixes the race alone. And the PR's cross-version compatibility claim holds in practice: a new client against an old daemon behaves exactly like the old client (no regression), and an old client against a new daemon is unaffected by the new field.
4. The queue row when the daemon queues the resubmission
Same window, but a second client's turn is admitted before the browser resubmits, so the daemon queues the follow-up behind it rather than starting it.
Daemon state at that moment:
GET /session/:id/pending-prompts → running SLOWTURN14 CO-CLIENT (client_a2c6dbca-…)
queued FOLLOW-UP (client_c567dbe0-…)
On the PR arm the row stays in the panel bound to that prompt, with its Delete and Edit affordances, and the message later runs exactly once. On base the message is simply gone. This is the second failure mode the PR set out to close, and it is closed.
5. Removal does not cancel a running turn
Across every run that reached a removal — a queued resubmitted row removed just before the daemon promoted it, and a row removed while a co-client turn was mid-flight — no running turn was ever cancelled. The co-client turn completed normally each time (SLOWTURN9 CO-CLIENT … Processed 10s … ACK#31), and the DELETE was issued only after the daemon's snapshot confirmed the prompt was still queued.
6. The standing review Criticals, tested against the running product
The review loop was paused at round 6 with 7 standing Criticals and a land-with-residual-risk recommendation, asking a maintainer to fill in attack surface and blast radius. All of those findings were reached by code trace or hook-level probes; none was driven through the product. So I drove them.
R5-1 — reproduces, but it is pre-existing. The finding needs the client's own activity mirror to still read idle when its resubmission body resolves. That is hard to reach because three independent channels refresh it (the SSE stream, the co-client's own pending_prompt_started, and the HTTP live-state poll). Three orderings failed to reproduce it. Starving all three at once does:
The follow-up renders as a sent message, the queue row is filtered out, and the daemon still holds the prompt queued behind another turn — precisely the outcome the finding describes. The base arm does exactly the same thing, byte for byte in behaviour. So R5-1 is a pre-existing property of main, not something this branch introduces; the PR neither closes it nor makes it worse. For the residual-risk inventory: the trigger needs a second admission inside the window and every activity signal to be late, which in production is a stalled or slow connection at that instant.
R2-3 — could not be reproduced through the product. Driven from the browser with every pending-prompts GET stalled 8s so that the flight in progress at admission time genuinely predates it: the row was absent for the duration of the induced stall, then materialised, bound, and ran exactly once. The permanent loss the finding describes ("the user's queued message disappears from the panel … nothing re-refreshes") did not occur.
R6-3 / R6-4 — could not be reproduced through the product. Attempts included delaying the DELETE by 3–6 s so the daemon promotes the prompt first, and clearing the row during the confirming window. In every run the user's message was echoed exactly once and no turn was cancelled.
These are honest negatives from a real driver, not proofs of absence: the findings are stated at hook granularity and may still be reachable by an ordering I did not construct.
7. Linux gates
The PR description lists Linux as not tested. It is now:
| Gate | Result |
|---|---|
packages/web-shell full suite |
✅ 288 files / 6753 tests |
packages/acp-bridge full suite |
✅ 36 files / 1973 tests |
packages/sdk-typescript DaemonClient.test.ts |
✅ 415 tests |
packages/cli src/serve/routes |
✅ 41 files / 1068 tests |
packages/cli src/serve (whole tree) |
|
tsc --noEmit -p packages/web-shell |
✅ exit 0 |
node esbuild.config.js (full daemon bundle) |
✅ both arms build and run |
The 4 failures are workspace-agents, workspace-memory, workspace-file-system and session-archive — all "unlink silently fails" style assertions that cannot hold when the suite runs as root. The identical four fail on the base arm, so they are environmental, not this PR.
Non-blocking observations
IMAGE_ONLY_PROMPT_TEXTduplicates a daemon constant.useQueuedPrompts.ts:382hardcodes'[image]'; the producer isbridge.ts:2473(return hasImage ? '[image]' : ''), which also spells it at:10147and:13489. They match today — I verified against the live daemon — but the client's identity check silently stops matching if the daemon's placeholder ever changes. A shared exported constant would make that impossible.- Removing a row while it still reads
Submitting…is a no-op. Reproduced twice: the click issues no DELETE, the row goes on to bind and run, and the message is echoed exactly once. Nothing is lost, but the user's Remove is silently ignored for the length of the confirmation. I did not establish whether this predates the PR for ordinary submissions; the window is short and it fails in the safe direction. - The report's "Tested on" table can be updated — Linux is now covered by the run above.
Reproducing this
Everything is scripted: a fake model server, the SSE-withholding proxy with arm / delay-midturn / delay-delete / delay-pending / hold-all / stall controls, and the Playwright drivers. Happy to attach them if useful for the next web-shell race PR.
中文说明
验证报告 —— 真实 daemon + 真实浏览器，Linux 环境（9f8993e6a4）
结论：可以合入。 在真实 Chromium 驱动真实 qwen serve daemon 的条件下，所报告的故障在 main 上可复现、在本分支上消失；改动的两半都是必需的；而评审中最响的那条未决 Critical（R5-1）在 main 上以完全相同的方式复现，因此它是既存问题，不是本 PR 引入的。
本轮是对 2026-09-07 那轮（macOS、9add20f0ca、套件与 wire 层）的补充。以下内容全部是新的：在当前 head、在 Linux（PR 描述里标注为未测试）、并且驱动的是浏览器里真实的 Web Shell 产物，而不是 jsdom 里的 hook。
验证装置
两条臂各自从源码构建、并排运行：
| 浏览器产物 | daemon | |
|---|---|---|
| PR | 9f8993e6a4 构建的 packages/web-shell/dist |
9f8993e6a4 构建的 node dist/cli.js serve |
| base | merge base cfb173ec46 构建的同一产物 |
cfb173ec46 构建的同一命令 |
- 一个约 40 行的 OpenAI 兼容服务代替真实模型，回合时长完全可控：提示词里含
SLOWTURNn就把回合撑开 n 秒。 - 浏览器与每个 daemon 之间放了一个透明 HTTP 代理。它原样转发、记录整条 wire，而关键在于把
event: turn_complete及其之后的 SSE 字节扣住一段可设定的时间。这把原本只有毫秒级的竞态变成确定性的：daemon 确实已经空闲，而浏览器仍然把回合渲染成运行中 —— 正是 PR 描述的那个时序。 - Playwright 驱动 daemon 在
/上提供的真实 Web Shell（没有 mock，没有page.route，没有installMockDaemon）。下面每张截图都出自这个浏览器。
结果总览
| # | 检查项 | 结果 |
|---|---|---|
| 1 | wire 上的 reason: "session_idle"（纯文本与纯图片） |
✅ PR 有，base 没有 |
| 2 | 所报告缺陷的端到端浏览器复现 | ✅ PR 已修复，base 上复现 |
| 3 | 哪一半在起作用（2×2 交叉矩阵） | ✅ 两半都必需，单独任一半都修不好 |
| 4 | daemon 把重投排队时队列行是否保留 | ✅ 行保留，可编辑可删除 |
| 5 | 清空不得取消已在运行的回合 | ✅ 所有运行中都未出现取消 |
| 6 | 未决 Critical R5-1 在运行态产品上 | main 上表现完全一致 |
| 7 | 未决 Critical R2-3、R6-3/R6-4 | ➖ 通过产品未能复现，说明见下 |
| 8 | Linux 门禁（PR 标注 Linux 未测试） | ✅ 全绿；4 个无关失败在 base 上完全相同 |
1. wire 层
对着活的 daemon、在空闲会话上：
# PR 9f8993e6a4
POST /session/:id/mid-turn-message → 200 {"accepted":false,"reason":"session_idle"}
POST /session/:id/mid-turn-message (图片) → 200 {"accepted":false,"reason":"session_idle"}
# base cfb173ec46
POST /session/:id/mid-turn-message → 200 {"accepted":false}
POST /session/:id/mid-turn-message (图片) → 200 {"accepted":false}
客户端新依赖的 '[image]' 占位文本不是假设：在本提交上，无文本的图片提示词在 daemon 侧确实就渲染成这个值 ——
GET /session/:id/pending-prompts → running '<另一条提示词>'
queued '[image]' originatorClientId client_4566d30a-…
2. 所报告缺陷的端到端复现
起一个回合，把 turn_complete 对浏览器扣住，在窗口内输入并发送追问。
PR 臂上浏览器观测到的 wire：
POST /session/:id/prompt 202 (FIRST-TURN)
POST /session/:id/mid-turn-message 200 {"accepted":false,"reason":"session_idle"}
POST /session/:id/prompt 202 {"promptId":"7cdfa9c6-…"} ← 重投
GET /session/:id/pending-prompts 200 [{promptId:"7cdfa9c6-…", state:"running"}]
base 臂上第三行根本不会出现：浏览器弹出 Daemon rejected mid-turn message，输入框内容被丢弃，模型从未看到这条消息（假模型只记录到一次请求，而不是两次）。这就是反向对照 —— 说明这套装置能判别修复本身，而不只是"绿的还是绿的"。
3. 哪一半在起作用
代理可以把静态产物从一个 daemon 取、把 API 打到另一个 daemon，于是浏览器半边和 daemon 半边可以任意组合：
| 浏览器产物 | daemon | 结果 |
|---|---|---|
| base | base | ❌ 报错提示，消息丢失 |
| PR | base | ❌ 报错提示，消息丢失 |
| base | PR | ❌ 报错提示，消息丢失 |
| PR | PR | ✅ 重投成功，只运行一次，无报错 |
由此得到两点。其一，这个改动不是装饰性的 —— 单独任何一半都修不好这个竞态。其二，PR 声称的跨版本兼容性在实测中成立：新客户端连旧 daemon 的行为与旧客户端完全一致（无回归），旧客户端连新 daemon 不受新字段影响。
4. daemon 把重投排队时的队列行
同一个窗口，但在浏览器重投之前先有第二个客户端的回合被接纳，于是 daemon 把追问排在它后面而不是立即启动。
该时刻的 daemon 状态：
GET /session/:id/pending-prompts → running SLOWTURN14 CO-CLIENT (client_a2c6dbca-…)
queued FOLLOW-UP (client_c567dbe0-…)
PR 臂上该行留在面板里、绑定到那个 prompt，删除与编辑按钮都在，消息稍后恰好运行一次。base 臂上消息直接没了。这正是 PR 要关掉的第二种失效模式，它确实关掉了。
5. 删除不会取消正在运行的回合
在所有走到删除动作的运行里 —— 包括在 daemon 提升它之前删掉一个已排队的重投行，以及在协同客户端回合进行中删除某一行 —— 从未出现正在运行的回合被取消。协同客户端的回合每次都正常跑完（SLOWTURN9 CO-CLIENT … Processed 10s … ACK#31），并且 DELETE 只在 daemon 快照确认该 prompt 仍是 queued 之后才发出。
6. 把未决 Critical 放到运行态产品上验
评审循环在第 6 轮暂停，留下 7 条未决 Critical 与 land-with-residual-risk 建议，要求 maintainer 填写攻击面与影响范围。那些结论全部来自代码追踪或 hook 级探针，没有一条被驱动到产品上。于是我把它们驱动了一遍。
R5-1 —— 可复现，但是既存问题。 该结论要求客户端自身的活跃度镜像在重投响应返回时仍读作 idle。这很难达到，因为有三条互相独立的通道会刷新它（SSE 流、协同客户端自己的 pending_prompt_started、以及 HTTP live-state 轮询）。三种时序都没能复现。把三条全部饿住之后复现了：
追问被渲染成已发送的消息、队列行被过滤掉，而 daemon 仍把该 prompt queued 在另一个回合之后 —— 与结论描述的结果完全一致。base 臂的行为一模一样。 所以 R5-1 是 main 上既有的性质，不是本分支引入的；本 PR 既没有关掉它，也没有让它更糟。供残余风险清单参考：触发条件需要窗口内出现第二次接纳，并且所有活跃度信号同时迟到 —— 在生产中对应那一瞬间连接卡顿或变慢。
R2-3 —— 通过产品未能复现。 从浏览器驱动，把每一次 pending-prompts GET 都拖住 8 秒，确保接纳发生时在飞的那次请求确实早于它：该行在人为拖延期间不可见，随后正常出现、绑定、恰好运行一次。结论描述的永久丢失（"用户排队的消息从面板消失……没有任何东西再刷新"）没有发生。
R6-3 / R6-4 —— 通过产品未能复现。 尝试包括把 DELETE 延迟 3–6 秒好让 daemon 先提升该 prompt，以及在确认窗口内清空该行。每次运行里用户消息都恰好回显一次，也没有回合被取消。
这些是真实驱动器给出的诚实否定结果，不是"不存在"的证明：这些结论是在 hook 粒度上表述的，仍可能由我没有构造出的某种时序到达。
7. Linux 门禁
PR 描述把 Linux 列为未测试。现在测了：
| 门禁 | 结果 |
|---|---|
packages/web-shell 全量套件 |
✅ 288 文件 / 6753 测试 |
packages/acp-bridge 全量套件 |
✅ 36 文件 / 1973 测试 |
packages/sdk-typescript DaemonClient.test.ts |
✅ 415 测试 |
packages/cli src/serve/routes |
✅ 41 文件 / 1068 测试 |
packages/cli src/serve（整棵树） |
|
tsc --noEmit -p packages/web-shell |
✅ 退出码 0 |
node esbuild.config.js（完整 daemon 打包） |
✅ 两条臂都能构建并运行 |
那 4 个失败是 workspace-agents、workspace-memory、workspace-file-system 与 session-archive，都属于"unlink 静默失败"这类断言 —— 以 root 身份跑套件时它们不可能成立。base 臂上失败的是完全相同的四个，因此是环境问题，与本 PR 无关。
非阻塞观察
IMAGE_ONLY_PROMPT_TEXT与 daemon 的常量重复。useQueuedPrompts.ts:382硬编码了'[image]'；生产方是bridge.ts:2473（return hasImage ? '[image]' : ''），同一字面量在:10147与:13489还各写了一遍。今天它们是一致的 —— 我对着活 daemon 核过 —— 但只要 daemon 侧的占位文本变了，客户端的身份判定就会静默地不再匹配。导出一个共享常量可以从根上排除这种漂移。- 在行仍显示
Submitting…时删除是空操作。 复现两次：点击不会发出 DELETE，该行随后照常绑定并运行，消息恰好回显一次。没有东西丢失，但在确认期间用户的删除被静默忽略了。我没有确认这对普通提交是否早于本 PR 就存在；窗口很短，且失败方向是安全的。 - PR 里的 "Tested on" 表可以更新 —— Linux 已由本轮覆盖。
复现方式
全部脚本化：假模型服务、带 arm / delay-midturn / delay-delete / delay-pending / hold-all / stall 控制口的 SSE 扣留代理，以及 Playwright 驱动脚本。如果对后续 web-shell 竞态类 PR 有用，我可以把它们附上。
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 2/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (7200000ms)）（第 2/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/34373353642 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head 9f8993e6. Not approving: the review posted against this same commit on 2026-09-09T10:04:08Z reported four Criticals, no commit follows it, and I re-verified each one in packages/web-shell/client/hooks/useQueuedPrompts.ts as it stands at this head. Two further Criticals from the previous round are also still unresolved and unchanged in shape.
Verified still standing at this head
R6-1 — deferred refresh continuation drops the entry guards (:899-910). The wait path is:
return inflight.promise.then(() => {
const latest = inflightRefreshRef.current;
if (latest && latest.sessionId === targetSessionId &&
isCurrentOwnerTokenRef.current(latest.ownerToken) && latest.seq > notBefore) {
return latest.promise;
}
return dispatchRefresh();
});The continuation re-checks only inflightRefreshRef; the synchronous entry guards !connected || !targetSessionId (:836-837) and latestSessionIdRef.current !== targetSessionId (:838-839) are not re-applied, and runRefresh's post-await check (:853-858) re-tests owner token and session but never connected — there is no connectedRef in this file. Because dispatchRefresh bumps the hook-global refreshRequestSeqRef at dispatch time (:842), the deferred dispatch at :909 — which can fire up to DEFAULT_FETCH_TIMEOUT_MS later, for a session the user has left — supersedes the switched-to session's in-flight GET, which then returns { status: 'superseded' } at :851-852 and never reaches syncServerQueuedPrompts.
R5-2 — the cleared-row DELETE licence omits the two start markers (:1702-1711). queuedInSnapshot consults refresh.status, settledServerPromptIdsRef and the snapshot's state === 'queued', and the gate adds only displayedServerPromptIdsRef. The sibling arm below (startedSinceSnapshot, :1820-1823) vetoes on displayedServerPromptIdsRef plus pendingStartedByPromptIdRef plus completedPromptIdsRef. A prompt start this client already recorded at :1501 therefore does not stop removePendingPrompt, which aborts a prompt the daemon is running.
R6-2 — the failed-GET arm has no settle veto and re-reads a pre-await local (:1778-1786). In the refresh.status !== 'refreshed' branch, startedOrCompleted is displayed || pendingStarted || completed with no settledServerPromptIdsRef term, unlike both sibling verdict sites (:1702-1707, :1803-1806), and if (!localMessageAppended) at :1784 reads a local captured before the confirming await at :1673 — this branch never re-consumes appendedBeforeResponsePromptIdsRef. A start echo followed by a settle and then a failed GET appends the user's message to the transcript a second time.
R6-4 — the catch path reports a false queue failure for an admitted submission (:1922-1938). syncClaimed = syncClaimedSubmittingRowIdsRef.current.delete(localId) (:1922-1923) lets the body proceed even when admissionStarted is true; the editor restore is correctly gated on !admissionStarted (:1935-1937), but reportError(error, t('queue.queueFailed')) at :1938 is unconditional. For a POST that dies after the daemon admitted, started and echoed the prompt, the user is told the queue failed about a message already on screen — where the success path at :1688 reads the same syncClaimed fact as proof of admission.
Unresolved from the previous round, unchanged at this head
- R5-1 (
:1664-1666) — the confirmation branch is still gated onlatestSessionActiveRef.current, a ref assigned in the render body at:639, so the branch that exists to confirm an idle-rejected resubmission is keyed on a mirror that lags the daemon state it is meant to correct. - R2-3 (
:894) — the single-flight share is stillif (inflight.seq > notBefore) return inflight.promise;withnotBefore = 0as the default (:834), so every caller that does not pass a fence joins whatever GET is in flight regardless of its age; the freshness term exists only at the call sites that were fenced individually.
I stopped the scan here: with confirmed standing Criticals, approval is not available and widening the investigation would not change the verdict.
CI at this head
Status rollup is SUCCESS — 25 checks passed, 27 skipped, none failed or pending. CI is not the reason for this review.
Next step
Address the four findings above (each has a measured fix arm in its thread) plus R5-1 and R2-3, or record the ones you consider out of scope in a linked follow-up issue and say so on the thread, then push and re-request review.
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 3/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (7200000ms)）（第 3/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/34388905628 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix round — PR #11289 review feedbackAll seven standing Criticals are addressed in Feedback dispositions
The Deferred to follow-upFrom
Both are recorded in Verification
中文说明Autofix 本轮处理 —— PR #11289 评审反馈七条未决 Critical 全部在 反馈处置
延后到后续来自
两条均已记录进 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 7 selected review thread(s). · 已关闭全部选中的 7 条评审线程。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action — PR #11289 (growth-audit round)This round made no code changes and no commit. The growth audit ran before any other work, as required: verdict Growth audit (required this round)The window's entire growth is one commit, Feedback triage
Failed check:
|
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review at head 536ab851. Approving: all six Criticals that were standing at the previous head are fixed in the code at this one, each verified first-hand, and the whole production delta since then is 91 lines that map one-to-one onto those findings.
The six standing findings — verified fixed at this head
All in packages/web-shell/client/hooks/useQueuedPrompts.ts:
- R6-1 (deferred refresh continuation dropped the entry guards). A
latestConnectedRefnow exists (:540, assigned beside the otherlatest-*refs at:641), the synchronous entry guard reads it (:848-849), and the continuation re-applies both guards before dispatching:if (!latestConnectedRef.current || latestSessionIdRef.current !== targetSessionId) return { status: 'skipped' }(:911-918), withconnectedremoved from the callback's dependency list (:934). A wait that outlives the session or the connection can no longer bump the hook-global sequence against the session the user switched to. - R2-3 (single-flight join had no freshness term by default). The default fence is now
notBefore = refreshRequestSeqRef.current(:846) instead of0, so a caller that passes nothing cannot join — and sync from — a GET dispatched before its own state change; the join still requiresinflight.seq > notBefore(:906). This changes the default rather than fencing consumers one call site at a time, which is what the finding asked for. - R5-1 (confirmation branch gated on the render-body activity mirror). The gate is now
if (prompt.resubmittedAfterIdleRejection && !localMessageAppended)(:1686) with the comment recording why: the mirror lags the daemon state this confirmation exists to correct. - R5-2 (cleared-row DELETE licence omitted the two start markers).
startedSinceSnapshotis computed before the licence (:1706-1712) overdisplayedServerPromptIdsRef,pendingStartedByPromptIdRefandcompletedPromptIdsRef, and folded in as!startedSinceSnapshot(:1731), so a prompt start this client already recorded now vetoes the DELETE instead of lettingremovePendingPromptabort a running turn. The vetoed arm also drops a row the confirming sync materialized for the cleared prompt (:1735-1748), preserving the editing/removing rows, so the cleared draft is not resurrected. - R6-2 (failed-GET arm had no settle veto and re-read a pre-await local). The arm now computes
settledfromsettledServerPromptIdsRef∪completedPromptIdsRef, includes it instartedOrCompleted, and gates the echo onif (!settled && !localMessageAppended)(:1819-1828) — a start echo followed by a settle and then a failed GET drops the row without appending the message a second time. - R6-4 (catch path reported a false queue failure for an admitted submission). The gate is now
!(syncClaimed && !admissionStarted) && !queuedPromptsRef.current.some(…)(:1963-1968), so a sync-claimed row keeps the failure path alive only when nothing reached the daemon; past admission the transport failure returns silently, matching how the success path reads the same claim.
One refinement rides along and is documented in the code: clearedPromptRemovalIdsRef (:544-550) tracks the ids this arm is DELETE-ing separately from removingServerPromptIdsRef, because that set's started-event skip exists for user-initiated removals and must not swallow the echo of a prompt the daemon started before the DELETE landed; it is consulted in the sync skip list (:697) and cleared on the owner/session reset (:1376).
Critical-only scan
The production delta of 12d2fb1 is +65/-26 in this hook and nothing else; I read every hunk and each one maps to a finding above. The rest of the PR's production surface is additive and read in full: the bridge returns reason: 'session_idle' on the idle rejection (bridge.ts:13761), the bridge interface and the SDK's DaemonMidTurnMessageResult add reason?: 'session_idle' as an optional field with the doc stating it is absent on older daemons and on every other rejection cause and that callers must keep their own idle detection, the serve route change is comment-only, QueuedPrompt gains the optional resubmittedAfterIdleRejection flag, and the protocol doc now states the rejection contract instead of over-promising it. All 30 review threads are resolved and no Critical thread is open.
CI at this head
Lint & Static, Integration Tests (no-AK, No Sandbox), both Desktop Shell jobs, the Live Host lane and TUI parity are green; Test (ubuntu-latest, Node 22.x), Serve A/B, the web-shell visual capture and review-pr were still running when I wrote this, so the rollup reads PENDING. I did not wait on them, and nothing in the completed set is evidence of a defect here.
wenshao
removed
the
autofix/takeover
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Full review at head 536ab851. Approving: C=0. This is the first review of 12d2fb19c3, the commit that landed after the last round — every Critical standing on this PR is closed there, and I verified each one against the code as it stands rather than against the commit it was filed on.
The six standing Criticals, re-checked at this head
| Finding | Verdict at 536ab851 |
Evidence |
|---|---|---|
| R5-1 confirmation gated on the lagging activity mirror | fixed | :1686 is now if (prompt.resubmittedAfterIdleRejection && !localMessageAppended) — the latestSessionActiveRef.current term is gone. |
| R2-3 single-flight join ignores freshness by default | fixed | :846 defaults notBefore = refreshRequestSeqRef.current, so a caller can never join a flight dispatched before its own state change; :906 still requires inflight.seq > notBefore. |
| R6-1 deferred refresh continuation drops the entry guards | fixed | :912-919 re-applies both guards inside the continuation, and the missing connectedRef now exists (:540, assigned :641) and is what :848 reads. |
| R5-2 cleared-row DELETE licence omits the start markers | fixed | startedSinceSnapshot (:1709-1712) carries all three markers the sibling arm trusts and is now a term of queuedInSnapshot (:1731), so a recorded start vetoes removePendingPrompt. |
| R6-2 failed-GET arm has no settle veto | fixed | :1819-1827 adds settled and gates the echo on !settled && !localMessageAppended. The double-echo the finding predicted is unreachable independently of that: appendLocalQueuedPrompt returns early on displayedServerPromptIdsRef.has(promptId) (:1389) and hideSettledServerPrompt always records settledServerPromptIdsRef (:624) when it clears the displayed marker. |
| R6-4 catch path reports a false queue failure | fixed for the reported path | :1964 is now !(syncClaimed && !admissionStarted), so the sync-claimed row of an admitted-and-echoed prompt returns before reportError. |
Each has a named regression test: does not join a pre-admission refresh from the post-admission tail, confirms an idle-rejected resubmission even when the session mirror reads idle, does not redispatch a queued refresh for a session the user left / after the connection drops, does not abort a cleared delayed fallback the snapshot reports running, does not delete a cleared fallback whose start its snapshot predates, does not echo a settled resubmission twice when its confirmation fails, does not report a queue failure for an admitted submission the sync claimed.
All 30 inline threads on this PR are resolved; reviewDecision still reads CHANGES_REQUESTED from a bot review filed against 9f8993e6, two commits back.
What I checked beyond the findings
Added field is not a dead switch. resubmittedAfterIdleRejection is written at :2344 and :3143 and read at :1686 and :1890. clearedPromptRemovalIdsRef is written at :1762, read at :697, cleared at :1795 and reset at :1376 — the split from removingServerPromptIdsRef is exactly what keeps the started-event skip at :1469 from swallowing the echo while preserving the sync-side suppression. No leak: the failure arm dispatches its refresh inside .then (:1787), but .finally clears the id one microtask later, long before that GET resolves and reaches syncServerQueuedPrompts.
Dropping connected from the refreshPendingPrompts deps does not regress the connect-time refresh. The effect that issues it keeps connected in its own dependency list (:1420, :1443), so it still re-runs when the connection flips; the callback only stopped changing identity.
The new reason field survives every hop. bridge.ts:13761 returns it, the route passes the bridge result through verbatim (res.status(200).json(result)), DaemonClient.enqueueMidTurnMessage returns the parsed body unchanged, and actions.ts forwards it — with a test pinning each seam, including forwards a daemon idle rejection across the session hop. The documented condition matches the code: the gate is entry.pendingPromptCount === 0 && entry.goalTurnActive !== true, and the attachment-budget and queue-full rejections do return a bare { accepted: false }, so "a missing reason is not evidence of a busy session" is accurate rather than aspirational.
CI: the red check is inherited from main, not caused by this PR
Test (ubuntu-latest, Node 22.x) fails 8 tests in 2 files — components/WorkspaceSessionProvider.loading.test.tsx (an unexpected GET /brand) and components/sidebar/WebShellSidebar.brand.test.tsx (the version tooltip). Neither file nor any of their sources is in this diff. The same job on main at 13b69b3313 — the exact commit this PR merged — fails the identical 8 tests in the identical 2 files (run 34439027258), while main at the preceding 005fc97b33 was green. git diff 13b69b3313 536ab851cb --stat is exactly this PR's 12 files, so the merge introduced nothing of its own. The breakage is #11244 landing on top of #11470 (which had moved the sidebar version label) — a semantic conflict that CI could not have caught on either branch alone.
This PR's own suites are green in that same run: web-shell reports 7161 tests with only those 2 inherited files failing, against 7113 on main — so all 48 added tests pass. acp-bridge, cli and sdk all pass with counts identical to main. Merge is gated on main going green, not on anything here.
Deferred, not blocking
This PR is past its fifth review round, so per AGENTS.md I am filing these as follow-ups rather than asking for another turn:
:1963-1977— whenadmissionStartedis true and the local row is still present,reportError(error, t('queue.queueFailed'))at:1977still fires for a prompt the daemon may already hold. The reported path (row claimed and dropped by the sync) is closed; this residual one self-heals through the.finallyrefresh at:1985, so it is a transient toast, not a lost message. Untested.:1692-1695passesrefreshRequestSeqRef.currentexplicitly, which is now exactly the parameter default. Harmless, and it does document intent at the call site.:382IMAGE_ONLY_PROMPT_TEXT = '[image]'duplicates a private literal fromextractPromptText(bridge.ts:2482) across a package boundary with no shared constant and no cross-package test pinning the pair. The comment names the source, which is the mitigation; the coupling stays unenforced.
中文说明
在 head 536ab851 上完成全面审查，批准，Critical = 0。
这是对 12d2fb19c3 的首次审查——该提交在上一轮 review 之后才推上来，此前所有轮次都发生在它之前。本 PR 上仍然挂着的 6 个 Critical 全部在其中被关闭，我是对照当前 head 的代码逐条核验的，而不是对照它们被提出时的那个提交。
六个 Critical 的核验结论：R5-1（:1686 已去掉 latestSessionActiveRef 门控）、R2-3（:846 默认 notBefore = refreshRequestSeqRef.current）、R6-1（:912-919 在 continuation 中重新施加守卫，新增的 latestConnectedRef 补上了原先缺失的 connectedRef）、R5-2（startedSinceSnapshot 三个 start 标记已成为 queuedInSnapshot 的一项，:1731）、R6-2（:1819-1827 加入 settled 否决项；且该 finding 预测的重复回显本就不可达——appendLocalQueuedPrompt 在 :1389 就按 displayedServerPromptIdsRef 幂等返回）、R6-4（:1964 改为 !(syncClaimed && !admissionStarted)，所报路径已闭合）。每条都有对应的具名回归测试。PR 上 30 个 inline thread 全部已解决；reviewDecision 仍显示 CHANGES_REQUESTED，来自两个提交之前针对 9f8993e6 的 bot review。
额外核查：新增字段 resubmittedAfterIdleRejection 有写有读（写 :2344/:3143，读 :1686/:1890），不是死开关；clearedPromptRemovalIdsRef 的写/读/清理/重置四点自洽，且 .finally 清理发生在失败分支那次 refresh 的 GET 返回之前，不会永久抑制 row 物化；refreshPendingPrompts 依赖数组去掉 connected 不影响连接建立时的刷新，因为发起它的 effect 自身仍依赖 connected；新增的 reason 字段在 bridge → route → SDK → actions 四跳中逐层原样透传，每个接缝都有测试钉住，且文档描述的空闲判定条件与 entry.pendingPromptCount === 0 && entry.goalTurnActive !== true 完全一致。
CI 红是从 main 继承的，与本 PR 无关：失败的 8 个测试落在 WorkspaceSessionProvider.loading.test.tsx 与 WebShellSidebar.brand.test.tsx，这两个文件及其被测源码都不在本 diff 内。main 在本 PR 所合入的那个提交 13b69b3313 上，同一个 job 失败于完全相同的 8 个测试、完全相同的 2 个文件；而其前一个提交 005fc97b33 上 main 是绿的。git diff 13b69b3313 536ab851cb --stat 恰好只有本 PR 的 12 个文件，说明这次 merge 没有引入任何额外改动。根因是 #11244 落在 #11470（移动了 sidebar 版本标签）之上产生的语义冲突。本 PR 自身的套件在同一次运行中全绿：web-shell 共 7161 个测试，仅上述 2 个继承文件失败，对比 main 的 7113 个，即新增的 48 个测试全部通过。acp-bridge、cli、sdk 的通过数与 main 完全一致。合并只被 main 转绿这一件事挡住。
延后处理（不阻断）：本 PR 已超过第 5 轮 review，按 AGENTS.md 我把以下三项作为 follow-up 记录，不再要求本轮修改：:1963-1977 在 admissionStarted 为真且本地行仍存在时仍会报一次假的 queue 失败（所报路径已闭合，此残留会被 :1985 的 refresh 自愈，属瞬时提示而非消息丢失，且无测试）；:1692-1695 显式传入的实参已与新的参数默认值相同（无害，但在调用点保留了意图说明）；:382 的 '[image]' 跨包重复了 bridge.ts:2482 中 extractPromptText 的私有字面量，注释指明了来源但没有共享常量或跨包测试来固定这层耦合。
Audit follow-ups deliberately not in this PRThis branch went through 24 rounds of a reverse-audit battery (18-20 independent auditors per round over the whole diff, each round's findings recorded and dispositioned). Everything that was a correctness defect, and everything whose fix was cheap and local, is in the PR. What follows is the residue: items whose fix needs new machinery or a new fixture, recorded here so none of them is silently dropped. Each was traced to code, not guessed. Echo fidelity
Coverage
Robustness
One rejected fix, recorded so it is not re-proposed Dispatching a refresh from the resubmission branch's claim arm — to restore a queue row for an admitted prompt the claiming snapshot did not list — was tried and reverted: the extra snapshot is taken while the daemon may still not list the prompt, and it drops the row the sync just materialized (a sync-materialized row carries no bind sequence, so the retention filter cannot exempt it). Two existing pins reddened. The window is real but benign — the started event's own refresh restores the row, and the echo is already guaranteed by the payload stash — and closing it properly needs a stamp or a fence on the materialized row, not an extra GET. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at acc327275923239b87744740746d23471c0487c9 — 1 confirmed standing Critical; Partial review — coverage gaps.
Pinned base: ae78d5b800a275b526d3e707ce3112926d34b156. Read the complete current production diff, surrounding admission/requeue/drain/confirmation/echo logic, daemon and SDK forwarding, and selected regression tests. The 11,000+ added test lines and every historical race have not been exhaustively audited.
Critical — R7-1 held-row entrance still reaches premature echo
Rechecked thread 3977665713, including the author's refutation 3987100054, against this head. The unheld path is improved, but the held path still has this reachable ordering:
- A mid-turn request is in flight when a hold/write block activates; its response is
{accepted:false, reason:'session_idle'}.packages/web-shell/client/hooks/useQueuedPrompts.ts:2905-2925keeps a local row withoutresubmittedAfterIdleRejection. - Lift the hold while the client activity mirror reads idle. The release effect (
:3178-3225) stamps onlyserverState:'submitting';releaseChainedPromptpasses that unflagged row to ordinary admission (:2617). Another client's prompt can meanwhile occupy the daemon FIFO before this POST, without its SSE activity update having reached this browser. - Ordinary admission returns HTTP 202 before execution (
packages/cli/src/serve/routes/session.ts:7037-7074;packages/sdk-typescript/src/daemon/DaemonSessionClient.ts:667-686explicitly implements admission-only). With no start event and the mirror still idle, the row skips confirmation atuseQueuedPrompts.ts:2027, then:2382-2397echoes it as sent and removes it locally, although it is still queued. - The final refresh does not repair the queue affordance: the echo sets the displayed-ID marker (
:909-922), and snapshot reconciliation skips an already-displayed prompt (:765-767,870-871). The message remains scheduled but its editable/cancellable queue row is gone.
The refutation's statement that the ordinary tail binds the row is true only when the activity mirror is active; that is precisely the condition not guaranteed here. A confirmation dispatched after the released row's own admission would be fresh evidence about that admission, not reuse of the earlier idle verdict. Keep the fix scoped to this provenance/ownership gap rather than stamping every held row indiscriminately.
Add the hold→idle rejection→release case with a delayed activity stream, an admission-only response, and the returned prompt still queued; require no sent echo and a retained daemon-ID-bound queue row. The existing hold-change test (useQueuedPrompts.midTurnReconcile.test.tsx:3155-3207) stops before release, so it does not adjudicate this outcome. This is a static producer/consumer proof, not a locally executed reproduction.
Prior fixes and coverage
The previously reported freshness and deferred-redispatch guards remain present (useQueuedPrompts.ts:1078-1101,1224-1242); immediate idle-rejected submissions no longer gate confirmation on the activity mirror (:2027). The current matcher rejects content-less attachment binding and degraded/unhydrated media. The new boundAtSeq producer/read sites were traced, and its stale-flight regression body was read. These checks do not certify every older Critical as fixed.
The reason field survives bridge → owner-runtime route → SDK JSON result → session-actions forwarding; read the new actions-hop test. It is emitted only at the open-session/no-prompt-FIFO/no-Goal idle gate; closing, budget and mismatched-ID failures remain reasonless, and attachment validation can throw before the idle verdict. The route is live-session-owner routed, retaining the existing single-runtime compatibility case rather than introducing a new selected-workspace fallback.
Remaining gaps: exhaustive old Critical adjudication, every new echo/removal/settle interleaving and callback lifetime, all changed tests, and live browser behavior. Suggestions remain deferred after the existing rounds. No PR code, tests, builds, installs, or workflows executed. Maintainer status not independently established; bugfix size is advisory, not a refactor policy block. Comment only; no approval implied.
Follow-up list update (after
|
Maintainer verification — real daemon + real browser, LinuxI built a local verification environment and drove this PR end to end against a real
Verdict: the central claim reproduces and the fix is load-bearing. Recommend merge. One new Minor finding (cosmetic, self-healing) and one pre-existing reachability gap are reported below; neither loses data and neither is a reason to hold the PR. How the race was made deterministicThe bug needs a window where the daemon is already idle but the browser's streaming mirror is not. In production that window is milliseconds. Here it is opened on purpose and asserted on every run:
A/B arms differ only in the two web-shell client files this PR touches:
Same daemon binary in both arms, so every difference below is the client half of this PR. Case A — the reported bug
Identical wire answer in both arms, observed in the page:
The daemon-side one-liner also checks out directly on Linux — an idle session answers Case B — image-only follow-up in the same windowThe case the PR calls out as the hard one: the daemon's summary carries no text, so the row has only the rendered placeholder to bind on. Observed
Case C — daemon queues the resubmission behind another client's turn, then the row is removedTo reach this deliberately, the browser's
10/10 checks pass on the PR arm:
One honest caveat on strength: I mutated Case D — the backward-compatibility claim, measuredStripping
This is consistent with what the PR documents ( Finding 1 (new, Minor) — the resubmitted turn is folded into the previous transcript block, until a reloadAfter the fallback resubmits, the daemon reports 2 turns (
Controlled twice:
Nothing is lost and it self-heals, so I would not block on it — but per-turn duration and token attribution are wrong in the live view until the session is reopened. Worth a follow-up issue. Finding 2 (pre-existing, not this PR) —
|
| target | result |
|---|---|
packages/web-shell useQueuedPrompts.midTurnReconcile.test.tsx + daemon/session/actions.test.ts |
418 passed (217 in the reconcile file) |
packages/acp-bridge src/bridge.test.ts |
936 passed |
packages/sdk-typescript test/unit/DaemonClient.test.ts |
425 passed |
packages/cli src/serve/server.test.ts |
1277 passed |
Plus npm ci (full build) and npm run build --workspace packages/web-shell clean. All CI checks on 768f876421 are green; the BLOCKED merge state is the stale CHANGES_REQUESTED from the bot review on f9b4de664d.
Environment
host Linux 6.12.63 x86_64, Node v22.22.2
worktree /root/git/pr11289 @ 768f8764210f151e159fdea0a4cbc3cf091e487f
merge-base ae78d5b800a275b526d3e707ce3112926d34b156
daemon node dist/cli.js serve --port 4289 --token <random> --workspace <ws>
isolated QWEN_RUNTIME_DIR and HOME; workspace trusted (writes not blocked)
capabilities include session_mid_turn_message_mutation + _query
model local OpenAI-compatible mock, 127.0.0.1:18289, 40 chunks x 250ms, echoes ACK<prompt>
browser real Chromium via Playwright, viewport 1180x900, deviceScaleFactor 2
中文说明
维护者本地验证 —— 真实 daemon + 真实浏览器（Linux）
我在本地搭了验证环境，用真实的 qwen serve daemon + 真实 Chromium 端到端跑了这个 PR，正好补上 PR 描述里自己留下的那个缺口：
修复的浏览器侧行为 —— 只重投一次、daemon 排队时保留队列行、绝不取消已经在运行的回合 —— 由上面的 hook 级用例覆盖，不是通过驱动真实浏览器验证的。
结论：核心问题可复现，修复是真正起作用的（load-bearing）。建议合并。 下面报告 1 个新发现（Minor，纯显示且刷新自愈）和 1 个既有的可达性缺口；两者都不丢数据，都不构成拦截理由。
如何把竞态做成确定性的
这个 bug 需要一个窗口：daemon 已经空闲，但浏览器的流式状态镜像还没跟上。生产环境里这个窗口只有几毫秒。这里是刻意打开的，并且每次运行都断言它确实存在：
- 通过真实 daemon 跑一个真实的 ~10 秒回合（本地 OpenAI 兼容 mock 模型，40 chunk × 250ms，回复
ACK<提示词>）。 - 一旦客户端显示回合正在运行，就把它的事件投递挂起 —— 用
addInitScript包裹window.fetch，把GET /session/:id/events的 chunk 以及活动轮询（/session/:id/status、/sessions/live-state、/session/:id/turn-index）缓存下来，之后按序释放。不改任何应用代码，只改投递时序 —— 而时序本身就是这个竞态。 - 直接轮询 daemon，直到
GET /session/:id/status返回{"activeWorkState":"idle","hasActivePrompt":false}。 - 断言客户端仍然不同意 —— 输入框占位符仍是
Processing. New messages will be queued.（客户端自己的流式镜像）。每次运行该前置条件都成立 —— 仅用例 A 就有 11 次。 - 在这个窗口里输入追问并发送。客户端走 mid-turn 插入路径，daemon 因会话已空闲而拒绝。
A/B 两臂只相差本 PR 改动的那两个 web-shell 客户端文件：
| 分支臂 | 产物 |
|---|---|
pr |
head 768f876421 上 npm ci 产出的 bundle —— 已验证与在 head 上重新 npx vite build 的结果逐字节相同，说明这套构建配方能复现实际发布产物 |
base |
同一棵树，仅把 client/hooks/useQueuedPrompts.ts 与 client/components/QueuedPromptDisplay.tsx 回退到 merge-base ae78d5b800a（已用 git show ae78d5b800a:<file> 证明逐字节相同），再用同样方式构建 |
两臂使用同一个 daemon 二进制，因此下面所有差异都来自本 PR 的客户端那一半。
用例 A —— 被报告的那个问题
两臂在页面里观察到的 wire 应答完全相同：
POST /session/:id/mid-turn-message → 200 {"accepted":false,"reason":"session_idle"}
| base | 本 PR | |
|---|---|---|
| 是否报错 | Daemon rejected mid-turn message |
无 |
| 追问是否进入对话记录 | 完全不出现 | 出现一次 |
| 携带该追问、真正到达模型的提示词数 | 0 | 1 |
| wire 上的重投 | — | POST /session/:id/prompt → 202 |
| 运行次数 | 4 次，每次都是同样的 4 项失败 | 7 次，每次 9/9 全通过 |
daemon 侧那一行改动在 Linux 上也直接验证通过 —— 空闲会话返回 {"accepted":false,"reason":"session_idle"}，重复 messageId 返回同样结果，之后 mid-turn-messages / pending-prompts 仍为空，说明这次拒绝没有转移所有权（daemon 日志：[mid-turn] session="…" rejected id "…": session idle）。
用例 B —— 同一窗口内的纯图片追问
也就是 PR 自己点名的那个难点：daemon 的摘要不带文本，队列行只能靠渲染出的占位文本去绑定。实测 GET /session/:id/pending-prompts → {"pendingPrompts":[{"text":"[image]", …}]}。
| base | 本 PR | |
|---|---|---|
| 是否报错 | Daemon rejected mid-turn message |
无 |
| 模型收到的带图提示词数 | 0 | 1（[image: image/png]） |
| 对话记录里的图片气泡 | 0 | 1（未重复，也不是空占位） |
用例 C —— daemon 把重投排在另一个客户端的回合后面，然后该行被移除
为了稳定复现，浏览器的 POST /prompt 在页面网络层被挂起 4 秒，期间由第二个客户端占住 prompt FIFO。daemon 侧的一切都是真实的。
PR 臂 10/10 通过：
- daemon 确实持有
rival: state "running"+follow-up: state "queued"； - UI 把追问保留为
Queued on server...的队列行 —— 没有渲染成已发送； - 删除操作只
DELETE了排队中那个promptId，从未指向正在运行的那个； - 正在运行的回合完整跑完（回答的 39 个 chunk 全部到达）；
- 被移除的追问从未执行（模型侧 0 次）。
关于证据强度的一点如实说明：我把本 PR 新增的 daemon-queued 确认逻辑 queuedInSnapshot 变异为 false 并重新构建，该变异体在 10 项检查下全部存活。在这个场景里，队列行之所以还在，来自 pending-prompts 的同步，因此本场景验证的是行为，并不能钉住那一个具体子句。钉住它的是 PR 里的 hook 级用例。
用例 D —— 对"向后兼容"这一说法的实测
把 wire 上的 reason 字段剥掉，就让真实 daemon 看起来像一个早于该字段的 daemon。此时新客户端会走旧的失败路径：报错、消息丢失（模型侧 0 次）。
这与 PR 自己的说明一致（"客户端会同时保留自己的空闲判定"）—— 而客户端那个信号恰恰就是滞后的那一个 —— 但有必要明确写出来：这个修复不是纯客户端的。 对 Web Shell 而言无所谓（bundle 由 daemon 提供，两半一起发布）；只有当 @qwen-code SDK 使用方独立升级、却连到较旧的 daemon 时才有影响。
发现 1（新，Minor）—— 重投的那个回合在实时视图里被并进了上一个转录块，刷新后自愈
回退逻辑重投之后，daemon 报告的是2 个回合（GET /session/:id/turn-index），但实时 UI 只渲染出一个块：只有一个 Processed 头、且丢掉了耗时，两段回答被拼在一起。刷新同一会话后就正确显示为两个块、各自 Processed 11s。
做了两次对照：
- 不是我 harness 的突发投递造成的：在同样被挂起再释放的突发下，一个普通的第二条提示词会渲染成两个块（
["Processed 11s","Processed 11s"]，未合并）。合并只发生在重投这条路径上。 - 不是窗口被人为放大造成的：把释放时机收紧到"拒绝一应答就释放"（接近生产时序），仍然合并。
没有数据丢失，且刷新即自愈，所以我不会因此拦截 —— 但在重新打开会话之前，实时视图里的单回合耗时与 token 归属是错的。值得开一个后续 issue。
发现 2（既有问题，与本 PR 无关）—— clearQueuedPrompts 目前在 UI 上没有触发入口
PR 的审查测试计划里要求"在这条追问正在重投时清空队列"。在当前的 Web Shell 里用户做不到：onClearQueuedMessages 在 ChatEditor / useComposerCore 里声明了，也从 App.tsx / ChatPane.tsx 传了进去，但从未被解构、也从未被调用 —— merge-base 与 head 都是如此，而且这三个文件本 PR 都没碰。所以页脚文案 Press ↑ to edit the latest queued message · Esc to clear queue 里承诺的手势是失效的；用例 C 里按 Esc 后，两个 pending prompt 原封不动。真正可达的移除路径是队列行自带的 Delete（removeServerPromptForAction），也就是上面用例 C 实际走的那条。
对本次评审的含义：这次"先确认再取消"工作中的"清空全部"那一半，目前只能从测试里到达，产品里到不了。这是保留 hook 级覆盖的理由，而不是改动本 PR 的理由。
改动涉及的测试文件，在 Linux 上重跑
PR 描述里的套件结果来自 macOS，并带有九个无关失败。在这台机器上，改动涉及的文件全绿：
| 目标 | 结果 |
|---|---|
packages/web-shell useQueuedPrompts.midTurnReconcile.test.tsx + daemon/session/actions.test.ts |
418 通过（其中 reconcile 文件 217） |
packages/acp-bridge src/bridge.test.ts |
936 通过 |
packages/sdk-typescript test/unit/DaemonClient.test.ts |
425 通过 |
packages/cli src/serve/server.test.ts |
1277 通过 |
另外 npm ci（完整构建）与 npm run build --workspace packages/web-shell 均干净通过。768f876421 上所有 CI 检查为绿；BLOCKED 的合并状态来自 bot 在 f9b4de664d 上那条已过期的 CHANGES_REQUESTED。
Round 25 of the local audit — dispositions (
|
Rounds 10-11 of the automated review, plus three local audit rounds — dispositionsEverything below landed in 8fe4a06. Each fix was reproduced first, then pinned by a witness that was verified by mutation (apply the fix → green; revert or narrow the fix → that witness and only that witness reddens; restore byte-identical). Counts: 15 new or updated witnesses, 12 mutations run, From the automated review (6 Criticals across rounds 10 and 11, all confirmed, none refuted)
From three local reverse-audit rounds over the same delta (round 26: four auditors; round 27: two; round 28: one fresh-eyes) Fixed with witnesses: a co-client's prompt parking inside our DELETE and being echoed locally on top of the daemon's stream; the sync binding an unreleased row to an id it cannot own; the failing-body echo firing when admission never started (an attachment upload rejects before the POST); the candidate count including rows the drain has not released, which manufactured an ambiguity and lost the echo; and a Reverted after analysis, so they are not re-proposed: the started handler's raw-text branch writing Corrected as inaccurate: five code comments and six test comments or fixtures that stated something the shipped daemon contradicts — among them three fixtures justifying Round 28 was intended as the confirmation round and found no Critical, but its three Majors — the reverted consult, the doc verdict and the toast — were all real and all fixed, so the clean-pass count restarted at zero rather than reaching the two consecutive dry rounds this branch has been holding itself to. Round 29 runs on 8fe4a06. Audit follow-ups deliberately not in this PR (updated, 24 items)This branch went through 28 rounds of a local reverse-audit battery (independent auditors over the whole diff each round, every finding recorded and dispositioned) plus 11 rounds of the automated PR review. Everything that was a correctness defect, and everything whose fix was cheap and local, is in the PR. What follows is the residue: items whose fix needs new machinery, a redesigned fixture, or a decision that belongs in its own change. Each was traced to code, not guessed. Echo fidelity
Coverage
Robustness
Fixes tried and rejected, recorded so they are not re-proposed Dispatching a refresh from the resubmission branch's claim arm — to restore a queue row for an admitted prompt the claiming snapshot did not list — was tried and reverted: the extra snapshot is taken while the daemon may still not list the prompt, and it drops the row the sync just materialized (a sync-materialized row carries no bind sequence, so the retention filter cannot exempt it). Two existing pins reddened. The window is real but benign — the started event's own refresh restores the row, and the echo is already guaranteed by the payload stash — and closing it properly needs a stamp or a fence on the materialized row, not an extra GET. Adding Writing Stashing the payload where the started handler suppresses an ambiguous echo: at that moment the handler cannot attribute the event to a row, which is why it suppressed, so the stash would attach one message's attachments to another's echo. The shipped fix instead lets the body that holds the payload consume a park that named its row as the only candidate. Widening the removal replay's bound-row source to any payload-complete row, so that a text-only row outranks the parked rendering: applied and reverted. A hydration-loss row carries the daemon's |
Round 29 of the local audit — dispositions (d16e53f)Round 29 audited the claims of the previous commit rather than its diff, which is why it found what nine diff-focused auditors had walked past. One Critical, pre-existing rather than a regression. An echo sourced from a bound payload-complete row records itself only in the displayed marker — no park, because the row is bound, and no This supersedes an earlier rejection on this PR: a marker write in the started handler's raw-text branch was applied and reverted in round 26 as not load-bearing, and it also rerouted bodies away from the confirming refresh. The write that shipped is at the settle, is load-bearing, and cannot reroute anything, because a settle is terminal. One Suggestion, fixed. A row rebuilt from a fully hydrated snapshot carries the daemon's rendering of an attachment-only message as its text, so every row-sourced echo showed a literal One Suggestion on witness quality, acted on against myself. A caption witness written during this round to pin the replay's Gates at d16e53f: The clean-pass count restarted again: round 29 was not dry. Round 30 runs on d16e53f. |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review pass at head 8728e5f33e4377cb5413ecdb25bf738d98aa126c. Not approving: six Critical findings remain unresolved on this PR, two of them verified still present at this head, and the main CI lanes were cancelled on this commit so there is no concluded test or lint evidence for it. The pass short-circuited once standing blockers were confirmed.
Verified still present at this head
- The protocol reference states a
{ "removed": false }verdict the route does not implement -docs/developers/qwen-serve-protocol.md, the single replaced line in the Mid-turn messages section. The head text reads: "{ \"removed\": false }- already injected, completed, or not found, as the DELETE route below defines it - means the daemon no longer holds the id as a queued message and it must not be re-posted: in the first two cases it was delivered, and in the third either a peer removed it or its record aged out after delivery." The not-found arm ofremoveMidTurnMessageanswers the same shape for an id in neither the queue nor either ring, so this sentence converts an absence of knowledge into a positive "delivered, do not resend" verdict and into a prohibition that contradicts the resend procedure two sentences earlier in the same paragraph ("resend every id it enqueued during that window ... plus any id it holds no other delivery evidence for"). This is the published contract a client implements against, so the contradiction is the defect rather than a wording nit. - The re-attach rule treats a filtered listing as complete - same file, same line. The head text prescribes resending "every id ... that
GET /session/:id/mid-turn-messagesstill reports as queued, plus any id it holds no other delivery evidence for", and then asserts "An id in neither the queue nor either ring has nothing left to delete". The listing is filtered -getPendingPromptsskips prompts markedremoved, and anonymous enqueues are handled off the shared queue surface - so an id can be absent from it while the daemon still holds the payload, and the stated equivalence does not hold.
Verified premise, consumer arm not read
admissionStartedis set before the POST is dispatched -packages/web-shell/client/daemon/session/actions.tscallsoptions?.onAdmissionStarted?.();at :1483 and only thenawait session.submitPrompt(...)at :1486 (the same ordering appears on the other submit path at :1167). The reported defect is that the new submit-failure echo arm inpackages/web-shell/client/hooks/useQueuedPrompts.tsaround :2710 reads that flag as "the daemon may already hold this message", which makes a definite rejection arriving after the callback - aDaemonHttpError, a 413 - echo as a possible delivery. I confirmed the ordering the finding rests on and did not read the consuming arm, so that half is unconfirmed here; if the arm gates on anything beyondadmissionStartedthe finding narrows, but the flag alone cannot support the inference.
Reported at this head, not verified within this pass
Three further Criticals are unresolved and anchored at this head. Recorded as unconfirmed rather than cleared:
useQueuedPrompts.tsaround :4312 - inclearQueuedPrompts, the replay echoes a parked start into the transcript and the next statement rebuildsrestoredPromptsfrom all offailedPromptsunconditionally, so one message is both shown as having run and returned to the visible queue where it can be cancelled or edited.useQueuedPrompts.tsaround :649 -clearedUnconfirmedPromptIdsRefis the only record of a cancellation that could not be confirmed, and the owner-change reset wipes it without first issuing the DELETEs it stands for, so a cancelled message can be re-materialized into the queue and later run.useQueuedPrompts.tsaround :1954 - the ambiguity gate leavessoleCandidateRowIdstructurally settable only for an attachment row, since a text row can reachpromptonly through theunboundMatches.length === 1term wherecandidatesis empty, so a text row's start goes unechoed exactly when its park can never be attributed back to it.
CI
On this head Test (ubuntu-latest, Node 22.x), Lint & Static (ubuntu-latest, Node 22.x), Serve A/B, web-shell E2E Smoke and Integration Tests (no-AK, No Sandbox) all concluded cancelled, and review-pr was still running. Cancelled lanes are not attributed to this change and are not treated as a gate on their own, but the consequence is that this head carries no concluded unit, lint or integration evidence at all - so nothing in CI corroborates or contradicts the findings above.
Next step
Resolve the six open Criticals, starting with the two protocol-reference sentences, which are cheap to correct and are the contract every client reads: state the not-found arm as "the daemon does not hold this id as a queued message; whether it was delivered is unknown" and drop the completeness claim about the listing, or make the listing unfiltered before relying on it. Then confirm or close the four useQueuedPrompts.ts items and get a clean run of the cancelled lanes on the new head before re-requesting review.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
What this PR does / PR 主旨
When a message typed during a running turn is refused because the session already went idle, the daemon now returns reason: 'session_idle' and the browser treats it as "resubmit as an ordinary prompt" instead of "the send failed". Before acting on its own queue view, the client confirms against the daemon's authoritative pending-prompt state, and any ambiguous/absent confirmation leaves the queue row in place rather than guessing. The daemon-side change is small and additive (bridge.ts returns the new reason, mirrored in bridgeTypes.ts, the SDK DaemonMidTurnMessageResult type, the route doc-comment, and the protocol reference); the weight is in the useQueuedPrompts.ts echo/park/reconcile state machine.
当回合进行中输入的消息因会话已空闲被拒时，daemon 现在返回 reason: 'session_idle'，浏览器据此按"作为普通提示词重投"处理，而不是当作发送失败。客户端在依据自身队列视图行动前，会先向 daemon 权威的 pending-prompt 状态确认；任何含糊或缺失的确认都保留队列行而不猜测。daemon 侧改动小且为增量（bridge.ts 返回新 reason，在 bridgeTypes.ts、SDK 的 DaemonMidTurnMessageResult、路由文档注释与协议参考中镜像）；主要复杂度集中在 useQueuedPrompts.ts 的 echo/park/reconcile 状态机。
Why this is a COMMENT, not an approval / 结论
This is an independent static adjudication at head 8728e5f3, focused on the Criticals the previous human pass recorded as unconfirmed. I confirm one, refute one, downgrade one, and narrow one — with head-tree witnesses. One independently confirmed Critical plus the still-unresolved protocol-doc contradiction plus red CI on this commit mean I cannot approve.
这是在 head 8728e5f3 上的一次独立静态判定，聚焦上一轮人评标记为"未确认"的几条 Critical：我确认一条、证伪一条、降级一条、收窄一条，均附 head tree 举证。存在一条独立确认的 Critical、协议文档矛盾仍未解、且该 commit 的 CI 为红，因此不能批准。
🔴 Confirmed Critical — the deferred-clear record is wiped on owner/session change without flushing its DELETEs
clearedUnconfirmedPromptIdsRef is the sole record of a user cancellation whose DELETE was deferred pending positive "still queued" evidence. Its own doc comment (useQueuedPrompts.ts:642-648) states the contract: removing "needs positive evidence that the prompt is still queued, so the clear is applied by the next snapshot that carries it instead of being dropped on the floor."
- Written at
:2307(submit body) and:4221(clear handoff); consumed only by the deferred-clear loop at:1194/:1207, which runs against a fresh snapshot for the current session. - The owner/session reset effect (deps
[ownerToken, sessionId, workspaceCwd],:1801) doesclearedUnconfirmedPromptIdsRef.current = new Map()at:1795— the whole effect body only reassigns refs to empty; noremovePendingPromptis issued before the wipe.
Failure scenario: the user clears/cancels a queued follow-up during the idle-rejection window; the DELETE is deferred because the daemon still holds it queued. Before the confirming snapshot lands, the owner token changes (switching sessions during the window — a case this PR's own test plan calls out — or, per the reviewer trace, a same-session reconnect that re-mints the token via !snapshot.isCurrent()). The map is wiped, the deferred DELETE is never sent, the daemon promotes the still-queued prompt on the next settle, and a message the user explicitly cancelled runs anyway — exactly the outcome the ref exists to prevent. A flush is feasible: removePendingPrompt already accepts a non-current sessionId (actions.ts foreign-session path), so the pending DELETEs can be issued against the stale session during teardown (or the map preserved across a same-session reconnect).
clearedUnconfirmedPromptIdsRef 是"用户取消、但 DELETE 因缺乏正面证据被延迟"的唯一记录。owner/session 重置 effect 在 :1795 直接把它清成空 Map，清前不发任何 DELETE；消费点只在针对当前会话的快照循环里。于是：在空闲拒绝窗口内取消一条排队消息后、确认快照到达前发生 owner 变更（切会话，或同会话重连重铸 token），待删记录被丢弃，daemon 仍把该 prompt 当 queued 持有并在下次 settle 时提升执行——用户明确取消的消息反而跑起来。修法方向：teardown 时用已支持的"非当前 sessionId"路径把挂起的 DELETE 补发出去，或在同会话重连时保留该 Map。
🟢 Refuted — soleCandidateRowId "unreachable for text rows" is not a defect
The ambiguity gate makes soleCandidateRowId settable only for an attachment row, and that is correct by construction: candidates.length === 1 requires prompt === undefined, i.e. uncountableAttachmentRow || unboundMatches.length !== 1; with a single candidate the first disjunct forces it to be an images/files row. Nothing is lost for text rows — a text row's start is echoed with its full row payload on the event path (:1952-1962), and the settle-time last chance (:1060-1089) reproduces parked.text verbatim, which for a plain text row is the payload. The catch arm's extra eventCannotReproducePayload(prompt) gate (:2710) is false for plain text, so that arm was never intended to serve text rows. No drop, no double echo. This standing Critical can be cleared.
那条"文本行永远设不上 soleCandidateRowId"是幻影：按构造，唯一候选必然是附件行；而文本行的 start 在事件路径上用完整行 payload 回显，纯文本的 settle 兜底又能逐字复现 parked.text（对纯文本行 text 即 payload），catch 臂的 eventCannotReproducePayload 门对纯文本恒 false，本就不服务文本行。无丢失、无重复回显。
🟢 Downgraded — clearQueuedPrompts replay "both run and still queued" is transient/self-healing
The parked-start replay (:4308-4312) and the unconditional restoredPrompts rebuild do coexist, but the replay's echo sets the displayedServerPromptIdsRef marker, the same block dispatches a confirming refresh (:4333), and syncServerQueuedPrompts splices out any row whose serverPromptId is already displayed (:802-805) — so the duplicate row disappears within one GET round trip. Deleting it in that window is a no-op (removeServerPromptForAction refuses non-queued, :3543). No message is lost or duplicated; at most a briefly-redundant row. Not Critical.
parked-start 回放与无条件 restoredPrompts 重建确实并存，但回放会置 displayed 标记、同块派发确认刷新，且 sync 会把已 displayed 的行 splice 掉——重复行在一个 GET round trip 内消失，期间删除是 no-op。无丢失/无重复，最多短暂多显示一行，非 Critical。
🟡 Narrowed — the submit-failure echo arm around admissionStarted
The "definite rejection is silently swallowed" half does not hold: with no coincident park, echoedParkId === undefined and queue.queueFailed is still reported (:2733-2736). The real residual core is narrower and needs a coincidence: an attachment-bearing row (eventCannotReproducePayload true) + an unattributable foreign prompt_started that parked with soleCandidateRowId === localId + this row's own POST then failing with a definite rejection (413 / queue-full). In that case the message is echoed under a foreign promptId, the row is dropped, the draft is not restored, and the toast is suppressed — a message the daemon never received is shown as sent. Fix in spirit: gate the echo arm on ambiguity rather than treating every post-callback failure as possible-delivery (the daemon layer already has isDefinitePromptAdmissionRejection at actions.ts:646-651; export and consult it so a definite rejection restores the draft and reports). Reachability depends on multi-client timing, so treat as Important, not a hard blocker.
"确定性拒绝被静默吞掉"这半不成立（无巧合 park 时仍报 queue.queueFailed）。真正残留的核心更窄，需三重巧合：附件行 + 无法归因的他端 prompt_started 恰好以 soleCandidateRowId===localId park + 本行 POST 随后以确定性拒绝（413/队列满）失败——此时消息被挂到他端 promptId 回显、行被删、草稿不恢复、toast 被抑制，一条 daemon 从未收到的消息显示为已发送。修法：回显臂按"歧义"而非"回调后一律视为可能已投递"来 gate（daemon 层已有 isDefinitePromptAdmissionRejection，导出并据此在确定性拒绝时恢复草稿并报错）。可达性依赖多客户端时序，按 Important 处理，非硬阻断。
Protocol-doc contradiction (agree with the prior human pass)
The single replaced line in docs/developers/qwen-serve-protocol.md is the published contract clients implement against, and it contradicts itself: it instructs a re-attaching client to "resend every id ... plus any id it holds no other delivery evidence for", then states { "removed": false } (which the DELETE route also returns for the not-found arm) "means the daemon no longer holds the id as a queued message and it must not be re-posted ... in the third either a peer removed it or its record aged out after delivery." Conflating not-found with delivered turns an absence of knowledge into a positive do-not-resend verdict, and is order-dependent against the resend rule two sentences earlier. This should be tightened before merge (the code is fine; the contract text is the defect). The companion "an id in neither the queue nor either ring has nothing left to delete" equivalence was flagged as unsound because the listing is filtered — I did not independently re-derive the getPendingPrompts filtering, so I defer to that pass on it.
docs/developers/qwen-serve-protocol.md 那一整行替换是客户端据以实现的公开契约，却自相矛盾：先要求重连客户端"重投所有……以及任何没有其他投递证据的 id"，随后又说 { "removed": false }（DELETE 路由对 not-found 也返回同一形状）"意味着 daemon 不再持有该 id……不得再重投……第三种情况要么被对端删除、要么其记录在投递后老化"。把 not-found 等同于 delivered，将"未知"变成肯定的"勿重投"判定，且与两句前的重投规则形成顺序依赖的矛盾。代码本身没问题，问题在契约文本，合并前应收紧。配套的"既不在队列也不在两个 ring 的 id 无可删"等价式被指因列表经过过滤而不成立——我未独立复核 getPendingPrompts 的过滤，此点以该轮人评为准。
CI honesty / CI 现状
Against this head the reported key lanes are not green: Test (ubuntu-latest), Lint & Static (ubuntu-latest), Integration Tests (no-AK), web-shell E2E Smoke, and Serve A/B are failing; Test (macos/windows) and Integration Tests (CLI) are skipping; only Desktop Shell, Capture visuals, Live Host, OpenTUI no-flicker and Real daemon E2E pass. So there is no concluded test/lint evidence for this commit — notably Lint & Static failing sits against the PR body's "repo-wide lint passed" claim, which is worth the author reconciling. I did not run any suite myself (static review only) and did not attribute each failure.
在当前 head 上，报告出的关键 lane 并非全绿：Test (ubuntu-latest)、Lint & Static、Integration Tests (no-AK)、web-shell E2E Smoke、Serve A/B 均 fail；Test (macos/windows)、Integration Tests (CLI) 在 skipping；仅 Desktop Shell、visuals、Live Host、OpenTUI no-flicker、Real daemon E2E pass。即该 commit 没有结论性的测试/lint 证据——尤其 Lint & Static fail 与 PR body"全仓 lint 通过"的说法相左，值得作者对齐。我未运行任何测试（纯静态），也未逐一归因每个失败。
Verdict / 判定
🔄 Request changes. One independently confirmed Critical (deferred-clear wipe), an unresolved protocol-doc contract contradiction, and red CI on this commit. The refuted (soleCandidateRowId) and downgraded (clearQueuedPrompts replay) items can be cleared from the standing backlog; the narrowed echo arm is Important. Given the review loop is at round 12 with the diff still growing, the confirmed Critical and the doc contradiction are the two worth batching into the next push.
🔄 请求修改。 一条独立确认的 Critical（延迟清除记录被 wipe）、未解的协议文档契约矛盾、以及该 commit 的红色 CI。被证伪的 soleCandidateRowId 与被降级的 clearQueuedPrompts 回放可从遗留清单中清掉；收窄后的回显臂为 Important。鉴于已到第 12 轮且 diff 仍在增长，建议把"确认的 Critical + 文档矛盾"两项一起在下一次 push 中批处理。
(Static, read-only review at head 8728e5f3; no tests/build/lint were run. 纯静态只读审查，未运行任何测试/构建/lint。)
Round 12 answered; round 13 acknowledged, re-opened, not yet fixedRound 12 — all six Criticals fixed in A correction to my own handling of this PR, stated plainly: while resolving round 12's threads I resolved round 13's three threads in the same batch, without having read or addressed them. That was wrong — a resolved thread reads as a dispositioned finding. All three are un-resolved again and the unresolved count is back to 3. Nothing about them is fixed yet. Round 13 — three Criticals, acknowledged and open:
Gates at The two Criticals round 12 carried forward from round 10 were already fixed in earlier commits and are answered in their threads: R10-2 by excluding unreleased rows from the sync's match count ( |
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review pass at head bf4453daf31c2982ba584cc9c7fc1e9fd801fa94. Not approving: three Critical findings are unresolved on this PR and two of them are anchored at this exact head, so nothing newer exists that could have closed them. One is partly verified in the head source below; the other two are recorded as unconfirmed because this pass was cut short by repeated GitHub API timeouts.
Open Criticals
-
attachmentMatchescarries no originator condition -packages/web-shell/client/hooks/useQueuedPrompts.ts:1944-1953, verified at this head:const attachmentMatches = queuedPromptsRef.current.filter( (item) => !item.serverPromptId && !unreleasedPromptIdsRef.current.has(item.id) && item.serverState === 'submitting' && ((item.images?.length ?? 0) > 0 || (item.files?.length ?? 0) > 0) && pendingPromptTextsMatch(item.text, eventText), ); const uncountableAttachmentRow = attachmentMatches.length > 0;
The filter keys on local row state and text equality only. There is no term restricting a match to a start this client originated, so the finding's concern - that an originator-less foreign start whose text matches can stamp a local attachment-bearing row as the sole owner of a payload it did not submit - has no guard at this site. What I did not confirm: the sibling matcher the finding contrasts it with, which it says refuses an attachment row unless the originator is this client. My read of that half was interrupted by an API timeout, so the asymmetry itself is unverified here even though the missing condition at this site is not.
-
The resubmission arm leaves a row unbound when the confirming refresh is not "refreshed" - reported at
useQueuedPrompts.ts:2511, anchored at this head, not confirmed. The region I did read (:2495-2530) shows the started-or-completed arm dropping the row and the else arm recording the id inreturnedUnboundPromptIdsRefbefore registering the completion callback; I did not reach the refresh-result arm the finding names, so neither the mechanism nor the claim that its premise is unconditionally false for a file-bearing row is verified either way. -
The protocol reference's resend mandate against its own caution - reported on
docs/developers/qwen-serve-protocol.mdand anchored at the previous head, so it may already be addressed. The doc line was rewritten at this head: the{ "removed": false }sentence now separates "already injected or completed - delivered, so it must not be re-posted" from "an id not found, which for one the query had just listed as queued means it left the queue between the two calls and may have been dropped undelivered by the close-authorization path above", and the phrase the finding quotes about a client that has not watched this session's prompts start no longer appears in the diff. The mandate it conflicts with is still there - "must therefore resend every id it enqueued during that window thatGET /session/:id/mid-turn-messagesstill reports as queued, plus any id it holds no other delivery evidence for" - and the DELETE-then-re-post procedure beside it. Whether the rewritten paragraph still forbids, for a re-attaching client, the recovery it mandates is the part I did not establish; reading the paragraph end to end settles it.
CI
Every non-skipped check on this head concluded success, including the unit, lint, serve A/B, web-shell E2E smoke and integration lanes that were cancelled on the previous head. Only review-pr was still running at review time, which is not treated as a gate. So CI is healthy here and nothing in it is attributable to this change - and equally, the green lanes do not cover the three items above.
Next step
Confirm or close the three open Criticals against this head. The originator question is the one to answer first, since the code at :1944 is unambiguous and the fix is a single added condition if the sibling really is stricter; the doc item may already be resolved by the rewrite and needs only a read of the full paragraph to close. A short reply on each thread naming the commit that closes it would let the next pass verify fixes rather than re-derive a 1,425-line hook. Note for the record that this pass hit repeated api.github.com timeouts, which is why two of the three items are reported as unconfirmed rather than cleared.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review pass at head b759e7067848e8b886643bc15ccba42f153d1f4c. Not approving: one Critical remains unresolved and is anchored at this exact head, so no newer commit closes it. The other two findings from the same round - the missing originator condition on the attachment matcher and the protocol reference's resend contradiction - are now resolved, and the head source confirms both were addressed: docs/developers/qwen-serve-protocol.md now separates the already-injected-or-completed case from the not-found case and states that the latter "may have been dropped undelivered", rather than asserting a delivered verdict for it.
Open Critical, not confirmed closed
The non-refreshed confirmation arm returns without binding or removing the row - packages/web-shell/client/hooks/useQueuedPrompts.ts:2497-2541. The arm the finding names reads, at this head:
if (refresh.status !== 'refreshed') {
const settled = settledServerPromptIdsRef.current.has(result.promptId) ||
completedPromptIdsRef.current.has(result.promptId);
const startedOrCompleted = settled ||
displayedServerPromptIdsRef.current.has(result.promptId) ||
pendingStartedByPromptIdRef.current.has(result.promptId);
if (startedOrCompleted) {
/* drop the row, settle the callback */
} else {
returnedUnboundPromptIdsRef.current.set(result.promptId, localId);
/* trim to 200 */
if (prompt.onComplete) settleCompletionCallback(result.promptId, prompt.onComplete);
}
return;
}What I can confirm from the source: in the else branch the arm neither removes the row nor stamps it - no serverPromptId, no serverState transition, no bindRowToPrompt() - and then returns. The row therefore persists in the state it entered with, and the only thing recorded for it is an entry in returnedUnboundPromptIdsRef plus a registered completion callback. Every sibling arm in this body that reaches a terminal decision does one of the two: it filters the row out (:2508-2520, :2549-2559) or it calls bindRowToPrompt() / stamps serverPromptId with serverState: 'queued' (:2470-2479, :2569-2573). This branch does neither, which matches the reported shape of a row left unbound and still submitting.
What I could not establish within this pass, and why the item is reported unconfirmed rather than confirmed: the finding's load-bearing claim is that the arm's premise is unconditionally false for a file-bearing row, so the row is not merely delayed but permanently stuck and suppresses every other queued prompt. Proving that needs the consumers of returnedUnboundPromptIdsRef and the binding path a row carrying resubmittedAfterIdleRejection with images/files takes - the flag is set at :3170 and :3972, and the post-refresh arm at :2576-2590 echoes and drops such a row - and that trace runs through a 15k-line hook that this pass could not cover in budget. So the mechanism is partly corroborated and the file-bearing-row half is unverified in both directions.
CI
Every non-skipped check on this head concluded success, including the unit, lint, serve A/B, web-shell E2E smoke and integration lanes. Only review-pr was still running at review time, which is not treated as a gate. Nothing in CI is attributable to this change, and the green lanes do not cover the arm above.
Next step
Close the one open Critical, or answer it on its thread with the trace that shows a file-bearing row carrying resubmittedAfterIdleRejection cannot reach the else branch at :2521 - and if it can reach it, name what later rebinds or removes the row. A test that drives a non-refreshed confirmation for an attachment-bearing resubmission and asserts the row is either bound or gone would settle it in both directions. Then re-request review; with the other two findings closed this is the only gate left.
Re-verification at head
|
| # | Check | head b759e70678 |
base bc7a186cda |
|---|---|---|---|
| A | Text follow-up sent in the race window | ✅ 9/9 — resubmitted, runs once, no toast | ❌ Daemon rejected mid-turn message, message lost (0 prompts at the model) |
| B | Image-only follow-up in the same window | ✅ 7/7 — 1 image prompt at the model, 1 bubble | ❌ toast, 0 image prompts, 0 bubbles |
| C | Resubmission queued behind another client's turn, then row Delete | ✅ 10/10 — row Queued on server..., DELETE names only the queued id, running turn completes in full |
— |
| W | Wire, idle session | ✅ {"accepted":false,"reason":"session_idle"} twice; mid-turn-messages and pending-prompts empty afterwards |
— |
| F1 | Previous Finding 1 (resubmitted turn folded into the prior block) | Processed header, a reload shows Processed 11s ×2, and turn-index reports 2 turns |
— |
| F2 | Previous Finding 2 (Esc to clear queue not wired) |
— | |
| R13-2 | Stranded attachment row | ❌ reproduces (below) | n/a: base drops these messages outright |
R13-2 through the product
Staging. Same race window. A second client takes the prompt FIFO while the browser's resubmission is in flight, so the daemon queues the follow-up. Then I abort exactly one request: the first GET /session/:id/pending-prompts after the resubmission's 202, which is the confirmation refreshPendingPrompts makes. That is what a single network blip does. Every later GET goes through untouched. A third prompt is then queued over REST without a client id; it is "possibly ours", so it should appear in the panel.
| Follow-up shape | Confirming GET | Row in the panel | Row Delete | 3rd prompt queued on the daemon |
|---|---|---|---|---|
| file + caption | aborted once | Submitting... for 14.6 s, until the follow-up settled |
disabled, 0 DELETE sent |
hidden in 19/19 samples (~12 s) |
| image + caption | aborted once | Submitting... for 14.5 s |
disabled | hidden in 19/19 samples |
| image only | aborted once | Queued on server... |
works | listed |
| text only | aborted once | Queued on server... (text-bound on the next sync) |
works | listed |
| file + caption | succeeds (control) | Queued on server... |
works; removed prompt never ran | listed |
| file + caption | 1.5 s latency, no failure | Submitting... ~1 s, then bound |
works | listed (hidden for 1 of 5 samples while the row was still unbound) |
In the stranded runs, the 8 successful GET /pending-prompts that followed the aborted one all returned 200, and none of them bound the row. The row vanished at exactly the sample where the follow-up settled. The third prompt went straight from invisible to running, so for about 12 s the user could neither see, edit, nor delete a message the daemon still held queued. After the follow-up starts, the panel shows the same message twice: once as sent in the transcript, and once as a Submitting... row with dead actions.
What this adds to the bot's reading of R13-2:
- The scope is wider than "file-bearing". An image row with a caption strands the same way.
matchesUnboundSubmittingRowrefuses any attachment row whoseitem.textis non-empty, and the started event carries nocontent. Only text-only and image-only rows recover. - It needs a confirmation that did not come back
refreshed. In one run, 1.5 s of latency on every GET, with the event stream live, did not strand the row. I did not construct a naturalsuperseded. - No data loss. In every stranded run the follow-up reached the model exactly once (mock log), was echoed once, and raised no error toast. On base, the same file-bearing follow-up is never resubmitted at all (0 prompts at the model). This is a degraded success on a path that used to lose the message, but it is still a user-visible wedge on new surface.
- The sibling test
keeps an idle fallback submitting when its confirmation snapshot failsuses a text-only row, which is exactly the shape that recovers. That is why the suite is green.
Suggested direction (my reading, not verified): when a later snapshot lists a prompt id held in returnedUnboundPromptIdsRef as queued, bind that row by id. The map already pairs the daemon's id with the local row, so the id-bind would no longer depend on the confirmation being the one snapshot that arrives.
Gates at head, Linux
| Target | Result |
|---|---|
packages/web-shell: useQueuedPrompts.midTurnReconcile + useQueuedPrompts.dom + QueuedPromptDisplay + daemon/session/actions tests |
4 files / 566 passed |
packages/acp-bridge src/bridge.test.ts |
940 passed |
packages/sdk-typescript test/unit/DaemonClient.test.ts |
432 passed |
packages/cli src/serve/server.test.ts |
1281 passed |
root npm run build / npm run bundle |
exit 0 / exit 0 |
CI on b759e70 is green across the board; only the bot's review-pr job is still pending.
Environment
host Linux 6.12.63 x86_64, Node v22.22.2
worktree /root/git/pr11289 @ b759e7067848e8b886643bc15ccba42f153d1f4c
merge-base bc7a186cdadc1235cafda295799080d1d6ab0b5a
daemon node dist/cli.js serve --port 4289 --token <random> --workspace <ws>
isolated QWEN_RUNTIME_DIR (fresh) and HOME; workspace trusted
capabilities include session_attachments, session_mid_turn_message_mutation/_query
model local OpenAI-compatible mock, 40 chunks x 250 ms, echoes ACK<prompt>
browser Chromium via Playwright, 1180x900 @2x
injection page.route on GET /session/:id/pending-prompts: one route.abort('failed')
right after the resubmission's 202; no app code or daemon code modified
中文说明
在 head b759e70678 上重新验证 —— 真实 daemon + 真实浏览器（Linux）
本轮取代我 2026-09-12 那一轮（当时测的是 768f876421）。此后分支又进了 12 个 first-parent 提交，期间 main 又被合入三次：useQueuedPrompts.ts +324/−79，reconcile 测试 +1,695/−6，另有针对 R13-3 的协议文档改写（+259/−19）。我用同一套 harness 在当前 head 上、针对新的 merge base bc7a186cda 重跑了一遍；bot 仍标为未关闭的那条 Critical R13-2，此前只有读代码得出的结论，这次也放进真实产品里驱动了。
结论：修复在当前 head 上依然成立，且依然是真正起作用的。R13-2 真实存在、能在浏览器里复现，范围比 bot 说的略宽；它需要一次失败的确认请求才会触发，不丢数据，prompt 结束后自行消失。 从数据完整性角度看可以合并；我倾向把 R13-2 记为后续 issue，而不是在这份 1.7 万行的 diff 上再开一轮；如果反正还要再推一个提交，R13-2 就是该修的那一处。
Harness（与上一轮相同）
- 真实
qwen serve，在b759e70678上构建（npm run build+npm run bundle，均 exit 0）；真实 Chromium（Playwright）；本地 OpenAI 兼容 mock 模型，40 chunk × 250 ms。 - 竞态做成确定性的方法：挂起页面的 SSE 与活动轮询，直到 daemon 自己报告
{"activeWorkState":"idle","hasActivePrompt":false}；每次运行都断言此刻客户端仍显示Processing. New messages will be queued.。 - 两臂只在 web-shell 客户端上不同。
pr是 head 的 bundle（在 head 上重新构建与之逐字节相同）；base是同一棵树，仅把useQueuedPrompts.ts与QueuedPromptDisplay.tsx换成 merge basebc7a186cda的版本（cmp确认逐字节一致）。两臂使用同一个 daemon 二进制。
结果
| # | 检查项 | head b759e70678 |
base bc7a186cda |
|---|---|---|---|
| A | 在竞态窗口内发送文本追问 | ✅ 9/9 —— 重投、只执行一次、无报错 | ❌ Daemon rejected mid-turn message，消息丢失（模型侧 0 次） |
| B | 同一窗口内发送纯图片追问 | ✅ 7/7 —— 模型收到 1 条带图提示词，1 个气泡 | ❌ 报错，0 条带图提示词，0 个气泡 |
| C | 重投被排在另一客户端回合之后，再点该行 Delete | ✅ 10/10 —— 行显示 Queued on server...，DELETE 只针对排队中的 id，运行中的回合完整跑完 |
— |
| W | wire：空闲会话 | ✅ 两次 {"accepted":false,"reason":"session_idle"}；之后 mid-turn-messages / pending-prompts 为空 |
— |
| F1 | 上轮发现 1（重投回合并入上一块） | Processed 头，刷新后是两个 Processed 11s，turn-index 报告 2 个回合 |
— |
| F2 | 上轮发现 2（Esc to clear queue 未接线） |
— | |
| R13-2 | 附件行被困住 | ❌ 可复现（见下） | 不适用：base 直接丢掉这类消息 |
在真实产品里驱动 R13-2
构造方式。 同样的竞态窗口；浏览器重投进行中时，由第二个客户端占住 prompt FIFO，于是 daemon 把追问排进队列。然后只中断一个请求：重投拿到 202 之后的第一个 GET /session/:id/pending-prompts，也就是 refreshPendingPrompts 发起的那次确认。一次网络抖动就是这个效果；之后的每个 GET 都原样放行。随后再通过 REST 以不带 client id 的方式排入第三条 prompt（属于"可能是我的"，按理应出现在面板里）。
| 追问形态 | 确认 GET | 面板里的行 | 该行 Delete | daemon 上排队的第三条 prompt |
|---|---|---|---|---|
| 文件 + 文字 | 中断一次 | Submitting... 持续 14.6 s，直到追问结束 |
禁用，发出 0 个 DELETE |
19/19 次采样均不显示（约 12 s） |
| 图片 + 文字 | 中断一次 | Submitting... 持续 14.5 s |
禁用 | 19/19 次采样均不显示 |
| 仅图片 | 中断一次 | Queued on server... |
可用 | 显示 |
| 仅文字 | 中断一次 | Queued on server...（下一次同步按文本绑定） |
可用 | 显示 |
| 文件 + 文字 | 成功（对照组） | Queued on server... |
可用；被删的 prompt 从未执行 | 显示 |
| 文件 + 文字 | 仅 1.5 s 延迟、无失败 | Submitting... 约 1 s 后绑定 |
可用 | 显示（行尚未绑定时 5 次采样中有 1 次不显示） |
在被困住的运行里，被中断的那次之后又有 8 次 GET /pending-prompts 成功返回 200，没有一次绑定上该行；该行恰好在追问结束的那次采样时消失。第三条 prompt 从"不可见"直接变成"运行中"——在约 12 s 里，用户既看不到、也改不了、删不掉一条 daemon 仍在排队的消息。追问开始运行后，同一条消息在界面上出现两次：对话记录里显示为已发送，同时队列里还有一行动作全部失效的 Submitting...。
相对 bot 对 R13-2 的描述，补充几点：
- 范围比"带文件的行"更宽。 带文字的图片行也会以同样方式被困住：
matchesUnboundSubmittingRow会拒绝任何item.text非空的附件行，而 started 事件又不带content。只有纯文本行和纯图片行能恢复。 - 需要一次没有返回
refreshed的确认。 在事件流保持畅通、每个 GET 加 1.5 s 延迟的情况下，一次运行里该行没有被困住；我没有构造出自然发生的superseded。 - 不丢数据。 所有被困住的运行里，追问都恰好到达模型一次（mock 日志）、回显一次、没有报错提示。base 上同样的带文件追问根本不会被重投（模型侧 0 次）。所以这是在一条"以前会丢消息"的路径上的降级成功——但它仍是新增表面上一个用户可见的卡死。
- 姊妹测试
keeps an idle fallback submitting when its confirmation snapshot fails用的是纯文本行，恰恰是能恢复的那种形态，所以测试套件是绿的。
建议方向（我的判断，未验证）：当后续某个快照把 returnedUnboundPromptIdsRef 中记录的 prompt id 列为 queued 时，按 id 绑定那一行。这个 map 已经把 daemon 的 id 和本地行对应起来了，这样按 id 绑定就不再依赖"恰好是确认那一次快照成功返回"。
head 上的门禁（Linux）
| 目标 | 结果 |
|---|---|
packages/web-shell：useQueuedPrompts.midTurnReconcile + useQueuedPrompts.dom + QueuedPromptDisplay + daemon/session/actions 测试 |
4 个文件 / 566 通过 |
packages/acp-bridge src/bridge.test.ts |
940 通过 |
packages/sdk-typescript test/unit/DaemonClient.test.ts |
432 通过 |
packages/cli src/serve/server.test.ts |
1281 通过 |
根目录 npm run build / npm run bundle |
exit 0 / exit 0 |
b759e70 上的 CI 全部为绿，只有 bot 的 review-pr 仍在进行中。
Follow-up list: one addition, and the published version is superseded(Edited after a browser-level reproduction at The follow-up list published earlier on this PR has 25 items plus a section recovered from a killed review job. It now has 28. The three additions are the two unpinned-term records from the local audit rounds and the item below, which is the one a reviewer needs in order to judge R13-2 — the thread reply refers to it, so it is posted here rather than left in a scratch file.
The other two additions, for completeness: both terms of the removal replay's bound-row source ( Current head |
A mid-turn message the daemon refuses with `session_idle` is not a failure: the session can take an ordinary prompt instead. The web shell fell back to that route but then had no way to tell what the daemon did with the message, so it surfaced a rejection toast for a message that had run, or echoed it twice, or dropped it in silence — and a queue row could be left that the user could neither see nor remove. The fallback now confirms the resubmission against a pending-prompt snapshot instead of trusting the activity mirror, which another client's queued prompt can lag, and a row the daemon refused at idle carries that provenance through a hold and the drain so a later release confirms the same way. Echoes are sourced from the payload the client actually holds — a row bound by the daemon's id, a stash keyed by it, or the started event's own rendering — and every route that echoes leaves a record that outlives the settle, so no submit body appends the same message twice. A start that arrives while a removal is in flight parks, and every removal-failure arm replays it, so a message the daemon really ran is not lost because the user's cancel lost the race; it replays only this client's own messages, and never as the daemon's `[image]` placeholder. Attribution of a suppressed start to a row requires an event this client owns, because a rendering is not an identity. A row the drain has stamped but not yet released cannot own a snapshot entry or a started event, so it is out of every match count: it can no longer be spliced or bound to a prompt it is not, which read as a cancellation and bailed without posting, reporting or restoring anything. A cancellation that could not be confirmed is recorded and applied by the next snapshot that can prove something, and an owner change issues those removals rather than wiping them. Also documents the mid-turn resend rule after a refused close: waiting for the promotion is the default and is loss-free, and forcing the drain means releasing every still-queued id before re-posting any, because no DELETE verdict on its own establishes delivery.
b759e70 to
28cfb18
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving the current head after reading the round-12/13 disposition and the diff surface myself.
- Round 12's six Criticals are fixed with mutation-verified witnesses; round 13's R13-1 (originator condition on
attachmentMatches) and R13-3 (the self-contradicting resend rule) are genuinely fixed in b8ae494 / b759e70 — the thread replies cite the commits and I spot-checked them. - The one remaining open thread, R13-2 (a file-bearing or captioned follow-up left stamped
submittingwhen the confirming refresh is notrefreshed), was re-measured in a real browser against a real daemon: the wedge is bounded (the row clears when the follow-up settles, ~14.5 s in the report), no message is lost (it reached the model exactly once in every stranded run, versus zero at the merge base), and the trigger is narrow. Tracking it as follow-up #11507 with the bind-by-id direction recorded is the right call over another round on this diff. - The protocol surface (
reason: 'session_idle') is additive, absent-on-older-daemons, and documented on both the route comment and the published SDK type.
Note: CI on this head is still running (Test / Lint & Static / Integration pending) — approval here is on the code and disposition; merge should wait for green.
6 participants
What this PR does
When a message typed during a running turn is refused because the session has already gone idle, the daemon now says that explicitly instead of returning a bare refusal, and the browser reads that answer as "send this as an ordinary prompt" rather than "the send failed". Before acting on its own view of the queue, the browser confirms against the daemon's authoritative pending-prompt state: it renders the message as sent only when the daemon really took it, keeps the queue row when the daemon queued it behind another turn, and cancels a cleared follow-up only when the daemon confirms that follow-up is still waiting rather than already running. A confirmation that never arrives is treated as proof of nothing, so the row is left alone instead of being removed on a guess. The new response field is documented in the protocol reference and on the published client type, including what its absence means.
Why it's needed
A follow-up typed in the last moment of a turn could vanish from the composer and come back as a "Daemon rejected mid-turn message" error even though nothing had failed: the server's idle verdict reaches the browser before the browser's own streaming state updates, so the client concluded the message was lost and dropped it. The message had never been accepted, so dropping it was the wrong response — it should have been sent as the next prompt.
The opposite error was reachable too. Because the client guessed that an idle refusal meant its resubmission would start immediately, it could render the message as sent and delete the queue row while the daemon still held that message waiting behind another turn — leaving something on screen that cannot be edited or cancelled and later runs as a surprise. And when the user cleared the queue during that window, the client asked the daemon to delete the follow-up without knowing its state; deleting a prompt the daemon has already started cancels that live turn and also swallows the echo of the user's message.
Reviewer Test Plan
How to verify
Run the Web Shell against a daemon and start a turn that takes a few seconds. Type a follow-up and send it so that it lands just as the turn ends — the daemon refuses the mid-turn insert because the session is already idle.
Automated coverage added with this change: the idle-refusal fallback with and without attachments; the image-only placeholder binding, plus the refusals that keep a row from binding to a prompt it does not own (a foreign originator, a captioned rendering, a longer payload, a partially hydrated one, a file-bearing row); two identical follow-ups racing into one queue, including that both still reach the transcript; an explicit insert refused because the session went idle; a cleared follow-up across every removal outcome (daemon still queues it → removed; daemon already runs it → left alone; confirmation unavailable → left alone; DELETE lost → replayed and re-synced); and the echo guarantees for a message whose queue row a stale snapshot consumed, so an admitted message can neither go missing from the transcript nor appear as a bare placeholder. Locally on macOS after merging current
main:packages/web-shell305 files / 7451 tests,packages/acp-bridgebridge.test.ts936 tests,packages/sdk-typescript49 files / 1992 tests,packages/cliserve routes 7453 tests, plusnpm run build,npm run typecheckand repo-wide lint. Two notes on those runs: oneuseComposerCorepaste test fails only under parallel load and passes in isolation, and thepackages/clirun carries nine failures inscheduled-tasks.test.tsandvirtual-subagent-sessions.test.ts— both files are byte-identical tomainand cover scheduled tasks and subagent metrics, which this change does not touch.Evidence (Before & After)
No screenshots — the evidence below is wire and log level.
Before (daemon refusing a mid-turn insert at idle, client dropping the message):
After (same refusal, but stated, and the client keeps the message) — captured against a real
serveprocess built from this PR's commit9add20f0ca: isolatedQWEN_RUNTIME_DIR, random port64272, fresh thread session01b1c049-de2d-4ffc-be3e-6272c027479b, session-bound client idclient_311df798-…:All three refusals matched
{"accepted":false,"reason":"session_idle"}under a strict deep-equality assertion, and the snapshots afterwards confirm the refusal transfers no ownership — nothing queued, nothing promoted, no ordinary prompt created. The check exited 0 (WIRE_IDLE_REASON_PASS); the daemon was then stopped with SIGTERM (daemon stopped, port refusing connections afterwards). The browser half of the fix — resubmitting exactly once, keeping the queue row when the daemon queues the message, and never cancelling a turn that is already running — is covered by the hook-level cases above, not by a driven browser.Tested on
Environment
Local runtime:
npm ci+npm run buildagainst the currentmain, unit and integration suites viavitest. The live daemon capture usednode dist/cli.js serve --http-bridge --port 0 --token <random> --no-web --safe-mode --workspace <repo>with an isolatedQWEN_RUNTIME_DIR.Risk & Scope
Linked Issues
None — this was reported internally and no GitHub issue was filed for it.
中文说明
这个 PR 做了什么
当用户在回合进行中输入的消息因为会话已经进入空闲而被拒绝时，daemon 现在会明确说明原因，而不是只返回一个"未接受"；浏览器把这个答复读成"按普通提示词重新发送"，而不是"发送失败"。在依据自己对队列的判断行动之前，浏览器会先向 daemon 权威的 pending-prompt 状态确认：只有 daemon 确实接手了才把消息渲染为已发送；daemon 把它排在另一个回合后面时就保留队列行；只有在 daemon 确认该追问仍在排队（而不是已经在运行）时，才去取消用户清掉的追问。确认没有到达时视为"什么也没证明"，于是保留该行而不是凭猜测删除。新增的响应字段已写入协议参考文档与对外发布的客户端类型，包括"字段缺失意味着什么"。
为什么需要
在回合即将结束的瞬间发出的追问，可能从输入框消失并以 "Daemon rejected mid-turn message" 报错回来，尽管并没有任何失败：服务端的空闲判定比浏览器自身的流式状态更新更早到达，于是客户端认为消息丢失并把它丢掉。而这条消息其实从未被接受，丢掉它是错误的处理 —— 它本该作为下一个提示词发出去。
反向的错误同样可达。因为客户端猜测"空闲拒绝意味着重投会立刻开始"，它可能把消息渲染为已发送并删掉队列行，而 daemon 其实还把它排在另一个回合后面 —— 屏幕上留下一个既不能编辑也不能取消、之后还会突然自己跑起来的消息。另外，当用户在这个窗口内清空队列时，客户端会在不知道状态的情况下要求 daemon 删除该追问；删除一个 daemon 已经启动的提示词会取消那个正在进行的回合，并且连用户消息的回显一起吞掉。
审查测试计划
如何验证
用 Web Shell 连上 daemon，启动一个需要几秒的回合。输入一条追问并让它在回合结束的那一刻发出 —— daemon 会因为会话已空闲而拒绝这次 mid-turn 插入。
本改动新增的自动化覆盖：带附件与不带附件的空闲拒绝回退；纯图片占位文本绑定，以及阻止队列行绑定到不属于自己的 prompt 的各类拒绝（来源客户端不同、渲染带文案、附件数量更多、水合不完整、带文件的行）；两条相同追问同时进入一个队列（并验证两条都进入对话记录）；因会话转空闲而被拒绝的显式插入；"已清空的追问"在各种删除结果下的行为（daemon 仍在排队 → 删除；daemon 已在运行 → 不动；确认不可用 → 不动；DELETE 丢失 → 重放并重新同步）；以及"队列行被陈旧快照消费掉"的消息的回显保证 —— 已被接收的消息既不会从对话记录中消失，也不会以一个占位符气泡出现。本地（macOS，合并当前
main之后）：packages/web-shell305 文件 / 7451 测试、packages/acp-bridge的bridge.test.ts936 测试、packages/sdk-typescript49 文件 / 1992 测试、packages/cliserve 路由 7453 测试，加上npm run build、npm run typecheck与全仓 lint。两点说明：一个useComposerCore粘贴测试仅在并行负载下失败、单独运行通过；packages/cli那次运行有九个失败，集中在scheduled-tasks.test.ts与virtual-subagent-sessions.test.ts—— 这两个文件与main逐字节相同，覆盖的是定时任务与子代理指标，本改动没有触碰。证据（前后对比）
没有截图 —— 下面的证据是 wire 与日志层面的。
修改前（daemon 在空闲时拒绝 mid-turn 插入，客户端丢弃消息）：
修改后（同样的拒绝，但会说明原因，客户端保留消息）—— 针对本 PR 提交
9add20f0ca构建的真实serve进程抓取：独立QWEN_RUNTIME_DIR、随机端口64272、全新 thread 会话01b1c049-de2d-4ffc-be3e-6272c027479b、绑定会话的 client idclient_311df798-…：三次拒绝都在严格深比较断言下等于
{"accepted":false,"reason":"session_idle"}，其后的快照确认拒绝不转移所有权 —— 没有入队、没有提升、也没有创建普通提示词。检查程序退出码 0（WIRE_IDLE_REASON_PASS）；随后用 SIGTERM 停止 daemon（日志daemon stopped，端口之后拒绝连接）。修复的浏览器侧行为 —— 只重投一次、daemon 排队时保留队列行、绝不取消已经在运行的回合 —— 由上面的 hook 级用例覆盖，不是通过驱动真实浏览器验证的。测试环境
运行环境
本地：针对当前
main执行npm ci+npm run build，用vitest跑单元与集成套件。真实 daemon 抓取使用node dist/cli.js serve --http-bridge --port 0 --token <random> --no-web --safe-mode --workspace <repo>，并配合独立的QWEN_RUNTIME_DIR。风险与范围
关联 Issue
无 —— 该问题来自内部反馈，没有为它创建 GitHub issue。