From e783ae9427ecc5a1d9e865476a54f911d8554d69 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 02:31:48 +0000 Subject: [PATCH] ci(parity): gate that a required check can be superseded, not only that it can exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-503's gate half. The workflow fix landed in 49b89dd; this is the property that stops it regressing, and the reason it was split has gone — CLOUD-494's PR landed, so `ci-local-parity` and its suite are free. Property 11: every `pull_request` workflow producing a name in `$CI_REQUIRED_CHECKS` must declare `ready_for_review` in its `types:`. It sits on the seam properties 5 and 6 already own. Those couple `CI_REQUIRED_CHECKS` to job names in both directions — a required name that is no job fails, a `pull_request` job missing from the list fails. Both ask whether a run can EXIST. This asks whether it can be REFRESHED, which is the half that was missing: omitting `types:` defaults to `[opened, synchronize, reopened]`, every job is draft-gated because property 1 makes that universal, so a PR created as a draft mints a `skipped` run on `opened` and no event remains that could replace it. `land` readies before pushing and its push moves nothing whenever HEAD is already on the remote, so the ready event is the only one that can arrive. `checks-green` correctly refuses to read a skip as an answer, and the poll never terminates while the branch holds the fleet lease. SCOPED TO WORKFLOWS PRODUCING A REQUIRED NAME, deliberately. A workflow whose checks nothing waits on may miss the ready event with no consequence, and demanding the key of every `pull_request` workflow would assert more than the failure needs. That scoping is why the check sits beside the job-name collection inside the loop rather than in the `want_names` comparison at the end, which has the names but no longer knows which file they came from. The `pull_request:` sub-block is read on its own, never the whole `on:` block: a `types:` belonging to another trigger says nothing about this one, and reading them together would let one satisfy the other. SHOWN ABLE TO FAIL, BOTH DIRECTIONS (CLOUD-418). A fixture naming a required check and omitting `ready_for_review` is refused; one producing no required name is not asked for the key. Mutation-checked rather than argued: replacing the comparison with `true` reddens case 3 and only case 3. The `workflow` fixture helper hardcoded `types: [opened, synchronize]`, which is non-compliant under this property — so it gains a `types` argument defaulting to the full set, and the `fanin` fixture's own `final.yml` is corrected the same way. Without that both would have failed for the new reason rather than their own, which is a suite that no longer tests what its names say. Refs: CLOUD-503 --- mise-tasks/ci-local-parity | 46 ++++++++++++++++++++++++++++++++++---- tests/ci-local-parity.bats | 35 ++++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/mise-tasks/ci-local-parity b/mise-tasks/ci-local-parity index 789071f78..2e59f6546 100755 --- a/mise-tasks/ci-local-parity +++ b/mise-tasks/ci-local-parity @@ -170,12 +170,50 @@ for wf in "$workflows"/*.yml; do grep -q '^ pull_request:' <<<"$triggers" || continue checked=$((checked + 1)) - job_names="$job_names -$(awk ' + wf_job_names=$(awk ' /^jobs:/ { injobs = 1; next } injobs && /^[a-z]/ { injobs = 0 } injobs && /^ name:[[:space:]]/ { sub(/^ name:[[:space:]]*/, ""); print } - ' "$wf")" + ' "$wf") + job_names="$job_names +$wf_job_names" + + # Property 11. A REQUIRED CHECK MUST BE ABLE TO BE SUPERSEDED (CLOUD-503). + # + # Omitting `types:` defaults to `[opened, synchronize, reopened]`. Every job + # here is draft-gated — property 1 above makes that universal — so a PR + # created as a draft mints a `skipped` run on `opened`, and with no + # `ready_for_review` there is no event left that could replace it. `land` + # readies before pushing and its push moves nothing whenever HEAD is already + # on the remote, so the ready event is the only one that can arrive; + # `checks-green` correctly refuses to read a skip as an answer, and the poll + # never terminates while the branch holds the fleet lease. + # + # Measured as a deadlock across two PRs at once, both fully green but for one + # such name, with `main` frozen so neither could rebase out of it. + # + # SCOPED TO WORKFLOWS PRODUCING A REQUIRED NAME, deliberately. A workflow + # whose checks nothing waits on may miss the ready event with no consequence, + # and demanding the key of every `pull_request` workflow would be a gate + # asserting more than the failure needs — the reason this sits beside the + # job-name collection rather than in the `want_names` comparison at the end, + # which has the names but no longer knows which file they came from. + produces_required=0 + while IFS= read -r job_name; do + [ -n "$job_name" ] || continue + if grep -qxF "$job_name" <<<"$want_names"; then + produces_required=1 + break + fi + done < <(printf '%s\n' "$wf_job_names" | sed '/^$/d' | base_names) + if [ "$produces_required" = 1 ]; then + # The `pull_request:` sub-block only. A `types:` belonging to another + # trigger says nothing about this one, and reading the whole `on:` block + # would let one satisfy the other. + pr_block=$(awk '/^ pull_request:/{p=1;next} /^ [A-Za-z_]+:/{p=0} p' <<<"$triggers") + grep -q 'ready_for_review' <<<"$pr_block" || + problem "$wf produces a required check but its pull_request trigger omits \`ready_for_review\` — the draft-era \`opened\` run is a skip nothing can then supersede, so \`ci-wait\` polls it forever while the lease is held. Add: types: [opened, synchronize, reopened, ready_for_review] (CLOUD-503)." + fi grep -q 'cancel-in-progress: true' "$wf" || problem "$wf triggers on pull_request but never supersedes its own runs — add a concurrency group with cancel-in-progress: true, or a lap pays out the run it just made obsolete." @@ -380,4 +418,4 @@ while IFS= read -r name; do done <<<"$have_names" [ "$fail" -eq 0 ] || exit 1 -echo "ci-local-parity: $checked pull_request workflow(s) — draft-gated, self-superseding, lease-gated before they spend, running nothing verify does not, asserting every needs: they wait on, and named in full by CI_REQUIRED_CHECKS; the release PR opens as a draft; all $all_seen workflow(s) declare a concurrency group and no two share a cron expression" +echo "ci-local-parity: $checked pull_request workflow(s) — draft-gated, self-superseding, lease-gated before they spend, running nothing verify does not, asserting every needs: they wait on, named in full by CI_REQUIRED_CHECKS, and subscribing to ready_for_review wherever a required check depends on it; the release PR opens as a draft; all $all_seen workflow(s) declare a concurrency group and no two share a cron expression" diff --git a/tests/ci-local-parity.bats b/tests/ci-local-parity.bats index 6aaf9e946..a1bf5a8fa 100644 --- a/tests/ci-local-parity.bats +++ b/tests/ci-local-parity.bats @@ -37,13 +37,14 @@ workflow() { local name="$1" guard="${2:- if: \${{ github.event.pull_request.draft == false }}}" \ conc="${3:-cancel-in-progress: true}" task="${4:-ci}" \ lease="${5:- - name: Landing lease precondition - run: ':'}" + run: ':'}" \ + types="${6:-[opened, synchronize, reopened, ready_for_review]}" cat >"$WF/$name.yml" <<-EOF name: $name on: pull_request: - types: [opened, synchronize] + types: $types concurrency: group: $name-\${{ github.ref }} @@ -92,6 +93,34 @@ scheduled() { [[ "$output" == *"draft == false"* ]] } +@test "a required check whose workflow cannot see ready_for_review is refused" { + # CLOUD-503. Omitting `ready_for_review` defaults the trigger to + # `[opened, synchronize, reopened]`. Every job is draft-gated, so the draft + # `opened` run is a `skipped` — and `land` readies before pushing, with + # nothing to push whenever HEAD is already on the remote. No event remains + # that could supersede the skip, `checks-green` refuses to read it as an + # answer, and the lease is held while the poll never ends. + workflow ci " if: \${{ github.event.pull_request.draft == false }}" \ + "cancel-in-progress: true" ci " - name: Landing lease precondition + run: ':'" "[opened, synchronize, reopened]" + run "$GATE" + [ "$status" -eq 1 ] + [[ "$output" == *"omits \`ready_for_review\`"* ]] +} + +@test "a workflow producing no required check may omit ready_for_review" { + # The other direction, and the reason the property is scoped rather than + # demanded of every pull_request workflow: nothing waits on `other`, so a + # skip nothing can supersede costs nobody a poll. A gate that asserted more + # than the failure needs would refuse this file for no reason. + printf 'CI_REQUIRED_CHECKS = "elsewhere"\n\n[tasks.ci]\nrun = "hk check --all"\n\n[tasks.verify]\ndepends = ["ci"]\n\n[tasks.elsewhere]\nrun = "true"\n' >"$MANIFEST" + workflow ci " if: \${{ github.event.pull_request.draft == false }}" \ + "cancel-in-progress: true" ci " - name: Landing lease precondition + run: ':'" "[opened, synchronize, reopened]" + run "$GATE" + [[ "$output" != *"ready_for_review"* ]] +} + @test "a workflow that does not supersede its own runs is refused" { # A lap rebases and pushes; without this the superseded SHA's run is paid # out in full for a verdict nobody reads. @@ -253,7 +282,7 @@ fanin() { on: pull_request: - types: [opened, synchronize] + types: [opened, synchronize, reopened, ready_for_review] concurrency: group: final-\${{ github.ref }}