Add agent-guard.py hook: block catastrophic bash commands by blast radius#632
Add agent-guard.py hook: block catastrophic bash commands by blast radius#632dkx955 wants to merge 3 commits into
Conversation
A PreToolUse (Bash) hook that denies the handful of irreversible commands that actually destroy machines (rm -rf ~, force-push to main, curl|sh, dd, chmod -R 777, fork bomb) while allowing safe cases like rm -rf node_modules. Complements smart-approve.py: blocks by blast radius, not by keyword, and denies even under --dangerously-skip-permissions. Zero-dep stdlib, fails open, 38-case test suite. CC0/public-domain. Source: https://github.com/dkx955/agent-guard
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Bash PreToolUse guard script, wires it into the hook registry, updates the README hook list, and adds regression tests for blocked and allowed command cases. ChangesAgent Guard Hook
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
hooks/scripts/agent-guard.py (3)
106-106: 📐 Maintainability & Code Quality | 🔵 TrivialPattern 6 is unreachable — already covered by pattern 3.
Pattern 3 (line 93) includes
>\s*/dev/(?:sd|nvme|hd|disk)\w*which matches> /dev/sda,> /dev/sdb, etc. Pattern 6 (>\s*/dev/sd[a-z]\b) is a strict subset and is never reached since patterns are checked sequentially inmain(). Remove it or merge its distinct error message into pattern 3.🤖 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 `@hooks/scripts/agent-guard.py` at line 106, Pattern 6 in agent-guard.py is unreachable because the main() pattern list checks earlier regexes first and pattern 3 already matches the same /dev/sd* redirects. Remove the redundant Pattern 6 entry or fold its message into the existing pattern 3 rule, using the regex tuple and the pattern list in main() as the places to update.
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial
_RM_RECURSIVE_FORCEis defined but never used.This regex is not referenced anywhere in the file —
check_rmuses_RM_FLAGSinstead. It appears to be leftover from an earlier approach. Remove it to avoid confusion for future maintainers.🤖 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 `@hooks/scripts/agent-guard.py` around lines 40 - 43, The _RM_RECURSIVE_FORCE regex in agent-guard.py is unused and appears to be leftover code. Remove the _RM_RECURSIVE_FORCE definition from the module, and keep check_rm relying on _RM_FLAGS only so the guard logic stays clear and maintainable.
60-76: 📐 Maintainability & Code Quality | 🔵 Trivial
check_rmprefilter has a redundant\brm\bcheck.Lines 65-66 and 67-68 both run
re.search(r"\brm\b", seg). When the prefilter block is entered (segment doesn't start with"rm "or contain" rm "), the check at line 65 already confirmed\brm\bmatches, making line 67 redundant. When the prefilter block is skipped (segment starts with"rm "or contains" rm "), line 67 is the first and only check. The logic works but is unnecessarily convoluted — the prefilter block could be simplified to a single early-continue, leaving line 67 as the sole\brm\bgate.♻️ Simplified prefilter logic
def check_rm(cmd: str): for seg in re.split(r"[;\n]|&&|\|\|", cmd): seg = seg.strip() - if not seg.startswith("rm ") and " rm " not in f" {seg} ": - # cheap prefilter - if not re.search(r"\brm\b", seg): - continue if not re.search(r"\brm\b", seg): continue if not _RM_FLAGS.search(seg): continue if _CATASTROPHIC_TARGET.search(seg): return ("This `rm` is recursive/forced and targets your home dir, " "filesystem root, a system dir, or a bare wildcard — the " "single most common way agents destroy a machine. Blocked. " "Delete a specific named subdirectory instead.") return NoneThe string prefilter (
startswith/" rm " in) doesn't meaningfully reduce work compared to a single\brm\bregex on short command segments, so the nested prefilter can be safely dropped.🤖 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 `@hooks/scripts/agent-guard.py` around lines 60 - 76, The `check_rm` prefilter in `agent-guard.py` is redundant because it checks for `\brm\b` twice in the same flow. Simplify `check_rm` by removing the string-based prefilter block and keep a single `re.search(r"\brm\b", seg)` gate before `_RM_FLAGS.search(seg)` and `_CATASTROPHIC_TARGET.search(seg)`. Preserve the existing behavior of returning the same blocked-message from `check_rm` when a recursive/forced `rm` targets dangerous paths.
🤖 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.
Nitpick comments:
In `@hooks/scripts/agent-guard.py`:
- Line 106: Pattern 6 in agent-guard.py is unreachable because the main()
pattern list checks earlier regexes first and pattern 3 already matches the same
/dev/sd* redirects. Remove the redundant Pattern 6 entry or fold its message
into the existing pattern 3 rule, using the regex tuple and the pattern list in
main() as the places to update.
- Around line 40-43: The _RM_RECURSIVE_FORCE regex in agent-guard.py is unused
and appears to be leftover code. Remove the _RM_RECURSIVE_FORCE definition from
the module, and keep check_rm relying on _RM_FLAGS only so the guard logic stays
clear and maintainable.
- Around line 60-76: The `check_rm` prefilter in `agent-guard.py` is redundant
because it checks for `\brm\b` twice in the same flow. Simplify `check_rm` by
removing the string-based prefilter block and keep a single
`re.search(r"\brm\b", seg)` gate before `_RM_FLAGS.search(seg)` and
`_CATASTROPHIC_TARGET.search(seg)`. Preserve the existing behavior of returning
the same blocked-message from `check_rm` when a recursive/forced `rm` targets
dangerous paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24725050-673f-4a0a-b5a1-4d5992f8943d
📒 Files selected for processing (3)
README.mdhooks/hooks.jsonhooks/scripts/agent-guard.py
|
Addressed the three review notes in 9e7e168 and added the previously referenced dependency-free 38-case regression suite. The direct /dev/sdX rule initially looked redundant, but removing it exposed a word-boundary gap in the broader alternation; I fixed the broader raw-device expression instead. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hooks/tests/test_agent_guard.py (1)
57-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider distinguishing crash from deny in
is_blocked.When
returncode != 0, the function returnsTrue(blocked) unconditionally. If the hook crashes with a non-zero exit on aSHOULD_BLOCKcase, the test would incorrectly pass — masking a real bug. The hook is designed to fail open withexit(0), so an unhandled exception (non-zero exit) on a block case would slip through.This is low-risk in practice since the hook's error paths all use
sys.exit(0), but adding a stderr check or distinguishing non-zero exits from valid deny payloads would make the test more robust.♻️ Optional: surface crashes instead of masking them
def is_blocked(command: str) -> bool: event = json.dumps( { "hook_event_name": "PreToolUse", "tool_name": "Bash", "tool_input": {"command": command}, } ) result = subprocess.run( [sys.executable, str(HOOK)], input=event, capture_output=True, text=True, check=False, ) - if result.returncode != 0 or not result.stdout.strip(): - return result.returncode != 0 + if result.returncode != 0: + raise RuntimeError( + f"hook crashed (exit {result.returncode}) on: {command}\n" + f"stderr: {result.stderr.strip()}" + ) + if not result.stdout.strip(): + return False try: payload = json.loads(result.stdout) except json.JSONDecodeError: return False return payload.get("hookSpecificOutput", {}).get("permissionDecision") == "deny"🤖 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 `@hooks/tests/test_agent_guard.py` around lines 57 - 78, The is_blocked helper currently treats any non-zero subprocess return code as a blocked command, which can mask hook crashes as valid denials. Update is_blocked to distinguish a real deny from an execution failure by checking the hook output from HOOK: only return blocked when subprocess.run succeeds with a parseable JSON payload whose hookSpecificOutput.permissionDecision is deny, and otherwise surface non-zero exits or stderr as a test failure/false result instead of automatically passing.
🤖 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.
Nitpick comments:
In `@hooks/tests/test_agent_guard.py`:
- Around line 57-78: The is_blocked helper currently treats any non-zero
subprocess return code as a blocked command, which can mask hook crashes as
valid denials. Update is_blocked to distinguish a real deny from an execution
failure by checking the hook output from HOOK: only return blocked when
subprocess.run succeeds with a parseable JSON payload whose
hookSpecificOutput.permissionDecision is deny, and otherwise surface non-zero
exits or stderr as a test failure/false result instead of automatically passing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d01619ae-f9b8-40fc-a3f0-03eb9ea0f683
📒 Files selected for processing (2)
hooks/scripts/agent-guard.pyhooks/tests/test_agent_guard.py
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/scripts/agent-guard.py
A non-zero exit from the hook now raises instead of passing as a deny, so an unhandled exception on a SHOULD_BLOCK case can't mask a real bug. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Applied the remaining review note in f9828fb: a hook crash (non-zero exit) now raises in the test helper instead of counting as a deny, so a crash on a SHOULD_BLOCK case can't silently pass. All 38 cases still green. From my side the PR is ready for maintainer review — happy to adjust scope if you'd prefer it slimmer. |
1 participant
This adds one PreToolUse (Bash) hook,
agent-guard.py, tohooks/.What it does: denies the small set of irreversible commands that actually destroy machines —
rm -rf ~, force-push tomain,curl | sh,ddonto a disk,chmod -R 777, fork bombs — while allowing everyday safe cases likerm -rf node_modules.How it differs from the existing
smart-approve.py: it blocks by blast radius, not by keyword.rm -rf node_modulesis allowed;rm -rf ~,/,$HOME,../, system dirs, and bare trailing*are blocked. It also denies even under--dangerously-skip-permissions(a PreToolUsedenyoverrides that flag). Complementary tosmart-approve.py, not a duplicate.Quality: pure-Python stdlib (zero deps), fails open (never bricks a session), and ships with a 38-case test suite (23 blocked / 15 allowed) in the source repo. Licensed CC0 / public domain.
Changes in this PR:
hooks/scripts/agent-guard.py— the hook (one file)hooks/hooks.json— new entryREADME.md— new row in the Hook Scripts table + count bumpSource & tests: https://github.com/dkx955/agent-guard
Smoke test:
Summary by CodeRabbit
New Features
Documentation
Tests