fix: make effort optional for gemini #65
Conversation
WalkthroughThe validation logic for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ReasoningConfig
User->>ReasoningConfig: Validate(effort, max_tokens)
alt max_tokens is set
ReasoningConfig-->>User: Accept empty effort
else max_tokens not set
alt effort is empty
ReasoningConfig-->>User: Return error
else effort is "low"/"medium"/"high"
ReasoningConfig-->>User: Validation passes
else
ReasoningConfig-->>User: Return error (invalid value)
end
end
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
galzilber
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 47c1a44 in 57 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/models/chat.rs:34
- Draft comment:
Ensure that allowing an empty 'effort' is intentional: now an empty 'effort' only triggers an error if 'max_tokens' is absent. Please consider adding a comment to document this behavior for clarity and maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already has clear comments explaining the behavior: "Only validate effort if max_tokens is not present (since max_tokens takes priority)" on line 31. The behavior is intentional and documented. The comment violates the rule about not asking for verification of intended behavior. It also asks for additional documentation when documentation already exists. Maybe there could be value in having even more explicit documentation about the empty string case specifically? The existing comment focuses on the general priority relationship. The existing comment clearly establishes that max_tokens takes priority over effort validation in general, which logically includes the empty string case. Additional documentation would be redundant. Delete the comment. It asks for verification of intended behavior and requests documentation that already exists.
Workflow ID: wflow_r1oIn6vXIwTGsdR1
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/models/chat.rs (2)
62-64: Potential i32 overflow when mapping max_tokens to Gemini thinking budgetCasting u32 to i32 with as can wrap for values > i32::MAX, producing negative budgets. Prefer a fallible conversion to avoid silent wrap.
Apply this diff:
- pub fn to_gemini_thinking_budget(&self) -> Option<i32> { - self.max_tokens.map(|tokens| tokens as i32) - } + pub fn to_gemini_thinking_budget(&self) -> Option<i32> { + self.max_tokens.and_then(|tokens| i32::try_from(tokens).ok()) + }Optionally, consider clamping and logging if a larger value is supplied.
31-41: Add unit tests for ReasoningConfig behaviorThe new validation and mapping logic in
ReasoningConfigisn’t fully covered by existing tests. Please add unit tests (e.g. intests/models_chat_tests.rsor next tosrc/models/chat.rs) for these scenarios:
effort = Some("") & max_tokens = Some(100)
• validate() → Ok
• to_openai_effort() → None
• to_thinking_prompt() → Some("Think through this step-by-step with detailed reasoning.")
• to_gemini_thinking_budget() → Some(100)effort = Some("") & max_tokens = None
• validate() → Err("Effort cannot be empty string")effort = Some("medium") & max_tokens = None
• validate() → Ok
• to_openai_effort() → Some("medium")
• to_thinking_prompt() → Some("Consider this problem thoughtfully.")effort = Some("weird") & max_tokens = None
• validate() → Err("Invalid effort value. Must be 'low', 'medium', or 'high'")effort = None & max_tokens = Some(250)
• validate() → Ok
• to_openai_effort() → None
• to_thinking_prompt() → Some("Think through this step-by-step with detailed reasoning.")
• to_gemini_thinking_budget() → Some(250)These tests will lock in the behavior of
validate(),to_openai_effort(),to_thinking_prompt(), andto_gemini_thinking_budget().
🧹 Nitpick comments (1)
src/models/chat.rs (1)
35-35: Nit: adjust error message grammarMinor wording improvement for the error string.
Apply this diff:
- return Err("Effort cannot be empty string".to_string()); + return Err("Effort cannot be an empty string".to_string());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/models/chat.rs(1 hunks)
🔇 Additional comments (1)
src/models/chat.rs (1)
34-36: Allowing empty effort when max_tokens is set aligns with “prioritize max_tokens”This change precisely implements the intended precedence: if max_tokens is provided, an empty effort no longer fails validation. The rest of the call sites (to_openai_effort, to_thinking_prompt) already honor this priority, so the behavior stays consistent.
Important
Make
effortoptional inReasoningConfigwhenmax_tokensis specified, adjusting validation logic accordingly.ReasoningConfig::validate(),effortis now optional ifmax_tokensis specified.effortis an empty string, it only triggers an error ifmax_tokensis not set.validate()to checkmax_tokensbefore returning an error for emptyeffort.This description was created by
for 47c1a44. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit