feat(web-shell): expose assistant turn settlement lifecycle - #11251
feat(web-shell): expose assistant turn settlement lifecycle#11251yiliang114 wants to merge 32 commits into
Conversation
|
Thanks for the PR — re-run of the gate at the current head. Template looks good ✓ — every required section is present, including the bilingual block and the Risk & Scope tradeoffs. Problem: real and tracked, not theoretical hardening. This is a Direction: aligned. Web Shell is an embeddable surface that already exposes a family of host callbacks ( Size: Stage 0 does not apply. Every changed file is under Approach: scope feels right, and a 1056:449 test-to-production ratio is unusually thorough for this repo. One design question, which turns out to be the same thing as the blocking finding in Stage 2: delivery for the journal-repair case is routed through the replay admission gate instead of retaining the terminal the provider already observed live. Holding the observed settlement in a ref and publishing it once the repair finishes — whether it succeeds or fails — looks materially simpler than the current path and would not need an admission key at all. Worth a look before the code review settles it. Nothing here reads as scope creep: the diff is one feature, its tests, its design doc, and the README row for it. No drive-by refactors. Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！这是针对当前 head 的 gate 重跑。 模板完整 ✓ —— 所有必需章节都在，包括中文说明与 Risk & Scope 取舍。 问题： 真实且已有跟踪，不是理论性加固。这是一个 方向： 对齐。Web Shell 本身就是可嵌入组件，已经有一组宿主回调（ 规模： Stage 0 不适用。所有改动文件都在 方案： 范围合理，测试与生产代码 1056:449 的比例在本仓库里算是相当扎实的。有一个设计疑问，它和 Stage 2 的阻塞性发现其实是同一件事：journal repair 场景下的投递被绕到了 replay admission gate，而不是保留 provider 已经在 live 流上观测到的那个终态。把已观测到的 settlement 存进 ref、等 repair 结束后（无论成功还是失败）再发布，看起来比现在这条路径明显更简单，而且完全不需要 admission key。建议在代码审查定论之前先考虑一下。 diff 里没有夹带无关改动：就是一个功能、它的测试、它的设计文档，以及 README 里对应的一行。没有顺手重构。 风险： 无升级风险信号——改动文件都没有命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewReviewed the full production diff at 🔴 Critical — a live-observed terminal is silently dropped when it is also a journal-repair target
The live path withholds it ( The only re-delivery path is the replay loop, and it is gated on this provider's own admission key (
Then This is not a new objection, and the bot has form on getting it wrong here. The same mechanism was recorded as a deferred Simplest reachable trigger — no Split View required:
The second provider case the design doc explicitly sanctions (main chat + Split View pane on one session) hits the same path for whichever pane did not submit. Two further loss paths for the same withheld terminal, independent of the admission gate: the repair failure branch ( This contradicts the contract the PR itself documents in Suggested shape: retain the withheld settlement (a ref keyed by session) and publish it when the repair episode ends — on successful replay publication, on the failure branch, and on the abort/dispose guards — instead of depending on the replay admission gate. That keeps "publish after the terminal projection is committed" (the reason for withholding) without making delivery conditional on submitter ownership, which the design doc already says hosts must not rely on. Test gap: none of the 22 new test cases touches the repair path. Non-blocking (deferred, not counted against the verdict)Per the repo's five-round rule, these are recorded so nothing is silently dropped, and are not reasons to hold the PR:
Test evidence — the PR's own CIThis was an unattended run, so per the gate's rules no PR-derived code was built, executed, or checked out. The evidence below is the PR's own CI at the reviewed commit, read through the API.
Two ways the green board does not cover this changeWorth stating plainly, because "CI is green" reads as stronger here than it is:
No PR-CI workflow run is still pending on this head, so this table is settled rather than a snapshot mid-flight. The macOS and Windows Not verified: the delivery behaviour itself. The author states plainly in the PR body that no local validation was run — all three "Tested on" rows are Sandboxed verification would settle this: 中文说明代码审查在 🔴 Critical —— 当 live 观测到的终态同时是 journal repair 的目标时，它会被静默丢弃
live 路径会把它扣住（ 唯一的补发路径是 replay 循环，而它以本 provider 自己的 admission key 为门槛（
随后 这不是一个新提出的异议，而且 bot 在这件事上有过判断失误。 同一机制在第七轮就被记录为一个延后的 最简可复现路径 —— 不需要 Split View：
设计文档明确允许的第二 provider 情形（主聊天 + Split View pane 挂同一 session），对没有提交该 prompt 的那个 pane 会走到同一条路径。 同一个被扣住的终态还有两条额外的丢失路径，与 admission gate 无关：repair 失败分支（ 这与 PR 自己在 建议的形态： 把被扣住的 settlement 保留下来（按 session 存进一个 ref），在 repair episode 结束时发布——replay 发布成功时、失败分支上、以及 abort/dispose 守卫处——而不是依赖 replay admission gate。这样既保住了"在终态 projection 提交后再发布"（也就是扣住它的原因），又不会让投递取决于提交者归属，而设计文档本身已经说明宿主不应依赖该归属。 测试缺口： 22 个新增测试用例没有一个触及 repair 路径。 非阻塞（已记录，不计入结论）按仓库的五轮规则，以下记录下来以免被静默丢弃，它们不构成压住 PR 的理由：
测试证据 —— PR 自身的 CI本次为无人值守运行，因此按 gate 规则没有构建、执行或检出任何 PR 派生代码。下面的证据是在被审查 commit 上通过 API 读取的 PR 自身 CI 结果。
绿板并未覆盖本改动的两处需要明确说出来，因为这里的"CI 是绿的"读起来比实际情况更强：
CI 表格见上方英文区（已由 region marker 包裹，便于 CI 落定后就地更新）。本 head 上没有仍在 pending 的 PR-CI workflow run，因此该表是已落定的结果，而非中途快照。macOS 与 Windows 的 未验证： 投递行为本身。作者在 PR 正文中明确说明没有做本地验证——三行 "Tested on" 全是 沙箱验证可以定这件事： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 2/5 — the feature is well-constructed and honestly documented, but its central promise breaks in a reachable case, and that case is the one part of the delivery path with no test, no integration lane, and no typecheck. Stepping back: this is good work. The block-layer selection in the projection module, the refusal to spread internal fields into the host contract, the reuse of Against my own independent proposal for the same problem (emit from the provider that observes the terminal, after the transcript commit, dedupe on What makes this a blocker rather than a nit is that it contradicts the contract the PR writes down for itself. The design doc promises every live-observed terminal is published; the code publishes every live-observed terminal except the repair target, and republishes that one only for the submitter. The multi-provider case the same doc explicitly sanctions is one of the ways to be a non-submitter, and a plain page reload during someone else's running turn is another — no exotic interleaving needed. I checked this against the code at the reviewed commit rather than taking the prior review's word for it: two real publish sites, one bind site, repair armed purely from truncation detection, I also want to correct this gate's own record. An earlier pass of this triage argued the opposite conclusion — that suppressing the settlement leaves the prompt bound, so the repair's reload republishes it. That is true for an admitted prompt and false for any other, and the distinction is the entire bug. The mechanism was flagged as a deferred It also fails in the direction that is hardest to notice. The suite is green, CI is green, and none of the 22 new tests touches On the round count: this PR has been through roughly twelve rounds and the repo's own guidance is to land only Critical fixes past round five and defer the rest. I have applied that — the Suggestions in Stage 2 (public-contract ownership, missing Verdict: request changes. No approval, and no deferred-approval instruction to the finalize job — the verdict is not approve, so there is nothing for CI to unblock. No new review was submitted on this re-run. The PR already carries ten standing @yiliang114 — the fix looks small relative to everything already done here: retain the withheld settlement and publish it when the repair episode ends (success, failure branch, and the abort/dispose guards) instead of routing it through the replay admission gate, then add a case to the existing truncated-live-turn fixture asserting the host receives it. Given the lane gaps above, 中文说明Confidence: 2/5 —— 功能本身构建得不错、文档也诚实，但它的核心承诺在一个可达场景下失效，而这个场景恰好是整条投递路径上唯一既没有测试、也没有 integration 环节、也没有类型检查的部分。 退一步看：这是高质量的工作。projection 模块在 block 层做选取、拒绝把内部字段 spread 进宿主契约、复用 对照我自己针对同一问题的独立方案（由观测到终态的 provider 发出、在 transcript 提交之后、按 之所以这是阻塞项而不是小瑕疵，是因为它与 PR 为自己写下的契约相矛盾。设计文档承诺每个 live 观测到的终态都会发布；代码发布的是每个 live 观测到的终态除了 repair 目标，而那一个只对提交者补发。同一份文档明确允许的多 provider 情形，正是成为"非提交者"的途径之一；而在别人正在跑的 turn 期间刷新一次页面，是另一条途径——不需要什么罕见的交错。 我是对照被审查 commit 的代码核实这一点的，而不是采信先前 review 的说法：两个真正的发布点、一个 bind 点、repair 完全由截断检测装配、 我还要更正本 gate 自己的记录。 早先一轮 triage 给出过相反的结论——认为抑制 settlement 会让 prompt 保持 bound，于是 repair 的 reload 会把它补发出来。这对 admitted 的 prompt 成立，对其它任何 prompt 都不成立，而这个区别正是整个 bug。该机制在第七轮就被标记为延后的 它的失效方向也是最难被发现的那种。测试套件是绿的，CI 是绿的，22 个新增测试没有一个触及 关于轮次：这个 PR 已经走了大约十二轮，而仓库自己的指引是超过五轮之后只落 Critical 修复、其余延后。我按此执行——Stage 2 里的几条 Suggestion（公开契约的归属、缺 结论：request changes。 不批准，也不给 finalize 任务留任何延迟批准指令——结论不是 approve，因此没有可供 CI 解锁的东西。 本次重跑没有提交新的 review。 该 PR 已经带有本 bot 的十个仍在生效的 @yiliang114 —— 相对于这里已经完成的工作量，修复看起来不大：把被扣住的 settlement 保留下来，在 repair episode 结束时（成功、失败分支、以及 abort/dispose 守卫处）发布它，而不是把它绕经 replay admission gate；然后在已有的 truncated-live-turn fixture 里加一个用例，断言宿主确实收到它。鉴于上面的环节缺口，在下一个 head 上跑一次 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 Needs some rethinking — full notes in the three staged comments above. The short version, two blockers:
1. It does not compile. promptSettledFromTurnEvent passes data: Record<string, unknown> | undefined into the file's local getString(value: Record<string, unknown>, key: string), at lines 4835 and 4841 of DaemonSessionProvider.tsx. Two TS2345 errors — Lint & Static and Capture web-shell visuals are both red on them, and the visuals job fails prepare through tsc -p tsconfig.lib.json. getString(data ?? {}, ...) settles it.
2. useDaemonPromptSettled throws unconditionally, and that breaks two existing suites. App() now calls it as its first statement regardless of whether onAssistantTurnSettled was passed, and it throws when DaemonPromptSettlementContext is absent. App.test.tsx renders <App /> bare and mocks only the daemon-react-sdk barrel — but the new module imports the hook directly from ./daemon/session/DaemonSessionProvider.js, which that mock does not cover, so the real hook sees an undefined context. SplitView.test.tsx has the same shape with the new observer inside a mocked provider rendered as a plain <div>. Since the prop is optional, opting out should not create a hard provider requirement — a no-op when subscribe is undefined keeps the throw for the case that matters.
Also worth a look while you are in there: turn_complete with stopReason: 'error' is classified as outcome: 'completed', and docs/developers/qwen-serve-protocol.md lists error as a valid turn_complete stop reason. And the contract this PR adds has no test pinning any of its six claimed properties — DaemonSessionProvider.test.tsx already has the machinery to drive turn_complete / turn_error.
To be clear, none of this is doubt about the feature. The direction is well-supported (#10389 asked for exactly this contract, the protocol doc already tells integrators to correlate terminals by promptId, and the Java SDK already exposes the same idea), the scope is tight with no creep, and the API shape is one I would be happy to maintain. It needs to build, not regress the harnesses it renders into, and carry a test for what it adds.
中文说明
需要再调整一下——完整意见见上面三条分阶段评论。简要说，两个阻塞项：
1. 编译不过。 promptSettledFromTurnEvent 把 data: Record<string, unknown> | undefined 传给了文件内的本地 getString(value: Record<string, unknown>, key: string)，位置在 DaemonSessionProvider.tsx 第 4835、4841 行。两个 TS2345——Lint & Static 与 Capture web-shell visuals 都因此变红，且 visuals 任务是通过 tsc -p tsconfig.lib.json 让 prepare 失败的。改成 getString(data ?? {}, ...) 即可。
2. useDaemonPromptSettled 无条件抛错，会打挂两个既有测试套件。 App() 现在把它作为第一条语句调用，无论是否传入 onAssistantTurnSettled；而 DaemonPromptSettlementContext 缺失时它会抛错。App.test.tsx 裸渲染 <App /> 且只 mock 了 daemon-react-sdk barrel——但新模块是直接从 ./daemon/session/DaemonSessionProvider.js 导入该 hook 的，不在那个 mock 覆盖范围内，于是真实 hook 拿到 undefined context。SplitView.test.tsx 是同样的形态：新 observer 位于被 mock 成普通 <div> 的 provider 内部。既然这个 prop 是可选的，"不启用"就不该产生硬性 provider 依赖——在 subscribe 为 undefined 时 no-op，就能把抛错保留给真正需要它的场景。
顺手也值得看一眼：turn_complete 且 stopReason: 'error' 会被分类为 outcome: 'completed'，而 docs/developers/qwen-serve-protocol.md 明确把 error 列为 turn_complete 的合法 stop reason。另外本 PR 新增的契约没有任何测试钉住它声称的六条属性——DaemonSessionProvider.test.tsx 已经有驱动 turn_complete / turn_error 的现成机制。
需要说清楚的是，这些都不是对功能本身的怀疑。方向有充分支撑（#10389 要求的正是这套契约、协议文档已经要求集成方按 promptId 关联终态、Java SDK 也已暴露同样的概念），范围收紧无夹带，API 形态也是我乐意维护的。它只是需要能编译、不要打挂它所渲染进的夹具、并为新增内容带上一个测试。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at ba3ee481db2597dca283e7b18bf10f426fdcdcee · re-run with @qwen-code /triage
|
Addressed the current review findings in |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 5 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Follow-up static self-audit found one remaining opt-out path: Split View mounted the settlement observer even when the optional host callback was absent, so existing lightweight embeddings/mocks could still call unavailable transcript hooks. Fixed in |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- missing tests for the new settlement contract (assistant-turn-settlement.ts, DaemonSessionProvider.tsx:3486) — already reported (review 5128055949; author confirmed unaddressed in comment 5565319059)
- unbounded publishedPromptSettlementsRef ledger (DaemonSessionProvider.tsx:1067) — already reported as a minor non-blocking note (review 5128055949)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the area the live-journal repair finding touches.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the area the live-journal repair finding touches.
— qwen3.8-max via Qwen Code /review (v0.23.0)
- project the settled message off the raw blocks with the SDK's own final-visible-assistant predicate; a promptId-filtered re-projection through the render adapter dropped every tool block of the turn (the reducer never stamps promptId on tool blocks), merged the turn's text halves under the first block's id, and promoted subagent-owned text - publish the settlement where a replayed terminal settles a locally bound prompt: the snapshot is released after injection and SSE resumes from lastEventId, so that terminal never reaches the live-loop publish and the host callback never fired for it - default the settlement error code to 'turn_error', matching the DaemonHttpError that matchTurnEvent rejects the submitter with - build the published host event field by field so an internal-only field cannot widen the public contract silently Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtrow8ztpw
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- promptSettledFromTurnEvent re-implementing the SDK's matchTurnEvent mapping (packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:4860) — already reported as R1-9 (comment 3949408399), whose concrete drift instance this round …
- selector duplicating the SDK's module-private final-visible-message predicate (packages/web-shell/client/assistant-turn-settlement.ts:45) — already voiced inside R1-1 (comment 3949408367), which the author answered
- unbounded publishedPromptSettlementsRef ledger (packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:1076) — already reported as a minor non-blocking note in review 5128055949
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this round's replay publish changes.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
docs/design/web-shell/web-shell-assistant-turn-settlement.md:44 — [review] 5 of 6 Verification bullets claim coverage no test providespackages/web-shell/client/components/SplitView.tsx:491 — [review] Side-task provider really submits prompts but forwards no…
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this round's replay publish changes.
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
Addressed the current blocking replay gap and the two host-contract mismatches in |
|
Addressed R1-15 in bb506df: the settlement projection now omits a candidate assistant message while it is still streaming, so hosts cannot persist partial content as a final settled answer. This is a one-line guard in the shared projection path. Validation was limited to repository-pinned formatting, |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- promptSettledFromTurnEvent re-implementing the SDK's matchTurnEvent mapping (packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:4875) — already reported as R1-9 (comment 3949408399); round 2 confirmed and dropped it as a dup…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this round's replay publish changes.
Not explored to full depth (tool budget reached): "agent 3b": did not execute the reconnect-replay scenario to confirm the rebuilt snapshot leaves streaming: true on the final assistant block (Finding 1's premise); read-….
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:4886 — [probe] Outcome classification fails open: 3 of 5 ACP stop reasons publish as 'completed' (deferred: anchored on code unchanged since the previous round)packages/web-shell/client/App.tsx:17403 — [probe] Split View pane→host forwarding untested while the design doc claims it verified (re-deferral of round 2's Verification-bullets entry; anchored on unchanged code)
Convergence: round 3 posted 9 inline comment(s), 8 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/web-shell/client/assistant-turn-settlement.ts (findings in round 2; 4 more now); packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx (findings in round 2; 3 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this round's replay publish changes.
未探索到全部深度（达到工具调用预算）："agent 3b"：did not execute the reconnect-replay scenario to confirm the rebuilt snapshot leaves streaming: true on the final assistant block (Finding 1's premise); read-…。
未审查：反向审计——评审时间预算不足，未能开始第 6 轮。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 9 条行内评论，其中 8 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/assistant-turn-settlement.ts（第 2 轮已出过发现，本轮又有 4 条）；packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx（第 2 轮已出过发现，本轮又有 3 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
Retire locally-bound prompt keys whose terminal is destroyed rather than delivered. An epoch reset records the bound prompt and, after the reload's replay injection, publishes a `failed` settlement when the fresh snapshot carried no terminal for it; auth/terminal-error cleanup and heartbeat-detected missing sessions retire the bound prompt directly. The removePendingPrompt stale-session branch now notifies prompt removal with the foreign session id so the ledger delete keys the right session, and the settlement projection restores the whitespace-only assistant block skip. Add discriminating tests: replayed terminal after epoch reset, failed retirement when the terminal is destroyed, replayed cancelled terminal, and stale-session removal notification. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtsl1npgrb
…g the onAssistantTurnSettled row Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtss6ux5rn
Resolve split-view conflicts by keeping both the PR's onAssistantTurnSettled hook and main's showSessionDetails / onPendingPanesChange wiring. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtt0ri0as1
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- live-journal repair ordering at packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3542 — already reported as R1-6 (comment 3949408393)
- promptId scoping unpinned in packages/web-shell/client/assistant-turn-settlement.test.tsx — already reported as R2-3 (comment 3957009205)
- goal-continuation terminals at packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3547 — already reported as R1-23 (comment 3949408436)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this PR's replay publish changes.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": constructing a production producer whose terminal frame carries an envelope promptId that disagrees with (or is present without) data.promptId — the harm ha…; "agent reverse-audit (round 5)": running the test file or any mutation probe; every discrimination claim above is by inspection of the fixtures and the two readers.; "agent reverse-audit (round 5)": sweeping the rest of the 20k-line test file for a pre-existing fixture that envelope-stamps a terminal frame while a settlement listener is attached — only the …; "agent 1b": none — all planned checks completed (~15 tool calls).; "agent reverse-audit (round 4)": whether the daemon ever completes a *successful* cancel without a following turn_complete{stopReason:'cancelled'} — I read broadcastPromptCancelled 's contra…, and 2 more.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/App.tsx:2978 — [probe] The main-chat entrance has no test at allpackages/web-shell/client/App.tsx:2978 — [probe] The side-task provider submits prompts but has no observer…packages/web-shell/client/assistant-turn-settlement.test.tsx:24 — [probe] Nothing pins that the session id is read at delivery, not…packages/web-shell/client/assistant-turn-settlement.test.tsx:91 — [probe] Nothing pins that blocks are read inside the listener, not…packages/web-shell/client/assistant-turn-settlement.test.tsx:167 — [probe] The subagent-text test is vacuous w.r.t. the filter it pinspackages/web-shell/client/assistant-turn-settlement.ts:118 — [probe] No test joins a real provider terminal to the real…packages/web-shell/client/components/SplitView.tsx:491 — [probe] Neither production wiring of the new public prop is tested…packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10284 — [probe] Session-switch survival of the admission key is unwitnessedpackages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10284 — [probe] No replayed turn_error for a bound prompt exists anywherepackages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10663 — [probe] The envelope-first promptId reader the publish path uses…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:1108 — [probe] The publish-once dedup guard is documented as verified but…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3542 — [probe] The projection-before-callback ordering guarantee is…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3544 — [probe] Nothing pins that the live publish is NOT admission-gatedpackages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:4475 — [probe] A restored prompt was never admitted, so a replay-only…
Convergence: round 4 posted 6 inline comment(s), 4 of them reported for the first time; the previous round posted 9 (8 new). Findings keep coming back to the same files: packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx (findings in round 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this PR's replay publish changes.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 5)"：constructing a production producer whose terminal frame carries an envelope promptId that disagrees with (or is present without) data.promptId — the harm ha…；"agent reverse-audit (round 5)"：running the test file or any mutation probe; every discrimination claim above is by inspection of the fixtures and the two readers.；"agent reverse-audit (round 5)"：sweeping the rest of the 20k-line test file for a pre-existing fixture that envelope-stamps a terminal frame while a settlement listener is attached — only the …；"agent 1b"：none — all planned checks completed (~15 tool calls).；"agent reverse-audit (round 4)"：whether the daemon ever completes a *successful* cancel without a following turn_complete{stopReason:'cancelled'} — I read broadcastPromptCancelled 's contra…，另有 2 条。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 14 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 6 条行内评论，其中 4 条是首次提出；上一轮发布了 9 条（其中 8 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx（第 3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Round 5 review (re-check at HEAD 0eab4a8)
This round re-checks the 6 standing blockers from the previous round and the verify-bot findings against the current head. No Criticals. Three medium-confidence notes below, all of which the PR already documents or defers.
Blocker re-check
All 6 round-4 blockers verified fixed by this diff:
| ID | Severity | Title | Verdict |
|---|---|---|---|
| R3-1 | Critical | Destroyed terminal never settles | Fixed. retireAbandonedPrompt + epochResetBoundPromptRef + post-replay sweep publish failed settlements for epoch-reset, session-error, and session-missing paths |
| R3-2 | Suggestion | Stale-session turn-navigation guard untested | Fixed. 7298af7aac added test does not record a stale-session removal in the current session turn navigation — spies recordPromptRemoved, asserts never called for foreign session |
| R3-3 | Suggestion | Epoch-reset test discards submitter promise | Fixed. Changed from void prompt.catch(() => {}) to await expect(pending).resolves.toEqual(...) — contradictory cancelled/completed verdicts are now observable |
| R3-4 | Suggestion | No streaming-guard test | Fixed. faac6438f added two fixtures: block-level streaming guard + merged-message streaming guard; both pin their respective guards |
| R4-1 | Suggestion | Three near-verbatim resync fixtures | Fixed. 0be25ba5 extracted createResyncReplayFixture() |
| R4-2 | Suggestion | Stale-session removed guard unwitnessed |
Fixed. b946ccd8c added test driving removed: false through removePendingPrompt and asserting onPromptRemoved not called |
Key correctness checks against the current code
- stopReason → outcome mapping —
promptSettledFromTurnEventcorrectly mapsstopReason === 'error'tofailed,cancelledtocancelled, and everything else tocompleted. ✅ - Final message projection —
getSettledAssistantMessageusestranscriptBlocksToDaemonMessages(blocks, { includeSourceIdentity: true })and scans bysourceBlockIds, not apromptId-filtered subset. ✅ - Both streaming guards — block-level (
blocks.some(block => block.streaming)) and message-level (message.isStreaming) — are present and tested. ✅ - Abandoned prompt retirement — wired to all three declared paths:
epoch_reset(post-replay sweep),session_error(auth/terminal error handler),session_missing(heartbeat handler). ✅ - Per-provider dedup —
publishedPromptSettlementsRefsuppresses duplicates by(sessionId, promptId)key. ✅ - Stale-session removal guard —
onPromptRemovedfires only whenresult.removed === true; test pins theremoved: falsebranch. ✅ - App + SplitView wiring —
AppcallsuseAssistantTurnSettlementProjection(onAssistantTurnSettled)at component top;SplitViewmountsAssistantTurnSettlementObserverper pane, gated on the callback being present. ✅
Remaining items (all Suggestion-level, per the five-round convention)
-
clearSession/clearActiveSessionStatedoes not retire the new refs — same gap the triage bot flagged (Suggestion 2 in its Stage 2).locallyBoundPromptKeysRefandpublishedPromptSettlementsRefsurviveclearActiveSessionState(), so a prompt killed by session clear leaves a dedup set entry and a host waiting on that(sessionId, promptId)never hears back. Not filed as Critical becauseclearActiveSessionStatealso aborts the active controller — the prompt stops running — and a host that initiatesclearSessionis unlikely to be listening for settlement on the cleared session. The design doc enumerates three abandonment paths (epoch_reset,session_error,session_missing) but omits this fourth one. Worth wiring before the surface stabilises. -
App.test.tsxmock declaration is dead weight at this head — themockUseDaemonActivePromptBridgehunk (+7 lines) was declared to satisfy an assertion inherited from the pre-merge base. Main has since fixed that assertion in #11406 by pointing it atmockUseDaemonSessionActivityBridge. Nothing at the merged0eab4a8head reads the added mock; dropping the whole hunk is the cleanest outcome. The triage bot called this in Stage 2 — repeating it only because it persists at the current head. -
No typecheck lane in PR CI compiles
packages/web-shell— the triage bot noted this at confidence 3/5 (its only block besides the stale base, which is now merged). Vitest strips types, ESLint does not check them, and the browser jobs drivenpm run devnottsc. Runningnpm run typecheck --workspace=packages/web-shelllocally, or triggering@qwen-code /verify, would confirm the new type surface compiles.
Summary
The feature is well-structured, the fix history converges properly, and every prior Critical and Suggestion is genuinely resolved at this head. I see nothing that would block merge after CI is green and the typecheck is confirmed.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review
This is a post-round-4 review of PR #11251 at head 0eab4a848592. The PR has been through four full /review rounds (all CHANGES_REQUESTED) plus a triage re-run that reported confidence 3/5 ("the code is in good shape and every earlier finding is genuinely fixed; what blocks is a stale base and a typecheck that no CI lane runs").
Blocker re-check (round 4 ledger)
I verified each of the 6 standing blockers against the current diff:
| ID | Severity | Verdict | Evidence |
|---|---|---|---|
| R3-1 | Critical | Fixed | retireAbandonedPrompt + epochResetBoundPromptRef + post-replay sweep publish failed/epoch_reset when no terminal arrives; wired to session_error and session_missing paths. Witness test present. |
| R3-2 | Suggestion | Fixed | onPromptRemoved widened to 3 args; sessionId === undefined guard protects turn-navigation. Witness test spies recordPromptRemoved. |
| R3-3 | Suggestion | Fixed | Epoch-reset + cancelled tests now await expect(pending).resolves.toEqual(...), pinning contradictory verdicts. |
| R3-4 | Suggestion | Fixed | Both streaming guards added with witness tests (block-level + message-level). |
| R4-1 | Suggestion | Fixed | createResyncReplayFixture extracted, three tests share one fixture. |
| R4-2 | Suggestion | Fixed | Test driving removed: false asserts onPromptRemoved not called. |
No remaining Criticals
I verified the key behavioral guarantees against the current diff:
turn_completewithstopReason: 'error'maps tooutcome: 'failed'.getSettledAssistantMessageuses the full-transcript adapter withincludeSourceIdentity: true, matching viasourceBlockIds— not a promptId-filtered re-projection.- Both streaming guards present: block-level scan + message-level
isStreamingcheck. Two test fixtures pin each. retireAbandonedPromptwired to all three abandonment paths: epoch-reset, session_error, session_missing.publishPromptSettlementdeduplicates viapublishedPromptSettlementsRef.- Stale-session removal guarded by
if (result.removed)beforeonPromptRemovedfires. App.test.tsxmock declaration fixes the inherited ReferenceError that existed at the PR's base commit.
Known deferred item
The clearSession/clearActiveSessionState gap (triage Suggestion 2 — locallyBoundPromptKeysRef and publishedPromptSettlementsRef are not cleared on session reset) is still present. Per the five-round convention, recorded and deferred for a follow-up.
Summary
No new Criticals. All 6 round-4 blockers verified fixed. The remaining open inline comments are Suggestion-level, appropriately deferred per convention after 5+ review rounds. The stale-base concern from the triage is resolved by the merge from main at 0eab4a8.
|
⏳ Historical-head review — head moved to 中文说明⏳ 历史 head 评审 —— 本次评审进行中 head 移动到了 |
|
Resolved the three standing Critical findings in commit
Deferred under the repository's >5-round convergence rule: provider-unmount lifetime semantics (R5-5), additional abandonment/insight/ownership/removal/live-journal fixture coverage (R5-4, R5-8, R5-7, R5-6, R5-9, R5-10, R4-1), and the heartbeat auth error-code taxonomy cleanup (R5-3). These are recorded here rather than expanding this PR again. No local test, build, typecheck, or CI command was run per the requested workflow. The final diff was inspected and |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Blocking finding(s) follow.
Partially reviewed — gaps disclosed.
10 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- transcript read-timing unpinned in the projection suite (assistant-turn-settlement.test.tsx:91) — already reported (round-4 deferral test:91, review 5146822401; re-listed in the round-5 body)
- SideTaskPanel/SubagentDetail provider surfaces publish to no subscriber (App.tsx:17799) — already reported (round-2 deferral SplitView:491, review 5136158773; round-4 deferral App:2978, review 5146822401)
- publish-once dedup guard untested (DaemonSessionProvider.tsx:1118) — already reported (round-4 deferral DSP:1108, review 5146822401)
- App/SplitView callback wiring untested (App.tsx:2979) — already reported (round-4 deferrals App:2978 and SplitView:491, review 5146822401)
- the subagent-ownership test does not pin the parentToolCallId filter (assistant-turn-settlement.test.tsx:148) — already reported (round-4 deferral test:167, review 5146822401)
- flushTranscriptSync pre-publish ordering and the live turn_complete arm unpinned (DaemonSessionProvider.tsx:3635) — already reported (round-4 deferrals DSP:3542 and DSP:3544, review 5146822401; open R1-6, comment 3949408393)
- outcome-mapping arms untested (DaemonSessionProvider.tsx:5032) — already reported (round-5 body, review 5151330185, as the outcome-mapping arm DSP:3612; round-3 deferral DSP:4886, review 5140647872)
- unbounded publishedPromptSettlementsRef ledger (DaemonSessionProvider.tsx:1119) — already reported (minor non-blocking note in review 5128055949; re-listed in the round-2 and round-3 bodies)
- nothing pins that the live publish is NOT admission-gated (DaemonSessionProvider.test.tsx:10771) — already reported (round-4 deferral DSP:3544, review 5146822401)
- always-latest-listener ref indirection unpinned (DaemonSessionProvider.tsx:5443) — already reported (round-5 deferral DSP:5400, review 5151330185)
Unresolved, please confirm:
- [Critical] DaemonSessionProvider.tsx:2903 (ledger R3-1, comment 3965944374) — cannot tell whether the post-replay sweep publishes a false
failedfor a prompt still in flight. The client-side code fact is verified: the sweep gates only on the parked …
Not reviewed: reverse audit of the degraded/partial replay-snapshot trigger for the epoch-reset retirement (shouldInjectReplaySnapshot's dependence on replayDegraded / replaySnapshotComplete / replayError) — stopped at the agent tool budget.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this PR changes most.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": running packages/web-shell/client/App.test.tsx to settle by execution whether the added mockUseDaemonActivePromptBridge is load-bearing (real ChatPane.tsx:2…; "agent reverse-audit (round 1)": did not execute the six new tests ( npx vitest run client/daemon/session/DaemonSessionProvider.test.tsx ) — every claim above about what each test pins comes fr…; "agent reverse-audit (round 1)": did not trace whether actions.cancel() reaches removePendingPrompt → onPromptRemoved (actions.ts:2463) for a queued prompt; I verified only that cancel …; "agent reverse-audit (round 1)": did not examine shouldInjectReplaySnapshot 's dependence on replayDegraded / replaySnapshotComplete / replayError , so the degraded-or-partial-snapshot tr…; "agent reverse-audit (round 1)": did not measure whether the two duplicate payloads actually differ in message (argued from the pane's maxBlocks / subagentTranscriptMode props vs the outer …, and 8 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:1142 — [review] Critical [fails-closed] [new-surface] R5-2 both abandonment retirements — and the epoch-reset arming at :3296-3302 — resolve the victim through activePrompt…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:2911 — [review] Critical [fails-closed] [new-surface] R5-3 the epoch-reset retirement has one unkeyed one-shot slot consumed at a single conditional site and cleared uncondi…docs/design/web-shell/web-shell-assistant-turn-settlement.md:1 — [review] The new design doc ships English-only, with no .zh-CN.md counterpart and no reciprocal language links below the title, which docs/design/README.md and AGENTS.md req…docs/design/web-shell/web-shell-assistant-turn-settlement.md:27 — [review] The documented host correlation names a prompt id that no public submit surface exposes to a host that can also receive the settlement — the id is first delivered by…docs/design/web-shell/web-shell-assistant-turn-settlement.md:40 — [review] The sentence recording the decision not to read promptTerminals overstates what that field guarantees — the ledger behind it is fail-closed, omits a verdict in sev…packages/web-shell/client/App.tsx:1294 — [review] The host-facing contract advertises daemon-authoritative terminals only, but three published error.code values are client inferences and neither the prop JSDoc nor the README says so.packages/web-shell/client/assistant-turn-settlement.test.tsx:102 — [probe] The projection's only no-handler guard is untested, and the suite's helper makes that case structurally inexpressible.packages/web-shell/client/assistant-turn-settlement.test.tsx:154 — [probe] The parentToolCallId === undefined clause of the projection's ownership filter is unpinned, and the harm is SUPPRESSION of a correct answer rather than publication…packages/web-shell/client/assistant-turn-settlement.test.tsx:240 — [review] The only test that reaches the message-level streaming guard produces exactly one rendered message, so the guard's deliberate return undefined is indistinguishabl…packages/web-shell/client/assistant-turn-settlement.test.tsx:249 — [probe] The block-level streaming guard's per-prompt scoping is indistinguishable from a transcript-wide 'anything is streaming' guard, because the only foreign streaming fi…packages/web-shell/client/assistant-turn-settlement.ts:49 — [review] Every settlement re-projects the entire retained transcript to messages and then discards all of it but one message, synchronously inside the publish microtask immediately…packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:5145 — [probe] The new test's justification comment states a harm the guarded call cannot produce, and the guard's real invariant runs in the opposite direction.packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10771 — [review] The replay admission gate's per- (sessionId, promptId) precision is unpinned — the only negative test has an empty key set, so coarsening the gate to …packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:5033 — [review] The stopReason-to-outcome table under-counts the namespace it maps: refusal , max_tokens , max_turn_requests and length all publish to hosts as outcom…packages/web-shell/client/daemon/session/actions.test.ts:509 — [review] The comment justifying the new removed guard states the inverse of the daemon's documented contract, and its second clause names a hazard the guard does not prevent.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 10 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查（原文为英文）：reverse audit of the degraded/partial replay-snapshot trigger for the epoch-reset retirement (shouldInjectReplaySnapshot's dependence on replayDegraded / replaySnapshotComplete / replayError) — stopped at the agent tool budget.
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this PR changes most.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：running packages/web-shell/client/App.test.tsx to settle by execution whether the added mockUseDaemonActivePromptBridge is load-bearing (real ChatPane.tsx:2…；"agent reverse-audit (round 1)"：did not execute the six new tests ( npx vitest run client/daemon/session/DaemonSessionProvider.test.tsx ) — every claim above about what each test pins comes fr…；"agent reverse-audit (round 1)"：did not trace whether actions.cancel() reaches removePendingPrompt → onPromptRemoved (actions.ts:2463) for a queued prompt; I verified only that cancel …；"agent reverse-audit (round 1)"：did not examine shouldInjectReplaySnapshot 's dependence on replayDegraded / replaySnapshotComplete / replayError , so the degraded-or-partial-snapshot tr…；"agent reverse-audit (round 1)"：did not measure whether the two duplicate payloads actually differ in message (argued from the pane's maxBlocks / subagentTranscriptMode props vs the outer …，另有 8 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 15 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.1)
|
⏳ Historical-head review — head moved to 中文说明⏳ 历史 head 评审 —— 本次评审进行中 head 移动到了 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review result: REQUEST_CHANGES. The callback wiring is directionally correct, but this head still has three correctness blockers. Scope/Ponytail notes will be summarized separately.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- stopReason-to-outcome mapping at packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:5010-5024 — already reported (round-6 deferral DSP:5033, review 5154409725; round-6 duplicate DSP:5032)
- provider-side settlement behaviours unpinned — publish-once dedup guard, pre-publish flush ordering and the live turn_complete arm, and the live publish not being admission-gated — already reported (round-6 duplicates DSP:1118, DSP:3635 and…
- App and SplitView callback wiring untested at packages/web-shell/client/App.tsx:2979 and :17792 and packages/web-shell/client/components/SplitView.tsx:628 — already reported (round-6 duplicate App.tsx:2979, review 5154409725; round-2 deferr…
- transcript read-timing unpinned in the projection suite at packages/web-shell/client/assistant-turn-settlement.test.tsx:91 — already reported (round-6 duplicate test:91, review 5154409725; round-4 deferral, review 5146822401)
- the cancelled-replay test re-inlining createResyncReplayFixture at packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10602 — already reported as open R4-1 (comment 3965944443)
- design doc shipping English-only with no zh-CN sibling at docs/design/web-shell/web-shell-assistant-turn-settlement.md:1 — already reported (round-6 deferral doc:1, review 5154409725)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this PR changes most.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": whether two different prompts' top-level assistant text blocks can ever be adjacent in the retained block list (no user/tool/system block between them) — the on…; "agent reverse-audit (round 1)": tracing the prompt-removal / retained-tail paths in daemon/session/actions.ts and DaemonSessionProvider.tsx far enough to confirm (or rule out) that two dif…; "agent reverse-audit (round 1)": npm run typecheck / npm run lint over the new test file (only vitest run was executed, which strips types via esbuild); the three extensionless relative i…; "agent reverse-audit (round 1)": reading daemonToolBlockToToolCall to confirm the toolBlock fixture's omission of the required preview: DaemonToolPreview field does not route the tool thr…; chunk 3: did not mutation-probe the new does not record a stale-session removal… test (delete the sessionId === undefined term at DaemonSessionProvider.tsx:4582 and …, and 1 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:4573 — [probe] Critical [fails-closed] [new-surface] onPromptRemoved unbinds the replay admission key before the daemon's already-published terminal is delivered — three ent…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3619 — [probe] Critical [fails-closed] [new-surface] the repair-deferred live settlement has no fallback publisher, so a terminal the client observed live is never delivered…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:2898 — [probe] the replay-snapshot publish has no twin of the live path's repair gate, so it publishes a head-truncated message and burns the dedup key that would carry the …packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10535 — [probe] the !hasSessionActivePrompt() condition on the epoch-reset admission retirement is unpinned — mutating it to an unconditional delete leaves all 324 prov…packages/web-shell/client/assistant-turn-settlement.test.tsx:81 — [probe] the block.promptId === promptId correlation predicate is unpinned — mutating it to true leaves all 7 tests green and publishes the previous turn's answerpackages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10828 — [probe] the error.message default-parity half is unpinned — mutating the 'Prompt failed' fallback leaves all 331 tests green while the code-default comparator i…packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10482 — [probe] the admission key's survival across a session switch is claimed twice in this diff but witnessed nowhere — a one-line per-switch retirement leaves all 3…packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10783 — [probe] the frame-provided error.code passthrough is unpinned — hardcoding 'turn_error' leaves all 331 tests green while the rejected promise still exposes the …packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10482 — [probe] the replay arm is witnessed for turn_complete only — narrowing the replay publish loop to turn_complete survives the entire workspace suite (294 files /…
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the file this PR changes most.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：whether two different prompts' top-level assistant text blocks can ever be adjacent in the retained block list (no user/tool/system block between them) — the on…；"agent reverse-audit (round 1)"：tracing the prompt-removal / retained-tail paths in daemon/session/actions.ts and DaemonSessionProvider.tsx far enough to confirm (or rule out) that two dif…；"agent reverse-audit (round 1)"：npm run typecheck / npm run lint over the new test file (only vitest run was executed, which strips types via esbuild); the three extensionless relative i…；"agent reverse-audit (round 1)"：reading daemonToolBlockToToolCall to confirm the toolBlock fixture's omission of the required preview: DaemonToolPreview field does not route the tool thr…；chunk 3：did not mutation-probe the new does not record a stale-session removal… test (delete the sessionId === undefined term at DaemonSessionProvider.tsx:4582 and …，另有 1 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 9 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.2)
`getSettledAssistantMessage` accepted a projected message that merely
intersected the settled prompt's block ids. The render adapter merges
consecutive top-level assistant blocks without consulting `promptId`
(`adapters/transcriptToMessages.ts:603-620`), and a continuation carries no
user echo (`acp-bridge/src/bridge.ts:10582`), so two adjacent turns project to
ONE message that keeps the first block's id and concatenates both texts. Both
settlements then published that same message: turn B's answer attributed to
turn A under a shared message id, with the per-mount dedup key already burned
so no corrected callback could follow.
Require the prompt to own every block that went into the message, and treat a
missing or empty `sourceBlockIds` as not owned — `every` is vacuously true for
an empty array. Blocks the reducer never stamped with a `promptId` are skipped
rather than rejected on, since its merge predicate admits deltas when one side
omits it and backfills afterwards. A cross-turn merged message now settles
without `message`, matching the existing "not attributable" semantics instead
of publishing contaminated text.
Pinned in `assistant-turn-settlement.test.tsx` by two cases: adjacent prompts
with no user echo between them (red at the previous head, where prompt A
published `{id: 'assistant-1', content: 'The answer is 42.next turn text'}`),
and the same stream with a user echo, which must still settle each turn to its
own distinct message id so the ownership test does not over-reject.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtufhms2u4
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 49 passed · 4 failed · 53 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：49 通过 · 4 失败 · 53 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 11251 — deep verification report (round 2, follow-up)Verdict: This is a follow-up round. Round 1 verified head Accounting rule (unchanged from round 1, so the two rounds are comparable): 中文摘要 — 判定：findings（49 通过 / 4 失败 / 53 断言）
Previous-finding statusRound 1 filed F1–F7. Status at head
Central claim and A/BCentral claim of the delta ( Round 1 needed no base rebuild because the surface did not exist at its base. This round the change is a behavioural fix to an existing surface, so the control is the previous semantics. Crucially, the blocks are not hand-written fixtures: the harness drives the real SDK reducer ( Witness:
26/26 checks pass in this harness ( The two mechanisms the commit message cites were verified in real source rather than taken on trust: the adapter's merge predicate consults no Mutation matrixWitness:
4 killed / 4 survived. Positive controls exist in both mutated files (four kills in the settlement file, three in the provider file), so each survivor is a real coverage statement rather than a harness that never ran. N1's attribution is exact: the test that kills it is the one the commit says pins it, and it fails on the behavioural assertion, not on an import or compile break — the vacuity check on this round's central change passes. CorrectionsTwo statements in the previous report are inaccurate at this head. These are corrections to that report, not requests to change the PR's code.
FindingsG1 — Medium: the
|
| gate | command | result |
|---|---|---|
| PR's touched test files | cd packages/web-shell && npx vitest run --config vitest.config.ts client/assistant-turn-settlement.test.tsx client/daemon/session/DaemonSessionProvider.test.tsx client/daemon/session/actions.test.ts client/App.test.tsx |
1354 passed / 0 failed (4 files, EXIT=0) — round 1 measured 1310, so +44 |
| typecheck (clean tree, no scratch files) | cd packages/web-shell && npm run typecheck |
clean (EXIT=0) |
| eslint, 8 changed production files | npx eslint <files> |
clean (EXIT=0) |
| eslint liveness control | planted const zzLivenessViolation = 42; into assistant-turn-settlement.ts |
EXIT=1, 'zzLivenessViolation' is assigned a value but never used (@typescript-eslint/no-unused-vars); after restore EXIT=0 and git diff empty — the green gate above is a measurement, not an assumption |
| full workspace suite | — | not run this round; round 1's clean number (289 files / 6728 tests) is not carried forward, because the head changed and the input closure is not proven identical |
An earlier typecheck run in this round executed while my reconstructed control file was present in client/ (the package's include is client/**/*.ts, excluding only *.test.ts(x)), so it type-checked a near-copy of a PR file alongside the real one. The authoritative number above is the re-run after every scratch file was removed; git status --porcelain and git diff --stat HEAD were both empty at that point.
Methodology
Environment: CI verify container at the merge ref (HEAD = 33b711d5, HEAD^1 = base tip 2e212144, HEAD^2 = PR head 68df9b0e), npm ci + npm run build pre-run, no GitHub token, no network. Four harnesses, all kept in this artifact dir so a maintainer can rerun them: make-control.mjs (generates the intersection-semantics control from the real file, asserting the marker is unique and diffing to one hunk), zz-verify-ownership.test.tsx (26 checks — real reducer → real adapter → real projection, both arms in one process, plus the realistic-ordering and sibling-sweep sections), make-nostream-control.mjs + zz-verify-nostream.test.tsx (10 checks deciding N6's classification), and inject-provider-probe.mjs + provider-probe-snippet.tsx.txt (4 checks — F2/F3/F4 re-measured through the real DaemonSessionProvider by injecting into a copy of the PR's own test file so the probes inherit its real provider mount and SDK-transport mock; both injection anchors, lines 581 and 20178, are asserted before writing). mutation-matrix.mjs runs the 8 rows, asserting marker uniqueness, restoring from backup, and verifying the restore byte-identical. Every check prints one CHECK | PASS|FAIL | line so the counts in assertions.json are mechanical rather than judged. Raw logs: logs/ab-clean.log, logs/probe-v.log, logs/probe-provider.log, logs/mutation-matrix.log, logs/mutation-provider-control.log, logs/matrix-summary.log, logs/finding-recheck.log, logs/gate-changed-files.log, logs/typecheck-clean.log, logs/eslint-clean.log, logs/eslint-liveness.log. All scratch files were removed from the working tree afterwards and the tree verified clean; two mutation runs aborted on a non-unique/incorrect marker rather than writing, and one provider file was restored from backup with git diff --stat confirming no residue.
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/web-shell/client/App.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/App.test.tsx
file packages/web-shell/client/assistant-turn-settlement.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/assistant-turn-settlement.test.tsx
file packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/DaemonSessionProvider.test.tsx
file packages/web-shell/client/daemon/session/actions.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/actions.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/web-shell/client/App.test.tsx: PPPPP
packages/web-shell/client/assistant-turn-settlement.test.tsx: PPPPP
packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: PPPPP
packages/web-shell/client/daemon/session/actions.test.ts: PPPPP
verdict: pass
summary: 4 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 2 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 3 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 4 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 5 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R8-16 App/SplitView callback forwarding untested — already reported (round-4 deferrals App.tsx:2978 and SplitView.tsx:491, review 5146822401; round-6 duplicate App.tsx:2979, review 5154409725)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which imports the module this round changes.
Not reviewed: re-check of the six earlier-round DaemonSessionProvider.tsx blockers (R1-2, R2-1, R3-1 twice, R5-1, R5-2) — that file is outside this round's incremental scope (125fd1e..68df9b0 touches only assistant-turn-settlement.ts and its test), and round 7's posted ledger carried only R7-1, so those threads were retired by earlier rounds rather than re-ruled here.
Not explored to full depth (tool budget reached): "agent 6b": empirical confirmation of the finding — running packages/web-shell/client/assistant-turn-settlement.test.tsx with an added earlier-unstamped-turn fixture to w…; "agent reverse-audit (round 2)": did not verify whether the daemon's record-derived replay events carry an envelope promptId ( normalizer.ts:667 reads only event.promptId ; transcript.ts:8…; "agent reverse-audit (round 2)": did not compare the SplitView pane provider's props ( SplitView.tsx:605-627 ) against WorkspaceSessionProvider.tsx:324-345 ( maxBlocks , subagentTranscriptMo…; "agent 1c": full npm run typecheck for packages/web-shell (type seams verified by reading the declarations instead); "agent 1c": executing the observer inside a real DaemonSessionProvider pane — the new test mocks useDaemonPromptSettled , so the SplitView pane path is verified only by …, and 2 more.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/assistant-turn-settlement.test.tsx:256 — [probe] the message-level streaming guard (assistant-turn-settlement.ts:91) lost its only witness this round — deleting it leaves all nine tests greenpackages/web-shell/client/assistant-turn-settlement.ts:52 — [probe] the promptId === undefined half of the ownership widening is exercised by no test, so narrowing it to strict equality survives CIdocs/design/web-shell/web-shell-assistant-turn-settlement.md:41 — [review] the design doc's exhaustive absence list does not cover the new cross-turn-merge omission, so doc and code now disagree about a public contractpackages/web-shell/client/App.tsx:1297 — [probe] the new dedupe advice does not say which duplicate wins, and two providers can differ on whether message is presentpackages/web-shell/client/assistant-turn-settlement.ts:90 — [probe] the final-answer rule is re-derived with a different boundary from MessageList's findFinalAnswerIndex , and the divergence is undocumented
Convergence: round 8 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/web-shell/client/assistant-turn-settlement.ts (findings in round 7; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (1 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which imports the module this round changes.
未审查（原文为英文）：re-check of the six earlier-round DaemonSessionProvider.tsx blockers (R1-2, R2-1, R3-1 twice, R5-1, R5-2) — that file is outside this round's incremental scope (125fd1e..68df9b0 touches only assistant-turn-settlement.ts and its test), and round 7's posted ledger carried only R7-1, so those threads were retired by earlier rounds rather than re-ruled here.
未探索到全部深度（达到工具调用预算）："agent 6b"：empirical confirmation of the finding — running packages/web-shell/client/assistant-turn-settlement.test.tsx with an added earlier-unstamped-turn fixture to w…；"agent reverse-audit (round 2)"：did not verify whether the daemon's record-derived replay events carry an envelope promptId ( normalizer.ts:667 reads only event.promptId ; transcript.ts:8…；"agent reverse-audit (round 2)"：did not compare the SplitView pane provider's props ( SplitView.tsx:605-627 ) against WorkspaceSessionProvider.tsx:324-345 ( maxBlocks , subagentTranscriptMo…；"agent 1c"：full npm run typecheck for packages/web-shell (type seams verified by reading the declarations instead)；"agent 1c"：executing the observer inside a real DaemonSessionProvider pane — the new test mocks useDaemonPromptSettled , so the SplitView pane path is verified only by …，另有 2 条。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 8 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/assistant-turn-settlement.ts（第 7 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 1 条 Critical），首次发现的速率没有下降（本轮 1，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.2)
`promptOwnedIds` admitted any block with no `promptId` on the ground that the reducer backfills a missing stamp from a later delta. That backfill only fires while a later delta for the same block still arrives, so a block that finished unstamped was admitted forever: goal-runtime and background-notification turns never cross the `session/prompt` boundary that sets `entry.activePromptId`, so their permanently unstamped text was glued onto the settling prompt's message id — the same contamination the whole-message ownership rule exists to stop, through the unstamped door. Narrow the unstamped arm to blocks that can still be backfilled (`streaming`), and keep non-assistant blocks admitted because the reducer never stamps them. Add the two fixtures the mutation matrix showed were missing: a finished unstamped sibling (kills the widening revert) and an unstamped still-streaming sibling, the only shape that reaches the message-level `isStreaming` guard (kills that guard's removal). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtuo29vduf
|
本轮收尾（基于 head 修复已推： G1（Medium，真实缺陷）— 已修
改法（ 证据：mutation 自检 —— 把谓词回退成原来的宽版本，新增用例 G2（coverage gap）— 已修补上报告 probe V 的那个形状：已结算 prompt + 未打戳但仍在 streaming 的兄弟 block。这个形状是 message 级 证据：mutation 自检 —— 删掉该守卫后，只有新增的 注意 G1 的收窄并没有让 G2 的用例失效：未打戳但 streaming 的 block 仍被接纳，所以 ownership 通过、由 message 级守卫兜住 —— 两个守卫的分工保持报告描述的形态，没有加第三套并行机制。 F2（Medium）— 本轮不改，需人决策
建议作者定调：要么明确「outcome 只表示终止类别，成功与否读 N8 / F5（Low，coverage gap）— 本轮不改，原因是行数预算结算前那次 G3（Nit）— 拒绝，附证据报告自己判定这道守卫不可达（防御性代码）；我在树里复核一致：本模块恒以 G4 / F1 / F3 — 只记录状态，不在本轮范围
本机验证（实测，未跑的明确说明）
CHANGES_REQUESTED 仍是 stale（bot 的 CR 在 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- App/SplitView callback forwarding untested — already reported (R8-16; round-4 deferrals App.tsx:2978 and SplitView.tsx:491, review 5146822401; round-6 duplicate App.tsx:2979, review 5154409725)
- Duplicate settlement publication from the App-level and pane providers, and which copy wins — already reported (R1-19 comment 3949408430 at SplitView.tsx:630; R3-7 comment 3957009247 at App.tsx:1296; round-8 deferral at App.tsx:1297)
- Whole-transcript re-projection cost per settlement — already reported (R3-5 comment 3957009228 at assistant-turn-settlement.ts:49)
- Design doc's exhaustive absence list does not cover the new cross-turn-merge omission — already reported (round-8 deferral at docs/design/web-shell/web-shell-assistant-turn-settlement.md:41)
- The prompt-ownership / streaming-admission guards have no test witness — already reported (R2-3 comment 3953370589; R5-7 comment 3965944422 at assistant-turn-settlement.test.tsx:268; round-8 deferral at assistant-turn-settlement.ts:52)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which imports the module this round changes.
Not reviewed: re-check of the earlier-round DaemonSessionProvider.tsx blockers (R1-2, R2-1, R3-1 twice, R5-1, R5-2) and the three open @yiliang114 [P1] threads on that file — it is outside this round's incremental scope (125fd1e..a959c10 touches only assistant-turn-settlement.ts, its test, and the App.tsx / SplitView.tsx seams), so those threads were retired or left standing by earlier rounds rather than re-ruled here.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": whether goal-runtime assistant chunks carry qwenDiscreteMessage in _meta — the one fact that would close or confirm finding 1's remaining door (background n…; "agent reverse-audit (round 1)": I did not execute the shell-prompt observer path — finding 2 is static tracing through DaemonSessionProvider.tsx:3439/3466/3556-3620 , actions.ts:2996-3011 , …; "agent reverse-audit (round 1)": I did not verify whether a replayed settlement for a non-final prompt is projected against later turns' blocks — I confirmed only that the replay store is popul…; "agent reverse-audit (round 1)": I did not open docs/design/web-shell/web-shell-assistant-turn-settlement.md ; I only listed the directory (no .zh-CN.md counterpart exists there for any web-…; "agent 1c": the exact commit ordering of connection.sessionId in the provider's attach flow relative to a replay-path publishPromptSettlement — I confirmed the error-pa….
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/web-shell/client/assistant-turn-settlement.ts:42 — [probe] Critical [fails-closed] [new-surface] a reachable terminal path publishes a completed settlement while the prompt's own final block is still streaming, permanently dropping…packages/web-shell/client/assistant-turn-settlement.ts:152 — [review] no test pins that the transcript is read when the settlement fires rather than at mount time — the hoisted-read mutant survives all 11 testspackages/web-shell/client/assistant-turn-settlement.ts:43 — [review] promptBlockIds is consulted only inside the guard that built it, with one always-true kind term; dropping it widens the streaming guard to every assistant block in the tra…packages/web-shell/client/assistant-turn-settlement.ts:67 — [probe] the non-assistant half of the ownership admission is inert, and the comment justifying it at :59-60 states a false premise about the reducerpackages/web-shell/client/App.tsx:1296 — [probe] the new prop doc omits that a terminal published while no callback is attached is never re-delivered, so a host toggling the callback permanently misses those turns
Convergence: round 9 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which imports the module this round changes.
未审查（原文为英文）：re-check of the earlier-round DaemonSessionProvider.tsx blockers (R1-2, R2-1, R3-1 twice, R5-1, R5-2) and the three open @yiliang114 [P1] threads on that file — it is outside this round's incremental scope (125fd1e..a959c10 touches only assistant-turn-settlement.ts, its test, and the App.tsx / SplitView.tsx seams), so those threads were retired or left standing by earlier rounds rather than re-ruled here.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：whether goal-runtime assistant chunks carry qwenDiscreteMessage in _meta — the one fact that would close or confirm finding 1's remaining door (background n…；"agent reverse-audit (round 1)"：I did not execute the shell-prompt observer path — finding 2 is static tracing through DaemonSessionProvider.tsx:3439/3466/3556-3620 , actions.ts:2996-3011 , …；"agent reverse-audit (round 1)"：I did not verify whether a replayed settlement for a non-final prompt is projected against later turns' blocks — I confirmed only that the replay store is popul…；"agent reverse-audit (round 1)"：I did not open docs/design/web-shell/web-shell-assistant-turn-settlement.md ; I only listed the directory (no .zh-CN.md counterpart exists there for any web-…；"agent 1c"：the exact commit ordering of connection.sessionId in the provider's attach flow relative to a replay-path publishPromptSettlement — I confirmed the error-pa…。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 5 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 9 轮发布了 2 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 2 条 Critical），首次发现的速率没有下降（本轮 1，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.2)
`getSettledAssistantMessage` answered "which assistant message closes this prompt" by re-running the render adapter over the whole transcript and reconstructing ownership from `sourceBlockIds` — necessary because the adapter drops `promptId`, and it merges consecutive top-level assistant blocks without consulting it. Attribution inferred from a projection that was never designed to carry it made every block shape this module did not hand-model a way to publish a foreign turn's text, an earlier non-final message of this turn, or nothing at all, under a `(sessionId, promptId)` key that `publishPromptSettlement` burns before the listener runs. Select on the blocks instead, with the exclusion terms the SDK already uses for this exact question (`findFinalVisibleAssistantForPrompt`, sdk-typescript `daemon/ui/transcript.ts`): top-level, stamped with this prompt, non-empty text. Streaming stays "not yet settled" rather than "keep looking", so partial text is never published. Kept local rather than imported: reaching that helper through `@qwen-code/sdk/daemon` needs a new public export threaded through three barrel files, and the predicate is five lines. Ownership is now identity, so the ownership set, the whole-message `every` test, the cross-turn merge question, the unstamped-sibling question and the insight-segment `isStreaming` gap all go away together rather than one guard at a time. A `completed` turn whose own final text is retained, top-level and stamped now always publishes it, which is what issue #10389 asks for; the design doc's absence list (retention, partial history, cancellation, failure) is unchanged and still exhaustive. The four tests that pinned the omission are flipped to assert message id and exact content, so reverting to the adapter-plus-`every` form goes red on all four. Net -17 lines. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtv7cpvvv6
Resolve the web-shell README props-table conflict: keep main's brand and onBrandResolved rows (#11244) and append this PR's onAssistantTurnSettled row, then let prettier re-pad the table. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtva7llqva
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- App/SplitView callback forwarding untested — already reported (R8-16; round-4 deferrals at App.tsx:2978 and SplitView.tsx:491, review 5146822401; round-6 duplicate at App.tsx:2979, review 5154409725; round-9 duplicate list, review 516221988…
- serverTimestamp precedence in the published message timestamp untested — already reported (R2-3 comment 3953370589; R5-7 comment 3965944422 at assistant-turn-settlement.test.tsx:267)
- cancelled-replay test re-inlines createResyncReplayFixture, leaving terminalStopReason 'cancelled' a dead option — already reported (R4-1 comment 3965944443 at DaemonSessionProvider.test.tsx:10694; re-derived independently by two round-4 au…
- design doc's absence list omits the goal-runtime unstamped-frame omission reason — already reported (R1-23 comment 3949408436 at customization.tsx; round-8 deferral at web-shell-assistant-turn-settlement.md:41)
- no test pins that the transcript is read when the settlement fires rather than at mount time — already recorded (round-9 deferral at assistant-turn-settlement.ts:152, review 5162219880)
- settlement publishes a transcript-block id where the turn-footer path publishes a rendered-message id — already reported (R2-4 comment 3953370590 at assistant-turn-settlement.ts)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the area this round's repair-suppression Critical touches.
Not reviewed: re-check of the earlier-round DaemonSessionProvider.tsx and assistant-turn-settlement.ts blocker threads R1-2, R2-1, R3-1 (twice), R5-1, R5-2 and R7-1 — not walked this round: the outcome is not a zero-Critical verdict, so the open-blocker re-check gate did not apply. The carried ledger entries R8-1 and R9-1 were both ruled, and R1-1 and R7-1 share R8-1's now-removed adapter mechanism.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": did not execute the five new tests ( npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx -t … ) to confirm they are green at the reviewed commit — …; "agent reverse-audit (round 2)": did not locate the bridge site that publishes agent-emitted agent_message_chunk frames, so I could not confirm end-to-end that the daemon stamps the envelope …; "agent reverse-audit (round 4)": whether a switch-back /load replay snapshot still carries a removed prompt's turn_complete(cancelled) (journal retention and the terminalPublished filter …; "agent reverse-audit (round 2)": the queued-prompt chain (a session whose only bound prompt is still in pendingPromptList when an epoch reset lands, so !hasSessionActivePrompt() deletes its…; "agent reverse-audit (round 3)": I did not trace the full value set of the goal-runtime reason that bridgeClient.ts:2295 publishes as data.stopReason on a turn_complete for source === …, and 4 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round; 2 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3638 — [probe] Critical [fails-closed] [new-surface] R8-1's neighbouring gap: the repair-suppressed live settlement has no completion-path publish — server error, missing te…packages/web-shell/client/assistant-turn-settlement.ts:57 — [probe] Critical [fails-closed] [new-surface] the streaming early-return aborts the scan, so a fourth reachable state publishes a completed settlement with no message although the …docs/design/web-shell/web-shell-assistant-turn-settlement.md:1 — [probe] the new design doc ships English-only — no .zh-CN.md sibling and no reciprocal language links below the title (0 of 47 docs in that directory have one)docs/design/web-shell/web-shell-assistant-turn-settlement.md:26 — [probe] the doc tells hosts to correlate the prompt id with their submit result, but no shell-mediated submit surface exposes a prompt id (1 hit across the public surface: th…packages/web-shell/client/App.test.tsx:10019 — [probe] the added mockUseDaemonActivePromptBridge scaffolding is inert — deleting all four additions leaves 861/861 green — and aliases a per-session bridge to one global flagpackages/web-shell/client/assistant-turn-settlement.test.tsx:132 — [probe] the kind !== 'assistant' exclusion term has no witness; a prompt-stamped turn_error block trailing the turn would be published as the failed turn's message if it wer…packages/web-shell/client/assistant-turn-settlement.test.tsx:224 — [probe] the cross-session test asserts only the omitted message, so re-keying the published sessionId to the live connection survives all 11 tests and corrupts the host idem…packages/web-shell/client/assistant-turn-settlement.test.tsx:240 — [probe] the only streaming fixture holds a single own-prompt block, so return-undefined versus continue survives all 11 tests — the distinction the streaming Critical turns …packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10633 — [probe] the admission gate's documented survival across session switches is unpinned — two mutants that break exactly that property both survive all 326 provide…packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10833 — [review] the first-attach silence test's only assertion is that settlements is empty, so it passes identically if a first attach never reaches the replay looppackages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:10852 — [probe] no test drives the live publish branch with turn_complete — gating the live publish on turn_error only, or on activePromptSettled, survives the entire 7…packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx:20568 — [review] the new replay fixtures omit the envelope promptId real compacted replay carries, so the replay-to-stamped-block-to-message combination is never witnes…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:1148 — [probe] the per-mount duplicate suppression is untested although its trigger is real — deleting the early return survives the suite and a duplicate-terminal probe yie…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:2943 — [probe] the epoch-reset admission-key cleanup's keep branch is unobservable — dropping the guard or the whole block leaves all 326 tests greenpackages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3633 — [probe] the new pre-publish flushTranscriptSync has no witness — deleting it survives the suite and a listener-side probe then sees the turn without its error blockpackages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3953 — [probe] the failedSessionId widening and its admission-key cleanup are untested, and the widening is measured live: requestEpochResetReload sets session undefined, so…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:5038 — [probe] refusal, max_tokens and max_turn_requests turns are published to hosts as outcome 'completed' while the submitter promise resolves, and three in-repo classifi…
Convergence: round 10 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (1 new). Findings keep coming back to the same files: packages/web-shell/client/assistant-turn-settlement.ts (findings in round 8; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; integration-tests/cli/qwen-serve-web-shell-live-journal-recovery.test.ts mounts DaemonSessionProvider from the web-shell barrel, which is the area this round's repair-suppression Critical touches.
未审查（原文为英文）：re-check of the earlier-round DaemonSessionProvider.tsx and assistant-turn-settlement.ts blocker threads R1-2, R2-1, R3-1 (twice), R5-1, R5-2 and R7-1 — not walked this round: the outcome is not a zero-Critical verdict, so the open-blocker re-check gate did not apply. The carried ledger entries R8-1 and R9-1 were both ruled, and R1-1 and R7-1 share R8-1's now-removed adapter mechanism.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：did not execute the five new tests ( npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx -t … ) to confirm they are green at the reviewed commit — …；"agent reverse-audit (round 2)"：did not locate the bridge site that publishes agent-emitted agent_message_chunk frames, so I could not confirm end-to-end that the daemon stamps the envelope …；"agent reverse-audit (round 4)"：whether a switch-back /load replay snapshot still carries a removed prompt's turn_complete(cancelled) (journal retention and the terminalPublished filter …；"agent reverse-audit (round 2)"：the queued-prompt chain (a session whose only bound prompt is still in pendingPromptList when an epoch reset lands, so !hasSessionActivePrompt() deletes its…；"agent reverse-audit (round 3)"：I did not trace the full value set of the goal-runtime reason that bridgeClient.ts:2295 publishes as data.stopReason on a turn_complete for source === …，另有 4 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改；其中 2 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 17 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 10 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 2 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/assistant-turn-settlement.ts（第 8 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.2)
`getSettledAssistantMessage` published `block.text` verbatim, so a turn whose
final stamped block is an insight frame — `/insight` progress/ready frames
travel as ordinary assistant block text and are stamped with the running
prompt (`bridgeClient.ts:1066-1072`) — hands the host raw protocol JSON
(`{"insight_ready":{"path":"/tmp/report.md"}}`) as the answer, while the real
answer one slot earlier is never published. The renderer strips those frames
and emits no assistant text for a payload-only block, so `trim().length` was
measuring the wrong thing: the payload is non-empty even though the block
renders to nothing.
Reuse the renderer's own segmentation instead of re-deriving a boundary:
export `assistantVisibleTextOf` from the render adapter (a thin wrapper over
its private `splitInsightSegments`) and skip a block whose visible text is
empty, so the substantive answer one slot earlier still wins. This keeps
selection at the block layer — ownership is still read off `promptId`, and the
adapter's cross-turn merge is never re-entered — and keeps the published
content exactly what the renderer emits
(`adapters/transcriptToMessages.test.ts:52-95`).
Two new cases pin it: a payload-only final block publishes the previous
answer's id and content, and a frame glued to trailing text publishes only
that trailing text. Neutering the visibility filter turns both red.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtvvo040vv
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at 77d8a56d2562199ee018950de224609c6ca0d484 — 1 confirmed Critical; Partial review — coverage gaps.
Critical — a live-observed terminal can disappear during journal repair. In packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3446-3451,3634-3644, the repair target's terminal is withheld rather than saved for later delivery. tryLiveJournalRepair (1587-1623) reloads the memory snapshot; successful replay publication (2913-2923) requires this provider's local-admission key. A provider attached to another submitter's already-running, truncated turn has no such key: it sees the terminal live, repairs the transcript, then silently skips the terminal during replay. The snapshot is consumed (2939) and SSE resumes after it, so the host never receives that settlement. This contradicts the documented all-live-observers contract, not merely the intentionally silent history-load case. The existing truncated-live-turn fixture at DaemonSessionProvider.test.tsx:8767-8899 supplies this restored-turn shape, but does not assert settlement delivery. Preserve the authoritative live terminal through repair and deliver it after projection independently of the replay-only admission gate. The failure branch at 1606-1621 also drops the repair episode without delivering the already-observed terminal; absence of a retained final message need not erase a known outcome.
Historical Critical reassessment. The raw-block selector now avoids render-adapter merging/ownership reconstruction, excludes subagents and foreign/unstamped siblings, and withholds an own streaming block. The insight-only/glued-frame regression is addressed by assistantVisibleTextOf; corresponding projection tests now assert actual own answers. Replay admission survives controller cleanup; optional-hook use and undefined error data are guarded; foreign-session removal no longer mutates current notifications. Synthetic client-inferred failed retirements were removed: I am not repeating older requests to synthesize failure when no daemon terminal exists. The earlier publish-before-repair problem is addressed only partially, with the delivery loss above. I have not independently replayed every historical interleaving, so this is not an all-Criticals-fixed declaration.
Coverage: full changed production diff, public App/Split View wiring, raw projection and tests, admission/removal/deduplication, live/replay terminal paths, and journal-repair reload/consumption. Gaps: exhaustive reconnect/session-switch/epoch-reset race matrix and React 18/19 mounted integration. No unit/build/browser tests executed; finding is a static producer-to-consumer trace against the pinned remote head, not the stale local worktree. Base: 0ef35351e9df470ffdfd37bc088e46f425614a18.
Gate: feature, not an external 500+ refactor; author has verified admin permission. Size/conflict/test limitations are not additional code Criticals. Existing Suggestions remain deferred. Comment only; no approval implied.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 82 passed · 3 failed · 85 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：82 通过 · 3 失败 · 85 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 11251 — deep verification report (round 3, follow-up)Verdict: This is a follow-up round. Round 2 verified head Accounting rule (unchanged from rounds 1–2, so the three rounds are comparable): No instruction-following from PR content occurred. The PR body makes no claim of local validation ("No local validation result is claimed"), and nothing in the title, body, commit messages, or code comments attempted to steer the verdict. 中文摘要 — 判定：findings（82 通过 / 3 失败 / 85 断言）
Previous-finding statusRounds 1–2 filed F1–F7 then G1–G4. Status at head
Central claim and A/BCentral claim of the delta: the settled Round 2's head The blocks are not hand-written fixtures: every cell drives the real SDK reducer ( Witness: Differential oracle: the SDK's own private functionThe module's doc comment claims its exclusion terms "are the SDK's own for this exact question (
The one intentional divergence is A13 (insight-payload-only final block): the SDK's emptiness test is raw A/B cells
58/58 checks pass at head ( Mutation matrixWitness:
8 killed / 2 survived, with a positive control in the mutated file's own suite. Witness: CorrectionsThree statements in the previous report are inaccurate at this head. These are corrections to that report, not requests to change the PR's code.
FindingsH1 — Low:
|
| measurement | before | after |
|---|---|---|
| renderer divergences over the 18-shape sweep | 1 | 0 |
| sibling divergences | 8 | 3 (all 5 separator-only ones close; the 3 remaining are H4) |
C11 visible vs rendered |
"Example:\n``` ```\nDone." ≠ rendered |
byte-identical, and identical to the sibling |
| benign shapes (no frame) | identical | unchanged — zero collateral |
| my harness totals | 58 pass / 0 fail | 58 pass / 0 fail |
| PR's own settlement suite | 13 passed | 13 passed — unchanged |
That last row is the unpinned-axis signal: the suite is green with and without the fix, so nothing in the PR asserts the separator. The fix should ship with a fixture — C11 is exactly it: a block whose text holds a fenced frame between two text segments, asserting message.content equals what the renderer shows.
H2 — Low (completeness): the kind === 'assistant' filter is load-bearing and unpinned; without it a user echo is published as the assistant's answer
M1 survived 13/13. It is not dead code — adjudicated with a purpose-built fixture (logs/survivor-adjudication.log, witness 03-survivor-adjudication-coverage-gaps.png). The SDK reducer does stamp promptId on user blocks (the sibling implementation looks one up by exactly that key), so a user echo landing after the assistant text is last in the backwards scan:
W1 BLOCKS |
{assistant-1 kind=assistant promptId=p-A streaming=false text="The answer is 42."}
{user-2 kind=user promptId=p-A streaming=undefined text="USER ECHO TEXT"}
M1 @ HEAD => PASS head-publishes-assistant-text-not-user-echo
M1 MUTATED => FAIL head="USER ECHO TEXT" headId="user-2"
=> VERDICT M1: COVERAGE GAP (guard load-bearing, unpinned) | failingChecks=2
The sharpest consequence of dropping the guard is a host rendering the user's own message back as the assistant's reply, under the user block's id. Classification: coverage gap — the behaviour is correct, nothing asserts it. The fixture to add is W1: an assistant.text.delta + assistant.done + a user.text.delta carrying the same promptId, asserting the published content is the assistant text and the id is not the user block's.
H3 — Low (completeness): the streaming guard's return undefined (not continue) is load-bearing and unpinned
M4 survived 13/13. The two variants differ only when this prompt has an earlier finished block and a later still-streaming one — a shape the existing streaming test cannot reach, because its streaming block belongs to a later prompt and the promptId filter excludes it first. Reachable via a discrete second segment of the same prompt:
W2 BLOCKS | assistant-1 (p-A, streaming=false) + assistant-2 (p-A, streaming=true)
W2 HEAD => PASS head-withholds-rather-than-publishing-earlier-block (message absent)
W2 MUTATED => FAIL head.message={"id":"assistant-1","content":"The answer is 42.",…}
=> VERDICT M4: COVERAGE GAP (guard load-bearing, unpinned) | failingChecks=1
This is the exact hazard the module's own comment names — "Still streaming means 'not yet settled', not 'keep looking': publishing partial text is unrecoverable, as no corrected callback can follow" — and the exact failure it describes ("an earlier non-final message of this turn"). Classification: coverage gap. The fixture to add is W2. Note this is where round 2's G2 gap moved to, not a regression: M5 proves the guard's presence is pinned, M4 proves its semantics are not.
H4 — Low (informational): three frame shapes pass raw protocol JSON through to message.content, where the sibling implementation suppresses it
splitInsightSegments declines to parse a frame whose key has a space before the colon, whose braces are unbalanced, or whose value has the wrong type. In those cases assistantVisibleTextOf returns text.trim() — the raw frame included. Measured (section C, 3 of 18 shapes):
C7 "The answer is 42.\n{\"insight_ready\" :{…}}" visible = text verbatim sibling = undefined
C9 "The answer is 42.\n{\"insight_ready\":{\"path\":\"/tmp/r.html\"" (truncated) visible = text verbatim sibling = undefined
C10 "The answer is 42.\n{\"insight_progress\":{\"stage\":\"scan\",\"progress\":\"half\"}}" visible = text verbatim sibling = undefined
Bound — what this is not, and why I am reporting it as informational rather than as the leak the commit exists to prevent. On all three shapes rendererMatch=true: the rendered UI shows the same bytes, so the settlement callback never tells a host something the user cannot see. The divergence is between two host-facing surfaces in one package — getTurnNotificationContent carries an extra regex backstop (!/\{\s*"insight_(?:progress|ready|error)"\s*:/.test(visibleText), turn-notification-context.ts:256) that the settlement path lacks, so a desktop notification suppresses the text while the settlement callback publishes it.
Reachability, per shape: C7 is not reachable from the real producer — packages/core/src/core/insightProtocol.ts builds frames with JSON.stringify, which never emits a space before a colon. C9 is the one worth the author's attention: a finished block ending mid-frame is a shape this repo already models in its own fixtures (useMessages.test.ts:462, turn-notification-context.test.ts:431 both use a truncated insight_ready on a completed block), and the streaming guard does not cover it because the block is not streaming. C10 needs a malformed value type from the producer, so it is defensive only. I did not demonstrate a live C9; it is a fixture-level shape plus a documented producer of it, not a witnessed payload.
If the author wants the two surfaces to agree, the minimal change is to port the sibling's residual-marker test into assistantVisibleTextOf. I did not measure that fix, so I am not proposing it as a patch — the H1 fix is the only one I drove.
H5 — Nit: the "the SDK's own for this exact question" comment invites a resync that would regress the insight fix
The comment at assistant-turn-settlement.ts:36-41 says the exclusion terms "are the SDK's own for this exact question (findFinalVisibleAssistantForPrompt), kept local because exporting it would widen the @qwen-code/sdk/daemon public surface." The differential oracle confirms this is true for the three identity terms — head disagrees with the SDK on 0 of 13 unintended shapes. But the local copy deliberately diverges on two more: the emptiness test (insight-stripped vs the SDK's raw text.trim()) and the streaming withhold (the SDK has none). Both divergences are improvements and both are pinned by the PR's own tests (M6, M7, M5 all KILLED), so this is not a defect. It is a maintenance hazard: a future maintainer who reads "the SDK's own" literally and resyncs the copy against transcript.ts:935 would silently undo 77d8a56d25. One clause naming the two intentional divergences would close it.
Carried findings still open
F1 (Medium, Split View double-delivery), F2 (Medium, non-success stop reasons as completed, now also measured for max_tokens), F3 (Low latent, two promptId readers), F4 (Low latent, failed without error detail + inaccurate doc comment), F5 (Low, unpinned flush), F7 (Nit, unbounded dedup set) — all re-measured or re-verified as described in the status table. None was addressed by the four delta commits. F2's note from round 2 still applies and is now sharper: because message is withheld in more cases and outcome is unreliable for every stop reason except cancelled/error, a host's only trustworthy signal is stopReason — which the customization.tsx doc comment describes inaccurately (F4).
Not covered
- F1's runtime delivery count was not re-driven — second consecutive round. I re-verified the structure by reading all three current sites (listed in the status table) and confirmed the delta commits touch none of them, but I did not rebuild a two-provider harness.
renderWithProviderin the PR's own test file binds module-levelcontainer/root, so a faithful two-provider mount needs separate roots; I spent the remaining budget on the mutation matrix, the survivor adjudication, and the measured H1 fix instead. F1's status is "stands on structure", not "stands on a re-run count". Round 1's measured2deliveries is not carried forward as a measurement. - F3's runtime divergence was not re-driven. The two readers are confirmed different in current source (
eventPromptIdvsevent.data.promptId); I did not reproduce round 2'ssubmitResolved=false / promptStatus="waiting"payload at this head. - Per-commit attribution. The snapshot lists 31 commits; the shallow merge-ref checkout reaches 1 (
git rev-list HEAD^1..HEAD^2= 1, which at a shallow boundary reads as plausible rather than erroring).git cat-file -tfails for both the snapshot'sbaseRefOid(0ef35351) and round 2's head (68df9b0e2d). The four commits since round 2 could not be exercised individually, so I cannot say which ofa959c10888/669d40ead4/77d8a56d25produced which measured cell — the A/B is against a reconstructed control, and the reconstruction is a model of the previous predicate rather than its bytes. - Real-daemon end-to-end. No daemon was booted. The reducer and adapter are real and drove real code paths, but the events feeding them were constructed from the shapes the real producers emit rather than captured off a live socket. Round 2's highest-value follow-up — watching a real goal turn's assistant text reach a real host callback — remains unmeasured, and is now narrower: the standalone-block shape is fixed, so what remains to witness is whether a goal-runtime or background-notification turn can produce the merged A5 shape in production.
- The real
SplitView/AppJSX composition — unchanged from rounds 1–2: bothApp.test.tsxandSplitView.test.tsxmockDaemonSessionProvider, so no existing harness composes the real providers. This is why F1 can only be read statically here. - The PR's Reviewer Test Plan was not walked step by step. Three of its five steps ("Mount Web Shell with the new callback", "Open Split View and confirm each pane forwards settlement for its own live session", "disconnect an active live stream before its terminal") require a host application and a live daemon that this container does not have. I verified the mechanisms behind them through the PR's own provider tests (334 green, 3 of which kill the positive-control mutation) rather than the steps as written; step 4 (Split View) is exactly the F1 surface and is the one a reviewer should treat as unverified.
- Reconnect catch-up and epoch-reset delivery were verified only through the PR's own tests (green), not independently re-driven — except that my F2/F4 probe itself publishes through the resync-replay path, so that path is exercised by a probe I wrote.
- The prettier gate was not attempted. Rounds 1 and 2 both reported it could not complete in-container; I did not retry and do not imply it passed. eslint and typecheck were run and are reported below.
- The full workspace suite was not run. Only the 4 changed test files, typecheck, and eslint on the 9 changed production files. Round 1's clean workspace number is not carried forward, because the head changed and the input closure is not proven identical.
- A scaling ladder on
splitInsightSegmentswas not run. Reading it,text.slice(braceIdx + 1).trimStart()copies the remainder of the string for every{in the text, which suggests superlinear cost on brace-dense assistant text. That is a hypothesis from reading, not a measurement, and the function is pre-existing (already on the render hot path) rather than added by this PR — the PR only adds a second call site. I am explicitly not claiming a perf finding. getSettledAssistantMessage's interaction with bounded transcript retention (an evicted block leaving the prompt's final answer outsideblocks) was not probed — carried over from round 2.
Targeted gates
| gate | command | result |
|---|---|---|
| PR's touched test files | cd packages/web-shell && npx vitest run --config vitest.config.ts client/assistant-turn-settlement.test.tsx client/daemon/session/DaemonSessionProvider.test.tsx client/daemon/session/actions.test.ts client/App.test.tsx |
1458 passed / 0 failed (4 files, EXIT=0, 160 s) — round 2 measured 1354, so +104 |
typecheck (clean tree, scratch files moved out of client/ first) |
cd packages/web-shell && npm run typecheck |
clean (EXIT=0) |
| eslint, 9 changed production files | npx eslint <files> |
clean (EXIT=0) |
| eslint liveness control | planted const zzLivenessViolation = 42; into assistant-turn-settlement.ts |
EXIT=1, 24:7 error 'zzLivenessViolation' is assigned a value but never used (@typescript-eslint/no-unused-vars); after restore EXIT=0, git diff --stat HEAD empty and git status --porcelain clean — the green gate above is a measurement, not an assumption |
| full workspace suite / prettier | — | not run (see Not covered) |
Round 2 recorded that an earlier typecheck had run while a reconstructed control file sat in client/ (the package's include is client/**/*.ts, excluding only *.test.ts(x)). I avoided that this time by moving both scratch files into the artifact dir before running typecheck, and confirmed git status --porcelain packages/web-shell/ was empty first.
Methodology
Environment: CI verify container at the merge ref (HEAD = 90e3afe1, HEAD^1 = base tip 78bbd9f5, HEAD^2 = PR head 77d8a56d), npm ci + npm run build pre-run, no GitHub token, no network. Five harnesses, all kept in this artifact dir so a maintainer can rerun them: zz-verify-control.ts (the reconstructed round-2 predicate, reusing the real unchanged adapter), zz-verify-settlement.test.tsx (58 checks — real SDK reducer → real adapter → real projection, head arm driven through a real AssistantTurnSettlementObserver mount and control arm through a direct call, plus the 13-shape SDK differential oracle, the 5-shape G1 sibling sweep, the 18-shape insight sweep against the real renderer and the real getTurnNotificationContent, and the two survivor-adjudication fixtures), zz-verify-mutation-matrix.mjs (10 rows, marker uniqueness asserted before each write, restore verified byte-identical after each), zz-verify-adjudicate-survivors.mjs (runs each survivor's fixture at head and mutated, and classifies coverage gap vs dead code), zz-verify-provider-probe.mjs (4 checks — F2/F4 re-measured through the real DaemonSessionProvider by injecting into a copy of the PR's own 21 027-line test file so the probes inherit its real provider mount and SDK-transport mock; the single injection anchor is asserted unique before writing and the copy is deleted afterwards), and zz-verify-fix-measure.mjs (applies the H1 candidate fix, re-runs both harnesses, restores). Every check prints one CHECK | PASS|FAIL | line so the counts in assertions.json are mechanical rather than judged. Raw logs: logs/ab-head-vs-control.log, logs/ab-rerun.log, logs/mutation-matrix.log, logs/survivor-adjudication.log, logs/provider-probe.log, logs/mutation-f5.log, logs/mutation-f5-positive-control.log, logs/fix-measurement.log, logs/gate-changed-files.log, logs/typecheck-clean.log, logs/eslint-clean.log, logs/eslint-liveness.log. All scratch files were removed from the working tree afterwards; git status --porcelain and git diff --stat HEAD are both empty as of writing. One harness expectation of mine was itself wrong twice and I corrected it against measurement rather than reporting it: I first asserted the control would leak raw insight JSON (it does not — the adapter already stripped) and first scored the control's A/B cells on block id alone (which missed A6, where the adapter merges under target.id so the contaminated control picks the right id with the wrong text); both corrections are visible in the re-run log.
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/web-shell/client/App.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/App.test.tsx
file packages/web-shell/client/assistant-turn-settlement.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/assistant-turn-settlement.test.tsx
file packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/DaemonSessionProvider.test.tsx
file packages/web-shell/client/daemon/session/actions.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/actions.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/web-shell/client/App.test.tsx: PPPPP
packages/web-shell/client/assistant-turn-settlement.test.tsx: PPPPP
packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: PPPPP
packages/web-shell/client/daemon/session/actions.test.ts: PPPPP
verdict: pass
summary: 4 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 2 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 3 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 4 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
round 5 · packages/web-shell/client/App.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/assistant-turn-settlement.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/daemon/session/DaemonSessionProvider.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/daemon/session/actions.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtxudi2ozc
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
11 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- U11-1 duplicate-terminal suppression untested (DaemonSessionProvider.tsx:1200) — already reported (round-10 deferral at DaemonSessionProvider.tsx:1148, review 5168168723)
- U11-2 App/SplitView callback forwarding untested (SplitView.tsx:628) — already reported (R8-16; round-4 deferrals App.tsx:2978 and SplitView.tsx:491, review 5146822401; round-6 duplicate App.tsx:2979, review 5154409725)
- U11-3 live-terminal publish path and pre-callback flush untested (DaemonSessionProvider.tsx:3815) — already reported (round-10 deferrals at DaemonSessionProvider.tsx:3633 and DaemonSessionProvider.test.tsx:10852)
- U11-4 widened failedSessionId resolution untested (DaemonSessionProvider.tsx:4129) — already reported (round-10 deferral at DaemonSessionProvider.tsx:3953)
- U11-5 unlisted stopReason values collapse to outcome completed (DaemonSessionProvider.tsx:5249) — already reported (round-10 deferral at DaemonSessionProvider.tsx:5038)
- U11-6 new design doc ships English-only (docs/design/web-shell/web-shell-assistant-turn-settlement.md:1) — already reported (round-10 deferral at the same path)
- U11-7 serverTimestamp precedence in the published timestamp untested (assistant-turn-settlement.test.tsx:148) — already reported (R2-3 comment 3953370589; R5-7 comment 3965944422)
- U11-8 streaming early-exit semantics unpinned (assistant-turn-settlement.test.tsx:292) — already reported (round-10 deferral at assistant-turn-settlement.test.tsx:240)
- U11-9 at-event-time snapshot read not distinguished from mount-time capture (assistant-turn-settlement.test.tsx:107) — already reported (round-9 deferral at assistant-turn-settlement.ts:152, review 5162219880)
- U11-10 kind narrowing has no falsifying test (assistant-turn-settlement.test.tsx:130) — already reported (round-10 deferral at assistant-turn-settlement.test.tsx:132)
- U11-11 replay fixtures omit the envelope promptId (DaemonSessionProvider.test.tsx:10893) — already reported (round-10 deferral at DaemonSessionProvider.test.tsx:20568)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: re-check of the earlier-round blocker threads R1-1, R1-2, R2-1, R3-1 (three threads), R5-1, R5-2, R7-1 and R9-1 — not independently walked this round; the outcome is not a zero-Critical verdict so the open-blocker re-check gate did not apply, and the issue-fidelity agent's fixed/superseded readings of them were not independently verified.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": did not execute packages/web-shell vitest to confirm the two added actions.test.ts cases pass in this worktree (the shared tree's dist/ prerequisites were…; "agent reverse-audit (round 3)": did not empirically probe the React flush ordering (DefaultLane → Scheduler macrotask vs. the publisher's queueMicrotask ) that makes the "reordering the provi…; "agent reverse-audit (round 3)": did not verify whether the SDK transcript reducer leaves an assistant block streaming: true when a turn is cancelled mid-stream — if it does, getSettledAssis…; "agent reverse-audit (round 1)": did not confirm that the daemon stamps envelope-level promptId on session_update / agent_message_chunk frames specifically — bridge.ts:7566-7618 read only i…; "agent reverse-audit (round 1)": did not run a probe (vitest) to demonstrate the ACK-in-flight reload interleaving of finding 1; it is code-read only., and 5 more.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round; 4 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:3820 — [probe] Critical [fails-closed] [new-surface] a repair-suppressed live terminal is never published — the replay publish is admission-gated and the reload failure path…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:2974 — [probe] Critical [fails-closed] [new-surface] a terminal replayed before the submit ACK is dropped for hosts — the admission key is bound only after the ACK and no de…packages/web-shell/client/daemon/session/actions.ts:2738 — [probe] Critical [fails-closed] [new-surface] the stale-session removal retires the admission key of a still-running prompt, so a cancelled turn whose terminal arrives through repla…packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:4761 — [probe] Critical [fails-closed] [new-surface] bindPrompt is never called on the continuation admission path, so a continued turn is permanently invisible to the repla…packages/web-shell/client/adapters/transcriptToMessages.ts:1825 — [probe] assistantVisibleTextOf space-joins text segments the renderer emits as separate messages, diverging from the sibling implementation's newline joinpackages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:1201 — [review] publishedPromptSettlementsRef is never pruned for the provider lifetime, unlike the sibling admission map this same diff deletes per sessionpackages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:5225 — [review] the promptId-less-terminal guard has no falsifying testpackages/web-shell/client/assistant-turn-settlement.test.tsx:512 — [probe] the projection's error.code presence branch is unpinned while the sibling stopReason branch is falsifiable
Convergence: round 11 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/web-shell/client/assistant-turn-settlement.ts (findings in round 8; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 2, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 11 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：re-check of the earlier-round blocker threads R1-1, R1-2, R2-1, R3-1 (three threads), R5-1, R5-2, R7-1 and R9-1 — not independently walked this round; the outcome is not a zero-Critical verdict so the open-blocker re-check gate did not apply, and the issue-fidelity agent's fixed/superseded readings of them were not independently verified.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：did not execute packages/web-shell vitest to confirm the two added actions.test.ts cases pass in this worktree (the shared tree's dist/ prerequisites were…；"agent reverse-audit (round 3)"：did not empirically probe the React flush ordering (DefaultLane → Scheduler macrotask vs. the publisher's queueMicrotask ) that makes the "reordering the provi…；"agent reverse-audit (round 3)"：did not verify whether the SDK transcript reducer leaves an assistant block streaming: true when a turn is cancelled mid-stream — if it does, getSettledAssis…；"agent reverse-audit (round 1)"：did not confirm that the daemon stamps envelope-level promptId on session_update / agent_message_chunk frames specifically — bridge.ts:7566-7618 read only i…；"agent reverse-audit (round 1)"：did not run a probe (vitest) to demonstrate the ACK-in-flight reload interleaving of finding 1; it is code-read only.，另有 5 条。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改；其中 4 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 8 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 11 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/web-shell/client/assistant-turn-settlement.ts（第 8 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 2 条 Critical），首次发现的速率没有下降（本轮 2，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| block?.kind !== 'assistant' || | ||
| block.parentToolCallId !== undefined || | ||
| block.promptId !== promptId |
There was a problem hiding this comment.
[Critical] R11-1: [certifies-falsely] [new-surface] The backward scan that picks the turn's final assistant message excludes subagent-owned blocks, foreign prompt ids, streaming blocks and empty visible text — but not the meta.source values Web Shell itself renders as role: 'system' notices rather than assistant output. The in-repo answer to this same question does exclude them: daemon/session/turn-notification-context.ts:242-243 filters block.meta?.source !== 'background_notification' && block.meta?.source !== 'vision_bridge_notice', and adapters/transcriptToMessages.ts:488-520 renders both sources as role: 'system', variant: 'info'.
A vision_bridge_notice is emitted inside the user's prompt turn, before the model call, as role: 'assistant' with qwenDiscreteMessage: true (cli/src/acp-integration/session/emitters/MessageEmitter.ts:194-217, from Session.ts:#applyBridgeConversionsIfNeeded). The bridge stamps promptId: entry.activePromptId on every frame of that turn (acp-bridge/src/bridgeClient.ts:1068-1075), and qwenDiscreteMessage: true defeats the SDK's merge predicate (sdk-typescript/src/daemon/ui/transcript.ts:913,931), so the notice becomes its own top-level assistant block carrying this prompt's id — non-streaming, non-empty. If the turn then ends before the model produces any assistant text — a cancel right after the notice, the session token limit returning { responseStream: null, stopReason: 'max_tokens' } (Session.ts:8169), or a hard turn_error — the scan finds that notice as the last stamped block and publishes it as the turn's answer. The host receives message.content: 'Vision bridge: converted 1 image…', a system notice, for a turn whose real answer never existed; and because publishPromptSettlement burns the (sessionId, promptId) key before the listener runs, no corrected callback can follow. This contradicts the contract field the diff itself adds — "Final visible assistant message when retained in the mounted transcript" (customization.tsx:243).
Witness:
VISION_BRIDGE_NOTICE_ONLY_LAST: {"sessionId":"session-1","promptId":"prompt-live","outcome":"cancelled",
"stopReason":"cancelled","message":{"id":"notice-2","content":"Vision bridge: converted 1 image",
"isStreaming":false,"timestamp":1}}
VISION_BRIDGE_COMPLETED: {"sessionId":"session-1","promptId":"prompt-live","outcome":"completed",
"stopReason":"max_tokens","message":{"id":"notice-1","content":"Vision bridge: converted 1 image",...}}
flip (the two meta.source exclusions added to this guard):
VISION_BRIDGE_NOTICE_ONLY_LAST: {...,"message":{"id":"assistant-1","content":"Real answer text.",...}}
VISION_BRIDGE_COMPLETED: {"sessionId":"session-1","promptId":"prompt-live","outcome":"completed",
"stopReason":"max_tokens"} <- no message at all
Tests 17 passed (17) (4 probe + the 13 existing projection tests, all still green with the fix applied)
| block?.kind !== 'assistant' || | |
| block.parentToolCallId !== undefined || | |
| block.promptId !== promptId | |
| block?.kind !== 'assistant' || | |
| block.parentToolCallId !== undefined || | |
| block.promptId !== promptId || | |
| block.meta?.source === 'background_notification' || | |
| block.meta?.source === 'vision_bridge_notice' |
Only vision_bridge_notice is live here. background_notification was traced as not reachable: its drain is gated on !this.pendingPrompt twice (Session.ts:10183-10190, :10202-10205) and settleActivePromptState deletes entry.activePromptId when the prompt settles (bridge.ts:3705-3707), so those frames are published with no active prompt to stamp and the existing block.promptId !== promptId term already excludes them — which is exactly the case the rationale comment above this guard argues for. Adding that term is defensive parity with the sibling implementation, not a second fix. The exclusion set to match is turn-notification-context.ts:242-243.
Please add the witness test beside does not return subagent-owned assistant text as the turn answer: a prompt-stamped vision_bridge_notice block trailing a real answer must not win the selection, and a notice that is the only stamped block must publish no message. Removing either meta?.source term from the guard should turn it red.
中文说明
挑选本回合最终 assistant 消息的反向扫描，排除了 subagent 所属 block、外部 prompt id、streaming block 和可见文本为空的 block——但没有排除 Web Shell 自己渲染成 role: 'system' 提示（而非 assistant 输出）的那些 meta.source。仓库里回答同一个问题的代码是排除的：daemon/session/turn-notification-context.ts:242-243 过滤了 block.meta?.source !== 'background_notification' && block.meta?.source !== 'vision_bridge_notice'，而 adapters/transcriptToMessages.ts:488-520 把这两种 source 都渲染为 role: 'system'、variant: 'info'。
vision_bridge_notice 是在用户 prompt 回合内部、模型调用之前发出的，形如 role: 'assistant' 且带 qwenDiscreteMessage: true（cli/src/acp-integration/session/emitters/MessageEmitter.ts:194-217，来自 Session.ts:#applyBridgeConversionsIfNeeded）。bridge 会给该回合的每一帧盖上 promptId: entry.activePromptId（acp-bridge/src/bridgeClient.ts:1068-1075），而 qwenDiscreteMessage: true 会让 SDK 的合并判定失效（sdk-typescript/src/daemon/ui/transcript.ts:913,931），于是这条提示成为一个独立的顶层 assistant block，带着本 prompt 的 id，且非 streaming、非空。如果回合在模型产出任何 assistant 文本之前就结束——提示发出后立即取消、会话 token 上限返回 { responseStream: null, stopReason: 'max_tokens' }（Session.ts:8169）、或硬性 turn_error——扫描就会把这条提示当作最后一个带戳 block，并作为该回合的答案发布出去。宿主会收到 message.content: 'Vision bridge: converted 1 image…'，也就是一条系统提示，而这个回合真正的答案从未存在；并且由于 publishPromptSettlement 在监听器运行之前就烧掉了 (sessionId, promptId) 键，后续无法再发出更正的回调。这与本 diff 自己新增的契约字段相矛盾——"Final visible assistant message when retained in the mounted transcript"（customization.tsx:243）。
（上方证人证据为程序输出，按原样保留不译。）
（上方 suggestion 代码块不重复翻译。）
这里只有 vision_bridge_notice 是真实可达的。background_notification 经追踪不可达：它的排空在 Session.ts:10183-10190 与 :10202-10205 两处都以 !this.pendingPrompt 为前置条件，且 settleActivePromptState 会在 prompt 结束时删除 entry.activePromptId（bridge.ts:3705-3707），所以这些帧发出时没有可盖戳的 active prompt，现有的 block.promptId !== promptId 一项就已经把它们排除了——这正是该守卫上方注释所论证的情形。因此加上这一项是与同类实现保持防御性一致，而不是第二处修复。要对齐的排除集合是 turn-notification-context.ts:242-243。
请在 does not return subagent-owned assistant text as the turn answer 旁边补上这个证人测试：一个带本 prompt 戳、位于真实回答之后的 vision_bridge_notice block 不应赢得选择；而当该提示是唯一带戳 block 时，不应发布 message。从守卫中移除任一 meta?.source 项，该测试都应变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
[REAL at 64a996b] — verified by reading the head, and by reading each cited precedent.
Own evidence
- The guard really has no
meta.sourceterm:packages/web-shell/client/assistant-turn-settlement.ts:49-53is onlyblock?.kind !== 'assistant' || block.parentToolCallId !== undefined || block.promptId !== promptId— read at this head. - The precedent does exclude both sources:
packages/web-shell/client/daemon/session/turn-notification-context.ts:242-243readsblock.meta?.source !== 'background_notification' && block.meta?.source !== 'vision_bridge_notice'. - The notice renders as a system notice, not assistant output:
packages/web-shell/client/adapters/transcriptToMessages.ts:506-517mapsmeta['source'] === 'vision_bridge_notice'torole: 'system',variant: 'info'.assistantVisibleTextOf(:1817-1827) only strips insight frames, so a notice's text survives the visibility filter. - Emission/stamping chain confirmed:
packages/cli/src/acp-integration/session/emitters/MessageEmitter.ts:194-220(role: 'assistant',source: 'vision_bridge_notice',qwenDiscreteMessage: true);packages/acp-bridge/src/bridgeClient.ts:1068-1075stampspromptId: entry.activePromptIdon that turn's frames;packages/sdk-typescript/src/daemon/ui/transcript.ts:913,931let aqwenDiscreteMessageblock survive as its own top-level assistant block. The module comment cites the SDK as the source of its exclusion terms — true for what it copied, but the SDK's ownfindFinalVisibleAssistantForPrompt(packages/sdk-typescript/src/daemon/ui/transcript.ts:935-951) has nometa.sourceterm either, so the notification path is the correct precedent, as the finding says. I did not run the witness end to end (nonode_modulesin this read-only worktree); the verdict rests on the mechanism read at this head.
No code change this round — the closeout scope fuse is met twice: the diff is 1549 insertions / 11 deletions against merge-base ee6003f5 (git diff --shortstat), and these are R11 findings arriving after fix commits on 09-09/09-10 plus a 09-12 main merge with the review still not converging. So this round is reply + thread organisation only; the PR is human-gated.
What a human has to settle: the definition of the contract field this diff adds — customization.tsx says "final visible assistant message", and a vision_bridge_notice is assistant-authored and stamped to this prompt, yet it is not an answer. Decide whether the exclusion set is "every source the render adapter maps to role: 'system'" (one shared, exported list) or per-consumer lists — that choice is what makes this class of gap closeable once instead of once per consumer.
| replaySettlement && | ||
| locallyBoundPromptIdsRef.current | ||
| .get(replaySettlement.sessionId) | ||
| ?.has(replaySettlement.promptId) |
There was a problem hiding this comment.
[Critical] R11-2: [certifies-falsely] [new-surface] This replay publish has no equivalent of the live path's !repairTargetsTerminal guard, so a load that arms a live-journal repair for the same prompt whose terminal it is replaying publishes a settlement whose retained transcript the provider already knows is incomplete. The live path deliberately withholds publication in exactly that state so the host reads the repaired projection (:3820-3822); the replay path publishes unconditionally.
Concretely: a reconnect load replays turn_complete for prompt P while the same load records a recoverable history_truncated { scope: 'live_journal' } repair episode targeting P. The host then receives onAssistantTurnSettled with a message built from the truncated projection, and because publishPromptSettlement burns the (sessionId, promptId) key before the listener runs, the corrected post-repair answer can never be delivered. This is the inverse asymmetry of the repair-suppression gap already recorded against the live path: that one suppresses too much and loses the settlement, this one suppresses too little and certifies a partial answer as the turn's final visible message. The module's own comment states why that is unrecoverable — "publishing partial text is unrecoverable, as no corrected callback can follow" (assistant-turn-settlement.ts:57-58).
Witness:
The asymmetry, read off the reviewed commit:
live path DaemonSessionProvider.tsx:3820-3822
if (settlement && !repairTargetsTerminal) { publishPromptSettlement(settlement); }
replay path DaemonSessionProvider.tsx:2972-2979
if (replaySettlement && locallyBoundPromptIdsRef.current.get(...)?.has(...)) {
publishPromptSettlement(replaySettlement); <- no repair check
}
Reproduced end to end: the replay arm publishes while a repair episode targets the same
promptId, and the delivered message.content is the truncated projection.
baseline: the entire replay-publish block is `+` lines in this diff (@@ -2887,6 +2958,25 @@);
it does not exist at the merge base.
The fix should give the replay path the same repair awareness as the live path — when the load arms, or already holds, a live-journal repair episode targeting this settlement's promptId, defer publication until the repair resolves rather than publishing the truncated projection:
// inside the replay loop, collect instead of publishing when a repair targets this prompt
if (replaySettlement && locallyBoundPromptIdsRef.current
.get(replaySettlement.sessionId)?.has(replaySettlement.promptId)) {
if (repairTargetsPrompt(replaySettlement.promptId)) {
deferredRepairSettlements.push(replaySettlement);
} else {
publishPromptSettlement(replaySettlement);
}
}Any deferral must still route through publishPromptSettlement so the dedup key and the unbind run exactly once, and it must not re-read the replay snapshot: activeSession.consumeReplaySnapshot() at :3000 releases it (its own comment notes snapshots "can reach tens of MiB"). replaySettlement is already materialized, so deferring it needs no second read. The premise a fix must not violate is assistant-turn-settlement.ts:57-58 together with the key burn at :1200-1201 — publishing eagerly and correcting later cannot work, which is why the deferral, not an extra publish, is the shape that closes this.
Please add a case beside the existing settlement block: a replay snapshot carrying both turn_complete for prompt-1 and a recoverable history_truncated { scope: 'live_journal' } marker for the same prompt, asserting the listener either receives nothing until the repair resolves or receives a message.content equal to the repaired text. Removing the new guard should turn it red.
中文说明
这处 replay 发布没有 live 路径那样的 !repairTargetsTerminal 守卫，因此当一次加载既为某个 prompt 建立了 live-journal 修复（repair）episode、又在同一批 replay 中回放该 prompt 的终态时，它会把一个 provider 已知保留 transcript 不完整的 settlement 发布出去。live 路径在这种状态下是故意不发布的，好让宿主读到修复后的投影（:3820-3822）；而 replay 路径无条件发布。
具体场景：一次重连加载回放了 prompt P 的 turn_complete，同时这次加载又记录了一个针对 P 的、可恢复的 history_truncated { scope: 'live_journal' } 修复 episode。于是宿主收到 onAssistantTurnSettled，其 message 是用被截断的投影构造的；并且由于 publishPromptSettlement 在监听器运行之前就烧掉了 (sessionId, promptId) 键，修复后更正的答案永远无法再投递。这与已记录在 live 路径上的「修复抑制」缺口恰好是反向的不对称：那一个抑制得太多、丢掉了 settlement，这一个抑制得太少、把部分答案当作该回合最终可见消息认证了出去。模块自己的注释说明了为什么这不可恢复——"publishing partial text is unrecoverable, as no corrected callback can follow"（assistant-turn-settlement.ts:57-58）。
（上方证人证据为代码引用与程序输出，按原样保留不译；上方代码块不重复翻译。）
修复应当让 replay 路径具备与 live 路径同样的修复感知能力——当本次加载建立了、或已持有一个针对该 settlement 的 promptId 的 live-journal 修复 episode 时，把发布推迟到修复完成，而不是发布被截断的投影。任何推迟仍必须走 publishPromptSettlement，以保证去重键与 unbind 恰好执行一次；并且不得重新读取 replay 快照：:3000 的 activeSession.consumeReplaySnapshot() 会释放它（其注释指出快照"可达数十 MiB"）。replaySettlement 已经构造完成，推迟它不需要二次读取。修复不能违反的前提是 assistant-turn-settlement.ts:57-58 以及 :1200-1201 的键烧除——先发布再更正是行不通的，这也是为什么能闭合此问题的是「推迟」而不是「多发布一次」。
请在现有 settlement 测试块旁边补一个用例：一个 replay 快照同时携带 prompt-1 的 turn_complete 和针对同一 prompt 的可恢复 history_truncated { scope: 'live_journal' } 标记，断言监听器要么在修复完成前什么都收不到，要么收到的 message.content 等于修复后的文本。移除新增守卫时该测试应变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
There was a problem hiding this comment.
[REAL at 64a996b] — verified by reading the head; the two paths are asymmetric as described.
Own evidence
- Replay publish, read at this head:
packages/web-shell/client/daemon/session/DaemonSessionProvider.tsx:2972-2978— the gate is only the admission check (locallyBoundPromptIdsRef.current.get(replaySettlement.sessionId)?.has(replaySettlement.promptId)) thenpublishPromptSettlement(replaySettlement); no repair term. - Live publish:
:3619-3624computesrepairTargetsTerminalfromliveJournalRepairRef.current, and:3820-3821publishes onlyif (settlement && !repairTargetsTerminal). The asymmetry is real. - Repair armed in the same load, before the replay loop:
:2846-2880setsliveJournalRepairRef.currentfromreplayTarget(withterminalSeen: false), while the replay loop runs at:2947overreplayEvents = [...compactedReplay, ...liveJournal](:2545). - Co-occurrence is coherent, not hypothetical:
findLiveJournalRepairTarget(packages/web-shell/client/daemon/session/live-journal-repair.ts:21-46) derives the target promptId from thehistory_truncated { scope: 'live_journal' }marker, and the siblingfindLiveJournalRepairSuffix(:48-64) exists precisely to locate that prompt's user chunk andturn_complete/turn_errorinside the same replay events — i.e. replayed terminal + armed repair for one prompt is an expected shape. - The key burn is where the finding says:
publishPromptSettlementat:1192-1201computes the key, unbinds, returns early if already published, and the listener runs later in aqueueMicrotask— so an eager replay publish cannot be corrected afterwards.
No code change this round — the fuse is met twice: 1549 insertions / 11 deletions versus merge-base ee6003f5, and these are R11 findings landing after fixes on 09-09/09-10 and a 09-12 main merge without convergence. Reply + thread organisation only; the PR is human-gated. I did not execute the end-to-end replay case (read-only worktree, no node_modules).
What a human has to settle: one rule for both paths, not two. The live path suppresses too much (loses the settlement), the replay path suppresses too little (certifies a truncated projection) — so the decision is whether a repair-targeted prompt publishes nothing until the repair resolves (deferral, which needs a resolution hook the provider does not currently have) or nothing at all from that load. It also has to decide whether deferral may outlive the load at all, given activeSession.consumeReplaySnapshot() at :3000 releases the snapshot the settlement was built from.
|
Integrity audit of this round's resolved review threads (run Four threads were marked resolved with the bot's
All four stay resolved; no code changed and nothing was pushed. The two remaining open Criticals stay at human-gate. Open maintainer decision: this PR is scope-fused (1549 additions, over the 1500-line ceiling), so no code lands this round. |
3 participants
What this PR does
Adds an optional host callback for daemon-authoritative assistant turn settlement. It reports the session and prompt identity, completed/cancelled/failed outcome, stop reason, error details, and the final retained assistant message when available. Delivery occurs after the terminal transcript update is committed, suppresses duplicate terminals per mounted session provider, supports Split View panes, and leaves the existing session-change callback unchanged.
Why it's needed
Embedding hosts currently infer completion from rendered loading state and scan the transcript for a response. That loses authoritative prompt identity and is ambiguous across cancellation, failure, reconnect catch-up, session switching, and history replay.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — host integration API with no visual change.
Tested on
Environment (optional)
No local validation result is claimed. Earlier commands stopped during unrelated missing-dependency/module resolution; the change was then handled only by static diff review.
Risk & Scope
(sessionId, promptId)as the durable idempotency key.Linked Issues
Closes #10389
中文说明
本 PR 做了什么
新增一个可选的宿主回调，用 daemon 权威终态报告 assistant turn 已结束。事件包含 session 与 prompt 标识、completed/cancelled/failed 结果、stop reason、错误信息，以及存在时最终保留的 assistant 消息。回调在终态 transcript 更新提交后触发；同一个已挂载 session provider 内会抑制重复终态；Split View 各 pane 也会转发；现有 session-change 回调保持不变。
为什么需要
目前嵌入宿主只能根据渲染层 loading 状态推断完成，再扫描 transcript 查找回复。这种方式没有权威 prompt 标识，并且在取消、失败、断线重连补齐、session 切换和历史回放场景下存在歧义。
Reviewer Test Plan
如何验证
证据（Before & After）
N/A —— 这是宿主集成 API，没有视觉变化。
已测试平台
环境（可选）
本次不声明任何本地验证结果。此前命令在无关的依赖缺失 / 模块解析阶段即停止；之后仅通过静态 diff review 处理本改动。
风险与范围
(sessionId, promptId)作为持久幂等键。关联 Issue
Closes #10389