fix(litellm): strip embedded thought_signature from tool call id#6460
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
Reviewed exact head 96aac45a. The focused thought-signature tests pass (17/17) and the full LiteLLM model suite passes (352/352). I found one persistence-safety edge where an undecodable __thought__ suffix still causes the opaque tool-call ID to be truncated; details and a minimal reproducer are inline.
| """ | ||
| if not tool_call_id: | ||
| return tool_call_id | ||
| return tool_call_id.split(_THOUGHT_SIGNATURE_SEPARATOR, 1)[0] |
There was a problem hiding this comment.
Please only strip the suffix when it was successfully recognized as an embedded signature. _extract_thought_signature_from_tool_call() rejects malformed base64, but this helper truncates every ID containing the separator. At this head I reproduced call_keep__thought__not-base64 becoming function_call.id == "call_keep" while part.thought_signature is None. That mutates the opaque ID before it is persisted even though no signature was extracted; the Gemini guidance also requires returning the exact function-call ID in the response. Could the ID and signature be parsed together (or the raw ID retained when decoding fails), with a malformed-suffix regression test?
There was a problem hiding this comment.
Please only strip the suffix when it was successfully recognized as an embedded signature.
_extract_thought_signature_from_tool_call()rejects malformed base64, but this helper truncates every ID containing the separator. At this head I reproducedcall_keep__thought__not-base64becomingfunction_call.id == "call_keep"whilepart.thought_signature is None. That mutates the opaque ID before it is persisted even though no signature was extracted; the Gemini guidance also requires returning the exact function-call ID in the response. Could the ID and signature be parsed together (or the raw ID retained when decoding fails), with a malformed-suffix regression test?
Good catch, thanks! You're right the separator can be part of the id itself. I changed it to only strip when the suffix actually decodes as a signature, otherwise the id is left alone. call_keep__thought__not-base64 stays intact now (signature None) and I added tests for it.
When a Gemini thinking model exposes its thought_signature only embedded in the tool call id via the __thought__ separator, _message_to_generate_content_response extracted the signature onto part.thought_signature but assigned the full, unsplit id (including the __thought__<signature> suffix) to part.function_call.id. That corrupted id is persisted to session history and, on later turns, replayed to the model as a literal tool_call_id, which OpenAI-compatible endpoints reject (422 'Tool Call ID required on tool calls'). Because the malformed id is now part of the event history, every subsequent turn on the thread fails identically, permanently breaking the conversation. Strip the embedded suffix so function_call.id is the clean, original id, while still preserving the signature separately on part.thought_signature. The extra_content and provider_specific_fields paths are unchanged. Adds regression and unit tests. Fixes google#6454
…nature Address review feedback: _strip_thought_signature_from_tool_call_id previously truncated any id containing the __thought__ separator, even when the suffix was not a valid base64 signature. Since the separator can legitimately appear inside an opaque tool call id, that mutated ids from which no signature was actually extracted (e.g. 'call_keep__thought__not-base64' became 'call_keep' while thought_signature stayed None), contradicting the Gemini requirement to echo back the exact function-call id. Now the suffix is decoded first (mirroring _extract_thought_signature_from_tool_call); the id is only stripped when the suffix is a genuine signature, otherwise it is returned unchanged. Adds malformed-suffix regression tests at both the helper and _message_to_generate_content_response levels.
2b44e59 to
15bbca8
Compare
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 15bbca823e48. The malformed-base64 case is fixed, and the seven focused tests pass locally. One empty-signature boundary remains inline. Separately, uv run pyink --check src/google/adk/models/lite_llm.py tests/unittests/models/test_litellm.py reports that the test file still needs formatting.
| base_id, _, encoded_signature = tool_call_id.partition( | ||
| _THOUGHT_SIGNATURE_SEPARATOR | ||
| ) | ||
| if _decode_thought_signature(encoded_signature) is None: |
There was a problem hiding this comment.
[P2] Preserve IDs when the decoded signature is empty
The malformed-base64 case is fixed, but an empty suffix still breaks the same invariant. base64.b64decode("") returns b"", so call_keep__thought__ reaches return base_id here and becomes call_keep; later _message_to_generate_content_response() uses if thought_signature:, so that empty value is not persisted on part.thought_signature. I reproduced _decode_thought_signature("") == b"" and _strip_thought_signature_from_tool_call_id("call_keep__thought__") == "call_keep" on this head. Please retain the raw ID when the decoded value is empty (or parse once and base stripping on the same value that will actually be stored), and add this boundary as a regression test.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
N/A — this PR resolves the existing issue linked above.
Problem:
Gemini "thinking" models attach a
thought_signatureto their function-callparts. Depending on the provider path, that signature can arrive in one of
three places, which
_extract_thought_signature_from_tool_callalready handles:extra_content.google.thought_signature(OpenAI-compatible API)provider_specific_fieldson the tool call or its function (Vertex)__thought__separator(
call_789__thought__<base64-signature>) — the shape some upstream proxypaths return for Gemini 2.5 thinking models.
For paths 1 and 2 the tool call ID is never touched. For path 3, however,
_message_to_generate_content_responseinsrc/google/adk/models/lite_llm.pyextracted the signature ontopart.thought_signaturebut then assigned the entire, unsplit ID(including the
__thought__<signature>suffix) topart.function_call.id:Consequences:
function_call.idbecomes a several-hundred-character string containing raw,non-URL-safe base64 characters (
/,+).That corrupted ID is persisted verbatim into session history (e.g. a
DatabaseSessionService-backed events row).On any later turn where the authoring agent's own history is replayed, the
malformed ID is sent back to the model as a literal
tool_call_idandrejected by OpenAI-compatible endpoints / proxies:
Because the corrupted ID is now a permanent part of the session's event
history, every subsequent turn on that thread fails identically,
permanently breaking the conversation.
This is consistent with the official guidance that a thought signature is a
separate field, distinct from the function-call identifier, and must be
preserved as such across turns
(Gemini thinking guide — thought signatures).
The bug was introduced by the change that added
thought_signaturepreservation itself; it is not a regression of previously correct behavior.
Solution:
Strip any embedded
__thought__suffix from the tool call ID before assigningit to
function_call.id, so the persisted ID is the clean, original identifier— while the signature continues to be preserved separately on
part.thought_signature.A small helper mirrors the existing extraction logic (splitting on the first
__thought__separator, exactly as_extract_thought_signature_from_tool_calldoes):
and is applied at the assignment site:
This solution was chosen because:
malformed ID is ever persisted, rather than papering over it downstream.
unchanged, so the
extra_contentandprovider_specific_fieldspaths and allplain tool-call IDs are unaffected.
leaves outbound LiteLLM re-embedding untouched.
Testing Plan
Unit Tests:
Added to
tests/unittests/models/test_litellm.py:test_message_to_generate_content_response_strips_thought_signature_from_id— the core regression: an embedded-ID tool call yields a clean
function_call.id(call_789) whilethought_signatureis still preserved.test_strip_thought_signature_from_tool_call_id_removes_suffixtest_strip_thought_signature_from_tool_call_id_leaves_plain_idtest_strip_thought_signature_from_tool_call_id_handles_empty(None/"")test_strip_thought_signature_from_tool_call_id_splits_onceThe core regression test was verified to fail before the fix (proving it
reproduces the bug) and pass after the fix.
Summary of passed
pytestresults:Manual End-to-End (E2E) Tests:
The failure is in pure data-handling logic, so it is fully reproducible without
a live model. Minimal reproduction:
Before the fix, the printed ID retains the
__thought__<signature>suffix;after the fix it is the clean
call_789, and the signature is still present onpart.thought_signature.Checklist
Additional context
src/google/adk/models/lite_llm.py— new_strip_thought_signature_from_tool_call_idhelper and its use in_message_to_generate_content_response.tests/unittests/models/test_litellm.py— regression and unit tests.extra_contentandprovider_specific_fieldsextraction paths, and theoutbound tool-call serialization, are left unchanged.