cad-phase remove treats an unreadable git state as clean and deletes recursively #136

New issue

Closed

opened 2026-08-15 14:33:38 +00:00 by john · 2 comments

john commented

2026-08-15 14:33:38 +00:00

Owner

Source: deep dive F4
Verdict: AGREE - medium

Claim as reviewed

uncommittedUnder returns an empty array whenever git status fails, and any failure from git rm - not only "not a repository" - falls back to a recursive rmSync. This converts "unable to prove clean" into "safe to erase".

At HEAD `f354864` (v3.3.0)

cadence-core/bin/planning.mjs:3032-3039 - the catch returns []. The doc comment at :3025-3027 reasons only about the outside-a-git-repo case, where the fallback is genuinely safe.

cadence-core/bin/planning.mjs:3240-3244 - try { execFileSync('git', ['rm', '-r', '-q', ...]) } catch { rmSync(..., { recursive: true }) }.

Git can fail because it is missing, the repository is unsafe-owned, permissions changed, the index is locked, or configuration is broken. None of those prove the contents are recoverable.

There is also a time-of-check/time-of-use gap: work can become dirty after the preflight git status and before git rm; git rm then refuses with "file has local modifications" and the unconditional rmSync deletes it anyway.

Note

Bounded by the dry-run gate at :3225 and by needing git to fail for a non-obvious reason.

Fix shape: a discriminated preflight result {state:'clean'|'dirty'|'unprovable'} that refuses on unprovable; determine repository membership once; use filesystem deletion only for a confirmed non-git tree whose contents were listed to the user.

**Source:** deep dive F4 **Verdict:** AGREE - medium ### Claim as reviewed `uncommittedUnder` returns an empty array whenever `git status` fails, and any failure from `git rm` - not only "not a repository" - falls back to a recursive `rmSync`. This converts "unable to prove clean" into "safe to erase". ### At HEAD f354864 (v3.3.0) `cadence-core/bin/planning.mjs:3032-3039` - the `catch` returns `[]`. The doc comment at `:3025-3027` reasons only about the outside-a-git-repo case, where the fallback is genuinely safe. `cadence-core/bin/planning.mjs:3240-3244` - `try { execFileSync('git', ['rm', '-r', '-q', ...]) } catch { rmSync(..., { recursive: true }) }`. Git can fail because it is missing, the repository is unsafe-owned, permissions changed, the index is locked, or configuration is broken. None of those prove the contents are recoverable. There is also a time-of-check/time-of-use gap: work can become dirty after the preflight `git status` and before `git rm`; `git rm` then refuses with "file has local modifications" and the unconditional `rmSync` deletes it anyway. ### Note Bounded by the dry-run gate at `:3225` and by needing git to fail for a non-obvious reason. Fix shape: a discriminated preflight result `{state:'clean'|'dirty'|'unprovable'}` that refuses on `unprovable`; determine repository membership once; use filesystem deletion only for a confirmed non-git tree whose contents were listed to the user.

john added the

bug

external-review

labels

2026-08-15 14:33:38 +00:00

john added this to the v3.5.5 milestone

2026-08-15 15:09:29 +00:00

john commented

2026-08-15 15:09:31 +00:00

Author

Owner

Solo milestone by design - this is the only open issue that can destroy uncommitted work, so it gets its own review surface rather than riding with adjacent planning.mjs fixes.

Fix shape from the issue body, restated so a plan does not narrow it: a discriminated preflight result {state:'clean'|'dirty'|'unprovable'} that REFUSES on unprovable. The bug is not that git status can fail; it is that failure and cleanliness return the same value. Note the TOCTOU half too - the preflight can be clean and the tree dirty by the time git rm runs, and the unconditional rmSync fallback at planning.mjs:3240-3244 then destroys it. Fixing only the preflight leaves that arm open.

Solo milestone by design - this is the only open issue that can destroy uncommitted work, so it gets its own review surface rather than riding with adjacent planning.mjs fixes. Fix shape from the issue body, restated so a plan does not narrow it: a discriminated preflight result {state:'clean'|'dirty'|'unprovable'} that REFUSES on unprovable. The bug is not that git status can fail; it is that failure and cleanliness return the same value. Note the TOCTOU half too - the preflight can be clean and the tree dirty by the time git rm runs, and the unconditional rmSync fallback at planning.mjs:3240-3244 then destroys it. Fixing only the preflight leaves that arm open.

john modified the milestone from v3.5.5 to v3.5.4

2026-08-18 10:23:28 +00:00

john referenced this issue from a commit

2026-08-18 10:37:55 +00:00

chore: open the v3.5.4 roadmap - three phases ordered by what a wrong answer costs

john commented

2026-08-18 19:29:00 +00:00

Author

Owner

Fixed in v3.5.4 as PHS-01, merged in #217 and tagged v3.5.4. The fix carries a check watched failing against the unpatched tree first.

john closed this issue

2026-08-18 19:29:05 +00:00