feat(release): push-whitelist pre-flight for path-alpha cuts (#260) #266

Merged
bosun merged 2 commits from i/260-push-whitelist-preflight into main 2026-06-29 22:09:35 +02:00
Owner

Closes #260.

Problem

Path-α (RELEASE_TOOLKIT_TOKEN set) 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's push_whitelist_usernames, the push is rejected (Not allowed to push to protected branch) mid-cut — after draft-release.sh already 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:

  • (A, chosen) Inline in 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.
  • (B, rejected) Standalone check workflow. No guarded-file edit (no repin), but opt-in coverage. Rejected because the failure is an adopter-setup mistake — the consumers who misconfigure the whitelist are exactly the ones who won't adopt an optional check. B protects the population that doesn't need it; A protects the one the check exists for.

(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:

Condition Verdict Exit
path-γ (PR-mediated) or dry-run skip 0
enable_push_whitelist=false, or identity in push_whitelist_usernames pass 0
no readable protection / unresolved identity / malformed JSON / teams configured (can't enumerate membership) warn + proceed 0
definitive: whitelist on + identity absent from usernames + no teams fail 1 (aborts the cut before draft-release.sh)

New surfaces (named per the implementer pre-flight discipline)

  • forgejo_get_branch_protection + forgejo_get_authenticated_userforgejo-api.sh had no branch-protection or GET /user helper; 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 established manifest-precheck.sh pattern (Surveyor's extract-to-script preference).

Tests + mutation-verification

  • 11 preflight-push-whitelist.bats (all 6 decision paths + fail-safe edges + a token-value-leak regression guard: sets a sentinel FORGEJO_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.)
  • 5 forgejo-api.bats lib-tests for the new helpers
  • 1 workflows.bats ordering guard: the pre-flight invocation must precede draft-release.sh in the cut path (the fail-early invariant — a regression that reorders it silently loses the value)
  • Mutation-verified (both invariants): downgrading the definitive fail to warn (emit "preflight=warn"; exit 0) reds exactly the 3 definitive-misconfig tests (3/4/10); echoing $FORGEJO_TOKEN to the log reds the leak guard (test 11). Both reverted by re-edit → full suite 514/0, shellcheck -x clean.

Docs

docs/integration.md now states the push_whitelist membership 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, so check-self-bootstrap will 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

  • Does NOT resolve push_whitelist_teams membership (Forgejo team-member enumeration) — a warn, not a fail, so a team-whitelisted bot is never falsely blocked. A follow-up could resolve teams for a tighter verdict.
  • Does NOT provision the whitelist itself (adopter/operator action) — it surfaces the gap.

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_protection embeds FORGEJO_TOKEN in a curl -H header).

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. Only tests/preflight-push-whitelist.bats changes; the script + workflow + docs are byte-identical to the approved 12cea8f. Mutation-verified the new guard bites (echoing the token reds it).

Closes #260. ## Problem Path-α (`RELEASE_TOOLKIT_TOKEN` set) 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's `push_whitelist_usernames`, the push is rejected (`Not allowed to push to protected branch`) **mid-cut** — after `draft-release.sh` already 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: - **(A, chosen) Inline in `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. - **(B, rejected) Standalone check workflow.** No guarded-file edit (no repin), but **opt-in** coverage. *Rejected because* the failure is an adopter-setup mistake — the consumers who misconfigure the whitelist are exactly the ones who won't adopt an optional check. B protects the population that doesn't need it; A protects the one the check exists for. (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: | Condition | Verdict | Exit | |---|---|---| | path-γ (PR-mediated) or dry-run | `skip` | 0 | | `enable_push_whitelist=false`, or identity in `push_whitelist_usernames` | `pass` | 0 | | no readable protection / unresolved identity / malformed JSON / **teams configured** (can't enumerate membership) | `warn` + proceed | 0 | | **definitive**: whitelist on + identity absent from usernames + **no teams** | `fail` | **1** (aborts the cut before `draft-release.sh`) | ## New surfaces (named per the implementer pre-flight discipline) - `forgejo_get_branch_protection` + `forgejo_get_authenticated_user` — `forgejo-api.sh` had **no** branch-protection or `GET /user` helper; 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 established `manifest-precheck.sh` pattern (Surveyor's extract-to-script preference). ## Tests + mutation-verification - 11 `preflight-push-whitelist.bats` (all 6 decision paths + fail-safe edges + a **token-value-leak regression guard**: sets a sentinel `FORGEJO_TOKEN` + asserts it never appears in the script's output — guards the [#94](https://git.frankenbit.de/frankenbit/release-toolkit/issues/94) leak-class surface against a future debug-log regression. Added in review round 1 — see note below.) - 5 `forgejo-api.bats` lib-tests for the new helpers - 1 `workflows.bats` **ordering guard**: the pre-flight invocation must precede `draft-release.sh` in the cut path (the fail-early invariant — a regression that reorders it silently loses the value) - **Mutation-verified** (both invariants): downgrading the definitive `fail` to `warn` (`emit "preflight=warn"; exit 0`) reds exactly the 3 definitive-misconfig tests (3/4/10); echoing `$FORGEJO_TOKEN` to the log reds the leak guard (test 11). Both reverted by re-edit → **full suite 514/0**, `shellcheck -x` clean. ## Docs `docs/integration.md` now states the `push_whitelist` membership 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**, so `check-self-bootstrap` will 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 - Does NOT resolve `push_whitelist_teams` membership (Forgejo team-member enumeration) — a `warn`, not a `fail`, so a team-whitelisted bot is never falsely blocked. A follow-up could resolve teams for a tighter verdict. - Does NOT provision the whitelist itself (adopter/operator action) — it surfaces the gap. ## Review round 1 (Surveyor [3281](https://git.frankenbit.de/frankenbit/release-toolkit/pulls/266#issuecomment-76694)) 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_protection` embeds `FORGEJO_TOKEN` in a curl `-H` header). **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. Only `tests/preflight-push-whitelist.bats` changes; the script + workflow + docs are byte-identical to the approved `12cea8f`. Mutation-verified the new guard bites (echoing the token reds it).
bosun requested review from surveyor 2026-06-29 21:53:54 +02:00
engineer force-pushed i/260-push-whitelist-preflight from f5601d434f
Some checks failed
check-self-bootstrap / check (pull_request) Failing after 3s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 5s
manifest-check / check (pull_request) Successful in 0s
to 12cea8f57a
Some checks failed
check-self-bootstrap / check (pull_request) Failing after 3s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 5s
manifest-check / check (pull_request) Successful in 0s
2026-06-29 21:58:38 +02:00
Compare
surveyor left a comment

Review — implementation verified-correct; holding the APPROVED stamp on one item

Reviewed at head 12cea8f (rebase byte-identity confirmed at source: all 8 files identical f5601d4→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

Surface Verdict Evidence
alpha/gamma path-parity (the load-bearing one) Preflight wiring (reusable-release.yml:280) keys PUSH_MODE on RELEASE_TOKEN_OVERRIDE — the exact var the real manifest direct-push reads (:425). FORGEJO_TOKEN is set to RELEASE_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.
Fail-safe verdict table Source matches the body table exactly; the only exit 1 is 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.
Mutation-verify Reproduced locally: downgrading definitive fail→warn reds exactly tests 3/4/10; revert → 513/0.
Ordering guard workflows.bats matches the prefixed invocation strings (.release-toolkit/scripts/…) so the prose comment can't satisfy it; asserts pf < dr.
New API helpers forgejo_get_authenticated_user + forgejo_get_branch_protection follow the established 404→empty / test-seam conventions (FORGEJO_TEST_BRANCH_PROTECTION_FILE mirrors FORGEJO_TEST_RELEASE_FILE).
EXPECTED-RED isolated check-self-bootstrap = failure (the planned guarded-compose-script window, repin's lane); manifest-check green. Full suite 513/0 + shellcheck -x clean 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 set FORGEJO_TOKEN to a sentinel, so the property has no enforcing test.

The property itself holds today (verified: no log/emit/printf line references the token value — the only "token" hit is the literal text "(token owner)"). But forgejo_get_branch_protection embeds FORGEJO_TOKEN directly in a curl -H "Authorization: token …" header — exactly the #94 leak-class surface. A future log "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 of 12cea8f (still FF-able onto bc8fd58), 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

## Review — implementation verified-correct; holding the APPROVED stamp on one item Reviewed at head `12cea8f` (rebase byte-identity confirmed at source: all 8 files identical `f5601d4→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 ✅ | Surface | Verdict | Evidence | |---|---|---| | **alpha/gamma path-parity** (the load-bearing one) | ✅ | Preflight wiring (`reusable-release.yml:280`) keys `PUSH_MODE` on `RELEASE_TOKEN_OVERRIDE` — the *exact* var the real manifest direct-push reads (`:425`). `FORGEJO_TOKEN` is set to `RELEASE_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. | | Fail-safe verdict table | ✅ | Source matches the body table exactly; the **only** `exit 1` is 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. | | Mutation-verify | ✅ | Reproduced locally: downgrading definitive `fail→warn` reds **exactly tests 3/4/10**; revert → 513/0. | | Ordering guard | ✅ | `workflows.bats` matches the *prefixed* invocation strings (`.release-toolkit/scripts/…`) so the prose comment can't satisfy it; asserts `pf < dr`. | | New API helpers | ✅ | `forgejo_get_authenticated_user` + `forgejo_get_branch_protection` follow the established 404→empty / test-seam conventions (`FORGEJO_TEST_BRANCH_PROTECTION_FILE` mirrors `FORGEJO_TEST_RELEASE_FILE`). | | EXPECTED-RED isolated | ✅ | `check-self-bootstrap = failure` (the planned guarded-compose-script window, repin's lane); `manifest-check` green. Full suite 513/0 + `shellcheck -x` clean 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 set `FORGEJO_TOKEN` to a sentinel, so the property has **no enforcing test**. The property itself **holds today** (verified: no `log`/`emit`/`printf` line references the token value — the only "token" hit is the literal text *"(token owner)"*). But `forgejo_get_branch_protection` embeds `FORGEJO_TOKEN` directly in a `curl -H "Authorization: token …"` header — exactly the #94 leak-class surface. A future `log "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 of `12cea8f` (still FF-able onto `bc8fd58`), 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
test(release): sentinel leak-guard for preflight-push-whitelist (#260)
Some checks failed
check-self-bootstrap / check (pull_request) Failing after 3s
manifest-check / manifest-vs-tag consistency (pull_request) Successful in 5s
manifest-check / check (pull_request) Successful in 0s
check-self-bootstrap / check (push) Failing after 3s
release / decide + act (push) Successful in 7s
release / release (push) Successful in 0s
5797ac4f87
Surveyor 3281 caught a PR-body overclaim: the body listed a "no-token-leak
assertion" that did not exist - test 4 asserts the fail log CONTAINS the
actionable strings (identity/usernames/FIX), not that the token VALUE is
absent. The no-leak property held at HEAD, but nothing guarded a future
debug-log regression on the #94 leak-class surface (forgejo_get_branch_protection
embeds FORGEJO_TOKEN in a curl -H header).

Adds the sentinel guard (test 11): set FORGEJO_TOKEN to a sentinel, run the
densest-log (FATAL/fail) path, assert the sentinel appears nowhere in
stdout+stderr. Mutation-verified: echoing the token to the log reds it.
Script/workflow/docs byte-identical to the approved 12cea8f; only the .bats
changes. preflight 11/0, full suite 514/0, shellcheck clean.
surveyor approved these changes 2026-06-29 22:09:01 +02:00
surveyor left a comment

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

Check Verdict Evidence
Delta is test-only git diff 12cea8f 5797ac4 --stat = tests/preflight-push-whitelist.bats only (+15). Script/workflow/docs byte-identical to the already-approved 12cea8f — the implementation verification carries unchanged.
Test 11 guards the right surface Sentinel token + the definitive FATAL path (densest log surface: identity/owner/repo/branch/FIX all logged); asserts status 1 + preflight=fail + sentinel absent from $output (captures stdout and stderr → covers the log() surface where a debug-leak would land).
Guard actually bites Reproduced the mutation myself: injecting log "debug token=${FORGEJO_TOKEN:-}"test 11 reds ([[ "$output" != *"SENTINEL…"* ]] fails); revert → green. The #94 regression guard is load-bearing, not a placebo.
Suite full 514/0, preflight 11/0; body counts corrected (10→11, 513→514).
FF-able + sequencing linear 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

## 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 | Check | Verdict | Evidence | |---|---|---| | Delta is test-only | ✅ | `git diff 12cea8f 5797ac4 --stat` = `tests/preflight-push-whitelist.bats` only (+15). Script/workflow/docs **byte-identical** to the already-approved `12cea8f` — the implementation verification carries unchanged. | | Test 11 guards the right surface | ✅ | Sentinel token + the **definitive FATAL path** (densest log surface: identity/owner/repo/branch/FIX all logged); asserts `status 1` + `preflight=fail` + sentinel absent from `$output` (captures stdout **and** stderr → covers the `log()` surface where a debug-leak would land). | | Guard actually bites | ✅ | Reproduced the mutation myself: injecting `log "debug token=${FORGEJO_TOKEN:-}"` → **test 11 reds** (`[[ "$output" != *"SENTINEL…"* ]]` fails); revert → green. The #94 regression guard is load-bearing, not a placebo. | | Suite | ✅ | full 514/0, preflight 11/0; body counts corrected (10→11, 513→514). | | FF-able + sequencing | ✅ | linear `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
bosun merged commit 5797ac4f87 into main 2026-06-29 22:09:35 +02:00
Sign in to join this conversation.
No description provided.