From eaf4654c0a2d9fb7371a2af5590c03a90226e611 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 22:11:51 -0400 Subject: [PATCH] test(release): make the version derivation executable, and guard it on dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steps 1 and 2 of this milestone shipped with no CI coverage at all, and the reason generalises: release.yml triggers only on main and tags, so nothing inside it is exercised until a release is already running. That is the worst place in the repo to be unguarded, because the failure mode is silence — a version nobody can compare looks exactly like being up to date, and nobody reports an update they were never offered. The fix is not a test that reads YAML. The derivation moved into ci/version.sh, so it can be RUN, and internal/server/release_version_test.go runs it on every push. release.yml now calls the same script, so the thing that ships and the thing under test are one artifact rather than two copies that agree until they don't. test-go.yml gains 'ci/**' and '.gitea/workflows/release.yml' in its paths. Without that the guard exists but never fires on the changes it protects, which is the same nothing it replaces. What is pinned, and why each one: - HHMM is zero-padded. A build at 00:42 must emit "0042"; a stripped leading zero shifts the segment two orders of magnitude and reverses comparisons against every other build that day. It only bites for a tenth of the day, so it will not be found by chance. - The name derives from the COMMIT and the code from the BUILD. Asserted by holding one clock and moving the other: the name must not move, the code must. - The code clears 1895, the highest versionCode the retired commit-count scheme shipped. Below that Android refuses the upgrade as a downgrade and the channel becomes a one-way door. - The tag is the name with a `v`, never chosen. - release.yml still calls the script, and does not derive a commit count again. This pins the WIRING: without it every other assertion keeps passing while the shipped path silently drifts out of coverage. The script rejects unusable clocks rather than emitting something plausible, and those rejections are tested — a guard that cannot fail is worse than none, because it reads as coverage. Falsified before committing rather than after: ran the script against good and broken inputs and watched all three failure paths fire; verified every asserted value by executing it rather than by reading it; and checked the two workflow predicates catch their regressions while staying immune to a comment that merely names the old formula. Step 5 of 5 — Scribe task #3812, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .gitea/workflows/release.yml | 45 +----- .gitea/workflows/test-go.yml | 6 + ci/version.sh | 71 ++++++++++ internal/server/release_version_test.go | 179 ++++++++++++++++++++++++ 4 files changed, 262 insertions(+), 39 deletions(-) create mode 100755 ci/version.sh create mode 100644 internal/server/release_version_test.go diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index c0bcdfaa..8389ab32 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -110,45 +110,12 @@ jobs: working-directory: ${{ github.workspace }} run: | set -euo pipefail - - # Two different clocks, deliberately. They answer different - # questions, and using one for both breaks whichever it fits worse. - # - # The NAME answers "is this the same code?" — so it derives from - # COMMIT time and reads identically on every lane that builds this - # source. A dev build and a main build of one commit must report the - # same string; build time cannot do that, it prints two numbers for - # one thing. - COMMIT_TS=$(git log --format=%ct -1 HEAD) - VERSION_NAME=$(date -u -d "@${COMMIT_TS}" +%Y.%m.%d.%H%M) - - # The ORDERING KEY answers "may this be installed over that?" — so it - # must be monotonic BY CONSTRUCTION. Minutes since 2020-01-01: ~3.5M - # today, ~525k/year, against a 2^31 ceiling. - # - # This replaced `git rev-list --count HEAD`, which was NOT monotonic - # and was commented as if it were. A commit count runs ahead on `dev`, - # so a dev build outranked the `main` release that superseded it and - # Android refused the install as a downgrade — a channel you could - # enter and not leave without uninstalling. - # - # Commit time would be wrong here too, for the mirror-image reason: - # rebuild an older commit and it goes DOWN, which on a phone is a - # refused install rather than a merely confusing label. - VERSION_CODE=$(( ( $(date -u +%s) - 1577836800 ) / 60 )) - - # Assert the emitted shape at the source. A malformed name still - # builds, signs and publishes perfectly happily, and only surfaces as - # an update nobody is ever offered — which nobody reports, because - # "no update available" and "I cannot read this" look identical. - if [[ ! "${VERSION_NAME}" =~ ^[0-9]{4}\.[0-9]{2}\.[0-9]{2}\.[0-9]{4}$ ]]; then - echo "::error::version name '${VERSION_NAME}' is not YYYY.MM.DD.HHMM" - exit 1 - fi - - echo "name=${VERSION_NAME}" >> "$GITHUB_OUTPUT" - echo "code=${VERSION_CODE}" >> "$GITHUB_OUTPUT" - echo "::notice::APK version: ${VERSION_NAME} (code=${VERSION_CODE})" + # The derivation lives in ci/version.sh, not here, so it can be + # executed by a test on every push. Anything inline in this file is + # unverifiable until a release is already running. + out="$(ci/version.sh HEAD)" + printf '%s\n' "${out}" >> "$GITHUB_OUTPUT" + echo "::notice::APK $(printf '%s' "${out}" | tr '\n' ' ')" # Checked BEFORE the expensive work, not after it. "Attach APK to gitea # Release" below resolves the release by tag and fails if it is absent — diff --git a/.gitea/workflows/test-go.yml b/.gitea/workflows/test-go.yml index b3eb990f..49cb3684 100644 --- a/.gitea/workflows/test-go.yml +++ b/.gitea/workflows/test-go.yml @@ -32,6 +32,12 @@ on: - 'cmd/**' - '.golangci.yml' - '.gitea/workflows/test-go.yml' + # The release lane's own trigger is `main` + tags, so nothing it + # contains is exercised until a release is already running. These two + # entries are what let internal/server/release_version_test.go guard + # the version derivation on ordinary dev pushes instead. + - 'ci/**' + - '.gitea/workflows/release.yml' # pull_request trigger intentionally omitted — see test-web.yml for # the rationale (single-author repo, push covers PR-merge equivalent). diff --git a/ci/version.sh b/ci/version.sh new file mode 100755 index 00000000..36583f45 --- /dev/null +++ b/ci/version.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# +# Derives the three values a build is stamped with, and the tag that names it. +# +# name=YYYY.MM.DD.HHMM label for people, from the COMMIT's timestamp +# code= ordering key, minutes since 2020-01-01 at BUILD time +# tag=v what a release of this commit must be called +# +# Usage: ci/version.sh [] (default HEAD) +# +# This exists as a script rather than inline workflow YAML for one reason: +# release.yml only runs on `main` and on tags, so anything living inside it is +# unverifiable until a release is already happening — which is the worst +# possible moment to discover the version is wrong, because the failure mode +# is silent (an update nobody is offered looks exactly like being current). +# As a script it can be executed by a test on every push instead. +# +# The two clocks are deliberate and are NOT interchangeable: +# +# The NAME answers "is this the same code?" — so it must read identically on +# every lane that builds this commit. Commit time does that; build time +# prints two different strings for one thing. +# +# The CODE answers "may this be installed over that?" — so it must be +# monotonic BY CONSTRUCTION. Build time is; commit time is not (rebuild an +# older commit and it goes down, which on a phone is a refused install), and +# a commit COUNT is worse still, because it runs ahead on `dev` and inverts +# against `main`. +set -euo pipefail + +readonly EPOCH_2020=1577836800 # 2020-01-01T00:00:00Z +readonly REF="${1:-HEAD}" + +# Both clocks are overridable so a test can pin them. Nothing but tests should +# set these — the defaults are the real derivation. +commit_epoch="${MINSTREL_COMMIT_EPOCH:-}" +if [ -z "${commit_epoch}" ]; then + commit_epoch="$(git log --format=%ct -1 "${REF}")" +fi +now_epoch="${MINSTREL_NOW_EPOCH:-$(date -u +%s)}" + +if ! name="$(date -u -d "@${commit_epoch}" +%Y.%m.%d.%H%M 2>/dev/null)"; then + echo "version.sh: could not read a commit timestamp from '${commit_epoch}'" >&2 + exit 1 +fi + +if ! [ "${now_epoch}" -eq "${now_epoch}" ] 2>/dev/null; then + echo "version.sh: build timestamp '${now_epoch}' is not a number" >&2 + exit 1 +fi +code=$(( (now_epoch - EPOCH_2020) / 60 )) + +# Assert the shape here, at the source. A malformed name builds, signs and +# publishes perfectly happily; it only surfaces later as an update channel +# that has quietly stopped offering anything. +if [[ ! "${name}" =~ ^[0-9]{4}\.[0-9]{2}\.[0-9]{2}\.[0-9]{4}$ ]]; then + echo "version.sh: name '${name}' is not YYYY.MM.DD.HHMM" >&2 + exit 1 +fi + +# A non-positive key means the build clock is set before 2020, and every +# comparison downstream would be nonsense. +if [ "${code}" -le 0 ]; then + echo "version.sh: ordering key '${code}' is not positive — build clock wrong?" >&2 + exit 1 +fi + +# KEY=VALUE, which is also exactly $GITHUB_OUTPUT's format. +echo "name=${name}" +echo "code=${code}" +echo "tag=v${name}" diff --git a/internal/server/release_version_test.go b/internal/server/release_version_test.go new file mode 100644 index 00000000..a63dcbcd --- /dev/null +++ b/internal/server/release_version_test.go @@ -0,0 +1,179 @@ +package server + +import ( + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// Guards the version derivation that stamps every build. +// +// These assertions used to be impossible to run. The derivation lived inline +// in release.yml, which triggers only on `main` and on tags — so a mistake in +// it could not surface until a release was already under way, and its failure +// mode is silence: a version nobody can compare looks exactly like being up to +// date, and nobody reports an update they were never offered. +// +// Moving it to ci/version.sh made it executable, so this runs on every push +// that touches the release machinery. That is the whole point of the file; the +// specific assertions below matter less than the fact that they run at all. +// +// The tests EXECUTE the script rather than asserting on its text, so they +// break when the behaviour changes rather than when the wording does. + +// highestOldSchemeCode is the largest versionCode ever shipped under the +// retired commit-count scheme (v2026.09.09 shipped 1895). Every code the new +// scheme emits must clear it, or Android would refuse the upgrade as a +// downgrade and the update channel would be a one-way door. +const highestOldSchemeCode = 1895 + +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatalf("no go.mod above %s", dir) + } + dir = parent + } +} + +// runVersion executes ci/version.sh with both clocks pinned, so the result is +// deterministic. Returns the parsed KEY=VALUE output. +func runVersion(t *testing.T, commitEpoch, nowEpoch string) map[string]string { + t.Helper() + out, err := versionScript(t, commitEpoch, nowEpoch) + if err != nil { + t.Fatalf("ci/version.sh failed: %v\n%s", err, out) + } + parsed := map[string]string{} + for _, line := range strings.Split(strings.TrimSpace(out), "\n") { + if k, v, ok := strings.Cut(line, "="); ok { + parsed[k] = v + } + } + return parsed +} + +func versionScript(t *testing.T, commitEpoch, nowEpoch string) (string, error) { + t.Helper() + root := repoRoot(t) + cmd := exec.Command(filepath.Join(root, "ci", "version.sh")) + cmd.Dir = root + cmd.Env = append(os.Environ(), + "MINSTREL_COMMIT_EPOCH="+commitEpoch, + "MINSTREL_NOW_EPOCH="+nowEpoch, + ) + out, err := cmd.CombinedOutput() + return string(out), err +} + +func TestVersionName_IsCommitTimeToTheMinute(t *testing.T) { + // 2025-09-09T18:48:56Z + got := runVersion(t, "1757443736", "1789000920") + if want := "2025.09.09.1848"; got["name"] != want { + t.Errorf("name = %q, want %q", got["name"], want) + } +} + +// HHMM is the segment most likely to be silently mangled, and it only bites +// for about a tenth of the day — a build just after midnight must emit "0042", +// never "42". A stripped leading zero shifts the segment by two orders of +// magnitude and reverses comparisons against every other build that day. +func TestVersionName_PadsTheMinuteSegment(t *testing.T) { + // 2026-09-10T00:42:00Z + got := runVersion(t, "1789000920", "1789000920") + if want := "2026.09.10.0042"; got["name"] != want { + t.Errorf("name = %q, want %q — leading zero lost?", got["name"], want) + } +} + +func TestVersionName_DerivesFromCommitNotBuildClock(t *testing.T) { + // Same commit, two different build clocks: the NAME must not move, or a + // dev build and a main build of one commit would report different strings + // and the channel field would stop being the only thing separating them. + a := runVersion(t, "1757443736", "1789000920") + b := runVersion(t, "1757443736", "1789500000") + if a["name"] != b["name"] { + t.Errorf("name moved with the build clock: %q vs %q", a["name"], b["name"]) + } + if a["code"] == b["code"] { + t.Errorf("code did NOT move with the build clock (%q) — it is not build-derived", a["code"]) + } +} + +func TestVersionCode_IsMinutesSince2020AndClearsTheOldScheme(t *testing.T) { + got := runVersion(t, "1789000920", "1789000920") + code, err := strconv.Atoi(got["code"]) + if err != nil { + t.Fatalf("code %q is not an integer: %v", got["code"], err) + } + if want := (1789000920 - 1577836800) / 60; code != want { + t.Errorf("code = %d, want %d", code, want) + } + if code <= highestOldSchemeCode { + t.Errorf("code %d does not clear the retired commit-count scheme (%d) — "+ + "Android would refuse the upgrade as a downgrade", code, highestOldSchemeCode) + } + if int64(code) > 2147483647 { + t.Errorf("code %d overflows versionCode's int32 ceiling", code) + } +} + +// The tag is not chosen, it is the name with a `v`. Anything else reintroduces +// the mismatch between what a tag claims and what the artifact reports. +func TestTag_IsTheNameWithAPrefix(t *testing.T) { + got := runVersion(t, "1757443736", "1789000920") + if want := "v" + got["name"]; got["tag"] != want { + t.Errorf("tag = %q, want %q", got["tag"], want) + } +} + +// A guard that cannot fail is worse than no guard. These prove the script +// rejects the shapes it claims to reject, rather than emitting something +// plausible and letting it ship. +func TestVersionScript_RejectsUnusableClocks(t *testing.T) { + for _, tc := range []struct{ name, commit, now string }{ + {"unreadable commit timestamp", "notanumber", "1789000920"}, + {"non-numeric build clock", "1789000920", "abc"}, + {"build clock before 2020", "1789000920", "1000000000"}, + } { + t.Run(tc.name, func(t *testing.T) { + out, err := versionScript(t, tc.commit, tc.now) + if err == nil { + t.Errorf("script succeeded on %s, output: %s", tc.name, out) + } + }) + } +} + +// Pins the wiring, not the formula: if release.yml stops calling the script, +// every assertion above keeps passing while the thing that actually ships goes +// unguarded again. That silent decoupling is the specific regression here. +func TestReleaseWorkflow_UsesTheSharedDerivation(t *testing.T) { + body, err := os.ReadFile(filepath.Join(repoRoot(t), ".gitea", "workflows", "release.yml")) + if err != nil { + t.Fatal(err) + } + yaml := string(body) + + if !strings.Contains(yaml, "ci/version.sh") { + t.Error("release.yml no longer calls ci/version.sh — the derivation has drifted out of test coverage") + } + // The retired scheme, which must not come back. A presence check is safe + // against prose; the historical note in that file names the old formula + // only in comments, so match the executable form. + if strings.Contains(yaml, "$(git rev-list --count") { + t.Error("release.yml derives a commit count again — that is not monotonic across branches") + } +}