Frame XHDR responses with the correct status code - #108
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The change drops multiline handling for XHDR status 225, which can cause mis-framing if a backend returns 225 for XHDR (a behavior also reflected in the repo’s response-code reference).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts NNTP multiline framing rules so XHDR success responses are recognized with the correct status code, ensuring fragmented XHDR replies are routed through the production multiline framer and preserve deferred-reply ordering.
Changes:
- Treat
XHDRsuccess status 221 as a multiline body response (separate fromHDR’s 225 mapping). - Add a regression test to ensure a fragmented
XHDR221response completes correctly before a deferred reply is emitted.
File summaries
| File | Description |
|---|---|
src/session/multiline_framing.rs |
Adds a test covering fragmented XHDR 221 replies and deferred-response ordering through the production framing path. |
src/protocol/request.rs |
Updates the request/status → multiline-body mapping so XHDR is recognized as multiline on 221. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
67610e7 to
e7d9d92
Compare
9bc367d to
61d9a3f
Compare
Summary
Branch-added tests