From 016ae17d4f40bf5ab929cc1be6b295818a862332 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:41:41 -0400 Subject: [PATCH 1/3] feat(pr-review-cli): add an optional hold label step The review schema gains an optional hold (criteria cost, trust-gate or critical-security, and a reason up to 2000 characters), checked by the extract step in both legs. New inputs hold-label (empty turns holds off) and hold-paths (globs; a changed file matching one adds trust-gate whatever the model says). The status job's add-only "Apply the hold" step, through the new apply-review-hold action, adds the label and one claude-lane-hold marker comment, never removes a label, and fails the job when a write fails. The workflow outputs hold and hold-criteria. Co-Authored-By: Claude Opus 5.5 --- .github/actions/apply-review-hold/action.yml | 31 ++ .github/actions/apply-review-hold/hold.cjs | 199 ++++++++++++ .github/actions/publish-review/publish.cjs | 1 + .github/scripts/apply-review-hold.test.cjs | 321 +++++++++++++++++++ .github/scripts/pr-review-cli.test.cjs | 115 +++++++ .github/workflows/pr-review-cli.yml | 137 +++++++- README.md | 16 +- 7 files changed, 815 insertions(+), 5 deletions(-) create mode 100644 .github/actions/apply-review-hold/action.yml create mode 100644 .github/actions/apply-review-hold/hold.cjs create mode 100644 .github/scripts/apply-review-hold.test.cjs diff --git a/.github/actions/apply-review-hold/action.yml b/.github/actions/apply-review-hold/action.yml new file mode 100644 index 0000000..1b84ea2 --- /dev/null +++ b/.github/actions/apply-review-hold/action.yml @@ -0,0 +1,31 @@ +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 + reason. It only adds: it never removes or replaces a label. A failed write + fails the step. hold.cjs documents the env it reads. + +outputs: + hold: + description: "`true` when the pull request is held, else `false`." + value: ${{ steps.hold.outputs.hold }} + criteria: + description: The hold's criteria, comma-separated, or empty. + value: ${{ steps.hold.outputs.criteria }} + +runs: + using: composite + steps: + - name: Apply the hold + id: hold + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + ACTION_PATH: ${{ github.action_path }} + with: + script: | + const path = require("node:path"); + const { run } = require(path.join(process.env.ACTION_PATH, "hold.cjs")); + await run({ github, context, core }); diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs new file mode 100644 index 0000000..5db0db2 --- /dev/null +++ b/.github/actions/apply-review-hold/hold.cjs @@ -0,0 +1,199 @@ +// 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, PUBLISH_GATE, 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, + stripHtml, +} = require("../publish-review/publish.cjs"); + +const CRITERIA = ["cost", "trust-gate", "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) { + const verdict = judge(env); + const hold = verdict.ok ? verdict.review.hold : undefined; + if (!hold || !Array.isArray(hold.criteria)) return null; + const criteria = hold.criteria.filter((c) => CRITERIA.includes(c)); + if (criteria.length === 0) return null; + return { + criteria, + reason: typeof hold.reason === "string" ? hold.reason : "", + }; +} + +// 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. +async function pathHold(github, owner, repo, number, globs) { + if (globs.length === 0) return null; + const files = 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) { + const shown = matched.slice(0, MAX_LISTED).map((name) => `\`${name}\``); + if (matched.length > shown.length) + shown.push(`and ${matched.length - shown.length} more`); + return `Changed files match hold-paths: ${shown.join(", ")}.`; + } + const { data } = await github.rest.pulls.get({ + owner, + repo, + pull_number: number, + }); + if (files.length < (data.changed_files ?? 0)) { + return `GitHub listed ${files.length} of the pull request's ${data.changed_files} 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 === "") { + core.setOutput("hold", "false"); + core.setOutput("criteria", ""); + return { hold: false, criteria: [] }; + } + if (!/^[^,\n]{1,50}$/u.test(label)) + throw new Error("hold-label must be one label name of 1 to 50 characters"); + if (!/^[0-9a-f]{40}$/u.test(env.HEAD_SHA ?? "")) + throw new Error("HEAD_SHA is not a commit"); + if (!/^[a-z-]+$/u.test(env.LANE_ID ?? "")) + throw new Error("LANE_ID is not a lane id"); + const { owner, repo } = context.repo; + const number = Number(env.PR_NUMBER); + const lane = env.LANE_ID; + + const found = new Set(); + const reasons = []; + const model = modelHold(env); + if (model) { + for (const c of model.criteria) found.add(c); + reasons.push(model.reason); + } + const byPath = await pathHold( + github, + owner, + repo, + number, + patterns(env.HOLD_PATHS), + ); + 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(",")); + if (criteria.length === 0) return { hold: false, criteria }; + + let reason = stripHtml(reasons.filter(Boolean).join("\n\n")).slice( + 0, + 2 * MAX_REASON, + ); + const findSecret = await loadGate(env); + const withheld = findSecret + ? scan(findSecret, [["the hold reason", reason]]).withheld + : "the publish gate could not be loaded"; + if (withheld) { + core.warning(`${lane}: the hold reason was withheld: ${withheld}.`); + reason = `The reason was withheld: ${withheld}.`; + } + + // Both writes are tried; either failing fails the step afterwards. + const errors = []; + try { + await github.rest.issues.addLabels({ + owner, + repo, + issue_number: number, + labels: [label], + }); + } catch (error) { + errors.push(`could not add the ${label} label: ${error.message}`); + } + const marker = ``; + try { + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: number, + per_page: 100, + }); + const posted = comments.some( + (c) => + c.user?.login === "github-actions[bot]" && + (c.body === marker || c.body?.startsWith(`${marker}\n`)), + ); + if (!posted) { + await github.rest.issues.createComment({ + owner, + repo, + issue_number: number, + body: [ + marker, + `**${lane}** holds this pull request for a person (${criteria.join(", ")}) at \`${env.HEAD_SHA}\`.`, + "", + reason, + "", + `claude-lane: ${lane}`, + ].join("\n"), + }); + } + } catch (error) { + errors.push(`could not post the hold comment: ${error.message}`); + } + if (errors.length > 0) throw new Error(`${lane}: ${errors.join("; ")}`); + core.notice( + `claude-lane-hold lane=${lane} sha=${env.HEAD_SHA} criteria=${criteria.join(",")} label=${label}`, + ); + return { hold: true, criteria }; +} + +module.exports = { CRITERIA, globToRegExp, modelHold, run }; diff --git a/.github/actions/publish-review/publish.cjs b/.github/actions/publish-review/publish.cjs index 3e8b59c..23bd454 100644 --- a/.github/actions/publish-review/publish.cjs +++ b/.github/actions/publish-review/publish.cjs @@ -475,6 +475,7 @@ module.exports = { commentBody, commentableLines, judge, + loadGate, parseReview, placeComments, run, diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs new file mode 100644 index 0000000..11441f6 --- /dev/null +++ b/.github/scripts/apply-review-hold.test.cjs @@ -0,0 +1,321 @@ +"use strict"; + +// The apply-review-hold action: pr-review-cli's add-only hold step. A fake +// GitHub records every call and throws on any label removal or replacement; +// a stand-in publish gate flags an Anthropic-key shape. + +const assert = require("node:assert/strict"); +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 { 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"); +fs.writeFileSync( + gate, + `export const findSecret = (text) => + /sk-ant-[A-Za-z0-9_-]{20,}/.test(text) ? ["Anthropic key", 1] : null; +`, +); +// Key-shaped, built at run time so no scanner reads a key in this file. +const FAKE_KEY = ["sk", "ant", "api03", "x".repeat(26)].join("-"); +const HEAD = "a".repeat(40); +const TRUST_PATHS = [ + ".github/standards/trusted-actors/**", + ".github/actions/check-trusted-trigger/**", + "**/CODEOWNERS", + "**/.gitmodules", +].join("\n"); + +function fakeGithub({ files = [], changedFiles, comments = [], fail = {} }) { + const calls = []; + const refuse = (name) => () => { + throw new Error(`${name} must never be called`); + }; + const github = { + calls, + paginate: async (method, params) => method(params), + rest: { + pulls: { + listFiles: (params) => { + calls.push(["listFiles", params]); + if (fail.listFiles) throw new Error("HTTP 502"); + return files.map((f) => + typeof f === "string" ? { filename: f } : f, + ); + }, + get: async (params) => { + calls.push(["get", params]); + return { data: { changed_files: changedFiles ?? files.length } }; + }, + }, + issues: { + addLabels: async (params) => { + calls.push(["addLabels", params]); + if (fail.addLabels) + throw new Error("HTTP 403 Resource not accessible"); + return { data: [] }; + }, + listComments: (params) => { + calls.push(["listComments", params]); + return comments; + }, + createComment: async (params) => { + calls.push(["createComment", params]); + if (fail.createComment) throw new Error("HTTP 500"); + return { data: {} }; + }, + removeLabel: refuse("removeLabel"), + removeAllLabels: refuse("removeAllLabels"), + setLabels: refuse("setLabels"), + deleteLabel: refuse("deleteLabel"), + updateLabel: refuse("updateLabel"), + }, + }, + }; + return github; +} + +function fakeCore() { + const core = { outputs: {}, warnings: [], notices: [] }; + core.setOutput = (k, v) => { + core.outputs[k] = v; + }; + core.warning = (m) => core.warnings.push(m); + core.notice = (m) => core.notices.push(m); + return core; +} + +const review = (extra = {}) => + JSON.stringify({ findings: 0, summary: "S", comments: [], ...extra }); + +const baseEnv = (extra = {}) => ({ + HOLD_LABEL: "needs-human", + HOLD_PATHS: TRUST_PATHS, + LANE_ID: "claude-review", + PR_NUMBER: "7", + HEAD_SHA: HEAD, + PUBLISH_GATE: gate, + SUB_RESULT: "skipped", + KEY_PRESENT: "true", + KEY_FAILED: "false", + KEY_CLASS: "", + KEY_VERDICT: "complete", + KEY_REVIEW: review(), + ...extra, +}); + +async function apply(env, options = {}) { + const github = fakeGithub(options); + const core = fakeCore(); + let error; + try { + await run({ + github, + context: { repo: { owner: "o", repo: "r" } }, + core, + env, + }); + } catch (e) { + error = e; + } + const writes = github.calls.filter(([name]) => + ["addLabels", "createComment"].includes(name), + ); + return { github, core, error, writes }; +} + +test("an empty hold-label does nothing: no read, no write, hold false", async () => { + const { github, core, error } = await apply( + baseEnv({ + HOLD_LABEL: "", + KEY_REVIEW: review({ hold: { criteria: ["cost"], reason: "R" } }), + }), + { files: ["CODEOWNERS"] }, + ); + assert.equal(error, undefined); + assert.deepEqual(github.calls, []); + assert.deepEqual(core.outputs, { hold: "false", criteria: "" }); +}); + +test("no hold from the review and no matching path writes nothing", async () => { + const { core, error, writes } = await apply(baseEnv(), { + files: ["src/a.js", "docs/CODEOWNERS.md"], + }); + assert.equal(error, undefined); + assert.deepEqual(writes, []); + assert.deepEqual(core.outputs, { hold: "false", criteria: "" }); +}); + +test("the review's hold adds the label and one marker comment with the reason", async () => { + const { core, error, writes } = await apply( + baseEnv({ + KEY_REVIEW: review({ + hold: { + criteria: ["critical-security", "cost"], + reason: "Spends money.", + }, + }), + }), + { files: ["src/a.js"] }, + ); + assert.equal(error, undefined); + assert.deepEqual(core.outputs, { + hold: "true", + criteria: "cost,critical-security", + }); + assert.deepEqual(writes[0], [ + "addLabels", + { owner: "o", repo: "r", issue_number: 7, labels: ["needs-human"] }, + ]); + const body = writes[1][1].body; + assert.equal( + body.split("\n")[0], + ``, + ); + assert.match(body, /Spends money\./u); + assert.doesNotMatch(body, /hidden/u); + assert.match(body, /\nclaude-lane: claude-review$/u); + assert.equal(writes.length, 2); +}); + +test("a changed file matching hold-paths adds trust-gate whatever the review says", 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, { + 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); + } +}); + +test("a model hold and a path match combine their criteria", async () => { + const { core } = await apply( + baseEnv({ + KEY_REVIEW: review({ hold: { criteria: ["cost"], reason: "R" } }), + }), + { files: ["CODEOWNERS"] }, + ); + assert.equal(core.outputs.criteria, "cost,trust-gate"); +}); + +test("a file list shorter than the pull request's changed files holds", async () => { + const { core } = await apply(baseEnv(), { + files: ["src/a.js"], + changedFiles: 3001, + }); + assert.equal(core.outputs.criteria, "trust-gate"); +}); + +test("the subscription leg's review counts when it ran", async () => { + const { core } = await apply( + baseEnv({ + SUB_RESULT: "success", + SUB_PRESENT: "true", + SUB_FAILED: "false", + SUB_CLASS: "", + SUB_VERDICT: "complete", + SUB_REVIEW: review({ hold: { criteria: ["cost"], reason: "R" } }), + }), + { files: ["src/a.js"] }, + ); + assert.equal(core.outputs.criteria, "cost"); +}); + +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 }, + }); + 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); + const listing = await apply(baseEnv(), { fail: { listFiles: true } }); + assert.match(listing.error.message, /HTTP 502/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` }, + ], + }); + assert.deepEqual( + writes.map(([name]) => name), + ["addLabels"], + ); +}); + +test("a credential-shaped reason is withheld but the hold still applies", async () => { + const { writes, core } = await apply( + baseEnv({ + KEY_REVIEW: review({ hold: { criteria: ["cost"], reason: FAKE_KEY } }), + }), + { files: [] }, + ); + assert.equal(writes[0][0], "addLabels"); + assert.doesNotMatch(writes[1][1].body, /sk-ant/u); + assert.match(writes[1][1].body, /withheld: Anthropic key/u); + assert.equal(core.warnings.length, 1); +}); + +test("the hold step never names a label removal or replacement API", () => { + const source = fs.readFileSync( + path.join(__dirname, "..", "actions", "apply-review-hold", "hold.cjs"), + "utf8", + ); + assert.doesNotMatch( + source, + /removeLabel|removeAllLabels|setLabels|deleteLabel|updateLabel|\bDELETE\b|\bPUT\b/u, + ); +}); + +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")); +}); diff --git a/.github/scripts/pr-review-cli.test.cjs b/.github/scripts/pr-review-cli.test.cjs index a146f0c..d6cc148 100644 --- a/.github/scripts/pr-review-cli.test.cjs +++ b/.github/scripts/pr-review-cli.test.cjs @@ -617,3 +617,118 @@ test("the extract step passes on a review no larger than one environment string assert.match(refused.verdict, /too large to pass on/u); assert.equal(refused.review, undefined); }); + +test("the extract step carries a well-formed hold and calls a malformed one incomplete", async () => { + const good = { findings: 0, summary: "S", comments: [] }; + const held = await extract({ + structured: JSON.stringify({ + ...good, + hold: { criteria: ["cost", "trust-gate"], reason: "Why." }, + }), + }); + assert.equal(held.verdict, "complete"); + assert.deepEqual(JSON.parse(held.review).hold, { + criteria: ["cost", "trust-gate"], + reason: "Why.", + }); + const none = await extract({ structured: JSON.stringify(good) }); + assert.equal("hold" in JSON.parse(none.review), false); + for (const hold of [ + null, + { criteria: [], reason: "R" }, + { criteria: ["cost", "cost"], reason: "R" }, + { criteria: ["style"], reason: "R" }, + { criteria: ["cost"], reason: "" }, + { criteria: "cost", reason: "R" }, + ]) { + const outputs = await extract({ + structured: JSON.stringify({ ...good, hold }), + }); + assert.match(outputs.verdict, /hold is malformed/u, JSON.stringify(hold)); + assert.equal(outputs.review, undefined); + } +}); + +test("the schema offers the hold criteria the extract step accepts, in both lanes", () => { + for (const job of [key, subscription]) { + const script = step(job, "compose").with.script; + assert.match( + script, + /enum: \["cost", "trust-gate", "critical-security"\]/u, + ); + assert.match( + script, + /reason: \{ type: "string", minLength: 1, maxLength: 2000 \}/u, + ); + assert.doesNotMatch( + script, + /required: \["findings", "summary", "comments", "hold"\]/u, + ); + assert.match( + step(job, "extract").with.script, + /\["cost", "trust-gate", "critical-security"\]\.includes\(c\)/u, + ); + } +}); + +test("the review with the largest hold still fits one environment string", async () => { + const largest = { + findings: 25, + summary: "s".repeat(20000), + comments: Array.from({ length: 25 }, () => ({ + ...COMMENT, + body: "b".repeat(3000), + })), + hold: { + criteria: ["cost", "trust-gate", "critical-security"], + reason: "r".repeat(2000), + }, + }; + const fits = await extract({ structured: JSON.stringify(largest) }); + assert.equal(fits.verdict, "complete"); +}); + +test("the hold step runs only in comments mode with hold-label set, even after a red publish", () => { + const hold = step(status, "hold"); + const names = status.steps.map((s) => s.name); + assert.ok( + names.indexOf("Publish the review") < names.indexOf("Apply the hold"), + ); + assert.equal(hold.uses, "$/.github/actions/apply-review-hold"); + const runs = (inputs) => Boolean(evaluate(hold.if, { inputs })); + assert.equal( + runs({ "hold-label": "needs-human", publish: "comments" }), + true, + ); + assert.equal(runs({ "hold-label": "", publish: "comments" }), false); + assert.equal( + runs({ "hold-label": "needs-human", publish: "summary" }), + false, + ); + assert.match(hold.if, /^always\(\) && /u); + assert.equal( + evaluate(hold["continue-on-error"], { inputs: { publish: "comments" } }), + false, + ); + // Every input reaches the script through env, never an expression in it. + for (const name of [ + "HOLD_LABEL", + "HOLD_PATHS", + "HEAD_SHA", + "KEY_REVIEW", + "SUB_REVIEW", + "SUB_RESULT", + ]) { + assert.ok(hold.env[name], name); + } + assert.deepEqual(status.outputs, { + hold: `\${{ steps.hold.outputs.hold || 'false' }}`, + "hold-criteria": `\${{ steps.hold.outputs.criteria }}`, + }); + assert.equal( + evaluate("steps.hold.outputs.hold || 'false'", { + steps: { hold: { outputs: {} } }, + }), + "false", + ); +}); diff --git a/.github/workflows/pr-review-cli.yml b/.github/workflows/pr-review-cli.yml index 2ab9e8a..b4261d1 100644 --- a/.github/workflows/pr-review-cli.yml +++ b/.github/workflows/pr-review-cli.yml @@ -83,6 +83,14 @@ name: pr-review-cli # `pull-requests: write` could write a marker and narrow a later # incremental review; same-repository writers already receive this # lane's secrets, so the lane trusts them. +# - 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. It +# only adds, through the same pull-requests: write grant, and fails the +# job when a write fails. The reason is model text: HTML comments are +# stripped and the publish gate withholds a credential-shaped one. # # REVIEW CADENCE: as pr-review.yml's header states, with this lane's marker: # whole pull request on open, reopen and ready; after a push, the files @@ -173,6 +181,26 @@ on: closing issues the review reads in full, as pr-review.yml's input. type: string 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. + 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. + type: string + default: "" + outputs: + hold: + 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. + value: ${{ jobs.status.outputs.hold-criteria }} secrets: CLAUDE_API_KEY: description: The role's Console API key. Tried first, in the review-key job; empty skips it. @@ -835,6 +863,20 @@ jobs: findings: { type: "integer", minimum: 0 }, summary: { type: "string", minLength: 1, maxLength: 20000 }, comments: { type: "array", maxItems: 25, items: comment }, + hold: { + type: "object", + additionalProperties: false, + properties: { + criteria: { + type: "array", + minItems: 1, + uniqueItems: true, + items: { type: "string", enum: ["cost", "trust-gate", "critical-security"] }, + }, + reason: { type: "string", minLength: 1, maxLength: 2000 }, + }, + required: ["criteria", "reason"], + }, }, required: ["findings", "summary", "comments"], }; @@ -862,6 +904,8 @@ 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.", ] .filter((line) => line !== undefined) .join("\n"); @@ -1044,12 +1088,30 @@ jobs: } // Every comment and every `manifest : flag` summary line is // a finding, so a lower count is raised to them, never trusted. + // An optional hold for a person: known criteria, each once, and a + // reason. A malformed hold makes the review incomplete. + let hold; + if (raw.hold !== undefined) { + const criteria = raw.hold?.criteria; + const reason = + typeof raw.hold?.reason === "string" ? raw.hold.reason.replace(//gu, "").trim().slice(0, 2000) : ""; + if ( + !Array.isArray(criteria) || + criteria.length === 0 || + new Set(criteria).size !== criteria.length || + !criteria.every((c) => ["cost", "trust-gate", "critical-security"].includes(c)) || + !reason + ) { + return incomplete("the review's hold is malformed"); + } + hold = { criteria, reason }; + } const flags = summary.match(/^[ \t]*(?:[-*+][ \t]+)?`?manifest \S+: flag/gmu)?.length ?? 0; const findings = Math.max(raw.findings, comments.length, flags); // The status job receives the review as one environment string, which // Linux caps at 128 KiB (MAX_ARG_STRLEN); the schema's caps keep an // ASCII review near 100 KB, and anything over that is not passed on. - const review = JSON.stringify({ findings, summary, comments }); + const review = JSON.stringify({ findings, summary, comments, ...(hold ? { hold } : {}) }); if (Buffer.byteLength(review, "utf8") > 100000) return incomplete("the review is over 100,000 bytes, too large to pass on"); core.setOutput("review", review); core.setOutput("verdict", "complete"); @@ -1732,6 +1794,20 @@ jobs: findings: { type: "integer", minimum: 0 }, summary: { type: "string", minLength: 1, maxLength: 20000 }, comments: { type: "array", maxItems: 25, items: comment }, + hold: { + type: "object", + additionalProperties: false, + properties: { + criteria: { + type: "array", + minItems: 1, + uniqueItems: true, + items: { type: "string", enum: ["cost", "trust-gate", "critical-security"] }, + }, + reason: { type: "string", minLength: 1, maxLength: 2000 }, + }, + required: ["criteria", "reason"], + }, }, required: ["findings", "summary", "comments"], }; @@ -1759,6 +1835,8 @@ 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.", ] .filter((line) => line !== undefined) .join("\n"); @@ -1941,12 +2019,30 @@ jobs: } // Every comment and every `manifest : flag` summary line is // a finding, so a lower count is raised to them, never trusted. + // An optional hold for a person: known criteria, each once, and a + // reason. A malformed hold makes the review incomplete. + let hold; + if (raw.hold !== undefined) { + const criteria = raw.hold?.criteria; + const reason = + typeof raw.hold?.reason === "string" ? raw.hold.reason.replace(//gu, "").trim().slice(0, 2000) : ""; + if ( + !Array.isArray(criteria) || + criteria.length === 0 || + new Set(criteria).size !== criteria.length || + !criteria.every((c) => ["cost", "trust-gate", "critical-security"].includes(c)) || + !reason + ) { + return incomplete("the review's hold is malformed"); + } + hold = { criteria, reason }; + } const flags = summary.match(/^[ \t]*(?:[-*+][ \t]+)?`?manifest \S+: flag/gmu)?.length ?? 0; const findings = Math.max(raw.findings, comments.length, flags); // The status job receives the review as one environment string, which // Linux caps at 128 KiB (MAX_ARG_STRLEN); the schema's caps keep an // ASCII review near 100 KB, and anything over that is not passed on. - const review = JSON.stringify({ findings, summary, comments }); + const review = JSON.stringify({ findings, summary, comments, ...(hold ? { hold } : {}) }); if (Buffer.byteLength(review, "utf8") > 100000) return incomplete("the review is over 100,000 bytes, too large to pass on"); core.setOutput("review", review); core.setOutput("verdict", "complete"); @@ -1984,10 +2080,13 @@ jobs: needs: [review-key, review-subscription] if: always() && needs.review-key.result != 'skipped' runs-on: ubuntu-24.04 - timeout-minutes: 15 + timeout-minutes: 17 permissions: contents: read - pull-requests: write # the review, its inline comments and the lane's marker comment + pull-requests: write # the review, its inline comments, the lane's marker comment and the hold label + outputs: + hold: ${{ steps.hold.outputs.hold || 'false' }} + hold-criteria: ${{ steps.hold.outputs.criteria }} steps: # lib/publish-gate.mjs from claude-code-plugins at its default # branch's tip, resolved here, into a fresh directory; it must import @@ -2060,6 +2159,36 @@ jobs: SUB_LEG_MS: ${{ needs.review-subscription.outputs.leg-ms }} 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. + - name: Apply the hold + id: hold + if: always() && inputs.hold-label != '' && inputs.publish == 'comments' + continue-on-error: ${{ inputs.publish != 'comments' }} + timeout-minutes: 2 + uses: $/.github/actions/apply-review-hold + env: + HOLD_LABEL: ${{ inputs.hold-label }} + HOLD_PATHS: ${{ inputs.hold-paths }} + LANE_ID: ${{ inputs.lane == 'security' && 'claude-security-review' || 'claude-review' }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PUBLISH_GATE: ${{ steps.gate.outputs.path }} + KEY_LANE_CLASS: ${{ needs.review-key.outputs.lane-class }} + SUB_RESULT: ${{ needs.review-subscription.result }} + KEY_PRESENT: ${{ needs.review-key.outputs.credential-present }} + KEY_FAILED: ${{ needs.review-key.outputs.review-failed }} + KEY_CLASS: ${{ needs.review-key.outputs.failure-class }} + KEY_VERDICT: ${{ needs.review-key.outputs.verdict }} + KEY_REVIEW: ${{ needs.review-key.outputs.review }} + SUB_PRESENT: ${{ needs.review-subscription.outputs.credential-present }} + SUB_FAILED: ${{ needs.review-subscription.outputs.review-failed }} + SUB_CLASS: ${{ needs.review-subscription.outputs.failure-class }} + SUB_VERDICT: ${{ needs.review-subscription.outputs.verdict }} + SUB_REVIEW: ${{ needs.review-subscription.outputs.review }} + - name: Upload the shadow report if: inputs.publish != 'comments' continue-on-error: true diff --git a/README.md b/README.md index acbe1da..0cb6723 100644 --- a/README.md +++ b/README.md @@ -710,6 +710,9 @@ consumer to audit it. - `.github/actions/publish-review` — `pr-review-cli.yml`'s publish step, with no model; internal to that workflow, which passes it everything through env (its `action.yml` describes what it does). +- `.github/actions/apply-review-hold` — `pr-review-cli.yml`'s add-only hold + step, with no model; internal to that workflow, which passes it + everything through env (its `action.yml` describes what it does). Each input's meaning and default is documented inline in the action's `inputs:` block. @@ -1125,7 +1128,18 @@ GitHub continues the normal weekly patching of each hosted image generation. lane id, and no job ever red. The caller grants `contents: read` and `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. `pr-review.yml` and `pr-review-security.yml` are + 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 + `` + 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 + 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. - `.github/workflows/intake-triage.yml` — the intake-triage lane. On `issues: [opened]`, a script step fetches the issue, its trusted comments and the From 8a90441cdcf990a1f0cf277fb19bbab0c07b8933 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:46:00 -0400 Subject: [PATCH 2/3] fix(pr-review-cli): hold when the file listing fails and quote file names safely A failed changed-file listing now adds trust-gate and still applies the hold before the step fails, so a model hold is never lost to it. Matched file names in the hold comment lose backticks and line breaks, and their mentions are broken. Co-Authored-By: Claude Opus 5.5 --- .github/actions/apply-review-hold/hold.cjs | 23 +++++++++++++--------- .github/scripts/apply-review-hold.test.cjs | 23 ++++++++++++++++++++-- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs index 5db0db2..3de6197 100644 --- a/.github/actions/apply-review-hold/hold.cjs +++ b/.github/actions/apply-review-hold/hold.cjs @@ -77,7 +77,10 @@ async function pathHold(github, owner, repo, number, globs) { ), ]; if (matched.length > 0) { - const shown = matched.slice(0, MAX_LISTED).map((name) => `\`${name}\``); + // 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(", ")}.`; @@ -117,13 +120,16 @@ async function run({ github, context, core, env = process.env }) { for (const c of model.criteria) found.add(c); reasons.push(model.reason); } - const byPath = await pathHold( - github, - owner, - repo, - number, - patterns(env.HOLD_PATHS), - ); + // 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)); + } 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); @@ -147,7 +153,6 @@ async function run({ github, context, core, env = process.env }) { } // Both writes are tried; either failing fails the step afterwards. - const errors = []; try { await github.rest.issues.addLabels({ owner, diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index 11441f6..0e149a3 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -205,6 +205,11 @@ test("a changed file matching hold-paths adds trust-gate whatever the review say assert.match(writes[1][1].body, /criteria=trust-gate -->/u); assert.match(writes[1][1].body, /Changed files match hold-paths/u); } + // 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 () => { @@ -255,8 +260,22 @@ test("a failed comment or file listing fails the step", async () => { fail: { createComment: true }, }); assert.match(comment.error.message, /could not post the hold comment/u); - const listing = await apply(baseEnv(), { fail: { listFiles: true } }); - assert.match(listing.error.message, /HTTP 502/u); + // A failed listing still holds with trust-gate, then fails the step. + const listing = await apply( + baseEnv({ + KEY_REVIEW: review({ hold: { criteria: ["cost"], 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"], + ); }); test("a rerun posts no second marker for the same head and criteria", async () => { From 3649a4160b1f6b80cb1910845b54db20461db2e4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 01:02:53 -0400 Subject: [PATCH 3/3] fix(pr-review-cli): log a warn-only hit in the hold reason The hold step now logs the publish gate's warn-only labels the way publish-review does, instead of dropping them. Co-Authored-By: Claude Opus 5.5 --- .github/actions/apply-review-hold/hold.cjs | 9 ++++++--- .github/scripts/apply-review-hold.test.cjs | 21 ++++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs index 3de6197..15355e7 100644 --- a/.github/actions/apply-review-hold/hold.cjs +++ b/.github/actions/apply-review-hold/hold.cjs @@ -144,9 +144,12 @@ async function run({ github, context, core, env = process.env }) { 2 * MAX_REASON, ); const findSecret = await loadGate(env); - const withheld = findSecret - ? scan(findSecret, [["the hold reason", reason]]).withheld - : "the publish gate could not be loaded"; + const scanned = findSecret + ? scan(findSecret, [["the hold reason", reason]]) + : { withheld: "the publish gate could not be loaded", warnings: [] }; + for (const warning of scanned.warnings) + core.warning(`${lane}: ${warning}; published anyway.`); + const { withheld } = scanned; if (withheld) { core.warning(`${lane}: the hold reason was withheld: ${withheld}.`); reason = `The reason was withheld: ${withheld}.`; diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index 0e149a3..e5b8775 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -18,7 +18,11 @@ const gate = path.join(root, "publish-gate.mjs"); fs.writeFileSync( gate, `export const findSecret = (text) => - /sk-ant-[A-Za-z0-9_-]{20,}/.test(text) ? ["Anthropic key", 1] : null; + /sk-ant-[A-Za-z0-9_-]{20,}/.test(text) + ? ["Anthropic key", 1] + : /password\\s*=\\s*"[^"]{8,}"/.test(text) + ? ["password or secret assignment", 1] + : null; `, ); // Key-shaped, built at run time so no scanner reads a key in this file. @@ -338,3 +342,18 @@ test("hold.cjs converts globs exactly as the scope step does", () => { 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({ + KEY_REVIEW: review({ + hold: { criteria: ["cost"], reason: 'password = "hunter2hunter2"' }, + }), + }), + { files: [] }, + ); + assert.match(writes[1][1].body, /hunter2hunter2/u); + assert.deepEqual(core.warnings, [ + "claude-review: password or secret assignment in the hold reason; published anyway.", + ]); +});