fix: enforce strict clippy linting across entire workspace - #489
Merged
Merged
Conversation
slin1237
requested review from
CatherineSue,
key4ng,
tonyluj and
whybeyoung
as code owners
February 21, 2026 01:10
github-actions
Bot
added
tokenizer
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a workspace Clippy lint configuration and applies widespread mechanical changes: formatting interpolation, lint attribute shifts (#[allow] → #[expect]), parking_lot locks, clone_from optimizations, many async→sync conversions, and numerous signature/receiver adjustments across core, router, worker, protocols, multimodal, and tests. No new runtime features. Changes
Sequence Diagram(s)Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Comment |
Contributor
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
![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: 156e4eab55
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
model_gateway/src/routers/parse/handlers.rs (2)
57-62: 🧹 Nitpick | 🔵 TrivialInconsistent format string style within function.
Line 61 now uses inlined format args
{e}, but lines 42 and 58 in this same function still use the positional"{}", varstyle. Consider updating all format strings for consistency.♻️ Suggested fix to align format string style
return error_response( StatusCode::BAD_REQUEST, - &format!("Unknown tool parser: {}", req.tool_call_parser), + &format!("Unknown tool parser: {}", req.tool_call_parser), // line 42: consider {req.tool_call_parser} if clippy permits ); }; let parser = pooled_parser.lock().await; match parser.parse_complete(&req.text).await { // ... Err(e) => { - error!("Failed to parse function calls: {}", e); + error!("Failed to parse function calls: {e}"); error_response( StatusCode::BAD_REQUEST, &format!("Failed to parse function calls: {e}"), ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/parse/handlers.rs` around lines 57 - 62, The error logging and error_response calls in this handler use mixed format styles (some use "{}", var and one uses "{e}"); make them consistent by switching all format strings in this function to the inline named-argument style (e.g., error!("Failed to parse function calls: {e}") and error_response(StatusCode::BAD_REQUEST, &format!("Failed to parse function calls: {e}")), and update the other error! and format! calls in the same function (the ones using "{}", var) to the {name} form so all uses (error!, format!, error_response) consistently use named inline placeholders.
94-99: 🧹 Nitpick | 🔵 TrivialSame inconsistency as above.
Line 98 uses inlined
{e}, but lines 79 and 95 still use positional args. For consistency, consider updating these as well.♻️ Suggested fix
return error_response( StatusCode::BAD_REQUEST, - &format!("Unknown reasoning parser: {}", req.reasoning_parser), + &format!("Unknown reasoning parser: {}", req.reasoning_parser), // consider inlining if clippy permits ); }; let mut parser = pooled_parser.lock().await; match parser.detect_and_parse_reasoning(&req.text) { // ... Err(e) => { - error!("Failed to separate reasoning: {}", e); + error!("Failed to separate reasoning: {e}"); error_response( StatusCode::BAD_REQUEST, &format!("Failed to separate reasoning: {e}"), ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/parse/handlers.rs` around lines 94 - 99, The log and response formatting in the Err(e) arm are inconsistent: the error! macro uses positional "{}" while the error_response uses inline "{e}"; update both places to the same style (pick one convention—e.g., use "{}" for both) so the error message for "Failed to separate reasoning" is formatted consistently across error! and error_response calls in handlers.rs (identify the error! invocation and the error_response(... format!(...)) call and make them use the same formatter).mcp/src/approval/manager.rs (1)
261-270:⚠️ Potential issue | 🟡 MinorUnreachable match arm with misleading fallback.
When
approvedisfalse(line 264 branch),decisionis alwaysApprovalDecision::Denied(set at lines 255-258). TheApprovedarm at line 267 is unreachable, and its "User denied" text is semantically incorrect for an approved decision.If this arm was added for exhaustiveness, consider making the invariant explicit with
unreachable!():Proposed fix
reason: match &decision { ApprovalDecision::Denied { reason } => reason.clone(), - ApprovalDecision::Approved => "User denied".to_string(), + ApprovalDecision::Approved => unreachable!("decision is Denied when !approved"), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/approval/manager.rs` around lines 261 - 270, The match on decision when building result contains an unreachable ApprovalDecision::Approved arm and a misleading "User denied" message; update the match in the Denied branch of result (where approved is false) to handle only the expected ApprovalDecision::Denied case and replace the Approved arm with an explicit unreachable!() (or other assertion) to make the invariant clear — locate the creation of result and the match over ApprovalDecision to remove the incorrect fallback and use unreachable!() (or panic/assert) for the Approved variant so the code documents the invariant and avoids the misleading string.model_gateway/build.rs (1)
13-20:⚠️ Potential issue | 🟠 MajorAvoid silent fallback on build metadata failures.
read_cargo_version()andget_rustc_host()failures now quietly default to0.0.0/ empty target, which can ship incorrect metadata without any signal. At minimum, emit acargo:warningwhen falling back (or consider failing the build if version/target is required).💡 Suggested fix (warn on fallback)
- let version = read_cargo_version().unwrap_or_else(|_| DEFAULT_VERSION.to_string()); - let target = std::env::var("TARGET").unwrap_or_else(|_| get_rustc_host().unwrap_or_default()); + let version = read_cargo_version().unwrap_or_else(|e| { + println!("cargo:warning=failed to read Cargo.toml version: {e}"); + DEFAULT_VERSION.to_string() + }); + let target = std::env::var("TARGET").unwrap_or_else(|e| { + println!("cargo:warning=missing TARGET env var: {e}"); + get_rustc_host().unwrap_or_else(|| { + println!("cargo:warning=failed to determine rustc host triple"); + String::new() + }) + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/build.rs` around lines 13 - 20, The current build script silently falls back to DEFAULT_VERSION and an empty host when read_cargo_version() or get_rustc_host() fail; update the logic around read_cargo_version()/DEFAULT_VERSION and get_rustc_host()/TARGET to emit a visible warning (e.g., println!("cargo:warning=...")) when those functions return Err or when you use DEFAULT_VERSION/get_rustc_host defaulting, or optionally return a non-zero exit (panic!) if version/target must be required; specifically, wrap the calls to read_cargo_version() and get_rustc_host() so failures produce a cargo:warning with context (including the error) before applying DEFAULT_VERSION or empty target, referencing the read_cargo_version, get_rustc_host, DEFAULT_VERSION, and the TARGET/PROFILE env var logic so the warning appears during cargo build.model_gateway/src/policies/cache_aware.rs (1)
155-160: 🧹 Nitpick | 🔵 TrivialConsider avoiding the clone entirely by restructuring.
Since
mesh_syncis passed by value and only used afterward for theis_some()check, you can eliminate the clone by checking first, then moving:♻️ Proposed refactor to avoid clone
pub fn set_mesh_sync(&mut self, mesh_sync: OptionalMeshSyncManager) { - self.mesh_sync.clone_from(&mesh_sync); - if mesh_sync.is_some() { + let should_restore = mesh_sync.is_some(); + self.mesh_sync = mesh_sync; + if should_restore { self.restore_tree_state_from_mesh(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/policies/cache_aware.rs` around lines 155 - 160, In set_mesh_sync, avoid the unnecessary clone of mesh_sync by checking mesh_sync.is_some() first and then moving mesh_sync into the struct field only when needed; specifically, use the OptionalMeshSyncManager parameter directly (move it into self.mesh_sync) instead of calling self.mesh_sync.clone_from(&mesh_sync), and call restore_tree_state_from_mesh() after the move when mesh_sync was Some; reference: function set_mesh_sync, parameter mesh_sync, field self.mesh_sync, and method restore_tree_state_from_mesh.model_gateway/src/policies/consistent_hashing.rs (1)
163-177:⚠️ Potential issue | 🟠 MajorAvoid TOCTOU between health counting and selection (can yield
Nonewith healthy workers).On Line 164–175, a health change between the count and
nthcan produceNoneeven when at least one worker remains healthy, and the branch is still markedRandomFallback. This can surface as false “no worker” results and misleading metrics.✅ Suggested fix (single pass over healthy workers)
- let healthy_count = workers.iter().filter(|w| w.is_healthy()).count(); - if healthy_count == 0 { - return (None, Branch::NoHealthyWorkers); - } - - let random_healthy_idx = rand::rng().random_range(0..healthy_count); - let idx = workers - .iter() - .enumerate() - .filter(|(_, w)| w.is_healthy()) - .nth(random_healthy_idx) - .map(|(i, _)| i); - - (idx, Branch::RandomFallback) + let healthy_indices: Vec<usize> = workers + .iter() + .enumerate() + .filter(|(_, w)| w.is_healthy()) + .map(|(i, _)| i) + .collect(); + if healthy_indices.is_empty() { + return (None, Branch::NoHealthyWorkers); + } + + let random_healthy_idx = rand::rng().random_range(0..healthy_indices.len()); + let idx = Some(healthy_indices[random_healthy_idx]); + + (idx, Branch::RandomFallback)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/policies/consistent_hashing.rs` around lines 163 - 177, The current two-pass logic over workers risks TOCTOU: between counting healthy workers and selecting the nth healthy, health can change and idx become None while healthy_count > 0; fix by performing a single pass to collect the indices of healthy workers (use workers.iter().enumerate().filter(|(_, w)| w.is_healthy()).map(|(i, _)| i).collect::<Vec<_>>()), then if the collected Vec is empty return (None, Branch::NoHealthyWorkers), otherwise choose a random index within that Vec (replace random_healthy_idx with rand::rng().random_range(0..healthy_indices.len())) and return (Some(healthy_indices[random_idx]), Branch::RandomFallback) so selection cannot race with the count.grpc_client/src/sglang_scheduler.rs (2)
520-528:⚠️ Potential issue | 🟡 MinorMissing
"grammar"alias for constraint type.SGLang only matches
"ebnf"while TRT-LLM matches"ebnf" | "grammar"and vLLM matches"grammar" | "ebnf". This inconsistency means if"grammar"is passed as the constraint type, it will succeed on TRT-LLM and vLLM but return"Unknown constraint type"on SGLang.🐛 Proposed fix to add grammar alias
"structural_tag" => { proto::sampling_params::Constraint::StructuralTag(constraint_value) } "json_schema" => proto::sampling_params::Constraint::JsonSchema(constraint_value), - "ebnf" => proto::sampling_params::Constraint::EbnfGrammar(constraint_value), + "ebnf" | "grammar" => proto::sampling_params::Constraint::EbnfGrammar(constraint_value), "regex" => proto::sampling_params::Constraint::Regex(constraint_value),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_client/src/sglang_scheduler.rs` around lines 520 - 528, The match on constraint_type (creating tool_constraint) misses the "grammar" alias and currently only handles "ebnf", causing "grammar" inputs to hit the default Unknown constraint error; update the match in the block that builds tool_constraint (the match over constraint_type.as_str() that produces proto::sampling_params::Constraint variants) to accept "grammar" as an additional arm mapping to proto::sampling_params::Constraint::EbnfGrammar(constraint_value) (e.g., add a "grammar" => proto::...::EbnfGrammar(constraint_value) branch or combine it with the "ebnf" arm).
580-588:⚠️ Potential issue | 🟡 MinorSame missing
"grammar"alias inbuild_constraint_for_responses.This function has the same inconsistency as
build_constraint_for_chat.🐛 Proposed fix
"json_schema" => proto::sampling_params::Constraint::JsonSchema(constraint_value), - "ebnf" => proto::sampling_params::Constraint::EbnfGrammar(constraint_value), + "ebnf" | "grammar" => proto::sampling_params::Constraint::EbnfGrammar(constraint_value), "regex" => proto::sampling_params::Constraint::Regex(constraint_value),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_client/src/sglang_scheduler.rs` around lines 580 - 588, The match in build_constraint_for_responses is missing the "grammar" alias (same inconsistency as build_constraint_for_chat); update the match on constraint_type in build_constraint_for_responses to treat "grammar" the same as "ebnf" (e.g., match "ebnf" | "grammar" => proto::sampling_params::Constraint::EbnfGrammar(constraint_value)) so both aliases produce an EbnfGrammar constraint.data_connector/src/memory.rs (1)
237-258:⚠️ Potential issue | 🔴 CriticalFix lock-order inversion to prevent deadlocks.
delete_itemacquiresrev_indexthenlinks, whilelink_itemacquireslinksthenrev_index. Concurrent calls can deadlock. Enforce a single lock order everywhere (e.g.,links→rev_index).🔒 Proposed fix (consistent lock order)
- let key_to_remove = { - let mut rev = self.rev_index.write(); - if let Some(conv_idx) = rev.get_mut(conversation_id) { - conv_idx.remove(&item_id.0) - } else { - None - } - }; - - // If the item was in rev_index, remove it from links as well - if let Some(key) = key_to_remove { - let mut links = self.links.write(); - if let Some(conv_links) = links.get_mut(conversation_id) { - conv_links.remove(&key); - } - } + let mut links = self.links.write(); + let mut rev = self.rev_index.write(); + if let Some(conv_idx) = rev.get_mut(conversation_id) { + if let Some(key) = conv_idx.remove(&item_id.0) { + if let Some(conv_links) = links.get_mut(conversation_id) { + conv_links.remove(&key); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/memory.rs` around lines 237 - 258, delete_item currently acquires rev_index then links, which can deadlock with link_item that acquires links then rev_index; change delete_item to acquire locks in the same order as link_item (links then rev_index) by first taking a write lock on self.links to remove any entry for conversation_id and capture the key to remove, then taking a write lock on self.rev_index to remove the corresponding rev_index entry (or vice‑versa if you prefer the canonical order links → rev_index used across the codebase); update the logic in delete_item to use that single consistent ordering and ensure you still only hold each lock for the minimal time while preserving the same behavior.model_gateway/src/routers/grpc/utils.rs (2)
932-941: 🧹 Nitpick | 🔵 TrivialLog tokenizer decode failures before falling back.
Now that logprob conversion is infallible, decode errors are silently masked; add a warn to preserve observability.Suggested fix
convert_proto_logprobs(proto_logprobs, |token_id| { - tokenizer - .decode(&[token_id], false) - .unwrap_or_else(|_| format!("<token_{token_id}>")) + tokenizer.decode(&[token_id], false).unwrap_or_else(|e| { + warn!(token_id, error = %e, "Failed to decode token id for logprobs"); + format!("<token_{token_id}>") + }) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/utils.rs` around lines 932 - 941, The conversion currently swallows tokenizer.decode errors in convert_proto_to_openai_logprobs; update the closure passed to convert_proto_logprobs so that when tokenizer.decode(&[token_id], false) returns Err(e) you log a warning (e.g., tracing::warn! or log::warn!) including the token_id and the error, then return the same fallback format!("<token_{token_id}>"). Keep the call target symbols: convert_proto_to_openai_logprobs, convert_proto_logprobs, and tokenizer.decode.
452-468:⚠️ Potential issue | 🟡 MinorAvoid dropping the last assistant message when content isn’t a string.
Incontinue_final_messagemode the last assistant message is always popped, but if itscontentisn’t a string (e.g., tool_calls or multimodal), the message is lost. Only pop after confirming a string prefix.Suggested fix
- let assistant_prefix = if request.continue_final_message - && !transformed_messages.is_empty() - && transformed_messages - .last() - .and_then(|msg| msg.get("role")) - .and_then(|v| v.as_str()) - == Some("assistant") - { - // Pop the last message to handle it separately — guarded by !is_empty() check above - let Some(last_msg) = transformed_messages.pop() else { - return Ok(ProcessedMessages { - text: String::new(), - multimodal_inputs: None, - stop_sequences: request.stop.clone(), - }); - }; - last_msg - .get("content") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()) - } else { - None - }; + let assistant_prefix = if request.continue_final_message + && !transformed_messages.is_empty() + && transformed_messages + .last() + .and_then(|msg| msg.get("role")) + .and_then(|v| v.as_str()) + == Some("assistant") + { + if let Some(prefix) = transformed_messages + .last() + .and_then(|msg| msg.get("content")) + .and_then(|v| v.as_str()) + { + transformed_messages.pop(); + Some(prefix.to_string()) + } else { + None + } + } else { + None + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/utils.rs` around lines 452 - 468, The code currently unconditionally pops the last assistant message when request.continue_final_message is true, which discards messages whose "content" is non-string; change the logic in the block that uses continue_final_message and transformed_messages so you inspect transformed_messages.last() (check its "role" == "assistant" and its "content".as_str() is Some) before calling pop; only call transformed_messages.pop() after confirming the content is a string and bind that to last_msg, otherwise leave the message in place and proceed to build ProcessedMessages without removing it (refer to continue_final_message, transformed_messages, last_msg, and the ProcessedMessages return).model_gateway/tests/common/mock_worker.rs (1)
592-1099:⚠️ Potential issue | 🟡 MinorStore generated background response IDs for later lookup.
Whenbackground=trueand norequest_idis supplied, you generate aridbut never insert it intoRESP_STORE, so subsequent GET/cancel returns 404. Store the generated id in the background branch.🔧 Proposed fix
- // Background storage simulation - let is_background = payload + // Background storage simulation + let is_background = payload .get("background") .and_then(|v| v.as_bool()) .unwrap_or(false); let req_id = payload .get("request_id") .and_then(|v| v.as_str()) .map(|s| s.to_string()); - if is_background { - if let Some(id) = &req_id { - store_response_for_port(config.port, id); - } - } @@ - } else if is_background { - let rid = req_id.unwrap_or_else(|| format!("resp-{}", Uuid::new_v4())); + } else if is_background { + let rid = req_id.unwrap_or_else(|| format!("resp-{}", Uuid::new_v4())); + store_response_for_port(config.port, &rid); Json(json!({ "id": rid, "object": "response",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/tests/common/mock_worker.rs` around lines 592 - 1099, In responses_handler, when handling the is_background branch you generate a rid via req_id.unwrap_or_else(|| format!("resp-{}", Uuid::new_v4())) but never insert it into RESP_STORE, causing later GET/cancel to 404; fix by calling the existing helper store_response_for_port (or otherwise inserting into RESP_STORE) with config.port and the generated rid immediately after you create rid (i.e., in the is_background branch where rid is set) so the background-generated id is recorded for lookup; reference symbols: responses_handler, is_background, req_id, rid, RESP_STORE, store_response_for_port.model_gateway/src/core/steps/worker/local/discover_metadata.rs (1)
18-27:⚠️ Potential issue | 🟡 MinorHandle HTTP client initialization failures gracefully instead of panicking.
reqwest::Client::builder().build()can fail for multiple reasons per the official documentation: TLS backend initialization, DNS resolver configuration issues, invalid TLS version/certificate settings, or unrecognized TLS backends. While some are infrastructure-level issues, the currentexpect()will crash the process on first HTTP use. Since this codebase has a gRPC fallback path (viaConnectionMode), HTTP client build failures should be propagated as errors rather than causing a panic.Wrap the
HTTP_CLIENTinitialization in aLazy<Result<Client, _>>and propagate the error from HTTP-only call sites:Suggested pattern
-static HTTP_CLIENT: Lazy<Client> = Lazy::new(|| { - Client::builder() - .timeout(Duration::from_secs(10)) - .build() - .expect("Failed to create HTTP client") -}); +static HTTP_CLIENT: Lazy<Result<Client, reqwest::Error>> = Lazy::new(|| { + Client::builder() + .timeout(Duration::from_secs(10)) + .build() +});Then in
get_json_with_fallbackandhttp_get_json:- let mut req = HTTP_CLIENT.get(&url); + let client = HTTP_CLIENT + .as_ref() + .map_err(|e| format!("Failed to create HTTP client: {e}"))?; + let mut req = client.get(&url);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/core/steps/worker/local/discover_metadata.rs` around lines 18 - 27, The static HTTP_CLIENT currently panics on build failure; change its type to Lazy<Result<Client, reqwest::Error>> and return the build error instead of calling expect, then update call sites (get_json_with_fallback and http_get_json) to handle the Result by propagating an Err when HTTP_CLIENT is Err and only use the Client when Ok(client), falling back to the existing gRPC path via ConnectionMode; ensure error types are converted/mapped to the function's error return so callers receive a propagated error instead of the process panicking.model_gateway/src/routers/grpc/regular/streaming.rs (1)
1200-1214: 🧹 Nitpick | 🔵 TrivialConsider context struct pattern for argument reduction.
This function has 13 parameters. The PR introduces
GenerateStreamContext(lines 46-53) to reduce arguments for generate streaming. A similarChatStreamContextstruct could bundle common parameters (request_id,model,created,system_fingerprint,history_tool_calls_count) for cleaner signatures in chat streaming helpers.Not blocking, as the
#[expect(clippy::too_many_arguments)]is appropriate for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 1200 - 1214, The function process_tool_calls_stream currently accepts 13 parameters; create a ChatStreamContext struct (similar to GenerateStreamContext) that bundles common chat-streaming fields (request_id, model, created, system_fingerprint, history_tool_calls_count and any other repeated metadata) and replace those individual parameters in process_tool_calls_stream with a single &ChatStreamContext argument; update any helper calls and signatures that pass those same fields to accept &ChatStreamContext, and ensure tool_parsers, has_tool_calls, tools, delta, index, and use_json_parser remain as explicit params so callers and semantics are unchanged aside from the reduced argument list.bindings/golang/src/tool_parser.rs (1)
223-234:⚠️ Potential issue | 🟡 MinorSilent JSON parsing failure may hide tool configuration errors.
When
tools_jsonis non-null but contains invalid JSON,unwrap_or_default()at line 233 silently returns an emptyVec<Tool>. This could mask configuration errors from callers who provided malformed tool definitions.Consider returning an error to the caller instead of silently falling back to an empty vector, similar to how UTF-8 errors are handled on lines 228-231.
Proposed fix
let tools: Vec<Tool> = if tools_json.is_null() { vec![] } else { let tools_str = match CStr::from_ptr(tools_json).to_str() { Ok(s) => s, Err(_) => { set_error_message(error_out, "Invalid UTF-8 in tools_json"); return SglErrorCode::InvalidArgument; } }; - serde_json::from_str::<Vec<Tool>>(tools_str).unwrap_or_default() + match serde_json::from_str::<Vec<Tool>>(tools_str) { + Ok(t) => t, + Err(e) => { + set_error_message(error_out, &format!("Failed to parse tools JSON: {e}")); + return SglErrorCode::ParsingError; + } + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/golang/src/tool_parser.rs` around lines 223 - 234, The JSON parsing currently uses serde_json::from_str(...).unwrap_or_default(), which silently drops malformed JSON; change this to handle the Result from serde_json::from_str for Vec<Tool> and on Err call set_error_message(error_out, "<descriptive message>") with the serde_json error text and return SglErrorCode::InvalidArgument (mirroring the UTF-8 error path). Specifically, replace the unwrap_or_default branch that produces tools with a match on serde_json::from_str::<Vec<Tool>>(tools_str) so that Ok(v) sets tools = v and Err(e) sets an error message (include e.to_string()) and returns SglErrorCode::InvalidArgument.model_gateway/src/routers/http/router.rs (1)
134-145:⚠️ Potential issue | 🟠 MajorModel routing ignores
model_idin non-IGW mode, causing incorrect worker selection.When
enable_igw=false,effective_model_idis set toNone(line 140), causingget_workers_filtered()to return all workers regardless of the providedmodel_id. The hash ring then falls back toUNKNOWN_MODEL_ID. This contradictsRouterManager's behavior, which preservesmodel_idin non-IGW mode using.or(model_id). If workers are registered with specific models, requests will route to the wrong pool. Either model-aware routing should be consistently applied in non-IGW mode, or this behavior needs explicit justification if single-model deployment is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/http/router.rs` around lines 134 - 145, select_worker_for_model currently forces effective_model_id to None when enable_igw is false, causing get_workers_filtered to ignore the provided model_id and fallback to UNKNOWN_MODEL_ID; change the logic to preserve the incoming model_id in non-IGW mode (match RouterManager behavior) so get_workers_filtered receives either the IGW-resolved id or the original model_id: adjust the computation of effective_model_id in select_worker_for_model (and any related callers) to use the IGW-resolved id when enable_igw is true, otherwise pass through the provided model_id, ensuring get_workers_filtered and the hash ring get the correct model identifier (reference: select_worker_for_model, effective_model_id, get_workers_filtered, RouterManager, UNKNOWN_MODEL_ID).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kv_index/src/string_tree.rs`:
- Around line 481-484: The code currently silently returns when
contracted_text.first_char() is None, dropping the insert on an invariant
violation; replace this silent early-return with a fail-fast assertion (e.g.,
use unreachable! or panic!) so corruption is surfaced. Specifically, in the
block that checks let Some(first_new_char) = contracted_text.first_char() else {
... }, remove the return and instead call unreachable! or panic! with a message
that includes context (e.g., mention split_at_char, shared_count < char_count,
and include contracted_text or its length) so the invariant failure is explicit
and debuggable.
In `@mcp/src/core/oauth.rs`:
- Around line 149-155: The background OAuth callback server spawned in the
tokio::spawn block currently moves the listener into the task and never shuts
down, so replace the fire-and-forget axum::serve(listener, app).await with a
graceful shutdown: create a oneshot shutdown signal (sender/receiver) in the
scope where the auth flow completes, move the receiver into the spawned task and
call axum::Server::from_tcp(listener)?.with_graceful_shutdown(async {
receiver.await.ok(); }).serve(app.into_make_service()).await (i.e., use
with_graceful_shutdown), and send the shutdown signal (sender.send(())) right
after the authorization code is received so the listener is closed and the port
is released; update any error handling around tokio::spawn and
axum::serve(listener, app) accordingly.
In `@model_gateway/src/core/token_bucket.rs`:
- Around line 58-63: Validate token inputs at the public API boundaries: in
try_acquire (and its call to try_acquire_sync), acquire, and return_tokens check
that the provided tokens value is finite (not NaN/inf), not negative, and does
not exceed the bucket capacity; for invalid values return a failure immediately
(e.g., Err(()) for try_acquire) instead of proceeding, and keep the existing
behavior for valid inputs by delegating to try_acquire_sync or the existing
implementation after the guard.
In `@model_gateway/src/middleware.rs`:
- Around line 425-428: The #[expect(..., reason = "...")] attached to the
clippy::disallowed_methods attribute has an inaccurate reason string; update the
reason to state that the spawned, fire-and-forget task is bounded by
remaining_timeout (used with acquire_timeout) rather than the request lifetime,
and note that dropping the oneshot receiver does not cancel the spawned task—it
only notifies the sender while the task continues until acquire_timeout
completes or the runtime shuts down; change the reason text accordingly on the
#[expect(...)] for clarity.
In `@model_gateway/src/policies/bucket.rs`:
- Around line 147-169: The bucket state isn't fully reset when the last URL is
removed—stale entries remain in boundary and chars_per_url causing invalid
selections; after computing updated_len and updated_urls (from
bucket_guard.prefill_worker_urls and bucket_guard.chars_per_url), set
bucket_guard.bucket_cnt and always call
bucket_guard.init_prefill_worker_urls(updated_urls) (do not only call it when
updated_len>0), and when updated_len == 0 explicitly clear/reset remaining
state: clear bucket_guard.boundary and clear the chars_per_url map (via the
chars_per_url lock) so the bucket has no leftover entries.
- Around line 116-137: The code always reinitializes bucket state even when the
worker_url already exists; change add_prefill_url so you detect whether the push
actually inserted a new URL (by checking prefill_worker_urls.contains before
mutation) and only when it's newly inserted perform
chars_per_url.entry(worker_url.clone()).or_insert(0) and call
bucket_guard.init_prefill_worker_urls(cloned); otherwise skip the init and skip
zeroing per-URL state to preserve balancing history, and adjust the info! log to
only report "Added worker ..." when insertion occurred (log a different message
or skip logging for duplicates).
In `@model_gateway/src/routers/grpc/client.rs`:
- Around line 189-194: Replace the panic in the fallback match arm that
currently calls panic!("Mismatched client and request types") with returning a
proper Err value so callers can handle the mismatch; locate the match containing
the arm `_ => panic!("Mismatched client and request types")` in client.rs and
change it to return an error (e.g., Err("Mismatched client and request
types".into()) or a typed crate error) consistent with the surrounding
function's Result return type (adjust the function signature to return Result if
needed), and preserve the `#[expect(...)]` comment if you want to keep the
invariant noted.
- Around line 206-211: The catch-all match arm currently panics ("Mismatched
client and request types or unsupported embedding backend"); change it to return
a gRPC error for unsupported backends instead of panicking: in the match inside
the client implementation in client.rs, replace the `_ => panic!(...)` arm with
logic that detects unsupported embedding backends and returns an appropriate
tonic::Status (e.g., Status::unimplemented or Status::invalid_argument with a
clear message like "unsupported embedding backend"), while keeping any explicit
panic/expect for true invariant violations (mismatched client/request types) if
you must; reference the existing match and the `_ =>` arm to locate where to
return Err(Status::...) so callers receive a proper error instead of a panic.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 372-417: The code currently discards normal content when tool
parsing fails because process_tool_calls_stream returns an empty Vec and the
surrounding code unconditionally continues; change process_tool_calls_stream
(and/or process_specific_function_stream) to surface parse failures (e.g.,
return Result<Vec<Chunk>, ParseError> or include a parse_failed flag), then in
the caller check that result: on Ok(chunks) behave as today, but on Err(_) (or
parse_failed) log the error (include request_id/model) and do NOT execute the
continue so the delta falls through to regular content emission; alternatively,
if you prefer to keep the current signature, make it return a tuple (chunks,
parse_failed:bool) and only continue when parse_failed == false and chunks are
non-empty—also consider referencing get_unstreamed_tool_args if you opt to
implement buffering instead of falling through.
In `@model_gateway/tests/metrics_aggregator_test.rs`:
- Around line 260-261: The call to assert_eq_sorted(result.trim(),
expected.trim()) uses redundant .trim() calls because assert_eq_sorted already
trims both inputs; remove the .trim() invocations and pass result and expected
directly to assert_eq_sorted to match the other test usage and avoid
double-trimming.
In `@model_gateway/tests/otel_tracing_test.rs`:
- Around line 249-254: The assertion currently uses exact equality on span_count
and an inconsistent error message; change the check in the test (reference
span_count) to assert that span_count >= 2 if the intent is "at least 2 spans",
update the assertion message to read "Expected to receive at least 2 spans, but
got {}." and use proper formatting placeholder with span_count as an argument,
and likewise update the println to "Test passed! Collector received {} spans"
with span_count passed to the formatter so pluralization and interpolation are
correct.
In `@multimodal/src/tracker.rs`:
- Around line 166-169: The reason string on the
#[expect(clippy::disallowed_methods, reason = "...")] attribute incorrectly
references resolve(); update that explanatory text to reference finalize() (the
actual method that awaits pending tasks) so the attribute accurately describes
why the disallowed method is allowed for the spawn handle stored in
self.pending; locate the expect attribute applied to the spawn/fire-and-forget
block in tracker.rs and change "resolve()" to "finalize()".
In `@protocols/src/responses.rs`:
- Around line 930-933: The inline comment above the guard uses the `// SAFETY:`
prefix which is reserved for explanations around unsafe blocks; rename it to `//
INVARIANT:` (or `// Note:`) to indicate this documents a safe code invariant and
avoid confusion—update the comment immediately above the `let Some(tools) =
request.tools.as_ref() else { return Ok(()); };` guard in the `responses.rs`
code so it reads `// INVARIANT:` (or `// Note:`) and leave the rest of the
explanatory text unchanged.
In `@protocols/src/worker.rs`:
- Around line 338-343: The match arm handling the single-element case is overly
defensive: after checking models.len() == 1, calling models.into_iter().next()
cannot return None, yet the code falls back to Self::Wildcard; replace the
fallback with a direct unwrap/expect so the invariant is explicit — i.e., in the
branch that uses models.into_iter().next() for construction of
Self::Single(Box::new(...)), swap the conditional Some(...) else { return
Self::Wildcard } for models.into_iter().next().expect("expected one model when
models.len() == 1") and construct Self::Single from that value, removing the
unreachable Wildcard fallback.
---
Outside diff comments:
In `@bindings/golang/src/tool_parser.rs`:
- Around line 223-234: The JSON parsing currently uses
serde_json::from_str(...).unwrap_or_default(), which silently drops malformed
JSON; change this to handle the Result from serde_json::from_str for Vec<Tool>
and on Err call set_error_message(error_out, "<descriptive message>") with the
serde_json error text and return SglErrorCode::InvalidArgument (mirroring the
UTF-8 error path). Specifically, replace the unwrap_or_default branch that
produces tools with a match on serde_json::from_str::<Vec<Tool>>(tools_str) so
that Ok(v) sets tools = v and Err(e) sets an error message (include
e.to_string()) and returns SglErrorCode::InvalidArgument.
In `@data_connector/src/memory.rs`:
- Around line 237-258: delete_item currently acquires rev_index then links,
which can deadlock with link_item that acquires links then rev_index; change
delete_item to acquire locks in the same order as link_item (links then
rev_index) by first taking a write lock on self.links to remove any entry for
conversation_id and capture the key to remove, then taking a write lock on
self.rev_index to remove the corresponding rev_index entry (or vice‑versa if you
prefer the canonical order links → rev_index used across the codebase); update
the logic in delete_item to use that single consistent ordering and ensure you
still only hold each lock for the minimal time while preserving the same
behavior.
In `@grpc_client/src/sglang_scheduler.rs`:
- Around line 520-528: The match on constraint_type (creating tool_constraint)
misses the "grammar" alias and currently only handles "ebnf", causing "grammar"
inputs to hit the default Unknown constraint error; update the match in the
block that builds tool_constraint (the match over constraint_type.as_str() that
produces proto::sampling_params::Constraint variants) to accept "grammar" as an
additional arm mapping to
proto::sampling_params::Constraint::EbnfGrammar(constraint_value) (e.g., add a
"grammar" => proto::...::EbnfGrammar(constraint_value) branch or combine it with
the "ebnf" arm).
- Around line 580-588: The match in build_constraint_for_responses is missing
the "grammar" alias (same inconsistency as build_constraint_for_chat); update
the match on constraint_type in build_constraint_for_responses to treat
"grammar" the same as "ebnf" (e.g., match "ebnf" | "grammar" =>
proto::sampling_params::Constraint::EbnfGrammar(constraint_value)) so both
aliases produce an EbnfGrammar constraint.
In `@mcp/src/approval/manager.rs`:
- Around line 261-270: The match on decision when building result contains an
unreachable ApprovalDecision::Approved arm and a misleading "User denied"
message; update the match in the Denied branch of result (where approved is
false) to handle only the expected ApprovalDecision::Denied case and replace the
Approved arm with an explicit unreachable!() (or other assertion) to make the
invariant clear — locate the creation of result and the match over
ApprovalDecision to remove the incorrect fallback and use unreachable!() (or
panic/assert) for the Approved variant so the code documents the invariant and
avoids the misleading string.
In `@model_gateway/build.rs`:
- Around line 13-20: The current build script silently falls back to
DEFAULT_VERSION and an empty host when read_cargo_version() or get_rustc_host()
fail; update the logic around read_cargo_version()/DEFAULT_VERSION and
get_rustc_host()/TARGET to emit a visible warning (e.g.,
println!("cargo:warning=...")) when those functions return Err or when you use
DEFAULT_VERSION/get_rustc_host defaulting, or optionally return a non-zero exit
(panic!) if version/target must be required; specifically, wrap the calls to
read_cargo_version() and get_rustc_host() so failures produce a cargo:warning
with context (including the error) before applying DEFAULT_VERSION or empty
target, referencing the read_cargo_version, get_rustc_host, DEFAULT_VERSION, and
the TARGET/PROFILE env var logic so the warning appears during cargo build.
In `@model_gateway/src/core/steps/worker/local/discover_metadata.rs`:
- Around line 18-27: The static HTTP_CLIENT currently panics on build failure;
change its type to Lazy<Result<Client, reqwest::Error>> and return the build
error instead of calling expect, then update call sites (get_json_with_fallback
and http_get_json) to handle the Result by propagating an Err when HTTP_CLIENT
is Err and only use the Client when Ok(client), falling back to the existing
gRPC path via ConnectionMode; ensure error types are converted/mapped to the
function's error return so callers receive a propagated error instead of the
process panicking.
In `@model_gateway/src/policies/cache_aware.rs`:
- Around line 155-160: In set_mesh_sync, avoid the unnecessary clone of
mesh_sync by checking mesh_sync.is_some() first and then moving mesh_sync into
the struct field only when needed; specifically, use the OptionalMeshSyncManager
parameter directly (move it into self.mesh_sync) instead of calling
self.mesh_sync.clone_from(&mesh_sync), and call restore_tree_state_from_mesh()
after the move when mesh_sync was Some; reference: function set_mesh_sync,
parameter mesh_sync, field self.mesh_sync, and method
restore_tree_state_from_mesh.
In `@model_gateway/src/policies/consistent_hashing.rs`:
- Around line 163-177: The current two-pass logic over workers risks TOCTOU:
between counting healthy workers and selecting the nth healthy, health can
change and idx become None while healthy_count > 0; fix by performing a single
pass to collect the indices of healthy workers (use
workers.iter().enumerate().filter(|(_, w)| w.is_healthy()).map(|(i, _)|
i).collect::<Vec<_>>()), then if the collected Vec is empty return (None,
Branch::NoHealthyWorkers), otherwise choose a random index within that Vec
(replace random_healthy_idx with
rand::rng().random_range(0..healthy_indices.len())) and return
(Some(healthy_indices[random_idx]), Branch::RandomFallback) so selection cannot
race with the count.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1200-1214: The function process_tool_calls_stream currently
accepts 13 parameters; create a ChatStreamContext struct (similar to
GenerateStreamContext) that bundles common chat-streaming fields (request_id,
model, created, system_fingerprint, history_tool_calls_count and any other
repeated metadata) and replace those individual parameters in
process_tool_calls_stream with a single &ChatStreamContext argument; update any
helper calls and signatures that pass those same fields to accept
&ChatStreamContext, and ensure tool_parsers, has_tool_calls, tools, delta,
index, and use_json_parser remain as explicit params so callers and semantics
are unchanged aside from the reduced argument list.
In `@model_gateway/src/routers/grpc/utils.rs`:
- Around line 932-941: The conversion currently swallows tokenizer.decode errors
in convert_proto_to_openai_logprobs; update the closure passed to
convert_proto_logprobs so that when tokenizer.decode(&[token_id], false) returns
Err(e) you log a warning (e.g., tracing::warn! or log::warn!) including the
token_id and the error, then return the same fallback
format!("<token_{token_id}>"). Keep the call target symbols:
convert_proto_to_openai_logprobs, convert_proto_logprobs, and tokenizer.decode.
- Around line 452-468: The code currently unconditionally pops the last
assistant message when request.continue_final_message is true, which discards
messages whose "content" is non-string; change the logic in the block that uses
continue_final_message and transformed_messages so you inspect
transformed_messages.last() (check its "role" == "assistant" and its
"content".as_str() is Some) before calling pop; only call
transformed_messages.pop() after confirming the content is a string and bind
that to last_msg, otherwise leave the message in place and proceed to build
ProcessedMessages without removing it (refer to continue_final_message,
transformed_messages, last_msg, and the ProcessedMessages return).
In `@model_gateway/src/routers/http/router.rs`:
- Around line 134-145: select_worker_for_model currently forces
effective_model_id to None when enable_igw is false, causing
get_workers_filtered to ignore the provided model_id and fallback to
UNKNOWN_MODEL_ID; change the logic to preserve the incoming model_id in non-IGW
mode (match RouterManager behavior) so get_workers_filtered receives either the
IGW-resolved id or the original model_id: adjust the computation of
effective_model_id in select_worker_for_model (and any related callers) to use
the IGW-resolved id when enable_igw is true, otherwise pass through the provided
model_id, ensuring get_workers_filtered and the hash ring get the correct model
identifier (reference: select_worker_for_model, effective_model_id,
get_workers_filtered, RouterManager, UNKNOWN_MODEL_ID).
In `@model_gateway/src/routers/parse/handlers.rs`:
- Around line 57-62: The error logging and error_response calls in this handler
use mixed format styles (some use "{}", var and one uses "{e}"); make them
consistent by switching all format strings in this function to the inline
named-argument style (e.g., error!("Failed to parse function calls: {e}") and
error_response(StatusCode::BAD_REQUEST, &format!("Failed to parse function
calls: {e}")), and update the other error! and format! calls in the same
function (the ones using "{}", var) to the {name} form so all uses (error!,
format!, error_response) consistently use named inline placeholders.
- Around line 94-99: The log and response formatting in the Err(e) arm are
inconsistent: the error! macro uses positional "{}" while the error_response
uses inline "{e}"; update both places to the same style (pick one
convention—e.g., use "{}" for both) so the error message for "Failed to separate
reasoning" is formatted consistently across error! and error_response calls in
handlers.rs (identify the error! invocation and the error_response(...
format!(...)) call and make them use the same formatter).
In `@model_gateway/tests/common/mock_worker.rs`:
- Around line 592-1099: In responses_handler, when handling the is_background
branch you generate a rid via req_id.unwrap_or_else(|| format!("resp-{}",
Uuid::new_v4())) but never insert it into RESP_STORE, causing later GET/cancel
to 404; fix by calling the existing helper store_response_for_port (or otherwise
inserting into RESP_STORE) with config.port and the generated rid immediately
after you create rid (i.e., in the is_background branch where rid is set) so the
background-generated id is recorded for lookup; reference symbols:
responses_handler, is_background, req_id, rid, RESP_STORE,
store_response_for_port.
---
Duplicate comments:
In `@model_gateway/src/routers/grpc/proto_wrapper.rs`:
- Around line 483-528: The typed accessors as_sglang, as_sglang_mut, as_vllm,
and as_trtllm currently panic on a variant mismatch; update all call sites that
use these functions to first check the corresponding predicate (is_sglang,
is_vllm, is_trtllm) before calling the accessor, or replace usage with a safe
non-panicking alternative by adding
try_as_sglang/try_as_sglang_mut/try_as_vllm/try_as_trtllm that return
Option<&...>/Option<&mut ...> (or Result) and refactor callers to handle the
None/Error case. Ensure changes reference the methods as_sglang, as_sglang_mut,
as_vllm, as_trtllm and their predicate helpers is_sglang/is_vllm/is_trtllm so no
panics occur unexpectedly.
- Around line 345-378: The typed accessors as_sglang, as_vllm, and as_trtllm
currently panic on mismatches; add non-panicking alternatives (e.g.
try_as_sglang -> Option<&sglang::GenerateStreamChunk>, try_as_vllm ->
Option<&vllm::GenerateStreamChunk>, try_as_trtllm ->
Option<&trtllm::GenerateStreamChunk>) and update call sites to use these safe
helpers or explicitly check is_sglang()/is_vllm()/is_trtllm() before calling the
panicking as_* methods; keep the existing as_* for hot paths if you still want
panics but prefer callers to use try_as_* for correctness.
Comment on lines
+481
to
+484
| let Some(first_new_char) = contracted_text.first_char() else { | ||
| // split_at_char with shared_count < char_count guarantees non-empty suffix | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid silent early-return on an invariant break.
This branch should be unreachable; returning will silently drop the insert if the invariant is violated. Prefer a fail-fast expectation so corruption is surfaced.
Proposed change
- let Some(first_new_char) = contracted_text.first_char() else {
- // split_at_char with shared_count < char_count guarantees non-empty suffix
- return;
- };
+ let first_new_char = contracted_text
+ .first_char()
+ .expect("split_at_char with shared_count < char_count must yield non-empty suffix");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let Some(first_new_char) = contracted_text.first_char() else { | |
| // split_at_char with shared_count < char_count guarantees non-empty suffix | |
| return; | |
| }; | |
| let first_new_char = contracted_text | |
| .first_char() | |
| .expect("split_at_char with shared_count < char_count must yield non-empty suffix"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kv_index/src/string_tree.rs` around lines 481 - 484, The code currently
silently returns when contracted_text.first_char() is None, dropping the insert
on an invariant violation; replace this silent early-return with a fail-fast
assertion (e.g., use unreachable! or panic!) so corruption is surfaced.
Specifically, in the block that checks let Some(first_new_char) =
contracted_text.first_char() else { ... }, remove the return and instead call
unreachable! or panic! with a message that includes context (e.g., mention
split_at_char, shared_count < char_count, and include contracted_text or its
length) so the invariant failure is explicit and debuggable.
Member
Author
There was a problem hiding this comment.
Valid observation about invariant enforcement. However, changing error-handling behavior from a defensive return to a panic is a behavioral change beyond the scope of this clippy-focused PR. The defensive return avoids panics in production — a corrupted index is preferable to a crashed server. Will consider converting to debug_assert\! in a follow-up.
There was a problem hiding this comment.
@slin1237 Totally fair—agree we shouldn’t change runtime behavior in this Clippy PR. Let’s keep the defensive early-return.
If helpful, I can open a follow-up issue to:
- replace the early-return with a debug_assert!(...) so it’s caught in debug/tests but remains non-panicking in release
- add a short comment explaining the invariant and why we choose a defensive return in release
- optionally add a tracing::warn! behind cfg(debug_assertions) for extra visibility during dev
Want me to file that issue and link back to this discussion?
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: kv_index/src/string_tree.rs:481-484
Timestamp: 2026-02-21T02:30:52.128Z
Learning: Preference noted: In kv_index/src/string_tree.rs (Rust radix tree), production code should avoid panics on invariant breaches; prefer a defensive non-panicking path (e.g., early return) and use debug_assert! for debug/test builds. Consider documenting the invariant with a comment, and optionally log under cfg(debug_assertions).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+149
to
155
| #[expect( | ||
| clippy::disallowed_methods, | ||
| reason = "fire-and-forget OAuth callback server; runs until listener drops when auth flow completes" | ||
| )] | ||
| tokio::spawn(async move { | ||
| let _ = axum::serve(listener, app).await; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the oauth.rs file
fd "oauth.rs" --type fRepository: lightseekorg/smg
Length of output: 81
🏁 Script executed:
# Read the oauth.rs file to understand the context
wc -l mcp/src/core/oauth.rsRepository: lightseekorg/smg
Length of output: 85
🏁 Script executed:
# Get the specific lines mentioned in the review and surrounding context
sed -n '130,170p' mcp/src/core/oauth.rs | cat -nRepository: lightseekorg/smg
Length of output: 1673
🏁 Script executed:
# Search for the entire function containing this code to understand the full context
ast-grep --pattern $'fn $FUNC($$$) {
$$$
tokio::spawn(async move {
let _ = axum::serve(listener, app).await;
});
$$$
}'Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's look at a larger section to see the full function
sed -n '100,170p' mcp/src/core/oauth.rs | cat -nRepository: lightseekorg/smg
Length of output: 3004
🏁 Script executed:
# Also search for where this function is defined to see the full signature
rg "fn.*auth.*\{" mcp/src/core/oauth.rs -A 30Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there's any cleanup or shutdown logic elsewhere
rg "shutdown|drop|cancel" mcp/src/core/oauth.rsRepository: lightseekorg/smg
Length of output: 172
🏁 Script executed:
# Search for all calls to start_callback_server to see if it's called multiple times
rg "start_callback_server" mcp/src/core/oauth.rsRepository: lightseekorg/smg
Length of output: 185
🏁 Script executed:
# Look at the callback handler to see if code_sender is consumed and what happens when callback is received
sed -n '70,100p' mcp/src/core/oauth.rs | cat -nRepository: lightseekorg/smg
Length of output: 1187
🏁 Script executed:
# Check if there are any tests that show multiple auth attempts
fd "test" --type f | xargs rg "start_callback_server|OAuth" | head -20Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Get the callback_handler implementation to see when code_sender is called
sed -n '69,130p' mcp/src/core/oauth.rs | cat -nRepository: lightseekorg/smg
Length of output: 2546
🏁 Script executed:
# Search for where OAuthHelper::authenticate is called to see if it's called multiple times
rg "authenticate\(" mcp/src/core --type rust -B 2 -A 2Repository: lightseekorg/smg
Length of output: 700
🏁 Script executed:
# Get the full callback handler implementation
sed -n '69,120p' mcp/src/core/oauth.rs | cat -nRepository: lightseekorg/smg
Length of output: 2074
🏁 Script executed:
# Check if OAuthHelper is reused or created fresh each time
rg "OAuthHelper::new|OAuthHelper {" mcp/src --type rust -B 2 -A 2Repository: lightseekorg/smg
Length of output: 183
🏁 Script executed:
# Get callback_handler to see the full implementation
rg "callback_handler" mcp/src/core/oauth.rs -A 15Repository: lightseekorg/smg
Length of output: 1145
🏁 Script executed:
# Check usage of OAuthHelper
rg "OAuthHelper::new" mcp/src --type rust -B 2 -A 5Repository: lightseekorg/smg
Length of output: 576
🏁 Script executed:
# See if authenticate is called in a loop or multiple times
rg "\.authenticate\(" mcp/src --type rust -B 5 -A 5Repository: lightseekorg/smg
Length of output: 694
🏁 Script executed:
# Verify the comment's claim about listener dropping - check the full function signature
head -70 mcp/src/core/oauth.rs | tail -40Repository: lightseekorg/smg
Length of output: 1093
Implement graceful shutdown of the OAuth callback server.
The spawned background server keeps the port bound indefinitely after the auth code is received, preventing subsequent authentication attempts from binding to the same port. The misleading comment suggests the listener drops when auth completes, but the listener is moved into the spawned task and never released until process exit.
Implement graceful shutdown using with_graceful_shutdown() to close the server once the authorization code is received:
🔧 Suggested graceful shutdown
- let (code_sender, code_receiver) = oneshot::channel::<String>();
+ let (code_sender, code_receiver) = oneshot::channel::<String>();
+ let (shutdown_tx, shutdown_rx) = oneshot::channel::<()>();
@@
- tokio::spawn(async move {
- let _ = axum::serve(listener, app).await;
- });
+ tokio::spawn(async move {
+ let _ = axum::serve(listener, app)
+ .with_graceful_shutdown(async move {
+ let _ = shutdown_rx.await;
+ })
+ .await;
+ });
@@
- code_receiver
- .await
- .map_err(|_| McpError::Auth("Failed to receive authorization code".to_string()))
+ let code = code_receiver
+ .await
+ .map_err(|_| McpError::Auth("Failed to receive authorization code".to_string()))?;
+ let _ = shutdown_tx.send(());
+ Ok(code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/src/core/oauth.rs` around lines 149 - 155, The background OAuth callback
server spawned in the tokio::spawn block currently moves the listener into the
task and never shuts down, so replace the fire-and-forget axum::serve(listener,
app).await with a graceful shutdown: create a oneshot shutdown signal
(sender/receiver) in the scope where the auth flow completes, move the receiver
into the spawned task and call
axum::Server::from_tcp(listener)?.with_graceful_shutdown(async {
receiver.await.ok(); }).serve(app.into_make_service()).await (i.e., use
with_graceful_shutdown), and send the shutdown signal (sender.send(())) right
after the authorization code is received so the listener is closed and the port
is released; update any error handling around tokio::spawn and
axum::serve(listener, app) accordingly.
Member
Author
There was a problem hiding this comment.
Valid observation about resource cleanup. Adding graceful shutdown logic is a behavioral change beyond the scope of this clippy-focused PR.
There was a problem hiding this comment.
@slin1237 Thanks—agreed to keep the graceful-shutdown change out of this clippy-focused PR.
Non-behavioral tweak (optional for this PR)
- To avoid a misleading note in the lint expectation, we can update the reason string without touching runtime behavior:
#[expect(
clippy::disallowed_methods,
reason = "fire-and-forget OAuth callback server; temporary: listener remains bound for the session; graceful shutdown will be added in a follow-up"
)]
tokio::spawn(async move {
let _ = axum::serve(listener, app).await;
});Would you like me to open a follow-up issue?
Proposed details:
- Title: OAuth callback server should shut down after code receipt (release port)
- Summary: start_callback_server spawns axum::serve and never stops; subsequent auth attempts can’t bind the same port.
- Acceptance criteria:
- Use with_graceful_shutdown driven by a oneshot triggered when the auth code is received.
- Ensure the port is released and a second authentication flow in the same process can rebind.
- Keep the clippy #[expect] with an updated reason that reflects the new behavior.
- I can file it and assign it to you.
If you prefer, I can just land the tiny reason-string tweak here and leave the issue creation for later.
(=^・^=)兔 Thoughtful: noted the scope preference and queued a clean follow-up path.
Comment on lines
+58
to
63
| #[expect( | ||
| clippy::result_unit_err, | ||
| reason = "Try-acquire pattern: callers only need success/failure, a custom error type adds no information" | ||
| )] | ||
| pub fn try_acquire(&self, tokens: f64) -> Result<(), ()> { | ||
| self.try_acquire_sync(tokens) |
There was a problem hiding this comment.
Validate token amounts to preserve bucket invariants.
Allowing negative/non‑finite or >capacity values can increase the bucket or lead to requests that can never be satisfied (e.g., refill_rate == 0 waits forever). Add validation at the public entry points (try_acquire/return_tokens/acquire) to reject invalid token values.
🛡️ Proposed guard at the API boundary
pub fn try_acquire(&self, tokens: f64) -> Result<(), ()> {
+ if !tokens.is_finite() || tokens < 0.0 || tokens > self.capacity {
+ return Err(());
+ }
self.try_acquire_sync(tokens)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[expect( | |
| clippy::result_unit_err, | |
| reason = "Try-acquire pattern: callers only need success/failure, a custom error type adds no information" | |
| )] | |
| pub fn try_acquire(&self, tokens: f64) -> Result<(), ()> { | |
| self.try_acquire_sync(tokens) | |
| #[expect( | |
| clippy::result_unit_err, | |
| reason = "Try-acquire pattern: callers only need success/failure, a custom error type adds no information" | |
| )] | |
| pub fn try_acquire(&self, tokens: f64) -> Result<(), ()> { | |
| if !tokens.is_finite() || tokens < 0.0 || tokens > self.capacity { | |
| return Err(()); | |
| } | |
| self.try_acquire_sync(tokens) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/core/token_bucket.rs` around lines 58 - 63, Validate token
inputs at the public API boundaries: in try_acquire (and its call to
try_acquire_sync), acquire, and return_tokens check that the provided tokens
value is finite (not NaN/inf), not negative, and does not exceed the bucket
capacity; for invalid values return a failure immediately (e.g., Err(()) for
try_acquire) instead of proceeding, and keep the existing behavior for valid
inputs by delegating to try_acquire_sync or the existing implementation after
the guard.
Member
Author
There was a problem hiding this comment.
Valid hardening suggestion. Adding new input validation logic is a behavioral change out of scope for this clippy lint PR.
There was a problem hiding this comment.
@slin1237 Thanks — agreed. Let’s keep this PR strictly to lint/mechanical changes.
If helpful, I can open a follow-up issue to track the hardening. Proposed scope:
- Validate inputs in TokenBucket:
- try_acquire: return Err(()) for non-finite, negative, or >capacity requests.
- return_tokens: ignore/reject non-finite or negative amounts; continue capping at capacity for large returns.
- Add brief rustdoc on valid token ranges and invariants.
- Add unit tests for the above.
Would you like me to file that issue and reference this PR?
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repo (e.g., PR `#489`), avoid introducing behavioral changes (like new input validation). Prefer a separate, follow-up issue/PR for such hardening.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+116
to
+137
| let mut bucket_guard = bucket.write(); | ||
| let worker_url = worker.url().to_string(); | ||
|
|
||
| let prefill_worker_urls_clone = { | ||
| let mut prefill_worker_urls = bucket_guard.prefill_worker_urls.lock().unwrap(); | ||
| if !prefill_worker_urls.contains(&worker_url) { | ||
| prefill_worker_urls.push(worker_url.clone()); | ||
| } | ||
| let cloned = prefill_worker_urls.clone(); | ||
| let prefill_worker_urls_clone = { | ||
| let mut prefill_worker_urls = bucket_guard.prefill_worker_urls.lock(); | ||
| if !prefill_worker_urls.contains(&worker_url) { | ||
| prefill_worker_urls.push(worker_url.clone()); | ||
| } | ||
| let cloned = prefill_worker_urls.clone(); | ||
|
|
||
| let mut chars_per_url = bucket_guard.chars_per_url.lock().unwrap(); | ||
| chars_per_url.entry(worker_url.clone()).or_insert(0); | ||
| let mut chars_per_url = bucket_guard.chars_per_url.lock(); | ||
| chars_per_url.entry(worker_url.clone()).or_insert(0); | ||
|
|
||
| cloned | ||
| }; | ||
| cloned | ||
| }; | ||
|
|
||
| bucket_guard.init_prefill_worker_urls(prefill_worker_urls_clone); | ||
| bucket_guard.init_prefill_worker_urls(prefill_worker_urls_clone); | ||
|
|
||
| info!( | ||
| "Added worker {} to bucket for model {}", | ||
| worker_url, model_key | ||
| ); | ||
| } else { | ||
| error!( | ||
| "Failed to acquire write lock for bucket of model {}", | ||
| model_key | ||
| ); | ||
| } | ||
| info!( | ||
| "Added worker {} to bucket for model {}", | ||
| worker_url, model_key | ||
| ); |
There was a problem hiding this comment.
Avoid reinitializing bucket state on duplicate add.
If add_prefill_url is called for a URL that’s already present, the current flow still reinitializes boundaries and zeroes per‑URL load state. That can wipe balancing history on duplicate registration/health events. Consider reinitializing only when a URL is newly inserted (and log accordingly).
🛠️ Suggested adjustment
- let prefill_worker_urls_clone = {
- let mut prefill_worker_urls = bucket_guard.prefill_worker_urls.lock();
- if !prefill_worker_urls.contains(&worker_url) {
- prefill_worker_urls.push(worker_url.clone());
- }
- let cloned = prefill_worker_urls.clone();
+ let (inserted, prefill_worker_urls_clone) = {
+ let mut prefill_worker_urls = bucket_guard.prefill_worker_urls.lock();
+ if prefill_worker_urls.contains(&worker_url) {
+ (false, prefill_worker_urls.clone())
+ } else {
+ prefill_worker_urls.push(worker_url.clone());
+ (true, prefill_worker_urls.clone())
+ }
+ };
-
- let mut chars_per_url = bucket_guard.chars_per_url.lock();
- chars_per_url.entry(worker_url.clone()).or_insert(0);
-
- cloned
- };
-
- bucket_guard.init_prefill_worker_urls(prefill_worker_urls_clone);
-
- info!(
- "Added worker {} to bucket for model {}",
- worker_url, model_key
- );
+ if inserted {
+ bucket_guard.init_prefill_worker_urls(prefill_worker_urls_clone);
+ info!(
+ "Added worker {} to bucket for model {}",
+ worker_url, model_key
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut bucket_guard = bucket.write(); | |
| let worker_url = worker.url().to_string(); | |
| let prefill_worker_urls_clone = { | |
| let mut prefill_worker_urls = bucket_guard.prefill_worker_urls.lock().unwrap(); | |
| if !prefill_worker_urls.contains(&worker_url) { | |
| prefill_worker_urls.push(worker_url.clone()); | |
| } | |
| let cloned = prefill_worker_urls.clone(); | |
| let prefill_worker_urls_clone = { | |
| let mut prefill_worker_urls = bucket_guard.prefill_worker_urls.lock(); | |
| if !prefill_worker_urls.contains(&worker_url) { | |
| prefill_worker_urls.push(worker_url.clone()); | |
| } | |
| let cloned = prefill_worker_urls.clone(); | |
| let mut chars_per_url = bucket_guard.chars_per_url.lock().unwrap(); | |
| chars_per_url.entry(worker_url.clone()).or_insert(0); | |
| let mut chars_per_url = bucket_guard.chars_per_url.lock(); | |
| chars_per_url.entry(worker_url.clone()).or_insert(0); | |
| cloned | |
| }; | |
| cloned | |
| }; | |
| bucket_guard.init_prefill_worker_urls(prefill_worker_urls_clone); | |
| bucket_guard.init_prefill_worker_urls(prefill_worker_urls_clone); | |
| info!( | |
| "Added worker {} to bucket for model {}", | |
| worker_url, model_key | |
| ); | |
| } else { | |
| error!( | |
| "Failed to acquire write lock for bucket of model {}", | |
| model_key | |
| ); | |
| } | |
| info!( | |
| "Added worker {} to bucket for model {}", | |
| worker_url, model_key | |
| ); | |
| let (inserted, prefill_worker_urls_clone) = { | |
| let mut prefill_worker_urls = bucket_guard.prefill_worker_urls.lock(); | |
| if prefill_worker_urls.contains(&worker_url) { | |
| (false, prefill_worker_urls.clone()) | |
| } else { | |
| prefill_worker_urls.push(worker_url.clone()); | |
| (true, prefill_worker_urls.clone()) | |
| } | |
| }; | |
| if inserted { | |
| let mut chars_per_url = bucket_guard.chars_per_url.lock(); | |
| chars_per_url.entry(worker_url.clone()).or_insert(0); | |
| bucket_guard.init_prefill_worker_urls(prefill_worker_urls_clone); | |
| info!( | |
| "Added worker {} to bucket for model {}", | |
| worker_url, model_key | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/policies/bucket.rs` around lines 116 - 137, The code always
reinitializes bucket state even when the worker_url already exists; change
add_prefill_url so you detect whether the push actually inserted a new URL (by
checking prefill_worker_urls.contains before mutation) and only when it's newly
inserted perform chars_per_url.entry(worker_url.clone()).or_insert(0) and call
bucket_guard.init_prefill_worker_urls(cloned); otherwise skip the init and skip
zeroing per-URL state to preserve balancing history, and adjust the info! log to
only report "Added worker ..." when insertion occurred (log a different message
or skip logging for duplicates).
Member
Author
There was a problem hiding this comment.
Valid optimization suggestion. Changing load-balancing reinitialization behavior is out of scope for this clippy-focused PR.
There was a problem hiding this comment.
@slin1237 Understood—let’s keep this PR strictly lint-focused.
Would you like me to open a follow-up GitHub issue to track the optimization? Here’s what I’d file:
- Title: BucketPolicy: avoid reinitializing boundaries/zeroing per-URL loads on duplicate add_prefill_url
- Scope:
- File: model_gateway/src/policies/bucket.rs
- Function: BucketPolicy::add_prefill_url
- Problem: On duplicate registration, we still call init_prefill_worker_urls(...), which clears chars_per_url and rebuilds boundaries, wiping recent balancing history.
- Proposed fix:
- Only reinitialize when the URL is newly inserted.
- On duplicates, skip reinit and keep existing per-URL load; optionally log at debug level.
- Acceptance criteria:
- Duplicate add does not change Bucket.boundary or reset chars_per_url for existing URLs.
- Unit test proving loads and boundaries remain stable after duplicate add.
- Backlinks: PR
#489and this discussion.
If you give the go-ahead, I’ll open the issue and assign it to you with those details.
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/policies/bucket.rs:116-137
Timestamp: 2026-02-21T02:30:56.454Z
Learning: For clippy-focused PRs in this repository, avoid behavior changes (e.g., load-balancing logic). Defer such changes to a separate PR/issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+338
to
+343
| 1 => { | ||
| let Some(model) = models.into_iter().next() else { | ||
| return Self::Wildcard; | ||
| }; | ||
| Self::Single(Box::new(model)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive but overly cautious handling for single-element case.
After matching models.len() == 1, the into_iter().next() call is guaranteed to return Some. The fallback to Self::Wildcard is logically unreachable. While this aligns with the PR's panic elimination objective, a more precise approach would use expect() with a clear message since this condition is invariant-guaranteed.
That said, the current code is safe and doesn't introduce bugs—Wildcard is a valid fallback that won't cause issues.
💡 Alternative: Use expect with invariant message
1 => {
- let Some(model) = models.into_iter().next() else {
- return Self::Wildcard;
- };
+ let model = models
+ .into_iter()
+ .next()
+ .expect("len==1 guarantees next() returns Some");
Self::Single(Box::new(model))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 1 => { | |
| let Some(model) = models.into_iter().next() else { | |
| return Self::Wildcard; | |
| }; | |
| Self::Single(Box::new(model)) | |
| } | |
| 1 => { | |
| let model = models | |
| .into_iter() | |
| .next() | |
| .expect("len==1 guarantees next() returns Some"); | |
| Self::Single(Box::new(model)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@protocols/src/worker.rs` around lines 338 - 343, The match arm handling the
single-element case is overly defensive: after checking models.len() == 1,
calling models.into_iter().next() cannot return None, yet the code falls back to
Self::Wildcard; replace the fallback with a direct unwrap/expect so the
invariant is explicit — i.e., in the branch that uses models.into_iter().next()
for construction of Self::Single(Box::new(...)), swap the conditional Some(...)
else { return Self::Wildcard } for models.into_iter().next().expect("expected
one model when models.len() == 1") and construct Self::Single from that value,
removing the unreachable Wildcard fallback.
Member
Author
There was a problem hiding this comment.
The defensive fallback to Self::Wildcard is intentional — consistent with this PR's philosophy of replacing unwrap()/expect() with non-panicking alternatives. Using expect() would reintroduce a panic path.
There was a problem hiding this comment.
@slin1237 Acknowledged. Leaving the non-panicking fallback to WorkerModels::Wildcard as-is in the single-element branch is consistent with this PR’s “avoid panics/expect” philosophy. No change needed. If helpful for future readers, we could add a brief comment documenting the “len() == 1 invariant; prefer non-panicking path” rationale, but I won’t push for it in this lint-focused PR.
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: protocols/src/worker.rs:338-343
Timestamp: 2026-02-21T02:36:00.013Z
Learning: Repo: lightseekorg/smg PR: 489
File: protocols/src/worker.rs (impl From<Vec<ModelCard>> for WorkerModels)
Learning: For the single-element case (models.len() == 1), keep the defensive Option guard with a non-panicking fallback to WorkerModels::Wildcard instead of using expect()/unwrap(). This is intentional to avoid introducing panic paths in production code for PR `#489` and similar clippy/lint-only efforts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs:189-194
Timestamp: 2026-02-21T02:32:04.647Z
Learning: Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs
Context: GrpcClient::generate fallback match arm
Learning: The catch-all panic for mismatched client/request types in GrpcClient::generate is intentional to enforce pipeline invariants; do not convert this to a returned error in PR `#489` or similar lint-only changes.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs:206-211
Timestamp: 2026-02-21T02:31:14.142Z
Learning: Repo: lightseekorg/smg
File: model_gateway/src/routers/grpc/client.rs
Context: GrpcClient::embed match arm
Learning: The catch-all panic in GrpcClient::embed for mismatched client/request types or unsupported embedding backends is intentional to catch invariant violations and unsupported configurations at development time. Converting this path to a returned error is out of scope for PR `#489`.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: kv_index/src/string_tree.rs:481-484
Timestamp: 2026-02-21T02:30:52.128Z
Learning: Preference noted: In kv_index/src/string_tree.rs (Rust radix tree), production code should avoid panics on invariant breaches; prefer a defensive non-panicking path (e.g., early return) and use debug_assert! for debug/test builds. Consider documenting the invariant with a comment, and optionally log under cfg(debug_assertions).
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repo (e.g., PR `#489`), avoid introducing behavioral changes (like new input validation). Prefer a separate, follow-up issue/PR for such hardening.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/policies/bucket.rs:116-137
Timestamp: 2026-02-21T02:30:56.454Z
Learning: For clippy-focused PRs in this repository, avoid behavior changes (e.g., load-balancing logic). Defer such changes to a separate PR/issue.
Introduce clippy.toml and workspace-level lint configuration to enforce
production-grade code quality standards across all 26 crates (320 files,
~4400 lines changed). Every warning is now denied via `-D warnings`.
What changed:
Cargo.toml: Add workspace-level [lints.clippy] configuration denying
unwrap_used, expect_used, panic, print_stdout/stderr, dbg_macro,
todo/unimplemented, and other unsafe patterns. Enable pedantic and
nursery lint groups with targeted exceptions.
clippy.toml: New file configuring disallowed-methods (tokio::spawn
requires justification), allow-expect-in-tests, and
allow-unwrap-in-tests for test/bench ergonomics.
model_gateway/src/policies/bucket.rs: Migrate from std::sync::Mutex
to parking_lot::Mutex. Removes ~50 lines of dead lock-poisoning error
handling since parking_lot never poisons. This is a behavioral
improvement: previously a panic while holding a lock would cause all
subsequent lock attempts to also panic; now the lock is released and
the next caller succeeds.
data_connector/src/memory.rs: Same std::sync::RwLock to parking_lot
migration for MemoryConversationItemStorage.
data_connector/src/postgres.rs: Replace three .expect() calls in
storage constructors with ? propagation. Previously, database schema
initialization failure would panic the process; now it returns a proper
error to the caller.
model_gateway/src/core/token_bucket.rs: Remove unnecessary async from
try_acquire(), return_tokens(), available_tokens(). These used
parking_lot::Mutex (sync) but were wrapped in async fn, creating
pointless Future overhead on every rate-limit check. The genuinely
async methods (acquire, acquire_timeout) that use tokio::Notify and
tokio::select! remain async.
model_gateway/src/core/worker.rs: Change Worker::api_key() trait
method from &Option<String> to Option<&String> (idiomatic Rust).
All callers updated (.clone() to .cloned(), removed .as_ref()).
Add Drop implementation for HealthChecker to abort the spawned
health-check task on drop, preventing task leaks on shutdown.
model_gateway/src/observability/gauge_histogram.rs: Fix TOCTOU race
condition in CachedGaugeHistogram::get_or_register() where a second
.get().expect() after or_insert_with() could panic if another thread
called remove() between the two operations. Restructured to use
.downgrade() on the entry ref directly.
mesh/src/crdt.rs, incremental.rs, ping_server.rs: Replace
SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default()
with .expect(). In CRDT code, timestamp=0 causes silent data loss
during LWW merge; a system clock before UNIX epoch is a fatal
misconfiguration that must crash, not silently corrupt.
grpc_client/src/trtllm_service.rs: Remove Result wrappers from 5
builder methods that never returned Err. Eliminates dead error paths
and unnecessary ? operators at call sites.
wasm/src/runtime.rs, module_manager.rs: Remove Result from infallible
constructors (WasmRuntime::new, WasmModuleManager::new). These never
failed; the Result wrapper was dead code.
model_gateway/src/routers/factory.rs and grpc router constructors:
Remove async from 19 functions that contained no .await (flagged by
unused_async lint). Includes create_grpc_router, prepare_chat,
prepare_responses, execute_tool_loop_streaming, and various streaming
handlers that use spawn-and-return patterns. All callers updated.
protocols/src/chat.rs, responses.rs: Replace unsafe .unwrap() on
optional tool arrays with if-let/else-return patterns.
kv_index/src/string_tree.rs: Replace .chars().next().unwrap() in
while loops with while-let patterns, eliminating theoretical panics.
All tokio::spawn call sites: Add #[expect(clippy::disallowed_methods)]
with reason strings documenting task lifecycle (who stores the handle,
how it's aborted, what coordinates shutdown).
All 320 files: Mechanical fixes for format string inlining (Rust 2021
format!("{x}") syntax), raw string hash reduction (r#"..."# to r"..."
where safe), #[allow] to #[expect] migration (expect fails if the
lint never fires, catching stale suppressions), clone_from() for
efficiency, wildcard matches to explicit enum variants for
exhaustiveness safety, and Copy type pass-by-value.
Why:
This codebase serves billions of tokens per minute. The previous lint
configuration was permissive, allowing unwrap/expect/panic in
production code paths without justification. This change enforces that
every potential panic point is either eliminated (proper error
propagation) or explicitly justified with a reason string explaining
why it's safe.
The audit also uncovered real bugs:
- CRDT timestamp corruption (unwrap_or_default producing timestamp=0)
- DashMap race condition in gauge histogram (TOCTOU between insert
and get)
- HealthChecker task leak (no Drop impl despite documented contract)
- Dead Result wrappers hiding the fact that errors were impossible
These are fixed in this commit, not just suppressed.
How verified:
cargo check --all-targets passes cleanly.
cargo clippy --all-targets -- -D warnings produces zero errors.
cargo fmt --all -- --check produces zero diffs.
Full 6-agent audit reviewed all 319 changed files across:
- async-to-sync conversions (19 verified, all callers updated)
- API signature changes (all callers updated, zero missed)
- Error handling quality (3 HIGH issues fixed, 7 MEDIUM reviewed)
- Dead code annotations (all justified or flagged for future removal)
- Test code changes (all mechanical, no broken assertions)
- Non-gateway crate changes (all verified correct)
Signed-off-by: Simon Lin <simon@simon.dev>
Signed-off-by: Simo Lin <simo.lin@oracle.com>
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
model_gateway/src/routers/parse/handlers.rs (1)
57-63: 🧹 Nitpick | 🔵 TrivialFormat string inlining is incomplete in this file.
The changes on lines 61 and 98 correctly inline the format arguments, but adjacent lines using the same variables were not updated:
- Line 58:
error!("Failed to parse function calls: {}", e)→ should use{e}- Line 95:
error!("Failed to separate reasoning: {}", e)→ should use{e}Additionally, lines 42 and 79 also use positional format arguments that could be inlined for consistency with the PR's lint enforcement goal.
♻️ Proposed fix for consistency
@@ -39,7 +39,7 @@ let Some(pooled_parser) = factory.registry().get_pooled_parser(&req.tool_call_parser) else { return error_response( StatusCode::BAD_REQUEST, - &format!("Unknown tool parser: {}", req.tool_call_parser), + &format!("Unknown tool parser: {}", req.tool_call_parser), // req.tool_call_parser is a field, cannot inline ); }; @@ -55,7 +55,7 @@ ) .into_response(), Err(e) => { - error!("Failed to parse function calls: {}", e); + error!("Failed to parse function calls: {e}"); error_response( StatusCode::BAD_REQUEST, &format!("Failed to parse function calls: {e}"), @@ -76,7 +76,7 @@ let Some(pooled_parser) = factory.registry().get_pooled_parser(&req.reasoning_parser) else { return error_response( StatusCode::BAD_REQUEST, - &format!("Unknown reasoning parser: {}", req.reasoning_parser), + &format!("Unknown reasoning parser: {}", req.reasoning_parser), // req.reasoning_parser is a field, cannot inline ); }; @@ -92,7 +92,7 @@ ) .into_response(), Err(e) => { - error!("Failed to separate reasoning: {}", e); + error!("Failed to separate reasoning: {e}"); error_response( StatusCode::BAD_REQUEST, &format!("Failed to separate reasoning: {e}"),Note: Lines 42 and 79 cannot use inline interpolation because
req.tool_call_parserandreq.reasoning_parserare field accesses, which Rust format strings don't support for inlining. Only theerror!macros on lines 58 and 95 should be updated.Also applies to: 94-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/parse/handlers.rs` around lines 57 - 63, Update the error! macro format strings to use named interpolation for the error variable instead of positional placeholders: replace occurrences like error!("Failed to parse function calls: {}", e) and error!("Failed to separate reasoning: {}", e) with error!("Failed to parse function calls: {e}") and error!("Failed to separate reasoning: {e}") respectively in the parse handler in handlers.rs; leave the logs that reference struct field accesses (e.g., req.tool_call_parser and req.reasoning_parser) as-is since Rust's format inlining does not support field access in the same way.model_gateway/src/routers/grpc/proto_wrapper.rs (1)
483-528: 🧹 Nitpick | 🔵 TrivialLGTM!
Consistent
#[expect]application forProtoGenerateCompleteaccessors with correct reason strings.As a future enhancement, you might consider adding
try_as_*() -> Option<&T>methods alongside these panicking accessors. This would give callers a choice between:
is_sglang() + as_sglang()(current pattern)try_as_sglang()with directOptionhandlingThis is entirely optional and the current design with documented panics is valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/proto_wrapper.rs` around lines 483 - 528, Add non-panicking "try" accessors alongside the existing panicking ones: implement try_as_sglang(&self) -> Option<&sglang::GenerateComplete>, try_as_sglang_mut(&mut self) -> Option<&mut sglang::GenerateComplete>, try_as_vllm(&self) -> Option<&vllm::GenerateComplete>, and try_as_trtllm(&self) -> Option<&trtllm::GenerateComplete> in the same impl that contains as_sglang/as_sglang_mut/as_vllm/as_trtllm; each should match self and return Some(complete) for the corresponding variant (Self::Sglang, Self::Vllm, Self::Trtllm) or None for other variants, so callers can choose Option-based handling instead of relying on is_* + panicking accessors.multimodal/src/vision/image_processor.rs (1)
79-84:⚠️ Potential issue | 🟡 MinorMissing validation: data length should match declared shape.
uint_2ddoes not verify thatdata.len() == rows * cols. If a caller passes mismatched values, downstream code trusting the shape could panic or produce incorrect results.🛡️ Proposed fix to add validation
/// Create a 2D uint tensor. -pub fn uint_2d(data: Vec<u32>, rows: usize, cols: usize) -> Self { +pub fn uint_2d(data: Vec<u32>, rows: usize, cols: usize) -> Result<Self, TransformError> { + let expected_len = rows * cols; + if data.len() != expected_len { + return Err(TransformError::InvalidShape { + expected: format!("{rows}x{cols} = {expected_len} elements"), + actual: vec![data.len()], + }); + } - Self::UintTensor { + Ok(Self::UintTensor { data, shape: vec![rows, cols], - } + }) }Alternatively, if you want to keep the infallible signature, add a debug assertion:
pub fn uint_2d(data: Vec<u32>, rows: usize, cols: usize) -> Self { + debug_assert_eq!( + data.len(), + rows * cols, + "data length {} does not match shape {}x{}", + data.len(), + rows, + cols + ); Self::UintTensor { data, shape: vec![rows, cols], } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@multimodal/src/vision/image_processor.rs` around lines 79 - 84, The uint_2d constructor (uint_2d) currently constructs Self::UintTensor without validating that data.len() matches rows * cols; add a check that data.len() == rows.checked_mul(cols).unwrap_or(usize::MAX) (or equivalent safe multiply) and handle mismatches: either change uint_2d to return a Result<Self, Error> and return Err when lengths differ, or keep the infallible API but add a debug_assert!(data.len() == rows * cols, "mismatched data length") and document the precondition; update any callers/tests accordingly.model_gateway/benches/tool_parser_benchmark.rs (1)
698-748: 🧹 Nitpick | 🔵 TrivialVerify the returned duration accuracy for criterion.
The inverted control flow makes the first iteration collect detailed latency statistics (warmup + 1000 samples) while returning
p50 * iters as u32as the estimated duration. Subsequent iterations perform regular benchmarking.The concern: when
itersdiffers significantly from the 1000 samples collected, or when the actual time spent (100 warmup + 1000 measurements) doesn't matchp50 * iters, criterion may report inconsistent timing data for this benchmark group compared to others.Since the primary goal is the custom latency statistics printed to stdout (P50/P95/P99/Max), this may be acceptable. However, if criterion's built-in statistics matter for CI/comparison purposes, consider aligning the returned duration with actual time spent or documenting this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/benches/tool_parser_benchmark.rs` around lines 698 - 748, The current branch that computes latency stats returns p50 * iters as u32 which can misreport the benchmark duration; instead compute and return a Duration that reflects the measured work: after warmup and the 1000-sample loop, sum or average the collected latencies (latencies.iter().sum() or average p50) and scale that to the requested iters to produce a Duration (or compute the actual elapsed time across the warmup+measurement loops with Instant and return that); update the total_duration assignment (the variable total_duration and the branch using printed_clone) to use that Duration so Criterion sees a consistent timing, and ensure types convert correctly between Duration and the iters scaling when replacing p50 * iters as u32.model_gateway/src/policies/bucket.rs (1)
33-39:⚠️ Potential issue | 🟠 MajorDrop impl does not actually stop the background thread.
The
drop(handle)call merely drops theJoinHandle, but the spawned thread (line 54) runs an infinite loop and continues executing after theBucketPolicyis dropped. This results in a thread leak where the background adjustment thread keeps running with a danglingArc<DashMap>reference.Consider using an
AtomicBoolor a channel to signal the thread to exit, then optionally join it:🛠️ Suggested fix pattern
pub struct BucketPolicy { config: BucketConfig, buckets: Arc<DashMap<String, Arc<RwLock<Bucket>>>>, + shutdown: Arc<AtomicBool>, adjustment_handle: Option<thread::JoinHandle<()>>, } impl Drop for BucketPolicy { fn drop(&mut self) { + self.shutdown.store(true, Ordering::Relaxed); if let Some(handle) = self.adjustment_handle.take() { - drop(handle); + let _ = handle.join(); } } }Then in the spawned thread loop, check
shutdown.load(Ordering::Relaxed)to break out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/policies/bucket.rs` around lines 33 - 39, The Drop implementation for BucketPolicy only drops the JoinHandle (adjustment_handle) but doesn't signal the spawned adjustment thread to exit, causing a leak; modify BucketPolicy to include a shutdown signal (e.g., an Arc<AtomicBool> named shutdown or a channel receiver) that the spawned thread checks in its loop (check shutdown.load(Ordering::Relaxed) or recv timeout) and breaks out when signaled, set that signal in BucketPolicy::drop prior to taking/joining adjustment_handle, and then join the handle (call join on the taken JoinHandle) to ensure the background thread terminates cleanly.model_gateway/src/core/job_queue.rs (1)
785-791:⚠️ Potential issue | 🟡 MinorPotential integer underflow with
unwrap_or_default().If
duration_since(UNIX_EPOCH)fails (system clock before 1970),nowbecomes 0. The subtractionnow - value.timestampon line 791 would underflow whenvalue.timestamp > 0, causing a panic in debug mode or wraparound in release mode (breaking cleanup by retaining all statuses).While this edge case is extremely rare, consider using
saturating_subfor consistent defensive handling:Suggested fix
- status_map.retain(|_key, value| now - value.timestamp < STATUS_TTL); + status_map.retain(|_key, value| now.saturating_sub(value.timestamp) < STATUS_TTL);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/core/job_queue.rs` around lines 785 - 791, The computation of now from SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_secs() can yield 0 on error and cause underflow when doing now - value.timestamp inside status_map.retain; update the retain predicate to use a saturating subtraction (e.g., now.saturating_sub(value.timestamp) < STATUS_TTL) so the subtraction cannot underflow, referencing the now variable, status_map.retain call, value.timestamp field and STATUS_TTL constant.model_gateway/src/core/worker_manager.rs (1)
369-381:⚠️ Potential issue | 🟡 MinorUpdate policies/watchers even when loads are empty.
On Line 369, skipping updates when
loadsis empty can leave stale load data inPowerOfTwopolicies and subscribers if all workers are removed or temporarily unavailable. Consider sending the empty map (clears state) while still warning.💡 Suggested change
- if loads.is_empty() { - warn!("No loads fetched from workers"); - } else { - debug!( - "Fetched loads from {} workers, updating {} PowerOfTwo policies", - loads.len(), - power_of_two_policies.len() - ); - for policy in &power_of_two_policies { - policy.update_loads(&loads); - } - let _ = tx.send(loads); - } + if loads.is_empty() { + warn!("No loads fetched from workers"); + } else { + debug!( + "Fetched loads from {} workers, updating {} PowerOfTwo policies", + loads.len(), + power_of_two_policies.len() + ); + } + for policy in &power_of_two_policies { + policy.update_loads(&loads); + } + let _ = tx.send(loads);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/core/worker_manager.rs` around lines 369 - 381, The current branch skips updating when loads.is_empty(), which leaves stale state in PowerOfTwo policies and subscribers; change the logic to always iterate over power_of_two_policies and call policy.update_loads(&loads) even when loads.is_empty(), and still send the (empty) loads via let _ = tx.send(loads) so watchers are notified/cleared; keep the warn!("No loads fetched from workers") but do not return early—always perform the update_loads loop and tx.send call using the existing variables (loads, power_of_two_policies, policy.update_loads, tx.send).protocols/src/interactions.rs (1)
1076-1109: 🧹 Nitpick | 🔵 TrivialAdd/confirm tests + doc for the new agent/background constraints.
These rules are critical; please add or confirm unit coverage for (agent ⇒ background=true, background ⇒ agent, background ∧ stream conflict) and note the requirement in docs/comments to prevent regressions. Based on learnings: Enforce background=true for all Gemini Interactions API agent calls in Rust code under protocols/src. If an agent is used, ensure the request includes background: true; add validation tests and document the requirement for future contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocols/src/interactions.rs` around lines 1076 - 1109, Update validate_interactions_request to enforce and document the agent/background constraints and add unit tests: ensure that validate_interactions_request (operating on InteractionsRequest) requires background == true whenever agent is set, rejects requests that set background == true without an agent, and rejects requests where background and stream are both true (these checks already exist but confirm behavior for Gemini agent by adding a comment above validate_interactions_request documenting "Agent interactions must set background = true (Gemini requirement)"); add unit tests exercising (1) agent present with background=false fails, (2) background=true without agent fails, and (3) background=true and stream=true fails, using the existing ValidationError variants ("agent_requires_background", "background_requires_agent", "background_conflicts_with_stream") to assert correct errors so future changes are caught.reasoning_parser/src/factory.rs (3)
600-606:⚠️ Potential issue | 🟡 MinorThroughput assertion may cause CI flakiness.
The assertion
throughput > 1000.0could fail on slower CI runners, resource-constrained containers, or when the system is under load from parallel jobs. Consider lowering the threshold or making it configurable via environment variable.💡 Optional: Make threshold configurable or lower it
// Performance check: should handle at least 1000 req/sec let throughput = (total_requests as f64) / duration.as_secs_f64(); + let min_throughput = std::env::var("MIN_PARSER_THROUGHPUT") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(500.0); // Conservative default for CI assert!( - throughput > 1000.0, + throughput > min_throughput, "Throughput too low: {throughput:.0} req/sec", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reasoning_parser/src/factory.rs` around lines 600 - 606, The hardcoded throughput assertion using the computed variable throughput (from total_requests and duration) can cause CI flakiness; replace the literal 1000.0 with a configurable threshold read from an environment variable (e.g. THROUGHPUT_THRESHOLD) parsed into an f64 with a safe default (or lower default if you prefer), validate/handle parse errors by falling back to the default, and then assert throughput > threshold using the parsed value so tests can run reliably on slower CI runners; update the assertion message to include the actual threshold variable for clarity.
63-85:⚠️ Potential issue | 🟠 MajorTOCTOU race in parser pooling logic.
The check-then-act pattern between reading the pool (line 66) and inserting (lines 78-79) allows a race where two threads both find the parser missing, both create instances, and the second overwrites the first. This leaves the first caller holding an
Arcto an orphaned parser that differs from what's in the pool, violating pooling semantics.The PR objectives mention fixing a similar "DashMap TOCTOU race ... by restructuring entry handling" elsewhere—the same fix should apply here.
🔧 Proposed fix using double-checked locking
pub fn get_pooled_parser(&self, name: &str) -> Option<PooledParser> { // First check if we have a pooled instance { let pool = self.pool.read(); if let Some(parser) = pool.get(name) { return Some(Arc::clone(parser)); } } // If not in pool, create one and add to pool - let creators = self.creators.read(); - if let Some(creator) = creators.get(name) { - let parser = Arc::new(Mutex::new(creator())); - - // Add to pool for future use - let mut pool = self.pool.write(); - pool.insert(name.to_string(), Arc::clone(&parser)); - - Some(parser) - } else { - None - } + // Clone the creator while holding read lock, then release before creating parser + let creator = { + let creators = self.creators.read(); + creators.get(name).map(Arc::clone) + }?; + + // Acquire write lock and double-check before inserting + let mut pool = self.pool.write(); + if let Some(existing) = pool.get(name) { + return Some(Arc::clone(existing)); + } + + let parser = Arc::new(Mutex::new(creator())); + pool.insert(name.to_string(), Arc::clone(&parser)); + Some(parser) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reasoning_parser/src/factory.rs` around lines 63 - 85, The get_pooled_parser function has a TOCTOU race: two threads can both miss the pool read and create distinct parser instances, with one overwriting the other. Fix by using double-checked locking: first read self.pool to see if the parser exists (keep that early fast path), then if missing read self.creators to get the creator, then acquire a write lock on self.pool and check again if the entry exists; if it exists return that Arc, otherwise call the creator to build the parser, insert it into the pool (pool.insert(name.to_string(), Arc::clone(&parser))) and return the newly created Arc. Ensure you reference get_pooled_parser, self.pool, self.creators, and the creator() call when making the change.
608-643: 🧹 Nitpick | 🔵 TrivialNote: This test may not reliably catch the TOCTOU race.
This test exercises concurrent pool access and clearing, which is the scenario affected by the race condition in
get_pooled_parser. However, the test only verifies no deadlock/panic occurs—it doesn't assert that pooled parsers maintain referential integrity. Consider adding an assertion that validates Arc pointer equality after concurrent operations if the race fix is applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@reasoning_parser/src/factory.rs` around lines 608 - 643, The test currently only checks for absence of panic but not that pooled parsers preserve referential integrity; update the test_concurrent_pool_modifications to capture a baseline Arc from ParserFactory::get_pooled (use ParserFactory::get_pooled("deepseek-r1") or other model), run the concurrent tasks, then retrieve the parser again and assert Arc::ptr_eq(&baseline_arc, &new_arc) (or use pointer equality checks for multiple model keys) to ensure the TOCTOU race is fixed; if needed, use a synchronization primitive (Barrier) to ensure the baseline is taken before concurrent clears begin and the final fetch happens after operations complete.model_gateway/src/policies/consistent_hashing.rs (1)
163-177:⚠️ Potential issue | 🟠 MajorRandom fallback can return
Noneeven when healthy workers remain.
Line 169-175 counts healthy workers, then re-iterates; if health changes between passes,nth(random_healthy_idx)can returnNone, producing(None, Branch::RandomFallback)and dropping the request. Build the healthy index list once to keep the selection consistent.🔧 Suggested fix (single pass, consistent selection)
- let healthy_count = workers.iter().filter(|w| w.is_healthy()).count(); - if healthy_count == 0 { + let healthy_indices: Vec<usize> = workers + .iter() + .enumerate() + .filter(|(_, w)| w.is_healthy()) + .map(|(i, _)| i) + .collect(); + if healthy_indices.is_empty() { return (None, Branch::NoHealthyWorkers); } - let random_healthy_idx = rand::rng().random_range(0..healthy_count); - let idx = workers - .iter() - .enumerate() - .filter(|(_, w)| w.is_healthy()) - .nth(random_healthy_idx) - .map(|(i, _)| i); - - (idx, Branch::RandomFallback) + let random_healthy_idx = rand::rng().random_range(0..healthy_indices.len()); + let idx = healthy_indices[random_healthy_idx]; + (Some(idx), Branch::RandomFallback)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/policies/consistent_hashing.rs` around lines 163 - 177, The fallback selection currently counts healthy workers then re-iterates workers to pick the Nth healthy, which can return None if health changes between iterations; instead collect the healthy worker indices in a single pass (e.g., build a Vec<usize> of indices from workers.iter().enumerate().filter(|(_, w)| w.is_healthy()).map(|(i,_)| i).collect()), check if that Vec is empty, then pick a random index from that Vec (use rand rng to choose within 0..vec.len()) and return Some(chosen_index) with Branch::RandomFallback; update the code references around healthy_count, random_healthy_idx, and the .nth(...) logic accordingly so selection is consistent.data_connector/src/config.rs (1)
135-149:⚠️ Potential issue | 🟡 MinorUpdate the retention_days comment to reflect the serde default behavior.
The current comment "If None, data persists indefinitely" is now misleading. With
#[serde(default = "default_redis_retention_days")], configs that omitretention_dayswill deserialize toSome(30), notNone. The 30-day TTL will be applied in redis expiration calls (lines 117, 311, 618 in redis.rs).Update the comment to clarify:
- // Data retention in days. If None, data persists indefinitely. + // Data retention in days. Defaults to 30 days if omitted; if explicitly set to null, data persists indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/config.rs` around lines 135 - 149, The comment for the struct field retention_days is incorrect given the serde default; update the comment on the retention_days field to state that when omitted it defaults to Some(30) via the #[serde(default = "default_redis_retention_days")] function and that a 30-day TTL will be applied in Redis expiration calls (see default_redis_retention_days and usages in redis.rs where expiration is applied). Keep the comment concise and mention that explicitly setting None still means data persists indefinitely, while omitting the field results in a 30-day retention.model_gateway/benches/wasm_middleware_latency.rs (1)
42-55:⚠️ Potential issue | 🟡 MinorAvoid accumulating sleeping tasks between iterations.
The spawned task sleeps for 500ms even after the receiver is dropped. With the benchmark's 10 iterations, up to 10 sleeping tasks can accumulate concurrently, distorting latency measurements. Exit early when the receiver is dropped using
tx.closed().Suggested fix
tokio::spawn(async move { // Send first chunk immediately let _ = tx .send(Ok::<_, std::io::Error>(bytes::Bytes::from("chunk 1 "))) .await; - // Simulate generation delay - tokio::time::sleep(tokio::time::Duration::from_millis(500)).await; + // Simulate generation delay, but abort if the receiver is gone + tokio::select! { + _ = tokio::time::sleep(tokio::time::Duration::from_millis(500)) => {} + _ = tx.closed() => return, + } // Send final chunk let _ = tx .send(Ok::<_, std::io::Error>(bytes::Bytes::from("chunk 2"))) .await; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/benches/wasm_middleware_latency.rs` around lines 42 - 55, The spawned task in mock_next_streaming currently sleeps for 500ms even if the receiver is dropped, causing accumulating sleeping tasks across iterations; modify the task to detect receiver closure (use tx.closed().await or check tx.is_closed()/tx.closed() before/after the sleep and before sending the final chunk) and return early if the receiver is dropped so the task doesn't continue sleeping and skew benchmark latency. Ensure checks are placed after the first send and before the sleep/send of the final chunk to bail out promptly when the receiver is gone.model_gateway/tests/common/mock_worker.rs (1)
1235-1259:⚠️ Potential issue | 🟡 MinorClear the per-port RESP_STORE entry on worker shutdown to avoid cross-test leakage.
RESP_STOREpersists for the process lifetime; if ports/IDs are reused, GET/CANCEL could succeed unexpectedly. Consider clearing per-port entries when a worker stops.🧹 Suggested cleanup hook
+fn clear_responses_for_port(port: u16) { + let mut map = get_store().lock().unwrap(); + map.remove(&port); +} + pub async fn stop(&mut self) { if let Some(shutdown_tx) = self.shutdown_tx.take() { let _ = shutdown_tx.send(()); } if let Some(handle) = self.shutdown_handle.take() { // Wait for the server to shut down let _ = tokio::time::timeout(tokio::time::Duration::from_secs(5), handle).await; } + + let port = self.config.read().await.port; + clear_responses_for_port(port); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/tests/common/mock_worker.rs` around lines 1235 - 1259, RESP_STORE currently retains responses for the process lifetime causing cross-test leakage; add a cleanup function (e.g., remove_responses_for_port or clear_responses_for_port) that locks get_store(), removes the HashMap entry for the given port (map.remove(&port)), and call this cleanup from the worker shutdown/stop path so per-port entries are cleared when a worker stops; reference RESP_STORE, get_store(), store_response_for_port, and response_exists_for_port to locate where to add and invoke the cleanup.auth/src/jwt.rs (1)
397-452:⚠️ Potential issue | 🟠 MajorAvoid silently defaulting to
Role::Userwhen role mappings are configured.With
extract_rolenow returningRoledirectly, unmapped roles no longer fail validation and will be treated asUser. Ifrole_mappingis meant to be strict, this can weaken authorization. Consider restoring an error path (or making the fallback configurable) so missing mappings don’t implicitly grant access.🔧 Suggested fix (restore strict mapping semantics)
- fn extract_role(&self, claims: &StandardClaims) -> Role { + fn extract_role(&self, claims: &StandardClaims) -> Result<Role, JwtValidatorError> { // Try to get the role claim value let role_value = claims.extra.get(&self.config.role_claim); @@ - if let Ok(role) = role_str.parse::<Role>() { - return role; - } + if let Ok(role) = role_str.parse::<Role>() { + return Ok(role); + } } // Default to User if no explicit role found warn!("No role found in JWT claims, defaulting to User"); - return Role::User; + return Ok(Role::User); } @@ - if let Some(role) = self.config.role_mapping.get(role_str) { - return *role; - } + if let Some(role) = self.config.role_mapping.get(role_str) { + return Ok(*role); + } } @@ - Role::User + Err(JwtValidatorError::NoRoleMapping(role_strings.join(","))) }🔧 Update call site
- let role = self.extract_role(&claims); + let role = self.extract_role(&claims)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth/src/jwt.rs` around lines 397 - 452, The extract_role function currently defaults to Role::User even when self.config.role_mapping is non-empty, which silently grants access for unmapped roles; change extract_role to return Result<Role, JwtError> (or Option<Role>) instead of Role, return Err(...) when role_mapping is configured but no mapping or direct parse matches (replace the final warn+Role::User with an error), and update all callers of extract_role to propagate or handle the error so missing mappings cause authentication/authorization failure rather than implicit User access; keep the direct-parse/default behavior only when role_mapping.is_empty() if you want to preserve backward compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mesh/src/crdt.rs`:
- Around line 296-299: Add a short documentation comment above the contains_key
method explaining its purpose and why it exists despite being unused internally
(e.g., provided for API completeness or future external use), so the
#[expect(dead_code)] attribute isn’t the only signal; update the doc to
reference the method name contains_key and mention that it checks membership via
self.inner.read().contains_key(key) for clarity to future readers.
In `@mesh/src/sync.rs`:
- Line 83: The key formatting is inconsistent: several places construct the
policy key with named interpolation using format!("policy:{model_id}") (e.g.,
SKey::new calls that use that pattern), but apply_remote_policy_state still uses
positional interpolation format!("policy:{}", state.model_id); update the call
inside apply_remote_policy_state to use the same named-interpolation style
(format!("policy:{model_id}") referencing state.model_id) so all
SKey::new(policy...) usages are consistent with the rest of the file.
In `@model_gateway/benches/wasm_middleware_latency.rs`:
- Line 1: The file-level #[expect(clippy::unwrap_used,
clippy::disallowed_methods)] is too broad—narrow the scope by removing the
module-level attribute and applying #[expect(...)] only to the specific
functions that call unwrap or spawn (e.g., the benchmark functions, setup
helpers, or a main/run_benchmark function that actually use unwrap/spawn);
update each annotated function with the scoped attribute and include a short
reason if your lint config requires it.
- Around line 88-91: The closure passed into tower::service_fn currently uses an
unnecessary async block to call the now-sync mock_next_streaming; replace the
async closure with a ready future by returning future::ready(Ok::<_,
std::convert::Infallible>(mock_next_streaming(req))) so the
middleware::from_fn_with_state(app_state.clone(), wasm_middleware).layer(...)
uses a synchronous-ready service instead of creating an extra task; update the
service_fn closure accordingly and ensure future::ready is imported or fully
qualified.
In `@model_gateway/src/core/steps/worker/local/update_worker_properties.rs`:
- Around line 89-93: The code silently skips DP configuration when
worker.is_dp_aware() is true but worker.dp_rank() or worker.dp_size() is None;
add an observable warning there: inside the branch that checks is_dp_aware() but
before/after the existing if let (Some(rank), Some(size)) = (worker.dp_rank(),
worker.dp_size()) { builder = builder.dp_config(rank, size); }, insert a warning
log (using the local logger/context) that includes the worker identifier and
which DP metadata is missing when either dp_rank() or dp_size() is None so the
inconsistent DP-aware state is visible for debugging.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 671-673: The code builds an error JSON by interpolating the error
string (error_chunk = format!("data: {{\"error\": \"{e}\"}}\n\n")), which can
produce invalid JSON when the error contains quotes/newlines; change this to
serialize the error via serde_json (e.g., let payload = serde_json::json!({
"error": e.to_string() }); let json = serde_json::to_string(&payload)? or handle
the to_string error with a fallback) and then build the chunk as format!("data:
{}\n\n", json); apply the same change to the other occurrence (the block using
tx.send(Ok(Bytes::from(error_chunk))) at the second location) so tx.send always
gets a properly escaped JSON string wrapped with "data: ...\n\n".
In `@multimodal/src/vision/image_processor.rs`:
- Around line 411-413: Add a test that exercises invalid tensor dimensionality
for PreprocessedImages by creating a mismatched-rank ArrayD (e.g., 3D or 6D) and
asserting that PreprocessedImages::new_dynamic yields an instance where calling
channels(), height(), and width() returns Err(TransformError::InvalidShape);
locate the APIs by referencing PreprocessedImages::new_dynamic and the methods
channels(), height(), width() and assert the error variant
TransformError::InvalidShape for each call.
In `@multimodal/src/vision/processors/phi4_vision.rs`:
- Around line 486-490: Replace the defensive ok_or(TransformError::EmptyBatch)?
on the max_crops computation with an expect that documents the proven invariant
(e.g., change the call on all_outputs.iter().map(|o| o.shape()[0]).max() to
.expect("images non-empty; all_outputs populated from images") so the code
reflects the checked precondition), updating the variable max_crops accordingly
and leaving TransformError usage untouched elsewhere.
In `@multimodal/tests/vision_golden_tests.rs`:
- Around line 576-578: Replace the .unwrap() call on
processor.reshape_to_patches(&tensor_3d, grid_t, grid_h, grid_w) with an
.expect(...) to provide a clearer diagnostic; update the call that produces
rust_patches (currently: let rust_patches =
processor.reshape_to_patches(&tensor_3d, grid_t, grid_h, grid_w).unwrap();) to
use .expect("reshape_to_patches failed") so failures in reshape_to_patches are
reported consistently and with a helpful message.
- Around line 402-404: Replace the call that uses .unwrap() on
processor.reshape_to_patches(&tensor_3d, grid_t, grid_h, grid_w) with
.expect(...) to provide actionable failure context; specifically update the
expression that assigns rust_patches (the result of
processor.reshape_to_patches) to use .expect("reshape_to_patches failed for
qwen2_vl") so test failures identify the reshape operation and model.
---
Outside diff comments:
In `@auth/src/jwt.rs`:
- Around line 397-452: The extract_role function currently defaults to
Role::User even when self.config.role_mapping is non-empty, which silently
grants access for unmapped roles; change extract_role to return Result<Role,
JwtError> (or Option<Role>) instead of Role, return Err(...) when role_mapping
is configured but no mapping or direct parse matches (replace the final
warn+Role::User with an error), and update all callers of extract_role to
propagate or handle the error so missing mappings cause
authentication/authorization failure rather than implicit User access; keep the
direct-parse/default behavior only when role_mapping.is_empty() if you want to
preserve backward compatibility.
In `@data_connector/src/config.rs`:
- Around line 135-149: The comment for the struct field retention_days is
incorrect given the serde default; update the comment on the retention_days
field to state that when omitted it defaults to Some(30) via the #[serde(default
= "default_redis_retention_days")] function and that a 30-day TTL will be
applied in Redis expiration calls (see default_redis_retention_days and usages
in redis.rs where expiration is applied). Keep the comment concise and mention
that explicitly setting None still means data persists indefinitely, while
omitting the field results in a 30-day retention.
In `@model_gateway/benches/tool_parser_benchmark.rs`:
- Around line 698-748: The current branch that computes latency stats returns
p50 * iters as u32 which can misreport the benchmark duration; instead compute
and return a Duration that reflects the measured work: after warmup and the
1000-sample loop, sum or average the collected latencies (latencies.iter().sum()
or average p50) and scale that to the requested iters to produce a Duration (or
compute the actual elapsed time across the warmup+measurement loops with Instant
and return that); update the total_duration assignment (the variable
total_duration and the branch using printed_clone) to use that Duration so
Criterion sees a consistent timing, and ensure types convert correctly between
Duration and the iters scaling when replacing p50 * iters as u32.
In `@model_gateway/benches/wasm_middleware_latency.rs`:
- Around line 42-55: The spawned task in mock_next_streaming currently sleeps
for 500ms even if the receiver is dropped, causing accumulating sleeping tasks
across iterations; modify the task to detect receiver closure (use
tx.closed().await or check tx.is_closed()/tx.closed() before/after the sleep and
before sending the final chunk) and return early if the receiver is dropped so
the task doesn't continue sleeping and skew benchmark latency. Ensure checks are
placed after the first send and before the sleep/send of the final chunk to bail
out promptly when the receiver is gone.
In `@model_gateway/src/core/job_queue.rs`:
- Around line 785-791: The computation of now from
SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_secs() can
yield 0 on error and cause underflow when doing now - value.timestamp inside
status_map.retain; update the retain predicate to use a saturating subtraction
(e.g., now.saturating_sub(value.timestamp) < STATUS_TTL) so the subtraction
cannot underflow, referencing the now variable, status_map.retain call,
value.timestamp field and STATUS_TTL constant.
In `@model_gateway/src/core/worker_manager.rs`:
- Around line 369-381: The current branch skips updating when loads.is_empty(),
which leaves stale state in PowerOfTwo policies and subscribers; change the
logic to always iterate over power_of_two_policies and call
policy.update_loads(&loads) even when loads.is_empty(), and still send the
(empty) loads via let _ = tx.send(loads) so watchers are notified/cleared; keep
the warn!("No loads fetched from workers") but do not return early—always
perform the update_loads loop and tx.send call using the existing variables
(loads, power_of_two_policies, policy.update_loads, tx.send).
In `@model_gateway/src/policies/bucket.rs`:
- Around line 33-39: The Drop implementation for BucketPolicy only drops the
JoinHandle (adjustment_handle) but doesn't signal the spawned adjustment thread
to exit, causing a leak; modify BucketPolicy to include a shutdown signal (e.g.,
an Arc<AtomicBool> named shutdown or a channel receiver) that the spawned thread
checks in its loop (check shutdown.load(Ordering::Relaxed) or recv timeout) and
breaks out when signaled, set that signal in BucketPolicy::drop prior to
taking/joining adjustment_handle, and then join the handle (call join on the
taken JoinHandle) to ensure the background thread terminates cleanly.
In `@model_gateway/src/policies/consistent_hashing.rs`:
- Around line 163-177: The fallback selection currently counts healthy workers
then re-iterates workers to pick the Nth healthy, which can return None if
health changes between iterations; instead collect the healthy worker indices in
a single pass (e.g., build a Vec<usize> of indices from
workers.iter().enumerate().filter(|(_, w)| w.is_healthy()).map(|(i,_)|
i).collect()), check if that Vec is empty, then pick a random index from that
Vec (use rand rng to choose within 0..vec.len()) and return Some(chosen_index)
with Branch::RandomFallback; update the code references around healthy_count,
random_healthy_idx, and the .nth(...) logic accordingly so selection is
consistent.
In `@model_gateway/src/routers/grpc/proto_wrapper.rs`:
- Around line 483-528: Add non-panicking "try" accessors alongside the existing
panicking ones: implement try_as_sglang(&self) ->
Option<&sglang::GenerateComplete>, try_as_sglang_mut(&mut self) -> Option<&mut
sglang::GenerateComplete>, try_as_vllm(&self) ->
Option<&vllm::GenerateComplete>, and try_as_trtllm(&self) ->
Option<&trtllm::GenerateComplete> in the same impl that contains
as_sglang/as_sglang_mut/as_vllm/as_trtllm; each should match self and return
Some(complete) for the corresponding variant (Self::Sglang, Self::Vllm,
Self::Trtllm) or None for other variants, so callers can choose Option-based
handling instead of relying on is_* + panicking accessors.
In `@model_gateway/src/routers/parse/handlers.rs`:
- Around line 57-63: Update the error! macro format strings to use named
interpolation for the error variable instead of positional placeholders: replace
occurrences like error!("Failed to parse function calls: {}", e) and
error!("Failed to separate reasoning: {}", e) with error!("Failed to parse
function calls: {e}") and error!("Failed to separate reasoning: {e}")
respectively in the parse handler in handlers.rs; leave the logs that reference
struct field accesses (e.g., req.tool_call_parser and req.reasoning_parser)
as-is since Rust's format inlining does not support field access in the same
way.
In `@model_gateway/tests/common/mock_worker.rs`:
- Around line 1235-1259: RESP_STORE currently retains responses for the process
lifetime causing cross-test leakage; add a cleanup function (e.g.,
remove_responses_for_port or clear_responses_for_port) that locks get_store(),
removes the HashMap entry for the given port (map.remove(&port)), and call this
cleanup from the worker shutdown/stop path so per-port entries are cleared when
a worker stops; reference RESP_STORE, get_store(), store_response_for_port, and
response_exists_for_port to locate where to add and invoke the cleanup.
In `@multimodal/src/vision/image_processor.rs`:
- Around line 79-84: The uint_2d constructor (uint_2d) currently constructs
Self::UintTensor without validating that data.len() matches rows * cols; add a
check that data.len() == rows.checked_mul(cols).unwrap_or(usize::MAX) (or
equivalent safe multiply) and handle mismatches: either change uint_2d to return
a Result<Self, Error> and return Err when lengths differ, or keep the infallible
API but add a debug_assert!(data.len() == rows * cols, "mismatched data length")
and document the precondition; update any callers/tests accordingly.
In `@protocols/src/interactions.rs`:
- Around line 1076-1109: Update validate_interactions_request to enforce and
document the agent/background constraints and add unit tests: ensure that
validate_interactions_request (operating on InteractionsRequest) requires
background == true whenever agent is set, rejects requests that set background
== true without an agent, and rejects requests where background and stream are
both true (these checks already exist but confirm behavior for Gemini agent by
adding a comment above validate_interactions_request documenting "Agent
interactions must set background = true (Gemini requirement)"); add unit tests
exercising (1) agent present with background=false fails, (2) background=true
without agent fails, and (3) background=true and stream=true fails, using the
existing ValidationError variants ("agent_requires_background",
"background_requires_agent", "background_conflicts_with_stream") to assert
correct errors so future changes are caught.
In `@reasoning_parser/src/factory.rs`:
- Around line 600-606: The hardcoded throughput assertion using the computed
variable throughput (from total_requests and duration) can cause CI flakiness;
replace the literal 1000.0 with a configurable threshold read from an
environment variable (e.g. THROUGHPUT_THRESHOLD) parsed into an f64 with a safe
default (or lower default if you prefer), validate/handle parse errors by
falling back to the default, and then assert throughput > threshold using the
parsed value so tests can run reliably on slower CI runners; update the
assertion message to include the actual threshold variable for clarity.
- Around line 63-85: The get_pooled_parser function has a TOCTOU race: two
threads can both miss the pool read and create distinct parser instances, with
one overwriting the other. Fix by using double-checked locking: first read
self.pool to see if the parser exists (keep that early fast path), then if
missing read self.creators to get the creator, then acquire a write lock on
self.pool and check again if the entry exists; if it exists return that Arc,
otherwise call the creator to build the parser, insert it into the pool
(pool.insert(name.to_string(), Arc::clone(&parser))) and return the newly
created Arc. Ensure you reference get_pooled_parser, self.pool, self.creators,
and the creator() call when making the change.
- Around line 608-643: The test currently only checks for absence of panic but
not that pooled parsers preserve referential integrity; update the
test_concurrent_pool_modifications to capture a baseline Arc from
ParserFactory::get_pooled (use ParserFactory::get_pooled("deepseek-r1") or other
model), run the concurrent tasks, then retrieve the parser again and assert
Arc::ptr_eq(&baseline_arc, &new_arc) (or use pointer equality checks for
multiple model keys) to ensure the TOCTOU race is fixed; if needed, use a
synchronization primitive (Barrier) to ensure the baseline is taken before
concurrent clears begin and the final fetch happens after operations complete.
---
Duplicate comments:
In `@kv_index/src/string_tree.rs`:
- Around line 481-484: The code silently returns when let Some(first_new_char) =
contracted_text.first_char() else { ... } finds no char, which hides invariant
violations; replace the silent return with a fail-fast assertion (e.g.,
unreachable! or panic!/debug_assert!) including context (contracted_text and the
invariant "split_at_char with shared_count < char_count guarantees non-empty
suffix") so corruption is surfaced; update the branch in string_tree.rs where
contracted_text.first_char() is matched (the split_at_char / shared_count <
char_count path) to assert/unreachable with a clear error message instead of
returning.
In `@mcp/src/core/oauth.rs`:
- Around line 149-155: The spawned callback server currently runs indefinitely;
replace the fire-and-forget tokio::spawn of axum::serve(listener, app) with a
graceful shutdown tied to the OAuth callback: create a oneshot::channel (or
similar) before spawning, pass the Receiver into
axum::Server::bind(listener).serve(...).with_graceful_shutdown(receiver), and
have the OAuth callback handler send on the Sender when it receives the
authorization code so the server shuts down; update the code around
tokio::spawn, listener, and app to wire the sender into the handler and remove
the misleading reason in the clippy expect if desired.
In `@model_gateway/src/core/token_bucket.rs`:
- Around line 58-63: Add input validation at the public API boundary: in
try_acquire (before delegating to try_acquire_sync) check that tokens is finite
(tokens.is_finite()), non‑negative (tokens >= 0.0) and does not exceed the
bucket capacity (tokens <= self.capacity); if any check fails return the error
variant (Err(())) immediately. Mirror the same validations at the start of
acquire and return_tokens so callers cannot pass
non‑finite/negative/over‑capacity values and thus preserve bucket invariants.
Ensure you reference the same error return convention used by try_acquire (and
consistently for acquire/return_tokens) and do not alter internal helpers like
try_acquire_sync beyond relying on the validated input.
In `@model_gateway/src/middleware.rs`:
- Around line 425-428: The expect attribute's reason text misstates the task
lifetime; update the reason string on the #[expect(...)] attribute (the one
guarding clippy::disallowed_methods) to say that dropping the oneshot receiver
does not cancel the spawned task and that the spawned task can outlive the
request until remaining_timeout completes or the runtime shuts down (mentioning
"remaining_timeout" and "oneshot receiver" in the text for clarity).
In `@model_gateway/src/policies/bucket.rs`:
- Around line 147-169: The removal path leaves stale boundary state when the
last URL is removed because init_prefill_worker_urls is only called when
updated_len > 0; update the logic so that after removing the worker and updating
chars_per_url and bucket_cnt you always reset the bucket boundary state—call
bucket_guard.init_prefill_worker_urls with an empty list (or otherwise clear the
internal boundary structure) when updated_len == 0 so find_boundary and related
logic won't return URLs that no longer exist; adjust the branch around
init_prefill_worker_urls in the removal block (references:
bucket_guard.prefill_worker_urls, bucket_guard.chars_per_url,
bucket_guard.init_prefill_worker_urls, bucket_guard.bucket_cnt, boundary,
find_boundary).
- Around line 116-137: The code always calls
bucket_guard.init_prefill_worker_urls and logs "Added worker" even when the URL
already existed, which resets per-URL state; modify the block that reads
bucket_guard.prefill_worker_urls and bucket_guard.chars_per_url so it first
checks for existence and only performs the push, chars_per_url.entry(...)
insertion, cloning, the call to init_prefill_worker_urls, and the info! log when
the URL was actually newly inserted. Use the existing symbols worker.url(),
bucket_guard.prefill_worker_urls, bucket_guard.chars_per_url, and
init_prefill_worker_urls to gate the state mutation and logging on an actual
addition.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 372-417: The current branch always continues when tool parsing is
attempted, causing normal delta text to be dropped if process_tool_calls_stream
(called on self.process_tool_calls_stream) fails or returns an empty
tool_chunks; change the control flow so you only skip regular content when
tool_chunks actually contains emitted/buffered chunks (i.e., when the parser
successfully handled the delta). Concretely, after calling
Self::process_specific_function_stream or
self.process_tool_calls_stream(...).await, check whether tool_chunks is empty
(or have the called function return a clear "handled" flag/result) and only run
the loop that formats/sends chunks and execute continue when there were emitted
chunks; otherwise fall through to the regular delta processing path so normal
content is preserved.
In `@model_gateway/tests/otel_tracing_test.rs`:
- Around line 249-254: The assertion message in otel_tracing_test.rs is
inconsistent: it currently says "Expected to receive at least 2 span" while the
code asserts equality (span_count == 2); update the assertion message used with
assert! to reflect exact equality and correct pluralization (e.g., "Expected to
receive exactly 2 spans, but got {span_count}."), targeting the assertion
involving the span_count variable in the test and the subsequent println message
to ensure both use consistent wording ("spans" and "exactly 2").
In `@multimodal/src/tracker.rs`:
- Around line 166-169: The expect attribute's reason string in the
#[expect(clippy::disallowed_methods, reason = "...")] on the spawn handle
comment is inaccurate — it says the handle is awaited in resolve() but the
actual method is finalize(); update the reason string to reference finalize()
instead so it reads something like "spawn handle is stored in self.pending and
awaited in finalize(); fire-and-forget is intentional for concurrent media
fetching", keeping the rest of the attribute intact (locate the expect attribute
in tracker.rs near the spawn/pending logic).
In `@protocols/src/responses.rs`:
- Around line 928-931: Replace the unsafe-style comment before the guard with a
safe-code invariant note: change the line starting with "// SAFETY: has_tools is
true here, so tools is Some and non-empty" that precedes the pattern match "let
Some(tools) = request.tools.as_ref() else { return Ok(()); }" to use "//
INVARIANT:" (or "// Note:") instead, keeping the same explanation about
has_tools and tools being Some and non-empty.
In `@protocols/src/worker.rs`:
- Around line 338-343: The defensive fallback in the match arm that handles the
case where models.len() == 1 is unreachable because into_iter().next() must
return Some; replace the current let Some(model) = models.into_iter().next()
else { return Self::Wildcard; } pattern with a direct unwrap/expect to make the
invariant explicit (e.g., call expect with a clear message) and return
Self::Single(Box::new(model)) as before; update the match arm around
models.len() == 1, Self::Single, and Self::Wildcard to reflect this change.
Comment on lines
+296
to
299
| #[expect(dead_code)] | ||
| pub fn contains_key(&self, key: &SKey) -> bool { | ||
| self.inner.read().contains_key(key) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Search for contains_key usage across the codebase
echo "=== Searching for contains_key usage ==="
rg -n '\.contains_key\(' --type rust
echo -e "\n=== Searching for SyncCRDTMap context ==="
rg -n 'SyncCRDTMap' --type rust -B 2 -A 5
echo -e "\n=== Checking mesh/src/crdt.rs around line 296 ==="
sed -n '285,310p' mesh/src/crdt.rsRepository: lightseekorg/smg
Length of output: 16715
Consider adding a documentation comment to clarify API intent.
The #[expect(dead_code)] attribute correctly identifies that contains_key is unused internally. If this method is retained for API completeness or future use, add a comment explaining that intent rather than relying solely on the attribute to communicate purpose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mesh/src/crdt.rs` around lines 296 - 299, Add a short documentation comment
above the contains_key method explaining its purpose and why it exists despite
being unused internally (e.g., provided for API completeness or future external
use), so the #[expect(dead_code)] attribute isn’t the only signal; update the
doc to reference the method name contains_key and mention that it checks
membership via self.inner.read().contains_key(key) for clarity to future
readers.
Member
Author
There was a problem hiding this comment.
The #[expect(dead_code)] with reason attribute already documents the intent. Adding additional doc comments is a style preference out of scope for this PR.
There was a problem hiding this comment.
@slin1237 Acknowledged. We’ll leave contains_key as-is with #[expect(dead_code)] and won’t push additional doc comments in this clippy-enforcement PR. Noting this preference for future reviews.
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: mesh/src/crdt.rs:296-299
Timestamp: 2026-02-21T02:36:27.133Z
Learning: Repo lightseekorg/smg — For clippy/lint-only PRs (e.g., PR `#489`), avoid requesting stylistic doc comments when an item is already annotated with #[expect(...)] (e.g., #[expect(dead_code)] on SyncCRDTMap::contains_key in mesh/src/crdt.rs); such style changes are considered out of scope.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: kv_index/src/string_tree.rs:481-484
Timestamp: 2026-02-21T02:30:52.128Z
Learning: Preference noted: In kv_index/src/string_tree.rs (Rust radix tree), production code should avoid panics on invariant breaches; prefer a defensive non-panicking path (e.g., early return) and use debug_assert! for debug/test builds. Consider documenting the invariant with a comment, and optionally log under cfg(debug_assertions).
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repo (e.g., PR `#489`), avoid introducing behavioral changes (like new input validation). Prefer a separate, follow-up issue/PR for such hardening.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/policies/bucket.rs:116-137
Timestamp: 2026-02-21T02:30:56.454Z
Learning: For clippy-focused PRs in this repository, avoid behavior changes (e.g., load-balancing logic). Defer such changes to a separate PR/issue.
| /// Sync policy state to mesh stores | ||
| pub fn sync_policy_state(&self, model_id: String, policy_type: String, config: Vec<u8>) { | ||
| let key = SKey::new(format!("policy:{}", model_id)); | ||
| let key = SKey::new(format!("policy:{model_id}")); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent key formatting style with apply_remote_policy_state.
Lines 83, 112, and 130 use named interpolation format!("policy:{model_id}"), but line 172 in apply_remote_policy_state still uses positional interpolation format!("policy:{}", state.model_id). Consider updating line 172 for consistency.
Also applies to: 112-112, 130-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mesh/src/sync.rs` at line 83, The key formatting is inconsistent: several
places construct the policy key with named interpolation using
format!("policy:{model_id}") (e.g., SKey::new calls that use that pattern), but
apply_remote_policy_state still uses positional interpolation
format!("policy:{}", state.model_id); update the call inside
apply_remote_policy_state to use the same named-interpolation style
(format!("policy:{model_id}") referencing state.model_id) so all
SKey::new(policy...) usages are consistent with the rest of the file.
Member
Author
There was a problem hiding this comment.
False positive. Rust named interpolation ({model_id}) only works with simple identifiers, not field access expressions. format\!("policy:{}", state.model_id) on line 172 uses positional interpolation because state.model_id is a field access which cannot be inlined into the format string.
There was a problem hiding this comment.
@slin1237 You’re absolutely right—thanks for the correction. Rust’s implicit capture in format strings only supports simple identifiers, not field paths, so {state.model_id} isn’t valid. Line 172 using format!("policy:{}", state.model_id) is correct as-is. No change needed.
If we ever wanted to unify style, we’d have to bind a local first (e.g., let model_id = &state.model_id; then format!("policy:{model_id}")), but that’s unnecessary churn here, especially for this clippy-focused PR where we avoid nonessential edits.
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: mesh/src/sync.rs:83-83
Timestamp: 2026-02-21T02:37:01.416Z
Learning: Rust format! implicit capture like "{ident}" only supports simple identifiers, not field accesses. In mesh/src/sync.rs, keep format!("policy:{}", state.model_id) rather than attempting "{state.model_id}". Use a local binding (e.g., let model_id = &state.model_id;) if named interpolation is desired.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repo (e.g., PR `#489`), avoid introducing behavioral changes (like new input validation). Prefer a separate, follow-up issue/PR for such hardening.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/policies/bucket.rs:116-137
Timestamp: 2026-02-21T02:30:56.454Z
Learning: For clippy-focused PRs in this repository, avoid behavior changes (e.g., load-balancing logic). Defer such changes to a separate PR/issue.
Comment on lines
+411
to
+413
| assert_eq!(images.channels().unwrap(), 3); | ||
| assert_eq!(images.height().unwrap(), 336); | ||
| assert_eq!(images.width().unwrap(), 336); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for invalid tensor dimensionality.
The existing tests only cover the happy path. Consider adding a test that verifies channels(), height(), and width() return TransformError::InvalidShape when given a tensor with unsupported dimensionality (e.g., 3D or 6D).
🧪 Proposed test for error case
#[test]
fn test_preprocessed_images_invalid_ndim() {
use ndarray::ArrayD;
// Create a 3D tensor (invalid for this API)
let pixel_values_3d = ArrayD::<f32>::zeros(vec![2, 3, 336]);
let images = PreprocessedImages::new_dynamic(
pixel_values_3d,
vec![576],
vec![(640, 480)],
);
assert!(images.channels().is_err());
assert!(images.height().is_err());
assert!(images.width().is_err());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multimodal/src/vision/image_processor.rs` around lines 411 - 413, Add a test
that exercises invalid tensor dimensionality for PreprocessedImages by creating
a mismatched-rank ArrayD (e.g., 3D or 6D) and asserting that
PreprocessedImages::new_dynamic yields an instance where calling channels(),
height(), and width() returns Err(TransformError::InvalidShape); locate the APIs
by referencing PreprocessedImages::new_dynamic and the methods channels(),
height(), width() and assert the error variant TransformError::InvalidShape for
each call.
Member
Author
There was a problem hiding this comment.
Adding new tests is out of scope for this clippy-focused PR.
There was a problem hiding this comment.
@slin1237 Acknowledged—sticking to the clippy-only scope; I won’t propose adding new tests in this PR. If you’d like, I can open a follow-up issue to add invalid-ndim tests for PreprocessedImages::channels/height/width later. I’ve noted this repo preference for future reviews.
🧠 Learnings used
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/policies/bucket.rs:116-137
Timestamp: 2026-02-21T02:30:56.454Z
Learning: For clippy-focused PRs in this repository, avoid behavior changes (e.g., load-balancing logic). Defer such changes to a separate PR/issue.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repo (e.g., PR `#489`), avoid introducing behavioral changes (like new input validation). Prefer a separate, follow-up issue/PR for such hardening.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: mesh/src/crdt.rs:296-299
Timestamp: 2026-02-21T02:36:27.133Z
Learning: Repo lightseekorg/smg — For clippy/lint-only PRs (e.g., PR `#489`), avoid requesting stylistic doc comments when an item is already annotated with #[expect(...)] (e.g., #[expect(dead_code)] on SyncCRDTMap::contains_key in mesh/src/crdt.rs); such style changes are considered out of scope.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/benches/wasm_middleware_latency.rs:88-91
Timestamp: 2026-02-21T02:36:55.621Z
Learning: Repo: lightseekorg/smg — For clippy-only/enforcement PRs (e.g., PR `#489`), even micro-optimizations (like replacing an async closure with std::future::ready in benches such as model_gateway/benches/wasm_middleware_latency.rs) should be deferred to a follow-up PR rather than included inline.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/benches/wasm_middleware_latency.rs:1-1
Timestamp: 2026-02-21T02:37:00.043Z
Learning: Repo: lightseekorg/smg — For benchmark files (e.g., model_gateway/benches/*.rs), using a crate-level `#![expect(clippy::unwrap_used, clippy::disallowed_methods)]` is preferred when unwrap/spawn are used throughout. Do not push to per-function scoping; a single crate-level `reason` is acceptable when justification is required.
Comment on lines
+486
to
+490
| let max_crops = all_outputs | ||
| .iter() | ||
| .map(|o| o.shape()[0]) | ||
| .max() | ||
| .ok_or(TransformError::EmptyBatch)?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive but technically unreachable error case.
The ok_or(TransformError::EmptyBatch)? is better than the previous .unwrap() since it provides a proper error type. However, this error case is unreachable because:
- The early check at lines 459-464 returns an error if
images.is_empty(). all_outputsis populated by iterating overimages(lines 477-483).
The defensive approach is acceptable, but consider whether an expect("checked non-empty above") would be clearer about the invariant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multimodal/src/vision/processors/phi4_vision.rs` around lines 486 - 490,
Replace the defensive ok_or(TransformError::EmptyBatch)? on the max_crops
computation with an expect that documents the proven invariant (e.g., change the
call on all_outputs.iter().map(|o| o.shape()[0]).max() to .expect("images
non-empty; all_outputs populated from images") so the code reflects the checked
precondition), updating the variable max_crops accordingly and leaving
TransformError usage untouched elsewhere.
Member
Author
There was a problem hiding this comment.
The defensive .ok_or(TransformError::EmptyBatch)? is intentional — returning a proper error type is safer than panicking via expect(), consistent with this PR's philosophy of replacing panicking code with recoverable errors.
There was a problem hiding this comment.
@slin1237 Got it—thanks for the clarification. We’ll keep the defensive ok_or(TransformError::EmptyBatch)? here. That aligns with the repository’s preference to avoid panics in production paths for this PR. I’ll refrain from suggesting expect/unreachable on proven invariants in similar spots going forward.
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: multimodal/src/vision/processors/phi4_vision.rs:486-490
Timestamp: 2026-02-21T02:39:17.064Z
Learning: Repo: lightseekorg/smg — For multimodal/src/vision/processors/phi4_vision.rs (Phi4VisionProcessor::preprocess), prefer returning a recoverable error via ok_or(TransformError::EmptyBatch)? over using expect/unreachable, even when a prior non-empty check exists. This follows the repo’s “avoid panics in production code” guideline for PR `#489` and similar lint-only efforts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: protocols/src/worker.rs:338-343
Timestamp: 2026-02-21T02:36:00.013Z
Learning: Repo: lightseekorg/smg PR: 489
File: protocols/src/worker.rs (impl From<Vec<ModelCard>> for WorkerModels)
Learning: For the single-element case (models.len() == 1), keep the defensive Option guard with a non-panicking fallback to WorkerModels::Wildcard instead of using expect()/unwrap(). This is intentional to avoid introducing panic paths in production code for PR `#489` and similar clippy/lint-only efforts.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs:189-194
Timestamp: 2026-02-21T02:32:04.647Z
Learning: Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs
Context: GrpcClient::generate fallback match arm
Learning: The catch-all panic for mismatched client/request types in GrpcClient::generate is intentional to enforce pipeline invariants; do not convert this to a returned error in PR `#489` or similar lint-only changes.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: kv_index/src/string_tree.rs:481-484
Timestamp: 2026-02-21T02:30:52.128Z
Learning: Preference noted: In kv_index/src/string_tree.rs (Rust radix tree), production code should avoid panics on invariant breaches; prefer a defensive non-panicking path (e.g., early return) and use debug_assert! for debug/test builds. Consider documenting the invariant with a comment, and optionally log under cfg(debug_assertions).
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs:206-211
Timestamp: 2026-02-21T02:31:14.142Z
Learning: Repo: lightseekorg/smg
File: model_gateway/src/routers/grpc/client.rs
Context: GrpcClient::embed match arm
Learning: The catch-all panic in GrpcClient::embed for mismatched client/request types or unsupported embedding backends is intentional to catch invariant violations and unsupported configurations at development time. Converting this path to a returned error is out of scope for PR `#489`.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repo (e.g., PR `#489`), avoid introducing behavioral changes (like new input validation). Prefer a separate, follow-up issue/PR for such hardening.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: mesh/src/crdt.rs:296-299
Timestamp: 2026-02-21T02:36:27.133Z
Learning: Repo lightseekorg/smg — For clippy/lint-only PRs (e.g., PR `#489`), avoid requesting stylistic doc comments when an item is already annotated with #[expect(...)] (e.g., #[expect(dead_code)] on SyncCRDTMap::contains_key in mesh/src/crdt.rs); such style changes are considered out of scope.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/benches/wasm_middleware_latency.rs:88-91
Timestamp: 2026-02-21T02:36:55.621Z
Learning: Repo: lightseekorg/smg — For clippy-only/enforcement PRs (e.g., PR `#489`), even micro-optimizations (like replacing an async closure with std::future::ready in benches such as model_gateway/benches/wasm_middleware_latency.rs) should be deferred to a follow-up PR rather than included inline.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/policies/bucket.rs:116-137
Timestamp: 2026-02-21T02:30:56.454Z
Learning: For clippy-focused PRs in this repository, avoid behavior changes (e.g., load-balancing logic). Defer such changes to a separate PR/issue.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/benches/wasm_middleware_latency.rs:1-1
Timestamp: 2026-02-21T02:37:00.043Z
Learning: Repo: lightseekorg/smg — For benchmark files (e.g., model_gateway/benches/*.rs), using a crate-level `#![expect(clippy::unwrap_used, clippy::disallowed_methods)]` is preferred when unwrap/spawn are used throughout. Do not push to per-function scoping; a single crate-level `reason` is acceptable when justification is required.
What changed: - server.rs: Fix P1 bug — HealthChecker handle was bound to block-local variable inside else branch, causing immediate Drop and aborting the health check background task. Moved binding to outer scope so it lives for the entire server lifetime. - middleware.rs: Fix inaccurate #[expect] reason string about spawned task lifetime — task is bounded by remaining_timeout and self-terminates, dropping JoinHandle only detaches it. - metrics_aggregator_test.rs: Remove redundant .trim() calls since assert_eq_sorted already trims internally. - otel_tracing_test.rs: Fix assertion/message mismatch — assertion uses == (exact) but message said "at least". Fixed to "exactly 2 spans". - tracker.rs: Fix incorrect method name in #[expect] reason string — resolve() → finalize() to match actual method. - responses.rs: Rename // SAFETY: to // INVARIANT: for non-unsafe code following Rust documentation conventions. Why: Addresses PR #489 review feedback. The HealthChecker bug is a real production issue where periodic worker health checks were never running after startup. Other fixes correct documentation accuracy. Signed-off-by: Simo Lin <simo.lin@oracle.com>
Contributor
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances code quality and reliability across the entire workspace by enforcing strict Clippy linting rules. It addresses numerous potential panic points, improves error handling, and refines API consistency and efficiency. The changes span across 26 crates, involving a comprehensive audit and fix of over 300 files, leading to a more robust and maintainable codebase. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the codebase's quality and robustness by enforcing a much stricter set of clippy lints across the entire workspace. The changes are extensive, touching over 300 files, and demonstrate a deep understanding of Rust best practices.
I'm particularly impressed by the proactive bug fixes that emerged from this linting effort, such as resolving data corruption issues, race conditions, and task leaks. The mechanical fixes, like migrating to parking_lot locks and eliminating unnecessary async blocks, further enhance maintainability and performance.
The use of #[expect] with clear justifications for lint suppressions is a great practice that will help maintain code quality going forward. The entire pull request is well-structured and the detailed description provides valuable context for this large-scale refactoring. This is a fantastic contribution to the project's long-term health.
slin1237
added a commit
that referenced
this pull request
Feb 21, 2026
…error events
Hand-built JSON format strings like `format!("data: {{\"error\": \"{e}\"}}\n\n")`
produce invalid JSON when the error message contains quotes, newlines, or other
special characters. This introduces a shared `send_error_sse()` helper in
`grpc/utils.rs` that uses `serde_json::json!()` for proper escaping and
standardizes the error event shape to `{"error":{"message":"...","type":"..."}}`.
Files changed:
- model_gateway/src/routers/grpc/utils.rs: add SseSender type alias and
send_error_sse() helper function
- model_gateway/src/routers/grpc/regular/streaming.rs: replace 5 hand-built
SSE error chunks (chat single/dual, generate single/dual, embedding) with
send_error_sse()
- model_gateway/src/routers/grpc/harmony/streaming.rs: replace 3 hand-built
SSE error chunks (single, dual, embedding) with send_error_sse()
- model_gateway/src/routers/grpc/regular/responses/streaming.rs: replace 2
hand-built SSE error events (stream_error, tool_loop_error) with
send_error_sse()
Addresses PR #489 review comment about JSON escaping correctness.
Signed-off-by: Simo Lin <simo.lin@oracle.com>
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@model_gateway/src/middleware.rs`:
- Around line 425-428: The clippy expect attribute's reason string for
clippy::disallowed_methods is already correct—leave the attribute as-is on the
spawn/oneshot block (the expect applied to the spawn/JoinHandle/oneshot usage
that performs a fire-and-forget permit acquisition bounded by
remaining_timeout); no code change required other than keeping this precise
reason text to document why the disallowed_methods lint is allowed here.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/grpc/utils.rs (1)
475-507:⚠️ Potential issue | 🟡 MinorAvoid silent fallback on invariant violation.
The
pop()fallback returns an emptyProcessedMessages, which silently changes behavior if the invariant is ever violated. Prefer anexpectwith an invariant message to keep fail-fast behavior.Suggested fix
- let Some(last_msg) = transformed_messages.pop() else { - return Ok(ProcessedMessages { - text: String::new(), - multimodal_inputs: None, - stop_sequences: request.stop.clone(), - }); - }; + let last_msg = transformed_messages + .pop() + .expect("INVARIANT: last message exists after !is_empty() check");Based on learnings: For lint-only/Clippy enforcement PRs in this repository, avoid introducing behavioral changes (e.g., new input validation or logic changes). Treat such PRs as non-functional changes and plan a separate follow-up issue/PR for hardening or behavior changes; and in Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code. Reserve SAFETY: for explaining why unsafe blocks are sound.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/utils.rs` around lines 475 - 507, The code currently silently returns an empty ProcessedMessages if transformed_messages.pop() unexpectedly fails; change this to fail fast by replacing the fallible pop handling with a panic via expect (e.g., call transformed_messages.pop().expect(...)) and add an INVARIANT: comment documenting the assumption that transformed_messages is non-empty when request.continue_final_message is true and last role == "assistant" (refer to the assistant_prefix block and the transformed_messages.pop() usage); do not alter surrounding control flow or return values—only replace the silent fallback with expect and annotate the invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/harmony/streaming.rs`:
- Around line 110-117: The Embedding arm (context::ExecutionResult::Embedding)
sends an error via utils::send_error_sse(&tx, ...) but does not send the final
"[DONE]" terminator like the Single and Dual arms; update this branch to call
the same done-terminator used elsewhere (e.g., utils::send_done_sse(&tx) or the
existing helper used in the Single/Dual paths) immediately after
utils::send_error_sse so SSE clients receive the final "[DONE]" event and don't
hang.
In `@model_gateway/src/routers/grpc/regular/responses/streaming.rs`:
- Around line 517-520: The computed created_at value in streaming.rs (the let
created_at =
SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_secs();)
silently falls back to 0 if the system clock is before 1970; add a brief inline
comment next to this created_at calculation clarifying that unwrap_or_default()
intentionally yields 0 as a cosmetic metadata fallback (safe for SSE event
metadata) so future readers/debuggers understand the behavior and that this is
not the CRDT timestamp bug.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/utils.rs`:
- Around line 475-507: The code currently silently returns an empty
ProcessedMessages if transformed_messages.pop() unexpectedly fails; change this
to fail fast by replacing the fallible pop handling with a panic via expect
(e.g., call transformed_messages.pop().expect(...)) and add an INVARIANT:
comment documenting the assumption that transformed_messages is non-empty when
request.continue_final_message is true and last role == "assistant" (refer to
the assistant_prefix block and the transformed_messages.pop() usage); do not
alter surrounding control flow or return values—only replace the silent fallback
with expect and annotate the invariant.
---
Duplicate comments:
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 349-394: The code unconditionally continues when tool parsing is
active, causing normal_text to be dropped if parsing fails; change the control
flow around process_specific_function_stream/process_tool_calls_stream so you
only skip the normal content when the parser actually succeeded/emitted chunks
or explicitly buffered the normal_text (i.e., inspect the result/ok-value of
Self::process_specific_function_stream and self.process_tool_calls_stream and
only execute the continue when those return a successful indication of handled
tool output), otherwise let execution fall through to the existing normal-text
handling path and ensure parse errors are propagated or logged alongside
emitting any partial normal_text; reference functions
process_specific_function_stream, process_tool_calls_stream, variables
tool_chunks, sse_buffer, tx, and delta to locate and adjust the logic.
Comment on lines
110
to
117
| context::ExecutionResult::Embedding { .. } => { | ||
| error!("Harmony streaming not supported for embeddings"); | ||
| let error_chunk = format!( | ||
| "data: {}\n\n", | ||
| json!({ | ||
| "error": { | ||
| "message": "Embeddings not supported in Harmony streaming", | ||
| "type": "invalid_request_error" | ||
| } | ||
| }) | ||
| utils::send_error_sse( | ||
| &tx, | ||
| "Embeddings not supported in Harmony streaming", | ||
| "invalid_request_error", | ||
| ); | ||
| let _ = tx.send(Ok(Bytes::from(error_chunk))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pre-existing: Missing [DONE] terminator for embedding error path.
The Single and Dual paths (lines 93, 107) send [DONE] after errors, but the Embedding path only sends the error SSE without a [DONE] terminator. This could leave SSE clients hanging.
Since this is a lint-only PR, consider addressing in a follow-up:
context::ExecutionResult::Embedding { .. } => {
error!("Harmony streaming not supported for embeddings");
utils::send_error_sse(
&tx,
"Embeddings not supported in Harmony streaming",
"invalid_request_error",
);
+ let _ = tx.send(Ok(Bytes::from("data: [DONE]\n\n")));
}Based on learnings: For lint-only/Clippy enforcement PRs, avoid introducing behavioral changes; prefer a separate follow-up issue/PR for such hardening.
Do you want me to open a follow-up issue to track this?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| context::ExecutionResult::Embedding { .. } => { | |
| error!("Harmony streaming not supported for embeddings"); | |
| let error_chunk = format!( | |
| "data: {}\n\n", | |
| json!({ | |
| "error": { | |
| "message": "Embeddings not supported in Harmony streaming", | |
| "type": "invalid_request_error" | |
| } | |
| }) | |
| utils::send_error_sse( | |
| &tx, | |
| "Embeddings not supported in Harmony streaming", | |
| "invalid_request_error", | |
| ); | |
| let _ = tx.send(Ok(Bytes::from(error_chunk))); | |
| } | |
| context::ExecutionResult::Embedding { .. } => { | |
| error!("Harmony streaming not supported for embeddings"); | |
| utils::send_error_sse( | |
| &tx, | |
| "Embeddings not supported in Harmony streaming", | |
| "invalid_request_error", | |
| ); | |
| let _ = tx.send(Ok(Bytes::from("data: [DONE]\n\n"))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/routers/grpc/harmony/streaming.rs` around lines 110 - 117,
The Embedding arm (context::ExecutionResult::Embedding) sends an error via
utils::send_error_sse(&tx, ...) but does not send the final "[DONE]" terminator
like the Single and Dual arms; update this branch to call the same
done-terminator used elsewhere (e.g., utils::send_done_sse(&tx) or the existing
helper used in the Single/Dual paths) immediately after utils::send_error_sse so
SSE clients receive the final "[DONE]" event and don't hang.
Comment on lines
517
to
520
| let created_at = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .unwrap() | ||
| .unwrap_or_default() | ||
| .as_secs(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Consider documenting the fallback behavior.
duration_since(UNIX_EPOCH) fails if the system clock is set before 1970. Using unwrap_or_default() silently falls back to 0, which is acceptable for SSE event metadata but could be confusing during debugging.
This is different from the CRDT timestamp fix mentioned in the PR objectives—there, timestamp=0 causes data corruption, whereas here it's just cosmetic metadata. No change needed, but a brief inline comment could clarify intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/routers/grpc/regular/responses/streaming.rs` around lines
517 - 520, The computed created_at value in streaming.rs (the let created_at =
SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_secs();)
silently falls back to 0 if the system clock is before 1970; add a brief inline
comment next to this created_at calculation clarifying that unwrap_or_default()
intentionally yields 0 as a cosmetic metadata fallback (safe for SSE event
metadata) so future readers/debuggers understand the behavior and that this is
not the CRDT timestamp bug.
…error events
Hand-built JSON format strings like `format!("data: {{\"error\": \"{e}\"}}\n\n")`
produce invalid JSON when the error message contains quotes, newlines, or other
special characters. This introduces a shared `send_error_sse()` helper in
`grpc/utils.rs` that uses `serde_json::json!()` for proper escaping and
standardizes the error event shape to `{"error":{"message":"...","type":"..."}}`.
Files changed:
- model_gateway/src/routers/grpc/utils.rs: add SseSender type alias and
send_error_sse() helper function
- model_gateway/src/routers/grpc/regular/streaming.rs: replace 5 hand-built
SSE error chunks (chat single/dual, generate single/dual, embedding) with
send_error_sse()
- model_gateway/src/routers/grpc/harmony/streaming.rs: replace 3 hand-built
SSE error chunks (single, dual, embedding) with send_error_sse()
- model_gateway/src/routers/grpc/regular/responses/streaming.rs: replace 2
hand-built SSE error events (stream_error, tool_loop_error) with
send_error_sse()
Addresses PR #489 review comment about JSON escaping correctness.
Signed-off-by: Simo Lin <simo.lin@oracle.com>
3 tasks
This was referenced Feb 21, 2026
slin1237
added a commit
that referenced
this pull request
Feb 22, 2026
Fixes deferred from the clippy enforcement PR that required behavioral changes: SSE streaming: - harmony/streaming.rs, regular/streaming.rs: add missing [DONE] terminator to embedding error paths. Single/Dual paths already sent [DONE] after errors, but the Embedding path did not, which could leave SSE clients hanging waiting for the stream to end. Bucket policy: - policies/bucket.rs (add_prefill_url): add early-return guard when the worker URL already exists. Previously, duplicate adds would call init_prefill_worker_urls which resets load counters (chars_per_url) and recomputes boundaries, wiping balancing history. - policies/bucket.rs (remove_prefill_url): always call init_prefill_worker_urls after removal, even when the last URL is removed. Previously the cleanup was skipped when updated_len == 0, leaving stale boundary and chars_per_url state. Token bucket: - core/token_bucket.rs: add debug_assert! on try_acquire_sync and return_tokens_sync to catch non-finite or negative token amounts during development. Zero-cost in release builds. Worker properties: - update_worker_properties.rs: add warn! log when a DP-aware worker is missing dp_rank or dp_size metadata. Previously this inconsistent state was silently ignored, making misconfigurations hard to diagnose. Refs: PR #489 review comments Signed-off-by: Simo Lin <simo.lin@oracle.com>
4 tasks
slin1237
added a commit
that referenced
this pull request
Feb 22, 2026
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
5 tasks
3 tasks
2 tasks
4 tasks
4 tasks
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.
Summary
Introduce
clippy.tomland workspace-level[lints.clippy]configuration to enforce production-grade code quality standards across all 26 crates. Every clippy warning is now denied via-D warnings. This is a comprehensive audit and fix of 320 files with ~4,400 lines of changes.This codebase serves billions of tokens per minute. The previous lint configuration was permissive, allowing
unwrap/expect/panicin production code paths without justification. This change enforces that every potential panic point is either eliminated (proper error propagation) or explicitly justified with a reason string explaining why it is safe.What changed
New configuration files
clippy.toml: Configures disallowed methods (tokio::spawnrequires justification),allow-expect-in-tests, andallow-unwrap-in-testsfor test/bench ergonomics.Cargo.toml: Workspace-level[lints.clippy]denyingunwrap_used,expect_used,panic,print_stdout/stderr,dbg_macro,todo/unimplemented, and other unsafe patterns. Enables pedantic and nursery lint groups with targeted exceptions.Real bugs found and fixed
unwrap_or_default()onSystemTimesilently producedtimestamp=0, causing LWW data loss during merge (timestamp 0 always loses to any other write)mesh/src/crdt.rs,incremental.rs,ping_server.rs.expect()— a clock before UNIX epoch is a fatal misconfiguration that must crash, not silently corrupt data.get().expect()afteror_insert_with()could panic if another thread calledremove()between the two operationsmodel_gateway/src/observability/gauge_histogram.rs.downgrade()on the entry ref directly, eliminating the race windowtokio::spawnreason claimed "abort() is called on drop" but noDropimpl existed.shutdown()was dead code. Spawned health-check tasks would leak on shutdown.model_gateway/src/core/worker.rsDropimpl that callshandle.abort(). Changed handle toOption<JoinHandle>so gracefulshutdown()andDropcoexist correctly.Resultwrappers hiding infallible code: 5 trtllm builder methods and 2 WASM constructors returnedResultbut never returnedErr, adding dead error paths and unnecessary?operatorsgrpc_client/src/trtllm_service.rs,wasm/src/runtime.rs,wasm/src/module_manager.rsResultwrappers entirely.expect()calls in storage constructors would crash the process on DB schema initialization failuredata_connector/src/postgres.rs?error propagationProduction code quality improvements
Lock migration (
bucket.rs,memory.rs):std::sync::Mutex/RwLocktoparking_lotequivalentsUnnecessary async removal (19 functions):
asyncfrom functions that contained no.awaitcalls (flagged byunused_asynclint)TokenBucket::try_acquire/return_tokens/available_tokens(usedparking_lot::Mutex, never truly async),create_grpc_router,prepare_chat,prepare_responses,execute_tool_loop_streaming,handle_streaming_with_tool_interception,execute_mcp_streaming,merge_state,create_snapshot_chunks,start_rotation_monitor.await— verified by 6-agent audit that zero callers were missedAPI signature improvements:
Worker::api_key():&Option<String>→Option<&String>(idiomatic Rust). All callers updated (.clone()→.cloned(), removed.as_ref())WorkerRegistry::get_by_type/get_by_connection: Changed to take ownedCopytypes instead of referencesprotocols/src/common.rs:&Option<ToolChoice>→Option<&ToolChoice>Panic elimination in production code:
protocols/src/chat.rs,responses.rs: Replaced unsafe.unwrap()on optional tool arrays withif-let/else-returnpatternskv_index/src/string_tree.rs: Replaced.chars().next().unwrap()in while loops withwhile-letpatternsmcp/src/core/pool.rs: Added.max(1)guard beforeNonZeroUsize::new()to prevent panic on user-provided 0tokio::spawndocumentation:tokio::spawncall site now has#[expect(clippy::disallowed_methods)]with a reason string documenting: who stores the handle, how it's aborted, and what coordinates shutdownMechanical fixes (all 320 files)
format!("{}", x)→format!("{x}")(Rust 2021 syntax)r#"..."#→r"..."where content has no double-quotes#[allow]→#[expect]migration:#[expect]fails if the lint never fires, catching stale suppressionsclone_from()for efficiency:x = y.clone()→x.clone_from(&y)(avoids reallocation when capacity suffices)_ => panic!→SpecificVariant => panic!for compile-time exhaustiveness safetyCopytype pass-by-value:&self→selffor methods onCopytypes like enums,Ipv4Addr, bitflagsunused_selfcleanup: Private methods that don't useselfchanged fromself.method()toSelf::method()in callers. Public methods keep&selfwith#[expect(clippy::unused_self)]for API stability.Resultremoval: Functions that never returnErrhad theirResultwrappers removedTest and benchmark changes
create_app()changed fromasync fntofn(was never truly async). All ~90 callers updated.#![expect(clippy::unwrap_used, ...)]for ergonomic benchmark code#[expect(clippy::expect_used)]on test helpers,#[expect(clippy::disallowed_methods)]on testtokio::spawncallsCrates affected
model_gatewayprotocolsmeshmcptool_parsertokenizergrpc_clientmultimodaldata_connectorkv_indexauthwasmworkflowbindings/golangbindings/pythonreasoning_parserHow verified
cargo check --all-targets— passes cleanlycargo clippy --all-targets -- -D warnings— zero errors across all 26 cratescargo fmt --all -- --check— zero formatting diffs.awaitin body, all callers updated)Test plan
cargo check --all-targetspassescargo clippy --all-targets -- -D warningsproduces zero errorscargo fmt --all -- --checkproduces zero diffsSummary by CodeRabbit
New Features
Refactor
Improvements
Chores