From 12e86c028f8b6d2760fa3b3a34105ef4bf2a9d43 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:13:21 -0400 Subject: [PATCH 1/8] fix(pr-pipeline): resend an auto-merge request that fails on the network once enable-auto-merge's GraphQL calls (the listing, discussion reads, arm and disarm) are POSTs, which createGitHub's retryServerErrors never resends. A "fetch failed" TypeError or an ECONNRESET, ETIMEDOUT, UND_ERR_SOCKET or UND_ERR_CONNECT_TIMEOUT cause is now sent once more after 3 s; an HTTP error answer or a GraphQL error is not. A retried arm passes the same expectedHeadOid. Failure lines name the cause's code and message, tokens scrubbed. Co-Authored-By: Claude Opus 5.5 --- .../agent-ab0dff723afdb6f4c-77b6478d.md | 7 +++ .../scripts/pr-merge/enable-auto-merge.mjs | 45 +++++++++++++--- .../pr-merge/enable-auto-merge.test.mjs | 51 +++++++++++++++++-- 3 files changed, 94 insertions(+), 9 deletions(-) create mode 100644 .changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md diff --git a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md new file mode 100644 index 0000000000..18f83d90e3 --- /dev/null +++ b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- pr-merge's enable-auto-merge job no longer leaves a ready lanes App pull request unarmed until the next scheduled run when one GraphQL request (the open pull request listing, a discussion read, an arm or a disarm) fails on the network. A "fetch failed" error, or a cause code of `ECONNRESET`, `ETIMEDOUT`, `UND_ERR_SOCKET` or `UND_ERR_CONNECT_TIMEOUT`, is sent once more after 3 s; an HTTP error answer or a GraphQL error is not. A retried arm passes the same `expectedHeadOid`, so it arms only the head the run read. Each failure line now names the error's cause code and message, with any token scrubbed. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 1faa628db6..76c4c37110 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -64,7 +64,8 @@ // review hold. Read only after the verdict allows. // Arming passes the head just read as expectedHeadOid, so a push in between // fails the arm rather than arming a head nobody read. An armed pull request -// that fails any rule is disarmed. While merge.report-only is true it reports +// that fails any rule is disarmed. A read or write that fails on the network +// is sent once more (retryOnce). While merge.report-only is true it reports // each decision and writes nothing. // // Environment: @@ -152,6 +153,38 @@ export const DISARM_MUTATION = `mutation($id: ID!) { const escapeCommand = (text) => String(text).replaceAll(/[\r\n%]/g, " "); +// A network failure undici raises before any answer: its "fetch failed" +// TypeError, or one of these cause codes. An HTTP answer (a 4xx included) and +// a GraphQL error are GitHubErrors, 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); +export const RETRY_DELAY_MS = 3000; +// 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+/g, "***")); +} +// `github` with a POST that fails on the network sent once more after +// RETRY_DELAY_MS. A GET is left to createGitHub's own retry +// (retryServerErrors), which resends only GETs. A second arm is pinned to the +// same expectedHeadOid, so it can arm only the head the run read. +export function retryOnce(github, sleep) { + return async (method, ...rest) => { + try { + return await github(method, ...rest); + } catch (error) { + if (method === "GET" || !transient(error)) { + throw error; + } + console.log(`::warning::enable-auto-merge: ${method} ${rest[0]} failed (${describe(error)}); sending it again in ${RETRY_DELAY_MS} ms`); + await sleep(RETRY_DELAY_MS); + return github(method, ...rest); + } + }; +} + // Why `pull` must not be armed, or null when it may be. export function holdReason(pull, humanGated) { const labels = pull.labels?.nodes ?? []; @@ -375,7 +408,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)) } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -392,7 +425,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 = retryOnce(github ?? createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }), sleep); verify ??= pythonVerify(env); const reads = READER_EVENTS.includes(env.EVENT_NAME); const pulls = await listPulls(github, repo[1], repo[2]); @@ -453,7 +486,7 @@ 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)); } @@ -501,12 +534,12 @@ 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)}`); } } 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 bac7dbf3a8..852ab14fee 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -29,6 +29,7 @@ import { 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,7 +92,8 @@ 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 } = {}) { +function world(pulls, { failArm = false, armErrors = [] } = {}) { + const armThrows = [...armErrors]; const calls = []; const github = async (method, path, body) => { calls.push({ method, path, body }); @@ -122,6 +124,9 @@ function world(pulls, { failArm = false } = {}) { const found = pulls.find((candidate) => candidate.number === body.variables.number); return { data: { repository: { pullRequest: detail(found?.discussion) } } }; } + if (body.query === ARM_MUTATION && armThrows.length > 0) { + throw armThrows.shift(); + } if (body.query === ARM_MUTATION && failArm) { throw new Error("POST /graphql (GraphQL errors) failed with status 200"); } @@ -148,11 +153,12 @@ function world(pulls, { failArm = false } = {}) { async function run(pulls, env = {}, options = {}, lane = ROOM) { const { github, writes, verify, verified } = 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) }); + return { code, writes: writes(), lines, verified, slept }; } finally { console.log = log; } @@ -234,6 +240,45 @@ 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" }) }); + +test("an arm that fails once on the network is sent again after a short wait and arms", async () => { + const { code, writes, lines, slept } = await run([pull(1)], {}, { armErrors: [reset()] }); + assert.equal(code, 0); + assert.deepEqual(writes, [ + { mutation: "arm", id: "PR_1", head: HEAD }, + { mutation: "arm", id: "PR_1", head: HEAD }, + ]); + assert.equal(slept.length, 1); + assert.ok(slept[0] >= 2000 && slept[0] <= 5000); + assert.ok(lines.includes(`enable-auto-merge: #1 armed at ${HEAD}`)); +}); + +test("an arm that fails twice on the network fails the run, naming the cause", async () => { + const { code, writes, lines } = await run([pull(1)], {}, { armErrors: [reset(), reset()] }); + assert.equal(code, 1); + assert.equal(writes.length, 2); + assert.ok(lines.includes("::error title=enable-auto-merge::#1 arm: fetch failed (cause: ECONNRESET read ECONNRESET)")); +}); + +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: [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"), { code: "UND_ERR_SOCKET" }) }); + const { lines } = await run([pull(1)], {}, { armErrors: [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"))); +}); + 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); From 88dc3d1b4b8e3de10d7de7072bf5da85846dc4e9 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:21:26 -0400 Subject: [PATCH 2/8] fix(pr-pipeline): arm on a fresh connection and resend a failed request up to twice The arm failures came after the verifier's subprocess: the mutation went out on undici's pooled keep-alive socket, which had sat idle past GitHub's keep-alive window. Every request now sends Connection: close, so each one opens its own connection. A POST that still fails on the network is sent at most three times in all, with capped full-jitter waits (4 s, then 8 s). The retry warning names the arm or disarm and its pull request. The remaining catch sites log the cause, and the scrub also covers github_pat_ tokens. Co-Authored-By: Claude Opus 5.5 --- .../agent-ab0dff723afdb6f4c-77b6478d.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 73 +++++++++++------- .../pr-merge/enable-auto-merge.test.mjs | 77 ++++++++++++++----- 3 files changed, 104 insertions(+), 48 deletions(-) diff --git a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md index 18f83d90e3..0771933632 100644 --- a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md +++ b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- pr-merge's enable-auto-merge job no longer leaves a ready lanes App pull request unarmed until the next scheduled run when one GraphQL request (the open pull request listing, a discussion read, an arm or a disarm) fails on the network. A "fetch failed" error, or a cause code of `ECONNRESET`, `ETIMEDOUT`, `UND_ERR_SOCKET` or `UND_ERR_CONNECT_TIMEOUT`, is sent once more after 3 s; an HTTP error answer or a GraphQL error is not. A retried arm passes the same `expectedHeadOid`, so it arms only the head the run read. Each failure line now names the error's cause code and message, with any token scrubbed. +- 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, 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. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 76c4c37110..a6a4a945d5 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -64,8 +64,11 @@ // review hold. Read only after the verdict allows. // Arming passes the head just read as expectedHeadOid, so a push in between // fails the arm rather than arming a head nobody read. An armed pull request -// that fails any rule is disarmed. A read or write that fails on the network -// is sent once more (retryOnce). 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: @@ -153,34 +156,52 @@ export const DISARM_MUTATION = `mutation($id: ID!) { const escapeCommand = (text) => String(text).replaceAll(/[\r\n%]/g, " "); -// A network failure undici raises before any answer: its "fetch failed" -// TypeError, or one of these cause codes. An HTTP answer (a 4xx included) and -// a GraphQL error are GitHubErrors, never this. +// 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); -export const RETRY_DELAY_MS = 3000; +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+/g, "***")); + return escapeCommand(text.replaceAll(/Bearer\s+\S+|gh[pousr]_\w+|github_pat_\w+/g, "***")); } -// `github` with a POST that fails on the network sent once more after -// RETRY_DELAY_MS. A GET is left to createGitHub's own retry -// (retryServerErrors), which resends only GETs. A second arm is pinned to the -// same expectedHeadOid, so it can arm only the head the run read. -export function retryOnce(github, sleep) { - return async (method, ...rest) => { - try { - return await github(method, ...rest); - } catch (error) { - if (method === "GET" || !transient(error)) { - throw error; +// `github` with every request on a connection of its own, and a 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 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 (method === "GET" || 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 (${describe(error)}); sending it again in ${wait} ms (retry ${attempt + 1} of ${RETRY_CAPS_MS.length})`); + await sleep(wait); } - console.log(`::warning::enable-auto-merge: ${method} ${rest[0]} failed (${describe(error)}); sending it again in ${RETRY_DELAY_MS} ms`); - await sleep(RETRY_DELAY_MS); - return github(method, ...rest); } }; } @@ -408,7 +429,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, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)) } = {}) { +export async function main({ env = process.env, github, verify, room, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -425,7 +446,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m const write = env.REPORT_ONLY === "false"; const humanGated = humanGatedLabelFrom(env.TRACKER_CONFIG || ".base/.work-item-tracker.json"); const holdPaths = readHoldPaths(env.RESOLVED_CONFIG ?? ""); - github = retryOnce(github ?? createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }), sleep); + 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]); @@ -437,7 +458,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m 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))}`); } @@ -502,7 +523,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m 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 })); } diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 852ab14fee..9e3150c8f7 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -92,11 +92,12 @@ 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 = [] } = {}) { - const armThrows = [...armErrors]; +// armErrors: per pull request id, the errors its arm sends throw, in order. +function world(pulls, { failArm = false, armErrors = {} } = {}) { + 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 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) { const files = pulls.find((candidate) => candidate.number === Number(listing[1]))?.files ?? [{ filename: "plugins/work-items/README.md" }]; @@ -124,8 +125,8 @@ function world(pulls, { failArm = false, armErrors = [] } = {}) { const found = pulls.find((candidate) => candidate.number === body.variables.number); return { data: { repository: { pullRequest: detail(found?.discussion) } } }; } - if (body.query === ARM_MUTATION && armThrows.length > 0) { - throw armThrows.shift(); + if (body.query === ARM_MUTATION && armThrows[body.variables.id]?.length > 0) { + throw armThrows[body.variables.id].shift(); } if (body.query === ARM_MUTATION && failArm) { throw new Error("POST /graphql (GraphQL errors) failed with status 200"); @@ -151,14 +152,14 @@ function world(pulls, { failArm = false, armErrors = [] } = {}) { } async function run(pulls, env = {}, options = {}, lane = ROOM) { - const { github, writes, verify, verified } = world(pulls, options); + const { github, writes, verify, verified, calls } = 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), sleep: async (ms) => slept.push(ms) }); - return { code, writes: writes(), lines, verified, slept }; + return { code, writes: writes(), lines, verified, slept, calls }; } finally { console.log = log; } @@ -244,28 +245,62 @@ test("a failed arm fails the run and still reaches the next pull request", async // carries the code and the socket's message. const reset = () => new TypeError("fetch failed", { cause: Object.assign(new Error("read ECONNRESET"), { code: "ECONNRESET" }) }); -test("an arm that fails once on the network is sent again after a short wait and arms", async () => { - const { code, writes, lines, slept } = await run([pull(1)], {}, { armErrors: [reset()] }); +// 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, [ - { mutation: "arm", id: "PR_1", head: HEAD }, - { mutation: "arm", id: "PR_1", head: HEAD }, - ]); - assert.equal(slept.length, 1); - assert.ok(slept[0] >= 2000 && slept[0] <= 5000); + 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("an arm that fails twice on the network fails the run, naming the cause", async () => { - const { code, writes, lines } = await run([pull(1)], {}, { armErrors: [reset(), reset()] }); +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, 2); + 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 (fetch failed (cause: UND_ERR_SOCKET other side closed))"))); +}); + +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: [error] }); + 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, []); @@ -274,7 +309,7 @@ test("an arm GitHub answers 4xx, or with GraphQL errors, is not sent again", asy 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"), { code: "UND_ERR_SOCKET" }) }); - const { lines } = await run([pull(1)], {}, { armErrors: [leak, leak] }); + 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"))); }); From df1920fb3dbbccb626ca166eab1c64a2553c1782 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:22:02 -0400 Subject: [PATCH 3/8] fix(pr-pipeline): name only the error and cause code on the resend warning The ::error line keeps the scrubbed cause message for diagnosis; the warning follows the shared client's errorLabel convention, since undici can put a header value in a message. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/enable-auto-merge.mjs | 4 ++-- .github/scripts/pr-merge/enable-auto-merge.test.mjs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index a6a4a945d5..fa2a1a71c9 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -95,7 +95,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"; @@ -199,7 +199,7 @@ export function withRetry(github, { sleep, random = Math.random }) { 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 (${describe(error)}); sending it again in ${wait} ms (retry ${attempt + 1} of ${RETRY_CAPS_MS.length})`); + 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); } } diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 9e3150c8f7..2543d8f8be 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -289,7 +289,7 @@ test("each network trigger alone is sent again, and an error with another code i 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 (fetch failed (cause: UND_ERR_SOCKET other side 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 () => { From aab020ab5823017809b8d2500b6e894ebf46dc67 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:31:45 -0400 Subject: [PATCH 4/8] test(pr-pipeline): pin the retry caps, the GET pass-through and header forwarding Review findings: the jitter caps, the GET guard and createGitHub's forwarding of the Connection header had no test that fails when broken, and the github_pat_ scrub had none. Co-Authored-By: Claude Opus 5.5 --- .../check-trusted-trigger.test.mjs | 11 ++++++++ .../pr-merge/enable-auto-merge.test.mjs | 27 +++++++++++++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs b/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs index 837df376cc..ab93985694 100644 --- a/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs +++ b/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs @@ -1843,6 +1843,17 @@ test("the CLI exits 0 with proceed=false when its inputs are missing", () => { // GitHub client +test("createGitHub forwards a caller's headers to fetch", async () => { + let headers; + const fetchImpl = async (url, init) => { + headers = init.headers; + return new Response("{}", { status: 200 }); + }; + await createGitHub({ token: "t0ken", fetchImpl })("POST", "/graphql", {}, { Connection: "close" }); + assert.equal(headers.Connection, "close"); + assert.equal(headers.Authorization, "Bearer t0ken"); +}); + test("createGitHub sends the token and raises GitHubError with the status on failure", async () => { const seen = []; const fetchImpl = async (url, init) => { diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 2543d8f8be..49b7b73471 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -93,12 +93,16 @@ const room = (answer = ROOM) => async () => answer; const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name) => ({ name })) }); // armErrors: per pull request id, the errors its arm sends throw, in order. -function world(pulls, { failArm = false, armErrors = {} } = {}) { +// filesError: what every changed-files GET throws. +function world(pulls, { failArm = false, armErrors = {}, filesError = null } = {}) { const armThrows = Object.fromEntries(Object.entries(armErrors).map(([id, errors]) => [id, [...errors]])); const calls = []; 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); @@ -158,7 +162,7 @@ async function run(pulls, env = {}, options = {}, lane = ROOM) { const log = console.log; console.log = (line) => lines.push(line); try { - const code = await main({ env: { ...ENV, ...env }, github, verify, room: room(lane), sleep: async (ms) => slept.push(ms) }); + const code = await main({ env: { ...ENV, ...env }, github, verify, room: room(lane), sleep: async (ms) => slept.push(ms), random: options.random }); return { code, writes: writes(), lines, verified, slept, calls }; } finally { console.log = log; @@ -259,6 +263,19 @@ test("an arm that fails on the network is sent again after a capped wait and arm 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); @@ -308,10 +325,10 @@ test("an arm GitHub answers 4xx, or with GraphQL errors, is not sent again", asy }); 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"), { code: "UND_ERR_SOCKET" }) }); + 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"))); + 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 () => { From 36c0e32d147b3ffc27f3ddc40558302631cc47ec Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:39:19 -0400 Subject: [PATCH 5/8] test(pr-pipeline): keep the arm fix out of the trust-gate test tree Co-Authored-By: Claude Opus 5.5 --- .../check-trusted-trigger.test.mjs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs b/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs index ab93985694..837df376cc 100644 --- a/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs +++ b/.github/actions/check-trusted-trigger/check-trusted-trigger.test.mjs @@ -1843,17 +1843,6 @@ test("the CLI exits 0 with proceed=false when its inputs are missing", () => { // GitHub client -test("createGitHub forwards a caller's headers to fetch", async () => { - let headers; - const fetchImpl = async (url, init) => { - headers = init.headers; - return new Response("{}", { status: 200 }); - }; - await createGitHub({ token: "t0ken", fetchImpl })("POST", "/graphql", {}, { Connection: "close" }); - assert.equal(headers.Connection, "close"); - assert.equal(headers.Authorization, "Bearer t0ken"); -}); - test("createGitHub sends the token and raises GitHubError with the status on failure", async () => { const seen = []; const fetchImpl = async (url, init) => { From d21f6e5a52afa64bb59ff9464a910977702d578b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:06:47 -0400 Subject: [PATCH 6/8] fix(pr-pipeline): say on the pull request why native auto-merge leaves it unarmed Under merge.lane-prs native the gate never lists a lanes App pull request, so a refusal enable-auto-merge read left only a log line. A reader run now writes one babysit-merge-risk comment naming the head and reason, edited in place and left alone while unchanged, and adds the human-gated label for a refusal a person must clear: a hold or cost path, a link the lane does not take, a needsHuman verdict, or a risk hold that stood merge.risk-escalation-minutes, as the gate escalates it. Co-Authored-By: Claude Opus 5.5 --- .../agent-ab0dff723afdb6f4c-77b6478d.md | 1 + .../scripts/pr-merge/enable-auto-merge.mjs | 147 +++++++++++++++++- .../pr-merge/enable-auto-merge.test.mjs | 125 ++++++++++++++- 3 files changed, 262 insertions(+), 11 deletions(-) diff --git a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md index 0771933632..e223f65a6a 100644 --- a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md +++ b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md @@ -5,3 +5,4 @@ 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, 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 fa2a1a71c9..ada3b67df6 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -62,6 +62,17 @@ // 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, once; 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. Each request opens its own connection, and @@ -135,7 +146,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 @@ -293,6 +304,88 @@ 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; + if (prior?.body === body) { + return null; + } + return { body, commentId: prior?.databaseId ?? null, label: body === person ? label : null }; +} + // 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) { @@ -394,7 +487,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 }; } @@ -406,7 +503,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" }; } @@ -429,7 +527,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, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random } = {}) { +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) { @@ -486,8 +584,40 @@ export async function main({ env = process.env, github, verify, room, sleep = (m 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 { + 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; @@ -509,7 +639,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m } catch (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 } = @@ -525,7 +655,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m } catch (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; @@ -539,10 +669,12 @@ export async function main({ env = process.env, github, verify, room, sleep = (m 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 { @@ -557,6 +689,9 @@ export async function main({ env = process.env, github, verify, room, sleep = (m failed += 1; console.log(`::error title=enable-auto-merge::#${pull.number} ${action}: ${describe(error)}`); } + if (action === "disarm") { + await note(pull, detail, { clears, reason }); + } } return failed === 0 ? 0 : 1; } catch (error) { diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 49b7b73471..1efee29ff5 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -97,6 +97,7 @@ const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name function world(pulls, { failArm = false, armErrors = {}, filesError = null } = {}) { const armThrows = Object.fromEntries(Object.entries(armErrors).map(([id, errors]) => [id, [...errors]])); const calls = []; + 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); @@ -113,6 +114,12 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null } = { 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 }); + return {}; + } assert.equal(`${method} ${path}`, "POST /graphql"); if (body.query === PULL_OPENED_QUERY) { const nodes = [{ number: body.variables.number + 1000, repository: { nameWithOwner: REPO } }]; @@ -150,25 +157,131 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null } = { }; 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].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, calls } = 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), sleep: async (ms) => slept.push(ms), random: options.random }); - return { code, writes: writes(), lines, verified, slept, calls }; + 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 .github/workflows/x.yml, 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: ".github/workflows/x.yml" }] }); + 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) } }]); +}); + +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: ".github/workflows/x.yml" }], discussion: { comments: [lanesSaid(personNote(HOLD_PATH_REASON))] } }); + const { code, notes } = await run([lane, person]); + assert.equal(code, 0); + assert.deepEqual(notes, []); +}); + +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: ".github/workflows/x.yml" }] }); + 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 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); @@ -493,6 +606,7 @@ test("a pull request whose head changes the marketplace manifest is not armed, o assert.deepEqual(decide(pull(1), options, { ...detail(), files, verify: PASSED }), { action: "none", reason: "it changes .claude-plugin/marketplace.json, a hold path", + clears: "person", }); assert.equal(decide(pull(1, { autoMergeRequest: {} }), options, { ...detail(), files, verify: PASSED }).action, "disarm"); }); @@ -505,6 +619,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"); From b9f5483b14a575bd85433084f69b7734bc4a6b86 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:13:04 -0400 Subject: [PATCH 7/8] fix(pr-pipeline): put the human-gated label back beside a standing refusal comment A label write that failed after its person comment was never retried: every later run saw the comment unchanged and wrote nothing. When the lanes App's person comment stands and the label is off, the run now reads the pull request's label removals and adds the label back unless a person removed it at or after the comment's last edit. A timeline not read whole adds nothing. Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-merge/enable-auto-merge.mjs | 44 ++++++++++++++++++- .../pr-merge/enable-auto-merge.test.mjs | 33 +++++++++++++- 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index ada3b67df6..24d853c4b1 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -70,7 +70,8 @@ // 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, once; clear-escalations never removes it under native, +// 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 @@ -380,12 +381,40 @@ export function refusalWrites(pull, comments, { clears, reason }, { bot, 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 null; + 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) { @@ -603,6 +632,17 @@ export async function main({ env = process.env, github, verify, room, sleep = (m } 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 })); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 1b8f3d6601..b203490a6b 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -22,7 +22,9 @@ import { DISARM_MUTATION, HUMAN_VETO, PULLS_QUERY, + UNLABELED_QUERY, decide, + liftedByPerson, READER_EVENTS, main, pythonRoom, @@ -132,6 +134,10 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null } = { 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) } } }; @@ -157,7 +163,7 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null } = { }; const writes = () => calls - .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![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, notes }; } @@ -237,14 +243,37 @@ test("a refusal a lane clears is commented on, with no label", async () => { 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))] } }); + 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]); From cd7b9dea61ec6ead98884cae0eac24173c582f2c Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 23:44:28 -0400 Subject: [PATCH 8/8] fix(pr-pipeline): resend only GraphQL requests, and note a disarm only once it succeeds The refusal note's comment POST is not idempotent, so a lost answer could post a second marker comment; only /graphql requests are resent now. A disarm that threw left the pull request armed, yet the note followed and said it was unarmed; the note now runs only after a disarm that succeeded. Co-Authored-By: Claude Opus 5.5 --- .../agent-ab0dff723afdb6f4c-77b6478d.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 11 +++++--- .../pr-merge/enable-auto-merge.test.mjs | 25 ++++++++++++++++++- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md index e223f65a6a..991288bc1d 100644 --- a/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md +++ b/.changes/pr-pipeline/agent-ab0dff723afdb6f4c-77b6478d.md @@ -4,5 +4,5 @@ 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, 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. +- 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 24d853c4b1..dba8c6c4dd 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -188,12 +188,13 @@ export function describe(error) { 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 POST that -// fails on the network sent again (RETRY_CAPS_MS). `Connection: close` closes +// `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 resent arm is pinned to the same expectedHeadOid, so it can arm +// 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 @@ -205,7 +206,7 @@ export function withRetry(github, { sleep, random = Math.random }) { return await github(method, path, body, { Connection: "close", ...headers }, options); } catch (error) { const cap = RETRY_CAPS_MS[attempt]; - if (method === "GET" || cap === undefined || !transient(error)) { + if (path !== "/graphql" || cap === undefined || !transient(error)) { throw error; } const wait = Math.round(random() * cap); @@ -728,6 +729,8 @@ export async function main({ env = process.env, github, verify, room, sleep = (m } catch (error) { failed += 1; 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 }); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index b203490a6b..527e9725fd 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -96,7 +96,8 @@ const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name // 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 } = {}) { +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 notes = []; @@ -120,6 +121,9 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null } = { 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"); @@ -145,6 +149,9 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null } = { 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"); } @@ -311,6 +318,22 @@ test("a disarmed pull request gets the note after the disarm", async () => { 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);