* 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>
334 lines
13 KiB
JavaScript
334 lines
13 KiB
JavaScript
const { describe, test } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
|
|
const { evaluatePrTemplate, allPathsAreTooling, hasExemptMarker, TOOLING_PATH_ALLOWLIST, EXEMPT_MARKER_REGEX } = require('../scripts/pr-template-policy.cjs');
|
|
const { FILE_LIST_PAGE_LIMIT } = require('../scripts/pr-changed-files.cjs');
|
|
|
|
const fixBody = [
|
|
'## Fix PR',
|
|
'',
|
|
'## Linked Issue',
|
|
'Fixes #123',
|
|
'',
|
|
'## What was broken',
|
|
'The thing was broken.',
|
|
'',
|
|
'## What this fix does',
|
|
'The thing now works.',
|
|
'',
|
|
'## Root cause',
|
|
'A missing guard.',
|
|
'',
|
|
'## Testing',
|
|
'node --test tests/example.test.cjs',
|
|
'',
|
|
'## Checklist',
|
|
'- [x] Issue linked above with `Fixes #NNN`',
|
|
].join('\n');
|
|
|
|
const enhancementBody = [
|
|
'## Enhancement PR',
|
|
'',
|
|
'## Linked Issue',
|
|
'Closes #123',
|
|
'',
|
|
'## What this enhancement improves',
|
|
'Existing output.',
|
|
'',
|
|
'## Before / After',
|
|
'**Before:** noisy',
|
|
'**After:** clear',
|
|
'',
|
|
'## How it was implemented',
|
|
'Small refactor.',
|
|
'',
|
|
'## Testing',
|
|
'node --test tests/example.test.cjs',
|
|
'',
|
|
'## Scope confirmation',
|
|
'- [x] Matches approved issue.',
|
|
'',
|
|
'## Checklist',
|
|
'- [x] Tests pass',
|
|
].join('\n');
|
|
|
|
const featureBody = [
|
|
'## Feature PR',
|
|
'',
|
|
'## Linked Issue',
|
|
'Closes #123',
|
|
'',
|
|
'## Feature summary',
|
|
'Adds a new thing.',
|
|
'',
|
|
'## What changed',
|
|
'### New files',
|
|
'None.',
|
|
'### Modified files',
|
|
'One file.',
|
|
'',
|
|
'## Implementation notes',
|
|
'Implemented as approved.',
|
|
'',
|
|
'## Spec compliance',
|
|
'- [x] Criterion met',
|
|
'',
|
|
'## Testing',
|
|
'node --test tests/example.test.cjs',
|
|
'',
|
|
'## Scope confirmation',
|
|
'- [x] Exact scope.',
|
|
'',
|
|
'## Checklist',
|
|
'- [x] Tests pass',
|
|
].join('\n');
|
|
|
|
describe('pr-template-policy carve-out', () => {
|
|
// A. Path-scope auto-skip — CI-only PR is accepted
|
|
test('auto-skips enforcement for CI-only changed files', () => {
|
|
const result = evaluatePrTemplate('This is a CI change with no template.', 'NONE', ['.github/workflows/test.yml']);
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.skipped, 'tooling-paths');
|
|
});
|
|
|
|
// B. Path-scope auto-skip — docs-only PR is accepted
|
|
test('auto-skips enforcement for docs-only changed files', () => {
|
|
const result = evaluatePrTemplate('Updated documentation.', 'NONE', ['docs/CONFIGURATION.md', 'README.md']);
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.skipped, 'tooling-paths');
|
|
});
|
|
|
|
// C. Path-scope auto-skip — dependency-bump PR is accepted
|
|
test('auto-skips enforcement for dependency-bump changed files', () => {
|
|
const result = evaluatePrTemplate('Bump deps.', 'NONE', ['package.json', 'package-lock.json']);
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.skipped, 'tooling-paths');
|
|
});
|
|
|
|
// D. Path-scope auto-skip is path-strict — mixed PR still enforced
|
|
test('does NOT skip enforcement when any changed file is outside the tooling allowlist', () => {
|
|
const result = evaluatePrTemplate('Mixed change.', 'NONE', ['.github/workflows/test.yml', 'src/feature.ts']);
|
|
assert.equal(result.valid, false);
|
|
assert.equal(result.action, 'close');
|
|
});
|
|
|
|
// E. Explicit marker accepted (non-empty reason)
|
|
test('auto-skips enforcement when PR body contains a valid exempt marker with a reason', () => {
|
|
const body = '<!-- pr-template-exempt: dropping node 26 lane -->\n\nThis removes the Node 26 CI lane.';
|
|
const result = evaluatePrTemplate(body, 'NONE', ['src/anything.ts']);
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.skipped, 'exempt-marker');
|
|
});
|
|
|
|
// F. Explicit marker requires non-empty reason
|
|
test('does NOT skip enforcement when exempt marker has an empty reason', () => {
|
|
const bodyEmpty = '<!-- pr-template-exempt: -->\n\nSome description.';
|
|
const bodyNoReason = '<!-- pr-template-exempt: -->\n\nSome description.';
|
|
const r1 = evaluatePrTemplate(bodyEmpty, 'NONE', ['src/anything.ts']);
|
|
const r2 = evaluatePrTemplate(bodyNoReason, 'NONE', ['src/anything.ts']);
|
|
assert.equal(r1.valid, false);
|
|
assert.equal(r2.valid, false);
|
|
});
|
|
|
|
// G. Regression — DEFAULT_TEMPLATE_MARKERS still rejected
|
|
test('regression: DEFAULT_TEMPLATE_MARKERS body is still rejected when no carve-out applies', () => {
|
|
const body = 'Wrong template — please use a typed template.\n\nEvery PR must use a typed template.';
|
|
const result = evaluatePrTemplate(body, 'NONE', ['src/feature.ts']);
|
|
assert.equal(result.valid, false);
|
|
assert.match(result.reason, /default wrong-template guidance/);
|
|
});
|
|
|
|
// H. Regression — fix template still accepted
|
|
test('regression: fix template PR body is still accepted', () => {
|
|
const result = evaluatePrTemplate(fixBody, 'NONE', ['src/feature.ts']);
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.template, 'fix');
|
|
});
|
|
});
|
|
|
|
describe('pr-template-policy helper: allPathsAreTooling', () => {
|
|
test('returns true when all paths match the allowlist', () => {
|
|
assert.equal(allPathsAreTooling(['.github/workflows/ci.yml'], TOOLING_PATH_ALLOWLIST), true);
|
|
assert.equal(allPathsAreTooling(['package.json', 'package-lock.json'], TOOLING_PATH_ALLOWLIST), true);
|
|
assert.equal(allPathsAreTooling(['README.md'], TOOLING_PATH_ALLOWLIST), true);
|
|
});
|
|
|
|
test('returns false when any path does not match the allowlist', () => {
|
|
assert.equal(allPathsAreTooling(['.github/workflows/ci.yml', 'src/index.ts'], TOOLING_PATH_ALLOWLIST), false);
|
|
});
|
|
|
|
test('returns false for an empty file list', () => {
|
|
assert.equal(allPathsAreTooling([], TOOLING_PATH_ALLOWLIST), false);
|
|
});
|
|
});
|
|
|
|
describe('pr-template-policy helper: hasExemptMarker', () => {
|
|
test('matches a marker with a non-empty reason', () => {
|
|
assert.equal(hasExemptMarker('<!-- pr-template-exempt: ci -->', EXEMPT_MARKER_REGEX), true);
|
|
assert.equal(hasExemptMarker('<!-- pr-template-exempt: dropping node 26 lane -->', EXEMPT_MARKER_REGEX), true);
|
|
assert.equal(hasExemptMarker('<!-- pr-template-exempt: drop node-26 lane -->', EXEMPT_MARKER_REGEX), true);
|
|
});
|
|
|
|
test('does not match a marker with empty or whitespace-only reason', () => {
|
|
assert.equal(hasExemptMarker('<!-- pr-template-exempt: -->', EXEMPT_MARKER_REGEX), false);
|
|
assert.equal(hasExemptMarker('<!-- pr-template-exempt: -->', EXEMPT_MARKER_REGEX), false);
|
|
});
|
|
|
|
test('does not match a marker with no pr-template-exempt keyword', () => {
|
|
assert.equal(hasExemptMarker('<!-- gsd-pr-template-policy -->', EXEMPT_MARKER_REGEX), false);
|
|
assert.equal(hasExemptMarker('some random text', EXEMPT_MARKER_REGEX), false);
|
|
});
|
|
});
|
|
|
|
describe('pr-template-policy', () => {
|
|
test('passes PR bodies that use the fix template', () => {
|
|
const result = evaluatePrTemplate(fixBody, 'NONE');
|
|
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.template, 'fix');
|
|
});
|
|
|
|
test('passes PR bodies that use the enhancement template', () => {
|
|
const result = evaluatePrTemplate(enhancementBody, 'FIRST_TIMER');
|
|
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.template, 'enhancement');
|
|
});
|
|
|
|
test('does not flag default-template marker phrase inside a valid enhancement template', () => {
|
|
const body = enhancementBody.replace(
|
|
'## Linked Issue',
|
|
[
|
|
'> **Using the wrong template?**',
|
|
'> - Bug fix: use [fix.md](?template=fix.md)',
|
|
'> - New feature: use [feature.md](?template=feature.md)',
|
|
'',
|
|
'## Linked Issue',
|
|
].join('\n'),
|
|
);
|
|
const result = evaluatePrTemplate(body, 'COLLABORATOR');
|
|
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.template, 'enhancement');
|
|
});
|
|
|
|
test('passes PR bodies that use the feature template', () => {
|
|
const result = evaluatePrTemplate(featureBody, 'FIRST_TIME_CONTRIBUTOR');
|
|
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.template, 'feature');
|
|
});
|
|
|
|
test('closes first-time PRs that keep the default template', () => {
|
|
const result = evaluatePrTemplate([
|
|
'## Wrong template - please use the correct one for your PR type',
|
|
'',
|
|
'Every PR must use a typed template.',
|
|
].join('\n'), 'FIRST_TIMER');
|
|
|
|
assert.equal(result.valid, false);
|
|
assert.equal(result.action, 'close');
|
|
assert.equal(result.trusted, false);
|
|
assert.match(result.reason, /default wrong-template guidance/);
|
|
});
|
|
|
|
test('warns contributors instead of closing when the template is missing', () => {
|
|
const result = evaluatePrTemplate('This is a free-form PR body.', 'CONTRIBUTOR');
|
|
|
|
assert.equal(result.valid, false);
|
|
assert.equal(result.action, 'warn');
|
|
assert.equal(result.trusted, true);
|
|
});
|
|
|
|
test('warns collaborators, members, and owners instead of closing', () => {
|
|
for (const association of ['COLLABORATOR', 'MEMBER', 'OWNER']) {
|
|
const result = evaluatePrTemplate('This is a free-form PR body.', association);
|
|
|
|
assert.equal(result.valid, false);
|
|
assert.equal(result.action, 'warn');
|
|
assert.equal(result.trusted, true);
|
|
}
|
|
});
|
|
|
|
test('does not close for an unfilled issue slug when the template is present', () => {
|
|
const body = fixBody.replace('Fixes #123', 'Fixes #');
|
|
const result = evaluatePrTemplate(body, 'FIRST_TIMER');
|
|
|
|
assert.equal(result.valid, true);
|
|
assert.equal(result.action, 'pass');
|
|
assert.equal(result.template, 'fix');
|
|
});
|
|
|
|
test('closes first-time PRs that remove required template sections', () => {
|
|
const result = evaluatePrTemplate(fixBody.replace('## What was broken', '## Background'), 'NONE');
|
|
|
|
assert.equal(result.valid, false);
|
|
assert.equal(result.action, 'close');
|
|
assert.deepEqual(result.missingHeadings, ['What was broken']);
|
|
});
|
|
|
|
test('closes first-time PRs with empty body', () => {
|
|
const result = evaluatePrTemplate('', 'FIRST_TIMER');
|
|
|
|
assert.equal(result.valid, false);
|
|
assert.equal(result.action, 'close');
|
|
assert.match(result.reason, /PR body is empty; a typed pull request template is required\./);
|
|
});
|
|
|
|
test('warns trusted contributors with empty body', () => {
|
|
const result = evaluatePrTemplate('', 'CONTRIBUTOR');
|
|
|
|
assert.equal(result.valid, false);
|
|
assert.equal(result.action, 'warn');
|
|
assert.equal(result.trusted, true);
|
|
assert.match(result.reason, /PR body is empty; a typed pull request template is required\./);
|
|
});
|
|
});
|
|
|
|
// `gh pr view --json files` returns at most 100 paths and does not
|
|
// paginate, while the PR's `changedFiles` field reports the true total.
|
|
// The tooling-paths carve-out RELAXES template enforcement, so trusting a
|
|
// possibly-truncated list would let a >100-file PR skip enforcement on the
|
|
// strength of its first 100 (all-tooling) paths. Verified live: PR
|
|
// open-gsd/gsd-core#3202 returns 100 paths for 118 changed files.
|
|
describe('pr-template-policy carve-out — truncated file lists (#3211)', () => {
|
|
const toolingPaths = (n) => Array.from({ length: n }, (_, i) => `docs/generated-${i}.md`);
|
|
|
|
// A. Forgery vector — a truncated 100-of-118 list must not be trusted
|
|
test('a truncated 100-of-118 tooling file list does not skip enforcement', () => {
|
|
const result = evaluatePrTemplate('no template here', 'NONE', toolingPaths(FILE_LIST_PAGE_LIMIT), 118);
|
|
assert.equal(result.skipped, undefined);
|
|
// author association 'NONE' is untrusted, so enforcement closes the PR.
|
|
assert.equal(result.action, 'close');
|
|
});
|
|
|
|
// B. A 100-file tooling list whose total agrees is genuinely complete
|
|
test('a 100-file tooling list whose total agrees still skips enforcement', () => {
|
|
const result = evaluatePrTemplate('no template here', 'NONE', toolingPaths(FILE_LIST_PAGE_LIMIT), 100);
|
|
assert.equal(result.skipped, 'tooling-paths');
|
|
assert.equal(result.valid, true);
|
|
});
|
|
|
|
// C. A 100-file tooling list with no corroborating total fails closed
|
|
test('a 100-file tooling list with no corroborating total fails closed', () => {
|
|
const result = evaluatePrTemplate('no template here', 'NONE', toolingPaths(FILE_LIST_PAGE_LIMIT), undefined);
|
|
assert.equal(result.skipped, undefined);
|
|
});
|
|
|
|
// D. Back-compat boundary — every existing caller passes no total, and
|
|
// lists shorter than the page cap cannot have been truncated.
|
|
test('a 99-file tooling list is trusted without a total (below the page cap)', () => {
|
|
const result = evaluatePrTemplate('no template here', 'NONE', toolingPaths(FILE_LIST_PAGE_LIMIT - 1), undefined);
|
|
assert.equal(result.skipped, 'tooling-paths');
|
|
});
|
|
});
|