Skip to content

Commit 65277b0

Browse files
donislawdevclaude
andauthored
release: the check of a published release answers with every question, not the first (#42)
The job that checks a published release stopped at the first red step, so the questions behind it were never asked and nothing said so. Measured on the v0.2.0 publish, 2026-08-28. The step running the two commands offline went red. The three steps after it were skipped: whether every Windows program carries our certificate, whether the page promises what was just checked, and whether a full release is the one people are offered. So three questions about a published release went unasked, and the run reported one problem. One of those three would have gone red as well. The published notes carry none of the three sentences the notes step looks for, measured the same way. That turns a list into a queue, and every trip round the queue costs a publish. This is the rule a recipe already follows when it refuses, RC7, applied to the one gate standing over what strangers download. Each check now carries continue-on-error and an id, and a final step reads all seven and decides. It reads outcome rather than conclusion, because continue-on-error rewrites conclusion to success and a verdict reading that would pass however the checks went. The macOS job is left alone. It has a single check, so there is nothing for a first failure to hide. The guard holds every check between the download and the verdict to being collected, so a check added later cannot quietly go back to covering for the ones behind it. Three mutations, all caught. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent c15205d commit 65277b0

2 files changed

Lines changed: 247 additions & 0 deletions

File tree

.github/workflows/verify-release.yml

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,8 @@ jobs:
178178
ls -l
179179
180180
- name: everything a release is made of is there
181+
id: assets
182+
continue-on-error: true
181183
shell: bash
182184
working-directory: published
183185
# A release missing one file looks almost exactly like a finished one,
@@ -197,13 +199,17 @@ jobs:
197199
echo "every expected asset is published"
198200
199201
- name: the checksum a user is told to compare
202+
id: checksums
203+
continue-on-error: true
200204
shell: bash
201205
working-directory: published
202206
run: |
203207
set -euo pipefail
204208
sha256sum -c verify-SHA256SUMS.txt
205209
206210
- name: the two commands the notes tell people to run
211+
id: commands_api
212+
continue-on-error: true
207213
shell: bash
208214
working-directory: published
209215
env:
@@ -225,6 +231,8 @@ jobs:
225231
--predicate-type https://spdx.dev/Document/v2.3
226232
227233
- name: the same two commands with no network to the API
234+
id: commands_offline
235+
continue-on-error: true
228236
shell: bash
229237
working-directory: published
230238
# The notes promise these work offline with --bundle, and a person
@@ -240,6 +248,8 @@ jobs:
240248
--predicate-type https://spdx.dev/Document/v2.3
241249
242250
- name: every Windows program is signed, stamped, and by OUR certificate
251+
id: signatures
252+
continue-on-error: true
243253
shell: pwsh
244254
working-directory: published
245255
# This is the step the whole job runs on Windows for. All three, not the
@@ -278,6 +288,8 @@ jobs:
278288
"all three signed by the pinned certificate, each with a timestamp"
279289
280290
- name: the page promises what was just checked
291+
id: notes
292+
continue-on-error: true
281293
shell: bash
282294
env:
283295
GH_TOKEN: ${{ github.token }}
@@ -299,6 +311,8 @@ jobs:
299311
echo "the notes and this job say the same thing"
300312
301313
- name: a full release has to be the one people are offered
314+
id: latest
315+
continue-on-error: true
302316
shell: bash
303317
env:
304318
GH_TOKEN: ${{ github.token }}
@@ -317,3 +331,56 @@ jobs:
317331
latest="$(gh api "repos/$GITHUB_REPOSITORY/releases/latest" --jq .tag_name)"
318332
echo "latest is $latest, this release is $TAG"
319333
test "$latest" = "$TAG"
334+
335+
- name: one verdict, not the first failure
336+
if: always()
337+
shell: bash
338+
# Every check above carries continue-on-error, so this step is reached
339+
# whatever happened, and this step is the one that decides. A person
340+
# reads a release page once and wants the whole list of what is wrong
341+
# with it, not the first line - the same rule a recipe follows when it
342+
# refuses (RC7).
343+
#
344+
# Measured on the v0.2.0 publish, 2026-08-28: the offline command step
345+
# went red and the three checks after it never ran at all. One of those
346+
# three would have gone red too, because the published notes carry none
347+
# of the three sentences the notes check looks for. So the run reported
348+
# one problem where there were at least two, and nothing on the page
349+
# said the rest had not been asked.
350+
#
351+
# outcome and not conclusion. continue-on-error rewrites conclusion to
352+
# success and leaves outcome alone, so reading conclusion here would
353+
# report every check as passing however they went.
354+
env:
355+
ASSETS: ${{ steps.assets.outcome }}
356+
CHECKSUMS: ${{ steps.checksums.outcome }}
357+
COMMANDS_API: ${{ steps.commands_api.outcome }}
358+
COMMANDS_OFFLINE: ${{ steps.commands_offline.outcome }}
359+
SIGNATURES: ${{ steps.signatures.outcome }}
360+
NOTES: ${{ steps.notes.outcome }}
361+
LATEST: ${{ steps.latest.outcome }}
362+
run: |
363+
set -euo pipefail
364+
failed=0
365+
report() {
366+
printf ' %-52s %s\n' "$2" "$1"
367+
if [ "$1" != "success" ]; then
368+
failed=1
369+
fi
370+
}
371+
echo "what this release page answers for:"
372+
report "$ASSETS" "everything a release is made of is there"
373+
report "$CHECKSUMS" "the checksum a user is told to compare"
374+
report "$COMMANDS_API" "the commands the notes tell people to run"
375+
report "$COMMANDS_OFFLINE" "the same commands with no network"
376+
report "$SIGNATURES" "every Windows program by our certificate"
377+
report "$NOTES" "the page promises what was just checked"
378+
report "$LATEST" "a full release is the one people are offered"
379+
echo
380+
if [ "$failed" != "0" ]; then
381+
echo "This release page does not answer for itself."
382+
echo "Every line above that is not success is a separate thing to fix,"
383+
echo "and they are all listed rather than found one release at a time."
384+
exit 1
385+
fi
386+
echo "the release page answers for itself"
Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
package guard
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
9+
"github.com/goccy/go-yaml"
10+
)
11+
12+
// What this defends. The job that checks a published release asks every one of
13+
// its questions and answers with all of them, rather than stopping at the first
14+
// one that goes wrong.
15+
//
16+
// Why it needed a guard. Measured on the v0.2.0 publish, 2026-08-28. The job
17+
// ran, the step checking the two commands offline went red, and the three steps
18+
// after it were skipped - the Windows signatures, whether the page promises what
19+
// was checked, and whether a full release is the one people are offered. So
20+
// three questions about the published release were never asked at all, and the
21+
// run said nothing about that. One of the three would have gone red too: the
22+
// published notes carry none of the three sentences the notes step looks for,
23+
// measured the same way.
24+
//
25+
// Why it matters more here than in most places. This job runs once per release
26+
// and a person reads it once. A gate that reports one problem per release turns
27+
// a list into a queue, and every trip round that queue costs a publish. It is
28+
// the rule the recipe already follows when it refuses, RC7, applied to the one
29+
// gate standing over the thing strangers download.
30+
//
31+
// Why the macOS job is not held to this. It has a single check, so there is
32+
// nothing for a first failure to hide. The rule here is about a step covering
33+
// for the ones behind it, which needs at least two.
34+
//
35+
// What this does NOT check. That the checks themselves are right, or that the
36+
// list of them is complete. It checks that whatever list exists is asked in
37+
// full and reported in full.
38+
39+
// releaseWorkflow is the shape of a workflow file, as much of it as these
40+
// questions need.
41+
type releaseWorkflow struct {
42+
Jobs map[string]struct {
43+
Steps []struct {
44+
Name string `yaml:"name"`
45+
ID string `yaml:"id"`
46+
ContinueOnError bool `yaml:"continue-on-error"`
47+
If string `yaml:"if"`
48+
Env map[string]string `yaml:"env"`
49+
Run string `yaml:"run"`
50+
Uses string `yaml:"uses"`
51+
} `yaml:"steps"`
52+
} `yaml:"jobs"`
53+
}
54+
55+
// The step the checks begin after. Named here rather than counted, because a
56+
// position is the thing that moves when somebody inserts a step.
57+
const releaseSetupEndsAfter = "download every published asset"
58+
59+
func readReleaseWorkflow(t *testing.T) releaseWorkflow {
60+
t.Helper()
61+
62+
body, err := os.ReadFile(filepath.Join(repoRoot(t), ".github", "workflows", "verify-release.yml"))
63+
if err != nil {
64+
t.Skipf("no verify-release.yml here: %v", err)
65+
}
66+
67+
var parsed releaseWorkflow
68+
if err := yaml.Unmarshal(body, &parsed); err != nil {
69+
t.Fatalf("verify-release.yml does not parse as YAML, so the job it describes cannot run: %v", err)
70+
}
71+
if len(parsed.Jobs) == 0 {
72+
t.Fatal("verify-release.yml declares no jobs, so this proved nothing")
73+
}
74+
return parsed
75+
}
76+
77+
// Every check is asked, and every answer reaches the verdict.
78+
func TestNoReleaseCheckCanHideTheOnesBehindIt(t *testing.T) {
79+
parsed := readReleaseWorkflow(t)
80+
81+
job, ok := parsed.Jobs["verify"]
82+
if !ok {
83+
t.Fatalf("verify-release.yml has no job called verify. It has: %v", jobNames(parsed))
84+
}
85+
if len(job.Steps) < 3 {
86+
t.Fatalf("the verify job has %d steps, which is too few for it to be the job this "+
87+
"guard describes", len(job.Steps))
88+
}
89+
90+
// The verdict is the last step, and it has to be able to run after a red
91+
// one and to fail on what it read.
92+
verdict := job.Steps[len(job.Steps)-1]
93+
if !strings.Contains(verdict.If, "always()") {
94+
t.Errorf("the last step of the verify job is %q and it runs on %q.\n"+
95+
"It has to run on always(), because the steps before it are allowed to go red "+
96+
"and this is the step that reports them. Without it the job ends silently at "+
97+
"the first failure, which is the state measured on the v0.2.0 publish.",
98+
verdict.Name, verdict.If)
99+
}
100+
if !strings.Contains(verdict.Run, "exit 1") {
101+
t.Errorf("the last step of the verify job, %q, has no way to fail.\n"+
102+
"Every check before it carries continue-on-error, so this step is the only "+
103+
"thing that can turn a red check into a red job. A gate that cannot fail is "+
104+
"not a gate.", verdict.Name)
105+
}
106+
107+
// Where the checks start. Anything after the setup and before the verdict
108+
// is a check and has to be collected.
109+
start := -1
110+
for i, s := range job.Steps {
111+
if s.Name == releaseSetupEndsAfter {
112+
start = i + 1
113+
break
114+
}
115+
}
116+
if start < 0 {
117+
t.Fatalf("no step of the verify job is called %q, so this guard cannot tell setup "+
118+
"from checks. Rename it back or teach this guard the new name.", releaseSetupEndsAfter)
119+
}
120+
121+
checks := job.Steps[start : len(job.Steps)-1]
122+
if len(checks) == 0 {
123+
t.Fatal("the verify job has no check steps between the download and the verdict, " +
124+
"so the loop below proved nothing")
125+
}
126+
127+
// Everything the verdict reads, in one string, so a missing wire is a
128+
// missing substring rather than a guess about how it was written.
129+
wiring := strings.Join(values(verdict.Env), "\n")
130+
131+
for _, s := range checks {
132+
if !s.ContinueOnError {
133+
t.Errorf("the check %q stops the job when it fails, so every check after it is "+
134+
"skipped and its question goes unasked.\n"+
135+
"Give it continue-on-error: true and an id, and read that id in %q.",
136+
s.Name, verdict.Name)
137+
continue
138+
}
139+
if s.ID == "" {
140+
t.Errorf("the check %q is allowed to fail without stopping the job and has no id, "+
141+
"so nothing can read how it went. Its failure would be invisible, which is "+
142+
"worse than stopping the job.", s.Name)
143+
continue
144+
}
145+
146+
want := "steps." + s.ID + ".outcome"
147+
if !strings.Contains(wiring, want) {
148+
t.Errorf("the check %q (id %q) is allowed to fail and %q never reads it.\n"+
149+
"Add %s to that step's env. A check whose result nothing reads is a check "+
150+
"whose failure is a warning nobody sees.",
151+
s.Name, s.ID, verdict.Name, want)
152+
}
153+
}
154+
155+
// conclusion is the field continue-on-error rewrites to success. Reading it
156+
// here would make the verdict report every check as passing, whatever
157+
// happened, and the job would be green for the rest of its life.
158+
if strings.Contains(wiring, ".conclusion") {
159+
t.Errorf("%q reads conclusion for at least one check.\n"+
160+
"continue-on-error rewrites conclusion to success and leaves outcome alone, so "+
161+
"this verdict would pass however the checks went. Read outcome.\n"+
162+
" it reads: %s", verdict.Name, wiring)
163+
}
164+
}
165+
166+
func jobNames(w releaseWorkflow) []string {
167+
out := make([]string, 0, len(w.Jobs))
168+
for name := range w.Jobs {
169+
out = append(out, name)
170+
}
171+
return out
172+
}
173+
174+
func values(m map[string]string) []string {
175+
out := make([]string, 0, len(m))
176+
for _, v := range m {
177+
out = append(out, v)
178+
}
179+
return out
180+
}

0 commit comments

Comments
 (0)