golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

chore(#3211): accept a non-closing issue reference for docs/test-only PRs (#3289)

Browse Source 
* test(#3211): failing-first coverage for the issue-link follow-up exemption

Adds the regression suite before the policy module exists, so the RED state
is recorded against a real verdict rather than asserted. Covers the reported
gap (a fork test-only follow-up PR cannot satisfy the gate without an inert
closing keyword) and the file-list truncation vector that any diff-shape
exemption must fail closed on.

Refs #3211

* chore(#3211): accept a non-closing issue reference for docs/test-only PRs

The `Issue link required` gate modelled exactly one PR->issue relationship —
"this PR closes that issue" — and its sole exemption additionally required
same-repo identity (#1389), so a fork PR had no exemption path of any kind. A
test-only or docs-only follow-up therefore had to ship a knowingly-inert
`Closes #<already-closed-issue>` to get a green check.

The verdict now lives in scripts/require-issue-link-policy.cjs as a pure,
unit-tested function returning a typed reason. It additionally accepts a
non-closing reference (`Refs #N`, `Follow-up to #N`, ...) but only when every
changed file is under tests/, under docs/, or is a root-level *.md — the same
doc-only shape pre-pr-gate.sh:111 recognizes, minus CHANGELOG.md, which
changeset/lint.cjs classes as user-facing. Source-touching PRs still require a
closing keyword and a PR with no reference at all still hard-fails, so gate
strength is unchanged.

Both constraints the issue names as hard requirements are preserved: the
backmerge exemption keeps its same-repo conjunct, and the failing step's `if:`
stays step-level so the required check reports SUCCESS rather than a
branch-protection-blocking `skipped`.

Also closes a forgery vector found while building this. `gh pr view --json
files` returns at most 100 paths and does not paginate, while the payload's
`changed_files` reports the true total (verified live: PR #3202 returns 100 of
118). A >100-file PR could therefore present a falsely tests-only list. The new
shared helper scripts/lib/pr-changed-files.cjs fails closed when the list
cannot be confirmed complete, and the pre-existing tooling-paths carve-out in
scripts/pr-template-policy.cjs — which relaxed template enforcement on the same
untrustworthy list — now uses it too.

Closes #3211

* fix(#3211): treat the authoritative file count as authority at every list size

Both orthogonal review passes independently found the same blocker.
`fileListIsComplete` only compared the list length against the PR's true
`changed_files` count once the list reached the 100-entry page cap, so any
mechanism that shortened the list BELOW the cap went undetected:

  evaluateIssueLink({prBody:"Refs #1", sameRepo:false,
                     changedFiles:["CONTRIBUTING.md"], changedFilesTotal:3})
  -> {ok:true, reason:"ok_followup_reference"}

The concrete exploit was a $GITHUB_OUTPUT heredoc collision. Both this
workflow and pr-template-format.yml wrote the file list with a fixed
terminator (`GSD_EOF` / the even weaker `EOF`), and every path in that value
is attacker-controlled on a fork PR. A file named after the delimiter closes
the value early and drops every path after it, so a fork PR touching src/
could present a list of only its exempt-looking files and take the follow-up
exemption. That is exactly the #1389 property this change is required to
preserve.

Fixed in two independent layers:

  1. The total is now the authority at every size, not only at/above the cap.
     One rule catches truncation, delimiter collision, and a path containing
     a newline, without having to enumerate the mechanisms.
  2. Both workflows now use an unguessable random delimiter, per GitHub's
     documented guidance for untrusted multiline output.

Also from review: pr-template-format.yml never passed CHANGED_FILES_TOTAL, so
the parameter threaded through evaluatePrTemplate was always undefined in
production and would have permanently blocked the tooling carve-out for any
100+-file PR; its env is now wired. Root-doc exclusion is case-insensitive.
Dropped a no-op `tr '\n' '\n'`.

Refs #3211

* chore(#3211): regenerate install-tree fixtures for the new shared helper

scripts/lib/** ships in the install tree, so adding
scripts/lib/pr-changed-files.cjs drifts all 19 golden fixtures by exactly one
path each. Caught by tests/golden-install-tree.test.cjs (25 failures on the
remote runner), which is the drift detector doing its job — not a defect.

Placement is deliberate: every existing occupant of scripts/lib/ is a CI/dev
helper that already ships (alias-drift-families, allowlist-ratchet, cli-exit,
drift-scan), so a shared helper used by two policy scripts belongs there. The
two policy modules themselves live at the top level of scripts/ and do not
ship.

Regenerated with `npm run gen:install-tree`; the delta is one added path per
fixture and nothing else.

Refs #3211

* fix(#3211): keep the shared CI helper out of the shipped install tree

The remote runner reported 6 failures on the previous head. Two causes.

`scripts/lib/**` is enumerated in `bin/install.js` (GSD_SCRIPTS_LIB_FILES) and
ships to users, and the install suite asserts that enumeration is complete.
Putting the new shared helper there broke four install tests and drifted all 19
golden install-tree fixtures. The right answer is not to add it to the manifest
— it is CI-only tooling used by two scripts that do not ship, so it has no
business in a user's config directory. Moved to `scripts/pr-changed-files.cjs`;
top-level `scripts/` ships only what the installer names explicitly, so nothing
is enumerated and nothing ships. The fixture regeneration from the previous
commit is reverted: the install-tree fixtures are byte-identical to `next`
again, and `bin/` is untouched. That also keeps the diff free of any
user-facing path, so no changeset fragment is required.

The other failure was a stale test, not a regression. The workflow carve-out
suite asserted the backmerge exemption by grepping require-issue-link.yml for
`startsWith(github.head_ref, ...)` and `steps.check.outputs.found`. This change
moved the whole verdict — carve-out included — into the policy module and
renamed the step, so those assertions measured a location the logic no longer
occupies.

Rewritten to lock the property at its new home, and made stronger in the
process: the step-level placement is now verified by PARSING the YAML and
asserting the job carries no `if:` of its own (a job-level `if:` would make the
required check report `skipped` and block branch protection), and the #1389
anti-forgery conjunct is asserted BEHAVIORALLY against evaluateIssueLink for
both sameRepo branches rather than by matching text. The bootstrap fallback
grep is locked too, so the introducing-PR path cannot be silently dropped.

Refs #3211

* fix(#3211): correct the contributor guidance and pin it against the rule

The sticky comment the gate posts still described the qualifying diff shape as
"nothing outside tests/ and docs/". The predicate had since been widened to
also accept root-level *.md, so the guidance was narrower than the rule it
describes — and narrower in the worst direction: a contributor whose PR is
CONTRIBUTING.md plus a test, which is exactly the shape #3211 was filed about,
would have been told they do not qualify while the gate was in fact passing
them. The two failure explanations now name all three accepted shapes and the
CHANGELOG.md exclusion.

This is a shared-rule-across-parallel-surfaces drift: the guidance restates a
rule whose definition lives in EXEMPT_PATH_PREFIXES / isRootLevelDoc /
EXCLUDED_ROOT_DOCS. It was caught by eye, which is not a control. Added the
parity assertion CLAUDE.md prescribes for exactly this: the test parses the
workflow, pulls the github-script body out of the failing step, and asserts it
names every entry of EXEMPT_PATH_PREFIXES and every entry of
EXCLUDED_ROOT_DOCS — derived from the module's exports, never from a second
hardcoded copy — plus the root-level shape and an actionable `Refs #` example.

The test is non-vacuous by construction and by demonstration: it guards against
zero-length iteration and an empty script body, and removing any single
expected token from the real text makes it fail (verified per token, plus the
empty-string case which reports all five missing).

Refs #3211

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-09 21:38:53 -04:00
committed by GitHub
parent c2f24265f2
commit dced41f536
10 changed files with 1086 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

63
scripts/pr-changed-files.cjs Normal file
View File

@@ -0,0 +1,63 @@
'use strict';
/**
* Shared truncation-detection helper for `gh pr view --json files` (#3211).
*
* GitHub's GraphQL `files` connection on a pull request is a paginated
* connection. `gh pr view --json files` requests it with a single page at
* `first: 100` and does NOT paginate through the rest — so any PR touching
* more than 100 files silently returns only the first 100, with no error and
* no indication in the `files` array itself that entries are missing.
* `changedFiles` (a separate, non-paginated scalar field) reports the true
* total. A policy that relaxes a gate based on "every changed file matches
* pattern X" must treat a `files` list it cannot confirm is complete as
* incomplete — i.e. fail closed — rather than silently approving a PR whose
* 101st+ file might violate the policy.
*
* This file lives at the top level of scripts/ rather than in scripts/lib/
* because scripts/lib/** is enumerated in bin/install.js and ships to users
* on install; this is CI-only tooling with no reason to be installed.
*/
// GitHub's GraphQL `files` connection is requested at `first: 100` by
// `gh pr view --json files`; `gh` does not paginate this field.
const FILE_LIST_PAGE_LIMIT = 100;
/**
* Returns true iff `changedFiles` can be trusted as the COMPLETE list of
* files changed in the PR (i.e. it was not silently truncated).
*
* - An empty/non-array list cannot confirm anything about the PR — mirror
* the fail-closed stance `allPathsAreTooling` already takes on an empty
* list in scripts/pr-template-policy.cjs.
* - When the true total is known it is the AUTHORITY, at every list size —
* not just when the list has hit the page cap. The 100-entry page cap is
* only ONE way a list can be short of the truth; a `$GITHUB_OUTPUT`
* heredoc terminated early by an attacker-named file, or a path
* containing a newline, truncates or inflates the list just as
* effectively, and at any length. Comparing against the total catches all
* of them without enumerating the mechanisms.
* - Only when no total is supplied do we fall back to the page-cap
* heuristic: a list below the cap cannot have been truncated BY THE CAP,
* which is the only mechanism a caller without a total can rule out.
*/
function fileListIsComplete(changedFiles, changedFilesTotal) {
if (!Array.isArray(changedFiles) || changedFiles.length === 0) return false;
if (Number.isInteger(changedFilesTotal)) return changedFilesTotal === changedFiles.length;
return changedFiles.length < FILE_LIST_PAGE_LIMIT;
}
/**
* Parses a newline-delimited env var (as produced by `git diff --name-only`
* or `gh pr view --json files -q '.files[].path'`) into a trimmed,
* empty-line-free array. Returns [] for null/undefined/empty input.
*/
function parseChangedFilesEnv(value) {
if (!value) return [];
return String(value)
.split(/\r?\n/)
.map((line) => line.trim())
.filter(Boolean);
}
module.exports = { fileListIsComplete, FILE_LIST_PAGE_LIMIT, parseChangedFilesEnv };

18
scripts/pr-template-policy.cjs
View File

@@ -1,6 +1,7 @@
#!/usr/bin/env node
const { matchesGlob } = require('path');
const { fileListIsComplete, parseChangedFilesEnv } = require('./pr-changed-files.cjs');
const TRUSTED_AUTHOR_ASSOCIATIONS = new Set([
'CONTRIBUTOR',
@@ -142,8 +143,12 @@ function matchingTemplate(body) {
* pattern in the allowlist. Returns false for an empty file list (no
* files means we cannot confirm it is a tooling-only PR).
*/
function allPathsAreTooling(changedFiles, allowlist) {
function allPathsAreTooling(changedFiles, allowlist, changedFilesTotal) {
if (!Array.isArray(changedFiles) || changedFiles.length === 0) return false;
// #3211: `gh pr view --json files` truncates at 100 entries with no
// in-band signal. A truncated list must not relax enforcement — an unseen
// 101st+ file could fall outside the tooling allowlist entirely.
if (!fileListIsComplete(changedFiles, changedFilesTotal)) return false;
return changedFiles.every((file) =>
allowlist.some((pattern) => matchesGlob(file, pattern)),
);
@@ -159,13 +164,13 @@ function hasExemptMarker(body, regex) {
return match[1].trim().length > 0;
}
function evaluatePrTemplate(body, authorAssociation, changedFiles) {
function evaluatePrTemplate(body, authorAssociation, changedFiles, changedFilesTotal) {
const association = String(authorAssociation || '').toUpperCase();
const trusted = TRUSTED_AUTHOR_ASSOCIATIONS.has(association);
const normalizedBody = String(body || '').trim();
// --- Carve-out 1: all changed files are in the tooling allowlist ---
if (allPathsAreTooling(changedFiles, TOOLING_PATH_ALLOWLIST)) {
if (allPathsAreTooling(changedFiles, TOOLING_PATH_ALLOWLIST, changedFilesTotal)) {
return {
valid: true,
action: 'pass',
@@ -235,13 +240,18 @@ function evaluatePrTemplate(body, authorAssociation, changedFiles) {
}
function main() {
// Preserve the existing distinction between "unset" (undefined) and
// "set but empty" ([]) — allPathsAreTooling treats them differently.
const changedFiles = process.env.CHANGED_FILES
? process.env.CHANGED_FILES.split('\n').map((f) => f.trim()).filter(Boolean)
? parseChangedFilesEnv(process.env.CHANGED_FILES)
: undefined;
const parsedTotal = Number.parseInt(process.env.CHANGED_FILES_TOTAL, 10);
const changedFilesTotal = Number.isNaN(parsedTotal) ? undefined : parsedTotal;
const result = evaluatePrTemplate(
process.env.PR_BODY || '',
process.env.AUTHOR_ASSOCIATION || '',
changedFiles,
changedFilesTotal,
);
process.stdout.write(`${JSON.stringify(result)}\n`);
if (process.env.GITHUB_OUTPUT) {

192
scripts/require-issue-link-policy.cjs Normal file
View File

@@ -0,0 +1,192 @@
#!/usr/bin/env node
'use strict';
/**
* Require-issue-link policy (#3211, preserving #1389).
*
* Replaces the shell-only `grep -qiE '(closes|fixes|resolves)\s+#[0-9]+'`
* step with a pure, testable `evaluateIssueLink` verdict function so a PR
* that only REFERENCES an issue (rather than closing it — e.g. a follow-up
* regression-coverage PR) is not forced to fabricate a closing keyword, while
* two pre-existing constraints are preserved exactly:
*
* 1. (#1389, anti-forgery) The backmerge exemption only fires when the
* head ref carries the backmerge prefix AND the PR is same-repo (not a
* fork) — dropping the `sameRepo` conjunct would let a fork forge a
* branch name to bypass the issue-link requirement entirely.
* 2. (#3211, truncation) `gh pr view --json files` truncates the file list
* at 100 entries with no in-band signal that it did so (see
* scripts/pr-changed-files.cjs). The reference-only carve-out below
* only applies when every changed file is a test/doc file, so a PR
* whose file list may be truncated must fail closed rather than let an
* unseen 101st+ file (which could touch `src/`) slip through.
*
* Tests assert on the typed ISSUE_LINK_REASON enum, never on free text.
*/
const { fileListIsComplete, parseChangedFilesEnv } = require('./pr-changed-files.cjs');
const { runMain } = require('./lib/cli-exit.cjs');
const ISSUE_LINK_REASON = Object.freeze({
OK_CLOSING_KEYWORD: 'ok_closing_keyword',
OK_BACKMERGE_EXEMPT: 'ok_backmerge_exempt',
OK_FOLLOWUP_REFERENCE: 'ok_followup_reference',
FAIL_NO_ISSUE_REFERENCE: 'fail_no_issue_reference',
FAIL_REFERENCE_NEEDS_CLOSING: 'fail_reference_needs_closing',
FAIL_FILE_LIST_INCOMPLETE: 'fail_file_list_incomplete',
});
// #1389: backmerge PRs are opened by CI against `next`, never by a human or a
// fork, so they are exempt from the issue-link requirement outright — but
// ONLY when combined with `sameRepo === true` below (see header comment).
const BACKMERGE_BRANCH_PREFIX = 'chore/backmerge-main-to-next-';
// A follow-up-only PR (references an issue without closing it) is only
// allowed to skip the closing keyword when every changed file is a test or
// doc file — i.e. it cannot be the PR that actually implements the fix.
const EXEMPT_PATH_PREFIXES = ['tests/', 'docs/'];
// Root-level markdown is documentation — this mirrors the repo's own doc-only
// classifier (.claude/hooks/pre-pr-gate.sh:111), whose `[^/]+\.md` anchor is
// deliberately root-only so runtime-loaded text under a subdirectory
// (gsd-core/workflows/*.md, agents/*.md, commands/**/*.md) stays gated.
// CHANGELOG.md is excluded: scripts/changeset/lint.cjs classes a direct edit to
// it as user-facing precisely to close a bypass, so it must not ride in on the
// docs carve-out either.
const EXCLUDED_ROOT_DOCS = new Set(['CHANGELOG.md']);
// Case-insensitive lookup set derived from EXCLUDED_ROOT_DOCS. The exclusion
// check below is case-insensitive because the `.md` extension test above it
// already is (`/\.md$/i`) — `changelog.md` or `CHANGELOG.MD` would otherwise
// slip past the exclusion while still passing the extension test. In
// practice this is defense in depth rather than a live bypass: the GitHub
// API always reports the real path with its actual, fixed casing (the
// filesystem is case-sensitive on the runners this executes on), so a PR
// cannot rename CHANGELOG.md to bypass the check by casing alone.
const EXCLUDED_ROOT_DOCS_UPPER = new Set(
Array.from(EXCLUDED_ROOT_DOCS, (name) => name.toUpperCase()),
);
function isRootLevelDoc(normalizedPath) {
if (!normalizedPath) return false;
if (normalizedPath.includes('/')) return false;
if (!/\.md$/i.test(normalizedPath)) return false;
if (EXCLUDED_ROOT_DOCS_UPPER.has(normalizedPath.toUpperCase())) return false;
return true;
}
// Mirrors the shipped `grep -qiE '(closes|fixes|resolves)\s+#[0-9]+'` exactly:
// no additional keywords, no `\b` anchors (the shell grep it replaces has
// none either) — see the corpus-parity test in
// tests/require-issue-link-policy.test.cjs.
const CLOSING_KEYWORD_REGEX = /(?:closes|fixes|resolves)\s+#[0-9]+/i;
// Accepts a soft "this PR relates to #N" reference without claiming to close
// it. The leading `\b` prevents matching inside a longer word (e.g. `xref#1`,
// `prefs #1`) because there is no word-boundary transition between the
// preceding word character and the start of the alternative — verified
// explicitly for `preferences #1` / `unreferenced #1` in the test suite.
const FOLLOWUP_REFERENCE_REGEX = /\b(?:refs?|references?|relates\s+to|related\s+to|follow[-\s]?up\s+to)\s+#[0-9]+/i;
function hasClosingKeyword(body) {
return CLOSING_KEYWORD_REGEX.test(String(body || ''));
}
function hasFollowUpReference(body) {
return FOLLOWUP_REFERENCE_REGEX.test(String(body || ''));
}
// Path separator normalization — unconditional, per repo convention (see
// CLAUDE.md "Path-sep normalization"), so a Windows-style diff entry like
// `tests\windows\a.test.cjs` is still recognized as tests/-prefixed.
function normalizePath(p) {
return String(p).replace(/\\/g, '/');
}
/**
* Returns true iff every changed file qualifies as a test/doc file, i.e. each
* one is one of:
* 1. under one of EXEMPT_PATH_PREFIXES (`tests/`, `docs/`) — the trailing
* slash on each prefix makes this directory-boundary aware, so
* lookalikes like `tests-e2e/`, `testsuite/`, or `docsite/` are
* correctly rejected (they are not `tests/` or `docs/`); or
* 2. a root-level markdown file (no `/`, `.md` extension, case-insensitive)
* that is not in EXCLUDED_ROOT_DOCS — see isRootLevelDoc.
*/
function allPathsAreTestsOrDocs(changedFiles) {
if (!Array.isArray(changedFiles) || changedFiles.length === 0) return false;
return changedFiles.every((file) => {
const normalized = normalizePath(file);
if (EXEMPT_PATH_PREFIXES.some((prefix) => normalized.startsWith(prefix))) return true;
return isRootLevelDoc(normalized);
});
}
function evaluateIssueLink({ prBody, headRef, sameRepo, changedFiles, changedFilesTotal }) {
if (hasClosingKeyword(prBody)) {
return { ok: true, reason: ISSUE_LINK_REASON.OK_CLOSING_KEYWORD };
}
// #1389: `sameRepo === true` is required, not just the branch-name prefix —
// a fork PR could otherwise name its branch to forge this exemption.
if (String(headRef || '').startsWith(BACKMERGE_BRANCH_PREFIX) && sameRepo === true) {
return { ok: true, reason: ISSUE_LINK_REASON.OK_BACKMERGE_EXEMPT };
}
if (!hasFollowUpReference(prBody)) {
return { ok: false, reason: ISSUE_LINK_REASON.FAIL_NO_ISSUE_REFERENCE };
}
// #3211: a truncated file list cannot be trusted to prove "tests/docs
// only" — fail closed rather than risk approving on an unseen file.
if (!fileListIsComplete(changedFiles, changedFilesTotal)) {
return { ok: false, reason: ISSUE_LINK_REASON.FAIL_FILE_LIST_INCOMPLETE };
}
if (allPathsAreTestsOrDocs(changedFiles)) {
return { ok: true, reason: ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE };
}
return { ok: false, reason: ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING };
}
function main() {
const changedFiles = parseChangedFilesEnv(process.env.CHANGED_FILES);
const parsedTotal = Number.parseInt(process.env.CHANGED_FILES_TOTAL, 10);
const changedFilesTotal = Number.isNaN(parsedTotal) ? undefined : parsedTotal;
const result = evaluateIssueLink({
prBody: process.env.PR_BODY || '',
headRef: process.env.HEAD_REF || '',
sameRepo: process.env.SAME_REPO === 'true',
changedFiles,
changedFilesTotal,
});
process.stdout.write(`${JSON.stringify(result)}\n`);
if (process.env.GITHUB_OUTPUT) {
const fs = require('node:fs');
fs.appendFileSync(process.env.GITHUB_OUTPUT, `ok=${result.ok ? 'true' : 'false'}\n`);
fs.appendFileSync(process.env.GITHUB_OUTPUT, `reason=${result.reason}\n`);
fs.appendFileSync(process.env.GITHUB_OUTPUT, `result=${JSON.stringify(result)}\n`);
}
return result.ok ? 0 : 1;
}
if (require.main === module) runMain(main);
module.exports = {
ISSUE_LINK_REASON,
BACKMERGE_BRANCH_PREFIX,
EXEMPT_PATH_PREFIXES,
EXCLUDED_ROOT_DOCS,
CLOSING_KEYWORD_REGEX,
FOLLOWUP_REFERENCE_REGEX,
hasClosingKeyword,
hasFollowUpReference,
normalizePath,
isRootLevelDoc,
allPathsAreTestsOrDocs,
evaluateIssueLink,
};