Skip to content

fix(rules-rm): allow temp targets when cwd is home directory#42

Merged
kenryu42 merged 1 commit into
mainfrom
fix/rm-home-cwd-classification
Mar 25, 2026
Merged

fix(rules-rm): allow temp targets when cwd is home directory#42
kenryu42 merged 1 commit into
mainfrom
fix/rm-home-cwd-classification

Conversation

@kenryu42
Copy link
Copy Markdown
Owner

@kenryu42 kenryu42 commented Mar 25, 2026

Summary

  • Fix rm -rf classification when cwd is the home directory to correctly allow temp targets (e.g. /tmp/..., /var/tmp/..., $TMPDIR/...)
  • Move home-directory-cwd check from analyzeSegment/explainSegment into classifyTarget in rules-rm.ts, so the decision respects target classification order (temp targets checked first)

Changes

  • src/core/rules-rm.ts: Added home_cwd_target variant to TargetClassification union. Refactored classifyTarget to check isTempTarget before isCwdHomeForRmPolicy, so temp targets are allowed even when cwd is $HOME. Added REASON_RM_HOME_CWD constant and corresponding reasonForClassification case. Exported isHomeDirectory for testing.
  • src/core/analyze/segment.ts: Removed the inline isHomeDirectory pre-check and REASON_RM_HOME_CWD constant; the logic is now handled inside analyzeRm via classifyTarget.
  • src/bin/explain/segment.ts: Removed the separate isHomeDirectory explain step; the explain flow now delegates to analyzeRm which covers the home-cwd case internally.
  • tests/core/rules-rm.test.ts: Added tests for temp targets (/tmp, /var/tmp, $TMPDIR) being allowed when cwd is home, mixed temp + relative targets being blocked, and updated the rm -rf cwd itself test to reflect that a temp-path cwd-self target is now allowed. Updated analyzeRm unit test expectation from 'extremely dangerous' to 'home directory'.
  • tests/bin/explain/command.test.ts: Updated explain trace assertions to expect analyzeRm rule step instead of the removed isHomeDirectory step. Added test for temp-target rm in home-directory cwd being allowed.

Testing

bun run check

Related Issue

#41

PR Checklist

  • I have read the CONTRIBUTING.md
  • Code follows project conventions (type hints, naming, etc.)
  • bun run check passes (lint, types, dead code, rules, tests)
  • Tests added for new rules (minimum 90% coverage required)
  • Tested locally with Claude Code, OpenCode, Gemini CLI or GitHub Copilot CLI
  • Updated documentation if needed (README, AGENTS.md)
  • No version changes in package.json

Summary by CodeRabbit

  • Refactor
    • Reorganized rm command safety analysis to use a unified processing flow with refined context variables.
    • Introduced improved classification logic for distinguishing home directory targets from other target types, enabling more precise risk assessment.
    • Streamlined the pre-check phase by consolidating detection logic into the primary analysis handler.

Consolidate home-cwd classification into analyzeRm by introducing a
dedicated 'home_cwd_target' kind. Temp targets are now checked before
the home-cwd guard so rm -rf /tmp/... is allowed even when cwd is HOME.
Remove the duplicate pre-checks from analyzeSegment and explainSegment.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 92fbbcf7-8ed1-406d-bd97-a0571f5e3286

📥 Commits

Reviewing files that changed from the base of the PR and between e87a8bc and d157dd6.

