From 1670414211accd8fa331c7db3bd36239275fce50 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Thu, 30 Jul 2026 03:45:33 +0200 Subject: [PATCH] fix(release): skip_tests silently skipped smoke and soak too Run 30504814537 built all eight platforms successfully with skip_tests=true, and reported: test: skipped (intended) smoke: skipped (NOT intended) soak: skipped (NOT intended) release-draft: running GitHub propagates "skipped" TRANSITIVELY down the needs graph. `build` carried `!cancelled() && !failure()` so it overrode the skipped `test` and ran; `smoke` had no `if:` and `soak` only checked soak_level, so both inherited the skip. Nothing failed. Nothing announced it. The run then proceeded toward publishing binaries that had never been smoke-tested or soaked -- the exact opposite of what skip_tests was for, which was to skip a phase already proven green, not to quietly stop testing the artifacts. The second half of the defect is worse than the first: `release-draft` guarded itself with a bare `!cancelled() && !failure()`, which is fail-OPEN. A skipped job is neither cancelled nor failed, so a skipped smoke sails straight through it. The draft was one gate away from being cut from unverified binaries, and the only reason it was not is that the run was cancelled by hand. Fixes, both directions: - smoke and soak get `!cancelled() && !failure()`, so a deliberately skipped ancestor no longer skips them; - release-draft now requires `needs.smoke.result == 'success'` and enumerates `needs.soak.result` explicitly, so a SKIPPED smoke BLOCKS the draft, and a legitimately skipped soak (soak_level=none) stays distinguishable from a soak that never ran. tests/test_release_gate_chain_contract.sh pins both properties: every job downstream of the optional phase must carry the override, and the draft must demand smoke success rather than smoke non-failure. Verified in both directions -- it passes on this workflow and fails with 2 violations when the previous smoke/soak conditions are restored. Nothing was published: the release remained a draft (isDraft=true, publishedAt=null) and the run is cancelled. Signed-off-by: Martin Vogel --- .github/workflows/release.yml | 20 ++++- tests/test_release_gate_chain_contract.sh | 94 +++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) create mode 100755 tests/test_release_gate_chain_contract.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index cded89f8d..2432e02d4 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -80,7 +80,15 @@ jobs: attest: true # ── 4. Smoke test every binary ────────────────────────────────── + # !cancelled() && !failure(): GitHub propagates "skipped" TRANSITIVELY down the + # needs graph, so with skip_tests=true a skipped `test` skipped smoke and soak + # too — even though `build` overrode the same condition and succeeded. Nothing + # failed and nothing said so; the pipeline simply carried on toward publishing + # artifacts that had never been smoke-tested or soaked. Every job downstream of + # an optional phase needs this override, or the phase being optional silently + # makes the phases after it optional as well. smoke: + if: ${{ !cancelled() && !failure() }} needs: [build] uses: ./.github/workflows/_smoke.yml with: @@ -88,7 +96,7 @@ jobs: # ── 5. Soak tests ────────────────────────────────────────────── soak: - if: ${{ inputs.soak_level != 'none' }} + if: ${{ !cancelled() && !failure() && inputs.soak_level != 'none' }} needs: [build] uses: ./.github/workflows/_soak.yml with: @@ -97,9 +105,17 @@ jobs: version: ${{ inputs.version }} # ── 6. Create DRAFT release ──────────────────────────────────── + # Requires smoke to have actually SUCCEEDED, not merely "not failed". The bare + # !cancelled() && !failure() form is fail-OPEN: a skipped smoke is neither + # cancelled nor failed, so a draft was one condition away from being cut from + # binaries nobody had run. soak may legitimately be 'skipped' (soak_level=none), + # so that one is enumerated explicitly rather than lumped in. release-draft: needs: [smoke, soak] - if: ${{ !cancelled() && !failure() }} + if: >- + ${{ !cancelled() && !failure() + && needs.smoke.result == 'success' + && (needs.soak.result == 'success' || needs.soak.result == 'skipped') }} runs-on: ubuntu-latest permissions: contents: write diff --git a/tests/test_release_gate_chain_contract.sh b/tests/test_release_gate_chain_contract.sh new file mode 100755 index 000000000..3d7ceb7ac --- /dev/null +++ b/tests/test_release_gate_chain_contract.sh @@ -0,0 +1,94 @@ +#!/usr/bin/env bash +# Contract: making a release phase optional must not silently make the phases +# AFTER it optional too, and a draft must require smoke to have actually run. +# +# This exists because it nearly shipped untested binaries. `skip_tests: true` +# skipped `test`, and GitHub propagates "skipped" TRANSITIVELY down the needs +# graph: `build` overrode the condition and ran, but `smoke` and `soak` had no +# override and were skipped. Nothing failed. Nothing said so. `release-draft` +# then ran anyway, because `!cancelled() && !failure()` is fail-OPEN — a skipped +# job is neither cancelled nor failed — so the pipeline was one gate away from +# publishing artifacts nobody had smoke-tested or soaked. +# +# Two properties are pinned: +# 1. every job downstream of an optional phase carries the +# `!cancelled() && !failure()` override, so it runs when an ancestor was +# deliberately skipped; +# 2. release-draft requires `needs.smoke.result == 'success'` explicitly, so a +# skipped smoke BLOCKS the draft instead of sailing past it. +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +WF="$ROOT/.github/workflows/release.yml" +[ -f "$WF" ] || { echo "FAIL: $WF not found" >&2; exit 2; } + +python3 - "$WF" <<'PY' +import pathlib +import re +import sys + +text = pathlib.Path(sys.argv[1]).read_text() + +# Slice the file into top-level job blocks: two-space indented "name:". +blocks, current, name = {}, [], None +for line in text.splitlines(): + m = re.match(r"^ ([A-Za-z0-9_-]+):\s*$", line) + if m: + if name: + blocks[name] = "\n".join(current) + name, current = m.group(1), [] + continue + if name is not None: + current.append(line) +if name: + blocks[name] = "\n".join(current) + +failures = [] + +def cond(job): + """The job's `if:` expression, block scalars folded onto one line.""" + body = blocks.get(job, "") + m = re.search(r"^ if:\s*(>-|>|\|-|\|)?\s*(.*?)(?=^ [a-z_-]+:|\Z)", + body, re.S | re.M) + if not m: + return "" + return " ".join(m.group(2).split()) + +# 1. Downstream-of-optional jobs must tolerate a deliberately skipped ancestor. +# `test` is the optional phase (if: !inputs.skip_tests); everything after it +# in the chain has to survive that. +TOLERATE = ["build", "smoke", "soak", "release-draft"] +for job in TOLERATE: + if job not in blocks: + failures.append(f"{job}: job missing from release.yml — update this contract") + continue + c = cond(job) + if "!cancelled()" not in c or "!failure()" not in c: + failures.append( + f"{job}: `if:` lacks `!cancelled() && !failure()` (got: {c or ''}).\n" + f" With skip_tests=true a skipped ancestor SKIPS this job silently.") + +# 2. The draft must require smoke to have genuinely succeeded. +draft = cond("release-draft") +if "needs.smoke.result == 'success'" not in draft: + failures.append( + "release-draft: `if:` must require needs.smoke.result == 'success'.\n" + " `!cancelled() && !failure()` alone is fail-open: a SKIPPED smoke\n" + " passes it, and the draft gets cut from unsmoked binaries.") + +# soak is allowed to be skipped (soak_level=none) but must be enumerated, not +# ignored — otherwise 'none' and 'silently never ran' are indistinguishable. +if "needs.soak.result" not in draft: + failures.append( + "release-draft: `if:` must mention needs.soak.result explicitly so a\n" + " legitimately skipped soak (soak_level=none) is distinguishable\n" + " from a soak that never ran.") + +if failures: + for f in failures: + print("FAIL: " + f, file=sys.stderr) + print(f"release gate-chain contract FAILED with {len(failures)} violation(s)", + file=sys.stderr) + sys.exit(1) +print("PASS: optional phases cannot silently disable the phases after them") +PY