fix(llm-client): drop upstream urls from transport errors - #649
fix(llm-client): drop upstream urls from transport errors#649afourniernv wants to merge 1 commit into
Conversation
Signed-off-by: Alex Fournier <afournier@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe client now sanitizes ChangesTransport Error Redaction
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
A rabbit watched the error trail, Comment |
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_urlcontaining?key=..., that response returns the configured key to the caller.I reproduced this on untouched
mainwith a closed local port: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::TransportandLlmClientError::Timeoutremain typed errors with their existing sources.This uses the
reqwest::Error::without_url()approach from Ryan's draft #587. The original server-side investigation is in #428.How tested
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspaceuv run ruff check .uv run mypy switchyarduv run pytest tests/— 117 passedNotes 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.