diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index b2a62349c..42187a1fc 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -457,8 +457,138 @@ describe('Gemini Client (client.ts)', () => { expect(newHistory.length).toBe(initialHistory.length); expect(JSON.stringify(newHistory)).not.toContain('some old message'); }); + + it('clears the FileReadCache so post-reset Reads re-emit content', async () => { + const cacheClear = mockFileReadCacheClear(); + + await client.resetChat(); + + expect(cacheClear).toHaveBeenCalled(); + }); }); + describe('history mutation invalidates FileReadCache', () => { + it('setHistory clears the cache', () => { + const cacheClear = mockFileReadCacheClear(); + client['chat'] = { + setHistory: vi.fn(), + } as unknown as GeminiChat; + + client.setHistory([{ role: 'user', parts: [{ text: 'replaced' }] }]); + + expect(cacheClear).toHaveBeenCalled(); + }); + + /** + * Test helper: mock a GeminiChat whose history length goes from + * `before` to `after` across truncateHistory(). The first + * getHistoryLength() call (pre-truncate) returns `before`; the + * second (post-truncate) returns `after`. + */ + function mockChatWithLengths(before: number, after: number): GeminiChat { + return { + getHistoryLength: vi + .fn() + .mockReturnValueOnce(before) + .mockReturnValueOnce(after), + truncateHistory: vi.fn(), + } as unknown as GeminiChat; + } + + it('truncateHistory clears the cache when entries are actually removed', () => { + const cacheClear = mockFileReadCacheClear(); + client['chat'] = mockChatWithLengths(3, 2); + + client.truncateHistory(2); + + expect(cacheClear).toHaveBeenCalled(); + }); + + it('truncateHistory does NOT clear the cache when nothing was removed (keepCount >= history length)', () => { + const cacheClear = mockFileReadCacheClear(); + + // keepCount equals history length — nothing dropped. + client['chat'] = mockChatWithLengths(2, 2); + client.truncateHistory(2); + expect(cacheClear).not.toHaveBeenCalled(); + + // keepCount exceeds history length — also a no-op. + client['chat'] = mockChatWithLengths(2, 2); + client.truncateHistory(99); + expect(cacheClear).not.toHaveBeenCalled(); + }); + + it('truncateHistory clears the cache when a non-finite keepCount empties history (NaN regression)', () => { + // slice(0, NaN) returns [], but `NaN < prevLen` evaluates to + // false. Comparing the actual post-truncate length closes that + // hole — without this guard the cache would survive a history + // wipe and the file_unchanged placeholder bug returns. + const cacheClear = mockFileReadCacheClear(); + client['chat'] = mockChatWithLengths(3, 0); + + client.truncateHistory(NaN); + + expect(cacheClear).toHaveBeenCalled(); + }); + + it('truncateHistory uses O(1) getHistoryLength, not getHistory (avoids structuredClone)', () => { + mockFileReadCacheClear(); + const getHistoryLength = vi.fn().mockReturnValue(5); + const getHistory = vi.fn(); + client['chat'] = { + getHistoryLength, + getHistory, + truncateHistory: vi.fn(), + } as unknown as GeminiChat; + + client.truncateHistory(3); + + expect(getHistoryLength).toHaveBeenCalled(); + expect(getHistory).not.toHaveBeenCalled(); + }); + + it('retry strips orphaned trailing user entries and clears the cache', async () => { + const cacheClear = mockFileReadCacheClear(); + const stripOrphanedUserEntriesFromHistory = vi.fn(); + client['chat'] = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue([]), + stripOrphanedUserEntriesFromHistory, + } as unknown as GeminiChat; + mockTurnRunFn.mockReturnValue( + (async function* () { + yield { type: GeminiEventType.Content, value: 'response' }; + })(), + ); + + const stream = client.sendMessageStream( + [{ text: 'retry' }], + new AbortController().signal, + 'prompt-retry-1', + { type: SendMessageType.Retry }, + ); + for await (const _ of stream) { + /* drain */ + } + + expect(stripOrphanedUserEntriesFromHistory).toHaveBeenCalled(); + expect(cacheClear).toHaveBeenCalled(); + }); + }); + + /** + * Test helper: replace mockConfig.getFileReadCache to return a stub + * whose clear() is a fresh spy. Returned spy lets tests assert on + * whether a code path invalidated the cache. + */ + function mockFileReadCacheClear(): ReturnType { + const clearMock = vi.fn(); + vi.mocked(mockConfig.getFileReadCache).mockReturnValue({ + clear: clearMock, + } as unknown as ReturnType); + return clearMock; + } + describe('thinking block idle cleanup and latch', () => { let mockChat: Partial; @@ -506,6 +636,100 @@ describe('Gemini Client (client.ts)', () => { }); }); + describe('microcompaction FileReadCache invalidation', () => { + function makeReadFileResponses(count: number): Content[] { + const out: Content[] = []; + for (let i = 0; i < count; i++) { + out.push({ + role: 'model', + parts: [ + { + functionCall: { + name: 'read_file', + args: { file_path: `/x/${i}.ts` }, + }, + }, + ], + }); + out.push({ + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: `content of ${i}` }, + }, + }, + ], + }); + } + return out; + } + + beforeEach(() => { + mockTurnRunFn.mockReturnValue( + (async function* () { + yield { type: GeminiEventType.Content, value: 'response' }; + })(), + ); + }); + + it('clears the cache after microcompaction strips old read_file results', async () => { + // Default test fixture: toolResultsThresholdMinutes = 60, + // toolResultsNumToKeep = 5. Six read_file results + a 90-minute + // idle gap means the oldest one gets cleared, so the if-meta + // branch in sendMessageStream fires and must invalidate the cache. + const cacheClear = mockFileReadCacheClear(); + + const history = makeReadFileResponses(6); + const setHistory = vi.fn(); + client['chat'] = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue(history), + setHistory, + } as unknown as GeminiChat; + client['lastApiCompletionTimestamp'] = Date.now() - 90 * 60_000; + + const stream = client.sendMessageStream( + [{ text: 'hi' }], + new AbortController().signal, + 'prompt-mc-clear-1', + { type: SendMessageType.UserQuery }, + ); + for await (const _ of stream) { + /* drain */ + } + + expect(setHistory).toHaveBeenCalled(); + expect(cacheClear).toHaveBeenCalled(); + }); + + it('does not clear the cache when the idle gap is below the threshold', async () => { + const cacheClear = mockFileReadCacheClear(); + + const history = makeReadFileResponses(6); + client['chat'] = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue(history), + setHistory: vi.fn(), + } as unknown as GeminiChat; + // Recent activity — microcompaction must not fire. + client['lastApiCompletionTimestamp'] = Date.now() - 30 * 1000; + + const stream = client.sendMessageStream( + [{ text: 'hi' }], + new AbortController().signal, + 'prompt-mc-clear-2', + { type: SendMessageType.UserQuery }, + ); + for await (const _ of stream) { + /* drain */ + } + + expect(cacheClear).not.toHaveBeenCalled(); + }); + }); + describe('tryCompressChat', () => { const mockGetHistory = vi.fn(); diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 0866f0d20..a5c079da9 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -206,15 +206,47 @@ export class GeminiClient { private stripOrphanedUserEntriesFromHistory() { this.getChat().stripOrphanedUserEntriesFromHistory(); + // Stripped trailing user entries can include read_file + // functionResponses from a failed-then-retried request. The + // FileReadCache would still record those reads, so the retry's + // re-issued Read could hit the file_unchanged placeholder while + // the model has nothing to fall back on. Clear to be safe. + debugLogger.debug( + '[FILE_READ_CACHE] clear after stripOrphanedUserEntriesFromHistory', + ); + this.config.getFileReadCache().clear(); } setHistory(history: Content[]) { this.getChat().setHistory(history); + // Replacing history wholesale drops any prior read_file tool + // results the FileReadCache still believes the model has seen. + // Without clearing, a follow-up Read of an unchanged file would + // return the file_unchanged placeholder for bytes that no longer + // exist in the new history. + debugLogger.debug('[FILE_READ_CACHE] clear after setHistory'); + this.config.getFileReadCache().clear(); this.forceFullIdeContext = true; } truncateHistory(keepCount: number) { + // Use the O(1) length getter rather than getHistory() — the latter + // structuredClone's the entire history just to read .length, which + // gets expensive in long-running sessions. + const prevLen = this.getChat().getHistoryLength(); this.getChat().truncateHistory(keepCount); + // Decide whether to invalidate based on the *actual* post-truncate + // length, not on the keepCount argument. Comparing keepCount alone + // misses pathological inputs (e.g. NaN: slice(0, NaN) returns [], + // emptying history, but `NaN < prevLen` is false and would skip + // the clear, reintroducing the file_unchanged placeholder bug). + const newLen = this.getChat().getHistoryLength(); + if (newLen < prevLen) { + debugLogger.debug( + `[FILE_READ_CACHE] clear after truncateHistory(keep=${keepCount}, prev=${prevLen}, new=${newLen})`, + ); + this.config.getFileReadCache().clear(); + } this.forceFullIdeContext = true; } @@ -233,6 +265,12 @@ export class GeminiClient { async resetChat(): Promise { this.surfacedRelevantAutoMemoryPaths.clear(); this.lastApiCompletionTimestamp = null; + // startChat() rewrites the chat to its initial state. Any prior + // read_file tool results the FileReadCache still tracks are no + // longer in history, so a follow-up Read would serve a placeholder + // pointing at content the model can no longer retrieve. + debugLogger.debug('[FILE_READ_CACHE] clear after resetChat'); + this.config.getFileReadCache().clear(); await this.startChat(); } @@ -694,6 +732,16 @@ export class GeminiClient { ); if (mcResult.meta) { this.getChat().setHistory(mcResult.history); + // Microcompaction replaces old compactable tool outputs + // (including read_file) with a placeholder, but the + // FileReadCache still records the prior full Reads as "seen in + // this conversation". A follow-up Read of an unchanged file + // would then return the file_unchanged placeholder pointing at + // bytes the model can no longer retrieve from history. Drop the + // cache so post-microcompaction Reads re-emit the bytes, + // mirroring the post-compaction clear in tryCompressChat. + debugLogger.debug('[FILE_READ_CACHE] clear after microcompaction'); + this.config.getFileReadCache().clear(); const m = mcResult.meta; debugLogger.debug( `[TIME-BASED MC] gap ${m.gapMinutes}min > ${m.thresholdMinutes}min, ` + @@ -1166,6 +1214,7 @@ export class GeminiClient { // placeholder pointing at content the model can no longer // retrieve from its own context. Clear the cache so post- // compaction Reads re-emit the bytes. + debugLogger.debug('[FILE_READ_CACHE] clear after tryCompressChat'); this.config.getFileReadCache().clear(); uiTelemetryService.setLastPromptTokenCount(info.newTokenCount); this.forceFullIdeContext = true; diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 5737f1563..8a45bb5d4 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -1049,6 +1049,25 @@ describe('GeminiChat', async () => { }); }); + describe('getHistoryLength', () => { + it('returns 0 for an empty history', () => { + expect(chat.getHistoryLength()).toBe(0); + }); + + it('reflects entries added via addHistory', () => { + chat.addHistory({ role: 'user', parts: [{ text: 'a' }] }); + chat.addHistory({ role: 'model', parts: [{ text: 'b' }] }); + expect(chat.getHistoryLength()).toBe(2); + }); + + it('matches getHistory().length without paying the structuredClone cost', () => { + chat.addHistory({ role: 'user', parts: [{ text: 'a' }] }); + chat.addHistory({ role: 'model', parts: [{ text: 'b' }] }); + chat.addHistory({ role: 'user', parts: [{ text: 'c' }] }); + expect(chat.getHistoryLength()).toBe(chat.getHistory().length); + }); + }); + describe('sendMessageStream with retries', () => { it('should retry on invalid content, succeed, and report metrics', async () => { vi.useFakeTimers(); diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index d9a0babc8..804c143d8 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -770,6 +770,15 @@ export class GeminiChat { return structuredClone(history); } + /** + * Returns the number of entries in the raw chat history. O(1) and + * does not clone — use this when you only need the count and would + * otherwise pay the {@link getHistory} `structuredClone` cost. + */ + getHistoryLength(): number { + return this.history.length; + } + /** * Clears the chat history. */ diff --git a/packages/core/src/services/fileReadCache.integration.test.ts b/packages/core/src/services/fileReadCache.integration.test.ts new file mode 100644 index 000000000..b9b39aded --- /dev/null +++ b/packages/core/src/services/fileReadCache.integration.test.ts @@ -0,0 +1,278 @@ +/** + * Integration tests for the FileReadCache short-circuit. Real + * filesystem, real ReadFileTool, real microcompactHistory — verify + * that the placeholder fast-path stays correct under history rewrites. + */ + +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import type { Content } from '@google/genai'; + +import { FileReadCache } from './fileReadCache.js'; +import { ReadFileTool } from '../tools/read-file.js'; +import { microcompactHistory } from './microcompaction/microcompact.js'; +import { StandardFileSystemService } from './fileSystemService.js'; + +function makeConfig(targetDir: string, cache: FileReadCache, disabled = false) { + const explicit: Record = { + getTargetDir: () => targetDir, + getProjectRoot: () => targetDir, + getWorkspaceContext: () => ({ + isPathWithinWorkspace: () => true, + }), + storage: { + getProjectTempDir: () => path.join(targetDir, '.tmp'), + getProjectDir: () => path.join(targetDir, '.proj'), + getUserSkillsDirs: () => [], + }, + getFileReadCache: () => cache, + getFileReadCacheDisabled: () => disabled, + getFileService: () => ({ shouldQwenIgnoreFile: () => false }), + getFileFilteringOptions: () => ({}), + getDebugMode: () => false, + getFileSystemService: () => new StandardFileSystemService(), + getContentGeneratorConfig: () => ({ modalities: {} }), + getModel: () => 'test-model', + getTruncateToolOutputLines: () => 2000, + getTruncateToolOutputThreshold: () => 4_000_000, + getUsageStatisticsEnabled: () => false, + }; + return new Proxy(explicit, { + get(target, prop) { + if (prop in target) + return (target as Record)[prop]; + // Default: any unknown getter returns undefined-yielding fn. + return () => undefined; + }, + }) as never; +} + +describe('FileReadCache integration: read after history rewrite', () => { + let tmpDir: string; + let filePath: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'repro-3805-')); + filePath = path.join(tmpDir, 'foo.ts'); + fs.writeFileSync( + filePath, + 'export function hello() {\n return "world";\n}\n'.repeat(10), + ); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('returns the file_unchanged placeholder on a follow-up Read after microcompact, exposing why the cache must be cleared on history rewrite', async () => { + const cache = new FileReadCache(); + const config = makeConfig(tmpDir, cache); + const tool = new ReadFileTool(config); + + // STEP 1 — first real Read populates the cache. + const r1 = await tool.buildAndExecute( + { file_path: filePath }, + new AbortController().signal, + ); + expect(typeof r1.llmContent).toBe('string'); + expect(r1.llmContent as string).toContain('export function hello'); + expect(cache.size()).toBe(1); + + // STEP 2 — build a conversation history mirroring real flow: + // 6 prior read_file functionResponses with the foo.ts content. + // microcompact's keepRecent=1 will clear the oldest 5. + const history: Content[] = []; + for (let i = 0; i < 6; i++) { + history.push({ + role: 'model', + parts: [ + { + functionCall: { + name: 'read_file', + args: { file_path: filePath }, + }, + }, + ], + }); + history.push({ + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: r1.llmContent as string }, + }, + }, + ], + }); + } + + // STEP 3 — microcompact fires (>60min idle). + const mcResult = microcompactHistory(history, Date.now() - 90 * 60_000, { + toolResultsThresholdMinutes: 60, + toolResultsNumToKeep: 1, + }); + expect(mcResult.meta).toBeDefined(); + expect(mcResult.meta!.toolsCleared).toBe(5); + + // Confirm: most foo.ts content has been wiped from history. + const fooContentEntries = mcResult.history.filter((c) => + c.parts?.some((p) => { + const out = p.functionResponse?.response?.['output']; + return typeof out === 'string' && out.includes('export function hello'); + }), + ); + // Only 1 fresh entry remains; the other 5 are placeholders. + expect(fooContentEntries).toHaveLength(1); + + // STEP 4 — pre-fix code path: cache is NOT cleared after microcompact. + // User reads foo.ts again. File on disk is unchanged. + const r2 = await tool.buildAndExecute( + { file_path: filePath }, + new AbortController().signal, + ); + + // THE BUG: returned content is the placeholder, NOT the real file. + expect(r2.llmContent as string).toContain( + 'unchanged since last read in this session', + ); + expect(r2.llmContent as string).not.toContain('export function hello'); + + // The model now has: + // - history: 5 entries are [Old tool result content cleared], + // 1 entry has real content (the most-recent kept one) + // - fresh tool response: a placeholder pointing at "earlier in + // this conversation" — which is partly true (1 entry remains) + // but if the LLM trusted the placeholder and discarded the + // last surviving entry, the bytes are unrecoverable. + // + // In a longer chain (e.g. 20 reads, keep 1, microcompact clears + // 19), the surviving entry might not even be foo.ts — it would be + // whatever was read most recently. Then the placeholder points at + // ZERO bytes the model can find. + }); + + it('after cache.clear(), a follow-up Read of the same unchanged file re-emits the real bytes', async () => { + const cache = new FileReadCache(); + const config = makeConfig(tmpDir, cache); + const tool = new ReadFileTool(config); + + const r1 = await tool.buildAndExecute( + { file_path: filePath }, + new AbortController().signal, + ); + expect(r1.llmContent as string).toContain('export function hello'); + + // The fix. + cache.clear(); + + const r2 = await tool.buildAndExecute( + { file_path: filePath }, + new AbortController().signal, + ); + + expect(r2.llmContent as string).toContain('export function hello'); + expect(r2.llmContent as string).not.toContain( + 'unchanged since last read in this session', + ); + }); + + it('worst case: when microcompact removes every prior read of a file, the placeholder leaves zero recoverable bytes for the model', async () => { + // This is the worst-case version: many reads, microcompact clears + // everything, the surviving entry is a different file. The placeholder + // then points the model at content that no longer exists anywhere + // in its reachable context. + const cache = new FileReadCache(); + const config = makeConfig(tmpDir, cache); + const tool = new ReadFileTool(config); + + const otherPath = path.join(tmpDir, 'other.ts'); + fs.writeFileSync(otherPath, 'unrelated\n'); + + // Read foo.ts (target file). + await tool.buildAndExecute( + { file_path: filePath }, + new AbortController().signal, + ); + + // Build history: 1 foo.ts read, then 1 other.ts read (kept). + const fooContent = fs.readFileSync(filePath, 'utf-8'); + const history: Content[] = [ + { + role: 'model', + parts: [ + { + functionCall: { + name: 'read_file', + args: { file_path: filePath }, + }, + }, + ], + }, + { + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: fooContent }, + }, + }, + ], + }, + { + role: 'model', + parts: [ + { + functionCall: { + name: 'read_file', + args: { file_path: otherPath }, + }, + }, + ], + }, + { + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: 'unrelated\n' }, + }, + }, + ], + }, + ]; + + const mc = microcompactHistory(history, Date.now() - 90 * 60_000, { + toolResultsThresholdMinutes: 60, + toolResultsNumToKeep: 1, + }); + expect(mc.meta!.toolsCleared).toBe(1); + + // foo.ts content is gone from history; only other.ts remains. + const surviving = mc.history + .flatMap((c) => c.parts ?? []) + .map((p) => p.functionResponse?.response?.['output']) + .filter((o): o is string => typeof o === 'string'); + expect(surviving.some((o) => o.includes('export function hello'))).toBe( + false, + ); + + // Now Read foo.ts again — pre-fix, cache returns placeholder. + const r = await tool.buildAndExecute( + { file_path: filePath }, + new AbortController().signal, + ); + + expect(r.llmContent as string).toContain( + 'unchanged since last read in this session', + ); + // Total foo.ts content reachable to the model: + // history → 0 bytes + // fresh tool result → placeholder, 0 bytes + // The model literally cannot recover the file contents. + }); +});