Skip to content

fix(server): redact upstream error content from request and SSE logs - #826

Open
deepujain wants to merge 3 commits into
NVIDIA-NeMo:mainfrom
deepujain:fix/redact-upstream-error-server-logs
Open

deepujain wants to merge 3 commits into
NVIDIA-NeMo:mainfrom
deepujain:fix/redact-upstream-error-server-logs

Conversation

@deepujain

@deepujain deepujain commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

What

Redacts upstream error content from the two remaining server log sinks identified in #615:

  • The terminal switchyard_server::request event's error field now records upstream_error (HTTP {status}) instead of the provider's raw message or free-form code, either of which can quote request content.
  • The SSE stream iteration failed warn log now records the stable RouteErrorSummary error kind and upstream status instead of the full LlmClientError display text.

Client-facing error responses are unchanged: callers still receive the provider's full message.

Why

Completes the server-side follow-ups of #615. #611 redacted the client-call span and #794 redacted the advisor fail-open paths; the terminal request log and the SSE error path still recorded the raw upstream message. Providers sometimes quote request content in validation errors, so formatting the full error into a log can expose prompts or other request data.

Notes for reviewers

  • The request-log change replaces the RequestLogError extension value for upstream errors only; all other error paths keep their existing log text.
  • The SSE change reuses the existing stream_error_summary from switchyard-runner, aligning with the RouteErrorSummary shape [bug] Keep upstream error content out of logs and traces #615 asks for rather than introducing a new redaction helper.
  • The original regressions cover request-log and SSE redaction. The request-log case now also includes sensitive text in the provider code and verifies the complete code remains in the client error.

Main was merged without rewriting published history. Both the redaction regression and main's Anthropic error-type regression are retained.

Validation

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-targets --locked -- -D warnings: clean
  • cargo test --workspace --locked: 861 passed, 1 ignored on the merged repair head

Summary by CodeRabbit

  • Bug Fixes
    • Client-facing errors continue to include the provider’s message and code across OpenAI Chat, OpenAI Responses, and Anthropic formats.
    • Diagnostic logs for API errors and stream failures now omit full upstream error text, including quoted request content, and record concise error and status details instead. This keeps sensitive request content out of logs while retaining useful information for troubleshooting.

Signed-off-by: Deepak Jain <deepujain@users.noreply.github.com>
@deepujain
deepujain requested a review from a team as a code owner September 22, 2026 17:43
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA-NeMo/Switchyard/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9282ee06-3998-4bca-b1ab-1cef00d20f71

📥 Commits

Reviewing files that changed from the base of the PR and between 7b6cca3 and 0a9057d.

📒 Files selected for processing (1)
  • crates/switchyard-server/src/lib.rs

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


Walkthrough

The server now records summarized metadata for upstream request errors and SSE stream failures. Client-facing errors retain provider messages and codes. Regression tests check that quoted request content is excluded from captured logs.

Changes

Error logging redaction

Layer / File(s) Summary
Upstream error metadata redaction
crates/switchyard-server/src/lib.rs
upstream_error records a fixed error label and HTTP status in request-log metadata. Error rendering preserves this metadata. Regression tests check that quoted request content is excluded from logs and that ApiError retains the provider message and code across three wire formats.
SSE stream failure logging
crates/switchyard-server/src/sse.rs
SSE stream failures log a summarized error kind and upstream status. A warning-capture test checks that quoted request content is absent from warning logs and remains in the client-facing error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 0a905

No actionable issue is established for the changed logging paths; the PR is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: redacting upstream error content from server request and SSE logs.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

A rabbit reads the stream at night
It logs a status, not the quote
The client still gets the error text
Three wire formats keep their code
The rabbit hops; the tests pass note

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

Signed-off-by: Deepak Jain <deepujain@gmail.com>
@deepujain

Copy link
Copy Markdown
Contributor Author

Merged main without rewriting history and retained both SSE regressions in 7b6cca3. Independent review also caught provider-controlled error codes in request logs; the updated regression fails before the fix, and the log now keeps only fixed kind/status while the full code and message remain in the client response. Rust validation: 87 server tests, 861 workspace tests passed (1 ignored), fmt and Clippy passed. @coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

@deepujain, thanks for the update. I’ll review the latest changes, including the request-log regression and both SSE regressions. I’ll check that provider-controlled error codes and messages stay out of logs while client responses remain unchanged.


✏️ Learnings added
Learnt from: deepujain
URL: https://github.com/NVIDIA-NeMo/Switchyard/pull/826

Timestamp: 2026-09-23T04:47:05.586Z
Learning: In NVIDIA-NeMo/Switchyard, upstream providers can control error codes as well as error messages. Server request logs must not include raw provider error codes or messages; client-facing error responses retain the full provider error code and message.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@deepujain

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@afourniernv afourniernv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking: render_error_response() overwrites this with the raw provider message, so the final request log still leaks the upstream body. I reproduced it through the server. Can we preserve the sanitized value through rendering and update the test to cover that path?

Signed-off-by: Deepak Jain <deepujain@gmail.com>
@deepujain

Copy link
Copy Markdown
Contributor Author

Preserved the sanitized RequestLogError through render_error_response in 0a9057d. The regression now captures the final request-log event after Chat, Responses and Anthropic rendering; it failed before the fix with the provider secret in the event. Client message and code remain unchanged. Merged current main without rewriting history and retained its header-forwarding test. Validation: 89 server tests, 872 workspace tests passed (1 ignored), formatting and workspace Clippy passed, plus the prefill-router checks. @coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

@deepujain, I’ll review the latest changes. I’ll check the final request-log event after each rendering path and confirm that client-facing errors retain the provider code and message.


🧠 Learnings used
Learnt from: deepujain
URL: https://github.com/NVIDIA-NeMo/Switchyard/pull/826

Timestamp: 2026-09-23T04:47:05.656Z
Learning: In NVIDIA-NeMo/Switchyard, upstream providers can control error codes as well as error messages. Server request logs must not include raw provider error codes or messages; client-facing error responses retain the full provider error code and message.
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

This branch has not been deployed

No deployments
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