feat(hook): move authz to direct func call#34
Merged
Conversation
📝 WalkthroughWalkthroughHandlers now accept a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant RequestCtx as RequestContext
participant Auth as authorization::check
participant AppState
participant HookMgr as HOOK_MANAGER
participant Provider
Client->>Handler: HTTP request (includes model)
Handler->>RequestCtx: Extract RequestContext (contains ApiKey)
Handler->>Auth: check(&mut RequestContext, model_name)
Auth->>AppState: lookup model by name
alt Model not found
Auth-->>Handler: AuthorizationError::ModelNotFound
else Model found
Auth->>RequestCtx: get ApiKey from extensions
alt ApiKey missing
Auth-->>Handler: AuthorizationError::MissingApiKeyInContext
else ApiKey present
Auth->>AppState: verify ApiKey.allowed_models contains model
alt Access forbidden
Auth-->>Handler: AuthorizationError::AccessForbidden
else Access granted
Auth->>RequestCtx: insert resolved model & RequestModel(model_name)
Auth-->>Handler: Ok(())
Handler->>HookMgr: HOOK_MANAGER.pre_call(...)
HookMgr-->>Handler: pre_call results
Handler->>Provider: send downstream request
Provider-->>Handler: provider response
Handler-->>Client: return response
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/proxy/handlers/chat_completions/mod.rs (1)
38-45:⚠️ Potential issue | 🔴 CriticalCritical bug: Missing
RequestModelinsertion will cause MetricHook to panic at runtime.The
MetricHookexpectsmiddlewares::RequestModelinhook_ctx(metric.rs:28, calls.expect("RequestModel should be in context")). However,authorization::checkinsertshooks2::authorization::RequestModel(different type due to separate struct definitions), and this handler doesn't manually insert the expected type.In
embeddings/mod.rs:35, afterauthorization::check, there's an explicithook_ctx.insert(RequestModel(...))that works around this type mismatch. Without this insertion inchat_completions, the metric hook will panic whenHOOK_MANAGER.pre_callexecutes.Fix: Insert
hook_ctx.insert(RequestModel(request_data.model))afterauthorization::checkand beforeHOOK_MANAGER.pre_call, matching the pattern in embeddings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/handlers/chat_completions/mod.rs` around lines 38 - 45, After authorization::check(&mut request_ctx, request_data.model.clone()).await? insert the expected middleware type into hook_ctx so MetricHook won't panic: create and insert the local RequestModel (the middlewares::RequestModel type used by MetricHook) from request_data.model (i.e., call hook_ctx.insert(RequestModel(request_data.model)) ) before calling HOOK_MANAGER.pre_call(&mut hook_ctx, ...); ensure you use the same RequestModel type that MetricHook expects and perform the insertion on the same hook_ctx passed to HOOK_MANAGER.pre_call.
🧹 Nitpick comments (6)
src/proxy/handlers/embeddings/mod.rs (2)
26-35: Add#[fastrace::trace]decorator for distributed tracing.Per coding guidelines, public functions should have the
#[fastrace::trace]decorator. Thechat_completionshandler has it, butembeddingsis missing it.Proposed fix
+#[fastrace::trace] pub async fn embeddings( State(_state): State<AppState>, mut hook_ctx: HookContext, mut request_ctx: RequestContext, Json(mut request_data): Json<EmbeddingRequest>, ) -> Result<Response, EmbeddingError> {As per coding guidelines: "Use #[fastrace::trace] decorator for distributed tracing on public functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/handlers/embeddings/mod.rs` around lines 26 - 35, The public handler function embeddings is missing the required #[fastrace::trace] attribute for distributed tracing; add the #[fastrace::trace] decorator immediately above the pub async fn embeddings(...) declaration so it matches other handlers (e.g., chat_completions) and enables fastrace instrumentation for this function.
32-35:RequestModelis inserted into bothrequest_ctx(byauthorization::check) andhook_ctx(Line 35).Looking at
authorization::checkinsrc/proxy/hooks2/authorization/mod.rs:78-79, it insertsRequestModelinto the context. Here, it's also inserted intohook_ctx. This duplication may be intentional for backward compatibility with hooks, but consider documenting this or consolidating if hooks can read fromrequest_ctx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/handlers/embeddings/mod.rs` around lines 32 - 35, Duplicate insertion of RequestModel occurs because authorization::check already inserts RequestModel into request_ctx; embeddings handler also inserts RequestModel into hook_ctx. Remove the redundant hook_ctx.insert(RequestModel(...)) in the embeddings handler or, if backward compatibility for hooks requires it, add a clarifying comment documenting that authorization::check inserts RequestModel into request_ctx and that the explicit hook_ctx.insert(RequestModel(...)) is intentional to expose the model to legacy hooks; update embeddings/mod.rs around the PRE CALL HOOKS block and reference authorization::check and RequestModel in the comment.src/proxy/mod.rs (1)
3-3: Consider a more descriptive module name.The name
hooks2suggests this is a transitional/versioned module. Once the migration fromhooksis complete, consider renaming to something more descriptive likeauthzorrequest_context, or consolidating with the existinghooksmodule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/mod.rs` at line 3, The module name hooks2 in src/proxy/mod.rs is non-descriptive; rename the module to a clear name (e.g., authz or request_context) or merge its contents into the existing hooks module: update the mod declaration (mod hooks2 -> mod authz or mod request_context or remove and re-export items from hooks), adjust any uses/imports that reference hooks2 to the new module name (search for hooks2:: symbols) and ensure Cargo builds and tests pass after the rename/merge.src/proxy/hooks2/authorization/mod.rs (3)
13-14: Consider removing#[allow(unused)]or adding accessor.The
#[allow(unused)]attribute on the inner field suggests the value is stored but never read. If the model name will be needed later, consider adding a public accessor method. Otherwise, if it's truly unused, reconsider whether storing it is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/authorization/mod.rs` around lines 13 - 14, The RequestModel tuple struct currently has an inner field annotated with #[allow(unused)] which hides that the stored String is never read; either remove the attribute and stop storing the value if it truly isn't needed, or add a public accessor (e.g., impl RequestModel { pub fn name(&self) -> &str }) so the inner String is actually used; update all call sites that construct or need the model name to use the new accessor or stop passing the value when it's unnecessary.
53-55: Inconsistent error response forMissingApiKeyInContext.This variant returns only a status code without a JSON body, unlike the other error variants. For API consistency and easier debugging by clients, consider returning a structured JSON error response.
Proposed fix
AuthorizationError::MissingApiKeyInContext => { - (StatusCode::INTERNAL_SERVER_ERROR).into_response() + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ + "error": { + "message": "Internal server error", + "type": "internal_error", + "code": "internal_error" + } + })), + ) + .into_response() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/authorization/mod.rs` around lines 53 - 55, AuthorizationError::MissingApiKeyInContext currently returns only (StatusCode::INTERNAL_SERVER_ERROR).into_response(), making it inconsistent with other variants; update the match arm for AuthorizationError::MissingApiKeyInContext to return the same structured JSON error response used by the other variants (include a clear error message and the INTERNAL_SERVER_ERROR status), e.g. build the same error body type/serde structure and call into_response() with that body instead of a bare StatusCode so clients receive a consistent JSON payload.
69-69: Unnecessary.clone()calls.On lines 69 and 83,
model_name.clone()is unnecessary since the function returns immediately after andmodel_nameis already owned. You can movemodel_namedirectly into the error.Proposed fix
let model = match state.resources().models.get_by_name(&model_name) { Some(model) => model, None => { - return Err(AuthorizationError::ModelNotFound(model_name.clone())); + return Err(AuthorizationError::ModelNotFound(model_name)); } };For line 83, since you still need
model_nameafter the check for insertingRequestModel, you can restructure:// Check if API key has access to this model if !api_key.allowed_models.contains(&model_name) { - return Err(AuthorizationError::AccessForbidden(model_name.clone())); + return Err(AuthorizationError::AccessForbidden(model_name)); }Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/authorization/mod.rs` at line 69, Remove the unnecessary .clone() on model_name when returning the error: change return Err(AuthorizationError::ModelNotFound(model_name.clone())) to return Err(AuthorizationError::ModelNotFound(model_name)) so you move ownership directly into AuthorizationError::ModelNotFound; for the other occurrence around the RequestModel insertion (the second clone at line 83) restructure the code so you either borrow model_name for the insert (e.g., use &model_name for RequestModel::insert) or take ownership earlier (e.g., move model_name into a variable only when needed) so you don't need to clone—ensure AuthorizationError::ModelNotFound receives the owned model_name when you return the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/proxy/hooks2/authorization/mod.rs`:
- Line 1: Remove the unused import `use anyhow::Result;` from the top of the
file; the `check` function returns `Result<(), AuthorizationError>` using the
standard `std::result::Result`, so delete the `anyhow::Result` import to avoid
the unused import warning and keep imports accurate (look for the `check`
function and the `AuthorizationError` type in this module to confirm).
- Around line 60-64: Add the #[fastrace::trace] attribute to the public function
check and replace the panic-prone expect() when retrieving AppState from
RequestContext with a safe error return: add a new AuthorizationError variant
(e.g., MissingAppStateInContext) and change the retrieval in check (the
RequestContext.get::<AppState>().cloned() call) to return
Err(AuthorizationError::MissingAppStateInContext) when AppState is absent
instead of calling expect; keep the rest of check's logic unchanged.
In `@src/proxy/hooks2/context.rs`:
- Around line 11-32: RequestContext::from_request_parts currently stores
AppState in the struct field but never inserts it into the http::Extensions so
authorization::check (which does ctx.get::<AppState>()) will panic; fix by
inserting a clone of the AppState into the extensions before returning (e.g.,
call ctx.insert(state.clone()) or otherwise insert AppState into ctx inside
from_request_parts), then construct RequestContext with that extensions value so
ctx.get::<AppState>() succeeds in authorization::check.
---
Outside diff comments:
In `@src/proxy/handlers/chat_completions/mod.rs`:
- Around line 38-45: After authorization::check(&mut request_ctx,
request_data.model.clone()).await? insert the expected middleware type into
hook_ctx so MetricHook won't panic: create and insert the local RequestModel
(the middlewares::RequestModel type used by MetricHook) from request_data.model
(i.e., call hook_ctx.insert(RequestModel(request_data.model)) ) before calling
HOOK_MANAGER.pre_call(&mut hook_ctx, ...); ensure you use the same RequestModel
type that MetricHook expects and perform the insertion on the same hook_ctx
passed to HOOK_MANAGER.pre_call.
---
Nitpick comments:
In `@src/proxy/handlers/embeddings/mod.rs`:
- Around line 26-35: The public handler function embeddings is missing the
required #[fastrace::trace] attribute for distributed tracing; add the
#[fastrace::trace] decorator immediately above the pub async fn embeddings(...)
declaration so it matches other handlers (e.g., chat_completions) and enables
fastrace instrumentation for this function.
- Around line 32-35: Duplicate insertion of RequestModel occurs because
authorization::check already inserts RequestModel into request_ctx; embeddings
handler also inserts RequestModel into hook_ctx. Remove the redundant
hook_ctx.insert(RequestModel(...)) in the embeddings handler or, if backward
compatibility for hooks requires it, add a clarifying comment documenting that
authorization::check inserts RequestModel into request_ctx and that the explicit
hook_ctx.insert(RequestModel(...)) is intentional to expose the model to legacy
hooks; update embeddings/mod.rs around the PRE CALL HOOKS block and reference
authorization::check and RequestModel in the comment.
In `@src/proxy/hooks2/authorization/mod.rs`:
- Around line 13-14: The RequestModel tuple struct currently has an inner field
annotated with #[allow(unused)] which hides that the stored String is never
read; either remove the attribute and stop storing the value if it truly isn't
needed, or add a public accessor (e.g., impl RequestModel { pub fn name(&self)
-> &str }) so the inner String is actually used; update all call sites that
construct or need the model name to use the new accessor or stop passing the
value when it's unnecessary.
- Around line 53-55: AuthorizationError::MissingApiKeyInContext currently
returns only (StatusCode::INTERNAL_SERVER_ERROR).into_response(), making it
inconsistent with other variants; update the match arm for
AuthorizationError::MissingApiKeyInContext to return the same structured JSON
error response used by the other variants (include a clear error message and the
INTERNAL_SERVER_ERROR status), e.g. build the same error body type/serde
structure and call into_response() with that body instead of a bare StatusCode
so clients receive a consistent JSON payload.
- Line 69: Remove the unnecessary .clone() on model_name when returning the
error: change return Err(AuthorizationError::ModelNotFound(model_name.clone()))
to return Err(AuthorizationError::ModelNotFound(model_name)) so you move
ownership directly into AuthorizationError::ModelNotFound; for the other
occurrence around the RequestModel insertion (the second clone at line 83)
restructure the code so you either borrow model_name for the insert (e.g., use
&model_name for RequestModel::insert) or take ownership earlier (e.g., move
model_name into a variable only when needed) so you don't need to clone—ensure
AuthorizationError::ModelNotFound receives the owned model_name when you return
the error.
In `@src/proxy/mod.rs`:
- Line 3: The module name hooks2 in src/proxy/mod.rs is non-descriptive; rename
the module to a clear name (e.g., authz or request_context) or merge its
contents into the existing hooks module: update the mod declaration (mod hooks2
-> mod authz or mod request_context or remove and re-export items from hooks),
adjust any uses/imports that reference hooks2 to the new module name (search for
hooks2:: symbols) and ensure Cargo builds and tests pass after the rename/merge.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7956c983-0484-40dc-a1ad-755438c9e292
📒 Files selected for processing (11)
src/proxy/handlers/chat_completions/mod.rssrc/proxy/handlers/chat_completions/types.rssrc/proxy/handlers/embeddings/mod.rssrc/proxy/handlers/embeddings/types.rssrc/proxy/handlers/models.rssrc/proxy/hooks/mod.rssrc/proxy/hooks/validate_model.rssrc/proxy/hooks2/authorization/mod.rssrc/proxy/hooks2/context.rssrc/proxy/hooks2/mod.rssrc/proxy/mod.rs
💤 Files with no reviewable changes (2)
- src/proxy/hooks/validate_model.rs
- src/proxy/hooks/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/proxy/hooks2/context.rs (2)
49-52: Missing#[fastrace::trace]on public function.Per coding guidelines, public functions in
src/**/*.rsshould use#[fastrace::trace]for distributed tracing. While this is a simple accessor, adding the attribute maintains consistency.Suggested change
impl RequestContext { + #[fastrace::trace] pub fn app_state(&self) -> &AppState { &self.app_state } }As per coding guidelines: "Use #[fastrace::trace] decorator for distributed tracing on public functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/context.rs` around lines 49 - 52, The public accessor RequestContext::app_state is missing the #[fastrace::trace] attribute required by our tracing guideline; add the #[fastrace::trace] attribute directly above the impl method definition for app_state (the pub fn app_state(&self) -> &AppState function) so the function is instrumented for distributed tracing and remains otherwise unchanged.
17-32: TheRejection = ()type is unused due to theexpect()panic.The extractor defines
type Rejection = ()suggesting it might return an error, but the implementation panics viaexpect()on line 25-27 ifResourceEntry<ApiKey>is missing. This is inconsistent—either handle the missing extension gracefully by returningErr(())(though()provides no useful error info), or useInfallibleas the rejection type to signal that extraction never returnsErr.If the intention is to panic on missing API key (treating it as a programmer error), consider using
Infallible:Suggested change
impl FromRequestParts<AppState> for RequestContext { - type Rejection = (); + type Rejection = std::convert::Infallible;Alternatively, if graceful handling is preferred, return a proper error response type instead of
().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/context.rs` around lines 17 - 32, The extractor currently declares type Rejection = () but panics via expect() when ResourceEntry<ApiKey> is missing in from_request_parts, which is inconsistent; either make the extractor genuinely infallible by changing type Rejection to std::convert::Infallible and keep the expect (or otherwise assert) semantics, or handle the missing extension gracefully by removing the expect() call and returning Err(()) (or better, replace () with a meaningful error type) from from_request_parts; update the signature and control flow in FromRequestParts<AppState> for RequestContext and adjust the code around parts.extensions.remove::<ResourceEntry<ApiKey>>() and the expect() accordingly to match the chosen approach.src/proxy/hooks2/authorization/mod.rs (2)
64-66: Minor: Unnecessaryclone()on early return.Since this branch returns immediately and
model_nameisn't used afterward in this path, you can movemodel_namedirectly into the error without cloning:Suggested change
None => { - return Err(AuthorizationError::ModelNotFound(model_name.clone())); + return Err(AuthorizationError::ModelNotFound(model_name)); }Note: The
clone()on line 79 is necessary sincemodel_nameis used later on line 83.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/authorization/mod.rs` around lines 64 - 66, The None branch unnecessarily clones model_name when returning the error; replace the clone by moving model_name into AuthorizationError::ModelNotFound so the early return consumes model_name (i.e., use AuthorizationError::ModelNotFound(model_name) in the None arm of the match), leaving the later clone at the other call site intact where model_name is still needed.
53-55: Inconsistent error response body forMissingApiKeyInContext.The other error variants return structured JSON bodies with
error.message,error.type, anderror.codefields, butMissingApiKeyInContextreturns only a bare500status with no body. For API consistency (and to help debugging), consider returning a similar structure:Suggested change
AuthorizationError::MissingApiKeyInContext => { - (StatusCode::INTERNAL_SERVER_ERROR).into_response() + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ + "error": { + "message": self.to_string(), + "type": "internal_error", + "code": "internal_error" + } + })), + ) + .into_response() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/authorization/mod.rs` around lines 53 - 55, AuthorizationError::MissingApiKeyInContext currently returns only (StatusCode::INTERNAL_SERVER_ERROR).into_response(), creating an inconsistent bare 500 response; change the handler for AuthorizationError::MissingApiKeyInContext to return the same structured JSON body used by the other variants (include error.message, error.type, error.code) and the INTERNAL_SERVER_ERROR status—e.g., construct the same error response object/struct used elsewhere and convert it into a response instead of returning a bare status; update the match arm for MissingApiKeyInContext to build and return that structured JSON response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/proxy/hooks2/authorization/mod.rs`:
- Around line 64-66: The None branch unnecessarily clones model_name when
returning the error; replace the clone by moving model_name into
AuthorizationError::ModelNotFound so the early return consumes model_name (i.e.,
use AuthorizationError::ModelNotFound(model_name) in the None arm of the match),
leaving the later clone at the other call site intact where model_name is still
needed.
- Around line 53-55: AuthorizationError::MissingApiKeyInContext currently
returns only (StatusCode::INTERNAL_SERVER_ERROR).into_response(), creating an
inconsistent bare 500 response; change the handler for
AuthorizationError::MissingApiKeyInContext to return the same structured JSON
body used by the other variants (include error.message, error.type, error.code)
and the INTERNAL_SERVER_ERROR status—e.g., construct the same error response
object/struct used elsewhere and convert it into a response instead of returning
a bare status; update the match arm for MissingApiKeyInContext to build and
return that structured JSON response.
In `@src/proxy/hooks2/context.rs`:
- Around line 49-52: The public accessor RequestContext::app_state is missing
the #[fastrace::trace] attribute required by our tracing guideline; add the
#[fastrace::trace] attribute directly above the impl method definition for
app_state (the pub fn app_state(&self) -> &AppState function) so the function is
instrumented for distributed tracing and remains otherwise unchanged.
- Around line 17-32: The extractor currently declares type Rejection = () but
panics via expect() when ResourceEntry<ApiKey> is missing in from_request_parts,
which is inconsistent; either make the extractor genuinely infallible by
changing type Rejection to std::convert::Infallible and keep the expect (or
otherwise assert) semantics, or handle the missing extension gracefully by
removing the expect() call and returning Err(()) (or better, replace () with a
meaningful error type) from from_request_parts; update the signature and control
flow in FromRequestParts<AppState> for RequestContext and adjust the code around
parts.extensions.remove::<ResourceEntry<ApiKey>>() and the expect() accordingly
to match the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbaed441-f1e8-4bec-babd-921d7d2fd765
📒 Files selected for processing (2)
src/proxy/hooks2/authorization/mod.rssrc/proxy/hooks2/context.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/proxy/handlers/embeddings/mod.rs (1)
32-35:RequestModelis inserted into both contexts during migration.
authorization::checkinsertsRequestModelintorequest_ctx(seesrc/proxy/hooks2/authorization/mod.rs:83), while line 35 also inserts it intohook_ctx. This duplication appears intentional during the migration from the old hook system to the newhooks2architecture.Consider adding a TODO comment to clarify this is temporary and should be cleaned up once the migration is complete.
Proposed clarification
authorization::check(&mut request_ctx, request_data.model.clone()).await?; // PRE CALL HOOKS START + // TODO: Remove this insertion once hooks2 migration is complete - authorization::check already inserts RequestModel into request_ctx hook_ctx.insert(RequestModel(request_data.model));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/handlers/embeddings/mod.rs` around lines 32 - 35, authorization::check currently inserts RequestModel into request_ctx and the code then inserts RequestModel again into hook_ctx (see authorization::check, RequestModel, request_ctx, hook_ctx) as part of the hooks2 migration; add a concise TODO comment next to the hook_ctx.insert(RequestModel(...)) call stating this duplication is intentional and temporary for migration and should be removed once hooks2 fully replaces the old hook system (include reference to the migration and ticket/issue if available).src/proxy/hooks2/context.rs (2)
11-15: Remove unnecessary#[allow(unused)]attribute.The
app_statefield is used by theapp_state()method on line 56-58, so the#[allow(unused)]annotation is not needed.Proposed fix
pub struct RequestContext { - #[allow(unused)] app_state: AppState, extensions: http::Extensions, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/context.rs` around lines 11 - 15, The #[allow(unused)] attribute on the RequestContext.app_state field is unnecessary because the field is used by the app_state() method; remove the #[allow(unused)] annotation from the app_state field declaration in the RequestContext struct so the field is declared simply as app_state: AppState while keeping the rest of the struct (extensions: http::Extensions) and the app_state() method unchanged.
55-59: Consider adding#[fastrace::trace]decorator for distributed tracing.As per coding guidelines, public functions in
src/**/*.rsshould use#[fastrace::trace]decorator for distributed tracing.Proposed fix
impl RequestContext { + #[fastrace::trace] pub fn app_state(&self) -> &AppState { &self.app_state } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/context.rs` around lines 55 - 59, Add the #[fastrace::trace] attribute to the public accessor RequestContext::app_state so it participates in distributed tracing; place the attribute directly above the pub fn app_state(&self) -> &AppState declaration (and ensure the fastrace crate is available in dependencies/imports so the attribute macro resolves).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/proxy/hooks2/context.rs`:
- Line 33: Fix the typo in the TODO comment: find the comment "TODO: remove
instand of clone" and change "instand" to "instead" so it reads "TODO: remove
instead of clone"; leave the surrounding code unchanged.
---
Nitpick comments:
In `@src/proxy/handlers/embeddings/mod.rs`:
- Around line 32-35: authorization::check currently inserts RequestModel into
request_ctx and the code then inserts RequestModel again into hook_ctx (see
authorization::check, RequestModel, request_ctx, hook_ctx) as part of the hooks2
migration; add a concise TODO comment next to the
hook_ctx.insert(RequestModel(...)) call stating this duplication is intentional
and temporary for migration and should be removed once hooks2 fully replaces the
old hook system (include reference to the migration and ticket/issue if
available).
In `@src/proxy/hooks2/context.rs`:
- Around line 11-15: The #[allow(unused)] attribute on the
RequestContext.app_state field is unnecessary because the field is used by the
app_state() method; remove the #[allow(unused)] annotation from the app_state
field declaration in the RequestContext struct so the field is declared simply
as app_state: AppState while keeping the rest of the struct (extensions:
http::Extensions) and the app_state() method unchanged.
- Around line 55-59: Add the #[fastrace::trace] attribute to the public accessor
RequestContext::app_state so it participates in distributed tracing; place the
attribute directly above the pub fn app_state(&self) -> &AppState declaration
(and ensure the fastrace crate is available in dependencies/imports so the
attribute macro resolves).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9beb7804-38d3-46a1-8451-7cf204e9b3af
📒 Files selected for processing (5)
src/proxy/handlers/chat_completions/mod.rssrc/proxy/handlers/embeddings/mod.rssrc/proxy/handlers/models.rssrc/proxy/hooks2/context.rssrc/proxy/middlewares/auth.rs
💤 Files with no reviewable changes (1)
- src/proxy/middlewares/auth.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/proxy/handlers/chat_completions/mod.rs
- src/proxy/handlers/models.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/proxy/hooks2/context.rs (1)
11-15: Add rustdoc for the publicRequestContexttype.Line 11 defines a public item without
///docs.Suggested patch
+/// Per-request context used by proxy hooks and handlers. pub struct RequestContext {As per coding guidelines, "Use /// for doc comments on public items in Rust".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/hooks2/context.rs` around lines 11 - 15, RequestContext is a public type with no rustdoc; add a /// doc comment above the pub struct RequestContext that briefly describes its purpose (e.g., that it carries per-request state and extensions for proxy handling) and document the important fields by mentioning app_state and extensions in the docs; use /// style doc comments and keep the description concise and aligned with coding guidelines for public items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/proxy/hooks2/context.rs`:
- Around line 55-58: Add a Rust doc comment describing what
RequestContext::app_state returns (the shared AppState reference) and annotate
the public accessor with the synchronous tracing attribute from the fastrace
crate (e.g. #[fastrace::instrument]) per coding guidelines; update the signature
for pub fn app_state(&self) -> &AppState in RequestContext to include the doc
comment above it and the #[fastrace::instrument] attribute immediately above the
fn so the method is documented and traced.
---
Nitpick comments:
In `@src/proxy/hooks2/context.rs`:
- Around line 11-15: RequestContext is a public type with no rustdoc; add a ///
doc comment above the pub struct RequestContext that briefly describes its
purpose (e.g., that it carries per-request state and extensions for proxy
handling) and document the important fields by mentioning app_state and
extensions in the docs; use /// style doc comments and keep the description
concise and aligned with coding guidelines for public items.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef392f00-00b1-4246-81e1-bd6779808ecb
📒 Files selected for processing (3)
src/proxy/handlers/chat_completions/mod.rssrc/proxy/handlers/embeddings/mod.rssrc/proxy/hooks2/context.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/proxy/handlers/embeddings/mod.rs
- src/proxy/handlers/chat_completions/mod.rs
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.
The current hook was over-engineered (from today’s perspective) as a framework, but we don’t actually need a full-fledged plugin system based on global hooks.
Specifically, the old hook system used a global hook list that allowed any hook implementation conforming to the
Hooktrait to be registered with the system in a fixed order; whenever a request arrived, a filter closure function would determine whether the functions in theHooktrait should be executed.Admittedly, this structure offered some flexibility and had the potential for further evolution. However, it also had limitations, such as the inability to dynamically determine the execution order of hooks and the coupling of the
Hooktrait with other data structures.We will now split hooks by domain and provide internal abstractions on demand in the future. Any specific hook function will be reduced to a direct function call, with the resolution of abstraction and replacement of implementations handled internally.
For example, the handler explicitly calls the
rate_limit::pre_check()function, which may then determine the specific strategy to execute (local or Redis) based on configuration files or other factors.Summary by CodeRabbit
New Features
Bug Fixes
Refactor