mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
* feat(shell): add Low/Medium/High risk levels for graduated approval (#172) - Add `RiskLevel` enum (Low/Medium/High, Ord-comparable) to `tool.rs` and re-export from `tools/mod.rs` - Add `risk_level_for(¶ms) -> RiskLevel` to the `Tool` trait (default: Low); override on `ShellTool` via `classify_command_risk` - Add `classify_command_risk(command: &str) -> RiskLevel` to `shell.rs`: High for NEVER_AUTO_APPROVE patterns, Low for read-only prefixes, Medium for reversible mutations, Medium as the unknown-command default - Add `extract_command_param` helper to de-duplicate JSON extraction - Add `sudo ` to `NEVER_AUTO_APPROVE_PATTERNS` (now classified High) - Wire `risk_level_for` into `requires_approval`: Low → Never, Medium → UnlessAutoApproved, High → Always (uses upstream's new API) - Log risk level at INFO on every tool call in `worker.rs` - Replace `requires_explicit_approval` (simple bool) with the richer `classify_command_risk`; update dispatcher.rs test - Add tests: `test_classify_command_risk_high/low/medium/pipeline`, `test_risk_level_for_via_tool_trait`, updated approval tests Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * style: apply cargo fmt to shell.rs and dispatcher.rs Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(shell): fix pipeline risk aggregation and word-boundary matching Address reviewer feedback: - `classify_command_risk` now iterates ALL pipeline segments and takes the maximum risk, so `echo hello | cargo build` → Medium instead of the previous (wrong) Low - Replace `starts_with` with `matches_command_pattern`: single-word patterns use exact first-token comparison so `lsblk` no longer matches `ls`, `makeself` no longer matches `make`, etc.; multi-word patterns (e.g. `git status`) still use starts_with + space boundary - Drop `--help` / `-h` from LOW_RISK_PATTERNS (can never be first token) - Add `test_classify_command_risk_word_boundary` and extend pipeline test with mixed Low+Medium and unknown-command cases Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(shell): move sed/awk/find from Low to Medium risk `sed -i`, `awk -i inplace`, and `find -delete`/`find -exec rm` can all modify or delete files. Classifying these as Low (auto-approve) was unsafe. Moving to Medium requires UnlessAutoApproved approval, which prompts the user unless they have explicitly enabled auto-approve mode. Fixes review feedback from zmanian on PR #368. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(shell): update test to use classify_command_risk after requires_explicit_approval removal The rebase brought in upstream commits that removed requires_explicit_approval. Update the mixed-case destructive command test to assert RiskLevel::High via classify_command_risk instead. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(shell): use word-boundary matching for High-risk patterns to prevent false positives The NEVER_AUTO_APPROVE_PATTERNS check used `contains()` on the full command string, causing false positives: `makeshutdownscript` matched `shutdown`, `nftables-config` matched `nft`, and `passwdqc-check` matched `passwd`. Fix: move the High-risk check inside the per-segment loop and use `matches_command_pattern` (the same word-boundary logic used for Low/Medium), so classification is consistent across all three risk levels. Also remove the trailing spaces from `"nft "` and `"sudo "` in NEVER_AUTO_APPROVE_PATTERNS since `matches_command_pattern` handles word-boundary detection without them. Adds three regression tests for the false-positive cases. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(shell): address zmanian review — redirect safety + explicit git push pattern Two issues from zmanian's CHANGES_REQUESTED review on PR #368: 1. **Security (Low → UnlessAutoApproved)**: `Low` was mapped to `ApprovalRequirement::Never`, bypassing approval entirely for commands like `cat /etc/shadow > /tmp/out` since the pipeline splitter does not split on shell redirections (`>`, `>>`). Changing to `UnlessAutoApproved` preserves the graduated risk metadata for audit while keeping approval policy conservative until redirect-aware parsing is in place. 2. **Minor (explicit git push pattern)**: `git push origin feature-branch` fell through to the unknown-command Medium default rather than matching an explicit pattern. Adding `"git push"` to MEDIUM_RISK_PATTERNS makes the classification intentional. Force-push variants (`git push --force`, `git push -f`) remain in NEVER_AUTO_APPROVE_PATTERNS (High). Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(shell): add regression tests for redirect bypass and git push pattern fixes Two regression tests for the fixes in the previous commit: 1. `test_low_risk_with_redirect_not_never` — verifies that Low-risk commands containing shell redirections (`echo x > /etc/passwd`, `cat /etc/shadow > /tmp/out`, etc.) return `UnlessAutoApproved`, not `Never`. Before the fix, `Low` mapped to `Never` which would have allowed these writes to bypass approval entirely. 2. `test_git_push_explicit_medium_pattern` — verifies that `git push origin branch` is classified `Medium` via the explicit `MEDIUM_RISK_PATTERNS` entry (not the unknown-command fallthrough). Force variants (`--force`, `-f`) remain `High`. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * test(shell): add integration regression tests for redirect bypass and git push Covers the two fixes from the previous commits at the integration-test level (tests/ directory) to ensure the CI regression-test gate is satisfied: 1. `low_risk_command_with_redirect_is_unless_auto_approved` -- verifies that Low-risk commands containing shell redirections return UnlessAutoApproved, not Never (the pre-fix behaviour that allowed redirect-based bypass). 2. `git_push_is_unless_auto_approved` -- verifies git push is Medium risk (UnlessAutoApproved) via the explicit pattern, not unknown-command fallthrough. 3. `git_push_force_requires_always_approval` -- verifies force-push variants remain High risk (Always approval required). Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * refactor(test): move inline assertions to tests/ to satisfy no-panics CI check The project's no-panics CI check (code_style.yml) scans src/**/*.rs for assert_eq!/assert_ne!/.unwrap() in added lines. Moving classify_command_risk tests to tests/shell_risk_regression.rs and adding // safety: comments on the two remaining assertions in dispatcher.rs eliminates all false positives. - Remove test_classify_command_risk_* and related functions from shell.rs - Remove test_low_risk_with_redirect_not_never and test_git_push_* from shell.rs (covered by integration tests in tests/) - Expand tests/shell_risk_regression.rs with full coverage via public API - Add // safety: test code comments on dispatcher.rs assert lines Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(shell): address review findings — force-with-lease, test runners, Display - Add `git push --force-with-lease` to NEVER_AUTO_APPROVE_PATTERNS — the word-boundary matching in matches_command_pattern would not match it against the existing `git push --force` pattern (next char is `-`, not space), causing it to fall through to Medium instead of High. - Move `cargo test`, `npm test`, `npm run test`, `yarn test` from LOW_RISK_PATTERNS to MEDIUM_RISK_PATTERNS — test runners execute arbitrary code and can have side effects (file creation, network calls, process spawning). - Add `Display` impl for `RiskLevel` (lowercase: low/medium/high) and switch worker logging from `?risk` (Debug) to `%risk` (Display) for cleaner audit logs. - Fix integration test helper to call `register_dev_tools()` since ShellTool is registered there, not in `register_builtin_tools()`. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Sonnet 4.6 <[email protected]> Co-authored-by: [email protected] <[email protected]>
281 lines
9.0 KiB
Rust
281 lines
9.0 KiB
Rust
//! Regression and unit tests for shell command risk-level classification
|
|
//! (issue #172, PR #368).
|
|
//!
|
|
//! These tests live here (instead of inline in `src/tools/builtin/shell.rs`)
|
|
//! because the project's no-panics CI check scans `src/**/*.rs` for
|
|
//! `assert_eq!` / `assert_ne!` / `.unwrap()` in added lines. All assertions
|
|
//! on the public `ShellTool` API belong here.
|
|
//!
|
|
//! All tests access the shell tool through the public `ToolRegistry` +
|
|
//! `Tool` trait surface (`risk_level_for`, `requires_approval`).
|
|
//!
|
|
//! ## What is tested
|
|
//!
|
|
//! 1. **Risk level tiers** (`High`, `Medium`, `Low`) for representative commands.
|
|
//! 2. **Word-boundary matching** — commands whose names are substrings of other
|
|
//! words must not be misclassified.
|
|
//! 3. **Pipeline aggregation** — the whole pipeline takes the maximum risk of
|
|
//! its segments.
|
|
//! 4. **Redirect bypass regression** — Low-risk commands with shell redirections
|
|
//! must return `UnlessAutoApproved`, not `Never`.
|
|
//! 5. **`git push` regression** — non-force push is explicitly `Medium`; force
|
|
//! variants remain `High`.
|
|
//! 6. **`risk_level_for` trait method** — delegates to classify_command_risk.
|
|
|
|
use ironclaw::tools::{ApprovalRequirement, RiskLevel, Tool, ToolRegistry};
|
|
use std::sync::Arc;
|
|
|
|
// ---------------------------------------------------------------------------
|
|
// Helper: obtain a `ShellTool` from the registry
|
|
// ---------------------------------------------------------------------------
|
|
|
|
async fn shell_tool() -> Arc<dyn Tool> {
|
|
let registry = ToolRegistry::new();
|
|
registry.register_builtin_tools();
|
|
registry.register_dev_tools();
|
|
registry
|
|
.all()
|
|
.await
|
|
.into_iter()
|
|
.find(|t| t.name() == "shell")
|
|
.expect("shell tool must be registered")
|
|
}
|
|
|
|
fn risk(tool: &Arc<dyn Tool>, cmd: &str) -> RiskLevel {
|
|
tool.risk_level_for(&serde_json::json!({ "command": cmd }))
|
|
}
|
|
|
|
fn approval(tool: &Arc<dyn Tool>, cmd: &str) -> ApprovalRequirement {
|
|
tool.requires_approval(&serde_json::json!({ "command": cmd }))
|
|
}
|
|
|
|
// ---------------------------------------------------------------------------
|
|
// 1. Risk level tiers
|
|
// ---------------------------------------------------------------------------
|
|
|
|
#[tokio::test]
|
|
async fn high_risk_commands() {
|
|
let tool = shell_tool().await;
|
|
let cmds = [
|
|
"rm -rf /tmp/stuff",
|
|
"git push --force origin main",
|
|
"git reset --hard HEAD~5",
|
|
"docker rm container_name",
|
|
"kill -9 12345",
|
|
"DROP TABLE users;",
|
|
"sudo apt install something",
|
|
];
|
|
for cmd in &cmds {
|
|
assert_eq!(
|
|
risk(&tool, cmd),
|
|
RiskLevel::High,
|
|
"command `{cmd}` should be High risk"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn low_risk_commands() {
|
|
let tool = shell_tool().await;
|
|
let cmds = [
|
|
"ls -la",
|
|
"cat file.txt",
|
|
"grep foo bar.txt",
|
|
"git status",
|
|
"git log --oneline",
|
|
"echo hello",
|
|
"cargo check",
|
|
];
|
|
for cmd in &cmds {
|
|
assert_eq!(
|
|
risk(&tool, cmd),
|
|
RiskLevel::Low,
|
|
"command `{cmd}` should be Low risk"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn medium_risk_commands() {
|
|
let tool = shell_tool().await;
|
|
let cmds = [
|
|
"cargo build",
|
|
"cargo test",
|
|
"npm test",
|
|
"yarn test",
|
|
"git commit -m 'foo'",
|
|
"mkdir /tmp/dir",
|
|
"npm install lodash",
|
|
"git push origin feature-branch",
|
|
"my-custom-tool --flag",
|
|
"sed 's/foo/bar/g' file.txt",
|
|
"sed -i 's/foo/bar/' file.txt",
|
|
"awk '{print $1}' file.txt",
|
|
"find . -name '*.rs'",
|
|
"find . -delete",
|
|
];
|
|
for cmd in &cmds {
|
|
assert_eq!(
|
|
risk(&tool, cmd),
|
|
RiskLevel::Medium,
|
|
"command `{cmd}` should be Medium risk"
|
|
);
|
|
}
|
|
}
|
|
|
|
// ---------------------------------------------------------------------------
|
|
// 2. Word-boundary matching (no false positives for substrings)
|
|
// ---------------------------------------------------------------------------
|
|
|
|
#[tokio::test]
|
|
async fn word_boundary_no_false_positives() {
|
|
let tool = shell_tool().await;
|
|
// "lsblk" must NOT match "ls" (Low-risk prefix)
|
|
assert_eq!(risk(&tool, "lsblk"), RiskLevel::Medium);
|
|
// "makeself" must NOT match "make"
|
|
assert_eq!(risk(&tool, "makeself output.run"), RiskLevel::Medium);
|
|
// "git statusbar" must NOT match "git status"
|
|
assert_eq!(risk(&tool, "git statusbar"), RiskLevel::Medium);
|
|
// Commands with High-risk names as substrings must not be tagged High
|
|
assert_eq!(risk(&tool, "makeshutdownscript --help"), RiskLevel::Medium);
|
|
assert_eq!(risk(&tool, "nftables-config"), RiskLevel::Medium);
|
|
assert_eq!(risk(&tool, "passwdqc-check"), RiskLevel::Medium);
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn word_boundary_correct_positive_matches() {
|
|
let tool = shell_tool().await;
|
|
assert_eq!(risk(&tool, "ls -la"), RiskLevel::Low);
|
|
assert_eq!(risk(&tool, "make install"), RiskLevel::Medium);
|
|
assert_eq!(risk(&tool, "git status"), RiskLevel::Low);
|
|
}
|
|
|
|
// ---------------------------------------------------------------------------
|
|
// 3. Pipeline aggregation
|
|
// ---------------------------------------------------------------------------
|
|
|
|
#[tokio::test]
|
|
async fn pipeline_takes_max_risk() {
|
|
let tool = shell_tool().await;
|
|
// High-risk segment → whole pipeline is High
|
|
assert_eq!(risk(&tool, "ls /tmp | rm -rf /tmp/stuff"), RiskLevel::High);
|
|
// All-low pipeline stays Low
|
|
assert_eq!(risk(&tool, "ls -la | grep foo"), RiskLevel::Low);
|
|
// Low + Medium → max is Medium
|
|
assert_eq!(risk(&tool, "echo hello | cargo build"), RiskLevel::Medium);
|
|
// Unknown command in pipeline → Medium (safe default)
|
|
assert_eq!(
|
|
risk(&tool, "cat file.txt | my-custom-tool"),
|
|
RiskLevel::Medium
|
|
);
|
|
}
|
|
|
|
// ---------------------------------------------------------------------------
|
|
// 4. Redirect bypass regression (Low → UnlessAutoApproved, not Never)
|
|
// ---------------------------------------------------------------------------
|
|
|
|
#[tokio::test]
|
|
async fn low_risk_command_with_redirect_is_unless_auto_approved() {
|
|
let tool = shell_tool().await;
|
|
let cases = [
|
|
"echo secret_data > /etc/passwd",
|
|
"cat /etc/shadow > /tmp/exfil.txt",
|
|
"printf '%s' value > /tmp/leak",
|
|
"ls -la >> /tmp/log.txt",
|
|
];
|
|
for cmd in &cases {
|
|
let result = approval(&tool, cmd);
|
|
assert_eq!(
|
|
result,
|
|
ApprovalRequirement::UnlessAutoApproved,
|
|
"command `{cmd}` must be UnlessAutoApproved (not Never), got {result:?}"
|
|
);
|
|
}
|
|
}
|
|
|
|
// ---------------------------------------------------------------------------
|
|
// 5. git push regressions
|
|
// ---------------------------------------------------------------------------
|
|
|
|
#[tokio::test]
|
|
async fn git_push_classifies_as_medium_risk() {
|
|
let tool = shell_tool().await;
|
|
let cmds = [
|
|
"git push",
|
|
"git push origin main",
|
|
"git push --set-upstream origin feature",
|
|
"git push upstream feature/foo",
|
|
];
|
|
for cmd in &cmds {
|
|
assert_eq!(risk(&tool, cmd), RiskLevel::Medium, "command `{cmd}`");
|
|
}
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn git_push_force_remains_high_risk() {
|
|
let tool = shell_tool().await;
|
|
let cmds = [
|
|
"git push --force",
|
|
"git push -f",
|
|
"git push --force-with-lease",
|
|
"git push --force origin main",
|
|
"git push -f origin main",
|
|
];
|
|
for cmd in &cmds {
|
|
assert_eq!(risk(&tool, cmd), RiskLevel::High, "command `{cmd}`");
|
|
}
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn git_push_non_force_is_unless_auto_approved() {
|
|
let tool = shell_tool().await;
|
|
let cmds = [
|
|
"git push",
|
|
"git push origin main",
|
|
"git push upstream feature/foo",
|
|
];
|
|
for cmd in &cmds {
|
|
let result = approval(&tool, cmd);
|
|
assert_eq!(
|
|
result,
|
|
ApprovalRequirement::UnlessAutoApproved,
|
|
"command `{cmd}` should be UnlessAutoApproved, got {result:?}"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn git_push_force_requires_always_approval() {
|
|
let tool = shell_tool().await;
|
|
let cmds = [
|
|
"git push --force",
|
|
"git push -f",
|
|
"git push --force-with-lease",
|
|
];
|
|
for cmd in &cmds {
|
|
let result = approval(&tool, cmd);
|
|
assert_eq!(
|
|
result,
|
|
ApprovalRequirement::Always,
|
|
"force-push `{cmd}` should require Always approval, got {result:?}"
|
|
);
|
|
}
|
|
}
|
|
|
|
// ---------------------------------------------------------------------------
|
|
// 6. risk_level_for trait method
|
|
// ---------------------------------------------------------------------------
|
|
|
|
#[tokio::test]
|
|
async fn risk_level_for_via_tool_trait() {
|
|
let tool = shell_tool().await;
|
|
assert_eq!(risk(&tool, "ls -la"), RiskLevel::Low);
|
|
assert_eq!(risk(&tool, "cargo build"), RiskLevel::Medium);
|
|
assert_eq!(risk(&tool, "rm -rf /tmp"), RiskLevel::High);
|
|
// Missing params → Medium (safe default)
|
|
assert_eq!(
|
|
tool.risk_level_for(&serde_json::json!({})),
|
|
RiskLevel::Medium
|
|
);
|
|
}
|