Conversation
Signed-off-by: Deepak Jain <deepujain@users.noreply.github.com>
|
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 configurationConfiguration used: Repository: NVIDIA-NeMo/Switchyard/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe 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. ChangesError logging redaction
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
A rabbit reads the stream at night Comment |
Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
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 |
|
✏️ Learnings added
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
afourniernv
left a comment
There was a problem hiding this comment.
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>
|
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 |
|
🧠 Learnings used✅ Action performedReview finished.
|
What
Redacts upstream error content from the two remaining server log sinks identified in #615:
switchyard_server::requestevent'serrorfield now recordsupstream_error (HTTP {status})instead of the provider's raw message or free-form code, either of which can quote request content.stream iteration failedwarn log now records the stableRouteErrorSummaryerror kind and upstream status instead of the fullLlmClientErrordisplay 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
RequestLogErrorextension value for upstream errors only; all other error paths keep their existing log text.stream_error_summaryfrom switchyard-runner, aligning with theRouteErrorSummaryshape [bug] Keep upstream error content out of logs and traces #615 asks for rather than introducing a new redaction helper.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: cleancargo clippy --workspace --all-targets --locked -- -D warnings: cleancargo test --workspace --locked: 861 passed, 1 ignored on the merged repair headSummary by CodeRabbit