From 900504f9852862c33c71b0d2691fe2d047d8d6ea Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Tue, 1 Sep 2026 12:59:11 -0500 Subject: [PATCH] fix(#3776): decide nothing-to-commit from the staged diff, not from staging success (#3859) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3776): decide nothing-to-commit from the staged diff, not from staging success `cmdCommit`'s empty-diff guard tested `stagedPaths.length === 0`, but `stagedPaths` records paths whose `git add` exited 0 — "did staging succeed", not "is there anything to commit". Staging an already-committed, unmodified file succeeds while contributing no diff, so the guard was reachable only when every named path was missing from disk. The ordinary empty-diff case therefore fell through to `git commit`, where the only thing converting the failure back to `nothing_to_commit` was a string match on git's output. Git runs the pre-commit hook before it decides there is nothing to commit, so a rejecting hook pre-empted that match and the caller was handed `commit_failed` carrying a gate message about a commit that had nothing to gate. Ask git whether the staged paths actually differ instead. Two conjuncts are load-bearing: the `length === 0` short-circuit keeps the all-missing-paths case exact (a pathspec-less `diff --cached` would test the whole index, so unrelated staged work would suppress the guard), and `!isMergeInProgress` keeps a merge from being abandoned — during a merge git refuses a partial commit, so the pathspec describes nothing about what would land. Nine regression cases in tests/commands.test.cjs cover the brief's six acceptance criteria plus the merge interaction. Against the pre-fix build exactly one fails; the other eight pin behaviour that was already correct. Residual sibling of #2608/#2693, which covered `git add` failing; this covers `git add` succeeding and contributing nothing. * fix(#3776): exempt a cherry-pick too, but never a revert The empty-diff guard must not decide from a pathspec git will not honour. That test was merge-only; git refuses a partial commit during a cherry-pick for the same reason, so the guard would have fired there and reported a silent `nothing_to_commit` where the pre-fix code surfaced git's refusal. The three sequencer states do not agree, so this is driven rather than reasoned by analogy (git 2.54): MERGE_HEAD fatal: cannot do a partial commit during a merge. CHERRY_PICK_HEAD fatal: cannot do a partial commit during a cherry-pick. REVERT_HEAD permitted; behaves like an ordinary commit. REVERT_HEAD is therefore deliberately excluded: enumerating it alongside the other two — the obvious move — would suppress this fix during a revert and reintroduce the very misreport it removes. Both new states are pinned by a test, and the revert arm fails against the pre-fix build exactly as AC1 does. `canScope` keeps its narrower merge-only test on purpose; widening it would change pre-existing cherry-pick behaviour, which is outside this fix. * fix(#3776): probe the working tree, not the index `git commit -- ` is a PARTIAL commit: it records the working-tree content of those paths and ignores what is staged. The guard was probing `git diff --cached` — the index — which answers a different question than the commit asks. Driven on the same path, in this order: `git add` an unmodified file, then write to it, then probe. git diff --cached --quiet -- p rc 0 ("nothing staged") git diff --quiet HEAD -- p rc 1 ("the tree differs") git commit -m m -- p committed the new content So a working-tree write landing between the `git add` above and the probe — another process in a shared checkout, which this project explicitly supports — would let the guard report `nothing_to_commit` for a call that would have recorded that content. Probing `HEAD` asks the question the commit answers. Not reachable through this function single-threaded, because the staging loop re-adds every named path immediately beforehand, so index and working tree agree at the probe. The change is correctness by construction rather than a fix for an observed miscommit. An unborn HEAD makes `diff HEAD` fatal; that falls through to the commit as any other probe error does, and is now pinned by a test — the first commit in a repo must not be swallowed by an empty-diff guard. Both added probes are now gated on the guard being able to fire at all, so an unscoped commit and an `--amend` pay for neither. Found by adversarial pre-filing review; the index/worktree distinction was not something my own path-shape probes could have surfaced. * test(#3776): pin the assume-unchanged boundary; correct a stale comment `git update-index --assume-unchanged` makes `git add` stage nothing and makes BOTH diff forms — `--cached` and `HEAD` — report no difference, so no diff-based guard can see a change to such a path. `git commit -- ` is the odd one out: it reads the working tree directly and records it. So a modified assume-unchanged path now reports `nothing_to_commit` where it previously committed. That is the answer consistent with this function's own staging step, which honoured the flag one loop earlier — but it is a behaviour change, and it belongs on the record as a decision rather than surfacing later as a surprise. Also corrects an AC3 comment still describing the `diff --cached` whole-index form that the previous commit replaced. * chore(#3776): set changeset fragment pr to 3859 The fragment carries the PR's own number, which is unknowable before the PR exists. Backfilled post-create; the repo's changeset lint rejects the `pr: 0` placeholder. * fix(#3776): do not read an unanswered sequencer probe as "no merge" `execGit` surfaces a spawn timeout as `exitCode: 1` (`_spawnResult`: `result.status ?? 1`) — the same code `rev-parse --verify` returns for a ref that does not exist. So the MERGE_HEAD and CHERRY_PICK_HEAD probes could not tell "not in that state" from "never answered", and the empty-diff guard read both as "not in that state". That is the one path in #3776 that did not fail toward the previous behaviour: a timeout during a real merge decided `nothing_to_commit` from a pathspec git will not honour and left the merge unconcluded, where before it was a loud `commit_failed`. Treat an unanswered probe as "assume the partial commit would be refused" — which falls through to `git commit` and lets git speak for itself. Routed into `partialCommitRefused` only, deliberately never into `isMergeInProgress`. That flag also feeds the pre-existing `canScope`, and widening it there is worse than the misreport it fixes: with `canScope` false the commit runs bare, and a bare commit during a merge is PERMITTED — git concludes the merge with the whole index under a message naming one file. Driven: the whole-flag form reports `committed` where this form reports `commit_failed`, and it drops the pathspec on the ordinary timeout, re-opening the #2112 scope leak. * fix(#3776): pin the empty-diff probe against diff-only configuration `git diff` is porcelain and honours settings `git commit -- ` does not, so an unpinned probe let a caller's configuration decide whether the guard fires. Driven against git 2.54, each with the paired `git commit -- ` confirmed to record the change the unpinned probe reported as absent: diff.ignoreSubmodules=all a gitlink bump is invisible to the probe .gitmodules ignore = all the same, and it needs NO local config at all — it is checked in, so it arrives with a clone diff= + textconv two different blobs converge to one text, so the probe sees no change; no submodule involved `--ignore-submodules=dirty` rather than `=none`, because `dirty` is what a partial commit of a submodule path actually means: it records the gitlink, which moves only when the submodule's HEAD does. Under `=none` a merely dirty submodule work tree reports a difference the commit would not record, sending an empty call back to `git commit` — the same misreport, re-entered from the other side. `dirty` still overrides both `diff.ignoreSubmodules` and a checked-in `.gitmodules` `ignore`, so the gitlink vectors stay closed. `--no-ext-diff` is deliberately absent: `--quiet` short-circuits ahead of an external diff driver, so `diff..command` cannot invert the probe (driven: rc 1 with and without the flag). * docs(#3776): disclose the two outcome changes the changeset omitted The body listed what stays unchanged and never named the arms whose user-visible outcome moves, so neither would have reached the changelog: - a modified path under `git update-index --assume-unchanged` now reports `nothing_to_commit` where it was previously committed. `git add` already honoured the flag one loop earlier; the guard reports what staging did. Documented in a code comment and pinned by a test since the first round, but absent from the fragment. - naming a submodule whose work tree is dirty while its recorded commit has not moved now reports `nothing_to_commit` rather than `commit_failed`, because nothing would have landed. New in this round, from the `--ignore-submodules=dirty` pin. * test(#3776): use helpers.cleanup() for the submodule fixture teardown `local/no-raw-rmsync-in-tests` rejects a bare `fs.rmSync` in a test: the helper carries the Windows-EBUSY retry budget (`maxRetries`/`retryDelay`) that a raw call does not, and a submodule work tree is exactly the shape that holds handles open on Windows. Caught by CI, not locally — the round ran the two affected suites but not `npm run lint:ci`, so the repo's own rule never fired until the push. The chain now exits 0 locally against this tree. * fix(#3776): never drop a named assume-unchanged path `--assume-unchanged` is the one state where `git diff` and `git commit -- ` genuinely disagree: `git add` stages nothing, both diff forms report no difference, and `git commit -- ` still reads the working tree and records it. The empty-diff guard therefore reported `nothing_to_commit` about content the caller named in `--files` and git would have written. commit is made — so suppressing its misreport must not be paid for by a silent drop. Same rule the timeout routing already follows: a fix for a misreport may not cost content. The guard now asks `git commit --dry-run --porcelain` whether the commit would record anything, and stands aside on rc 0. That is the same decision the real commit makes, so there is no second implementation of it to drift. It does not run the `pre-commit` hook (driven: a rejecting one neither fires nor writes a marker), which is what matters — a firing `pre-commit` is the whole of #3776. It is NOT hook-free in general: git 2.54 fires `post-index-change` here, so a repo using that hook sees it once for the probe and once for the commit. Stated rather than claimed away. Asking git rather than reconstructing its answer was reached by measurement. Comparing `git hash-object` against `HEAD:` was tried and is wrong three ways, each a silent drop of named content: it misses a mode-only change (`chmod +x` leaves the blob identical while the commit records `100755`); it cannot hash a submodule path at all (`fatal: Unable to hash sub`, while the commit advances the gitlink); and the path it needs must be parsed out of `ls-files` output, which `core.quotePath` renders as `"caf\303\251.md"` by default. Each has its own arm, and the non-ASCII arm pins `core.quotePath` so it cannot go vacuous. Falling through on the `ls-files` tag alone — without asking whether anything would land — is also wrong: an UNMODIFIED assume-unchanged path would reach `git commit`, which with any unrelated modified file present prints `no changes added to commit`, a string the fallback does not match, and returns `commit_failed`. That is #3776 re-entered from the other side, the same shape `--ignore-submodules=none` would have re-entered it. Pinned by its own arm. The `ls-files` read is an optimisation, not a gate: it keeps the dry run off the hot path when no assume-unchanged entry is present, and when it cannot answer the dry run simply runs, because the dry run needs nothing from it. Failing closed there would drop content and failing open would re-enter #3776 — both are wrong answers to a question that can be asked directly. `--skip-worktree` is not a second instance. A present, modified one exits 1 from `git add` and fails closed as `staging_failed` above the guard; an absent one is skipped before `git add` runs (#2014) and is answered by the `stagedPaths.length === 0` arm, exactly as it was pre-fix. Both shapes pinned, because the shorter claim ("never reaches the guard") is too strong. * test(#3776): register fixture teardown so a failed assertion cannot leak `bumpedSubmodule()` creates its sub-repo as a SIBLING of `tmpDir`, and the unborn-HEAD arm creates `fresh` outside it too, so the describe's `afterEach(() => cleanup(tmpDir))` reaches neither. Both were cleaned by a trailing statement in the test body, which any failing assertion above it skips — leaking a git repo into the temp root. `bumpedSubmodule()` now records the path and a describe-scoped `afterEach` drains it, which covers all three of its callers at once; the unborn-HEAD arm takes `t.after`, the form already used elsewhere in this file. Negative-controlled both ways with a deliberate assertion failure injected into the dirty-submodule arm, under an overridden TMPDIR: before, one `*-sub` repo survives the run; after, none. * fix(#3776): never read an unanswered dry-run probe as "nothing to record" The `git commit --dry-run --porcelain` probe that decides the assume-unchanged boundary is the one probe in the guard whose rc 0 is the reassuring answer, so it inverts the diff probe's safety: `execGit` collapses a spawn timeout (or any spawn error) to `exitCode: 1`, byte-identical to git's own "nothing to record", and the guard then reported `nothing_to_commit` about content named in `--files` that git was never asked to write. Same conflation the sequencer probes already defend against. Only a CONFIRMED rc 1 with no spawn error closes the path now; a timeout, a spawn error, or rc 128 falls toward the real commit, where git speaks for itself. Five arms in tests/commit-files-pathspec.test.cjs pin it (posix + windows timeout shapes, rc 128, the ls-files optimisation's own timeout, and a negative control on an unmodified path); the injection helper gains an optional `matchArg` so the dry run can be targeted without intercepting the real commit. Also corrects the comment that claimed both sequencer probes are gated on `guardApplies` — the MERGE_HEAD probe predates this fix and is unconditional. * fix(#3776): probe with --no-verify so a hook-firing git cannot close the guard Round 4, review finding 3 (Minor). The `git commit --dry-run --porcelain` probe's safety rested on an empirical claim about one git version: that `--dry-run` does not run `pre-commit`. git 2.54 satisfies it, but the failure a differing version would produce is silent and lands in exactly #3776's own configuration. A `pre-commit` that fires and rejects exits 1 — the same code git returns for "nothing to record" — so the closure would read it as a CONFIRMED empty answer, drop the content the caller named in `--files`, and report `nothing_to_commit`. That is #3776 re-entered through the probe the fix added. `--no-verify` forecloses it structurally rather than documenting the version dependency. Driven on git 2.54: rc-identical in both directions (rc 0 would-record, rc 1 nothing) with and without the flag, so it is behaviour- neutral where the version already agrees. Two claims deliberately NOT widened: `--no-verify` does not suppress `post-index-change`, which still fires on this call with or without it (driven both ways); and the real `git commit` is untouched — #3776 is a bug about a hook's message reaching the caller wrongly, never a licence to skip hooks. The new arm pins the FLAG rather than an outcome, because the outcome it protects is unobservable on a git that already declines to run the hook. It is a seam assertion over the argv the guard actually issued, not a source grep. * test(#3776): pin all-missing --files during a merge or cherry-pick Round 4, review finding 1 (Major) and finding 7 (Nit, its coverage half). The review asks for the `stagedPaths.length === 0` disjunct to be gated on `!partialCommitRefused`, or for the combination to be documented and tested. Documented and tested — the gating is refused, with cause. The premise is confirmed: the state is reachable exactly as described, and during a merge the `nothing_to_commit` report does not tell the caller the merge is still open. The prescription is not. With every named path missing, `stagedPaths` is empty, so `canScope` is false and the fall-through reaches a BARE `git commit`, which git PERMITS during a merge and which then CONCLUDES it. Driven, git 2.54, through cmdCommit with the prescription applied: cmdCommit(cwd, 'add the thing', ['.planning/never-produced.md']) -> { "committed": true, "hash": "8e6bf45", "reason": "committed" } MERGE_HEAD gone; HEAD is a 2-parent merge commit recording .planning/shared.md with the caller's resolution content. So the gating trades a report that writes nothing for one that silently writes the whole index under a message naming a path that does not exist, and reports success. That is the same trade the timeout routing already refuses one block up, which is why the sequencer states gate the DIFF branch only. The behaviour is also pre-existing and unchanged by this PR: at 86452da7 the identical short-circuit sat ABOVE the MERGE_HEAD probe, so it never consulted the sequencer either. The residual — a merge held open behind a `nothing_to_commit` report — is offered as a separate issue alongside the three already deferred, not folded into this fix. These are behaviour pins, not regression tests: they pass at base and red on the gated implementation (both arms, verified). * docs(#3776): state the git-version provenance once, not at three claims Round 4, review finding 6 (Nit). The guard carries ~159 comment lines around 32 lines of executable logic, and the review's specific complaint is that "driven against git 2.54" is repeated near-verbatim in three places, which makes the decision tree harder to scan. Hoists the provenance to a single block header and reduces the three repeats to the observation each actually carries. One claim keeps its version explicitly and now says why: the `--no-verify` reasoning is version-SENSITIVE rather than merely version-observed, so it is the one place the version is load-bearing instead of incidental. The behavioural matrix stays inline rather than moving to an ADR or a doc block. Every claim in it is a constraint on the four flags immediately below it, and the value of having it here is that the next reader who wants to "simplify" one of those flags meets the driven counter-example in the same screen. Splitting the constraint from the code it constrains is how the flags get dropped. Comments only. No behaviour change; suite and lint:ci unchanged. * docs(#3776): correct two driven figures in the new guard commentary Both found by this round's own pre-push adversarial review, and both re-driven before adopting. 1. The empty-paths rationale said a bare commit during a merge produces a "three-parent commit". It produces a TWO-parent merge commit. Three was the token count of `git rev-list --parents -n1 HEAD` (commit + two parents) read as a parent count. `git cat-file -p HEAD | grep -c '^parent '` returns 2. This round's commit message for the pins already said two, so the tree contradicted itself. 2. The `post-index-change` disclosure said a repo using that hook "sees it once for the probe and once for the commit". Driven with a counting hook: git fires it TWICE per `git commit --dry-run`, and twice again for the real commit — 2/2/2 across the flagged probe, the unflagged probe and the real commit. The disclosure understated the cost by half in both halves. Comments only. No behaviour change; suite 351/351 and lint:ci unchanged. --------- Co-authored-by: Tom Boucher --- .changeset/quiet-hounds-report.md | 5 + src/commands.cts | 228 ++++++++++- tests/commands.test.cjs | 551 +++++++++++++++++++++++++++ tests/commit-files-pathspec.test.cjs | 385 ++++++++++++++++++- 4 files changed, 1165 insertions(+), 4 deletions(-) create mode 100644 .changeset/quiet-hounds-report.md diff --git a/.changeset/quiet-hounds-report.md b/.changeset/quiet-hounds-report.md new file mode 100644 index 000000000..7d84ce077 --- /dev/null +++ b/.changeset/quiet-hounds-report.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3859 +--- +**A scoped `commit --files` call whose named files are already committed and unmodified now reports `nothing_to_commit` instead of a failed commit carrying your pre-commit hook's rejection message.** The empty-diff case used to reach `git commit`, where a rejecting hook fires before git can report "nothing to commit" — so callers were handed `commit_failed` and a gate message that was true about the repository and irrelevant to the call. Genuine rejections still report `commit_failed` with the hook's message, and `--amend`, missing named paths, and merges or cherry-picks in progress are unchanged. A modified path under `git update-index --assume-unchanged` is still committed exactly as before: `git commit -- ` reads the working tree directly, so the guard compares that content against `HEAD` and stands aside rather than dropping content you named. One further outcome does change: naming a submodule whose work tree is dirty but whose recorded commit has not moved now reports `nothing_to_commit` rather than `commit_failed`, because nothing would have landed. (#3776) diff --git a/src/commands.cts b/src/commands.cts index 897ce3c65..6b72d9335 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1807,12 +1807,236 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // During a merge, git refuses partial commits — fall back to a bare commit. // --amend is left without a pathspec: amending with -- is a different // operation that rewrites the tip with only those paths. - if (explicitFiles && stagedPaths.length === 0 && !amend) { + const mergeHeadProbe = execGit(['rev-parse', '-q', '--verify', 'MERGE_HEAD'], { cwd }); + const isMergeInProgress = mergeHeadProbe.exitCode === 0; + // PROVENANCE FOR THIS WHOLE BLOCK: every behavioural claim below was DRIVEN + // against git 2.54, not reasoned by analogy. Individual claims state what was + // observed and omit the version; where a claim is version-SENSITIVE rather + // than merely version-observed, it says so at the claim. + // + // #3776: git refuses a PARTIAL commit (`git commit -- `) while a merge + // or a cherry-pick is in progress, so in those states the pathspec describes + // nothing about what would actually land and the empty-diff decision below + // must not be made from it. The three sequencer states do NOT agree: + // MERGE_HEAD -> `fatal: cannot do a partial commit during a merge.` + // CHERRY_PICK_HEAD -> `fatal: cannot do a partial commit during a cherry-pick.` + // REVERT_HEAD -> permitted; behaves like an ordinary commit. + // REVERT_HEAD is therefore deliberately absent: including it would suppress + // this fix during a revert, reintroducing the very misreport it removes. + // `canScope` below keeps its narrower merge-only test on purpose — widening it + // would change pre-existing cherry-pick behaviour, which is outside this fix. + // Only the scoped, non-amend call can return through the guard below, so the + // cherry-pick probe and the guard's own probes are gated on that — an + // unscoped commit or an --amend would otherwise pay for git invocations whose + // answer it can never use. The MERGE_HEAD probe above predates this fix and + // stays unconditional: `canScope` needs it on every path. + // A non-zero exit from either sequencer probe means "not in that state" AND + // "the probe never answered" — `execGit` surfaces a spawn timeout as + // `exitCode: 1` (`_spawnResult`: `result.status ?? 1`), which is the exact + // code `rev-parse --verify` returns for a ref that does not exist. Conflating + // them is the one path in this fix that does NOT fail toward the old + // behaviour: a timeout during a real merge would leave `partialCommitRefused` + // false, the guard would decide `nothing_to_commit` from a pathspec git will + // not honour, and the merge would be silently abandoned where it previously + // reported a loud `commit_failed`. So an unanswered probe is treated as + // "assume the partial commit would be refused" — the conservative reading, + // which falls through to `git commit` and lets git speak for itself. + // + // This is deliberately routed into `partialCommitRefused` ONLY, never into + // `isMergeInProgress`: that flag also feeds the pre-existing `canScope` below, + // where a spurious timeout would convert a scoped commit into a bare one and + // record the whole index instead of the named paths. Suppressing a misreport + // must not be paid for by committing content the caller never named. + const guardApplies = explicitFiles && !amend; + const cherryPickProbe = guardApplies + ? execGit(['rev-parse', '-q', '--verify', 'CHERRY_PICK_HEAD'], { cwd }) + : null; + const partialCommitRefused = isMergeInProgress + || isSpawnTimeout(mergeHeadProbe) + || (cherryPickProbe !== null + && (cherryPickProbe.exitCode === 0 || isSpawnTimeout(cherryPickProbe))); + // `stagedPaths` records paths whose `git add` exited 0 — that is "did + // staging succeed", not "is there anything to commit". Staging an + // already-committed, unmodified file succeeds while contributing no diff, so + // `length === 0` is reachable only when EVERY named path was missing from + // disk. For the ordinary empty-diff case control fell through to `git commit`, + // and the only thing converting that back to `nothing_to_commit` was the + // string match on git's output below — which a rejecting pre-commit hook + // pre-empts, because git runs the hook before it decides there is nothing to + // commit. The caller was then handed `commit_failed` carrying a gate message + // about a commit that had nothing to gate. Ask git whether the named paths + // actually differ instead. Three things about that probe are load-bearing: + // - it compares the WORKING TREE to HEAD (`diff HEAD`), not the index + // (`diff --cached`). `git commit -- ` is a partial commit: it takes + // the working-tree content of those paths and ignores what is staged. A + // probe against the index therefore answers a different question than the + // commit asks, and a working-tree write landing between the `git add` + // above and this line — another process in a shared checkout — would make + // the index say "empty" while the commit would still have recorded the new + // content. Driven: `diff --cached` rc 0 and `diff HEAD` rc 1 on the same + // path, with `git commit -- ` then committing it. + // - the `length === 0` short-circuit keeps the all-missing-paths case exact. + // Spreading an empty array yields a pathspec-less `diff`, which tests the + // WHOLE tree — unrelated work elsewhere would then suppress the guard and + // regress the skip-missing contract (#2014). + // It is deliberately NOT gated on `partialCommitRefused`, and gating it + // would be a REGRESSION rather than a hardening. With every named path + // missing, `stagedPaths` is empty, so `canScope` is false and the + // fall-through reaches a BARE `git commit` — which git PERMITS during a + // merge, and which then CONCLUDES that merge: rc 0, a two-parent merge + // commit recording the entire index, under a message naming a path that + // does not exist, reported to the caller as `committed: true` (driven). + // Today's answer writes nothing at all. That is the same trade the timeout + // routing above already refuses — a misreport must not be paid for by + // committing content the caller never named — which is why the sequencer + // states gate the DIFF branch only. The behaviour is also PRE-EXISTING and + // unchanged by this fix: before it the identical short-circuit ran ABOVE + // the MERGE_HEAD probe, so it never consulted the sequencer either. The + // residual it leaves — a merge held open behind a `nothing_to_commit` + // report — is offered as a separate issue with the other three, not folded + // in here. Both sequencer shapes are pinned in + // tests/commit-files-pathspec.test.cjs. + // - `!partialCommitRefused`: see above — deciding "nothing to commit" from a + // pathspec git will not honour would abandon an in-progress merge, so those + // states keep their pre-existing behaviour untouched. + // - the probe is pinned against user configuration that would make `git diff` + // answer a DIFFERENT question than `git commit -- ` asks. `git diff` + // is porcelain and honours settings the commit does not, so without these + // flags a caller's config decides whether the guard fires. Each vector + // below was driven with the paired `git commit -- ` confirmed to + // record the change the probe reported as absent: + // `diff.ignoreSubmodules=all` -> a gitlink bump is invisible to the probe + // `.gitmodules` `ignore = all` -> the same, and it needs NO local config: + // it is checked in, so it arrives with a + // clone + // `diff=` + `textconv` -> two different blobs converge to one + // text, so the probe sees no change at + // all; no submodule involved + // `--ignore-submodules=dirty` rather than `=none`, because `dirty` is what + // a partial commit of a submodule path actually means: it records the + // GITLINK, and the gitlink moves only when the submodule's HEAD does. Under + // `=none` a merely dirty submodule WORKTREE reports a difference the commit + // would not record, sending an empty call back to `git commit` — the #3776 + // misreport, re-entered from the other side. `dirty` still overrides both + // `diff.ignoreSubmodules` and a checked-in `.gitmodules` `ignore`, so the + // gitlink vectors above stay closed (driven: rc 1 under every one of them). + // `--no-ext-diff` is deliberately absent: `--quiet` short-circuits ahead of + // an external diff driver, so an external `diff..command` cannot + // invert the probe (driven: rc 1 with and without the flag). + // Any other non-zero exit from the probe (a genuine git error, or an unborn + // HEAD) leaves the guard shut and falls through to the commit — failing toward + // today's path rather than manufacturing a no-op. + // THE ONE STATE WHERE `git diff` AND `git commit -- ` GENUINELY DISAGREE. + // `--assume-unchanged` tells git to skip the worktree stat for a path, so + // `git add` stages nothing and BOTH diff forms report no difference — while + // `git commit -- ` reads the working tree directly and records it + // (driven: probe rc 0, commit rc 0, new content in the tree). Left + // to the diff probe alone the guard reports `nothing_to_commit` about content + // the caller explicitly named in `--files` and git would have written. #3776 + // is a purely diagnostic bug — nothing is corrupted and no wrong commit is + // made — so suppressing its misreport must not be paid for by dropping named + // content. The same rule the timeout routing already follows one block up. + // + // `git ls-files -v` is the discriminator for the STATE: it tags an + // assume-unchanged path with a LOWERCASE letter (`h`), where + // `--skip-worktree` is an uppercase `S` and never reaches THIS branch: + // `git add` exits 1 under it, so a present-but-modified skip-worktree path + // fails closed as `staging_failed` above the guard. (An ABSENT one is skipped + // before `git add` runs at all per #2014, and is answered by the + // `stagedPaths.length === 0` arm above — correctly, and exactly as it was + // pre-fix. Both shapes are pinned.) + // + // Then ASK GIT, rather than reconstructing its answer. `git commit --dry-run` + // is the same decision the real commit makes, and `--no-verify` is what keeps + // it a DECISION rather than an execution. git 2.54 already declines to run + // `pre-commit` on a dry run (driven: a rejecting one neither fires nor writes + // its marker), which is the property that matters here, because a firing + // `pre-commit` is the whole of #3776 — but that is an observed behaviour of + // one version, and the failure it would produce on a version that differs is + // SILENT. A `pre-commit` that fires and rejects exits 1, the same code git + // returns for `nothing to record`, so the closure below would read it as a + // CONFIRMED empty answer, drop the content the caller named, and report + // `nothing_to_commit` — #3776's exact shape, in #3776's exact configuration. + // `--no-verify` forecloses that structurally instead of resting on the + // version, and is behaviour-neutral where the version already agrees (driven: + // rc 0 would-record / rc 1 nothing, identical with and without it). This is + // VERSION-SENSITIVE reasoning, hence stated at the claim per the provenance + // note above. + // + // It is still NOT hook-free in general, and `--no-verify` does not widen that + // claim: `post-index-change` fires on this call with or without the flag + // (driven both ways), so a repo using that hook sees TWO extra invocations + // for the probe — git fires it twice per `commit --dry-run`, and twice again + // for the real commit (driven: 2/2/2 across flagged probe, unflagged probe + // and real commit). Stated rather than claimed away; the narrower + // claim is the true one. `--porcelain` keeps the output to a couple + // of machine-readable lines instead of a full status listing — the rc is + // identical either way (driven: 0 would-record / 1 nothing), but the plain + // form prints every untracked path, which on a large tree is output this + // probe has no use for and `execGit` would have to buffer. rc 0 means the + // commit would record something, so the guard must stand aside. + // + // Reconstructing it was tried and is WRONG in three measured ways, all of + // them silent drops of named content. Comparing `git hash-object` against + // `HEAD:` misses a mode-only change (`chmod +x` leaves the blob + // identical while `git commit -- ` records `100755`); it cannot hash a + // submodule path at all (`fatal: Unable to hash sub`, while the commit + // advances the gitlink); and the path it needs must be parsed out of + // `ls-files` output, which `core.quotePath` renders as `"caf\303\251.md"` + // by default, so the probe reads a filename that does not exist. Asking git + // needs no path parsed and no case enumerated. + // + // Scoped to this branch on purpose. The diff probe above answers the ordinary + // case cheaply and is pinned against the configuration vectors below; the + // dry run is the heavier, exact answer, and it runs only when an + // assume-unchanged path is actually present. + // + // The `ls-files` read is an OPTIMISATION, never a gate — so an unreadable one + // must not decide anything. It exists only to keep the dry run off the hot + // path when no assume-unchanged entry is present; when it cannot answer, the + // dry run simply runs, because the dry run needs nothing from it. Both + // 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 }); + // 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], + { cwd }, + ); + // Only a CONFIRMED "nothing to record" closes the path: rc 1 from a git + // that actually answered. This is the one probe in the guard whose rc 0 + // is the REASSURING answer, so it inverts the diff probe's safety: there + // a timeout can only yield non-zero and reads as "not clean"; here + // `execGit` collapses a spawn timeout (or any spawn error) to + // `exitCode: 1` (`_spawnResult`: `result.status ?? 1`), byte-identical to + // git's own "nothing to record" — and the guard then reports + // `nothing_to_commit` about content it never asked git to write. Same + // conflation the sequencer probes above defend against, same remedy: an + // unanswered probe falls toward the commit, where git speaks for itself + // (and a genuine error there is reported loudly, as it always was). rc 128 + // is likewise not an answer. Timeout kill of a dry run CAN leave a stale + // `index.lock` behind (it refreshes the index); the real commit then + // fails on it, loudly — never silently. + if (isSpawnTimeout(dryRun) || dryRun.error !== null) return true; + return dryRun.exitCode !== 1; + }; + const nothingToCommit = guardApplies + && (stagedPaths.length === 0 + || (!partialCommitRefused + && execGit( + ['diff', '--quiet', '--ignore-submodules=dirty', '--no-textconv', 'HEAD', '--', ...stagedPaths], + { cwd }, + ).exitCode === 0 + && !assumeUnchangedWouldRecord())); + if (nothingToCommit) { const result = { committed: false, hash: null, reason: 'nothing_to_commit' }; output(result, raw, 'nothing'); return; } - const isMergeInProgress = execGit(['rev-parse', '-q', '--verify', 'MERGE_HEAD'], { cwd }).exitCode === 0; const canScope = explicitFiles && stagedPaths.length > 0 && !amend && !isMergeInProgress; const commitArgs = amend diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index 98eda61f7..942242683 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -5189,6 +5189,557 @@ describe('query commit --files scoping (#2269)', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// #3776: `query commit --files` must report an empty diff as `nothing_to_commit` +// even when a pre-commit hook would reject. +// +// `stagedPaths` records paths whose `git add` exited 0 — "did staging succeed", +// not "is there anything to commit". Staging an already-committed, unmodified +// file succeeds and contributes nothing, so the old `stagedPaths.length === 0` +// guard was reachable only when EVERY named path was missing from disk. The +// ordinary empty-diff case fell through to `git commit`, where the sole rescue +// was a string match on git's "nothing to commit" output — and git runs the +// pre-commit hook BEFORE deciding there is nothing to commit, so a rejecting +// hook pre-empted the match and the caller was handed `commit_failed` carrying +// a gate message about a commit that had nothing to gate. +// +// The residual sibling of #2608/#2693, which covered `git add` FAILING; this +// covers `git add` succeeding and contributing nothing. +// ───────────────────────────────────────────────────────────────────────────── + +describe('#3776: query commit --files reports an empty diff as nothing_to_commit', () => { + const { createTempGitProject } = require('./helpers.cjs'); + // runGit (never gitOrThrow) for the conflicting merge below — that merge is + // MEANT to exit non-zero, and the throwing wrapper would fail the fixture. + const { runGit } = require('./helpers/process-seam.cjs'); + let tmpDir; + + const REJECTING_HOOK = '#!/bin/sh\necho "gate: BACKLOG.md is stale" >&2\nexit 1\n'; + const PASSING_HOOK = '#!/bin/sh\nexit 0\n'; + + // Writes .git/hooks/pre-commit. Every arm below drives the real hook, not a + // stub of it: the defect lives in git's own hook-before-empty-diff ordering, + // so a faked rejection would not exercise the mechanism under test. + function installHook(body) { + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + fs.writeFileSync(hookPath, body); + fs.chmodSync(hookPath, 0o755); + } + + // A tracked, committed, unmodified file — `git add` on it succeeds and + // contributes no diff. This is the exact shape the guard used to miss. + function commitFixtureFile(name = 'doc.md', body = 'hello\n') { + const rel = path.posix.join('.planning', name); + fs.writeFileSync(path.join(tmpDir, '.planning', name), body); + gitOrThrow(['add', '--', rel], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'fixture: ' + name], { cwd: tmpDir }); + return rel; + } + + // The command emits its JSON payload on either stream depending on outcome; + // read whichever carries it rather than assuming success. + function commitFiles(rel, extra = '') { + const result = runGsdTools('commit "m"' + extra + ' --files ' + rel, tmpDir); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + return JSON.parse(payload); + } + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // AC1 — the defect. Pre-fix this returned commit_failed + the hook's message. + test('AC1: empty diff + rejecting pre-commit hook reports nothing_to_commit, not the hook rejection', () => { + const rel = commitFixtureFile(); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit', + 'an empty-diff --files call must not be reported as a failed commit'); + assert.ok(!output.error, + 'no hook message may be surfaced for a call that had nothing to gate'); + }); + + // AC2 — the two controls that isolate the hook as the only variable. + test('AC2: empty diff + no hook still reports nothing_to_commit', () => { + const rel = commitFixtureFile(); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + }); + + test('AC2: empty diff + passing hook still reports nothing_to_commit', () => { + const rel = commitFixtureFile(); + installHook(PASSING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + }); + + // AC3 — the all-missing short-circuit must not regress. + test('AC3: every named path missing from disk still reports nothing_to_commit', () => { + const rel = commitFixtureFile(); + fs.unlinkSync(path.join(tmpDir, rel)); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + }); + + // AC3, sharp edge: the `stagedPaths.length === 0` short-circuit is + // load-bearing, not defensive noise. Without it an all-missing call spreads + // an empty array into the pathspec, and a pathspec-less `git diff HEAD` + // tests the WHOLE tree — so unrelated work would suppress the guard and turn + // this arm into a commit of somebody else's changes. + test('AC3: all named paths missing does not consult unrelated staged work', () => { + const rel = commitFixtureFile(); + fs.unlinkSync(path.join(tmpDir, rel)); + const unrelated = path.posix.join('.planning', 'unrelated.md'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'staged by the caller\n'); + gitOrThrow(['add', '--', unrelated], { cwd: tmpDir }); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit'); + + const staged = gitOrThrow(['diff', '--cached', '--name-only'], { cwd: tmpDir }); + assert.match(staged, /unrelated\.md/, + "the caller's own staged work must be left in the index, not swept into a commit"); + }); + + // AC4 — a genuine rejection must still be reported. The goal is to stop + // reporting a rejection for a call that never had anything to gate, not to + // stop reporting rejections. + test('AC4: a real diff rejected by the hook still reports commit_failed with the hook message', () => { + const rel = commitFixtureFile(); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified\n'); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'commit_failed'); + assert.match(String(output.error), /BACKLOG\.md is stale/, + "the hook's own message must still reach the caller"); + }); + + test('AC4: a real diff with no hook still commits', () => { + const rel = commitFixtureFile(); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified\n'); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, true); + assert.strictEqual(output.reason, 'committed'); + assert.ok(output.hash, 'a successful commit must carry its hash'); + }); + + // AC5 — amending has a different empty-diff meaning; the guard stays exempt. + test('AC5: --amend remains exempt from the empty-diff guard', () => { + const rel = commitFixtureFile(); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel, ' --amend'); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'commit_failed', + '--amend must still reach git, where the hook governs the rewrite'); + }); + + // Beyond the brief's ACs: during a merge git refuses a partial commit, so the + // commit runs WITHOUT the pathspec and the named paths describe nothing about + // what would land. Deciding "nothing to commit" from them would abandon the + // merge — which is why the empty-diff probe is gated on !isMergeInProgress. + // Sets up a conflicted history and leaves the caller mid-sequence. `rel` (the + // file the commit call names) is never touched by the conflict, so it always + // contributes no diff of its own — which is what puts these arms on the + // empty-diff branch under test. + function conflictedSequence(kind) { + const shared = path.posix.join('.planning', 'shared.md'); + fs.writeFileSync(path.join(tmpDir, shared), 'base\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'shared base'], { cwd: tmpDir }); + const trunk = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim(); + + if (kind === 'revert') { + fs.writeFileSync(path.join(tmpDir, shared), 'second\n'); + gitOrThrow(['commit', '-am', 'second'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'third\n'); + gitOrThrow(['commit', '-am', 'third'], { cwd: tmpDir }); + runGit(['revert', '--no-edit', 'HEAD~1'], { cwd: tmpDir }); + } else { + gitOrThrow(['checkout', '-b', 'side'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'side\n'); + gitOrThrow(['commit', '-am', 'side edit'], { cwd: tmpDir }); + gitOrThrow(['checkout', trunk], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'trunk\n'); + gitOrThrow(['commit', '-am', 'trunk edit'], { cwd: tmpDir }); + runGit([kind === 'merge' ? 'merge' : 'cherry-pick', 'side'], { cwd: tmpDir }); + } + fs.writeFileSync(path.join(tmpDir, shared), 'resolved\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + } + + // The one state where `git diff` and `git commit -- ` genuinely + // disagree. `--assume-unchanged` makes `git add` stage nothing and BOTH diff + // forms (`--cached` and `HEAD`) report no difference, while + // `git commit -- ` reads the working tree directly and records it. The + // pre-#3776 build therefore COMMITTED this, and the guard must not turn a + // purely diagnostic fix into a silent drop of content the caller named in + // `--files`. Asking `git commit --dry-run` preserves the pre-fix outcome + // exactly, because it is the same decision the real commit makes. + test('a modified assume-unchanged path is still committed, not swallowed by the guard', () => { + const rel = commitFixtureFile(); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified under assume-unchanged\n'); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, true, + 'git commit -- reads the working tree and records it; the guard must not pre-empt that'); + assert.strictEqual( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir }), + 'hello\nmodified under assume-unchanged\n', + 'and the content it records must be the working-tree content'); + }); + + // THE OTHER DIRECTION, and the reason the check compares CONTENT rather than + // stopping at the `ls-files -v` tag. An unmodified assume-unchanged path has + // nothing to record; falling through on the tag alone would hand it to + // `git commit`, which — with any unrelated modified file present — prints + // `no changes added to commit`, a string the fallback does not match, and + // returns `commit_failed`. That is #3776 re-entered from the other side, the + // same shape `--ignore-submodules=none` would have re-entered it. Pinned so a + // later simplification to a tag-only test cannot pass. + test('an UNMODIFIED assume-unchanged path still reports nothing_to_commit, even with unrelated dirt', () => { + const rel = commitFixtureFile(); + const unrelated = path.posix.join('.planning', 'unrelated.md'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'seed\n'); + gitOrThrow(['add', '--', unrelated], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'seed unrelated'], { cwd: tmpDir }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + // Unrelated modified work present — this is what turns git's answer from + // `nothing to commit` into `no changes added to commit`. + fs.writeFileSync(path.join(tmpDir, unrelated), 'unrelated edit\n'); + + assert.strictEqual(commitFiles(rel).reason, 'nothing_to_commit', + 'nothing would land for the named path, so the guard must still answer nothing_to_commit'); + }); + + // `--skip-worktree` is NOT a second instance of the above, and the PR body + // used to group them. `git add` exits 1 under it (the path reads as outside + // the sparse-checkout definition), so it fails closed as `staging_failed` + // ABOVE this guard and never reaches the empty-diff decision at all. + test('a modified skip-worktree path fails closed as staging_failed, never reaching the guard', () => { + const rel = commitFixtureFile(); + gitOrThrow(['update-index', '--skip-worktree', '--', rel], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, rel), 'hello\nmodified under skip-worktree\n'); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'staging_failed', + 'git add refuses the path, so the staging-failure block above the guard owns this case'); + }); + + // The same flag with the path ABSENT from disk — the canonical sparse shape — + // takes a DIFFERENT route, and the distinction is worth pinning because the + // obvious reading of the arm above ("skip-worktree never reaches the guard") + // is too strong. A missing path is skipped before `git add` runs at all + // (#2014), so `stagedPaths` is empty and the guard's own + // `stagedPaths.length === 0` arm answers it. `nothing_to_commit` is the + // correct answer there — the file does not exist, so a commit would record + // nothing — and it is the PRE-FIX answer too, unchanged by this PR. + test('a skip-worktree path absent from disk reports nothing_to_commit via the missing-path arm', () => { + const rel = commitFixtureFile(); + gitOrThrow(['update-index', '--skip-worktree', '--', rel], { cwd: tmpDir }); + fs.unlinkSync(path.join(tmpDir, rel)); + + assert.strictEqual(commitFiles(rel).reason, 'nothing_to_commit', + 'a missing path is skipped before git add, so the length === 0 arm owns this — not staging_failed'); + }); + + // THREE ARMS PINNING WHY THE PROBE ASKS GIT RATHER THAN RECONSTRUCTING ITS + // ANSWER. Each one reds if the dry run is replaced by a + // `hash-object` vs `HEAD:` blob comparison, and each is a silent drop + // of content the caller named — the exact class this whole guard is careful + // about. + + // A mode-only change leaves the blob identical, so a content comparison sees + // nothing — while `git commit -- ` records the new mode. + test('a mode-only change to an assume-unchanged path is still committed', (t) => { + const rel = commitFixtureFile('exec.md'); + // Windows, and any checkout with `core.filemode=false`, cannot represent + // the bit — `chmodSync` would then be a no-op and this arm would pass while + // pinning nothing. Assert the precondition and skip loudly instead. + gitOrThrow(['config', 'core.filemode', 'true'], { cwd: tmpDir }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + fs.chmodSync(path.join(tmpDir, rel), 0o755); + if (!/^100755 /.test(gitOrThrow(['ls-files', '-s', '--', rel], { cwd: tmpDir })) + && (fs.statSync(path.join(tmpDir, rel)).mode & 0o111) === 0) { + t.skip('filesystem cannot represent the executable bit — nothing to pin here'); + return; + } + + assert.strictEqual(commitFiles(rel).committed, true, + 'the mode moved and git would record it, so the guard must not report nothing_to_commit'); + assert.match( + gitOrThrow(['ls-tree', 'HEAD', '--', rel], { cwd: tmpDir }), /^100755 /, + 'and the recorded mode must actually be the executable one'); + }); + + // A non-ASCII path is rendered QUOTED by `git ls-files -v` under the default + // `core.quotePath` (`"caf\303\251.md"`), so any probe that parses the path + // out of that output reads a filename that does not exist and silently + // concludes there is nothing to commit. + test('a modified assume-unchanged path with a non-ASCII name is still committed', () => { + const rel = commitFixtureFile('caf\u00e9.md'); + // PIN the quoting explicitly. This arm's whole point is that a probe + // parsing the path out of `ls-files -v` reads `"caf\303\251.md"` and finds + // no such file; under an ambient `core.quotePath=false` the rejected + // implementation would pass here and the arm would be vacuous. + gitOrThrow(['config', 'core.quotePath', 'true'], { cwd: tmpDir }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, rel), 'modified\n'); + + assert.strictEqual(commitFiles(rel).committed, true, + 'core.quotePath must not be able to hide a real change from the probe'); + assert.strictEqual(gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir }), 'modified\n'); + }); + + // The probe compares the WORKING TREE to HEAD, so an unborn HEAD makes it + // fatal (rc 128). That must fall through to the commit rather than be read as + // "nothing to commit" — there is plenty to commit in a repo with no commits. + test('an unborn HEAD falls through to the commit rather than reporting nothing_to_commit', (t) => { + const fresh = createTempDir(); + // REGISTERED teardown, not a trailing statement: `fresh` lives outside + // `tmpDir`, so afterEach does not reach it and any failing assertion below + // would leak a git repo into the temp root. + t.after(() => cleanup(fresh)); + fs.mkdirSync(path.join(fresh, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(fresh, '.planning', 'config.json'), '{}\n'); + fs.writeFileSync(path.join(fresh, '.planning', 'doc.md'), 'first content\n'); + gitOrThrow(['init', '-q', '.'], { cwd: fresh }); + gitOrThrow(['config', 'user.email', 't@t'], { cwd: fresh }); + gitOrThrow(['config', 'user.name', 't'], { cwd: fresh }); + + const result = runGsdTools('commit "m" --files .planning/doc.md', fresh); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + const output = JSON.parse(payload); + assert.strictEqual(output.committed, true, + 'the very first commit in a repo must not be swallowed by the empty-diff guard'); + }); + + // git refuses a partial commit during a cherry-pick exactly as it does during + // a merge, so the guard must stay out of the way there too — this arm pins + // that the pre-fix outcome is preserved rather than turned into a silent + // no-op. Driven, not assumed: the three sequencer states disagree. + test('a cherry-pick in progress keeps its pre-existing outcome', () => { + const rel = commitFixtureFile(); + conflictedSequence('cherry-pick'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'CHERRY_PICK_HEAD')), + 'fixture must leave a cherry-pick in progress'); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'commit_failed', + 'git refuses the partial commit here; that must not become a silent nothing_to_commit'); + assert.match(String(output.error), /partial commit/, + "git's own refusal must reach the caller"); + }); + + // REVERT_HEAD is deliberately NOT in the refusal set: a revert permits partial + // commits, so the fix must still apply there. Including it would suppress the + // fix during a revert and reintroduce the misreport. + test('a revert in progress still reports nothing_to_commit, not the hook rejection', () => { + const rel = commitFixtureFile(); + conflictedSequence('revert'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'REVERT_HEAD')), + 'fixture must leave a revert in progress'); + installHook(REJECTING_HOOK); + + const output = commitFiles(rel); + assert.strictEqual(output.committed, false); + assert.strictEqual(output.reason, 'nothing_to_commit', + 'a revert permits partial commits, so the empty-diff guard must still apply'); + }); + + test('a merge in progress is still concluded when the named paths carry no diff', () => { + const shared = path.posix.join('.planning', 'shared.md'); + fs.writeFileSync(path.join(tmpDir, shared), 'base\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'shared base'], { cwd: tmpDir }); + const rel = commitFixtureFile(); + + const trunk = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir }).trim(); + gitOrThrow(['checkout', '-b', 'side'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'side\n'); + gitOrThrow(['commit', '-am', 'side edit'], { cwd: tmpDir }); + gitOrThrow(['checkout', trunk], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'trunk\n'); + gitOrThrow(['commit', '-am', 'trunk edit'], { cwd: tmpDir }); + + // Conflicting merge, then resolve it so the index carries real content. + runGit(['merge', 'side'], { cwd: tmpDir }); + fs.writeFileSync(path.join(tmpDir, shared), 'resolved\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir }); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'fixture must leave a merge in progress'); + + // `rel` is committed and unmodified — it contributes no diff of its own. + const output = commitFiles(rel); + assert.strictEqual(output.committed, true, + 'the merge must still be concluded, not reported as nothing to commit'); + assert.ok(!fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'MERGE_HEAD must be gone once the merge commit lands'); + }); +}); + +// #3859: the empty-diff probe must answer the question `git commit -- ` +// asks. `git diff` is porcelain and honours user configuration the commit does +// not, so an unpinned probe lets a caller's config decide whether the guard +// fires — and every arm below was driven against git 2.54 by confirming that +// `git commit -- ` records exactly the change the unpinned probe reports +// as absent. +describe('#3859: the empty-diff probe is pinned against diff-only configuration', () => { + const { createTempGitProject } = require('./helpers.cjs'); + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function commitFiles(rel) { + const result = runGsdTools('commit "m" --files ' + rel, tmpDir); + const payload = (result.output && result.output.trim()) ? result.output : result.error; + return JSON.parse(payload); + } + + // Sub-repos created by `bumpedSubmodule()` are SIBLINGS of `tmpDir`, so the + // `afterEach` above does not reach them. Registering them here cleans every + // caller at once and — unlike a trailing `cleanup(subSrc)` in each test body + // — survives a failing assertion, which would otherwise leak a git repo into + // the temp root. + const strayRepos = []; + afterEach(() => { + while (strayRepos.length > 0) cleanup(strayRepos.pop()); + }); + + // A submodule whose recorded gitlink is AHEAD of what the superproject has + // committed — i.e. `git commit -- ` has something real to record. + function bumpedSubmodule() { + const subSrc = path.join(tmpDir, '..', path.basename(tmpDir) + '-sub'); + strayRepos.push(subSrc); + fs.mkdirSync(subSrc, { recursive: true }); + gitOrThrow(['init', '-q', '.'], { cwd: subSrc }); + gitOrThrow(['config', 'user.email', 't@t'], { cwd: subSrc }); + gitOrThrow(['config', 'user.name', 't'], { cwd: subSrc }); + fs.writeFileSync(path.join(subSrc, 'f.txt'), 'v1\n'); + gitOrThrow(['add', 'f.txt'], { cwd: subSrc }); + gitOrThrow(['commit', '-m', 'v1'], { cwd: subSrc }); + + gitOrThrow(['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', subSrc, 'sub'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'add submodule'], { cwd: tmpDir }); + + fs.writeFileSync(path.join(subSrc, 'f.txt'), 'v2\n'); + gitOrThrow(['add', 'f.txt'], { cwd: subSrc }); + gitOrThrow(['commit', '-m', 'v2'], { cwd: subSrc }); + gitOrThrow(['-c', 'protocol.file.allow=always', 'submodule', 'update', '--remote', '--', 'sub'], { cwd: tmpDir }); + } + + // `diff.ignoreSubmodules=all` is local config; `.gitmodules` `ignore = all` is + // CHECKED IN and so arrives with a clone, needing no local setting at all — + // which makes it the stronger of the two vectors, and the one a reviewer + // reading only `diff.ignoreSubmodules` would not reach. + for (const vector of ['diff.ignoreSubmodules', '.gitmodules ignore']) { + test(`a submodule bump is not reported as nothing_to_commit under ${vector}=all`, () => { + bumpedSubmodule(); + if (vector === 'diff.ignoreSubmodules') { + gitOrThrow(['config', 'diff.ignoreSubmodules', 'all'], { cwd: tmpDir }); + } else { + gitOrThrow(['config', '-f', '.gitmodules', 'submodule.sub.ignore', 'all'], { cwd: tmpDir }); + gitOrThrow(['add', '--', '.gitmodules'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'gitmodules ignore=all'], { cwd: tmpDir }); + } + + const before = gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(); + const output = commitFiles('sub'); + + assert.notStrictEqual(output.reason, 'nothing_to_commit', + 'the gitlink moved and `git commit -- sub` records it, so the probe must not say there is nothing'); + assert.strictEqual(output.committed, true); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(), before, + 'the recorded gitlink must actually advance'); + }); + } + + // A submodule path cannot be hashed at all (`fatal: Unable to hash sub`), + // while `git commit -- sub` advances the recorded gitlink. + test('an assume-unchanged submodule with an advanced gitlink is still committed', () => { + bumpedSubmodule(); + const before = gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(); + gitOrThrow(['update-index', '--assume-unchanged', '--', 'sub'], { cwd: tmpDir }); + + assert.notStrictEqual(commitFiles('sub').reason, 'nothing_to_commit', + 'the gitlink would advance, so the guard must stand aside'); + assert.notStrictEqual( + gitOrThrow(['rev-parse', 'HEAD:sub'], { cwd: tmpDir }).trim(), before, + 'and the recorded gitlink must actually advance'); + }); + + // The other direction, and the reason the pin is `=dirty` rather than `=none`. + // A partial commit of a submodule path records the GITLINK, which moves only + // when the submodule's HEAD does — so a merely dirty submodule WORKTREE would + // land nothing. Under `--ignore-submodules=none` the probe reports a + // difference there and sends an empty call back to `git commit`, which is the + // #3776 misreport re-entered from the other side. Pinned so a later widening + // to `=none` cannot pass. + test('a dirty submodule worktree with an unchanged gitlink still reports nothing_to_commit', () => { + bumpedSubmodule(); + gitOrThrow(['add', '--', 'sub'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'bump sub'], { cwd: tmpDir }); + fs.appendFileSync(path.join(tmpDir, 'sub', 'f.txt'), 'dirty\n'); + + assert.strictEqual(commitFiles('sub').reason, 'nothing_to_commit', + 'nothing would land, so nothing_to_commit is the correct answer, not a misreport'); + }); + + // No submodule involved. A textconv driver maps two different blobs to the + // same text, so `git diff --quiet HEAD` reports no difference while + // `git commit -- ` records the new blob. + test('a change hidden by a textconv driver is not reported as nothing_to_commit', () => { + const rel = path.posix.join('.planning', 'binaryish.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'A\n'); + fs.writeFileSync(path.join(tmpDir, '.gitattributes'), 'binaryish.md diff=flat\n'); + gitOrThrow(['add', '--', rel, '.gitattributes'], { cwd: tmpDir }); + gitOrThrow(['commit', '-m', 'seed'], { cwd: tmpDir }); + // A textconv that collapses every input to one constant. `#` swallows the + // filename git appends, so the driver ignores its argument entirely. + gitOrThrow(['config', 'diff.flat.textconv', 'echo CONSTANT #'], { cwd: tmpDir }); + + fs.writeFileSync(path.join(tmpDir, rel), 'B\n'); + const output = commitFiles(rel); + + assert.notStrictEqual(output.reason, 'nothing_to_commit', + 'the blob changed and the commit would record it — textconv only changes how the DIFF renders'); + assert.strictEqual(output.committed, true); + assert.strictEqual( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir }), 'B\n', + 'the new content must actually be recorded'); + }); +}); + + describe('#2279: map-codebase date stamp instructions overwrite existing dates', () => { const REPO_ROOT = path.join(__dirname, '..'); diff --git a/tests/commit-files-pathspec.test.cjs b/tests/commit-files-pathspec.test.cjs index 1538b48cc..86868e874 100644 --- a/tests/commit-files-pathspec.test.cjs +++ b/tests/commit-files-pathspec.test.cjs @@ -317,7 +317,7 @@ const LIB = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib'); * returning the parsed JSON result and the git argv list that was actually * executed (so "git commit never ran" is asserted directly, not inferred). */ -function commitWithFailingAdd({ cwd, files, failFor = [], stderr = 'fatal: injected staging failure', timeout = false, amend = false, gitVerb = 'add' }) { +function commitWithFailingAdd({ cwd, files, failFor = [], stderr = 'fatal: injected staging failure', timeout = false, amend = false, gitVerb = 'add', matchArg = null }) { const callsOut = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2608-')), 'calls.json'); // `timeout` is `false` | `true` (alias for `'posix'`) | `'posix'` | `'windows'` — // #3050: the shared isSpawnTimeout predicate only requires `error.code === @@ -333,11 +333,12 @@ const failFor = ${JSON.stringify(failFor)}; const stderrText = ${JSON.stringify(stderr)}; const timeoutShape = ${JSON.stringify(timeoutShape)}; const gitVerb = ${JSON.stringify(gitVerb)}; +const matchArg = ${JSON.stringify(matchArg)}; const real = projection.execGit; const calls = []; projection.execGit = (args, opts) => { calls.push(args); - if (args[0] === gitVerb && failFor.includes(args[args.length - 1])) { + if (args[0] === gitVerb && (matchArg === null || args.includes(matchArg)) && failFor.includes(args[args.length - 1])) { if (timeoutShape === 'posix') { // The exact shape spawnSync produces on a POSIX timeout, which // shell-command-projection surfaces as signal + error.code. @@ -779,6 +780,386 @@ describe('#2608: commit --files fails closed when git add fails', () => { }); }); +describe('#3859: an unanswered sequencer probe must not open the empty-diff guard', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // `execGit` reports a spawn timeout as `exitCode: 1` (`_spawnResult`: + // `result.status ?? 1`) — byte-identical to the code `rev-parse --verify` + // returns for a ref that does not exist. So "the probe says no merge" and + // "the probe never answered" are the same value, and the #3776 guard read + // both as "no merge". These arms pin the conservative reading. + // + // The seam is `commitWithFailingAdd`'s injected `execGit` with + // `gitVerb: 'rev-parse'`: `args[0]` is `rev-parse` and `args[args.length-1]` + // is the ref, so `failFor: ['MERGE_HEAD']` selects exactly that one probe and + // leaves every other git call real. + + // Leaves a conflicted merge or cherry-pick in progress with `bystander` + // committed and unmodified — the empty-diff shape that reaches the guard. + const BYSTANDER = path.posix.join('.planning', 'bystander.md'); + function conflictedSequence(kind) { + const shared = path.posix.join('.planning', 'shared.md'); + fs.writeFileSync(path.join(tmpDir, shared), 'base\n'); + fs.writeFileSync(path.join(tmpDir, BYSTANDER), 'bystander\n'); + gitOrThrow(['add', '--', shared, BYSTANDER], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'shared base'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + const trunk = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }).trim(); + gitOrThrow(['checkout', '-b', 'side'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + fs.writeFileSync(path.join(tmpDir, shared), 'side\n'); + gitOrThrow(['commit', '-am', 'side edit'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['checkout', trunk], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + fs.writeFileSync(path.join(tmpDir, shared), 'trunk\n'); + gitOrThrow(['commit', '-am', 'trunk edit'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + // Deliberately conflicting — that is what leaves the sequencer ref behind. + spawnSync('git', [kind === 'merge' ? 'merge' : 'cherry-pick', 'side'], { + cwd: tmpDir, encoding: 'utf8', timeout: STAGING_GIT_TIMEOUT_MS, + }); + fs.writeFileSync(path.join(tmpDir, shared), 'resolved\n'); + gitOrThrow(['add', '--', shared], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + } + + for (const shape of ['posix', 'windows']) { + test(`a timed-out MERGE_HEAD probe (${shape}) reaches git instead of silently abandoning the merge`, () => { + conflictedSequence('merge'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'fixture must leave a merge in progress'); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [BYSTANDER], + failFor: ['MERGE_HEAD'], + gitVerb: 'rev-parse', + timeout: shape, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'an unanswered merge probe must not be read as "no merge in progress" — doing so decides ' + + 'nothing_to_commit from a pathspec git will not honour and leaves the merge unconcluded'); + assert.equal(result.reason, 'commit_failed', + 'git must be the one to refuse the partial commit, loudly, as it did before #3776'); + assert.ok(gitCalls.some((a) => a[0] === 'commit'), + 'the guard must fall through to git commit rather than returning early'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'MERGE_HEAD')), + 'the merge must still be in progress — silently abandoning it is the defect'); + }); + } + + test('a timed-out CHERRY_PICK_HEAD probe reaches git too', () => { + conflictedSequence('cherry-pick'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'CHERRY_PICK_HEAD')), + 'fixture must leave a cherry-pick in progress'); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [BYSTANDER], + failFor: ['CHERRY_PICK_HEAD'], + gitVerb: 'rev-parse', + timeout: true, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'the cherry-pick probe carries the identical conflation — #3776 added it, so it is in scope here'); + assert.equal(result.reason, 'commit_failed'); + assert.ok(gitCalls.some((a) => a[0] === 'commit')); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', 'CHERRY_PICK_HEAD')), + 'the cherry-pick must still be in progress'); + }); + + // THE ADAPTATION, PINNED. The obvious implementation — treat the timeout as + // `isMergeInProgress` — also flips `canScope`, which is PRE-EXISTING and + // gates the pathspec. A spurious timeout would then turn a scoped commit into + // a bare one and record whatever else happened to be staged, under a message + // describing only the named file: #2112, reintroduced by the fix for a + // misreport. The timeout must reach `partialCommitRefused` and nothing else. + test('a timed-out MERGE_HEAD probe outside a merge still commits ONLY the named paths', () => { + const named = path.posix.join('.planning', 'named.md'); + const unrelated = path.posix.join('.planning', 'unrelated.md'); + fs.writeFileSync(path.join(tmpDir, named), 'seed\n'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'seed\n'); + gitOrThrow(['add', '--', named, unrelated], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + // A real change to the named file, and an UNRELATED file sitting staged in + // the index — the #2112 shape a bare commit would sweep up. + fs.writeFileSync(path.join(tmpDir, named), 'named edit\n'); + fs.writeFileSync(path.join(tmpDir, unrelated), 'unrelated edit\n'); + gitOrThrow(['add', '--', unrelated], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [named], + failFor: ['MERGE_HEAD'], + gitVerb: 'rev-parse', + timeout: true, + }); + + assert.equal(result.committed, true, 'the commit must still happen — there is a real diff'); + const commitCall = gitCalls.find((a) => a[0] === 'commit'); + assert.ok(commitCall, 'git commit must have run'); + assert.ok(commitCall.includes('--') && commitCall.includes(named), + 'the pathspec must survive the timeout: routing it through isMergeInProgress would drop it'); + assert.deepEqual(committedFiles(tmpDir), [named], + 'only the named path may land — the staged unrelated file must not be swept in (#2112)'); + }); + + // NEGATIVE CONTROL for the conservative reading: outside a merge, treating an + // unanswered probe as "refused" must not manufacture a DIFFERENT answer. It + // falls through to git, git says there is nothing to commit, and the caller + // sees the same reason it would have seen anyway. + test('a timed-out MERGE_HEAD probe on a genuinely empty diff still reports nothing_to_commit', () => { + const rel = path.posix.join('.planning', 'quiet.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'unchanged\n'); + gitOrThrow(['add', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'quiet'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: ['MERGE_HEAD'], + gitVerb: 'rev-parse', + timeout: true, + }); + + assert.equal(result.reason, 'nothing_to_commit', + 'the conservative reading defers to git, which reports the same thing the guard would have'); + assert.ok(gitCalls.some((a) => a[0] === 'commit'), + 'and it gets there by asking git, not by short-circuiting on an unanswered probe'); + }); + + // Round 4, review finding 1 + its coverage half. The `stagedPaths.length === 0` + // disjunct is NOT gated on `partialCommitRefused`, so an all-missing `--files` + // list during a merge or cherry-pick returns `nothing_to_commit` while the + // sequencer ref is still live and the resolved content sits staged. + // + // These arms pin that as the DELIBERATE answer, not an oversight. Gating the + // disjunct sends this case to a BARE `git commit` (stagedPaths is empty, so + // `canScope` is false), which git permits during a merge and which CONCLUDES + // it — recording the whole index under a message naming a path that does not + // exist, and reporting `committed: true`. Trading a report that writes nothing + // for one that silently writes everything is the trade the timeout routing + // above already refuses. + // + // The behaviour is also pre-existing: before this fix the identical + // short-circuit ran ABOVE the MERGE_HEAD probe, so it never consulted the + // sequencer either. Nothing here is a regression pin; these are behaviour + // pins, and they red on the gated implementation rather than at base. + for (const kind of ['merge', 'cherry-pick']) { + const ref = kind === 'merge' ? 'MERGE_HEAD' : 'CHERRY_PICK_HEAD'; + test(`all named --files paths missing during a ${kind} reports nothing_to_commit and leaves the ${kind} open`, () => { + conflictedSequence(kind); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', ref)), + `fixture must leave a ${kind} in progress`); + const before = headCount(tmpDir); + const missing = path.posix.join('.planning', 'never-produced.md'); + assert.ok(!fs.existsSync(path.join(tmpDir, missing)), + 'the named path must genuinely be absent from disk'); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [missing], + failFor: [], + }); + + assert.equal(result.reason, 'nothing_to_commit', + 'every named path was skipped before git add (#2014), so there is nothing declared to commit'); + assert.equal(result.committed, false); + assert.ok(!gitCalls.some((a) => a[0] === 'commit'), + 'git commit must NOT run — a bare commit here would conclude the sequencer with the whole index'); + assert.equal(headCount(tmpDir), before, + 'and no commit may be recorded'); + assert.ok(fs.existsSync(path.join(tmpDir, '.git', ref)), + `the ${kind} must still be in progress — the caller's own resolution is untouched`); + assert.equal( + gitOrThrow(['diff', '--cached', '--name-only'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }).trim(), + path.posix.join('.planning', 'shared.md'), + 'and the staged resolution is still staged, not swallowed'); + }); + } +}); + + +describe('#3859: an unanswered dry-run probe must not close the assume-unchanged path', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // Round 3. The `git commit --dry-run --porcelain` probe is the ONE probe in + // the guard whose rc 0 is the reassuring answer ("git would record + // something, stand aside"). `execGit` reports a spawn timeout as + // `exitCode: 1` (`_spawnResult`: `result.status ?? 1`) — byte-identical to + // git's own "nothing to record" — so an unanswered probe read as a + // confirmed one, the guard returned `nothing_to_commit`, and content the + // caller named in `--files` was never committed. The diff probe is safe by + // construction (rc 0 is the DANGEROUS answer there, and a timeout can only + // produce non-zero); the two sequencer probes carry an explicit + // `isSpawnTimeout` disjunct. This probe now stands aside on anything but a + // CONFIRMED rc 1 with no spawn error — the same "only a confirmed answer + // decides" rule the diff probe already follows, from the other direction. + // + // Seam: `commitWithFailingAdd`'s injected `execGit` with `gitVerb: 'commit'` + // AND `matchArg: '--dry-run'` — the real `git commit … -- ` shares + // both `args[0]` and its last argument with the probe, so the verb alone + // would intercept the commit this arm exists to prove still happens. + function assumeUnchangedModified() { + const rel = path.posix.join('.planning', 'assumed.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'seed\n'); + gitOrThrow(['add', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed assumed'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + fs.writeFileSync(path.join(tmpDir, rel), 'modified under assume-unchanged\n'); + return rel; + } + + for (const shape of ['posix', 'windows']) { + test(`a timed-out dry-run probe (${shape}) still commits the modified assume-unchanged path`, () => { + const rel = assumeUnchangedModified(); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'commit', + matchArg: '--dry-run', + timeout: shape, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'an unanswered dry run must not be read as "nothing would be recorded" — that drops content ' + + 'the caller named in --files and reports there was nothing to write'); + assert.equal(result.committed, true, 'the commit must still happen — git would have recorded it'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && a.includes('--dry-run')), + 'the probe must have been the call that timed out'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && !a.includes('--dry-run')), + 'and the guard must fall through to the real git commit'); + assert.equal( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }), + 'modified under assume-unchanged\n', + 'and the content it records must be the working-tree content'); + }); + } + + // A genuine git error from the dry run (rc 128, no spawn error) is not a + // "nothing to record" answer either. It falls through to git, which fails + // the same way it would have — loudly — rather than manufacturing a no-op. + test('a dry-run probe that errors (rc 128) still reaches git instead of reporting nothing_to_commit', () => { + const rel = assumeUnchangedModified(); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'commit', + matchArg: '--dry-run', + timeout: false, + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'rc 128 is a failed probe, not a confirmed empty one'); + assert.equal(result.committed, true); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && !a.includes('--dry-run'))); + }); + + // The `ls-files -v` read is an optimisation, never a gate: when it cannot + // answer, the dry run runs anyway. Pinned, because the reviewer named it + // as untested and a later "tidy-up" that returns false on a failed read + // would drop the content by a different door. + test('a timed-out ls-files probe falls through to the dry run, and the path is still committed', () => { + const rel = assumeUnchangedModified(); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'ls-files', + timeout: true, + }); + + assert.equal(result.committed, true, + 'an unreadable tag list must not decide anything — the dry run answers instead'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && a.includes('--dry-run')), + 'the dry run must have run despite the unanswered ls-files read'); + assert.equal( + gitOrThrow(['show', 'HEAD:' + rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }), + 'modified under assume-unchanged\n'); + }); + + // NEGATIVE CONTROL for the conservative reading, mirroring the sequencer + // arms': an UNMODIFIED assume-unchanged path in a clean tree, dry run timed + // out. Standing aside must not manufacture a DIFFERENT answer — it falls + // through to git, git says there is nothing to commit, and the caller sees + // the same reason the guard would have given. + test('a timed-out dry-run probe on an unmodified assume-unchanged path still reports nothing_to_commit', () => { + const rel = path.posix.join('.planning', 'assumed.md'); + fs.writeFileSync(path.join(tmpDir, rel), 'seed\n'); + gitOrThrow(['add', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['commit', '-m', 'seed assumed'], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + gitOrThrow(['update-index', '--assume-unchanged', '--', rel], { cwd: tmpDir, timeoutMs: STAGING_GIT_TIMEOUT_MS }); + + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [rel], + gitVerb: 'commit', + matchArg: '--dry-run', + timeout: true, + }); + + assert.equal(result.reason, 'nothing_to_commit', + 'the conservative reading defers to git, which reports the same thing the probe would have'); + assert.ok(gitCalls.some((a) => a[0] === 'commit' && !a.includes('--dry-run')), + 'and it gets there by asking git, not by short-circuiting on an unanswered probe'); + }); + + // Round 4, review finding 3. The probe's safety rests on `git commit + // --dry-run` not running `pre-commit`, which git 2.54 satisfies on its own — + // so this arm pins the FLAG, not an outcome, and that is deliberate: the + // outcome it protects is unobservable on a git that already declines to run + // the hook. On a git that DID run it, a rejecting hook exits 1, the closure + // reads that as a confirmed "nothing to record", and the caller's content is + // dropped under a `nothing_to_commit` report — #3776 re-entered through the + // probe. `--no-verify` removes the dependency on the version rather than + // documenting it. + // + // It is a seam assertion over the argv the guard actually issued, not a + // source grep: the flag is read off the executed call, so deleting it from + // the probe reds this arm. + test('the dry-run probe carries --no-verify so a hook-firing git cannot close the guard', () => { + const rel = assumeUnchangedModified(); + + const { gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: [rel], + failFor: [], + }); + + const dryRuns = gitCalls.filter((a) => a[0] === 'commit' && a.includes('--dry-run')); + assert.equal(dryRuns.length, 1, + 'the assume-unchanged branch must have reached the dry-run probe exactly once'); + assert.ok(dryRuns[0].includes('--no-verify'), + 'the probe must not be able to execute a pre-commit hook, whatever the git version does by default'); + // And the REAL commit must not inherit it — #3776 is a bug about a hook + // whose message reached the caller wrongly, never a licence to skip hooks. + const realCommits = gitCalls.filter((a) => a[0] === 'commit' && !a.includes('--dry-run')); + assert.ok(realCommits.length > 0, 'the guard must have fallen through to a real commit'); + assert.ok(realCommits.every((a) => !a.includes('--no-verify')), + 'the real commit still runs the caller\'s hooks — only the probe is exempt'); + }); +}); + describe('workflow call sites declare --files (#2269)', () => { // WHAT COUNTS AS AN INVOCATION — the question this scan kept answering by // proxy, and kept getting wrong in both directions at once.