From d2db1836e1f7bba21c6fb7ea80cc89a341f1fade Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 27 Oct 2026 10:21:18 -0400 Subject: [PATCH 1/2] perf(pr-merge): reuse the last tick's post-merge report on an idle tick post-merge-dispatch's "Check earlier lane merges" ran in full on every tick (median 375 s over 72 ticks, ~115 gh calls), including ticks with no new lane merge since the tick before. It now reads the report the last trusted run on main kept (last-report.mjs, found as the lane records are) and reuses it when `settle --idle-tick` keeps it: the lane room's rule (settle_shared) plus idle_refusal, which covers the rest of what the check reads that can move without a merge (verifying runs completed since, quarantine issues, pins, quarantine excuses, the dispatch deadline, anything to start). The report keeps its computedAt, code and commit, so reuse ends SHARED_MAX_AGE after the check that made it, with 10 minutes left for the merge gate and lane room. pending_runs_unchanged now lists runs per verifying workflow, and the reuse rule refuses a report past an expired quarantine excuse. Co-Authored-By: Claude Opus 5.5 --- .../perf-postmerge-idle-tick-5c2e81d4.md | 7 + .../perf-postmerge-idle-tick-5c2e81d4.md | 9 + .github/scripts/pr-merge/last-report.mjs | 72 ++++++ .github/scripts/pr-merge/last-report.test.mjs | 81 +++++++ .github/scripts/pr-merge/merge-holds.mjs | 1 + .github/scripts/pr-merge/merge-job.test.mjs | 99 ++++++++ .github/scripts/pr-merge/workflow.test.mjs | 1 + .github/workflows/pr-merge.yml | 53 +++- .github/workflows/pr-require-checks.yml | 2 +- .github/workflows/pr-review-hosted.yml | 1 + .../workflows/pr-review-security-hosted.yml | 1 + docs/conventions/pr-pipeline/README.md | 4 +- .../scripts/babysit_merge_policy.py | 17 +- .../babysit-prs/scripts/babysit_postmerge.py | 228 ++++++++++++++---- .../babysit-prs/scripts/babysit_risk.py | 1 + .../scripts/babysit_verify_evidence.py | 24 +- .../scripts/tests/test_babysit_lane_room.py | 228 +++++++++++++++++- .../scripts/tests/test_babysit_postmerge.py | 10 + .../scripts/tests/test_babysit_risk.py | 1 + .../scripts/tests/test_merge_holds_sync.py | 2 +- 20 files changed, 778 insertions(+), 64 deletions(-) create mode 100644 .changes/pr-pipeline/perf-postmerge-idle-tick-5c2e81d4.md create mode 100644 .changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md create mode 100755 .github/scripts/pr-merge/last-report.mjs create mode 100644 .github/scripts/pr-merge/last-report.test.mjs diff --git a/.changes/pr-pipeline/perf-postmerge-idle-tick-5c2e81d4.md b/.changes/pr-pipeline/perf-postmerge-idle-tick-5c2e81d4.md new file mode 100644 index 0000000000..201407a47b --- /dev/null +++ b/.changes/pr-pipeline/perf-postmerge-idle-tick-5c2e81d4.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Changed + +- pr-merge's post-merge-dispatch job reads the post-merge report that the last scheduled or hand-started run kept. It reads only a run on main and finds it the way it finds the lane records (`.github/scripts/pr-merge/last-report.mjs`). On an idle tick it reuses that report and does not run "Check earlier lane merges". It keeps the report's `computedAt`, exit code and code commit, so a chain of reuses ends `SHARED_MAX_AGE` after the check that made the report. Any refusal or failed read runs the check as before. `last-report.mjs` is gate code on the risk floor. diff --git a/.changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md b/.changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md new file mode 100644 index 0000000000..9530ce38cc --- /dev/null +++ b/.changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md @@ -0,0 +1,9 @@ +--- +bump: patch +--- + +### Changed + +- `babysit_verify_evidence.py settle --idle-tick` lets pr-merge's post-merge-dispatch job reuse the post-merge report the last tick kept. The report must pass the lane room's reuse rule (`settle_shared`) and `babysit_postmerge.idle_refusal`. That rule takes a passing report only (exit 0 or 5) with no hold, no full run or re-run to start, no unattributed red run and no quarantine excuse, and at least 10 minutes left before `SHARED_MAX_AGE`. It also requires that since the report no verifying run completed, no quarantine issue changed, and every pin the check read reads the same again. The post-merge report now records those pins (`pins`). +- `pending_runs_unchanged` lists runs once for each verifying workflow instead of once for every workflow on the branch. On main, the all-workflow listing held 152 runs in under two hours, more than fit on one page. +- The reuse rule (`shared_refusal`) refuses a report once a quarantine excuse it carries has expired (`excuse-expired`). diff --git a/.github/scripts/pr-merge/last-report.mjs b/.github/scripts/pr-merge/last-report.mjs new file mode 100755 index 0000000000..287e91bac7 --- /dev/null +++ b/.github/scripts/pr-merge/last-report.mjs @@ -0,0 +1,72 @@ +#!/usr/bin/env node +// last-report.mjs: the post-merge report the last pr-merge tick kept (its +// post-merge-dispatch job's pr-merge-post-merge-report artifact), which this +// tick's post-merge-dispatch job reuses in place of its own check while +// nothing the check reads has moved (babysit_verify_evidence.py settle +// --idle-tick). Found as the lane records are (lane-records.mjs): the newest +// scheduled or hand-started run of main's pr-merge.yml, this run aside, +// holding the artifact unexpired (rotation.mjs `newestTrustedRun`), whose head +// commit is on main (`onMain`). Only a run's own steps upload its artifacts, +// so the files were written by main's pr-merge.yml. Any failure leaves no +// report and warns: the check then runs. +// +// Environment: +// ACTIONS_TOKEN the job's GITHUB_TOKEN (actions: read) +// GITHUB_REPOSITORY owner/repo +// GITHUB_RUN_ID this run, whose report is not written yet +// PR_MERGE_READS_PATH the job's shared GET answers (rotation.mjs sharedReads), if set +// GITHUB_API_URL +// LAST_REPORT_DIR where the report's files are written +import { execFileSync } from "node:child_process"; +import { renameSync, rmSync } from "node:fs"; +import process from "node:process"; +import { pathToFileURL } from "node:url"; + +import { createGitHub } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; +import { onMain } from "./head-ref-map.mjs"; +import { newestTrustedRun, sharedReads } from "./rotation.mjs"; + +export const REPORT_ARTIFACT = "pr-merge-post-merge-report"; +const REPO = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; + +const escapeCommand = (text) => String(text).replaceAll(/[\r\n%]/g, " "); + +export async function main({ env = process.env, actions, download } = {}) { + const repo = env.GITHUB_REPOSITORY ?? ""; + const dir = env.LAST_REPORT_DIR; + try { + if (!dir) throw new Error("LAST_REPORT_DIR is not set"); + // A report left in its place before this step is never read as one. + rmSync(dir, { recursive: true, force: true }); + if (!REPO.test(repo)) throw new Error("GITHUB_REPOSITORY is not owner/repo"); + actions = sharedReads( + actions ?? createGitHub({ token: env.ACTIONS_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }), + env.PR_MERGE_READS_PATH, + ); + download ??= (id, target) => + execFileSync("gh", ["run", "download", String(id), "-R", repo, "-n", REPORT_ARTIFACT, "-D", target], { + env: { ...process.env, GH_TOKEN: env.ACTIONS_TOKEN }, + stdio: ["ignore", "ignore", "inherit"], + }); + const accept = (run) => onMain({ repo, actions, sha: run.head_sha }); + const id = await newestTrustedRun({ repo, runId: String(env.GITHUB_RUN_ID), actions, name: REPORT_ARTIFACT, accept }); + if (id === undefined) { + console.log("no earlier post-merge report; the post-merge check runs"); + return 0; + } + // Downloaded beside its place and moved in whole, so a failed download + // leaves no report. + const part = `${dir}.part`; + rmSync(part, { recursive: true, force: true }); + download(id, part); + renameSync(part, dir); + console.log(`the post-merge report of run ${id}`); + } catch (error) { + console.log(`::warning title=last post-merge report::${escapeCommand(error?.message ?? error)}; the post-merge check runs`); + } + return 0; +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) { + process.exitCode = await main(); +} diff --git a/.github/scripts/pr-merge/last-report.test.mjs b/.github/scripts/pr-merge/last-report.test.mjs new file mode 100644 index 0000000000..d3b82bbcd5 --- /dev/null +++ b/.github/scripts/pr-merge/last-report.test.mjs @@ -0,0 +1,81 @@ +// Expected values come from the trust rule last-report.mjs shares with the +// lane records (lane-records.mjs): the report is read only from the newest +// scheduled or hand-started pr-merge.yml run on main other than this one, +// holding the artifact unexpired, whose head commit is on main; any failure +// leaves no report. +import assert from "node:assert/strict"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { after, test } from "node:test"; + +import { forgetMainTip } from "./head-ref-map.mjs"; +import { main } from "./last-report.mjs"; +import { mainCall } from "./on-main.fixture.mjs"; + +const REPO = "melodic-software/claude-code-plugins"; +const dir = mkdtempSync(join(tmpdir(), "last-report-")); +after(() => rmSync(dir, { recursive: true, force: true })); +const sha = (id) => String(id).padStart(40, "a"); +const wf = (id, event, head_sha = sha(id)) => ({ id, event, head_branch: "main", path: ".github/workflows/pr-merge.yml", head_sha }); + +let n = 0; +// `runs` is the pr-merge.yml run listing, newest first; `held` lists the runs +// holding the report artifact; `offMain` the head commits not on main. +async function readLast({ runs = [], held = [], offMain = [], downloadFails = false, planted = false } = {}) { + forgetMainTip(); + n += 1; + const out = join(dir, `last-${n}`); + if (planted) { + mkdirSync(out); + writeFileSync(join(out, "post-merge.json"), "planted"); + } + const downloads = []; + const actions = async (method, url) => { + if (/\/actions\/workflows\/pr-merge\.yml\/runs\?branch=main&.*&page=1$/.test(url)) return { workflow_runs: runs }; + const onMain = mainCall(url, { offMain }); + if (onMain !== undefined) return onMain; + const id = Number(/\/actions\/runs\/(\d+)\/artifacts\?per_page=100$/.exec(url)?.[1]); + assert.ok(id, `unexpected call ${url}`); + const artifacts = held.includes(id) ? [{ name: "pr-merge-post-merge-report", expired: false }] : []; + return { total_count: artifacts.length, artifacts }; + }; + const download = (id, target) => { + downloads.push(id); + if (downloadFails) throw new Error("download 502"); + mkdirSync(target); + writeFileSync(join(target, "post-merge.json"), `report of ${id}`); + }; + const lines = []; + const log = console.log; + console.log = (line) => lines.push(String(line)); + try { + await main({ env: { GITHUB_REPOSITORY: REPO, GITHUB_RUN_ID: "999", LAST_REPORT_DIR: out }, actions, download }); + } finally { + console.log = log; + } + const file = join(out, "post-merge.json"); + return { downloads, lines, report: existsSync(file) ? readFileSync(file, "utf8") : undefined }; +} + +test("the newest trusted run's report on main is read, this run and other events aside", async () => { + const result = await readLast({ + runs: [wf(999, "schedule"), wf(8, "pull_request_target"), wf(7, "schedule"), wf(6, "workflow_dispatch")], + held: [999, 8, 7, 6], + offMain: [sha(7)], + }); + // Run 7's head commit is off main, so run 6 (hand-started) is read. + assert.deepEqual(result.downloads, [6]); + assert.equal(result.report, "report of 6"); +}); + +test("no earlier report, or one that fails to download, leaves none, even one planted", async () => { + const none = await readLast({ runs: [wf(7, "schedule")], held: [], planted: true }); + assert.deepEqual(none.downloads, []); + assert.equal(none.report, undefined); + const failed = await readLast({ runs: [wf(7, "schedule")], held: [7], planted: true }); + assert.equal(failed.report, "report of 7"); + const broken = await readLast({ runs: [wf(7, "schedule")], held: [7], downloadFails: true, planted: true }); + assert.equal(broken.report, undefined); + assert.match(broken.lines.join("\n"), /^::warning title=last post-merge report::download 502; the post-merge check runs$/m); +}); diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 16a1d73231..5e2fc57c3e 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -265,6 +265,7 @@ export const HOLD_PATH_CLASSES = Object.freeze([ [".github/scripts/pr-merge/gate-check.mjs", "gate"], [".github/scripts/pr-merge/head-ref-map.mjs", "gate"], [".github/scripts/pr-merge/lane-records.mjs", "gate"], + [".github/scripts/pr-merge/last-report.mjs", "gate"], [".github/scripts/pr-merge/rate-stop.bash", "gate"], [".github/scripts/pr-merge/refire-refused.mjs", "gate"], [".github/scripts/pr-merge/rotation.mjs", "gate"], diff --git a/.github/scripts/pr-merge/merge-job.test.mjs b/.github/scripts/pr-merge/merge-job.test.mjs index 38845adbf7..21aeeb28bd 100644 --- a/.github/scripts/pr-merge/merge-job.test.mjs +++ b/.github/scripts/pr-merge/merge-job.test.mjs @@ -51,6 +51,7 @@ test("the re-dispatch for earlier lane merges waits on neither the listing nor t "Check the kill switch", "Read report-only", "Read the lane records the last run kept", + "Read the post-merge report the last run kept", "Check earlier lane merges", "Keep the post-merge report for enable-auto-merge", "Start the full CI run on main", @@ -526,6 +527,104 @@ test("the early check fails red on a rate-limit stop, after its outputs and repo assert.equal(run.keptCode, "75"); }); +// post-merge-dispatch's check, run with the last run's report in +// post-merge-last (or none) and a fake python3: `settle` answers whether the +// report stands (exit `settle`), any other call is the check, which marks that +// it ran. The rule (pr-merge.yml): an idle tick reuses a report whose check +// passed (exit 0 or 5) and that `settle --idle-tick` keeps +// (babysit_verify_evidence.py, tested in test_babysit_lane_room.py +// IdleRefusal and SettleCommand), keeping its code and commit so the reuse +// ends SHARED_MAX_AGE after the check that made it; otherwise it runs the +// check. +const LAST_SHA = "d".repeat(40); +const lastReport = () => ({ readable: true, ok: true, needsDispatch: false, shared: { mainSha: "a".repeat(40) } }); +const runIdleCheck = ({ last, code = "0", settle = 0, exit = 3 } = {}) => { + const dir = mkdtempSync(join(tmpdir(), "pr-merge-idle-")); + try { + const bin = join(dir, "bin"); + mkdirSync(bin); + writeFileSync( + join(bin, "python3"), + `#!/usr/bin/env bash\nif [ "$2" = settle ]; then echo "$@" >"${dir}/settled"; exit ${settle}; fi\ntouch "${dir}/ran"\necho '{"ran": true}'\nexit ${exit}\n`, + ); + chmodSync(join(bin, "python3"), 0o755); + symlinkSync(root, join(dir, ".base")); + writeFileSync(join(dir, "lane-records.json"), "read in"); + if (last !== undefined) { + mkdirSync(join(dir, "post-merge-last")); + writeFileSync(join(dir, "post-merge-last/post-merge.json"), JSON.stringify(last)); + writeFileSync(join(dir, "post-merge-last/post-merge-code"), `${code}\n`); + writeFileSync(join(dir, "post-merge-last/post-merge-code-sha"), `${LAST_SHA}\n`); + writeFileSync(join(dir, "post-merge-last/lane-records.json"), "the last run's"); + } + const output = join(dir, "output"); + const result = spawnSync("bash", ["-c", runScript("Check earlier lane merges", stepsOf(jobBlock("post-merge-dispatch")))], { + cwd: dir, + encoding: "utf8", + env: { ...process.env, PATH: `${bin}:${process.env.PATH}`, RUNNER_TEMP: dir, GITHUB_OUTPUT: output, REPO: "o/r" }, + }); + const kept = (file) => (existsSync(join(dir, "post-merge-report", file)) ? readFileSync(join(dir, "post-merge-report", file), "utf8").trim() : null); + return { + status: result.status, + stdout: result.stdout, + ran: existsSync(join(dir, "ran")), + settled: existsSync(join(dir, "settled")) ? readFileSync(join(dir, "settled"), "utf8").trim() : null, + output: existsSync(output) ? readFileSync(output, "utf8") : "", + report: kept("post-merge.json"), + keptCode: kept("post-merge-code"), + keptSha: kept("post-merge-code-sha"), + records: kept("lane-records.json"), + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}; + +test("an idle tick reuses the last run's report that settle --idle-tick keeps, with its code and commit", () => { + for (const code of ["0", "5"]) { + const run = runIdleCheck({ last: lastReport(), code }); + assert.equal(run.status, 0, run.stdout); + assert.equal(run.ran, false, `exit ${code}: the check ran again`); + assert.match(run.settled, new RegExp(`babysit_verify_evidence\\.py settle --idle-tick --repo o/r --postmerge-result \\S+/post-merge-last/post-merge\\.json --code-sha ${LAST_SHA}$`)); + assert.match(run.stdout, /::notice::the last run's post-merge check \(exit \d\) still stands: nothing it reads has moved; reused, not run again/); + assert.match(run.output, new RegExp(`^code=${code}$`, "m")); + assert.match(run.output, /^needs-dispatch=false$/m); + // Kept for this run's later jobs and the next tick as it was made, with + // the lane records this run read, not the last run's. + assert.deepEqual(JSON.parse(run.report), lastReport()); + assert.equal(run.keptCode, code); + assert.equal(run.keptSha, LAST_SHA); + assert.equal(run.records, "read in"); + } +}); + +test("a tick runs the check itself for no report, a refusal, a crash or a check that did not pass", () => { + const head = execFileSync("git", ["-C", root, "rev-parse", "HEAD"], { encoding: "utf8" }).trim(); + for (const [why, options] of [ + ["none", {}], + ["settle refuses", { last: lastReport(), settle: 1 }], + ["settle crashes", { last: lastReport(), settle: 2 }], + ...["1", "2", "3", "4", "75"].map((code) => [`exit ${code}`, { last: lastReport(), code }]), + ]) { + const run = runIdleCheck(options); + assert.equal(run.ran, true, why); + assert.match(run.output, /^code=3$/m, why); + assert.deepEqual(JSON.parse(run.report), { ran: true }, why); + assert.equal(run.keptSha, head, why); + if (/^exit /.test(why)) assert.equal(run.settled, null, `${why} is not offered to settle`); + } +}); + +test("post-merge-dispatch reads the last run's report before its check, from a trusted run alone", () => { + const list = stepsOf(jobBlock("post-merge-dispatch")); + const names = list.map((s) => s.name); + const read = step("Read the post-merge report the last run kept", list); + assert.equal(names.indexOf("Read the post-merge report the last run kept") + 1, names.indexOf("Check earlier lane merges")); + assert.match(read, /\n {8}if: steps\.mode\.outputs\.live == 'true'\n {8}continue-on-error: true\n/); + assert.ok(read.includes("LAST_REPORT_DIR: ${{ runner.temp }}/post-merge-last\n")); + assert.match(read, /\n {8}run: node \.base\/\.github\/scripts\/pr-merge\/last-report\.mjs\n/); +}); + // The merge job reuses the kept report only while the check's code at its // own commit reads as it did here: the report keeps the commit it ran. test("the early check keeps the commit whose code ran it beside its report", () => { diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 1ce3588696..234c66bb4d 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -335,6 +335,7 @@ test("the merge and enable-auto-merge jobs' kept-file readers share their reads" for (const [id, scripts] of [ ["merge", ["rotation.mjs", "head-ref-map.mjs read", "lane-records.mjs read"]], ["enable-auto-merge", ["head-ref-map.mjs read", "lane-records.mjs read", "verify-answers.mjs"]], + ["post-merge-dispatch", ["lane-records.mjs read", "last-report.mjs"]], ]) { for (const script of scripts) { const step = new RegExp(`\\n {10}PR_MERGE_READS_PATH: (.*)\\n {8}run: node \\.base/\\.github/scripts/pr-merge/${script.replace(".", "\\.")}\\n`).exec(job(id)); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 6b7c137e22..05f399894c 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -175,8 +175,25 @@ jobs: env: ACTIONS_TOKEN: ${{ github.token }} LANE_RECORDS_PATH: ${{ runner.temp }}/lane-records.json + PR_MERGE_READS_PATH: ${{ runner.temp }}/pr-merge-reads.json run: node .base/.github/scripts/pr-merge/lane-records.mjs read + # The post-merge report the last scheduled or hand-started run kept + # (its pr-merge-post-merge-report artifact), which the check below + # reuses while nothing it reads has moved. Read only from a trusted run + # on main, as the lane records are. Rules: + # .github/scripts/pr-merge/last-report.mjs. Any error leaves no report, + # and the check runs. + - name: Read the post-merge report the last run kept + if: steps.mode.outputs.live == 'true' + continue-on-error: true + timeout-minutes: 2 + env: + ACTIONS_TOKEN: ${{ github.token }} + LAST_REPORT_DIR: ${{ runner.temp }}/post-merge-last + PR_MERGE_READS_PATH: ${{ runner.temp }}/pr-merge-reads.json + run: node .base/.github/scripts/pr-merge/last-report.mjs + - name: Check earlier lane merges id: post-merge if: steps.mode.outputs.live == 'true' @@ -186,11 +203,32 @@ jobs: REPO: ${{ github.repository }} run: | set -euo pipefail - identity=$(yq '.merge."agent-identity" // ""' .base/docs/conventions/pr-pipeline.yaml) - code=0 - python3 .base/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py \ - --repo "$REPO" --agent-identity "$identity" --lane-merges-only \ - "$RUNNER_TEMP/post-merge.json" || code=$? + last=$RUNNER_TEMP/post-merge-last + scripts=.base/plugins/source-control/skills/babysit-prs/scripts + code='' code_sha='' + # An idle tick: the last run's report, from a check that passed + # (exit 0 or 5), stands while the lane room's reuse rule keeps it + # and nothing else the check reads has moved (settle --idle-tick). + # Its computedAt, code and commit are kept as they were, so the + # reuse ends SHARED_MAX_AGE after the check that made it. + if [ -f "$last/post-merge.json" ] && [ -f "$last/post-merge-code" ]; then + last_code=$(cat "$last/post-merge-code") + last_sha=$(cat "$last/post-merge-code-sha" 2>/dev/null || true) + if [[ $last_code =~ ^[05]$ ]] && + python3 "$scripts/babysit_verify_evidence.py" settle --idle-tick --repo "$REPO" \ + --postmerge-result "$last/post-merge.json" --code-sha "$last_sha"; then + cp "$last/post-merge.json" "$RUNNER_TEMP/post-merge.json" + code=$last_code code_sha=$last_sha + echo "::notice::the last run's post-merge check (exit $code) still stands: nothing it reads has moved; reused, not run again" + fi + fi + if [ -z "$code" ]; then + identity=$(yq '.merge."agent-identity" // ""' .base/docs/conventions/pr-pipeline.yaml) + code=0 + python3 "$scripts/babysit_postmerge.py" \ + --repo "$REPO" --agent-identity "$identity" --lane-merges-only \ + "$RUNNER_TEMP/post-merge.json" || code=$? + fi echo "::group::post-merge check: exit $code" cat "$RUNNER_TEMP/post-merge.json" echo "::endgroup::" @@ -225,6 +263,8 @@ jobs: cp "$RUNNER_TEMP/post-merge.json" "$kept/post-merge.json" echo "$code" >"$kept/post-merge-code" git -C .base rev-parse HEAD >"$kept/post-merge-code-sha" || true + # A reused report keeps the commit whose code made it. + [ -z "$code_sha" ] || echo "$code_sha" >"$kept/post-merge-code-sha" [ ! -f "$RUNNER_TEMP/lane-records.json" ] || cp "$RUNNER_TEMP/lane-records.json" "$kept/lane-records.json" # A rate-limit stop fails the job red once the outputs are written. [ "$code" != 75 ] || exit 1 @@ -234,7 +274,8 @@ jobs: # lane room's reuse rule keeps it (babysit_verify_evidence.py settle) # and the check's code at that job's commit reads as at the kept one: # neither runs the check again then. They download it from this run - # only. + # only. The next run's post-merge-dispatch reads it too (the step + # above), and reuses it while it is younger than SHARED_MAX_AGE. - name: Keep the post-merge report for enable-auto-merge if: steps.post-merge.conclusion == 'success' continue-on-error: true diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 1224401ff4..1092e37a57 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -4431,7 +4431,7 @@ jobs: } if [[ $AUTHOR_ID =~ ^[0-9]+$ && " $exempt_author_ids " == *" $AUTHOR_ID "* ]]; then # The trust-gate paths, as bash patterns (`*` crosses `/`). - hold_patterns=('.github/standards/trusted-actors/*' '.github/actions/check-trusted-trigger/*' '.github/actions/resolve-config/*' '.github/actions/request-lane-token/*' '.github/actions/check-kill-switch/*' '.github/actions/log-api-budget/rate-guard.mjs' '.github/actions/log-api-budget/rate-limits.json' '.github/scripts/pr-merge/merge-holds.mjs' '.github/scripts/pr-merge/enable-auto-merge.mjs' '.github/scripts/pr-merge/superseded.mjs' '.github/scripts/pr-merge/clear-escalations.mjs' '.github/scripts/pr-merge/carry-run-cache.mjs' '.github/scripts/pr-merge/gate-check.mjs' '.github/scripts/pr-merge/head-ref-map.mjs' '.github/scripts/pr-merge/lane-records.mjs' '.github/scripts/pr-merge/rate-stop.bash' '.github/scripts/pr-merge/refire-refused.mjs' '.github/scripts/pr-merge/rotation.mjs' '.github/scripts/pr-merge/sweep-summary.mjs' '.github/scripts/pr-merge/who.mjs' '.github/scripts/pr-merge/post-unarmed-status.mjs' '.github/scripts/pr-merge/verify-answers.mjs' '.github/scripts/pr-unblock/address-feedback.mjs' '.github/scripts/pr-unblock/gate-source.mjs' '.github/scripts/pr-unblock/thread-scope.mjs' '.github/scripts/pr-unblock/retry-fetch.mjs' '.github/scripts/work-resolve-issue/report-outcome.mjs' '.github/scripts/work-resolve-issue/limits.mjs' '.github/scripts/work-resolve-issue/retry-fetch.mjs' '.github/workflows/pr-require-checks.yml' '.github/workflows/pr-merge.yml' 'plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_check_inputs.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_checks.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_dependabot.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_repo_config.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_run_cache.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_state.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_util.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_whole_review.py' 'docs/conventions/pr-pipeline.yaml' 'CODEOWNERS' '*/CODEOWNERS' '.gitmodules' '*/.gitmodules') + hold_patterns=('.github/standards/trusted-actors/*' '.github/actions/check-trusted-trigger/*' '.github/actions/resolve-config/*' '.github/actions/request-lane-token/*' '.github/actions/check-kill-switch/*' '.github/actions/log-api-budget/rate-guard.mjs' '.github/actions/log-api-budget/rate-limits.json' '.github/scripts/pr-merge/merge-holds.mjs' '.github/scripts/pr-merge/enable-auto-merge.mjs' '.github/scripts/pr-merge/superseded.mjs' '.github/scripts/pr-merge/clear-escalations.mjs' '.github/scripts/pr-merge/carry-run-cache.mjs' '.github/scripts/pr-merge/gate-check.mjs' '.github/scripts/pr-merge/head-ref-map.mjs' '.github/scripts/pr-merge/lane-records.mjs' '.github/scripts/pr-merge/last-report.mjs' '.github/scripts/pr-merge/rate-stop.bash' '.github/scripts/pr-merge/refire-refused.mjs' '.github/scripts/pr-merge/rotation.mjs' '.github/scripts/pr-merge/sweep-summary.mjs' '.github/scripts/pr-merge/who.mjs' '.github/scripts/pr-merge/post-unarmed-status.mjs' '.github/scripts/pr-merge/verify-answers.mjs' '.github/scripts/pr-unblock/address-feedback.mjs' '.github/scripts/pr-unblock/gate-source.mjs' '.github/scripts/pr-unblock/thread-scope.mjs' '.github/scripts/pr-unblock/retry-fetch.mjs' '.github/scripts/work-resolve-issue/report-outcome.mjs' '.github/scripts/work-resolve-issue/limits.mjs' '.github/scripts/work-resolve-issue/retry-fetch.mjs' '.github/workflows/pr-require-checks.yml' '.github/workflows/pr-merge.yml' 'plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_check_inputs.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_checks.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_dependabot.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_repo_config.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_run_cache.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_state.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_util.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_whole_review.py' 'docs/conventions/pr-pipeline.yaml' 'CODEOWNERS' '*/CODEOWNERS' '.gitmodules' '*/.gitmodules') if [[ ! $PR_NUMBER =~ ^[1-9][0-9]*$ ]]; then echo "::error::the pull request number '$PR_NUMBER' is not a number." exit 1 diff --git a/.github/workflows/pr-review-hosted.yml b/.github/workflows/pr-review-hosted.yml index 8674a1c918..47e8d30a30 100644 --- a/.github/workflows/pr-review-hosted.yml +++ b/.github/workflows/pr-review-hosted.yml @@ -215,6 +215,7 @@ jobs: .github/scripts/pr-merge/gate-check.mjs .github/scripts/pr-merge/head-ref-map.mjs .github/scripts/pr-merge/lane-records.mjs + .github/scripts/pr-merge/last-report.mjs .github/scripts/pr-merge/rate-stop.bash .github/scripts/pr-merge/refire-refused.mjs .github/scripts/pr-merge/rotation.mjs diff --git a/.github/workflows/pr-review-security-hosted.yml b/.github/workflows/pr-review-security-hosted.yml index 47fb86da95..496e0bc2a6 100644 --- a/.github/workflows/pr-review-security-hosted.yml +++ b/.github/workflows/pr-review-security-hosted.yml @@ -215,6 +215,7 @@ jobs: .github/scripts/pr-merge/gate-check.mjs .github/scripts/pr-merge/head-ref-map.mjs .github/scripts/pr-merge/lane-records.mjs + .github/scripts/pr-merge/last-report.mjs .github/scripts/pr-merge/rate-stop.bash .github/scripts/pr-merge/refire-refused.mjs .github/scripts/pr-merge/rotation.mjs diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index b409e0b706..e68cf2f715 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1212,7 +1212,9 @@ gate's `` comment and no label, since a pu count; an unread count leaves it unarmed with no comment, and the next run reads it again. An armed one is not disarmed on the count. Each such run also waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge -report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it +report that job keeps as an artifact (on an idle tick the last run's report, which that job reuses +unchanged while nothing its check reads has moved, `babysit_verify_evidence.py settle --idle-tick`; +`babysit_verify_evidence.py lane-room`; with no report it holds as a refusal does; one the gate would not reuse, for any `shared_refusal`, is checked again as the gate does; one refused only because a demotion record was filed or changed since is first kept when the demotion records alone read as they were; one past the gate's diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 5f7cfc15ec..64e490df4a 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -4653,11 +4653,19 @@ def _demotion_changed( def _shared_age_refusal(shared: SharedPostMerge, age: timedelta) -> str | None: - """`stale` past `SHARED_MAX_AGE` (or from the future), `unverified-stale` - past `SHARED_UNVERIFIED_MAX_AGE` while it passes with a merge unverified, - else None.""" + """`stale` past `SHARED_MAX_AGE` (or from the future), `excuse-expired` + once a quarantine that excused a failing suite has expired + (`excusesExpireAt`, or one that cannot be read), after which the same + red run demotes, `unverified-stale` past `SHARED_UNVERIFIED_MAX_AGE` + while it passes with a merge unverified, else None.""" if not timedelta(0) <= age <= SHARED_MAX_AGE: return "stale" + excuse = shared.gate.report.get("excusesExpireAt") + try: + if excuse is not None and shared.computed_at + age >= _time(excuse): + return "excuse-expired" + except PolicyReadError: + return "excuse-expired" # A refusing answer refuses whatever was verified since, so only a # passing one with a merge still unverified is read again. if ( @@ -4688,7 +4696,8 @@ def shared_refusal( None: it must name the same identity, self logins and lane, give this base the floor its own read would (`merged_since_floor` with the shared demotion), be no older than `SHARED_MAX_AGE` (`SHARED_UNVERIFIED_MAX_AGE` - while it passes with a merge unverified), have no demotion record filed + while it passes with a merge unverified) nor past a quarantine excuse's + end (`_shared_age_refusal`), have no demotion record filed or changed since (`_demotion_changed`), and judge every merge the post-merge check would judge now. The demotion comes last, so `demotion-changed` means nothing else refused (`lane_room` keeps such an diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index b6328c60ab..3dcc7c0ea7 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -56,7 +56,7 @@ import re import sys import tempfile -from collections.abc import Callable, Collection, Iterable +from collections.abc import Callable, Collection, Iterable, Mapping from dataclasses import dataclass, field from datetime import UTC, datetime, timedelta from pathlib import Path @@ -305,6 +305,9 @@ class PostMergeVerdict: recover_dispatch: bool = False # When the earliest quarantine that excused a failing suite expires. excuses_expire_at: datetime | None = None + # Each pin the check read, by merge number (None: no pin), which a later + # tick reads again before it reuses the report (`idle_refusal`). + pins: Mapping[int, str | None] = field(default_factory=dict) @property def ok(self) -> bool: @@ -375,6 +378,7 @@ def report(self) -> dict[str, Any]: "excusesExpireAt": ( self.excuses_expire_at.isoformat() if self.excuses_expire_at else None ), + "pins": {str(n): sha for n, sha in sorted(self.pins.items())}, } @@ -2261,15 +2265,18 @@ def lane_prs_at(sha: str) -> str | None: prefetch( [m.number for m in merges if m.number in pinned_merges | optional_pins] ) + pins = { + m.number: pinned_sha(m) + for m in merges + if pinned_sha and m.number in pinned_merges | optional_pins + } states = tuple( _judge( merge, ctx, full_runs=full_runs, canary_runs=canary_runs, - pinned=pinned_sha(merge) - if pinned_sha and merge.number in pinned_merges | optional_pins - else None, + pinned=pins.get(merge.number), pin_required=merge.number in pinned_merges, now=now, tally=tally, @@ -2300,6 +2307,7 @@ def lane_prs_at(sha: str) -> str | None: branch=branch, quarantines=tuple(ctx.quarantines.values()), excuses_expire_at=ctx.excuses_expire_at, + pins=pins, ) @@ -3314,6 +3322,51 @@ def post_merge(floor: datetime, identity: str) -> policy.PostMergeGate: ) +def _verifying_runs_unchanged( + repo: str, branch: str, *, created: datetime, since: datetime +) -> bool: + """Whether no full or canary run on `branch` created at or after + `created` completed, or was re-run, at or after `since`, from one listing + per verifying workflow: every workflow's runs on a busy branch fill more + than one page in an hour or two. A run started since and still running + changes nothing yet. A listing that is not whole raises + `PostMergeReadError`.""" + owner, name = _split_repo(repo) + floor = quote( + f">={created.astimezone(UTC).strftime('%Y-%m-%dT%H:%M:%SZ')}", safe="" + ) + for workflow in (FULL_RUN_WORKFLOW, CANARY_WORKFLOW): + payload = json_object( + _read( + [ + "api", + f"repos/{owner}/{name}/actions/workflows/{workflow}/runs" + f"?branch={quote(branch, safe='')}&created={floor}&per_page=100", + ], + f"{workflow} runs", + ) + ) + runs = json_array(payload.get("workflow_runs")) + if payload.get("total_count") != len(runs): + raise PostMergeReadError(f"{workflow} runs: the listing is not whole") + if any( + run.get("path") in VERIFYING_WORKFLOWS + and run.get("status") == "completed" + and _time(run.get("updated_at")) >= since + for run in map(json_object, runs) + ): + return False + return True + + +def _pending_merges(report: Mapping[str, Any]) -> list[dict[str, Any]]: + return [ + m + for m in map(json_object, json_array(report.get("autoMerges"))) + if m.get("verified") is not True + ] + + def pending_runs_unchanged( repo: str, shared: policy.SharedPostMerge, @@ -3321,22 +3374,17 @@ def pending_runs_unchanged( *, now: datetime | None = None, ) -> bool: - """Whether `check` would judge `shared`'s unverified merges as it did, - from one run listing: no full or canary run on `branch` created since the - earliest of them completed or was re-run since `computed_at` (less - `policy.SINCE_CLOCK_MARGIN`), none has passed `DISPATCH_DEADLINE`, after - which a missing dispatch or pin is a failure, and no quarantine that - excused a failing suite has expired (`excusesExpireAt`; an answer - without the field is False). Those are `_judge`'s time-based excuses. A - run started since and still running only stays pending. A listing that - cannot be read or is not whole, or an entry without a merge time, is - False.""" - pending = [ - m - for m in map(json_object, json_array(shared.gate.report.get("autoMerges"))) - if m.get("verified") is not True - ] + """Whether `check` would judge `shared`'s unverified merges as it did: + no full or canary run on `branch` created since the earliest of them + completed or was re-run since `computed_at` (less + `policy.SINCE_CLOCK_MARGIN`; `_verifying_runs_unchanged`), none has + passed `DISPATCH_DEADLINE`, after which a missing dispatch or pin is a + failure, and no quarantine that excused a failing suite has expired + (`excusesExpireAt`; an answer without the field is False). Those are + `_judge`'s time-based excuses. A listing that cannot be read or is not + whole, or an entry without a merge time, is False.""" report = shared.gate.report + pending = _pending_merges(report) if not pending or "excusesExpireAt" not in report: return False now = now or datetime.now(UTC) @@ -3347,34 +3395,130 @@ def pending_runs_unchanged( excuse is not None and now >= _time(excuse) ): return False - owner, name = _split_repo(repo) - created = quote( - f">={earliest.astimezone(UTC).strftime('%Y-%m-%dT%H:%M:%SZ')}", safe="" - ) - payload = json_object( - _read( - [ - "api", - f"repos/{owner}/{name}/actions/runs?branch={quote(branch, safe='')}" - f"&created={created}&per_page=100", - ], - "verifying runs", - ) - ) - runs = json_array(payload.get("workflow_runs")) - if payload.get("total_count") != len(runs): - return False - since = shared.computed_at - policy.SINCE_CLOCK_MARGIN - return not any( - run.get("path") in VERIFYING_WORKFLOWS - and run.get("status") == "completed" - and _time(run.get("updated_at")) >= since - for run in map(json_object, runs) + return _verifying_runs_unchanged( + repo, + branch, + created=earliest, + since=shared.computed_at - policy.SINCE_CLOCK_MARGIN, ) except PostMergeReadError: return False +# How much of `policy.SHARED_MAX_AGE` a report reused from an earlier tick +# must have left: the merge gate (`assess`, per candidate) and the lane room +# read the same report after post-merge-dispatch ends, and one past the age +# sends them to the full check. Over 72 scheduled runs on 2026-10-26/27 the +# gate ended a median 17.7 minutes after a reuse tick's check would start; +# replayed on those runs, 10 minutes reuses as often as 5 and leaves fewer +# gates past the age. +IDLE_HEADROOM = timedelta(minutes=10) +QUARANTINE_MARKER = "\nbody"} + self.assertEqual(self.refusal(issues=[marked]), "quarantine-changed") + # A full page may hide one. + page = [{"number": n, "body": ""} for n in range(100)] + self.assertEqual(self.refusal(issues=page), "quarantine-changed") + # An issue without the marker, or a pull request quoting it, is not one. + other = [{"number": 4, "body": "triaged"}, {**marked, "pull_request": {}}] + self.assertIsNone(self.refusal(issues=other)) + + def test_a_pin_that_reads_differently_refuses(self) -> None: + # An edited pin reads as None, a deleted one too, a new one another SHA. + for pins in ({5: None}, {5: "c" * 40}): + with self.subTest(pins): + self.assertEqual(self.refusal(pins=pins), "pins-changed") + failed = self.refusal(pins=pm.PostMergeReadError("#5 pin: 502")) + self.assertEqual(failed, "unreadable: #5 pin: 502") + class CheckCodeUnchanged(unittest.TestCase): """`check_code_unchanged`: whether the check's code (the babysit-prs @@ -1029,8 +1221,11 @@ def main( cfg: policy.PipelineConfig | None = None, config_error: Exception | None = None, code_sha: str = MAIN, + idle: str | None = None, + extra: tuple[str, ...] = (), ) -> tuple[int, dict, dict]: seen: dict[str, object] = {} + self.idle = mock.Mock(return_value=idle) def settle(repo: str, answer: object, **kwargs: object) -> object: seen.update(kwargs, repo=repo, answer=answer) @@ -1050,6 +1245,7 @@ def settle(repo: str, answer: object, **kwargs: object) -> object: return_value=cfg or config(base_sha=NEW_MAIN), ) as read_config, mock.patch.object(policy, "settle_shared", side_effect=settle), + mock.patch.object(ve.postmerge, "idle_refusal", self.idle), contextlib.redirect_stdout(out), ): exit_code = ve.main( @@ -1061,6 +1257,7 @@ def settle(repo: str, answer: object, **kwargs: object) -> object: "/tmp/x.json", "--code-sha", code_sha, + *extra, ] ) self.code, self.read_config = code, read_config @@ -1133,6 +1330,29 @@ def test_a_missing_report_or_unreadable_config_is_refused(self) -> None: self.assertEqual(code, 1) self.assertIn("head 502", out["reason"]) + def test_an_idle_tick_also_needs_the_idle_rule(self) -> None: + # post-merge-dispatch's reuse of an earlier tick's report: the lane + # room's rule first, then `idle_refusal` on the same answer, at the + # default branch with the head config's confirm wait. + idle = ("--idle-tick",) + code, out, _ = self.main(("answer", None), extra=idle) + self.assertEqual((code, out), (0, {"reuse": True, "reason": None})) + self.idle.assert_called_once_with( + REPO, "answer", "main", confirm_wait_minutes=config().confirm_wait_minutes + ) + code, out, _ = self.main(("answer", None), extra=idle, idle="runs-changed") + self.assertEqual( + (code, out), (1, {"reuse": False, "reason": "idle tick runs-changed"}) + ) + # A report the lane room's rule refuses is never offered to it. + code, out, _ = self.main((None, "post-merge answer stale"), extra=idle) + self.assertEqual(code, 1) + self.idle.assert_not_called() + # Without the flag (the merge job's reuse of this run's report), no + # idle rule. + self.assertEqual(self.main(("answer", None), idle="runs-changed")[0], 0) + self.idle.assert_not_called() + class LaneRoomCommand(unittest.TestCase): def test_it_prints_the_room_from_the_post_merge_report(self) -> None: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py index e891547b01..ca512bcb0a 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py @@ -1028,6 +1028,16 @@ def test_a_pin_at_the_merged_head_verifies(self) -> None: self.assertEqual(verdict.merges[0].pending, ()) self.assertTrue(verdict.merges[0].verified) + def test_the_report_records_each_pin_it_read_once(self) -> None: + # A later tick reads them again before it reuses the report + # (`idle_refusal`), so the record is the read itself, None included. + self.all_green() + for pin in (HEAD_SHA, None): + with self.subTest(pin=pin): + report = self.check_unpinned(pin=pin).report() + self.assertEqual(report["pins"], {"7001": pin}) + self.assertEqual(self.pin_reads, [7001]) + def test_a_pin_at_another_head_fails_the_read_back(self) -> None: # A push after the arm: the merged head is not the one pinned. self.all_green() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py index b08074a324..2f8d5ef0b6 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py @@ -296,6 +296,7 @@ def test_floor_paths_are_high_whatever_the_switch(self): (".github/scripts/pr-merge/gate-check.mjs", "gate"), (".github/scripts/pr-merge/head-ref-map.mjs", "gate"), (".github/scripts/pr-merge/lane-records.mjs", "gate"), + (".github/scripts/pr-merge/last-report.mjs", "gate"), (".github/scripts/pr-merge/rate-stop.bash", "gate"), (".github/scripts/pr-merge/refire-refused.mjs", "gate"), (".github/scripts/pr-merge/rotation.mjs", "gate"), diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index 127cdc8f71..0b7356c553 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -226,7 +226,7 @@ def test_the_floor_is_the_same_on_both_sides(self) -> None: self.assertEqual(answer["classes"], [list(row) for row in risk.FLOOR_CLASSES]) self.assertEqual(answer["floor"], list(risk.HIGH_FLOOR)) self.assertEqual(answer["review"], list(risk.REVIEW_FLOOR)) - self.assertEqual(len(risk.FLOOR_CLASSES), 51) + self.assertEqual(len(risk.FLOOR_CLASSES), 52) self.assertEqual(len(risk.REVIEW_FLOOR), 21) self.assertEqual( {kind for _, kind in risk.FLOOR_CLASSES}, {"trust", "gate", "enforcement"} From 0a67719f0d44c711bcd025f9f52f948bcc4dfe2e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:36:59 -0400 Subject: [PATCH 2/2] fix(pr-merge): list runs back to a canary still queued in a merge's window An idle tick could reuse a passing report while an older canary in a verified merge's window was still queued: `_judge` verifies on a later green canary, and the queued one fails the merge once it turns red. Only unverified merges moved the run listing's floor past the fixed lookback, so a canary created before it was never listed and the reuse stood. The post-merge report now records when the earliest canary still running in a merge's window was created (`outstandingSince`). `idle_refusal` and `pending_runs_unchanged` move their listing floor back to it and refuse a report without the field. No extra API calls: the floor moves inside the same two per-workflow listings; a listing that no longer fits one page reads as not whole and runs the full check. Co-Authored-By: Claude Opus 5.5 --- .../perf-postmerge-idle-tick-5c2e81d4.md | 2 +- .../babysit-prs/scripts/babysit_postmerge.py | 46 ++++++++++++------ .../scripts/tests/test_babysit_lane_room.py | 48 ++++++++++++++++++- .../scripts/tests/test_babysit_postmerge.py | 21 ++++++++ 4 files changed, 100 insertions(+), 17 deletions(-) diff --git a/.changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md b/.changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md index 9530ce38cc..e525fa2daa 100644 --- a/.changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md +++ b/.changes/source-control/perf-postmerge-idle-tick-5c2e81d4.md @@ -4,6 +4,6 @@ bump: patch ### Changed -- `babysit_verify_evidence.py settle --idle-tick` lets pr-merge's post-merge-dispatch job reuse the post-merge report the last tick kept. The report must pass the lane room's reuse rule (`settle_shared`) and `babysit_postmerge.idle_refusal`. That rule takes a passing report only (exit 0 or 5) with no hold, no full run or re-run to start, no unattributed red run and no quarantine excuse, and at least 10 minutes left before `SHARED_MAX_AGE`. It also requires that since the report no verifying run completed, no quarantine issue changed, and every pin the check read reads the same again. The post-merge report now records those pins (`pins`). +- `babysit_verify_evidence.py settle --idle-tick` lets pr-merge's post-merge-dispatch job reuse the post-merge report the last tick kept. The report must pass the lane room's reuse rule (`settle_shared`) and `babysit_postmerge.idle_refusal`. That rule takes a passing report only (exit 0 or 5) with no hold, no full run or re-run to start, no unattributed red run and no quarantine excuse, and at least 10 minutes left before `SHARED_MAX_AGE`. It also requires that since the report no verifying run completed, no quarantine issue changed, and every pin the check read reads the same again. The post-merge report now records those pins (`pins`) and when the earliest canary still running in a merge's window was created (`outstandingSince`), which can still turn red and fail a verified merge. Both `idle_refusal` and `pending_runs_unchanged` list verifying runs back to that canary, and refuse a report without the field. - `pending_runs_unchanged` lists runs once for each verifying workflow instead of once for every workflow on the branch. On main, the all-workflow listing held 152 runs in under two hours, more than fit on one page. - The reuse rule (`shared_refusal`) refuses a report once a quarantine excuse it carries has expired (`excuse-expired`). diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index 3dcc7c0ea7..0ce39241c0 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -308,6 +308,10 @@ class PostMergeVerdict: # Each pin the check read, by merge number (None: no pin), which a later # tick reads again before it reuses the report (`idle_refusal`). pins: Mapping[int, str | None] = field(default_factory=dict) + # When the earliest canary in a merge's window still running was created: + # it can still turn red and fail a merge judged verified, so a later tick + # lists runs back to it before it reuses the report (`idle_refusal`). + outstanding_since: datetime | None = None @property def ok(self) -> bool: @@ -379,6 +383,9 @@ def report(self) -> dict[str, Any]: self.excuses_expire_at.isoformat() if self.excuses_expire_at else None ), "pins": {str(n): sha for n, sha in sorted(self.pins.items())}, + "outstandingSince": ( + self.outstanding_since.isoformat() if self.outstanding_since else None + ), } @@ -1901,6 +1908,7 @@ class _Tally: reruns: dict[int, Rerun] = field(default_factory=dict) flakes: dict[str, None] = field(default_factory=dict) unattributed: dict[str, None] = field(default_factory=dict) + outstanding: list[datetime] = field(default_factory=list) def _judge( @@ -2016,15 +2024,13 @@ def passes(run: Run) -> bool: elif state == PENDING: pending.append(f"{signal}-run-in-progress:{run.id}") for run in canaries: - if in_window(run) and ctx.outcome(run, attributable=False)[0] == RED: - failures.append( - Failure( - SIGNAL_CANARY, - merge, - ctx.sighting(run), - ctx.outcome(run, attributable=False)[1], - ) - ) + if not in_window(run): + continue + state, detail = ctx.outcome(run, attributable=False) + if state == RED: + failures.append(Failure(SIGNAL_CANARY, merge, ctx.sighting(run), detail)) + elif state == PENDING: + tally.outstanding.append(run.created_at) deadline = merged_at + DISPATCH_DEADLINE if not any(r.created_at <= deadline for r in own) and now > deadline: @@ -2308,6 +2314,7 @@ def lane_prs_at(sha: str) -> str | None: quarantines=tuple(ctx.quarantines.values()), excuses_expire_at=ctx.excuses_expire_at, pins=pins, + outstanding_since=min(tally.outstanding, default=None), ) @@ -3381,11 +3388,14 @@ def pending_runs_unchanged( passed `DISPATCH_DEADLINE`, after which a missing dispatch or pin is a failure, and no quarantine that excused a failing suite has expired (`excusesExpireAt`; an answer without the field is False). Those are - `_judge`'s time-based excuses. A listing that cannot be read or is not + `_judge`'s time-based excuses. The listing reaches back to the earliest + canary still running in a verified merge's window too + (`outstandingSince`; an answer without the field is False), which can + still turn red and fail it. A listing that cannot be read or is not whole, or an entry without a merge time, is False.""" report = shared.gate.report pending = _pending_merges(report) - if not pending or "excusesExpireAt" not in report: + if not pending or not {"excusesExpireAt", "outstandingSince"} <= report.keys(): return False now = now or datetime.now(UTC) try: @@ -3395,10 +3405,13 @@ def pending_runs_unchanged( excuse is not None and now >= _time(excuse) ): return False + outstanding = report["outstandingSince"] return _verifying_runs_unchanged( repo, branch, - created=earliest, + created=earliest + if outstanding is None + else min(earliest, _time(outstanding)), since=shared.computed_at - policy.SINCE_CLOCK_MARGIN, ) except PostMergeReadError: @@ -3460,8 +3473,10 @@ def idle_refusal( - an unverified merge past `DISPATCH_DEADLINE`; - verifying runs: none completed since `computedAt` (less `SINCE_CLOCK_MARGIN`) among those created since the earliest unverified - merge or the `_lane_reruns` lookback (`red-main.confirm-wait-minutes` - plus `FULL_RUN_ALLOWANCE`, once more for a baseline run still going); + merge, the earliest canary still running in a merge's window + (`outstandingSince`), or the `_lane_reruns` lookback + (`red-main.confirm-wait-minutes` plus `FULL_RUN_ALLOWANCE`, once more + for a baseline run still going); - quarantine issues: none carrying the marker changed since; - pins: each recorded one reads the same again, so an edited, new or deleted pin refuses. @@ -3486,6 +3501,7 @@ def idle_refusal( "lane-reruns-unread", ), (not is_json_object(report.get("pins")), "pins-unrecorded"), + ("outstandingSince" not in report, "outstanding-unrecorded"), ( now + IDLE_HEADROOM - shared.computed_at > policy.SHARED_MAX_AGE, "stale-soon", @@ -3505,6 +3521,8 @@ def idle_refusal( if now > earliest + DISPATCH_DEADLINE: return "dispatch-deadline" created = min(created, earliest) + if report["outstandingSince"] is not None: + created = min(created, _time(report["outstandingSince"])) if not _verifying_runs_unchanged(repo, branch, created=created, since=since): return "runs-changed" if _quarantine_touched(repo, since): diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py index fe5492a122..e0da9604d2 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -23,6 +23,7 @@ from dataclasses import replace from datetime import UTC, datetime, timedelta from unittest import mock +from urllib.parse import unquote sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) @@ -54,6 +55,7 @@ {"number": 5, "verified": False, "mergedAt": "2026-10-01T00:00:00Z"} ], "excusesExpireAt": None, + "outstandingSince": None, } STALE_NOW = FLOOR + timedelta(minutes=3) @@ -785,6 +787,20 @@ def test_a_conclusion_since_a_partial_listing_or_the_deadline_moves_it( ] ) + def test_a_canary_still_queued_in_a_verified_window_is_listed(self) -> None: + # A verified merge's canary queued before the unverified merge can + # still turn red and fail it, so the listing reaches back to it; an + # answer that does not say whether one is outstanding is False. + queued = FLOOR - timedelta(hours=1) + answer, calls = self.unchanged( + [], report={**PENDING, "outstandingSince": queued.isoformat()} + ) + self.assertTrue(answer) + for call in calls: + self.assertIn("created=%3E%3D2026-09-30T23%3A00%3A00Z", call[1]) + unsaid = {k: v for k, v in PENDING.items() if k != "outstandingSince"} + self.assertEqual(self.unchanged([], report=unsaid), (False, [])) + class SettleShared(unittest.TestCase): """`settle_shared`: the one reuse rule the room and the merge job's @@ -973,6 +989,7 @@ def excused(until: object) -> policy.SharedPostMerge: "warnings": [], "excusesExpireAt": None, "pins": {"5": SHA_PINNED}, + "outstandingSince": None, } CONFIRM_WAIT = 40 @@ -1000,9 +1017,18 @@ def refusal( def read(args: list[str], label: str) -> object: self.calls.append(args[1]) if "/actions/workflows/" in args[1]: + # The listing answers only runs created at or after its floor. + floor = datetime.fromisoformat( + unquote(args[1].split("&created=")[1].split("&")[0])[2:] + ) + listed = [ + r + for r in runs + if datetime.fromisoformat(str(r["created_at"])) >= floor + ] return { - "total_count": len(runs) if runs_total is None else runs_total, - "workflow_runs": list(runs), + "total_count": len(listed) if runs_total is None else runs_total, + "workflow_runs": listed, } return list(issues) @@ -1059,6 +1085,7 @@ def test_each_unsettled_part_of_the_report_refuses_before_any_read( for key, reason in ( ("excusesExpireAt", "excused"), ("pins", "pins-unrecorded"), + ("outstandingSince", "outstanding-unrecorded"), ): with self.subTest(f"no {key}"): report = {k: v for k, v in SETTLED.items() if k != key} @@ -1109,6 +1136,23 @@ def test_an_unverified_merge_lists_runs_from_its_merge_and_ends_at_the_deadline( with mock.patch.object(policy, "SHARED_MAX_AGE", timedelta(days=1)): self.assertEqual(self.refusal(pending, now=late), "dispatch-deadline") + def test_a_canary_still_queued_at_the_report_is_listed_however_old( + self, + ) -> None: + # A verified merge's window holds a canary queued before the green + # one that verified it; `_judge` fails the merge once it turns red, + # however long before the `_lane_reruns` lookback it was created. + queued = FLOOR - timedelta(hours=4) + report = {**SETTLED, "outstandingSince": queued.isoformat()} + red = run( + "post-merge-detect-silent-revert.yml", + status="completed", + updated=NOW, + created=queued, + ) + self.assertEqual(self.refusal(report, runs=[red]), "runs-changed") + self.assertIn("created=%3E%3D2026-09-30T20%3A00%3A00Z", self.calls[0]) + def test_a_quarantine_issue_changed_since_refuses(self) -> None: marked = {"number": 3, "body": "\nbody"} self.assertEqual(self.refusal(issues=[marked]), "quarantine-changed") diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py index ca512bcb0a..887ee62531 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge.py @@ -760,6 +760,27 @@ def test_no_dispatch_run_within_six_hours_is_a_failure(self) -> None: after = self.check(now=datetime(2026, 10, 12, 16, 1, tzinfo=UTC)) self.assertEqual([f.signal for f in after.failures], ["dispatch"]) + def test_the_report_records_the_earliest_canary_still_queued_in_a_window( + self, + ) -> None: + # A canary queued before the green one that verified the merge is in + # its window and fails it once it turns red, so a later tick that + # reuses the report must list runs back to it (`idle_refusal`). + self.all_green() + self.assertIsNone(self.check().report()["outstandingSince"]) + self.gh.add_runs( + "post-merge-detect-silent-revert.yml", + "push", + canary_run( + 20, conclusion=None, status="queued", created_at="2026-10-12T10:00:10Z" + ), + ) + verdict = self.check() + self.assertTrue(verdict.merges[0].verified) + self.assertEqual( + verdict.report()["outstandingSince"], "2026-10-12T10:00:10+00:00" + ) + def test_red_canary_is_a_failure(self) -> None: self.gh.add_runs("pr-require-checks.yml", "workflow_dispatch", dispatch_run(11)) self.gh.add_runs(