From 9fc496246e3c970b5e6e329a01c7fa991c6ae918 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:03:37 -0400 Subject: [PATCH 01/13] feat(pr-pipeline): merge lanes App pull requests through native auto-merge ci-status and enable-auto-merge now require an exact-head pr-verify pass (#9450), post-merge judges a native merge (#9459), and the native path applies the hold floor (#9488), so the gate no longer needs to list lanes App pull requests. Co-Authored-By: Claude Opus 5.5 --- docs/conventions/pr-pipeline.yaml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index b595e9423c..56bb43a716 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -787,11 +787,8 @@ merge: # (the enable-auto-merge activity arms them, ci-status gates them), and the # gate below neither lists, scores nor enqueues them. gate: the gate also # lists, scores and enqueues them. Owner PRs and Dependabot's are gated in - # both. Absent is native. This repository stays on gate until ci-status or - # enable-auto-merge requires the pr-verify verdict and the post-merge check - # judges a native merge with no pin comment: until then the gate is the only - # place either holds for a lanes App PR. - lane-prs: gate + # both. Absent is native. + lane-prs: native # A PR the gate lists merges only with a passing pr-verify.yml verdict at its # head (lane PRs under lane-prs native are not listed). Must be true at a # rung above off: the gate refuses otherwise. From 9f835fd18b1f5f4b04a06468bded831e9d8af884 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:47:18 -0400 Subject: [PATCH 02/13] feat(pr-pipeline): arm Refs lane pull requests on the gate's link rule enable-auto-merge no longer requires a body opening with Closes. The scheduled reader runs babysit_verify_evidence.py evidence --lane-link, which reads the issue link by babysit_merge_policy._linked_issue (one closing issue here, or one plain Refs: issue) before the verdict, and arms only when both allow. A refused or unreadable link reads no verdict and leaves the pull request unarmed or disarms it. Event runs read no link, so an armed Refs pull request is never disarmed on one. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 7 ++ .../feat-native-lane-refs-arm-7d1f3b96.md | 7 ++ .../scripts/pr-merge/enable-auto-merge.mjs | 41 ++++--- .../pr-merge/enable-auto-merge.test.mjs | 111 ++++++++++++++++-- .github/workflows/pr-merge.yml | 10 +- docs/conventions/pr-pipeline.yaml | 5 +- docs/conventions/pr-pipeline/README.md | 16 +-- .../scripts/babysit_verify_evidence.py | 51 ++++++-- .../tests/test_babysit_verify_evidence.py | 88 ++++++++++++++ 9 files changed, 284 insertions(+), 52 deletions(-) create mode 100644 .changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md create mode 100644 .changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md new file mode 100644 index 0000000000..ceeaf68af7 --- /dev/null +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Changed + +- The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. diff --git a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md new file mode 100644 index 0000000000..ee4a02e14c --- /dev/null +++ b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Added + +- `babysit_verify_evidence.py evidence --lane-link` first reads the pull request's issue link by the pr-merge lane's own rule (`babysit_merge_policy._linked_issue`: one closing issue in this repository, or, with none, the one issue its plain `Refs:` lines name) and adds `ineligible` to its line: null when the lane takes the link, else why not, with refusal `verify:ineligible` and no verdict or config read. A failed read refuses. The verdict read that follows takes the link read's pull request instead of reading it again. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index df063e5fec..51931949f3 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -8,8 +8,6 @@ // A pull request is armed only when all of these hold: // - The lanes App opened it (merge.agent-identity), into main, from a branch // of this repository, and it is not a draft. -// - Its body opens with `Closes #`: a `Refs:` pull request is merged -// by a person (report-outcome.mjs). // - It carries no hold: no `do-not-merge` and no human-gated label (the // tracker's role label, `needs-human` by default), and no body hold // (merge-holds.mjs). @@ -26,12 +24,17 @@ // decision: merge-holds.costHold and costDecision, which ci-status runs too. // - No person turned its auto-merge off: the newest auto-merge-disabled event // by a user account keeps it off until a person turns it on again. -// - pr-merge's gate would take its verifier verdict at the head just read: -// babysit_verify_evidence.py `evidence` refuses nothing (verifyHold), so a -// pr-verify PASS or PASS-NOTES stands at that head, no FAIL is at it, no -// earlier FAIL is uncleared, FAILs sit at fewer than merge.max-verify-fails -// heads, and the spec is unchanged and trusted. Only the scheduled run and -// a hand-started one read it (READER_EVENTS), last, for a pull request +// - pr-merge's gate would take its issue link and its verifier verdict at the +// head just read: babysit_verify_evidence.py `evidence --lane-link` refuses +// nothing (verifyHold). The link is the gate's own rule +// (babysit_merge_policy._linked_issue): one closing issue in this +// repository, or, with none, the one issue its plain `Refs: #` lines +// name. Then a pr-verify PASS or PASS-NOTES stands at that head, no FAIL is +// at it, no earlier FAIL is uncleared, FAILs sit at fewer than +// merge.max-verify-fails heads, and the spec is unchanged and trusted. A +// read that fails, or a line without the link's answer, refuses. Only the +// scheduled run and +// a hand-started one read them (READER_EVENTS), last, for a pull request // every other rule would arm or keep armed, sharing one artifact listing // and run cache in the run. An event run (the issue lane, a person's edit, // unlabel or ready) never arms and reads no verdict: it only disarms on a @@ -78,7 +81,6 @@ const SHA = /^[0-9a-f]{40}$/; const BOT_LOGIN = /^([a-z0-9][a-z0-9-]*)\[bot\]$/; // babysit_merge_policy.MERGE_HOLD_LABELS, plus the human-gated label. const HOLD_LABELS = Object.freeze(["do-not-merge"]); -const CLOSES = /^Closes #[1-9][0-9]*\r?$/; // 100 pull requests a page; more than this many open is a listing the run // stops on rather than one it reads only part of. const MAX_PAGES = 10; @@ -138,13 +140,7 @@ export function holdReason(pull, humanGated) { return `it carries ${label}`; } const body = String(pull.body ?? ""); - if (BODY_HOLD.test(body)) { - return "its body holds it"; - } - if (!CLOSES.test(body.split("\n")[0])) { - return "its body does not open with Closes #"; - } - return null; + return BODY_HOLD.test(body) ? "its body holds it" : null; } const whole = (connection) => Array.isArray(connection?.nodes) && connection.totalCount <= connection.nodes.length; @@ -194,7 +190,7 @@ async function changedFiles(github, slug, number) { return null; } -// Why the gate's verifier `evidence` (babysit_verify_evidence.py's report) +// Why the gate's `evidence --lane-link` (babysit_verify_evidence.py's report) // keeps a pull request at `head` from merging, or null when it allows it. export function verifyHold(evidence, head) { if (evidence === null || typeof evidence !== "object" || !("refusal" in evidence)) { @@ -203,7 +199,14 @@ export function verifyHold(evidence, head) { if (evidence.head !== head) { return "its verifier verdict was read at another head"; } - return evidence.refusal === null ? null : `pr-merge's gate refuses its verifier verdict (${escapeCommand(evidence.refusal)})`; + if (typeof evidence.ineligible === "string") { + return `pr-merge's lane does not take its issue link (${escapeCommand(evidence.ineligible)})`; + } + if (evidence.refusal !== null) { + return `pr-merge's gate refuses its verifier verdict (${escapeCommand(evidence.refusal)})`; + } + // A line without the link's answer read no link: it allows nothing. + return evidence.ineligible === null ? null : "its issue link could not be read"; } // The gate's own reader, one process per pull request; the run shares its @@ -214,7 +217,7 @@ export function pythonVerify(env) { throw new Error("its base commit is not a commit SHA"); } // --base-sha: the gate's FAIL cap, the lower of the base's and main's. - const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "evidence", "--repo", env.GITHUB_REPOSITORY, "--base-sha", base]; + const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "evidence", "--lane-link", "--repo", env.GITHUB_REPOSITORY, "--base-sha", base]; const done = spawnSync("python3", [...args, String(number), head], { encoding: "utf8", env, timeout: VERIFY_TIMEOUT_MS }); if (done.status !== 0) { throw new Error(`babysit_verify_evidence.py exited ${done.status ?? done.signal}: ${String(done.stderr ?? "").trim().split("\n").at(-1)}`); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 240fd235cb..17f3f20694 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -1,10 +1,11 @@ // Expected values come from the native-merge plan's arm-merge item and the // owner answers on it: a lanes App pull request is armed for native auto-merge (squash, at the head // read) with no session, and is never armed, or is disarmed, while it carries -// a `Hold:` line, do-not-merge or the human-gated label. A Refs pull request -// is merged by a person (report-outcome.mjs), so it is never armed. The body -// hold is the gate's own rule, babysit_merge_policy.BODY_MERGE_HOLD_RE, so the -// parity test reads its verdicts from Python. +// a `Hold:` line, do-not-merge or the human-gated label. Its issue link is +// the gate's own rule (docs/conventions/pr-pipeline/README.md, "Eligibility": +// one closing issue, or one plain `Refs: #` issue), read through the gate's +// Python, as is the body hold, babysit_merge_policy.BODY_MERGE_HOLD_RE, so +// the parity tests read their verdicts from Python. import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; import { mkdtempSync, writeFileSync } from "node:fs"; @@ -73,7 +74,7 @@ const said = (login, body, at, association = "OWNER", typename = "User") => ({ createdAt: at, authorAssociation: association, }); -const PASSED = { refusal: null, head: HEAD }; +const PASSED = { refusal: null, head: HEAD, ineligible: null }; const COST_LINK = `https://github.com/${REPO}/issues/1040#issuecomment-7`; const COST_COMMENT = { issue_url: `https://api.github.com/repos/${REPO}/issues/1040`, @@ -130,7 +131,7 @@ function world(pulls, { failArm = false } = {}) { if (found?.verdict instanceof Error) { throw found.verdict; } - return found?.verdict ?? { refusal: null, head }; + return found?.verdict ?? { refusal: null, head, ineligible: null }; }; const writes = () => calls @@ -191,15 +192,13 @@ test("an armed pull request that now holds is disarmed", async () => { ]); }); -test("only the lanes App's ready Closes pull requests from this repository are armed", async () => { +test("only the lanes App's ready pull requests from this repository are armed", async () => { const { writes } = await run([ pull(1, { author: { __typename: "User", login: "kyle-sexton" } }), pull(2, { author: { __typename: "Bot", login: "dependabot" } }), pull(3, { author: { __typename: "User", login: "melodic-automation-lanes" } }), pull(4, { isDraft: true }), pull(5, { isCrossRepository: true }), - pull(6, { body: "Refs: #12\n\n## Summary" }), - pull(7, { body: "## Summary\n\nCloses #12" }), pull(8, { headRefOid: "abc" }), pull(9, { labels: { totalCount: 101, nodes: [] } }), ]); @@ -275,7 +274,7 @@ test("the body hold agrees with the gate's BODY_MERGE_HOLD_RE", () => { // The body the issue lane opens its pull request with (report-outcome.mjs // decide), so a change to either side that stops a clean lane pull request // from arming, or arms a held one, fails here. -test("report-outcome's own pull request body arms, and its hold and Refs bodies do not", () => { +test("report-outcome's own Closes and Refs pull request bodies arm, and its hold bodies do not", () => { const plan = (overrides) => decideOutcome({ outcome: { exit: "done", reason: "", title: "docs(work-items): fix a typo", summary: "Fixed it." }, @@ -296,7 +295,7 @@ test("report-outcome's own pull request body arms, and its hold and Refs bodies assert.deepEqual(verdict({}), { action: "arm", reason: "no hold" }); assert.equal(verdict({ humanMerge: true }).action, "none"); assert.equal(verdict({ changedFiles: [".github/workflows/x.yml"] }).action, "none"); - assert.equal(verdict({ outcome: { exit: "done", reason: "", title: "t", summary: "s", link: "refs" } }).action, "none"); + assert.deepEqual(verdict({ outcome: { exit: "done", reason: "", title: "t", summary: "s", link: "refs" } }), { action: "arm", reason: "no hold" }); }); // The gate's human veto (babysit_merge.human_veto_logins): each member's @@ -515,7 +514,11 @@ test("verifyHold allows only the reader's null refusal at the head read", () => assert.match(verifyHold({ refusal: null, head: "c".repeat(40) }, HEAD), /another head/); assert.match(verifyHold(null, HEAD), /could not be read/); assert.match(verifyHold({ head: HEAD }, HEAD), /could not be read/); - assert.match(verifyHold({ refusal: "verify:too-old", head: HEAD }, HEAD), /verify:too-old/); + assert.match(verifyHold({ refusal: "verify:too-old", head: HEAD, ineligible: null }, HEAD), /verify:too-old/); + // The link's answer: a refusal names it, and a line without one allows nothing. + assert.match(verifyHold({ refusal: "verify:ineligible", head: HEAD, ineligible: "refs-issues:2" }, HEAD), /does not take its issue link \(refs-issues:2\)/); + assert.match(verifyHold({ refusal: null, head: HEAD }, HEAD), /issue link could not be read/); + assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: false }, HEAD), /issue link could not be read/); }); // The merge queue's check (merge-holds.mjs verdictHold): a pass counts only @@ -600,7 +603,8 @@ test("a reader that fails, prints no JSON or reports another head arms nothing a exits: "import sys\nsys.stderr.write('boom\\n')\nsys.exit(1)\n", garbage: "print('not json')\n", other: `import json\nprint(json.dumps({"refusal": None, "head": "${"c".repeat(40)}"}))\n`, - passes: `import json, sys\nassert sys.argv[-4:-2] == ["--base-sha", "${"d".repeat(40)}"], sys.argv\nprint(json.dumps({"refusal": None, "head": sys.argv[-1]}))\n`, + unlinked: `import json, sys\nprint(json.dumps({"refusal": None, "head": sys.argv[-1]}))\n`, + passes: `import json, sys\nassert sys.argv[2] == "--lane-link" and sys.argv[-4:-2] == ["--base-sha", "${"d".repeat(40)}"], sys.argv\nprint(json.dumps({"refusal": None, "head": sys.argv[-1], "ineligible": None}))\n`, }; for (const [name, body] of Object.entries(scripts)) { writeFileSync(join(dir, `${name}.py`), body); @@ -610,6 +614,7 @@ test("a reader that fails, prints no JSON or reports another head arms nothing a ["exits", [{ mutation: "disarm", id: "PR_2" }]], ["garbage", [{ mutation: "disarm", id: "PR_2" }]], ["other", [{ mutation: "disarm", id: "PR_2" }]], + ["unlinked", [{ mutation: "disarm", id: "PR_2" }]], ["passes", [{ mutation: "arm", id: "PR_1", head: HEAD }]], ]) { const { github, writes } = world([pull(1), pull(2, { autoMergeRequest: armed })]); @@ -624,3 +629,83 @@ test("a reader that fails, prints no JSON or reports another head arms nothing a assert.deepEqual(writes(), want, name); } }); + +// The issue link through the gate's own CLI (`evidence --lane-link`) and rule +// (babysit_merge_policy._linked_issue), GitHub's answers stubbed at gh and the +// verdict a pass at the head, so only the link decides. Expected values are +// docs/conventions/pr-pipeline/README.md "Eligibility": one closing issue, or +// one plain `Refs: #` issue; two `Refs:` issues refuse; a failed read refuses. +test("the gate's link rule arms a Closes or one plain Refs pull request, not two Refs or a failed read", async () => { + const dir = mkdtempSync(join(tmpdir(), "enable-auto-merge-link-")); + const wrapper = join(dir, "lane_link.py"); + writeFileSync( + wrapper, + `import json, os, subprocess, sys +sys.path.insert(0, os.environ["SCRIPTS"]) +from unittest import mock +import babysit_merge_policy as policy +import babysit_postmerge as postmerge +import babysit_verify_evidence as ve + +PULLS = json.loads(os.environ["PULLS"]) + +def gh(args, **_): + n = next(a for a in args if a.startswith("n="))[2:] + def ok(data): + return subprocess.CompletedProcess(args, 0, json.dumps({"data": {"repository": data}}), "") + if any("issueOrPullRequest" in a for a in args): + node = {"__typename": "Issue", "number": int(n), "repository": {"nameWithOwner": os.environ["GITHUB_REPOSITORY"]}} + return ok({"issueOrPullRequest": node}) + found = PULLS[n] + if found.get("error"): + return subprocess.CompletedProcess(args, 1, "", "gh: boom (HTTP 502)") + nodes = [{"number": c, "repository": {"nameWithOwner": os.environ["GITHUB_REPOSITORY"]}} for c in found["closing"]] + return ok({"pullRequest": {"body": found["body"], "closingIssuesReferences": {"totalCount": len(nodes), "nodes": nodes}}}) + +def passes(self, number, head, pr=None): + return ve.Evidence(None, head, (ve.Verdict(head, "pass", 1),), 0) + +argv = sys.argv[1:] +cut = argv.index("--base-sha") +del argv[cut : cut + 2] +with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "evidence", passes): + sys.exit(ve.main(argv)) +`, + ); + const body = (text) => `${text}\n\n## Summary\n\nDone.`; + const pulls = { + 1: { body: body("Closes #11"), closing: [11] }, + 2: { body: body("Refs: #12"), closing: [] }, + 3: { body: body("Refs: #12\nRefs: #13"), closing: [] }, + 4: { body: body("Refs: #12"), closing: [], error: true }, + 5: { body: body("Refs: #12\nRefs: #13"), closing: [] }, + }; + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const { github, writes } = world([1, 2, 3, 4].map((n) => pull(n, { body: pulls[n].body })).concat(pull(5, { body: pulls[5].body, autoMergeRequest: armed }))); + const scripts = fileURLToPath(new URL("../../../plugins/source-control/skills/babysit-prs/scripts", import.meta.url)); + const env = { ...ENV, PATH: process.env.PATH, SCRIPTS: scripts, PULLS: JSON.stringify(pulls), VERIFY_EVIDENCE_SCRIPT: wrapper }; + const lines = []; + const log = console.log; + console.log = (line) => lines.push(line); + try { + assert.equal(await main({ env, github }), 0); + } finally { + console.log = log; + } + assert.deepEqual(writes(), [ + { mutation: "arm", id: "PR_1", head: HEAD }, + { mutation: "arm", id: "PR_2", head: HEAD }, + { mutation: "disarm", id: "PR_5" }, + ]); + assert.ok(lines.some((line) => /#3 left: pr-merge's lane does not take its issue link \(refs-issues:2\)/.test(line)), lines.join("\n")); + assert.ok(lines.some((line) => /#4 left: pr-merge's lane does not take its issue link \(unreadable: /.test(line)), lines.join("\n")); +}); + +// An event run reads no link and no verdict, so it never disarms an armed +// Refs pull request on its link; the scheduled run judges it. +test("an event run leaves an armed Refs pull request armed", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const { writes, verified } = await run([pull(1, { autoMergeRequest: armed, body: "Refs: #12\n\n## Summary" })], { EVENT_NAME: "pull_request_target" }); + assert.deepEqual(writes, []); + assert.deepEqual(verified, []); +}); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 6bc5e4936a..cfaffe0474 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1312,16 +1312,18 @@ jobs: fi # Turns on GitHub's native auto-merge for each open lanes App pull request - # into main that closes an issue and carries no hold, and turns it off on + # into main that carries no hold, and turns it off on # one that now carries a hold (a `Hold:` line, do-not-merge, the human-gated # label). Every event is a wake-up: the script lists the pull requests # itself. The issue lane finishing arms the pull request it opened; a listed # human's edit, unlabel or ready disarms or arms at once; the schedule # catches what the events missed, such as a run the lanes App started, which # the broker refuses. Only the scheduled run and a hand-started one arm, on - # the verdict pr-merge's gate would take at the head (babysit_verify_evidence.py, - # read with the job's GITHUB_TOKEN from the head-ref map and run cache the - # last gate run kept); event runs read no verdict and only disarm on a hold. + # the issue link (one closing issue, or one plain `Refs:` issue) and verdict + # pr-merge's gate would take at the head (babysit_verify_evidence.py + # `evidence --lane-link`, its Actions reads with the job's GITHUB_TOKEN from + # the head-ref map and run cache the last gate run kept); event runs read + # neither and only disarm on a hold. # Its own job and token: the broker mints one token per # job. Rules: .github/scripts/pr-merge/enable-auto-merge.mjs. enable-auto-merge: diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index 56bb43a716..d7eaa7f35b 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -390,8 +390,9 @@ activities: inputs: [pr, head-sha] enable-auto-merge: # Turns on GitHub's native auto-merge (squash, at the head it read) for - # each open lanes App pull request into main that opens with `Closes #` - # and carries no hold (a `Hold:` line, do-not-merge, the human-gated + # each open lanes App pull request into main whose issue link the merge + # gate takes (one closing issue, or one plain `Refs: #` issue) and + # that carries no hold (a `Hold:` line, do-not-merge, the human-gated # label, a member's "do not merge" comment, a head that changes a denied # path), and turns it off on an armed one that now carries a hold; # auto-merge a person turned off stays off. No diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 03c8799d3b..d29d490290 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1056,19 +1056,21 @@ drops an escalation. The lane's `enable-auto-merge` activity, the `enable-auto-merge` job of `pr-merge.yml` with its own lanes App token (`.github/scripts/pr-merge/enable-auto-merge.mjs`, no model), turns on GitHub's native auto-merge (`enablePullRequestAutoMerge`, squash, `expectedHeadOid` the head it read) for -each open lanes App pull request into main from a branch of this repository that is not a draft, -opens with `Closes #`, and carries no hold: no `Hold:` line or "do not merge" in its body +each open lanes App pull request into main from a branch of this repository that is not a draft +and carries no hold: no `Hold:` line or "do not merge" in its body (the gate's own rule), no `do-not-merge` or human-gated label, and no member (owner, member or collaborator) whose newest comment, review or inline comment with text says "do not merge" (the gate's veto rule; a later comment lifts it), and whose head changes no path in the lane's `merge.diff-check.denied-paths`, read on the files the head changes at each run, and whose -verifier verdict at that head pr-merge's gate would take: `babysit_verify_evidence.py evidence` -refuses nothing, so a PASS or PASS-NOTES of [pr-verify](#verifier-verdict) stands at the head, +issue link and verifier verdict at that head pr-merge's gate would take: +`babysit_verify_evidence.py evidence --lane-link` refuses nothing, so the link is one the gate's +[eligibility](#the-pr-merge-lane) rule takes (one closing issue in this repository, or, with none, +the one issue its plain `Refs: #` lines name; a failed read refuses), a PASS or PASS-NOTES of [pr-verify](#verifier-verdict) stands at the head, no FAIL is at it, no earlier FAIL is uncleared, FAILs sit at fewer than `merge.max-verify-fails` heads, and the spec is unchanged and trusted. Only the scheduled run and a hand-started one read -it and arm, last, for a pull request every other rule would arm or keep armed, with the gate's -saved listing, head-ref map and run cache; an event run reads no verdict, never arms, and only -disarms on a hold, so a pull request waits up to one scheduled interval to be armed. `ci-status` +them and arm, last, for a pull request every other rule would arm or keep armed, with the gate's +saved listing, head-ref map and run cache; an event run reads no link or verdict, never arms, and +only disarms on a hold, so a pull request waits up to one scheduled interval to be armed. `ci-status` turns an armed lanes App pull request's auto-merge off on each push to its head, so a new head is armed only after the reader judges it. It turns auto-merge off on an armed one that now fails any of those, and does not turn it back on once a person has turned it off. GitHub then merges the pull request through the merge queue 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 2a6a09b1b7..3e1be86838 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 @@ -210,6 +210,9 @@ def verdict_name(name: str) -> re.Match[str] | None: TOO_OLD = "verify:too-old" UNTRUSTED_SPEC = "verify:untrusted-spec" SPEC_CHANGED = "verify:spec-changed" +# `evidence --lane-link`: the pull request's issue link is not one the +# pr-merge lane takes (`Reader.link_refusal`), so no verdict is read. +INELIGIBLE = "verify:ineligible" # A verdict's `intent` when its pull request closed no issue in this # repository: the title and body were the spec (write-verdict.mjs). INTENT_PULL_REQUEST = "pull-request" @@ -505,6 +508,9 @@ class Reader: _mapped: dict[int, str] | None = None # the head-ref map, once loaded _humans: frozenset[int] | None = None _humans_at: datetime | None = None # when `_humans` was read + # The pull requests `link_refusal` read, each taken by the next + # `_closing_issues` instead of reading it again. + _linked: dict[int, dict[str, Any]] = field(default_factory=dict) def _listing(self) -> list[dict[str, Any]]: """`_current_listing`, noting when it began to be read @@ -947,7 +953,7 @@ def _closing_issues(self, number: int) -> dict[Any, Any]: body's lastEditedAt, read as the resolver reads them (closingIssuesReferences, first 10), or, when it closes none, the one issue its body's `Refs:` lines name (`merge_policy.refs_issue`).""" - pull = json_object( + pull = self._linked.pop(number, None) or json_object( self._graphql(CLOSING_QUERY, number, "closing issues").get("pullRequest") ) refs = json_object(pull.get("closingIssuesReferences")) @@ -985,6 +991,25 @@ def _closing_issues(self, number: int) -> dict[Any, Any]: ) return live + def link_refusal(self, number: int) -> str | None: + """Why the pull request's issue link is not one the pr-merge lane + takes from a pull request the lane opened, or None when it is: the + gate's own rule (`merge_policy._linked_issue`, as `lane_eligibility` + reads it), so one closing issue in this repository, or, with none, + the one issue its plain `Refs:` lines name. A failed read refuses.""" + try: + pull = json_object( + self._graphql(CLOSING_QUERY, number, "closing issues").get( + "pullRequest" + ) + ) + _, refusal = merge_policy._linked_issue(pull, self.repo) + except (_Unreadable, merge_policy.PolicyReadError) as exc: + return f"unreadable: {exc}" + if refusal is None: + self._linked[number] = pull + return refusal + def _closers(self, live: dict[Any, Any]) -> set[int]: """The pull requests in this repository, open or closed, that close any of the closing issues `live` holds.""" @@ -1411,7 +1436,12 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: of the config at that base commit and at the default branch head, the lower one (`with_default_branch`); a config that cannot be read refuses `verify:unreadable`. A pass judged on the run's saved listing is judged - again on a listing of its own, as the gate does before it allows.""" + again on a listing of its own, as the gate does before it allows. + + With `--lane-link`, the line also carries `ineligible`: null when the + pull request's issue link is one the pr-merge lane takes + (`Reader.link_refusal`), else why not, with refusal `verify:ineligible` + and no verdict or config read.""" parser = argparse.ArgumentParser( prog="babysit_verify_evidence.py", allow_abbrev=False ) @@ -1421,6 +1451,7 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: cmd.add_argument("--branch", default="main") cmd.add_argument("--max-fails", type=int, default=merge_policy.MAX_VERIFY_FAILS) cmd.add_argument("--base-sha", default=None) + cmd.add_argument("--lane-link", action="store_true") cmd.add_argument("number", type=int) cmd.add_argument("head") args = parser.parse_args(argv) @@ -1435,7 +1466,14 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: parser.error( "--max-fails must be 1 or more, and head and --base-sha full commit SHAs" ) - max_fails = args.max_fails + reader = Reader(args.repo, args.branch, now=now, max_fails=args.max_fails) + link: dict[str, Any] = {} + if args.lane_link: + link["ineligible"] = reader.link_refusal(args.number) + if link["ineligible"] is not None: + refusal = Evidence(INELIGIBLE, args.head, (), 0, link["ineligible"]) + print(json.dumps(refusal.report() | link)) + return 0 if args.base_sha is not None: try: base = merge_policy.read_config_at(args.repo, args.base_sha) @@ -1444,15 +1482,14 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: config = merge_policy.with_default_branch(args.repo, base) except merge_policy.PolicyReadError as exc: refusal = Evidence(UNREADABLE, args.head, (), 0, f"config: {exc}") - print(json.dumps(refusal.report())) + print(json.dumps(refusal.report() | link)) return 0 - max_fails = config.max_verify_fails - reader = Reader(args.repo, args.branch, now=now, max_fails=max_fails) + reader.max_fails = config.max_verify_fails evidence = reader.evidence(args.number, args.head) if evidence.refusal is None and reader.saved: reader.refresh() evidence = reader.evidence(args.number, args.head) - print(json.dumps(evidence.report())) + print(json.dumps(evidence.report() | link)) return 0 diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py index e09d28b2cf..cdf870f140 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py @@ -4120,5 +4120,93 @@ def test_an_unreadable_config_refuses(self) -> None: self.assertEqual(json.loads(out.getvalue())["refusal"], ve.UNREADABLE) +class EvidenceCommandReadsTheLaneLink(unittest.TestCase): + """`evidence --lane-link`, which enable-auto-merge.mjs runs: the issue + link the pr-merge lane takes from its own pull request, by the rule in + docs/conventions/pr-pipeline/README.md ("Eligibility"): one closing issue + here, or, with none, the one issue plain `Refs: #` lines name; two + issues refuse `refs-issues:2`, a pull request or a number naming nothing + `refs-not-an-issue`, and a failed read refuses. A refused link reads no + verdict.""" + + def cli(self, fake: FakeGitHub) -> dict[str, Any]: + out = io.StringIO() + with ( + mock.patch.object(policy, "pull_request", return_value=pull()), + mock.patch.object(postmerge, "gh_capture", side_effect=fake), + mock.patch.object(policy, "gh_capture", side_effect=fake), + contextlib.redirect_stdout(out), + ): + argv = ["evidence", "--lane-link", "--repo", REPO, str(PR), HEAD] + self.assertEqual(ve.main(argv, now=NOW), 0) + return json.loads(out.getvalue()) + + def refs(self, body: str, **refs: str) -> FakeGitHub: + fake = passing() + fake.closing, fake.body = [], body + fake.refs = {int(n.removeprefix("n")): kind for n, kind in refs.items()} + return fake + + def test_a_closing_pull_request_is_linked_and_its_pass_allows(self) -> None: + fake = passing() + report = self.cli(fake) + self.assertEqual((report["ineligible"], report["refusal"]), (None, None)) + # The verdict read takes the link read's pull request: one read. + closing = [c for c in fake.calls if any("closingIssues" in a for a in c)] + self.assertEqual(len(closing), 1) + + def test_one_plain_refs_line_is_linked_and_its_verdict_is_read(self) -> None: + fake = self.refs("Refs: #7\n\n## Summary", n7=REPO) + report = self.cli(fake) + self.assertIsNone(report["ineligible"]) + self.assertEqual(listings(fake), 1) + + def test_a_refused_link_reads_no_verdict(self) -> None: + for body, refs, want in ( + ("Refs: #7\nRefs: #8\n", {"n7": REPO, "n8": REPO}, "refs-issues:2"), + ("Refs: #7, #8\n", {"n7": REPO}, "refs-line-not-plain"), + ("refs: #7\n", {"n7": REPO}, "refs-line-not-plain"), + ("Refs: #7\n", {"n7": "PullRequest"}, "refs-not-an-issue"), + ("Refs: #9\n", {}, "refs-not-an-issue"), + ("## Summary\n", {}, "linked-issues:0"), + ): + with self.subTest(body=body): + fake = self.refs(body, **refs) + report = self.cli(fake) + self.assertEqual( + (report["ineligible"], report["refusal"]), (want, ve.INELIGIBLE) + ) + self.assertEqual(listings(fake), 0) + + def test_a_closing_issue_elsewhere_or_two_refuse(self) -> None: + for closing, want in ( + ([(7, REPO), (8, REPO)], "linked-issues:2"), + ([(7, "other-org/other-repo")], "linked-issue-other-repository"), + ): + fake = passing() + fake.closing = closing + self.assertEqual(self.cli(fake)["ineligible"], want) + + def test_a_failed_link_read_refuses(self) -> None: + for fake in (passing(), self.refs("Refs: #7\n", n7="error")): + if not fake.refs: + fake.fail.add("graphql") + report = self.cli(fake) + self.assertTrue(str(report["ineligible"]).startswith("unreadable: ")) + self.assertEqual(report["refusal"], ve.INELIGIBLE) + self.assertEqual(listings(fake), 0) + + def test_without_the_flag_no_link_is_read(self) -> None: + fake = self.refs("Refs: #7\nRefs: #8\n") + out = io.StringIO() + with ( + mock.patch.object(policy, "pull_request", return_value=pull()), + mock.patch.object(postmerge, "gh_capture", side_effect=fake), + contextlib.redirect_stdout(out), + ): + ve.main(["evidence", "--repo", REPO, str(PR), HEAD], now=NOW) + self.assertNotIn("ineligible", json.loads(out.getvalue())) + + if __name__ == "__main__": unittest.main() From 174741149bedf030b28bf34baa40b8666735f3a9 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:47:56 -0400 Subject: [PATCH 03/13] ci(pr-merge): log the API budget at the end of the enable-auto-merge job The job logged no api-budget line. Its arm step now names the lane and job tokens' readings for the reader's gh calls, and a job-end log-api-budget step, as the merge job has, reports both. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/workflow.test.mjs | 16 ++++++++++++++++ .github/workflows/pr-merge.yml | 14 ++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 713a75f129..4ae2556b1d 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -171,6 +171,22 @@ test("the last verifier read enable-auto-merge starts ends inside its step's tim assert.ok(VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS < Number(step[1]) * 60_000); }); +test("enable-auto-merge logs both tokens' readings at job end, before the token is revoked", () => { + const arm = job("enable-auto-merge"); + const block = /- name: Log the API budget at job end\n([\s\S]*?)(?=\n {6}- name: |$)/.exec(arm)?.[1]; + assert.ok(block, "a job-end budget step"); + assert.match(block, /\n {8}uses: \.\/\.base\/\.github\/actions\/log-api-budget /); + assert.match(block, /^ {8}id: budget-end\n {8}if: always\(\) && steps\.lane-token\.outputs\.token != ''\n/); + assert.match(block, /\n {8}continue-on-error: true\n {8}timeout-minutes: 1\n/); + assert.match(block, /\n {10}tokens: lanes-app github-token\n/); + // The reader's gh calls read into both names. + const step = /- name: Arm and disarm auto-merge\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)[1]; + assert.match(step, /\n {10}API_BUDGET_TOKEN: lanes-app\n {10}API_BUDGET_OVERRIDE_TOKEN: github-token\n/); + const names = [...arm.matchAll(/\n {6}- name: (.*)/g)].map((m) => m[1]); + assert.ok(names.indexOf("Arm and disarm auto-merge") < names.indexOf("Log the API budget at job end")); + assert.ok(names.indexOf("Log the API budget at job end") < names.indexOf("Revoke the lane token")); +}); + test("enable-auto-merge asks the broker for its own activity's merge token", () => { const arm = job("enable-auto-merge"); const steps = [...arm.matchAll(/\n {10}lane: (.*)\n {10}activity: (.*)\n/g)].map((m) => `${m[1]}/${m[2]}`); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index cfaffe0474..06bcad4a6a 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1461,6 +1461,11 @@ jobs: BABYSIT_VERIFY_LISTING: ${{ runner.temp }}/verify-listing.json BABYSIT_VERIFY_HEAD_REFS: ${{ runner.temp }}/head-refs-in.json BABYSIT_RUN_CACHE: ${{ runner.temp }}/babysit-run-cache.json + # Name the two tokens' rate-limit readings for the reader's gh calls + # (api-budget.bash): the lane token by default, the job token for + # the Actions reads that carry it. + API_BUDGET_TOKEN: lanes-app + API_BUDGET_OVERRIDE_TOKEN: github-token run: | set -euo pipefail config=.base/docs/conventions/pr-pipeline.yaml @@ -1469,6 +1474,15 @@ jobs: export REPORT_ONLY LANES_BOT_LOGIN node .base/.github/scripts/pr-merge/enable-auto-merge.mjs + - name: Log the API budget at job end + id: budget-end + if: always() && steps.lane-token.outputs.token != '' + continue-on-error: true + timeout-minutes: 1 + uses: ./.base/.github/actions/log-api-budget # zizmor: ignore[self-repository] + with: + tokens: lanes-app github-token + - name: Revoke the lane token if: always() && steps.lane-token.outputs.token != '' continue-on-error: true From 676bfd0c4793a89366451b302d38e03c7a0e70d5 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:29:01 -0400 Subject: [PATCH 04/13] feat(pr-pipeline): hold native arming on the gate's demotion, rung and in-flight cap enable-auto-merge skipped the gate's run-level refusals, so after a demotion the next scheduled run re-armed clean lane pull requests. Each reader run now runs the post-merge check the merge job runs and reads the lane's room from it through babysit_merge_policy.lane_room. While the gate holds the lane off nothing is armed and armed lane pull requests are disarmed; at merge.max-in-flight nothing more is armed; an unreadable room arms nothing and disarms nothing. PULLS_QUERY now notes that it assumes main is the default branch. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 1 + .../feat-native-lane-refs-arm-7d1f3b96.md | 1 + .../scripts/pr-merge/enable-auto-merge.mjs | 102 +++++++- .../pr-merge/enable-auto-merge.test.mjs | 121 ++++++++- .github/scripts/pr-merge/workflow.test.mjs | 21 +- .github/workflows/pr-merge.yml | 40 +++ .../scripts/babysit_merge_policy.py | 102 +++++++- .../scripts/babysit_verify_evidence.py | 17 +- .../scripts/tests/test_babysit_lane_room.py | 230 ++++++++++++++++++ .../tests/test_babysit_merge_policy.py | 2 + 10 files changed, 628 insertions(+), 9 deletions(-) create mode 100644 plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index ceeaf68af7..5f742b15da 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -5,3 +5,4 @@ bump: minor ### Changed - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. +- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run runs the post-merge check, as the merge job does, and reads the lane's room from it (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A room that cannot be read arms nothing and disarms nothing. The job now holds `pull-requests: read` and `issues: read` for those reads. diff --git a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md index ee4a02e14c..68f69e0252 100644 --- a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md +++ b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md @@ -5,3 +5,4 @@ bump: minor ### Added - `babysit_verify_evidence.py evidence --lane-link` first reads the pull request's issue link by the pr-merge lane's own rule (`babysit_merge_policy._linked_issue`: one closing issue in this repository, or, with none, the one issue its plain `Refs:` lines name) and adds `ineligible` to its line: null when the lane takes the link, else why not, with refusal `verify:ineligible` and no verdict or config read. A failed read refuses. The verdict read that follows takes the link read's pull request instead of reading it again. +- `babysit_verify_evidence.py lane-room` prints whether the pr-merge lane may merge one more of its own pull requests now (`babysit_merge_policy.lane_room`): the gate's run-level refusals (another authority, a post-merge refusal, the effective rung off, a demotion included) as `refusal`, a failed read as `unreadable`, and otherwise the merges in flight against `merge.max-in-flight`. `InFlight` now carries that count. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 51931949f3..ff50777aa2 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -43,6 +43,16 @@ // head, so a new head is armed only after the reader judges it, and in the // merge queue it checks the pass again at the head it merges // (merge-holds.mjs verdictHold). +// - pr-merge's gate would let its lane merge one more now: babysit_verify_evidence.py +// `lane-room` (babysit_merge_policy.lane_room) reads, once per reader run, +// the run-level refusals the gate applies to every lane candidate. While +// the gate holds the lane off (a demotion record, merge.rung, the loop +// setting or the rung cap off, a post-merge failure, another authority) +// nothing is armed and every armed lanes App pull request is disarmed. +// Below merge.max-in-flight, counting the merge queue's lane entries, the +// lane's unverified merges and the armed pull requests not yet queued, it +// arms; at the cap it arms nothing more. A room that cannot be read arms +// nothing and disarms nothing. // Arming passes the head just read as expectedHeadOid, so a push in between // fails the arm rather than arming a head nobody read. An armed pull request // that fails any rule is disarmed. While merge.report-only is true it reports @@ -60,6 +70,9 @@ // TRUSTED_ACTORS the base trusted-actors.json, for the owner of a cost // decision (default .base/.github/standards/trusted-actors/trusted-actors.json) // EVENT_NAME the run's event; only READER_EVENTS read verdicts and arm +// POSTMERGE_RESULT this run's babysit_postmerge.py report, for lane-room +// RUNG_CAP vars.CLAUDE_LANES_MERGE_RUNG_CAP (empty is no cap; +// unset holds the lane off, as the gate reads it) // BABYSIT_VERIFY_*, BABYSIT_RUN_CACHE the reader's Actions token (the // job's GITHUB_TOKEN), saved listing, head-ref map and // run cache, as the gate's @@ -89,6 +102,8 @@ const MAX_PAGES = 10; const MAX_FILE_PAGES = 3; const VERIFY_SCRIPT = ".base/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py"; +// Into main, by name: the gate reads the default branch live +// (babysit_merge_policy._base_refusal), while this assumes it is main. export const PULLS_QUERY = `query($owner: String!, $name: String!, $after: String) { repository(owner: $owner, name: $name) { pullRequests(states: OPEN, baseRefName: "main", first: 100, after: $after) { @@ -227,6 +242,46 @@ export function pythonVerify(env) { return verify; } +// The gate's run-level answer for its lane (babysit_verify_evidence.py +// `lane-room`), read with the reader's Actions token when it has one, as the +// merge job reads the post-merge check, so the reads stay off the lane App's +// quota. +export function pythonRoom(env) { + return async () => { + const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "lane-room", "--repo", env.GITHUB_REPOSITORY, "--postmerge-result", env.POSTMERGE_RESULT ?? ""]; + if (env.RUNG_CAP !== undefined) { + args.push("--rung-cap", env.RUNG_CAP); + } + const token = env.BABYSIT_VERIFY_GH_TOKEN; + const roomEnv = token ? { ...env, GH_TOKEN: token, API_BUDGET_TOKEN: "github-token" } : env; + const done = spawnSync("python3", args, { encoding: "utf8", env: roomEnv, timeout: VERIFY_TIMEOUT_MS }); + if (done.status !== 0) { + throw new Error(`babysit_verify_evidence.py lane-room exited ${done.status ?? done.signal}: ${String(done.stderr ?? "").trim().split("\n").at(-1)}`); + } + return JSON.parse(String(done.stdout).trim().split("\n").at(-1)); + }; +} + +// `lane-room`'s answer as the run applies it, or an unreadable one when it is +// not of that shape: { refusal, unreadable, count, cap, queued }. +export function readRoom(answer) { + const text = (value) => value === null || typeof value === "string"; + const whole = (value, least) => Number.isSafeInteger(value) && value >= least; + if ( + answer === null || + typeof answer !== "object" || + !text(answer.refusal) || + !text(answer.unreadable) || + !whole(answer.count, 0) || + !whole(answer.cap, 1) || + !Array.isArray(answer.queued) || + !answer.queued.every((number) => whole(number, 1)) + ) { + return { refusal: null, unreadable: "its answer is not lane-room's", count: 0, cap: 1, queued: [] }; + } + return answer; +} + // The runs that read the verifier verdict and so may arm: the schedule, and a // hand-started run, which a person starts to retry a pull request that looks // stuck. @@ -299,7 +354,7 @@ async function listPulls(github, owner, name) { throw new Error(`more than ${MAX_PAGES * 100} open pull requests into main`); } -export async function main({ env = process.env, github, verify } = {}) { +export async function main({ env = process.env, github, verify, room } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -321,10 +376,47 @@ export async function main({ env = process.env, github, verify } = {}) { const reads = READER_EVENTS.includes(env.EVENT_NAME); const pulls = await listPulls(github, repo[1], repo[2]); const deadline = Date.now() + VERIFY_BUDGET_MS; + const lanes = (pull) => pull.author?.__typename === "Bot" && pull.author.login === slug; + // Only a reader run arms, so only it reads the room. + let lane = null; + if (reads) { + try { + lane = readRoom(await (room ?? pythonRoom(env))()); + } catch (error) { + lane = readRoom(null); + lane.unreadable = escapeCommand(error?.message ?? error); + } + console.log(`enable-auto-merge: lane room: ${escapeCommand(JSON.stringify(lane))}`); + } + // Merges in flight: the gate's count, plus each armed lanes App pull + // request the queue has not taken yet, which GitHub enqueues on its own. + let inFlight = lane === null ? 0 : lane.count + pulls.filter((pull) => lanes(pull) && pull.autoMergeRequest != null && !lane.queued.includes(pull.number)).length; + // Why the room keeps `pull` from arming, with `disarm` when it also turns + // an armed one off; null when it has room. + const roomHold = (pull) => { + if (lane === null || !lanes(pull)) { + return null; + } + if (lane.refusal !== null) { + return { disarm: true, reason: `pr-merge's gate holds its lane off (${lane.refusal})` }; + } + if (pull.autoMergeRequest != null) { + return null; + } + if (lane.unreadable !== null) { + return { disarm: false, reason: `the lane's room could not be read (${lane.unreadable})` }; + } + return inFlight >= lane.cap ? { disarm: false, reason: `merge.max-in-flight is ${lane.cap} and ${inFlight} merges are in flight` } : null; + }; let failed = 0; for (const pull of pulls) { let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }); let detail = null; + // Checked before the reads, so a held lane spends none. + const held = ["read", "verify", "arm"].includes(action) ? roomHold(pull) : null; + if (held !== null) { + ({ action, reason } = { action: held.disarm && pull.autoMergeRequest != null ? "disarm" : "none", reason: held.reason }); + } if (action === "read" && !reads && pull.autoMergeRequest == null) { ({ action, reason } = { action: "none", reason: "only a scheduled or hand-started run arms it, on the gate's verifier verdict" }); } @@ -358,6 +450,14 @@ export async function main({ env = process.env, github, verify } = {}) { } ({ action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence })); } + // A pull request armed earlier in this run counts toward the cap. + const full = action === "arm" ? roomHold(pull) : null; + if (full !== null) { + ({ action, reason } = { action: "none", reason: full.reason }); + } + if (action === "arm") { + inFlight += 1; + } if (action === "none") { if (reason !== "not the lanes App's") { console.log(`enable-auto-merge: #${pull.number} left: ${escapeCommand(reason)}`); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 17f3f20694..a26b8d6939 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -25,6 +25,8 @@ import { decide, READER_EVENTS, main, + pythonRoom, + readRoom, verifyHold, } from "./enable-auto-merge.mjs"; import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; @@ -84,6 +86,9 @@ const COST_COMMENT = { node_id: "IC_7", created_at: "2026-10-22T12:00:00Z", }; +// lane-room's answer for a lane with room: nothing in flight of five. +const ROOM = { refusal: null, unreadable: null, count: 0, cap: 5, queued: [] }; +const room = (answer = ROOM) => async () => answer; const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name) => ({ name })) }); function world(pulls, { failArm = false } = {}) { @@ -140,13 +145,13 @@ function world(pulls, { failArm = false } = {}) { return { github, writes, verify, verified, calls }; } -async function run(pulls, env = {}, options = {}) { +async function run(pulls, env = {}, options = {}, lane = ROOM) { const { github, writes, verify, verified } = world(pulls, options); const lines = []; const log = console.log; console.log = (line) => lines.push(line); try { - const code = await main({ env: { ...ENV, ...env }, github, verify }); + const code = await main({ env: { ...ENV, ...env }, github, verify, room: room(lane) }); return { code, writes: writes(), lines, verified }; } finally { console.log = log; @@ -622,7 +627,7 @@ test("a reader that fails, prints no JSON or reports another head arms nothing a const log = console.log; console.log = () => {}; try { - await main({ env, github }); + await main({ env, github, room: room() }); } finally { console.log = log; } @@ -688,7 +693,7 @@ with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, " const log = console.log; console.log = (line) => lines.push(line); try { - assert.equal(await main({ env, github }), 0); + assert.equal(await main({ env, github, room: room() }), 0); } finally { console.log = log; } @@ -709,3 +714,111 @@ test("an event run leaves an armed Refs pull request armed", async () => { assert.deepEqual(writes, []); assert.deepEqual(verified, []); }); + +// The gate's run-level refusals, which native auto-merge would skip +// (babysit_merge_policy.lane_room): a demotion record or a rung held off +// stops the lane, and merge.max-in-flight caps what is in flight. +test("while the gate holds its lane off, nothing is armed and armed lane pull requests are disarmed", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + for (const refusal of ["rung-off:demotion", "rung-off:rungCap", "post-merge:failure:red-main#40"]) { + const { writes, verified, lines } = await run( + [pull(1), pull(2, { autoMergeRequest: armed }), pull(3, { author: { __typename: "User", login: "kyle-sexton" }, autoMergeRequest: armed })], + {}, + {}, + { ...ROOM, refusal }, + ); + assert.deepEqual(writes, [{ mutation: "disarm", id: "PR_2" }], refusal); + // A held lane reads no verdict. + assert.deepEqual(verified, [], refusal); + assert.ok(lines.some((line) => line.includes(`#1 left: pr-merge's gate holds its lane off (${refusal})`)), refusal); + } +}); + +test("at merge.max-in-flight nothing more is armed, counting armed pull requests the queue has not taken", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + // Four in flight by the gate's count, one armed and not queued: the cap of five is reached. + let result = await run([pull(1), pull(2, { autoMergeRequest: armed })], {}, {}, { ...ROOM, count: 4, cap: 5 }); + assert.deepEqual(result.writes, []); + assert.deepEqual(result.verified, [2]); + assert.ok(result.lines.some((line) => /#1 left: merge\.max-in-flight is 5 and 5 merges are in flight/.test(line))); + // An armed pull request already in the queue is in the gate's count, not counted twice. + result = await run([pull(1), pull(2, { autoMergeRequest: armed })], {}, {}, { ...ROOM, count: 4, cap: 5, queued: [2] }); + assert.deepEqual(result.writes, [{ mutation: "arm", id: "PR_1", head: HEAD }]); + // Each arm in the run counts: three in flight of five arms two of three. + result = await run([pull(1), pull(2), pull(3)], {}, {}, { ...ROOM, count: 3, cap: 5 }); + assert.deepEqual( + result.writes.map(({ id }) => id), + ["PR_1", "PR_2"], + ); +}); + +test("a room that cannot be read arms nothing and disarms nothing", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const pulls = [pull(1), pull(2, { autoMergeRequest: armed })]; + for (const lane of [{ ...ROOM, unreadable: "in-flight:merge-queue-unreadable" }, { refusal: null }, null]) { + const { writes } = await run(pulls, {}, {}, lane); + assert.deepEqual(writes, [], JSON.stringify(lane)); + } + const { github, writes } = world(pulls); + const log = console.log; + console.log = () => {}; + try { + await main({ + env: ENV, + github, + verify: async ({ headRefOid: head }) => ({ refusal: null, head, ineligible: null }), + room: async () => { + throw new Error("lane-room exited 1"); + }, + }); + } finally { + console.log = log; + } + assert.deepEqual(writes(), []); +}); + +test("an event run reads no room", async () => { + let asked = 0; + const { github } = world([pull(1)]); + const log = console.log; + console.log = () => {}; + try { + await main({ + env: { ...ENV, EVENT_NAME: "pull_request_target" }, + github, + room: async () => { + asked += 1; + return ROOM; + }, + }); + } finally { + console.log = log; + } + assert.equal(asked, 0); +}); + +test("readRoom takes only lane-room's shape", () => { + assert.deepEqual(readRoom(ROOM), ROOM); + for (const bad of [null, "x", { ...ROOM, cap: 0 }, { ...ROOM, count: -1 }, { ...ROOM, queued: ["3"] }, { ...ROOM, refusal: 1 }, { ...ROOM, unreadable: undefined }]) { + assert.ok(readRoom(bad).unreadable, JSON.stringify(bad)); + } +}); + +test("pythonRoom passes the post-merge report, the rung cap as read, and the reader's token", async () => { + const dir = mkdtempSync(join(tmpdir(), "enable-auto-merge-room-")); + const script = join(dir, "room.py"); + writeFileSync( + script, + `import json, os, sys +print(json.dumps({"argv": sys.argv[1:], "token": os.environ.get("GH_TOKEN"), "budget": os.environ.get("API_BUDGET_TOKEN")})) +`, + ); + const env = { PATH: process.env.PATH, GITHUB_REPOSITORY: REPO, VERIFY_EVIDENCE_SCRIPT: script, POSTMERGE_RESULT: "/tmp/post-merge.json", GH_TOKEN: "lane", BABYSIT_VERIFY_GH_TOKEN: "job" }; + // An empty cap is no cap, so it is passed; an unset one is not, which the gate reads as off. + assert.deepEqual(await pythonRoom({ ...env, RUNG_CAP: "" })(), { + argv: ["lane-room", "--repo", REPO, "--postmerge-result", "/tmp/post-merge.json", "--rung-cap", ""], + token: "job", + budget: "github-token", + }); + assert.deepEqual((await pythonRoom(env)()).argv, ["lane-room", "--repo", REPO, "--postmerge-result", "/tmp/post-merge.json"]); +}); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 4ae2556b1d..5936edd66a 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -187,6 +187,25 @@ test("enable-auto-merge logs both tokens' readings at job end, before the token assert.ok(names.indexOf("Log the API budget at job end") < names.indexOf("Revoke the lane token")); }); +// The lane's room (lane-room) needs the run's post-merge check and the rung +// cap as the merge job passes it: an unset cap holds the lane off, so a step +// that drops it would disarm every lane pull request. +test("enable-auto-merge reads the lane's room from this run's post-merge check and the rung cap", () => { + const arm = job("enable-auto-merge"); + const check = /- name: Check earlier lane merges\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; + assert.ok(check, "a post-merge check step"); + assert.match(check, /^ {8}if: steps\.lane-token\.outputs\.token != '' && \(github\.event_name == 'schedule' \|\| github\.event_name == 'workflow_dispatch'\)\n/); + assert.match(check, /\n {10}GH_TOKEN: \$\{\{ github\.token \}\}\n/); + assert.match(check, /babysit_postmerge\.py \\\n {12}--repo "\$REPO" --agent-identity "\$identity" --lane-merges-only \\\n {12}<\/dev\/null >"\$RUNNER_TEMP\/post-merge\.json"/); + const step = /- name: Arm and disarm auto-merge\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)[1]; + assert.match(step, /\n {10}POSTMERGE_RESULT: \$\{\{ runner\.temp \}\}\/post-merge\.json\n/); + assert.match(step, /\n {10}RUNG_CAP: \$\{\{ vars\.CLAUDE_LANES_MERGE_RUNG_CAP \}\}\n/); + assert.match(arm, /\n {6}pull-requests: read #/); + assert.match(arm, /\n {6}issues: read #/); + const names = [...arm.matchAll(/\n {6}- name: (.*)/g)].map((m) => m[1]); + assert.ok(names.indexOf("Check earlier lane merges") < names.indexOf("Arm and disarm auto-merge")); +}); + test("enable-auto-merge asks the broker for its own activity's merge token", () => { const arm = job("enable-auto-merge"); const steps = [...arm.matchAll(/\n {10}lane: (.*)\n {10}activity: (.*)\n/g)].map((m) => `${m[1]}/${m[2]}`); @@ -202,7 +221,7 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () const block = /\n {4}permissions:\n((?: {6}[a-z-]+: [a-z]+.*\n)+)/.exec(arm)[1]; assert.deepEqual( [...block.matchAll(/ {6}([a-z-]+): ([a-z]+)/g)].map((m) => `${m[1]}: ${m[2]}`), - ["contents: read", "actions: read", "id-token: write"], + ["contents: read", "actions: read", "pull-requests: read", "issues: read", "id-token: write"], ); // The verifier evidence is read as the gate reads it: the job's // GITHUB_TOKEN for the Actions reads, with the run's head-ref map and diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 06bcad4a6a..4b48fdd5e0 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1344,6 +1344,8 @@ jobs: permissions: contents: read # check out main actions: read # the verifier verdicts, their runs and the head-ref map + pull-requests: read # the post-merge check's lane merges and the merge queue, for the lane's room + issues: read # the post-merge check's demotion records, read with this job's token id-token: write # prove to the token broker which workflow this is steps: - name: Check out the default branch tip @@ -1447,6 +1449,40 @@ jobs: CARRY_TO: ${{ runner.temp }}/babysit-run-cache.json run: node .base/.github/scripts/pr-merge/carry-run-cache.mjs + # The lane records the last run kept, as the merge job reads them, so the + # check below answers those with no read. Any error leaves no file, and + # the check reads each record. Rules: .github/scripts/pr-merge/lane-records.mjs. + - name: Read the lane records the last run kept + if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') + continue-on-error: true + env: + ACTIONS_TOKEN: ${{ github.token }} + LANE_RECORDS_PATH: ${{ runner.temp }}/lane-records.json + run: node .base/.github/scripts/pr-merge/lane-records.mjs read + + # The merge job's post-merge check, read-only, with this job's token: + # its demotion, floor and verdict are the gate's run-level answer that + # enable-auto-merge.mjs reads the lane's room from (lane-room), so a + # demotion or a failed lane merge stops native arming as it stops the + # gate. Only the runs that arm read it. + - name: Check earlier lane merges + if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') + env: + GH_TOKEN: ${{ github.token }} + API_BUDGET_TOKEN: github-token + BABYSIT_POSTMERGE_LANE_RECORDS: ${{ runner.temp }}/lane-records.json + REPO: ${{ github.repository }} + run: | + set -euo pipefail + identity=$(yq '.merge."agent-identity" // ""' .base/docs/conventions/pr-pipeline.yaml) + code=0 + python3 .base/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py \ + --repo "$REPO" --agent-identity "$identity" --lane-merges-only \ + "$RUNNER_TEMP/post-merge.json" || code=$? + echo "::group::post-merge check: exit $code" + cat "$RUNNER_TEMP/post-merge.json" + echo "::endgroup::" + # While merge.report-only is anything but false it reports each # decision and writes nothing. - name: Arm and disarm auto-merge @@ -1461,6 +1497,10 @@ jobs: BABYSIT_VERIFY_LISTING: ${{ runner.temp }}/verify-listing.json BABYSIT_VERIFY_HEAD_REFS: ${{ runner.temp }}/head-refs-in.json BABYSIT_RUN_CACHE: ${{ runner.temp }}/babysit-run-cache.json + # The lane's room (lane-room): the post-merge check above and the + # rung cap as the merge job passes it, empty meaning no cap. + POSTMERGE_RESULT: ${{ runner.temp }}/post-merge.json + RUNG_CAP: ${{ vars.CLAUDE_LANES_MERGE_RUNG_CAP }} # Name the two tokens' rate-limit readings for the reader's gh calls # (api-budget.bash): the lane token by default, the job token for # the Actions reads that carry it. 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 d668c4334b..b799bd93b3 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 @@ -2870,12 +2870,14 @@ def derive_auto_merge( class InFlight: """`queued` holds every merge-queue entry's pull request number, or None when the queue was not read whole; `unverified` counts the unverified - auto-merges read, all of them when clear.""" + auto-merges read, all of them when clear; `count` the merges in flight + when clear (`max_in_flight` or more when not).""" clear: bool reason: str | None queued: frozenset[int] | None = field(default=None, compare=False) unverified: int = field(default=0, compare=False) + count: int = field(default=0, compare=False) def in_flight( @@ -2952,7 +2954,7 @@ def in_flight( return InFlight(False, reason, queued, unverified) except PolicyReadError as exc: return InFlight(False, f"unreadable: {exc}", queued, unverified) - return InFlight(True, None, queued, unverified) + return InFlight(True, None, queued, unverified, count) # --- Holds on a queued entry ------------------------------------------------------------------- @@ -4437,3 +4439,99 @@ def assess( return UnattendedPolicy( reason, identity, details, enabled=True, promoted=promoted, inputs=inputs ) + + +# --- Room for a native lane merge ------------------------------------------------------------- + + +@dataclass(frozen=True) +class LaneRoom: + """`lane_room`'s answer. `refusal`: the gate holds the lane off, so + nothing is armed and armed lane pull requests are disarmed. `unreadable`: + a read failed, so nothing is armed and nothing is disarmed. Otherwise + `count` merges are in flight of `cap`, and `queued` holds the merge + queue's pull request numbers.""" + + refusal: str | None = None + unreadable: str | None = None + count: int = 0 + cap: int = 1 + queued: frozenset[int] = frozenset() + + def report(self) -> dict[str, Any]: + return { + "refusal": self.refusal, + "unreadable": self.unreadable, + "count": self.count, + "cap": self.cap, + "queued": sorted(self.queued), + } + + +def lane_room( + repo: str, shared: SharedPostMerge | None, *, lane_rung_cap: str | None +) -> LaneRoom: + """Whether the pr-merge lane may merge one more of its own pull requests + now, for native auto-merge, which skips `assess`: the run-level refusals + `assess` applies to every lane candidate, read once at the default branch + head, in its order. `shared` is this run's `babysit_postmerge.py` answer + (`babysit_postmerge.read_shared`), as the gate's `--postmerge-result`. + + - A config naming another authority stops (`enabled_authority`). + - A post-merge refusal stops (`post-merge:`). + - The effective rung off stops (`rung-off:`): `merge.rung`, + `babysit_loop_merge`, the rung cap (`lane_rung_cap`, as `rung_cap` + reads it) and an active demotion, an unreadable record included. A lane + pull request's class is `LANE_CLASS`, which no rung above off is below, + so no class-above-rung refusal applies. + - The merges in flight (`in_flight`, `merge.max-in-flight` at the head) + are counted, not refused: the caller arms below the cap. + + Any read failure, a missing `shared` and an in-flight count that could + not be read included, is `unreadable`.""" + if shared is None: + return LaneRoom(unreadable="post-merge answer missing or unreadable") + try: + default = _default_branch(repo, shared=True) + config = _default_branch_config(repo, default) + except PolicyReadError as exc: + return LaneRoom(unreadable=f"config: {exc}") + if enabled_authority(config) != PR_MERGE: + return LaneRoom(refusal=AUTHORITY_UNCONFIGURED) + identity = config.agent_identity + if not identity or _login_key(shared.identity) != _login_key(identity): + return LaneRoom(unreadable="post-merge answer is for another identity") + if shared.gate.refusal: + return LaneRoom(refusal=f"post-merge:{shared.gate.refusal}") + cap = rung_cap(repo, authority=PR_MERGE, lane_value=lane_rung_cap) + rung = effective_rung( + config_rung=config.rung, + loop_merge=config.loop_merge, + cap=cap.value, + demoted=shared.demotion.active, + ) + if rung.rung == "off": + return LaneRoom(refusal=f"rung-off:{rung.limited_by}") + flight_cap, _ = _run_max_in_flight(repo, default, config.base_sha, config) + flight = in_flight( + repo, + agent_identity=identity, + self_logins=shared.self_logins, + verified=shared.gate.verified, + branch=default, + merged_since=shared.floor, + lane_merges_only=True, + max_in_flight=flight_cap, + ) + counted = str(flight.reason).startswith(FLIGHT_COUNTED) + if flight.queued is None or not (flight.clear or counted): + return LaneRoom(unreadable=f"in-flight:{flight.reason}", cap=flight_cap) + return LaneRoom( + count=flight.count if flight.clear else flight_cap, + cap=flight_cap, + queued=flight.queued, + ) + + +# The `in_flight` reasons that are a count reaching the cap, not a read failure. +FLIGHT_COUNTED = ("identity-entry-queued:", "unverified-auto-merge:") 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 3e1be86838..954893ba1e 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 @@ -1441,7 +1441,13 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: With `--lane-link`, the line also carries `ineligible`: null when the pull request's issue link is one the pr-merge lane takes (`Reader.link_refusal`), else why not, with refusal `verify:ineligible` - and no verdict or config read.""" + and no verdict or config read. + + `lane-room --postmerge-result [--rung-cap ]`: whether the + pr-merge lane may merge one more of its own pull requests now + (`merge_policy.lane_room`, `LaneRoom.report`), from this run's + `babysit_postmerge.py` report and the `CLAUDE_LANES_MERGE_RUNG_CAP` value + the workflow read (empty is no cap; missing holds the rung off).""" parser = argparse.ArgumentParser( prog="babysit_verify_evidence.py", allow_abbrev=False ) @@ -1454,7 +1460,16 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: cmd.add_argument("--lane-link", action="store_true") cmd.add_argument("number", type=int) cmd.add_argument("head") + room = sub.add_parser("lane-room", allow_abbrev=False) + room.add_argument("--repo", required=True) + room.add_argument("--postmerge-result", required=True) + room.add_argument("--rung-cap", default=None) args = parser.parse_args(argv) + if args.command == "lane-room": + shared = postmerge.read_shared(args.postmerge_result) + answer = merge_policy.lane_room(args.repo, shared, lane_rung_cap=args.rung_cap) + print(json.dumps(answer.report())) + return 0 if ( args.max_fails < 1 or not merge_policy.SHA_RE.fullmatch(args.head) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py new file mode 100644 index 0000000000..1a7590d468 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -0,0 +1,230 @@ +"""The pr-merge lane's room for one more native merge (`lane_room`). + +Expected values come from the gate's own run-level refusals, which native +auto-merge skips: `effective_rung` (the lowest of `merge.rung`, +`babysit_loop_merge`, the rung cap and the demotion; an unknown or missing cap +is off, an empty one no cap), a post-merge refusal, an authority other than +pr-merge, and `in_flight`'s count against `merge.max-in-flight`. A read that +fails refuses nothing and allows nothing (`unreadable`). +""" + +from __future__ import annotations + +import contextlib +import io +import json +import pathlib +import sys +import unittest +from datetime import UTC, datetime +from unittest import mock + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) + +import babysit_merge_policy as policy # noqa: E402 +import babysit_verify_evidence as ve # noqa: E402 + +REPO = "example-org/example-repo" +IDENTITY = "agent-lanes[bot]" +MAIN = "c" * 40 +FLOOR = datetime(2026, 10, 1, tzinfo=UTC) + + +def config(**overrides: object) -> policy.PipelineConfig: + fields = { + "base_sha": MAIN, + "rung": "C2", + "authority": "pr-merge", + "agent_identity": IDENTITY, + "max_changed_lines": None, + "denied_paths": (), + "loop_merge": "c2-mechanical", + } + fields.update(overrides) + return policy.PipelineConfig(**fields) # type: ignore[arg-type] + + +def shared( + *, demoted: bool = False, refusal: str | None = None, identity: str = IDENTITY +) -> policy.SharedPostMerge: + return policy.SharedPostMerge( + main_sha=MAIN, + config_sha=MAIN, + identity=identity, + self_logins=frozenset(), + lane_merges_only=True, + computed_at=FLOOR, + demotion=policy.Demotion( + demoted, "record-open" if demoted else None, (7,) if demoted else () + ), + floor=FLOOR, + gate=policy.PostMergeGate(refusal, {}, lambda merge: True), + ) + + +class LaneRoom(unittest.TestCase): + def room( + self, + answer: policy.SharedPostMerge | None = None, + *, + cfg: policy.PipelineConfig | None = None, + cap: str | None = "", + flight: policy.InFlight | None = None, + config_error: Exception | None = None, + ) -> policy.LaneRoom: + seen: dict[str, object] = {} + + def in_flight(repo: str, **kwargs: object) -> policy.InFlight: + seen.update(kwargs) + return flight or policy.InFlight(True, None, frozenset({11}), 0, 2) + + with ( + mock.patch.object(policy, "_default_branch", return_value="main"), + mock.patch.object( + policy, + "_default_branch_config", + side_effect=config_error, + return_value=cfg or config(), + ), + mock.patch.object( + policy, "_run_max_in_flight", return_value=(5, "default-branch") + ), + mock.patch.object(policy, "in_flight", side_effect=in_flight), + ): + result = policy.lane_room( + REPO, answer if answer is not None else shared(), lane_rung_cap=cap + ) + self.seen = seen + return result + + def test_a_clear_lane_counts_its_merges_in_flight_against_the_cap(self) -> None: + room = self.room() + self.assertEqual( + (room.refusal, room.unreadable, room.count, room.cap, room.queued), + (None, None, 2, 5, frozenset({11})), + ) + # The lane's own merges, since the post-merge floor, judged by its check. + self.assertEqual( + ( + self.seen["lane_merges_only"], + self.seen["merged_since"], + self.seen["max_in_flight"], + ), + (True, FLOOR, 5), + ) + + def test_a_demotion_holds_the_lane_off(self) -> None: + self.assertEqual(self.room(shared(demoted=True)).refusal, "rung-off:demotion") + + def test_a_rung_cap_of_off_or_a_missing_cap_holds_it_off(self) -> None: + self.assertEqual(self.room(cap="off").refusal, "rung-off:rungCap") + self.assertEqual(self.room(cap=None).refusal, "rung-off:rungCap") + self.assertIsNone(self.room(cap="C2").refusal) + + def test_merge_rung_or_the_loop_setting_off_holds_it_off(self) -> None: + self.assertEqual( + self.room(cfg=config(rung="off")).refusal, "rung-off:merge.rung" + ) + self.assertEqual( + self.room(cfg=config(loop_merge="human-only")).refusal, + "rung-off:babysit_loop_merge", + ) + + def test_a_post_merge_failure_holds_it_off(self) -> None: + room = self.room(shared(refusal="failure:red-main#40")) + self.assertEqual(room.refusal, "post-merge:failure:red-main#40") + + def test_another_authority_holds_it_off(self) -> None: + room = self.room(cfg=config(authority="local-babysit")) + self.assertEqual(room.refusal, policy.AUTHORITY_UNCONFIGURED) + + def test_at_the_cap_it_reports_the_cap_and_refuses_nothing(self) -> None: + full = policy.InFlight(False, "identity-entry-queued:#14", frozenset({10, 14})) + room = self.room(flight=full) + self.assertEqual( + (room.refusal, room.unreadable, room.count, room.cap), (None, None, 5, 5) + ) + + def test_a_failed_read_refuses_nothing_and_allows_nothing(self) -> None: + cases = { + "no answer": dict(answer=None), + "queue": dict(flight=policy.InFlight(False, "merge-queue-unreadable")), + "enqueuer": dict( + flight=policy.InFlight( + False, "entry-enqueuer-unknown:#3", frozenset({3}) + ) + ), + "config": dict(config_error=policy.PolicyReadError("contents 502")), + "identity": dict(answer=shared(identity="someone-else[bot]")), + } + for name, kwargs in cases.items(): + with self.subTest(name): + answer = kwargs.pop("answer", shared()) + if name == "no answer": + with mock.patch.object(policy, "in_flight") as flight: + room = policy.lane_room(REPO, None, lane_rung_cap="") + flight.assert_not_called() + else: + room = self.room(answer, **kwargs) # type: ignore[arg-type] + self.assertIsNone(room.refusal) + self.assertTrue(room.unreadable) + + +class LaneRoomCommand(unittest.TestCase): + def test_it_prints_the_room_from_the_post_merge_report(self) -> None: + seen: list[object] = [] + + def room( + repo: str, answer: object, *, lane_rung_cap: str | None + ) -> policy.LaneRoom: + seen.append((repo, answer, lane_rung_cap)) + return policy.LaneRoom(count=1, cap=5, queued=frozenset({3})) + + out = io.StringIO() + with ( + mock.patch.object( + ve.postmerge, "read_shared", return_value="answer" + ) as read, + mock.patch.object(policy, "lane_room", side_effect=room), + contextlib.redirect_stdout(out), + ): + code = ve.main( + [ + "lane-room", + "--repo", + REPO, + "--postmerge-result", + "/tmp/x.json", + "--rung-cap", + "", + ] + ) + self.assertEqual(code, 0) + read.assert_called_once_with("/tmp/x.json") + self.assertEqual(seen, [(REPO, "answer", "")]) + self.assertEqual( + json.loads(out.getvalue()), + {"refusal": None, "unreadable": None, "count": 1, "cap": 5, "queued": [3]}, + ) + + def test_a_missing_report_is_unreadable(self) -> None: + out = io.StringIO() + with contextlib.redirect_stdout(out): + ve.main( + [ + "lane-room", + "--repo", + REPO, + "--postmerge-result", + "/nonexistent/post-merge.json", + "--rung-cap", + "", + ] + ) + report = json.loads(out.getvalue()) + self.assertIsNone(report["refusal"]) + self.assertTrue(report["unreadable"]) + + +if __name__ == "__main__": + unittest.main() 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 d9c99ba663..bf49d8f1d4 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 @@ -1985,6 +1985,8 @@ def test_a_cap_of_five_admits_four_in_flight_and_refuses_five(self) -> None: admitted = self.capped(5, 2, 2) self.assertEqual((admitted.clear, admitted.reason), (True, None)) self.assertEqual(admitted.unverified, 2) + # Two queued entries and two unverified merges: four in flight. + self.assertEqual(admitted.count, 4) self.assertEqual(self.capped(5, 3, 2).reason, "unverified-auto-merge:#21") self.assertEqual(self.capped(5, 5, 0).reason, "identity-entry-queued:#14") From b2791d01ad3962445939263318df42efaf3aa6de Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:34:33 -0400 Subject: [PATCH 05/13] fix(pr-pipeline): hold a merge group whose pass no longer binds the live link verdictHold accepted any pass at the head, so an armed Refs: #A pull request whose body was edited to Closes #B merged closing B on a pass that judged A. ci-status's merge-group step now reads the counted pass's verdict.json and the closing issues and holds unless the live link, by the gate's rule, is the issue the pass judged, linked the same way. refsIssue moves into merge-holds.mjs, which ci-status fetches alone, and resolve-pr.mjs re-exports it. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 1 + .github/scripts/pr-merge/merge-holds.mjs | 107 +++++++++++++++++- .github/scripts/pr-merge/merge-holds.test.mjs | 30 ++++- .github/scripts/pr-verify/resolve-pr.mjs | 39 +------ .github/workflows/pr-require-checks.yml | 41 ++++++- scripts/ci-lanes-pr-hold.test.sh | 48 +++++++- 6 files changed, 214 insertions(+), 52 deletions(-) diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index 5f742b15da..84eb944f54 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -6,3 +6,4 @@ bump: minor - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. - The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run runs the post-merge check, as the merge job does, and reads the lane's room from it (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A room that cannot be read arms nothing and disarms nothing. The job now holds `pull-requests: read` and `issues: read` for those reads. +- In the merge queue, `ci-status` holds a lanes App pull request whose verifier pass no longer binds its live issue link. It reads the counted pass's `verdict.json` (one artifact download) and the closing issues (one GraphQL read), and `merge-holds.mjs` `linkHold` holds unless the body still closes, or with no closing issue still names by a plain `Refs:` line, the one issue the pass judged, linked the same way. This is pr-merge's reader rule (`verify:spec-changed`), so a body edited after arming, such as `Refs: #A` to `Closes #B`, no longer merges on a pass for the old issue. A trusted approval of the head lifts it, as it lifts the verifier hold. `refsIssue` moves to `merge-holds.mjs`, which `resolve-pr.mjs` re-exports. diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index a41157fdb3..9bbce1c23e 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -26,10 +26,18 @@ // path for a pull request pr-merge's gate sends to a person. FAILs and the // repeated-FAIL cap are enable-auto-merge.mjs's, through pr-merge's own // reader, which disarms such a pull request. +// - A changed issue link, when a link file is given with the verdicts: the +// pass binds only while the live link is the one its verdict judged +// (linkHold), as pr-merge's reader binds it (babysit_verify_evidence.py +// `_pass_refusal`), so a body edited after arming to close or name another +// issue does not merge on a pass for the old one. The same approval lifts it. // // Command line: merge-holds.mjs // -// []. The verdicts file holds one {name, verdict, runId, run} +// [ ]. The link file holds {record, closing}: the +// verdict.json of the pass verdictHold counts (null when none was read) and +// the pull request's closingIssuesReferences (totalCount, nodes with number +// and repository.nameWithOwner). The verdicts file holds one {name, verdict, runId, run} // per line, run as GET actions/runs/{runId} gives it, for each unexpired artifact // named a pass at the head; GITHUB_REPOSITORY names the repository. The // discussion file holds one REST issue comment, review or review comment per @@ -48,6 +56,82 @@ import { pathToFileURL } from "node:url"; // text it tries. export const BODY_HOLD = /\bdo(?:n['’]?t| not)\s+merge\b|(?:^|\r)[^\S\r\n]*hold:/im; export const HUMAN_VETO = /\bdo(?:n['’]?t| not|-not)[\s-]*merge\b/i; + +// babysit_merge_policy.refs_issue, which test_verify_rules_sync.py keeps equal: +// the one issue the visible `Refs: #` lines of a pull request body name, +// else null. A line counts only from its first column, outside an HTML comment +// and a fenced code block; a `Refs:` line in any other form, or two numbers, +// name none. +const HTML_COMMENT = /|$)/g; +const FENCE = /^ {0,3}(`{3,}|~{3,})/; +const REFS_LINE = /^[Rr][Ee][Ff][Ss]:/; +const REFS_PLAIN = /^Refs:[ \t]*#([1-9][0-9]{0,8})[ \t]*$/; +export function refsIssue(body) { + const numbers = new Set(); + let fence = ""; + for (const line of String(body ?? "").replace(HTML_COMMENT, "").split(/\r\n|\r|\n/)) { + const marker = FENCE.exec(line); + if (fence) { + if (marker && marker[1][0] === fence[0] && marker[1].length >= fence.length && line.slice(marker[0].length).replace(/[ \t]/g, "") === "") { + fence = ""; + } + continue; + } + if (marker) { + fence = marker[1]; + continue; + } + if (!REFS_LINE.test(line)) { + continue; + } + const plain = REFS_PLAIN.exec(line); + if (!plain) { + return null; + } + numbers.add(Number(plain[1])); + } + return numbers.size === 1 ? [...numbers][0] : null; +} + +// Why the pass `record` (its verdict.json) does not bind the pull request's +// live link, or null when it does. The live link is the gate's +// (babysit_merge_policy._linked_issue): its one closing issue in `repo`, or, +// with no closing issue at all, the one issue its plain `Refs:` lines name +// (refsIssue). The pass binds when it judged that one issue, linked the same +// way (its `spec_kind`, `closes` when absent, as the reader reads an older +// record). +export function linkHold(record, body, closing, repo) { + if (record === null || typeof record !== "object" || !Array.isArray(record.linked_issues)) { + return "its verifier pass's record could not be read"; + } + const kind = record.spec_kind ?? "closes"; + if (!["closes", "refs"].includes(kind)) { + return `its verifier pass judged a spec of kind '${String(kind)}', not a linked issue`; + } + if (!Array.isArray(closing?.nodes) || !Number.isSafeInteger(closing.totalCount) || closing.totalCount !== closing.nodes.length) { + return "its closing issues could not be read"; + } + let live = null; + let linked = "refs"; + if (closing.totalCount > 0) { + const [issue] = closing.nodes; + if (closing.totalCount !== 1 || String(issue?.repository?.nameWithOwner).toLowerCase() !== String(repo).toLowerCase()) { + return "it does not close exactly one issue in this repository, which pr-merge's lane takes"; + } + live = issue.number; + linked = "closes"; + } else { + live = refsIssue(body); + } + if (!Number.isSafeInteger(live)) { + return "it names no issue pr-merge's lane takes"; + } + const judged = record.linked_issues.map((issue) => issue?.number); + if (judged.length !== 1 || judged[0] !== live || kind !== linked) { + return `its link changed since pr-verify passed it: it ${linked === "closes" ? "closes" : "refs"} #${live}, and the pass judged ${judged.map((n) => `#${n}`).join(", ") || "no issue"} (${kind})`; + } + return null; +} // babysit_merge_policy.BLOCKER_ASSOCIATIONS. const MEMBERS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); // The denied paths resolve-config adds to every config's (resolveMerge); @@ -148,7 +232,6 @@ export function costHold(files, decision) { // babysit_merge_policy COST_DECISION_LINK, DECISION_LINE and HTML_COMMENT. const DECISION_LINK = /https:\/\/github\.com\/([A-Za-z0-9-]+)\/([A-Za-z0-9._-]+)\/(?:issues|pull)\/([0-9]+)#issuecomment-([0-9]+)/g; const DECISION_LINE = /^\*\*Decision recorded\.\*\*[ \t]+Cost:/; -const HTML_COMMENT = /|$)/g; export const PULL_OPENED_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { createdAt closingIssuesReferences(first: 2) { totalCount nodes { number repository { nameWithOwner } } } } @@ -324,8 +407,18 @@ const escape = (text) => String(text).replaceAll("%", "%25").replaceAll("\r", "% export const humansOf = (actors) => new Set(actors.filter((actor) => actor?.kind === "human" && Number.isInteger(actor?.id)).map(({ id }) => id)); +// The link file's verdict, a file that does not read included. +function linkOf(file, body, repo) { + try { + const { record, closing } = JSON.parse(readFileSync(file, "utf8")); + return linkHold(record, body, closing, repo); + } catch { + return "its issue link could not be checked against its verifier pass"; + } +} + export async function main(argv = process.argv.slice(2), { env = process.env, github = fetchGitHub(env) } = {}) { - const [number, bodyFile, discussionFile, filesFile, deniedFile, actorsFile, head, verdictsFile] = argv; + const [number, bodyFile, discussionFile, filesFile, deniedFile, actorsFile, head, verdictsFile, linkFile] = argv; try { if ( !/^[1-9][0-9]*$/.test(String(number)) || @@ -337,7 +430,7 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi !/^[0-9a-f]{40}$/.test(String(head)) ) { throw new Error( - "usage: merge-holds.mjs []", + "usage: merge-holds.mjs [ ]", ); } const lines = (file) => readFileSync(file, "utf8").split("\n").filter((line) => line.trim()); @@ -364,8 +457,12 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi } if (verdictsFile) { const unverified = verdictHold(lines(verdictsFile).map((line) => JSON.parse(line)), env.GITHUB_REPOSITORY); - if (!unverified) { + // An approval lifts the verifier hold whole, the link with it. + const relinked = unverified || lifters.length ? null : linkFile ? linkOf(linkFile, body, repo) : "its issue link could not be checked against its verifier pass"; + if (!unverified && !relinked) { console.log(`#${number}: no verifier hold: pr-verify passed this head.`); + } else if (!unverified && lifters.length === 0) { + reasons.push(`${relinked}; pr-verify judges its new link, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); } else if (lifters.length) { console.log(`#${number}: no verifier hold: ${lifters.join(", ")} approved this head, which lifts '${unverified}'.`); } else { diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 05916fa81a..336e1d47e3 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -12,7 +12,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, costDecision, costHold, main } from "./merge-holds.mjs"; +import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, costDecision, costHold, linkHold, main } from "./merge-holds.mjs"; const REPO = "owner/repo"; const LINK = "https://github.com/owner/repo/issues/40#issuecomment-7"; @@ -173,3 +173,31 @@ test("ci-status holds a cost path that an approval does not lift, and the owner' assert.match(decided.out, /no body hold, no member veto and no hold path\./); assert.equal((await ci(["infra/main.tf"], { body: `Closes #40\n\n${LINK}`, options: { node: { lastEditedAt: "2026-10-22T13:00:00Z" } } })).code, 1); }); + +// The live link a pass binds, by the gate's rule (README "Eligibility"): one +// closing issue in this repository, or, with none, one plain `Refs: #` +// issue; a pass binds only the issue it judged, linked the same way +// (babysit_verify_evidence.py _pass_refusal: verify:spec-changed otherwise). +test("linkHold binds a pass to the issue and link kind it judged", () => { + const closes = (...numbers) => ({ totalCount: numbers.length, nodes: numbers.map((number) => ({ number, repository: { nameWithOwner: "o/r" } })) }); + const record = (number, spec_kind) => ({ linked_issues: [{ number }], ...(spec_kind ? { spec_kind } : {}) }); + assert.equal(linkHold(record(5, "closes"), "Closes #5", closes(5), "o/r"), null); + // A record from before the kind was written judged a closing issue. + assert.equal(linkHold(record(5), "Closes #5", closes(5), "O/R"), null); + assert.equal(linkHold(record(5, "refs"), "Refs: #5\n\n## Summary", closes(), "o/r"), null); + assert.match(linkHold(record(5, "refs"), "Closes #6", closes(6), "o/r"), /link changed since pr-verify passed it: it closes #6, and the pass judged #5 \(refs\)/); + assert.match(linkHold(record(5, "refs"), "Closes #5", closes(5), "o/r"), /link changed/); + assert.match(linkHold(record(5, "closes"), "Refs: #5", closes(), "o/r"), /link changed/); + assert.match(linkHold(record(5, "refs"), "Refs: #6", closes(), "o/r"), /refs #6, and the pass judged #5/); + assert.match(linkHold(record(5, "refs"), "Refs: #5\nRefs: #6", closes(), "o/r"), /names no issue/); + assert.match(linkHold(record(5, "refs"), "", closes(), "o/r"), /names no issue/); + assert.match(linkHold(record(5, "closes"), "Closes #5", closes(5, 6), "o/r"), /exactly one issue/); + assert.match( + linkHold(record(5, "closes"), "Closes x/y#5", { totalCount: 1, nodes: [{ number: 5, repository: { nameWithOwner: "x/y" } }] }, "o/r"), + /exactly one issue/, + ); + assert.match(linkHold(record(5, "pull-request"), "", closes(), "o/r"), /kind 'pull-request'/); + assert.match(linkHold(null, "Closes #5", closes(5), "o/r"), /record could not be read/); + assert.match(linkHold({ linked_issues: [{ number: 5 }, { number: 6 }] }, "Closes #5", closes(5), "o/r"), /link changed/); + assert.match(linkHold(record(5), "Closes #5", { totalCount: 11, nodes: [] }, "o/r"), /closing issues could not be read/); +}); diff --git a/.github/scripts/pr-verify/resolve-pr.mjs b/.github/scripts/pr-verify/resolve-pr.mjs index e867b673b7..64d2240f81 100644 --- a/.github/scripts/pr-verify/resolve-pr.mjs +++ b/.github/scripts/pr-verify/resolve-pr.mjs @@ -41,6 +41,9 @@ import { loadTrustedList, } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { headRefDigest, issuesSuffix, isVerdictName, NAME } from "./write-verdict.mjs"; +import { refsIssue } from "../pr-merge/merge-holds.mjs"; + +export { refsIssue }; // The name grammar lives with its writer; the resolver checks names against it. export { isVerdictName, NAME, NAME_MAX } from "./write-verdict.mjs"; @@ -81,42 +84,6 @@ const REFS_QUERY = `query($owner: String!, $name: String!, $number: Int!) { } ${ACTOR}`; -// babysit_merge_policy.refs_issue, which test_verify_rules_sync.py keeps equal: -// the one issue the visible `Refs: #` lines of a pull request body name, -// else null. A line counts only from its first column, outside an HTML comment -// and a fenced code block; a `Refs:` line in any other form, or two numbers, -// name none. -const HTML_COMMENT = /|$)/g; -const FENCE = /^ {0,3}(`{3,}|~{3,})/; -const REFS_LINE = /^[Rr][Ee][Ff][Ss]:/; -const REFS_PLAIN = /^Refs:[ \t]*#([1-9][0-9]{0,8})[ \t]*$/; -export function refsIssue(body) { - const numbers = new Set(); - let fence = ""; - for (const line of String(body ?? "").replace(HTML_COMMENT, "").split(/\r\n|\r|\n/)) { - const marker = FENCE.exec(line); - if (fence) { - if (marker && marker[1][0] === fence[0] && marker[1].length >= fence.length && line.slice(marker[0].length).replace(/[ \t]/g, "") === "") { - fence = ""; - } - continue; - } - if (marker) { - fence = marker[1]; - continue; - } - if (!REFS_LINE.test(line)) { - continue; - } - const plain = REFS_PLAIN.exec(line); - if (!plain) { - return null; - } - numbers.add(Number(plain[1])); - } - return numbers.size === 1 ? [...numbers][0] : null; -} - // babysit_merge_policy.names_nothing: whether a GraphQL answer says only that // `repository.` names nothing, null there with every error a NOT_FOUND // at that path, as GitHub answers a `Refs: #` whose number is no issue or diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 8f586df1fc..c2a2351421 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -4114,6 +4114,12 @@ jobs: # new head before it is armed again; the queue still checks the head # it merges. # FAILs and the repeated-FAIL cap are enable-auto-merge.mjs's. + # THE PASS MUST STILL BIND THE LIVE LINK: the counted pass's verdict.json + # (one artifact download) and the closing issues (one GraphQL read) go + # to merge-holds.mjs linkHold, which holds when the body now closes or + # names another issue than the one pr-verify judged, as pr-merge's + # reader holds it (verify:spec-changed). A record or link that does not + # read holds. # Pinned by scripts/ci-lanes-pr-hold.test.sh, which runs this body. - name: Fail a lanes App pull request its body or a member holds if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }} @@ -4232,18 +4238,41 @@ jobs: verdict=${name#"pr-verify-$pr-$head-"} verdict=${verdict%"-$digest"} runs=$(read_api "the $name artifacts" "repos/$GITHUB_REPOSITORY/actions/artifacts?name=$name&per_page=100" \ - --jq ".artifacts[] | select(.name == \"$name\" and .expired == false) | .workflow_run.id") || exit 1 - for run in $runs; do - [[ $run =~ ^[1-9][0-9]*$ ]] || continue + --jq ".artifacts[] | select(.name == \"$name\" and .expired == false) | \"\(.workflow_run.id) \(.id)\"") || exit 1 + while read -r run artifact; do + [[ $run =~ ^[1-9][0-9]*$ && $artifact =~ ^[1-9][0-9]*$ ]] || continue record=$(read_api "run $run" "repos/$GITHUB_REPOSITORY/actions/runs/$run") || exit 1 - jq -c --arg name "$name" --arg verdict "$verdict" --argjson id "$run" '{name: $name, verdict: $verdict, runId: $id, run: .}' <<<"$record" >>"$RUNNER_TEMP/verdicts.jsonl" || exit 1 - done + jq -c --arg name "$name" --arg verdict "$verdict" --argjson id "$run" --argjson artifact "$artifact" '{name: $name, verdict: $verdict, runId: $id, artifact: $artifact, run: .}' <<<"$record" >>"$RUNNER_TEMP/verdicts.jsonl" || exit 1 + done <<<"$runs" # A pass merge-holds.mjs verdictHold would count ends the lookup. ! jq -se --arg repo "$GITHUB_REPOSITORY" 'any(.runId as $id | .run | .id == $id and .status == "completed" and .conclusion == "success" and .path == ".github/workflows/pr-verify.yml" and .event == "workflow_run" and .head_branch == "main" and .repository.full_name == $repo and .head_repository.full_name == $repo)' "$RUNNER_TEMP/verdicts.jsonl" >/dev/null || break done - verdicts=("$RUNNER_TEMP/verdicts.jsonl") + # The counted pass's record and the closing issues, for linkHold. + artifact=$(jq -sr --arg repo "$GITHUB_REPOSITORY" '[.[] | select(.runId as $id | .run | .id == $id and .status == "completed" and .conclusion == "success" + and .path == ".github/workflows/pr-verify.yml" and .event == "workflow_run" and .head_branch == "main" + and .repository.full_name == $repo and .head_repository.full_name == $repo)] | first | .artifact // empty' "$RUNNER_TEMP/verdicts.jsonl") || exit 1 + record=null + if [[ $artifact =~ ^[1-9][0-9]*$ ]]; then + for attempt in 1 2 3; do + # gh-read-exempt: a binary archive, which gh_read's text capture would corrupt. + if gh api "repos/$GITHUB_REPOSITORY/actions/artifacts/$artifact/zip" >"$RUNNER_TEMP/verdict.zip"; then + record=$(unzip -p "$RUNNER_TEMP/verdict.zip" verdict.json | jq -c .) || record=null + break + fi + [ "$attempt" -lt 3 ] || { echo "::error::could not read the verdict of artifact $artifact; re-run this run."; exit 1; } + sleep $((attempt * 5)) + done + fi + # shellcheck disable=SC2016 # GraphQL variables, not shell + closing=$(read_api "the closing issues of #$pr" graphql -f query='query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){closingIssuesReferences(first:10){totalCount nodes{number repository{nameWithOwner}}}}}}' \ + -F owner="${GITHUB_REPOSITORY%%/*}" -F name="${GITHUB_REPOSITORY#*/}" -F number="$pr" --jq '.data.repository.pullRequest.closingIssuesReferences') || exit 1 + jq -cn --argjson record "${record:-null}" --argjson closing "${closing:-null}" '{record: $record, closing: $closing}' >"$RUNNER_TEMP/link.json" || { + echo "::error::the verdict record or closing issues of #$pr do not parse." + exit 1 + } + verdicts=("$RUNNER_TEMP/verdicts.jsonl" "$RUNNER_TEMP/link.json") fi out=$(node "$RUNNER_TEMP/merge-holds.mjs" "$pr" "$RUNNER_TEMP/pr-body.txt" "$RUNNER_TEMP/discussion.jsonl" \ "$RUNNER_TEMP/files.jsonl" "$RUNNER_TEMP/denied-paths.json" "$RUNNER_TEMP/trusted-actors.json" "$head" "${verdicts[@]}") || { diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index acbcb59095..5768fe1acd 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -95,8 +95,11 @@ fi # trusted-actors.json prints the working tree's list, or answers 404 when # STUB_NO_ACTORS is set. An artifacts read by name prints the run ids # STUB_ARTIFACTS (a JSON object of name to run ids) gives that name, through -# the step's own --jq; a run read prints that id's entry of STUB_RUNS, or -# answers 404. +# the step's own --jq, each artifact's id ten times its run's; a run read +# prints that id's entry of STUB_RUNS, or answers 404. An artifact archive +# holds STUB_RECORD as its verdict.json (one judging #1, closing, by +# default), or answers 404 when STUB_NO_RECORD is set; the closing-issues +# query answers STUB_CLOSING (#1 of o/r by default) through the step's --jq. mkdir -p "$TMP_ROOT/bin" cat >"$TMP_ROOT/bin/gh" <<'EOF' #!/usr/bin/env bash @@ -106,6 +109,22 @@ discussion() { [[ "$(grep -cE '/(comments|reviews)\?' "$STUB_LOG")" -gt "${STUB_FAILS:-0}" ]] || { echo 'gh: Server Error (HTTP 502)' >&2; exit 1; } } case "$*" in +"api graphql "*closingIssuesReferences*) + closing=${STUB_CLOSING:-} + [[ -n "$closing" ]] || closing='{"totalCount":1,"nodes":[{"number":1,"repository":{"nameWithOwner":"o/r"}}]}' + jq -c --argjson closing "$closing" \ + '{data: {repository: {pullRequest: {closingIssuesReferences: $closing}}}}' <<&2; exit 1; } + record=${STUB_RECORD:-} + [[ -n "$record" ]] || record='{"linked_issues":[{"number":1}],"spec_kind":"closes"}' + python3 -c 'import io, sys, zipfile +buffer = io.BytesIO() +with zipfile.ZipFile(buffer, "w") as archive: + archive.writestr("verdict.json", sys.argv[1]) +sys.stdout.buffer.write(buffer.getvalue())' "$record" + ;; "api graphql "*) [[ -z "${STUB_DISARM_FAILS:-}" ]] || { echo 'gh: Resource not accessible by integration' >&2; exit 1; } echo '{"data":{}}' @@ -132,8 +151,8 @@ case "$*" in # The step's own --jq filter, the last argument, runs over a listing that # also holds an expired artifact of that name and an unexpired one whose # name only starts with it, both of run 11, which neither may count. - jq -c --arg name "$name" '{artifacts: ([.[$name][]? | {name: $name, expired: false, workflow_run: {id: .}}] - + [{name: $name, expired: true, workflow_run: {id: 11}}, {name: ($name + "-i1"), expired: false, workflow_run: {id: 11}}])}' \ + jq -c --arg name "$name" '{artifacts: ([.[$name][]? | {name: $name, id: (. * 10), expired: false, workflow_run: {id: .}}] + + [{name: $name, id: 110, expired: true, workflow_run: {id: 11}}, {name: ($name + "-i1"), id: 110, expired: false, workflow_run: {id: 11}}])}' \ <<<"${STUB_ARTIFACTS:-{\}}" | jq -r "${*: -1}" ;; */actions/runs/*) @@ -348,6 +367,27 @@ if [[ "$(lookups)" -eq 1 ]]; then else fail "a plain-named pass took $(lookups) lookups" fi +# The pass binds the link it judged (merge-holds.mjs linkHold): a body edited +# after arming to close or name another issue holds the group. +NO_CLOSING='{"totalCount":0,"nodes":[]}' +export STUB_CLOSING STUB_RECORD STUB_NO_RECORD +STUB_PULL="$(pull 'Refs: #1')" STUB_CLOSING="$NO_CLOSING" STUB_RECORD='{"linked_issues":[{"number":1}],"spec_kind":"refs"}' +expect "in the merge queue, a Refs pull request whose pass judged that issue passes" 0 3 "#7: no verifier hold: pr-verify passed this head." +STUB_PULL="$(pull 'Closes #2')" STUB_CLOSING='{"totalCount":1,"nodes":[{"number":2,"repository":{"nameWithOwner":"o/r"}}]}' +expect "in the merge queue, a Refs: #1 pass does not merge a body edited to Closes #2" 1 3 "its link changed since pr-verify passed it: it closes #2, and the pass judged #1 (refs)" +STUB_PULL="$(pull 'Closes #1')" STUB_CLOSING='' STUB_RECORD='{"linked_issues":[{"number":1}],"spec_kind":"refs"}' +expect "in the merge queue, a pass judged as a Refs slice does not cover the issue closed" 1 3 "its link changed since pr-verify passed it" +STUB_PULL="$(pull $'Refs: #1\nRefs: #3')" STUB_CLOSING="$NO_CLOSING" +expect "in the merge queue, a body naming two Refs issues holds" 1 3 "it names no issue pr-merge's lane takes" +STUB_PULL="$(pull 'Closes #1')" STUB_RECORD='' STUB_CLOSING='{"totalCount":2,"nodes":[{"number":1,"repository":{"nameWithOwner":"o/r"}},{"number":3,"repository":{"nameWithOwner":"o/r"}}]}' +expect "in the merge queue, a body closing two issues holds" 1 3 "it does not close exactly one issue in this repository" +STUB_CLOSING='' STUB_RECORD='{"verdict":"pass"}' +expect "in the merge queue, a record with no linked issues holds" 1 3 "its verifier pass's record could not be read" +STUB_RECORD='' STUB_NO_RECORD=1 +expect "in the merge queue, a record that does not download fails closed" 1 3 "could not read the verdict of artifact 110" +STUB_NO_RECORD='' STUB_RECORD='{"linked_issues":[{"number":9}],"spec_kind":"closes"}' STUB_REVIEWS="[$(approval kyle-sexton 153232337 APPROVED 2026-10-23T11:00:00Z "$SHA")]" +expect "in the merge queue, a trusted approval of the head lifts a changed link" 0 3 "#7: no body hold, no member veto and no hold path." +STUB_RECORD='' STUB_REVIEWS='' STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass-notes-$DIGEST\":[11]}" expect "in the merge queue, a digest-named pass-notes at the head passes" 0 3 "no verifier hold: pr-verify passed this head" STUB_ARTIFACTS='{}' From dda84dd83e732496b8c0d1e4dc29556dde9b52cb Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:38:28 -0400 Subject: [PATCH 06/13] fix(pr-pipeline): hold native arming on the gate's review-lane evidence The native path held a CRITICAL security finding only through the needs-human label, which the security lane applies only when its model fills the hold field. The gate holds on the counted finding itself. enable-auto-merge's reader now runs evidence --lane-reviews, which binds both lanes' evidence to the head as the gate does and applies the gate's lane rule, so an open CRITICAL finding or an unbound review leaves the pull request unarmed or disarms it. The README describes the new holds. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 1 + .../feat-native-lane-refs-arm-7d1f3b96.md | 1 + .../scripts/pr-merge/enable-auto-merge.mjs | 18 ++- .../pr-merge/enable-auto-merge.test.mjs | 34 ++++- .github/scripts/pr-merge/workflow.test.mjs | 2 +- .github/workflows/pr-merge.yml | 1 + docs/conventions/pr-pipeline/README.md | 16 ++- .../scripts/babysit_verify_evidence.py | 62 ++++++++- .../tests/test_babysit_lane_reviews.py | 124 ++++++++++++++++++ 9 files changed, 244 insertions(+), 15 deletions(-) create mode 100644 plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index 84eb944f54..c1473c5608 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -7,3 +7,4 @@ bump: minor - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. - The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run runs the post-merge check, as the merge job does, and reads the lane's room from it (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A room that cannot be read arms nothing and disarms nothing. The job now holds `pull-requests: read` and `issues: read` for those reads. - In the merge queue, `ci-status` holds a lanes App pull request whose verifier pass no longer binds its live issue link. It reads the counted pass's `verdict.json` (one artifact download) and the closing issues (one GraphQL read), and `merge-holds.mjs` `linkHold` holds unless the body still closes, or with no closing issue still names by a plain `Refs:` line, the one issue the pass judged, linked the same way. This is pr-merge's reader rule (`verify:spec-changed`), so a body edited after arming, such as `Refs: #A` to `Closes #B`, no longer merges on a pass for the old issue. A trusted approval of the head lifts it, as it lifts the verifier hold. `refsIssue` moves to `merge-holds.mjs`, which `resolve-pr.mjs` re-exports. +- `enable-auto-merge` holds a lanes App pull request on its review lanes as the merge gate does: `evidence --lane-reviews` binds both lanes' evidence to the head (`babysit_merge._lane_evidence`) and applies the gate's lane rule (`babysit_risk`). An open CRITICAL security-lane finding, or a review not bound to the head, leaves it unarmed or disarms it, whether or not the lane applied its hold label. The job now also holds `checks: read` for the lanes' annotations. diff --git a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md index 68f69e0252..08f0423cdd 100644 --- a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md +++ b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md @@ -6,3 +6,4 @@ bump: minor - `babysit_verify_evidence.py evidence --lane-link` first reads the pull request's issue link by the pr-merge lane's own rule (`babysit_merge_policy._linked_issue`: one closing issue in this repository, or, with none, the one issue its plain `Refs:` lines name) and adds `ineligible` to its line: null when the lane takes the link, else why not, with refusal `verify:ineligible` and no verdict or config read. A failed read refuses. The verdict read that follows takes the link read's pull request instead of reading it again. - `babysit_verify_evidence.py lane-room` prints whether the pr-merge lane may merge one more of its own pull requests now (`babysit_merge_policy.lane_room`): the gate's run-level refusals (another authority, a post-merge refusal, the effective rung off, a demotion included) as `refusal`, a failed read as `unreadable`, and otherwise the merges in flight against `merge.max-in-flight`. `InFlight` now carries that count. +- `babysit_verify_evidence.py evidence --lane-reviews` adds `reviews` to a line whose refusal is null: null when the gate would not hold the pull request on its review lanes' evidence (`lane_review_refusal`: the holding reasons of `babysit_risk`'s lane rule, over `babysit_merge._lane_evidence`), else why. A failed read or a moved head holds. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index ff50777aa2..b2708f937f 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -53,6 +53,12 @@ // lane's unverified merges and the armed pull requests not yet queued, it // arms; at the cap it arms nothing more. A room that cannot be read arms // nothing and disarms nothing. +// - pr-merge's gate would not hold it on its review lanes: `evidence +// --lane-reviews` binds both lanes' evidence to the head as the gate does +// (babysit_merge._lane_evidence) and applies the gate's lane rule +// (babysit_risk._lane_reasons, holding), so an open CRITICAL security +// finding, or a review not bound to the head, holds whether or not the +// lane applied its hold label. Read only after the verdict allows. // Arming passes the head just read as expectedHeadOid, so a push in between // fails the arm rather than arming a head nobody read. An armed pull request // that fails any rule is disarmed. While merge.report-only is true it reports @@ -220,8 +226,14 @@ export function verifyHold(evidence, head) { if (evidence.refusal !== null) { return `pr-merge's gate refuses its verifier verdict (${escapeCommand(evidence.refusal)})`; } - // A line without the link's answer read no link: it allows nothing. - return evidence.ineligible === null ? null : "its issue link could not be read"; + // A line without the link's or the reviews' answer read neither: it allows nothing. + if (evidence.ineligible !== null) { + return "its issue link could not be read"; + } + if (typeof evidence.reviews === "string") { + return `pr-merge's gate holds it on its review lanes (${escapeCommand(evidence.reviews)})`; + } + return evidence.reviews === null ? null : "its review lanes could not be read"; } // The gate's own reader, one process per pull request; the run shares its @@ -232,7 +244,7 @@ export function pythonVerify(env) { throw new Error("its base commit is not a commit SHA"); } // --base-sha: the gate's FAIL cap, the lower of the base's and main's. - const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "evidence", "--lane-link", "--repo", env.GITHUB_REPOSITORY, "--base-sha", base]; + const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "evidence", "--lane-link", "--lane-reviews", "--repo", env.GITHUB_REPOSITORY, "--base-sha", base]; const done = spawnSync("python3", [...args, String(number), head], { encoding: "utf8", env, timeout: VERIFY_TIMEOUT_MS }); if (done.status !== 0) { throw new Error(`babysit_verify_evidence.py exited ${done.status ?? done.signal}: ${String(done.stderr ?? "").trim().split("\n").at(-1)}`); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index a26b8d6939..f88955effd 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -76,7 +76,7 @@ const said = (login, body, at, association = "OWNER", typename = "User") => ({ createdAt: at, authorAssociation: association, }); -const PASSED = { refusal: null, head: HEAD, ineligible: null }; +const PASSED = { refusal: null, head: HEAD, ineligible: null, reviews: null }; const COST_LINK = `https://github.com/${REPO}/issues/1040#issuecomment-7`; const COST_COMMENT = { issue_url: `https://api.github.com/repos/${REPO}/issues/1040`, @@ -136,7 +136,7 @@ function world(pulls, { failArm = false } = {}) { if (found?.verdict instanceof Error) { throw found.verdict; } - return found?.verdict ?? { refusal: null, head, ineligible: null }; + return found?.verdict ?? { refusal: null, head, ineligible: null, reviews: null }; }; const writes = () => calls @@ -524,6 +524,9 @@ test("verifyHold allows only the reader's null refusal at the head read", () => assert.match(verifyHold({ refusal: "verify:ineligible", head: HEAD, ineligible: "refs-issues:2" }, HEAD), /does not take its issue link \(refs-issues:2\)/); assert.match(verifyHold({ refusal: null, head: HEAD }, HEAD), /issue link could not be read/); assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: false }, HEAD), /issue link could not be read/); + // The review lanes' answer: a hold names it, and a line without one allows nothing. + assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: null, reviews: "security-finding: 1 open" }, HEAD), /holds it on its review lanes \(security-finding: 1 open\)/); + assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: null }, HEAD), /review lanes could not be read/); }); // The merge queue's check (merge-holds.mjs verdictHold): a pass counts only @@ -609,7 +612,7 @@ test("a reader that fails, prints no JSON or reports another head arms nothing a garbage: "print('not json')\n", other: `import json\nprint(json.dumps({"refusal": None, "head": "${"c".repeat(40)}"}))\n`, unlinked: `import json, sys\nprint(json.dumps({"refusal": None, "head": sys.argv[-1]}))\n`, - passes: `import json, sys\nassert sys.argv[2] == "--lane-link" and sys.argv[-4:-2] == ["--base-sha", "${"d".repeat(40)}"], sys.argv\nprint(json.dumps({"refusal": None, "head": sys.argv[-1], "ineligible": None}))\n`, + passes: `import json, sys\nassert sys.argv[2:4] == ["--lane-link", "--lane-reviews"] and sys.argv[-4:-2] == ["--base-sha", "${"d".repeat(40)}"], sys.argv\nprint(json.dumps({"refusal": None, "head": sys.argv[-1], "ineligible": None, "reviews": None}))\n`, }; for (const [name, body] of Object.entries(scripts)) { writeFileSync(join(dir, `${name}.py`), body); @@ -673,7 +676,8 @@ def passes(self, number, head, pr=None): argv = sys.argv[1:] cut = argv.index("--base-sha") del argv[cut : cut + 2] -with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "evidence", passes): +pr = lambda repo, number: {"number": number, "head": {"sha": argv[-1]}} +with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "evidence", passes), mock.patch.object(policy, "pull_request", pr), mock.patch.object(ve, "lane_review_refusal", lambda repo, number, pr: None): sys.exit(ve.main(argv)) `, ); @@ -766,7 +770,7 @@ test("a room that cannot be read arms nothing and disarms nothing", async () => await main({ env: ENV, github, - verify: async ({ headRefOid: head }) => ({ refusal: null, head, ineligible: null }), + verify: async ({ headRefOid: head }) => ({ refusal: null, head, ineligible: null, reviews: null }), room: async () => { throw new Error("lane-room exited 1"); }, @@ -822,3 +826,23 @@ print(json.dumps({"argv": sys.argv[1:], "token": os.environ.get("GH_TOKEN"), "bu }); assert.deepEqual((await pythonRoom(env)()).argv, ["lane-room", "--repo", REPO, "--postmerge-result", "/tmp/post-merge.json"]); }); + +// The gate holds on the security lane's open CRITICAL findings +// (babysit_risk LANES, security-finding) and on a review not bound to the +// head, whether or not the lane applied its hold label. +test("a review-lane hold the gate reads arms nothing and disarms an armed pull request", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const held = { refusal: null, head: HEAD, ineligible: null, reviews: "security-finding: 1 open" }; + const { writes, lines } = await run([ + pull(1, { verdict: held }), + pull(2, { autoMergeRequest: armed, verdict: { ...held, reviews: "evidence-unbound:claude-security-review: marker-missing" } }), + pull(3, { verdict: { refusal: null, head: HEAD, ineligible: null } }), + pull(4), + ]); + assert.deepEqual(writes, [ + { mutation: "disarm", id: "PR_2" }, + { mutation: "arm", id: "PR_4", head: HEAD }, + ]); + assert.ok(lines.some((line) => line.includes("#1 left: pr-merge's gate holds it on its review lanes (security-finding: 1 open)"))); + assert.ok(lines.some((line) => line.includes("#3 left: its review lanes could not be read"))); +}); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 5936edd66a..94d7e73671 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -221,7 +221,7 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () const block = /\n {4}permissions:\n((?: {6}[a-z-]+: [a-z]+.*\n)+)/.exec(arm)[1]; assert.deepEqual( [...block.matchAll(/ {6}([a-z-]+): ([a-z]+)/g)].map((m) => `${m[1]}: ${m[2]}`), - ["contents: read", "actions: read", "pull-requests: read", "issues: read", "id-token: write"], + ["contents: read", "actions: read", "pull-requests: read", "issues: read", "checks: read", "id-token: write"], ); // The verifier evidence is read as the gate reads it: the job's // GITHUB_TOKEN for the Actions reads, with the run's head-ref map and diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 4b48fdd5e0..c5aa2ad756 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1346,6 +1346,7 @@ jobs: actions: read # the verifier verdicts, their runs and the head-ref map pull-requests: read # the post-merge check's lane merges and the merge queue, for the lane's room issues: read # the post-merge check's demotion records, read with this job's token + checks: read # the review lanes' annotations, as the gate binds their evidence id-token: write # prove to the token broker which workflow this is steps: - name: Check out the default branch tip diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index d29d490290..4644248ee0 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1067,14 +1067,26 @@ issue link and verifier verdict at that head pr-merge's gate would take: [eligibility](#the-pr-merge-lane) rule takes (one closing issue in this repository, or, with none, the one issue its plain `Refs: #` lines name; a failed read refuses), a PASS or PASS-NOTES of [pr-verify](#verifier-verdict) stands at the head, no FAIL is at it, no earlier FAIL is uncleared, FAILs sit at fewer than `merge.max-verify-fails` -heads, and the spec is unchanged and trusted. Only the scheduled run and a hand-started one read +heads, and the spec is unchanged and trusted; and `evidence --lane-reviews` finds no hold the gate +reads from the review lanes' evidence bound to the head (an open CRITICAL security finding, or a +review not bound to the head), whether or not the lane applied its hold label. Each such run also +runs the post-merge check, as the `merge` job does, and reads the lane's room from it +(`babysit_verify_evidence.py lane-room`): while the gate holds its lane off (a demotion record, +`merge.rung`, the loop setting or the rung cap off, a lane merge that failed its post-merge check) +nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, +counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue +has not taken, nothing more is armed; a room that cannot be read arms nothing and disarms nothing. +Only the scheduled run and a hand-started one read them and arm, last, for a pull request every other rule would arm or keep armed, with the gate's saved listing, head-ref map and run cache; an event run reads no link or verdict, never arms, and only disarms on a hold, so a pull request waits up to one scheduled interval to be armed. `ci-status` turns an armed lanes App pull request's auto-merge off on each push to its head, so a new head is armed only after the reader judges it. It turns auto-merge off on an armed one that now fails any of those, and does not turn it back on once a person has turned it off. GitHub then merges the pull request through the merge queue -once `ci-status` passes. The issue lane finishing (`workflow_run`) wakes it, so a pull request is +once `ci-status` passes, which in the queue also holds a pass that no longer binds the live link: +the body must still close, or with no closing issue name by a plain `Refs:` line, the one issue +the pass judged, linked the same way (`merge-holds.mjs` `linkHold`), so a body edited after arming +does not merge on a pass for another issue. The issue lane finishing (`workflow_run`) wakes it, so a pull request is armed right after report-outcome opens it; a listed human's edit, unlabel or ready wakes it too, so adding a `Hold:` line disarms at once; and the 15-minute schedule arms or disarms what the events missed. The token broker mints `merge` only for `pr-merge.yml`, and outside the schedule only for a 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 954893ba1e..6a73f5ad24 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 @@ -112,7 +112,8 @@ from dataclasses import dataclass, field from datetime import UTC, datetime, timedelta from pathlib import Path -from typing import Any +from types import SimpleNamespace +from typing import Any, cast import babysit_merge_policy as merge_policy import babysit_postmerge as postmerge @@ -1425,6 +1426,40 @@ def queued_hold(self, number: int) -> str | None: return self.evidence(number, head, pr).refusal +def lane_review_refusal(repo: str, number: int, pr: dict[str, Any]) -> str | None: + """Why pr-merge's gate would hold the pull request on its review lanes' + evidence, or None: the holding lane reasons of `babysit_risk` + (`_lane_reasons`, `holding`), over the evidence the gate binds + (`babysit_merge._lane_evidence`). The security lane's open CRITICAL + findings hold, and so does a lane whose review is not bound to the head. + A read that fails holds.""" + # Imported here: babysit_merge imports this module. + import babysit_merge # noqa: PLC0415 + import babysit_risk # noqa: PLC0415 + + try: + identity = merge_policy._default_branch_config(repo).agent_identity + if not identity: + return "unreadable: no agent identity" + files, complete = merge_policy.changed_files(repo, pr) + lanes, _ = babysit_merge._lane_evidence( + repo, + number, + pr, + identity, + frozenset(), + merge_policy.permission_reader(repo), + [p for f in files for p in (f.path, f.previous_path) if p] + if complete + else None, + ) + reasons = babysit_risk._lane_reasons(lanes) + except Exception as exc: # noqa: BLE001 - any doubt holds, as the gate's + return f"unreadable: {type(exc).__name__}: {exc}" + held = babysit_risk.holding(cast(Any, SimpleNamespace(reasons=reasons))) + return "; ".join(f"{r.code}: {r.detail}" for r in held) or None + + def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: """`evidence `: one pull request's evidence at `head` as one JSON line (`Evidence.report`), as pr-merge's gate judges it. @@ -1441,7 +1476,10 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: With `--lane-link`, the line also carries `ineligible`: null when the pull request's issue link is one the pr-merge lane takes (`Reader.link_refusal`), else why not, with refusal `verify:ineligible` - and no verdict or config read. + and no verdict or config read. With `--lane-reviews`, a line whose + refusal is null also carries `reviews`: null when pr-merge's gate would + not hold the pull request on its review lanes (`lane_review_refusal`), + else why. `lane-room --postmerge-result [--rung-cap ]`: whether the pr-merge lane may merge one more of its own pull requests now @@ -1458,6 +1496,7 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: cmd.add_argument("--max-fails", type=int, default=merge_policy.MAX_VERIFY_FAILS) cmd.add_argument("--base-sha", default=None) cmd.add_argument("--lane-link", action="store_true") + cmd.add_argument("--lane-reviews", action="store_true") cmd.add_argument("number", type=int) cmd.add_argument("head") room = sub.add_parser("lane-room", allow_abbrev=False) @@ -1500,10 +1539,25 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: print(json.dumps(refusal.report() | link)) return 0 reader.max_fails = config.max_verify_fails - evidence = reader.evidence(args.number, args.head) + pr = None + if args.lane_reviews: + try: + pr = merge_policy.pull_request(args.repo, args.number) + except merge_policy.PolicyReadError as exc: + refusal = Evidence(UNREADABLE, args.head, (), 0, str(exc)) + print(json.dumps(refusal.report() | link)) + return 0 + evidence = reader.evidence(args.number, args.head, pr) if evidence.refusal is None and reader.saved: reader.refresh() - evidence = reader.evidence(args.number, args.head) + evidence = reader.evidence(args.number, args.head, pr) + if args.lane_reviews and evidence.refusal is None: + head = json_object(json_object(pr).get("head")).get("sha") + link["reviews"] = ( + lane_review_refusal(args.repo, args.number, json_object(pr)) + if head == args.head + else f"unreadable: the head is {head}, not {args.head}" + ) print(json.dumps(evidence.report() | link)) return 0 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 new file mode 100644 index 0000000000..b08cf6c8d6 --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py @@ -0,0 +1,124 @@ +"""The review-lane hold enable-auto-merge reads (`lane_review_refusal`). + +Expected values come from the gate's lane rule (babysit_risk.py, `LANES` and +`holding`): the security lane's open CRITICAL findings hold +(`security-finding`, HIGH), a lane whose evidence is not bound to the head +holds (`evidence-unbound:`), and claude-review's open findings are +reported and never hold. A read that fails holds. +""" + +from __future__ import annotations + +import contextlib +import io +import json +import pathlib +import sys +import unittest +from unittest import mock + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) + +import babysit_merge # noqa: E402 +import babysit_merge_policy as policy # noqa: E402 +import babysit_verify_evidence as ve # noqa: E402 +from babysit_risk import LaneEvidence # noqa: E402 + +REPO = "example-org/example-repo" +HEAD = "a" * 40 +PR = {"number": 41, "head": {"sha": HEAD, "ref": "feat/x"}, "base": {"sha": "b" * 40}} +CONFIG = policy.PipelineConfig( + "c" * 40, "C2", "pr-merge", "agent-lanes[bot]", None, (), None +) + + +def lanes( + *, code: int = 0, critical: int = 0, security_bound: bool = True +) -> tuple[LaneEvidence, ...]: + return ( + LaneEvidence("claude-review", True, None, code), + LaneEvidence( + "claude-security-review", + security_bound, + None if security_bound else "marker-missing", + critical if security_bound else None, + False, + critical if security_bound else None, + ), + ) + + +def refusal(evidence: object) -> str | None: + effect = evidence if isinstance(evidence, Exception) else None + with ( + mock.patch.object(policy, "_default_branch_config", return_value=CONFIG), + mock.patch.object(policy, "changed_files", return_value=((), True)), + mock.patch.object(policy, "permission_reader", return_value=lambda login: None), + mock.patch.object( + babysit_merge, + "_lane_evidence", + side_effect=effect, + return_value=None if effect else (evidence, []), + ), + ): + return ve.lane_review_refusal(REPO, 41, PR) + + +class LaneReviewRefusal(unittest.TestCase): + def test_a_critical_security_finding_holds(self) -> None: + self.assertEqual(refusal(lanes(critical=1)), "security-finding: 1 open") + + def test_clean_lanes_and_a_code_finding_hold_nothing(self) -> None: + self.assertIsNone(refusal(lanes())) + self.assertIsNone(refusal(lanes(code=3))) + + def test_an_unbound_security_review_holds(self) -> None: + held = refusal(lanes(security_bound=False)) + self.assertTrue( + str(held).startswith("evidence-unbound:claude-security-review"), held + ) + + def test_a_failed_read_holds(self) -> None: + held = refusal(babysit_merge.HeadPusherUnproven("head-pusher-unproven:abc")) + self.assertTrue(str(held).startswith("unreadable: "), held) + + +class EvidenceCommandReadsTheReviews(unittest.TestCase): + def cli( + self, evidence: ve.Evidence, reviews: str | None, head: str = HEAD + ) -> dict[str, object]: + out = io.StringIO() + with ( + mock.patch.object( + policy, "pull_request", return_value=PR | {"head": {"sha": head}} + ), + mock.patch.object( + ve.Reader, "evidence", lambda self, n, h, pr=None: evidence + ), + mock.patch.object(ve, "lane_review_refusal", return_value=reviews) as read, + contextlib.redirect_stdout(out), + ): + ve.main(["evidence", "--lane-reviews", "--repo", REPO, "41", HEAD]) + self.read = read + return json.loads(out.getvalue()) + + def test_a_pass_carries_the_review_hold(self) -> None: + report = self.cli(ve.Evidence(None, HEAD, (), 0), "security-finding: 1 open") + self.assertEqual( + (report["refusal"], report["reviews"]), (None, "security-finding: 1 open") + ) + self.assertIsNone(self.cli(ve.Evidence(None, HEAD, (), 0), None)["reviews"]) + + def test_a_refused_verdict_reads_no_reviews(self) -> None: + report = self.cli(ve.Evidence(ve.FAIL, HEAD, (), 0), None) + self.assertNotIn("reviews", report) + self.read.assert_not_called() + + def test_a_moved_head_holds(self) -> None: + report = self.cli(ve.Evidence(None, HEAD, (), 0), None, head="d" * 40) + self.assertTrue(str(report["reviews"]).startswith("unreadable: the head is")) + self.read.assert_not_called() + + +if __name__ == "__main__": + unittest.main() From 18a1bba75c7b6af21ba01e178d06bab2c140b5f1 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:44:36 -0400 Subject: [PATCH 07/13] fix(pr-pipeline): fit the enable-auto-merge job's new reads inside its timeouts The post-merge check gets its own 6-minute timeout, the arm step 14 minutes for the room read plus the verdict budget, and the job 30, with workflow.test.mjs checking that the step timeouts fit inside the job's. The verdict budget now starts after the room read. The pr-pipeline.yaml activity comment and the job header comment name the new holds. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/enable-auto-merge.mjs | 10 ++++++---- .github/scripts/pr-merge/workflow.test.mjs | 11 ++++++++++- .github/workflows/pr-merge.yml | 17 +++++++++++------ docs/conventions/pr-pipeline.yaml | 6 ++++-- 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index b2708f937f..8a8b2ac746 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -299,9 +299,10 @@ export function readRoom(answer) { // stuck. export const READER_EVENTS = Object.freeze(["schedule", "workflow_dispatch"]); // The reader's per pull request limit, and when the run stops starting one: -// a read started at the budget ends by VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS, -// 9 minutes, inside the step's 10 (pr-merge.yml), so no read is cut off and -// the rest of the pull requests are left for the next run. +// the room read (at most VERIFY_TIMEOUT_MS) comes first, then a read started +// at the budget ends by VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS, 13 minutes in +// all, inside the step's 14 (pr-merge.yml), so no read is cut off and the +// rest of the pull requests are left for the next run. export const VERIFY_TIMEOUT_MS = 4 * 60_000; export const VERIFY_BUDGET_MS = 5 * 60_000; @@ -387,7 +388,6 @@ export async function main({ env = process.env, github, verify, room } = {}) { verify ??= pythonVerify(env); const reads = READER_EVENTS.includes(env.EVENT_NAME); const pulls = await listPulls(github, repo[1], repo[2]); - const deadline = Date.now() + VERIFY_BUDGET_MS; const lanes = (pull) => pull.author?.__typename === "Bot" && pull.author.login === slug; // Only a reader run arms, so only it reads the room. let lane = null; @@ -400,6 +400,8 @@ export async function main({ env = process.env, github, verify, room } = {}) { } console.log(`enable-auto-merge: lane room: ${escapeCommand(JSON.stringify(lane))}`); } + // After the room read, so the verdict reads keep their whole budget. + const deadline = Date.now() + VERIFY_BUDGET_MS; // Merges in flight: the gate's count, plus each armed lanes App pull // request the queue has not taken yet, which GitHub enqueues on its own. let inFlight = lane === null ? 0 : lane.count + pulls.filter((pull) => lanes(pull) && pull.autoMergeRequest != null && !lane.queued.includes(pull.number)).length; diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 94d7e73671..e17abe1850 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -168,7 +168,8 @@ test("the last verifier read enable-auto-merge starts ends inside its step's tim const arm = job("enable-auto-merge"); const step = /- name: Arm and disarm auto-merge\n {8}if: .*\n {8}timeout-minutes: ([0-9]+)\n/.exec(arm); assert.ok(step, "the arm step names its timeout"); - assert.ok(VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS < Number(step[1]) * 60_000); + // The room read, then the verdict budget and the last read it starts. + assert.ok(VERIFY_TIMEOUT_MS + VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS < Number(step[1]) * 60_000); }); test("enable-auto-merge logs both tokens' readings at job end, before the token is revoked", () => { @@ -206,6 +207,14 @@ test("enable-auto-merge reads the lane's room from this run's post-merge check a assert.ok(names.indexOf("Check earlier lane merges") < names.indexOf("Arm and disarm auto-merge")); }); +test("enable-auto-merge's step timeouts fit inside its job's", () => { + const arm = job("enable-auto-merge"); + const total = Number(/\n {4}timeout-minutes: ([0-9]+)\n/.exec(arm)[1]); + const steps = [...arm.matchAll(/\n {8}timeout-minutes: ([0-9]+)\n/g)].map((m) => Number(m[1])); + assert.ok(steps.length >= 4, "the post-merge check, the cache restore, the arm step and the budget step name timeouts"); + assert.ok(steps.reduce((sum, minutes) => sum + minutes, 0) < total, `${steps.join(" + ")} inside ${total}`); +}); + test("enable-auto-merge asks the broker for its own activity's merge token", () => { const arm = job("enable-auto-merge"); const steps = [...arm.matchAll(/\n {10}lane: (.*)\n {10}activity: (.*)\n/g)].map((m) => `${m[1]}/${m[2]}`); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index c5aa2ad756..dc44c1f5ff 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1320,10 +1320,13 @@ jobs: # catches what the events missed, such as a run the lanes App started, which # the broker refuses. Only the scheduled run and a hand-started one arm, on # the issue link (one closing issue, or one plain `Refs:` issue) and verdict - # pr-merge's gate would take at the head (babysit_verify_evidence.py - # `evidence --lane-link`, its Actions reads with the job's GITHUB_TOKEN from - # the head-ref map and run cache the last gate run kept); event runs read - # neither and only disarm on a hold. + # pr-merge's gate would take at the head, with no review-lane hold the gate + # reads (babysit_verify_evidence.py `evidence --lane-link --lane-reviews`, + # its Actions reads with the job's GITHUB_TOKEN from the head-ref map and + # run cache the last gate run kept), and only while the gate's lane has + # room (`lane-room`, from the post-merge check this job runs): while the + # gate holds the lane off they also disarm every armed lane pull request. + # Event runs read none of these and only disarm on a hold. # Its own job and token: the broker mints one token per # job. Rules: .github/scripts/pr-merge/enable-auto-merge.mjs. enable-auto-merge: @@ -1340,7 +1343,8 @@ jobs: group: pr-merge-enable-auto-merge queue: max runs-on: ubuntu-24.04 - timeout-minutes: 15 + # The sum of its steps' timeouts (workflow.test.mjs checks it). + timeout-minutes: 30 permissions: contents: read # check out main actions: read # the verifier verdicts, their runs and the head-ref map @@ -1468,6 +1472,7 @@ jobs: # gate. Only the runs that arm read it. - name: Check earlier lane merges if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') + timeout-minutes: 6 env: GH_TOKEN: ${{ github.token }} API_BUDGET_TOKEN: github-token @@ -1488,7 +1493,7 @@ jobs: # decision and writes nothing. - name: Arm and disarm auto-merge if: steps.lane-token.outputs.token != '' - timeout-minutes: 10 + timeout-minutes: 14 env: GH_TOKEN: ${{ steps.lane-token.outputs.token }} KILL_SWITCH: ${{ steps.kill.outputs.proceed == 'true' && 'proceed' || '' }} diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index d7eaa7f35b..df96b6f363 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -394,8 +394,10 @@ activities: # gate takes (one closing issue, or one plain `Refs: #` issue) and # that carries no hold (a `Hold:` line, do-not-merge, the human-gated # label, a member's "do not merge" comment, a head that changes a denied - # path), and turns it off on an armed one that now carries a hold; - # auto-merge a person turned off stays off. No + # path, a review-lane hold the gate reads), while the gate's lane has + # room (no demotion, rung on, below merge.max-in-flight), and turns it + # off on an armed one that now carries a hold or while the gate holds the + # lane off; auto-merge a person turned off stays off. No # model. The enable-auto-merge job of .github/workflows/pr-merge.yml runs # it when the issue lane finishes, on a listed human's PR edit, and on the # schedule. From f2445dc0a518e4896640a04d5f3db14db0254c27 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:48:07 -0400 Subject: [PATCH 08/13] perf(pr-merge): read the lane's room from post-merge-dispatch's report Running the post-merge check a third time per scheduled run added about 350 GITHUB_TOKEN requests an hour against a 1,000 an hour limit. post-merge-dispatch now keeps its report as a one-day artifact, and enable-auto-merge needs that job and downloads the report from this run only. With no report the room is unreadable: nothing is armed and nothing is disarmed. Event runs, which skip post-merge-dispatch, run as before. The job drops issues: read and its timeout falls to 25 minutes. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 2 +- .github/scripts/pr-merge/merge-job.test.mjs | 1 + .github/scripts/pr-merge/workflow.test.mjs | 38 +++++---- .github/workflows/pr-merge.yml | 83 ++++++++++--------- docs/conventions/pr-pipeline/README.md | 5 +- 5 files changed, 71 insertions(+), 58 deletions(-) diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index c1473c5608..52b8d45ac1 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -5,6 +5,6 @@ bump: minor ### Changed - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. -- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run runs the post-merge check, as the merge job does, and reads the lane's room from it (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A room that cannot be read arms nothing and disarms nothing. The job now holds `pull-requests: read` and `issues: read` for those reads. +- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, or a room that cannot be read, arms nothing and disarms nothing. The job now holds `pull-requests: read` for the merge-queue and merge reads. - In the merge queue, `ci-status` holds a lanes App pull request whose verifier pass no longer binds its live issue link. It reads the counted pass's `verdict.json` (one artifact download) and the closing issues (one GraphQL read), and `merge-holds.mjs` `linkHold` holds unless the body still closes, or with no closing issue still names by a plain `Refs:` line, the one issue the pass judged, linked the same way. This is pr-merge's reader rule (`verify:spec-changed`), so a body edited after arming, such as `Refs: #A` to `Closes #B`, no longer merges on a pass for the old issue. A trusted approval of the head lifts it, as it lifts the verifier hold. `refsIssue` moves to `merge-holds.mjs`, which `resolve-pr.mjs` re-exports. - `enable-auto-merge` holds a lanes App pull request on its review lanes as the merge gate does: `evidence --lane-reviews` binds both lanes' evidence to the head (`babysit_merge._lane_evidence`) and applies the gate's lane rule (`babysit_risk`). An open CRITICAL security-lane finding, or a review not bound to the head, leaves it unarmed or disarms it, whether or not the lane applied its hold label. The job now also holds `checks: read` for the lanes' annotations. diff --git a/.github/scripts/pr-merge/merge-job.test.mjs b/.github/scripts/pr-merge/merge-job.test.mjs index 31647eed77..26b7f8d4e1 100644 --- a/.github/scripts/pr-merge/merge-job.test.mjs +++ b/.github/scripts/pr-merge/merge-job.test.mjs @@ -52,6 +52,7 @@ test("the re-dispatch for earlier lane merges waits on neither the listing nor t "Read report-only", "Read the lane records the last run kept", "Check earlier lane merges", + "Keep the post-merge report for enable-auto-merge", "Start the full CI run on main", "Record the dispatched run", "Re-run a red lane-dispatched full run", diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index e17abe1850..aac104a035 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -188,23 +188,33 @@ test("enable-auto-merge logs both tokens' readings at job end, before the token assert.ok(names.indexOf("Log the API budget at job end") < names.indexOf("Revoke the lane token")); }); -// The lane's room (lane-room) needs the run's post-merge check and the rung -// cap as the merge job passes it: an unset cap holds the lane off, so a step -// that drops it would disarm every lane pull request. -test("enable-auto-merge reads the lane's room from this run's post-merge check and the rung cap", () => { +// The lane's room (lane-room) reads this run's post-merge report, which +// post-merge-dispatch keeps, rather than running the check a third time per +// scheduled run, and the rung cap as the merge job passes it: an unset cap +// holds the lane off, so a step that drops it would disarm every lane pull +// request. +test("enable-auto-merge reads the lane's room from this run's post-merge-dispatch report and the rung cap", () => { + const dispatch = job("post-merge-dispatch"); + const keep = /- name: Keep the post-merge report for enable-auto-merge\n([\s\S]*?)(?=\n {6}- name: |\n*$)/.exec(dispatch)?.[1]; + assert.ok(keep, "post-merge-dispatch keeps its report"); + assert.match(keep, /uses: actions\/upload-artifact@[0-9a-f]{40} /); + assert.match(keep, /\n {10}name: pr-merge-post-merge-report\n {10}path: \$\{\{ runner\.temp \}\}\/post-merge\.json\n {10}retention-days: 1\n/); const arm = job("enable-auto-merge"); - const check = /- name: Check earlier lane merges\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; - assert.ok(check, "a post-merge check step"); - assert.match(check, /^ {8}if: steps\.lane-token\.outputs\.token != '' && \(github\.event_name == 'schedule' \|\| github\.event_name == 'workflow_dispatch'\)\n/); - assert.match(check, /\n {10}GH_TOKEN: \$\{\{ github\.token \}\}\n/); - assert.match(check, /babysit_postmerge\.py \\\n {12}--repo "\$REPO" --agent-identity "\$identity" --lane-merges-only \\\n {12}<\/dev\/null >"\$RUNNER_TEMP\/post-merge\.json"/); + assert.match(arm, /^ {4}# [\s\S]*?\n {4}needs: \[post-merge-dispatch\]\n/); + assert.match(/\n {4}if: >-\n([\s\S]*?)\n {4}[a-z-]+:/.exec(arm)[1], /^ {6}!cancelled\(\)\n/); + // No check of its own: the report comes from this run's job alone. + assert.doesNotMatch(arm, /python3 [^\n]*babysit_postmerge\.py/); + const read = /- name: Read this run's post-merge report\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; + assert.ok(read, "a download of the report"); + assert.match(read, /uses: actions\/download-artifact@[0-9a-f]{40} /); + assert.match(read, /\n {10}name: pr-merge-post-merge-report\n/); + assert.match(read, /needs\.post-merge-dispatch\.result == 'success'/); + assert.doesNotMatch(read, /run-id|github-token|repository:/); const step = /- name: Arm and disarm auto-merge\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)[1]; - assert.match(step, /\n {10}POSTMERGE_RESULT: \$\{\{ runner\.temp \}\}\/post-merge\.json\n/); + assert.match(step, /\n {10}POSTMERGE_RESULT: \$\{\{ runner\.temp \}\}\/post-merge\/post-merge\.json\n/); assert.match(step, /\n {10}RUNG_CAP: \$\{\{ vars\.CLAUDE_LANES_MERGE_RUNG_CAP \}\}\n/); - assert.match(arm, /\n {6}pull-requests: read #/); - assert.match(arm, /\n {6}issues: read #/); const names = [...arm.matchAll(/\n {6}- name: (.*)/g)].map((m) => m[1]); - assert.ok(names.indexOf("Check earlier lane merges") < names.indexOf("Arm and disarm auto-merge")); + assert.ok(names.indexOf("Read this run's post-merge report") < names.indexOf("Arm and disarm auto-merge")); }); test("enable-auto-merge's step timeouts fit inside its job's", () => { @@ -230,7 +240,7 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () const block = /\n {4}permissions:\n((?: {6}[a-z-]+: [a-z]+.*\n)+)/.exec(arm)[1]; assert.deepEqual( [...block.matchAll(/ {6}([a-z-]+): ([a-z]+)/g)].map((m) => `${m[1]}: ${m[2]}`), - ["contents: read", "actions: read", "pull-requests: read", "issues: read", "checks: read", "id-token: write"], + ["contents: read", "actions: read", "pull-requests: read", "checks: read", "id-token: write"], ); // The verifier evidence is read as the gate reads it: the job's // GITHUB_TOKEN for the Actions reads, with the run's head-ref map and diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index dc44c1f5ff..122d4229ff 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -213,6 +213,20 @@ jobs: rerun=$(jq -r '[.reruns[]? | select(.laneDispatched == true) | .runId][0] // ""' "$RUNNER_TEMP/post-merge.json" 2>/dev/null || true) echo "lane-rerun=$rerun" >>"$GITHUB_OUTPUT" + # The check's report for this run's enable-auto-merge job, which reads + # the lane's room from it instead of running the check again. That job + # downloads it from this run only. + - name: Keep the post-merge report for enable-auto-merge + if: steps.post-merge.conclusion == 'success' + continue-on-error: true + timeout-minutes: 2 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: pr-merge-post-merge-report + path: ${{ runner.temp }}/post-merge.json + retention-days: 1 + if-no-files-found: error + # The same body as dispatch-full-run's step of this name; see there. # WAIT_CAP: of the 15 minutes, the whole job took 97-127 s on the last # ten scheduled runs read on 2026-10-23 (the post-merge check 81-111 s @@ -1324,16 +1338,21 @@ jobs: # reads (babysit_verify_evidence.py `evidence --lane-link --lane-reviews`, # its Actions reads with the job's GITHUB_TOKEN from the head-ref map and # run cache the last gate run kept), and only while the gate's lane has - # room (`lane-room`, from the post-merge check this job runs): while the + # room (`lane-room`, from this run's post-merge-dispatch report): while the # gate holds the lane off they also disarm every armed lane pull request. # Event runs read none of these and only disarm on a hold. # Its own job and token: the broker mints one token per # job. Rules: .github/scripts/pr-merge/enable-auto-merge.mjs. enable-auto-merge: + # On the schedule and a hand-started run it waits for post-merge-dispatch + # and reads its report; on every other event that job is skipped, and + # !cancelled() runs this one anyway. + needs: [post-merge-dispatch] # An issue run that skipped every job (a label other than agent-ready) # opened nothing. if: >- - github.ref == 'refs/heads/main' && github.event_name != 'push' + !cancelled() + && github.ref == 'refs/heads/main' && github.event_name != 'push' && (github.event_name != 'workflow_run' || github.event.workflow_run.conclusion != 'skipped') && (github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.full_name == github.repository) @@ -1343,13 +1362,12 @@ jobs: group: pr-merge-enable-auto-merge queue: max runs-on: ubuntu-24.04 - # The sum of its steps' timeouts (workflow.test.mjs checks it). - timeout-minutes: 30 + # Above the sum of its steps' timeouts (workflow.test.mjs checks it). + timeout-minutes: 25 permissions: contents: read # check out main actions: read # the verifier verdicts, their runs and the head-ref map - pull-requests: read # the post-merge check's lane merges and the merge queue, for the lane's room - issues: read # the post-merge check's demotion records, read with this job's token + pull-requests: read # the lane's merges and the merge queue, for the lane's room checks: read # the review lanes' annotations, as the gate binds their evidence id-token: write # prove to the token broker which workflow this is steps: @@ -1454,40 +1472,23 @@ jobs: CARRY_TO: ${{ runner.temp }}/babysit-run-cache.json run: node .base/.github/scripts/pr-merge/carry-run-cache.mjs - # The lane records the last run kept, as the merge job reads them, so the - # check below answers those with no read. Any error leaves no file, and - # the check reads each record. Rules: .github/scripts/pr-merge/lane-records.mjs. - - name: Read the lane records the last run kept - if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') + # This run's post-merge report, which post-merge-dispatch kept: the + # gate's run-level answer that enable-auto-merge.mjs reads the lane's + # room from (lane-room), so a demotion or a failed lane merge stops + # native arming as it stops the gate. Downloaded from this run only (no + # run-id is given, so no other run's artifact is read). With no report + # the room is unreadable: nothing is armed and nothing is disarmed. + - name: Read this run's post-merge report + if: >- + steps.lane-token.outputs.token != '' + && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') + && needs.post-merge-dispatch.result == 'success' continue-on-error: true - env: - ACTIONS_TOKEN: ${{ github.token }} - LANE_RECORDS_PATH: ${{ runner.temp }}/lane-records.json - run: node .base/.github/scripts/pr-merge/lane-records.mjs read - - # The merge job's post-merge check, read-only, with this job's token: - # its demotion, floor and verdict are the gate's run-level answer that - # enable-auto-merge.mjs reads the lane's room from (lane-room), so a - # demotion or a failed lane merge stops native arming as it stops the - # gate. Only the runs that arm read it. - - name: Check earlier lane merges - if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') - timeout-minutes: 6 - env: - GH_TOKEN: ${{ github.token }} - API_BUDGET_TOKEN: github-token - BABYSIT_POSTMERGE_LANE_RECORDS: ${{ runner.temp }}/lane-records.json - REPO: ${{ github.repository }} - run: | - set -euo pipefail - identity=$(yq '.merge."agent-identity" // ""' .base/docs/conventions/pr-pipeline.yaml) - code=0 - python3 .base/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py \ - --repo "$REPO" --agent-identity "$identity" --lane-merges-only \ - "$RUNNER_TEMP/post-merge.json" || code=$? - echo "::group::post-merge check: exit $code" - cat "$RUNNER_TEMP/post-merge.json" - echo "::endgroup::" + timeout-minutes: 2 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 + with: + name: pr-merge-post-merge-report + path: ${{ runner.temp }}/post-merge # While merge.report-only is anything but false it reports each # decision and writes nothing. @@ -1503,9 +1504,9 @@ jobs: BABYSIT_VERIFY_LISTING: ${{ runner.temp }}/verify-listing.json BABYSIT_VERIFY_HEAD_REFS: ${{ runner.temp }}/head-refs-in.json BABYSIT_RUN_CACHE: ${{ runner.temp }}/babysit-run-cache.json - # The lane's room (lane-room): the post-merge check above and the + # The lane's room (lane-room): this run's post-merge report and the # rung cap as the merge job passes it, empty meaning no cap. - POSTMERGE_RESULT: ${{ runner.temp }}/post-merge.json + POSTMERGE_RESULT: ${{ runner.temp }}/post-merge/post-merge.json RUNG_CAP: ${{ vars.CLAUDE_LANES_MERGE_RUNG_CAP }} # Name the two tokens' rate-limit readings for the reader's gh calls # (api-budget.bash): the lane token by default, the job token for diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 4644248ee0..5cd68715c3 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1070,8 +1070,9 @@ no FAIL is at it, no earlier FAIL is uncleared, FAILs sit at fewer than `merge.m heads, and the spec is unchanged and trusted; and `evidence --lane-reviews` finds no hold the gate reads from the review lanes' evidence bound to the head (an open CRITICAL security finding, or a review not bound to the head), whether or not the lane applied its hold label. Each such run also -runs the post-merge check, as the `merge` job does, and reads the lane's room from it -(`babysit_verify_evidence.py lane-room`): while the gate holds its lane off (a demotion record, +waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge +report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it +arms nothing and disarms nothing): while the gate holds its lane off (a demotion record, `merge.rung`, the loop setting or the rung cap off, a lane merge that failed its post-merge check) nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue From 7f9526c479ab8038ff145f32889e0ac8d74f43a8 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:52:34 -0400 Subject: [PATCH 09/13] fix(pr-pipeline): disarm armed lane pull requests when the lane's room is unknown roomHold returned early for an armed pull request before it checked an unreadable room, so a missing post-merge report or a failed read left armed lane pull requests armed while the gate refused. Unknown holds: on a reader run an unreadable room now disarms every armed lane pull request and arms nothing, as a refusal does. Event runs read no room and are unchanged. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 12 ++++++---- .../pr-merge/enable-auto-merge.test.mjs | 24 ++++++++++++++----- .github/workflows/pr-merge.yml | 3 ++- docs/conventions/pr-pipeline/README.md | 5 ++-- .../scripts/babysit_merge_policy.py | 2 +- 6 files changed, 32 insertions(+), 16 deletions(-) diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index 52b8d45ac1..9afcda9b99 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -5,6 +5,6 @@ bump: minor ### Changed - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. -- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, or a room that cannot be read, arms nothing and disarms nothing. The job now holds `pull-requests: read` for the merge-queue and merge reads. +- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, or a room that cannot be read, holds as a refusal does: nothing is armed and every armed lanes App pull request is disarmed. The job now holds `pull-requests: read` for the merge-queue and merge reads. - In the merge queue, `ci-status` holds a lanes App pull request whose verifier pass no longer binds its live issue link. It reads the counted pass's `verdict.json` (one artifact download) and the closing issues (one GraphQL read), and `merge-holds.mjs` `linkHold` holds unless the body still closes, or with no closing issue still names by a plain `Refs:` line, the one issue the pass judged, linked the same way. This is pr-merge's reader rule (`verify:spec-changed`), so a body edited after arming, such as `Refs: #A` to `Closes #B`, no longer merges on a pass for the old issue. A trusted approval of the head lifts it, as it lifts the verifier hold. `refsIssue` moves to `merge-holds.mjs`, which `resolve-pr.mjs` re-exports. - `enable-auto-merge` holds a lanes App pull request on its review lanes as the merge gate does: `evidence --lane-reviews` binds both lanes' evidence to the head (`babysit_merge._lane_evidence`) and applies the gate's lane rule (`babysit_risk`). An open CRITICAL security-lane finding, or a review not bound to the head, leaves it unarmed or disarms it, whether or not the lane applied its hold label. The job now also holds `checks: read` for the lanes' annotations. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 8a8b2ac746..bd2360658e 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -51,8 +51,9 @@ // nothing is armed and every armed lanes App pull request is disarmed. // Below merge.max-in-flight, counting the merge queue's lane entries, the // lane's unverified merges and the armed pull requests not yet queued, it -// arms; at the cap it arms nothing more. A room that cannot be read arms -// nothing and disarms nothing. +// arms; at the cap it arms nothing more. A room that cannot be read (no +// post-merge report, a failed read) holds as a refusal does: nothing is +// armed and every armed lanes App pull request is disarmed. // - pr-merge's gate would not hold it on its review lanes: `evidence // --lane-reviews` binds both lanes' evidence to the head as the gate does // (babysit_merge._lane_evidence) and applies the gate's lane rule @@ -414,12 +415,13 @@ export async function main({ env = process.env, github, verify, room } = {}) { if (lane.refusal !== null) { return { disarm: true, reason: `pr-merge's gate holds its lane off (${lane.refusal})` }; } + // An unknown room holds as a refusal does: unknown holds. + if (lane.unreadable !== null) { + return { disarm: true, reason: `the lane's room could not be read (${lane.unreadable})` }; + } if (pull.autoMergeRequest != null) { return null; } - if (lane.unreadable !== null) { - return { disarm: false, reason: `the lane's room could not be read (${lane.unreadable})` }; - } return inFlight >= lane.cap ? { disarm: false, reason: `merge.max-in-flight is ${lane.cap} and ${inFlight} merges are in flight` } : null; }; let failed = 0; diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index f88955effd..d27069f046 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -756,12 +756,17 @@ test("at merge.max-in-flight nothing more is armed, counting armed pull requests ); }); -test("a room that cannot be read arms nothing and disarms nothing", async () => { +// Unknown holds: a room that cannot be read (no post-merge report, a failed +// read) holds as the gate's refusal does, so an armed lane pull request is +// disarmed rather than left to merge while the gate refuses. +test("a room that cannot be read arms nothing and disarms every armed lane pull request", async () => { const armed = { enabledAt: "2026-10-23T10:00:00Z" }; - const pulls = [pull(1), pull(2, { autoMergeRequest: armed })]; - for (const lane of [{ ...ROOM, unreadable: "in-flight:merge-queue-unreadable" }, { refusal: null }, null]) { - const { writes } = await run(pulls, {}, {}, lane); - assert.deepEqual(writes, [], JSON.stringify(lane)); + const pulls = [pull(1), pull(2, { autoMergeRequest: armed }), pull(3, { author: { __typename: "User", login: "kyle-sexton" }, autoMergeRequest: armed })]; + for (const lane of [{ ...ROOM, unreadable: "post-merge answer missing or unreadable" }, { refusal: null }, null]) { + const { writes, verified, lines } = await run(pulls, {}, {}, lane); + assert.deepEqual(writes, [{ mutation: "disarm", id: "PR_2" }], JSON.stringify(lane)); + assert.deepEqual(verified, [], JSON.stringify(lane)); + assert.ok(lines.some((line) => line.includes("#2 disarmed: the lane's room could not be read")), JSON.stringify(lane)); } const { github, writes } = world(pulls); const log = console.log; @@ -778,7 +783,14 @@ test("a room that cannot be read arms nothing and disarms nothing", async () => } finally { console.log = log; } - assert.deepEqual(writes(), []); + assert.deepEqual(writes(), [{ mutation: "disarm", id: "PR_2" }]); +}); + +// An event run reads no room, so it never disarms on one. +test("an event run leaves an armed lane pull request armed whatever the room", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const { writes } = await run([pull(2, { autoMergeRequest: armed })], { EVENT_NAME: "pull_request_target" }, {}, null); + assert.deepEqual(writes, []); }); test("an event run reads no room", async () => { diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 122d4229ff..a7ce9988be 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1477,7 +1477,8 @@ jobs: # room from (lane-room), so a demotion or a failed lane merge stops # native arming as it stops the gate. Downloaded from this run only (no # run-id is given, so no other run's artifact is read). With no report - # the room is unreadable: nothing is armed and nothing is disarmed. + # the room is unreadable, which holds as the gate's refusal does: + # nothing is armed and every armed lane pull request is disarmed. - name: Read this run's post-merge report if: >- steps.lane-token.outputs.token != '' diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 5cd68715c3..416404cb79 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1072,11 +1072,12 @@ reads from the review lanes' evidence bound to the head (an open CRITICAL securi review not bound to the head), whether or not the lane applied its hold label. Each such run also waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it -arms nothing and disarms nothing): while the gate holds its lane off (a demotion record, +holds as a refusal does): while the gate holds its lane off (a demotion record, `merge.rung`, the loop setting or the rung cap off, a lane merge that failed its post-merge check) nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue -has not taken, nothing more is armed; a room that cannot be read arms nothing and disarms nothing. +has not taken, nothing more is armed; a room that cannot be read (no report, a failed read) holds +as a refusal does. Only the scheduled run and a hand-started one read them and arm, last, for a pull request every other rule would arm or keep armed, with the gate's saved listing, head-ref map and run cache; an event run reads no link or verdict, never arms, and 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 b799bd93b3..4ee511f98e 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 @@ -4448,7 +4448,7 @@ def assess( class LaneRoom: """`lane_room`'s answer. `refusal`: the gate holds the lane off, so nothing is armed and armed lane pull requests are disarmed. `unreadable`: - a read failed, so nothing is armed and nothing is disarmed. Otherwise + a read failed, which the caller holds on as on a refusal. Otherwise `count` merges are in flight of `cap`, and `queued` holds the merge queue's pull request numbers.""" From 62c6679c24ebf240e9601ed98f658c29a754c7d4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:11:31 -0400 Subject: [PATCH 10/13] fix(pr-pipeline): hold the lane's room on a post-merge report the gate would not reuse lane_room read the demotion and post-merge refusal straight from the run's post-merge report. A re-run of enable-auto-merge downloads the run's first report, up to a day old, so a demotion filed since went unseen and lane pull requests armed. lane_room now applies the gate's own reuse checks (shared_refusal's age rules, now one helper, and _demotion_changed) and reads an answer that fails them as unreadable, which holds. The report is read whenever post-merge-dispatch ran, not only when it succeeded: its full-run steps can fail after the report is kept, and the gate does not refuse on them. The merge queue's relink hold no longer says pr-verify judges the new link: it skips a head it already passed, so a new head or an approval recovers it. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 2 +- .../feat-native-lane-refs-arm-7d1f3b96.md | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 4 +- .github/scripts/pr-merge/workflow.test.mjs | 3 +- .github/workflows/pr-merge.yml | 5 +- .../scripts/babysit_merge_policy.py | 54 +++++++++++++------ .../scripts/tests/test_babysit_lane_room.py | 34 +++++++++++- 7 files changed, 80 insertions(+), 24 deletions(-) diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index 9afcda9b99..5f672344f0 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -5,6 +5,6 @@ bump: minor ### Changed - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. -- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, or a room that cannot be read, holds as a refusal does: nothing is armed and every armed lanes App pull request is disarmed. The job now holds `pull-requests: read` for the merge-queue and merge reads. +- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, one the gate would not reuse (older than 30 minutes, as a re-run job reads the run's first report, or with a demotion record filed or changed since), or a room that cannot be read, holds as a refusal does: nothing is armed and every armed lanes App pull request is disarmed. The report is read whenever `post-merge-dispatch` ran, so a later step of that job failing does not disarm anything. The job now holds `pull-requests: read` for the merge-queue and merge reads. - In the merge queue, `ci-status` holds a lanes App pull request whose verifier pass no longer binds its live issue link. It reads the counted pass's `verdict.json` (one artifact download) and the closing issues (one GraphQL read), and `merge-holds.mjs` `linkHold` holds unless the body still closes, or with no closing issue still names by a plain `Refs:` line, the one issue the pass judged, linked the same way. This is pr-merge's reader rule (`verify:spec-changed`), so a body edited after arming, such as `Refs: #A` to `Closes #B`, no longer merges on a pass for the old issue. A trusted approval of the head lifts it, as it lifts the verifier hold. `refsIssue` moves to `merge-holds.mjs`, which `resolve-pr.mjs` re-exports. - `enable-auto-merge` holds a lanes App pull request on its review lanes as the merge gate does: `evidence --lane-reviews` binds both lanes' evidence to the head (`babysit_merge._lane_evidence`) and applies the gate's lane rule (`babysit_risk`). An open CRITICAL security-lane finding, or a review not bound to the head, leaves it unarmed or disarms it, whether or not the lane applied its hold label. The job now also holds `checks: read` for the lanes' annotations. diff --git a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md index 08f0423cdd..1c668b7c76 100644 --- a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md +++ b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md @@ -5,5 +5,5 @@ bump: minor ### Added - `babysit_verify_evidence.py evidence --lane-link` first reads the pull request's issue link by the pr-merge lane's own rule (`babysit_merge_policy._linked_issue`: one closing issue in this repository, or, with none, the one issue its plain `Refs:` lines name) and adds `ineligible` to its line: null when the lane takes the link, else why not, with refusal `verify:ineligible` and no verdict or config read. A failed read refuses. The verdict read that follows takes the link read's pull request instead of reading it again. -- `babysit_verify_evidence.py lane-room` prints whether the pr-merge lane may merge one more of its own pull requests now (`babysit_merge_policy.lane_room`): the gate's run-level refusals (another authority, a post-merge refusal, the effective rung off, a demotion included) as `refusal`, a failed read as `unreadable`, and otherwise the merges in flight against `merge.max-in-flight`. `InFlight` now carries that count. +- `babysit_verify_evidence.py lane-room` prints whether the pr-merge lane may merge one more of its own pull requests now (`babysit_merge_policy.lane_room`): the gate's run-level refusals (another authority, a post-merge refusal, the effective rung off, a demotion included) as `refusal`, a failed read, or a post-merge answer the gate would not reuse for its age or a changed demotion record (`shared_refusal`'s checks), as `unreadable`, and otherwise the merges in flight against `merge.max-in-flight`. `InFlight` now carries that count. - `babysit_verify_evidence.py evidence --lane-reviews` adds `reviews` to a line whose refusal is null: null when the gate would not hold the pull request on its review lanes' evidence (`lane_review_refusal`: the holding reasons of `babysit_risk`'s lane rule, over `babysit_merge._lane_evidence`), else why. A failed read or a moved head holds. diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 9bbce1c23e..cdd147e8d0 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -462,7 +462,9 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi if (!unverified && !relinked) { console.log(`#${number}: no verifier hold: pr-verify passed this head.`); } else if (!unverified && lifters.length === 0) { - reasons.push(`${relinked}; pr-verify judges its new link, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); + // pr-verify skips a head it already passed (resolve-pr.mjs `judged`), + // so only a new head or an approval recovers it. + reasons.push(`${relinked}; pr-verify does not judge this head again, so push a new head for it to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); } else if (lifters.length) { console.log(`#${number}: no verifier hold: ${lifters.join(", ")} approved this head, which lifts '${unverified}'.`); } else { diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index aac104a035..7a8ee9667d 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -208,7 +208,8 @@ test("enable-auto-merge reads the lane's room from this run's post-merge-dispatc assert.ok(read, "a download of the report"); assert.match(read, /uses: actions\/download-artifact@[0-9a-f]{40} /); assert.match(read, /\n {10}name: pr-merge-post-merge-report\n/); - assert.match(read, /needs\.post-merge-dispatch\.result == 'success'/); + // Not 'success': a step after the kept report can fail the job. + assert.match(read, /needs\.post-merge-dispatch\.result != 'skipped'/); assert.doesNotMatch(read, /run-id|github-token|repository:/); const step = /- name: Arm and disarm auto-merge\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)[1]; assert.match(step, /\n {10}POSTMERGE_RESULT: \$\{\{ runner\.temp \}\}\/post-merge\/post-merge\.json\n/); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index a7ce9988be..4615b4897e 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1479,11 +1479,14 @@ jobs: # run-id is given, so no other run's artifact is read). With no report # the room is unreadable, which holds as the gate's refusal does: # nothing is armed and every armed lane pull request is disarmed. + # Read whenever post-merge-dispatch ran: its later steps (a full run + # started or re-run) can fail after the report was kept, and the gate, + # which runs its own check, does not refuse on them. - name: Read this run's post-merge report if: >- steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') - && needs.post-merge-dispatch.result == 'success' + && needs.post-merge-dispatch.result != 'skipped' continue-on-error: true timeout-minutes: 2 uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 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 4ee511f98e..d82c616df3 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 @@ -3932,6 +3932,26 @@ def _demotion_changed( return False +def _shared_age_refusal(shared: SharedPostMerge, age: timedelta) -> str | None: + """`stale` past `SHARED_MAX_AGE` (or from the future), `unverified-stale` + past `SHARED_UNVERIFIED_MAX_AGE` while it passes with a merge unverified, + else None.""" + if not timedelta(0) <= age <= SHARED_MAX_AGE: + return "stale" + # A refusing answer refuses whatever was verified since, so only a + # passing one with a merge still unverified is read again. + if ( + age > SHARED_UNVERIFIED_MAX_AGE + and shared.gate.refusal is None + and any( + json_object(m).get("verified") is not True + for m in json_array(shared.gate.report.get("autoMerges")) + ) + ): + return "unverified-stale" + return None + + def shared_refusal( repo: str, shared: SharedPostMerge, @@ -3988,21 +4008,7 @@ def shared_refusal( return "floor-differs" except PolicyReadError: return "config-unreadable" - checks = ( - (not timedelta(0) <= age <= SHARED_MAX_AGE, "stale"), - # A refusing answer refuses whatever was verified since, so only a - # passing one with a merge still unverified is read again. - ( - age > SHARED_UNVERIFIED_MAX_AGE - and shared.gate.refusal is None - and any( - json_object(m).get("verified") is not True - for m in json_array(shared.gate.report.get("autoMerges")) - ), - "unverified-stale", - ), - ) - refusal = next((text for refused, text in checks if refused), None) + refusal = _shared_age_refusal(shared, age) if refusal: return refusal if not shared.demotion.active and _demotion_changed(owner, name, shared, identity): @@ -4469,7 +4475,11 @@ def report(self) -> dict[str, Any]: def lane_room( - repo: str, shared: SharedPostMerge | None, *, lane_rung_cap: str | None + repo: str, + shared: SharedPostMerge | None, + *, + lane_rung_cap: str | None, + now: datetime | None = None, ) -> LaneRoom: """Whether the pr-merge lane may merge one more of its own pull requests now, for native auto-merge, which skips `assess`: the run-level refusals @@ -4488,9 +4498,16 @@ def lane_room( are counted, not refused: the caller arms below the cap. Any read failure, a missing `shared` and an in-flight count that could - not be read included, is `unreadable`.""" + not be read included, is `unreadable`. So is a `shared` the gate would + not reuse for its age (`_shared_age_refusal`) or for a demotion record + filed or changed since (`_demotion_changed`): a re-run job downloads the + run's first report, up to a day old, and reads its demotion and + refusal.""" if shared is None: return LaneRoom(unreadable="post-merge answer missing or unreadable") + stale = _shared_age_refusal(shared, (now or datetime.now(UTC)) - shared.computed_at) + if stale: + return LaneRoom(unreadable=f"post-merge answer {stale}") try: default = _default_branch(repo, shared=True) config = _default_branch_config(repo, default) @@ -4501,6 +4518,9 @@ def lane_room( identity = config.agent_identity if not identity or _login_key(shared.identity) != _login_key(identity): return LaneRoom(unreadable="post-merge answer is for another identity") + owner, name = _split_repo(repo) + if not shared.demotion.active and _demotion_changed(owner, name, shared, identity): + return LaneRoom(unreadable="post-merge answer demotion-changed") if shared.gate.refusal: return LaneRoom(refusal=f"post-merge:{shared.gate.refusal}") cap = rung_cap(repo, authority=PR_MERGE, lane_value=lane_rung_cap) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py index 1a7590d468..c32b220d10 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -16,7 +16,7 @@ import pathlib import sys import unittest -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta from unittest import mock sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) @@ -28,6 +28,8 @@ IDENTITY = "agent-lanes[bot]" MAIN = "c" * 40 FLOOR = datetime(2026, 10, 1, tzinfo=UTC) +# Five minutes after the answer was computed: inside the gate's reuse age. +NOW = FLOOR + timedelta(minutes=5) def config(**overrides: object) -> policy.PipelineConfig: @@ -71,6 +73,8 @@ def room( cap: str | None = "", flight: policy.InFlight | None = None, config_error: Exception | None = None, + now: datetime = NOW, + demotion_changed: bool = False, ) -> policy.LaneRoom: seen: dict[str, object] = {} @@ -90,9 +94,15 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: policy, "_run_max_in_flight", return_value=(5, "default-branch") ), mock.patch.object(policy, "in_flight", side_effect=in_flight), + mock.patch.object( + policy, "_demotion_changed", return_value=demotion_changed + ), ): result = policy.lane_room( - REPO, answer if answer is not None else shared(), lane_rung_cap=cap + REPO, + answer if answer is not None else shared(), + lane_rung_cap=cap, + now=now, ) self.seen = seen return result @@ -145,6 +155,26 @@ def test_at_the_cap_it_reports_the_cap_and_refuses_nothing(self) -> None: (room.refusal, room.unreadable, room.count, room.cap), (None, None, 5, 5) ) + def test_an_answer_the_gate_would_not_reuse_is_unreadable(self) -> None: + # The gate reads afresh past SHARED_MAX_AGE or after a demotion record + # changed (shared_refusal); a re-run job reads the run's first report. + cases = { + "stale": dict(now=FLOOR + policy.SHARED_MAX_AGE + timedelta(seconds=1)), + "future": dict(now=FLOOR - timedelta(seconds=1)), + "demotion changed": dict(demotion_changed=True), + } + for name, kwargs in cases.items(): + with self.subTest(name): + room = self.room(**kwargs) # type: ignore[arg-type] + self.assertIsNone(room.refusal) + self.assertTrue(room.unreadable) + self.assertFalse(self.seen, "no in-flight read after an unusable answer") + + def test_an_active_demotion_is_not_re_read(self) -> None: + # Already refusing: a changed record cannot make it allow. + room = self.room(shared(demoted=True), demotion_changed=True) + self.assertEqual(room.refusal, "rung-off:demotion") + def test_a_failed_read_refuses_nothing_and_allows_nothing(self) -> None: cases = { "no answer": dict(answer=None), From 10c09405832432fa07043eb0bb7750414eb5bda5 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:32:45 -0400 Subject: [PATCH 11/13] fix(pr-pipeline): hold native lane merges on the gate's risk score and the kill switch The native path read only the review lanes' part of the gate's risk score, so a lane pull request adding a symlink or a submodule pointer, or touching a high path, a manifest, the trust gate or a cost path, armed where the gate holds it. evidence --lane-reviews now runs babysit_risk.score and holding over the inputs the gate builds, at the lane room's effective rung, and fails closed on any read error. ci-status's lanes App step now fails while the lanes kill switch is on, so an armed lane pull request stops in the merge queue; other authors are unaffected. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 3 +- .../feat-native-lane-refs-arm-7d1f3b96.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 31 +++-- .../pr-merge/enable-auto-merge.test.mjs | 78 +++++++++-- .github/workflows/pr-merge.yml | 4 +- .github/workflows/pr-require-checks.yml | 13 ++ docs/conventions/pr-pipeline.yaml | 2 +- docs/conventions/pr-pipeline/README.md | 11 +- .../scripts/babysit_merge_policy.py | 8 +- .../scripts/babysit_verify_evidence.py | 107 +++++++++++---- .../tests/test_babysit_lane_reviews.py | 128 +++++++++++++----- .../scripts/tests/test_babysit_lane_room.py | 14 +- scripts/ci-lanes-pr-hold.test.sh | 23 +++- 13 files changed, 329 insertions(+), 95 deletions(-) diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index 5f672344f0..ecb0a36fd4 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -7,4 +7,5 @@ bump: minor - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. - The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, one the gate would not reuse (older than 30 minutes, as a re-run job reads the run's first report, or with a demotion record filed or changed since), or a room that cannot be read, holds as a refusal does: nothing is armed and every armed lanes App pull request is disarmed. The report is read whenever `post-merge-dispatch` ran, so a later step of that job failing does not disarm anything. The job now holds `pull-requests: read` for the merge-queue and merge reads. - In the merge queue, `ci-status` holds a lanes App pull request whose verifier pass no longer binds its live issue link. It reads the counted pass's `verdict.json` (one artifact download) and the closing issues (one GraphQL read), and `merge-holds.mjs` `linkHold` holds unless the body still closes, or with no closing issue still names by a plain `Refs:` line, the one issue the pass judged, linked the same way. This is pr-merge's reader rule (`verify:spec-changed`), so a body edited after arming, such as `Refs: #A` to `Closes #B`, no longer merges on a pass for the old issue. A trusted approval of the head lifts it, as it lifts the verifier hold. `refsIssue` moves to `merge-holds.mjs`, which `resolve-pr.mjs` re-exports. -- `enable-auto-merge` holds a lanes App pull request on its review lanes as the merge gate does: `evidence --lane-reviews` binds both lanes' evidence to the head (`babysit_merge._lane_evidence`) and applies the gate's lane rule (`babysit_risk`). An open CRITICAL security-lane finding, or a review not bound to the head, leaves it unarmed or disarms it, whether or not the lane applied its hold label. The job now also holds `checks: read` for the lanes' annotations. +- `enable-auto-merge` holds a lanes App pull request on the merge gate's risk score: `evidence --lane-reviews` runs `babysit_risk.score` and `holding` over the inputs the gate builds, at the lane room's effective rung. Each of these leaves it unarmed or disarms it: an open CRITICAL security-lane finding or a review not bound to the head (whether or not the lane applied its hold label), a symlink, submodule or other non-regular mode, a high path, the manifest review, the trust gate, and the cost review unless the owner's decision lifts it. The job now also holds `checks: read` for the lanes' annotations. +- `ci-status` fails a lanes App pull request while the lanes kill switch (`CLAUDE_LANES_DISABLED` or `CLAUDE_LANES_REPO_DISABLED`) reads anything but `false`, so one armed before the switch was turned on stops in the merge queue. It reads the vars context and makes no API call. Owner and Dependabot pull requests are not affected. diff --git a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md index 1c668b7c76..f3735a1ea9 100644 --- a/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md +++ b/.changes/source-control/feat-native-lane-refs-arm-7d1f3b96.md @@ -6,4 +6,4 @@ bump: minor - `babysit_verify_evidence.py evidence --lane-link` first reads the pull request's issue link by the pr-merge lane's own rule (`babysit_merge_policy._linked_issue`: one closing issue in this repository, or, with none, the one issue its plain `Refs:` lines name) and adds `ineligible` to its line: null when the lane takes the link, else why not, with refusal `verify:ineligible` and no verdict or config read. A failed read refuses. The verdict read that follows takes the link read's pull request instead of reading it again. - `babysit_verify_evidence.py lane-room` prints whether the pr-merge lane may merge one more of its own pull requests now (`babysit_merge_policy.lane_room`): the gate's run-level refusals (another authority, a post-merge refusal, the effective rung off, a demotion included) as `refusal`, a failed read, or a post-merge answer the gate would not reuse for its age or a changed demotion record (`shared_refusal`'s checks), as `unreadable`, and otherwise the merges in flight against `merge.max-in-flight`. `InFlight` now carries that count. -- `babysit_verify_evidence.py evidence --lane-reviews` adds `reviews` to a line whose refusal is null: null when the gate would not hold the pull request on its review lanes' evidence (`lane_review_refusal`: the holding reasons of `babysit_risk`'s lane rule, over `babysit_merge._lane_evidence`), else why. A failed read or a moved head holds. +- `babysit_verify_evidence.py evidence --lane-reviews --base-sha --rung ` adds `reviews` to a line whose refusal is null: null when the gate would not hold the pull request on its risk score (`lane_risk_refusal`: `babysit_risk.score` and `holding` over the inputs `babysit_merge.score_unattended` builds for a lane pull request, the cost decision included), else why. A failed read, a moved head, or no rung holds. `lane-room` reports the effective rung as `rung`. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index bd2360658e..1faa628db6 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -54,12 +54,14 @@ // arms; at the cap it arms nothing more. A room that cannot be read (no // post-merge report, a failed read) holds as a refusal does: nothing is // armed and every armed lanes App pull request is disarmed. -// - pr-merge's gate would not hold it on its review lanes: `evidence -// --lane-reviews` binds both lanes' evidence to the head as the gate does -// (babysit_merge._lane_evidence) and applies the gate's lane rule -// (babysit_risk._lane_reasons, holding), so an open CRITICAL security -// finding, or a review not bound to the head, holds whether or not the -// lane applied its hold label. Read only after the verdict allows. +// - pr-merge's gate would not hold it on its risk score: `evidence +// --lane-reviews --rung ` runs the gate's scorer +// (babysit_risk.score, holding) over the inputs the gate builds, the +// review lanes' evidence bound to the head included. So an open CRITICAL +// security finding or a review not bound to the head (whether or not the +// lane applied its hold label), a symlink, submodule or other non-regular +// mode, a high path, the manifest review, the trust gate and the cost +// review hold. Read only after the verdict allows. // Arming passes the head just read as expectedHeadOid, so a push in between // fails the arm rather than arming a head nobody read. An armed pull request // that fails any rule is disarmed. While merge.report-only is true it reports @@ -232,20 +234,24 @@ export function verifyHold(evidence, head) { return "its issue link could not be read"; } if (typeof evidence.reviews === "string") { - return `pr-merge's gate holds it on its review lanes (${escapeCommand(evidence.reviews)})`; + return `pr-merge's gate holds it on its risk score (${escapeCommand(evidence.reviews)})`; } - return evidence.reviews === null ? null : "its review lanes could not be read"; + return evidence.reviews === null ? null : "its risk score could not be read"; } // The gate's own reader, one process per pull request; the run shares its // artifact listing, head-ref map and run cache through the BABYSIT_* files. export function pythonVerify(env) { - const verify = async ({ number, headRefOid: head, baseRefOid: base }) => { + // `rung`: the lane room's effective rung, which the gate scores at. + const verify = async ({ number, headRefOid: head, baseRefOid: base }, rung) => { if (!SHA.test(String(base))) { throw new Error("its base commit is not a commit SHA"); } // --base-sha: the gate's FAIL cap, the lower of the base's and main's. const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "evidence", "--lane-link", "--lane-reviews", "--repo", env.GITHUB_REPOSITORY, "--base-sha", base]; + if (typeof rung === "string") { + args.push("--rung", rung); + } const done = spawnSync("python3", [...args, String(number), head], { encoding: "utf8", env, timeout: VERIFY_TIMEOUT_MS }); if (done.status !== 0) { throw new Error(`babysit_verify_evidence.py exited ${done.status ?? done.signal}: ${String(done.stderr ?? "").trim().split("\n").at(-1)}`); @@ -288,9 +294,10 @@ export function readRoom(answer) { !whole(answer.count, 0) || !whole(answer.cap, 1) || !Array.isArray(answer.queued) || - !answer.queued.every((number) => whole(number, 1)) + !answer.queued.every((number) => whole(number, 1)) || + typeof answer.rung !== "string" ) { - return { refusal: null, unreadable: "its answer is not lane-room's", count: 0, cap: 1, queued: [] }; + return { refusal: null, unreadable: "its answer is not lane-room's", count: 0, cap: 1, queued: [], rung: "off" }; } return answer; } @@ -460,7 +467,7 @@ export async function main({ env = process.env, github, verify, room } = {}) { } else if (action === "verify") { let evidence = null; try { - evidence = await verify(pull); + evidence = await verify(pull, lane?.rung); } catch (error) { console.log(`enable-auto-merge: #${pull.number} verifier verdict did not read: ${escapeCommand(error?.message ?? error)}`); } diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index d27069f046..bac7dbf3a8 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -87,7 +87,7 @@ const COST_COMMENT = { created_at: "2026-10-22T12:00:00Z", }; // lane-room's answer for a lane with room: nothing in flight of five. -const ROOM = { refusal: null, unreadable: null, count: 0, cap: 5, queued: [] }; +const ROOM = { refusal: null, unreadable: null, count: 0, cap: 5, queued: [], rung: "C2" }; const room = (answer = ROOM) => async () => answer; const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name) => ({ name })) }); @@ -525,8 +525,8 @@ test("verifyHold allows only the reader's null refusal at the head read", () => assert.match(verifyHold({ refusal: null, head: HEAD }, HEAD), /issue link could not be read/); assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: false }, HEAD), /issue link could not be read/); // The review lanes' answer: a hold names it, and a line without one allows nothing. - assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: null, reviews: "security-finding: 1 open" }, HEAD), /holds it on its review lanes \(security-finding: 1 open\)/); - assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: null }, HEAD), /review lanes could not be read/); + assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: null, reviews: "security-finding: 1 open" }, HEAD), /holds it on its risk score \(security-finding: 1 open\)/); + assert.match(verifyHold({ refusal: null, head: HEAD, ineligible: null }, HEAD), /risk score could not be read/); }); // The merge queue's check (merge-holds.mjs verdictHold): a pass counts only @@ -612,7 +612,7 @@ test("a reader that fails, prints no JSON or reports another head arms nothing a garbage: "print('not json')\n", other: `import json\nprint(json.dumps({"refusal": None, "head": "${"c".repeat(40)}"}))\n`, unlinked: `import json, sys\nprint(json.dumps({"refusal": None, "head": sys.argv[-1]}))\n`, - passes: `import json, sys\nassert sys.argv[2:4] == ["--lane-link", "--lane-reviews"] and sys.argv[-4:-2] == ["--base-sha", "${"d".repeat(40)}"], sys.argv\nprint(json.dumps({"refusal": None, "head": sys.argv[-1], "ineligible": None, "reviews": None}))\n`, + passes: `import json, sys\nassert sys.argv[2:4] == ["--lane-link", "--lane-reviews"] and sys.argv[-6:-2] == ["--base-sha", "${"d".repeat(40)}", "--rung", "C2"], sys.argv\nprint(json.dumps({"refusal": None, "head": sys.argv[-1], "ineligible": None, "reviews": None}))\n`, }; for (const [name, body] of Object.entries(scripts)) { writeFileSync(join(dir, `${name}.py`), body); @@ -674,10 +674,8 @@ def passes(self, number, head, pr=None): return ve.Evidence(None, head, (ve.Verdict(head, "pass", 1),), 0) argv = sys.argv[1:] -cut = argv.index("--base-sha") -del argv[cut : cut + 2] pr = lambda repo, number: {"number": number, "head": {"sha": argv[-1]}} -with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "evidence", passes), mock.patch.object(policy, "pull_request", pr), mock.patch.object(ve, "lane_review_refusal", lambda repo, number, pr: None): +with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, "gh_capture", gh), mock.patch.object(ve.Reader, "evidence", passes), mock.patch.object(policy, "pull_request", pr), mock.patch.object(ve, "lane_risk_refusal", lambda *args: None), mock.patch.object(policy, "read_config_at", lambda repo, sha: policy.PipelineConfig(sha, "C2", "pr-merge", "agent[bot]", None, (), "c2-mechanical")), mock.patch.object(policy, "with_default_branch", lambda repo, base, *rest: base): sys.exit(ve.main(argv)) `, ); @@ -855,6 +853,68 @@ test("a review-lane hold the gate reads arms nothing and disarms an armed pull r { mutation: "disarm", id: "PR_2" }, { mutation: "arm", id: "PR_4", head: HEAD }, ]); - assert.ok(lines.some((line) => line.includes("#1 left: pr-merge's gate holds it on its review lanes (security-finding: 1 open)"))); - assert.ok(lines.some((line) => line.includes("#3 left: its review lanes could not be read"))); + assert.ok(lines.some((line) => line.includes("#1 left: pr-merge's gate holds it on its risk score (security-finding: 1 open)"))); + assert.ok(lines.some((line) => line.includes("#3 left: its risk score could not be read"))); +}); + +// The gate's whole risk score through its own CLI and scorer +// (babysit_risk.score, holding), the reads stubbed: only a regular file that +// stays regular may rate below HIGH, so a symlink or a submodule pointer a +// lane pull request adds holds, and a plain documentation change arms. +test("a lane pull request adding a symlink or a submodule pointer is not armed, or is disarmed; a doc change arms", async () => { + const dir = mkdtempSync(join(tmpdir(), "enable-auto-merge-risk-")); + const wrapper = join(dir, "risk.py"); + writeFileSync( + wrapper, + `import json, os, sys +sys.path.insert(0, os.environ["SCRIPTS"]) +from unittest import mock +import babysit_merge +import babysit_merge_policy as policy +import babysit_verify_evidence as ve +from babysit_risk import ChangedFile, LaneEvidence + +KINDS = json.loads(os.environ["KINDS"]) +DOC = ChangedFile("docs/guide.md", None, "modified", 1, 1, True, "100644", "100644", patch="@@ -1 +1 @@\\n-old\\n+new") +EXTRA = { + "doc": (), + "symlink": (ChangedFile("docs/link", None, "added", 1, 0, True, "120000", "000000"),), + "submodule": (ChangedFile("vendor/lib", None, "modified", 1, 1, False, "160000", "160000"),), +} +number = int(sys.argv[-2]) +pr = {"number": number, "head": {"sha": sys.argv[-1]}, "base": {"sha": "d" * 40, "repo": {"private": False}}} +lanes = (LaneEvidence("claude-review", True, None, 0), LaneEvidence("claude-security-review", True, None, 0, False, 0)) +config = policy.PipelineConfig("c" * 40, "C2", "pr-merge", "agent[bot]", None, (), "c2-mechanical") +with mock.patch.object(ve.Reader, "link_refusal", lambda self, n: None), \\ + mock.patch.object(ve.Reader, "evidence", lambda self, n, h, pr=None: ve.Evidence(None, h, (), 0)), \\ + mock.patch.object(policy, "read_config_at", lambda repo, sha: config), \\ + mock.patch.object(policy, "with_default_branch", lambda repo, base, *rest: base), \\ + mock.patch.object(policy, "pull_request", lambda repo, n: pr), \\ + mock.patch.object(policy, "changed_files", lambda repo, p: ((DOC, *EXTRA[KINDS[str(number)]]), True)), \\ + mock.patch.object(policy, "with_contents", lambda repo, p, f: f), \\ + mock.patch.object(policy, "pulumi_dirs_at", lambda repo, p: ()), \\ + mock.patch.object(policy, "permission_reader", lambda repo: (lambda login: None)), \\ + mock.patch.object(babysit_merge, "_lane_evidence", lambda *args: (lanes, [])): + sys.exit(ve.main(sys.argv[1:])) +`, + ); + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const kinds = { 1: "doc", 2: "symlink", 3: "submodule" }; + const { github, writes } = world([pull(1), pull(2), pull(3, { autoMergeRequest: armed })]); + const scripts = fileURLToPath(new URL("../../../plugins/source-control/skills/babysit-prs/scripts", import.meta.url)); + const env = { ...ENV, PATH: process.env.PATH, SCRIPTS: scripts, KINDS: JSON.stringify(kinds), VERIFY_EVIDENCE_SCRIPT: wrapper }; + const lines = []; + const log = console.log; + console.log = (line) => lines.push(line); + try { + assert.equal(await main({ env, github, room: room() }), 0); + } finally { + console.log = log; + } + assert.deepEqual(writes(), [ + { mutation: "arm", id: "PR_1", head: HEAD }, + { mutation: "disarm", id: "PR_3" }, + ]); + assert.ok(lines.some((line) => /#2 left: pr-merge's gate holds it on its risk score \(.*docs\/link/.test(line)), lines.join("\n")); + assert.ok(lines.some((line) => /#3 disarmed: pr-merge's gate holds it on its risk score \(.*vendor\/lib/.test(line)), lines.join("\n")); }); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 4615b4897e..de47ac7395 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1334,8 +1334,8 @@ jobs: # catches what the events missed, such as a run the lanes App started, which # the broker refuses. Only the scheduled run and a hand-started one arm, on # the issue link (one closing issue, or one plain `Refs:` issue) and verdict - # pr-merge's gate would take at the head, with no review-lane hold the gate - # reads (babysit_verify_evidence.py `evidence --lane-link --lane-reviews`, + # pr-merge's gate would take at the head, with no hold on the gate's risk + # score (babysit_verify_evidence.py `evidence --lane-link --lane-reviews`, # its Actions reads with the job's GITHUB_TOKEN from the head-ref map and # run cache the last gate run kept), and only while the gate's lane has # room (`lane-room`, from this run's post-merge-dispatch report): while the diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index c2a2351421..4cc446bcd6 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -4100,6 +4100,11 @@ jobs: # it); every other pull request merges through pr-merge's gate or a # person. An unreadable pull request, module, config, file list or # discussion fails closed. + # WHILE THE LANES KILL SWITCH IS ON A LANES APP PULL REQUEST FAILS, so + # one armed before it was turned on stops in the merge queue, as the + # gate stops: CLAUDE_LANES_DISABLED and CLAUDE_LANES_REPO_DISABLED from + # the vars context, no API read, and only the exact value `false` on + # both proceeds. Every other author's pull request is not affected. # IN THE MERGE QUEUE A LANES APP PULL REQUEST ALSO NEEDS A VERIFIER PASS # AT ITS HEAD: a PASS or PASS-NOTES artifact of a completed, successful # pr-verify.yml run on main (merge-holds.mjs verdictHold), looked up by @@ -4128,6 +4133,10 @@ jobs: PR_NUMBER: ${{ github.event.pull_request.number }} EVENT_ACTION: ${{ github.event.action }} GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} + # The lanes kill switches, as check-kill-switch reads them: only the + # exact value `false` lets a lanes App pull request merge. + LANES_DISABLED: ${{ vars.CLAUDE_LANES_DISABLED }} + LANES_REPO_DISABLED: ${{ vars.CLAUDE_LANES_REPO_DISABLED }} run: | . "$RUNNER_TEMP/gh-read.sh" lanes_app_id=337595936 @@ -4163,6 +4172,10 @@ jobs: echo "#$pr is not the lanes App's; pr-merge's gate or a person reads its holds." exit 0 fi + if [ "${LANES_DISABLED:-}" != false ] || [ "${LANES_REPO_DISABLED:-}" != false ]; then + echo "::error::#$pr: the lanes kill switch is on (CLAUDE_LANES_DISABLED or CLAUDE_LANES_REPO_DISABLED is not 'false'), so no lanes App pull request merges; it merges once both read 'false' and ci-status is re-run." + exit 1 + fi BASE_SHA=$GROUP_BASE_SHA if [ "$GITHUB_EVENT_NAME" != merge_group ]; then if [ "$(jq -r '.base.ref' <<<"$pull")" != main ]; then diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index df96b6f363..df6a67ec06 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -394,7 +394,7 @@ activities: # gate takes (one closing issue, or one plain `Refs: #` issue) and # that carries no hold (a `Hold:` line, do-not-merge, the human-gated # label, a member's "do not merge" comment, a head that changes a denied - # path, a review-lane hold the gate reads), while the gate's lane has + # path, a hold on the gate's risk score), while the gate's lane has # room (no demotion, rung on, below merge.max-in-flight), and turns it # off on an armed one that now carries a hold or while the gate holds the # lane off; auto-merge a person turned off stays off. No diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index a295cfea1b..18ef5349c6 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1075,9 +1075,11 @@ issue link and verifier verdict at that head pr-merge's gate would take: [eligibility](#the-pr-merge-lane) rule takes (one closing issue in this repository, or, with none, the one issue its plain `Refs: #` lines name; a failed read refuses), a PASS or PASS-NOTES of [pr-verify](#verifier-verdict) stands at the head, no FAIL is at it, no earlier FAIL is uncleared, FAILs sit at fewer than `merge.max-verify-fails` -heads, and the spec is unchanged and trusted; and `evidence --lane-reviews` finds no hold the gate -reads from the review lanes' evidence bound to the head (an open CRITICAL security finding, or a -review not bound to the head), whether or not the lane applied its hold label. Each such run also +heads, and the spec is unchanged and trusted; and `evidence --lane-reviews` finds no hold on the +gate's risk score (`babysit_risk.score` over the inputs the gate builds, at the lane room's rung): +an open CRITICAL security finding or a review not bound to the head, whether or not the lane +applied its hold label, a symlink, submodule or other non-regular mode, a high path, the manifest +review, the trust gate and the cost review all hold. Each such run also waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it holds as a refusal does): while the gate holds its lane off (a demotion record, @@ -1111,6 +1113,9 @@ and the post-merge check, the full-run dispatch, re-firing refused runs and clea run unchanged. `gate` lists the lanes App's pull requests for the gate as well, which is how the lane ran before this setting; the `enable-auto-merge` job arms them in both modes. +While the lanes kill switch is on (`CLAUDE_LANES_DISABLED` or `CLAUDE_LANES_REPO_DISABLED` reads +anything but `false`), `ci-status` fails every lanes App pull request, so one armed before the switch +was turned on stops in the merge queue; owner and Dependabot pull requests are not affected. `ci-status` enforces the same holds on every lanes App pull request, the two text holds and the hold paths, so one added after arming stops the merge without waiting for this job: its step `Fail a lanes App pull request its body or a member holds` runs 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 d82c616df3..2e663bfcaf 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 @@ -4455,14 +4455,16 @@ class LaneRoom: """`lane_room`'s answer. `refusal`: the gate holds the lane off, so nothing is armed and armed lane pull requests are disarmed. `unreadable`: a read failed, which the caller holds on as on a refusal. Otherwise - `count` merges are in flight of `cap`, and `queued` holds the merge - queue's pull request numbers.""" + `count` merges are in flight of `cap`, `queued` holds the merge queue's + pull request numbers, and `rung` is the effective rung the gate scores + each candidate at ("off" unless the lane has room).""" refusal: str | None = None unreadable: str | None = None count: int = 0 cap: int = 1 queued: frozenset[int] = frozenset() + rung: str = "off" def report(self) -> dict[str, Any]: return { @@ -4471,6 +4473,7 @@ def report(self) -> dict[str, Any]: "count": self.count, "cap": self.cap, "queued": sorted(self.queued), + "rung": self.rung, } @@ -4550,6 +4553,7 @@ def lane_room( count=flight.count if flight.clear else flight_cap, cap=flight_cap, queued=flight.queued, + rung=rung.rung, ) 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 6a73f5ad24..1e9e89626b 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 @@ -109,11 +109,10 @@ import tempfile import time import zipfile -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from datetime import UTC, datetime, timedelta from pathlib import Path -from types import SimpleNamespace -from typing import Any, cast +from typing import Any import babysit_merge_policy as merge_policy import babysit_postmerge as postmerge @@ -512,6 +511,8 @@ class Reader: # The pull requests `link_refusal` read, each taken by the next # `_closing_issues` instead of reading it again. _linked: dict[int, dict[str, Any]] = field(default_factory=dict) + # The issue number `link_refusal` took for each pull request. + linked_issue: dict[int, Any] = field(default_factory=dict) def _listing(self) -> list[dict[str, Any]]: """`_current_listing`, noting when it began to be read @@ -1004,11 +1005,12 @@ def link_refusal(self, number: int) -> str | None: "pullRequest" ) ) - _, refusal = merge_policy._linked_issue(pull, self.repo) + issue, refusal = merge_policy._linked_issue(pull, self.repo) except (_Unreadable, merge_policy.PolicyReadError) as exc: return f"unreadable: {exc}" if refusal is None: self._linked[number] = pull + self.linked_issue[number] = issue.get("number") return refusal def _closers(self, live: dict[Any, Any]) -> set[int]: @@ -1426,21 +1428,33 @@ def queued_hold(self, number: int) -> str | None: return self.evidence(number, head, pr).refusal -def lane_review_refusal(repo: str, number: int, pr: dict[str, Any]) -> str | None: - """Why pr-merge's gate would hold the pull request on its review lanes' - evidence, or None: the holding lane reasons of `babysit_risk` - (`_lane_reasons`, `holding`), over the evidence the gate binds - (`babysit_merge._lane_evidence`). The security lane's open CRITICAL - findings hold, and so does a lane whose review is not bound to the head. - A read that fails holds.""" +def lane_risk_refusal( + repo: str, + number: int, + pr: dict[str, Any], + config: merge_policy.PipelineConfig, + rung: str, + issue: int | None, +) -> str | None: + """Why pr-merge's gate would hold the pull request on its risk score, or + None: `babysit_risk.score` and `holding` over the inputs the gate builds + (`babysit_merge.score_unattended`) for a lane pull request, class + `LANE_CLASS` at `rung`, with `config` the base's tightened by the + default branch head's. So the review lanes' evidence (an open CRITICAL + security finding, a review not bound to the head), a symlink, submodule + or other non-regular mode, a high path, the manifest review, the trust + gate and the cost review all hold, the cost review lifted only by the + owner's decision on the pull request or its linked `issue` + (`merge_policy.cost_decision`). A read that fails holds.""" # Imported here: babysit_merge imports this module. import babysit_merge # noqa: PLC0415 import babysit_risk # noqa: PLC0415 try: - identity = merge_policy._default_branch_config(repo).agent_identity + identity = config.agent_identity if not identity: return "unreadable: no agent identity" + permission = merge_policy.permission_reader(repo) files, complete = merge_policy.changed_files(repo, pr) lanes, _ = babysit_merge._lane_evidence( repo, @@ -1448,15 +1462,39 @@ def lane_review_refusal(repo: str, number: int, pr: dict[str, Any]) -> str | Non pr, identity, frozenset(), - merge_policy.permission_reader(repo), + permission, [p for f in files for p in (f.path, f.previous_path) if p] if complete else None, ) - reasons = babysit_risk._lane_reasons(lanes) + files = merge_policy.with_contents(repo, pr, files) + private = json_object(json_object(pr.get("base")).get("repo")).get("private") + risk_input = babysit_risk.RiskInput( + work_class=merge_policy.LANE_CLASS, + rung=babysit_risk.Rung[rung.upper()], + files=files, + files_complete=complete, + max_changed_lines=config.max_changed_lines, + high_paths=config.denied_paths, + lanes=lanes, + blast_radius=None, + public=True if private is False else None, + pulumi_dirs=merge_policy.pulumi_dirs_at(repo, pr), + high_floor=config.high_path_floor, + c2_contradictions=config.c2_contradictions, + max_changed_files=config.max_changed_files, + lines_optional=True, + ) + risk = babysit_risk.score(risk_input) + if any(r.code == babysit_risk.COST_CODE for r in risk.reasons): + decision = merge_policy.cost_decision( + repo, pr, issue, permission=permission, agent_identity=identity + ) + if decision: + risk = babysit_risk.score(replace(risk_input, cost_decision=decision)) except Exception as exc: # noqa: BLE001 - any doubt holds, as the gate's return f"unreadable: {type(exc).__name__}: {exc}" - held = babysit_risk.holding(cast(Any, SimpleNamespace(reasons=reasons))) + held = babysit_risk.holding(risk) return "; ".join(f"{r.code}: {r.detail}" for r in held) or None @@ -1476,10 +1514,10 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: With `--lane-link`, the line also carries `ineligible`: null when the pull request's issue link is one the pr-merge lane takes (`Reader.link_refusal`), else why not, with refusal `verify:ineligible` - and no verdict or config read. With `--lane-reviews`, a line whose - refusal is null also carries `reviews`: null when pr-merge's gate would - not hold the pull request on its review lanes (`lane_review_refusal`), - else why. + and no verdict or config read. With `--lane-reviews` (and `--base-sha` + and `--rung`, the lane room's effective rung), a line whose refusal is + null also carries `reviews`: null when pr-merge's gate would not hold + the pull request on its risk score (`lane_risk_refusal`), else why. `lane-room --postmerge-result [--rung-cap ]`: whether the pr-merge lane may merge one more of its own pull requests now @@ -1497,6 +1535,7 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: cmd.add_argument("--base-sha", default=None) cmd.add_argument("--lane-link", action="store_true") cmd.add_argument("--lane-reviews", action="store_true") + cmd.add_argument("--rung", default=None) cmd.add_argument("number", type=int) cmd.add_argument("head") room = sub.add_parser("lane-room", allow_abbrev=False) @@ -1522,6 +1561,7 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: ) reader = Reader(args.repo, args.branch, now=now, max_fails=args.max_fails) link: dict[str, Any] = {} + config: merge_policy.PipelineConfig | None = None if args.lane_link: link["ineligible"] = reader.link_refusal(args.number) if link["ineligible"] is not None: @@ -1553,11 +1593,30 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: evidence = reader.evidence(args.number, args.head, pr) if args.lane_reviews and evidence.refusal is None: head = json_object(json_object(pr).get("head")).get("sha") - link["reviews"] = ( - lane_review_refusal(args.repo, args.number, json_object(pr)) - if head == args.head - else f"unreadable: the head is {head}, not {args.head}" - ) + if head != args.head: + link["reviews"] = f"unreadable: the head is {head}, not {args.head}" + elif config is None: + link["reviews"] = "unreadable: no --base-sha, so no config to score at" + else: + # The lowest of the room's effective rung and this base's config. + rung = merge_policy.effective_rung( + config_rung=config.rung, + loop_merge=config.loop_merge, + cap=args.rung if args.rung in merge_policy.RUNG_RANK else "off", + demoted=False, + ).rung + link["reviews"] = ( + f"unreadable: the lane's rung is {args.rung}" + if rung == "off" + else lane_risk_refusal( + args.repo, + args.number, + json_object(pr), + config, + rung, + reader.linked_issue.get(args.number), + ) + ) print(json.dumps(evidence.report() | link)) return 0 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 b08cf6c8d6..fc9e9dfa8b 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 @@ -1,10 +1,12 @@ -"""The review-lane hold enable-auto-merge reads (`lane_review_refusal`). - -Expected values come from the gate's lane rule (babysit_risk.py, `LANES` and -`holding`): the security lane's open CRITICAL findings hold -(`security-finding`, HIGH), a lane whose evidence is not bound to the head -holds (`evidence-unbound:`), and claude-review's open findings are -reported and never hold. A read that fails holds. +"""The gate's risk hold enable-auto-merge reads (`lane_risk_refusal`). + +Expected values come from the gate's own scoring rules in babysit_risk.py, +which native auto-merge would otherwise skip: only a regular file that stays +regular may rate below HIGH (`_mode_problems`), so a symlink or a submodule +pointer holds; the security lane's open CRITICAL findings hold +(`security-finding`); a lane whose evidence is not bound to the head holds; +claude-review's open findings are reported only; and a plain documentation +change by the lane merges. A read that fails holds. """ from __future__ import annotations @@ -22,38 +24,61 @@ import babysit_merge # noqa: E402 import babysit_merge_policy as policy # noqa: E402 import babysit_verify_evidence as ve # noqa: E402 -from babysit_risk import LaneEvidence # noqa: E402 +from babysit_risk import ChangedFile, LaneEvidence # noqa: E402 REPO = "example-org/example-repo" HEAD = "a" * 40 -PR = {"number": 41, "head": {"sha": HEAD, "ref": "feat/x"}, "base": {"sha": "b" * 40}} +BASE = "b" * 40 +PR = { + "number": 41, + "head": {"sha": HEAD, "ref": "feat/x"}, + "base": {"sha": BASE, "repo": {"private": False}}, +} CONFIG = policy.PipelineConfig( - "c" * 40, "C2", "pr-merge", "agent-lanes[bot]", None, (), None + "c" * 40, "C2", "pr-merge", "agent-lanes[bot]", None, (), "c2-mechanical" +) +DOC = ChangedFile( + "docs/guide.md", + None, + "modified", + 1, + 1, + True, + "100644", + "100644", + patch="@@ -1 +1 @@\n-old\n+new", ) +SYMLINK = ChangedFile("docs/link", None, "added", 1, 0, True, "120000", "000000") +SUBMODULE = ChangedFile("vendor/lib", None, "modified", 1, 1, False, "160000", "160000") def lanes( - *, code: int = 0, critical: int = 0, security_bound: bool = True + *, code: int = 0, critical: int = 0, bound: bool = True ) -> tuple[LaneEvidence, ...]: return ( LaneEvidence("claude-review", True, None, code), LaneEvidence( "claude-security-review", - security_bound, - None if security_bound else "marker-missing", - critical if security_bound else None, + bound, + None if bound else "marker-missing", + critical if bound else None, False, - critical if security_bound else None, + critical if bound else None, ), ) -def refusal(evidence: object) -> str | None: +def refusal( + files: tuple[ChangedFile, ...] = (DOC,), evidence: object = None +) -> str | None: + evidence = lanes() if evidence is None else evidence effect = evidence if isinstance(evidence, Exception) else None with ( - mock.patch.object(policy, "_default_branch_config", return_value=CONFIG), - mock.patch.object(policy, "changed_files", return_value=((), True)), + mock.patch.object(policy, "changed_files", return_value=(files, True)), + mock.patch.object(policy, "with_contents", side_effect=lambda repo, pr, f: f), + mock.patch.object(policy, "pulumi_dirs_at", return_value=()), mock.patch.object(policy, "permission_reader", return_value=lambda login: None), + mock.patch.object(policy, "cost_decision", return_value=None), mock.patch.object( babysit_merge, "_lane_evidence", @@ -61,55 +86,84 @@ def refusal(evidence: object) -> str | None: return_value=None if effect else (evidence, []), ), ): - return ve.lane_review_refusal(REPO, 41, PR) + return ve.lane_risk_refusal(REPO, 41, PR, CONFIG, "C2", 7) -class LaneReviewRefusal(unittest.TestCase): - def test_a_critical_security_finding_holds(self) -> None: - self.assertEqual(refusal(lanes(critical=1)), "security-finding: 1 open") +class LaneRiskRefusal(unittest.TestCase): + def test_a_plain_doc_change_holds_nothing(self) -> None: + self.assertIsNone(refusal()) + # claude-review's findings are reported, never held. + self.assertIsNone(refusal(evidence=lanes(code=3))) + + def test_a_symlink_or_a_submodule_pointer_holds(self) -> None: + for changed in (SYMLINK, SUBMODULE): + with self.subTest(changed.path): + held = refusal((DOC, changed)) + self.assertIsNotNone(held) + self.assertIn(changed.path, str(held)) - def test_clean_lanes_and_a_code_finding_hold_nothing(self) -> None: - self.assertIsNone(refusal(lanes())) - self.assertIsNone(refusal(lanes(code=3))) + def test_a_critical_security_finding_holds(self) -> None: + self.assertIn( + "security-finding: 1 open", str(refusal(evidence=lanes(critical=1))) + ) def test_an_unbound_security_review_holds(self) -> None: - held = refusal(lanes(security_bound=False)) - self.assertTrue( - str(held).startswith("evidence-unbound:claude-security-review"), held + self.assertIn( + "evidence-unbound:claude-security-review", + str(refusal(evidence=lanes(bound=False))), ) def test_a_failed_read_holds(self) -> None: - held = refusal(babysit_merge.HeadPusherUnproven("head-pusher-unproven:abc")) + held = refusal( + evidence=babysit_merge.HeadPusherUnproven("head-pusher-unproven:abc") + ) self.assertTrue(str(held).startswith("unreadable: "), held) -class EvidenceCommandReadsTheReviews(unittest.TestCase): +class EvidenceCommandReadsTheRisk(unittest.TestCase): def cli( - self, evidence: ve.Evidence, reviews: str | None, head: str = HEAD + self, + evidence: ve.Evidence, + risk: str | None, + *, + head: str = HEAD, + rung: str | None = "C2", ) -> dict[str, object]: out = io.StringIO() + argv = ["evidence", "--lane-reviews", "--repo", REPO, "--base-sha", BASE] + argv += ["--rung", rung] if rung is not None else [] with ( + mock.patch.object(policy, "read_config_at", return_value=CONFIG), + mock.patch.object(policy, "with_default_branch", return_value=CONFIG), mock.patch.object( policy, "pull_request", return_value=PR | {"head": {"sha": head}} ), mock.patch.object( ve.Reader, "evidence", lambda self, n, h, pr=None: evidence ), - mock.patch.object(ve, "lane_review_refusal", return_value=reviews) as read, + mock.patch.object(ve, "lane_risk_refusal", return_value=risk) as read, contextlib.redirect_stdout(out), ): - ve.main(["evidence", "--lane-reviews", "--repo", REPO, "41", HEAD]) + ve.main([*argv, "41", HEAD]) self.read = read return json.loads(out.getvalue()) - def test_a_pass_carries_the_review_hold(self) -> None: - report = self.cli(ve.Evidence(None, HEAD, (), 0), "security-finding: 1 open") + def test_a_pass_carries_the_risk_hold_at_the_room_s_rung(self) -> None: + report = self.cli(ve.Evidence(None, HEAD, (), 0), "unsafe-path: docs/link") self.assertEqual( - (report["refusal"], report["reviews"]), (None, "security-finding: 1 open") + (report["refusal"], report["reviews"]), (None, "unsafe-path: docs/link") ) + self.assertEqual(self.read.call_args.args[4], "C2") self.assertIsNone(self.cli(ve.Evidence(None, HEAD, (), 0), None)["reviews"]) - def test_a_refused_verdict_reads_no_reviews(self) -> None: + def test_no_rung_from_the_room_holds(self) -> None: + for rung in (None, "off", "C9"): + with self.subTest(rung=rung): + report = self.cli(ve.Evidence(None, HEAD, (), 0), None, rung=rung) + self.assertTrue(str(report["reviews"]).startswith("unreadable: ")) + self.read.assert_not_called() + + def test_a_refused_verdict_reads_no_risk(self) -> None: report = self.cli(ve.Evidence(ve.FAIL, HEAD, (), 0), None) self.assertNotIn("reviews", report) self.read.assert_not_called() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py index c32b220d10..906246fa75 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -113,6 +113,9 @@ def test_a_clear_lane_counts_its_merges_in_flight_against_the_cap(self) -> None: (room.refusal, room.unreadable, room.count, room.cap, room.queued), (None, None, 2, 5, frozenset({11})), ) + # The rung the gate scores at: merge.rung C2, the loop setting c2. + self.assertEqual(room.rung, "C2") + self.assertEqual(self.room(cap="C2").rung, "C2") # The lane's own merges, since the post-merge floor, judged by its check. self.assertEqual( ( @@ -208,7 +211,7 @@ def room( repo: str, answer: object, *, lane_rung_cap: str | None ) -> policy.LaneRoom: seen.append((repo, answer, lane_rung_cap)) - return policy.LaneRoom(count=1, cap=5, queued=frozenset({3})) + return policy.LaneRoom(count=1, cap=5, queued=frozenset({3}), rung="C2") out = io.StringIO() with ( @@ -234,7 +237,14 @@ def room( self.assertEqual(seen, [(REPO, "answer", "")]) self.assertEqual( json.loads(out.getvalue()), - {"refusal": None, "unreadable": None, "count": 1, "cap": 5, "queued": [3]}, + { + "refusal": None, + "unreadable": None, + "count": 1, + "cap": 5, + "queued": [3], + "rung": "C2", + }, ) def test_a_missing_report_is_unreadable(self) -> None: diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index 5768fe1acd..a7e574bec3 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -188,7 +188,8 @@ expect() { : >"$TMP_ROOT/log" out="$(PATH="$TMP_ROOT/bin:$PATH" STUB_LOG="$TMP_ROOT/log" STUB_MODULE="${STUB_MODULE:-$MODULE}" \ GITHUB_EVENT_NAME="${EVENT:-pull_request}" EVENT_ACTION="${ACTION-synchronize}" GITHUB_REF="${REF:-refs/pull/7/merge}" PR_NUMBER="${PR_NUMBER-7}" \ - GITHUB_REPOSITORY=o/r RUNNER_TEMP="$TMP_ROOT" GROUP_BASE_SHA="${GROUP_BASE_SHA-}" bash -e "$TMP_ROOT/body.sh" 2>&1)" && rc=0 || rc=$? + GITHUB_REPOSITORY=o/r RUNNER_TEMP="$TMP_ROOT" GROUP_BASE_SHA="${GROUP_BASE_SHA-}" \ + LANES_DISABLED="${KILL-false}" LANES_REPO_DISABLED="${REPO_KILL-false}" bash -e "$TMP_ROOT/body.sh" 2>&1)" && rc=0 || rc=$? calls=$(grep -cE '/(comments|reviews)\?' "$TMP_ROOT/log") if [[ "$rc" -ne "$want_rc" ]]; then fail "$label: expected rc=$want_rc got rc=$rc out='$out'" @@ -215,6 +216,26 @@ expect "a person's pull request is left to pr-merge's gate, with no discussion r STUB_PULL="$(pull 'Closes #1' '{"id":1,"login":"melodic-automation-lanes[bot]","type":"User"}')" expect "a user account with the lanes App's login is not the lanes App" 0 0 "not the lanes App's" +# The lanes kill switch stops every lanes App pull request, and only those. +for kill in "KILL=true" "KILL=" "REPO_KILL=true" "REPO_KILL=1"; do + STUB_PULL="$(pull 'Closes #1')" + export "${kill?}" + expect "with $kill, a lanes App pull request fails" 1 0 "the lanes kill switch is on" + STUB_PULL="$(pull 'Closes #1' '{"id":153232337,"login":"kyle-sexton","type":"User"}')" + expect "with $kill, a person's pull request is not held by it" 0 0 "not the lanes App's" + STUB_PULL="$(pull 'Closes #1' '{"id":49699333,"login":"dependabot[bot]","type":"Bot"}')" + expect "with $kill, a Dependabot pull request is not held by it" 0 0 "not the lanes App's" + STUB_PULL="$(pull 'Closes #1')" + unset KILL REPO_KILL +done +# shellcheck disable=SC2016 # a literal workflow expression, not shell +if grep -qF 'LANES_DISABLED: ${{ vars.CLAUDE_LANES_DISABLED }}' <<<"$step" && + grep -qF 'LANES_REPO_DISABLED: ${{ vars.CLAUDE_LANES_REPO_DISABLED }}' <<<"$step"; then + ok "the step reads both kill switches from the vars context" +else + fail "the step does not read both kill switches from the vars context" +fi + STUB_PULL="$(pull $'Closes #1\n\nHold: wait for the canary')" expect "a Hold: line in the body fails" 1 3 "::error::#7: its body holds it" STUB_PULL="$(pull $'Closes #1\r\n hold: owner call')" From 2948665ac904b32cc221c099d62f76727c287b40 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:38:00 -0400 Subject: [PATCH 12/13] fix(pr-pipeline): give enable-auto-merge read access to the demotion records The lane's room re-checks the demotion records through the job's GITHUB_TOKEN; without issues: read that read fails, the room reads as unreadable, and no lane pull request is ever armed. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/workflow.test.mjs | 2 +- .github/workflows/pr-merge.yml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 7a8ee9667d..fce9696f03 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -241,7 +241,7 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () const block = /\n {4}permissions:\n((?: {6}[a-z-]+: [a-z]+.*\n)+)/.exec(arm)[1]; assert.deepEqual( [...block.matchAll(/ {6}([a-z-]+): ([a-z]+)/g)].map((m) => `${m[1]}: ${m[2]}`), - ["contents: read", "actions: read", "pull-requests: read", "checks: read", "id-token: write"], + ["contents: read", "actions: read", "pull-requests: read", "checks: read", "issues: read", "id-token: write"], ); // The verifier evidence is read as the gate reads it: the job's // GITHUB_TOKEN for the Actions reads, with the run's head-ref map and diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index de47ac7395..9efdc2aae5 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1369,6 +1369,7 @@ jobs: actions: read # the verifier verdicts, their runs and the head-ref map pull-requests: read # the lane's merges and the merge queue, for the lane's room checks: read # the review lanes' annotations, as the gate binds their evidence + issues: read # the demotion records the lane's room re-checks id-token: write # prove to the token broker which workflow this is steps: - name: Check out the default branch tip From d3c80641d3f89c8cee674e13c606272e3b1747b8 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:50:50 -0400 Subject: [PATCH 13/13] fix(pr-pipeline): reuse the post-merge report only where the gate would lane_room checked the report's age and demotion changes but not whether a lane merge had landed since it was computed, so a merge whose full run had already failed did not stop native arming. It now applies the gate's own reuse rule, shared_refusal, as a candidate at the default branch head, and any refusal (new-merge:# included) makes the room unreadable, which disarms and arms nothing. Co-Authored-By: Claude Opus 5.5 --- .../feat-native-lane-refs-arm-5c2e81a4.md | 2 +- docs/conventions/pr-pipeline/README.md | 5 ++- .../scripts/babysit_merge_policy.py | 34 +++++++++++------ .../scripts/tests/test_babysit_lane_room.py | 37 +++++++++++++++++++ 4 files changed, 63 insertions(+), 15 deletions(-) diff --git a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md index ecb0a36fd4..0d5b9036dd 100644 --- a/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md +++ b/.changes/pr-pipeline/feat-native-lane-refs-arm-5c2e81a4.md @@ -5,7 +5,7 @@ bump: minor ### Changed - The `enable-auto-merge` activity arms a lanes App pull request whose issue link the merge gate takes, a plain `Refs: #` issue included, instead of only one whose body opens with `Closes #`. The link is read with the verdict, through the gate's own rule (`babysit_verify_evidence.py evidence --lane-link`, which runs `babysit_merge_policy._linked_issue`), on the scheduled and hand-started runs only: two or more `Refs:` issues, a `Refs:` line in any other form, a number that is no issue, two closing issues, a closing issue in another repository, and a failed read all leave the pull request unarmed, or disarm it. An event run reads no link, so it never disarms on one. -- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, one the gate would not reuse (older than 30 minutes, as a re-run job reads the run's first report, or with a demotion record filed or changed since), or a room that cannot be read, holds as a refusal does: nothing is armed and every armed lanes App pull request is disarmed. The report is read whenever `post-merge-dispatch` ran, so a later step of that job failing does not disarm anything. The job now holds `pull-requests: read` for the merge-queue and merge reads. +- The `enable-auto-merge` activity applies the merge gate's run-level refusals, which native auto-merge would otherwise skip. Each scheduled or hand-started run waits for the same run's `post-merge-dispatch` job, which keeps its post-merge report as a one-day artifact, and reads the lane's room from that report (`babysit_verify_evidence.py lane-room`, `babysit_merge_policy.lane_room`). While a demotion record, `merge.rung`, the loop setting or the rung cap holds the lane off, or a lane merge failed its post-merge check, nothing is armed and every armed lanes App pull request is disarmed. At `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests not yet queued, nothing more is armed. A missing report, one the gate would not reuse (`shared_refusal`: older than 30 minutes, as a re-run job reads the run's first report, a demotion record filed or changed since, or a lane merge landed since that it does not count), or a room that cannot be read, holds as a refusal does: nothing is armed and every armed lanes App pull request is disarmed. The report is read whenever `post-merge-dispatch` ran, so a later step of that job failing does not disarm anything. The job now holds `pull-requests: read` for the merge-queue and merge reads. - In the merge queue, `ci-status` holds a lanes App pull request whose verifier pass no longer binds its live issue link. It reads the counted pass's `verdict.json` (one artifact download) and the closing issues (one GraphQL read), and `merge-holds.mjs` `linkHold` holds unless the body still closes, or with no closing issue still names by a plain `Refs:` line, the one issue the pass judged, linked the same way. This is pr-merge's reader rule (`verify:spec-changed`), so a body edited after arming, such as `Refs: #A` to `Closes #B`, no longer merges on a pass for the old issue. A trusted approval of the head lifts it, as it lifts the verifier hold. `refsIssue` moves to `merge-holds.mjs`, which `resolve-pr.mjs` re-exports. - `enable-auto-merge` holds a lanes App pull request on the merge gate's risk score: `evidence --lane-reviews` runs `babysit_risk.score` and `holding` over the inputs the gate builds, at the lane room's effective rung. Each of these leaves it unarmed or disarms it: an open CRITICAL security-lane finding or a review not bound to the head (whether or not the lane applied its hold label), a symlink, submodule or other non-regular mode, a high path, the manifest review, the trust gate, and the cost review unless the owner's decision lifts it. The job now also holds `checks: read` for the lanes' annotations. - `ci-status` fails a lanes App pull request while the lanes kill switch (`CLAUDE_LANES_DISABLED` or `CLAUDE_LANES_REPO_DISABLED`) reads anything but `false`, so one armed before the switch was turned on stops in the merge queue. It reads the vars context and makes no API call. Owner and Dependabot pull requests are not affected. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index c783661fef..9ecc12b085 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1083,8 +1083,9 @@ an open CRITICAL security finding or a review not bound to the head, whether or applied its hold label, a symlink, submodule or other non-regular mode, a high path, the manifest review, the trust gate and the cost review all hold. Each such run also waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge -report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it -holds as a refusal does): while the gate holds its lane off (a demotion record, +report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report, or +one the gate would not reuse (`shared_refusal`: too old, a demotion record changed since, or a lane +merge landed since that it does not count), it holds as a refusal does): while the gate holds its lane off (a demotion record, `merge.rung`, the loop setting or the rung cap off, a lane merge that failed its post-merge check) nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue 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 2e663bfcaf..fa0d026353 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 @@ -4502,15 +4502,11 @@ def lane_room( Any read failure, a missing `shared` and an in-flight count that could not be read included, is `unreadable`. So is a `shared` the gate would - not reuse for its age (`_shared_age_refusal`) or for a demotion record - filed or changed since (`_demotion_changed`): a re-run job downloads the - run's first report, up to a day old, and reads its demotion and - refusal.""" + not reuse (`shared_refusal`): too old (a re-run job downloads the run's + first report, up to a day old), a demotion record filed or changed + since, or a lane merge landed since that it does not count.""" if shared is None: return LaneRoom(unreadable="post-merge answer missing or unreadable") - stale = _shared_age_refusal(shared, (now or datetime.now(UTC)) - shared.computed_at) - if stale: - return LaneRoom(unreadable=f"post-merge answer {stale}") try: default = _default_branch(repo, shared=True) config = _default_branch_config(repo, default) @@ -4519,11 +4515,25 @@ def lane_room( if enabled_authority(config) != PR_MERGE: return LaneRoom(refusal=AUTHORITY_UNCONFIGURED) identity = config.agent_identity - if not identity or _login_key(shared.identity) != _login_key(identity): - return LaneRoom(unreadable="post-merge answer is for another identity") - owner, name = _split_repo(repo) - if not shared.demotion.active and _demotion_changed(owner, name, shared, identity): - return LaneRoom(unreadable="post-merge answer demotion-changed") + if not identity: + return LaneRoom(unreadable="no agent identity") + # The gate's own reuse rule, as a candidate at the default branch head + # with the gate's self logins (none): the same identity and lane, its + # age, no demotion record filed or changed since, and every lane merge + # the check would judge now counted (`new-merge:#` otherwise). + unusable = shared_refusal( + repo, + shared, + branch=default, + base_sha=config.base_sha, + identity=identity, + self_logins=frozenset(), + lane=True, + authority=PR_MERGE, + now=now, + ) + if unusable: + return LaneRoom(unreadable=f"post-merge answer {unusable}") if shared.gate.refusal: return LaneRoom(refusal=f"post-merge:{shared.gate.refusal}") cap = rung_cap(repo, authority=PR_MERGE, lane_value=lane_rung_cap) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py index 906246fa75..581849973b 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -97,6 +97,10 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: mock.patch.object( policy, "_demotion_changed", return_value=demotion_changed ), + # shared_refusal's reads: the answer's own config commit and main + # head, so only its age and demotion decide. + mock.patch.object(policy, "newest_config_commit", return_value=MAIN), + mock.patch.object(policy, "_get", return_value=(200, {"sha": MAIN})), ): result = policy.lane_room( REPO, @@ -173,6 +177,39 @@ def test_an_answer_the_gate_would_not_reuse_is_unreadable(self) -> None: self.assertTrue(room.unreadable) self.assertFalse(self.seen, "no in-flight read after an unusable answer") + def test_a_lane_merge_the_answer_does_not_count_is_unreadable(self) -> None: + # The gate's reuse rule (shared_refusal) reads afresh when a lane + # merge landed after the report was computed (`new-merge:#`), so + # its failed full run is not missed; the room holds rather than arm. + seen: list[dict[str, object]] = [] + + def refusal(repo: str, answer: object, **kwargs: object) -> str: + seen.append(kwargs) + return "new-merge:#7" + + with mock.patch.object(policy, "shared_refusal", side_effect=refusal): + room = self.room() + self.assertEqual( + (room.refusal, room.unreadable), + (None, "post-merge answer new-merge:#7"), + ) + self.assertFalse(self.seen, "no in-flight read after an unusable answer") + # Bound as the gate binds a candidate at the default branch head. + self.assertEqual( + { + k: seen[0][k] + for k in ("branch", "base_sha", "identity", "lane", "authority") + }, + { + "branch": "main", + "base_sha": MAIN, + "identity": IDENTITY, + "lane": True, + "authority": "pr-merge", + }, + ) + self.assertEqual(seen[0]["self_logins"], frozenset()) + def test_an_active_demotion_is_not_re_read(self) -> None: # Already refusing: a changed record cannot make it allow. room = self.room(shared(demoted=True), demotion_changed=True)