fix(flows): keep non-text static_instruction as a stable request prefix - #6653
Open
chelsealong wants to merge 2 commits into
Open
fix(flows): keep non-text static_instruction as a stable request prefix#6653chelsealong wants to merge 2 commits into
chelsealong wants to merge 2 commits into
Conversation
On the second and later turns, static_instruction content containing non-text parts (e.g. a PDF via file_data) was inserted after existing conversation history instead of staying at the front of the request. This broke the stable prefix that provider-side implicit context caching relies on. Track the user contents extracted from a Content passed to append_instructions (currently only static_instruction) separately, and insert them at the very beginning of the request, ahead of both the dynamic instruction and the conversation history. Fixes google#6652
…nd regression _add_instructions_to_user_content had a second call site (_finalize_dynamic_instructions in base_llm_flow.py), reached when the experimental DYNAMIC_INSTRUCTION_ROUTING feature is on and a tool (e.g. preload_memory_tool) contributes a dynamic instruction. Because the guard added in the previous commit checked a flag that is never reset, that second call also inserted at index 0, pushing the tool's content in front of the static prefix the first call had just placed there -- defeating the fix. Track the index right after the inserted static-instruction prefix and have subsequent calls insert there instead of at index 0, so later tool-triggered instructions land after the prefix without displacing it.
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.
Fixes #6652
Bug
When
LlmAgent.static_instructioncontains non-text content (e.g. a PDFprovided through
file_data), ADK extracts that content into a "user"content entry (via
LlmRequest.append_instructions) so it can be sent aspart of
contents.On the first turn this lands at the front of the request as expected. On
the second and later turns,
_add_instructions_to_user_contentincontents.pyinserts all instruction-related contents (the staticnon-text content and the dynamic instruction) right before the last
continuous batch of user content — i.e. after the existing conversation
history rather than at the front of the request:
This defeats the "stable prefix" that provider-side implicit context
caching relies on, so a large static PDF never gets a cache hit past the
first turn, as described in #6652.
Fix
LlmRequest.append_instructionsnow tracks the user contents it extractsfrom a
Contentargument (currently only used forstatic_instruction)in a new private list,
_static_instruction_contents, in addition toappending them to
contentsas before._add_instructions_to_user_contentincontents.pychecks this list: ifit's non-empty, the full set of instruction-related contents (static
content followed by the dynamic instruction, in the order they were
built) is inserted ahead of conversation history instead of before the
last user batch. This keeps the static content — and the dynamic
instruction that follows it — as a stable prefix ahead of conversation
history:
When
static_instructionhas no non-text parts (the common case, and theexisting behavior for pure dynamic-instruction agents), nothing changes:
instructions still get inserted right before the latest user turn.
_add_instructions_to_user_contentactually has a second call site:_finalize_dynamic_instructionsinbase_llm_flow.py, reached when theexperimental
DYNAMIC_INSTRUCTION_ROUTINGfeature is on and a tool (e.g.preload_memory_tool,load_artifacts_tool,load_mcp_resource_tool)contributes a dynamic instruction via
_append_dynamic_instructions. Thatcall happens later in the same turn's preprocessing, after the static
prefix has already been placed by the first call. An earlier version of
this fix inserted at a fixed index 0 on every call, which meant this
second call re-inserted at the front too, pushing the tool's dynamic
content in front of the static prefix the first call had just placed —
recreating the exact bug this PR fixes, one call later.
To fix that,
LlmRequestnow tracks_static_instruction_prefix_end_index, the index right after theinserted static-instruction prefix. Only the first call inserts at index
0 (and records where the prefix ends); any later call for the same
request inserts right after that tracked index instead, so tool-triggered
dynamic instructions land after the static prefix without displacing it.
Testing plan
Added
test_static_instruction_file_precedes_multi_turn_historyintests/unittests/flows/llm_flows/test_instructions.py, which reproducesthe multi-turn scenario from the issue (static file_data instruction +
dynamic instruction + 3 turns of history) and asserts the static content
and dynamic instruction both precede the history.
Added
test_static_instruction_file_stays_prefix_after_tool_dynamic_instruction,which reproduces the second-call-site regression: after the main content
processor places the static prefix, it simulates a tool-triggered dynamic
instruction the way
_finalize_dynamic_instructionsdoes, and asserts thestatic content is still at index 0 afterward.
Verified both new tests fail without their respective fixes:
With the fix:
Also verified formatting with
pyinkand import order withisort(project's pinned versions) — no changes needed.
AI assistance disclosure
This change was authored with the help of an AI coding agent (Claude),
with the diff reviewed and the reproduction/test verified before being
pushed. An earlier version of this PR was caught by review as fixing the
bug in the main content-processor call path while leaving a second call
path (tool-triggered dynamic instructions under the experimental
DYNAMIC_INSTRUCTION_ROUTINGfeature) able to re-break the sameinvariant; that gap has been closed and covered by a new regression test.