fix(agent): keep the turn's final call in the same cache lineage - #940
Merged
Conversation
Closes the measurement question #933's test plan deferred, by removing the condition that made it ambiguous rather than by measuring it. The finalization call — the one that extracts an answer after the loop exhausts its iterations — went out with Tools: nil. Providers render tools -> system -> messages into one prefix-matched cache key, so a tools-absent request cannot read the prefix the tool-bearing iterations just wrote, and pays its own cache write at the 1.25x premium for an entry no following call in that turn can read back. Keeping the tool list and suppressing it with tool_choice=none puts the call back in the turn's lineage. This is also the first time ToolChoiceNone actually reaches the wire: every provider emits tool_choice only inside `if len(tools) > 0` (anthropic.go, openai_compat_client.go, gemini_native_convert.go all share the shape), so with Tools: nil the "none" was silently dropped and suppression relied entirely on there being no tools to call. Dropping the tools is what made a text answer structural rather than a request the provider might ignore, so that guarantee is kept as a fallback: if the finalization response comes back with tool calls and no content, retry once the old way. Without it that case degrades to the max-iterations error instead of an answer. Costs an extra round trip only when a provider misbehaves. TestLoop_Run_FinalizesWithoutToolsWhenMaxIterationsReached asserted the mechanism (tool count 0); it now asserts the outcome it was protecting, with the ignore-none path covered by its own test. Verified: make test green, make lint-diff 0 issues, diff coverage 100% (28/28). CLI providers are unaffected — claude-code-cli and antigravity-cli never read ChatOptions.Tools.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Resolves the open question from the #933 review — does the tools-absent final call pay a cache-write premium for an entry nothing reads back? — by removing the condition that made it ambiguous, rather than by measuring it.
The finalization call (the one that extracts an answer after the loop exhausts its iterations,
internal/agent/loop.go:428) went out withTools: nil. Providers rendertools→system→messagesinto one prefix-matched cache key, so that request:Keeping the tool list and suppressing it with
tool_choice: noneputs the call back in the turn's lineage.A side effect worth stating plainly
This is the first time
ToolChoiceNone()actually reaches the wire. Every provider emitstool_choiceonly insideif len(tools) > 0:internal/llm/anthropic.go:171internal/llm/openai_compat_client.go:108internal/llm/gemini_native_convert.go(same shape)So with
Tools: nil, thenonewas silently dropped on every provider and suppression rested entirely on there being no tools to call.The guarantee that had to be preserved
Dropping the tools is what made a text answer structural rather than a request the provider might ignore.
TestLoop_Run_FinalizesWithoutToolsWhenMaxIterationsReachedpinned that, though it asserted the mechanism (toolCount == 0) rather than the outcome.So the old behavior is kept as a fallback: if the finalization response comes back with tool calls and no content, retry once with the tools removed. Without it, a provider that ignores
nonedegrades that turn into theexceeded max iterationserror instead of an answer. The extra round trip happens only when a provider misbehaves.The existing test now asserts the outcome it was protecting; the ignore-
nonepath has its own test.Provider impact
This is
internal/agent/loop.go, so it affects every provider — not just Anthropic:ToolChoiceModeNonemaps to{"type":"none"}"none"Mode: "NONE"ChatOptions.ToolsThe cost tradeoff: the tools block is now sent on the final call too. On a caching provider it is read back cheaply; on a non-caching one it is a modest input increase, since the same block already went out on every loop iteration.
Test plan
tool_choice=nonenonetriggers the tools-removed retry and still answersgot [1 0])make test— full suite greenmake vet, gofmt cleanmake lint-diff— 0 issuesmake test-cover-diff— 100.0% (28/28 changed lines)🤖 Generated with Claude Code