Repository navigation
fix(wr-agent): refuse a session list whose frame would overflow its envelope - #247
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesSession-list reply limits
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Session-list replies are bounded by both protocol limits, with tests covering the encoded boundary. No material merge risk is identified. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert_eq!( | ||
| sent(vec![0u8; MAX_ENVELOPE_PAYLOAD - HEADER_SIZE]), | ||
| FrameKind::Sessions | ||
| ); | ||
| // And the refusal must itself be sendable, which is the whole point. | ||
| let _ = over.encode(); | ||
| assert_eq!( | ||
| sent(vec![0u8; MAX_ENVELOPE_PAYLOAD - HEADER_SIZE + 1]), | ||
| FrameKind::Failure | ||
| ); | ||
| assert_eq!(sent(vec![0u8; MAX_PAYLOAD_SIZE]), FrameKind::Failure); |
There was a problem hiding this comment.
The production check uses the smaller of the frame and envelope limits, but these assertions assume the envelope limit is always smaller. If the frame limit becomes smaller, the test will fail even though list_reply still behaves correctly. Testing acceptance and refusal around the computed minimum would keep this regression test useful when either limit changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: vcs/crates/wr-agent/src/serve.rs
Line: 1031-1040
Comment:
**Boundary test assumes fixed cap order**
The production check uses the smaller of the frame and envelope limits, but these assertions assume the envelope limit is always smaller. If the frame limit becomes smaller, the test will fail even though `list_reply` still behaves correctly. Testing acceptance and refusal around the computed minimum would keep this regression test useful when either limit changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
🤖 Left by Claude Code
Fixed in 336c51b. The test now derives the boundary from MAX_PAYLOAD_SIZE.min(MAX_ENVELOPE_PAYLOAD - HEADER_SIZE), so it holds whichever cap is smaller. The exact-MAX_PAYLOAD_SIZE case expects a failure only when that cap exceeds the computed limit.
…nvelope list_reply checked the encoded list against the frame cap only. The frame's 5-byte header then pushed a list just under 1 MiB past the envelope cap, and Envelope::encode's assert panicked the connection's thread. Cap the list at the smaller of the frame cap and what an envelope leaves room for, and test it the way dispatch sends it: inside an envelope. Fixes #246 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the two caps Greptile noted the boundary assertions assumed the envelope cap is always the smaller one. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
a67c022 to
336c51b
Compare
🤖 Left by Claude Code
Fixes #246. Part of #154 (Phase 3 follow-ups).
What changes
list_replychecked an encoded session list against the frame cap only (MAX_PAYLOAD_SIZE, 1 MiB).dispatchthen wraps the frame in an envelope. The frame's 5-byte header counts against the envelope's own 1 MiB cap. So a list between 1 MiB − 4 and 1 MiB bytes passedlist_replyand then tripped the assert inEnvelope::encode. That assert panicked the thread serving the connection. Since #245, that connection is the app's shared service connection to a host, so the panic also ended its VCS, File, Status and Forward traffic.The list is now capped at
MAX_PAYLOAD_SIZE.min(MAX_ENVELOPE_PAYLOAD - HEADER_SIZE). The two caps are equal today, and taking the smaller one keeps the check right if either cap changes.Listis the only reply on that path whose size has no fixed bound: output and repaints are chunked, and hand-off reasons are truncated. So this one check covers every caller.The largest reply now allowed is an envelope of exactly 1 MiB. Both decoders accept that: the Rust one rejects only
length > MAX_ENVELOPE_PAYLOAD, and the app's Swift decoder acceptslength <= 1 << 20.Test
an_oversized_session_list_is_refused_rather_than_panickingnow wraps each reply in an envelope, the waydispatchsends it. It checks these sizes:Before the fix, the updated test failed with the reported panic:
envelope payload 1048577 exceeds the 1048576-byte cap.Validation
cargo test -p wr-agent: all pass (266 unit tests, plus the integration suites). Run outside the command sandbox, because the sandbox blocksforkpty.cargo fmt --all -- --check: clean.cargo clippy --workspace --all-targets -- -D warnings: clean.cargo clippy -p wr-agent --features terminal-state --all-targets -- -D warnings: clean.No Swift or app changes.
🤖 Generated with Claude Code
Summary by CodeRabbit