feat(policy): ✨ Allow shell in plan mode via approval policy with read-only constraint - #89
Conversation
Previously, the shell tool was hard-denied in plan mode, which prevented agents from running read-only commands (git log, npm ls, command -v, etc.) needed for evidence gathering during planning. This commit moves shell out of the plan-mode hard-deny list so it falls through to the normal approval policy instead. Key changes: - Extract PLAN_HARD_DENY_TOOLS from MUTATING_TOOLS, excluding shell - Make shell tool available in both plan and full tool profiles - Update plan-mode prompt to clarify shell is read-only only - Add test confirming shell is not hard-denied in plan mode and follows normal approval policy
AI Code Review SummaryPR: #89 (feat(policy): ✨ Allow shell in plan mode via approval policy with read-only constraint) Overall AssessmentDetected 1 actionable findings, prioritize CRITICAL/HIGH before merge. Major Findings by Severity
Actionable Suggestions
Potential Risks
Test Suggestions
File-Level Coverage Notes
Inline Downgraded Items (processed but not inline)
Coverage Status
Uncovered list:
No-patch covered list:
Runtime/Budget
|
| "market_install", | ||
| ]; | ||
|
|
||
| /// Tools that are hard-denied in plan mode. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| /// Shell is intentionally excluded — it follows the normal approval policy so that | ||
| /// read-only commands (git log, npm ls, command -v, skill CLIs, etc.) remain | ||
| /// available for information gathering in plan mode. | ||
| const PLAN_HARD_DENY_TOOLS: &[&str] = &[ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| // Shell is not in the hard-deny list; it follows the normal approval policy. | ||
| let blocked = mutating_tools.contains(&"shell"); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Address PR review #3: test variable name was misleading since shell is now excluded from the plan-mode hard-deny list. Renamed to hard_deny_tools and renamed test function to test_plan_mode_blocks_hard_deny_tools.
| /// Shell is intentionally excluded — it follows the normal approval policy so that | ||
| /// read-only commands (git log, npm ls, command -v, skill CLIs, etc.) remain | ||
| /// available for information gathering in plan mode. | ||
| const PLAN_HARD_DENY_TOOLS: &[&str] = &[ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| #[test] | ||
| fn plan_read_only_profile_does_not_expose_mutating_terminal_tools() { | ||
| fn plan_read_only_profile_includes_shell_excludes_mutating_terminal_tools() { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if run_mode == "plan" && MUTATING_TOOLS.contains(&tool_name) { | ||
| // 4. Run mode restriction (plan mode blocks hard-deny mutations; | ||
| // shell is excluded so it falls through to the normal approval policy) | ||
| if run_mode == "plan" && PLAN_HARD_DENY_TOOLS.contains(&tool_name) { |
There was a problem hiding this comment.
[MEDIUM] No enforcement mechanism for read-only shell commands in plan mode
Shell is excluded from hard-deny in plan mode with the expectation that it will only be used for read-only commands. However, there's no technical enforcement preventing a user from running mutating commands via shell in plan mode - the policy relies entirely on prompt instructions to guide the LLM behavior.
Suggestion: Consider whether the approval policy should have a plan-mode-specific check for shell commands that attempts to detect mutating operations (e.g., via command pattern matching for rm, mv, git push, npm install, etc.) and either denies them or requires stricter approval. Alternatively, document this as an intentional trust boundary.
Risk: LLM could potentially execute mutating shell commands in plan mode if it misinterprets the instructions or if the user explicitly requests them.
Confidence: 0.85
Summary
shelltool toplan_read_onlytool profile so it is available in plan modePLAN_HARD_DENY_TOOLSconstant excludes shell from plan mode hard-denyChanges
agent_session.rs: Move shell tool definition out ofDEFAULT_FULL_TOOL_PROFILE-only block into shared sectionpolicy_engine.rs: AddPLAN_HARD_DENY_TOOLS(all mutating tools except shell). Plan mode check usesPLAN_HARD_DENY_TOOLSinstead ofMUTATING_TOOLS, so shell falls through to normal approval policyproviders.rs: Plan mode prompt adds "Shell tool: shell — use ONLY for read-only commands" constraint line; removes shell from "Do NOT use" listm1_6_tool_gateway.rs: Remove shell frommutating_toolstest vecs; addtest_plan_mode_shell_follows_approval_policytestagent_session.rstests: Rename test toplan_read_only_profile_includes_shell_excludes_mutating_terminal_tools, add shell presence assertionTest Plan
cargo test— all 245 unit tests pass including newtest_plan_mode_shell_follows_approval_policytest_plan_mode_blocks_mutating_tools— still blocks write/edit/patch/etc but NOT shellplan_read_only_profile_includes_shell_excludes_mutating_terminal_tools— shell IS in plan profilenpm run typecheck— passes🤖 Generated with TiyCode