Skip to content

release: the check of a published release answers with every question, not the first #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
donislawdev merged 1 commit into from
Sep 2, 2026
Merged

release: the check of a published release answers with every question, not the first #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions .github/workflows/verify-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -197,13 +199,17 @@ 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: |
set -euo pipefail
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:
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 }}
Expand All @@ -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 }}
Expand All @@ -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"
180 changes: 180 additions & 0 deletions internal/guard/releaseverdict_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}
Loading