Skip to content

fix(proxy): box forward errors for clippy on Rust 1.98 - #417

Closed
r-uben wants to merge 2 commits into
pleaseai:mainfrom
r-uben:fix/forward-error-clippy
Closed

fix(proxy): box forward errors for clippy on Rust 1.98#417
r-uben wants to merge 2 commits into
pleaseai:mainfrom
r-uben:fix/forward-error-clippy

Conversation

@r-uben

@r-uben r-uben commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Box ForwardError across proxy failover, token-count, and Codex forwarding paths so clippy::result_large_err passes on Rust 1.98 CI.
  • Box read_body error responses for the same lint in http_tuning.rs.

Unblocks CI on PRs #370, #410, #411, #414, and #415.

Test plan

  • cargo fmt --all --check
  • cargo clippy --all-targets --all-features -- -D warnings (Rust 1.98)
  • cargo test --all-features --workspace

Made with Cursor


Summary by cubic

Boxes ForwardError and Response in proxy failover, Codex forwarding, and http_tuning to satisfy clippy::result_large_err on Rust 1.98 and restore CI. Previously these functions returned large error types by value; now they return boxed errors. HTTP behavior and metrics remain unchanged.

  • Review focus: verify updated signatures in proxy/failover.rs (forward, count_tokens_response), codex_endpoint.rs (forward), and http_tuning::read_body now return boxed errors and that call sites deref only in tests and response mapping.
  • No migration for external users; internal callers must handle Box<ForwardError> and Box<Response> (tests updated to deref when asserting).

Written for commit 84fe2da. Summary will update on new commits.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request wraps ForwardError and Response in Box within Result types across src/codex_endpoint.rs, src/http_tuning.rs, and src/proxy/failover.rs to address a clippy lint for large error types. The review feedback recommends boxing the large response field inside the ForwardError struct itself rather than boxing the entire struct. This alternative approach resolves the lint at its source, eliminating the need for widespread signature changes and boilerplate code.

Comment thread src/codex_endpoint.rs
@greptile-apps

greptile-apps Bot commented Aug 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR boxes large HTTP and forwarding error values to satisfy Rust 1.98's clippy::result_large_err lint without changing request handling or client-facing errors.

  • Changes proxy failover and token-count forwarding paths to return boxed ForwardError values.
  • Changes the Codex forwarding path to return boxed ForwardError values.
  • Boxes shared read_body error responses and unwraps them when constructing endpoint-specific forwarding errors.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable behavioral, security, or compatibility defects identified.

The changes consistently add heap indirection around existing error values, and every affected caller preserves the original status, response body, logging, and protocol-specific error handling.

Important Files Changed

Filename Overview
src/codex_endpoint.rs Boxes forwarding errors while preserving Codex authentication, body-limit handling, logging, and OpenAI error shaping.
src/http_tuning.rs Boxes body-read error responses and updates focused tests to consume the boxed response.
src/proxy/failover.rs Propagates boxed forwarding errors through routing, authentication, failover, and token-count paths without changing their decisions or responses.

Reviews (1): Last reviewed commit: "fix(http): box read_body error responses..." | Re-trigger Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

Architecture diagram
sequenceDiagram
    participant Client as Client
    participant Proxy as Proxy Service (Failover/Codex)
    participant Tuning as HTTP Tuning
    participant Router as Routing Engine
    participant Upstream as Upstream Provider (e.g. OpenAI)

    Note over Client,Upstream: Request Inbound & Validation Flow

    Client->>Proxy: POST Request (Body, Headers)
    
    Proxy->>Tuning: content_length_exceeds(limit)
    opt NEW: Length Exceeded
        Tuning-->>Proxy: NEW: Err(Box<ForwardError>)
    end

    Proxy->>Tuning: read_body(body, limit)
    alt Body Processing
        Tuning->>Tuning: to_bytes(body)
        Tuning-->>Proxy: Ok(Bytes)
    else NEW: Body Error / Too Large
        Tuning-->>Proxy: NEW: Err(Box<Response>)
    end

    Proxy->>Router: resolve_request_chain_value(json)
    alt Routing Success
        Router-->>Proxy: routes list + model
    else Routing Error
        Router-->>Proxy: NEW: Err(Box<ForwardError>)
    end

    Note over Proxy,Upstream: Forwarding & Failover Loop

    loop For each route in chain
        Proxy->>Upstream: Forward request to provider
        alt Upstream Success (200 OK)
            Upstream-->>Proxy: Response
            Proxy-->>Client: Result<(StatusCode, Response)>
        else Upstream Failure (Retriable)
            Upstream-->>Proxy: 429 / 5xx Error
            Proxy->>Proxy: Attempt next route
        else NEW: Final Failure Path
            Upstream-->>Proxy: Final Error
            Proxy->>Proxy: NEW: Box::new(ForwardError)
            Proxy-->>Client: NEW: Err(Box<ForwardError>)
        end
    end

    Note over Proxy,Upstream: Token Counting Flow (Special Case)

    opt count_tokens requested
        Proxy->>Upstream: count_tokens_response()
        alt NEW: Token Count Error
            Upstream-->>Proxy: Error
            Proxy-->>Client: NEW: Err(Box<ForwardError>)
        end
    end
Loading

Re-trigger cubic

@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.69565% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/proxy/failover.rs 57.69% 11 Missing ⚠️
src/codex_endpoint.rs 42.85% 8 Missing ⚠️

📢 Thoughts on this report? Let us know!

@codspeed-hq

codspeed-hq Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will improve performance by 12.33%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 3 improved benchmarks
✅ 80 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
arc_stored_continuation_retrieve[1048576] 275.8 ns 245.6 ns +12.33%
arc_stored_continuation_retrieve[307200] 275.8 ns 245.6 ns +12.33%
arc_stored_continuation_retrieve[3145728] 275.8 ns 245.6 ns +12.33%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing r-uben:fix/forward-error-clippy (84fe2da) with main (ecf5664)

Open in CodSpeed

@amondnet

Copy link
Copy Markdown
Contributor

Thanks for catching this and for the fix, @r-uben — the CI breakage was real and you diagnosed it correctly.

Unfortunately this one got overtaken: while it was open, #418 landed the same repair from the other direction and is already on main (960802f). Rather than boxing at every call site, it boxes the response field inside both ForwardError types — which is also what @gemini-code-assist suggested in review here:

pub(crate) struct ForwardError {
    message: String,
    response: Box<axum::response::Response>,
}

That mirrors the existing AdapterError idiom, shrinks ForwardError from 152 to 32 bytes, and leaves the function signatures untouched. http_tuning::read_body's Err was boxed the same way, and #418 additionally covered src/proxy.rs, which this PR did not reach.

main at 8a844da is green on fmt · clippy · test under Rust 1.98, so the lint is resolved and this branch now conflicts with main with nothing left to add. Closing as superseded by #418 — no reflection on the work, the timing just went against it.

Really appreciate you unblocking this; hope to see you in the next one. 🙏

@amondnet amondnet closed this Aug 25, 2026
@r-uben

r-uben commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

No argument here — #418 is the better fix. Boxing inside the struct keeps every
signature intact and matches AdapterError, where my version pushed the boxing
out to each call site and still missed src/proxy.rs. Right call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants