diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs index 5558a01d..2195a147 100644 --- a/.github/actions/apply-review-hold/hold.cjs +++ b/.github/actions/apply-review-hold/hold.cjs @@ -132,8 +132,15 @@ function linkedIssue(pull, owner, repo) { return refs === null ? null : { number: refs, refs: true }; } -const names_ = (number) => - new RegExp(`(?`: not part of a +// longer number, and not after a letter, digit, `_`, `#`, `&`, `/`, `.` or +// `-`, so `owner/repo#` and `&#;` do not count +// (claude-code-plugins .github/scripts/pr-merge/merge-holds.mjs:436-438 +// namesPull at #9569's head 6c3b6ae9). +const namesPull = (text, number) => + new RegExp(`(?:^|[^A-Za-z0-9_#&/.-])#${number}(?:[^0-9]|$)`, "u").test( + String(text ?? ""), + ); // Whether a trusted person recorded a cost decision for pull request // `number`. `prComments` is the pull request's comment list. Reads only @@ -173,7 +180,9 @@ async function costDecided(github, owner, repo, number, env, prComments) { const read = ( await github.rest.issues.get({ owner, repo, issue_number: issue.number }) ).data; - if (read?.pull_request) return false; + // An issue whose number is the Refs number (merge-holds.mjs:484 at + // #9569's head 6c3b6ae9), never a pull request. + if (read?.number !== issue.number || read.pull_request != null) return false; } const opened = Date.parse(pull.createdAt); const comments = await github.paginate(github.rest.issues.listComments, { @@ -186,7 +195,7 @@ async function costDecided(github, owner, repo, number, env, prComments) { (c) => candidate(c) && Date.parse(c.created_at) > opened && - names_(number).test(String(c.body ?? "")), + namesPull(c.body, number), )) if (await unedited(c)) return true; } catch { @@ -367,4 +376,12 @@ async function run({ github, context, core, env = process.env }) { return { hold: true, criteria }; } -module.exports = { CRITERIA, globToRegExp, linkedIssue, modelHold, refsIssue, run }; +module.exports = { + CRITERIA, + globToRegExp, + linkedIssue, + modelHold, + namesPull, + refsIssue, + run, +}; diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index 02e44bca..09c024ec 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -9,7 +9,11 @@ const fs = require("node:fs"); const path = require("node:path"); const test = require("node:test"); -const { globToRegExp, run } = require("../actions/apply-review-hold/hold.cjs"); +const { + globToRegExp, + namesPull, + run, +} = require("../actions/apply-review-hold/hold.cjs"); const { makeTemporaryDirectory } = require("./temporary-directories.cjs"); const { parseWorkflow } = require("./workflow-yaml.cjs"); @@ -40,7 +44,8 @@ const TRUST_PATHS = [ // `comments` are the pull request's (#7); `issueComments` maps another // issue number to its comments; `pull` is the GraphQL pull request; // `edited` holds the node ids of edited comments; `pullNumbers` the numbers -// that are pull requests, not issues. +// that are pull requests, not issues; `misnumbered` the numbers whose read +// answers another number. function fakeGithub({ files = [], changedFiles, @@ -54,6 +59,7 @@ function fakeGithub({ }, edited = new Set(), pullNumbers = new Set([7]), + misnumbered = new Set(), }) { const calls = []; const refuse = (name) => () => { @@ -103,6 +109,8 @@ function fakeGithub({ }, get: async (params) => { calls.push(["issues.get", params]); + if (misnumbered.has(params.issue_number)) + return { data: { number: params.issue_number + 1 } }; return { data: pullNumbers.has(params.issue_number) ? { number: params.issue_number, pull_request: {} } @@ -550,6 +558,16 @@ test("an edited, quoted, older or non-cost decision still holds", async () => { }, }, ], + [ + "a Refs read that answers another number", + { + pull: refsPull("Refs: #9436"), + misnumbered: new Set([9436]), + issueComments: { + 9436: [decision({ body: "**Decision recorded.** Cost: #7" })], + }, + }, + ], [ "two Refs issues", { @@ -593,3 +611,43 @@ test("a hold with no cost criterion reads no cost decision", async () => { 1, ); }); + +// The same cases merge-holds.mjs namesPull reads, so both halves agree. +test("a decision names the pull request as # the way merge-holds reads it", () => { + for (const [text, named] of [ + ["approved for #7.", true], + ["#7", true], + ["(#7)", true], + ["##7", false], + ["", false], + ["#70", false], + ["#77", false], + ["o/r#7", false], + ["x.#7", false], + ["x-#7", false], + ["a#7", false], + ["_#7", false], + ]) + assert.equal(namesPull(text, 7), named, text); +}); + +// #9436: the owner lifted the cost hold at head A; a base-only merge makes +// head B, and the review holds for cost again. The decision covers B. +test("a cost decision recorded at an earlier head keeps the cost hold off at a later one", async () => { + const later = "b".repeat(40); + const atA = decision({ + body: `**Decision recorded.** Cost: approve the schedule at ${HEAD}.`, + }); + const { core, writes } = await apply(costEnv(["cost"], { HEAD_SHA: later }), { + comments: [ + atA, + { + id: 2, + user: { login: "github-actions[bot]", type: "Bot" }, + body: ``, + }, + ], + }); + assert.equal(core.outputs.hold, "false"); + assert.deepEqual(writes, []); +});