diff --git a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md new file mode 100644 index 0000000000..991288bc1d --- /dev/null +++ b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Fixed + +- pr-merge's enable-auto-merge job no longer fails its arm with `fetch failed` after a long verifier read. Every request now sends `Connection: close`, so none goes out on a pooled keep-alive connection that sat idle while the verifier ran and that GitHub had closed. A GraphQL request that still fails on the network (Node's "fetch failed", or a cause code of `ECONNRESET`, `ETIMEDOUT`, `UND_ERR_SOCKET` or `UND_ERR_CONNECT_TIMEOUT`, or "other side closed") is sent up to twice more (a REST write, such as the refusal comment, never is), after full-jitter waits capped at 4 s and then 8 s. An HTTP error answer or a GraphQL error is not sent again. A resent arm passes the same `expectedHeadOid`, so it arms only the head the run read. A pull request whose arm or disarm still fails is left as it was and named with the error's cause code and message, with any token scrubbed. The run then goes on to the other pull requests and exits 1. +- A lanes App pull request that native auto-merge leaves unarmed no longer sits with only an Actions log line to say why. On a scheduled or hand-started run, a refusal on a hold path, a cost path, the gate's verifier verdict or its risk score is written on the pull request in one `` comment that names the head and the reason. The comment is edited in place when the reason changes, and left alone while it does not. A refusal a person must clear also gets the human-gated label: a hold or cost path, an issue link the lane does not take, a verdict the gate marks `needsHuman`, or a risk hold that has stood unchanged for `merge.risk-escalation-minutes`. That comment carries the attended queue's escalation marker. A refusal the lanes clear on their own, such as a missing verdict, gets the comment only. A label a person removes stays off until the reason or the head changes. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 1faa628db6..dba8c6c4dd 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -62,9 +62,25 @@ // lane applied its hold label), a symlink, submodule or other non-regular // mode, a high path, the manifest review, the trust gate and the cost // review hold. Read only after the verdict allows. +// A reader run that leaves a lanes App pull request unarmed, or disarms it, +// on a hold path, a cost hold, the gate's verdict or its risk score says why +// on the pull request, since under merge.lane-prs native the gate never +// lists it: one `` comment, edited in place and +// left alone while the reason and head are unchanged (refusalWrites). A +// refusal a person must clear (a hold or cost path, an issue link the lane +// does not take, a verdict the gate marks needsHuman, a risk hold whose +// comment has stood merge.risk-escalation-minutes) also gets the +// human-gated label, put back while the comment stands unless a person +// removed it since; clear-escalations never removes it under native, +// so the person removes it after fixing the reason. Holds a person already +// set (labels, a body hold, a veto), the room and failed reads write nothing. // Arming passes the head just read as expectedHeadOid, so a push in between // fails the arm rather than arming a head nobody read. An armed pull request -// that fails any rule is disarmed. While merge.report-only is true it reports +// that fails any rule is disarmed. Each request opens its own connection, and +// a read or write that fails on the network is sent up to twice more +// (withRetry). One whose arm or disarm still fails is left as it was, named +// with its cause, and the run goes on to the rest, then exits 1. While +// merge.report-only is true it reports // each decision and writes nothing. // // Environment: @@ -91,7 +107,7 @@ import { readFileSync } 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 { createGitHub, errorLabel } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; import { readHoldPaths } from "../work-resolve-issue/report-outcome.mjs"; import { BODY_HOLD, costDecision, costHold, humansOf, pathHold, vetoers } from "./merge-holds.mjs"; @@ -131,7 +147,7 @@ export const PULLS_QUERY = `query($owner: String!, $name: String!, $after: Strin export const DETAIL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { - comments(last: 100) { totalCount nodes { author { __typename login } body createdAt authorAssociation } } + comments(last: 100) { totalCount nodes { databaseId author { __typename login } body createdAt updatedAt authorAssociation } } reviews(last: 100) { totalCount nodes { author { __typename login } body submittedAt authorAssociation } } reviewThreads(last: 100) { totalCount @@ -152,6 +168,57 @@ export const DISARM_MUTATION = `mutation($id: ID!) { const escapeCommand = (text) => String(text).replaceAll(/[\r\n%]/g, " "); +// A network failure, with no answer from GitHub: undici's "fetch failed" +// TypeError, which every failure of Node's fetch is whatever its cause, or any +// error whose cause has one of these codes or undici's "other side closed". +// An HTTP answer (a 4xx included) and a GraphQL error are GitHubErrors with +// no cause, never this. +const TRANSIENT_CODES = new Set(["ECONNRESET", "ETIMEDOUT", "UND_ERR_SOCKET", "UND_ERR_CONNECT_TIMEOUT"]); +export const transient = (error) => + (error?.name === "TypeError" && error.message === "fetch failed") || + TRANSIENT_CODES.has(error?.cause?.code) || + error?.cause?.message === "other side closed"; +// At most three attempts. Before each resend, a full-jitter wait: a random +// share of 4 s, then of 8 s, so the waits sum to at most 12 s. +export const RETRY_CAPS_MS = Object.freeze([4000, 8000]); +// An error's message with its cause's code and message, any token scrubbed: +// undici puts a whole header value in some messages. +export function describe(error) { + const cause = error?.cause; + const text = cause ? `${error?.message ?? error} (cause: ${[cause.code, cause.message].filter(Boolean).join(" ")})` : String(error?.message ?? error); + return escapeCommand(text.replaceAll(/Bearer\s+\S+|gh[pousr]_\w+|github_pat_\w+/g, "***")); +} +// `github` with every request on a connection of its own, and a GraphQL +// POST that fails on the network sent again (RETRY_CAPS_MS). `Connection: close` closes +// each socket after its answer, so no request goes out on a pooled one that +// sat idle through a verifier read and that GitHub has since closed. A GET is +// left to createGitHub's own retry (retryServerErrors), which resends only +// GETs; a REST write such as the refusal comment is never resent, since a +// lost answer would post it twice. A resent arm is pinned to the same expectedHeadOid, so it can arm +// only the head the run read. When the lost answer was a success, the resend +// may arm or disarm a pull request already in that state; should GitHub +// answer that with an error, the run reports it and exits 1 with the state +// right, and the next run reads it as it is. +export function withRetry(github, { sleep, random = Math.random }) { + return async (method, path, body, headers = {}, options) => { + for (let attempt = 0; ; attempt += 1) { + try { + return await github(method, path, body, { Connection: "close", ...headers }, options); + } catch (error) { + const cap = RETRY_CAPS_MS[attempt]; + if (path !== "/graphql" || cap === undefined || !transient(error)) { + throw error; + } + const wait = Math.round(random() * cap); + const what = body?.query === ARM_MUTATION ? " (arm)" : body?.query === DISARM_MUTATION ? " (disarm)" : ""; + const pull = body?.variables?.number ?? body?.variables?.id; + console.log(`::warning::enable-auto-merge: ${method} ${path}${what}${pull === undefined ? "" : ` for ${pull}`} failed (${errorLabel(error)}); sending it again in ${wait} ms (retry ${attempt + 1} of ${RETRY_CAPS_MS.length})`); + await sleep(wait); + } + } + }; +} + // Why `pull` must not be armed, or null when it may be. export function holdReason(pull, humanGated) { const labels = pull.labels?.nodes ?? []; @@ -239,6 +306,116 @@ export function verifyHold(evidence, head) { return evidence.reviews === null ? null : "its risk score could not be read"; } +// Who clears the refusal verifyHold reads in `evidence` at `head`: `person` +// for an issue link the lane does not take or a verifier refusal the gate +// marks needsHuman (babysit_verify_evidence.NEEDS_HUMAN), `risk` for a hold +// on the risk score (a person once it stands, as the gate escalates it), +// `lane` for any other verifier refusal, and null for an unread answer, +// which the next run reads again. +export function verifyClears(evidence, head) { + if (evidence === null || typeof evidence !== "object" || evidence.head !== head) { + return null; + } + if (typeof evidence.ineligible === "string") { + return "person"; + } + if (typeof evidence.refusal === "string") { + return evidence.needsHuman === true ? "person" : "lane"; + } + return evidence.ineligible === null && typeof evidence.reviews === "string" ? "risk" : null; +} + +// The refusal comment on a lanes App pull request the run leaves unarmed: +// pr-merge's reasons-comment marker and shape (babysit_merge.py +// risk_comment_body), its second line naming the head, and for a person's +// refusal the attended queue's escalation marker first (work-items +// escalation-marker.md), as the gate's Dependabot escalation carries it. A +// function of the class, the reason and the head only, so an unchanged +// refusal renders the same body. +export const REFUSAL_MARKER = ""; +export const ATTEND_QUEUE_MARKER = ""; +export function refusalBody(person, reason, head, label) { + const who = person + ? `A person must clear this reason, so the pr-merge lane added the \`${label}\` label: fix the reason, then remove the label, and the next scheduled run judges it again.` + : "The lanes clear this reason on their own; the next scheduled run judges it again."; + return [ + ...(person ? [ATTEND_QUEUE_MARKER] : []), + REFUSAL_MARKER, + `Unattended merge: native auto-merge leaves this pull request unarmed at head \`${head}\`. ${who}`, + "", + "```text", + String(reason).replaceAll(/[\u0000-\u0008\u000b-\u001f\u007f]/g, "?").replaceAll("```", "'''"), + "```", + ].join("\n"); +} + +// The lanes App's newest refusal comment in `comments` (DETAIL_QUERY's). +function priorRefusal(comments, bot) { + return (comments ?? []) + .filter((c) => c?.author?.__typename === "Bot" && c.author.login === bot && String(c.body ?? "").split("\n", 2).map((line) => line.replace(/\r$/, "")).includes(REFUSAL_MARKER)) + .sort((a, b) => String(a.createdAt).localeCompare(String(b.createdAt))) + .at(-1); +} + +// merge.risk-escalation-minutes from the resolved config, else the gate's 60 +// (babysit_merge_policy.RISK_ESCALATION_MINUTES). +export function riskEscalationMinutes(path) { + try { + const minutes = JSON.parse(readFileSync(path, "utf8"))?.merge?.["risk-escalation-minutes"]; + return Number.isSafeInteger(minutes) && minutes >= 0 ? minutes : 60; + } catch { + return 60; + } +} + +// The comment and label write for a refusal `clears` (verifyClears, or +// `person` for a hold path or cost hold) with `reason` at `pull`'s head: the +// comment first, so a label never stands unexplained, and the label only +// when this write made the comment a person's, so a label a person removed +// stays off until the reason or the head changes. A risk hold is a lane's +// until its comment has stood unchanged for `minutes`, the gate's +// escalation rule. Returns what it wrote, or null when the comment already +// says this. +export function refusalWrites(pull, comments, { clears, reason }, { bot, label, minutes, now }) { + const prior = priorRefusal(comments, bot); + const lane = refusalBody(false, reason, pull.headRefOid, label); + const person = refusalBody(true, reason, pull.headRefOid, label); + const stood = prior?.body === lane && now - Date.parse(String(prior.updatedAt ?? prior.createdAt)) >= minutes * 60_000; + const body = clears === "person" || (clears === "risk" && (prior?.body === person || stood)) ? person : lane; + // A person's comment already standing while the label is off (a pull + // request carrying it never gets here: holdReason holds it): the label + // goes back on unless a person removed it after the comment last changed, + // which the caller reads from the timeline (`liftedSince`). + if (prior?.body === body) { + return body === person ? { body: null, commentId: prior.databaseId ?? null, label, liftedSince: String(prior.updatedAt ?? prior.createdAt) } : null; + } + return { body, commentId: prior?.databaseId ?? null, label: body === person ? label : null }; +} + +// The newest label removals on a pull request, for refusalWrites' lift check. +export const UNLABELED_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + timelineItems(itemTypes: [UNLABELED_EVENT], last: 100) { totalCount nodes { ... on UnlabeledEvent { createdAt actor { __typename login } label { name } } } } + } + } +}`; + +// Whether a person removed `label` at or after `since`, from UNLABELED_QUERY's +// timeline; a timeline not read whole counts as removed, so an unknown never +// puts back a label a person may have taken off. +export function liftedByPerson(timeline, label, since) { + if (!Array.isArray(timeline?.nodes) || !(timeline.totalCount <= timeline.nodes.length)) { + return true; + } + return timeline.nodes.some( + (event) => + event?.actor?.__typename === "User" && + String(event?.label?.name).toLowerCase() === String(label).toLowerCase() && + Date.parse(String(event?.createdAt)) >= Date.parse(since), + ); +} + // The gate's own reader, one process per pull request; the run shares its // artifact listing, head-ref map and run cache through the BABYSIT_* files. export function pythonVerify(env) { @@ -340,7 +517,11 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { if (detail === null) { return { action: "none", reason: "its discussion did not read" }; } - const vetoed = pathHold(detail.files, holdPaths) ?? costHold(detail.files, detail.costDecision) ?? discussionHold(detail); + const pathHeld = pathHold(detail.files, holdPaths) ?? costHold(detail.files, detail.costDecision); + if (pathHeld !== null) { + return { action: armed ? "disarm" : "none", reason: pathHeld, clears: "person" }; + } + const vetoed = discussionHold(detail); if (vetoed !== null) { return { action: armed ? "disarm" : "none", reason: vetoed }; } @@ -352,7 +533,8 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { } const unverified = verifyHold(detail.verify, pull.headRefOid); if (unverified !== null) { - return { action: armed ? "disarm" : "none", reason: unverified }; + const clears = verifyClears(detail.verify, pull.headRefOid); + return { action: armed ? "disarm" : "none", reason: unverified, ...(clears ? { clears } : {}) }; } return armed ? { action: "none", reason: "already armed" } : { action: "arm", reason: "no hold" }; } @@ -375,7 +557,7 @@ async function listPulls(github, owner, name) { throw new Error(`more than ${MAX_PAGES * 100} open pull requests into main`); } -export async function main({ env = process.env, github, verify, room } = {}) { +export async function main({ env = process.env, github, verify, room, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random, now = () => Date.now() } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -392,7 +574,7 @@ export async function main({ env = process.env, github, verify, room } = {}) { const write = env.REPORT_ONLY === "false"; const humanGated = humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json"); const holdPaths = readHoldPaths(env.RESOLVED_CONFIG ?? ""); - github ??= createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); + github = withRetry(github ?? createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }), { sleep, random }); verify ??= pythonVerify(env); const reads = READER_EVENTS.includes(env.EVENT_NAME); const pulls = await listPulls(github, repo[1], repo[2]); @@ -404,7 +586,7 @@ export async function main({ env = process.env, github, verify, room } = {}) { lane = readRoom(await (room ?? pythonRoom(env))()); } catch (error) { lane = readRoom(null); - lane.unreadable = escapeCommand(error?.message ?? error); + lane.unreadable = describe(error); } console.log(`enable-auto-merge: lane room: ${escapeCommand(JSON.stringify(lane))}`); } @@ -432,8 +614,51 @@ export async function main({ env = process.env, github, verify, room } = {}) { return inFlight >= lane.cap ? { disarm: false, reason: `merge.max-in-flight is ${lane.cap} and ${inFlight} merges are in flight` } : null; }; let failed = 0; + const minutes = riskEscalationMinutes(env.RESOLVED_CONFIG ?? ""); + // A reader run's per pull request refusal (a hold path or cost hold, or + // the gate's verdict or risk score) goes on the pull request itself: + // under merge.lane-prs native the gate never lists it, so a log line + // would be all anyone saw. + const note = async (pull, detail, refusal) => { + if (!reads || refusal.clears === undefined) { + return; + } + const writes = refusalWrites(pull, detail?.comments?.nodes, refusal, { bot: slug, label: humanGated, minutes, now: now() }); + if (writes === null) { + return; + } + if (!write) { + console.log(`enable-auto-merge: #${pull.number} would note its refusal (report-only)${writes.label ? `, labeled ${writes.label}` : ""}`); + return; + } + const base = `/repos/${repo[0]}/issues`; + try { + if (writes.body === null) { + const variables = { owner: repo[1], name: repo[2], number: pull.number }; + const timeline = (await github("POST", "/graphql", { query: UNLABELED_QUERY, variables }))?.data?.repository?.pullRequest?.timelineItems; + if (liftedByPerson(timeline, writes.label, writes.liftedSince)) { + console.log(`enable-auto-merge: #${pull.number} left unlabeled: a person removed ${escapeCommand(writes.label)} after its refusal comment, or the timeline did not read whole`); + return; + } + await github("POST", `${base}/${pull.number}/labels`, { labels: [writes.label] }); + console.log(`enable-auto-merge: #${pull.number} labeled ${writes.label} again: its refusal comment stands and no person removed the label`); + return; + } + await (writes.commentId === null + ? github("POST", `${base}/${pull.number}/comments`, { body: writes.body }) + : github("PATCH", `${base}/comments/${writes.commentId}`, { body: writes.body })); + if (writes.label) { + await github("POST", `${base}/${pull.number}/labels`, { labels: [writes.label] }); + } + console.log(`enable-auto-merge: #${pull.number} refusal noted on the pull request${writes.label ? `, labeled ${writes.label}` : ""}`); + } catch (error) { + failed += 1; + console.log(`::error title=enable-auto-merge::#${pull.number} refusal note: ${describe(error)}`); + } + }; for (const pull of pulls) { let { action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }); + let clears; let detail = null; // Checked before the reads, so a held lane spends none. const held = ["read", "verify", "arm"].includes(action) ? roomHold(pull) : null; @@ -453,9 +678,9 @@ export async function main({ env = process.env, github, verify, room } = {}) { detail = { ...detail, files, costDecision: decision }; } } catch (error) { - console.log(`enable-auto-merge: #${pull.number} discussion did not read: ${escapeCommand(error?.message ?? error)}`); + console.log(`enable-auto-merge: #${pull.number} discussion did not read: ${describe(error)}`); } - ({ action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }, detail)); + ({ action, reason, clears } = decide(pull, { bot: slug, humanGated, holdPaths }, detail)); } if (action === "verify" && !reads) { ({ action, reason } = @@ -469,9 +694,9 @@ export async function main({ env = process.env, github, verify, room } = {}) { try { evidence = await verify(pull, lane?.rung); } catch (error) { - console.log(`enable-auto-merge: #${pull.number} verifier verdict did not read: ${escapeCommand(error?.message ?? error)}`); + console.log(`enable-auto-merge: #${pull.number} verifier verdict did not read: ${describe(error)}`); } - ({ action, reason } = decide(pull, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence })); + ({ action, reason, clears } = decide(pull, { bot: slug, humanGated, holdPaths }, { ...detail, verify: evidence })); } // A pull request armed earlier in this run counts toward the cap. const full = action === "arm" ? roomHold(pull) : null; @@ -485,10 +710,12 @@ export async function main({ env = process.env, github, verify, room } = {}) { if (reason !== "not the lanes App's") { console.log(`enable-auto-merge: #${pull.number} left: ${escapeCommand(reason)}`); } + await note(pull, detail, { clears, reason }); continue; } if (!write) { console.log(`enable-auto-merge: #${pull.number} would ${action} (report-only): ${escapeCommand(reason)}`); + await note(pull, detail, { clears, reason }); continue; } try { @@ -501,12 +728,17 @@ export async function main({ env = process.env, github, verify, room } = {}) { } } catch (error) { failed += 1; - console.log(`::error title=enable-auto-merge::#${pull.number} ${action}: ${escapeCommand(error?.message ?? error)}`); + console.log(`::error title=enable-auto-merge::#${pull.number} ${action}: ${describe(error)}`); + // Still armed: a note saying it is unarmed would be false. + continue; + } + if (action === "disarm") { + await note(pull, detail, { clears, reason }); } } return failed === 0 ? 0 : 1; } catch (error) { - console.log(`::error title=enable-auto-merge::${escapeCommand(error?.message ?? error)}`); + console.log(`::error title=enable-auto-merge::${describe(error)}`); return 1; } } diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index d787a38f84..527e9725fd 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -22,13 +22,16 @@ import { DISARM_MUTATION, HUMAN_VETO, PULLS_QUERY, + UNLABELED_QUERY, decide, + liftedByPerson, READER_EVENTS, main, pythonRoom, readRoom, verifyHold, } from "./enable-auto-merge.mjs"; +import { GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; import { decide as decideOutcome } from "../work-resolve-issue/report-outcome.mjs"; @@ -91,11 +94,19 @@ const ROOM = { refusal: null, unreadable: null, count: 0, cap: 5, queued: [], ru const room = (answer = ROOM) => async () => answer; const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name) => ({ name })) }); -function world(pulls, { failArm = false } = {}) { +// armErrors: per pull request id, the errors its arm sends throw, in order. +// filesError: what every changed-files GET throws. +function world(pulls, { failArm = false, armErrors = {}, filesError = null, noteErrors = [], disarmError = null } = {}) { + const noteThrows = [...noteErrors]; + const armThrows = Object.fromEntries(Object.entries(armErrors).map(([id, errors]) => [id, [...errors]])); const calls = []; - const github = async (method, path, body) => { - calls.push({ method, path, body }); + const notes = []; + const github = async (method, path, body, headers) => { + calls.push({ method, path, body, headers }); const listing = /^\/repos\/melodic-software\/claude-code-plugins\/pulls\/([0-9]+)\/files\?per_page=100&page=([0-9]+)$/.exec(path); + if (method === "GET" && listing && filesError) { + throw filesError; + } if (method === "GET" && listing) { const files = pulls.find((candidate) => candidate.number === Number(listing[1]))?.files ?? [{ filename: "plugins/work-items/README.md" }]; return files.slice((Number(listing[2]) - 1) * 100, Number(listing[2]) * 100); @@ -106,6 +117,15 @@ function world(pulls, { failArm = false } = {}) { if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/issues/comments/7") { return COST_COMMENT; } + // The refusal note: a comment posted or edited, and the label. + const note = /^\/repos\/melodic-software\/claude-code-plugins\/issues\/(?:([0-9]+)\/(comments|labels)|comments\/([0-9]+))$/.exec(path); + if (note && (method === "POST" || method === "PATCH")) { + notes.push({ method, path, body }); + if (noteThrows.length > 0) { + throw noteThrows.shift(); + } + return {}; + } assert.equal(`${method} ${path}`, "POST /graphql"); if (body.query === PULL_OPENED_QUERY) { const nodes = [{ number: body.variables.number + 1000, repository: { nameWithOwner: REPO } }]; @@ -118,10 +138,20 @@ function world(pulls, { failArm = false } = {}) { assert.deepEqual(body.variables, { owner: "melodic-software", name: "claude-code-plugins", after: null }); return { data: { repository: { pullRequests: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: pulls } } } }; } + 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: detail(found?.discussion) } } }; } + if (body.query === ARM_MUTATION && armThrows[body.variables.id]?.length > 0) { + throw armThrows[body.variables.id].shift(); + } + if (body.query === DISARM_MUTATION && disarmError) { + throw disarmError; + } if (body.query === ARM_MUTATION && failArm) { throw new Error("POST /graphql (GraphQL errors) failed with status 200"); } @@ -140,24 +170,170 @@ function world(pulls, { failArm = false } = {}) { }; const writes = () => calls - .filter(({ method, body }) => method === "POST" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY].includes(body.query)) + .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY, UNLABELED_QUERY].includes(body.query)) .map(({ body }) => ({ mutation: body.query === ARM_MUTATION ? "arm" : body.query === DISARM_MUTATION ? "disarm" : "?", ...body.variables })); - return { github, writes, verify, verified, calls }; + return { github, writes, verify, verified, calls, notes }; } async function run(pulls, env = {}, options = {}, lane = ROOM) { - const { github, writes, verify, verified } = world(pulls, options); + const { github, writes, verify, verified, calls, notes } = world(pulls, options); const lines = []; + const slept = []; const log = console.log; console.log = (line) => lines.push(line); try { - const code = await main({ env: { ...ENV, ...env }, github, verify, room: room(lane) }); - return { code, writes: writes(), lines, verified }; + const code = await main({ env: { ...ENV, ...env }, github, verify, room: room(lane), sleep: async (ms) => slept.push(ms), random: options.random, now: options.now }); + return { code, writes: writes(), lines, verified, slept, calls, notes }; } finally { console.log = log; } } +// The refusal note's two bodies, written out by hand: the gate's reasons +// marker on its own line, the head named on the next, the reason fenced. +const laneNote = (reason) => + [ + "", + `Unattended merge: native auto-merge leaves this pull request unarmed at head \`${HEAD}\`. The lanes clear this reason on their own; the next scheduled run judges it again.`, + "", + "```text", + reason, + "```", + ].join("\n"); +const personNote = (reason) => + [ + "", + "", + `Unattended merge: native auto-merge leaves this pull request unarmed at head \`${HEAD}\`. A person must clear this reason, so the pr-merge lane added the \`needs-human\` label: fix the reason, then remove the label, and the next scheduled run judges it again.`, + "", + "```text", + reason, + "```", + ].join("\n"); +const ISSUES = "/repos/melodic-software/claude-code-plugins/issues"; +const lanesSaid = (body, updatedAt = "2026-10-23T10:00:00Z") => ({ + databaseId: 77, + author: { __typename: "Bot", login: LANES.login }, + body, + createdAt: "2026-10-23T10:00:00Z", + updatedAt, + authorAssociation: "NONE", +}); +const HOLD_PATH_REASON = "it changes docs/held.md, a hold path"; +const MISSING = { refusal: "verify:missing", needsHuman: false, head: HEAD, ineligible: null, reviews: null }; +const MISSING_REASON = "pr-merge's gate refuses its verifier verdict (verify:missing)"; + +test("a refusal a person must clear is commented on and labeled human-gated", async () => { + const held = pull(1, { files: [{ filename: "docs/held.md" }] }); + const { code, writes, notes } = await run([held]); + assert.equal(code, 0); + assert.deepEqual(writes, []); + assert.deepEqual(notes, [ + { method: "POST", path: `${ISSUES}/1/comments`, body: { body: personNote(HOLD_PATH_REASON) } }, + { method: "POST", path: `${ISSUES}/1/labels`, body: { labels: ["needs-human"] } }, + ]); +}); + +test("a verifier refusal the gate marks needsHuman is a person's; one it does not is a lane's", async () => { + const human = { refusal: "verify:spec-changed", needsHuman: true, head: HEAD, ineligible: null, reviews: null }; + const { notes } = await run([pull(1, { verdict: human }), pull(2, { verdict: MISSING })]); + assert.deepEqual(notes, [ + { method: "POST", path: `${ISSUES}/1/comments`, body: { body: personNote("pr-merge's gate refuses its verifier verdict (verify:spec-changed)") } }, + { method: "POST", path: `${ISSUES}/1/labels`, body: { labels: ["needs-human"] } }, + { method: "POST", path: `${ISSUES}/2/comments`, body: { body: laneNote(MISSING_REASON) } }, + ]); +}); + +test("a refusal a lane clears is commented on, with no label", async () => { + const { code, notes } = await run([pull(1, { verdict: MISSING })]); + assert.equal(code, 0); + assert.deepEqual(notes, [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: laneNote(MISSING_REASON) } }]); +}); + +const unlabeled = (at, typename = "User", name = "needs-human") => ({ createdAt: at, actor: { __typename: typename, login: "someone" }, label: { name } }); + +test("an unchanged refusal writes nothing, and a label a person removed stays off", async () => { + const lane = pull(1, { verdict: MISSING, discussion: { comments: [lanesSaid(laneNote(MISSING_REASON))] } }); + const person = pull(2, { + files: [{ filename: "docs/held.md" }], + discussion: { comments: [lanesSaid(personNote(HOLD_PATH_REASON))] }, + unlabeled: [unlabeled("2026-10-23T10:30:00Z", "User", "Needs-Human")], + }); + const { code, notes } = await run([lane, person]); + assert.equal(code, 0); + assert.deepEqual(notes, []); +}); + +test("a person's refusal comment standing without its label gets the label back", async () => { + // An earlier run's label write failed: no removal at all, a bot's, a + // person's before the comment last changed, or another label's. + for (const removals of [[], [unlabeled("2026-10-23T10:30:00Z", "Bot")], [unlabeled("2026-10-23T09:59:00Z")], [unlabeled("2026-10-23T10:30:00Z", "User", "do-not-merge")]]) { + const person = pull(2, { files: [{ filename: "docs/held.md" }], discussion: { comments: [lanesSaid(personNote(HOLD_PATH_REASON))] }, unlabeled: removals }); + const { code, notes } = await run([person]); + assert.equal(code, 0); + assert.deepEqual(notes, [{ method: "POST", path: `${ISSUES}/2/labels`, body: { labels: ["needs-human"] } }]); + } +}); + +test("a timeline not read whole puts no label back", () => { + assert.equal(liftedByPerson({ totalCount: 101, nodes: [] }, "needs-human", "2026-10-23T10:00:00Z"), true); + assert.equal(liftedByPerson(null, "needs-human", "2026-10-23T10:00:00Z"), true); + assert.equal(liftedByPerson({ totalCount: 0, nodes: [] }, "needs-human", "2026-10-23T10:00:00Z"), false); +}); + +test("a changed refusal edits the one comment instead of posting another", async () => { + const moved = pull(1, { verdict: MISSING, discussion: { comments: [lanesSaid(laneNote("an earlier reason"))] } }); + const { notes } = await run([moved]); + assert.deepEqual(notes, [{ method: "PATCH", path: `${ISSUES}/comments/77`, body: { body: laneNote(MISSING_REASON) } }]); +}); + +test("a risk hold is a lane's until its comment has stood an hour unchanged, then a person's", async () => { + const risk = { refusal: null, needsHuman: false, head: HEAD, ineligible: null, reviews: "evidence-unbound:claude-review" }; + const reason = "pr-merge's gate holds it on its risk score (evidence-unbound:claude-review)"; + const at = (updatedAt) => pull(1, { verdict: risk, discussion: { comments: [lanesSaid(laneNote(reason), updatedAt)] } }); + const now = () => Date.parse("2026-10-23T11:00:00Z"); + assert.deepEqual((await run([pull(1, { verdict: risk })], {}, { now })).notes, [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: laneNote(reason) } }]); + assert.deepEqual((await run([at("2026-10-23T10:01:00Z")], {}, { now })).notes, []); + assert.deepEqual((await run([at("2026-10-23T10:00:00Z")], {}, { now })).notes, [ + { method: "PATCH", path: `${ISSUES}/comments/77`, body: { body: personNote(reason) } }, + { method: "POST", path: `${ISSUES}/1/labels`, body: { labels: ["needs-human"] } }, + ]); +}); + +test("an armed pull request, an event run and report-only write no refusal note", async () => { + assert.deepEqual((await run([pull(1)])).notes, []); + const armedHeld = pull(2, { autoMergeRequest: {}, files: [{ filename: "docs/held.md" }] }); + const event = await run([armedHeld], { EVENT_NAME: "pull_request" }); + assert.deepEqual(event.writes, [{ mutation: "disarm", id: "PR_2" }]); + assert.deepEqual(event.notes, []); + const reported = await run([pull(3, { verdict: MISSING })], { REPORT_ONLY: "true" }); + assert.deepEqual(reported.notes, []); + assert.ok(reported.lines.includes("enable-auto-merge: #3 would note its refusal (report-only)")); +}); + +test("a disarmed pull request gets the note after the disarm", async () => { + const armed = pull(1, { autoMergeRequest: {}, verdict: MISSING }); + const { writes, notes } = await run([armed]); + assert.deepEqual(writes, [{ mutation: "disarm", id: "PR_1" }]); + assert.deepEqual(notes, [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: laneNote(MISSING_REASON) } }]); +}); + +test("a failed disarm writes no note, since the pull request is still armed", async () => { + const armed = pull(1, { autoMergeRequest: {}, files: [{ filename: "docs/held.md" }] }); + const { code, writes, notes, lines } = await run([armed], {}, { disarmError: new GitHubError(200, "POST /graphql (GraphQL errors)") }); + assert.equal(code, 1); + assert.deepEqual(writes, [{ mutation: "disarm", id: "PR_1" }]); + assert.deepEqual(notes, []); + assert.ok(lines.some((line) => line.startsWith("::error title=enable-auto-merge::#1 disarm"))); +}); + +test("a refusal comment that fails on the network is not sent again", async () => { + const { code, notes, slept } = await run([pull(1, { verdict: MISSING })], {}, { noteErrors: [closed()] }); + assert.equal(code, 1); + assert.deepEqual(notes, [{ method: "POST", path: `${ISSUES}/1/comments`, body: { body: laneNote(MISSING_REASON) } }]); + assert.deepEqual(slept, []); +}); + test("a lanes App pull request with no hold is armed, squash, at the head it read", async () => { const { code, writes } = await run([pull(7)]); assert.equal(code, 0); @@ -234,6 +410,92 @@ test("a failed arm fails the run and still reaches the next pull request", async assert.ok(lines.some((line) => line.startsWith("::error title=enable-auto-merge::#1 arm"))); }); +// undici's shape for a dropped socket: a "fetch failed" TypeError whose cause +// carries the code and the socket's message. +const reset = () => new TypeError("fetch failed", { cause: Object.assign(new Error("read ECONNRESET"), { code: "ECONNRESET" }) }); + +// The socket GitHub closed while the connection sat idle: undici's +// UND_ERR_SOCKET "other side closed". +const closed = () => new TypeError("fetch failed", { cause: Object.assign(new Error("other side closed"), { code: "UND_ERR_SOCKET" }) }); + +test("an arm that fails on the network is sent again after a capped wait and arms", async () => { + const { code, writes, lines, slept } = await run([pull(1)], {}, { armErrors: { PR_1: [reset(), closed()] } }); + assert.equal(code, 0); + assert.deepEqual(writes, Array(3).fill({ mutation: "arm", id: "PR_1", head: HEAD })); + // Two resends, each waiting no longer than the 8 s cap. + assert.equal(slept.length, 2); + assert.ok(slept.every((ms) => ms >= 0 && ms <= 8000)); + assert.ok(lines.includes(`enable-auto-merge: #1 armed at ${HEAD}`)); +}); + +test("the resend waits are full jitter under caps of 4 s, then 8 s", async () => { + const errors = () => ({ PR_1: [reset(), reset()] }); + assert.deepEqual((await run([pull(1)], {}, { armErrors: errors(), random: () => 1 })).slept, [4000, 8000]); + assert.deepEqual((await run([pull(1)], {}, { armErrors: errors(), random: () => 0 })).slept, [0, 0]); +}); + +test("a GET that fails on the network is left to the client's own retry, not sent again here", async () => { + const { calls, slept, writes } = await run([pull(1)], {}, { filesError: reset() }); + assert.equal(calls.filter(({ method }) => method === "GET").length, 1); + assert.deepEqual(slept, []); + assert.deepEqual(writes, []); +}); + +test("an arm that fails three times on the network fails the run, naming the cause", async () => { + const { code, writes, lines } = await run([pull(1)], {}, { armErrors: { PR_1: [reset(), reset(), reset(), reset()] } }); + assert.equal(code, 1); + assert.equal(writes.length, 3); + assert.ok(lines.includes("::error title=enable-auto-merge::#1 arm: fetch failed (cause: ECONNRESET read ECONNRESET)")); +}); + +test("one pull request whose arm still fails leaves it unarmed and the next is still armed", async () => { + const { code, writes, lines } = await run([pull(1), pull(2)], {}, { armErrors: { PR_1: [closed(), closed(), closed()] } }); + assert.equal(code, 1); + assert.deepEqual(writes.at(-1), { mutation: "arm", id: "PR_2", head: HEAD }); + assert.ok(lines.includes("::error title=enable-auto-merge::#1 arm: fetch failed (cause: UND_ERR_SOCKET other side closed)")); + assert.ok(lines.includes(`enable-auto-merge: #2 armed at ${HEAD}`)); + assert.ok(!lines.includes(`enable-auto-merge: #1 armed at ${HEAD}`)); +}); + +test("each network trigger alone is sent again, and an error with another code is not", async () => { + const coded = (code) => Object.assign(new Error("socket"), { cause: Object.assign(new Error("x"), { code }) }); + for (const error of [new TypeError("fetch failed"), coded("ETIMEDOUT"), coded("UND_ERR_CONNECT_TIMEOUT"), Object.assign(new Error("socket"), { cause: new Error("other side closed") })]) { + const { code, writes } = await run([pull(1)], {}, { armErrors: { PR_1: [error] } }); + assert.equal(code, 0); + assert.equal(writes.length, 2); + } + const { code, writes } = await run([pull(1)], {}, { armErrors: { PR_1: [coded("EPIPE_NOT_LISTED")] } }); + assert.equal(code, 1); + assert.equal(writes.length, 1); +}); + +test("the resend warning names the arm and its pull request", async () => { + const { lines } = await run([pull(1)], {}, { armErrors: { PR_1: [closed()] } }); + assert.ok(lines.some((line) => line.startsWith("::warning::enable-auto-merge: POST /graphql (arm) for PR_1 failed (TypeError UND_ERR_SOCKET)"))); +}); + +test("every request asks GitHub to close its connection after answering", async () => { + const { calls } = await run([pull(1)]); + assert.ok(calls.length > 1); + assert.ok(calls.every(({ headers }) => headers?.Connection === "close")); +}); + +test("an arm GitHub answers 4xx, or with GraphQL errors, is not sent again", async () => { + for (const error of [new GitHubError(422, "POST /graphql"), new GitHubError(200, "POST /graphql (GraphQL errors)")]) { + const { code, writes, slept } = await run([pull(1)], {}, { armErrors: { PR_1: [error] } }); + assert.equal(code, 1); + assert.deepEqual(writes, [{ mutation: "arm", id: "PR_1", head: HEAD }]); + assert.deepEqual(slept, []); + } +}); + +test("a logged cause has any token in it scrubbed", async () => { + const leak = new TypeError("fetch failed", { cause: Object.assign(new Error("bad header Bearer ghs_abc123 sent github_pat_11AB_cd"), { code: "UND_ERR_SOCKET" }) }); + const { lines } = await run([pull(1)], {}, { armErrors: { PR_1: [leak, leak, leak] } }); + assert.ok(lines.includes("::error title=enable-auto-merge::#1 arm: fetch failed (cause: UND_ERR_SOCKET bad header *** sent ***)")); + assert.ok(!lines.some((line) => line.includes("ghs_abc123") || line.includes("github_pat_"))); +}); + test("a bad bot login or repository fails red", async () => { assert.equal((await run([pull(1)], { LANES_BOT_LOGIN: "kyle-sexton" })).code, 1); assert.equal((await run([pull(1)], { GITHUB_REPOSITORY: "nope" })).code, 1); @@ -405,6 +667,7 @@ test("a manifest change arms only when the gate's risk score allows it", () => { assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: review }), { action: "none", reason: "pr-merge's gate holds it on its risk score (manifest-review-required: plugins/a/.claude-plugin/plugin.json: hooks)", + clears: "risk", }); assert.equal(decide(pull(1, { autoMergeRequest: {} }), options, { ...detail(), files, verify: review }).action, "disarm"); assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: PASSED }), { action: "arm", reason: "no hold" }); @@ -418,6 +681,7 @@ test("a cost path holds until the body links the owner's cost decision", async ( assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: PASSED }), { action: "none", reason: "it changes infra/Pulumi.prod.yaml, a cost path", + clears: "person", }); assert.equal(decide(pull(1, { autoMergeRequest: {} }), options, { ...detail(), files, verify: PASSED }).action, "disarm"); assert.equal(decide(pull(1), options, { ...detail(), files, costDecision: COST_LINK, verify: PASSED }).action, "arm");