diff --git a/.github/workflows/pr-template-format.yml b/.github/workflows/pr-template-format.yml index 99b8b7874..c006209b1 100644 --- a/.github/workflows/pr-template-format.yml +++ b/.github/workflows/pr-template-format.yml @@ -28,10 +28,20 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | - files=$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json files --jq '.files[].path' | tr '\n' '\n') - echo "files<> "$GITHUB_OUTPUT" + # The delimiter must be unguessable: every path in `files` is + # attacker-controlled on a fork PR, and a file named after a fixed + # delimiter (e.g. a file literally named `EOF`, the weakest + # possible choice here) would terminate the heredoc value early, + # truncating everything after it — this is GitHub's own documented + # guidance for untrusted multiline output + # (https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings). + # The length-vs-total check in the policy (fileListIsComplete) is + # the second, independent layer. + delim="GSD_EOF_$(openssl rand -hex 16)" + files=$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json files --jq '.files[].path') + echo "files<<$delim" >> "$GITHUB_OUTPUT" echo "$files" >> "$GITHUB_OUTPUT" - echo "EOF" >> "$GITHUB_OUTPUT" + echo "$delim" >> "$GITHUB_OUTPUT" - name: Evaluate PR template format id: policy @@ -39,6 +49,7 @@ jobs: PR_BODY: ${{ github.event.pull_request.body }} AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} CHANGED_FILES: ${{ steps.changed_files.outputs.files }} + CHANGED_FILES_TOTAL: ${{ github.event.pull_request.changed_files }} run: node scripts/pr-template-policy.cjs - name: Warn trusted contributor about missing template diff --git a/.github/workflows/require-issue-link.yml b/.github/workflows/require-issue-link.yml index 5f6d0ec1c..a23c3d12c 100644 --- a/.github/workflows/require-issue-link.yml +++ b/.github/workflows/require-issue-link.yml @@ -6,10 +6,33 @@ name: Require Issue Link # rejection kills the step, and the core.setFailed on the last line never runs — # the contributor sees an API stack trace instead of "add Closes #NNN". # pull_request_target runs in the base-repo context with a write-capable token. -# Safe here: this job performs NO checkout at all and reads the PR body only via -# an env var (never interpolated into a shell), so no head code executes. The -# #1389 fork-forgery carve-out below still holds — it is keyed on +# Safe here: the only checkout is the BASE branch (persist-credentials: false), +# and every PR-controlled input — body, head ref, changed-file list — is read as +# data via env vars, never interpolated into a shell. No head code executes. The +# #1389 fork-forgery carve-out still holds — it is keyed on # head.repo.full_name == github.repository, not on the branch name alone. +# +# #3211: the gate used to model exactly one PR→issue relationship ("this PR +# closes that issue"), so a test-only or docs-only FOLLOW-UP PR — which +# references an issue without closing it — had no way to pass. A fork could not +# use the backmerge carve-out either (it requires same-repo identity), so the +# only green path was a knowingly-inert `Closes #N` against an already-closed +# issue. The verdict now lives in scripts/require-issue-link-policy.cjs, which +# additionally accepts a non-closing reference (`Refs #N`, `Follow-up to #N`, …) +# but ONLY when every changed file is under tests/, under docs/, or a root-level +# *.md — the same doc-only shape pre-pr-gate.sh:111 already recognizes, minus +# CHANGELOG.md, which changeset/lint.cjs classes as user-facing. Ordinary +# source-touching PRs still require a closing keyword, so gate strength is +# unchanged. The exemption is keyed on the server-computed diff shape, which a +# fork author cannot forge — preserving the #1389 property. +# +# Trust boundary: the policy module is loaded from a BASE-branch checkout (the +# already-merged, reviewed copy on the PR's target), exactly as +# pr-title-validator.yml and pr-template-format.yml load theirs. A PR cannot +# edit the ruler that measures it. Until the module lands on the base branch it +# does not exist there — the introducing PR falls back to the legacy +# closing-keyword grep (see the bootstrap branch below) rather than skipping, +# so the gate is never silently open. on: pull_request_target: types: [opened, edited, reopened, synchronize] @@ -18,62 +41,158 @@ concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true -# Scope unchanged by #2331 — see the note in pr-title-validator.yml. `issues: -# write` alone already authorizes this workflow's issues.createComment call on a -# PR: its sticky comment has posted on same-repo PRs (#106, #164, #232, #259) on -# exactly this scope. The 403 was the fork token downgrade, not a missing scope, -# so no `pull-requests: write` is added — this job never calls a pulls.* API. +# `issues: write` alone already authorizes this workflow's issues.createComment +# call on a PR: its sticky comment has posted on same-repo PRs (#106, #164, +# #232, #259) on exactly this scope. The #2331 403 was the fork token +# downgrade, not a missing scope, so no `pull-requests: WRITE` is added — this +# job still calls no mutating pulls.* API. #3211 adds exactly two read scopes, +# each for one concrete need: `contents: read` for the base-branch checkout +# that supplies the policy module, and `pull-requests: read` for the +# `gh pr view --json files,changedFiles` call that supplies the diff shape. permissions: + contents: read issues: write + pull-requests: read jobs: check-issue-link: name: Issue link required runs-on: ubuntu-latest + timeout-minutes: 2 steps: - - name: Check PR body for issue reference - id: check + # BASE branch only — the trusted policy source. Never the PR head. + - name: Check out policy from base branch + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.pull_request.base.ref }} + persist-credentials: false + + # The authoritative diff shape. `gh pr view --json files` returns at most + # 100 paths and does NOT paginate, so `changedFiles` (the true total) is + # fetched alongside it: the policy compares the two and fails closed when + # they disagree. Without that check a >100-file fork PR could present a + # falsely tests-only list and forge the follow-up exemption (#3211). + - name: Get changed files + id: changed_files env: - # Bound to env var — never interpolated into shell directly - PR_BODY: ${{ github.event.pull_request.body }} + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} run: | - if echo "$PR_BODY" | grep -qiE '(closes|fixes|resolves)\s+#[0-9]+'; then - echo "found=true" >> "$GITHUB_OUTPUT" - else - echo "found=false" >> "$GITHUB_OUTPUT" + # The delimiter must be unguessable: every path in `files` is + # attacker-controlled on a fork PR, and a file named after a fixed + # delimiter (e.g. a file literally named `GSD_EOF`) would terminate + # the heredoc value early, truncating everything after it — this is + # GitHub's own documented guidance for untrusted multiline output + # (https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings). + # The length-vs-total check in the policy (fileListIsComplete) is + # the second, independent layer: even if a delimiter collision slid + # past this, a truncated or inflated list would still fail closed. + delim="GSD_EOF_$(openssl rand -hex 16)" + files=$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json files --jq '.files[].path') + { + echo "files<<$delim" + echo "$files" + echo "$delim" + } >> "$GITHUB_OUTPUT" + + - name: Evaluate issue link + id: policy + env: + # Every PR-controlled value is bound to an env var and read as data — + # never interpolated into the shell or into a JS template literal. + PR_BODY: ${{ github.event.pull_request.body }} + HEAD_REF: ${{ github.head_ref }} + SAME_REPO: ${{ github.event.pull_request.head.repo.full_name == github.repository }} + CHANGED_FILES: ${{ steps.changed_files.outputs.files }} + CHANGED_FILES_TOTAL: ${{ github.event.pull_request.changed_files }} + run: | + # Bootstrap: the policy module is loaded from the BASE checkout, so it + # is absent on the PR that first introduces it. Fall back to the legacy + # closing-keyword grep — enforcing, not skipping — so the gate is never + # silently open during the changeover. Mirrors the bootstrap guard in + # pr-title-validator.yml, which skips; this one keeps enforcing because + # the legacy behavior is a strict subset of the new policy. + if [ ! -f scripts/require-issue-link-policy.cjs ]; then + if echo "$PR_BODY" | grep -qiE '(closes|fixes|resolves)\s+#[0-9]+'; then + printf 'ok=true\nreason=ok_closing_keyword\n' >> "$GITHUB_OUTPUT" + else + printf 'ok=false\nreason=fail_no_issue_reference\n' >> "$GITHUB_OUTPUT" + fi + exit 0 + fi + + # Exit 0 = satisfied, 1 = not satisfied (both are verdicts, and the + # step must survive to let the comment step run). Anything else is a + # real error in the policy script and must fail the job loudly rather + # than be mistaken for "no issue link". + set +e + node scripts/require-issue-link-policy.cjs + status=$? + set -e + if [ "$status" -ne 0 ] && [ "$status" -ne 1 ]; then + echo "::error::require-issue-link-policy.cjs exited with status $status" >&2 + exit "$status" fi - name: Comment and fail if no issue link - # Exempt auto-backmerge PRs (chore/backmerge-main-to-next-*): they map to - # no issue and a `Closes #N` would pollute the released CHANGELOG. Keyed on - # the workflow-authored branch name AND same-repo identity so a fork PR - # cannot forge the exemption. Step-level (not job-level) so the required - # "Issue link required" check still reports SUCCESS rather than a - # branch-protection-blocking "skipped". See #1389. - if: >- - steps.check.outputs.found == 'false' && - !(startsWith(github.head_ref, 'chore/backmerge-main-to-next-') && - github.event.pull_request.head.repo.full_name == github.repository) + # Step-level (not job-level) so the required "Issue link required" check + # still reports SUCCESS rather than a branch-protection-blocking + # "skipped" when a PR is exempt. See #1389 — that placement is load + # bearing and every exemption added since must preserve it. + if: steps.policy.outputs.ok != 'true' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + REASON: ${{ steps.policy.outputs.reason }} with: # Uses GitHub API SDK — no shell string interpolation of untrusted input script: | const marker = ''; const repoUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}`; const prNumber = context.payload.pull_request.number; + // #3211: the verdict is a typed reason, so the guidance can name the + // rule that actually fired instead of always saying "no issue link". + // Read from an env var — never interpolated into this script body. + const reason = process.env.REASON || 'fail_no_issue_reference'; + const explanation = { + fail_reference_needs_closing: [ + 'This PR references an issue without a closing keyword, but its diff touches files outside `tests/`, `docs/`, and root-level `*.md`.', + '', + 'The non-closing reference form is accepted **only** for test-only or docs-only follow-up PRs. Because this PR changes other files, it needs a closing keyword:', + ], + fail_file_list_incomplete: [ + 'This PR references an issue without a closing keyword, but its changed-file list could not be confirmed as complete, so it cannot be verified as a test-only or docs-only follow-up.', + '', + 'This happens on very large PRs (more than 100 files). Use a closing keyword instead:', + ], + fail_no_issue_reference: [ + 'This PR does not reference an issue. **All PRs must link to an issue** in the PR body:', + ], + }[reason] || [ + 'This PR does not reference an issue. **All PRs must link to an issue** in the PR body:', + ]; const body = [ marker, '## Missing issue link', '', - 'This PR does not reference an issue. **All PRs must link to an open issue** using a closing keyword in the PR body:', + ...explanation, '', '```', 'Closes #123', '```', '', + 'Accepted closing keywords are `Closes #NNN`, `Fixes #NNN`, and `Resolves #NNN`.', + '', + '**Test-only or docs-only follow-up PR?** If every file your PR changes is under `tests/`, under `docs/`, or a root-level `*.md` (`README.md`, `CONTRIBUTING.md`, … but not `CHANGELOG.md`), and there is no open issue for it to close, reference the related issue without closing it:', + '', + '```', + 'Refs #123', + '```', + '', + '`Ref`, `Refs`, `References`, `Relates to`, `Related to`, and `Follow-up to` are all accepted in that position. See CONTRIBUTING.md → "Link with a closing keyword".', + '', `If no issue exists for this change, [open one first](${repoUrl}/issues/new/choose), then update this PR body with the reference.`, '', - 'Edit the PR description to add a valid `Closes #NNN`, `Fixes #NNN`, or `Resolves #NNN` line. This check will re-evaluate on the next PR update.', + 'This check will re-evaluate on the next PR update.', ].join('\n'); // #2331: the comment is a COURTESY, the setFailed below is the GATE. // A failure here must never suppress the verdict. pull_request_target diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2dd63216d..62bad9835 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -189,6 +189,9 @@ Contributor requirements (summary): - **No draft PRs** — draft PRs are automatically closed. Only open a PR when it is complete, tested, and ready for review. If your work is not finished, keep it on your local branch until it is. - **Use the correct PR template** — there are separate templates for [Fix](.github/PULL_REQUEST_TEMPLATE/fix.md), [Enhancement](.github/PULL_REQUEST_TEMPLATE/enhancement.md), and [Feature](.github/PULL_REQUEST_TEMPLATE/feature.md). Using the wrong template or using the default template for a feature is a rejection reason. - **Link with a closing keyword** — use `Closes #123`, `Fixes #123`, or `Resolves #123` in the PR body. The CI check will fail and the PR will be auto-closed if no valid issue reference is found. + - **Test-only and docs-only follow-up PRs may reference without closing.** If your PR is documentation or regression coverage only — say, a repo-wide guard for a fix that already shipped — and there is no open issue for it to close, use a non-closing reference instead: `Refs #123`. `Ref`, `Refs`, `References`, `Relates to`, `Related to`, and `Follow-up to` are all accepted in that position. Do **not** write a closing keyword against an already-closed issue to satisfy the check; on merge it closes nothing, and it trains readers to treat closing keywords as decorative. + - **Qualifying diff shape:** every changed file must be under `tests/`, under `docs/`, or a root-level `*.md` (`README.md`, `CONTRIBUTING.md`, …). This mirrors the doc-only classification the push gate already uses, and it is deliberately root-only — markdown under a subdirectory (`gsd-core/workflows/*.md`, `agents/*.md`, `commands/**/*.md`) is runtime-loaded text, not documentation, so it still requires a closing keyword. `CHANGELOG.md` is excluded too: edit it through a `.changeset/` fragment, never directly. + - This weaker form is accepted **only** for that diff shape. A PR touching anything else still needs a closing keyword, and a PR with no issue reference at all still fails. On a very large PR (more than 100 changed files) the check cannot confirm the diff shape and falls back to requiring a closing keyword. - **One concern per PR** — bug fixes, enhancements, and features must be separate PRs - **No drive-by formatting** — don't reformat code unrelated to your change - **Don't bundle test-fixture updates into `docs:` or unrelated commits** — when a production change makes an existing test assertion stale, the test correction MUST land as its own `test:` (or `fix:`) commit, not bundled into a `docs:` commit that also updates the explanation. The release-sdk hotfix cherry-pick filter routes by commit-subject prefix (`fix:`, `chore:`, `test:`); a test-fixture correction packed under a `docs:` prefix is invisible to the picker and ships a half-state to the hotfix branch — production code changed, test assertion stale. v1.42.3 hit this exact mode (#3621). The fix is upstream: keep the test-fixture commit separate. diff --git a/scripts/pr-changed-files.cjs b/scripts/pr-changed-files.cjs new file mode 100644 index 000000000..b38488818 --- /dev/null +++ b/scripts/pr-changed-files.cjs @@ -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 }; diff --git a/scripts/pr-template-policy.cjs b/scripts/pr-template-policy.cjs index 0e9222449..59579776a 100644 --- a/scripts/pr-template-policy.cjs +++ b/scripts/pr-template-policy.cjs @@ -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) { diff --git a/scripts/require-issue-link-policy.cjs b/scripts/require-issue-link-policy.cjs new file mode 100644 index 000000000..ca283aafb --- /dev/null +++ b/scripts/require-issue-link-policy.cjs @@ -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, +}; diff --git a/tests/pr-template-policy.test.cjs b/tests/pr-template-policy.test.cjs index 5e4ca450d..35d0039ed 100644 --- a/tests/pr-template-policy.test.cjs +++ b/tests/pr-template-policy.test.cjs @@ -2,6 +2,7 @@ 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', @@ -292,3 +293,41 @@ describe('pr-template-policy', () => { 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'); + }); +}); diff --git a/tests/require-issue-link-policy.property.test.cjs b/tests/require-issue-link-policy.property.test.cjs new file mode 100644 index 000000000..906d22b6d --- /dev/null +++ b/tests/require-issue-link-policy.property.test.cjs @@ -0,0 +1,126 @@ +'use strict'; + +/** + * Property-based tests for scripts/require-issue-link-policy.cjs (#3211). + * + * See CLAUDE.md "Property-Based Testing": parsers/budget-limit/bijective + * contracts require at least one fast-check property test. This module has + * three such contracts: the closing-keyword parse, the tests/docs-only + * carve-out, and the truncation-detection fail-closed guarantee. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fc = require('fast-check'); + +const { ISSUE_LINK_REASON, evaluateIssueLink } = require('../scripts/require-issue-link-policy.cjs'); + +describe('evaluateIssueLink — properties', () => { + test('P1: any body containing a closing keyword yields OK_CLOSING_KEYWORD', () => { + fc.assert( + fc.property( + fc.string(), + fc.string(), + fc.integer({ min: 1, max: 99999 }), + (pre, post, n) => { + // Guard: a leading/trailing newline stops `pre`/`post` from gluing + // onto the "Closes #" token and changing what CLOSING_KEYWORD_REGEX + // sees — the regex has no word-boundary anchors, so word characters + // immediately adjacent to "Closes" are irrelevant to it either way, + // but the newline keeps the constructed body unambiguous to read. + const body = `${pre}\nCloses #${n}\n${post}`; + const result = evaluateIssueLink({ + prBody: body, + headRef: 'fix/1-something', + sameRepo: false, + changedFiles: ['src/init.cts'], + changedFilesTotal: 1, + }); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_CLOSING_KEYWORD); + }, + ), + { numRuns: 200 }, + ); + }); + + test('P2: a non-exempt path in the diff never yields OK_FOLLOWUP_REFERENCE', () => { + fc.assert( + fc.property( + fc.array( + fc.oneof( + fc.constantFrom('tests/a.test.cjs', 'docs/guide.md'), + fc.string({ minLength: 1, maxLength: 20 }).map((s) => `src/${s}.cts`), + ), + { minLength: 1, maxLength: 8 }, + ), + (paths) => { + // At least one path must actually be non-exempt for the property + // to be meaningful; skip runs where fc happened to draw only + // tests/docs paths. The exempt set is tests/, docs/, and root-level + // markdown (see EXEMPT_PATH_PREFIXES / isRootLevelDoc), so this + // filter names the non-exempt generator (`src/*.cts`) directly + // rather than re-deriving the exempt predicate — re-deriving it + // here would silently go stale the next time the exempt set grows. + fc.pre(paths.some((p) => p.startsWith('src/'))); + const result = evaluateIssueLink({ + prBody: 'Refs #1', + headRef: 'fix/1-something', + sameRepo: false, + changedFiles: paths, + changedFilesTotal: paths.length, + }); + assert.notStrictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }, + ), + { numRuns: 200 }, + ); + }); + + test('P3: changedFiles at/above the page cap with a mismatched total never yields OK_FOLLOWUP_REFERENCE', () => { + fc.assert( + fc.property( + fc.integer({ min: 100, max: 150 }), + fc.integer({ min: 0, max: 300 }), + (length, total) => { + fc.pre(total !== length); + const changedFiles = Array.from({ length }, (_, i) => `tests/generated-${i}.test.cjs`); + const result = evaluateIssueLink({ + prBody: 'Refs #1', + headRef: 'fix/1-something', + sameRepo: false, + changedFiles, + changedFilesTotal: total, + }); + assert.notStrictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }, + ), + { numRuns: 200 }, + ); + }); + + test('P4: changedFiles below the page cap with a mismatched total never yields OK_FOLLOWUP_REFERENCE', () => { + // The sub-100 counterpart to P3, and the property the reviewed blocker + // violated: fileListIsComplete used to only consult the total once + // length >= FILE_LIST_PAGE_LIMIT, so any mismatch below the cap + // (heredoc truncation, newline inflation) went undetected below 100. + fc.assert( + fc.property( + fc.integer({ min: 1, max: 60 }), + fc.integer({ min: 0, max: 60 }), + (len, total) => { + fc.pre(total !== len); + const changedFiles = Array.from({ length: len }, (_, i) => `tests/generated-${i}.test.cjs`); + const result = evaluateIssueLink({ + prBody: 'Refs #1', + headRef: 'fix/1-something', + sameRepo: false, + changedFiles, + changedFilesTotal: total, + }); + assert.notStrictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }, + ), + { numRuns: 200 }, + ); + }); +}); diff --git a/tests/require-issue-link-policy.test.cjs b/tests/require-issue-link-policy.test.cjs new file mode 100644 index 000000000..ed9fb80cf --- /dev/null +++ b/tests/require-issue-link-policy.test.cjs @@ -0,0 +1,430 @@ +'use strict'; + +/** + * Tests for scripts/require-issue-link-policy.cjs (#3211, preserving #1389). + * + * All assertions are on the typed ISSUE_LINK_REASON enum, never on free + * text — see CLAUDE.md "Mutation Score" / test conventions. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const yaml = require('js-yaml'); + +const { + ISSUE_LINK_REASON, + hasClosingKeyword, + hasFollowUpReference, + allPathsAreTestsOrDocs, + evaluateIssueLink, + EXEMPT_PATH_PREFIXES, + EXCLUDED_ROOT_DOCS, +} = require('../scripts/require-issue-link-policy.cjs'); + +const { fileListIsComplete } = require('../scripts/pr-changed-files.cjs'); + +function forkPr(overrides = {}) { + return { + prBody: '', + headRef: 'fix/123-something', + sameRepo: false, + changedFiles: ['src/init.cts'], + changedFilesTotal: 1, + ...overrides, + }; +} + +function testPaths(n) { + return Array.from({ length: n }, (_, i) => `tests/generated-${i}.test.cjs`); +} + +describe('evaluateIssueLink', () => { + // 1. Real-world vector: a fork PR that adds regression coverage for an + // issue without closing it. + test('fork PR referencing an issue with a tests-only diff is OK_FOLLOWUP_REFERENCE', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269 — regression coverage.', + changedFiles: ['tests/commit-files-pathspec.test.cjs'], + changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + assert.strictEqual(result.ok, true); + }); + + // 2. Every accepted reference form, including a lowercase variant. + test('every accepted follow-up reference form passes with a tests-only diff', () => { + const forms = [ + 'Refs #1', 'Ref #1', 'References #1', 'Relates to #1', + 'Related to #1', 'Follow-up to #1', 'Follow up to #1', 'refs #1', + ]; + for (const body of forms) { + const result = evaluateIssueLink(forkPr({ prBody: body, changedFiles: ['tests/a.test.cjs'], changedFilesTotal: 1 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE, `form: ${body}`); + } + }); + + // 3. Docs-only and mixed tests+docs diffs are both allowed. + test('docs-only and mixed tests+docs diffs pass', () => { + const docsOnly = evaluateIssueLink(forkPr({ + prBody: 'Refs #1', changedFiles: ['docs/CONFIGURATION.md'], changedFilesTotal: 1, + })); + assert.strictEqual(docsOnly.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + + const mixed = evaluateIssueLink(forkPr({ + prBody: 'Refs #1', changedFiles: ['tests/a.test.cjs', 'docs/guide.md'], changedFilesTotal: 2, + })); + assert.strictEqual(mixed.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }); + + // 4. Windows-style backslash path is normalized before the prefix check. + test('backslash path is normalized and recognized as tests/', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #1', changedFiles: ['tests\\windows\\a.test.cjs'], changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }); + + // 5. CRLF vs LF bodies must produce the same reason. + test('CRLF and LF bodies with the same reference produce the same reason', () => { + const lf = evaluateIssueLink(forkPr({ prBody: 'Refs #1\n\nMore prose.', changedFiles: ['tests/a.test.cjs'], changedFilesTotal: 1 })); + const crlf = evaluateIssueLink(forkPr({ prBody: 'Refs #1\r\n\r\nMore prose.', changedFiles: ['tests/a.test.cjs'], changedFilesTotal: 1 })); + assert.strictEqual(lf.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + assert.strictEqual(crlf.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }); + + // 6. No reference at all, even with a docs-only diff, fails. + test('no reference at all fails even with a docs-only diff', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Just a description, no issue mentioned.', changedFiles: ['docs/guide.md'], changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_NO_ISSUE_REFERENCE); + }); + + // 7 & 8. A reference (not a closing keyword) touching source files needs + // an actual closing keyword instead. + test('reference-only PR touching a source file fails needs-closing', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', changedFiles: ['src/init.cts'], changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING); + }); + + test('reference-only PR with a mixed tests+source diff fails needs-closing', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', + changedFiles: ['tests/a.test.cjs', 'tests/b.test.cjs', 'src/b.cts'], + changedFilesTotal: 3, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING); + }); + + // 9. Bodies that must NOT be recognized as any kind of issue reference. + test('non-reference bodies fail with FAIL_NO_ISSUE_REFERENCE', () => { + const bodies = ['see #123', '#123', 'unlike #123', 'issue 123', 'a #123 b']; + for (const body of bodies) { + const result = evaluateIssueLink(forkPr({ prBody: body, changedFiles: ['tests/a.test.cjs'], changedFilesTotal: 1 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_NO_ISSUE_REFERENCE, `body: ${JSON.stringify(body)}`); + } + }); + + // 10. Lookalike words that embed "ref"/"reference" inside a longer word + // must not be treated as a reference. + test('lookalike embedded-word bodies fail with FAIL_NO_ISSUE_REFERENCE', () => { + const bodies = ['prefs #1', 'unreferenced #1', 'xref#1', 'preferences #1']; + for (const body of bodies) { + const result = evaluateIssueLink(forkPr({ prBody: body, changedFiles: ['tests/a.test.cjs'], changedFilesTotal: 1 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_NO_ISSUE_REFERENCE, `body: ${JSON.stringify(body)}`); + } + }); + + // 11. Directory-lookalike paths (tests-e2e/, src/tests/, testsuite/, + // docsite/) must NOT be treated as tests/ or docs/. + test('directory-lookalike paths are rejected, forcing needs-closing', () => { + const paths = ['tests-e2e/src/a.ts', 'src/tests/x.ts', 'testsuite/y.cjs', 'docsite/z.md']; + for (const p of paths) { + const result = evaluateIssueLink(forkPr({ prBody: 'Refs #1', changedFiles: [p], changedFilesTotal: 1 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING, `path: ${p}`); + } + }); + + // 12. A closing keyword on a source diff passes outright. + test('Closes #123 with a source diff is OK_CLOSING_KEYWORD', () => { + const result = evaluateIssueLink(forkPr({ prBody: 'Closes #123', changedFiles: ['src/init.cts'], changedFilesTotal: 1 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_CLOSING_KEYWORD); + }); + + // 13. Closing keyword case/whitespace variants. + test('closing keyword variants (case, whitespace) all pass', () => { + const bodies = ['closes #1', 'FIXES #1', 'Resolves #1', 'resolves\t#1']; + for (const body of bodies) { + const result = evaluateIssueLink(forkPr({ prBody: body, changedFiles: ['src/init.cts'], changedFilesTotal: 1 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_CLOSING_KEYWORD, `body: ${JSON.stringify(body)}`); + } + }); + + // 14 & 15. #1389 anti-forgery property: the backmerge exemption requires + // BOTH the branch prefix AND sameRepo === true. + test('backmerge branch + sameRepo true is exempt with no reference at all', () => { + const result = evaluateIssueLink(forkPr({ + prBody: '', headRef: 'chore/backmerge-main-to-next-20260101', sameRepo: true, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_BACKMERGE_EXEMPT); + }); + + test('#1389 anti-forgery: backmerge branch name from a FORK (sameRepo false) is NOT exempt', () => { + const result = evaluateIssueLink(forkPr({ + prBody: '', headRef: 'chore/backmerge-main-to-next-20260101', sameRepo: false, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_NO_ISSUE_REFERENCE); + }); + + // 16. hasClosingKeyword corpus parity — expected values come from the + // shipped shell grep this regex replaces: + // grep -qiE '(closes|fixes|resolves)\s+#[0-9]+' + test('hasClosingKeyword corpus parity with the replaced shell grep', () => { + const cases = [ + ['Closes #2269', true], + ['closes #1', true], + ['Fixes #12', true], + ['Resolves #3', true], + ['fixes #4', true], + ['Closes #123 and more prose', true], + ['Refs #2269', false], + ['Follow-up to #2269', false], + ['no reference at all', false], + ['Closes #', false], + ['Closes123', false], + ['closes issue 5', false], + ]; + for (const [body, expected] of cases) { + assert.strictEqual(hasClosingKeyword(body), expected, `body: ${JSON.stringify(body)}`); + } + }); + + // 17-18. BOUNDARY: exactly at and just below the page cap, with a total + // that matches, must pass. + test('BOUNDARY 99 tests-only paths, total 99 passes', () => { + const paths = testPaths(99); + const result = evaluateIssueLink(forkPr({ prBody: 'Refs #1', changedFiles: paths, changedFilesTotal: 99 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }); + + test('BOUNDARY 100 tests-only paths, total 100 passes', () => { + const paths = testPaths(100); + const result = evaluateIssueLink(forkPr({ prBody: 'Refs #1', changedFiles: paths, changedFilesTotal: 100 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }); + + // 19. Real vector: PR #3202 — gh returns 100 of 118 changed files. + test('BOUNDARY 100 tests-only paths, total 118 fails FAIL_FILE_LIST_INCOMPLETE (PR #3202 vector)', () => { + const paths = testPaths(100); + const result = evaluateIssueLink(forkPr({ prBody: 'Refs #1', changedFiles: paths, changedFilesTotal: 118 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_FILE_LIST_INCOMPLETE); + }); + + test('100 tests-only paths, total undefined fails FAIL_FILE_LIST_INCOMPLETE', () => { + const paths = testPaths(100); + const result = evaluateIssueLink(forkPr({ prBody: 'Refs #1', changedFiles: paths, changedFilesTotal: undefined })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_FILE_LIST_INCOMPLETE); + }); + + // Reviewed BLOCKER: the old fileListIsComplete only consulted the total + // when length >= FILE_LIST_PAGE_LIMIT, so ANY mechanism that shortens the + // list below 100 went undetected. A $GITHUB_OUTPUT heredoc terminated + // early by a file named after the delimiter is exactly such a mechanism — + // it truncates the list well below the page cap, with the true total + // still available from the separate, non-paginated `changedFiles` field. + test('a list shorter than its authoritative total fails closed', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #1', changedFiles: ['CONTRIBUTING.md'], changedFilesTotal: 3, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_FILE_LIST_INCOMPLETE); + }); + + // A path containing a literal newline can inflate the parsed list past the + // true total (e.g. a filename that itself looks like another path once + // split on newlines) — the total is the authority in both directions. + test('a list longer than its authoritative total fails closed', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #1', + changedFiles: ['tests/a.test.cjs', 'tests/b.test.cjs', 'tests/c.test.cjs'], + changedFilesTotal: 2, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_FILE_LIST_INCOMPLETE); + }); + + // 21. An empty changedFiles list cannot confirm anything. + test('empty changedFiles fails FAIL_FILE_LIST_INCOMPLETE', () => { + const result = evaluateIssueLink(forkPr({ prBody: 'Refs #1', changedFiles: [], changedFilesTotal: 0 })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_FILE_LIST_INCOMPLETE); + }); +}); + +describe('fileListIsComplete', () => { + // 22. Direct table of (changedFiles, changedFilesTotal) -> expected. + test('boundary table', () => { + const cases = [ + [['a', 'b', 'c'], 3, true], + [['a', 'b', 'c'], undefined, true], + [testPaths(100), 100, true], + [testPaths(100), 101, false], + [testPaths(100), undefined, false], + [[], 0, false], + [undefined, 0, false], + [testPaths(3), 5, false], + [testPaths(5), 3, false], + [testPaths(3), 3, true], + [testPaths(3), undefined, true], + ]; + for (const [changedFiles, changedFilesTotal, expected] of cases) { + assert.strictEqual( + fileListIsComplete(changedFiles, changedFilesTotal), + expected, + `changedFiles.length=${Array.isArray(changedFiles) ? changedFiles.length : changedFiles}, total=${changedFilesTotal}`, + ); + } + }); +}); + +describe('hasFollowUpReference', () => { + // 23. Direct spot checks on the raw predicate. + test('rejects a closing keyword, accepts a reference', () => { + assert.strictEqual(hasFollowUpReference('Closes #1'), false); + assert.strictEqual(hasFollowUpReference('Refs #1'), true); + }); +}); + +describe('allPathsAreTestsOrDocs', () => { + // 24. Direct spot checks on the raw predicate. + test('true for tests/docs mix, false for a non-exempt path, false for empty', () => { + assert.strictEqual(allPathsAreTestsOrDocs(['tests/a.cjs', 'docs/b.md']), true); + assert.strictEqual(allPathsAreTestsOrDocs(['tests/a.cjs', '.github/workflows/x.yml']), false); + assert.strictEqual(allPathsAreTestsOrDocs([]), false); + }); + + // 25. Root-level markdown is a third accepted shape; CHANGELOG.md is + // excluded from it even though it is root-level markdown. + test('root-level markdown is accepted, CHANGELOG.md and subdirectory markdown are not', () => { + assert.strictEqual(allPathsAreTestsOrDocs(['CONTRIBUTING.md']), true); + assert.strictEqual(allPathsAreTestsOrDocs(['CHANGELOG.md']), false); + assert.strictEqual(allPathsAreTestsOrDocs(['agents/x.md']), false); + assert.strictEqual(allPathsAreTestsOrDocs(['package.json']), false); + }); + + // The exclusion of CHANGELOG.md must not be defeatable by casing — the + // extension test (`/\.md$/i`) is already case-insensitive, so the + // exclusion lookup must match it rather than silently letting a + // differently-cased CHANGELOG.md ride in on the docs carve-out. + test('CHANGELOG.md exclusion is case-insensitive', () => { + assert.strictEqual(allPathsAreTestsOrDocs(['changelog.md']), false); + assert.strictEqual(allPathsAreTestsOrDocs(['CHANGELOG.MD']), false); + }); +}); + +describe('require-issue-link policy — root-level documentation (#2290 shape)', () => { + // #2290 is the motivating PR the follow-up-reference exemption failed to + // cover: its diff is CONTRIBUTING.md (root-level markdown) plus a tests/ + // file, and the old EXEMPT_PATH_PREFIXES-only predicate rejected it because + // CONTRIBUTING.md is neither tests/ nor docs/-prefixed. + test('the motivating PR #2290 shape qualifies', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', + changedFiles: ['CONTRIBUTING.md', 'tests/commit-files-pathspec.test.cjs'], + changedFilesTotal: 2, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }); + + test('a root-level README change qualifies', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', changedFiles: ['README.md'], changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.OK_FOLLOWUP_REFERENCE); + }); + + // scripts/changeset/lint.cjs classes a direct CHANGELOG.md edit as + // user-facing specifically to close a bypass — the docs carve-out here + // must not undo that by treating CHANGELOG.md as ordinary documentation. + test('a direct CHANGELOG.md edit does NOT qualify', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', changedFiles: ['CHANGELOG.md'], changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING); + }); + + test('CHANGELOG.md alongside real docs still disqualifies', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', changedFiles: ['docs/a.md', 'CHANGELOG.md'], changedFilesTotal: 2, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING); + }); + + // These are runtime-loaded text, deliberately gated by the same root-only + // anchor pre-pr-gate.sh uses — a subdirectory .md file is not root-level. + test('subdirectory markdown is not root-level documentation', () => { + const paths = ['gsd-core/workflows/next.md', 'agents/reviewer.md', 'commands/gsd/plan.md', 'src/notes.md']; + for (const p of paths) { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', changedFiles: [p], changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING, `path: ${p}`); + } + }); + + test('a root-level non-markdown file does not qualify', () => { + const result = evaluateIssueLink(forkPr({ + prBody: 'Refs #2269', changedFiles: ['package.json'], changedFilesTotal: 1, + })); + assert.strictEqual(result.reason, ISSUE_LINK_REASON.FAIL_REFERENCE_NEEDS_CLOSING); + }); +}); + +describe('require-issue-link policy — the workflow guidance matches the rule', () => { + // Parity assertion (CLAUDE.md "Generative Fix Divergence"): the sticky + // comment's guidance text is generated separately from the predicate it + // describes, and the two have already drifted once (the predicate widened + // to accept root-level markdown while the guidance kept saying "nothing + // outside tests/ and docs/"). Every expectation below is derived from the + // module's actual exports, never hardcoded, so a future widening of the + // predicate without a guidance update fails this test. + const workflowPath = path.join(__dirname, '..', '.github', 'workflows', 'require-issue-link.yml'); + const workflowDoc = yaml.load(fs.readFileSync(workflowPath, 'utf8')); + const job = workflowDoc.jobs['check-issue-link']; + const steps = job.steps; + const lastStep = steps[steps.length - 1]; + const script = lastStep.with.script; + + // Non-vacuous guards: if these fail, the assertions below would otherwise + // silently pass against zero-length input. + test('the resolved script text and export lists are non-empty (guard)', () => { + assert.strictEqual(typeof script, 'string'); + assert.ok(script.length > 200, `expected script.length > 200, got ${script.length}`); + assert.ok(EXEMPT_PATH_PREFIXES.length > 0, 'EXEMPT_PATH_PREFIXES must be non-empty'); + assert.ok(EXCLUDED_ROOT_DOCS instanceof Set, 'EXCLUDED_ROOT_DOCS must be a Set'); + assert.ok(EXCLUDED_ROOT_DOCS.size > 0, 'EXCLUDED_ROOT_DOCS must be non-empty'); + }); + + test('guidance names every EXEMPT_PATH_PREFIXES entry verbatim', () => { + for (const prefix of EXEMPT_PATH_PREFIXES) { + assert.ok(script.includes(prefix), `guidance script missing prefix: ${prefix}`); + } + }); + + // isRootLevelDoc accepts root-level *.md — the guidance must say so; this + // is the exact shape that drifted before. + test('guidance mentions root-level markdown', () => { + assert.ok(script.includes('root-level'), 'guidance script missing "root-level"'); + }); + + test('guidance names every EXCLUDED_ROOT_DOCS entry verbatim', () => { + for (const doc of EXCLUDED_ROOT_DOCS) { + assert.ok(script.includes(doc), `guidance script missing excluded doc: ${doc}`); + } + }); + + test('guidance mentions an accepted non-closing reference marker', () => { + assert.ok(script.includes('Refs #'), 'guidance script missing "Refs #"'); + }); +}); diff --git a/tests/workflow-maintainer-skip.test.cjs b/tests/workflow-maintainer-skip.test.cjs index c75f0eafd..1af1c5138 100644 --- a/tests/workflow-maintainer-skip.test.cjs +++ b/tests/workflow-maintainer-skip.test.cjs @@ -7,6 +7,9 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const yaml = require('js-yaml'); + +const { evaluateIssueLink, ISSUE_LINK_REASON } = require('../scripts/require-issue-link-policy.cjs'); const MAINTAINER_SKIP_EXPR = 'contains(fromJSON(\'["OWNER","MEMBER","COLLABORATOR"]\'), github.event.pull_request.author_association) == false'; @@ -261,26 +264,65 @@ describe('PR policy workflow maintainer carve-outs', () => { }); describe('Require Issue Link back-merge automation carve-out', () => { - test('the fail step is skipped for same-repo auto-backmerge PRs', () => { + // #3211 moved the whole verdict — including the #1389 backmerge carve-out — + // out of the YAML `if:` and into scripts/require-issue-link-policy.cjs + // (evaluateIssueLink), renaming the step from `check`/`found` to + // `policy`/`ok`. The old assertions here measured the carve-out's PREVIOUS + // location (raw `startsWith(github.head_ref, ...)` / `head.repo.full_name` + // text inside the workflow's `if:`) and went stale once that logic + // relocated — the property they guarded still holds, just not at that + // text. This test locks the property at its new home: (a)+(b) assert the + // workflow now delegates to the policy module at the correct step-level + // `if:`, (c) asserts the carve-out itself BEHAVIORALLY against the real + // module, and (d) keeps the bootstrap fallback's legacy grep locked so the + // introducing-PR path (before the module exists on the base branch) is + // never silently open. + test('the backmerge carve-out survives the move into the policy module', () => { const workflow = readWorkflow('.github/workflows/require-issue-link.yml'); - // Auto-backmerge PRs (chore/backmerge-main-to-next-*) map to no issue, and a - // `Closes #N` would pollute the released CHANGELOG. The fail step must carve - // them out — keyed on the workflow-authored branch name AND same-repo - // identity so a fork PR cannot forge the exemption (#1389). - assert.match( - workflow, - /startsWith\(github\.head_ref, 'chore\/backmerge-main-to-next-'\)/ - ); - assert.match( - workflow, - /github\.event\.pull_request\.head\.repo\.full_name == github\.repository/ + // (a) The failing step's `if:` is keyed on the policy output and is + // STEP-level, not job-level. A job-level `if:` would make the required + // "Issue link required" check report `skipped` instead of `success` on + // an exempt PR, which blocks branch protection (#1389). + assert.match(workflow, /if: steps\.policy\.outputs\.ok != 'true'/); + + const doc = yaml.load(workflow); + assert.equal( + doc.jobs['check-issue-link'].if, + undefined, + 'job-level `if:` would report "skipped" on an exempt PR and block branch protection (#1389)' ); - // The carve-out must live on the failing step's `if:` alongside the - // found=='false' check (step-level, so the required check still reports - // SUCCESS rather than a branch-protection-blocking "skipped"). - assert.match(workflow, /steps\.check\.outputs\.found == 'false'/); + // (b) The workflow delegates the verdict to the policy module. + assert.match(workflow, /node scripts\/require-issue-link-policy\.cjs/); + + // (c) The carve-out itself still holds — asserted BEHAVIORALLY against + // the real module rather than by grepping YAML, since the verdict no + // longer lives in the YAML text at all. + const backmergeArgs = { + prBody: 'Automated backmerge.', + headRef: 'chore/backmerge-main-to-next-20260101', + changedFiles: ['src/a.cts'], + changedFilesTotal: 1, + }; + assert.equal( + evaluateIssueLink({ ...backmergeArgs, sameRepo: true }).reason, + ISSUE_LINK_REASON.OK_BACKMERGE_EXEMPT + ); + // #1389 anti-forgery: a fork must not be able to forge the exemption by + // branch name alone — dropping the `sameRepo` conjunct would let a fork + // PR name its branch `chore/backmerge-main-to-next-*` and bypass the + // issue-link requirement entirely. + assert.equal( + evaluateIssueLink({ ...backmergeArgs, sameRepo: false }).reason, + ISSUE_LINK_REASON.FAIL_NO_ISSUE_REFERENCE + ); + + // (d) The bootstrap fallback (used only on the PR that first introduces + // the policy module, before it exists on the base branch) still carries + // the legacy closing-keyword grep, so the gate is never silently open + // during the changeover. + assert.match(workflow, /\(closes\|fixes\|resolves\)/); }); });