From 0c6e337f092d26a60436d0466c38c42c1b0cc911 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:51:43 -0400 Subject: [PATCH 01/11] fix(pr-merge): require a pr-verify pass before a lane pull request merges natively Native auto-merge armed lanes App pull requests on text holds alone, so one could merge with no verifier pass and past the repeated-FAIL cap. - enable-auto-merge.mjs arms, and keeps armed, only when pr-merge's own reader (babysit_verify_evidence.py, new `evidence` command) refuses nothing at the head it read: a pass stands, no FAIL at the head, FAILs under merge.max-verify-fails. The job reads it with GITHUB_TOKEN, the head-ref map and the carried run cache, as the gate does. - ci-status's lanes-hold step, in the merge queue only, requires a PASS or PASS-NOTES artifact at the queued head from a successful pr-verify run on main (merge-holds.mjs verdictHold), looked up by exact name. A trusted approval of the head lifts it. A pull request event reads nothing: pr-verify runs only after a green pull_request run, so a red ci-status there would never get a verdict. Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-merge/enable-auto-merge.mjs | 71 +++++++++-- .../pr-merge/enable-auto-merge.test.mjs | 117 ++++++++++++++++-- .github/scripts/pr-merge/merge-holds.mjs | 56 ++++++++- .github/workflows/pr-merge.yml | 47 ++++++- .github/workflows/pr-require-checks.yml | 46 ++++++- .github/workflows/pr-verify.yml | 6 +- docs/conventions/pr-pipeline/README.md | 18 ++- .../scripts/babysit_verify_evidence.py | 33 +++++ .../tests/test_babysit_verify_evidence.py | 41 ++++++ scripts/ci-lanes-pr-hold.test.sh | 93 +++++++++++++- 10 files changed, 499 insertions(+), 29 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index c20fbab94c..b4c6738333 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -24,6 +24,13 @@ // group, so a push after arming is held without waiting for this job. // - 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's `evidence` refuses nothing (verifyHold), so +// a PASS or PASS-NOTES of pr-verify at that head stands, no FAIL at it, and +// FAILs at fewer than merge.max-verify-fails heads. It is read last, only +// for a pull request every other rule would arm or keep armed. The merge +// queue's ci-status checks the pass again at the head it merges +// (merge-holds.mjs verdictHold), since a push after arming keeps it armed. // 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 @@ -38,7 +45,11 @@ // TRACKER_CONFIG the base .work-item-tracker.json, for the human-gated // label (default .base/.work-item-tracker.json) // RESOLVED_CONFIG the resolve-config output, for the hold paths +// MAX_VERIFY_FAILS merge.max-verify-fails (default: the reader's) +// BABYSIT_VERIFY_*, BABYSIT_RUN_CACHE the reader's Actions token, saved +// listing, head-ref map and run cache, as the gate's // GITHUB_API_URL +import { spawnSync } from "node:child_process"; import process from "node:process"; import { pathToFileURL } from "node:url"; @@ -61,6 +72,7 @@ const MAX_PAGES = 10; // Changed files read per pull request, 100 a page; more is a head the run // does not arm on. const MAX_FILE_PAGES = 3; +const VERIFY_SCRIPT = ".base/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py"; export const PULLS_QUERY = `query($owner: String!, $name: String!, $after: String) { repository(owner: $owner, name: $name) { @@ -158,9 +170,38 @@ async function changedFiles(github, slug, number) { return null; } +// Why the gate's verifier `evidence` (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)) { + return "its verifier verdict could not be read"; + } + 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)})`; +} + +// 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) { + return async (number, head) => { + const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "evidence", "--repo", env.GITHUB_REPOSITORY]; + if (env.MAX_VERIFY_FAILS) { + args.push("--max-fails", String(env.MAX_VERIFY_FAILS)); + } + const done = spawnSync("python3", [...args, String(number), head], { encoding: "utf8", env, timeout: 240_000 }); + if (done.status !== 0) { + throw new Error(`babysit_verify_evidence.py exited ${done.status ?? done.signal}: ${String(done.stderr ?? "").trim().split("\n").at(-1)}`); + } + return JSON.parse(String(done.stdout).trim().split("\n").at(-1)); + }; +} + // What to do with one listed pull request: { action: arm | disarm | none, reason }. // `detail` is its discussion; undefined means not read yet, and an arm or a -// keep then waits on it (action `read`). +// keep then waits on it (action `read`). Its `verify` is the gate's verifier +// evidence; absent, an arm or a keep waits on it (action `verify`). export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { if (pull.author?.__typename !== "Bot" || pull.author.login !== bot) { return { action: "none", reason: "not the lanes App's" }; @@ -187,13 +228,17 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { if (vetoed !== null) { return { action: armed ? "disarm" : "none", reason: vetoed }; } - if (armed) { - return { action: "none", reason: "already armed" }; - } - if (detail.timelineItems?.nodes?.at(-1)?.actor?.__typename === "User") { + if (!armed && detail.timelineItems?.nodes?.at(-1)?.actor?.__typename === "User") { return { action: "none", reason: "a person turned its auto-merge off" }; } - return { action: "arm", reason: "no hold" }; + if (!("verify" in detail)) { + return { action: "verify", reason: "its verifier verdict is not read yet" }; + } + const unverified = verifyHold(detail.verify, pull.headRefOid); + if (unverified !== null) { + return { action: armed ? "disarm" : "none", reason: unverified }; + } + return armed ? { action: "none", reason: "already armed" } : { action: "arm", reason: "no hold" }; } async function listPulls(github, owner, name) { @@ -214,7 +259,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 } = {}) { +export async function main({ env = process.env, github, verify } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -232,12 +277,13 @@ export async function main({ env = process.env, github } = {}) { const humanGated = humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json"); const holdPaths = readHoldPaths(env.RESOLVED_CONFIG ?? ""); github ??= createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); + verify ??= pythonVerify(env); const pulls = await listPulls(github, repo[1], repo[2]); let failed = 0; for (const pull of pulls) { let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }); + let detail = null; if (action === "read") { - let detail = null; try { const variables = { owner: repo[1], name: repo[2], number: pull.number }; detail = (await github("POST", "/graphql", { query: DETAIL_QUERY, variables }))?.data?.repository?.pullRequest ?? null; @@ -249,6 +295,15 @@ export async function main({ env = process.env, github } = {}) { } ({ action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }, detail)); } + if (action === "verify") { + let evidence = null; + try { + evidence = await verify(pull.number, pull.headRefOid); + } catch (error) { + console.log(`enable-auto-merge: #${pull.number} verifier verdict did not read: ${escapeCommand(error?.message ?? error)}`); + } + ({ action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence })); + } if (action === "none") { if (reason !== "not the lanes App's") { console.log(`enable-auto-merge: #${pull.number} left: ${reason}`); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 9dba8db3ad..c43b5bbe46 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -14,7 +14,8 @@ import process from "node:process"; import { fileURLToPath } from "node:url"; import { test } from "node:test"; -import { ARM_MUTATION, BODY_HOLD, DETAIL_QUERY, DISARM_MUTATION, HUMAN_VETO, PULLS_QUERY, decide, main } from "./enable-auto-merge.mjs"; +import { ARM_MUTATION, BODY_HOLD, DETAIL_QUERY, DISARM_MUTATION, HUMAN_VETO, PULLS_QUERY, decide, main, verifyHold } from "./enable-auto-merge.mjs"; +import { VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; import { decide as decideOutcome } from "../work-resolve-issue/report-outcome.mjs"; const REPO = "melodic-software/claude-code-plugins"; @@ -58,6 +59,7 @@ const said = (login, body, at, association = "OWNER", typename = "User") => ({ createdAt: at, authorAssociation: association, }); +const PASSED = { refusal: null, head: HEAD }; const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name) => ({ name })) }); function world(pulls, { failArm = false } = {}) { @@ -83,21 +85,32 @@ function world(pulls, { failArm = false } = {}) { } return { data: {} }; }; + // The gate's verifier evidence: a pull request's `verdict` (a report, or + // an Error to throw), else a pass at its head. + const verified = []; + const verify = async (number, head) => { + verified.push(number); + const found = pulls.find((candidate) => candidate.number === number); + if (found?.verdict instanceof Error) { + throw found.verdict; + } + return found?.verdict ?? { refusal: null, head }; + }; const writes = () => calls .filter(({ method, body }) => method === "POST" && body.query !== PULLS_QUERY && body.query !== DETAIL_QUERY) .map(({ body }) => ({ mutation: body.query === ARM_MUTATION ? "arm" : body.query === DISARM_MUTATION ? "disarm" : "?", ...body.variables })); - return { github, writes }; + return { github, writes, verify, verified }; } async function run(pulls, env = {}, options = {}) { - const { github, writes } = world(pulls, options); + 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 }); - return { code, writes: writes(), lines }; + const code = await main({ env: { ...ENV, ...env }, github, verify }); + return { code, writes: writes(), lines, verified }; } finally { console.log = log; } @@ -242,7 +255,7 @@ test("report-outcome's own pull request body arms, and its hold and Refs bodies const opened = plan(overrides); assert.equal(opened.action, "open-pr"); // Files outside the hold paths, so only the body decides. - return decide(pull(1, { body: opened.body }), { bot: "melodic-automation-lanes", humanGated: "needs-human", holdPaths: [".github/**"] }, { ...detail(), files: [] }); + return decide(pull(1, { body: opened.body }), { bot: "melodic-automation-lanes", humanGated: "needs-human", holdPaths: [".github/**"] }, { ...detail(), files: [], verify: PASSED }); }; assert.deepEqual(verdict({}), { action: "arm", reason: "no hold" }); assert.equal(verdict({ humanMerge: true }).action, "none"); @@ -282,7 +295,7 @@ test("an armed pull request a member now vetoes is disarmed", async () => { test("a pull request whose discussion cannot all be read is not armed", async () => { const cut = { totalCount: 101, nodes: [] }; const options = { bot: LANES.login, humanGated: "needs-human", holdPaths: [] }; - assert.equal(decide(pull(1), options, { ...detail(), files: [] }).action, "arm"); + assert.equal(decide(pull(1), options, { ...detail(), files: [], verify: PASSED }).action, "arm"); const held = (pr) => decide(pull(1), options, { ...detail(), files: [], ...pr }); assert.equal(held({ comments: cut }).action, "none"); assert.equal(held({ reviews: cut }).action, "none"); @@ -369,3 +382,93 @@ test("hold paths that cannot be read arm nothing", async () => { const { writes } = await run([pull(1)], { RESOLVED_CONFIG: "/nonexistent/resolved-config.json" }); assert.deepEqual(writes, []); }); + +// pr-merge's gate merges only on a pr-verify pass at the head, FAILs at fewer +// than merge.max-verify-fails heads (babysit_verify_evidence.py), and native +// auto-merge must not merge what the gate would refuse. +test("no pass at the head, a FAIL or the repeated-FAIL cap arms nothing", async () => { + const { code, writes, verified } = await run([ + pull(1, { verdict: { refusal: "verify:missing", head: HEAD } }), + pull(2, { verdict: { refusal: "verify:fail", head: HEAD } }), + pull(3, { verdict: { refusal: "verify:repeated-fail", head: HEAD, needsHuman: true } }), + pull(4, { verdict: { refusal: "verify:earlier-fail", head: HEAD } }), + pull(5, { verdict: { refusal: null, head: "c".repeat(40) } }), + pull(6, { verdict: new Error("gh: rate limited") }), + pull(7, { verdict: { refusal: "verify:unreadable", head: HEAD } }), + pull(8), + ]); + assert.equal(code, 0); + assert.deepEqual(verified, [1, 2, 3, 4, 5, 6, 7, 8]); + assert.deepEqual(writes, [{ mutation: "arm", id: "PR_8", head: HEAD }]); +}); + +test("an armed pull request whose head lost its pass, or passed the cap, is disarmed", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const { writes, lines } = await run([ + // Armed on a passing head, then pushed: the gate finds no verdict at the new head. + pull(1, { autoMergeRequest: armed, verdict: { refusal: "verify:missing", head: HEAD } }), + pull(2, { autoMergeRequest: armed, verdict: { refusal: "verify:repeated-fail", head: HEAD } }), + pull(3, { autoMergeRequest: armed, verdict: new Error("listing failed") }), + pull(4, { autoMergeRequest: armed }), + ]); + assert.deepEqual(writes, [ + { mutation: "disarm", id: "PR_1" }, + { mutation: "disarm", id: "PR_2" }, + { mutation: "disarm", id: "PR_3" }, + ]); + assert.ok(lines.some((line) => /#2 disarmed: pr-merge's gate refuses its verifier verdict \(verify:repeated-fail\)/.test(line))); +}); + +test("the verdict is read only for a lanes App pull request nothing else holds", async () => { + const { writes, verified } = await run([ + pull(1, { author: { __typename: "User", login: "kyle-sexton" } }), + pull(2, { labels: labels("do-not-merge") }), + pull(3, { isDraft: true }), + pull(4, { discussion: { comments: [said("kyle-sexton", "do not merge", "2026-10-23T10:00:00Z")] } }), + pull(5, { discussion: { disabledBy: { __typename: "User", login: "kyle-sexton" } } }), + ]); + assert.deepEqual(writes, []); + assert.deepEqual(verified, []); +}); + +test("verifyHold allows only the reader's null refusal at the head read", () => { + assert.equal(verifyHold(PASSED, HEAD), null); + 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/); +}); + +// The merge queue's check (merge-holds.mjs verdictHold): a pass counts only +// from pr-verify.yml's completed, successful workflow_run run on main here. +test("verdictHold counts a pass only from a successful pr-verify run on main", () => { + const run = (overrides = {}) => ({ + path: VERIFY_PATH, + event: "workflow_run", + head_branch: "main", + status: "completed", + conclusion: "success", + repository: { full_name: REPO }, + head_repository: { full_name: REPO }, + ...overrides, + }); + const hold = (records) => verdictHold(records, REPO); + assert.equal(hold([{ verdict: "pass", run: run() }]), null); + assert.equal(hold([{ verdict: "pass-notes", run: run() }]), null); + assert.equal(hold([{ verdict: "pass", run: run({ conclusion: "failure" }) }, { verdict: "pass-notes", run: run() }]), null); + for (const bad of [ + { path: ".github/workflows/other.yml" }, + { event: "pull_request" }, + { head_branch: "feature" }, + { status: "in_progress", conclusion: null }, + { conclusion: "failure" }, + { repository: { full_name: "fork/claude-code-plugins" } }, + { head_repository: { full_name: "fork/claude-code-plugins" } }, + ]) { + assert.match(hold([{ verdict: "pass", run: run(bad) }]), /has not passed this head/, JSON.stringify(bad)); + } + assert.match(hold([{ verdict: "fail", run: run() }]), /has not passed this head/); + assert.match(hold([]), /has not passed this head/); + assert.match(hold(null), /could not be read/); + assert.match(verdictHold([], "not a repo"), /could not be read/); +}); diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index bc8dd278fc..66982b3144 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -16,9 +16,19 @@ // merge.diff-check.denied-paths plus HOLD_PATH_FLOOR. A person on the // trusted-actor list lifts it by approving the head: their newest review of // the head that approves, requests changes or was dismissed must approve. +// - No verifier pass, when a verdicts file is given (ci-status gives one in +// the merge queue only): the head merges on a PASS or PASS-NOTES that +// pr-verify.yml uploaded from a completed, successful run on main +// (verdictHold). The same approval of the head lifts it, the human merge +// 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. // // Command line: merge-holds.mjs -// . The +// +// []. The verdicts file holds one {name, verdict, run} per +// line, run as GET actions/runs/{id} 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 // line; the files file one JSON array per changed file, its path then any // renamed-from path; the denied-paths file the config's @@ -157,12 +167,40 @@ export function holds(body, items, files, holdPaths, lifters = []) { return reasons; } +// The verifier workflow (resolve-pr.mjs VERIFY_PATH). +export const VERIFY_PATH = ".github/workflows/pr-verify.yml"; +const PASSES = Object.freeze(["pass", "pass-notes"]); + +// Why the head has no verifier pass, or null when a record shows one: a pass +// or pass-notes artifact whose run is pr-verify.yml's, event workflow_run, on +// `branch` of `repo` with its head in `repo`, completed with success +// (babysit_verify_evidence `_origin` and a counted run). The records are +// read for the head alone, by exact name, so a record of another head never +// reaches here. +export function verdictHold(records, repo, branch = "main") { + if (!Array.isArray(records) || !/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(String(repo))) { + return "its pr-verify verdicts could not be read"; + } + const pass = records.find( + ({ verdict, run } = {}) => + PASSES.includes(verdict) && + run?.path === VERIFY_PATH && + run.event === "workflow_run" && + run.head_branch === branch && + run.repository?.full_name === repo && + run.head_repository?.full_name === repo && + run.status === "completed" && + run.conclusion === "success", + ); + return pass ? null : "pr-verify has not passed this head"; +} + // A log line can carry a changed file's name: escape what would start a // workflow command on a new line. const escape = (text) => String(text).replaceAll("%", "%25").replaceAll("\r", "%0D").replaceAll("\n", "%0A"); export function main(argv = process.argv.slice(2)) { - const [number, bodyFile, discussionFile, filesFile, deniedFile, actorsFile, head] = argv; + const [number, bodyFile, discussionFile, filesFile, deniedFile, actorsFile, head, verdictsFile] = argv; try { if ( !/^[1-9][0-9]*$/.test(String(number)) || @@ -174,7 +212,7 @@ export function main(argv = process.argv.slice(2)) { !/^[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()); @@ -194,6 +232,18 @@ export function main(argv = process.argv.slice(2)) { if (held && lifters.length && Array.isArray(holdPaths)) { console.log(`#${number}: ${lifters.join(", ")} approved this head, which lifts its hold path (${escape(held)}).`); } + if (verdictsFile) { + const unverified = verdictHold(lines(verdictsFile).map((line) => JSON.parse(line)), process.env.GITHUB_REPOSITORY); + if (!unverified) { + console.log(`#${number}: no verifier hold: pr-verify passed this head.`); + } else if (lifters.length) { + console.log(`#${number}: no verifier hold: ${lifters.join(", ")} approved this head, which lifts '${unverified}'.`); + } else { + reasons.push( + `${unverified}; wait for pr-verify to pass it, 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`, + ); + } + } for (const reason of reasons) { console.log(`::error::#${number}: ${escape(reason)}.`); } diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 22eda86492..562d09663d 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1258,7 +1258,11 @@ jobs: # 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. Its own job and token: the broker mints one token per + # the broker refuses. It arms only on the verifier evidence pr-merge's gate + # would take at the head (babysit_verify_evidence.py, read with the job's + # GITHUB_TOKEN as the gate does, from the head-ref map and run cache the + # last gate run kept), so a FAIL or the repeated-FAIL cap disarms too. + # Its own job and token: the broker mints one token per # job. Rules: .github/scripts/pr-merge/enable-auto-merge.mjs. enable-auto-merge: # An issue run that skipped every job (a label other than agent-ready) @@ -1274,9 +1278,10 @@ jobs: group: pr-merge-enable-auto-merge queue: max runs-on: ubuntu-24.04 - timeout-minutes: 10 + timeout-minutes: 15 permissions: contents: read # check out main + actions: read # the verifier verdicts, their runs and the head-ref map id-token: write # prove to the token broker which workflow this is steps: - name: Check out the default branch tip @@ -1350,21 +1355,55 @@ jobs: issues: ${{ steps.config.outputs.issues }} superseded-ok: true + # The verdict reader's untied FAILs, as the gate job reads them: the + # head-ref map and the carried run cache. Any error leaves none, and + # the reader reads GitHub instead. + - name: Read the head refs the last gate run kept + if: steps.lane-token.outputs.token != '' + continue-on-error: true + env: + ACTIONS_TOKEN: ${{ github.token }} + HEAD_REFS_PATH: ${{ runner.temp }}/head-refs-in.json + run: node .base/.github/scripts/pr-merge/head-ref-map.mjs read + + - name: Restore the verdict reads the last gate run kept + if: steps.lane-token.outputs.token != '' + continue-on-error: true + timeout-minutes: 2 + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ${{ runner.temp }}/pr-merge-carried.json + key: pr-merge-run-cache-${{ github.run_id }}-${{ github.run_attempt }} + restore-keys: pr-merge-run-cache- + + - name: Start the run cache from them + if: steps.lane-token.outputs.token != '' + continue-on-error: true + env: + CARRY_FROM: ${{ runner.temp }}/pr-merge-carried.json + CARRY_TO: ${{ runner.temp }}/babysit-run-cache.json + run: node .base/.github/scripts/pr-merge/carry-run-cache.mjs + # While merge.report-only is anything but false it reports each # decision and writes nothing. - name: Arm and disarm auto-merge if: steps.lane-token.outputs.token != '' - timeout-minutes: 3 + timeout-minutes: 10 env: GH_TOKEN: ${{ steps.lane-token.outputs.token }} KILL_SWITCH: ${{ steps.kill.outputs.proceed == 'true' && 'proceed' || '' }} RESOLVED_CONFIG: ${{ runner.temp }}/resolved-config.json + BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }} + 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 run: | set -euo pipefail config=.base/docs/conventions/pr-pipeline.yaml REPORT_ONLY=$(yq '.merge."report-only"' "$config") LANES_BOT_LOGIN=$(yq '.merge."agent-identity" // ""' "$config") - export REPORT_ONLY LANES_BOT_LOGIN + MAX_VERIFY_FAILS=$(yq '.merge."max-verify-fails" // ""' "$config") + export REPORT_ONLY LANES_BOT_LOGIN MAX_VERIFY_FAILS node .base/.github/scripts/pr-merge/enable-auto-merge.mjs - name: Revoke the lane token diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 097fa65b3e..828d4a5264 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3110,7 +3110,8 @@ jobs: # of own-gate retries, 7 s of status-write retries and the 90 s # `rerun-wait-seconds` wait, about four and a half minutes, plus up to # 15 s of retry sleeps for each of the hold-lift step's three reads, the - # review-lane step's four and the lanes-hold step's eight, about four + # review-lane step's four and the lanes-hold step's eight (in the merge + # queue up to four verdict lookups and their run reads more), about four # minutes, plus the cost-hold step's reads: three for most pull requests, # which stop at the label events; for a lane-labeled one whose body cites a # decision, about ten, and two more per review run since the window opened. @@ -3838,6 +3839,17 @@ 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. + # 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 + # exact name, with and without the head-branch digest, stopping at the + # first such run: 2 reads for a plain-named pass, at most 4 lookups plus + # one run read per artifact found. The same approval of the head lifts + # it. Only in the queue: pr-verify runs only after a successful + # pull_request run of this workflow, so a red ci-status there would + # never get a verdict, and a pull request event reads nothing for it. + # A push after arming re-enters the queue, which checks the new head. + # FAILs and the repeated-FAIL cap are enable-auto-merge.mjs's. # 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') }} @@ -3922,14 +3934,44 @@ jobs: rows=$(read_api "the $kind of #$pr" "repos/$GITHUB_REPOSITORY/$kind?per_page=100" --paginate --jq '.[] | @json') || exit 1 [ -z "$rows" ] || printf '%s\n' "$rows" >>"$RUNNER_TEMP/discussion.jsonl" done + verdicts=() + if [ "$GITHUB_EVENT_NAME" = merge_group ]; then + ref=$(jq -r '.head.ref // ""' <<<"$pull") + if [ -z "$ref" ]; then + echo "::error::the head branch of #$pr could not be read." + exit 1 + fi + digest=$(printf '%s' "$ref" | sha256sum | cut -c1-16) + : >"$RUNNER_TEMP/verdicts.jsonl" + for name in "pr-verify-$pr-$head-pass" "pr-verify-$pr-$head-pass-$digest" \ + "pr-verify-$pr-$head-pass-notes" "pr-verify-$pr-$head-pass-notes-$digest"; do + 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 + record=$(read_api "run $run" "repos/$GITHUB_REPOSITORY/actions/runs/$run") || exit 1 + jq -c --arg name "$name" --arg verdict "$verdict" '{name: $name, verdict: $verdict, run: .}' <<<"$record" >>"$RUNNER_TEMP/verdicts.jsonl" || exit 1 + done + # A completed, successful run ends the lookup; merge-holds.mjs + # checks it is pr-verify's. + ! jq -se 'any(.run.status == "completed" and .run.conclusion == "success")' "$RUNNER_TEMP/verdicts.jsonl" >/dev/null || break + done + verdicts=("$RUNNER_TEMP/verdicts.jsonl") + 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") || { + "$RUNNER_TEMP/files.jsonl" "$RUNNER_TEMP/denied-paths.json" "$RUNNER_TEMP/trusted-actors.json" "$head" "${verdicts[@]}") || { printf '%s\n' "$out" exit 1 } # Exit 0 without the module's no-hold line ran no check: fail closed. printf '%s\n' "$out" grep -qxF "#$pr: no body hold, no member veto and no hold path." <<<"$out" || { echo "::error::merge-holds.mjs exited 0 without its verdict."; exit 1; } + if [ "${#verdicts[@]}" -gt 0 ] && ! grep -qE "^#$pr: no verifier hold: " <<<"$out"; then + echo "::error::merge-holds.mjs exited 0 without its verifier verdict." + exit 1 + fi # ORDERED SKIPS ARE CHECKED HERE. A test lane `test-`, or `lint-shell` # on its `run_shell` row, skips as a job when `select-tests`'s `run_` # row says the change gives it no work. Its diff --git a/.github/workflows/pr-verify.yml b/.github/workflows/pr-verify.yml index a5f09d6a33..0f0993dc17 100644 --- a/.github/workflows/pr-verify.yml +++ b/.github/workflows/pr-verify.yml @@ -7,8 +7,10 @@ name: pr-verify # Only steps of a run of this file on main can upload that run's artifacts, so # the pr-merge lane counts a verdict only through its run record # (plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py). -# The verdict is not a check run and stays out of ci-status -# (docs/conventions/pr-pipeline/README.md, "Verifier verdict"). +# The verdict is not a check run. ci-status reads a pass by exact name only in +# the merge queue, for a lanes App pull request, since a red ci-status on a +# pull request event would stop this workflow from running +# (docs/conventions/pr-pipeline/README.md, "Native auto-merge"). # # No pull request code runs here. The diff, the trusted PR text and the linked # issues are data files in pr-verify-data under the working directory, and the diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 46ddfdaddf..b98f49e8e4 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1049,7 +1049,12 @@ opens with `Closes #`, and carries no hold: no `Hold:` line or "do not me (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. It turns auto-merge off on an armed one that now fails +`merge.diff-check.denied-paths`, read on the files the head changes at each run, and whose +verifier verdict at that head the pr-merge gate would take: `babysit_verify_evidence.py evidence` +(the gate's reader, with its saved listing, head-ref map and run cache) refuses nothing, so a +PASS or PASS-NOTES of [pr-verify](#verifier-verdict) stands at the head, no FAIL is at it, and +FAILs sit at fewer than `merge.max-verify-fails` heads. It is read last, only for a pull request +every other rule would arm or keep armed. 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 armed right after report-outcome opens it; a listed human's edit, unlabel or ready wakes it too, so @@ -1076,6 +1081,17 @@ resolve-config always denies, so a lanes App pull request cannot weaken the rule `ci-status` at once. A comment does not, so a veto posted while armed takes effect when the queue builds the group, and after a person lifts one with a later comment, `ci-status` needs a re-run. +In the merge queue the same step also requires a verifier pass at the queued pull request's live +head: an unexpired `pr-verify---pass` or `-pass-notes` artifact, with or without the +head-branch digest, looked up by exact name and uploaded by a completed, successful +`workflow_run` run of `pr-verify.yml` on main here (`verdictHold` in `merge-holds.mjs`). A push +after arming keeps auto-merge armed, so the queue's check on the new head is what holds it until +pr-verify passes that head. The same trusted approval of the head lifts it, the path for a pull +request the gate sends to a person. It runs only in the queue: pr-verify runs only after a +successful `pull_request` run of `pr-require-checks`, so a red `ci-status` there would never get a +verdict, and a pull request event makes no verdict read. FAILs and the repeated-FAIL cap are +`enable-auto-merge.mjs`'s, which disarms such a pull request on its next run. + ### Dependabot pull requests No Dependabot pull request waits on a human 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 7f8352f414..998a0ddcce 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 @@ -87,16 +87,23 @@ steps can upload (.github/scripts/pr-merge/head-ref-map.mjs). An entry ties or unties the FAIL as the download would; a missing or malformed map is no map, and the FAIL is downloaded. + +`python3 babysit_verify_evidence.py evidence --repo R [--max-fails N] +` prints `Evidence.report()` for one pull request as one JSON line; +.github/scripts/pr-merge/enable-auto-merge.mjs arms native auto-merge only on +its null `refusal` at the head it read. """ from __future__ import annotations +import argparse import contextlib import hashlib import io import json import os import re +import sys import tempfile import time import zipfile @@ -1246,3 +1253,29 @@ def queued_hold(self, number: int) -> str | None: return None head = str(json_object(pr.get("head")).get("sha")) return self.evidence(number, head, pr).refusal + + +def main(argv: list[str] | None = None) -> int: + """`evidence `: one pull request's evidence at `head` as + one JSON line (`Evidence.report`), as pr-merge's gate judges it. + enable-auto-merge.mjs arms native auto-merge only on a null `refusal` + at the head it read. Exit 0 whenever the line is printed, a refusal + included.""" + parser = argparse.ArgumentParser(prog="babysit_verify_evidence.py") + sub = parser.add_subparsers(dest="command", required=True) + cmd = sub.add_parser("evidence") + cmd.add_argument("--repo", required=True) + cmd.add_argument("--branch", default="main") + cmd.add_argument("--max-fails", type=int, default=merge_policy.MAX_VERIFY_FAILS) + cmd.add_argument("number", type=int) + cmd.add_argument("head") + args = parser.parse_args(argv) + if args.max_fails < 1 or not merge_policy.SHA_RE.fullmatch(args.head): + parser.error("--max-fails must be 1 or more and head a full commit SHA") + reader = Reader(args.repo, args.branch, max_fails=args.max_fails) + print(json.dumps(reader.evidence(args.number, args.head).report())) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) 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 714919131d..49a48aaf67 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 @@ -21,6 +21,7 @@ from __future__ import annotations +import contextlib import dataclasses import hashlib import io @@ -3591,5 +3592,45 @@ def test_the_next_sweep_downloads_only_the_new_fails(self) -> None: self.assertEqual(after[ZIP_KIND], 6) + +class EvidenceCommandTest(unittest.TestCase): + """`evidence ` prints the gate's judgment for + enable-auto-merge.mjs, which arms only on a null refusal.""" + + def run_main(self, argv: list[str], evidence: ve.Evidence) -> tuple[dict[str, Any], ve.Reader]: + seen: list[ve.Reader] = [] + + def judge(reader: ve.Reader, number: int, head: str, pr: Any = None) -> ve.Evidence: + seen.append(reader) + self.assertEqual((number, head, pr), (PR, HEAD, None)) + return evidence + + out = io.StringIO() + with mock.patch.object(ve.Reader, "evidence", judge), contextlib.redirect_stdout(out): + self.assertEqual(ve.main(argv), 0) + return json.loads(out.getvalue()), seen[0] + + def test_a_pass_prints_a_null_refusal_under_the_given_cap(self) -> None: + report, reader = self.run_main( + ["evidence", "--repo", REPO, "--max-fails", "2", str(PR), HEAD], + ve.Evidence(None, HEAD, (ve.Verdict(HEAD, "pass", 1),), 0), + ) + self.assertEqual((report["refusal"], report["head"]), (None, HEAD)) + self.assertEqual((reader.repo, reader.branch, reader.max_fails), (REPO, "main", 2)) + + def test_a_repeated_fail_prints_its_refusal(self) -> None: + report, _ = self.run_main( + ["evidence", "--repo", REPO, str(PR), HEAD], + ve.Evidence(ve.REPEATED_FAIL, HEAD, (), 0, "fails at 3 heads"), + ) + self.assertEqual(report["refusal"], ve.REPEATED_FAIL) + self.assertTrue(report["needsHuman"]) + + def test_a_short_head_or_a_zero_cap_is_refused(self) -> None: + for argv in (["evidence", "--repo", REPO, str(PR), "abc"], ["evidence", "--repo", REPO, "--max-fails", "0", str(PR), HEAD]): + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit): + ve.main(argv) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index 1e4f5f467b..df7fb2fe06 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -88,7 +88,10 @@ fi # set. A files read prints each row of STUB_FILES (a JSON array of # [path, renamed-from?] arrays) as one line. A contents read of # trusted-actors.json prints the working tree's list, or answers 404 when -# STUB_NO_ACTORS is set. +# 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, as the +# step's --jq does; a run read prints that id's entry of STUB_RUNS, or answers +# 404. mkdir -p "$TMP_ROOT/bin" cat >"$TMP_ROOT/bin/gh" <<'EOF' #!/usr/bin/env bash @@ -114,6 +117,15 @@ case "$*" in */issues/*/comments\?*) discussion; rows "${STUB_ISSUE:-}" ;; */pulls/*/reviews\?*) discussion; rows "${STUB_REVIEWS:-}" ;; */pulls/*/comments\?*) discussion; rows "${STUB_INLINE:-}" ;; +*/actions/artifacts\?name=*) + all="$*" name=${all##*actions/artifacts?name=} + name=${name%%&*} + jq -r --arg name "$name" '.[$name][]?' <<<"${STUB_ARTIFACTS:-{\}}" + ;; +*/actions/runs/*) + all="$*" id=${all##*actions/runs/} + jq -ce --arg id "$id" '.[$id] // error' <<<"${STUB_RUNS:-{\}}" 2>/dev/null || { echo 'gh: Not Found (HTTP 404)' >&2; exit 1; } + ;; */pulls/*) printf '%s\n' "$STUB_PULL" ;; *) exit 1 ;; esac @@ -126,7 +138,7 @@ BASE=89abcdef0123456789abcdef0123456789abcdef QUEUE="refs/heads/gh-readonly-queue/main/pr-7-$SHA" LANES='{"id":337595936,"login":"melodic-automation-lanes[bot]","type":"Bot"}' pull() { jq -nc --argjson user "${2:-$LANES}" --arg body "$1" --arg ref "${STUB_BASE_REF:-main}" --arg sha "${STUB_BASE_SHA:-$BASE}" \ - --argjson changed "${STUB_CHANGED:-1}" '{number: 7, user: $user, body: $body, head: {sha: "'"$SHA"'"}, base: {ref: $ref, sha: $sha}, changed_files: $changed}'; } + --argjson changed "${STUB_CHANGED:-1}" '{number: 7, user: $user, body: $body, head: {sha: "'"$SHA"'", ref: "work-resolve-issue/issue-1"}, base: {ref: $ref, sha: $sha}, changed_files: $changed}'; } comment() { jq -nc --arg login "$1" --arg type "${2:-User}" --arg assoc "$3" --arg at "$4" --arg body "$5" \ '{user: {login: $login, type: $type}, author_association: $assoc, created_at: $at, body: $body}'; } # approval : a review of a commit. @@ -255,7 +267,84 @@ export STUB_NO_CONFIG=1 expect "an unreadable config fails closed" 1 0 "::error::could not read docs/conventions/pr-pipeline.yaml at $BASE" unset STUB_NO_CONFIG +# verifier runs: run [path] [event] [branch] [status] [conclusion] +run() { jq -nc --argjson id "$1" --arg path "${2:-.github/workflows/pr-verify.yml}" --arg event "${3:-workflow_run}" \ + --arg branch "${4:-main}" --arg status "${5:-completed}" --arg conclusion "${6:-success}" \ + '{id: $id, path: $path, event: $event, head_branch: $branch, status: $status, conclusion: $conclusion, + repository: {full_name: "o/r"}, head_repository: {full_name: "o/r"}}'; } +DIGEST="$(printf '%s' work-resolve-issue/issue-1 | sha256sum | cut -c1-16)" +OLD=fedcba9876543210fedcba9876543210fedcba98 +# lookups: the artifact-by-name reads the last run made. +lookups() { grep -c 'actions/artifacts?name=' "$TMP_ROOT/log"; } +STUB_RUNS="$(jq -sc 'map({(.id | tostring): .}) | add' <<<"$(run 11) +$(run 12 .github/workflows/pr-verify.yml workflow_run main in_progress '') +$(run 13 .github/workflows/other.yml) +$(run 14 .github/workflows/pr-verify.yml pull_request) +$(run 15 .github/workflows/pr-verify.yml workflow_run work-resolve-issue/issue-1) +$(run 16 .github/workflows/pr-verify.yml workflow_run main completed failure)")" +export STUB_RUNS + +STUB_PULL="$(pull 'Closes #1')" +expect "a pull request event reads no verdict" 0 3 "no hold path" +if [[ "$(lookups)" -eq 0 ]] && ! grep -q 'actions/runs/' "$TMP_ROOT/log"; then + ok "a pull request event makes no verdict read, so pr-verify can follow a green ci-status" +else + fail "a pull request event read a verdict: '$(sort -u "$TMP_ROOT/log")'" +fi + export EVENT=merge_group REF="$QUEUE" PR_NUMBER='' GROUP_BASE_SHA="$SHA" WANT_BASE="$SHA" +export STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass\":[11]}" +expect "in the merge queue, a pass at the head passes" 0 3 "#7: no verifier hold: pr-verify passed this head." +if [[ "$(lookups)" -eq 1 ]]; then + ok "a plain-named pass ends the lookup after one name" +else + fail "a plain-named pass took $(lookups) lookups" +fi +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='{}' +expect "in the merge queue, no verdict fails the group" 1 3 "::error::#7: pr-verify has not passed this head; wait for pr-verify to pass it" +if [[ "$(lookups)" -eq 4 ]]; then + ok "no verdict looks up each pass name once" +else + fail "no verdict took $(lookups) lookups" +fi +STUB_ARTIFACTS="{\"pr-verify-7-$OLD-pass\":[11],\"pr-verify-7-$SHA-fail\":[11]}" +expect "in the merge queue, a pass at an older head and a FAIL at this one fail the group" 1 3 "pr-verify has not passed this head" +STUB_ARTIFACTS="{\"pr-verify-8-$SHA-pass\":[11]}" +expect "in the merge queue, another pull request's pass fails the group" 1 3 "pr-verify has not passed this head" +for bad in 12:in-progress 13:another-workflow 14:a-pull-request-event 15:another-branch 16:a-failed-run 99:an-unreadable-run; do + STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass\":[${bad%%:*}]}" + if [[ ${bad%%:*} == 99 ]]; then + expect "in the merge queue, a pass from ${bad#*:} fails closed" 1 3 "could not read run 99" + else + expect "in the merge queue, a pass from ${bad#*:} fails the group" 1 3 "pr-verify has not passed this head" + fi +done +STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass\":[16],\"pr-verify-7-$SHA-pass-$DIGEST\":[11]}" +expect "in the merge queue, a failed run's pass does not hide a later name's good one" 0 3 "pr-verify passed this head" +STUB_ARTIFACTS='{}' 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 missing pass" 0 3 "no verifier hold: kyle-sexton approved this head" +STUB_REVIEWS="[$(approval kyle-sexton 153232337 APPROVED 2026-10-23T11:00:00Z "$OLD")]" +expect "in the merge queue, an approval of an earlier head lifts nothing" 1 3 "pr-verify has not passed this head" +STUB_REVIEWS='' +# Armed at a passing head, then pushed: the queue judges the new head, whose +# names no verdict carries yet. +STUB_ARTIFACTS="{\"pr-verify-7-$OLD-pass\":[11]}" +expect "in the merge queue, a push after a pass fails the group on the new head" 1 3 "pr-verify has not passed this head" +STUB_PULL="$(pull 'Closes #1' '{"id":153232337,"login":"kyle-sexton","type":"User"}')" +expect "in the merge queue, a person's pull request reads no verdict" 0 0 "not the lanes App's" +if [[ "$(lookups)" -eq 0 ]]; then + ok "a person's pull request makes no verdict lookup" +else + fail "a person's pull request made $(lookups) lookups" +fi +STUB_PULL="$(pull 'Closes #1')" +printf 'console.log("#7: no body hold, no member veto and no hold path.");\n' >"$TMP_ROOT/old.mjs" +STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass\":[11]}" STUB_MODULE="$TMP_ROOT/old.mjs" +expect "in the merge queue, a base module that ignores the verdicts fails closed" 1 3 "::error::merge-holds.mjs exited 0 without its verifier verdict" +unset STUB_MODULE +STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass\":[11]}" STUB_PULL="$(pull $'Closes #1\nHold: owner call')" expect "in the merge queue, a body hold fails the group" 1 3 "::error::#7: its body holds it" STUB_PULL="$(pull 'Closes #1')" STUB_ISSUE="[$KYLE_VETO]" From b3277604dd10e6400698c15ac4277b1986a5eab9 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:51:59 -0400 Subject: [PATCH 02/11] chore(pr-merge): add changelog fragments for the native-merge verdict fix Co-Authored-By: Claude Opus 5.5 --- .../fix-native-merge-requires-verdict-7c41e2a9.md | 7 +++++++ .../fix-native-merge-requires-verdict-3d9b06f1.md | 7 +++++++ 2 files changed, 14 insertions(+) create mode 100644 .changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md create mode 100644 .changes/source-control/fix-native-merge-requires-verdict-3d9b06f1.md diff --git a/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md b/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md new file mode 100644 index 0000000000..e09197175d --- /dev/null +++ b/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- Native auto-merge of a lanes App pull request now needs the verifier verdict pr-merge's gate needs. `enable-auto-merge.mjs` arms, and keeps armed, only when the gate's reader refuses nothing at the head it read (a pr-verify pass stands, no FAIL at the head, FAILs under `merge.max-verify-fails`), and disarms otherwise. In the merge queue, `ci-status` requires a pass or pass-notes artifact at the queued head from a successful pr-verify run on main; a trusted approval of the head lifts it. Pull request events read no verdict, since pr-verify runs only after a green pull request run. diff --git a/.changes/source-control/fix-native-merge-requires-verdict-3d9b06f1.md b/.changes/source-control/fix-native-merge-requires-verdict-3d9b06f1.md new file mode 100644 index 0000000000..17181be53c --- /dev/null +++ b/.changes/source-control/fix-native-merge-requires-verdict-3d9b06f1.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Added + +- `babysit_verify_evidence.py evidence --repo R [--max-fails N] ` prints the gate's verifier evidence for one pull request as one JSON line, which the pr-merge lane's native auto-merge arms on. From 09c0c851bed39ec131b3e1e2b3b42c505e0b131a Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:55:42 -0400 Subject: [PATCH 03/11] test(pr-merge): run the step's own jq filter and tie verdictHold's workflow path to the resolver's Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/enable-auto-merge.mjs | 2 +- .github/scripts/pr-merge/enable-auto-merge.test.mjs | 6 ++++++ .github/workflows/pr-require-checks.yml | 3 ++- docs/conventions/pr-pipeline/README.md | 6 +++--- scripts/ci-lanes-pr-hold.test.sh | 13 +++++++++---- 5 files changed, 21 insertions(+), 9 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index b4c6738333..a2ed85a071 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -30,7 +30,7 @@ // FAILs at fewer than merge.max-verify-fails heads. It is read last, only // for a pull request every other rule would arm or keep armed. The merge // queue's ci-status checks the pass again at the head it merges -// (merge-holds.mjs verdictHold), since a push after arming keeps it armed. +// (merge-holds.mjs verdictHold), whatever a push after arming does. // 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 diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index c43b5bbe46..2b2f24eb66 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -16,6 +16,7 @@ import { test } from "node:test"; import { ARM_MUTATION, BODY_HOLD, DETAIL_QUERY, DISARM_MUTATION, HUMAN_VETO, PULLS_QUERY, decide, main, verifyHold } from "./enable-auto-merge.mjs"; import { VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; +import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; import { decide as decideOutcome } from "../work-resolve-issue/report-outcome.mjs"; const REPO = "melodic-software/claude-code-plugins"; @@ -472,3 +473,8 @@ test("verdictHold counts a pass only from a successful pr-verify run on main", ( assert.match(hold(null), /could not be read/); assert.match(verdictHold([], "not a repo"), /could not be read/); }); + +// test_verify_rules_sync.py ties the resolver's path to the gate's WORKFLOW_PATH. +test("verdictHold names the verifier workflow the resolver and the gate name", () => { + assert.equal(VERIFY_PATH, RESOLVER_VERIFY_PATH); +}); diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 828d4a5264..55f7dc0c92 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3848,7 +3848,8 @@ jobs: # it. Only in the queue: pr-verify runs only after a successful # pull_request run of this workflow, so a red ci-status there would # never get a verdict, and a pull request event reads nothing for it. - # A push after arming re-enters the queue, which checks the new head. + # Whatever a push after arming does to the request, the queue checks + # the head it merges. # FAILs and the repeated-FAIL cap are enable-auto-merge.mjs's. # 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 diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index b98f49e8e4..f8abe1153e 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1084,9 +1084,9 @@ builds the group, and after a person lifts one with a later comment, `ci-status` In the merge queue the same step also requires a verifier pass at the queued pull request's live head: an unexpired `pr-verify---pass` or `-pass-notes` artifact, with or without the head-branch digest, looked up by exact name and uploaded by a completed, successful -`workflow_run` run of `pr-verify.yml` on main here (`verdictHold` in `merge-holds.mjs`). A push -after arming keeps auto-merge armed, so the queue's check on the new head is what holds it until -pr-verify passes that head. The same trusted approval of the head lifts it, the path for a pull +`workflow_run` run of `pr-verify.yml` on main here (`verdictHold` in `merge-holds.mjs`). Whatever +GitHub does with an armed request when the head moves, the queue's check on the head it merges +holds a head pr-verify has not passed. The same trusted approval of the head lifts it, the path for a pull request the gate sends to a person. It runs only in the queue: pr-verify runs only after a successful `pull_request` run of `pr-require-checks`, so a red `ci-status` there would never get a verdict, and a pull request event makes no verdict read. FAILs and the repeated-FAIL cap are diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index df7fb2fe06..9e6cf520e5 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -89,9 +89,9 @@ fi # [path, renamed-from?] arrays) as one line. A contents read of # 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, as the -# step's --jq does; a run read prints that id's entry of STUB_RUNS, or answers -# 404. +# 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. mkdir -p "$TMP_ROOT/bin" cat >"$TMP_ROOT/bin/gh" <<'EOF' #!/usr/bin/env bash @@ -120,7 +120,12 @@ case "$*" in */actions/artifacts\?name=*) all="$*" name=${all##*actions/artifacts?name=} name=${name%%&*} - jq -r --arg name "$name" '.[$name][]?' <<<"${STUB_ARTIFACTS:-{\}}" + # 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}}])}' \ + <<<"${STUB_ARTIFACTS:-{\}}" | jq -r "${*: -1}" ;; */actions/runs/*) all="$*" id=${all##*actions/runs/} From 91e9eb490dc9caf23255a0e35af1c6d3cc97b7dd Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:56:50 -0400 Subject: [PATCH 04/11] test(pr-merge): pin enable-auto-merge's actions read and verdict reader env Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/workflow.test.mjs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 40cd2edfd8..52a2e3ee1f 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -177,8 +177,20 @@ 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", "id-token: write"], + ["contents: read", "actions: 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 + // carried run cache. + for (const line of [ + "BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }}", + "BABYSIT_VERIFY_HEAD_REFS: ${{ runner.temp }}/head-refs-in.json", + "BABYSIT_RUN_CACHE: ${{ runner.temp }}/babysit-run-cache.json", + ]) { + assert.ok(arm.includes(`\n ${line}\n`), line); + } + assert.match(arm, /node \.base\/\.github\/scripts\/pr-merge\/head-ref-map\.mjs read\n/); + assert.match(arm, /MAX_VERIFY_FAILS=\$\(yq '\.merge\."max-verify-fails" \/\/ ""' "\$config"\)\n/); }); // The dispatch POST's retry, run under bash against a stub gh that answers From ca7ab6e5cf790a8336fdfa7bb7ee7ee37dab8847 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:00:11 -0400 Subject: [PATCH 05/11] fix(pr-merge): read the full verdict reader only on the scheduled run Event runs of enable-auto-merge look the head's own pass up by exact artifact name (2 to 4 lookups and a run read per pull request), the same lookup and rule as ci-status in the merge queue. Only the scheduled run spends babysit_verify_evidence.py, which adds the FAIL and repeated-FAIL refusals, and only it reads the head-ref map and run cache. Co-Authored-By: Claude Opus 5.5 --- ...-native-merge-requires-verdict-7c41e2a9.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 79 +++++++++++--- .../pr-merge/enable-auto-merge.test.mjs | 102 +++++++++++++++++- .github/scripts/pr-merge/workflow.test.mjs | 3 + .github/workflows/pr-merge.yml | 23 ++-- docs/conventions/pr-pipeline/README.md | 16 +-- 6 files changed, 190 insertions(+), 35 deletions(-) diff --git a/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md b/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md index e09197175d..e9ef9023f2 100644 --- a/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md +++ b/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- Native auto-merge of a lanes App pull request now needs the verifier verdict pr-merge's gate needs. `enable-auto-merge.mjs` arms, and keeps armed, only when the gate's reader refuses nothing at the head it read (a pr-verify pass stands, no FAIL at the head, FAILs under `merge.max-verify-fails`), and disarms otherwise. In the merge queue, `ci-status` requires a pass or pass-notes artifact at the queued head from a successful pr-verify run on main; a trusted approval of the head lifts it. Pull request events read no verdict, since pr-verify runs only after a green pull request run. +- Native auto-merge of a lanes App pull request now needs the verifier verdict pr-merge's gate needs. `enable-auto-merge.mjs` arms, and keeps armed, only with a pr-verify pass at the head it read, and disarms otherwise: an event run looks the head's pass up by exact artifact name, and the scheduled run asks the gate's reader, which also refuses a FAIL at the head and FAILs at `merge.max-verify-fails` or more heads. In the merge queue, `ci-status` requires a pass or pass-notes artifact at the queued head from a successful pr-verify run on main; a trusted approval of the head lifts it. Pull request events read no verdict, since pr-verify runs only after a green pull request run. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index a2ed85a071..fb7c37f049 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -24,13 +24,20 @@ // group, so a push after arming is held without waiting for this job. // - 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's `evidence` refuses nothing (verifyHold), so -// a PASS or PASS-NOTES of pr-verify at that head stands, no FAIL at it, and -// FAILs at fewer than merge.max-verify-fails heads. It is read last, only -// for a pull request every other rule would arm or keep armed. The merge -// queue's ci-status checks the pass again at the head it merges -// (merge-holds.mjs verdictHold), whatever a push after arming does. +// - A verifier pass at the head just read. It is read last, only for a pull +// request every other rule would arm or keep armed, and how depends on the +// run (verifyFor): +// - an event run (the issue lane, a person's edit, unlabel or ready) looks +// the head's own PASS or PASS-NOTES up by exact artifact name +// (headPassVerify, the same lookup and rule as ci-status's merge-queue +// check, merge-holds.mjs verdictHold): 2 to 4 lookups and a run read; +// - the scheduled run asks pr-merge's own reader (pythonVerify, +// babysit_verify_evidence.py `evidence`), which also refuses a FAIL at +// the head, an earlier FAIL not cleared and FAILs at merge.max-verify-fails +// or more heads, sharing one artifact listing and run cache in the run. +// A pull request that goes over the cap between scheduled runs can stay +// armed until the next one; the merge queue's ci-status checks the pass +// again at the head it merges, whatever a push after arming does. // 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 @@ -45,9 +52,11 @@ // TRACKER_CONFIG the base .work-item-tracker.json, for the human-gated // label (default .base/.work-item-tracker.json) // RESOLVED_CONFIG the resolve-config output, for the hold paths +// EVENT_NAME the run's event; `schedule` reads with the full reader // MAX_VERIFY_FAILS merge.max-verify-fails (default: the reader's) -// BABYSIT_VERIFY_*, BABYSIT_RUN_CACHE the reader's Actions token, saved -// listing, head-ref map and run cache, as the gate's +// BABYSIT_VERIFY_GH_TOKEN the job's GITHUB_TOKEN, for the Actions reads +// BABYSIT_VERIFY_*, BABYSIT_RUN_CACHE the reader's saved listing, head-ref +// map and run cache, as the gate's // GITHUB_API_URL import { spawnSync } from "node:child_process"; import process from "node:process"; @@ -55,8 +64,9 @@ import { pathToFileURL } from "node:url"; import { createGitHub } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; +import { headRefDigest } from "../pr-verify/write-verdict.mjs"; import { readHoldPaths } from "../work-resolve-issue/report-outcome.mjs"; -import { BODY_HOLD, pathHold, vetoers } from "./merge-holds.mjs"; +import { BODY_HOLD, pathHold, verdictHold, vetoers } from "./merge-holds.mjs"; export { BODY_HOLD, HUMAN_VETO } from "./merge-holds.mjs"; @@ -79,7 +89,7 @@ export const PULLS_QUERY = `query($owner: String!, $name: String!, $after: Strin pullRequests(states: OPEN, baseRefName: "main", first: 100, after: $after) { pageInfo { hasNextPage endCursor } nodes { - id number isDraft isCrossRepository headRefOid body + id number isDraft isCrossRepository headRefOid headRefName body author { __typename login } labels(first: 100) { totalCount nodes { name } } autoMergeRequest { enabledAt } @@ -182,10 +192,41 @@ export function verifyHold(evidence, head) { return evidence.refusal === null ? null : `pr-merge's gate refuses its verifier verdict (${escapeCommand(evidence.refusal)})`; } +// The head's own PASS or PASS-NOTES, looked up by exact artifact name with and +// without the head-branch digest, as ci-status does in the merge queue, under +// the Actions token: one lookup per name until a completed, successful run, +// one run read per unexpired artifact found. The answer is a report shaped +// like the reader's. +export function headPassVerify(actions, repo) { + const verify = async ({ number, headRefOid: head, headRefName: ref }) => { + const digest = headRefDigest(String(ref ?? "")); + const records = []; + for (const verdict of ["pass", "pass-notes"]) { + for (const name of [`pr-verify-${number}-${head}-${verdict}`, `pr-verify-${number}-${head}-${verdict}-${digest}`]) { + const listed = await actions("GET", `/repos/${repo}/actions/artifacts?name=${encodeURIComponent(name)}&per_page=100`); + if (!Array.isArray(listed?.artifacts)) { + throw new Error(`the ${name} artifacts did not read`); + } + for (const artifact of listed.artifacts) { + const id = artifact?.workflow_run?.id; + if (artifact?.name === name && artifact.expired === false && Number.isSafeInteger(id)) { + records.push({ name, verdict, run: await actions("GET", `/repos/${repo}/actions/runs/${id}`) }); + } + } + if (records.some(({ run }) => run?.status === "completed" && run.conclusion === "success")) { + return { refusal: verdictHold(records, repo) === null ? null : "verify:missing", head }; + } + } + } + return { refusal: "verify:missing", head }; + }; + return Object.assign(verify, { mode: "head-pass" }); +} + // 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) { - return async (number, head) => { + const verify = async ({ number, headRefOid: head }) => { const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "evidence", "--repo", env.GITHUB_REPOSITORY]; if (env.MAX_VERIFY_FAILS) { args.push("--max-fails", String(env.MAX_VERIFY_FAILS)); @@ -196,6 +237,13 @@ export function pythonVerify(env) { } return JSON.parse(String(done.stdout).trim().split("\n").at(-1)); }; + return Object.assign(verify, { mode: "reader" }); +} + +// The scheduled run reads with the full reader; every event run with the +// head-pass lookup. +export function verifyFor(env, actions) { + return env.EVENT_NAME === "schedule" ? pythonVerify(env) : headPassVerify(actions, env.GITHUB_REPOSITORY); } // What to do with one listed pull request: { action: arm | disarm | none, reason }. @@ -259,7 +307,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, actions, verify } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -277,7 +325,8 @@ export async function main({ env = process.env, github, verify } = {}) { const humanGated = humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json"); const holdPaths = readHoldPaths(env.RESOLVED_CONFIG ?? ""); github ??= createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); - verify ??= pythonVerify(env); + actions ??= createGitHub({ token: env.BABYSIT_VERIFY_GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); + verify ??= verifyFor(env, actions); const pulls = await listPulls(github, repo[1], repo[2]); let failed = 0; for (const pull of pulls) { @@ -298,7 +347,7 @@ export async function main({ env = process.env, github, verify } = {}) { if (action === "verify") { let evidence = null; try { - evidence = await verify(pull.number, pull.headRefOid); + evidence = await verify(pull); } 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 2b2f24eb66..66d272d1a8 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -14,7 +14,20 @@ import process from "node:process"; import { fileURLToPath } from "node:url"; import { test } from "node:test"; -import { ARM_MUTATION, BODY_HOLD, DETAIL_QUERY, DISARM_MUTATION, HUMAN_VETO, PULLS_QUERY, decide, main, verifyHold } from "./enable-auto-merge.mjs"; +import { + ARM_MUTATION, + BODY_HOLD, + DETAIL_QUERY, + DISARM_MUTATION, + HUMAN_VETO, + PULLS_QUERY, + decide, + headPassVerify, + main, + verifyFor, + verifyHold, +} from "./enable-auto-merge.mjs"; +import { headRefDigest } from "../pr-verify/write-verdict.mjs"; import { VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; import { decide as decideOutcome } from "../work-resolve-issue/report-outcome.mjs"; @@ -89,7 +102,7 @@ function world(pulls, { failArm = false } = {}) { // The gate's verifier evidence: a pull request's `verdict` (a report, or // an Error to throw), else a pass at its head. const verified = []; - const verify = async (number, head) => { + const verify = async ({ number, headRefOid: head }) => { verified.push(number); const found = pulls.find((candidate) => candidate.number === number); if (found?.verdict instanceof Error) { @@ -478,3 +491,88 @@ test("verdictHold counts a pass only from a successful pr-verify run on main", ( test("verdictHold names the verifier workflow the resolver and the gate name", () => { assert.equal(VERIFY_PATH, RESOLVER_VERIFY_PATH); }); + +// Event runs look the head's own pass up by exact name, as ci-status does in +// the merge queue; only the scheduled run spends the full reader. +test("the scheduled run reads with the gate's reader, every event run by exact name", () => { + const env = { GITHUB_REPOSITORY: REPO }; + assert.equal(verifyFor({ ...env, EVENT_NAME: "schedule" }, null).mode, "reader"); + for (const event of ["workflow_run", "pull_request_target", "workflow_dispatch", undefined]) { + assert.equal(verifyFor({ ...env, EVENT_NAME: event }, null).mode, "head-pass", String(event)); + } +}); + +function actionsWorld(artifacts, runs) { + const reads = []; + const actions = async (method, path) => { + reads.push(path); + assert.equal(method, "GET"); + const byName = /\/actions\/artifacts\?name=([^&]+)&per_page=100$/.exec(path); + if (byName) { + const name = decodeURIComponent(byName[1]); + return { + artifacts: [ + ...(artifacts[name] ?? []).map((id) => ({ name, expired: false, workflow_run: { id } })), + { name, expired: true, workflow_run: { id: 1 } }, + { name: `${name}-i1`, expired: false, workflow_run: { id: 1 } }, + ], + }; + } + const run = /\/actions\/runs\/([0-9]+)$/.exec(path); + if (run && runs[run[1]]) { + return runs[run[1]]; + } + throw new Error(`GET ${path} failed with status 404`); + }; + return { actions, reads }; +} + +test("headPassVerify passes on a successful pr-verify run's pass at the head, by exact name", async () => { + const good = { + path: VERIFY_PATH, + event: "workflow_run", + head_branch: "main", + status: "completed", + conclusion: "success", + repository: { full_name: REPO }, + head_repository: { full_name: REPO }, + }; + const runs = { 1: good, 11: good, 12: { ...good, conclusion: "failure" }, 13: { ...good, path: ".github/workflows/x.yml" } }; + const digest = headRefDigest("work-resolve-issue/issue-1"); + const target = pull(7, { headRefName: "work-resolve-issue/issue-1" }); + const check = async (artifacts) => { + const { actions, reads } = actionsWorld(artifacts, runs); + return { ...(await headPassVerify(actions, REPO)(target)), lookups: reads.filter((path) => path.includes("artifacts?name=")).length }; + }; + assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass`]: [11] }), { refusal: null, head: HEAD, lookups: 1 }); + assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass-notes-${digest}`]: [11] }), { refusal: null, head: HEAD, lookups: 4 }); + assert.deepEqual(await check({}), { refusal: "verify:missing", head: HEAD, lookups: 4 }); + assert.deepEqual(await check({ [`pr-verify-7-${"c".repeat(40)}-pass`]: [11] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); + assert.deepEqual(await check({ [`pr-verify-8-${HEAD}-pass`]: [11] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); + assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass`]: [12] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); + assert.equal((await check({ [`pr-verify-7-${HEAD}-pass`]: [13] })).refusal, "verify:missing"); + await assert.rejects(check({ [`pr-verify-7-${HEAD}-pass`]: [99] }), /404/); +}); + +test("an event run arms on the head's pass and leaves a head with none", async () => { + const { github, writes } = world([pull(1), pull(2)]); + const { actions } = actionsWorld({ [`pr-verify-1-${HEAD}-pass`]: [11] }, { + 11: { + path: VERIFY_PATH, + event: "workflow_run", + head_branch: "main", + status: "completed", + conclusion: "success", + repository: { full_name: REPO }, + head_repository: { full_name: REPO }, + }, + }); + const log = console.log; + console.log = () => {}; + try { + assert.equal(await main({ env: { ...ENV, EVENT_NAME: "workflow_run" }, github, actions }), 0); + } finally { + console.log = log; + } + assert.deepEqual(writes(), [{ mutation: "arm", id: "PR_1", head: HEAD }]); +}); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 52a2e3ee1f..f8245f6746 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -183,6 +183,7 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () // GITHUB_TOKEN for the Actions reads, with the run's head-ref map and // carried run cache. for (const line of [ + "EVENT_NAME: ${{ github.event_name }}", "BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }}", "BABYSIT_VERIFY_HEAD_REFS: ${{ runner.temp }}/head-refs-in.json", "BABYSIT_RUN_CACHE: ${{ runner.temp }}/babysit-run-cache.json", @@ -190,6 +191,8 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () assert.ok(arm.includes(`\n ${line}\n`), line); } assert.match(arm, /node \.base\/\.github\/scripts\/pr-merge\/head-ref-map\.mjs read\n/); + // Only the scheduled run spends the full reader, so only it reads the map. + assert.match(arm, /- name: Read the head refs the last gate run kept\n {8}if: steps\.lane-token\.outputs\.token != '' && github\.event_name == 'schedule'\n/); assert.match(arm, /MAX_VERIFY_FAILS=\$\(yq '\.merge\."max-verify-fails" \/\/ ""' "\$config"\)\n/); }); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 6b49a02ae4..4cc4bdd6a7 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1258,10 +1258,12 @@ jobs: # 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. It arms only on the verifier evidence pr-merge's gate - # would take at the head (babysit_verify_evidence.py, read with the job's - # GITHUB_TOKEN as the gate does, from the head-ref map and run cache the - # last gate run kept), so a FAIL or the repeated-FAIL cap disarms too. + # the broker refuses. It arms only on a pr-verify pass at the head, read + # with the job's GITHUB_TOKEN: an event run looks the head's own pass up by + # exact name (2 to 4 lookups and a run read per pull request), and the + # scheduled run asks the gate's reader (babysit_verify_evidence.py, from the + # head-ref map and run cache the last gate run kept), so a FAIL or the + # repeated-FAIL cap disarms on the schedule. # Its own job and token: the broker mints one token per # job. Rules: .github/scripts/pr-merge/enable-auto-merge.mjs. enable-auto-merge: @@ -1355,11 +1357,11 @@ jobs: issues: ${{ steps.config.outputs.issues }} superseded-ok: true - # The verdict reader's untied FAILs, as the gate job reads them: the - # head-ref map and the carried run cache. Any error leaves none, and - # the reader reads GitHub instead. + # The scheduled run's verdict reader ties untied FAILs as the gate job + # does: the head-ref map and the carried run cache. Any error leaves + # none, and the reader reads GitHub instead. Event runs need neither. - name: Read the head refs the last gate run kept - if: steps.lane-token.outputs.token != '' + if: steps.lane-token.outputs.token != '' && github.event_name == 'schedule' continue-on-error: true env: ACTIONS_TOKEN: ${{ github.token }} @@ -1367,7 +1369,7 @@ jobs: run: node .base/.github/scripts/pr-merge/head-ref-map.mjs read - name: Restore the verdict reads the last gate run kept - if: steps.lane-token.outputs.token != '' + if: steps.lane-token.outputs.token != '' && github.event_name == 'schedule' continue-on-error: true timeout-minutes: 2 uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 @@ -1377,7 +1379,7 @@ jobs: restore-keys: pr-merge-run-cache- - name: Start the run cache from them - if: steps.lane-token.outputs.token != '' + if: steps.lane-token.outputs.token != '' && github.event_name == 'schedule' continue-on-error: true env: CARRY_FROM: ${{ runner.temp }}/pr-merge-carried.json @@ -1393,6 +1395,7 @@ jobs: GH_TOKEN: ${{ steps.lane-token.outputs.token }} KILL_SWITCH: ${{ steps.kill.outputs.proceed == 'true' && 'proceed' || '' }} RESOLVED_CONFIG: ${{ runner.temp }}/resolved-config.json + EVENT_NAME: ${{ github.event_name }} BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }} BABYSIT_VERIFY_LISTING: ${{ runner.temp }}/verify-listing.json BABYSIT_VERIFY_HEAD_REFS: ${{ runner.temp }}/head-refs-in.json diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index f8abe1153e..4ebce1d5a3 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1049,12 +1049,14 @@ opens with `Closes #`, and carries no hold: no `Hold:` line or "do not me (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 the pr-merge gate would take: `babysit_verify_evidence.py evidence` -(the gate's reader, with its saved listing, head-ref map and run cache) refuses nothing, so a -PASS or PASS-NOTES of [pr-verify](#verifier-verdict) stands at the head, no FAIL is at it, and -FAILs sit at fewer than `merge.max-verify-fails` heads. It is read last, only for a pull request -every other rule would arm or keep armed. It turns auto-merge off on an armed one that now fails +`merge.diff-check.denied-paths`, read on the files the head changes at each run, and that has a +PASS or PASS-NOTES of [pr-verify](#verifier-verdict) at that head. That is read last, only for a +pull request every other rule would arm or keep armed. An event run looks the head's own pass up +by exact artifact name, as `ci-status` does in the merge queue (below). The scheduled run asks the +gate's reader instead (`babysit_verify_evidence.py evidence`, with its saved listing, head-ref map +and run cache), which also refuses a FAIL at the head, an earlier FAIL not cleared, and FAILs at +`merge.max-verify-fails` or more heads; a pull request that goes over the cap between scheduled +runs stays armed until the next one. 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 armed right after report-outcome opens it; a listed human's edit, unlabel or ready wakes it too, so @@ -1090,7 +1092,7 @@ holds a head pr-verify has not passed. The same trusted approval of the head lif request the gate sends to a person. It runs only in the queue: pr-verify runs only after a successful `pull_request` run of `pr-require-checks`, so a red `ci-status` there would never get a verdict, and a pull request event makes no verdict read. FAILs and the repeated-FAIL cap are -`enable-auto-merge.mjs`'s, which disarms such a pull request on its next run. +`enable-auto-merge.mjs`'s, which disarms such a pull request on its next scheduled run. ### Dependabot pull requests From b74356c9974cf5507c047a9ea8806b2be98ad796 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:10:58 -0400 Subject: [PATCH 06/11] fix(pr-merge): end the verdict lookup only at a pass pr-verify uploaded Review fixes. The merge-queue lookup and headPassVerify stopped at any successful run, so another workflow's artifact under one name could hide a real pass under a later name; both now stop only at a pass verdictHold counts. A hand-started run uses the full reader, an event run leaves an armed pull request armed instead of re-reading its verdict, and the reader stops starting new reads after seven minutes of the step's ten. Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-merge/enable-auto-merge.mjs | 37 ++++++++---- .../pr-merge/enable-auto-merge.test.mjs | 57 ++++++++++++++++++- .github/scripts/pr-merge/merge-holds.mjs | 6 +- .github/workflows/pr-require-checks.yml | 9 +-- docs/conventions/pr-pipeline/README.md | 13 +++-- scripts/ci-lanes-pr-hold.test.sh | 7 +++ 6 files changed, 104 insertions(+), 25 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index fb7c37f049..f9aa8052f8 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -31,13 +31,15 @@ // the head's own PASS or PASS-NOTES up by exact artifact name // (headPassVerify, the same lookup and rule as ci-status's merge-queue // check, merge-holds.mjs verdictHold): 2 to 4 lookups and a run read; -// - the scheduled run asks pr-merge's own reader (pythonVerify, +// - the scheduled run and a hand-started one ask pr-merge's own reader (pythonVerify, // babysit_verify_evidence.py `evidence`), which also refuses a FAIL at // the head, an earlier FAIL not cleared and FAILs at merge.max-verify-fails // or more heads, sharing one artifact listing and run cache in the run. -// A pull request that goes over the cap between scheduled runs can stay -// armed until the next one; the merge queue's ci-status checks the pass -// again at the head it merges, whatever a push after arming does. +// An event run arms on the pass alone: it does not see a FAIL at the head +// beside a re-run's pass, an earlier FAIL, the cap, or a spec changed since +// the judgment, and it leaves an armed pull request armed. The next reader +// run disarms such a one. The merge queue's ci-status checks the pass again +// at the head it merges, whatever a push after arming does. // 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 @@ -194,8 +196,8 @@ export function verifyHold(evidence, head) { // The head's own PASS or PASS-NOTES, looked up by exact artifact name with and // without the head-branch digest, as ci-status does in the merge queue, under -// the Actions token: one lookup per name until a completed, successful run, -// one run read per unexpired artifact found. The answer is a report shaped +// the Actions token: one lookup per name until one holds a pass verdictHold +// counts, one run read per unexpired artifact found. The answer is a report shaped // like the reader's. export function headPassVerify(actions, repo) { const verify = async ({ number, headRefOid: head, headRefName: ref }) => { @@ -213,8 +215,8 @@ export function headPassVerify(actions, repo) { records.push({ name, verdict, run: await actions("GET", `/repos/${repo}/actions/runs/${id}`) }); } } - if (records.some(({ run }) => run?.status === "completed" && run.conclusion === "success")) { - return { refusal: verdictHold(records, repo) === null ? null : "verify:missing", head }; + if (verdictHold(records, repo) === null) { + return { refusal: null, head }; } } } @@ -240,11 +242,17 @@ export function pythonVerify(env) { return Object.assign(verify, { mode: "reader" }); } -// The scheduled run reads with the full reader; every event run with the -// head-pass lookup. +// The scheduled run and a hand-started one, which a person starts to retry a +// pull request that looks stuck, read with the full reader; every other event +// run with the head-pass lookup. +export const READER_EVENTS = Object.freeze(["schedule", "workflow_dispatch"]); export function verifyFor(env, actions) { - return env.EVENT_NAME === "schedule" ? pythonVerify(env) : headPassVerify(actions, env.GITHUB_REPOSITORY); + return READER_EVENTS.includes(env.EVENT_NAME) ? pythonVerify(env) : headPassVerify(actions, env.GITHUB_REPOSITORY); } +// Reader time per run: past it, the rest of the pull requests are left for +// the next run rather than cut off by the step's timeout (judgment, inside +// the step's 10 minutes). +const VERIFY_BUDGET_MS = 7 * 60_000; // What to do with one listed pull request: { action: arm | disarm | none, reason }. // `detail` is its discussion; undefined means not read yet, and an arm or a @@ -328,6 +336,7 @@ export async function main({ env = process.env, github, actions, verify } = {}) actions ??= createGitHub({ token: env.BABYSIT_VERIFY_GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); verify ??= verifyFor(env, actions); const pulls = await listPulls(github, repo[1], repo[2]); + const deadline = Date.now() + VERIFY_BUDGET_MS; let failed = 0; for (const pull of pulls) { let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }); @@ -344,7 +353,11 @@ export async function main({ env = process.env, github, actions, verify } = {}) } ({ action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }, detail)); } - if (action === "verify") { + if (action === "verify" && pull.autoMergeRequest != null && verify.mode === "head-pass") { + ({ action, reason } = { action: "none", reason: "already armed; the next reader run checks its verdict" }); + } else if (action === "verify" && Date.now() > deadline) { + ({ action, reason } = { action: "none", reason: "no time left in this run to read its verifier verdict" }); + } else if (action === "verify") { let evidence = null; try { evidence = await verify(pull); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 66d272d1a8..ff348873b1 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -497,7 +497,8 @@ test("verdictHold names the verifier workflow the resolver and the gate name", ( test("the scheduled run reads with the gate's reader, every event run by exact name", () => { const env = { GITHUB_REPOSITORY: REPO }; assert.equal(verifyFor({ ...env, EVENT_NAME: "schedule" }, null).mode, "reader"); - for (const event of ["workflow_run", "pull_request_target", "workflow_dispatch", undefined]) { + assert.equal(verifyFor({ ...env, EVENT_NAME: "workflow_dispatch" }, null).mode, "reader"); + for (const event of ["workflow_run", "pull_request_target", undefined]) { assert.equal(verifyFor({ ...env, EVENT_NAME: event }, null).mode, "head-pass", String(event)); } }); @@ -551,6 +552,13 @@ test("headPassVerify passes on a successful pr-verify run's pass at the head, by assert.deepEqual(await check({ [`pr-verify-8-${HEAD}-pass`]: [11] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass`]: [12] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); assert.equal((await check({ [`pr-verify-7-${HEAD}-pass`]: [13] })).refusal, "verify:missing"); + // Another workflow's successful run under one name does not end the lookup + // before a later name's real pass. + assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass`]: [13], [`pr-verify-7-${HEAD}-pass-${digest}`]: [11] }), { + refusal: null, + head: HEAD, + lookups: 2, + }); await assert.rejects(check({ [`pr-verify-7-${HEAD}-pass`]: [99] }), /404/); }); @@ -576,3 +584,50 @@ test("an event run arms on the head's pass and leaves a head with none", async ( } assert.deepEqual(writes(), [{ mutation: "arm", id: "PR_1", head: HEAD }]); }); + +test("an event run leaves an armed pull request armed without reading its verdict", async () => { + const { github, writes } = world([pull(1, { autoMergeRequest: { enabledAt: "2026-10-23T10:00:00Z" } })]); + const { actions, reads } = actionsWorld({}, {}); + const lines = []; + const log = console.log; + console.log = (line) => lines.push(line); + try { + assert.equal(await main({ env: { ...ENV, EVENT_NAME: "pull_request_target" }, github, actions }), 0); + } finally { + console.log = log; + } + assert.deepEqual(writes(), []); + assert.deepEqual(reads, []); + assert.ok(lines.some((line) => /#1 left: already armed; the next reader run checks its verdict/.test(line))); +}); + +test("a reader that fails, prints no JSON or reports another head arms nothing and disarms", async () => { + const dir = mkdtempSync(join(tmpdir(), "enable-auto-merge-")); + const scripts = { + 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\nprint(json.dumps({"refusal": None, "head": sys.argv[-1]}))\n`, + }; + for (const [name, body] of Object.entries(scripts)) { + writeFileSync(join(dir, `${name}.py`), body); + } + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + for (const [name, want] of [ + ["exits", [{ mutation: "disarm", id: "PR_2" }]], + ["garbage", [{ mutation: "disarm", id: "PR_2" }]], + ["other", [{ mutation: "disarm", id: "PR_2" }]], + ["passes", [{ mutation: "arm", id: "PR_1", head: HEAD }]], + ]) { + const { github, writes } = world([pull(1), pull(2, { autoMergeRequest: armed })]); + const env = { ...ENV, EVENT_NAME: "schedule", PATH: process.env.PATH, VERIFY_EVIDENCE_SCRIPT: join(dir, `${name}.py`) }; + const log = console.log; + console.log = () => {}; + try { + await main({ env, github, actions: async () => assert.fail("the reader path reads no Actions API itself") }); + } finally { + console.log = log; + } + assert.deepEqual(writes(), want, name); + } +}); diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 66982b3144..6ead6be9ad 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -173,8 +173,10 @@ const PASSES = Object.freeze(["pass", "pass-notes"]); // Why the head has no verifier pass, or null when a record shows one: a pass // or pass-notes artifact whose run is pr-verify.yml's, event workflow_run, on -// `branch` of `repo` with its head in `repo`, completed with success -// (babysit_verify_evidence `_origin` and a counted run). The records are +// `branch` of `repo` with its head in `repo`, completed with success: the +// fields of babysit_verify_evidence `_origin` and a counted run, without its +// compare read that main holds the run's commit, since main's ruleset +// forbids the force push that could drop it. The records are // read for the head alone, by exact name, so a record of another head never // reaches here. export function verdictHold(records, repo, branch = "main") { diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 4f16fee774..c0f9f362f9 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3874,7 +3874,7 @@ jobs: # 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 # exact name, with and without the head-branch digest, stopping at the - # first such run: 2 reads for a plain-named pass, at most 4 lookups plus + # first name that holds such a run: 2 reads for a plain-named pass, at most 4 lookups plus # one run read per artifact found. The same approval of the head lifts # it. Only in the queue: pr-verify runs only after a successful # pull_request run of this workflow, so a red ci-status there would @@ -3986,9 +3986,10 @@ jobs: record=$(read_api "run $run" "repos/$GITHUB_REPOSITORY/actions/runs/$run") || exit 1 jq -c --arg name "$name" --arg verdict "$verdict" '{name: $name, verdict: $verdict, run: .}' <<<"$record" >>"$RUNNER_TEMP/verdicts.jsonl" || exit 1 done - # A completed, successful run ends the lookup; merge-holds.mjs - # checks it is pr-verify's. - ! jq -se 'any(.run.status == "completed" and .run.conclusion == "success")' "$RUNNER_TEMP/verdicts.jsonl" >/dev/null || break + # A pass merge-holds.mjs verdictHold would count ends the lookup. + ! jq -se --arg repo "$GITHUB_REPOSITORY" 'any(.run | .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") fi diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 5ee0e39b04..e8b7566b4a 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1052,11 +1052,12 @@ gate's veto rule; a later comment lifts it), and whose head changes no path in t `merge.diff-check.denied-paths`, read on the files the head changes at each run, and that has a PASS or PASS-NOTES of [pr-verify](#verifier-verdict) at that head. That is read last, only for a pull request every other rule would arm or keep armed. An event run looks the head's own pass up -by exact artifact name, as `ci-status` does in the merge queue (below). The scheduled run asks the -gate's reader instead (`babysit_verify_evidence.py evidence`, with its saved listing, head-ref map -and run cache), which also refuses a FAIL at the head, an earlier FAIL not cleared, and FAILs at -`merge.max-verify-fails` or more heads; a pull request that goes over the cap between scheduled -runs stays armed until the next one. It turns auto-merge off on an armed one that now fails +by exact artifact name, as `ci-status` does in the merge queue (below), and leaves an armed one +armed. The scheduled run and a hand-started one ask the gate's reader instead +(`babysit_verify_evidence.py evidence`, with its saved listing, head-ref map and run cache), which +also refuses a FAIL at the head beside a re-run's pass, an earlier FAIL not cleared, FAILs at +`merge.max-verify-fails` or more heads, and a spec changed since the judgment. A pull request an +event run armed on its pass alone stays armed until the next reader run. 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 armed right after report-outcome opens it; a listed human's edit, unlabel or ready wakes it too, so @@ -1092,7 +1093,7 @@ holds a head pr-verify has not passed. The same trusted approval of the head lif request the gate sends to a person. It runs only in the queue: pr-verify runs only after a successful `pull_request` run of `pr-require-checks`, so a red `ci-status` there would never get a verdict, and a pull request event makes no verdict read. FAILs and the repeated-FAIL cap are -`enable-auto-merge.mjs`'s, which disarms such a pull request on its next scheduled run. +`enable-auto-merge.mjs`'s, which disarms such a pull request on its next reader run. ### Dependabot pull requests diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index 9e6cf520e5..98deb9efba 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -328,6 +328,13 @@ for bad in 12:in-progress 13:another-workflow 14:a-pull-request-event 15:another done STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass\":[16],\"pr-verify-7-$SHA-pass-$DIGEST\":[11]}" expect "in the merge queue, a failed run's pass does not hide a later name's good one" 0 3 "pr-verify passed this head" +STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass\":[13],\"pr-verify-7-$SHA-pass-$DIGEST\":[11]}" +expect "in the merge queue, another workflow's successful run does not end the lookup before a real pass" 0 3 "pr-verify passed this head" +if [[ "$(lookups)" -eq 2 ]]; then + ok "the lookup ends at the first name holding a pr-verify pass" +else + fail "the lookup took $(lookups) lookups" +fi STUB_ARTIFACTS='{}' 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 missing pass" 0 3 "no verifier hold: kyle-sexton approved this head" STUB_REVIEWS="[$(approval kyle-sexton 153232337 APPROVED 2026-10-23T11:00:00Z "$OLD")]" From 6948bfc613aaacdec7b022c5a118b4fccb758013 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:14:34 -0400 Subject: [PATCH 07/11] fix(pr-merge): arm only on the gate's reader, and disarm on each push Owner direction on the review findings: native auto-merge now agrees with the gate's full refusal list. Only the scheduled or hand-started enable-auto-merge run reads the verdict (babysit_verify_evidence.py) and arms; event runs read no verdict, never arm, and only disarm on a hold. ci-status turns an armed lanes App pull request's auto-merge off on a push to its head (one mutation, no read), so each new head waits for the reader. The merge-queue pass check stays as the backstop. Co-Authored-By: Claude Opus 5.5 --- ...-native-merge-requires-verdict-7c41e2a9.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 101 +++++-------- .../pr-merge/enable-auto-merge.test.mjs | 135 ++++-------------- .github/scripts/pr-merge/workflow.test.mjs | 7 +- .github/workflows/pr-merge.yml | 23 ++- .github/workflows/pr-require-checks.yml | 21 ++- docs/conventions/pr-pipeline/README.md | 21 +-- scripts/ci-lanes-pr-hold.test.sh | 40 +++++- 8 files changed, 149 insertions(+), 201 deletions(-) diff --git a/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md b/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md index e9ef9023f2..bdf0b05d78 100644 --- a/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md +++ b/.changes/pr-pipeline/fix-native-merge-requires-verdict-7c41e2a9.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- Native auto-merge of a lanes App pull request now needs the verifier verdict pr-merge's gate needs. `enable-auto-merge.mjs` arms, and keeps armed, only with a pr-verify pass at the head it read, and disarms otherwise: an event run looks the head's pass up by exact artifact name, and the scheduled run asks the gate's reader, which also refuses a FAIL at the head and FAILs at `merge.max-verify-fails` or more heads. In the merge queue, `ci-status` requires a pass or pass-notes artifact at the queued head from a successful pr-verify run on main; a trusted approval of the head lifts it. Pull request events read no verdict, since pr-verify runs only after a green pull request run. +- Native auto-merge of a lanes App pull request now needs the verifier verdict pr-merge's gate needs. `enable-auto-merge.mjs` arms, and keeps armed, only when pr-merge's gate reader refuses nothing at the head it read (a pr-verify pass, no FAIL at the head, FAILs under `merge.max-verify-fails`, an unchanged trusted spec), and only on its scheduled or hand-started run; event runs never arm. `ci-status` turns an armed lanes App pull request's auto-merge off on each push to its head. In the merge queue, `ci-status` requires a pass or pass-notes artifact at the queued head from a successful pr-verify run on main; a trusted approval of the head lifts it. Pull request events read no verdict, since pr-verify runs only after a green pull request run. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index f9aa8052f8..f095630842 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -24,22 +24,20 @@ // group, so a push after arming is held without waiting for this job. // - 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. -// - A verifier pass at the head just read. It is read last, only for a pull -// request every other rule would arm or keep armed, and how depends on the -// run (verifyFor): -// - an event run (the issue lane, a person's edit, unlabel or ready) looks -// the head's own PASS or PASS-NOTES up by exact artifact name -// (headPassVerify, the same lookup and rule as ci-status's merge-queue -// check, merge-holds.mjs verdictHold): 2 to 4 lookups and a run read; -// - the scheduled run and a hand-started one ask pr-merge's own reader (pythonVerify, -// babysit_verify_evidence.py `evidence`), which also refuses a FAIL at -// the head, an earlier FAIL not cleared and FAILs at merge.max-verify-fails -// or more heads, sharing one artifact listing and run cache in the run. -// An event run arms on the pass alone: it does not see a FAIL at the head -// beside a re-run's pass, an earlier FAIL, the cap, or a spec changed since -// the judgment, and it leaves an armed pull request armed. The next reader -// run disarms such a one. The merge queue's ci-status checks the pass again -// at the head it merges, whatever a push after arming does. +// - 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 +// 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 +// hold, so a pull request waits up to one scheduled interval to be armed. +// ci-status disarms an armed lanes App pull request on each push to its +// 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). // 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 @@ -54,11 +52,11 @@ // TRACKER_CONFIG the base .work-item-tracker.json, for the human-gated // label (default .base/.work-item-tracker.json) // RESOLVED_CONFIG the resolve-config output, for the hold paths -// EVENT_NAME the run's event; `schedule` reads with the full reader +// EVENT_NAME the run's event; only READER_EVENTS read verdicts and arm // MAX_VERIFY_FAILS merge.max-verify-fails (default: the reader's) -// BABYSIT_VERIFY_GH_TOKEN the job's GITHUB_TOKEN, for the Actions reads -// BABYSIT_VERIFY_*, BABYSIT_RUN_CACHE the reader's saved listing, head-ref -// map and run cache, as the gate's +// 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 // GITHUB_API_URL import { spawnSync } from "node:child_process"; import process from "node:process"; @@ -66,9 +64,8 @@ import { pathToFileURL } from "node:url"; import { createGitHub } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; -import { headRefDigest } from "../pr-verify/write-verdict.mjs"; import { readHoldPaths } from "../work-resolve-issue/report-outcome.mjs"; -import { BODY_HOLD, pathHold, verdictHold, vetoers } from "./merge-holds.mjs"; +import { BODY_HOLD, pathHold, vetoers } from "./merge-holds.mjs"; export { BODY_HOLD, HUMAN_VETO } from "./merge-holds.mjs"; @@ -91,7 +88,7 @@ export const PULLS_QUERY = `query($owner: String!, $name: String!, $after: Strin pullRequests(states: OPEN, baseRefName: "main", first: 100, after: $after) { pageInfo { hasNextPage endCursor } nodes { - id number isDraft isCrossRepository headRefOid headRefName body + id number isDraft isCrossRepository headRefOid body author { __typename login } labels(first: 100) { totalCount nodes { name } } autoMergeRequest { enabledAt } @@ -194,37 +191,6 @@ export function verifyHold(evidence, head) { return evidence.refusal === null ? null : `pr-merge's gate refuses its verifier verdict (${escapeCommand(evidence.refusal)})`; } -// The head's own PASS or PASS-NOTES, looked up by exact artifact name with and -// without the head-branch digest, as ci-status does in the merge queue, under -// the Actions token: one lookup per name until one holds a pass verdictHold -// counts, one run read per unexpired artifact found. The answer is a report shaped -// like the reader's. -export function headPassVerify(actions, repo) { - const verify = async ({ number, headRefOid: head, headRefName: ref }) => { - const digest = headRefDigest(String(ref ?? "")); - const records = []; - for (const verdict of ["pass", "pass-notes"]) { - for (const name of [`pr-verify-${number}-${head}-${verdict}`, `pr-verify-${number}-${head}-${verdict}-${digest}`]) { - const listed = await actions("GET", `/repos/${repo}/actions/artifacts?name=${encodeURIComponent(name)}&per_page=100`); - if (!Array.isArray(listed?.artifacts)) { - throw new Error(`the ${name} artifacts did not read`); - } - for (const artifact of listed.artifacts) { - const id = artifact?.workflow_run?.id; - if (artifact?.name === name && artifact.expired === false && Number.isSafeInteger(id)) { - records.push({ name, verdict, run: await actions("GET", `/repos/${repo}/actions/runs/${id}`) }); - } - } - if (verdictHold(records, repo) === null) { - return { refusal: null, head }; - } - } - } - return { refusal: "verify:missing", head }; - }; - return Object.assign(verify, { mode: "head-pass" }); -} - // 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) { @@ -239,16 +205,13 @@ export function pythonVerify(env) { } return JSON.parse(String(done.stdout).trim().split("\n").at(-1)); }; - return Object.assign(verify, { mode: "reader" }); + return verify; } -// The scheduled run and a hand-started one, which a person starts to retry a -// pull request that looks stuck, read with the full reader; every other event -// run with the head-pass lookup. +// 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. export const READER_EVENTS = Object.freeze(["schedule", "workflow_dispatch"]); -export function verifyFor(env, actions) { - return READER_EVENTS.includes(env.EVENT_NAME) ? pythonVerify(env) : headPassVerify(actions, env.GITHUB_REPOSITORY); -} // Reader time per run: past it, the rest of the pull requests are left for // the next run rather than cut off by the step's timeout (judgment, inside // the step's 10 minutes). @@ -315,7 +278,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, actions, verify } = {}) { +export async function main({ env = process.env, github, verify } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -333,14 +296,17 @@ export async function main({ env = process.env, github, actions, verify } = {}) const humanGated = humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json"); const holdPaths = readHoldPaths(env.RESOLVED_CONFIG ?? ""); github ??= createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); - actions ??= createGitHub({ token: env.BABYSIT_VERIFY_GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); - verify ??= verifyFor(env, actions); + 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; let failed = 0; for (const pull of pulls) { let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }); let detail = null; + 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" }); + } if (action === "read") { try { const variables = { owner: repo[1], name: repo[2], number: pull.number }; @@ -353,8 +319,11 @@ export async function main({ env = process.env, github, actions, verify } = {}) } ({ action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }, detail)); } - if (action === "verify" && pull.autoMergeRequest != null && verify.mode === "head-pass") { - ({ action, reason } = { action: "none", reason: "already armed; the next reader run checks its verdict" }); + if (action === "verify" && !reads) { + ({ action, reason } = + pull.autoMergeRequest != null + ? { action: "none", reason: "already armed; the next scheduled run checks its verifier verdict" } + : { action: "none", reason: "only a scheduled or hand-started run arms it, on the gate's verifier verdict" }); } else if (action === "verify" && Date.now() > deadline) { ({ action, reason } = { action: "none", reason: "no time left in this run to read its verifier verdict" }); } else if (action === "verify") { diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index ff348873b1..0df1d034b0 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -22,12 +22,10 @@ import { HUMAN_VETO, PULLS_QUERY, decide, - headPassVerify, + READER_EVENTS, main, - verifyFor, verifyHold, } from "./enable-auto-merge.mjs"; -import { headRefDigest } from "../pr-verify/write-verdict.mjs"; import { VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; import { decide as decideOutcome } from "../work-resolve-issue/report-outcome.mjs"; @@ -36,6 +34,7 @@ const REPO = "melodic-software/claude-code-plugins"; const LANES = { __typename: "Bot", login: "melodic-automation-lanes" }; const HEAD = "b".repeat(40); const ENV = { + EVENT_NAME: "schedule", GITHUB_REPOSITORY: REPO, KILL_SWITCH: "proceed", REPORT_ONLY: "false", @@ -492,113 +491,37 @@ test("verdictHold names the verifier workflow the resolver and the gate name", ( assert.equal(VERIFY_PATH, RESOLVER_VERIFY_PATH); }); -// Event runs look the head's own pass up by exact name, as ci-status does in -// the merge queue; only the scheduled run spends the full reader. -test("the scheduled run reads with the gate's reader, every event run by exact name", () => { - const env = { GITHUB_REPOSITORY: REPO }; - assert.equal(verifyFor({ ...env, EVENT_NAME: "schedule" }, null).mode, "reader"); - assert.equal(verifyFor({ ...env, EVENT_NAME: "workflow_dispatch" }, null).mode, "reader"); - for (const event of ["workflow_run", "pull_request_target", undefined]) { - assert.equal(verifyFor({ ...env, EVENT_NAME: event }, null).mode, "head-pass", String(event)); - } +// Only the gate's full reader arms, so native merge refuses what the gate +// refuses; an event run disarms on a hold and reads no verdict. +test("only the scheduled run and a hand-started one read the verdict and arm", () => { + assert.deepEqual([...READER_EVENTS], ["schedule", "workflow_dispatch"]); }); -function actionsWorld(artifacts, runs) { - const reads = []; - const actions = async (method, path) => { - reads.push(path); - assert.equal(method, "GET"); - const byName = /\/actions\/artifacts\?name=([^&]+)&per_page=100$/.exec(path); - if (byName) { - const name = decodeURIComponent(byName[1]); - return { - artifacts: [ - ...(artifacts[name] ?? []).map((id) => ({ name, expired: false, workflow_run: { id } })), - { name, expired: true, workflow_run: { id: 1 } }, - { name: `${name}-i1`, expired: false, workflow_run: { id: 1 } }, - ], - }; - } - const run = /\/actions\/runs\/([0-9]+)$/.exec(path); - if (run && runs[run[1]]) { - return runs[run[1]]; - } - throw new Error(`GET ${path} failed with status 404`); - }; - return { actions, reads }; -} - -test("headPassVerify passes on a successful pr-verify run's pass at the head, by exact name", async () => { - const good = { - path: VERIFY_PATH, - event: "workflow_run", - head_branch: "main", - status: "completed", - conclusion: "success", - repository: { full_name: REPO }, - head_repository: { full_name: REPO }, - }; - const runs = { 1: good, 11: good, 12: { ...good, conclusion: "failure" }, 13: { ...good, path: ".github/workflows/x.yml" } }; - const digest = headRefDigest("work-resolve-issue/issue-1"); - const target = pull(7, { headRefName: "work-resolve-issue/issue-1" }); - const check = async (artifacts) => { - const { actions, reads } = actionsWorld(artifacts, runs); - return { ...(await headPassVerify(actions, REPO)(target)), lookups: reads.filter((path) => path.includes("artifacts?name=")).length }; - }; - assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass`]: [11] }), { refusal: null, head: HEAD, lookups: 1 }); - assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass-notes-${digest}`]: [11] }), { refusal: null, head: HEAD, lookups: 4 }); - assert.deepEqual(await check({}), { refusal: "verify:missing", head: HEAD, lookups: 4 }); - assert.deepEqual(await check({ [`pr-verify-7-${"c".repeat(40)}-pass`]: [11] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); - assert.deepEqual(await check({ [`pr-verify-8-${HEAD}-pass`]: [11] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); - assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass`]: [12] }), { refusal: "verify:missing", head: HEAD, lookups: 4 }); - assert.equal((await check({ [`pr-verify-7-${HEAD}-pass`]: [13] })).refusal, "verify:missing"); - // Another workflow's successful run under one name does not end the lookup - // before a later name's real pass. - assert.deepEqual(await check({ [`pr-verify-7-${HEAD}-pass`]: [13], [`pr-verify-7-${HEAD}-pass-${digest}`]: [11] }), { - refusal: null, - head: HEAD, - lookups: 2, - }); - await assert.rejects(check({ [`pr-verify-7-${HEAD}-pass`]: [99] }), /404/); -}); - -test("an event run arms on the head's pass and leaves a head with none", async () => { - const { github, writes } = world([pull(1), pull(2)]); - const { actions } = actionsWorld({ [`pr-verify-1-${HEAD}-pass`]: [11] }, { - 11: { - path: VERIFY_PATH, - event: "workflow_run", - head_branch: "main", - status: "completed", - conclusion: "success", - repository: { full_name: REPO }, - head_repository: { full_name: REPO }, - }, - }); - const log = console.log; - console.log = () => {}; - try { - assert.equal(await main({ env: { ...ENV, EVENT_NAME: "workflow_run" }, github, actions }), 0); - } finally { - console.log = log; +test("an event run arms nothing and reads neither discussion nor verdict for an unarmed pull request", async () => { + for (const event of ["workflow_run", "pull_request_target"]) { + const { code, writes, verified, lines } = await run([pull(1)], { EVENT_NAME: event }); + assert.equal(code, 0); + assert.deepEqual(writes, []); + assert.deepEqual(verified, []); + assert.ok(lines.some((line) => /#1 left: only a scheduled or hand-started run arms it/.test(line)), event); } - assert.deepEqual(writes(), [{ mutation: "arm", id: "PR_1", head: HEAD }]); }); -test("an event run leaves an armed pull request armed without reading its verdict", async () => { - const { github, writes } = world([pull(1, { autoMergeRequest: { enabledAt: "2026-10-23T10:00:00Z" } })]); - const { actions, reads } = actionsWorld({}, {}); - const lines = []; - const log = console.log; - console.log = (line) => lines.push(line); - try { - assert.equal(await main({ env: { ...ENV, EVENT_NAME: "pull_request_target" }, github, actions }), 0); - } finally { - console.log = log; - } - assert.deepEqual(writes(), []); - assert.deepEqual(reads, []); - assert.ok(lines.some((line) => /#1 left: already armed; the next reader run checks its verdict/.test(line))); +test("an event run still disarms an armed pull request that now holds", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const { writes, verified } = await run( + [ + pull(1, { autoMergeRequest: armed, labels: labels("do-not-merge") }), + pull(2, { autoMergeRequest: armed, discussion: { comments: [said("kyle-sexton", "do not merge", "2026-10-23T10:00:00Z")] } }), + pull(3, { autoMergeRequest: armed }), + ], + { EVENT_NAME: "pull_request_target" }, + ); + assert.deepEqual(writes, [ + { mutation: "disarm", id: "PR_1" }, + { mutation: "disarm", id: "PR_2" }, + ]); + assert.deepEqual(verified, []); }); test("a reader that fails, prints no JSON or reports another head arms nothing and disarms", async () => { @@ -624,7 +547,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, actions: async () => assert.fail("the reader path reads no Actions API itself") }); + await main({ env, github }); } finally { console.log = log; } diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index f8245f6746..4d3a8e369b 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -191,8 +191,11 @@ test("enable-auto-merge asks the broker for its own activity's merge token", () assert.ok(arm.includes(`\n ${line}\n`), line); } assert.match(arm, /node \.base\/\.github\/scripts\/pr-merge\/head-ref-map\.mjs read\n/); - // Only the scheduled run spends the full reader, so only it reads the map. - assert.match(arm, /- name: Read the head refs the last gate run kept\n {8}if: steps\.lane-token\.outputs\.token != '' && github\.event_name == 'schedule'\n/); + // Only the runs that read the verdict (READER_EVENTS) read the map. + assert.match( + arm, + /- name: Read the head refs the last gate run kept\n {8}if: steps\.lane-token\.outputs\.token != '' && \(github\.event_name == 'schedule' \|\| github\.event_name == 'workflow_dispatch'\)\n/, + ); assert.match(arm, /MAX_VERIFY_FAILS=\$\(yq '\.merge\."max-verify-fails" \/\/ ""' "\$config"\)\n/); }); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 09a2c47c66..a42ee9d923 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1258,12 +1258,10 @@ jobs: # 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. It arms only on a pr-verify pass at the head, read - # with the job's GITHUB_TOKEN: an event run looks the head's own pass up by - # exact name (2 to 4 lookups and a run read per pull request), and the - # scheduled run asks the gate's reader (babysit_verify_evidence.py, from the - # head-ref map and run cache the last gate run kept), so a FAIL or the - # repeated-FAIL cap disarms on the schedule. + # 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. # Its own job and token: the broker mints one token per # job. Rules: .github/scripts/pr-merge/enable-auto-merge.mjs. enable-auto-merge: @@ -1357,11 +1355,12 @@ jobs: issues: ${{ steps.config.outputs.issues }} superseded-ok: true - # The scheduled run's verdict reader ties untied FAILs as the gate job - # does: the head-ref map and the carried run cache. Any error leaves - # none, and the reader reads GitHub instead. Event runs need neither. + # The verdict reader of a scheduled or hand-started run ties untied + # FAILs as the gate job does: the head-ref map and the carried run + # cache. Any error leaves none, and the reader reads GitHub instead. + # Event runs read no verdict and need neither. - name: Read the head refs the last gate run kept - if: steps.lane-token.outputs.token != '' && github.event_name == 'schedule' + if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') continue-on-error: true env: ACTIONS_TOKEN: ${{ github.token }} @@ -1369,7 +1368,7 @@ jobs: run: node .base/.github/scripts/pr-merge/head-ref-map.mjs read - name: Restore the verdict reads the last gate run kept - if: steps.lane-token.outputs.token != '' && github.event_name == 'schedule' + if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') continue-on-error: true timeout-minutes: 2 uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 @@ -1379,7 +1378,7 @@ jobs: restore-keys: pr-merge-run-cache- - name: Start the run cache from them - if: steps.lane-token.outputs.token != '' && github.event_name == 'schedule' + if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') continue-on-error: true env: CARRY_FROM: ${{ runner.temp }}/pr-merge-carried.json diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index c0f9f362f9..d6a7408574 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3879,8 +3879,10 @@ jobs: # it. Only in the queue: pr-verify runs only after a successful # pull_request run of this workflow, so a red ci-status there would # never get a verdict, and a pull request event reads nothing for it. - # Whatever a push after arming does to the request, the queue checks - # the head it merges. + # A push to an armed lanes App pull request turns its auto-merge off + # (one mutation, no read), so enable-auto-merge's reader judges each + # 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. # 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 @@ -3888,6 +3890,7 @@ jobs: env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} + EVENT_ACTION: ${{ github.event.action }} GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }} run: | lanes_app_id=337595936 @@ -3930,6 +3933,20 @@ jobs: exit 0 fi BASE_SHA=$(jq -r '.base.sha' <<<"$pull") + # A push gives an armed pull request a head enable-auto-merge's + # reader has not judged: turn its auto-merge off until a reader + # run arms the new head. A failure only warns, so it cannot hold + # ci-status red and keep pr-verify from judging the head; the + # merge queue still needs a pass at the head it merges. + if [ "${EVENT_ACTION:-}" = synchronize ] && jq -e '.auto_merge != null' <<<"$pull" >/dev/null; then + # shellcheck disable=SC2016 # $id is a GraphQL variable + if gh api graphql -f query='mutation($id: ID!) { disablePullRequestAutoMerge(input: {pullRequestId: $id}) { clientMutationId } }' \ + -f id="$(jq -r '.node_id' <<<"$pull")" >/dev/null; then + echo "#$pr: a push moved its head, so its native auto-merge is off until enable-auto-merge's next scheduled run judges the new head." + else + echo "::warning::could not turn off native auto-merge on #$pr after a push to its head; the merge queue still needs a pr-verify pass at the head it merges." + fi + fi fi if [[ ! $BASE_SHA =~ ^[0-9a-f]{40}$ ]]; then echo "::error::the base commit '$BASE_SHA' is not a commit SHA." diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index e8b7566b4a..05c1bea3a6 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1049,15 +1049,16 @@ opens with `Closes #`, and carries no hold: no `Hold:` line or "do not me (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 that has a -PASS or PASS-NOTES of [pr-verify](#verifier-verdict) at that head. That is read last, only for a -pull request every other rule would arm or keep armed. An event run looks the head's own pass up -by exact artifact name, as `ci-status` does in the merge queue (below), and leaves an armed one -armed. The scheduled run and a hand-started one ask the gate's reader instead -(`babysit_verify_evidence.py evidence`, with its saved listing, head-ref map and run cache), which -also refuses a FAIL at the head beside a re-run's pass, an earlier FAIL not cleared, FAILs at -`merge.max-verify-fails` or more heads, and a spec changed since the judgment. A pull request an -event run armed on its pass alone stays armed until the next reader run. It turns auto-merge off on an armed one that now fails +`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, +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` +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 armed right after report-outcome opens it; a listed human's edit, unlabel or ready wakes it too, so @@ -1093,7 +1094,7 @@ holds a head pr-verify has not passed. The same trusted approval of the head lif request the gate sends to a person. It runs only in the queue: pr-verify runs only after a successful `pull_request` run of `pr-require-checks`, so a red `ci-status` there would never get a verdict, and a pull request event makes no verdict read. FAILs and the repeated-FAIL cap are -`enable-auto-merge.mjs`'s, which disarms such a pull request on its next reader run. +`enable-auto-merge.mjs`'s, whose reader arms nothing they refuse. ### Dependabot pull requests diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index 98deb9efba..ccd0dd09c1 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -101,6 +101,10 @@ discussion() { [[ "$(grep -cE '/(comments|reviews)\?' "$STUB_LOG")" -gt "${STUB_FAILS:-0}" ]] || { echo 'gh: Server Error (HTTP 502)' >&2; exit 1; } } case "$*" in +"api graphql "*) + [[ -z "${STUB_DISARM_FAILS:-}" ]] || { echo 'gh: Resource not accessible by integration' >&2; exit 1; } + echo '{"data":{}}' + ;; */contents/.github/scripts/pr-merge/merge-holds.mjs*) [[ -z "${STUB_NO_MODULE:-}" ]] || { echo 'gh: Not Found (HTTP 404)' >&2; exit 1; } cat "$STUB_MODULE" @@ -143,7 +147,8 @@ BASE=89abcdef0123456789abcdef0123456789abcdef QUEUE="refs/heads/gh-readonly-queue/main/pr-7-$SHA" LANES='{"id":337595936,"login":"melodic-automation-lanes[bot]","type":"Bot"}' pull() { jq -nc --argjson user "${2:-$LANES}" --arg body "$1" --arg ref "${STUB_BASE_REF:-main}" --arg sha "${STUB_BASE_SHA:-$BASE}" \ - --argjson changed "${STUB_CHANGED:-1}" '{number: 7, user: $user, body: $body, head: {sha: "'"$SHA"'", ref: "work-resolve-issue/issue-1"}, base: {ref: $ref, sha: $sha}, changed_files: $changed}'; } + --argjson changed "${STUB_CHANGED:-1}" '{number: 7, node_id: "PR_7", user: $user, body: $body, head: {sha: "'"$SHA"'", ref: "work-resolve-issue/issue-1"}, base: {ref: $ref, sha: $sha}, changed_files: $changed, + auto_merge: (if env.STUB_ARMED then {merge_method: "squash"} else null end)}'; } comment() { jq -nc --arg login "$1" --arg type "${2:-User}" --arg assoc "$3" --arg at "$4" --arg body "$5" \ '{user: {login: $login, type: $type}, author_association: $assoc, created_at: $at, body: $body}'; } # approval : a review of a commit. @@ -158,7 +163,7 @@ expect() { local label="$1" want_rc="$2" want_calls="$3" out rc calls : >"$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}" GITHUB_REF="${REF:-refs/pull/7/merge}" PR_NUMBER="${PR_NUMBER-7}" \ + 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=$? calls=$(grep -cE '/(comments|reviews)\?' "$TMP_ROOT/log") if [[ "$rc" -ne "$want_rc" ]]; then @@ -297,6 +302,37 @@ else fail "a pull request event read a verdict: '$(sort -u "$TMP_ROOT/log")'" fi +# disarms: the auto-merge mutations the last run sent. +disarms() { grep -c '^api graphql .*disablePullRequestAutoMerge' "$TMP_ROOT/log"; } +# no_disarm