diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index b9fd4caa5c..6c9f781ea6 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -4139,32 +4139,70 @@ jobs: uses: ./.base/.github/actions/setup-pinned-node # zizmor: ignore[self-repository] with: node-version-file: .base/.node-version - # THE HOLD STEPS READ THEIR CONFIG FROM THE BASE COMMIT, ONCE. The tracker + # THE HOLD STEPS READ THEIR CONFIG ONCE, FROM THE DEFAULT BRANCH'S TIP AT + # RUN TIME, OR THE BASE COMMIT FOR ANOTHER BASE. The tracker # config names the human-gated label, the trusted-actor list names who # may lift a hold and, with pr-pipeline.yaml's merge.agent-identity, the # lanes App's id that LANES_APP_ID must equal, and its # merge.trust-path-hold-apps the agent Apps whose trust-gate changes the - # review-lanes step holds; all are read at the base + # review-lanes step holds; all are read there, never at the head, # so the pull request cannot rewrite its own hold, as is merge-holds.mjs, # whose refsIssue the cost step runs, and clear-escalations.mjs with every # module and data file it loads, whose recheckStaleHold the hold-lift - # step runs. A sparse checkout of these files at the base SHA + # step runs. A re-run replays the event, whose base.sha stays stale, so + # a pull request into the default branch reads that branch's tip as it + # stands now: a policy merged since applies on the next run. A pull + # request's head cannot change the default branch, so the tip is no more + # the pull request's code than the base commit is. A merge group, and a + # pull request into another branch, keep the event's base SHA. + # hold_ref resolves the commit and outputs it only as 40 hex: it reads + # the tip with an unauthenticated `git ls-remote` of this public + # repository, run outside any checkout, and falls back to the base SHA + # with a warning when that read fails or returns anything else. + - name: Resolve the hold config's commit + id: hold_ref + if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }} + env: + EVENT: ${{ github.event_name }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }} + run: | + hex='^[0-9a-f]{40}$' + if [[ ! $BASE_SHA =~ $hex ]]; then + echo "::error::the base commit '$BASE_SHA' is not a commit SHA, so the hold config has no commit to read." + exit 1 + fi + sha=$BASE_SHA + if [ "$EVENT" = pull_request ] && [ -n "$DEFAULT_BRANCH" ] && [ "$BASE_REF" = "$DEFAULT_BRANCH" ]; then + tip=$(GIT_TERMINAL_PROMPT=0 timeout 60 git -C "$RUNNER_TEMP" ls-remote "https://github.com/$GITHUB_REPOSITORY" "refs/heads/$DEFAULT_BRANCH" | + awk -v ref="refs/heads/$DEFAULT_BRANCH" '$2 == ref { print $1 }') || tip= + if [[ $tip =~ $hex ]]; then + sha=$tip + else + echo "::warning::could not read the tip of $DEFAULT_BRANCH, so the hold config falls back to the base commit $BASE_SHA, which a policy merged after it does not reach." + fi + fi + echo "sha=$sha" >>"$GITHUB_OUTPUT" + # A sparse checkout of these files at that SHA # is a git fetch, not a REST request: as of actions/checkout v7.0.1 its # API calls (default branch, object format, the stale-merge tracepoint, the # no-git archive download) all return early for a 40-hex `ref` on a runner # with git; recheck on a checkout bump. It checks out no pull request - # code: the ref is the base commit, the only code run from it is those - # base modules, and no credential is persisted. + # code: the ref is the default branch's tip or the base commit, the only + # code run from it is those modules, and no credential is persisted. It + # never runs on an empty ref, which would check out the event's ref, the + # pull request's own merge commit. # The steps below read BASE_CONFIG: empty when this step did not succeed, # which each of them treats as an unreadable config; a file missing from - # a checkout that succeeded is a file the base commit does not have. + # a checkout that succeeded is a file that commit does not have. - name: Check out the base commit's hold config id: base_config - if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }} + if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') && steps.hold_ref.outputs.sha != '' }} continue-on-error: true uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }} + ref: ${{ steps.hold_ref.outputs.sha }} path: base-config persist-credentials: false sparse-checkout: | @@ -5023,7 +5061,7 @@ jobs: if [ ! -d "$BASE_CONFIG" ] || { [ -e "$conf" ] && ! apps=$(set -o pipefail; yq -o=json -I=0 '.merge."trust-path-hold-apps" // []' "$conf" | jq -ce 'if type == "array" and all(.[]; type == "string" and test("^[A-Za-z0-9]+(-[A-Za-z0-9]+)*\\[bot\\]$")) then map(ascii_downcase) else error("not a list of [bot] logins") end'); } || [ -z "$apps" ]; then - echo "::error::could not read merge.trust-path-hold-apps from the base commit's pr-pipeline.yaml, so whether an agent App touched the trust-gate paths of #$PR_NUMBER is unknown; re-run this run." + echo "::error::could not read merge.trust-path-hold-apps from pr-pipeline.yaml in the hold config (the default branch's tip at run time, or the base commit for another base), so whether an agent App touched the trust-gate paths of #$PR_NUMBER is unknown; re-run this run." exit 1 fi if [ "$apps" != '[]' ]; then @@ -5545,7 +5583,7 @@ jobs: . "$RUNNER_TEMP/gh-read.sh" module=${BASE_CONFIG:-}/.github/scripts/pr-merge/merge-holds.mjs if [ -z "${BASE_CONFIG:-}" ] || [ ! -s "$module" ]; then - echo "::error::cannot read merge-holds.mjs at the base commit; re-run this run." + echo "::error::cannot read merge-holds.mjs in the hold config (the default branch's tip at run time, or the base commit for another base); re-run this run." exit 1 fi read3() { @@ -5584,7 +5622,7 @@ jobs: "$RUNNER_TEMP/pr-unblock-statuses.json" "$module") || ! jq -e 'type == "array"' <<<"$stops" >/dev/null || ! failing=$(jq -r '.[] | [.context, .state, .description] | @tsv' <<<"$stops"); then - echo "::error::merge-holds.mjs at the base commit could not judge the statuses of $head; re-run this run." + echo "::error::merge-holds.mjs in the hold config (the default branch's tip at run time, or the base commit for another base) could not judge the statuses of $head; re-run this run." exit 1 fi if [ -z "$failing" ]; then @@ -5635,7 +5673,7 @@ jobs: ! identity=$(yq -er '.merge."agent-identity"' "$BASE_CONFIG/docs/conventions/pr-pipeline.yaml") || ! want=$(jq -er --arg login "$identity" '[.actors[] | select(.login == $login) | .id | numbers] | if length == 1 then .[0] else error("not one entry") end' "$BASE_CONFIG/.github/standards/trusted-actors/trusted-actors.json"); then - echo "::error::cannot read merge.agent-identity's trusted-actor id from the base commit's config; re-run this run." + echo "::error::cannot read merge.agent-identity's trusted-actor id from the hold config (the default branch's tip at run time, or the base commit for another base); re-run this run." exit 1 fi if [ "$want" != "$LANES_APP_ID" ]; then @@ -5690,7 +5728,7 @@ jobs: } console.log(JSON.stringify(reason));' "$RUNNER_TEMP/lanes-pull.json" "$BASE_CONFIG/.github/scripts/pr-merge/merge-holds.mjs") || [ -z "$switched" ] || ! reason=$(jq -er 'if . == null then "" else strings end' <<<"$switched"); then - echo "::error::merge-holds.mjs at the base commit could not judge the lanes kill switch for #$pr; re-run this run." + echo "::error::merge-holds.mjs in the hold config (the default branch's tip at run time, or the base commit for another base) could not judge the lanes kill switch for #$pr; re-run this run." exit 1 fi if [ -n "$reason" ]; then diff --git a/scripts/ci-hold-human-gated.test.sh b/scripts/ci-hold-human-gated.test.sh index a0a4d634ca..ad63d27533 100755 --- a/scripts/ci-hold-human-gated.test.sh +++ b/scripts/ci-hold-human-gated.test.sh @@ -69,14 +69,17 @@ if [[ -n "${agg_line:-}" && "$job_line" -lt "$hold_line" && "$hold_line" -lt "$a else fail "step is not inside ci-status before the aggregate (lines: $order)" fi -# The config comes from a sparse checkout of the base commit: never the head, -# never a persisted credential, and before every step that reads it. +# The config comes from a sparse checkout of the commit hold_ref resolves: +# never the head, never a persisted credential, never an empty ref (which +# checks out the event's ref, the pull request's own merge commit), and before +# every step that reads it. checkout="$(awk '$0 == " - name: Check out the base commit'"'"'s hold config" { on = 1; print; next } on && /^ - / { exit } on { print }' "$WORKFLOW")" # shellcheck disable=SC2016 # workflow literals to match, not shell expansions for line in $'\n id: base_config\n' 'continue-on-error: true' \ - 'ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}' \ + "if: \${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') && steps.hold_ref.outputs.sha != '' }}" \ + 'ref: ${{ steps.hold_ref.outputs.sha }}' \ 'persist-credentials: false' '/.work-item-tracker.json' '/.github/standards/trusted-actors/trusted-actors.json' \ 'sparse-checkout-cone-mode: false' 'path: base-config'; do if [[ "$checkout" == *"$line"* ]]; then ok "the base checkout carries '${line//$'\n'/}'"; else fail "the base checkout lost '${line//$'\n'/}'"; fi @@ -88,6 +91,96 @@ if [[ -n "${cost_line:-}" && "$job_line" -lt "$checkout_line" && "$checkout_line else fail "the base checkout is not in ci-status before the cost-hold step (lines: $order)" fi + +# hold_ref resolves the commit the hold config is read at: a pull request into +# the default branch reads the branch's tip at run time, so a re-run (which +# replays the event and its stale base.sha) still sees a policy merged since; +# any other base, and a merge group, keep the event's base SHA. +REF_NAME="Resolve the hold config's commit" +ref_step="$(awk -v name=" - name: $REF_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 }' <<<"$ref_step" >"$TMP_ROOT/hold-ref.sh" +# shellcheck disable=SC2016 # workflow literals to match, not shell expansions +for line in $'\n id: hold_ref\n' \ + "if: \${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }}" \ + 'BASE_REF: ${{ github.event.pull_request.base.ref }}' \ + 'DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}' \ + 'BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}'; do + if [[ "$ref_step" == *"$line"* ]]; then ok "hold_ref carries '${line//$'\n'/}'"; else fail "hold_ref lost '${line//$'\n'/}'"; fi +done +if [[ "$ref_step" == *"continue-on-error"* ]]; then + fail "hold_ref must not continue on error: the checkout's guard reads its output" +else + ok "hold_ref carries no continue-on-error" +fi +# shellcheck disable=SC2016 # a workflow expression to match, not a shell expansion +if [[ -s "$TMP_ROOT/hold-ref.sh" ]] && ! grep -q '\${{' "$TMP_ROOT/hold-ref.sh"; then + ok "hold_ref's run body takes every event value through env" +else + fail "hold_ref has no run body, or inlines an expression in it" +fi +order="$(grep -nE "^ - name: ($REF_NAME|Check out the base commit's hold config)$" "$WORKFLOW" | cut -d: -f1 | paste -sd' ')" +read -r ref_line checkout_line <<<"$order" +if [[ -n "${checkout_line:-}" && "$ref_line" -lt "$checkout_line" ]]; then + ok "hold_ref sits before the checkout that reads it" +else + fail "hold_ref is not before the base checkout (lines: $order)" +fi + +mkdir -p "$TMP_ROOT/git-bin" +cat >"$TMP_ROOT/git-bin/git" <<'EOF' +#!/usr/bin/env bash +printf '%s\n' "$*" >>"$GIT_LOG" +[[ -z "${STUB_LS-}" ]] || printf '%b' "$STUB_LS" +exit "${STUB_GIT_RC:-0}" +EOF +chmod +x "$TMP_ROOT/git-bin/git" +REF_BASE=1111111111111111111111111111111111111111 +REF_TIP=2222222222222222222222222222222222222222 +REF_DECOY=3333333333333333333333333333333333333333 +# hold_ref_case LABEL EVENT BASE_REF WANT_RC WANT_SHA WANT_GIT_CALLS WANT_WARNING +hold_ref_case() { + local label="$1" out rc sha calls + : >"$TMP_ROOT/hold-ref.out" + : >"$TMP_ROOT/git.log" + out="$(cd "$TMP_ROOT" && PATH="$TMP_ROOT/git-bin:$PATH" GIT_LOG="$TMP_ROOT/git.log" GITHUB_OUTPUT="$TMP_ROOT/hold-ref.out" \ + GITHUB_REPOSITORY=melodic-software/claude-code-plugins RUNNER_TEMP="$TMP_ROOT" \ + EVENT="$2" BASE_REF="$3" DEFAULT_BRANCH=main BASE_SHA="${REF_BASE_SHA-$REF_BASE}" \ + bash -e "$TMP_ROOT/hold-ref.sh" 2>&1)" && rc=0 || rc=$? + sha="$(sed -n 's/^sha=//p' "$TMP_ROOT/hold-ref.out")" + calls="$(grep -c . "$TMP_ROOT/git.log")" + if [[ "$rc" == "$4" && "$sha" == "$5" && "$calls" == "$6" ]] && + { [[ -z "$7" ]] && [[ "$out" != *"::warning::"* ]] || [[ -n "$7" && "$out" == *"$7"* ]]; }; then + ok "hold_ref: $label" + else + fail "hold_ref: $label (rc=$rc sha='$sha' git calls=$calls out: $out)" + fi +} +STUB_LS="$REF_TIP\trefs/heads/main\n" hold_ref_case "a pull request into the default branch reads its tip" \ + pull_request main 0 "$REF_TIP" 1 "" +if grep -qxF -e "-C $TMP_ROOT ls-remote https://github.com/melodic-software/claude-code-plugins refs/heads/main" "$TMP_ROOT/git.log"; then + ok "hold_ref asks the public remote for the default branch, outside any checkout" +else + fail "hold_ref's ls-remote call is not the expected one: $(cat "$TMP_ROOT/git.log")" +fi +STUB_LS="$REF_TIP\trefs/heads/main\n" hold_ref_case "a pull request into another branch keeps the base SHA" \ + pull_request release/1 0 "$REF_BASE" 0 "" +STUB_LS="$REF_TIP\trefs/heads/main\n" hold_ref_case "a merge group keeps the base SHA" \ + merge_group "" 0 "$REF_BASE" 0 "" +STUB_GIT_RC=128 hold_ref_case "a failed ls-remote falls back to the base SHA with a warning" \ + pull_request main 0 "$REF_BASE" 1 "::warning::" +STUB_LS="not-a-sha\trefs/heads/main\n" hold_ref_case "a non-hex answer falls back to the base SHA with a warning" \ + pull_request main 0 "$REF_BASE" 1 "::warning::" +STUB_LS="$REF_DECOY\trefs/heads/x/refs/heads/main\n" hold_ref_case "a tail-matching ref is not the default branch" \ + pull_request main 0 "$REF_BASE" 1 "::warning::" +STUB_LS="$REF_DECOY\trefs/heads/x/refs/heads/main\n$REF_TIP\trefs/heads/main\n" hold_ref_case "the exact ref wins over a tail match" \ + pull_request main 0 "$REF_TIP" 1 "" +REF_BASE_SHA=main hold_ref_case "a base that is not a commit SHA outputs no ref" \ + pull_request release/1 1 "" 0 "" gate="$(awk '/^ ci-status:$/ { on = 1; next } on && /^ if: / { print; exit }' "$WORKFLOW")" if [[ "$gate" == " if: \${{ !cancelled() }}" ]]; then ok "ci-status carries no contract-only gate, so the step runs on label events" diff --git a/scripts/ci-hold-pr-unblock-status.test.sh b/scripts/ci-hold-pr-unblock-status.test.sh index 44c8db2753..287accf549 100755 --- a/scripts/ci-hold-pr-unblock-status.test.sh +++ b/scripts/ci-hold-pr-unblock-status.test.sh @@ -147,13 +147,13 @@ expect "an unreadable status list fails closed" 1 "[]" "::error::could not read unset STUB_STATUS_FAILS expect "a description's tab and newline become spaces" 1 "[$(status pr-unblock/fix-ci failure "$LANE" $'one\ttwo\nthree')]" "on $HEAD: one two three. The lane" export BASE_CONFIG= -expect "no base config fails closed" 1 "[]" "::error::cannot read merge-holds.mjs at the base commit; re-run this run." +expect "no base config fails closed" 1 "[]" "::error::cannot read merge-holds.mjs in the hold config (the default branch's tip at run time, or the base commit for another base); re-run this run." if [[ -s "$TMP_ROOT/log" ]]; then fail "no base config still read: '$(cat "$TMP_ROOT/log")'"; else ok "no base config makes no call"; fi mkdir -p "$TMP_ROOT/old/.github/scripts/pr-merge" echo 'export const refsIssue = () => null;' >"$TMP_ROOT/old/.github/scripts/pr-merge/merge-holds.mjs" export BASE_CONFIG="$TMP_ROOT/old" expect "a base module without unblockStops fails closed" 1 "[$(status pr-unblock/fix-ci success)]" \ - "::error::merge-holds.mjs at the base commit could not judge the statuses of $HEAD; re-run this run." + "::error::merge-holds.mjs in the hold config (the default branch's tip at run time, or the base commit for another base) could not judge the statuses of $HEAD; re-run this run." echo 'export const unblockStops = () => ({});' >"$TMP_ROOT/old/.github/scripts/pr-merge/merge-holds.mjs" expect "a base module that answers no list fails closed" 1 "[]" "could not judge the statuses of $HEAD" unset BASE_CONFIG diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index 32920cbf7d..5197b02fc5 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -328,7 +328,7 @@ for stub in '' 'export const holds = () => [];' 'export const switchHold = () => 'export const switchHold = () => { process.exit(0); };' 'export const switchHold = () => { throw new Error("x"); };'; do if [[ -z "$stub" ]]; then rm -f "$held_module"; else printf '%s\n' "$stub" >"$held_module"; fi expect "a base module that cannot judge the kill switch fails closed (${stub:-no module})" 1 0 \ - "::error::merge-holds.mjs at the base commit could not judge the lanes kill switch for #7; re-run this run." + "::error::merge-holds.mjs in the hold config (the default branch's tip at run time, or the base commit for another base) could not judge the lanes kill switch for #7; re-run this run." done cp "$MODULE" "$held_module" # shellcheck disable=SC2016 # a literal workflow expression, not shell diff --git a/scripts/ci-review-lanes.test.sh b/scripts/ci-review-lanes.test.sh index 122f677dea..71e91b5f8f 100755 --- a/scripts/ci-review-lanes.test.sh +++ b/scripts/ci-review-lanes.test.sh @@ -526,7 +526,7 @@ for dir in yq-fails yq-missing; do STUB_PATH="$TMP_ROOT/$dir" STUB_CHANGED=3000 SHARED_FILES="$(jq -cn '[range(3000) | {filename: "docs/f\(.).md"}]')" \ expect "a 3000-file list with $dir is held" 1 "::error::could not read merge.trust-path-hold-apps" done -BASE_CONFIG='' expect "an unreadable base config is held" 1 "::error::could not read merge.trust-path-hold-apps from the base commit's pr-pipeline.yaml" +BASE_CONFIG='' expect "an unreadable base config is held" 1 "::error::could not read merge.trust-path-hold-apps from pr-pipeline.yaml in the hold config (the default branch's tip at run time, or the base commit for another base)" printf 'merge:\n trust-path-hold-apps: melodic-agent[bot]\n' >"$TMP_ROOT/bare-config/docs/conventions/pr-pipeline.yaml" BASE_CONFIG="$TMP_ROOT/bare-config" expect "a malformed App list is held" 1 "could not read merge.trust-path-hold-apps" printf 'merge:\n rung: C2\n' >"$TMP_ROOT/bare-config/docs/conventions/pr-pipeline.yaml"