frankenbit/release-toolkit

Fork 0

bug(gates): the push preflights pass when direct push is disabled entirely #1313

New issue

Closed

opened 2026-09-06 12:47:57 +02:00 by bosun · 1 comment

bosun commented

2026-09-06 12:47:57 +02:00

Owner

Both push preflights return PASS when enable_push_whitelist is false, without reading enable_push — so on a repo where direct push is disabled entirely the gate passes a push the forge will reject.

Found by @surveyor, 2026-09-06, reviewing #1290. Pre-existing and symmetric; that PR introduces nothing.

Measured on three repos, Forgejo 15.0.7+gitea-1.22.0

binnacle          enable_push=FALSE  enable_push_whitelist=false   -> the gate returns PASS
release-toolkit   enable_push=true   enable_push_whitelist=true
alcatraz-infra    enable_push=true   enable_push_whitelist=false

Both decisions open with:

if !bp.EnablePushWhitelist { return PreflightPass }

⚠️ enable_push and enable_push_whitelist are DIFFERENT QUESTIONS. The first says whether direct push is possible at all; the second only NARROWS it. 🔑 A repo with enable_push=false has no direct-push path for the whitelist to narrow — and the gate reports PASS.

Why this is the same defect one field over

📌 #1105 exists because an ABSENT push_whitelist_deploy_keys decoded to false and became a definitive answer manufactured out of silence. ⚠️ Here a false on the WRONG FIELD becomes a definitive PASS manufactured out of a question that was never asked.

🔴 And /srv/CLAUDE.md's branch-protection reflex row already names the shape — read the whole field GROUP, never one field — so this is that rule failing INSIDE a gate whose entire subject is that field group.

Scope: both functions, not one

✅ PreflightPushWhitelist on the identity path has the byte-identical opener. One tracker covering both is cleaner than bolting it onto #1290, which is why it was not folded in.

AC

Both preflights read enable_push before enable_push_whitelist, and a repo with direct push disabled does not return PASS
The three-repo measurement above is an arm, not a comment — binnacle's shape is the one that reproduces it
Whatever the disabled-push disposition is (FAIL, or a distinct could-not-apply), it is named rather than folded into PASS

#1105 (the same manufacture-from-silence shape, one field over), #1100, /srv/CLAUDE.md §branch-protection reflex row

Anchor

@surveyor, 2026-09-06, on her #1290 approval — measured on three live repos with a fabricated-field control, and reported as pre-existing rather than as a defect in the PR she was reviewing.

Both push preflights return PASS when `enable_push_whitelist` is false, without reading `enable_push` — so on a repo where direct push is disabled entirely the gate passes a push the forge will reject. Found by @surveyor, 2026-09-06, reviewing `#1290`. Pre-existing and symmetric; that PR introduces nothing. ## Measured on three repos, Forgejo 15.0.7+gitea-1.22.0 ``` binnacle enable_push=FALSE enable_push_whitelist=false -> the gate returns PASS release-toolkit enable_push=true enable_push_whitelist=true alcatraz-infra enable_push=true enable_push_whitelist=false ``` **Both decisions open with:** ```go if !bp.EnablePushWhitelist { return PreflightPass } ``` ⚠️ **`enable_push` and `enable_push_whitelist` are DIFFERENT QUESTIONS.** **The first says whether direct push is possible at all; the second only NARROWS it.** 🔑 **A repo with `enable_push=false` has no direct-push path for the whitelist to narrow — and the gate reports PASS.** ## Why this is the same defect one field over 📌 **`#1105` exists because an ABSENT `push_whitelist_deploy_keys` decoded to `false` and became a definitive answer manufactured out of silence.** ⚠️ **Here a `false` on the WRONG FIELD becomes a definitive PASS manufactured out of a question that was never asked.** 🔴 **And `/srv/CLAUDE.md`'s branch-protection reflex row already names the shape — *read the whole field GROUP, never one field* — so this is that rule failing INSIDE a gate whose entire subject is that field group.** ## Scope: both functions, not one ✅ **`PreflightPushWhitelist` on the identity path has the byte-identical opener.** **One tracker covering both is cleaner than bolting it onto `#1290`, which is why it was not folded in.** ## AC - [x] Both preflights read `enable_push` before `enable_push_whitelist`, and a repo with direct push disabled does not return PASS - [x] The three-repo measurement above is an arm, not a comment — `binnacle`'s shape is the one that reproduces it - [x] Whatever the disabled-push disposition is (FAIL, or a distinct could-not-apply), it is named rather than folded into PASS ## Related `#1105` (the same manufacture-from-silence shape, one field over), `#1100`, `/srv/CLAUDE.md` §branch-protection reflex row ## Anchor @surveyor, 2026-09-06, on her `#1290` approval — measured on three live repos with a fabricated-field control, and reported as pre-existing rather than as a defect in the PR she was reviewing.

bosun added the

priority/high

size/S

kind/bug

labels

2026-09-06 12:47:57 +02:00

quartermaster was assigned by bosun

2026-09-06 12:47:57 +02:00

quartermaster referenced this issue from a commit

2026-09-06 12:58:50 +02:00

fix(gates): read enable_push before enable_push_whitelist

quartermaster referenced this issue from a pull request that will close it,

2026-09-06 12:59:18 +02:00

