From af030275574e471ba889a86ce34fc95759d469fa Mon Sep 17 00:00:00 2001 From: QuinnBot Date: Thu, 20 Aug 2026 16:45:15 -0700 Subject: [PATCH] fix: retire the permanently red no-mistakes body check 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 - ` 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". --- .../firstmate-coding-guidelines/SKILL.md | 13 + .github/workflows/no-mistakes-required.yml | 40 +-- CONTRIBUTING.md | 12 +- bin/fm-pr-body-compliance.sh | 283 ++++++++++++++++++ bin/fm-pr-verify-lib.sh | 17 +- docs/verification/pr-check-set-gate.md | 16 +- tests/fm-pr-body-compliance.test.sh | 169 +++++++++++ 7 files changed, 522 insertions(+), 28 deletions(-) create mode 100755 bin/fm-pr-body-compliance.sh create mode 100755 tests/fm-pr-body-compliance.test.sh diff --git a/.agents/skills/firstmate-coding-guidelines/SKILL.md b/.agents/skills/firstmate-coding-guidelines/SKILL.md index a9e21543077..6293569b09f 100644 --- a/.agents/skills/firstmate-coding-guidelines/SKILL.md +++ b/.agents/skills/firstmate-coding-guidelines/SKILL.md @@ -73,6 +73,19 @@ Briefs for tasks that touch firstmate's own tracked material should tell the cre Firstmate adds this skill's load instruction to firstmate-repo briefs by hand instead. `CONTRIBUTING.md`'s "Development" section carries the same instruction as a durable reminder. +## Declaring a direct-PR delivery on this repo + +This repo's `Require no-mistakes` check reads the PR body for the delivery path that raised the pull request. +A `direct-PR` delivery does not run the pipeline, so its PR body must carry one declaration line or the check is red: + +``` +no-mistakes-bypass: direct-PR - +``` + +`CONTRIBUTING.md` owns the contract and `bin/fm-pr-body-compliance.sh` owns the verdict. +Write the declaration into the body when opening the PR rather than waiting for the red check. +The declaration is not a formality to satisfy a check: it is the record that a delivery skipped the pipeline, and it only counts because GitHub reports the author as a maintainer, so never hand-write the pipeline's own signature to go green instead. + ## Compatibility and enforcement Before changing shared tracked behavior, review every affected supported primary harness and runtime backend rather than checking only the adapters active in the current fleet. diff --git a/.github/workflows/no-mistakes-required.yml b/.github/workflows/no-mistakes-required.yml index f56afee4188..c64a744feef 100644 --- a/.github/workflows/no-mistakes-required.yml +++ b/.github/workflows/no-mistakes-required.yml @@ -19,6 +19,9 @@ concurrency: cancel-in-progress: true jobs: + # The job NAME is the published check name and may be a required status check + # on a protecting branch, so it stays byte-stable across changes to what the + # check actually decides. bin/fm-pr-body-compliance.sh owns that decision. check: name: PR must be raised via no-mistakes runs-on: ubuntu-latest @@ -26,29 +29,32 @@ jobs: github.event.pull_request.user.login != 'github-actions[bot]' && github.event.pull_request.user.login != 'dependabot[bot]' steps: - - name: Verify no-mistakes signature in PR body + - uses: actions/checkout@v6 + - name: Verify declared delivery path in PR body env: PR_BODY: ${{ github.event.pull_request.body }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} + PR_AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | set -eu - marker='Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes)' - if printf '%s' "${PR_BODY:-}" | grep -qF -- "$marker"; then - echo "Found no-mistakes signature in PR #${PR_NUMBER} body." + # Pass the body through a file rather than an argument: a PR body is + # attacker-controlled text of arbitrary length, and a file keeps it + # data instead of anything the shell could reinterpret. + printf '%s' "${PR_BODY:-}" > "$RUNNER_TEMP/pr-body.txt" + if bin/fm-pr-body-compliance.sh \ + --body-file "$RUNNER_TEMP/pr-body.txt" \ + --author-association "${PR_AUTHOR_ASSOCIATION:-}" \ + --author "${PR_AUTHOR:-}" \ + --pr "${PR_NUMBER:-}" > "$RUNNER_TEMP/verdict.txt"; then + cat "$RUNNER_TEMP/verdict.txt" + # Publish which path was taken, so a declared bypass is a visible + # record on the run rather than a green tick that says nothing. + { + echo '### Delivery path' + echo + sed 's/^/- /' "$RUNNER_TEMP/verdict.txt" + } >> "$GITHUB_STEP_SUMMARY" exit 0 fi - { - echo "::error::This PR was not raised through no-mistakes." - echo - echo "Contributions to this repository must be submitted via 'git push no-mistakes'." - echo "That pipeline runs the required review/test/lint/CI steps and writes a" - echo "deterministic '## Pipeline' section into the PR body containing:" - echo - echo " $marker" - echo - echo "See CONTRIBUTING.md for setup and the full workflow." - echo - echo "PR author: ${PR_AUTHOR}" - } >&2 exit 1 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cef1f1180f1..2815563c64b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,10 +9,20 @@ We require this to reduce the maintainer's burden of reviewing and merging contr `no-mistakes` puts a local git proxy in front of your real remote. Pushing through it runs an AI-driven review/test/lint pipeline in an isolated worktree, forwards the push upstream only after every check passes, and opens a clean PR automatically. -A GitHub Actions check (`Require no-mistakes`) runs on PRs targeting `main` and fails if the body is missing the deterministic signature that no-mistakes writes. +A GitHub Actions check (`Require no-mistakes`) runs on PRs targeting `main` and fails unless the PR body shows which delivery path raised it. It evaluates every PR opening and body edit independently, so a later edit cannot replace an earlier pending compliance check. GitHub Actions and Dependabot are exempt so their automation keeps working, but regular contributor PRs without the signature will not be reviewed or merged. +Two things satisfy the check, and `bin/fm-pr-body-compliance.sh` owns the verdict: + +1. **The pipeline raised it.** The body carries the deterministic signature no-mistakes writes. + This is the path for every contribution. +2. **A maintainer declared a bypass.** The body carries one line of the form `no-mistakes-bypass: direct-PR - `, and GitHub reports the PR author as an owner, member, or collaborator of this repository. + +The second path exists because maintainers ship some internal work without the pipeline, and a check that failed on correct work would train everyone to dismiss red. +It is not a way around the first path: GitHub computes the author association, so a contributor cannot grant it to themselves by copying the line, and the declared reason is published to the check run so a bypass is a recorded fact rather than a silent one. +A red `Require no-mistakes` therefore always means a real problem - the pipeline was skipped and nobody with the standing to skip it said why. + ## Workflow 1. Fork the repo, then clone the parent repo or set your local `origin` back to the parent (`git@github.com:kunchenguid/firstmate.git`). diff --git a/bin/fm-pr-body-compliance.sh b/bin/fm-pr-body-compliance.sh new file mode 100755 index 00000000000..8db8f6359ba --- /dev/null +++ b/bin/fm-pr-body-compliance.sh @@ -0,0 +1,283 @@ +#!/usr/bin/env bash +# fm-pr-body-compliance.sh - single owner of the "was this pull request raised +# through a declared delivery path" verdict that +# .github/workflows/no-mistakes-required.yml publishes as the +# `PR must be raised via no-mistakes` check. +# +# WHY THIS EXISTS AS A SCRIPT RATHER THAN INLINE WORKFLOW BASH. The verdict is +# a safety contract, so it needs a portable regression that drives it through a +# real interface. Inline `run:` bash can only be tested by reading the YAML, +# which asserts implementation bytes instead of behavior. Keeping the decision +# here also gives the marker strings exactly one owner. +# +# WHAT WENT WRONG BEFORE (quinnbot-ai/firstmate, 2026-08-20). The check used to +# accept exactly one thing: the signature the no-mistakes pipeline writes into a +# PR body. Firstmate's own `direct-PR` delivery mode legitimately ships without +# that pipeline, so every single direct-PR pull request failed this check +# permanently and by construction - PR #152 merged with the check red twice and +# every other check green. A check that is red on correct work teaches everyone +# reading it to dismiss red, which is the exact habit that lets a real failure +# through, 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". +# +# THE RULE NOW. There are two ways to comply, and a red result means neither +# happened: +# +# 1. The pipeline raised it. The body carries no-mistakes' deterministic +# signature. +# 2. A maintainer declared the bypass. The body carries a +# `no-mistakes-bypass: - ` line AND GitHub itself reports +# the author as having write access to the repository. +# +# WHY THE AUTHOR ASSOCIATION IS THE GATE ON PATH 2. Any body marker is +# hand-writable, so a marker alone would just move the gaming target. GitHub +# computes `author_association` from repository membership; a pull request +# author cannot assert it about themselves. So an outside contributor who +# copies the bypass line still fails, and the only people who can take path 2 +# are the people who are actually entitled to choose the delivery mode. The +# declaration then makes the bypass a recorded, reviewable fact instead of a +# silent one, which is what the original check was reaching for. +# +# RESIDUAL LIMITATION, STATED HONESTLY. A maintainer could still hand-write +# path 1's pipeline signature and misrepresent an unvalidated PR as pipeline +# raised. Nothing a body-content check can read would catch that, and pinning +# this script to the base ref would not help either, because `pull_request` +# workflows run from the PR's own merge ref and can be edited by the PR. What +# changed is that nobody needs to: path 2 is a legitimate, honest way to be +# green, so faking path 1 is now a deliberate misrepresentation rather than the +# only route past a check that could never pass. +# +# Usage: +# fm-pr-body-compliance.sh --body-file --author-association +# [--author ] [--pr ] +# +# --body-file PR body to inspect; `-` reads STDIN. +# --author-association GitHub's computed author_association for the PR. +# Must be passed, may be empty; anything outside the +# write-access set below is treated as no write +# access, so an unexpected or missing payload value +# refuses the bypass rather than granting it. +# --author / --pr Identifiers echoed into the failure report only. +# +# Exit status: 0 compliant, 1 non-compliant, 2 usage error. +# Compliant runs print one verdict line to STDOUT; the caller may publish it. +# Non-compliant runs print the actionable guidance to STDERR. +# +# No side effects. set -u / set -e safe. + +set -eu + +# The deterministic signature `no-mistakes` writes into a PR body it raised. +# Fixed string, compared literally; CONTRIBUTING.md quotes it for contributors. +FM_PIPELINE_MARKER='Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes)' + +# Line prefix a maintainer uses to declare a pipeline bypass. +FM_BYPASS_PREFIX='no-mistakes-bypass:' + +# Delivery modes that legitimately reach a pull request without the pipeline. +# `local-only` never opens one and `no-mistakes` is path 1, so `direct-PR` is +# the whole list. An unrecognized mode is refused rather than waved through. +FM_BYPASS_MODES='direct-PR' + +# GitHub author_association values that mean write access to this repository. +# GitHub computes these; a pull request author cannot claim one. +FM_WRITE_ASSOCIATIONS='OWNER MEMBER COLLABORATOR' + +usage() { + cat >&2 <<'USAGE' +usage: fm-pr-body-compliance.sh --body-file --author-association + [--author ] [--pr ] +USAGE + exit 2 +} + +BODY_FILE= +AUTHOR_ASSOCIATION= +HAVE_ASSOCIATION=0 +AUTHOR= +PR_NUMBER= + +while [ $# -gt 0 ]; do + case "$1" in + --body-file) + [ $# -ge 2 ] || usage + BODY_FILE=$2 + shift 2 + ;; + --author-association) + [ $# -ge 2 ] || usage + AUTHOR_ASSOCIATION=$2 + HAVE_ASSOCIATION=1 + shift 2 + ;; + --author) + [ $# -ge 2 ] || usage + AUTHOR=$2 + shift 2 + ;; + --pr) + [ $# -ge 2 ] || usage + PR_NUMBER=$2 + shift 2 + ;; + -h|--help) + sed -n '/^# Usage:/,/^# No side effects/p' "$0" | sed 's/^# \{0,1\}//' + exit 0 + ;; + *) + echo "error: unknown argument '$1'" >&2 + usage + ;; + esac +done + +[ -n "$BODY_FILE" ] || usage +# Required rather than defaulted: a caller that forgot to wire the association +# through would otherwise reach the bypass path with an empty value on every +# pull request, which is the one mistake that would make this check meaningless +# again. +[ "$HAVE_ASSOCIATION" -eq 1 ] || { + echo "error: --author-association is required (may be empty, but must be passed)" >&2 + usage +} + +if [ "$BODY_FILE" = - ]; then + BODY=$(cat) || BODY= +else + [ -f "$BODY_FILE" ] || { echo "error: body file not found: $BODY_FILE" >&2; exit 2; } + BODY=$(cat -- "$BODY_FILE") || BODY= +fi + +fm_has_write_access() { + local want=$1 assoc + for assoc in $FM_WRITE_ASSOCIATIONS; do + [ "$want" = "$assoc" ] && return 0 + done + return 1 +} + +fm_is_bypass_mode() { + local want=$1 mode + for mode in $FM_BYPASS_MODES; do + [ "$want" = "$mode" ] && return 0 + done + return 1 +} + +# Normalizes one field of a declaration line. `[[:space:]]` is load-bearing +# beyond cosmetics: GitHub delivers PR bodies with CRLF endings, so this is what +# keeps a trailing carriage return out of the delivery mode, out of the reason's +# non-empty test, and out of the verdict this check publishes. Narrowing the +# class to space and tab would let ` - \r` read as a declared reason. +fm_trim() { + local s=$1 + s=${s#"${s%%[![:space:]]*}"} + s=${s%"${s##*[![:space:]]}"} + printf '%s' "$s" +} + +# --- path 1: raised through the pipeline ------------------------------------- + +if printf '%s' "$BODY" | grep -qF -- "$FM_PIPELINE_MARKER"; then + echo "Compliant: raised through no-mistakes (pipeline signature found in the PR body)." + exit 0 +fi + +# --- path 2: maintainer-declared bypass -------------------------------------- + +# Scan every declaration line rather than only the first, so one malformed +# attempt above a corrected one does not fail the pull request. +BYPASS_SEEN=0 +BYPASS_MODE= +BYPASS_REASON= +BYPASS_VALID=0 +BAD_MODE= +while IFS= read -r line; do + trimmed=$(fm_trim "$line") + case "$trimmed" in + "$FM_BYPASS_PREFIX"*) ;; + *) continue ;; + esac + BYPASS_SEEN=1 + rest=$(fm_trim "${trimmed#"$FM_BYPASS_PREFIX"}") + case "$rest" in + *" - "*) + mode=$(fm_trim "${rest%%" - "*}") + reason=$(fm_trim "${rest#*" - "}") + ;; + *) + mode=$(fm_trim "$rest") + reason= + ;; + esac + if ! fm_is_bypass_mode "$mode"; then + [ -n "$BAD_MODE" ] || BAD_MODE=$mode + continue + fi + [ -n "$reason" ] || continue + BYPASS_MODE=$mode + BYPASS_REASON=$reason + BYPASS_VALID=1 + break +done <}', which does not carry" + echo "write access. The declaration is not something a contributor can grant themselves." + echo + echo "Raise this PR through the pipeline instead:" + echo + echo " git push no-mistakes" + echo + echo "See CONTRIBUTING.md for setup and the full workflow." + echo + [ -n "$AUTHOR" ] && echo "PR author: ${AUTHOR}" + [ -n "$PR_NUMBER" ] && echo "PR number: ${PR_NUMBER}" + } >&2 + exit 1 +fi + +# --- neither path satisfied --------------------------------------------------- + +{ + echo "::error::This PR was neither raised through no-mistakes nor declared as a maintainer bypass." + echo + echo "Contributions to this repository must be submitted via 'git push no-mistakes'." + echo "That pipeline runs the required review/test/lint/CI steps and writes a" + echo "deterministic '## Pipeline' section into the PR body containing:" + echo + echo " $FM_PIPELINE_MARKER" + echo + echo "See CONTRIBUTING.md for setup and the full workflow." + if [ "$BYPASS_SEEN" -eq 1 ]; then + echo + if [ -n "$BAD_MODE" ]; then + echo "This body carries a bypass declaration naming an unrecognized delivery mode" + echo "'${BAD_MODE}'. Recognized modes: ${FM_BYPASS_MODES}." + else + echo "This body carries a bypass declaration with no reason after the ' - ' separator." + fi + echo "The declaration must read exactly:" + else + echo + echo "MAINTAINERS ONLY: a delivery that legitimately bypasses the pipeline is" + echo "declared in the PR body rather than left to fail this check. Add one line:" + fi + echo + echo " $FM_BYPASS_PREFIX direct-PR - " + echo + [ -n "$AUTHOR" ] && echo "PR author: ${AUTHOR}" + [ -n "$PR_NUMBER" ] && echo "PR number: ${PR_NUMBER}" +} >&2 +exit 1 diff --git a/bin/fm-pr-verify-lib.sh b/bin/fm-pr-verify-lib.sh index 64f88da1e03..2a198b19c7e 100755 --- a/bin/fm-pr-verify-lib.sh +++ b/bin/fm-pr-verify-lib.sh @@ -22,12 +22,17 @@ # WHAT THIS LIB DECIDES, AND WHAT IT DELIBERATELY DOES NOT. It answers only # "does the head commit carry a check set at all". It does NOT adjudicate red # versus green, because a red check set is a DIFFERENT state that is already -# visible to whoever is merging, and because a repository can legitimately carry -# a permanently failing advisory check - this one does: "PR must be raised via -# no-mistakes" fails by design on every direct-PR task. Refusing red here would -# block every such merge. AGENTS.md section 7's "never merge a red PR" rule -# keeps owning that judgement; this lib owns only the state nothing else could -# see. +# visible to whoever is merging. AGENTS.md section 7's "never merge a red PR" +# rule keeps owning that judgement; this lib owns only the state nothing else +# could see. +# +# THAT SPLIT IS NO LONGER PROPPED UP BY A FALSE RED. This comment used to add a +# second reason: the repo carried "PR must be raised via no-mistakes", which +# failed by design on every direct-PR delivery, so refusing red would have +# blocked every such merge. bin/fm-pr-body-compliance.sh retired that permanent +# false red on 2026-08-20. No check here is expected to be red on a healthy +# delivery now, so a red check is a stop-and-read result rather than a +# documented exception - which is the whole point of section 7 owning it. # # FAIL CLOSED ON AN UNREADABLE ANSWER. gh-axi renders a scalar sometimes bare # and sometimes wrapped in an `api_response:`/`body:` envelope, and it reports diff --git a/docs/verification/pr-check-set-gate.md b/docs/verification/pr-check-set-gate.md index b554fcd753c..963b1cce1df 100644 --- a/docs/verification/pr-check-set-gate.md +++ b/docs/verification/pr-check-set-gate.md @@ -72,12 +72,20 @@ The verdict for 148 names the conflict as the cause; 154 reported 13 check runs ## Why the gate does not adjudicate red -This repository carries a check that fails by design on every direct-PR task: `PR must be raised via no-mistakes`. -On 2026-08-20 head `b553350e` reported 13 check runs of which three concluded `failure`, and pull requests in that state are merged deliberately. -A gate that refused red here would block every direct-PR merge, so the check-set gate answers only presence, and `AGENTS.md` section 7 keeps owning the red judgment. +The check-set gate answers only "does this head carry a check set at all", because red versus green is a different state that is already visible to whoever is merging, and `AGENTS.md` section 7's "never merge a red PR" rule keeps owning that judgment. + +That division of labour was originally forced rather than chosen. +Until 2026-08-20 this repository carried a check that failed by design on every direct-PR delivery: `PR must be raised via no-mistakes` accepted only the signature the no-mistakes pipeline writes, which a direct-PR delivery legitimately never has. +On 2026-08-20 head `b553350e` reported 13 check runs of which three concluded `failure`, and pull requests in that state were merged deliberately; PR #152 merged with that check red twice and every other check green. +A gate that refused red would have blocked every direct-PR merge. + +That permanent false red is retired. +`bin/fm-pr-body-compliance.sh` now accepts either the pipeline signature or a maintainer-declared bypass, so a red `PR must be raised via no-mistakes` again means an undeclared pipeline bypass rather than routine correct work. +No check in this repository is expected to be red on a healthy delivery, and a red one is a stop-and-read result rather than a documented exception to skim past. ## Regression coverage `tests/fm-pr-merge.test.sh` covers the merge refusal for an absent check set, the same refusal for an unreadable one, and the explicit `--allow-unverified` override. `tests/fm-crew-state.test.sh` covers the `unverified` classification, including the case where the worker's own status line claims checks are green while the CI log reports none. -Both suites run under `bin/fm-test-run.sh`. +`tests/fm-pr-body-compliance.test.sh` covers the delivery-path compliance verdict that retired the permanent false red, including the refusal of an undeclared bypass by a maintainer and of a declaration by an author without write access. +All three suites run under `bin/fm-test-run.sh`. diff --git a/tests/fm-pr-body-compliance.test.sh b/tests/fm-pr-body-compliance.test.sh new file mode 100755 index 00000000000..33069257c60 --- /dev/null +++ b/tests/fm-pr-body-compliance.test.sh @@ -0,0 +1,169 @@ +#!/usr/bin/env bash +# Behavioral regressions for the PR delivery-path compliance verdict owned by +# bin/fm-pr-body-compliance.sh and published by +# .github/workflows/no-mistakes-required.yml as `PR must be raised via no-mistakes`. +# +# Regression origin (quinnbot-ai/firstmate, 2026-08-20): the check accepted only +# the no-mistakes pipeline signature, so every `direct-PR` delivery failed it +# permanently and by construction. PR #152 merged with this check red twice and +# every other check green, and the merge gate in bin/fm-pr-verify-lib.sh had to +# refuse to adjudicate red at all because of it. A check that is red on correct +# work trains everyone to dismiss red. +# +# The properties pinned here are what make the check meaningful when red: +# - correct work has a green path (pipeline, or a declared maintainer bypass); +# - a bypass is never silent, so a maintainer alone is not enough; +# - a bypass declaration is not self-grantable by a contributor; +# - a miswired caller fails closed rather than granting the bypass to everyone. +set -u + +# shellcheck source=tests/lib.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +CHECK="$ROOT/bin/fm-pr-body-compliance.sh" +TMP_ROOT=$(fm_test_tmproot fm-pr-body-compliance) + +PIPELINE_SIGNATURE='Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes)' +DECLARATION='no-mistakes-bypass: direct-PR - internal-only tooling change, shipped without the pipeline' + +OUT= +ERR= +CODE= + +# run_check [extra args...] -> sets OUT (stdout), ERR (stderr), CODE. +run_check() { + local body=$1 + shift + printf '%s' "$body" > "$TMP_ROOT/body.txt" + OUT=$("$CHECK" --body-file "$TMP_ROOT/body.txt" "$@" 2>"$TMP_ROOT/err.txt") + CODE=$? + ERR=$(cat "$TMP_ROOT/err.txt") +} + +test_pipeline_signature_is_compliant() { + local assoc + for assoc in OWNER MEMBER COLLABORATOR CONTRIBUTOR FIRST_TIME_CONTRIBUTOR NONE; do + run_check "## Pipeline"$'\n\n'"$PIPELINE_SIGNATURE"$'\n' --author-association "$assoc" + expect_code 0 "$CODE" "pipeline-raised PR rejected for author association $assoc" + assert_contains "$OUT" 'raised through no-mistakes' \ + "pipeline-raised PR did not report the pipeline path for $assoc" + done + pass "a PR carrying the pipeline signature is compliant for every author association" +} + +test_undeclared_bypass_still_fails_for_a_maintainer() { + # The fix must NOT be "exempt maintainers". A delivery that skips the pipeline + # has to say so, or the bypass goes unnoticed - the thing the check exists to + # prevent. + local assoc + for assoc in OWNER MEMBER COLLABORATOR; do + run_check '## Summary'$'\n\n''Ordinary direct-PR body with nothing declared.'$'\n' \ + --author-association "$assoc" --author quinnbot-ai --pr 152 + expect_code 1 "$CODE" "an undeclared pipeline bypass passed for author association $assoc" + done + # Red has to be actionable: the failure names the exact line to add and says + # who may add it, so the author can fix it instead of learning to ignore it. + assert_contains "$ERR" 'no-mistakes-bypass: direct-PR - ' \ + "failure report omitted the declaration line the author must add" + assert_contains "$ERR" 'MAINTAINERS ONLY' \ + "failure report did not say who may declare a bypass" + pass "a maintainer skipping the pipeline without declaring it still fails" +} + +test_declared_bypass_is_compliant_for_write_access() { + local assoc + for assoc in OWNER MEMBER COLLABORATOR; do + run_check '## Summary'$'\n\n'"$DECLARATION"$'\n' --author-association "$assoc" + expect_code 0 "$CODE" "declared bypass rejected for write-access association $assoc" + assert_contains "$OUT" 'declared pipeline bypass, mode direct-PR' \ + "compliant bypass did not record the declared delivery mode for $assoc" + done + # The recorded reason is the whole point of declaring: it turns a bypass into + # a reviewable fact on the run instead of a silent one. + assert_contains "$OUT" 'internal-only tooling change, shipped without the pipeline' \ + "compliant bypass did not record the declared reason" + pass "a declared direct-PR bypass by a write-access author is compliant and recorded" +} + +test_declared_bypass_is_not_self_grantable() { + # A body marker anyone can type would only move the gaming target. The gate is + # GitHub's own computed author_association, which a PR author cannot assert + # about themselves. + local assoc + for assoc in CONTRIBUTOR FIRST_TIME_CONTRIBUTOR FIRST_TIMER MANNEQUIN NONE '' UNKNOWN_FUTURE_VALUE; do + run_check '## Summary'$'\n\n'"$DECLARATION"$'\n' --author-association "$assoc" --author mallory + expect_code 1 "$CODE" "a bypass declaration granted itself write access as '$assoc'" + assert_contains "$ERR" 'available only to maintainers' \ + "refusal for '$assoc' did not explain that the declaration is maintainer-only" + assert_contains "$ERR" 'git push no-mistakes' \ + "refusal for '$assoc' did not point at the pipeline as the way through" + done + pass "a bypass declaration cannot grant its own author write access" +} + +test_missing_association_fails_closed() { + # A caller that forgot to wire author_association through would otherwise + # reach the bypass path with an empty value on every PR, quietly making the + # check meaningless again. That is a usage error, not a pass. + printf '%s' "$DECLARATION" > "$TMP_ROOT/body.txt" + "$CHECK" --body-file "$TMP_ROOT/body.txt" >/dev/null 2>&1 + expect_code 2 "$?" "omitting --author-association was not a usage error" + # An explicitly empty association is a real payload value, and must refuse. + run_check "$DECLARATION" --author-association '' + expect_code 1 "$CODE" "an empty author association granted the bypass" + pass "a miswired or empty author association refuses the bypass" +} + +test_github_crlf_bodies_are_handled() { + # GitHub delivers PR bodies with CRLF line endings; a trailing carriage return + # would otherwise make the declared mode read as 'direct-PR' and fail. + run_check $'## Summary\r\n\r\n'"$DECLARATION"$'\r\nmore text\r\n' --author-association OWNER + expect_code 0 "$CODE" "a CRLF PR body broke the declared bypass" + assert_contains "$OUT" 'mode direct-PR' "CRLF body produced a malformed delivery mode" + # The carriage return must not survive into the published verdict, which is + # what the workflow writes into the run summary as the bypass record. + case "$OUT" in + *$'\r'*) fail "the published verdict carried a stray carriage return" ;; + esac + + # The case where the carriage return actually decides the verdict: a CRLF + # declaration with no reason must still be refused, not read as reason='\r'. + run_check $'no-mistakes-bypass: direct-PR - \r\n' --author-association OWNER + expect_code 1 "$CODE" "a CRLF-only bypass reason was accepted as a declared reason" + pass "a CRLF PR body is read the same as an LF one" +} + +test_malformed_declarations_are_refused_with_the_reason() { + run_check 'no-mistakes-bypass: yolo - because I said so'$'\n' --author-association OWNER + expect_code 1 "$CODE" "an unrecognized delivery mode was accepted as a bypass" + assert_contains "$ERR" "'yolo'" "refusal did not name the unrecognized delivery mode" + + run_check 'no-mistakes-bypass: direct-PR'$'\n' --author-association OWNER + expect_code 1 "$CODE" "a bypass declaration with no reason was accepted" + assert_contains "$ERR" 'no reason' "refusal did not explain the missing reason" + + run_check 'no-mistakes-bypass: direct-PR - '$'\n' --author-association OWNER + expect_code 1 "$CODE" "a whitespace-only bypass reason was accepted" + + # A corrected declaration below a malformed attempt is what an author actually + # produces when fixing the body; it must be accepted. + run_check 'no-mistakes-bypass: yolo - bad first attempt'$'\n'"$DECLARATION"$'\n' \ + --author-association OWNER + expect_code 0 "$CODE" "a corrected declaration below a malformed one was not accepted" + pass "malformed declarations are refused and a corrected one is accepted" +} + +test_empty_body_fails() { + run_check '' --author-association OWNER + expect_code 1 "$CODE" "an empty PR body was treated as compliant" + pass "an empty PR body is not compliant" +} + +test_pipeline_signature_is_compliant +test_undeclared_bypass_still_fails_for_a_maintainer +test_declared_bypass_is_compliant_for_write_access +test_declared_bypass_is_not_self_grantable +test_missing_association_fails_closed +test_github_crlf_bodies_are_handled +test_malformed_declarations_are_refused_with_the_reason +test_empty_body_fails