From 4e5c1fce8da6e7dc17cf9b62424f6ee4a748d376 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Tue, 16 Sep 2025 12:12:45 -0700 Subject: [PATCH] Remove unused code. (#8497) --- packages/core/src/core/geminiChat.test.ts | 207 ---------------------- packages/core/src/core/geminiChat.ts | 121 +++---------- 2 files changed, 29 insertions(+), 299 deletions(-) diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 4d5b6f4ab1..ea783d9211 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -8,7 +8,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { Content, GenerateContentConfig, - Part, GenerateContentResponse, } from '@google/genai'; import type { ContentGenerator } from '../core/contentGenerator.js'; @@ -286,67 +285,6 @@ describe('GeminiChat', () => { expect(modelTurn?.parts?.length).toBe(1); expect(modelTurn?.parts![0]!.text).toBe('Initial valid content...'); }); - it('should not consolidate text into a part that also contains a functionCall', async () => { - // 1. Mock the API to stream a malformed part followed by a valid text part. - const multiChunkStream = (async function* () { - // This malformed part has both text and a functionCall. - yield { - candidates: [ - { - content: { - role: 'model', - parts: [ - { - text: 'Some text', - functionCall: { name: 'do_stuff', args: {} }, - }, - ], - }, - }, - ], - } as unknown as GenerateContentResponse; - // This valid text part should NOT be merged into the malformed one. - yield { - candidates: [ - { - content: { - role: 'model', - parts: [{ text: ' that should not be merged.' }], - }, - }, - ], - } as unknown as GenerateContentResponse; - })(); - - vi.mocked(mockContentGenerator.generateContentStream).mockResolvedValue( - multiChunkStream, - ); - - // 2. Action: Send a message and consume the stream. - const stream = await chat.sendMessageStream( - 'test-model', - { message: 'test message' }, - 'prompt-id-malformed-chunk', - ); - for await (const _ of stream) { - // Consume the stream to trigger history recording. - } - - // 3. Assert: Check that the final history was not incorrectly consolidated. - const history = chat.getHistory(); - - expect(history.length).toBe(2); - const modelTurn = history[1]!; - - // CRUCIAL ASSERTION: There should be two separate parts. - // The old, non-strict logic would incorrectly merge them, resulting in one part. - expect(modelTurn?.parts?.length).toBe(2); - - // Verify the contents of each part. - expect(modelTurn?.parts![0]!.text).toBe('Some text'); - expect(modelTurn?.parts![0]!.functionCall).toBeDefined(); - expect(modelTurn?.parts![1]!.text).toBe(' that should not be merged.'); - }); it('should consolidate subsequent text chunks after receiving an empty text chunk', async () => { // 1. Mock the API to return a stream where one chunk is just an empty text part. @@ -620,151 +558,6 @@ describe('GeminiChat', () => { }); }); - describe('recordHistory', () => { - const userInput: Content = { - role: 'user', - parts: [{ text: 'User input' }], - }; - - it('should consolidate all consecutive model turns into a single turn', () => { - const userInput: Content = { - role: 'user', - parts: [{ text: 'User input' }], - }; - // This simulates a multi-part model response with different part types. - const modelOutput: Content[] = [ - { role: 'model', parts: [{ text: 'Thinking...' }] }, - { - role: 'model', - parts: [{ functionCall: { name: 'do_stuff', args: {} } }], - }, - ]; - - // @ts-expect-error Accessing private method for testing - chat.recordHistory(userInput, modelOutput); - const history = chat.getHistory(); - - // The history should contain the user's turn and ONE consolidated model turn. - // The old code would fail here, resulting in a length of 3. - //expect(history).toBe([]); - expect(history.length).toBe(2); - - const modelTurn = history[1]!; - expect(modelTurn.role).toBe('model'); - - // The consolidated turn should contain both the text part and the functionCall part. - expect(modelTurn?.parts?.length).toBe(2); - expect(modelTurn?.parts![0]!.text).toBe('Thinking...'); - expect(modelTurn?.parts![1]!.functionCall).toBeDefined(); - }); - - it('should add a placeholder model turn when a tool call is followed by an empty response', () => { - // 1. Setup: A history where the model has just made a function call. - const initialHistory: Content[] = [ - { role: 'user', parts: [{ text: 'Initial prompt' }] }, - { - role: 'model', - parts: [{ functionCall: { name: 'test_tool', args: {} } }], - }, - ]; - chat.setHistory(initialHistory); - - // 2. Action: The user provides the tool's response, and the model's - // final output is empty (e.g., just a thought, which gets filtered out). - const functionResponse: Content = { - role: 'user', - parts: [{ functionResponse: { name: 'test_tool', response: {} } }], - }; - const emptyModelOutput: Content[] = []; - - // @ts-expect-error Accessing private method for testing - chat.recordHistory(functionResponse, emptyModelOutput, [ - functionResponse, - ]); - - // 3. Assert: The history should now have four valid, alternating turns. - const history = chat.getHistory(); - expect(history.length).toBe(4); - - // The final turn must be the empty model placeholder. - const lastTurn = history[3]!; - expect(lastTurn.role).toBe('model'); - expect(lastTurn?.parts?.length).toBe(0); - - // The second-to-last turn must be the function response we provided. - const secondToLastTurn = history[2]!; - expect(secondToLastTurn.role).toBe('user'); - expect(secondToLastTurn?.parts![0]!.functionResponse).toBeDefined(); - }); - - it('should add user input and a single model output to history', () => { - const modelOutput: Content[] = [ - { role: 'model', parts: [{ text: 'Model output' }] }, - ]; - // @ts-expect-error Accessing private method for testing - chat.recordHistory(userInput, modelOutput); - const history = chat.getHistory(); - expect(history.length).toBe(2); - expect(history[0]).toEqual(userInput); - expect(history[1]).toEqual(modelOutput[0]); - }); - - it('should consolidate adjacent text parts from multiple content objects', () => { - const modelOutput: Content[] = [ - { role: 'model', parts: [{ text: 'Part 1.' }] }, - { role: 'model', parts: [{ text: ' Part 2.' }] }, - { role: 'model', parts: [{ text: ' Part 3.' }] }, - ]; - // @ts-expect-error Accessing private method for testing - chat.recordHistory(userInput, modelOutput); - const history = chat.getHistory(); - expect(history.length).toBe(2); - expect(history[1]!.role).toBe('model'); - expect(history[1]!.parts).toEqual([{ text: 'Part 1. Part 2. Part 3.' }]); - }); - - it('should add an empty placeholder turn if modelOutput is empty', () => { - // This simulates receiving a pre-filtered, thought-only response. - const emptyModelOutput: Content[] = []; - // @ts-expect-error Accessing private method for testing - chat.recordHistory(userInput, emptyModelOutput); - const history = chat.getHistory(); - expect(history.length).toBe(2); - expect(history[0]).toEqual(userInput); - expect(history[1]!.role).toBe('model'); - expect(history[1]!.parts).toEqual([]); - }); - - it('should preserve model outputs with undefined or empty parts arrays', () => { - const malformedOutput: Content[] = [ - { role: 'model', parts: [{ text: 'Text part' }] }, - { role: 'model', parts: undefined as unknown as Part[] }, - { role: 'model', parts: [] }, - ]; - // @ts-expect-error Accessing private method for testing - chat.recordHistory(userInput, malformedOutput); - const history = chat.getHistory(); - expect(history.length).toBe(4); // userInput + 3 model turns - expect(history[1]!.parts).toEqual([{ text: 'Text part' }]); - expect(history[2]!.parts).toBeUndefined(); - expect(history[3]!.parts).toEqual([]); - }); - - it('should not consolidate content with different roles', () => { - const mixedOutput: Content[] = [ - { role: 'model', parts: [{ text: 'Model 1' }] }, - { role: 'user', parts: [{ text: 'Unexpected User' }] }, - { role: 'model', parts: [{ text: 'Model 2' }] }, - ]; - // @ts-expect-error Accessing private method for testing - chat.recordHistory(userInput, mixedOutput); - const history = chat.getHistory(); - expect(history.length).toBe(4); // userInput, model1, unexpected_user, model2 - expect(history[1]).toEqual(mixedOutput[0]); - expect(history[2]).toEqual(mixedOutput[1]); - expect(history[3]).toEqual(mixedOutput[2]); - }); - }); describe('addHistory', () => { it('should add a new content item to the history', () => { const newContent: Content = { diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 375588024a..7db86ace83 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -81,6 +81,19 @@ function isValidResponse(response: GenerateContentResponse): boolean { return isValidContent(content); } +export function isValidNonThoughtTextPart(part: Part): boolean { + return ( + typeof part.text === 'string' && + !part.thought && + // Technically, the model should never generate parts that have text and + // any of these but we don't trust them so check anyways. + !part.functionCall && + !part.functionResponse && + !part.inlineData && + !part.fileData + ); +} + function isValidContent(content: Content): boolean { if (content.parts === undefined || content.parts.length === 0) { return false; @@ -261,7 +274,6 @@ export class GeminiChat { requestContents, params, prompt_id, - userContent, ); for await (const chunk of stream) { @@ -326,7 +338,6 @@ export class GeminiChat { requestContents: Content[], params: SendMessageParameters, prompt_id: string, - userContent: Content, ): Promise> { const apiCall = () => { const modelToUse = getEffectiveModel( @@ -371,7 +382,7 @@ export class GeminiChat { authType: this.config.getContentGeneratorConfig()?.authType, }); - return this.processStreamResponse(model, streamResponse, userContent); + return this.processStreamResponse(model, streamResponse); } /** @@ -476,7 +487,6 @@ export class GeminiChat { private async *processStreamResponse( model: string, streamResponse: AsyncGenerator, - userInput: Content, ): AsyncGenerator { const modelResponseParts: Part[] = []; let hasReceivedAnyChunk = false; @@ -501,8 +511,10 @@ export class GeminiChat { if (content.parts.some((part) => part.functionCall)) { hasToolCall = true; } - // Always add parts - thoughts will be filtered out later in recordHistory - modelResponseParts.push(...content.parts); + + modelResponseParts.push( + ...content.parts.filter((part) => !part.thought), + ); } } else { logInvalidChunk( @@ -547,7 +559,7 @@ export class GeminiChat { // Record model response text from the collected parts if (modelResponseParts.length > 0) { const responseText = modelResponseParts - .filter((part) => part.text && !part.thought) + .filter((part) => part.text) .map((part) => part.text) .join(''); @@ -560,97 +572,22 @@ export class GeminiChat { } } - // Bundle all streamed parts into a single Content object - const modelOutput: Content[] = - modelResponseParts.length > 0 - ? [{ role: 'model', parts: modelResponseParts }] - : []; - - // Pass the raw, bundled data to the new, robust recordHistory - this.recordHistory(userInput, modelOutput); - } - - private recordHistory(userInput: Content, modelOutput: Content[]) { - // Part 1: Handle the user's turn. - - const lastTurn = this.history[this.history.length - 1]; - // The only time we don't push is if it's the *exact same* object, - // which happens in streaming where we add it preemptively. - if (lastTurn !== userInput) { - if (lastTurn?.role === 'user') { - // This is an invalid sequence. - throw new Error('Cannot add a user turn after another user turn.'); - } - this.history.push(userInput); - } - - // Part 2: Process the model output into a final, consolidated list of turns. - const finalModelTurns: Content[] = []; - for (const content of modelOutput) { - // A. Preserve malformed content that has no 'parts' array. - if (!content.parts) { - finalModelTurns.push(content); - continue; - } - - // B. Filter out 'thought' parts. - const visibleParts = content.parts.filter((part) => !part.thought); - - const newTurn = { ...content, parts: visibleParts }; - const lastTurnInFinal = finalModelTurns[finalModelTurns.length - 1]; - - // Consolidate this new turn with the PREVIOUS turn if they are adjacent model turns. + // String thoughts and consolidate text parts. + const consolidatedParts: Part[] = []; + for (const part of modelResponseParts) { + const lastPart = consolidatedParts[consolidatedParts.length - 1]; if ( - lastTurnInFinal && - lastTurnInFinal.role === 'model' && - newTurn.role === 'model' && - lastTurnInFinal.parts && // SAFETY CHECK: Ensure the destination has a parts array. - newTurn.parts + lastPart?.text && + isValidNonThoughtTextPart(lastPart) && + isValidNonThoughtTextPart(part) ) { - lastTurnInFinal.parts.push(...newTurn.parts); + lastPart.text += part.text; } else { - finalModelTurns.push(newTurn); + consolidatedParts.push(part); } } - // Part 3: Add the processed model turns to the history, with one final consolidation pass. - if (finalModelTurns.length > 0) { - // Re-consolidate parts within any turns that were merged in the previous step. - for (const turn of finalModelTurns) { - if (turn.parts && turn.parts.length > 1) { - const consolidatedParts: Part[] = []; - for (const part of turn.parts) { - const lastPart = consolidatedParts[consolidatedParts.length - 1]; - if ( - lastPart && - // Ensure lastPart is a pure text part - typeof lastPart.text === 'string' && - !lastPart.functionCall && - !lastPart.functionResponse && - !lastPart.inlineData && - !lastPart.fileData && - !lastPart.thought && - // Ensure current part is a pure text part - typeof part.text === 'string' && - !part.functionCall && - !part.functionResponse && - !part.inlineData && - !part.fileData && - !part.thought - ) { - lastPart.text += part.text; - } else { - consolidatedParts.push({ ...part }); - } - } - turn.parts = consolidatedParts; - } - } - this.history.push(...finalModelTurns); - } else { - // If, after all processing, there's NO model output, add the placeholder. - this.history.push({ role: 'model', parts: [] }); - } + this.history.push({ role: 'model', parts: consolidatedParts }); } /**