Skip to content

fix(bin): verify a head-bound attestation instead of PR body text - #1825

Open
sbracewell64 wants to merge 15 commits into
kunchenguid:mainfrom
sbracewell64:fm/honour-system-check-blocks-the-new-merge-guard
Open

fix(bin): verify a head-bound attestation instead of PR body text#1825
sbracewell64 wants to merge 15 commits into
kunchenguid:mainfrom
sbracewell64:fm/honour-system-check-blocks-the-new-merge-guard

Conversation

@sbracewell64

@sbracewell64 sbracewell64 commented Aug 6, 2026

Copy link
Copy Markdown

Intent

GOAL. Replace the Require no-mistakes GitHub 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 -qF for 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:

  • Credential redaction is DEFAULT-DENY (allowlist). The script emits only positively-matched credential-free URL shapes and suppresses everything else. The earlier parse-then-redact path was REMOVED rather than kept alongside; keeping both was explicitly refused.
  • scp-style remotes (user@host:path) are modelled as one more positively-matched safe shape, guarded on NO COLON BEFORE THE @, never as an exception carved out of default-deny.
  • When any token on a line is withheld, the WHOLE LINE is suppressed and the marker says the whole line was withheld, not the token. Emitting the safe half of a mixed line was explicitly refused, because that is more parsing and parsing is the thing that kept failing.
  • emit() is the single structural place a line leaves the script, so a caller gets scrubbing by printing rather than by remembering to call a scrubber. This was chosen over adding scrubbing at individual call sites.
  • Two owners, deliberately: bin/fm-timeout-lib.sh owns bounding execution and bin/fm-nm-run-lib.sh owns parsing the run record. An earlier attempt collapsed them and regressed hosts lacking timeout, gtimeout and perl.
  • The workflow reports verifier exit 2 ("could not evaluate") distinctly from exit 1 ("refused"), because collapsing them would be the gate itself committing the absence-is-not-evidence defect this task exists to fix. Both arms still exit 1 overall.
  • The workflow subscribes to edited for two documented reasons: re-evaluating an unchanged head after the note is published, and base-branch retargeting.
  • Tests assert behaviour only. Two earlier assertions on awk-extracted workflow source were removed under the repo rule against asserting implementation-source bytes; anti-vacuity is preserved by an emptiness guard plus behavioural assertions, verified by witnessing that both mis-extraction modes still fail.
  • Negative controls were witnessed red before any of this was accepted, per the captain's verification discipline.

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

  • The Require no-mistakes workflow no longer greps the mutable pull-request body for a marker string. It now fetches refs/notes/no-mistakes and runs bin/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.
  • New bin/fm-attest.sh produces and verifies the attestation as a git note keyed by commit SHA (no branch rewrite, readable with contents: read), sourcing it from the pipeline's run record via bin/fm-nm-run-lib.sh with reads bounded by the shared timeout owner. Everything it emits goes through a single emit() 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.
  • Added 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, plus docs/no-mistakes-attestation.md documenting 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)

### SETUP: a fork (the head repository the gate reads) and a contributor clone
head under review: 92ee976e797685ea152135d09df91e7eb7f8b2be

### A stub 'no-mistakes' tool reports the pipeline's real run-record shape for this head

### CONTRIBUTOR: publish the attestation for the validated head
$ bin/fm-attest.sh write
fm-attest: recorded refs/notes/no-mistakes for 92ee976e797685ea152135d09df91e7eb7f8b2be
fm-attest: published refs/notes/no-mistakes to /tmp/fm-attest-e2e.fj2SAV/fork.git (the push target of origin)
[exit 0]

### The note now on the fork (SHA-keyed evidence, not PR prose):
$ git notes --ref=refs/notes/verify-roundtrip show 92ee976e797685ea152135d09df91e7eb7f8b2be
no-mistakes-attestation: v1
head: 92ee976e797685ea152135d09df91e7eb7f8b2be
run: 01KZ5YTADR5YAXZSNKFXTW8W9F
gates: intent,rebase,review,test,document,lint,push
tool: no-mistakes/v1.40.3
[exit 0]

### CI (scenario 1): checkout the head, fetch the notes ref, run the workflow's verify step -> PASS
warning: remote HEAD refers to nonexistent ref, unable to checkout
fm-attest: attested 92ee976e797685ea152135d09df91e7eb7f8b2be (run 01KZ5YTADR5YAXZSNKFXTW8W9F, gates intent,rebase,review,test,document,lint,push, no-mistakes/v1.40.3)
[check exit 0]

### CI (scenario 2): a NEW head with no attestation must FAIL CLOSED (absence is not evidence)
new head: aa5fc1c78eb2a823f24879b5ea47c249b969854f
warning: remote HEAD refers to nonexistent ref, unable to checkout
fm-attest: not attested (no-attestation-for-head)
  refs/notes/no-mistakes exists but carries no attestation for aa5fc1c78eb2a823f24879b5ea47c249b969854f.
  An attestation for any other commit says nothing about this one.
::error::This pull request carries no verified no-mistakes attestation for its head commit.

Contributions to this repository must be validated by 'git push no-mistakes',
which reviews, tests and lints the change before it is pushed. The proof of
that is a git note on refs/notes/no-mistakes naming the exact commit it covers:

    no-mistakes-attestation: v1
    head: <the 40-character lowercase sha of the commit this attests>
    run: <the pipeline run identity that validated it>
    gates: <comma-separated pipeline steps that completed for that head>
    tool: <the pipeline binary and version that ran them>

Publish it for the head under review with:

    bin/fm-attest.sh write

It reads the pipeline's own run record and refuses unless that run validated
this exact commit, so it cannot be satisfied by editing text. A new commit is
a new head and needs its own attestation.

Publishing the note does not re-run this check by itself. refs/notes/no-mistakes
is not a pull request head, so pushing it fires no pull request event and this
verdict stands until something re-evaluates the head. Once the note is published,
close and reopen this pull request, or edit its title or body; both re-run the
check, and because the verdict is bound to the head commit an unchanged head
simply re-derives its result from the evidence now present.

See CONTRIBUTING.md for setup and docs/no-mistakes-attestation.md for what
this check does and does not establish.

PR author: sbracewell64
[check exit 1]

### CI (scenario 3): FORGERY - the author copies the genuine note onto the new head -> refused as unbound
fm-attest: not attested (attestation-not-bound)
  The attestation attached to aa5fc1c78eb2a823f24879b5ea47c249b969854f attests 92ee976e797685ea152135d09df91e7eb7f8b2be instead.
  An attestation that does not name this exact commit does not cover it.
::error::This pull request carries no verified no-mistakes attestation for its head commit.

Contributions to this repository must be validated by 'git push no-mistakes',
which reviews, tests and lints the change before it is pushed. The proof of
that is a git note on refs/notes/no-mistakes naming the exact commit it covers:

    no-mistakes-attestation: v1
    head: <the 40-character lowercase sha of the commit this attests>
    run: <the pipeline run identity that validated it>
    gates: <comma-separated pipeline steps that completed for that head>
    tool: <the pipeline binary and version that ran them>

Publish it for the head under review with:

    bin/fm-attest.sh write

It reads the pipeline's own run record and refuses unless that run validated
this exact commit, so it cannot be satisfied by editing text. A new commit is
a new head and needs its own attestation.

Publishing the note does not re-run this check by itself. refs/notes/no-mistakes
is not a pull request head, so pushing it fires no pull request event and this
verdict stands until something re-evaluates the head. Once the note is published,
close and reopen this pull request, or edit its title or body; both re-run the
check, and because the verdict is bound to the head commit an unchanged head
simply re-derives its result from the evidence now present.

See CONTRIBUTING.md for setup and docs/no-mistakes-attestation.md for what
this check does and does not establish.

PR author: sbracewell64
[check exit 1]

### CI (scenario 4): verifier cannot run at all -> 'could not evaluate', distinct from a refusal, still fails
/tmp/fm-attest-e2e.fj2SAV/verify-step.sh: line 14: bin/fm-attest.sh: Permission denied
::error::This check could not evaluate the no-mistakes attestation for its head commit.

bin/fm-attest.sh exited 126, which is not a verdict on the evidence.
It reached no conclusion, so this says nothing about whether this head carries
an attestation: it says the check was unable to look. Do not read it as an
absent attestation, and do not publish one expecting it to clear this.

The verifier runs from this pull request's own head, so the usual causes are
that bin/fm-attest.sh is missing at this head, has lost its executable bit,
or that this head predates the check entirely. Rebase onto the base branch so
the head carries the verifier, then re-run this check.

The check fails, because a check that could not look must not report a pass.

See docs/no-mistakes-attestation.md for the error model these exits belong to.

PR author: sbracewell64
[check exit 1]

### OLD DEFECT GONE: the workflow never reads the pull request body
$ grep -c PR_BODY /home/shane/.no-mistakes/worktrees/5f306883d81c/01KZCZGW04XDBYRWF28GK0JJWY/.github/workflows/no-mistakes-required.yml
0
[exit 1]

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info
  • ⚠️ 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&#39; passes through unchanged, and 'https://alice:pw@host.invalid/r.git?token=SECRET&#39; 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 0
  • Manual end-to-end gate demo: contributor repo + fork bare remote + stubbed no-mistakes run record; bin/fm-attest.sh write recorded and pushed refs/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 SHA
  • Fail-closed check: new unattested head → verify step exit 1 with no-attestation-for-head refusal and contributor guidance
  • Forgery check: genuine note copied onto a different head → refused as attestation-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 1
  • grep -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 at docs/no-mistakes-attestation.md:76 and 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.

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.
@sbracewell64
sbracewell64 force-pushed the fm/honour-system-check-blocks-the-new-merge-guard branch from e6855d7 to c0128ef Compare August 7, 2026 11:16
@sbracewell64 sbracewell64 changed the title fix(bin): verify a head-bound no-mistakes attestation instead of PR body text fix(bin): verify a head-bound attestation instead of PR body text Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant