feat(bin): automate safe task branch cleanup - #2999
trevorallred wants to merge 37 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.
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.
…missing-adapter-origin fix(bin): refuse teardown before cleanup when Herdr prerequisites are missing
Confidence Score: 4/5The PR is not yet safe to merge because routine fleet synchronization still performs destructive local branch cleanup without explicit captain authorization. The default fleet-sync path invokes gone-upstream pruning whenever FM_FLEET_PRUNE is unset and ultimately runs Files Needing Attention: bin/fm-fleet-sync.sh, bin/fm-branch-merge-lib.sh Reviews (9): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
Speaking as Kun's firstmate: scheduled 7:10pm PT 8/24 pass (FM-FMOSS-CRON). First look on current class=default-behavior. Inspected THIS DIFF, not the #2981 hold writeup. Related #2981 was mixed (adapter guard + default-on
On-demand cleanup + opt-in fleet prune would be opt-in. Default-on VISION.md (inspected
Overlap / holds: This HEAD: Security: git-ref deletion with fail-safe proofs; no secrets. Land-eligible: NO. Captain-flag NOW: no (NM mismatch is an author blocker; default-behavior is a captain-decision only when otherwise ready). waiting-on-author for HEAD-matching attestation. Did not squash. |
|
Speaking as Kun's firstmate: first look on current class=default-behavior (mixed). Fleet-sync prune is now opt-in ( VISION.md (inspected
This HEAD: Security: none (fail-safe git-ref delete; force/scout retain). Overlap / holds: Land-eligible rec: NO. Captain-flag NOW: no (NM mismatch + teardown hold overlap; do not escalate the product call while those stand). This is a captain-decision on default-on landed teardown drop, and waiting-on-author for a HEAD-matching attestation. Not a merge I will recommend. |
trevorallred
changed the title
132f21a to
9c9ac9f
Compare
|
Closing because this PR was opened against the public upstream template by mistake. The branch has been rebased onto trevorallred/firstmate's current main and will be submitted to that repository instead. |
2 participants
Intent
Automate provably safe cleanup of Firstmate task branches across both a project's GitHub origin and its separate local no-mistakes bare remote. Add --delete-branch to fm-pr-merge's gh-axi squash merge so GitHub deletion activates fm-fleet-sync's existing default-on gone-upstream pruning; verify and preserve that established gone-upstream-with-no-worktree proof. Extend the shared fm-branch-merge library as the one owner of cleanup proofs and exact-tip deletion, silently skip projects without a configured no-mistakes remote, wire ordinary landed ship-task cleanup into teardown using its existing GitHub-aware landedness proof, and provide fm-branch-cleanup.sh for a full on-demand sweep without duplicated safety logic. Every ordinary deletion must require an existing strong proof (ancestor of default, established gone-upstream proof, or shared GitHub-aware landedness); uncertainty and checked-out branches must be preserved. Keep the existing explicitly captain-authorized --force discard exception and disposable scout exception unchanged. Validate behavior with executable tests and real scratch repositories/remotes, including origin deletion, no-mistakes cleanup, remote exact-tip leases, repo-root resolution, temporary-worktree cleanup, pushed-but-unmerged preservation, and default-on sweep behavior. Serialize every branch-cleanup path with a repository-common per-branch lock acquired across the complete check/detach/delete sequence; retain in-lock exact-tip comparisons, Git's native branch -D linked-worktree refusal for local deletion, and force-with-lease for remote deletion. Cover competing locked ref movement, concurrent linked-worktree checkout at the native deletion boundary, and the remote-only checkout race. Do not change Herdr lifecycle behavior or fix herdr-preflight-missing-adapter: that test failure is independently confirmed pre-existing on clean main and unrelated to this task.
What Changed
fm/*task branches across local,origin, and configuredno-mistakesremotes.Risk Assessment
✅ Low: The change centralizes locked proof-and-delete operations, preserves exact-tip and native worktree safeguards, and the reviewed call paths conform to the required cleanup behavior.
Testing
Targeted end-to-end Git validation exercised real origin and no-mistakes bare remotes: proven landed branches were cleaned up, unmerged and checked-out branches were preserved, GitHub squash/merge deletion and default-on gone-upstream pruning worked, and exact-tip leases plus per-branch locks protected concurrent ref/worktree races. Evidence transcripts were saved in the designated evidence directory; no UI is involved because this is a shell/Git CLI change.
Evidence: Branch cleanup E2E transcript
Source: Branch cleanup E2E transcript
Focused end-to-end branch-cleanup scenarios exercised real scratch Git repositories and bare remotes. The transcript records deletion of landed branches from both remotes, preservation of unlanded and checked-out branches, squash-merge proof reuse, exact-tip remote lease protection, remote-only checkout locking, and GitHub merge deletion.Evidence: Fleet-sync cleanup transcript
Source: Fleet-sync cleanup transcript
Fleet-sync scenarios demonstrate default-on gone-upstream pruning, disable behavior, active-worktree preservation, protected-default exclusion, ref-movement locking, and native linked-worktree deletion refusal.Evidence: Teardown cleanup transcript
Source: Teardown cleanup transcript
Teardown scenario demonstrates ordinary landed ship-task cleanup removes the exact branch tip from both origin and the no-mistakes bare remote.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
bin/fm-branch-cleanup.sh:65- Captain, the required "lock acquired across the complete check/detach/delete sequence" is not met for remote-only sweep candidates: this path checks worktree state, fetches the tip, and establishes GitHub/content landedness before enteringfm_branch_delete_remote_proven_tip's lock. Move this whole candidate proof and delete into one locked callback so the final deletion is authorized by an in-lock proof.🔧 Fix: Moved remote candidate proof inside branch lock
1 error still open:
bin/fm-branch-merge-lib.sh:231- The required “lock acquired across the complete check/detach/delete sequence” is still violated for the remote-delete callers.fm_branch_delete_remote_proven_tipacquires the lock only after its caller has established landedness:fm-branch-cleanup.shproves merged/gone/content at lines 95–115, andfm-fleet-sync.shproves[gone]at line 232. While waiting for the lock, a fetch can rewind the default ref or restore the upstream tracking ref without changing the candidate tip; this helper then checks only tip/worktree state and deletes the local and remote refs although the original proof is no longer true. Add shared locked merged/gone/landed proof-and-delete operations in this library and route those callers through them, so the proof is re-established under the same lock as deletion.🔧 Fix: Lock remote cleanup proofs with deletion
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-branch-cleanup.test.sh(first four end-to-end scenarios completed before the harness’s fixed 30-second command window)isolated execution oftest_pr_merge_delete_flag_drives_real_origin_deletionfromtests/fm-branch-cleanup.test.shbin/fm-test-run.sh tests/fm-fleet-sync.test.sh(relevant cleanup/locking/default-on scenarios completed before the harness window)isolated execution oftest_teardown_prunes_landed_task_from_both_remotesfromtests/fm-teardown.test.sh✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.