test(core): pin transport retry diagnostics and correct the replay-safety comment - #8861
Conversation
…fety comment Follow-up to the QwenLM#7938 maintainer verification, addressing both non-blocking findings. The comments justifying the thinking-phase replay claimed thought parts are never recorded in history. That is not the invariant: the successful attempt's thoughts are recorded. What makes the replay safe is that the failed attempt's accumulated partial turn is discarded wholesale before the retry (popPendingPartialAssistantTurn) and thought parts are never user-visible content. Both comment sites now state that. The two retry diagnostics were unpinned: hardcoding yieldedNonContentChunks on the scheduled log or relabeling the skipped_after_content decision on the not-taken log survived the suite. The scheduled-log field is now asserted in the thinking-only replay test, and a new test covers the path on current main that still emits skipped_after_content — a cut after a delivered functionCall, where the replay gate and the continuation gate are both closed. Each mutant now fails exactly one test.
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
Re-run: five commits landed since the last pass (review-round fixes + a merge of main), so the gate re-ran against the current head. Template looks good ✓ Problem: observed and documented — this is the direct follow-up to the maintainer verification on #7938, which explicitly named both items: an imprecise replay-safety rationale in the comments, and Direction: aligned — comment accuracy plus test pinning for the retry diagnostics that make thinking-phase replays auditable in the debug log. Exactly the kind of follow-up a verification report should produce. Size: core path touched. Production logic: 0 lines — both Approach: minimal — the diff answers the two named findings one-for-one. The fixture hoist ( Risk: Moving on to code review. 🔍 中文说明重跑：自上次审查后分支新增了五个提交（review 轮次修复 + 合并 main），因此对当前 head 重新执行了 gate。 模板完整 ✓ 问题：已观测且有据可查——这是 #7938 维护者验证报告的直接后续，报告明确点名了这两项：注释中不准确的 replay 安全性论证，以及 方向：对齐——修正注释并补测试，固定让 thinking 阶段重放在调试日志中可审计的重试诊断。正是验证报告应该产出的那种后续。 规模：触及核心路径。生产逻辑：0 行—— 方案：最小化——diff 与两个点名的 finding 一一对应。fixture 提升（ 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review of the five commits landed since the last pass — no blockers. I checked the claims against the source rather than taking the description at face value:
Testing — the PR's own CI on the reviewed commit (triage never builds or runs PR code): the
Real-scenario testing: N/A — comment-only source change, nothing user-visible, and this is an unattended run. 中文说明针对上次审查后新增的五个提交的代码审查——无阻断问题。我对照源码而非仅凭 PR 描述核实了各项声明：
测试——被审查提交上 PR 自身的 CI（triage 从不构建或运行 PR 代码）： 真实场景测试：N/A——源码改动仅注释，没有用户可见变化，且本次为无人值守运行。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, tightly-scoped follow-up; the only open items are a Suggestion-level surviving mutant on a diagnostic field and a PR-body sentence the tests outgrew. Stepping back: this is the kind of PR the gate should wave through. The motivation is a maintainer's own verification report naming two findings, and the diff answers them one-for-one with nothing else attached. My independent proposal — rewrite both comment sites around the actual invariant (error-path persistence needs a delivered functionCall, which the replay gate excludes; pop-before-retry as defense in depth) and pin each diagnostic field in the tests that already exercise its paths — matches what landed, and the final state goes one step further by pinning the ternary's key with a scenario where the attempt flags diverge. I verified the corrected comments against the code rather than taking them on faith, and both hold. The five review rounds did their job: each round's substantive suggestions (the stale test-header rationale, duplicated fixtures, the untested Verdict: approve. CI is settled on the reviewed commit, so no deferred-approval marker is needed. 中文说明置信度：4/5 —— 干净、范围紧凑的后续 PR；仅剩的开放项是一个 Suggestion 级别的诊断字段 mutant，以及 PR 描述中一句已被测试覆盖超越的话。 退一步看：这正是 gate 应该放行的那种 PR。动机是维护者自己的验证报告点名的两个 finding，diff 一一对应，没有夹带任何其他内容。我的独立方案——围绕真实不变量重写两处注释（错误路径持久化需要已交付的 functionCall，而重放门将其排除；重试前的 pop 作为纵深防御），并在已覆盖相应路径的测试中固定每个诊断字段——与最终落地的方案一致，且最终版本更进一步：用两个尝试标志取值不同的场景固定了三元键。我对照代码核实了修正后的注释而非轻信，两者都成立。五轮 review 尽到了职责：每一轮的实质性建议（测试头部过时的论证、重复的 fixture、未被测试的 false 与 exhausted 分支）都在当前 diff 中得到了可见的回应。第 5 轮遗留的一个 Suggestion（跨尝试标志互换）如果维护者想杀死该 mutant，值得开个后续 issue，但不构成本 PR 的门槛。维护者已在完全相同的提交上批准，该提交的 CI 为绿色，且没有任何用户可见变化。 结论：批准。被审查提交上的 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.
Reviewed — no blockers. Suggestions are inline. Test Plan (not a blocker): src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1090, 18137, 1463, 475, 2861, 444 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。 Test Plan（非阻断）：src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1090, 18137, 1463, 475, 2861, 444 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // The retry log must record that non-content chunks (the | ||
| // thinking) had already flowed — the diagnostic that makes | ||
| // thinking-phase replays visible in the debug log. |
There was a problem hiding this comment.
[Suggestion] The stale replay-safety rationale this PR removes from geminiChat.ts survives in one more place: the header comment of this very test (lines 7382-7386) still says "Thought parts are ephemeral (never recorded as the assistant's response in history)", directly contradicting the corrected source comment ("the successful attempt's thoughts are recorded there" — verified against the success-path history.push, which includes thoughtContentPart at ~geminiChat.ts:4902). — Concrete cost: a future maintainer tracing why thinking-phase replays are safe reads this test (the canonical entry point, now that it pins the diagnostic) and adopts the falsified invariant this PR exists to eliminate; a change reasoned from it (e.g. reworking popPendingPartialAssistantTurn or pruning thoughts from history) could silently break the real safety mechanism — wholesale discard of the failed attempt.
Update the header comment at lines 7384-7386 to the corrected rationale, e.g.:
// Replaying after thinking-only output is safe because the failed
// attempt's partial turn is popped wholesale before the retry
// (`popPendingPartialAssistantTurn`), and thought parts are never
// user-visible answer content, so the replay cannot duplicate
// anything the caller saw.中文说明
[建议] 本 PR 从 geminiChat.ts 中删除的过时重放安全性论证还有一处残留：本测试的头部注释（第 7382-7386 行）仍写着 "Thought parts are ephemeral (never recorded as the assistant's response in history)"，与修正后的源码注释直接矛盾（"the successful attempt's thoughts are recorded there"——已对照成功路径的 history.push 验证，其中包含 thoughtContentPart，约 geminiChat.ts:4902）。—— 具体代价：未来维护者追溯 thinking 阶段重放为何安全时会读到这个测试（它现在钉住了诊断字段，是天然的入口），从而接受本 PR 要消除的被证伪的不变量；基于它做出的修改（例如重构 popPendingPartialAssistantTurn 或从 history 中裁剪 thought）可能悄悄破坏真正的安全机制——整体丢弃失败 attempt 的累积内容。
建议将第 7384-7386 行的头部注释更新为修正后的论证，示例见上方英文代码块。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const transportError = Object.assign(new TypeError('terminated'), { | ||
| cause: Object.assign(new Error('other side closed'), { | ||
| code: 'UND_ERR_SOCKET', | ||
| }), | ||
| }); |
There was a problem hiding this comment.
[Suggestion] This hand-rolled socket-cut fixture is character-for-character socketCut() from the sibling describe('transport stream continuation (#7832)') block (~line 7664), and the yield-two-chunks-then-throw generator is exactly cutAfter([...]) (~line 7686). — Concrete cost: if the error shape the retry gate classifies as transport changes (the lists live in stream-transport-retry.ts / retryErrorClassification.ts), a maintainer fixing the shared helper leaves this inline copy stale — it would stop classifying as transport, silently flipping this test from exercising the skipped_after_content attribution it was written to pin to a plain error-propagation path, or failing with a misleading diagnostic.
Hoist socketCut/cutAfter out of the nested describe (or move this test into it), then build the mock as:
vi.mocked(mockContentGenerator.generateContentStream).mockResolvedValue(
cutAfter([thoughtChunkLiteral, functionCallChunkLiteral]),
);(Context: seven pre-existing inline copies of this fixture already exist in the outer describe, so hand-rolling is its established idiom — this is the newest copy, ~110 lines from the helpers.)
中文说明
[建议] 这里手写的 socket 切断错误 fixture 与相邻的 describe('transport stream continuation (#7832)') 块（约第 7664 行）中的 socketCut() 逐字符相同，「产出两个 chunk 后抛错」的生成器也恰好是 cutAfter([...])（约第 7686 行）。—— 具体代价：如果重试门控归类为 transport 的错误形状发生变化（清单位于 stream-transport-retry.ts / retryErrorClassification.ts），维护者修复共享 helper 时会让这份内联副本过时——它将不再被归类为 transport，使本测试从覆盖它所要钉住的 skipped_after_content 归因，悄悄变成覆盖普通的错误传播路径，或以误导性的诊断失败。
建议将 socketCut/cutAfter 提升出嵌套 describe（或把本测试移进去），然后按上方英文代码块构造 mock。
（背景：外层 describe 中已有 7 处相同的内联 fixture 副本，手写是该处的既有惯例——本 PR 增加的是最新的一份，且距离 helper 仅约 110 行。）
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(mockDebugLoggerWarn).toHaveBeenCalledWith( | ||
| 'Transport stream retry scheduled', | ||
| expect.objectContaining({ | ||
| retryDecision: 'retry', | ||
| yieldedNonContentChunks: true, |
There was a problem hiding this comment.
[Suggestion] This pins yieldedNonContentChunks: true, but the false side of the field is asserted nowhere in the repo — the existing zero-chunk retry fixtures (e.g. 'retries a transport stream error after yielding only tool preparation metadata', ~line 7607, whose empty-parts prep chunk fails hasCandidateOutput so streamYieldedChunk stays false) log yieldedNonContentChunks: false without asserting it. — Concrete cost: the mutant hardcoding yieldedNonContentChunks: true at geminiChat.ts:2880 ships green, reporting thinking-phase chunks on every chunkless retry in the debug log — erasing exactly the thinking-phase-replay visibility this assertion's comment says it pins. (Upstream corroboration: the maintainer's M6 mutant survived PR 7938's suite for this same reason.)
Add to the tool-preparation-metadata test, which already exercises the false path:
expect(mockDebugLoggerWarn).toHaveBeenCalledWith(
'Transport stream retry scheduled',
expect.objectContaining({
retryDecision: 'retry',
yieldedNonContentChunks: false,
}),
);中文说明
[建议] 此处钉住了 yieldedNonContentChunks: true，但该字段的 false 一侧在整个仓库中没有任何断言——已有的零 chunk 重试 fixture（例如 'retries a transport stream error after yielding only tool preparation metadata'，约第 7607 行，其空 parts 的准备 chunk 无法通过 hasCandidateOutput，因此 streamYieldedChunk 保持 false）会记录 yieldedNonContentChunks: false，却没有断言。—— 具体代价：在 geminiChat.ts:2880 把 yieldedNonContentChunks 硬编码为 true 的变异体可以在全套件绿灯下通过，使每次无 chunk 的重试都在 debug 日志中报告 thinking 阶段 chunk——恰好抹掉本断言注释声称要钉住的 thinking 阶段重放可见性。（上游佐证：维护者的 M6 变异体正是因此在 PR 7938 的套件中幸存。）
建议在已经覆盖 false 路径的 tool-preparation-metadata 测试中补充上方英文代码块中的断言。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(mockDebugLoggerWarn).toHaveBeenCalledWith( | ||
| 'Transport stream retry not taken', | ||
| expect.objectContaining({ | ||
| retryDecision: 'skipped_after_content', | ||
| }), |
There was a problem hiding this comment.
[Suggestion] This pins the 'skipped_after_content' arm of the retryDecision ternary (geminiChat.ts:2961-2962), but the sibling 'exhausted' arm is asserted nowhere in the repo — the budget-exhaustion test (~line 7244) reaches 'Transport stream retry not taken' with streamYieldedContentChunk === false and never asserts the value it gets. — Concrete cost: the mutant collapsing the ternary to always 'skipped_after_content' ships green, misattributing a budget-exhausted "gave up" as "unsafe to recover" in the debug log — silently deleting the very distinction this test's own comment names ("separates 'unsafe to recover' from 'gave up'"). (Upstream corroboration: the M7 mutant survived PR 7938's suite for this same reason.)
Add to the budget-exhaustion test:
expect(mockDebugLoggerWarn).toHaveBeenCalledWith(
'Transport stream retry not taken',
expect.objectContaining({ retryDecision: 'exhausted' }),
);中文说明
[建议] 此处钉住了 retryDecision 三元表达式（geminiChat.ts:2961-2962）的 'skipped_after_content' 分支，但兄弟分支 'exhausted' 在整个仓库中没有任何断言——预算耗尽测试（约第 7244 行）会在 streamYieldedContentChunk === false 时到达 'Transport stream retry not taken'，却从未断言它得到的值。—— 具体代价：把三元表达式坍缩为恒返回 'skipped_after_content' 的变异体可以在全套件绿灯下通过，使预算耗尽的「放弃重试」在 debug 日志中被误报为「不安全而无法恢复」——悄悄删除本测试注释自己点名的区分（"separates 'unsafe to recover' from 'gave up'"）。（上游佐证：M7 变异体正是因此在 PR 7938 的套件中幸存。）
建议在预算耗尽测试中补充上方英文代码块中的断言。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 1/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31379619548 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 2/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31383566918 🧠 Handled by Qwen Code · model/模型 |
Address the inline review on the follow-up: - Correct the last surviving copy of the stale replay-safety rationale (the thinking-phase test's header comment still claimed thoughts never enter history). - Hoist socketCut/cutAfter out of the continuation suite and reuse them in the function-call cut test instead of a character-for-character inline copy, so the transport error shape has a single producer. - Pin the false side of yieldedNonContentChunks in the tool-preparation retry test, and the 'exhausted' arm of retryDecision in the budget exhaustion test. Hardcoding the diagnostic true or collapsing the ternary now each fail exactly one test.
|
All four inline suggestions addressed in 0f95b42 — each one was a real gap:
301/301 green, lint/prettier clean. The point about both sides of a diagnostic needing a pin is well taken — it mirrors exactly how the original M6/M7 mutants survived #7938's suite, one level up. 中文四条 inline 建议已全部在 0f95b42 落地——每条都是真实缺口：
301/301 全绿，lint/prettier 干净。「诊断字段两侧都要钉住」这点很受用——这正是当初 M6/M7 变异体在 #7938 套件中幸存的同款成因，高了一层。 |
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. 2 of those were silent-sandbox (idle) timeouts that no budget increase can cure — investigate the sandbox image and runner docker daemon for those. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31389561905 🧠 Handled by Qwen Code · model/模型 |
CI noteThe
中文说明
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): This PR is a follow-up to PR #7938 (issue #7832): it corr...: full-package npm run typecheck /lint was not run (vitest transform of the changed files passed cleanly; changes are comments + test expectations + const fixtur…; This PR is a follow-up to PR #7938 (issue #7832): it corr...: did not fetch the PR #8861 round-1 conversation (gh/PR thread) to confirm whether the "never user-visible content" phrasing was already discussed — relevant onl…. Test Plan (not a blocker): src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 19230, 1090, 18140, 1463, 475, 2861, 444 passed.
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：This PR is a follow-up to PR #7938 (issue #7832): it corr...：full-package npm run typecheck /lint was not run (vitest transform of the changed files passed cleanly; changes are comments + test expectations + const fixtur…；This PR is a follow-up to PR #7938 (issue #7832): it corr...：did not fetch the PR #8861 round-1 conversation (gh/PR thread) to confirm whether the "never user-visible content" phrasing was already discussed — relevant onl…。 Test Plan（非阻断）：src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 19230, 1090, 18140, 1463, 475, 2861, 444 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // Shared transport-cut fixtures — used by the function-call cut test | ||
| // below and by the 'transport stream continuation' suite. A single | ||
| // producer for the error shape the transport retry gate classifies, | ||
| // so a change to the retryable-code handling has one place to update. |
There was a problem hiding this comment.
[Suggestion] The hoisted fixture's comment claims "a single producer … one place to update", but seven sibling tests in this same describe still inline-construct the byte-identical error shape (TypeError('terminated') + cause { code: 'UND_ERR_SOCKET' }) at lines ~7189, 7247, 7320, 7400, 7488, 7634, 8792 — three of them edited by this very PR. — Failure scenario: if the classified transport error shape changes (the lists live in stream-transport-retry.ts / retryErrorClassification.ts), a maintainer following this comment updates socketCut() only; the seven inline copies keep exercising the old shape — the tests pass while no longer verifying the production classification. Suggested fix: route the seven inline transportError constructions through socketCut() (and move the declaration above its first use — six of the seven call sites textually precede it), or reword the comment to the true scope: "so those consumers have one place to update".
中文说明
[建议] 提升后的 fixture 注释声称「单一产地……只需更新一处」，但同一 describe 中另有七个兄弟测试仍在逐字符地内联构造相同的错误形状（TypeError('terminated') + cause { code: 'UND_ERR_SOCKET' }），位于约第 7189、7247、7320、7400、7488、7634、8792 行——其中三个正是本 PR 修改的测试。—— 失败场景：如果重试门控归类的传输错误形状发生变化（清单位于 stream-transport-retry.ts / retryErrorClassification.ts），维护者依照此注释只更新 socketCut()；七份内联副本会继续演练旧形状——测试仍然通过，却不再验证生产环境的归类逻辑。建议修复：将七处内联的 transportError 构造改为调用 socketCut()（并把声明移到首个调用点之前——七个调用点中有六个在文本上位于声明之前），或将注释改写为真实范围："so those consumers have one place to update"。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| expect(mockDebugLoggerWarn).toHaveBeenCalledWith( | ||
| 'Transport stream retry not taken', | ||
| expect.objectContaining({ | ||
| retryDecision: 'exhausted', |
There was a problem hiding this comment.
[Suggestion] This pin leaves a one-line mutant alive: substituting streamYieldedChunk for streamYieldedContentChunk in the retryDecision ternary (geminiChat.ts:~2961) survives the whole suite, because the two not-taken tests sit exactly on the sides where both flags agree (zero chunks ⇒ both false; thought + functionCall ⇒ both true). — Failure scenario: a thinking-only stream that exhausts the replay budget (three consecutive socket cuts mid-thinking — the dominant #7832 shape) reaches the not-taken branch with the flags diverged; if the ternary's flag ever regresses, the log reports retryDecision: 'skipped_after_content' for a give-up — the exact misattribution the comment above warns against — and the suite stays green. Probe-verified at HEAD: the mutant passes 301/301; the test sketched below fails under the mutant and passes on the correct code. (Distinct from the round-1 'exhausted' pin, which killed only the constant-collapse mutant.) Suggested fix — add a budget-exhaustion variant that yields a thinking chunk before each cut:
it('attributes budget exhaustion correctly when thinking chunks flowed', async () => {
vi.useFakeTimers();
try {
vi.mocked(mockContentGenerator.generateContentStream).mockResolvedValue(
cutAfter([
{
candidates: [
{ content: { parts: [{ text: 'hmm', thought: true }] } },
],
},
] as unknown as GenerateContentResponse[]),
);
// drain the stream (it rejects after the third cut), advancing fake
// timers past both retry delays: 3 generateContentStream calls,
// 2 RETRY events, then the not-taken log —
expect(mockDebugLoggerWarn).toHaveBeenCalledWith(
'Transport stream retry not taken',
expect.objectContaining({ retryDecision: 'exhausted' }),
);
} finally {
vi.useRealTimers();
}
});中文说明
[建议] 此断言仍留有一个单行变异体存活：把 retryDecision 三元表达式（geminiChat.ts:~2961）中的 streamYieldedContentChunk 替换为 streamYieldedChunk 可以通过整个套件，因为两个 not-taken 测试恰好都落在两个标志取值一致的侧（零 chunk ⇒ 两者皆 false；thought + functionCall ⇒ 两者皆 true）。—— 失败场景：仅产出 thinking 的流耗尽重试预算（thinking 阶段连续三次 socket 切断——#7832 的主要形态）时会以两标志分歧的状态到达 not-taken 分支；若三元表达式引用的标志将来发生回归，日志会把「放弃重试」误报为 retryDecision: 'skipped_after_content'——正是上方注释警告的误归因——而全套件保持绿灯。已在 HEAD 上用探针验证：变异体 301/301 通过；下方草图测试在变异体下失败、在正确代码下通过。（与第一轮钉住的 'exhausted' 不同——那一断言只杀死了常量坍缩变异体。）建议修复——新增一个在每次切断前先产出 thinking chunk 的预算耗尽变体测试（代码见上方英文代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
|
@qwen-code /takeover |
|
🔄 Takeover re-armed: the round counter starts a fresh window (previous rounds no longer count toward the cap); management continues. 中文说明🔄 已重新武装：轮次计数开启新窗口（此前轮次不再计入上限），托管继续。 |
|
This review round raced the push: its four inline suggestions were generated against the previous head
中文本轮 review 与推送发生了竞速：四条 inline 建议是基于旧 head |
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 1/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31399630023 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): PR #8861 (QwenLM/qwen-code), a follow-up to #7938 (issue ...: none — all planned checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — the walk completed within budget..
Test Plan (not a blocker): src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1200, 18901, 1502, 481, 3069, 473 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：PR #8861 (QwenLM/qwen-code), a follow-up to #7938 (issue ...：none — all planned checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — the walk completed within budget.。
Test Plan（非阻断）：src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1200, 18901, 1502, 481, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| * retry (`popPendingPartialAssistantTurn`), and thought parts are never | ||
| * user-visible content — so nothing the caller saw from the failed | ||
| * attempt can appear twice. The transport stream retry gate relies on |
There was a problem hiding this comment.
[Suggestion] The corrected rationale introduces a new empirically false clause: "thought parts are never user-visible content". Thought parts from a failed attempt DO reach user-visible surfaces before the retry gate runs: chunks are yielded to the caller first (geminiChat.ts ~2819); turn.ts converts them to GeminiEventType.Thought events (turn.ts:605-612); the TUI renders them as a live thinking indicator and commits them as a collapsible gemini_thought history item (useGeminiStream.ts ~742-744, ~1853-1866); non-interactive JSON output appends them (BaseJsonOutputAdapter.appendThinking); session resume re-renders persisted thoughts (resumeHistoryUtils.ts:465). The maintainer's own #7938 verification table lists attempt-1 thoughts under "streamed to caller", and this file's own usage of the phrase includes thoughts (the ~3829-3835 fallback skip keys on streamYieldedAnyChunk, which thought-only chunks set). — Failure scenario: this PR exists to remove a false invariant "so the next reader doesn't rely on it"; the replacement premise plants a new one. A maintainer generalizing the replay gate could admit other chunk classes judged "invisible", or notice the contradiction (a user reporting "I saw the interrupted attempt's thinking, then a second thinking block after the retry") and distrust the whole rationale — re-creating the exact failure mode this PR removes. The safety conclusion holds independently (wholesale discard via popPendingPartialAssistantTurn), so this is not a blocker. The same clause echoes in the gate comment at ~2933-2937 and in the thinking-test header (geminiChat.test.ts ~7624-7628).
| * retry (`popPendingPartialAssistantTurn`), and thought parts are never | |
| * user-visible content — so nothing the caller saw from the failed | |
| * attempt can appear twice. The transport stream retry gate relies on | |
| * retry (`popPendingPartialAssistantTurn`), so nothing the caller saw | |
| * from the failed attempt can appear twice. The transport stream retry | |
| * gate relies on |
中文说明
修正后的论证引入了一个新的、与事实不符的子句：「thought part 永远不是用户可见内容」。失败 attempt 的 thought part 在重试门控运行之前确实会到达用户可见的界面：chunk 会先 yield 给调用方（geminiChat.ts ~2819）；turn.ts 将其转换为 GeminiEventType.Thought 事件（turn.ts:605-612）；TUI 将其渲染为实时思考指示器，并作为可折叠的 gemini_thought 历史项提交（useGeminiStream.ts ~742-744、~1853-1866）；非交互 JSON 输出通过 BaseJsonOutputAdapter.appendThinking 追加；会话恢复时会重新渲染持久化的 thought（resumeHistoryUtils.ts:465）。维护者在 #7938 的验证表格本身就把 attempt 1 的 thought 列在 "streamed to caller" 之下；本文件对该短语的用法也包含 thought（~3829-3835 处跳过回退链的依据是 streamYieldedAnyChunk，纯 thought chunk 同样会置位该标志）。失败场景：本 PR 的目的正是移除错误不变量以免「后续读者依赖它」，而替换后的前提又植入了一个新的错误不变量。后续维护者在泛化重放门控时可能据此放行其他被判定为「不可见」的 chunk 类别，或者发现矛盾（例如用户报告「看到了被打断 attempt 的思考，重试后又出现第二个思考块」）后不再信任整段论证——重新制造出本 PR 要消除的失败模式。安全结论本身独立成立（popPendingPartialAssistantTurn 整体丢弃），因此这不是阻断项。同一子句在 ~2933-2937 的门控注释与思考测试头部注释（geminiChat.test.ts ~7624-7628）中重复出现。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(mockDebugLoggerWarn).toHaveBeenCalledWith( | ||
| 'Transport stream retry not taken', | ||
| expect.objectContaining({ | ||
| retryDecision: 'exhausted', | ||
| }), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R2-2: This pin leaves a one-line mutant alive: substituting streamYieldedChunk for streamYieldedContentChunk in the retryDecision ternary (geminiChat.ts:3052) survives the whole suite — measured under the mutant: 318/318 pass. The only two not-taken assertions set both flags equal (this test: false/false; the function-call cut test ~7855: true/true), so neither discriminates the ternary's condition variable. — Failure scenario: a probe test that exhausts the retry budget while each attempt yields one thought-only chunk before the cut flips the mutant: it passes on unmutated code and fails under the mutant with retryDecision: 'skipped_after_content' where 'exhausted' is expected — the exact misattribution ("gave up" recorded as "unsafe to recover") these pins exist to prevent. Until such a test exists, a refactor of the flag computation or the ternary condition ships a mislabeled diagnostic past CI. Suggested fix: add that budget-exhaustion-after-thinking test asserting retryDecision: 'exhausted' (the verifier's probe confirmed it kills the mutant).
中文说明
R2-2：该 pin 留有一个单行变异体存活：将 retryDecision 三元式（geminiChat.ts:3052）中的 streamYieldedContentChunk 替换为 streamYieldedChunk 后，整套测试仍然通过——变异体下实测 318/318 通过。仅有的两处 not-taken 断言中两个标志都相等（本测试：false/false；function-call 切断测试 ~7855：true/true），都无法区分三元式的条件变量。失败场景：一个「每次切断前 yield 一个纯 thought chunk 直至预算耗尽」的探针测试可使变异体翻转——在未变异代码上通过，在变异体下失败：期望 'exhausted' 却得到 'skipped_after_content'（把「放弃」误标为「不可恢复」），正是这些 pin 要防止的误标。在补充该测试之前，对标志计算或三元式条件的重构会让被误标的诊断字段绕过 CI。建议修复：补充该 thinking 后预算耗尽测试并断言 retryDecision: 'exhausted'（验证代理的探针已确认该测试可杀死变异体）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #8861 (review rounds 1–2 feedback)Verification-gate rejection from the previous round (fixed first)The previous round was rejected because Round-2 inline findings
Round-1 inline findings (re-verified)All four were already addressed in
ConflictsNone ( VerificationCommands actually run this round (repo root unless noted):
中文说明本轮总结 — PR #8861（review 第 1–2 轮反馈）上一轮验证门禁的拒绝（优先修复）上一轮被拒绝的原因是 第 2 轮 inline 发现
第 1 轮 inline 发现（复查确认）四条均已在
冲突无（ 验证本轮实际执行的命令（未注明者均在仓库根目录）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summaryAddressed both inline suggestions from the round-3 automated review of PR #8861. One required a comment correction in three places; the other was already resolved in the code and was re-verified empirically this round. Feedback points and decisions1. [rc:3751530800] Verified the finding's evidence against the code: thought parts DO reach user-visible surfaces before the retry gate runs. Chunks are yielded to callers first (the Removed the false clause from all three places where it echoed, grounding the safety rationale solely in the wholesale partial-turn discard, which holds independently:
2. [rc:3751530833] The suggested probe test already exists at HEAD: Review-body notes (non-blocking)The review body's Test Plan observation (description test counts differ from the observed run counts) reflects stale numbers in the PR description from earlier rounds, not a code defect — no action taken. ConflictsNone ( VerificationAll commands actually run this round; the change is comment-only in
Integration tests: not run — the change is comment-only and the touched behavior is fully exercised by the unit suite above; nothing here is bundled-CLI-only behavior. 中文说明本轮总结处理了 PR #8861 第三轮自动审查中的两条行内建议。其中一条需要在三处修正注释；另一条在代码中已经被解决，本轮通过实验重新验证确认。 反馈点与决定1. [rc:3751530800] 已对照代码核实该发现的证据：thought part 在重试门控运行之前确实会到达用户可见的界面。chunk 会先 yield 给调用方（attempt 循环中的 已从该子句重复出现的三处全部移除，将安全性论证完全建立在独立成立的 partial turn 整体丢弃之上：
2. [rc:3751530833] 建议中的探针测试在当前 HEAD 上已经存在： 审查正文备注（非阻断）审查正文中的 Test Plan 观察（PR 描述中的测试数量与实际运行数量不一致）反映的是 PR 描述中来自早前轮次的过期数字，并非代码缺陷——未采取行动。 冲突无（ 验证本轮实际运行的所有命令；改动为仅注释变更，涉及
集成测试：未运行——本次改动仅为注释，且涉及的行为已由上述单元测试完整覆盖；此处不存在仅能通过打包后 CLI 验证的行为。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1200, 18897, 1502, 481, 3069, 473 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1200, 18897, 1502, 481, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // Shared transport-cut fixtures. `socketCut` is the single producer of | ||
| // the retryable socket-failure shape the transport retry gate | ||
| // classifies, so a classified-shape change has one place to update | ||
| // (non-retryable shapes are constructed inline on purpose). |
There was a problem hiding this comment.
[Suggestion] The new fixture comment's "single producer … one place to update" claim is false: it.each([...RETRYABLE_STREAM_TRANSPORT_CODES]) (~line 9188) constructs the same classified retryable shape inline, parameterized over all 6 allow-listed codes including UND_ERR_SOCKET. The parenthetical holds only for the two genuinely non-retryable inline shapes (4xx at ~9433, ECONNREFUSED at ~9472). In a PR whose explicit purpose is making rationale comments factually correct, this newly written comment is inaccurate. — Failure scenario: if the classified error shape changes (e.g. undici's nesting changes and the classifier is updated), a maintainer trusting this comment updates only socketCut; the inline construction in the drift guard silently goes stale and its six tests fail confusingly, and the parenthetical actively reassures them that any inline shape must be non-retryable — so the drift gets debugged under a wrong mental model.
| // Shared transport-cut fixtures. `socketCut` is the single producer of | |
| // the retryable socket-failure shape the transport retry gate | |
| // classifies, so a classified-shape change has one place to update | |
| // (non-retryable shapes are constructed inline on purpose). | |
| // Shared transport-cut fixtures. `socketCut` is the shared producer of the | |
| // canonical `UND_ERR_SOCKET` retryable shape. The per-code drift-guard test | |
| // (`it.each` over the allow-list) constructs parameterized retryable shapes | |
| // inline on purpose; non-retryable shapes stay inline on purpose too. |
中文说明
新增的 fixture 注释声称「socketCut 是可重试形状的唯一生产者……只需改一处」，但这不成立：it.each([...RETRYABLE_STREAM_TRANSPORT_CODES])（约第 9188 行）同样以内联方式构造相同的、可被分类的重试形状，并对包括 UND_ERR_SOCKET 在内的全部 6 个白名单 code 参数化。括号里的说明只对两个真正不可重试的内联形状（4xx 约 9433、ECONNREFUSED 约 9472）成立。本 PR 的目的正是让注释中的论证与事实一致，这条新写的注释却不准确。——失败场景：若被分类的错误形状发生变化（例如 undici 的嵌套结构变化、分类器随之更新），信任此注释的维护者只会更新 socketCut；drift-guard 中的内联构造悄然过时，其六个测试以令人困惑的方式失败，而括号说明还会让人误以为所见内联形状一定是不可重试的——于是漂移在错误的心智模型下被排查。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // Same catch-and-assert drain as the zero-chunk exhaustion test: | ||
| // the rejection settles only after both retry delays elapse. | ||
| let caughtError: unknown; | ||
| const collecting = (async () => { |
There was a problem hiding this comment.
[Suggestion] This new test copies the 12-line catch-and-drain block byte-identically from the adjacent zero-chunk exhaustion test (~lines 7516-7531), leaving two identical copies in the file (a third variant lives near line 8751). The block hard-codes the settle protocol — collect events, catch the terminal error, advanceTimersByTimeAsync(0) + advanceTimersByTimeAsync(10_000) to cover both retry delays — and the original copy carries a 5-line comment documenting the fake-timer traps the protocol avoids, evidence it is genuinely delicate. — Concrete cost: any change to that protocol (the retry-delay sum moving off 10_000 ms, or the rejection needing a different timer advance) must be applied to both copies in lockstep; missing one makes that test fail, or pass for the wrong reason, silently.
Fix: extract a helper next to cutAfter and call it from both tests, keeping the timer-advance steps explicit at the call sites:
async function drainCollecting(stream: AsyncIterable<StreamEvent>) {
const events: StreamEvent[] = [];
let caughtError: unknown;
try {
for await (const event of stream) events.push(event);
} catch (error) {
caughtError = error;
}
return { events, caughtError };
}中文说明
新测试逐字节复制了相邻零 chunk 耗尽测试（约 7516-7531 行）的 12 行「捕获并排空」块，文件中因此存在两份完全相同的副本（约 8751 行还有第三个变体）。该块硬编码了排空协议——收集事件、捕获终结错误、advanceTimersByTimeAsync(0) + advanceTimersByTimeAsync(10_000) 以覆盖两次重试延迟——且原副本带有 5 行注释说明该协议规避的假定时器陷阱，足见其精细。——具体代价：对该协议的任何修改（重试延迟总和不再是 10_000 ms，或拒绝需要不同的定时器推进）都必须同步应用到两个副本；漏改其一会让该测试静默失败，或以错误的原因通过。
修复方式：在 cutAfter 旁提取一个辅助函数并在两个测试中调用，同时在调用处保留显式的定时器推进步骤（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(mockDebugLoggerWarn).toHaveBeenCalledWith( | ||
| 'Transport stream retry not taken', | ||
| expect.objectContaining({ | ||
| retryDecision: 'skipped_after_content', | ||
| }), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This pin cannot discriminate the ternary's real key: the cut here also delivers a functionCall, so both streamYieldedContentChunk and streamYieldedFunctionCall are true at this site. Measured under mutation: re-keying the not-taken ternary (geminiChat.ts:~3051) to streamYieldedFunctionCall survives all 319 tests; a companion assertion that reaches the not-taken branch with content delivered but no functionCall flips the mutant (observed retryDecision: 'exhausted' where 'skipped_after_content' was expected) and passes on the correct key. — Failure scenario: a future re-keying ships silently; for the dominant #7832 shape (plain answer text cut repeatedly, no functionCall ever delivered) the debug log then reports 'exhausted' instead of 'skipped_after_content' — misattributing "unsafe to recover" as "gave up", the exact misattribution this diagnostic exists to prevent.
Fix: extend 'propagates once the continuation budget is exhausted' — a plain-text cut whose continuation budget exhausts reaches the not-taken branch with streamYieldedContentChunk=true, streamYieldedFunctionCall=false — to also assert retryDecision: 'skipped_after_content'. Note: a first-cut plain-text scenario enters the continuation branch, not the not-taken branch, so it cannot host this pin.
中文说明
这个钉住无法区分三元表达式的真实键：此处的切断同时送达了 functionCall，因此该处 streamYieldedContentChunk 与 streamYieldedFunctionCall 同为 true。变异实测：把 not-taken 三元（geminiChat.ts:~3051）的键改为 streamYieldedFunctionCall 后，全部 319 个测试依然通过；而一个「已送达内容但没有 functionCall」并走到 not-taken 分支的伴随断言能让该变异体失败（实际观察到 retryDecision: 'exhausted'，期望 'skipped_after_content'），且在正确的键下通过。——失败场景：未来一次改键会悄无声息地合入；对 #7832 的主要形态（纯文本答复被反复切断、始终没有 functionCall 送达），debug 日志将报告 'exhausted' 而非 'skipped_after_content'——把「无法安全恢复」误报为「放弃重试」，正是该诊断字段要避免的误归因。
修复方式：扩展 'propagates once the continuation budget is exhausted'——该测试中文本被切断且续传预算耗尽，会以 streamYieldedContentChunk=true, streamYieldedFunctionCall=false 走到 not-taken 分支——为其补充 retryDecision: 'skipped_after_content' 断言。注意：首次切断的纯文本场景走的是续传分支而非 not-taken 分支，不能承载该钉住。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| * attempt's thoughts are recorded there. It is that the failed | ||
| * attempt's accumulated partial turn is discarded wholesale before the | ||
| * retry (`popPendingPartialAssistantTurn`), so nothing the caller saw |
There was a problem hiding this comment.
[Suggestion] The corrected rationale attributes thinking-only replay safety to popPendingPartialAssistantTurn, but on this path the pop is a provable no-op: a mid-stream partial turn is persisted on error only when willPersistToHistory holds, which for streamError !== null reduces to hasToolCall && … (~line 4894; the in-code comment at ~5003 states "Plain-text partial turns (no functionCall yielded) are deliberately NOT persisted"). A yielded functionCall is a non-thought part, so it sets streamYieldedContentChunk, which closes this very gate — therefore whenever the replay branch executes, pendingPartialAssistantTurnIndex is null and the pop early-returns (~1806). The load-bearing mechanism is the non-persistence policy; the pop is defense in depth. The code is correct — but rationale accuracy is exactly the defect class this PR exists to fix, and the gate comment at ~2932 repeats the same misattribution. — Failure scenario: (a) a maintainer pruning retry cleanup sees the pop, observes thinking-only failures never push to history, and removes the "dead" pop — behavior-neutral today, silently wrong the day persistence widens to thought-only partials (JSONL parity is a documented concern at ~4886); (b) someone debugging duplicated output reads this comment and stops at the pop, never reaching the persistence gate that actually does the work.
Fix (both comment sites): restate as the non-persistence policy with the pop as belt-and-braces, e.g.
* attempt's thoughts are recorded there. It is that a failed attempt that
* produced only thought parts persists nothing: a mid-stream partial turn is
* recorded on error only when a functionCall was already delivered, which the
* replay gate excludes. `popPendingPartialAssistantTurn()` before the retry is
* defense in depth — it has nothing to pop on this path today, but keeps the
* replay safe if that persistence policy ever widens.
中文说明
修正后的论证把 thinking-only 重放的安全性归因于 popPendingPartialAssistantTurn，但在该路径上这个 pop 是可证明的空操作：流中错误时只有 willPersistToHistory 成立才会持久化 partial turn，而 streamError !== null 时它退化为 hasToolCall && …（约 4894 行；约 5003 行的代码内注释明确写着「纯文本 partial turn（未送达 functionCall）刻意不持久化」）。送达的 functionCall 是非 thought part，会置位 streamYieldedContentChunk，从而关闭这个门控本身——因此重放分支执行时 pendingPartialAssistantTurnIndex 必为 null，pop 提前返回（约 1806 行）。真正承重的机制是「不持久化」策略；pop 只是纵深防御。代码本身是正确的——但注释论证的准确性正是本 PR 要修复的缺陷类别，且约 2932 行的门控注释重复了同样的误归因。——失败场景：(a) 维护者精简重试清理路径时看到这个 pop，发现 thinking-only 失败从不写入 history，便删掉这个「死代码」——今天行为等价，可一旦持久化策略扩展到 thought-only partial（约 4886 行已把 JSONL 一致性列为关注点），就会悄无声息地出错；(b) 排查重试后输出重复的人读到这条注释，会停在 pop 处，而不到达真正起作用的持久化门控。
修复方式（两处注释同步）：改为陈述「不持久化」策略、把 pop 表述为纵深防御（见上方示例措辞）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // Thinking-only output does not block the replay: the | ||
| // failed attempt's partial turn is popped wholesale below, | ||
| // so nothing the caller saw from that attempt can appear |
There was a problem hiding this comment.
[Suggestion] Same misattribution as the hasNonThoughtCandidateParts docstring (companion comment at ~line 152): on the thinking-only path nothing was persisted, so "popped wholesale below" discards nothing — the replay is safe because a mid-stream partial turn is recorded on error only when a functionCall was already delivered, which this gate excludes; the pop below is defense in depth. — Failure scenario: same as the companion comment: a future pruning of the seemingly dead pop, or a debugger stopping here instead of at the persistence gate.
Fix: mirror the corrected wording, e.g. "Thinking-only output does not block the replay: such an attempt persists nothing (error-path persistence requires a delivered functionCall, which this gate excludes), and the partial turn is popped wholesale below as defense in depth — so nothing the caller saw from that attempt can appear twice."
中文说明
与 hasNonThoughtCandidateParts 文档注释（约 152 行的伴生评论）相同的误归因：在 thinking-only 路径上什么都没有持久化，所以下文的「整体 pop」丢弃的其实是空——重放之所以安全，是因为流中错误时只有已送达 functionCall 才会记录 partial turn，而该门控恰好排除了这种情况；下文的 pop 只是纵深防御。——失败场景：同伴生评论：未来有人删掉这个看似无用的 pop，或调试者停在这里而不到达持久化门控。
修复方式：与文档注释保持一致措辞，例如「thinking-only 输出不阻塞重放：这样的 attempt 不会持久化任何内容（错误路径的持久化要求已送达 functionCall，而此门控排除了它），下文的整体 pop 只是纵深防御——因此调用方从该 attempt 看到的任何内容都不可能出现两次。」
— qwen3.8-max via Qwen Code /review (v0.21.9)
…enLM#8861) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #8861Feedback addressed: the five inline suggestions from the automated reviewer's Feedback points and decisions1. [Suggestion] Fixture comment's "single producer" claim is false (
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1200, 18898, 1502, 481, 3069, 473 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 1200, 18898, 1502, 481, 3069, 473 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // The retry log must record that non-content chunks (the | ||
| // thinking) had already flowed — the diagnostic that makes | ||
| // thinking-phase replays visible in the debug log. | ||
| expect(mockDebugLoggerWarn).toHaveBeenCalledWith( | ||
| 'Transport stream retry scheduled', |
There was a problem hiding this comment.
[Suggestion] R5-1: The new yieldedNonContentChunks pins leave one reachable mutation alive: swapping the per-attempt flag for the cross-attempt flag (yieldedNonContentChunks: streamYieldedChunk → streamYieldedAnyChunk, geminiChat.ts:2974) passes every current test, because in all existing retry-scheduling scenarios the two flags agree (all attempts yield thought chunks, or none do). — Failure scenario: attempt 1 yields a thought chunk then is socket-cut (retry scheduled — both flags true); attempt 2 is cut immediately with zero chunks — the retry gate is reached again (transportStreamRetryCount 1 < 2, no content delivered, continuation buffer empty since thought text is excluded by isPlainTextPart), and attempt 2's 'Transport stream retry scheduled' log must say yieldedNonContentChunks: false; the mutant logs true. No test asserts that mixed sequence, so a future regression to the cross-attempt flag would ship and misattribute thinking-phase flow in the very diagnostic this PR pins. Impact is confined to the debug log; the code as written is correct today. Measured: with the mutant applied the full geminiChat.test.ts suite passes 319/319; a mixed-attempt probe fails under the mutant and passes on revert. Suggested fix: add a mixed-attempt test (attempt 1: cutAfter([thoughtChunk]), attempt 2: immediate cut, attempt 3: success or exhaustion under fake timers) asserting the second 'Transport stream retry scheduled' call carries yieldedNonContentChunks: false — e.g. filter mockDebugLoggerWarn.mock.calls to the scheduled logs and assert [true, false].
中文说明
新增的 yieldedNonContentChunks 断言留下了一个可达的变异体：把 per-attempt 标志换成跨 attempt 标志（yieldedNonContentChunks: streamYieldedChunk → streamYieldedAnyChunk，geminiChat.ts:2974）能通过当前所有测试，因为在现有的所有重试调度场景中两个标志都一致（要么每次 attempt 都产生 thought chunk，要么都没有）。失败场景：attempt 1 先 yield 一个 thought chunk 再被切断（重试被调度——两个标志均为 true）；attempt 2 在零 chunk 时立即被切断——重放门控再次可达（transportStreamRetryCount 1 < 2、未送达内容、续传缓冲区为空，因为 thought 文本被 isPlainTextPart 排除），attempt 2 的 'Transport stream retry scheduled' 日志本应输出 yieldedNonContentChunks: false，变异体却输出 true。没有任何测试断言这个混合序列，因此未来若回归到跨 attempt 标志，将会悄悄把 thinking 阶段的流量错误地记入本 PR 正要钉住的诊断字段。影响仅限于 debug 日志；当前代码本身是正确的。实测：应用该变异体后 geminiChat.test.ts 全量套件 319/319 通过；混合 attempt 探针在变异体下失败、还原后通过。建议修复：新增一个混合 attempt 测试（attempt 1：cutAfter([thoughtChunk])，attempt 2：立即切断，attempt 3：在 fake timers 下成功或耗尽），断言第二次 'Transport stream retry scheduled' 调用携带 yieldedNonContentChunks: false——例如过滤 mockDebugLoggerWarn.mock.calls 中的 scheduled 日志并断言 [true, false]。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action required this round (PR #8861)No actionable feedback was found for this round:
The branch remains at commit 中文说明本轮无需采取任何操作（PR #8861）本轮未发现可操作的反馈：
分支仍停留在提交 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@wenshao @yiliang114 Gentle ping when you have a moment — this follow-up to your #7938 verification has been green for a while (all checks passing, both findings landed plus the four inline-review items), and branch protection is now only waiting on code-owner approvals. No rush, just flagging it's ready. / 顺手 ping 一下：#7938 验证报告的这个后续已全绿多时（两 finding + 四条 inline 建议均已落地），分支保护只差 code owner approve，方便时请过目，不急。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round result: no action required (PR #8861)Triage summaryThis round's feedback contains no actionable items:
ResultNo code changes were made this round and nothing was committed. The branch remains at 中文说明Autofix 轮次结果：无需处理（PR #8861）分类摘要本轮反馈中没有任何可处理的条目：
结果本轮未做任何代码改动，也没有创建提交。分支仍停留在 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Maintainer verification — real local stackVerdict: LGTM, merge-ready. Both findings from the #7938 verification are genuinely closed: the corrected comment is factually true (checked at runtime, not by reading), and both diagnostics are now pinned — every mutant I could write against them survives the suite on Environment
1. The source change really is comment-onlyNot just "the diff lines start with Zero runtime behavior change, confirmed mechanically. 2. Suite and gates
3. Mutation matrix — 6 mutants × {main, PR}This is the load-bearing evidence for finding (b). I mutated
6/6 survive on 4. Finding (a) — is the corrected rationale actually true?I did not take the comment's word for it. Three probes driven against the real
5. Real stack — both diagnostics observed in a production debug logTo confirm these are not test-only fields, I bundled the CLI from this branch (
Scenario A — cut while only The mock also recorded every upstream body: request #1 is a byte-identical replay of #0 ( Scenario B — visible text delivered then cut, on every attempt. Three continuations, then: Both pinned diagnostics are real, reachable artifacts. The assertions are worth having. Non-blocking notes (description only — no code change requested)
Tested on
中文版本维护者验证 —— 本地真实环境结论：LGTM，可以合并。 #7938 验证报告里的两个 finding 都被真正闭合了：修正后的注释在运行时被证实为真（不是靠读代码），两个诊断字段也确实被钉住了 —— 我针对它们写的每一个变异体，在 环境
1. 源码改动确实只有注释不是只看 diff 行是否以 运行时行为零变化，机械可证。 2. 测试与门禁
3. 变异矩阵 —— 6 个变异体 × {main, PR}这是 finding (b) 的核心证据。我对
6/6 在 4. Finding (a) —— 修正后的论证是不是真的成立？我没有直接采信注释。三个探针直接驱动真实的
5. 真实栈 —— 两个诊断都在真实 debug 日志里被观测到为了确认这不是只存在于测试里的字段，我从本分支打了 CLI bundle（ 场景 A —— 只流出了 mock 还记录了每一次上行请求体：request #1 与 #0 逐字节相同（两者 场景 B —— 每次都先送出可见文本再切断。 三次续传后： 两个被钉住的诊断都是真实可达的产物，这些断言值得保留。 不阻塞的说明（只涉及描述，不要求改代码）
测试平台
|
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action taken — PR #8861This round's feedback contains no actionable code changes, so no commit was made.
No source files were modified; the branch remains at its verified head commit 中文说明未采取行动 —— PR #8861本轮反馈中没有需要处理的代码改动，因此未产生任何提交。
未修改任何源文件；分支保持在已验证的 head commit Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@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: 44 passed · 0 failed · 44 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：44 通过 · 0 失败 · 44 总计 Verification reportVerification report — PR #8861Verdict: 中文摘要
Central claim and A/BCentral claim: the
Witnesses: Mutation A/B matrix (7 mutants × 2 arms: 14 mutant runs + 2 unmutated controls = 16 vitest runs)
Every restore verified byte-identical (sha256) after each cell; both trees pristine at the end. Witness: Reviewer Test Plan walkthrough
Corrections (description-level, not code-change requests)
FindingsNo blocking findings. C1–C3 above are the complete finding list; all are description-level (the code, comments, and pins are correct and load-bearing). 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 |
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. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action taken — PR #8861This round's feedback contains no actionable code changes, so no commit was made.
No source files were modified; the branch remains at its verified head commit 中文说明未采取行动 —— PR #8861本轮反馈中没有需要处理的代码改动，因此未产生任何提交。
未修改任何源文件；分支保持在已验证的 head commit Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
…iagnostics-followup # Conflicts: # packages/core/src/core/geminiChat.ts
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8861 (address-review)Feedback triageThis round ran in Critical-only mode (5 change-producing rounds complete). The round's work was therefore the requested base-conflict resolution Conflict resolution
Resolution: keep our corrected comment block and adopt main's code shape
Other notes
Verification
中文说明Autofix 轮次总结 — PR #8861（address-review）反馈分诊本轮运行于 仅 Critical 模式（已完成 5 个产生改动的轮次）。可执行区域为空：没有评审、没有行内评论、没有 issue 级评论、没有失败的检查，也没有持续失败的检查。 因此本轮的工作是所请求的与 base 的冲突解决（ 冲突解决
解决方式：保留我方修正后的注释块，同时采用 main 的代码形态（删除本地 const，门槛改为调用导入的函数）。解决前已验证：
其他说明
验证
🧭 Gate advisory — this round modified areas outside the PR footprint (machine-measured, not agent-authored):
Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
c87e511
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 3b": executed the new/modified transport-retry tests to confirm they pass (worktree has no node_modules; npm ci + build not run).
Test Plan (not a blocker): src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 21249, 1689, 23736, 1658, 601, 4227, 626 passed.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/core/geminiChat.test.ts:9270 — [probe] R5-1 (round-5 ledger, still stands): yieldedNonContentChunks pins leave the per-attempt→cross-attempt flag-swap mutation alivepackages/core/src/core/geminiChat.test.ts:8970 — [review] socketCut dedup sweep missed this test — sole remaining inline UND_ERR_SOCKET copypackages/core/src/core/geminiChat.test.ts:10244 — [review] inline drain-and-catch IIFE survives beside the new drainCollecting helper in an edited test
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 3b"：executed the new/modified transport-retry tests to confirm they pass (worktree has no node_modules; npm ci + build not run)。
Test Plan（非阻断）：src/core/geminiChat.test.ts — no such file or directory; 300 passed — this review observed 21249, 1689, 23736, 1658, 601, 4227, 626 passed。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Released in v0.22.2. |
4 participants
What this PR does
Follow-up to #7938, landing the two non-blocking findings from the maintainer verification that were fixed locally but did not make it into the branch before the merge landed.
Corrects the replay-safety rationale in both comment sites (the
hasNonThoughtCandidatePartsdocstring and the replay gate comment). The comments claimed thought parts are never recorded in history; that is not the invariant — the successful attempt's thoughts are recorded there. What actually makes the replay safe is that the failed attempt's accumulated partial turn is discarded wholesale before the retry (popPendingPartialAssistantTurn), and thought parts are never user-visible content, so nothing the caller saw from the failed attempt can appear twice.Pins the two retry diagnostics with test assertions.
yieldedNonContentChunkson the "Transport stream retry scheduled" log is now asserted in the thinking-only replay test, and theskipped_after_contentdecision on the "Transport stream retry not taken" log is covered by a new test. Since fix(core): resume long streams cut by a socket-level close #7896 added continuation recovery, a text cut after visible content no longer reaches the not-taken log — the one path on current main that still emitsskipped_after_contentis a cut after a deliveredfunctionCall, where the replay gate (non-thought output delivered) and the continuation gate (functionCall boundary excluded) are both closed. The new test exercises exactly that path.The source diff to
geminiChat.tsis comment-only; there is zero runtime behavior change.Why it's needed
The verification on #7938 confirmed the fix merge-ready but reported two follow-ups: an imprecise safety rationale in the comments (a future reader relying on "thoughts never enter history" would be misled), and two mutation survivors showing the diagnostic fields were untested — hardcoding
yieldedNonContentChunksor relabelingskipped_after_contentpassed the full suite. Those diagnostics are what make thinking-phase replays auditable in the debug log, so they should be pinned.Reviewer Test Plan
How to verify
vitest run src/core/geminiChat.test.tsinpackages/core— 301/301 pass.yieldedNonContentChunks: streamYieldedChunk→yieldedNonContentChunks: false⇒ 1 failed / 300 passed (killed by "retries a transport stream error after yielding only thinking chunks").'skipped_after_content'→'skipped_after_chunk'⇒ 1 failed / 300 passed (killed by the new "attributes a blocked replay to delivered content when a function call was cut").popPendingPartialAssistantTurn()before retrying, andrecordHistorykeeps the successful attempt's thought parts.Evidence (Before & After)
N/A (comment-only source change + test additions).
Tested on
Environment (optional)
N/A (unit tests only).
Risk & Scope
objectContaining.geminiChatsuite plustsc/eslint/prettieron the two files; the remaining verification follow-ups tracked elsewhere (continuation semantics) are untouched.Linked Issues
Follow-up to #7938 (issue #7832); addresses the two findings in #7938 (comment).
中文说明
本 PR 做了什么
#7938 的后续：落地维护者验证报告中两个不阻塞 finding 的修复（当时已在本地完成，但没赶在合并前推上分支）。
修正两处注释中的重放安全性论证（
hasNonThoughtCandidateParts的 docstring 与重放门控处的注释）。原注释声称 thought part 永远不会进入 history；这不是真正的不变量——成功那次 attempt 的 thought 是会被记录的。重放之所以安全，是因为失败 attempt 累积的 partial turn 在重试前会被整体丢弃（popPendingPartialAssistantTurn），且 thought part 永远不是用户可见内容，所以调用方从失败 attempt 看到的任何东西都不可能出现两次。用测试断言钉住两个重试诊断字段。"Transport stream retry scheduled" 日志上的
yieldedNonContentChunks现由 thinking-only 重放测试断言；"Transport stream retry not taken" 日志上的skipped_after_content由一个新测试覆盖。由于 fix(core): resume long streams cut by a socket-level close #7896 引入了续传恢复，可见文本被切断后已不再走 not-taken 日志——当前 main 上仍会产生skipped_after_content的路径是functionCall送达后被切断：重放门控（已送达非 thought 输出）与续传门控（functionCall 边界被排除）同时关闭。新测试正是覆盖这条路径。geminiChat.ts的源码 diff 仅为注释，运行时行为零变化。为什么需要
#7938 的验证确认修复可合并，但报告了两个后续项：注释中的安全性论证不精确（未来读者若依赖「thought 不进 history」会被误导）；两个变异体幸存说明诊断字段无测试——硬编码
yieldedNonContentChunks或改写skipped_after_content标签都能通过全量套件。这些诊断字段是 thinking 阶段重放在 debug 日志中可审计的依据，应当钉住。验证方式
packages/core下vitest run src/core/geminiChat.test.ts—— 301/301 通过。yieldedNonContentChunks硬编码为false⇒ 1 失败 / 300 通过；'skipped_after_content'改为'skipped_after_chunk'⇒ 1 失败 / 300 通过（由新增的 function-call 切断测试杀死）。popPendingPartialAssistantTurn()，且recordHistory保留成功 attempt 的 thought part。风险与范围
objectContaining断言验证报告点名的两个字段。geminiChat套件与两文件的tsc/eslint/prettier外未做其他验证；验证报告中其余后续项（续传语义）不在本 PR 范围。关联 Issue
#7938（issue #7832）的后续；处理 #7938 (comment) 中的两个 finding。