vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-09-03 06:44:35 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 12

refactor(core): consolidate partial-tool_use repair docs into one design block (PR #4176)

Browse source 
Addresses the pomelo-nwu review observation on c30bba6e7:
~60% of the +918 lines added to geminiChat.ts were comments, with
several 20–40 line prose blocks repeating the same race-class /
tool_use_id wedge analysis at every call site. Net –139 / +117
(net delete ~22 lines but mostly redistributing weight from per-site
blocks to a single canonical note).

Changes:

* Add one canonical design block above ORPHAN_TOOL_USE_REPAIR_REASON
  covering: the tool_use_id 400 wedge, the three race classes (A
  Ctrl+Y mid-flight, B process crash, C SSE drop), the two-layer fix
  (partial-push + repair), and the partial-push marker lifecycle.
  Every per-site comment that used to repeat this now points back.

* Trim popPartialIfPushed COMPRESSED-branch comment from 14 lines to
  4 (no-op-today + reason for keeping).

* Trim recovery-catch comment block from ~38 lines to ~7 (pop-order
  matters + index-checked, pointer to canonical note).

* Trim addHistory invariant comment from ~22 lines to ~5 (when this
  fires + why error-level, pointer to canonical note).

* Trim processStreamResponse partial-push comment from ~22 lines to
  ~8 (Race C name + signal + plain-text-not-persisted rule).

* Replace tool_use_id wedge re-explanations at 5 sites (repair doc,
  scanModelTurn doc, applyRepair doc, popPartialIfPushed comment,
  processStreamResponse comment) with single-line pointers. Canonical
  block at line 411 is now the only authoritative copy.

Reviewer marked this as non-blocking ("shouldn't gate the merge");
done as cleanup polish while LGTM is in.

Tests: 375/375 (no test changes). tsc + eslint + prettier clean.
This commit is contained in:
高铁 2026-05-21 12:22:41 +08:00
parent a8ac579af3
commit 298588190d
1 changed files with 117 additions and 139 deletions
Download patch file Download diff file Expand all files Collapse all files

256
packages/core/src/core/geminiChat.ts
View file

@ -397,6 +397,75 @@ const ORPHAN_TOOL_USE_REPAIR_REASON =
'Tool execution result was not recorded — likely interrupted by network ' +
'failure, abort, or process exit. Treat as failure and retry if needed.';
/*
* ============================================================================
* Partial-tool_use repair subsystem — canonical design note.
* ============================================================================
*
* Every comment block elsewhere in this file that mentions one of the
* concepts below points back here. Per-site comments should be one or two
* lines stating WHAT the local code does; the WHY lives here.
*
* --- The wedge ----------------------------------------------------------
*
* Anthropic-compatible backends (Anthropic, DeepSeek, …) reject a request
* whose `user[tool_result]` blocks are not at the HEAD of the user message
* immediately following the `model[tool_use]` they answer:
*
* "tool_use_id ... must have a corresponding tool_use block in the
* previous message"
*
* Without a matching pair the session is unrecoverable — `stripOrphanedUser
* EntriesFromHistory` only strips trailing user entries, so a lost tool_use
* cannot be resurrected and the next send 400s repeatedly.
*
* --- The race classes that produce dangling tool_uses --------------------
*
* Race A (Ctrl+Y mid-flight): user retries before the in-flight tool
* finishes. The scheduler's `onAllToolCallsComplete` is single-shot
* per batch and would otherwise leave the tool stuck in
* `completed-but-not-submitted` forever.
* Race B (process crash / OOM mid-flight): the JSONL transcript captures
* the dangling `model[fc]` and `--resume` rehydrates it.
* Race C (network drop between `content_block_stop` of a tool_use and
* the terminal `message_stop`): `processStreamResponse` re-throws
* after we have already yielded a `functionCall` chunk, so the React
* scheduler is on its way to submit a real `functionResponse` while
* in-memory history has no matching `model[fc]`.
*
* --- The two-layer fix ---------------------------------------------------
*
* (1) Persist the partial assistant turn at the failure point in
* `processStreamResponse` (`this.history.push({role: 'model', parts:
* [...]})` plus the `pendingPartialAssistantTurnIndex` /
* `pendingPartialAssistantRecord` markers) so the matching
* `model[fc]` is on disk and in memory when the late `user[fr]`
* arrives.
* (2) Repair any remaining dangling `model[fc]` whose
* `user[fr]` never landed (`repairOrphanedToolUseTurns`):
* - SYNTHESIZE an `error` fr for ids with no matching response;
* - HOIST the real fr into the immediately-adjacent user turn
* when it landed in a non-adjacent later turn;
* - DROP duplicate fr copies for the same id.
* Then `useGeminiStream.handleCompletedTools` dedupes the
* scheduler's late real result against `chat.history` so the
* synthetic and the real result never collide on the wire.
*
* --- Partial-push marker lifecycle ---------------------------------------
*
* Set together on (streamError + hasToolCall + hasContent) inside
* `processStreamResponse`. Cleared together by `popPartialIfPushed` on a
* retryable error rollback, or flushed together to JSONL by the outer
* `finally` after the retry loop exits. Defense-in-depth: every
* history-mutation method (clearHistory / addHistory / setHistory /
* truncateHistory / stripThoughtsFromHistory /
* stripOrphanedUserEntriesFromHistory) resets both markers in lockstep so
* a stale index can't shift onto an unrelated model turn and cause
* `popPartialIfPushed` to splice the wrong entry. Any single-field reset
* is a bug.
* ============================================================================
*/
/**
* Walk `history` left-to-right and close every dangling tool_use ↔ tool_result
* pair so the wire format the next API call sees is always
@ -410,15 +479,9 @@ const ORPHAN_TOOL_USE_REPAIR_REASON =
* `yieldMissingToolResultBlocks` (`query.ts:123-149`).
* - HOIST: for any `functionCall.id` whose real `functionResponse` lives in
* a non-adjacent following user turn (typical shape:
* `model[fc], user[text], user[fr_real]` — produced when a user aborts a
* long-running tool, types a follow-up, and the React scheduler's late
* `submitQuery` appends the real `fr` as a SEPARATE user entry), MOVE the
* real `fr` part out of its original turn into the adjacent one. Without
* hoisting, the synthesis pass correctly skips the call (a real `fr`
* exists somewhere later) but the wire layout still serializes
* `model[tool_use] → user[text] → user[tool_result]`, which
* Anthropic-compatible backends reject with "tool_use_id ... must have a
* corresponding tool_use block in the previous message".
* `model[fc], user[text], user[fr_real]`), MOVE the real `fr` part out
* of its original turn into the adjacent one. Required by the wire
* layout — see the canonical design note above for the wedge.
*
* Mutates `history` in place and returns the set of injected `(callId, name)`
* tuples so callers (the React tool scheduler) can dedupe a real `tool_result`
@ -492,16 +555,10 @@ interface RepairPlan {
* follows. Pure read; no mutation.
*
* Storing ALL locations (not just the first) is load-bearing for the
* duplicate case: if the same callId is echoed back more than once
* across the consecutive user turns (e.g.
* `model[fc id=cid], user[text], user[fr cid], user[fr cid]` — possible
* when the React scheduler retries the late `submitQuery` and a
* duplicate fr lands), hoisting only the first would leave the
* duplicate behind. The wire payload then serializes
* `model[tool_use] -> user[tool_result] -> user[tool_result]`
* and the backend rejects the trailing block as an orphan
* ("tool_use_id ... must have a corresponding tool_use block in the
* previous message").
* duplicate case: e.g. `model[fc id=cid], user[text], user[fr cid],
* user[fr cid]` (the React scheduler retries the late `submitQuery`).
* The downstream decision phase needs every copy so it can drop
* duplicates — otherwise the wire payload re-triggers the wedge.
*/
function scanModelTurn(history: Content[], modelIdx: number): ScanResult {
const expected = new Map<string, string>();
@ -604,16 +661,10 @@ function planRepair(scan: ScanResult): RepairPlan {
* user turn (before any non-fr parts) OR insert a new user turn
* between `modelIdx` and whatever follows.
*
* Anthropic-compatible backends require the tool_result blocks at the
* head of the immediately following user message; appending instead
* (`[text, fr]`) re-triggers the 400 the synthesis pass exists to
* escape. Mirrors upstream Claude Code's `hoistToolResults`.
*
* CONSEQUENCE OF REMOVAL of the head-insert: dropping this hoist (e.g.
* naively `next.parts = [...existing, ...partsToInject]`) re-introduces
* the "tool_use_id ... must have a corresponding tool_use block in the
* previous message" 400 the synthesis pass exists to prevent. Do not
* "simplify" this branch.
* Step 3 inserts at the HEAD (before non-fr parts), not the tail —
* mirrors upstream Claude Code's `hoistToolResults`. See the canonical
* design note above `ORPHAN_TOOL_USE_REPAIR_REASON` for why a tail
* append re-triggers the 400. Do not "simplify" the head-insert away.
*/
function applyRepair(
history: Content[],
@ -1172,15 +1223,13 @@ export class GeminiChat {
lastError = error;
// If `processStreamResponse` persisted a partial assistant turn
// (mid-stream error after a `functionCall` was already yielded),
// every retry-and-continue path below must drop that turn first.
// Otherwise a successful retry's response lands AFTER the stale
// failed-attempt model turn — two consecutive `model` entries
// with an orphan tool_use in the first, re-triggering the
// "tool_use_id ... corresponding tool_use" 400 this fix is
// supposed to escape. Paths that `break` (unretryable) keep
// the partial — the caller will see it as part of the error
// surface.
// (mid-stream error after a `functionCall` was already
// yielded), every retry-and-continue path below must drop
// that turn first; otherwise the retry's response lands as
// a second consecutive model turn with an orphan tool_use
// (the wedge — see the canonical note above
// `ORPHAN_TOOL_USE_REPAIR_REASON`). Paths that `break`
// (unretryable) keep the partial.
const popPartialIfPushed = () => {
const idx = self.pendingPartialAssistantTurnIndex;
if (idx === null) return;
@ -1302,21 +1351,10 @@ export class GeminiChat {
reactiveInfo.compressionStatus ===
CompressionStatus.COMPRESSED
) {
// Defense-in-depth no-op: tryCompress() succeeded
// means it has already replaced this.history via
// setHistory(), which calls clearPendingPartialState()
// — so by the time we reach this line, the marker is
// null and popPartialIfPushed splices nothing. We
// keep the call as a uniformity assertion against
// future refactors that might switch tryCompress to
// an in-place mutation: in that world, the marker
// would NOT be reset by setHistory and this call
// becomes the only thing that drops the stale
// partial before requestContents is rebuilt below.
// Removing it would couple correctness to the
// implementation detail "setHistory always clears
// the marker", which the other retry branches don't
// share.
// No-op today: tryCompress's setHistory has already
// cleared the marker. Kept for uniformity with the
// other retry branches in case a future in-place
// tryCompress stops resetting it.
popPartialIfPushed();
requestContents = self.getRequestHistory();
debugLogger.info(
@ -1552,47 +1590,13 @@ export class GeminiChat {
// coalesced back into the preceding model entry after the loop.
successfulRecoveries++;
} catch (recoveryError) {
// If a recovery attempt fails (e.g., empty response, network
// error), stop recovering and let the partial output stand.
// Pop the dangling recovery message to keep history valid.
//
// Order matters: when the recovery stream errors AFTER
// yielding a `functionCall` chunk, `processStreamResponse`
// pushes a partial `model` turn into history before
// re-throwing. The naive "if last is user, pop" check
// would then no-op (last is now the partial `model`),
// leaving `user(OUTPUT_RECOVERY_MESSAGE)` stranded as a
// real user turn the user never sent. Two consequences:
// - the control-prompt text (which carries instructions
// meant only for the model's own continuation context)
// pollutes durable history and biases later turns,
// - the inline repair on the next sendMessageStream
// synthesizes an `error` `functionResponse` for the
// dangling `functionCall`, which the
// `handleCompletedTools` history-based dedup then drops
// when the React scheduler's REAL tool result arrives,
// so the model sees an "execution result was not
// recorded" error for a tool that actually succeeded.
// Pop the partial model turn FIRST, then the recovery
// user turn. The partial-push markers are also cleared
// in lockstep so the outer `finally` JSONL flush can't
// resurrect a partial we just deleted from live history.
//
// Index-checked pop instead of a positional `pop()` so
// we match the diagnostic standard set by
// `popPartialIfPushed` above (splice at `idx` + warn on
// bounds/role mismatch). The two rollback strategies
// share an undocumented positional assumption: nothing
// mutates `this.history` between
// `processStreamResponse`'s push and the for-await
// catch here. If a future change inserts a mutation in
// that window (compression side-effect, abort-signal
// handler, telemetry hook), a naked
// `history.pop()` would silently remove the wrong
// entry while `clearPendingPartialState()` clears
// markers for the actual partial — leaving it
// permanently stranded with no log trail. The warn
// makes any future violation visible immediately.
// Pop the partial `model[fc]` FIRST (if processStreamResponse
// pushed one before re-throwing), THEN the recovery user
// turn. Reversed order would strand `OUTPUT_RECOVERY_MESSAGE`
// as a real user turn. Index-checked pop mirrors
// `popPartialIfPushed` above — see the design note above
// `ORPHAN_TOOL_USE_REPAIR_REASON` for the wedge mechanism
// and the partial-push marker lifecycle.
const expectedIdx = self.pendingPartialAssistantTurnIndex;
const lastIdx = self.history.length - 1;
if (
@ -1936,34 +1940,18 @@ export class GeminiChat {
*/
addHistory(content: Content): void {
this.history.push(content);
// The marker is per-send-attempt. Today's callers (cancelled-tool
// synthesis in useGeminiStream, ACP session injects,
// shellCommandProcessor) only run between sends, so the originating
// sendMessageStream has either already popped the partial via the
// retry loop or hit an unrecoverable break — in both cases the
// marker is no longer load-bearing.
//
// If a future code path ever calls addHistory BETWEEN the partial
// push and the retry attempt, silently clearing the marker would
// strand the partial: popPartialIfPushed would no-op, the failed
// attempt's `model[functionCall]` would survive into the retry,
// and a successful retry's response would land as a SECOND
// consecutive model turn (the wedge this whole subsystem exists
// to prevent). The log below makes that coupling observable —
// anyone investigating a stale-partial bug will see this log line
// pointing straight at the offending caller. Error-level (not
// warn) because this is a true invariant violation: the existing
// call graph cannot legitimately hit this branch, so any
// occurrence is a real bug in a future caller, not noise.
// addHistory only runs between sends, so the partial-push marker
// should already be cleared. If it is not, a new caller is
// violating that invariant — surface it at error level so the
// offending stack is visible. See the design note above
// `ORPHAN_TOOL_USE_REPAIR_REASON` for the marker lifecycle.
if (
this.pendingPartialAssistantTurnIndex !== null ||
this.pendingPartialAssistantRecord !== null
) {
debugLogger.error(
'[INVARIANT_VIOLATION] addHistory called while a partial-push ' +
'marker is active — clearing it. This is unexpected during an active sendMessageStream ' +
'and likely indicates a new caller violating the between-sends ' +
'invariant. See comment at GeminiChat.addHistory for context.',
'marker is active — clearing it.',
);
}
this.clearPendingPartialState();
@ -2251,30 +2239,20 @@ export class GeminiChat {
}
}
// Mid-stream failure recovery: if the upstream stream threw (typical on
// weak networks — SSE cut between a tool_use `content_block_stop` and
// the terminal `message_stop`) AND any `functionCall` chunk was already
// yielded to consumers, we must persist the partial assistant turn here.
// Mid-stream failure recovery (Race C in the canonical note above
// `ORPHAN_TOOL_USE_REPAIR_REASON`): if the upstream stream threw
// AFTER a `functionCall` chunk was already yielded — typical on
// weak networks: SSE cut between a tool_use `content_block_stop`
// and the terminal `message_stop` — we persist the partial
// assistant turn so the React scheduler's incoming
// `user[functionResponse]` has a matching `model[tool_use]` to
// pair with.
//
// The content generator (Anthropic / OpenAI) emits a `functionCall` part
// only at the end of a tool_use block. Once yielded, `Turn.run` registers
// a `ToolCallRequest` event, the React tool scheduler queues the call,
// and `handleCompletedTools` will fire `submitQuery(..., ToolResult)` —
// pushing a user message with `functionResponse` into history — even
// though the parent stream errored. Without preserving the matching
// tool_use on the model side, the next request body would have
// `user → user[tool_result]` with no tool_use in between, and the
// Anthropic-compatible API (DeepSeek, Anthropic, etc.) rejects with
// "tool_use_id ... must have a corresponding tool_use block in the
// previous message"
// — an unrecoverable state because Ctrl+Y's `stripOrphanedUserEntries`
// only strips trailing user entries; the lost tool_use can't be
// resurrected.
//
// Plain-text partial turns (no functionCall yielded) are deliberately
// NOT persisted — the Retry path pops the trailing user prompt and
// re-issues it; a stale partial-text model turn between them would
// either bias the retry or surface as a duplicate.
// Plain-text partial turns (no functionCall yielded) are
// deliberately NOT persisted — the Retry path pops the trailing
// user prompt and re-issues it; a stale partial-text model turn
// between them would either bias the retry or surface as a
// duplicate.
if (streamError !== null) {
// Reuse the `willPersistToHistory` gate from the recordAssistantTurn
// block above instead of re-deriving it. When `streamError !== null`,