Files
msd-core/tests/commit-files-pathspec.test.cjs
Tom Boucher 28e486faf7 fix(#2608): fail closed when git add fails during commit staging (#2693)
* fix(#2608): fail closed when `git add` fails during commit staging

`cmdCommit` ignored `git add` failures. #2523 had already stopped a failed path
entering the commit pathspec, but skipping it silently left two bad outcomes,
both reproduced against the pre-fix build:

- SOME paths fail  -> `{"committed":true}`. `git commit` still ran and PARTIALLY
  committed the subset that happened to stage, under a message describing the
  full requested scope.
- EVERY path fails -> `{"reason":"nothing_to_commit"}`, which is not what
  happened and points the operator nowhere.

In both cases git's original `add` stderr was discarded, so the user saw a
downstream `commit_failed` / pathspec error naming an innocent file — the
symptom reported in the issue from a linked worktree whose git directory was
outside the managed writable root.

Staging failures are now collected and the command fails closed BEFORE
`git commit` runs, returning the issue's specified shape:

  { committed: false, hash: null, reason: "staging_failed",
    file: "<first failing path>", error: "<original git add stderr>",
    failures: [ { file, error, timed_out }, ... ] }

A timeout is distinguished as `staging_timeout` (issue AC5) using the projection's
SIGTERM+ETIMEDOUT signal — the same idiom worktree-safety.cts uses. The check is
placed ahead of the `nothing_to_commit` branch so an all-paths-failed run reports
the staging cause rather than an empty changeset.

Unchanged: successful staging still commits exactly the declared scope and leaves
unrelated staged files alone; an explicitly-named file that does not exist is
still skipped rather than staged as a deletion (#2014/#2523), and a request where
every named file is missing still reports `nothing_to_commit` — no `git add` ran,
so there is no staging failure to report.

Regression tests inject the failure by monkeypatching `execGit` on the projection
module (per CLAUDE.md, over `chmod 0o000`, which does not fault under root and
would make the tests vacuous), driven in a `node -e` child because `output()`
writes via `fs.writeSync(1, …)` and cannot be captured in-process. Pre-fix, 6 of
the 10 assertions fail; post-fix all pass.

Closes #2608

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf

* fix(#2608): roll back the index, guard the sibling surfaces, document the new reasons

Six findings from the orthogonal review of the first commit, all fixed here.

1. A `staging_failed` return left the index PARTIALLY STAGED. The paths that did
   stage stayed in the index with no commit made and no cleanup, so the next bare
   `git commit` would sweep them up — the same silent partial commit this fix
   exists to prevent, deferred one step. (Pre-fix the partial state at least got
   consumed by the incorrect commit.) The staging failure path now resets the
   paths it staged, matching cmdPrSubrepo's established rollback-then-error
   convention. The reset is scoped to what THIS call staged — paths the caller
   had already staged are captured up front and excluded, so a caller's own work
   is never destroyed — and is best-effort, since an unwritable index (the very
   failure being reported) cannot be reset either.

2. `cmdCommitToSubrepo` still had the identical defect: a failed `git add` was
   dropped silently and the function committed the subset that happened to stage,
   discarding git's stderr. It now fails closed per sub-repo with the same
   staging_failed/staging_timeout reasons and the same scoped rollback.

3. The `git rm --cached --ignore-unmatch` branch (default mode, for a planning
   file that no longer exists on disk) still discarded its result. It mutates the
   index exactly like `git add`, and `--ignore-unmatch` already makes "no such
   path" a success, so a non-zero exit there is a real I/O failure — now routed
   through the same staging-failure path.

4. `agents/gsd-executor.md` documented the commit envelope as an exhaustive
   three-shape enum and pattern-matched only `nothing_to_commit | commit_failed`.
   It is the sole consumer doc for this surface, so the new reasons are added
   with explicit guidance not to retry (a retry hits the same unwritable index),
   and the "one of three shapes" framing is corrected.

5. The default (non---files) staging path and `--amend` are now covered by tests.
   Both were already guarded by the first commit but unexercised.

6. The changeset framed the fix as `--files`-only; it applies to default and
   sub-repo commits too, and now mentions the rollback.

Regenerated the agent size baseline and the 18 golden install-parity fixtures for
the gsd-executor.md edit.

16 assertions across both surfaces verified against the built lib.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf

* test(#2608): update the #2523 out-of-repo contract to the new staging_failed reason

The remote test run surfaced this: `#2523: out-of-repo --files path is rejected by
git` asserted `reason: 'nothing_to_commit'`, and now gets `staging_failed`.

This is a deliberate contract improvement, not a papered-over failure. The old
reason existed only because a failed `git add` was skipped and the resulting empty
`stagedPaths` fell through to the empty-changeset branch. But "nothing to commit"
is not what happened — the caller named a file and git refused it — and that
misreport is exactly the class of defect #2608 closes. The result now carries the
offending path and git's own message ("… is outside repository at …"), which is
strictly more actionable for the same condition.

#2523's two substantive invariants are untouched and still asserted: no commit is
created, and the index is left clean. Two assertions are ADDED (the path is named,
git's message is preserved) so the richer contract is pinned rather than merely
allowed.

Per CONTRIBUTING, a stale-test correction rides its own commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf

* fix(#2608): compact the executor doc addition to stay under the agent LARGE cap

The remote test run failed: `gsd-executor.md is 49217 bytes — exceeds the LARGE
hard cap of 49152`. The file was already at 48596 (556 bytes of headroom) and the
new commit-envelope documentation pushed it 65 bytes over.

The cap is a red line, not a budget to raise, so the addition is compacted rather
than the cap moved: four lines instead of eight, keeping the load-bearing facts —
the two new reasons, that nothing was committed and the index was rolled back,
that `file` + `error` should be surfaced, and that retrying is wrong because a
retry hits the same cause. Dropped only the restatement of the linked-worktree
example (already in the changeset and PR) and the `failures[]` field (a superset
of `file`/`error`, discoverable from the payload).

Net addition is now 276 bytes; the file sits at 48872 with 280 bytes of headroom.
Extracting the agent's shared boilerplate to references/ would buy much more, but
that is a restructuring of the executor agent and does not belong in a
commit-staging bugfix.

Agent size baseline and the golden install-parity fixtures regenerated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf

* chore(#2608): backfill changeset PR number (#2693)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-27 02:45:35 -04:00

253 lines
11 KiB
JavaScript

/**
* Regression test for #2112: gsd-tools commit --files commits the entire
* index, not the declared paths.
*
* `cmdCommit` staged exactly the files named in --files but then ran a bare
* `git commit` with no pathspec, absorbing anything else that happened to be
* staged into a commit whose message described only the named files.
*
* The fix adds `'--', ...stagedPaths` to the commit args **only when** the
* caller declared a scope (explicitFiles), and only for paths that were
* actually staged (skipped missing files are excluded to avoid #2014).
*/
const { describe, test, beforeEach, afterEach } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const path = require('path');
const { execSync } = require('child_process');
const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs');
describe('commit --files: pathspec honors declared scope (#2112)', () => {
let tmpDir;
beforeEach(() => {
tmpDir = createTempGitProject();
});
afterEach(() => {
cleanup(tmpDir);
});
test('commit --files does not absorb unrelated staged files', () => {
// Developer stages a WIP file via git add (not via --files).
fs.writeFileSync(path.join(tmpDir, 'src-wip.txt'), 'work in progress\n');
execSync('git add src-wip.txt', { cwd: tmpDir, stdio: 'pipe' });
// GSD writes and commits a planning artifact, naming ONLY that file.
fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n');
runGsdTools(
['commit', 'docs(01): add PLAN.md', '--files', '.planning/PLAN.md'],
tmpDir,
);
// The commit must contain ONLY .planning/PLAN.md.
const diffOutput = execSync('git diff HEAD~1 HEAD --name-only', {
cwd: tmpDir,
encoding: 'utf-8',
}).trim();
assert.strictEqual(
diffOutput,
'.planning/PLAN.md',
'commit --files must contain only the named files, got:\n' + diffOutput,
);
// The WIP file must still be staged, not committed.
const statusOutput = execSync('git status --porcelain', {
cwd: tmpDir,
encoding: 'utf-8',
}).trim();
assert.ok(
statusOutput.includes('A src-wip.txt') || statusOutput.includes('A\tsrc-wip.txt'),
'src-wip.txt should remain staged, not committed. Status:\n' + statusOutput,
);
});
test('commit --files with two files commits exactly those two', () => {
fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n');
fs.writeFileSync(path.join(tmpDir, '.planning', 'RESEARCH.md'), '# Research\n');
runGsdTools(
['commit', 'docs: artifacts', '--files', '.planning/PLAN.md', '.planning/RESEARCH.md'],
tmpDir,
);
const diffOutput = execSync('git diff HEAD~1 HEAD --name-only', {
cwd: tmpDir,
encoding: 'utf-8',
});
const files = diffOutput.trim().split('\n').sort();
assert.deepEqual(
files,
['.planning/PLAN.md', '.planning/RESEARCH.md'],
'commit should contain exactly the two named files',
);
});
test('commit without --files still commits the entire .planning/ index (default path)', () => {
// Write a planning artifact and stage it.
fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n');
execSync('git add .planning/PLAN.md', { cwd: tmpDir, stdio: 'pipe' });
// Also stage an unrelated file.
fs.writeFileSync(path.join(tmpDir, 'extra.txt'), 'extra\n');
execSync('git add extra.txt', { cwd: tmpDir, stdio: 'pipe' });
runGsdTools(['commit', 'docs: default commit'], tmpDir);
// Default path (no --files) commits everything staged.
const diffOutput = execSync('git diff HEAD~1 HEAD --name-only', {
cwd: tmpDir,
encoding: 'utf-8',
});
const files = diffOutput.trim().split('\n').sort();
assert.ok(
files.includes('.planning/PLAN.md') && files.includes('extra.txt'),
'default commit (no --files) should commit everything staged, got:\n' + files,
);
});
test('missing tracked file in --files is still not committed as deletion (#2014 guard)', () => {
// Create and commit STATE.md, then remove it from disk.
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n');
execSync('git add .planning/STATE.md', { cwd: tmpDir, stdio: 'pipe' });
execSync('git commit -m "add STATE.md"', { cwd: tmpDir, stdio: 'pipe' });
fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md'));
// Also create a valid file to commit.
fs.writeFileSync(path.join(tmpDir, '.planning', 'PLAN.md'), '# Plan\n');
runGsdTools(
['commit', 'docs: add plan', '--files', '.planning/PLAN.md', '.planning/STATE.md'],
tmpDir,
);
const diffOutput = execSync('git diff HEAD~1 HEAD --name-status', {
cwd: tmpDir,
encoding: 'utf-8',
});
assert.ok(
!diffOutput.includes('D\t.planning/STATE.md'),
'missing tracked file must not appear as a deletion, diff was:\n' + diffOutput,
);
assert.ok(
diffOutput.includes('.planning/PLAN.md'),
'PLAN.md should be committed',
);
});
test('commit --files with only missing files returns nothing_to_commit', () => {
// Create and commit STATE.md, then remove it from disk.
fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State\n');
execSync('git add .planning/STATE.md', { cwd: tmpDir, stdio: 'pipe' });
execSync('git commit -m "add STATE.md"', { cwd: tmpDir, stdio: 'pipe' });
fs.unlinkSync(path.join(tmpDir, '.planning', 'STATE.md'));
// Stage an unrelated file so the index is non-empty.
fs.writeFileSync(path.join(tmpDir, 'extra.txt'), 'extra\n');
execSync('git add extra.txt', { cwd: tmpDir, stdio: 'pipe' });
const result = runGsdTools(
['commit', 'docs: try', '--files', '.planning/STATE.md'],
tmpDir,
);
const parsed = JSON.parse(result.output);
assert.strictEqual(
parsed.committed, false,
'should not commit when all --files are missing',
);
assert.strictEqual(
parsed.reason, 'nothing_to_commit',
'should report nothing_to_commit, not absorb the index',
);
// The unrelated staged file must still be staged, not committed.
const statusOutput = execSync('git status --porcelain', {
cwd: tmpDir,
encoding: 'utf-8',
}).trim();
assert.ok(
statusOutput.includes('extra.txt'),
'extra.txt should remain staged, not absorbed into a commit',
);
});
test('#2523: absolute --files path inside the repo is committed, not silently dropped', () => {
// init phase-op emits phase_dir as an ABSOLUTE path (#2428); cmdCommit must
// accept it. The bug was path.join(cwd, absPath) → cwd+absPath (non-existent)
// → silently skipped as nothing_to_commit (#2523).
fs.writeFileSync(path.join(tmpDir, '.planning', 'A.md'), 'a\n');
const absPath = path.join(tmpDir, '.planning', 'A.md');
const res = runGsdTools(['commit', 'docs: abs path', '--files', absPath], tmpDir);
const parsed = JSON.parse(res.output);
assert.strictEqual(parsed.committed, true, `absolute path must commit, not nothing_to_commit: ${res.output}`);
// The absolute path must land in the commit, normalized to repo-relative.
const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' }).trim();
assert.strictEqual(diff, '.planning/A.md', `absolute --files path must be committed (normalized to relative); got: ${diff}`);
});
test('#2523: mixed relative+absolute --files list commits BOTH (no silent partial commit)', () => {
// The sharpest symptom: a mixed list committed the relative entry, dropped the
// absolute one, and reported committed:true (#2523). Both must land.
fs.writeFileSync(path.join(tmpDir, '.planning', 'REL.md'), 'r\n');
fs.writeFileSync(path.join(tmpDir, '.planning', 'ABS.md'), 'a\n');
const absPath = path.join(tmpDir, '.planning', 'ABS.md');
const res = runGsdTools(
['commit', 'docs: mixed', '--files', '.planning/REL.md', absPath],
tmpDir,
);
const parsed = JSON.parse(res.output);
assert.strictEqual(parsed.committed, true, `mixed list must commit: ${res.output}`);
const diff = execSync('git diff HEAD~1 HEAD --name-only', { cwd: tmpDir, encoding: 'utf-8' })
.trim().split('\n').sort();
assert.deepStrictEqual(
diff,
['.planning/ABS.md', '.planning/REL.md'],
`mixed relative+absolute list must commit BOTH entries (the bug dropped the absolute one); got: ${diff.join(',')}`,
);
});
test('#2523: out-of-repo --files path is rejected by git (staging_failed), no index pollution', (t) => {
// An absolute path resolving OUTSIDE the project root: git add rejects it. No
// index pollution (#2523). Not "path_outside_repo" (that guard was removed for
// macOS symlink compatibility — git's own rejection suffices).
//
// #2608 changed the REASON this reports, deliberately. It used to be
// `nothing_to_commit`, because a failed `git add` was skipped and the empty
// stagedPaths list fell through to the empty-changeset branch. But "nothing to
// commit" is not what happened — the caller named a file and git refused it —
// and that misreport is the very class of defect #2608 closes. The result now
// carries `staging_failed` plus the offending path and git's own message
// ("… is outside repository at …"), which is strictly more actionable.
//
// #2523's two substantive invariants are unchanged and still asserted below:
// no commit is created, and the index is left clean.
const outsideDir = path.join(tmpDir, '..', `gsd-2523-outside-${process.pid}-${Date.now()}`);
fs.mkdirSync(outsideDir, { recursive: true });
t.after(() => cleanup(outsideDir));
const outsideFile = path.join(outsideDir, 'secret.md');
fs.writeFileSync(outsideFile, 's\n');
const res = runGsdTools(
['commit', 'docs: outside', '--files', path.resolve(outsideFile)],
tmpDir,
);
const parsed = JSON.parse(res.output);
assert.strictEqual(parsed.committed, false, 'out-of-repo path must not commit');
assert.strictEqual(parsed.reason, 'staging_failed', `out-of-repo: git rejects → staging_failed (#2608): ${res.output}`);
assert.strictEqual(parsed.file, path.resolve(outsideFile), 'the rejected path must be named');
assert.match(parsed.error, /outside repository/, "git's own rejection message must be preserved (#2608)");
// No new commit created (still at the single initial commit).
const logCount = execSync('git rev-list --count HEAD', { cwd: tmpDir, encoding: 'utf-8' }).trim();
assert.strictEqual(logCount, '1', 'no new commit must be created for an out-of-repo path');
// Index stays clean (git add failed → nothing staged).
const status = execSync('git status --porcelain', { cwd: tmpDir, encoding: 'utf-8' }).trim();
assert.strictEqual(status, '', `index must be clean (no pollution): ${status}`);
});
});