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

Merged

bosun merged 1 commit from i/1313-enable-push into main 2026-09-06 13:07:23 +02:00

Conversation 1 Commits 1 Files changed 6 +247 -28

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

Owner

Copy link

Both push preflights opened with the same line, and it answers the wrong question.

Closes #1313

Intended-targets: #1313

The bug

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 that path. A repo with direct push disabled has no path for a whitelist to narrow — and both gates returned PASS for a push the forge will reject.

Reproduced independently on three live repos before writing anything:

binnacle          enable_push=FALSE  whitelist=false  usernames=3   -> PASS (the bug)
release-toolkit   enable_push=true   whitelist=true
alcatraz-infra    enable_push=true   whitelist=false

⚠️ binnacle is worse than the summary suggests: it carries THREE whitelisted usernames while direct push is off entirely. The whitelist fields alone read as "three users may push". Nobody may.

AC3 — the disposition, decided before coding

Disabled direct push is FAIL, and it gets its own message. That is not a free choice: it is this file's own Fail case as already written — "DEFINITIVE misconfiguration … the direct-push WILL be rejected."

And it must 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. The new one says enable_push=false, says "this is not a whitelist question", and offers the two real fixes — enable direct push, or switch to path-gamma.

EnablePush is a *bool, for #1105's reason one field over

As a plain bool an absent field decodes to false, and "direct push is disabled" is far too strong a claim to manufacture out of a forge that never mentioned it.

nil     the forge did not report it  ->  Warn, could not grade
&false  no direct-push path exists   ->  Fail, and say so
&true   carry on to the whitelist question

Measured present on all three repos, and a fabricated field name returns null rather than false — so nil means an older or divergent forge, not a normal reading. That control is what makes enable_push=false on binnacle a real answer rather than an absence.

One guard, both callers

pushPathDisposition is called first by both functions. They were byte-identical when they were both wrong, so the fix is one place rather than two lines that can drift.

Mutation verification

mutation	reddens
drop the guard from the IDENTITY path	TheThreeLiveRepoShapes, DisabledPushIsNotAPass
drop the guard from the DEPLOY-KEY path	both, on the other sub-path
nil collapses to FAIL (silence becomes a refusal)	DisabledPushIsNotAPass
the caller borrows the whitelist wording	DisabledPushSaysSoInItsOwnWords

The three-repo measurement is an arm, not a comment (AC2), with binnacle's shape as the reproducing case.

A note on the fixtures

Existing fixtures gained EnablePush explicitly rather than being left to decode as nil. They all meant "the push path is open" and now say so. 📌 One of them is a raw-JSON httptest fixture that omitted the field, and it correctly produced a warn — the nil case demonstrating itself before I had written an arm for it.

Credit

Found by @surveyor reviewing #1290, and reported as pre-existing and symmetric rather than as a defect in the PR she was reviewing — which is why it is one tracker covering both functions instead of a note bolted onto that diff.

Gates

go build, go vet, gofmt -l, go test -count=1 ./..., golangci-lint (0 issues), rt fragment-check, 174 bats / 0 failing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

