fix(#4208): add --files-removed so commit --files can record a move without a directory pathspec (#4253)

* 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 <paths>` 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) <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 -- <path>` 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 <noreply@anthropic.com>
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 </step>. Harmless at runtime, a formatting artifact of this PR's
own diff; removed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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 -- <paths>`, 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) <noreply@anthropic.com>
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 (`<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) <noreply@anthropic.com>
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' ? '<reason>' : false }` form. The
behaviours they pin are platform-independent; only the fixtures are not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 :<path>`
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) <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FUcGM4FWeZV4cqvR7QBtJh

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: CI Rebase Check <ci@gsd-redux>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
0xdhx
2026-09-11 11:26:55 -05:00
committed by GitHub
parent 9f6f0d27dd
commit 4cc2a466b5
10 changed files with 1820 additions and 47 deletions

View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4253
---
**`commit --files` can now record a file move without a directory pathspec** — a new `--files-removed <paths>` 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.

View File

@@ -1194,9 +1194,11 @@ node gsd-tools.cjs audit-open acknowledge --category <category> --milestone <ver
node gsd-tools.cjs from-gsd2 [--path <dir>] [--force] [--dry-run]
# Git commit with config checks
node gsd-tools.cjs commit <message> [--files f1 f2] [--amend] [--no-verify] [--respect-staged]
node gsd-tools.cjs commit <message> [--files f1 f2] [--files-removed f3 dir/] [--amend] [--no-verify] [--respect-staged]
```
> `--files-removed <paths>` (#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 <paths>` **staging behaviour**: by default, `--files` runs `git add -- <path>` 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 `-- <paths>` pathspec on the commit is applied under both modes, so files staged outside the `--files` scope are never included (#3061 invariant).

View File

@@ -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.

View File

@@ -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.
</step>
<step name="report">

View File

@@ -1381,42 +1381,37 @@ Copy failure must NOT block phase completion.
</step>
<step name="close_phase_todos">
**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.
</step>
<step name="delegate_post_completion_to_transition">

View File

@@ -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",

View File

@@ -1583,7 +1583,349 @@ const COMMIT_DOCS_SKIP_REASON: Record<Exclude<CommitDocsSource, 'default'>, 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 -- <path>` 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<string, IndexEntry>();
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<string>();
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 <empty
// blob> 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 -- <paths>` 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 <path>` 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 (`<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 -- <paths>` 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<string, string>();
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

View File

@@ -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}`);
});
});

File diff suppressed because it is too large Load Diff

View File

@@ -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
}
}