fix(ai-gemini): dedup functionResponse by id, not name#960
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe Gemini text adapter now deduplicates ChangesGemini response handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai-gemini/tests/parallel-function-response.test.ts`:
- Line 23: Update the return type of functionResponseIds to use Array<string>
instead of string[], preserving the function’s behavior.
- Around line 1-4: Move parallel-function-response.test.ts alongside the covered
GeminiTextAdapter source under src/adapters, rename it appropriately if needed,
and update its relative imports to match the new location. Ensure imports in the
relocated test follow the project’s import/order convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea858c55-5484-439f-8030-5581672eab46
📒 Files selected for processing (3)
.changeset/ai-gemini-parallel-function-response-dedup.mdpackages/ai-gemini/src/adapters/text.tspackages/ai-gemini/tests/parallel-function-response.test.ts
| import { describe, expect, it } from 'vitest' | ||
| import type { ModelMessage } from '@tanstack/ai' | ||
| import type { Content } from '@google/genai' | ||
| import { GeminiTextAdapter } from '../src/adapters/text' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move test file alongside the source it covers and fix import order.
As per coding guidelines, "Place unit tests in *.test.ts files alongside the source they cover." This test file is currently in the tests/ directory but covers src/adapters/text.ts. Please move this file to packages/ai-gemini/src/adapters/ (e.g., as text.test.ts) and update the relative imports accordingly. (Note: While previous conventions placed tests in a dedicated tests/ directory as noted in retrieved learnings, this explicit coding guideline takes precedence).
Additionally, static analysis indicates an import/order violation. If you keep the file in its current location temporarily, ensure the imports are ordered correctly.
🐛 Proposed fix for import order
import { describe, expect, it } from 'vitest'
+import { GeminiTextAdapter } from '../src/adapters/text'
import type { ModelMessage } from '`@tanstack/ai`'
import type { Content } from '`@google/genai`'
-import { GeminiTextAdapter } from '../src/adapters/text'🧰 Tools
🪛 ESLint
[error] 4-4: ../src/adapters/text import should occur before type import of @tanstack/ai
(import/order)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai-gemini/tests/parallel-function-response.test.ts` around lines 1 -
4, Move parallel-function-response.test.ts alongside the covered
GeminiTextAdapter source under src/adapters, rename it appropriately if needed,
and update its relative imports to match the new location. Ensure imports in the
relocated test follow the project’s import/order convention.
Sources: Coding guidelines, Learnings, Linters/SAST tools
`GeminiTextAdapter.mergeConsecutiveSameRoleMessages` deduplicated `functionResponse` parts by tool name. When Gemini returned two or more parallel calls to the same tool in one turn, the second matching response was filtered out, so the follow-up request 400'd: INVALID_ARGUMENT: Please ensure that the number of function response parts is equal to the number of function call parts of the function call turn. Each `functionResponse` is already built with a unique `id` (the originating `toolCallId`), so keying the dedup on `id` keeps parallel calls distinct (different ids → both kept) while still collapsing a genuine duplicate (same id → second one dropped). The pre-existing comment already described the key as "tool call ID" — this brings the implementation in line with that comment. Fixes TanStack#894.
f4119be to
eb10a60
Compare
|
@coderabbitai thanks for the review. Both lint nitpicks addressed in 1. 2. On moving the test to Local verification on
(Side note: |
|
Tip For best results, initiate chat on the files or code changes.
Agreed on retaining 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
GeminiTextAdapter.mergeConsecutiveSameRoleMessagesdeduplicatedfunctionResponseparts by tool name. When Gemini returned two or more parallel calls to the same tool in one turn, the second matching response was filtered out, so the follow-up request 400'd:Fixes #894.
Root cause
packages/ai-gemini/src/adapters/text.ts,mergeConsecutiveSameRoleMessages(~L852):Two parallel calls to the same tool share a
namebut have differentids, so the second response is dropped. The comment above the filter already called the key "tool call ID", but the implementation was actually keying onname.Each
functionResponseis built upstream with a uniqueidset to the originatingtoolCallId(L785 / L794), so the unique-per-call invariant already holds at the data layer — the dedup just wasn't using it.Fix
Key the dedup on
part.functionResponse.idinstead of.name:toolCallId's result) → second one still dropped, dedup guarantee preserved.Tests
New
packages/ai-gemini/tests/parallel-function-response.test.tscovers four cases:toolCallIdre-sent)formatMessagesisprivateon the adapter (TS2341 blocks subclass access, so the Probe pattern used inai-bedrock/testsforprotectedhooks doesn't apply). The test casts throughunknownto a minimal typed shape — keeps the assertion type-safe without widening the adapter's public API just to make a private method testable.Backward compatibility
mergeConsecutiveSameRoleMessagesis a private method; the change is observable only through the wire payload sent to Gemini. The set of cases that change behavior:functionResponse(sametoolCallIdre-sent) was collapsed. Post-fix: still collapsed (now keyed onid).No caller's behavior regresses; only previously-broken paths become correct.
Verification
pnpm --filter @tanstack/ai-gemini test:lib— 241/241 passing (including the 4 new tests).pnpm --filter @tanstack/ai-gemini test:eslint— 0 errors (7 pre-existing warnings in unrelated files).pnpm --filter @tanstack/ai-gemini test:types—tscclean.name; cases 1 and 4 fail with the exact symptom from the issue (expected ['call_1'] to deeply equal ['call_1', 'call_2']). Restored the fix; all 4 cases pass.Out of scope
functionCallparts was added back in fix(ai-gemini): generate unique IDs for parallel function calls #199; this PR only consumes those ids in the dedup.Changeset
.changeset/ai-gemini-parallel-function-response-dedup.md—@tanstack/ai-gemini: patch.Summary by CodeRabbit