diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index 6e89d541..4c7f7f88 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -2,15 +2,51 @@ 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 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) # -# 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. +# 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 +# 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 @@ -24,33 +60,37 @@ 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 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. on: push: - branches: [main] + branches: [main, dev] tags: ['v*'] paths-ignore: - 'docs/**' - '**/*.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 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 @@ -75,14 +115,18 @@ jobs: outputs: version_name: ${{ steps.ver.outputs.name }} version_code: ${{ steps.ver.outputs.code }} + channel: ${{ steps.ver.outputs.channel }} steps: - 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 +135,23 @@ 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}" - echo "name=${VERSION_NAME}" >> "$GITHUB_OUTPUT" - echo "code=${COMMIT_COUNT}" >> "$GITHUB_OUTPUT" - echo "::notice::APK version: ${VERSION_NAME} (code=${COMMIT_COUNT})" + # 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" + + # 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 — @@ -108,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: @@ -171,9 +227,15 @@ 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 }} + VERSION_NAME: ${{ steps.ver.outputs.name }} + VERSION_CODE: ${{ steps.ver.outputs.code }} run: | set -euxo pipefail TAG="${GITHUB_REF#refs/tags/}" @@ -181,6 +243,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}")" @@ -202,6 +278,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 @@ -222,11 +312,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 count and reconstruct the bundled - # APK's exact versionName (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 @@ -249,11 +339,20 @@ 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. - # 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" @@ -267,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 @@ -289,32 +391,38 @@ 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: - # 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. + # 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 - echo "${APK_VERSION_NAME}" > 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/ - 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 (${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. + # 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. - if: steps.guard.outputs.ready == 'true' && !startsWith(github.ref, 'refs/tags/v') + # version — if no release or APK asset can be resolved. + if: steps.guard.outputs.ready == 'true' && github.ref == 'refs/heads/main' shell: bash env: CI_TOKEN: ${{ secrets.CI_TOKEN }} @@ -331,14 +439,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 - 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 - fi - VERSION_NAME="${TAG#v}.${COUNT}" 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/.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/README.md b/README.md index 58db87d4..c9d8e8b1 100644 --- a/README.md +++ b/README.md @@ -113,10 +113,17 @@ 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. +- `: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` 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. ## Specs 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 = 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/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, 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")) + } +} diff --git a/ci/version.sh b/ci/version.sh new file mode 100755 index 00000000..72d115df --- /dev/null +++ b/ci/version.sh @@ -0,0 +1,84 @@ +#!/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 + +# 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}" +echo "tag=v${name}" 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()) + } } } diff --git a/internal/server/release_version_test.go b/internal/server/release_version_test.go new file mode 100644 index 00000000..892103ff --- /dev/null +++ b/internal/server/release_version_test.go @@ -0,0 +1,242 @@ +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") + } +} + +// 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) + } +}