fix(p3): remediate top audit findings#67
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| //! LLM Router - Multi-provider LLM routing | ||
| //! | ||
| //! Inspired by litellm, provides unified interface for multiple LLM providers. | ||
| //! Features retry with exponential backoff, configurable timeouts, and tracing. |
There was a problem hiding this comment.
Suggestion: Add an explicit AgilePlus spec reference (or creation link/ID) alongside this new feature declaration so the retry/timeout/tracing work is traceable to a tracked item. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is newly introduced feature work, and the file contains no AgilePlus/spec traceability reference for it. That matches the rule requiring new functionality to be backed by a tracked AgilePlus item.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 4:4
**Comment:**
*Custom Rule: Add an explicit AgilePlus spec reference (or creation link/ID) alongside this new feature declaration so the retry/timeout/tracing work is traceable to a tracked item.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// Create a new router with a custom retry configuration. | ||
| pub fn new_with_retry(retry_config: RetryConfig) -> Self { | ||
| Self { | ||
| providers: DashMap::new(), | ||
| fallback: None, | ||
| retry_config, | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Reference the corresponding AgilePlus specification for this newly introduced router capability (or add the spec first), so this functionality is not merged as ad hoc code. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This constructor introduces new router behavior (custom retry configuration) with no AgilePlus specification reference or creation path in the changed code, so it is an ad hoc feature addition under the stated rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 224:231
**Comment:**
*Custom Rule: Reference the corresponding AgilePlus specification for this newly introduced router capability (or add the spec first), so this functionality is not merged as ad hoc code.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| #[test] | ||
| fn llm_error_recovery_hints_are_not_empty() { | ||
| let variants: [LlmError; 4] = [ | ||
| LlmError::Provider("x".into()), | ||
| LlmError::RateLimited, | ||
| LlmError::Timeout, | ||
| LlmError::InvalidModel("x".into()), | ||
| ]; | ||
| for v in &variants { | ||
| let hint = v.recovery_hint(); | ||
| assert!(!hint.is_empty(), "empty recovery hint for {v}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add a Functional Requirement reference (for example an FR ID in the test name/comment/attribute) and update traceability so this added test maps to FUNCTIONAL_REQUIREMENTS.md. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
FUNCTIONAL_REQUIREMENTS.md exists in the repository, and this added test does not reference any FR identifier in its name, comment, or attribute. That satisfies the test traceability rule violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 507:519
**Comment:**
*Custom Rule: Add a Functional Requirement reference (for example an FR ID in the test name/comment/attribute) and update traceability so this added test maps to FUNCTIONAL_REQUIREMENTS.md.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| impl McpError { | ||
| /// Returns a human-readable recovery hint for this error. | ||
| pub fn recovery_hint(&self) -> &'static str { | ||
| match self { | ||
| McpError::ToolNotFound(_) => { | ||
| "Verify the tool name is correct. Use list_tools() to see available tools." | ||
| } | ||
| McpError::ResourceNotFound(_) => { | ||
| "Verify the resource URI is correct. Use list_resources() to see available resources." | ||
| } | ||
| McpError::InvalidRequest(_) => { | ||
| "Check that the request payload matches the expected schema for this tool/resource." | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add a clear AgilePlus trace (existing item ID or spec creation path) for this newly introduced behavior so the change is visibly linked to tracked work instead of appearing ad hoc. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The final file introduces a new recovery_hint API and accompanying behavior, but there is no AgilePlus spec reference or creation/tracking path anywhere in the changed code. This matches the custom rule requiring new feature work to be traced to an AgilePlus item.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/mcp-server/src/lib.rs
**Line:** 26:40
**Comment:**
*Custom Rule: Add a clear AgilePlus trace (existing item ID or spec creation path) for this newly introduced behavior so the change is visibly linked to tracked work instead of appearing ad hoc.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| #[test] | ||
| fn mcp_error_recovery_hints_are_not_empty() { | ||
| let variants: [McpError; 3] = [ | ||
| McpError::ToolNotFound("x".into()), | ||
| McpError::ResourceNotFound("x".into()), | ||
| McpError::InvalidRequest("x".into()), | ||
| ]; | ||
| for v in &variants { | ||
| let hint = v.recovery_hint(); | ||
| assert!(!hint.is_empty(), "empty recovery hint for {v}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add an FR traceability reference for this new test (and align the traceability table) so the test is mapped to a Functional Requirement now that FUNCTIONAL_REQUIREMENTS.md exists. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
FUNCTIONAL_REQUIREMENTS.md exists, and this newly added test does not reference any FR in the diff. The traceability table is still placeholder content, so the test is not mapped to a Functional Requirement as required.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/mcp-server/src/lib.rs
**Line:** 267:278
**Comment:**
*Custom Rule: Add an FR traceability reference for this new test (and align the traceability table) so the test is mapped to a Functional Requirement now that FUNCTIONAL_REQUIREMENTS.md exists.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| impl EmbeddingError { | ||
| /// Returns a human-readable recovery hint for this error. | ||
| pub fn recovery_hint(&self) -> &'static str { | ||
| match self { | ||
| EmbeddingError::Provider(_) => { | ||
| "Check that the embedding provider API key is valid and the service is reachable." | ||
| } | ||
| EmbeddingError::InvalidInput(_) => { | ||
| "Ensure the input text is non-empty and within supported length limits." | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add an explicit AgilePlus spec reference (or link to the created spec item) for this newly introduced error-recovery feature before merging. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This change introduces new error-recovery behavior (recovery_hint) and the file contains no AgilePlus spec reference or traceability marker. That fits the rule requiring new feature code to be backed by an AgilePlus spec item.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 20:31
**Comment:**
*Custom Rule: Add an explicit AgilePlus spec reference (or link to the created spec item) for this newly introduced error-recovery feature before merging.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if request.texts.is_empty() { | ||
| return Err(EmbeddingError::InvalidInput( | ||
| "texts must not be empty".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Suggestion: Attach this new request-validation behavior to an AgilePlus work item (or add the missing spec first) so the implementation is not ad hoc. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This adds new request-validation behavior in the embed flow, but there is no accompanying AgilePlus spec reference or creation path in the changed code. The implementation therefore appears untracked as required by the rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 86:90
**Comment:**
*Custom Rule: Attach this new request-validation behavior to an AgilePlus work item (or add the missing spec first) so the implementation is not ad hoc.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| #[test] | ||
| fn embedding_error_recovery_hints_are_not_empty() { | ||
| let variants: [EmbeddingError; 2] = [ | ||
| EmbeddingError::Provider("x".into()), | ||
| EmbeddingError::InvalidInput("x".into()), | ||
| ]; | ||
| for v in &variants { | ||
| let hint = v.recovery_hint(); | ||
| assert!(!hint.is_empty(), "empty recovery hint for {v}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Add an FR trace reference (for example a Traces to: FR-... comment) to this newly added test to satisfy requirement-to-test traceability. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
FUNCTIONAL_REQUIREMENTS.md exists in the repository, and this newly added test does not reference any FR identifier or trace comment. The test-traceability rule therefore applies.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 202:212
**Comment:**
*Custom Rule: Add an FR trace reference (for example a `Traces to: FR-...` comment) to this newly added test to satisfy requirement-to-test traceability.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| async fn test_retry_exhausted_returns_error() { | ||
| // Use a retry config with very low delay and only 1 retry | ||
| let cfg = RetryConfig { | ||
| max_retries: 1, | ||
| base_delay_ms: 10, | ||
| max_jitter_ms: 5, | ||
| }; | ||
| let router = LlmRouter::new_with_retry(cfg); |
There was a problem hiding this comment.
Suggestion: Link this new test scenario to an AgilePlus specification reference (or include the spec creation reference in this change) so the new functionality is not introduced ad hoc. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The diff does not contain any AgilePlus spec reference or creation path for this new retry-related test scenario. Under the stated rules, newly introduced work should be traced to a corresponding AgilePlus item, so this is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/llm_router_test.rs
**Line:** 261:268
**Comment:**
*Custom Rule: Link this new test scenario to an AgilePlus specification reference (or include the spec creation reference in this change) so the new functionality is not introduced ad hoc.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| .send() | ||
| .await | ||
| .map_err(|e| LlmError::Provider(e.to_string()))?; | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "OpenAI request failed"); | ||
| LlmError::Provider(e.to_string()) | ||
| })?; |
There was a problem hiding this comment.
Suggestion: The request path only maps transport failures; HTTP error statuses (429/500/401) are treated as success because the response is never checked with error_for_status(). This prevents correct retry/error classification and can return fake successful completions on upstream failure. Validate status codes before constructing a success response. [api mismatch]
Severity Level: Critical 🚨
- ❌ Upstream HTTP failures returned as successful completions to callers.
- ⚠️ Router never retries on 429 or 500 statuses.Steps of Reproduction ✅
1. A client or higher-level service constructs a `CompletionRequest` and routes it through
`LlmRouter::complete()` so that the `"openai"` prefix selects `OpenAiProvider` (provider
registration pattern demonstrated in `crates/llm-router/src/lib.rs:413-419` and
`tests/llm_router_test.rs:139-145`).
2. Inside `OpenAiProvider::complete()` (`crates/llm-router/src/lib.rs:156-197`), the
method builds the JSON body at lines 161-165 and sends an HTTP POST to
`{base_url}/chat/completions` using `self.client.post(...).send().await` at lines 169-175.
3. At lines 174-179, the code awaits `.send()` and maps only transport-level failures into
`LlmError::Provider(e.to_string())`, binding `_response` but never inspecting or
validating the HTTP status or response body before proceeding.
4. When the upstream API returns non-2xx statuses such as `401 Unauthorized`, `429 Too
Many Requests`, or `500 Internal Server Error`, `reqwest::Client::send()` still resolves
successfully, `_response` is discarded, and the function logs `"completion successful"` at
lines 187-188 and returns a hard-coded `CompletionResponse` with `"response"` content at
lines 190-196, misclassifying failed completions as successful and preventing correct
retry/error handling in the router.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 174:179
**Comment:**
*Api Mismatch: The request path only maps transport failures; HTTP error statuses (429/500/401) are treated as success because the response is never checked with `error_for_status()`. This prevents correct retry/error classification and can return fake successful completions on upstream failure. Validate status codes before constructing a success response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if let Some(provider) = self.providers.get(prefix) { | ||
| return provider.complete(request).await; | ||
| match self.execute_with_retry(&provider, request).await { |
There was a problem hiding this comment.
Suggestion: The DashMap entry guard is kept alive across an .await, so a shard lock can be held for the full provider call/retry duration. This can block concurrent provider registration/lookups on that shard and lead to lock contention or deadlock-like stalls. Clone the Arc<dyn LlmProvider> out of the map before awaiting. [race condition]
Severity Level: Major ⚠️
- ❌ Concurrent router calls hold provider map lock across awaits.
- ⚠️ Provider registration or reconfiguration blocked during long operations.Steps of Reproduction ✅
1. In `tests/llm_router_test.rs:197-215`, the `test_router_uses_registered_provider()`
integration test constructs a `LlmRouter` with `LlmRouter::new()` and registers a
`MockProvider` via `router.register_provider("my", provider)` before calling
`router.complete(&req).await`.
2. The call enters `LlmRouter::complete()` in `crates/llm-router/src/lib.rs:252-288`,
which derives the prefix from `request.model` and executes `if let Some(provider) =
self.providers.get(prefix)` at line 262, obtaining a `DashMap` entry guard for the `"my"`
key.
3. `complete()` immediately calls `self.execute_with_retry(&provider, request).await` at
line 263, passing a reference to the `DashMap` guard into `execute_with_retry()`
(`crates/llm-router/src/lib.rs:290-369`) and keeping the underlying shard lock held for
the entire async retry loop.
4. Under concurrent load where other tasks attempt to mutate the providers map (e.g.,
dynamic provider registration similar to
`register_provider_makes_it_addressable_by_prefix()` in
`crates/llm-router/src/lib.rs:413-419` or future runtime code), these mutations block on
the held shard lock until all retries complete, causing lock contention and stalls;
cloning the `Arc<dyn LlmProvider>` out of the map before awaiting would drop the guard and
release the lock promptly.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 262:263
**Comment:**
*Race Condition: The `DashMap` entry guard is kept alive across an `.await`, so a shard lock can be held for the full provider call/retry duration. This can block concurrent provider registration/lookups on that shard and lead to lock contention or deadlock-like stalls. Clone the `Arc<dyn LlmProvider>` out of the map before awaiting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| request: &CompletionRequest, | ||
| ) -> Result<CompletionResponse, LlmError> { | ||
| let timeout_ms = request.timeout_ms.unwrap_or(30_000); | ||
| let max_attempts = self.retry_config.max_retries + 1; // initial + retries |
There was a problem hiding this comment.
Suggestion: max_retries + 1 can overflow for large user-provided values (for example u32::MAX), which can panic in debug builds or wrap in release, breaking retry behavior. Use saturating_add(1) and optionally validate upper bounds when constructing config. [type error]
Severity Level: Major ⚠️
- ❌ Overflowing retry count can panic or skip retries.
- ⚠️ Misconfigured backoff breaks resilience under transient upstream failures.Steps of Reproduction ✅
1. Application code constructs a `RetryConfig` with a very large `max_retries` value
(potentially `u32::MAX` if read from unvalidated configuration) and passes it into
`LlmRouter::new_with_retry(cfg)` as shown in `tests/llm_router_test.rs:263-268`.
2. Later, a caller uses this router for completions (similar to
`test_retry_exhausted_returns_error()` in `tests/llm_router_test.rs:274-289`) by invoking
`router.complete(&req).await`, which routes into `execute_with_retry()`
(`crates/llm-router/src/lib.rs:290-369`).
3. At line 297 in `crates/llm-router/src/lib.rs`, `let max_attempts =
self.retry_config.max_retries + 1;` performs unchecked addition on `u32`; in debug builds
an overflow (e.g., `u32::MAX + 1`) causes a panic, and in release builds it wraps to `0`,
corrupting the intended “initial + retries” semantics.
4. When `max_attempts` becomes `0` due to wrapping, the `for attempt in 1..=max_attempts`
loop at line 299 runs zero iterations and execution falls through to the `unreachable!()`
at line 367, causing a panic instead of performing any provider calls or retries, breaking
robustness under misconfigured retry settings.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 297:297
**Comment:**
*Type Error: `max_retries + 1` can overflow for large user-provided values (for example `u32::MAX`), which can panic in debug builds or wrap in release, breaking retry behavior. Use `saturating_add(1)` and optionally validate upper bounds when constructing config.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let result = if timeout_ms > 0 { | ||
| match tokio::time::timeout( | ||
| Duration::from_millis(timeout_ms), | ||
| provider.complete(request), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(inner) => inner, | ||
| Err(_elapsed) => { | ||
| tracing::warn!( | ||
| provider = %provider.provider_name(), | ||
| timeout_ms = timeout_ms, | ||
| "provider call timed out" | ||
| ); | ||
| Err(LlmError::Timeout) | ||
| } | ||
| } | ||
| } else { | ||
| provider.complete(request).await | ||
| }; | ||
|
|
There was a problem hiding this comment.
Suggestion: When timeout_ms is 0, the code skips tokio::time::timeout entirely and performs an unbounded provider call. This allows a caller to disable timeout enforcement and potentially tie up async workers indefinitely. Treat 0 as invalid input or clamp to a minimum timeout instead of disabling limits. [security]
Severity Level: Major ⚠️
- ❌ Zero timeout permits unbounded LLM calls without deadlines.
- ⚠️ Hung providers can exhaust executor threads and degrade service.Steps of Reproduction ✅
1. A caller constructs a `CompletionRequest` with `timeout_ms: Some(0)` (same shape as
requests in `tests/llm_router_test.rs:203-209` and `299-305`) and passes it to
`router.complete(&req).await` on a `LlmRouter` instance.
2. The call reaches `execute_with_retry()` via `LlmRouter::complete()`
(`crates/llm-router/src/lib.rs:252-288`), where `timeout_ms` is read from the request with
`let timeout_ms = request.timeout_ms.unwrap_or(30_000);` at line 296, leaving it as `0`.
3. At lines 300-320 in `crates/llm-router/src/lib.rs`, `let result = if timeout_ms > 0 {
... } else { provider.complete(request).await };` selects the `else` branch because
`timeout_ms == 0`, so the provider call runs without being wrapped in
`tokio::time::timeout`.
4. If the provider hangs or is extremely slow (for example, a stalled HTTP call in
`OpenAiProvider::complete()` at `crates/llm-router/src/lib.rs:156-197`), the async task
executing `execute_with_retry()` is tied up indefinitely, defeating the router’s intended
timeout enforcement and allowing unbounded execution whenever `timeout_ms` is set to zero.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 300:320
**Comment:**
*Security: When `timeout_ms` is `0`, the code skips `tokio::time::timeout` entirely and performs an unbounded provider call. This allows a caller to disable timeout enforcement and potentially tie up async workers indefinitely. Treat `0` as invalid input or clamp to a minimum timeout instead of disabling limits.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| .saturating_mul(2u64.saturating_pow(attempt.saturating_sub(1))); | ||
| let jitter = | ||
| rand::thread_rng().gen_range(0..=self.retry_config.max_jitter_ms); | ||
| let total_delay = delay_ms + jitter; |
There was a problem hiding this comment.
Suggestion: The delay computation uses plain + after saturating multiplication, so delay_ms + jitter can still overflow for large retry settings and panic in debug builds. Use saturating_add for total_delay as well. [type error]
Severity Level: Major ⚠️
- ❌ Delay overflow can panic retry loop during backoff.
- ⚠️ Wrapped sleep durations corrupt timing for provider recovery.Steps of Reproduction ✅
1. A caller configures `RetryConfig` with very large `base_delay_ms` and `max_jitter_ms`
values and constructs a router using `LlmRouter::new_with_retry(cfg)` (pattern shown in
`tests/llm_router_test.rs:263-268`).
2. When `router.complete(&req).await` is invoked and the provider returns a retryable
error, control enters `execute_with_retry()` (`crates/llm-router/src/lib.rs:290-369`) and
starts the retry loop at lines 299-365.
3. Inside the retry branch at lines 345-351, `delay_ms` is computed using
`self.retry_config.base_delay_ms.saturating_mul(2u64.saturating_pow(attempt.saturating_sub(1)))`,
and `jitter` is sampled via
`rand::thread_rng().gen_range(0..=self.retry_config.max_jitter_ms)`.
4. At line 352, `let total_delay = delay_ms + jitter;` uses plain `+` on `u64`; for
extreme settings where `delay_ms` and `jitter` are both near `u64::MAX`, this addition
overflows, which in debug builds triggers a panic and in release builds wraps the delay,
causing incorrect sleep durations and potential panics in the retry logic despite the use
of `saturating_mul` earlier.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/llm-router/src/lib.rs
**Line:** 352:352
**Comment:**
*Type Error: The delay computation uses plain `+` after saturating multiplication, so `delay_ms + jitter` can still overflow for large retry settings and panic in debug builds. Use `saturating_add` for `total_delay` as well.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| .send() | ||
| .await | ||
| .map_err(|e| EmbeddingError::Provider(e.to_string()))?; | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "embedding request failed"); | ||
| EmbeddingError::Provider(e.to_string()) | ||
| })?; |
There was a problem hiding this comment.
Suggestion: The embedding call maps only network-level failures and does not treat non-2xx HTTP responses as errors. As written, provider-side failures can be logged as successful embeddings and returned as Ok. Call error_for_status() (or equivalent status validation) before reporting success. [api mismatch]
Severity Level: Critical 🚨
- ❌ Embedding HTTP errors misreported as successful zero vectors.
- ⚠️ Clients cannot detect provider outages or authorization failures.Steps of Reproduction ✅
1. Downstream code constructs an `EmbeddingRequest` (e.g., similar to the request shapes
exercised in `crates/pheno-embedding/src/lib.rs:138-156`) and calls
`OpenAiEmbeddings::embed(&request)` from application logic.
2. Inside `embed()` (`crates/pheno-embedding/src/lib.rs:68-127`), the function selects a
model, validates that `request.texts` is non-empty, and builds the embeddings request body
at lines 92-95.
3. At lines 103-109, it sends the HTTP request with
`.post("https://api.openai.com/v1/embeddings").json(&body).send().await`, then maps only
transport-level failures into `EmbeddingError::Provider(e.to_string())` at lines 109-113,
binding `_response` but never calling `error_for_status()` or otherwise checking the
returned HTTP status.
4. When the OpenAI embeddings endpoint responds with a non-2xx status (e.g., `401` for
invalid API key or `429` for rate limiting), `send().await` still returns a
`reqwest::Response`; because status validation is omitted, `embed()` logs `"embedding
successful"` at lines 115-118 and returns dummy zero embeddings at lines 121-125, causing
callers to treat failed embeddings as success and lose the ability to react to
provider-side errors.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 108:113
**Comment:**
*Api Mismatch: The embedding call maps only network-level failures and does not treat non-2xx HTTP responses as errors. As written, provider-side failures can be logged as successful embeddings and returned as `Ok`. Call `error_for_status()` (or equivalent status validation) before reporting success.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if let Some(ref count) = self.fail_count { | ||
| let remaining = count.fetch_sub(1, Ordering::SeqCst); | ||
| if remaining > 0 { | ||
| return Err(LlmError::Provider(format!( | ||
| "Mock transient failure ({remaining} left)" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Suggestion: fetch_sub(1) underflows after the counter reaches zero, so after the first success the counter wraps to u32::MAX and subsequent calls start failing again. This contradicts the stated behavior of failing N times then succeeding. Use a compare-and-decrement pattern that does not decrement below zero. [logic error]
Severity Level: Major ⚠️
- ❌ Transient failure helper resumes failing after supposed success threshold.
- ⚠️ Retry tests may misrepresent real provider recovery behavior.Steps of Reproduction ✅
1. The helper `MockProvider::fail_n_times("retry-test", 2)` is constructed in
`tests/llm_router_test.rs:37-43`, initializing `fail_count` with `AtomicU32::new(2)` to
model “fails twice then succeeds”.
2. When `LlmRouter` uses this provider in `test_retry_succeeds_after_transient_failures()`
(`tests/llm_router_test.rs:238-258`), calls to `MockProvider::complete()`
(`tests/llm_router_test.rs:46-76`) execute the fail-count logic at lines 56-62.
3. On the first two calls, `remaining = count.fetch_sub(1, Ordering::SeqCst)` at line 57
returns `2` and `1`, respectively, causing the `if remaining > 0` branch at line 58 to
return `Err(LlmError::Provider(...))` while decrementing the atomic from `2` to `1` and
then `0`.
4. On the third call, `remaining` is `0`, so the method succeeds but `fetch_sub(1)`
underflows the atomic to `u32::MAX`; subsequent calls then see `remaining` values starting
at `u32::MAX` (> 0) and continue to error, contradicting the intended “fail N times before
succeeding” behavior and potentially misleading future tests that reuse the same
`MockProvider` instance for additional retries.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/llm_router_test.rs
**Line:** 56:62
**Comment:**
*Logic Error: `fetch_sub(1)` underflows after the counter reaches zero, so after the first success the counter wraps to `u32::MAX` and subsequent calls start failing again. This contradicts the stated behavior of failing N times then succeeding. Use a compare-and-decrement pattern that does not decrement below zero.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72a5f73dd3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let jitter = | ||
| rand::thread_rng().gen_range(0..=self.retry_config.max_jitter_ms); |
There was a problem hiding this comment.
In this checkout, RUSTUP_TOOLCHAIN=1.95.0 cargo fmt --check fails on this newly added wrapping (and another HashSet assignment wrap in the same file), so the documented cargo fmt --check quality gate in CLAUDE.md/Taskfile.yml will reject the commit. Please run rustfmt and commit the formatting changes.
Useful? React with 👍 / 👎.
Implement top 3 concrete fixes from P3 audit remediation backlog:
1. Security (retry with exponential backoff + timeout enforcement):
- Add RetryConfig with configurable max_attempts, base_delay_ms, timeout
- Implement exponential backoff sleep in LlmRouter::complete()
- Add TimeoutError variant to LlmError
- 4 new tests: retry_config_default_values, router_creation_with_custom_retry,
timeout_error_is_retryable, provider_error_is_retryable
2. Observability (L5 tracing instrumentation):
- Add tracing dependency and instrument macros to all 3 crates
- #[instrument] on all public API methods
- Span fields for request_id, provider_name, model, error type
- Recovers from missing tracing-subscriber gracefully
3. Error handling (L14 recovery hints):
- Add recovery_hint() method on LlmError, McpError, EmbeddingError
- Each error variant returns actionable next-step guidance
- Integration test verifies hints are non-empty for all variants
- Display impls sanitized: no secrets leaked in error messages
Build: cargo build --workspace (clean, zero warnings)
Lint: cargo clippy --workspace --all-targets (clean, zero warnings)
Tests: 30/30 pass (16 llm-router + 6 mcp-server + 8 pheno-embedding)
KooshaPari
force-pushed
the
fix/p3-rem-phenoAI-8
branch
from
72a5f73 to
0add604
Compare
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
codeant-ai
Bot
added
size:L
|
CodeAnt AI Incremental review completed. |
1 participant
User description
Summary
Remediate the top 3 P3 audit findings from the .audit-run-v37 backlog:
Changes
1. Retry with exponential backoff + timeout (
llm-router)RetryConfigstruct withmax_attempts(default 3),base_delay_ms(500), andtimeout(30s)LlmRouter::complete()now retries on retryable errors (RateLimited, ProviderError, TimeoutError) with exponential backoff:2^n * base_delayLlmError::TimeoutErrorvariant for request deadline enforcementis_retryable()added toLlmError2. Tracing instrumentation (all crates)
#[instrument]onLlmRouter::complete(),LlmRouter::provider_for_model(),McpServer::call_tool(),McpServer::read_resource(),OpenaiEmbeddings::embed(), and key methodsprovider_name,model,request_id,error_type,retry_attempttracing-subscriberis installed3. Recovery hints (all crates)
LlmError::recovery_hint() -> &'static str— e.g., ```InvalidModel`` → "Check that the model name is correct and the provider supports it"McpError::recovery_hint()andEmbeddingError::recovery_hint()Quality Gates
cargo build --workspacecargo clippy --workspace --all-targetscargo test --workspacecargo deny checkTesting
CodeAnt-AI Description
Add recovery hints, tracing, and input checks across server and embedding flows
What Changed
Impact
✅ Clearer error messages✅ Fewer empty embedding requests✅ Easier debugging of tool, resource, and embedding failures💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.