fix(hooks): resolve deletion targets in block-rm-rf

[Nitjsefnie]

blockRmRf decided whether a command was catastrophic by pattern-matching the raw command
text: a token was destructive only if it was exactly /, exactly ~, or a single-segment
absolute path (^/[A-Za-z_][\w.-]*$), and only the literal command word rm was known. So
the shape of the text, not the target the shell would actually resolve, drove the verdict.

This replaces the text match with target resolution. For each shell segment we ask two
questions: is this a recursive delete? and what does it delete?

• Is it a recursive delete? rm with both -r and -f (any spelling/order, including
  /bin/rm), or find with -delete / -exec rm (find recurses by design).
• What does it delete? The path operands, resolved as far as a shell would: quotes
  stripped, ~ / ~user / $HOME / ${HOME} recognised as a home root, trailing /* and
  slashes dropped, then the segments below the root counted.

Bypass forms from the issue — now blocked

Command	Resolves to	Verdict
rm -rf $HOME / rm -rf ${HOME} / rm -rf "$HOME"	home root	deny
rm -rf $HOME/Documents	1 segment below home	deny
rm -rf ~/Documents	1 segment below home	deny
rm -rf ~chetan	another user's home root	deny
rm -rf /home/chetan	2 segments below /	deny
rm -rf /var/lib	2 segments below /	deny
find / -delete	/	deny
find / -exec rm -rf {} \;	/	deny
find $HOME -delete	home root	deny
find -L /home/chetan -delete	2 segments below /	deny
rm -rf $(echo /)	unresolvable → fail safe	deny
/bin/rm -rf /home/chetan	2 segments below /	deny

The baselines the issue lists as already-correct (rm -rf /, rm -rf /etc, rm -rf ~,
rm -rf /*) stay denied, and every pre-existing block-rm-rf test passes unchanged.

The blocked-vs-allowed boundary

Blocked — a recursive delete whose resolved target is:

• within 2 segments of a root, where a root is / or a home directory
  (~, ~user, $HOME, ${HOME}). That covers /, /etc, /home/chetan, /var/lib,
  ~, ~/Documents, ~/dev. These are what take out the machine or the user's data.
• unresolvable: the token's head is command substitution ($(…), backticks) or a
  variable other than $HOME (rm -rf $BUILD_DIR). It could expand to /, we cannot know,
  so we deny. This is the deliberate fail-safe, and it is the one place the policy trades
  false positives for safety ($BUILD_DIR is the shape of the classic rm -rf "$STEAMROOT/"*
  bug). allowPaths is the escape hatch.

Allowed — ordinary work:

• relative targets (./dist, node_modules, build/, dist/$VERSION) — they resolve under
  the working directory, not under a root;
• anything 3+ segments below a root (/home/chetan/myapp/dist, ~/dev/myapp/node_modules);
• anything inside a scratch root (/tmp/…, /var/tmp/…) — deleting /tmp itself is
  still denied;
• non-recursive rm, and find without a delete expression (find / -name '*.log').

Two consequences worth flagging for review:

1. Deliberate asymmetry. /home/chetan/project (3 segments) is allowed, but ~/project
   (1 segment below home) is denied — the same directory. This is forced, and I think
   correctly: the existing test allows rm -rf /home/user/project (depth-2+ path is not flagged) fixes the first, and the issue explicitly requires the second. Treating ~ as a
   root in its own right is what satisfies both. It also keeps the rule machine-independent —
   we never compare a typed path against the actual homedir() to decide destructiveness,
   so verdicts don't shift with $HOME.
2. The issue's wording ("home-relative deletes should be treated like deletes of the home
   dir") read literally would deny every ~/… path, including ~/dev/myapp/node_modules.
   I narrowed it to the same 2-segment rule used for /, on the grounds that a policy that
   blocks rm -rf ~/dev/myapp/node_modules gets turned off. Happy to widen if you disagree.

Side effects of resolving per segment rather than scanning the whole command:

• ls /etc && rm -rf ./dist is now allowed (previously the /etc in the ls flagged the
  command and the unrelated rm was denied).
• allowPaths entries may now be written home-relative (~/scratch); target and allowPath
  are expanded on both sides before comparison.

What is out of reach, and why

Not attempted — a policy hook cannot be a shell, and guessing here would be worse than
failing safe:

• Command substitution / arbitrary indirection — rm -rf $(cat target), `pwd`,
  $BUILD_DIR. Not evaluated (that would mean running the command). Denied instead.
• Aliases, functions, eval, sh -c "…" — the command word we see is not the command
  that runs. eval $CMD is invisible to any static check.
• Pipelines into a deleter — find / -type f | xargs rm -rf has no target token on the
  rm; the reach comes from stdin. Detecting this needs dataflow across pipeline stages.
  Not in the issue's list; happy to take it in a follow-up.
• Relative escapes — rm -rf ../../.., or cd / earlier in the chain. Verdicts would
  depend on the working directory, which the policy does not resolve.
• Deep system paths — rm -rf /usr/local/lib (3 segments) is allowed. Blocking it means
  either a curated list of system roots or dropping the depth rule, and the latter would
  block /home/chetan/myapp/dist. The 2-segment line is the tradeoff the existing tests
  already encode.
• Variables mid-token — /opt/foo/$X is treated as its literal depth; $X=../.. would
  escape. Only a token's head is checked for unresolvable expansion, otherwise every
  rm -rf dist/$VERSION would be denied.

Tests

__tests__/hooks/builtin-policies.test.ts, in the existing style: a table of the 15 bypass
forms above (each asserted deny), two fail-safe cases ($BUILD_DIR, backticks), 11
ordinary-delete cases asserted allow (relative paths, build/, a temp dir, deep home and
absolute paths, find without -delete, non-recursive rm), and 5 new allowPaths cases
covering home-relative allowlisting and find. No existing test was relaxed, deleted or
weakened. None of these commands are executed — the tests assert the policy's verdict on
command strings.

Gates

Gate	Result
bun run lint	pass — 0 errors, 5 warnings (all pre-existing, none in the touched files)
bunx tsc --noEmit	pass — clean
bun run test:run	2098 passed, 1 failed — the failure is pre-existing and environmental: integrations.test.ts > Pi integration > writeHookEntries… asserts a resolved package path contains failproofai, which fails because my checkout directory is not named failproofai. Verified it fails identically on unmodified main (git stash). All 459 builtin-policies tests pass.
bun run build	pass

Scope: block-rm-rf only — src/hooks/builtin-policies.ts + its test file. No other policy's
behaviour changed.

Closes #520

Prepared with AI assistance (Claude Opus 4.8), human-reviewed before submission.

[coderabbitai]

Warning

Review limit reached

@Nitjsefnie, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fd008746-0440-4986-99ff-b16787c03b43

📥 Commits

Reviewing files that changed from the base of the PR and between 446484b and 7c17147.

📒 Files selected for processing (2)

• __tests__/hooks/builtin-policies.test.ts
• src/hooks/builtin-policies.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[hermes-exosphere]

Automated code review started - full review. Results will be posted here.

[hermes-exosphere]

⏳ Phase 0-1 complete. Read existing comments (2 bot comments, 0 human reviews, 0 review threads). Analyzing 2 files changed (+262/-46 lines in blockRmRf and tests). Starting Phase 3: Build & Test.

[hermes-exosphere]

✅ Build & test complete.

Gate	Result
bun run lint	0 errors, 5 warnings (pre-existing)
bunx tsc --noEmit	clean
bun run test:run	2099 passed, 121 files
bun run build	pass

Starting Phase 4: Posting detailed review.

[hermes-exosphere]

🔍 Automated Code Review

📋 Executive Summary

This PR replaces block-rm-rf's naive text-pattern matching with token-level target resolution — a significant security upgrade. It correctly identifies recursive deletes (rm -rf, find -delete) and resolves path operands through $HOME, ~, and ${HOME} expansion before applying a 2-segment depth rule below root. The implementation is well-structured, thoroughly tested (33 new test cases, all 2099 tests pass), and comes with clearly documented tradeoffs and limitations. No breaking changes to existing behavior — all pre-existing tests pass unchanged.

---

📊 Change Architecture

graph TD
    A["blockRmRf() — entry point"] --> B["shellSegments() — split on &&, ||, ;, |"]
    B --> C["recursiveDeletionTargets() — per-segment parsing"]
    C --> D["isCatastrophicTarget() — depth check"]
    D --> E["deletionTargetIsAllowed() — allowPaths fallback"]
    E --> F["VERDICT: deny/allow"]
    
    C -->|"rm handler"| C1["RM_CMD_RE matches /bin/rm too"]
    C -->|"find handler"| C2["parses -exec rm and -delete"]
    D --> D1["HOME_PREFIX_RE: ~, $HOME, ${HOME}"]
    D --> D2["CATASTROPHIC_DEPTH = 2"]
    D --> D3["SCRATCH_ROOTS: /tmp, /var/tmp"]
    
    style A fill:#87CEEB
    style C fill:#90EE90
    style D fill:#90EE90
    style D1 fill:#90EE90
    style D2 fill:#FFD700
    style E fill:#87CEEB

Loading

Legend: 🟢 New | 🔵 Modified | 🟡 Configurable constant

---

🔴 Breaking Changes

✅ No breaking changes detected. All pre-existing block-rm-rf tests pass unchanged. The policy's interface and behavior for all previously-handled cases are preserved.

---

⚠️ Issues Found

1. 🔵 Info — src/hooks/builtin-policies.ts:678 — recursiveDeletionTargets only matches literal find, not absolute-path invocations like /usr/bin/find. RM_CMD_RE already supports absolute paths for rm; find should have parity.

2. 🔵 Info — src/hooks/builtin-policies.ts:652 — Quote stripping is single-level only (replace(/^['"]|['"]$/g, "")). Nested quotes like "'$HOME'" would be handled correctly by subsequent checks, but this is a subtlety worth documenting.

---

🔬 Logical / Bug Analysis

Design soundness: The token-level resolution is the correct approach. Key design decisions are well-reasoned:

• 2-segment depth rule — strikes the right balance between safety and usability
• Home-as-root treatment — prevents verdicts from shifting with $HOME
• Unresolvable → deny — correct fail-safe for $(...) and unknown variables
• Per-segment isolation — ls /etc && rm -rf ./dist is now allowed (was previously denied)

Scratch root handling (line 663): The check !homePrefix && SCRATCH_ROOTS.some(...) correctly prevents home-relative paths like ~/tmp from being exempted by the scratch-root logic.

Edge: deletionTargetIsAllowed returns sawRecursiveDelete (line 746): If a segment has a recursive delete with all targets allowed, but a different segment has a non-recursive rm, the function correctly returns true. Non-recursive rm segments are intentionally not validated.

No regressions: The function rename from rmTargetIsAllowed to deletionTargetIsAllowed and shellSegments extraction are clean refactors.

---

🧪 Evidence — Build & Test Results

Build Output

bun run build: pass
├ ƒ /api/audit/status
├ ƒ /api/auth/login-request
...
└ ƒ /projects
[prune-standalone] 2287 files / 65.33 MB -> 1521 files / 49.19 MB

Test Results

bun run test:run
Test Files  121 passed (121)
     Tests  2099 passed (2099)
  Duration  86.86s

bun run lint: 0 errors, 5 warnings (all pre-existing, none in touched files)
bunx tsc --noEmit: clean

---

🔗 Issue Linkage

Closes #520 — "Bypass forms in block-rm-rf policy." The PR body documents 15 bypass forms from the issue, each now correctly denied. All 15 are tested.

---

👥 Human Review Feedback

No human review comments on this PR. The only previous comments are:

• coderabbitai[bot]: Review failed (infrastructure error, not substantive)
• hermes-exosphere: Review started marker

---

💡 Suggestions

1. Consider find absolute-path parity (see inline comment at line 678). Adding FIND_CMD_RE analogous to RM_CMD_RE would close the minor asymmetry where /usr/bin/find / -delete is not recognized.

2. Test for find with -ok rm — the regex on line 603 includes ok and okdir, but no test exercises find / -ok rm -rf {} \;. A test would validate the regex and prevent future regressions.

3. Document the deletionTargetIsAllowed edge — when sawRecursiveDelete is false (e.g., only non-recursive rm in the command), the function returns false. This is correct but non-obvious from the name alone.

---

🏆 Verdict

VERDICT: APPROVED_WITH_SUGGESTIONS

---

Automated code review · 2026-07-16 12:43 UTC

[hermes-exosphere]

Automated review: Approved with suggestions. ✅ See the summary comment for details and 2 inline comments for minor improvements.

[hermes-exosphere]

Automated review: Approved. ✅

[Nitjsefnie]

Both good catches — took the first, documented the second.

find by absolute path — fixed in the latest commit. You're right, and it was an inconsistency in my own diff: RM_CMD_RE already accepted /bin/rm, but the find branch tested t === "find" literally. So the policy blocked find / -delete and waved through /usr/bin/find / -delete — same command, same blast radius, different spelling. Added FIND_CMD_RE with RM_CMD_RE's exact shape, plus two cases in the bypass table:

["absolute find binary", "/usr/bin/find / -delete"],
["absolute find binary with -exec rm", "/usr/bin/find /home/chetan -exec rm -rf {} \;"],

Both fail without the change and pass with it; full suite 461/461, tsc clean.

Nested quote stripping — agreed it's single-level, leaving it. Your read matches mine: a mixed-quoted token gets its outer pair removed only, and the unresolvable-expansion check catches what survives, so it fails safe. I'd rather not add a quoting mini-parser to a security policy for a case that currently lands on "deny" anyway — more parser is more surface. The comment now says single-level explicitly so the next reader doesn't assume otherwise.

Worth saying plainly: the boundary in the PR body is where the judgement lives, and I'd rather you overrule it than have it merged on my say-so. Particularly the ~/dev/app/node_modules-stays-allowed call — that's a one-constant change if you want the issue's literal "all ~/…" reading instead.

[hermes-exosphere]

Automated code review started - full review. Results will be posted here.

[hermes-exosphere]

Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.gg/qaFM2uYFb

[hermes-exosphere]

✅ Resolved 2 previously-reported issues that have been addressed in this update:

1. find by absolute path — /usr/bin/find / -delete now matches via FIND_CMD_RE (commit 7c17147)
2. Quote stripping comment — documented that stripping is single-level only (as author confirmed)

[hermes-exosphere]

🔍 Automated Code Review (Re-Review)

📋 Executive Summary

PR #531 replaces block-rm-rf's naive text-pattern matching with token-level target resolution — a significant security upgrade that closes 15 documented bypass forms (#520). The implementation resolves shell tokens as the shell would (quote stripping, ~/$HOME expansion), counts path segments to distinguish catastrophic deletes from ordinary work, handles find -delete/-exec rm paths, and introduces a fail-safe for unresolvable expansions. Code quality is high: well-factored, thoroughly documented, and the boundary decisions are explicitly reasoned in comments. No blocking issues found.

---

📊 Change Architecture

graph TD
    A[blockRmRf entry point] --> B[shellSegments: split on && | ; \n]
    B --> C[recursiveDeletionTargets: per segment]
    C --> D{rm with -r and -f?}
    C --> E{find with -delete/-exec rm?}
    D -->|yes| F[extract path args]
    E -->|yes| G[extract find path operands]
    F --> H[isCatastrophicTarget: per target]
    G --> H
    H --> I{home-relative? expandHomePrefix}
    H --> J{absolute? count segments}
    H --> K{unresolvable $VAR/\$()/\`? deny}
    J --> L{segments <= 2? catastrophic}
    I --> L
    L -->|yes| M{allowPaths cover?}
    M -->|no| N[deny: Catastrophic deletion blocked]
    M -->|yes| O[allow]
    L -->|no| O
    
    style C fill:#90EE90
    style H fill:#90EE90
    style E fill:#FFD700
    style K fill:#FFD700

Loading

Legend: 🟢 New | 🔵 Modified | 🟡 Breaking Change Risk (boundary adjustment)

---

🔴 Breaking Changes

✅ No breaking changes detected. All existing tests pass unchanged. The policy boundary has shifted slightly (relative paths are now never flagged, per-segment resolution means /etc in ls /etc && rm -rf ./dist no longer blocks the rm), but these are all improvements and deliberately backward-compatible.

---

⚠️ Issues Found

1. 🟡 Info — src/hooks/builtin-policies.ts:655 — Quote stripping is single-level only (previously flagged, acknowledged by author). A token like "'$HOME'" (outer single-quotes wrapping double-quoted $HOME) would strip only the outer layer, leaving "$HOME" for HOME_PREFIX_RE to catch. This works for all practical shell cases. Already documented in the comment on line 655.

2. 🔵 Info — src/hooks/builtin-policies.ts:690-691 — FIND_GLOBAL_OPT_RE with -D branch: start += expr[start] === "-D" ? 2 : 1. This correctly skips 2 tokens for find -D tree /path -delete (where -D takes an argument), but -D without an argument in some shells can be a debug flag with no following token. In the pathological case find -D -delete /, this would skip -delete as if it were the argument to -D, missing the delete expression. This is extremely unlikely in practice (nobody combines -D with destructive operations) and the fail-safe would still catch an unresolvable target afterwards.

---

🔬 Logical / Bug Analysis

Phase 2 Deep Analysis — all checks passed:

Check	Result
SQL injection / shell injection	✅ Clean
Hardcoded secrets / credentials	✅ Clean
Race conditions	✅ N/A (synchronous policy hook)
Null/empty checks	✅ Well-handled: raw === "" guard, ?? "" on expr[execIdx+1]
Resource leaks	✅ N/A (pure function, no I/O)
Off-by-one errors	✅ Segment counting verified (simulated all edge cases)
Infinite loops	✅ None — while bounded by token array length
Variable shadowing	✅ Clean
Wrong function calls	✅ All correct
Copy-paste errors	✅ No stale references to old variable names
Missing error handling	✅ homedir() call is always safe; no other fallible calls

Specific edge cases verified manually:

• /tmp itself → catastrophic ✓
• /tmp/foo → allowed (inside scratch root) ✓
• /var/tmp itself → catastrophic ✓
• $HOME → catastrophic ✓
• ~/dev/myapp/node_modules (3 segments) → allowed ✓
• $BUILD_DIR → unresolvable, denied ✓
• `pwd` → unresolvable, denied ✓
• $(echo /) → unresolvable, denied ✓
• find / -name '*.log' (no -delete) → not a recursive delete ✓
• Absolute path in unrelated segment (ls /etc && rm -rf ./dist) → allowed ✓

---

🧪 Evidence — Build & Test Results

Gate	Result
bun run lint	0 errors, 5 warnings (all pre-existing, none in touched files)
bunx tsc --noEmit	clean
bun run test:run	2101 passed, 121 files, 0 failures
bun run build	pass

All 459 builtin-policies tests pass, including all 32 new tests covering bypass forms, ordinary deletes, fail-safe cases, and allowPaths expansions.

---

🔗 Issue Linkage

This PR closes #520 — "Bypass forms in block-rm-rf". The 15 bypass forms listed in the PR body map directly to the 16 deny-test entries in the test table (+1 for "absolute find binary with -exec rm" which was added as part of the prior review fix). Changes fully address the issue.

---

👥 Human Review Feedback

• @Nitjsefnie (PR author): Confirmed both previous bot review items were addressed — committed the find absolute-path fix and documented the quote-stripping limitation. Both resolved.
• No external human reviewers yet.

---

💡 Suggestions

• 🟢 Minor: Consider adding a test for the pathological find -D -delete / case (though extremely unlikely in practice). The risk is that -D without an argument causes -delete to be skipped as an expression token.
• 🟢 Optional: The FIND_GLOBAL_OPT_RE regex could be extended to support -O0/-O1/-O2/-O3 optimization flags (currently -O\d* only). -Olevel without a digit separator (e.g., find -O2) is covered, but -O followed by space+level is not a global option — it's an expression. So this is correct as-is.

---

🏆 Verdict

VERDICT: APPROVED

---

Automated code review · 2026-07-16 18:40:00 UTC

[hermes-exosphere]

Automated re-review: All 2101 tests pass, lint is clean, TypeScript compiles with no errors. Both previously-reported issues have been addressed (find absolute-path matching + quote-stripping documentation). The target-resolution logic is well-factored and correctly handles all documented bypass forms from #520. No blocking issues found. ✅

[hermes-exosphere]

Automated review: Approved. ✅

[hermes-exosphere]

Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.befailproof.ai

[hermes-exosphere]

Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai