diff --git a/.changeset/silly-badgers-frolic.md b/.changeset/silly-badgers-frolic.md new file mode 100644 index 000000000..bfb0366bc --- /dev/null +++ b/.changeset/silly-badgers-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2693 +--- +**A commit whose `git add` fails now says so, instead of partially committing or reporting "nothing to commit"** — when staging failed (an unwritable index in a linked worktree, permissions, or a timeout), GSD discarded the error: a multi-file request silently committed only the paths that happened to stage, and a total failure surfaced as `nothing_to_commit` or a downstream pathspec error naming an innocent file. Staging failures are now collected and reported as `staging_failed` (or `staging_timeout`) with the offending file and git's original stderr, before any commit is attempted, and the index is rolled back to its prior state. Applies to scoped (`--files`) commits, default `.planning/` commits, and sub-repo commits alike. (#2608) diff --git a/agents/gsd-executor.md b/agents/gsd-executor.md index ce827d567..58b741b3a 100644 --- a/agents/gsd-executor.md +++ b/agents/gsd-executor.md @@ -789,7 +789,7 @@ gsd_run query commit "docs({phase}-{plan}): complete [plan-name] plan" --files \ Separate from per-task commits — captures execution results only. **Handling the SDK return envelope (#3678):** `gsd-tools query commit` returns -one of three shapes: +one of these shapes: - `{committed: true, hash, reason: 'committed'}` — commit succeeded; record the hash in the completion format. @@ -802,6 +802,10 @@ one of three shapes: success path.** Record "skipped (.planning gitignored)" and move on. - `{committed: false, reason: 'nothing_to_commit' | 'commit_failed', ...}` — no-op / genuine failure; surface in the completion notes. +- `{committed: false, reason: 'staging_failed' | 'staging_timeout', file, error}` — + `git add` itself failed (#2608), e.g. an unwritable index. Nothing committed, + index rolled back. Surface `file` + `error` (git's stderr); do not retry — a + retry hits the same cause. **Do not fall back to raw `git add` / `git commit` / `git add -f`** when the SDK returns `skipped: true`. The SDK's skip is the user's deliberate choice diff --git a/src/commands.cts b/src/commands.cts index 68efa923f..ca5ac6996 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -889,6 +889,21 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u const explicitFiles = files && files.length > 0; const filesToStage = explicitFiles ? files : ['.planning/']; const stagedPaths: string[] = []; + // #2608: a `git add` that fails must abort the commit, not be skipped. + // #2523 stopped a failed path entering the commit pathspec, but skipping it + // silently left two bad outcomes: a PARTIAL commit when only some requested + // paths failed, and a misleading `nothing_to_commit` when all of them did — + // in both cases the original staging error (permissions, unwritable index in + // a linked worktree, timeout) was discarded and the operator saw a downstream + // pathspec error pointing at an innocent file. + const stagingFailures: Array<{ file: string; error: string; timed_out: boolean }> = []; + // Paths already in the index BEFORE this call. On a staging failure the + // rollback below unstages only what THIS call added — unstaging a path the + // caller had staged themselves would destroy their work. + const preStaged = new Set( + execGit(['diff', '--cached', '--name-only'], { cwd }) + .stdout.split('\n').map(s => s.trim()).filter(Boolean), + ); for (const file of filesToStage) { const fullPath = path.resolve(cwd, file); if (!fs.existsSync(fullPath)) { @@ -900,7 +915,19 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u } // Default mode (staging all of .planning/): stage the deletion so // removed planning files are not left dangling in the index. - execGit(['rm', '--cached', '--ignore-unmatch', file], { cwd }); + // This mutates the index exactly like `git add` does, so it fails closed + // the same way — an unwritable index must not be swallowed here either. + // `--ignore-unmatch` already makes "no such path" a success, so a non-zero + // exit is a real I/O failure, not a missing file. + const rmResult = execGit(['rm', '--cached', '--ignore-unmatch', file], { cwd }); + if (rmResult.exitCode !== 0) { + const rmErr: NodeJS.ErrnoException | null = rmResult.error; + stagingFailures.push({ + file, + error: rmResult.stderr || rmResult.stdout, + timed_out: rmResult.signal === 'SIGTERM' && rmErr?.code === 'ETIMEDOUT', + }); + } } else { const addResult = execGit(['add', file], { cwd }); // Only record paths that actually staged — a failed `git add` (permissions, @@ -908,10 +935,54 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u // cmdCommitToSubrepo's exitCode-gated push. if (addResult.exitCode === 0) { stagedPaths.push(file); + } else { + // `SpawnResultOutput.error` is typed `Error | null`; widen to the errno + // shape by ANNOTATION rather than assertion — `Error` is assignable to + // `NodeJS.ErrnoException` (its extra fields are optional), so an `as` + // cast here trips no-unnecessary-type-assertion. + const addErr: NodeJS.ErrnoException | null = addResult.error; + stagingFailures.push({ + file, + error: addResult.stderr || addResult.stdout, + // The projection exposes a timeout distinctly (#2608 AC5); this is the + // same SIGTERM+ETIMEDOUT idiom worktree-safety.cts uses. + timed_out: addResult.signal === 'SIGTERM' && addErr?.code === 'ETIMEDOUT', + }); } } } + // #2608: fail closed before `git commit` runs. Checked ahead of the + // nothing_to_commit branch below so a run where EVERY path failed to stage + // reports the staging cause rather than "nothing to commit", and ahead of the + // commit itself so a multi-file scope never partially commits the subset that + // happened to stage. + if (stagingFailures.length > 0) { + // Fail closed AND clean. Without this the paths that DID stage stay in the + // index with no commit made, so the next bare `git commit` sweeps them up — + // the same silent partial commit this fix exists to prevent, deferred one + // step. Mirrors cmdPrSubrepo's rollback-then-error convention. Only paths + // this call staged are unstaged (preStaged is excluded), and the reset is + // best-effort: if the index is unwritable — the very failure being reported + // — the reset cannot succeed either, and the staging error is still what + // gets returned. + const toUnstage = stagedPaths.filter(p => !preStaged.has(p)); + if (toUnstage.length > 0) { + execGit(['reset', '-q', '--', ...toUnstage], { cwd }); + } + const first = stagingFailures[0]; + const result = { + committed: false, + hash: null, + reason: first.timed_out ? 'staging_timeout' : 'staging_failed', + file: first.file, + error: first.error, + failures: stagingFailures, + }; + output(result, raw, 'failed'); + return; + } + // Commit — when the caller declared a scope (--files), append a pathspec so // only the declared files land in the commit, not the entire index (#2112). // The pathspec uses stagedPaths (not filesToStage) so skipped missing files @@ -1037,14 +1108,46 @@ function cmdCommitToSubrepo(cwd: string, message: string | undefined, files: str const repoCwd = path.join(cwd, repo); // Stage files (strip sub-repo prefix for paths relative to that repo) + // #2608: this is the sub-repo twin of cmdCommit's staging loop and carried + // the identical defect — a failed `git add` was dropped silently and the + // function went straight on to commit the subset that happened to stage, + // discarding git's stderr. Fails closed per-repo, with the same rollback of + // only what this call staged. + const preStagedSub = new Set( + execGit(['diff', '--cached', '--name-only'], { cwd: repoCwd }) + .stdout.split('\n').map(s => s.trim()).filter(Boolean), + ); const stagedRelPaths: string[] = []; + const subStagingFailures: Array<{ file: string; error: string; timed_out: boolean }> = []; for (const file of repoFiles) { const relativePath = file.slice(repo.length + 1); const addResult = execGit(['add', relativePath], { cwd: repoCwd }); if (addResult.exitCode === 0) { stagedRelPaths.push(relativePath); + } else { + const addErr: NodeJS.ErrnoException | null = addResult.error; + subStagingFailures.push({ + file, + error: addResult.stderr || addResult.stdout, + timed_out: addResult.signal === 'SIGTERM' && addErr?.code === 'ETIMEDOUT', + }); } } + if (subStagingFailures.length > 0) { + const toUnstageSub = stagedRelPaths.filter(p => !preStagedSub.has(p)); + if (toUnstageSub.length > 0) { + execGit(['reset', '-q', '--', ...toUnstageSub], { cwd: repoCwd }); + } + const firstSub = subStagingFailures[0]; + repos[repo] = { + committed: false, + hash: null, + files: repoFiles, + reason: firstSub.timed_out ? 'staging_timeout' : 'staging_failed', + error: firstSub.error, + }; + continue; + } // Commit — pathspec limits the commit to the staged files only (#2112) const isMergeInProgressSub = execGit(['rev-parse', '-q', '--verify', 'MERGE_HEAD'], { cwd: repoCwd }).exitCode === 0; diff --git a/tests/agent-size-baseline.json b/tests/agent-size-baseline.json index 3f5aa8d7b..db1334fcf 100644 --- a/tests/agent-size-baseline.json +++ b/tests/agent-size-baseline.json @@ -14,7 +14,7 @@ "gsd-domain-researcher.md": 7032, "gsd-eval-auditor.md": 12496, "gsd-eval-planner.md": 7008, - "gsd-executor.md": 48596, + "gsd-executor.md": 48872, "gsd-framework-selector.md": 6778, "gsd-integration-checker.md": 15238, "gsd-intel-updater.md": 18166, diff --git a/tests/commit-files-pathspec.test.cjs b/tests/commit-files-pathspec.test.cjs index ff751ef1c..2dc81f09e 100644 --- a/tests/commit-files-pathspec.test.cjs +++ b/tests/commit-files-pathspec.test.cjs @@ -211,11 +211,21 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { ); }); - test('#2523: out-of-repo --files path is rejected by git (nothing_to_commit), no index pollution', (t) => { - // An absolute path resolving OUTSIDE the project root: git add rejects it → the - // gated stagedPaths.push (on git-add exitCode) skips it → nothing_to_commit. No + 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 — the gated push + git's own rejection suffice). + // 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)); @@ -228,7 +238,9 @@ describe('commit --files: pathspec honors declared scope (#2112)', () => { ); const parsed = JSON.parse(res.output); assert.strictEqual(parsed.committed, false, 'out-of-repo path must not commit'); - assert.strictEqual(parsed.reason, 'nothing_to_commit', `out-of-repo: git rejects → nothing_to_commit: ${res.output}`); + 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(); diff --git a/tests/fix-2608-commit-staging-failure.test.cjs b/tests/fix-2608-commit-staging-failure.test.cjs new file mode 100644 index 000000000..187f94f2d --- /dev/null +++ b/tests/fix-2608-commit-staging-failure.test.cjs @@ -0,0 +1,446 @@ +/** + * Regression tests for #2608 — `query commit --files` ignored `git add` failures + * and misreported them. + * + * A `git add` that fails (unwritable index in a linked worktree whose git dir is + * outside the managed writable root, permissions, timeout) was not surfaced. + * #2523 had already stopped a failed path entering the commit pathspec, but + * skipping it silently left two bad outcomes, both reproduced by this suite + * 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 the original `git add` stderr was discarded, so the user saw a + * downstream `commit_failed` / pathspec error naming an innocent file. + * + * The fix collects staging failures and fails closed BEFORE `git commit` runs, + * returning `staging_failed` (or `staging_timeout`) with the offending file and + * the original stderr preserved. + * + * ── INJECTION SEAM ──────────────────────────────────────────────────────────── + * `execGit` is monkeypatched on the shell-command-projection module object. The + * compiled call site is `(0, mod.execGit)(...)` — a property lookup at call time + * — so the override takes effect. Per CLAUDE.md this is required over + * `chmod 0o000` permission tricks, which do not fault under root (root + * Docker/CI) and would make these tests silently vacuous. + * + * The patched call runs in a short-lived `node -e` CHILD rather than in-process, + * for two reasons: `output()` writes with `fs.writeSync(1, …)`, which neither + * `process.stdout.write` nor `console.log` interception can capture; and a child + * keeps the patch from leaking into sibling suites. It is a plain + * `process.execPath` spawn — no PATH stub and no exec bit, so it is not subject + * to DEFECT.WINDOWS-TEST-PORTABILITY and runs on every platform. + */ + +'use strict'; + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { execFileSync, spawnSync } = require('node:child_process'); + +const { createTempGitProject, cleanup } = require('./helpers.cjs'); + +const LIB = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib'); + +/** + * Run cmdCommit with `git add ` forced to fail for the paths in `failFor`, + * 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 }) { + const callsOut = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2608-')), 'calls.json'); + const script = ` +const path = require('path'); +const LIB = ${JSON.stringify(LIB)}; +const projection = require(path.join(LIB, 'shell-command-projection.cjs')); +const { cmdCommit } = require(path.join(LIB, 'commands.cjs')); +const failFor = ${JSON.stringify(failFor)}; +const stderrText = ${JSON.stringify(stderr)}; +const timedOut = ${JSON.stringify(timeout)}; +const real = projection.execGit; +const calls = []; +projection.execGit = (args, opts) => { + calls.push(args); + if (args[0] === 'add' && failFor.includes(args[args.length - 1])) { + if (timedOut) { + // The exact shape spawnSync produces on a timeout, which + // shell-command-projection surfaces as signal + error.code. + const e = new Error('spawnSync git ETIMEDOUT'); + e.code = 'ETIMEDOUT'; + return { exitCode: 1, stdout: '', stderr: stderrText, signal: 'SIGTERM', error: e }; + } + return { exitCode: 128, stdout: '', stderr: stderrText, signal: null, error: null }; + } + return real(args, opts); +}; +process.on('exit', () => { + require('fs').writeFileSync(${JSON.stringify(callsOut)}, JSON.stringify(calls)); +}); +cmdCommit(${JSON.stringify(cwd)}, 'docs: map existing codebase', ${JSON.stringify(files)}, false, ${JSON.stringify(amend)}, false); +`; + + const run = spawnSync(process.execPath, ['-e', script], { + encoding: 'utf8', + timeout: 30000, + killSignal: 'SIGKILL', + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + + assert.ok( + run.stdout && run.stdout.trim(), + `cmdCommit child produced no stdout (status=${run.status}): ${run.stderr}`, + ); + return { + result: JSON.parse(run.stdout), + gitCalls: JSON.parse(fs.readFileSync(callsOut, 'utf8')), + }; +} + +function headCount(cwd) { + return Number(execFileSync('git', ['rev-list', '--count', 'HEAD'], { cwd, encoding: 'utf-8' }).trim()); +} + +function committedFiles(cwd) { + return execFileSync('git', ['diff', 'HEAD~1', 'HEAD', '--name-only'], { cwd, encoding: 'utf-8' }) + .trim().split('\n').filter(Boolean).sort(); +} + +/** + * Same harness for `cmdCommitToSubrepo` — the sub-repo twin of the staging loop, + * which carried the identical defect (failed `git add` dropped, commit proceeds + * with the subset that staged). + */ +function subrepoCommitWithFailingAdd({ cwd, files, failFor = [] }) { + const script = ` +const path = require('path'); +const LIB = ${JSON.stringify(LIB)}; +const projection = require(path.join(LIB, 'shell-command-projection.cjs')); +const { cmdCommitToSubrepo } = require(path.join(LIB, 'commands.cjs')); +const failFor = ${JSON.stringify(failFor)}; +const real = projection.execGit; +projection.execGit = (args, opts) => { + if (args[0] === 'add' && failFor.includes(args[args.length - 1])) { + return { exitCode: 128, stdout: '', stderr: 'fatal: injected subrepo staging failure', signal: null, error: null }; + } + return real(args, opts); +}; +cmdCommitToSubrepo(${JSON.stringify(cwd)}, 'feat: subrepo change', ${JSON.stringify(files)}, false); +`; + const run = spawnSync(process.execPath, ['-e', script], { + encoding: 'utf8', + timeout: 30000, + killSignal: 'SIGKILL', + env: { ...process.env, GSD_TEST_MODE: '1' }, + }); + assert.ok(run.stdout && run.stdout.trim(), + `cmdCommitToSubrepo child produced no stdout (status=${run.status}): ${run.stderr}`); + return JSON.parse(run.stdout); +} + +describe('#2608: commit-to-subrepo fails closed when git add fails', () => { + let rootDir; + let subDir; + + beforeEach(() => { + rootDir = createTempGitProject(); + fs.writeFileSync( + path.join(rootDir, '.planning', 'config.json'), + JSON.stringify({ planning: { sub_repos: ['backend'] } }, null, 2), + ); + subDir = path.join(rootDir, 'backend'); + fs.mkdirSync(subDir, { recursive: true }); + for (const [cmd, args] of [['init', []], ['config', ['user.email', 'test@example.com']], ['config', ['user.name', 'Test']]]) { + execFileSync('git', [cmd, ...args], { cwd: subDir, stdio: 'pipe' }); + } + fs.writeFileSync(path.join(subDir, 'seed.js'), '// seed\n'); + execFileSync('git', ['add', 'seed.js'], { cwd: subDir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'seed'], { cwd: subDir, stdio: 'pipe' }); + fs.writeFileSync(path.join(subDir, 'a.js'), '// a\n'); + fs.writeFileSync(path.join(subDir, 'b.js'), '// b\n'); + }); + + afterEach(() => { + cleanup(rootDir); + }); + + test('a failed sub-repo git add reports staging_failed and commits nothing', () => { + const before = headCount(subDir); + const result = subrepoCommitWithFailingAdd({ + cwd: rootDir, + files: ['backend/a.js', 'backend/b.js'], + failFor: ['b.js'], + }); + + assert.equal(result.repos.backend.reason, 'staging_failed', + `expected staging_failed for the sub-repo, got ${JSON.stringify(result)}`); + assert.equal(result.repos.backend.committed, false); + assert.match(result.repos.backend.error, /injected subrepo staging failure/, + "git's original stderr must be preserved"); + assert.equal(headCount(subDir), before, 'no partial sub-repo commit may be created'); + + const status = execFileSync('git', ['status', '--porcelain'], { cwd: subDir, encoding: 'utf-8' }); + assert.deepEqual(status.split('\n').filter((l) => /^A[ \t]/.test(l)), [], + `the sub-repo index must be rolled back, status:\n${status}`); + }); + + test('successful sub-repo staging still commits', () => { + const before = headCount(subDir); + const result = subrepoCommitWithFailingAdd({ + cwd: rootDir, + files: ['backend/a.js', 'backend/b.js'], + failFor: [], + }); + + assert.equal(result.repos.backend.committed, true, `expected a commit, got ${JSON.stringify(result)}`); + assert.equal(headCount(subDir), before + 1); + }); +}); + +describe('#2608: commit --files fails closed when git add fails', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + for (const name of ['ARCHITECTURE', 'CONCERNS', 'CONVENTIONS']) { + fs.writeFileSync(path.join(tmpDir, '.planning', `${name}.md`), `# ${name}\n`); + } + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + // ── AC1 + AC3: the failure is reported, with its original stderr ────────── + + test('a failed git add returns staging_failed with the file and original stderr', () => { + const before = headCount(tmpDir); + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md'], + failFor: ['.planning/ARCHITECTURE.md'], + stderr: 'fatal: Unable to create index.lock: Permission denied', + }); + + assert.equal(result.committed, false); + assert.equal(result.hash, null); + assert.equal(result.reason, 'staging_failed', + 'the staging cause must be reported, not a downstream commit_failed/pathspec error'); + assert.equal(result.file, '.planning/ARCHITECTURE.md', 'the offending file must be named'); + assert.match(result.error, /Unable to create index\.lock/, + 'the original git add stderr must be preserved'); + + // AC2: git commit must never have been invoked. + assert.ok( + !gitCalls.some((a) => a[0] === 'commit'), + `git commit must not run after a staging failure, calls: ${JSON.stringify(gitCalls)}`, + ); + assert.equal(headCount(tmpDir), before, 'no commit may be created'); + }); + + // ── AC4: no partial commit of a multi-file explicit scope ───────────────── + + test('when the second of three paths fails to stage, nothing is committed', () => { + // Pre-fix this returned {"committed":true} — the two paths that DID stage + // were committed under a message describing all three. + const before = headCount(tmpDir); + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md', '.planning/CONVENTIONS.md'], + failFor: ['.planning/CONCERNS.md'], + }); + + assert.equal(result.reason, 'staging_failed'); + assert.equal(result.file, '.planning/CONCERNS.md'); + assert.ok( + !gitCalls.some((a) => a[0] === 'commit'), + 'a partial commit of the paths that DID stage must not happen', + ); + assert.equal(headCount(tmpDir), before, 'no partial commit may be created'); + }); + + test('every failing path is reported, not just the first', () => { + const { result } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md', '.planning/CONVENTIONS.md'], + failFor: ['.planning/ARCHITECTURE.md', '.planning/CONVENTIONS.md'], + }); + + assert.equal(result.failures.length, 2); + assert.deepEqual( + result.failures.map((f) => f.file).sort(), + ['.planning/ARCHITECTURE.md', '.planning/CONVENTIONS.md'], + ); + }); + + // ── An all-paths-fail run must not masquerade as nothing_to_commit ──────── + + test('when every path fails to stage, the reason is staging_failed not nothing_to_commit', () => { + const { result } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md'], + failFor: ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md'], + }); + + assert.notEqual(result.reason, 'nothing_to_commit', + 'every path failing to stage is a staging failure, not an empty changeset'); + assert.equal(result.reason, 'staging_failed'); + }); + + // ── AC5: a staging timeout is distinguishable from an ordinary failure ──── + + test('a staging timeout is reported as staging_timeout, not staging_failed', () => { + const { result } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md'], + failFor: ['.planning/ARCHITECTURE.md'], + stderr: '', + timeout: true, + }); + + assert.equal(result.reason, 'staging_timeout', + 'the projection exposes SIGTERM+ETIMEDOUT; a timeout must not read as an ordinary failure'); + assert.equal(result.failures[0].timed_out, true); + }); + + test('an ordinary non-zero git add is NOT reported as a timeout', () => { + // Boundary: the timeout carve-out must not swallow the ordinary case. + const { result } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md'], + failFor: ['.planning/ARCHITECTURE.md'], + }); + + assert.equal(result.reason, 'staging_failed'); + assert.equal(result.failures[0].timed_out, false); + }); + + // ── Successful staging preserves the current scoped-commit behaviour ────── + + test('successful staging still commits exactly the declared scope', () => { + const before = headCount(tmpDir); + fs.writeFileSync(path.join(tmpDir, 'unrelated-wip.txt'), 'wip\n'); + execFileSync('git', ['add', 'unrelated-wip.txt'], { cwd: tmpDir, stdio: 'pipe' }); + + const { result } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md'], + failFor: [], + }); + + assert.equal(result.committed, true, `expected a commit, got ${JSON.stringify(result)}`); + assert.equal(result.reason, 'committed'); + assert.equal(headCount(tmpDir), before + 1); + assert.deepEqual( + committedFiles(tmpDir), + ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md'], + 'the declared scope must still be honoured, and the unrelated staged file left alone', + ); + }); + + // ── Missing explicit files keep their existing documented handling ──────── + + test('a missing explicit file is still skipped, not reported as a staging failure', () => { + // #2014/#2523 behaviour: an explicitly-named file that does not exist is + // skipped rather than staged as a deletion. It never reaches `git add`, so + // it is not a staging failure and must not become one. + const { result } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md', '.planning/DOES-NOT-EXIST.md'], + failFor: [], + }); + + assert.equal(result.committed, true, `expected a commit, got ${JSON.stringify(result)}`); + assert.deepEqual(committedFiles(tmpDir), ['.planning/ARCHITECTURE.md']); + }); + + // ── The index must be left clean, not partially staged ─────────────────── + + test('a staging failure rolls back the paths this call had already staged', () => { + // Without the rollback the paths that DID stage stay in the index with no + // commit made, so the next bare `git commit` sweeps them up — the same + // silent partial commit this fix exists to prevent, just deferred a step. + commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md', '.planning/CONVENTIONS.md'], + failFor: ['.planning/CONCERNS.md'], + }); + + const status = execFileSync('git', ['status', '--porcelain'], { cwd: tmpDir, encoding: 'utf-8' }); + const stagedAdds = status.split('\n').filter((l) => /^A[ \t]/.test(l)); + assert.deepEqual(stagedAdds, [], + `no path may remain staged after a staging failure, status:\n${status}`); + }); + + test('the rollback does not unstage work the caller had staged before the call', () => { + // Boundary: the reset must touch only what THIS call staged. Unstaging a + // path the caller staged themselves would destroy their work. + fs.writeFileSync(path.join(tmpDir, 'caller-staged.txt'), 'mine\n'); + execFileSync('git', ['add', 'caller-staged.txt'], { cwd: tmpDir, stdio: 'pipe' }); + + commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/ARCHITECTURE.md', '.planning/CONCERNS.md'], + failFor: ['.planning/CONCERNS.md'], + }); + + const status = execFileSync('git', ['status', '--porcelain'], { cwd: tmpDir, encoding: 'utf-8' }); + assert.match(status, /^A[ \t]+caller-staged\.txt$/m, + `the caller's own staged file must survive the rollback, status:\n${status}`); + }); + + // ── The default (non---files) staging path is guarded too ───────────────── + + test('a failed default-mode git add fails closed instead of committing the index', () => { + // Default mode stages `.planning/`. Pre-fix a failure there also fell + // through to an unguarded `git commit`. + const before = headCount(tmpDir); + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: undefined, + failFor: ['.planning/'], + }); + + assert.equal(result.reason, 'staging_failed'); + assert.ok(!gitCalls.some((a) => a[0] === 'commit'), 'git commit must not run'); + assert.equal(headCount(tmpDir), before); + }); + + test('a failed default-mode git add blocks --amend too', () => { + // --amend has no carve-out: amending on top of a failed staging would + // rewrite the tip without the changes the caller asked for. + const before = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: tmpDir, encoding: 'utf-8' }).trim(); + const { result, gitCalls } = commitWithFailingAdd({ + cwd: tmpDir, + files: undefined, + failFor: ['.planning/'], + amend: true, + }); + + assert.equal(result.reason, 'staging_failed'); + assert.ok(!gitCalls.some((a) => a[0] === 'commit'), 'git commit --amend must not run'); + assert.equal( + execFileSync('git', ['rev-parse', 'HEAD'], { cwd: tmpDir, encoding: 'utf-8' }).trim(), + before, + 'HEAD must not be rewritten when staging failed', + ); + }); + + test('when all explicit files are missing the reason is still nothing_to_commit', () => { + // The nothing_to_commit path must survive: no `git add` ran, so there is no + // staging failure to report. + const { result } = commitWithFailingAdd({ + cwd: tmpDir, + files: ['.planning/GONE-A.md', '.planning/GONE-B.md'], + failFor: [], + }); + + assert.equal(result.reason, 'nothing_to_commit'); + }); +}); diff --git a/tests/fixtures/golden-install-parity/antigravity.json b/tests/fixtures/golden-install-parity/antigravity.json index 609401ac5..f45c2591b 100644 --- a/tests/fixtures/golden-install-parity/antigravity.json +++ b/tests/fixtures/golden-install-parity/antigravity.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "1db46cac3f4d9889", "agents/gsd-eval-auditor.md": "1b8391f1aafb067f", "agents/gsd-eval-planner.md": "3d10fd11147f6857", - "agents/gsd-executor.md": "f9bd85ba91a2a042", + "agents/gsd-executor.md": "5c31f63a3e9559a0", "agents/gsd-framework-selector.md": "daa62c79619c76bf", "agents/gsd-integration-checker.md": "0643cd2d779b131c", "agents/gsd-intel-updater.md": "26c1f1e028c6346a", diff --git a/tests/fixtures/golden-install-parity/augment.json b/tests/fixtures/golden-install-parity/augment.json index 8b2bf199e..641674578 100644 --- a/tests/fixtures/golden-install-parity/augment.json +++ b/tests/fixtures/golden-install-parity/augment.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "671c9ea949889c4a", "agents/gsd-eval-auditor.md": "fcaec7b00f94c435", "agents/gsd-eval-planner.md": "a4a5b4b3f7828ba3", - "agents/gsd-executor.md": "3f03ad10b7251f68", + "agents/gsd-executor.md": "0fd0329cfa05519d", "agents/gsd-framework-selector.md": "4b77eebbe9288d80", "agents/gsd-integration-checker.md": "fa53e2d78be1de74", "agents/gsd-intel-updater.md": "fa40e685d7441ace", diff --git a/tests/fixtures/golden-install-parity/claude-local.json b/tests/fixtures/golden-install-parity/claude-local.json index 87605b5e9..54e2a5746 100644 --- a/tests/fixtures/golden-install-parity/claude-local.json +++ b/tests/fixtures/golden-install-parity/claude-local.json @@ -15,7 +15,7 @@ "agents/gsd-domain-researcher.md": "f1e03df842ddfb95", "agents/gsd-eval-auditor.md": "d0f45fff7370bb0b", "agents/gsd-eval-planner.md": "9cc049b82897daa4", - "agents/gsd-executor.md": "c6a3f08a31afc6bf", + "agents/gsd-executor.md": "b5262908713115ab", "agents/gsd-framework-selector.md": "85005d716f9d98f7", "agents/gsd-integration-checker.md": "17a8ee731986564d", "agents/gsd-intel-updater.md": "4953a465db9dadc1", diff --git a/tests/fixtures/golden-install-parity/claude.json b/tests/fixtures/golden-install-parity/claude.json index 61c0fcd53..8a2b4f1e8 100644 --- a/tests/fixtures/golden-install-parity/claude.json +++ b/tests/fixtures/golden-install-parity/claude.json @@ -15,7 +15,7 @@ "agents/gsd-domain-researcher.md": "5f7d366251b957fe", "agents/gsd-eval-auditor.md": "fea2759beff0a642", "agents/gsd-eval-planner.md": "112f6730f23854e3", - "agents/gsd-executor.md": "dc6785301ba81da4", + "agents/gsd-executor.md": "74e35ceab5fc727f", "agents/gsd-framework-selector.md": "c350ee693cb1aa4e", "agents/gsd-integration-checker.md": "c8b4e65dee89c8ea", "agents/gsd-intel-updater.md": "5b41e05f90ce89d9", diff --git a/tests/fixtures/golden-install-parity/cline.json b/tests/fixtures/golden-install-parity/cline.json index 56fb268d8..aa233a4c2 100644 --- a/tests/fixtures/golden-install-parity/cline.json +++ b/tests/fixtures/golden-install-parity/cline.json @@ -19,7 +19,7 @@ "agents/gsd-domain-researcher.md": "0fecdaea86466a56", "agents/gsd-eval-auditor.md": "36c44303085df2f8", "agents/gsd-eval-planner.md": "3ddea88a69b4da3f", - "agents/gsd-executor.md": "eb82990b9b81e39d", + "agents/gsd-executor.md": "b7270d3e755f6058", "agents/gsd-framework-selector.md": "564669d479433f15", "agents/gsd-integration-checker.md": "1bbbdd3d420b994e", "agents/gsd-intel-updater.md": "42c40fffbc720d0b", diff --git a/tests/fixtures/golden-install-parity/codebuddy.json b/tests/fixtures/golden-install-parity/codebuddy.json index 405e082cf..d68c77e0b 100644 --- a/tests/fixtures/golden-install-parity/codebuddy.json +++ b/tests/fixtures/golden-install-parity/codebuddy.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "1c1a800108a2b225", "agents/gsd-eval-auditor.md": "99012004b14ea602", "agents/gsd-eval-planner.md": "4ebdd7fe9cbb0cfe", - "agents/gsd-executor.md": "412ddd97399607a2", + "agents/gsd-executor.md": "5c9d4b46c025a8af", "agents/gsd-framework-selector.md": "7726fccc86bfeb50", "agents/gsd-integration-checker.md": "2d8339790bbb2dc3", "agents/gsd-intel-updater.md": "c51339956197cbd3", diff --git a/tests/fixtures/golden-install-parity/codex.json b/tests/fixtures/golden-install-parity/codex.json index 3c8c42755..7bf44b59b 100644 --- a/tests/fixtures/golden-install-parity/codex.json +++ b/tests/fixtures/golden-install-parity/codex.json @@ -102,8 +102,8 @@ "agents/gsd-eval-auditor.toml": "9b81d61b3c5f722d", "agents/gsd-eval-planner.md": "73f2ad2ff2797a51", "agents/gsd-eval-planner.toml": "09468ad1a34ac468", - "agents/gsd-executor.md": "cd4dac78e03debd1", - "agents/gsd-executor.toml": "427bb38fc924d3e8", + "agents/gsd-executor.md": "c357dad443b0bdf0", + "agents/gsd-executor.toml": "5d3156365295b12d", "agents/gsd-framework-selector.md": "ebae32430887d2e0", "agents/gsd-framework-selector.toml": "637e4e021b7ec380", "agents/gsd-integration-checker.md": "9cc875676cf7d741", diff --git a/tests/fixtures/golden-install-parity/copilot.json b/tests/fixtures/golden-install-parity/copilot.json index f1516381e..54b2475e2 100644 --- a/tests/fixtures/golden-install-parity/copilot.json +++ b/tests/fixtures/golden-install-parity/copilot.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.agent.md": "d603239b3e9fe428", "agents/gsd-eval-auditor.agent.md": "3c03009564de55c8", "agents/gsd-eval-planner.agent.md": "14751876fc2b5f16", - "agents/gsd-executor.agent.md": "d8439a4f44e7e6eb", + "agents/gsd-executor.agent.md": "79770a630b4f9e58", "agents/gsd-framework-selector.agent.md": "cafeec0b3489be45", "agents/gsd-integration-checker.agent.md": "30439b804927acc7", "agents/gsd-intel-updater.agent.md": "238c1a886f35a25c", diff --git a/tests/fixtures/golden-install-parity/cursor.json b/tests/fixtures/golden-install-parity/cursor.json index 158f8c7fa..64b54da0c 100644 --- a/tests/fixtures/golden-install-parity/cursor.json +++ b/tests/fixtures/golden-install-parity/cursor.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "56395dbdabf076f6", "agents/gsd-eval-auditor.md": "ad2840fd5cd76172", "agents/gsd-eval-planner.md": "2049dac060d00eda", - "agents/gsd-executor.md": "4246bd0e27197e6e", + "agents/gsd-executor.md": "8c86bf037dc02e8c", "agents/gsd-framework-selector.md": "4b77eebbe9288d80", "agents/gsd-integration-checker.md": "5da30584d06b878c", "agents/gsd-intel-updater.md": "b8971c5d96e63b38", diff --git a/tests/fixtures/golden-install-parity/hermes.json b/tests/fixtures/golden-install-parity/hermes.json index bb826cc3c..bda7da186 100644 --- a/tests/fixtures/golden-install-parity/hermes.json +++ b/tests/fixtures/golden-install-parity/hermes.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "412cdbb05ba252ea", "agents/gsd-eval-auditor.md": "4ffb265063c318e5", "agents/gsd-eval-planner.md": "03448fc9c5774b56", - "agents/gsd-executor.md": "53379f898b45b192", + "agents/gsd-executor.md": "481d921115690b41", "agents/gsd-framework-selector.md": "ea9981d65d6b3429", "agents/gsd-integration-checker.md": "35b4f2969d279871", "agents/gsd-intel-updater.md": "5fe5edfae2719cb8", diff --git a/tests/fixtures/golden-install-parity/kilo.json b/tests/fixtures/golden-install-parity/kilo.json index cd13ab426..0f7f93f0a 100644 --- a/tests/fixtures/golden-install-parity/kilo.json +++ b/tests/fixtures/golden-install-parity/kilo.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "a3874d80bcbc7380", "agents/gsd-eval-auditor.md": "630d4cd3bd6ea195", "agents/gsd-eval-planner.md": "3db12cde12aeb2c1", - "agents/gsd-executor.md": "2dfa6f871d65bab1", + "agents/gsd-executor.md": "9ac9bf30383cf94c", "agents/gsd-framework-selector.md": "ad5f2c6b9bec6270", "agents/gsd-integration-checker.md": "c503e2f4a3d8ec05", "agents/gsd-intel-updater.md": "231393da62a45b2e", diff --git a/tests/fixtures/golden-install-parity/kimi-code.json b/tests/fixtures/golden-install-parity/kimi-code.json index 0c6483a57..3554b71ff 100644 --- a/tests/fixtures/golden-install-parity/kimi-code.json +++ b/tests/fixtures/golden-install-parity/kimi-code.json @@ -45,7 +45,7 @@ "agents/gsd-domain-researcher.md": "049f588663814fa2", "agents/gsd-eval-auditor.md": "54870d3b07433525", "agents/gsd-eval-planner.md": "552e9fa164c51ce8", - "agents/gsd-executor.md": "258fcf3cc19fb411", + "agents/gsd-executor.md": "ab4f1cbca7c22663", "agents/gsd-framework-selector.md": "8a795f230436ad2e", "agents/gsd-integration-checker.md": "c1760a0bbd4f7bf5", "agents/gsd-intel-updater.md": "944f1d903e2e9e09", diff --git a/tests/fixtures/golden-install-parity/kimi.json b/tests/fixtures/golden-install-parity/kimi.json index 64d3872fc..0f92f5917 100644 --- a/tests/fixtures/golden-install-parity/kimi.json +++ b/tests/fixtures/golden-install-parity/kimi.json @@ -62,7 +62,7 @@ "agents/subagents/gsd-eval-auditor.yaml": "e3d868bd5fefe938", "agents/subagents/gsd-eval-planner.md": "70f8c5727bfb9876", "agents/subagents/gsd-eval-planner.yaml": "df8499f7af297ec2", - "agents/subagents/gsd-executor.md": "77f6d8e414227fcd", + "agents/subagents/gsd-executor.md": "6cbab4fe3103856a", "agents/subagents/gsd-executor.yaml": "e29422986636fd64", "agents/subagents/gsd-framework-selector.md": "a15b7aa1e0576e16", "agents/subagents/gsd-framework-selector.yaml": "fb52c31cde27b0e3", diff --git a/tests/fixtures/golden-install-parity/opencode.json b/tests/fixtures/golden-install-parity/opencode.json index 5e17044ce..0fe3bcd13 100644 --- a/tests/fixtures/golden-install-parity/opencode.json +++ b/tests/fixtures/golden-install-parity/opencode.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "71250e759ca9e723", "agents/gsd-eval-auditor.md": "c88890105f32ace6", "agents/gsd-eval-planner.md": "60bddb70a937f796", - "agents/gsd-executor.md": "3fdbe96e721a8099", + "agents/gsd-executor.md": "92a84bdd8895a4bb", "agents/gsd-framework-selector.md": "1c0a10355e787675", "agents/gsd-integration-checker.md": "a9de5928e5a5c649", "agents/gsd-intel-updater.md": "493e07482fa6198a", diff --git a/tests/fixtures/golden-install-parity/qwen.json b/tests/fixtures/golden-install-parity/qwen.json index e06b16f02..cf247be34 100644 --- a/tests/fixtures/golden-install-parity/qwen.json +++ b/tests/fixtures/golden-install-parity/qwen.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "bd054bb27beed2a7", "agents/gsd-eval-auditor.md": "57cc7458ab5de6b7", "agents/gsd-eval-planner.md": "01b665728dde4ccf", - "agents/gsd-executor.md": "ebca30765efc0b68", + "agents/gsd-executor.md": "18b780103d9fd511", "agents/gsd-framework-selector.md": "82ba6abea84226b7", "agents/gsd-integration-checker.md": "90835dbc7dfa1691", "agents/gsd-intel-updater.md": "3cc4f6ddd04676ec", diff --git a/tests/fixtures/golden-install-parity/trae.json b/tests/fixtures/golden-install-parity/trae.json index bee521ddc..708519e60 100644 --- a/tests/fixtures/golden-install-parity/trae.json +++ b/tests/fixtures/golden-install-parity/trae.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "b80f76874c04e515", "agents/gsd-eval-auditor.md": "470bf16303ec4d2e", "agents/gsd-eval-planner.md": "22334fde85723c9d", - "agents/gsd-executor.md": "191215b38db8bb1b", + "agents/gsd-executor.md": "e47f3ac8cf7384c9", "agents/gsd-framework-selector.md": "7726fccc86bfeb50", "agents/gsd-integration-checker.md": "7cd2072984411c7f", "agents/gsd-intel-updater.md": "83de6ba9172891c3", diff --git a/tests/fixtures/golden-install-parity/windsurf.json b/tests/fixtures/golden-install-parity/windsurf.json index ca66b6254..4d6c11a64 100644 --- a/tests/fixtures/golden-install-parity/windsurf.json +++ b/tests/fixtures/golden-install-parity/windsurf.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "56395dbdabf076f6", "agents/gsd-eval-auditor.md": "fb64fc5acf359747", "agents/gsd-eval-planner.md": "2049dac060d00eda", - "agents/gsd-executor.md": "09666c96d441ecfa", + "agents/gsd-executor.md": "f7c8d3dba51391b2", "agents/gsd-framework-selector.md": "4b77eebbe9288d80", "agents/gsd-integration-checker.md": "4ffb37fb230c2b90", "agents/gsd-intel-updater.md": "a81d77c143c02108", diff --git a/tests/fixtures/golden-install-parity/zcode.json b/tests/fixtures/golden-install-parity/zcode.json index 245775a94..34ef745c0 100644 --- a/tests/fixtures/golden-install-parity/zcode.json +++ b/tests/fixtures/golden-install-parity/zcode.json @@ -16,7 +16,7 @@ "agents/gsd-domain-researcher.md": "049f588663814fa2", "agents/gsd-eval-auditor.md": "54870d3b07433525", "agents/gsd-eval-planner.md": "552e9fa164c51ce8", - "agents/gsd-executor.md": "258fcf3cc19fb411", + "agents/gsd-executor.md": "ab4f1cbca7c22663", "agents/gsd-framework-selector.md": "8a795f230436ad2e", "agents/gsd-integration-checker.md": "c1760a0bbd4f7bf5", "agents/gsd-intel-updater.md": "944f1d903e2e9e09",