diff --git a/.github/scripts/pr-readiness/closing-issues.mjs b/.github/scripts/pr-readiness/closing-issues.mjs new file mode 100644 index 0000000..c7d8e52 --- /dev/null +++ b/.github/scripts/pr-readiness/closing-issues.mjs @@ -0,0 +1,207 @@ +import { stripQuotedText } from "../review-request/common.mjs"; + +export const CLOSING_ISSUE_LIMIT = 100; + +const REPOSITORY = + "[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?/[A-Za-z0-9._-]+"; +// One of GitHub's closing keywords, an optional colon, then one Issue +// reference: `#N`, `owner/repo#N`, or an Issue URL. +const CLOSING_REFERENCE = new RegExp( + "(?|$)/g; +const LIST_MARKER = /^(?:[-*+]|\d{1,9}[.)])(?=[ \t]|$)/; + +// The column where a run of leading whitespace ends when it starts at +// `column`, with tabs advancing to four-column stops as CommonMark counts them. +function whitespaceEnd(text, column = 0) { + let end = column; + for (const char of text) { + if (char === " ") end += 1; + else if (char === "\t") end += 4 - (end % 4); + else break; + } + return end; +} + +// An indented code block is four or more columns past the enclosing list +// item's content, starts after a blank line, and cannot interrupt a paragraph. +// Less indentation under a list item is that item's own text. +function blankIndentedCode(text) { + const contentColumns = []; + let afterBlank = true; + let inCode = false; + return text.split("\n").map((line) => { + if (line.trim() === "") { + afterBlank = true; + return line; + } + const indent = whitespaceEnd(line); + while (contentColumns.length > 0 && indent < contentColumns.at(-1)) { + contentColumns.pop(); + } + const relative = indent - (contentColumns.at(-1) ?? 0); + const code = relative >= 4 && (afterBlank || inCode); + afterBlank = false; + inCode = code; + if (code) return ""; + const rest = line.trimStart(); + const marker = relative <= 3 ? LIST_MARKER.exec(rest) : null; + if (marker) { + const markerEnd = indent + marker[0].length; + const padding = whitespaceEnd(rest.slice(marker[0].length), markerEnd) + - markerEnd; + contentColumns.push( + markerEnd + (padding >= 1 && padding <= 4 ? padding : 1), + ); + } + return line; + }).join("\n"); +} + +const ISSUE_FIELDS = ` + repository { nameWithOwner } + number + title + body + state + issueType { name } + parent { number } + subIssues(first: 100) { + totalCount + nodes { + repository { nameWithOwner } + number + state + } + } + blockedBy(first: 100) { + totalCount + nodes { + repository { nameWithOwner } + number + state + } + } + blocking(first: 100) { + totalCount + nodes { + repository { nameWithOwner } + number + state + } + } +`; + +function referenceKey(repository, number) { + return `${String(repository).toLowerCase()}#${Number(number)}`; +} + +// The Issues a pull-request body declares it closes, in body order and +// without duplicates. Quoted text, code, and HTML comments declare nothing, +// so a template placeholder or a pasted log cannot satisfy the linkage rule. +export function closingIssueReferences(body, repository) { + const text = blankIndentedCode(stripQuotedText( + String(body ?? "").replace(HTML_COMMENT, (chunk) => ( + chunk.replace(/[^\n]/g, " ") + )), + )); + const references = []; + const seen = new Set(); + for (const match of text.matchAll(CLOSING_REFERENCE)) { + const target = match[1] ?? match[4] ?? String(repository ?? ""); + const number = Number(match[2] ?? match[3] ?? match[5]); + if (!target || !Number.isSafeInteger(number) || number < 1) continue; + const key = referenceKey(target, number); + if (seen.has(key)) continue; + seen.add(key); + references.push({ + repository: target.toLowerCase() === String(repository ?? "").toLowerCase() + ? String(repository) + : target, + number, + }); + } + return references; +} + +const clip = (value, length) => String(value ?? "").slice(0, length); +const relation = (item) => ({ + repository: item.repository.nameWithOwner, + number: item.number, + state: item.state, +}); + +function normalizeIssue(issue) { + return { + repository: issue.repository.nameWithOwner, + number: issue.number, + title: clip(issue.title, 500), + body: clip(issue.body, 80_000), + body_truncated: String(issue.body ?? "").length > 80_000, + state: issue.state, + issue_type: issue.issueType?.name || "", + parent_number: issue.parent?.number ?? null, + sub_issue_count: issue.subIssues.totalCount, + sub_issue_numbers: issue.subIssues.nodes.map((item) => item.number), + sub_issues: issue.subIssues.nodes.map(relation), + blocked_by_count: issue.blockedBy.totalCount, + blocked_by: issue.blockedBy.nodes.map(relation), + blocking_count: issue.blocking.totalCount, + blocking: issue.blocking.nodes.map(relation), + }; +} + +// Reads the referenced Issues in one request. `graphql(query)` resolves to the +// raw `{ data, errors }` response. A reference that is not a readable Issue +// (a pull request, a missing number, another private repository) resolves to +// null, as GitHub ignores it; any other error fails closed. +export function graphqlIssueFetcher(graphql) { + return async (references) => { + if (references.length === 0) return []; + const fields = references.map((reference, index) => { + const [owner, name] = reference.repository.split("/"); + return `i${index}: repository(owner: ${JSON.stringify(owner)}, ` + + `name: ${JSON.stringify(name)}) { ` + + `issue(number: ${reference.number}) { ${ISSUE_FIELDS} } }`; + }); + const payload = await graphql(`query { ${fields.join("\n")} }`); + const fatal = (payload?.errors ?? []).find( + (error) => error.type !== "NOT_FOUND", + ); + if (fatal) throw new Error(`GitHub GraphQL failed: ${fatal.message}`); + if (!payload?.data) { + throw new Error("GitHub GraphQL returned no closing-Issue data"); + } + return references.map( + (_reference, index) => payload.data[`i${index}`]?.issue ?? null, + ); + }; +} + +// The linked-Issue evidence of a pull request: every readable Issue its body +// closes. More references than the bound are counted but not read, which the +// readiness analysis reports instead of reviewing a truncated set. +export async function collectClosingIssues({ repository, body, fetchIssues }) { + const references = closingIssueReferences(body, repository); + const issues = await fetchIssues(references.slice(0, CLOSING_ISSUE_LIMIT)); + const linkedIssues = []; + const seen = new Set(); + for (const issue of issues) { + if (!issue) continue; + const normalized = normalizeIssue(issue); + const key = referenceKey(normalized.repository, normalized.number); + if (seen.has(key)) continue; + seen.add(key); + linkedIssues.push(normalized); + } + return { + linked_issue_count: references.length > CLOSING_ISSUE_LIMIT + ? references.length + : linkedIssues.length, + linked_issues: linkedIssues, + }; +} diff --git a/.github/scripts/pr-readiness/common.mjs b/.github/scripts/pr-readiness/common.mjs index 5c0554f..4eecc6e 100644 --- a/.github/scripts/pr-readiness/common.mjs +++ b/.github/scripts/pr-readiness/common.mjs @@ -73,14 +73,14 @@ export function analyzePullRequest(input) { deterministicBlockers.push(blocker( "pr-linkage", "missing-closing-issue", - "Pull request must natively close at least one same-repository Issue.", + "Pull request body must close at least one same-repository Issue with a closing keyword, for example `Closes #123`.", )); } if (Number(input.linked_issue_count ?? linkedIssues.length) > linkedIssues.length) { deterministicBlockers.push(blocker( "pr-linkage", "too-many-closing-issues", - "The workflow could not review every native closing Issue within its configured bound.", + "The workflow could not review every closing Issue within its configured bound.", )); } if (Number(input.unresolved_openai_thread_count ?? 0) > 0) { @@ -127,7 +127,7 @@ export function analyzePullRequest(input) { ...blocker( "pr-linkage", "missing-open-sub-issues", - `Pull request closes ${parent} but does not natively close its open sub-issue(s): ${missing.join(", ")}.`, + `Pull request closes ${parent} but does not close its open sub-issue(s): ${missing.join(", ")}.`, ), issue_repository: issue.snapshot.repository, issue_number: issue.snapshot.number, diff --git a/.github/scripts/pr-readiness/test.mjs b/.github/scripts/pr-readiness/test.mjs index 139a0ba..e4b9dc8 100644 --- a/.github/scripts/pr-readiness/test.mjs +++ b/.github/scripts/pr-readiness/test.mjs @@ -5,6 +5,12 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { spawnSync } from "node:child_process"; +import { + CLOSING_ISSUE_LIMIT, + closingIssueReferences, + collectClosingIssues, + graphqlIssueFetcher, +} from "./closing-issues.mjs"; import { analyzePullRequest, evaluateReadiness } from "./common.mjs"; const issueBody = [ @@ -34,7 +40,7 @@ const input = { repository: "GizClaw/example", number: 11, title: "ci: Add readiness gate", - body: "Implements the plan.\n\nValidation: node test.mjs", + body: "Closes #10\n\nImplements the plan.\n\nValidation: node test.mjs", base_sha: "a".repeat(40), head_sha: "b".repeat(40), linked_issues: [{ @@ -83,6 +89,11 @@ const manyLinkedIssues = [{ }, ...closingChildren]; const manyLinkedIssuesInput = { ...input, + body: [ + ...manyLinkedIssues.map((issue) => `Closes #${issue.number}`), + "", + "Implements the plan.", + ].join("\n"), linked_issues: manyLinkedIssues, linked_issue_count: manyLinkedIssues.length, }; @@ -95,13 +106,222 @@ const workflowSource = fs.readFileSync( path.join(scriptDirectory, "..", "..", "workflows", "codex-openai-review.yml"), "utf8", ); +const closingIssuesSource = fs.readFileSync( + path.join(scriptDirectory, "closing-issues.mjs"), + "utf8", +); +// Both collectors read the Issues the body closes through the shared module +// and never GitHub's Development links. for (const source of [verifySource, workflowSource]) { - assert.match(source, /closingIssuesReferences\(first: 100\)/); - assert.match(source, /blockedBy\(first: 100\)/); - assert.match(source, /blocking\(first: 100\)/); - assert.match(source, /blocked_by_count:\s*issue\.blockedBy\.totalCount/); - assert.match(source, /blocking_count:\s*issue\.blocking\.totalCount/); - assert.doesNotMatch(source, /closingIssuesReferences\.nodes\s*\.slice\(/); + assert.match(source, /collectClosingIssues\(/); + assert.doesNotMatch(source, /closingIssuesReferences/); + assert.doesNotMatch(source, /closedByPullRequestsReferences/); +} +assert.match(closingIssuesSource, /subIssues\(first: 100\)/); +assert.match(closingIssuesSource, /blockedBy\(first: 100\)/); +assert.match(closingIssuesSource, /blocking\(first: 100\)/); +assert.match( + closingIssuesSource, + /blocked_by_count:\s*issue\.blockedBy\.totalCount/, +); +assert.match( + closingIssuesSource, + /blocking_count:\s*issue\.blocking\.totalCount/, +); +// The reusable reviewer and the verifier both check out the module's +// dependency on the request parser's quoted-text stripping. +assert.equal( + workflowSource.match( + /\.github\/scripts\/pr-readiness\n\s+(?:\.github\/scripts\/pr-review\n\s+)?\.github\/scripts\/review-request\n/g, + )?.length, + 2, +); + +const references = (body) => closingIssueReferences(body, "GizClaw/example") + .map((item) => `${item.repository}#${item.number}`); +assert.deepEqual(references("Closes #10"), ["GizClaw/example#10"]); +for (const keyword of [ + "close", "closes", "closed", "fix", "fixes", "fixed", + "resolve", "resolves", "resolved", "CLOSES", "Fixes:", +]) { + assert.deepEqual( + references(`Summary.\n\n${keyword} #7.`), + ["GizClaw/example#7"], + keyword, + ); +} +assert.deepEqual( + references([ + "Closes #10, fixes gizclaw/EXAMPLE#10 and resolves Other/repo#3.", + "Closes https://github.com/Other/repo/issues/4", + "Closes #11", + ].join("\n")), + ["GizClaw/example#10", "Other/repo#3", "Other/repo#4", "GizClaw/example#11"], +); +for (const body of [ + "", + "Related to #10, but this does not close it.", + "Part of #10. See #10.", + "hotfixes #10 and discloses #10", + "Closes 10", + "Closes #0", + "Closes #10abc", + "Closes\n#10", + "Closes https://github.com/Other/repo/pull/4", + "`Closes #10`", + "```\nCloses #10\n```", + "> Closes #10", + "", + "