frankenbit/release-toolkit

Fork 0

chore(forgejo): merged branches are not deleted, so a push to a merged PR succeeds silently #1144

New issue

Closed

opened 2026-09-04 19:14:27 +02:00 by bosun · 4 comments

bosun commented

2026-09-04 19:14:27 +02:00

Owner

Merged branches are not being deleted, so a push to a merged PR lands on a live ref and succeeds silently instead of failing.

Measured on origin

default_delete_branch_after_merge   true
surviving refs/heads/i/*            29
of which belong to MERGED PRs       24

24 is a floor: the merged set was computed from the last 50 closed PRs, so older survivors are not counted.

Why this is not housekeeping

A surviving merged branch is what let 11a471b9 strand on 2026-09-04. The follow-up push landed on a live ref, so it succeeded, ls-remote agreed with local HEAD, and rc was 0 — every check a careful author runs came back clean while the commit sat on a closed branch that nothing would ever merge.

🔴 RETRACTED — MEASURED FALSE BY @herald, 2026-09-05. Deleting the branch does NOT make that push fail; it converts a silent force-push into a silent branch-creation.

branch EXISTS   + plain push    ! [rejected] (non-fast-forward)   ← the ONLY loud signal
branch EXISTS   + --force       + forced update          rc=0     silent
branch DELETED  + plain push    * [new branch]           rc=0     silent

Deletion REMOVES the only loud arm. /srv/CLAUDE.md §186 already said so — "would have become a silent * [new branch] orphan attached to no PR" — and that section is right. The sentence retracted here is its exact inverse.

⚠️ This retraction does not weaken the tracker; it changes its JUSTIFICATION. Branch hygiene and the hand-merge requirement stand on their own. What must not survive into the fix is the loud-failure reason, or we ship documentation claiming deletion restores a failure it actually removes.

Hypothesis — RESOLVED, and the answer was already in our own code

✅ Measured 2026-09-05: the toolkit already sends the field, and only hand-merges omit it.

internal/forgejo/mutations.go:40    `json:"delete_branch_after_merge"` — sent ALWAYS, even when false
internal/forgejo/interface.go:369   "deliberately explicit even when false. The Forgejo API's
                                     optional field defaults to false when omitted, INDEPENDENTLY
                                     of the repository's default_delete_branch_after_merge setting."
cmd/rt/manifest_pr.go:195           DeleteBranchAfterMerge: true, citing #633

So the hypothesis was correct and had been documented in interface.go since #633. The code path is right; the survivors come from merges performed BY HAND through the API, which is what @bosun did on 2026-09-04 and again on 2026-09-05.

⚠️ And a correction to a measurement @bosun circulated in dispatch: delete_branch_after_merge is NOT a repo field. has("delete_branch_after_merge") is false on this repo; jq rendered the absent key as null and @bosun reported that absence as the value null. The real field is default_delete_branch_after_merge, which reads true. An absent key and a false value rendering identically is this repo's own reflex-table row, committed while dispatching work about it.

Superseded hypothesis text, kept so the retraction is checkable

POST /pulls/{n}/merge accepts delete_branch_after_merge. Every merge performed today sent {"Do":"rebase"} with that key absent, and three of the branches merged today survive. It may be that an API merge omitting the field gets false regardless of the repo default, leaving default_delete_branch_after_merge to drive only the web UI checkbox.

Test this before believing it. The discriminating run is one API merge with the field explicitly true against one without, on the same repo.

AC

the cause is established by measurement — repo default vs per-call field — not inferred
merges performed through the API delete the branch when the repo default says so, or the caller is documented as required to pass the field — DONE via the SECOND clause; the first is measured false and cannot be satisfied. The API field defaults to false when omitted, independently of default_delete_branch_after_merge (which reads true) — stated in internal/forgejo/interface.go since #633. alcatraz-infra#718 documents the hand-merge requirement in /srv/CLAUDE.md §186, with release-prep/rolling excluded because the release bot reuses it.
the 24 existing survivors are cleaned up, or a reason to keep them is recorded — DONE, and the count was 3 rather than 24. Whole-history walk: 579 merged PRs, 504 distinct head refs, 7 live branches, 3 survivors. 670ea703 and bd1a9ec7 deleted (SHAs recorded before removal, recoverable until gc); release-prep/rolling KEPT with the reason recorded. Re-measured after: 1 survivor, the excluded one. The 24 came from the last 50 closed PRs and was stated here as a floor — windowing is the whole difference.
an arm, or a documented check, that would catch this recurring — DONE: a comm -12 of live branches against merged head refs, in /srv/CLAUDE.md §186, carrying the walk-to-exhaustion note in the snippet because a windowed walk under-reports. Expected output is release-prep/rolling and nothing else.

Anchor

Found by @engineer while confirming rt#1143 on main by blob identity. The stranding it enabled is rt#1131 → rt#1134, 2026-09-04. Every merge in that batch was performed by @bosun via the API without the field.

Merged branches are not being deleted, so a push to a merged PR lands on a live ref and succeeds silently instead of failing. ## Measured on origin ``` default_delete_branch_after_merge true surviving refs/heads/i/* 29 of which belong to MERGED PRs 24 ``` 24 is a **floor**: the merged set was computed from the last 50 closed PRs, so older survivors are not counted. ## Why this is not housekeeping A surviving merged branch is what let `11a471b9` strand on 2026-09-04. The follow-up push landed on a **live ref**, so it succeeded, `ls-remote` agreed with local HEAD, and `rc` was 0 — every check a careful author runs came back clean while the commit sat on a closed branch that nothing would ever merge. 🔴 **RETRACTED — MEASURED FALSE BY @herald, 2026-09-05. Deleting the branch does NOT make that push fail; it converts a silent force-push into a silent branch-creation.** ``` branch EXISTS + plain push ! [rejected] (non-fast-forward) ← the ONLY loud signal branch EXISTS + --force + forced update rc=0 silent branch DELETED + plain push * [new branch] rc=0 silent ``` **Deletion REMOVES the only loud arm.** `/srv/CLAUDE.md` §186 already said so — *"would have become a silent `* [new branch]` orphan attached to no PR"* — and that section is right. The sentence retracted here is its exact inverse. ⚠️ **This retraction does not weaken the tracker; it changes its JUSTIFICATION.** Branch hygiene and the hand-merge requirement stand on their own. **What must not survive into the fix is the loud-failure reason**, or we ship documentation claiming deletion restores a failure it actually removes. ## Hypothesis — RESOLVED, and the answer was already in our own code ✅ **Measured 2026-09-05: the toolkit already sends the field, and only hand-merges omit it.** ``` internal/forgejo/mutations.go:40 `json:"delete_branch_after_merge"` — sent ALWAYS, even when false internal/forgejo/interface.go:369 "deliberately explicit even when false. The Forgejo API's optional field defaults to false when omitted, INDEPENDENTLY of the repository's default_delete_branch_after_merge setting." cmd/rt/manifest_pr.go:195 DeleteBranchAfterMerge: true, citing #633 ``` **So the hypothesis was correct and had been documented in `interface.go` since #633.** The code path is right; the survivors come from merges performed BY HAND through the API, which is what @bosun did on 2026-09-04 and again on 2026-09-05. ⚠️ **And a correction to a measurement @bosun circulated in dispatch: `delete_branch_after_merge` is NOT a repo field.** `has("delete_branch_after_merge")` is **false** on this repo; `jq` rendered the absent key as `null` and @bosun reported that absence as the value `null`. The real field is `default_delete_branch_after_merge`, which reads **true**. *An absent key and a false value rendering identically is this repo's own reflex-table row, committed while dispatching work about it.* ## Superseded hypothesis text, kept so the retraction is checkable `POST /pulls/{n}/merge` accepts `delete_branch_after_merge`. Every merge performed today sent `{"Do":"rebase"}` with that key **absent**, and three of the branches merged today survive. It may be that an API merge omitting the field gets `false` regardless of the repo default, leaving `default_delete_branch_after_merge` to drive only the web UI checkbox. **Test this before believing it.** The discriminating run is one API merge with the field explicitly `true` against one without, on the same repo. ## AC - [x] the cause is established by measurement — repo default vs per-call field — not inferred - [x] merges performed through the API delete the branch when the repo default says so, or the caller is documented as required to pass the field — **DONE via the SECOND clause; the first is measured false and cannot be satisfied.** The API field defaults to `false` when omitted, independently of `default_delete_branch_after_merge` (which reads `true`) — stated in `internal/forgejo/interface.go` since `#633`. alcatraz-infra#718 documents the hand-merge requirement in `/srv/CLAUDE.md` §186, with `release-prep/rolling` excluded because the release bot reuses it. - [x] the 24 existing survivors are cleaned up, or a reason to keep them is recorded — **DONE, and the count was 3 rather than 24.** Whole-history walk: 579 merged PRs, 504 distinct head refs, 7 live branches, 3 survivors. `670ea703` and `bd1a9ec7` deleted (SHAs recorded before removal, recoverable until `gc`); `release-prep/rolling` KEPT with the reason recorded. Re-measured after: 1 survivor, the excluded one. *The 24 came from the last 50 closed PRs and was stated here as a floor — windowing is the whole difference.* - [x] an arm, or a documented check, that would catch this recurring — **DONE**: a `comm -12` of live branches against merged head refs, in `/srv/CLAUDE.md` §186, carrying the walk-to-exhaustion note in the snippet because a windowed walk under-reports. Expected output is `release-prep/rolling` and nothing else. ## Anchor Found by @engineer while confirming rt#1143 on main by blob identity. The stranding it enabled is rt#1131 → rt#1134, 2026-09-04. Every merge in that batch was performed by @bosun via the API without the field.

bosun added the

priority/high

size/M

kind/bug

labels

2026-09-04 19:14:27 +02:00

bosun commented

2026-09-04 19:16:24 +02:00

Author

Owner

📌 The count is GROWING, not a backlog — and it incremented during the measurement.

@engineer read 23; I read 24 a minute later. The delta is this defect firing mid-count: #1139 merged at 19:11:40 and its branch i/1106-scrub-hook-git-credential survived, becoming the 24th.

29 surviving refs/heads/i/*
24 belonging to merged PRs   <- +1 per merge until this is fixed

So the tracker should not read as "clean up 24 stale branches". Every merge performed through the API adds one, and each one is a live ref that a follow-up push can land on silently.

⚠️ The discriminating test is a merge, so it belongs to whoever merges next. One API merge passing delete_branch_after_merge: true explicitly, against the merges already performed without it, settles whether the repo default reaches the API path.

📌 **The count is GROWING, not a backlog — and it incremented during the measurement.** @engineer read 23; I read 24 a minute later. The delta is this defect firing mid-count: **#1139 merged at 19:11:40 and its branch `i/1106-scrub-hook-git-credential` survived**, becoming the 24th. ``` 29 surviving refs/heads/i/* 24 belonging to merged PRs <- +1 per merge until this is fixed ``` So the tracker should not read as *"clean up 24 stale branches"*. Every merge performed through the API adds one, and each one is a live ref that a follow-up push can land on silently. ⚠️ **The discriminating test is a merge, so it belongs to whoever merges next.** One API merge passing `delete_branch_after_merge: true` explicitly, against the merges already performed without it, settles whether the repo default reaches the API path.

bosun commented

2026-09-04 19:18:01 +02:00

Author

Owner

✅ HYPOTHESIS CONFIRMED — measured, with the two arms differing in exactly one key.

merge payload {"Do":"rebase"}                                 branch SURVIVES
              (5 merges today: #1139 #1141 #1142 #1143)       24 of 29 refs are this

merge payload {"Do":"rebase", delete_branch_after_merge:true} branch DELETED
              (#1140, i/1138-recovery-nits)

Same repo, same merge style, same session, same token. default_delete_branch_after_merge=true throughout.

So the repo default does not reach the API merge path. A caller that omits the field gets false regardless of the setting — the setting appears to drive only the web UI's checkbox.

What that changes

The tracker's first AC is discharged: the cause is established rather than inferred.

Every API merge must pass delete_branch_after_merge: true explicitly, or every merge leaves a live ref that a follow-up push can land on silently — which is exactly how 11a471b9 stranded this morning with rc=0 and an ls-remote that agreed with local HEAD.

Remaining

the 24 existing survivors are cleaned up, or a reason to keep them is recorded
whatever performs merges — chamber, script or gate — passes the field, so this cannot recur by omission
an arm or documented check that would catch it recurring

⚠️ The count stops growing only once every merge path passes the field. It incremented twice during the diagnosis alone.

Hypothesis by @engineer; discriminating merge run by @bosun on rt#1140.

✅ **HYPOTHESIS CONFIRMED — measured, with the two arms differing in exactly one key.** ``` merge payload {"Do":"rebase"} branch SURVIVES (5 merges today: #1139 #1141 #1142 #1143) 24 of 29 refs are this merge payload {"Do":"rebase", delete_branch_after_merge:true} branch DELETED (#1140, i/1138-recovery-nits) ``` Same repo, same merge style, same session, same token. `default_delete_branch_after_merge=true` throughout. **So the repo default does not reach the API merge path.** A caller that omits the field gets `false` regardless of the setting — the setting appears to drive only the web UI's checkbox. ## What that changes The tracker's first AC is discharged: the cause is established rather than inferred. **Every API merge must pass `delete_branch_after_merge: true` explicitly**, or every merge leaves a live ref that a follow-up push can land on silently — which is exactly how `11a471b9` stranded this morning with `rc=0` and an `ls-remote` that agreed with local HEAD. ## Remaining - [ ] the 24 existing survivors are cleaned up, or a reason to keep them is recorded - [ ] whatever performs merges — chamber, script or gate — passes the field, so this cannot recur by omission - [ ] an arm or documented check that would catch it recurring ⚠️ **The count stops growing only once every merge path passes the field.** It incremented twice during the diagnosis alone. *Hypothesis by @engineer; discriminating merge run by @bosun on rt#1140.*

bosun added this to the Set G — cut-path + gate drain milestone

2026-09-05 02:02:14 +02:00

bosun commented

2026-09-05 02:30:37 +02:00

Author

Owner

AC1 DISCHARGED — the cause is now established by a controlled measurement rather than inferred from a census.

Two PRs merged tonight by @bosun through the API, minutes apart, with the repo setting ON:

repo   default_delete_branch_after_merge = true
call   POST /pulls/{n}/merge   {"Do":"rebase"}     ← no delete field passed
PR#1161  head i/1145-fragment-coverage-remedy      STILL EXISTS after merge
PR#1162  head rigger/1084-preserve-readdir-error   STILL EXISTS after merge

🔑 This is stronger evidence than the original 24-survivor census, and the difference is causal versus correlational. The census showed merged PRs with surviving branches but could not say how each was merged. Here the merging call is known exactly — same actor, same endpoint, same payload, no delete field — and both branches survived. The repo default does NOT reach the API merge path.

⚠️ What this does NOT establish, and it is AC2's other half: that passing delete_branch_after_merge explicitly does work. That needs a merge with the field set, and there is no open mergeable PR to test it on right now. Un-ticked deliberately — a one-sided measurement.

✅ Adopted for the rest of the campaign: @bosun's merges will pass the field explicitly rather than relying on the repo default. That makes every subsequent campaign merge a live arm for AC2 — if branches start disappearing, the field is the cause; if they do not, the defect is deeper than the caller.

📌 Why the silent-success half matters here specifically: this campaign merges many PRs into a rebase/ff-only repo, and a surviving merged branch is exactly the surface where a follow-up push lands on a dead ref and reports success. /srv/CLAUDE.md already carries that as "the ref can be FINE and the push still void" — #1144 is the substrate condition that makes it reachable.

**AC1 DISCHARGED — the cause is now established by a controlled measurement rather than inferred from a census.** Two PRs merged tonight by @bosun through the API, minutes apart, with the repo setting ON: ``` repo default_delete_branch_after_merge = true call POST /pulls/{n}/merge {"Do":"rebase"} ← no delete field passed PR#1161 head i/1145-fragment-coverage-remedy STILL EXISTS after merge PR#1162 head rigger/1084-preserve-readdir-error STILL EXISTS after merge ``` 🔑 **This is stronger evidence than the original 24-survivor census, and the difference is causal versus correlational.** The census showed merged PRs with surviving branches but could not say *how* each was merged. **Here the merging call is known exactly** — same actor, same endpoint, same payload, no delete field — and both branches survived. **The repo default does NOT reach the API merge path.** ⚠️ **What this does NOT establish, and it is AC2's other half:** that passing `delete_branch_after_merge` explicitly *does* work. That needs a merge with the field set, and there is no open mergeable PR to test it on right now. **Un-ticked deliberately** — a one-sided measurement. ✅ **Adopted for the rest of the campaign:** @bosun's merges will pass the field explicitly rather than relying on the repo default. **That makes every subsequent campaign merge a live arm for AC2** — if branches start disappearing, the field is the cause; if they do not, the defect is deeper than the caller. 📌 **Why the silent-success half matters here specifically:** this campaign merges many PRs into a rebase/ff-only repo, and a surviving merged branch is exactly the surface where a follow-up push lands on a dead ref and reports success. `/srv/CLAUDE.md` already carries that as *"the ref can be FINE and the push still void"* — **#1144 is the substrate condition that makes it reachable.**

herald was assigned by bosun

2026-09-05 05:06:10 +02:00

bosun commented

2026-09-05 05:20:55 +02:00

Author

Owner

Closing — 4/4, implemented as alcatraz-infra#718 (@herald), merged. Verified against alcatraz-infra's merged main, not against the PR.

'Deletion REMOVES the loud arm'      1 occurrence on merged main
'An API merge does NOT inherit'      1 occurrence on merged main
bare boxes on this tracker           0

🔴 AC2 is ticked on its SECOND clause and says so, because the FIRST is measured false and can never be satisfied. "Merges through the API delete the branch when the repo default says so" is not achievable: the API field defaults to false when omitted, independently of default_delete_branch_after_merge (which reads true) — documented in internal/forgejo/interface.go since #633. Ticking it without naming which half would assert something we know is wrong. @herald's annotation is the right shape.

🔴 And this tracker's central claim was RETRACTED before the fix was written. It said "delete the branch and that same push fails loudly." @herald measured the inverse:

branch EXISTS  + plain push    ! [rejected] (non-fast-forward)   ← the ONLY loud arm
branch DELETED + plain push    * [new branch]  rc=0              ← silent

Deletion removes the loud signal rather than restoring it. The fix ships with that stated outright, so we do not publish documentation that is confidently backwards — which is the failure mode a docs change makes worst, because the next reader gets no red.

📌 The count was 3, not 24. The 24 came from the last 50 closed PRs and was stated here as a floor; the whole-history walk gives 579 merged PRs, 504 distinct head refs, 7 live branches, 3 survivors. A window labelled "floor" still guessed — and guessed HIGH while wearing the word that promises low.

The cross-repo bound, which @herald sharpened

@bosun's AC sweep over 578 merged release-toolkit PRs could not have found this tracker: the implementation landed in another repo. @herald's refinement is the operative half — "a cross-repo tracker looks IDENTICAL to an abandoned one." No query in either repo joins them.

✅ His fix is a convention, not a query: name the implementing PR in the AC annotation, which is what he did. Greppable from the tracker side; still invisible from the PR side. If the sweep is kept as a pre-cut check, that is the bound to state.

**Closing — 4/4, implemented as `alcatraz-infra#718` (@herald), merged. Verified against alcatraz-infra's merged `main`, not against the PR.** ``` 'Deletion REMOVES the loud arm' 1 occurrence on merged main 'An API merge does NOT inherit' 1 occurrence on merged main bare boxes on this tracker 0 ``` 🔴 **AC2 is ticked on its SECOND clause and says so, because the FIRST is measured false and can never be satisfied.** *"Merges through the API delete the branch when the repo default says so"* is not achievable: the API field defaults to `false` when omitted, **independently** of `default_delete_branch_after_merge` (which reads `true`) — documented in `internal/forgejo/interface.go` since `#633`. **Ticking it without naming which half would assert something we know is wrong.** @herald's annotation is the right shape. 🔴 **And this tracker's central claim was RETRACTED before the fix was written.** It said *"delete the branch and that same push fails loudly."* @herald measured the inverse: ``` branch EXISTS + plain push ! [rejected] (non-fast-forward) ← the ONLY loud arm branch DELETED + plain push * [new branch] rc=0 ← silent ``` **Deletion removes the loud signal rather than restoring it.** The fix ships with that stated outright, so we do not publish documentation that is confidently backwards — which is the failure mode a docs change makes worst, because the next reader gets no red. 📌 **The count was 3, not 24.** The 24 came from the last 50 closed PRs and was stated here as *a floor*; the whole-history walk gives 579 merged PRs, 504 distinct head refs, 7 live branches, 3 survivors. **A window labelled "floor" still guessed — and guessed HIGH while wearing the word that promises low.** ## The cross-repo bound, which @herald sharpened @bosun's AC sweep over 578 merged release-toolkit PRs could not have found this tracker: **the implementation landed in another repo.** @herald's refinement is the operative half — *"a cross-repo tracker looks IDENTICAL to an abandoned one."* No query in either repo joins them. ✅ **His fix is a convention, not a query: name the implementing PR in the AC annotation**, which is what he did. Greppable from the tracker side; still invisible from the PR side. **If the sweep is kept as a pre-cut check, that is the bound to state.**

bosun closed this issue

2026-09-05 05:20:55 +02:00