Repository navigation
fix(cache): preserve tools prefix in side-query for Anthropic prompt-cache hits #6225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
0ff640a
fa04bea
9aaa329
2ab6f80
7405fed
8d53a0e
4f2e89f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,6 +156,7 @@ async function generateViaForkedQuery( | |
| cacheSafeParams, | ||
| jsonSchema: SUGGESTION_SCHEMA, | ||
| model, | ||
| preserveTools: true, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 8d53a0e:
All 49 tests pass. |
||
| }); | ||
|
|
||
| if (result.jsonResult) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -612,6 +612,188 @@ describe('runForkedAgent (cache path)', () => { | |
| expect(createRuntimeContentGeneratorView).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not strip tools when preserveTools is true', async () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 2ab6f80 — new test
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 8d53a0e — new test The test:
All 49 tests pass (18 forkedAgent.cache + 21 suggestionGenerator + 10 speculation). |
||
| saveCacheSafeParams( | ||
| { | ||
| systemInstruction: 'You are helpful', | ||
| tools: [ | ||
| { | ||
| functionDeclarations: [ | ||
| { name: 'edit', description: 'Edit a file' }, | ||
| { name: 'shell', description: 'Run a command' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| [{ role: 'user', parts: [{ text: 'hello' }] }], | ||
| 'test-model', | ||
| ); | ||
|
|
||
| let capturedParams: unknown = null; | ||
|
|
||
| const mockSendMessageStream = vi.fn( | ||
| (_model: string, params: unknown, _promptId: string) => { | ||
| capturedParams = params; | ||
| async function* generate() { | ||
| yield { | ||
| type: StreamEventType.CHUNK, | ||
| value: { | ||
| candidates: [ | ||
| { | ||
| content: { | ||
| role: 'model', | ||
| parts: [{ text: '{"suggestion":"run tests"}' }], | ||
| }, | ||
| }, | ||
| ], | ||
| usageMetadata: { | ||
| promptTokenCount: 10, | ||
| candidatesTokenCount: 5, | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| return Promise.resolve(generate()); | ||
| }, | ||
| ); | ||
|
|
||
| vi.mocked(GeminiChat).mockImplementation( | ||
| () => | ||
| ({ | ||
| sendMessageStream: mockSendMessageStream, | ||
| }) as unknown as GeminiChat, | ||
| ); | ||
|
|
||
| await runForkedAgent({ | ||
| config: {} as Config, | ||
| userMessage: 'suggest something', | ||
| cacheSafeParams: getCacheSafeParams()!, | ||
| preserveTools: true, | ||
| }); | ||
|
|
||
| const sendParams = capturedParams as { | ||
| config?: { tools?: unknown }; | ||
| }; | ||
| expect(sendParams.config!.tools).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('strips tools by default when preserveTools is omitted', async () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 7405fed — removed this test. The first test at line 199 already covers the default |
||
| saveCacheSafeParams( | ||
| { | ||
| tools: [ | ||
| { | ||
| functionDeclarations: [ | ||
| { name: 'edit', description: 'Edit a file' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| [], | ||
| 'test-model', | ||
| ); | ||
|
|
||
| let capturedParams: unknown = null; | ||
|
|
||
| const mockSendMessageStream = vi.fn( | ||
| (_model: string, params: unknown, _promptId: string) => { | ||
| capturedParams = params; | ||
| async function* generate() { | ||
| yield { | ||
| type: StreamEventType.CHUNK, | ||
| value: { | ||
| candidates: [ | ||
| { | ||
| content: { | ||
| role: 'model', | ||
| parts: [{ text: 'ok' }], | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| } | ||
| return Promise.resolve(generate()); | ||
| }, | ||
| ); | ||
|
|
||
| vi.mocked(GeminiChat).mockImplementation( | ||
| () => | ||
| ({ | ||
| sendMessageStream: mockSendMessageStream, | ||
| }) as unknown as GeminiChat, | ||
| ); | ||
|
|
||
| await runForkedAgent({ | ||
| config: {} as Config, | ||
| userMessage: 'suggest something', | ||
| cacheSafeParams: getCacheSafeParams()!, | ||
| }); | ||
|
|
||
| const sendParams = capturedParams as { | ||
| config?: { tools?: unknown }; | ||
| }; | ||
| expect(sendParams.config!.tools).toEqual([]); | ||
| }); | ||
|
|
||
| it('strips tools when preserveTools is explicitly false', async () => { | ||
| saveCacheSafeParams( | ||
| { | ||
| tools: [ | ||
| { | ||
| functionDeclarations: [ | ||
| { name: 'edit', description: 'Edit a file' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| [], | ||
| 'test-model', | ||
| ); | ||
|
|
||
| let capturedParams: unknown = null; | ||
|
|
||
| const mockSendMessageStream = vi.fn( | ||
| (_model: string, params: unknown, _promptId: string) => { | ||
| capturedParams = params; | ||
| async function* generate() { | ||
| yield { | ||
| type: StreamEventType.CHUNK, | ||
| value: { | ||
| candidates: [ | ||
| { | ||
| content: { | ||
| role: 'model', | ||
| parts: [{ text: 'ok' }], | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| } | ||
| return Promise.resolve(generate()); | ||
| }, | ||
| ); | ||
|
|
||
| vi.mocked(GeminiChat).mockImplementation( | ||
| () => | ||
| ({ | ||
| sendMessageStream: mockSendMessageStream, | ||
| }) as unknown as GeminiChat, | ||
| ); | ||
|
|
||
| await runForkedAgent({ | ||
| config: {} as Config, | ||
| userMessage: 'suggest something', | ||
| cacheSafeParams: getCacheSafeParams()!, | ||
| preserveTools: false, | ||
| }); | ||
|
|
||
| const sendParams = capturedParams as { | ||
| config?: { tools?: unknown }; | ||
| }; | ||
| expect(sendParams.config!.tools).toEqual([]); | ||
| }); | ||
|
|
||
| it('throws when CacheSafeParams are not available', async () => { | ||
| const mockConfig = {} as unknown as Config; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,15 +9,20 @@ | |
| * | ||
| * The two execution paths are selected by whether cacheSafeParams is supplied: | ||
| * | ||
| * WITH cacheSafeParams → GeminiChat single-turn, NO tools, shares parent | ||
| * prompt cache (systemInstruction + history). | ||
| * WITH cacheSafeParams → GeminiChat single-turn, shares parent prompt | ||
| * cache (systemInstruction + history). Tools are | ||
| * stripped by default (NO_TOOLS) to prevent | ||
| * function calls; pass preserveTools: true to | ||
| * keep the parent's tools prefix for Anthropic | ||
| * prompt-cache hits. | ||
| * Use for: /btw, suggestions, pipelined suggestions. | ||
| * | ||
| * WITHOUT cacheSafeParams → AgentHeadless multi-turn, full tool access, | ||
| * isolated session (no shared history). | ||
| * Use for: memory extract, dream consolidation. | ||
| * | ||
| * Tool-deny for forked queries is enforced at the per-request level (NO_TOOLS). | ||
| * Tool-deny for forked queries is enforced at the per-request level (NO_TOOLS) | ||
| * unless the caller opts out via preserveTools to share the cache prefix. | ||
| * | ||
| * Callers (extractScheduler, dreamScheduler) own concurrency control. | ||
| * runSideQuery() remains a separate primitive for structured-JSON calls that | ||
|
|
@@ -37,6 +42,7 @@ import { ApprovalMode, type Config } from '../config/config.js'; | |
| import { GeminiChat, StreamEventType } from '../core/geminiChat.js'; | ||
| import { createRuntimeContentGeneratorView } from '../models/content-generator-config.js'; | ||
| import { createApprovalModeOverride } from '../tools/agent/agent.js'; | ||
| import { createDebugLogger } from './debugLogger.js'; | ||
| import { | ||
| AgentHeadless, | ||
| AgentEventEmitter, | ||
|
|
@@ -57,6 +63,8 @@ import { | |
| import { ToolNames } from '../tools/tool-names.js'; | ||
| import { runWithChatRecordingSuppressed } from './chat-recording-suppression-context.js'; | ||
|
|
||
| const debugLogger = createDebugLogger('FORKED_AGENT'); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // CacheSafeParams — shared prompt-cache slot | ||
| // --------------------------------------------------------------------------- | ||
|
|
@@ -145,7 +153,11 @@ export function clearCacheSafeParams(): void { | |
| // Forked chat — shared by runForkedAgent (cache path) and speculation | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /** Per-request config that strips tools so the model never produces function calls. */ | ||
| /** | ||
| * Per-request config that strips tools so the model never produces function | ||
| * calls. Applied by default in the cache path; skipped when preserveTools | ||
| * is true (to share the Anthropic prompt-cache prefix). | ||
| */ | ||
| const NO_TOOLS = Object.freeze({ tools: [] as const }) as Pick< | ||
| GenerateContentConfig, | ||
| 'tools' | ||
|
|
@@ -267,7 +279,8 @@ export async function runWithForkedChatModel<T>( | |
|
|
||
| /** | ||
| * Result from a cache-path runForkedAgent (with cacheSafeParams). | ||
| * Single-turn, text-only — tools are denied. | ||
| * Single-turn, text-only. Tools stripped by default; pass preserveTools | ||
| * to keep the parent's tools for cache-prefix matching. | ||
| */ | ||
| export interface ForkedQueryResult { | ||
| /** Extracted text response, or null if no text */ | ||
|
|
@@ -305,7 +318,7 @@ function extractQueryUsage( | |
| */ | ||
| export type ForkedAgentParams = CachePathParams | AgentPathParams; | ||
|
|
||
| /** Cache path: single-turn, tool-free, shares parent prompt cache. */ | ||
| /** Cache path: single-turn, shares parent prompt cache. */ | ||
| export interface CachePathParams { | ||
| /** Runtime config. */ | ||
| config: Config; | ||
|
|
@@ -319,6 +332,13 @@ export interface CachePathParams { | |
| model?: string; | ||
| /** External cancellation signal. */ | ||
| abortSignal?: AbortSignal; | ||
| /** | ||
| * When true, keep the parent's tools in the per-request config so the | ||
| * Anthropic prompt-cache key (system + tools) matches the main agent's. | ||
| * Default (false/omitted): strip tools via NO_TOOLS to prevent function | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in fa04bea! Updated the JSDoc to:
Thanks for catching the inconsistency.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in fa04bea! Updated the JSDoc to:
Thanks for catching the inconsistency. |
||
| * calls — appropriate for most forked queries. | ||
| */ | ||
| preserveTools?: boolean; | ||
| } | ||
|
|
||
| /** AgentHeadless path: multi-turn, full tool access, isolated session. */ | ||
|
|
@@ -418,7 +438,8 @@ function isMutatingFileTool(toolName: string): boolean { | |
| * Two overloads selected by the shape of `params`: | ||
| * | ||
| * params.cacheSafeParams present → cache path (ForkedQueryResult) | ||
| * Single-turn, NO tools, shares parent prompt cache. | ||
| * Single-turn, tools stripped by default (preserveTools overrides), | ||
| * shares parent prompt cache. | ||
| * Use for: /btw, suggestions, pipelined suggestions. | ||
| * | ||
| * params.taskPrompt present → agent path (ForkedAgentResult) | ||
|
|
@@ -436,8 +457,14 @@ export async function runForkedAgent( | |
| ): Promise<ForkedQueryResult | ForkedAgentResult> { | ||
| // ── Cache path ──────────────────────────────────────────────────────────── | ||
| if ('cacheSafeParams' in params) { | ||
| const { config, userMessage, cacheSafeParams, jsonSchema, abortSignal } = | ||
| params; | ||
| const { | ||
| config, | ||
| userMessage, | ||
| cacheSafeParams, | ||
| jsonSchema, | ||
| abortSignal, | ||
| preserveTools, | ||
| } = params; | ||
| const modelSelector = params.model ?? cacheSafeParams.model; | ||
| const modelRuntime = await buildForkedModelRuntime( | ||
| config, | ||
|
|
@@ -448,7 +475,9 @@ export async function runForkedAgent( | |
| return runWithForkedModelRuntime(modelRuntime, async (model) => { | ||
| const chat = createForkedChat(config, cacheSafeParams); | ||
|
|
||
| const requestConfig: GenerateContentConfig = { ...NO_TOOLS }; | ||
| const requestConfig: GenerateContentConfig = preserveTools | ||
| ? {} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Added in fa04bea:
All 57 tests pass (cache: 16, agent: 13, suggestion: 20, speculation: 8). |
||
| : { ...NO_TOOLS }; | ||
| if (abortSignal) requestConfig.abortSignal = abortSignal; | ||
| if (jsonSchema) { | ||
| requestConfig.responseMimeType = 'application/json'; | ||
|
|
@@ -471,8 +500,22 @@ export async function runForkedAgent( | |
| for await (const event of stream) { | ||
| if (event.type !== StreamEventType.CHUNK) continue; | ||
| const response = event.value; | ||
| const text = response.candidates?.[0]?.content?.parts | ||
| ?.filter((p) => !(p as Record<string, unknown>)['thought']) | ||
| const parts = response.candidates?.[0]?.content?.parts ?? []; | ||
|
|
||
| // Defensive: when preserveTools is true the model could produce | ||
| // functionCall parts instead of text. Log and discard them. | ||
| if ( | ||
| preserveTools && | ||
| parts.some((p) => (p as Record<string, unknown>).functionCall) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 2ab6f80 — changed both |
||
| ) { | ||
| debugLogger.warn( | ||
| 'Cache-path forked query received functionCall with preserveTools; discarding.', | ||
| ); | ||
| } | ||
|
|
||
| const text = parts | ||
| .filter((p) => !(p as Record<string, unknown>)['thought']) | ||
| .filter((p) => !(p as Record<string, unknown>).functionCall) | ||
| .map((p) => p.text ?? '') | ||
| .join(''); | ||
| if (text) fullText += text; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 2ab6f80 — new test
'passes preserveTools: true to runForkedAgent'inspeculation.test.ts. The test runs a text-only speculation to completion (triggeringgeneratePipelinedSuggestion), then assertsrunForkedAgentwas called withpreserveTools: true. All 9 tests in the file pass.