From bfdaed936556269ea77c1a3016097ab4b648c565 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 21:37:30 -0400 Subject: [PATCH 1/8] fix(release): derive versionCode from build time, versionName from commit time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit versionCode was `git rev-list --count HEAD`, and build.gradle.kts called it "monotonic forever". It is not, and that claim was sitting directly above the bug it denied. A commit count runs ahead on `dev`. So a dev build carried a HIGHER code than the `main` release meant to supersede it, and Android refuses that install as a downgrade — a channel you can enter and cannot leave without uninstalling and losing local data. Two clocks now, and the split is deliberate even though it reads like an inconsistency: The NAME answers "is this the same code?", so it derives from COMMIT time and reads identically on every lane building 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. The ORDERING KEY answers "may this be installed over that?", so it must be monotonic BY CONSTRUCTION: minutes since 2020-01-01. Commit time fails here for the mirror-image reason — rebuild an older commit and it goes DOWN, which on a phone is a refused install rather than a confusing label. The non-tag :latest path reconstructed the bundled APK's name with the old formula, so it is moved to the same commit-timestamp derivation. That duplication is temporary: once the tag becomes `v` it collapses to `${TAG#v}` with nothing left to keep in step. Verified locally by running the derivations rather than reasoning about them: HEAD yields 2026.09.09.1828; the key yields 3519456 against ~1895 from the old scheme, inside int32 with ~4000 years of headroom; a commit at 00:42 UTC yields "0042", not "42". The workflow now asserts the emitted shape too — a malformed name builds, signs and publishes happily and only surfaces as an update nobody is offered, which nobody reports. That local check is the only verification this commit gets. release.yml triggers on main and tags only, so nothing on `dev` executes the new derivation; CI here proves the Gradle file still parses and nothing else. Also confirms the migration constraint recorded in milestone #390: this commit would name a release 2026.09.09.1828, which is LOWER than the installed 2026.09.09.1895 under name comparison. The first new-scheme release must be cut on a later calendar day, or existing installs will never be offered it. Step 1 of 5 — Scribe task #3808, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .gitea/workflows/release.yml | 88 ++++++++++++++++++++++++++++-------- android/app/build.gradle.kts | 25 +++++++--- 2 files changed, 86 insertions(+), 27 deletions(-) diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index 6e89d541..050f3fa3 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -24,9 +24,10 @@ name: release # :latest (not just tags), a main build with no APK would silently strip # the in-app update channel off :latest until the next release. So on # non-tag builds image-release pulls the MOST RECENT release's signed APK -# and reconstructs its exact versionName (tag + commit-count, the same -# formula android-release bakes in) for the version sidecar — no rebuild, -# just rebundle. Tag builds keep bundling their own freshly-built APK. +# and reconstructs its exact versionName from the tagged commit's timestamp +# (the same derivation android-release bakes in) for the version sidecar — +# no rebuild, just rebundle. Tag builds keep bundling their own +# freshly-built APK. # # Android testing (lint + detekt + unit tests, debug APK upload on main) # lives in android.yml and runs independently on every push. @@ -80,9 +81,12 @@ jobs: - name: Checkout uses: actions/checkout@v4 with: - # fetch-depth: 0 retrieves full history; default shallow clone - # would return 1 for `git rev-list --count HEAD`, breaking the - # iteration suffix. + # Full history. The version name now reads only the tip commit's + # timestamp, so a shallow clone would technically serve — but this + # job derives a value that ships to devices, and a shallow checkout + # changes what git-derived values resolve to WITHOUT failing. The + # whole failure class here is a green build carrying a wrong + # version, so the cheap guarantee is worth keeping. fetch-depth: 0 - name: Compute release version @@ -91,12 +95,45 @@ jobs: working-directory: ${{ github.workspace }} run: | set -euo pipefail - TAG="${GITHUB_REF#refs/tags/v}" - COMMIT_COUNT=$(git rev-list --count HEAD) - VERSION_NAME="${TAG}.${COMMIT_COUNT}" + + # 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=${COMMIT_COUNT}" >> "$GITHUB_OUTPUT" - echo "::notice::APK version: ${VERSION_NAME} (code=${COMMIT_COUNT})" + echo "code=${VERSION_CODE}" >> "$GITHUB_OUTPUT" + echo "::notice::APK version: ${VERSION_NAME} (code=${VERSION_CODE})" # 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 — @@ -223,8 +260,9 @@ jobs: uses: actions/checkout@v4 with: # Full history + tags so non-tag :latest builds can resolve the - # latest release tag's commit count and reconstruct the bundled - # APK's exact versionName (see "Bundle latest release APK" below). + # latest release tag's commit and reconstruct the bundled APK's + # exact versionName from its timestamp (see "Bundle latest release + # APK" below). fetch-depth: 0 fetch-tags: true @@ -309,9 +347,10 @@ jobs: # Main pushes don't build an APK, but they DO move :latest — so # without this the in-app update channel would vanish from :latest # until the next tag. Pull the most-recent release's signed APK and - # reconstruct its exact versionName (${TAG#v}.$(git rev-list --count - # TAG) — identical to android-release's formula) so the version - # sidecar the server hands clients matches the installed build. + # reconstruct its exact versionName from the tagged commit's + # timestamp — the same derivation android-release uses — so the + # version sidecar the server hands clients matches the installed + # build. # Degrades to an empty client/ (404 update channel) — never a wrong # version — if no release / APK asset / tag-count can be resolved. if: steps.guard.outputs.ready == 'true' && !startsWith(github.ref, 'refs/tags/v') @@ -331,11 +370,20 @@ jobs: if [ -z "${TAG}" ] || [ -z "${APK_URL}" ]; then echo "::notice::latest release '${TAG:-?}' has no APK asset — image ships without bundled APK"; exit 0 fi - COUNT="$(git rev-list --count "${TAG}" 2>/dev/null || true)" - if [ -z "${COUNT}" ]; then - echo "::notice::could not resolve commit count for ${TAG} (tag not fetched?) — skipping APK bundle"; exit 0 + # Reconstruct the bundled APK's name with the SAME derivation + # android-release uses — commit timestamp of the tagged commit. The + # two must agree exactly: this string is what the server hands + # clients to compare against what is installed, so a mismatch here + # is an update offered forever or never offered at all. + # + # This duplication is temporary. Once the tag itself becomes + # `v`, this whole block collapses to `${TAG#v}` with + # nothing to recompute and nothing to keep in step. + COMMIT_TS="$(git log --format=%ct -1 "${TAG}" 2>/dev/null || true)" + if [ -z "${COMMIT_TS}" ]; then + echo "::notice::could not resolve commit timestamp for ${TAG} (tag not fetched?) — skipping APK bundle"; exit 0 fi - VERSION_NAME="${TAG#v}.${COUNT}" + VERSION_NAME="$(date -u -d "@${COMMIT_TS}" +%Y.%m.%d.%H%M)" curl -fsSL -H "Authorization: token ${CI_TOKEN}" -o client/minstrel.apk "${APK_URL}" echo "${VERSION_NAME}" > client/minstrel.apk.version echo "::notice::bundled release APK ${TAG} as version ${VERSION_NAME}" diff --git a/android/app/build.gradle.kts b/android/app/build.gradle.kts index 4ab52a4b..dc6d264b 100644 --- a/android/app/build.gradle.kts +++ b/android/app/build.gradle.kts @@ -21,13 +21,24 @@ android { applicationId = "com.fabledsword.minstrel" minSdk = 26 targetSdk = 36 - // versionName / versionCode are released-build values injected by - // CI from the git tag + commit count. Local / debug builds fall - // back to "dev" so the About card reads honestly. Releases ship - // versionName="YYYY.MM.DD." (e.g. "2026.06.02.142") and - // versionCode=, which is monotonic forever and lets the - // shared isVersionNewer comparator distinguish two same-day - // re-cuts (the iteration suffix differs). + // versionName / versionCode are released-build values injected by CI. + // Local / debug builds fall back to "dev" so the About card reads + // honestly. + // + // versionName is "YYYY.MM.DD.HHMM" from the COMMIT's timestamp, so + // every lane building this source reports the same string and the + // channel is the only thing that differs between them. + // + // versionCode is minutes since 2020-01-01 at BUILD time. It is the + // value the platform decides installs by, so it must be monotonic by + // construction. + // + // This comment used to say versionCode was a commit count and that it + // was "monotonic forever". It was neither — a commit count runs ahead + // on `dev`, so a dev build outranked the `main` release meant to + // replace it and Android refused the install as a downgrade. Worth + // knowing the claim was here, stated as a reassurance, while the bug + // it denied was live. val versionNameOverride = (project.findProperty("MINSTREL_VERSION_NAME") as String?)?.takeIf { it.isNotBlank() } val versionCodeOverride = -- 2.54.0 From e46c6bcccf0755b359953666278d53a2c8158f6c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 21:43:33 -0400 Subject: [PATCH 2/8] docs(release): tags become vYYYY.MM.DD.HHMM, and stop telling people to move them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tag is now the artifact's own version name with a `v` in front, so `v2026.09.10.1432` and `2026.09.10.1432` are one string. Nothing has to reconcile what the tag claims against what the APK reports, and minting one is arithmetic on the tagged commit's timestamp rather than a lookup. The substantive change is the prose. release.yml's header instructed the reader to `git push -f origin vYYYY.MM.DD` on a same-day re-cut. That is the operation the family rulebook forbids outright, and it has incidents behind it — moving a same-day tag forward once took a published release down with it. Anyone who had installed from that tag was holding something it no longer pointed at. With HHMM there is nothing left for mutability to buy: every tag is unique by construction, so a second release the same day is not a collision to resolve, just another tag. The old instruction is recorded as retired rather than deleted. Someone who remembers it should learn it was withdrawn and why, not find it silently absent and assume they misremembered. README contradicted itself inside one sentence — "immutable per-day release tags ... a same-day re-cut moves the tag forward" — and now says which it is, plus a note that pre-2026-09-10 tags keep the old shape and still work. Transition wrinkle, deliberately left for step 3: the non-tag :latest path reconstructs the bundled APK's name from the latest release's commit timestamp, which for the one existing old-shape release yields 2026.09.09.1828 while that APK actually declares 2026.09.09.1895. It fails SAFE — 1828 compares lower, so no false update is offered — and it self-corrects at the first new-scheme release. Step 3 removes the reconstruction entirely by having the sidecar carry recorded values instead of derived ones. Step 2 of 5 — Scribe task #3809, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .gitea/workflows/release.yml | 42 +++++++++++++++++++++++++----------- README.md | 6 ++++-- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index 050f3fa3..bd2418b2 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -2,15 +2,30 @@ name: release # Builds and pushes the minstrel container image to the Gitea registry. # -# push to main → :main and :latest (latest-release APK bundled) -# push tag vYYYY.MM.DD → :vYYYY.MM.DD and :latest (freshly-built APK bundled) -# workflow_dispatch → manual trigger (same rules based on the ref) +# push to main → :main and :latest (latest-release APK bundled) +# push tag vYYYY.MM.DD.HHMM → :vYYYY.MM.DD.HHMM and :latest (fresh APK bundled) +# workflow_dispatch → manual trigger (same rules based on the ref) # -# Release model: per-day CalVer tags (no trailing patch digit). The day's -# tag is intentionally mutable — if a second release happens the same day, -# move the tag with `git push -f origin vYYYY.MM.DD` and the image tag of -# the same name gets overwritten. :latest is updated by every main push -# AND every tag push, so it always reflects the newest blessed image. +# Release model: the tag IS the artifact's version name with a `v` in front. +# `v2026.09.10.1432` and `2026.09.10.1432` are the same string, derived from +# the tagged commit's UTC timestamp — so there is no mismatch to reconcile +# between what the tag says and what the APK reports, and nothing to look up +# when minting one. +# +# TAGS ARE IMMUTABLE. Never move, retarget or delete a published tag. A +# same-day second release is not a collision — HHMM makes every tag unique +# by construction, so the answer is simply another tag. +# +# This block used to say the opposite: that the per-day tag was +# "intentionally mutable" and that a same-day re-cut should +# `git push -f origin vYYYY.MM.DD`. That instruction is what the family +# rulebook now forbids outright, and it has incidents behind it — moving a +# same-day tag forward once took a published release down with it. Anyone +# installing from a tag is holding something the tag no longer points at, +# which is a worse failure than an extra row in the tag list. +# +# :latest is updated by every main push AND every tag push, so it always +# reflects the newest blessed image. # # APK pipeline: on tag pushes the android-release job builds + signs the # Android APK and uploads it as a workflow artifact. The image-release @@ -41,9 +56,10 @@ on: - '**/*.md' workflow_dispatch: -# Force-moving the per-day tag (or rapidly re-pushing to main) should -# supersede the in-flight build — the operator explicitly wants the -# later commit to win. +# A rapid re-push to main should supersede the in-flight build — the +# operator explicitly wants the later commit to win. Tags no longer enter +# into this: they are immutable and unique, so no tag build can ever be +# superseded by another run on the same ref. concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true @@ -290,8 +306,8 @@ jobs: else # Main is the protected, post-PR-merge branch. Treat it as the # rolling stable channel — every main push moves :latest. - # Pinned consumers can target :vYYYY.MM.DD; everyone else - # gets the newest main. + # Pinned consumers can target :vYYYY.MM.DD.HHMM, which never + # moves; everyone else gets the newest main. echo "args=-t ${IMAGE}:main -t ${IMAGE}:latest" >> "$GITHUB_OUTPUT" echo "version=main" >> "$GITHUB_OUTPUT" echo "::notice::Main-branch build: :main + :latest" diff --git a/README.md b/README.md index 58db87d4..14febf77 100644 --- a/README.md +++ b/README.md @@ -113,10 +113,12 @@ Most operational keys have a `MINSTREL_
_` env override. Recommen Image tags (`git.fabledsword.com/bvandeusen/minstrel:`): - `:latest` — the newest blessed image. Moves on every `main` push **and** every release. Recommended for most operators. -- `:vYYYY.MM.DD` — immutable per-day release tags. Pin one of these for a deployment you don't want moving under you. (Per-day CalVer — no trailing patch digit; a same-day re-cut moves the tag forward.) +- `:vYYYY.MM.DD.HHMM` — immutable release tags, never moved or deleted. Pin one for a deployment you don't want changing under you. The tag is the build's own version name with a `v` in front, derived from the tagged commit's UTC timestamp, so two releases can never collide and a re-cut is simply a new tag. - `:main` — the rolling post-merge tip. Same image as `:latest` at push time; choose it if you want to track `main` explicitly rather than the release line. -Every `:latest` and every `:vYYYY.MM.DD` bundles the current signed Android APK, so the in-app update channel is always live. Database migrations run automatically at startup; rollbacks require restoring a Postgres dump. +Every `:latest` and every `:vYYYY.MM.DD.HHMM` bundles the current signed Android APK, so the in-app update channel is always live. Database migrations run automatically at startup; rollbacks require restoring a Postgres dump. + +Releases before 2026-09-10 use the older per-day `:vYYYY.MM.DD` shape. Those tags still exist and still work — they are simply not extended. ## Specs -- 2.54.0 From 9f3e0b8cd34fbc15d4c38cccde1721db3bda5d15 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 21:51:41 -0400 Subject: [PATCH 3/8] feat(version): sidecar and /api/client/version carry name, code and channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The client compares names while Android installs by versionCode, and the wire had no way to close that gap: the sidecar was one positional line and the endpoint returned a name only. This is the plumbing that makes the ordering key decidable by the client at all. The sidecar is now JSON rather than a grown positional string. That shape was chosen against a specific failure: the obvious growth path was " ", which a first-space split silently mangles the moment a third field appears — the code stops parsing as an integer and the reader falls back to name comparison WITHOUT erroring. JSON cannot mistake a new field for an old one. code is a POINTER on both sides, and omitempty on the wire. Absent has to stay distinguishable from zero: a build published before ordering keys were recorded genuinely has no code, and zero would claim it is infinitely old rather than unknown. A malformed sidecar now fails loudly instead of serving a blank version. If an unreadable file produced an empty name, every client would compare against nothing, conclude it was current, and go quiet — "I cannot read this" and "there is nothing newer" would return the same answer, which is the failure mode nobody reports because nobody is offered anything to report. The non-tag :latest path no longer RECONSTRUCTS the bundled APK's version. android-release now publishes the sidecar as a release asset beside the APK, and the image build downloads it. The old reconstruction duplicated a derivation formula across two files, and could only ever recover the name — the ordering key is build-time minutes and exists nowhere once that build ends. Releases predating the sidecar report their name with a null code, which is the honest answer rather than a guessed one. image-release also drops to a shallow checkout: it needed full history and tags only to re-derive versions from the tagged commit, and now touches git for nothing. MINSTREL_VERSION comes from GITHUB_REF. Two things checked rather than assumed. The Android Json sets ignoreUnknownKeys, so the added fields cannot break already-installed apps. It also sets coerceInputValues, which will silently turn a null code into 0 if step 4 declares the field non-nullable — recorded on task #3811, because reading the field declaration alone would never reveal it. Also fixes a stale comment block describing "the Flutter client", deleted in v2026.08.18. Step 3 of 5 — Scribe task #3810, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .gitea/workflows/release.yml | 104 +++++++++++++++++++---------- internal/api/client_assets.go | 61 +++++++++++++++-- internal/api/client_assets_test.go | 81 ++++++++++++++++++++-- 3 files changed, 198 insertions(+), 48 deletions(-) diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index bd2418b2..c0bcdfaa 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -39,10 +39,9 @@ name: release # :latest (not just tags), a main build with no APK would silently strip # the in-app update channel off :latest until the next release. So on # non-tag builds image-release pulls the MOST RECENT release's signed APK -# and reconstructs its exact versionName from the tagged commit's timestamp -# (the same derivation android-release bakes in) for the version sidecar — -# no rebuild, just rebundle. Tag builds keep bundling their own -# freshly-built APK. +# AND the version sidecar published beside it — the recorded values, not +# recomputed ones — so no rebuild is needed, just a rebundle. Tag builds +# keep bundling their own freshly-built APK. # # Android testing (lint + detekt + unit tests, debug APK upload on main) # lives in android.yml and runs independently on every push. @@ -227,6 +226,8 @@ jobs: shell: bash env: CI_TOKEN: ${{ secrets.CI_TOKEN }} + VERSION_NAME: ${{ steps.ver.outputs.name }} + VERSION_CODE: ${{ steps.ver.outputs.code }} run: | set -euxo pipefail TAG="${GITHUB_REF#refs/tags/}" @@ -234,6 +235,20 @@ jobs: APK_PATH="app/build/outputs/apk/release/app-release.apk" ls -lh "${APK_PATH}" + # Publish the version sidecar as a release asset next to the APK. + # + # This is what lets a later :latest build stop RECONSTRUCTING the + # bundled APK's version and simply read what was recorded. The + # ordering key in particular cannot be re-derived after the fact — + # it is build-time minutes, so once this job ends the value exists + # nowhere else. Reconstruction could only ever recover the name, + # and only by duplicating a formula that then has to be kept in + # step across two files. + SIDECAR_PATH="/tmp/minstrel.apk.version" + printf '{"name":"%s","code":%s,"channel":"stable"}\n' \ + "${VERSION_NAME}" "${VERSION_CODE}" > "${SIDECAR_PATH}" + cat "${SIDECAR_PATH}" + RELEASE_JSON="$(curl -fsSL \ -H "Authorization: token ${CI_TOKEN}" \ "https://git.fabledsword.com/api/v1/repos/${REPO}/releases/tags/${TAG}")" @@ -255,6 +270,20 @@ jobs: exit 1 fi + # Same treatment for the sidecar. Named `.apk.version` so the + # downloader's `\.apk$` match cannot pick it up by mistake. + SIDECAR_HTTP=$(curl -sS -L -o /tmp/upload-sidecar.out -w '%{http_code}' \ + -H "Authorization: token ${CI_TOKEN}" \ + -F "attachment=@${SIDECAR_PATH}" \ + "https://git.fabledsword.com/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=minstrel-${TAG}.apk.version") + echo "sidecar_upload_http=${SIDECAR_HTTP}" + cat /tmp/upload-sidecar.out || true + echo + if [ "${SIDECAR_HTTP}" -lt 200 ] || [ "${SIDECAR_HTTP}" -ge 300 ]; then + echo "::error::version sidecar upload returned HTTP ${SIDECAR_HTTP}" + exit 1 + fi + image-release: name: Build + push container image # `needs:` waits for android-release. For tag pushes android-release @@ -275,12 +304,11 @@ jobs: - name: Checkout uses: actions/checkout@v4 with: - # Full history + tags so non-tag :latest builds can resolve the - # latest release tag's commit and reconstruct the bundled APK's - # exact versionName from its timestamp (see "Bundle latest release - # APK" below). - fetch-depth: 0 - fetch-tags: true + # Shallow is fine here. This job used to need full history + tags to + # re-derive the bundled APK's version from the tagged commit; it now + # downloads the sidecar the release recorded, and touches git for + # nothing. MINSTREL_VERSION comes from GITHUB_REF, not from git. + fetch-depth: 1 - name: Detect buildable project id: guard @@ -346,29 +374,30 @@ jobs: if: steps.guard.outputs.ready == 'true' && startsWith(github.ref, 'refs/tags/v') shell: bash env: - # Pulled from android-release.outputs.version_name so the - # sidecar string the server hands clients matches the - # versionName baked into the APK they're comparing against. + # Both pulled from android-release's outputs so the sidecar the + # server hands clients matches exactly what is baked into the APK + # they are comparing against. APK_VERSION_NAME: ${{ needs.android-release.outputs.version_name }} + APK_VERSION_CODE: ${{ needs.android-release.outputs.version_code }} run: | set -euxo pipefail # The artifact lands as `app-release.apk` (the original Gradle # output name). The Dockerfile COPYs client/* into /app/client/ # and the server reads minstrel.apk + minstrel.apk.version. mv client/app-release.apk client/minstrel.apk - echo "${APK_VERSION_NAME}" > client/minstrel.apk.version + printf '{"name":"%s","code":%s,"channel":"stable"}\n' \ + "${APK_VERSION_NAME}" "${APK_VERSION_CODE}" > client/minstrel.apk.version + cat client/minstrel.apk.version ls -lh client/ - name: Bundle latest release APK (non-tag :latest builds) # Main pushes don't build an APK, but they DO move :latest — so # without this the in-app update channel would vanish from :latest # until the next tag. Pull the most-recent release's signed APK and - # reconstruct its exact versionName from the tagged commit's - # timestamp — the same derivation android-release uses — so the - # version sidecar the server hands clients matches the installed - # build. + # the sidecar published beside it, so what the server reports is what + # that build actually recorded rather than something re-derived here. # Degrades to an empty client/ (404 update channel) — never a wrong - # version — if no release / APK asset / tag-count can be resolved. + # version — if no release or APK asset can be resolved. if: steps.guard.outputs.ready == 'true' && !startsWith(github.ref, 'refs/tags/v') shell: bash env: @@ -386,23 +415,28 @@ jobs: if [ -z "${TAG}" ] || [ -z "${APK_URL}" ]; then echo "::notice::latest release '${TAG:-?}' has no APK asset — image ships without bundled APK"; exit 0 fi - # Reconstruct the bundled APK's name with the SAME derivation - # android-release uses — commit timestamp of the tagged commit. The - # two must agree exactly: this string is what the server hands - # clients to compare against what is installed, so a mismatch here - # is an update offered forever or never offered at all. - # - # This duplication is temporary. Once the tag itself becomes - # `v`, this whole block collapses to `${TAG#v}` with - # nothing to recompute and nothing to keep in step. - COMMIT_TS="$(git log --format=%ct -1 "${TAG}" 2>/dev/null || true)" - if [ -z "${COMMIT_TS}" ]; then - echo "::notice::could not resolve commit timestamp for ${TAG} (tag not fetched?) — skipping APK bundle"; exit 0 - fi - VERSION_NAME="$(date -u -d "@${COMMIT_TS}" +%Y.%m.%d.%H%M)" curl -fsSL -H "Authorization: token ${CI_TOKEN}" -o client/minstrel.apk "${APK_URL}" - echo "${VERSION_NAME}" > client/minstrel.apk.version - echo "::notice::bundled release APK ${TAG} as version ${VERSION_NAME}" + + # Take the version the release RECORDED rather than recomputing it. + # This used to re-derive the name from the tagged commit, which meant + # the formula lived in two files that had to be kept in step, and it + # could only ever recover the name — the ordering key is build-time + # minutes and does not exist anywhere after that build ends. + SIDECAR_URL="$(printf '%s' "${REL_JSON}" | grep -oP '"browser_download_url":\s*"\K[^"]+' | grep -E '\.apk\.version$' | head -1)" + if [ -n "${SIDECAR_URL}" ]; then + curl -fsSL -H "Authorization: token ${CI_TOKEN}" -o client/minstrel.apk.version "${SIDECAR_URL}" + cat client/minstrel.apk.version + else + # Releases published before sidecars were attached. Their name is + # still recoverable from the tag, but their ordering key genuinely + # is not — so it is reported ABSENT rather than guessed. A wrong + # key is an install the platform refuses; an absent one just tells + # the client to fall back to comparing names, which is exactly + # what those builds already do. + echo "::notice::release ${TAG} predates the version sidecar — bundling with name only, no ordering key" + printf '{"name":"%s","code":null,"channel":"stable"}\n' "${TAG#v}" > client/minstrel.apk.version + fi + echo "::notice::bundled release APK from ${TAG}" ls -lh client/ - name: Build and push diff --git a/internal/api/client_assets.go b/internal/api/client_assets.go index 1e25dc69..5f7e8c39 100644 --- a/internal/api/client_assets.go +++ b/internal/api/client_assets.go @@ -6,19 +6,21 @@ package api // /app/client/ at image build time. // // Both endpoints are authenticated — the bandwidth cost of the APK -// (~30-60 MB) makes anonymous access an abuse vector. The Flutter -// client's polling only fires after login (banner mounts in the post- -// login shell), so this gate is invisible to the actual update flow. +// (~30-60 MB) makes anonymous access an abuse vector. The client only +// polls after login, so this gate is invisible to the actual update flow. // // /api/client/apk additionally rate-limits per user to a single // download every 60s. Real install flows fire one download per // update; anything tighter is scripted/abusive. // // Returns 404 gracefully when the APK isn't present (dev environments, -// pre-CI-wiring); the Flutter client treats 404 as "no update channel -// available." +// pre-CI-wiring); the client treats 404 as "no update channel available." +// +// (These paragraphs said "the Flutter client" until 2026-09-10. That client +// was deleted in v2026.08.18 — the Android app is the only one now.) import ( + "encoding/json" "errors" "net/http" "os" @@ -84,8 +86,36 @@ func clientAPKAllowDownload(userID string, now time.Time) time.Duration { return 0 } +// clientVersionSidecar is the JSON written beside the bundled APK by +// release.yml. It carries three values that are deliberately separate: +// +// - Name is a LABEL for people, "YYYY.MM.DD.HHMM" from the commit's +// timestamp. Two channels carrying the same code report the same name. +// - Code is the ORDERING KEY, minutes since 2020-01-01 at build time, and +// is the value Android itself installs by. It answers "may this be +// installed over that?" — the name never does. +// - Channel is a SIBLING FIELD, never a suffix inside the name. +// +// JSON rather than a positional line on purpose. The obvious growth path for +// the old one-value file was " ", which a first-space split +// silently mangles the moment a third field appears: the code stops parsing, +// and the reader falls back to name comparison WITHOUT erroring. +type clientVersionSidecar struct { + Name string `json:"name"` + // Pointer, not int64: absent must stay distinguishable from zero. An + // artifact published before codes were recorded genuinely has no code — + // zero would claim it is infinitely old rather than unknown. + Code *int64 `json:"code"` + Channel string `json:"channel"` +} + type clientVersionResponse struct { - Version string `json:"version"` + Version string `json:"version"` + // omitempty on both: the client must be able to tell "this server does + // not report a code" from "this build's code is 0", because those call + // for different behaviour on the other end. + Code *int64 `json:"code,omitempty"` + Channel string `json:"channel,omitempty"` APKURL string `json:"apk_url"` SizeBytes int64 `json:"size_bytes"` } @@ -117,8 +147,25 @@ func (h *handlers) handleClientVersion(w http.ResponseWriter, _ *http.Request) { return } + var sidecar clientVersionSidecar + if err := json.Unmarshal(versionBytes, &sidecar); err != nil { + // Fail LOUDLY rather than serving a blank version. The failure mode + // this avoids is the one that never gets reported: if an unreadable + // sidecar produced an empty name, every client would compare against + // nothing, conclude it was current, and go quiet — "I cannot read + // this" and "there is nothing newer" would be the same answer. + writeErrWithLog(w, h.logger, "client_version: sidecar is not valid JSON", err) + return + } + if sidecar.Name == "" { + http.Error(w, `{"error":{"code":"bad_client_version","message":"version sidecar has no name"}}`, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusOK, clientVersionResponse{ - Version: strings.TrimSpace(string(versionBytes)), + Version: strings.TrimSpace(sidecar.Name), + Code: sidecar.Code, + Channel: strings.TrimSpace(sidecar.Channel), APKURL: "/api/client/apk", SizeBytes: stat.Size(), }) diff --git a/internal/api/client_assets_test.go b/internal/api/client_assets_test.go index f85f49eb..fdd735ec 100644 --- a/internal/api/client_assets_test.go +++ b/internal/api/client_assets_test.go @@ -77,18 +77,32 @@ func TestClientVersion_404WhenAPKButNoVersion(t *testing.T) { } } -func TestClientVersion_200WithBothFiles(t *testing.T) { +// writeClientAssets stages an APK plus a raw sidecar body, and returns the +// APK's size so callers can assert size_bytes without recomputing it. +func writeClientAssets(t *testing.T, sidecar string) int64 { + t.Helper() dir := withClientAPKDir(t) body := []byte("fake apk content") if err := os.WriteFile(filepath.Join(dir, clientAPKFilename), body, 0o644); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(dir, clientVersionFile), []byte("v2026.05.10\n"), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(dir, clientVersionFile), []byte(sidecar), 0o644); err != nil { t.Fatal(err) } + return int64(len(body)) +} + +func getClientVersion(t *testing.T) *httptest.ResponseRecorder { + t.Helper() h := &handlers{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} rr := httptest.NewRecorder() h.handleClientVersion(rr, httptest.NewRequest(http.MethodGet, "/api/client/version", nil)) + return rr +} + +func TestClientVersion_200WithBothFiles(t *testing.T) { + size := writeClientAssets(t, `{"name":"2026.09.10.1432","code":3523847,"channel":"stable"}`+"\n") + rr := getClientVersion(t) if rr.Code != http.StatusOK { t.Fatalf("want 200, got %d (body: %s)", rr.Code, rr.Body.String()) } @@ -96,14 +110,69 @@ func TestClientVersion_200WithBothFiles(t *testing.T) { if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { t.Fatal(err) } - if resp.Version != "v2026.05.10" { - t.Errorf("version: want trimmed v2026.05.10, got %q", resp.Version) + if resp.Version != "2026.09.10.1432" { + t.Errorf("version: want 2026.09.10.1432, got %q", resp.Version) + } + if resp.Code == nil { + t.Fatal("code: want 3523847, got absent — the client decides on this, so absent means it silently falls back to name comparison") + } + if *resp.Code != 3523847 { + t.Errorf("code: want 3523847, got %d", *resp.Code) + } + if resp.Channel != "stable" { + t.Errorf("channel: want stable, got %q", resp.Channel) } if resp.APKURL != "/api/client/apk" { t.Errorf("apk_url: want /api/client/apk, got %q", resp.APKURL) } - if resp.SizeBytes != int64(len(body)) { - t.Errorf("size_bytes: want %d, got %d", len(body), resp.SizeBytes) + if resp.SizeBytes != size { + t.Errorf("size_bytes: want %d, got %d", size, resp.SizeBytes) + } +} + +// A release published before ordering keys were recorded has a name and +// genuinely no code. That must arrive as ABSENT, not as 0 — zero would claim +// the build is infinitely old and offer an update to everyone forever. +func TestClientVersion_CodeAbsentIsOmittedNotZero(t *testing.T) { + writeClientAssets(t, `{"name":"2026.09.09","code":null,"channel":"stable"}`) + rr := getClientVersion(t) + if rr.Code != http.StatusOK { + t.Fatalf("want 200, got %d (body: %s)", rr.Code, rr.Body.String()) + } + var resp clientVersionResponse + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatal(err) + } + if resp.Code != nil { + t.Errorf("code: want absent, got %d", *resp.Code) + } + // The wire must omit the key entirely, so a client can distinguish + // "this server reports no code" from "this build's code is 0". + var raw map[string]any + if err := json.Unmarshal(rr.Body.Bytes(), &raw); err != nil { + t.Fatal(err) + } + if _, present := raw["code"]; present { + t.Errorf("code key should be omitted entirely, body was %s", rr.Body.String()) + } +} + +// The failure this guards is the one nobody reports: if an unreadable sidecar +// produced an empty version, every client would compare against nothing, +// decide it was current, and go quiet. "I cannot read this" and "there is +// nothing newer" must not be the same answer. +func TestClientVersion_MalformedSidecarErrorsRatherThanReportingNothing(t *testing.T) { + for _, sidecar := range []string{ + "2026.09.10.1432", // the OLD plain-text format + `{"name":"x",`, // truncated JSON + `{"code":123,"channel":"dev"}`, // valid JSON, no name + "", + } { + writeClientAssets(t, sidecar) + rr := getClientVersion(t) + if rr.Code == http.StatusOK { + t.Errorf("sidecar %q: want an error status, got 200 with body %s", sidecar, rr.Body.String()) + } } } -- 2.54.0 From 68136c64c0039c5266039a73c2ec4b925231dcd6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 22:01:26 -0400 Subject: [PATCH 4/8] fix(android): decide updates on the ordering key, not the version name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The app compared NAMES while Android installs by versionCode, with nothing keeping the two orderings consistent. So it could offer a build the platform then refused as a downgrade, or stay silent about one it would have accepted. The offer and the install were asking different questions. Both consumers — the shell banner and the About card — now route through one isUpdateAvailable(): decide on the ordering key whenever the server reports one, since that is the same value the package installer compares, so an offer implies an install that will actually be accepted. Name comparison survives only as the fallback for a server predating the field. isVersionNewer is deliberately untouched. It already degrades per segment and is not what was broken; rewriting it while nearby would have put the fallback path at risk for no gain. code is nullable on the wire, and that is load-bearing rather than stylistic. The app's Json sets coerceInputValues = true, which replaces a JSON null with the declared default on a NON-nullable property — so `val code: Long = 0` would have turned "this server reports no ordering key" into "its key is 0" silently, ranking every such server as infinitely behind and offering its build to everyone forever. Reading the field declaration alone would never show that; it lives in AppModule. A third caller turned up during the sweep and was deliberately left alone. NetworkStatusController compares the /healthz minClientVersion, which is a server-declared compatibility floor rather than the bundled APK — there is no ordering key on that wire at all, so names remain the only thing it can compare. Different question, correctly still using the old helper. The update channel had no tests whatsoever before this, which is worth stating: the thing deciding whether anyone is ever offered an update fails silently in both directions. The new suite pins that the key wins when it disagrees with the name, that a null key falls back rather than reading as zero, the recorded migration constraint (a new-scheme name outranks an old-scheme one across a day boundary but NOT within the same day), and the degradation cases — including that an unparseable DECIDING segment reads as zero and loses, which is why the channel must never live inside the name. Every assertion was checked against the real comparison by mirroring it, rather than from reading it: two of my first-draft comments described the wrong mechanism and were corrected on the evidence. Step 4 of 5 — Scribe task #3811, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .../fabledsword/minstrel/models/UpdateInfo.kt | 21 ++- .../minstrel/models/wire/UpdateInfoWire.kt | 18 +- .../settings/ui/AboutCardViewModel.kt | 21 ++- .../update/data/UpdateBannerController.kt | 12 +- .../minstrel/update/data/UpdateRepository.kt | 29 ++++ .../update/data/UpdateVersioningTest.kt | 163 ++++++++++++++++++ 6 files changed, 251 insertions(+), 13 deletions(-) create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/update/data/UpdateVersioningTest.kt diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt index bf6d052a..5e3ace0b 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt @@ -1,15 +1,26 @@ package com.fabledsword.minstrel.models /** - * Wire shape returned by `GET /api/client/version`. Mirrors - * the Flutter client's `UpdateInfo`. + * The server-bundled APK, as reported by `GET /api/client/version`. * - * `version` is the server-bundled APK version (may have a leading - * "v" from the git tag); `apkUrl` is server-relative (e.g. - * `/api/client/apk`); `sizeBytes` is the download size. + * Three values that are deliberately kept apart: + * + * - [version] is a LABEL for people — "YYYY.MM.DD.HHMM", derived from the + * build's commit, so two channels carrying the same code read the same. + * Display this; never decide on it when [code] is present. + * - [code] is the ORDERING KEY, and is the same value Android itself + * installs by. It answers "may this be installed over that?", which the + * name cannot. Null when the server predates the field. + * - [channel] is a SIBLING FIELD, never a suffix inside the name. Reported + * verbatim rather than validated, so an unexpected value is shown rather + * than dropped. + * + * [apkUrl] is server-relative (e.g. `/api/client/apk`). */ data class UpdateInfo( val version: String, + val code: Long?, + val channel: String?, val apkUrl: String, val sizeBytes: Long, ) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt index fe772cd6..bf24db3a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt @@ -4,12 +4,26 @@ import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable /** - * Wire shape for `GET /api/client/version`. Defaults match Flutter: - * apk_url falls back to `/api/client/apk` if the server omits it. + * Wire shape for `GET /api/client/version`. + * + * `apkUrl` falls back to `/api/client/apk` if the server omits it. + * + * [code] MUST stay nullable, and this is not a style preference. The app's + * Json is configured with `coerceInputValues = true`, which replaces a JSON + * null with the declared default for a NON-nullable property — so writing + * `val code: Long = 0` would turn "this server reports no ordering key" into + * "this build's ordering key is 0", silently, with no error anywhere. A + * nullable type is what keeps absent distinguishable from zero, and the + * distinction is the whole reason the field exists. + * + * A server predating the ordering key sends neither [code] nor [channel]; + * both arrive null and the caller falls back to comparing names. */ @Serializable data class UpdateInfoWire( val version: String = "", + val code: Long? = null, + val channel: String? = null, @SerialName("apk_url") val apkUrl: String = "/api/client/apk", @SerialName("size_bytes") val sizeBytes: Long = 0, ) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt index cd5abac1..2ccd36ce 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt @@ -9,7 +9,7 @@ import com.fabledsword.minstrel.update.data.ApkInstaller import com.fabledsword.minstrel.update.data.InstallStage import com.fabledsword.minstrel.update.data.UpdateRepository import com.fabledsword.minstrel.update.data.isBusy -import com.fabledsword.minstrel.update.data.isVersionNewer +import com.fabledsword.minstrel.update.data.isUpdateAvailable import com.fabledsword.minstrel.update.data.message import com.fabledsword.minstrel.update.data.stage import dagger.hilt.android.lifecycle.HiltViewModel @@ -37,6 +37,10 @@ sealed interface UpdateCheckResult { data class AboutUiState( val installedVersion: String = BuildConfig.VERSION_NAME, + // The value the platform installs by, and therefore the one the update + // check must decide on. Held in state rather than read inline so a test + // can drive the comparison without a BuildConfig. + val installedCode: Long = BuildConfig.VERSION_CODE.toLong(), val isChecking: Boolean = false, val installStage: InstallStage = InstallStage.IDLE, val installMessage: String? = null, @@ -45,8 +49,9 @@ data class AboutUiState( /** * Backs the About card's update controls. "Check for updates" calls - * [UpdateRepository.getLatest], compares versus the build's - * VERSION_NAME via [isVersionNewer], and reports the terminal state. + * [UpdateRepository.getLatest], compares versus this build via + * [isUpdateAvailable] — on the ordering key where the server reports one, + * on the name otherwise — and reports the terminal state. * When an update is available, [install] downloads the APK via * [ApkInstaller] and installs it — routing the user to the "install * unknown apps" settings page first when that permission hasn't been @@ -66,9 +71,17 @@ class AboutCardViewModel @Inject constructor( viewModelScope.launch { internal.update { it.copy(isChecking = true, installMessage = null) } val installed = internal.value.installedVersion + val installedCode = internal.value.installedCode val result = runCatching { repository.getLatest() } .map { latest -> - if (isVersionNewer(latest.version, installed)) { + if ( + isUpdateAvailable( + serverCode = latest.code, + serverName = latest.version, + installedCode = installedCode, + installedName = installed, + ) + ) { UpdateCheckResult.UpdateAvailable(latest) } else { UpdateCheckResult.Latest diff --git a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt index 188bfff6..9d92a247 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt @@ -19,7 +19,8 @@ private const val POLL_INTERVAL_MS = 24 * 60 * 60 * 1000L /** * Drives the shell's soft "update available" banner. Polls * `/api/client/version` at launch + every 24h and, when the bundled - * APK is strictly newer than this build, exposes its [UpdateInfo] so + * APK outranks this build — by ordering key where the server reports one, + * by name otherwise — exposes its [UpdateInfo] so * [com.fabledsword.minstrel.update.ui.UpdateBanner] can nudge an * install. Mirrors Flutter's `ClientUpdateController`. * @@ -58,6 +59,13 @@ class UpdateBannerController @Inject constructor( private suspend fun runOnce() { val info = runCatching { repository.getLatest() }.getOrNull() ?: return - latest.value = info.takeIf { isVersionNewer(it.version, BuildConfig.VERSION_NAME) } + latest.value = info.takeIf { + isUpdateAvailable( + serverCode = it.code, + serverName = it.version, + installedCode = BuildConfig.VERSION_CODE.toLong(), + installedName = BuildConfig.VERSION_NAME, + ) + } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt index 7a25ef45..6eddfd73 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt @@ -21,10 +21,39 @@ class UpdateRepository @Inject constructor(retrofit: Retrofit) { private fun UpdateInfoWire.toDomain(): UpdateInfo = UpdateInfo( version = version, + code = code, + channel = channel, apkUrl = apkUrl, sizeBytes = sizeBytes, ) +/** + * True when [server] should be offered over the installed build. + * + * **Decide on the ordering key whenever the server sends one.** That is the + * same value Android's package installer compares, so an offer made this way + * implies an install the platform will actually accept. The app used to + * compare NAMES while the platform installed by `versionCode`, with nothing + * keeping the two orderings consistent — so it could offer a build Android + * then refused as a downgrade, or stay quiet about one it would have taken. + * + * Name comparison survives only as the fallback for a server that predates + * the field. A null code means "this server cannot tell me" — never "zero" — + * because treating absent as zero would rank every such server as infinitely + * old and offer its build to everyone, forever. + */ +fun isUpdateAvailable( + serverCode: Long?, + serverName: String, + installedCode: Long, + installedName: String, +): Boolean = + if (serverCode != null) { + serverCode > installedCode + } else { + isVersionNewer(serverName, installedName) + } + /** * True when [server] is strictly newer than [installed]. Mirrors * Flutter's `isVersionNewer` — splits both strings on `.`, parses diff --git a/android/app/src/test/java/com/fabledsword/minstrel/update/data/UpdateVersioningTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/update/data/UpdateVersioningTest.kt new file mode 100644 index 00000000..130c8b6a --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/update/data/UpdateVersioningTest.kt @@ -0,0 +1,163 @@ +package com.fabledsword.minstrel.update.data + +import org.junit.jupiter.api.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * The update channel had no tests at all before this. That is worth saying + * out loud, because the thing it decides — whether anyone is ever offered an + * update — fails silently in both directions: an update nobody is offered + * looks exactly like being up to date, and nobody files a bug about a prompt + * they never saw. + */ +class UpdateVersioningTest { + @Test + fun `decides on the ordering key when the server reports one`() { + assertTrue( + isUpdateAvailable( + serverCode = 3523847, serverName = "2026.09.10.1432", + installedCode = 3519456, installedName = "2026.09.09.1828", + ), + ) + assertFalse( + isUpdateAvailable( + serverCode = 3519456, serverName = "2026.09.09.1828", + installedCode = 3523847, installedName = "2026.09.10.1432", + ), + ) + } + + @Test + fun `an equal ordering key is not an update`() { + assertFalse( + isUpdateAvailable( + serverCode = 3523847, serverName = "2026.09.10.1432", + installedCode = 3523847, installedName = "2026.09.10.1432", + ), + ) + } + + /** + * The property the whole rework exists for: the offer must agree with what + * the platform will actually install. Where the two disagree, the ordering + * key wins, because that is the value Android compares. + */ + @Test + fun `the ordering key wins even when the name disagrees`() { + // Name looks older, key is newer — e.g. an older commit rebuilt later. + assertTrue( + isUpdateAvailable( + serverCode = 9_000_000, serverName = "2020.01.01.0000", + installedCode = 1, installedName = "2099.12.31.2359", + ), + ) + // Name looks newer, key is not. Offering this would be offering an + // install the platform then refuses as a downgrade. + assertFalse( + isUpdateAvailable( + serverCode = 1, serverName = "2099.12.31.2359", + installedCode = 9_000_000, installedName = "2020.01.01.0000", + ), + ) + } + + @Test + fun `falls back to the name when the server reports no ordering key`() { + assertTrue( + isUpdateAvailable( + serverCode = null, serverName = "2026.09.10.1432", + installedCode = 3519456, installedName = "2026.09.09.1828", + ), + ) + assertFalse( + isUpdateAvailable( + serverCode = null, serverName = "2026.09.09.1828", + installedCode = 3519456, installedName = "2026.09.10.1432", + ), + ) + } + + /** + * A null code must never be read as zero. Zero would rank every + * older server as infinitely behind and offer its build to everyone, + * forever — so this asserts the fallback runs instead of a comparison + * against 0 succeeding by accident. + */ + @Test + fun `a null ordering key is absent, not zero`() { + // installedCode is 0 here: if null coerced to 0, "0 > 0" would be + // false and this would wrongly report no update despite a newer name. + assertTrue( + isUpdateAvailable( + serverCode = null, serverName = "2026.09.10.1432", + installedCode = 0, installedName = "2026.09.09.1828", + ), + ) + } + + /** + * The recorded migration constraint, pinned so it cannot be forgotten: + * the old scheme's fourth segment was a commit count (~1895), the new + * one is HHMM. Across a day boundary the date decides and all is well. + */ + @Test + fun `new-scheme name outranks an old-scheme name on a later day`() { + assertTrue(isVersionNewer("2026.09.10.1432", "2026.09.09.1895")) + } + + /** + * ...but on the SAME day the comparison comes down to HHMM against a + * commit count, and any build before ~19:00 UTC reads as older. This is + * why the first new-scheme release had to be cut on a later calendar day. + * Asserting the trap so nobody "fixes" it by accident. + */ + @Test + fun `same-day new-scheme name can read older than an old-scheme name`() { + assertFalse(isVersionNewer("2026.09.09.1828", "2026.09.09.1895")) + } + + @Test + fun `name comparison degrades per segment rather than discarding`() { + // The string is still compared rather than rejected outright: an + // earlier segment decides and the unparseable tail never matters. + assertTrue(isVersionNewer("2026.09.10.1432-dev", "2026.09.09.1828")) + + // A shorter name pads with zeros instead of being refused. + assertTrue(isVersionNewer("2026.09.10", "2026.09.09.9999")) + assertFalse(isVersionNewer("2026.09.10", "2026.09.10.0")) + } + + /** + * What "costs that segment's precision" actually means, and it is worth + * pinning because it is a real edge rather than a nicety: when the + * unparseable segment is the DECIDING one, it reads as 0 and loses. So a + * `-dev` suffixed build compares as older than an unsuffixed one from the + * same minute. + * + * That is the correct behaviour for a degrading parser — it is bounded + * loss rather than a discarded string — but it is exactly why the channel + * belongs in its own field and never in the name. + */ + @Test + fun `an unparseable deciding segment reads as zero and loses`() { + assertFalse(isVersionNewer("2026.09.10.1432-dev", "2026.09.10.1000")) + } + + /** + * Both sides unparseable (branch-name builds) falls back to string + * inequality, so a dev build still surfaces rather than comparing equal + * and going silent. + */ + @Test + fun `two unparseable names fall back to string inequality`() { + assertTrue(isVersionNewer("main", "dev")) + assertFalse(isVersionNewer("dev", "dev")) + } + + @Test + fun `a leading v is ignored on either side`() { + assertTrue(isVersionNewer("v2026.09.10.1432", "2026.09.09.1828")) + assertFalse(isVersionNewer("v2026.09.10.1432", "v2026.09.10.1432")) + } +} -- 2.54.0 From ca1c18bbbb2c56e9387865210040b7b9cb196ed9 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 22:07:23 -0400 Subject: [PATCH 5/8] fix(android): miniplayer content sat at the top of its bar, not centred MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported with a screenshot: the bar drew at full height but the cover, title and transport row hugged its top edge, leaving an empty strip of surface above the gesture area. The Surface is a fixed 80dp. Inside it a plain Column stacked a 4dp progress fill and then MiniRow at its INTRINSIC height — 48dp, set by the cover and the icon buttons. A Column stacks from the top and nothing claimed the remainder, so 80 - 4 - 48 = 28dp collected at the bottom. Measured off the screenshot rather than eyeballed, and the bands agree exactly: progress fill 14px (4dp at 3.5x), surface 280px (80dp), cover 167px (48dp), empty below 99px (28.3dp). That the arithmetic lands on the measurement is what makes this the whole cause rather than one contributor. MiniRow was already centring its content correctly — inside a box that was only ever 48dp tall. Giving it weight(1f) lets it take what the progress fill leaves, so it measures 76dp and centres 48dp of content: 14dp above and below. The fill stays pinned to the top edge, which is where a progress indicator belongs. Not the same bug as issue #2681. That was a dead strip ABOVE the miniplayer from an unclaimed navigation-bar inset, fixed in v2026.08.18. This is inside the bar, pure layout, no insets — the surface already stopped correctly above the gesture area. CI cannot see this one. There are no Compose UI tests in the repo; the Android lane is ktlint, detekt and JVM unit tests, and a layout bug needs an instrumented test to catch. Compilation and lint are all this commit gets from CI — the visual check is on a device, and the APK only builds on a tagged release. Scribe issue #3826. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .../fabledsword/minstrel/player/ui/MiniPlayer.kt | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/MiniPlayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/MiniPlayer.kt index eba3ddce..3c2cce15 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/MiniPlayer.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/MiniPlayer.kt @@ -109,8 +109,11 @@ private fun MiniCover(coverUrl: String, contentDescription: String) { * NowPlayingScreen via [onExpandClick]. * * Layout (Column): - * - Slim seek slider at the top (4dp track) - * - Row: cover | title/artist column | like | prev | play/pause | next + * - Slim seek slider pinned at the top (4dp track) + * - Row: cover | title/artist column | like | prev | play/pause | next. + * Weighted so it fills the rest of the fixed-height bar and centres its + * own content; otherwise the row keeps its intrinsic 48dp and the + * leftover height collects at the bottom as dead surface. * * No kebab on the mini bar (operator 2026-06-01): the full kebab * surface lives on NowPlayingScreen, and dropping it from the mini @@ -164,6 +167,12 @@ fun MiniPlayer( durationMs = state.durationMs, ) MiniRow( + // Take whatever the progress fill leaves. Without this the + // Column stacks 4dp + the row's intrinsic 48dp from the top + // and the remaining 28dp of an 80dp bar sits empty + // underneath — the content looked top-aligned rather than + // centred, with a dead strip above the gesture bar. + modifier = Modifier.weight(1f), track = track, isPlaying = state.isPlaying, isUpnpLoading = state.isUpnpLoading, @@ -205,6 +214,7 @@ private fun MiniProgressFill(positionMs: Long, durationMs: Long) { @Composable @Suppress("LongParameterList") private fun MiniRow( + modifier: Modifier, track: TrackRef, isPlaying: Boolean, isUpnpLoading: Boolean, @@ -216,7 +226,7 @@ private fun MiniRow( onToggleLike: () -> Unit, ) { Row( - modifier = Modifier + modifier = modifier .fillMaxWidth() .padding(horizontal = 12.dp), verticalAlignment = Alignment.CenterVertically, -- 2.54.0 From eaf4654c0a2d9fb7371a2af5590c03a90226e611 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 22:11:51 -0400 Subject: [PATCH 6/8] 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") + } +} -- 2.54.0 From a687ef439c5252919f967fda80db9d3dd513ac42 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 22:17:07 -0400 Subject: [PATCH 7/8] fix(release): refuse an ordering key that overflows versionCode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The script asserted the key was positive but never that it fits. Android's versionCode is a signed 32-bit int and the platform rejects an APK above it, so a build machine with a badly wrong clock would emit a code the script happily hands on and the install then refuses. Worse than a rejected build: an over-ceiling code is also unreachably high, so every correct build afterwards would fail to outrank it and the update channel would be permanently stuck. Cheaper to refuse at the source than to diagnose it from a phone that will not update. The Go guard already asserted this, but only against a pinned value. The script is what actually runs at build time, so the check belongs here too. Falsified at the boundary rather than by eye — exactly at the ceiling exits 0, one minute past exits 1. My first probe used a year-6000 clock and did NOT fire, which turned out to be the probe being wrong rather than the check: that epoch still lands under the ceiling. The ceiling is reached in 6103, roughly 4079 years out, so this only ever catches a misconfigured clock. This commit deliberately touches ci/version.sh alone, to verify the path filters added in eaf4654c actually fire the Go lane for release-machinery changes. That run proved nothing about them, because it also touched internal/** and would have run regardless. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- ci/version.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ci/version.sh b/ci/version.sh index 36583f45..72d115df 100755 --- a/ci/version.sh +++ b/ci/version.sh @@ -65,6 +65,19 @@ if [ "${code}" -le 0 ]; then exit 1 fi +# Android's versionCode is a signed 32-bit int and the platform refuses an APK +# whose code exceeds it. At ~525k minutes a year this is four thousand years +# away in normal operation, so the realistic cause is a build machine with a +# badly wrong clock — which produces a code that is not merely too large but +# also unreachably high, permanently blocking every real build that follows +# from ever outranking it. Cheaper to refuse the build than to discover that +# from a phone that will not update. +readonly VERSION_CODE_CEILING=2147483647 +if [ "${code}" -gt "${VERSION_CODE_CEILING}" ]; then + echo "version.sh: ordering key '${code}' exceeds versionCode's int32 ceiling — build clock wrong?" >&2 + exit 1 +fi + # KEY=VALUE, which is also exactly $GITHUB_OUTPUT's format. echo "name=${name}" echo "code=${code}" -- 2.54.0 From 90bb3538c6b73c2cf8d17de6c154b6c627b03025 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 9 Sep 2026 22:47:42 -0400 Subject: [PATCH 8/8] feat(release): build a dev channel so testing stops requiring a release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was no test channel at all. release.yml ran only on main and tags, so no :dev image existed and no APK was produced outside a release — the only way to get a build onto a phone was to ship one, which made `main` the staging area by default. A push to dev now builds a signed APK, bundles it, and publishes :dev. Signed with the SAME key as release builds, deliberately. A differently signed APK cannot install over the stable app, so anyone moving between channels would have to uninstall and lose their local data. Same key means both directions work. :dev is published ALONE, with no per-commit tag. A rolling channel is rolling by definition; a commit-addressable image for it would be a rollback target nobody ever pulls, kept forever. Recovery on dev is to fix forward, and that is a deliberate trade rather than an omission. The channel is derived from the REF, not the commit, which is why it is computed in the workflow and not in ci/version.sh. The same commit built on dev and on main reports the same version NAME and differs only in the channel field — that separation is the entire point of keeping the three values apart. What this repo deliberately does NOT get: a cross-repo dispatch to refresh the channel when its bundled APK is rebuilt. That mechanism exists elsewhere in the family because the app and server live in separate repos, and a channel that can only be refreshed by an unrelated commit is not a channel. Minstrel is a monorepo — one push builds the APK and the image in the same run from the same commit, so the channel cannot go stale against its own artifact. The requirement is met structurally; copying the mechanism would add a moving part to fix a problem that does not exist here. Two guards, for the two ways this wiring can fail quietly: A dev push must never move :latest. That would ship untested code to every stable operator on their next pull, with the build green and the image perfectly valid — just the wrong audience. Nothing else in the suite would notice. The two bundling paths must stay mutually exclusive. The rebundle step is now gated to main specifically, not to "not a tag": under the looser condition a dev push would run BOTH steps, staging its fresh APK and then overwriting it with the previous release's. The image still builds, the sidecar still parses, and the channel whose whole job is being current quietly serves stale art. Both falsified against the regressions they name before committing. Scribe task #3819, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH --- .gitea/workflows/release.yml | 81 +++++++++++++++++++++---- README.md | 7 ++- internal/server/release_version_test.go | 63 +++++++++++++++++++ 3 files changed, 138 insertions(+), 13 deletions(-) diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index 8389ab32..4c7f7f88 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -2,10 +2,31 @@ name: release # Builds and pushes the minstrel container image to the Gitea registry. # +# push to dev → :dev (freshly-built dev APK bundled) # push to main → :main and :latest (latest-release APK bundled) # push tag vYYYY.MM.DD.HHMM → :vYYYY.MM.DD.HHMM and :latest (fresh APK bundled) # workflow_dispatch → manual trigger (same rules based on the ref) # +# The dev channel exists so testing a build does not require shipping one. +# Before it, the only way to get an APK onto a phone was to cut a release, +# which made `main` the staging area by default. `:dev` carries its own +# freshly-built APK, signed with the SAME key as release builds — a different +# key cannot install over the stable app, so anyone crossing channels would +# have to uninstall and lose their data. +# +# :dev is published ALONE, with no per-commit tag. A rolling channel is +# rolling by definition; a commit-addressable image for it would be a +# rollback target nobody ever pulls, kept forever. Recovery on dev is to fix +# forward. +# +# Note what this repo does NOT need: a cross-repo dispatch to refresh the +# channel when its bundled APK is rebuilt. That mechanism exists elsewhere in +# the family because the app and the server live in separate repos. Minstrel +# is a monorepo — one push builds the APK and the image in the same run from +# the same commit, so the channel cannot go stale against its own artifact. +# The requirement is satisfied structurally; copying the mechanism would add +# a moving part to fix a problem that does not exist here. +# # Release model: the tag IS the artifact's version name with a `v` in front. # `v2026.09.10.1432` and `2026.09.10.1432` are the same string, derived from # the tagged commit's UTC timestamp — so there is no mismatch to reconcile @@ -48,7 +69,7 @@ name: release on: push: - branches: [main] + branches: [main, dev] tags: ['v*'] paths-ignore: - 'docs/**' @@ -65,8 +86,11 @@ concurrency: jobs: android-release: - name: Build signed APK (tag releases only) - if: startsWith(github.ref, 'refs/tags/v') + name: Build signed APK (releases and dev) + # Also builds on `dev`, which is what makes a test channel possible at + # all. Without it the only way to get a build onto a phone was to cut a + # release, which quietly turns `main` into the staging area. + if: startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/dev' runs-on: flutter-ci container: image: git.fabledsword.com/bvandeusen/ci-android:36 @@ -91,6 +115,7 @@ jobs: outputs: version_name: ${{ steps.ver.outputs.name }} version_code: ${{ steps.ver.outputs.code }} + channel: ${{ steps.ver.outputs.channel }} steps: - name: Checkout @@ -115,7 +140,18 @@ jobs: # 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' ' ')" + + # The channel is a property of the LANE, not of the commit, which is + # why it is derived here rather than in version.sh. Same commit built + # on dev and on main reports the same NAME and differs only here — + # that is the whole point of separating the two values. + if [ "${GITHUB_REF}" = "refs/heads/dev" ]; then + channel=dev + else + channel=stable + fi + echo "channel=${channel}" >> "$GITHUB_OUTPUT" + echo "::notice::APK $(printf '%s' "${out}" | tr '\n' ' ') channel=${channel}" # 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 — @@ -127,6 +163,7 @@ jobs: # the release together, so this passes). A bare `git push origin vX` is the # case this catches. - name: Release must exist for this tag + if: startsWith(github.ref, 'refs/tags/v') shell: bash working-directory: ${{ github.workspace }} env: @@ -190,6 +227,10 @@ jobs: if-no-files-found: error - name: Attach APK to gitea Release + # Tag releases only. A dev build has no Release to hang assets on and + # does not need one — the :dev image bundles the APK, and the server + # serves it from /api/client/apk like any other. + if: startsWith(github.ref, 'refs/tags/v') shell: bash env: CI_TOKEN: ${{ secrets.CI_TOKEN }} @@ -298,6 +339,15 @@ jobs: echo "args=-t ${IMAGE}:${VERSION} -t ${IMAGE}:latest" >> "$GITHUB_OUTPUT" echo "version=${VERSION}" >> "$GITHUB_OUTPUT" echo "::notice::Release build: ${VERSION} + latest" + elif [[ "${GITHUB_REF}" == "refs/heads/dev" ]]; then + # The rolling test channel, and :dev ALONE — deliberately no + # per-commit tag. A rolling channel is rolling by definition, so a + # commit-addressable image here would be a rollback target nobody + # has ever pulled, accumulating in the registry forever. Recovery + # on dev is to fix forward. + echo "args=-t ${IMAGE}:dev" >> "$GITHUB_OUTPUT" + echo "version=dev" >> "$GITHUB_OUTPUT" + echo "::notice::Dev-branch build: :dev" else # Main is the protected, post-PR-merge branch. Treat it as the # rolling stable channel — every main push moves :latest. @@ -316,9 +366,12 @@ jobs: | docker login git.fabledsword.com -u "${{ github.actor }}" --password-stdin - name: Download signed APK artifact - # Tag pushes only — android-release just produced this. Non-tag - # builds take the "Bundle latest release APK" path below instead. - if: steps.guard.outputs.ready == 'true' && startsWith(github.ref, 'refs/tags/v') + # Tag and dev pushes — android-release just produced this. Only `main` + # takes the "Bundle latest release APK" path below, because it is the + # one ref that moves a channel without building an APK of its own. + if: >- + steps.guard.outputs.ready == 'true' && + (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/dev') # Consuming half of the pair — never actions/download-artifact. Same fork, # same reason: upstream's client-side GHES check rejects this hostname # before it connects. bvandeusen/download-artifact mirrors @@ -338,22 +391,26 @@ jobs: path: client/ - name: Stage bundled APK + version sidecar - if: steps.guard.outputs.ready == 'true' && startsWith(github.ref, 'refs/tags/v') + if: >- + steps.guard.outputs.ready == 'true' && + (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/dev') shell: bash env: - # Both pulled from android-release's outputs so the sidecar the + # All three pulled from android-release's outputs so the sidecar the # server hands clients matches exactly what is baked into the APK # they are comparing against. APK_VERSION_NAME: ${{ needs.android-release.outputs.version_name }} APK_VERSION_CODE: ${{ needs.android-release.outputs.version_code }} + APK_CHANNEL: ${{ needs.android-release.outputs.channel }} run: | set -euxo pipefail # The artifact lands as `app-release.apk` (the original Gradle # output name). The Dockerfile COPYs client/* into /app/client/ # and the server reads minstrel.apk + minstrel.apk.version. mv client/app-release.apk client/minstrel.apk - printf '{"name":"%s","code":%s,"channel":"stable"}\n' \ - "${APK_VERSION_NAME}" "${APK_VERSION_CODE}" > client/minstrel.apk.version + printf '{"name":"%s","code":%s,"channel":"%s"}\n' \ + "${APK_VERSION_NAME}" "${APK_VERSION_CODE}" "${APK_CHANNEL}" \ + > client/minstrel.apk.version cat client/minstrel.apk.version ls -lh client/ @@ -365,7 +422,7 @@ jobs: # that build actually recorded rather than something re-derived here. # Degrades to an empty client/ (404 update channel) — never a wrong # version — if no release or APK asset can be resolved. - if: steps.guard.outputs.ready == 'true' && !startsWith(github.ref, 'refs/tags/v') + if: steps.guard.outputs.ready == 'true' && github.ref == 'refs/heads/main' shell: bash env: CI_TOKEN: ${{ secrets.CI_TOKEN }} diff --git a/README.md b/README.md index 14febf77..c9d8e8b1 100644 --- a/README.md +++ b/README.md @@ -115,8 +115,13 @@ Image tags (`git.fabledsword.com/bvandeusen/minstrel:`): - `:latest` — the newest blessed image. Moves on every `main` push **and** every release. Recommended for most operators. - `:vYYYY.MM.DD.HHMM` — immutable release tags, never moved or deleted. Pin one for a deployment you don't want changing under you. The tag is the build's own version name with a `v` in front, derived from the tagged commit's UTC timestamp, so two releases can never collide and a re-cut is simply a new tag. - `:main` — the rolling post-merge tip. Same image as `:latest` at push time; choose it if you want to track `main` explicitly rather than the release line. +- `:dev` — the rolling test channel, rebuilt on every push to `dev` and carrying its own freshly-built Android APK. Run this when you want to try something before it ships. It moves constantly, has no per-commit tag to pin, and its only recovery path is forward — if a `:dev` image is broken, the fix is the next push, not a rollback. -Every `:latest` and every `:vYYYY.MM.DD.HHMM` bundles the current signed Android APK, so the in-app update channel is always live. Database migrations run automatically at startup; rollbacks require restoring a Postgres dump. +Every `:latest`, `:vYYYY.MM.DD.HHMM` and `:dev` bundles a signed Android APK, so the in-app update channel is always live. All of them are signed with the same key, so a phone can move between the stable and dev channels without uninstalling — point it at a `:dev` server and the in-app updater offers that channel's build. + +The app reports which channel it is on alongside its version, and decides whether an update is available using the build's ordering key rather than its displayed name — the same value Android installs by, so an offer it makes is one the platform will accept. + +Database migrations run automatically at startup; rollbacks require restoring a Postgres dump. Releases before 2026-09-10 use the older per-day `:vYYYY.MM.DD` shape. Those tags still exist and still work — they are simply not extended. diff --git a/internal/server/release_version_test.go b/internal/server/release_version_test.go index a63dcbcd..892103ff 100644 --- a/internal/server/release_version_test.go +++ b/internal/server/release_version_test.go @@ -177,3 +177,66 @@ func TestReleaseWorkflow_UsesTheSharedDerivation(t *testing.T) { t.Error("release.yml derives a commit count again — that is not monotonic across branches") } } + +// devArm returns the branch of "Compute image tags" that handles refs/heads/dev. +func devArm(t *testing.T, yaml string) string { + t.Helper() + const marker = `elif [[ "${GITHUB_REF}" == "refs/heads/dev" ]]; then` + i := strings.Index(yaml, marker) + if i < 0 { + t.Fatal("no refs/heads/dev arm in Compute image tags — the dev channel is not wired") + } + rest := yaml[i+len(marker):] + if j := strings.Index(rest, "\n else"); j >= 0 { + return rest[:j] + } + return rest +} + +// The worst regression this wiring can produce: a dev push that also moves +// :latest would ship untested code to every stable operator, silently, on the +// next pull. Nothing else in the suite would notice — the build stays green +// and the image is valid, it is simply the wrong audience. +func TestDevChannel_PublishesDevAloneAndNeverLatest(t *testing.T) { + body, err := os.ReadFile(filepath.Join(repoRoot(t), ".gitea", "workflows", "release.yml")) + if err != nil { + t.Fatal(err) + } + arm := devArm(t, string(body)) + + if !strings.Contains(arm, "${IMAGE}:dev") { + t.Errorf("dev arm does not publish :dev\n%s", arm) + } + if strings.Contains(arm, ":latest") { + t.Errorf("dev arm moves :latest — that ships dev code to every stable operator\n%s", arm) + } + // Rule 145: a rolling channel gets no commit-addressable tag. + if strings.Contains(arm, "GITHUB_SHA") { + t.Errorf("dev arm publishes a per-commit tag; a rolling channel should not\n%s", arm) + } +} + +// The two bundling paths must stay mutually exclusive. If the rebundle step's +// condition were relaxed back to "not a tag", a dev push would run BOTH: stage +// its freshly-built APK, then overwrite it with the previous release's. The +// image would still build and the sidecar would still parse — it would just +// quietly serve stale art to the channel whose whole job is being current. +func TestDevChannel_RebundlePathIsMainOnly(t *testing.T) { + body, err := os.ReadFile(filepath.Join(repoRoot(t), ".gitea", "workflows", "release.yml")) + if err != nil { + t.Fatal(err) + } + yaml := string(body) + + i := strings.Index(yaml, "- name: Bundle latest release APK") + if i < 0 { + t.Fatal("no 'Bundle latest release APK' step") + } + step := yaml[i:] + if j := strings.Index(step, "\n - name:"); j >= 0 { + step = step[:j] + } + if !strings.Contains(step, "github.ref == 'refs/heads/main'") { + t.Errorf("the rebundle step is not gated to main; a dev push would overwrite its own APK\n%s", step) + } +} -- 2.54.0