⛔ Files ignored due to path filters (3)
  • dist/bin/cc-safety-net.js is excluded by !**/dist/**
  • dist/core/rules-rm.d.ts is excluded by !**/dist/**
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (5)
  • src/bin/explain/segment.ts
  • src/core/analyze/segment.ts
  • src/core/rules-rm.ts
  • tests/bin/explain/command.test.ts
  • tests/core/rules-rm.test.ts

📝 Walkthrough

Walkthrough

The PR refactors rm home-directory detection by removing early pre-checks from segment analyzers and consolidating the logic into the core rules module with a new home_cwd_target classification, while updating context variables passed to the analyzeRm function.

Changes

Cohort / File(s) Summary
Segment Analyzers
src/bin/explain/segment.ts, src/core/analyze/segment.ts
Removed early pre-check for rm with recursive force flags in home directory. Deleted imports (hasRecursiveForceFlags, isHomeDirectory) and constant REASON_RM_HOME_CWD. Updated context variables passed to analyzeRm to use cwdForRm and originalCwd.
Core Rule Logic
src/core/rules-rm.ts
Introduced new classification category home_cwd_target distinct from root_or_home_target. Added REASON_RM_HOME_CWD and updated reasonForClassification to detect home-directory targets. Refined classifyTarget control flow to classify home-directory targets before self-targets when anchoredCwd is set. Exported isHomeDirectory with testing annotation.
Test Updates
tests/bin/explain/command.test.ts, tests/core/rules-rm.test.ts
Updated home-directory test expectations: changed rule-check step to reference analyzeRm instead of isHomeDirectory. Added new test case for rm -rf /tmp/test-dir from home directory (allowed). Added cwd-aware test cases validating temp-directory targets and adjusted "cwd itself" behavior assertions.

Possibly Related PRs

  • Fix/windows path separator #27: Directly modifies the same rm-rule logic in src/core/rules-rm.ts, particularly around isHomeDirectory and target classification behavior.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit's tale of reorganized code,
Where home-directory checks found a new abode—
From segments they roamed, to rules they now stay,
Classified, tested, and safer each day!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing rm classification to allow temp targets when cwd is home directory.
Description check ✅ Passed The description comprehensively covers all required template sections with detailed changes, testing instructions, checklist completion, and related issue reference.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/rm-home-cwd-classification

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.51%. Comparing base (864445a) to head (d157dd6).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
- Coverage   99.51%   99.51%   -0.01%     
==========================================
  Files          50       50              
  Lines        5379     5362      -17     
==========================================
- Hits         5353     5336      -17     
  Misses         26       26              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Mar 25, 2026

Greptile Summary

This PR fixes a bug where rm -rf /tmp/... (and other temp targets) was incorrectly blocked when the current working directory happened to be the user's home directory. The fix moves the isTempTarget check ahead of both isCwdHomeForRmPolicy and isCwdSelfTarget inside classifyTarget, ensuring the safer temp-path allowance takes priority over the home-cwd restriction. A new home_cwd_target classification is introduced to replace the previous reuse of root_or_home_target, giving a more accurate and actionable error message.

  • classifyTarget ordering fix (src/core/rules-rm.ts): isTempTarget is now evaluated before isCwdHomeForRmPolicy and isCwdSelfTarget. As a side effect, rm -rf <temp-path> is now correctly allowed even when cwd is the home directory or when cwd itself is the temp path (e.g. rm -rf /tmp/xyz with cwd=/tmp/xyz).
  • New home_cwd_target classification: replaces the old overloaded root_or_home_target return for the home-cwd case, with a dedicated REASON_RM_HOME_CWD message ("Change to a project directory first") that is more actionable than "extremely dangerous".
  • explainSegment alignment (src/bin/explain/segment.ts): The separate isHomeDirectory pre-check in the explain path is removed; analyzeRm is now called with cwdForRm and originalCwd instead of the previous effectiveCwd ?? undefined — fixing a subtle inconsistency where the explain path didn't propagate originalCwd.
  • Test coverage: new tests cover /tmp, /var/tmp, $TMPDIR targets allowed in home-cwd, mixed temp+relative targets blocked, and cwd-self temp-path allowed.

Confidence Score: 5/5

  • This PR is safe to merge — the logic change is intentional, well-scoped, and fully tested.
  • The ordering fix in classifyTarget is correct: temp targets must be evaluated before the home-cwd guard to avoid false positives. The new home_cwd_target classification is cleaner than reusing root_or_home_target and provides a better user message. The explain path is now properly aligned with the analyze path (passing originalCwd). All edge cases — mixed targets, cwd-self temp paths, $TMPDIR, /var/tmp — are covered by new tests, and bun run check is reported passing.
  • No files require special attention.

Important Files Changed

Filename Overview
src/core/rules-rm.ts Core logic fix: reorders classifyTarget to check isTempTarget before isCwdHomeForRmPolicy/isCwdSelfTarget; adds home_cwd_target classification; exports isHomeDirectory for testing. Logic is sound and well-ordered.
src/core/analyze/segment.ts Removes the inline isHomeDirectory pre-check (now handled inside analyzeRm); also drops unused hasRecursiveForceFlags import. Clean simplification — analyzeRm now owns the full home-cwd decision.
src/bin/explain/segment.ts Removes the separate isHomeDirectory explain step; now delegates to analyzeRm with correctly derived cwdForRm and originalCwd — aligning the explain path with the analyze path for the first time.
tests/core/rules-rm.test.ts Adds tests for /tmp, /var/tmp, $TMPDIR targets allowed in home cwd; mixed temp+relative blocked; cwd-self temp path allowed. Coverage of the new home_cwd_target path is solid.
tests/bin/explain/command.test.ts Updates explain trace assertions from isHomeDirectory step to analyzeRm step; adds test verifying temp-target rm in home cwd is allowed. Tests correctly reflect the consolidated explain flow.
dist/core/rules-rm.d.ts Type declaration correctly adds the @internal isHomeDirectory export. Consistent with the source export added for testing.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["classifyTarget(target, ctx)"] --> B{"isDangerousRootOrHomeTarget?\n(~, $HOME, /, /*)"}
    B -- yes --> C["root_or_home_target\n→ BLOCKED (extremely dangerous)"]
    B -- no --> D{"isTempTarget?\n(/tmp/…, /var/tmp/…, $TMPDIR/…)"}
    D -- yes --> E["temp_target\n→ ALLOWED ✓"]
    D -- no --> F{"anchoredCwd present?"}
    F -- no --> K["outside_anchored_cwd\n→ BLOCKED"]
    F -- yes --> G{"isCwdHomeForRmPolicy?\n(cwd == HOME)"}
    G -- yes --> H["home_cwd_target ★ NEW\n→ BLOCKED (change directory first)"]
    G -- no --> I{"isCwdSelfTarget?\n(target resolves to cwd)"}
    I -- yes --> J["cwd_self_target\n→ BLOCKED"]
    I -- no --> L{"isTargetWithinCwd?"}
    L -- yes --> M{"paranoid mode?"}
    M -- yes --> N["within_anchored_cwd\n→ BLOCKED (paranoid)"]
    M -- no --> O["within_anchored_cwd\n→ ALLOWED ✓"]
    L -- no --> K

    style E fill:#c8f7c5,stroke:#2ecc71
    style O fill:#c8f7c5,stroke:#2ecc71
    style H fill:#ffeaa7,stroke:#fdcb6e
    style C fill:#fab1a0,stroke:#e17055
    style J fill:#fab1a0,stroke:#e17055
    style K fill:#fab1a0,stroke:#e17055
    style N fill:#fab1a0,stroke:#e17055
Loading

Reviews (1): Last reviewed commit: "fix(rules-rm): allow temp targets when c..." | Re-trigger Greptile

@kenryu42 kenryu42 merged commit 33739bb into main Mar 25, 2026
9 checks passed
@kenryu42 kenryu42 deleted the fix/rm-home-cwd-classification branch March 25, 2026 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant