feat(cli): expose sealed WebSocket sessions - #25
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe CLI now supports finite WebSocket sources through inspection, planning, grant-controlled invocation, transcript and payload evidence, and documented terminal outcomes. The plan library adds operation inspection, identity validation, filtering, pagination, and risk metadata. ChangesWebSocket CLI support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant StoredPlan
participant execute_websocket
participant WebSocketServer
CLI->>StoredPlan: Load WebSocket plan
CLI->>execute_websocket: Invoke with grants and policy fingerprint
execute_websocket->>WebSocketServer: Connect and exchange bounded actions
execute_websocket-->>CLI: Return observations, denials, and exit status
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/kahea-plan/src/lib.rs (2)
2501-2510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
cursor == countboundary case.The test covers
cursor = 2with one operation, which fails. It does not covercursor = 1, which the implementation accepts and returns an empty page for. Add that assertion so the accepted cursor range stays pinned.💚 Proposed addition
+ let exhausted = inspect_websocket_session( + Path::new("session.json"), + include_bytes!("../../../fixtures/websocket/session.json"), + None, + 50, + 1, + ) + .unwrap(); + assert!(exhausted.operations.is_empty()); assert!(matches!( inspect_websocket_session(🤖 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 `@crates/kahea-plan/src/lib.rs` around lines 2501 - 2510, Add a boundary assertion alongside the existing inspect_websocket_session test for cursor == count (cursor 1 with the one-operation fixture), asserting it is accepted and returns an empty page. Keep the existing cursor 2 InvalidWebSocketSource assertion unchanged.
402-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared risk inference.
This block duplicates the inference in
build_websocket_plan_with_configurationat lines 498-516. The two copies can drift. Extract one helper that takes the declared risk and the actions, and call it from both sites. The plan path keeps its configuration override on top of the helper result.♻️ Proposed helper
+fn infer_websocket_risk(declared: Option<RiskClass>, actions: &[WebSocketAction]) -> RiskClass { + let sends_data = actions.iter().any(|action| { + matches!( + action, + WebSocketAction::SendText { .. } | WebSocketAction::SendBinary { .. } + ) + }); + match declared { + Some(RiskClass::Read | RiskClass::Unknown) if sends_data => RiskClass::Write, + Some(risk) => risk, + None if sends_data => RiskClass::Write, + None => RiskClass::Unknown, + } +}Then in
inspect_websocket_session:- let sends_data = source.actions.iter().any(|action| { - matches!( - action, - WebSocketAction::SendText { .. } | WebSocketAction::SendBinary { .. } - ) - }); - let risk = match source.risk { - Some(RiskClass::Read | RiskClass::Unknown) if sends_data => RiskClass::Write, - Some(risk) => risk, - None if sends_data => RiskClass::Write, - None => RiskClass::Unknown, - }; + let risk = infer_websocket_risk(source.risk, &source.actions);🤖 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 `@crates/kahea-plan/src/lib.rs` around lines 402 - 413, Extract the shared risk inference from the current block into a helper accepting the declared risk and action collection, preserving the SendText/SendBinary detection and Write/Unknown behavior. Replace the duplicated logic in both inspect_websocket_session and build_websocket_plan_with_configuration with calls to this helper, while keeping the plan path’s configuration override applied after the helper result.crates/kahea/tests/cli.rs (3)
525-637: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the three scenarios into separate tests.
This function covers expectation failure, action timeout, and handshake failure in sequence. If the first scenario fails, the other two never run, and their temporary directories are never created or cleaned. Three
#[test]functions give independent signals and allow parallel execution.🤖 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 `@crates/kahea/tests/cli.rs` around lines 525 - 637, Split websocket_cli_maps_expectation_timeout_and_handshake_failures into three independent #[test] functions covering expectation failure, action timeout, and handshake failure. Move each scenario’s setup, invocation, assertions, server join, and temporary-directory cleanup into its own test, preserving the existing symbols and expected outcomes.
447-459: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the accept in the test server thread.
listener.accept()blocks without a deadline. If the CLI never connects,server.join()at line 476 blocks forever and the test hangs instead of failing. Set a read timeout on the accepted stream, or set the listener to non-blocking with a bounded poll, so a connection failure surfaces as a test failure. The failure-path tests at lines 548-552, 585-589, and 619-626 use the same pattern.🤖 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 `@crates/kahea/tests/cli.rs` around lines 447 - 459, Bound the server-thread accept flow around listener.accept() so missing CLI connections fail within a timeout instead of blocking server.join() indefinitely. Apply the same bounded-accept handling to the analogous failure-path test servers, preserving connection handling while ensuring timeout errors propagate as test failures.
665-688: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every rejected override, not only
--server.
crates/kahea/src/main.rslines 326-338 reject--input,--set,--server,--auth,--content-type, and--check. This test exercises only--server. If one condition is dropped from that guard, the test still passes. Loop over each flag and assertinvalid-websocket-plan-optionsfor all of them.💚 Proposed change
- let override_attempt = Command::new(binary()) - .args([ - "plan", - source.to_str().unwrap(), - "cliSession", - "--server", - "ws://attacker.example.test/socket", - "--store", - store.to_str().unwrap(), - ]) - .output() - .unwrap(); - assert_eq!(override_attempt.status.code(), Some(2)); - let error: Value = serde_json::from_slice(&override_attempt.stdout).unwrap(); - assert_eq!(error["code"], "invalid-websocket-plan-options"); + for override_flag in [ + vec!["--server", "ws://attacker.example.test/socket"], + vec!["--auth", "chat-sandbox"], + vec!["--content-type", "application/json"], + vec!["--check", "status:200"], + vec!["--set", "body.injected=true"], + ] { + let mut arguments = vec!["plan", source.to_str().unwrap(), "cliSession"]; + arguments.extend(override_flag.iter().copied()); + arguments.extend(["--store", store.to_str().unwrap()]); + let override_attempt = Command::new(binary()).args(&arguments).output().unwrap(); + assert_eq!( + override_attempt.status.code(), + Some(2), + "override {override_flag:?} was accepted" + ); + let error: Value = serde_json::from_slice(&override_attempt.stdout).unwrap(); + assert_eq!(error["code"], "invalid-websocket-plan-options"); + }🤖 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 `@crates/kahea/tests/cli.rs` around lines 665 - 688, Expand the override rejection test around the existing `override_attempt` command to iterate over `--input`, `--set`, `--server`, `--auth`, `--content-type`, and `--check`, supplying each flag’s required value. Execute the CLI for every override and assert each response exits with status 2 and reports `invalid-websocket-plan-options`, while preserving the existing WebSocket plan setup.crates/kahea/src/main.rs (1)
605-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the exit computation into the terminal arms.
result.exit()returnsNoneonly forWebSocketConnectResult::Connected, and that arm returns an error beforeexitis used. Theunwrap_or(3)default is therefore unreachable. Bind the exit inside each terminal arm instead, so the code states the invariant directly.♻️ Proposed simplification
- let exit = result.exit().unwrap_or(3); - match result { + let exit = match result { WebSocketConnectResult::Observation(observation) => { + let exit = observation.exit; write_envelope(&observation).map_err(io_error)?; + exit } WebSocketConnectResult::Denied(denial) => { + let exit = denial.exit; write_envelope(&denial).map_err(io_error)?; + exit } WebSocketConnectResult::Connected(_) => { return Err(CliError { code: "websocket-invocation-failed", message: "WebSocket executor returned a non-terminal connection".into(), exit: 3, }); } - } + }; return Ok(exit);🤖 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 `@crates/kahea/src/main.rs` around lines 605 - 621, Move the result.exit() computation out of the shared pre-match binding and into the terminal Observation and Denied arms, using the matched value to obtain and return the exit status after writing the envelope. Keep the Connected arm returning the existing websocket-invocation-failed error without computing a fallback exit.
🤖 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.
Inline comments:
In `@crates/kahea/tests/cli.rs`:
- Around line 585-597: Update the timeout server logic in the timeout_server
thread to keep the accepted WebSocket connection open until the client
disconnects, or otherwise wait beyond the configured 2000 ms total timeout.
Preserve the existing invoke_websocket assertions so the client consistently
reports terminal_cause as action-timeout.
---
Nitpick comments:
In `@crates/kahea-plan/src/lib.rs`:
- Around line 2501-2510: Add a boundary assertion alongside the existing
inspect_websocket_session test for cursor == count (cursor 1 with the
one-operation fixture), asserting it is accepted and returns an empty page. Keep
the existing cursor 2 InvalidWebSocketSource assertion unchanged.
- Around line 402-413: Extract the shared risk inference from the current block
into a helper accepting the declared risk and action collection, preserving the
SendText/SendBinary detection and Write/Unknown behavior. Replace the duplicated
logic in both inspect_websocket_session and
build_websocket_plan_with_configuration with calls to this helper, while keeping
the plan path’s configuration override applied after the helper result.
In `@crates/kahea/src/main.rs`:
- Around line 605-621: Move the result.exit() computation out of the shared
pre-match binding and into the terminal Observation and Denied arms, using the
matched value to obtain and return the exit status after writing the envelope.
Keep the Connected arm returning the existing websocket-invocation-failed error
without computing a fallback exit.
In `@crates/kahea/tests/cli.rs`:
- Around line 525-637: Split
websocket_cli_maps_expectation_timeout_and_handshake_failures into three
independent #[test] functions covering expectation failure, action timeout, and
handshake failure. Move each scenario’s setup, invocation, assertions, server
join, and temporary-directory cleanup into its own test, preserving the existing
symbols and expected outcomes.
- Around line 447-459: Bound the server-thread accept flow around
listener.accept() so missing CLI connections fail within a timeout instead of
blocking server.join() indefinitely. Apply the same bounded-accept handling to
the analogous failure-path test servers, preserving connection handling while
ensuring timeout errors propagate as test failures.
- Around line 665-688: Expand the override rejection test around the existing
`override_attempt` command to iterate over `--input`, `--set`, `--server`,
`--auth`, `--content-type`, and `--check`, supplying each flag’s required value.
Execute the CLI for every override and assert each response exits with status 2
and reports `invalid-websocket-plan-options`, while preserving the existing
WebSocket plan setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96aac1e6-f346-44f1-b2b1-f8b2904a9ca6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
README.mdcrates/kahea-plan/src/lib.rscrates/kahea/Cargo.tomlcrates/kahea/src/main.rscrates/kahea/tests/cli.rs
1 participant
Closes #14
Summary
kahea explainValidation
scripts/gates.shSummary by CodeRabbit
New Features
Documentation