golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#2608): fail closed when git add fails during commit staging (#2693)

Browse Source 
* 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>
This commit is contained in:
Tom Boucher
2026-07-27 02:45:35 -04:00
committed by GitHub
parent 2c44241a0b
commit 28e486faf7
24 changed files with 597 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

105
src/commands.cts
View File

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