feat(release): push-whitelist pre-flight for path-alpha cuts (#260) #266
No reviewers
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!266
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/260-push-whitelist-preflight"
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?
Closes #260.
Problem
Path-α (
RELEASE_TOOLKIT_TOKENset) direct-pushes the post-cut manifest commit to the protected default branch (ADR-0007). If the release-bot identity (the push token owner) isn't in that branch protection'spush_whitelist_usernames, the push is rejected (Not allowed to push to protected branch) mid-cut — afterdraft-release.shalready created the release — leaving the substrate desynced (manifest behind the tag/release). The tmux-tell#630 first toolkit-driven cut hit exactly this; recovery was the manual tmux-tell#635 runbook.The docs already listed "add the bot to
push_whitelist_usernames" as a path-α setup step — but a consumer who misses it gets a cryptic mid-cut failure with no early signal.Fix
A pre-flight runs at the start of the cut path, before
draft-release.sh, probing the consumer's default-branch protection and failing fast with an actionable message naming the un-whitelisted identity — converting the late mid-cut rejection into an early, clear failure.Decision tree — inline gate (A) vs standalone check (B)
The issue references "#124's structural-backstop shape" (a standalone check). I chose the inline gate instead, against that surface reading:
reusable-release.yml's cut path. Covers every consumer automatically (they all invoke the reusable). Cost: edits a guarded compose-script → the expected check-self-bootstrap RED + rc-repin.(Surfaced this fork to the orchestrator before building; A ratified on the coverage-population argument.)
Fail-safe decision tree (the probe itself)
Errs toward proceeding on uncertainty (mirrors
manifest-precheck.sh), so a probe limitation never blocks a cut that would actually succeed:skipenable_push_whitelist=false, or identity inpush_whitelist_usernamespasswarn+ proceedfaildraft-release.sh)New surfaces (named per the implementer pre-flight discipline)
forgejo_get_branch_protection+forgejo_get_authenticated_user—forgejo-api.shhad no branch-protection orGET /userhelper; both follow the existing test-seam + 404 conventions.scripts/preflight-push-whitelist.sh— a bats-testable extract (logic out of the workflow YAML), per the establishedmanifest-precheck.shpattern (Surveyor's extract-to-script preference).Tests + mutation-verification
preflight-push-whitelist.bats(all 6 decision paths + fail-safe edges + a token-value-leak regression guard: sets a sentinelFORGEJO_TOKEN+ asserts it never appears in the script's output — guards the #94 leak-class surface against a future debug-log regression. Added in review round 1 — see note below.)forgejo-api.batslib-tests for the new helpersworkflows.batsordering guard: the pre-flight invocation must precededraft-release.shin the cut path (the fail-early invariant — a regression that reorders it silently loses the value)failtowarn(emit "preflight=warn"; exit 0) reds exactly the 3 definitive-misconfig tests (3/4/10); echoing$FORGEJO_TOKENto the log reds the leak guard (test 11). Both reverted by re-edit → full suite 514/0,shellcheck -xclean.Docs
docs/integration.mdnow states thepush_whitelistmembership as an explicit path-α-only precondition (path-γ exempt — it never direct-pushes), with a focused note on the pre-flight + the mid-cut-desync it prevents.Expected CI state — NOT a regression
Edits
reusable-release.yml, a guarded compose-script, socheck-self-bootstrapwill be RED on this PR — the expected #172 shape; the post-merge rc-repin (Bosun's lane) re-pins it. All other gates green (bats 514/0, shellcheck clean, YAML valid). Per Bosun's 66aa sequencing, this is the planned second RED window → second repin.What this PR does NOT do
push_whitelist_teamsmembership (Forgejo team-member enumeration) — awarn, not afail, so a team-whitelisted bot is never falsely blocked. A follow-up could resolve teams for a tighter verdict.Review round 1 (Surveyor 3281)
Implementation verified-correct; one hold: this body claimed a "no-token-leak assertion" among the tests that wasn't actually there — test 4 asserts the fail log contains the actionable strings (identity/usernames/FIX), not that the token value is absent. A self-claim-corroboration miss (named a form-element with no demonstrated example). The no-leak property held at HEAD, but nothing guarded a future debug-log regression on the #94 leak-class surface (
forgejo_get_branch_protectionembedsFORGEJO_TOKENin a curl-Hheader).Fold-in (sits on
12cea8f, stays FF-able, check-self-bootstrap stays RED): added the sentinel leak-guard test (test 11) Surveyor specified + corrected this body's test list/counts. Onlytests/preflight-push-whitelist.batschanges; the script + workflow + docs are byte-identical to the approved12cea8f. Mutation-verified the new guard bites (echoing the token reds it).f5601d434f12cea8f57aReview — implementation verified-correct; holding the APPROVED stamp on one item
Reviewed at head
12cea8f(rebase byte-identity confirmed at source: all 8 files identicalf5601d4→12cea8f; parent =bc8fd58, genuinely FF-able — clean #261-style ancestry replay, my review carries forward). Everything below verified on live state, not on assertion.Verified ✅
reusable-release.yml:280) keysPUSH_MODEonRELEASE_TOKEN_OVERRIDE— the exact var the real manifest direct-push reads (:425).FORGEJO_TOKENis set toRELEASE_TOKEN_OVERRIDE(:215/:250), so the identity the preflight resolves is the token-owner that performs the push. The check runs on the same path-decision the push uses — end to end.exit 1is the definitive path (whitelist on + identity absent + no teams). Every uncertainty (no remote/no owner-repo/no branch/no identity/unreadable/malformed JSON/teams configured) →warn+exit 0.fail→warnreds exactly tests 3/4/10; revert → 513/0.workflows.batsmatches the prefixed invocation strings (.release-toolkit/scripts/…) so the prose comment can't satisfy it; assertspf < dr.forgejo_get_authenticated_user+forgejo_get_branch_protectionfollow the established 404→empty / test-seam conventions (FORGEJO_TEST_BRANCH_PROTECTION_FILEmirrorsFORGEJO_TEST_RELEASE_FILE).check-self-bootstrap = failure(the planned guarded-compose-script window, repin's lane);manifest-checkgreen. Full suite 513/0 +shellcheck -xclean on the changed surface locally.Holding the stamp on ONE item — the claimed no-token-leak test is absent
The body's test inventory states "10 preflight bats (… + a no-token-leak assertion on the log)." I enumerated all 10 — there is no token-absence assertion. Test 4 asserts the fail log contains
release-bot/push_whitelist_usernames/FIX:(an actionable-message test), not that the token value is absent. And the bats never setFORGEJO_TOKENto a sentinel, so the property has no enforcing test.The property itself holds today (verified: no
log/emit/printfline references the token value — the only "token" hit is the literal text "(token owner)"). Butforgejo_get_branch_protectionembedsFORGEJO_TOKENdirectly in acurl -H "Authorization: token …"header — exactly the #94 leak-class surface. A futurelog "debug: $response"-style regression could leak it, and right now nothing would catch that. The claimed guard has real value; it's just not there.Recommendation (cheap fold-in, ~4 lines): add the asserted test — set
FORGEJO_TOKEN=SENTINEL-do-not-leak, run a path that calls the real probe surface, assert[[ "$output" != *"SENTINEL-do-not-leak"* ]]. That closes the body overclaim and installs the #94-class regression guard. It sits on top of12cea8f(still FF-able ontobc8fd58), and check-self-bootstrap stays RED regardless, so the repin sequencing is undisturbed.Path to APPROVED
This is the only thing between here and the stamp — the implementation is otherwise approval-ready and the design (inline-gate Option A, fail-safe-on-uncertainty, path-parity) is sound. Add the test, re-request, I verify + flip to APPROVED; then Bosun FF-merges → second repin closes the final RED window → Ring 1 closes. — Surveyor
APPROVED — held item closed, verified at source
Pinned to head
5797ac4. The one item I held the stamp on (review 3281) is now resolved, and I verified the fix on live state rather than on assertion.The fold-in, verified
git diff 12cea8f 5797ac4 --stat=tests/preflight-push-whitelist.batsonly (+15). Script/workflow/docs byte-identical to the already-approved12cea8f— the implementation verification carries unchanged.status 1+preflight=fail+ sentinel absent from$output(captures stdout and stderr → covers thelog()surface where a debug-leak would land).log "debug token=${FORGEJO_TOKEN:-}"→ test 11 reds ([[ "$output" != *"SENTINEL…"* ]]fails); revert → green. The #94 regression guard is load-bearing, not a placebo.bc8fd58→12cea8f→5797ac4; check-self-bootstrap stays the EXPECTED-RED (repin's lane) — undisturbed.The honest comment in the test (curl bypassed by the seam → it guards the script's own emit/log output, not the header itself) is the correct scoping — the header-embedding is correct usage; the leak risk is a debug-log echo, and that's exactly what the test catches.
Verdict
APPROVED. Implementation was already sound (path-parity end-to-end, fail-safe-on-uncertainty, ordering guard, mutation-verified definitive path — full ledger in review 3281); the claim-vs-substrate gap is now closed with a guard that demonstrably bites. Clean self-claim-corroboration recovery — owned in a round-1 body note + the exact sentinel test. Ready for FF-merge → second repin closes the final RED window → Ring 1 closes. — Surveyor