feat: fix(worktree): reap all candidate worktrees + kill orphaned coder on cancel (#175)

[mabry1985]

Summary

• reap_feature_worktree previously only removed the canonical feat-<id> worktree, so cancelling a feature mid-first-generation — before any candidate was promoted — silently no-opped and stranded the feat-<id>.gN worktree and its branch on disk (reap_feature_worktree only knows the canonical name — cancelling mid-first-generation orphans the running coder #175).
• After the (unchanged) canonical removal, the reap now lists worktrees_root and removes every candidate worktree whose suffixes strip back to this feature id via the existing parent_feature_id/_CANDIDATE_SUFFIX_RE naming (.gN, .cN, .test, including stacked .test.gN), deleting each candidate's feat/<id>.<suffix> branch with it.
• The sweep is best-effort (missing dirs skipped, failures logged, never raises into the loop) and logs one info line naming everything it cleaned.
• Subprocess kill is deliberately out of scope: the coder is tracked by the loop's _inflight/coder_seam, and removing its cwd makes an orphaned coder fail fast on its next filesystem op.

Fixes #175

[protoreview]

QA panel review — WARN

code-review-structural · head 877e5071707f · formal

[review-synthesizer completed: workflow code-review-structural:report]

All the material I need is in the verifier's report, so no further tool calls are required. Synthesizing:

Dispositions: none — the prior-requests block is empty, this panel has not reviewed the PR before, so no dispositions JSON is emitted.

Synthesis decisions:

• Finding #1 (line 679): CONFIRMED, keep as minor/tests. The verifier's nuance matters — parent_feature_id's .test/.test.g2 strip is unit-covered pre-PR, so the claim is phrased as a sweep-integration coverage gap (that's the accurate defect), and the evidence/note are carried verbatim.
• Finding #2 (line 661): CONFIRMED, keep as nit/conventions. The verifier corrected the framing: there is no module-level _fake_git to shadow — the pre-existing stub is a local stub inside another test — so the claim is rewritten as a repeated local-stub pattern per the verifier's explicit instruction, while evidence and note stay byte-for-byte (the note already carries the correction).
• Rank: minor → nit. Neither was refuted; neither is "uncertain" (both verdict: confirmed).
• Coverage gaps for the brief: the structural pass was skipped; worktree.py (the production side being rewritten) drew zero findings — the panel's production-code read was lighter than its test-side read; grounding relied on the server-resolved diff after two head-SHA file-read 404s.

This PR rewrites the reap sweep in worktree.py (stacked .test.g2 candidates) and adds five tests; overall risk is low, but the one thing to fix first is the untested integration path — add a test that builds a .test.g2 dir so the sweep's stacked-suffix strip can't regress silently. Verification confirmed both findings as-is; it corrected #2's framing (there is no module-level _fake_git to shadow — the pre-existing stub is local to another test, so this is a repeated-local-stub nit, not shadowing) and narrowed #1 to sweep-integration coverage since parent_feature_id's strip is already unit-tested. Coverage gaps: the structural pass was skipped, and the production-side rewrite in worktree.py drew zero findings — the panel read the test side far more closely than the production code; grounding rests on the server-resolved PR diff because the head-SHA file read 404'd twice.

Findings

	Severity	Location	Finding	Verified
🟡	minor	tests/test_worktree.py:679	The new sweep's advertised .test and stacked .test.g2 handling — the trickiest matching path, which depends on parent_feature_id stripping stacked suffix…	confirmed
⚪	nit	tests/test_worktree.py:661	Three of the five new tests copy-paste the same local _fake_git stub — differing only by a trailing comment or the error string — a repeated 4-line local stu…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "tests/test_worktree.py",
    "line": 679,
    "severity": "minor",
    "category": "tests",
    "claim": "The new sweep's advertised `.test` and stacked `.test.g2` handling — the trickiest matching path, which depends on `parent_feature_id` stripping stacked suffixes back to the owning feature id — has no integration coverage in the five new tests: they exercise only `.g1`/`.c1`/`.c2`/`.gx` shapes plus the absent-root and locked-candidate paths, so the exact stranding case this fix targets would regress silently.",
    "evidence": "Concrete scenario: the rewritten sweep in worktree.py documents candidate shapes .g<n> / .c<n> / .test and an inline comment calls out the stacked .test.g2 case, but the new tests in tests/test_worktree.py build only feat-bd-9.g1, feat-bd-9.c1, feat-bd-9.c2, feat-bd-90.g1 and feat-bd-9.gx dirs (plus absent-root and locked-candidate paths); none creates a .test or .test.g2 directory, so the stacked-suffix strip is never exercised.",
    "verdict": "confirmed",
    "note": "Verified against PR #176 diff plus-side: worktree.py docstring advertises `.g<n>`/`.c<n>`/`.test` and the sweep comment names the stacked `.test.g2` shape, and none of the five added tests builds a `.test`/`.test.g2` dir (only .g1/.c1/.c2/.gx + canonical + absent-root + locked). Nuance: parent_feature_id('bd-1.test')/('bd-1.test.g2') ARE unit-tested pre-PR (test_parent_feature_id_strips_candidate_suffixes), so the gap is the reap-sweep integration of the stacked-suffix strip, not the strip itself. Head-SHA file read 404'd; grounded in the server-resolved PR diff."
  },
  {
    "file": "tests/test_worktree.py",
    "line": 661,
    "severity": "nit",
    "category": "conventions",
    "claim": "Three of the five new tests copy-paste the same local `_fake_git` stub — differing only by a trailing comment or the error string — a repeated 4-line local stub pattern that could be reused or parameterized instead.",
    "evidence": "test_reap_removes_candidate_dirs_from_disk_and_logs defines:\nasync def _fake_git(repo, *args, timeout=60):\n        if args[:2] == (\"worktree\", \"remove\"):\n            return (1, \"\", \"fatal: 'x' is not a working tree\")  # metadata gone → rmtree fallback\n        return (0, \"\", \"\")\nThe same stub recurs (sans comment) in test_reap_is_best_effort_when_nothing_exists and with \"fatal: 'x' is locked\" in test_reap_tolerates_a_candidate_that_fails_to_remove; hunk header: @@ -599,3 +600,103 @@ async def _fake_git(repo, *args, timeout=60):",
    "verdict": "confirmed",
    "note": "Substance verified in the PR diff: exactly three of the five added tests define a near-identical local `_fake_git` stub and the quoted stub + hunk header match the diff byte-for-byte. Framing corrected: there is NO module-level `_fake_git` — the base file's pre-existing `_fake_git` is a local stub nested inside test_reap_feature_worktree_rmtree_fallback_when_git_metadata_gone (its asserts use result/wt_dir/caplog), so 'shadowing a module-level helper' is a mischaracterization; it is a repeated local-stub pattern. Head-SHA file read 404'd; grounded in the server-resolved PR diff + base-file tail."
  }
]

[protoreview]

Promoting the WARN verdict for head 877e5071707f: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

• minor tests/test_worktree.py:679 — The new sweep's advertised .test and stacked .test.g2 handling — the trickiest matching path, which depends on parent_feature_id stripping stacked suffixes back to the owning feature id — has no
• nit tests/test_worktree.py:661 — Three of the five new tests copy-paste the same local _fake_git stub — differing only by a trailing comment or the error string — a repeated 4-line local stub pattern that could be reused or paramet

Approving a WARN does not resolve its findings (issue #22).