Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion .github/scripts/pr-merge/enable-auto-merge.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Comment thread
kyle-sexton marked this conversation as resolved.
Resolved
console.log(`enable-auto-merge: #${pull.number} labeled ${writes.label} again: its refusal comment stands and no person removed the label`);
return;
}
Expand All @@ -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) {
Expand Down
62 changes: 59 additions & 3 deletions .github/scripts/pr-merge/enable-auto-merge.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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]]));
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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 };
}

Expand Down Expand Up @@ -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), []);
});
41 changes: 39 additions & 2 deletions .github/scripts/pr-unblock/address-feedback.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 } }
}`;
Expand Down Expand Up @@ -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 });
Expand Down
35 changes: 33 additions & 2 deletions .github/scripts/pr-unblock/report-escalation.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
58 changes: 56 additions & 2 deletions .github/scripts/pr-unblock/report-escalation.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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" ;;
Expand Down Expand Up @@ -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 '<!-- pr-unblock:lane-stopped activity=fix-ci -->'$'\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"
Loading