fix(cli): deliver teammate messages at tool-round boundaries, not whole-task end - #9638
fix(cli): deliver teammate messages at tool-round boundaries, not whole-task end#9638yiliang114 wants to merge 25 commits into
Conversation
…le-task end Teammate→leader messages were only drained into the leader's session when `streamingState === Idle`. During a long multi-round agentic task `streamingState` never reaches Idle between rounds (tool calls are continuously scheduled/executing or terminal-but-unsubmitted), so queued teammate messages waited for the entire task — minutes — even though the tool description promises delivery when the current turn ends (#8172). Drain `teammateQueueRef` at the tool-round boundary in `handleCompletedTools`, appending the envelopes after the tool-response parts of the next `SendMessageType.ToolResult` submission — the same mechanism and ordering already used for steer messages at that exact site (tool_result blocks lead the user message). The existing Idle drain stays as the fallback for turns that end without another tool round. Race/loss safety: the drain is skipped on cancelled boundaries and generation-change-surviving continuations (mirroring steer), and the batch is restored (idempotently) on cancel/preempt/admission/delivery failure so messages are never lost or double-delivered. The `isSubmittingQueryRef` guard (#4844) on the Idle path is untouched.
|
Re-run at head
The gate passes on direction; the size escalation means this run cannot auto-approve regardless of review outcome — and the review itself found a concrete blocker at this head (see the Stage 2 comment). 中文说明在头提交
方向上通过门槛；规模升级意味着无论评审结果如何本次运行都不能自动批准——而评审本身在这个头提交上发现了一个具体阻断项（见 Stage 2 评论）。 — Qwen Code · qwen3.8-max Reviewed at |
Code review at
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) — packages/core suite | ❌ failure — client.test.ts 2 failed / 22196 passed (see findings) |
| Test (ubuntu-latest, Node 22.x) — packages/cli suite | |
| Test (macos-latest / windows-latest, Node 22.x) | ⏭️ skipped at this head |
| Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
| Integration Tests (no-AK, No Sandbox) | ✅ success |
| Desktop Shell (ubuntu-22.04 / windows-2022) | ✅ success |
| Security Checks (Secret scan, Dependency CVE audit) | ✅ success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ⏳ queued |
| Qwen Code CI (workflow) | 🔄 in progress |
Sandboxed verification would settle the one claim CI cannot — that envelopes still arrive at round boundaries without loss or duplication under the new snapshot settlement: the /verify run triggered alongside this re-run is already in flight; re-trigger @qwen-code /verify once the suite is green.
中文说明
在 1c23c6f2 上的代码审查 —— R15-1 已确认修复，但该头提交自身的测试套件是红的
R15-1 的修复在结构上是正确的。 上一轮的 Critical 是 client.ts 外层 finally 里的 restoreStrippedRetryEntries() 仍在用全局 user-content push 计数器决定是否重新添加——而紧邻的载体结算早已放弃了同样的假设。这个头提交按 R15-1 要求的"按每次发送锚定"替换了该判定，且以下结论是我从 diff 重新推导的，不是照抄收尾说明：
LlmChat.sendMessageStream在 push 之前、且 publish 与 push 之间没有任何 await 的情况下，把 push 前的计数器以userContentPushSnapshotKey发布到调用方的 request 数组上（llm-chat.ts）——push 周围的比较窗口为空。client.ts中的restoreStrippedRetryEntries()与settleSteerInput()：没有发布快照时无条件恢复（"这次发送确定没有到达 push 点"），有快照则与发布的快照比较。unwind 窗口内的并发/btwpush 不再能冒充本次发送的计数器增长——正是 R15-1 的失败形态。- 两个提前退出分支（hook 失败、Goal 准入失败）在重新抛出之前完成载体结算并把弹出的 retry 条目加回历史，堵住了绕过结算 try/finally 的出口。
client.test.ts的新测试为每条结算路径复现了并发 push 窗口；改写后的restores stripped retry entries when only a concurrent send pushes就是 R15-1 形态的直接回归钉。
阻断项：Test (ubuntu-latest, Node 22.x) 在该头提交上失败——2 个失败用例都在本 PR 的改动区域内。 packages/core 套件结果为 Test Files 1 failed | 616 passed、Tests 2 failed | 22196 passed，两个失败都在 client.test.ts > sendMessageStream > retry sendMessageType：
does not re-add stripped retry entries when the chat already pushed them before failingdoes not re-add stripped retry entries when auto-compression shrank history below the pre-send length after the push
两者都是钉住旧全局计数器判定的既有测试，而本提交恰恰替换了该判定，它们没有被同步更新：其 mockTurnRunFn 只通过递增 getUserContentPushCount 模拟 push，从不发布 userContentPushSnapshotKey，于是新代码读成"无快照 ⇒ 本次发送从未 push ⇒ 无条件恢复"，把条目加了回去，expect(mockChat.addHistory).not.toHaveBeenCalled() 因此失败（已对照 client.test.ts:11041 的失败摘录核实）。diff 已把四个同类测试更新为发布快照的"微缩契约"——这两个被漏掉了。这两个测试守护的生产行为其实仍然成立：真实 LlmChat 总是在 push 前一刻发布快照，"已 push ⇒ 抑制重加"在生产中不变，因此修复只需改测试——给这两个 mock 补上同类测试已有的微缩契约即可（压缩那个测试要在模拟压缩之后再发布，与 llm-chat.ts 的实际顺序一致）。
增量其余部分复查无问题：CLI 侧的排空/结算（drainTeammateQueue 的代际守卫、settleDrainedTeammates 两种结果都剥离、带指纹的 retry 债务、retryLastPrompt 里的准入门预检）与历轮钉住的行为一致，use-llm-stream.test.tsx 新增的 21 个用例覆盖了轮次边界路径、swap 丢弃与 Ctrl+Y 循环。不再重提：第 15 轮记录的两条建议级延后项（swap-drop 注释位置；retry carrier restore 分支缺测试配对）——仍然存在，仍然非阻断。
测试证据——通过 API 读取 1c23c6f2 上本 PR 自己的 CI（本次运行不执行任何 PR 代码）：
失败腿确系本 PR 所致——两个失败用例恰好位于本提交重写契约的区域，且在它们为之编写的旧判定下是通过的。日志摘录见英文部分。一个如实的说明：本轮 packages/core 失败后 packages/cli 单测套件未跑完，Integration Tests (CLI, No Sandbox) 与 macos/windows 腿在该头提交上为跳过状态——因此钉修复之后的绿灯重跑仍需展示完整矩阵。
CI 明细表见英文部分的 qwen-triage-ci 区块（表格不重复翻译）。
沙箱验证可以了结 CI 无法证明的那条行为主张——新快照结算下信封仍在轮次边界送达、不丢不重：随本次重跑触发的 /verify 已在运行中；套件转绿后可再次触发 @qwen-code /verify。
— Qwen Code · qwen3.8-max
Reviewed at 1c23c6f29f0f1192c43c7c422b00f0a69a2d463e · re-run with @qwen-code /triage
|
Confidence: 2/5 — the R15-1 fix is structurally right and re-derived from the code, but this head ships with the PR's own suite red in the exact region it rewrote, and nothing approves on top of that. Stepping back over the whole picture:
Verdict: request changes — solely for the red suite at this head. @yiliang114 the two tests named in the Stage 2 comment need the same snapshot miniature their siblings got (compression test: publish after the simulated compression); everything else in this round checks out, and a green full-matrix re-run is the remaining gate before the maintainer call. 中文说明置信度：2/5 —— R15-1 的修复在结构上是正确的，且是从代码重新推导核实的；但该头提交携带着自身测试套件在其重写区域内变红，任何批准都不能建立在这之上。 退一步看整体：
结论：request changes —— 仅因该头提交上的红色套件。@yiliang114 Stage 2 评论点名的两个测试需要补上同类测试已有的快照微缩契约（压缩那个测试：在模拟压缩之后发布）；本轮其余部分均核实无问题，转绿后的全矩阵重跑是维护者拍板前剩下的唯一门槛。 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
…ot failure Review round on #9638 found two real loss/duplication edges in the tool-round boundary teammate drain, plus test/observability gaps: - A drained envelope was delivered TWICE when the boundary submission failed after being accepted (user cancel mid-stream, terminal API error): GeminiChat pushes the submission's user content before any model attempt, yet onDeliveryFailed unconditionally requeued the envelope and the Idle drain resubmitted it. - A blocking UserPromptSubmit hook permanently LOST the envelope: ToolResult is not in the hook's exclusion list, the blocked stream dispatched onDelivered, and nothing restored the drained queue. - Boundary deliveries skipped chat recording entirely (recordNotification is keyed on Teammate/Notification), so resumed sessions lost both the notification item and the envelope from the reconstructed context. Settle the drained batch by acceptance instead of by failure: snapshot GeminiChat's user-content push counter (the same signal settleSteerInput uses in client.ts) before the submission; on failure, requeue only when the counter did not advance, and when it did, record the delivery via recordNotification instead of redelivering. Count a UserPromptSubmitBlocked stream as a delivery failure so blocked submissions restore their drained payloads. Also extract the drain protocol (splice + display marking + idempotent restore) into one drainTeammateQueue helper shared by the boundary and Idle paths, add debug logs at the drain/record/restore transitions, and shrink the #8172 test harness to a wrapper over renderTestHook. New regression tests pin the no-duplicate, hook-block restore, recording, and survivesGenerationChange-skip behaviors (mutation-checked).
Review closeout — round pinned at fd6bce2All 8 unresolved threads from the 07:11Z Criticals
Suggestions (all fixed)
Verification — |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review of ba478fa7 — round-2 findings addressed, no new issues
Round-2 Criticals re-verified
R2-1 (C) userPromptBlockedRef is a hook-wide singleton → Fixed. The userPromptBlocked flag is now a local boolean inside processStream, set in the event loop and returned in StreamProcessingResult.userPromptBlocked. The handleCompletedTools check reads the per-submission processingResult rather than a hook-wide ref.
R2-2 (C) acceptance snapshot of GeminiChat's global user-content push counter spans too wide → Fixed. The acceptance/restore decision is now carried by submissionSettlement (a SteerInput-shaped object passed through the existing steerInput option), so GeminiClient calls accept()/restore() at send entry, next to the actual history push. No hook-wide counter needed.
Round-2 Suggestions re-verified
R2-3 (S) single-queue test only → Fixed. New test delivers and records every envelope in a boundary batch exercises a 2-message batch.
R2-4 (S) per-submission reset unpinned → Fixed. The userPromptBlocked flag is now a local variable inside processStream, naturally reset on each submission.
Round-1 carry-over check
All 8 round-1 findings (2 Criticals, 6 Suggestions) were confirmed fixed in 45e5abda and remain fixed at ba478fa7. The two Criticals (R1-1 double-delivery, R1-2 UserPromptSubmit loss) are addressed by the settlement protocol; the remaining Suggestions are covered by the drainTeammateQueue shared protocol, the survivesGenerationChange test, debugLogger.debug calls, recordNotification journaling, and the batch-level test.
What the new commit (ba478fa7) does
The commit replaces the userPromptBlockedRef + GeminiChat push-counter snapshot approach with a submissionSettlement object that bundles steer and teammate settlement into one SteerInput-compatible carrier. The GeminiClient drives the accept()/restore() callbacks at send entry, which is the correct point to decide whether the drained batch reached the model. This eliminates the two Criticals in one clean refactor.
Code review
The changes are well-structured:
-
submissionSettlementbundles steer + teammate. When both are present,accept()calls bothdrainedSteer?.accept()andsettleDrainedTeammates(true);restore()calls both restores. When only one is present, the other's?.optional chaining makes it a no-op. Thepartsfield defaults to[]when steer is absent, which is harmless. -
settleDrainedTeammatesis idempotent. ThedrainedTeammatesvariable is set toundefinedafter the first settlement call, soonDeliveryFailedafteronDelivered(or vice versa, should a bug in the callback contract arise) is a no-op. -
userPromptBlockedgatesonDeliveryFailed. TheprocessingResult.userPromptBlockedis OR'd withlastPromptErroredRefandgoalTerminalErrorRefto trigger theonDeliveryFailedcallback, which callssubmissionSettlement?.restore()— the correct restore path for hook-blocked submissions. -
onAdmissionFailednow restores steer too. This is a latent bugfix in the existing steer path — the original code only restored steer ononDeliveryFailed, not ononAdmissionFailed. The PR unifies both restore sites. -
recordNotificationjournals boundary deliveries. The accepted-route callsconfig.getChatRecordingService()?.recordNotification()with the batch, matching the Idle-path Teammate submission's journaling. This closes R1-8. -
Tests exercise all failure paths. The 8-test suite covers: core injection, Idle fallback, cancel-no-loss, second-message-after-boundary, accepted-then-failed mid-stream (no-redundant-delivery), UserPromptSubmit hook block + Idle recovery, multi-envelope batch, and
survivesGenerationChangeexclusion.
Remaining observations (none are blockers)
-
The
onAdmissionFailedsteer restore is a latent bugfix. The old code did not restore steer on admission failure, which means a misbehaving admission gate could silently lose steer messages. This PR fixes it by restoring both steer and teammate on admission failure. This is correct behavior — no regression risk. -
No CI visibility at
ba478fa7. The last CI run was atfd6bce2; the two subsequent commits (round-1 fix, round-2 fix) have not been pushed through CI. The author should be prepared for CI to catch any integration issues.
Verdict
All prior Criticals and Suggestions are addressed. The changes are correct, the failure-path coverage is thorough, and the code is clean. No new issues found.
— manual review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6b": none — all planned checks completed (no check was cut short)..
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.ts:5300 — [review] Boundary recordNotification journaling duplicates the teammate journaling contract in core client.tspackages/cli/src/ui/hooks/useGeminiStream.ts:689 — [probe] The display-once guard in drainTeammateQueue is load-bearing but pinned by no test
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6b"：none — all planned checks completed (no check was cut short).。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
The attached steer-input carrier (which the #8172 teammate boundary settlement rides) was decided from a push-counter snapshot taken at sendMessageStream generator entry, before the awaited UserPromptSubmit hook. ToolResult submissions are not hook-exempt, so a concurrent submission admitted during the hook await could push its own content into the global counter and supply the observed push for a send that never pushed: a hook-blocked or cancelled round settled as accepted, journaling a delivery that never happened while the drained teammate envelope was never requeued — silent message loss. - Blocked sends and hook failures exit before the settlement try/finally and provably never pushed: restore the carrier unconditionally instead of comparing the counter. - Re-snapshot the counter immediately before `turn.run` (after the hook await) so sends that reach the push compare against the tightest window; exits before `turn.run` restore unconditionally. - Drop the dead push-counter fixture from useGeminiStream.test.tsx (nothing under test reads it at this head), correct the settlement wrapper comment, and pin the Idle teammate drain's goal-claim deferral restore path plus its exactly-once redelivery.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.test.tsx:1630 — [probe] blocked-round test shim settles the carrier itself, masking the userPromptBlocked delivery-decision mutation
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…load When a boundary submission fails before the history push (e.g. a UserPromptSubmit hook that throws on the ToolResult prompt), the drained teammate batch is restored to the queue, but the same envelopes stay baked into lastPromptRef. A Ctrl+Y retry then re-sends them while the queue still holds them, so the leader receives the report twice (retry + Idle drain). Strip the restored envelopes from the retry payload so the queue redelivery is the single source.
restoreSteerInput duplicated settleSteerInput's idempotence guard, try/catch, and failure warning — only the decision differed. settleSteerInput now takes an optional pushCountBefore: undefined marks a send that provably never pushed and restores unconditionally. Keeps the 'settle each carrier exactly once' invariant in one closure.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 629 passed · 0 failed · 629 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：629 通过 · 0 失败 · 629 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportVerification report — PR #9638Verdict: 中文摘要
Central claim + A/BCentral claim: teammate→leader messages queued during a multi-round tool task are delivered at the next tool-round boundary — appended to the Secondary claims: (1) the steer-carrier settlement in A/B cells: base =
Witnesses:
Witness: Corrections
Findings (non-blocking, completeness)
Neither survivor is a merge condition; both are defense-in-depth with their load-bearing siblings pinned red in the matrix. Mutation matrixWitness:
Positive controls: M1/M8 are the known-pin mutations (the PR's red→green tests), landed in the same files they mutate; every core guard kills exactly its own test (1:1 attribution). No mutant regressed a green test to red outside its pin set. Targeted gates
Not covered
MethodologyEnvironment: CI Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.ts:5347 — [probe] composite steer+teammate carrier never tested with both queues drained
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.test.tsx:1960 — [review] R5-1 still stands (round 5, code unchanged since): Ctrl+Y test rebuilds the renderBusyMultiRoundTask harness inline; the two settlement shims have drifted (shared shim accep…packages/core/src/core/client.ts:2761 — [review] R5-2 still stands (round 5, code unchanged since): hook-failure catch settles the carrier unconditionally, but the sibling goal-admission catch (~2821) still rethrows without settling (covere…packages/cli/src/ui/hooks/useGeminiStream.ts:5330 — [review] R5-3 still stands (round 5, code unchanged since): the trailing-match strip guard is load-bearing but pinned by no test — an unconditional strip leaves all tests passingpackages/cli/src/ui/hooks/useGeminiStream.test.tsx:394 — [review] Scheduler mock updated to the production 3-tuple here, but four sibling mocks in the same file were left on the stale 4-tuple (mockCancelAllToolCalls in the markToolsAsSubmit…packages/cli/src/ui/hooks/useGeminiStream.ts:4031 — [probe] userPromptBlocked delivery-settlement condition is not effectively pinned — removing it leaves 240/240 tests green (test shim settles the carrier before the decision runs)packages/cli/src/ui/hooks/useGeminiStream.ts:5351 — [probe] Composite steer+teammate carrier never tested with both queues drained — an either/or accept mutant survives 240/240 (re-report of the round-5 deferred probe)packages/cli/src/ui/hooks/useGeminiStream.ts:5388 — [probe] Boundary onAdmissionFailed restore is untested — deleting it survives all 240 tests (silent message loss if regressed)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
GeminiChat now publishes the user-content push counter on the request array immediately before pushing it into history, and GeminiClient settles the attached steer/teammate carrier against that push-site snapshot instead of a client-side one: a snapshot taken before turn.run still covers the send-lock and tryCompress awaits ahead of the push, where a concurrently admitted send (/btw) can push and supply the counter growth that reads as acceptance for a send that then exits before its own push (silent teammate loss plus a false delivery journal). A send that exits before the publish never pushed and restores unconditionally. Also settle the attached carrier on the Goal turn admission failure path, which rethrows before the settlement try/finally and would otherwise leak the carrier (drained messages neither delivered nor requeued) for future attachers without their own onDeliveryFailed fallback.
…ntract The Ctrl+Y test rebuilt ~40 lines of the renderBusyMultiRoundTask harness inline because the shared shim settled in a finally (a throwing stream accepted) while the real GeminiClient restores on any pre-push exit. Fold the inline harness back into the shared one and align the single shim with the real contract: blocked sends and pre-push throws restore, while a push that landed (first event observed / completed stream) accepts — including mid-stream failures and consumer abandonment after the first event.
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:
- userPromptBlocked delivery-settlement condition unpinned by tests (packages/cli/src/ui/hooks/useGeminiStream.ts:4058) — already reported and deferred in the round-6/7/9 review bodies (review 4998826762)
- onAdmissionFailed carrier restore untested (packages/cli/src/ui/hooks/useGeminiStream.ts:5616) — already reported and deferred in the round-6/9 review bodies (review 4998826762)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.test.tsx:1966 — [review] no test pins the production-normal full-fingerprint re-attach path (positive tests exercise only the fallback fingerprint)packages/cli/src/ui/hooks/useGeminiStream.test.tsx:1639 — [probe] settlement shim accepts cancel-before-push streams the real client restores
Convergence: round 12 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/ui/hooks/useGeminiStream.ts (findings in round 11; 4 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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 12 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/ui/hooks/useGeminiStream.ts（第 11 轮已出过发现，本轮又有 4 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
…es, and team swaps Four Criticals from the round-14 review of the boundary envelope retry debt mechanism: 1. TeamManager swap leaked in-flight boundary batches: the swap handler cleared only the queue, while a batch already drained into a tool-round submission survived in the settlement closure and its restore requeued it into the NEW team's session. A queue generation counter now makes the restore drop the batch and the settlement skip journal/debt once a swap moved the generation. 2. Retry debt was one-shot: reattach consumed the debt and nothing recorded debt for the retry's own re-pushed entry, so an envelope surviving one retry could be permanently popped by a later retry of a different payload while the journal claimed delivered. The consumed records now transfer into a settlement carrier on the retry's own submission: accept records debt for the retry's pushed entry, restore re-records the original records (core re-adds popped entries as-is when the push never landed). 3. Debt was consumed during argument evaluation before submitQuery's admission gate ran, so a lease-rejected Ctrl+Y permanently discarded it. retryLastPrompt now bails on isSubmittingQueryRef before evaluating the debt (for Retry the gate rejects exactly when the lease is held, and the path to the gate is synchronous). 4. Debt was recorded only when the accept-time strip matched lastPromptRef, but a concurrent submission admitted during the time-to-first-token window overwrites it and the orphan pop drops the accepted entry regardless. Debt is now recorded unconditionally on accept; the retry-time orphan check still keeps double delivery impossible. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…p guards Regression pins for the four round-14 Criticals: - a boundary batch restored after a TeamManager swap is dropped, never resubmitted into the new team's session - an accepted-after-swap batch is neither journaled nor recorded as retry debt against the new team - an envelope re-attached by one retry stays protected when that retry's own entry is orphaned by a later different payload (debt transfers through the retry's settlement carrier) - a lease-rejected Ctrl+Y does not discard the debt (no history scan, no submission, debt still usable afterwards) - a concurrent /btw overwriting lastPromptRef before the accept settlement no longer suppresses debt recording Adds a non-awaiting startToolRound helper to the multi-round harness so tests can hold the boundary submission in flight. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Closeout — all 4 round-14 Criticals verified real at
Verification: useGeminiStream.test.tsx 251 passing (246 baseline + 5 new pins, incl. a |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2592 passed · 0 failed · 2592 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2592 通过 · 0 失败 · 2592 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportVerdict: 中文摘要
Previous-finding status (round at
|
| # | finding | severity | status at new head |
|---|---|---|---|
| 1 | userPromptBlocked dispatch flag is redundant defence for message safety (telemetry-only effect) |
low | stands, re-measured: M3 survives both arms at the new head (old tests 15/15, new tests 20/20; logs/matrix-M3-*.txt). Load-bearing siblings still kill (C1: 9 red old arm, 13 red new arm). Classification unchanged: redundant defence, keep as-is. |
| 2 | cancelled-boundary drain guard is redundant for delivery correctness (display hygiene + wasted round trip) | low | stands, re-measured: M11 survives both arms (15/15 old, 20/20 new; logs/matrix-M11-*.txt). Classification unchanged. |
| 3 | the geminiChat.ts push-site publication hunk is not pinned by any test in the repo |
low | stands, re-measured and widened: M12a survives the full client.test.ts (372/372) AND all 370 native tests of geminiChat.test.ts (the suite's sole red under M12a is the probe itself). The mock-free probe kills it (expected undefined to be +0, logs/probe-m12a.txt), probe green at head (2/2, logs/probe-head.txt). Delta commits added no geminiChat.test.ts coverage, so the gap persists. Recommendation unchanged: port the two VERIFY-PROBE cases into geminiChat.test.ts. Not a merge condition. |
| 4 | informational: degraded fingerprint window at accept time | — | stands, widened: capturePushedTeammateEntry (the backwards scan for the youngest entry carrying every envelope text) is now called from TWO accept sites — the boundary settlement and the new retry carrier's accept — so the documented displacement window (a concurrent byte-identical push between push and settlement degrades the fingerprint to envelope-text containment) applies to both. Code comment names the tradeoff; no exploit demonstrated. |
| 5 | informational: retry-debt consumption is one-shot (cleared on every evaluation incl. unreadable history) | — | superseded — fixed by c17 (Criticals 2+3): consumption now runs only after the admission pre-check passes, the history-unreadable path leaves the debt untouched for a later retry (asserted in reattachOrphanedRetryEnvelopes's catch), and consumed records transfer into the retry's settlement carrier (accept re-records debt for the re-pushed entry; restore re-records the originals after stripping them out of lastPromptRef). Pin: keeps an envelope protected when the retry that re-attached it is itself orphaned… kills R3 (the one-shot shape re-introduced). |
| corr. | PR body's "Full file: 234/234 pass" | — | superseded again: the file at this head holds 251 tests, 251/251 green. Description predates the later commits; not a code issue. |
Declined/deferred rows were re-measured, not diffed: M3, M11, and M12 were re-run as fresh mutations at c6aee955 (the delta touches none of those guards — it is cli retry-debt machinery only), and finding 5's mechanism was re-read in the new code rather than carried by description.
Central claim + A/B (re-run at the new head)
Central claim: teammate→leader messages queued during a multi-round tool task are delivered at the next tool-round boundary — appended to the SendMessageType.ToolResult submission after the tool-response parts — with no loss and no double delivery across cancelled/preempted boundaries, admission/delivery failures, hook blocks, and Ctrl+Y retries; settlement is decided by the push-site snapshot GeminiChat publishes; accepted-but-terminally-failed rounds survive the Ctrl+Y path via journaled retry debt; and (delta) that debt is swap-safe in flight, transfers through the retry's own re-pushed entry, survives lease-rejected retries, and is recorded even when a concurrent submission overwrote the stored payload.
Cells: base = scratch worktree at HEAD^1 (db78bdecad, removed after capture) with the PR test files copied in (core test file's userContentPushSnapshotKey import shimmed to an inert local Symbol — 0 refs in base source and base dist asserted). Base reused the root node_modules (PR leaves package.json/package-lock.json untouched — diff empty). Internal-link hygiene: the base tree's node_modules/@qwen-code/* links were rebuilt to point INTO the base tree (readlink -f node_modules/@qwen-code/qwen-code-core → tmp/base-tree/packages/core), the nested packages/{cli,core}/node_modules symlinked from head contain no @qwen-code entries, dists of PR-untouched workspace packages (acp-bridge, web-templates, channels/* — git diff HEAD^1..HEAD over them empty) were symlinked to satisfy the vitest build-prerequisite guard, base core dist was rebuilt from base source and content-asserted (0 userContentPushSnapshotKey refs in production dist; the only dist refs are the compiled shimmed test file itself), cli-side core resolution goes through the vitest source alias into the base tree (../core/index.ts).
| suite | base (no PR) | head (PR) |
|---|---|---|
cli useGeminiStream.test.tsx |
13 failed | 238 passed (251) — the 9 pre-existing #8172 reds plus 4 of the 5 new c18 pins (04-raw-red-lists-…) |
251/251 |
core client.test.ts (settlement races) |
5 failed | 367 passed (372) — same 5 race reds as prior rounds | 372/372 |
The one new pin that is trivially green on base — does not journal or record retry debt for a boundary batch accepted after a TeamManager swap — holds there because base never drains (nothing to journal, retry payload never carries the envelope); it is load-bearing at head, where it goes red under R2 and under head−c17. All base failures are expected-vs-actual assertion mismatches, zero load/collection errors. Witness: 01-ab-cli-core-base-reds-vs-head-green.png, 04-raw-red-lists-base-and-delta.png; raw logs logs/base-cli-suite.txt, logs/base-core-suite.txt, logs/head-cli-suite.txt, logs/head-core-suite.txt.
Delta validation since the previous round (commits 17–18)
Commit objects 17–18 exist locally though the shallow walk reports one reachable commit; their diffs were computed directly (d17.diff, d18.diff in the artifact). c17 touches only useGeminiStream.ts (+249/−89); c18 is test-only (+566, five pins and the non-awaiting startToolRound helper).
c17 is load-bearing — reverse-apply its production hunks (tests stay intact) and exactly the five c18 pins go red, each failing the behavioral assertion it exists for, the other 15 #8172 tests staying green:
| build | #8172 selection | red tests |
|---|---|---|
| base (no PR) | 13 failed | 238 passed | the 13 rows above |
| head − c17 | 5 failed | 15 passed | swap-restore drop (spy called 2 times, expected 1), accepted-after-swap (recordNotification called once, expected never), retry-transfer ('new question' missing the envelope part), lease-held (historyScan called once, expected never), unconditional-debt ('/btw status check' missing the envelope part) |
| head (all commits) | 0 failed | 20 passed | — |
Witness: 01-ab-… (delta row), logs/delta-minus-c17.txt.
c18's pins are load-bearing — mutation A/B across the test files: single-point mutants of the UNMODIFIED head production file, run against the old test file (001f8dbf) and the new one (c6aee955), nothing else changed. Each Rn re-introduces exactly one of the four round-14 Criticals:
| mutant | reverts | old tests (15) | new tests (20) | killer on the new arm |
|---|---|---|---|---|
| CONTROL (unmutated) | — | green | green | — |
| R1 | Critical 1 restore-side generation guard | SURVIVED | KILLED (1) | drops a boundary-drained teammate batch restored after a TeamManager swap… |
| R2 | Critical 1 accept-side swapped skip | SURVIVED | KILLED (1) | does not journal or record retry debt for a boundary batch accepted after a TeamManager swap |
| R3 | Critical 2 retry settlement carrier | SURVIVED | KILLED (1) | keeps an envelope protected when the retry that re-attached it is itself orphaned… |
| R4 | Critical 3 lease pre-check in retryLastPrompt | SURVIVED | KILLED (1) | does not discard retry debt when Ctrl+Y is pressed while the submission lease is held |
| R5 | Critical 4 unconditional debt on accept (re-gated on strip match) | SURVIVED | KILLED (1) | records retry debt even when a concurrent submission overwrote the stored payload… |
| C1 (positive control) | the central boundary drain | KILLED (9) | KILLED (13) | both arms — harness live on both |
| M3 (carried finding 1) | drop userPromptBlocked |
SURVIVED | SURVIVED | — (redundant defence) |
| M11 (carried finding 2) | drop the cancelled-boundary guard | SURVIVED | SURVIVED | — (redundant defence) |
No mutant regressed killed→survived across the test-file change. Every Rn kill is attributed to exactly one test — the corresponding c18 pin (matrix-results.json carries the per-cell failing names). Witness: 02-mutation-ab-old-vs-new-test-file.png; raw logs logs/matrix-*.txt; rerunnable as node mutation-matrix.mjs (occurrences asserted per edit, git status clean after every cell).
Admission-window claim verified (c17's Critical-3 comment): between retryLastPrompt's isSubmittingQueryRef pre-check and submitQuery's gate there is no await (reattachOrphanedRetryEnvelopes and clearRetryCountdown are synchronous); for SendMessageType.Retry the gate rejects exactly when the lease is held (Retry is never a turn continuation nor /btw), the second gate shares the same render snapshot as retryLastPrompt's own state check, Retry bypasses prepareQueryForGemini ({ queryToSend: query, shouldProceed: true } literal), there is no other early return between the gates and the client call, and waitForReservationSettlement is .catch()-wrapped at its source so it cannot reject. Consumption therefore always ends in a carrier settlement — accept (debt re-recorded for the re-pushed entry) or restore (originals re-recorded, envelopes stripped back out of lastPromptRef).
Sibling sweep on the swap guard: the Idle-drain path shares drainTeammateQueue, so its restore is covered by the same generation guard; it performs no journal/debt bookkeeping on acceptance (the Teammate submission IS the delivery), so there is no new-team leak to skip there. A swap-then-swap-back still drops (generation +2 ≠ captured), consistent with the queue having been cleared twice. No escaping shape found.
Corrections
- Carried and still true: the PR body's "234/234" describes the test file at the first commit; it now holds 251 tests, all passing. Not a code issue.
- The Reviewer Test Plan's run command (
npx vitest run packages/cli/src/ui/hooks/useGeminiStream.test.tsxfrom the repo root) WORKS despite the repo's general "don't run vitest from the root" guidance — the rootvitest.config.tscollects the file and all 251 pass (logs/root-run-plan-command.txt). No correction needed; recorded so the next verifier does not assume the command is broken.
Findings (non-blocking)
userPromptBlockeddispatch flag remains a redundant defence (low, carried, re-measured atc6aee955). M3 survives both arms (15/15 old, 20/20 new); its independent effect is telemetry only — settlement safety is held by the client-side unconditional restores. Keep as-is.- Cancelled-boundary drain guard remains redundant for delivery correctness (low, carried, re-measured). M11 survives both arms; the guard's role is display hygiene (no premature
● …) and avoiding a drain/restore round trip. Keep as-is. - The
geminiChat.tspush-site publication hunk remains unpinned by any repo test (low, carried, re-measured and widened). M12a survives the fullclient.test.ts(372/372) and all 370 native tests ofgeminiChat.test.ts; only the external mock-free probe kills it, and the probe is green at head (2/2) with the string-send validity control staying green under the mutant. Behavior verified correct by probe + base-cell reds, so this is a test to add, not code to change — port the twoVERIFY-PROBEcases (array publication value + string no-carrier) intogeminiChat.test.ts. Not a merge condition. - Informational, carried and widened: degraded fingerprint window at accept time.
capturePushedTeammateEntry's backwards scan can be displaced only by a concurrent push carrying byte-identical envelope texts; c17 added a second call site (the retry carrier's accept), so the documented window now covers both the boundary acceptance and the retry re-push acceptance. No exploit demonstrated; the window needs a byte-identical concurrent push inside a sub-event span, which the component harness cannot stage deterministically. - Informational, resolved: the one-shot retry debt (prior finding 5) is fixed by c17. See the status table row — consumption is admission-gated, unreadable history preserves the debt, and consumed records transfer through the retry's settlement carrier. R3 (the old one-shot shape re-introduced) is killed by exactly the transfer pin.
Targeted gates
packages/cliuseGeminiStream.test.tsxat head: 251 passed (251) (logs/head-cli-suite.txt); also green from the repo root per the PR's own command (logs/root-run-plan-command.txt).packages/coreclient.test.tsat head: 372 passed (372) (logs/head-core-suite.txt).npm run typecheckequivalent (tsc --noEmit) inpackages/cliandpackages/core: exit 0 both (logs/typecheck-cli.txt,logs/typecheck-core.txt).- Flakiness gate: 2 changed test files × 5 identical rounds,
PPPPPboth, verdict pass (logs/flake-gate.txt, per-round logslogs/flake-r*). Witness for the M12 probe cells and all gates:03-m12-probe-and-gates-flake.png.
Not covered
- Per-commit attribution for commits 1–16 as standalone builds: the shallow walk reports one reachable commit; commit objects 17–18 (and their trees) exist locally, which the delta controls above exploit; commits 1–16 were individually exercised in the prior two rounds and their aggregate behavior is re-measured here by the A/B, the delta revert, and the full mutation matrix at the new head. The merge commits carry no PR content.
- Live multi-agent e2e / TUI evidence (the PR states the same; the cli harness drives the real
handleCompletedTools/submitQuery/retryLastPromptpaths with a mocked model and a settlement shim mirroring the client contract, whose acceptance semantics are pinned in core'sclient.test.tsand by the base-cell reds). This round's harness replays the bug's wire shapes, not a live team's timing. - Steer's two exotic round-starting paths (duplicate-tool-response bypass,
client.tsno-tool-call continuations) for teammate messages — PR-declared v1 scope; they fall back to the Idle drain (today's behavior), so no regression. dualOutput.emitUserMessagesidecar for boundary deliveries — PR-declared, identical to steer at the same site; chat recording covered byrecordNotification(re-asserted by the R2/swap-accept cell at this head).- The accept-time scan displacement window (finding 4) beyond its named bounds — demonstrating it would require a concurrent byte-identical push inside a sub-event window, which the component harness cannot stage deterministically.
- Repo-wide lint/format gates (the PR's CI covers them); only affected-workspace tests and typecheck were run here.
Methodology
Assertion counting: every test execution with an encoded expectation counts one assertion — expected base/delta/mutant reds count as PASSED control expectations (fail counts unexpected outcomes only). Primary cells (head/base suites, delta, probe, M12 suites, root-run) and the mutation matrix are counted at test level (matrix cells run distinct mutants, so each execution is a distinct assertion context); the flakiness gate is counted at round level (10 rounds — the same assertions repeated to measure stability); typecheck counts 2. Sum: 2265 primary + 315 matrix + 10 flake rounds + 2 typecheck = 2592.
Environment: CI node:22-bookworm container, merge-ref checkout (HEAD 2a3d13d135 merge, HEAD^1 base tip db78bdecad, HEAD^2 verified head c6aee95585), npm ci + build pre-run. A/B base cell: scratch worktree at HEAD^1 (removed after capture) with root node_modules reused (lockfile untouched — git diff HEAD^1..HEAD over all package manifests empty); the base tree's node_modules/@qwen-code/* links rebuilt to resolve INTO the base tree (realpath-asserted: qwen-code-core → tmp/base-tree/packages/core), head's nested packages/{cli,core}/node_modules symlinked for external deps (verified free of @qwen-code entries), dists of PR-untouched workspace packages symlinked past the vitest build-prerequisite guard, base core dist compiled from base source (tsc --build via the repo's own build script) and content-asserted, PR test files copied in with the documented inert-symbol shim for the core file. Delta control: git apply -R of the c17 production diff (tests intact), run, restored. Test-file mutation A/B: mutation-matrix.mjs (occurrence asserted per edit, files restored via git checkout, git status clean after every cell), selections -t "#8172" against the old (001f8dbf) and new test files. Probe: two temporary VERIFY-PROBE tests patched into geminiChat.test.ts (probe-patch.mjs), green at head, publication-red under M12a, restored. Flake gate: flake-gate.sh, 5 rounds × 2 files. Raw logs in logs/; harnesses (mutation-matrix.mjs, mutants-cli.mjs, probe-patch.mjs, print-*.mjs, flake-gate.sh, d17.diff, d18.diff) and matrix-results.json in the artifact dir; captures in evidence/.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/cli/src/ui/hooks/useGeminiStream.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/useGeminiStream.test.tsx
file packages/core/src/core/client.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/client.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/ui/hooks/useGeminiStream.test.tsx: PPPPP
packages/core/src/core/client.test.ts: PPPPP
verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 1 · packages/core/src/core/client.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 2 · packages/core/src/core/client.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 3 · packages/core/src/core/client.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 4 · packages/core/src/core/client.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 5 · packages/core/src/core/client.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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:
- retry debt accumulates unboundedly in sessions that never retry (packages/cli/src/ui/hooks/useGeminiStream.ts:5710-5713) — already reported and deferred in the round-11 review body (review 5013385570, reported at line 5462; code moved by la…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
packages/core/src/core/geminiChat.ts:2928 — [review] push-snapshot publication (producer half of the settlement contract) has no testpackages/cli/src/ui/hooks/useGeminiStream.ts:4086 — [review] userPromptBlocked → onDeliveryFailed wiring is not falsifiable by any testpackages/cli/src/ui/hooks/useGeminiStream.ts:4586 — [probe] retry carrier's restore branch (strip-then-re-record) has no paired testpackages/cli/src/ui/hooks/useGeminiStream.ts:5727 — [probe] mixed steer + teammate boundary drain is unreachable by any testpackages/cli/src/ui/hooks/useGeminiStream.test.tsx:2544 — [review] createExecutingToolCall('call-r1') passes an argument to a zero-parameter factory (TS2554, masked by tsconfig exclude)packages/core/src/core/client.ts:4308 — [review] pushInitiated=false settlement arm untested; its comment misnames the triggerspackages/core/src/core/client.test.ts:13180 — [review] Goal-admission carrier test cannot exercise the R13-2 defect (Goal sends pop nothing and trip a different branch)packages/core/src/core/client.ts:2601 — [review] the snapshot's two-hop pass-by-reference chain is pinned by no testpackages/core/src/core/client.ts:2885 — [review] catch-site settlement comment names the wrong idempotence mechanism
Convergence: round 13 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/cli/src/ui/hooks/useGeminiStream.ts (findings in round 12; 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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 13 轮，非阻断）——已记录，本轮不要求修改：共 9 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 13 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/ui/hooks/useGeminiStream.ts（第 12 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
…he carrier accept The retry carrier's accept re-recorded debt but never stripped the re-attached envelopes back out of lastPromptRef, so each accept-fail-before-content-Ctrl+Y cycle re-attached the envelopes onto a base that still carried them, appending one duplicate copy per cycle. Mirror the boundary settlement and the carrier's own restore: strip the consumed envelope texts before re-recording debt. Regression test pinned and mutation-checked: reverting the strip makes the second Ctrl+Y submit [toolResponses, envelope, envelope]. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-inc.com>
…e carrier The Goal-admission catch settles the attached carrier by restore after the Retry orphan pop has run but before the only restoreStrippedRetryEntries call site (inside the settlement try/finally below), so the popped entries were permanently dropped from history while the carrier re-recorded debt against entries that no longer exist. Call restoreStrippedRetryEntries before settling in that catch, and symmetrically in the hook-failure catch (a no-op today, since hooks never fire for Retry, the only type that populates the entries). Regression test pinned and mutation-checked: without the re-add the Goal-admission rejection drops the orphaned entry from history. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-inc.com>
|
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 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 5800 passed · 0 failed · 5800 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：5800 通过 · 0 失败 · 5800 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportVerdict: 中文 — 判定：✅ 通过 · 可合入（agent 判定）
Previous-finding status (round at
|
| # | finding | severity | status at new head |
|---|---|---|---|
| 1 | userPromptBlocked dispatch flag is redundant defence for message safety (telemetry-only effect) |
low | stands, re-measured: M3 survives both arms at the new head (old tests 20/20, new tests 21/21; logs/matrix-run.txt, matrix-results.json). Load-bearing siblings still kill (C1: 13 red old arm, 14 red new arm). Classification unchanged: redundant defence, keep as-is. |
| 2 | cancelled-boundary drain guard is redundant for delivery correctness (display hygiene + wasted round trip) | low | stands, re-measured: M11 survives both arms (20/20 old, 21/21 new). Classification unchanged. |
| 3 | the geminiChat.ts push-site publication hunk is not pinned by any test in the repo |
low | stands, re-measured: M12a survives the full client.test.ts (373/373) AND all 377 native tests of geminiChat.test.ts (378 passed + the probe = 379; the suite's sole red under M12a is the probe itself). The mock-free probe kills it (logs/probe-m12a.txt), probe green at head (2/2, logs/probe-head.txt). Delta commits added no geminiChat.test.ts coverage, so the gap persists. Recommendation unchanged: port the two VERIFY-PROBE cases into geminiChat.test.ts. Not a merge condition. |
| 4 | informational: degraded fingerprint window at accept time | — | stands, unchanged: capturePushedTeammateEntry is still called from exactly TWO accept sites (boundary settlement at line ~5730 and the retry carrier's accept at line ~4598 — grep at the new head). The delta's accept-side strip did not add a capture site; the documented displacement window (a concurrent byte-identical push between push and settlement degrades the fingerprint to envelope-text containment) applies to both, as before. No exploit demonstrated. |
| 5 | informational: retry-debt consumption is one-shot | — | superseded (carried): c17's fix (admission-gated consumption, unreadable-history preservation, carrier transfer) is intact at the new head; the delta c19 strip strengthens the accept side without touching consumption. R3 (the one-shot shape re-introduced) is still killed by exactly the transfer pin at this head. |
| corr. | PR body's "Full file: 234/234 pass" | — | superseded again: at this head the suite executes 254 tests, 254/254 green. The count reconciles exactly against the previous head: 251 (at c6aee955) − 1 (main deleted the old does not copy the objective into a synthetic Goal turn) + 3 (main added a guarded-Goal-data test and two auto-compaction-notice tests) + 1 (this PR's delta pin) = 254. Description predates the later commits; not a code issue. |
Declined/deferred rows were re-measured, not diffed: M3, M11 and M12a were re-run as fresh mutations/probes at 6e3cf5be (the delta touches the retry carrier's accept closure and client.ts catches, so the closure of findings 1–3 changed and no carry-forward shortcut applied); finding 4's call-site census was re-grepped at the new head; finding 5's mechanism was re-read in the new code.
Central claim + A/B (re-run at the new head)
Central claim: teammate→leader messages queued during a multi-round tool task are delivered at the next tool-round boundary — appended to the SendMessageType.ToolResult submission after the tool-response parts — with no loss and no double delivery across cancelled/preempted boundaries, admission/delivery failures, hook blocks, and Ctrl+Y retries; settlement is decided by the push-site snapshot GeminiChat publishes; accepted-but-terminally-failed rounds survive the Ctrl+Y path via journaled retry debt; that debt is swap-safe, transfers through the retry's own re-pushed entry, survives lease-rejected retries, is recorded even when a concurrent submission overwrote the stored payload, and (delta) does not accumulate duplicate envelope copies across repeated accept→fail-before-content→Ctrl+Y cycles, while popped orphan entries are re-added even when pre-try catches settle the carrier.
Cells: base = scratch worktree at HEAD^1 (42b4c09ceb, removed after capture) with the PR test files copied in (core test file's userContentPushSnapshotKey import shimmed to an inert local Symbol — 0 refs in base source and 0 refs in the rebuilt base dist asserted). Base reused the root node_modules (PR leaves package.json/package-lock.json untouched — diff empty). Internal-link hygiene: all 23 node_modules/@qwen-code/* links were rebuilt to point INTO the base tree (readlink -f tmp/base-tree/node_modules/@qwen-code/qwen-code-core → /__w/qwen-code/qwen-code/tmp/base-tree/packages/core, asserted); the nested packages/{cli,core}/node_modules symlinked from head were verified free of @qwen-code entries before linking; dists of PR-untouched workspace packages (acp-bridge, web-templates, channels/* — git diff HEAD^1..HEAD over them empty) plus the generated git-commit.ts build-metadata file were symlinked to satisfy the vitest build-prerequisite guard; base core dist was rebuilt from base source (root tsc --build; the 63 pre-existing type errors are all @opentelemetry/node-pty/ignore/ajv resolution noise in telemetry/utils — zero in core/client.ts/core/geminiChat.ts; head's tsc --noEmit exits 0 on the same node_modules as an A/A control on the invocation) and content-asserted (0 userContentPushSnapshotKey refs anywhere in base dist). cli-side core resolution goes through the vitest source alias (path.resolve(__dirname, '../core/index.ts')), which resolves into whichever tree the config file lives in; all six subpath alias targets verified present in the base tree.
| suite | base (no PR) | head (PR) |
|---|---|---|
cli useGeminiStream.test.tsx (254) |
14 failed | 240 passed — the 9 pre-existing #8172 reds + 4 of the 5 c18 pins + the new Ctrl+Y×2 pin | 254/254 |
core client.test.ts (373) |
6 failed | 367 passed — the 5 settlement-race reds + the new Goal-admission re-add pin | 373/373 |
The one pin that is trivially green on base — does not journal or record retry debt for a boundary batch accepted after a TeamManager swap — holds there because base never drains (nothing to journal, retry payload never carries the envelope); it is load-bearing at head (killed by R2). All base failures are expected-vs-actual assertion mismatches (accept-vs-restore flips under concurrent pushes, missing catch settlement, duplicate envelope in the retry payload), zero load/collection errors. Witness: 01-ab-base-reds-vs-head-green.png; raw logs logs/base-cli-suite.txt, logs/base-core-suite.txt, logs/head-cli-suite.txt, logs/head-core-suite.txt.
Delta validation since the previous round (commits 19–20)
Both delta hunks are load-bearing — each reverted alone (tests stay intact; the hunks live in different packages, so each revert is attributable to its commit's package scope) and each turns exactly its own new pin red with the behavioral mismatch the pin exists for, every other test staying green:
| build | suite result | red test (behavioral failure) |
|---|---|---|
| base (no PR) | cli 14 failed | 240 passed; core 6 failed | 367 passed | the 14 + 6 rows above |
| head − c19 (cli strip hunk) | 1 failed | 253 passed (254) | does not accumulate duplicate envelopes … (Ctrl+Y x2) — second retry payload Array(3) vs expected Array(2): tool-response + envelope + duplicate envelope |
| head − c20 (core catch hunks) | 1 failed | 372 passed (373) | re-adds popped retry entries when Goal admission rejects a Retry after the orphan pop — addHistory called 0 times (popped entry dropped); the carrier still settles by restore (catch settlement predates the delta) |
| head (all commits) | cli 254/254; core 373/373 | — |
Witness: 01-ab-base-reds-vs-head-green.png (delta rows); raw logs logs/delta-minus-c19-cli.txt, logs/delta-minus-c20-core.txt; diffs d19-cli.diff, d20-core.diff, delta-c19-c20.diff.
One attribution control: running the PREVIOUS head's test file (c6aee955) against the merge-head production code shows 2 reds OUTSIDE #8172 (sends a hidden Goal turn without user admission side effects and a sibling) — these are main-side tests whose goal_runtime_data expectations the merge commit updated, and the PR-branch test file predates them. Re-running the same old file against head − c19 − c20 (both delta hunks reverted) yields the IDENTICAL result (2 failed | 249 passed (251) in both runs, same two test names), so they are main-side expectation drift, not a delta regression. The mutation matrix's -t "#8172" selection and all head-file cells are unaffected.
The new pins are load-bearing and non-vacuous — mutation A/B across the test files: single-point mutants of the UNMODIFIED head production code, run against the old test file (c6aee955: 20 cli #8172 / 372 core tests) and the new one (6e3cf5be: 21 / 373), nothing else changed. Controls unmutated on both arms; occurrences asserted per edit; tree asserted clean after every cell:
| mutant | reverts | old arm | new arm | killer on the new arm |
|---|---|---|---|---|
| CONTROL | — | green | green | — |
| D1 | delta c19 accept-side strip | SURVIVED | KILLED (1) | does not accumulate duplicate envelopes … (Ctrl+Y x2) |
| D2 | delta c20 Goal-admission catch restore | SURVIVED | KILLED (1) | re-adds popped retry entries when Goal admission rejects a Retry after the orphan pop |
| D3 | delta c20 hook-failure catch restore | SURVIVED | SURVIVED | — (documented no-op today: hooks never fire for Retry, the only type that populates the entries) |
| R1–R5 | c17 Criticals 1–4 (carried) | KILLED (1 each) | KILLED (1 each) | same c18 pins as the prior round |
| C1 (positive control) | the central boundary drain | KILLED (13) | KILLED (14) | both arms — harness live on both |
| M3 (carried F1) | drop userPromptBlocked clause |
SURVIVED | SURVIVED | — (redundant defence) |
| M11 (carried F2) | drop the cancelled-boundary guard | SURVIVED | SURVIVED | — (redundant defence) |
No mutant regressed killed→survived across the test-file change. Each Rn kill is attributed to exactly one c18 pin, each Dn kill to exactly its own new pin. D3's survival is the measurement behind its classification: the hook-failure catch cannot fire for Retry today (the hook block is guarded messageType !== SendMessageType.Retry), so removing its restore changes nothing — the code's own comment ("a no-op today … keeps this exit safe under future hook-scope changes") matches the evidence. Witness: 02-mutation-matrix-old-vs-new.png; raw logs logs/matrix-run.txt; rerunnable as node mutation-matrix.mjs; matrix-results.json carries all 26 cells.
Sibling sweep on the delta mechanism (no escaping shape found):
- Accept-side strip: the trailing-match guard makes it a no-op when
lastPromptRefwas overwritten between re-attach and settlement; multiple consumed records strip in the same concatenation order the re-attach appended them; envelope parts are text-only machine text, sopart.text ?? ''never mis-maps. A swap-then-swap-back still drops, consistent with the queue having been cleared twice (carried behavior). - The
restoreStrippedRetryEntriescounter gate inside the two catches could in principle be suppressed by a concurrent push advancing the per-chat counter before the catch runs — but during a Retry in flight the submission lease (isSubmittingQueryRef) blocks every othersubmitQuery-gated send in the same session (including /btw and Teammate submissions), and the counter is per-GeminiChat(per-session), so no concurrent pusher exists inside the pop→catch window. The gate design predates the delta; the catches inherit it consistently. Verified boundary, not a finding. - Ordering difference (catches restore→settle, finally settle→restore) is immaterial: cli-side settlement bookkeeping touches only cli state (
lastPromptRef, debt ref), core-side restore touches only history; the re-added entries are exactly the popped entries, so the restored debt records' original fingerprints stay valid. - Non-Retry sends with carriers reaching the Goal-admission catch are unaffected (no popped entries; carrier restores exactly as before the delta).
Corrections
- Carried and still true: the PR body's "234/234" describes the test file at the first commit; it now holds 254 tests, all passing. Not a code issue.
- Carried: the Reviewer Test Plan's run command (
npx vitest run packages/cli/src/ui/hooks/useGeminiStream.test.tsxfrom the repo root) works despite the repo's general "don't run vitest from the root" guidance. No correction needed.
Findings (non-blocking)
userPromptBlockeddispatch flag remains a redundant defence (low, carried, re-measured at6e3cf5be). M3 survives both arms (20/20 old, 21/21 new); its independent effect is telemetry only — settlement safety is held by the client-side unconditional restores. Keep as-is.- Cancelled-boundary drain guard remains redundant for delivery correctness (low, carried, re-measured). M11 survives both arms; the guard's role is display hygiene (no premature
● …) and avoiding a drain/restore round trip. Keep as-is. - The
geminiChat.tspush-site publication hunk remains unpinned by any repo test (low, carried, re-measured). M12a survives the fullclient.test.ts(373/373) and all 377 native tests ofgeminiChat.test.ts; only the external mock-free probe kills it, and the probe is green at head (2/2) with the string-send validity control staying green under the mutant. Behavior verified correct by probe + base-cell reds, so this is a test to add, not code to change — port the twoVERIFY-PROBEcases (array publication value + string no-carrier) intogeminiChat.test.ts. Not a merge condition. - Informational, carried: degraded fingerprint window at accept time.
capturePushedTeammateEntry's backwards scan can be displaced only by a concurrent push carrying byte-identical envelope texts; still exactly two accept sites at this head. No exploit demonstrated; the window needs a byte-identical concurrent push inside a sub-event span, which the component harness cannot stage deterministically. - Informational, carried as resolved: the one-shot retry debt is fixed by c17 and the delta preserves the fix (consumption stays admission-gated; the new accept-side strip only de-duplicates the stored payload).
- Informational, new classification: the hook-failure catch restore (D3) is an unobservable future-proofing defence today. Both arms survive its removal; the code comment documents the intent ("keeps this exit safe under future hook-scope changes"). Same family as findings 1–2; keep as-is.
Targeted gates
packages/cliuseGeminiStream.test.tsxat head: 254 passed (254) (logs/head-cli-suite.txt).packages/coreclient.test.tsat head: 373 passed (373) (logs/head-core-suite.txt).tsc --noEmitinpackages/cliandpackages/core: exit 0 both (logs/typecheck-cli.txt,logs/typecheck-core.txt).- Flakiness gate: 2 changed test files × 5 identical rounds,
PPPPPboth, verdict pass (logs/flake-gate.txt, per-round logslogs/flake-r*). Witness for the probe cells and gates:03-m12a-probe-and-gates.png.
Not covered
- Per-commit attribution for commits 1–18 as standalone builds: the depth-2 checkout makes only HEAD^2 reachable (
git rev-list HEAD^1..HEAD^2= 1 commit vs the 20 in the metadata snapshot). Commits 1–18 were individually exercised in prior rounds and their aggregate behavior is re-measured here by the A/B, the delta reverts, and the full mutation matrix at the new head. The merge commits carry no PR content. Commit 19's object is absent locally; its contribution was separated from commit 20 by package-disjoint revert (cli-only vs core-only, matching commit scopes) rather than by the commit object itself. - Live multi-agent e2e / TUI evidence (the PR states the same; the cli harness drives the real
handleCompletedTools/submitQuery/retryLastPromptpaths with a mocked model and a settlement shim mirroring the client contract, whose acceptance semantics are pinned in core'sclient.test.tsand by the base-cell reds). This round's harness replays the bug's wire shapes, not a live team's timing. - Steer's two exotic round-starting paths (duplicate-tool-response bypass,
client.tsno-tool-call continuations) for teammate messages — PR-declared v1 scope; they fall back to the Idle drain (today's behavior), so no regression. dualOutput.emitUserMessagesidecar for boundary deliveries — PR-declared, identical to steer at the same site.- The accept-time scan displacement window (finding 4) beyond its named bounds — demonstrating it would require a concurrent byte-identical push inside a sub-event window, which the component harness cannot stage deterministically.
- Repo-wide lint/format gates (the PR's CI covers them); only affected-workspace tests and typecheck were run here.
- Base-side
tsc --buildexits 1 on pre-existing optional-dependency type noise (telemetry/utils); the dist still emitted and was content-asserted, and head'stsc --noEmiton the same node_modules exits 0 (A/A control on the invocation), so this is not attributable to the PR.
Methodology
Assertion counting: every test execution with an encoded expectation counts one assertion — expected base/delta/mutant/probe reds count as PASSED control expectations (fail counts unexpected outcomes only). Primary cells (head/base/delta-revert suites: 254+373 each × 3 pairs; two old-file attribution controls of 251 each; probe runs 2+2+379+373) and the mutation matrix are counted at test level (26 cells × their suite sizes = 2645; each cell runs a distinct mutant, so each execution is a distinct assertion context); the flakiness gate is counted at round level (10); typecheck counts 2; four environment-hygiene checks (base-tree link realpath, dist content assert, shim presence, nested-node_modules @qwen-code-free) count 4. Sum: 2383 primary + 2645 matrix + 756 probe + 10 flake + 2 typecheck + 4 hygiene = 5800.
Environment: CI node:22-bookworm container, merge-ref checkout (HEAD f15ce79906 merge, HEAD^1 base tip 42b4c09ceb, HEAD^2 verified head 6e3cf5be1f), npm ci + build pre-run. A/B base cell: scratch worktree at HEAD^1 (removed after capture) with root node_modules reused (lockfile untouched — git diff HEAD^1..HEAD over all package manifests empty); the base tree's node_modules/@qwen-code/* links rebuilt to resolve INTO the base tree (realpath-asserted), head's nested packages/{cli,core}/node_modules symlinked for external deps (verified free of @qwen-code entries), dists of PR-untouched workspace packages and the generated git-commit.ts symlinked past the vitest build-prerequisite guard, base core dist compiled from base source and content-asserted, PR test files copied in with the documented inert-symbol shim for the core file. Delta controls: git apply -R of each package's production diff (tests intact), run, restored. Test-file mutation A/B: mutation-matrix.mjs (occurrence asserted per edit, files restored via git checkout, git status clean after every cell), selections -t "#8172" for cli against the old (c6aee955) and new test files. Probe: probe-m12a.mjs patches two temporary VERIFY-PROBE tests into geminiChat.test.ts (green at head, publication-red under M12a, restored). Flake gate: flake-gate.sh, 5 rounds × 2 files. Raw logs in logs/; harnesses (mutation-matrix.mjs, probe-m12a.mjs, print-ab.mjs, print-matrix.mjs, flake-gate.sh, d19-cli.diff, d20-core.diff, delta-c19-c20.diff) and matrix-results.json in the artifact dir; captures in evidence/.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/cli/src/ui/hooks/useGeminiStream.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/useGeminiStream.test.tsx
file packages/core/src/core/client.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/client.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/ui/hooks/useGeminiStream.test.tsx: PPPPP
packages/core/src/core/client.test.ts: PPPPP
verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 1 · packages/core/src/core/client.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 2 · packages/core/src/core/client.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 3 · packages/core/src/core/client.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 4 · packages/core/src/core/client.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/hooks/useGeminiStream.test.tsx: P (exit 0)
round 5 · packages/core/src/core/client.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — both round-13 Criticals verified fixed at this head against the code itself. ✅
|
@qwen-code-ci-bot I have not checked but one thing, is does it cover both leader sending to Agent team members as well. I.e. will the recipient team member also get messages at the tool call boundary? Because I see the issue go both ways currently |
Adopt main's Gemini->Llm rename refactor while keeping this PR's tool-round boundary delivery logic: - use-llm-stream.ts: keep drainTeammateQueue() drain/restore protocol at the Idle drain site; rename geminiClient refs to llmClient. - client.ts / client.test.ts: import userContentPushSnapshotKey from llm-chat.js; align test type refs to LlmChat/LlmEventType. - geminiChat.ts: keep main's deprecation shim; port the userContentPushSnapshotKey export and the pre-push snapshot publication into llm-chat.ts. - use-llm-stream.test.tsx: align PR-added tests with renamed identifiers (MockedLlmClientClass, ServerLlmEventType, responseSubmittedToLlm, useLlmStream). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- push-snapshot publication (producer half of the settlement contract) has no direct test — already deferred in rounds 7/8/9/13 (reviews 4999300995, 5005382385, 5008201023, 5017676649; geminiChat.ts:2626/2637/2928, now llm-chat.ts:2984)
- onAdmissionFailed boundary carrier restore unpinned by tests — already deferred in rounds 6/9/12 (review 4998826762; useGeminiStream.ts:5388/5616, now use-llm-stream.ts:5775)
- retry debt accumulates unboundedly in sessions that never retry — already deferred in rounds 11/13 (review 5013385570; useGeminiStream.ts:5462/5710-5713, now use-llm-stream.ts:5718)
- retry carrier's restore branch (strip-then-re-record) has no paired test — already deferred in round 13 (review 5017676649; useGeminiStream.ts:4586, now use-llm-stream.ts:4603)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
Test Plan (not a blocker): packages/cli/src/ui/hooks/useGeminiStream.test.tsx — no such file or directory.
Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/use-llm-stream.ts:4438 — [review] orphan-pop stop predicate hand-duplicated instead of calling core's exported isSystemReminderContent
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。
Test Plan（非阻断）：packages/cli/src/ui/hooks/useGeminiStream.test.tsx — no such file or directory。
收敛姿态下延后（第 14 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Bring in #10402 (test(ci): make release classifier stub module-safe), which fixes the classify-release-notes helper-test failure this branch's last CI run hit; no conflicts. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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:
- real-chat snapshot publication has no test pairing (llm-chat.ts:2984) — already reported and deferred in rounds 7/8/9/13 (reviews 4999300995, 5005382385, 5008201023, 5017676649)
- boundary onAdmissionFailed carrier restore untested (use-llm-stream.ts:5776) — already reported and deferred in rounds 6/9/12 (review 4998826762)
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — the packages/cli unit suite never completed (killed at its full deadline under host load average ~125); the PR's own test file passes 254/254 standalone and the five mid-run failing files were measured pre-existing against the merge base.
Test Plan (not a blocker): packages/cli/src/ui/hooks/useGeminiStream.test.tsx — no such file or directory.
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/use-llm-stream.ts:719 — [review] swap-drop rationale comment attached to the requeue branch it contradictspackages/cli/src/ui/hooks/use-llm-stream.test.tsx:3203 — [test] retry carrier restore branch (debt re-record) has zero test pairing
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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — the packages/cli unit suite never completed (killed at its full deadline under host load average ~125); the PR's own test file passes 254/254 standalone and the five mid-run failing files were measured pre-existing against the merge base。
Test Plan（非阻断）：packages/cli/src/ui/hooks/useGeminiStream.test.tsx — no such file or directory。
收敛姿态下延后（第 15 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@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: 4706 passed · 4 failed · 4710 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：4706 通过 · 4 失败 · 4710 总计 抖动门： Verification report<!-- qwen-triage:verify --> Sandboxed verification: ❌ findings — 4706 passed · 4 failed · 4710 total (agent verdict) - verified head The central mechanism is proven load-bearing (A/B + mutation matrix re-run at this head), but the delta commit 中文 — 判定：❌ 有发现（agent 判定）
Verification reportVerdict: Previous-finding status (round at
|
| # | finding | severity | status at new head |
|---|---|---|---|
| 1 | userPromptBlocked dispatch flag is redundant defence (telemetry-only effect) |
low | stands, re-measured: M3 survives at the new head (21/21 #8172 green under the mutant; C1 still kills 14). Keep as-is. |
| 2 | cancelled-boundary drain guard is redundant for delivery correctness | low | stands, re-measured: M11 survives (21/21). Keep as-is. |
| 3 | the push-site publication hunk is not pinned by any repo test | low | stands, re-measured: M12a survives the full client.test.ts (374/376 — only the two known PR-broken reds) AND all 385 native tests of llm-chat.test.ts (green at head too); the injected VERIFY-PROBE pair kills it (array probe red under M12a, string validity control green) and is green at head 2/2. Not a merge condition; recommendation unchanged (port the two probe cases into llm-chat.test.ts). |
| 4 | informational: degraded fingerprint window at accept time | — | stands: capturePushedTeammateEntry still called from exactly two accept sites (lines 4597, 5720 at this head). No exploit demonstrated. |
| 5 | informational: one-shot retry debt fixed (c17) | — | stands: admission-gated consumption intact; the delta's snapshot gate does not touch consumption; D4's revert left the transfer pin green. |
| 6 | informational: hook-failure catch restore is unobservable future-proofing | — | stands, re-measured: D3 survives client.test.ts at this head (only the two known reds). |
| corr | PR body names useGeminiStream.test.tsx and 4 tests |
— | superseded: main renamed the file to use-llm-stream.test.tsx; the #8172 describe now holds 21 tests. The 4 tests the body names all exist and behave as described (A/B + matrix). The body's run command needs the new filename. |
Declined/deferred rows were re-measured, not diffed: M3/M11/D3 re-run as fresh mutants at 1c23c6f2; M12a re-run against both suites plus the probe; F4's census re-grepped; F5's mechanism re-read in the new code. The delta touched client.ts's retry gate and the two old tests' closure, so no carry-forward shortcut applied to any row.
Central claim + A/B (re-run at the new head)
Central claim: teammate→leader messages queued during a multi-round tool task are delivered at the next tool-round boundary (appended to the SendMessageType.ToolResult submission after the tool-response parts) with no loss and no double delivery across cancelled/preempted boundaries, admission/delivery failures, hook blocks, TeamManager swaps, and Ctrl+Y retries; settlement is decided by the push-site snapshot the chat publishes on the request array; accepted-but-terminally-failed rounds survive via journaled retry debt; and (delta) the retry-entry restore decision is per-send via that snapshot.
Base cell = HEAD tree with the three production files (use-llm-stream.ts, client.ts, llm-chat.ts) reverted to their HEAD^1 contents — byte-identical to a base worktree because git diff HEAD^1..HEAD --name-only is exactly those 3 production files + 2 test files — with the PR test files kept and the core test file's userContentPushSnapshotKey import shimmed to an inert local Symbol (0 refs in base production, asserted). Restored via git checkout afterwards; tree clean.
| suite | base (no PR) | head (PR) |
|---|---|---|
cli use-llm-stream.test.tsx (254) |
14 failed | 240 passed — all 14 are #8172 delivery/debt tests (logs/base-cli-reds.txt) |
254/254 |
core client.test.ts (376) |
6 failed | 370 passed — the 6 settlement/catch pins the PR adds (logs/base-core-reds.txt); the two old suppression tests green at base |
2 failed | 374 passed — the two pre-existing tests broken by the delta (see F7) |
Witnesses: 03-ab-base-vs-head.png (table), 01-head-two-broken-tests.png (the two head reds as they print). Note: the delta pin restores stripped retry entries when only a concurrent send pushes is trivially green at base (the base setup's extra counter read consumes the mock's one-shot 0), so its load-bearing proof is the D4 flip below, not the base cell.
Delta validation (1c23c6f2) — D4 reverts only the retry gate to the old client-side counter (pushCountAfterStrip); everything else stays at head:
| build | old test: pushed-before-fail | old test: compression-shrink | delta pin: concurrent-push restore |
|---|---|---|---|
| head (snapshot gate) | RED | RED | green |
| head − D4 (old gate) | green | green | RED (addHistory calls: 0) |
Witness: 02-delta-d4-symmetric-flip.png. The delta is load-bearing and pinned by exactly its new test; the same delta is what flips the two pre-existing tests red — the PR changed the contract on the production side and updated its new tests but not these two.
Findings
F7 (blocking): the delta's protocol change leaves two pre-existing core tests red — the affected workspace's unit gate fails at head. does not re-add stripped retry entries when the chat already pushed them before failing and …when auto-compression shrank history below the pre-send length after the push (client.test.ts:10927/10979) fail deterministically at 1c23c6f2 (observed in 4 independent runs). They mock the chat's push by bumping getUserContentPushCount inside the mock turn without publishing userContentPushSnapshotKey on the request — the old contract. Under the new gate, absence of the snapshot means "provably never pushed" ⇒ unconditional restore ⇒ addHistory called once ⇒ expect(...).not.toHaveBeenCalled() fails. Reproduce: cd packages/core && npx vitest run src/core/client.test.ts -t "does not re-add stripped retry entries".
Production behavior is correct: flatMapTextParts normalizes every send (incl. string Retry payloads) to a fresh Part[] before turn.run (client.ts:3567/3769); Turn.run passes req by reference into chat.sendMessageStream (turn.ts:614); the real LlmChat.sendMessageStream publishes the snapshot on that same array synchronously immediately before history.push (llm-chat.ts:2978-2987); no LlmChat subclass or alternate sendMessageStream implementation exists. So "push landed ⇒ snapshot present ⇒ suppress" and "exited pre-push ⇒ no snapshot ⇒ restore" both hold on every real path; the two tests' mocks are the only non-conforming chat left in the repo.
Measured fix (test-side only, mirrors the PR's own new tests' miniature contract): switch both tests' mockTurnRunFn.mockReturnValue(...) to mockImplementation((_model, request) => { request[userContentPushSnapshotKey] = pushCount; return <same generator>; }), assertions unchanged. Result: 376/376 green (logs/fix-two-tests-suite.txt); reverted afterwards (this round must not modify the PR). Suggested diff:
suggested fix (packages/core/src/core/client.test.ts, both tests)
// in 'does not re-add stripped retry entries when the chat already pushed them before failing'
mockTurnRunFn.mockImplementation((_model, request) => {
(request as unknown as Record<PropertyKey, unknown>)[
userContentPushSnapshotKey
] = pushCount;
return (async function* () {
pushCount++;
yield* [] as ServerLlmStreamEvent[];
throw new Error('retry failed after push, before first event');
})();
});
// in '…when auto-compression shrank history below the pre-send length after the push'
mockTurnRunFn.mockImplementation((_model, request) => {
(request as unknown as Record<PropertyKey, unknown>)[
userContentPushSnapshotKey
] = pushCount;
return (async function* () {
historyRef.length = 0;
historyRef.push({ role: 'user', parts: [{ text: 'summary' }] });
historyRef.push(orphanedPrompt);
pushCount++;
yield* [] as ServerLlmStreamEvent[];
throw new Error('failed after compression+push, before first event');
})();
});F1–F3, F6 (low, carried, re-measured) — see status table. Classifications unchanged: M3/M11 redundant defences, D3 unobservable future-proofing, M12a coverage gap (probe kills it both directions; port the probe into llm-chat.test.ts). None is a merge condition.
F4, F5 (informational, carried) — fingerprint displacement window at accept time (two accept sites, no exploit demonstrated); one-shot retry debt remains fixed.
Targeted gates
- cli
use-llm-stream.test.tsxat head: 254/254 (5 identical rounds,05-flake-gate-cli.png). - core
client.test.tsat head: 2 failed | 374 passed (F7). - core
llm-chat.test.tsat head: 385/385; cliSession.test.ts(push-counter consumer): 722/722. - Full
packages/coreworkspace suite at head: 13 failed files / 89 failed tests — 12 files (87 tests) environmental (HOME-path assertions/home/testvs/__w/_temp/verify-agent-home, timeouts under a loaded runner, timing thresholds); those files are byte-identical at base and none imports the changed modules (import census), so they are excluded from the assertion counts and from F7; the 13th file isclient.test.ts(F7). tsc --noEmitinpackages/cliandpackages/core: exit 0 both.
Not covered
- Per-commit attribution for commits 1–23 as standalone builds (depth-2 checkout;
git rev-list HEAD^1..HEAD^2= 1 vs 24 in the metadata). Commits 1–20 were individually exercised in prior rounds; the aggregate is re-measured here (A/B + full matrix at the new head);1c23c6f2separated by the D4 revert. - Live multi-agent e2e / TUI evidence (PR states the same; the cli harness drives the real
handleCompletedTools/submitQuery/retryLastPromptpaths). - Steer's two exotic round-starting paths for teammate messages and the
dualOutput.emitUserMessagesidecar — PR-declared v1 scope. - The 12 environmentally failing core test files (not run at base; argued by byte-identity + import census + failure signatures rather than an A/A run).
- Repo-wide lint/format gates (the PR's CI covers them).
Methodology
Assertion counting: every test execution with an encoded expectation counts one assertion; expected base/delta/mutant/probe reds count as PASSED control expectations; fail counts only unexpected outcomes — the 4 fails are the two PR-broken tests observed in two independent head runs. Cells counted: head cli 254; head core ×2 runs 374+374; D4 376; base cli 254; base core 376; C1/M3/M11 21 each; D3 376; M12a core 376; M12a native 385; probe 2+2; fix cell 376; head llm-chat 385; Session 722; flake rounds 5; typecheck 2; hygiene 4 (diff file list == 5 files; base production 0 snapshot refs; tree clean after every phase; no LlmChat subclass). Sum 4706 pass + 4 fail = 4710. Witness re-runs for the two captures and the superseded broken-D4 run are not double-counted. Environment: CI node:22-bookworm container, merge-ref checkout, npm ci + build pre-run. Harnesses in the artifact dir: mutate.py (D4/C1/M3/M11/D3/M12a with occurrence assertions), probe.py (VERIFY-PROBE inject/eject into llm-chat.test.ts), fix-two-tests.py (measured fix); raw logs in logs/; captures in evidence/.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/cli/src/ui/hooks/use-llm-stream.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/use-llm-stream.test.tsx
file packages/core/src/core/client.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/client.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/ui/hooks/use-llm-stream.test.tsx: PPPPP
packages/core/src/core/client.test.ts: FFFFF
verdict: consistent-fail
summary: 1 of 2 changed test file(s) failed identically in every round — deterministic, so CI owns that signal
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/ui/hooks/use-llm-stream.test.tsx: P (exit 0)
round 1 · packages/core/src/core/client.test.ts: F (exit 1)
--- output tail · round 1 · packages/core/src/core/client.test.ts ---
��[39m Gemini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould use fast model authType for retry, not main model authType�[32m 142�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould cache per-model content generators�[32m 174�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould resolve model across authTypes when main authType misses�[32m 148�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould clear per-model generator cache on resetChat�[32m 175�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mfirst drain without snapshot seed announces all entries as new�[32m 151�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mfirst drain with snapshot seed emits nothing for seeded entries�[32m 183�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mdrain with a genuinely new skill emits a reminder�[32m 176�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mdrain with no new skills after seed emits nothing�[32m 153�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mremoved skill prunes its key so re-adding re-announces�[32m 169�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mremoved skill emits a reminder�[32m 154�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mpath-activated skill is announced by drain (no suppression based on shared activation set)�[32m 174�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mpath-activated skill re-announces after disable/re-enable�[32m 173�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mreturns early when Skill tool is not registered�[32m 152�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mreturns early and logs when collectAvailableSkillEntries throws�[32m 182�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mcommand entries use cmd: key prefix and are not suppressed by activatedConditional�[32m 186�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mcommand entry prunes and re-announces correctly�[32m 167�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mseedSkillReminderDedupFromSnapshot seeds from provided entries�[32m 185�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22mseedSkillReminderDedupFromSnapshot with empty entries resets state�[32m 187�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22minline-announced skills consumed from config are not re-announced by drain�[32m 162�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainSkillAndCommandReminders�[2m > �[22minline-announced does not suppress genuinely new skills�[32m 189�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22m#5147 shutdown gate�[2m > �[22mskips background memory tasks after shutdown is requested�[32m 181�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22m#5147 shutdown gate�[2m > �[22mis idempotent when called multiple times�[32m 163�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22mreturns early when the Agent tool is not registered�[32m 194�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22mseeds current agents on first drain without emitting a reminder�[32m 197�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22mreturns early when listing agents fails�[32m 190�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22memits no reminder when agents are unchanged�[32m 180�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22mannounces added-only agents�[32m 194�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22mannounces removed-only agents�[32m 221�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22mannounces added and removed agents�[32m 172�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mdrainAgentReminders�[2m > �[22mkeeps agent reminder state unchanged if history append fails�[32m 146�[2mms�[22m�[39m
�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 2 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[39m
�[41m�[1m FAIL �[22m�[49m src/core/client.test.ts�[2m > �[22mGemini Client (client.ts)�[2m > �[22msendMessageStream�[2m > �[22mretry sendMessageType�[2m > �[22mdoes not re-add stripped retry entries when the chat already pushed them before failing
�[31m�[1mAssertionError�[22m: expected "spy" to not be called at all, but actually been called 1 times�[90m
Received:
�[1m 1st spy call:
�[22m Array [
Object {
"parts": Array [
Object {
"text": "retry me",
},
],
"role": "user",
},
]
�[31m�[90m
Number of calls: �[1m1�[22m
�[31m�[39m
�[36m �[2m❯�[22m src/core/client.test.ts:�[2m10976:41�[22m�[39m
�[90m10974| �[39m // The push counter advanced past the post-strip snapshot, so …
�[90m10975| �[39m // restore must be suppressed — no duplicate addHistory.
�[90m10976| �[39m expect(mockChat.addHistory).not.toHaveBeenCalled();
�[90m | �[39m �[31m^�[39m
�[90m10977| �[39m });
�[90m10978| �[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/2]⎯�[22m�[39m
�[41m�[1m FAIL �[22m�[49m src/core/client.test.ts�[2m > �[22mGemini Client (client.ts)�[2m > �[22msendMessageStream�[2m > �[22mretry sendMessageType�[2m > �[22mdoes not re-add stripped retry entries when auto-compression shrank history below the pre-send length after the push
�[31m�[1mAssertionError�[22m: expected "spy" to not be called at all, but actually been called 1 times�[90m
Received:
�[1m 1st spy call:
�[22m Array [
Object {
"parts": Array [
Object {
"text": "retry me",
},
],
"role": "user",
},
]
�[31m�[90m
Number of calls: �[1m1�[22m
�[31m�[39m
�[36m �[2m❯�[22m src/core/client.test.ts:�[2m11041:41�[22m�[39m
�[90m11039| �[39m // guard would restore here — but the push counter advanced, s…
�[90m11040| �[39m // counter guard must suppress the re-add.
�[90m11041| �[39m expect(mockChat.addHistory).not.toHaveBeenCalled();
�[90m | �[39m �[31m^�[39m
�[90m11042| �[39m });
�[90m11043| �[39m
�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/2]⎯�[22m�[39m
�[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[90m (1)�[39m
�[2m Tests �[22m �[1m�[31m2 failed�[39m�[22m�[2m | �[22m�[1m�[32m374 passed�[39m�[22m�[90m (376)�[39m
�[2m Start at �[22m 08:12:09
�[2m Duration �[22m 56.51s�[2m (transform 7.73s, setup 373ms, collect 10.64s, tests 36.95s, environment 0ms, prepare 391ms)�[22m
JUNIT report written to /__w/qwen-code/qwen-code/packages/core/junit.xml
round 2 · packages/cli/src/ui/hooks/use-llm-stream.test.tsx: P (exit 0)
round 2 · packages/core/src/core/client.test.ts: F (exit 1)
--- output tail · round 2 · packages/core/src/core/client.test.ts ---
mini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould use fast model authType for retry, not main model authType�[32m 157�[2mms�[22m�[39m
�[33m�[2m✓�[22m�[39m Gemini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould cache per-model content generators �[33m 445�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould resolve model across authTypes when main authType misses�[32m 154�[2mms�[22m�[39m
�[32m✓�[39m Gemini Client (client.ts)�[2m > �[22mgenerateContent with fast model�[2m > �[22mshould clear per-model generator cache on resetChat�[32m 156�[2mms�[22m�[39
...truncated -- full content in the run artifacts.
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.
Requesting changes for one concrete reason at head 1c23c6f2: the packages/core unit suite is red on this PR's own change area — two pre-existing pins of the old global-counter gate in client.test.ts (retry sendMessageType) were not updated to the new userContentPushSnapshotKey contract, while four sibling tests were. The R15-1 fix itself is structurally right (verified from the diff); this is a test-only follow-up. Details, log excerpt, and the exact fix shape are in my Stage 2 comment above.
@yiliang114 once the two pins carry the same snapshot miniature their siblings got, a green full-matrix re-run is the remaining gate before the maintainer call.
中文说明
在头提交 1c23c6f2 上因一个具体原因 request changes：packages/core 单测套件在本 PR 自己的改动区域变红——client.test.ts（retry sendMessageType）里两个钉住旧全局计数器判定的既有测试没有同步更新到新的 userContentPushSnapshotKey 契约，而四个同类测试已经更新。R15-1 的修复本身在结构上是正确的（已从 diff 核实）；这只需要一个改测试的后续提交。细节、日志摘录和具体修复形态见上方的 Stage 2 评论。
@yiliang114 给这两个测试补上同类测试已有的快照微缩契约后，转绿的全矩阵重跑就是维护者拍板前的唯一门槛。
— Qwen Code · qwen3.8-max
The two retry-restore tests anchored the old global push-counter gate and never published userContentPushSnapshotKey from their mocked turn, so the per-send snapshot gate saw "no snapshot, never pushed" and restored unconditionally. Mirror the sibling tests' GeminiChat contract miniature: publish the counter on the request immediately before the simulated push, after the simulated compression in the shrink test. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
5 participants
What this PR does
Teammate→leader messages in Agent Team now reach the leader at the boundary between tool-call rounds instead of waiting for the leader's entire multi-round task to finish. When a tool round completes and its results are about to be submitted back to the model, any queued teammate envelopes are drained and appended to that same submission as user text after the tool-response parts — the exact mechanism and ordering already used for mid-turn "steer" input at that site. The existing drain-on-Idle path stays untouched as the fallback for turns that end without another tool round. The queue state moved to the top of
useGeminiStreamso the tool-completion dispatcher can drain it, and drained batches are restored on every failure path (cancelled/preempted boundary, admission/delivery failure), so messages are never lost or delivered twice.Why it's needed
streamingStateis onlyIdlewhen nothing is responding and no tracked tool call is scheduled/validating/executing or terminal-but-unsubmitted. In a long agentic task, back-to-back tool rounds keep that condition continuously true — theisRespondingdip between rounds is bridged by the just-scheduled/just-completed tool calls — so the teammate drain effect (which requiresIdle) never fires until the whole task concludes. A message sent early in a multi-minute task sat queued the entire time, far beyond the "delivered when your turn ends" behavior thesend_messagetool description promises. TheisSubmittingQueryRefguard from #4844 correctly prevents message loss but does nothing about the delay.The issue listed possible directions without prescribing one ("not prescribing one"). This PR picks round-boundary injection because: (a) surfacing a queued-message count in the UI would only make the wait visible, not fix delivery; (b) an interrupt/cancel-and-resume path for urgent messages is materially riskier inside the live tool loop and can be layered later; (c) rewording the tool description lowers expectations without improving behavior; while round-boundary injection reuses a pattern already proven in production for steer messages, delivers between rounds without interrupting in-flight tools, and matches the reporter's verified design analysis in the issue thread (which the triage run also confirmed).
Reviewer Test Plan
How to verify
Reproduced at component level before the fix (red tests), then verified green. New tests in
packages/cli/src/ui/hooks/useGeminiStream.test.tsx(describeteammate messages during multi-round tool tasks (#8172)):injects queued teammate messages into the next tool-round submission instead of waiting for the whole task— the core red→green repro: message queued while a round is executing, round completes through the realhandleCompletedToolspath, envelope must ride theSendMessageType.ToolResultsubmission after the tool-response parts, with the compact● …notification rendered at delivery; no double delivery when the state later reaches Idle. Failed before the fix (envelope stayed queued, round went out without it), passes after.delivers later teammate messages after an earlier round-boundary delivery— a message arriving after a boundary delivery is not swallowed; delivered via the Idle fallback when the task ends. Failed before the fix (both messages batched at whole-task end), passes after.still delivers queued teammate messages at Idle when the task ends without another tool round— regression guard for the existing fallback path (green before and after).does not lose queued teammate messages when the round boundary was cancelled— regression guard: a cancelled boundary must not carry the message away in a tool-result submission; the message survives and is delivered once the state settles (green before and after).Run:
npx vitest run packages/cli/src/ui/hooks/useGeminiStream.test.tsxfrom repo root (or insidepackages/cli). Full file: 234/234 pass. Also verified:packages/cliandpackages/corenpm run typecheckclean; ESLint + Prettier clean on both changed files.Evidence (Before & After)
No live TUI capture: reproducing this in a real session requires a multi-agent team where a teammate messages the leader mid-way through the leader's long multi-round task, which could not be staged deterministically in this environment. Verification is at the component level via the red→green tests above (they drive the real
handleCompletedTools/submitQuerypath with the mocked scheduler, exactly where the bug lives). Marking TUI evidence as not provided rather than faking it.Tested on
Environment (optional)
Unit/component tests only (
vitestunderpackages/cli), no live runtime.Risk & Scope
ToolResultpath share steer's known sidecar properties —dualOutput.emitUserMessageand the Notification/TeammaterecordNotificationbranch are skipped for tool-result submissions, so the injected text is not separately journaled to the sidecar/chat-recording the way the Idle-pathSendMessageType.Teammatesubmission is. This is identical to how steer text behaves today at the same site; the model still receives the envelope in the round's user content.handleCompletedTools), which is the reported case. The two exotic round-starting paths steer additionally covers viagetSteerInput(duplicate-tool-response bypass and the three internal no-tool-call continuations inclient.ts) are not covered for teammate messages; they fall back to the Idle drain, i.e. today's behavior, so no regression — full parity can be a follow-up if needed.TeamManager/mailbox queueing (verified correct in the issue); compaction explicitly not touched (issue author already ruled it out as an independent cause).isSubmittingQueryRefrace guard, and thestreamingStatederivation are unchanged.Linked Issues
Fixes #8172
中文说明
这个 PR 做了什么
Agent Team 中 teammate→leader 的消息，现在会在工具调用轮次之间的边界送达 leader，而不是等 leader 的整个多轮任务全部结束。当一轮工具调用完成、结果即将回传给模型时，排队的 teammate 消息信封会被排空，并作为 user 文本追加到同一次提交的 tool-response parts 之后——与该位置既有的 mid-turn steer 输入完全相同的机制和顺序。原有的 Idle 时排空路径保持不变，作为"本轮之后没有新工具轮次"场景的兜底。队列状态移到了
useGeminiStream顶部以便工具完成分发器排空；被排空的批次在所有失败路径（边界被取消/被抢占、准入失败/投递失败）都会还原来保证消息不丢、不重复投递。为什么需要
streamingState只有在没有响应中、且没有任何 tracked 工具调用处于 scheduled/validating/executing 或"已终态但未提交"状态时才为Idle。在长 agentic 任务中，一轮接一轮的工具调用让该条件持续为真——轮次之间isResponding的短暂回落会被刚调度/刚完成的工具调用 bridging 掉——所以 teammate 排空 effect（要求Idle）在整个任务结束前都不会触发。任务早期发来的消息会在队列里躺满整个任务时长（可能是几分钟），远超send_message工具描述承诺的"delivered when your turn ends"。#4844 引入的isSubmittingQueryRef守卫正确地防止了消息丢失，但解决不了延迟问题。issue 里列了几个可能方向但没有指定任何一个（"not prescribing one"）。本 PR 选择轮次边界注入，理由是：(a) 在 UI 上显示排队消息数量只能让等待可见，不能修复投递；(b) 为紧急消息做中断/取消重启路径，在运行中的工具循环里风险明显更大，可以后续再做；(c) 改工具描述文案只是降低预期，不改善行为；而轮次边界注入复用了 steer 消息已在生产中验证过的模式，在不打断在途工具的前提下实现轮次间投递，并且与报告者在 issue 线程里经过代码核实的设计分析一致（triage 运行也确认了这一点）。
Reviewer 测试计划
如何验证
修复前先做了组件级复现（红测试），修复后转绿。新测试位于
packages/cli/src/ui/hooks/useGeminiStream.test.tsx（describe 块teammate messages during multi-round tool tasks (#8172)）：injects queued teammate messages into the next tool-round submission instead of waiting for the whole task——核心红转绿复现：消息在一轮工具执行中入队，轮次通过真实的handleCompletedTools路径完成，信封必须跟随SendMessageType.ToolResult提交、位于 tool-response parts 之后，且投递时渲染紧凑的● …通知；状态后续到 Idle 时不重复投递。修复前失败（信封留在队列里，轮次提交不带它），修复后通过。delivers later teammate messages after an earlier round-boundary delivery——边界投递之后到达的消息不会被吞掉；任务结束时走 Idle 兜底投递。修复前失败（两条消息在整个任务结束时才被一起批量投递），修复后通过。still delivers queued teammate messages at Idle when the task ends without another tool round——既有兜底路径的回归守护（修复前后都绿）。does not lose queued teammate messages when the round boundary was cancelled——回归守护：被取消的边界不能把消息带进 tool-result 提交；消息存活并在状态稳定后送达（修复前后都绿）。运行方式：在仓库根目录执行
npx vitest run packages/cli/src/ui/hooks/useGeminiStream.test.tsx（或在packages/cli内运行）。整个文件 234/234 通过。另外已验证：packages/cli与packages/core的npm run typecheck干净；两个改动文件的 ESLint + Prettier 干净。前后对比证据
没有做 live TUI 截图：要在真实会话里复现，需要一个多 agent 团队在 leader 的长多轮任务中途给 leader 发消息，本环境无法确定性地搭出来。验证停留在组件级的红转绿测试（它们用 mocked scheduler 驱动真实的
handleCompletedTools/submitQuery路径，正是 bug 所在的位置）。如实标注未提供 TUI 证据，不伪造。测试环境
环境（可选）
仅单元/组件测试（
packages/cli下的vitest），无 live runtime。风险与范围
ToolResult路径注入的 teammate 信封与 steer 有相同的 sidecar 属性——dualOutput.emitUserMessage和 Notification/Teammate 的recordNotification分支对 tool-result 提交是跳过的，所以注入的文本不会像 Idle 路径的SendMessageType.Teammate提交那样单独写入 sidecar/聊天记录。这与 steer 文本在同一位置的现有行为完全一致；模型仍然能在该轮的 user 内容里收到信封。handleCompletedTools），即本 issue 报告的场景。steer 额外通过getSteerInput覆盖的两条特殊轮次启动路径（重复工具响应旁路、client.ts里三条无工具调用的内部 continuation）对 teammate 消息暂不覆盖；它们会落回 Idle 兜底，即现状行为，无回归——如需要可作为后续 follow-up 做完全对齐。TeamManager/邮箱排队侧（issue 中已核实该侧正确）；明确不碰 compaction（issue 作者已排除其为独立成因）。isSubmittingQueryRef竞态守卫、streamingState推导均未改动。关联 Issue
Fixes #8172