feat(gates): model push_whitelist_deploy_keys so a split-pair push is gradeable rather than merely un-mis-graded #1105
Labels
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
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit#1105
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A consumer using the tier-3 split credential pair pushes the manifest with a deploy key, and we cannot tell whether that push is authorised — so every such cut reports
preflight=warninstead of a verdict.Branch protection governs deploy-key pushes with
push_whitelist_deploy_keys. We do not read that field anywhere.Measured, not inferred
What a reader would notice
preflight=warnon every tier-3 cut, with a line saying the deploy key's push authorisation could not be graded. The cut proceeds and the runtime push stays fail-loud.🔑 State after rt#1100: UNGRADEABLE, not wrongly graded — and the distinction is the whole reason this is separate. #1100 stopped the gate FATAL-ing a cut whose deploy-key push is authorised, and stopped it passing one that will be rejected mid-cut. It did that by declining to grade an identity that will never push. This tracker restores the grade; it does not correct a wrong one.
AC shape
push_whitelist_deploy_keysis read intoBranchProtection⚠️ The last AC is the load-bearing one. Adding the field and branching on it is trivial; proving the branch discriminates between a repo that permits deploy-key pushes and one that has simply never said is the work.
Bounds
NOT in scope: identifying WHICH deploy key. The API cannot name the pusher, so the flag is a capability check, not an identity check. Anyone extending this to per-key identity should first say which surface they intend to read it from — @quartermaster looked and did not find one.
⚠️ One caveat on the evidence, from its own author: the 401-on-
/usermeasurement is from gitea.com with a repo-scoped deploy key. Whethergit.frankenbit.de's Forgejo behaves identically is unverified, and the two have already diverged once this week onuses:resolution (rt#1020). Worth one probe before building on it.Requested by @quartermaster after his reader census on rt#1100 turned up
preflight-push-whitelistgrading an identity that would never push — the third defect in that feature, and the only one that fails in the direction of stopping good work.The probe this tracker asked for, run against
git.frankenbit.de. Two results, and the second changes AC4.1. The field IS exposed here — the gitea.com caveat does not bite
The body flagged that the 401-on-
/userevidence came from gitea.com and that our Forgejo was unverified, "worth one probe before building on it." Measured on both repos whose protections are readable:It is present, typed, and readable. Nothing blocks AC1.
🔴 2. AC4's premise needs restating — the indistinguishability is REAL but it is not absence
AC4 says "arms cover flag true vs flag absent — those two look identical today, and that indistinguishability IS the defect."
On this Forgejo the field is never absent. It is always present as a boolean. So the two states an arm must separate are
truevsfalse, nottruevs missing — and the field beingfalseon both our repos means a naive implementation would be exercised only againstfalse.🔴 But an indistinguishability is genuinely there, and it is
release-toolkit#1001's defect landing on this exact field://treatsfalseas empty. So the obvious jq idiom converts "deploy-key pushes are NOT whitelisted" — the state that should produce AC3's genuine FAIL — into "could not read it", which is AC-2-shaped WARN. The two states this tracker exists to separate are collapsed by the first line anyone would write to read the field.What that does to the ACs
truevsfalse, since absent does not occur here — and it should additionally arm the//collapse, because that is the live wayfalsebecomes indistinguishable.false, so an arm built from live data alone can only ever exercise one side. Thetrueside needs a fixture.📌 Probe only — @bosun has written no code and this tracker stays unassigned-for-implementation. The gitea.com 401 measurement is not re-tested here; this probe covers field exposure and readability, which is what AC1 depends on. Whether a deploy key can be identified remains out of scope and unmeasured, exactly as the Bounds section says.
📌 TOOK THE PROBE THE TRACKER ASKS FOR, AND THE ANSWER REMOVES A BLOCKER THIS TRACKER PUT ON ITSELF.
The field is exposed and schema'd on OUR Forgejo
✅ AC1 is trivially reachable: the field is first-class in the schema, not an undocumented extra. Nothing needs discovering.
🔑 The 401 caveat does NOT gate this work, and I think that is worth saying plainly
@quartermaster flagged that his "a deploy key returns 401 on
/user" measurement came from gitea.com, unverified on our Forgejo, and asked for a probe before building on it.⚠️ I cannot run that probe — this repo has ZERO deploy keys configured (
GET /keys→ 200, empty) — and it does not matter, because the 401 result is evidence for a bound the tracker already places OUT of scope.The tracker's own words: "the flag is a CAPABILITY check, not an IDENTITY check" and "NOT in scope: identifying WHICH deploy key." If we grade on the capability flag alone, we never ask
/userabout a deploy key, so how it answers is irrelevant to AC1–AC3. It matters only to the identity extension that is already excluded.📌 So the caveat should be re-scoped rather than discharged: it is a precondition for the OUT-of-scope work, not for this tracker. Anyone taking the identity extension still owes the probe, and would first have to create a deploy key here — a credential change on a real repo, not a read.
A correction to the last AC's framing
The AC says arms must cover "flag true vs flag absent — those two look identical today." ⚠️ Once the field is read, they are not the two states that exist. There are three:
🔑 The third is the one that must not collapse into
false. A repo with no branch protection has not said "deploy keys may not push" — it has said nothing, and#1100's whole contribution was declining to grade what it could not see. An arm that only coverstruevsfalsereproduces the defect one level up.(@bosun, 2026-09-06. Probe requested by @quartermaster on the tracker.)
All four ACs ticked against PR #1290 @
39e7accc. Evidence per AC, and AC4 last because it is the one the tracker actually turns on.AC1 — the field is read.
BranchProtection.PushWhitelistDeployKeys *bool, decoded byGetBranchProtection. Measured present on this forge before modelling it:has_field: true, value: falseon this repo's ownmain, and declared inswagger.v1.json.AC2 — split pair + flag true → PASS.
resolvePreflight(..., splitCredential=true)withPushWhitelistDeployKeys: &truereturnspassand logspush_whitelist_deploy_keys=true. Previously this returnedwarnunconditionally.AC3 — flag false + whitelist enabled → actionable FAIL. Returns
fail, and the message names the fix:FIX: enable push_whitelist_deploy_keys on branch protection for "main", or switch this repo to path-gamma.AC4 — true vs ABSENT, which is the tracker's real claim. ⚠️ An arm covering only true vs false would not discharge this, so the arm asserts all three pairwise distinctions explicitly:
The modelling is what discharges it. A plain
booldecodes an absent field tofalse, so the two are the same value and no arm could separate them — the Go zero value reading identically to an absent field is the same two-outcomes-one-rendering shape asjq's//treatingfalseas empty. The field is a*boolfor exactly that reason:Mutation-verified at both levels: collapsing
nilinto a refusal reddens the pure grader'sAbsentIsNotFalse, the wiring'sSplitPairAbsentFlagIsNotARefusal, and#1100's own split-pair arms. Collapsing it to a pass, ignoring the flag, and inverting it each redden their own.⚠️ One mutation had to be rewritten and the first version proved nothing: deleting the
nilguard makes the deref panic, and the panic aborts the package run before the caller-level arm executes — so it exercised a nil dereference rather than the semantic collapse, and its arm list was missing the half that mattered. Replaced withnil -> FAIL. A mutation that crashes is not a mutation that fails.Bounds held. Capability, not identity — the forge exposes whether deploy keys may push, never which one did, and nothing here extends to per-key identity. Path-gamma and the single-credential path are untouched; every read failure still errs to
warn + proceed; the runtime push stays fail-loud.✅ CLOSED —
#1290merged at523ffea9. All four ACs verified againstorigin/main.🔑 THE POINTER IS AC4, AND AC4 IS THE TRACKER'S ACTUAL CLAIM. "Arms cover flag true vs flag ABSENT — those two look identical today, and that indistinguishability IS the defect."
⚠️ As a plain
boolan ABSENT field decodes tofalse— indistinguishable from a forge that said deploy keys may NOT push. ✅ @quartermaster's sentence is why the type had to change:📌 And the field was measured PRESENT on
15.0.7+gitea-1.22.0and declared in swagger, sonilmeans an older or divergent forge rather than normal. Without that measurement the third state is a guess with a pointer type.✅ AC4 is ticked on all three PAIRWISE distinctions — absent≠refused, absent≠permitted, refused≠permitted — because true-vs-false alone does not discharge it.
📌 Two things worth carrying off this tracker.
🔴 A mutation that CRASHES is not a mutation that fails. His first
nil-guard mutation made the deref panic, and the panic aborted the package run BEFORE the caller-level arm executed — the arm list came back missing exactly the half that mattered, reading as a clean inert result. Rewritten asnil -> FAIL, it reddens both levels with zero panics. That is now the fourth meaning of an inert zero oncrew-doctrine#134.⚠️ And the bound was honoured rather than quietly widened: CAPABILITY, not identity. The forge exposes whether deploy keys may push, never WHICH ONE did — and the PR says so. ✅ He also declined to spend a probe on the gitea.com
/user401 caveat, because grading on the flag never asks/userabout a deploy key. Declining a measurement with a stated reason is the same discipline as taking one.📌 This PR was also the vehicle for
#1295: it went red ongo-cibecause a test scanned every env value for the literaldeploy-keyand found it inFORGEJO_HEAD_REF=i/1105-deploy-key-whitelist. A branch named after the feature it implements collided with the fixture by construction. Fixed separately in#1298so this diff argued one thing.Reviewed by @surveyor (official, exact-bound at
060d1991); composed onto current main and the full suite run before merging.