task-done --remove-worktree: force-delete reviewer branch on PR_NUMBER (#148)

[martin-conur]

Summary

• Branch task-done's branch-deletion on the PR_NUMBER= marker that task-reviewer writes into the worktree's .info file. Reviewer worktrees get git branch -D (their task/review-pr<N> branch is pure scaffolding forked from the PR's head ref and is never going to land in main); worker worktrees keep the safe-delete git branch -d.
• Fixes the per-PR guard collision in task-reviewer (Error: a review worktree or branch already exists for PR #N) that bit every re-review round-trip and forced a manual git branch -D task/review-pr<N> between cycles.
• Applied across the task-done-std and task-done-local drift groups (7 binaries, byte-identical). tools/check-drift.sh stays green — the new conditional lives inside the existing confirm-and-cleanup region.

Closes #148

Test plan

• ./run_tests.sh task_done — 60 tests pass, including 7 new reviewer-branch force-delete tests (one per variant) plus a worker regression for claude-gh.
• ./run_tests.sh drift_check — 8 tests pass; task-done-std and task-done-local confirm-and-cleanup regions remain byte-identical.
• tools/check-drift.sh — green (21 groups checked).
• ./run_tests.sh — full suite green (783 tests).
• End-to-end: dispatch task-reviewer 42 → task-done --remove-worktree → re-dispatch task-reviewer 42 succeeds without manual git branch -D.

🤖 Generated with Claude Code

[martin-conur]

PR Review — #153: task-done --remove-worktree: force-delete reviewer branch on PR_NUMBER (#148)

Spec compliance

Spec issue #148 defined four deliverables. All are present:

Deliverable	Status
Part A — Branch deletion in all 7 task-done variants conditioned on PR_NUMBER marker	✅
Part B — Audit of reviewer-state cleanup (no new leaks found)	✅ (audit confirmed in PR description; table in spec was already green except for the branch)
Part C — Tests: reviewer force-delete (7 variants) + worker safe-delete regression	✅
Part D — CHANGELOG [Unreleased] Fixed entry	✅

The change is applied consistently across the task-done-std (5 binaries: claude-gh, claude-jira, claude-notion, kiro-gh, kiro-notion) and task-done-local (2 binaries: claude-local, kiro-local) drift groups, byte-identical within each group as required.

---

Code-review findings

Blocker — PR_NUMBER not zeroed before source "$INFO_FILE"

File: claude-gh/bin/task-done (and all 6 sibling variants) — sourcing region (~line 53)

The pre-source initialization pattern already exists for BASE_BRANCH:

BASE_BRANCH="main"
[[ -f "$INFO_FILE" ]] && source "$INFO_FILE"

PR_NUMBER is not given the same treatment. A worker's .info file contains no PR_NUMBER= line, so sourcing it never writes that variable. If PR_NUMBER is set in the ambient shell environment (e.g., the user ran a task-reviewer session earlier in the same shell session, or has it exported), [[ -n "${PR_NUMBER:-}" ]] will fire on the worker worktree and call git branch -D on a branch that may contain unmerged commits — silently destroying work that was deliberately protected by git branch -d.

Fix: add PR_NUMBER= on the line before source "$INFO_FILE", mirroring BASE_BRANCH="main":

BASE_BRANCH="main"
PR_NUMBER=
[[ -f "$INFO_FILE" ]] && source "$INFO_FILE"

Apply to all 7 variants.

Test gap: The regression test claude-gh: worker worktree (no PR_NUMBER) still uses safe-delete -d does not set PR_NUMBER=42 in the test environment before invoking task-done, so it passes even with the bug present and would not catch this scenario. A follow-on test that exports PR_NUMBER=42 and then runs task-done from a worker worktree (no PR_NUMBER in its info file) would close this.

---

Nit — Misleading message when reviewer branch is already gone

File: claude-gh/bin/task-done (and all 6 siblings) — new else branch in force-delete path

echo "Done. Branch '$BRANCH' could not be deleted — clean up manually: git branch -D $BRANCH"

If git branch -D fails because the branch was already deleted (double-invocation, or the user ran git branch -D manually before task-done), this message instructs the user to run the exact command that already succeeded. The script exits 0 and the worktree is gone, so there's nothing to actually clean up. Not a blocker — the flow still exits cleanly — but worth noting if message accuracy matters for the dogfooding experience.

---

Minor — Worker regression test covers only claude-gh

File: tests/task_done.bats

The spec asked for a regression test against a non-reviewer worktree to confirm git branch -d behavior is unchanged. The test exists but only for claude-gh. The other 6 variants have the reviewer force-delete test but no corresponding worker-path assertion. This is a coverage gap; not a blocker given the drift-check suite, but a future copy-paste in one of the sibling task-work scripts that accidentally introduces PR_NUMBER= into a worker's info file would not be caught by the sibling's test.

---

Verdict: changes-requested

One blocker: PR_NUMBER must be zeroed before source "$INFO_FILE" in all 7 variants so an ambient shell export cannot trigger git branch -D on a worker branch with unmerged commits. The rest of the implementation (logic, drift consistency, CHANGELOG, test structure) is solid. The nits are non-blocking.

[martin-conur]

Changes requested — one real blocker (data-loss risk)

Blocker — PR_NUMBER not zeroed before source "$INFO_FILE"

BASE_BRANCH="main" is correctly defaulted before the source (task-done:52); PR_NUMBER is not. Worker .info files contain no PR_NUMBER= line, so sourcing one never touches that variable. If PR_NUMBER is set in the ambient shell (e.g., the user ran task-reviewer 42 earlier in the same shell session, leaving the export in place, then cd'd into a worker worktree and ran task-done), the new [[ -n "${PR_NUMBER:-}" ]] guard fires on a worker branch and calls git branch -D — silently force-deleting any unmerged worker commits.

Fix — mirror the existing default. In all 7 task-done variants (task-done-std + task-done-local drift groups):

  BASE_BRANCH="main"
+ PR_NUMBER=
  [[ -f "$INFO_FILE" ]] && source "$INFO_FILE"

Test gap — the existing regression test worker worktree (no PR_NUMBER) still uses safe-delete -d does NOT export PR_NUMBER=42 in the test environment before invoking task-done. It only proves the conditional doesn't fire when both file and environment are absent — i.e., it would silently pass even with the bug present. Add a new test case:

@test "ambient PR_NUMBER export does NOT trigger force-delete on worker" {
  # Ambient export — simulates user having run task-reviewer earlier this shell.
  export PR_NUMBER=42
  # Build a worker worktree info file (no PR_NUMBER inside).
  # Run task-done.
  # Assert: git branch -d was called (safe delete), NOT -D, and the worker branch's
  # unmerged commit is preserved.
  unset PR_NUMBER
}

This is the single test that would catch the bug. Without it, future maintainers could revert the PR_NUMBER= zero-init and CI stays green.

Non-blocking nits (take while you're in there or skip)

Misleading message when branch already gone: when git branch -D fails because the branch was already deleted (double-invoke, or user ran it manually before task-done), the error message instructs the user to run the exact command that already succeeded. Worth a graceful "already deleted" branch — but the flow exits cleanly so it's not load-bearing.

Worker regression coverage only on claude-gh: the worker-path safe-delete assertion runs only against claude-gh's task-done. Other 6 variants get the reviewer force-delete test but no worker-path assertion. Drift-check covers structural copy, but if a future sibling change drifts the worker-path behavior, only claude-gh's test catches it. Low priority since the bodies are byte-identical inside the drift group regions.

Out of scope

Just the PR_NUMBER zero-init + the ambient-export regression test. Don't touch #151 / #152 / #149.

Note on worker session

Your TAB_ID is populated (post-#150 fix is active), so this ping should auto-wake you. LOADOUT=unknown is expected until #151 lands — doesn't affect radio routing.

[martin-conur]

Re-review — PR #153 (round 2)

Context

This is a re-review after the changes-requested findings from round 1. The prior blocker was: PR_NUMBER not zeroed before source "$INFO_FILE", allowing an ambient shell export to trigger git branch -D on a worker branch with unmerged commits. Round 1 also asked for an ambient-export regression test to prove that the zero-init actually blocks the leak.

---

Changes-requested blocker: resolved ✅

All 7 task-done variants now have:

PR_NUMBER=
[[ -f "$INFO_FILE" ]] && source "$INFO_FILE"

This is byte-identical to the BASE_BRANCH="main" pattern that precedes it — consistent with the existing convention, and verified present in every file in the drift groups.

The ambient-export regression test (assert_ambient_pr_number_does_not_force_delete) is now present, exercising:

• claude-gh (representative of the 5-file task-done-std drift group)
• claude-local (representative of the 2-file task-done-local drift group)

The test correctly exports PR_NUMBER=42 before invoking the script via bash -c (child process inherits the export), confirms git branch -d is used (not -D), and asserts the unmerged commit SHA survives. Mechanically sound.

---

Code-review findings (this round)

Nit — ambient-export regression test covers 2 of 7 variants

tests/task_done.bats calls assert_ambient_pr_number_does_not_force_delete for $CLAUDE_GH_TASK_DONE and $CLAUDE_LOCAL_TASK_DONE only. The other 5 variants (claude-jira, claude-notion, kiro-gh, kiro-notion, kiro-local) have no ambient-export test.

Mitigating factor: tools/check-drift.sh enforces byte-identical worktree-context regions across both drift groups — task-done-std (5 files) and task-done-local (2 files). The drift check fails if any variant's region diverges. The test comment even names this rationale explicitly. So the 2-of-7 coverage is a deliberate and documented tradeoff, not an oversight. Non-blocking.

Residual risk: if a future change splits a variant out of its drift group and does not add a dedicated ambient test, the regression would go undetected for that variant. Worth noting as a follow-on if the drift groups are ever refactored.

Nit — worker safe-delete regression covers only claude-gh

"claude-gh: worker worktree (no PR_NUMBER) still uses safe-delete -d" is the only test asserting that a worker branch with unmerged commits is NOT force-deleted when PR_NUMBER is absent. The other 6 variants have the reviewer force-delete test but no worker-path assertion. Same drift-check mitigation applies; non-blocking.

Nit — worker regression uses --force instead of --remove-worktree --force

The reviewer force-delete tests use --remove-worktree --force; the worker regression test uses run_task_done "$CLAUDE_GH_TASK_DONE" --force (no --remove-worktree). These can traverse different code paths before reaching the branch-deletion guard. A future change that accidentally gates the PR_NUMBER guard on REMOVE_ONLY would pass both the reviewer test and this worker test but silently force-delete worker branches when task-done --remove-worktree is called without PR_NUMBER. Non-blocking; easy to address if the regression test is ever extended.

Nit (pre-existing, noted in round 1) — misleading message when reviewer branch is already gone

When git branch -D "$BRANCH" fails because the branch was already deleted (double-invoke), the error message instructs the user to run the exact command that already succeeded or that they already ran. Exit code is 0 and cleanup is otherwise complete. No functional impact; message accuracy only.

---

TOCTOU candidate (evaluated, refuted)

One angle raised a concern that rm -f "$INFO_FILE" at line 153 precedes the branch-delete guard at line 162, creating a crash-between window where a retry would have no info file and therefore an empty PR_NUMBER. After reading the full script: the script calls cd "$MAIN_WORKTREE" before the info-file deletion, and a re-entry is blocked immediately by the worktree guard ("Error: you're in the main repo, not a worktree"). The retry cannot reach the branch-delete code through any normal path. Not a bug.

---

Spec compliance (final check)

Deliverable	Status
Part A — PR_NUMBER zero-init + force-delete in all 7 variants	✅
Part B — audit confirms no new leaks beyond the branch	✅
Part C — reviewer force-delete tests (7) + worker regression + ambient-export tests (2)	✅
Part D — CHANGELOG [Unreleased] Fixed entry	✅

tools/check-drift.sh should stay green — both the worktree-context and confirm-and-cleanup regions are byte-identical within each drift group.

---

Verdict: clean-with-nits

The round-1 blocker is resolved. The PR_NUMBER= zero-init is in place across all 7 variants, the ambient-export regression test covers one file per drift group and the test logic is sound. Remaining findings are all nits — coverage gaps documented and mitigated by drift enforcement, not holes. Ready to merge at PM's discretion.