Skip to content

Commit 1670414

Browse files
committed
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 <martin.vogel.tech@gmail.com>
1 parent c16da50 commit 1670414

2 files changed

Lines changed: 112 additions & 2 deletions

File tree

‎.github/workflows/release.yml‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,23 @@ jobs:
8080
attest: true
8181

8282
# ── 4. Smoke test every binary ──────────────────────────────────
83+
# !cancelled() && !failure(): GitHub propagates "skipped" TRANSITIVELY down the
84+
# needs graph, so with skip_tests=true a skipped `test` skipped smoke and soak
85+
# too — even though `build` overrode the same condition and succeeded. Nothing
86+
# failed and nothing said so; the pipeline simply carried on toward publishing
87+
# artifacts that had never been smoke-tested or soaked. Every job downstream of
88+
# an optional phase needs this override, or the phase being optional silently
89+
# makes the phases after it optional as well.
8390
smoke:
91+
if: ${{ !cancelled() && !failure() }}
8492
needs: [build]
8593
uses: ./.github/workflows/_smoke.yml
8694
with:
8795
broad_platforms: true
8896

8997
# ── 5. Soak tests ──────────────────────────────────────────────
9098
soak:
91-
if: ${{ inputs.soak_level != 'none' }}
99+
if: ${{ !cancelled() && !failure() && inputs.soak_level != 'none' }}
92100
needs: [build]
93101
uses: ./.github/workflows/_soak.yml
94102
with:
@@ -97,9 +105,17 @@ jobs:
97105
version: ${{ inputs.version }}
98106

99107
# ── 6. Create DRAFT release ────────────────────────────────────
108+
# Requires smoke to have actually SUCCEEDED, not merely "not failed". The bare
109+
# !cancelled() && !failure() form is fail-OPEN: a skipped smoke is neither
110+
# cancelled nor failed, so a draft was one condition away from being cut from
111+
# binaries nobody had run. soak may legitimately be 'skipped' (soak_level=none),
112+
# so that one is enumerated explicitly rather than lumped in.
100113
release-draft:
101114
needs: [smoke, soak]
102-
if: ${{ !cancelled() && !failure() }}
115+
if: >-
116+
${{ !cancelled() && !failure()
117+
&& needs.smoke.result == 'success'
118+
&& (needs.soak.result == 'success' || needs.soak.result == 'skipped') }}
103119
runs-on: ubuntu-latest
104120
permissions:
105121
contents: write
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
#!/usr/bin/env bash
2+
# Contract: making a release phase optional must not silently make the phases
3+
# AFTER it optional too, and a draft must require smoke to have actually run.
4+
#
5+
# This exists because it nearly shipped untested binaries. `skip_tests: true`
6+
# skipped `test`, and GitHub propagates "skipped" TRANSITIVELY down the needs
7+
# graph: `build` overrode the condition and ran, but `smoke` and `soak` had no
8+
# override and were skipped. Nothing failed. Nothing said so. `release-draft`
9+
# then ran anyway, because `!cancelled() && !failure()` is fail-OPEN — a skipped
10+
# job is neither cancelled nor failed — so the pipeline was one gate away from
11+
# publishing artifacts nobody had smoke-tested or soaked.
12+
#
13+
# Two properties are pinned:
14+
# 1. every job downstream of an optional phase carries the
15+
# `!cancelled() && !failure()` override, so it runs when an ancestor was
16+
# deliberately skipped;
17+
# 2. release-draft requires `needs.smoke.result == 'success'` explicitly, so a
18+
# skipped smoke BLOCKS the draft instead of sailing past it.
19+
set -euo pipefail
20+
21+
ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
22+
WF="$ROOT/.github/workflows/release.yml"
23+
[ -f "$WF" ] || { echo "FAIL: $WF not found" >&2; exit 2; }
24+
25+
python3 - "$WF" <<'PY'
26+
import pathlib
27+
import re
28+
import sys
29+
30+
text = pathlib.Path(sys.argv[1]).read_text()
31+
32+
# Slice the file into top-level job blocks: two-space indented "name:".
33+
blocks, current, name = {}, [], None
34+
for line in text.splitlines():
35+
m = re.match(r"^ ([A-Za-z0-9_-]+):\s*$", line)
36+
if m:
37+
if name:
38+
blocks[name] = "\n".join(current)
39+
name, current = m.group(1), []
40+
continue
41+
if name is not None:
42+
current.append(line)
43+
if name:
44+
blocks[name] = "\n".join(current)
45+
46+
failures = []
47+
48+
def cond(job):
49+
"""The job's `if:` expression, block scalars folded onto one line."""
50+
body = blocks.get(job, "")
51+
m = re.search(r"^ if:\s*(>-|>|\|-|\|)?\s*(.*?)(?=^ [a-z_-]+:|\Z)",
52+
body, re.S | re.M)
53+
if not m:
54+
return ""
55+
return " ".join(m.group(2).split())
56+
57+
# 1. Downstream-of-optional jobs must tolerate a deliberately skipped ancestor.
58+
# `test` is the optional phase (if: !inputs.skip_tests); everything after it
59+
# in the chain has to survive that.
60+
TOLERATE = ["build", "smoke", "soak", "release-draft"]
61+
for job in TOLERATE:
62+
if job not in blocks:
63+
failures.append(f"{job}: job missing from release.yml — update this contract")
64+
continue
65+
c = cond(job)
66+
if "!cancelled()" not in c or "!failure()" not in c:
67+
failures.append(
68+
f"{job}: `if:` lacks `!cancelled() && !failure()` (got: {c or '<none>'}).\n"
69+
f" With skip_tests=true a skipped ancestor SKIPS this job silently.")
70+
71+
# 2. The draft must require smoke to have genuinely succeeded.
72+
draft = cond("release-draft")
73+
if "needs.smoke.result == 'success'" not in draft:
74+
failures.append(
75+
"release-draft: `if:` must require needs.smoke.result == 'success'.\n"
76+
" `!cancelled() && !failure()` alone is fail-open: a SKIPPED smoke\n"
77+
" passes it, and the draft gets cut from unsmoked binaries.")
78+
79+
# soak is allowed to be skipped (soak_level=none) but must be enumerated, not
80+
# ignored — otherwise 'none' and 'silently never ran' are indistinguishable.
81+
if "needs.soak.result" not in draft:
82+
failures.append(
83+
"release-draft: `if:` must mention needs.soak.result explicitly so a\n"
84+
" legitimately skipped soak (soak_level=none) is distinguishable\n"
85+
" from a soak that never ran.")
86+
87+
if failures:
88+
for f in failures:
89+
print("FAIL: " + f, file=sys.stderr)
90+
print(f"release gate-chain contract FAILED with {len(failures)} violation(s)",
91+
file=sys.stderr)
92+
sys.exit(1)
93+
print("PASS: optional phases cannot silently disable the phases after them")
94+
PY

0 commit comments

Comments
 (0)