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

fix(#4721): give worktree cleanup-wave's merge its own timeout, report merge_timed_out, and restore the index a killed merge leaves staged (#4766)

Browse Source 
* fix(#4721): give cleanup-wave's merge its own timeout, report merge_timed_out, and restore the index a killed merge leaves staged

`worktree cleanup-wave` ran `git merge --no-ff` under the module-wide
DEFAULT_GIT_TIMEOUT_MS (10 s) that is sized for plumbing calls. The merge is
the one call in the wave that runs user hooks, so a repo whose
pre-merge-commit hook is a test-suite gate lost every code-bearing executor
merge. Three things went wrong at once, each fixed here:

1. Budget. The merge now passes an explicit timeout —
   DEFAULT_MERGE_TIMEOUT_MS (10 min), overridable via deps.mergeTimeoutMs.
   Every other git call in the wave keeps the module default; the shared
   constant is untouched, because every other caller is exactly what its
   10 s comment describes.

2. Reason. A merge that does time out blocks on `merge_timed_out`, and its
   stderr names the budget and says the hook may still be running, instead
   of `merge_failed` carrying whatever the hook had printed before git was
   killed — which made a healthy executor branch look broken.

3. Residue. A merge killed during its hook has already staged the merged
   tree into the primary's index but never wrote MERGE_HEAD, so
   `git merge --abort` finds nothing and repoRootStillMidMerge (#2852)
   reads the primary as clean while the executor's whole diff sits staged
   against the old HEAD; a `git commit` from that state squashes the
   executor's history into one parent. After any failed merge the wave now
   reads `git diff --cached --name-only`; anything staged is the merge's
   own (git refuses to start a merge when the index differs from HEAD), so
   it runs `git reset --merge` — restores exactly those paths, keeps
   unrelated unstaged edits — and re-reads. Restored paths are reported as
   WAVE_CLEANUP_WARNING.MERGE_RESIDUE_RESTORED and the wave continues; a
   still-dirty or unreadable index reports MERGE_RESIDUE_LEFT_STAGED and
   halts the remaining entries, the same repo-level carve-out an
   unfinished merge takes.

Tests: five mock-driven rows (budget wiring incl. the deps override, the
timeout classification with restore, the no-reset control for an ordinary
refused merge, an unrestorable residue halting the wave, an unverifiable
index failing closed) plus a real-git row that runs a sleeping
pre-merge-commit hook under a 1 s budget and asserts HEAD unmoved, index
and worktree clean, the executor branch intact — with the same fixture
merging cleanly under the default budget as its negative control. Two
existing #2852 rows gain a handler for the new post-failure index read.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9

* docs(#4721): add Fixed changeset

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9

* test(#4721): release the real-git fixtures with t.after, not try/finally

The two real-git rows cleaned up their scratch repo in a `finally` block;
this file's own convention for fixture teardown is the test context's
`t.after(() => cleanup(dir))`, and the house PR ruleset flags `finally` in a
test body. Behaviour-neutral.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9

* fix(#4721): gate the residue restore on the timeout, re-apply a merge autostash, and correct the hook census

Three findings from the pre-file adversarial review of the previous commit,
each driven on real git before changing code:

1. A merge git REFUSED ("your local changes … would be overwritten") also
   leaves no MERGE_HEAD — and that refusal is exactly what a pre-existing
   dirty primary index earns. The residue restore read that index as the
   merge's own and `reset --merge`d the operator's staged work away
   (driven: a staged edit to an unrelated file was discarded and reported
   as "restored"). The restore now runs ONLY when the merge timed out; a
   refusal is an immediate exit, never a timeout, so on that path nothing
   is read or reset.

2. `merge.autoStash=true` lets a merge start on a dirty index by parking
   the work in MERGE_AUTOSTASH, which a killed merge never re-applies.
   `git reset --merge` moves that stash into the stash list; the wave now
   runs `git stash pop --index` afterwards (the outcome `merge --abort`
   gives an autostashed merge), and reports
   WAVE_CLEANUP_WARNING.MERGE_AUTOSTASH_UNRESTORED (path null) when the
   pop fails or the autostash state could not be read — the work stays in
   the stash, the index is clean, the wave continues. Because of this the
   reset runs on a timed-out merge even when the index reads clean.

3. The merge is not the only hook-running git call in the module:
   `worktree add` runs post-checkout and every ref update runs
   reference-transaction. It is the only call that runs the commit-family
   hooks, which is what the budget is for. Comments and docs say so now.

Tests: the "ordinary merge_failed" control becomes the regression row for
finding 1 (strict mock — a `diff --cached` or `reset --merge` on a refused
merge throws), plus a mock row for the autostash pop (dirty and clean
index, pop success and failure), and two real-git rows: a refused merge
over pre-existing staged work leaves it byte-identical, and a killed merge
under merge.autoStash restores the executor residue AND puts the
operator's staged work back. The real-git hook now sleeps 4 s against a
1.5 s budget for margin on slow runners. The two #2852 handlers added
earlier are removed — the residue read no longer fires on their path.
Negative control: 4 of the 10 #4721 rows fail on the previous commit.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9

* fix(#4721): key the residue restore on a killed merge, and re-read the index after a failed autostash pop

Two more findings from the continuation review, both driven:

1. An externally delivered SIGTERM leaves the same staged/no-MERGE_HEAD
   state as the timeout, and the seam reports it as exitCode null + signal
   with timedOut false — so the timeout-only gate skipped the restore on a
   state it was written for. The gate is now "killed": timedOut, or a null
   exit code with a signal. A refused merge still exits with a code and is
   still never touched. The reason stays merge_failed for a signal kill.

2. A failed `git stash pop --index` keeps the stash entry but can leave
   conflict entries (UU) and partially applied paths, after which the next
   merge fails on "you have unmerged files"; the code returned halt:false
   on the strength of the pre-pop recheck. The index is now re-read after a
   failed pop and a dirty result halts the wave as merge_residue_left_staged
   alongside the merge_autostash_unrestored warning.

Also driven and now documented rather than changed: a kill that lands once
MERGE_HEAD exists (inside commit-msg) is the ordinary #2852 abort path —
`git merge --abort` restores the tree and re-applies an autostash itself,
unstaged, as git does for any aborted autostashed merge.

Tests: the pop-failure mock row now asserts the post-pop re-read and gains
a conflict-leftover variant that halts; a signal-kill mock row; a real-git
row with the sleeping hook moved to commit-msg (timed out, no residue
warnings, MERGE_HEAD cleared, primary clean). 414 pass.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9

* fix(#4721): key the kill gate on the seam's signal, not on a null exit code

The shell projection seam normalizes a signal death to exitCode 1 and
carries the signal alongside (`_spawnResult`: `result.status ?? 1`), so the
previous `exitCode === null && signal` gate could never fire in production
and the unit row that covered it modelled a shape the seam does not emit
(caught in the round-3 review). The gate is now `timedOut || signal`; a
refused merge exits with a code and no signal. The mock row uses the real
shape, and a mocked spawnSync signal death driven through the compiled seam
reaches `reset --merge` and reports the residue restored.

Also: three comments that still said "at its budget" / "runs user hooks" /
"the index is clean", and the CLI-TOOLS sentence that reserved
`merge_failed` for refusals and conflicts, now name the signal case.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9

* chore(#4721): set changeset fragment pr to 4766

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
0xdhx
2026-09-15 21:28:09 -05:00
committed by GitHub
parent b5fc8061b3
commit 25d1cb916f
5 changed files with 625 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

194
src/worktree-safety.cts
View File

@@ -19,6 +19,18 @@ import { isContainedIn } from './security.cjs';
// remote, stalled NFS mount, etc.). Callers can override via deps.timeout.
const DEFAULT_GIT_TIMEOUT_MS = 10000;
// #4721: the wave's `git merge --no-ff` is the one call in this module that runs
// the commit-family hooks (`pre-merge-commit`, `prepare-commit-msg`,
// `commit-msg`, `post-merge`), and a repo whose pre-merge hook is a test-suite
// gate routinely runs for minutes. That is hook runtime, not "git stalling", so
// the merge gets its own budget instead of inheriting DEFAULT_GIT_TIMEOUT_MS —
// raising the shared default would be the wrong lever, because every other
// caller in the module is exactly what the 10 s comment above describes.
// (`worktree add` runs `post-checkout` and every ref update runs
// `reference-transaction`; those are plumbing-cheap and stay on the default.)
// Callers override via deps.mergeTimeoutMs.
const DEFAULT_MERGE_TIMEOUT_MS = 10 * 60 * 1000;
// #3021: accept the Workflow tool's worktree-wf_<runid>-<n> naming convention
// (claude-orchestration's isolation:"worktree" emission) alongside the
// existing agent-<id> / worktree-agent-<id> shapes.
@@ -103,6 +115,12 @@ interface WorktreeDeps {
/** Injected current time in ms since epoch for deterministic tests (#1191). */
nowMs?: number;
parseWorktreePorcelain?: (porcelain: string) => WorktreeBranchEntry[];
/**
* #4721: budget for the wave's `git merge --no-ff` — the one call that runs
* the commit-family hooks. Defaults to DEFAULT_MERGE_TIMEOUT_MS; every other
* git call in the wave keeps the module default.
*/
mergeTimeoutMs?: number;
}
function readWorktreeList(repoRoot: string, deps: WorktreeDeps = {}): WorktreeListResult {
@@ -645,6 +663,102 @@ function repoRootStillMidMerge(execGit: ExecGitFn, repoRoot: string): boolean {
return true; // any other exit code (e.g. a fatal git error) — fail closed
}
/**
* #4721: after a merge that was KILLED — at its budget or by a signal — and
* did not leave MERGE_HEAD behind, undo whatever it staged in repoRoot's index
* and re-apply any work it had autostashed. (A kill that lands once MERGE_HEAD
* exists — inside `commit-msg`, say — is the ordinary #2852 path: `git merge
* --abort` restores the tree and re-applies an autostash itself, unstaged, as
* it does for any aborted autostashed merge.)
*
* Why the staged set is attributable to the merge — on this path only: `git
* merge` refuses to start when the index already differs from HEAD ("your
* local changes … would be overwritten", even for paths the branch never
* touches), and a refusal is an immediate exit with a code, never a kill. The
* one way for a KILLED merge to leave a dirty index with no MERGE_HEAD is a
* kill between populating the index and writing MERGE_HEAD — i.e. during a
* merge hook. The exception is `merge.autoStash`: git then parks the pre-existing
* work in MERGE_AUTOSTASH and starts anyway, and a killed merge never
* re-applies it. Handled below; it is why the reset runs even on a clean
* index.
*
* `git reset --merge` (no commit → HEAD) is the restore: it resets the index
* to HEAD and updates the worktree only for the paths the index changed,
* keeping unrelated unstaged edits intact, and it refuses rather than clobbers
* when an unstaged edit overlaps a staged path. It also moves a pending
* MERGE_AUTOSTASH into the stash list ("Autostash exists; creating a new stash
* entry"), which `git stash pop --index` then re-applies — the same outcome
* `git merge --abort` gives an autostashed merge that could be aborted.
*
* Returns `halt: true` only when repoRoot is still (or unverifiably) dirty —
* the same repo-level carve-out `repoRootStillMidMerge` uses, and for the same
* reason: every remaining entry's merge would run against a dirty index.
*/
function restoreMergeResidue(
execGit: ExecGitFn,
repoRoot: string,
branch: string,
): { halt: boolean; warnings: WaveCleanupWarning[] } {
const stagedPaths = (raw: string): string[] => raw
.split('\n')
.map((line) => decodeGitQuotedPath(line.trim()))
.filter((p) => p.length > 0);
const leftStaged = (paths: Array<string | null>): { halt: boolean; warnings: WaveCleanupWarning[] } => ({
halt: true,
warnings: paths.map((p) => ({ code: WAVE_CLEANUP_WARNING.MERGE_RESIDUE_LEFT_STAGED, branch, path: p })),
});
const staged = execGit(['diff', '--cached', '--name-only'], { cwd: repoRoot });
if (!gitResultOk(staged)) {
// Cannot tell whether the index is dirty — fail closed, same as an
// unverifiable MERGE_HEAD check. A null path marks "the check itself could
// not run", the convention SCOPE_CHECK_UNAVAILABLE already uses.
return leftStaged([null]);
}
const before = stagedPaths(staged.stdout || '');
// Exit 0 = git parked pre-existing work here before starting the merge;
// exit 1 = no autostash. Anything else is unknown: do not pop blind, but do
// say so — the reset below will have moved any stash into the list unread.
const autostash = execGit(['rev-parse', '--verify', '-q', 'MERGE_AUTOSTASH'], { cwd: repoRoot });
const hadAutostash = !autostash.timedOut && autostash.exitCode === 0;
const autostashUnknown = !!autostash.timedOut || (autostash.exitCode !== 0 && autostash.exitCode !== 1);
if (before.length === 0 && !hadAutostash && !autostashUnknown) return { halt: false, warnings: [] };
const reset = execGit(['reset', '--merge'], { cwd: repoRoot });
const recheck = gitResultOk(reset) ? execGit(['diff', '--cached', '--name-only'], { cwd: repoRoot }) : null;
if (!recheck || !gitResultOk(recheck)) {
// The reset failed, or its result could not be re-read: report the set we
// know was staged, and halt.
return leftStaged(before.length > 0 ? before : [null]);
}
const after = stagedPaths(recheck.stdout || '');
if (after.length > 0) return leftStaged(after);
const warnings: WaveCleanupWarning[] = before.map((p) => ({ code: WAVE_CLEANUP_WARNING.MERGE_RESIDUE_RESTORED, branch, path: p }));
if (hadAutostash) {
const pop = execGit(['stash', 'pop', '--index'], { cwd: repoRoot });
if (!gitResultOk(pop)) {
warnings.push({ code: WAVE_CLEANUP_WARNING.MERGE_AUTOSTASH_UNRESTORED, branch, path: null });
// A failed pop keeps the stash entry, but it can leave conflict entries
// (`UU`) and partially applied paths behind it — and the next merge then
// fails with "you have unmerged files" (caught in review). Re-read rather
// than assume: a dirty index here halts exactly as an unrestorable
// residue does.
const afterPop = execGit(['diff', '--cached', '--name-only'], { cwd: repoRoot });
if (!gitResultOk(afterPop)) return { halt: true, warnings: [...warnings, ...leftStaged([null]).warnings] };
const dirty = stagedPaths(afterPop.stdout || '');
if (dirty.length > 0) return { halt: true, warnings: [...warnings, ...leftStaged(dirty).warnings] };
}
} else if (autostashUnknown) {
warnings.push({ code: WAVE_CLEANUP_WARNING.MERGE_AUTOSTASH_UNRESTORED, branch, path: null });
}
// Not a halt: the index is verified clean, or holds only the operator's own
// re-applied work (a successful `--index` pop), which the next merge
// autostashes again under the same config.
return { halt: false, warnings };
}
// #2596: the single definition of "this file is an executor-written SUMMARY
// artifact". Shared by `defaultFindSummaryFiles` (which walks for them to
// rescue) and the scope advisory below (which must never flag them) — a plan's
@@ -887,6 +1001,29 @@ const WAVE_CLEANUP_WARNING = Object.freeze({
SCOPE_OUT_OF_DECLARED: 'scope_out_of_declared',
/** The scope diff could not be computed, so conformance is unknown. */
SCOPE_CHECK_UNAVAILABLE: 'scope_check_unavailable',
/**
* #4721: a killed merge (at its budget, or by a signal) left this path
* staged in repoRoot's index with no MERGE_HEAD, and `git reset --merge`
* restored it to HEAD. Informational — repoRoot is clean again.
*/
MERGE_RESIDUE_RESTORED: 'merge_residue_restored',
/**
* #4721: a killed merge left this path staged in repoRoot's index with no
* MERGE_HEAD and it could NOT be restored (null path: the index could not
* be read at all) — or a failed autostash pop left it unmerged. repoRoot is
* dirty; committing from it would squash the executor's history into one
* parent. The wave halts.
*/
MERGE_RESIDUE_LEFT_STAGED: 'merge_residue_left_staged',
/**
* #4721: the killed merge had parked pre-existing work in MERGE_AUTOSTASH
* (`merge.autoStash`), and re-applying it failed or could not be verified.
* The work is in the stash list, not lost. Path is always null. On its own
* the index is clean and the wave continues; when a failed pop left
* unmerged entries it is accompanied by MERGE_RESIDUE_LEFT_STAGED rows and
* the wave halts.
*/
MERGE_AUTOSTASH_UNRESTORED: 'merge_autostash_unrestored',
});
interface WaveCleanupWarning {
@@ -1160,9 +1297,30 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
continue; // #2852: isolate
}
const merge = execGit(['merge', entry.branch, '--no-ff', '--no-edit', '-m', `chore: merge executor worktree (${entry.branch})`], { cwd: plan.repoRoot });
// #4721: the merge runs user hooks, so it carries its own budget — see
// DEFAULT_MERGE_TIMEOUT_MS. Every other call in this gauntlet keeps the
// module default.
const mergeTimeoutMs = deps.mergeTimeoutMs ?? DEFAULT_MERGE_TIMEOUT_MS;
const merge = execGit(
['merge', entry.branch, '--no-ff', '--no-edit', '-m', `chore: merge executor worktree (${entry.branch})`],
{ cwd: plan.repoRoot, timeout: mergeTimeoutMs },
);
if (!gitResultOk(merge)) {
blockEntry(result, 'merge_failed', merge?.stderr || merge?.stdout || '');
if (merge?.timedOut) {
// #4721: say "timeout" when it was one. The captured output is whatever
// the hook printed before git was killed, which read as a git error under
// the old `merge_failed` label and made a healthy executor branch look
// broken. The hook itself is a child of the killed git process and may
// still be running.
const partial = (merge.stderr || merge.stdout || '').trim();
blockEntry(
result,
'merge_timed_out',
`git merge did not finish within ${mergeTimeoutMs} ms and was killed (a merge hook such as pre-merge-commit may still be running; raise deps.mergeTimeoutMs or shorten the hook)${partial ? `; output before the kill: ${partial}` : ''}`,
);
} else {
blockEntry(result, 'merge_failed', merge?.stderr || merge?.stdout || '');
}
// #2852: a failed --no-ff merge MIGHT leave repoRoot itself mid-merge
// (MERGE_HEAD set, conflict markers in the tree) — unlike every other block
// reason above, that specific state is NOT scoped to this one entry: a second
@@ -1181,6 +1339,37 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
pending.push(...entries.slice(i + 1));
break;
}
// #4721: "no MERGE_HEAD" is not "tree never touched". A merge killed while
// its pre-merge-commit hook ran has already written the merged tree into
// repoRoot's index (and set ORIG_HEAD) but never got to write MERGE_HEAD,
// so the #2852 check above reads it as clean while the executor's whole
// diff sits staged against the old HEAD. `git merge --abort` cannot see
// that state either. Left alone, the next `git merge` in this wave would
// refuse ("your local changes would be overwritten") or, worse, an
// orchestrator that trusts the block reason and commits from repoRoot
// squashes the executor's history into one parent. Restore it; if that
// cannot be verified, halt the wave exactly as the mid-merge case does.
//
// ONLY when git was KILLED — at its budget, or by a signal from outside.
// A merge git REFUSED (no MERGE_HEAD either) leaves the index exactly as
// it found it — and "your local changes would be overwritten" is precisely
// the refusal a pre-existing dirty index earns, so on that path anything
// staged is the operator's own work and must not be touched (caught in
// review). A kill is the one shape that stages a tree git never finished
// with, and an external SIGTERM produces the same state as the timeout
// without `timedOut` (caught in review too). The seam normalizes a
// signal death to exitCode 1 and carries the signal alongside, so the
// signal — never the exit code — is the tell; a refused merge has none.
const mergeKilled = !!merge?.timedOut || !!merge?.signal;
if (mergeKilled) {
const residue = restoreMergeResidue(execGit, plan.repoRoot, entry.branch);
result.warnings.push(...residue.warnings);
allWarnings.push(...residue.warnings);
if (residue.halt) {
pending.push(...entries.slice(i + 1));
break;
}
}
continue; // #2852: isolate — repoRoot is not (or no longer) mid-merge
}
@@ -2617,6 +2806,7 @@ export = {
planWorktreeWaveCleanup,
executeWorktreeWaveCleanupPlan,
WAVE_CLEANUP_WARNING,
DEFAULT_MERGE_TIMEOUT_MS,
planWaveScopeConformance,
isSummaryArtifactRelPath,
cmdWorktreeCleanupWave,