Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughTest-only changes across the codebase: many test modules now import ChangesTest assertion migration & pattern-match assertions
Span attributes tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 14 minutes and 21 seconds. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/gateway/formats/openai/mod.rs (1)
228-231: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
assert_matches!per coding guidelines.The coding guidelines specify using
assert_matches::assert_matchesinstead ofassert!(matches!(...))for better test output. This is especially relevant given this PR's objective of improving test assertion quality.♻️ Refactor to use assert_matches
First, add the import at the top of the test module (after line 71):
use pretty_assertions::assert_eq; +use assert_matches::assert_matches; use serde_json::json;Then replace the assertion:
- assert!(matches!( + assert_matches!( error, GatewayError::NativeNotSupported { provider } if provider == "dummy" - )); + );As per coding guidelines: "Use pretty_assertions::assert_eq and assert_matches::assert_matches for better test output"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/formats/openai/mod.rs` around lines 228 - 231, Replace the assert!(matches!(...)) test assertion with assert_matches! for better diagnostics: add an import for assert_matches::assert_matches to the test module imports, then change the assertion that checks GatewayError::NativeNotSupported { provider } if provider == "dummy" to use assert_matches!(error, GatewayError::NativeNotSupported { provider } if provider == "dummy"); ensure the symbol names (assert_matches!, GatewayError::NativeNotSupported, provider) are used exactly as in the diff.src/gateway/traits/provider.rs (1)
322-336:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix import ordering to comply with rustfmt conventions.
The
pretty_assertionsimport (external crate) should come after standard library imports and be grouped alphabetically with other external crate imports. As per coding guidelines, imports should be sorted: standard library first, then external crates (alphabetical), then local modules.📦 Proposed fix for import ordering
- use pretty_assertions::assert_eq; use std::borrow::Cow; use http::HeaderMap; + use pretty_assertions::assert_eq; use serde_json::json; use super::{As per coding guidelines: "Sort imports alphabetically with rustfmt rules: standard library first, then external crates (alphabetical), then local modules"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/traits/provider.rs` around lines 322 - 336, The import block is out of rustfmt ordering: move std::borrow::Cow to the top (standard library), then group external crates alphabetically (http::HeaderMap, pretty_assertions::assert_eq, serde_json::json), and finally keep local module imports (the super::{ChatTransform, CompatQuirks, EmbedTransform, ProviderMeta, ProviderSemanticConventions, StreamReaderKind} and crate::gateway::{provider_instance::ProviderAuth, traits::chat_format::ChatStreamState, types::embed::{EmbedRequestBody, EmbedResponseBody, EmbeddingRequest}}) last; update the import list accordingly so it follows "std, external (alphabetical), local" ordering.
🧹 Nitpick comments (11)
src/gateway/types/openai/responses.rs (1)
409-413: ⚡ Quick winAlso switch pattern assertions to
assert_matchesin this touched test module.Nice move to
pretty_assertions::assert_eq; this module still uses manyassert!(matches!(...))checks. Please import/useassert_matches::assert_matcheshere as well to fully align with the test assertion guideline and improve mismatch diagnostics.As per coding guidelines,
{tests,src}/**/*.rsshould usepretty_assertions::assert_eqandassert_matches::assert_matchesfor better test output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/types/openai/responses.rs` around lines 409 - 413, Add an import for assert_matches (use assert_matches::assert_matches;) to the test module and replace all pattern checks written as assert!(matches!(...)) with assert_matches!(<expr>, <pattern>) calls (e.g., update assertions inside this module that currently use matches! to use assert_matches!); update the module that already imports pretty_assertions::assert_eq (alongside serde_json::json and super::*) so all pattern-matching assertions use assert_matches for improved diagnostics.src/gateway/traits/chat_format.rs (1)
148-152: ⚡ Quick winPlace
stdimports before external crates in the test moduleThe new
pretty_assertionsimport is currently ordered beforestd::borrow::Cow. Reorder these imports to follow the repository Rust import-order rule.Suggested diff
- use pretty_assertions::assert_eq; use std::borrow::Cow; use http::HeaderMap; + use pretty_assertions::assert_eq; use serde_json::json;As per coding guidelines, "
**/*.rs: Sort imports alphabetically with rustfmt rules: standard library first, then external crates (alphabetical), then local modules".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/traits/chat_format.rs` around lines 148 - 152, The import ordering in the test module is incorrect: move the standard-library import std::borrow::Cow to appear before external-crate imports like pretty_assertions::assert_eq so std imports come first, then external crates (alphabetically), then local modules; update the use statements around HeaderMap and serde_json::json in src/gateway/traits/chat_format.rs accordingly to follow rustfmt/ repository rules.src/gateway/types/openai/mod.rs (1)
363-363: ⚡ Quick winSwitch pattern assertions to
assert_matches!to match test guideline.These assertions currently use
assert!(matches!(...)); the repo guideline asks forassert_matches::assert_matches.Suggested patch
mod tests { + use assert_matches::assert_matches; use pretty_assertions::assert_eq; use serde_json::json; @@ - assert!(matches!(req.tool_choice, Some(ToolChoice::Mode(ref s)) if s == "auto")); + assert_matches!(req.tool_choice, Some(ToolChoice::Mode(ref s)) if s == "auto"); @@ - assert!(matches!(msg.content, Some(MessageContent::Text(ref s)) if s == "Hello")); + assert_matches!(msg.content, Some(MessageContent::Text(ref s)) if s == "Hello"); @@ - assert!(matches!(msg.content, Some(MessageContent::Parts(ref p)) if p.len() == 2)); + assert_matches!(msg.content, Some(MessageContent::Parts(ref p)) if p.len() == 2); @@ - assert!(matches!(single, StopCondition::Single(ref s) if s == "stop")); + assert_matches!(single, StopCondition::Single(ref s) if s == "stop"); @@ - assert!(matches!(multiple, StopCondition::Multiple(ref v) if v.len() == 2)); + assert_matches!(multiple, StopCondition::Multiple(ref v) if v.len() == 2);As per coding guidelines: "
{tests,src}/**/*.rs: Usepretty_assertions::assert_eqandassert_matches::assert_matchesfor better test output".Also applies to: 441-441, 451-451, 457-457, 460-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/types/openai/mod.rs` at line 363, Replace assertions using assert!(matches!(...)) with the assert_matches::assert_matches macro: change the line asserting the pattern on req.tool_choice to assert_matches::assert_matches!(req.tool_choice, Some(ToolChoice::Mode(ref s)) if s == "auto"); do the same for the other occurrences that match ToolChoice patterns (the assertions at the other noted locations) so tests use assert_matches::assert_matches instead of assert!(matches!(...)).src/proxy/hooks/rate_limit/ratelimit/local.rs (1)
66-67: ⚡ Quick winSort external test imports alphabetically.
Lines 66–67 are out of order;
http::HeaderMapshould come beforepretty_assertions::assert_eq.As per coding guidelines: "`**/*.rs`: Sort imports alphabetically with rustfmt rules: standard library first, then external crates (alphabetical), then local modules".Proposed import reorder
- use pretty_assertions::assert_eq; use http::HeaderMap; + use pretty_assertions::assert_eq;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks/rate_limit/ratelimit/local.rs` around lines 66 - 67, Reorder the test imports so external crates follow alphabetical order per rustfmt rules: place http::HeaderMap before pretty_assertions::assert_eq in the import list; update the use statements referencing HeaderMap and assert_eq in src/proxy/hooks/rate_limit/ratelimit/local.rs so external imports are alphabetized (std first, then external crates alphabetically, then local modules).src/proxy/hooks/rate_limit/ratelimit/utils.rs (1)
258-259: ⚡ Quick winImport order in tests is flipped (std should be first).
Please place the
std::time::Durationimport beforepretty_assertions::assert_eqto match repository ordering rules.As per coding guidelines: "`**/*.rs`: Sort imports alphabetically with rustfmt rules: standard library first, then external crates (alphabetical), then local modules".Proposed import reorder
- use pretty_assertions::assert_eq; use std::time::Duration; + use pretty_assertions::assert_eq;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks/rate_limit/ratelimit/utils.rs` around lines 258 - 259, Reorder the test imports so the standard library comes before external crates: move the std::time::Duration import to appear before pretty_assertions::assert_eq to follow rustfmt import ordering (standard library first, then external crates); update the use statements in the module containing Duration and assert_eq accordingly.src/utils/instance.rs (1)
100-101: ⚡ Quick winReorder test imports to keep std before external crates.
Line 100 should come after Line 101 to match the Rust import-order rule used in this repo.
As per coding guidelines: "`**/*.rs`: Sort imports alphabetically with rustfmt rules: standard library first, then external crates (alphabetical), then local modules".Proposed import reorder
- use pretty_assertions::assert_eq; use std::path::PathBuf; + use pretty_assertions::assert_eq;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/instance.rs` around lines 100 - 101, Move the standard library import "use std::path::PathBuf;" above the external crate import "use pretty_assertions::assert_eq;" in src/utils/instance.rs so that std imports come before external crates (i.e., reorder the two use statements to follow the repo's rustfmt/rust import ordering rules).src/gateway/providers/mistral.rs (1)
45-45: ⚡ Quick winKeep test assertions on
pretty_assertions::assert_eqLine 45 switches to
assert!, which breaks the consistency goal of this PR and the test assertion guideline. Prefer:Suggested change
- assert!(provider.default_quirks().tool_args_may_be_object); + assert_eq!(provider.default_quirks().tool_args_may_be_object, true);As per coding guidelines,
{tests,src}/**/*.rs: “Use pretty_assertions::assert_eq and assert_matches::assert_matches for better test output”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/mistral.rs` at line 45, Replace the bare assert! on provider.default_quirks().tool_args_may_be_object with a pretty_assertions::assert_eq call to keep test assertion consistency; update the assertion to assert_eq!(provider.default_quirks().tool_args_may_be_object, true) and ensure pretty_assertions::assert_eq is in scope (add the crate import in the test module if missing).src/gateway/providers/macros.rs (1)
150-151: ⚡ Quick winReorder imports per coding guidelines.
Standard library imports should come before external crates. As per coding guidelines, sort imports with std first, then external crates alphabetically, then local modules.
📦 Proposed fix
-use pretty_assertions::assert_eq; use std::borrow::Cow; + +use pretty_assertions::assert_eq;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/macros.rs` around lines 150 - 151, Reorder the import statements so standard library imports come before external crates: move "use std::borrow::Cow;" above "use pretty_assertions::assert_eq;" and then place any local module imports after externals; update the import block around the symbols "std::borrow::Cow" and "pretty_assertions::assert_eq" to follow the coding guideline ordering (std, external crates alphabetically, then local modules).src/gateway/streams/hub.rs (1)
110-111: ⚡ Quick winReorder imports per coding guidelines.
Standard library imports should come before external crates. As per coding guidelines, sort imports with std first, then external crates alphabetically, then local modules.
📦 Proposed fix
-use pretty_assertions::assert_eq; use std::sync::Arc; + +use pretty_assertions::assert_eq;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/streams/hub.rs` around lines 110 - 111, Reorder the top-level imports in src/gateway/streams/hub.rs so standard library imports come first, then external crates alphabetically, then any local modules; specifically move use std::sync::Arc; above use pretty_assertions::assert_eq; (and ensure any other imports follow the same std → external → local ordering and alphabetical within groups) so the import block follows the project's import ordering guidelines.src/gateway/types/anthropic.rs (1)
361-361: ⚡ Quick winConsider also adopting
assert_matches!to fully comply with coding guidelines.The coding guidelines specify using both
pretty_assertions::assert_eqandassert_matches::assert_matchesfor better test output. Several assertions in this file use theassert!(matches!(...))pattern, which would benefit from theassert_matches!macro's improved failure diagnostics.Example refactor
Add the import:
use pretty_assertions::assert_eq; +use assert_matches::assert_matches; use serde_json::json;Then convert patterns like:
-assert!(matches!(req.system, Some(SystemPrompt::Text(ref s)) if s == "You are helpful.")); +assert_matches!(req.system, Some(SystemPrompt::Text(ref s)) if s == "You are helpful.");Similar conversions can be applied to assertions on lines 415, 460, 509, and 543.
As per coding guidelines: "Use pretty_assertions::assert_eq and assert_matches::assert_matches for better test output"
Also applies to: 415-415, 460-460, 509-509, 543-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/types/anthropic.rs` at line 361, Replace usages of assert!(matches!(...)) with assert_matches::assert_matches for clearer failure diagnostics: add an import for assert_matches::assert_matches at the top of the test module and change assertions like the one in anthropic.rs that checks req.system (e.g., assert!(matches!(req.system, Some(SystemPrompt::Text(ref s)) if s == "You are helpful."))) to use assert_matches!(req.system, Some(SystemPrompt::Text(ref s)) if s == "You are helpful."); apply the same conversion to the similar assertions at the other mentioned locations (lines with checks at 415, 460, 509, and 543) so all pattern-match assertions use assert_matches!.src/gateway/streams/reader/aws_event_stream.rs (1)
183-183: 💤 Low valueGood addition of
pretty_assertions, but fix import ordering.The import of
pretty_assertions::assert_eqcorrectly aligns with the coding guidelines for improved test output. However, imports should be sorted alphabetically (external crates, then local modules). Thepretty_assertionsimport should come afterfuturesand beforeserde_json.♻️ Proposed fix to sort imports alphabetically
- use pretty_assertions::assert_eq; use aws_smithy_eventstream::frame::write_message_to; use aws_smithy_types::event_stream::{Header, HeaderValue, Message}; use bytes::Bytes; use futures::StreamExt; + use pretty_assertions::assert_eq; use serde_json::json;As per coding guidelines: "Use pretty_assertions::assert_eq and assert_matches::assert_matches for better test output" and "Sort imports alphabetically with rustfmt rules: standard library first, then external crates (alphabetical), then local modules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/streams/reader/aws_event_stream.rs` at line 183, The import for pretty_assertions::assert_eq is out of alphabetical order; reorder the use statements so external crates are alphabetical (ensure futures comes before pretty_assertions::assert_eq, and pretty_assertions::assert_eq comes before serde_json) while keeping standard library imports first and local modules last to comply with rustfmt/import sorting rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/gateway/formats/openai/mod.rs`:
- Around line 228-231: Replace the assert!(matches!(...)) test assertion with
assert_matches! for better diagnostics: add an import for
assert_matches::assert_matches to the test module imports, then change the
assertion that checks GatewayError::NativeNotSupported { provider } if provider
== "dummy" to use assert_matches!(error, GatewayError::NativeNotSupported {
provider } if provider == "dummy"); ensure the symbol names (assert_matches!,
GatewayError::NativeNotSupported, provider) are used exactly as in the diff.
In `@src/gateway/traits/provider.rs`:
- Around line 322-336: The import block is out of rustfmt ordering: move
std::borrow::Cow to the top (standard library), then group external crates
alphabetically (http::HeaderMap, pretty_assertions::assert_eq,
serde_json::json), and finally keep local module imports (the
super::{ChatTransform, CompatQuirks, EmbedTransform, ProviderMeta,
ProviderSemanticConventions, StreamReaderKind} and
crate::gateway::{provider_instance::ProviderAuth,
traits::chat_format::ChatStreamState, types::embed::{EmbedRequestBody,
EmbedResponseBody, EmbeddingRequest}}) last; update the import list accordingly
so it follows "std, external (alphabetical), local" ordering.
---
Nitpick comments:
In `@src/gateway/providers/macros.rs`:
- Around line 150-151: Reorder the import statements so standard library imports
come before external crates: move "use std::borrow::Cow;" above "use
pretty_assertions::assert_eq;" and then place any local module imports after
externals; update the import block around the symbols "std::borrow::Cow" and
"pretty_assertions::assert_eq" to follow the coding guideline ordering (std,
external crates alphabetically, then local modules).
In `@src/gateway/providers/mistral.rs`:
- Line 45: Replace the bare assert! on
provider.default_quirks().tool_args_may_be_object with a
pretty_assertions::assert_eq call to keep test assertion consistency; update the
assertion to assert_eq!(provider.default_quirks().tool_args_may_be_object, true)
and ensure pretty_assertions::assert_eq is in scope (add the crate import in the
test module if missing).
In `@src/gateway/streams/hub.rs`:
- Around line 110-111: Reorder the top-level imports in
src/gateway/streams/hub.rs so standard library imports come first, then external
crates alphabetically, then any local modules; specifically move use
std::sync::Arc; above use pretty_assertions::assert_eq; (and ensure any other
imports follow the same std → external → local ordering and alphabetical within
groups) so the import block follows the project's import ordering guidelines.
In `@src/gateway/streams/reader/aws_event_stream.rs`:
- Line 183: The import for pretty_assertions::assert_eq is out of alphabetical
order; reorder the use statements so external crates are alphabetical (ensure
futures comes before pretty_assertions::assert_eq, and
pretty_assertions::assert_eq comes before serde_json) while keeping standard
library imports first and local modules last to comply with rustfmt/import
sorting rules.
In `@src/gateway/traits/chat_format.rs`:
- Around line 148-152: The import ordering in the test module is incorrect: move
the standard-library import std::borrow::Cow to appear before external-crate
imports like pretty_assertions::assert_eq so std imports come first, then
external crates (alphabetically), then local modules; update the use statements
around HeaderMap and serde_json::json in src/gateway/traits/chat_format.rs
accordingly to follow rustfmt/ repository rules.
In `@src/gateway/types/anthropic.rs`:
- Line 361: Replace usages of assert!(matches!(...)) with
assert_matches::assert_matches for clearer failure diagnostics: add an import
for assert_matches::assert_matches at the top of the test module and change
assertions like the one in anthropic.rs that checks req.system (e.g.,
assert!(matches!(req.system, Some(SystemPrompt::Text(ref s)) if s == "You are
helpful."))) to use assert_matches!(req.system, Some(SystemPrompt::Text(ref s))
if s == "You are helpful."); apply the same conversion to the similar assertions
at the other mentioned locations (lines with checks at 415, 460, 509, and 543)
so all pattern-match assertions use assert_matches!.
In `@src/gateway/types/openai/mod.rs`:
- Line 363: Replace assertions using assert!(matches!(...)) with the
assert_matches::assert_matches macro: change the line asserting the pattern on
req.tool_choice to assert_matches::assert_matches!(req.tool_choice,
Some(ToolChoice::Mode(ref s)) if s == "auto"); do the same for the other
occurrences that match ToolChoice patterns (the assertions at the other noted
locations) so tests use assert_matches::assert_matches instead of
assert!(matches!(...)).
In `@src/gateway/types/openai/responses.rs`:
- Around line 409-413: Add an import for assert_matches (use
assert_matches::assert_matches;) to the test module and replace all pattern
checks written as assert!(matches!(...)) with assert_matches!(<expr>, <pattern>)
calls (e.g., update assertions inside this module that currently use matches! to
use assert_matches!); update the module that already imports
pretty_assertions::assert_eq (alongside serde_json::json and super::*) so all
pattern-matching assertions use assert_matches for improved diagnostics.
In `@src/proxy/hooks/rate_limit/ratelimit/local.rs`:
- Around line 66-67: Reorder the test imports so external crates follow
alphabetical order per rustfmt rules: place http::HeaderMap before
pretty_assertions::assert_eq in the import list; update the use statements
referencing HeaderMap and assert_eq in
src/proxy/hooks/rate_limit/ratelimit/local.rs so external imports are
alphabetized (std first, then external crates alphabetically, then local
modules).
In `@src/proxy/hooks/rate_limit/ratelimit/utils.rs`:
- Around line 258-259: Reorder the test imports so the standard library comes
before external crates: move the std::time::Duration import to appear before
pretty_assertions::assert_eq to follow rustfmt import ordering (standard library
first, then external crates); update the use statements in the module containing
Duration and assert_eq accordingly.
In `@src/utils/instance.rs`:
- Around line 100-101: Move the standard library import "use
std::path::PathBuf;" above the external crate import "use
pretty_assertions::assert_eq;" in src/utils/instance.rs so that std imports come
before external crates (i.e., reorder the two use statements to follow the
repo's rustfmt/rust import ordering rules).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ee9fb01-7532-4b48-ba42-12198c865185
📒 Files selected for processing (40)
src/config/entities/apikeys.rssrc/config/entities/mod.rssrc/config/entities/models.rssrc/config/entities/providers.rssrc/gateway/error.rssrc/gateway/formats/anthropic_messages.rssrc/gateway/formats/openai/mod.rssrc/gateway/gateway.rssrc/gateway/provider_instance.rssrc/gateway/providers/anthropic/mod.rssrc/gateway/providers/anthropic/transform.rssrc/gateway/providers/bedrock/transform.rssrc/gateway/providers/groq.rssrc/gateway/providers/macros.rssrc/gateway/providers/mistral.rssrc/gateway/providers/mod.rssrc/gateway/providers/openai.rssrc/gateway/providers/openrouter.rssrc/gateway/streams/bridged.rssrc/gateway/streams/hub.rssrc/gateway/streams/native.rssrc/gateway/streams/reader/aws_event_stream.rssrc/gateway/streams/reader/sse.rssrc/gateway/traits/chat_format.rssrc/gateway/traits/provider.rssrc/gateway/types/anthropic.rssrc/gateway/types/common.rssrc/gateway/types/embed.rssrc/gateway/types/openai/mod.rssrc/gateway/types/openai/responses.rssrc/proxy/handlers/chat_completions/span_attributes/tests.rssrc/proxy/handlers/messages/types.rssrc/proxy/hooks/rate_limit/concurrent/local.rssrc/proxy/hooks/rate_limit/concurrent/mod.rssrc/proxy/hooks/rate_limit/concurrent/utils.rssrc/proxy/hooks/rate_limit/ratelimit/local.rssrc/proxy/hooks/rate_limit/ratelimit/mod.rssrc/proxy/hooks/rate_limit/ratelimit/utils.rssrc/proxy/provider.rssrc/utils/instance.rs
Summary by CodeRabbit