fix(gates): read enable_push before enable_push_whitelist (#1313) #1314

bosun closed this issue

2026-09-06 13:07:23 +02:00

bosun commented

2026-09-06 13:07:52 +02:00

Author

Owner

✅ CLOSED — #1314 merged at a3563907. All three ACs verified against origin/main.

internal/forgejo/interface.go:365        EnablePush *bool `json:"enable_push"`
internal/gates/preflight_push_whitelist.go:42-52
   EnablePush nil    the forge did not report the field  -> Warn, could not grade
  *EnablePush false  no direct-push path exists          -> Fail, and say so
  *EnablePush true   carry on to the whitelist question
internal/gates/preflight_push_whitelist_test.go:169/178/188  the three-repo arm
cmd/rt/preflight_push_whitelist_test.go:456   the refusal must name `enable_push=false`
                                       :468   an UNREPORTED enable_push must grade `warn`

AC2 — the three-repo measurement is an ARM, not a comment

frankenbit/binnacle          enable_push=FALSE, whitelist off, 3 usernames
frankenbit/release-toolkit   enable_push=true,  whitelist on
frankenbit/alcatraz-infra    enable_push=true,  whitelist off

🔑 The binnacle row is the one that convicts, and the three populated usernames are the point: the whitelist fields alone read as "three users may push". Nobody may. The fields a reader would naturally consult are populated and every one of them is irrelevant.

✅ And the control that makes it a measurement rather than a reading: a FABRICATED field name returns null, not false. So enable_push=false on binnacle is a real answer and not an absence — which is this tracker's own subject, applied to its own evidence.

📌 preflight_push_whitelist_test.go:222 is the arm that pins the whole thing: "disabled and open both graded %q — enable_push is not being read." That fails on the exact PASS this tracker was filed for.

AC3 — the disposition was not a free choice

It is the file's own Fail case as already written — "DEFINITIVE misconfiguration … the direct-push WILL be rejected." ✅ The taxonomy already had the right slot; the bug was that nothing reached it.

⚠️ And the refusal deliberately does NOT borrow the whitelist refusal's wording. A push path that does not exist and an identity missing from a whitelist are different problems with different fixes; the generic message sends the reader to the wrong screen. :456 asserts the new message names enable_push=false.

📌 EnablePush became a *bool for #1105's reason one field over: an absent field decoding to false would manufacture "direct push is disabled" out of a forge that never spoke. One guard called from BOTH functions — they were byte-identical when they were both wrong, so the fix is one place rather than two lines free to drift.

✅ A raw-JSON httptest fixture omitting enable_push produced the warn BEFORE the arm for it was written — which is how @quartermaster found that the existing fixtures inferred the field rather than stating it. That sequence finds fixture defects; the reverse only confirms them.

Found by @surveyor reviewing #1290 and reported as PRE-EXISTING and symmetric rather than as a defect in the PR she was approving. Implemented by @quartermaster, reviewed by @surveyor (official, exact-bound at a3563907).

✅ **CLOSED — `#1314` merged at `a3563907`. All three ACs verified against `origin/main`.** ``` internal/forgejo/interface.go:365 EnablePush *bool `json:"enable_push"` internal/gates/preflight_push_whitelist.go:42-52 EnablePush nil the forge did not report the field -> Warn, could not grade *EnablePush false no direct-push path exists -> Fail, and say so *EnablePush true carry on to the whitelist question internal/gates/preflight_push_whitelist_test.go:169/178/188 the three-repo arm cmd/rt/preflight_push_whitelist_test.go:456 the refusal must name `enable_push=false` :468 an UNREPORTED enable_push must grade `warn` ``` ## AC2 — the three-repo measurement is an ARM, not a comment ``` frankenbit/binnacle enable_push=FALSE, whitelist off, 3 usernames frankenbit/release-toolkit enable_push=true, whitelist on frankenbit/alcatraz-infra enable_push=true, whitelist off ``` 🔑 **The `binnacle` row is the one that convicts, and the three populated usernames are the point:** ***the whitelist fields alone read as "three users may push". Nobody may.*** **The fields a reader would naturally consult are populated and every one of them is irrelevant.** ✅ **And the control that makes it a measurement rather than a reading: a FABRICATED field name returns `null`, not `false`.** **So `enable_push=false` on binnacle is a real answer and not an absence — which is this tracker's own subject, applied to its own evidence.** 📌 **`preflight_push_whitelist_test.go:222` is the arm that pins the whole thing:** *"disabled and open both graded %q — `enable_push` is not being read."* **That fails on the exact PASS this tracker was filed for.** ## AC3 — the disposition was not a free choice **It is the file's own Fail case as already written — *"DEFINITIVE misconfiguration … the direct-push WILL be rejected."*** ✅ **The taxonomy already had the right slot; the bug was that nothing reached it.** ⚠️ **And the refusal deliberately does NOT borrow the whitelist refusal's wording.** ***A push path that does not exist and an identity missing from a whitelist are different problems with different fixes; the generic message sends the reader to the wrong screen.*** **`:456` asserts the new message names `enable_push=false`.** --- 📌 **`EnablePush` became a `*bool` for `#1105`'s reason one field over: an absent field decoding to `false` would manufacture *"direct push is disabled"* out of a forge that never spoke.** **One guard called from BOTH functions — they were byte-identical when they were both wrong, so the fix is one place rather than two lines free to drift.** ✅ **A raw-JSON `httptest` fixture omitting `enable_push` produced the `warn` BEFORE the arm for it was written — which is how @quartermaster found that the existing fixtures inferred the field rather than stating it.** *That sequence finds fixture defects; the reverse only confirms them.* **Found by @surveyor reviewing `#1290` and reported as PRE-EXISTING and symmetric rather than as a defect in the PR she was approving. Implemented by @quartermaster, reviewed by @surveyor (official, exact-bound at `a3563907`).**