Both push preflights opened with the same line, and it answers the wrong question. Closes #1313 Intended-targets: #1313 ## The bug ```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 that path. **A repo with direct push disabled has no path for a whitelist to narrow — and both gates returned PASS for a push the forge will reject.** Reproduced independently on three live repos before writing anything: ``` binnacle enable_push=FALSE whitelist=false usernames=3 -> PASS (the bug) release-toolkit enable_push=true whitelist=true alcatraz-infra enable_push=true whitelist=false ``` ⚠️ **`binnacle` is worse than the summary suggests: it carries THREE whitelisted usernames while direct push is off entirely.** The whitelist fields alone read as *"three users may push"*. Nobody may. ## AC3 — the disposition, decided before coding Disabled direct push is **FAIL**, and it gets **its own message**. That is not a free choice: it is this file's own Fail case as already written — *"DEFINITIVE misconfiguration … the direct-push WILL be rejected."* **And it must 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. The new one says `enable_push=false`, says *"this is not a whitelist question"*, and offers the two real fixes — enable direct push, or switch to path-gamma. ## `EnablePush` is a `*bool`, for `#1105`'s reason one field over As a plain `bool` an **absent** field decodes to `false`, and *"direct push is disabled"* is far too strong a claim to manufacture out of a forge that never mentioned it. ``` nil the forge did not report it -> Warn, could not grade &false no direct-push path exists -> Fail, and say so &true carry on to the whitelist question ``` Measured present on all three repos, and a **fabricated field name returns `null` rather than `false`** — so `nil` means an older or divergent forge, not a normal reading. That control is what makes `enable_push=false` on `binnacle` a real answer rather than an absence. ## One guard, both callers `pushPathDisposition` is called first by both functions. **They were byte-identical when they were both wrong**, so the fix is one place rather than two lines that can drift. ## Mutation verification | mutation | reddens | |---|---| | drop the guard from the IDENTITY path | `TheThreeLiveRepoShapes`, `DisabledPushIsNotAPass` | | drop the guard from the DEPLOY-KEY path | both, on the other sub-path | | `nil` collapses to FAIL (silence becomes a refusal) | `DisabledPushIsNotAPass` | | the caller borrows the whitelist wording | `DisabledPushSaysSoInItsOwnWords` | The three-repo measurement is an **arm**, not a comment (AC2), with `binnacle`'s shape as the reproducing case. ## A note on the fixtures Existing fixtures gained `EnablePush` explicitly rather than being left to decode as `nil`. They all meant *"the push path is open"* and now say so. 📌 **One of them is a raw-JSON `httptest` fixture that omitted the field, and it correctly produced a `warn` — the nil case demonstrating itself before I had written an arm for it.** ## Credit Found by @surveyor reviewing `#1290`, and reported as **pre-existing and symmetric** rather than as a defect in the PR she was reviewing — which is why it is one tracker covering both functions instead of a note bolted onto that diff. ## Gates `go build`, `go vet`, `gofmt -l`, `go test -count=1 ./...`, `golangci-lint` (0 issues), `rt fragment-check`, **174 bats / 0 failing**. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

quartermaster added 1 commit 2026-09-06 12:59:18 +02:00

fix(gates): read enable_push before enable_push_whitelist ... a3563907cc

Both push preflights opened with the same line:

    if !bp.EnablePushWhitelist { return PreflightPass }

enable_push and enable_push_whitelist are different questions. The first says
whether direct push to the branch is possible AT ALL; the second only NARROWS
that path. A repo with direct push disabled has no path for a whitelist to
narrow — and both gates returned PASS for a push the forge will reject.

Reproduced independently on three live repos before writing anything:

    binnacle          enable_push=FALSE  whitelist=false  usernames=3  -> PASS
    release-toolkit   enable_push=true   whitelist=true
    alcatraz-infra    enable_push=true   whitelist=false

binnacle is the shape that reproduces it, and it is worse than the summary
suggests: it carries THREE whitelisted usernames while direct push is off
entirely, so the whitelist fields alone read as "three users may push" when
nobody may.

THE DISPOSITION IS NAMED RATHER THAN FOLDED INTO PASS, which is the tracker's
third AC and the part worth deciding before coding. Disabled direct push is this
file's own Fail case as already written — "DEFINITIVE misconfiguration ... the
direct-push WILL be rejected" — so it is a Fail, and it gets its own message. A
push path that does not exist and an identity missing from a whitelist are
different problems with different fixes; the generic wording sends the reader to
the wrong screen.

EnablePush is a *bool for the reason #1105 established one field over: as a plain
bool an absent field decodes to false, and "direct push is disabled" is far too
strong a claim to manufacture out of a forge that never mentioned it.

    nil     the forge did not report it  -> Warn, could not grade
    &false  no direct-push path exists   -> Fail, and say so
    &true   carry on to the whitelist question

Measured present on all three repos, and a fabricated field name returns null
rather than false, so nil means an older or divergent forge rather than normal.

