Skip to content

fix(llm-client): drop upstream urls from transport errors - #649

Open
afourniernv wants to merge 1 commit into
mainfrom
codex/sygh-64-redact-transport-url
Open

fix(llm-client): drop upstream urls from transport errors#649
afourniernv wants to merge 1 commit into
mainfrom
codex/sygh-64-redact-transport-url

Conversation

@afourniernv

@afourniernv afourniernv commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What

Removes the upstream request URL from transport and timeout errors before they leave switchyard-llm-client.

The same conversion now covers buffered requests and streaming body failures. The server's raw fallback proxy applies the same protection because it sends requests without going through switchyard-llm-client.

Why

Fixes SYGH-64 and closes #423.

A failed upstream request currently returns the full reqwest URL in the client-visible 502. If a deployment uses a Gemini-style base_url containing ?key=..., that response returns the configured key to the caller.

I reproduced this on untouched main with a closed local port:

error sending request for url (http://127.0.0.1:1/v1?key=SYGH64_CANARY/chat/completions)

After this change, the response keeps the existing status, type, and code without the URL:

{
  "error": {
    "message": "error sending request",
    "type": "upstream_error",
    "code": "upstream_error"
  }
}

Behavior

  • LlmClientError::Transport and LlmClientError::Timeout remain typed errors with their existing sources.
  • The attached reqwest error no longer carries the upstream URL.
  • HTTP status codes, error codes, retries, and fallback behavior are unchanged.
  • No public APIs or dependencies change.

This uses the reqwest::Error::without_url() approach from Ryan's draft #587. The original server-side investigation is in #428.

How tested

  • Live local reproduction for OpenAI Chat, streaming Chat, OpenAI Responses, Anthropic Messages, and the raw fallback proxy. Every request returned 502 without the canary.
  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace
  • uv run ruff check .
  • uv run mypy switchyard
  • uv run pytest tests/ — 117 passed
  • Commit carries the required DCO sign-off.

Notes for reviewers

The production change is five added lines across the shared reqwest error conversion and the raw fallback path. The remaining additions are regression coverage for both client-visible 502 paths.

Signed-off-by: Alex Fournier <afournier@nvidia.com>
@afourniernv
afourniernv requested a review from a team as a code owner September 9, 2026 04:02
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9b0d2ec2-938c-436e-a8d3-7e91540f756c

📥 Commits

Reviewing files that changed from the base of the PR and between 98df182 and 71cc59c.

📒 Files selected for processing (3)
  • crates/libsy-llm-client/src/client.rs
  • crates/switchyard-server/src/lib.rs
  • crates/switchyard-server/tests/server.rs

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The client now sanitizes reqwest transport errors before propagation. The server applies the same URL redaction to fallback errors. Tests verify that credential-bearing query parameters do not appear in client or server error responses.

Changes

Transport Error Redaction

Layer / File(s) Summary
Client transport error sanitization
crates/libsy-llm-client/src/client.rs
Streaming errors use convert_reqwest_error, which removes the request URL before creating typed errors. A regression test checks that query credentials are omitted.
Server error response sanitization
crates/switchyard-server/src/lib.rs, crates/switchyard-server/tests/server.rs
Fallback errors use URL-redacted text. Integration coverage checks routed and fallback 502 Bad Gateway responses.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Severity of issue fixed: High

Merge Risk: ⚪ Minimal · up to 71cc5

Transport and streaming failures now omit upstream URLs from client-visible errors while preserving 502 behavior. Routed and fallback credential-redaction cases are covered, with no current merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address issue #423 by removing upstream URLs before transport errors propagate, including streaming client failures and fallback proxy errors. The added tests verify that routed and fallba…
Out of Scope Changes check ✅ Passed All changes are directly related to preventing upstream URLs and embedded credentials from appearing in transport errors and 502 responses. The regression tests support the linked issue objective.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: removing upstream URLs from LLM client transport errors.
  • Fix all pre-merge checks with AI

A rabbit watched the error trail,
Then hid the key inside the veil.
The stream stayed clean, the gateway bright,
Two failing paths concealed the bite.
No query secret hops in sight.

Comment @coderabbitai help to get the list of available commands.

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.

[bug]: transport 502 echoes base_url including ?key=

1 participant