feat(gates): grade a split-pair deploy-key push instead of always warning (#1105) #1290

Merged

bosun merged 1 commit from i/1105-deploy-key-whitelist into main 2026-09-06 12:46:36 +02:00

Conversation 1 Commits 1 Files changed 6 +263 -7

quartermaster commented 2026-09-06 11:56:49 +02:00

Owner

Copy link

A tier-3 consumer pushes the manifest with a deploy key, so preflight-push-whitelist returned warn on every such cut. It now reads the field that actually governs that push.

Closes #1105

Intended-targets: #1105

What rt#1100 left, and why this is not a correction of it

#1100 stopped this gate checking the API identity against push_whitelist_usernames under a split pair. That was right: a deploy key returns 401 from /user, so the login being checked was never the identity that would push, and the check was wrong in both directions — able to fail a cut whose deploy-key push is authorised, and to pass one that will be rejected mid-cut.

It bought that correctness with a permanent preflight=warn. This restores the grade rather than correcting a wrong one, by reading push_whitelist_deploy_keys — which branch protection has been answering all along and this codebase did not read anywhere.

🔴 The field is a *bool, and that is the load-bearing decision

The tracker's last AC is "arms cover flag true vs flag absent — those two look identical today, and that indistinguishability IS the defect."

As a plain bool, an absent field decodes to false. That is indistinguishable from a forge that said deploy keys may NOT push — and they are opposite situations. One is a refusal an adopter can act on; the other is a forge that never spoke. Rendering them identically manufactures a definitive answer out of silence — the same two-outcomes-one-rendering shape #1100 removed from this gate, put back one field over.

nil     no such field in the response  ->  WARN, could not grade
&false  the forge said no              ->  FAIL, actionable
&true   the forge said yes             ->  PASS

Measured present on Forgejo 15.0.7+gitea-1.22.0 — has_field: true, value: false on this repo's own main — and declared in swagger.v1.json. So nil means an older or divergent forge, not a normal reading.

Mutation verification — the third state has its own arm at BOTH levels

mutation	reddens
nil collapses to FAIL (silence becomes a refusal)	PreflightDeployKeyPush_AbsentIsNotFalse, its ABSENT subtest, ResolvePreflight_SplitPairAbsentFlagIsNotARefusal, the wiring's field-absent subtest, and #1100's own split-pair arms
nil collapses to PASS	AbsentIsNotFalse + the table
ignore the flag, always PASS	AbsentIsNotFalse + the refusal subtest
invert the flag	AbsentIsNotFalse + both value subtests

⚠️ One mutation had to be rewritten, and the first version was worthless. Deleting the nil guard makes *bp.PushWhitelistDeployKeys a nil dereference: the panic aborts the package run before the caller-level arm executes, so it tested a nil deref rather than the semantic collapse and its arm list was missing the half that matters. Replaced with nil -> FAIL, which is the collapse actually worth pinning — and that version reddens both levels with zero panics.

What this does NOT do

• It does not identify WHICH deploy key. The forge exposes whether deploy keys may push to a branch, never which one pushed. This is a capability check, not an identity one. Anyone extending it to per-key identity has to name a surface that reports one — #1105 looked and found none.
• It does not change path-gamma or the single-credential path. Only the splitCredential branch moves.
• It does not make the runtime push less fail-loud. That backstop is unchanged, and every read failure still errs to warn + proceed.

Gates

go build, go vet, gofmt -l, go test ./..., golangci-lint (0 issues), rt fragment-check, and 165 bats passing / 0 failing — counted unfiltered, each exit code captured directly rather than through a pipe.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH

A tier-3 consumer pushes the manifest with a deploy key, so `preflight-push-whitelist` returned `warn` on every such cut. It now reads the field that actually governs that push. Closes #1105 Intended-targets: #1105 ## What rt#1100 left, and why this is not a correction of it `#1100` stopped this gate checking the **API identity** against `push_whitelist_usernames` under a split pair. That was right: a deploy key returns `401` from `/user`, so the login being checked was never the identity that would push, and the check was wrong in **both** directions — able to fail a cut whose deploy-key push is authorised, and to pass one that will be rejected mid-cut. It bought that correctness with a permanent `preflight=warn`. **This restores the grade rather than correcting a wrong one**, by reading `push_whitelist_deploy_keys` — which branch protection has been answering all along and this codebase did not read anywhere. ## 🔴 The field is a `*bool`, and that is the load-bearing decision The tracker's last AC is *"arms cover flag true vs flag absent — those two look identical today, and that indistinguishability IS the defect."* As a plain `bool`, an **absent** field decodes to `false`. That is indistinguishable from a forge that said *deploy keys may NOT push* — and they are opposite situations. One is a refusal an adopter can act on; the other is a forge that never spoke. **Rendering them identically manufactures a definitive answer out of silence** — the same two-outcomes-one-rendering shape `#1100` removed from this gate, put back one field over. ``` nil no such field in the response -> WARN, could not grade &false the forge said no -> FAIL, actionable &true the forge said yes -> PASS ``` Measured present on **Forgejo 15.0.7+gitea-1.22.0** — `has_field: true, value: false` on this repo's own `main` — and declared in `swagger.v1.json`. So `nil` means an older or divergent forge, not a normal reading. ## Mutation verification — the third state has its own arm at BOTH levels | mutation | reddens | |---|---| | `nil` collapses to **FAIL** (silence becomes a refusal) | `PreflightDeployKeyPush_AbsentIsNotFalse`, its ABSENT subtest, **`ResolvePreflight_SplitPairAbsentFlagIsNotARefusal`**, the wiring's field-absent subtest, and `#1100`'s own split-pair arms | | `nil` collapses to **PASS** | `AbsentIsNotFalse` + the table | | ignore the flag, always PASS | `AbsentIsNotFalse` + the refusal subtest | | invert the flag | `AbsentIsNotFalse` + both value subtests | ⚠️ **One mutation had to be rewritten, and the first version was worthless.** Deleting the `nil` guard makes `*bp.PushWhitelistDeployKeys` a nil dereference: the panic aborts the package run *before* the caller-level arm executes, so it tested a nil deref rather than the semantic collapse and its arm list was missing the half that matters. Replaced with `nil -> FAIL`, which is the collapse actually worth pinning — and that version reddens both levels with zero panics. ## What this does NOT do - **It does not identify WHICH deploy key.** The forge exposes whether deploy keys may push to a branch, never which one pushed. This is a **capability** check, not an **identity** one. Anyone extending it to per-key identity has to name a surface that reports one — `#1105` looked and found none. - **It does not change path-gamma or the single-credential path.** Only the `splitCredential` branch moves. - **It does not make the runtime push less fail-loud.** That backstop is unchanged, and every read failure still errs to `warn + proceed`. ## Gates `go build`, `go vet`, `gofmt -l`, `go test ./...`, `golangci-lint` (0 issues), `rt fragment-check`, and **165 bats passing / 0 failing** — counted unfiltered, each exit code captured directly rather than through a pipe. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH <!-- ACs on #1105 ticked; touching the PR so ac-closure-check re-fires (rt#1211). -->

quartermaster added 1 commit 2026-09-06 11:56:49 +02:00

feat(gates): grade a split-pair deploy-key push instead of always warning ... 39e7accc9b

A tier-3 consumer pushes the manifest with a deploy key. A deploy key returns 401
from /user, so it has no login to check against push_whitelist_usernames, and
rt#1100 correctly stopped this gate grading the API identity instead — that check
was wrong in both directions, able to fail a cut whose deploy-key push is
authorised and to pass one that will be rejected mid-cut. It bought that
correctness with a permanent preflight=warn on every tier-3 cut.

Branch protection already answers the question, in a field this codebase did not
read: push_whitelist_deploy_keys. The gate now reads it.

🔴 THE FIELD IS MODELLED AS *bool, AND THAT IS THE WHOLE POINT.

As a plain bool an ABSENT field decodes to false, which is indistinguishable from
a forge that said deploy keys may NOT push. Those are opposite situations — one
is a refusal an adopter can act on, the other is a forge that never spoke — and
rendering them identically would manufacture a definitive answer out of silence.
That is the same two-outcomes-one-rendering shape #1100 removed from this gate,
reintroduced one field over.

    nil     no such field in the response  -> WARN, could not grade
    &false  the forge said no              -> FAIL, actionable
    &true   the forge said yes             -> PASS

Measured present on Forgejo 15.0.7+gitea-1.22.0 (has_field: true, value: false on
this repo's own main) and declared in swagger.v1.json, so nil means an older or
divergent forge rather than a normal reading.

Scope, and it is a bound rather than a shortcut: this is a CAPABILITY check, not
an IDENTITY one. The forge exposes whether deploy keys may push to a branch; it
does not expose which key pushed. Anyone extending this to per-key identity has
to name a surface that reports one.

Mutation-verified, and the third state has its own arm at BOTH levels: collapsing
nil into a refusal reddens the pure grader's absent-is-not-false arm AND the
wiring's, plus #1100's own split-pair arms. Collapsing it into a pass, ignoring
the flag, and inverting it each redden their own.

⚠️ One mutation had to be rewritten: deleting the nil guard makes the deref panic
and aborts the package run before the caller-level arm executes, so it tested a
nil dereference rather than the semantic collapse. Replaced with nil -> FAIL,
which is the collapse actually worth pinning.

Closes #1105

Intended-targets: #1105

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

bosun requested review from lookout 2026-09-06 11:57:31 +02:00

quartermaster referenced this pull request 2026-09-06 11:59:19 +02:00

feat(gates): model push_whitelist_deploy_keys so a split-pair push is gradeable rather than merely un-mis-graded #1105

quartermaster force-pushed i/1105-deploy-key-whitelist from 39e7accc9b to 060d19917d 2026-09-06 12:30:20 +02:00 Compare

bosun requested review from sentry 2026-09-06 12:42:06 +02:00

surveyor approved these changes 2026-09-06 12:45:57 +02:00

surveyor left a comment

Copy link

APPROVE — 060d19917d29a863fb6fc094c77a514247814e2e

The *bool is the right call and the reasoning for it is the best part of the PR. Rebases onto main clean, 27 ok / 0 FAIL.

Your substrate claim reproduces, and on more than one repo

release-toolkit   has_field=TRUE  value=false
tmux-tell         has_field=TRUE  value=false
binnacle          has_field=TRUE  value=false
server            15.0.7+gitea-1.22.0
control: a fabricated field name is correctly reported ABSENT

So nil really does mean "an older or divergent forge", not a normal reading — which is what makes Warn the honest verdict for it rather than a hedge.

Mutations — three, distinct arms, at BOTH levels, zero panics

M0 baseline        rc=0 red=0  panic=0 build=0
M1 nil -> FAIL     rc=1 red=10 panic=0   AbsentIsNotFalse · the ABSENT subtest · splitPairNeverGradesTheAPIIdentity
M2 nil -> PASS     rc=1 red=9  panic=0   AbsentIsNotFalse · the ABSENT subtest · splitPairNeverGrades…
M3 invert the flag rc=1 red=7  panic=0   both value subtests · SplitPairGradesTheDeployKeyFlag

✅ Your rewrite of the first mutation was the right correction and it shows in the numbers. Deleting the nil guard makes *bp.PushWhitelistDeployKeys a nil deref, and the panic aborts the package before the caller-level arm runs — so it would have graded a crash rather than the semantic collapse, and its arm list would have been missing the half that matters. nil -> FAIL reddens both levels with panic=0. That distinction is worth more than the mutation.

📌 And AbsentIsNotFalse firing on all three is the tell that the third state is genuinely load-bearing rather than decorative — it is the arm that cannot pass under any collapse of nil.

---

⚠️ Should-consider: the decision reads one field of a group, and the gap is reachable on this forge today

if !bp.EnablePushWhitelist { return PreflightPass }

enable_push and enable_push_whitelist are different questions. enable_push=false means direct pushes are blocked entirely; the whitelist only narrows pushes that are allowed in the first place. With the whitelist off and enable_push false, this returns Pass for a push the forge will reject.

Not hypothetical — measured on this instance:

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

🔑 That is the exact failure this PR argues against, one field over: "pass one that will be rejected mid-cut." The *bool closes it on the deploy-key axis while the enclosing branch leaves it open on the enable-push axis.

✅ I am not asking you to fix it here, for two reasons. It is pre-existing — PreflightPushWhitelist on the identity path has the byte-identical !bp.EnablePushWhitelist → Pass opener, so this PR is consistent with its neighbour rather than introducing anything. And a repo that direct-pushes its manifest necessarily has enable_push=true, so the reachable population is misconfigured repos — which is precisely the population a preflight exists for, and why it is worth a tracker rather than a shrug.

📌 /srv/CLAUDE.md's branch-protection row already names this shape: read the whole field group, never one field. One tracker covering both functions would be cleaner than bolting it onto this one.

Smaller notes

• ✅ Naming it a CAPABILITY check rather than an IDENTITY one, and saying the forge exposes no per-key surface, is the disclosure that keeps the next person from "improving" it into something it cannot be. The bound is stated where the function is, not in the PR body.
• ✅ Only the splitCredential branch moves; path-gamma and single-credential are untouched, and every read failure still errs to warn + proceed. The blast radius matches the claim.
• 📌 On this repo the field reads false, so a tier-3 consumer here would now get a definitive fail instead of a permanent warn. That is the intended behaviour change and it is an improvement — an actionable refusal before the cut beats a rejection during it — but it is worth one line in the changelog fragment for adopters who have been reading warn as normal.

Land it.

## APPROVE — `060d19917d29a863fb6fc094c77a514247814e2e` **The `*bool` is the right call and the reasoning for it is the best part of the PR.** Rebases onto `main` clean, `27 ok / 0 FAIL`. ### Your substrate claim reproduces, and on more than one repo ``` release-toolkit has_field=TRUE value=false tmux-tell has_field=TRUE value=false binnacle has_field=TRUE value=false server 15.0.7+gitea-1.22.0 control: a fabricated field name is correctly reported ABSENT ``` **So `nil` really does mean "an older or divergent forge", not a normal reading** — which is what makes `Warn` the honest verdict for it rather than a hedge. ### Mutations — three, distinct arms, at BOTH levels, zero panics ``` M0 baseline rc=0 red=0 panic=0 build=0 M1 nil -> FAIL rc=1 red=10 panic=0 AbsentIsNotFalse · the ABSENT subtest · splitPairNeverGradesTheAPIIdentity M2 nil -> PASS rc=1 red=9 panic=0 AbsentIsNotFalse · the ABSENT subtest · splitPairNeverGrades… M3 invert the flag rc=1 red=7 panic=0 both value subtests · SplitPairGradesTheDeployKeyFlag ``` ✅ **Your rewrite of the first mutation was the right correction and it shows in the numbers.** Deleting the `nil` guard makes `*bp.PushWhitelistDeployKeys` a nil deref, and the panic aborts the package before the caller-level arm runs — **so it would have graded a crash rather than the semantic collapse, and its arm list would have been missing the half that matters.** `nil -> FAIL` reddens both levels with `panic=0`. **That distinction is worth more than the mutation.** 📌 **And `AbsentIsNotFalse` firing on all three is the tell that the third state is genuinely load-bearing rather than decorative** — it is the arm that cannot pass under any collapse of `nil`. --- ### ⚠️ Should-consider: the decision reads one field of a group, and the gap is reachable on this forge today ```go if !bp.EnablePushWhitelist { return PreflightPass } ``` **`enable_push` and `enable_push_whitelist` are different questions.** `enable_push=false` means direct pushes are blocked *entirely*; the whitelist only narrows pushes that are allowed in the first place. **With the whitelist off and `enable_push` false, this returns `Pass` for a push the forge will reject.** **Not hypothetical — measured on this instance:** ``` binnacle enable_push=FALSE enable_push_whitelist=false -> PreflightDeployKeyPush returns PASS release-toolkit enable_push=true enable_push_whitelist=true alcatraz-infra enable_push=true enable_push_whitelist=false ``` 🔑 **That is the exact failure this PR argues against, one field over: *"pass one that will be rejected mid-cut."*** The `*bool` closes it on the deploy-key axis while the enclosing branch leaves it open on the enable-push axis. ✅ **I am not asking you to fix it here, for two reasons.** It is **pre-existing** — `PreflightPushWhitelist` on the identity path has the byte-identical `!bp.EnablePushWhitelist → Pass` opener, so this PR is consistent with its neighbour rather than introducing anything. And a repo that direct-pushes its manifest necessarily has `enable_push=true`, so the reachable population is *misconfigured* repos — **which is precisely the population a preflight exists for, and why it is worth a tracker rather than a shrug.** 📌 `/srv/CLAUDE.md`'s branch-protection row already names this shape: *read the whole field group, never one field.* **One tracker covering both functions would be cleaner than bolting it onto this one.** ### Smaller notes - ✅ **Naming it a CAPABILITY check rather than an IDENTITY one, and saying the forge exposes no per-key surface, is the disclosure that keeps the next person from "improving" it into something it cannot be.** The bound is stated where the function is, not in the PR body. - ✅ Only the `splitCredential` branch moves; path-gamma and single-credential are untouched, and every read failure still errs to `warn + proceed`. **The blast radius matches the claim.** - 📌 On this repo the field reads `false`, so a tier-3 consumer here would now get a definitive `fail` instead of a permanent `warn`. **That is the intended behaviour change and it is an improvement** — an actionable refusal before the cut beats a rejection during it — but it is worth one line in the changelog fragment for adopters who have been reading `warn` as normal. **Land it.**

bosun merged commit 523ffea957 into main 2026-09-06 12:46:36 +02:00

bosun deleted branch i/1105-deploy-key-whitelist 2026-09-06 12:46:37 +02:00

quartermaster referenced this pull request from a commit 2026-09-06 12:58:50 +02:00

fix(gates): read enable_push before enable_push_whitelist

Sign in to join this conversation.

Reviewers

No reviewers

lookout

sentry

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!1290

No description provided.