vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-09-01 02:36:10 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 22

fix(core,cli): close 4 deepseek-v4-pro review threads on PR #4176

Browse source 
Four follow-ups from the deepseek-v4-pro pass on commit 2dbfc4e3b:

[A] useGeminiStream.ts — dedup recordCompletedToolCall now skips
cancelled tools. `dedupedTools` includes anything in a terminal state
(success | error | cancelled), but cancelled means the tool never
actually produced model-visible output. Counting it via
`recordCompletedToolCall` would inflate `toolCallCount` and could flip
`skillsModifiedInSession` for a never-executed skill-write. The
markToolsAsSubmitted call still fires so the scheduler unblocks.
Mirrors the rationale of the existing `allToolsCancelled` branch which
surfaces non-deduped cancellations via addHistory + reportCancelled
rather than the completed-call metric.

[B] client.ts — JSDoc on `repairOrphanedToolUseTurnsInHistory` claimed
three call points (startChat, Retry submit path, defensive
UserQuery/Cron pass) but in practice this method is only called once
from `startChat()`. The other two coverage points live one layer down
inside `GeminiChat.sendMessageStream` and call the standalone
`repairOrphanedToolUseTurns(history)` function directly without
routing through this wrapper. Updated to reflect the actual coupling.

[C] geminiChat.ts — `addHistory` now warns via debugLogger if it
clears a non-null partial-push marker. Today's callers
(useGeminiStream cancelled-tool synthesis, ACP session injects,
shellCommandProcessor) only run between sends, so the marker is null
on entry. If a future code path calls addHistory between the partial
push and the retry attempt, the silent clear would strand the
partial: popPartialIfPushed would no-op, the failed 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 warn surfaces the offending caller in
the log instead of forcing a blind trace through the marker lifecycle.

[D] geminiChat.ts — extracted `clearPendingPartialState()` helper
covering all 7 sites that need to reset both
`pendingPartialAssistantTurnIndex` and `pendingPartialAssistantRecord`
in lockstep (sendMessageStream entry, popPartialIfPushed, the
post-loop flush hook, clearHistory, addHistory, setHistory,
truncateHistory, stripThoughtsFromHistory). The two fields ARE always
paired by lifecycle (set together on stream-error stash, popped
together on retry, flushed together at the rethrow site), so any
single-field reset would be a bug. The helper makes the lockstep
invariant explicit and means a future history-mutating method can't
forget to clear one field.

Regression test: useGeminiStream.test.tsx adds
"skips recordCompletedToolCall for deduped CANCELLED tools" — sets up
a deduped cancelled tool with callId paired in history, asserts
markToolsAsSubmitted IS called (scheduler unblocks) but
recordCompletedToolCall is NOT called. Verified the test fails when
the cancelled-filter line is removed (regression-injection check).

Tests: 237/237 core, 95/95 useGeminiStream (+1 new). tsc + eslint +
prettier clean. Existing CI failure on Test (ubuntu-latest, Node 22.x)
is an unrelated timing flake in promptHookRunner.test.ts
("expected 49 to be greater than or equal to 50") — macOS and Windows
both pass.
This commit is contained in:
高铁 2026-05-19 00:24:18 +08:00
parent 2dbfc4e3bb
commit fd12639c9c
4 changed files with 220 additions and 55 deletions
Download patch file Download diff file Expand all files Collapse all files

123
packages/cli/src/ui/hooks/useGeminiStream.test.tsx
View file

