From 03007c0a40ca5f5783a5a9e3f2a7263f8c049628 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 12:15:48 -0400 Subject: [PATCH] fix(pr-review-cli): hold only for a real charge or a critical finding Owner decisions Q52 and Q53 (2026-10-24): only a real security issue or a real charge stops a pull request for a person. - hold-paths no longer holds. The compose step lists its globs in the prompt as TRUSTED PATHS for extra security scrutiny, and the hold step neither reads hold-paths nor lists the changed files. - A model trust-gate criterion holds nothing; cost and critical-security still hold. trust-gate stays in the schema and extract allowlist so a review skill that still returns it is not dropped as malformed. - The prompt's hold classes replace the review command's own: cost holds only when the change can cause a real charge (a paid plan, extra usage, auto-reload or a payment method; Azure resources or deploys that bill; GitHub billing). Spend from prepaid API credits or subscription usage is one summary line, not a hold, an inline comment or a finding. The cost-decision carry logic is unchanged. Co-Authored-By: Claude Opus 5.5 --- .github/actions/apply-review-hold/action.yml | 6 +- .github/actions/apply-review-hold/hold.cjs | 121 ++----------- .github/scripts/apply-review-hold.test.cjs | 168 ++++++++---------- .../scripts/pr-review-cli-api-calls.test.cjs | 118 +++++------- .github/scripts/pr-review-cli.test.cjs | 103 ++++++++++- .github/workflows/pr-review-cli.yml | 88 ++++++--- README.md | 12 +- 7 files changed, 319 insertions(+), 297 deletions(-) diff --git a/.github/actions/apply-review-hold/action.yml b/.github/actions/apply-review-hold/action.yml index 18ce8a04..b38a0720 100644 --- a/.github/actions/apply-review-hold/action.yml +++ b/.github/actions/apply-review-hold/action.yml @@ -2,9 +2,9 @@ name: apply-review-hold description: >- pr-review-cli's hold step, with no model: holds the pull request for a person when the review of the leg that counts (the subscription job's when - it ran, else the key job's) returned a hold, or when a changed file matches - HOLD_PATHS, which adds criterion trust-gate whatever the review says. A hold - adds HOLD_LABEL and posts one `claude-lane-hold` marker comment with the + it ran, else the key job's) returned a hold with criterion cost or + critical-security. A trust-gate criterion holds nothing. A hold adds + HOLD_LABEL and posts one `claude-lane-hold` marker comment with the reason. It only adds: it never removes or replaces a label. A failed write fails the step. hold.cjs documents the env it reads. diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs index ebde7964..336ec924 100644 --- a/.github/actions/apply-review-hold/hold.cjs +++ b/.github/actions/apply-review-hold/hold.cjs @@ -1,60 +1,26 @@ // pr-review-cli's hold step. No model runs here. It reads, from env: -// HOLD_LABEL, HOLD_PATHS (newline-separated globs), LANE_ID, PR_NUMBER, -// HEAD_SHA, BASE_REF (the pull request's base branch), PUBLISH_GATE, PR_FILES (the scope step's file list), -// TRUSTED_AUTHOR_IDS (the people whose cost decision counts), and the -// legs' KEY_* and SUB_* outputs that publish-review's judge reads. It only adds: the label and one marker -// comment. Every write goes through issues.addLabels or issues.createComment. +// HOLD_LABEL, LANE_ID, PR_NUMBER, HEAD_SHA, BASE_REF (the pull request's base +// branch), PUBLISH_GATE, TRUSTED_AUTHOR_IDS (the people whose cost decision +// counts), and the legs' KEY_* and SUB_* outputs that publish-review's judge +// reads. It only adds: the label and one marker comment. Every write goes +// through issues.addLabels or issues.createComment. "use strict"; -const { - judge, - loadGate, - scan, - scopedFiles, - stripHtml, -} = require("../publish-review/publish.cjs"); +const { judge, loadGate, scan, stripHtml } = require("../publish-review/publish.cjs"); -const CRITERIA = ["cost", "trust-gate", "critical-security"]; +// The criteria that stop a pull request: a real-charge cost and a CRITICAL +// security finding. A trusted path (the review's trust-gate, or hold-paths) +// is a reason for extra security scrutiny, never a stop by itself. +const CRITERIA = ["cost", "critical-security"]; const MAX_REASON = 2000; -const MAX_LISTED = 20; - -// The same conversion as pr-review-cli.yml's scope step (a test holds them -// equal): `**/` any directories, `**` anything, `*` and `?` within one name. -const globToRegExp = (glob) => { - let out = ""; - for (let i = 0; i < glob.length; i += 1) { - if (glob.startsWith("**/", i)) { - out += "(?:.*/)?"; - i += 2; - } else if (glob.startsWith("**", i)) { - out += ".*"; - i += 1; - } else if (glob[i] === "*") { - out += "[^/]*"; - } else if (glob[i] === "?") { - out += "[^/]"; - } else { - out += glob[i].replace(/[.+^$(){}|[\]\\]/u, "\\$&"); - } - } - return new RegExp(`^${out}$`, "u"); -}; - -const patterns = (text) => - String(text ?? "") - .split("\n") - .map((line) => line.trim()) - .filter(Boolean) - .map(globToRegExp); - -const names = (file) => - [file.filename, file.previous_filename].filter(Boolean); // The hold the counted leg's checked review returned, or null. -function modelHold(env) { +function modelHold(env, core) { const verdict = judge(env); const hold = verdict.ok ? verdict.review.hold : undefined; if (!hold || !Array.isArray(hold.criteria)) return null; + if (hold.criteria.includes("trust-gate")) + core.info(`${env.LANE_ID}: the review returned trust-gate; trust-gate is not a hold.`); const criteria = hold.criteria.filter((c) => CRITERIA.includes(c)); if (criteria.length === 0) return null; return { @@ -350,44 +316,6 @@ async function carryDecision(github, owner, repo, env, prComments, decision) { : { carry: false, why: `the net diff against ${base} differs from the decided head ${decided}` }; } -// trust-gate when a changed file, or a renamed file's old path, matches a -// hold path, or when GitHub listed fewer files than the pull request changed. -// The list is the scope step's at the head (PR_FILES); without one it is read. -async function pathHold(github, owner, repo, number, globs, env) { - if (globs.length === 0) return null; - const scoped = scopedFiles(env); - const files = scoped - ? scoped.files.map((f) => ({ filename: f.n, previous_filename: f.p })) - : await github.paginate(github.rest.pulls.listFiles, { - owner, - repo, - pull_number: number, - per_page: 100, - }); - const matched = [ - ...new Set( - files.flatMap(names).filter((name) => globs.some((re) => re.test(name))), - ), - ]; - if (matched.length > 0) { - // File names are the author's text: no backticks, line breaks or mentions. - const shown = matched - .slice(0, MAX_LISTED) - .map((name) => `\`${name.replace(/[`\r\n]/gu, "?").replaceAll("@", "@\u200b")}\``); - if (matched.length > shown.length) - shown.push(`and ${matched.length - shown.length} more`); - return `Changed files match hold-paths: ${shown.join(", ")}.`; - } - const changed = scoped - ? scoped.changed - : ((await github.rest.pulls.get({ owner, repo, pull_number: number })).data - .changed_files ?? 0); - if (files.length < changed) { - return `GitHub listed ${files.length} of the pull request's ${changed} changed files, so hold-paths could not be checked against every file.`; - } - return null; -} - async function run({ github, context, core, env = process.env }) { const label = env.HOLD_LABEL ?? ""; if (label === "") { @@ -407,7 +335,7 @@ async function run({ github, context, core, env = process.env }) { const found = new Set(); const reasons = []; - const model = modelHold(env); + const model = modelHold(env, core); // The pull request's comments, read once: for the cost decision and for // the marker this step posts. let prComments; @@ -441,27 +369,7 @@ async function run({ github, context, core, env = process.env }) { for (const c of model.criteria) found.add(c); reasons.push(model.reason); } - // A listing that fails holds too, so a model hold is never lost to it; - // the error still fails the step after the writes. const errors = []; - let byPath; - try { - byPath = await pathHold( - github, - owner, - repo, - number, - patterns(env.HOLD_PATHS), - env, - ); - } catch (error) { - errors.push(`could not list the changed files: ${error.message}`); - byPath = "The changed files could not be listed, so hold-paths could not be checked."; - } - if (byPath) { - found.add("trust-gate"); - reasons.push(byPath); - } const criteria = CRITERIA.filter((c) => found.has(c)); core.setOutput("hold", criteria.length > 0 ? "true" : "false"); core.setOutput("criteria", criteria.join(",")); @@ -529,7 +437,6 @@ async function run({ github, context, core, env = process.env }) { module.exports = { CRITERIA, - globToRegExp, linkedIssue, modelHold, namesPull, diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index 01638ceb..82dcb9ea 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -9,13 +9,8 @@ const fs = require("node:fs"); const path = require("node:path"); const test = require("node:test"); -const { - globToRegExp, - namesPull, - run, -} = require("../actions/apply-review-hold/hold.cjs"); +const { namesPull, run } = require("../actions/apply-review-hold/hold.cjs"); const { makeTemporaryDirectory } = require("./temporary-directories.cjs"); -const { parseWorkflow } = require("./workflow-yaml.cjs"); const root = makeTemporaryDirectory("apply-review-hold-"); const gate = path.join(root, "publish-gate.mjs"); @@ -274,51 +269,69 @@ test("the review's hold adds the label and one marker comment with the reason", assert.equal(writes.length, 2); }); -test("a changed file matching hold-paths adds trust-gate whatever the review says", async () => { +// Owner decision Q52: a trusted path is a reason for extra security +// scrutiny, never a stop by itself. Only the security review's CRITICAL +// finding (critical-security) or a real-charge cost hold stops a pull request. +test("a changed file matching hold-paths holds nothing and reads no file list", async () => { for (const [file, failed] of [ ["CODEOWNERS", false], [".github/CODEOWNERS", false], ["vendor/x/.gitmodules", false], [".github/standards/trusted-actors/trusted-actors.json", false], [{ filename: "moved.txt", previous_filename: ".gitmodules" }, false], - // A failed or incomplete review still holds by path. [".github/actions/check-trusted-trigger/action.yml", true], ]) { const env = failed ? baseEnv({ KEY_FAILED: "true", KEY_CLASS: "timeout", KEY_REVIEW: "" }) : baseEnv({ KEY_REVIEW: review({ hold: undefined }) }); - const { core, error, writes } = await apply(env, { + const { core, error, writes, github } = await apply(env, { files: ["src/a.js", file], }); assert.equal(error, undefined, JSON.stringify(file)); - assert.deepEqual(core.outputs, { hold: "true", criteria: "trust-gate" }); - assert.equal(writes[0][0], "addLabels"); - assert.match(writes[1][1].body, /criteria=trust-gate -->/u); - assert.match(writes[1][1].body, /Changed files match hold-paths/u); + assert.deepEqual(core.outputs, { hold: "false", criteria: "" }); + assert.deepEqual(writes, []); + assert.deepEqual(github.calls, [], JSON.stringify(file)); } - // File names cannot break out of their code span or mention anyone. - const { writes } = await apply(baseEnv(), { - files: ["a`b@team/CODEOWNERS"], - }); - assert.match(writes[1][1].body, /`a\?b@\u200bteam\/CODEOWNERS`/u); }); -test("a model hold and a path match combine their criteria", async () => { - const { core } = await apply( +test("a model trust-gate criterion alone holds nothing; the others still hold", async () => { + const alone = await apply( + baseEnv({ + KEY_REVIEW: review({ + hold: { criteria: ["trust-gate"], reason: "Touches CODEOWNERS." }, + }), + }), + { files: ["CODEOWNERS"] }, + ); + assert.equal(alone.error, undefined); + assert.deepEqual(alone.core.outputs, { hold: "false", criteria: "" }); + assert.deepEqual(alone.writes, []); + assert.match(alone.core.infos.join("\n"), /trust-gate is not a hold/u); + const security = await apply( + baseEnv({ + KEY_REVIEW: review({ + hold: { criteria: ["trust-gate", "critical-security"], reason: "R" }, + }), + }), + { files: ["CODEOWNERS"] }, + ); + assert.equal(security.core.outputs.criteria, "critical-security"); + assert.match(security.writes[1][1].body, /criteria=critical-security -->/u); + const cost = await apply( baseEnv({ KEY_REVIEW: review({ hold: { criteria: ["cost"], reason: "R" } }), }), { files: ["CODEOWNERS"] }, ); - assert.equal(core.outputs.criteria, "cost,trust-gate"); + assert.equal(cost.core.outputs.criteria, "cost"); }); -test("a file list shorter than the pull request's changed files holds", async () => { +test("a file list shorter than the pull request's changed files holds nothing", async () => { const { core } = await apply(baseEnv(), { files: ["src/a.js"], changedFiles: 3001, }); - assert.equal(core.outputs.criteria, "trust-gate"); + assert.deepEqual(core.outputs, { hold: "false", criteria: "" }); }); test("the subscription leg's review counts when it ran", async () => { @@ -337,47 +350,49 @@ test("the subscription leg's review counts when it ran", async () => { }); test("a failed label write fails the step and the marker is still tried", async () => { - const { error, github } = await apply(baseEnv(), { - files: ["CODEOWNERS"], - fail: { addLabels: true }, - }); + const { error, github } = await apply( + baseEnv({ + KEY_REVIEW: review({ + hold: { criteria: ["critical-security"], reason: "R" }, + }), + }), + { + fail: { addLabels: true }, + }, + ); assert.match(error.message, /could not add the needs-human label/u); assert.ok(github.calls.some(([name]) => name === "createComment")); }); -test("a failed comment or file listing fails the step", async () => { - const comment = await apply(baseEnv(), { - files: ["CODEOWNERS"], - fail: { createComment: true }, - }); - assert.match(comment.error.message, /could not post the hold comment/u); - // A failed listing still holds with trust-gate, then fails the step. - const listing = await apply( +test("a failed comment fails the step", async () => { + const comment = await apply( baseEnv({ - KEY_REVIEW: review({ hold: { criteria: ["cost"], reason: "R" } }), + KEY_REVIEW: review({ + hold: { criteria: ["critical-security"], reason: "R" }, + }), }), - { fail: { listFiles: true } }, - ); - assert.match( - listing.error.message, - /could not list the changed files: HTTP 502/u, - ); - assert.equal(listing.core.outputs.criteria, "cost,trust-gate"); - assert.deepEqual( - listing.writes.map(([name]) => name), - ["addLabels", "createComment"], + { + fail: { createComment: true }, + }, ); + assert.match(comment.error.message, /could not post the hold comment/u); }); test("a rerun posts no second marker for the same head and criteria", async () => { - const marker = ``; - const { writes } = await apply(baseEnv(), { - files: ["CODEOWNERS"], - comments: [ - { user: { login: "someone" }, body: "x" }, - { user: { login: "github-actions[bot]" }, body: `${marker}\nwhy` }, - ], - }); + const marker = ``; + const { writes } = await apply( + baseEnv({ + KEY_REVIEW: review({ + hold: { criteria: ["critical-security"], reason: "R" }, + }), + }), + { + comments: [ + { user: { login: "someone" }, body: "x" }, + { user: { login: "github-actions[bot]" }, body: `${marker}\nwhy` }, + ], + }, + ); assert.deepEqual( writes.map(([name]) => name), ["addLabels"], @@ -408,28 +423,6 @@ test("the hold step never names a label removal or replacement API", () => { ); }); -test("hold.cjs converts globs exactly as the scope step does", () => { - const workflow = parseWorkflow( - fs.readFileSync( - path.join(__dirname, "..", "workflows", "pr-review-cli.yml"), - "utf8", - ), - ); - const scope = workflow.jobs["review-key"].steps.find((s) => s.id === "scope") - .with.script; - const body = (text) => - /const globToRegExp = \(glob\) => \{[\s\S]*?\n\s*return new RegExp\(`\^\$\{out\}\$`, "u"\);\n\s*\};/u - .exec(text)[0] - .replace(/^\s+/gmu, ""); - const source = fs.readFileSync( - path.join(__dirname, "..", "actions", "apply-review-hold", "hold.cjs"), - "utf8", - ); - assert.equal(body(source), body(scope)); - assert.ok(globToRegExp("**/CODEOWNERS").test("CODEOWNERS")); - assert.ok(!globToRegExp("**/CODEOWNERS").test("CODEOWNERS.md")); -}); - test("a warn-only hit in the reason posts it with a warning", async () => { const { writes, core } = await apply( baseEnv({ @@ -497,11 +490,11 @@ test("a cost decision on the pull request keeps the cost hold from coming back", assert.equal(core.outputs.hold, "false"); assert.deepEqual(writes, []); // The lookup's reads, the pull request's comments and the decision's edit - // check, then the marker's edit check and the deletion read, then - // hold-paths' own file list: the head is the decided head, so no compare. + // check, then the marker's edit check and the deletion read: the head is + // the decided head, so no compare. assert.deepEqual( github.calls.map(([name]) => name), - ["listComments", "graphql", "graphql", "graphql", "listFiles", "get"], + ["listComments", "graphql", "graphql", "graphql"], ); assert.deepEqual(github.calls[3][1], { owner: "o", name: "r", number: 7 }); assert.deepEqual( @@ -534,8 +527,6 @@ test("a cost decision on the Refs issue that names the pull request keeps the co "graphql", "graphql", "graphql", - "listFiles", - "get", ], ); }); @@ -648,18 +639,17 @@ test("an edited, quoted, older or non-cost decision still holds", async () => { } }); -test("a trust-gate hold still holds when a cost decision exists", async () => { +test("a carried cost decision leaves only critical-security to hold", async () => { const both = await apply(costEnv(["cost", "trust-gate"]), { comments: [marker(), decision()], }); - assert.equal(both.core.outputs.hold, "true"); - assert.equal(both.core.outputs.criteria, "trust-gate"); + assert.equal(both.core.outputs.hold, "false"); const byPath = await apply(costEnv(), { comments: [marker(), decision()], files: ["CODEOWNERS"], }); - assert.equal(byPath.core.outputs.criteria, "trust-gate"); - assert.equal(byPath.writes.length, 2); + assert.equal(byPath.core.outputs.hold, "false"); + assert.deepEqual(byPath.writes, []); const security = await apply(costEnv(["cost", "critical-security"]), { comments: [marker(), decision()], }); @@ -667,7 +657,7 @@ test("a trust-gate hold still holds when a cost decision exists", async () => { }); test("a hold with no cost criterion reads no cost decision", async () => { - const { github } = await apply(costEnv(["trust-gate"]), { + const { github } = await apply(costEnv(["critical-security"]), { comments: [decision()], }); assert.ok(!github.calls.some(([name]) => name === "graphql")); @@ -924,7 +914,7 @@ test("the carry check reads one deletion list, at most two compares and nothing const { github } = await carryCase(atDecided); const names = github.calls.map(([name]) => name); // The pull request's comments, the decision's and the marker's edit - // checks, the deletion read, two compares, then hold-paths' own file list. + // checks, the deletion read and two compares. assert.deepEqual(names, [ "listComments", "graphql", @@ -932,8 +922,6 @@ test("the carry check reads one deletion list, at most two compares and nothing "graphql", "compare", "compare", - "listFiles", - "get", ]); }); diff --git a/.github/scripts/pr-review-cli-api-calls.test.cjs b/.github/scripts/pr-review-cli-api-calls.test.cjs index c1fd97dc..0834e15c 100644 --- a/.github/scripts/pr-review-cli-api-calls.test.cjs +++ b/.github/scripts/pr-review-cli-api-calls.test.cjs @@ -464,6 +464,12 @@ const INLINE = { }; const NONE = { findings: 0, summary: "Nothing found.", comments: [] }; const A = { filename: "a.js", status: "modified", patch: PATCH, changes: 3 }; +const CODEOWNERS = { + filename: "CODEOWNERS", + status: "modified", + patch: "@@ -1 +1 @@\n-a\n+b", + changes: 2, +}; const marker = { id: 41, user: { login: BOT }, @@ -515,20 +521,21 @@ const SHAPES = { }, review: NONE, }, - "a whole review that holds on a hold-paths file": { + "a whole review that changes a hold-paths file and holds nothing": { action: "opened", - prFiles: [ - A, - { - filename: "CODEOWNERS", - status: "modified", - patch: "@@ -1 +1 @@\n-a\n+b", - changes: 2, - }, - ], + prFiles: [A, CODEOWNERS], compares: { [`main...${HEAD}`]: { merge_base_commit: { sha: MB_OLD } } }, review: INLINE, }, + "a whole review that returns a critical-security hold": { + action: "opened", + prFiles: [A, CODEOWNERS], + compares: { [`main...${HEAD}`]: { merge_base_commit: { sha: MB_OLD } } }, + review: { + ...INLINE, + hold: { criteria: ["critical-security"], reason: "A CRITICAL finding." }, + }, + }, }; test("each run shape spends the requests pinned here, reading the file list once", async () => { @@ -556,7 +563,8 @@ test("each run shape spends the requests pinned here, reading the file list once "a whole review with no inline comment": 6, "an incremental review with an inline comment": 10, "a no-change skip after a base-branch merge": 8, - "a whole review that holds on a hold-paths file": 11, + "a whole review that changes a hold-paths file and holds nothing": 8, + "a whole review that returns a critical-security hold": 11, }); }); @@ -631,7 +639,7 @@ test("the scope step's hunk ranges give the lines commentableLines reads from th assert.equal(publishReview.hunkLines(["1,2 x"]), null); }); -test("publish and the hold read the file list themselves without a usable scope list", async () => { +test("publish reads the file list itself without a usable scope list", async () => { const good = JSON.stringify({ head: HEAD, changed: 1, @@ -661,63 +669,35 @@ test("publish and the hold read the file list themselves without a usable scope } }); -test("without the scope list a hold-paths file still holds, from the hold's own read", async () => { - const api = fakeApi({ - prFiles: [{ filename: "CODEOWNERS" }], - }); - const outputs = {}; - await applyHold.run({ - github: api.github, - context: { repo: { owner: "o", repo: "r" } }, - core: quietCore(outputs), - env: { - HOLD_LABEL: "needs-human", - HOLD_PATHS: "**/CODEOWNERS", - LANE_ID: "claude-review", - PR_NUMBER: "7", - HEAD_SHA: HEAD, - PR_FILES: "", - SUB_RESULT: "skipped", - KEY_PRESENT: "true", - KEY_FAILED: "false", - KEY_CLASS: "", - KEY_VERDICT: "complete", - KEY_REVIEW: JSON.stringify(NONE), - }, - }); - assert.equal(outputs.hold, "true"); - assert.equal(api.calls.filter((c) => c === "pulls.listFiles").length, 1); -}); - -test("a scope list shorter than the pull request's changed files holds", async () => { - const api = fakeApi({ prFiles: [] }); - const outputs = {}; - await applyHold.run({ - github: api.github, - context: { repo: { owner: "o", repo: "r" } }, - core: quietCore(outputs), - env: { - HOLD_LABEL: "needs-human", - HOLD_PATHS: "**/CODEOWNERS", - LANE_ID: "claude-review", - PR_NUMBER: "7", - HEAD_SHA: HEAD, - PR_FILES: JSON.stringify({ - head: HEAD, - changed: 3001, - files: [{ n: "a.js" }], - }), - SUB_RESULT: "skipped", - KEY_PRESENT: "true", - KEY_FAILED: "false", - KEY_CLASS: "", - KEY_VERDICT: "complete", - KEY_REVIEW: JSON.stringify(NONE), - }, - }); - assert.equal(outputs.hold, "true"); - assert.ok(!api.calls.includes("pulls.listFiles")); - assert.ok(!api.calls.includes("pulls.get")); +test("the hold reads no file list, with or without the scope list", async () => { + for (const PR_FILES of [ + "", + JSON.stringify({ head: HEAD, changed: 3001, files: [{ n: "CODEOWNERS" }] }), + ]) { + const api = fakeApi({ prFiles: [{ filename: "CODEOWNERS" }] }); + const outputs = {}; + await applyHold.run({ + github: api.github, + context: { repo: { owner: "o", repo: "r" } }, + core: quietCore(outputs), + env: { + HOLD_LABEL: "needs-human", + HOLD_PATHS: "**/CODEOWNERS", + LANE_ID: "claude-review", + PR_NUMBER: "7", + HEAD_SHA: HEAD, + PR_FILES, + SUB_RESULT: "skipped", + KEY_PRESENT: "true", + KEY_FAILED: "false", + KEY_CLASS: "", + KEY_VERDICT: "complete", + KEY_REVIEW: JSON.stringify(NONE), + }, + }); + assert.equal(outputs.hold, "false"); + assert.deepEqual(api.calls, []); + } }); test("the scope step passes on no file list larger than one environment string", async () => { diff --git a/.github/scripts/pr-review-cli.test.cjs b/.github/scripts/pr-review-cli.test.cjs index 92fd0303..862ef032 100644 --- a/.github/scripts/pr-review-cli.test.cjs +++ b/.github/scripts/pr-review-cli.test.cjs @@ -719,6 +719,106 @@ test("the schema offers the hold criteria the extract step accepts, in both lane } }); +async function compose(env) { + const dir = makeTemporaryDirectory("pr-review-cli-compose-"); + const outputs = {}; + const values = { + LANE: "security", + MODEL: "sonnet", + EFFORT: "high", + MAX_TURNS: "30", + MODEL_ARGS: "", + CLI_VERSION: "latest", + REPOSITORY: "o/r", + PR_NUMBER: "7", + HEAD_SHA: "a".repeat(40), + SCOPE_NOTE: "", + FULL_DIFF_NOTE: "", + CONTEXT_NOTE: "", + HOLD_PATHS: "", + RUNNER_TEMP: dir, + ...env, + }; + const saved = Object.fromEntries( + Object.keys(values).map((k) => [k, process.env[k]]), + ); + Object.assign(process.env, values); + try { + await new AsyncFunction( + "require", + "core", + step(key, "compose").with.script, + )(require, { + setOutput: (k, v) => { + outputs[k] = v; + }, + setFailed: (m) => { + throw new Error(m); + }, + }); + } finally { + for (const [k, v] of Object.entries(saved)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + } + return fs.readFileSync(outputs["prompt-file"], "utf8"); +} + +// Owner decisions Q52 and Q53 (2026-10-24): only a real security issue or a +// real charge stops a pull request. A trusted path gets extra security +// scrutiny; spend from prepaid credits or subscription usage is a summary line. +test("the prompt sends hold-paths to extra security scrutiny and holds cost only for a real charge, in both lanes", async () => { + for (const job of [key, subscription]) { + assert.equal( + step(job, "compose").env.HOLD_PATHS, + `\${{ inputs.hold-paths }}`, + ); + assert.equal( + step(job, "compose").with.script, + step(key, "compose").with.script, + ); + } + const prompt = await compose({ + HOLD_PATHS: "**/CODEOWNERS\n\n .github/standards/trusted-actors/** \n", + }); + assert.match( + prompt, + /TRUSTED PATHS: give every changed file matching one of these globs extra security scrutiny[^\n]*\n- \*\*\/CODEOWNERS\n- \.github\/standards\/trusted-actors\/\*\*\n/u, + ); + const flat = prompt.replace(/\n +/gu, " "); + assert.match( + flat, + /A trusted-path match alone is never a hold: do not return trust-gate\./u, + ); + assert.match( + flat, + /replace the review command's own "Hold for a person" classes/u, + ); + for (const charge of [ + "enabling or changing a paid plan, extra usage, auto-reload or a payment method", + "Azure resources or deploys that bill", + "GitHub billing (paid seats, paid Actions minutes, larger runners, Codespaces, storage)", + ]) + assert.ok(prompt.replace(/\n +/gu, " ").includes(charge), charge); + for (const spend of [ + "turn limits", + "timeouts", + "model or effort choice", + "more or longer lane runs", + "extra review runs", + "schedule frequency", + ]) + assert.ok(prompt.replace(/\n +/gu, " ").includes(spend), spend); + assert.match( + prompt.replace(/\n +/gu, " "), + /is not a hold: name what it spends in one summary line, not an inline comment, and do not count it in findings\./u, + ); + const plain = await compose({ HOLD_PATHS: "" }); + assert.doesNotMatch(plain, /TRUSTED PATHS/u); + assert.match(plain, /real charge/u); +}); + test("the review with the largest hold still fits one environment string", async () => { const largest = { findings: 25, @@ -756,9 +856,10 @@ test("the hold step runs only in comments mode with hold-label set, even after a assert.match(hold.if, /^always\(\) && /u); assert.equal(continues(hold, "comments"), false); // Every input reaches the script through env, never an expression in it. + // hold-paths reaches the prompt, never the hold: a path is not a stop. + assert.equal(hold.env.HOLD_PATHS, undefined); for (const name of [ "HOLD_LABEL", - "HOLD_PATHS", "HEAD_SHA", "KEY_REVIEW", "SUB_REVIEW", diff --git a/.github/workflows/pr-review-cli.yml b/.github/workflows/pr-review-cli.yml index 1a6ff6d6..743df5f1 100644 --- a/.github/workflows/pr-review-cli.yml +++ b/.github/workflows/pr-review-cli.yml @@ -98,10 +98,10 @@ name: pr-review-cli # - Holds (hold-label set, comments mode only): the status job adds # hold-label and one `` comment when the counted leg's checked review -# returned a hold, or when a changed file (or a renamed file's old path) -# matches hold-paths, which adds trust-gate whatever the model says. The -# file list is the scope step's at the head; without it (scope failed, -# the head moved, or the list was too large) the step reads it. It +# returned a hold with criterion cost (a real charge, as the compose +# step's prompt defines it) or critical-security. A trust-gate criterion +# holds nothing: hold-paths goes to the prompt as paths for extra +# security scrutiny, and a match is never a stop by itself. It # only adds, through the same pull-requests: write grant, and fails the # job when a write fails. A cost criterion is not added again when a # person in allowed-author-ids recorded an unedited comment opening @@ -225,15 +225,16 @@ on: default: "" hold-label: description: >- - The label the status job adds when the review or hold-paths holds - the pull request for a person. Empty turns holds off. + The label the status job adds when the review holds the pull + request for a person: a real-charge cost or a CRITICAL security + finding. Empty turns holds off. type: string default: "" hold-paths: description: >- - Newline-separated path globs, as docs-only-paths. A changed file - matching one holds the pull request with criterion trust-gate, - whatever the review says. Used only when hold-label is set. + Newline-separated path globs, as docs-only-paths. The prompt asks + the review to give a changed file matching one extra security + scrutiny. A match never holds the pull request by itself. type: string default: "" outputs: @@ -241,7 +242,7 @@ on: description: "`true` when this run held the pull request, else `false`." value: ${{ jobs.status.outputs.hold }} hold-criteria: - description: The hold's criteria, comma-separated (cost, trust-gate, critical-security), or empty. + description: The hold's criteria, comma-separated (cost, critical-security), or empty. value: ${{ jobs.status.outputs.hold-criteria }} secrets: CLAUDE_API_KEY: @@ -593,8 +594,8 @@ jobs: }; }; - // The file list at the head, for the status job's anchor check and - // hold-paths, so neither reads it again: each name, a rename's old + // The file list at the head, for the status job's anchor check, so + // it does not read it again: each name, a rename's old // name, and each patch's hunk ranges (" ", as in its @@ // line). Dropped when it would not fit one environment string. const writePrFiles = (files, changed) => { @@ -894,6 +895,7 @@ jobs: SCOPE_NOTE: ${{ steps.scope.outputs.note }} FULL_DIFF_NOTE: ${{ steps.full-diff.outputs.note }} CONTEXT_NOTE: ${{ steps.pr-context.outputs.note }} + HOLD_PATHS: ${{ inputs.hold-paths }} with: script: | const fs = require("node:fs"); @@ -970,6 +972,12 @@ jobs: }, required: ["findings", "summary", "comments"], }; + // Owner decisions Q52 and Q53 (2026-10-24): only a real security + // issue or a real charge stops a pull request for a person. + const trusted = String(env.HOLD_PATHS ?? "") + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); const prompt = [ `REPO: ${env.REPOSITORY}`, `PR NUMBER: ${env.PR_NUMBER}`, @@ -977,6 +985,12 @@ jobs: env.SCOPE_NOTE, env.FULL_DIFF_NOTE, env.CONTEXT_NOTE, + ...(trusted.length > 0 + ? [ + "TRUSTED PATHS: give every changed file matching one of these globs extra security scrutiny (read its whole change and what it gates):", + ...trusted.map((glob) => `- ${glob}`), + ] + : []), "", `Invoke /${skill} now and follow its instructions exactly for this pull request.`, "The REPO / PR NUMBER / HEAD SHA header above is authoritative context for the command.", @@ -994,8 +1008,17 @@ jobs: ` https://github.com/${env.REPOSITORY}/blob/${env.HEAD_SHA}/#L`, "- findings: the number of findings you report, comments included (0 when none).", "- summary: your review summary in Markdown.", - "- hold: only when the review command says to hold the pull request for a person: criteria", - " (cost, trust-gate or critical-security) and reason. Omit it otherwise.", + "- hold: criteria and reason, only for the classes below, which replace the review command's own", + " \"Hold for a person\" classes. Omit it otherwise.", + " - critical-security: as the review command defines it (a CRITICAL security finding).", + " - cost: only when the change can cause a real charge: enabling or changing a paid plan, extra", + " usage, auto-reload or a payment method; Azure resources or deploys that bill; or", + " GitHub billing (paid seats, paid Actions minutes, larger runners, Codespaces, storage).", + " A change that only spends prepaid API credits or subscription usage (turn limits, timeouts,", + " model or effort choice, more or longer lane runs, extra review runs, schedule frequency)", + " is not a hold: name what it spends in one summary line, not an inline comment, and do not", + " count it in findings.", + " A trusted-path match alone is never a hold: do not return trust-gate.", ] .filter((line) => line !== undefined) .join("\n"); @@ -1643,8 +1666,8 @@ jobs: }; }; - // The file list at the head, for the status job's anchor check and - // hold-paths, so neither reads it again: each name, a rename's old + // The file list at the head, for the status job's anchor check, so + // it does not read it again: each name, a rename's old // name, and each patch's hunk ranges (" ", as in its @@ // line). Dropped when it would not fit one environment string. const writePrFiles = (files, changed) => { @@ -1944,6 +1967,7 @@ jobs: SCOPE_NOTE: ${{ steps.scope.outputs.note }} FULL_DIFF_NOTE: ${{ steps.full-diff.outputs.note }} CONTEXT_NOTE: ${{ steps.pr-context.outputs.note }} + HOLD_PATHS: ${{ inputs.hold-paths }} with: script: | const fs = require("node:fs"); @@ -2020,6 +2044,12 @@ jobs: }, required: ["findings", "summary", "comments"], }; + // Owner decisions Q52 and Q53 (2026-10-24): only a real security + // issue or a real charge stops a pull request for a person. + const trusted = String(env.HOLD_PATHS ?? "") + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); const prompt = [ `REPO: ${env.REPOSITORY}`, `PR NUMBER: ${env.PR_NUMBER}`, @@ -2027,6 +2057,12 @@ jobs: env.SCOPE_NOTE, env.FULL_DIFF_NOTE, env.CONTEXT_NOTE, + ...(trusted.length > 0 + ? [ + "TRUSTED PATHS: give every changed file matching one of these globs extra security scrutiny (read its whole change and what it gates):", + ...trusted.map((glob) => `- ${glob}`), + ] + : []), "", `Invoke /${skill} now and follow its instructions exactly for this pull request.`, "The REPO / PR NUMBER / HEAD SHA header above is authoritative context for the command.", @@ -2044,8 +2080,17 @@ jobs: ` https://github.com/${env.REPOSITORY}/blob/${env.HEAD_SHA}/#L`, "- findings: the number of findings you report, comments included (0 when none).", "- summary: your review summary in Markdown.", - "- hold: only when the review command says to hold the pull request for a person: criteria", - " (cost, trust-gate or critical-security) and reason. Omit it otherwise.", + "- hold: criteria and reason, only for the classes below, which replace the review command's own", + " \"Hold for a person\" classes. Omit it otherwise.", + " - critical-security: as the review command defines it (a CRITICAL security finding).", + " - cost: only when the change can cause a real charge: enabling or changing a paid plan, extra", + " usage, auto-reload or a payment method; Azure resources or deploys that bill; or", + " GitHub billing (paid seats, paid Actions minutes, larger runners, Codespaces, storage).", + " A change that only spends prepaid API credits or subscription usage (turn limits, timeouts,", + " model or effort choice, more or longer lane runs, extra review runs, schedule frequency)", + " is not a hold: name what it spends in one summary line, not an inline comment, and do not", + " count it in findings.", + " A trusted-path match alone is never a hold: do not return trust-gate.", ] .filter((line) => line !== undefined) .join("\n"); @@ -2502,9 +2547,9 @@ jobs: SUB_WALL_MS: ${{ needs.review-subscription.outputs.wall-ms }} # Add-only: in comments mode with hold-label set, the label and one - # marker comment when the counted leg's review returned a hold or a - # changed file matches hold-paths, even when the review failed. It never - # removes a label. A failed write fails the job. + # marker comment when the counted leg's review returned a cost or + # critical-security hold. It never removes a label. A failed write + # fails the job. - name: Apply the hold id: hold if: always() && inputs.hold-label != '' && inputs.publish == 'comments' @@ -2514,7 +2559,6 @@ jobs: env: PUBLISH: ${{ inputs.publish }} HOLD_LABEL: ${{ inputs.hold-label }} - HOLD_PATHS: ${{ inputs.hold-paths }} TRUSTED_AUTHOR_IDS: ${{ inputs.allowed-author-ids }} LANE_ID: ${{ inputs.lane == 'security' && 'claude-security-review' || 'claude-review' }} PR_NUMBER: ${{ github.event.pull_request.number }} diff --git a/README.md b/README.md index 2a044853..aa82d55c 100644 --- a/README.md +++ b/README.md @@ -1133,15 +1133,17 @@ GitHub continues the normal weekly patching of each hosted image generation. `pull-requests: write` in both modes and, in summary mode, does not cancel in-progress runs. The workflow header carries the canonical caller and the security model. With `hold-label` set, comments mode also holds the - pull request for a person: when the review returns a `hold` (criteria - `cost`, `trust-gate` or `critical-security`, and a reason) or a changed - file matches a `hold-paths` glob (criterion `trust-gate`, whatever the - review says), the status job adds that label and one + pull request for a person: when the review returns a `hold` with criterion + `cost` (only a change that can cause a real charge, as the compose step's + prompt lists) or `critical-security`, and a reason, the status job adds + that label and one `` comment through `.github/actions/apply-review-hold`. It never removes a label, and a failed write turns the check red. The workflow outputs `hold` and `hold-criteria`. The hold needs no permission beyond - `pull-requests: write`; create the label in the repository first. The checks stay + `pull-requests: write`; create the label in the repository first. A + `hold-paths` glob never holds: the prompt asks the review to give a + matching changed file extra security scrutiny. The checks stay advisory: a caller that wants a hold enforced reads the label in its own required check. `pr-review.yml` and `pr-review-security.yml` are unchanged.