From 25d1cb916f2959271ab3b233ccdf54e705a373bb Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Tue, 15 Sep 2026 21:28:09 -0500 Subject: [PATCH] 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) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9 * docs(#4721): add Fixed changeset Co-Authored-By: Claude Fable 5.1 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 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 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 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 Claude-Session: https://claude.ai/code/session_01TbbqrGJMuiuLftAMVLayb9 * chore(#4721): set changeset fragment pr to 4766 --------- Co-authored-by: Claude Fable 5.1 Co-authored-by: Tom Boucher --- .changeset/sunny-koalas-hop.md | 5 + CONTEXT.md | 2 +- docs/CLI-TOOLS.md | 12 + src/worktree-safety.cts | 194 ++++++++++++++- tests/worktree-safety.test.cjs | 416 ++++++++++++++++++++++++++++++++- 5 files changed, 625 insertions(+), 4 deletions(-) create mode 100644 .changeset/sunny-koalas-hop.md diff --git a/.changeset/sunny-koalas-hop.md b/.changeset/sunny-koalas-hop.md new file mode 100644 index 000000000..0e5eacec8 --- /dev/null +++ b/.changeset/sunny-koalas-hop.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4766 +--- +**`worktree cleanup-wave` no longer kills an executor merge at the 10-second git plumbing timeout while a `pre-merge-commit` hook runs** — the merge step now carries its own 10-minute budget (`deps.mergeTimeoutMs`), a merge that does exceed it blocks on a distinct `merge_timed_out` reason naming the budget instead of a `merge_failed` carrying the hook's partial output, and the staged-but-no-`MERGE_HEAD` index a killed merge leaves in the primary checkout is detected and restored with `git reset --merge` (reported per path as `merge_residue_restored`; `merge_residue_left_staged` halts the wave when it cannot be), so committing from the primary after a killed merge no longer squashes the executor's history. (#4721) diff --git a/CONTEXT.md b/CONTEXT.md index 14bca6b5c..66eeebf05 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -212,7 +212,7 @@ Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P Leaf module owning the **out-of-band** half of ADR-1411's "corrupt is not absent" amendment (epic #1879). Where a read already returns a provenance envelope the cause is named in-band (`ConfigResolution.reason`, #1880); where a read returns a bare sentinel or a plausible default it cannot extend, the return value is preserved exactly and the cause is surfaced here instead. Interface: `UNUSABLE_REASON` (frozen reason enum — one entry per condition that has an emitting call site; adding a reason is three coordinated changes: enum + call site + the test locking `Object.keys(...).sort()`), `warnUnusableInput({reason, source?, content?}) → boolean` (returns whether this call actually wrote, so tests assert emission *counts* on a typed surface rather than scraping stderr), plus the `_resetUnusableInputWarningsForTests` / `_unusableInputWarningCountForTests` seams. Dedup key is `\0` — **both halves are load-bearing**: keying on the path alone would let a second, different fault on the same file go unreported, and keying on message prose would couple the guard to wording (ADR-1411 dedup clause). Path separators are deliberately **not** normalized: an earlier revision folded backslashes to `/` so two spellings of one Windows path would not double-report, but a backslash is a legal filename character on Linux and macOS, so that folding collapsed two genuinely distinct POSIX files onto one key and swallowed the second file's diagnostic. The trade is now one-directional — two spellings of one Windows path may report twice (noise), but two distinct files can never silence each other (lost signal), and ADR-1411 ranks the swallow the worse failure; ASCII control characters are stripped from the source before it is keyed or written, because the key separator is NUL (a crafted path could otherwise forge a collision) and because a path carrying ANSI escapes would replay into the operator's terminal. Callers with no path (in-memory content) fall back to a short content digest so *different* bad inputs still key differently. The diagnostic is **unconditional** — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since "an opt-in nobody sets is indistinguishable from the silence #1879 is about" — and **never throws**: a failed stderr write is swallowed so a degraded read is never escalated into a crash. Adopted by `extractFrontmatter` (#1882, `frontmatter_unterminated`) and by `getRoadmapPhaseInternal`/`getMilestoneInfo` (#1881, `roadmap_unreadable`); `planning-workspace`/`verify` (#1883) follow. Tenth site: `cmdMigrateConfig` (Config CRUD Module) and `loadConfigResolved` (Config Loader Module) emit `config_section_not_object` when a legacy-key migration's destination section holds a non-object (#3760). The detecting module, `configuration.cjs`, deliberately does NOT emit: it reports in-band as `skipped[]` and its callers — which hold both the resolved path and an unconstrained dependency budget — do the emitting, because `configuration.cjs` must stay loadable with no sibling requires under the #3571 install-layout contract. #1881 detects on the errno alone: `platformReadSync` returns `null` for ENOENT and its callers convert that to an errno-less Error, so reporting unconditionally in those catches would flag every project without a ROADMAP.md as corrupt. Exists as a shared seam rather than a per-site copy because four sites need identical behavior and four hand-rolled copies is `RULESET.GENERATIVE-FIX` by construction. Source of truth: `gsd-core/bin/lib/unusable-input.cjs` (generated from `src/unusable-input.cts`). Test anchor: `tests/unusable-input.test.cjs`. See Resolution Provenance, Config Loader Module. ### Worktree Safety Policy Module -CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult` (per-entry gauntlet: branch → base → deletions → **advisory scope conformance (#2596)** → SUMMARY-rescue → clean-worktree → merge → remove; the scope check compares the branch's committed diff against the entry's declared `files_modified` and appends `WAVE_CLEANUP_WARNING`-coded entries to a `warnings` channel WITHOUT touching `ok` — an advisory, not a gate, and skipped entirely with no git call when no scope was declared), `planWaveScopeConformance(changedPaths, declaredFiles, branch) → WaveCleanupWarning[]` (pure; literal-prefix path coverage deliberately mirroring the submodule-intersection gate's glob-prefix rule rather than introducing a second matcher; over-accepts by design because a false alarm costs an advisory more than a miss), `isSummaryArtifactRelPath(relPath) → boolean` (the single definition of "executor-written SUMMARY artifact", shared with `defaultFindSummaryFiles` so the rescue walker and the scope exemption cannot drift), `WAVE_CLEANUP_WARNING` (frozen advisory-code enum: `scope_out_of_declared`, `scope_check_unavailable`), `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b `; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: requires `--root` — confinement is mandatory, not opt-in; omitting it fails closed with `reason:'root_required'` before any git side effect, rather than silently creating an unconfined worktree (#3050); plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — consumed since #2584 Phase 3 — `executor-isolation-dispatch.md` calls it to create the worktree an `orchestrator-worktree` host is then process-spawned into. `worktree record-agent` / `worktree create` accept an optional `--files` recording the plan's declared scope, consumed by the advisory scope-conformance check above; a blank or omitted value leaves the 4-field on-disk entry shape unchanged. Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps) → {root, reason}` (a projection over `resolveWorktreeContext`; returns the `reason` alongside `root` — a `git_timed_out` reason means `root` is a best-effort cwd fallback, not a confirmed resolution, and callers must surface that risk rather than trust it silently, #3050) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. +CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult` (per-entry gauntlet: branch → base → deletions → **advisory scope conformance (#2596)** → SUMMARY-rescue → clean-worktree → merge → remove; the merge is the one call that runs the commit-family hooks, so it carries its own budget — `deps.mergeTimeoutMs`, default `DEFAULT_MERGE_TIMEOUT_MS` (10 min) — instead of the module's 10 s plumbing timeout, and a merge killed at that budget blocks on `merge_timed_out` naming the budget rather than on `merge_failed` carrying the hook's partial output (#4721); the scope check compares the branch's committed diff against the entry's declared `files_modified` and appends `WAVE_CLEANUP_WARNING`-coded entries to a `warnings` channel WITHOUT touching `ok` — an advisory, not a gate, and skipped entirely with no git call when no scope was declared), `planWaveScopeConformance(changedPaths, declaredFiles, branch) → WaveCleanupWarning[]` (pure; literal-prefix path coverage deliberately mirroring the submodule-intersection gate's glob-prefix rule rather than introducing a second matcher; over-accepts by design because a false alarm costs an advisory more than a miss), `isSummaryArtifactRelPath(relPath) → boolean` (the single definition of "executor-written SUMMARY artifact", shared with `defaultFindSummaryFiles` so the rescue walker and the scope exemption cannot drift), `WAVE_CLEANUP_WARNING` (frozen advisory-code enum: `scope_out_of_declared`, `scope_check_unavailable`, and — #4721 — `merge_residue_restored` / `merge_residue_left_staged`, emitted per path when a killed merge (at its budget, or by a signal) left repoRoot's index staged with no `MERGE_HEAD`: the wave runs `git reset --merge` and continues on a verified-clean index, or halts the remaining entries when the index stays dirty or cannot be read, because a dirty primary index is repo-level state exactly as an unfinished merge is; gated on the KILL, since a merge git refused exits with a code and leaves a pre-existing dirty index untouched — that index is the operator's own work; plus `merge_autostash_unrestored` (`path: null`) when a `merge.autoStash`-parked stash could not be re-applied after the reset — the work stays in the stash list, and the wave continues only after re-reading the index clean, halting on conflict entries a failed pop left behind), `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b `; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: requires `--root` — confinement is mandatory, not opt-in; omitting it fails closed with `reason:'root_required'` before any git side effect, rather than silently creating an unconfined worktree (#3050); plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — consumed since #2584 Phase 3 — `executor-isolation-dispatch.md` calls it to create the worktree an `orchestrator-worktree` host is then process-spawned into. `worktree record-agent` / `worktree create` accept an optional `--files` recording the plan's declared scope, consumed by the advisory scope-conformance check above; a blank or omitted value leaves the 4-field on-disk entry shape unchanged. Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps) → {root, reason}` (a projection over `resolveWorktreeContext`; returns the `reason` alongside `root` — a `git_timed_out` reason means `root` is a best-effort cwd fallback, not a confirmed resolution, and callers must surface that risk rather than trust it silently, #3050) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. ### Worktree Lifecycle Module Workflow contract seam covering agent worktree lifecycle orchestration rules. The `worktree_branch_check` block lives in one canonical fragment (`gsd-core/references/worktree-branch-check.md`) that `execute-phase.md`, `quick.md`, `diagnose-issues.md`, and `execute-plan.md` embed at dispatch. Key invariants: `worktree_branch_check` is **verify-only and fail-closed** — the orchestrator owns worktree lifecycle and base recovery, so the sub-agent holds no state-correction primitives; HEAD attachment verified via `git symbolic-ref`; positive allow-list `^worktree-agent-*` enforced; `git update-ref` on protected refs is prohibited; on base mismatch the sub-agent halts with `exit 42` and surfaces to the orchestrator (#48); the orchestrator runs a cwd-drift guard at `execute_waves` entry that resolves the worktree root and refuses drift into an agent worktree (#48); #1856: that refusal now also reports what the agent worktree holds — commits ahead of the resolved base and uncommitted files, both with true counts plus a truncation notice — and the commit/switch/merge-or-cherry-pick sequence to integrate them, because `re-run from the orchestrator worktree` alone silently meant abandoning work that lives only on the agent branch. The refusal condition and exit code are unchanged, and every added command is diagnostic and `|| true`-guarded so a failure degrades to the plain refusal; cleanup is manifest-scoped (`WAVE_WORKTREE_MANIFEST`) not global-discovery-based; worktree spawning is sequential (one `run_in_background` at a time to avoid `config.lock` contention). Test anchor: `tests/worktree.test.cjs`. diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index a37f07e5c..14af1f993 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1365,6 +1365,18 @@ This is advisory: it does not change `ok`, `reason`, the per-entry `status`, or Two deliberate limits keep it from crying wolf. `.planning/**/*SUMMARY.md` paths are always exempt — the executor writes a SUMMARY by orchestration contract and no plan declares it. Glob patterns are matched by their literal prefix only, so `src/**/*.ts` covers everything under `src/`, and a pattern with no literal prefix (`*.md`) suppresses warnings for that entry rather than reporting every file. +**Merge timeout and a killed merge's residue (#4721)** + +The merge step is the one git call in `cleanup-wave` that runs the commit-family hooks (`pre-merge-commit`, `prepare-commit-msg`, `commit-msg`, `post-merge`), so it runs under its own budget — 10 minutes by default (`DEFAULT_MERGE_TIMEOUT_MS`; `deps.mergeTimeoutMs` for callers of the module) — rather than the 10-second timeout every other git call in the wave keeps. (`worktree add` runs `post-checkout` and every ref update runs `reference-transaction`; those are plumbing-cheap and stay on the default.) A repo whose pre-merge hook is a test-suite gate therefore merges instead of being killed mid-hook. + +When the merge does exceed its budget the entry blocks on `reason: "merge_timed_out"`, and its `stderr` names the budget, says the hook may still be running, and labels whatever the hook had printed as output before the kill — instead of the old `merge_failed`, which carried that partial output as though it were git's own error. `merge_failed` is otherwise unchanged — a merge git refused, one that conflicted, or one killed by a signal from outside (which the seam reports with the signal, not as a timeout; that case still takes the restore below). + +A merge killed while its hook runs has already staged the merged tree into the primary checkout's index but never wrote `MERGE_HEAD`, so `git merge --abort` finds nothing and the mid-merge check (#2852) reads the primary as clean. Left there, a plain `git commit` from the primary would squash the executor's history into a single-parent commit. After a merge that was **killed** — at its budget, or by a signal from outside — and only then, `cleanup-wave` reads the index, runs `git reset --merge` (which restores exactly the paths the merge staged and keeps unrelated unstaged edits), and re-reads it. Each restored path is reported as a `code: "merge_residue_restored"` warning and the wave continues. If the index is still dirty afterwards, or cannot be read at all, each remaining path (or a single `path: null` when the read itself failed) is reported as `code: "merge_residue_left_staged"` and the remaining entries are moved to `pending` — the same repo-level halt an unfinished merge triggers, because every later merge would run against that dirty index. + +The kill gate is what makes the staged set attributable to the merge. A merge git *refuses* (`error: Your local changes to the following files would be overwritten by merge`) is the immediate exit a pre-existing dirty index earns, and it leaves that index untouched; on that path nothing is read or reset, because anything staged is your own work. The one exception is `merge.autoStash`: git then parks your staged work in `MERGE_AUTOSTASH` and starts anyway, and a merge killed before `MERGE_HEAD` exists never re-applies it. `git reset --merge` moves that autostash into the stash list, and `cleanup-wave` then runs `git stash pop --index` to put it back with its staged state intact. If the pop fails, or the autostash state could not be determined, the entry carries a `code: "merge_autostash_unrestored"` warning (`path: null`) and your work stays in `git stash list`; the index is then re-read, and the wave continues only if it is clean — a pop that left conflict entries behind halts the remaining entries as `merge_residue_left_staged`, since the next merge would fail on unmerged files. A kill that lands once `MERGE_HEAD` exists (inside `commit-msg`, say) is the ordinary abort path: `git merge --abort` restores the tree and re-applies an autostash itself — unstaged, as git does for any aborted autostashed merge — and the residue step finds nothing to do. + +Known limit: the kill terminates `git`, not the hook process it spawned. A hook that keeps running and itself stages files after the wave has verified the index clean can re-dirty the primary; the `merge_timed_out` detail says so, and a hook that takes minutes belongs under a larger `mergeTimeoutMs`, not under this recovery. + --- ## Graphify diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index 670464bff..0d3e4fd1a 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -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_- naming convention // (claude-orchestration's isolation:"worktree" emission) alongside the // existing agent- / worktree-agent- 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): { 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, diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index 64010364d..d0db8892f 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -2510,6 +2510,420 @@ describe('executeWorktreeWaveCleanupPlan', () => { assert.deepEqual(result.pending.map((entry) => entry.branch), ['worktree-agent-a2']); }); + // #4721: the wave's `git merge --no-ff` is the one call in the gauntlet that runs + // user hooks, and it used to inherit the module's 10 s plumbing timeout. A repo + // whose `pre-merge-commit` hook is a test-suite gate lost every executor merge: + // the kill was reported as a plain `merge_failed` carrying the hook's partial + // stdout, and — the dangerous half — it landed after git had staged the merged + // tree but before it wrote MERGE_HEAD, so the #2852 mid-merge check read the + // primary as clean while the executor's whole diff sat staged against the old + // HEAD. Committing from that state squashes the executor's history. + + const fs = require('node:fs'); + const WAVE_WARNING = require(WORKTREE_SAFETY_PATH).WAVE_CLEANUP_WARNING; + const MERGE_BUDGET_DEFAULT = require(WORKTREE_SAFETY_PATH).DEFAULT_MERGE_TIMEOUT_MS; + + function mergeGauntletStub(overrides = {}) { + // A one-entry gauntlet whose every call before the merge succeeds; callers + // override the merge and the post-failure calls per row. Unknown calls throw so + // a row cannot pass by accident on a call it never modelled. + return (args) => { + const key = args.join(' '); + if (Object.prototype.hasOwnProperty.call(overrides, key)) return overrides[key](args); + for (const prefix of Object.keys(overrides)) { + if (prefix.endsWith('*') && key.startsWith(prefix.slice(0, -1))) return overrides[prefix](args); + } + if (key === '-C /repo/.claude/worktrees/agent-a1 rev-parse --abbrev-ref HEAD') return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '' }; + if (key === 'merge-base HEAD worktree-agent-a1') return { exitCode: 0, stdout: 'abc123', stderr: '' }; + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a1') return { exitCode: 0, stdout: '', stderr: '' }; + if (key === '-C /repo/.claude/worktrees/agent-a1 status --porcelain --untracked-files=all') return { exitCode: 0, stdout: '', stderr: '' }; + if (key === '-C /repo/.claude/worktrees/agent-a2 rev-parse --abbrev-ref HEAD') return { exitCode: 0, stdout: 'worktree-agent-a2', stderr: '' }; + if (key === 'merge-base HEAD worktree-agent-a2') return { exitCode: 0, stdout: 'abc123', stderr: '' }; + if (key === 'diff --diff-filter=D --name-only HEAD...worktree-agent-a2') return { exitCode: 0, stdout: '', stderr: '' }; + if (key === '-C /repo/.claude/worktrees/agent-a2 status --porcelain --untracked-files=all') return { exitCode: 0, stdout: '', stderr: '' }; + if (key.startsWith('merge worktree-agent-a2')) return { exitCode: 0, stdout: '', stderr: '' }; + if (key === 'worktree remove /repo/.claude/worktrees/agent-a2 --force') return { exitCode: 0, stdout: '', stderr: '' }; + if (key === 'branch -D worktree-agent-a2') return { exitCode: 0, stdout: '', stderr: '' }; + throw new Error(`unexpected git call: ${key}`); + }; + } + + const twoEntryPlan = () => ({ + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [ + { agent_id: 'a1', worktree_path: '/repo/.claude/worktrees/agent-a1', branch: 'worktree-agent-a1', expected_base: 'abc123' }, + { agent_id: 'a2', worktree_path: '/repo/.claude/worktrees/agent-a2', branch: 'worktree-agent-a2', expected_base: 'abc123' }, + ], + }); + + // The kill lands after the merged tree is staged and before MERGE_HEAD is written, + // so `git merge --abort` finds nothing and the MERGE_HEAD probe says "clean". + const killedMidHook = { + 'merge worktree-agent-a1*': () => ({ + exitCode: null, + stdout: 'pre-merge-commit: slow gate starting (sleep 15)\n', + stderr: '', + timedOut: true, + signal: 'SIGTERM', + error: Object.assign(new Error('spawnSync git ETIMEDOUT'), { code: 'ETIMEDOUT' }), + }), + 'merge --abort': () => ({ exitCode: 128, stdout: '', stderr: 'fatal: There is no merge to abort (MERGE_HEAD missing)?' }), + 'rev-parse --verify -q MERGE_HEAD': () => ({ exitCode: 1, stdout: '', stderr: '' }), + 'rev-parse --verify -q MERGE_AUTOSTASH': () => ({ exitCode: 1, stdout: '', stderr: '' }), + }; + + test('#4721: the merge carries its own budget; every other call in the gauntlet keeps the module default', () => { + const seen = []; + const plan = twoEntryPlan(); + plan.entries.pop(); + executeWorktreeWaveCleanupPlan(plan, { + execGit: (args, opts) => { + seen.push({ key: args.join(' '), timeout: opts && opts.timeout }); + return mergeGauntletStub({ + 'merge worktree-agent-a1*': () => ({ exitCode: 0, stdout: '', stderr: '' }), + 'worktree remove /repo/.claude/worktrees/agent-a1 --force': () => ({ exitCode: 0, stdout: '', stderr: '' }), + 'branch -D worktree-agent-a1': () => ({ exitCode: 0, stdout: '', stderr: '' }), + })(args); + }, + }); + const merge = seen.filter((c) => c.key.startsWith('merge worktree-agent-a1')); + assert.equal(merge.length, 1); + assert.equal(merge[0].timeout, MERGE_BUDGET_DEFAULT, 'the merge must pass an explicit budget, not inherit the plumbing default'); + assert.ok(MERGE_BUDGET_DEFAULT >= 60_000, 'the merge budget is sized for a hook, not for plumbing'); + for (const other of seen.filter((c) => !c.key.startsWith('merge worktree-agent-a1'))) { + assert.equal(other.timeout, undefined, `${other.key} must keep the module default — only the merge runs hooks`); + } + + // And the budget is a dep, so a caller can size it to its hooks. + const seenOverride = []; + executeWorktreeWaveCleanupPlan(plan, { + mergeTimeoutMs: 4242, + execGit: (args, opts) => { + seenOverride.push({ key: args.join(' '), timeout: opts && opts.timeout }); + return mergeGauntletStub({ + 'merge worktree-agent-a1*': () => ({ exitCode: 0, stdout: '', stderr: '' }), + 'worktree remove /repo/.claude/worktrees/agent-a1 --force': () => ({ exitCode: 0, stdout: '', stderr: '' }), + 'branch -D worktree-agent-a1': () => ({ exitCode: 0, stdout: '', stderr: '' }), + })(args); + }, + }); + assert.equal(seenOverride.find((c) => c.key.startsWith('merge worktree-agent-a1')).timeout, 4242); + }); + + test('#4721: a merge killed at its budget is blocked as merge_timed_out, its staged residue is restored, and the wave continues', () => { + const calls = []; + const result = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => { + calls.push(args.join(' ')); + let cachedReads = 0; + return mergeGauntletStub({ + ...killedMidHook, + 'diff --cached --name-only': () => { + // First read: the executor's tree, staged against the old HEAD. Second + // read (after `reset --merge`): clean. + cachedReads = calls.filter((c) => c === 'diff --cached --name-only').length; + return cachedReads === 1 + ? { exitCode: 0, stdout: 'a.txt\nb.txt\n', stderr: '' } + : { exitCode: 0, stdout: '', stderr: '' }; + }, + 'reset --merge': () => ({ exitCode: 0, stdout: '', stderr: '' }), + })(args); + }, + }); + assert.equal(result.ok, false); + assert.equal(result.entries[0].status, 'blocked'); + assert.equal(result.entries[0].reason, 'merge_timed_out', 'a timeout is not a merge_failed'); + assert.notEqual(result.entries[0].reason, 'merge_failed'); + assert.deepEqual( + result.entries[0].warnings, + [ + { code: WAVE_WARNING.MERGE_RESIDUE_RESTORED, branch: 'worktree-agent-a1', path: 'a.txt' }, + { code: WAVE_WARNING.MERGE_RESIDUE_RESTORED, branch: 'worktree-agent-a1', path: 'b.txt' }, + ], + 'every path the killed merge left staged is named as restored', + ); + assert.equal(result.warnings.length, 2, 'residue warnings are aggregated on the wave result too'); + assert.ok(calls.includes('reset --merge'), 'the staged residue is undone with git reset --merge'); + assert.ok(calls.indexOf('reset --merge') > calls.indexOf('rev-parse --verify -q MERGE_HEAD'), 'the residue check runs only once the repo is known not to be mid-merge'); + assert.equal(result.entries[1].status, 'merged_removed', 'entry 2 still merges — repoRoot was restored to clean'); + assert.deepEqual(result.pending, []); + }); + + test('#4721: a merge git REFUSED never reads or resets the index — a pre-existing dirty index is the operator\'s work', () => { + // Caught in review: a refusal ("your local changes would be overwritten") is + // exactly what a pre-existing dirty index earns, and it leaves that index as it + // was. Attributing it to the merge and running `reset --merge` would discard + // the operator's staged work. The residue path is gated on the TIMEOUT. + const calls = []; + const result = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => { + calls.push(args.join(' ')); + return mergeGauntletStub({ + 'merge worktree-agent-a1*': () => ({ exitCode: 1, stdout: '', stderr: 'error: Your local changes to the following files would be overwritten by merge:\n d.txt' }), + 'merge --abort': () => ({ exitCode: 128, stdout: '', stderr: 'fatal: There is no merge to abort (MERGE_HEAD missing)?' }), + 'rev-parse --verify -q MERGE_HEAD': () => ({ exitCode: 1, stdout: '', stderr: '' }), + // The stub throws on any call not modelled here — so a `diff --cached` + // or `reset --merge` on this path fails the test loudly. + })(args); + }, + }); + assert.equal(result.entries[0].reason, 'merge_failed'); + assert.deepEqual(result.entries[0].warnings, []); + assert.equal(calls.some((c) => c.startsWith('diff --cached')), false, 'a refused merge does not read the index'); + assert.equal(calls.includes('reset --merge'), false, 'and never resets it'); + assert.equal(result.entries[1].status, 'merged_removed'); + }); + + test('#4721: a killed merge that had autostashed pre-existing work re-applies it after the reset', () => { + const calls = []; + const result = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => { + calls.push(args.join(' ')); + return mergeGauntletStub({ + ...killedMidHook, + // merge.autoStash parked the operator's staged edit here and started anyway. + 'rev-parse --verify -q MERGE_AUTOSTASH': () => ({ exitCode: 0, stdout: 'c0ffee00c0ffee00c0ffee00c0ffee00c0ffee00\n', stderr: '' }), + 'diff --cached --name-only': () => (calls.filter((c) => c === 'diff --cached --name-only').length === 1 + ? { exitCode: 0, stdout: 'b.txt\n', stderr: '' } + : { exitCode: 0, stdout: '', stderr: '' }), + 'reset --merge': () => ({ exitCode: 0, stdout: '', stderr: 'Autostash exists; creating a new stash entry.' }), + 'stash pop --index': () => ({ exitCode: 0, stdout: '', stderr: '' }), + })(args); + }, + }); + assert.equal(result.entries[0].reason, 'merge_timed_out'); + assert.deepEqual(result.entries[0].warnings, [{ code: WAVE_WARNING.MERGE_RESIDUE_RESTORED, branch: 'worktree-agent-a1', path: 'b.txt' }]); + assert.ok(calls.indexOf('stash pop --index') > calls.indexOf('reset --merge'), 'the autostash is re-applied AFTER the reset parks it in the stash list'); + assert.equal(result.entries[1].status, 'merged_removed'); + + // Clean index, autostash present (killed before the index was populated): still reset + pop. + // The pop fails but leaves the index clean → reported, wave continues. + const calls2 = []; + const result2 = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => { + calls2.push(args.join(' ')); + return mergeGauntletStub({ + ...killedMidHook, + 'rev-parse --verify -q MERGE_AUTOSTASH': () => ({ exitCode: 0, stdout: 'c0ffee00c0ffee00c0ffee00c0ffee00c0ffee00\n', stderr: '' }), + 'diff --cached --name-only': () => ({ exitCode: 0, stdout: '', stderr: '' }), + 'reset --merge': () => ({ exitCode: 0, stdout: '', stderr: '' }), + 'stash pop --index': () => ({ exitCode: 1, stdout: '', stderr: 'error: could not restore untracked files from stash' }), + })(args); + }, + }); + assert.ok(calls2.includes('reset --merge') && calls2.includes('stash pop --index')); + assert.equal(calls2.filter((c) => c === 'diff --cached --name-only').length, 3, 'the index is re-read after a failed pop'); + assert.deepEqual(result2.entries[0].warnings, [{ code: WAVE_WARNING.MERGE_AUTOSTASH_UNRESTORED, branch: 'worktree-agent-a1', path: null }], 'a failed pop is reported, never silent'); + assert.equal(result2.entries[1].status, 'merged_removed', 'the index is clean, so the wave continues'); + + // A failed pop that leaves conflict entries behind is NOT clean — halt (caught in review: + // the next merge would fail on "you have unmerged files"). + const calls3 = []; + const result3 = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => { + calls3.push(args.join(' ')); + return mergeGauntletStub({ + ...killedMidHook, + 'rev-parse --verify -q MERGE_AUTOSTASH': () => ({ exitCode: 0, stdout: 'c0ffee00c0ffee00c0ffee00c0ffee00c0ffee00\n', stderr: '' }), + 'diff --cached --name-only': () => (calls3.filter((c) => c === 'diff --cached --name-only').length === 3 + ? { exitCode: 0, stdout: 'a.txt\nb.txt\n', stderr: '' } + : { exitCode: 0, stdout: '', stderr: '' }), + 'reset --merge': () => ({ exitCode: 0, stdout: '', stderr: '' }), + 'stash pop --index': () => ({ exitCode: 1, stdout: '', stderr: 'CONFLICT (content): Merge conflict in a.txt' }), + })(args); + }, + }); + assert.deepEqual(result3.entries[0].warnings, [ + { code: WAVE_WARNING.MERGE_AUTOSTASH_UNRESTORED, branch: 'worktree-agent-a1', path: null }, + { code: WAVE_WARNING.MERGE_RESIDUE_LEFT_STAGED, branch: 'worktree-agent-a1', path: 'a.txt' }, + { code: WAVE_WARNING.MERGE_RESIDUE_LEFT_STAGED, branch: 'worktree-agent-a1', path: 'b.txt' }, + ]); + assert.equal(result3.entries.length, 1, 'entry 2 must not merge over unmerged entries'); + assert.deepEqual(result3.pending.map((entry) => entry.branch), ['worktree-agent-a2']); + }); + + test('#4721: a merge killed by an external signal (no timedOut) takes the same restore path as a timeout', () => { + // The seam (`_spawnResult`) normalizes a signal death to exitCode 1 and carries + // the signal alongside, timedOut false — so the exit code is NOT the tell, the + // signal is (a refused merge has none). The index state a SIGTERM leaves is + // identical to the timeout's, so the residue path keys on "killed", not on + // "timed out" (caught in review, twice: first the gate, then the shape). + const calls = []; + const result = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => { + calls.push(args.join(' ')); + return mergeGauntletStub({ + ...killedMidHook, + 'merge worktree-agent-a1*': () => ({ exitCode: 1, stdout: '', stderr: '', timedOut: false, signal: 'SIGTERM', error: null }), + 'diff --cached --name-only': () => (calls.filter((c) => c === 'diff --cached --name-only').length === 1 + ? { exitCode: 0, stdout: 'b.txt\n', stderr: '' } + : { exitCode: 0, stdout: '', stderr: '' }), + 'reset --merge': () => ({ exitCode: 0, stdout: '', stderr: '' }), + })(args); + }, + }); + assert.equal(result.entries[0].reason, 'merge_failed', 'not a timeout — the reason stays merge_failed'); + assert.deepEqual(result.entries[0].warnings, [{ code: WAVE_WARNING.MERGE_RESIDUE_RESTORED, branch: 'worktree-agent-a1', path: 'b.txt' }]); + assert.ok(calls.includes('reset --merge')); + assert.equal(result.entries[1].status, 'merged_removed'); + }); + + test('#4721: residue that git reset --merge cannot clear is reported as left staged and halts the wave', () => { + const result = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => mergeGauntletStub({ + ...killedMidHook, + // Still `b.txt` on both reads: the reset refused (an unstaged edit overlaps), + // or ran but left it. + 'diff --cached --name-only': () => ({ exitCode: 0, stdout: 'b.txt\n', stderr: '' }), + 'reset --merge': () => ({ exitCode: 1, stdout: '', stderr: 'error: Entry \'b.txt\' not uptodate. Cannot merge.' }), + })(args), + }); + assert.equal(result.entries[0].reason, 'merge_timed_out'); + assert.deepEqual( + result.entries[0].warnings, + [{ code: WAVE_WARNING.MERGE_RESIDUE_LEFT_STAGED, branch: 'worktree-agent-a1', path: 'b.txt' }], + ); + assert.equal(result.entries.length, 1, 'entry 2 must not be evaluated against a dirty index'); + assert.deepEqual(result.pending.map((entry) => entry.branch), ['worktree-agent-a2']); + }); + + test('#4721: an unverifiable index after merge_failed fails closed — null path, wave halted', () => { + const result = executeWorktreeWaveCleanupPlan(twoEntryPlan(), { + execGit: (args) => mergeGauntletStub({ + ...killedMidHook, + 'diff --cached --name-only': makeTimeoutStub(), + })(args), + }); + assert.deepEqual( + result.entries[0].warnings, + [{ code: WAVE_WARNING.MERGE_RESIDUE_LEFT_STAGED, branch: 'worktree-agent-a1', path: null }], + 'a null path marks "the check itself could not run", as scope_check_unavailable does', + ); + assert.equal(result.entries.length, 1); + assert.deepEqual(result.pending.map((entry) => entry.branch), ['worktree-agent-a2']); + }); + + describe('#4721: real git — a pre-merge-commit hook slower than the merge budget', { skip: isWindows ? 'POSIX sh hook' : false }, () => { + const { gitOrThrow: gitFixture } = require('./helpers/git-fixture.cjs'); + const HOOK_SLEEP_S = 4; + const KILL_BUDGET_MS = 1500; + + function buildRepoWithSlowHook({ hook = true, hookName = 'pre-merge-commit', autoStash = false, prestage = false } = {}) { + const root = createTempDir('gsd-4721-'); + const repo = path.join(root, 'rr'); + const wt = path.join(root, 'rr-wt'); + const git = (args, cwd = repo) => gitFixture(args, { cwd, timeoutMs: SUBPROCESS_TIMEOUT_MS }); + fs.mkdirSync(repo); + git(['init', '-q', '-b', 'main']); + git(['config', 'user.email', 'test@example.com']); + git(['config', 'user.name', 'test']); + git(['config', 'commit.gpgsign', 'false']); + fs.writeFileSync(path.join(repo, 'a.txt'), 'base\n'); + git(['add', 'a.txt']); + git(['commit', '-q', '-m', 'base']); + if (hook) { + const hookPath = path.join(repo, '.git', 'hooks', hookName); + fs.writeFileSync(hookPath, `#!/bin/sh\necho "${hookName}: slow gate starting"\nsleep ${HOOK_SLEEP_S}\nexit 0\n`, { mode: 0o755 }); + } + if (autoStash) git(['config', 'merge.autoStash', 'true']); + git(['worktree', 'add', '-q', '-b', 'agent-repro1', wt]); + fs.writeFileSync(path.join(wt, 'b.txt'), 'change\n'); + fs.appendFileSync(path.join(wt, 'a.txt'), 'executor line\n'); + git(['add', 'a.txt', 'b.txt'], wt); + git(['commit', '-q', '-m', 'executor: add b.txt'], wt); + const base = git(['rev-parse', 'HEAD']).trim(); + if (prestage) { + // The operator's own staged work in the primary, unrelated to the branch. + fs.appendFileSync(path.join(repo, 'a.txt'), 'operator staged line\n'); + fs.writeFileSync(path.join(repo, 'ops.txt'), 'precious\n'); + git(['add', 'a.txt', 'ops.txt']); + } + const plan = { + ok: true, + repoRoot: repo, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ agent_id: 'repro1', worktree_path: wt, branch: 'agent-repro1', expected_base: base }], + }; + return { root, repo, wt, base, plan, git }; + } + + test('is blocked as merge_timed_out, leaves no MERGE_HEAD, and repoRoot ends with a clean index at the old HEAD', (t) => { + const fx = buildRepoWithSlowHook(); + t.after(() => cleanup(fx.root)); + const result = executeWorktreeWaveCleanupPlan(fx.plan, { mergeTimeoutMs: KILL_BUDGET_MS }); + assert.equal(result.ok, false); + assert.equal(result.entries[0].reason, 'merge_timed_out'); + assert.equal(fx.git(['rev-parse', 'HEAD']).trim(), fx.base, 'HEAD unmoved'); + assert.equal(fx.git(['diff', '--cached', '--name-only']).trim(), '', 'the killed merge\'s staged tree was restored'); + assert.equal(fx.git(['status', '--porcelain']).trim(), '', 'worktree clean too'); + assert.equal(fs.readFileSync(path.join(fx.repo, 'a.txt'), 'utf8'), 'base\n'); + assert.equal(fs.existsSync(path.join(fx.repo, 'b.txt')), false, 'the merge-added file is gone from the primary'); + assert.deepEqual( + result.entries[0].warnings.map((w) => w.code), + [WAVE_WARNING.MERGE_RESIDUE_RESTORED, WAVE_WARNING.MERGE_RESIDUE_RESTORED], + ); + assert.deepEqual(result.entries[0].warnings.map((w) => w.path).sort(), ['a.txt', 'b.txt']); + assert.equal(fs.existsSync(path.join(fx.wt, 'b.txt')), true, 'the executor branch and its worktree are untouched'); + }); + + test('negative control: the same hook under the default budget merges cleanly', (t) => { + const fx = buildRepoWithSlowHook(); + t.after(() => cleanup(fx.root)); + const result = executeWorktreeWaveCleanupPlan(fx.plan); + assert.equal(result.ok, true, JSON.stringify(result)); + assert.equal(result.entries[0].status, 'merged_removed'); + assert.deepEqual(result.entries[0].warnings, []); + assert.equal(fx.git(['rev-list', '--count', 'HEAD']).trim(), '3', 'base + executor + merge commit: history preserved, not squashed'); + assert.equal(fs.readFileSync(path.join(fx.repo, 'b.txt'), 'utf8'), 'change\n'); + }); + + test('a merge git refuses because the primary index is dirty leaves the operator\'s staged work exactly as it was', (t) => { + // The review-caught case, on real git: no hook, pre-existing staged work in the + // primary. git refuses instantly; nothing may be reset. + const fx = buildRepoWithSlowHook({ hook: false, prestage: true }); + t.after(() => cleanup(fx.root)); + const result = executeWorktreeWaveCleanupPlan(fx.plan, { mergeTimeoutMs: KILL_BUDGET_MS }); + assert.equal(result.entries[0].reason, 'merge_failed'); + assert.deepEqual(result.entries[0].warnings, []); + assert.deepEqual(fx.git(['diff', '--cached', '--name-only']).trim().split('\n').sort(), ['a.txt', 'ops.txt'], 'the operator\'s staged set is untouched'); + assert.equal(fs.readFileSync(path.join(fx.repo, 'ops.txt'), 'utf8'), 'precious\n'); + assert.equal(fs.existsSync(path.join(fx.repo, 'b.txt')), false); + }); + + test('a killed merge under merge.autoStash restores the executor residue AND re-applies the operator\'s autostashed work', (t) => { + const fx = buildRepoWithSlowHook({ autoStash: true, prestage: true }); + t.after(() => cleanup(fx.root)); + const result = executeWorktreeWaveCleanupPlan(fx.plan, { mergeTimeoutMs: KILL_BUDGET_MS }); + assert.equal(result.entries[0].reason, 'merge_timed_out'); + assert.deepEqual(result.entries[0].warnings.map((w) => w.code), [WAVE_WARNING.MERGE_RESIDUE_RESTORED, WAVE_WARNING.MERGE_RESIDUE_RESTORED], 'no autostash warning — the pop succeeded'); + assert.equal(fx.git(['rev-parse', 'HEAD']).trim(), fx.base); + assert.equal(fs.existsSync(path.join(fx.repo, 'b.txt')), false, 'executor residue gone'); + assert.deepEqual(fx.git(['diff', '--cached', '--name-only']).trim().split('\n').sort(), ['a.txt', 'ops.txt'], 'the operator\'s staged work is back in the index'); + assert.equal(fs.readFileSync(path.join(fx.repo, 'ops.txt'), 'utf8'), 'precious\n'); + assert.equal(fs.readFileSync(path.join(fx.repo, 'a.txt'), 'utf8'), 'base\noperator staged line\n', 'a.txt carries the operator line, not the executor line'); + assert.equal(fx.git(['stash', 'list']).trim(), '', 'nothing left parked in the stash'); + assert.equal(fs.existsSync(path.join(fx.repo, '.git', 'MERGE_AUTOSTASH')), false); + }); + + test('a kill inside commit-msg (MERGE_HEAD already written) is the ordinary abort path — timed out, no residue, clean primary', (t) => { + // By commit-msg time git has written MERGE_HEAD, so `git merge --abort` can and + // does restore the tree; restoreMergeResidue then finds nothing to do. + const fx = buildRepoWithSlowHook({ hookName: 'commit-msg' }); + t.after(() => cleanup(fx.root)); + const result = executeWorktreeWaveCleanupPlan(fx.plan, { mergeTimeoutMs: KILL_BUDGET_MS }); + assert.equal(result.entries[0].reason, 'merge_timed_out'); + assert.deepEqual(result.entries[0].warnings, [], 'abort cleaned it; the residue path reports nothing'); + assert.equal(fx.git(['rev-parse', 'HEAD']).trim(), fx.base); + assert.equal(fs.existsSync(path.join(fx.repo, '.git', 'MERGE_HEAD')), false, 'abort cleared MERGE_HEAD'); + assert.equal(fx.git(['status', '--porcelain']).trim(), ''); + assert.equal(fs.existsSync(path.join(fx.repo, 'b.txt')), false); + }); + }); + test('#3804: rescues uncommitted SUMMARY.md from worktree .planning/ before dirty check', () => { // Fixture: the only dirty file is .planning/q1-SUMMARY.md (executor left it uncommitted // per documented contract — orchestrator commits it). cleanup-wave MUST rescue it @@ -7331,7 +7745,7 @@ describe('#2596 scope conformance — executeWorktreeWaveCleanupPlan integration test('WAVE_CLEANUP_WARNING is a frozen, locked code set', () => { assert.deepEqual( Object.keys(WAVE_CLEANUP_WARNING).sort(), - ['SCOPE_CHECK_UNAVAILABLE', 'SCOPE_OUT_OF_DECLARED'], + ['MERGE_AUTOSTASH_UNRESTORED', 'MERGE_RESIDUE_LEFT_STAGED', 'MERGE_RESIDUE_RESTORED', 'SCOPE_CHECK_UNAVAILABLE', 'SCOPE_OUT_OF_DECLARED'], ); assert.equal(Object.isFrozen(WAVE_CLEANUP_WARNING), true); });