[fix]: Gemini - send the provider the tool-call id it issued, not the signature-joined one - #8565

No actionable comments were generated in the recent review.

Merge Risk: Low · up to 1d22c

The change makes Gemini and Vertex receive the original tool-call ID, with the signature kept separately. Well-formed IDs pair correctly. A narrow edge case with malformed signature suffixes remains deferred to a follow-up, and the live harness has not been run, so merging carries low risk.

Security architecture risk: Low · up to 1d22c

The change removes signatures from provider-facing IDs while preserving their separate transport. A conditional tool-result pairing regression remains when different call IDs collapse to the same base ID. No privilege expansion or cross-tenant exposure was established.

Retained concerns

• Medium · reliability · inferred: Normalizing IDs before response ordering can lose tool-result ownership. Distinct full IDs with the same base receive the same ordering rank, so reversed outputs retain their input order instead of following their calls. Vertex then removes both IDs and relies on positional pairing. The existing optional-ID fallback to the function name provides a possible producer path for these collisions when repeated same-name calls carry different signature suffixes. Before this PR, distinct full IDs remained distinguishable during ordering. This is a conditional result-integrity regression, not a verified authorization bypass.

Security review details

Security Blast Radius

• inferred — The demonstrated collision mechanism is confined to correlation within a supplied request history and its downstream provider request. Cross-request state mixing, cross-tenant access, and additional tool-execution privileges were not established by the inspected flow.

Security Findings and Attack Paths

• inferred — A history containing lookup_ts_AQ and lookup_ts_Ag illustrates the ownership failure: both normalize to lookup, so reversed outputs tie during stable sorting. Vertex subsequently removes the IDs. This establishes a conditional attribution defect, but not an independently verified security exploit; downstream security-sensitive use of misattributed outputs was not traced.

Trust Boundaries and Controls

• observed — Signature decoding and the existing missing-signature fallback remain separate from ID normalization. The edited paths do not add a new authentication or authorization check, nor does stripping establish cryptographic validity of caller-supplied signatures.

Hardening Proposals

• proposed — Preserve full correlation identities through response ordering and normalize only the provider-facing representation afterward. Define explicit handling for duplicate normalized bases so signature removal cannot silently change tool-result ownership.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 4 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the primary fix: Gemini receives the provider-issued tool-call ID instead of the signature-joined ID.
Description check		The description is complete and follows the repository template. It explains the issue, changes, affected routes, tests, limitations, breaking-change status, related issues, security impact, and check…
Linked Issues check		Issue #8546 requires the base tool-call ID in Gemini and Vertex functionCall.id and functionResponse.id, while preserving the signature in thoughtSignature and the client-facing joined ID. The t…
Out of Scope Changes check		The changed code, regression tests, benchmark, changelog entry, and provider-harness rows all support issue #8546 or validate its Gemini and Vertex request paths. No unrelated change is demonstrated.

Full details: Description check

Explanation

The description is complete and follows the repository template. It explains the issue, changes, affected routes, tests, limitations, breaking-change status, related issues, security impact, and checklist results.

Full details: Linked Issues check

Explanation

Issue #8546 requires the base tool-call ID in Gemini and Vertex functionCall.id and functionResponse.id, while preserving the signature in thoughtSignature and the client-facing joined ID. The two shared converters strip both IDs. The tests cover Responses, Chat Completions, Vertex entry points, signature preservation, and response pairing. The full-ID name maps remain unchanged.

Full details: Docstring Coverage

Explanation

Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

@Vivek1898 can you fix the comments - also share the result of the harness run?

@akshaydeo rebased onto dev (9bcea6777) and pushed — comments addressed below. Conflict was only provider-harness.json, where upstream appends at the same tail.

Tests moved into an existing file

