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