From 9d3ed2de736240d706f5c95d1dad00b07d31d293 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:55:32 -0400 Subject: [PATCH 1/3] fix(pr-review-cli): read # in a cost decision as merge-holds does The hold step's matcher for `#` in a linked-issue decision differed from merge-holds.mjs namesPull (claude-code-plugins#9569). It took `.#7` and `-#7` and refused `##7` and ``. It now uses the same pattern, so the hold step and the merge-side readers agree on which decisions name the pull request. Co-Authored-By: Claude Opus 5.5 --- .github/actions/apply-review-hold/hold.cjs | 22 +++++++++++++++---- .github/scripts/apply-review-hold.test.cjs | 25 +++++++++++++++++++++- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs index 5558a01..2b673c2 100644 --- a/.github/actions/apply-review-hold/hold.cjs +++ b/.github/actions/apply-review-hold/hold.cjs @@ -132,8 +132,14 @@ 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#` does not count (claude-code-plugins +// .github/scripts/pr-merge/merge-holds.mjs namesPull, from #9569). +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 @@ -186,7 +192,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 +373,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 02e44bc..a399c1e 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"); @@ -593,3 +597,22 @@ 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", true], + ["", true], + ["#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); +}); From a6edea3f7989c0d4dc01d9a2e892f9f75053e11e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:00:11 -0400 Subject: [PATCH 2/3] test(pr-review-cli): a cost decision from an earlier head covers a later one #9436's case: the owner lifts the cost hold at head A, a base-only merge makes head B, and the review holds for cost again at B. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/apply-review-hold.test.cjs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index a399c1e..18684ab 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -616,3 +616,24 @@ test("a decision names the pull request as # the way merge-holds reads i ]) 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, []); +}); From 5c726a1e830c8251a0a4c8e261de576fe7e5f78b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:05:10 -0400 Subject: [PATCH 3/3] fix(pr-review-cli): match #9569's # exclusions and Refs-number check claude-code-plugins#9569 (head 6c3b6ae9) now also refuses `##` and `&#`, and takes a Refs target only when the issue read answers that same number and is not a pull request. The hold step matches both. Co-Authored-By: Claude Opus 5.5 --- .github/actions/apply-review-hold/hold.cjs | 13 ++++++++----- .github/scripts/apply-review-hold.test.cjs | 20 +++++++++++++++++--- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs index 2b673c2..2195a14 100644 --- a/.github/actions/apply-review-hold/hold.cjs +++ b/.github/actions/apply-review-hold/hold.cjs @@ -133,11 +133,12 @@ function linkedIssue(pull, owner, repo) { } // Whether `text` names pull request `number` as `#`: not part of a -// longer number, and not after a letter, digit, `_`, `/`, `.` or `-`, so -// `owner/repo#` does not count (claude-code-plugins -// .github/scripts/pr-merge/merge-holds.mjs namesPull, from #9569). +// 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( + new RegExp(`(?:^|[^A-Za-z0-9_#&/.-])#${number}(?:[^0-9]|$)`, "u").test( String(text ?? ""), ); @@ -179,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, { diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index 18684ab..09c024e 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -44,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, @@ -58,6 +59,7 @@ function fakeGithub({ }, edited = new Set(), pullNumbers = new Set([7]), + misnumbered = new Set(), }) { const calls = []; const refuse = (name) => () => { @@ -107,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: {} } @@ -554,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", { @@ -604,8 +618,8 @@ test("a decision names the pull request as # the way merge-holds reads i ["approved for #7.", true], ["#7", true], ["(#7)", true], - ["##7", true], - ["", true], + ["##7", false], + ["", false], ["#70", false], ["#77", false], ["o/r#7", false],