diff --git a/.changes/pr-pipeline/perf-enable-auto-merge-batch-7c4e2a91.md b/.changes/pr-pipeline/perf-enable-auto-merge-batch-7c4e2a91.md new file mode 100644 index 0000000000..20264ea409 --- /dev/null +++ b/.changes/pr-pipeline/perf-enable-auto-merge-batch-7c4e2a91.md @@ -0,0 +1,19 @@ +--- +bump: minor +--- + +### Changed + +- enable-auto-merge leaves an unarmed lanes App pull request on its lane refusal (a risk-score hold + or a pr-unblock stop) with no read when nothing it reads has changed since the last reader run: + head, `updatedAt`, labels, check and status rollup, reviews, comments, review threads, rung and + main's code and config. It reuses `merge.skip-unchanged` (max-minutes, max-skips), never skips + past the refusal's escalation to a failing status, never skips an armed one, and logs + `enable-auto-merge: skip-unchanged skipped=N recorded=M keyed=K of L`. The records ride in the + `pr-merge-verify-answers` artifact. +- enable-auto-merge's discussion read lists the changed files, so a head with no rename reads no + REST files page. +- A `pull_request_target` run of pr-merge handles only the pull request its event names; the + 15-minute schedule keeps the full sweep. A pending event run no longer supersedes another one. +- The merge job lists its candidates with one `gh pr list` and a jq author filter in place of one + `gh pr list --author` call per author. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index a7bade51c7..8250023c90 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -166,6 +166,10 @@ // TRUSTED_ACTORS the base trusted-actors.json, for the owner of a cost // decision and the approvers who lift a hold path (default .base/.github/standards/trusted-actors/trusted-actors.json) // EVENT_NAME the run's event; only READER_EVENTS read verdicts and arm +// EVENT_PR a pull_request_target run's pull request number, the only one it handles +// EAM_SKIP_IN, EAM_SKIP_OUT the skip records the last reader run kept, and +// the file this one keeps (SKIPPABLE); no OUT, no skip +// SKIP_MAX_MINUTES, SKIP_MAX_SKIPS merge.skip-unchanged's limits; 0 in either: no skip // POSTMERGE_RESULT this run's babysit_postmerge.py report, for lane-room // POSTMERGE_CODE_SHA the commit whose code ran that report's check (empty // when unknown: lane-room reads the report again) @@ -195,7 +199,8 @@ // refusal keeps waiting for a person to remove the label. import { spawnSync } from "node:child_process"; -import { appendFileSync, readFileSync } from "node:fs"; +import { createHash } from "node:crypto"; +import { appendFileSync, readFileSync, renameSync, writeFileSync } from "node:fs"; import process from "node:process"; import { pathToFileURL } from "node:url"; @@ -279,6 +284,7 @@ export const DETAIL_QUERY = `query($owner: String!, $name: String!, $number: Int reviews(last: 100) { totalCount nodes { ${REVIEW_FIELDS} } } reviewThreads(last: 100) { totalCount nodes { ${THREAD_FIELDS} } } timelineItems(itemTypes: [AUTO_MERGE_DISABLED_EVENT], last: 1) { nodes { ... on AutoMergeDisabledEvent { actor { __typename login } } } } + files(first: 100) { totalCount nodes { path changeType } } } } }`; @@ -516,6 +522,19 @@ function trustedHumans(env) { } } +// The changed paths DETAIL_QUERY's `files` lists, or null when the REST +// listing must be read: a list cut at one page, or a rename or copy, whose +// old path only REST names (previous_filename). +export function listedFiles(files) { + const nodes = files?.nodes; + if (!Array.isArray(nodes) || files.totalCount !== nodes.length || nodes.length >= 100) { + return null; + } + return nodes.every((node) => typeof node?.path === "string" && typeof node.changeType === "string" && !["RENAMED", "COPIED"].includes(node.changeType)) + ? nodes.map((node) => node.path) + : null; +} + // The head's changed paths, null when a page did not read, and `capped` // when the listing has a page past MAX_FILE_PAGES, which no later read // completes. Exactly MAX_FILE_PAGES full pages read one page more to tell. @@ -1347,6 +1366,127 @@ export function decide(pull, { bot, humanGated, holdPaths = null, pathHolds = tr return armed ? { action: "none", reason: "already armed" } : { action: "arm", reason: "no hold" }; } +// Skip-unchanged, as babysit_merge_skip.py keys the merge gate's skip: a +// reader run keeps, for each unarmed lanes App pull request it left on a +// lane's refusal in SKIPPABLE with nothing written, the key of every input +// that refusal reads: its head, update time and labels, its head's check and +// status rollup, review decision and count, newest comments and review +// threads (none of which need move `updatedAt`), main's code and config, +// the resolved config and the rung the gate scores at. The next reader run leaves a pull request whose +// key still matches on that refusal with no read, for at most +// merge.skip-unchanged max-minutes from its judgment and max-skips runs in a +// row, and never past the time its lane comment turns into a failing status. +// A skip only repeats a refusal: it never arms. A verdict refusal is never +// kept, since a new pr-verify artifact changes nothing in the key. The +// records ride in the pr-merge-verify-answers artifact (EAM_SKIP_IN, _OUT). +export const SKIP_VERSION = 1; +export const SKIPPABLE = Object.freeze([READER_HOLDS.risk, "a pr-unblock lane stopped its head ("]); +// merge.skip-unchanged's defaults (babysit_merge_skip.py). +export const SKIP_MAX_MINUTES = 60; +export const SKIP_MAX_SKIPS = 3; +const SKIP_CHUNK = 20; +// The paths of main whose tree ids go in the key: the config this run reads +// and every script it, the verifier and the risk score run. +export const SKIP_MAIN_PATHS = Object.freeze([ + "docs/conventions/pr-pipeline.yaml", + ".work-item-tracker.json", + ".github/standards/trusted-actors/trusted-actors.json", + ".github/workflows/pr-merge.yml", + ".github/actions", + ".github/scripts", + "plugins/source-control", +]); +const SKIP_STATE_FIELDS = + "number headRefOid updatedAt reviewDecision reviews { totalCount } comments(last: 50) { totalCount nodes { databaseId updatedAt } } reviewThreads(first: 100) { totalCount nodes { isResolved isOutdated } } commits(last: 1) { nodes { commit { oid statusCheckRollup { contexts(first: 100) { totalCount nodes { __typename ... on CheckRun { databaseId name status conclusion } ... on StatusContext { context state } } } } } } }"; +export const SKIP_STATE_PREFIX = "query($o: String!, $r: String!) { repository(owner: $o, name: $r) {"; +export const skipStateQuery = (numbers) => `${SKIP_STATE_PREFIX} ${numbers.map((n) => `p${n}: pullRequest(number: ${n}) { ...s }`).join(" ")} } } fragment s on PullRequest { ${SKIP_STATE_FIELDS} }`; +const ZONED_TIME = /^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(\.\d+)?Z$/; + +// A merge.skip-unchanged limit: empty is `fallback`, a whole number is itself, +// anything else 0 (off). +export function skipLimit(text, fallback) { + const value = String(text ?? "").trim(); + if (value === "" || value === "null") return fallback; + return /^[0-9]{1,6}$/.test(value) ? Number(value) : 0; +} + +// main's tree ids of SKIP_MAIN_PATHS in the `.base` clone, or null. +export function mainInputsOf(base = ".base") { + const run = spawnSync("git", ["-C", base, "ls-tree", "HEAD", "--", ...SKIP_MAIN_PATHS], { encoding: "utf8" }); + return run.status === 0 && run.stdout.trim() !== "" ? run.stdout : null; +} + +// A state node as the key reads it, or null when any part is missing, cut +// or not at `head`: the head's check and status rollup (a status's time is +// left out, so the same state posted again on the next run is no change), +// the review decision and count, the newest comments' ids and edit times, +// and the review threads' resolved and outdated flags, none of which need +// move `updatedAt`, as babysit_merge_skip.pr_state keys them. +export function rollupOf(node, head) { + const commit = node?.commits?.nodes?.[0]?.commit; + if (commit?.oid !== head) return null; + const whole = (list, cap) => Array.isArray(list?.nodes) && list.totalCount === list.nodes.length && list.nodes.length <= cap; + const contexts = commit.statusCheckRollup == null ? { totalCount: 0, nodes: [] } : commit.statusCheckRollup.contexts; + if (!whole(contexts, 100) || !whole(node.reviewThreads, 100) || !Array.isArray(node.comments?.nodes) || !Number.isSafeInteger(node.comments.totalCount) || !Number.isSafeInteger(node.reviews?.totalCount)) { + return null; + } + return { + checks: contexts.nodes.map((c) => JSON.stringify([c?.__typename ?? null, c?.databaseId ?? c?.context ?? null, c?.name ?? null, c?.status ?? c?.state ?? null, c?.conclusion ?? null])).sort(), + reviewDecision: node.reviewDecision ?? null, + reviews: node.reviews.totalCount, + comments: [node.comments.totalCount, ...node.comments.nodes.map((c) => [c?.databaseId ?? null, c?.updatedAt ?? null])], + threads: node.reviewThreads.nodes.map((t) => JSON.stringify([Boolean(t?.isResolved), Boolean(t?.isOutdated)])).sort(), + }; +} + +export const skipKey = (pull, rollup, main) => + createHash("sha256") + .update( + JSON.stringify({ + v: SKIP_VERSION, + pr: pull.number, + head: pull.headRefOid, + updatedAt: pull.updatedAt ?? null, + labels: (pull.labels?.nodes ?? []).map((node) => String(node?.name).toLowerCase()).sort(), + rollup, + main, + }), + ) + .digest("hex"); + +// The kept records at `path` that still stand at `at`, by number; any doubt +// keeps none. +export function loadSkips(path, repo, at, { maxMinutes, maxSkips }) { + let data; + try { + data = JSON.parse(readFileSync(path, "utf8")); + } catch { + return {}; + } + if (data?.v !== SKIP_VERSION || data.repo !== repo || data.entries === null || typeof data.entries !== "object") return {}; + const kept = {}; + for (const [number, entry] of Object.entries(data.entries)) { + const judged = ZONED_TIME.test(String(entry?.at)) ? Date.parse(entry.at) : Number.NaN; + const until = ZONED_TIME.test(String(entry?.until)) ? Date.parse(entry.until) : Number.NaN; + if ( + /^[0-9]+$/.test(number) && + at - judged >= 0 && + at - judged < maxMinutes * 60_000 && + at < until && + Number.isSafeInteger(entry.skips) && + entry.skips >= 0 && + entry.skips < maxSkips && + /^[0-9a-f]{64}$/.test(String(entry.key)) && + SHA.test(String(entry.head)) && + typeof entry.reason === "string" && + SKIPPABLE.some((prefix) => entry.reason.startsWith(prefix)) + ) { + kept[number] = { at: entry.at, until: entry.until, key: entry.key, head: entry.head, reason: entry.reason, skips: entry.skips }; + } + } + return kept; +} + async function listPulls(github, owner, name) { const pulls = []; let after = null; @@ -1365,7 +1505,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, room, whole, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random, latch, now = Date.now } = {}) { +export async function main({ env = process.env, github, actions, verify, room, whole, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random, latch, now = Date.now, mainInputs } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -1416,7 +1556,10 @@ export async function main({ env = process.env, github, actions, verify, room, w whole ??= pythonWholeReview(env); const reads = READER_EVENTS.includes(env.EVENT_NAME); const serve = cacheMode(env) === "serve"; - const pulls = await listPulls(github, repo[1], repo[2]); + // A pull_request_target run handles only the pull request its event + // names, as the listing holds it; a scheduled or hand-started run, every one. + const listed = await listPulls(github, repo[1], repo[2]); + const pulls = env.EVENT_NAME === "pull_request_target" ? listed.filter((pull) => String(pull.number) === String(env.EVENT_PR ?? "")) : listed; const lanes = (pull) => pull.author?.__typename === "Bot" && pull.author.login === slug; // Only a reader run arms, so only it reads the room. let lane = null; @@ -1502,6 +1645,8 @@ export async function main({ env = process.env, github, actions, verify, room, w // pull request itself: // under merge.lane-prs native the gate never lists it, so a log line // would be all anyone saw. + // Writes note sent or tried, for skip-unchanged. + let sent = 0; const note = async (pull, detail, refusal) => { if (!reads || stopped !== null || refusal.clears === undefined) { return; @@ -1516,6 +1661,7 @@ export async function main({ env = process.env, github, actions, verify, room, w // budget the next run asks. let record = null; if (write && refusal.wholeReview === true && refusal.clears === "lane" && typeof refusal.comment !== "string" && now() <= deadline) { + sent += 1; try { const answer = whole.request(pull, priorRefusal(detail?.comments?.nodes, slug)?.body ?? ""); if (typeof answer?.line === "string" && WHOLE_REVIEW_RECORD.test(answer.line)) { @@ -1549,6 +1695,7 @@ export async function main({ env = process.env, github, actions, verify, room, w return; } const base = `/repos/${repo[0]}/issues`; + sent += 1; try { if (writes.body === null) { const variables = { owner: repo[1], name: repo[2], number: pull.number }; @@ -1588,6 +1735,43 @@ export async function main({ env = process.env, github, actions, verify, room, w } } }; + // Skip-unchanged (SKIPPABLE): a reader run that writes keeps its records + // and reads the last run's. + const skips = { on: reads && write && Boolean(env.EAM_SKIP_OUT), maxMinutes: skipLimit(env.SKIP_MAX_MINUTES, SKIP_MAX_MINUTES), maxSkips: skipLimit(env.SKIP_MAX_SKIPS, SKIP_MAX_SKIPS) }; + const rollups = new Map(); + const kept = {}; + let skipStore = {}; + let skipMain = null; + let skipped = 0; + let recorded = 0; + let keyable = 0; + if (skips.on && skips.maxMinutes > 0 && skips.maxSkips > 0) { + skipMain = (mainInputs ?? (() => mainInputsOf()))(); + try { + // The rung the gate scores the risk at (pythonVerify's `rung`). + skipMain = skipMain === null ? null : `${skipMain}\n${readFileSync(env.RESOLVED_CONFIG ?? "", "utf8")}\n${minutes}\n${lane?.rung ?? ""}\n${env.RUNG_CAP ?? ""}`; + } catch { + skipMain = null; + } + if (skipMain !== null) { + skipStore = loadSkips(env.EAM_SKIP_IN ?? "", repo[0], now(), skips); + const numbers = pulls.filter((pull) => lanes(pull) && pull.autoMergeRequest == null && pull.isDraft === false && holdReason(pull, humanGated) === null).map((pull) => pull.number); + keyable = numbers.length; + for (let start = 0; start < numbers.length; start += SKIP_CHUNK) { + try { + const read = (await github("POST", "/graphql", { query: skipStateQuery(numbers.slice(start, start + SKIP_CHUNK)), variables: { o: repo[1], r: repo[2] } }))?.data?.repository; + for (const number of numbers.slice(start, start + SKIP_CHUNK)) { + const node = read?.[`p${number}`]; + const listed = pulls.find((pull) => pull.number === number); + const rollup = node?.number === number && node.headRefOid === listed.headRefOid && node.updatedAt === listed.updatedAt ? rollupOf(node, listed.headRefOid) : null; + if (rollup !== null) rollups.set(number, rollup); + } + } catch (error) { + console.log(`enable-auto-merge: skip-unchanged state read failed, so those pull requests are read in full: ${describe(error)}`); + } + } + } + } for (let pull of rotated(pulls, Number(env.GITHUB_RUN_NUMBER))) { if (stopped === null && lanes(pull) && !(pull.labels?.totalCount <= (pull.labels?.nodes?.length ?? 0))) { try { @@ -1634,6 +1818,15 @@ export async function main({ env = process.env, github, actions, verify, room, w if (widen) { action = "read"; } + // This pull request's skip key, when its outcome may be kept or skipped. + const key = skipMain !== null && action === "read" && !widen && held === null && stopped === null && pull.autoMergeRequest == null && rollups.has(pull.number) ? skipKey(pull, rollups.get(pull.number), skipMain) : null; + const prior = key === null ? undefined : skipStore[pull.number]; + if (prior !== undefined && prior.key === key && prior.head === pull.headRefOid) { + skipped += 1; + kept[pull.number] = { ...prior, skips: prior.skips + 1 }; + console.log(`enable-auto-merge: #${pull.number} left, unchanged since ${prior.at} (skip ${prior.skips + 1} of ${skips.maxSkips}): ${escapeCommand(prior.reason)}`); + continue; + } if (action === "read") { try { const variables = { owner: repo[1], name: repo[2], number: pull.number }; @@ -1647,7 +1840,8 @@ export async function main({ env = process.env, github, actions, verify, room, w unmarked = widen && !isPartial(standing, pull.headRefOid) && !((approvedBy || pathHolds === false) && holdPathOnly(standing, pull.headRefOid, humanGated, pathHolds)); } if (detail !== null && !unmarked) { - const { paths: files, capped } = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); + const graphed = listedFiles(detail.files); + const { paths: files, capped } = graphed !== null ? { paths: graphed, capped: false } : await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); const humans = trustedHumans(env); const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, humans, pull.headRefOid, actions) : null; let lifters = headLifters(detail, humans, pull.headRefOid); @@ -1823,7 +2017,21 @@ export async function main({ env = process.env, github, actions, verify, room, w if (reads && write && withdraw === true && sinceAt(priorRefusal(detail?.comments?.nodes, slug)?.body, pull.headRefOid) !== undefined) { statuses.push({ sha: pull.headRefOid, state: "success", description: "its lanes refusal no longer stands" }); } + const sentBefore = sent; + const failedBefore = failed; await note(pull, detail, { clears, reason, comment, cached, partial, answered, wholeReview }); + // Kept only when this run wrote nothing for it, so the key read + // before it still stands. + if (key !== null && detail != null && sent === sentBefore && failed === failedBefore && stopped === null && clears === "lane" && typeof comment !== "string" && wholeReview !== true && SKIPPABLE.some((prefix) => String(reason).startsWith(prefix))) { + const since = sinceAt(priorRefusal(detail.comments?.nodes, slug)?.body, pull.headRefOid); + const at = now(); + const escalated = unarmedStatus(pull, detail.comments?.nodes, { clears, reason, comment }, { bot: slug, now: at, minutes }) !== null; + const until = Math.min(at + skips.maxMinutes * 60_000, escalated ? Number.POSITIVE_INFINITY : Date.parse(String(since)) + minutes * 60_000); + if (since !== undefined && until > at) { + recorded += 1; + kept[pull.number] = { at: new Date(at).toISOString(), until: new Date(until).toISOString(), key, head: pull.headRefOid, reason: String(reason), skips: 0 }; + } + } continue; } if (!write) { @@ -1928,6 +2136,15 @@ export async function main({ env = process.env, github, actions, verify, room, w for (const [level, text] of lines) { console.log(`::${level} title=enable-auto-merge::${text}`); } + if (skips.on) { + console.log(`enable-auto-merge: skip-unchanged skipped=${skipped} recorded=${recorded} keyed=${rollups.size} of ${keyable}`); + try { + writeFileSync(`${env.EAM_SKIP_OUT}.tmp`, JSON.stringify({ v: SKIP_VERSION, repo: repo[0], entries: kept })); + renameSync(`${env.EAM_SKIP_OUT}.tmp`, env.EAM_SKIP_OUT); + } catch (error) { + console.log(`::warning title=enable-auto-merge::the skip records were not kept, so the next run reads every pull request: ${describe(error)}`); + } + } if (statuses.length > 0 && env.GITHUB_OUTPUT) { try { appendFileSync(env.GITHUB_OUTPUT, `${UNARMED_OUTPUT}=${JSON.stringify(statuses)}\n`); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 1c85796703..8582b280ab 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -59,6 +59,7 @@ import { UNARMED_OUTPUT, verifyClears, verifyHold, + SKIP_STATE_PREFIX, } from "./enable-auto-merge.mjs"; import { GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { createRateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; @@ -248,13 +249,40 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note assert.deepEqual(body.variables, { owner: "melodic-software", name: "claude-code-plugins", after: null }); return { data: { repository: { pullRequests: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: pulls } } } }; } + // The skip key's state read (SKIP_STATE_QUERY): each pull request's + // listed head and update time, and its `rollup` (none by default). + if (String(body.query).startsWith(SKIP_STATE_PREFIX)) { + const repository = Object.fromEntries( + [...body.query.matchAll(/p([0-9]+): pullRequest/g)].map(([, n]) => { + const found = pulls.find((candidate) => candidate.number === Number(n)); + const rollup = found?.rollup ?? { contexts: { totalCount: 0, nodes: [] } }; + const none = { totalCount: 0, nodes: [] }; + return [ + `p${n}`, + { + number: Number(n), + headRefOid: found?.headRefOid, + updatedAt: found?.updatedAt, + reviewDecision: found?.reviewDecision ?? null, + reviews: { totalCount: found?.reviewCount ?? 0 }, + comments: found?.stateComments ?? none, + reviewThreads: found?.stateThreads ?? none, + commits: { nodes: [{ commit: { oid: found?.headRefOid, statusCheckRollup: rollup } }] }, + }, + ]; + }), + ); + return { data: { repository } }; + } if (body.query === UNLABELED_QUERY) { const nodes = pulls.find((candidate) => candidate.number === body.variables.number)?.unlabeled ?? []; return { data: { repository: { pullRequest: { timelineItems: { totalCount: nodes.length, nodes } } } } }; } if (body.query === DETAIL_QUERY) { const found = pulls.find((candidate) => candidate.number === body.variables.number); - return { data: { repository: { pullRequest: found?.cutDiscussion ?? detail(found?.discussion) } } }; + // `graphFiles`: the changed files the query lists, absent by default. + const read = found?.cutDiscussion ?? detail(found?.discussion); + return { data: { repository: { pullRequest: found?.graphFiles === undefined ? read : { ...read, files: found.graphFiles } } } }; } // The rest of a cut comment list, two to a page. if (body.query === DISCUSSION_PAGE_QUERIES.comments) { @@ -299,7 +327,7 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note }; const writes = () => calls - .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY, UNLABELED_QUERY, DISCUSSION_PAGE_QUERIES.comments, THREAD_PAGE_QUERY].includes(body.query)) + .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY, UNLABELED_QUERY, DISCUSSION_PAGE_QUERIES.comments, THREAD_PAGE_QUERY].includes(body.query) && !String(body.query).startsWith(SKIP_STATE_PREFIX)) .map(({ body }) => ({ mutation: body.query === ARM_MUTATION ? "arm" : body.query === DISARM_MUTATION ? "disarm" : "?", ...body.variables })); return { github, writes, verify, verified, calls, notes, pins }; } @@ -340,7 +368,7 @@ async function run(pulls, env = {}, options = {}, lane = ROOM) { console.log = (line) => lines.push(line); try { const actions = options.jobToken ? options.jobToken(github) : undefined; - const code = await main({ env: { ...ENV, GITHUB_OUTPUT: output, ...env }, github, actions, verify, room: room(lane), whole, sleep: async (ms) => slept.push(ms), random: options.random, now: options.now ?? (() => Date.parse(NOW)) }); + const code = await main({ env: { ...ENV, GITHUB_OUTPUT: output, ...env }, github, actions, verify, room: room(lane), whole, sleep: async (ms) => slept.push(ms), random: options.random, now: options.now ?? (() => Date.parse(NOW)), mainInputs: options.mainInputs }); // The `pr-merge/unarmed` statuses the run hands the status job. const line = readFileSync(output, "utf8").split("\n").find((text) => text.startsWith(`${UNARMED_OUTPUT}=`)); const statuses = line === undefined ? [] : JSON.parse(line.slice(UNARMED_OUTPUT.length + 1)); @@ -405,6 +433,25 @@ test("a refusal a person must clear, a cost path or a configured denied path, is ]); }); +// GitHub Support's audit item 2: the discussion read lists the changed files +// too, so a head whose files fit one page and include no rename (whose old +// path only the REST listing names) reads no files page. +const filesPages = (calls) => calls.filter(({ path }) => /\/pulls\/[0-9]+\/files\?/.test(path)).length; +test("the discussion read's file list stands in for the files page, unless a rename or a cut list needs it", async () => { + const listed = (...nodes) => ({ totalCount: nodes.length, nodes }); + const folded = await run([pull(1, { graphFiles: listed({ path: "infra/main.tf", changeType: "MODIFIED" }), files: [] })]); + assert.equal(filesPages(folded.calls), 0); + assert.deepEqual(folded.notes.slice(0, 1), [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: personNote(COST_REASON) } }]); + // A rename away from a cost path: only the REST page names the old path. + const renamed = await run([pull(1, { graphFiles: listed({ path: "docs/main.md", changeType: "RENAMED" }), files: [{ filename: "docs/main.md", previous_filename: "infra/main.tf" }] })]); + assert.equal(filesPages(renamed.calls), 1); + assert.deepEqual(renamed.notes.slice(0, 1), [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: personNote(COST_REASON) } }]); + // A list longer than the query read. + const cut = await run([pull(1, { graphFiles: { totalCount: 101, nodes: [{ path: "docs/a.md", changeType: "MODIFIED" }] }, files: COST_FILES })]); + assert.equal(filesPages(cut.calls), 1); + assert.deepEqual(cut.notes.slice(0, 1), [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: personNote(COST_REASON) } }]); +}); + // Owner decision Q26 option B: the gate holds an enforcement floor path as // `review-path:` until the security review binds clean to the head; the lanes // run that review, so the refusal gets the comment and no label. @@ -446,7 +493,7 @@ test("a whole review is requested only for a lane's risk hold the gate says one "not wanted": [{ ...PATH_HELD, wholeReviewWanted: false }, {}], "a person's hold": [{ ...PATH_HELD, reviewsPerson: true }, {}], "report-only": [PATH_HELD, { REPORT_ONLY: "true" }], - "an event run": [PATH_HELD, { EVENT_NAME: "pull_request_target" }], + "an event run": [PATH_HELD, { EVENT_NAME: "pull_request_target", EVENT_PR: "1" }], }; for (const [name, [verdict, env]] of Object.entries(cases)) { const { asked } = await run([pull(1, { verdict })], env, { request: { line: RECORD } }); @@ -506,7 +553,7 @@ test("a lanes App draft a toggle may have left goes to recover; another author's const { asked, lines } = await run([draft, other]); assert.deepEqual(asked, [["recover", [{ number: 1, id: "PR_1", head: HEAD, updatedAt: "2026-10-23T09:00:00Z", labels: ["ci"] }]]]); assert.ok(lines.includes("enable-auto-merge: #1 marked ready again: a whole-review toggle left it a draft")); - assert.deepEqual((await run([draft], { EVENT_NAME: "pull_request_target" })).asked, []); + assert.deepEqual((await run([draft], { EVENT_NAME: "pull_request_target", EVENT_PR: "1" })).asked, []); assert.deepEqual((await run([draft], { REPORT_ONLY: "true" })).asked, []); }); @@ -1129,7 +1176,7 @@ test("a pr-verify-failed pull request is not armed, and an armed one is disarmed for (const EVENT_NAME of ["schedule", "pull_request_target"]) { const { code, writes } = await run( [pull(1, { labels: labels("pr-verify-failed") }), pull(2, { autoMergeRequest: armed, labels: labels("PR-Verify-Failed") })], - { EVENT_NAME }, + { EVENT_NAME, EVENT_PR: "2" }, ); assert.equal(code, 0); assert.deepEqual(writes, [{ mutation: "disarm", id: "PR_2" }], EVENT_NAME); @@ -1972,7 +2019,7 @@ test("a person's own hold text and an event run write no note, and an event run const { notes, writes } = await run([pull(1, { body: "Closes #5\n\nHold: waiting on a review", userContentEdits: person })]); assert.deepEqual(writes, []); assert.deepEqual(notes, []); - const event = await run([laneHeld(2, { autoMergeRequest: {} }), laneHeld(3)], { EVENT_NAME: "pull_request_target" }); + const event = await run([laneHeld(2, { autoMergeRequest: {} }), laneHeld(3)], { EVENT_NAME: "pull_request_target", EVENT_PR: "2" }); assert.deepEqual(event.writes, [{ mutation: "disarm", id: "PR_2" }]); assert.deepEqual(event.notes, []); assert.ok(!event.calls.some(({ body }) => body?.query === DETAIL_QUERY)); @@ -1982,6 +2029,22 @@ test("a person's own hold text and an event run write no note, and an event run assert.deepEqual(reader.notes, []); }); +// GitHub Support's audit item 5: a pull_request_target run handles only the +// pull request its event names (EVENT_PR), as the listing holds it; the +// scheduled run keeps the whole sweep. +test("an event run handles only its own pull request", async () => { + const pulls = [laneHeld(2, { autoMergeRequest: {} }), laneHeld(3, { autoMergeRequest: {} })]; + const event = await run(pulls, { EVENT_NAME: "pull_request_target", EVENT_PR: "3" }); + assert.deepEqual(event.writes, [{ mutation: "disarm", id: "PR_3" }]); + // An event naming no listed pull request handles none. + assert.deepEqual((await run(pulls, { EVENT_NAME: "pull_request_target", EVENT_PR: "9" })).writes, []); + // The schedule handles both, whatever EVENT_PR says. + assert.deepEqual((await run(pulls, { EVENT_PR: "3" })).writes, [ + { mutation: "disarm", id: "PR_2" }, + { mutation: "disarm", id: "PR_3" }, + ]); +}); + // The start moves with the run number, so the pull requests past the // verify budget differ from run to run. test("the run starts at an offset that moves with the run number", async () => { @@ -2180,7 +2243,7 @@ test("only the scheduled run and a hand-started one read the verdict and arm", ( }); test("an event run arms nothing and reads neither discussion nor verdict for an unarmed pull request", async () => { - const { code, writes, verified, lines } = await run([pull(1)], { EVENT_NAME: "pull_request_target" }); + const { code, writes, verified, lines } = await run([pull(1)], { EVENT_NAME: "pull_request_target", EVENT_PR: "1" }); assert.equal(code, 0); assert.deepEqual(writes, []); assert.deepEqual(verified, []); @@ -2189,19 +2252,16 @@ test("an event run arms nothing and reads neither discussion nor verdict for an 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, []); + const pulls = [ + 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 }), + ]; + for (const [number, expected] of [[1, [{ mutation: "disarm", id: "PR_1" }]], [2, [{ mutation: "disarm", id: "PR_2" }]], [3, []]]) { + const { writes, verified } = await run(pulls, { EVENT_NAME: "pull_request_target", EVENT_PR: String(number) }); + assert.deepEqual(writes, expected, String(number)); + assert.deepEqual(verified, []); + } }); test("a reader that fails, prints no JSON or reports another head arms nothing and disarms", async () => { @@ -2408,7 +2468,7 @@ with mock.patch.object(policy, "gh_capture", gh), mock.patch.object(postmerge, " // Refs pull request on its link; the scheduled run judges it. test("an event run leaves an armed Refs pull request armed", async () => { const armed = { enabledAt: "2026-10-23T10:00:00Z" }; - const { writes, verified } = await run([pull(1, { autoMergeRequest: armed, body: "Refs: #12\n\n## Summary" })], { EVENT_NAME: "pull_request_target" }); + const { writes, verified } = await run([pull(1, { autoMergeRequest: armed, body: "Refs: #12\n\n## Summary" })], { EVENT_NAME: "pull_request_target", EVENT_PR: "1" }); assert.deepEqual(writes, []); assert.deepEqual(verified, []); }); @@ -2548,7 +2608,7 @@ test("a held room in report-only names the armed pull request a disarm would hav // An event run reads no room, so it never disarms on one. test("an event run leaves an armed lane pull request armed whatever the room", async () => { const armed = { enabledAt: "2026-10-23T10:00:00Z" }; - const { writes } = await run([pull(2, { autoMergeRequest: armed })], { EVENT_NAME: "pull_request_target" }, {}, null); + const { writes } = await run([pull(2, { autoMergeRequest: armed })], { EVENT_NAME: "pull_request_target", EVENT_PR: "2" }, {}, null); assert.deepEqual(writes, []); }); @@ -2559,7 +2619,7 @@ test("an event run reads no room", async () => { console.log = () => {}; try { await main({ - env: { ...ENV, EVENT_NAME: "pull_request_target" }, + env: { ...ENV, EVENT_NAME: "pull_request_target", EVENT_PR: "1" }, github, room: async () => { asked += 1; @@ -3477,3 +3537,142 @@ test("with path holds off, a standing risk refusal naming only path holds comes assert.ok(!kept.notes.some(({ method }) => method === "DELETE"), `${why} lifts nothing`); } }); + +// GitHub Support's audit item 2, as babysit_merge_skip.py keys the merge +// gate's skip: an unarmed lanes App pull request whose last reader run left it +// on a lane's risk hold or a pr-unblock stop, with nothing written, is left on +// that refusal with no read while its head, update time, labels, check and +// status rollup and main's code all match. A check or status does not change +// `updatedAt`, so the rollup is in the key on its own. +const LANE_RISK = "lines-over: 900 changed lines, over the 800 cap"; +const LANE_RISK_REASON = `pr-merge's gate holds it on its risk score (${LANE_RISK})`; +const LANE_RISK_HELD = { refusal: null, needsHuman: false, head: HEAD, ineligible: null, reviews: LANE_RISK, reviewsPerson: false, queue: UNEJECTED }; +const check = (conclusion) => ({ contexts: { totalCount: 2, nodes: [ + { __typename: "CheckRun", databaseId: 11, name: "ci-status", status: "COMPLETED", conclusion }, + { __typename: "StatusContext", context: "pr-merge/unarmed", state: "PENDING", createdAt: "2026-10-23T09:00:00Z" }, +] } }); +const riskHeld = (overrides = {}) => + pull(1, { updatedAt: "2026-10-23T09:30:00Z", verdict: LANE_RISK_HELD, discussion: { comments: [lanesSaid(laneNote(LANE_RISK_REASON))] }, rollup: check("SUCCESS"), ...overrides }); +const at = (minutes) => () => Date.parse(NOW) + minutes * 60_000; +// One reader run that keeps its skip records at `out`, from `from` when given. +async function skipRun(pulls, { from, minutes = 0, main: inputs = "main-1", env = {}, lane = ROOM } = {}) { + const dir = tempDir("enable-auto-merge-skip-"); + const out = join(dir, "eam-skip.json"); + const result = await run(pulls, { EAM_SKIP_IN: from ?? join(dir, "none.json"), EAM_SKIP_OUT: out, ...env }, { now: at(minutes), mainInputs: () => inputs }, lane); + let kept = null; + try { + kept = readFileSync(out, "utf8"); + } catch {} + const path = join(dir, "kept.json"); + if (kept !== null) writeFileSync(path, kept); + return { ...result, kept: kept === null ? null : JSON.parse(kept), from: path, details: result.calls.filter(({ body }) => body?.query === DETAIL_QUERY).length }; +} + +test("an unchanged lane risk hold is left on its refusal with no read, and counted", async () => { + const first = await skipRun([riskHeld()]); + assert.deepEqual(first.verified, [1]); + assert.deepEqual(first.notes, []); + assert.equal(first.kept.entries["1"]?.reason, LANE_RISK_REASON, first.lines.join("\n")); + const second = await skipRun([riskHeld()], { from: first.from, minutes: 15 }); + assert.deepEqual(second.verified, []); + assert.equal(second.details, 0); + assert.equal(filesPages(second.calls), 0); + assert.deepEqual(second.writes, []); + assert.deepEqual(second.notes, []); + assert.ok(second.lines.includes("enable-auto-merge: skip-unchanged skipped=1 recorded=0 keyed=1 of 1"), second.lines.join("\n")); + assert.equal(second.kept.entries["1"].skips, 1); +}); + +test("a changed check rollup alone, with the same updatedAt, is read again", async () => { + const first = await skipRun([riskHeld()]); + const second = await skipRun([riskHeld({ rollup: check("FAILURE") })], { from: first.from, minutes: 15 }); + assert.deepEqual(second.verified, [1]); + assert.equal(second.details, 1); + assert.ok(second.lines.some((line) => line.startsWith("enable-auto-merge: skip-unchanged skipped=0 "))); +}); + +// As babysit_merge_skip.py keys them: a thread resolved, a review or a +// comment edited need not move `updatedAt`. +test("a resolved review thread, a review or an edited comment alone, with the same updatedAt, is read again", async () => { + const threads = (isResolved) => ({ stateThreads: { totalCount: 1, nodes: [{ isResolved, isOutdated: false }] } }); + const first = await skipRun([riskHeld(threads(false))]); + assert.equal((await skipRun([riskHeld(threads(false))], { from: first.from, minutes: 15 })).details, 0); + for (const [name, changed] of Object.entries({ + thread: threads(true), + review: { ...threads(false), reviewCount: 1 }, + decision: { ...threads(false), reviewDecision: "APPROVED" }, + comment: { ...threads(false), stateComments: { totalCount: 1, nodes: [{ databaseId: 5, updatedAt: "2026-10-23T10:05:00Z" }] } }, + })) { + assert.equal((await skipRun([riskHeld(changed)], { from: first.from, minutes: 15 })).details, 1, name); + } +}); + +test("a changed head, update time, label set or main is read again, and an armed one always is", async () => { + const first = await skipRun([riskHeld()]); + const cases = { + head: [riskHeld({ headRefOid: "c".repeat(40), verdict: { ...LANE_RISK_HELD, head: "c".repeat(40) } }), {}], + updatedAt: [riskHeld({ updatedAt: "2026-10-23T10:05:00Z" }), {}], + labels: [riskHeld({ labels: labels("ci") }), {}], + main: [riskHeld(), { main: "main-2" }], + // The rung the gate scores the risk at (pythonVerify's `rung`). + rung: [riskHeld(), { lane: { ...ROOM, rung: "C3" } }], + armed: [riskHeld({ autoMergeRequest: { enabledAt: "2026-10-23T10:01:00Z" } }), {}], + }; + for (const [name, [changed, options]] of Object.entries(cases)) { + const again = await skipRun([changed], { from: first.from, minutes: 15, ...options }); + assert.equal(again.details, 1, name); + } +}); + +test("a skip record is read again past max-minutes, after max-skips, and never when either is 0", async () => { + const first = await skipRun([riskHeld()]); + // The lane comment escalates at 60 minutes, so 50 stays inside both bounds. + assert.equal((await skipRun([riskHeld()], { from: first.from, minutes: 50, env: { SKIP_MAX_MINUTES: "45" } })).details, 1); + let from = first.from; + for (const minutes of [15, 30]) { + from = (await skipRun([riskHeld()], { from, minutes, env: { SKIP_MAX_SKIPS: "2" } })).from; + } + assert.equal((await skipRun([riskHeld()], { from, minutes: 45, env: { SKIP_MAX_SKIPS: "2" } })).details, 1); + assert.equal((await skipRun([riskHeld()], { from: first.from, minutes: 15, env: { SKIP_MAX_MINUTES: "0" } })).details, 1); +}); + +test("a lane hold due to escalate to a failing status is read again at its escalation", async () => { + // max-minutes past the escalation, so only the escalation bounds the skip. + const env = { SKIP_MAX_MINUTES: "120" }; + const first = await skipRun([riskHeld()], { env }); + assert.equal((await skipRun([riskHeld()], { from: first.from, minutes: 59, env })).details, 0); + const late = await skipRun([riskHeld()], { from: first.from, minutes: 61, env }); + assert.equal(late.details, 1); + assert.deepEqual(late.statuses, [{ sha: HEAD, state: "failure", description: LANE_RISK_REASON }]); +}); + +test("a refusal that wrote, a verdict refusal and an event run keep no skip record", async () => { + // The first lane comment is written, or a changed reason edited in, so the + // next run reads again. + for (const comments of [[], [lanesSaid(laneNote("pr-merge's gate holds it on its risk score (lines-over: 850 changed lines)"))]]) { + const wrote = await skipRun([riskHeld({ discussion: { comments } })]); + assert.equal(wrote.notes.length, 1); + assert.deepEqual(wrote.kept.entries, {}); + } + // A verdict a later artifact may answer differently, with no change to the key. + const missing = await skipRun([riskHeld({ verdict: MISSING, discussion: { comments: [lanesSaid(laneNote(MISSING_REASON))] } })]); + assert.deepEqual(missing.kept.entries, {}); + const event = await skipRun([riskHeld()], { env: { EVENT_NAME: "pull_request_target", EVENT_PR: "1" } }); + assert.equal(event.kept, null); +}); + +test("a skip record of another repository, from the future, or malformed skips nothing", async () => { + const first = await skipRun([riskHeld()]); + const kept = JSON.parse(readFileSync(first.from, "utf8")); + const dir = tempDir("enable-auto-merge-skip-bad-"); + const forged = { + repo: { ...kept, repo: "someone/else" }, + future: { ...kept, entries: { 1: { ...kept.entries["1"], at: "2026-10-23T11:00:00.000Z" } } }, + malformed: { ...kept, entries: { 1: { ...kept.entries["1"], skips: "0" } } }, + }; + for (const [name, data] of Object.entries(forged)) { + const path = join(dir, `${name}.json`); + writeFileSync(path, JSON.stringify(data)); + assert.equal((await skipRun([riskHeld()], { from: path, minutes: 15 })).details, 1, name); + } +}); diff --git a/.github/scripts/pr-merge/merge-job.test.mjs b/.github/scripts/pr-merge/merge-job.test.mjs index 2fc33d2e3b..178d82eb14 100644 --- a/.github/scripts/pr-merge/merge-job.test.mjs +++ b/.github/scripts/pr-merge/merge-job.test.mjs @@ -90,22 +90,26 @@ test("the early re-dispatch keeps the trusted mechanics and skips untrusted even }); // Runs the listing step's script against a fake `gh` whose `gh pr list` -// fails its first `failures` calls, with `.base` pointing at this checkout, +// fails its first `failures` calls and then answers `prs` (by default one pull +// request of the lanes App's), with `.base` pointing at this checkout, // or, given `edit`, at a copy whose pr-pipeline.yaml is `edit(text)` and whose // trusted-actors.json is `actors`, when given, else this checkout's. // `env` overrides the step's environment: merge.lane-prs is `gate` unless it // says otherwise, the mode that lists the lanes App's own pull requests. -const runListing = (failures, edit, actors, env = {}, failure = "HTTP 502: Bad Gateway") => { +const listed = (number, login, isDraft = false) => ({ id: `PR_${number}`, number, headRefOid: "abc", isDraft, updatedAt: "2026-10-18T00:00:00Z", labels: [], body: "", author: { login } }); +const LANES_PR = [listed(7, "app/melodic-automation-lanes")]; +const runListing = (failures, edit, actors, env = {}, failure = "HTTP 502: Bad Gateway", prs = LANES_PR) => { const dir = tempDir("pr-merge-listing-"); try { const bin = join(dir, "bin"); execFileSync("mkdir", [bin]); + writeFileSync(join(dir, "prs.json"), JSON.stringify(prs)); writeFileSync( join(bin, "gh"), `#!/usr/bin/env bash\necho "$*" >>"${dir}/args"\n` + `n=$(($(cat "${dir}/calls" 2>/dev/null || echo 0) + 1))\necho "$n" >"${dir}/calls"\n` + `if [ "$n" -le ${failures} ]; then echo "${failure}" >&2; exit 1; fi\n` + - `echo '[{"number":7,"headRefOid":"abc","isDraft":false,"updatedAt":"2026-10-18T00:00:00Z","labels":[],"body":""}]'\n`, + `cat "${dir}/prs.json"\n`, ); writeFileSync(join(bin, "sleep"), "#!/usr/bin/env bash\n"); chmodSync(join(bin, "gh"), 0o755); @@ -138,10 +142,12 @@ const runListing = (failures, edit, actors, env = {}, failure = "HTTP 502: Bad G REPO: "o/r", DEPENDABOT: "off", LANE_PRS: "gate", + EVENT_PR: "", ...env, }, }); const read = (name) => (existsSync(join(dir, name)) ? readFileSync(join(dir, name), "utf8") : ""); + const numbers = (name) => (read(name).trim() ? JSON.parse(read(name)).map((item) => item.number) : []); return { status: result.status, stdout: result.stdout, @@ -149,6 +155,8 @@ const runListing = (failures, edit, actors, env = {}, failure = "HTTP 502: Bad G calls: Number(read("calls") || 0), args: read("args"), candidates: read("candidates.json"), + listed: numbers("candidates.json"), + draftsListed: numbers("drafts.json"), drafts: read("drafts.json"), prefiltered: existsSync(join(dir, "prefilter.json")), output: existsSync(output) ? readFileSync(output, "utf8") : "", @@ -158,18 +166,28 @@ const runListing = (failures, edit, actors, env = {}, failure = "HTTP 502: Bad G } }; -const authorsOf = (args) => [...args.matchAll(/^pr list .*--author (\S+) /gm)].map((m) => m[1]); - test("the listing retries a failed gh pr list", () => { const run = runListing(2); assert.equal(run.status, 0, run.stderr); - // Three tries for the lane's list, then one for each other author's. - assert.equal(run.calls, new Set(authorsOf(run.args)).size + 2); + // Three tries of the one list. + assert.equal(run.calls, 3); assert.match(run.output, /^count=1$/m); }); +// GitHub Support's audit item 7: one listing of every open pull request into +// main, filtered by author, in place of one `gh pr list --author` (a search +// call) per author. +const AUTHORED = [ + listed(1, "app/melodic-automation-lanes"), + listed(2, "a-person"), + listed(3, "app/some-release"), + listed(4, "app/dependabot"), + listed(5, "a-stranger"), + listed(6, "app/melodic-automation-lanes", true), +]; + // Owner decision 2026-10-22: every trusted actor's PR is a candidate. -test("the listing adds each trusted actor, a person by login and an App as app/, once", () => { +test("the listing keeps each trusted actor's pull requests, a person by login and an App as app/, from one list", () => { const actors = [ { id: 1, login: "a-person", kind: "human" }, { id: 2, login: "some-release[bot]", kind: "bot" }, @@ -177,37 +195,39 @@ test("the listing adds each trusted actor, a person by login and an App as app/< { id: 4, login: "dependabot[bot]", kind: "bot" }, { id: 5, login: "bad login; rm", kind: "human" }, ]; - const run = runListing(0, (config) => config, actors); + const run = runListing(0, (config) => config, actors, {}, undefined, AUTHORED); assert.equal(run.status, 0, run.stderr); - assert.deepEqual(authorsOf(run.args), ["app/melodic-automation-lanes", "a-person", "app/some-release"]); + assert.equal(run.calls, 1); + assert.doesNotMatch(run.args, /--author/); + assert.deepEqual(run.listed, [1, 2, 3]); + assert.deepEqual(run.draftsListed, [6]); assert.doesNotMatch(run.args, /^api /m); }); test("without an adoption section no trusted actor is listed", () => { - const run = runListing(0, (config) => config.replace(/^adoption:\n( {2}.*\n)*/m, "")); + const run = runListing(0, (config) => config.replace(/^adoption:\n( {2}.*\n)*/m, ""), undefined, {}, undefined, AUTHORED); assert.equal(run.status, 0, run.stderr); - assert.deepEqual(authorsOf(run.args), ["app/melodic-automation-lanes"]); + assert.deepEqual(run.listed, [1]); // Nor with a section that sets no idle-hours, which the gate adopts nothing under. - const noIdle = runListing(0, (config) => config.replace(/^ {2}idle-hours: 2\n/m, "")); + const noIdle = runListing(0, (config) => config.replace(/^ {2}idle-hours: 2\n/m, ""), undefined, {}, undefined, AUTHORED); assert.equal(noIdle.status, 0, noIdle.stderr); - assert.deepEqual(authorsOf(noIdle.args), ["app/melodic-automation-lanes"]); + assert.deepEqual(noIdle.listed, [1]); }); // merge.lane-prs: native leaves the lanes App's pull requests to GitHub's -// auto-merge: the listing never asks for them, so the gate never reads them. +// auto-merge: the listing drops them, so the gate never reads them. test("under lane-prs native the listing skips the lanes App and keeps every other author", () => { const actors = [ { id: 1, login: "a-person", kind: "human" }, { id: 3, login: "melodic-automation-lanes[bot]", kind: "bot" }, ]; - const native = runListing(0, (config) => config, actors, { LANE_PRS: "native", DEPENDABOT: "enforce" }); + const native = runListing(0, (config) => config, actors, { LANE_PRS: "native", DEPENDABOT: "enforce" }, undefined, AUTHORED); assert.equal(native.status, 0, native.stderr); - assert.deepEqual(authorsOf(native.args), ["a-person", "app/dependabot"]); - assert.match(native.output, /^count=1$/m); - // gate lists the lanes App first, as before the setting existed. - const gate = runListing(0, (config) => config, actors, { LANE_PRS: "gate", DEPENDABOT: "enforce" }); + assert.deepEqual(native.listed, [2, 4]); + assert.match(native.output, /^count=2$/m); + const gate = runListing(0, (config) => config, actors, { LANE_PRS: "gate", DEPENDABOT: "enforce" }, undefined, AUTHORED); assert.equal(gate.status, 0, gate.stderr); - assert.deepEqual(authorsOf(gate.args), ["app/melodic-automation-lanes", "a-person", "app/dependabot"]); + assert.deepEqual(gate.listed, [1, 2, 4]); }); test("under lane-prs native with no other author the listing calls nothing and lists no candidate", () => { @@ -219,6 +239,30 @@ test("under lane-prs native with no other author the listing calls nothing and l assert.match(run.output, /^count=0$/m); }); +// GitHub Support's audit item 5: a pull_request_target run gates only the +// pull request its event names, from the listing (never the payload); the +// scheduled run keeps the whole sweep. +test("an event run lists only its own pull request", () => { + const actors = [{ id: 1, login: "a-person", kind: "human" }]; + const run = runListing(0, (config) => config, actors, { EVENT_PR: "2" }, undefined, AUTHORED); + assert.equal(run.status, 0, run.stderr); + assert.deepEqual(run.listed, [2]); + assert.deepEqual(run.draftsListed, []); + assert.match(run.output, /^count=1$/m); + // A pull request the listing does not hold lists nothing. + const absent = runListing(0, (config) => config, actors, { EVENT_PR: "5" }, undefined, AUTHORED); + assert.deepEqual(absent.listed, []); + assert.match(step("List the candidate PRs"), /\n {10}EVENT_PR: \$\{\{ github\.event\.pull_request\.number \}\}\n/); +}); + +test("a listing that may be cut short stops red", () => { + const many = Array.from({ length: 1000 }, (_, i) => listed(i + 1, "app/melodic-automation-lanes")); + const run = runListing(0, undefined, undefined, {}, undefined, many); + assert.equal(run.status, 1); + assert.match(run.stdout, /^::error::gh pr list returned 1000 pull requests/m); + assert.equal(run.prefiltered, false); +}); + test("the mode step outputs merge.lane-prs as resolve-config resolved it, and the listing reads it", () => { for (const mode of ["native", "gate"]) { const dir = tempDir("pr-merge-mode-"); @@ -245,7 +289,7 @@ test("a rate-limited listing stops red after one call, a bare HTTP 429 included" const run = runListing(3, undefined, undefined, {}, failure); assert.equal(run.status, 1, failure); assert.equal(run.calls, 1, failure); - assert.match(run.stdout, /^::error title=GitHub rate limit::gh pr list --author app\/[a-z0-9-]+ was rate limited \(every pool until /m, failure); + assert.match(run.stdout, /^::error title=GitHub rate limit::gh pr list was rate limited \(every pool until /m, failure); assert.equal(run.prefiltered, false); } }); @@ -254,7 +298,7 @@ test("the listing fails with one clear error after three failed tries", () => { const run = runListing(3); assert.equal(run.status, 1); assert.equal(run.calls, 3); - assert.match(run.stdout, /^::error::gh pr list --author app\/[a-z0-9-]+ failed 3 times/m); + assert.match(run.stdout, /^::error::gh pr list failed 3 times/m); assert.equal(run.prefiltered, false); assert.doesNotMatch(run.stderr, /jq: error|Traceback/); }); diff --git a/.github/scripts/pr-merge/rotation.mjs b/.github/scripts/pr-merge/rotation.mjs index ea5cb0051d..81cefc50a0 100755 --- a/.github/scripts/pr-merge/rotation.mjs +++ b/.github/scripts/pr-merge/rotation.mjs @@ -60,14 +60,20 @@ export function trustedRun(run) { } // `gh run download` of the artifact `name`'s file `file` from a run. +// A run's artifact `name`, downloaded once per run id, as the text of `file` +// (another file of the same artifact when the caller names one). export function ghDownload({ repo, token, dir, name = CURSOR_ARTIFACT, file = "cursor.json" }) { - return (runId) => { - const target = mkdtempSync(join(dir, `${name}-`)); - execFileSync("gh", ["run", "download", String(runId), "-R", repo, "-n", name, "-D", target], { - env: { ...process.env, GH_TOKEN: token }, - stdio: ["ignore", "ignore", "inherit"], - }); - return readFileSync(join(target, file), "utf8"); + const targets = new Map(); + return (runId, other = file) => { + if (!targets.has(String(runId))) { + const target = mkdtempSync(join(dir, `${name}-`)); + execFileSync("gh", ["run", "download", String(runId), "-R", repo, "-n", name, "-D", target], { + env: { ...process.env, GH_TOKEN: token }, + stdio: ["ignore", "ignore", "inherit"], + }); + targets.set(String(runId), target); + } + return readFileSync(join(targets.get(String(runId)), other), "utf8"); }; } diff --git a/.github/scripts/pr-merge/superseded.mjs b/.github/scripts/pr-merge/superseded.mjs index d82fc2a024..2c6eb83e3d 100755 --- a/.github/scripts/pr-merge/superseded.mjs +++ b/.github/scripts/pr-merge/superseded.mjs @@ -24,12 +24,13 @@ // bot), or any other run whose actor and triggering actor are each a listed // human. A scheduled or // hand-started run counts only on main, since another branch runs its own -// copy; pull_request_target runs always run main's. And its +// copy. A pull_request_target run reads only its own pull request, so it +// supersedes no run. And its // event must rank at least as high as this run's (RANKS). For `merge`, only a // scheduled run re-fires refused lane runs, and a pull_request_target run // starts no full CI run: pull_request_target, then workflow_dispatch, then -// schedule. For `enable-auto-merge`, every run lists every lane pull request -// and disarms one on a hold, and only a scheduled or hand-started one arms: +// schedule. For `enable-auto-merge`, a scheduled or hand-started run reads +// every lane pull request and disarms one on a hold, and only it arms: // pull_request_target, then workflow_dispatch and schedule. // // Any failure answers `superseded=false`: the job does its work as it would @@ -75,8 +76,10 @@ export function eligible(runs, { runId, event, trusted, ranks }) { String(run.id) !== runId && Object.hasOwn(ranks, run.event) && ranks[run.event] >= ranks[event] && + // An event run reads only its own pull request (pr-merge.yml EVENT_PR). + run.event !== "pull_request_target" && // A dispatch on another branch runs that branch's copy of the workflow. - (run.event === "pull_request_target" || run.head_branch === "main") && + run.head_branch === "main" && (run.event === "schedule" ? run.run_attempt === 1 || trusted.humans.has(run.triggering_actor?.id) : startedByHuman(trusted, run)), diff --git a/.github/scripts/pr-merge/superseded.test.mjs b/.github/scripts/pr-merge/superseded.test.mjs index 68231e61f5..6e680fd75b 100644 --- a/.github/scripts/pr-merge/superseded.test.mjs +++ b/.github/scripts/pr-merge/superseded.test.mjs @@ -115,9 +115,11 @@ test("a pending run a listed human started by hand supersedes a pull_request_tar assert.equal(run.superseded, true); }); -test("a pending pull_request_target run by a listed human supersedes another pull_request_target run", async () => { +// An event run gates only its own pull request (pr-merge.yml EVENT_PR), so +// another event's run does not do this one's work. +test("a pending pull_request_target run supersedes no other pull_request_target run", async () => { const run = await check({ event: "pull_request_target", runs: [pending(600, "pull_request_target")] }); - assert.equal(run.superseded, true); + assert.equal(run.superseded, false); }); test("a pending run a bot started stops before its sweep, so it supersedes nothing", async () => { @@ -244,9 +246,9 @@ test("a failed jobs read does the work, with a warning", async () => { assert.match(run.stdout, /^::warning title=superseded::.*status 502/m); }); -test("enable-auto-merge: a pending edit run a listed human started supersedes another", async () => { +test("enable-auto-merge: a pending edit run, which reads only its own pull request, supersedes no other", async () => { const run = await check({ job: "enable-auto-merge", event: "pull_request_target", runs: [pending(600, "pull_request_target")] }); - assert.equal(run.superseded, true); + assert.equal(run.superseded, false); }); test("enable-auto-merge: a scheduled and a hand-started run, which both arm, supersede each other and every event run", async () => { diff --git a/.github/scripts/pr-merge/verify-answers.mjs b/.github/scripts/pr-merge/verify-answers.mjs index 05dd479847..324d4efd21 100755 --- a/.github/scripts/pr-merge/verify-answers.mjs +++ b/.github/scripts/pr-merge/verify-answers.mjs @@ -17,6 +17,10 @@ // // Any failure writes nothing and warns: the reader then reads in full. // +// The same artifact holds enable-auto-merge's skip records (SKIP_FILE), read +// from the one download into EAM_SKIP_PATH; the arm step checks them itself, +// and a skip only repeats a refusal. A run without them writes none. +// // Environment: // ACTIONS_TOKEN the job's GITHUB_TOKEN (actions: read) // GITHUB_REPOSITORY owner/repo @@ -25,6 +29,7 @@ // PR_MERGE_READS_PATH the job's shared GET answers (rotation.mjs // sharedReads), if set // VERIFY_ANSWERS_PATH the file to write +// EAM_SKIP_PATH where the skip records go, if set // GITHUB_API_URL import { renameSync, rmSync, writeFileSync } from "node:fs"; import process from "node:process"; @@ -36,21 +41,30 @@ import { ghDownload, newestTrustedRun, sharedReads } from "./rotation.mjs"; export const ANSWERS_ARTIFACT = "pr-merge-verify-answers"; export const ANSWERS_FILE = "verify-answers.json"; +export const SKIP_FILE = "eam-skip.json"; const REPO = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; const escapeCommand = (text) => String(text).replaceAll(/[\r\n%]/g, " "); const isObject = (value) => value !== null && typeof value === "object" && !Array.isArray(value); -// The newest trusted run's answers file and its run id, or undefined when no -// run has one; an error for a file that is not this repository's. -export async function readAnswers({ repo, runId, actions: client, download, shared }) { +// The newest trusted run holding the answers artifact, or undefined. +export async function answersRun({ repo, runId, actions: client, shared }) { const actions = sharedReads(client, shared); const accept = (run) => onMain({ repo, actions, sha: run.head_sha }); - const id = await newestTrustedRun({ repo, runId, actions, name: ANSWERS_ARTIFACT, accept }); - if (id === undefined) { - return undefined; + return newestTrustedRun({ repo, runId, actions, name: ANSWERS_ARTIFACT, accept }); +} + +// Run `id`'s answers file, as `{ run, text, count }`; undefined when the +// artifact holds none (a run whose reader read nothing), an error for a file +// that is not this repository's or a download that failed. +export function readAnswers({ repo, id, download }) { + let text; + try { + text = download(id); + } catch (error) { + if (error?.code === "ENOENT" && error?.syscall === "open") return undefined; + throw error; } - const text = download(id); const data = JSON.parse(text); if (!isObject(data) || data.repo !== repo || !isObject(data.entries)) { throw new Error(`the answers of run ${id} are not this repository's`); @@ -58,21 +72,39 @@ export async function readAnswers({ repo, runId, actions: client, download, shar return { run: id, text, count: Object.keys(data.entries).length }; } +const keep = (path, text) => { + writeFileSync(`${path}.tmp`, text); + renameSync(`${path}.tmp`, path); +}; + export async function main({ env = process.env, actions, download } = {}) { const repo = env.GITHUB_REPOSITORY ?? ""; try { if (!REPO.test(repo)) throw new Error("GITHUB_REPOSITORY is not owner/repo"); if (!env.VERIFY_ANSWERS_PATH) throw new Error("VERIFY_ANSWERS_PATH is not set"); rmSync(env.VERIFY_ANSWERS_PATH, { force: true }); + if (env.EAM_SKIP_PATH) rmSync(env.EAM_SKIP_PATH, { force: true }); actions ??= createGitHub({ token: env.ACTIONS_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); download ??= ghDownload({ repo, token: env.ACTIONS_TOKEN, dir: env.RUNNER_TEMP, name: ANSWERS_ARTIFACT, file: ANSWERS_FILE }); - const found = await readAnswers({ repo, runId: String(env.GITHUB_RUN_ID), actions, download, shared: env.PR_MERGE_READS_PATH }); - if (found === undefined) { + const id = await answersRun({ repo, runId: String(env.GITHUB_RUN_ID), actions, shared: env.PR_MERGE_READS_PATH }); + if (id === undefined) { console.log("no earlier verifier answers; each pull request is read in full"); return 0; } - writeFileSync(`${env.VERIFY_ANSWERS_PATH}.tmp`, found.text); - renameSync(`${env.VERIFY_ANSWERS_PATH}.tmp`, env.VERIFY_ANSWERS_PATH); + // The skip records first: a malformed answers file stops only the answers. + if (env.EAM_SKIP_PATH) { + try { + keep(env.EAM_SKIP_PATH, download(id, SKIP_FILE)); + } catch { + console.log(`run ${id} kept no skip records; each pull request is read`); + } + } + const found = readAnswers({ repo, id, download }); + if (found === undefined) { + console.log(`run ${id} kept no verifier answers; each pull request is read in full`); + return 0; + } + keep(env.VERIFY_ANSWERS_PATH, found.text); console.log(`verifier answers of run ${found.run}: ${found.count} pull requests`); } catch (error) { console.log(`::warning title=verifier answers::${escapeCommand(error?.message ?? error)}; each pull request is read in full`); diff --git a/.github/scripts/pr-merge/verify-answers.test.mjs b/.github/scripts/pr-merge/verify-answers.test.mjs index 882d7d0ec2..ac16a84925 100644 --- a/.github/scripts/pr-merge/verify-answers.test.mjs +++ b/.github/scripts/pr-merge/verify-answers.test.mjs @@ -11,7 +11,7 @@ import { after, test } from "node:test"; import { forgetMainTip } from "./head-ref-map.mjs"; import { mainCall } from "./on-main.fixture.mjs"; -import { ANSWERS_ARTIFACT, main } from "./verify-answers.mjs"; +import { ANSWERS_ARTIFACT, ANSWERS_FILE, SKIP_FILE, main } from "./verify-answers.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; const REPO = "melodic-software/claude-code-plugins"; @@ -22,11 +22,13 @@ const sha = (id) => String(id).padStart(40, "a"); const wf = (id, event, branch = "main") => ({ id, event, head_branch: branch, path: ".github/workflows/pr-merge.yml", head_sha: sha(id) }); let n = 0; -async function readRun({ pages = [[]], files = {}, offMain = [], fail, before } = {}) { +async function readRun({ pages = [[]], files = {}, skips = {}, offMain = [], fail, before, beforeSkips } = {}) { forgetMainTip(); n += 1; const path = join(dir, `in-${n}.json`); + const skipPath = join(dir, `skip-${n}.json`); if (before !== undefined) writeFileSync(path, before); + if (beforeSkips !== undefined) writeFileSync(skipPath, beforeSkips); const downloads = []; const actions = async (method, url) => { if (fail) throw fail; @@ -36,22 +38,32 @@ async function readRun({ pages = [[]], files = {}, offMain = [], fail, before } if (onMain !== undefined) return onMain; const id = /\/actions\/runs\/(\d+)\/artifacts\?per_page=100$/.exec(url)?.[1]; assert.ok(id, `unexpected call ${url}`); - const artifacts = id in files ? [{ name: ANSWERS_ARTIFACT, expired: false }] : [{ name: "pr-merge-head-refs", expired: false }]; + const artifacts = id in files || id in skips ? [{ name: ANSWERS_ARTIFACT, expired: false }] : [{ name: "pr-merge-head-refs", expired: false }]; return { total_count: artifacts.length, artifacts }; }; - const download = (id) => { - downloads.push(id); + // The run's artifact holds the answers file and, in `skips`, its skip records. + // A file the artifact lacks fails as readFileSync does; "download-fails" fails + // as `gh run download` does. + const missing = (file) => Object.assign(new Error(`ENOENT: no such file, open '${file}'`), { code: "ENOENT", syscall: "open" }); + const download = (id, file = ANSWERS_FILE) => { + downloads.push(file === ANSWERS_FILE ? id : `${id}:${file}`); + if (files[id] === "download-fails") throw new Error("Command failed: gh run download"); + if (file === SKIP_FILE) { + if (!(id in skips)) throw missing(file); + return skips[id]; + } + if (files[id] === null) throw missing(file); return files[id]; }; const lines = []; const log = console.log; console.log = (line) => lines.push(String(line)); try { - assert.equal(await main({ env: { GITHUB_REPOSITORY: REPO, GITHUB_RUN_ID: "999", VERIFY_ANSWERS_PATH: path }, actions, download }), 0); + assert.equal(await main({ env: { GITHUB_REPOSITORY: REPO, GITHUB_RUN_ID: "999", VERIFY_ANSWERS_PATH: path, EAM_SKIP_PATH: skipPath }, actions, download }), 0); } finally { console.log = log; } - return { written: existsSync(path) ? readFileSync(path, "utf8") : undefined, downloads, lines }; + return { written: existsSync(path) ? readFileSync(path, "utf8") : undefined, skipsWritten: existsSync(skipPath) ? readFileSync(skipPath, "utf8") : undefined, downloads, lines }; } test("the newest trusted run on main's answers are written as they were kept", async () => { @@ -62,8 +74,8 @@ test("the newest trusted run on main's answers are written as they were kept", a offMain: [sha(8)], }); // This run is skipped, a pull_request_target run is never asked, and run 8 - // is not on main. - assert.deepEqual(result.downloads, [7]); + // is not on main. Its skip records are read from the same download. + assert.deepEqual(result.downloads, [`7:${SKIP_FILE}`, 7]); assert.equal(result.written, kept); assert.ok(result.lines.includes("verifier answers of run 7: 1 pull requests"), result.lines.join("\n")); }); @@ -77,6 +89,7 @@ test("no earlier answers write nothing, and remove a file left at the path", asy test("an unreadable or foreign answers file writes nothing and warns", async () => { for (const options of [ { fail: new Error("HTTP 502") }, + { pages: [[wf(1, "schedule")]], files: { 1: "download-fails" } }, { pages: [[wf(1, "schedule")]], files: { 1: "not json" } }, { pages: [[wf(1, "schedule")]], files: { 1: answersOf({}, { repo: "someone/else" }) } }, { pages: [[wf(1, "schedule")]], files: { 1: answersOf([]) } }, @@ -86,3 +99,27 @@ test("an unreadable or foreign answers file writes nothing and warns", async () assert.ok(result.lines.some((line) => line.startsWith("::warning title=verifier answers::")), result.lines.join("\n")); } }); + +// enable-auto-merge's skip records (EAM_SKIP_IN) ride in the same artifact, +// so they cost no call of their own; a run without them hands on none. +test("the answers run's skip records are written beside its answers, and none is left planted", async () => { + const kept = JSON.stringify({ v: 1, repo: REPO, entries: {} }); + const found = await readRun({ pages: [[wf(7, "schedule")]], files: { 7: answersOf({}) }, skips: { 7: kept }, beforeSkips: "planted" }); + assert.equal(found.skipsWritten, kept); + assert.deepEqual(found.downloads, [`7:${SKIP_FILE}`, 7]); + const none = await readRun({ pages: [[wf(7, "schedule")]], files: { 7: answersOf({}) }, beforeSkips: "planted" }); + assert.equal(none.skipsWritten, undefined); + assert.ok(none.written !== undefined); + const noAnswers = await readRun({ pages: [[wf(5, "schedule")]], beforeSkips: "planted" }); + assert.equal(noAnswers.skipsWritten, undefined); +}); + +// A run whose reader read nothing (every pull request skipped or held) keeps +// no answers file; its skip records still reach the next run. +test("a run that kept only skip records hands them on, with no answers and no warning", async () => { + const kept = JSON.stringify({ v: 1, repo: REPO, entries: {} }); + const found = await readRun({ pages: [[wf(7, "schedule")]], files: { 7: null }, skips: { 7: kept } }); + assert.equal(found.skipsWritten, kept); + assert.equal(found.written, undefined); + assert.ok(!found.lines.some((line) => line.startsWith("::warning")), found.lines.join("\n")); +}); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index ebfa401276..f40d3f7453 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -1373,7 +1373,7 @@ test("enable-auto-merge reads the verifier answers only from a trusted run and u assert.match(upload, /\n {8}uses: actions\/upload-artifact@[0-9a-f]{40} /); // Bounded, so the job's timeout stays above the sum of its steps'. assert.match(upload, /\n {8}timeout-minutes: [0-9]+\n/); - assert.match(upload, /\n {10}name: pr-merge-verify-answers\n {10}path: \$\{\{ runner\.temp \}\}\/verify-answers\/verify-answers\.json\n {10}retention-days: 1\n {10}if-no-files-found: ignore\n/); + assert.match(upload, /\n {10}name: pr-merge-verify-answers\n {10}path: \|\n {12}\$\{\{ runner\.temp \}\}\/verify-answers\/verify-answers\.json\n {12}\$\{\{ runner\.temp \}\}\/verify-answers\/eam-skip\.json\n {10}retention-days: 1\n {10}if-no-files-found: ignore\n/); const names = [...arm.matchAll(/\n {6}- name: (.*)/g)].map((m) => m[1]); assert.ok(names.indexOf("Read the verifier answers the last run kept") < names.indexOf("Arm and disarm auto-merge")); assert.ok(names.indexOf("Arm and disarm auto-merge") < names.indexOf("Upload the verifier answers for the next run")); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 9791450c19..18936b43cc 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -65,12 +65,12 @@ name: pr-merge on: # Every trigger below is only a wake-up call. The run never trusts the event - # to say which PR to look at: it lists the open PRs from the GitHub API and - # checks each one, so any trigger leads to the same decisions. + # for a PR's state: it lists the open PRs from the GitHub API and checks + # each one it reads as listed. A pull_request_target run reads only the PR + # its event names (EVENT_PR); the schedule reads every one. - # A label was removed, the PR body changed, or a draft became ready. Since - # every run reads every PR, an event starts a run only when it can make a - # PR mergeable sooner. Adding a label can only add a hold, which the next + # A label was removed, the PR body changed, or a draft became ready. An + # event starts a run only when it can make its PR mergeable sooner. Adding a label can only add a hold, which the next # run sees anyway, so it starts none; a body edit can unblock (a new # closing-issue line, a removed `Hold:` line). pull_request_target runs # this file as it is on main, not the PR's copy, so a PR cannot change @@ -797,7 +797,7 @@ jobs: # wait in line (`queue: max`) instead of being dropped. Without it GitHub # keeps only one waiting run and cancels the rest, which leaves CANCELLED # `merge` checks on PRs and makes the merge hook refuse them. Waiting is - # harmless because every run re-reads every PR from the API, and a run + # harmless because every run re-reads its PRs from the API, and a run # with a pending successor that does its work skips the gate (the # superseded step). GitHub caps # the waiting line at 100 runs and rejects the rest, so the triggers above @@ -1036,6 +1036,8 @@ jobs: REPO: ${{ github.repository }} DEPENDABOT: ${{ steps.mode.outputs.dependabot }} LANE_PRS: ${{ steps.mode.outputs.lane-prs }} + # Empty on the schedule and a hand-started run. + EVENT_PR: ${{ github.event.pull_request.number }} run: | set -euo pipefail identity=$(yq '.merge."agent-identity" // ""' .base/docs/conventions/pr-pipeline.yaml) @@ -1081,36 +1083,50 @@ jobs: . .base/.github/actions/log-api-budget/api-budget.bash . .base/.github/scripts/pr-merge/rate-stop.bash api_budget_wrap_gh github-token - listing="$RUNNER_TEMP/listing.jsonl" - : >"$listing" - for author in "${authors[@]}"; do + listing="$RUNNER_TEMP/listing.json" + # One list of every open pull request into main (GraphQL), kept + # by author below, rather than one `gh pr list --author` search + # call per author. + limit=1000 + echo '[]' >"$listing" + if [ "${#authors[@]}" -gt 0 ]; then for attempt in 1 2 3; do - if gh pr list --repo "$REPO" --base main --state open --limit 500 \ - --author "$author" --json id,number,headRefOid,isDraft,updatedAt,labels,body >>"$listing" 2>"$RUNNER_TEMP/list-stderr"; then + if gh pr list --repo "$REPO" --base main --state open --limit "$limit" \ + --json id,number,headRefOid,isDraft,updatedAt,labels,body,author >"$listing" 2>"$RUNNER_TEMP/list-stderr"; then break fi cat "$RUNNER_TEMP/list-stderr" >&2 - stop=$(rate_stop_from "$RUNNER_TEMP/list-stderr" "gh pr list --author $author") + stop=$(rate_stop_from "$RUNNER_TEMP/list-stderr" "gh pr list") if [ -n "$stop" ]; then - echo "::error title=GitHub rate limit::gh pr list --author $author was rate limited ($stop); nothing more is sent, and the next scheduled run lists again" + echo "::error title=GitHub rate limit::gh pr list was rate limited ($stop); nothing more is sent, and the next scheduled run lists again" exit 1 fi if [ "$attempt" = 3 ]; then - echo "::error::gh pr list --author $author failed 3 times; no candidates listed, the gate does not run" + echo "::error::gh pr list failed 3 times; no candidates listed, the gate does not run" exit 1 fi sleep $((attempt * 15)) done - done - jq -cs '[(add // [])[] | select(.isDraft | not) + fi + if [ "$(jq length "$listing")" -ge "$limit" ]; then + echo "::error::gh pr list returned $limit pull requests, so the listing may be cut short; no candidates listed, the gate does not run" + exit 1 + fi + # A pull_request_target run gates only its own pull request, as the + # listing holds it; the scheduled run gates every candidate. + jq -c --args '($ARGS.positional | map(ascii_downcase)) as $authors + | [.[] | select((.author.login // "" | ascii_downcase) as $a | $authors | index($a)) + | select(env.EVENT_PR == "" or (.number | tostring) == env.EVENT_PR)]' \ + "${authors[@]}" <"$listing" >"$RUNNER_TEMP/authored.json" + jq -c '[.[] | select(.isDraft | not) | {number, head: .headRefOid, updatedAt, labels: [.labels[].name], body}] - | unique_by(.number)' "$listing" | + | unique_by(.number)' "$RUNNER_TEMP/authored.json" | python3 .base/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_prefilter.py \ --human-gated="$human_gated" >"$RUNNER_TEMP/prefilter.json" jq -c .candidates "$RUNNER_TEMP/prefilter.json" >"$RUNNER_TEMP/candidates.json" jq -c .held "$RUNNER_TEMP/prefilter.json" >"$RUNNER_TEMP/held.json" - jq -cs '[(add // [])[] | select(.isDraft) | {number, id, head: .headRefOid, updatedAt, - labels: [.labels[].name]}] | unique_by(.number)' "$listing" >"$RUNNER_TEMP/drafts.json" + jq -c '[.[] | select(.isDraft) | {number, id, head: .headRefOid, updatedAt, + labels: [.labels[].name]}] | unique_by(.number)' "$RUNNER_TEMP/authored.json" >"$RUNNER_TEMP/drafts.json" echo "count=$(jq length "$RUNNER_TEMP/candidates.json")" >>"$GITHUB_OUTPUT" # The gate loop below can stop at its time budget, so it starts after @@ -2135,6 +2151,8 @@ jobs: API_BUDGET_TOKEN: github-token ACTIONS_TOKEN: ${{ github.token }} VERIFY_ANSWERS_PATH: ${{ runner.temp }}/verify-answers-in.json + # The arm step's skip records, from the same artifact. + EAM_SKIP_PATH: ${{ runner.temp }}/eam-skip-in.json PR_MERGE_READS_PATH: ${{ runner.temp }}/pr-merge-reads.json run: node .base/.github/scripts/pr-merge/verify-answers.mjs @@ -2186,6 +2204,8 @@ jobs: KILL_SWITCH: ${{ steps.kill.outputs.proceed == 'true' && 'proceed' || '' }} RESOLVED_CONFIG: ${{ runner.temp }}/resolved-config.json EVENT_NAME: ${{ github.event_name }} + # Empty on the schedule and a hand-started run. + EVENT_PR: ${{ github.event.pull_request.number }} BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }} BABYSIT_VERIFY_LISTING: ${{ runner.temp }}/verify-listing.json BABYSIT_VERIFY_LISTING_CARRY: ${{ runner.temp }}/verify-listing-carry.json @@ -2197,6 +2217,10 @@ jobs: # The verifier answers read above, and the file this run keeps. VERIFY_ANSWERS_IN: ${{ runner.temp }}/verify-answers-in.json VERIFY_ANSWERS_OUT: ${{ runner.temp }}/verify-answers/verify-answers.json + # Skip-unchanged (enable-auto-merge.mjs SKIPPABLE): the records read + # above, and the file this run keeps beside its answers. + EAM_SKIP_IN: ${{ runner.temp }}/eam-skip-in.json + EAM_SKIP_OUT: ${{ runner.temp }}/verify-answers/eam-skip.json # The lane's room (lane-room): this run's post-merge report and the # rung cap as the merge job passes it, empty meaning no cap. POSTMERGE_RESULT: ${{ runner.temp }}/post-merge/post-merge.json @@ -2215,16 +2239,20 @@ jobs: LANES_BOT_LOGIN=$(yq '.merge."agent-identity" // ""' "$config") RISK_ESCALATION_MINUTES=$(yq '.merge."risk-escalation-minutes" // ""' "$config") VERIFY_CACHE=$(yq '.merge."verify-cache" // "shadow"' "$config") + SKIP_MAX_MINUTES=$(yq '.merge."skip-unchanged"."max-minutes" // ""' "$config") + SKIP_MAX_SKIPS=$(yq '.merge."skip-unchanged"."max-skips" // ""' "$config") + mkdir -p "$(dirname "$EAM_SKIP_OUT")" # The commit whose code ran the report's check, for lane-room: none # reads the report again. POSTMERGE_CODE_SHA=$(cat "$RUNNER_TEMP/post-merge/post-merge-code-sha" 2>/dev/null || true) + export SKIP_MAX_MINUTES SKIP_MAX_SKIPS export REPORT_ONLY LANES_BOT_LOGIN RISK_ESCALATION_MINUTES VERIFY_CACHE POSTMERGE_CODE_SHA node .base/.github/scripts/pr-merge/enable-auto-merge.mjs # The answers this run's reader kept, for the next run: refusals only, - # each to expire within the hour. A file name of its own: the merge job - # uploads pr-merge-head-refs. Only a scheduled or hand-started run's file - # is read back. + # each to expire within the hour, with the arm step's skip records. A + # file name of its own: the merge job uploads pr-merge-head-refs. Only a + # scheduled or hand-started run's file is read back. - name: Upload the verifier answers for the next run if: >- always() && steps.arm.outcome != 'skipped' @@ -2234,7 +2262,9 @@ jobs: uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: name: pr-merge-verify-answers - path: ${{ runner.temp }}/verify-answers/verify-answers.json + path: | + ${{ runner.temp }}/verify-answers/verify-answers.json + ${{ runner.temp }}/verify-answers/eam-skip.json retention-days: 1 if-no-files-found: ignore diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index 6ec73fbaec..beec0280f5 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -1033,7 +1033,8 @@ merge: # figures come from serve's order. Any other value fails the schema. verify-cache: shadow # A candidate the last run refused on unchanged inputs is reported with that - # refusal instead of gated again (babysit_merge_skip.py); it is gated again + # refusal instead of gated again (babysit_merge_skip.py, and for a lanes + # App pull request's lane refusal enable-auto-merge.mjs); it is gated again # once its refusal is max-minutes old or has been skipped max-skips runs in # a row. 3 skips at the 15-minute schedule gate it about hourly; 60 caps # the wait whatever else starts runs (judgment). 0 in either: no skips.