feat: migrate Gemini provider from OpenAI-compatible shim to native A…#563
Open
LeeroyDing wants to merge 6 commits intospacedriveapp:mainfrom
Open
feat: migrate Gemini provider from OpenAI-compatible shim to native A…#563LeeroyDing wants to merge 6 commits intospacedriveapp:mainfrom
LeeroyDing wants to merge 6 commits intospacedriveapp:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds native Gemini provider support: new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm/gemini.rs`:
- Around line 267-268: The current serialization of the full Gemini response
uses serde_json::to_value(&response).unwrap_or_default(), which silently drops
errors and can produce a null body; replace that pattern so serialization
failures are not discarded: change
serde_json::to_value(&response).unwrap_or_default() to explicitly handle the
Result (match or ?), logging or returning the error (e.g., via trace/error or
propagating the Error from the enclosing function) and ensure you always persist
the latest streamed body instead of leaving it null — specifically update the
`body` assignment for RawResponse, the stream-path update in the
`usage_metadata` branch, and the other occurrences noted around lines 323-331
and 355-357 so they either propagate the serialization error or log it and
fallback to the last successful streamed body rather than defaulting to null.
- Around line 162-186: The code currently swallows non-text UserContent variants
in the match inside the loop over content, which can drop
images/audio/video/documents and produce incomplete Gemini requests; update the
"_" branch (in the match over rig::message::UserContent within the loop that
builds text_parts and tool_responses) to either (a) convert those variants into
the corresponding Gemini Part objects and append them to the outgoing request
parts (so the request includes the multimodal data), or (b) return an explicit
error/Err result indicating unsupported multimodal input (with clear context
including the offending variant id), rather than silently ignoring them; ensure
you reference and update whatever collection is used to accumulate Gemini Parts
so downstream code sees the converted parts.
- Around line 179-205: The code collapses repeated tool calls because it uses
the same tool name for both the Gemini function identity and the tool-invocation
id; fix by preserving the original function name separately (e.g., keep
function_call.name as tool_name) and synthesize a unique invocation id for
correlation (e.g., append an incrementing index or UUID when building
ToolCall.id) so each invocation is distinct; update the places that create
ToolCall.id and the code that constructs the Gemini function-response name
(where ToolResult.id is sent back and where
Content::function_response_json(&name, ...), GeminiMessage, with_message(), and
with_user_message() are used) to use the synthesized unique id for correlation
while still including the original function_call.name/tool_name in the payload
or meta so you can match results to the correct function.
- Around line 28-40: build_client currently ignores ProviderConfig.base_url and
naively prepends "models/" to every model_name; update it so it respects custom
endpoints by passing provider_config.base_url (when present) into the Gemini
client constructor instead of always calling Gemini::with_model with only
api_key, and fix model name handling in build_client by only prepending
"models/" when model_name has no resource prefix (e.g., does not start with
"models/", "tunedModels/", "cachedContent/", etc.); alternatively, if the gemini
client doesn't support custom endpoints, fail fast with a clear CompletionError
mentioning base_url. Locate build_client and the Gemini::with_model call and
adjust to use ProviderConfig.base_url and a guarded model_name prefix check.
In `@src/llm/model.rs`:
- Around line 587-593: The usage parsing currently branches on the literal
provider string (`self.provider`) which misses cases where a provider is
configured with `provider_config.api_type = "gemini"`; update the selection that
calls `crate::llm::usage::ExtendedUsage::from_anthropic_body`,
`from_gemini_body`, and `from_openai_body` to branch on the resolved `ApiType`
(e.g., `ApiType::Anthropic`, `ApiType::Gemini`, `ApiType::OpenAi`) instead of
`self.provider`, and thread the resolved `ApiType` value through the helper call
site so the correct `from_gemini_body` path is taken for any provider whose
`api_type` is Gemini; apply the same change to the analogous branch around the
other occurrence (previously lines ~1775-1781) so both usage-parsing sites use
`ApiType` dispatch rather than the provider id.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b35ca4e9-97f6-4ffc-b186-e2ec9a04c5ee
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (5)
src/config/providers.rssrc/llm.rssrc/llm/gemini.rssrc/llm/model.rssrc/llm/usage.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/llm/gemini.rs (2)
188-210:⚠️ Potential issue | 🔴 CriticalTool results are sent back under the UUID instead of the declared function name.
Lines 296 and 363 synthesize
call_*ids for Gemini tool calls, but this branch feedstool_result.idback intofunction_response_json(). After the first tool execution, Gemini will seecall_...as the function name instead of the tool it invoked, so tool-call round-trips break. Keep the original tool name alongside the synthetic correlation id and use the tool name here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/gemini.rs` around lines 188 - 210, The bug is that you feed the synthetic correlation id (tool_result.id / the call_* id) back into function_response_json instead of the original declared tool name; update the code that builds tool_responses so it preserves and pushes the original tool name (e.g., the declared function name) alongside the result JSON, and then call Content::function_response_json(&original_tool_name, result_json) in the loop; locate the code that creates tool_responses and change the tuple to carry the declared tool name (not tool_result.id) so function_response_json receives the real tool name.
339-351:⚠️ Potential issue | 🟠 MajorThe streamed raw body can still end up stale or
Null.
accumulated_bodyonly changes inside theusage_metadatabranch, so later chunks without usage data are invisible to the finalRawStreamingResponseon Line 377. In the no-usage case it staysNull, and both ignoredtry_lock()failures have the same effect. Update the body on every chunk and handle lock failures explicitly.As per coding guidelines, "Don't silently discard errors. No
let _ =on Results. Handle them, log them, or propagate them."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/gemini.rs` around lines 339 - 351, The bug: accumulated_body is only updated when chunk.usage_metadata is Some, causing later chunks without usage to leave accumulated_body null; also try_lock failures are ignored. Fix by moving the serde_json::to_value(&chunk) -> val update of accumulated_body outside the usage_metadata branch so every chunk updates accumulated_body, and replace the silent try_lock ignores with explicit error handling/logging (e.g., if let Err(e) = accumulated_body.try_lock() { tracing::warn!(...) } and similarly for accumulated_usage), keeping the existing convert_usage(usage_meta) and accumulated_usage assignment inside the usage_metadata branch; ensure both locks are attempted each chunk and any lock or serialization errors are logged rather than discarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm/gemini.rs`:
- Around line 224-243: The loop over content currently drops any unsupported
AssistantContent variants via `_ => {}`, losing history; update that branch in
the code that builds Parts from AssistantContent (the match on AssistantContent,
which produces Part::Text and Part::FunctionCall and uses fields like thought
and thought_signature) to either (a) convert unknown variants into a safe
fallback Part (e.g., Part::Text containing a serialized/diagnostic
representation of the variant and preserve any available signature/thought
fields) or (b) return/propagate a clear error (not silently ignore) so the
caller can handle it; replace the `_ => {}` with one of these behaviors and use
the function’s existing error type or logging to report the unsupported variant.
---
Duplicate comments:
In `@src/llm/gemini.rs`:
- Around line 188-210: The bug is that you feed the synthetic correlation id
(tool_result.id / the call_* id) back into function_response_json instead of the
original declared tool name; update the code that builds tool_responses so it
preserves and pushes the original tool name (e.g., the declared function name)
alongside the result JSON, and then call
Content::function_response_json(&original_tool_name, result_json) in the loop;
locate the code that creates tool_responses and change the tuple to carry the
declared tool name (not tool_result.id) so function_response_json receives the
real tool name.
- Around line 339-351: The bug: accumulated_body is only updated when
chunk.usage_metadata is Some, causing later chunks without usage to leave
accumulated_body null; also try_lock failures are ignored. Fix by moving the
serde_json::to_value(&chunk) -> val update of accumulated_body outside the
usage_metadata branch so every chunk updates accumulated_body, and replace the
silent try_lock ignores with explicit error handling/logging (e.g., if let
Err(e) = accumulated_body.try_lock() { tracing::warn!(...) } and similarly for
accumulated_usage), keeping the existing convert_usage(usage_meta) and
accumulated_usage assignment inside the usage_metadata branch; ensure both locks
are attempted each chunk and any lock or serialization errors are logged rather
than discarded.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61f95d77-ea5f-48a2-94c8-61f672fc52c4
📒 Files selected for processing (2)
src/llm/gemini.rssrc/llm/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/llm/model.rs
…ool result testing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/scratch.rs (2)
2-3: Renametrto a non-abbreviated parameter name.Line 2 uses
tr, which violates the no-abbreviations naming rule. Prefertool_result(and ideally a more descriptive function name thantest).✏️ Proposed rename
-fn test(tr: &ToolResult) -> &str { - &tr.name +fn tool_result_name(tool_result: &ToolResult) -> &str { + &tool_result.name }As per coding guidelines: "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch. Common abbreviations likeconfigare fine."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scratch.rs` around lines 2 - 3, Rename the abbreviated parameter tr in the function test to a descriptive name (e.g., tool_result) and update all references inside the function (currently &tr.name) to match; also consider renaming the function test to a more descriptive name (e.g., get_tool_name) so the signature becomes something like fn get_tool_name(tool_result: &ToolResult) -> &str, ensuring ToolResult and its .name access remain correct.
1-4: Avoid introducing a standalone scratch module for this helper.This file is a tiny scratch helper and doesn’t look like a new logical component. Please fold this into an existing relevant module (or test module) instead of keeping a dedicated
src/scratch.rsfile.As per coding guidelines: "Don't create many small files. Implement functionality in existing files unless it's a new logical component."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scratch.rs` around lines 1 - 4, The standalone helper function test(&ToolResult) that returns &str should be removed from the isolated scratch file and relocated into an existing relevant module or test module that already uses rig::message::ToolResult; move the function (or inline its logic) into that existing file, adjust imports to use rig::message::ToolResult there, and ensure lifetimes remain valid when returning &tr.name (keep signature fn test(tr: &ToolResult) -> &str or refactor to return String if borrowing is problematic); delete the orphan scratch module after moving.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm/gemini.rs`:
- Around line 388-392: The final extraction uses body_for_final.lock().unwrap()
and usage_for_final.lock().unwrap() which can panic on a poisoned mutex; change
these to handle poisoning gracefully by matching the lock result (e.g.,
lock().map_or_else(|poison| poison.into_inner(), |guard| guard.clone() or
.take()) or similar), log or warn if poisoning occurred, and provide sensible
defaults if needed before yielding
RawStreamingChoice::FinalResponse(RawStreamingResponse { body, usage }) so the
stream does not panic when a prior panic poisoned the mutex.
---
Nitpick comments:
In `@src/scratch.rs`:
- Around line 2-3: Rename the abbreviated parameter tr in the function test to a
descriptive name (e.g., tool_result) and update all references inside the
function (currently &tr.name) to match; also consider renaming the function test
to a more descriptive name (e.g., get_tool_name) so the signature becomes
something like fn get_tool_name(tool_result: &ToolResult) -> &str, ensuring
ToolResult and its .name access remain correct.
- Around line 1-4: The standalone helper function test(&ToolResult) that returns
&str should be removed from the isolated scratch file and relocated into an
existing relevant module or test module that already uses
rig::message::ToolResult; move the function (or inline its logic) into that
existing file, adjust imports to use rig::message::ToolResult there, and ensure
lifetimes remain valid when returning &tr.name (keep signature fn test(tr:
&ToolResult) -> &str or refactor to return String if borrowing is problematic);
delete the orphan scratch module after moving.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4bcb0dc-7974-4d26-819d-044a3f2e9c1b
📒 Files selected for processing (2)
src/llm/gemini.rssrc/scratch.rs
…for Gemini response handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm/gemini.rs`:
- Around line 39-44: The code incorrectly treats provider_config.base_url as an
Option; change the handling in the Gemini client setup: instead of `if let
Some(base_url) = &provider_config.base_url` treat base_url as a plain String —
either always call client =
client.with_base_url(provider_config.base_url.clone()) if base_url is required,
or guard with a presence check like `if !provider_config.base_url.is_empty()`
before calling client.with_base_url(...); update the block around
Gemini::with_model and client.with_base_url to use provider_config.base_url as a
String accordingly.
- Around line 231-256: The loop currently rejects AssistantContent::Reasoning,
causing hard-fail; instead update the match arm for AssistantContent::Reasoning
to convert the reasoning payload into a plain-text Part (e.g., Part::Text) using
the same pattern as AssistantContent::Text: push Part::Text with the reasoning
text (concatenate or extract the visible string like
convert_messages_to_openai_responses does), and populate thought and
thought_signature if the Reasoning variant exposes them (or set them to None if
not); leave ToolCall behavior and tool_map handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…tex safety - Fix base_url handling: ProviderConfig.base_url is String, not Option<String>. Use is_empty() guard instead of if-let Some(). - Handle AssistantContent::Reasoning in Gemini provider by mapping reasoning content to thought-tagged Part::Text instead of returning a hard error. Follows the same pattern used in model.rs and compactor.rs. - Poisoned mutex handling was already fixed in 47102c7 — no further changes needed there.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/llm/gemini.rs (1)
32-37:⚠️ Potential issue | 🟠 MajorKeep explicit Gemini resource prefixes intact.
Line 33 only special-cases
models/. A configured resource liketunedModels/...will be rewritten tomodels/tunedModels/..., which breaks valid native Gemini identifiers. Only prependmodels/for bare model ids.🔧 Proposed fix
- let model_str = if model_name.starts_with("models/") { + let model_str = if model_name.contains('/') { model_name.to_string() } else { format!("models/{model_name}") };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/gemini.rs` around lines 32 - 37, The current logic only checks starts_with("models/") and prepends "models/" which incorrectly rewrites valid Gemini resource prefixes like "tunedModels/..."; update the condition that builds model_str so it only prepends "models/" for bare model ids (i.e., when model_name contains no '/' character). Specifically, change the branch that sets model_str (referencing model_str and model_name) to return model_name.to_string() when model_name contains '/' (preserving any existing prefix like "tunedModels/..."), and only format!("models/{model_name}") when model_name has no '/'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm/gemini.rs`:
- Around line 166-223: The code currently accumulates all text into text_parts
and all tool results into tool_responses, changing the original ordering; modify
the content iteration in the block that processes rig::message::UserContent so
that when you encounter a ToolResult you first flush any buffered text_parts
into the builder (call with_user_message with the joined text and clear the
buffer) before emitting the function response (use
Content::function_response_json and with_message / GeminiMessage as done now);
likewise after the loop flush any remaining text_parts into current_builder so
messages are replayed in the original interleaved order (references: variables
text_parts, tool_responses, current_builder and methods with_user_message,
with_message, Content::function_response_json, GeminiMessage).
- Around line 452-465: The match arm for effort strings currently doesn't handle
the "max" value and falls through to the numeric/unknown branch; update the
pattern matching in the match on effort (the branch handling "auto" | "" |
"none" | "off" | "low" | "medium" | "high" | "minimal") to include "max" and map
it to Some(ThinkingConfig::new().with_thinking_level(ThinkingLevel::High));
ensure this change is made where ThinkingConfig and ThinkingLevel are used so
"max" is treated equivalent to "high" instead of hitting the unknown branch that
logs a warning.
- Around line 187-195: The parsed JSON assigned to result_json may be a
primitive/array, but Gemini requires functionResponse.response to be a JSON
object; change the logic around serde_json::from_str(&result_text) so that after
parsing you check result_json.is_object(), and if it is not an object (including
cases where parsing returned a primitive, array, number, boolean, or null),
replace/wrap it with serde_json::json!({"result": result_json}) before pushing
into tool_responses; keep using tool_map and tool_result.id as before to build
tool_name and push (i.e., update the handling in the block that currently sets
result_json and pushes (tool_name, result_json)).
---
Duplicate comments:
In `@src/llm/gemini.rs`:
- Around line 32-37: The current logic only checks starts_with("models/") and
prepends "models/" which incorrectly rewrites valid Gemini resource prefixes
like "tunedModels/..."; update the condition that builds model_str so it only
prepends "models/" for bare model ids (i.e., when model_name contains no '/'
character). Specifically, change the branch that sets model_str (referencing
model_str and model_name) to return model_name.to_string() when model_name
contains '/' (preserving any existing prefix like "tunedModels/..."), and only
format!("models/{model_name}") when model_name has no '/'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…ing levels - Update prefix check to avoid rewriting valid resource paths - Fix message ordering by flushing accumulated text into builder upon encountering a function response instead of doing all text then all responses at the very end - Map thinking effort to - Ensure the JSON generated for Gemini function responses is an Object, wrapping primitive JSON returns as to satisfy the API
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
1 participant
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.
…PI integration