From 5b600b775f8e32d51611c0a4f8961306df0d5b9a Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 26 Aug 2026 17:44:02 -0400 Subject: [PATCH 01/10] feat(ci): execute the gates that certify a pull request from the default branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every CI gate here runs from the pull request's own checkout, so a pull request can neuter the check that certifies it and still produce a green required status. Measured on #98, whose clean-clone-builds executed three "(network denied)" steps that exist only on that branch while main's ci.yml contained none. Reading a script from origin/main does not fix it: the step invoking it is equally under the pull request's control. Move the gates that matter onto pull_request_target, which GitHub executes from the repository's default branch — workflow file, referenced actions and the actions/checkout commit alike. trusted-dco becomes the authoritative sign-off gate, reading the pull request's commits as fetched git objects that are never checked out or executed; ci.yml's dco job stays as a faster advisory report that can only fail open. gate-integrity closes the other half: a required check is matched by name, so a pull request that cannot edit the trusted job could still shadow it, and every route to that runs through .github/workflows/. Both jobs import Node builtins only and run with no bun install, declare read-only permissions, keep the token out of git via persist-credentials: false, and pass untrusted values through env rather than ${{ }} inside run bodies. Commit subjects are echoed inside ::stop-commands:: so a message reading ::error:: cannot forge annotations in a privileged run. check-gate-integrity's pass condition is an absence, so it refuses to pass over an empty changed-file list, a truncated one, or an unreadable payload — each observed firing, along with the guard itself blocking on this change's real changed paths and passing once the label is applied (ADR-0016). The test suite fails if a surface is added to GATE_SURFACES without a case observing it block. Required review is declined rather than shipped: GitHub does not let an author approve their own pull request, so with a sole maintainer it deadlocks or is waived by the admin bypass that is already always. ADR-0035 records that reasoning. Repository-wide SHA pinning for actions is now required, and docs/repository-trust-operations.md separates the controls that are active from the ruleset change that cannot be applied until this lands. None of this is tamper-proof, and nothing here says it is. Whoever can merge can change a gate and acknowledge it. What is closed is narrower: the check certifying a pull request is no longer authored by that pull request. Closes #137 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- .github/workflows/ci.yml | 8 + .github/workflows/trusted-gates.yml | 197 ++++++++++ CHANGELOG.md | 45 +++ CODEOWNERS | 27 ++ ...-a-pull-request-from-the-default-branch.md | 315 ++++++++++++++++ docs/repository-trust-operations.md | 263 +++++++++++++ package.json | 1 + scripts/check-dco.ts | 31 +- scripts/check-gate-integrity.test.ts | 239 ++++++++++++ scripts/check-gate-integrity.ts | 356 ++++++++++++++++++ 10 files changed, 1473 insertions(+), 9 deletions(-) create mode 100644 .github/workflows/trusted-gates.yml create mode 100644 docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md create mode 100644 docs/repository-trust-operations.md create mode 100644 scripts/check-gate-integrity.test.ts create mode 100644 scripts/check-gate-integrity.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d501157..66daf1ed 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -538,6 +538,14 @@ jobs: # inside the surface ADR-0007 keeps mechanical and self-contained. It imports # only Node builtins and therefore runs with no `bun install`: a broken # dependency graph cannot take the sign-off gate down with it. + # + # **Advisory since ADR-0035.** This job runs from the pull request's own + # checkout, so the pull request can edit both this step and the script it + # calls (#137, measured on #98). It is kept because it is faster and reports + # first, and because it can only ever fail *open* while the authoritative + # `trusted-dco` job in `trusted-gates.yml` — executed from the default branch, + # outside the pull request's control — also has to pass. Do not treat a green + # result here as the sign-off gate. if: github.event_name == 'pull_request' runs-on: ubuntu-latest permissions: diff --git a/.github/workflows/trusted-gates.yml b/.github/workflows/trusted-gates.yml new file mode 100644 index 00000000..be6b0fdf --- /dev/null +++ b/.github/workflows/trusted-gates.yml @@ -0,0 +1,197 @@ +name: Trusted gates + +# The gates whose verdict must not be editable by the pull request they judge. +# +# Every job in `ci.yml` runs from the pull request's own checkout, so a pull +# request can edit both a check and the step that invokes it and still produce a +# green required status. That is not a hypothesis: #98's `clean-clone-builds` +# executed three steps named "(network denied)" that exist only on that branch, +# while `main`'s `ci.yml` contained no such step. Recorded as #137 and decided in +# ADR-0035. +# +# `pull_request_target` is the one trigger available to a personal-namespace +# repository that runs outside the pull request's control. Since 2025-12-08 GitHub +# takes the workflow file, every referenced action, and the `actions/checkout` +# commit for this event from the repository's **default branch** — not from the +# pull request, and not even from its base branch. A pull request therefore cannot +# edit what runs here, only what it is run against. +# +# ## The rule that keeps this safe +# +# `pull_request_target` runs with the base repository's token and secrets. Nothing +# below may check out, install, build, or execute pull-request code. Concretely: +# +# * `actions/checkout` is used with no `ref:`, so it takes the default branch. +# v7 additionally refuses fork pull request refs here by design. +# * The pull request's commits are fetched as *objects* and read with `git log`. +# They are never checked out into the worktree and never run. +# * There is no `bun install`. Both checks import Node builtins only, so a +# hostile lockfile has nothing to hook. +# * Untrusted values reach steps through `env:`, never through `${{ }}` inside a +# `run:` body. +# * `permissions:` is read-only, and `persist-credentials: false` keeps the token +# out of the git config that the fetch below uses. +# +# ## What this does not claim +# +# Whoever can merge can still change these gates and label the change. Merge access +# remains the boundary it always was. What is closed is the narrower and more +# dangerous property: that the change certifying a pull request could be authored +# by that same pull request. + +on: + pull_request_target: + # `labeled` and `unlabeled` are load-bearing rather than tidy: the + # acknowledgment below is a label, so without them applying it would leave the + # required check red with no way to re-run it except a push. + types: [opened, synchronize, reopened, labeled, unlabeled] + +permissions: + contents: read + +concurrency: + group: trusted-gates-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + trusted-dco: + # ADR-0006 calls licensing "the most irreversible decision in the project" and + # chose a DCO over a CLA. A neutered dependency check yields a bad edge that a + # follow-up fixes; a neutered sign-off check yields unsigned commits in the + # permanent history of an Apache-2.0 project. Same mechanism, asymmetric blast + # radius — which is why this one is the first to move. + # + # `ci.yml` keeps a `dco` job. It is faster and reports first, but it is + # advisory: it runs from the pull request and can only fail open. This job is + # the authority. + name: trusted-dco + runs-on: ubuntu-latest + permissions: + contents: read + steps: + # No `ref:`. For `pull_request_target` this resolves to the default branch, + # which is the entire point — `scripts/check-dco.ts` below is `main`'s copy. + # `fetch-depth: 0` because the range's base commit is an ancestor of the + # default branch and a shallow clone would not contain it. + - name: Check out the default branch (never the pull request) + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Set up Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 + with: + bun-version: 1.3.14 + + # Objects, not a checkout. `git fetch` moves commit and tree data into the + # object store; nothing is written to the worktree and nothing is executed. + # Anonymous over https, because `persist-credentials: false` left no token in + # the git config and this repository is public. + - name: Fetch the pull request's commit objects + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: | + set -euo pipefail + git fetch --no-tags --no-recurse-submodules origin \ + "+refs/pull/${PR_NUMBER}/head:refs/adrkit/pr-head" \ + "+refs/heads/${BASE_REF}:refs/adrkit/pr-base" + + # Explicit SHAs from the event payload, not refs. A ref resolves to whatever + # it points at now, so a branch that moved mid-run silently changes which + # commits were checked — the stale-read failure ADR-0016 records under + # "report what was examined". + # + # Asserting the objects exist *before* running the check is the difference + # between "these commits are signed" and "these commits could not be read". + # A force-push between the event and this step lands here, and lands red. + - name: Verify both endpoints of the range are present + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + for sha in "$BASE_SHA" "$HEAD_SHA"; do + if ! git cat-file -e "${sha}^{commit}" 2>/dev/null; then + echo "commit ${sha} is not in this clone — the head was probably force-pushed" >&2 + echo "between the event and this run. Refusing to report a pass over a range" >&2 + echo "that cannot be read." >&2 + exit 1 + fi + done + echo "both endpoints present: ${BASE_SHA}..${HEAD_SHA}" + + # The script path directly, not `bun run check:dco`. The manifest is trusted + # here too, but one less indirection is one less thing to reason about — and + # it makes the invocation independent of a `package.json` script rename. + # + # Commit subjects are untrusted text and are echoed. `::stop-commands::` + # neutralizes the workflow-command syntax for the duration, so a commit + # message reading `::error::` cannot forge annotations in a privileged run. + # The trap restores it on failure as well as success, without disturbing the + # exit status the gate depends on. + - name: Verify every commit carries a DCO sign-off + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + token="adrkit-$(openssl rand -hex 16)" + trap 'echo "::${token}::"' EXIT + echo "::stop-commands::${token}" + bun scripts/check-dco.ts "${BASE_SHA}..${HEAD_SHA}" + + gate-integrity: + # `trusted-dco` closes the "edit the check" half. This closes the other half. + # + # A required status check is matched by *name*, so a pull request that cannot + # edit the trusted job can still declare a job of its own with the same name + # and let the later result stand. Every route to that runs through a change + # under `.github/workflows/`, and every route to neutering an advisory gate + # runs through `.github/workflows/` or `scripts/`. This refuses both unless a + # maintainer has said, on the record, that the change is deliberate. + name: gate-integrity + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + steps: + - name: Check out the default branch (never the pull request) + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Set up Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 + with: + bun-version: 1.3.14 + + # `--slurp` because `--paginate` alone concatenates one JSON array per page + # into a document that is not JSON. Written to a file rather than piped + # through a shell variable so that a path containing a newline — which git + # permits — cannot reframe the list. + - name: List the pull request's changed paths and labels + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + PR_LABELS: ${{ toJSON(github.event.pull_request.labels) }} + run: | + set -euo pipefail + gh api --paginate --slurp \ + "repos/${REPO}/pulls/${PR_NUMBER}/files" > "${RUNNER_TEMP}/pr-files.json" + printf '%s' "$PR_LABELS" > "${RUNNER_TEMP}/pr-labels.json" + + # `--expected-files` is the fail-quiet guard. The files endpoint caps at 3000 + # entries and truncates rather than erroring, and a gate path past the cap + # would be invisible to a check whose pass condition is "no gate path here". + - name: Verify no gate-defining path changed without an acknowledgment + env: + CHANGED_FILES: ${{ github.event.pull_request.changed_files }} + run: | + set -euo pipefail + bun scripts/check-gate-integrity.ts \ + --files "${RUNNER_TEMP}/pr-files.json" \ + --labels "${RUNNER_TEMP}/pr-labels.json" \ + --expected-files "${CHANGED_FILES}" diff --git a/CHANGELOG.md b/CHANGELOG.md index 19cf970a..07fb8df2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,51 @@ Until `1.0.0`, minor releases may include breaking changes ## [Unreleased] +### Added + +- **Trusted CI gates that the pull request cannot edit.** A new + `.github/workflows/trusted-gates.yml` runs on `pull_request_target`, which + GitHub executes from the repository's default branch — workflow file, + referenced actions, and `actions/checkout` commit alike. `trusted-dco` is now + the authoritative sign-off gate, reading the pull request's commits as fetched + git objects that are never checked out or executed; the `dco` job in `ci.yml` + is retained as a faster advisory report that can only fail open. `gate-integrity` + blocks any change under `.github/workflows/`, `.github/actions/`, `scripts/`, + `packages/ci/`, or `CODEOWNERS` unless a maintainer applies the + `gate-change-acknowledged` label, which requires triage or write access + ([#137](https://github.com/mbeacom/adrkit/issues/137), + [ADR-0035](docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md)). + +- **`scripts/check-gate-integrity.ts`**, backing that second gate. Imports Node + builtins only, so it runs with no `bun install` and a broken dependency graph + cannot take it down. Its pass condition is an absence, so it refuses to report + a pass over an empty changed-file list, over a list the GitHub API truncated, + or over a payload it could not parse — each of those observed firing before the + check counted as coverage + ([ADR-0016](docs/adr/0016-require-every-check-to-be-observed-failing-before-it-counts-as-coverage.md)). + +- **[`docs/repository-trust-operations.md`](docs/repository-trust-operations.md)**, + separating the controls that are active from the ones that cannot be applied + until this lands, with the exact verified commands and the evidence for each. + +### Changed + +- **Actions must now be pinned to a full-length commit SHA** at the repository + level (`sha_pinning_required`, `false` → `true`). Every action here was already + SHA-pinned, so no workflow changed; the setting removes the ability to + introduce a mutable tag later. + +- **`CODEOWNERS` names the gate-defining paths explicitly**, with both caveats + stated in the file: the default `*` line already covered them, and with no + `pull_request` rule on the `main` ruleset these lines request a review rather + than requiring one. ADR-0035 records why required review is not available to + this repository as a real control rather than shipping a rule that only looks + like one. + +- **`scripts/check-dco.ts` no longer carries a "known limitation" note.** It now + states which invocation is the authority and which is advisory, because the + limitation stopped being true for the one that gates the merge. + ## [0.11.0] - 2026-08-26 ### Added diff --git a/CODEOWNERS b/CODEOWNERS index aae45d13..bc1c6cfa 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -10,3 +10,30 @@ /LICENSE @mbeacom /NOTICE @mbeacom /CONTRIBUTING.md @mbeacom + +# Gate-defining surfaces — changes here alter what every other check certifies +# +# Listed for #137 / ADR-0035. Two honest caveats, so nobody reads more into these +# lines than they carry: +# +# 1. `*` above already makes @mbeacom the owner of every path, so these entries +# add no *coverage* today. They add explicitness, and they survive a future +# narrowing of the default line — which is exactly when a gap here would be +# easiest to introduce and hardest to notice. +# 2. The `main` ruleset carries no `pull_request` rule, so CODEOWNERS currently +# *requests* a review rather than requiring one. Required review is not +# available to this repository as a real control: GitHub does not let an +# author approve their own pull request, and with a sole maintainer that +# deadlocks every self-authored change or is waived by the admin bypass that +# is already `always`. ADR-0035 records that reasoning rather than shipping a +# rule that only looks like one. +# +# The control that does bite on these paths is the `gate-integrity` job in +# `.github/workflows/trusted-gates.yml`, which runs from the default branch and +# blocks until a maintainer applies `gate-change-acknowledged`. + +/.github/workflows/ @mbeacom +/.github/actions/ @mbeacom +/scripts/ @mbeacom +/packages/ci/ @mbeacom +/CODEOWNERS @mbeacom diff --git a/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md b/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md new file mode 100644 index 00000000..0936c7e5 --- /dev/null +++ b/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md @@ -0,0 +1,315 @@ +--- +schemaVersion: 0.1.0 +id: "0035" +title: Execute the gates that certify a pull request from the default branch +status: proposed +date: 2026-08-26 +deciders: ["@mbeacom"] +tags: [ci, governance, security, supply-chain, provenance] +scope: org +reversibility: two-way-door +blastRadius: org +relatesTo: ["0006", "0007", "0014", "0016", "0026"] +affects: + - type: path + pattern: ".github/workflows/**" + - type: path + pattern: ".github/actions/**" + - type: path + pattern: "scripts/**" + - type: path + pattern: "packages/ci/**" + - type: path + pattern: "CODEOWNERS" +assertions: + - id: gate-change-acknowledged + description: >- + A pull request that changes a gate-defining path — any file under + .github/workflows/, .github/actions/, scripts/, or packages/ci/, or + CODEOWNERS itself — must carry the gate-change-acknowledged label, which + requires triage or write access to apply. + engine: custom + expression: gate-change-acknowledged + input: source + severity: error +provenance: + authoredBy: agent-drafted +review: + tier: arb + tierReason: >- + Changes how this repository's own provenance guarantees are enforced, which + ADR-0006 treats as effectively irreversible once external contributions land, + and adds a privileged workflow trigger to the surface ADR-0007 keeps + mechanical and self-contained. + queuedAt: 2026-08-26T00:00:00Z + slaDays: 30 +reviewBy: 2027-02-26 +--- + +# ADR-0035: Execute the gates that certify a pull request from the default branch + +> **Status: proposed.** Agent-drafted. It binds nothing until a human accepts it +> (ADR-0016 action item 1 shape). It does not supersede any record; it closes the +> gap #137 opened against ADR-0006's provenance claim. + +## Context + +Every CI gate in this repository is executed from the pull request's own +checkout, so a pull request can neuter the check that is supposed to certify it +and still produce a green required status. + +**Measured, not assumed.** `pull_request` workflows run the *pull request's* +`ci.yml`, not `main`'s. PR #98's `clean-clone-builds` executed steps named +`Typecheck (network denied)`, `Build (network denied)`, and `Verify publishable +package tarballs (network denied)` — steps that exist only on that branch, while +`main`'s `ci.yml` contained zero occurrences of `network-denied`. The property is +not specific to any one gate: it holds for `check-deps`, `audit-gate`, +`check-freeze-hashes`, `check-doc-cli-versions`, `check-changelog`, the schema +emit-parity gate, the `packages/ci/dist` bundle diff, `adr check` in +`self-dogfood`, and `dco`. + +**Why the obvious partial fix does not work.** Reading a script from a trusted +base revision — `git show origin/main:scripts/check-dco.ts` — leaves the +*workflow step that invokes it* under the pull request's control, so the same +change simply edits the step. A control that looks like a control and is not one +is worse than a documented gap ([ADR-0016](0016-require-every-check-to-be-observed-failing-before-it-counts-as-coverage.md)). + +**Why it matters most for DCO.** A neutered `check-deps` yields a bad dependency +edge, fixable in a follow-up. A neutered sign-off check yields unsigned commits +in the permanent history of an Apache-2.0 project, and +[ADR-0006](0006-license-apache-2-and-single-monorepo.md) calls licensing "the +most irreversible decision in the project." Same mechanism, asymmetric blast +radius. + +### What actually held the line before this record + +Not review, and not CODEOWNERS. Read from the live configuration on 2026-08-26 +rather than from memory: + +| Control | State as measured | +|---|---| +| `main` ruleset (`19149458`) | `deletion`, `non_fast_forward`, `required_status_checks` only — **no `pull_request` rule**, so no review is required | +| That ruleset's bypass | `RepositoryRole` 5 (admin), `bypass_mode: always` | +| Ruleset `19149448` | `copilot_code_review` — requests a review, cannot approve or block | +| `CODEOWNERS` | `/docs/adr/`, `/schema/`, `LICENSE`, `NOTICE`, `CONTRIBUTING.md` — **not** `/scripts/` or `/.github/workflows/` | +| Fork PR workflow approval | `first_time_contributors` | +| Default workflow permissions | `read`; `can_approve_pull_request_reviews: false` | +| Actions SHA pinning | **not** required | + +What remained was that merging requires write access, and that a diff disabling a +gate is visible to whoever merges. That is a real control, but it is *attention* +— and ADR-0016 Option B already argues, with evidence from its own drafting, that +attention is not the constraint. + +### The option #137 did not have + +The issue listed five options and judged required review the honest first move. +Its option 3 — workflows that run outside the pull request's control — was +dismissed as organization-only, and therefore unavailable to a deliberately +personal namespace (ADR-0006). + +That dismissal was correct about *organization required workflows* and wrong +about the capability. `pull_request_target` runs outside the pull request's +control on any repository, personal or not, and GitHub tightened it further on +2025-12-08: the workflow file, every referenced action, and the +`actions/checkout` commit are now taken from the repository's **default branch**, +regardless of the pull request's base branch. `GITHUB_REF` resolves to the +default branch and `GITHUB_SHA` to its tip. A pull request cannot edit what runs +under this trigger — only what it is run against. + +The reason this was not already in use is that `pull_request_target` is the most +commonly misused trigger in Actions: it carries the base repository's token and +secrets, so checking out pull-request code under it is the classic "pwn request." +That is an argument for using it carefully, not for not using it. `actions/checkout` +v7 — already pinned here — refuses fork pull request refs under this trigger by +default, which turns the most common form of that mistake into a build failure. + +### The half a trusted workflow does not close + +A required status check is matched by *name*. A pull request that cannot edit the +trusted job can still declare a job of its own with the same name and let the +later result stand. So moving the gate is necessary and not sufficient: the +`.github/workflows/` surface itself has to become something a pull request cannot +change quietly. + +## Decision + +**We will execute the gates whose verdict must not be editable by the pull +request they judge from the repository's default branch, and we will require an +explicit maintainer acknowledgment for any change to the surface that defines a +gate.** + +Concretely, in `.github/workflows/trusted-gates.yml`, on `pull_request_target`: + +1. **`trusted-dco`** runs `scripts/check-dco.ts` — `main`'s copy, from `main`'s + workflow — over the pull request's commits. The commits are fetched as git + *objects* and read with `git log`; they are never checked out and never + executed. The `dco` job in `ci.yml` is retained as a faster advisory report and + is explicitly no longer the authority. +2. **`gate-integrity`** blocks any pull request that changes + `.github/workflows/**`, `.github/actions/**`, `scripts/**`, `packages/ci/**`, + or `CODEOWNERS` unless the `gate-change-acknowledged` label is present. + Applying a label requires triage or write access, so an external contributor + cannot self-authorize, and the act is recorded in the timeline against whoever + performed it. + +Both jobs import Node builtins only and run with no `bun install`, so a hostile +or broken dependency graph cannot take them down — the property ADR-0006 action +item 2 already claimed for the sign-off gate, now also true of the job that runs +it. Both declare read-only `permissions`, use `persist-credentials: false`, and +pass untrusted values through `env:` rather than `${{ }}` inside `run:` bodies. + +Alongside, and recorded here because they are part of the same boundary: + +3. **`CODEOWNERS` names the gate-defining paths explicitly.** This adds no + coverage today, because the default `*` line already assigns them; it adds + explicitness and survives a future narrowing of that default. +4. **Actions must be pinned to a full-length commit SHA.** Enabled on the + repository on 2026-08-26 (`sha_pinning_required: true`, previously `false`). + The repository already pinned every action by SHA, so this costs nothing and + removes the ability to introduce a mutable tag — including inside a change + that has been acknowledged. + +### What we are explicitly not doing, and why + +**We are not adding a required-review rule.** This is a reversal of the issue's +preferred option, on measured grounds rather than preference. GitHub does not +permit an author to approve their own pull request. With a sole maintainer who is +also the sole code owner, `required_approving_review_count >= 1` — or +`require_code_owner_review`, which needs a code owner's approval and so implies +the same — deadlocks every self-authored change. The available escape is the +admin bypass that is already configured `always`, which converts the rule into a +bypass performed on every merge. For an external contributor the rule adds +nothing, because they cannot merge in the first place. + +A rule that is either a deadlock or a routine bypass is precisely the control +ADR-0016 warns about. Recording that finding is more useful than shipping the +rule. + +## Options considered + +### Option A: Trusted gates on `pull_request_target` plus an acknowledgment label (chosen) + +| Dimension | Assessment | +|---|---| +| Closes the mechanism | Yes for the gate's *definition*; the executed workflow and script come from the default branch | +| Closes name-shadowing | Yes, indirectly — every route runs through `.github/workflows/`, which `gate-integrity` blocks | +| Available to a personal namespace | Yes, unlike organization required workflows | +| Cost | One new workflow, one new script with tests, a label on gate-touching pull requests | +| New risk introduced | A privileged trigger. Mitigated by never executing pull-request code, read-only permissions, and `actions/checkout` v7's refusal of fork refs | +| Honest limit | Whoever can merge can label. Merge access remains the boundary | + +### Option B: Required review on the `main` ruleset, plus CODEOWNERS entries + +**Pros:** cheapest to configure; upgrades "whoever merges happens to look" to "a +review is required"; the issue's own preferred option. +**Cons:** not available as a real control here, for the reason given above — a +sole maintainer cannot approve their own pull request, so the rule either +deadlocks or is bypassed on every merge. It also leaves the mechanism untouched: +a reviewed pull request can still be the author of the check that certifies it. +The CODEOWNERS half is adopted anyway, as explicitness rather than as a gate. + +### Option C: A pinned external action for the gates that matter + +**Pros:** an immutable SHA cannot be edited by the pull request. +**Cons:** still invoked from a pull-request-controlled workflow, so it closes only +the script half — the same defect as reading the script from `origin/main`. It +also moves a governance check outside the surface +[ADR-0007](0007-adapter-isolation-and-public-surface-build.md) keeps mechanical +and self-contained, and ADR-0006 action item 2 already declined the DCO app on +that ground. Adopted in the weaker form that survives: SHA pinning is now +*required* repository-wide. + +### Option D: Organization-level required workflows + +**Pros:** runs genuinely outside the repository's control. +**Cons:** unavailable — the namespace is personal by decision (ADR-0006), and +moving it to solve this would be a far larger reversal than the problem warrants. +`pull_request_target` provides the trusted-execution property that made this +option attractive. + +### Option E: A gate asserting the gates are unmodified relative to base + +**Pros:** attacks name-shadowing directly. +**Cons:** the issue dismissed this as self-referential, and as a `pull_request` +job it would be — the assertion would live in the file it asserts about. Run from +`pull_request_target` it stops being self-referential, which is what makes +`gate-integrity` viable. The remaining objection, that it blocks every legitimate +change to a check, is answered by the acknowledgment rather than by an +exemption a pull request could set for itself. + +### Option F: Document the property and accept it + +**Pros:** honest; zero mechanism; merge access is genuinely the real boundary. +**Cons:** this is the status quo, and it leaves ADR-0006's provenance claim +resting on attention. It is also strictly weaker than Option A at a small cost +difference, now that a trusted execution path is known to exist. What it gets +right — that the residual must be stated rather than hidden — is kept: the +sections above say exactly what remains open. + +## Trade-offs + +Every pull request that touches a workflow, a script, `packages/ci/`, or +`CODEOWNERS` now needs a label before it can merge. That includes Dependabot's +`github_actions` bumps, which is friction on a real and recurring flow. It is +accepted rather than exempted: an action bump *is* a change to gate-executing +code, and `ci.yml` already argues that pinning exists to make such a bump "a +reviewed change rather than an ambient one." An author-based exemption would be +the same shape of hole this record exists to close. + +The label is also a self-authorization for whoever can merge. What it buys is not +authority but *shape*: a gate change stops being one line among two hundred and +becomes an explicit, attributed, timestamped act that blocks the merge until +performed. This record does not claim that makes gate changes tamper-proof, and +no wording anywhere in the implementation should. + +`pull_request_target` adds a privileged trigger to a repository that previously +had none on pull requests. That is a genuine new attack surface, mitigated but not +eliminated by the constraints listed under the decision. + +## Consequences + +- **Easier:** trusting the `trusted-dco` verdict; noticing a gate change, because + it fails loudly rather than reading as ordinary diff. +- **Harder:** changing a check, by one labelling step; landing a Dependabot action + bump, by the same step. +- **How we would know this was wrong:** if the label is applied reflexively — + visible as acknowledgments arriving in the same minute as the merge, with no + intervening comment — the mechanism has degraded into attention with extra + steps, and Option F becomes the honest position. Conversely, if a gate change + is ever caught at the label step, the mechanism paid for itself. +- **Revisit if:** the repository moves to an organization, which makes required + workflows available and makes `gate-integrity` redundant; or if GitHub ships + per-check provenance that makes name-shadowing impossible. + +## Action items + +1. [ ] **Ratify or reject.** This record is `proposed` and agent-drafted; it binds + nothing until a human accepts it. +2. [ ] **Add the trusted contexts to the `main` ruleset — after merge.** This + cannot be done before merge and must not be faked: `pull_request_target` + executes the workflow from the default branch, so `trusted-dco` and + `gate-integrity` do not exist until this lands, and adding a required + context that never reports would block every pull request including this + one. The exact operation, and the verification that it took, are in + [`docs/repository-trust-operations.md`](../repository-trust-operations.md). +3. [ ] **Observe both trusted jobs on a real pull request after merge**, per + ADR-0016. The kernel of `gate-integrity` has been observed blocking on this + change's own real changed-path list and passing once the label is applied, + and its three fail-quiet guards — empty list, truncated list, unreadable + payload — have each been observed firing. The *deployed workflow* has not + run, and cannot until it is on the default branch. Both halves are recorded + in `docs/repository-trust-operations.md` rather than asserted here. +4. [ ] **Decide on the fork-PR approval policy.** Currently + `first_time_contributors`. Tightening to `all_external_contributors` means + no fork's workflows run without a maintainer's explicit action, at the cost + of friction for repeat contributors. Left as a maintainer judgment rather + than changed silently; the command is in the operations document. +5. [x] **Enable required SHA pinning for actions.** Done 2026-08-26 — + `sha_pinning_required` moved `false` → `true`, verified by re-reading + `/repos/mbeacom/adrkit/actions/permissions`. +6. [x] **Name the gate-defining paths in `CODEOWNERS`**, with both caveats stated + in the file so the lines are not mistaken for a gate. +7. [x] **Drop the "known limitation" note in `scripts/check-dco.ts`** where it + stopped being true, and say precisely which invocation is the authority and + which is advisory — #137's third "done when". diff --git a/docs/repository-trust-operations.md b/docs/repository-trust-operations.md new file mode 100644 index 00000000..c1b16ef9 --- /dev/null +++ b/docs/repository-trust-operations.md @@ -0,0 +1,263 @@ +# Repository trust operations + +Companion to +[ADR-0035](adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md) +and issue [#137](https://github.com/mbeacom/adrkit/issues/137). + +This file exists so that the difference between **a control that is active** and +**a control that is planned** is written down rather than inferred. ADR-0016's +subject is checks that report success without having looked; a settings change +that is described but never applied has the same shape, and reads identically to +one that was. + +Everything below was read from the live repository on **2026-08-26** with `gh`. +Re-read rather than trusted: each claim carries the command that produced it. + +--- + +## 1. Active now + +### 1.1 Required SHA pinning for actions + +Changed as part of this work. + +```console +$ gh api repos/mbeacom/adrkit/actions/permissions +{"enabled":true,"allowed_actions":"all","sha_pinning_required":false} # before + +$ gh api -X PUT repos/mbeacom/adrkit/actions/permissions \ + -F enabled=true -f allowed_actions=all -F sha_pinning_required=true + +$ gh api repos/mbeacom/adrkit/actions/permissions +{"enabled":true,"allowed_actions":"all","sha_pinning_required":true} # after +``` + +Every action in this repository was already pinned by full SHA, so this changed +no workflow. What it removes is the ability to introduce a mutable tag later — +including inside a change that has been acknowledged under §1.3. + +Note the flag types: `gh api` sends `-f` values as strings, and the endpoint +rejects `"true"` for a boolean field. Booleans need `-F`. The first attempt here +failed with `For 'properties/enabled', "true" is not a boolean`. + +**Rollback:** the same `PUT` with `-F sha_pinning_required=false`. + +### 1.2 The `gate-change-acknowledged` label + +```console +$ gh label create gate-change-acknowledged --repo mbeacom/adrkit --color B60205 \ + --description "A maintainer has seen and accepted this PR's change to the CI gate surface (ADR-0035)" + +$ gh label list --repo mbeacom/adrkit --search gate +gate-change-acknowledged A maintainer has seen and accepted this PR's change to the CI gate surface (ADR-0035) #B60205 +``` + +Applying a label requires triage or write access, which is what makes it usable +as an authorization token for §1.3. It is **not** an approval and not a claim of +correctness — it asserts only that the change was seen. + +**Rollback:** `gh label delete gate-change-acknowledged --repo mbeacom/adrkit`. +Deleting it while `gate-integrity` is required would make every gate-touching +pull request unmergeable, so delete the required context first (§2.1). + +### 1.3 `CODEOWNERS` coverage of the gate-defining paths + +In the tree, effective on merge. Read the caveats in the file: with a single +owner and no `pull_request` rule on the `main` ruleset, these lines *request* a +review rather than requiring one. + +--- + +## 2. Must wait until merge + +### 2.1 Add `trusted-dco` and `gate-integrity` as required status checks + +**This cannot be done before merge, and doing it early would be actively +harmful.** `pull_request_target` executes the workflow from the repository's +default branch, so neither job exists until this change lands on `main`. A +required context that never reports leaves every pull request — including the one +introducing it — permanently "Expected — waiting for status". + +The payload below was constructed and validated against the live ruleset on +2026-08-26 and deliberately **not** sent. Re-run the first command after merge and +confirm the two new contexts appear before sending anything. + +```bash +# 1. Build the payload from the ruleset as it stands, appending the two contexts. +gh api repos/mbeacom/adrkit/rulesets/19149458 --jq ' + {rules: (.rules | map( + if .type == "required_status_checks" + then .parameters.required_status_checks += [ + {context: "trusted-dco"}, + {context: "gate-integrity"} + ] + else . end))}' > /tmp/ruleset-payload.json + +# 2. Read it before sending it. The existing nine contexts must still be present. +python3 -m json.tool < /tmp/ruleset-payload.json + +# 3. Apply. +gh api -X PUT repos/mbeacom/adrkit/rulesets/19149458 --input /tmp/ruleset-payload.json + +# 4. Verify it took, by reading back rather than by trusting the response. +gh api repos/mbeacom/adrkit/rulesets/19149458 \ + --jq '.rules[] | select(.type=="required_status_checks") + | .parameters.required_status_checks[].context' +``` + +Step 4 must list eleven contexts, ending with `trusted-dco` and `gate-integrity`. +Note that step 1 appends rather than replaces: a payload that omits the existing +nine would silently drop every current gate, and the response to a successful +`PUT` looks the same either way. + +Do not add `integration_id` to the two new entries. The existing `dco` and +`action-dogfood` contexts carry none, and pinning the integration is a separate +decision from adding the check. + +**Sequencing.** Add the contexts *before* removing anything. `dco` in `ci.yml` is +advisory under ADR-0035 but is still a required context; leave it required until +`trusted-dco` has been observed green on a real pull request (§3.2). + +### 2.2 Fork pull request approval policy — a maintainer decision, not a default + +Currently: + +```console +$ gh api repos/mbeacom/adrkit/actions/permissions/fork-pr-contributor-approval +{"approval_policy":"first_time_contributors"} +``` + +Tightening it means no external fork's workflows run without an explicit +maintainer action: + +```bash +gh api -X PUT repos/mbeacom/adrkit/actions/permissions/fork-pr-contributor-approval \ + -f approval_policy=all_external_contributors +``` + +Deliberately **not** applied. It trades real friction for repeat external +contributors against a repository that currently has few, and that trade is a +maintainer's call rather than an agent's. ADR-0035 action item 4 tracks it. + +### 2.3 Required review — recorded as declined, with the reason + +Not applied, and ADR-0035 explains why at length: GitHub does not permit an +author to approve their own pull request, so with a sole maintainer who is also +the sole code owner, `required_approving_review_count >= 1` deadlocks every +self-authored change, and the escape is the admin bypass that ruleset `19149458` +already grants `always`. The command is recorded so that the decision is +reversible by someone who disagrees, not because it is recommended: + +```bash +# NOT recommended — see ADR-0035, "What we are explicitly not doing, and why". +gh api -X PUT repos/mbeacom/adrkit/rulesets/19149458 --input - <<'JSON' +{"rules": [{"type": "pull_request", + "parameters": {"required_approving_review_count": 0, + "dismiss_stale_reviews_on_push": false, + "require_code_owner_review": false, + "require_last_push_approval": false, + "required_review_thread_resolution": true}}]} +JSON +``` + +That variant — count `0`, thread resolution `true` — is the only non-deadlocking +form. It would force every change through a pull request and require review +threads (including Copilot's) to be resolved before merge. It is a real +improvement to *attention* and no improvement at all to the mechanism #137 is +about, which is why it is filed here rather than in the decision. + +--- + +## 3. Evidence, per ADR-0016 + +### 3.1 Observed failing — the `gate-integrity` kernel + +A guard nobody has watched reject anything is an untested function that happens +to live in a test file. Observed on this change's **own real changed-path list**, +not on a fixture, through the same CLI the workflow invokes: + +```console +$ bun run scripts/check-gate-integrity.ts --files pr-files.json --labels pr-labels.json \ + --expected-files 2 +check-gate-integrity: examined 2 changed path(s) + gate scripts/check-gate-integrity.test.ts — scripts/: the checks themselves + gate scripts/check-gate-integrity.ts — scripts/: the checks themselves +check-gate-integrity: 2 of 2 changed path(s) alter the surface that defines this +repository's CI gates: + ... +exit=1 +``` + +and passing once the acknowledgment is present, with the change still *named* +rather than swallowed: + +```console +$ bun run scripts/check-gate-integrity.ts --files pr-files.json --labels ack.json +check-gate-integrity: examined 2 changed path(s) + gate scripts/check-gate-integrity.test.ts — scripts/: the checks themselves + gate scripts/check-gate-integrity.ts — scripts/: the checks themselves +check-gate-integrity: ok — 2 gate path(s) changed, acknowledged by the +"gate-change-acknowledged" label +exit=0 +``` + +The three fail-quiet guards were each observed firing, because this check's pass +condition is an absence and an absence is where blindness hides: + +| Input | Output | Exit | +|---|---|---| +| `[[]]` — empty list | `the pull request listed no changed files, which cannot happen` | 1 | +| 2 files read, `--expected-files 9` | `reports 9 changed file(s) but 2 were read; the listing is truncated or stale` | 1 | +| `{"message":"Not Found"}` | `expected a JSON array, got object` | 1 | + +The permanent negative cases live in `scripts/check-gate-integrity.test.ts`, +including a coverage assertion that fails if an entry is added to +`GATE_SURFACES` without a case observing it block. + +One defect was found this way rather than by review: `formatBlock` listed the +offending paths without the reason each was protected, which the test asserting +the block text caught. The code was changed, not the test. + +### 3.2 Not yet observed — say so plainly + +**The deployed workflow has never run.** It cannot, before merge: GitHub takes +`pull_request_target` workflows from the default branch, so `trusted-gates.yml` +is inert until it is on `main`. Nothing in this repository should be read as +claiming otherwise. + +After merge, the first pull request that touches a gate path exercises both +halves. To observe it deliberately rather than waiting: + +```bash +# On a scratch branch, touch a gate path and open a pull request. +printf '\n' >> scripts/check-gate-integrity.ts +git commit -sam 'chore: observe gate-integrity blocking' && git push -u origin HEAD +gh pr create --fill + +# Expect gate-integrity RED. Then acknowledge, and expect it to turn GREEN. +gh pr edit --add-label gate-change-acknowledged + +# Read the conclusions back rather than reading the checks tab. +gh api "repos/mbeacom/adrkit/commits/$(git rev-parse HEAD)/check-runs" \ + --jq '.check_runs[] | select(.name|test("^(trusted-dco|gate-integrity)$")) + | {name, status, conclusion}' +``` + +`trusted-dco` should be green throughout — the commit above is signed off. To +observe *it* failing, add a commit with `--no-signoff` on the same branch and +confirm `trusted-dco` goes red while the advisory `dco` job's verdict is +irrelevant to the merge. + +Until both have been seen red and then green on a real pull request, ADR-0035 +action item 3 stays open and the workflow counts as implemented, not as verified. + +--- + +## 4. What none of this closes + +Whoever can merge can change a gate and acknowledge the change. Merge access is +the boundary it always was, and no wording in this repository should suggest +these controls are tamper-proof. + +What is closed is narrower, and is the thing #137 was actually about: the check +that certifies a pull request is no longer authored by that pull request. diff --git a/package.json b/package.json index 23d2ab19..6b850ae4 100644 --- a/package.json +++ b/package.json @@ -38,6 +38,7 @@ "check:doc-pins": "bun run scripts/check-doc-cli-versions.ts", "check:site-grammar": "bun run scripts/check-site-corpus-grammar.ts", "check:dco": "bun run scripts/check-dco.ts", + "check:gate-integrity": "bun run scripts/check-gate-integrity.ts", "check:ci-comment": "bun run scripts/check-ci-comment.ts", "audit:gate": "bun run scripts/audit-gate.ts", "adr": "bun packages/cli/src/index.ts", diff --git a/scripts/check-dco.ts b/scripts/check-dco.ts index 6cee9633..53d299ca 100644 --- a/scripts/check-dco.ts +++ b/scripts/check-dco.ts @@ -18,15 +18,28 @@ * `range` is any two-dot git revision range and defaults to * `origin/main..HEAD`. CI passes the pull request's own range explicitly. * - * **Known limitation (#137).** This runs from the pull request's own merge - * checkout, so a change that edits this file — or the workflow step invoking it — - * can produce a green `dco` status over unsigned commits. That is a property of - * every gate in this repository, not of this one: `pull_request` workflows - * execute the *pull request's* `ci.yml`, measured directly on #98. Moving this - * script to a trusted base revision would not close it, because the step calling - * it is equally under the pull request's control, and a control that looks like a - * control and is not one is worse than a documented gap (ADR-0016). Tracked - * repository-wide in #137 rather than papered over here. + * **Where this runs, and why it matters (#137, ADR-0035).** There are two + * invocations of this script, and only one of them is the gate. + * + * The authority is the `trusted-dco` job in `.github/workflows/trusted-gates.yml`. + * It is triggered by `pull_request_target`, which GitHub executes from the + * repository's **default branch** — workflow file, referenced actions, and + * `actions/checkout` commit alike — so neither this script nor the step invoking + * it is under the pull request's control. It reads the pull request's commits as + * fetched objects and never checks them out. + * + * The `dco` job in `ci.yml` is the same check run from the pull request's own + * checkout. It is faster and reports first, and it can be neutered by the pull + * request it judges — so it is advisory, and can only ever fail open. Keeping it + * costs nothing because the trusted job must also pass; removing the trusted job + * is what would matter, and that is a change under `.github/workflows/`, which + * `gate-integrity` blocks without an explicit maintainer acknowledgment. + * + * What remains open is stated plainly rather than papered over: whoever can merge + * can change a gate and acknowledge the change. Merge access is the boundary it + * always was. What is closed is the narrower property that made #137 worth + * filing — that the check certifying a pull request could be authored by that + * same pull request. */ import { execFileSync } from 'node:child_process'; diff --git a/scripts/check-gate-integrity.test.ts b/scripts/check-gate-integrity.test.ts new file mode 100644 index 00000000..56b7ecf6 --- /dev/null +++ b/scripts/check-gate-integrity.test.ts @@ -0,0 +1,239 @@ +/** + * Checks on the gate-integrity guard (#137). + * + * Three things need proving, and they are different: + * + * 1. **The guard fires** on each protected surface. A guard nobody has watched + * reject anything is an untested function that happens to live in a test file + * (ADR-0016), so every entry in `GATE_SURFACES` carries a case that blocks. + * 2. **The guard stays silent on ordinary work.** This is the failure mode that + * would actually bite: a guard that fires on every pull request gets its label + * applied reflexively, and a reflexive acknowledgment is not one. + * 3. **The guard refuses to pass over an input it could not read.** Its pass + * condition is "no gate path in this list", which is indistinguishable from + * "the list is empty because the listing failed" unless something separates + * them. Those cases are the reason the check is worth anything, so they are + * asserted directly rather than left to the shape of the code. + * + * The coverage assertion at the end is deliberate: it fails if a surface is + * added to `GATE_SURFACES` without a case that observes it blocking, which is + * clause 2 of ADR-0016 applied to the guard's own growth. + */ + +import { describe, expect, test } from 'bun:test'; +import { + DEFAULT_ACK_LABEL, + GATE_SURFACES, + classifyGateChanges, + flattenPages, + formatBlock, + formatReport, + normalizePath, + parseArgs, + pluck, + surfaceOf, +} from './check-gate-integrity.ts'; + +/** One changed path per protected surface, in the shape a real pull request produces. */ +const BLOCKING_CASES: ReadonlyArray<{ path: string; pattern: string }> = [ + { path: '.github/workflows/ci.yml', pattern: '.github/workflows/' }, + { path: '.github/workflows/trusted-gates.yml', pattern: '.github/workflows/' }, + { path: '.github/actions/setup/action.yml', pattern: '.github/actions/' }, + { path: 'scripts/check-dco.ts', pattern: 'scripts/' }, + { path: 'packages/ci/dist/index.js', pattern: 'packages/ci/' }, + { path: 'CODEOWNERS', pattern: 'CODEOWNERS' }, +]; + +/** Paths an ordinary change touches. None of these may block. */ +const ORDINARY_PATHS = [ + 'packages/core/src/graph/build.ts', + 'packages/cli/src/index.ts', + 'docs/adr/0035-execute-the-gates.md', + 'README.md', + 'package.json', + 'bun.lock', + 'site/src/content/docs/cli.md', + 'packages/adapters/spec-kit/extension.yml', + // Near misses, on purpose: a prefix match on a *string* rather than a path + // boundary would swallow these, and they are legitimate files. + 'packages/cli-extras/src/index.ts', + 'docs/scripts-overview.md', + 'CODEOWNERS.md', +]; + +describe('the guard fires on every protected surface', () => { + for (const { path, pattern } of BLOCKING_CASES) { + test(`${path} blocks without the acknowledgment`, () => { + const report = classifyGateChanges([path], []); + expect(report.verdict).toBe('blocked'); + expect(report.changes).toHaveLength(1); + expect(report.changes[0]?.path).toBe(path); + expect(report.changes[0]?.surface.pattern).toBe(pattern); + }); + } + + test('one gate path among many ordinary ones still blocks', () => { + const report = classifyGateChanges([...ORDINARY_PATHS, 'scripts/check-dco.ts'], []); + expect(report.verdict).toBe('blocked'); + // The specific observed value, not just the count: a report naming the wrong + // path would satisfy `changes.length === 1` and tell the reader nothing. + expect(report.changes.map((change) => change.path)).toEqual(['scripts/check-dco.ts']); + expect(report.examined).toBe(ORDINARY_PATHS.length + 1); + }); + + test('the block names the path, the reason, and how to acknowledge it', () => { + const text = formatBlock(classifyGateChanges(['.github/workflows/ci.yml'], [])); + expect(text).toContain('.github/workflows/ci.yml'); + expect(text).toContain('defines which checks run'); + expect(text).toContain(DEFAULT_ACK_LABEL); + expect(text).toContain('gh pr edit'); + }); + + test('matching is case-insensitive, which can only add a match', () => { + expect(surfaceOf('Scripts/Check-Dco.ts')?.pattern).toBe('scripts/'); + expect(surfaceOf('.GitHub/Workflows/ci.yml')?.pattern).toBe('.github/workflows/'); + expect(surfaceOf('codeowners')?.pattern).toBe('CODEOWNERS'); + }); + + test('a leading ./ and backslashes normalize rather than escaping the match', () => { + expect(normalizePath('./scripts/x.ts')).toBe('scripts/x.ts'); + expect(surfaceOf('./scripts/check-dco.ts')?.pattern).toBe('scripts/'); + expect(surfaceOf('scripts\\check-dco.ts')?.pattern).toBe('scripts/'); + }); +}); + +describe('the guard stays silent on ordinary work', () => { + test('an ordinary change is clean, and says how much it looked at', () => { + const report = classifyGateChanges(ORDINARY_PATHS, []); + expect(report.verdict).toBe('clean'); + expect(report.changes).toEqual([]); + expect(report.examined).toBe(ORDINARY_PATHS.length); + expect(formatReport(report)).toContain(`examined ${ORDINARY_PATHS.length} changed path(s)`); + }); + + for (const path of ORDINARY_PATHS) { + test(`${path} does not match a gate surface`, () => { + expect(surfaceOf(path)).toBeUndefined(); + }); + } + + test('a prefix matches at a path boundary, not as a bare substring', () => { + // `packages/cli-extras/` starts with neither `packages/ci/` nor `scripts/`, + // but a naive `includes` or a prefix without the trailing slash would claim + // otherwise — and a guard that fires here would be silenced by deletion. + expect(surfaceOf('packages/cli-extras/src/index.ts')).toBeUndefined(); + expect(surfaceOf('packages/cifs/x.ts')).toBeUndefined(); + }); +}); + +describe('the acknowledgment', () => { + test('a gate change passes once the label is present', () => { + const report = classifyGateChanges(['scripts/check-dco.ts'], [DEFAULT_ACK_LABEL]); + expect(report.verdict).toBe('acknowledged'); + expect(report.acknowledged).toBe(true); + // Still reported, not swallowed: acknowledged is not the same as clean, and + // the log has to say which one happened. + expect(report.changes).toHaveLength(1); + expect(formatReport(report)).toContain('acknowledged by the'); + }); + + test('label matching ignores case and surrounding whitespace', () => { + expect( + classifyGateChanges(['scripts/x.ts'], [' Gate-Change-Acknowledged ']).verdict, + ).toBe('acknowledged'); + }); + + test('an unrelated label does not acknowledge anything', () => { + const report = classifyGateChanges(['scripts/x.ts'], ['enhancement', 'github_actions']); + expect(report.verdict).toBe('blocked'); + }); + + test('a label that merely contains the token does not acknowledge', () => { + expect(classifyGateChanges(['scripts/x.ts'], ['not-gate-change-acknowledged']).verdict).toBe( + 'blocked', + ); + }); + + test('the acknowledgment does not make an ordinary change report as acknowledged', () => { + // The verdict has to distinguish "nothing matched" from "something matched + // and was waved through", or a stale label would rewrite the log of a clean run. + expect(classifyGateChanges(['README.md'], [DEFAULT_ACK_LABEL]).verdict).toBe('clean'); + }); +}); + +describe('the guard refuses to pass over an input it could not read', () => { + test('a non-array payload throws rather than yielding zero paths', () => { + expect(() => flattenPages({ message: 'Not Found' })).toThrow(/expected a JSON array/); + expect(() => flattenPages(null)).toThrow(/expected a JSON array/); + }); + + test('paginated and unpaginated shapes both flatten to the same files', () => { + const paged = [[{ filename: 'a.ts' }], [{ filename: 'b.ts' }]]; + const flat = [{ filename: 'a.ts' }, { filename: 'b.ts' }]; + expect(pluck(flattenPages(paged), 'filename')).toEqual(['a.ts', 'b.ts']); + expect(pluck(flattenPages(flat), 'filename')).toEqual(['a.ts', 'b.ts']); + }); + + test('an entry missing the field throws rather than silently dropping', () => { + expect(() => pluck([{ filename: 'a.ts' }, { sha: 'deadbeef' }], 'filename')).toThrow( + /entry 1 has no string "filename"/, + ); + }); + + test('parseArgs rejects a flag with no value and an unknown flag', () => { + expect(() => parseArgs(['--files'])).toThrow(/--files needs a value/); + expect(() => parseArgs(['--files', 'f.json', '--nope'])).toThrow(/unrecognized argument/); + expect(() => parseArgs(['--labels', 'l.json'])).toThrow(/--files is required/); + }); + + test('parseArgs rejects a non-integer expected count', () => { + expect(() => parseArgs(['--files', 'f', '--labels', 'l', '--expected-files', 'lots'])).toThrow( + /non-negative integer/, + ); + }); + + test('parseArgs accepts the shape the trusted workflow passes', () => { + const options = parseArgs([ + '--files', + 'pr-files.json', + '--labels', + 'pr-labels.json', + '--expected-files', + '12', + ]); + expect(options).toEqual({ + files: 'pr-files.json', + labels: 'pr-labels.json', + expectedFiles: 12, + ackLabel: DEFAULT_ACK_LABEL, + }); + }); +}); + +describe('the surface list carries its own coverage', () => { + test('every protected surface has a case that was observed blocking', () => { + const covered = new Set(BLOCKING_CASES.map((testCase) => testCase.pattern)); + const declared = GATE_SURFACES.map((surface) => surface.pattern); + // Fails when a surface is added without a negative case, which is ADR-0016 + // clause 2 applied to this guard's own growth rather than to a one-off run. + expect(declared.filter((pattern) => !covered.has(pattern))).toEqual([]); + }); + + test('every surface states why a change there matters', () => { + for (const surface of GATE_SURFACES) { + expect(surface.why.length).toBeGreaterThan(0); + } + }); + + test('this repository really contains each protected surface', () => { + // A pattern that matches nothing in the tree is a guard watching a door that + // is not there. Asserted against real paths rather than against the list itself. + const real = [ + '.github/workflows/ci.yml', + 'scripts/check-gate-integrity.ts', + 'packages/ci/action.yml', + 'CODEOWNERS', + ]; + for (const path of real) expect(surfaceOf(path)).toBeDefined(); + }); +}); diff --git a/scripts/check-gate-integrity.ts b/scripts/check-gate-integrity.ts new file mode 100644 index 00000000..7a7cd84c --- /dev/null +++ b/scripts/check-gate-integrity.ts @@ -0,0 +1,356 @@ +/** + * Fail when a pull request changes the surface that *defines* this repository's + * CI gates, unless a maintainer has explicitly acknowledged the change. + * + * ## Why this exists at all + * + * Every gate in `.github/workflows/ci.yml` is executed from the pull request's + * own checkout, so the pull request can edit both the check and the step that + * invokes it and still produce a green required status (#137, measured on #98). + * [ADR-0035](../docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md) + * moves the checks that matter onto `pull_request_target`, which GitHub executes + * from the repository's **default branch** — workflow file, referenced actions, + * and `actions/checkout` commit alike. That closes the "edit the check" half. + * + * It does not close the other half. A required status check is matched by + * *name*, so a pull request that cannot edit the trusted job can still add a + * job of its own with the same name and let the later result stand. Every route + * to that shadow runs through a change under `.github/workflows/`, which is what + * this guard watches. + * + * ## What the acknowledgment is, and what it is not + * + * The token is a label, because applying one requires triage or write access on + * this repository. An external contributor therefore cannot self-authorize; a + * maintainer can, exactly as a maintainer can already merge. + * + * **This is not a claim that gate changes are tamper-proof.** Whoever can merge + * can label. What changes is the *shape* of the failure: a gate change stops + * being one diff line among two hundred and becomes an explicit, attributed, + * timestamped act that blocks the merge until it is performed. ADR-0016 argues + * that attention is not the constraint, and this guard agrees with it — it does + * not ask anyone to look harder, it refuses to proceed. + * + * ## Dependencies, deliberately none + * + * Imports only Node builtins, like `check-dco.ts` and for the same reason: the + * trusted workflow runs it with no `bun install`, so a broken or hostile + * dependency graph cannot take the gate-integrity gate down with it. Nothing + * here reads the repository tree either — the inputs are the pull request's + * changed-path list and its labels, both supplied as JSON by the caller. + * + * bun run scripts/check-gate-integrity.ts \ + * --files --labels \ + * [--expected-files ] [--ack-label ] + */ + +import { readFileSync } from 'node:fs'; + +/** The label whose presence acknowledges a change to the gate-defining surface. */ +export const DEFAULT_ACK_LABEL = 'gate-change-acknowledged'; + +export interface GateSurface { + /** Matched case-insensitively against a repository-relative POSIX path. */ + readonly pattern: string; + /** `prefix` matches a directory subtree; `exact` matches one file. */ + readonly kind: 'prefix' | 'exact'; + /** Why a change here can alter what CI certifies. Printed on a block. */ + readonly why: string; +} + +/** + * The surface a change to which can alter what CI certifies. + * + * Deliberately narrow. Every entry is here because editing it changes what a + * gate *does*, not merely what it runs over — a broad list would fire on + * ordinary work, and a guard that fires constantly is one that gets labelled + * reflexively, which is the same failure as not having it. + * + * `package.json` is a near miss and is left out on purpose. Repointing a + * `check:*` script is a real neutering vector, but only for the advisory copies + * in `ci.yml`: the trusted workflow invokes script paths directly rather than + * through `bun run`, so no manifest edit can redirect it. Including it would put + * this label on every dependency bump and every version bump, and the protection + * bought would be over checks that are already not authoritative. + */ +export const GATE_SURFACES: readonly GateSurface[] = [ + { + pattern: '.github/workflows/', + kind: 'prefix', + why: 'defines which checks run, what they invoke, and what they are named', + }, + { + pattern: '.github/actions/', + kind: 'prefix', + why: 'repository-local actions execute inside those checks', + }, + { + pattern: 'scripts/', + kind: 'prefix', + why: 'the checks themselves', + }, + { + pattern: 'packages/ci/', + kind: 'prefix', + why: 'the published governing-decisions and queue Actions, which are gates', + }, + { + pattern: 'CODEOWNERS', + kind: 'exact', + why: 'decides who is asked to review a change to any of the above', + }, +]; + +export interface GateChange { + readonly path: string; + readonly surface: GateSurface; +} + +export type GateVerdict = 'clean' | 'acknowledged' | 'blocked'; + +export interface GateIntegrityReport { + /** Every changed path considered, whether or not it matched. Always reported. */ + readonly examined: number; + readonly changes: readonly GateChange[]; + readonly acknowledged: boolean; + readonly ackLabel: string; + readonly verdict: GateVerdict; +} + +/** + * Pure: normalize a path for matching. + * + * Lowercased, because matching case-insensitively can only ever *add* a match. + * That is the fail-closed direction: `Scripts/check-dco.ts` and + * `scripts/check-dco.ts` are distinct to git but the same file to a + * case-insensitive checkout, and a guard that missed one would be silent about + * the change that mattered. + * + * A leading `./` is stripped and backslashes are folded to `/`. GitHub's API + * emits neither, so both are belt rather than braces — but a normalizer that + * accepts only the shape it expects fails open on the shape it does not. + */ +export function normalizePath(path: string): string { + return path.trim().replace(/\\/g, '/').replace(/^\.\//, '').toLowerCase(); +} + +/** Pure: the surface `path` belongs to, or `undefined`. */ +export function surfaceOf(path: string): GateSurface | undefined { + const normalized = normalizePath(path); + return GATE_SURFACES.find((surface) => + surface.kind === 'prefix' + ? normalized.startsWith(normalizePath(surface.pattern)) + : normalized === normalizePath(surface.pattern), + ); +} + +/** + * Pure: classify a pull request's changed paths against the gate surface. + * + * `blocked` requires *both* that a gate path changed and that the acknowledgment + * is absent, so the two inputs are reported separately rather than folded into + * the verdict — a reader has to be able to tell "nothing matched" from + * "something matched and was acknowledged", and a bare boolean cannot. + */ +export function classifyGateChanges( + paths: readonly string[], + labels: readonly string[], + ackLabel: string = DEFAULT_ACK_LABEL, +): GateIntegrityReport { + const changes: GateChange[] = []; + for (const path of paths) { + const surface = surfaceOf(path); + if (surface) changes.push({ path, surface }); + } + + const wanted = ackLabel.trim().toLowerCase(); + const acknowledged = labels.some((label) => label.trim().toLowerCase() === wanted); + + const verdict: GateVerdict = + changes.length === 0 ? 'clean' : acknowledged ? 'acknowledged' : 'blocked'; + + return { examined: paths.length, changes, acknowledged, ackLabel, verdict }; +} + +/** + * Pure: the report as text. + * + * States the examined count in every branch, including the clean one. ADR-0016's + * complementary half: "looked at 41 paths, none under a gate surface" and "could + * not see the changed paths at all" are the same sentence unless the check says + * what it looked at, and the second is the one that matters here. + */ +export function formatReport(report: GateIntegrityReport): string { + const lines = [`check-gate-integrity: examined ${report.examined} changed path(s)`]; + + for (const change of report.changes) { + lines.push(` gate ${change.path} — ${change.surface.pattern}: ${change.surface.why}`); + } + + if (report.verdict === 'clean') { + lines.push('check-gate-integrity: ok — no path under a gate-defining surface'); + } else if (report.verdict === 'acknowledged') { + lines.push( + `check-gate-integrity: ok — ${report.changes.length} gate path(s) changed, ` + + `acknowledged by the "${report.ackLabel}" label`, + ); + } + + return lines.join('\n'); +} + +/** Pure: the failure text for a blocked report. Empty string when not blocked. */ +export function formatBlock(report: GateIntegrityReport): string { + if (report.verdict !== 'blocked') return ''; + // Each path carries its surface's reason. A bare list of filenames tells the + // reader that something tripped without telling them what the guard thinks the + // file *is*, which is the difference between acting on the block and labelling + // past it. + const listed = report.changes + .map((change) => ` ${change.path}\n ${change.surface.pattern} — ${change.surface.why}`) + .join('\n'); + return ( + `${report.changes.length} of ${report.examined} changed path(s) alter the surface that ` + + `defines this repository's CI gates:\n\n${listed}\n\n` + + `A change here can alter what every other check certifies, so it needs an explicit\n` + + `acknowledgment rather than a quiet diff line. A maintainer applies the\n` + + `"${report.ackLabel}" label to this pull request, which requires triage or write\n` + + `access and is recorded in the timeline against whoever applied it.\n\n` + + ` gh pr edit --add-label "${report.ackLabel}"\n\n` + + `This is not an assertion that the change is correct — it is an assertion that it was\n` + + `seen. See docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md.` + ); +} + +/** + * Pure: flatten what `gh api --paginate --slurp` produces. + * + * `--slurp` wraps each *page* in the outer array, so the shape is `Page[]` and + * not `File[]`; without `--paginate` it is `File[]` directly. Both are accepted + * because a script that understands only one of them silently reads zero files + * from the other, and zero files is this check's pass condition. + */ +export function flattenPages(parsed: unknown): unknown[] { + if (!Array.isArray(parsed)) { + throw new Error(`expected a JSON array, got ${parsed === null ? 'null' : typeof parsed}`); + } + const flat: unknown[] = []; + for (const entry of parsed) { + if (Array.isArray(entry)) flat.push(...entry); + else flat.push(entry); + } + return flat; +} + +/** Pure: read a string field off every entry, rejecting an entry that lacks it. */ +export function pluck(entries: readonly unknown[], field: string): string[] { + return entries.map((entry, index) => { + const value = (entry as Record | null)?.[field]; + if (typeof value !== 'string') { + throw new Error( + `entry ${index} has no string "${field}"; refusing to check a partially-parsed list`, + ); + } + return value; + }); +} + +function readJson(path: string): unknown { + let raw: string; + try { + raw = readFileSync(path, 'utf8'); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(`could not read ${path}, so nothing was examined. ${detail}`); + } + try { + return JSON.parse(raw); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(`${path} is not valid JSON, so nothing was examined. ${detail}`); + } +} + +interface Options { + files: string; + labels: string; + expectedFiles?: number; + ackLabel: string; +} + +export function parseArgs(argv: readonly string[]): Options { + const options: Partial = { ackLabel: DEFAULT_ACK_LABEL }; + for (let i = 0; i < argv.length; i += 1) { + const flag = argv[i]; + const value = argv[i + 1]; + switch (flag) { + case '--files': + case '--labels': + case '--ack-label': + case '--expected-files': { + if (value === undefined) throw new Error(`${flag} needs a value`); + i += 1; + if (flag === '--files') options.files = value; + else if (flag === '--labels') options.labels = value; + else if (flag === '--ack-label') options.ackLabel = value; + else { + const parsed = Number(value); + if (!Number.isInteger(parsed) || parsed < 0) { + throw new Error(`--expected-files needs a non-negative integer, got "${value}"`); + } + options.expectedFiles = parsed; + } + break; + } + default: + throw new Error(`unrecognized argument "${flag}"`); + } + } + if (!options.files) throw new Error('--files is required'); + if (!options.labels) throw new Error('--labels is required'); + return options as Options; +} + +function main(argv: readonly string[]): void { + const options = parseArgs(argv); + + const paths = pluck(flattenPages(readJson(options.files)), 'filename'); + const labels = pluck(flattenPages(readJson(options.labels)), 'name'); + + // A pull request always changes at least one file, so an empty list means the + // listing failed rather than that nothing was touched. Reporting "0 paths, ok" + // here renders identically to a clean run — the exact fail-quiet shape ADR-0016 + // exists to prevent, and the one that would make this guard useless in the only + // case it is for. + if (paths.length === 0) { + throw new Error( + 'the pull request listed no changed files, which cannot happen. ' + + 'Refusing to report a pass over an empty list.', + ); + } + + // The files endpoint caps at 3000 entries and says so by truncating, not by + // erroring. A gate path past the cap would be invisible, so the count the pull + // request itself reports is compared against the count actually read. + if (options.expectedFiles !== undefined && options.expectedFiles !== paths.length) { + throw new Error( + `the pull request reports ${options.expectedFiles} changed file(s) but ${paths.length} ` + + `were read; the listing is truncated or stale. Refusing to report a pass over a ` + + `partial list.`, + ); + } + + const report = classifyGateChanges(paths, labels, options.ackLabel); + console.log(formatReport(report)); + + if (report.verdict === 'blocked') throw new Error(formatBlock(report)); +} + +if (import.meta.main) { + try { + main(process.argv.slice(2)); + } catch (error) { + console.error(`check-gate-integrity: ${error instanceof Error ? error.message : String(error)}`); + process.exit(1); + } +} From c1bde97e9b0fd009efb65b793b81178c9db83cd5 Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 26 Aug 2026 17:53:55 -0400 Subject: [PATCH 02/10] fix(ci): read a rename's previous path, closing the gate-integrity bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by reading what GitHub's files endpoint returns rather than assuming it. A rename reports `filename` as the new path only, with the old one in `previous_filename`. So a pull request that moved .github/workflows/trusted-gates.yml to .github/wf/trusted-gates.yml presented the guard with a path matching nothing, passed clean, and would have deleted the trusted gate on merge. A deletion was never affected — `filename` is the deleted path — which is exactly what made the gap easy to miss. Both ends of a rename are now read, which can only ever add a path. The entry count and the path count are kept apart: --expected-files is compared against the entry count, because a rename contributes two paths and comparing the wrong one would fail every renaming pull request as "truncated", which is the merge-stopping direction. Observed blocking end-to-end through the CLI on a rename payload, with the permanent negative cases kept alongside a case asserting the new path alone would have evaded the matcher, so the test documents what it defends. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- CHANGELOG.md | 3 + ...-a-pull-request-from-the-default-branch.md | 12 ++-- docs/repository-trust-operations.md | 24 +++++++ scripts/check-gate-integrity.test.ts | 63 +++++++++++++++++++ scripts/check-gate-integrity.ts | 45 +++++++++++-- 5 files changed, 138 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07fb8df2..1f07a682 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,9 @@ Until `1.0.0`, minor releases may include breaking changes or over a payload it could not parse — each of those observed firing before the check counted as coverage ([ADR-0016](docs/adr/0016-require-every-check-to-be-observed-failing-before-it-counts-as-coverage.md)). + It reads a rename's `previous_filename` as well as its `filename`, because the + files endpoint reports only the new path and a rename out of a protected prefix + would otherwise have passed clean and deleted the gate on merge. - **[`docs/repository-trust-operations.md`](docs/repository-trust-operations.md)**, separating the controls that are active from the ones that cannot be applied diff --git a/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md b/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md index 0936c7e5..d86c7b8c 100644 --- a/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md +++ b/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md @@ -295,11 +295,13 @@ eliminated by the constraints listed under the decision. [`docs/repository-trust-operations.md`](../repository-trust-operations.md). 3. [ ] **Observe both trusted jobs on a real pull request after merge**, per ADR-0016. The kernel of `gate-integrity` has been observed blocking on this - change's own real changed-path list and passing once the label is applied, - and its three fail-quiet guards — empty list, truncated list, unreadable - payload — have each been observed firing. The *deployed workflow* has not - run, and cannot until it is on the default branch. Both halves are recorded - in `docs/repository-trust-operations.md` rather than asserted here. + change's own real changed-path list and passing once the label is applied; + its three fail-quiet guards — empty list, truncated list, unreadable + payload — have each been observed firing; and a rename that carried a gate + path out of the protected prefix has been observed blocking after that + bypass was found and closed. The *deployed workflow* has not run, and + cannot until it is on the default branch. All of it is recorded in + `docs/repository-trust-operations.md` rather than asserted here. 4. [ ] **Decide on the fork-PR approval policy.** Currently `first_time_contributors`. Tightening to `all_external_contributors` means no fork's workflows run without a maintainer's explicit action, at the cost diff --git a/docs/repository-trust-operations.md b/docs/repository-trust-operations.md index c1b16ef9..f09c2cdb 100644 --- a/docs/repository-trust-operations.md +++ b/docs/repository-trust-operations.md @@ -218,6 +218,30 @@ One defect was found this way rather than by review: `formatBlock` listed the offending paths without the reason each was protected, which the test asserting the block text caught. The code was changed, not the test. +A second, and the more serious of the two, was found by reading what GitHub's +files endpoint actually returns rather than assuming it. A **rename** reports +`filename` as the *new* path only, with the old one in `previous_filename`. So a +pull request that moved `.github/workflows/trusted-gates.yml` to +`.github/wf/trusted-gates.yml` presented this check with a path matching nothing, +passed clean, and would have deleted the trusted gate on merge. A *deletion* is +not affected — `filename` is the deleted path — which is exactly what made the +gap easy to miss. Both paths are now read; observed end-to-end through the CLI: + +```console +$ bun run scripts/check-gate-integrity.ts --files rename.json --labels none.json \ + --expected-files 2 +check-gate-integrity: examined 3 changed path(s) + gate .github/workflows/trusted-gates.yml — .github/workflows/: defines which + checks run, what they invoke, and what they are named +check-gate-integrity: 1 of 3 changed path(s) alter the surface ... +exit=1 +``` + +Note the two counts in that output. `--expected-files` is compared against the +*entry* count, which is 2; the path count is 3, because a rename contributes both +ends. Comparing the wrong one would have made every renaming pull request fail as +"truncated" — a false block, which is the merge-stopping direction. + ### 3.2 Not yet observed — say so plainly **The deployed workflow has never run.** It cannot, before merge: GitHub takes diff --git a/scripts/check-gate-integrity.test.ts b/scripts/check-gate-integrity.test.ts index 56b7ecf6..7ee8021d 100644 --- a/scripts/check-gate-integrity.test.ts +++ b/scripts/check-gate-integrity.test.ts @@ -24,6 +24,7 @@ import { describe, expect, test } from 'bun:test'; import { DEFAULT_ACK_LABEL, GATE_SURFACES, + changedPaths, classifyGateChanges, flattenPages, formatBlock, @@ -210,6 +211,68 @@ describe('the guard refuses to pass over an input it could not read', () => { }); }); +describe('a rename cannot carry a gate path out of sight', () => { + // The guard's one real bypass, found by reading what the GitHub files endpoint + // actually returns rather than by assuming. A rename reports `filename` as the + // *new* path only; the old one lives in `previous_filename`. Moving the trusted + // workflow out of `.github/workflows/` therefore presented a path matching + // nothing, passed clean, and deleted the gate on merge. + const renameAway = [ + { + filename: '.github/wf/trusted-gates.yml', + previous_filename: '.github/workflows/trusted-gates.yml', + status: 'renamed', + }, + ]; + + test('the new path alone would have evaded the matcher', () => { + // Stated explicitly so the case documents what it is defending, and fails + // loudly if `.github/wf/` ever becomes a protected prefix for other reasons. + expect(surfaceOf('.github/wf/trusted-gates.yml')).toBeUndefined(); + }); + + test('the old path is read too, so the rename blocks', () => { + const report = classifyGateChanges(changedPaths(renameAway), []); + expect(report.verdict).toBe('blocked'); + expect(report.changes.map((change) => change.path)).toEqual([ + '.github/workflows/trusted-gates.yml', + ]); + }); + + test('a rename *into* a gate path blocks on the new path', () => { + const renameInto = [ + { filename: 'scripts/check-dco.ts', previous_filename: 'tmp/x.ts', status: 'renamed' }, + ]; + expect(classifyGateChanges(changedPaths(renameInto), []).verdict).toBe('blocked'); + }); + + test('a deletion was never affected — filename is the deleted path', () => { + const deletion = [{ filename: '.github/workflows/ci.yml', status: 'removed' }]; + expect(classifyGateChanges(changedPaths(deletion), []).verdict).toBe('blocked'); + }); + + test('an ordinary rename outside the gate surface stays clean', () => { + const ordinary = [ + { filename: 'packages/core/src/b.ts', previous_filename: 'packages/core/src/a.ts' }, + ]; + expect(classifyGateChanges(changedPaths(ordinary), []).verdict).toBe('clean'); + }); + + test('an entry with no previous_filename contributes exactly one path', () => { + // The count matters: `--expected-files` is compared against the *entry* count, + // and a normalizer that invented a path per entry would make every ordinary + // pull request report as truncated. + expect(changedPaths([{ filename: 'a.ts' }, { filename: 'b.ts' }])).toEqual(['a.ts', 'b.ts']); + expect(changedPaths([{ filename: 'a.ts', previous_filename: null }])).toEqual(['a.ts']); + }); + + test('a non-string previous_filename throws rather than being ignored', () => { + expect(() => changedPaths([{ filename: 'a.ts', previous_filename: 42 }])).toThrow( + /non-string "previous_filename"/, + ); + }); +}); + describe('the surface list carries its own coverage', () => { test('every protected surface has a case that was observed blocking', () => { const covered = new Set(BLOCKING_CASES.map((testCase) => testCase.pattern)); diff --git a/scripts/check-gate-integrity.ts b/scripts/check-gate-integrity.ts index 7a7cd84c..72d6941d 100644 --- a/scripts/check-gate-integrity.ts +++ b/scripts/check-gate-integrity.ts @@ -255,6 +255,37 @@ export function pluck(entries: readonly unknown[], field: string): string[] { }); } +/** + * Pure: every path a changed-file entry touches — its current path and, for a + * rename, the path it came from. + * + * The second half is load-bearing and was the guard's one real bypass. GitHub's + * files endpoint reports a rename as `filename: ` with the old path only in + * `previous_filename`, so a pull request that moved + * `.github/workflows/trusted-gates.yml` to `.github/wf/trusted-gates.yml` would + * present this check with a path matching nothing, pass clean, and delete the + * trusted gate on merge. A deletion is not affected — `filename` is the deleted + * path — which is exactly why the gap was easy to miss. + * + * Reading both is the fail-closed direction: it can only ever add a path. + */ +export function changedPaths(entries: readonly unknown[]): string[] { + const paths: string[] = []; + for (const path of pluck(entries, 'filename')) paths.push(path); + for (const [index, entry] of entries.entries()) { + const previous = (entry as Record | null)?.previous_filename; + if (previous === undefined || previous === null) continue; + if (typeof previous !== 'string') { + throw new Error( + `entry ${index} has a non-string "previous_filename"; refusing to check a ` + + `partially-parsed list`, + ); + } + paths.push(previous); + } + return paths; +} + function readJson(path: string): unknown { let raw: string; try { @@ -314,7 +345,13 @@ export function parseArgs(argv: readonly string[]): Options { function main(argv: readonly string[]): void { const options = parseArgs(argv); - const paths = pluck(flattenPages(readJson(options.files)), 'filename'); + const entries = flattenPages(readJson(options.files)); + // Two different counts, deliberately kept apart. `entries` is what the pull + // request changed and is what `changed_files` counts; `paths` can be longer, + // because a rename contributes both the path it went to and the one it came + // from. Comparing the wrong one against `--expected-files` would make every + // renaming pull request fail as "truncated". + const paths = changedPaths(entries); const labels = pluck(flattenPages(readJson(options.labels)), 'name'); // A pull request always changes at least one file, so an empty list means the @@ -322,7 +359,7 @@ function main(argv: readonly string[]): void { // here renders identically to a clean run — the exact fail-quiet shape ADR-0016 // exists to prevent, and the one that would make this guard useless in the only // case it is for. - if (paths.length === 0) { + if (entries.length === 0) { throw new Error( 'the pull request listed no changed files, which cannot happen. ' + 'Refusing to report a pass over an empty list.', @@ -332,9 +369,9 @@ function main(argv: readonly string[]): void { // The files endpoint caps at 3000 entries and says so by truncating, not by // erroring. A gate path past the cap would be invisible, so the count the pull // request itself reports is compared against the count actually read. - if (options.expectedFiles !== undefined && options.expectedFiles !== paths.length) { + if (options.expectedFiles !== undefined && options.expectedFiles !== entries.length) { throw new Error( - `the pull request reports ${options.expectedFiles} changed file(s) but ${paths.length} ` + + `the pull request reports ${options.expectedFiles} changed file(s) but ${entries.length} ` + `were read; the listing is truncated or stale. Refusing to report a pass over a ` + `partial list.`, ); From bf6cf5cd4b0f3827d223f2255d4e9792f777442d Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 26 Aug 2026 18:01:51 -0400 Subject: [PATCH 03/10] fix(ci): bind the acknowledgment to a head, cover .github/CODEOWNERS, escape printed paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from an independent security review of the privileged workflow, against a fork-author-with-no-write-access threat model. Two were bypasses. A stale acknowledgment authorized later pushes. The label was read from the event payload and never bound to a commit, so: open a small, plausible scripts/ change, get it acknowledged, then push a workflow edit. The synchronize run saw the same label and reported success over gate paths nobody had looked at. Nothing else invalidated it — the main ruleset has no pull_request rule, so there is not even stale-review dismissal to inherit. Closed by dismiss-stale-acknowledgment, which removes the label on every push, and by reading labels from the API rather than the pre-dismissal payload. The timestamp alternative was rejected: commit dates are author-controlled, so GIT_COMMITTER_DATE would make a stale acknowledgment look fresh. Dismissal depends on no attacker-controlled value. .github/CODEOWNERS was unprotected while the root file was, and GitHub resolves .github/ first — so adding one supersedes the protected file without touching it. All three locations GitHub honors are covered, asserted against a stated list rather than against the surface list, which cannot see a surface that was never added. Attacker-chosen paths could forge workflow commands: git permits a newline in a filename and the runner trims leading whitespace before testing for the :: prefix, so indentation was not protection. ::stop-commands:: now wraps both steps and printed paths escape every control and format character. JSON.stringify was insufficient — it leaves U+200B as invisible as it found it, which the test caught. Also removed a fail-open in the dismissal: piping gh into `grep -q` lets a match close the pipe, gh dies of SIGPIPE, and under pipefail the pipeline reports failure — reading as "the label is gone" exactly when it is present. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- .github/workflows/trusted-gates.yml | 74 +++++++++++++++++- CHANGELOG.md | 7 +- ...-a-pull-request-from-the-default-branch.md | 44 +++++++++++ docs/repository-trust-operations.md | 26 +++++++ scripts/check-gate-integrity.test.ts | 75 +++++++++++++++++++ scripts/check-gate-integrity.ts | 67 ++++++++++++++++- undefined/f.json | 1 + undefined/l.json | 1 + 8 files changed, 288 insertions(+), 7 deletions(-) create mode 100644 undefined/f.json create mode 100644 undefined/l.json diff --git a/.github/workflows/trusted-gates.yml b/.github/workflows/trusted-gates.yml index be6b0fdf..651fc938 100644 --- a/.github/workflows/trusted-gates.yml +++ b/.github/workflows/trusted-gates.yml @@ -142,6 +142,55 @@ jobs: echo "::stop-commands::${token}" bun scripts/check-dco.ts "${BASE_SHA}..${HEAD_SHA}" + dismiss-stale-acknowledgment: + # An acknowledgment authorizes a *state*, not a pull request. Without this + # job the label survives every subsequent push, so the sequence is: open a + # small, plausible change under `scripts/`; get it acknowledged; then push a + # workflow edit. The `synchronize` run still sees the label and reports + # success over gate paths nobody looked at. That is a real bypass available + # to a fork author with no write access, and it is the reason + # `dismiss_stale_reviews_on_push` exists for reviews. + # + # Dismissal rather than a timestamp comparison, deliberately. The obvious + # alternative — compare the label event against the newest commit — rests on + # `committed` dates, which the author sets. `GIT_COMMITTER_DATE` is free, so + # a stale acknowledgment could be made to look fresh. Removing the label + # depends on no attacker-controlled value at all. + # + # This is the one job here that writes. It touches a label and nothing else, + # checks nothing out, and runs no repository code. + name: dismiss-stale-acknowledgment + if: github.event.action == 'synchronize' + runs-on: ubuntu-latest + permissions: + pull-requests: write + steps: + - name: Remove the acknowledgment, because the head moved under it + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: | + set -euo pipefail + # A 404 is the ordinary case — most pushes carry no acknowledgment — and + # any other failure is deliberately swallowed here too, because the + # verification below is what decides. Trusting the delete's exit status + # and skipping the read would leave a 403 looking like a success. + gh api -X DELETE \ + "repos/${REPO}/issues/${PR_NUMBER}/labels/gate-change-acknowledged" \ + --silent || true + # No pipe into `grep -q`. Under `pipefail` a matching grep closes the + # pipe, `gh` dies of SIGPIPE, and the pipeline reports failure — which + # would read as "the label is gone" at exactly the moment it is present. + remaining=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/labels" \ + --jq '[.[].name] | index("gate-change-acknowledged") // "absent"') + if [ "$remaining" != "absent" ]; then + echo "the acknowledgment is still present after dismissal; refusing to" >&2 + echo "let a stale acknowledgment authorize a head it never saw." >&2 + exit 1 + fi + echo "no acknowledgment is in effect for this head" + gate-integrity: # `trusted-dco` closes the "edit the check" half. This closes the other half. # @@ -152,6 +201,11 @@ jobs: # runs through `.github/workflows/` or `scripts/`. This refuses both unless a # maintainer has said, on the record, that the change is deliberate. name: gate-integrity + # `always()` because the dismissal job is skipped on every event that is not a + # push, and a plain `needs` would skip this with it. The result is still + # ordered after dismissal, which is what closes the race described below. + needs: [dismiss-stale-acknowledgment] + if: always() && needs.dismiss-stale-acknowledgment.result != 'failure' runs-on: ubuntu-latest permissions: contents: read @@ -171,26 +225,40 @@ jobs: # into a document that is not JSON. Written to a file rather than piped # through a shell variable so that a path containing a newline — which git # permits — cannot reframe the list. - - name: List the pull request's changed paths and labels + # + # Labels come from the API, **not** from `github.event.pull_request.labels`. + # The payload is a snapshot taken before `dismiss-stale-acknowledgment` ran, + # so on a push it would still show the label this run has just removed — + # reintroducing the bypass the dismissal exists to close. + - name: List the pull request's changed paths and its labels as they are now env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUMBER: ${{ github.event.pull_request.number }} REPO: ${{ github.repository }} - PR_LABELS: ${{ toJSON(github.event.pull_request.labels) }} run: | set -euo pipefail gh api --paginate --slurp \ "repos/${REPO}/pulls/${PR_NUMBER}/files" > "${RUNNER_TEMP}/pr-files.json" - printf '%s' "$PR_LABELS" > "${RUNNER_TEMP}/pr-labels.json" + gh api --paginate --slurp \ + "repos/${REPO}/issues/${PR_NUMBER}/labels" > "${RUNNER_TEMP}/pr-labels.json" # `--expected-files` is the fail-quiet guard. The files endpoint caps at 3000 # entries and truncates rather than erroring, and a gate path past the cap # would be invisible to a check whose pass condition is "no gate path here". + # + # Wrapped in `::stop-commands::` for the same reason the DCO step is: the + # paths printed here are attacker-chosen, git permits a newline in a + # filename, and the runner trims leading whitespace before looking for the + # `::` prefix — so indentation is not protection. The script escapes control + # characters as well; neither measure is trusted alone. - name: Verify no gate-defining path changed without an acknowledgment env: CHANGED_FILES: ${{ github.event.pull_request.changed_files }} run: | set -euo pipefail + token="adrkit-$(openssl rand -hex 16)" + trap 'echo "::${token}::"' EXIT + echo "::stop-commands::${token}" bun scripts/check-gate-integrity.ts \ --files "${RUNNER_TEMP}/pr-files.json" \ --labels "${RUNNER_TEMP}/pr-labels.json" \ diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f07a682..0530ce9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,11 @@ Until `1.0.0`, minor releases may include breaking changes git objects that are never checked out or executed; the `dco` job in `ci.yml` is retained as a faster advisory report that can only fail open. `gate-integrity` blocks any change under `.github/workflows/`, `.github/actions/`, `scripts/`, - `packages/ci/`, or `CODEOWNERS` unless a maintainer applies the - `gate-change-acknowledged` label, which requires triage or write access + `packages/ci/`, or any of the three locations GitHub resolves `CODEOWNERS` from, + unless a maintainer applies the `gate-change-acknowledged` label — which + requires triage or write access, and which is dismissed automatically on every + push, so an acknowledgment authorizes the head it was given for and not the one + that follows it ([#137](https://github.com/mbeacom/adrkit/issues/137), [ADR-0035](docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md)). diff --git a/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md b/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md index d86c7b8c..c53a7100 100644 --- a/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md +++ b/docs/adr/0035-execute-the-gates-that-certify-a-pull-request-from-the-default-branch.md @@ -247,6 +247,50 @@ difference, now that a trusted execution path is known to exist. What it gets right — that the residual must be stated rather than hidden — is kept: the sections above say exactly what remains open. +## What independent security review changed + +The implementation was reviewed against a fork-author threat model before it was +proposed, and the review found three things worth recording, because two of them +were bypasses rather than polish. They are named here rather than quietly fixed, +since a record claiming a boundary is more trustworthy when it says where the +boundary leaked during construction. + +**A stale acknowledgment authorized later pushes.** The label was read from the +event payload and never bound to a commit, so the sequence *open a small, +plausible `scripts/` change → get it acknowledged → push a workflow edit* left +the `synchronize` run seeing the same label and reporting success over gate paths +nobody had looked at. This was reachable by a fork author with no write access, +and no other mechanism invalidated it: the `main` ruleset has no `pull_request` +rule, so there is not even a stale-review dismissal to inherit from. + +Closed by a `dismiss-stale-acknowledgment` job that removes the label on every +`synchronize`, and by reading labels from the API rather than the payload — the +payload is a snapshot taken before that job runs. The obvious alternative, +comparing the label's timestamp against the newest commit, was rejected: commit +dates are author-controlled, so `GIT_COMMITTER_DATE` would make a stale +acknowledgment look fresh. Dismissal depends on no attacker-controlled value. + +**`.github/CODEOWNERS` was unprotected.** Only the root file was, but GitHub +resolves `.github/CODEOWNERS` *first*. A pull request that added one would +supersede the protected file without touching it. All three locations GitHub +honors are now covered. The coverage assertion in the test suite could not have +caught this — it iterates the surface list, so it cannot see a surface that was +never added, which is a small instance of ADR-0016's own subject. + +**Attacker-chosen paths could forge workflow commands.** Git permits a newline in +a filename, and the runner trims leading whitespace before testing for the `::` +prefix, so indenting the output was not protection. The `::stop-commands::` +hardening now wraps both steps, and printed paths escape every control and format +character. `JSON.stringify` was tried first and is insufficient: it escapes +control characters but leaves U+200B ZERO WIDTH SPACE exactly as invisible as it +found it, which the test observed. + +The review also confirmed, with evidence, the properties the decision rests on: +no pull-request code is checked out, installed, built, or executed; no `${{ }}` +appears in any `run:` body; the fetch is genuinely anonymous under +`persist-credentials: false`; and the `::stop-commands::` token is masked by the +runner before it is echoed, so it cannot be learned and replayed. + ## Trade-offs Every pull request that touches a workflow, a script, `packages/ci/`, or diff --git a/docs/repository-trust-operations.md b/docs/repository-trust-operations.md index f09c2cdb..53473fb1 100644 --- a/docs/repository-trust-operations.md +++ b/docs/repository-trust-operations.md @@ -242,6 +242,32 @@ Note the two counts in that output. `--expected-files` is compared against the ends. Comparing the wrong one would have made every renaming pull request fail as "truncated" — a false block, which is the merge-stopping direction. +Two further bypasses were found by independent security review against a +fork-author threat model, and both are closed with permanent negative cases: + +- **A stale acknowledgment authorized later pushes.** Acknowledge a small + `scripts/` change, then push a workflow edit; the `synchronize` run saw the + same label and reported success. Closed by `dismiss-stale-acknowledgment`, + which removes the label on every push, and by reading labels from the API + rather than from the pre-dismissal event payload. +- **`.github/CODEOWNERS` was unprotected** while the root file was, and GitHub + resolves `.github/` first. All three locations are covered now. + +And one hardening gap: attacker-chosen paths were printed unescaped into a +privileged job's log. Observed end-to-end after the fix, on a path carrying a +forged annotation: + +```console +$ bun run scripts/check-gate-integrity.ts --files forged.json --labels none.json \ + --expected-files 2 +exit: 1 +lines the runner would parse as commands: NONE +``` + +`JSON.stringify` was the first attempt and was insufficient — it escapes control +characters but leaves U+200B ZERO WIDTH SPACE untouched, so an invisible +character would still have printed invisibly. The test caught it. + ### 3.2 Not yet observed — say so plainly **The deployed workflow has never run.** It cannot, before merge: GitHub takes diff --git a/scripts/check-gate-integrity.test.ts b/scripts/check-gate-integrity.test.ts index 7ee8021d..d715c3f2 100644 --- a/scripts/check-gate-integrity.test.ts +++ b/scripts/check-gate-integrity.test.ts @@ -22,10 +22,12 @@ import { describe, expect, test } from 'bun:test'; import { + CODEOWNERS_LOCATIONS, DEFAULT_ACK_LABEL, GATE_SURFACES, changedPaths, classifyGateChanges, + displayPath, flattenPages, formatBlock, formatReport, @@ -43,6 +45,8 @@ const BLOCKING_CASES: ReadonlyArray<{ path: string; pattern: string }> = [ { path: 'scripts/check-dco.ts', pattern: 'scripts/' }, { path: 'packages/ci/dist/index.js', pattern: 'packages/ci/' }, { path: 'CODEOWNERS', pattern: 'CODEOWNERS' }, + { path: '.github/CODEOWNERS', pattern: '.github/CODEOWNERS' }, + { path: 'docs/CODEOWNERS', pattern: 'docs/CODEOWNERS' }, ]; /** Paths an ordinary change touches. None of these may block. */ @@ -273,6 +277,77 @@ describe('a rename cannot carry a gate path out of sight', () => { }); }); +describe('CODEOWNERS is protected at every location GitHub honors', () => { + // Found in security review, not by the coverage assertion below — which reads + // GATE_SURFACES and therefore cannot see a surface that was never added. GitHub + // resolves `.github/CODEOWNERS` *before* the root file, so a pull request that + // adds one supersedes the protected root file without ever touching it. Asserted + // against a stated list rather than against the surface list itself. + for (const location of CODEOWNERS_LOCATIONS) { + test(`${location} blocks without the acknowledgment`, () => { + expect(classifyGateChanges([location], []).verdict).toBe('blocked'); + }); + } + + test('the three locations are the ones GitHub honors, in precedence order', () => { + expect([...CODEOWNERS_LOCATIONS]).toEqual([ + '.github/CODEOWNERS', + 'CODEOWNERS', + 'docs/CODEOWNERS', + ]); + }); + + test('every stated location is actually in the surface list', () => { + const declared = new Set(GATE_SURFACES.map((surface) => surface.pattern)); + expect(CODEOWNERS_LOCATIONS.filter((location) => !declared.has(location))).toEqual([]); + }); + + test('a file merely named like CODEOWNERS elsewhere does not block', () => { + expect(surfaceOf('packages/core/CODEOWNERS')).toBeUndefined(); + expect(surfaceOf('CODEOWNERS.md')).toBeUndefined(); + }); +}); + +describe('printed paths cannot forge workflow commands', () => { + // Git permits a newline in a filename and the files endpoint carries it + // through, so an attacker-chosen path can put `::` at the start of a physical + // log line inside a privileged job. The runner trims leading whitespace before + // testing for the prefix, so the output's indentation is not protection. + const forged = '.github/workflows/a.yml\n::error title=Gate::forged'; + + test('a path carrying a newline is escaped, not printed raw', () => { + const report = classifyGateChanges([forged], []); + for (const text of [formatReport(report), formatBlock(report)]) { + const parsed = text.split('\n').filter((line) => line.trimStart().startsWith('::')); + expect(parsed).toEqual([]); + // Escaped rather than dropped: the reader still learns the exact path. + expect(text).toContain('\\u000a::error title=Gate::forged'); + } + }); + + test('the escaped path still blocks, and names its surface', () => { + const report = classifyGateChanges([forged], []); + expect(report.verdict).toBe('blocked'); + expect(report.changes[0]?.surface.pattern).toBe('.github/workflows/'); + }); + + test('an ordinary path is printed unchanged', () => { + expect(displayPath('scripts/check-dco.ts')).toBe('scripts/check-dco.ts'); + }); + + test('carriage returns and zero-width characters are escaped too', () => { + expect(displayPath('a\rb')).toBe('"a\\u000db"'); + // The case JSON.stringify gets wrong: U+200B is a format character, not a + // control character, so JSON leaves it exactly as invisible as it found it. + expect(displayPath('scripts/\u200bx.ts')).toBe('"scripts/\\u200bx.ts"'); + expect(JSON.stringify('scripts/\u200bx.ts')).not.toContain('\\u200b'); + }); + + test('a quote or backslash in a path cannot break the rendering', () => { + expect(displayPath('a"b\\c\nd')).toBe('"a\\"b\\\\c\\u000ad"'); + }); +}); + describe('the surface list carries its own coverage', () => { test('every protected surface has a case that was observed blocking', () => { const covered = new Set(BLOCKING_CASES.map((testCase) => testCase.pattern)); diff --git a/scripts/check-gate-integrity.ts b/scripts/check-gate-integrity.ts index 72d6941d..4b534462 100644 --- a/scripts/check-gate-integrity.ts +++ b/scripts/check-gate-integrity.ts @@ -94,13 +94,39 @@ export const GATE_SURFACES: readonly GateSurface[] = [ kind: 'prefix', why: 'the published governing-decisions and queue Actions, which are gates', }, + // All three locations GitHub honors, in its resolution order. Protecting only + // the root file left the gap that matters: GitHub resolves `.github/CODEOWNERS` + // *first*, so a pull request that adds one supersedes the protected root file + // entirely without ever touching it. Found in security review, not by the + // coverage assertion below — which iterates this list and therefore cannot + // detect a surface that was never added. + { + pattern: '.github/CODEOWNERS', + kind: 'exact', + why: 'GitHub resolves this before the root file, so adding it supersedes CODEOWNERS', + }, { pattern: 'CODEOWNERS', kind: 'exact', why: 'decides who is asked to review a change to any of the above', }, + { + pattern: 'docs/CODEOWNERS', + kind: 'exact', + why: 'the third location GitHub honors, after .github/ and the root', + }, ]; +/** + * The locations GitHub resolves CODEOWNERS from, in its own precedence order. + * + * Named separately from {@link GATE_SURFACES} so a test can assert the list is + * complete against a specific stated set rather than against the surface list + * itself. A coverage check that reads the thing it is checking cannot see an + * omission, which is how `.github/CODEOWNERS` was missing in the first place. + */ +export const CODEOWNERS_LOCATIONS = ['.github/CODEOWNERS', 'CODEOWNERS', 'docs/CODEOWNERS'] as const; + export interface GateChange { readonly path: string; readonly surface: GateSurface; @@ -172,6 +198,38 @@ export function classifyGateChanges( return { examined: paths.length, changes, acknowledged, ackLabel, verdict }; } +/** + * Pure: a path rendered safe to print in a GitHub Actions log. + * + * Git permits a newline in a filename and the files endpoint carries it through, + * so an attacker-chosen path can place `::` at the start of a physical log line + * inside a privileged job. The runner trims leading whitespace before testing for + * the workflow-command prefix, so indenting the output is not protection: it + * would parse `::add-mask::` or `::error::` and let a pull request forge + * annotations and suppress this guard's own output. + * + * `JSON.stringify` escapes every control character and is reversible, so the + * reader still sees exactly which path tripped the guard. Applied to *every* + * printed path rather than only suspicious ones, because a rule that decides + * which paths are dangerous is one more thing that can be wrong. + */ +export function displayPath(path: string): string { + if (!/[\p{Cc}\p{Cf}]/u.test(path)) return path; + // Not `JSON.stringify` alone. It escapes control characters below U+0020 but + // leaves format characters such as U+200B ZERO WIDTH SPACE exactly as they are, + // so a path carrying one would still print as though it did not — which is the + // whole failure this function exists to prevent. Escaping every Cc and Cf + // explicitly makes the invisible visible; the surrounding quotes mark the path + // as rendered rather than literal. + const escaped = path + .replace(/[\\"]/g, (character) => `\\${character}`) + .replace( + /[\p{Cc}\p{Cf}]/gu, + (character) => `\\u${character.codePointAt(0)!.toString(16).padStart(4, '0')}`, + ); + return `"${escaped}"`; +} + /** * Pure: the report as text. * @@ -184,7 +242,9 @@ export function formatReport(report: GateIntegrityReport): string { const lines = [`check-gate-integrity: examined ${report.examined} changed path(s)`]; for (const change of report.changes) { - lines.push(` gate ${change.path} — ${change.surface.pattern}: ${change.surface.why}`); + lines.push( + ` gate ${displayPath(change.path)} — ${change.surface.pattern}: ${change.surface.why}`, + ); } if (report.verdict === 'clean') { @@ -207,7 +267,10 @@ export function formatBlock(report: GateIntegrityReport): string { // file *is*, which is the difference between acting on the block and labelling // past it. const listed = report.changes - .map((change) => ` ${change.path}\n ${change.surface.pattern} — ${change.surface.why}`) + .map( + (change) => + ` ${displayPath(change.path)}\n ${change.surface.pattern} — ${change.surface.why}`, + ) .join('\n'); return ( `${report.changes.length} of ${report.examined} changed path(s) alter the surface that ` + diff --git a/undefined/f.json b/undefined/f.json new file mode 100644 index 00000000..57301a29 --- /dev/null +++ b/undefined/f.json @@ -0,0 +1 @@ +[[{"filename":".github/workflows/a.yml\n::error title=Gate::forged\n::add-mask::gate"},{"filename":"README.md"}]] \ No newline at end of file diff --git a/undefined/l.json b/undefined/l.json new file mode 100644 index 00000000..0637a088 --- /dev/null +++ b/undefined/l.json @@ -0,0 +1 @@ +[] \ No newline at end of file From a61d811d85672219a56f5120bc5256901a9b5c36 Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 26 Aug 2026 18:02:01 -0400 Subject: [PATCH 04/10] chore: remove scratch files committed by mistake undefined/f.json and undefined/l.json were left by a verification probe whose output path env var was unset, and were swept in by `git add -A`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- undefined/f.json | 1 - undefined/l.json | 1 - 2 files changed, 2 deletions(-) delete mode 100644 undefined/f.json delete mode 100644 undefined/l.json diff --git a/undefined/f.json b/undefined/f.json deleted file mode 100644 index 57301a29..00000000 --- a/undefined/f.json +++ /dev/null @@ -1 +0,0 @@ -[[{"filename":".github/workflows/a.yml\n::error title=Gate::forged\n::add-mask::gate"},{"filename":"README.md"}]] \ No newline at end of file diff --git a/undefined/l.json b/undefined/l.json deleted file mode 100644 index 0637a088..00000000 --- a/undefined/l.json +++ /dev/null @@ -1 +0,0 @@ -[] \ No newline at end of file From 6d988c4102a352af295985aafb2dc6c3bc3e5d78 Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 26 Aug 2026 18:08:58 -0400 Subject: [PATCH 05/10] refactor(ci): fold acknowledgment dismissal into gate-integrity as an ordered step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The separate dismiss-stale-acknowledgment job needed a `needs` edge plus an `always()` guard to survive being skipped on non-push events. Dismissal only has to happen before the labels are read, and step order gives that ordering directly, so the machinery existed solely to reproduce across a job boundary something a step boundary provides for free. The cost is that gate-integrity now carries pull-requests: write. It is contained — that job executes no repository code and nothing from the pull request — and it removes an ordering race rather than adding one. A correction, because the first version of this message asserted a cause that was not true. I saw no workflow runs for two pushed commits, checked that other apps' check suites had been created for the same SHA while github-actions' had not, and concluded the head was being rejected — then attributed it to this job's hyphenated `needs.dismiss-stale-acknowledgment` dereference. Both claims were wrong. The runs were queued, not refused: a61d811 reports three runs once they arrived, roughly five minutes after I looked, and bf6cf5c has none only because the next push superseded it. That is ADR-0016's subject exactly — an absence read as a fact, "I could not see any runs" rendered as "no runs were created" — committed by an author who had spent the session reading that record. The refactor is kept because it is simpler on its own merits, not because it fixed anything. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- .github/workflows/trusted-gates.yml | 70 +++++++++++++---------------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/.github/workflows/trusted-gates.yml b/.github/workflows/trusted-gates.yml index 651fc938..c5a39bb6 100644 --- a/.github/workflows/trusted-gates.yml +++ b/.github/workflows/trusted-gates.yml @@ -142,30 +142,44 @@ jobs: echo "::stop-commands::${token}" bun scripts/check-dco.ts "${BASE_SHA}..${HEAD_SHA}" - dismiss-stale-acknowledgment: - # An acknowledgment authorizes a *state*, not a pull request. Without this - # job the label survives every subsequent push, so the sequence is: open a - # small, plausible change under `scripts/`; get it acknowledged; then push a - # workflow edit. The `synchronize` run still sees the label and reports - # success over gate paths nobody looked at. That is a real bypass available - # to a fork author with no write access, and it is the reason - # `dismiss_stale_reviews_on_push` exists for reviews. + gate-integrity: + # `trusted-dco` closes the "edit the check" half. This closes the other half. # - # Dismissal rather than a timestamp comparison, deliberately. The obvious - # alternative — compare the label event against the newest commit — rests on - # `committed` dates, which the author sets. `GIT_COMMITTER_DATE` is free, so - # a stale acknowledgment could be made to look fresh. Removing the label - # depends on no attacker-controlled value at all. + # A required status check is matched by *name*, so a pull request that cannot + # edit the trusted job can still declare a job of its own with the same name + # and let the later result stand. Every route to that runs through a change + # under `.github/workflows/`, and every route to neutering an advisory gate + # runs through `.github/workflows/` or `scripts/`. This refuses both unless a + # maintainer has said, on the record, that the change is deliberate. # - # This is the one job here that writes. It touches a label and nothing else, - # checks nothing out, and runs no repository code. - name: dismiss-stale-acknowledgment - if: github.event.action == 'synchronize' + # Dismissal is a *step* here rather than a separate job on purpose. It has to + # happen before the labels are read, and expressing that across jobs needs a + # `needs` edge plus an `always()` guard to survive the skip on non-push + # events — machinery whose only job is to reproduce the ordering that step + # order already gives for free. The cost is that this job carries + # `pull-requests: write`; it is contained, because the job executes no + # repository code and nothing from the pull request. + name: gate-integrity runs-on: ubuntu-latest permissions: + contents: read pull-requests: write steps: - - name: Remove the acknowledgment, because the head moved under it + # An acknowledgment authorizes a *state*, not a pull request. Without this + # the label survives every subsequent push, so the sequence is: open a + # small, plausible change under `scripts/`; get it acknowledged; then push + # a workflow edit. The `synchronize` run still sees the label and reports + # success over gate paths nobody looked at. That is a real bypass available + # to a fork author with no write access, and it is the reason + # `dismiss_stale_reviews_on_push` exists for reviews. + # + # Dismissal rather than a timestamp comparison, deliberately. The obvious + # alternative — compare the label event against the newest commit — rests + # on `committed` dates, which the author sets. `GIT_COMMITTER_DATE` is + # free, so a stale acknowledgment could be made to look fresh. Removing the + # label depends on no attacker-controlled value at all. + - name: Dismiss the acknowledgment, because the head moved under it + if: github.event.action == 'synchronize' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUMBER: ${{ github.event.pull_request.number }} @@ -191,26 +205,6 @@ jobs: fi echo "no acknowledgment is in effect for this head" - gate-integrity: - # `trusted-dco` closes the "edit the check" half. This closes the other half. - # - # A required status check is matched by *name*, so a pull request that cannot - # edit the trusted job can still declare a job of its own with the same name - # and let the later result stand. Every route to that runs through a change - # under `.github/workflows/`, and every route to neutering an advisory gate - # runs through `.github/workflows/` or `scripts/`. This refuses both unless a - # maintainer has said, on the record, that the change is deliberate. - name: gate-integrity - # `always()` because the dismissal job is skipped on every event that is not a - # push, and a plain `needs` would skip this with it. The result is still - # ordered after dismissal, which is what closes the race described below. - needs: [dismiss-stale-acknowledgment] - if: always() && needs.dismiss-stale-acknowledgment.result != 'failure' - runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: read - steps: - name: Check out the default branch (never the pull request) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: From 2457fed49f26821dbd27f1fecf089afccfa7896d Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 26 Aug 2026 18:12:33 -0400 Subject: [PATCH 06/10] docs: record the absence-read-as-fact instance in the operations doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-0016 collects instances of a check or a claim reporting "nothing" when it means "nothing visible". This session produced one, and it is worth more written down than quietly corrected. Two pushes appeared to produce no workflow runs; for the same head SHA other apps' check suites existed while github-actions' did not, which is what a rejected workflow file looks like. A cause was then attributed to a hyphenated job id in a needs..result dereference and written into a commit message as though established. The runs were queued, not refused. Every individual observation was accurate — the suites really were missing at the moment they were read. The defect was treating a read of a system with latency as a final state, which is ADR-0016's own sentence. Recorded here rather than in ADR-0016 itself, to avoid editing an accepted record from a change about something else. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- docs/repository-trust-operations.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/repository-trust-operations.md b/docs/repository-trust-operations.md index 53473fb1..2ad325fa 100644 --- a/docs/repository-trust-operations.md +++ b/docs/repository-trust-operations.md @@ -301,6 +301,35 @@ irrelevant to the merge. Until both have been seen red and then green on a real pull request, ADR-0035 action item 3 stays open and the workflow counts as implemented, not as verified. +### 3.3 An instance, recorded rather than tidied away + +While building this, its author read an absence as a fact — the exact failure +ADR-0016 exists to name — and committed the wrong conclusion before catching it. + +Two pushes appeared to produce no workflow runs. Checking further seemed to +confirm it: for the same head SHA, other apps' check suites existed and +`github-actions`' did not, which is what a rejected workflow file looks like. The +cause was then attributed to a hyphenated job id in a `needs..result` +dereference, and that attribution was written into a commit message as though it +had been established. + +All of it was wrong. The runs were **queued, not refused**. The commit reports +three runs once they arrived, roughly five minutes after the check; the other +commit has none only because the next push superseded it. Nothing was ever +blocked, and the expression was never the problem. + +What produced the error is worth naming precisely, because it is not +carelessness: every individual observation was accurate. The check suites really +were missing *at the moment they were read*. The defect was treating a read of a +system with latency as a final state — "I could not see any runs" rendered as "no +runs were created", which is ADR-0016's sentence almost verbatim, committed by an +author who had spent the session reading that record. + +The refactor it motivated was kept, because it is simpler on its own merits. The +commit message was amended to say plainly that it fixed nothing. That amendment +is the point: a false diagnosis left in a commit message is permanent here, since +the repository's squash-merge body carries commit messages into `main`. + --- ## 4. What none of this closes From cbb6ad4fc2929073069b5baf99805848d4f39b65 Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 26 Aug 2026 18:58:59 -0400 Subject: [PATCH 07/10] fix(ci): rerun trusted gates on base retarget, and stop overstating what they cover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two material defects from adversarial review, both verified before fixing. Retargeting a pull request's base fires `pull_request_target: edited` with `changes.base` — not `synchronize`, which only fires when the head moves. The workflow listened for neither. Because the head SHA does not change on a retarget, the check runs computed against the old base stayed the latest results for that SHA and kept the required contexts green, while the commit range and the changed-file set both belonged to a base nothing had examined, and the acknowledgment carried over to a diff nobody acknowledged. `edited` is now an activity type, so both gates rerun; trusted-dco reports the base move rather than printing an identical-looking range; and the acknowledgment is dismissed when `changes.base` is present but not on a title or body edit, because a control that fires on noise gets waved through on signal. The second defect was in the claims, not the mechanism. The workflow and ADR-0035 both asserted that every route to neutering an advisory gate ran through a protected path. It does not: ci.yml reaches typecheck, build, lint, release:pack, check:deps, check:freeze-hashes, check:doc-pins, check:clause8, check:no-spike-heuristics, check:site-grammar and adr lint through `bun run `, so the root manifest redirects eleven invocations across three required contexts without touching GATE_SURFACES; self-dogfood runs adr check out of packages/cli. The claim is narrowed to what is true — the trusted gates are complete because they invoke script paths directly, and the advisory gates cannot be made trustworthy by any path list because they execute the pull request's own code. The unprotected routes are enumerated in DOCUMENTED_UNPROTECTED_ROUTES and pinned by a test, so the gap cannot move without the documentation moving with it. Protecting package.json was considered and rejected: it would put the acknowledgment on a weekly Dependabot bump while still not reaching the code those gates run. New assertions observed failing on the exact regressions they defend, by mutating the file and rerunning: removing `edited` fires two; unwiring changes.base fires the dismissal-scope case; un-paginating the label listing fires its own; adding package.json to GATE_SURFACES fires five including the documented-gap test. Related fixes: ADR-0035 affects now covers all three CODEOWNERS locations; operations doc §2.1 records that pull requests already open when the contexts are added need a new event before they can report, with the command to confirm it; the dismissal verification paginates like the files listing; and an empty --expected-files fails as missing rather than coercing to zero, since Number('') is 0 and Number.isInteger(0) is true. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- .github/workflows/trusted-gates.yml | 83 ++++++-- CHANGELOG.md | 12 +- ...-a-pull-request-from-the-default-branch.md | 87 +++++++- docs/repository-trust-operations.md | 57 +++++ scripts/check-gate-integrity.test.ts | 56 +++++ scripts/check-gate-integrity.ts | 63 ++++++ scripts/trusted-gates-workflow.test.ts | 194 ++++++++++++++++++ 7 files changed, 524 insertions(+), 28 deletions(-) create mode 100644 scripts/trusted-gates-workflow.test.ts diff --git a/.github/workflows/trusted-gates.yml b/.github/workflows/trusted-gates.yml index c5a39bb6..a6ce0b6e 100644 --- a/.github/workflows/trusted-gates.yml +++ b/.github/workflows/trusted-gates.yml @@ -44,7 +44,15 @@ on: # `labeled` and `unlabeled` are load-bearing rather than tidy: the # acknowledgment below is a label, so without them applying it would leave the # required check red with no way to re-run it except a push. - types: [opened, synchronize, reopened, labeled, unlabeled] + # + # `edited` is load-bearing for a different reason, and its absence was a real + # hole. Retargeting a pull request to a different base fires `edited` with + # `changes.base` — **not** `synchronize`. Without it the head SHA never moves, + # so the check runs computed against the old base stay the latest results for + # that SHA and keep the required contexts green, while the commit range and + # the changed-file set both belong to a base nothing ever examined. The + # acknowledgment would carry over to a diff nobody acknowledged. + types: [opened, synchronize, reopened, labeled, unlabeled, edited] permissions: contents: read @@ -110,13 +118,23 @@ jobs: env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + BASE_CHANGE: ${{ toJSON(github.event.changes.base) }} run: | set -euo pipefail + # Say when the base moved, and to what. On a retarget the head SHA does + # not move, so this job re-runs over a *different* range while printing + # the same head — and a log that did not mention the move would read + # identically to the run before it. + if [ "${BASE_CHANGE}" != "null" ]; then + echo "the base moved; this is not the range the previous run examined:" + printf '%s\n' "${BASE_CHANGE}" + fi for sha in "$BASE_SHA" "$HEAD_SHA"; do if ! git cat-file -e "${sha}^{commit}" 2>/dev/null; then echo "commit ${sha} is not in this clone — the head was probably force-pushed" >&2 - echo "between the event and this run. Refusing to report a pass over a range" >&2 - echo "that cannot be read." >&2 + echo "between the event and this run, or the base was retargeted to a ref this" >&2 + echo "clone does not carry. Refusing to report a pass over a range that cannot" >&2 + echo "be read." >&2 exit 1 fi done @@ -143,14 +161,23 @@ jobs: bun scripts/check-dco.ts "${BASE_SHA}..${HEAD_SHA}" gate-integrity: - # `trusted-dco` closes the "edit the check" half. This closes the other half. + # `trusted-dco` closes the "edit the check" half. This closes name-shadowing. # # A required status check is matched by *name*, so a pull request that cannot # edit the trusted job can still declare a job of its own with the same name - # and let the later result stand. Every route to that runs through a change - # under `.github/workflows/`, and every route to neutering an advisory gate - # runs through `.github/workflows/` or `scripts/`. This refuses both unless a - # maintainer has said, on the record, that the change is deliberate. + # and let the later result stand. Declaring a job requires a workflow file, so + # that route does run through `.github/workflows/` and this blocks it. + # + # **It does not make the advisory gates in `ci.yml` trustworthy, and nothing + # here should be read as claiming otherwise.** Those jobs execute the pull + # request's own code from the pull request's own checkout, and they reach it + # through `bun run