From d393ba75eb083cec12d10dd9513f2ba67b4dcee5 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:20:53 -0500 Subject: [PATCH 1/3] feat(pr-merge): dequeue a pull request when a person adds a hold label pr-merge.yml wakes on `labeled` too. Only a hold label a person added runs work, and only the enable-auto-merge job's: the label set is enable-auto-merge.mjs HOLD_LABELS plus the tracker's human-gated label, checked by workflow.test.mjs. Any other label, and any bot-sent event, skips every job. The labeled run reads that one pull request live. When a hold label still stands, it takes the PR out of the merge queue with the address-feedback dequeue mutation, now exported, and disarms a lanes App PR that is still armed. It prints one pr-merge-hold-dequeue notice naming the outcome. A labeled run is never superseded, and it runs in a concurrency group of its own, so it does not wait behind a scheduled run's verdict reads. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VcupomBPg7kuobwLoNpxEW --- .../scripts/pr-merge/enable-auto-merge.mjs | 99 ++++++++++++++++++- .../pr-merge/enable-auto-merge.test.mjs | 90 +++++++++++++++++ .github/scripts/pr-merge/workflow.test.mjs | 69 ++++++++++--- .../scripts/pr-unblock/address-feedback.mjs | 4 +- .github/workflows/pr-merge.yml | 48 ++++++--- 5 files changed, 278 insertions(+), 32 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index a88ca26c682..c854c0b6f5b 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -2,8 +2,8 @@ // enable-auto-merge.mjs: turns on GitHub's native auto-merge for the lanes // App's open pull requests into main, and turns it off on one that now carries // a hold. No model. pr-merge.yml's enable-auto-merge job runs it when a listed -// human edits, unlabels or readies a pull request, and on the schedule, which -// arms. +// human unlabels a pull request, or adds a hold label to one (which also takes +// it out of the merge queue), and on the schedule, which arms. // // A pull request is armed only when all of these hold: // - The lanes App opened it (merge.agent-identity), into main, from a branch @@ -169,6 +169,9 @@ // decision and the approvers who lift a hold path (default .base/.github/standards/trusted-actors/trusted-actors.json) // EVENT_NAME the run's event; only READER_EVENTS read verdicts and arm // EVENT_PR a pull_request_target run's pull request number, the only one it handles +// EVENT_ACTION, EVENT_LABEL that run's action and, on `labeled`, the label +// added: a labeled run only dequeues or disarms on a hold +// (holdDequeue) // EAM_SKIP_IN, EAM_SKIP_OUT the skip records the last reader run kept, and // the file this one keeps (SKIPPABLE); no OUT, no skip // SKIP_MAX_MINUTES, SKIP_MAX_SKIPS merge.skip-unchanged's limits; 0 in either: no skip @@ -208,7 +211,7 @@ import { pathToFileURL } from "node:url"; import { createGitHub, errorLabel, GitHubError, paginate } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { rateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; -import { humanGatedLabelFrom, quote } from "../pr-unblock/address-feedback.mjs"; +import { DEQUEUE_MUTATION, humanGatedLabelFrom, quote } from "../pr-unblock/address-feedback.mjs"; import { lines as bodyLines, foreignMarker, @@ -415,14 +418,93 @@ async function answerBody(text) { // LANE_HOLD), a cause a person must clear (owner decision Q-HOLDLINE). export const LANE_HOLD_REASON = "its body carries the lane's own 'Hold: a human merges this' line: a person merges it by hand, or removes that line"; +// The first hold label (HOLD_LABELS, then the human-gated label) among +// `labels`, in any case; undefined when none is. +export function holdLabel(labels, humanGated) { + const names = new Set(labels.map((label) => String(label?.name).toLowerCase())); + return [...HOLD_LABELS, humanGated].find((name) => names.has(name.toLowerCase())); +} + +// A `labeled` run's one pull request, read live: the event names the label, +// never whether it still holds. +export const HOLD_PULL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + id number state baseRefName + author { __typename login } + labels(first: 100) { totalCount nodes { name } } + autoMergeRequest { enabledAt } + mergeQueueEntry { id } + } + } +}`; +export const HOLD_DEQUEUE_TITLE = "pr-merge-hold-dequeue"; +export const holdDequeueLine = (number, label, outcome) => + `::notice title=${HOLD_DEQUEUE_TITLE}::pr=${escapeCommand(number)} label=${escapeCommand(label)} outcome=${escapeCommand(outcome)}`; + +// A person added a label (EVENT_ACTION labeled): when a hold label stands on +// the pull request now, it leaves the merge queue, so its merge group does not +// run only for ci-status to fail it, and a lanes App pull request still armed +// is disarmed. Any author: the queue also holds the pull requests the gate +// enqueued. Two calls at most past the read, and one line naming the outcome. +// Returns the exit code. +async function holdDequeue(github, { repo, env, humanGated, write, slug, disarm }) { + const number = Number(env.EVENT_PR); + const said = (outcome) => console.log(holdDequeueLine(env.EVENT_PR ?? "", env.EVENT_LABEL ?? "", outcome)); + let pull; + try { + pull = (await github("POST", "/graphql", { query: HOLD_PULL_QUERY, variables: { owner: repo[1], name: repo[2], number } }))?.data?.repository?.pullRequest; + if (pull != null && !whole(pull.labels)) { + pull.labels = await allLabels(github, repo[0], number); + } + } catch (error) { + said("refused:read-failed"); + console.log(`::error title=enable-auto-merge::#${number} hold read: ${describe(error)}`); + return 1; + } + if (pull?.number !== number || pull.state !== "OPEN" || pull.baseRefName !== "main") { + said("refused:not-open-into-main"); + return 0; + } + if (holdLabel(pull.labels?.nodes ?? [], humanGated) === undefined) { + said("refused:no-hold-label"); + return 0; + } + const queued = pull.mergeQueueEntry != null; + const armed = pull.autoMergeRequest != null && pull.author?.__typename === "Bot" && pull.author.login === slug; + if (!queued && !armed) { + said("not-queued"); + return 0; + } + if (!write) { + said("refused:report-only"); + return 0; + } + let step = "dequeue"; + try { + if (queued) { + await github("POST", "/graphql", { query: DEQUEUE_MUTATION, variables: { id: pull.id } }); + } + step = "disarm"; + if (armed) { + await disarm(pull); + } + } catch (error) { + said(`refused:${step}-failed`); + console.log(`::error title=enable-auto-merge::#${number} ${step}: ${describe(error)}`); + return 1; + } + said(queued ? "dequeued" : "disarmed"); + return 0; +} + // Why `pull` must not be armed, or null when it may be. export function holdReason(pull, humanGated) { const labels = pull.labels?.nodes ?? []; if (!(pull.labels?.totalCount <= labels.length)) { return "its labels could not all be read"; } - const names = new Set(labels.map((label) => String(label?.name).toLowerCase())); - const label = [...HOLD_LABELS, humanGated].find((name) => names.has(name.toLowerCase())); + const label = holdLabel(labels, humanGated); if (label !== undefined) { return `it carries ${label}`; } @@ -1645,7 +1727,11 @@ export async function main({ env = process.env, github, actions, verify, room, c if (!repo) { throw new Error("GITHUB_REPOSITORY is not owner/repo"); } + const labeled = env.EVENT_NAME === "pull_request_target" && env.EVENT_ACTION === "labeled"; if (env.KILL_SWITCH !== "proceed") { + if (labeled) { + console.log(holdDequeueLine(env.EVENT_PR ?? "", env.EVENT_LABEL ?? "", "refused:kill-switch")); + } console.log("enable-auto-merge: the lanes kill switch stops this run; nothing read or armed"); return 0; } @@ -1717,6 +1803,9 @@ export async function main({ env = process.env, github, actions, verify, room, c console.log(`enable-auto-merge: #${pull.number} ${action} failed after a resend (${describe(error)}), but it is already ${action === "arm" ? `armed at ${pull.headRefOid}` : "unarmed"}, so an earlier send landed`); } }; + if (labeled) { + return await holdDequeue(github, { repo, env, humanGated, write, slug, disarm: (pull) => sendSettled("disarm", pull) }); + } verify ??= pythonVerify(env); whole ??= pythonWholeReview(env); const reads = READER_EVENTS.includes(env.EVENT_NAME); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index b83ac53c20d..bd6ecaa7aec 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -60,12 +60,14 @@ import { readRoom, TRUST_REFUSALS, FILES_CAPPED_REASON, + HOLD_PULL_QUERY, UNARMED_OUTPUT, verifyClears, verifyHold, SKIP_STATE_PREFIX, } from "./enable-auto-merge.mjs"; import { GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; +import { DEQUEUE_MUTATION } from "../pr-unblock/address-feedback.mjs"; import { createRateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; import { COMMENT_EDITED_QUERY, LANE_HOLD, PULL_OPENED_QUERY, VERIFY_PATH, personWrote, unblockStops, verdictHold } from "./merge-holds.mjs"; import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; @@ -3983,3 +3985,91 @@ test("a skip record of another repository, from the future, or malformed skips n assert.equal((await skipRun([riskHeld()], { from: path, minutes: 15 })).details, 1, name); } }); + +// A person adds a label after the pull request entered the merge queue: a +// hold label standing on it now takes it out of the queue, or disarms a lanes +// App pull request still armed, and the run says which in one notice. +// Expected outcomes are the work item's: queued dequeues, armed disarms, no +// hold left writes nothing. +async function labeledRun(live, { env = {}, fail = null } = {}) { + const sent = []; + const github = async (method, path, body) => { + sent.push(body?.query === HOLD_PULL_QUERY ? "read" : body?.query === DEQUEUE_MUTATION ? `dequeue ${body.variables.id}` : body?.query === DISARM_MUTATION ? `disarm ${body.variables.id}` : `${method} ${path}`); + if (fail !== null && sent.at(-1).startsWith(fail)) { + throw new GitHubError(200, "POST /graphql (GraphQL errors)"); + } + if (body?.query === HOLD_PULL_QUERY) { + return { data: { repository: { pullRequest: live } } }; + } + return { data: {} }; + }; + const lines = []; + const log = console.log; + console.log = (line) => lines.push(line); + try { + const code = await main({ env: { ...ENV, EVENT_NAME: "pull_request_target", EVENT_ACTION: "labeled", EVENT_PR: "7", EVENT_LABEL: "do-not-merge", ...env }, github }); + return { code, sent, notice: lines.filter((line) => line.startsWith("::notice title=pr-merge-hold-dequeue::")) }; + } finally { + console.log = log; + } +} +const live = (overrides = {}) => ({ + id: "PR_7", + number: 7, + state: "OPEN", + baseRefName: "main", + author: { __typename: "User", login: "someone" }, + labels: { totalCount: 1, nodes: [{ name: "do-not-merge" }] }, + autoMergeRequest: null, + mergeQueueEntry: null, + ...overrides, +}); +const noticeOf = (outcome, label = "do-not-merge") => [`::notice title=pr-merge-hold-dequeue::pr=7 label=${label} outcome=${outcome}`]; + +test("a hold label added to a queued pull request dequeues it, whoever opened it", async () => { + const person = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } })); + assert.equal(person.code, 0); + assert.deepEqual(person.sent, ["read", "dequeue PR_7"]); + assert.deepEqual(person.notice, noticeOf("dequeued")); + // A lanes App pull request still armed in the queue is disarmed too. + const lanes = await labeledRun(live({ author: LANES, autoMergeRequest: { enabledAt: NOW }, mergeQueueEntry: { id: "MQE_1" } })); + assert.deepEqual(lanes.sent, ["read", "dequeue PR_7", "disarm PR_7"]); + assert.deepEqual(lanes.notice, noticeOf("dequeued")); +}); + +test("a hold label added to an armed lanes App pull request not yet queued disarms it", async () => { + const { code, sent, notice } = await labeledRun(live({ author: LANES, autoMergeRequest: { enabledAt: NOW }, labels: { totalCount: 1, nodes: [{ name: "needs-human" }] } }), { env: { EVENT_LABEL: "needs-human" } }); + assert.equal(code, 0); + assert.deepEqual(sent, ["read", "disarm PR_7"]); + assert.deepEqual(notice, noticeOf("disarmed", "needs-human")); +}); + +test("a labeled run writes nothing when no hold stands, the pull request is not queued or armed, or the lane only reports", async () => { + // The label came off again before the run read it. + const gone = await labeledRun(live({ labels: { totalCount: 1, nodes: [{ name: "documentation" }] }, mergeQueueEntry: { id: "MQE_1" } })); + assert.deepEqual([gone.sent, gone.notice], [["read"], noticeOf("refused:no-hold-label")]); + const idle = await labeledRun(live()); + assert.deepEqual([idle.sent, idle.notice], [["read"], noticeOf("not-queued")]); + // A person's own auto-merge is theirs: only the lanes App's is disarmed. + const theirs = await labeledRun(live({ autoMergeRequest: { enabledAt: NOW } })); + assert.deepEqual([theirs.sent, theirs.notice], [["read"], noticeOf("not-queued")]); + const closed = await labeledRun(live({ state: "MERGED", mergeQueueEntry: { id: "MQE_1" } })); + assert.deepEqual([closed.sent, closed.notice], [["read"], noticeOf("refused:not-open-into-main")]); + const reported = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } }), { env: { REPORT_ONLY: "true" } }); + assert.deepEqual([reported.sent, reported.notice], [["read"], noticeOf("refused:report-only")]); + const off = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } }), { env: { KILL_SWITCH: "" } }); + assert.deepEqual([off.sent, off.notice], [[], noticeOf("refused:kill-switch")]); +}); + +test("a refused dequeue fails the run and names it", async () => { + const { code, sent, notice } = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } }), { fail: "dequeue" }); + assert.equal(code, 1); + assert.equal(sent[0], "read"); + assert.ok(sent.slice(1).every((call) => call === "dequeue PR_7")); + assert.deepEqual(notice, noticeOf("refused:dequeue-failed")); +}); + +test("an unlabel run still takes the listing path, never the hold dequeue", async () => { + const { lines } = await run([pull(1)], { EVENT_NAME: "pull_request_target", EVENT_ACTION: "unlabeled", EVENT_PR: "1" }); + assert.equal(lines.filter((line) => String(line).includes("pr-merge-hold-dequeue")).length, 0); +}); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 738f01bd193..5d19d70d612 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -15,7 +15,8 @@ import { join } from "node:path"; import { test } from "node:test"; import { fileURLToPath } from "node:url"; -import { CACHE_ONLY_TIMEOUT_MS, CACHE_ONLY_WINDOW_MS, LANE_ROOM_TIMEOUT_MS, VERIFY_BUDGET_MS, VERIFY_TIMEOUT_MS } from "./enable-auto-merge.mjs"; +import { CACHE_ONLY_TIMEOUT_MS, CACHE_ONLY_WINDOW_MS, HOLD_LABELS, LANE_ROOM_TIMEOUT_MS, VERIFY_BUDGET_MS, VERIFY_TIMEOUT_MS } from "./enable-auto-merge.mjs"; +import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; import { classify, resetLabel } from "../../actions/log-api-budget/rate-guard.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; @@ -482,39 +483,85 @@ test("no workflow_run trigger wakes pr-merge, and the enable-auto-merge job skip assert.doesNotMatch(armIf, /workflow_run/); // A push to main is a merge landing, never a new lane pull request. assert.match(armIf, /github\.event_name != 'push'/); - assert.match(arm, /\n {4}concurrency:\n {6}group: pr-merge-enable-auto-merge\n {6}queue: max\n/); + assert.match( + arm, + /\n {4}concurrency:\n {6}group: \$\{\{ github\.event\.action == 'labeled' && 'pr-merge-hold-dequeue' \|\| 'pr-merge-enable-auto-merge' \}\}\n {6}queue: max\n/, + ); }); // A job its condition skips never joins the group, so a pull request the job // cannot arm (not the lanes App's) shows a skipped check, not one pending // behind every queued run. The other events keep their conditions. -test("a pull request event wakes enable-auto-merge only for a lanes App pull request", () => { - const armIf = /\n {4}if: >-\n([\s\S]*?)\n {4}[a-z-]+:/.exec(job("enable-auto-merge"))[1]; +test("a pull request event wakes enable-auto-merge only for a lanes App pull request, or a hold label added", () => { + const armIf = /\n {4}if: >-\n([\s\S]*?)\n {4}(?:[a-z-]+:|#)/.exec(job("enable-auto-merge"))[1]; assert.match( armIf, - /\(github\.event_name != 'pull_request_target'\n {6}\|\| \(github\.event\.pull_request\.head\.repo\.full_name == github\.repository\n {6}&& format\('\{0\}', github\.event\.pull_request\.user\.id\) == vars\.AUTOMATION_LANES_APP_SENDER_ID\n {6}&& github\.event\.sender\.type != 'Bot'\)\)\n/, + /\(github\.event_name != 'pull_request_target'\n {6}\|\| \(github\.event\.pull_request\.head\.repo\.full_name == github\.repository\n {6}&& \(\(github\.event\.action == 'labeled'\n {6}&& contains\(fromJSON\('\[[^\]]*\]'\), github\.event\.label\.name\)\)\n {6}\|\| \(github\.event\.action != 'labeled'\n {6}&& format\('\{0\}', github\.event\.pull_request\.user\.id\) == vars\.AUTOMATION_LANES_APP_SENDER_ID\)\)\n {6}&& github\.event\.sender\.type != 'Bot'\)\)$/, ); assert.match(armIf, /^ {6}!cancelled\(\)\n/); assert.doesNotMatch(armIf, /schedule|workflow_dispatch/); }); +// The condition's hold labels are enable-auto-merge.mjs's own: HOLD_LABELS +// and the human-gated label the base tracker config names, so a hold the +// script refuses at arm is one a label added wakes it to dequeue. +test("the labeled wake's hold labels are the script's HOLD_LABELS and the human-gated label", () => { + const armIf = /\n {4}if: >-\n([\s\S]*?)\n {4}[a-z-]+:/.exec(job("enable-auto-merge"))[1]; + const listed = JSON.parse(/contains\(fromJSON\('(\[[^\]]*\])'\), github\.event\.label\.name\)/.exec(armIf)[1]); + const humanGated = humanGatedLabelFrom(fileURLToPath(new URL("../../../.work-item-tracker.json", import.meta.url))); + assert.deepEqual(listed, [...HOLD_LABELS, humanGated]); +}); + // A bot-sent pull_request_target run would wait in the queue and then stop at // who.mjs (a bot is never a listed human), so the job's condition skips it // first. The condition's syntax is the subset JavaScript reads the same way, -// so the test evaluates it against hand-written events. +// so the test evaluates it against hand-written events, with contains() and +// fromJSON() as GitHub documents them (contains is not case sensitive). const evalIf = (id, event) => { - const text = /\n {4}if: >-\n([\s\S]*?)\n {4}(?:[a-z-]+:|#)/.exec(job(id))[1].replaceAll("\n", " "); + const text = /\n {4}if: >-\n([\s\S]*?)\n {4}(?:[a-z-]+:|#)/.exec(`\n${job(id)}`)[1].replaceAll("\n", " "); const format = (template, value) => template.replace("{0}", value); - return new Function("github", "vars", "cancelled", "format", `return (${text});`)( + const contains = (search, item) => search.some((value) => String(value).toLowerCase() === String(item).toLowerCase()); + return new Function("github", "vars", "cancelled", "format", "contains", "fromJSON", `return (${text});`)( { ref: "refs/heads/main", repository: "o/r", ...event }, { AUTOMATION_LANES_APP_SENDER_ID: "99" }, () => false, format, + contains, + JSON.parse, ); }; -const pullRequestEvent = (senderType) => ({ +const pullRequestEvent = (senderType, { action = "unlabeled", label = "needs-human", author = 99 } = {}) => ({ event_name: "pull_request_target", - event: { sender: { type: senderType }, pull_request: { head: { repo: { full_name: "o/r" } }, user: { id: 99 } } }, + event: { action, label: { name: label }, sender: { type: senderType }, pull_request: { head: { repo: { full_name: "o/r" } }, user: { id: author } } }, +}); + +// A label added only adds a hold: a hold label wakes the enable-auto-merge job +// alone, for any author's pull request, and any other label wakes no job, so +// that run makes no call. The labels and authors here are the issue's cases: +// do-not-merge and needs-human are holds, `documentation` is not; author 7 is +// a person, 99 the lanes App. +test("a labeled event runs only enable-auto-merge, and only for a hold label a person added", () => { + const ids = ["post-merge-dispatch", "merge", "enable-auto-merge"]; + const runs = (event) => ids.filter((id) => evalIf(id, event)); + assert.deepEqual(runs(pullRequestEvent("User", { action: "labeled", label: "do-not-merge", author: 7 })), ["enable-auto-merge"]); + assert.deepEqual(runs(pullRequestEvent("User", { action: "labeled", label: "Needs-Human", author: 99 })), ["enable-auto-merge"]); + assert.deepEqual(runs(pullRequestEvent("User", { action: "labeled", label: "documentation", author: 99 })), []); + assert.deepEqual(runs(pullRequestEvent("Bot", { action: "labeled", label: "do-not-merge", author: 99 })), []); + // An unlabel keeps its rule: the merge job for any author, enable-auto-merge + // for the lanes App's. + assert.deepEqual(runs(pullRequestEvent("User", { author: 7 })), ["merge"]); + assert.deepEqual(runs(pullRequestEvent("User")), ["merge", "enable-auto-merge"]); + // The jobs that follow the merge job skip with it. + for (const id of ["dispatch-full-run", "gate-check"]) { + assert.match(job(id), /^ {4}needs: \[merge\]\n/); + } +}); + +// A scheduled run does not dequeue, so a labeled run is never superseded. +test("a labeled run never skips its work as superseded", () => { + const step = /- name: Check for a newer run that does this work\n([\s\S]*?)(?=\n {6}- name: )/.exec(job("enable-auto-merge"))?.[1]; + assert.match(step, /^ {8}id: superseded\n {8}if: steps\.who\.outputs\.proceed == 'true' && github\.event\.action != 'labeled'\n/); + assert.match(job("enable-auto-merge"), /\n {10}EVENT_ACTION: \$\{\{ github\.event\.action \}\}\n {10}EVENT_LABEL: \$\{\{ github\.event\.label\.name \}\}\n/); }); for (const id of ["merge", "enable-auto-merge"]) { @@ -538,7 +585,7 @@ test("enable-auto-merge skips its work when a waiting run's enable-auto-merge jo const arm = job("enable-auto-merge"); const step = /- name: Check for a newer run that does this work\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; assert.ok(step, "a superseded step"); - assert.match(step, /^ {8}id: superseded\n {8}if: steps\.who\.outputs\.proceed == 'true'\n {8}continue-on-error: true\n/); + assert.match(step, /^ {8}id: superseded\n {8}if: steps\.who\.outputs\.proceed == 'true' && github\.event\.action != 'labeled'\n {8}continue-on-error: true\n/); assert.match(step, /\n {10}ACTIONS_TOKEN: \$\{\{ github\.token \}\}\n {10}SUPERSEDED_JOB: enable-auto-merge\n/); assert.match(step, /\n {8}run: node \.base\/\.github\/scripts\/pr-merge\/superseded\.mjs\n/); const names = [...arm.matchAll(/\n {6}- name: (.*)/g)].map((m) => m[1]); diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index ee31a1c3023..e58d0b209e3 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -1227,6 +1227,8 @@ const TREE_QUERY = `query($owner: String!, $name: String!, $expression: String!) const QUEUE_ENTRY_QUERY = `query($owner: String!, $name: String!, $pr: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $pr) { id mergeQueueEntry { id } } } }`; +// Takes a pull request, by node id, out of the merge queue. +export const DEQUEUE_MUTATION = "mutation($id: ID!) { dequeuePullRequest(input: { id: $id }) { clientMutationId } }"; const RESOLVE_MUTATION = `mutation($id: ID!) { resolveReviewThread(input: { threadId: $id }) { thread { isResolved } } }`; @@ -1445,7 +1447,7 @@ export function createGitHubApi({ token, repository, apiUrl, graphqlUrl, fetch: return { id: pull.id, queued: pull.mergeQueueEntry !== null }; }, async dequeuePull(id) { - await graphql("mutation($id: ID!) { dequeuePullRequest(input: { id: $id }) { clientMutationId } }", { id }); + await graphql(DEQUEUE_MUTATION, { id }); }, async createIssueComment(pr, body) { await request(`${repo}/issues/${pr}/comments`, "POST", { body }); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 3598aa42a93..f366852c022 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -74,15 +74,19 @@ on: # A label came off. This run's completion starts pr-feedback-dispatch.yml, # the only thing that decides again a lanes App head the human gate skipped - # once needs-human comes off, so `unlabeled` stays. No other PR event starts - # a run; the schedule picks up each change. Adding a label can only add a - # hold. A person's ready or rename restarts their PR's idle clock, so the - # gate would refuse it. An event run never arms a lane PR, and `ci-status` - # runs on a body edit and fails on a hold. pull_request_target runs this - # file as it is on main, not the PR's copy, so a PR cannot change what this - # lane does; the job below never checks out the PR's code. + # once needs-human comes off, so `unlabeled` stays. A label went on: adding + # one can only add a hold, so only a hold label starts work, and only the + # enable-auto-merge job's, which takes that one PR out of the merge queue + # (or disarms it) so its merge group does not run only for `ci-status` to + # fail it; any other label's run skips every job. No other PR event starts + # a run; the schedule picks up each change. A person's ready or rename + # restarts their PR's idle clock, so the gate would refuse it. An event run + # never arms a lane PR, and `ci-status` runs on a body edit and fails on a + # hold. pull_request_target runs this file as it is on main, not the PR's + # copy, so a PR cannot change what this lane does; the job below never + # checks out the PR's code. pull_request_target: # zizmor: ignore[dangerous-triggers] - types: [unlabeled] + types: [labeled, unlabeled] # A person can start a run by hand (Actions tab, "Run workflow") to retry a # PR that looks stuck. Only a run on main does anything. @@ -836,12 +840,14 @@ jobs: # a fork PR or sent by a bot are skipped before the job joins the # concurrency group, so an outsider editing a fork PR over and over cannot # keep pushing real runs out of the queue, and a bot's event does not wait - # in it only to stop at the who step (a bot is never a listed human). + # in it only to stop at the who step (a bot is never a listed human). A + # label added is the enable-auto-merge job's alone: it only adds a hold. if: >- !cancelled() && github.ref == 'refs/heads/main' && github.event_name != 'push' && (github.event_name != 'pull_request_target' || (github.event.pull_request.head.repo.full_name == github.repository + && github.event.action != 'labeled' && github.event.sender.type != 'Bot')) # One run at a time, and a running one is never cancelled: a cancelled run # could stop between the gate's check and its merge-queue write. Extra runs @@ -2184,18 +2190,27 @@ jobs: # kind it arms, and only when a person sent the event (a bot is never a # listed human at the who step): any other event's job is skipped before # it joins the group, so its check reads skipped, never pending behind the - # queue. + # queue. A label added wakes it for any author's pull request, but only a + # hold label: enable-auto-merge.mjs HOLD_LABELS and the tracker's + # human-gated label (workflow.test.mjs checks the list against both). + # GitHub's contains() compares strings in any case, as holdLabel does. if: >- !cancelled() && github.ref == 'refs/heads/main' && github.event_name != 'push' && (github.event_name != 'pull_request_target' || (github.event.pull_request.head.repo.full_name == github.repository - && format('{0}', github.event.pull_request.user.id) == vars.AUTOMATION_LANES_APP_SENDER_ID + && ((github.event.action == 'labeled' + && contains(fromJSON('["do-not-merge","pr-verify-failed","security: outside-actor","needs-human"]'), github.event.label.name)) + || (github.event.action != 'labeled' + && format('{0}', github.event.pull_request.user.id) == vars.AUTOMATION_LANES_APP_SENDER_ID)) && github.event.sender.type != 'Bot')) # A pending run is never replaced: a cancelled run leaves a CANCELLED - # check on the pull request that woke it. + # check on the pull request that woke it. A labeled run takes a group of + # its own, so a dequeue does not wait behind a scheduled run's verdict + # reads (up to its 18-minute arm step) while the merge group it would + # spare runs; it writes only that one pull request's queue entry and arm. concurrency: - group: pr-merge-enable-auto-merge + group: ${{ github.event.action == 'labeled' && 'pr-merge-hold-dequeue' || 'pr-merge-enable-auto-merge' }} queue: max runs-on: ubuntu-24.04 # Above the sum of its steps' timeouts (35, workflow.test.mjs checks it), @@ -2285,11 +2300,12 @@ jobs: # scheduled or hand-started run arms, every run disarms on a hold), this # run ends green here with a notice before it takes the lane token or # reads anything else, so a backlog costs one pass and no run is - # cancelled. Any error leaves superseded unset and the job runs. + # cancelled. Any error leaves superseded unset and the job runs. A + # labeled run's dequeue is work no other run does, so it never skips. # Rules: .github/scripts/pr-merge/superseded.mjs. - name: Check for a newer run that does this work id: superseded - if: steps.who.outputs.proceed == 'true' + if: steps.who.outputs.proceed == 'true' && github.event.action != 'labeled' continue-on-error: true timeout-minutes: 2 env: @@ -2447,6 +2463,8 @@ jobs: EVENT_NAME: ${{ github.event_name }} # Empty on the schedule and a hand-started run. EVENT_PR: ${{ github.event.pull_request.number }} + EVENT_ACTION: ${{ github.event.action }} + EVENT_LABEL: ${{ github.event.label.name }} BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }} BABYSIT_VERIFY_LISTING: ${{ runner.temp }}/verify-listing.json BABYSIT_VERIFY_LISTING_CARRY: ${{ runner.temp }}/verify-listing-carry.json From f7d83dd95a459c125f33ace7d17beb6915183d3d Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:58:25 -0500 Subject: [PATCH 2/3] feat(pr-pipeline): dequeue a pull request when the lanes App adds needs-human Drops the person-only `labeled` wake from pr-merge.yml. Instead, the lanes App's own writers of the human-gated label now dequeue the PR they label, when it sits in the merge queue: - report-escalation.sh on the hold route - enable-auto-merge.mjs refusal notes, both where they add the label and where they put it back This is the same step verify-notes.mjs leaveQueue takes for pr-verify-failed. address-feedback.mjs leaveQueueOnHold does one QUEUE_ENTRY_QUERY read and, when the PR is queued, one DEQUEUE_MUTATION, and prints one hold-label-dequeue notice. A refused read or dequeue warns and never fails the label add. report-escalation.sh loads both queries from address-feedback.mjs, so there is one copy of each. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VcupomBPg7kuobwLoNpxEW --- .../scripts/pr-merge/enable-auto-merge.mjs | 122 ++++------------ .../pr-merge/enable-auto-merge.test.mjs | 131 ++++++------------ .github/scripts/pr-merge/workflow.test.mjs | 67 ++------- .../scripts/pr-unblock/address-feedback.mjs | 37 ++++- .../scripts/pr-unblock/report-escalation.sh | 35 ++++- .../pr-unblock/report-escalation.test.sh | 58 +++++++- .github/workflows/pr-merge.yml | 48 ++----- 7 files changed, 218 insertions(+), 280 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index b8059778d62..b7373ce89dc 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -2,8 +2,8 @@ // enable-auto-merge.mjs: turns on GitHub's native auto-merge for the lanes // App's open pull requests into main, and turns it off on one that now carries // a hold. No model. pr-merge.yml's enable-auto-merge job runs it when a listed -// human unlabels a pull request, or adds a hold label to one (which also takes -// it out of the merge queue), and on the schedule, which arms. +// human edits, unlabels or readies a pull request, and on the schedule, which +// arms. // // A pull request is armed only when all of these hold: // - The lanes App opened it (merge.agent-identity), into main, from a branch @@ -172,9 +172,6 @@ // PR-scoped workflow_dispatch's (inputs.pr), the only // one it handles; such a dispatch reads and arms it as // the schedule would, and writes WAKE_OUTPUT -// EVENT_ACTION, EVENT_LABEL a pull_request_target run's action and, on -// `labeled`, the label added: a labeled run only -// dequeues or disarms on a hold (holdDequeue) // EAM_SKIP_IN, EAM_SKIP_OUT the skip records the last reader run kept, and // the file this one keeps (SKIPPABLE); no OUT, no skip // SKIP_MAX_MINUTES, SKIP_MAX_SKIPS merge.skip-unchanged's limits; 0 in either: no skip @@ -214,7 +211,7 @@ import { pathToFileURL } from "node:url"; import { createGitHub, errorLabel, GitHubError, paginate } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { rateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; -import { DEQUEUE_MUTATION, humanGatedLabelFrom, quote } from "../pr-unblock/address-feedback.mjs"; +import { DEQUEUE_MUTATION, humanGatedLabelFrom, leaveQueueOnHold, QUEUE_ENTRY_QUERY, quote } from "../pr-unblock/address-feedback.mjs"; import { lines as bodyLines, foreignMarker, @@ -421,93 +418,14 @@ async function answerBody(text) { // LANE_HOLD), a cause a person must clear (owner decision Q-HOLDLINE). export const LANE_HOLD_REASON = "its body carries the lane's own 'Hold: a human merges this' line: a person merges it by hand, or removes that line"; -// The first hold label (HOLD_LABELS, then the human-gated label) among -// `labels`, in any case; undefined when none is. -export function holdLabel(labels, humanGated) { - const names = new Set(labels.map((label) => String(label?.name).toLowerCase())); - return [...HOLD_LABELS, humanGated].find((name) => names.has(name.toLowerCase())); -} - -// A `labeled` run's one pull request, read live: the event names the label, -// never whether it still holds. -export const HOLD_PULL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { - repository(owner: $owner, name: $name) { - pullRequest(number: $number) { - id number state baseRefName - author { __typename login } - labels(first: 100) { totalCount nodes { name } } - autoMergeRequest { enabledAt } - mergeQueueEntry { id } - } - } -}`; -export const HOLD_DEQUEUE_TITLE = "pr-merge-hold-dequeue"; -export const holdDequeueLine = (number, label, outcome) => - `::notice title=${HOLD_DEQUEUE_TITLE}::pr=${escapeCommand(number)} label=${escapeCommand(label)} outcome=${escapeCommand(outcome)}`; - -// A person added a label (EVENT_ACTION labeled): when a hold label stands on -// the pull request now, it leaves the merge queue, so its merge group does not -// run only for ci-status to fail it, and a lanes App pull request still armed -// is disarmed. Any author: the queue also holds the pull requests the gate -// enqueued. Two calls at most past the read, and one line naming the outcome. -// Returns the exit code. -async function holdDequeue(github, { repo, env, humanGated, write, slug, disarm }) { - const number = Number(env.EVENT_PR); - const said = (outcome) => console.log(holdDequeueLine(env.EVENT_PR ?? "", env.EVENT_LABEL ?? "", outcome)); - let pull; - try { - pull = (await github("POST", "/graphql", { query: HOLD_PULL_QUERY, variables: { owner: repo[1], name: repo[2], number } }))?.data?.repository?.pullRequest; - if (pull != null && !whole(pull.labels)) { - pull.labels = await allLabels(github, repo[0], number); - } - } catch (error) { - said("refused:read-failed"); - console.log(`::error title=enable-auto-merge::#${number} hold read: ${describe(error)}`); - return 1; - } - if (pull?.number !== number || pull.state !== "OPEN" || pull.baseRefName !== "main") { - said("refused:not-open-into-main"); - return 0; - } - if (holdLabel(pull.labels?.nodes ?? [], humanGated) === undefined) { - said("refused:no-hold-label"); - return 0; - } - const queued = pull.mergeQueueEntry != null; - const armed = pull.autoMergeRequest != null && pull.author?.__typename === "Bot" && pull.author.login === slug; - if (!queued && !armed) { - said("not-queued"); - return 0; - } - if (!write) { - said("refused:report-only"); - return 0; - } - let step = "dequeue"; - try { - if (queued) { - await github("POST", "/graphql", { query: DEQUEUE_MUTATION, variables: { id: pull.id } }); - } - step = "disarm"; - if (armed) { - await disarm(pull); - } - } catch (error) { - said(`refused:${step}-failed`); - console.log(`::error title=enable-auto-merge::#${number} ${step}: ${describe(error)}`); - return 1; - } - said(queued ? "dequeued" : "disarmed"); - return 0; -} - // Why `pull` must not be armed, or null when it may be. export function holdReason(pull, humanGated) { const labels = pull.labels?.nodes ?? []; if (!(pull.labels?.totalCount <= labels.length)) { return "its labels could not all be read"; } - const label = holdLabel(labels, humanGated); + const names = new Set(labels.map((label) => String(label?.name).toLowerCase())); + const label = [...HOLD_LABELS, humanGated].find((name) => names.has(name.toLowerCase())); if (label !== undefined) { return `it carries ${label}`; } @@ -1462,7 +1380,7 @@ export function unblockHold(stops) { if (!Array.isArray(stops)) { return "its head's pr-unblock statuses could not be read"; } - return stops.length === 0 ? null : `a pr-unblock lane stopped its head (${stops.map(({ context, state }) => `${context} is ${state}`).join(", ")}), which ci-status fails in the merge queue`; + return stops.length === 0 ? null : `a pr-unblock lane stopped its head (${stops.map(({ context, state }) => `${context} is ${state}`).join(", ")}), which clears when the lane runs again`; } // What to do with one listed pull request: { action: arm | disarm | none, reason }. @@ -1504,8 +1422,8 @@ export function decide(pull, { bot, humanGated, holdPaths = null, pathHolds = tr if (detail.filesCapped === true) { return { action: armed ? "disarm" : "none", reason: FILES_CAPPED_REASON, clears: "person" }; } - // ci-status's merge-queue run fails a head a pr-unblock lane stopped, so it - // is never queued; a status list that did not read holds it too. + // A head a pr-unblock lane stopped is never queued: only this job reads the + // stop. A status list that did not read holds it too. const stopped = unblockHold(detail.unblockStops); // Review rounds the lane stopped as not converging at this head close below, // after the path, cost and discussion holds, as the verify cap does. @@ -1732,11 +1650,7 @@ export async function main({ env = process.env, github, actions, verify, room, c if (!repo) { throw new Error("GITHUB_REPOSITORY is not owner/repo"); } - const labeled = env.EVENT_NAME === "pull_request_target" && env.EVENT_ACTION === "labeled"; if (env.KILL_SWITCH !== "proceed") { - if (labeled) { - console.log(holdDequeueLine(env.EVENT_PR ?? "", env.EVENT_LABEL ?? "", "refused:kill-switch")); - } console.log("enable-auto-merge: the lanes kill switch stops this run; nothing read or armed"); return 0; } @@ -1808,9 +1722,6 @@ export async function main({ env = process.env, github, actions, verify, room, c console.log(`enable-auto-merge: #${pull.number} ${action} failed after a resend (${describe(error)}), but it is already ${action === "arm" ? `armed at ${pull.headRefOid}` : "unarmed"}, so an earlier send landed`); } }; - if (labeled) { - return await holdDequeue(github, { repo, env, humanGated, write, slug, disarm: (pull) => sendSettled("disarm", pull) }); - } verify ??= pythonVerify(env); whole ??= pythonWholeReview(env); const reads = READER_EVENTS.includes(env.EVENT_NAME); @@ -1933,6 +1844,21 @@ export async function main({ env = process.env, github, actions, verify, room, c // would be all anyone saw. // Writes note sent or tried, for skip-unchanged. let sent = 0; + // A human-gated label this run added holds the pull request: a queued one + // leaves the queue (leaveQueueOnHold), which warns and never fails the note. + const leaveQueue = (pull, label) => + leaveQueueOnHold({ + pr: pull.number, + label, + readQueueEntry: async (pr) => { + const read = (await github("POST", "/graphql", { query: QUEUE_ENTRY_QUERY, variables: { owner: repo[1], name: repo[2], pr } }))?.data?.repository?.pullRequest; + if (typeof read?.id !== "string") { + throw new Error("the pull request did not read"); + } + return { id: read.id, queued: read.mergeQueueEntry != null }; + }, + dequeuePull: (id) => github("POST", "/graphql", { query: DEQUEUE_MUTATION, variables: { id } }), + }); const note = async (pull, detail, refusal) => { if (!reads || stopped !== null || refusal.clears === undefined) { return; @@ -1991,6 +1917,7 @@ export async function main({ env = process.env, github, actions, verify, room, c return; } await github("POST", `${base}/${pull.number}/labels`, { labels: [writes.label] }); + await leaveQueue(pull, writes.label); console.log(`enable-auto-merge: #${pull.number} labeled ${writes.label} again: its refusal comment stands and no person removed the label`); return; } @@ -1999,6 +1926,7 @@ export async function main({ env = process.env, github, actions, verify, room, c : github("PATCH", `${base}/comments/${writes.commentId}`, { body: writes.body })); if (writes.label) { await github("POST", `${base}/${pull.number}/labels`, { labels: [writes.label] }); + await leaveQueue(pull, writes.label); } console.log(`enable-auto-merge: #${pull.number} refusal noted on the pull request${writes.label ? `, labeled ${writes.label}` : ""}`); } 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 448a1c1937e..33dc4f1faad 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -60,7 +60,6 @@ import { readRoom, TRUST_REFUSALS, FILES_CAPPED_REASON, - HOLD_PULL_QUERY, UNARMED_OUTPUT, WAKE_OUTPUT, verifyClears, @@ -68,7 +67,7 @@ import { SKIP_STATE_PREFIX, } from "./enable-auto-merge.mjs"; import { GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; -import { DEQUEUE_MUTATION } from "../pr-unblock/address-feedback.mjs"; +import { DEQUEUE_MUTATION, QUEUE_ENTRY_QUERY } from "../pr-unblock/address-feedback.mjs"; import { createRateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; import { COMMENT_EDITED_QUERY, LANE_HOLD, PULL_OPENED_QUERY, VERIFY_PATH, personWrote, unblockStops, verdictHold } from "./merge-holds.mjs"; import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; @@ -142,7 +141,7 @@ const labels = (...names) => ({ totalCount: names.length, nodes: names.map((name // filesError: what every changed-files GET throws. // A pull request's `after` is its auto-merge state as the read-back sees it // (AUTO_MERGE_STATE_QUERY), else its listed state; `after: "error"` fails the read. -function world(pulls, { failArm = false, armErrors = {}, disarmErrors = {}, filesError = null, noteErrors = [], disarmError = null, pinError = null } = {}) { +function world(pulls, { failArm = false, armErrors = {}, disarmErrors = {}, filesError = null, noteErrors = [], disarmError = null, pinError = null, dequeueError = null } = {}) { const pins = []; const noteThrows = [...noteErrors]; const armThrows = Object.fromEntries(Object.entries(armErrors).map(([id, errors]) => [id, [...errors]])); @@ -310,6 +309,14 @@ function world(pulls, { failArm = false, armErrors = {}, disarmErrors = {}, file const nodes = all.slice(start, start + 2); return { data: { node: { comments: { totalCount: all.length, pageInfo: { hasNextPage: start + 2 < all.length, endCursor: String(start + 2) }, nodes } } } }; } + // A pull request's merge-queue entry: `queued` puts it in the queue. + if (body.query === QUEUE_ENTRY_QUERY) { + const found = pulls.find((candidate) => candidate.number === body.variables.pr); + return { data: { repository: { pullRequest: { id: found?.id, mergeQueueEntry: found?.queued ? { id: `MQE_${found.number}` } : null } } } }; + } + if (body.query === DEQUEUE_MUTATION && dequeueError) { + throw dequeueError; + } if (body.query === ARM_MUTATION && armThrows[body.variables.id]?.length > 0) { throw armThrows[body.variables.id].shift(); } @@ -348,8 +355,8 @@ function world(pulls, { failArm = false, armErrors = {}, disarmErrors = {}, file }; const writes = () => calls - .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY, UNLABELED_QUERY, DISCUSSION_PAGE_QUERIES.comments, THREAD_PAGE_QUERY, AUTO_MERGE_STATE_QUERY].includes(body.query) && !String(body.query).startsWith(SKIP_STATE_PREFIX)) - .map(({ body }) => ({ mutation: body.query === ARM_MUTATION ? "arm" : body.query === DISARM_MUTATION ? "disarm" : "?", ...body.variables })); + .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY, UNLABELED_QUERY, DISCUSSION_PAGE_QUERIES.comments, THREAD_PAGE_QUERY, AUTO_MERGE_STATE_QUERY, QUEUE_ENTRY_QUERY].includes(body.query) && !String(body.query).startsWith(SKIP_STATE_PREFIX)) + .map(({ body }) => ({ mutation: body.query === ARM_MUTATION ? "arm" : body.query === DISARM_MUTATION ? "disarm" : body.query === DEQUEUE_MUTATION ? "dequeue" : "?", ...body.variables })); return { github, writes, verify, verified, calls, notes, pins }; } @@ -3593,9 +3600,8 @@ test("serve asks for a kept answer only for an unarmed pull request, and never o assert.deepEqual(spawned[0].slice(spawned[0].indexOf("--answers-out"), spawned[0].indexOf("--answers-out") + 6), ["--answers-out", "/tmp/out.json", "--answers-mode", "serve", "--answers-in", "/tmp/in.json"]); }); -// ci-status's merge-queue run fails a head whose newest pr-unblock/* status by -// github-actions[bot] is failure or error (pr-require-checks.yml "Fail a head a -// pr-unblock lane stopped"), so enable-auto-merge never queues one. +// enable-auto-merge never queues a head whose newest pr-unblock/* status by +// github-actions[bot] is failure or error; ci-status does not read the stop. const BOT = { login: "github-actions[bot]" }; const unblock = (context, state, creator = BOT, description = "stopped") => ({ context, state, creator, description }); @@ -3624,7 +3630,7 @@ test("a head a pr-unblock lane stopped is not queued and is named in the summary const stopped = pull(1, { verdict: PASSED, statuses: [unblock("pr-unblock/fix-ci", "failure")] }); const { writes, lines } = await run([stopped], { GITHUB_STEP_SUMMARY: summary }); assert.deepEqual(writes, []); - const reason = "a pr-unblock lane stopped its head (pr-unblock/fix-ci is failure), which ci-status fails in the merge queue"; + const reason = "a pr-unblock lane stopped its head (pr-unblock/fix-ci is failure), which clears when the lane runs again"; assert.ok(lines.some((line) => String(line).includes(reason)), lines.join("\n")); assert.equal(readFileSync(summary, "utf8"), `enable-auto-merge: #1 not queued: ${reason}\n`); @@ -4040,90 +4046,39 @@ test("a skip record of another repository, from the future, or malformed skips n } }); -// A person adds a label after the pull request entered the merge queue: a -// hold label standing on it now takes it out of the queue, or disarms a lanes -// App pull request still armed, and the run says which in one notice. -// Expected outcomes are the work item's: queued dequeues, armed disarms, no -// hold left writes nothing. -async function labeledRun(live, { env = {}, fail = null } = {}) { - const sent = []; - const github = async (method, path, body) => { - sent.push(body?.query === HOLD_PULL_QUERY ? "read" : body?.query === DEQUEUE_MUTATION ? `dequeue ${body.variables.id}` : body?.query === DISARM_MUTATION ? `disarm ${body.variables.id}` : `${method} ${path}`); - if (fail !== null && sent.at(-1).startsWith(fail)) { - throw new GitHubError(200, "POST /graphql (GraphQL errors)"); - } - if (body?.query === HOLD_PULL_QUERY) { - return { data: { repository: { pullRequest: live } } }; - } - return { data: {} }; - }; - const lines = []; - const log = console.log; - console.log = (line) => lines.push(line); - try { - const code = await main({ env: { ...ENV, EVENT_NAME: "pull_request_target", EVENT_ACTION: "labeled", EVENT_PR: "7", EVENT_LABEL: "do-not-merge", ...env }, github }); - return { code, sent, notice: lines.filter((line) => line.startsWith("::notice title=pr-merge-hold-dequeue::")) }; - } finally { - console.log = log; - } -} -const live = (overrides = {}) => ({ - id: "PR_7", - number: 7, - state: "OPEN", - baseRefName: "main", - author: { __typename: "User", login: "someone" }, - labels: { totalCount: 1, nodes: [{ name: "do-not-merge" }] }, - autoMergeRequest: null, - mergeQueueEntry: null, - ...overrides, +// The lanes App adding the human-gated label to a queued pull request takes it +// out of the merge queue, so its merge group does not run only for ci-status +// to fail it on the label: one queue read per label add, and the dequeue only +// when it is queued. A refused dequeue warns; the label still holds it. +const queueReads = (calls) => calls.filter(({ body }) => body?.query === QUEUE_ENTRY_QUERY).map(({ body }) => body.variables.pr); +const holdNotice = (outcome) => `::notice title=hold-label-dequeue::pr=1 label=needs-human outcome=${outcome}`; + +test("a human-gated label the lane adds to a queued pull request dequeues it", async () => { + const { code, writes, calls, notes, lines } = await run([pull(1, { files: COST_FILES, queued: true })]); + assert.equal(code, 0); + assert.deepEqual(notes.at(-1), { method: "POST", path: `${ISSUES}/1/labels`, body: { labels: ["needs-human"] } }); + assert.deepEqual(queueReads(calls), [1]); + assert.deepEqual(writes, [{ mutation: "dequeue", id: "PR_1" }]); + assert.ok(lines.includes(holdNotice("dequeued"))); }); -const noticeOf = (outcome, label = "do-not-merge") => [`::notice title=pr-merge-hold-dequeue::pr=7 label=${label} outcome=${outcome}`]; -test("a hold label added to a queued pull request dequeues it, whoever opened it", async () => { - const person = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } })); - assert.equal(person.code, 0); - assert.deepEqual(person.sent, ["read", "dequeue PR_7"]); - assert.deepEqual(person.notice, noticeOf("dequeued")); - // A lanes App pull request still armed in the queue is disarmed too. - const lanes = await labeledRun(live({ author: LANES, autoMergeRequest: { enabledAt: NOW }, mergeQueueEntry: { id: "MQE_1" } })); - assert.deepEqual(lanes.sent, ["read", "dequeue PR_7", "disarm PR_7"]); - assert.deepEqual(lanes.notice, noticeOf("dequeued")); +test("a human-gated label on a pull request not in the queue reads its entry once and writes nothing more", async () => { + const { code, writes, calls, lines } = await run([pull(1, { files: COST_FILES })]); + assert.equal(code, 0); + assert.deepEqual(queueReads(calls), [1]); + assert.deepEqual(writes, []); + assert.ok(lines.includes(holdNotice("not-queued"))); }); -test("a hold label added to an armed lanes App pull request not yet queued disarms it", async () => { - const { code, sent, notice } = await labeledRun(live({ author: LANES, autoMergeRequest: { enabledAt: NOW }, labels: { totalCount: 1, nodes: [{ name: "needs-human" }] } }), { env: { EVENT_LABEL: "needs-human" } }); +test("a refused dequeue warns and leaves the label add standing", async () => { + const { code, notes, lines } = await run([pull(1, { files: COST_FILES, queued: true })], {}, { dequeueError: new GitHubError(200, "POST /graphql (GraphQL errors)") }); assert.equal(code, 0); - assert.deepEqual(sent, ["read", "disarm PR_7"]); - assert.deepEqual(notice, noticeOf("disarmed", "needs-human")); -}); - -test("a labeled run writes nothing when no hold stands, the pull request is not queued or armed, or the lane only reports", async () => { - // The label came off again before the run read it. - const gone = await labeledRun(live({ labels: { totalCount: 1, nodes: [{ name: "documentation" }] }, mergeQueueEntry: { id: "MQE_1" } })); - assert.deepEqual([gone.sent, gone.notice], [["read"], noticeOf("refused:no-hold-label")]); - const idle = await labeledRun(live()); - assert.deepEqual([idle.sent, idle.notice], [["read"], noticeOf("not-queued")]); - // A person's own auto-merge is theirs: only the lanes App's is disarmed. - const theirs = await labeledRun(live({ autoMergeRequest: { enabledAt: NOW } })); - assert.deepEqual([theirs.sent, theirs.notice], [["read"], noticeOf("not-queued")]); - const closed = await labeledRun(live({ state: "MERGED", mergeQueueEntry: { id: "MQE_1" } })); - assert.deepEqual([closed.sent, closed.notice], [["read"], noticeOf("refused:not-open-into-main")]); - const reported = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } }), { env: { REPORT_ONLY: "true" } }); - assert.deepEqual([reported.sent, reported.notice], [["read"], noticeOf("refused:report-only")]); - const off = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } }), { env: { KILL_SWITCH: "" } }); - assert.deepEqual([off.sent, off.notice], [[], noticeOf("refused:kill-switch")]); -}); - -test("a refused dequeue fails the run and names it", async () => { - const { code, sent, notice } = await labeledRun(live({ mergeQueueEntry: { id: "MQE_1" } }), { fail: "dequeue" }); - assert.equal(code, 1); - assert.equal(sent[0], "read"); - assert.ok(sent.slice(1).every((call) => call === "dequeue PR_7")); - assert.deepEqual(notice, noticeOf("refused:dequeue-failed")); + assert.deepEqual(notes.at(-1), { method: "POST", path: `${ISSUES}/1/labels`, body: { labels: ["needs-human"] } }); + assert.ok(lines.some((line) => String(line).startsWith("::warning::#1: could not dequeue it"))); + assert.ok(lines.includes(holdNotice("refused:dequeue-failed"))); }); -test("an unlabel run still takes the listing path, never the hold dequeue", async () => { - const { lines } = await run([pull(1)], { EVENT_NAME: "pull_request_target", EVENT_ACTION: "unlabeled", EVENT_PR: "1" }); - assert.equal(lines.filter((line) => String(line).includes("pr-merge-hold-dequeue")).length, 0); +test("a refusal that adds no label reads no queue entry", async () => { + const { calls } = await run([pull(1, { verdict: MISSING })]); + assert.deepEqual(queueReads(calls), []); }); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index fa3da4468bb..239357fb944 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -15,8 +15,7 @@ import { join } from "node:path"; import { test } from "node:test"; import { fileURLToPath } from "node:url"; -import { CACHE_ONLY_TIMEOUT_MS, CACHE_ONLY_WINDOW_MS, HOLD_LABELS, LANE_ROOM_TIMEOUT_MS, VERIFY_BUDGET_MS, VERIFY_TIMEOUT_MS } from "./enable-auto-merge.mjs"; -import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; +import { CACHE_ONLY_TIMEOUT_MS, CACHE_ONLY_WINDOW_MS, LANE_ROOM_TIMEOUT_MS, VERIFY_BUDGET_MS, VERIFY_TIMEOUT_MS } from "./enable-auto-merge.mjs"; import { classify, resetLabel } from "../../actions/log-api-budget/rate-guard.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; @@ -483,52 +482,35 @@ test("no workflow_run trigger wakes pr-merge, and the enable-auto-merge job skip assert.doesNotMatch(armIf, /workflow_run/); // A push to main is a merge landing, never a new lane pull request. assert.match(armIf, /github\.event_name != 'push'/); - assert.match( - arm, - /\n {4}concurrency:\n {6}group: \$\{\{ github\.event\.action == 'labeled' && 'pr-merge-hold-dequeue' \|\| 'pr-merge-enable-auto-merge' \}\}\n {6}queue: max\n/, - ); + assert.match(arm, /\n {4}concurrency:\n {6}group: pr-merge-enable-auto-merge\n {6}queue: max\n/); }); // A job its condition skips never joins the group, so a pull request the job // cannot arm (not the lanes App's) shows a skipped check, not one pending // behind every queued run. The other events keep their conditions. -test("a pull request event wakes enable-auto-merge only for a lanes App pull request, or a hold label added", () => { - const armIf = /\n {4}if: >-\n([\s\S]*?)\n {4}(?:[a-z-]+:|#)/.exec(job("enable-auto-merge"))[1]; +test("a pull request event wakes enable-auto-merge only for a lanes App pull request", () => { + const armIf = /\n {4}if: >-\n([\s\S]*?)\n {4}[a-z-]+:/.exec(job("enable-auto-merge"))[1]; assert.match( armIf, - /\(github\.event_name != 'pull_request_target'\n {6}\|\| \(github\.event\.pull_request\.head\.repo\.full_name == github\.repository\n {6}&& \(\(github\.event\.action == 'labeled'\n {6}&& contains\(fromJSON\('\[[^\]]*\]'\), github\.event\.label\.name\)\)\n {6}\|\| \(github\.event\.action != 'labeled'\n {6}&& format\('\{0\}', github\.event\.pull_request\.user\.id\) == vars\.AUTOMATION_LANES_APP_SENDER_ID\)\)\n {6}&& github\.event\.sender\.type != 'Bot'\)\)$/, + /\(github\.event_name != 'pull_request_target'\n {6}\|\| \(github\.event\.pull_request\.head\.repo\.full_name == github\.repository\n {6}&& format\('\{0\}', github\.event\.pull_request\.user\.id\) == vars\.AUTOMATION_LANES_APP_SENDER_ID\n {6}&& github\.event\.sender\.type != 'Bot'\)\)\n/, ); assert.match(armIf, /^ {6}!cancelled\(\)\n/); assert.doesNotMatch(armIf, /schedule|workflow_dispatch/); }); -// The condition's hold labels are enable-auto-merge.mjs's own: HOLD_LABELS -// and the human-gated label the base tracker config names, so a hold the -// script refuses at arm is one a label added wakes it to dequeue. -test("the labeled wake's hold labels are the script's HOLD_LABELS and the human-gated label", () => { - const armIf = /\n {4}if: >-\n([\s\S]*?)\n {4}[a-z-]+:/.exec(job("enable-auto-merge"))[1]; - const listed = JSON.parse(/contains\(fromJSON\('(\[[^\]]*\])'\), github\.event\.label\.name\)/.exec(armIf)[1]); - const humanGated = humanGatedLabelFrom(fileURLToPath(new URL("../../../.work-item-tracker.json", import.meta.url))); - assert.deepEqual(listed, [...HOLD_LABELS, humanGated]); -}); - // A bot-sent pull_request_target run would wait in the queue and then stop at // who.mjs (a bot is never a listed human), so the job's condition skips it // first. The condition's syntax is the subset JavaScript reads the same way, -// so the test evaluates it against hand-written events, with contains() and -// fromJSON() as GitHub documents them (contains is not case sensitive). +// so the test evaluates it against hand-written events. const evalIf = (id, event, inputs = {}) => { const text = /\n {4}if: >-\n([\s\S]*?)\n {4}(?:[a-z-]+:|#)/.exec(`\n${job(id)}`)[1].replaceAll("\n", " "); const format = (template, value) => template.replace("{0}", value); - const contains = (search, item) => search.some((value) => String(value).toLowerCase() === String(item).toLowerCase()); - return new Function("github", "vars", "cancelled", "format", "inputs", "contains", "fromJSON", `return (${text});`)( + return new Function("github", "vars", "cancelled", "format", "inputs", `return (${text});`)( { ref: "refs/heads/main", repository: "o/r", ...event }, { AUTOMATION_LANES_APP_SENDER_ID: "99" }, () => false, format, inputs, - contains, - JSON.parse, ); }; @@ -667,38 +649,9 @@ test("a PR-scoped run with no standing report skips the arm step, so it disarms assert.match(report, /\n {10}STANDS: \$\{\{ steps\.keep-report\.outputs\.stands \}\}\n/); assert.match(report, /elif \[ "\$STANDS" = false \]; then\n\s+OUTCOME="skipped: the last sweep's report does not stand"/); }); -const pullRequestEvent = (senderType, { action = "unlabeled", label = "needs-human", author = 99 } = {}) => ({ +const pullRequestEvent = (senderType) => ({ event_name: "pull_request_target", - event: { action, label: { name: label }, sender: { type: senderType }, pull_request: { head: { repo: { full_name: "o/r" } }, user: { id: author } } }, -}); - -// A label added only adds a hold: a hold label wakes the enable-auto-merge job -// alone, for any author's pull request, and any other label wakes no job, so -// that run makes no call. The labels and authors here are the issue's cases: -// do-not-merge and needs-human are holds, `documentation` is not; author 7 is -// a person, 99 the lanes App. -test("a labeled event runs only enable-auto-merge, and only for a hold label a person added", () => { - const ids = ["post-merge-dispatch", "merge", "enable-auto-merge"]; - const runs = (event) => ids.filter((id) => evalIf(id, event)); - assert.deepEqual(runs(pullRequestEvent("User", { action: "labeled", label: "do-not-merge", author: 7 })), ["enable-auto-merge"]); - assert.deepEqual(runs(pullRequestEvent("User", { action: "labeled", label: "Needs-Human", author: 99 })), ["enable-auto-merge"]); - assert.deepEqual(runs(pullRequestEvent("User", { action: "labeled", label: "documentation", author: 99 })), []); - assert.deepEqual(runs(pullRequestEvent("Bot", { action: "labeled", label: "do-not-merge", author: 99 })), []); - // An unlabel keeps its rule: the merge job for any author, enable-auto-merge - // for the lanes App's. - assert.deepEqual(runs(pullRequestEvent("User", { author: 7 })), ["merge"]); - assert.deepEqual(runs(pullRequestEvent("User")), ["merge", "enable-auto-merge"]); - // The jobs that follow the merge job skip with it. - for (const id of ["dispatch-full-run", "gate-check"]) { - assert.match(job(id), /^ {4}needs: \[merge\]\n/); - } -}); - -// A scheduled run does not dequeue, so a labeled run is never superseded. -test("a labeled run never skips its work as superseded", () => { - const step = /- name: Check for a newer run that does this work\n([\s\S]*?)(?=\n {6}- name: )/.exec(job("enable-auto-merge"))?.[1]; - assert.match(step, /^ {8}id: superseded\n {8}if: steps\.who\.outputs\.proceed == 'true' && github\.event\.action != 'labeled'\n/); - assert.match(job("enable-auto-merge"), /\n {10}EVENT_ACTION: \$\{\{ github\.event\.action \}\}\n {10}EVENT_LABEL: \$\{\{ github\.event\.label\.name \}\}\n/); + event: { sender: { type: senderType }, pull_request: { head: { repo: { full_name: "o/r" } }, user: { id: 99 } } }, }); for (const id of ["merge", "enable-auto-merge"]) { @@ -722,7 +675,7 @@ test("enable-auto-merge skips its work when a waiting run's enable-auto-merge jo const arm = job("enable-auto-merge"); const step = /- name: Check for a newer run that does this work\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; assert.ok(step, "a superseded step"); - assert.match(step, /^ {8}id: superseded\n {8}if: steps\.who\.outputs\.proceed == 'true' && github\.event\.action != 'labeled'\n {8}continue-on-error: true\n/); + assert.match(step, /^ {8}id: superseded\n {8}if: steps\.who\.outputs\.proceed == 'true'\n {8}continue-on-error: true\n/); assert.match(step, /\n {10}ACTIONS_TOKEN: \$\{\{ github\.token \}\}\n {10}SUPERSEDED_JOB: enable-auto-merge\n/); assert.match(step, /\n {8}run: node \.base\/\.github\/scripts\/pr-merge\/superseded\.mjs\n/); const names = [...arm.matchAll(/\n {6}- name: (.*)/g)].map((m) => m[1]); diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index e58d0b209e3..d4cfc02dbc2 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -1224,11 +1224,46 @@ const TREE_QUERY = `query($owner: String!, $name: String!, $expression: String!) } }`; -const QUEUE_ENTRY_QUERY = `query($owner: String!, $name: String!, $pr: Int!) { +export const QUEUE_ENTRY_QUERY = `query($owner: String!, $name: String!, $pr: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $pr) { id mergeQueueEntry { id } } } }`; // Takes a pull request, by node id, out of the merge queue. export const DEQUEUE_MUTATION = "mutation($id: ID!) { dequeuePullRequest(input: { id: $id }) { clientMutationId } }"; + +export const HOLD_DEQUEUE_TITLE = "hold-label-dequeue"; +export const holdDequeueLine = (pr, label, outcome) => + `::notice title=${HOLD_DEQUEUE_TITLE}::pr=${String(pr).replaceAll(/[\r\n%]/g, " ")} label=${String(label).replaceAll(/[\r\n%]/g, " ")} outcome=${outcome}`; + +// After the lanes App adds a hold label to pull request `pr`: one read of its +// queue entry and, when it is queued, the dequeue, so its merge group does not +// run only for ci-status to fail it on the label. A refused read or dequeue +// warns and returns; the label is the hold, and ci-status still reads it in +// the queue. Logs one HOLD_DEQUEUE_TITLE notice; returns its outcome. +export async function leaveQueueOnHold({ readQueueEntry, dequeuePull, pr, label, log = console.log }) { + let outcome; + let entry = null; + try { + entry = await readQueueEntry(pr); + } catch (error) { + outcome = "refused:read-failed"; + log(`::warning::#${pr}: its merge-queue entry did not read (${error.message}); it was not dequeued`); + } + if (entry !== null) { + if (!entry.queued) { + outcome = "not-queued"; + } else { + try { + await dequeuePull(entry.id); + outcome = "dequeued"; + } catch (error) { + outcome = "refused:dequeue-failed"; + log(`::warning::#${pr}: could not dequeue it (${error.message}); ci-status still fails its merge group on ${label}`); + } + } + } + log(holdDequeueLine(pr, label, outcome)); + return outcome; +} const RESOLVE_MUTATION = `mutation($id: ID!) { resolveReviewThread(input: { threadId: $id }) { thread { isResolved } } }`; diff --git a/.github/scripts/pr-unblock/report-escalation.sh b/.github/scripts/pr-unblock/report-escalation.sh index a32818aeb1b..d800883cdcc 100755 --- a/.github/scripts/pr-unblock/report-escalation.sh +++ b/.github/scripts/pr-unblock/report-escalation.sh @@ -9,8 +9,9 @@ # question, a CRITICAL security risk or a billing risk, a person's own # thread or hold): hands the PR to a person with one machine-marked comment # holding the reason, the resume key and how to resume, plus the -# human-gated role label. A reason the table does not list holds too, with -# no activity to resume (fail-safe). +# human-gated role label; a PR the label goes on while it sits in the merge +# queue leaves it (leave_queue). A reason the table does not list holds +# too, with no activity to resume (fail-safe). # - route `fail` (a stop the lane's own bounded retries did not mend): no # human-gated label; the entry's `label` (needs-session), when it names one, # puts the PR in the session-builder queue for a change the lane cannot @@ -68,6 +69,35 @@ die() { routes=$(dirname "${BASH_SOURCE[0]}")/escalation-routes.json +# After the hold route adds the human-gated label: a queued PR leaves the merge +# queue, so its group does not run only for ci-status to fail it on the label. +# The read and the dequeue are address-feedback.mjs's QUEUE_ENTRY_QUERY and +# DEQUEUE_MUTATION (leaveQueueOnHold), the one copy of each. A refused read or +# dequeue warns and the escalation goes on: the label is the hold. Prints one +# hold-label-dequeue notice. +leave_queue() { + local queries answer entry outcome number=${PR_NUMBER:-} repo=${GITHUB_REPOSITORY:-} + if ! queries=$(QUEUE_MODULE="$(dirname "${BASH_SOURCE[0]}")/address-feedback.mjs" node --input-type=module -e ' + const { pathToFileURL } = await import("node:url"); + const m = await import(pathToFileURL(process.env.QUEUE_MODULE).href); + process.stdout.write(JSON.stringify({ read: m.QUEUE_ENTRY_QUERY, dequeue: m.DEQUEUE_MUTATION }));'); then + outcome=refused:queries-unread + echo "::warning::#$number: the merge-queue queries did not load; it was not dequeued" + elif ! answer=$(gh api graphql -f "query=$(jq -r .read <<<"$queries")" -f "owner=${repo%/*}" -f "name=${repo#*/}" -F "pr=$number") || + ! entry=$(jq -er '.data.repository.pullRequest | select(.id | type == "string") | [.id, (.mergeQueueEntry != null)] | @tsv' <<<"$answer"); then + outcome=refused:read-failed + echo "::warning::#$number: its merge-queue entry did not read; it was not dequeued" + elif [[ ${entry#*$'\t'} != true ]]; then + outcome=not-queued + elif gh api graphql -f "query=$(jq -r .dequeue <<<"$queries")" -f "id=${entry%%$'\t'*}" >/dev/null; then + outcome=dequeued + else + outcome=refused:dequeue-failed + echo "::warning::#$number: could not dequeue it; ci-status still fails its merge group on $1" + fi + echo "::notice title=hold-label-dequeue::pr=$number label=$1 outcome=$outcome" +} + # `clear`: an attempt of ACTIVITY on HEAD_SHA ended without a stop (a same-head # retry landed). When the newest status of the activity's context on the # head, by github-actions[bot], is a failure or error, it posts success over it, so a @@ -335,6 +365,7 @@ EOF if [[ $labeled != true ]]; then gh api -X POST "$issue/labels" -f "labels[]=$label" >/dev/null + leave_queue "$label" fi gh api -X POST "$issue/comments" -F "body=@$body" >/dev/null || die "cannot post the escalation comment; the $label label is on" echo "escalated: $ESCALATION_REASON, label $label, resume key $HEAD_SHA" diff --git a/.github/scripts/pr-unblock/report-escalation.test.sh b/.github/scripts/pr-unblock/report-escalation.test.sh index 1742e6d211e..7c8d1f5a54d 100755 --- a/.github/scripts/pr-unblock/report-escalation.test.sh +++ b/.github/scripts/pr-unblock/report-escalation.test.sh @@ -51,6 +51,21 @@ printf '%s %s\n' "$method" "$endpoint" >>"$STUB/requests" # The api-budget wrapper asks for the headers (-i): a status line and one # rate-limit header lead the body. [[ " $* " != *" -i "* ]] || printf 'HTTP/2.0 201 Created\r\nx-ratelimit-remaining: 4999\r\n\r\n' +# The merge-queue entry read and the dequeue (leave_queue): queued holds the +# entry, fail_dequeue refuses the dequeue. +if [[ $endpoint == graphql && " $* " == *mergeQueueEntry* ]]; then + echo queue-read >>"$STUB/queue" + entry=null + [[ -f $STUB/queued ]] && entry='{"id":"MQE_1"}' + printf '{"data":{"repository":{"pullRequest":{"id":"PR_42","mergeQueueEntry":%s}}}}\n' "$entry" + exit 0 +fi +if [[ $endpoint == graphql && " $* " == *dequeuePullRequest* ]]; then + echo "dequeue $*" >>"$STUB/queue" + [[ -f $STUB/fail_dequeue ]] && { echo 'stub gh: dequeue failed' >&2; exit 1; } + echo '{"data":{"dequeuePullRequest":{"clientMutationId":null}}}' + exit 0 +fi case "$method $endpoint" in "GET repos/o/r/issues/42") cat "$STUB/issue.json" ;; "GET repos/o/r/issues/42/comments?per_page=100") cat "$STUB/comments.json" ;; @@ -704,14 +719,53 @@ clear_case bogus '[]' check "clear: an activity the table does not route fails" 1 "$([[ $RC -ne 0 ]] && echo 1 || echo 0)" check "clear: an unknown activity makes no gh call" '' "$(cat "$STUB/calls")" -# Never touches the branch: no git, no contents API, no write outside the PR's issue. +# Never touches the branch: no git, no contents API, no write outside the PR's +# issue, beside the hold route's GraphQL queue read. for reason in feedback-question-design protected-path-conflict workflow-staged; do setup '[]' "$(comment ''$'\nold' 'melodic-automation-lanes[bot]' Bot 9)" run "$reason" check "$reason: never runs git" 0 "$(grep -c '^git' "$STUB/calls" || true)" check "$reason: never calls the contents API" 0 "$(grep -c 'contents/' "$STUB/calls" || true)" check "$reason: writes only the issue, its comments and labels" 0 \ - "$(grep -vE '^(GET|POST) repos/o/r/issues/42(/comments(\?per_page=100)?|/labels)?$|^PATCH repos/o/r/issues/comments/9$' "$STUB/requests" | grep -c . || true)" + "$(grep -vE '^(GET|POST) repos/o/r/issues/42(/comments(\?per_page=100)?|/labels)?$|^PATCH repos/o/r/issues/comments/9$|^GET graphql$' "$STUB/requests" | grep -c . || true)" done +# The hold route's human-gated label on a queued PR takes it out of the merge +# queue: one queue read per label add, and the dequeue only when it is queued. +# A refused dequeue warns and the escalation still posts. Expected values are +# the work item's: queued dequeues, unqueued reads once and writes nothing more. +NOTICE='::notice title=hold-label-dequeue::pr=42 label=needs-human outcome=' +setup '[]' '[]' +: >"$STUB/queued" +run feedback-question-design +check "queued hold: exits 0" 0 "$RC" +check "queued hold: one read, then the dequeue of its node id" $'queue-read\ndequeue api graphql -f query=mutation($id: ID!) { dequeuePullRequest(input: { id: $id }) { clientMutationId } } -f id=PR_42' "$(cat "$STUB/queue")" +has "queued hold: names the dequeue" "${NOTICE}dequeued" "$STUB/out" +check "queued hold: the label goes on before the queue read" "POST repos/o/r/issues/42/labels" "$(grep -B1 -m1 '^GET graphql$' "$STUB/requests" | head -1)" + +setup '[]' '[]' +run feedback-question-design +check "unqueued hold: exits 0" 0 "$RC" +check "unqueued hold: one read and no dequeue" queue-read "$(cat "$STUB/queue")" +has "unqueued hold: says it was not queued" "${NOTICE}not-queued" "$STUB/out" + +setup '[]' '[]' +: >"$STUB/queued" +: >"$STUB/fail_dequeue" +run feedback-question-design +check "refused dequeue: the escalation still exits 0" 0 "$RC" +has "refused dequeue: warns" "::warning::#42: could not dequeue it" "$STUB/out" +has "refused dequeue: names the refusal" "${NOTICE}refused:dequeue-failed" "$STUB/out" +check "refused dequeue: the escalation comment still posts" 1 "$(grep -c '^POST repos/o/r/issues/42/comments$' "$STUB/requests" || true)" + +# The label already on (a later reason on the same head): no add, no read. +setup '["needs-human"]' '[]' +: >"$STUB/queued" +run feedback-question-intent +check "label already on: no queue read" '' "$(cat "$STUB/queue" 2>/dev/null)" +# A fail route adds no human-gated label, so it reads no queue entry. +setup '[]' '[]' +run workflow-staged +check "fail route: no queue read" '' "$(cat "$STUB/queue" 2>/dev/null)" + exit "$FAILED" diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 76722356329..5c2c371e8a8 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -77,19 +77,15 @@ on: # A label came off. This run's completion starts pr-feedback-dispatch.yml, # the only thing that decides again a lanes App head the human gate skipped - # once needs-human comes off, so `unlabeled` stays. A label went on: adding - # one can only add a hold, so only a hold label starts work, and only the - # enable-auto-merge job's, which takes that one PR out of the merge queue - # (or disarms it) so its merge group does not run only for `ci-status` to - # fail it; any other label's run skips every job. No other PR event starts - # a run; the schedule picks up each change. A person's ready or rename - # restarts their PR's idle clock, so the gate would refuse it. An event run - # never arms a lane PR, and `ci-status` runs on a body edit and fails on a - # hold. pull_request_target runs this file as it is on main, not the PR's - # copy, so a PR cannot change what this lane does; the job below never - # checks out the PR's code. + # once needs-human comes off, so `unlabeled` stays. No other PR event starts + # a run; the schedule picks up each change. Adding a label can only add a + # hold. A person's ready or rename restarts their PR's idle clock, so the + # gate would refuse it. An event run never arms a lane PR, and `ci-status` + # runs on a body edit and fails on a hold. pull_request_target runs this + # file as it is on main, not the PR's copy, so a PR cannot change what this + # lane does; the job below never checks out the PR's code. pull_request_target: # zizmor: ignore[dangerous-triggers] - types: [labeled, unlabeled] + types: [unlabeled] # A person can start a run by hand (Actions tab, "Run workflow") to retry a # PR that looks stuck. Only a run on main does anything. @@ -864,15 +860,13 @@ jobs: # keep pushing real runs out of the queue, and a bot's event does not wait # in it only to stop at the who step (a bot is never a listed human). A # PR-scoped dispatch (inputs.pr) only arms, so it skips this job and with - # it dispatch-full-run and gate-check. A label added is the - # enable-auto-merge job's alone: it only adds a hold. + # it dispatch-full-run and gate-check. if: >- !cancelled() && github.ref == 'refs/heads/main' && github.event_name != 'push' && !inputs.pr && (github.event_name != 'pull_request_target' || (github.event.pull_request.head.repo.full_name == github.repository - && github.event.action != 'labeled' && github.event.sender.type != 'Bot')) # One run at a time, and a running one is never cancelled: a cancelled run # could stop between the gate's check and its merge-queue write. Extra runs @@ -2219,27 +2213,18 @@ jobs: # kind it arms, and only when a person sent the event (a bot is never a # listed human at the who step): any other event's job is skipped before # it joins the group, so its check reads skipped, never pending behind the - # queue. A label added wakes it for any author's pull request, but only a - # hold label: enable-auto-merge.mjs HOLD_LABELS and the tracker's - # human-gated label (workflow.test.mjs checks the list against both). - # GitHub's contains() compares strings in any case, as holdLabel does. + # queue. if: >- !cancelled() && github.ref == 'refs/heads/main' && github.event_name != 'push' && (github.event_name != 'pull_request_target' || (github.event.pull_request.head.repo.full_name == github.repository - && ((github.event.action == 'labeled' - && contains(fromJSON('["do-not-merge","pr-verify-failed","security: outside-actor","needs-human"]'), github.event.label.name)) - || (github.event.action != 'labeled' - && format('{0}', github.event.pull_request.user.id) == vars.AUTOMATION_LANES_APP_SENDER_ID)) + && format('{0}', github.event.pull_request.user.id) == vars.AUTOMATION_LANES_APP_SENDER_ID && github.event.sender.type != 'Bot')) # A pending run is never replaced: a cancelled run leaves a CANCELLED - # check on the pull request that woke it. A labeled run takes a group of - # its own, so a dequeue does not wait behind a scheduled run's verdict - # reads (up to its 18-minute arm step) while the merge group it would - # spare runs; it writes only that one pull request's queue entry and arm. + # check on the pull request that woke it. concurrency: - group: ${{ github.event.action == 'labeled' && 'pr-merge-hold-dequeue' || 'pr-merge-enable-auto-merge' }} + group: pr-merge-enable-auto-merge queue: max runs-on: ubuntu-24.04 # Above the sum of its steps' timeouts (41, workflow.test.mjs checks it), @@ -2331,12 +2316,11 @@ jobs: # scheduled or hand-started run arms, every run disarms on a hold), this # run ends green here with a notice before it takes the lane token or # reads anything else, so a backlog costs one pass and no run is - # cancelled. Any error leaves superseded unset and the job runs. A - # labeled run's dequeue is work no other run does, so it never skips. + # cancelled. Any error leaves superseded unset and the job runs. # Rules: .github/scripts/pr-merge/superseded.mjs. - name: Check for a newer run that does this work id: superseded - if: steps.who.outputs.proceed == 'true' && github.event.action != 'labeled' + if: steps.who.outputs.proceed == 'true' continue-on-error: true timeout-minutes: 2 env: @@ -2561,8 +2545,6 @@ jobs: # The event's pull request, or a PR-scoped dispatch's; empty on the # schedule and a hand-started sweep. EVENT_PR: ${{ github.event.pull_request.number || inputs.pr }} - EVENT_ACTION: ${{ github.event.action }} - EVENT_LABEL: ${{ github.event.label.name }} BABYSIT_VERIFY_GH_TOKEN: ${{ github.token }} BABYSIT_VERIFY_LISTING: ${{ runner.temp }}/verify-listing.json BABYSIT_VERIFY_LISTING_CARRY: ${{ runner.temp }}/verify-listing-carry.json From e500cf25dd4fc4099a927b26a10fa3318f6439ba Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 2 Nov 2026 22:13:38 -0500 Subject: [PATCH 3/3] test(pr-pipeline): cover the dequeue when the lane puts a refused PR's label back Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VcupomBPg7kuobwLoNpxEW --- .github/scripts/pr-merge/enable-auto-merge.test.mjs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 33dc4f1faad..419273fddae 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -4078,6 +4078,16 @@ test("a refused dequeue warns and leaves the label add standing", async () => { assert.ok(lines.includes(holdNotice("refused:dequeue-failed"))); }); +test("a refused pull request whose label the lane puts back while queued is dequeued", async () => { + const laneComment = lanesSaid(laneNote(MISSING_REASON)); + const posted = { ...lanesSaid(personNote(LANE_HOLD_NOTE), "2026-10-23T10:05:00Z"), databaseId: 78, createdAt: "2026-10-23T10:05:00Z" }; + const { code, notes, writes, lines } = await run([laneHeld(1, { discussion: { comments: [laneComment, posted] }, queued: true })]); + assert.equal(code, 0); + assert.deepEqual(notes, [{ method: "POST", path: `${ISSUES}/1/labels`, body: { labels: ["needs-human"] } }]); + assert.deepEqual(writes, [{ mutation: "dequeue", id: "PR_1" }]); + assert.ok(lines.includes(holdNotice("dequeued"))); +}); + test("a refusal that adds no label reads no queue entry", async () => { const { calls } = await run([pull(1, { verdict: MISSING })]); assert.deepEqual(queueReads(calls), []);