feat(shell): add Low/Medium/High risk levels for graduated command approval (closes #172)

[nlok5923]

Summary

Implements graduated command approval tiers for the shell tool, as requested in #172.

• RiskLevel enum (Low / Medium / High, Ord-comparable) added to tool.rs and re-exported from tools/mod.rs
• risk_level_for(&params) -> RiskLevel added to the Tool trait (default: Low); overridden on ShellTool to delegate to classify_command_risk
• classify_command_risk(command: &str) -> RiskLevel in shell.rs:
  • High — matches NEVER_AUTO_APPROVE_PATTERNS (destructive / irreversible; e.g. rm -rf, git push --force, kill -9, DROP TABLE)
  • Low — matches LOW_RISK_PATTERNS (read-only, no side effects; e.g. ls, cat, grep, git status, cargo check)
  • Medium — matches MEDIUM_RISK_PATTERNS (reversible mutations; e.g. git commit, cargo build, npm install)
  • Medium — unknown commands default to Medium (safer than silently auto-approving an unrecognised binary)
  • Pipeline commands (ls | grep foo) are split on |, &, ; — a single High-risk segment makes the whole pipeline High
• requires_approval_for updated to use risk_level_for: High → always require approval even with auto-approve; Low/Medium → auto-approvable
• extract_command_param helper de-duplicates the JSON extraction logic shared by risk_level_for and requires_approval_for
• Worker logging (worker.rs): risk level logged at INFO on every tool call for operator-visible audit trail
• requires_explicit_approval removed — replaced by the richer three-tier classify_command_risk

Test plan

• test_classify_command_risk_high — destructive commands → High
• test_classify_command_risk_low — read-only commands → Low
• test_classify_command_risk_medium — reversible mutations → Medium
• test_classify_command_risk_pipeline — High segment in pipeline → High; all-Low pipeline → Low
• test_risk_level_for_via_tool_trait — ShellTool.risk_level_for returns correct level per command; missing params → Medium
• Existing test_requires_approval_for_* tests still pass
• cargo clippy --all-features — zero warnings
• cargo test --lib — 1184 passed, 0 failed (single-threaded)

🤖 Generated with Claude Code

[gemini-code-assist]

Summary of Changes

Hello @nlok5923, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a graduated command approval system for the shell tool, categorizing commands into Low, Medium, and High risk levels. This enhancement improves security and control by allowing more granular approval policies, ensuring that destructive or sensitive commands always require explicit user confirmation, while less risky operations can be auto-approved based on configuration. The system provides better visibility into command execution risk through updated logging.

Highlights

• Graduated Command Approval: Introduced a RiskLevel enum (Low, Medium, High) for classifying shell command risk, which is Ord-comparable for easy comparison.
• Tool Trait Extension: Added a risk_level_for method to the Tool trait, with a default Low risk, and overridden it for ShellTool to delegate to a new command classification logic.
• Command Risk Classification: Implemented classify_command_risk in shell.rs to categorize commands based on predefined patterns for High (destructive), Low (read-only), and Medium (reversible mutations) risks, with unknown commands defaulting to Medium.
• Pipeline Command Handling: Enhanced classify_command_risk to correctly assess pipeline commands, where the presence of any high-risk segment makes the entire pipeline high-risk.
• Approval Logic Update: Updated requires_approval_for in ShellTool to leverage the new risk_level_for method, ensuring that High risk commands always require explicit user approval.
• Command Parameter Extraction: Created a new extract_command_param helper function to centralize and de-duplicate the logic for extracting the command string from tool parameters.
• Worker Logging Enhancement: Modified worker logging to include the determined RiskLevel at an INFO level for every tool call, providing an operator-visible audit trail.
• Deprecated Function Removal: Removed the requires_explicit_approval function, as its functionality is now superseded and enriched by the new three-tier risk classification system.

Changelog

• src/agent/worker.rs
  • Changed tool call logging from debug to info level.
  • Added the determined risk level to the tool call log.
• src/tools/builtin/shell.rs
  • Imported the new RiskLevel enum.
  • Added sudo to the NEVER_AUTO_APPROVE_PATTERNS list.
  • Defined new static lists: LOW_RISK_PATTERNS and MEDIUM_RISK_PATTERNS.
  • Implemented the classify_command_risk function to categorize commands based on predefined patterns and pipeline segments.
  • Added the extract_command_param helper function for robust command extraction from JSON parameters.
  • Overrode the risk_level_for method in ShellTool to utilize classify_command_risk.
  • Updated the requires_approval_for method in ShellTool to check against RiskLevel::High.
  • Removed the requires_explicit_approval function.
  • Replaced test_requires_explicit_approval with new tests for classify_command_risk covering High, Low, Medium, and pipeline scenarios.
  • Updated existing tests to use classify_command_risk and added test_risk_level_for_via_tool_trait.
• src/tools/mod.rs
  • Re-exported the new RiskLevel enum from the tool module.
• src/tools/tool.rs
  • Defined the RiskLevel enum with Low, Medium, and High variants, implementing Ord for comparison.
  • Added a default risk_level_for method to the Tool trait, returning RiskLevel::Low.

Activity

• No specific activity (comments, reviews, progress updates) was provided in the context.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a more granular, three-tier risk classification for shell commands, enhancing security and improving observability by adding risk levels to worker logs. However, a vulnerability exists in the risk assessment of low and medium-risk commands within pipelines, as only the first command is checked, and the command detection itself is not robust enough. This could lead to a medium-risk command being executed with low-risk privileges, potentially allowing auto-approval of commands that should require review. The implementation is generally well-structured, but the current pipeline risk calculation and individual command classification need to be improved for accuracy and security.

[gemini-code-assist]

The classify_command_risk function incorrectly assesses the risk of shell command pipelines, leading to a vulnerability where only the first command is checked for low and medium risk levels, and even then, it uses a starts_with check which can lead to false positives. This allows a medium-risk command to be classified as low-risk by prepending a low-risk command (e.g., echo "hello" | medium_risk_command), potentially leading to auto-approval of commands that should require user review. The overall risk of a pipeline should be the maximum risk of any of its individual commands. Furthermore, command detection should use token-based or word-boundary checks to avoid false positives (e.g., 'ls-l' being classified as 'ls'). The logic should be updated to iterate through all segments of the pipeline and determine the highest risk level among them, using robust command detection for each segment.

    // For pipelines, the risk is the maximum risk of any segment.
    command
        .split(['|', '&', ';'])
        .map(str::trim)
        .filter(|s| !s.is_empty())
        .map(|segment| {
            let lower_segment = segment.to_lowercase();
            // Extract the first word/command for risk classification,
            // adhering to word-boundary checks as per rules.
            let first_command = lower_segment.split_whitespace().next().unwrap_or("");

            if LOW_RISK_PATTERNS
                .iter()
                .any(|p| first_command == p.to_lowercase().as_str())
            {
                RiskLevel::Low
            } else if MEDIUM_RISK_PATTERNS
                .iter()
                .any(|p| first_command == p.to_lowercase().as_str())
            {
                RiskLevel::Medium
            } else {
                // Unknown commands default to Medium.
                RiskLevel::Medium
            }
        })
        .max()
        .unwrap_or(RiskLevel::Medium) // Default for empty/whitespace-only commands.

References

1. When detecting commands or keywords in a string, use token-based or word-boundary checks instead of simple substring containment to avoid false positives (e.g., 'sync' matching 'nc', 'ghost' matching 'host').
2. When checking for piped shell commands, use word boundary validation to avoid false positives where the shell name is a substring of another word (e.g., 'shift' matching 'sh').

[zmanian]

Review: Graduated Shell Command Risk Levels

Well-designed feature. The RiskLevel enum with Ord derivation, pipeline-aware classification, and word-boundary matching are all well thought out.

What's Good

• RiskLevel on the Tool trait: Clean extension point. Default is Low for most tools, shell overrides per-invocation based on the command string.
• Pipeline analysis: max() across |, &, ; segments ensures a dangerous sub-command is never hidden in a pipeline. Good.
• Word-boundary matching: matches_command_pattern prevents lsblk from matching ls and git statusbar from matching git status. The multi-word vs single-word distinction is correct.
• Unknown commands default to Medium: Safe default.
• sudo added to NEVER_AUTO_APPROVE: Good catch.
• Test coverage: 12+ tests covering all risk levels, pipelines, word boundaries, extraction from JSON.
• Logging risk level: tracing::info! with risk = ?risk per tool call is useful for operators.

Suggestion: Reclassify Some "Low" Commands

sed, awk, and find are currently classified as Low (read-only), but they can be destructive with certain flags:

• sed -i 's/foo/bar/g' *.py -- modifies files in-place
• awk -i inplace '{...}' file -- same
• find . -delete or find . -exec rm {} \; -- deletes files

Since the risk classification drives approval UX (Low = ApprovalRequirement::Never), a user who auto-approves Low commands could have files modified without prompting.

Recommendation: Move sed, awk, and find to Medium. They are often read-only but have destructive modes. The safe default should be Medium when a command can go either way.

Alternatively, add flag-specific patterns: "sed -i" → High, "find.*-delete" → High. But that's more complexity -- moving to Medium is simpler and sufficient.

Minor

• cargo test and npm test in Low is defensible (tests are expected to be safe), though some test suites have side effects. Fine as-is.
• The extract_command_param helper is a good refactor of previously duplicated logic.

Overall this is solid work. The reclassification of sed/awk/find is the main item.

[zmanian]

Good structure overall -- the RiskLevel enum with Ord derive is clean, the pipeline max logic is sound, and the test coverage is thorough. Two issues need addressing before merge, one security-critical:

Security: Low-risk commands with redirections bypass approval entirely

Low maps to ApprovalRequirement::Never, meaning zero approval even if the user has NOT auto-approved shell. The pipeline splitter uses ['|', '&', ';'] but does not split on shell redirections (>, >>, <). This means:

• echo secret_data > /etc/passwd -- classified Low (matches echo), never needs approval
• cat /etc/shadow > /tmp/exfil.txt -- classified Low (matches cat), never needs approval
• printf '%s' "$SECRET" > /tmp/leak -- classified Low (matches printf)

These are real write/exfiltration vectors that skip approval entirely. The detect_command_injection function does not cover simple redirect-based writes.

Fix options (pick one):

1. Promote any command containing > or >> to at least Medium. A one-liner check before the Low-risk match would work.
2. Keep Low mapped to UnlessAutoApproved instead of Never, which preserves the graduated classification without opening a bypass. This is the safer/simpler option -- operators get the risk metadata for audit, but approval policy stays conservative.
3. Split on redirection operators too, but this gets complicated with heredocs and 2>&1.

I'd recommend option 2 for the initial merge and revisit Never once redirect-aware parsing is in place.

Minor: git push (non-force) classified Medium may surprise users

git push origin feature-branch is classified Medium (matched by the MEDIUM_RISK_PATTERNS since git push is not explicitly there -- it falls through to "unknown" which defaults Medium). This is fine and correct, but the test comment says "Non-force push is medium (reversible)" implying it matches a pattern. Worth adding "git push" explicitly to MEDIUM_RISK_PATTERNS so the classification is intentional rather than accidental via the unknown-command fallback.

Everything else looks solid -- word-boundary matching, case-insensitive High checks, extract_command_param dedup, pipeline max semantics, sudo addition, and comprehensive tests.

[ilblackdragon]

Code Review

Overview

Well-designed PR that replaces the binary requires_explicit_approval with a three-tier RiskLevel enum (Low/Medium/High). Key changes:

• RiskLevel enum on Tool trait with risk_level_for() method
• classify_command_risk() with word-boundary matching (fixing substring false positives like lsblk matching ls)
• Pipeline aggregation (max risk across segments)
• Worker audit logging of risk levels
• Comprehensive integration test suite in tests/shell_risk_regression.rs

Bug: git push --force-with-lease misclassified

The test git_push_force_remains_high_risk will fail. "git push --force-with-lease" is expected to be High, but "git push --force" won't match it with the new word-boundary logic:

// matches_command_pattern("git push --force-with-lease", "git push --force")
// Multi-word pattern → checks:
//   segment == pattern → false
//   segment.starts_with("git push --force ") → false (next char is '-', not ' ')
// Result: no match → falls through to MEDIUM_RISK_PATTERNS → "git push" matches → Medium

Fix: Add "git push --force-with-lease" explicitly to NEVER_AUTO_APPROVE_PATTERNS.

Security Considerations

1. sudo correctly added to NEVER_AUTO_APPROVE_PATTERNS — good catch. Note it overlaps with DANGEROUS_PATTERNS which has "sudo " for injection detection. Different purposes, so the overlap is fine.

2. Redirect blindspot is documented but real — echo secret > /etc/passwd classifies as Low risk because > isn't a pipeline separator. The code correctly maps Low → UnlessAutoApproved (not Never) as a mitigation, with a clear comment about needing redirect-aware parsing. Acceptable interim design, but the risk level itself is misleading for audit purposes — a log showing risk=Low for a write to /etc/passwd could give false comfort.

3. cargo test / npm test / yarn test as Low risk is debatable — tests run arbitrary code and can have side effects (file creation, network calls, process spawning). Consider Medium, or document the rationale for Low.

Code Quality

4. Word-boundary matching (matches_command_pattern) — clean implementation, well-documented. The multi-word vs single-word split is the right approach. The false-positive tests (lsblk, nftables-config, makeshutdownscript) are excellent.

5. extract_command_param helper — good de-duplication of the JSON extraction logic.

6. Test structure — moving integration tests to tests/shell_risk_regression.rs to avoid the no-panics CI check on src/ is pragmatic and well-documented. Tests exercise the public ToolRegistry API surface rather than internals.

Minor

• Worker logging uses risk = ?risk (Debug fmt). A Display impl on RiskLevel would produce cleaner logs.
• The nft → nft change is correctly motivated by the new word-boundary matching.

Verdict

Solid design with excellent test coverage. Must fix the git push --force-with-lease bug before merge — the test suite will fail as-is. The cargo test/npm test classification is worth a discussion. Everything else looks good.

Addressed all comments