25d1cb916f
* 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>
754 B
754 B
type, pr
| type | pr |
|---|---|
| Fixed | 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)