fix(telemetry): Address main agent tracing edge cases - #9121
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
35d862c to
49ca217
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
E2E Test ReportReused the main-agent tracing baseline established for #9107 and verified the follow-up against the locally built bundle.
|
|
Re-run on the same head ( Template ✓ — all required sections present, bilingual body complete. Problem: confirmed, not theoretical. Every fix traces to an observed problem — the maintainer review findings on #9107 (budget-abort vs. cancellation-finalizer race, swallowed aborts, mixed main/ Direction: inherited from #9107. Main-agent invocation tracing was accepted when #9107 merged with maintainer approvals; this PR repairs defects flagged in that review rather than extending scope. The bounded arena-error reporting is a sanitization improvement, not a direction change. Size: 684 production-logic lines vs 2037 test lines across 18 files (core telemetry + CLI lifecycle paths). Above the 500-line maintainer-awareness threshold — and the maintainer is now fully engaged: @wenshao ran an independent local A/B verification on real builds and approved this exact commit, so the awareness escalation has been answered rather than left pending. Approach: every hunk maps to a named fix or a review-thread finding; the re-walk found no unrelated changes or drive-by refactors. One standing hygiene note: the PR body still doesn't mention the arena reporting or status normalization the review rounds added — worth a refresh before merge, not blocking. Risk: no revert-correlated paths touched; no elevated risk signals. Moving on to code review. 🔍 中文说明同一 head（ 模板 ✓ —— 所有必需章节齐全，中英文正文完整。 问题：已确认，非理论性问题。 每个修复都对应已观测到的问题——#9107 维护者评审发现（预算 abort 与取消 finalizer 的竞争、被吞掉的 abort、main 与 方向：继承自 #9107。 主 Agent invocation tracing 已随 #9107 经维护者批准合并；本 PR 修复的是那次评审指出的缺陷而非扩大范围。有界的 arena 错误上报是 sanitization 改进，不构成方向变化。 规模：684 行生产逻辑 vs 2037 行测试，共 18 个文件（核心 telemetry + CLI 生命周期路径）。超过 500 行维护者关注阈值——而维护者现已深度介入：@wenshao 在真实构建上独立运行了本地 A/B 验证，并批准了这个 commit，关注度升级已得到回应而非悬置。 方案： 每个 hunk 都对应已列出的修复或评审线程发现；复查未发现无关改动或顺手重构。一条遗留卫生建议：PR 正文仍未提及评审轮次新增的 arena 上报与状态归一化——合并前值得更新，不构成阻断。 风险： 未触及与 revert 相关的路径；无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-walked the full production diff at this head independently (budget-stamp listener ordering, abort-flag bookkeeping around the logging wrapper's completion flag, deferred-batch queue with per-call owners and fail-closed secondary dispatch, start-type-keyed contract). Same verdict as the last pass: no correctness blockers. The pieces that matter most, verified against the code: Deferred tool-batch drain + continuation lifecycle. Batches completing while a model stream is active are pushed to Budget abort precedence. Swallowed aborts. Non-stream path: JSON Schema verdict scoping. A Error sanitization. Mid-turn error events report bounded arena categories derived from the normalized status (401/403 → auth, 429 → rate limit, 5xx → provider unavailable) instead of raw messages, and Standing non-blocking items (carried from the last pass, none blocking): the contract predicate is hand-maintained in two places ( Flow: deferred batch drain and continuation submitsequenceDiagram
participant P1 as Tool batch completes
participant P2 as handleCompletedTools
participant P3 as Pending queue
participant P4 as Last stream settles
participant P5 as Owner dispatch
participant P6 as Continuation submitQuery
P1->>P2: batch done while streams active
P2->>P3: defer batch, keep per-call owners
P4->>P3: drain merged batches (queue spliced first)
P3->>P2: re-enter dispatch
P2->>P5: primary owner continues, secondary closed as cancelled
P5->>P6: tool responses submitted under owning prompt
Files changed (18)
Testing evidenceUnattended CI run — per the gate rules I do not build or execute PR code; the evidence below is the PR's own CI on the reviewed commit, fetched via API. All checks on this head are complete.
The macOS/Windows unit legs and the CLI integration suite are skipped for this fork PR, so the green ubuntu suite alone would be partial evidence — but the behavioural claims no longer rest on it. Two independent verification lanes have since run on this exact head and both substantiate the change:
Not verified: Windows behavior (CI leg skipped, 中文说明代码审查在当前 head 上独立复查了全部生产 diff（预算戳监听器顺序、日志包装层完成标志周围的 abort 记账、按调用捕获 owner 并 fail-closed 分发 secondary 的延迟批次队列、以启动类型为键的 contract）。结论与上一轮一致：未发现正确性阻断问题。关键机制均已对照代码核实：延迟批次 drain 前先 遗留非阻断项（沿用上轮，均不阻断）：contract 谓词在 测试证据无人值守 CI 运行——按 gate 规则不构建、不执行 PR 代码；以上为该 commit 自身 CI 信号（API 获取），所有检查已完成。macOS/Windows 测试腿与 CLI 集成套件对本 fork PR 跳过，仅凭 ubuntu 全绿只是部分证据——但行为性主张已不再依赖它：两条独立验证通道已在同一 head 上运行并证实改动：
未验证：Windows 行为（CI 腿跳过、 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean review at this head and both questions the last pass deferred on are now settled by the maintainer; approving with a short list of non-blocking follow-ups. Stepping back: the reason this run existed is that the last one deferred with two explicit questions for @wenshao — confirm the earlier Criticals about mixed-owner batches were resolved, and sign off the core/telemetry scope at 684 production lines. Both now have answers on the record:
Everything else corroborates: the sandboxed Non-blocking follow-ups named for the record: the structured-output contract predicate is hand-maintained in two places (drift risk, R7-2); the Approving now, pinned to the reviewed commit — CI is settled on this head. ✅ 中文说明置信度：4/5 —— 当前 head 审查干净，上一轮 defer 的两个问题均已由维护者落定；带着一组非阻断跟进项批准。 退一步看：本轮之所以存在，是因为上一轮带着两个明确问题转交给 @wenshao——确认早前关于混合 owner 批次的 Critical 是否已解决，以及对 684 行生产规模的核心/telemetry 改动签核。两者现在都有在案的答案：
其余证据相互印证：沙箱 非阻断跟进项留档：structured-output contract 谓词在两处手工维护（漂移风险，R7-2）； 现在批准，钉在所审 commit 上——该 head 的 CI 已尘埃落定。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the 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): chunk 2: none — all checks I intended completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": nothing significant. The only checks I did not perform: I did not run the test suite / verify by runtime that the btw-merge drain is reachable in a real session…; "You are review agent reverse-audit — Reverse audit agent…": did not trace whether cancelOngoingRequest aborts a concurrent /btw stream's own controller ( allowConcurrentBtwDuringResponse leaves abortControllerRef …; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I opened were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget., and 3 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 2：none — all checks I intended completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：nothing significant. The only checks I did not perform: I did not run the test suite / verify by runtime that the btw-merge drain is reachable in a real session…；"You are review agent reverse-audit — Reverse audit agent…"：did not trace whether cancelOngoingRequest aborts a concurrent /btw stream's own controller ( allowConcurrentBtwDuringResponse leaves abortControllerRef …；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I opened were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.，另有 3 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
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): chunk 2: none — all checks I intended completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": nothing significant. The only checks I did not perform: I did not run the test suite / verify by runtime that the btw-merge drain is reachable in a real session…; "You are review agent reverse-audit — Reverse audit agent…": did not trace whether cancelOngoingRequest aborts a concurrent /btw stream's own controller ( allowConcurrentBtwDuringResponse leaves abortControllerRef …; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I opened were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget., and 3 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 2：none — all checks I intended completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：nothing significant. The only checks I did not perform: I did not run the test suite / verify by runtime that the btw-merge drain is reachable in a real session…；"You are review agent reverse-audit — Reverse audit agent…"：did not trace whether cancelOngoingRequest aborts a concurrent /btw stream's own controller ( allowConcurrentBtwDuringResponse leaves abortControllerRef …；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I opened were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.，另有 3 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review feedback batch c71b6a9
Verification: 343 CLI tests passed (1 skipped), CLI typecheck passed, focused lint passed, full build and bundle passed, and scheduler queue/finalization checks passed. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
[Critical] R2-7 Idle timeout ends the span but leaves the abort listener attached for an abandoned stream. If a consumer leaves a pending next() on a hanging stream and never calls return(), the timeout ends telemetry but generator finally never runs, so a long-lived AbortSignal retains markStreamAborted and the generator state indefinitely. Witness: the PR-code probe ended the span with removeEventListener calls = 0; removing the listener in the timeout callback changed the count to 1. Remove markStreamAborted in the idle-timeout callback as well as in finally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
[Critical] R2-7 Idle timeout ends the span but leaves the abort listener attached for an abandoned stream. If a consumer leaves a pending next() on a hanging stream and never calls return(), the timeout ends telemetry but generator finally never runs, so a long-lived AbortSignal retains markStreamAborted and the generator state indefinitely. Witness: the PR-code probe ended the span with removeEventListener calls = 0; removing the listener in the timeout callback changed the count to 1. Remove markStreamAborted in the idle-timeout callback as well as in finally.
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the latest review batch in
Verification on the exact commit: build, bundle, full workspace typecheck, 594 affected core tests, and 217 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] useGeminiStream.ts:4173 — dead-owner/ownerless secondary classification (this account's R1-1; ci-bot threads, posted twice): the non-live
ownerForToolCallpredicate is unchanged at HEAD and would drop a dead-owner or ownerless tool's successful result from a mixed batch; whether a mixed live/dead-owner batch is reachable in production (shared GeminiChat serialization + orphan-repair contracts, which this round's auditors could neither construct nor fully verify) could not be determined from the code - [Critical] useGeminiStream.ts:4173 — R2-1 (classify secondary tools by live owners): same predicate and same contested reachability as above; could not be determined
- [Critical] useGeminiStream.ts:4194 — the secondary owner's real function response is marked submitted and filtered out before
finalizeToolResponses/addHistory, leaving a dangling call that orphan repair replaces with a synthesized error; the author declines the change citing production serialization contracts that this round could not independently verify; could not be determined - [Critical] useGeminiStream.ts:4197 — R1-1 (persist the secondary owner's response before removal): same mechanism and contested reachability as the :4194 thread; could not be determined
- [Critical] useGeminiStream.ts:4206 — R2-3 (every secondary interaction ends as 'cancelled' regardless of owned-tool status, where the primary path uses continuation_tool_error): the unconditional loop stands at HEAD, but it only fires inside the mixed-owner batch whose production reachability could not be determined
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): "This PR is a follow-up to #9107 fixing review-confirmed…": none — all checks above completed within budget. The only thing I deliberately did not walk is the useGeminiStream.ts production hunks (diff lines 545–845: dr…; "This PR is a follow-up to #9107 fixing review-confirmed…": none — all checks I started were completed within budget (~20 tool calls).; "This PR is a follow-up to #9107 fixing review-confirmed…": did not trace the consumer-side UX of onAdmissionFailed / onDeliveryFailed for user-typed input rejected during the drain window (input restore semantics).; "This PR is a follow-up to #9107 fixing review-confirmed…": did not walk the notification/teammate batch-drain effect bodies at 5267/5448 (f1 references them; they sit outside my diff range).; "This PR is a follow-up to #9107 fixing review-confirmed…": none — all checks above completed within budget., and 3 more.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 5 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："This PR is a follow-up to #9107 fixing review-confirmed…"：none — all checks above completed within budget. The only thing I deliberately did not walk is the useGeminiStream.ts production hunks (diff lines 545–845: dr…；"This PR is a follow-up to #9107 fixing review-confirmed…"：none — all checks I started were completed within budget (~20 tool calls).；"This PR is a follow-up to #9107 fixing review-confirmed…"：did not trace the consumer-side UX of onAdmissionFailed / onDeliveryFailed for user-typed input rejected during the drain window (input restore semantics).；"This PR is a follow-up to #9107 fixing review-confirmed…"：did not walk the notification/teammate batch-drain effect bodies at 5267/5448 (f1 references them; they sit outside my diff range).；"This PR is a follow-up to #9107 fixing review-confirmed…"：none — all checks above completed within budget.，另有 3 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Pushed review follow-up
Validation: 222/222 hook tests, focused ESLint, CLI typecheck, repository build, and bundle all pass. Two consecutive broad diff audits found no further actionable correctness issue. Replied to and resolved 4/4 newly surfaced review threads. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] useGeminiStream.ts:4205 (thread 3781921163) — dead-owner/ownerless secondary-tool classification drops successful results from a mixed batch; the mechanism stands at HEAD, the author declines it as intentional fail-closed, and the production reachability of a mixed live/dead-owner batch (shared GeminiChat serialization + orphan-repair contracts) could not be determined from the code
- [Critical] useGeminiStream.ts:4205 (thread 3781923797) — same classification claim (posted twice); reachability likewise could not be determined
- [Critical] useGeminiStream.ts:4205 (thread 3783065821, R2-1) — classify secondary tools by live owners; same predicate and same contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4226 (thread 3782611141) — the secondary owner's real function response is marked submitted and filtered out before finalizeToolResponses/addHistory, leaving a dangling call that orphan repair replaces with a synthesized error; the author's production-serialization rationale could not be independently verified — could not be determined
- [Critical] useGeminiStream.ts:4229 (thread 3783065817, R1-1) — persist the secondary owner's response before removal; same mechanism and contested reachability as the :4226 thread — could not be determined
- [Critical] useGeminiStream.ts:4238 (thread 3783065832, R2-3) — every secondary interaction ends as 'cancelled' regardless of owned-tool status (the primary path uses continuation_tool_error); the unconditional loop stands at HEAD but only fires inside the mixed-owner batch whose production reachability could not be determined
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 4 reported findings; round 5 dry across all audited chunks; chunk 1 retired after dry rounds 3-4).
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): "This PR is a follow-up to #9107 fixing review-confirmed…": none — all checks above completed within budget.; "This PR is a follow-up to #9107 fixing review-confirmed…": none — all checks above completed; I verified statically by tracing the mocks and source rather than executing the vitest suite.; "This PR is a follow-up to #9107 fixing review-confirmed…": none — all checks above completed within budget.; "This PR is a follow-up to #9107 fixing review-confirmed…": none — all checks above completed within budget.; chunk 1: none — all checks I intended completed within budget. I did not run a full npm run typecheck , but the only production-logic type surface in my chunk is the tr…, and 12 more.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 6 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 4 reported findings; round 5 dry across all audited chunks; chunk 1 retired after dry rounds 3-4)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："This PR is a follow-up to #9107 fixing review-confirmed…"：none — all checks above completed within budget.；"This PR is a follow-up to #9107 fixing review-confirmed…"：none — all checks above completed; I verified statically by tracing the mocks and source rather than executing the vitest suite.；"This PR is a follow-up to #9107 fixing review-confirmed…"：none — all checks above completed within budget.；"This PR is a follow-up to #9107 fixing review-confirmed…"：none — all checks above completed within budget.；chunk 1：none — all checks I intended completed within budget. I did not run a full npm run typecheck , but the only production-logic type surface in my chunk is the tr…，另有 12 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Handled the latest review batch in commit
Validation: affected full unit files passed (CLI 367/367, core 379/379), focused ESLint passed, root typecheck passed, and build + bundle passed. Two post-change diff audits and independent test-engineer verification found no additional actionable correctness issue. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] comment 3781921163 (packages/cli/src/ui/hooks/useGeminiStream.ts:4213) — mixed live/dead-owner classification remains production-reachability disputed
- [Critical] comment 3782611141 (packages/cli/src/ui/hooks/useGeminiStream.ts:4234) — successful-secondary response persistence remains production-reachability disputed
- [Critical] comment 3783065832 (packages/cli/src/ui/hooks/useGeminiStream.ts:4246) — unconditional secondary cancellation telemetry remains production-reachability disputed
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 — local verification ran on Node 24.18.1 rather than the repository-required Node 22.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 3 条（原文未翻译，列表见上方英文部分）。
未审查：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 — local verification ran on Node 24.18.1 rather than the repository-required Node 22。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
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.
Unresolved, please confirm:
- [Critical] useGeminiStream.ts:4213 (thread 3781921163) — dead-owner/ownerless secondary-tool classification drops successful results from a mixed batch; mechanism unchanged at HEAD, author declines as intentional fail-closed; production reachability of a mixed live/dead-owner batch could not be determined from the code
- [Critical] useGeminiStream.ts:4213 (thread 3781923797) — same classification claim (posted twice); reachability likewise could not be determined
- [Critical] useGeminiStream.ts:4213 (thread 3783065821, R2-1) — classify secondary tools by live owners; same predicate and same contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4234 (thread 3782611141) — the secondary owner's function response is filtered before finalizeToolResponses/addHistory leaving a dangling call; the author's production-serialization rationale could not be independently verified — could not be determined
- [Critical] useGeminiStream.ts:4237 (thread 3783065817, R1-1) — persist the secondary owner's response before removal; same mechanism and contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4246 (thread 3783065832, R2-3) — every secondary interaction ends as 'cancelled' regardless of owned-tool status; the loop stands at HEAD but only fires inside the mixed-owner batch whose reachability could not be determined
- [Critical] useGeminiStream.ts:4134 (thread 3786904525, R3-13 — posted after this review's context fetch) — deferred batches carry no submission-generation identity; this run's admission-gate enumeration blocks the claimed stale-generation interleaving (only /btw is admissible in the cancel window and it preserves the cancel refs), but the filed probe contests that — could not be determined
- [Critical] useGeminiStream.ts:4213 (thread 3786904531, R3-14 — posted after this review's context fetch) — with telemetry disabled interactionOwner is absent, so concurrent main/?btw results are not partitioned; the mechanism is present but telemetry-off behavior matches pre-PR, and whether this PR must partition without telemetry is a maintainer scope call — could not be determined
- [Critical] AppContainer.tsx:2606 (thread 3786904539, R3-16 — posted after this review's context fetch) — a re-queued ?btw drains as a plain UserQuery, losing the isolated BTW routing; this run independently traced the same behavior (it matches idle-time ?btw semantics) — whether that is acceptable is a design call — could not be determined
- [Critical] client.ts:3434 (thread 3786904543, R3-17 — posted after this review's context fetch) — top-level-only .status extraction misses statusCode/response.status/SSE-text status shapes; the extraction is pre-existing in unchanged turn.ts and pre-PR arena received 'Unknown error' for all of these — whether this PR must normalize could not be determined
- [Critical] useGeminiStream.ts:4134 (thread 3786904546, R4-18 — posted after this review's context fetch) — a completed deferred ?btw batch is discarded when only the main response is cancelled; the mechanism matches this run's trace of the cancel gate, and whether discarding completed side-interaction results is intended fail-closed behavior could not be determined
- [Critical] client.ts:3445 (thread 3786904551, R4-19 — posted after this review's context fetch) — the awaited arena reportError in the Error-event path can turn a gracefully-yielded provider error into a thrown stream failure; this run's verifier determined the await predates the diff at the merge base — whether this PR must fix a pre-existing hazard could not be determined
- [Critical] useGeminiStream.ts:3881 (thread 3786904554, R5-20 — posted after this review's context fetch) — the deferred drain and scheduler-driven dispatch can both enter handleCompletedTools for separate batches while zero streams are active; not independently probed by this review and GeminiChat send-serialization may or may not neutralize it — could not be determined
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (rounds 4 and 5 both reported findings).
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 reverse-audit (round 3)": whether headless nonInteractiveCli.ts's structured_output_missing endActiveInteraction (~line 2978) can stamp a drain-item (Notification/Teammate/Cron) interact…; "agent reverse-audit (round 3)": whether headless endActiveInteraction('error', … 'structured_output_missing') at nonInteractiveCli.ts:~2978 can stamp a drain-item (Notification/Teammate/Cron….
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未决，请确认：共 13 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (rounds 4 and 5 both reported findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：whether headless nonInteractiveCli.ts's structured_output_missing endActiveInteraction (~line 2978) can stamp a drain-item (Notification/Teammate/Cron) interact…；"agent reverse-audit (round 3)"：whether headless endActiveInteraction('error', … 'structured_output_missing') at nonInteractiveCli.ts:~2978 can stamp a drain-item (Notification/Teammate/Cron…。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review follow-up pushed in 06ef3cb.
Verification: CLI useGeminiStream 224/224; core Turn 41/41; core client 343/343; changed-file Prettier and ESLint; core and CLI typecheck; full build and bundle. The mixed Race A regression was also mutation-checked: capturing the main owner before dedup makes the new test fail at the missing ?btw ToolResult continuation. Resolved-thread target: all 12 replied threads. Decisions needed: none. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] useGeminiStream.ts AppContainer queue path (thread 3786904539, R3-16) — a re-queued ?btw drains as a plain UserQuery, losing isolated BTW routing; whether that matches the intended legacy ?btw contract is a design call that could not be settled from the code
- [Critical] useGeminiStream.ts:3923 (thread 3786904554, R5-20) — the deferred drain and scheduler-driven dispatch can both enter handleCompletedTools for separate batches while zero streams are active; production reachability under GeminiChat send serialization could not be determined
- [Critical] useGeminiStream.ts:4176 (thread 3786904525, R3-13) — deferred batches carry no submission-generation identity; whether a stale-generation batch can be drained after a superseding turn could not be determined from the code
- [Critical] useGeminiStream.ts:4272 (thread 3781921163) — dead-owner/ownerless secondary classification drops successful results from a mixed batch; production reachability of such a batch could not be determined
- [Critical] useGeminiStream.ts:4272 (thread 3781923797) — same classification claim (posted twice); reachability likewise could not be determined
- [Critical] useGeminiStream.ts:4272 (thread 3783065821, R2-1) — classify secondary tools by live owners; same predicate and contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4272 (thread 3786904531, R3-14) — with telemetry disabled, concurrent main/?btw results are not partitioned; whether this PR must partition without telemetry is a maintainer scope call — could not be determined
- [Critical] useGeminiStream.ts:4293 (thread 3782611141) — the secondary owner's function response is filtered before finalizeToolResponses/addHistory leaving a dangling call; the author's production-serialization rationale could not be independently verified — could not be determined
- [Critical] useGeminiStream.ts:4296 (thread 3783065817, R1-1) — persist the secondary owner's response before removal; same mechanism and contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4308 (thread 3783065832, R2-3) — every secondary interaction ends as 'cancelled' regardless of owned-tool status; the loop stands at HEAD but only fires inside the mixed-owner batch whose reachability could not be determined
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 reverse-audit (round 2)": verifying whether the currently-running batch's allToolCallsComplete still fires after an abort-time splice rejects a queued request (coreToolScheduler's cancel…; "agent reverse-audit (round 2)": full cascade of a queue-abort reject in CoreToolScheduler (whether the in-flight batch's allToolCallsCompleteHandler still fires correctly afterward and wha…; "agent reverse-audit (round 2)": I did not trace every caller that passes options.abortController into runNonInteractive to rule out a pre-registered, span-ending abort listener beating st…; "agent reverse-audit (round 3)": none — I completed every check I started (full-chunk read, harness read, wiring verification, and the live test run) within budget..
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 10 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：verifying whether the currently-running batch's allToolCallsComplete still fires after an abort-time splice rejects a queued request (coreToolScheduler's cancel…；"agent reverse-audit (round 2)"：full cascade of a queue-abort reject in CoreToolScheduler (whether the in-flight batch's allToolCallsCompleteHandler still fires correctly afterward and wha…；"agent reverse-audit (round 2)"：I did not trace every caller that passes options.abortController into runNonInteractive to rule out a pre-registered, span-ending abort listener beating st…；"agent reverse-audit (round 3)"：none — I completed every check I started (full-chunk read, harness read, wiring verification, and the live test run) within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Implemented the actionable Critical feedback in Discovered issues and fixes:
Rejected or deferred:
Verification:
Decisions needed: none. |
|
Resolved 5/5 handled review threads for this fix batch. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] useGeminiStream.ts:4176 (thread 3786904525, R3-13) — deferred batches carry no submission-generation identity; whether a stale-generation batch can be drained after a superseding turn could not be determined from the code (author: the probe bypasses production GeminiChat send serialization and orphan repair)
- [Critical] useGeminiStream.ts:3923 (thread 3786904554, R5-20) — the deferred drain and scheduler-driven dispatch can both enter handleCompletedTools for separate batches while zero streams are active; production reachability under GeminiChat send serialization could not be determined (author declined)
- [Critical] useGeminiStream.ts:4272 (thread 3781921163) — dead-owner/ownerless secondary classification drops successful results from a mixed batch; production reachability of such a batch could not be determined (author: intentional fail-closed)
- [Critical] useGeminiStream.ts:4272 (thread 3781923797) — same classification claim (posted twice); reachability likewise could not be determined
- [Critical] useGeminiStream.ts:4272 (thread 3783065821, R2-1) — classify secondary tools by live owners; same predicate and contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4272 (thread 3786904531, R3-14) — with telemetry disabled, concurrent main/?btw results are not partitioned; whether this PR must partition without telemetry is a maintainer scope call — could not be determined
- [Critical] useGeminiStream.ts:4293 (thread 3782611141) — the secondary owner's function response is filtered before finalizeToolResponses/addHistory leaving a dangling call; the author's production-serialization rationale could not be independently verified — could not be determined
- [Critical] useGeminiStream.ts:4296 (thread 3783065817, R1-1) — persist the secondary owner's response before removal; same mechanism and contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4308 (thread 3783065832, R2-3) — every secondary interaction ends as 'cancelled' regardless of owned-tool status; the loop stands at HEAD but only fires inside the mixed-owner batch whose reachability could not be determined
- [Critical] AppContainer.tsx:2606 (thread 3786904539, R3-16) — a re-queued ?btw drains as a plain UserQuery, losing isolated BTW routing; whether that matches the intended legacy ?btw contract is a design call that could not be settled from the code
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 reported findings; all nine chunks audited every round, chunk 3 cold-checked in round 4).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 10 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 reported findings; all nine chunks audited every round, chunk 3 cold-checked in round 4)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review follow-up for c559f1d:
Validation: full useGeminiStream.test.tsx (226/226), CLI typecheck, CLI build, changed-file ESLint, root build, and bundle all passed. The new detached-steer test is load-bearing: removing the guard makes it fail deterministically. |
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.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 reported findings; all nine chunks audited every round).
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 reported findings; all nine chunks audited every round)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
@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: 3427 passed · 0 failed · 3427 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：3427 通过 · 0 失败 · 3427 总计 Verification reportPR 9121 Deep Verification — fix(telemetry): Address main agent tracing edge casesVerdict: 中文摘要结论： A/B 结论：在 base worktree（ 进程级验证：用真实 headless 进程 + 本地"永不响应"的 OpenAI 兼容假服务 + 6 秒墙钟预算，验证导出的遥测 span：head 侧 interaction span 以 变异矩阵（5/5 全部击杀）：删除 JSON Schema 契约门控 → 5 红且 userQuery 对照保持绿；关闭非流式 abort 跟踪 → 恰好 1 红（后续 abort 与流侧测试仍绿，粒度精确）；删除预算盖章 listener → 1 红且展示 Findings：无阻塞项。两条信息性说明见正文（baseRefOid 漂移与浅克隆按 commit 归因不可达；live 进程交错无法复现该竞争）。 未覆盖：仓库级套件/lint/typecheck（PR 自身 CI 覆盖）；Windows 行为（本容器为 Linux，全部绿色可作为 Linux 证据）；workflow invocation/dispatch tracing（PR 明示范围外）；浅克隆下 8 个 commit 的逐一归因（仅聚合 diff 可达）。 Scope selection
A/B load-bearing proofIdentical test files executed against head sources and against a base worktree at
Every base-side red is a behavioral assertion mismatch (spy-call / object-shape), not an import or collection error, and maps 1:1 onto a claimed fix:
Witnesses: Wire-oracle harness H1 — budget abort in a real headless processMock-free process harness (
Head assertions (6/6): exit 55, span exported, status ERROR, bounded diagnostic message present, low-cardinality Note on the base control cell: the live base arm stamped the span correctly because the real fetch rejection propagates slowly enough that Mutation matrix (vacuity) at head — 5/5 killed
Positive control: M1's first run is the caught-mutation proof quoted beside the survivors; there are no survivors — every guard the PR introduces is pinned by at least one test, and each mutation failed the intended behavioral assertion (expected-vs-actual quoted in CorrectionsNone — no inaccurate claims from earlier review rounds were encountered in the metadata snapshot, and every mechanism checked matched the PR description (including the design-doc sentence that automatic drain invocations may complete with plain text while the headless runner stays the session-level authority). FindingsNo blocking findings.
Not covered
MethodologyEnvironment: CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Maintainer local verification — real build, real spans, base vs. head A/BI built both sides from source in isolated worktrees and drove them against a scriptable OpenAI-compatible provider with real OpenTelemetry export, to see how much of this PR is observable outside unit tests. Setup — base 1. Structured-output verdict scoping — reproduced end to end ✅A headless run with
Base charges the automatic 2.
|
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定：❌ 不通过 · PR 构建失败由于 Install/build log— 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 — clean re-review at this head, both deferred questions settled by the maintainer verification and approval. Approving pinned to the reviewed commit. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- scheduler.schedule non-abort rejection handling — already reported (comment 3788410400, R6-4), deferred by the author
- Retry-branch structured-output ownership coverage — already reported (comments 3781921174 / 3783065854, R2-9), deferred by the author
- Arena error-category mapping untested branches — already reported (comment 3783650489, R2-5), deferred by the author
- turn.ts ?? originalStatus unreachable fallback — already reported (comment 3788922560, R7-3), deferred by the author
- steer-isolation test pins only the sendOptions guard, not the completion-boundary drain guard — already reported (comment 3788922570, R7-1), deferred by the author
Unresolved, please confirm:
- [Critical] R3-13 (useGeminiStream.ts:4278, thread 3786904525) — deferred batches carry no submission-generation identity; the admission-gate enumeration vs. the filed probe contest was not settled this round
- [Critical] R3-14 (useGeminiStream.ts:4374, thread 3786904531) — partitioning with telemetry disabled; a maintainer scope call, mechanism matches pre-PR behavior — could not be determined
- [Critical] R3-16 (AppContainer.tsx:2606, thread 3786904539) — re-queued ?btw drains as a plain UserQuery; author declined as the intended legacy contract; this round's probe confirmed the behavior matches the tested R3-1 design; maintainer approved an…
- [Critical] R5-20 (useGeminiStream.ts:3979, thread 3786904554) — deferred drain and scheduler-driven dispatch can both enter handleCompletedTools; production reachability under GeminiChat send serialization contested — could not be determined
- [Critical] useGeminiStream.ts:4374 (thread 3781921163) — dead-owner/ownerless secondary classification drops successful results from a mixed batch; production reachability contested — could not be determined
- [Critical] useGeminiStream.ts:4374 (thread 3781923797) — same classification claim (posted twice) — could not be determined
- [Critical] useGeminiStream.ts:4374 (thread 3783065821, R2-1) — classify secondary tools by live owners; same contested reachability — could not be determined
- [Critical] useGeminiStream.ts:4395 (thread 3782611141) — secondary owner's response filtered before finalizeToolResponses/addHistory; the author's serialization rationale could not be independently verified — could not be determined
- [Critical] useGeminiStream.ts:4398 (thread 3783065817, R1-1) — persist the secondary owner's response before removal; same contested mechanism — could not be determined
- [Critical] useGeminiStream.ts:4410 (thread 3783065832, R2-3) — unconditional secondary 'cancelled' telemetry; the loop stands but only fires inside the contested mixed batch — could not be determined
- [Critical] useGeminiStream.ts:4603 (thread 3788410385, R6-2) — parked duplicate-response batch readiness; author declined; this round's auditors judged repair covers the wire but the duplicate-provider shape was not probed — could not be determined
Not reviewed: build-and-test — packages/cli and packages/core full-suite runs exceeded the 300s harness deadline (infrastructure timeout, not a defect); the PR's 8 changed test files ran individually green (core 638/638, cli 503 passed + 1 pre-existing skip); dependent-workspace suites not run: acp-bridge, sdk-typescript, vscode-ide-companion, web-shell, webui; CI's Test (macos-latest / windows-latest, Node 22.x) matrix jobs were skipped at this commit.
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:13671 — [probe] D4-1 — the mixed main+?btw drain tests cannot regress against the R4-18 root cause (both pass at the defect commit)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 11 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — packages/cli and packages/core full-suite runs exceeded the 300s harness deadline (infrastructure timeout, not a defect); the PR's 8 changed test files ran individually green (core 638/638, cli 503 passed + 1 pre-existing skip); dependent-workspace suites not run: acp-bridge, sdk-typescript, vscode-ide-companion, web-shell, webui; CI's Test (macos-latest / windows-latest, Node 22.x) matrix jobs were skipped at this commit。
未审查：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.11)
| const continuationOwner = completedAndReadyToSubmitTools | ||
| .filter( | ||
| (toolCall) => | ||
| !historyCallIdsWithResponse.has(toolCall.request.callId), | ||
| ) | ||
| .map((toolCall) => | ||
| continuationOwnersByToolCallIdRef.current.get( | ||
| toolCall.request.callId, | ||
| ), | ||
| ) | ||
| .find((owner) => owner !== undefined); | ||
| const continuationWasCancelled = () => | ||
| continuationOwner | ||
| ? continuationOwner.signal.aborted | ||
| : turnCancelledRef.current || | ||
| abortControllerRef.current?.signal.aborted === true; |
There was a problem hiding this comment.
[Critical] R4-18: continuationOwner is selected over the WHOLE drain-merged batch in completion order, before the secondary-owner split — so both the batch's cancellation verdict (continuationWasCancelled) and the owner identity propagated into the next submitQuery (toolContinuationOwner) can be taken from an unrelated or already-aborted continuation instead of the interaction the batch actually drives. This re-demonstrates R4-18: the 06ef3cb fix only covered the history-repaired shape (the PR's own regression test mocks getHistoryFunctionResponseIds to Set(['main-tool'])); in the unrepaired shape — no send occurs between Ctrl+C and the drain, so no repair has run — the surviving result is still dropped.
— Failure scenario (both probe-verified on this commit): (1) the main tool completes while a concurrent ?btw stream is active → deferred; Ctrl+C aborts only the foreground controller; the ?btw tool completes → deferred; the drain merges [mainTool, btwTool]; continuationOwner = the main owner (first .find match) whose signal is aborted → continuationWasCancelled() is true even though secondary filtering has already narrowed the batch to the surviving ?btw tool — its successful result is marked submitted and dropped without ever being sent, and its interaction is ended cancelled. (2) With [btwTool, mainTool] order, the MAIN continuation instead runs detached under the ?btw identity (no foreground controller, processingSignal embeds the ?btw signal), and a first Ctrl+C fails to cancel it.
Witness (probes on the unmodified PR):
probe 1: sendMessageStream call count 2 (no continuation for the surviving btw tool);
endInteractionSpan [['cancelled',{main}],['cancelled',{btw}]];
recomputing the owner from the surviving tools -> call count 3, btw functionResponse sent
probe 2: {abortedOnFirstPress:false, abortedOnSecondPress:true, mainSignalAfterFirstPress:true};
preferring the owner whose promptId matches the active interaction -> abortedOnFirstPress:true
Suggested fix: compute continuationOwner after the secondary-owner split from the surviving geminiTools (falling back to the current pre-split scan), or prefer an owner whose promptId matches the driving interaction (ownerToolCall.request.prompt_id / activeInteractionPromptIdRef), so the cancellation verdict and the toolContinuationOwner metadata both describe the interaction the batch actually drives.
中文说明
[Critical] R4-18：continuationOwner 在 secondary-owner 拆分之前按完成顺序在整个 drain 合并批次上选取——因此批次的取消判定（continuationWasCancelled）与传给下一次 submitQuery 的 owner 身份（toolContinuationOwner）都可能来自一个无关的、甚至已被 abort 的 continuation，而不是批次实际驱动的 interaction。这再次验证了 R4-18：06ef3cbaf5 的修复只覆盖了 history 已修复的形状（PR 自带的回归测试把 getHistoryFunctionResponseIds mock 成 Set(['main-tool'])）；在未修复形状下——Ctrl+C 到 drain 之间没有任何 send，repair 根本不会运行——幸存结果仍会被丢弃。
— 失败场景（均已在本 commit 上用探针验证）：(1) main 工具在活跃的 ?btw stream 期间完成 → 被延迟；Ctrl+C 只 abort 前台 controller；?btw 工具完成 → 被延迟；drain 合并为 [mainTool, btwTool]；continuationOwner = main owner（.find 首个命中），其 signal 已被 abort → 即使 secondary 过滤已把批次缩小到幸存的 ?btw 工具，continuationWasCancelled() 仍为 true——其成功结果被标记为已提交并直接丢弃，该 interaction 被记为 cancelled。(2) 当顺序为 [btwTool, mainTool] 时，MAIN continuation 反而以 ?btw 身份 detached 运行（不安装前台 controller，processingSignal 内嵌 ?btw signal），第一次 Ctrl+C 无法取消它。
建议修复：在 secondary-owner 拆分之后从幸存的 geminiTools 计算 continuationOwner（回退到当前的 pre-split 扫描），或优先选择 promptId 与驱动 interaction 匹配的 owner，使取消判定与 toolContinuationOwner 元数据都描述批次实际驱动的 interaction。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const foregroundAbortController = abortControllerRef.current; | ||
| if ( | ||
| foregroundAbortController && | ||
| !foregroundAbortController.signal.aborted | ||
| ) { | ||
| foregroundAbortController.abort(); | ||
| } else { |
There was a problem hiding this comment.
[Critical] R4-1: A first Ctrl+C deliberately leaves detached ?btw streams alive (this two-stage branch), but cancelOngoingRequest's tail finally still unconditionally runs setIsResponding(false) (~line 1231, unchanged from base), collapsing streamingState to Idle while the surviving stream is still active (activeModelStreamsRef > 0). The R3-3 fix's settleSubmissionStateIfIdle guard only covers the activity-release path, not this direct write — the author's own stated R3-3 invariant ("submission state settles only when the global model-stream count is zero") is exactly what the cancel tail violates in the survivor scenario.
— Failure scenario (probe-verified on this commit): after one Ctrl+C with a concurrent ?btw stream still live: (a) a ?btw typed in this window fails allowConcurrentBtwDuringResponse (requires Responding) and AppContainer's btw fast-path, and is admitted as an ordinary FOREGROUND turn — it seizes abortControllerRef and resets turnCancelledRef, so a first press no longer reaches the original survivor; (b) with YOLO approval mode the ConversationFinishedEvent fires prematurely in the window; (c) the notification/teammate batch-drain effects (guarded only by Idle && !isSubmittingQueryRef) can auto-start new turns concurrent with the survivor.
Witness (probe):
BASE: {window_streamingState:'idle', conversation_finished_events_in_window:1,
third ?btw admitted as foreground, survivor_aborted_by_cancel2:false}
gating the cancel-tail write on activeModelStreamsRef===0 flips every observable
Note (tested by the verifier): the naive one-line guard breaks the PR's own replacement-turn regression test — a correct fix must key btw-admission, the drain guards, and the YOLO effect on activeModelStreamsRef rather than streamingState alone.
Suggested fix: key the affected consumers on the live stream count — allowConcurrentBtwDuringResponse / the AppContainer btw fast-path, the YOLO ConversationFinishedEvent effect, and the notification/teammate drain effects should require activeModelStreamsRef.current === 0 before treating the session as idle.
中文说明
[Critical] R4-1：第一次 Ctrl+C 通过这里的两阶段分支故意让 detached ?btw stream 存活，但 cancelOngoingRequest 末尾的 finally 仍无条件执行 setIsResponding(false)（约 1231 行，与 base 相同），在幸存 stream 仍然活跃（activeModelStreamsRef > 0）时把 streamingState 塌缩为 Idle。R3-3 修复的 settleSubmissionStateIfIdle guard 只覆盖 activity-release 路径，管不到这个直接写入——R3-3 自己声明的不变量（"只有全局 model-stream 计数为零时才结算 submission 状态"）正是被这行在幸存场景下违反。
— 失败场景（已在本 commit 上用探针验证）：在并发 ?btw stream 存活时按一次 Ctrl+C 后：(a) 此窗口内输入的 ?btw 不满足 allowConcurrentBtwDuringResponse（要求 Responding）与 AppContainer 的 btw 快路径，会作为普通前台 turn 被接纳——它夺取 abortControllerRef 并重置 turnCancelledRef，第一次按键再也无法触达原幸存者；(b) YOLO 模式下 ConversationFinishedEvent 在窗口内提前触发；(c) notification/teammate batch-drain effect（仅以 Idle && !isSubmittingQueryRef 为门）可在幸存者旁边自动启动新 turn。
注意（验证者已实测）：简单的一行 guard 会破坏 PR 自带的 replacement-turn 回归测试——正确修复应把 btw 准入、drain 门与 YOLO effect 改为以 activeModelStreamsRef 为准，而不是只看 streamingState。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (!allowConcurrentBtwDuringResponse && !isDetachedToolContinuation) { | ||
| abortControllerRef.current = abortController; | ||
| turnCancelledRef.current = false; | ||
| } |
There was a problem hiding this comment.
[Critical] R4-2: Surviving detached ?btw continuations run with turnCancelledRef.current === true — the reset is reserved for non-btw non-detached foreground submissions (anchored block) — and the shared output gates in handleContentEvent (~line 1590), handleThoughtEvent (~line 1862) and the image path (~line 2401) drop EVERY event while that flag is set. Net effect: the stream this PR deliberately keeps alive renders no visible output at all — tokens are consumed, tools execute, and the model conversation continues invisibly until the model stops or a second Ctrl+C kills the detached controller. No diff comment, doc, or test argues survivors are meant to render silently; pre-diff nothing survived a cancel, so these gates only ever swallowed the brief abort-propagation tail.
— Failure scenario (probe-verified on this commit): main turn streaming; the user submits ?btw concurrently; one Ctrl+C aborts only the foreground controller; the survivor's model keeps producing, its tool completes, and the continuation re-submits (passing the ~1347 guard via preserveTurnOwnership) — but every content/thought/image event hits the turnCancelledRef gate and is discarded: the remainder of the btw answer is silently dropped. A NEW ?btw admitted while the survivor keeps streamingState Responding is likewise muted.
Witness (probe on the unmodified PR):
continuation ran: a third sendMessageStream carries the btw promptId and yields
Content('PROBE_INVISIBLE_ANSWER') + Finished, yet addItem count=4, answerRendered=false;
reverting the anchored condition flips the same probe to addItem count=5, answerRendered=true
Suggested fix: make the output gates stream-aware — pass the stream's own abort signal (or the allowConcurrentBtwDuringResponse / isDetachedToolContinuation context already available in processStreamChanges) into the gates and suppress only when THAT stream's signal is aborted; or reset turnCancelledRef for detached/concurrent submissions that are admitted by design after a cancel.
中文说明
[Critical] R4-2：幸存的 detached ?btw continuation 运行时 turnCancelledRef.current === true——重置只保留给非 btw、非 detached 的前台提交（锚点代码块）——而 handleContentEvent（约 1590 行）、handleThoughtEvent（约 1862 行）与 image 路径（约 2401 行）的共享输出门在该标志为真时丢弃所有事件。结果是：这条 PR 故意保活的 stream 完全渲染不出任何输出——token 照烧、工具照跑、模型会话不可见地继续，直到模型停止或第二次 Ctrl+C 杀掉 detached controller。diff 的注释、文档或测试都没有说幸存者应该静默渲染；diff 之前没有任何东西能在 cancel 后存活，这些门过去只吞掉 abort 传播的短暂尾部。
— 失败场景（已在本 commit 上用探针验证）：main turn 流式中；用户并发提交 ?btw；一次 Ctrl+C 只 abort 前台 controller；幸存者的模型继续输出、其工具完成、continuation 重新提交（经 preserveTurnOwnership 通过 ~1347 的 guard）——但每个 content/thought/image 事件都撞上 turnCancelledRef 门被丢弃：btw 回答的剩余部分被静默丢弃。幸存者维持 Responding 期间新接纳的 ?btw 同样被静音。
建议修复：让输出门改为 stream-aware——把 stream 自己的 abort signal（或 processStreamChanges 中已有的 allowConcurrentBtwDuringResponse / isDetachedToolContinuation 上下文）传进门里，只在该 stream 的 signal 被 abort 时抑制；或者对 cancel 后按设计接纳的 detached/concurrent 提交重置 turnCancelledRef。
— qwen3.8-max via Qwen Code /review (v0.21.11)
3 participants
What this PR does
This follow-up to #9107 fixes review-confirmed edge cases in main-agent tracing. Budget-triggered aborts are classified before the core generator can close the interaction as a user cancellation; non-streaming calls and hanging streams now preserve cancellation semantics when providers swallow aborts; and deferred TUI tool batches retain their exact interaction owners so mixed main and legacy
?btwwork cannot leak tool results into the wrong continuation.It also keeps the headless JSON Schema verdict scoped correctly: user-origin invocations retain the structured-output contract across tool continuations, while automatic Cron, Notification, Teammate, and runtime Goal drain invocations are not individually mislabeled when they return plain text. Goal-finalization and headless terminal failures now retain their bounded diagnostic message instead of an unrelated generic phase label, and the Steer regression test now proves the continuation actually ran and wrote to the original owner.
Why it's needed
Without these fixes, traces could export a budget overrun as
cancelled, export a swallowed user abort asERROR, leave legacy?btwinteractions open until TTL, submit a secondary tool result under the main prompt, or report successful automatic drain work asstructured_output_missing. The remaining phase-label issues made valid failures materially harder to diagnose even though the real error was already available to the bounded telemetry finalizer.Reviewer Test Plan
How to verify
Confirm that a wall-time budget abort wins over the core cancellation finalizer, non-stream and idle-timeout provider paths remain UNSET/cancelled after a swallowed abort, and an abort that occurs only after provider completion does not rewrite success. In the TUI regression, keep a main stream active while a legacy
?btwtool batch completes, then verify the secondary interaction closes independently and only the main tool response is submitted under the main prompt. With a JSON Schema configured, verify plain-text automatic drain invocations end normally while a user-origin invocation still reportsstructured_output_missing, including after a tool continuation. Finally, force Goal finalization and headless terminal errors and confirm the owning span contains the accurate bounded message and stable low-cardinalityerror.type.Evidence (Before & After)
N/A — telemetry and lifecycle behavior only; no user-visible UI change.
Tested on
Environment (optional)
Node.js v22.22.3, npm 10.9.8, local bundle with sandbox disabled for the GenAI telemetry integration test.
Risk & Scope
Linked Issues
Follow-up to #9107.
中文说明
本 PR 做了什么
这是 #9107 的后续修复，处理评审确认的主 Agent tracing 边界问题。预算触发的 abort 会在 core generator 将 interaction 关闭为用户取消之前完成分类；非流式调用和挂起流在 provider 吞掉 abort 时会保留 cancellation 语义；TUI 延迟工具批次会保留精确 interaction owner，避免 main 与旧版
?btw工作混合后把工具结果泄漏到错误 continuation。同时修正 headless JSON Schema 判定的归属：用户来源 invocation 会跨工具 continuation 保留 structured-output contract，而自动 Cron、Notification、Teammate 与 runtime Goal drain invocation 返回纯文本时不会被单独误标。Goal finalization 和 headless 终止失败现在保留经过边界控制的真实诊断消息，不再写入无关的通用阶段标签；Steer 回归测试也会证明 continuation 确实执行并写回原 owner。
为什么需要
没有这些修复时，trace 可能把预算超限导出为
cancelled、把被 provider 吞掉的用户 abort 导出为ERROR、让旧版?btwinteraction 泄漏到 TTL、把 secondary 工具结果提交到 main prompt，或把成功的自动 drain 工作误报为structured_output_missing。其余阶段标签问题虽然真实错误已经存在于作用域中，但仍会显著降低故障诊断质量。Reviewer 测试计划
如何验证
确认 wall-time 预算 abort 优先于 core cancellation finalizer；non-stream 与 idle-timeout provider 路径在吞掉 abort 后保持 UNSET/cancelled；仅在 provider 完成后发生的 abort 不会改写成功状态。TUI 回归中保持 main stream 活跃，同时完成旧版
?btw工具批次，然后验证 secondary interaction 独立关闭，并且只有 main 工具响应以 main prompt 提交。配置 JSON Schema 后，验证纯文本自动 drain invocation 正常结束，而用户来源 invocation（包括工具 continuation 后）仍报告structured_output_missing。最后强制 Goal finalization 与 headless 终止错误，确认 owner span 包含准确的有界消息和稳定低基数error.type。证据（前后对比）
N/A——仅 telemetry 与生命周期行为变化，没有用户可见 UI 改动。
测试平台
环境（可选）
Node.js v22.22.3、npm 10.9.8；使用本地 bundle，并在关闭 sandbox 的条件下运行 GenAI telemetry 集成测试。
风险与范围
关联问题
#9107 的后续修复。