fix(teardown): recognize work that landed under rewritten commits - #2586
fix(teardown): recognize work that landed under rewritten commits#2586IanQiu979 wants to merge 23 commits into
Conversation
fm-teardown.sh proved landed work by asking whether the branch's own commits are reachable from the default branch. A rebase gives every commit a new object id, so once landing rewrote them that question can never be answered yes again, and teardown refused work that was fully in main. Because bin/fm-merge-local.sh is fast-forward-only, rebasing onto a moved default branch is the normal way a local-only chain lands, so every multi-chain graph left earlier nodes permanently unreclaimable. The documented remedies were all wrong for that state: merging again is redundant, and --force would need captain authority to discard work that was never at risk. Add patch equivalence - the relationship `git cherry` reports - as an additional landed-work proof, in the local-only merged check and in work_is_landed alike. It is deliberately strict so it can only turn a refusal into an allow on proof: every commit the ref cannot reach must match a patch id the ref does contribute, an unreadable or empty patch (an empty commit, or a merge commit whose conflict resolution `git show` does not emit) never counts as landed, and a ref contributing no patches at all stays inconclusive. The uncommitted-changes refusal and the explicit-authority gate on --force are untouched. Tests cover the three distinguishing cases: a branch whose patch landed under a rewritten commit is reclaimable; a branch holding a commit whose patch is absent from main still refuses; and a worktree with uncommitted changes still refuses even when every commit's patch landed. Mutation runs confirm each case fails on its own weakening - relaxing the match from every commit to any commit fails the second, and dropping the uncommitted check fails the third. A fourth case covers the same rebase fix on the PR path, where main edited the file after the patch landed so the whole-tree content check cannot conclude anything.
fm-teardown.sh proved landed work by asking whether the branch's own commits are reachable from the default branch. A rebase gives every commit a new object id, so once landing rewrote them that question can never be answered yes again, and teardown refused work that was fully in main. Because bin/fm-merge-local.sh is fast-forward-only, rebasing onto a moved default branch is the normal way a local-only chain lands, so every multi-chain graph left earlier nodes permanently unreclaimable. The documented remedies were all wrong for that state: merging again is redundant, and --force would need captain authority to discard work that was never at risk. Add patch equivalence - the relationship `git cherry` reports - as an additional landed-work proof, in the local-only merged check and in work_is_landed alike. It is deliberately strict so it can only turn a refusal into an allow on proof: every commit the ref cannot reach must match a patch id the ref does contribute, an unreadable or empty patch (an empty commit, or a merge commit whose conflict resolution `git show` does not emit) never counts as landed, and a ref contributing no patches at all stays inconclusive. The uncommitted-changes refusal and the explicit-authority gate on --force are untouched. Tests cover the three distinguishing cases: a branch whose patch landed under a rewritten commit is reclaimable; a branch holding a commit whose patch is absent from main still refuses; and a worktree with uncommitted changes still refuses even when every commit's patch landed. Mutation runs confirm each case fails on its own weakening - relaxing the match from every commit to any commit fails the second, and dropping the uncommitted check fails the third. A fourth case covers the same rebase fix on the PR path, where main edited the file after the patch landed so the whole-tree content check cannot conclude anything.
…to fm/fm-teardown-false-refusal-after-rebase # Conflicts: # bin/fm-teardown.sh # docs/gitlab-merge-watch.md # tests/fm-teardown.test.sh
bin/fm-lint.sh is the repo's canonical lint gate and CI invokes it directly, so the branch has to leave it clean. - Assert the numstat record carries its path field instead of parsing a partial line, which also uses the field the reader was discarding (SC2034). - Give the subject-range locals explicit empty-string initialisers (SC1007). - Check the reference-tree diff's exit status directly (SC2181). All three are behavior-preserving or fail-closed: the new path assertion can only turn a proof into a refusal, never a refusal into an allow.
2896c83 to
2c8dc6c
Compare
Confidence Score: 3/5The PR should not merge until bundled repository overrides are rejected again; the unpaced signal test is an additional non-blocking reliability concern. Extra merge arguments can bypass the canonical-repository guard through a short-option cluster and reach the merge CLI, while the modified signal test can flood Bash during trap execution and manufacture intermittent failures. Files Needing Attention: bin/fm-pr-merge.sh, tests/fm-pr-merge.test.sh, tests/fm-remote-job.test.sh Reviews (1): Last reviewed commit: "fix(teardown): clear shellcheck findings..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # A single-dash argument is a short-option cluster, which both CLIs expand | ||
| # one character at a time, so -yR carries --repo exactly as a bare -R does. | ||
| -*R*) | ||
| --repo|--repo=*|-R|-R?*) |
There was a problem hiding this comment.
When a caller supplies an extra argument such as -dR wrong/repo, the narrowed filter accepts it and forwards it after the URL-derived --repo, allowing the merge command to target a repository other than the canonical PR URL names.
How this was verified: The removed regression case identifies -dR as a repository override, while the new pattern no longer matches that cluster and the accepted arguments are forwarded verbatim.
| while kill -0 "$REPEAT_WORKER_PID" 2>/dev/null && [ "$SECONDS" -lt "$REPEAT_DEADLINE" ]; do | ||
| kill -TERM "$REPEAT_WORKER_PID" 2>/dev/null || true | ||
| sleep 0.05 | ||
| done |
There was a problem hiding this comment.
The tight loop now sends SIGTERM continuously throughout worker shutdown. This recreates the documented Bash pending-trap overload that can interrupt or corrupt shutdown, leave temporary ownership state behind, and produce flaky failures unrelated to the worker behavior under test.
|
Speaking as Kun's firstmate: CRITICAL: VISION (per rule):
Class: corrective (teardown rewrite) but the GitLab merge revert is blocking and must not land. Security: the Overlap / do not land together: CI: no structured Ahead 23, behind 1, mergeable, mergeStateStatus UNSTABLE. Not merge-eligible. Waiting on the author to restore the GitLab merge path and keep the teardown rewrite on top of current main — not waiting on the captain. |
2 participants
Intent
Goal: teach bin/fm-teardown.sh's landed-work test about patch equivalence, so it stops
falsely refusing to reclaim a worktree whose content landed under REWRITTEN commits,
without weakening its refusal of genuinely unlanded work.
Background / the defect: the landed-work test asked only whether a branch's COMMITS are
reachable from the default branch. A rebase gives every commit a new object id, so after
any rebase the pre-rebase branch can never satisfy that test even when its content landed
in full. This is not a corner case here: bin/fm-merge-local.sh is fast-forward-only, so any
second chain landing into a moved default branch must rebase first. Every multi-chain
local-only graph therefore ended with parent nodes that ordinary teardown could never
reclaim, and the remedies the refusal offered were both wrong - merging again is redundant,
and --force needs captain authority for work that is not actually at risk.
The fix: a new patches_are_in_ref() asks whether every commit HEAD holds that the reference
cannot reach is already present there as a PATCH - the relationship
git cherryreports.work_is_landed() consults it only after pr_is_merged and the content check have already
failed, and the local-only arm consults it only after the reachability listing already
decided to refuse.
Deliberate decisions:
failed, so it can only ever turn a refusal into an allow on proof, never turn an allow
into a refusal, and never relax an existing guarantee.
git cherry. Verifiedempirically on git 2.50.1:
git cherryOMITS merge commits from its output entirely. Abranch whose only unlanded content lived in a merge commit's conflict resolution would
have reported as fully landed - a real safety hole. Hand-rolling lets an unreadable or
empty patch id count as NOT landed.
(an empty commit, or a merge commit whose conflict resolution
git showdoes not emit)never counts as landed; and a reference contributing no patch ids at all is inconclusive
rather than permissive. Anything short of a complete match refuses.
path, so a dirty worktree refuses regardless of patch state.
this case, not to make --force easier to reach.
display list, so display truncation can never manufacture a false "landed".
Behaviour-preserving refactor along the way: content_in_default() split into
default_landing_ref() + content_in_ref() so both fallbacks share one ref resolution and one
network fetch.
Tests: four cases added to tests/fm-teardown.test.sh, exercised through the executable
interface. (q1) patch landed under a rewritten commit -> ALLOW, with a vacuity guard that
fails the test if the branch turns out to be plainly reachable. (q2) one commit's patch
absent -> REFUSE. (q3) every patch landed but the worktree dirty -> REFUSE. (q4) no-mistakes
mode, patch landed on origin main under a rewritten commit and the file edited again
afterwards, so the content check is inconclusive and patch equivalence is isolated as the
thing doing the work -> ALLOW. Proven genuinely distinguishing by mutation: relaxing the
all-commits-must-match rule to any-match breaks q2; dropping dirty enforcement breaks q3.
Full suite 62 ok / 0 failures; the q1 case fails before the fix. Adjacent suites all pass:
teardown-endpoint-safety, gate-refuse, secondmate-safety, pr-merge, decision-hold-lifecycle,
public-followup. bin/fm-lint.sh clean.
Documentation: the script header is the single owner of the "landed" definition per the
repo's knowledge-placement rules, so it is updated in place to cover both the rewritten-
commit case and why the local-only path meets it most often. AGENTS.md already points at
bin/fm-teardown.sh as the owner of the complete landed-work test, so it needs no change.
Decisions already made by the human on the previous review round, carried forward so they
are not re-litigated:
dirty. Gating it on a clean worktree would also suppress the "commits not yet on "
detail line in the dirty refusal, and that line is the most valuable part of a teardown
refusal - it is what tells the captain exactly what work is at stake before deciding
anything. A redundant scan on a path that refuses anyway is a trivial cost; correctness
beats speed on a destructive-action guard. Accepted as-is, do not reorder.
correctness problem.
landed is accepted. The work genuinely did land, the revert is a deliberate later choice,
and the original commit remains reachable in history, so it is not a data-loss case.
patches_are_in_ref() and unpushed_patches_are_in_pr_head(): duplicated logic in a
safety-critical check is a real hazard, because a future fix applied to only one copy is
the realistic failure mode.
What Changed
bin/fm-teardown.shgainspatches_are_in_ref(), a patch-equivalence check that asks whether every commit the reference cannot reach is already present there as a patch.work_is_landed()consults it only afterpr_is_mergedand the content check have failed, and the local-only arm consults it only after the reachability listing has already decided to refuse — so it can turn a refusal into an allow on proof, never the reverse. It is deliberately strict: every unreachable commit must match, an empty or unreadable patch id never counts as landed, and a reference contributing no patch ids is inconclusive. The uncommitted-changes check and--forcegating are untouched, and the local-only arm recomputes its own full commit list rather than reusing the truncated display list.content_in_default()splits intodefault_landing_ref()+content_in_ref()so both fallbacks share one ref resolution and one fetch, and the patch comparison duplicated between the new check andunpushed_patches_are_in_pr_head()collapses into a singlesubject_patches_are_in_reference()helper. A dirty worktree whose commits provably landed now gets an accurate refusal message instead of the generic "not yet merged" wording.tests/fm-teardown.test.shthrough the executable interface — patch landed under a rewritten commit (allow, with a vacuity guard), one commit's patch absent (refuse), all patches landed but worktree dirty (refuse), and firstmate mode with the file edited after landing so patch equivalence is isolated (allow). Suite runs 62 ok / 0 failures; adjacent suites (pr-merge, gate-refuse, teardown-endpoint-safety, secondmate-safety) pass. Header docs updated in place as the owner of the "landed" definition, plus a one-line correction indocs/gitlab-merge-watch.md.Risk Assessment
✅ Low: The round-2 changes are a behavior-preserving extraction of one shared patch-set comparison plus a narrowly scoped refusal-copy fix, both verified against every caller's prior exit-code semantics and covered by the updated q3 assertions.
Testing
Ran the full teardown suite (62 ok, 0 failures, four new rebase cases included) plus the four adjacent suites the shared patch-matching helper touches (pr-merge, gate-refuse, teardown-endpoint-safety, secondmate-safety) — all clean. Because passing unit tests alone do not show the defect is gone, I also built a real fixture that lands work exactly the way bin/fm-merge-local.sh does (rebase onto a moved main, then fast-forward) and ran the actual fm-teardown.sh CLI on it twice: the base-commit script printed "REFUSED: ... has work not yet merged into main" and exited 1, while the fixed script completed teardown and exited 0 — with the same transcript proving the commit really is unreachable from main yet its content is fully landed. A second transcript exercises the refusal side on the fixed script: a never-landed commit still refuses and is named, a dirty worktree whose patches all landed refuses with the new "its commits already landed in main / Commit the uncommitted changes" wording, and plainly unmerged work refuses unchanged. No UI surface is involved, so CLI transcripts are the end-user-visible artifact. Working tree left clean.
Evidence: Rebase-landed teardown: before vs after the fix (real git rebase, real CLI)
$ git -C $WT merge-base --is-ancestor HEAD main; echo $? # 1 = commit NOT reachable from main 1 $ git -C $WT diff HEAD main -- beta.txt # ...yet the content is identical (no output = beta.txt is fully landed on main) ### BEFORE the fix (bin/fm-teardown.sh at d843712) $ fm-teardown.sh task-x1 REFUSED: local-only worktree .../wt has work not yet merged into main and not on any remote. commits not yet on main: 5a5cab2 add beta Merge the branch into local main first (bin/fm-merge-local.sh after the captain approves), or push to a fork/remote, or get the captain's explicit OK to discard, then --force. [exit 1] ### AFTER the fix (bin/fm-teardown.sh at HEAD) $ fm-teardown.sh task-x1 teardown task-x1 complete (window firstmate:fm-task-x1, worktree .../wt) [exit 0]Evidence: Refusals preserved on the fixed script (unlanded commit / dirty worktree / nothing landed)
### Safety case 1 - one commit landed by rebase, a second commit never landed REFUSED: local-only worktree .../wt has work not yet merged into main and not on any remote. commits not yet on main: 9beaaca add gamma (never landed) a22ba3e add beta [exit 1] ### Safety case 2 - every commit's patch landed by rebase, but the worktree is dirty REFUSED: local-only worktree .../wt has uncommitted changes; its commits already landed in main. uncommitted changes present Commit the uncommitted changes (or get the captain's explicit OK to discard, then --force). [exit 1] ### Safety case 3 - nothing landed at all (plain unmerged work) REFUSED: local-only worktree .../wt has work not yet merged into main and not on any remote. commits not yet on main: c8b3704 add beta [exit 1]Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-teardown.sh:910- The intent's carried-forward human decision requires: "The one change wanted from that round is removing the duplication between patches_are_in_ref() and unpushed_patches_are_in_pr_head(): duplicated logic in a safety-critical check is a real hazard, because a future fix applied to only one copy is the realistic failure mode." That dedup is absent from the change. bin/fm-teardown.sh:910 (patches_are_in_ref) and bin/fm-teardown.sh:812 (unpushed_patches_are_in_pr_head) still each carry their own copy of the identical logic: agit log --format=%H <range> | while read commit; do patch_id_for_commit; done | sed '/^$/d' | sort -ucollector, an[ -n "$ids" ] || return 1guard, and awhile read ... patch_id=$(patch_id_for_commit ...); [ -n "$patch_id" ] || return 1; grep -qxFall-must-match loop. Only the two rev ranges and the empty-candidate-list behavior differ. Suggested shape: onepatch_ids_for_range()helper plus oneall_patches_present <id-set> <commit-list>loop, with both callers supplying only their ranges.bin/fm-teardown.sh:1237- On the local-only path, when patches_are_in_ref() clearsunmergedbut the worktree is dirty, the refusal headline still reads "has work not yet merged into $DEFAULT and not on any remote" even though every commit provably landed and the only work at stake is the uncommitted change. The detail lines are correct (only "uncommitted changes present" prints), but the headline and the remedy line ("Merge the branch into local $DEFAULT first") point the captain at an action that is already done. The q3 test asserts the current text, so changing it is a deliberate product-copy decision.🔧 Fix: dedupe patch matching, fix landed-but-dirty refusal copy
1 info still open:
bin/fm-teardown.sh:1255- The new accurate dirty-but-landed refusal (line 1249) fires only when the patch scan cleared a previously non-emptyunmerged. A local-only worktree that is dirty but whose commits are plainly REACHABLE from $DEFAULT still falls through to the generic headline at line 1255 ("has work not yet merged into $DEFAULT and not on any remote") plus the "Merge the branch into local $DEFAULT first" remedy, even though nothing is unmerged there either. That inconsistency is pre-existing and was explicitly scoped out by the round-1 instruction ("Leave the wording of every other refusal case alone"), so no change is expected here - noting it only so the remaining rough edge is on record.✅ **Test** - passed
✅ No issues found.
bash tests/fm-teardown.test.sh— 62 ok / 0 failures, including the four new cases (q1 rebase-landed allow, q2 absent patch refuse, q3 landed-but-dirty refuse, q4 no-mistakes rebase-landed-then-edited allow)bash tests/fm-pr-merge.test.sh(10 ok) — covers the PR-head path now sharing the dedupedsubject_patches_are_in_reference()bash tests/fm-gate-refuse.test.sh(7 ok)bash tests/fm-teardown-endpoint-safety.test.sh(7 ok)bash tests/fm-secondmate-safety.test.sh(77 ok)Manual A/B CLI demo: built a local-only project where a second chain lands via realgit rebase+git merge --ff-only(the fm-merge-local.sh flow), then ranbin/fm-teardown.sh task-x1against the base-commit script and the HEAD script on the identical fixtureManual safety demo on the fixed script: (1) one commit landed by rebase plus a later never-landed commit, (2) all patches landed but worktree dirty, (3) plain unmerged work — each run throughbin/fm-teardown.sh task-x1✅ **Document** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.