fix(pr-review-cli): hold only for a real charge or a critical finding

.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.
 

.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,