fix(gemini): give a new native tool call its own slot instead of colliding on part_index - #75528
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating this to the native adapter rather than adding a downstream repair pass. The current remote main still constructs its stream key from the per-event part_index at agent/gemini_native_adapter.py:742-750, so the source-side collision is real.
Problems
agent/gemini_native_adapter.py:765only derives a new suffix from the base key. After calls A then B createbaseandbase#1, a resend of B starts frombaseagain and allocatesbase#2; it cannot reachbase#1for the existing dedup path. Please preserve deduplication for collision-created slots as well.tests/agent/test_gemini_native_adapter.py:357-368covers[A, A], but needs[A, B, B]to exercise that secondary-slot replay.
Suggested changes
- Remove the leftover insertion instructions at
tests/agent/test_gemini_native_adapter.py:262-265.
The PR base is only one commit behind remote main and neither modified file changed, so this remains a focused, high-salvageability fix. This is an automated hermes-sweeper review.
| except (json.JSONDecodeError, TypeError, ValueError): | ||
| pass | ||
| else: | ||
| call_key = f"{call_key}#{len(tool_call_indices)}" |
There was a problem hiding this comment.
This allocates base#N without checking collision-created slots. With events [A, B, B], the second B starts from base (whose arguments are A) and becomes base#2, so it is not deduplicated with base#1. Please retain/locate the secondary slot before allocating another one, and add that replay regression.
| @@ -259,3 +259,137 @@ def test_stream_event_translation_emits_tool_call_delta_with_stable_index(): | |||
|
|
|||
|
|
|||
|
|
|||
| # Bloque de tests para anadir al final de | |||
There was a problem hiding this comment.
Please remove these leftover Spanish instructions for inserting the test block; they are contributor-process notes rather than test documentation.
Same mechanism as the earliest-open canonical fix #24676 (identical |
|
Both review points are addressed in f1e0f8c.
The lookup now walks the slots already derived from the same key before allocating another one, so a continuation or a resend lands on the slot that call opened. The rule deciding it is unchanged and now lives in one helper, Replay regression added. Leftover notes removed from the top of the test block. I also corrected the On the duplicate label — happy for a maintainer to pick the canonical implementation. One data point in case it is useful: the 2026-07-13 review on #24676 found that its regression test sends the same complete CI has never run on this PR: every workflow is |
|
Verified against current main (75901a2) as part of the duplicate-cluster triage for #72488. Premise holds and the fix works: Premise (reproduced, no network): Fix (cherry-picked onto current main, additive, no conflicts): all three synthetic cases now yield one valid slot per call ( Tests ( Green-light from the #72488 cluster triage: this is the source-side fix the #72489 close verdict pointed to (native adapter, not downstream reconstruction). |
…iding on part_index
Two different calls to the same tool arriving in separate stream events
collide in one accumulator slot in the native Gemini adapter, because
`call_key` is built from `part_index`, which restarts at 0 on every
event. Their arguments are then emitted under the same index and
concatenated downstream into unparseable JSON (`{"query": "A"}{"query":
"B"}`), and the call is dropped.
Same-event parallel calls are unaffected: each part gets its own
`part_index`, hence a distinct key. Only calls split across events
collide, which is the tell that this concatenation is produced inside
Hermes rather than by the model.
When a payload arrives for an existing slot, is not an extension of the
arguments already accumulated there, and those arguments are a complete
JSON object, it is a new call and gets its own slot. The "already parses
as complete JSON" guard keeps partial-argument streaming working: a
half-sent object does not parse, so it stays in its slot and keeps
accumulating.
Refs NousResearch#72488
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review on NousResearch#75528: after calls A and B open `base` and `base#1`, a resend of B restarts its lookup from `base`, whose accumulated arguments are A's. It matched nothing there and allocated `base#2`, so the resent call was emitted under a third index instead of being deduplicated into the slot it had already opened — replaying `[A, B, B]` produced three tool calls for two. The slot lookup now walks the slots derived from the same key before allocating another one, so a continuation or a resend lands on the slot that call opened. The acceptance rule that decides it is unchanged and now lives in one place: a slot takes a payload that extends or repeats the arguments it already holds, or that follows arguments which are not yet complete JSON. Also drop the leftover contributor notes at the top of the new test block, and correct the `_accumulate` docstring, which named `run_agent.py` as the streaming loop instead of `_stream_completion`. Refs NousResearch#72488 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f1e0f8c to
653f626
Compare
|
Rebased onto current What the conflict was. The premise still holds on today's Re-verified against
So the defect still reproduces on current Still purely additive: +198 / −0 across two files, no existing line modified or deleted. CI has never run on this PR — every workflow sits at |
What does this PR do?
Two different calls to the same tool arriving in separate stream events collide in one
accumulator slot in the native Gemini adapter. Their arguments are emitted under the same
indexand concatenated downstream into unparseable JSON ({"query": "A"}{"query": "B"}),and the call is dropped.
This is the source-side defect pointed at when #72489 was closed under the standing
model-output-repairpolicy:That is what this PR is. No repair or reconstruction pass is added — the call boundary is
simply never discarded in the first place.
Where it manifests on current
mainIn
translate_stream_event(agent/gemini_native_adapter.py:718), the dedup key is builtfrom
part_index(742-749):part_indexcomes fromfor part_index, part in enumerate(parts)— it restarts at 0 onevery stream event. Two different calls to the same tool, arriving in two events, produce
an identical
call_keyand land in the same slot.The accumulator (759-764) then emits both under one index:
A genuinely different call is neither equal nor a prefix extension, so the full
{"query": "B"}is emitted under the sameindexas{"query": "A"}.The tell: same-event parallel calls work fine — each part gets its own
part_index,hence a distinct key. Only calls split across events collide. If the model were the one
concatenating, both cases would fail identically.
Why this approach
When a payload arrives for an existing slot, is not an extension of what is already there,
and what is already there is a complete JSON object, it is a new call and gets its own slot.
The "already parses as complete JSON" guard is what keeps partial-argument streaming working:
a half-sent object does not parse, so it stays in its slot and keeps accumulating.
This mirrors the existing Ollama workaround in
agent/chat_completion_helpers.py(
_last_id_at_idx/_active_slot_by_idx, 3244-3260), which solves the sibling problem of anendpoint reusing index 0 for a whole parallel batch.
Scope — what this deliberately does not claim
idonthe same raw index already redirects to a fresh slot. This PR does not touch it. Whether
Gemini's OpenAI-compat endpoint can still collide when it sends
index=Noneand omits idsis not something I can demonstrate — I run the native provider and have no trace of that
endpoint, so I am not asserting it.
that produces the symptom on its own, from well-formed input. Both causes can coexist.
argsis always serialized from a complete dict(
json.dumps(fc.get("args") or {}, ..., sort_keys=True)), so in practicestartswithis onlyever true for an identical resend and
_prevalways parses. The two guards are thereforedefensive rather than load-bearing today — they are what keeps this safe if a partial
argument string ever reaches this accumulator. Behaviour changes only in cases that currently
produce invalid JSON; every case that works today still works.
Related Issue
Fixes #72488
Type of Change
Changes Made
agent/gemini_native_adapter.py— intranslate_stream_event, when an incoming payload isnot an extension of the arguments already accumulated in the slot and those arguments are a
complete JSON object, allocate a fresh slot instead of reusing the colliding one (16 lines).
tests/agent/test_gemini_native_adapter.py— six tests covering the defect and thebehaviours that must not regress.
How to Test
Deterministic, no network and no model — synthetic well-formed stream events, each carrying one
valid
functionCallpart.git checkout HEAD~1 -- agent/gemini_native_adapter.pypytest tests/agent/test_gemini_native_adapter.py -q→ 2 failed, 12 passed.git checkout HEAD -- agent/gemini_native_adapter.py) and re-run→ 14 passed (8 pre-existing + 6 new).
Test matrix (behaviour on
main, without this change):maintest_same_tool_called_twice_across_events_gets_distinct_slotstest_three_calls_to_same_tool_across_events_each_get_a_slottest_parallel_calls_in_one_event_keep_workingtest_different_tools_across_events_keep_workingtest_identical_resend_is_still_deduplicated_into_one_slottest_partial_json_arguments_keep_accumulating_in_one_slotChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — partially, see noteDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Reproduction against
mainat126ff7071— this branch with the adapter hunk reverted, so theonly thing running is stock
translate_stream_eventfed synthetic, well-formed stream events:One slot where there should be two, and one where there should be three — with the arguments of
distinct calls concatenated into a string that no JSON parser will accept. Restoring the adapter
hunk gives
14 passed.This fix has been running in production here since 2026-07-29 (Telegram gateway, native Gemini
provider) with no regressions observed.