From a7e0c3071f131b2918295bd25b7e4baa6e25880b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 09:35:41 -0400 Subject: [PATCH 1/7] fix(ci): clear a cost-only review hold the owner already decided A review lane re-applies its cost hold on every new head, so needs-human came back after the owner recorded the cost decision. ci-status now clears a hold whose every lane hold since a trusted person's last removal is cost-only when the body cites the owner's unedited `**Decision recorded.** Cost:` comment, the rule babysit's cost gate reads. It comments citing the decision, removes the label, and the hold-lift step skips. trust-gate and critical-security holds are unchanged. Closes #9403 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pr-require-checks.yml | 152 ++++++++++++- scripts/ci-hold-cost-decision.test.sh | 274 ++++++++++++++++++++++++ scripts/ci-hold-lifted-by.test.sh | 4 +- 3 files changed, 423 insertions(+), 7 deletions(-) create mode 100755 scripts/ci-hold-cost-decision.test.sh diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 0c0ee8ddc0..acf3d595c4 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3052,9 +3052,9 @@ jobs: # (`rate-limit-wait-seconds` on check-contract and aggregate-results), 45 s # of own-gate retries, 7 s of status-write retries and the 90 s # `rerun-wait-seconds` wait, about four and a half minutes, plus up to - # 15 s of retry sleeps for each of the hold-lift step's three reads and - # the review-lane step's four, under two minutes. - timeout-minutes: 7 + # 15 s of retry sleeps for each of the hold-lift step's three reads, the + # review-lane step's four and the cost-hold step's nine, under four minutes. + timeout-minutes: 9 permissions: # `yield-to-full-run` lists this workflow's runs on the head SHA, and # `rerun-contract-only-siblings` re-runs the red contract-only ones once @@ -3192,6 +3192,147 @@ jobs: msg="draft: lanes not run; the ready_for_review run lints and tests this SHA" echo "::notice::$msg" echo "$msg" >>"$GITHUB_STEP_SUMMARY" + # A COST HOLD THE OWNER ALREADY DECIDED COMES OFF. A review lane re-holds + # every new head, so its `cost` hold went back on after the owner had + # recorded the cost decision (#9403). This step clears a hold only when all + # of these hold, else it changes nothing and the two steps below judge + # the label as before: + # - The window opens at the newest removal of the human-gated label by a + # `human` of the base commit's trusted-actor list (ever, when none). + # - The label was added in it, and only by github-actions[bot] or + # claude[bot], each time followed within five minutes by a cost hold. + # - Every review-lane hold comment in it (as the step below reads them) + # is github-actions[bot]'s with a first line of exactly + # ``, and + # no other bot wrote a marked comment in it (a lanes App escalation + # adds no label event to a label already on). + # - The pull request body, outside HTML comments, links a comment that + # babysit_merge_policy.py's cost_decision would accept: on the one + # closing issue in this repository (written after the pull request) or + # on the pull request itself, by a User on the trusted-actor list, not + # through an App, never edited, first line `**Decision recorded.** Cost:`. + # It then outputs cleared=true and the decision link, and, with the label + # on, posts one comment citing the decision and removes the label. A + # GITHUB_TOKEN removal starts no run, so the steps below read the label + # off in this run, and the hold-lift step skips on cleared=true. + # Unreadable input clears nothing; the step itself never fails the job. + # Pinned by scripts/ci-hold-cost-decision.test.sh, which runs this body. + - name: Clear a cost-only review hold the owner decided + id: cost_hold + if: ${{ !cancelled() && github.event_name == 'pull_request' }} + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + keep() { + echo "The cost hold stays for a person: $1." + exit 0 + } + read_api() { + local attempt out + for attempt in 1 2 3; do + if out=$(gh api "$@" 2>"$RUNNER_TEMP/cost-hold.err"); then + printf '%s\n' "$out" + return 0 + fi + ! grep -q 'HTTP 404' "$RUNNER_TEMP/cost-hold.err" || return 2 + [ "$attempt" -lt 3 ] || return 1 + sleep $((attempt * 5)) + done + } + [[ $BASE_SHA =~ ^[0-9a-f]{40}$ && $HEAD_SHA =~ ^[0-9a-f]{40}$ && $PR_NUMBER =~ ^[1-9][0-9]*$ ]] || keep "the event names no commit or pull request" + repo=$GITHUB_REPOSITORY + tracker=$(read_api -H 'Accept: application/vnd.github.raw+json' "repos/$repo/contents/.work-item-tracker.json?ref=$BASE_SHA") && rc=0 || rc=$? + [ "$rc" -ne 1 ] || keep "the tracker config is unreadable" + label=needs-human + if [ "$rc" -eq 0 ] && [ -n "$tracker" ]; then + label=$(jq -er '[objects | .config | objects | .role_labels | objects + | .["human-gated"] | strings | gsub("^\\s+|\\s+$"; "")][0] // "" + | if . == "" then "needs-human" else . end' <<<"$tracker") || keep "the tracker config is invalid" + fi + actors=$(read_api -H 'Accept: application/vnd.github.raw+json' "repos/$repo/contents/.github/standards/trusted-actors/trusted-actors.json?ref=$BASE_SHA") || keep "the trusted-actor list is unreadable" + humans=$(jq -ec '[.actors[] | select(.kind == "human") | .id | numbers] | select(length > 0)' <<<"$actors") || keep "the trusted-actor list names no person" + events=$(read_api "repos/$repo/issues/$PR_NUMBER/events?per_page=100" --paginate) || keep "the events are unreadable" + comments=$(read_api "repos/$repo/issues/$PR_NUMBER/comments?per_page=100" --paginate) || keep "the comments are unreadable" + # The window, then whether only cost holds put the label on in it. + # shellcheck disable=SC2016 # the $ in the jq filters are jq variables and regex anchors + verdict=$(jq -nr --arg label "$label" --argjson humans "$humans" --slurpfile events <(printf '%s\n' "$events") \ + --slurpfile comments <(printf '%s\n' "$comments") ' + def t: fromdateiso8601; + def lane: .user.type == "Bot" and (.body | test("(^|\n)$")); + ([$events[][] | select(.label.name == $label)]) as $ev + | ([$ev[] | select(.event == "unlabeled" and (.actor.id as $id | $humans | index($id))) + | .created_at | t] | max // -1) as $since + | ([$ev[] | select(.event == "labeled" and (.created_at | t) > $since)]) as $added + | ([$comments[][] | select((.created_at | t) > $since)]) as $window + | ([$window[] | select(lane)]) as $holds + | if ($added | length) == 0 then "no label was added since a trusted person last removed it" + elif any($added[]; .actor.login != "github-actions[bot]" and .actor.login != "claude[bot]") then "someone other than a review lane added the label" + elif ($holds | length) == 0 then "no review-lane hold was written since a trusted person last removed the label" + elif any($holds[]; cost | not) then "a review-lane hold since then is not cost-only" + elif any($window[]; .user.type == "Bot" and .user.login != "github-actions[bot]" + and .user.login != "claude[bot]" and (.body | contains("|\\z)"; "") + | match("https://github\\.com/([A-Za-z0-9-]+)/([A-Za-z0-9._-]+)/(?:issues|pull)/([0-9]+)#issuecomment-([0-9]+)"; "g") + | [.string, (.captures[] | .string)] | @tsv' <<<"$pr") || links= + decision= decision_id= + while IFS=$'\t' read -r link owner name number id; do + [ -n "$link" ] || continue + [ "$(tr '[:upper:]' '[:lower:]' <<<"$owner/$name")" = "$(tr '[:upper:]' '[:lower:]' <<<"$repo")" ] || continue + [ "$number" = "$PR_NUMBER" ] || [ "$number" = "$issue" ] || continue + comment=$(read_api "repos/$repo/issues/comments/$id") || continue + jq -e --arg number "$number" --arg pr "$PR_NUMBER" --argjson humans "$humans" --argjson opened "$opened" ' + (.issue_url // "" | endswith("/issues/\($number)")) + and .user.type == "User" and .performed_via_github_app == null + and (.user.id as $id | $humans | index($id) != null) + and (.body // "" | sub("\\A\\s+"; "") | test("\\A\\*\\*Decision recorded\\.\\*\\*[ \\t]+Cost:")) + and ($number == $pr or (.created_at | fromdateiso8601) > $opened)' <<<"$comment" >/dev/null 2>&1 || continue + node=$(jq -r .node_id <<<"$comment") + edited=$(read_api graphql -f query='query($id:ID!){node(id:$id){... on IssueComment{lastEditedAt}}}' -F id="$node") || continue + jq -e '.data.node | objects | has("lastEditedAt") and .lastEditedAt == null' <<<"$edited" >/dev/null 2>&1 || continue + decision=$link decision_id=$id + break + done <<<"$links" + [ -n "$decision" ] || keep "the body cites no recorded owner cost decision covering it" + { + echo "cleared=true" + echo "decision=$decision" + } >>"$GITHUB_OUTPUT" + echo "::notice::the review-lane hold on #$PR_NUMBER is cost-only, and $decision records the owner's cost decision." + labels=$(read_api "repos/$repo/issues/$PR_NUMBER/labels?per_page=100" --paginate --jq '.[].name') || { + echo "::warning::could not read the labels of #$PR_NUMBER, so '$label' was not removed." + exit 0 + } + grep -qxF -- "$label" <<<"$labels" || exit 0 + marker="" + if ! jq -se --arg marker "$marker" 'any(.[][]; .user.login == "github-actions[bot]" and (.body | startswith($marker)))' \ + <<<"$comments" >/dev/null; then + gh api -X POST "repos/$repo/issues/$PR_NUMBER/comments" -f body="$marker + ci-status removed \`$label\`: every review-lane hold since a trusted person last removed it is cost-only, and the owner recorded the cost decision in $decision. A person adding \`$label\` back holds it again." >/dev/null || + echo "::warning::could not post the removal comment on #$PR_NUMBER." + fi + 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." # 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 @@ -3278,10 +3419,11 @@ jobs: # later: an event cannot be deleted or edited, so removing the comment # does not remove the hold, and a review re-run that labels the same head # again starts a new hold. Unreadable comments, events or actor list fail - # closed. + # closed. It skips when the cost_hold step cleared a cost-only hold, whose + # label that step (or anyone) may have removed. # Pinned by scripts/ci-hold-lifted-by.test.sh, which runs this body. - name: Fail a review-lane hold an untrusted account lifted - if: ${{ !cancelled() && github.event_name == 'pull_request' && steps.human_gated.outputs.held == 'false' }} + if: ${{ !cancelled() && github.event_name == 'pull_request' && steps.human_gated.outputs.held == 'false' && steps.cost_hold.outputs.cleared != 'true' }} env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh new file mode 100755 index 0000000000..0e7bd780d1 --- /dev/null +++ b/scripts/ci-hold-cost-decision.test.sh @@ -0,0 +1,274 @@ +#!/usr/bin/env bash +# Runs the `Clear a cost-only review hold the owner decided` step body of +# .github/workflows/pr-require-checks.yml's ci-status under the shell Actions +# gives a step with no `shell:` (bash -e), against stub `gh` and `sleep`. +# +# A review lane's `cost` hold is cleared when every lane hold since a trusted +# person last removed the human-gated label is cost-only, and the pull request +# body cites the owner's unedited `**Decision recorded.** Cost:` comment on its +# closing issue (newer than the pull request) or on the pull request itself. +# The step then outputs cleared=true and the decision, and takes the label off +# with one comment citing it. Anything else, an unreadable read included, +# clears nothing and leaves the hold to the steps after it. +# test-scope: .github/workflows/pr-require-checks.yml +set -uo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +WORKFLOW="$ROOT/.github/workflows/pr-require-checks.yml" + +# shellcheck source=lib/test-harness.sh +. "$ROOT/scripts/lib/test-harness.sh" + +TMP_ROOT="$(mktemp -d)" +trap 'rm -rf "$TMP_ROOT"' EXIT + +NAME='Clear a cost-only review hold the owner decided' +step="$(awk -v name=" - name: $NAME" '$0 == name { on = 1; print; next } + on && /^ - / { exit } + on { print }' "$WORKFLOW")" +awk '/^ run: \|$/ { on = 1; next } + on && /^ / { print substr($0, 11); next } + on && /^[[:blank:]]*$/ { print ""; next } + on { exit }' <<<"$step" >"$TMP_ROOT/body.sh" + +if [[ ! -s "$TMP_ROOT/body.sh" ]]; then + fail "no '$NAME' step with a run: | body in $WORKFLOW" + test_harness::report + exit 1 +fi +if [[ "$step" == *"if: \${{ !cancelled() && github.event_name == 'pull_request' }}"* ]]; then + ok "step runs on every pull_request run that is not cancelled" +else + fail "step lost its pull_request condition" +fi +if [[ "$step" == *$'\n id: cost_hold\n'* ]]; then + ok "step carries the id the hold-lift step reads its outputs by" +else + fail "step lost its cost_hold id" +fi +# shellcheck disable=SC2016 # the ${{ }} text is workflow source, not shell +for env in 'BASE_SHA: ${{ github.event.pull_request.base.sha }}' 'HEAD_SHA: ${{ github.event.pull_request.head.sha }}'; do + if [[ "$step" == *"$env"* ]]; then ok "step env carries $env"; else fail "step env lacks $env"; fi +done +order="$(grep -nE "^ ci-status:$|^ - name: ($NAME|Fail a pull request carrying the human-gated label|Fail a review-lane hold an untrusted account lifted)$" "$WORKFLOW" | cut -d: -f1 | paste -sd' ')" +read -r job_line cost_line gated_line lift_line <<<"$order" +if [[ -n "${lift_line:-}" && "$job_line" -lt "$cost_line" && "$cost_line" -lt "$gated_line" && "$gated_line" -lt "$lift_line" ]]; then + ok "step sits in ci-status before the human-gated and hold-lift steps" +else + fail "step is not inside ci-status before the human-gated step (lines: $order)" +fi +lift="$(awk '$0 == " - name: Fail a review-lane hold an untrusted account lifted" { on = 1; next } on && /^ if: / { print; exit }' "$WORKFLOW")" +if [[ "$lift" == *"steps.cost_hold.outputs.cleared != 'true'"* ]]; then + ok "the hold-lift step does not run once a recorded decision clears the hold" +else + fail "the hold-lift step does not read cost_hold's cleared output: '$lift'" +fi + +# The stub logs its arguments and answers from STUB_* variables; a read whose +# name is in STUB_FAIL fails. +mkdir -p "$TMP_ROOT/bin" +cat >"$TMP_ROOT/bin/gh" <<'EOF' +#!/usr/bin/env bash +printf '%s\n' "$*" >>"$STUB_LOG" +failing() { [[ " ${STUB_FAIL:-} " == *" $1 "* ]] && exit 1; return 0; } +case "$*" in +*/contents/.work-item-tracker.json*) + failing tracker + [[ -n "${STUB_TRACKER+set}" ]] || { echo 'gh: Not Found (HTTP 404)' >&2; exit 1; } + printf '%s\n' "$STUB_TRACKER" + ;; +*/contents/.github/standards/trusted-actors/*) failing actors; printf '%s\n' "$STUB_ACTORS" ;; +*'-X DELETE'*/labels/*) failing delete ;; +*/issues/6419/labels*) failing labels; [[ -z "$STUB_LABELS" ]] || tr ',' '\n' <<<"$STUB_LABELS" ;; +*/issues/6419/events*) failing events; printf '%s\n' "$STUB_EVENTS" ;; +*'-X POST'*/issues/6419/comments*) failing post ;; +*/issues/6419/comments*) failing comments; printf '%s\n' "$STUB_COMMENTS" ;; +*/issues/comments/*) + failing decision + id="${*##*/issues/comments/}" + jq -ce --arg id "${id##* }" '.[$id] // error' <<<"$STUB_DECISIONS" || { echo 'gh: Not Found (HTTP 404)' >&2; exit 1; } + ;; +*/pulls/6419*) failing pull; printf '%s\n' "$STUB_PR" ;; +*graphql*closingIssuesReferences*) failing closing; printf '%s\n' "$STUB_CLOSING" ;; +*graphql*lastEditedAt*) failing edited; printf '%s\n' "$STUB_EDITED" ;; +*) exit 1 ;; +esac +EOF +printf '#!/usr/bin/env bash\n' >"$TMP_ROOT/bin/sleep" +chmod +x "$TMP_ROOT/bin/gh" "$TMP_ROOT/bin/sleep" + +BASE=89abcdef0123456789abcdef0123456789abcdef +HEAD=0123456789abcdef0123456789abcdef01234567 +OWNER_ID=153232337 +LINK='https://github.com/o/r/issues/9306#issuecomment-111' +LANE='{"login":"github-actions[bot]","type":"Bot"}' +CLAUDE='{"login":"claude[bot]","type":"Bot"}' +APP='{"login":"melodic-automation-lanes[bot]","type":"Bot"}' +OWNER="{\"login\":\"owner\",\"id\":$OWNER_ID,\"type\":\"User\"}" +STRANGER='{"login":"someone","id":42,"type":"User"}' + +comment() { jq -cn --argjson user "$1" --arg at "$2" --arg body "$3" '{user: $user, created_at: $at, body: $body}'; } +hold() { + comment "$LANE" "$1" " +**${3:-claude-review}** holds this pull request for a person ($2) at \`$HEAD\`. + +A reason. + +claude-lane: ${3:-claude-review}" +} +event() { jq -cn --arg e "$1" --argjson actor "$2" --arg at "$3" '{event: $e, actor: $actor, created_at: $at, label: {name: "needs-human"}}'; } +decision() { + jq -cn --arg url "https://api.github.com/repos/o/r/issues/$1" --argjson user "$2" --arg at "$3" --arg body "$4" \ + '{issue_url: $url, user: $user, performed_via_github_app: null, created_at: $at, body: $body, node_id: "IC_1"}' +} + +reset() { + export STUB_FAIL='' STUB_LABELS='needs-human' + export STUB_ACTORS="{\"actors\":[{\"kind\":\"human\",\"id\":$OWNER_ID},{\"kind\":\"bot\",\"id\":5}]}" + export STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"body\":\"Closes #9306\\n\\n- Cost decision: [Q50]($LINK)\"}" + export STUB_CLOSING='{"data":{"repository":{"pullRequest":{"closingIssuesReferences":{"totalCount":1,"nodes":[{"number":9306,"repository":{"nameWithOwner":"o/r"}}]}}}}}' + STUB_DECISIONS="{\"111\":$(decision 9306 "$OWNER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted.')}" + export STUB_DECISIONS + export STUB_EDITED='{"data":{"node":{"lastEditedAt":null}}}' + STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T13:20:00Z)]" + STUB_COMMENTS="[$(hold 2026-10-23T13:20:05Z cost)]" + export STUB_EVENTS STUB_COMMENTS + unset STUB_TRACKER +} + +# expect