fix(#706): skip rescue of already-committed SUMMARY to avoid worktree cleanup merge_failed (#709)

* fix(#706): skip rescueSummaryArtifacts when SUMMARY is already committed

rescueSummaryArtifacts now probes `git cat-file -e HEAD:<path>` before
copying a SUMMARY.md into the main checkout.  When the file is already
committed on the worktree branch, copying it as an untracked file causes
`git merge --no-ff` to abort with "untracked working tree files would be
overwritten by merge" — a permanent merge_failed cleanup-wave failure.

Fail-closed on timeout: if cat-file is unreliable we skip rescue (the
merge will surface the collision as it did before, which is recoverable).

Adds 4 new test cases in worktree-safety.test.cjs covering:
- committed SUMMARY skipped, merge succeeds (#706 regression case)
- committed SUMMARY skipped even when timeout (fail-closed)
- uncommitted SUMMARY still rescued (existing contract preserved)
- rescue failure on ENOSPC still propagates (unchanged)

Closes #706

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* chore: add changeset for #706

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(#706): treat cat-file exit 128 as uncertain — skip rescue (fail-closed)

The previous guard skipped rescue only when `exitCode === 0` (committed) or
`timedOut`. Any other non-zero exit, including `128` (fatal git error: corrupt
object store, unborn HEAD, missing repo), fell through and PROCEEDED with
rescue — potentially re-creating the #706 untracked-file merge collision.

Fix: rescue ONLY when `exitCode === 1` (cat-file definitively reports the
object absent). All other outcomes — 0 (committed), 128 (fatal), null/SIGTERM
(timeout), or any other code — are treated as "uncertain → skip rescue".

Also corrects the JSDoc bullet that still referenced `git ls-files
--error-unmatch` (the old mechanism); updated to `git cat-file -e HEAD:<relPath>`.

Regression test added: asserts rescue is SKIPPED when cat-file returns exit 128,
leaving the merge to surface the issue safely rather than silently copying an
already-committed file.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* chore: link changeset to PR #709

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-06-06 12:40:28 -04:00
committed by GitHub
parent 4b50a686e5
commit dfbb684843
3 changed files with 404 additions and 0 deletions

View File

@@ -543,6 +543,12 @@ function defaultFindSummaryFiles(worktreePath: string): string[] {
*
* For each *SUMMARY.md found under <worktreePath>/.planning/:
* - compute relative path from worktree root → .planning/<id>-SUMMARY.md
* - if the file is ALREADY COMMITTED on the worktree branch
* (`git cat-file -e HEAD:<relPath>` returns exit 0), skip the copy entirely:
* the merge will carry it naturally and copying it as an untracked file would
* cause a "untracked working tree files would be overwritten by merge" collision.
* On timeout or fatal exit (128) the rescue is also skipped (fail-closed).
* (#706 — execute-phase committed-SUMMARY contract)
* - destination = <repoRoot>/<relPath>
* - copy when dest is absent or content differs
*
@@ -560,6 +566,7 @@ function rescueSummaryArtifacts(
repoRoot: string,
deps: WorktreeDeps,
): { rescuedRelPaths: Set<string>; failures: Array<{ relPath: string; error: string }> } {
const execGit = deps.execGit || execGitDefault;
const findSummaryFiles = deps.findSummaryFiles || defaultFindSummaryFiles;
const existsSync = deps.existsSync || fs.existsSync;
const readFileSync = deps.readFileSync || ((p: string) => fs.readFileSync(p, 'utf8'));
@@ -576,6 +583,31 @@ function rescueSummaryArtifacts(
// output works on Windows too (git always emits forward slashes in porcelain output).
const relPath = absPath.slice(worktreePath.length).replace(/^[/\\]/, '').replace(/\\/g, '/');
// #706: skip rescue when the SUMMARY is already committed on the branch.
// Use `git cat-file -e HEAD:<relPath>` (not `ls-files --error-unmatch`) so
// the check is against the committed tree, not the index. ls-files also
// matches staged-but-uncommitted files, which would skip rescue when the
// file is staged but not yet committed — the merge wouldn't carry it, and
// the executor's content could be lost. cat-file -e HEAD:<path> returns
// exit 0 only when the object exists in the committed HEAD tree.
//
// Fail-closed on timeout/fatal git errors: if we cannot determine whether
// the file is committed, do NOT rescue it (rescuing an actually-committed
// file would re-create the untracked collision; the merge will surface the
// issue). The cleanup will be blocked by merge_failed in the worst case,
// which is the observable behaviour before this fix and is recoverable.
const catFileResult = execGit(['-C', worktreePath, 'cat-file', '-e', `HEAD:${relPath}`], { cwd: repoRoot });
if (catFileResult.exitCode !== 1) {
// Rescue only when cat-file definitively reports the object is absent (exit 1).
// exit 0 → object exists (committed on HEAD) — merge will carry it, skip.
// exit 128 → fatal git error (corrupt store, unborn HEAD, etc.) — uncertain,
// fail-closed: do NOT rescue to avoid recreating the #706 collision.
// timedOut / null / other → unreliable result — same fail-closed policy.
// In all non-1 cases the merge will either succeed naturally (0) or surface
// the problem safely (128/timeout), which is the recoverable pre-fix behaviour.
continue;
}
const dest = path.join(repoRoot, relPath);
let needsCopy = !existsSync(dest);
if (!needsCopy) {