feat(tools): add read-only support for bash tool - #253
Merged
F16shen merged 4 commits intoJun 26, 2026
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
F16shen
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds session-scoped bash read-only policy plumbing, shared command classification and enforcement, shell caller updates, and react-agent event filtering in LLM callbacks. ChangesRead-only bash policy
ReAct sub-session event filtering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Add read-only command classification in aish-tools::bash, wire ToolExecutionPolicy through preflight_with_context, and enforce it on /diagnose verify and failure diagnose sub-sessions while leaving the main session unchanged.
F16shen
force-pushed
the
feat/bash-read-only-enforcement
branch
from
June 25, 2026 12:58
d8bbe50 to
08f34ab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
crates/aish-llm/src/subsession.rs (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLock the new default in the unit test.
falsepreserves existing main-session behavior, so add an assertion intest_subsession_config_defaultto prevent accidental drift.Suggested test addition
assert_eq!(config.max_context_messages, 50); assert_eq!(config.max_iterations, 10); assert!(config.system_prompt.is_none()); + assert!(!config.enforce_read_only_bash);Also applies to: 129-134
🤖 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/aish-llm/src/subsession.rs` at line 29, Add an explicit assertion in test_subsession_config_default to lock in the new default for enforce_read_only_bash, since the SubsessionConfig default currently sets it to false and should not drift. Update the unit test in subsession.rs so it verifies the default config value alongside the other defaults, using the existing test name and SubsessionConfig initializer as the anchor for the change.
🤖 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/aish-llm/src/session.rs`:
- Around line 1054-1056: The execute_tool_by_name path in LlmSession still
bypasses tool preflight by calling execute_async directly, so update that method
to run the same preflight step used in the tool-call loop before execution.
Reuse ToolContext::for_session(self) and call preflight or
preflight_with_context on the selected tool, then only proceed to execute_async
if the check passes so read-only bash enforcement and future ToolContext
policies apply consistently.
- Line 1265: When creating a subsession in create_subsession(), do not reset
tool_execution_policy to ToolExecutionPolicy::default(); instead inherit the
parent session’s current policy so stricter settings carry forward to child
sessions. Update the subsession construction in Session to clone or copy the
existing tool_execution_policy from the parent session, and keep the
diagnose-path override only if it is still explicitly needed for that flow.
In `@crates/aish-tools/src/bash/read_only.rs`:
- Around line 196-203: The curl checks in read_only handling are too narrow and
miss common mutating and output-writing forms. Update the logic in the curl
branch of the read-only parser to properly parse curl options in the segment
instead of relying on substring matches, and ensure the existing curl detection
path covers symbols like the mutating-method check and the output-file check so
it catches forms such as -XPOST, --request POST, -d, -o/tmp/x, and
--output=/tmp/x.
- Around line 168-211: Fix the read-only detector in read_only.rs so it does not
assume commands are safe just because the first token is not blocked. The
current logic in the command classification path only checks the first token and
then falls through to ReadOnly, which lets wrapped or prefixed mutating commands
slip through. Update the decision logic around the command parsing helper to use
a positive allowlist for known safe commands, and explicitly treat
wrappers/interpreters like command, env, sh, bash, and similar shell launchers
as non-read-only unless the full command is safely parsed. Ensure cases like
variable-prefixed commands and shell-invoked rm-style operations are classified
as unsafe.
- Around line 86-103: The bash parser in read_only.rs does not handle backslash
escapes outside quotes, so an escaped quote can be misread as starting a quoted
region. Update the token-scan logic in the quote-handling loop so backslashes
are recognized and consumed when outside single quotes as well, without letting
them toggle quoting state in the main parser flow. Make the fix in the read-only
parsing path that includes this `chars` loop and mirror the same behavior in the
related matching logic around the referenced block. Add regression tests for
escaped single quotes before separators and redirects, such as escaped-quote
cases that must still expose `;` and `>` as active operators.
- Around line 27-42: The read-only check in `read_only.rs` only removes a
leading `sudo` once before `split_compound_segments`, so later compound segments
like `sudo rm` are not analyzed correctly. Update `is_read_only_command` to
handle `sudo` stripping per segment, likely by applying `strip_sudo_prefix`
inside the loop before calling `non_readonly_segment_reason`, so each `seg` is
evaluated after removing any sudo wrapper.
- Around line 171-175: The blanket BLOCKED list in read_only.rs is overreaching
by including sed, which makes the later sed -i-specific handling in the command
validation logic unreachable. Update the bash read-only checks around the
BLOCKED constant and the sed validation path so only destructive sed usage is
blocked while read-only commands like sed -n remain allowed, keeping the
existing command parsing logic in the read_only module consistent.
---
Nitpick comments:
In `@crates/aish-llm/src/subsession.rs`:
- Line 29: Add an explicit assertion in test_subsession_config_default to lock
in the new default for enforce_read_only_bash, since the SubsessionConfig
default currently sets it to false and should not drift. Update the unit test in
subsession.rs so it verifies the default config value alongside the other
defaults, using the existing test name and SubsessionConfig initializer as the
anchor for the change.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1075b2b1-d3c1-4970-8e89-0c53a01dc033
📒 Files selected for processing (16)
crates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-llm/src/diagnose_agent.rscrates/aish-llm/src/lib.rscrates/aish-llm/src/session.rscrates/aish-llm/src/subsession.rscrates/aish-llm/src/tool_context.rscrates/aish-llm/src/types.rscrates/aish-llm/tests/llm_integration_test.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-tools/src/bash/bash.rscrates/aish-tools/src/bash/read_only.rscrates/aish-tools/src/lib.rscrates/aish-tools/src/secure_bash/secure_bash.rscrates/aish-tools/src/system_diagnose/system_diagnose.rs
Strip sudo per compound segment so later segments like `sudo rm` are classified correctly, and treat backslash escapes outside single quotes so separators and redirects cannot be hidden behind escaped quotes.
Strip sudo per segment, handle shell wrappers and subshells recursively, split background jobs on single &, tighten curl detection, and allow read-only sed while blocking -i. Route execute_tool_by_name through shared preflight and inherit tool_execution_policy in subsessions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/aish-tools/src/bash/read_only.rs (3)
199-200: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winBlock
find -execcommand execution.
find . -exec rm -rf {} \;is allowed today because only-deleteis checked, but-exec/-execdir/-okcan run mutating commands.🛡️ Proposed fix
- if base == "find" && segment.to_lowercase().contains("-delete") { - return Some("find delete".into()); + if base == "find" { + let tokens = tokenize_shell_words(segment); + if tokens.iter().any(|t| { + matches!( + t.to_ascii_lowercase().as_str(), + "-delete" | "-exec" | "-execdir" | "-ok" | "-okdir" + ) + }) { + return Some("find mutating or command execution".into()); + } }🤖 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/aish-tools/src/bash/read_only.rs` around lines 199 - 200, The find-command guard in read_only.rs only blocks "-delete", so potentially mutating forms like "-exec", "-execdir", and "-ok" can still slip through. Update the filtering logic in the find validation branch to detect these execution flags in the same place where base == "find" is checked, and return a rejection for any segment containing them so read-only mode blocks command execution consistently.
175-176: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winNormalize shell escapes before matching blocked commands.
Bash resolves quoted/escaped command words before execution, so forms like
r\m -rf /tmp/xor'r'm -rf /tmp/xexecutermbut won’t matchBLOCKEDwith the current rawsplit_whitespace()base extraction.🛡️ Suggested direction
- let first = segment.split_whitespace().next().unwrap_or(""); - let base = first.rsplit('/').next().unwrap_or(first).to_lowercase(); + let tokens = tokenize_shell_words(segment); + let first = tokens.first().map(|t| normalize_shell_word(t)).unwrap_or_default(); + let base = first.rsplit('/').next().unwrap_or(&first).to_lowercase();
normalize_shell_wordshould remove shell quote delimiters and unescaped backslashes outside single quotes before command comparison.🤖 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/aish-tools/src/bash/read_only.rs` around lines 175 - 176, The blocked-command check in `read_only.rs` is comparing the raw first token, so escaped or quoted forms like split shell words can bypass `BLOCKED`. Update the command extraction in the `segment.split_whitespace().next()` / `rsplit('/')` path to normalize the first shell word before matching by removing shell quote delimiters and unescaped backslashes outside single quotes. Apply this normalization in the same flow used for command comparison so the blocked-command lookup sees the executed command name, not the raw text.
148-152: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winBlock process substitution as command execution.
cat <(rm -rf /tmp/x)executes the inner command, but only$(and backticks are detected here, so this can bypass the read-only policy.🛡️ Proposed fix
- if scan_outside_quotes(segment, |window| { - window.starts_with("$(") || window.starts_with('`') - }) { - return Some("command substitution".into()); + if scan_outside_quotes(segment, |window| { + window.starts_with("$(") + || window.starts_with('`') + || window.starts_with("<(") + || window.starts_with(">(") + }) { + return Some("command or process substitution".into()); }🤖 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/aish-tools/src/bash/read_only.rs` around lines 148 - 152, The command-substitution check in the read-only bash parser only recognizes "$(" and backticks, so process substitution like "<(…)" can still execute commands. Update the detection logic in the `scan_outside_quotes` block to also flag process substitution patterns (for example, `<(` and `>(` if applicable) and return the same forbidden-command result so `read_only.rs` blocks these cases consistently.
🤖 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/aish-llm/src/session.rs`:
- Around line 1242-1248: The PreflightSecurityContext::fallback call is passing
one argument too many in the session security setup. Update the fallback
invocation in the security unwrap_or_else block inside session.rs to match the
defined fallback(tool_name, message, mode) signature by removing the extra None,
and make the same correction at the other fallback call site referenced by this
review.
In `@crates/aish-tools/src/bash/read_only.rs`:
- Around line 108-118: The segment parsing in split_compound_segments is
dropping a standalone background operator, which lets trailing background jobs
slip through. Update the logic around the '&' handling so a single '&' is
treated as unsafe instead of being discarded, while preserving '&&' as a normal
compound separator; add or use a has_background_operator check with the same
quote/escape handling as split_compound_segments to detect this in read_only.rs.
---
Outside diff comments:
In `@crates/aish-tools/src/bash/read_only.rs`:
- Around line 199-200: The find-command guard in read_only.rs only blocks
"-delete", so potentially mutating forms like "-exec", "-execdir", and "-ok" can
still slip through. Update the filtering logic in the find validation branch to
detect these execution flags in the same place where base == "find" is checked,
and return a rejection for any segment containing them so read-only mode blocks
command execution consistently.
- Around line 175-176: The blocked-command check in `read_only.rs` is comparing
the raw first token, so escaped or quoted forms like split shell words can
bypass `BLOCKED`. Update the command extraction in the
`segment.split_whitespace().next()` / `rsplit('/')` path to normalize the first
shell word before matching by removing shell quote delimiters and unescaped
backslashes outside single quotes. Apply this normalization in the same flow
used for command comparison so the blocked-command lookup sees the executed
command name, not the raw text.
- Around line 148-152: The command-substitution check in the read-only bash
parser only recognizes "$(" and backticks, so process substitution like "<(…)"
can still execute commands. Update the detection logic in the
`scan_outside_quotes` block to also flag process substitution patterns (for
example, `<(` and `>(` if applicable) and return the same forbidden-command
result so `read_only.rs` blocks these cases consistently.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9eb1d7e5-b369-4013-90de-bd9dde335ef6
📒 Files selected for processing (4)
crates/aish-llm/src/session.rscrates/aish-llm/src/subsession.rscrates/aish-tools/src/bash/bash.rscrates/aish-tools/src/bash/read_only.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/aish-llm/src/subsession.rs
- crates/aish-tools/src/bash/bash.rs
Block trailing background operators, find -exec/-ok, process substitution, and escaped command names; normalize shell words before BLOCKED matching.
This was referenced Jul 7, 2026
This was referenced Jul 14, 2026
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
aish-tools::bash(ReadOnlyVerdict,classify,is_read_only,preflight_enforce), with quote-aware segment parsing and 20+ unit tests.ToolExecutionPolicy.enforce_read_only_bashandTool::preflight_with_contextso bash/secure_bash can block non-read-only commands when policy is enabled./diagnose: verify commands are gated by read-only classification; failure diagnose sub-sessions setenforce_read_only_bash: trueand remove duplicate shell-side readonly logic.react_agentgeneration end/start for spinner stop/restart.Main session behavior is unchanged (
enforce_read_only_bashdefaults tofalse).Test plan
make format-check && make lintcargo test -p aish-tools -p aish-llm -p aish-shell/diagnosewith a non-read-only verify command → blocked with i18n messagerm/echo >→ preflight block/diagnoserun → thinking spinner no longer flickers mid-runSummary by CodeRabbit