From b8f44b5cd2d50078186e82018449aedfc449dee8 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 2 Sep 2026 16:35:19 +0200 Subject: [PATCH] release: the check of a published release answers with every question, not the first 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 --- .github/workflows/verify-release.yml | 67 ++++++++++ internal/guard/releaseverdict_test.go | 180 ++++++++++++++++++++++++++ 2 files changed, 247 insertions(+) create mode 100644 internal/guard/releaseverdict_test.go diff --git a/.github/workflows/verify-release.yml b/.github/workflows/verify-release.yml index d52732e..0f728ac 100644 --- a/.github/workflows/verify-release.yml +++ b/.github/workflows/verify-release.yml @@ -178,6 +178,8 @@ jobs: ls -l - name: everything a release is made of is there + id: assets + continue-on-error: true shell: bash working-directory: published # A release missing one file looks almost exactly like a finished one, @@ -197,6 +199,8 @@ jobs: echo "every expected asset is published" - name: the checksum a user is told to compare + id: checksums + continue-on-error: true shell: bash working-directory: published run: | @@ -204,6 +208,8 @@ jobs: sha256sum -c verify-SHA256SUMS.txt - name: the two commands the notes tell people to run + id: commands_api + continue-on-error: true shell: bash working-directory: published env: @@ -225,6 +231,8 @@ jobs: --predicate-type https://spdx.dev/Document/v2.3 - name: the same two commands with no network to the API + id: commands_offline + continue-on-error: true shell: bash working-directory: published # The notes promise these work offline with --bundle, and a person @@ -240,6 +248,8 @@ jobs: --predicate-type https://spdx.dev/Document/v2.3 - name: every Windows program is signed, stamped, and by OUR certificate + id: signatures + continue-on-error: true shell: pwsh working-directory: published # This is the step the whole job runs on Windows for. All three, not the @@ -278,6 +288,8 @@ jobs: "all three signed by the pinned certificate, each with a timestamp" - name: the page promises what was just checked + id: notes + continue-on-error: true shell: bash env: GH_TOKEN: ${{ github.token }} @@ -299,6 +311,8 @@ jobs: echo "the notes and this job say the same thing" - name: a full release has to be the one people are offered + id: latest + continue-on-error: true shell: bash env: GH_TOKEN: ${{ github.token }} @@ -317,3 +331,56 @@ jobs: latest="$(gh api "repos/$GITHUB_REPOSITORY/releases/latest" --jq .tag_name)" echo "latest is $latest, this release is $TAG" test "$latest" = "$TAG" + + - name: one verdict, not the first failure + if: always() + shell: bash + # Every check above carries continue-on-error, so this step is reached + # whatever happened, and this step is the one that decides. A person + # reads a release page once and wants the whole list of what is wrong + # with it, not the first line - the same rule a recipe follows when it + # refuses (RC7). + # + # Measured on the v0.2.0 publish, 2026-08-28: the offline command step + # went red and the three checks after it never ran at all. One of those + # three would have gone red too, because the published notes carry none + # of the three sentences the notes check looks for. So the run reported + # one problem where there were at least two, and nothing on the page + # said the rest had not been asked. + # + # outcome and not conclusion. continue-on-error rewrites conclusion to + # success and leaves outcome alone, so reading conclusion here would + # report every check as passing however they went. + env: + ASSETS: ${{ steps.assets.outcome }} + CHECKSUMS: ${{ steps.checksums.outcome }} + COMMANDS_API: ${{ steps.commands_api.outcome }} + COMMANDS_OFFLINE: ${{ steps.commands_offline.outcome }} + SIGNATURES: ${{ steps.signatures.outcome }} + NOTES: ${{ steps.notes.outcome }} + LATEST: ${{ steps.latest.outcome }} + run: | + set -euo pipefail + failed=0 + report() { + printf ' %-52s %s\n' "$2" "$1" + if [ "$1" != "success" ]; then + failed=1 + fi + } + echo "what this release page answers for:" + report "$ASSETS" "everything a release is made of is there" + report "$CHECKSUMS" "the checksum a user is told to compare" + report "$COMMANDS_API" "the commands the notes tell people to run" + report "$COMMANDS_OFFLINE" "the same commands with no network" + report "$SIGNATURES" "every Windows program by our certificate" + report "$NOTES" "the page promises what was just checked" + report "$LATEST" "a full release is the one people are offered" + echo + if [ "$failed" != "0" ]; then + echo "This release page does not answer for itself." + echo "Every line above that is not success is a separate thing to fix," + echo "and they are all listed rather than found one release at a time." + exit 1 + fi + echo "the release page answers for itself" diff --git a/internal/guard/releaseverdict_test.go b/internal/guard/releaseverdict_test.go new file mode 100644 index 0000000..41bf9e7 --- /dev/null +++ b/internal/guard/releaseverdict_test.go @@ -0,0 +1,180 @@ +package guard + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/goccy/go-yaml" +) + +// What this defends. The job that checks a published release asks every one of +// its questions and answers with all of them, rather than stopping at the first +// one that goes wrong. +// +// Why it needed a guard. Measured on the v0.2.0 publish, 2026-08-28. The job +// ran, the step checking the two commands offline went red, and the three steps +// after it were skipped - the Windows signatures, whether the page promises what +// was checked, and whether a full release is the one people are offered. So +// three questions about the published release were never asked at all, and the +// run said nothing about that. One of the three would have gone red too: the +// published notes carry none of the three sentences the notes step looks for, +// measured the same way. +// +// Why it matters more here than in most places. This job runs once per release +// and a person reads it once. A gate that reports one problem per release turns +// a list into a queue, and every trip round that queue costs a publish. It is +// the rule the recipe already follows when it refuses, RC7, applied to the one +// gate standing over the thing strangers download. +// +// Why the macOS job is not held to this. It has a single check, so there is +// nothing for a first failure to hide. The rule here is about a step covering +// for the ones behind it, which needs at least two. +// +// What this does NOT check. That the checks themselves are right, or that the +// list of them is complete. It checks that whatever list exists is asked in +// full and reported in full. + +// releaseWorkflow is the shape of a workflow file, as much of it as these +// questions need. +type releaseWorkflow struct { + Jobs map[string]struct { + Steps []struct { + Name string `yaml:"name"` + ID string `yaml:"id"` + ContinueOnError bool `yaml:"continue-on-error"` + If string `yaml:"if"` + Env map[string]string `yaml:"env"` + Run string `yaml:"run"` + Uses string `yaml:"uses"` + } `yaml:"steps"` + } `yaml:"jobs"` +} + +// The step the checks begin after. Named here rather than counted, because a +// position is the thing that moves when somebody inserts a step. +const releaseSetupEndsAfter = "download every published asset" + +func readReleaseWorkflow(t *testing.T) releaseWorkflow { + t.Helper() + + body, err := os.ReadFile(filepath.Join(repoRoot(t), ".github", "workflows", "verify-release.yml")) + if err != nil { + t.Skipf("no verify-release.yml here: %v", err) + } + + var parsed releaseWorkflow + if err := yaml.Unmarshal(body, &parsed); err != nil { + t.Fatalf("verify-release.yml does not parse as YAML, so the job it describes cannot run: %v", err) + } + if len(parsed.Jobs) == 0 { + t.Fatal("verify-release.yml declares no jobs, so this proved nothing") + } + return parsed +} + +// Every check is asked, and every answer reaches the verdict. +func TestNoReleaseCheckCanHideTheOnesBehindIt(t *testing.T) { + parsed := readReleaseWorkflow(t) + + job, ok := parsed.Jobs["verify"] + if !ok { + t.Fatalf("verify-release.yml has no job called verify. It has: %v", jobNames(parsed)) + } + if len(job.Steps) < 3 { + t.Fatalf("the verify job has %d steps, which is too few for it to be the job this "+ + "guard describes", len(job.Steps)) + } + + // The verdict is the last step, and it has to be able to run after a red + // one and to fail on what it read. + verdict := job.Steps[len(job.Steps)-1] + if !strings.Contains(verdict.If, "always()") { + t.Errorf("the last step of the verify job is %q and it runs on %q.\n"+ + "It has to run on always(), because the steps before it are allowed to go red "+ + "and this is the step that reports them. Without it the job ends silently at "+ + "the first failure, which is the state measured on the v0.2.0 publish.", + verdict.Name, verdict.If) + } + if !strings.Contains(verdict.Run, "exit 1") { + t.Errorf("the last step of the verify job, %q, has no way to fail.\n"+ + "Every check before it carries continue-on-error, so this step is the only "+ + "thing that can turn a red check into a red job. A gate that cannot fail is "+ + "not a gate.", verdict.Name) + } + + // Where the checks start. Anything after the setup and before the verdict + // is a check and has to be collected. + start := -1 + for i, s := range job.Steps { + if s.Name == releaseSetupEndsAfter { + start = i + 1 + break + } + } + if start < 0 { + t.Fatalf("no step of the verify job is called %q, so this guard cannot tell setup "+ + "from checks. Rename it back or teach this guard the new name.", releaseSetupEndsAfter) + } + + checks := job.Steps[start : len(job.Steps)-1] + if len(checks) == 0 { + t.Fatal("the verify job has no check steps between the download and the verdict, " + + "so the loop below proved nothing") + } + + // Everything the verdict reads, in one string, so a missing wire is a + // missing substring rather than a guess about how it was written. + wiring := strings.Join(values(verdict.Env), "\n") + + for _, s := range checks { + if !s.ContinueOnError { + t.Errorf("the check %q stops the job when it fails, so every check after it is "+ + "skipped and its question goes unasked.\n"+ + "Give it continue-on-error: true and an id, and read that id in %q.", + s.Name, verdict.Name) + continue + } + if s.ID == "" { + t.Errorf("the check %q is allowed to fail without stopping the job and has no id, "+ + "so nothing can read how it went. Its failure would be invisible, which is "+ + "worse than stopping the job.", s.Name) + continue + } + + want := "steps." + s.ID + ".outcome" + if !strings.Contains(wiring, want) { + t.Errorf("the check %q (id %q) is allowed to fail and %q never reads it.\n"+ + "Add %s to that step's env. A check whose result nothing reads is a check "+ + "whose failure is a warning nobody sees.", + s.Name, s.ID, verdict.Name, want) + } + } + + // conclusion is the field continue-on-error rewrites to success. Reading it + // here would make the verdict report every check as passing, whatever + // happened, and the job would be green for the rest of its life. + if strings.Contains(wiring, ".conclusion") { + t.Errorf("%q reads conclusion for at least one check.\n"+ + "continue-on-error rewrites conclusion to success and leaves outcome alone, so "+ + "this verdict would pass however the checks went. Read outcome.\n"+ + " it reads: %s", verdict.Name, wiring) + } +} + +func jobNames(w releaseWorkflow) []string { + out := make([]string, 0, len(w.Jobs)) + for name := range w.Jobs { + out = append(out, name) + } + return out +} + +func values(m map[string]string) []string { + out := make([]string, 0, len(m)) + for _, v := range m { + out = append(out, v) + } + return out +}