From 0ab7d94294f7c763b44520169be3a38c516d7a00 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 28 Aug 2026 21:33:00 -0400 Subject: [PATCH] versioning: refuse to publish a version below what the channel already serves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 5 of M314, note 3127 §6.3. Everything else in this milestone derives a number and trusts it; this compares the derived value against what the channel is actually serving and fails the lane if it went down. Too-low is the unrecoverable direction: every installed client reports "up to date" forever, and no later build fixes it until one climbs back above the bad number. #2183 and #2993 are both that symptom. ## Two hazards, two mechanisms A shallow clone is now tested DIRECTLY, in `version.sh`, via `--is-shallow-repository`. The empty-result guard only caught the case where nothing matched — and run 4796 showed the worse one, where a partial match returned a real six-days-stale answer. Asking the question outright costs no network and covers artifacts with nothing published to compare against. `guard-forward.sh` handles the rest: a squash or rebase merge rewriting the committer date, a rebuild of an older commit, and clock skew between runners. ## The comparison is per artifact, and the operator differs desktop derived >= published commit time, so equality is the ORDINARY no-change case and `<=` would fail every build that changed nothing android derived > published build time, so equality means two builds in one minute — and Android refuses to install an APK whose versionCode does not RISE The server is deliberately unguarded: nothing compares its version, `:latest` moves regardless, and rule 145 removed the version tags that would be the published list. A too-low value there is a wrong date in a footer, not a stranded client. It still gets the shallow-clone check. ## Proved to fire, not assumed Cloned the repo, checked out a commit eight back, ran the guard against the LIVE dev feed: at the tip derived 1.0.3502151, published 1.0.3502151 -> pass eight back derived 1.0.3501535, published 1.0.3502151 -> FAILS android tip derived 3502171, published 3502152 -> pass stable derived 1.0.3502151, published 0.2.0 -> pass That last row is worth keeping: stable still advertises the bare `0.2.0` from the old Cargo.toml scheme, so the transition orders upward on BOTH channels, not just the one being exercised. A channel with nothing published passes rather than failing — otherwise the first publish to a new channel could never happen. The guard runs BEFORE the build in all three lanes, so a bad derivation costs seconds rather than a five-minute compile and a publish to undo. `compare` is exposed as an explicit mode so the ordering is testable without a network and inspectable without a push — 16 cases including `1.0.9 < 1.0.10`, which a string compare gets exactly backwards. #3145 Co-Authored-By: Claude Opus 5 --- .forgejo/workflows/android.yml | 20 +++++ .forgejo/workflows/desktop.yml | 40 +++++++++ packaging/guard-forward.sh | 155 +++++++++++++++++++++++++++++++++ packaging/version.sh | 18 ++++ tests/test_versioning.py | 55 ++++++++++++ 5 files changed, 288 insertions(+) create mode 100755 packaging/guard-forward.sh diff --git a/.forgejo/workflows/android.yml b/.forgejo/workflows/android.yml index 56e997d..0788f0b 100644 --- a/.forgejo/workflows/android.yml +++ b/.forgejo/workflows/android.yml @@ -126,6 +126,26 @@ jobs: echo "apk=android/app/build/outputs/apk/debug/app-debug.apk" >> $GITHUB_OUTPUT fi + # THE ONE CHECK THAT LOOKS AT REALITY (note 3127 §6.3). Everything else in this + # lane derives a number and trusts it; this compares the derived value against + # what the channel is actually serving, and fails the lane if it went DOWN. + # + # Placed before the build, not after: a bad derivation should cost seconds, not + # a five-minute compile and a publish that has to be undone. Too-low is the + # unrecoverable direction — every installed client reports "up to date" forever + # and no later build fixes it until one climbs back above the bad number + # (#2183, #2993). + - name: Guard — the version must not go backwards + if: github.ref == 'refs/heads/dev' || github.ref == 'refs/heads/main' + env: + GITHUB_TOKEN: ${ github.token } + run: | + case "$GITHUB_REF_NAME" in + main) channel=stable ;; + *) channel=dev ;; + esac + sh ../packaging/guard-forward.sh android "$channel" + - name: Make gradlew executable run: chmod +x ./gradlew diff --git a/.forgejo/workflows/desktop.yml b/.forgejo/workflows/desktop.yml index 5815b6f..9fc6959 100644 --- a/.forgejo/workflows/desktop.yml +++ b/.forgejo/workflows/desktop.yml @@ -126,6 +126,26 @@ jobs: # so making it conditional is what lets the pipeline stay green before the # operator has added the secret. With the key present, each bundle gets a # `.sig` beside it — the file the updater actually verifies against. + # THE ONE CHECK THAT LOOKS AT REALITY (note 3127 §6.3). Everything else in this + # lane derives a number and trusts it; this compares the derived value against + # what the channel is actually serving, and fails the lane if it went DOWN. + # + # Placed before the build, not after: a bad derivation should cost seconds, not + # a five-minute compile and a publish that has to be undone. Too-low is the + # unrecoverable direction — every installed client reports "up to date" forever + # and no later build fixes it until one climbs back above the bad number + # (#2183, #2993). + - name: Guard — the version must not go backwards + if: github.ref == 'refs/heads/dev' || github.ref == 'refs/heads/main' + env: + GITHUB_TOKEN: ${ github.token } + run: | + case "$GITHUB_REF_NAME" in + main) channel=stable ;; + *) channel=dev ;; + esac + sh packaging/guard-forward.sh desktop "$channel" + - name: Tauri build (deb + AppImage) env: TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_SIGNING_PRIVATE_KEY }} @@ -328,6 +348,26 @@ jobs: # --runner cargo-xwin swaps cargo for the cross-compiling driver (it supplies # the MSVC CRT/SDK, pre-warmed into the image, and links with lld-link). # Frontend already built above; skip the beforeBuildCommand rebuild. + # THE ONE CHECK THAT LOOKS AT REALITY (note 3127 §6.3). Everything else in this + # lane derives a number and trusts it; this compares the derived value against + # what the channel is actually serving, and fails the lane if it went DOWN. + # + # Placed before the build, not after: a bad derivation should cost seconds, not + # a five-minute compile and a publish that has to be undone. Too-low is the + # unrecoverable direction — every installed client reports "up to date" forever + # and no later build fixes it until one climbs back above the bad number + # (#2183, #2993). + - name: Guard — the version must not go backwards + if: github.ref == 'refs/heads/dev' || github.ref == 'refs/heads/main' + env: + GITHUB_TOKEN: ${ github.token } + run: | + case "$GITHUB_REF_NAME" in + main) channel=stable ;; + *) channel=dev ;; + esac + sh packaging/guard-forward.sh desktop "$channel" + - name: Tauri build (NSIS installer) env: TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_SIGNING_PRIVATE_KEY }} diff --git a/packaging/guard-forward.sh b/packaging/guard-forward.sh new file mode 100755 index 0000000..3328bd3 --- /dev/null +++ b/packaging/guard-forward.sh @@ -0,0 +1,155 @@ +#!/usr/bin/env sh +# +# Refuse to publish a version lower than the one already on the channel. +# +# guard-forward.sh +# guard-forward.sh compare exit 0 iff a sorts strictly below b +# +# Note 3127 §6.3. Everything else in this milestone derives a number and trusts it; +# this is the one thing that checks the answer against reality before a user gets it. +# +# WHAT IT CATCHES that nothing else does: +# +# * A SQUASH OR REBASE MERGE (§6.2). Both rewrite the committer date, so `main` +# could stamp a value unrelated to the dev commit it merged. Rule 153 mandates +# plain merge commits — but that rule governs people, and a forge UI's squash +# button does not read it. +# * A REBUILD OF AN OLDER COMMIT. Commit time can go backwards; this is the entire +# mitigation for the desktop key's clock choice (step 4), and the thing to +# revisit first if this repo ever starts rebuilding old commits routinely. +# * CLOCK SKEW between runners, for a build-time key. +# +# What it does NOT catch, because something better does: a shallow clone. That is +# tested directly in `version.sh` via `--is-shallow-repository`, which needs no +# network and covers artifacts that have no published value to compare against. +# +# TOO-LOW IS THE UNRECOVERABLE DIRECTION. A version below what is published means +# every installed client reports "up to date" forever and there is no build you can +# ship to fix it — you have to get back ABOVE the bad number. That is #2183 and +# #2993's shared symptom, and it is why this fails the lane rather than warning. +set -eu + +ROOT="$(cd "$(dirname "$0")/.." && pwd)" +SERVER="${GITHUB_SERVER_URL:-https://git.fabledsword.com}" +REPO="${GITHUB_REPOSITORY:-bvandeusen/thoughtsync}" + +artifact="${1:?usage: guard-forward.sh }" + +# True when $1 sorts strictly below $2, comparing NUMERICALLY per dot-segment. +# +# Not a string compare, which is the classic way to get this wrong: `1.0.10` sorts +# below `1.0.9` as text. A missing segment reads as 0, so `1.0` == `1.0.0`. +version_lt() { + _a="$1"; _b="$2" + while [ -n "$_a" ] || [ -n "$_b" ]; do + if [ "${_a%%.*}" = "$_a" ]; then _ah="$_a"; _at=""; else _ah="${_a%%.*}"; _at="${_a#*.}"; fi + if [ "${_b%%.*}" = "$_b" ]; then _bh="$_b"; _bt=""; else _bh="${_b%%.*}"; _bt="${_b#*.}"; fi + [ -n "$_ah" ] || _ah=0 + [ -n "$_bh" ] || _bh=0 + if [ "$_ah" -lt "$_bh" ]; then return 0; fi + if [ "$_ah" -gt "$_bh" ]; then return 1; fi + _a="$_at"; _b="$_bt" + done + return 1 # equal +} + +# An explicit comparison mode, so the ordering logic is testable without a network +# and inspectable without a push. Read-only and bypasses nothing — it is the same +# function the guard itself uses, which is the point: a test of a reimplementation +# would prove nothing about the code that runs. +if [ "$artifact" = "compare" ]; then + a="${2:?usage: guard-forward.sh compare }" + b="${3:?usage: guard-forward.sh compare }" + if version_lt "$a" "$b"; then exit 0; else exit 1; fi +fi + +channel="${2:?usage: guard-forward.sh }" + +case "$artifact" in desktop|android) : ;; *) + echo "guard-forward.sh: unknown artifact '$artifact'" >&2; exit 2 ;; +esac +case "$channel" in dev|stable) : ;; *) + echo "guard-forward.sh: unknown channel '$channel'" >&2; exit 2 ;; +esac + +BASE="$SERVER/$REPO/releases/download/$channel" + +# Auth if we have it, anonymous if not — the releases are public, but a token costs +# nothing and keeps this working if that ever changes. +fetch() { + if [ -n "${GITHUB_TOKEN:-}" ]; then + curl -fsSL -H "Authorization: token $GITHUB_TOKEN" "$1" 2>/dev/null || true + else + curl -fsSL "$1" 2>/dev/null || true + fi +} + + +case "$artifact" in + desktop) + derived="$(sh "$ROOT/packaging/version.sh" key desktop)" + # What the UPDATER reads, not what the release happens to hold — the manifest is + # the thing that decides whether a client is offered this build. + published="$(fetch "$BASE/latest.json" \ + | grep -oE '"version"[[:space:]]*:[[:space:]]*"[^"]+"' | head -1 \ + | sed -E 's/.*"([^"]+)"$/\1/')" + # COMMIT time, so EQUALITY IS THE ORDINARY CASE: an unchanged source derives + # exactly what it derived last time, and `<=` would fail every no-change build. + # §6.3 says *strictly* less for exactly this reason. + strict="" + ;; + android) + derived="$(sh "$ROOT/packaging/version.sh" key android)" + published="$(fetch "$BASE/thoughtsync-android.json" \ + | grep -oE '"version_code"[[:space:]]*:[[:space:]]*[0-9]+' | head -1 \ + | grep -oE '[0-9]+$')" + # BUILD time, so equality is NOT ordinary — it means two builds landed in the + # same minute, and Android refuses to install an APK whose versionCode does not + # RISE. So this one requires strictly greater. + # + # If it ever fires, the cheap fix is seconds rather than minutes in version.sh + # (~210M today against Android's 2.1e9 ceiling, so ~60 years of headroom). + # Not done pre-emptively: the concurrency group cancels older runs on a branch, + # so two builds finishing in one minute needs concurrent runs on different + # branches, and the failure is a refused install rather than a stranded channel. + strict="yes" + ;; +esac + +if [ -z "$published" ]; then + # A channel with nothing on it yet — `stable` before its first merge, or a fresh + # repo. PASS: there is nothing to go backwards from. Failing here would block the + # very first publish to a channel, which is the one case where "lower than what is + # published" is meaningless. + echo "guard: $channel has no published $artifact version yet — nothing to compare." + echo "guard: publishing $derived." + exit 0 +fi + +echo "guard: $artifact on $channel — derived $derived, published $published" + +if version_lt "$derived" "$published"; then + echo "" >&2 + echo "GUARD FAILED: $derived is BELOW the published $published on $channel." >&2 + echo "" >&2 + echo " Publishing it would leave every installed client reporting 'up to date'" >&2 + echo " forever, and no later build fixes that until one climbs back above the" >&2 + echo " bad number. Do not force past this." >&2 + echo "" >&2 + echo " Usual causes (note 3127 §6.2, §6.3):" >&2 + echo " - a squash or rebase merge rewrote the committer date" >&2 + echo " - this build is a rebuild of an older commit" >&2 + echo " - clock skew between runners (build-time keys)" >&2 + exit 1 +fi + +if [ -n "$strict" ] && [ "$derived" = "$published" ]; then + echo "" >&2 + echo "GUARD FAILED: $derived EQUALS the published $published on $channel." >&2 + echo "" >&2 + echo " Android requires versionCode to RISE; an equal one cannot be installed" >&2 + echo " over what is already out there. Two builds landed in the same minute." >&2 + exit 1 +fi + +echo "guard: ok — $derived may be published." diff --git a/packaging/version.sh b/packaging/version.sh index ac63984..d6beafc 100755 --- a/packaging/version.sh +++ b/packaging/version.sh @@ -58,6 +58,24 @@ set -eu # match nothing from `android/`. Same bug, louder symptom, pure luck. cd "$(git rev-parse --show-toplevel)" +# A SHALLOW CLONE IS FATAL, and asked directly rather than inferred. +# +# Landmine §6.1: depth-1 sees one commit, so `git log -- ` answers about +# whatever happens to be in that commit and the result is a too-LOW version — the +# unrecoverable direction, arrived at silently with every lane green. +# +# The empty-result guard below catches only the case where NOTHING matches. It missed +# the worse one: on run 4796 a partial match returned a real, six-days-stale answer. +# `--is-shallow-repository` tests the actual hazard instead of a symptom of it, costs +# no network, and covers every artifact including the ones with no published value to +# compare against. +if [ "$(git rev-parse --is-shallow-repository)" = "true" ]; then + echo "version.sh: this is a SHALLOW clone — any version derived here would be" >&2 + echo " too low, silently. Add 'fetch-depth: 0' to the checkout." >&2 + echo " (note 3127 §6.1)" >&2 + exit 1 +fi + # 2020-01-01T00:00:00Z. The counter epoch, and it must NEVER move: shifting it # renumbers every artifact downwards, which is the one direction you cannot recover # from (note 3127 §6.4). diff --git a/tests/test_versioning.py b/tests/test_versioning.py index 6e5307d..ab7d404 100644 --- a/tests/test_versioning.py +++ b/tests/test_versioning.py @@ -23,6 +23,7 @@ from pathlib import Path import pytest SCRIPT = Path(__file__).resolve().parent.parent / "packaging" / "version.sh" +GUARD = Path(__file__).resolve().parent.parent / "packaging" / "guard-forward.sh" # 2020-01-01T00:00:00Z, the counter epoch. Duplicated from the script deliberately: # a test that imported the value could not catch the value being changed, and moving @@ -249,3 +250,57 @@ def test_no_matching_history_fails_rather_than_guessing(tmp_path: Path, what: st assert r.returncode != 0, f"{what} exited 0 with stdout={r.stdout!r}" assert "shallow" in r.stderr assert r.stdout.strip() == "", f"{what} emitted a value anyway: {r.stdout!r}" + + +# --- the backwards guard's comparison ---------------------------------------- +# +# Exercised through the guard's own `compare` mode rather than a reimplementation +# here: a test of a copy proves nothing about the code that runs. No network — the +# comparison is pure, and the fetch/compare halves are separable for exactly this. + + +def compare(a: str, b: str) -> bool: + """True when the guard considers `a` to sort strictly below `b`.""" + return subprocess.run(["sh", str(GUARD), "compare", a, b], + capture_output=True, text=True).returncode == 0 + + +@pytest.mark.parametrize("a,b,expect_lt", [ + # THE trap: as text, "1.0.9" > "1.0.10". The comparison must be numeric + # per dot-segment, which is what note 3127 §1 spells out and what a naive + # `[ "$a" \< "$b" ]` would get exactly backwards. + ("1.0.9", "1.0.10", True), + ("1.0.10", "1.0.9", False), + # Equal is NOT less. Under commit time an unchanged source derives what it + # derived last time, so this is the ordinary no-change build. + ("1.0.5", "1.0.5", False), + # A missing segment reads as zero. + ("1.0", "1.0.0", False), + ("1.0.0", "1.0", False), + ("1.0", "1.0.1", True), + # The real transition this milestone performs, on both channels: dev was + # publishing 0.2., stable was on the bare 0.2.0 from Cargo.toml. + ("0.2.466", "1.0.3502151", True), + ("0.2.0", "1.0.3502151", True), + ("1.0.3502151", "0.2.466", False), + # Android codes are bare integers. + ("3502151", "3502152", True), + ("3502152", "3502151", False), + # Zero-padded display versions compare correctly despite the leading zeros + # (which is why they are stripped on parse rather than compared as text). + ("2026.08.29.0110", "2026.08.29.0111", True), + ("2026.08.29.0111", "2026.08.29.0110", False), + ("2026.08.09.0111", "2026.08.10.0111", True), + ("2026.12.31.2359", "2027.01.01.0000", True), +]) +def test_the_guard_orders_versions_numerically(a: str, b: str, expect_lt: bool) -> None: + assert compare(a, b) is expect_lt + + +@pytest.mark.parametrize("args", [["compare"], ["compare", "1.0.0"], ["nope", "dev"], + ["desktop", "nope"]]) +def test_the_guard_rejects_bad_invocations(args: list[str]) -> None: + """Including the two-place validation the version script needed twice — a guard + that answers confidently for input it does not understand is worse than none.""" + r = subprocess.run(["sh", str(GUARD), *args], capture_output=True, text=True) + assert r.returncode != 0