fix(core): reject upstream fail-fast placeholder responses - #8938
Merged
Conversation
Upstream endpoints occasionally fail fast with an HTTP 200 whose entire
body is the placeholder text '(request timeout)'. The response passes
all existing stream validation and gets persisted as a normal assistant
reply, polluting subsequent request context and surfacing to users as a
lost reply.
- Throw InvalidStreamError('UPSTREAM_DEGRADED_RESPONSE') at stream end
when the whole response body is exactly the placeholder, reusing the
transient retry budget so the turn is rolled back and re-attempted.
- Treat placeholder-only model turns as invalid in extractCuratedHistory
so already-polluted sessions (including --resume) stop replaying them.
Both checks are exact whole-turn matches to avoid false positives on
legitimate mentions of the text.
Fixes #8916
yiliang114
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
wenshao
as code owners
August 11, 2026 12:48
github-actions
Bot
added
the
review/self-reported
Collaborator
Collaborator
|
Re-run after new pushes — the previous pass reviewed Template looks good ✓ — all required sections present, Before/After correctly marked N/A for a non-user-visible runtime change. Problem: observed, not theoretical. Issue #8916 documents a production session that hit the degraded response 3 times (HTTP 200 + normal finish reason with an entire body of just the 9-token Direction: aligned. Robust handling of degraded upstream responses is core chat-reliability work, and there is in-repo precedent for special-casing known placeholders ( Size: core paths touched ( Approach: the scope is now exactly the issue's two defenses plus the consequences they imply. Stream-end detection throws a new Risk: Moving on to code review. 🔍 中文说明新 push 后的 re-run —— 上一轮审查的是 模板完整 ✓ —— 所有必需章节齐全，非用户可见的运行时改动正确地将 Before/After 标注为 N/A。 问题：已观测到的真实问题，非理论加固。issue #8916 记录了生产会话 3 次命中降级响应（HTTP 200 + 正常 finish reason，完整正文仅为 9 token 的 方向：对齐。降级上游响应的稳健处理是核心聊天可靠性工作，仓库内已有特判已知占位符的先例（ 规模：触及核心路径（ 方案：范围现在恰好是 issue 的两层防御及其必要推论。流末检测抛出新 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent proposal first (written from the title + "Why it's needed" before reading the diff): wrap the raw stream in a guard that holds chunks only while they can still form the exact placeholder, throws a new Mechanics verified against the code at this head:
No critical issues, no convention violations. This is unattended CI triage — PR code was never executed here; the test evidence below is the PR's own CI read via the API. CI test evidenceAll checks on the reviewed commit have completed — nothing pending, nothing failed. The primary unit suite
Sandboxed verification would settle this: 中文说明先给独立方案（仅凭标题与"为什么需要"、未读 diff 时写下）：用守卫包装原始流，只在累积文本仍可能构成精确占位符时暂存 chunk；完整匹配时抛出新 对本 head 代码逐项核验：
无阻断问题，无规范违规。本次为无人值守 CI triage——从未执行 PR 代码；下方测试证据来自 PR 自身 CI（经 API 读取）。 CI 证据：reviewed commit 上所有检查已完成——无进行中、无失败。主单测 沙箱验证可以一锤定音： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; the revised diff is tighter than the one last reviewed, and nothing in it is speculative. Re-run verdict after re-reviewing from scratch at Stepping back: this remains what a small reliability PR should look like. A real production failure — three user-visible replies silently replaced by a 9-token placeholder that then replayed into every subsequent request — meets a defense that leans entirely on machinery already proven in this file: the new The residual is honest and named: exact-match detection of upstream wording can only be validated by production observability — the design fails open there by intent, and no sandbox lane changes that. CI is fully green on the reviewed commit (primary unit suite included; macOS/Windows/integration are skipped by CI configuration, not failures), and the six new tests pin the behavior — they fail with the guard removed. Approving, pinned to the reviewed commit. ✅ 中文说明在 回顾整体：这仍然是小型可靠性 PR 应有的样子。真实的生产故障——三轮用户可见回复被 9 token 占位符静默替换并持续回放进后续每次请求——对应的是完全依托本文件既有成熟机制的防御：新 剩余的不确定性是诚实且已声明的：对上游文案的精确匹配只能靠生产观测验证——设计上有意在此 fail-open，任何沙箱通道都无法改变这一点。reviewed commit 上 CI 全绿（含主单测；macOS/Windows/集成任务为 CI 配置跳过，并非失败），6 个新测试钉住行为——移除守卫即失败。 批准，钉在 reviewed commit 上。✅ — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
Refreshed this branch onto latest |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): PR #8938 adds two defenses against upstream fail-fast pla...: none — all checks I started were completed within budget..
Test Plan (not a blocker): 320 passed — this review observed 19753, 1277, 19139, 1511, 494, 3333, 554 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：PR #8938 adds two defenses against upstream fail-fast pla...：none — all checks I started were completed within budget.。
Test Plan（非阻断）：320 passed — this review observed 19753, 1277, 19139, 1511, 494, 3333, 554 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
previously requested changes
Aug 12, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): 这是 PR #8938（上游 fail-fast 占位符响应防御）的反向审计第 4 轮：寻找前几轮都漏掉的缺陷。已...: none — all checks above completed within budget.; 本次 PR 为 qwen-code 增加两层针对上游 fail-fast 占位符响应（HTTP 200 但正文仅为...: verifying whether real upstream fail-fast payloads actually surface as streamable text chunks (vs. a generator-level parse error) and whether they carry usageM…; 本次 PR 为 qwen-code 增加两层针对上游 fail-fast 占位符响应（HTTP 200 但正文仅为...: none — all planned checks completed within budget..
Test Plan (not a blocker): 320 passed — this review observed 19772, 1285, 19291, 1511, 494, 3393, 555 passed.
中文说明
未探索到全部深度（达到工具调用预算）：这是 PR #8938（上游 fail-fast 占位符响应防御）的反向审计第 4 轮：寻找前几轮都漏掉的缺陷。已...：none — all checks above completed within budget.；本次 PR 为 qwen-code 增加两层针对上游 fail-fast 占位符响应（HTTP 200 但正文仅为...：verifying whether real upstream fail-fast payloads actually surface as streamable text chunks (vs. a generator-level parse error) and whether they carry usageM…；本次 PR 为 qwen-code 增加两层针对上游 fail-fast 占位符响应（HTTP 200 但正文仅为...：none — all planned checks completed within budget.。
Test Plan（非阻断）：320 passed — this review observed 19772, 1285, 19291, 1511, 494, 3393, 555 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Collaborator
Author
|
Closeout from resolve-pr-comments automation: Changed: merged latest main to refresh the exact-head linter-install CI failure. No product code changes beyond the base refresh. Pending: new CI and automatic review are running; existing review threads still need a separate fix/decision pass. |
qwen-code-ci-bot
previously requested changes
Aug 12, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
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): PR #8938 adds two defenses against upstream fail-fast pla...: none — all planned checks completed (~13 of ~35 tool calls).; PR #8938 adds two defenses against upstream fail-fast pla...: did not empirically verify (in the CLI UI code) that the streamed placeholder text is visually cleared when the Retry event fires; inferred it from the shared p….
Test Plan (not a blocker): 320 passed — this review observed 19808, 1285, 19356, 1511, 494, 3392, 555 passed.
[Critical] R2-1: Re-checked this round on c504077 — still stands (probe-verified). The placeholder chunk reaches user-visible output before this end-of-stream throw can fire, so the degraded text is still shown on the very attempt this PR exists to suppress. The chunk (text + finishReason: 'STOP') is yielded inside the stream loop first; in the TUI, Turn.run emits Content + Finished before Retry, and the Finished handler in useGeminiStream commits the pending '(request timeout)' item before the non-continuation Retry discard runs — committed items have no removal path. Headless: BaseJsonOutputAdapter.processEvent appends Content unconditionally and has no Retry case (default: break) → qwen -p output becomes '(request timeout)Recovered response'. A probe on this commit reproduced the headless outcome end-to-end ('(request timeout)Recovered response'), and a minimal Retry-case fix flips it. Persistence and replay are fixed by this PR; display is not. Suggested fix: detect before delivery for the dominant fail-fast shape (when an attempt's first chunk already carries a finishReason, hold it and validate before yielding), and make consumers roll back on non-continuation Retry (remove the failed attempt's committed assistant item in useGeminiStream; add a Retry case to the JSON adapters). (Already filed inline at geminiChat.ts:4875 in round 2 — carried in the body to avoid a duplicate comment.)
中文说明
R2-1：本轮在 c504077 上复查——仍然存在（探针验证）。占位符 chunk 在这处流末 throw 生效之前就已到达用户可见输出，因此在本 PR 旨在消除的那次请求里，退化文本仍然会展示给用户。chunk（文本 + finishReason: 'STOP'）在流式循环内先被 yield；TUI 中 Turn.run 先发 Content + Finished 再发 Retry，useGeminiStream 的 Finished 处理器会在非续传 Retry 的丢弃逻辑运行之前提交待定的 '(request timeout)' 项——已提交的项没有移除路径。Headless 模式：BaseJsonOutputAdapter.processEvent 无条件追加 Content 且没有 Retry 分支（default: break）→ qwen -p 的输出变成 '(request timeout)Recovered response'。对本提交的探针端到端复现了 headless 结果（'(request timeout)Recovered response'），一处最小化的 Retry 分支修复即可翻转该结果。本 PR 修复了持久化与回放；展示面未修复。修复建议：对主要的 fail-fast 形态在投递前检测（当一次尝试的首个 chunk 已携带 finishReason 时，先暂存并在 yield 前完成校验），同时让消费方在非续传 Retry 时回滚（在 useGeminiStream 中移除失败尝试已提交的 assistant 项；为 JSON 适配器补 Retry 分支）。（已在第 2 轮以行内评论提交于 geminiChat.ts:4875——为避免重复评论，此处置于正文。）
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #8938 adds two defenses against upstream fail-fast pla...：none — all planned checks completed (~13 of ~35 tool calls).；PR #8938 adds two defenses against upstream fail-fast pla...：did not empirically verify (in the CLI UI code) that the streamed placeholder text is visually cleared when the Retry event fires; inferred it from the shared p…。
Test Plan（非阻断）：320 passed — this review observed 19808, 1285, 19356, 1511, 494, 3392, 555 passed。
[Critical] R2-1: Re-checked this round on c504077 — still stands (probe-verified). The placeholder chunk reaches user-visible output before this end-of-stream throw can fire, so the degraded text is still shown on the very attempt this PR exists to suppress. The chunk (text + finishReason: 'STOP') is yielded inside the stream loop first; in the TUI, Turn.run emits Content + Finished before Retry, and the Finished handler in useGeminiStream commits the pending '(request timeout)' item before the non-continuation Retry discard runs — committed items have no removal path. Headless: BaseJsonOutputAdapter.processEvent appends Content unconditionally and has no Retry case (default: break) → qwen -p output becomes '(request timeout)Recovered response'. A probe on this commit reproduced the headless outcome end-to-end ('(request timeout)Recovered response'), and a minimal Retry-case fix flips it. Persistence and replay are fixed by this PR; display is not. Suggested fix: detect before delivery for the dominant fail-fast shape (when an attempt's first chunk already carries a finishReason, hold it and validate before yielding), and make consumers roll back on non-continuation Retry (remove the failed attempt's committed assistant item in useGeminiStream; add a Retry case to the JSON adapters). (Already filed inline at geminiChat.ts:4875 in round 2 — carried in the body to avoid a duplicate comment.)
中文说明
R2-1：本轮在 c504077 上复查——仍然存在（探针验证）。占位符 chunk 在这处流末 throw 生效之前就已到达用户可见输出，因此在本 PR 旨在消除的那次请求里，退化文本仍然会展示给用户。chunk（文本 + finishReason: 'STOP'）在流式循环内先被 yield；TUI 中 Turn.run 先发 Content + Finished 再发 Retry，useGeminiStream 的 Finished 处理器会在非续传 Retry 的丢弃逻辑运行之前提交待定的 '(request timeout)' 项——已提交的项没有移除路径。Headless 模式：BaseJsonOutputAdapter.processEvent 无条件追加 Content 且没有 Retry 分支（default: break）→ qwen -p 的输出变成 '(request timeout)Recovered response'。对本提交的探针端到端复现了 headless 结果（'(request timeout)Recovered response'），一处最小化的 Retry 分支修复即可翻转该结果。本 PR 修复了持久化与回放；展示面未修复。修复建议：对主要的 fail-fast 形态在投递前检测（当一次尝试的首个 chunk 已携带 finishReason 时，先暂存并在 yield 前完成校验），同时让消费方在非续传 Retry 时回滚（在 useGeminiStream 中移除失败尝试已提交的 assistant 项；为 JSON 适配器补 Retry 分支）。（已在第 2 轮以行内评论提交于 geminiChat.ts:4875——为避免重复评论，此处置于正文。）
— qwen3.8-max via Qwen Code /review (v0.21.10)
Collaborator
Author
|
No code change. The exact-head failed Qwen Code CI run had empty failed-job logs from the API, and this branch was already refreshed to latest base in the previous pass, so I reran the failed jobs and left it pending verification. |
Collaborator
Author
|
The exact-head failed-job logs for run 31633955399 were empty via |
#8938) - Defer the first chunk with a finishReason in non-continuation turns so post-stream validation (placeholder check, empty-text check) can reject it before consumers see the content. Without this a single-chunk fail-fast placeholder (text + finishReason: 'STOP') was yielded to the TUI / headless adapter before the throw, and neither consumer rolled back the committed output. - Add exhaustion test for UPSTREAM_DEGRADED_RESPONSE retry budget. - Add curation test for whitespace-padded placeholder (.trim() guard). - Add curation test for placeholder turn carrying a functionCall. - Assert the placeholder text is absent from emitted stream events.
Collaborator
Author
|
R1-1 (Side-query/compaction path): Follow-up. The side-query/compaction path is a separate code path outside this PR's scope (conversation-layer defense). Tracked for a follow-up issue. R2-2 (Fallback model path): Follow-up. The fallback model path is a separate code path with its own retry wiring. Outside this PR's scope. Tracked for a follow-up issue. |
Collaborator
Author
|
R3-1 (Blocklist fragility): Follow-up. The exact-text approach is by design — fail-open when upstream changes the placeholder. The 4-retry budget is intentional and the comment block already documents this tradeoff. If a real false-positive case emerges, a transport-level signal or diagnostic can be added then. R3-2 (Allocation overhead): Follow-up. The early-return length check is a valid micro-optimization, but the allocation is transient (GC'd) and this is a curated-history path, not a streaming hot path. Can be addressed in a separate perf PR. R3-3 (Duplicated predicate): Follow-up. The two predicates have different gates (tool-call check vs hasToolCall), which makes direct unification non-trivial. Deferred drift risk is acknowledged; can be addressed when a second placeholder variant appears. |
Collaborator
Author
|
Refreshed this branch with the latest CI/review are running on the refreshed head. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
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 explored to full depth (tool budget reached): "PR #8938 adds two defenses against upstream fail-fast…": did not run the new unit tests ( geminiChat.test.ts ) to confirm they pass.; "PR #8938 adds two defenses against upstream fail-fast…": did not inspect useGeminiStream.ts / nonInteractiveCli.ts abort handling — finding 2's mechanism is verified via Turn.run only.; "PR #8938 adds two defenses against upstream fail-fast…": did not enumerate anthropic/OpenAI converter chunk shapes to bound how often two finishReason chunks can precede the first yield (finding 1 trigger frequency).; "PR #8938 adds two defenses against upstream fail-fast…": did not verify the OpenAI/compat transformer's chunk boundaries for fail-fast bodies (whether the split finishReason shape of finding 1 is emitted for the targe…; "PR #8938 adds two defenses against upstream fail-fast…": did not run the new unit tests (review-only scope; no code changes made)., and 8 more.
Test Plan (not a blocker): 320 passed — this review observed 19816, 1285, 19448, 1539, 494, 3485, 559 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："PR #8938 adds two defenses against upstream fail-fast…"：did not run the new unit tests ( geminiChat.test.ts ) to confirm they pass.；"PR #8938 adds two defenses against upstream fail-fast…"：did not inspect useGeminiStream.ts / nonInteractiveCli.ts abort handling — finding 2's mechanism is verified via Turn.run only.；"PR #8938 adds two defenses against upstream fail-fast…"：did not enumerate anthropic/OpenAI converter chunk shapes to bound how often two finishReason chunks can precede the first yield (finding 1 trigger frequency).；"PR #8938 adds two defenses against upstream fail-fast…"：did not verify the OpenAI/compat transformer's chunk boundaries for fail-fast bodies (whether the split finishReason shape of finding 1 is emitted for the targe…；"PR #8938 adds two defenses against upstream fail-fast…"：did not run the new unit tests (review-only scope; no code changes made).，另有 8 条。
Test Plan（非阻断）：320 passed — this review observed 19816, 1285, 19448, 1539, 494, 3485, 559 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
|
Fixed the two remaining placeholder findings: continuation placeholder chunks are now deferred until validation, and split text-part placeholders are covered in curated-history tests. Verified: |
Collaborator
Author
|
Scope reset complete. The image-retention and fork-snapshot work has moved to #9423; this PR now contains only the degraded-placeholder defense (3 files, +341/-5). The replacement implementation handles split streams and transport cuts before content is yielded, preserves longer/structured responses, and keeps tool-result repair stable after curated-history removal. Local verification: 324 core tests, repository typecheck, and build all pass. @qwen-code /triage |
yiliang114
dismissed stale reviews from qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, and qwen-code-ci-bot
August 18, 2026 17:39
Dismissed after the scope reset. R12-1 and R12-3 belonged to the removed stream-processing implementation; R11-3 moved to #9423 and is covered there; R12-2 is replaced by the current pre-processing guard with split-stream and transport-cut regression tests. Current head e281211 has green required CI and is undergoing a fresh review.
yiliang114
dismissed stale reviews from qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, qwen-code-ci-bot, and qwen-code-ci-bot
August 18, 2026 18:09
Dismissed after the scope reset and current-head fix 74211a0. Placeholder hold/continuation findings target the removed implementation; image/fork findings moved to #9423; whitespace and curation cases are covered by the current pre-stream guard; R18-1 is fixed by carrying the resolved adjacentIdx through mutation and by the duplicate-result idempotence regression.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 20, not a blocker) — recorded, not requested in this round:
packages/core/src/core/geminiChat.ts:1218 — [probe] placeholder criterion (visible-text join + part predicate) duplicated across detection and curation paths; one-sided drift leaks the placeholder back into request historypackages/core/src/core/geminiChat.ts:1589 — [review] scan-phase doc contracts not updated for the new adjacentIdx semanticspackages/core/src/core/geminiChat.ts:1699 — [probe] applyRepair's placeholder-aware adjacentIdx never exercised with non-empty partsToInject (mutant survives the suite)packages/core/src/core/geminiChat.ts:1237 — [probe] end-of-stream flush of held proper-prefix chunks executed by no test (mutant survives the suite)packages/core/src/core/geminiChat.ts:1182 — [probe] thought-part exclusion in the placeholder predicate untested (mutant survives the suite)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 20 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
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.
Test Plan (not a blocker): 324 tests pass — this review observed 20509, 1578, 21843, 1597, 494 passed.
Deferred under the convergence posture (round 21, not a blocker) — recorded, not requested in this round:
packages/core/src/core/geminiChat.ts:1218 — [review] visible-text extraction duplicated across detection and curation paths; re-implements shared getResponseText helperpackages/core/src/core/geminiChat.ts:1237 — [review] strict-prefix clean-end flush executed by no test (behavior-equivalent mutant survives the suite)packages/core/src/core/geminiChat.ts:1218 — [review] multi-part text join untested (join-mutation survives the suite)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：324 tests pass — this review observed 20509, 1578, 21843, 1597, 494 passed。
收敛姿态下延后（第 21 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
yiliang114
enabled auto-merge
August 19, 2026 04:38
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定：
|
qwen-code-ci-bot
approved these changes
Aug 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
approved these changes
Aug 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Independent review — verified against HEAD f1e647a5
Reviewed the production logic and adjudicated every unresolved thread against the code as it currently stands. Verdict: APPROVE. The blocking Critical (R18-1) is fixed in this PR's own diff, and the remaining unresolved threads are stale anchors rather than live defects.
R18-1 — fixed by this diff (not merely "looks addressed")
Three unresolved threads still read isOutdated: false for R18-1 ("planRepair's placeholder-aware adjacency never reaches applyRepair; applyRepair still recomputes adjacentIdx = plan.modelIdx + 1"). Their last re-check targeted 7974050f; HEAD is now f1e647a5. Both suggested fixes are present in the current code, and both arrive as + lines in this PR's diff (not inherited from the merge):
RepairPlannow carriesadjacentIdxandplanRepairpopulates it fromscan.adjacentIdx.applyRepairreadsconst adjacentIdx = plan.adjacentIdx;— used for both the step-(2) empty-turn retention bound and the step-(3) insert position. Theplan.modelIdx + 1recomputation is gone.- The removal-only guard the threads asked for exists:
if (partsToInject.length === 0) return { insertedBefore: 0 };— placed after step (2) and before the step-(3) splice, so a duplicate-removal-only plan can no longer splice a phantom empty user turn.
The regression test the threads requested is also present, and it is the witness scenario verbatim — [user, model[fc call-1], model['(request timeout)'], user[fr call-1], user[fr call-1 dup]] driven through two repairOrphanedToolUseTurns() passes, asserting pass 1 is { injected: [], droppedDuplicates: [call-1] }, pass 2 is { injected: [], droppedDuplicates: [] } (the false-error-fr synthesis the Critical predicted), and that getHistory() keeps the placeholder with no empty turn spliced and no hoist. That closes the separate geminiChat.ts:1592 Suggestion too, which asked for exactly this pinning case in describe('repairOrphanedToolUseTurns').
Remaining unresolved threads — all stale
- R17-2 (
image-payload-references.ts, uncapped last-content exemption) and R19-3 (forkedAgent.ts, shallow-clone protection): bothisOutdated: trueand both point at files not in this PR's 3-file diff. They belong to the image-payload eviction work (#9423, which I reviewed and approved separately) and are not actionable here. - The whitespace-intolerance Critical (
isOutdated: true): addressed. Every placeholder comparison now normalizes —isDegradedPlaceholderTurnjoins non-thought text and compares.trim() === UPSTREAM_DEGRADED_PLACEHOLDER, the streaming prefix guard testsUPSTREAM_DEGRADED_PLACEHOLDER.startsWith(trimmed), and the terminal check istext.trim() === …. A placeholder completing across a transport cut without its boundary space is caught. - The rest are
isOutdated: truetest-coverage Suggestions.
Logic audit (independent pass, no new findings)
rejectDegradedPlaceholderResponsebuffers only while the accumulated text remains a prefix of the placeholder and flips topassthroughthe moment it diverges, on afunctionCall, or on a non-thought part with notext— so real content is never withheld past the point of divergence, and a never-completing prefix is still emitted via the trailingyield* pending.isDegradedPlaceholderTurnrequires every part to be thought-or-text with nofunctionCall, so aninlineData(image) part cannot be misclassified as a placeholder and silently dropped by curation.- Divergence in the safe direction: an unexpected variant such as a double-space
(request timeout)failsstartsWithand is delivered as ordinary text rather than falsely rejected. - Curation-side filtering (
extractCuratedHistory) drops placeholders request-side only, which is why durable history legitimately still carries them — consistent with the repair pass being placeholder-aware rather than placeholder-stripping.
State
CI green (13/13 pass, Test (ubuntu-latest, Node 22.x) included), MERGEABLE. Heads-up that this lands in core (packages/core/src/core/**), so it's maintainer-visible per the repo's core-infra policy; at +372/-13 with no refactor scope it isn't size-gated.
If any of the three R18-1 threads are still open only because the flag tracks an anchored line rather than the fix, they can be resolved.
Collaborator
|
Released in v0.21.15. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Rejects the known upstream fail-fast placeholder
(request timeout)when it is the entire visible model response. Candidate chunks are held only while they can still form that exact placeholder; a completed match enters the existing transient retry path without being yielded or persisted, while longer text, tool calls, structured payloads, and ordinary responses pass through unchanged. A transport failure after a partial placeholder prefix also retries without leaking the prefix to the user.Already-persisted placeholder-only model turns are excluded from curated request history without rewriting comprehensive history or transcript records. Tool-result repair treats an excluded placeholder between a tool call and its result as logically transparent.
The previous image-retention and fork-snapshot work has been removed from this PR and moved to #9423. The final diff is limited to 3 files with 372 additions and 13 deletions.
Why it's needed
Production traces showed an upstream endpoint returning HTTP 200 with a normal finish reason while the complete response body was only
(request timeout). Existing stream validation accepted and persisted it as a normal assistant reply, then replayed it in later requests. Because the failure is transient, routing this exact response through the existing bounded retry mechanism recovers without adding another retry subsystem.Reviewer Test Plan
How to verify
(request timeout)split across multiple chunks and confirm one retry occurs, only the recovered response is emitted, and the placeholder is absent from history.cd packages/core && npx vitest run src/core/geminiChat.test.ts; 324 tests pass locally.Evidence (Before & After)
N/A (non-UI runtime reliability fix).
Tested on
Environment (optional)
Node.js 22 workspace with package unit tests and repository build/typecheck gates.
Risk & Scope
Linked Issues
Fixes #8916
中文说明
本 PR 做了什么
当已知上游 fail-fast 占位符
(request timeout)构成完整的模型可见响应时拒绝该响应。只有仍可能组成这一精确占位符的候选 chunk 会被暂存；完整匹配后进入现有瞬时重试路径，不向用户输出也不持久化。更长文本、工具调用、结构化 payload 和普通响应保持原样通过。若传输在占位符前缀后中断，也会在不泄漏前缀的情况下重试。已持久化的纯占位符 model turn 会从 curated 请求历史中排除，但不会改写全量历史或 transcript 记录。若工具调用与工具结果之间夹有被排除的占位符，工具结果修复会把它视为逻辑透明。
此前的图片保留和 fork 快照改动已从本 PR 移除并迁移到 #9423。最终 diff 仅涉及 3 个文件，新增 372 行、删除 13 行。
为什么需要
生产 trace 显示，上游端点会返回 HTTP 200 和正常 finish reason，但完整正文只有
(request timeout)。现有流校验会把它当作正常 assistant 回复接受并持久化，后续请求还会继续回放。该失败具有瞬时性，因此让这一精确响应进入现有有界重试机制即可恢复，无需再增加一套重试系统。评审测试计划
如何验证
(request timeout)拆成多个 chunk 返回，确认发生一次重试，仅输出恢复后的响应，历史中不含占位符。cd packages/core && npx vitest run src/core/geminiChat.test.ts；本地 324 个测试通过。前后对比证据
N/A（非 UI 的运行时可靠性修复）。
测试环境
macOS ✅；Windows / Linux 未本地测试，交由 CI 验证。
环境说明
Node.js 22 workspace，运行 package 单元测试及仓库 build/typecheck 门禁。
风险与范围
关联 Issue
Fixes #8916