Skip to content

fix: recognize squash-merged work as landed#96

Merged
kunchenguid merged 5 commits into
mainfrom
fm/teardown-squash-fix-t6
Jun 26, 2026
Merged

fix: recognize squash-merged work as landed#96
kunchenguid merged 5 commits into
mainfrom
fm/teardown-squash-fix-t6

Conversation

@kunchenguid

Copy link
Copy Markdown
Owner

Intent

Fix a real bug in bin/fm-teardown.sh: it refused to tear down a worktree whose PR was squash-merged and whose head branch was then deleted (the most common GitHub flow), because its safety check used commit reachability (git log HEAD --not --remotes) instead of whether the work actually landed. A squash merge writes one new commit on the default branch, so the branch's own commits are on no remote-tracking branch, and auto-delete-on-merge drops the head ref too; reachability was non-empty and teardown false-refused merged work.

The fix: for a normal ship task whose commits are not remote-reachable, before refusing, treat the work as landed if EITHER (1) the PR is merged - resolved from meta pr= or the branch name via gh-axi, authoritative for squash/rebase/merge alike and surviving head-branch deletion; OR (2) the content is already in the up-to-date default branch - a 3-way 'git merge-tree --write-tree' whose merged tree equals the default branch's tree, which isolates branch-only changes so unrelated commits the default gained past the merge-base do not count as added. The dirty-worktree refusal was split out and still refuses unconditionally (uncommitted changes are never landed). Genuinely unlanded work still refuses. Fail-safe: on a gh lookup error we fall back to the content check, and if that is inconclusive we refuse - the merged-PR path is an additional way to pass, never a way to skip a real refusal. The fork-as-remote, local-only, scout, secondmate, and --force paths are deliberately untouched.

Deliberate decisions a diff-only reviewer would not know: merge-tree --write-tree (git >= 2.38; production is 2.53) is chosen over patch-id/cherry because it correctly detects squash landings; its non-zero exit on conflict is intentionally treated as 'not landed' (fail-safe). The content check fetches origin first so it compares against the up-to-date default. The new tests use real git fixtures (matching the existing harness that builds real bare origins/clones/worktrees) with only gh-axi/treehouse/tmux mocked, so they stay hermetic; the pre-existing 'truly unpushed' test was changed from an empty commit to a real-file commit because an empty commit's content is trivially already in the default branch and would now (correctly) be considered landed. Also updated the script header comment and AGENTS.md sections 1.3 and 7 so the documented definition of 'landed' matches the implementation.

What Changed

  • Allows teardown to treat squash-merged ship work as landed when the merged PR is proven for the current head, or when the branch content already exists in the up-to-date default branch.
  • Hardens PR proof recording and teardown checks so stale PR metadata cannot mark later local commits as landed, while dirty or genuinely unlanded work still refuses teardown.
  • Updates teardown landing docs and expands real-git teardown coverage for merged PR, content fallback, unlanded, and dirty-worktree cases.

Captain, pipeline passed with review auto-fixes rechecked clean.

Risk Assessment

✅ Low: Captain, the teardown safety change is scoped, fail-safe on inconclusive GitHub/content checks, and I found no material issues in the reviewed diff.

Testing

I reviewed the teardown intent and targeted diff, ran the focused teardown safety suite, then produced a product-level CLI transcript from real git fixtures showing the new landed-work behavior and preserved refusal paths; all exercised checks passed.

Evidence: Teardown CLI transcript

Manual CLI evidence shows: merged PR with unreachable head exited 0, content already on default exited 0, genuinely unlanded work refused with exit 1, and dirty worktree refused with exit 1.

Manual CLI evidence for bin/fm-teardown.sh landed-work safety
repo: /Users/kunchen/.no-mistakes/worktrees/016d88035d58/01KW2DJ0XT3VCY3YY2PT3RC6MY
fixture root: /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T//fm-teardown-evidence.syiESW (removed after run)