thoughtsignatureproviderids_test.go is gone; all four tests and the benchmark now live in core/providers/gemini/responses_test.go. The chat-completions one is there too rather than in chat_test.go, for two reasons: the tests use the package-private thoughtSignatureSeparator and convertResponsesMessagesToGeminiContents, so they have to be package gemini and chat_test.go is package gemini_test; and they share two helpers, which would otherwise be duplicated. Say the word if you'd rather they split and the helpers be exported for the test.

core/providers/gemini passes in full (2.9s), go vet clean, gofmt clean.

Harness folder renumbered 244 → 252

Upstream's numbered folders now run to 251, so 244 collided. The feature keyword still selects exactly the six rows:

[filter-collection] wrote ... with 6 requests after filter (feature=gemini-call-id-strip)

The harness run

Per AGENTS.md I haven't run it — the live run is yours to trigger. Both commands, ready to paste:

make dev

make run-provider-harness-test PROVIDER=gemini FEATURE="gemini-call-id-strip"

One thing worth knowing before you run it: the replay rows need a live-captured signature, because Gemini validates it — a synthetic one is rejected, which is why each pair is capture-then-replay rather than a fixed fixture.

CodeRabbit's note on invalid signature suffixes

This one I have not changed, and I'd rather flag the reasoning than quietly skip it. The suggestion is to validate the suffix before stripping, so call_ts_! stays intact instead of becoming call.

• StripThoughtSignature is a shared util, already used on the OpenAI path (#4810, #7654). Adding validation changes OpenAI's behaviour too, which is outside this fix.
• For a well-formed joined id the first-separator cut is already correct: the base id comes first, so a _ts_ occurring inside a base64url signature doesn't mislead it. The existing table covers that (fc_123_ts_QUJD → fc_123).
• The only way to reach the collision is a client fabricating an id containing _ts_, and the determinism the doc comment promises still holds — call and response strip identically, so pairing survives.
• The tradeoff looks deliberate: the comment on ReasoningItemIDMarker right below explains why that one uses a prefix checked at offset 0 rather than an infix, and contrasts it with ThoughtSignatureSeparator.

So I think it's a real but pre-existing, accepted property of the shared helper rather than something this PR introduces. If you want it tightened I'd rather do it in its own PR covering both providers, with the OpenAI call sites and their tests — happy to open that.

One leak I did not change

funcName still falls back to the raw call id when the name lookup misses (responses.go ~5259, utils.go ~2191). On a miss the signature-joined id would reach Gemini as a function name. I left it alone because it's a different bug with a different fix, but flagging it since it's adjacent.

@akshaydeo pushed 3d62541, which fixes a real defect CodeRabbit found in my own harness rows — worth flagging since it would have shown up as a false red in exactly the run you asked for.

The three replay rows asserted pm.response.text(), the whole body, must not contain _ts_. But Bifrost re-joins the signature onto a tool-call id it issues, so any correct run that emits a new tool call carries _ts_ in an id field and the assertion fails on it. Each row now extracts only model-authored output for its route and scans that, with every generated id field excluded, plus a non-empty assertion on the extraction so narrowing the scan cannot make it vacuous.

Verified with newman 6.2.3 against canned bodies for all three routes, driving the exec arrays straight out of the collection: the scan fails when the separator is in the model's text, passes when it appears only in a generated tool-call id, and fails the non-empty guard on empty output — 9 of 9. The old one-liner fails all three id-only bodies.

I also measured CodeRabbit's other harness point and it does not apply: those indexOf('{{') skip guards sit in test scripts, not pre-request scripts, so the body is already substituted by then. Positive and negative probe in that thread. The pattern is also already on dev 30 times over; mine were copied from it, so I left the idiom alone rather than introduce a second one.

Still green: full core/providers/gemini package, go vet, and the feature filter selects 6 requests.

On the harness run itself — I can't run it, since it needs live provider credentials and a real Gemini turn to capture a valid signature. Commands ready to paste:

make dev

make run-provider-harness-test PROVIDER=gemini FEATURE="gemini-call-id-strip"

Rebased onto dev (34269e7) — the PR had gone red as conflicting. New head 3a972f41e; the commit I referenced above as 3d62541 is that same change re-applied, now 3a972f4.

The conflict was provider-harness.json, and only that file. Folder numbers 253–255 went to #8604–#8607 while I was rebasing, so mine is now 256. The FEATURE keyword is unchanged — it keys off gemini-call-id-strip, not the number.

I resolved it by lifting only the text my commit adds and re-applying it to upstream's copy rather than hand-editing a 273k-line file, then checked the result structurally instead of eyeballing it:

• 333 top-level folders = upstream's 332 + mine
• every one of upstream's 332 folders byte-identical to dev's copy
• my folder byte-identical to the pre-rebase version apart from its number, 6 rows

core/providers/gemini   ok          go vet  clean
filter-collection       6 requests after filter (feature=gemini-call-id-strip)

Worth saying plainly: this will go red again on its own

Every PR that adds a harness case appends to the same tail of provider-harness.json, so any PR carrying a folder conflicts with the next one merged. Six landed in the ~90 minutes I spent on this rebase, and all six touched that file. So the state of this PR is not a signal about the branch — it will read conflicting again shortly with nothing changing on either side.

Two ways I can make that not your problem, your call:

1. Ping me when you're ready to merge and I'll re-resolve immediately — it's a scripted splice now, a couple of minutes.
2. I move folder 256 to a follow-up PR, leaving this one as the Go fix plus unit tests. Those files nobody else is touching, so it would stay mergeable indefinitely. I haven't done this unilaterally because AGENTS.md asks for a provider-harness case per route, and dropping it from the PR that adds the route behaviour is your decision, not mine.

On the harness run

That one I genuinely can't produce: it needs live Gemini credentials, and the row that matters also needs a real captured thought signature. Both commands, unchanged:

make dev

make run-provider-harness-test PROVIDER=gemini FEATURE="gemini-call-id-strip"

If there's a unit-level shape you'd rather see pinned while that waits, name it and I'll add it.

Pushed 1d22c5f, rebased onto dev (0b9ca04). CodeRabbit's latest pass found two genuine holes in my own harness rows and I've fixed both — details and measurements in the threads, summary here.

Both were ways the pair could report green without exercising the fix.

I measured both directions by driving the shipped exec arrays against canned bodies rather than reasoning about them — 18 cases over the three routes: correct replay passes 3/0, a joined echo fails on the separator, an unrelated reply fails on the echo requirement, an empty extraction fails on the vacuity guard, a bare captured id fails at capture. The two "before" rows above are the old exec arrays run against the same bodies, which is how I know the holes were real and not theoretical.

The third thread (field order through a JSON round trip) I've pushed back on with a measurement — in JS the round trip is order-preserving and the rows don't use Object.fromEntries. Resolved so the gate clears; happy to be overruled.

Also in this push:

• core/providers/gemini/responses_test.go had a real conflict, not just the collection — feat: map prompt cache key to session id in vertex provider #8650 appended TestGenAISessionIDRidesAsPromptCacheKey at the same EOF. Both sides are pure additions, so I kept both rather than picking a side; upstream's test is intact and passing.
• Folder renumbered to 259 (256–258 went to feat: map prompt cache key to session id in vertex provider #8650 and friends while this sat).

core/providers/gemini   ok      go vet  clean      gofmt  clean
filter-collection       6 requests after filter (feature=gemini-call-id-strip)

The live harness run is still the one thing I can't produce — it needs Gemini credentials and a real captured thought signature:

make dev
make run-provider-harness-test PROVIDER=gemini FEATURE="gemini-call-id-strip"

Standing offer from my last comment: this will go red as conflicting again on its own, since every harness PR appends to the same tail. Ping me when you want to merge and I'll re-resolve, or I'll split the folder into a follow-up PR so the Go fix stays mergeable — your call given AGENTS.md.