From 4cc2a466b5ca6c189381d166391cd4b350537cf8 Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Fri, 11 Sep 2026 11:26:55 -0500 Subject: [PATCH] fix(#4208): add `--files-removed` so `commit --files` can record a move without a directory pathspec (#4253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4208): add --files-removed so commit --files can record a move without a directory pathspec `cmdCommit`'s `--files` list can stage an addition but never a deletion: the #2014 guard skips a missing explicit entry because the filesystem cannot tell "moved away" from "not written yet". A caller that moves a file therefore had two forms, both wrong — a directory entry records the move but also commits every unrelated file in that directory (a concurrent session's in-flight todo, in the unattended execute-phase sweep), and a file entry leaves the old path's deletion dangling with the todo tracked at both paths. `--files-removed ` is the caller-declared delete intent. Each entry names a file, or a directory whose tracked-but-absent files are the removals; those paths are staged with `git rm --cached` and join the commit pathspec. `--files` keeps its skip-if-missing contract untouched. A file entry still present on disk fails the commit closed with the existing staging-failure rollback; a never-tracked path is a no-op. `--files-removed` alone is a declared scope, not the unscoped .planning/ sweep. The dispatcher previously folded every non-flag token after `--files` into that list, so a second list flag could not exist; each list now runs from its flag to the next `--` token. The execute-phase todo sweep names the moved todos on both sides from CLOSED[@], and cleanup's archive commit moves .planning/phases/ and .planning/quick/ under --files-removed. Fixes #4208 Emitted-Drift-Ack-Growth: cleanup.md — the archive commit moves phases/ and quick/ under --files-removed; the growth is one paragraph stating why those two directories must not be --files entries * chore(#4208): set changeset fragment pr to 4253 * fix(#4208): fit execute-phase.md under the ADR-857 ceiling and re-point the #2415 guard Three CI failures, all consequences of this PR's own change. 1. gsd-core/workflows/execute-phase.md was 93,577 bytes against the ADR-857 Phase 6 margin gate's <= 93,400 (hard ceiling 93,600). The three-line rationale comment plus the four-line array-building block added 318 bytes to a file that had only 141 of headroom on next. Move the rationale to docs/CLI-TOOLS.md -- which this PR already extends with the --files-removed contract, and which is where the ADR-857 gate wants call-site detail to live rather than in the host workflow -- and fold the array build onto one line. 93,577 -> 93,372. 2/3. tests/close-phase-todos-stage-deletion.test.cjs pinned the #2415 guarantee to its old MECHANISM: it regex-matched the literal .planning/todos/{completed,pending}/ directory pathspecs in the commit --files list. This PR deliberately replaced those with named files (a directory entry also committed an unrelated todo a concurrent session dropped in mid-close), so the guard failed on a change it should have accepted. Re-point it at the new mechanism without weakening it: assert the ADDED array reaches --files, the REMOVED array reaches --files-removed, STATE.md is still committed, and -- newly -- that the two arrays are built from $COMPLETED_DIR and $PENDING_DIR respectively. Verified by negative control: deleting --files-removed "${REMOVED[@]}" from the workflow still fails the test, so the #2415 regression remains caught. Note for the merge queue: #4233 also grows execute-phase.md (+114). The two are additive -- different regions, no textual conflict -- so with both landed the file reaches ~93,486, over the 93,400 margin though under the 93,600 hard ceiling. Whichever merges second will need to reclaim ~86 bytes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0183892Y3fxxirte4WNmBKbv * fix(#4208): reclaim execute-phase.md bytes so the PR is net-neutral under the ADR-857 margin Rebasing onto next surfaced the byte-gate collision flagged earlier on this PR: #4284 grew execute-phase.md by 95 bytes (93,259 -> 93,354), so this PR's +113 landed at 93,467 against the <= 93,400 margin in tests/claude-orchestration.test.cjs. Compact the close_phase_todos step this PR already edits -- drop the PHASE_NUM indirection, fold the normaliser and the match guard, print the closed list with one printf, shorten the step's prose -- without touching the mechanism the #2415 guard pins (ADDED/REMOVED arrays, the plain mv). 93,467 -> 93,349: 5 bytes under the base, so the PR no longer spends any of next's 46 bytes of headroom. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): classify absent index entries before staging a removal; restore removed entries exactly on rollback Review of #4253 found three Majors with one root cause: the removal side judged presence by fs.lstatSync alone, where the addition side already reads `git ls-files -v` state. Absence from the worktree is not removal: - a submodule gitlink (mode 160000) whose directory was deleted by hand lists like a file and was `rm --cached` with no .gitmodules cleanup; - a skip-worktree path is never materialised by a cone-mode sparse checkout, so a directory entry over a sparse-excluded tree dropped that whole tree from the index; - an assume-unchanged path's worktree state is not something git itself consults; - an intent-to-add entry (`git add -N`) renders as a plain cached entry on the empty blob, yet nothing tracked exists to remove and no rollback can restore the flag. The index listing now carries each entry's `ls-files -v -s` tag, mode and stage. Only a plain cached (H), stage-0, non-gitlink entry is a removal candidate; every other state is left alone under a directory entry (exactly like a present file) and fails closed when named directly, with the state in the error. "Named directly" is decided on RESOLVED paths, not strings -- realpath of the longest existing prefix with the absent tail re-appended: an absolute path, `./x`, `--cwd`, or a symlinked spelling of the tree (macOS `/var` -> `/private/var`, where `process.cwd()` is the real path and the caller's absolute path is not -- CI on this round's first push) all resolve to the same entry, where a string compare against git's cwd-relative output silently took the directory polarity (pre-push review, driven; the symlink case is driven with an aliased fixture directory). The enumeration's domain is what `ls-files -v -s` can emit for an index entry, stated at the classifier. The third Major -- on an unborn HEAD a successful `rm --cached` was never rolled back when a later entry failed -- is fixed differently from the review's suggestion. Pushing the path into stagedPaths would put it on the commit pathspec, which a root commit refuses ("pathspec did not match", driven), and `git reset -- ` cannot restore an entry with no HEAD anyway. Instead every index entry this call removes is recorded (mode, blob) before the `rm` and put back with `update-index --cacheinfo` on rollback. That also restores a caller-pre-staged blob at a removed path exactly, where a reset would have silently replaced it with HEAD's version. The rollback is best-effort, as the addition-side reset already was, and the docs say so. Eight tests: gitlink under a directory entry, named directly, and named by absolute path; skip-worktree both forms; intent-to-add both forms; assume-unchanged named; unborn-HEAD partial failure restores the removal; pre-staged blob survives the rollback. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): drop the empty fenced block left dangling in cleanup.md's commit step Review nit on #4253: inserting the --files-removed rationale between the original bash block and its closing fence left an empty ```bash``` pair before . Harmless at runtime, a formatting artifact of this PR's own diff; removed. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): a boolean flag inside a commit path list no longer ends the list Review minor on #4253: collectList stopped at the next `--` token, so a positional wedged between a boolean flag and the next list flag (`--files a --amend b --files-removed c`) was claimed by neither list and silently dropped -- a regression in shape against the old slice-to-end parse, which filtered `--` tokens and kept `b`. No current call site interleaves that way, but the gap was real. A list now runs to the next LIST flag (`--files` / `--files-removed`) and skips boolean flags on the way, and a REPEATED list flag merges its runs (`--files a --files b` -> [a, b]) as the slice-to-end parse did -- a first cut stopped at the repeat and dropped `b`, the same silent-drop shape one level over (pre-post comment audit). The only change #4208 makes to parsing is that a second list flag can exist. Tests: STATE.md wedged between --no-verify and --files-removed lands in the commit; both runs of a repeated --files reach it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * test(#4208): drive the reappearance window with a post-index-change hook Review nit on #4253: the defensive re-check for a file recreated between the absence test and `git rm --cached` -- the concurrent-session race this PR's own changeset names -- had no test. git fires post-index-change the moment `rm --cached` writes the index, so a hook that copies the file back exactly then exercises the window deterministically. The call reports staging_failed / "reappeared on disk", commits nothing, and the rollback restores the removed entry. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): restore a staged removal when the call records nothing A `git rm --cached` that succeeds mutates the index whether or not a commit follows. Only the staging-failure rollback put those entries back, so a call that reached `nothing_to_commit` reported no state change while the removal sat staged -- riding along on the caller's next commit. The review named the unborn-HEAD, removal-only shape. Keying on `headExists` would have fixed half of it: the guard also fires with a real HEAD when the removed path is index-only (added, never committed), because `diff HEAD` reads clean with the path absent on both sides. Both shapes now restore, at both `nothing_to_commit` exits. The failure exits are deliberately left alone -- they report a failure rather than no-change, and the addition side leaves its own staged paths there too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * refactor(#4208): lift declared-removal staging out of the cmdCommit hotspot `cmdCommit` was a critical-risk hotspot before this flag existed, and #4208 had inlined another ~270 lines into it. `stageDeclaredRemovals(cwd, removedDeclared)` now owns the index-state classification, path canonicalisation and entry recording, returning the pathspec entries and the recorded removals its caller merges. Pure motion: no branch, message or probe changed. Only the two accumulators became local names, and `restoreRemovedEntries` stays with the caller because the exits that restore are the caller's. cmdCommit 888 -> 625 lines here; the extracted helper is 277. (Figures corrected after publication: an earlier version of this message said 854 -> 591 and claimed the result was below cmdCommit's pre-#4208 shape. Both were wrong -- the count came from a faulty brace scanner, and `next`'s cmdCommit is 581, so this is above it, not below.) Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): property-test the two-list commit parser RULESET.TESTS.property-based-testing asks a parser for at least one property test asserting a domain invariant; `collectList` had only hand-picked examples, one per shape a review round had already broken. Hoisted it to module scope as `collectListFlagValues` and exported it in the file's existing exported-for-tests convention -- a parser reachable only by spawning the CLI can be tested one example at a time and no faster. Three properties over generated argv: every positional lands in exactly the run open at it whatever the flag order or count; no positional after the first list flag is dropped or double-claimed; and with `--files-removed` absent the parse equals the pre-#4208 slice-to-end parse. Controlled against two mutants -- a run ending at any `--` token, and a repeated list flag that does not merge -- each of which the properties catch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): pin cleanup.md's archive commit to --files-removed execute-phase.md's rewrite is pinned by the #2415 guard in this file; cleanup.md's equivalent was not, so reverting its routing would have been caught by nothing -- the mechanism's unit tests never read this file and pass either way. Asserts the two archived directories are under --files-removed and NOT under --files (where a directory entry sweeps in a concurrent session's in-flight writes), and that the destinations and STATE.md stay on the additive half. Controlled by restoring the pre-#4208 sweep, which fails it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): pin that a symlink to a directory is one tracked path Review of #4253 read the `lstatSync(...).isDirectory()` test as a symlink-following defect. Driving it says the opposite: git tracks the link as a single blob (mode 120000) and does not traverse it, so the tracked paths "under" it live at the real directory and were never named by the caller. Following the link would stage those -- the directory sweep #4208 exists to remove -- while the named entry still sat present on disk. Pinned rather than changed, with the premise driven in the test body. Swapping `lstatSync` for `statSync` -- the prescription as written -- fails it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * chore(#4208): refresh the compact-content baseline for this PR's execute-phase edit The base range added `tests/benchmark-compact-content.test.cjs` and a committed token baseline over the compacted workflows. This PR edits `gsd-core/workflows/execute-phase.md`, so the baseline drifts by +12 tokens on that entry and on the aggregate. Refreshed with `node scripts/benchmark-compact-content.cjs --write`; the diff is those two entries and nothing else. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): report a removal the call could not put back Round review of this round found the restore itself unchecked: the helper ignored `update-index`'s exit code, so a FAILED restore still reported `nothing_to_commit` -- the same false "no state changed" the restore exists to prevent, surviving one level down on the restore-failure path. It now returns a boolean. The two no-change exits report `staging_failed` naming the paths left staged; the staging-failure rollback still ignores it, deliberately, because it is already reporting a failure and an unwritable index is usually the failure being reported. Driven with a post-index-change hook that makes the git dir unwritable the moment `rm --cached` lands, so the restore cannot take its lock. Reverting both guards fails the test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): disclose a removal the rollback could not restore Round review refuted the reasoning behind leaving the rollback path's restore unchecked. The claim was that this exit is already reporting a failure, so the restore's result adds nothing. The counterexample is the ordinary case: the reported failure is usually a DIFFERENT cause -- a contradictory declaration, a reappeared path -- so a caller reading `failures` sees only that cause and learns nothing about the removal still sitting in its index. The rollback now appends a disclosure entry per un-restored removal, naming the path. The reason and `file` still report the failure that caused the rollback; the disclosure is additive. Also moves the restore-failure test's chmod into a `finally`: `t.after` runs AFTER the parent `afterEach`, so a throw before it left the fixture undeletable. Both driven; reverting the disclosure fails the new test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): decide index state by observation, never by an exit code The restore added two commits earlier keyed both its record decision and its success verdict on git's exit code. An exit code answers "did the command succeed", never "did the index change" -- execGit collapses a spawn timeout to a non-zero exit, and a killed git can already have written the index. Round review drove four failures from that one assumption, in both directions: - a failed `rm` still contributed an entry, so the rollback disclosed a removal that was never staged (stale index.lock); - a timed-out `rm` whose write DID land contributed none, so a real mutation was neither restored nor disclosed; - a timed-out `update-index` whose write landed reported failure, publishing a "could NOT be restored" disclosure that was false; - and the read-back that replaced it omitted `-z`, so core.quotePath rendered `café.md` as `"caf\303\251.md"` and an exactly-restored entry read as not restored -- the same quoting defect this PR already fixed for `preStaged`. Everything now observes the index. A failed `rm` re-reads `ls-files -z` for the path: gone means this call owns the removal and records it; still there means nothing was staged; a probe that cannot answer becomes its own failure entry rather than an assumption. The restore verifies the same way, comparing the WHOLE entry (mode, blob, stage), because `--cacheinfo` restores all three and a path-only test accepts an entry that came back as something else. The verdict is three-valued -- `restored` / `not-restored` / `unverified` -- and the unverified wording says the restore could not be VERIFIED rather than that it failed. The rm's own failure is pushed ahead of any probe diagnostic so a timed-out removal keeps `timed_out: true` and its own message as the reported cause. Five regression cases, each negative-controlled against the shape it pins. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): treat a declared removal path as a path, not a pathspec An index path handed back to git is parsed as a PATHSPEC, and the removal side handed several back. Three driven harms, all of them the sweep-in this flag exists to remove, arriving through the operand rather than through a directory entry: - a tracked file literally named `.planning/*.md` made `rm --cached` GLOB: it removed `peer.md` and `stays.md` too, only the declared entry was recorded, so the rollback restored one of three and the other two rode out as staged deletions the result disclosed nowhere; - the same name reached `git commit -- `, which globbed and committed an undeclared `M peer.md` alongside the declared removal; - and the intent-to-add probe (`diff --cached` over the path) matched a STAGED PEER instead of itself, so an `add -N` entry was misclassified as ordinary content, removed, and restored by `--cacheinfo` -- which cannot restore the intent flag. It came back as a real staged addition. Every operand on this path is now `:(literal)`: the `rm`, both index probes, the intent-to-add probe, the restore read-back, the entry-level `ls-files` / `ls-tree`, and -- for the REMOVAL-derived entries only -- the downstream `ls-files` / dry-run / `diff HEAD` / `commit` pathspec. `--files` entries keep whatever pathspec behaviour they have today; that is not this change's to alter. `:(literal)` still resolves a directory to its descendants (driven), so the directory form is unchanged. Closes what an earlier cut of this commit declared as a residual: a filename beginning with `:` is now removable end to end, because the commit pathspec no longer reinterprets it. Also fixes a MINOR from the same review: cleanup.md's contract test checked the destinations' position relative to `--files-removed` but never that `--files` was present at all, so deleting the flag still passed. Un-literalising the seven sites fails three of the new tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): scope the rollback to the caller's own name space Round review drove a rollback that destroyed the caller's own staged work. Two causes, one of them pre-existing: - `git diff --cached` prints REPO-relative paths whatever the cwd, while `stagedPaths` holds the caller's cwd-relative names. In a project nested inside its repo (`/sub/.planning/...`) the two name spaces never intersect, so `preStaged` matched NOTHING, every path landed in `toUnstage`, and the reset unstaged a caller-staged deletion and modification that this call had never touched. `--relative` makes the two sets comparable, and is a no-op when the project IS the repo root. This governs the `--files` side too and predates this flag. - the rollback's `reset` was the last place a removal-derived name reached git as a bare pathspec; it takes `asPathspec` like every other site. Driven on a nested fixture; dropping `--relative` fails the new test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): gate six fixtures that Windows cannot construct CI's `test (windows-latest, 24, shard 2/3)` went red on this round. Two primitives the new fixtures rely on do not exist on Windows, both driven on a real Windows host rather than inferred: - a filename containing `*` or `:` cannot be created at all (`IOException` / `FileNotFoundException`), which is four of the pathspec fixtures; - `chmod` cannot make a directory unwritable — a write into a ReadOnly directory succeeds — so the two restore-failure fixtures cannot drive the failure they exist to drive. Each is skipped on win32 with its measured reason, in the repo's existing `{ skip: process.platform === 'win32' ? '' : false }` form. The behaviours they pin are platform-independent; only the fixtures are not. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): build git's index-syntax path with forward slashes The remaining Windows red was mine, not the platform's: `git rev-parse :` takes a forward-slash path, and `path.join` yields backslashes there, so git rejected it as an ambiguous argument. The hook in the same test already used the slash form. Not gated — the behaviour it pins is portable; only the argument was not. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * chore(#4208): refresh the compact-content baseline against the rebased base `next` moved the `new-project` split and the aggregate under this PR's execute-phase entry; regenerated with `scripts/benchmark-compact-content.cjs --write` so the only leaves differing from the base's copy are the execute-phase split and the aggregate it feeds. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FUcGM4FWeZV4cqvR7QBtJh * chore(#4208): regenerate the macOS conformance tier for this PR's fixtures `next` gained the macOS-specific conformance tier (#4593) after this branch was cut. Its classifier (`scripts/gen-platform-conformance-tier.cjs --target macos`) now selects `tests/commit-files-deletion.test.cjs` on the `chmod-mode-bit` and `symlink-keyword` signals the PR's fixtures carry (the chmod-driven failed-restore cases and the symlink-to-directory case). Regenerated with `--target macos --write`; the platform tier was already in sync. The file was modified, not added, which is why the added-files check did not surface it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FUcGM4FWeZV4cqvR7QBtJh --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: CI Rebase Check Co-authored-by: Tom Boucher --- .changeset/witty-wasps-romp.md | 5 + docs/CLI-TOOLS.md | 4 +- gsd-core/bin/gsd-tools.cjs | 49 +- gsd-core/workflows/cleanup.md | 4 +- gsd-core/workflows/execute-phase.md | 27 +- .../lib/macos-conformance-tier.generated.cjs | 1 + src/commands.cts | 509 ++++++- .../close-phase-todos-stage-deletion.test.cjs | 72 +- tests/commit-files-deletion.test.cjs | 1186 +++++++++++++++++ .../compact-content-benchmark-baseline.json | 10 +- 10 files changed, 1820 insertions(+), 47 deletions(-) create mode 100644 .changeset/witty-wasps-romp.md diff --git a/.changeset/witty-wasps-romp.md b/.changeset/witty-wasps-romp.md new file mode 100644 index 000000000..90dc2517a --- /dev/null +++ b/.changeset/witty-wasps-romp.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4253 +--- +**`commit --files` can now record a file move without a directory pathspec** — a new `--files-removed ` list declares the deletions the caller intends: each named file, or each tracked-but-absent file under a named directory, is staged as a deletion and joins the commit pathspec. Previously the #2014 skip-if-missing guard meant the only form that recorded a move was a directory entry in `--files`, which also committed any unrelated file sitting in that directory — in the unattended end-of-phase todo sweep, a concurrent session's in-flight todo landed under a phase-close message with no warning, while the file-precise form left the old path's deletion dangling and the todo tracked at both paths. `--files` keeps its skip-if-missing contract unchanged; a `--files-removed` file entry that is still present on disk fails the commit closed, and an index entry that is absent by design (a submodule gitlink, a skip-worktree or assume-unchanged path, an unmerged or intent-to-add entry) is never taken for a removal. A staging failure rolls back every removal the call made with its recorded mode and blob, including on an unborn `HEAD` (best-effort, as the existing addition-side reset is). The `execute-phase` todo sweep and the `cleanup` archive commit now name their removals instead of their directories. diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 027867288..a7e8704fa 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1194,9 +1194,11 @@ node gsd-tools.cjs audit-open acknowledge --category --milestone ] [--force] [--dry-run] # Git commit with config checks -node gsd-tools.cjs commit [--files f1 f2] [--amend] [--no-verify] [--respect-staged] +node gsd-tools.cjs commit [--files f1 f2] [--files-removed f3 dir/] [--amend] [--no-verify] [--respect-staged] ``` +> `--files-removed ` (#4208): the caller-declared deletions. A `--files` entry that is missing on disk is skipped, never staged as a deletion (#2014) — so a moved file's old path cannot be recorded through `--files` at all, and the only form that recorded a move was a directory entry, which also commits any unrelated file sitting in that directory. Each `--files-removed` entry names a file, or a directory whose tracked-but-absent files are the removals; those paths are staged as deletions and join the commit pathspec. "Tracked" means in the index or in `HEAD`, so a deletion the caller already staged with `git rm` is committed too; "present" is the path itself (`lstat`), so a symlink counts as present even when its target is gone. A file entry that is still present on disk fails the commit closed (`reason: 'staging_failed'`); a path git never tracked is a no-op. Absence alone is not removal: an index entry that is absent from the worktree by design — a submodule gitlink, a skip-worktree (sparse-checkout) path, an assume-unchanged path, an unmerged entry, an intent-to-add (`git add -N`) entry — is never staged as a deletion; under a directory entry it is left alone like a present file, and named directly (by any spelling that resolves to it) it fails closed naming the state. On a staging failure the rollback puts back every index entry this call removed with its recorded mode and blob (`update-index --cacheinfo`), including on an unborn `HEAD` where `git reset` has nothing to restore from; like the addition-side reset it is best-effort — an index that cannot be written reports the staging error, not a clean rollback. `--files` keeps its skip-if-missing contract unchanged. A move is therefore `--files new/path --files-removed old/path`. GSD's own `close_phase_todos` step (`execute-phase.md`) uses exactly that form, naming each moved todo on both sides rather than passing the two directories: a directory entry would also commit an unrelated todo a concurrent session dropped into `pending/` or `completed/` while the phase was closing. + > `--no-verify`: Skips pre-commit hooks. Used by parallel executor agents during wave-based execution to avoid build lock contention (e.g., cargo lock fights in Rust projects). The orchestrator runs hooks once after each wave completes. Do not use `--no-verify` during sequential execution — let hooks run normally. > `--files ` **staging behaviour**: by default, `--files` runs `git add -- ` for each named file before committing. This overwrites any per-hunk staging set up via `git add -p`. Pass `--respect-staged` to skip the `git add` step and commit only what is already in the index within the requested pathspec. If nothing is staged within that scope, the command returns `{ committed: false, reason: 'nothing staged' }` without error. The trailing `-- ` pathspec on the commit is applied under both modes, so files staged outside the `--files` scope are never included (#3061 invariant). diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index 389ef811f..d575db4e3 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -950,15 +950,27 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load function routeCommit({ args, cwd, raw, error }) { const amend = args.includes('--amend'); const noVerify = args.includes('--no-verify'); - const filesIndex = args.indexOf('--files'); + // #4208: `--files` and `--files-removed` are two path lists, each + // running from its flag to the NEXT LIST FLAG. A boolean flag + // inside a list (`--files a --amend b`) is skipped, not a + // terminator: that is what the previous slice-to-end collection + // did (it filtered `--` tokens and kept everything else), and a + // list that stopped at any `--` token silently dropped `b` + // (review of #4253). The previous form could not carry a second + // list flag at all, which is the only thing that changed. + // A REPEATED list flag (`--files a --files b`) merges, as the old + // slice-to-end parse merged it: every occurrence contributes its + // run, and none of them ends another's silently. + const firstListFlag = args.findIndex((a, i) => i > 0 && COMMIT_LIST_FLAGS.has(a)); // Collect all positional args between command name and first flag, // then join them — handles both quoted ("multi word msg") and // unquoted (multi word msg) invocations from different shells - const endIndex = filesIndex !== -1 ? filesIndex : args.length; + const endIndex = firstListFlag !== -1 ? firstListFlag : args.length; const messageArgs = args.slice(1, endIndex).filter(a => !a.startsWith('--')); const message = messageArgs.join(' ') || undefined; - const files = filesIndex !== -1 ? args.slice(filesIndex + 1).filter(a => !a.startsWith('--')) : []; - commands.cmdCommit(cwd, message, files, raw, amend, noVerify); + const files = collectListFlagValues(args, '--files'); + const filesRemoved = collectListFlagValues(args, '--files-removed'); + commands.cmdCommit(cwd, message, files, raw, amend, noVerify, filesRemoved); } function routeCheckCommit({ args, cwd, raw, error }) { @@ -4280,6 +4292,31 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load * declares that file "All OS-facing I/O; single platform seam", and a private * duplicate here is what made it untrue. */ +const COMMIT_LIST_FLAGS = new Set(['--files', '--files-removed']); + +// #4208 review: hoisted out of routeCommit's closure so the parser is reachable +// from a test. It is the whole of the two-list argument contract, and its edge +// cases (a boolean flag inside a run, a repeated list flag, either order) were +// already the subject of a review round -- a parser that only the CLI can reach +// can only be tested by example, one spawn at a time. +// +// Every occurrence of `flag` contributes a run; a run ends at the next LIST +// flag and skips boolean flags on the way, so no token strictly between one +// list flag and the next is ever dropped. Repeated runs of the same flag merge, +// as the pre-#4208 slice-to-end parse merged them. +function collectListFlagValues(args, flag) { + const values = []; + args.forEach((a, i) => { + if (a !== flag) return; + for (const b of args.slice(i + 1)) { + if (COMMIT_LIST_FLAGS.has(b)) break; + if (b.startsWith('--')) continue; + values.push(b); + } + }); + return values; +} + function resolveSpawnBinary(name, platform = process.platform, env = process.env) { const { resolveExecutableBinary } = require('./lib/shell-command-projection.cjs'); return resolveExecutableBinary(name, { platform, env }); @@ -5199,6 +5236,10 @@ module.exports = { // #3275: exported for tests — the shared PATH+PATHEXT resolver behind // review-lane invoke's `deps.spawn` / `deps.hasBinary` seams. resolveSpawnBinary, + // #4208 review: exported for tests — the two-list commit parser is otherwise + // reachable only by spawning the CLI, which a property test cannot afford. + collectListFlagValues, + COMMIT_LIST_FLAGS, // #3714 follow-up: exported for tests — the dispatch model-pin VALUE // policy (charset accept/render parity, max-length boundary, leading-char // anchor) is otherwise unreachable from outside the dispatchOverlayCapabilityCommand closure. diff --git a/gsd-core/workflows/cleanup.md b/gsd-core/workflows/cleanup.md index 6f11e9790..7b314e384 100644 --- a/gsd-core/workflows/cleanup.md +++ b/gsd-core/workflows/cleanup.md @@ -223,9 +223,11 @@ Notes: Commit the changes: ```bash -gsd_run query commit "chore: archive phase directories from completed milestones" --files .planning/milestones/ .planning/phases/ .planning/quick/ .planning/STATE.md +gsd_run query commit "chore: archive phase directories from completed milestones" --files .planning/milestones/ .planning/STATE.md --files-removed .planning/phases/ .planning/quick/ ``` +`.planning/phases/` and `.planning/quick/` go under `--files-removed`, not `--files` (#4208): a `--files` directory entry stages everything under it, so it would also commit any in-flight phase or quick-task file a concurrent session had written there. `--files-removed` stages only the tracked files under those directories that the archival `mv` moved away, and leaves everything still present untouched. + diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index c62a88174..14f8fb236 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -1381,42 +1381,37 @@ Copy failure must NOT block phase completion. -**Auto-close pending todos tagged for this phase (#2433).** - -After `update_roadmap`, moves todos whose `resolves_phase` matches to `completed/`. +**Auto-close todos whose `resolves_phase` matches this phase (#2433)**, after `update_roadmap`. ```bash shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null -PHASE_NUM="${PHASE_NUMBER}" PENDING_DIR=".planning/todos/pending" COMPLETED_DIR=".planning/todos/completed" mkdir -p "$COMPLETED_DIR" - +PHASE_NUM="${PHASE_NUMBER}" #2576 normalize_phase_num() { - local p="${1//\"/}"; printf '%s' "$p" | sed 's/^0*\([0-9]\)/\1/' + printf '%s' "${1//\"/}" | sed 's/^0*\([0-9]\)/\1/' } PHASE_NUM_NORM=$(normalize_phase_num "$PHASE_NUM") - CLOSED=() for TODO_FILE in "$PENDING_DIR"/*.md; do [ -f "$TODO_FILE" ] || continue RP=$(awk '/^---/{c++;next} c==1 && /^resolves_phase:/{print $2;exit} c==2{exit}' "$TODO_FILE" 2>/dev/null || true) RP_NORM=$(normalize_phase_num "$RP") - if [ -n "$RP_NORM" ] && [ "$RP_NORM" = "$PHASE_NUM_NORM" ]; then - mv "$TODO_FILE" "$COMPLETED_DIR/" - CLOSED+=("$(basename "$TODO_FILE")") - fi + [ -n "$RP_NORM" ] && [ "$RP_NORM" = "$PHASE_NUM_NORM" ] || continue + mv "$TODO_FILE" "$COMPLETED_DIR/" + CLOSED+=("$(basename "$TODO_FILE")") done - if [ ${#CLOSED[@]} -gt 0 ]; then - gsd_run query commit "docs(phase-${PHASE_NUMBER}): close ${#CLOSED[@]} resolved todo(s)" --files .planning/todos/completed/ .planning/todos/pending/ .planning/STATE.md|| true - echo "◆ Closed ${#CLOSED[@]} todo(s) resolved by Phase ${PHASE_NUMBER}:" - for f in "${CLOSED[@]}"; do echo " ✓ $f"; done + ADDED=(); REMOVED=() + for f in "${CLOSED[@]}"; do ADDED+=("$COMPLETED_DIR/$f"); REMOVED+=("$PENDING_DIR/$f"); done + gsd_run query commit "docs(phase-${PHASE_NUMBER}): close ${#CLOSED[@]} resolved todo(s)" --files "${ADDED[@]}" .planning/STATE.md --files-removed "${REMOVED[@]}" || true + echo "◆ Closed ${#CLOSED[@]} todo(s) for Phase ${PHASE_NUMBER}:"; printf ' ✓ %s\n' "${CLOSED[@]}" fi ``` -**No matches:** skip silently (always additive, non-blocking). +No matches: skip silently, never blocks. diff --git a/scripts/lib/macos-conformance-tier.generated.cjs b/scripts/lib/macos-conformance-tier.generated.cjs index c52e2426c..01ce918d5 100644 --- a/scripts/lib/macos-conformance-tier.generated.cjs +++ b/scripts/lib/macos-conformance-tier.generated.cjs @@ -43,6 +43,7 @@ module.exports = { "tests/codex-config.test.cjs", "tests/commands.test.cjs", "tests/commit-docs-bypass.test.cjs", + "tests/commit-files-deletion.test.cjs", "tests/commit-files-pathspec.test.cjs", "tests/commonjs-marker.test.cjs", "tests/completion-ratio-scope-withholding.test.cjs", diff --git a/src/commands.cts b/src/commands.cts index c84dd8cd5..95f9ccbe3 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1583,7 +1583,349 @@ const COMMIT_DOCS_SKIP_REASON: Record, stri gitignore: 'skipped_gitignored', }; -function cmdCommit(cwd: string, message: string | undefined, files: string[] | undefined, raw: boolean, amend: boolean, noVerify: boolean): void { +type DeclaredRemoval = { path: string; mode: string; sha: string }; +type StagingFailure = { file: string; error: string; timed_out: boolean }; + +// #4208 review: the declared-removal staging lifted out of cmdCommit, which was +// already a critical-risk hotspot before this flag existed. Pure motion -- the +// classification, canonicalisation and entry recording below are unchanged; only +// the two accumulators are local names that the caller merges. `removedPathspec` +// is what joins the commit's pathspec; `removedEntries` is what the caller's +// rollback and its no-change exits restore from. +function stageDeclaredRemovals(cwd: string, removedDeclared: string[]): { + removedEntries: DeclaredRemoval[]; + removedPathspec: string[]; + failures: StagingFailure[]; +} { + const failures: StagingFailure[] = []; + const removedPathspec: string[] = []; + // #4208: caller-declared removals. The #2014 guard above skips a missing + // `--files` entry because the filesystem cannot tell "moved away" from "not + // written yet" — only the caller can. `--files-removed` is where the caller + // says it: every tracked path it names that is absent from disk is staged as + // a deletion and joins the commit pathspec, so a move is recorded at file + // granularity without a directory entry that also sweeps in whatever else + // happens to sit in that directory (a concurrent session's uncommitted todo, + // in the motivating execute-phase sweep). `--files` keeps its skip-if-missing + // contract untouched: the two lists are disjoint by construction and only the + // caller populates the second. + // + // An entry may name a file or a directory. `git ls-files` resolves both the + // same way — a file matches itself, a directory its tracked descendants — + // and the subsequent absent-from-disk filter is what makes the directory + // form precise: a tracked file that is still present is NOT a removal and is + // never touched, and an untracked file under the directory is invisible to + // `ls-files` in the first place. So `--files-removed .planning/phases/` + // after an archival `mv` stages exactly the moved-away tracked files. + // + // A FILE entry that is still present on disk contradicts the declaration + // and fails closed as a staging failure rather than being reinterpreted: + // staging a deletion of a present file would commit a removal git then + // reports as untracked — #2014's failure from the other side. A path that + // was never tracked is a no-op (a todo created and moved within the same + // phase has nothing to remove) rather than an error. + // + // `-z` keeps `core.quotePath` from octal-escaping non-ASCII names — the + // same trap the assume-unchanged probe below documents for `ls-files -v`. + // + // Presence is `lstat`, never `stat` / `existsSync`: both of those FOLLOW a + // symlink, so a tracked link whose target is gone reads as absent, gets its + // index entry removed, and the worktree still holds the link — the commit + // then finds no difference against HEAD, reports `nothing_to_commit`, and + // leaves the deletion staged (driven at review). To git a symlink is a + // tracked path in its own right; presence means the link, not its target. + // + // "Tracked" is the index UNION HEAD. The index alone misses a deletion the + // caller already staged (`git rm` before this call): the entry is gone from + // the index, so `ls-files` never lists it, it never reaches the pathspec, + // and a removal-only call reports `nothing_to_commit` with the deletion + // still staged (driven at review). HEAD still has it, and `rm --cached + // --ignore-unmatch` on an already-removed entry is a no-op, so the union + // costs nothing on the ordinary path. On an unborn HEAD the union is + // index-only, and an absent index-only path is unstaged but never joins the + // pathspec: a root commit has no parent to delete it from, and naming it + // makes `git commit` refuse with "pathspec did not match" (driven). + // + // Only ENOENT / ENOTDIR establish absence. Any other `lstat` error (EPERM, + // EIO) is not "the caller removed this" and fails closed as a staging + // failure rather than staging a deletion of a path that may well exist. + // + // And absence alone does not establish REMOVAL (#4208 review). Some index + // entries are absent from the worktree BY DESIGN, and `lstat` cannot tell + // them from a path the caller moved away: a submodule gitlink (mode + // `160000`) whose directory was deleted by hand — `git ls-files` lists it + // like any file, and `rm --cached` would detach the submodule with no + // `.gitmodules` cleanup; a `--skip-worktree` path, which a cone-mode sparse + // checkout never materialises at all, so a directory entry over a + // sparse-excluded tree would drop that whole tree from the index; an + // `--assume-unchanged` path, whose worktree state git itself does not + // consult; an unmerged entry. So the index listing carries each entry's + // `ls-files -v` tag, mode and stage alongside the path, and only a plain + // cached (`H`), stage-0, non-gitlink entry is a removal candidate. Every + // other state is "not this call's removal to make": under a directory + // entry it is left alone, exactly like a present file; named directly it + // contradicts the declaration and fails closed, naming the state. The + // domain this enumeration covers is what `ls-files -v -s` can emit for an + // index entry — tags `H`/`S`/`M`/`h` (the `R`/`C`/`K`/`?` letters belong to + // the `-d`/`-m`/`-k`/`-o` listing modes, never a bare `-s`), modes + // `100644`/`100755`/`120000` (a symlink is a candidate; presence is the + // link) /`160000`, and `040000` only under `--sparse`, which is not passed. + // The same `-v` read the assume-unchanged probe below performs for the + // ADDITION side, applied here to the removal side. + type IndexEntry = { tag: string; mode: string; sha: string; stage: string }; + // The empty blob under SHA-1 and SHA-256 object formats — intent-to-add's tell. + const EMPTY_BLOBS = new Set(['e69de29bb2d1d6434b8b29ae775ad8c2e48c5391', '473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813']); + // A PATH FROM THE INDEX IS NOT A PATHSPEC. `git rm`, `ls-files` and friends + // parse their operands as pathspecs, so a tracked file literally named + // `.planning/*.md` GLOBS when handed back to git: driven, `rm --cached` on it + // also removed `peer.md` and `stays.md`, and only the declared entry was + // recorded — so the rollback restored one of three and the other two rode out + // as undisclosed staged deletions. The magic-prefix twin is quieter still: a + // file named `:(literal)mine` has its prefix PARSED, so the rm matches nothing, + // exits 0, and the entry silently survives a removal this call then claims. + // `:(literal)` disables every other magic, including globbing, so the operand + // means the file it names. + const lit = (p: string): string => `:(literal)${p}`; + const notARemoval = (e: IndexEntry): string | null => { + if (e.mode === '160000') return 'a submodule gitlink, not a file'; + if (e.tag === 'S') return 'skip-worktree (sparse-checkout): absent by checkout, not removed'; + if (e.tag === 'h') return 'assume-unchanged: git does not consult its worktree state'; + if (e.stage !== '0') return 'an unmerged index entry'; + if (e.tag !== 'H') return `index state '${e.tag}'`; + return null; + }; + const lstatState = (p: string): 'present' | 'absent' | NodeJS.ErrnoException => { + try { + fs.lstatSync(p); + return 'present'; + } catch (e) { + const err = e as NodeJS.ErrnoException; + return err.code === 'ENOENT' || err.code === 'ENOTDIR' ? 'absent' : err; + } + }; + // `rev-parse -q --verify HEAD` exits 1 both for an unborn HEAD and for a + // spawn timeout (`execGit` collapses one to `exitCode: 1`). Only a probe that + // actually answered may downgrade the union to index-only; an unanswered one + // fails closed, because silently dropping the HEAD half re-opens the + // pre-staged-deletion omission this union exists to close. + let headExists = false; + let headProbeFailure: { error: string; timed_out: boolean } | null = null; + if (removedDeclared.length > 0) { + const headProbe = execGit(['rev-parse', '-q', '--verify', 'HEAD'], { cwd }); + if (headProbe.exitCode === 0) { + headExists = true; + } else if (isSpawnTimeout(headProbe) || headProbe.error !== null) { + headProbeFailure = { error: headProbe.stderr || headProbe.stdout || 'HEAD probe failed', timed_out: isSpawnTimeout(headProbe) }; + } + } + // Every index entry this call removes, recorded BEFORE the `rm --cached` + // so the rollback below can put it back exactly — mode and blob — with + // `update-index --cacheinfo`. `git reset -- ` cannot do that: it + // restores from HEAD, which does not exist on an unborn branch (so a root + // commit's failed call used to leave every earlier removal unstaged, in + // violation of the only-what-THIS-call-staged invariant above) and which + // is not what the index held when the caller had pre-staged a modified + // blob at that path. Recording the entry answers both without putting the + // path on the commit pathspec, where an unborn HEAD makes `git commit` + // refuse it (driven; see the union note above). + const removedEntries: Array<{ path: string; mode: string; sha: string }> = []; + for (const entry of removedDeclared) { + if (headProbeFailure !== null) { + failures.push({ file: entry, ...headProbeFailure }); + continue; + } + // `-v -s`: tag, mode, blob, stage and path per record — see notARemoval. + // `lit` here too: the caller's declared entry is a PATH, not a glob — + // that is `--files-removed`'s whole contract — and :(literal) still + // resolves a directory to its descendants (driven), so the directory form + // is unaffected while a file literally named `*.md` or `:(literal)x` means + // itself. + const listed = execGit(['ls-files', '-v', '-s', '-z', '--', lit(entry)], { cwd }); + if (listed.exitCode !== 0) { + failures.push({ + file: entry, + error: listed.stderr || listed.stdout, + timed_out: isSpawnTimeout(listed), + }); + continue; + } + const indexed = new Map(); + let unparseable: string | null = null; + for (const rec of listed.stdout.split('\0').filter(Boolean)) { + const m = /^(\S) (\d{6}) ([0-9a-f]+) ([0-3])\t([\s\S]+)$/.exec(rec); + if (m === null) { unparseable = rec; break; } + indexed.set(m[5], { tag: m[1], mode: m[2], sha: m[3], stage: m[4] }); + } + if (unparseable !== null) { + // A record this code cannot read is not a path it may remove. + failures.push({ file: entry, error: `unparseable ls-files record: ${unparseable}`, timed_out: false }); + continue; + } + const tracked = new Set(indexed.keys()); + // Does the entry name THIS tracked path itself (the caller declared a + // FILE removed) or a directory above it? Decided on RESOLVED paths, never + // on the strings: `ls-files` prints cwd-relative paths, and a caller may + // pass an absolute path, `./x`, a trailing slash, or run under `--cwd`, + // any of which fails a string compare and would silently take the + // directory polarity — a directly named gitlink then SKIPS instead of + // refusing (found by the round's review, driven with an absolute path). + const entryAbs = path.resolve(cwd, entry); + const entryRel = path.relative(cwd, entryAbs).split(path.sep).join('/'); + // Canonical form: realpath of the longest EXISTING prefix, with the absent + // tail re-appended. The declared path is usually absent (that is the + // point), and `process.cwd()` returns the real path where the caller may + // hold a symlinked spelling — macOS `/var` → `/private/var` is the live + // instance (CI, this PR's own test) — so a resolve-only compare still + // took the directory polarity there. + const canon = (p: string): string => { + let cur = path.resolve(cwd, p); const tail: string[] = []; + for (;;) { + try { return path.join(fs.realpathSync.native(cur), ...tail); } catch { /* absent: climb */ } + const parent = path.dirname(cur); + if (parent === cur) return path.join(cur, ...tail); + tail.unshift(path.basename(cur)); cur = parent; + } + }; + const namesItself = (p: string): boolean => p === entryRel || path.resolve(cwd, p) === entryAbs || canon(p) === canon(entry); + const inHeadPaths = new Set(); + if (headExists) { + const inHead = execGit(['ls-tree', '-r', '-z', '--name-only', 'HEAD', '--', lit(entry)], { cwd }); + if (inHead.exitCode !== 0) { + failures.push({ + file: entry, + error: inHead.stderr || inHead.stdout, + timed_out: isSpawnTimeout(inHead), + }); + continue; + } + for (const p of inHead.stdout.split('\0').filter(Boolean)) { tracked.add(p); inHeadPaths.add(p); } + } + if (tracked.size === 0) continue; + const entryState = lstatState(path.resolve(cwd, entry)); + if (entryState !== 'present' && entryState !== 'absent') { + failures.push({ file: entry, error: `lstat ${entryState.code ?? ''}: ${entryState.message}`, timed_out: false }); + continue; + } + let entryIsDirectory = false; + if (entryState === 'present') { + try { entryIsDirectory = fs.lstatSync(path.resolve(cwd, entry)).isDirectory(); } catch { /* raced away: treat as a present non-directory below */ } + } + if (entryState === 'present' && !entryIsDirectory) { + // A present non-directory entry (a file, or ANY symlink — a link to a + // directory is still one tracked path) contradicts the declaration. + failures.push({ + file: entry, + error: `declared in --files-removed but still present on disk: ${entry}`, + timed_out: false, + }); + continue; + } + for (const trackedPath of tracked) { + const indexEntry = indexed.get(trackedPath); + let reason = indexEntry === undefined ? null : notARemoval(indexEntry); + // Intent-to-add (`git add -N`) renders as a plain `H 100644 0` — the flag is not in the listing — yet nothing tracked exists + // to remove, and a rollback via `--cacheinfo` cannot restore the flag. + // It is the one state whose blob is the empty blob, whose path is not in + // HEAD, and which `diff --cached` treats as absent from the index; an + // ordinary staged empty file shows there as added. Three probes, on the + // rare empty-blob path only. + if (reason === null && indexEntry !== undefined && EMPTY_BLOBS.has(indexEntry.sha) && !inHeadPaths.has(trackedPath)) { + const cached = execGit(['diff', '--cached', '--name-only', '-z', '--', lit(trackedPath)], { cwd }); + if (cached.exitCode === 0 && cached.stdout.split('\0').filter(Boolean).length === 0) reason = 'an intent-to-add entry (git add -N), not tracked content'; + } + if (reason !== null) { + if (namesItself(trackedPath)) { + failures.push({ + file: entry, + error: `declared in --files-removed but is ${reason}: ${trackedPath}`, + timed_out: false, + }); + } + continue; + } + const state = lstatState(path.resolve(cwd, trackedPath)); + if (state === 'present') continue; + if (state !== 'absent') { + failures.push({ file: trackedPath, error: `lstat ${state.code ?? ''}: ${state.message}`, timed_out: false }); + continue; + } + // A HEAD-only path (the caller already `git rm`'d it) has no index entry + // to record or restore; the `rm` below is then a no-op. + // READ the entry before the mutation, RECORD it only after the mutation + // SUCCEEDS. The read must precede (the rm is what destroys the mode/blob + // the restore needs); the record must not, because `removedEntries` is + // the set this call claims to have staged. Recording ahead of the rm made + // a FAILED rm — a stale `index.lock` is the driven case — contribute an + // entry the rollback then reported as "still staged in the index" when + // nothing had been staged at all: a false disclosure, the mirror of the + // silent one the disclosure was added to fix. + const recordable = indexEntry !== undefined + ? { path: trackedPath, mode: indexEntry.mode, sha: indexEntry.sha } + : null; + // `--ignore-unmatch` makes "no such index entry" a success, so a non-zero + // exit is a real I/O failure — same reading as the default-mode branch. + const rmResult = execGit(['rm', '--cached', '--ignore-unmatch', '--', lit(trackedPath)], { cwd }); + if (rmResult.exitCode === 0) { + if (recordable !== null) removedEntries.push(recordable); + // Re-check AFTER the index mutation. The absence test and the `rm` are + // not atomic, and the scoped `git commit -- ` below reads the + // WORKTREE, so a path recreated in between would be committed as its + // new content under a message that declared it removed. A reappearance + // is a contradiction like any other: staging failure, and the rollback + // restores the recorded entry. Narrows the window; does not close it. + if (lstatState(path.resolve(cwd, trackedPath)) !== 'absent') { + failures.push({ + file: trackedPath, + error: `declared in --files-removed but reappeared on disk: ${trackedPath}`, + timed_out: false, + }); + continue; + } + // Unborn HEAD: nothing to delete FROM, so the path is unstaged only and + // never joins the pathspec; its rollback is the recorded entry above. + if (headExists) removedPathspec.push(trackedPath); + } else { + // A NON-ZERO rm is NOT proof the index is untouched. `execGit` collapses + // a spawn timeout to a non-zero exit, and a killed `git rm` can already + // have written the index — so keying the record on the exit code alone + // drops a real mutation on the timeout path (driven: a post-index-change + // hook that outlives the timeout leaves `D ` staged and reported + // nowhere). The exit code answers "did the command succeed", never "did + // the index change". ASK THE INDEX instead — three honest arms, and no + // arm asserts a state it did not observe. + // THE ORIGINAL FAILURE IS PUSHED FIRST. `failures[0]` sets the result's + // `reason`, `file`, `error` and timeout classification, so appending the + // probe's diagnostic ahead of it renamed the cause: a timed-out rm was + // reported as a permission error and lost its `timed_out: true`. + failures.push({ + file: trackedPath, + error: rmResult.stderr || rmResult.stdout, + timed_out: isSpawnTimeout(rmResult), + }); + if (recordable !== null) { + const after = execGit(['ls-files', '-s', '-z', '--', lit(trackedPath)], { cwd }); + if (after.exitCode !== 0) { + // Could not determine. Say so; never silently assume either way. + failures.push({ + file: trackedPath, + error: `removal failed and the index state for this path could NOT be determined: ${after.stderr || after.stdout}`, + timed_out: isSpawnTimeout(after), + }); + } else if (after.stdout.replace(/\0/g, '').trim() === '') { + // The entry is gone: the rm mutated the index before it failed, so + // this call owns the removal and must restore/disclose it. + removedEntries.push(recordable); + } + // else: the entry is still there — nothing was staged, nothing to undo. + } + } + } + } + return { removedEntries, removedPathspec, failures }; +} + +function cmdCommit(cwd: string, message: string | undefined, files: string[] | undefined, raw: boolean, amend: boolean, noVerify: boolean, filesRemoved?: string[]): void { if (!message && !amend) { error('commit message required'); } @@ -1716,8 +2058,12 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u } // Stage files - const explicitFiles = files && files.length > 0; - const filesToStage = explicitFiles ? files : ['.planning/']; + // #4208: `--files-removed` is a declared scope in its own right — a caller + // that names only removals must not fall through to the unscoped + // `.planning/` sweep, which would commit everything under it. + const removedDeclared = filesRemoved ?? []; + const explicitFiles = (files && files.length > 0) || removedDeclared.length > 0; + const filesToStage = explicitFiles ? (files ?? []) : ['.planning/']; const stagedPaths: string[] = []; // #2608: a `git add` that fails must abort the commit, not be skipped. // #2523 stopped a failed path entering the commit pathspec, but skipping it @@ -1734,9 +2080,21 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // Paths already in the index BEFORE this call. On a staging failure the // rollback below unstages only what THIS call added — unstaging a path the // caller had staged themselves would destroy their work. + // `-z`: without it `core.quotePath` renders a non-ASCII name as + // `"caf\303\251.md"`, which never equals the raw path in `stagedPaths`, so + // the rollback below would treat a caller-pre-staged `café.md` as this + // call's own and unstage it (#4208 review, driven). + // `--relative`: `diff --cached` prints REPO-relative paths whatever the cwd, + // while `stagedPaths` holds the caller's own cwd-relative names. In a project + // nested inside its repo (`/sub/.planning/...`) the two name spaces + // never intersect, so `preStaged` matched NOTHING and the rollback unstaged + // every path including the caller's own pre-staged work. Driven on a nested + // fixture: a caller-staged deletion vanished from `diff --cached` after an + // unrelated declaration failed. Pre-existing -- it governs the `--files` side + // too -- and a no-op when the project IS the repo root. const preStaged = new Set( - execGit(['diff', '--cached', '--name-only'], { cwd }) - .stdout.split('\n').map(s => s.trim()).filter(Boolean), + execGit(['diff', '--cached', '--name-only', '-z', '--relative'], { cwd }) + .stdout.split('\0').filter(Boolean), ); for (const file of filesToStage) { const fullPath = path.resolve(cwd, file); @@ -1784,6 +2142,90 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u } } + // #4208: caller-declared removals -- see stageDeclaredRemovals. + const declaredRemovals = stageDeclaredRemovals(cwd, removedDeclared); + const removedEntries = declaredRemovals.removedEntries; + stagingFailures.push(...declaredRemovals.failures); + stagedPaths.push(...declaredRemovals.removedPathspec); + // A REMOVAL'S PATH IS A PATH DOWNSTREAM TOO. Literalising the staging alone + // does not protect the COMMIT's own pathspec: with a tracked file literally + // named `.planning/*.md` declared removed beside a MODIFIED `peer.md`, the + // `git commit -- ` below globs and commits `M peer.md` the caller + // never declared — the sweep this flag exists to remove, arriving one step + // later. Driven. Only the removal-derived entries are literalised: `--files` + // entries keep whatever pathspec behaviour they have today, which is not this + // change's to alter. + const removalPathspecs = new Set(declaredRemovals.removedPathspec); + const asPathspec = (p: string): string => (removalPathspecs.has(p) ? `:(literal)${p}` : p); + + // Put every entry this call removed back, exactly — mode and blob. Called + // from EVERY exit that mutated the index and then records nothing, not just + // the staging-failure rollback: a `git rm --cached` that SUCCEEDS is still an + // index mutation this call owns, and an exit reporting `nothing_to_commit` + // tells the caller no state changed. Leaving the removal staged there makes + // that report false and hands the removal to the caller's NEXT commit. + // Returns FALSE when the restore itself failed. The rollback path below may + // ignore that (it is already reporting a failure, and an unwritable index is + // usually the failure being reported); the no-change exits may NOT. Reporting + // `nothing_to_commit` over a removal we tried and FAILED to put back is the + // same false "no state changed" this helper exists to prevent, surviving one + // level down on the restore-failure path. + // THREE outcomes, never two. `restored` and `not-restored` are observations; + // `unverified` is the absence of one, and collapsing it into `not-restored` + // asserts a failure that was never seen — the same conflation the removal + // side's own probe already refuses one screen up. + type RestoreVerdict = 'restored' | 'not-restored' | 'unverified'; + const restoreRemovedEntries = (): RestoreVerdict => { + if (removedEntries.length === 0) return 'restored'; + execGit(['update-index', '--add', ...removedEntries.flatMap(e => ['--cacheinfo', `${e.mode},${e.sha},${e.path}`])], { cwd }); + // VERIFY BY READING THE INDEX BACK, never by the exit code. `execGit` + // collapses a spawn timeout to a non-zero exit, and a killed `update-index` + // can already have written the index — so an exit code answers "did the + // command succeed", never "is the entry back". Driven: a post-index-change + // hook outliving the timeout made the restore report failure over an index + // it had in fact restored, publishing a disclosure that was simply false. + // + // `-z` IS LOAD-BEARING, and its absence is the #2014-era defect this PR + // already fixed once for `preStaged`: without it `core.quotePath` renders a + // non-ASCII name as `"caf\303\251.md"`, which never equals the raw path, so + // an exactly-restored `café.md` (and any name carrying a tab or a newline) + // read as NOT restored. Driven on all three shapes. + const back = execGit(['ls-files', '-s', '-z', '--', ...removedEntries.map(e => `:(literal)${e.path}`)], { cwd }); + if (back.exitCode !== 0) return 'unverified'; // no observation — never an assertion of failure + // COMPARE THE WHOLE ENTRY, not just the path. `--cacheinfo` restores mode, + // blob and stage; a path present at a DIFFERENT mode or blob is not the + // entry this call removed. Driven: a hook that rewrote the restored entry + // 100644 -> 100755 was reported as restored by a path-only test. + const present = new Map(); + for (const rec of back.stdout.split('\0')) { + if (rec === '') continue; + const tab = rec.indexOf('\t'); + if (tab === -1) continue; + present.set(rec.slice(tab + 1), rec.slice(0, tab)); + } + const ok = removedEntries.every(e => present.get(e.path) === `${e.mode} ${e.sha} 0`); + return ok ? 'restored' : 'not-restored'; + }; + // The no-change exits' shared arm: restore, and if the restore failed, say so + // instead of claiming nothing changed. `staging_failed` is the honest reason — + // the index carries a mutation this call made and could not undo. + const removalsLeftStaged = (verdict: 'not-restored' | 'unverified') => ({ + committed: false, + hash: null, + reason: 'staging_failed', + file: removedEntries[0]?.path ?? null, + error: verdict === 'not-restored' + ? `declared removal(s) staged but could not be restored after the commit recorded nothing: ${removedEntries.map(e => e.path).join(', ')}` + : `declared removal(s) staged and the restore could NOT be VERIFIED after the commit recorded nothing: ${removedEntries.map(e => e.path).join(', ')}`, + failures: removedEntries.map(e => ({ + file: e.path, + error: verdict === 'not-restored' + ? 'update-index --cacheinfo restore failed' + : 'update-index --cacheinfo restore could not be verified — the index was not readable', + timed_out: false, + })), + }); + // #2608: fail closed before `git commit` runs. Checked ahead of the // nothing_to_commit branch below so a run where EVERY path failed to stage // reports the staging cause rather than "nothing to commit", and ahead of the @@ -1798,10 +2240,37 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // best-effort: if the index is unwritable — the very failure being reported // — the reset cannot succeed either, and the staging error is still what // gets returned. - const toUnstage = stagedPaths.filter(p => !preStaged.has(p)); + const removedPaths = new Set(removedEntries.map(e => e.path)); + const toUnstage = stagedPaths.filter(p => !preStaged.has(p) && !removedPaths.has(p)); if (toUnstage.length > 0) { - execGit(['reset', '-q', '--', ...toUnstage], { cwd }); + // `asPathspec` here too. This reset is the LAST place a removal-derived + // name reaches git as a pathspec, and it is the most damaging: driven, + // a wildcard-named entry that slipped into `toUnstage` globbed and + // unstaged the CALLER'S OWN pre-staged deletion and modification, then + // reported only the contradiction that triggered the rollback. + execGit(['reset', '-q', '--', ...toUnstage.map(asPathspec)], { cwd }); } + // Removals are restored from the recorded entries, never via `reset` + // (no HEAD to reset to on an unborn branch; not the pre-staged blob when + // the caller had one) — and unconditionally, since a removal this call + // performed is this call's to undo whether or not the path was pre-staged. + // DISCLOSE a failed restore here too. The earlier reading -- that this exit + // is already reporting a failure, so the restore's result adds nothing -- + // is wrong, and the counterexample is the ordinary one: the reported + // failure is usually a DIFFERENT cause (a contradictory declaration, a + // reappeared path), so a caller reading `failures` sees only that cause + // and learns nothing about the removal still sitting in its index. Append + // rather than replace: the original failure is still the reason. + const restoreVerdict = restoreRemovedEntries(); + const failures = restoreVerdict === 'restored' + ? stagingFailures + : [...stagingFailures, ...removedEntries.map(e => ({ + file: e.path, + error: restoreVerdict === 'not-restored' + ? 'staged removal could NOT be restored during rollback — it is still staged in the index' + : 'staged removal was rolled back but the result could NOT be VERIFIED — the index was not readable', + timed_out: false, + }))]; const first = stagingFailures[0]; const result = { committed: false, @@ -1809,7 +2278,7 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u reason: first.timed_out ? 'staging_timeout' : 'staging_failed', file: first.file, error: first.error, - failures: stagingFailures, + failures, }; output(result, raw, 'failed'); return; @@ -2013,13 +2482,13 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // failing-closed (drop the content) and failing-open (re-enter #3776) are // wrong answers to a question we can just ask directly. const assumeUnchangedWouldRecord = (): boolean => { - const listed = execGit(['ls-files', '-v', '--', ...stagedPaths], { cwd }); + const listed = execGit(['ls-files', '-v', '--', ...stagedPaths.map(asPathspec)], { cwd }); // Only the TAG is read; the path is deliberately never parsed out — see the // `core.quotePath` note above, and the dry run below needs no path anyway. if (listed.exitCode === 0 && !listed.stdout.split('\n').some((line) => /^[a-z] /.test(line))) return false; const dryRun = execGit( - ['commit', '--dry-run', '--porcelain', '--no-verify', '-m', sanitizedMessage as string, '--', ...stagedPaths], + ['commit', '--dry-run', '--porcelain', '--no-verify', '-m', sanitizedMessage as string, '--', ...stagedPaths.map(asPathspec)], { cwd }, ); // Only a CONFIRMED "nothing to record" closes the path: rc 1 from a git @@ -2043,11 +2512,20 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u && (stagedPaths.length === 0 || (!partialCommitRefused && execGit( - ['diff', '--quiet', '--ignore-submodules=dirty', '--no-textconv', 'HEAD', '--', ...stagedPaths], + ['diff', '--quiet', '--ignore-submodules=dirty', '--no-textconv', 'HEAD', '--', ...stagedPaths.map(asPathspec)], { cwd }, ).exitCode === 0 && !assumeUnchangedWouldRecord())); if (nothingToCommit) { + // Nothing is being recorded, so any removal this call staged has no commit + // to land in. Put it back before reporting no state change. Reachable on + // two shapes, and keying on either one alone leaves the other broken: + // an unborn HEAD (a removal never joins `stagedPaths`, so the pathspec is + // empty), and a HEAD that simply does not carry the removed path -- an + // index-only entry the caller `git add`ed but never committed, where the + // `diff HEAD` probe reads clean because the path is absent on both sides. + const rv = restoreRemovedEntries(); + if (rv !== 'restored') { output(removalsLeftStaged(rv), raw, 'failed'); return; } // #4454: an explicit --files list where every named path was missing // reaches this branch via `stagedPaths.length === 0` above — surface // which path(s) were the reason, same as the success result below. @@ -2067,7 +2545,7 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u : ['commit', '-m', sanitizedMessage as string]; if (noVerify) commitArgs.push('--no-verify'); if (canScope) { - commitArgs.push('--', ...stagedPaths); + commitArgs.push('--', ...stagedPaths.map(asPathspec)); } // #3859 follow-up: on git 2.39.5 (confirmed on the CI Linux bench image, // ghcr.io/open-gsd/gsd-tester-linux:v1.8.0-node24; NOT reproducible on git @@ -2125,6 +2603,13 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u return; } if (commitResult.stdout.includes('nothing to commit') || commitResult.stderr.includes('nothing to commit')) { + // Same reading as the guard above: git recorded nothing, so a removal + // this call staged must not be left behind under a `nothing_to_commit` + // report. The failure exits below are deliberately NOT restored -- they + // report a failure rather than "no state changed", and the addition side + // leaves its own staged paths in place there too. + const rv = restoreRemovedEntries(); + if (rv !== 'restored') { output(removalsLeftStaged(rv), raw, 'failed'); return; } // #4454: this is the residual window the surrounding comments already // document (a partial skip + partialCommitRefused bypassing the diff // probe + git's own empty-commit refusal) — skippedFiles can be diff --git a/tests/close-phase-todos-stage-deletion.test.cjs b/tests/close-phase-todos-stage-deletion.test.cjs index e9646902a..e1cab2351 100644 --- a/tests/close-phase-todos-stage-deletion.test.cjs +++ b/tests/close-phase-todos-stage-deletion.test.cjs @@ -7,11 +7,13 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const EXECUTE_PHASE = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md'); +const CLEANUP = path.join(__dirname, '..', 'gsd-core', 'workflows', 'cleanup.md'); describe('#2415: close_phase_todos must stage the pending/ deletion alongside completed/', () => { - test('the close_phase_todos commit --files list includes .planning/todos/pending/', () => { + test('close_phase_todos stages the completed/ destination and the pending/ deletion (via --files-removed since #4208)', () => { const content = fs.readFileSync(EXECUTE_PHASE, 'utf8'); // Isolate the close_phase_todos step body so we don't match unrelated --files lists @@ -22,19 +24,30 @@ describe('#2415: close_phase_todos must stage the pending/ deletion alongside co assert.ok(stepEnd > stepStart, 'close_phase_todos step must be properly closed'); const stepBody = content.slice(stepStart, stepEnd); - // The commit must include BOTH the destination (completed/) AND the source (pending/) - // — git add of pending/ stages the deletion of each moved file. Without pending/ in - // the list, only the new completed/ copy gets committed and the moved-away file - // persists as an unstaged deletion in git status until some later broad git add -A - // happens to catch it (#2415). + // The commit must reach BOTH the destination (completed/) AND the source-side + // deletion (pending/). Without the source side, only the new completed/ copy gets + // committed and the moved-away file persists as an unstaged deletion in git status + // until some later broad git add -A happens to catch it (#2415). + // + // #4208 changed the MECHANISM, not that guarantee. The step used to pass the two + // directories to --files, which also committed any unrelated todo a concurrent + // session had dropped into pending/ or completed/ mid-close. It now names each + // moved todo: destinations via the ADDED array under --files, and the source-side + // deletions via the REMOVED array under --files-removed (--files alone cannot + // record a deletion — a missing --files entry is skipped, never staged, per #2014). const gsdRunCommit = /gsd_run\s+query\s+commit\b[^\n]*--files\s+([^\n]+)/; const match = stepBody.match(gsdRunCommit); assert.ok(match, `close_phase_todos step must contain a gsd_run query commit ... --files invocation. Step body:\n${stepBody}`); const filesList = match[1]; - assert.match(filesList, /\.planning\/todos\/completed/, 'commit --files must include .planning/todos/completed/ (destination of the move)'); - assert.match(filesList, /\.planning\/todos\/pending/, 'commit --files must include .planning/todos/pending/ so the moved-away file is staged as a deletion (#2415)'); + assert.match(filesList, /"\$\{ADDED\[@\]\}"/, 'commit --files must carry the ADDED array (destinations of the move)'); + assert.match(filesList, /--files-removed\s+"\$\{REMOVED\[@\]\}"/, 'the moved-away file must be staged as a deletion via --files-removed (#2415, #4208)'); assert.match(filesList, /\.planning\/STATE\.md/, 'commit --files must still include .planning/STATE.md (the step also updates state)'); + + // The arrays are only worth asserting if they are built from the right two dirs: + // ADDED from completed/ (destination), REMOVED from pending/ (source). + assert.match(stepBody, /ADDED\+=\("\$COMPLETED_DIR\/\$f"\)/, 'ADDED must be built from $COMPLETED_DIR — the destination of the move'); + assert.match(stepBody, /REMOVED\+=\("\$PENDING_DIR\/\$f"\)/, 'REMOVED must be built from $PENDING_DIR — the source whose deletion #2415 requires'); }); test('close_phase_todos uses plain mv (not git mv) so untracked todos and non-git .planning dirs still work', () => { @@ -55,3 +68,46 @@ describe('#2415: close_phase_todos must stage the pending/ deletion alongside co assert.doesNotMatch(withoutComments, /\bgit\s+mv\b/, 'close_phase_todos must NOT use git mv as the actual move command — it fails on untracked todos and on non-git .planning dirs'); }); }); + +describe('#4208: cleanup.md archives phase directories without a --files directory sweep', () => { + // The other caller #4208 rewrote. execute-phase.md's equivalent rewrite is + // pinned above; this one was not, so a revert of the routing here would be + // caught by nothing -- the mechanism's own unit tests pass either way, + // because they never read this file. + function archiveStepBody() { + // splitLines (the text-lines seam), not a `[^\n]*` match over the whole + // file: a bare \n is CRLF-fragile under Windows autocrlf, and an unbounded + // quantifier over readFileSync content is the #2128 backtracking class. + const lines = splitLines(fs.readFileSync(CLEANUP, 'utf8')); + const line = lines.find(l => /gsd_run\s+query\s+commit\b/.test(l) && l.includes('--files-removed')); + assert.ok(line, `cleanup.md must commit the archive via gsd_run query commit ... --files-removed. Lines scanned: ${lines.length}`); + return line; + } + + test('the archive commit routes the moved-away directories through --files-removed, not --files', () => { + const line = archiveStepBody(); + const [added, removed] = line.split('--files-removed'); + + // The two directories the archival mv empties. Under --files a directory + // entry stages EVERYTHING under it, so an in-flight phase or quick-task + // file a concurrent session had written there would be committed too -- + // the sweep #4208 exists to remove. + for (const dir of ['.planning/phases/', '.planning/quick/']) { + assert.ok(removed.includes(dir), `${dir} must be under --files-removed. Line: ${line}`); + assert.ok(!added.includes(dir), `${dir} must NOT be under --files -- a directory entry there sweeps in concurrent writes. Line: ${line}`); + } + }); + + test('the destinations and STATE.md stay under --files, which cannot record a deletion', () => { + const line = archiveStepBody(); + const added = line.split('--files-removed')[0]; + // Assert the FLAG, not just the substrings: without this, deleting + // `--files` entirely leaves the destinations sitting before + // `--files-removed` and the test still passes (round review, MINOR). + assert.match(added, /--files\s/, `the additive half must actually carry --files. Line: ${line}`); + // --files keeps its #2014 skip-if-missing contract: it is the additive + // half and the only half that can carry a path that must be WRITTEN. + assert.ok(added.includes('.planning/milestones/'), `the archive destination must stay under --files. Line: ${line}`); + assert.ok(added.includes('.planning/STATE.md'), `STATE.md must stay under --files. Line: ${line}`); + }); +}); diff --git a/tests/commit-files-deletion.test.cjs b/tests/commit-files-deletion.test.cjs index 1eba8570b..6228424ca 100644 --- a/tests/commit-files-deletion.test.cjs +++ b/tests/commit-files-deletion.test.cjs @@ -14,6 +14,8 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs'); +const fc = require('fast-check'); +const { collectListFlagValues, COMMIT_LIST_FLAGS } = require('../gsd-core/bin/gsd-tools.cjs'); const { gitOrThrow } = require('./helpers/git-fixture.cjs'); // #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. const { GIT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); @@ -100,3 +102,1187 @@ describe('commit --files: missing files must not stage deletions (#2014)', () => ); }); }); + +/** + * Regression tests for #4208: `commit --files` could not record a file move. + * + * The #2014 guard above skips a missing `--files` entry, so the only form that + * recorded a move was a DIRECTORY entry — which also committed any unrelated + * file sitting in that directory (a concurrent session's in-flight todo, in + * the execute-phase sweep). `--files-removed` is the caller-declared deletion + * intent that lets a move be recorded at file granularity, with the #2014 + * skip-if-missing contract on `--files` left untouched. + */ +describe('commit --files-removed: caller-declared deletions record a move (#4208)', () => { + let tmpDir; + const PENDING = path.join('.planning', 'todos', 'pending'); + const COMPLETED = path.join('.planning', 'todos', 'completed'); + + function nameStatus() { + // `--no-renames`: a clean move would otherwise collapse to one `R100` row + // and hide whether the old path's deletion was actually recorded. + return gitOrThrow(['diff', '--no-renames', 'HEAD~1', 'HEAD', '--name-status'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }) + .trim().split('\n').filter(Boolean).sort(); + } + + function status() { + // `-uall`: once the move empties pending/ of tracked files, plain + // `--porcelain` collapses its untracked contents to the bare directory. + return gitOrThrow(['status', '--porcelain', '-uall'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); + } + + beforeEach(() => { + tmpDir = createTempGitProject(); + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, COMPLETED), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'mine.md'), '---\nresolves_phase: 5\n---\nmine\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n'); + gitOrThrow(['add', '.planning/'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed todo'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + // The move this phase performs, plus a peer's unrelated in-flight todo. + fs.renameSync(path.join(tmpDir, PENDING, 'mine.md'), path.join(tmpDir, COMPLETED, 'mine.md')); + fs.writeFileSync(path.join(tmpDir, PENDING, 'peer-inflight.md'), '---\nresolves_phase: 99\n---\npeer\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n\nphase 5 closed\n'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('records the move at file granularity and leaves the peer file alone', () => { + const result = runGsdTools( + ['commit', 'docs(phase-5): close 1 resolved todo(s)', + '--files', '.planning/todos/completed/mine.md', '.planning/STATE.md', + '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, true, 'move commit must succeed: ' + result.output); + + assert.deepStrictEqual( + nameStatus(), + ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md', 'M\t.planning/STATE.md'], + 'commit must contain exactly the move and STATE.md', + ); + // The peer's file is untouched: still untracked, never committed — and + // nothing about the moved todo is left dangling. + const st = status(); + assert.ok(st.includes('?? .planning/todos/pending/peer-inflight.md'), 'peer file must stay untracked: ' + st); + assert.ok(!st.includes('mine.md'), 'no dangling state for the moved todo: ' + st); + // No dual-tracking: the todo is tracked at the new path only. + const tracked = gitOrThrow(['ls-files', '--', '.planning/todos'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); + assert.strictEqual(tracked, '.planning/todos/completed/mine.md'); + }); + + test('a directory entry stages only the tracked files that are absent from disk', () => { + // A second tracked todo that stays put must NOT be touched by the + // directory form, and the untracked peer file must stay invisible to it. + fs.writeFileSync(path.join(tmpDir, PENDING, 'stays.md'), 'stays\n'); + gitOrThrow(['add', path.join(PENDING, 'stays.md')], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed a todo that stays'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + fs.appendFileSync(path.join(tmpDir, PENDING, 'stays.md'), 'edited by a peer, uncommitted\n'); + + const result = runGsdTools( + ['commit', 'docs(phase-5): close 1 resolved todo(s)', + '--files', '.planning/todos/completed/mine.md', + '--files-removed', '.planning/todos/pending/'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual( + nameStatus(), + ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md'], + ); + const st = status(); + assert.ok(st.includes(' M .planning/todos/pending/stays.md'), 'present tracked file must stay uncommitted: ' + st); + assert.ok(st.includes('?? .planning/todos/pending/peer-inflight.md'), 'untracked peer file must stay untracked: ' + st); + }); + + test('a --files-removed file entry that is still on disk fails closed and rolls back', () => { + // The declaration is wrong: pending/mine.md was put back. + fs.copyFileSync(path.join(tmpDir, COMPLETED, 'mine.md'), path.join(tmpDir, PENDING, 'mine.md')); + const head = gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); + + const result = runGsdTools( + ['commit', 'docs(phase-5): bad declaration', + '--files', '.planning/todos/completed/mine.md', + '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, false); + assert.strictEqual(parsed.reason, 'staging_failed'); + assert.strictEqual(parsed.file, '.planning/todos/pending/mine.md'); + assert.strictEqual( + gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(), + head, + 'nothing may be committed on a refused declaration', + ); + // Rollback: the addition this call staged is unstaged again. + assert.strictEqual( + gitOrThrow(['diff', '--cached', '--name-only'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(), + '', + ); + }); + + test('a --files-removed path git never tracked is a no-op, not an error', () => { + const result = runGsdTools( + ['commit', 'docs(phase-5): close 1 resolved todo(s)', + '--files', '.planning/todos/completed/mine.md', + '--files-removed', '.planning/todos/pending/mine.md', '.planning/todos/pending/never-tracked.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual( + nameStatus(), + ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md'], + ); + }); + + test('--files-removed alone is a declared scope, not the unscoped .planning/ sweep', () => { + const result = runGsdTools( + ['commit', 'docs: drop a todo', '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + // Only the deletion — not completed/mine.md, STATE.md, or the peer file. + assert.deepStrictEqual(nameStatus(), ['D\t.planning/todos/pending/mine.md']); + }); + + test('--files keeps its #2014 skip-if-missing contract when --files-removed is also given', () => { + // A tracked file that is temporarily absent (NOT moved) named via --files + // must still be skipped, even though the same call declares a removal. + fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md')); + const result = runGsdTools( + ['commit', 'docs(phase-5): close 1 resolved todo(s)', + '--files', '.planning/todos/completed/mine.md', '.planning/STATE.md', + '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual( + nameStatus(), + ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md'], + 'the temporarily-absent STATE.md must not be committed as a deletion', + ); + }); + + test('a tracked symlink whose target is gone is present, not a removal', () => { + // `stat`/`existsSync` follow the link and read it as absent; `lstat` does + // not. Declaring it removed while it still sits in the worktree must fail + // closed like any other present entry, with nothing left staged. + const link = path.join(PENDING, 'dangling'); + fs.symlinkSync('target-that-will-vanish.md', path.join(tmpDir, link)); + gitOrThrow(['add', link], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed a symlink'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + const head = gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); + + const result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files', '.planning/todos/completed/mine.md', + '--files-removed', '.planning/todos/pending/dangling'], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, false, result.output); + assert.strictEqual(parsed.reason, 'staging_failed'); + assert.strictEqual(parsed.file, '.planning/todos/pending/dangling'); + assert.strictEqual(gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(), head); + assert.strictEqual(gitOrThrow(['diff', '--cached', '--name-only'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(), ''); + }); + + test('a deletion the caller already staged is committed, not reported as nothing to commit', () => { + // `git rm` before the call empties the index entry; `ls-files` alone would + // never list it, so the path would miss the pathspec. + gitOrThrow(['rm', '-q', '--cached', path.join(PENDING, 'mine.md')], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + const result = runGsdTools( + ['commit', 'docs: drop a todo', '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual(nameStatus(), ['D\t.planning/todos/pending/mine.md']); + }); + + test('a deletion staged by this call is rolled back when a later entry fails', () => { + // Ordering: the good removal is processed first, then the contradicted one. + fs.copyFileSync(path.join(tmpDir, COMPLETED, 'mine.md'), path.join(tmpDir, PENDING, 'stays-put.md')); + gitOrThrow(['add', path.join(PENDING, 'stays-put.md')], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed a second todo'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + const head = gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); + + const result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files-removed', '.planning/todos/pending/mine.md', '.planning/todos/pending/stays-put.md'], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.strictEqual(parsed.file, '.planning/todos/pending/stays-put.md'); + assert.strictEqual(gitOrThrow(['rev-parse', 'HEAD'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(), head); + // The staged deletion of mine.md was restored to the index by the rollback. + assert.strictEqual(gitOrThrow(['diff', '--cached', '--name-only'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(), ''); + assert.ok( + gitOrThrow(['ls-files', '--', PENDING], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).includes('mine.md'), + 'mine.md must be back in the index after rollback', + ); + }); + + test('a caller-pre-staged deletion with a non-ASCII name survives the rollback', () => { + // `diff --cached --name-only` without -z quotes `café.md` as + // `"caf\303\251.md"`, which never matched the raw path, so the rollback + // treated the caller's own staged deletion as this call's and undid it. + const cafe = path.join(PENDING, 'café.md'); + fs.writeFileSync(path.join(tmpDir, cafe), 'accent\n'); + gitOrThrow(['add', cafe], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed café'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['rm', '-q', cafe], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); // caller-staged deletion + fs.copyFileSync(path.join(tmpDir, COMPLETED, 'mine.md'), path.join(tmpDir, PENDING, 'mine.md')); // contradiction + + const result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files-removed', '.planning/todos/pending/café.md', '.planning/todos/pending/mine.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).reason, 'staging_failed', result.output); + const cached = gitOrThrow(['diff', '--cached', '--name-only', '-z'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).split('\0').filter(Boolean); + assert.deepStrictEqual(cached, ['.planning/todos/pending/café.md'], 'the caller\'s own staged deletion must survive'); + }); + + test('on an unborn HEAD an absent index-only path is unstaged, never a pathspec entry', (t) => { + // A root commit has no parent to delete from; naming the path would make + // `git commit` refuse with "pathspec did not match". + const fresh = createTempGitProject(); + t.after(() => cleanup(fresh)); + // The fixture may seed commits; make an unborn branch explicitly. + gitOrThrow(['checkout', '-q', '--orphan', 'unborn'], { cwd: fresh, timeoutMs: GIT_TIMEOUT_MS }); + gitOrThrow(['rm', '-rfq', '--cached', '.'], { cwd: fresh, timeoutMs: GIT_TIMEOUT_MS }); + fs.mkdirSync(path.join(fresh, PENDING), { recursive: true }); + fs.writeFileSync(path.join(fresh, PENDING, 'a.md'), 'a\n'); + fs.writeFileSync(path.join(fresh, PENDING, 'gone.md'), 'gone\n'); + gitOrThrow(['add', PENDING], { cwd: fresh, timeoutMs: GIT_TIMEOUT_MS }); + fs.unlinkSync(path.join(fresh, PENDING, 'gone.md')); + + const result = runGsdTools( + ['commit', 'docs: root commit', + '--files', '.planning/todos/pending/a.md', + '--files-removed', '.planning/todos/pending/gone.md'], + fresh, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + const tree = gitOrThrow(['ls-tree', '-r', '--name-only', 'HEAD'], { cwd: fresh, timeoutMs: GIT_TIMEOUT_MS }).trim().split('\n'); + assert.ok(tree.includes('.planning/todos/pending/a.md'), tree.join(',')); + assert.ok(!tree.includes('.planning/todos/pending/gone.md'), tree.join(',')); + assert.strictEqual(gitOrThrow(['diff', '--cached', '--name-only'], { cwd: fresh, timeoutMs: GIT_TIMEOUT_MS }).trim(), ''); + }); + + test('a boolean flag inside a list does not end it: the positional after it stays in that list', () => { + // `--files a --no-verify b --files-removed c`: before #4208 the single + // slice-to-end list swept `b` into --files; a list that stops at ANY + // `--` token silently drops it instead (review of #4253). A list runs to + // the next LIST flag and skips boolean flags on the way. + const result = runGsdTools( + ['commit', 'docs(phase-5): close 1 resolved todo(s)', + '--files', '.planning/todos/completed/mine.md', '--no-verify', '.planning/STATE.md', + '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual( + nameStatus(), + ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md', 'M\t.planning/STATE.md'], + 'STATE.md, wedged between --no-verify and --files-removed, must still be in the --files list', + ); + }); + + test('a repeated list flag merges its runs, as the old parser did', () => { + // `--files a --files b`: the pre-#4208 slice-to-end parse yielded [a, b]; + // a parser that stops at the next list flag — including a repeat of the + // same one — silently dropped b (found by the round's comment audit). + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n'); + const result = runGsdTools( + ['commit', 'docs(phase-5): close 1 resolved todo(s)', + '--files', '.planning/todos/completed/mine.md', '--files', '.planning/ROADMAP.md', + '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual( + nameStatus(), + ['A\t.planning/ROADMAP.md', 'A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md'], + 'both --files runs must reach the commit', + ); + }); + + test('--files-removed before --files parses both lists and the message', () => { + const result = runGsdTools( + ['commit', 'docs(phase-5): close 1 resolved todo(s)', + '--files-removed', '.planning/todos/pending/mine.md', + '--files', '.planning/todos/completed/mine.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual( + nameStatus(), + ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md'], + ); + const subject = gitOrThrow(['log', '-1', '--format=%s'], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }).trim(); + assert.strictEqual(subject, 'docs(phase-5): close 1 resolved todo(s)'); + }); +}); + +/** + * #4253 review: absence from the worktree is not removal. Some index entries + * are absent BY DESIGN — a submodule gitlink whose directory was deleted by + * hand, a skip-worktree path a sparse checkout never materialised, an + * assume-unchanged path — and `lstat` cannot tell them from a moved-away file. + * Under a directory entry they are left alone, exactly like a present file; + * named directly they contradict the declaration and fail closed. And a + * staging failure restores every index entry this call removed EXACTLY, + * including on an unborn HEAD, where `git reset` has nothing to restore from. + */ +describe('commit --files-removed: index states absent by design are never removals (#4208 review)', () => { + let tmpDir; + let stray; + const PENDING = path.join('.planning', 'todos', 'pending'); + const COMPLETED = path.join('.planning', 'todos', 'completed'); + + function git(args, cwd = tmpDir) { + return gitOrThrow(args, { cwd, timeoutMs: GIT_TIMEOUT_MS }).trim(); + } + function nameStatus() { + return git(['diff', '--no-renames', 'HEAD~1', 'HEAD', '--name-status']).split('\n').filter(Boolean).sort(); + } + // Untrimmed: porcelain's leading column is significant (` D` = unstaged deletion). + function porcelain(...pathspec) { + return gitOrThrow(['status', '--porcelain', '--', ...pathspec], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + } + // Seed the standard move: pending/mine.md -> completed/mine.md, committed at pending/. + function seedMove() { + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, COMPLETED), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'mine.md'), 'mine\n'); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed todo']); + fs.renameSync(path.join(tmpDir, PENDING, 'mine.md'), path.join(tmpDir, COMPLETED, 'mine.md')); + } + // A submodule at pending/sub whose directory is then deleted by hand — the + // one gitlink shape that reads as absent (an uninitialised submodule leaves + // an empty directory behind, which lstat sees as present). + function addSubmoduleThenDeleteDir() { + const subSrc = path.join(tmpDir, '..', path.basename(tmpDir) + '-sub'); + stray = subSrc; + fs.mkdirSync(subSrc, { recursive: true }); + git(['init', '-q', '.'], subSrc); + git(['config', 'user.email', 't@t'], subSrc); + git(['config', 'user.name', 't'], subSrc); + fs.writeFileSync(path.join(subSrc, 'f.txt'), 'v1\n'); + git(['add', 'f.txt'], subSrc); + git(['commit', '-q', '-m', 'v1'], subSrc); + git(['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', subSrc, '.planning/todos/pending/sub']); + git(['commit', '-q', '-m', 'add submodule']); + cleanup(path.join(tmpDir, PENDING, 'sub')); + assert.match(porcelain(PENDING), /^ D \.planning\/todos\/pending\/sub$/m, 'git itself reads the gitlink as deleted'); + } + + beforeEach(() => { tmpDir = createTempGitProject(); stray = null; }); + afterEach(() => { cleanup(tmpDir); if (stray) cleanup(stray); }); + + test('a directory entry leaves a hand-deleted submodule gitlink in the index and records only the file move', () => { + seedMove(); + addSubmoduleThenDeleteDir(); + const result = runGsdTools( + ['commit', 'docs: close a todo', '--files', '.planning/todos/completed/mine.md', '--files-removed', '.planning/todos/pending/'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.deepStrictEqual(nameStatus(), ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md']); + // The gitlink is still tracked, at mode 160000, and git still reports the + // hand-deletion as the caller's unstaged business — not this call's. + assert.match(git(['ls-files', '-s', '--', PENDING]), /^160000 [0-9a-f]+ 0\t\.planning\/todos\/pending\/sub$/m); + assert.match(porcelain(PENDING), /^ D \.planning\/todos\/pending\/sub$/m); + }); + + test('a submodule gitlink named directly under --files-removed fails closed, naming the state', () => { + seedMove(); + addSubmoduleThenDeleteDir(); + const head = git(['rev-parse', 'HEAD']); + const result = runGsdTools( + ['commit', 'docs: bad declaration', '--files', '.planning/todos/completed/mine.md', '--files-removed', '.planning/todos/pending/sub'], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, false, result.output); + assert.strictEqual(parsed.reason, 'staging_failed'); + assert.strictEqual(parsed.file, '.planning/todos/pending/sub'); + assert.match(parsed.error, /submodule gitlink/); + assert.strictEqual(git(['rev-parse', 'HEAD']), head); + assert.strictEqual(git(['diff', '--cached', '--name-only']), '', 'the addition this call staged is rolled back'); + assert.match(git(['ls-files', '-s', '--', PENDING]), /^160000 /m, 'the gitlink is untouched'); + }); + + test('a skip-worktree path is absent by checkout, not removed: skipped under a directory entry, refused when named', () => { + seedMove(); + // A second tracked todo that a sparse checkout would not materialise. + fs.writeFileSync(path.join(tmpDir, PENDING, 'sparse.md'), 'sparse\n'); + git(['add', path.join(PENDING, 'sparse.md')]); + git(['commit', '-q', '-m', 'seed sparse']); + git(['update-index', '--skip-worktree', '--', '.planning/todos/pending/sparse.md']); + fs.unlinkSync(path.join(tmpDir, PENDING, 'sparse.md')); + assert.strictEqual(porcelain(path.join(PENDING, 'sparse.md')), '', 'git itself does not report a skip-worktree path as deleted'); + + const dirForm = runGsdTools( + ['commit', 'docs: close a todo', '--files', '.planning/todos/completed/mine.md', '--files-removed', '.planning/todos/pending/'], + tmpDir, + ); + assert.strictEqual(JSON.parse(dirForm.output).committed, true, dirForm.output); + assert.deepStrictEqual(nameStatus(), ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md']); + assert.match(git(['ls-files', '-v', '--', PENDING]), /^S \.planning\/todos\/pending\/sparse\.md$/m, 'the sparse entry stays in the index, still skip-worktree'); + + const head = git(['rev-parse', 'HEAD']); + const named = runGsdTools(['commit', 'docs: bad declaration', '--files-removed', '.planning/todos/pending/sparse.md'], tmpDir); + const parsed = JSON.parse(named.output); + assert.strictEqual(parsed.reason, 'staging_failed', named.output); + assert.strictEqual(parsed.file, '.planning/todos/pending/sparse.md'); + assert.match(parsed.error, /skip-worktree/); + assert.strictEqual(git(['rev-parse', 'HEAD']), head); + assert.match(git(['ls-files', '-v', '--', PENDING]), /^S \.planning\/todos\/pending\/sparse\.md$/m); + }); + + test('a directly named path is recognised by any spelling that resolves to it (absolute path)', () => { + // The direct-vs-directory decision is made on resolved paths. A string + // compare against git's cwd-relative output silently took the directory + // polarity for an absolute path, so a named gitlink SKIPPED instead of + // refusing (found by review, driven). + seedMove(); + addSubmoduleThenDeleteDir(); + const head = git(['rev-parse', 'HEAD']); + const result = runGsdTools( + ['commit', 'docs: bad declaration', '--files', '.planning/todos/completed/mine.md', '--files-removed', path.join(tmpDir, PENDING, 'sub')], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.match(parsed.error, /submodule gitlink/); + assert.strictEqual(git(['rev-parse', 'HEAD']), head); + assert.match(git(['ls-files', '-s', '--', PENDING]), /^160000 /m, 'the gitlink is untouched'); + + // And through a SYMLINKED spelling of the same directory — the macOS + // `/var` → `/private/var` shape, where `process.cwd()` is the real path + // and the caller's absolute path is not (CI, first push of this round). + const alias = tmpDir + '-alias'; + fs.symlinkSync(tmpDir, alias, 'dir'); + try { + const viaLink = runGsdTools( + ['commit', 'docs: bad declaration', '--files', '.planning/todos/completed/mine.md', '--files-removed', path.join(alias, PENDING, 'sub')], + tmpDir, + ); + const p2 = JSON.parse(viaLink.output); + assert.strictEqual(p2.reason, 'staging_failed', viaLink.output); + assert.match(p2.error, /submodule gitlink/); + assert.strictEqual(git(['rev-parse', 'HEAD']), head); + } finally { + fs.unlinkSync(alias); + } + }); + + test('an intent-to-add entry is not tracked content: skipped under a directory entry, refused when named', () => { + // `git add -N` renders as a plain `H 100644 ` entry, yet there + // is nothing committed to remove and a cacheinfo rollback cannot restore + // the flag (found by review, driven). + seedMove(); + fs.writeFileSync(path.join(tmpDir, PENDING, 'planned.md'), 'planned\n'); + git(['add', '-N', path.join(PENDING, 'planned.md')]); + fs.unlinkSync(path.join(tmpDir, PENDING, 'planned.md')); + const before = git(['ls-files', '-s', '--', path.join(PENDING, 'planned.md')]); + assert.match(before, /^100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0/, 'fixture: intent-to-add entry present'); + + const dirForm = runGsdTools( + ['commit', 'docs: close a todo', '--files', '.planning/todos/completed/mine.md', '--files-removed', '.planning/todos/pending/'], + tmpDir, + ); + assert.strictEqual(JSON.parse(dirForm.output).committed, true, dirForm.output); + assert.deepStrictEqual(nameStatus(), ['A\t.planning/todos/completed/mine.md', 'D\t.planning/todos/pending/mine.md']); + assert.strictEqual(git(['ls-files', '-s', '--', path.join(PENDING, 'planned.md')]), before, 'the intent-to-add entry is left alone'); + + const named = runGsdTools(['commit', 'docs: bad declaration', '--files-removed', '.planning/todos/pending/planned.md'], tmpDir); + const parsed = JSON.parse(named.output); + assert.strictEqual(parsed.reason, 'staging_failed', named.output); + assert.match(parsed.error, /intent-to-add/); + assert.strictEqual(git(['ls-files', '-s', '--', path.join(PENDING, 'planned.md')]), before); + }); + + test('an assume-unchanged path named directly fails closed and stays in the index', () => { + seedMove(); + git(['update-index', '--assume-unchanged', '--', '.planning/todos/pending/mine.md']); + const result = runGsdTools(['commit', 'docs: drop a todo', '--files-removed', '.planning/todos/pending/mine.md'], tmpDir); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.match(parsed.error, /assume-unchanged/); + assert.match(git(['ls-files', '-v', '--', PENDING]), /^h \.planning\/todos\/pending\/mine\.md$/m); + }); + + test('on an unborn HEAD a removal staged by this call is restored when a later entry fails', (t) => { + // The rollback cannot `git reset` to a HEAD that does not exist; the + // entry is put back from the record this call kept of it. + const fresh = createTempGitProject(); + t.after(() => cleanup(fresh)); + git(['checkout', '-q', '--orphan', 'unborn'], fresh); + git(['rm', '-rfq', '--cached', '.'], fresh); + fs.mkdirSync(path.join(fresh, PENDING), { recursive: true }); + fs.writeFileSync(path.join(fresh, PENDING, 'gone.md'), 'gone\n'); + fs.writeFileSync(path.join(fresh, PENDING, 'stays.md'), 'stays\n'); + git(['add', PENDING], fresh); + const before = git(['ls-files', '-s', '--', PENDING], fresh); + fs.unlinkSync(path.join(fresh, PENDING, 'gone.md')); + + const result = runGsdTools( + ['commit', 'docs: root commit', '--files-removed', '.planning/todos/pending/gone.md', '.planning/todos/pending/stays.md'], + fresh, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.strictEqual(parsed.file, '.planning/todos/pending/stays.md'); + assert.throws(() => git(['rev-parse', '-q', '--verify', 'HEAD'], fresh), 'nothing may be committed'); + assert.strictEqual(git(['ls-files', '-s', '--', PENDING], fresh), before, 'gone.md is back in the index, same mode and blob'); + }); + + test('on an unborn HEAD a removal-only call that stages nothing else leaves no removal behind', (t) => { + // The rollback above fires only on a staging FAILURE. On an unborn HEAD a + // removal never joins `stagedPaths` (there is no parent to delete from), so + // a removal-only call that SUCCEEDS reaches the nothing-to-commit guard with + // an empty pathspec -- and `nothing_to_commit` tells the caller no state + // changed while `rm --cached` has already mutated the index. The removal + // would then ride along on the caller's next commit. + const fresh = createTempGitProject(); + t.after(() => cleanup(fresh)); + git(['checkout', '-q', '--orphan', 'unborn'], fresh); + git(['rm', '-rfq', '--cached', '.'], fresh); + fs.mkdirSync(path.join(fresh, PENDING), { recursive: true }); + fs.writeFileSync(path.join(fresh, PENDING, 'gone.md'), 'gone\n'); + git(['add', PENDING], fresh); + const before = git(['ls-files', '-s', '--', PENDING], fresh); + fs.unlinkSync(path.join(fresh, PENDING, 'gone.md')); + + const result = runGsdTools( + ['commit', 'docs: root commit', '--files-removed', '.planning/todos/pending/gone.md'], + fresh, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, false, result.output); + assert.throws(() => git(['rev-parse', '-q', '--verify', 'HEAD'], fresh), 'nothing may be committed'); + assert.strictEqual( + git(['ls-files', '-s', '--', PENDING], fresh), before, + 'a call reporting no commit must leave the index as it found it', + ); + }); + + test('a removal of an index-only path leaves no removal behind when nothing is recorded', () => { + // The same defect with a real HEAD, so the fix cannot key on `headExists`. + // gone.md was `git add`ed and never committed, then deleted from disk: the + // removal DOES join the pathspec here, but `diff HEAD -- gone.md` reads + // clean because the path is absent from the worktree and from HEAD alike, + // so the guard reports nothing_to_commit over a staged removal. + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'seed.md'), 'seed\n'); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed todo']); + fs.writeFileSync(path.join(tmpDir, PENDING, 'gone.md'), 'gone\n'); + git(['add', path.join(PENDING, 'gone.md')]); + const before = git(['ls-files', '-s', '--', PENDING]); + const head = git(['rev-parse', 'HEAD']); + fs.unlinkSync(path.join(tmpDir, PENDING, 'gone.md')); + + const result = runGsdTools( + ['commit', 'docs: remove an uncommitted path', '--files-removed', '.planning/todos/pending/gone.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, false, result.output); + assert.strictEqual(git(['rev-parse', 'HEAD']), head, 'nothing may be committed'); + assert.strictEqual( + git(['ls-files', '-s', '--', PENDING]), before, + 'a call reporting no commit must leave the index as it found it', + ); + }); + + test('a removal the call cannot put back is reported, never as nothing_to_commit', + { skip: process.platform === 'win32' ? 'chmod cannot make a directory unwritable on Windows (driven: a write into a ReadOnly directory succeeds), so the fixture cannot drive a failed restore' : false }, + (t) => { + // The restore is best-effort, so it can FAIL -- and reporting + // nothing_to_commit over a removal we tried and could not undo is the same + // false "no state changed" the restore exists to prevent, one level down. + // Driven with a post-index-change hook that makes the git dir unwritable + // the moment `rm --cached` lands, so the `update-index --cacheinfo` restore + // cannot take its lock. + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'seed.md'), 'seed\n'); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed todo']); + fs.writeFileSync(path.join(tmpDir, PENDING, 'gone.md'), 'gone\n'); + git(['add', path.join(PENDING, 'gone.md')]); + fs.unlinkSync(path.join(tmpDir, PENDING, 'gone.md')); + const gitDir = path.join(tmpDir, '.git'); + const hooksDir = path.join(gitDir, 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync(path.join(hooksDir, 'post-index-change'), + '#!/bin/sh\nchmod a-w "$(git rev-parse --git-dir)"\n', { mode: 0o755 }); + // Give the dir back in a FINALLY below, not only in `t.after`: t.after runs + // AFTER the parent afterEach, so a throw between the hook and the explicit + // chmod leaves afterEach unable to delete the fixture. t.after stays as a + // belt for the case where the finally itself is skipped. + t.after(() => { try { fs.chmodSync(gitDir, 0o755); } catch { /* already writable */ } }); + const emptyConfig = path.join(tmpDir, 'empty.gitconfig'); + fs.writeFileSync(emptyConfig, ''); + + let result; + try { + result = runGsdTools( + ['commit', 'docs: remove an uncommitted path', '--files-removed', '.planning/todos/pending/gone.md'], + tmpDir, + { GIT_CONFIG_GLOBAL: emptyConfig, GIT_CONFIG_NOSYSTEM: '1' }, + ); + } finally { + fs.chmodSync(gitDir, 0o755); + } + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, false, result.output); + assert.notStrictEqual(parsed.reason, 'nothing_to_commit', 'a removal left staged must never be reported as no state change'); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.match(parsed.error, /could not be restored/); + assert.match(parsed.error, /gone\.md/); + }); + + test('a rollback that cannot restore a removal discloses it, even when the reported failure is another entry', + { skip: process.platform === 'win32' ? 'chmod cannot make a directory unwritable on Windows (driven: a write into a ReadOnly directory succeeds), so the fixture cannot drive a failed restore' : false }, + (t) => { + // The rollback exit reports the failure that CAUSED it -- here a + // contradictory declaration about a path still on disk -- so a caller + // reading `failures` would learn nothing about the removal this call had + // already staged and then could not put back. Both must be disclosed. + seedMove(); + fs.writeFileSync(path.join(tmpDir, PENDING, 'stays.md'), 'stays\n'); + git(['add', path.join(PENDING, 'stays.md')]); + git(['commit', '-q', '-m', 'seed a present todo']); + const gitDir = path.join(tmpDir, '.git'); + const hooksDir = path.join(gitDir, 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync(path.join(hooksDir, 'post-index-change'), + '#!/bin/sh\nchmod a-w "$(git rev-parse --git-dir)"\n', { mode: 0o755 }); + t.after(() => { try { fs.chmodSync(gitDir, 0o755); } catch { /* already writable */ } }); + const emptyConfig = path.join(tmpDir, 'empty.gitconfig'); + fs.writeFileSync(emptyConfig, ''); + + let result; + try { + // mine.md was moved away (a real removal); stays.md is still on disk, so + // declaring it removed contradicts the declaration and fails the call. + result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files-removed', '.planning/todos/pending/mine.md', '.planning/todos/pending/stays.md'], + tmpDir, + { GIT_CONFIG_GLOBAL: emptyConfig, GIT_CONFIG_NOSYSTEM: '1' }, + ); + } finally { + fs.chmodSync(gitDir, 0o755); + } + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.strictEqual(parsed.file, '.planning/todos/pending/stays.md', 'the REPORTED failure is still the contradictory declaration'); + const disclosed = parsed.failures.filter(f => /could NOT be restored/.test(f.error)); + assert.ok( + disclosed.length > 0, + `a removal left staged by a failed rollback must be disclosed; failures were ${JSON.stringify(parsed.failures)}`, + ); + assert.ok( + disclosed.some(f => f.file === '.planning/todos/pending/mine.md'), + `the disclosure must name the un-restored path; got ${JSON.stringify(disclosed)}`, + ); + }); + + test('a removal whose own rm failed is not disclosed as still staged', () => { + // The mirror of the disclosure above. `removedEntries` is the set this call + // claims to have STAGED, so an entry recorded before a `rm --cached` that + // then FAILED would be reported as "still staged in the index" when nothing + // was staged at all. Driven with a pre-existing index.lock, which fails the + // rm and the restore alike. + seedMove(); + fs.writeFileSync(path.join(tmpDir, PENDING, 'stays.md'), 'stays\n'); + git(['add', path.join(PENDING, 'stays.md')]); + git(['commit', '-q', '-m', 'seed a present todo']); + const before = git(['ls-files', '-s', '--', PENDING]); + fs.writeFileSync(path.join(tmpDir, '.git', 'index.lock'), ''); + + const result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files-removed', '.planning/todos/pending/mine.md', '.planning/todos/pending/stays.md'], + tmpDir, + ); + fs.unlinkSync(path.join(tmpDir, '.git', 'index.lock')); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.strictEqual( + parsed.failures.filter(f => /could NOT be restored/.test(f.error)).length, 0, + `nothing was staged, so nothing may be disclosed as left staged; failures were ${JSON.stringify(parsed.failures)}`, + ); + assert.strictEqual(git(['ls-files', '-s', '--', PENDING]), before, 'the index is untouched'); + }); + + test('a timed-out removal does not produce a false could-not-restore disclosure', () => { + // The exit code answers "did the command succeed", never "did the index + // change": execGit collapses a spawn timeout to a non-zero exit, and a + // killed git can already have written the index. This pins the RESTORE + // side of that -- a restore whose update-index was killed after its write + // landed must not report "could NOT be restored" over an index it did in + // fact restore. Forcing the restore verdict back onto the exit code fails + // this test. + // + // NAMED RESIDUAL: the RECORD side of the same rule -- a timed-out `rm` + // whose write DID land must still be recorded and undone -- is NOT pinned + // here. Whether that write survives the in-process kill is not + // deterministic (driven: it lands under a shell `timeout`, and did not + // under execGit's spawnSync bound), so an assertion on it would read as + // coverage and never run. It is driven by hand instead. + seedMove(); + const before = git(['ls-files', '-s', '--', PENDING]); + const hooksDir = path.join(tmpDir, '.git', 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync(path.join(hooksDir, 'post-index-change'), '#!/bin/sh\nsleep 12\n', { mode: 0o755 }); + const emptyConfig = path.join(tmpDir, 'empty.gitconfig'); + fs.writeFileSync(emptyConfig, ''); + + const result = runGsdTools( + ['commit', 'docs: close a todo', '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + { GIT_CONFIG_GLOBAL: emptyConfig, GIT_CONFIG_NOSYSTEM: '1' }, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, false, result.output); + // Consistency only, NOT the control: whichever way the killed write went, + // the call must leave the index coherent. See the residual note above. + assert.strictEqual(git(['ls-files', '-s', '--', PENDING]), before, 'the index must be coherent after a timed-out removal'); + // THE CONTROL: the restore succeeded, so nothing may claim otherwise. + assert.strictEqual( + (parsed.failures || []).filter(f => /could NOT be restored/.test(f.error)).length, 0, + `the index was restored, so no disclosure may fire; failures were ${JSON.stringify(parsed.failures)}`, + ); + }); + + test('a restored non-ASCII path is recognised as restored, not reported as a failure', () => { + // The restore verification reads the index back, so it must read it with + // `-z`: core.quotePath renders café.md as "caf\\303\\251.md", which never + // equals the raw path, and an EXACTLY restored entry then read as not + // restored -- the same quoting defect this PR already fixed for preStaged. + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'seed.md'), 'seed\n'); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed todo']); + // Index-only and absent from disk: the call stages the removal, records + // nothing, and must restore -- the path the verification runs on. + fs.writeFileSync(path.join(tmpDir, PENDING, 'café.md'), 'cafe\n'); + git(['add', path.join(PENDING, 'café.md')]); + const before = gitOrThrow(['ls-files', '-s', '-z', '--', PENDING], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }); + fs.unlinkSync(path.join(tmpDir, PENDING, 'café.md')); + + const result = runGsdTools( + ['commit', 'docs: remove an uncommitted path', '--files-removed', '.planning/todos/pending/café.md'], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'nothing_to_commit', result.output); + assert.strictEqual( + gitOrThrow(['ls-files', '-s', '-z', '--', PENDING], { cwd: tmpDir, timeoutMs: GIT_TIMEOUT_MS }), before, + 'the entry is restored exactly', + ); + }); + + test('a path restored at a different mode is not accepted as restored', () => { + // --cacheinfo restores mode, blob and stage, so a path-only membership test + // would accept an entry that came back as something else. Driven with a + // post-index-change hook that rewrites the restored entry's mode. + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'seed.md'), 'seed\n'); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed todo']); + fs.writeFileSync(path.join(tmpDir, PENDING, 'gone.md'), 'gone\n'); + git(['add', path.join(PENDING, 'gone.md')]); + // git's `:` index syntax takes a FORWARD-slash path; `path.join` + // yields backslashes on Windows and git rejects them as an ambiguous + // argument. The hook below already uses the slash form for the same reason. + const GONE = '.planning/todos/pending/gone.md'; + const blob = git(['rev-parse', ':' + GONE]); + fs.unlinkSync(path.join(tmpDir, PENDING, 'gone.md')); + const hooksDir = path.join(tmpDir, '.git', 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + // Fires after the restore's index write; flips the mode so the entry that + // comes back is not the entry that was removed. + fs.writeFileSync(path.join(hooksDir, 'post-index-change'), + '#!/bin/sh\n' + + 'git ls-files -s -- .planning/todos/pending/gone.md | grep -q "^100644" ' + + '&& git update-index --add --cacheinfo 100755,' + blob + ',.planning/todos/pending/gone.md\n', + { mode: 0o755 }); + const emptyConfig = path.join(tmpDir, 'empty.gitconfig'); + fs.writeFileSync(emptyConfig, ''); + + const result = runGsdTools( + ['commit', 'docs: remove an uncommitted path', '--files-removed', '.planning/todos/pending/gone.md'], + tmpDir, + { GIT_CONFIG_GLOBAL: emptyConfig, GIT_CONFIG_NOSYSTEM: '1' }, + ); + const parsed = JSON.parse(result.output); + assert.notStrictEqual( + parsed.reason, 'nothing_to_commit', + `the entry came back at a different mode, so the restore is not clean: ${result.output}`, + ); + }); + + test('a tracked filename containing a glob removes only itself, never its neighbours', + { skip: process.platform === 'win32' ? 'a filename containing `*` cannot exist on Windows (driven: IOException)' : false }, + () => { + // An index path handed back to git is parsed as a PATHSPEC. A tracked file + // literally named `*.md` therefore GLOBS: `rm --cached` on it also removed + // the peers, only the declared entry was recorded, and the rollback then + // restored one of three -- leaving the others staged as undisclosed + // deletions. :(literal) is what makes the operand mean the file it names. + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, '*.md'), 'wildcard\n'); + fs.writeFileSync(path.join(tmpDir, PENDING, 'peer.md'), 'peer\n'); + fs.writeFileSync(path.join(tmpDir, PENDING, 'stays.md'), 'stays\n'); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed a wildcard-named todo']); + fs.unlinkSync(path.join(tmpDir, PENDING, '*.md')); + const before = git(['ls-files', '-s', '--', PENDING]); + + // stays.md is still present, so the call fails and rolls back. Whatever the + // rollback restores, the peers must never have been touched at all. + const result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files-removed', '.planning/todos/pending/', '.planning/todos/pending/stays.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).reason, 'staging_failed', result.output); + assert.strictEqual( + git(['diff', '--cached', '--name-status']), '', + 'no unrelated deletion may be left staged by a globbing pathspec', + ); + assert.strictEqual(git(['ls-files', '-s', '--', PENDING]), before, 'the index is exactly as it was'); + }); + + test('a tracked filename containing pathspec magic is removed, and commits nothing else', + { skip: process.platform === 'win32' ? 'a filename containing `:` cannot exist on Windows (driven: FileNotFoundException)' : false }, + () => { + // The quieter half of the same defect: pathspec magic binds at the START of + // the operand, so a file named `:(literal)mine` at the repo ROOT has its + // prefix PARSED -- the rm matched nothing, exited 0, and the entry survived + // a removal this call went on to report as done. A path under a directory + // never starts with `:`, so the fixture must be top-level to reach it. + const odd = ':(literal)mine'; + fs.writeFileSync(path.join(tmpDir, odd), 'mine\n'); + git(['add', '--', ':(literal)' + odd]); + git(['commit', '-q', '-m', 'seed a magic-named file']); + assert.strictEqual(git(['ls-files', '--', ':(literal)' + odd]), odd, 'fixture: the odd name is tracked'); + fs.unlinkSync(path.join(tmpDir, odd)); + + // A peer that is MODIFIED but never declared: the commit's own pathspec is + // where an unliteralised name sweeps it in. + fs.writeFileSync(path.join(tmpDir, 'peer.md'), 'peer\n'); + git(['add', 'peer.md']); + git(['commit', '-q', '-m', 'seed a peer']); + fs.writeFileSync(path.join(tmpDir, 'peer.md'), 'peer, modified\n'); + + const result = runGsdTools( + ['commit', 'docs: close a todo', '--files-removed', odd], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, true, result.output); + assert.strictEqual( + git(['ls-files', '--', ':(literal)' + odd]), '', + 'the entry the caller named must actually be gone from the index', + ); + // The commit must contain the declared removal and NOTHING else -- an + // undeclared `M peer.md` is the sweep this flag exists to remove. + assert.strictEqual( + git(['diff', '--no-renames', 'HEAD~1', 'HEAD', '--name-status']), 'D\t' + odd, + 'only the declared removal may be committed', + ); + }); + + test('a glob-named removal commits only itself, never an undeclared peer edit', + { skip: process.platform === 'win32' ? 'a filename containing `*` cannot exist on Windows (driven: IOException)' : false }, + () => { + // Literalising the STAGING is not enough: `git commit -- ` takes the + // same paths as a pathspec, so a tracked file named `*.md` swept a MODIFIED + // peer into the commit the caller never declared -- the sweep this flag + // exists to remove, arriving one step after staging. + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, '*.md'), 'wildcard\n'); + fs.writeFileSync(path.join(tmpDir, PENDING, 'peer.md'), 'peer\n'); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed a wildcard-named todo']); + fs.unlinkSync(path.join(tmpDir, PENDING, '*.md')); + fs.writeFileSync(path.join(tmpDir, PENDING, 'peer.md'), 'peer, modified\n'); + + const result = runGsdTools( + ['commit', 'docs: close a todo', '--files-removed', '.planning/todos/pending/'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).committed, true, result.output); + assert.strictEqual( + git(['diff', '--no-renames', 'HEAD~1', 'HEAD', '--name-status']), + 'D\t' + path.join(PENDING, '*.md'), + 'only the declared removal may be committed', + ); + assert.match(porcelain(PENDING), /^ M \.planning\/todos\/pending\/peer\.md$/m, "the peer's edit stays uncommitted"); + }); + + test('an intent-to-add entry with a glob name keeps its intent flag', + { skip: process.platform === 'win32' ? 'a filename containing `*` cannot exist on Windows (driven: IOException)' : false }, + () => { + // The intent-to-add probe is a `diff --cached` over the path, so an + // unliteralised glob name matched a STAGED PEER instead of itself, the + // entry was misclassified as ordinary content, removed, and then restored + // by --cacheinfo -- which cannot restore the intent flag. It came back as a + // real staged addition. + fs.mkdirSync(path.join(tmpDir, PENDING), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'peer.md'), 'peer\n'); + git(['add', path.join(PENDING, 'peer.md')]); // a STAGED peer for the glob to find + fs.writeFileSync(path.join(tmpDir, PENDING, '*.md'), 'wildcard\n'); + git(['add', '-N', '--', ':(literal)' + path.join(PENDING, '*.md')]); + fs.unlinkSync(path.join(tmpDir, PENDING, '*.md')); + // OBSERVE THE FLAG, not the entry. `ls-files -v` renders an intent-to-add + // exactly like an ordinary cached entry, so comparing it cannot see the + // flag at all -- an earlier cut of this test did that and passed with the + // fix reverted. An intent-to-add is absent from `diff --cached`; losing the + // flag turns it into a real staged addition, which is what to assert on. + assert.doesNotMatch( + git(['diff', '--cached', '--name-status']), /^A\t.*\*\.md$/m, + 'fixture: the intent-to-add entry is not a staged addition yet', + ); + + // A directory entry: the intent-to-add path must be SKIPPED, not removed. + const result = runGsdTools( + ['commit', 'docs: close a todo', '--files-removed', '.planning/todos/pending/'], + tmpDir, + ); + assert.ok(result.output, 'the tool produced output'); + assert.doesNotMatch( + git(['diff', '--cached', '--name-status']), /^A\t.*\*\.md$/m, + 'the intent-to-add entry must keep its flag -- a --cacheinfo restore turns it into a real staged addition', + ); + assert.match( + git(['ls-files', '--', ':(literal)' + path.join(PENDING, '*.md')]), /\*\.md/, + 'and it must still be in the index at all', + ); + }); + + test("a nested project's rollback leaves the caller's own staged work alone", (t) => { + // `diff --cached` prints REPO-relative paths whatever the cwd, while + // stagedPaths holds the caller's cwd-relative names. In a project nested + // inside its repo the two name spaces never intersect, so `preStaged` + // matched NOTHING and the rollback unstaged everything -- including work + // the caller had staged themselves, which is precisely what preStaged + // exists to protect. Pre-existing: it governs the --files side too. + const repo = createTempGitProject(); + t.after(() => cleanup(repo)); + const proj = path.join(repo, 'sub'); + const pending = path.join(proj, PENDING); + fs.mkdirSync(pending, { recursive: true }); + fs.writeFileSync(path.join(pending, 'mine.md'), 'mine\n'); + fs.writeFileSync(path.join(pending, 'peer.md'), 'peer\n'); + fs.writeFileSync(path.join(pending, 'stays.md'), 'stays\n'); + const g = (args) => gitOrThrow(args, { cwd: repo, timeoutMs: GIT_TIMEOUT_MS }).trim(); + g(['add', 'sub']); + g(['commit', '-q', '-m', 'seed a nested project']); + // The caller stages their OWN work: a deletion and a modification. + fs.unlinkSync(path.join(pending, 'mine.md')); + g(['add', '-A', '--', 'sub/.planning/todos/pending/mine.md']); + fs.writeFileSync(path.join(pending, 'peer.md'), 'peer, modified\n'); + g(['add', 'sub/.planning/todos/pending/peer.md']); + const before = g(['diff', '--cached', '--name-status']); + + // stays.md is present, so the declaration is contradictory and the call + // rolls back. The rollback must not touch what the caller staged. + const result = runGsdTools( + ['commit', 'docs: bad declaration', + '--files-removed', '.planning/todos/pending/', '.planning/todos/pending/stays.md'], + proj, + ); + assert.strictEqual(JSON.parse(result.output).reason, 'staging_failed', result.output); + assert.strictEqual( + g(['diff', '--cached', '--name-status']), before, + "the caller's own staged deletion and modification must survive the rollback", + ); + }); + + + + + + + + + + + test('a file that reappears between the absence check and the rm is refused, and the rollback restores its entry', () => { + // The window this PR's own headline scenario names: a concurrent session + // recreates the path after this call judged it absent. Driven + // deterministically with a post-index-change hook, which git fires the + // moment `rm --cached` writes the index -- the hook puts the file back + // exactly then, so the re-check after the mutation must catch it. + seedMove(); + const hooksDir = path.join(tmpDir, '.git', 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync( + path.join(hooksDir, 'post-index-change'), + '#!/bin/sh\n' + + 'git ls-files --error-unmatch -- .planning/todos/pending/mine.md >/dev/null 2>&1 ' + + '|| cp .planning/todos/completed/mine.md .planning/todos/pending/mine.md\n', + { mode: 0o755 }, + ); + // Pin the hook location against a host core.hooksPath (#3901 shape). + const emptyConfig = path.join(tmpDir, 'empty.gitconfig'); + fs.writeFileSync(emptyConfig, ''); + const head = git(['rev-parse', 'HEAD']); + + const result = runGsdTools( + ['commit', 'docs: close a todo', '--files', '.planning/todos/completed/mine.md', '--files-removed', '.planning/todos/pending/mine.md'], + tmpDir, + { GIT_CONFIG_GLOBAL: emptyConfig, GIT_CONFIG_NOSYSTEM: '1' }, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.committed, false, result.output); + assert.strictEqual(parsed.reason, 'staging_failed'); + assert.strictEqual(parsed.file, '.planning/todos/pending/mine.md'); + assert.match(parsed.error, /reappeared on disk/); + assert.strictEqual(git(['rev-parse', 'HEAD']), head, 'nothing may be committed under a message that declared the path removed'); + assert.strictEqual(git(['diff', '--cached', '--name-only']), '', 'the addition is unstaged and the removed entry is back'); + assert.ok(fs.existsSync(path.join(tmpDir, PENDING, 'mine.md')), 'the hook did put the file back (the window was exercised)'); + assert.match(git(['ls-files', '--', PENDING]), /mine\.md/, 'the index entry this call removed is restored'); + }); + + test('the rollback restores a caller-pre-staged blob at a removed path exactly, not HEAD\'s version', () => { + seedMove(); + fs.writeFileSync(path.join(tmpDir, PENDING, 'present.md'), 'present\n'); + git(['add', path.join(PENDING, 'present.md')]); + git(['commit', '-q', '-m', 'seed a present todo']); + // The caller staged an edit to mine.md at its OLD path (index only, HEAD + // still holds the seed blob), then moved the file and declared the old + // path removed; a `git reset` rollback would put HEAD's blob back, + // silently discarding the staged edit. + fs.writeFileSync(path.join(tmpDir, PENDING, 'mine.md'), 'mine, edited and staged\n'); + git(['add', path.join(PENDING, 'mine.md')]); + const staged = git(['ls-files', '-s', '--', path.join(PENDING, 'mine.md')]); + assert.notEqual(staged, git(['ls-tree', 'HEAD', '--', path.join(PENDING, 'mine.md')]).replace(/\t/, ' '), 'fixture: the staged blob must differ from HEAD'); + fs.unlinkSync(path.join(tmpDir, PENDING, 'mine.md')); + + const result = runGsdTools( + ['commit', 'docs: bad declaration', '--files-removed', '.planning/todos/pending/mine.md', '.planning/todos/pending/present.md'], + tmpDir, + ); + assert.strictEqual(JSON.parse(result.output).reason, 'staging_failed', result.output); + assert.strictEqual(git(['ls-files', '-s', '--', path.join(PENDING, 'mine.md')]), staged, 'the pre-staged blob survives the rollback'); + }); + + test('a symlink to a directory is one tracked path, not a directory entry', () => { + // Review of #4253 read the `lstatSync(...).isDirectory()` test as a defect + // because it does not follow symlinks. It is deliberate, and following the + // link would be the bug: git tracks a symlink as a single blob (mode + // 120000) and does NOT traverse it, so the tracked paths "under" it live + // at the REAL directory and were never named by the caller. Treating the + // link as a directory entry would stage those -- the directory sweep + // #4208 exists to remove -- while the entry the caller DID name still sat + // present on disk, contradicting its own declaration. + fs.mkdirSync(path.join(tmpDir, PENDING, 'real'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, PENDING, 'real', 'a.md'), 'a\n'); + fs.symlinkSync('real', path.join(tmpDir, PENDING, 'link')); + git(['add', '.planning/']); + git(['commit', '-q', '-m', 'seed a symlinked dir']); + + // The premise, driven rather than asserted: one path, and git does not + // traverse it. + assert.match(git(['ls-files', '-s', '--', path.join(PENDING, 'link')]), /^120000 /, 'git tracks the symlink itself'); + assert.strictEqual(git(['ls-files', '--', path.join(PENDING, 'link') + '/']), '', 'git does not traverse the symlink'); + + const head = git(['rev-parse', 'HEAD']); + const result = runGsdTools( + ['commit', 'docs: remove a symlinked dir', '--files-removed', '.planning/todos/pending/link'], + tmpDir, + ); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.reason, 'staging_failed', result.output); + assert.match(parsed.error, /still present on disk/); + assert.strictEqual(git(['rev-parse', 'HEAD']), head, 'nothing may be committed'); + assert.match(git(['ls-files', '--', PENDING]), /real\/a\.md/, 'the path behind the link is untouched -- the caller never named it'); + }); + +}); + +// RULESET.TESTS.property-based-testing: the two-list commit parser is a real +// parser (split argv into two lists by boundary flags, skip embedded boolean +// flags, merge repeated occurrences), and its edge cases were already the +// subject of a review round. The example cases above pin the shapes that broke; +// this pins the invariant they are instances of, over interleavings nobody +// enumerated. +describe('commit --files/--files-removed: the two-list parser upholds its partition invariant (#4208 review)', () => { + const LIST = [...COMMIT_LIST_FLAGS]; + // The alphabet a real invocation draws from: positionals, both list flags, + // and the boolean flags that may sit inside a run without ending it. + const token = fc.oneof( + fc.constantFrom('a', 'b', 'c', 'd'), + fc.constantFrom(...LIST), + fc.constantFrom('--amend', '--no-verify', '--raw'), + ); + const argv = fc.array(token, { minLength: 0, maxLength: 10 }) + .map(rest => ['commit', ...rest]); + + // The invariant, stated independently of the implementation: walking argv + // left to right, a list flag opens a run that every later positional joins + // until the next list flag; a boolean flag is transparent; a positional + // before any list flag belongs to the message, not to a list. + function partition(args) { + const out = Object.fromEntries(LIST.map(f => [f, []])); + let open = null; + for (const t of args.slice(1)) { + if (COMMIT_LIST_FLAGS.has(t)) { open = t; continue; } + if (t.startsWith('--')) continue; + if (open !== null) out[open].push(t); + } + return out; + } + + test('every positional lands in exactly the run that is open at it, whatever the flag order or count', () => { + fc.assert(fc.property(argv, (args) => { + const expected = partition(args); + for (const flag of LIST) { + assert.deepStrictEqual(collectListFlagValues(args, flag), expected[flag]); + } + return true; + }), { numRuns: 500 }); + }); + + test('no positional after the first list flag is dropped, and none is claimed by both lists', () => { + fc.assert(fc.property(argv, (args) => { + const first = args.findIndex((a, i) => i > 0 && COMMIT_LIST_FLAGS.has(a)); + if (first === -1) return true; + const afterFirst = args.slice(first + 1).filter(a => !a.startsWith('--')); + const collected = LIST.flatMap(f => collectListFlagValues(args, f)); + // Multiset equality: every such positional is collected exactly once. + assert.deepStrictEqual([...collected].sort(), [...afterFirst].sort()); + return true; + }), { numRuns: 500 }); + }); + + test('with --files-removed absent the parse is the pre-#4208 slice-to-end parse', () => { + // The compatibility half: the only intended change to an invocation that + // never names the second list is that a second list flag now exists. + const legacy = fc.array( + fc.oneof(fc.constantFrom('a', 'b', 'c', 'd'), fc.constantFrom('--files'), fc.constantFrom('--amend', '--no-verify')), + { minLength: 0, maxLength: 8 }, + ).map(rest => ['commit', ...rest]); + fc.assert(fc.property(legacy, (args) => { + const i = args.indexOf('--files'); + const old = i === -1 ? [] : args.slice(i + 1).filter(a => !a.startsWith('--')); + assert.deepStrictEqual(collectListFlagValues(args, '--files'), old); + return true; + }), { numRuns: 500 }); + }); +}); diff --git a/tests/fixtures/compact-content-benchmark-baseline.json b/tests/fixtures/compact-content-benchmark-baseline.json index c7fd6eb63..af0ef488e 100644 --- a/tests/fixtures/compact-content-benchmark-baseline.json +++ b/tests/fixtures/compact-content-benchmark-baseline.json @@ -18,8 +18,8 @@ "reductionPct": 16.51 }, "execute-phase": { - "offTokens": 25631, - "onTokens": 23380, + "offTokens": 25643, + "onTokens": 23392, "reductionPct": 8.78 }, "new-project": { @@ -39,8 +39,8 @@ } }, "aggregate": { - "offTokens": 107090, - "onTokens": 90442, - "reductionPct": 15.55 + "offTokens": 107102, + "onTokens": 90454, + "reductionPct": 15.54 } }