diff --git a/.changes/pr-pipeline/feat-merge-path-holds-switch-3c1d7a20.md b/.changes/pr-pipeline/feat-merge-path-holds-switch-3c1d7a20.md new file mode 100644 index 0000000000..6a3e632a8a --- /dev/null +++ b/.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. diff --git a/.changes/source-control/feat-merge-path-holds-switch-8e52b4f9.md b/.changes/source-control/feat-merge-path-holds-switch-8e52b4f9.md new file mode 100644 index 0000000000..025c085ba6 --- /dev/null +++ b/.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. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index c000ae6941..a7bade51c7 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.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 , 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:` 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 })); } } } diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 7b487b719f..1c85796703 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.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`); + } +}); diff --git a/.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json b/.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json index 79931726c3..1024ce08e1 100644 --- a/.github/scripts/pr-merge/fixtures/enable-auto-merge-resolved.json +++ b/.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 } } } diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index daf330d861..1602bfe74e 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -71,7 +71,8 @@ // line; the files file one changed file per line, its path, renamed-from // path and status (changeOf); the denied-paths file the config's // merge.diff-check.denied-paths as JSON; the trusted-actors file -// trusted-actors.json. Each hold prints as an ::error:: and exits 1; no hold +// trusted-actors.json. PATH_HOLDS "false" turns the hold paths off +// (pathHoldsOf). Each hold prints as an ::error:: and exits 1; no hold // exits 0. Who wrote a body that holds, a changed trusted-actor list, a // cost path with a decision link in the body, and the approvals of earlier // heads when no approval of the head lifts a hold path, are read with @@ -385,11 +386,27 @@ export const REVIEW_FLOOR = Object.freeze(HOLD_PATH_CLASSES.filter(([, kind]) => // Why the head's changed `files` hold it, or null: one matching `holdPaths` // or HOLD_PATH_FLOOR. Null `holdPaths` or `files` (unreadable) holds it. -export const pathHold = (files, holdPaths) => pathHoldKind(files, holdPaths)?.reason ?? null; +// `on` false (pathHoldsOf) matches no hold path. +export const pathHold = (files, holdPaths, on = true) => pathHoldKind(files, holdPaths, on)?.reason ?? null; + +// merge.diff-check.path-holds (owner decision Q15): absent or false, no +// changed path holds for its path alone; any other value keeps every hold +// path, as babysit_merge_policy's switch() reads it. +export const pathHoldsOf = (value) => ![undefined, null, false, "false"].includes(value); + +// The switch from a resolve-config output file; unreadable keeps the holds. +export function readPathHolds(path) { + try { + return pathHoldsOf(JSON.parse(readFileSync(path, "utf8"))?.merge?.["diff-check"]?.["path-holds"]); + } catch { + return true; + } +} // pathHold's answer with its kind: `hold-path` for a changed file a hold // path matches, `unread` for a list that could not be read or checked. -export function pathHoldKind(files, holdPaths) { +// With `on` false a readable list matches nothing. +export function pathHoldKind(files, holdPaths, on = true) { if (!Array.isArray(holdPaths)) { return { kind: "unread", reason: "the hold paths could not be read" }; } @@ -399,6 +416,9 @@ export function pathHoldKind(files, holdPaths) { if (typeof posix.matchesGlob !== "function") { return { kind: "unread", reason: "this Node has no path.matchesGlob, so its hold paths cannot be checked" }; } + if (on === false) { + return null; + } const globs = [...holdPaths, ...HOLD_PATH_FLOOR]; const held = files.find((path) => globs.some((glob) => matchesHoldPath(path, glob))); return held === undefined ? null : { kind: "hold-path", reason: `it changes ${held}, a hold path` }; @@ -1289,13 +1309,13 @@ export function pathLifters(items, humans, head) { // hold path but not an unreadable file list or hold-path list, nor a cost // path, which only the owner's `decision` lifts. It lifts `widened` // (actorsHold's reason) too. `byPerson` (personWrote) says whether the body -// can hold. -export function holds(body, items, files, holdPaths, lifters = [], decision = null, byPerson = true, widened = null) { +// can hold; `pathHolds` false (pathHoldsOf) holds no hold path. +export function holds(body, items, files, holdPaths, lifters = [], decision = null, byPerson = true, widened = null, pathHolds = true) { const reasons = []; if (widened && !lifters.length) { reasons.push(`${widened}; an approving review of this head by a person on the trusted-actor list lifts it, and the approval carries over later merges that bring only the base's changes; then re-run ci-status`); } - const pathReason = pathHold(files, holdPaths); + const pathReason = pathHold(files, holdPaths, pathHolds); const readable = Array.isArray(files) && Array.isArray(holdPaths); if (pathReason && !(readable && lifters.length)) { reasons.push( @@ -1589,13 +1609,20 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi console.log(`#${number}: its body's hold text is the lanes App's own, which holds nothing.`); } const widened = actorsHold(await withActorContents(github, repo, changes, env.BASE_SHA, head)); - const held = pathHold(files, holdPaths); + // PATH_HOLDS, merge.diff-check.path-holds as the base has it: only + // "false" turns the hold paths off. + const pathHolds = env.PATH_HOLDS !== "false"; + const skipped = pathHolds ? null : pathHoldKind(files, holdPaths); + if (skipped?.kind === "hold-path") { + console.log(`#${number}: path holds are off (merge.diff-check.path-holds), so nothing holds it for this: ${escape(skipped.reason)}.`); + } + const held = pathHold(files, holdPaths, pathHolds); let lifted = { lifters: [] }; if (!approved.length && ((held && Array.isArray(holdPaths)) || widened) && /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo)) { lifted = await liftersOf(github, repo, Number(number), head, humans, holdPaths); } const lifters = approved.length ? approved : lifted.lifters; - const reasons = holds(body, items, files, holdPaths, lifters, decision, byPerson, widened); + const reasons = holds(body, items, files, holdPaths, lifters, decision, byPerson, widened, pathHolds); const how = approved.length ? "approved this head" : "approved an earlier head whose every change since is a base merge"; if (held && lifters.length && Array.isArray(holdPaths)) { console.log(`#${number}: ${lifters.join(", ")} ${how}, which lifts its hold path (${escape(held)}).`); diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index f3163eef9a..3291629ef3 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.mjs @@ -278,6 +278,20 @@ export function readHoldPaths(path) { } } +// The denied paths a PR holds on: readHoldPaths while +// merge.diff-check.path-holds is on, none while it is absent or false (owner +// decisions Q15 and Q16; merge-holds.mjs pathHoldsOf reads it alike). An +// unreadable config still holds. +export function heldPaths(path) { + const paths = readHoldPaths(path); + try { + const on = JSON.parse(readFileSync(path, "utf8"))?.merge?.["diff-check"]?.["path-holds"]; + return paths !== null && [undefined, null, false, "false"].includes(on) ? [] : paths; + } catch { + return paths; + } +} + // The read runner's scan refusal markers, the ones address-feedback.mjs // scanRefusal reads; the runner copies only this script's directory, so it // cannot import that one. `patch` when every marker names a patch over its @@ -1689,7 +1703,7 @@ export async function main({ env = process.env, fetch = globalThis.fetch, now = } } const resolvedPath = env.RESOLVED_CONFIG || join(env.RUNNER_TEMP ?? "", "resolved-config.json"); - const holdPaths = readHoldPaths(resolvedPath); + const holdPaths = heldPaths(resolvedPath); // A reuse whose outcome cannot be read never takes the limit record: the // skipped run hit no limit, and a limited marker would loop restarts. A // failed compare leaves its file list unread, which holds the PR. diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index 5425b11314..b6f883a1e6 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.test.mjs @@ -23,6 +23,7 @@ import { main, mustHold, parseOutcomeMarker, + heldPaths, readHoldPaths, readFunding, readChecks, @@ -309,6 +310,25 @@ for (const [name, text] of [ }); } +// merge.diff-check.path-holds (owner decisions Q15 and Q16): off, a denied +// path holds nothing; on, it holds as before; an unreadable list still holds. +test("with path holds off a denied path holds nothing, and on it holds the PR", () => { + const config = (pathHolds) => JSON.stringify({ merge: { "diff-check": { "denied-paths": ["a/**"], ...(pathHolds === undefined ? {} : { "path-holds": pathHolds }) } } }); + for (const off of [undefined, false]) { + withDir({ "resolved-config.json": config(off) }, (dir) => { + const holdPaths = heldPaths(join(dir, "resolved-config.json")); + assert.deepEqual(holdPaths, []); + assert.ok(!decideDone({ holdPaths, changedFiles: ["a/x.md"] }).body.includes("Hold: a human merges this")); + }); + } + withDir({ "resolved-config.json": config(true) }, (dir) => { + const holdPaths = heldPaths(join(dir, "resolved-config.json")); + assert.deepEqual(holdPaths, ["a/**"]); + assert.ok(decideDone({ holdPaths, changedFiles: ["a/x.md"] }).body.split("\n").includes("Hold: a human merges this")); + }); + withDir({ "resolved-config.json": "{" }, (dir) => assert.equal(heldPaths(join(dir, "resolved-config.json")), null)); +}); + test("an unreadable resolved config reads as no denied-path list and holds the PR", () => { withDir({}, (dir) => { const holdPaths = readHoldPaths(dir); @@ -2512,7 +2532,7 @@ const summaryFile = (overrides = {}, output = {}) => }); // Readable denied paths, so the README-only, read-only-class reuse PRs below // hold only because no runner checks or leak scan reached them. -const RESOLVED = JSON.stringify({ merge: { "diff-check": { "denied-paths": HOLD_GLOBS } } }); +const RESOLVED = JSON.stringify({ merge: { "diff-check": { "denied-paths": HOLD_GLOBS, "path-holds": true } } }); const summaryEnv = (dir) => ({ ...reuseEnv(dir), REUSE_SUMMARY_DIR: dir, RESOLVED_CONFIG: join(dir, "resolved.json") }); const tipRoute = (sha = TIP) => ["GET", /\/git\/ref\/heads\/work-resolve-issue\/issue-77-400-1$/, 200, { object: { sha } }]; const usable = { title: "docs(work-items): summary title", summary: "SUMMARY-PASS-TEXT", changes: ["CHANGE-ONE"] }; diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index cd24be2eac..e5f7fc2e7e 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -5351,7 +5351,8 @@ jobs: # on the holds that gate honors: a body hold, a member's veto comment, # and a changed file or renamed-from path matching the merge lane's # denied paths (pr-pipeline.yaml merge.diff-check.denied-paths, as main - # has it, plus merge-holds.mjs's HOLD_PATH_FLOOR), so a push after + # has it, plus merge-holds.mjs's HOLD_PATH_FLOOR) while the base's + # merge.diff-check.path-holds is true (owner decision Q15), so a push after # arming is held on its own pull request event; a person on the # trusted-actor list (as main has it) lifts it by approving the head, and # the approval carries to a later head whose every change since is a @@ -5662,6 +5663,13 @@ jobs: echo "::error::cannot read merge.diff-check.denied-paths from $config at $BASE_SHA." exit 1 } + # merge.diff-check.path-holds at BASE_SHA: absent or false turns the + # hold paths off (merge-holds.mjs pathHoldsOf). + PATH_HOLDS=$(yq '.merge."diff-check"."path-holds" // false' "$RUNNER_TEMP/pr-pipeline.yaml") || { + echo "::error::cannot read merge.diff-check.path-holds from $config at $BASE_SHA." + exit 1 + } + export PATH_HOLDS read_api "the files of #$pr" "repos/$GITHUB_REPOSITORY/pulls/$pr/files?per_page=100" --paginate \ --jq '.[] | [.filename, .previous_filename, .status] | @json' >"$RUNNER_TEMP/files.jsonl" || exit 1 changed=$(jq -er '.changed_files | numbers' <<<"$pull") || { echo "::error::the changed-file count of #$pr could not be read."; exit 1; } diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index 84a90d798e..6ec73fbaec 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -1076,7 +1076,8 @@ merge: # (pr-merge.yml and pr-require-checks.yml, by owner direction 2026-10-27; # the other modules the floored scripts import; the rest of the Python that # decides the holds) wait for a clean security review bound to the head. - # It holds whatever `high-path-floor` says. `high-path-floor: + # It holds whatever `high-path-floor` says, while `path-holds` (below) is + # true. `high-path-floor: # false` turns off the manifest review, which the security lane flags # instead. The trusted-actor widening and cost holds stay on either way. The # verifier verdict, both AI review lanes, ci-status and the post-merge @@ -1089,6 +1090,12 @@ merge: # Owner decision 2026-10-17: the lane hold covers structural and untrusted- # provenance work only; scoped lane PRs off the HIGH floor merge on the # verifier verdict. + # `path-holds: false`: no changed path holds a pull request for its path + # alone, not the floor above, denied-paths nor `review-path:` (owner + # decision Q15, 2026-10-29: "We shouldn't really have any path + # restrictions at all"). True brings every one back. The gate report and + # enable-auto-merge's log name each hold it skipped. diff-check: denied-paths: [] high-path-floor: false + path-holds: false diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index b1ceb6798e..5d522bad11 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -984,7 +984,8 @@ the rules that only tighten in `hold_patterns`, and `test_merge_holds_sync.py` runs its `--check`; owner decisions Q-FLOOR (B), [Q-ENFORCE (A)](https://github.com/melodic-software/claude-code-plugins/pull/9826#issuecomment-6401599691) - and Q26 option B), matched case-insensitively whatever `high-path-floor` says: + and Q26 option B), matched case-insensitively whatever `high-path-floor` says, while + `merge.diff-check.path-holds` is true (owner decision Q15 turns it off here): - `trust`, a person merges it: `.github/actions/request-lane-token/**`, which mints the lanes App token, `.github/actions/resolve-config/**`, whose code the token broker runs from main in its signer process with `effect-grants.json`, `lane-rules.json` and `activity-rules.json`, @@ -1392,9 +1393,13 @@ except six: - **Risk switches.** Each check below is a `merge.diff-check` setting that is off when absent: `denied-paths` (an empty list denies no path; a listed path makes the pull request ineligible), `high-path-floor` (true scores the manifest review HIGH; the fixed HIGH path floor scores - either way), + either way while `path-holds` is true), + `path-holds` (true holds a pull request for a changed path alone: the path floor, `denied-paths` + and `review-path:` in the gate, enable-auto-merge's and ci-status's hold paths; absent or false + holds none and the gate report lists each skipped hold as `risk.pathHoldsSkipped`), `max-changed-files` and `max-changed-lines` (absent is no cap). This repository sets - `high-path-floor: false` (owner direction 2026-10-24); the non-author verifier verdict, both AI review lanes, `ci-status` and the post-merge revert breaker gate the rest. It + `high-path-floor: false` (owner direction 2026-10-24) and `path-holds: false` (owner decision + Q15, 2026-10-29, until open issues and pull requests reach zero); the non-author verifier verdict, both AI review lanes, `ci-status` and the post-merge revert breaker gate the rest. It leaves the rest off (owner decision, 2026-10-17). The local babysit lane ignores these switches and keeps the manifest review and both caps. `c2-contradictions` is deprecated and has no effect: the risk score reads no work class. diff --git a/docs/conventions/pr-pipeline/pr-pipeline.schema.json b/docs/conventions/pr-pipeline/pr-pipeline.schema.json index b472e6e0f7..3c38f0fd6f 100644 --- a/docs/conventions/pr-pipeline/pr-pipeline.schema.json +++ b/docs/conventions/pr-pipeline/pr-pipeline.schema.json @@ -1160,7 +1160,12 @@ "high-path-floor": { "type": "boolean", "default": false, - "description": "true scores a plugin or marketplace manifest change that installs or runs something new HIGH. Absent or false: no manifest review. The risk scorer's fixed HIGH path floor (resolve-config, request-lane-token, the trusted-trigger check, check-kill-switch, the rate guard, code owners and submodules) and denied-paths count either way. The local babysit lane always applies the manifest review." + "description": "true scores a plugin or marketplace manifest change that installs or runs something new HIGH. Absent or false: no manifest review. The risk scorer's fixed HIGH path floor (resolve-config, request-lane-token, the trusted-trigger check, check-kill-switch, the rate guard, code owners and submodules) and denied-paths count either way while path-holds is true. The local babysit lane always applies the manifest review." + }, + "path-holds": { + "type": "boolean", + "default": false, + "description": "pr-merge lane only: true holds a pull request for a changed path alone, on the risk scorer's path floor and denied-paths (`high-path:` and `review-path:`), enable-auto-merge's and ci-status's hold paths. Absent or false: no path holds it; the gate report lists each skipped hold. Unsafe or aliasing paths, the manifest review, cost holds and the trusted-actor widening count either way. The local babysit lane always holds paths." }, "c2-contradictions": { "type": "boolean", diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 75777bf378..dde4da3aa3 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -3559,11 +3559,12 @@ def score_unattended( # The manifest review compares base and head text; the cost hold reads a # workflow's head text for its model legs. files = merge_policy.with_contents(repo, pr, files) - # The pr-merge lane's switches (owner decision Q624); the local lane - # keeps the manifest review and both caps. HIGH_FLOOR holds in both. + # The pr-merge lane's switches (owner decisions Q624 and Q15); the local + # lane keeps the manifest review, both caps and every path hold. lane_switches = ( { "high_floor": config.high_path_floor, + "path_holds": config.path_holds, "max_changed_files": config.max_changed_files, "lines_optional": True, } @@ -3606,6 +3607,11 @@ def score_unattended( {"code": r.code, "severity": r.severity.name, "detail": r.detail} for r in risk.reasons ], + # The path holds merge.diff-check.path-holds turned off, counted + # by the rollup. + "pathHoldsSkipped": [ + {"code": r.code, "detail": r.detail} for r in risk.skipped + ], }, "evidence": [ { diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 678b109e18..24f5cdad0c 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -635,6 +635,10 @@ class PipelineConfig: # cap (`max_changed_lines` None: no line cap); a value other than true or # false turns a switch on. The local babysit lane ignores them. high_path_floor: bool = False + # `merge.diff-check.path-holds`: absent or false, no changed path holds + # a pull request for its path alone (owner decision Q15); true restores + # the path floor, the configured denied paths and `review-path:`. + path_holds: bool = False max_changed_files: int | None = None # `merge.max-in-flight`: lane merges in flight at once (`in_flight`). # Absent, or anything but a whole number of at least 1, is 1. The gate @@ -879,6 +883,7 @@ def file_at(path: str, sha: str) -> str | None: ("merge.diff-check.max-changed-lines", False), ("merge.diff-check.max-changed-files", False), ("merge.diff-check.high-path-floor", False), + ("merge.diff-check.path-holds", False), ("merge.diff-check.c2-contradictions", False), ("merge.diff-check.denied-paths", True), ("merge.report-only", False), @@ -927,6 +932,7 @@ def switch(key: str) -> bool: return value not in (None, "false") high_path_floor = switch("merge.diff-check.high-path-floor") + path_holds = switch("merge.diff-check.path-holds") if scalar("merge.diff-check.c2-contradictions") not in (None, "false"): notes.append( "merge.diff-check.c2-contradictions is retired and has no effect:" @@ -1041,6 +1047,7 @@ def switch(key: str) -> bool: dependabot_mode=dependabot_mode, dependabot_majors=dependabot_majors, high_path_floor=high_path_floor, + path_holds=path_holds, max_changed_files=int(files) if files else None, max_in_flight=int(in_flight_cap) if in_flight_cap else 1, max_queue_ejections=int(ejections) if ejections else 2, @@ -1116,6 +1123,7 @@ def loop_rank(value: str | None) -> int: if "hold" in (base.dependabot_majors, main.dependabot_majors) else base.dependabot_majors, high_path_floor=base.high_path_floor or main.high_path_floor, + path_holds=base.path_holds or main.path_holds, max_changed_files=_cap(base.max_changed_files, main.max_changed_files), max_queue_ejections=min(base.max_queue_ejections, main.max_queue_ejections), max_verify_fails=min(base.max_verify_fails, main.max_verify_fails), @@ -1186,6 +1194,7 @@ def count(v: Any, least: int) -> bool: c.report_only, c.require_verifier_verdict, c.high_path_floor, + c.path_holds, ) ) and c.dependabot_mode in DEPENDABOT_MODES diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py index bcfaa99f17..a9088f326a 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py @@ -201,6 +201,10 @@ class RiskInput: # The switches the pr-merge lane may turn off (merge.diff-check); the # defaults keep every check. An unset line cap is MED unless optional. high_floor: bool = True + # `merge.diff-check.path-holds`: False moves every path hold (the + # `high-path:` floor and configured globs, `review-path:`) to + # `RiskResult.skipped`; anything else keeps them (owner decision Q15). + path_holds: bool = True max_changed_files: int | None = 8 # None: no cap lines_optional: bool = False # The repository is public, where standard hosted runners are free; any @@ -241,6 +245,8 @@ class Reason: class RiskResult: score: Risk # max(reason.severity), LOW when no reason reasons: tuple[Reason, ...] # sorted by severity desc, then code + # The path holds `RiskInput.path_holds` False left out of `reasons`. + skipped: tuple[Reason, ...] = () # What a lane pull request must not change unattended (owner decisions @@ -2256,17 +2262,31 @@ def _score(inp: RiskInput) -> RiskResult: files = None if raw_files is None else [_check_file(f) for f in raw_files] # Each producer emits a code at most once, and the only code two of them # could share (high-path:**) is refused as a configured glob. + paths = _high_path_reasons(inp, files or [], list(raw_files or ())) + skipped = ( + [] if inp.path_holds is not False else [r for r in paths if is_path_hold(r)] + ) reasons = [ *_size_reasons(inp, files), *_lane_reasons(inp.lanes), - *_high_path_reasons(inp, files or [], list(raw_files or ())), + *(r for r in paths if r not in skipped), *_cost_reasons(inp, files or [], list(raw_files or ())), *_trust_gate_reasons(files or [], list(raw_files or ())), *_long_line_reasons(inp, files or [], list(raw_files or ())), ] reasons.sort(key=lambda reason: (-reason.severity, reason.code)) worst = max((reason.severity for reason in reasons), default=Risk.LOW) - return RiskResult(Risk(worst), tuple(reasons)) + return RiskResult(Risk(worst), tuple(reasons), tuple(skipped)) + + +def is_path_hold(reason: Reason) -> bool: + """Whether `reason` holds for a changed path alone (`high-path:`, + `review-path:`), which `merge.diff-check.path-holds` turns off. An + unsafe or aliasing path (`high-path:**`) and the manifest review read + more than the path, and stay.""" + return reason.code != UNSAFE_PATH_CODE and reason.code.startswith( + ("high-path:", REVIEW_PATH_PREFIX) + ) # MED reasons an unattended merge passes: the diff's size and the review and diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index 2f8e4efe7a..1e66001913 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -2471,6 +2471,7 @@ def lane_risk_refusal( pulumi_dirs=merge_policy.pulumi_dirs_at(repo, pr), base_sha=json_object(pr.get("base")).get("sha"), high_floor=config.high_path_floor, + path_holds=config.path_holds, max_changed_files=config.max_changed_files, lines_optional=True, ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py index 4c9cc0bec0..238d3ff1a7 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py @@ -39,9 +39,11 @@ "head": {"sha": HEAD, "ref": "feat/x"}, "base": {"sha": BASE, "repo": {"private": False}}, } +# Path holds on: these tests read what lifts one. CONFIG = policy.PipelineConfig( - "c" * 40, "C2", "pr-merge", "agent-lanes[bot]", None, (), "c2-mechanical" -) + "c" * 40, "C2", "pr-merge", "agent-lanes[bot]", None, (), "c2-mechanical", + path_holds=True, +) # fmt: skip DOC = ChangedFile( "docs/guide.md", None, diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py index 169f5f98bd..050205e449 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py @@ -3810,6 +3810,16 @@ def test_the_retired_c2_contradictions_key_only_leaves_a_note(self) -> None: self.assertFalse(hasattr(config, "c2_contradictions")) self.assertEqual(self.diff_check(["c2-contradictions: false"]).notes, ()) + def test_path_holds_are_off_unless_set_true(self) -> None: + # Owner decision Q15: absent or false, no path holds; any other + # value keeps them, as every switch here reads. + self.assertIs(self.diff_check([]).path_holds, False) + self.assertIs(self.diff_check(["path-holds: false"]).path_holds, False) + self.assertIs(self.diff_check(["path-holds: true"]).path_holds, True) + config = self.diff_check(["path-holds: maybe"]) + self.assertIs(config.path_holds, True) + self.assertEqual(len(config.notes), 1) + def test_an_unreadable_switch_is_on_and_a_bad_cap_is_off(self) -> None: config = self.diff_check(["high-path-floor: maybe"]) self.assertIs(config.high_path_floor, True) @@ -6351,6 +6361,7 @@ def test_the_heads_cap_is_shared_per_cache_window_and_a_failure_never( diff-check: denied-paths: [".github/**"] high-path-floor: false + path-holds: false max-changed-lines: 500 """ STRICT_PIPELINE = """\ @@ -6370,6 +6381,7 @@ def test_the_heads_cap_is_shared_per_cache_window_and_a_failure_never( diff-check: denied-paths: [".github/**", "docs/**"] high-path-floor: true + path-holds: true max-changed-lines: 100 max-changed-files: 0 """ @@ -6433,6 +6445,7 @@ def assert_strict(self, config: Any, escalation_minutes: int) -> None: "report_only": True, "require_verifier_verdict": True, "high_path_floor": True, + "path_holds": True, "max_changed_lines": 100, "max_changed_files": 0, "dependabot_mode": "report", diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py index 8aa3160e7d..53efee2151 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_unattended.py @@ -1174,7 +1174,7 @@ def setUp(self) -> None: super().setUp() self.config = policy.PipelineConfig( BASE, "C2", "pr-merge", IDENTITY, 200, (".github/**",), "c2-mechanical", - require_verifier_verdict=True, + require_verifier_verdict=True, path_holds=True, ) # fmt: skip # A passing verdict at every head (test_babysit_verify_evidence covers # the reader). @@ -1265,6 +1265,23 @@ def test_a_trusted_persons_gate_path_clears_on_a_clean_security_review( f"high-path:{gate}" in codes, user["type"] == "Bot", codes ) + def test_path_holds_off_skips_the_floor_and_reports_it(self) -> None: + # Owner decision Q15: off, a lanes App gate-script change scores LOW + # and the gate report names the hold it skipped; on, it holds. + gate = ".github/scripts/pr-merge/clear-escalations.mjs" + self.config = dataclasses.replace(self.config, denied_paths=()) + self.files = ((dataclasses.replace(LOW_FILE, path=gate),), True) + for on, score, skipped in ( + (False, "LOW", [{"code": f"high-path:{gate}", "detail": gate}]), + (True, "HIGH", []), + ): + self.config = dataclasses.replace(self.config, path_holds=on) + with self.subTest(path_holds=on): + _, out = self.lane() + risk = out["unattended"]["risk"] + self.assertEqual(risk["score"], score, risk) + self.assertEqual(risk["pathHoldsSkipped"], skipped) + def test_a_model_app_own_lines_skip_the_floor_only_under_scripts(self) -> None: # Owner decision Q14 (c): a model-driven App's own lines skip the # denied paths only under plugins/*/scripts/. diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py index 1aa82cc409..58ce30dfe0 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py @@ -1158,19 +1158,21 @@ def file_at(path, _sha): ) -def _lane_input(files): +def _lane_input(files, **changes): """The pr-merge lane's input under this repository's own docs/conventions/pr-pipeline.yaml, wired as babysit_merge.score_unattended - wires it.""" + wires it; `changes` overrides a switch.""" config = _lane_config() - return _input( - files=files, - max_changed_lines=config.max_changed_lines, - high_paths=config.denied_paths, - high_floor=config.high_path_floor, - max_changed_files=config.max_changed_files, - lines_optional=True, - ) + wired = { + "files": files, + "max_changed_lines": config.max_changed_lines, + "high_paths": config.denied_paths, + "high_floor": config.high_path_floor, + "path_holds": config.path_holds, + "max_changed_files": config.max_changed_files, + "lines_optional": True, + } + return _input(**(wired | changes)) class LanePathFloorTests(unittest.TestCase): @@ -1179,10 +1181,12 @@ class LanePathFloorTests(unittest.TestCase): (B) and Q-ENFORCE (A)); the trust gate still holds the trusted-actor list.""" - def test_the_live_config_has_the_floor_off(self): + def test_the_live_config_has_the_floor_and_path_holds_off(self): + # Owner decision Q15: path holds off until open issues and PRs reach 0. self.assertIs(_lane_config().high_path_floor, False) + self.assertIs(_lane_config().path_holds, False) - def test_the_floor_holds_with_the_switch_off(self): + def test_the_floor_holds_with_path_holds_on_and_is_skipped_live(self): for path in ( ".gitmodules", ".github/CODEOWNERS", @@ -1239,9 +1243,17 @@ def test_the_floor_holds_with_the_switch_off(self): _lane("claude-security-review", open_findings=1), ) result = score( - dataclasses.replace(_lane_input((_file(path),)), lanes=unreviewed) + _lane_input((_file(path),), lanes=unreviewed, path_holds=True) ) self.assertEqual(result.score, Risk.HIGH, result.reasons) + self.assertTrue( + any(babysit_risk.is_path_hold(r) for r in result.reasons) + ) + live = score(_lane_input((_file(path),), lanes=unreviewed)) + self.assertFalse( + any(babysit_risk.is_path_hold(r) for r in live.reasons) + ) + self.assertNotEqual(live.skipped, ()) def test_trusted_paths_off_the_floor_merge(self): for path in ( @@ -1308,9 +1320,10 @@ def test_a_pipeline_caps_edit_holds_on_the_floor_and_reports_cost(self): patch = "@@ -1,1 +1,1 @@\n- max-in-flight: 5\n+ max-in-flight: 50" files = (_file("docs/conventions/pr-pipeline.yaml", patch=patch),) self.assertEqual( - _codes(score(_lane_input(files))), + _codes(score(_lane_input(files, path_holds=True))), ("high-path:docs/conventions/pr-pipeline.yaml", "cost-finding"), ) + self.assertEqual(_codes(score(_lane_input(files))), ("cost-finding",)) MANIFEST = "plugins/foo/.claude-plugin/plugin.json" @@ -2446,3 +2459,49 @@ def test_a_clean_bound_review_clears_it(self): result = self._score(_lane("claude-security-review")) self.assertNotIn(f"review-path:{self.PATH}", _codes(result)) self.assertFalse(holds(result)) + + +class PathHoldsSwitchTests(unittest.TestCase): + """`merge.diff-check.path-holds` (owner decision Q15): off, a changed + path holds nothing for its path alone; on, the floor holds as before; + a cost path and an unsafe path hold either way.""" + + GATE = ".github/scripts/pr-merge/clear-escalations.mjs" + WORKFLOW = ".github/workflows/pr-merge.yml" + # The security review unbound, so an enforcement path is not cleared. + UNREVIEWED = (_lane("claude-review"), _lane("claude-security-review", bound=False)) + + def _paths(self, result): + return [r.code for r in result.reasons if babysit_risk.is_path_hold(r)] + + def test_off_a_gate_script_change_gets_no_path_hold(self): + result = score(_input(files=(_file(self.GATE),), path_holds=False)) + self.assertEqual(self._paths(result), []) + self.assertFalse(holds(result)) + self.assertEqual([r.code for r in result.skipped], [f"high-path:{self.GATE}"]) + + def test_on_a_gate_script_change_holds_for_a_person(self): + result = score(_input(files=(_file(self.GATE),), path_holds=True)) + self.assertEqual(self._paths(result), [f"high-path:{self.GATE}"]) + self.assertTrue(holds(result)) + self.assertEqual(result.skipped, ()) + + def test_off_a_workflow_change_gets_no_path_hold(self): + files = (_file(self.WORKFLOW),) + on = score(_input(files=files, lanes=self.UNREVIEWED)) + self.assertNotEqual(self._paths(on), []) + off = score(_input(files=files, lanes=self.UNREVIEWED, path_holds=False)) + self.assertEqual(self._paths(off), []) + self.assertEqual(sorted(r.code for r in off.skipped), sorted(self._paths(on))) + + def test_off_a_cost_path_still_holds(self): + result = score(_input(files=(_file("infra/main.tf"),), path_holds=False)) + self.assertIn("cost-review-required", _codes(result)) + self.assertTrue(holds(result)) + + def test_off_an_unsafe_path_still_holds(self): + result = score( + _input(files=(_file("docs/a", mode="120000"),), path_holds=False) + ) + self.assertIn("high-path:**", _codes(result)) + self.assertTrue(holds(result)) diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index cefbe78a09..f38dafad7c 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -292,7 +292,7 @@ expect() { export STUB_PULL STUB_ISSUE='' STUB_REVIEWS='' STUB_INLINE='' STUB_FILES='[["docs/guide.md"]]' export STUB_ACTORS="$ROOT/.github/standards/trusted-actors/trusted-actors.json" -export STUB_CONFIG=$'merge:\n diff-check:\n denied-paths: [docs/held/**]' +export STUB_CONFIG=$'merge:\n diff-check:\n denied-paths: [docs/held/**]\n path-holds: true' KYLE_VETO="$(comment kyle User MEMBER 2026-10-23T10:00:00Z 'Please do not merge this yet.')" STUB_PULL="$(pull 'Closes #1')" @@ -601,9 +601,19 @@ unset STUB_NO_ACTORS STUB_REVIEWS='' STUB_FILES='[["docs/guide.md"]]' STUB_CONFIG='merge: {}' expect "a config with no denied paths holds no path" 0 3 "no hold path" -STUB_CONFIG=$'merge:\n diff-check:\n denied-paths: docs/held/**' +# Owner decision Q15: merge.diff-check.path-holds absent or false turns the +# hold paths off; the module still prints its no-hold line. +STUB_FILES='[[".github/scripts/pr-merge/clear-escalations.mjs"]]' +for config in $'merge:\n diff-check:\n denied-paths: [docs/held/**]' $'merge:\n diff-check:\n path-holds: false'; do + STUB_CONFIG=$config + expect "path holds off ($(tr '\n' ' ' <<<"$config")) holds no floor path" 0 3 "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." +done +STUB_CONFIG=$'merge:\n diff-check:\n path-holds: true' +expect "path holds on holds a floor path" 1 3 "it changes .github/scripts/pr-merge/clear-escalations.mjs, a hold path" +STUB_FILES='[["docs/guide.md"]]' +STUB_CONFIG=$'merge:\n diff-check:\n denied-paths: docs/held/**\n path-holds: true' expect "denied paths that are not a list fail closed" 1 3 "the hold paths could not be read" -STUB_CONFIG=$'merge:\n diff-check:\n denied-paths: [docs/held/**]' +STUB_CONFIG=$'merge:\n diff-check:\n denied-paths: [docs/held/**]\n path-holds: true' STUB_PULL="$(STUB_CHANGED=2 pull 'Closes #1')" expect "a file list shorter than changed_files fails closed before the discussion" 1 0 "::error::GitHub listed fewer files than #7 changes" STUB_PULL="$(pull 'Closes #1')"