From 037b40606d28aad3f9d2b8f58adf8fc21ae4d576 Mon Sep 17 00:00:00 2001 From: Christopher Pitzi Date: Sat, 22 Aug 2026 09:59:24 -0400 Subject: [PATCH] fix: drop the dead Actions-app allowance and add a post-apply convergence check Co-Authored-By: Claude Fable 5 --- .github/workflows/terraform.yml | 16 ++++++++ docs/adr/0003-merge-gate-push-allowlist.md | 37 +++++++++++++++-- ...-08-12-merge-gate-silent-allowance-drop.md | 32 +++++++++++++++ terraform/README.md | 40 ++++++++++++++----- terraform/locals.tf | 34 +++++++++------- 5 files changed, 132 insertions(+), 27 deletions(-) diff --git a/.github/workflows/terraform.yml b/.github/workflows/terraform.yml index 48b993e..07d802f 100644 --- a/.github/workflows/terraform.yml +++ b/.github/workflows/terraform.yml @@ -245,6 +245,22 @@ jobs: - name: apply run: terraform apply -auto-approve -no-color -input=false + # Convergence check. "Apply complete" is the provider's claim, not the + # state's: GitHub drops an ineligible branch-protection actor silently, + # the provider reports success, and the change never lands — which is how + # one perpetual diff was re-"applied" green on every merge for ten days + # (#148). A plan that still proposes changes immediately after an apply + # is a red run, not a curiosity. `-lock=false`: apply has released the + # lock and this is read-only. + - name: verify convergence + run: | + terraform plan -detailed-exitcode -no-color -input=false -lock=false && rc=0 || rc=$? + case "$rc" in + 0) echo "converged: plan proposes no changes after apply" ;; + 2) echo "::error::apply did not converge — terraform plan still proposes changes immediately after apply. Something was accepted and not kept on the way to GitHub; read the plan above." ; exit 1 ;; + *) echo "::error::post-apply plan failed to run (exit $rc)" ; exit 1 ;; + esac + # Always-on fan-in. This is the context phase 2 adds to required-checks.json: # it reports on every PR (green when the terraform jobs skipped), so requiring # it cannot deadlock a docs-only PR. diff --git a/docs/adr/0003-merge-gate-push-allowlist.md b/docs/adr/0003-merge-gate-push-allowlist.md index cd53ca1..75efdcf 100644 --- a/docs/adr/0003-merge-gate-push-allowlist.md +++ b/docs/adr/0003-merge-gate-push-allowlist.md @@ -1,6 +1,6 @@ # ADR-0003: The merge gate is a classic branch-protection push allowlist, not ruleset bypass -**Status:** Accepted (2026-08-12; reconstructed 2026-08-13) +**Status:** Accepted (2026-08-12; reconstructed 2026-08-13; amended 2026-08-22 — see [Amendment](#amendment-2026-08-22)) ## Context @@ -43,6 +43,8 @@ register (`fleet-reports/incidents/2026-08-12-merge-gate-silent-allowance-drop.m mutation *silently drops* an app actor declared by its legacy global node id (`MDM6QXBwMTUzNjg=`) and reports no error; the fix (#98) re-declares the Actions app by its next-format id (`A_kwHNJr_NPAg`), which GraphQL's deprecation warning supplies. +*(This diagnosis was wrong — see the Amendment below. The text is kept as the record of +what was believed on 2026-08-12.)* PR #97 separately imported asclepias's live `main` ruleset id so adoption would not create a duplicate. @@ -68,8 +70,37 @@ create a duplicate. fleet; `music-curator` additionally allows the Actions app. - The failure mode is **silent** (a PR that never merges), so the rollout step is explicitly: apply, then verify auto-merge still arms and fires on the next routine PR. -- The node-id gotcha is now recorded in `locals.tf`, the adoption record, and the +- ~~The node-id gotcha is now recorded in `locals.tf`, the adoption record, and the incident register — a hand-authored app-actor declaration by legacy id will silently - under-provision the allowlist. + under-provision the allowlist.~~ Superseded by the Amendment: the real gotcha is + actor *eligibility*, and it is recorded in the same three places. - Workflows that push to PR branches (not `main`) are unaffected; the restriction matches `main` only. + +## Amendment (2026-08-22) + +**The music-curator allowance is withdrawn, and the "second defect" above was +misdiagnosed.** Issue #148 established, against live state and GitHub's own +documentation, that the built-in GitHub Actions app is **not an eligible push actor** +on classic branch protection in any id format: the allowlist accepts users, teams, and +GitHub Apps *installed* on the repository, and the identity behind `GITHUB_TOKEN` is not +an installation (GitHub declines it deliberately — otherwise any collaborator could reach +`main` by authoring a workflow; community discussion #25305). The mutation drops an +ineligible actor silently, so #98's next-format id "fix" changed nothing: every CI apply +from 2026-08-12 to 2026-08-22 reported `1 changed` on `merge_gate["music-curator"]` and +live `restrictions.apps` stayed `[]` throughout — a perpetual diff, not an apply still +owed. The allowance was removed under #148; `gate_extra_allowances` stays as the +extension point, with the eligibility rule in its comment. + +The decision itself stands unchanged: the gate is a classic push allowlist, and `cpitzi` +is its only actor. What changes is the consequence for `music-curator`: its follow-fold +bot merge was never reachable under `GITHUB_TOKEN` (a second, independent blocker — +required checks that never report on a `GITHUB_TOKEN` push — is recorded alongside it), +and **music-curator#87** owns the decision between a dedicated installed GitHub App, +a PAT of an allowed user, or dropping the bot merge. Whichever lands, the fleet-wide +invariant holds: a workflow reaches `main` only through an identity this allowlist +names explicitly. + +Also adopted under #148: the apply job now runs `terraform plan -detailed-exitcode` +immediately after `apply` and fails the run if changes remain. "Apply complete" is the +provider's claim, not the state's; a silent drop is now a red run. diff --git a/fleet-reports/incidents/2026-08-12-merge-gate-silent-allowance-drop.md b/fleet-reports/incidents/2026-08-12-merge-gate-silent-allowance-drop.md index 59a4c83..f1f02e7 100644 --- a/fleet-reports/incidents/2026-08-12-merge-gate-silent-allowance-drop.md +++ b/fleet-reports/incidents/2026-08-12-merge-gate-silent-allowance-drop.md @@ -96,3 +96,35 @@ Session `4f4eec0c` (2026-08-12, `~/repos` ↔ `~/repos/dotgithub`); read-back of music-curator branch protection at harvest; `terraform/locals.tf` comments carrying the node-ID and bypass_actors gotchas; community refs #162623/#113172 (auto-merge vs bypass_actors). + +--- + +## Correction (2026-08-22) + +**Item 5 above is closed — by removal, not by re-apply — and defect (4)'s root cause +was misread.** The "one-change re-apply" *did* run, on every CI apply from 2026-08-12 +through 2026-08-19; each reported `1 changed` on `merge_gate["music-curator"]`, and +live `restrictions.apps` stayed `[]` after every one. The legacy-vs-next-format node id +was never the mechanism. The built-in GitHub Actions app is not an eligible push actor +on classic branch protection in any id format — the allowlist admits users, teams, and +GitHub Apps *installed* on the repo, and the identity behind `GITHUB_TOKEN` is not an +installation (GitHub refuses it deliberately; community discussion #25305). The +mutation drops an ineligible actor silently, which is what the read-back saw on +2026-08-12 and what it would have seen after #98 had anyone read back again. + +Two further findings from the re-investigation (.github#148): + +- **music-curator's "degradation" never happened, because the capability never + existed.** Every one of the repo's 59 merged PRs was merged by the owner; no drain + PR has ever been opened; `follow-fold.yml` has run twice, both skipped by its own + branch guard. The bot merge was a design, not an observed behaviour — and it was + independently blocked by the required checks adopted on 2026-07-25, which never + report on a commit pushed with `GITHUB_TOKEN`. music-curator#87 owns the decision. +- **Lesson 1 was right and was not followed through.** "Read back what you wrote" + caught the drop once, by hand, on 2026-08-12 — and then the fix was trusted without + a second read-back, so the perpetual diff sat green for ten days. The durable + remedy landed under #148: the apply job runs `terraform plan -detailed-exitcode` + immediately after `apply` and fails the run if anything still differs. A silent + drop is now a red run without anyone remembering to look. + +The dead allowance was removed from the module (ADR-0003 amended the same day). diff --git a/terraform/README.md b/terraform/README.md index 7fc6571..50accfd 100644 --- a/terraform/README.md +++ b/terraform/README.md @@ -196,10 +196,11 @@ The state removal is the deliberate step that says "this is intentional." `protection.tf` puts a classic branch-protection **push allowlist** on `main` in every public repo: only `cpitzi` (org owner) can update the ref — direct -push or PR merge — plus, on `music-curator` only, the GitHub Actions app, so -follow-fold's documented bot merge (music-curator#9) keeps working. Colleagues -on the Players team contribute via PRs; an owner reviews and arms auto-merge. -A future write grant to anyone else still cannot reach `main`. +push or PR merge. Colleagues on the Players team contribute via PRs; an owner +reviews and arms auto-merge. A future write grant to anyone else still cannot +reach `main`. No repo carries an extra allowance today; `gate_extra_allowances` +in `locals.tf` is the extension point, and its comment records what GitHub will +and will not accept there. Two things about the shape that look odd and are load-bearing: @@ -214,9 +215,26 @@ Two things about the shape that look odd and are load-bearing: ruleset (`rulesets.tf`) — the two layers compose, and duplicating a rule in both would mean two places to drift. +**The built-in GitHub Actions app cannot be on the allowlist.** Classic +protection accepts only users, teams, and GitHub Apps *installed* on the repo +as push actors. The identity behind `GITHUB_TOKEN` is not an installation, and +GitHub refuses it on purpose (community discussion #25305 — otherwise any +collaborator could reach `main` by authoring a workflow). The mutation drops an +ineligible actor *silently*: the apply reports success, the read-back lacks the +actor, and the next plan proposes it again. That is how an allowance meant to +keep music-curator's follow-fold bot merge working sat in this module as a +perpetual diff from 2026-08-12 to 2026-08-22, re-"applied" on every merge +(#148; the earlier diagnosis in #98, a node-id format problem, was wrong). A +workflow that must update `main` needs a real identity — a dedicated GitHub App +installed on that repo, allowlisted here by its `A_…` node id, or a PAT of an +allowed user; music-curator#87 owns that decision for follow-fold. + After changing the gate, verify auto-merge still arms and fires on the next routine PR before trusting it fleet-wide — the failure mode is silent -(a PR that just never merges). +(a PR that just never merges). The apply job's post-apply **convergence +check** (`terraform plan -detailed-exitcode` immediately after `apply`) covers +the other silent failure: a change GitHub accepted and did not keep is now a +red run, not a green one. ### Required checks — the one rule Terraform cannot enforce @@ -275,11 +293,13 @@ is a drift correction rather than a policy change. > in #97), the 16 merge-gate branch protections created (#96), and the changes > were provider-flag materialisations plus asclepias's stock labels recoloring > to Tidewater. One defect surfaced in verification: the GitHub Actions app -> allowance on music-curator was **silently dropped** because the app was -> declared by its legacy global node id — the branch-protection mutation -> accepts only next-format (`A_…`) ids for apps, and reports no error for a -> legacy one. Fixed by re-declaring the app with its next-format id; when -> adding any app to an allowlist, verify it landed: +> allowance on music-curator was **silently dropped**. It was diagnosed at the +> time as a node-id format problem and "fixed" in #98 by re-declaring the app +> with its next-format (`A_…`) id. That diagnosis was wrong — the built-in +> Actions app is not an eligible push actor in any id format (see *The merge +> gate* above) — and #98 left a perpetual diff that every apply re-attempted +> until the allowance was removed on 2026-08-22 (#148). The verification step +> stands: after adding any actor to an allowlist, confirm it landed — > `gh api repos/lentago//branches/main/protection --jq '.restrictions.apps[].slug'`. ## Known asymmetries diff --git a/terraform/locals.tf b/terraform/locals.tf index a4ccec2..3806ac1 100644 --- a/terraform/locals.tf +++ b/terraform/locals.tf @@ -50,20 +50,26 @@ locals { # --------------------------------------------------------------------------- gate_allowlist = ["/cpitzi"] - # Repo-scoped additions to the allowlist. music-curator's follow-fold - # workflow performs its documented bot merge (`gh pr merge --squash` under - # GITHUB_TOKEN — see music-curator#9), so the GitHub Actions app is allowed - # there, and only there: fleet-wide, a workflow must not be able to update - # `main`. - # The id MUST be the next-format global node id (`A_…`). The legacy base64 - # form (`MDM6QXBwMTUzNjg=`) still resolves in queries, but the branch - # protection mutation SILENTLY DROPS it from pushAllowances — the first apply - # (2026-08-12) landed music-curator's rule with no app actor and no error. - # Resolve with: gh api graphql -f query='{ node(id:"") { id } }' - # and read next_global_id from the deprecation warning in extensions. - gate_extra_allowances = { - "music-curator" = ["A_kwHNJr_NPAg"] # the GitHub Actions app (id 15368) - } + # Repo-scoped additions to the allowlist — empty today, kept as the extension + # point. Fleet-wide, a workflow must not be able to update `main`. + # + # What CAN go here: a GitHub App that is INSTALLED on that repo with write + # access, by its next-format global node id (`A_…`); a team ("org/slug"); a + # user ("/login"). What CANNOT: the built-in GitHub Actions app (id 15368, + # the identity behind GITHUB_TOKEN). It is not an installation, so classic + # branch protection does not accept it as a push actor — GitHub's position is + # deliberate (any collaborator could otherwise reach `main` by authoring a + # workflow; community discussion #25305) — and the mutation DROPS an + # ineligible actor silently: the apply reports success, the read-back lacks + # the actor, and every following plan proposes it again. An entry for it sat + # here as exactly that perpetual diff from 2026-08-12 to 2026-08-22 under a + # wrong diagnosis (#98 blamed the legacy id format; the id format was never + # the cause) — see #148. After adding any actor here, verify it landed: + # gh api repos/lentago//branches/main/protection \ + # --jq '.restrictions | {users: [.users[].login], apps: [.apps[].slug]}' + # The apply job's post-apply convergence check turns a silent drop into a + # red run (.github/workflows/terraform.yml). + gate_extra_allowances = {} # --------------------------------------------------------------------------- # Free-plan carve-outs. Both are plan limits, not policy choices, and both