Fix/lint and ci#44
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 (6)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughMechanical cleanup across the codebase: derive Default where possible, standardize CallId fallbacks, add documented Clippy allow attributes, simplify router/stream guard patterns, tighten streaming parsing conditions, reflow formatting, and simplify example prompts. No public API changes. ChangesCode Cleanup and Refactoring
Sequence Diagram(s) sequenceDiagram
participant Router
participant CircuitBreaker
participant Provider
Router->>CircuitBreaker: resolve_order(request)
Router->>Provider: call provider (if available)
Provider-->>Router: response or error
Router->>CircuitBreaker: record_success()/record_failure()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
providers/mistralrs/src/lib.rs (1)
23-26: 💤 Low valueConsider function-level suppression for consistency with other providers.
While the crate-level
#![allow(clippy::result_large_err)]avoids repetition, other providers (Claude, Completions, Gemini, Responses) use function-level#[allow(clippy::result_large_err)]on their error handlers. Crate-level attributes can hide future lint violations in new code that might not need the suppression.If multiple functions truly need this suppression, the crate-level approach is reasonable. Otherwise, consider using function-level attributes for consistency and to limit the scope of the suppression.
🤖 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 `@providers/mistralrs/src/lib.rs` around lines 23 - 26, The crate currently uses a crate-level suppression `#![allow(clippy::result_large_err)]`; change this to function-level suppressions to match other providers by removing the crate-level attribute and adding `#[allow(clippy::result_large_err)]` only to the specific error-handling/conversion functions (e.g., the conversion functions that return `ChatFailure`/`ChatError` and any other functions producing large Err types) so the lint scope is limited and consistent with Claude/Completions/Gemini/Responses.
🤖 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 `@core/src/types/provider_meta.rs`:
- Around line 6-8: The ProviderMeta struct currently exposes data as
HashMap<String, Box<dyn Any + Send + Sync>> which violates the repo contract;
change the data field to use JSON values (HashMap<String, serde_json::Value>) in
the ProviderMeta definition, remove the Box<dyn Any + ...> usage, import
serde_json::Value, and update any constructors, getters, setters, impls, and
(de)serialization code that reference ProviderMeta::data to accept and return
HashMap<String, Value> so provider metadata is represented as JSON values across
the codebase.
In `@examples/claude/hitl.rs`:
- Around line 103-110: The match on prompt(...) uses raw input that may include
CR or case differences; capture the prompt result into a variable, call
.trim().to_lowercase() on it, and then match that normalized string so inputs
like "y\r" or "Y" map to "y" and produce Decision::Approve (reference the
prompt(...) call and the Decision::Approve / Decision::Reject arms).
In `@examples/openai/hitl.rs`:
- Around line 103-110: The match on the raw prompt result can misclassify inputs
like "y\r" on Windows; normalize the string from prompt() before matching by
trimming whitespace and converting to lowercase (e.g., call trim() and
to_lowercase() on the prompt() result) and then match against normalized values
so Decision::Approve and Decision::Reject branches (Decision::Approve,
Decision::Reject(...)) behave correctly across platforms.
In `@providers/gemini/src/api/types/request.rs`:
- Around line 190-213: The current PartEnum::Tool branch calls tool.to_tuple()
and unconditionally serializes a GeminiFunctionCall into assistant_parts, which
can leak unresolved Tool states; change this to use tool.try_to_tuple() (or
explicitly check ToolStatus and early-return/skip for Pending/Approved/Running)
and only build GeminiPart/GeminiFunctionCall and function_parts when
try_to_tuple() yields Some((fc, maybe_fr)) (i.e., the tool is in a resolved
state like Completed/Rejected/Failed); update code paths that reference
assistant_parts, function_parts, GeminiPart, and GeminiFunctionCall to only run
when the tool is resolved.
In `@providers/router/src/router.rs`:
- Around line 57-61: The loop over providers currently records every provider
error into last_failure and continues, which violates the contract; change the
error handling inside the loop that calls provider.complete() (and the
surrounding block that checks self.circuit_breaker and order iteration) so that:
for each provider call, if it succeeds return immediately; if it fails and the
returned ChatError::is_retryable() is true, set last_failure to that error and
continue to the next provider; if it fails and is_retryable() is false
(non-retryable errors such as Provider, InvalidResponse, MaxStepsExceeded,
Callback, Other), return that error immediately without updating last_failure;
ensure this logic replaces the current unconditional last_failure assignment in
the loop (affecting the blocks around where last_failure is set and where
iteration advances).
In `@providers/router/src/stream_router.rs`:
- Around line 46-50: Both complete and stream currently stash every provider
failure and continue, causing non-retryable (fatal) provider errors to be
retried or masked; update the failure handling in the retry/fallback loops
inside complete and stream so that when an attempt returns an error you check
error.is_retryable() (or equivalent) and if it returns false you immediately
return that error instead of pushing it into the failures list and continuing;
remove/stash only retryable errors and ensure the same change is applied to all
fallback loops handling provider responses (the loops around order iteration and
circuit_breaker checks) so non-retryable errors short-circuit the method.
---
Nitpick comments:
In `@providers/mistralrs/src/lib.rs`:
- Around line 23-26: The crate currently uses a crate-level suppression
`#![allow(clippy::result_large_err)]`; change this to function-level
suppressions to match other providers by removing the crate-level attribute and
adding `#[allow(clippy::result_large_err)]` only to the specific
error-handling/conversion functions (e.g., the conversion functions that return
`ChatFailure`/`ChatError` and any other functions producing large Err types) so
the lint scope is limited and consistent with
Claude/Completions/Gemini/Responses.
🪄 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: f73c26bc-8057-4ebe-98eb-d78789f45d11
📒 Files selected for processing (30)
core/src/chat/mod.rscore/src/chat/stream/handle.rscore/src/chat/stream/input.rscore/src/chat/stream/mod.rscore/src/types/messages/parts.rscore/src/types/messages/tool.rscore/src/types/metadata/usage.rscore/src/types/provider_meta.rsexamples/claude/hitl.rsexamples/gemini/google_maps.rsexamples/mistralrs/voice.rsexamples/openai/hitl.rsproviders/claude/src/api/stream.rsproviders/claude/src/api/types/error.rsproviders/completions/src/api/stream.rsproviders/completions/src/api/types/error.rsproviders/completions/src/api/types/response.rsproviders/gemini/src/api/types/error.rsproviders/gemini/src/api/types/request.rsproviders/mistralrs/src/api/stream.rsproviders/mistralrs/src/api/types/request.rsproviders/mistralrs/src/api/types/response.rsproviders/mistralrs/src/lib.rsproviders/openai/src/api/embedding.rsproviders/openai/src/client.rsproviders/responses/src/api/types/error.rsproviders/responses/src/lib.rsproviders/router/src/router.rsproviders/router/src/stream_router.rssrc/lib.rs
💤 Files with no reviewable changes (1)
- core/src/types/metadata/usage.rs
Summary by CodeRabbit
Examples & Documentation
Code Quality