diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index fad05310ed..ac8150fc13 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -1397,6 +1397,66 @@ export async function readStatuses(github, repo, head) { throw new Error("more than 1000 statuses on the head"); } +// The tracker config's human-gated role label (.work-item-tracker.json +// config.role_labels["human-gated"], trimmed), `needs-human` when it names +// none, as ci-status reads it. +export function humanGatedLabel(tracker) { + const label = tracker?.config?.role_labels?.["human-gated"]; + return typeof label === "string" && label.trim() ? label.trim() : "needs-human"; +} + +// The hold labels on pull request `number`: `do-not-merge`, the tracker's +// human-gated label and `pr-verify-failed`, one reason each. `labels` are +// names or label objects; a list that is not an array throws. +export function labelHolds(labels, tracker, number) { + if (!Array.isArray(labels)) { + throw new Error("the labels are not a list"); + } + const names = new Set(labels.map((label) => (typeof label === "string" ? label : label?.name))); + const gated = humanGatedLabel(tracker); + return [ + names.has("do-not-merge") && `#${number} carries the 'do-not-merge' label; the merge queue removes it. Remove the label and re-queue to merge.`, + names.has(gated) && `#${number} carries '${gated}'; the merge queue removes it. Removing the label approves this pull request; re-queue it to merge.`, + names.has("pr-verify-failed") && + `#${number} carries 'pr-verify-failed': pr-verify failed its head, and a pass at a new commit removes the label. The merge queue removes it; re-queue it once the label is off.`, + ].filter(Boolean); +} + +// The lanes kill switch on a lanes App pull request: while CLAUDE_LANES_DISABLED +// or CLAUDE_LANES_REPO_DISABLED is anything but the exact value `false`, a pull +// request whose author is the Bot of numeric id `appId` holds. Any other +// author, or an `appId` that is not a numeric id, holds nothing here. +export function switchHold(pull, { appId, disabled, repoDisabled } = {}) { + const id = Number(appId); + if (!/^[1-9][0-9]*$/.test(String(appId)) || pull?.user?.type !== "Bot" || pull?.user?.id !== id) { + return null; + } + if (disabled === "false" && repoDisabled === "false") { + return null; + } + return `#${pull.number}: the lanes kill switch is on (CLAUDE_LANES_DISABLED or CLAUDE_LANES_REPO_DISABLED is not 'false'), so no lanes App pull request merges; it merges once both read 'false' and ci-status is re-run, which the lost-runs reconciler does when this is the run's only failure.`; +} + +// THE HOLDS THAT COST NO LANE WORK, for a queued pull request: its hold labels, +// its head's pr-unblock stops and the lanes kill switch, from the pull request +// as the pulls API returns it, its head's statuses newest first, the tracker +// config (or null) and the kill switch's { appId, disabled, repoDisabled }. +// select-tests runs this before the lanes; ci-status keeps judging each hold. +export function queueHolds({ pull, statuses, tracker = null, lanes = {} } = {}) { + const head = pull?.head?.sha; + if (!Number.isInteger(pull?.number) || typeof head !== "string") { + throw new Error("the pull request does not read"); + } + return [ + ...labelHolds(pull.labels, tracker, pull.number), + ...unblockStops(statuses).map( + ({ context, state, description }) => + `'${context}' is ${state} on ${head}: ${description}. The lane that stopped clears it by running again on this head, or a new head carries none.`, + ), + switchHold(pull, lanes), + ].filter(Boolean); +} + // Whether a person wrote the body of pull request `number` (personWrote); a // read that fails counts as a person. async function bodyByPerson(github, repo, number) { diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 7aae9f5a75..a8eee4efc2 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -1455,3 +1455,70 @@ test("Q10: a compare not read whole, not ahead, or with a dismissal the push did } assert.equal(carriesQ10(pushed(["docs/a.md"]), null), false, "unread hold paths"); }); + +// The queue holds select-tests checks before the lanes (#10747). Expected +// reasons are ci-status's own error texts for the same holds: its hold-label +// step's three label errors, its pr-unblock step's error and its lanes App +// step's kill-switch error, which reconcile.test.mjs keys on. +const QUEUED = "fedcba9876543210fedcba9876543210fedcba98"; +const queuedPull = (over = {}) => ({ number: 7, head: { sha: QUEUED }, labels: [], user: { id: 42, type: "User" }, ...over }); +const ON = { appId: "99", disabled: "false", repoDisabled: "false" }; + +test("queueHolds: a pull request with no hold label, stop or switch passes", () => { + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(), statuses: [], tracker: null, lanes: ON }), []); +}); + +test("queueHolds: each hold label holds with ci-status's text", () => { + for (const [label, want] of [ + ["do-not-merge", "#7 carries the 'do-not-merge' label; the merge queue removes it. Remove the label and re-queue to merge."], + ["needs-human", "#7 carries 'needs-human'; the merge queue removes it. Removing the label approves this pull request; re-queue it to merge."], + ["pr-verify-failed", "#7 carries 'pr-verify-failed': pr-verify failed its head, and a pass at a new commit removes the label. The merge queue removes it; re-queue it once the label is off."], + ]) { + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull({ labels: [{ name: "bug" }, { name: label }] }), statuses: [] }), [want], label); + } + assert.equal(holdsModule.queueHolds({ pull: queuedPull({ labels: [{ name: "do-not-merge" }, { name: "needs-human" }] }), statuses: [] }).length, 2); +}); + +test("queueHolds: the human-gated label is the tracker's, trimmed, else needs-human", () => { + const tracker = { config: { role_labels: { "human-gated": " hitl " } } }; + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull({ labels: [{ name: "needs-human" }] }), statuses: [], tracker }), []); + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull({ labels: [{ name: "hitl" }] }), statuses: [], tracker }), [ + "#7 carries 'hitl'; the merge queue removes it. Removing the label approves this pull request; re-queue it to merge.", + ]); + for (const blank of [{ config: { role_labels: { "human-gated": " " } } }, { config: {} }, [], "x", null]) { + assert.equal(holdsModule.humanGatedLabel(blank), "needs-human", JSON.stringify(blank)); + } +}); + +test("queueHolds: a pr-unblock stop on the head holds; a newer success or another account's status does not", () => { + const lane = { login: "github-actions[bot]" }; + const failure = { context: "pr-unblock/fix-ci", state: "failure", creator: lane, description: "The lane stopped" }; + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(), statuses: [failure] }), [ + `'pr-unblock/fix-ci' is failure on ${QUEUED}: The lane stopped. The lane that stopped clears it by running again on this head, or a new head carries none.`, + ]); + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(), statuses: [{ ...failure, state: "success" }, failure] }), []); + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(), statuses: [{ ...failure, creator: { login: "someone" } }] }), []); +}); + +test("queueHolds: the kill switch holds only the lanes App's pull request, until both read false", () => { + const app = { user: { id: 99, type: "Bot" } }; + const text = + "#7: the lanes kill switch is on (CLAUDE_LANES_DISABLED or CLAUDE_LANES_REPO_DISABLED is not 'false'), so no lanes App pull request merges; it merges once both read 'false' and ci-status is re-run, which the lost-runs reconciler does when this is the run's only failure."; + for (const [name, lanes] of [ + ["on", { ...ON, disabled: "true" }], + ["repo on", { ...ON, repoDisabled: "1" }], + ["unset", { appId: "99" }], + ]) { + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(app), statuses: [], lanes }), [text], name); + } + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(app), statuses: [], lanes: ON }), []); + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(), statuses: [], lanes: { appId: "99", disabled: "true" } }), [], "a person's"); + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull({ user: { id: 99, type: "User" } }), statuses: [], lanes: { appId: "99" } }), [], "a User with the id"); + assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull(app), statuses: [], lanes: { appId: "", disabled: "true" } }), [], "no app id"); +}); + +test("queueHolds: a pull request, label list or status list that does not read throws", () => { + assert.throws(() => holdsModule.queueHolds({ pull: { number: 7, head: {} }, statuses: [] }), /does not read/); + assert.throws(() => holdsModule.queueHolds({ pull: queuedPull({ labels: null }), statuses: [] }), /labels are not a list/); + assert.throws(() => holdsModule.queueHolds({ pull: queuedPull(), statuses: {} }), /not a list/); +}); diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 02782f4aeb..dbd5aa0a4a 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -283,10 +283,14 @@ jobs: timeout-minutes: 3 # The detect-changes action lists the pull request's files through the # API. Nothing else in this workflow needs that read, so it is granted here - # and nowhere else. No write scope: this job runs pull request code. + # and nowhere else. The queue-hold step reads the queued pull request and + # its head's commit statuses ("Commit statuses" read, per + # https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps). + # No write scope: this job runs pull request code. permissions: contents: read pull-requests: read + statuses: read outputs: run_full: ${{ steps.detect.outputs.docs_only != 'true' }} run_tests: ${{ steps.detect.outputs.docs_only != 'true' && github.event.pull_request.draft != true }} @@ -320,6 +324,85 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + # A QUEUED PULL REQUEST A HOLD STOPS FAILS HERE, BEFORE THE LANES. In the + # merge queue the holds that cost no lane work, its hold labels, a + # pr-unblock stop on its head and the lanes kill switch, are judged by + # the base commit's merge-holds.mjs queueHolds: a held group fails this + # job in seconds, and every lane, which needs it, skips. ci-status still + # judges each hold itself, from its own reads, so the aggregate fails on + # the hold and on this job, and a skipped lane stays skipped there (this + # job did not succeed). Nothing here feeds a ci-status hold: this job runs + # pull request code. Only a hold read fails it: a read that fails, a ref + # it cannot parse, or a base module without queueHolds leaves the hold to + # ci-status, which retries and fails closed. Two reads, the pull request + # and its head's statuses (one per 100), through the api-budget gh + # wrapper, whose api-calls notice the step ends with. The base's + # module, tracker config and wrapper come from one shallow fetch of the + # group's base commit, which leaves `ok=true` only when all of it read; + # it never fails the job, so a failed fetch only leaves the holds to + # ci-status. The runner's Node runs the module: queueHolds needs no + # API newer than the image's. Pinned by + # scripts/ci-early-queue-holds.test.sh, which runs this body. + - name: Read the base commit's queue-hold rules + id: queue_rules + if: github.event_name == 'merge_group' + env: + BASE_SHA: ${{ github.event.merge_group.base_sha }} + run: | + dir="$RUNNER_TEMP/queue-rules" + if [[ $BASE_SHA =~ ^[0-9a-f]{40}$ ]] && git fetch --no-tags --depth=1 origin "$BASE_SHA" && mkdir -p "$dir" && + git show "$BASE_SHA:.github/scripts/pr-merge/merge-holds.mjs" >"$dir/merge-holds.mjs" && + git show "$BASE_SHA:.github/actions/log-api-budget/api-budget.bash" >"$dir/api-budget.bash"; then + git show "$BASE_SHA:.work-item-tracker.json" >"$dir/tracker.json" 2>/dev/null || rm -f "$dir/tracker.json" + echo "ok=true" >>"$GITHUB_OUTPUT" + else + echo "::warning::the base commit '$BASE_SHA' could not be read; ci-status judges the holds." + fi + - name: Fail a queued pull request a hold stops, before the lanes + if: github.event_name == 'merge_group' + env: + GH_TOKEN: ${{ github.token }} + RULES: ${{ steps.queue_rules.outputs.ok == 'true' && format('{0}/queue-rules', runner.temp) || '' }} + LANES_APP_SENDER_ID: ${{ vars.AUTOMATION_LANES_APP_SENDER_ID }} + LANES_DISABLED: ${{ vars.CLAUDE_LANES_DISABLED }} + LANES_REPO_DISABLED: ${{ vars.CLAUDE_LANES_REPO_DISABLED }} + run: | + if [ -z "$RULES" ] || [ ! -s "$RULES/merge-holds.mjs" ]; then + echo "::notice::the base commit's hold rules did not read; ci-status judges the holds." + exit 0 + fi + # shellcheck source=/dev/null + if . "$RULES/api-budget.bash"; then + api_budget_wrap_gh github-token + trap 'api_budget_emit github-token; api_budget_emit_calls github-token' EXIT + fi + leave() { echo "::warning::$1; ci-status judges the holds."; exit 0; } + pr=$(sed -nE 's#^(refs/heads/)?gh-readonly-queue/.+/pr-([0-9]+)-[0-9a-f]{40}$#\2#p' <<<"$GITHUB_REF") + [ -n "$pr" ] || leave "cannot read a pull request number from the merge-group ref '$GITHUB_REF'" + gh api "repos/$GITHUB_REPOSITORY/pulls/$pr" >"$RUNNER_TEMP/queued-pull.json" || leave "could not read #$pr" + head=$(jq -r '.head.sha // ""' "$RUNNER_TEMP/queued-pull.json") || head= + [[ $head =~ ^[0-9a-f]{40}$ ]] || leave "the head of #$pr is not a commit SHA" + rows=$(gh api "repos/$GITHUB_REPOSITORY/commits/$head/statuses?per_page=100" --paginate --jq '.[]') || leave "could not read the statuses of $head" + tracker=null + [ ! -s "$RULES/tracker.json" ] || tracker=$(cat "$RULES/tracker.json") + jq -n --slurpfile pull "$RUNNER_TEMP/queued-pull.json" --argjson tracker "$tracker" --slurpfile statuses <(printf '%s\n' "$rows") \ + --arg appId "${LANES_APP_SENDER_ID:-}" --arg disabled "${LANES_DISABLED:-}" --arg repoDisabled "${LANES_REPO_DISABLED:-}" \ + '{pull: $pull[0], statuses: $statuses, tracker: $tracker, lanes: {appId: $appId, disabled: $disabled, repoDisabled: $repoDisabled}}' \ + >"$RUNNER_TEMP/queue-holds.json" || leave "the reads of #$pr do not parse" + # shellcheck disable=SC2016 # JavaScript, not shell + reasons=$(node --input-type=module -e 'import { readFileSync } from "node:fs"; + import { pathToFileURL } from "node:url"; + const { queueHolds } = await import(pathToFileURL(process.argv[2]).href); + for (const reason of queueHolds(JSON.parse(readFileSync(process.argv[1], "utf8")))) console.log(reason.replaceAll(/[\r\n]/g, " "));' \ + "$RUNNER_TEMP/queue-holds.json" "$RULES/merge-holds.mjs") || leave "the base commit's merge-holds.mjs could not judge #$pr" + if [ -z "$reasons" ]; then + echo "#$pr: no hold label, pr-unblock stop or kill switch holds it before the lanes." + exit 0 + fi + while IFS= read -r reason; do + echo "::error::${reason//%/%25} The lanes do not run for this group." + done <<<"$reasons" + exit 1 - name: Fetch base uses: ./.github/actions/download-full-history # zizmor: ignore[self-repository] # THE DIFF BASE, RESOLVED ONCE. A pull request diffs against its base @@ -3837,13 +3920,56 @@ jobs: waited=1 done } + # ONE READ OF THE PULL, ITS LABELS AND ITS EVENTS PER JOB. pr_read + # ... answers what an earlier step of this job read for + # the same args, else reads through gh_read and keeps the answer in + # $RUNNER_TEMP/pr-reads. Steps use it for three reads only: the pull + # request (pulls/), its labels and its issue events. Comments, + # reviews, runs, jobs and annotations are always read live with + # gh_read, so a member's veto or a new review is never missed. A read + # that fails keeps nothing, so the next step reads again and fails + # closed as before. pr_read_put ... keeps an answer a + # step read its own way; pr_read_forget drops every kept answer, and + # every step that writes to the pull request runs it after its write + # (the cost-hold step and the lanes App step's auto-merge disarm). + # check-contract writes before any step reads through this cache. + pr_read() { + local file out + file=$(pr_read_file "$@") || file= + if [ -n "$file" ] && [ -f "$file" ]; then + cat "$file" + return 0 + fi + out=$(gh_read "$@") || return 1 + [ -z "$file" ] || pr_read_put "$out" "$@" + [ -z "$out" ] || printf '%s\n' "$out" + } + pr_read_file() { + local dir=${RUNNER_TEMP:-}/pr-reads + if [ -z "${RUNNER_TEMP:-}" ] || ! mkdir -p "$dir" 2>/dev/null; then + return 1 + fi + printf '%s/%s\n' "$dir" "$(printf '%s\0' "$@" | sha256sum | cut -c1-40)" + } + pr_read_put() { + local out=$1 file + shift + file=$(pr_read_file "$@") || return 0 + if ! { [ -z "$out" ] || printf '%s\n' "$out"; } >"$file.$$" 2>/dev/null || ! mv -f "$file.$$" "$file" 2>/dev/null; then + rm -f "$file.$$" + fi + return 0 + } + pr_read_forget() { + [ -z "${RUNNER_TEMP:-}" ] || rm -rf "$RUNNER_TEMP/pr-reads" + } echo $(($(date +%s) + GH_READ_JOB_SECONDS - GH_READ_MARGIN_SECONDS)) >"$RUNNER_TEMP/gh-read.deadline" # fd 9 is each sourcing step's stderr, so the notices reach the log # when a caller captures or redirects gh_read's own stderr. # The api-budget gh wrapper too: every `gh api` of a sourcing step, # gh_read's included, leaves its rate-limit reading for the notice # the lane-scope step writes. - { echo 'exec 9>&2'; declare -f gh_read; printf '%s\n' "${API_BUDGET_SH:-}" '! declare -F api_budget_wrap_gh >/dev/null || api_budget_wrap_gh github-token'; } >"$RUNNER_TEMP/gh-read.sh" + { echo 'exec 9>&2'; declare -f gh_read pr_read pr_read_file pr_read_put pr_read_forget; printf '%s\n' "${API_BUDGET_SH:-}" '! declare -F api_budget_wrap_gh >/dev/null || api_budget_wrap_gh github-token'; } >"$RUNNER_TEMP/gh-read.sh" - name: Check the pull-request contract uses: melodic-software/ci-workflows/.github/actions/pr-require-checks/check-contract@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 with: @@ -4103,16 +4229,19 @@ jobs: HEAD_SHA: ${{ github.event.pull_request.head.sha }} BASE_CONFIG: ${{ steps.base_config.outcome == 'success' && format('{0}/base-config', github.workspace) || '' }} run: | - [ -z "${API_BUDGET_SH:-}" ] || { eval "$API_BUDGET_SH"; api_budget_wrap_gh github-token; } + . "$RUNNER_TEMP/gh-read.sh" keep() { echo "The cost hold stays for a person: $1." exit 0 } + # Every read is live, as this step acts on it; its pull and events + # answers are kept for the later steps' pr_read of the same args. read_api() { local attempt out for attempt in 1 2 3; do # gh-read-exempt: an unreadable input keeps the hold, and a wait would act on stale reads. if out=$(gh api "$@" 2>"$RUNNER_TEMP/cost-hold.err"); then + [[ ! $1 =~ ^repos/[^/]+/[^/]+/(pulls/[0-9]+|issues/[0-9]+/events\?per_page=100)$ ]] || pr_read_put "$out" "$@" printf '%s\n' "$out" return 0 fi @@ -4365,6 +4494,8 @@ jobs: # shellcheck disable=SC2016 # $l is a jq variable gh api -X DELETE "repos/$repo/issues/$PR_NUMBER/labels/$(jq -rn --arg l "$label" '$l | @uri')" >/dev/null || echo "::warning::could not remove '$label' from #$PR_NUMBER; the step below holds it." + # The labels, comments and events changed: the steps below read them live. + pr_read_forget # A PULL REQUEST CARRYING THE HUMAN-GATED LABEL IS HELD. A review lane or # an escalation puts the label on so a person decides; removing it is the # approval, and the `unlabeled` event re-runs this job. The label's name @@ -4412,7 +4543,7 @@ jobs: fi echo "label=$human_gated" >>"$GITHUB_OUTPUT" for attempt in 1 2 3; do - if labels=$(gh_read "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/labels?per_page=100" --paginate --jq '.[].name'); then + if raw=$(pr_read "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/labels?per_page=100" --paginate) && labels=$(jq -r '.[].name' <<<"$raw"); then break fi [ "$attempt" -lt 3 ] || { echo "::error::could not read the labels of #$PR_NUMBER; re-run this run."; exit 1; } @@ -4427,9 +4558,9 @@ jobs: # hold comments name a reason. identity=$(yq -er '.merge."agent-identity"' "$BASE_CONFIG/docs/conventions/pr-pipeline.yaml" 2>/dev/null) || identity= [[ $identity =~ ^[a-z0-9][a-z0-9-]*\[bot\]$ ]] || identity= - # gh-read-exempt: the reason is best effort; the step fails either way. - reason=$(gh api "repos/$GITHUB_REPOSITORY/issues/$PR_NUMBER/comments?per_page=100" --paginate \ - --jq '.[] | select(.user.type == "Bot" and (((.body | test("(^|\n)