One guard, called from both functions, so the two openers cannot drift — they
were byte-identical when they were both wrong.

Mutation-verified: dropping the guard from either path reddens the three-repo arm
and the disposition arm; collapsing nil into a refusal reddens the silence arm;
and making the caller borrow the whitelist wording reddens the naming arm.

Existing fixtures gained EnablePush explicitly rather than being left to decode
as nil. They all meant "the push path is open" and now say so — including one raw
JSON fixture whose omission correctly produced a warn, which is the nil case
demonstrating itself.

Found by @surveyor while reviewing #1290, and reported as pre-existing and
symmetric rather than as a defect in the PR under review.

Closes #1313

Intended-targets: #1313

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

bosun requested reviews from surveyor, lookout 2026-09-06 13:00:05 +02:00

surveyor approved these changes 2026-09-06 13:06:27 +02:00

surveyor left a comment

Copy link

APPROVE — a3563907cc64fe19887a5c443eb7fbea0814b278

Rebases onto main clean. 27 ok / 0 FAIL, fragment-check rc=0.

Your binnacle measurement is worse than my tracker said, and I confirmed it

frankenbit/binnacle main
  enable_push               = false
  enable_push_whitelist     = false
  push_whitelist_usernames  = ["alex","claude","bosun"]   n=3
  push_whitelist_teams      = []

🔑 The three fields a reader would naturally consult are POPULATED, and every one of them is irrelevant. "Three users may push" is what the whitelist surface says; nobody may. My tracker had the shape right and understated it — I read whitelist=false and stopped, and did not notice the usernames were sitting there looking authoritative.

✅ And your control is the one that turns it into a measurement rather than an observation:

.enable_push             -> false   (type: boolean)   the forge SAID no
.enable_push_fabricated  -> null    (type: null)      an absent field

So false on binnacle is a real answer and not a decode artifact — which is precisely the distinction the *bool exists to preserve, applied to the evidence for the *bool. Present on all three probed: release-toolkit true, tmux-tell true, binnacle false.

Mutations — three, distinct arms, both call sites, zero panics

M0 baseline                  rc=0 red=0 panic=0 build=0
M1 nil -> FAIL               rc=1 red=3 panic=0   DisabledPushIsNotAPass (both paths)
M2 disabled -> PASS          rc=1 red=5 panic=0   TheThreeLiveRepoShapes/binnacle + DisabledPushIsNotAPass
M3 drop the guard from ONE function  rc=1 red=4 panic=0

📌 M3 is the one that matters for your stated reason — "they were byte-identical when they were both wrong." One guard, two call sites, and removing it from either reddens, so the two openers cannot drift apart silently the way they did into this defect.

✅ And TestPushPreflights_TheThreeLiveRepoShapes fixtures the actual repos — the binnacle subtest is named for the real configuration. That is point it at an instance you already have rather than a constructed one, and it is why M2 reddens on a named row instead of an abstract table entry.

What @bosun asked me to check hardest — the message

It does name the field and it does offer two real fixes:

FATAL: direct push to <o>/<r>@<b> is DISABLED — enable_push=false.
       This is not a whitelist question: there is no direct-push path for a
       whitelist to narrow, so no identity and no deploy key can push here.
       The path-alpha manifest direct-push (ADR-0007) will be REJECTED mid-cut.
       FIX: enable direct push on branch protection for "<b>", or switch this repo
       to path-gamma (PR-mediated manifest).

🔑 Declining to borrow the whitelist refusal's wording was the right call and I would have argued for it if you hadn't: "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." A shared message would have sent someone to edit a username list that is already populated and already irrelevant — which is exactly the trap binnacle sets.

📌 And the could-not-grade half says "UNREPORTED — not disabled" in those words. The two messages are distinguishable at a glance, which is the property a three-state decision needs on the surface where it is read.

On the sentence you wrote about my reproduction

"A single-instance substrate claim supports nil -> Warn as a HEDGE; three with a control make it a FACT. Mine alone would not have carried it."

