diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index c66376f0b7c..b7373ce89dc 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -211,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, leaveQueueOnHold, QUEUE_ENTRY_QUERY, quote } from "../pr-unblock/address-feedback.mjs"; import { lines as bodyLines, foreignMarker, @@ -1844,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; @@ -1902,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; } @@ -1910,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 58dd2a0fbc1..419273fddae 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -67,6 +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, 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"; @@ -140,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]])); @@ -308,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(); } @@ -346,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 }; } @@ -4036,3 +4045,50 @@ 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); } }); + +// 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"))); +}); + +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 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(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("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), []); +}); diff --git a/.github/scripts/pr-unblock/address-feedback.mjs b/.github/scripts/pr-unblock/address-feedback.mjs index ee31a1c3023..d4cfc02dbc2 100644 --- a/.github/scripts/pr-unblock/address-feedback.mjs +++ b/.github/scripts/pr-unblock/address-feedback.mjs @@ -1224,9 +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 } } }`; @@ -1445,7 +1482,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/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"