From b64aaeb86e004061ff69d6a2ca72786f807c023a Mon Sep 17 00:00:00 2001 From: idy Date: Wed, 30 Sep 2026 17:36:57 +0800 Subject: [PATCH 1/5] ci: Read closing Issues from the PR body The reviewer took a pull request's closing Issues from GraphQL closingIssuesReferences. GitHub stopped deriving those from closing keywords, so PRs whose body says "Closes #N" were blocked on pr-linkage. The closing Issues are now the ones the body names with a closing keyword, read in one GraphQL request; Development links are no longer consulted. Closes #53 Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-readiness/closing-issues.mjs | 159 +++++++++ .github/scripts/pr-readiness/common.mjs | 6 +- .github/scripts/pr-readiness/test.mjs | 322 +++++++++++++----- .github/scripts/pr-readiness/verify.mjs | 179 ++++------ .github/scripts/pr-review/run.mjs | 2 +- .github/scripts/pr-review/test.mjs | 58 +++- .github/workflows/codex-openai-review.yml | 95 ++---- .../workflows/openai-pr-review-dispatch.yml | 4 +- README.md | 43 ++- 9 files changed, 577 insertions(+), 291 deletions(-) create mode 100644 .github/scripts/pr-readiness/closing-issues.mjs diff --git a/.github/scripts/pr-readiness/closing-issues.mjs b/.github/scripts/pr-readiness/closing-issues.mjs new file mode 100644 index 0000000..8576ffc --- /dev/null +++ b/.github/scripts/pr-readiness/closing-issues.mjs @@ -0,0 +1,159 @@ +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 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 = 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..93c2cf1 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,201 @@ 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", + "", + "|$)/g; +const INDENTED = /^(?: {4,}| {0,3}\t)/; +const LIST_ITEM = /^ {0,3}(?:[-*+]|\d{1,9}[.)])(?:[ \t]|$)/; + +// An indented code block starts after a blank line and cannot interrupt a +// paragraph. Indented lines under a list item are that item's content. +function blankIndentedCode(text) { + let afterBlank = true; + let inCode = false; + let inList = false; + return text.split("\n").map((line) => { + if (line.trim() === "") { + afterBlank = true; + return line; + } + const indented = INDENTED.test(line); + const code = indented && !inList && (afterBlank || inCode); + afterBlank = false; + inCode = code; + if (!indented) inList = LIST_ITEM.test(line); + return code ? "" : line; + }).join("\n"); +} const ISSUE_FIELDS = ` repository { nameWithOwner } @@ -56,11 +78,11 @@ function referenceKey(repository, number) { // 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 = stripQuotedText( + 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)) { diff --git a/.github/scripts/pr-readiness/test.mjs b/.github/scripts/pr-readiness/test.mjs index 93c2cf1..6a45eb8 100644 --- a/.github/scripts/pr-readiness/test.mjs +++ b/.github/scripts/pr-readiness/test.mjs @@ -173,9 +173,23 @@ for (const body of [ "> Closes #10", "", "|$)/g; -const INDENTED = /^(?: {4,}| {0,3}\t)/; -const LIST_ITEM = /^ {0,3}(?:[-*+]|\d{1,9}[.)])(?:[ \t]|$)/; +const LIST_MARKER = /^(?:[-*+]|\d{1,9}[.)])(?=[ \t]|$)/; -// An indented code block starts after a blank line and cannot interrupt a -// paragraph. Indented lines under a list item are that item's content. +// Width of a line's leading whitespace, with tabs advancing to four-column +// stops as CommonMark counts them. +function leadingWidth(line) { + let width = 0; + for (const char of line) { + if (char === " ") width += 1; + else if (char === "\t") width += 4 - (width % 4); + else break; + } + return width; +} + +// 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; - let inList = false; return text.split("\n").map((line) => { if (line.trim() === "") { afterBlank = true; return line; } - const indented = INDENTED.test(line); - const code = indented && !inList && (afterBlank || inCode); + const indent = leadingWidth(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 (!indented) inList = LIST_ITEM.test(line); - return code ? "" : line; + if (code) return ""; + const rest = line.trimStart(); + const marker = relative <= 3 ? LIST_MARKER.exec(rest) : null; + if (marker) { + const padding = leadingWidth(rest.slice(marker[0].length)); + contentColumns.push( + indent + marker[0].length + (padding >= 1 && padding <= 4 ? padding : 1), + ); + } + return line; }).join("\n"); } diff --git a/.github/scripts/pr-readiness/test.mjs b/.github/scripts/pr-readiness/test.mjs index 6a45eb8..eecf41d 100644 --- a/.github/scripts/pr-readiness/test.mjs +++ b/.github/scripts/pr-readiness/test.mjs @@ -176,6 +176,9 @@ for (const body of [ " Closes #10", "\tCloses #10", "Summary.\n\n Closes #10\n\n Fixes #11\n", + "- Example\n\n Closes #10", + "1. Example\n\n Closes #10", + "- Outer\n - Inner\n\n Closes #10", ]) { assert.deepEqual(references(body), [], JSON.stringify(body)); } @@ -186,6 +189,8 @@ for (const body of [ "Summary:\n Closes #10", "- Linkage\n\n Closes #10", "1. Linkage\n - Closes #10", + "- Outer\n - Inner\n\n Closes #10", + "- Example\n\n code\n\nCloses #10", " code\n\nCloses #10", ]) { assert.deepEqual(references(body), ["GizClaw/example#10"], JSON.stringify(body)); From 824e0e3c5f7ec443a86042ca0a102bc195767d62 Mon Sep 17 00:00:00 2001 From: idy Date: Wed, 30 Sep 2026 18:15:52 +0800 Subject: [PATCH 5/5] ci: Measure tab-padded list markers from their column Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-readiness/closing-issues.mjs | 24 ++++++++++--------- .github/scripts/pr-readiness/test.mjs | 2 ++ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/.github/scripts/pr-readiness/closing-issues.mjs b/.github/scripts/pr-readiness/closing-issues.mjs index 2fc7c7e..c7d8e52 100644 --- a/.github/scripts/pr-readiness/closing-issues.mjs +++ b/.github/scripts/pr-readiness/closing-issues.mjs @@ -15,16 +15,16 @@ const CLOSING_REFERENCE = new RegExp( const HTML_COMMENT = /|$)/g; const LIST_MARKER = /^(?:[-*+]|\d{1,9}[.)])(?=[ \t]|$)/; -// Width of a line's leading whitespace, with tabs advancing to four-column -// stops as CommonMark counts them. -function leadingWidth(line) { - let width = 0; - for (const char of line) { - if (char === " ") width += 1; - else if (char === "\t") width += 4 - (width % 4); +// 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 width; + return end; } // An indented code block is four or more columns past the enclosing list @@ -39,7 +39,7 @@ function blankIndentedCode(text) { afterBlank = true; return line; } - const indent = leadingWidth(line); + const indent = whitespaceEnd(line); while (contentColumns.length > 0 && indent < contentColumns.at(-1)) { contentColumns.pop(); } @@ -51,9 +51,11 @@ function blankIndentedCode(text) { const rest = line.trimStart(); const marker = relative <= 3 ? LIST_MARKER.exec(rest) : null; if (marker) { - const padding = leadingWidth(rest.slice(marker[0].length)); + const markerEnd = indent + marker[0].length; + const padding = whitespaceEnd(rest.slice(marker[0].length), markerEnd) + - markerEnd; contentColumns.push( - indent + marker[0].length + (padding >= 1 && padding <= 4 ? padding : 1), + markerEnd + (padding >= 1 && padding <= 4 ? padding : 1), ); } return line; diff --git a/.github/scripts/pr-readiness/test.mjs b/.github/scripts/pr-readiness/test.mjs index eecf41d..e4b9dc8 100644 --- a/.github/scripts/pr-readiness/test.mjs +++ b/.github/scripts/pr-readiness/test.mjs @@ -179,6 +179,7 @@ for (const body of [ "- Example\n\n Closes #10", "1. Example\n\n Closes #10", "- Outer\n - Inner\n\n Closes #10", + "-\tExample\n\n Closes #10", ]) { assert.deepEqual(references(body), [], JSON.stringify(body)); } @@ -190,6 +191,7 @@ for (const body of [ "- Linkage\n\n Closes #10", "1. Linkage\n - Closes #10", "- Outer\n - Inner\n\n Closes #10", + "-\tExample\n\n Closes #10", "- Example\n\n code\n\nCloses #10", " code\n\nCloses #10", ]) {