From 099afd05ce283b0d2dc6f50867fdc680f8398c24 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 18 Aug 2026 00:01:05 +1200 Subject: [PATCH 1/5] ci(issue-refs): in-file opt-outs at line, block and file scope; honour the PR-body opt-out locally; repeat guidance after the hits Four defects in one opt-out mechanism, found when an agent hit the gate with 25 refs in a generated document: 1. The guidance was invisible. The failure message printed the opt-out instructions ABOVE the hits list, and a failing tool is read from its tail - often literally, through `| tail`. On 2026-08-17 an agent hand-qualified 25 refs, some inside quotations, never learning any opt-out existed; the header had said so all along, 23 hits earlier. formatFailure now repeats a fix/opt-out reminder AFTER the hits, with a unit test pinning the position. 2. There was no in-file opt-out at all - the only escape hatch was the whole-PR hammer, and it lived in the PR body. New, narrowest first, all written in the file's own comment syntax (invisible when rendered in markdown/HTML): issue-refs: exempt that line only issue-refs: exempt-begin/ a pasted block - for quoted material too issue-refs: exempt-end dense to mark per line without being a mess issue-refs: exempt-file a whole file (a generated doc, an imported archive) - the in-file equivalent of an EXEMPT_PATHS entry, without editing the gate These travel with the file: they hold for future PRs and for local runs with no PR body. Line/block markers govern added patch lines (same-line/ same-patch by necessity - a marker outside the patch is invisible to a patch-based gate); the file marker is judged against file CONTENT by both callers (local: disk; CI: contents API, fetched only for files with hits), so it holds however small the diff. An unclosed exempt-begin swallows the rest of the file's added lines, mirroring the unclosed-fence rule. A backslash escape (\#857) was rejected: it edits the text it is meant to preserve, renders visibly outside markdown, and is an illegal escape in a Java string literal. 3. The PR-body opt-out was one-sided. bin/check-issue-refs.sh never read bodies, so an opt-out CI accepted left every local run red on the same file forever - a "local mirror of CI" permanently disagreeing with CI. The script now resolves the current branch's PR via `gh pr view` (repo derived from origin), honours the opt-out exactly as pr-checklist.yml does, and scans the body as a synthetic file too, pre-empting the CI round-trip for body refs. Fetch failure (no PR yet, no gh, offline) degrades to the old body-less behaviour, and the message now says "could not read the PR body (no PR yet, or gh unavailable)" instead of implying it never does. 4. Nothing tested the local script's plumbing. New bin/test-check-issue-refs.sh stubs gh on PATH with eight end-to-end cases (no-PR fallback both ways, line and file opt-outs from disk, PR-body opt-out honoured, body ref flagged, clean pass naming the body, the reminder surviving `| tail -3`), runs in pr-checklist.yml beside the gate module's unit tests, and dogfoods the line marker on its own fixtures. Verified red against master's code with the new tests kept in place: 7 of 8 harness cases fail and the unit-test run (59 assertions on the new code) aborts on the first missing feature. All checks green on the new code: unit tests, harness, sigpipe guard, actionlint, and the gate itself run against this branch. docs/issue-references.md rewritten where this change falsified it ("purely textual, no API calls"; "the PR body ... does not exist when you run the script") and now documents all four opt-out scopes, narrowest first. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn --- .github/scripts/issue-ref-gate.js | 72 ++++++++++++-- .github/scripts/issue-ref-gate.test.js | 100 ++++++++++++++++++- .github/workflows/pr-checklist.yml | 28 +++++- bin/check-issue-refs.sh | 57 +++++++++-- bin/test-check-issue-refs.sh | 128 +++++++++++++++++++++++++ docs/issue-references.md | 50 ++++++++-- 6 files changed, 410 insertions(+), 25 deletions(-) create mode 100755 bin/test-check-issue-refs.sh diff --git a/.github/scripts/issue-ref-gate.js b/.github/scripts/issue-ref-gate.js index cf89e5e08..d28be2694 100644 --- a/.github/scripts/issue-ref-gate.js +++ b/.github/scripts/issue-ref-gate.js @@ -50,10 +50,44 @@ const NOT_A_REF = [ const OPT_OUT = /^\s*issue-refs:\s*N\/?A\b\s*-\s*\S[^\n]*/im; +// IN-FILE opt-outs, three scopes, narrowest first. They exist for the ref that genuinely must stay +// bare - above all quoted source material, where qualifying the number edits the quotation - and +// unlike the PR-body OPT_OUT above they travel with the file: they hold for every future PR +// touching the text and for local runs on branches with no PR body to opt out in. The author +// writes the marker in whatever comment syntax the file already has (`// issue-refs: exempt` in +// code, `` in markdown/HTML, where it is invisible when rendered). +// +// issue-refs: exempt this line only +// issue-refs: exempt-begin from here (added lines in the same patch) ... +// issue-refs: exempt-end ... to here +// issue-refs: exempt-file the whole file, judged against file CONTENT, not the patch +// +// Scopes are patch-shaped where they must be: suspectRefs judges added patch lines, so line and +// block markers take effect only when they are IN the patch - which they are, in the paste-a- +// quoted-block case they exist for. A lone later edit inside an old block re-carries its own +// marker. The file scope has no such caveat: hasFileOptOut is applied by both callers to the +// file's full text (local: disk; CI: contents API, fetched only for files with hits), so it holds +// however small the patch. A backslash-escape (`\#857`) was considered and rejected: it edits the +// text it is meant to preserve - so a quotation is no longer verbatim - renders visibly outside +// markdown, and is an illegal escape in a Java string literal. +// +// NB LINE_OPT_OUT's `exempt\b` also matches the longer markers (the `-` is a word boundary), so a +// begin/end/file marker line never flags its own bare refs either - harmless, and one less way to +// trip over the tool that is meant to be the escape hatch. +const LINE_OPT_OUT = /issue-refs:\s*exempt\b/i; +const BLOCK_BEGIN = /issue-refs:\s*exempt-begin\b/i; +const BLOCK_END = /issue-refs:\s*exempt-end\b/i; +const FILE_OPT_OUT = /issue-refs:\s*exempt-file\b/i; + function findOptOut(prBody) { return OPT_OUT.test(prBody || ""); } +// The file-scope test, applied to full file text by whichever caller can supply it. +function hasFileOptOut(text) { + return FILE_OPT_OUT.test(text || ""); +} + function isExempt(path) { return EXEMPT_PATHS.some((re) => re.test(path)); } @@ -100,10 +134,19 @@ function suspectRefs(files, opts = {}) { const out = []; for (const f of files || []) { if (!f.patch || isExempt(f.filename)) continue; + // Block opt-out state, per file. An unclosed exempt-begin swallows the rest of this file's + // added lines - the same direction prBodyEntry fails in for an unclosed fence, and for the + // same reason: silently resuming after a marker the author forgot to close would flag text + // they believed exempt, and a plausible-looking wrong answer is this gate's stated enemy. + let exemptBlock = false; for (const raw of f.patch.split("\n")) { if (!raw.startsWith("+") || raw.startsWith("+++")) continue; const line = raw.slice(1); + if (BLOCK_BEGIN.test(line)) { exemptBlock = true; continue; } + if (BLOCK_END.test(line)) { exemptBlock = false; continue; } + if (exemptBlock) continue; if (NOT_A_REF.some((re) => re.test(line))) continue; + if (LINE_OPT_OUT.test(line)) continue; for (const m of stripQualified(line).matchAll(/(?" on its own\n' + + "A ref that genuinely needs no qualifier (e.g. quoted source material) takes an IN-FILE opt-out,\n" + + "written in the file's own comment syntax (`// ...` in code, `` in markdown/HTML,\n" + + 'invisible when rendered): "issue-refs: exempt" on the line itself, "issue-refs: exempt-begin" /\n' + + '"issue-refs: exempt-end" around a pasted block, or "issue-refs: exempt-file" anywhere in a file\n' + + "that is legitimately full of them. These travel with the file, so they keep holding in future\n" + + "PRs and local runs.\n" + + 'To opt the WHOLE PR out instead, put "issue-refs: N/A - " on its own\n' + `${optOutTail}\n` + bodyNote + "\n" + - hits.map((h) => ` ${h.file}: ${h.ref} ${h.text}`).join("\n") + hits.map((h) => ` ${h.file}: ${h.ref} ${h.text}`).join("\n") + + "\n\n" + + "Fix: qualify each ref (`astubbs#NN` / `confluentinc#NN`). Refs that must stay bare (quoted\n" + + 'source material): in a comment, "issue-refs: exempt" on the line, "issue-refs: exempt-begin"/\n' + + '"-end" around a block, "issue-refs: exempt-file" for a whole file. Whole-PR opt-out:\n' + + '"issue-refs: N/A - " in the PR body. Full guidance above the list.' ); } module.exports = { - suspectRefs, findOptOut, isExempt, stripQualified, formatFailure, prBodyEntry, - EXEMPT_PATHS, QUALIFY_BELOW, PR_BODY_LABEL, + suspectRefs, findOptOut, hasFileOptOut, isExempt, stripQualified, formatFailure, prBodyEntry, + EXEMPT_PATHS, QUALIFY_BELOW, PR_BODY_LABEL, LINE_OPT_OUT, FILE_OPT_OUT, }; diff --git a/.github/scripts/issue-ref-gate.test.js b/.github/scripts/issue-ref-gate.test.js index 8f5821ddf..4b1c48f5b 100644 --- a/.github/scripts/issue-ref-gate.test.js +++ b/.github/scripts/issue-ref-gate.test.js @@ -2,7 +2,7 @@ // broken rule fails loudly rather than silently passing - or failing - every PR. const assert = require("assert"); const { - suspectRefs, findOptOut, isExempt, stripQualified, formatFailure, prBodyEntry, + suspectRefs, findOptOut, hasFileOptOut, isExempt, stripQualified, formatFailure, prBodyEntry, QUALIFY_BELOW, PR_BODY_LABEL, } = require("./issue-ref-gate.js"); @@ -345,15 +345,107 @@ check("formatFailure takes the repo from its caller, and defaults to the fork", assert.ok(formatFailure(HITS).includes("gh issue list -R astubbs/parallel-consumer")); }); -check("the opt-out tail matches whether the caller can read the PR body", () => { - assert.ok(!formatFailure(HITS).includes("does not read the PR body")); +check("the opt-out tail matches whether the caller could read the PR body", () => { + assert.ok(!formatFailure(HITS).includes("could not read the PR body")); const local = formatFailure(HITS, { readsPrBody: false }); - assert.ok(local.includes("does not read the PR body")); + assert.ok(local.includes("could not read the PR body")); // CI now scans the body as well as honouring the opt-out from it, so a caller with no body is // missing both. A tail naming only the opt-out would understate what the local run cannot see. assert.ok(local.includes("scans the body itself"), "must say CI reads the body for references too"); }); +check("the fix/opt-out reminder comes AFTER the last hit, where truncated reads still see it", () => { + // Readers of a failing tool consume its tail - often literally, via `| tail`. Guidance printed + // only above the hits list is invisible to them; this trailer is the copy that survives. + const msg = formatFailure(HITS); + const lastHit = msg.lastIndexOf("docs/x.md: #857"); + const trailer = msg.lastIndexOf("issue-refs: N/A - "); + assert.ok(lastHit !== -1 && trailer !== -1, "both the hit and the trailer must be present"); + assert.ok(trailer > lastHit, "the opt-out reminder must follow the hits list"); + assert.ok(msg.slice(lastHit).includes("Fix: qualify each ref"), "the trailer must lead with the fix"); + assert.ok(msg.slice(lastHit).includes("issue-refs: exempt"), "the trailer must name the line opt-out too"); +}); + +// The LINE opt-out: `issue-refs: exempt` on the flagged line itself. It exists for the ref that +// must stay bare - quoted source material above all, where qualifying the number edits the quote - +// and unlike the PR-body opt-out it travels with the file, so it holds for future PRs and for +// local runs on branches that have no PR body to opt out in. +check("a line carrying issue-refs: exempt is not flagged", () => { + const hits = suspectRefs(file("docs/x.md", 'He wrote "see #857 for that" ')); + assert.deepStrictEqual(hits, []); +}); + +check("the line opt-out exempts ONLY its own line", () => { + const hits = suspectRefs(file("docs/x.md", + "See #857 for the stall family.", + "And #858 too. ")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#857"]); +}); + +check("the line opt-out is case-insensitive and comment-syntax-agnostic", () => { + const hits = suspectRefs(file("src/X.java", "int x = 857; // was #857 upstream - Issue-Refs: EXEMPT")); + assert.deepStrictEqual(hits, []); +}); + +check("the line opt-out works in the PR body too - same rule, same adapter", () => { + const hits = suspectRefs(body("Quoting the old thread: fixes #858 ")); + assert.deepStrictEqual(hits, []); +}); + +check("issue-refs: exempt without a ref on the line exempts nothing else", () => { + const hits = suspectRefs(file("docs/x.md", + "issue-refs: exempt", + "See #857 for the stall family.")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#857"]); +}); + +// The BLOCK opt-out: exempt-begin/exempt-end around a pasted run of lines - the tool for quoted +// material too dense for per-line markers, where marking every line would be a mess in itself. +check("exempt-begin/exempt-end exempts the lines between, and only those", () => { + const hits = suspectRefs(file("docs/x.md", + "Before the quote: #100 is flagged.", + "", + "> the old thread said #857 and #858 fix it", + "> and #859 was a duplicate", + "", + "After the quote: #200 is flagged again.")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#100", "#200"]); +}); + +check("an unclosed exempt-begin swallows the rest of the file's added lines, like an unclosed fence", () => { + const hits = suspectRefs(file("docs/x.md", + "", + "quoted #857 forever")); + assert.deepStrictEqual(hits, []); +}); + +check("block state resets between files - one file's begin cannot exempt another file", () => { + const hits = suspectRefs([ + { filename: "docs/a.md", patch: "+\n+quoted #857" }, + { filename: "docs/b.md", patch: "+plain #858" }, + ]); + assert.deepStrictEqual(hits.map((h) => [h.file, h.ref]), [["docs/b.md", "#858"]]); +}); + +check("blocks work in the PR body too - same rule, same adapter", () => { + const hits = suspectRefs(body( + "Real text, so #100 is flagged.", + "", + "unfenced paste mentioning #857", + "")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#100"]); +}); + +// The FILE opt-out is content-based, applied by the callers via hasFileOptOut - so it holds +// however small the patch, unlike line/block markers which must be IN the patch to be seen. +check("hasFileOptOut finds the file marker in any comment syntax, and not its absence", () => { + assert.strictEqual(hasFileOptOut("\ntext"), true); + assert.strictEqual(hasFileOptOut("// Issue-Refs: EXEMPT-FILE"), true); + assert.strictEqual(hasFileOptOut("issue-refs: exempt on a line is not the file marker"), false); + assert.strictEqual(hasFileOptOut(""), false); + assert.strictEqual(hasFileOptOut(null), false); +}); + check("a body hit reads as the body in the failure listing", () => { const msg = formatFailure([{ file: PR_BODY_LABEL, ref: "#857", text: "Fixes #857" }]); assert.ok(msg.includes(` ${PR_BODY_LABEL}: #857 Fixes #857`), msg); diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml index 46216ea06..e9cab7924 100644 --- a/.github/workflows/pr-checklist.yml +++ b/.github/workflows/pr-checklist.yml @@ -45,6 +45,12 @@ jobs: - name: Self-test the issue-reference gate logic run: node .github/scripts/issue-ref-gate.test.js + # The local mirror (bin/check-issue-refs.sh) fetches the PR body via gh to honour the + # `issue-refs: N/A` opt-out the way this workflow does; that plumbing is shell, not gate + # logic, so it has its own self-test. + - name: Self-test the local issue-reference checker's body plumbing + run: bash bin/test-check-issue-refs.sh + # docs/todo-index.md is generated from the markers in the tree, so it can silently stop # matching them. Entries are keyed by marker TEXT, not line number, so this only fires when a # marker is actually added, removed or reworded - editing above one no longer churns the file. @@ -142,7 +148,27 @@ jobs: ...context.repo, pull_number: pr.number, per_page: 100, }); - const hits = gate.suspectRefs([...files, gate.prBodyEntry(pr.body)]); + let hits = gate.suspectRefs([...files, gate.prBodyEntry(pr.body)]); + + // File-scoped opt-out (`issue-refs: exempt-file`) is judged against file CONTENT, not + // the patch, so it holds however small the diff. Fetched only for files that actually + // have hits - usually none. A fetch failure keeps the hit: failing open here would let + // an unreadable file silently pass, which is the wrong direction for a gate. + for (const path of [...new Set(hits.map((h) => h.file))]) { + if (path === gate.PR_BODY_LABEL) continue; + try { + const res = await github.rest.repos.getContent({ + ...context.repo, path, ref: pr.head.sha, mediaType: { format: "raw" }, + }); + if (gate.hasFileOptOut(String(res.data))) { + hits = hits.filter((h) => h.file !== path); + core.info(`${path}: issue-refs: exempt-file present - its hits dropped.`); + } + } catch (e) { + core.info(`${path}: file opt-out check unavailable (${e.message}) - hits kept.`); + } + } + if (hits.length === 0) { core.info( `No unqualified references below #${gate.QUALIFY_BELOW} on added lines or in the PR body.`); diff --git a/bin/check-issue-refs.sh b/bin/check-issue-refs.sh index 3faa844a0..b96f0816b 100755 --- a/bin/check-issue-refs.sh +++ b/bin/check-issue-refs.sh @@ -28,11 +28,15 @@ # * CI reads patches from GitHub's `pulls.listFiles`, which omits `patch` on a very large diff, and # the gate skips a file it cannot see. This script builds its own patch with `git diff`, so it # still checks that file - it flags something CI would silently pass. -# * CI also scans the PR BODY (`gate.prBodyEntry`), which does not exist yet when you run this. So -# a bare `#NN` written into the description is CI's to catch, and this cannot pre-empt it. +# * CI also scans the PR BODY (`gate.prBodyEntry`) and honours the `issue-refs: N/A` opt-out +# written there. When `gh` can resolve the current branch to a PR, this script does the same - +# without that, the opt-out was one-sided: CI accepted it while every local run stayed red on +# the same file forever, teaching people the local check lies. When there is no PR yet (or no +# gh, or no network) there is no body to read, so a bare `#NN` destined for the description is +# still CI's to catch, and an opt-out you are only planning to write cannot be honoured. # # So a red result here is always real, but a green one promises neither that CI examined every file -# nor that the description you have not written yet will pass. +# nor that a description you have not written yet will pass. # # Usage: bin/check-issue-refs.sh [base-ref] (default base: origin/master, else master) # Exit codes: 0 = clean, 1 = unqualified refs found, @@ -69,6 +73,31 @@ const gate = require("./.github/scripts/issue-ref-gate.js"); const base = process.argv[2]; const git = (args) => execFileSync("git", args, { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 }); +// owner/name from origin, so the gh call and the mirror-lookup hint name the repo being checked +// rather than assuming the fork (AGENTS.md: qualify gh in anything written down). +let repo = null; +try { + const m = git(["remote", "get-url", "origin"]).trim().match(/[:/]([^/:]+\/[^/:]+?)(?:\.git)?$/); + if (m) repo = m[1]; +} catch { /* no origin - the gate's default stands */ } + +// Mirror CI's body handling whenever a body is reachable: honour the opt-out, scan the body. +// `gh pr view` resolves the PR from the current branch; ANY failure (no PR yet, gh missing, +// offline) degrades to the body-less behaviour, and the failure message says which run this was. +let prBody = null, prNumber = null; +try { + const pr = JSON.parse(execFileSync("gh", + ["pr", "view", ...(repo ? ["-R", repo] : []), "--json", "number,body"], + { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] })); + prBody = pr.body || ""; + prNumber = pr.number; +} catch { /* body unreadable this run */ } + +if (prBody !== null && gate.findOptOut(prBody)) { + console.log(`issue-refs: N/A opt-out found in PR ${prNumber}'s body - skipping, exactly as CI will.`); + process.exit(0); +} + // Working tree vs merge-base, so uncommitted edits are judged too. const names = git(["diff", "--name-only", base, "--"]).split("\n").filter(Boolean); @@ -76,17 +105,33 @@ const files = names.map((filename) => ({ filename, patch: git(["diff", "--unified=0", base, "--", filename]), })); +if (prBody !== null) files.push(gate.prBodyEntry(prBody)); + +let hits = gate.suspectRefs(files); -const hits = gate.suspectRefs(files); +// File-scoped opt-out (`issue-refs: exempt-file`) is judged against file CONTENT, not the patch, +// so it holds however small the diff. Only files that actually have hits are read. +const fs = require("fs"); +for (const path of [...new Set(hits.map((h) => h.file))]) { + if (path === gate.PR_BODY_LABEL) continue; + try { + if (gate.hasFileOptOut(fs.readFileSync(path, "utf8"))) { + hits = hits.filter((h) => h.file !== path); + } + } catch { /* deleted or unreadable - nothing to exempt */ } +} if (hits.length === 0) { + const bodyNote = prBody !== null ? ` and PR ${prNumber}'s body` : ""; console.log( `No unqualified references below #${gate.QUALIFY_BELOW} on added lines ` + - `(${files.length} changed file(s) vs ${base.slice(0, 12)}).` + `(${names.length} changed file(s) vs ${base.slice(0, 12)}${bodyNote}).` ); process.exit(0); } -console.error(gate.formatFailure(hits, { readsPrBody: false })); +const opts = repo ? { repo } : {}; +if (prBody === null) opts.readsPrBody = false; +console.error(gate.formatFailure(hits, opts)); process.exit(1); NODE diff --git a/bin/test-check-issue-refs.sh b/bin/test-check-issue-refs.sh new file mode 100755 index 000000000..a5f0e603b --- /dev/null +++ b/bin/test-check-issue-refs.sh @@ -0,0 +1,128 @@ +#!/usr/bin/env bash +# +# Copyright (C) 2026 Antony Stubbs and contributors +# + +# Self-test for bin/check-issue-refs.sh - specifically its PR-body handling, which the gate module's +# own unit tests (issue-ref-gate.test.js) cannot reach: they test the rule, this tests the plumbing +# that feeds the rule a body fetched via `gh`. +# +# WHY THE BODY PLUMBING NEEDS A TEST AT ALL. The `issue-refs: N/A` opt-out lives in the PR body, and +# for as long as the local script never read bodies, an opt-out CI accepted left every local run red +# on the same file forever - a "local mirror of CI" that permanently disagreed with CI. The script +# now fetches the body through `gh pr view` and honours the opt-out exactly as the workflow does; +# these cases pin that, plus the graceful fall-back when there is no PR to read. +# +# `gh` is stubbed on PATH: the stub prints $GH_STUB_JSON when set and fails like a PR-less branch +# when unset. No network, no real gh, deterministic either way. + +set -uo pipefail + +REPO_ROOT="$(cd "${BASH_SOURCE[0]%/*}/.." && pwd)" + +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +# --- fixture repo, with the script under test in its real layout --------------------------------- +mkdir -p "$TMP/repo/bin" "$TMP/repo/.github/scripts" "$TMP/stub" +cp "$REPO_ROOT/bin/check-issue-refs.sh" "$TMP/repo/bin/" +cp "$REPO_ROOT/.github/scripts/issue-ref-gate.js" "$TMP/repo/.github/scripts/" + +cat > "$TMP/stub/gh" <<'STUB' +#!/usr/bin/env bash +# Test stub: behave like `gh pr view` on a branch with no PR unless the case supplies a PR JSON. +[ -n "${GH_STUB_JSON:-}" ] || exit 1 +printf '%s' "$GH_STUB_JSON" +STUB +chmod +x "$TMP/stub/gh" +export PATH="$TMP/stub:$PATH" + +cd "$TMP/repo" +git init -q -b master +git config user.email test@example.invalid +git config user.name test +git add -A +git commit -qm base +git switch -qc feature +mkdir -p docs + +fails=0 +check() { # [must-not-contain] + local name=$1 want=$2 must=$3 mustnot=${4:-} out got + out="$(bash bin/check-issue-refs.sh master 2>&1)" + got=$? + if [ "$got" != "$want" ]; then + echo "FAIL: $name (expected exit $want, got $got)" + echo "$out" | sed 's/^/ /' + fails=$((fails + 1)) + return + fi + case "$out" in + *"$must"*) ;; + *) echo "FAIL: $name (output lacks: $must)"; echo "$out" | sed 's/^/ /'; fails=$((fails + 1)); return ;; + esac + if [ -n "$mustnot" ]; then + case "$out" in + *"$mustnot"*) echo "FAIL: $name (output must not contain: $mustnot)"; fails=$((fails + 1)); return ;; + esac + fi + echo "ok: $name" +} + +# --- no PR to read (gh fails) --------------------------------------------------------------------- +unset GH_STUB_JSON || true +echo "See #857 for details" > docs/note.md # issue-refs: exempt - fixture +git add docs/note.md +check "no PR: a bare ref still fails, and the message says the body was unreadable" \ + 1 "could not read the PR body" + +echo "See astubbs#857 for details" > docs/note.md +git add docs/note.md +check "no PR: a qualified ref passes, without claiming a body was checked" \ + 0 "changed file(s)" "body" + +echo 'He wrote "see #857" ' > docs/note.md +git add docs/note.md +check "no PR: a line carrying issue-refs: exempt passes end-to-end" \ + 0 "changed file(s)" + +printf '%s\n%s\n' '' 'A mess of quotes: #857 #858 #859' > docs/note.md +git add docs/note.md +check "no PR: issue-refs: exempt-file exempts the whole file, judged from disk content" \ + 0 "changed file(s)" + +echo "See astubbs#857 for details" > docs/note.md +git add docs/note.md + +# --- PR exists, body carries the opt-out ---------------------------------------------------------- +export GH_STUB_JSON='{"number":42,"body":"Explanation.\nissue-refs: N/A - refs occur inside quoted source material\n"}' +echo "See #857 for details" > docs/note.md # issue-refs: exempt - fixture +git add docs/note.md +check "opt-out in the PR body is honoured locally, exactly as CI honours it" \ + 0 "opt-out" + +# --- PR exists, body itself carries a bare ref ---------------------------------------------------- +export GH_STUB_JSON='{"number":42,"body":"This fixes #858 for good."}' # issue-refs: exempt - fixture +echo "See astubbs#857 for details" > docs/note.md +git add docs/note.md +check "a bare ref in the PR BODY is flagged locally, pre-empting the CI round-trip" \ + 1 "" + +# --- PR exists, everything clean ------------------------------------------------------------------ +export GH_STUB_JSON='{"number":42,"body":"All refs qualified, honest."}' +check "clean diff and clean body pass, and the success line says the body was checked" \ + 0 "PR 42's body" + +# --- the guidance survives truncation to the tail ------------------------------------------------- +export GH_STUB_JSON='{"number":42,"body":"This fixes #858 for good."}' # issue-refs: exempt - fixture +out="$(bash bin/check-issue-refs.sh master 2>&1 | tail -3)" +case "$out" in + *"issue-refs: N/A"*) echo "ok: the last three lines of a failure still name the opt-out" ;; + *) echo "FAIL: the opt-out reminder did not survive | tail -3"; echo "$out" | sed 's/^/ /'; fails=$((fails + 1)) ;; +esac + +if [ "$fails" -gt 0 ]; then + echo "$fails case(s) failed" + exit 1 +fi +echo "all cases passed" diff --git a/docs/issue-references.md b/docs/issue-references.md index 9f01c917b..f8a430328 100644 --- a/docs/issue-references.md +++ b/docs/issue-references.md @@ -111,13 +111,20 @@ drift from CI. It judges the working tree, like `bin/check-copyright-headers.sh` edits are caught too. Only lines you *add* are scanned; pre-existing bare refs in a file you touch are fine. -The check is purely textual - no API calls, so it cannot race issue creation. +The matching is purely textual and cannot race issue creation; the script's one network read is +`gh pr view`, fetching the current branch's PR body so the body is judged - and the +`issue-refs: N/A` opt-out honoured - by the same rules CI applies. Before the script read bodies, +the opt-out was one-sided: CI accepted it while every local run stayed red on the opted-out file +forever. Any failure of that read (no PR yet, no `gh`, offline) degrades to a body-less check, and +the failure message says so. The *inputs* differ from CI in two narrow ways. CI reads patches from GitHub's `pulls.listFiles`, which omits `patch` for a very large diff, and the gate skips a file it cannot see - while the local -script builds its own patch with `git diff` and still checks it. And CI additionally scans the **PR -body**, which does not exist when you run the script. So a green local run promises neither that CI -looked at every file nor that the description passes; a red one is always real. +script builds its own patch with `git diff` and still checks it. And when no PR exists yet there is +no **PR body** to read, so a bare ref destined for the description - or an opt-out you are only +planning to write - waits for CI, or for a re-run once the PR is open. So a green local run promises +neither that CI looked at every file nor that a description you have not written yet will pass; a +red one is always real. **When a PR serves an issue, the body's FIRST LINE links to it.** `Closes astubbs/parallel-consumer#155.` or `Part of astubbs/parallel-consumer#197.`, above everything else. @@ -141,7 +148,34 @@ Editing the body re-runs the job, so a fix there needs no push. The files listed in `EXEMPT_PATHS` are exempt, because a bare number legitimately means upstream in them: `CHANGELOG.adoc`, `upstream-map.yaml`, `upstream-pr-analysis.adoc`, and the gate's own test -fixtures. If a flagged reference really is fork-local, put `issue-refs: N/A - ` on its own -line in the PR body - which skips the body's own references along with everything else. - -Logic and tests live in `.github/scripts/issue-ref-gate.js` and `issue-ref-gate.test.js`. +fixtures. + +**Four opt-out scopes - reach for the narrowest that fits.** The in-file ones are written in +whatever comment syntax the file already has - `// ...` in code, `` in markdown/HTML, +where they are invisible when rendered - and because they live in the file they keep holding: +future PRs touching the text, and local runs on branches with no PR yet. They are the tool for the +ref that genuinely must stay bare - above all **quoted source material**, where qualifying the +number would edit the quotation. (A backslash escape was rejected for that job: `\#857` edits the +text it is meant to preserve, renders visibly outside markdown, and is an illegal escape in a Java +string literal.) + +- **One line: append `issue-refs: exempt` to the flagged line.** Same-line by necessity, not + taste: the gate judges added patch lines, and a marker on the previous line is not in the patch + when only the ref's line changed. +- **A block: wrap it in `issue-refs: exempt-begin` / `issue-refs: exempt-end`** - for a pasted + run of quoted lines too dense to mark individually. The markers govern added lines in the same + patch, so they cover the paste they arrive with; a later lone edit inside an old block carries + its own marker. An unclosed `exempt-begin` swallows the rest of the file's added lines, + deliberately - the same direction an unclosed fence fails in. +- **A file: put `issue-refs: exempt-file` anywhere in it** - for a file legitimately full of bare + refs (a generated document, an imported archive). This one is judged against the file's + **content**, not the patch - locally from disk, in CI via the contents API, fetched only for + files with hits - so it holds however small the diff. It is the in-file equivalent of an + `EXEMPT_PATHS` entry, without editing the gate. +- **Whole PR: put `issue-refs: N/A - ` on its own line in the PR body** - which skips + every file and the body's own references in one stroke, for this PR only. The big hammer; + reason mandatory. + +Logic and tests live in `.github/scripts/issue-ref-gate.js` and `issue-ref-gate.test.js`; the +local script's body-fetch plumbing has its own self-test, `bin/test-check-issue-refs.sh`, run by +the same workflow. From 68a1dc380bbfce1284b7e791a6d5f01ebf04f81f Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 18 Aug 2026 09:32:48 +1200 Subject: [PATCH 2/5] docs(solutions): red-proof with old code + new tests - the stash/amend/checkout silent-loss lesson Captures the process failure from this branch's own development as a durable learning: three git commands (stash pop, commit --amend against an emptied index, checkout -- ) each reported success while doing something other than intended, costing the same uncommitted work twice and voiding one red-proof entirely. The doc records the correct red-proof shape (old CODE + new TESTS; throwaway worktree as the safest form) and the prove-the-capture habit (status clean + grep the new symbol in git show HEAD:). Also adds "Red-proof" to CONCEPTS.md's Test reliability cluster - the practice bin/AGENTS.md mandates now has a name and a definition. Grounding: mechanical claims validator clean (4 paths, 3 links); semantic validator verified 10 claims including the git-stash man-page semantics, and its one finding (the quoted bin/AGENTS.md phrase line-wraps, so a literal grep fails) is fixed in the doc's wording. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn --- CONCEPTS.md | 8 + ...red-proof-old-code-new-tests-2026-08-18.md | 222 ++++++++++++++++++ 2 files changed, 230 insertions(+) create mode 100644 docs/solutions/workflow-issues/red-proof-old-code-new-tests-2026-08-18.md diff --git a/CONCEPTS.md b/CONCEPTS.md index 6135f2022..c4d65ca61 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -109,6 +109,14 @@ consumer-group progress evidence so the contention-versus-product-bug question i manual diagnosis starts. Its verdict is only informative when its detectors could have fired for the test in question — a short, low-volume test cannot trip them, and a clean reading there means nothing. +**Red-proof** +The verification that a new or extended test fails against the code as it was before the fix it +guards — a regression test that has never failed proves nothing. + +The proof requires a deliberately mismatched pair: old code, new tests. Any procedure that reverts +both together produces a matched pair and a vacuous pass, so a red-proof that does not go red is +first evidence against the method, not for the code. + ## Flagged ambiguities - **"Stall", "load-tightness flake" and "unforceable trigger" had been used loosely for the same red diff --git a/docs/solutions/workflow-issues/red-proof-old-code-new-tests-2026-08-18.md b/docs/solutions/workflow-issues/red-proof-old-code-new-tests-2026-08-18.md new file mode 100644 index 000000000..8f6c61d21 --- /dev/null +++ b/docs/solutions/workflow-issues/red-proof-old-code-new-tests-2026-08-18.md @@ -0,0 +1,222 @@ +--- +title: "Red-proof a checker with old code + new tests, and never trust a git command whose success output looks like the action" +date: 2026-08-18 +category: workflow-issues +module: tooling +problem_type: workflow_issue +component: development_workflow +severity: high +root_cause: wrong_api +resolution_type: workflow_improvement +applies_when: + - "Red-proofing a new regression test - verifying it fails against the old, pre-fix code before trusting it as a regression guard (the bin/AGENTS.md requirement)" + - "Using git stash, git checkout -- , or commit --amend around uncommitted work" + - "Verifying that a commit or amend actually captured the working-tree changes it was meant to" +symptoms: + - "git stash round-trip made the red-proof vacuously green - old tests ran against old code, 0 FAILs" + - "git commit --amend printed healthy-looking stats but captured only the message, because stash pop had unstaged everything" + - "git checkout HEAD -- silently overwrote the only copy of uncommitted work, twice in one session" +tags: + - "git" + - "git-stash" + - "git-checkout" + - "git-amend" + - "red-proof" + - "self-test" + - "regression-testing" + - "silent-data-loss" + - "worktree" +related_components: + - "git-workflow" + - "testing_framework" +--- + +# Red-proof a checker with old code + new tests, and never trust a git command whose success output looks like the action + +## Context + +`bin/AGENTS.md` requires that a new checker self-test be *verified red against the old code* - +"a regression test that has never failed proves nothing" (see +[`bin/AGENTS.md`](../../../bin/AGENTS.md), "Scripts that guard other scripts" - the phrase +line-wraps there, so search the section, not the literal string). While adding +opt-out features to the issue-ref gate (`.github/scripts/issue-ref-gate.js`, +`bin/check-issue-refs.sh`, plus the new `bin/test-check-issue-refs.sh`), a session attempted that +red-proof three ways. The first was methodologically void, and two of the attempts silently +destroyed uncommitted work - the same uncommitted work, twice - with every command printing output +indistinguishable from success. + +The three failures, in order: + +1. **Void method.** `git add -A; git stash -q` → run self-test → observe "0 FAILs" → + `git stash pop -q`. The stash reverted the *new tests together with the new code*, so old tests + ran against old code. Matched pairs always agree; the "0 FAILs" proved nothing while looking + like a completed red-proof step. +2. **Silent loss via stash pop + amend.** `git stash pop` (without `--index`) restored the files + but left everything **unstaged**. The next `git commit --amend -F ` therefore amended only + the message - and printed "6 files changed, 200 insertions(+)", the same healthy stats as the + original commit, so the author believed the round-two work was in HEAD. It was not. +3. **Silent loss via checkout-pathspec "restore".** The next red-proof used the correct shape - + `git checkout origin/master -- .github/scripts/issue-ref-gate.js bin/check-issue-refs.sh` + (old code only, new tests kept in place) and correctly observed failures - but then "restored" + with `git checkout HEAD -- `. Because of loss 2, HEAD did not contain the round-two + implementation, so the restore overwrote the only surviving copy of the uncommitted work. + `git checkout -- ` gives no warning when clobbering uncommitted changes. + +Recovery required re-applying the edits from the conversation record (the only surviving copy), +then proving capture: `git add -A`, amend, `git status --short` printing **nothing**, and +`git show HEAD:.github/scripts/issue-ref-gate.js | grep -c "LINE_OPT_OUT\|BLOCK_BEGIN\|FILE_OPT_OUT"` +returning a symbol count only the new work could produce. The final honest red-proof: with the +new tests in the tree and only the two code files checked out from `origin/master`, 7 of 8 harness +cases FAILED and the unit-test run aborted; code was then restored by re-applying, never by +checkout-to-HEAD until HEAD provably contained the work. + +## Guidance + +**1. A red-proof is OLD CODE + NEW TESTS. Never stash.** + +The point of the exercise is a *mismatched* pair: yesterday's implementation, today's assertions. +Any mechanism that reverts code and tests together - `git stash`, `git checkout ` of the +whole tree, switching branches - produces a matched pair and a vacuous pass. Two safe shapes: + +- Good: `git checkout -- ` while the new test files stay put. You must + then restore correctly (see rule 4). +- Best: a throwaway worktree at the old ref with the new test files copied in - zero mutation of + the live tree, and worktrees are already this repo's house pattern (`AGENTS.md`, "Worktree + ownership"): + + ```bash + git worktree add /tmp/redproof origin/master + cp bin/test-check-issue-refs.sh /tmp/redproof/bin/ + (cd /tmp/redproof && bash bin/test-check-issue-refs.sh) # MUST fail + git worktree remove --force /tmp/redproof + ``` + +A red-proof that does not fail is a red flag about the *method*, not a green light: first suspect +that you reverted both sides of the pair. + +**2. After any commit or amend meant to capture work, PROVE it captured the content.** + +An amend that captured nothing prints stats that look exactly like success (`--amend` re-reports +the whole commit's stats, not what the amend added). Two checks, both cheap: + +```bash +git status --short # must print NOTHING +git show HEAD: | grep -c "" # must be > 0 +``` + +Pick a symbol unique to the new work (a new constant, a new flag name). `git show --stat HEAD` +is corroboration, not proof - it was exactly the output that lied in this incident. + +**3. Treat `git stash pop` as an index-destroying operation.** + +Whatever was staged before the stash is not staged after `pop` (only `pop --index` tries to +restore it, and even that can fail on conflicts). Never sandwich a stash between `git add` and a +commit/amend; re-run `git add` and re-verify `git status --short` after every pop. + +**4. Treat `git checkout -- ` as `rm` for uncommitted work.** + +It overwrites the working copy with the ref's version, silently, with no reflog entry for what it +destroyed. Before using it to "restore", prove the ref actually contains the version you want +back: + +```bash +git show HEAD: | grep -c "" # must be > 0 FIRST +git checkout HEAD -- # only then +``` + +If the grep returns 0, HEAD does not have your work and the checkout would delete the only copy. +The worktree form of the red-proof (rule 1) avoids this entire trap: the live tree is never +mutated, so there is nothing to restore. + +## Why This Matters + +The defect class is **git commands whose success output is indistinguishable from the intended +action**. Each command here exited 0 and printed plausible output while doing something other than +what the operator believed: the stash round-trip "verified" nothing, the amend "captured" nothing, +the checkout "restored" a stale version over live work. No step errored; the loss was discovered +only because a later red-proof behaved impossibly. + +This repo has already paid for siblings of this class, and documents them: + +- A whole catalogue of checks that report success without having run, and the guard-design rules + that fix them: + [`a-check-that-reports-success-without-having-run.md`](a-check-that-reports-success-without-having-run.md) + (this doc is the git-plumbing instance of that genus - its "prove it by making it fail" rule is + the same move as grepping the new symbol in `git show`). +- Piping a git command through `tail`/`head` swallows the exit code, so a failed + `git checkout ` in the wrong worktree let an `&&`-chained rebase run against the wrong + branch - `AGENTS.md`, "Worktree ownership" ("never pipe a git command whose failure must stop an + `&&` chain"). Same command family, different mechanism: there the checkout *failed* silently; + here it *succeeded* at the wrong thing. +- Negative results need an instrument that could have said yes: + [`negative-results-need-an-instrument-that-could-have-said-yes.md`](negative-results-need-an-instrument-that-could-have-said-yes.md) + - the "0 FAILs" from the void stash method is exactly a negative result from an instrument that + could not have said yes. + +The common counter-move is the same in every case: do not read the tool's success output as +evidence the intended state was reached - **probe the state directly** (grep the symbol in +`git show`, check `git status --short` is empty, count the FAILs that must exist). + +The stakes are concrete: here, two rounds of implementation work survived only in the +conversation record. Without that record, the work would have been gone with no reflog entry, no +stash entry, and no error message to say when it died. + +## When to Apply + +- Every time `bin/AGENTS.md`'s red-proof requirement fires: a new `bin/test-check-*.sh`, or a new + case added to one after fixing a checker bug. The proof must pit old code against new tests. +- Any workflow that temporarily reverts files to an older ref and then restores them - red-proofs, + bisects by hand, "let me just check the old behaviour". +- Any `git commit --amend` intended to *add content* (not just reword): verify capture before the + next destructive command, especially before any `git checkout -- `. +- Any use of `git stash` while an index is staged for a specific commit. +- Not needed when the tree is clean and committed: `git checkout -- ` on a clean tree + is recoverable via `git checkout HEAD -- `, because HEAD then provably has everything. + +## Examples + +**Red-proof - wrong vs right:** + +```bash +# WRONG - vacuous: stash reverts new tests AND new code together; matched pairs always agree +git add -A && git stash -q +bash bin/test-check-issue-refs.sh # "0 FAILs" - proves nothing +git stash pop -q # ...and now nothing is staged (see next example) + +# RIGHT (in-tree) - old code, new tests; failures are the proof +git checkout origin/master -- .github/scripts/issue-ref-gate.js bin/check-issue-refs.sh +bash bin/test-check-issue-refs.sh # MUST fail (here: 7 of 8 cases FAILED) +# restore by re-applying or from a ref PROVEN to contain the work - never blind checkout-to-HEAD + +# RIGHT (safest) - throwaway worktree, live tree untouched +git worktree add /tmp/redproof origin/master +cp bin/test-check-issue-refs.sh /tmp/redproof/bin/ +(cd /tmp/redproof && bash bin/test-check-issue-refs.sh) # MUST fail +git worktree remove --force /tmp/redproof +``` + +**Amend capture - trusting stats vs proving content:** + +```bash +# WRONG - stash pop unstaged everything; this amends ONLY the message, +# yet prints "6 files changed, 200 insertions(+)" - identical to real success +git stash pop -q +git commit --amend -F /tmp/msg + +# RIGHT - stage, amend, then prove capture with state, not stats +git add -A +git commit --amend -F /tmp/msg +git status --short # must print NOTHING +git show HEAD:.github/scripts/issue-ref-gate.js \ + | grep -c "LINE_OPT_OUT\|BLOCK_BEGIN\|FILE_OPT_OUT" # must match the new symbols +``` + +**Restoring after a red-proof - blind vs proven:** + +```bash +# WRONG - if HEAD lacks the round-two work, this deletes the only copy, silently +git checkout HEAD -- .github/scripts/issue-ref-gate.js bin/check-issue-refs.sh + +# RIGHT - prove the ref holds the version you want back BEFORE overwriting the tree +git show HEAD:.github/scripts/issue-ref-gate.js | grep -c "LINE_OPT_OUT" # 0 → STOP +``` From 3d6aa74a3e444084502fee3a795a17f2986b352b Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 18 Aug 2026 13:02:03 +1200 Subject: [PATCH 3/5] refactor(issue-refs): deduplicate the file-opt-out control flow into the gate module, and fix PR resolution under -R ce-simplify pass over the branch, three reviewers. Applied: - The file-opt-out drop loop existed twice - bin/check-issue-refs.sh's node block and pr-checklist.yml's workflow step - identical control flow (dedupe, PR-body skip, fail-open-keeps-hit) differing only in the reader. That is the exact NO-SECOND-COPY violation the module's own doctrine names. Now one exported gate.dropFileOptedOutHits(hits, readFile, {onDrop, onError}); each caller supplies its reader (fs vs contents API) and its logging. Unit-tested (dedupe, body skip, fail-open, callbacks) - 61 assertions total. - Dropped the dead exports LINE_OPT_OUT / FILE_OPT_OUT - nothing imported them; hasFileOptOut is the intended surface, matching the unexported BLOCK_BEGIN/BLOCK_END. - gh pr view gained timeout: 3000 - this script runs in the pre-commit hook, whose documented budget is ~1.5s, and execFileSync otherwise blocks forever on a hung network. Measured: ~0.7s total with a live PR, ~0.2s without. - FIXED A REAL BUG the timing run exposed: `gh pr view -R ` refuses to infer the branch ("argument required when using the --repo flag"), so the body fetch silently failed on every run - the stubbed-gh harness could not catch a real gh CLI contract. The branch is now passed explicitly (detached HEAD skips), verified live: the success line names the PR body. - Deleted two dead fixture lines in the harness (written state overwritten before any check observed it). - docs/issue-references.md's backslash-rejection rationale trimmed to a pointer at the LINE_OPT_OUT design comment, which owns it (never state a fact twice). Skipped, deliberately: formatFailure's header/trailer repetition (the truncation-resilience feature itself, position-pinned by test); the harness's hand-rolled check() helper (bin/AGENTS.md documents copying, not sharing, as the convention); two pre-existing rationale duplications in docs/issue-references.md dating from before this branch (out of scope). Verified: 61 unit assertions, 8/8 harness cases, sigpipe guard, actionlint, the gate on this branch's own diff (now confirming "and PR 307's body"). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn --- .github/scripts/issue-ref-gate.js | 33 ++++++++++++++++++++-- .github/scripts/issue-ref-gate.test.js | 38 ++++++++++++++++++++++++++ .github/workflows/pr-checklist.yml | 27 ++++++++---------- bin/check-issue-refs.sh | 31 ++++++++++++--------- bin/test-check-issue-refs.sh | 3 -- docs/issue-references.md | 5 ++-- 6 files changed, 100 insertions(+), 37 deletions(-) diff --git a/.github/scripts/issue-ref-gate.js b/.github/scripts/issue-ref-gate.js index d28be2694..f1f632394 100644 --- a/.github/scripts/issue-ref-gate.js +++ b/.github/scripts/issue-ref-gate.js @@ -88,6 +88,34 @@ function hasFileOptOut(text) { return FILE_OPT_OUT.test(text || ""); } +/** + * Drops every hit belonging to a file whose CONTENT carries the file-scope opt-out. The control + * flow lives here so the two callers cannot drift - NO SECOND COPY OF THE RULE - and each caller + * supplies only what genuinely differs: how a file is read (local disk vs the contents API) and + * whether outcomes are logged. Read failures keep the hit (fail open here would let an unreadable + * file silently pass, the wrong direction for a gate) and are reported via onError when given. + * + * @param hits [{ file, ref, text }] from suspectRefs + * @param readFile async (path) => file text; may throw / reject + * @param opts { onDrop(path), onError(path, err) } - optional, for caller-side logging + * @returns the hits that survive + */ +async function dropFileOptedOutHits(hits, readFile, opts = {}) { + let out = hits; + for (const path of [...new Set(hits.map((h) => h.file))]) { + if (path === PR_BODY_LABEL) continue; + try { + if (hasFileOptOut(await readFile(path))) { + out = out.filter((h) => h.file !== path); + opts.onDrop?.(path); + } + } catch (e) { + opts.onError?.(path, e); + } + } + return out; +} + function isExempt(path) { return EXEMPT_PATHS.some((re) => re.test(path)); } @@ -295,6 +323,7 @@ function formatFailure(hits, opts = {}) { } module.exports = { - suspectRefs, findOptOut, hasFileOptOut, isExempt, stripQualified, formatFailure, prBodyEntry, - EXEMPT_PATHS, QUALIFY_BELOW, PR_BODY_LABEL, LINE_OPT_OUT, FILE_OPT_OUT, + suspectRefs, findOptOut, hasFileOptOut, dropFileOptedOutHits, isExempt, stripQualified, + formatFailure, prBodyEntry, + EXEMPT_PATHS, QUALIFY_BELOW, PR_BODY_LABEL, }; diff --git a/.github/scripts/issue-ref-gate.test.js b/.github/scripts/issue-ref-gate.test.js index 4b1c48f5b..4b564dc12 100644 --- a/.github/scripts/issue-ref-gate.test.js +++ b/.github/scripts/issue-ref-gate.test.js @@ -446,6 +446,44 @@ check("hasFileOptOut finds the file marker in any comment syntax, and not its ab assert.strictEqual(hasFileOptOut(null), false); }); +// dropFileOptedOutHits is the shared control flow both callers run (local script and CI step), +// each supplying only its reader - so the drift-prone part is pinned here, once. +(async () => { + const HITS3 = [ + { file: "docs/a.md", ref: "#857", text: "a" }, + { file: "docs/a.md", ref: "#858", text: "a2" }, + { file: "docs/b.md", ref: "#859", text: "b" }, + { file: PR_BODY_LABEL, ref: "#860", text: "body" }, + ]; + + await (async () => { + const reads = []; + const dropped = []; + const out = await require("./issue-ref-gate.js").dropFileOptedOutHits(HITS3, async (p) => { + reads.push(p); + return p === "docs/a.md" ? "" : "plain"; + }, { onDrop: (p) => dropped.push(p) }); + assert.deepStrictEqual(out.map((h) => h.ref), ["#859", "#860"], + "marked file's hits drop; other file and PR body survive"); + assert.deepStrictEqual(reads, ["docs/a.md", "docs/b.md"], + "each file read once (deduped), PR body never read"); + assert.deepStrictEqual(dropped, ["docs/a.md"], "onDrop fires per dropped file"); + console.log(" ok dropFileOptedOutHits drops the marked file's hits, deduped, skipping the body"); + run++; + })(); + + await (async () => { + const errors = []; + const out = await require("./issue-ref-gate.js").dropFileOptedOutHits(HITS3, + async () => { throw new Error("unreadable"); }, + { onError: (p) => errors.push(p) }); + assert.deepStrictEqual(out, HITS3, "a read failure keeps the hits - the gate must not fail open"); + assert.deepStrictEqual(errors, ["docs/a.md", "docs/b.md"], "onError fires per unreadable file"); + console.log(" ok dropFileOptedOutHits keeps hits when the reader throws"); + run++; + })(); +})(); + check("a body hit reads as the body in the failure listing", () => { const msg = formatFailure([{ file: PR_BODY_LABEL, ref: "#857", text: "Fixes #857" }]); assert.ok(msg.includes(` ${PR_BODY_LABEL}: #857 Fixes #857`), msg); diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml index e9cab7924..7cfb745c2 100644 --- a/.github/workflows/pr-checklist.yml +++ b/.github/workflows/pr-checklist.yml @@ -152,22 +152,17 @@ jobs: // File-scoped opt-out (`issue-refs: exempt-file`) is judged against file CONTENT, not // the patch, so it holds however small the diff. Fetched only for files that actually - // have hits - usually none. A fetch failure keeps the hit: failing open here would let - // an unreadable file silently pass, which is the wrong direction for a gate. - for (const path of [...new Set(hits.map((h) => h.file))]) { - if (path === gate.PR_BODY_LABEL) continue; - try { - const res = await github.rest.repos.getContent({ - ...context.repo, path, ref: pr.head.sha, mediaType: { format: "raw" }, - }); - if (gate.hasFileOptOut(String(res.data))) { - hits = hits.filter((h) => h.file !== path); - core.info(`${path}: issue-refs: exempt-file present - its hits dropped.`); - } - } catch (e) { - core.info(`${path}: file opt-out check unavailable (${e.message}) - hits kept.`); - } - } + // have hits - usually none. The control flow lives in the gate module (NO SECOND COPY + // OF THE RULE); this caller supplies the contents-API reader and the logging. + hits = await gate.dropFileOptedOutHits(hits, async (path) => { + const res = await github.rest.repos.getContent({ + ...context.repo, path, ref: pr.head.sha, mediaType: { format: "raw" }, + }); + return String(res.data); + }, { + onDrop: (path) => core.info(`${path}: issue-refs: exempt-file present - its hits dropped.`), + onError: (path, e) => core.info(`${path}: file opt-out check unavailable (${e.message}) - hits kept.`), + }); if (hits.length === 0) { core.info( diff --git a/bin/check-issue-refs.sh b/bin/check-issue-refs.sh index b96f0816b..808b0237c 100755 --- a/bin/check-issue-refs.sh +++ b/bin/check-issue-refs.sh @@ -68,11 +68,14 @@ fi node - "$MERGE_BASE" <<'NODE' const { execFileSync } = require("child_process"); +const fs = require("fs"); const gate = require("./.github/scripts/issue-ref-gate.js"); const base = process.argv[2]; const git = (args) => execFileSync("git", args, { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 }); +(async () => { + // owner/name from origin, so the gh call and the mirror-lookup hint name the repo being checked // rather than assuming the fork (AGENTS.md: qualify gh in anything written down). let repo = null; @@ -83,12 +86,19 @@ try { // Mirror CI's body handling whenever a body is reachable: honour the opt-out, scan the body. // `gh pr view` resolves the PR from the current branch; ANY failure (no PR yet, gh missing, -// offline) degrades to the body-less behaviour, and the failure message says which run this was. +// offline, or the 3s timeout - this runs inside the pre-commit hook, whose whole budget is +// ~1.5s, so a hung network call must be bounded) degrades to the body-less behaviour, and the +// failure message says which run this was. let prBody = null, prNumber = null; try { + // The branch is passed explicitly because `gh pr view` REFUSES to infer it when -R is given + // ("argument required when using the --repo flag") - and the self-test's stubbed gh cannot + // catch that contract, so it is pinned here in prose instead. + const branch = git(["rev-parse", "--abbrev-ref", "HEAD"]).trim(); + if (branch === "HEAD") throw new Error("detached HEAD - no branch to resolve a PR from"); const pr = JSON.parse(execFileSync("gh", - ["pr", "view", ...(repo ? ["-R", repo] : []), "--json", "number,body"], - { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] })); + ["pr", "view", branch, ...(repo ? ["-R", repo] : []), "--json", "number,body"], + { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], timeout: 3000 })); prBody = pr.body || ""; prNumber = pr.number; } catch { /* body unreadable this run */ } @@ -110,16 +120,10 @@ if (prBody !== null) files.push(gate.prBodyEntry(prBody)); let hits = gate.suspectRefs(files); // File-scoped opt-out (`issue-refs: exempt-file`) is judged against file CONTENT, not the patch, -// so it holds however small the diff. Only files that actually have hits are read. -const fs = require("fs"); -for (const path of [...new Set(hits.map((h) => h.file))]) { - if (path === gate.PR_BODY_LABEL) continue; - try { - if (gate.hasFileOptOut(fs.readFileSync(path, "utf8"))) { - hits = hits.filter((h) => h.file !== path); - } - } catch { /* deleted or unreadable - nothing to exempt */ } -} +// so it holds however small the diff. The control flow lives in the gate module (NO SECOND COPY +// OF THE RULE); this caller supplies only the reader. Silent on drops, matching the rest of this +// script's output discipline. +hits = await gate.dropFileOptedOutHits(hits, (path) => fs.readFileSync(path, "utf8")); if (hits.length === 0) { const bodyNote = prBody !== null ? ` and PR ${prNumber}'s body` : ""; @@ -134,4 +138,5 @@ const opts = repo ? { repo } : {}; if (prBody === null) opts.readsPrBody = false; console.error(gate.formatFailure(hits, opts)); process.exit(1); +})(); NODE diff --git a/bin/test-check-issue-refs.sh b/bin/test-check-issue-refs.sh index a5f0e603b..8cdffddcf 100755 --- a/bin/test-check-issue-refs.sh +++ b/bin/test-check-issue-refs.sh @@ -91,9 +91,6 @@ git add docs/note.md check "no PR: issue-refs: exempt-file exempts the whole file, judged from disk content" \ 0 "changed file(s)" -echo "See astubbs#857 for details" > docs/note.md -git add docs/note.md - # --- PR exists, body carries the opt-out ---------------------------------------------------------- export GH_STUB_JSON='{"number":42,"body":"Explanation.\nissue-refs: N/A - refs occur inside quoted source material\n"}' echo "See #857 for details" > docs/note.md # issue-refs: exempt - fixture diff --git a/docs/issue-references.md b/docs/issue-references.md index f8a430328..10df35dfa 100644 --- a/docs/issue-references.md +++ b/docs/issue-references.md @@ -155,9 +155,8 @@ whatever comment syntax the file already has - `// ...` in code, `` where they are invisible when rendered - and because they live in the file they keep holding: future PRs touching the text, and local runs on branches with no PR yet. They are the tool for the ref that genuinely must stay bare - above all **quoted source material**, where qualifying the -number would edit the quotation. (A backslash escape was rejected for that job: `\#857` edits the -text it is meant to preserve, renders visibly outside markdown, and is an illegal escape in a Java -string literal.) +number would edit the quotation. (A backslash escape was considered and rejected for that job - +the design comment above `LINE_OPT_OUT` in `.github/scripts/issue-ref-gate.js` owns the why.) - **One line: append `issue-refs: exempt` to the flagged line.** Same-line by necessity, not taste: the gate judges added patch lines, and a marker on the previous line is not in the patch From aa310e59281fc140cca55f6bd31f57ffcbd9eb69 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 18 Aug 2026 13:30:50 +1200 Subject: [PATCH 4/5] fix(review): mention is not use - quoting an opt-out marker no longer activates it, and a body fence closes an open block Applies all six findings from this branch's code review (validated 6/6 by independent repro). The two P1s were unintended false-pass paths that broke the PR's own contract: - MENTION IS NOT USE. Marker tests now run on a span-stripped view (stripCodeSpans): a marker quoted in a backtick code span is documentation. Before this, five of this branch's own files - including docs/issue-references.md, the convention's defining doc - were permanently whole-file exempt, and a doc line quoting the block markers opened a real, unclosed block. An unquoted prose mention still counts (indistinguishable from use); the docs now say to backtick marker names when writing about them. A line carrying BOTH block markers is treated as discussion: no state change. - A FENCE CLOSES ANY OPEN BLOCK in the PR body. prBodyEntry blanks fenced lines, so an exempt-end inside a fence was invisible and the block never closed - everything after silently escaped (fail-open). The fence boundary now emits a synthetic end marker, flipping the failure mode to fail-closed. - bin/test-check-issue-refs.sh joins EXEMPT_PATHS with the same rationale as the gate's unit-test file: its fixtures write real markers and bare refs deliberately. - The harness truncation case now uses tail -4 and asserts the trailer's LEAD line ("Fix: qualify each ref") as well as the opt-out substring - tail -3 was provably cutting the lead while the test passed. - The unit-test file's async block is captured and awaited before the summary line, so the printed count (now 67) is accurate and prints last. - AGENTS.md's issue-references entry updated: both the local script and CI scan the PR body when one is reachable (the old line implied CI-only). Regression tests: quoted file/line/block markers are inert at every scope; unquoted both-markers lines change no state; the fence repro flags both the ref before the block and the ref after the fence; a fence with no open block is unchanged. 67 unit assertions, 8/8 harness cases, sigpipe + actionlint + the gate itself all green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn --- .github/scripts/issue-ref-gate.js | 59 +++++++++++++++++++------ .github/scripts/issue-ref-gate.test.js | 60 ++++++++++++++++++++++++-- AGENTS.md | 3 +- bin/test-check-issue-refs.sh | 19 +++++--- docs/issue-references.md | 11 ++++- 5 files changed, 126 insertions(+), 26 deletions(-) diff --git a/.github/scripts/issue-ref-gate.js b/.github/scripts/issue-ref-gate.js index f1f632394..f7d7d7482 100644 --- a/.github/scripts/issue-ref-gate.js +++ b/.github/scripts/issue-ref-gate.js @@ -39,6 +39,9 @@ const EXEMPT_PATHS = [ /(^|\/)src\/docs\/development\/upstream-pr-analysis\.adoc$/, // this gate's own fixtures are deliberately full of unqualified refs /(^|\/)\.github\/scripts\/issue-ref-gate\.test\.js$/, + // same rationale for the local checker's end-to-end harness: its fixtures write real opt-out + // markers and bare refs on purpose, and a fixture that must dodge the gate stops testing it + /(^|\/)bin\/test-check-issue-refs\.sh$/, ]; // Constructs that look like a ref but are not one. @@ -57,10 +60,17 @@ const OPT_OUT = /^\s*issue-refs:\s*N\/?A\b\s*-\s*\S[^\n]*/im; // writes the marker in whatever comment syntax the file already has (`// issue-refs: exempt` in // code, `` in markdown/HTML, where it is invisible when rendered). // -// issue-refs: exempt this line only -// issue-refs: exempt-begin from here (added lines in the same patch) ... -// issue-refs: exempt-end ... to here -// issue-refs: exempt-file the whole file, judged against file CONTENT, not the patch +// `issue-refs: exempt` this line only +// `issue-refs: exempt-begin` from here (added lines in the same patch) ... +// `issue-refs: exempt-end` ... to here +// `issue-refs: exempt-file` the whole file, judged against file CONTENT, not the patch +// +// MENTION IS NOT USE. Every marker test runs on a span-stripped view (stripCodeSpans below): a +// marker quoted in a backtick code span is documentation, not an instruction. Without that rule, +// this PR's own docs made five files - including the convention's defining doc - permanently +// self-exempt, and a doc line quoting the block markers opened a real, unclosed block. An +// UNQUOTED prose mention still matches (there is no way to tell it from use); write marker names +// in backticks when writing ABOUT them, as this comment does. // // Scopes are patch-shaped where they must be: suspectRefs judges added patch lines, so line and // block markers take effect only when they are IN the patch - which they are, in the paste-a- @@ -79,13 +89,20 @@ const BLOCK_BEGIN = /issue-refs:\s*exempt-begin\b/i; const BLOCK_END = /issue-refs:\s*exempt-end\b/i; const FILE_OPT_OUT = /issue-refs:\s*exempt-file\b/i; +// The mention-vs-use rule: markers are matched only on text OUTSIDE backtick code spans, the same +// judgment stripQualified applies to refs. Spans are single-line (a lone stray backtick must not +// pair across lines and swallow a real marker between them). +function stripCodeSpans(text) { + return (text || "").replace(/`[^`\n]*`/g, " "); +} + function findOptOut(prBody) { return OPT_OUT.test(prBody || ""); } // The file-scope test, applied to full file text by whichever caller can supply it. function hasFileOptOut(text) { - return FILE_OPT_OUT.test(text || ""); + return FILE_OPT_OUT.test(stripCodeSpans(text)); } /** @@ -170,11 +187,18 @@ function suspectRefs(files, opts = {}) { for (const raw of f.patch.split("\n")) { if (!raw.startsWith("+") || raw.startsWith("+++")) continue; const line = raw.slice(1); - if (BLOCK_BEGIN.test(line)) { exemptBlock = true; continue; } - if (BLOCK_END.test(line)) { exemptBlock = false; continue; } + // Markers are matched on the span-stripped view, so quoting one never activates it. + const markerView = stripCodeSpans(line); + const opensBlock = BLOCK_BEGIN.test(markerView); + const closesBlock = BLOCK_END.test(markerView); + if (opensBlock && closesBlock) { + // A line carrying BOTH is discussing the markers, not using them: no state change. Its own + // refs are still spared by LINE_OPT_OUT below, same as any single unquoted mention. + } else if (opensBlock) { exemptBlock = true; continue; } + else if (closesBlock) { exemptBlock = false; continue; } if (exemptBlock) continue; if (NOT_A_REF.some((re) => re.test(line))) continue; - if (LINE_OPT_OUT.test(line)) continue; + if (LINE_OPT_OUT.test(markerView)) continue; for (const m of stripQualified(line).matchAll(/(?` in markdown/HTML,\n" + - 'invisible when rendered): "issue-refs: exempt" on the line itself, "issue-refs: exempt-begin" /\n' + - '"issue-refs: exempt-end" around a pasted block, or "issue-refs: exempt-file" anywhere in a file\n' + + "invisible when rendered): `issue-refs: exempt` on the line itself, `issue-refs: exempt-begin` /\n" + + "`issue-refs: exempt-end` around a pasted block, or `issue-refs: exempt-file` anywhere in a file\n" + "that is legitimately full of them. These travel with the file, so they keep holding in future\n" + - "PRs and local runs.\n" + + "PRs and local runs. A marker inside a backtick code span (as written here) is a mention, not a\n" + + "use - quoting a marker never activates it.\n" + 'To opt the WHOLE PR out instead, put "issue-refs: N/A - " on its own\n' + `${optOutTail}\n` + bodyNote + @@ -316,8 +347,8 @@ function formatFailure(hits, opts = {}) { hits.map((h) => ` ${h.file}: ${h.ref} ${h.text}`).join("\n") + "\n\n" + "Fix: qualify each ref (`astubbs#NN` / `confluentinc#NN`). Refs that must stay bare (quoted\n" + - 'source material): in a comment, "issue-refs: exempt" on the line, "issue-refs: exempt-begin"/\n' + - '"-end" around a block, "issue-refs: exempt-file" for a whole file. Whole-PR opt-out:\n' + + "source material): in a comment, `issue-refs: exempt` on the line, `issue-refs: exempt-begin`/\n" + + "`-end` around a block, `issue-refs: exempt-file` for a whole file. Whole-PR opt-out:\n" + '"issue-refs: N/A - " in the PR body. Full guidance above the list.' ); } diff --git a/.github/scripts/issue-ref-gate.test.js b/.github/scripts/issue-ref-gate.test.js index 4b564dc12..6194e6715 100644 --- a/.github/scripts/issue-ref-gate.test.js +++ b/.github/scripts/issue-ref-gate.test.js @@ -117,7 +117,8 @@ check("exempts the files where a bare number means upstream", () => { for (const p of ["CHANGELOG.adoc", "src/docs/development/upstream-map.yaml", "src/docs/development/upstream-pr-analysis.adoc", - ".github/scripts/issue-ref-gate.test.js"]) { + ".github/scripts/issue-ref-gate.test.js", + "bin/test-check-issue-refs.sh"]) { assert.ok(isExempt(p), p + " should be exempt"); assert.deepStrictEqual(suspectRefs(file(p, "fix: something (#857)")), []); } @@ -446,9 +447,60 @@ check("hasFileOptOut finds the file marker in any comment syntax, and not its ab assert.strictEqual(hasFileOptOut(null), false); }); +// MENTION IS NOT USE. Before span-stripping, this review round's own docs made five files - +// including docs/issue-references.md, the convention's defining doc - permanently self-exempt, +// because documenting a marker in backticks activated it. These pin the fix. +check("a file marker quoted in a backtick span is a mention, not a use", () => { + assert.strictEqual(hasFileOptOut("put `issue-refs: exempt-file` anywhere in the file"), false); + assert.strictEqual(hasFileOptOut("real marker below\nissue-refs: exempt-file"), true, + "an unquoted marker still counts - only backtick spans are inert"); +}); + +check("a line marker quoted in a backtick span neither exempts its line nor blocks anything", () => { + const hits = suspectRefs(file("docs/x.md", + 'append `issue-refs: exempt` to the flagged line, e.g. for #857')); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#857"], + "the quoting line's own bare ref must still be flagged"); +}); + +check("a line QUOTING the block markers does not open a block - the doc-section shape", () => { + const hits = suspectRefs(file("docs/x.md", + "wrap it in `issue-refs: exempt-begin` / `issue-refs: exempt-end` markers", + "and a bare #57 after it must still be flagged")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#57"]); +}); + +check("an unquoted line carrying BOTH block markers changes no state", () => { + const hits = suspectRefs(file("docs/x.md", + "use issue-refs: exempt-begin and issue-refs: exempt-end around it", + "bare #58 after the discussing line is still flagged")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#58"]); +}); + +check("a fence in the PR body closes any open exempt block - fail closed, not open", () => { + // The end marker sits INSIDE the fence, where prBodyEntry blanks it; before the synthetic + // fence-close rule, the block never ended and #200 silently escaped. + const hits = suspectRefs(body( + "before #100", + "", + "```", + "quoted #857", + "", + "```", + "after #200")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#100", "#200"], + "#100 before the block and #200 after the fence must both flag; only the fenced quote is spared"); +}); + +check("a fence with no open block stays a plain fence - synthetic close is a no-op", () => { + const hits = suspectRefs(body("real text #100", "```", "fenced #857", "```", "tail #200")); + assert.deepStrictEqual(hits.map((h) => h.ref), ["#100", "#200"]); +}); + // dropFileOptedOutHits is the shared control flow both callers run (local script and CI step), -// each supplying only its reader - so the drift-prone part is pinned here, once. -(async () => { +// each supplying only its reader - so the drift-prone part is pinned here, once. The block is +// captured and awaited before the summary line prints, so the count and ordering stay honest. +const asyncChecks = (async () => { const HITS3 = [ { file: "docs/a.md", ref: "#857", text: "a" }, { file: "docs/a.md", ref: "#858", text: "a2" }, @@ -508,4 +560,4 @@ check("every hit is listed, with its count in the first line", () => { assert.ok(msg.includes(" a.md: #29 and #29")); }); -console.log("\n" + run + " assertions passed"); +asyncChecks.then(() => console.log("\n" + run + " assertions passed")); diff --git a/AGENTS.md b/AGENTS.md index 1b76e7bf8..31d4715e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -359,7 +359,8 @@ flip - `#29` and `#114` mean different things in each repo. than a broken one**. Cite both numbers, fork first: `(astubbs#119, confluentinc#857)`. - **`Fixes astubbs#167` closes nothing** - closing keywords need `astubbs/parallel-consumer#167`. - **Run `bin/check-issue-refs.sh` before you push.** It calls the same gate module CI does, so the - rule cannot drift; a red run is always real. CI additionally scans the PR body. + rule cannot drift; a red run is always real. Both scan the PR body when one is reachable; before + a PR exists, the body stays CI's to catch. [`docs/issue-references.md`](docs/issue-references.md) **owns this topic** - the threshold, the exemptions, the reasoning - and wins where the two disagree. diff --git a/bin/test-check-issue-refs.sh b/bin/test-check-issue-refs.sh index 8cdffddcf..a5376ca9e 100755 --- a/bin/test-check-issue-refs.sh +++ b/bin/test-check-issue-refs.sh @@ -111,12 +111,21 @@ check "clean diff and clean body pass, and the success line says the body was ch 0 "PR 42's body" # --- the guidance survives truncation to the tail ------------------------------------------------- +# The trailer formatFailure emits is four lines, led by "Fix: qualify each ref" - so the tail width +# must cover all four, and the assertion must include the LEAD line: tail -3 passed for months +# while provably cutting the very sentence the trailer exists to deliver. export GH_STUB_JSON='{"number":42,"body":"This fixes #858 for good."}' # issue-refs: exempt - fixture -out="$(bash bin/check-issue-refs.sh master 2>&1 | tail -3)" -case "$out" in - *"issue-refs: N/A"*) echo "ok: the last three lines of a failure still name the opt-out" ;; - *) echo "FAIL: the opt-out reminder did not survive | tail -3"; echo "$out" | sed 's/^/ /'; fails=$((fails + 1)) ;; -esac +out="$(bash bin/check-issue-refs.sh master 2>&1 | tail -4)" +tail_ok=1 +case "$out" in *"Fix: qualify each ref"*) ;; *) tail_ok=0 ;; esac +case "$out" in *"issue-refs: N/A"*) ;; *) tail_ok=0 ;; esac +if [ "$tail_ok" = 1 ]; then + echo "ok: the last four lines of a failure carry the whole fix/opt-out reminder" +else + echo "FAIL: the fix/opt-out reminder did not survive | tail -4 intact" + echo "$out" | sed 's/^/ /' + fails=$((fails + 1)) +fi if [ "$fails" -gt 0 ]; then echo "$fails case(s) failed" diff --git a/docs/issue-references.md b/docs/issue-references.md index 10df35dfa..d1a0bbd4e 100644 --- a/docs/issue-references.md +++ b/docs/issue-references.md @@ -153,7 +153,10 @@ fixtures. **Four opt-out scopes - reach for the narrowest that fits.** The in-file ones are written in whatever comment syntax the file already has - `// ...` in code, `` in markdown/HTML, where they are invisible when rendered - and because they live in the file they keep holding: -future PRs touching the text, and local runs on branches with no PR yet. They are the tool for the +future PRs touching the text, and local runs on branches with no PR yet. **Mention is not use:** +a marker quoted in a backtick code span - as every mention in this section is - is documentation +and activates nothing. Write marker names in backticks whenever writing ABOUT them; an unquoted +prose mention is indistinguishable from use and will be treated as one. They are the tool for the ref that genuinely must stay bare - above all **quoted source material**, where qualifying the number would edit the quotation. (A backslash escape was considered and rejected for that job - the design comment above `LINE_OPT_OUT` in `.github/scripts/issue-ref-gate.js` owns the why.) @@ -165,7 +168,11 @@ the design comment above `LINE_OPT_OUT` in `.github/scripts/issue-ref-gate.js` o run of quoted lines too dense to mark individually. The markers govern added lines in the same patch, so they cover the paste they arrive with; a later lone edit inside an old block carries its own marker. An unclosed `exempt-begin` swallows the rest of the file's added lines, - deliberately - the same direction an unclosed fence fails in. + deliberately - the same direction an unclosed fence fails in. In a PR body, **a code fence + closes any open block**: fenced content is already skipped wholesale, and an end marker placed + inside a fence would otherwise be invisible to the gate, leaving the block open forever - so + the block ends at the fence and anything the author meant to exempt after it gets flagged, + seen, and re-marked. - **A file: put `issue-refs: exempt-file` anywhere in it** - for a file legitimately full of bare refs (a generated document, an imported archive). This one is judged against the file's **content**, not the patch - locally from disk, in CI via the contents API, fetched only for From a4196c6a2b187eb83ecc9bf7aaafc73dd9378e61 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Tue, 18 Aug 2026 13:44:48 +1200 Subject: [PATCH 5/5] fix(review): bound the pre-commit gh call at 1000ms, inside the hook's actual budget The cloud review caught the arithmetic the earlier bound ignored: timeout 3000ms justified itself against .githooks/pre-commit's "~1.5s" budget while being 2x that budget on its own, for one of SIX sequential gates. Now 1000ms, with the comment rewritten to carry the real reasoning: a slow gh answer must lose to the hook staying fast, and a body the pre-commit run missed is still caught at CI - a deferred check, never a lost one. Reviewer's optional note (a second owner/repo parser vs bin/check-quarantine-owners.sh's sed version) deliberately not taken: the two live in different languages (bash+sed vs a JS regex inside a node heredoc), so sharing an implementation buys drift risk in glue code instead of removing it - the reviewer's own assessment. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn --- bin/check-issue-refs.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bin/check-issue-refs.sh b/bin/check-issue-refs.sh index 808b0237c..ddfa6e86e 100755 --- a/bin/check-issue-refs.sh +++ b/bin/check-issue-refs.sh @@ -86,9 +86,12 @@ try { // Mirror CI's body handling whenever a body is reachable: honour the opt-out, scan the body. // `gh pr view` resolves the PR from the current branch; ANY failure (no PR yet, gh missing, -// offline, or the 3s timeout - this runs inside the pre-commit hook, whose whole budget is -// ~1.5s, so a hung network call must be bounded) degrades to the body-less behaviour, and the -// failure message says which run this was. +// offline, or the timeout) degrades to the body-less behaviour, and the failure message says +// which run this was. The timeout is 1000ms because this script is one of SIX sequential gates +// in .githooks/pre-commit, whose stated budget is ~1.5s TOTAL - a slower-than-that gh answer +// must lose to the hook staying fast, or the hook gets --no-verify'd by habit and protects +// nothing. A body the pre-commit run missed is still caught at CI, so the cost of a timeout +// here is a deferred check, never a lost one. let prBody = null, prNumber = null; try { // The branch is passed explicitly because `gh pr view` REFUSES to infer it when -R is given @@ -98,7 +101,7 @@ try { if (branch === "HEAD") throw new Error("detached HEAD - no branch to resolve a PR from"); const pr = JSON.parse(execFileSync("gh", ["pr", "view", branch, ...(repo ? ["-R", repo] : []), "--json", "number,body"], - { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], timeout: 3000 })); + { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], timeout: 1000 })); prBody = pr.body || ""; prNumber = pr.number; } catch { /* body unreadable this run */ }