sl/i178 cleanup merged worktrees false leaves a

[willprout]

Closes #178

cleanup_merged_worktrees is named for WORKTREES — an operator keeping the checkout to inspect it — but it was ANDed with auto_close_merged_windows (#168) to gate the whole merged-lane teardown. Setting it false skipped teardown entirely, so nothing closed the session and no pending_teardown marker was written either, so nothing ever retried.

Before #155 that was untidy but inert: only a FINISHED lane idling at the prompt could reach absorb_merged. #155 lets it fire on an IN-FLIGHT lane — a PR merged out of band while the worker is still building — and there the builder is left running against a branch that has already landed, free to commit and push to it and open a fresh PR nobody asked for, while local status already reads merged, the lane is freed, and nothing is watching.

What changed

_auto_close_merged() becomes _settle_merged_lane(iid) at all three call sites (_exec_merge, _exec_absorb_merged, _exec_absorb_close), asking one question each:

knob	question
auto_close_merged_windows (default true)	may the SESSION be ended?
cleanup_merged_worktrees (default true)	may the CHECKOUT then be pruned?

The #168 property that motivated the AND survives: an operator who sets EITHER knob to keep a finished checkout still gets it kept — window knob off means no teardown at all, so the worktree persists too (a prune can never run under the live CLI it would leave open, #149).

Two review findings shaped the second commit:

• The keep-the-checkout path must still OBSERVE the builder die. _close_pane is best-effort by contract, so without a wait a failed close would be recorded as a session ended while the pane markers tidy needs and the lock the liveness tiers read were deleted behind its back. _teardown_session grew an await_exit seam (defaulting to remove_worktree, so the D4 relaunch close is unchanged); a decline there clears nothing and writes no marker.
• A stale pending_teardown marker from an earlier decline on the same lane would have let the drain — which retries with remove_worktree=True unconditionally — prune the kept checkout one tick after the settle cleared the lock, with no live pid left to refuse it. The settle now retires that marker when the prune knob is off.

Evidence

Full engine suite green (2544 passed). Six new unit tests plus a simulation test that drives real runner ticks against a real git repo and a real worker process: with cleanup_merged_worktrees: false, an out-of-band merge of a building lane now kills the builder (verified against the OS, not a recorded call) and keeps the checkout. Details, including a standalone before/after drive of the real Runner over a real git worktree, are in the report.

[willprout]

Fresh-agent review verdict — CLEAN, no open findings at any severity

Reviewed: the cumulative diff origin/main (bc1ad95)...ed4d885 — all six commits, by agents that wrote none of it. Four review rounds, each re-deriving the delta from scratch rather than taking prior findings on trust. Two independent fresh reviewers were used across the rounds; neither authored any of the code.

What was reviewed: skills/superlooper/skill/bin/runner.py (_settle_merged_lane replacing the _auto_close_merged predicate at all three executor call sites plus the drain; the await_exit seam on _teardown_session; the pending_teardown marker lifecycle; _hold_close; the post-execute re-read feeding both hygiene sweeps in tick()), skill/lib/config.py, skill/docs/ADOPTING.md, and the tests in tests/test_runner.py + tests/test_simulation.py.

P0/P1 outcome: four P0/P1 findings were raised across the rounds and all are fixed, each pinned by a regression test verified to fail against the mutated fix:

#	Severity	Finding	Fixed in
1	P0	A stale declined-prune marker let the drain prune the kept checkout one tick after the settle cleared the lock — unguarded, with no live pid left to refuse it	7e83b6a
2	P1	The keep-the-checkout path never observed the builder die, yet deleted every handle to it (tidy's pane markers, the liveness lock), making a failed close silent and unrecoverable	7e83b6a
3	P1	The same stale-marker hazard on the auto_close_merged_windows: false early return — the split was half-fixed, and a declined close had no retry or evidence at all	85a4040
4	P0+P1	tick() handed both hygiene sweeps a pre-execute snapshot, so the owed-close marker was eaten by the very tick that wrote it — and an absorb_close lane still reading bounced routed the kept checkout into an unguarded prune	878d63b

Remaining P2s were addressed in 57084d2 (the residual harm — a builder that outlived its close — is now journaled once per episode with the pid that makes it actionable) and ed4d885 (a sentinel collision where an unreadable or trap-freed lock read as "already journaled").

Final verdict, quoted:

I reviewed head ed4d885 as a fresh reviewer across four rounds, re-deriving each delta from scratch rather than taking prior findings on trust. The branch does what issue #178 asks: cleanup_merged_worktrees is now strictly about the checkout, the window question is asked once by auto_close_merged_windows alone, and the keep-the-checkout path observes the builder's pid go rather than assuming it — which matters precisely because _close_pane is best-effort by contract. The riskiest change, moving both hygiene sweeps onto a post-execute read of issues.json, is sound in both directions: the fresher read is strictly more current, so the drain's "an id could have been re-approved between the decline and the retry" safety argument is strengthened rather than weakened, and I confirmed by reachability analysis that no lane can carry a pending_teardown marker across a non-terminal-to-park-family transition inside a single tick, which makes that half inert for non-merged lanes in practice. Its two real side effects — the opt-in parked reaper acting a tick earlier, still behind the live-pid gate and #190's dirty/unpushed guard, and the two rebuild executors losing one same-tick retry of a merely-failed git removal — I measured independently and both are now stated at the call site. Every finding I raised was fixed and each fix is mutation-tested: reverting fresh to the stale snapshot fails three tests, reverting the reclaim sweep's argument alone fails one, and the _hold_close journal line fails four different ways when its guard, its call, its retirement pop, or its iid in membership test is tampered with. The final commit closes the last one — a sentinel collision where an unreadable or trap-freed lock read as "already journaled" — and its test drives the genuine decline sequence rather than the degenerate one an earlier draft would have. The residual harm this whole path exists to bound, a builder that outlived its close and is still standing in a checkout whose branch is already on the mainline, is now visible in the journal with the pid that makes it actionable, bounded to one line per episode. Engine suites and the real-process simulation are green in my copy, the adjacent staleness in _publish_view is filed as #276 with its reasoning rather than left to be read as an oversight, and I have no open findings at any severity. Ready to merge.

Out-of-scope defects the reviews surfaced, filed rather than fixed here: #274 (absorb_close prunes a park-family checkout without #190's unsaved-work guard), #275 (close_investigate settles to merged but never tears down), #276 (_publish_view renders the pre-execute snapshot). All pre-existing on main, all labelled needs-owner with parent: #178.

Evidence: full engine suite green at 2551 passed; CI tests green on this head (8m27s); and the changed behaviour was driven end-to-end outside pytest against the real Runner over a real git worktree with a real detached worker process — before the fix the builder was left STILL RUNNING with local status already merged, after it the builder is DEAD and the checkout kept. Details in reports/i178.md.