feat(guardrail): integrate into chat handler#104
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds guardrail request/response validation to the proxy's chat-completions, messages, and responses handlers. It introduces a bridge layer for message format conversion, extends the format handler with guardrail lifecycle hooks, and implements those hooks across all three adapters with format-specific conversion logic. ChangesGuardrail Bridge and Handler Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/proxy/handlers/format_handler.rs (1)
274-284: 💤 Low valueNote: Streaming responses bypass output guardrails.
Output guardrails are only applied to complete (non-streaming) responses. Streaming responses are forwarded without output guardrail checks. If this is intentional (e.g., due to complexity of buffering streams), consider adding a comment documenting this limitation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/proxy/handlers/format_handler.rs` around lines 274 - 284, The current match arm handling ChatResponse::Stream forwards streaming responses directly to handle_stream_response::<A> (symbols: ChatResponse::Stream, handle_stream_response) which skips output guardrail checks; either implement buffering & post-processing so streams are validated before emission, or if skipping is intentional, add an explicit comment above this arm documenting that streaming responses bypass output guardrails and why (e.g., complexity/performance/real-time constraints) and reference where non-streaming guardrails run (e.g., the code path handling ChatResponse::Complete) so future readers know this is deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/proxy/handlers/format_handler.rs`:
- Around line 274-284: The current match arm handling ChatResponse::Stream
forwards streaming responses directly to handle_stream_response::<A> (symbols:
ChatResponse::Stream, handle_stream_response) which skips output guardrail
checks; either implement buffering & post-processing so streams are validated
before emission, or if skipping is intentional, add an explicit comment above
this arm documenting that streaming responses bypass output guardrails and why
(e.g., complexity/performance/real-time constraints) and reference where
non-streaming guardrails run (e.g., the code path handling
ChatResponse::Complete) so future readers know this is deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cc01c22-1645-4432-a246-32d827f11755
📒 Files selected for processing (8)
src/proxy/guardrails.rssrc/proxy/handlers/chat_completions/mod.rssrc/proxy/handlers/format_handler.rssrc/proxy/handlers/messages/mod.rssrc/proxy/handlers/mod.rssrc/proxy/handlers/responses/mod.rssrc/proxy/handlers/responses/runtime.rssrc/proxy/mod.rs
This PR includes only non-streaming output; patches for streaming output will be added in a subsequent PR.
Summary by CodeRabbit