fix(core,cli): close 3 review threads on partial-tool_use repair (PR #4176)

Three Suggestion threads from the qwen-latest-series-invite-beta-v34
pass on commit c8fa3143f.

[A] DIAGNOSTICS — `repairOrphanedToolUseTurns` return type now
carries a `droppedDuplicates: Array<{ callId; name }>` field
alongside `injected`. Previously a duplicate-only repair (no
synthesis, no hoist — the main scenario the prior commit added)
returned `injected.length === 0` and both call sites
(`repairOrphanedToolUseTurnsInHistory` in client.ts,
`sendMessageStream` inline in geminiChat.ts) only logged on
synthesis, leaving zero diagnostic trail. If a future callId
collision bug caused the wrong `functionResponse` to be dropped,
there was no breadcrumb pointing back to the repair function. Both
call sites now emit a `[REPAIR] Dropped N duplicate functionResponse(s)`
warn line tagged with their origin (session-load vs inline per-send)
so investigators can anchor to the exact pass.

[B] DEFENSIVE INVARIANT — max-tokens recovery catch now uses an
index-checked pop instead of a positional `history.pop()`. The
semantically equivalent `popPartialIfPushed` uses
`splice(idx, 1)` with an explicit bounds/role check and a
`debugLogger.warn` on mismatch; the recovery catch was the only
rollback site that pop'd whatever model entry happened to be last,
with zero diagnostic output. Today the invariant holds (nothing
mutates `this.history` between `processStreamResponse`'s push and
the for-await catch), but a future change that inserts a mutation
in that window — compression side-effect, abort-signal handler,
telemetry hook — would silently pop the wrong entry while
`clearPendingPartialState()` cleared markers for the actual
partial, leaving it permanently stranded. The new
`[RECOVERY_POP] Marker/last-index mismatch` warn surfaces any
future violation immediately.

[C] TEST COVERAGE — added 6 unit tests for
`GeminiChat.getHistoryFunctionResponseIds()` (empty, mixed
user/model, model[fc] ignored, duplicate collapse to Set,
malformed parts, no aliasing of internal state). Also wired the
mixed-batch dedup test in `useGeminiStream.test.tsx` through the
fast-path accessor (mock returns the dedup Set directly) and
asserted that `getHistoryFunctionResponseIds` IS called and the
legacy cloning `getHistory()` is NOT. The earlier version of that
test only mocked `getHistory()`, so the optimization the previous
commit added was never actually exercised — a regression that
dropped the fast-path branch from the dispatcher would re-route
every batch onto the slow `structuredClone` path with no test
failure. The new assertion makes that regression visible.

Tests: 353/353 (124 in geminiChat.test.ts +6, 96 in
useGeminiStream.test.tsx unchanged but mock surface expanded,
133 in client.test.ts unchanged). tsc + eslint + prettier clean
on changed files.

packages/cli/src/ui/hooks/useGeminiStream.test.tsx

@@ -1228,8 +1228,20 @@ describe('useGeminiStream', () => {
     } as unknown as TrackedCompletedToolCall;
 
     const client = new MockedGeminiClientClass(mockConfig);
-    // History has fr for ONLY the deduped callId — `call_mixed_fresh`
-    // is not paired and must flow through to sendMessageStream.
+    // Wire BOTH the fast-path accessor (`getHistoryFunctionResponseIds`)
+    // and the legacy `getHistory()` fallback. Wiring the fast path
+    // is the actual point of this test: production code prefers
+    // `getHistoryFunctionResponseIds` to skip the multi-millisecond
+    // `structuredClone` cost on long sessions, and an earlier
+    // version of this test only mocked `getHistory()` so the slow
+    // path was always the one exercised. We assert below that the
+    // fast path was the only one called — a regression that drops
+    // the fast-path branch from the dispatcher would silently
+    // re-route every batch onto the slow clone path with no test
+    // failure.
+    client.getHistoryFunctionResponseIds = vi
+      .fn()
+      .mockReturnValue(new Set(['call_mixed_deduped']));
     client.getHistory = vi.fn().mockReturnValue([
       { role: 'user', parts: [{ text: 'kick off' }] },
       {
@@ -1325,6 +1337,18 @@ describe('useGeminiStream', () => {
     // (c) The fresh tool's real result reaches sendMessageStream —
     // dedup didn't accidentally suppress it.
     expect(mockSendMessageStream).toHaveBeenCalled();
+
+    // (d) Fast-path was taken: `getHistoryFunctionResponseIds` was
+    // called for the dedup pass, and the cloning `getHistory()`
+    // fallback was NOT used by the dedup. (Other call sites in the
+    // hook may still call getHistory for their own purposes; we
+    // pin only that the dedup itself did not re-clone.) A future
+    // refactor that drops the fast-path branch from the dispatcher
+    // would re-route the dedup pass onto the structuredClone path
+    // and break this assertion — exactly the regression the
+    // accessor was added to prevent.
+    expect(client.getHistoryFunctionResponseIds).toHaveBeenCalled();
+    expect(client.getHistory).not.toHaveBeenCalled();
   });
 
   it('should not flicker streaming state to Idle between tool completion and submission', async () => {

packages/core/src/core/client.ts

@@ -397,6 +397,7 @@ export class GeminiClient {
    */
   repairOrphanedToolUseTurnsInHistory(reason?: string): {
     injected: Array<{ callId: string; name: string }>;
+    droppedDuplicates: Array<{ callId: string; name: string }>;
   } {
     const result = this.getChat().repairOrphanedToolUseTurns(reason);
     if (result.injected.length > 0) {
@@ -407,6 +408,20 @@ export class GeminiClient {
             .join(', ')}`,
       );
     }
+    if (result.droppedDuplicates.length > 0) {
+      // Surface the duplicate-cleanup pass so investigators tracing
+      // a dedup-drop log have a breadcrumb pointing back to the
+      // repair function. Without this a duplicate-only repair (no
+      // synthesis, no hoist) leaves zero diagnostic trail and a
+      // future callId-collision bug would silently delete the
+      // wrong fr. qwen-latest-series-invite-beta-v34 thread on PR #4176.
+      debugLogger.warn(
+        `[REPAIR] Dropped ${result.droppedDuplicates.length} duplicate ` +
+          `functionResponse(s) for callId(s): ${result.droppedDuplicates
+            .map((e) => `${e.name}(${e.callId})`)
+            .join(', ')}`,
+      );
+    }
     return result;
   }
 

packages/core/src/core/geminiChat.test.ts

@@ -9,6 +9,7 @@ import type {
   Content,
   GenerateContentConfig,
   GenerateContentResponse,
+  Part,
 } from '@google/genai';
 import { ApiError } from '@google/genai';
 import { AuthType, type ContentGenerator } from '../core/contentGenerator.js';
@@ -2222,6 +2223,163 @@ describe('GeminiChat', async () => {
     });
   });
 
+  describe('getHistoryFunctionResponseIds (qwen-latest-series-invite-beta-v34 thread on PR #4176)', () => {
+    // Walk-only accessor used by `useGeminiStream.handleCompletedTools`
+    // for the dedup pass. The whole point of this method is to avoid
+    // the multi-millisecond `structuredClone` hit that
+    // `getHistory()` pays on long sessions when only the id Set is
+    // needed. Pin the contract: returned Set contains every fr id
+    // present in user turns (including duplicates collapsed to one
+    // Set entry), and ignores parts that aren't functionResponses
+    // and turns that aren't user.
+    it('returns an empty Set for empty history', () => {
+      expect(chat.getHistoryFunctionResponseIds()).toEqual(new Set());
+    });
+
+    it('collects fr ids from user turns and ignores non-fr parts', () => {
+      chat.setHistory([
+        { role: 'user', parts: [{ text: 'go' }] },
+        {
+          role: 'model',
+          parts: [
+            { functionCall: { id: 'cid_a', name: 'read_file', args: {} } },
+          ],
+        },
+        {
+          role: 'user',
+          parts: [
+            {
+              functionResponse: {
+                id: 'cid_a',
+                name: 'read_file',
+                response: { output: 'a' },
+              },
+            },
+            { text: 'follow up' },
+          ],
+        },
+      ]);
+
+      expect(chat.getHistoryFunctionResponseIds()).toEqual(new Set(['cid_a']));
+    });
+
+    it('skips functionCall parts in model turns (only user[fr] counts)', () => {
+      // Defensive: a regression that walks all turns instead of just
+      // user turns would pull in `functionCall.id`s and double-count.
+      chat.setHistory([
+        {
+          role: 'model',
+          parts: [
+            { functionCall: { id: 'cid_model', name: 'read_file', args: {} } },
+          ],
+        },
+        {
+          role: 'user',
+          parts: [
+            {
+              functionResponse: {
+                id: 'cid_user',
+                name: 'read_file',
+                response: { output: 'u' },
+              },
+            },
+          ],
+        },
+      ]);
+
+      const ids = chat.getHistoryFunctionResponseIds();
+      expect(ids).toEqual(new Set(['cid_user']));
+      expect(ids.has('cid_model')).toBe(false);
+    });
+
+    it('collapses duplicate fr ids across multiple user turns to one Set entry', () => {
+      // Same id echoed twice in different user turns: dedup callers
+      // only need to know "is this id paired anywhere", not the
+      // count, so a Set is sufficient and natural.
+      chat.setHistory([
+        {
+          role: 'user',
+          parts: [
+            {
+              functionResponse: {
+                id: 'cid_dup',
+                name: 'read_file',
+                response: { output: '1' },
+              },
+            },
+          ],
+        },
+        {
+          role: 'user',
+          parts: [
+            {
+              functionResponse: {
+                id: 'cid_dup',
+                name: 'read_file',
+                response: { output: '2' },
+              },
+            },
+          ],
+        },
+      ]);
+
+      const ids = chat.getHistoryFunctionResponseIds();
+      expect(ids.size).toBe(1);
+      expect(ids.has('cid_dup')).toBe(true);
+    });
+
+    it('handles entries with no parts and parts with no functionResponse', () => {
+      // Defensive against malformed history (missing parts, parts
+      // with neither text nor fr): must not crash.
+      chat.setHistory([
+        { role: 'user', parts: undefined as unknown as Part[] },
+        { role: 'user', parts: [] },
+        {
+          role: 'user',
+          parts: [
+            {
+              functionResponse: {
+                id: 'cid_ok',
+                name: 'read_file',
+                response: { output: 'ok' },
+              },
+            },
+          ],
+        },
+      ]);
+
+      expect(chat.getHistoryFunctionResponseIds()).toEqual(new Set(['cid_ok']));
+    });
+
+    it('does not deep-clone history (returns a fresh Set, not aliased to internal state)', () => {
+      // The whole reason this method exists is to avoid the
+      // structuredClone in getHistory(). Mutating the returned Set
+      // must not bleed into the next call.
+      chat.setHistory([
+        {
+          role: 'user',
+          parts: [
+            {
+              functionResponse: {
+                id: 'cid_immut',
+                name: 'read_file',
+                response: { output: 'v' },
+              },
+            },
+          ],
+        },
+      ]);
+
+      const first = chat.getHistoryFunctionResponseIds();
+      first.add('cid_FAKE');
+      first.delete('cid_immut');
+
+      const second = chat.getHistoryFunctionResponseIds();
+      expect(second.has('cid_immut')).toBe(true);
+      expect(second.has('cid_FAKE')).toBe(false);
+    });
+  });
+
   describe('getHistoryTail', () => {
     it('returns only the requested recent entries as a deep copy', () => {
       const oldContent: Content = { role: 'user', parts: [{ text: 'old' }] };

packages/core/src/core/geminiChat.ts

@@ -444,8 +444,20 @@ const ORPHAN_TOOL_USE_REPAIR_REASON =
 export function repairOrphanedToolUseTurns(
   history: Content[],
   reason: string = ORPHAN_TOOL_USE_REPAIR_REASON,
-): { injected: Array<{ callId: string; name: string }> } {
+): {
+  injected: Array<{ callId: string; name: string }>;
+  droppedDuplicates: Array<{ callId: string; name: string }>;
+} {
   const injected: Array<{ callId: string; name: string }> = [];
+  // Duplicates removed during the cleanup pass (any callId that had
+  // more than one `functionResponse` echo across the consecutive
+  // user turns). Returned alongside `injected` so call sites can log
+  // the cleanup — without this, a duplicate-only repair (no
+  // synthesis, no hoist) leaves zero diagnostic trail and a future
+  // callId-collision bug in the resolver could silently drop the
+  // wrong fr with no breadcrumb pointing here.
+  // qwen-latest-series-invite-beta-v34 thread on PR #4176.
+  const droppedDuplicates: Array<{ callId: string; name: string }> = [];
 
   // Forward walk: i mutates as we splice, so use index-based iteration
   // and skip the freshly-inserted user turn to avoid re-scanning it.
@@ -558,6 +570,7 @@ export function repairOrphanedToolUseTurns(
           turnIdx: locations[k]!.turnIdx,
           partIdx: locations[k]!.partIdx,
         });
+        droppedDuplicates.push({ callId: id, name });
       }
     }
     if (synthesizeIds.length === 0 && allRemovalTargets.length === 0) continue;
@@ -649,7 +662,7 @@ export function repairOrphanedToolUseTurns(
     }
   }
 
-  return { injected };
+  return { injected, droppedDuplicates };
 }
 
 /**
@@ -1077,6 +1090,19 @@ export class GeminiChat {
               .join(', '),
         );
       }
+      if (inlineRepair.droppedDuplicates.length > 0) {
+        // Symmetrical with the synthesis log: a duplicate-only repair
+        // (no synthesis, no hoist) here would otherwise be silent.
+        // qwen-latest-series-invite-beta-v34 thread on PR #4176.
+        debugLogger.warn(
+          `[REPAIR] sendMessageStream inline pass dropped ` +
+            `${inlineRepair.droppedDuplicates.length} duplicate ` +
+            `functionResponse(s): ` +
+            inlineRepair.droppedDuplicates
+              .map((entry) => `${entry.name}(${entry.callId})`)
+              .join(', '),
+        );
+      }
       requestContents = this.getHistory(true);
     } catch (error) {
       if (userContentAdded) {
@@ -1571,11 +1597,39 @@ export class GeminiChat {
               // in lockstep so the outer `finally` JSONL flush can't
               // resurrect a partial we just deleted from live history.
               // qwen-latest-series-invite-beta-v34 thread on PR #4176.
+              // 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.
+              // qwen-latest-series-invite-beta-v34 thread on PR #4176.
+              const expectedIdx = self.pendingPartialAssistantTurnIndex;
+              const lastIdx = self.history.length - 1;
               if (
-                self.pendingPartialAssistantTurnIndex !== null &&
+                expectedIdx !== null &&
                 self.history.length > 0 &&
-                self.history[self.history.length - 1].role === 'model'
+                self.history[lastIdx]?.role === 'model'
               ) {
+                if (expectedIdx !== lastIdx) {
+                  debugLogger.warn(
+                    `[RECOVERY_POP] Marker/last-index mismatch: ` +
+                      `marker=${expectedIdx}, lastIdx=${lastIdx}, ` +
+                      `historyLength=${self.history.length}. Popping ` +
+                      `last entry as best-effort rollback — investigate ` +
+                      `any history mutation between processStreamResponse's ` +
+                      `partial push and this catch.`,
+                  );
+                }
                 self.history.pop();
                 self.clearPendingPartialState();
               }
@@ -1948,6 +2002,7 @@ export class GeminiChat {
    */
   repairOrphanedToolUseTurns(reason?: string): {
     injected: Array<{ callId: string; name: string }>;
+    droppedDuplicates: Array<{ callId: string; name: string }>;
   } {
     return repairOrphanedToolUseTurns(this.history, reason);
   }