@ -855,6 +855,129 @@ describe('useGeminiStream', () => {
expect(mockSendMessageStream).not.toHaveBeenCalled();
});
it('skips recordCompletedToolCall for deduped CANCELLED tools (telemetry parity)', async () => {
// A deduped tool with status='cancelled' never actually produced
// model-visible output — counting it via `recordCompletedToolCall`
// (which increments toolCallCount and can flip
// skillsModifiedInSession on a skill-write path) would inflate the
// metric for a call that never ran end-to-end. This test repros
// the deepseek-v4-pro thread on PR #4176: dedup must skip BOTH
// client-initiated (already skipped) AND cancelled tools, while
// still calling `markToolsAsSubmitted` so the scheduler unblocks.
const cancelledDedupedTool = {
request: {
callId: 'call_dedup_cancelled',
name: 'write_file',
args: { path: '/tmp/cancelled.txt', content: 'x' },
isClientInitiated: false,
prompt_id: 'prompt-dedup-cancel',
},
status: 'cancelled',
responseSubmittedToGemini: false,
response: {
callId: 'call_dedup_cancelled',
responseParts: [
{
functionResponse: {
id: 'call_dedup_cancelled',
name: 'write_file',
response: { error: 'cancelled' },
},
},
],
resultDisplay: undefined,
error: undefined,
errorType: undefined,
},
tool: {
name: 'write_file',
displayName: 'WriteFile',
description: 'Write a file',
build: vi.fn(),
} as any,
invocation: {
getDescription: () => 'cancelled write',
} as unknown as AnyToolInvocation,
} as unknown as TrackedCancelledToolCall;
const client = new MockedGeminiClientClass(mockConfig);
// Pre-paired in history: dedup will fire for this callId.
client.getHistory = vi.fn().mockReturnValue([
{ role: 'user', parts: [{ text: 'cancelled write' }] },
{
role: 'model',
parts: [
{
functionCall: {
id: 'call_dedup_cancelled',
name: 'write_file',
args: { path: '/tmp/cancelled.txt', content: 'x' },
},
},
],
},
{
role: 'user',
parts: [
{
functionResponse: {
id: 'call_dedup_cancelled',
name: 'write_file',
response: { error: 'synthetic' },
},
},
],
},
]);
let capturedOnComplete:
| ((completedTools: TrackedToolCall[]) => Promise<void>)
| null = null;
mockUseReactToolScheduler.mockImplementation((onComplete) => {
capturedOnComplete = onComplete;
return [[], mockScheduleToolCalls, mockMarkToolsAsSubmitted];
});
renderHook(() =>
useGeminiStream(
client,
[],
mockAddItem,
mockConfig,
mockLoadedSettings,
mockOnDebugMessage,
mockHandleSlashCommand,
false,
() => 'vscode' as EditorType,
() => {},
() => Promise.resolve(),
false,
() => {},
() => {},
() => {},
() => {},
80,
24,
),
);
await act(async () => {
if (capturedOnComplete) {
await capturedOnComplete([cancelledDedupedTool]);
}
});
// Scheduler still gets unblocked.
await waitFor(() => {
expect(mockMarkToolsAsSubmitted).toHaveBeenCalledWith([
'call_dedup_cancelled',
]);
});
// Telemetry NOT incremented — the cancelled filter held.
expect(client.recordCompletedToolCall).not.toHaveBeenCalled();
});
it('runs Race A dedup BEFORE the isResponding early-return (regression guard)', async () => {
// The dedup block in handleCompletedTools is intentionally placed
// ABOVE the `if (isResponding) return;` early-return: the scheduler's

12
packages/cli/src/ui/hooks/useGeminiStream.ts
View file

@ -2077,8 +2077,20 @@ export const useGeminiStream = (
// `recordCompletedToolCall` loop below over `geminiTools` —
// filter to the same shape (non-client-initiated) so client
// tools (which the original loop also skipped) stay skipped.
//
// Cancelled tools are also skipped: `dedupedTools` includes
// anything in a terminal state (success | error | cancelled),
// but cancelled means the tool never actually ran end-to-end —
// the `allToolsCancelled` branch below would have surfaced
// them via `addHistory + reportCancelled` rather than the
// completed-call metric, and the metric should match. Without
// this filter, a deduped + cancelled tool would inflate
// `toolCallCount` for a call that never produced a result
// (and could also flip `skillsModifiedInSession` for a
// never-executed skill-write).
for (const tc of dedupedTools) {
if (tc.request.isClientInitiated) continue;
if (tc.status === 'cancelled') continue;
geminiClient?.recordCompletedToolCall(
tc.request.name,
tc.request.args as Record<string, unknown>,

23
packages/core/src/core/client.ts
View file

@ -359,16 +359,19 @@ export class GeminiClient {
* {@link stripOrphanedUserEntriesFromHistory}, which only handles trailing
* `user` entries.
*
* Called from three points:
* 1. After {@link startChat} loads transcript (covers `--resume` of a
* session that crashed between partial-tool_use push and tool
* completion).
* 2. After `stripOrphanedUserEntriesFromHistory` on the Retry submit path
* (covers Ctrl+Y race — user retries while an in-flight tool's
* `tool_result` has not yet been submitted, leaving a trailing
* `model[functionCall]` without matching `functionResponse`).
* 3. Defensively at the start of UserQuery / Cron sends, so any state
* that slipped past 1+2 still gets fixed before hitting the wire.
* This `GeminiClient` method is the resume-path entry point — called once
* from {@link startChat} after the transcript loads, covering `--resume`
* of a session that crashed between a partial-tool_use push and the
* tool's eventual completion.
*
* The other two coverage points (Retry submit path after
* `stripOrphanedUserEntriesFromHistory`, and the defensive pass at the
* start of every UserQuery / Cron send) live one layer down inside
* `GeminiChat.sendMessageStream` and call the standalone
* `repairOrphanedToolUseTurns(history)` function directly — they don't
* route through this wrapper. Anyone tracing the repair-pass coupling
* between the client and chat layers should follow that path
* separately rather than expect everything to funnel through here.
*
* Synthesizes an `error` `functionResponse`. The React tool scheduler
* (`useGeminiStream.handleCompletedTools`) MUST dedupe by `callId` against

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

@ -609,6 +609,21 @@ export class GeminiChat {
| Parameters<ChatRecordingService['recordAssistantTurn']>[0]
| null = null;
/**
* Reset both partial-push markers in lockstep. Extracted so the seven
* call sites that need to drop both fields (sendMessageStream entry,
* popPartialIfPushed, clearHistory, addHistory, setHistory,
* truncateHistory, stripThoughtsFromHistory) can't drift apart — a
* future history-mutating method that only clears one would leak the
* other into a later flush. The fields ARE always paired by lifecycle
* (set together on stream-error stash, popped together on retry, flushed
* together at the rethrow site), so any single-field reset is a bug.
*/
private clearPendingPartialState(): void {
this.pendingPartialAssistantTurnIndex = null;
this.pendingPartialAssistantRecord = null;
}
/**
* Heap-pressure compaction is process-wide pressure applied per chat. If one
* heap-triggered attempt cannot reduce history, briefly back off this chat
@ -857,8 +872,7 @@ export class GeminiChat {
// leftover from a prior unretryable break would otherwise get
// appended to JSONL by THIS send's retry-loop flush, attaching
// someone else's failed turn to this conversation.
this.pendingPartialAssistantTurnIndex = null;
this.pendingPartialAssistantRecord = null;
this.clearPendingPartialState();
let compressionInfo: ChatCompressionInfo;
let requestContents: Content[];
@ -1004,14 +1018,13 @@ export class GeminiChat {
) {
self.history.splice(idx, 1);
}
self.pendingPartialAssistantTurnIndex = null;
// Discard the deferred chat-recording record alongside the
// in-memory pop so the JSONL transcript also drops the
// failed attempt. (Paired with the stash in
// processStreamResponse — see the field-level comment on
// `pendingPartialAssistantRecord` for the failure mode this
// fixes.)
self.pendingPartialAssistantRecord = null;
// Drop both markers in lockstep — the deferred chat-
// recording record must be discarded alongside the
// in-memory splice so the JSONL transcript also drops the
// failed attempt. See the field-level comment on
// `pendingPartialAssistantRecord` for the failure mode
// this prevents.
self.clearPendingPartialState();
};
// Handle rate-limit / throttling errors returned as stream content.
@ -1221,7 +1234,13 @@ export class GeminiChat {
self.chatRecordingService?.recordAssistantTurn(
self.pendingPartialAssistantRecord,
);
self.pendingPartialAssistantRecord = null;
// Clear both fields in lockstep. The marker is no longer
// load-bearing past this point (its consumer is the for-loop
// catch above, which has exited), and the next
// sendMessageStream entry would clear it anyway — but pairing
// the reset preserves the "marker and stash are always set or
// cleared together" invariant the helper enforces.
self.clearPendingPartialState();
}
// Max output tokens escalation: if the retry loop succeeded with
@ -1509,16 +1528,15 @@ export class GeminiChat {
*/
clearHistory(): void {
this.history = [];
// Any pending partial-push marker points into the now-empty history;
// Any pending partial-push state points into the now-empty history;
// resetting prevents `popPartialIfPushed` from splicing whatever
// shows up at that index in a future send (defense-in-depth — the
// helper also bounds-checks, but a stale marker that happens to
// line up with a real model turn could otherwise pop the wrong
// entry). Drop the deferred-record stash for the same reason: a
// later flush would otherwise append a turn that doesn't match
// the (now-empty) live history.
this.pendingPartialAssistantTurnIndex = null;
this.pendingPartialAssistantRecord = null;
// entry). The deferred-record stash is dropped for the same reason:
// a later flush would append a turn that doesn't match the (now-
// empty) live history.
this.clearPendingPartialState();
}
/**
@ -1526,22 +1544,35 @@ export class GeminiChat {
*/
addHistory(content: Content): void {
this.history.push(content);
// The marker is per-send-attempt. By the time external code calls
// addHistory (cancelled-tool synthesis in useGeminiStream, ACP
// session injects, shellCommandProcessor, etc.), the originating
// 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. Clearing here matches the
// defensive reset already applied in setHistory/truncateHistory/
// clearHistory: any future addHistory variant that splices into
// the middle (instead of plain push) would shift indices and a
// stale marker could splice the wrong entry. Belt-and-suspenders;
// the next sendMessageStream entry also clears it. The
// deferred-record stash is paired with the marker — keeping it
// around past an external addHistory could let a subsequent
// retry-loop flush land a stale partial in JSONL.
this.pendingPartialAssistantTurnIndex = null;
this.pendingPartialAssistantRecord = null;
// 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 warn below makes that coupling observable —
// anyone investigating a stale-partial bug will see this log line
// pointing straight at the offending caller, instead of having to
// trace through the marker lifecycle blind.
if (
this.pendingPartialAssistantTurnIndex !== null ||
this.pendingPartialAssistantRecord !== null
) {
debugLogger.warn(
'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.',
);
}
this.clearPendingPartialState();
}
setHistory(history: Content[]): void {
@ -1553,21 +1584,18 @@ export class GeminiChat {
// splice an entry that has nothing to do with the original partial
// push, corrupting the conversation. Drop the paired deferred-record
// stash too: its referent (the model turn at the old index) is gone.
this.pendingPartialAssistantTurnIndex = null;
this.pendingPartialAssistantRecord = null;
this.clearPendingPartialState();
}
truncateHistory(keepCount: number): void {
this.history = this.history.slice(0, keepCount);
// Truncation can drop the entry the partial-push marker points at,
// or leave it valid but shift the meaning of nearby indices. Reset
// the marker rather than try to fix it up — it's per-send and
// ephemeral, so losing it across a truncate is safe (the
// sendMessageStream that pushed it has already finished or will
// start fresh on the next call). Drop the paired deferred-record
// stash for the same reason.
this.pendingPartialAssistantTurnIndex = null;
this.pendingPartialAssistantRecord = null;
// both fields rather than try to fix them up — they're per-send and
// ephemeral, so losing them across a truncate is safe (the
// sendMessageStream that pushed them has already finished or will
// start fresh on the next call).
this.clearPendingPartialState();
}
stripThoughtsFromHistory(): void {
@ -1576,11 +1604,10 @@ export class GeminiChat {
.filter((content): content is Content => content !== null);
// Filter+map replaces `this.history` with a new array, so any pending
// partial-push marker is now indexed against an array that no longer
// exists. Clear it for the same reason setHistory does. The deferred
// chat-recording stash is paired with the marker — drop it too so a
// later flush can't land a turn that doesn't exist in live history.
this.pendingPartialAssistantTurnIndex = null;
this.pendingPartialAssistantRecord = null;
// exists. Clear it for the same reason setHistory does — and drop
// the paired deferred-record stash so a later flush can't land a
// turn that doesn't exist in live history.
this.clearPendingPartialState();
}
/**