Skip to content

fix(bin): refuse teardown while a task's recorded PR is still open - #2408

Closed
joliverMI wants to merge 1 commit into
kunchenguid:mainfrom
joliverMI:fm/fm-teardown-open-pr-guard
Closed

fix(bin): refuse teardown while a task's recorded PR is still open#2408
joliverMI wants to merge 1 commit into
kunchenguid:mainfrom
joliverMI:fm/fm-teardown-open-pr-guard

Conversation

@joliverMI

Copy link
Copy Markdown

Summary

Tearing down a task deletes state/<id>.meta, which is what bin/fm-pr-merge.sh resolves the task's PR through. Two incidents an hour apart showed that a task can be torn down while its PR is still open, stranding a proven, mergeable PR with no guarded path to land it.

The existing landed-work check in validate_worktree_teardown_safety only inspects local git state (dirty, unpushed, or unreachable-but-landed commits). A branch that is fully pushed and reachable from a remote-tracking branch passes that check cleanly even while its own PR is still open - exactly what happened tonight.

What changed

  • Added pr_open_state() next to pr_is_merged() in bin/fm-teardown.sh, reusing the same gh pr view --json state,headRefOid query shape so both share one call pattern (and existing test stubs).
  • Added a new refusal at the end of validate_worktree_teardown_safety, gated on a non-empty recorded pr=, so it never runs for scout/local-only/secondmate teardowns (none of which ever record one).

Decisions

  • Placement: the new check runs after the existing dirty/unpushed/landed checks, as an independent final gate, not nested inside the unpushed branch (it must fire even when nothing is unpushed - that's the actual incident shape). This keeps messages from contradicting: only one refusal fires per invocation, and the more urgent local-state refusals (uncommitted changes, unlanded commits) take precedence over the higher-level "is this task's PR actually done" question.
  • Forge unreachable: refuses loudly, naming the PR it could not confirm, rather than tearing down blind - "unreachable" and "confirmed closed" are different facts, and guessing safe on the former defeats the point of the guard.
  • --force: covers this check too, explicitly documented in the header and usage comment (not incidental - it falls out of the same [ "$FORCE" != "--force" ] || return 0 early return the existing checks already use). --force already means "the captain explicitly authorized discarding this," and bypassing the open-PR guard means accepting the PR now needs to be merged or closed by hand, outside fm-pr-merge.sh's guarded path.
  • Refusal text: tells the operator to merge with fm-pr-merge.sh (once green and authorized) or close the PR on GitHub, then retry teardown.

Test plan

  • bin/fm-lint.sh bin/fm-teardown.sh tests/fm-teardown.test.sh - clean (ShellCheck 0.11.0)
  • bash tests/fm-teardown.test.sh - all 62 cases pass, including 4 new ones covering: open PR refuses, --force overrides, gh lookup error refuses (fail-safe), and confirmed-closed PR does not block
  • bash tests/fm-teardown-endpoint-safety.test.sh - all 7 cases still pass (no regression)
  • Fixed two pre-existing tests (test_teardown_prompts_tasks_axi_done_when_compatible, test_teardown_manual_backend_prompts_hand_edit_even_when_tasks_axi_present) that incidentally recorded a pr= without caring about its state; they now pin it to a confirmed-closed stub so they stay decoupled from this new check

Tearing down a task deletes state/<id>.meta, the record bin/fm-pr-merge.sh
resolves the PR through. A branch can be fully pushed and reachable from a
remote (so the existing dirty/unpushed/landed checks find nothing to
refuse) while its own PR still sits open, stranding a mergeable PR with no
guarded path to land it - this happened twice in one night.

The new check runs after the existing dirty/unpushed/landed checks inside
validate_worktree_teardown_safety, so their refusal messages take
precedence and never contradict this one. It fires only when a pr= is
actually recorded (scout, local-only, and secondmate teardowns never
record one). A gh lookup error refuses loudly too, naming the PR firstmate
could not confirm, rather than tearing down blind. --force already skips
the dirty/landed checks and now explicitly skips this one too, since it is
the same "captain says discard" escape hatch - the usage comment spells
out what that means here (the PR needs merging or closing by hand,
outside fm-pr-merge.sh's guarded path).
@joliverMI

Copy link
Copy Markdown
Author

Withdrawn: this belongs on our own fork rather than upstream. Re-opened against joliverMI/firstmate.

@joliverMI joliverMI closed this Aug 16, 2026
@joliverMI
joliverMI deleted the fm/fm-teardown-open-pr-guard branch August 16, 2026 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants