feat(engine): escalate recursive delete of gitignored, uncommitted dirs (#198)

[harshitagrawal2O]

Pull Request

Slice

• Repo: doberman-core
• Feature / Slice: engine: escalate bulk-delete of gitignored, uncommitted directories #198 — engine: escalate bulk-delete of gitignored, uncommitted directories
• Plan reference: n/a (issue-driven enhancement)

Implements the approach @fu351 confirmed on the issue (option 2: a bounded, timeout-guarded git check-ignore -q, falling back to today's plain bulk_threshold on any git or subprocess error, with a short timeout so a hung git never stalls a decision).

What this PR does

AN-1's lexical gate (_rm_targets_unrecoverable_data) classifies an unrecoverable file operand by basename. A directory operand carries no lexical signal at all — data/ and build/ are indistinguishable as strings, yet deleting the first can be irrecoverable while deleting the second costs a rebuild. That is why the case was deferred, and it is what this picks up.

A recursive delete (rm -r, and the Windows delete verbs) whose target git check-ignore reports as ignored, and under which git ls-files finds nothing committed, now steps up to AUTH (destructive_command). git holds no copy of that directory, so the delete is genuinely unrecoverable.

Built beside the AN-1 gate, not over it. _rm_targets_unrecoverable_data is byte-identical and still runs first; the new branch is a separate function reached only after it declines.

Acceptance criteria

• A bulk rm targeting paths inside a git-ignored, never-committed directory escalates to AUTH — always, not at a lowered operand count. Documented choice, see below.
• The same command against a tracked (committed) directory is unaffected.
• git unavailable, or the target outside a git repo → falls back to today's plain bulk_threshold check. No crash, no silent PASS.
• No raw path or filename appears in the explanation.
• Existing behaviour unchanged everywhere else; no test weakened, no output format broken.
• ruff check ., ruff format --check ., lint-imports pass.
• pytest passes.

The documented choice: always AUTH, with a narrow carve-out

The issue offered "lower the threshold or always AUTH — contributor's documented choice."

Lowering the threshold does not reach the case in the title. rm -rf data/ is a single operand, so no threshold above 1 catches it. Any lowered-threshold implementation would have closed the issue without fixing the scenario it describes. So this forces AUTH regardless of operand count.

That alone would also escalate rm -rf node_modules and rm -rf build, which are routine — and an auth prompt people learn to click through protects nothing, so the noise would be a real safety cost, not just an annoyance. There is therefore a carve-out for tool-owned build/cache directories (node_modules, build, dist, target, .venv, __pycache__, .next, …).

The carve-out is deliberately conservative. Ambiguous names someone might legitimately keep un-backed-up work in — bin, out, vendor, data, tmp — are not on it, so the escalation fires for those. And because the whole gate is additive, the list can only ever make the new escalation quieter; it has no reach over any verdict that exists today. Happy to drop it for the simpler, noisier rule if you'd rather.

Safety constraints

Raise-only by construction. The branch sits after every existing BLOCK and every existing AUTH in both _segment_verdict and _windows_delete_verdict, so the only transition it can produce is "would have returned None (PASS)" → AUTH. It cannot lower, reorder, or mask a verdict that fires today. Tested: rm -rf / still BLOCKs inside a repo, .doberman still BLOCKs, and a 30-operand ignored delete still reports bulk_operation rather than being shadowed by the new reason code.

Fails toward today, not toward PASS. No git binary, not a repository, a timeout, an OSError, a SubprocessError, an operand escaping the repo root, or git reporting "ignored" while unable to say what is committed — every one returns False and lands on exactly today's plain bulk_threshold behaviour. Each is covered by its own test.

No raw path in the explanation. One shared category-only string; a test asserts a distinctively-named directory and its contents never reach result.explanation.

Cost

The git probe sits behind four cheap local gates — recursive flag → not a regenerable build directory → canonicalizes inside the root (via the shared canonicalize, not an ad-hoc join) → is a real directory, not a symlink — and then runs at most two subprocesses per segment, whatever the operand count, each with a 2s timeout. --stdin -z keeps the operand count off the command line and makes a path containing a newline unable to desynchronize the parse.

Measured against this repo on Windows: carve-out hits short-circuit in 0.1 ms; a probe that reaches git costs ~45 ms for one call, ~98 ms on the escalation path. A non-recursive rm never probes at all.

Tests added (run in CI)

• tests/unit/test_rule_commands_gitignored_delete.py — 38 tests against real throwaway git repositories under tmp_path (deterministic: no network, no clock, fixed committer identity, fresh repo per test; skipped if no git binary):
  • Escalation — rm -rf data, data/, rm -r, rm -Rf ./data; one qualifying operand hidden among tracked ones; Remove-Item -Recurse; survives a chained segment.
  • No false positives — tracked directories; a directory committed then gitignored; each carved-out build directory; case-insensitivity of the carve-out; non-recursive rm; a nonexistent target; a symlink to an ignored directory; an unexpanded glob operand; an operand escaping the repo root.
  • Fails toward today — no git binary; not a repository; TimeoutExpired / OSError / SubprocessError / ValueError; ls-files unable to answer; the 2s timeout actually reaching subprocess.run with shell not set.
  • Raise-only — catastrophic deletes still BLOCK inside a repo; control-plane delete still BLOCKs; the bulk threshold keeps precedence and its own bulk_operation reason code; AN-1 still owns the file case with its own wording.
  • Cost — a six-operand delete still spawns at most two subprocesses.
  • Redaction — the explanation names the category and never the directory or its contents.

Changelog

• changelog.d/<PR>.added.md fragment added

Public-release safety (doberman-core only)

• Contains nothing from the "not allowed" list: no enterprise/hosted code, no proprietary detection, no customer data, no secrets, no commercial-license code
• Core still builds/tests/runs with NO enterprise package installed

Security checklist

• Fails closed on error / uncertainty — every git/subprocess failure mode falls back to today's verdict rather than crashing or inventing one; each has a test
• No secret, full file, or unredacted prompt logged or committed — the explanation is category-only, asserted by test
• Any guardrail/learning change is raise-only (no silent loosening) — PASS → AUTH is the only transition possible, by construction and by test
• Every BLOCK/AUTH carries reason codes + a human explanation — destructive_command + a one-line explanation, documented in docs/REASON_CODES.md
• doberman-core does not import doberman_enterprise

Edge cases covered / Deviations from plan / Risks introduced

Two deviations, both deliberate — please push back on either.

1. Mirrored onto the Windows delete verbs. The issue scoped this to rm/_segment_verdict. But an agent on Windows spells the same irrecoverable delete Remove-Item -Recurse data, and _windows_delete_verdict already mirrors every other rung of the rm ladder (including AN-1's own _any_operand_unrecoverable). Leaving it POSIX-only would have shipped the fix with a documented bypass. It is the last check in that function, so it can only turn its None into AUTH. This required adding a root parameter to _windows_delete_verdict — a private helper with a single call site.

2. git ls-files is a backstop here, not the load-bearing layer. The issue suggested ls-files --error-unmatch to rule out "ignored but already tracked" false positives. Measuring on git 2.47, check-ignore already consults the index and will not call a directory with tracked contents ignored — so the first probe catches that case on its own. I kept the second pass anyway, because that index-awareness is a default (it is what --no-index turns off) rather than a documented guarantee, and because it can only ever make the escalation quieter, never louder. It runs only once check-ignore has said "ignored", so it costs nothing on the common path. Both the behaviour and the backstop are tested independently so neither can rot unnoticed.

One small refactor: _rm_is_catastrophic's inline flag parse is extracted to _rm_recursive_and_force so the new gate asks the same question rather than keeping a second, drifting copy. Pure extraction, behaviour byte-identical, covered by the existing catastrophic-delete tests.

Risks / limits, stated plainly:

• This is the first subprocess call in engine/rules/. It is bounded and non-shell, but it is a genuine change in what the objective decision path is allowed to do, per your confirmation on the issue. The AN-1 ponytail note has been updated to point at the new gate so the next reader is not misled about scope.
• A directory operand built dynamically (rm -rf $(cat target)) or still carrying a glob is not classified — same limit as every other operand-shaped check in this file.
• The carve-out is a hardcoded list and will need occasional additions as toolchains change. It is one tuple with a comment explaining the inclusion bar.
• rm -rf data/ where data/ is ignored but the user does have it backed up elsewhere will now prompt. That is the intended trade: the rule can see git, not the user's backups.

🤖 Generated with Claude Code

[fu351]

Thanks @harshitagrawal2O, this is close and the structure is right: the gate sits dead last in both _segment_verdict and _windows_delete_verdict, after every existing BLOCK and AUTH, so raise-only holds by construction. Hoisting it above the bulk-threshold check turns test_bulk_threshold_keeps_precedence red, which is the test I wanted to see. Three things need your call before I merge:

1. The gate hands git a lower-cased path. _delete_dir_candidates appends canonical.relposix (commands.py:1119), and canonicalize lower-cases relposix for pattern matching (canonical.py:146). On a case-sensitive filesystem, so Linux and CI, git check-ignore data does not match a .gitignore entry Data/, and rm -rf Exports against a gitignored, never-committed directory falls straight through to PASS. With core.ignorecase=false: check-ignore 'data' gives rc 1, check-ignore 'Data' gives rc 0. Every fixture name in the new test file is lowercase, so the suite cannot see it. Hand git a case-preserving root-relative path (for example PurePosixPath(Path(canonical.resolved).relative_to(root)).as_posix()), keep escapes_root as the confinement guard, and parametrize the escalation test over a capitalized name.

2. A trailing slash bypasses the build/cache carve-out. _unrecoverable_basename is posixpath.basename, which returns "" for node_modules/, so _is_regenerable_dir("") matches nothing: rm -rf node_modules gives PASS, rm -rf node_modules/ gives AUTH. Tab completion produces the slash form, so that is the common spelling. Strip trailing separators before the basename and parametrize the carve-out test over name and name/.

3. No per-decision budget on the git probe. The two-subprocess ceiling is per segment and a command can carry 256 segments: one chained command in a repo with one ordinary directory spawned 256 git subprocesses in 7.7 s, and a hung git gives 256 x 2 x 2 s worst case, against the body's "never stalls a decision". A module-level probe budget like _MAX_PAYLOAD_PATH_RESOLVES next door would settle it.

Smaller: test_operand_escaping_the_repo_root_does_not_escalate stays green with the guard deleted (relposix is already "" whenever escapes_root is set), so assert on _delete_dir_candidates directly; it becomes load-bearing once 1 is fixed. And add Closes #198 to the body. Ping me when it's pushed and I'll re-run the same checks.

[fu351]

Thanks @harshitagrawal2O, holding this one on what I think is a one-word fix. commands.py hands git check-ignore canonical.relposix, and canonical.py lower-cases relposix by design since it is the matching form. On a case-sensitive filesystem that makes the check inert for any capitalized directory, so a Unity repo's Library/ or Build/ never escalates, and it can also match a different ignored path and AUTH a directory git does have a copy of. Switching that call to canonical.resolved should do it. Separately, this puts the first subprocess call on the verdict path, and I'd rather write that down as its own decision than land it inside a release, so let me do that first.