fix: retire the permanently red no-mistakes body check - #160
Open
quinnbot-ai wants to merge 1 commit into
Open
Conversation
The `PR must be raised via no-mistakes` check accepted exactly one thing: the signature the no-mistakes pipeline writes into a PR body. Firstmate's `direct-PR` delivery mode legitimately ships without that pipeline, so every direct-PR pull request failed the check permanently and by construction. PR #152 merged with it red twice and every other check green. That is no longer cosmetic. An absent check set now blocks a merge exactly as a red one does, while a check that is red on correct work teaches every reader to dismiss red - and it is what forced bin/fm-pr-verify-lib.sh's merge gate to answer only "is a check set present" rather than "is it green". Give direct-PR a legitimate way to be green instead of deleting the check. Two things now satisfy it: 1. the pipeline raised it (unchanged), or 2. a maintainer declared the bypass: the body carries `no-mistakes-bypass: direct-PR - <reason>` AND GitHub reports the author as OWNER, MEMBER, or COLLABORATOR. The author association is the gate on path 2 because any body marker is hand-writable, so a marker alone would only move the gaming target. GitHub computes the association from repository membership; a PR author cannot assert it about themselves. An outside contributor who copies the line still fails, and the declared reason is published to the check run, so a bypass is a recorded fact rather than a silent one. Exempting maintainers wholesale would have made it silent, which is what the check exists to prevent. The verdict moves out of inline workflow bash into bin/fm-pr-body-compliance.sh so a portable regression can drive it through a real interface rather than asserting YAML bytes. The job name stays byte-stable because it may be a required status check on a protecting branch. Correct the two places that documented the false red as a standing exception, so neither keeps licensing "that one is always red".
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.
no-mistakes-bypass: direct-PR - internal contributor-process change to the PR compliance check itself, shipped direct-PR
Summary
The
PR must be raised via no-mistakescheck accepted exactly one thing: the deterministic signature the no-mistakes pipeline writes into a PR body. Firstmate'sdirect-PRdelivery mode legitimately ships without that pipeline, so every direct-PR pull request failed this check permanently and by construction. PR #152 merged with it red twice and every other check green.That stopped being cosmetic when an absent check set began blocking a merge exactly as a red one does. A check that is red on correct work teaches every reader to dismiss red, and it is what forced
bin/fm-pr-verify-lib.sh's merge gate to answer only "is a check set present" rather than "is it green".What changed
The check was not deleted - it exists to stop pipeline-bypassing deliveries going unnoticed. Instead,
direct-PRgot a legitimate way to be green. Two things now satisfy it:no-mistakes-bypass: direct-PR - <reason>and GitHub reports the author asOWNER,MEMBER, orCOLLABORATOR.Why the author association is the gate on path 2
Any body marker is hand-writable, so a marker alone would only move the gaming target - which is exactly the objection a worker raised when it declined to hand-write the pipeline signature. GitHub computes
author_associationfrom repository membership; a PR author cannot assert it about themselves. So an outside contributor who copies the bypass line still fails, and only people actually entitled to choose the delivery mode can take path 2.Two shapes were deliberately rejected:
OWNERstill fails.The declared reason is published to the check run's summary, so a bypass is a recorded, reviewable fact.
Residual limitation, stated honestly: a maintainer could still hand-write path 1's pipeline signature. No body-content check can catch that, and pinning the script to the base ref would not help either -
pull_requestworkflows run from the PR's own merge ref. What changed is that nobody needs to: faking path 1 is now a deliberate misrepresentation rather than the only route past a check that could never pass.Structure
The verdict moved out of inline workflow
run:bash intobin/fm-pr-body-compliance.sh, so a portable regression can drive it through a real interface instead of asserting YAML bytes, and so the marker strings have one owner. The job name is byte-stable because it may be a required status check on a protecting branch.Doctrinal cleanup
bin/fm-pr-verify-lib.shanddocs/verification/pr-check-set-gate.mdboth documented the false red as a standing exception. Both are corrected: no check here is expected to be red on a healthy delivery now, so a red check is a stop-and-read result.Verification
Base point, real CI. On current
main, PR #152 (author_association: OWNER, direct-PR):PR must be raised via no-mistakes→ fail, twice, with all 12 other checks green. That is the symptom.Fixed head, real CI. This PR's body carries the declaration and its author is
OWNER, so the same check should report green on this run.pull_requestworkflows run from the merge ref, so this PR validates itself.Smallest-mutation broken heads. One single-site mutation per load-bearing part, each reproducing a different symptom:
BYPASS_VALIDbranch →if falsefm_has_write_access→if trueexit 0for write access--author-associationdefaulted, not requiredfm_trimclass narrowed to space/tabreasonguardOne decoration found and removed. An explicit
BODY=${BODY//$'\r'/}normalization survived every mutation:fm_trim's[[:space:]]class already covers the carriage return, and with both removed the suite still passed - meaning the CRLF test was vacuous. The redundant line is gone,fm_trimis documented as the real owner of CR handling, and two cases were added that make that class load-bearing (a CRLF declaration with an empty reason, and no CR in the published verdict). M5 above is the re-run against the real owner.Gates run locally:
bin/fm-lint.sh(shellcheck 0.11.0 + actionlint 1.7.12, 3 workflows valid) exit 0 ·bin/fm-test-run.sh --check-coverageok (155 total) ·bin/fm-doc-audience-check.shok ·tests/fm-pr-body-compliance.test.sh,tests/fm-pr-merge.test.sh,tests/fm-documentation-audiences.test.shpass · full suite re-run under stock macOS bash 3.2.57 passes. The new test lands in CI serial shard 3 automatically.