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 help to get the list of available commands.}

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

Review failed: Could not clone repository.

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

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

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

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

Phase 2 in progress - deep code analysis. Detected a focused security-focused PR: extends block-rm-rf with critical path protection. Continuing to Phase 3 (build + test) next.

🔍 Automated Code Review

📋 Executive Summary

This PR extends the block-rm-rf builtin policy to recognize critical recursive-delete targets that were previously missed: $HOME, ${HOME}, home-relative paths (~/Documents), user home directories (/home/user), system directories (/var/lib, /usr/local), and find / -delete. The refactoring extracts a clean isCriticalRecursiveDeletePath helper and adds 6 targeted test cases. Code quality is good — linter and TypeScript pass cleanly, all 37 block-rm-rf tests pass. Three moderate findings noted below; none are blocking.

📊 Change Architecture

graph TD
    A["blockRmRf() policy"] -->|"extracts"| B["isCriticalRecursiveDeletePath()"]
    B --> C["Exact matches: / , ~ , $HOME , ${HOME}"]
    B --> D["Prefix check: ~/ paths"]
    B --> E["Regex: single-segment /X paths"]
    B --> F["Regex: /home/user, /Users/user"]
    B --> G["Regex: /var/lib, /var/log, /usr/lib, /usr/local"]
    A --> H["Raw regex: find PATH -delete"]
    A --> I["Existing: rm flag detection (combined + separated)"]
    style B fill:#90EE90
    style H fill:#FFD700

Legend: 🟢 New | 🟡 Risk (regex-based, no token parsing)

🔴 Breaking Changes

✅ No breaking changes detected. This is a pure additive extension to an existing policy — nothing removed, no API changed, no config modified.

⚠️ Issues Found

1. 🟡 Warning — src/hooks/builtin-policies.ts:635 — Path normalization missing multiple-slash collapse (see inline comment)
2. 🟡 Warning — src/hooks/builtin-policies.ts:651 — find path detection uses raw regex instead of parsed tokens (see inline comment)
3. 🟡 Warning — src/hooks/builtin-policies.ts:640 — Missing $HOME/ and ${HOME}/ relative-path startsWith checks (see inline comment)

🔬 Logical / Bug Analysis

What works well:

• Extracting isCriticalRecursiveDeletePath is clean refactoring. It's a pure function, easy to test in isolation, and reused correctly.
• The test coverage additions are targeted and effective. Each new test case maps directly to a bypass documented in issue block-rm-rf waves through several catastrophic-deletion forms #520.
• The allowPaths integration works correctly — the test for ~/scratch with allowPaths: ["~/scratch"] confirms the allowlist path is preserved.
• ESLint and TypeScript pass without errors.

What needs attention:

• The find check still uses raw-command regex matching instead of the parsed-token approach used for rm. This creates both false positives (triggering on unrelated strings) and bypasses (e.g., find // with multi slashes, find ... -exec rm -rf).
• Path normalization only strips trailing slashes but doesn't collapse internal multiple slashes (//usr bypass).
• startsWith("~/") is checked but not startsWith("$HOME/") or startsWith("${HOME}/") — though in practice this may be less critical since $HOME/Documents gets split into two tokens by parseArgvTokens.

No logic errors found:

• No race conditions (sync code only)
• No null/undefined issues (parseArgvTokens always returns array, fallback handles empty)
• No off-by-one issues
• No resource leaks

🧪 Evidence — Build & Test Results

RUN  v4.1.10 /tmp/pr524

 ✓ __tests__/hooks/builtin-policies.test.ts (431 tests | 394 skipped) 21ms
   block-rm-rf: 37 passed, 0 failed

 Test Files  1 passed (1)
      Tests  37 passed | 394 skipped (431)
   Start at  13:47:41
   Duration  1.18s

Full test suite: 2070 passed, 1 failed (pre-existing in Pi integration)

bunx eslint src/hooks/builtin-policies.ts __tests__/hooks/builtin-policies.test.ts
→ exit 0 (clean)

bunx tsc --noEmit
→ exit 0 (clean)

Docker smoke test (package build): ✅ npm pack succeeded, Next.js build completed.

🔗 Issue Linkage

This PR addresses #520 ("block-rm-rf waves through several catastrophic-deletion forms"). Changes directly close 3 of the 4 reported bypasses (rm -rf on home vars/home dirs/system dirs, find path with -delete) and partially address the rm -rf via command substitution (still acknowledged as out of scope for this pass, as noted in the issue).

👥 External Review Feedback

• @coderabbitai (2 unresolved threads, both on src/hooks/builtin-policies.ts):
  • Thread 1 (line 644): Path normalization — slashes + $HOME/ coverage → ⚠️ Still present in current code. This review re-identifies the same issue at the updated line 635.
  • Thread 2 (line 653): find -delete raw regex → ⚠️ Still present. This review re-identifies this at line 651.
  • Both CodeRabbit suggestions remain unaddressed in this commit.

No human reviewer feedback found on this PR.

💡 Suggestions

1. Consider find ... -exec rm -rf — Currently only -delete is checked, but -exec rm -rf {} \; and -exec rm -rf {} + have the same catastrophic effect. A follow-up could extend find checking to cover these patterns too.
2. Consider a follow-up for command substitution — rm -rf $(echo /) remains unaddressed (the PR body acknowledges this as out of scope), but it's documented in issue block-rm-rf waves through several catastrophic-deletion forms #520.
3. Test for false positives — Add a test confirming that echo "find / with cleanup to delete" is NOT blocked, to validate future token-based improvements.

🏆 Verdict

VERDICT: APPROVED_WITH_SUGGESTIONS

Automated code review · 2026-07-17 08:24:52 UTC

#!/bin/bash
set -euo pipefail

ast-grep outline src/hooks/builtin-policies.ts --items all --match 'blockRmRf|isCriticalRecursiveDeletePath|parseArgvTokens' --view expanded
printf '\n--- Relevant policy definitions and call sites ---\n'
rg -n -C 12 'function (isCriticalRecursiveDeletePath|blockRmRf|parseArgvTokens)|const (isCriticalRecursiveDeletePath|blockRmRf|parseArgvTokens)|\b(parseArgvTokens|isCriticalRecursiveDeletePath)\s*\(' src/hooks/builtin-policies.ts

printf '\n--- Focused block-rm-rf tests ---\n'
rg -n -C 8 'block-rm-rf|find .*-(delete|exec)|\$HOME|~/' __tests__/hooks/builtin-policies.test.ts

printf '\n--- parseArgvTokens definitions outside the policy file ---\n'
rg -n -C 6 '\b(parseArgvTokens)\s*[=:<(]' -g '*.{ts,tsx,js,mjs,cjs}' .

@hermes-exosphere Confirmed: the three warnings are valid in the current implementation.

• Repeated-slash bypass: //usr is not canonicalized to /usr, so rm -rf //usr misses the critical-path matcher.
• Home-relative environment paths: parseArgvTokens() preserves $HOME/Documents and ${HOME}/Documents as single tokens, so neither is caught without explicit prefix checks.
• find false positives: the whole-command regex can match quoted text, e.g. echo "find / -delete", rather than a real find invocation.

The suggested find / -exec rm -rf … case is already denied when its path is /, because the existing rm detection sees both / and the recursive/force flags. It is still reasonable to improve command-aware parsing as broader hardening.

The targeted repairs should canonicalize repeated separators, recognize $HOME/ and ${HOME}/ prefixes, and derive find’s first path argument plus -delete from tokens rather than matching the raw command string.