fix(bin): prune merged task branches across all project modes - #2768
fix(bin): prune merged task branches across all project modes#2768trevorallred wants to merge 10 commits into
Conversation
fm-teardown.sh already dropped a task's own fm/<task-id> branch inline once its work was confirmed landed, but a local-only-mode (or origin-less) project had no backstop for a branch that survived past that point - fm-fleet-sync.sh's existing periodic prune only recognized a squash-merged PR's now-gone remote branch, and skipped the whole remote-backed sync (including that prune) for exactly the projects with no remote to notice "gone" tracking on. That gap matched a concrete trigger: five rapid local-only ship tasks against a local-only project each left their fm/<task-id> branch behind even though bin/fm-merge-local.sh had already fast-forward-merged every one of them, and firstmate cleaned them up by hand with `git branch -d`. Add bin/fm-branch-merge-lib.sh, the one owner of "is this branch provably safe to delete": not checked out in any worktree, and either an ancestor of the merged-into ref (a clean fast-forward or non-squash merge - git's own safe `branch -d` can verify this itself) or its upstream tracking reads "[gone]" (a squash-merged PR's remote branch was deleted). Never force-deletes past that proof. fm-fleet-sync.sh gains prune_merged_fm_branches, a git-only sweep of a project's own fm/* branches using that shared proof, run unconditionally before any mode/remote gate - so it also covers local-only and no-origin projects, which the existing remote-backed prune_gone_branches never reaches. fm-teardown.sh's own inline branch-drop is refactored (no behavior change) into one local helper instead of two duplicated copies. Regression coverage: fm-fleet-sync.test.sh gains cases proving the sweep prunes a genuinely fast-forward-merged fm/* branch (including with no origin remote at all), leaves an unmerged/diverged branch and one still checked out in an active worktree untouched, and never targets the project's own default branch. fm-teardown.test.sh's existing local-only-merged and no-pr-recorded/externally-merged-PR cases now also assert the task branch is actually dropped, closing the concrete trigger and confirming today's externally-merged-PR reconciliation already covers that case end to end.
Confidence Score: 4/5The PR should not merge until the merged-task pruning sweep requires explicit captain authorization rather than running by default. Fleet sync still invokes a branch-deleting sweep when the pruning environment setting is unset, so qualifying local task refs are removed without the explicit opt-in required for newly expanded destructive behavior. Files Needing Attention: bin/fm-fleet-sync.sh, docs/configuration.md Reviews (8): Last reviewed commit: "fix(bin): make the merged-branch backsto..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Speaking as Kun's firstmate: VISION: mixed. The new What is not merge-ready:
Security: none. Residual: once enabled, this is ref deletion; keep it captain-gated. Overlap / do not land together: CI: first-time fork workflows approved after diff review. Greptile is red. no-mistakes is pending and blocking. Ahead 6, behind 0, mergeable. Waiting on the author for the worktree race and the architecture.md default-on wording — not waiting on the captain. Merge-eligible: NO. Captain-flag NOW: NO. |
|
Speaking as Kun's firstmate: Corrective: after confirmed merge, delete the task's own Not merge-eligible while CI is unfinished. File overlap with held teardown pair #2637/#2692 on Waiting on CI — not waiting on the captain. |
The prior CI fix round gated prune_merged_fm_branches behind a new FM_FLEET_PRUNE_MERGED opt-in (default off), responding to an automated review concern that a destructive fleet-sync mutation should not default on. That concern does not hold here: fm-fleet-sync.sh is AGENTS.md's own named exception to "never write to a project" (fleet sync, secondmate sync, and a few other guarded paths are explicitly carved out), and prune_gone_branches in this exact file already deletes local branches from a project clone by default, gated only by the pre-existing FM_FLEET_PRUNE variable. prune_merged_fm_branches extends that same, already-authorized mechanism to close a coverage gap (local-only and no-origin projects), not new destructive authority - so it belongs under the same default-on gate, matching the concrete trigger this whole change exists to fix (a task branch left behind with nothing to notice or clean it up automatically). Reverts the gate to FM_FLEET_PRUNE (default on, matching prune_gone_branches), removes every FM_FLEET_PRUNE_MERGED reference from comments and docs, and drops the now-inapplicable test_merged_task_branch_requires_explicit_prune_authority test along with its run_sync_with_merged_prune helper, restoring the other prune-positive tests to plain run_sync. The atomic branch-d-from-a-detached-worktree delete mechanism, the expected-tip and worktree-race regression tests, and the [gone]-without-merge test from the intervening CI fix rounds are kept unchanged - only the gating variable and the prose/tests describing it move back to default-on.
|
Speaking as Kun's firstmate: VISION: aligns on cleanup durability and the refusal to discard unlanded work. Shared Class: corrective. Backstop sweep Security: none. Git-only ancestor proof, scoped to Overlap / HOLD: touches CI: HEAD Workflows approved: yes (CI 32583347298, NM 32583347319). Land-eligible: NO even if CI later greens, because of the pair hold. CI/NM not green this pass, so waiting on the author, not the captain. If later fully green except the pair, that is a captain-flag for the pair decision — not now. |
2 participants
Intent
fm-teardown.sh (and the merge scripts it works with - fm-merge-local.sh for local-only tasks, and the no-mistakes/direct-PR paths) did not delete a task's own fm/ branch after its work was confirmed merged. Concrete trigger: five rapid-fire local-only ship tasks against one project each left their fm/ branch behind even though every one had already been safely fast-forward-merged by fm-merge-local.sh; firstmate cleaned those up by hand with
git branch -d(a safe delete that only succeeds when git itself proves the branch is already fully merged - that same safety property must be encoded here, never a forced delete). An earlier one-off task (fleet-merged-branch-cleanup) had already found the same class of stale branches across multiple projects and delivery modes, confirming this is a general lifecycle gap, not local-only-specific.Required:
Regression coverage required: tests colocated in tests/ (.test.sh naming, extending an existing runner rather than inventing one) proving a genuinely merged branch gets deleted, an unmerged/diverged branch is left alone, and the periodic sweep doesn't touch protected/default branches or branches with an active worktree still checked out. Tests must exercise real behavior through the actual scripts, never assert on script source text.
Decisions made while implementing:
What Changed
fm/<task-id>branches, including local-only and no-origin projects, while preserving active worktrees and unmerged branches.Risk Assessment
🚨 High: The changed teardown helper still force-deletes a task branch despite the authoritative intent explicitly requiring non-forced deletion only when safety is provable.
Testing
Exercised the affected shell interfaces end-to-end with real temporary Git repositories: merged
fm/*branches were pruned in local-only and no-origin projects; unmerged, gone-but-unmerged, active-worktree, and default branches remained intact; teardown removed branches after both local and externally merged PR flows while refusing unpushed work. Reviewer-visible CLI transcripts were saved as evidence.Evidence: Fleet-sync branch lifecycle transcript
Source: Fleet-sync branch lifecycle transcript
Evidence: Teardown merged/local and externally merged PR transcript
Source: Teardown merged/local and externally merged PR transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-branch-merge-lib.sh:49-[gone]only proves that the upstream ref was deleted; it does not prove the branch was merged. A reachable path is an unmergedfm/<id>branch whose remote branch is manually deleted, followed byfetch --prune: this returns[gone]at line 49 and line 67 force-deletes its unique local commits. That contradicts the required invariant that unmerged/diverged branches are never force-deleted. Require an actual landedness proof before deletion, or leave[gone]branches for the existing teardown/PR reconciliation path.🔧 Fix: Prevent unsafe gone-upstream branch deletion
1 error still open:
bin/fm-teardown.sh:2421- The diff retains a forcedgit branch -Din the new shared teardown helper. This contradicts the required criterion, “Never force-delete an unmerged or diverged branch” and the stated requirement to retain Git’sbranch -dsafety guarantee. The preceding landedness check can permit squash/content-equivalent paths whose tip is not an ancestor; confirm whether preserving that existing behavior is an intended exception, or change this helper to use the shared safe-delete proof and plain-d.✅ **Test** - passed
✅ No issues found.
bash tests/fm-fleet-sync.test.shFocused teardown cases executed fromtests/fm-teardown.test.sh:test_local_only_merged_to_local_main_allows,test_no_pr_recorded_discovers_merged_pr_by_branch_allows, andtest_local_only_truly_unpushed_refuses✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed (3) ✅
🔧 Fix: Verify repository lint passes cleanly
1 warning still open:
🔧 Fix: Verify pinned lint passes cleanly
1 warning still open:
🔧 Fix: Verify repository lint passes cleanly
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.