feat(pr-pipeline): turn path-based merge holds off behind one switch (owner Q15)

.changes/pr-pipeline/feat-merge-path-holds-switch-3c1d7a20.md

@@ -0,0 +1,17 @@
+---
+bump: major
+---
+
+### Changed
+
+- `merge.diff-check.path-holds` turns off every path-based merge hold with one switch. Absent or
+  false (the default, and this repository's setting by owner decision Q15), a changed path no
+  longer holds a pull request for its path alone. This covers the risk scorer's path floor,
+  denied-paths and `review-path:` holds, enable-auto-merge's hold paths and ci-status's
+  merge-holds step. A standing refusal that names only hold paths comes off without an approval.
+  Set it to true to bring every one back. Cost paths, unsafe or aliasing paths, the manifest
+  review, the trusted-actor widening, body holds, vetoes and `do-not-merge` still hold. The gate
+  report (`risk.pathHoldsSkipped`), enable-auto-merge's log and merge-holds.mjs name each hold
+  the switch skipped.
+- Breaking: a config that lists `denied-paths` or `review-path:` globs and has no `path-holds` key
+  no longer holds on them. Set `merge.diff-check.path-holds: true` to keep the old behavior.

.changes/source-control/feat-merge-path-holds-switch-8e52b4f9.md

@@ -0,0 +1,13 @@
+---
+bump: major
+---
+
+### Changed
+
+- The babysit merge gate reads `merge.diff-check.path-holds` in the pr-merge lane. Absent or
+  false, the `high-path:` floor and configured globs and the `review-path:` holds move out of
+  the risk score into the gate report's `risk.pathHoldsSkipped`. True keeps them. Unsafe or
+  aliasing paths (`high-path:**`), the manifest review and cost holds still hold. The local
+  babysit lane keeps every path hold.
+- Breaking: a config that lists `denied-paths` or `review-path:` globs and has no `path-holds` key
+  no longer holds on them. Set `merge.diff-check.path-holds: true` to keep the old behavior.

.github/scripts/pr-merge/enable-auto-merge.mjs

@@ -228,6 +228,7 @@ import {
   pathHoldKind,
   pathLifters,
   personWrote,
+  readPathHolds,
   readStatuses,
   unblockStops,
   vetoers,
@@ -664,26 +665,40 @@ function personReasons(body, head, label) {
 // (merge-holds.pathHoldKind's "it changes <path>, a hold path"): the one
 // escalation a trusted person's approval of the head lifts, so the lane may
 // take its label off. A cost, risk or verifier reason, a label a person set
-// with no refusal, or a comment at another head is not one.
+// with no refusal, or a comment at another head is not one, except that with
+// `pathHolds` false a risk reason whose every code is a path hold
+// (babysit_risk.is_path_hold: `high-path:<glob>` but `high-path:**`, or
+// `review-path:`) is one too.
 const HOLD_PATH_LINE = /^it changes .+, a hold path$/;
-export function holdPathOnly(body, head, label) {
+const PATH_HOLD_CODE = /^(high-path:(?!\*\*$)|review-path:)\S/;
+const pathOnlyRisk = (line) =>
+  line.startsWith(READER_HOLDS.risk) &&
+  line.endsWith(")") &&
+  line
+    .slice(READER_HOLDS.risk.length, -1)
+    .split("; ")
+    .every((part) => PATH_HOLD_CODE.test(part.split(": ")[0]));
+export function holdPathOnly(body, head, label, pathHolds = true) {
   const reasons = personReasons(body, head, label);
-  return reasons !== null && reasons.length > 0 && reasons.every((line) => HOLD_PATH_LINE.test(line));
+  return reasons !== null && reasons.length > 0 && reasons.every((line) => HOLD_PATH_LINE.test(line) || (pathHolds === false && pathOnlyRisk(line)));
 }
 
 // Whether the lane lifts `label` off `pull`: its newest refusal at the head
 // names only hold paths, the changed files still hold on nothing but a hold
 // path (no cost hold, a listing read whole), and `detail.lifters` holds a
 // trusted person whose approval binds the head (merge-holds pathLifters, or
-// liftersOf over base merges).
-export function approvalLift(pull, detail, { bot, label, holdPaths }) {
-  if (!holdPathOnly(priorRefusal(detail?.comments?.nodes, bot)?.body, pull.headRefOid, label)) {
+// liftersOf over base merges), or `pathHolds` is false, so no hold path holds.
+export function approvalLift(pull, detail, { bot, label, holdPaths, pathHolds = true }) {
+  if (!holdPathOnly(priorRefusal(detail?.comments?.nodes, bot)?.body, pull.headRefOid, label, pathHolds)) {
     return false;
   }
-  if (!Array.isArray(holdPaths) || detail.filesCapped === true || pathHoldKind(detail.files, holdPaths)?.kind !== "hold-path") {
+  // Off, a risk refusal's path may be one only the gate's floor holds, so a
+  // readable listing is enough.
+  const listed = pathHolds === false ? Array.isArray(detail.files) : pathHoldKind(detail.files, holdPaths)?.kind === "hold-path";
+  if (!Array.isArray(holdPaths) || detail.filesCapped === true || !listed) {
     return false;
   }
-  return costHold(detail.files, detail.costDecision) === null && Array.isArray(detail.lifters) && detail.lifters.length > 0;
+  return costHold(detail.files, detail.costDecision) === null && (pathHolds === false || (Array.isArray(detail.lifters) && detail.lifters.length > 0));
 }
 
 // Whether `label` stands for enable-auto-merge's refusal alone, so the lane
@@ -1231,7 +1246,7 @@ export function unblockHold(stops) {
 // `detail` is its discussion; undefined means not read yet, and an arm or a
 // keep then waits on it (action `read`). Its `verify` is the gate's verifier
 // evidence; absent, an arm or a keep waits on it (action `verify`).
-export function decide(pull, { bot, humanGated, holdPaths = null }, detail) {
+export function decide(pull, { bot, humanGated, holdPaths = null, pathHolds = true }, detail) {
   if (pull.author?.__typename !== "Bot" || pull.author.login !== bot) {
     return { action: "none", reason: "not the lanes App's" };
   }
@@ -1276,7 +1291,7 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) {
   // head or of an earlier head carried over base merges (`lifters`) as
   // ci-status lifts it; a listing or check that could not run
   // is a lane's, which the next run reads again.
-  const pathHeld = pathHoldKind(detail.files, holdPaths);
+  const pathHeld = pathHoldKind(detail.files, holdPaths, pathHolds);
   if (pathHeld !== null && pathHeld.kind !== "hold-path") {
     return { action: armed ? "disarm" : "none", reason: pathHeld.reason, clears: "lane" };
   }
@@ -1370,6 +1385,9 @@ export async function main({ env = process.env, github, actions, verify, room, w
     const statuses = [];
     const humanGated = humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json");
     const holdPaths = readHoldPaths(env.RESOLVED_CONFIG ?? "");
+    // merge.diff-check.path-holds (owner decision Q15): off, no hold path
+    // holds, and a refusal naming only hold paths comes off (approvalLift).
+    const pathHolds = readPathHolds(env.RESOLVED_CONFIG ?? "");
     // The newest answer's rate limit and body, for an arm GitHub refuses.
     let answer = null;
     const onResponse = (response) => {
@@ -1578,7 +1596,7 @@ export async function main({ env = process.env, github, actions, verify, room, w
           console.log(`enable-auto-merge: #${pull.number} labels did not read whole: ${describe(error)}`);
         }
       }
-      let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths });
+      let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths, pathHolds });
       let clears;
       let comment;
       let withdraw;
@@ -1626,7 +1644,7 @@ export async function main({ env = process.env, github, actions, verify, room, w
             // A hold-path refusal with a trusted person's approval in the
             // discussion reads on, for approvalLift below.
             const approvedBy = (detail.reviews?.nodes ?? []).map(itemOf).some((item) => item.isUser && trustedHumans(env).has(item.userId) && item.state === "APPROVED");
-            unmarked = widen && !isPartial(standing, pull.headRefOid) && !(approvedBy && holdPathOnly(standing, pull.headRefOid, humanGated));
+            unmarked = widen && !isPartial(standing, pull.headRefOid) && !((approvedBy || pathHolds === false) && holdPathOnly(standing, pull.headRefOid, humanGated, pathHolds));
           }
           if (detail !== null && !unmarked) {
             const { paths: files, capped } = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number);
@@ -1637,7 +1655,11 @@ export async function main({ env = process.env, github, actions, verify, room, w
             // (merge-holds.liftersOf) lifts the hold path as ci-status lifts
             // it, so a base merge after a person's approval does not send the
             // pull request back to them.
-            if (!lifters.length && Array.isArray(holdPaths) && pathHoldKind(files, holdPaths)?.kind === "hold-path") {
+            const skipped = pathHolds === false ? pathHoldKind(files, holdPaths) : null;
+            if (skipped?.kind === "hold-path") {
+              console.log(`enable-auto-merge: #${pull.number} path holds are off (merge.diff-check.path-holds), so nothing holds it for this: ${escapeCommand(skipped.reason)}`);
+            }
+            if (!lifters.length && Array.isArray(holdPaths) && pathHoldKind(files, holdPaths, pathHolds)?.kind === "hold-path") {
               lifters = (await liftersOf(github, repo[0], pull.number, pull.headRefOid, humans, holdPaths)).lifters;
             }
             let stops = null;
@@ -1656,7 +1678,7 @@ export async function main({ env = process.env, github, actions, verify, room, w
             // The label must stand for that refusal alone (liftBlocker), and
             // the head read again just before the delete must be the one
             // judged. A read that fails keeps the label.
-            let lift = widen && approvalLift(pull, detail, { bot: slug, label: humanGated, holdPaths });
+            let lift = widen && approvalLift(pull, detail, { bot: slug, label: humanGated, holdPaths, pathHolds });
             if (lift) {
               let kept;
               try {
@@ -1677,7 +1699,8 @@ export async function main({ env = process.env, github, actions, verify, room, w
               if (write) {
                 await github("DELETE", `/repos/${repo[0]}/issues/${pull.number}/labels/${encodeURIComponent(humanGated)}`);
               }
-              console.log(`enable-auto-merge: #${pull.number} ${write ? "lifted" : "would lift (report-only)"} ${escapeCommand(humanGated)}: ${detail.lifters.join(", ")} approved its head, which lifts its hold path`);
+              const why = pathHolds === false ? "path holds are off (merge.diff-check.path-holds)" : `${detail.lifters.join(", ")} approved its head`;
+              console.log(`enable-auto-merge: #${pull.number} ${write ? "lifted" : "would lift (report-only)"} ${escapeCommand(humanGated)}: ${why}, which lifts its hold path`);
               widen = false;
               // Off in this run's view too, so a person's reason the verifier
               // read adds puts the label back (refusalWrites).
@@ -1692,7 +1715,7 @@ export async function main({ env = process.env, github, actions, verify, room, w
         ({ action, reason, clears, comment, withdraw, partial, fold = null } =
           widen && (detail === null || unmarked)
             ? { action: "none", reason: labelHold, clears: undefined, comment: undefined, withdraw: undefined, partial: undefined, fold: null }
-            : decide(judged, { bot: slug, humanGated, holdPaths }, detail));
+            : decide(judged, { bot: slug, humanGated, holdPaths, pathHolds }, detail));
         if (detail && reason === unblockHold(detail.unblockStops)) {
           unblockSkips.push(`#${pull.number} ${action === "disarm" ? "disarmed" : "not queued"}: ${escapeCommand(reason)}`);
         }
@@ -1742,7 +1765,7 @@ export async function main({ env = process.env, github, actions, verify, room, w
         }
         answered = evidence !== null && !decided;
         if (!decided) {
-          ({ action, reason, clears, comment, withdraw, partial, wholeReview } = decide(judged, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence }));
+          ({ action, reason, clears, comment, withdraw, partial, wholeReview } = decide(judged, { bot: slug, humanGated, holdPaths, pathHolds }, { ...detail, verify: evidence }));
         }
         // A kept answer that holds nothing: read in full once, never armed on.
         if (!decided && action === "verify") {
@@ -1751,7 +1774,7 @@ export async function main({ env = process.env, github, actions, verify, room, w
           } else {
             evidence = await read({ fresh: true });
             if (!decided) {
-              ({ action, reason, clears, comment, withdraw, wholeReview } = decide(judged, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence }));
+              ({ action, reason, clears, comment, withdraw, wholeReview } = decide(judged, { bot: slug, humanGated, holdPaths, pathHolds }, { ...detail, verify: evidence }));
             }
           }
         }

.github/scripts/pr-merge/enable-auto-merge.test.mjs

@@ -3376,3 +3376,104 @@ test("an approved hold-path escalation keeps its label while anything else stand
   const lifted = await run([held({ timeline: [edited, label, approval, restSaid(claude, "Review done, no findings.", after)], restReviews: [{ user: KYLE, state: "APPROVED", submitted_at: after }] })], trustedKyle());
   assert.deepEqual(lifted.notes, [{ method: "DELETE", path: `${ISSUES}/2/labels/needs-human` }]);
 });
+
+// merge.diff-check.path-holds (owner decision Q15): off, a gate script or a
+// workflow on the floor arms, and a refusal naming only hold paths comes off
+// with no approval; a cost path and the do-not-merge label still hold.
+function pathHoldsOff() {
+  const dir = tempDir("enable-auto-merge-");
+  writeFileSync(join(dir, "resolved.json"), JSON.stringify({ merge: { "diff-check": { "denied-paths": ["docs/held.md"], "path-holds": false } } }));
+  return { RESOLVED_CONFIG: join(dir, "resolved.json") };
+}
+const GATE_SCRIPT = [{ filename: ".github/scripts/pr-merge/clear-escalations.mjs" }];
+const REVIEW_WORKFLOW = [{ filename: ".github/workflows/pr-review-main.yml" }];
+
+test("with path holds on, a gate script or a floor workflow change is not armed", async () => {
+  const { writes } = await run([pull(60, { files: GATE_SCRIPT }), pull(61, { files: REVIEW_WORKFLOW })]);
+  assert.deepEqual(writes, []);
+});
+
+test("with path holds off, a gate script or a floor workflow change is armed and the skip is logged", async () => {
+  const { writes, lines } = await run([pull(60, { files: GATE_SCRIPT }), pull(61, { files: REVIEW_WORKFLOW }), pull(62, { files: HELD })], pathHoldsOff());
+  assert.deepEqual(writes, [
+    { mutation: "arm", id: "PR_60", head: HEAD },
+    { mutation: "arm", id: "PR_61", head: HEAD },
+    { mutation: "arm", id: "PR_62", head: HEAD },
+  ]);
+  assert.ok(
+    lines.some((line) => line === "enable-auto-merge: #60 path holds are off (merge.diff-check.path-holds), so nothing holds it for this: it changes .github/scripts/pr-merge/clear-escalations.mjs, a hold path"),
+    lines.join("\n"),
+  );
+});
+
+test("with path holds off, a cost path and the do-not-merge label still hold", async () => {
+  const { writes } = await run([pull(63, { files: COST_FILES }), pull(64, { files: GATE_SCRIPT, labels: labels("do-not-merge") })], pathHoldsOff());
+  assert.deepEqual(writes, []);
+});
+
+test("with path holds off, a standing hold-path refusal comes off with no approval", async () => {
+  const LIFT = { method: "DELETE", path: `${ISSUES}/2/labels/needs-human` };
+  const reason = "it changes .github/scripts/pr-merge/clear-escalations.mjs, a hold path";
+  const held = (comments, files = GATE_SCRIPT) =>
+    pull(2, { files, labels: labels("needs-human"), verdict: PASSED, discussion: { comments }, timeline: liftTimeline(personNote(reason)).slice(0, 2) });
+  const lifted = await run([held([lanesSaid(personNote(reason))])], { ...trustedKyle(), ...pathHoldsOff() });
+  assert.deepEqual(lifted.notes, [LIFT]);
+  assert.equal(lifted.writes.at(-1)?.mutation, "arm");
+  assert.ok(lifted.lines.some((line) => String(line).includes("lifted needs-human: path holds are off")), lifted.lines.join("\n"));
+  // On, the same refusal waits for a person's approval.
+  const kept = await run([held([lanesSaid(personNote(reason))])], trustedKyle());
+  assert.ok(!kept.notes.some(({ method }) => method === "DELETE"));
+  // A refusal that also names a cost path stays.
+  const cost = await run([held([lanesSaid(personNote(`${reason}\n${COST_REASON}`))], [...GATE_SCRIPT, ...COST_FILES])], { ...trustedKyle(), ...pathHoldsOff() });
+  assert.ok(!cost.notes.some(({ method }) => method === "DELETE"));
+});
+
+// ci-status passes the base's merge.diff-check.path-holds as PATH_HOLDS.
+test("merge-holds.mjs holds no hold path from the command line with PATH_HOLDS false, and still holds a cost path or a body hold", () => {
+  const dir = tempDir("merge-holds-");
+  const head = "0123456789abcdef0123456789abcdef01234567";
+  const cli = (files, pathHolds, body = "Closes #1") => {
+    writeFileSync(join(dir, "body"), body);
+    writeFileSync(join(dir, "discussion"), "");
+    writeFileSync(join(dir, "actors"), JSON.stringify({ actors: [{ kind: "human", login: "ana", id: 5 }] }));
+    writeFileSync(join(dir, "files"), files.map((row) => JSON.stringify(row)).join("\n"));
+    writeFileSync(join(dir, "denied"), JSON.stringify([]));
+    const args = [...["7", "body", "discussion", "files", "denied", "actors"].map((name, index) => (index ? join(dir, name) : name)), head];
+    const env = { ...process.env, GITHUB_REPOSITORY: "", ...(pathHolds === undefined ? {} : { PATH_HOLDS: pathHolds }) };
+    delete env.GH_TOKEN;
+    return spawnSync(process.execPath, [fileURLToPath(new URL("./merge-holds.mjs", import.meta.url)), ...args], { encoding: "utf8", env });
+  };
+  const gate = [".github/scripts/pr-merge/clear-escalations.mjs"];
+  const off = cli([gate], "false");
+  assert.equal(off.status, 0, off.stdout);
+  assert.ok(off.stdout.split("\n").includes("#7: no body hold, no member veto and no hold path."), off.stdout);
+  assert.ok(off.stdout.includes("#7: path holds are off (merge.diff-check.path-holds), so nothing holds it for this: it changes .github/scripts/pr-merge/clear-escalations.mjs, a hold path."), off.stdout);
+  for (const on of [undefined, "true"]) {
+    const held = cli([gate], on);
+    assert.equal(held.status, 1, `PATH_HOLDS ${on}`);
+    assert.match(held.stdout, /it changes \.github\/scripts\/pr-merge\/clear-escalations\.mjs, a hold path/);
+  }
+  assert.equal(cli([["infra/main.tf"]], "false").status, 1);
+  assert.equal(cli([gate], "false", "Closes #1\n\nHold: wait for the owner").status, 1);
+});
+
+// The babysit gate's own path holds, recorded in a risk refusal before the
+// switch went off, come off the same way; an unsafe path or any other code stays.
+test("with path holds off, a standing risk refusal naming only path holds comes off", async () => {
+  const LIFT = { method: "DELETE", path: `${ISSUES}/2/labels/needs-human` };
+  const gate = ".github/scripts/pr-merge/clear-escalations.mjs";
+  const risk = (codes) => `pr-merge's gate holds it on its risk score (${codes})`;
+  const held = (reason) =>
+    pull(2, { files: GATE_SCRIPT, labels: labels("needs-human"), verdict: PASSED, discussion: { comments: [lanesSaid(personNote(reason))] }, timeline: liftTimeline(personNote(reason)).slice(0, 2) });
+  const off = { ...trustedKyle(), ...pathHoldsOff() };
+  const lifted = await run([held(risk(`high-path:${gate}: ${gate}`))], off);
+  assert.deepEqual(lifted.notes, [LIFT], lifted.lines.join("\n"));
+  for (const [why, reason, env] of [
+    ["path holds on", risk(`high-path:${gate}: ${gate}`), trustedKyle()],
+    ["an unsafe path", risk("high-path:**: 'a' mode '120000' (symlink)"), off],
+    ["a path hold beside an unbound review", risk(`high-path:${gate}: ${gate}; evidence-unbound:claude-review: evidence-missing`), off],
+  ]) {
+    const kept = await run([held(reason)], env);
+    assert.ok(!kept.notes.some(({ method }) => method === "DELETE"), `${why} lifts nothing`);
+  }
+});

.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json

@@ -1,7 +1,8 @@
 {
   "merge": {
     "diff-check": {
-      "denied-paths": ["docs/held.md", "docs/conventions/pr-pipeline.yaml", ".github/standards/trusted-actors/trusted-actors.json"]
+      "denied-paths": ["docs/held.md", "docs/conventions/pr-pipeline.yaml", ".github/standards/trusted-actors/trusted-actors.json"],
+      "path-holds": true
     }
   }
 }