--- merged PR with deleted/unreachable head branch is allowed ---
HEAD reachability before teardown: not reachable from origin/main
/var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T//fm-teardown-evidence.syiESW/squash-pr-merged/project: already current
teardown task-x1 complete (window fm-task-x1, worktree /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T//fm-teardown-evidence.syiESW/squash-pr-merged/wt)
Backlog: task-x1 just finished. Run tasks-axi done task-x1 --pr https://github.com/example/repo/pull/7, then run tasks-axi ready for dependency-cleared candidates, check date gates, and dispatch only work whose blockers are gone and date is due.
exit code: 0 (expected 0)

--- same content already on updated default branch is allowed ---
HEAD reachability before teardown: not reachable from origin/main
/var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T//fm-teardown-evidence.syiESW/content-fallback/project: synced f9d1be7..11cdc5d
teardown task-x1 complete (window fm-task-x1, worktree /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T//fm-teardown-evidence.syiESW/content-fallback/wt)
Backlog: task-x1 just finished. Run tasks-axi done task-x1 --pr PR_URL, then run tasks-axi ready for dependency-cleared candidates, check date gates, and dispatch only work whose blockers are gone and date is due.
exit code: 0 (expected 0)

--- genuinely unlanded work is refused ---
HEAD reachability before teardown: not reachable from origin/main
REFUSED: worktree /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T//fm-teardown-evidence.syiESW/genuinely-unlanded/wt has work not on any remote and not landed.
unpushed commits:
68733b5 add feature
Push the branch, land its PR, or get the captain's explicit OK to discard, then --force.
exit code: 1 (expected 1)

--- dirty worktree is refused even when committed work landed ---
HEAD reachability before teardown: not reachable from origin/main
REFUSED: worktree /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T//fm-teardown-evidence.syiESW/dirty-even-landed/wt has uncommitted changes.
uncommitted changes present
Commit them (or get the captain's explicit OK to discard, then --force).
exit code: 1 (expected 1)

manual CLI evidence completed
Evidence: Targeted teardown test log

Targeted teardown suite passed all covered cases, including squash-merged PR, content fallback, stale PR, dirty worktree, gh-error fail-safe, local-only, fork-remote, and force paths.

ok - local-only worktree with HEAD on a fork remote is torn down (fix holds)
ok - teardown prompts tasks-axi backlog refresh when compatible
ok - local-only worktree with truly unpushed work is refused (safety preserved)
ok - local-only worktree with work merged into local main is torn down (no regression)
ok - no-mistakes worktree with HEAD on origin is torn down (no regression)
ok - no-mistakes worktree with genuinely unlanded work is refused (safety preserved)
ok - local-only worktree with unpushed work is torn down under --force (escape hatch)
ok - squash-merged + deleted-branch worktree (PR merged) is torn down (the fix)
ok - merged PR does not allow teardown after a later local commit
ok - fm-pr-check does not refresh PR head after HEAD moves
ok - worktree whose content already landed in the default branch is torn down (content fallback)
ok - content fallback refreshes origin default before comparing trees
ok - dirty worktree is refused even when its committed work has landed (dirty always wins)
ok - gh lookup error with content not in default refuses (fail-safe)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 2 issues found → auto-fixed (2) ✅
  • 🚨 bin/fm-teardown.sh:151 - A merged PR is accepted before validating that the current worktree HEAD is the work that was merged. If pr= is stale or the crewmate made any extra local commit after the PR was recorded/merged, unpushed is non-empty but pr_is_merged still returns true and teardown deletes the branch/worktree, discarding unlanded work. Gate the PR path on the current HEAD/content, or only trust PR state when it proves this exact head was included.
  • ⚠️ bin/fm-teardown.sh:130 - git fetch origin "$name" fetches the branch to FETCH_HEAD, but the code then compares against refs/remotes/origin/$name, which can remain stale. That makes the content fallback false-refuse landed squash work when pr= is missing or gh lookup fails. Fetch the remote-tracking ref explicitly, or compare against FETCH_HEAD from a successful fetch and treat fetch failure as inconclusive.

🔧 Fix: Guard teardown against stale PR proof
1 error still open:

  • 🚨 bin/fm-pr-check.sh:27 - pr_head= is recorded independently of the PR URL and only reflects the local worktree HEAD, not the actual PR head. If fm-pr-check.sh is rerun with the same already-merged PR URL after any local follow-up commit, this appends a new pr_head, and fm-teardown.sh will accept that unlanded commit because the PR URL is merged and current HEAD matches the freshly recorded pr_head. Record the PR/head association atomically and only after proving the current HEAD is the PR head, or avoid updating pr_head for an existing PR without that verification.

🔧 Fix: Verify teardown PR heads
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • tests/fm-teardown.test.sh 2>&1 | tee /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T/no-mistakes-evidence/01KW2DJ0XT3VCY3YY2PT3RC6MY/fm-teardown-test.log
  • Manual real-git fixture invoking bin/fm-teardown.sh task-x1 for merged-PR, content-fallback, genuinely-unlanded, and dirty-worktree cases; transcript saved to /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T/no-mistakes-evidence/01KW2DJ0XT3VCY3YY2PT3RC6MY/fm-teardown-cli-transcript.txt
  • git status --short confirmed no working-tree artifacts were left behind.
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

fm-teardown refused to tear down a worktree whose PR was squash-merged
and whose head branch was then deleted - the most common GitHub flow -
because its safety check used commit reachability (HEAD --not --remotes)
rather than whether the work actually landed. A squash merge writes one
new commit on the default branch, so the branch's own commits are on no
remote-tracking branch, and auto-delete-on-merge drops the head ref too;
reachability was non-empty and teardown false-refused merged work.

For a normal ship task whose commits are not remote-reachable, before
refusing, treat the work as landed if either its PR is merged (resolved
from meta pr= or the branch name via gh-axi; authoritative for squash,
rebase, and merge alike, and surviving branch deletion) or its content
is already in the up-to-date default branch (a 3-way merge-tree that adds
nothing the default branch lacks - robust to the default having advanced
past the merge-base). Dirty worktrees and genuinely unlanded work still
refuse; a gh lookup error falls back to the content check and, if that is
inconclusive, refuses (fail-safe, never silently allows). Fork, local-only,
scout, secondmate, and --force paths are unchanged.

Tests cover squash-merged+deleted-branch (allow), genuinely unlanded
(refuse), dirty (refuse), normally pushed (allow), content-in-default
fallback (allow), and gh-error+content-absent (refuse, fail-safe).
@kunchenguid kunchenguid merged commit 37c694c into main Jun 26, 2026
4 checks passed
@kunchenguid kunchenguid deleted the fm/teardown-squash-fix-t6 branch June 26, 2026 19:11
leo1oel added a commit to leo1oel/nemo that referenced this pull request Jun 26, 2026
Port upstream kunchenguid#96, adapted to the herdr backend. fm-teardown false-refused a
worktree whose PR was squash-merged and whose head branch was then deleted (the
common GitHub flow, and exactly how this fork's own PRs merge): its safety check
used commit reachability (HEAD --not --remotes), but a squash merge writes one new
commit on the default branch and auto-delete drops the head ref, so the branch's
own commits are on no remote yet the work is fully landed.

- bin/fm-teardown.sh: before refusing unpushed-but-committed work, recognize it as
  landed when its PR is merged and GitHub reports the current HEAD as that PR's head
  (pr_is_merged, resolved from meta pr= or the branch via gh-axi), or its content is
  already in the up-to-date default branch (content_in_default, a 3-way merge-tree
  after a fresh fetch). Dirty worktrees always refuse; a gh error falls back to the
  content check and refuses if inconclusive (fail-safe). Fork/local-only/scout/
  secondmate/--force paths unchanged.
- bin/fm-pr-check.sh: record a verified pr_head=<sha> when the local HEAD matches
  the PR head at PR-ready time.
- bin/fm-promote.sh: comment wording (ship-task teardown protection).
- AGENTS.md (rule 3, section 2 meta, section 7) + docs: document the landed-work check.
- tests/fm-teardown.test.sh: hermetic gh/gh-axi stubs + real-content helpers; the
  unlanded-refuse case now commits real content (an empty commit is correctly landed),
  plus new cases: squash-merged+deleted (allow), content-in-default (allow), and
  gh-error+content-absent (refuse, fail-safe).

shellcheck clean; full suite green (91 checks).
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.

1 participant