feat(ci): execute the gates that certify a pull request from the default branch - #179
Merged
Conversation
…ult branch 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 <m@beacom.dev>
mbeacom
added
gate-change-acknowledged
Decisions governing this change
|
…pass 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 <m@beacom.dev>
There was a problem hiding this comment.
🟡 Changes recommended
Protected-path renames and stale acknowledgment labels can bypass the new gate, while privileged logs remain injectable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds default-branch-controlled CI gates to prevent pull requests from modifying their own certification logic.
Changes:
- Adds trusted DCO and gate-integrity workflows.
- Introduces gate-surface validation with tests.
- Documents governance decisions and repository operations.
File summaries
| File | Description |
|---|---|
.github/workflows/trusted-gates.yml |
Defines trusted PR gates. |
.github/workflows/ci.yml |
Marks the existing DCO job advisory. |
scripts/check-gate-integrity.ts |
Implements gate-path validation. |
scripts/check-gate-integrity.test.ts |
Tests gate classification and failures. |
scripts/check-dco.ts |
Documents trusted versus advisory execution. |
package.json |
Adds the gate-integrity command. |
CODEOWNERS |
Explicitly assigns gate surfaces. |
docs/adr/0035-*.md |
Records the proposed trust architecture. |
docs/repository-trust-operations.md |
Documents rollout and verification. |
CHANGELOG.md |
Records the new controls. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| function main(argv: readonly string[]): void { | ||
| const options = parseArgs(argv); | ||
|
|
||
| const paths = pluck(flattenPages(readJson(options.files)), 'filename'); |
Comment on lines
+174
to
+179
| - 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) }} |
| mechanical and self-contained. | ||
| queuedAt: 2026-08-26T00:00:00Z | ||
| slaDays: 30 | ||
| reviewBy: 2027-02-26 |
… escape printed paths 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 <m@beacom.dev>
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 <m@beacom.dev>
… ordered step 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 <m@beacom.dev>
mbeacom
force-pushed
the
mbeacom-ci-gates-repository-trust
branch
from
August 26, 2026 22:11
21ea500 to
6d988c4
Compare
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.<id>.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 <m@beacom.dev>
…hat they cover
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 <name>`, 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 <m@beacom.dev>
… cancelling it Two HIGH findings from re-review of the remediation. Both carried an acknowledgment onto a head it was never granted for. Close, push, reopen. Dismissal enumerated the actions that trigger it and `reopened` was not among them. GitHub delivers no pull_request_target event for a push to a closed pull request, and an author can close their own — so get a small change acknowledged, close, push the workflow edit, reopen. The reopened event arrives with the new head SHA, the dismissal step is skipped because its condition did not name that action, and the label is still there. That restored the full name-shadowing capability the gate exists to block. This repository's own operations document had been describing the route as a feature, telling operators to close and reopen to get a fresh event, which is how a live bypass sat in plain sight. Cancel the run that would dismiss. concurrency used cancel-in-progress: true. The dismissal is the first step of gate-integrity, but a job needs a runner before step one executes, so an author could push — queueing the run that would delete their label — then immediately edit the title, an event they can fire at will that shares the group and takes the in-shell early exit without dismissing. The title edit cancelled the push's run before its DELETE ran. A control whose job is to fire on a push must not be abortable by the pusher. Dismissal is now an exclusion list naming only labeled and unlabeled, the two activity types that cannot move the commit range, so anything added to types: later dismisses by default. cancel-in-progress is false; both jobs are short and install nothing, so queueing costs almost nothing. Enumerating the cases that trigger a safety control produced three holes across three reviews — edited, then reopened, and a near miss on opened — because every enumeration is a claim of completeness over a set someone else controls. A test now derives the expected set from the trigger list rather than restating it. Also fixed a case-folding inconsistency between two halves of one control: the dismissal verification matched the label name case-sensitively while classifyGateChanges matches it case-insensitively, so a label created as Gate-Change-Acknowledged would have verified as absent while the gate still honoured it. Observed failing by mutation: reverting to the enumeration fires 4 assertions, restoring cancel-in-progress fires its own, removing ascii_downcase fires its own. One assertion was strengthened first — "the condition does not mention reopened" is true of the correct exclusion list and of the enumeration that omitted it, so it passed against the bug it existed to catch. It now evaluates the condition instead of pattern-matching it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom <m@beacom.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #137.
What was wrong
Every CI gate here runs 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: #98's
clean-clone-buildsexecuted three steps named(network denied)that exist only on that branch, whilemain'sci.ymlcontained none.Reading a script from a trusted base revision does not fix it — the step invoking it is equally under the pull request's control.
The option #137 didn't have
The issue judged required review the honest first move and dismissed "workflows outside the PR's control" as organization-only. That was right about organization required workflows and wrong about the capability.
pull_request_targetruns outside the pull request's control on any repository, personal or not, and since 2025-12-08 GitHub takes the workflow file, every referenced action, and theactions/checkoutcommit for that event from the repository's default branch — not from the PR, and not even from its base branch.What this ships
.github/workflows/trusted-gates.yml(new), onpull_request_target:trusted-dco— the authoritative sign-off gate. Runsmain'sscripts/check-dco.tsfrommain's workflow. The PR's commits are fetched as git objects and read withgit log; they are never checked out and never executed.ci.yml'sdcojob stays as a faster advisory report that can only fail open, and now says so.gate-integrity— closes the other half. A required check is matched by name, so a PR that cannot edit the trusted job could still shadow it; every route to that runs through.github/workflows/. Blocks changes under.github/workflows/,.github/actions/,scripts/,packages/ci/, orCODEOWNERSunless a maintainer appliesgate-change-acknowledged— which requires triage or write access, so an external contributor cannot self-authorize.Safety rules for the privileged trigger, all verifiable in the diff: no PR code is checked out, installed, built or executed; no
bun install(both checks import Node builtins only); read-onlypermissions;persist-credentials: falseso the fetch is anonymous; untrusted values reach steps viaenv:, never${{ }}inside arun:body; commit subjects are echoed inside::stop-commands::so a message reading::error::cannot forge annotations in a privileged run.Required review is declined, with the reason recorded
The issue's preferred option is not available to this repository as a real control. GitHub does not let an author approve their own pull request, so with a sole maintainer who is also the sole code owner,
required_approving_review_count >= 1— orrequire_code_owner_review, which implies it — deadlocks every self-authored change. The escape is the admin bypass ruleset19149458already grantsalways, which turns the rule into a bypass performed on every merge. For an external contributor it adds nothing, because they cannot merge anyway.A rule that is either a deadlock or a routine bypass is exactly what ADR-0016 warns about. ADR-0035 records the finding instead. The
CODEOWNERShalf is adopted anyway, as explicitness — with both caveats stated in the file so the lines aren't mistaken for a gate.Evidence (ADR-0016)
gate-integrity's pass condition is an absence, which is where blindness hides. Observed failing on this change's own real changed-path list, not a fixture, through the CLI the workflow invokes — then passing once the label is applied, with the change still named rather than swallowed. All three fail-quiet guards observed firing:listed no changed files, which cannot happen→ exit 1--expected-files 9reports 9 changed file(s) but 2 were read→ exit 1{"message":"Not Found"}expected a JSON array, got object→ exit 1One defect was found this way rather than by review:
formatBlocklisted offending paths without the reason each was protected. The code was changed, not the test. The suite also fails if a surface is added toGATE_SURFACESwithout a case observing it block.Also validated:
actionlintclean on every workflow,adr lint35 records / 0 errors,bun test2681 pass / 0 fail, typecheck and lint clean, and the::stop-commands::trap verified to preserve a failing exit status while still restoring.Not yet observed, stated plainly: the deployed workflow has never run and cannot before merge —
pull_request_targettakes the workflow from the default branch, sotrusted-gates.ymlis inert until it lands. ADR-0035 action item 3 stays open until both jobs are seen red then green on a real PR. That is why this PR is not claiming rung-2 evidence.Independent security review, and what it changed
The implementation was reviewed against a fork-author-with-no-write-access threat model before this was proposed. Three findings, two of them bypasses. They're named in ADR-0035 rather than quietly fixed, because a record claiming a boundary is more trustworthy when it says where the boundary leaked during construction.
scripts/change → get it acknowledged → push a workflow edit. Thesynchronizerun saw the same label and reported success over gate paths nobody looked at. Nothing else invalidated it — themainruleset has nopull_requestrule, so there isn't even stale-review dismissal to inherit.Closed by a
dismiss-stale-acknowledgmentjob that 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, soGIT_COMMITTER_DATEwould make a stale ack look fresh. Dismissal depends on no attacker-controlled value..github/CODEOWNERSwas unprotected while the root file was — and GitHub resolves.github/first, so adding one supersedes the protected file without touching it. All three locations are covered now, asserted against a stated list rather than against the surface list (which can't see a surface that was never added — a small instance of ADR-0016's own subject).::prefix, so indentation was not protection.::stop-commands::now wraps both steps and printed paths escape every control and format character.JSON.stringifywas insufficient — it leaves U+200B as invisible as it found it. The test caught that.I also found one bypass myself before the review, by reading what the files endpoint actually returns: a rename reports only the new path, so moving
trusted-gates.ymlout of.github/workflows/would have passed clean and deleted the gate on merge. A deletion was never affected, which is what made it easy to miss. Both ends of a rename are read now.And one fail-open in the dismissal step: piping
ghintogrep -qlets a match close the pipe,ghdies of SIGPIPE, and underpipefailthe pipeline reports failure — reading as "the label is gone" exactly when it is present.The review also confirmed with evidence the properties this rests on: no PR code is checked out, installed, built or executed; no
${{ }}in anyrun:body; the fetch is genuinely anonymous underpersist-credentials: false; and the runner masks the::stop-commands::token before echoing it, so it can't be learned and replayed.Repository settings
Active now (applied and verified by re-reading the API):
sha_pinning_requiredfalse→true. Every action here was already SHA-pinned, so no workflow changed; it removes the ability to introduce a mutable tag later — including inside an acknowledged change.gate-change-acknowledgedcreated.Must wait until merge — the exact verified commands are in
docs/repository-trust-operations.md:trusted-dcoandgate-integrityas required contexts on ruleset19149458. This cannot be done before merge: the jobs don't exist until this lands, and a required context that never reports would block every PR including this one. The payload was constructed against the live ruleset, validated, and deliberately not sent.Left as a maintainer decision: tightening fork-PR workflow approval from
first_time_contributorstoall_external_contributors. Real friction against a real hardening; the command is in the operations doc rather than applied silently.What this does not close
Whoever can merge can change a gate and acknowledge the change. Merge access is the boundary it always was, and nothing here is tamper-proof. What is closed is narrower, and is what #137 was actually about: the check certifying a pull request is no longer authored by that pull request.
ADR-0035 is
proposedand agent-drafted — it binds nothing until ratified.