feat(aios-protocol): tier-specific shell execution policy (BRO-216)#3
Merged
feat(aios-protocol): tier-specific shell execution policy (BRO-216)#3
Conversation
…ous/free/pro/enterprise) Adds anonymous(), free(), pro(), and enterprise() constructors to PolicySet for BRO-211. Each tier sets appropriate allow/gate capability lists and per-turn event/runtime limits. Includes unit tests for all four tiers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- anonymous(): remove exec:cmd:* from gate_capabilities so StaticPolicyEngine immediately denies bash/shell tool calls without creating an approval ticket - free(): add 11 whitelisted exec commands to allow_capabilities (cat, ls, echo, grep, jq, python3, find, head, tail, sort, wc); remove exec:cmd:* wildcard from gate so unlisted commands are denied outright - pro()/enterprise(): unchanged — wildcard '*' in allow_capabilities covers all exec - Update tests: anonymous gate has 3 caps (exec removed), free allow has 13 caps Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/aios-protocol/src/event.rs`:
- Around line 590-604: The new enum variants Queued, Steered, QueueDrained and
the SteeringMode type were added but lack serde roundtrip tests; add unit tests
that serialize each variant (Queued { queue_id, mode, message }, Steered {
queue_id, preempted_at }, QueueDrained { queue_id, processed }) and deserialize
back asserting equality and that the serialized JSON contains the expected
variant tag and fields (and add tests for SteeringMode serialization); put these
tests alongside existing event serde tests (or in the crate tests module) and
ensure assertions cover both structure and content so future serde tag/field
regressions are caught.
- Around line 590-604: The event_kind_name function is missing explicit match
arms for the new enum variants Queued, Steered, and QueueDrained, causing them
to fall through to the generic "custom" label; update the match in
event_kind_name (crates/aios-runtime/src/lib.rs) to add arms matching
Event::Queued { .. } => "queued", Event::Steered { .. } => "steered", and
Event::QueueDrained { .. } => "queue_drained" (use wildcard field patterns to
ignore inner fields) so tracing emits the correct event names for these
variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e36cbbbf-ebf3-4d8c-87b3-dffb2e839217
📒 Files selected for processing (3)
crates/aios-protocol/src/event.rscrates/aios-protocol/src/lib.rscrates/aios-protocol/src/policy.rs
Comment on lines
+590
to
+604
| // ── Queue & steering (Phase 2.5) ── | ||
| Queued { | ||
| queue_id: String, | ||
| mode: SteeringMode, | ||
| message: String, | ||
| }, | ||
| Steered { | ||
| queue_id: String, | ||
| /// Tool boundary where preemption occurred (e.g. "tool:read_file:call-3"). | ||
| preempted_at: String, | ||
| }, | ||
| QueueDrained { | ||
| queue_id: String, | ||
| processed: usize, | ||
| }, |
There was a problem hiding this comment.
Add coverage for new queue/steering event behavior.
Queued, Steered, QueueDrained, and SteeringMode were introduced, but there are no dedicated roundtrip/serialization assertions for them. Please add tests so regressions in serde tags/fields are caught.
Proposed test additions
#[cfg(test)]
mod tests {
use super::*;
@@
fn voice_events_roundtrip() {
let kind = EventKind::VoiceSessionStarted {
@@
let back: EventKind = serde_json::from_str(&json).unwrap();
assert!(matches!(back, EventKind::VoiceSessionStarted { .. }));
}
+
+ #[test]
+ fn queue_events_roundtrip() {
+ let queued = EventKind::Queued {
+ queue_id: "q1".into(),
+ mode: SteeringMode::Followup,
+ message: "next task".into(),
+ };
+ let steered = EventKind::Steered {
+ queue_id: "q1".into(),
+ preempted_at: "tool:read_file:call-3".into(),
+ };
+ let drained = EventKind::QueueDrained {
+ queue_id: "q1".into(),
+ processed: 2,
+ };
+
+ for kind in [queued, steered, drained] {
+ let json = serde_json::to_string(&kind).unwrap();
+ let back: EventKind = serde_json::from_str(&json).unwrap();
+ match (kind, back) {
+ (EventKind::Queued { .. }, EventKind::Queued { .. }) => {}
+ (EventKind::Steered { .. }, EventKind::Steered { .. }) => {}
+ (EventKind::QueueDrained { .. }, EventKind::QueueDrained { .. }) => {}
+ _ => panic!("roundtrip mismatch"),
+ }
+ }
+ }
+
+ #[test]
+ fn steering_mode_serializes_snake_case() {
+ let kind = EventKind::Queued {
+ queue_id: "q2".into(),
+ mode: SteeringMode::Interrupt,
+ message: "urgent".into(),
+ };
+ let json = serde_json::to_string(&kind).unwrap();
+ assert!(json.contains("\"mode\":\"interrupt\""));
+ }
}As per coding guidelines: "Add or extend tests with every behavioral change; do not silently widen capabilities or safety boundaries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/aios-protocol/src/event.rs` around lines 590 - 604, The new enum
variants Queued, Steered, QueueDrained and the SteeringMode type were added but
lack serde roundtrip tests; add unit tests that serialize each variant (Queued {
queue_id, mode, message }, Steered { queue_id, preempted_at }, QueueDrained {
queue_id, processed }) and deserialize back asserting equality and that the
serialized JSON contains the expected variant tag and fields (and add tests for
SteeringMode serialization); put these tests alongside existing event serde
tests (or in the crate tests module) and ensure assertions cover both structure
and content so future serde tag/field regressions are caught.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether runtime event kind naming has explicit arms for new variants.
runtime_file=$(fd '^lib\.rs$' crates/aios-runtime/src | head -n1)
rg -n -C4 'fn event_kind_name|EventKind::Queued|EventKind::Steered|EventKind::QueueDrained|_ => "custom"' "$runtime_file"Repository: broomva/aiOS
Length of output: 781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete event_kind_name function
runtime_file=$(fd '^lib\.rs$' crates/aios-runtime/src | head -n1)
# Extract lines from function start to the closing brace
sed -n '1640,1666p' "$runtime_file"Repository: broomva/aiOS
Length of output: 1606
Add explicit match arms for Queued, Steered, and QueueDrained in event_kind_name.
The three new first-class event variants are not handled in the event_kind_name function (crates/aios-runtime/src/lib.rs:1640), so they fall through to _ => "custom". Add explicit arms to emit proper event names ("queued", "steered", "queue_drained") instead of generic labels, enabling accurate runtime observability via tracing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/aios-protocol/src/event.rs` around lines 590 - 604, The
event_kind_name function is missing explicit match arms for the new enum
variants Queued, Steered, and QueueDrained, causing them to fall through to the
generic "custom" label; update the match in event_kind_name
(crates/aios-runtime/src/lib.rs) to add arms matching Event::Queued { .. } =>
"queued", Event::Steered { .. } => "steered", and Event::QueueDrained { .. } =>
"queue_drained" (use wildcard field patterns to ignore inner fields) so tracing
emits the correct event names for these variants.
…anges Main had exec:cmd:* in gate_capabilities for anonymous/free tiers (old behavior). BRO-216 removes exec:cmd:* from gate so StaticPolicyEngine denies shell calls immediately without creating an approval ticket. Keeping BRO-216 semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
exec:cmd:*fromgate_capabilitiesso theStaticPolicyEngineimmediately denies bash/shell tool calls rather than queuing them for human approval (anonymous sessions have no approver)exec:cmd:*wildcard gate with 11 specific whitelisted exec commands inallow_capabilities(cat,ls,echo,grep,jq,python3,find,head,tail,sort,wc); unlisted commands fall through to denied"*"inallow_capabilitiesalready covers all capabilitiesEnforcement model
Capability evaluation in
StaticPolicyEngine:allow_capabilities→ allowedgate_capabilities→ requires_approval (human queue)Previously,
exec:cmd:*ingate_capabilitiesfor anonymous/free tiers caused the engine to queue bash calls for human approval — an approval that could never arrive. This PR removes the gate so the engine denies immediately.Test plan
policy_set_anonymous: gate has 3 caps (exec removed),exec:cmd:*NOT in gatepolicy_set_free: allow has 13 caps (session-read + net-egress + 11 exec), gate has 2 (fs:write + secrets),exec:cmd:*NOT in gatecargo test -p aios-protocolCloses BRO-216
🤖 Generated with Claude Code
Summary by CodeRabbit