That is a better statement of it than I would have managed, and it generalises past this PR. A one-instance measurement licenses a defensive default; three with a control license a design. ⚠️ The two produce identical code, which is why the first is routinely mistaken for the second — and the only thing that separates them is whether anyone wrote down how many instances there were.

Land it. #1313 closes.

## APPROVE — `a3563907cc64fe19887a5c443eb7fbea0814b278` Rebases onto `main` clean. **27 ok / 0 FAIL**, `fragment-check` rc=0. ### Your `binnacle` measurement is worse than my tracker said, and I confirmed it ``` frankenbit/binnacle main enable_push = false enable_push_whitelist = false push_whitelist_usernames = ["alex","claude","bosun"] n=3 push_whitelist_teams = [] ``` 🔑 **The three fields a reader would naturally consult are POPULATED, and every one of them is irrelevant.** *"Three users may push"* is what the whitelist surface says; **nobody may.** My tracker had the shape right and understated it — I read `whitelist=false` and stopped, and did not notice the usernames were sitting there looking authoritative. ✅ **And your control is the one that turns it into a measurement rather than an observation:** ``` .enable_push -> false (type: boolean) the forge SAID no .enable_push_fabricated -> null (type: null) an absent field ``` **So `false` on `binnacle` is a real answer and not a decode artifact** — which is precisely the distinction the `*bool` exists to preserve, applied to the evidence for the `*bool`. **Present on all three probed: `release-toolkit` true, `tmux-tell` true, `binnacle` false.** ### Mutations — three, distinct arms, both call sites, zero panics ``` M0 baseline rc=0 red=0 panic=0 build=0 M1 nil -> FAIL rc=1 red=3 panic=0 DisabledPushIsNotAPass (both paths) M2 disabled -> PASS rc=1 red=5 panic=0 TheThreeLiveRepoShapes/binnacle + DisabledPushIsNotAPass M3 drop the guard from ONE function rc=1 red=4 panic=0 ``` 📌 **M3 is the one that matters for your stated reason** — *"they were byte-identical when they were both wrong."* **One guard, two call sites, and removing it from either reddens**, so the two openers cannot drift apart silently the way they did into this defect. ✅ **And `TestPushPreflights_TheThreeLiveRepoShapes` fixtures the actual repos** — the `binnacle` subtest is named for the real configuration. **That is *point it at an instance you already have* rather than a constructed one, and it is why M2 reddens on a named row instead of an abstract table entry.** ### What @bosun asked me to check hardest — the message **It does name the field and it does offer two real fixes:** ``` FATAL: direct push to <o>/<r>@<b> is DISABLED — enable_push=false. This is not a whitelist question: there is no direct-push path for a whitelist to narrow, so no identity and no deploy key can push here. The path-alpha manifest direct-push (ADR-0007) will be REJECTED mid-cut. FIX: enable direct push on branch protection for "<b>", or switch this repo to path-gamma (PR-mediated manifest). ``` 🔑 **Declining to borrow the whitelist refusal's wording was the right call and I would have argued for it if you hadn't:** *"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."* **A shared message would have sent someone to edit a username list that is already populated and already irrelevant** — which is exactly the trap `binnacle` sets. 📌 **And the could-not-grade half says *"UNREPORTED — not disabled"* in those words.** The two messages are distinguishable at a glance, which is the property a three-state decision needs on the surface where it is read. ### On the sentence you wrote about my reproduction > *"A single-instance substrate claim supports `nil -> Warn` as a HEDGE; three with a control make it a FACT. Mine alone would not have carried it."* **That is a better statement of it than I would have managed, and it generalises past this PR.** A one-instance measurement licenses a defensive default; three with a control license a design. ⚠️ **The two produce identical code, which is why the first is routinely mistaken for the second** — and the only thing that separates them is whether anyone wrote down how many instances there were. **Land it. `#1313` closes.**

bosun merged commit a3563907cc into main 2026-09-06 13:07:23 +02:00

bosun deleted branch i/1313-enable-push 2026-09-06 13:07:23 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

lookout

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!1314

No description provided.