fix(bin): verify a head-bound attestation instead of PR body text - #1825
Open
sbracewell64 wants to merge 15 commits into
Open
fix(bin): verify a head-bound attestation instead of PR body text#1825sbracewell64 wants to merge 15 commits into
sbracewell64 wants to merge 15 commits into
Conversation
sbracewell64
force-pushed
the
fm/honour-system-check-blocks-the-new-merge-guard
branch
from
August 7, 2026 01:51
f1e63a1 to
e6855d7
Compare
The `Require no-mistakes` check grepped `github.event.pull_request.body` for a fixed literal marker. That field is mutable and author-controlled, so the check verified no evidence and bound to no commit: the marker could be typed once, pasted into any pull request, and survived every force-push and rebase. It was a label with a CI job attached, not an attestation. Two consequences were live. Every fork landing pull request carried a genuine FAILURE on this check and was merged past it. And a change the pipeline had genuinely validated could not be mirrored onto a fork landing branch without hand-writing the marker, which forges an attestation, so that class of work stalled. MEASURED FIRST, DESIGNED AFTER. The pipeline emits nothing head-bound to the forge today. Inspected on real pipeline-raised heads: commits are unsigned and carry no trailers, the head has no commit statuses, the only check runs are the repository's own Actions, and neither repository advertises any notes or attestation ref. The binding does exist, but only in the pipeline's local run record. The pull request body carries no run identity and no head sha. The check now verifies a git note on refs/notes/no-mistakes keyed by the exact head commit. A note is keyed by a sha, so it names the commit it covers without changing that commit's sha; it never rewrites the branch, so producing one cannot disturb the pipeline's custody of it; and it reaches the forge as an ordinary ref readable with contents: read. bin/fm-attest.sh owns both sides: `write` transcribes the pipeline's own run record and refuses unless that run covers this exact HEAD on this branch and completed review, test, lint and push; `verify` is what CI runs. docs/no-mistakes-attestation.md states what this establishes and what it does not. Absence never reads as evidence. A missing ref, a missing note for this head, an unavailable head commit, a malformed note, a note naming another commit, and a skipped required step each refuse in their own words, because they need different repairs. An unrecognized field is refused rather than ignored, so a later format cannot be read as a weaker v1 one. A head repository that cannot be read stops the job instead of resolving as either absent or present. Every negative control was watched failing before the code was trusted, against real pull request heads as well as fixtures: the old marker pasted verbatim is refused; a genuine attestation copied onto another head is refused; a genuine attestation carried across an amend is refused; and each refusal was re-run with its own guard removed to prove it is causal rather than vacuous. Each is paired with a matched positive control, because a verifier that refused everything would satisfy red-only assertions and would be a worse defect than the check replaced. WHAT THIS DOES NOT ESTABLISH, stated rather than implied. It does not establish who ran the pipeline. no-mistakes runs on the contributor's machine with the contributor's own credentials, so nothing it emits can be unforgeable by its operator; closing that needs an issuer the author does not control, such as a check run posted by an app the base repository installs, which is a change to the pipeline rather than to this check. What is removed is the far weaker property that any text typed into a mutable field satisfied it. KNOWN COST, ACCEPTED. Until an attestation is published, this check fails pull requests that the body-string check passed, including genuinely pipeline-raised ones. That is the honest consequence of requiring evidence that is not yet emitted, and it is actionable: the failure names the one command that fixes it. Required-check configuration is a repository setting and is untouched here. The unblock this enables: because the attestation binds to a commit rather than to pull request prose, a fork landing branch can be validated in its own right without proposing anything upstream, which removes the mutual exclusivity between holding a signature and duplicating a live contribution.
…stale script docs
… every emitted line
…source-byte assertions
…to its declared owner
… attestation refs
sbracewell64
force-pushed
the
fm/honour-system-check-blocks-the-new-merge-guard
branch
from
August 7, 2026 11:16
e6855d7 to
c0128ef
Compare
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.
Intent
GOAL. Replace the
Require no-mistakesGitHub Actions check's pull-request-body text match with verification of something bound to the exact head commit, so the gate attests rather than asks politely. The captain ruled "fix the check" after three independent reports classified it identically as an honour-system label with a CI job attached.THE DEFECT. .github/workflows/no-mistakes-required.yml ran
grep -qFfor a literal marker string against the pull request body, a mutable author-controlled text field. It verified no evidence, was bound to no commit, and was satisfied by anyone who typed the string.THE BAR THE CAPTAIN SET. The change must STRENGTHEN the gate, never weaken it: a design that would let more through than today's is the wrong design. Concretely (1) verify something bound to the exact head commit; (2) not be forgeable by the pull request author; (3) still pass for a genuinely pipeline-raised contribution, including one mirrored onto a fork landing branch; (4) fail closed on absence, because no evidence must never read as evidence.
ESTABLISHED BY MEASUREMENT, NOT ASSUMED. The pipeline emits nothing head-bound to GitHub today, so the design had to specify the smallest thing that must be produced rather than invent a verification for evidence that is not produced. bin/fm-attest.sh is that producer and also the CI-side verifier.
DESIGN DECISION - GIT NOTES. The attestation is a git note on refs/notes/no-mistakes keyed by commit SHA. Notes were chosen over commit trailers deliberately: SHA-keyed, no branch rewrite, so producing one cannot disturb the pipeline's custody of the branch, and readable by the workflow with contents: read alone.
DELIBERATE LIMITATION DISCLOSURE - DO NOT "FIX" IT. docs/no-mistakes-attestation.md states plainly that no-mistakes runs on the contributor's machine using the contributor's own credentials, so no artifact it emits can be unforgeable by the person running it: anything the pipeline can write, its operator can write. That paragraph is a deliberate disclosure the captain's decisions preserved verbatim across two review rounds. It is honesty about the trust boundary, not a softening of the requirement.
DECISIONS ALREADY MADE AND DIRECTED IN EARLIER ROUNDS, ALL DELIBERATE:
editedfor two documented reasons: re-evaluating an unchanged head after the note is published, and base-branch retargeting.WHY THIS RUN EXISTS. The branch was rebased onto upstream main fb368dc after main advanced and PR 1825 went conflicted. One conflict, in tests/fm-backend.test.sh: main replaced the explicit OLD_BIN_UNCHANGED_SIBLINGS list with a whole-tree copy of bin/, which subsumes this branch's only contribution there (adding fm-timeout-lib.sh to that list), so main's version was taken verbatim and this branch's hunk dropped. That resolution is deliberate and should not be reverted. The rebase rewrote every commit, so the previous head-bound attestation no longer covers the branch - by design, which bin/fm-attest.sh reports as run-covers-another-head. This run validates the rebased head so a fresh attestation can be published for it.
DELIBERATELY OUT OF SCOPE. Branch protection, rulesets and repository settings are the captain's and must not be changed; this task changes workflow logic only. Two captain decisions turn on this work - the conflicted-contribution landing question and the gate-enforcement-reality correction - and must not be resolved here.
What Changed
Require no-mistakesworkflow no longer greps the mutable pull-request body for a marker string. It now fetchesrefs/notes/no-mistakesand runsbin/fm-attest.sh verify, which passes only for an attestation bound to the exact head commit SHA and fails closed otherwise — missing attestation, a note copied onto a different head, and a stale post-rebase attestation are each refused with a distinct named reason, and a verifier error ("could not evaluate", exit 2) is reported separately from a refusal (exit 1), with both still failing the check.bin/fm-attest.shproduces and verifies the attestation as a git note keyed by commit SHA (no branch rewrite, readable withcontents: read), sourcing it from the pipeline's run record viabin/fm-nm-run-lib.shwith reads bounded by the shared timeout owner. Everything it emits goes through a singleemit()boundary with default-deny credential redaction: only positively-matched credential-free URL shapes (including scp-style remotes with no colon before the@) are printed, URLs carrying query strings or fragments are withheld, and any withheld token suppresses its whole line.tests/fm-attest.test.sh, a behavioral suite covering the verify refusal taxonomy, redaction, timeout bounding, and the workflow's verify step run as CI runs it, plusdocs/no-mistakes-attestation.mddocumenting the attestation flow and its deliberate trust-boundary disclosure (the pipeline runs with the contributor's own credentials, so its artifacts cannot be unforgeable by their operator).Risk Assessment
✅ Low: Both captain-directed fixes landed exactly as scoped and were verified empirically (query/fragment forms now withheld whole-line while all stay-green shapes still emit; unreadable notes refs get their own fail-closed refusal with docs and matched-control tests), leaving no open concerns beyond one informational note owned by a separately-authorized follow-up task.
Testing
Ran the change's targeted behavioural suite via the repo's own runner (all green) and demonstrated the gate end-to-end in a sandbox — attestation published to a fork, the workflow's verbatim verify step passing for the attested head, failing closed on absence, refusing a copied note as unbound, and distinguishing verifier failure from refusal — with the full CLI transcript captured as evidence; no visual artifact applies since the end-user surface is CI log output, which the transcript shows directly.
Evidence: End-to-end gate transcript (write → fork → CI verify: pass, fail-closed, forgery refused, could-not-evaluate)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-attest.sh:153- The credential-redaction allowlist in credential_safe_stream() treats any '@'-free path as positively safe (is_safe: '(/[^@]*)?$'), so a URL carrying a credential in its query string is emitted verbatim. Verified empirically against the changed code: 'https://host.invalid/repo.git?private_token=SECRET123' passes through unchanged, and 'https://alice:pw@host.invalid/r.git?token=SECRET' is rewritten to strip userinfo but still emitted with '?token=SECRET' intact; presigned shapes ('?X-Amz-Signature=...') also pass. This contradicts the design's stated invariant that a word is emitted only when it positively matches a shape with no place for a credential: a query string is such a place in real remote URLs and server rejection text, and these messages reach agent transcripts and logs. A fix (e.g. withholding tokens whose emitted form contains '?' or '#', consistent with default-deny) changes deliberately-decided emission behavior, so it needs the author's ruling rather than an auto-fix.bin/fm-attest.sh:386- cmd_verify collapses every 'git notes show' failure into the no-attestation-for-head refusal. Verified empirically: with refs/notes/no-mistakes pointing at a blob (ref resolves, notes tree unreadable), verify reports "refs/notes/no-mistakes exists but carries no attestation for <head>" and directs the contributor to publish one, which can never repair a corrupt/unreadable ref — the republish-then-identical-refusal loop the change's own error model ('no condition borrows another's reason') names as the defect to avoid. The outcome is still fail-closed (exit 1), so this is diagnostics accuracy only; distinguishing an unreadable ref would add a new refusal reason to the deliberately enumerated, documented set, so it needs the author's decision.🔧 Fix: withhold query/fragment URLs and name unreadable attestation refs
1 info still open:
bin/fm-attest.sh:760- cmd_show (the local convenience command, not the gate) still collapses every 'git notes show' failure into no-attestation-for-head at bin/fm-attest.sh:760, the same borrowed-reason pattern the directed fix just removed from cmd_verify. This is deliberately untouched: the captain's fix directive scoped the change to cmd_verify plus the redaction allowlist and explicitly forbade expanding further, and a broader hardening pass is separately authorized. Recorded so that task can pick it up; no action in this run.✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-attest.test.sh— the change's full behavioural suite (verify refusal taxonomy with positive controls, write-side emitter against the real run-record shape, default-deny credential redaction, timeout bounding, workflow step scripts run as CI runs them); exit 0Manual end-to-end gate demo: contributor repo + fork bare remote + stubbedno-mistakesrun record;bin/fm-attest.sh writerecorded and pushedrefs/notes/no-mistakes; CI checkout fetched the ref and ran the workflow's verify step script extracted verbatim from.github/workflows/no-mistakes-required.yml→ pass (exit 0) naming the exact head SHAFail-closed check: new unattested head → verify step exit 1 withno-attestation-for-headrefusal and contributor guidanceForgery check: genuine note copied onto a different head → refused asattestation-not-bound(exit 1)Could-not-evaluate arm: verifier stripped of its executable bit (exit 126) → workflow reports 'could not evaluate', worded distinctly from a refusal, still exit 1grep -c PR_BODY .github/workflows/no-mistakes-required.yml→ 0 (the mutable PR-body input is no longer consulted)Confirmed the deliberate trust-boundary disclosure is intact atdocs/no-mistakes-attestation.md:76and the diff touches only workflow logic, scripts, docs and tests✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.