From 84ce37d7fc6b30761f4337176c14252a48a32063 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Tue, 10 Nov 2026 14:40:17 -0500 Subject: [PATCH] ci: name the extra text in a CLEAN finding and add a NO REVIEWABLE FILES reply The PR body check reported "`### Findings` is neither `CLEAN` nor numbered findings" for a section that read `CLEAN` followed by a sentence explaining the review had no files to read, so the author could not tell what to fix. It now names the text after the token. The review replies `NO REVIEWABLE FILES` for a diff it reads none of, so `CLEAN` always means code was reviewed. The checking logic moves to `.github/scripts/pr-body.ts`, tested in the `github` project, and `scripts/check-pr-body.ts` becomes the CLI the workflow runs. Co-Authored-By: Claude Opus 5.5 --- .agents/docs/architecture/testing-strategy.md | 5 +- .agents/skills/adversarial-review/SKILL.md | 3 +- .agents/skills/opening-pr/SKILL.md | 6 +- .github/scripts/__tests__/pr-body.test.ts | 266 ++++++++++++++++++ .github/scripts/pr-body.ts | 248 ++++++++++++++++ scripts/check-pr-body.ts | 240 +--------------- tsconfig.json | 2 + vitest.config.ts | 2 +- 8 files changed, 531 insertions(+), 241 deletions(-) create mode 100644 .github/scripts/__tests__/pr-body.test.ts create mode 100644 .github/scripts/pr-body.ts diff --git a/.agents/docs/architecture/testing-strategy.md b/.agents/docs/architecture/testing-strategy.md index 0d7ae8bbc..7e9ed657d 100644 --- a/.agents/docs/architecture/testing-strategy.md +++ b/.agents/docs/architecture/testing-strategy.md @@ -40,7 +40,7 @@ opts into a DOM with a first-line `// @vitest-environment happy-dom` docblock. | Project | Package | Notes | | ------------------------ | --------------------------------- | -------------------------------------------------------- | -| `github` | `.github` | not a workspace; tests `scripts/release.cjs` | +| `github` | `.github` | not a workspace; tests every file in `scripts/` | | `api` | `apps/api` | libnest SWC plugin, confined to `apps/api` sources | | `gateway` | `apps/gateway` | | | `playground` | `apps/playground` | | @@ -112,7 +112,8 @@ on `["^build", "db:generate"]`. `test:e2e` is `cache: false`. **`test` is not a ## Coverage scope The 100% gate (root `AGENTS.md`) measures `coverage.include` in root `vitest.config.ts`: -`apps/{api,gateway,web}/src` and `packages/*/src`. `runtime/**` and `apps/playground` are outside it. +`.github/scripts`, `apps/{api,gateway,web}/src` and `packages/*/src`. `runtime/**` and `apps/playground` +are outside it. `apps/api` confines SWC to its own files because letting it transform imported workspace packages counts their functions twice in the merged report. diff --git a/.agents/skills/adversarial-review/SKILL.md b/.agents/skills/adversarial-review/SKILL.md index 4f21a1c85..d1fe64bd7 100644 --- a/.agents/skills/adversarial-review/SKILL.md +++ b/.agents/skills/adversarial-review/SKILL.md @@ -21,7 +21,8 @@ git ls-files --others --exclude-standard what upstream has changed underneath. A clone without the `upstream` remote adds it first: `git remote add upstream https://github.com/DouglasNeuroInformatics/OpenDataCapture.git`. Keep only `.ts`, `.tsx`, `.js`, `.jsx`, `.css` and `.prisma` files, and drop `apps/web/src/route-tree.ts`, -`testing/src/generated/` and `*.stories.*`. An empty list ends the review: reply `CLEAN`. +`testing/src/generated/` and `*.stories.*`. An empty list ends the review: reply `NO REVIEWABLE FILES`, which +tells the reader nothing was reviewed, where `CLEAN` would claim nothing was found. ## 2. Prosecute diff --git a/.agents/skills/opening-pr/SKILL.md b/.agents/skills/opening-pr/SKILL.md index 12ef9d1f8..01c5a05cd 100644 --- a/.agents/skills/opening-pr/SKILL.md +++ b/.agents/skills/opening-pr/SKILL.md @@ -52,7 +52,7 @@ Reviewed at commit . pnpm check: green. ### Findings - + ### Fixed @@ -74,8 +74,8 @@ named, with every finding it returned. - **Changes lists every behaviour a reviewer must know, however many.** A migration, an environment variable or a config change is always one. Effect terms: what now happens, not which file or function changed; the diff shows files. Length scales with the diff, and nothing is there for length. -- **Decisions, Risks, `Filed:` and Notes are omitted when empty.** With `CLEAN`, so are Fixed and Declined; - the attestation never is. +- **Decisions, Risks, `Filed:` and Notes are omitted when empty.** With `CLEAN` or `NO REVIEWABLE FILES`, so + are Fixed and Declined; the attestation never is. - **Notes holds critical context only**: what a reviewer needs to judge whether the change works, such as a behaviour you verified that `pnpm check` does not prove, and what it showed. Nothing pending and no open question belongs there. It sits before the review block; the workflow refuses a Notes heading anywhere diff --git a/.github/scripts/__tests__/pr-body.test.ts b/.github/scripts/__tests__/pr-body.test.ts new file mode 100644 index 000000000..a26ed6568 --- /dev/null +++ b/.github/scripts/__tests__/pr-body.test.ts @@ -0,0 +1,266 @@ +import { describe, expect, it } from 'vitest'; + +import { checkPullRequest } from '../pr-body.ts'; + +const HEAD = 'a'.repeat(40); + +const OTHER = 'b'.repeat(40); + +const FOOTER = '🤖 Generated with [Claude Code](https://claude.com/claude-code)'; + +const ATTESTATION = 'I, Claude, attest that the review block above reports the adversarial review as it ran.'; + +type ReviewParts = { + attestation?: string; + findings?: string; + stamp?: string; + tail?: string; +}; + +function review({ + attestation = ATTESTATION, + findings = 'CLEAN', + stamp = `Reviewed at commit ${HEAD}. pnpm check: green.`, + tail = '' +}: ReviewParts = {}): string { + return [ + 'Users get a thing.', + '', + '## Notes', + '', + 'Context.', + '', + '## Adversarial review', + '', + stamp, + '', + '### Findings', + '', + findings, + tail, + '', + '### Attestation', + '', + attestation, + '', + FOOTER + ].join('\n'); +} + +type EventParts = { + action?: string; + body?: null | string; + ref?: string; + user?: { login: string; type: string }; +}; + +function check( + { action = 'opened', body = review(), ref = 'feature', user = { login: 'someone', type: 'User' } }: EventParts = {}, + commitMessages = '' +) { + const eventJson = JSON.stringify({ action, pull_request: { body, head: { ref, sha: HEAD }, user } }); + return checkPullRequest(() => ({ commitMessages, eventJson })); +} + +function failuresOf(parts: EventParts, commitMessages?: string): string { + const { passed, report } = check(parts, commitMessages); + expect(passed).toBe(false); + return report; +} + +describe('checkPullRequest', () => { + describe('authorship', () => { + it('should pass a body with no agent trace unchecked, so the template binds agents only', () => { + expect(check({ body: 'A person wrote this.' })).toEqual({ + passed: true, + report: 'The pull request shows no agent authorship, so the template does not apply.\n' + }); + }); + + it('should pass a merge-back to a release branch unchecked, since its commits already passed', () => { + expect(check({ body: FOOTER, ref: 'release/2' }).passed).toBe(true); + }); + + it('should pass Dependabot unchecked, since it is the one exempt bot', () => { + expect(check({ body: null, user: { login: 'dependabot[bot]', type: 'Bot' } }).passed).toBe(true); + }); + + it('should check a bot author whatever its body says', () => { + expect(failuresOf({ body: null, user: { login: 'renovate[bot]', type: 'Bot' } })).toContain('The body is empty.'); + }); + + it('should check a body whose author committed with an agent trailer', () => { + expect(failuresOf({ body: 'Plain.' }, 'fix: x\n\nCo-Authored-By: Claude ')).toContain( + 'The body has no `## Adversarial review` section.' + ); + }); + + it.each([ + ['a trailer', 'Co-Authored-By: Codex '], + ['the footer', FOOTER], + ['an attestation', ATTESTATION], + ['a review heading', '## Adversarial review'] + ])('should check a body that carries %s', (_, trace) => { + expect(check({ body: `Plain.\n\n${trace}` }).passed).toBe(false); + }); + + it('should ignore a trace inside a code fence, so quoting the template does not bind a person', () => { + expect(check({ body: `Plain.\n\n\`\`\`\n${FOOTER}\n\`\`\`` }).passed).toBe(true); + }); + }); + + describe('body', () => { + it('should pass a body that meets the template', () => { + expect(check()).toEqual({ passed: true, report: 'The pull request body meets the template.\n' }); + }); + + it('should refuse a Verification section, since pnpm check is the only verification statement', () => { + expect(failuresOf({ body: `## Verification\n\nRan it.\n\n${review()}` })).toContain( + 'Remove the `## Verification` section' + ); + }); + + it('should refuse a Notes section after the review block', () => { + expect(failuresOf({ body: `${review()}\n\n## Notes\n\nLate.` })).toContain( + 'A Notes section goes before `## Adversarial review`' + ); + }); + + it('should refuse a review block that does not state pnpm check is green', () => { + expect(failuresOf({ body: review({ stamp: `Reviewed at commit ${HEAD}.` }) })).toContain( + 'does not state `pnpm check: green`' + ); + }); + + it('should refuse a review block without an attestation', () => { + expect(failuresOf({ body: review({ attestation: 'Trust me.' }) })).toContain('has no `### Attestation`'); + }); + + it('should refuse a review block without a Findings section', () => { + expect(failuresOf({ body: `${FOOTER}\n\n## Adversarial review\n\npnpm check: green.` })).toContain( + 'has no `### Findings` section' + ); + }); + }); + + describe('attestation', () => { + const cannotAttest = 'I, Claude, cannot attest: the review did not run.'; + + it('should pass a review that cannot attest without a stamp, so the reviewer decides', () => { + expect( + check({ body: review({ attestation: cannotAttest, findings: 'Skipped.', stamp: 'pnpm check: green.' }) }).passed + ).toBe(true); + }); + + it('should refuse a review that cannot attest and says nothing under Findings', () => { + expect(failuresOf({ body: review({ attestation: cannotAttest, findings: '' }) })).toContain( + 'says what happened under `### Findings`' + ); + }); + }); + + describe('stamp', () => { + it('should refuse a review block that names no commit', () => { + expect(failuresOf({ body: review({ stamp: 'pnpm check: green.' }) })).toContain('names no reviewed commit'); + }); + + it('should refuse a stamp on a push that does not name the head', () => { + expect( + failuresOf({ + action: 'synchronize', + body: review({ stamp: `Reviewed at commit ${OTHER}. pnpm check: green.` }) + }) + ).toContain(`names no commit matching the head \`${HEAD}\``); + }); + + it('should pass a stale stamp on an edit, since the body is edited before the push it names', () => { + expect( + check({ action: 'edited', body: review({ stamp: `Reviewed at commit ${OTHER}. pnpm check: green.` }) }).passed + ).toBe(true); + }); + + it('should pass a follow-up line that names the head', () => { + const stamp = `Reviewed at commit ${OTHER}. pnpm check: green.\nFollow-up ${HEAD}: addresses findings 1; no new code.`; + expect(check({ body: review({ stamp }) }).passed).toBe(true); + }); + }); + + describe('findings', () => { + it('should pass NO REVIEWABLE FILES, so a diff the review reads none of is reported as such', () => { + expect(check({ body: review({ findings: 'NO REVIEWABLE FILES' }) }).passed).toBe(true); + }); + + it('should refuse a CLEAN review that lists a fixed finding', () => { + expect(failuresOf({ body: review({ tail: '\n### Fixed\n\n- 1. Done.' }) })).toContain( + 'A `CLEAN` review lists nothing under Fixed or Declined.' + ); + }); + + it('should name the extra text after CLEAN, so the author knows the token itself is right', () => { + expect(failuresOf({ body: review({ findings: 'CLEAN\n\nThe review ended at file selection.' }) })).toContain( + "`### Findings` holds text besides `CLEAN`; it holds the review's reply alone, and context goes in Notes." + ); + }); + + it('should refuse Findings that are neither a reply token nor numbered', () => { + expect(failuresOf({ body: review({ findings: 'Looks fine.' }) })).toContain( + '`### Findings` is not `CLEAN`, `NO REVIEWABLE FILES` or numbered findings.' + ); + }); + + it('should pass numbered findings that are each fixed or declined', () => { + const tail = '\n### Fixed\n\n- 1. Guarded.\n\n### Declined\n\n- 2. Upstream guard.'; + expect(check({ body: review({ findings: '1. a.ts:1 — x — y\n2. b.ts:2 — x — y', tail }) }).passed).toBe(true); + }); + + it('should refuse a finding that is neither fixed nor declined', () => { + expect(failuresOf({ body: review({ findings: '1. a.ts:1 — x — y' }) })).toContain( + 'Finding 1 is neither fixed nor declined.' + ); + }); + + it('should refuse a finding that is both fixed and declined', () => { + const tail = '\n### Fixed\n\n- 1. Guarded.\n\n### Declined\n\n- 1. Upstream guard.'; + expect(failuresOf({ body: review({ findings: '1. a.ts:1 — x — y', tail }) })).toContain( + 'Finding 1 is listed as both fixed and declined.' + ); + }); + + it('should refuse a fixed finding the review did not return', () => { + const tail = '\n### Fixed\n\n- 1. Guarded.\n- 2. Invented.'; + expect(failuresOf({ body: review({ findings: '1. a.ts:1 — x — y', tail }) })).toContain( + 'Finding 2 is fixed or declined but not listed under Findings.' + ); + }); + }); + + describe('payload', () => { + it.each([ + ['invalid JSON', '{'], + ['a payload without an action', JSON.stringify({ pull_request: {} })], + [ + 'a non-string body', + JSON.stringify({ action: 'opened', pull_request: { body: 1, head: { ref: 'x', sha: HEAD } } }) + ], + [ + 'a user without a type', + JSON.stringify({ + action: 'opened', + pull_request: { body: null, head: { ref: 'x', sha: HEAD }, user: { login: 'x' } } + }) + ] + ])('should report %s as a failure of the check itself, so the PR is not passed', (_, eventJson) => { + const { passed, report } = checkPullRequest(() => ({ commitMessages: '', eventJson })); + expect(passed).toBe(false); + expect(report).toContain('The check itself failed: '); + }); + + it('should report an input that cannot be read, so the workflow still has a comment to post', () => { + const { passed, report } = checkPullRequest(() => { + throw new Error('ENOENT: event.json'); + }); + expect(passed).toBe(false); + expect(report).toContain('The check itself failed: Error: ENOENT: event.json'); + }); + }); +}); diff --git a/.github/scripts/pr-body.ts b/.github/scripts/pr-body.ts new file mode 100644 index 000000000..c4449c069 --- /dev/null +++ b/.github/scripts/pr-body.ts @@ -0,0 +1,248 @@ +type PullRequestEvent = { + action: string; + pull_request: { + body: null | string; + head: { ref: string; sha: string }; + user: { login: string; type: string }; + }; +}; + +type PullRequest = { + commitMessages: string; + event: PullRequestEvent; +}; + +type PullRequestInput = { + commitMessages: string; + eventJson: string; +}; + +type PullRequestBodyCheck = { + passed: boolean; + report: string; +}; + +const SKILL_PATH = '.agents/skills/opening-pr/SKILL.md'; + +const FORBIDDEN_HEADING = /^#{1,6} +(verification|validation|implementation notes)\b/i; + +const NOTES_HEADING = /^#{1,6} +notes\b/i; + +const REVIEW_HEADING = /^## +adversarial review\b/i; + +const FENCE = /^(```|~~~)/; + +const STAMP = /^(?:Reviewed at commit|Follow-up) ([0-9a-f]{40})\b/gm; + +const ATTESTATION = /^I, [^,\n]+, (attest that .+|cannot attest: .+)$/m; + +const AGENT_TRAILER = /^Co-Authored-By: *(?:Claude|GPT|Codex|Copilot)\b/im; + +const AGENT_FOOTER = /^🤖 Generated with /m; + +// The replies of `.agents/skills/adversarial-review/SKILL.md` that carry no numbered findings. +const EMPTY_REVIEW_REPLIES = ['CLEAN', 'NO REVIEWABLE FILES']; + +const EXEMPT_BOTS = new Set(['dependabot[bot]']); + +// A merge-back carries commits whose own pull requests already passed this check into the release branch. +const EXEMPT_HEAD_REF = /^release\/\d+$/; + +function isRecord(value: unknown): value is { [key: string]: unknown } { + return typeof value === 'object' && value !== null; +} + +function parseEvent(contents: unknown): PullRequestEvent { + if (!isRecord(contents) || typeof contents.action !== 'string' || !isRecord(contents.pull_request)) { + throw new Error('Expected a pull_request event payload with an action and a pull_request'); + } + const { body, head, user } = contents.pull_request; + if ( + (body !== null && typeof body !== 'string') || + !isRecord(head) || + typeof head.ref !== 'string' || + typeof head.sha !== 'string' + ) { + throw new Error('Expected pull_request.body to be a string or null and pull_request.head.ref and sha strings'); + } + if (!isRecord(user) || typeof user.login !== 'string' || typeof user.type !== 'string') { + throw new Error('Expected pull_request.user to have a string login and type'); + } + return { + action: contents.action, + pull_request: { body, head: { ref: head.ref, sha: head.sha }, user: { login: user.login, type: user.type } } + }; +} + +// The template binds agents, not people. An agent that leaves no trace cannot be told apart from a +// person, so a review block is a trace too: a body that carries one is checked whatever else it says. +function isAgentAuthored({ commitMessages, event }: PullRequest): boolean { + const { body, head, user } = event.pull_request; + if (EXEMPT_HEAD_REF.test(head.ref)) { + return false; + } + const lines = outsideFences((body ?? '').split(/\r?\n/)); + const text = lines.join('\n'); + return ( + (user.type === 'Bot' && !EXEMPT_BOTS.has(user.login)) || + AGENT_TRAILER.test(commitMessages) || + AGENT_TRAILER.test(text) || + AGENT_FOOTER.test(text) || + ATTESTATION.test(text) || + lines.some((line) => REVIEW_HEADING.test(line)) + ); +} + +function outsideFences(lines: string[]): string[] { + let fenced = false; + return lines.filter((line) => { + if (FENCE.test(line)) { + fenced = !fenced; + return false; + } + return !fenced; + }); +} + +function findSection(text: string, heading: string, level: number): null | string { + const lines = text.split(/\r?\n/); + const title = `${'#'.repeat(level)} ${heading}`.toLowerCase(); + const start = lines.findIndex((line) => line.trim().toLowerCase() === title); + if (start === -1) { + return null; + } + const boundary = new RegExp(`^#{1,${level}}\\s`); + const end = lines.findIndex((line, index) => index > start && boundary.test(line)); + return lines.slice(start + 1, end === -1 ? undefined : end).join('\n'); +} + +function listNumbers(text: null | string): Set { + return new Set(Array.from((text ?? '').matchAll(/^\s*(?:-\s*)?(\d+)\./gm), (match) => Number(match[1]))); +} + +function checkStamp(review: string, event: PullRequestEvent): string[] { + const stamps = Array.from(review.matchAll(STAMP), (match) => match[1]); + const headSha = event.pull_request.head.sha; + if (stamps.length === 0) { + return ['The review block names no reviewed commit.']; + } + if (event.action !== 'edited' && !stamps.includes(headSha)) { + // The body is edited for a new head before that head is pushed, so an edit is checked against + // the head only once the push that follows it arrives. + return [`The review block names no commit matching the head \`${headSha}\`.`]; + } + return []; +} + +function checkFindings(review: string, findings: string): string[] { + const failures: string[] = []; + const fixed = listNumbers(findSection(review, 'fixed', 3)); + const declined = listNumbers(findSection(review, 'declined', 3)); + const reply = findings.trim(); + if (EMPTY_REVIEW_REPLIES.includes(reply)) { + if (fixed.size + declined.size > 0) { + failures.push(`A \`${reply}\` review lists nothing under Fixed or Declined.`); + } + return failures; + } + const found = listNumbers(findings); + const annotatedReply = EMPTY_REVIEW_REPLIES.find((token) => new RegExp(`^${token}\\b`).test(reply)); + if (found.size === 0 && annotatedReply) { + failures.push( + `\`### Findings\` holds text besides \`${annotatedReply}\`; it holds the review's reply alone, and context goes in Notes.` + ); + } else if (found.size === 0) { + const replies = EMPTY_REVIEW_REPLIES.map((token) => `\`${token}\``).join(', '); + failures.push(`\`### Findings\` is not ${replies} or numbered findings.`); + } + for (const number of found) { + if (fixed.has(number) && declined.has(number)) { + failures.push(`Finding ${number} is listed as both fixed and declined.`); + } else if (!fixed.has(number) && !declined.has(number)) { + failures.push(`Finding ${number} is neither fixed nor declined.`); + } + } + for (const number of new Set([...fixed, ...declined])) { + if (!found.has(number)) { + failures.push(`Finding ${number} is fixed or declined but not listed under Findings.`); + } + } + return failures; +} + +function checkReview(review: string, event: PullRequestEvent): string[] { + const failures: string[] = []; + if (!/\bpnpm check: green\b/.test(review)) { + failures.push('The review block does not state `pnpm check: green`.'); + } + const attestation = findSection(review, 'attestation', 3)?.match(ATTESTATION); + if (!attestation) { + failures.push( + 'The review block has no `### Attestation` with an `I, , attest that …` or `cannot attest: …` line.' + ); + } + const findings = findSection(review, 'findings', 3); + if (findings === null) { + failures.push('The review block has no `### Findings` section.'); + return failures; + } + const cannotAttest = attestation?.[1]?.startsWith('cannot attest') ?? false; + if (cannotAttest) { + if (findings.trim() === '') { + failures.push('A review that cannot attest says what happened under `### Findings`.'); + } + return failures; + } + return [...failures, ...checkStamp(review, event), ...checkFindings(review, findings)]; +} + +function checkBody(event: PullRequestEvent): string[] { + const { body } = event.pull_request; + if (!body?.trim()) { + return ['The body is empty.']; + } + const lines = outsideFences(body.split(/\r?\n/)); + const failures = lines + .filter((line) => FORBIDDEN_HEADING.test(line)) + .map((line) => `Remove the \`${line.trim()}\` section; the body carries effects, decisions and risks only.`); + const reviewIndex = lines.findIndex((line) => REVIEW_HEADING.test(line)); + if (lines.some((line, index) => index > reviewIndex && NOTES_HEADING.test(line))) { + failures.push('A Notes section goes before `## Adversarial review`, and nowhere after it.'); + } + const review = findSection(body, 'adversarial review', 2); + if (review === null) { + failures.push('The body has no `## Adversarial review` section.'); + } else { + failures.push(...checkReview(review, event)); + } + return failures; +} + +function formatReport(failures: string[]): string { + return [ + `This pull request was closed because its body does not meet \`${SKILL_PATH}\`:`, + '', + ...failures.map((failure) => `- ${failure}`), + '', + 'Fix the body with `gh pr edit --body-file `, then `gh pr reopen `.', + '' + ].join('\n'); +} + +export function checkPullRequest(readInput: () => PullRequestInput): PullRequestBodyCheck { + let failures: string[]; + try { + const { commitMessages, eventJson } = readInput(); + const pullRequest = { commitMessages, event: parseEvent(JSON.parse(eventJson)) }; + if (!isAgentAuthored(pullRequest)) { + return { passed: true, report: 'The pull request shows no agent authorship, so the template does not apply.\n' }; + } + failures = checkBody(pullRequest.event); + } catch (error) { + failures = [`The check itself failed: ${String(error)}`]; + } + if (failures.length > 0) { + return { passed: false, report: formatReport(failures) }; + } + return { passed: true, report: 'The pull request body meets the template.\n' }; +} diff --git a/scripts/check-pr-body.ts b/scripts/check-pr-body.ts index 35512fad5..59f414fe8 100644 --- a/scripts/check-pr-body.ts +++ b/scripts/check-pr-body.ts @@ -1,244 +1,16 @@ import fs from 'node:fs'; -type PullRequestEvent = { - action: string; - pull_request: { - body: null | string; - head: { ref: string; sha: string }; - user: { login: string; type: string }; - }; -}; - -type PullRequest = { - commitMessages: string; - event: PullRequestEvent; -}; - -const SKILL_PATH = '.agents/skills/opening-pr/SKILL.md'; - -const FORBIDDEN_HEADING = /^#{1,6} +(verification|validation|implementation notes)\b/i; - -const NOTES_HEADING = /^#{1,6} +notes\b/i; - -const REVIEW_HEADING = /^## +adversarial review\b/i; - -const FENCE = /^(```|~~~)/; - -const STAMP = /^(?:Reviewed at commit|Follow-up) ([0-9a-f]{40})\b/gm; - -const ATTESTATION = /^I, [^,\n]+, (attest that .+|cannot attest: .+)$/m; - -const AGENT_TRAILER = /^Co-Authored-By: *(?:Claude|GPT|Codex|Copilot)\b/im; - -const AGENT_FOOTER = /^🤖 Generated with /m; - -const EXEMPT_BOTS = new Set(['dependabot[bot]']); +import { checkPullRequest } from '../.github/scripts/pr-body.ts'; -// A merge-back carries commits whose own pull requests already passed this check into the release branch. -const EXEMPT_HEAD_REF = /^release\/\d+$/; - -function isRecord(value: unknown): value is { [key: string]: unknown } { - return typeof value === 'object' && value !== null; -} - -function parseEvent(contents: unknown): PullRequestEvent { - if (!isRecord(contents) || typeof contents.action !== 'string' || !isRecord(contents.pull_request)) { - throw new Error('Expected a pull_request event payload with an action and a pull_request'); - } - const { body, head, user } = contents.pull_request; - if ( - (body !== null && typeof body !== 'string') || - !isRecord(head) || - typeof head.ref !== 'string' || - typeof head.sha !== 'string' - ) { - throw new Error('Expected pull_request.body to be a string or null and pull_request.head.ref and sha strings'); - } - if (!isRecord(user) || typeof user.login !== 'string' || typeof user.type !== 'string') { - throw new Error('Expected pull_request.user to have a string login and type'); - } - return { - action: contents.action, - pull_request: { body, head: { ref: head.ref, sha: head.sha }, user: { login: user.login, type: user.type } } - }; -} - -function readPullRequest(): PullRequest { +const { passed, report } = checkPullRequest(() => { const [eventPath, commitMessagesPath] = process.argv.slice(2); if (!eventPath || !commitMessagesPath) { throw new Error('Usage: node scripts/check-pr-body.ts '); } return { commitMessages: fs.readFileSync(commitMessagesPath, 'utf8'), - event: parseEvent(JSON.parse(fs.readFileSync(eventPath, 'utf8'))) + eventJson: fs.readFileSync(eventPath, 'utf8') }; -} - -// The template binds agents, not people. An agent that leaves no trace cannot be told apart from a -// person, so a review block is a trace too: a body that carries one is checked whatever else it says. -function isAgentAuthored({ commitMessages, event }: PullRequest): boolean { - const { body, head, user } = event.pull_request; - if (EXEMPT_HEAD_REF.test(head.ref)) { - return false; - } - const lines = outsideFences((body ?? '').split(/\r?\n/)); - const text = lines.join('\n'); - return ( - (user.type === 'Bot' && !EXEMPT_BOTS.has(user.login)) || - AGENT_TRAILER.test(commitMessages) || - AGENT_TRAILER.test(text) || - AGENT_FOOTER.test(text) || - ATTESTATION.test(text) || - lines.some((line) => REVIEW_HEADING.test(line)) - ); -} - -function outsideFences(lines: string[]): string[] { - let fenced = false; - return lines.filter((line) => { - if (FENCE.test(line)) { - fenced = !fenced; - return false; - } - return !fenced; - }); -} - -function findSection(text: string, heading: string, level: number): null | string { - const lines = text.split(/\r?\n/); - const title = `${'#'.repeat(level)} ${heading}`.toLowerCase(); - const start = lines.findIndex((line) => line.trim().toLowerCase() === title); - if (start === -1) { - return null; - } - const boundary = new RegExp(`^#{1,${level}}\\s`); - const end = lines.findIndex((line, index) => index > start && boundary.test(line)); - return lines.slice(start + 1, end === -1 ? undefined : end).join('\n'); -} - -function listNumbers(text: null | string): Set { - return new Set(Array.from((text ?? '').matchAll(/^\s*(?:-\s*)?(\d+)\./gm), (match) => Number(match[1]))); -} - -function checkStamp(review: string, event: PullRequestEvent): string[] { - const stamps = Array.from(review.matchAll(STAMP), (match) => match[1]); - const headSha = event.pull_request.head.sha; - if (stamps.length === 0) { - return ['The review block names no reviewed commit.']; - } - if (event.action !== 'edited' && !stamps.includes(headSha)) { - // The body is edited for a new head before that head is pushed, so an edit is checked against - // the head only once the push that follows it arrives. - return [`The review block names no commit matching the head \`${headSha}\`.`]; - } - return []; -} - -function checkFindings(review: string, findings: string): string[] { - const failures: string[] = []; - const fixed = listNumbers(findSection(review, 'fixed', 3)); - const declined = listNumbers(findSection(review, 'declined', 3)); - if (findings.trim() === 'CLEAN') { - if (fixed.size + declined.size > 0) { - failures.push('A `CLEAN` review lists nothing under Fixed or Declined.'); - } - return failures; - } - const found = listNumbers(findings); - if (found.size === 0) { - failures.push('`### Findings` is neither `CLEAN` nor numbered findings.'); - } - for (const number of found) { - if (fixed.has(number) && declined.has(number)) { - failures.push(`Finding ${number} is listed as both fixed and declined.`); - } else if (!fixed.has(number) && !declined.has(number)) { - failures.push(`Finding ${number} is neither fixed nor declined.`); - } - } - for (const number of new Set([...fixed, ...declined])) { - if (!found.has(number)) { - failures.push(`Finding ${number} is fixed or declined but not listed under Findings.`); - } - } - return failures; -} - -function checkReview(review: string, event: PullRequestEvent): string[] { - const failures: string[] = []; - if (!/\bpnpm check: green\b/.test(review)) { - failures.push('The review block does not state `pnpm check: green`.'); - } - const attestation = findSection(review, 'attestation', 3)?.match(ATTESTATION); - if (!attestation) { - failures.push( - 'The review block has no `### Attestation` with an `I, , attest that …` or `cannot attest: …` line.' - ); - } - const findings = findSection(review, 'findings', 3); - if (findings === null) { - failures.push('The review block has no `### Findings` section.'); - return failures; - } - const cannotAttest = attestation?.[1]?.startsWith('cannot attest') ?? false; - if (cannotAttest) { - if (findings.trim() === '') { - failures.push('A review that cannot attest says what happened under `### Findings`.'); - } - return failures; - } - return [...failures, ...checkStamp(review, event), ...checkFindings(review, findings)]; -} - -function checkBody(event: PullRequestEvent): string[] { - const { body } = event.pull_request; - if (!body?.trim()) { - return ['The body is empty.']; - } - const lines = outsideFences(body.split(/\r?\n/)); - const failures = lines - .filter((line) => FORBIDDEN_HEADING.test(line)) - .map((line) => `Remove the \`${line.trim()}\` section; the body carries effects, decisions and risks only.`); - const reviewIndex = lines.findIndex((line) => REVIEW_HEADING.test(line)); - if (lines.some((line, index) => index > reviewIndex && NOTES_HEADING.test(line))) { - failures.push('A Notes section goes before `## Adversarial review`, and nowhere after it.'); - } - const review = findSection(body, 'adversarial review', 2); - if (review === null) { - failures.push('The body has no `## Adversarial review` section.'); - } else { - failures.push(...checkReview(review, event)); - } - return failures; -} - -function report(failures: string[]): never { - const lines = [ - `This pull request was closed because its body does not meet \`${SKILL_PATH}\`:`, - '', - ...failures.map((failure) => `- ${failure}`), - '', - 'Fix the body with `gh pr edit --body-file `, then `gh pr reopen `.' - ]; - process.stdout.write(`${lines.join('\n')}\n`); - process.exit(1); -} - -function main(): void { - let failures: string[]; - try { - const pullRequest = readPullRequest(); - if (!isAgentAuthored(pullRequest)) { - process.stdout.write('The pull request shows no agent authorship, so the template does not apply.\n'); - return; - } - failures = checkBody(pullRequest.event); - } catch (error) { - report([`The check itself failed: ${error instanceof Error ? error.message : String(error)}`]); - } - if (failures.length > 0) { - report(failures); - } - process.stdout.write('The pull request body meets the template.\n'); -} - -main(); +}); +process.stdout.write(report); +process.exitCode = passed ? 0 : 1; diff --git a/tsconfig.json b/tsconfig.json index 1a3196143..0126ced6b 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -9,10 +9,12 @@ "module": "ESNext", "moduleResolution": "Bundler", "resolveJsonModule": true, + "rewriteRelativeImportExtensions": true, "types": ["node"] }, "include": [ ".github/scripts/__tests__/*.ts", + ".github/scripts/pr-body.ts", ".github/scripts/release.cjs", ".github/vitest.config.ts", "commitlint.config.ts", diff --git a/vitest.config.ts b/vitest.config.ts index dac228108..b7b10f9ac 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -33,7 +33,7 @@ export default mergeConfig( 'apps/web/src/testing/**' ], include: [ - '.github/scripts/release.cjs', + '.github/scripts/*.?(c|m)[jt]s', 'apps/{api,gateway,web}/src/**/*.?(c|m)[jt]s?(x)', 'packages/*/src/**/*.?(c|m)[jt]s?(x)' ],