From a7e626a67a795ac7c803df0c4ee9cda6c0b2a864 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 27 Aug 2026 11:47:30 -0400 Subject: [PATCH] feat(extension): report the channel beside the version (step 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the half of the ask the signing work didn't: a way to tell a dev build from a main one. FC_CHANNEL is baked into the web image at build time and /api/extension/manifest reports it as its own key, next to version — the popup banner, the toolbar tooltip and the Settings card all name it. Beside the version, never inside it. A `1.0.3499884-dev` suffix is the obvious shortcut and it is the exact failure this design comes from: versionIsNewer parses each dotted segment with parseInt, so a suffixed segment reads as 0, every dev build compares equal to every other, and "no update available" stops being distinguishable from "I cannot read this version". The comparator already degrades rather than discarding (rule 150), which is a reason not to NEED the suffix, not a licence to add one. Two tests hold the line — one backend, asserting version and channel are separate keys; one frontend, asserting the rendered version text stays the bare derived number. Optional on the read side, and absent rather than defaulted. An image built before this field says nothing by not having the key; an image built without a channel now says nothing the same way, so there is one absence to handle instead of a second spelling of "unknown". Every reader drops the label entirely when it is missing and reads exactly as it did before. Reported verbatim rather than validated against {dev, main}: if an image declares something else, showing what it claims helps whoever is debugging more than dropping it would. FC_CHANNEL is declared LAST in the Dockerfile. An ARG invalidates every layer below it, and this is the one value that differs between the dev and main builds of identical source — earlier, and the two channels could never share a cached pip install. A tag push counts as main: a vYY.MM.DD tag is cut from main, so that image is a main-channel artifact wearing an immutable name. No channel switcher, deliberately. background.js:34 already records that Firefox's static update_url cannot apply, because every FC instance is a different host — so the extension asks its configured backend, and the channel IS the instance it points at. Switching is repointing apiUrl and reinstalling from that host. A separate setting would contradict each server build shipping its own extension. This commit touches packaged extension files, so it moves the derived version and will sign a new one via AMO — the first push to exercise the extension-changed path from dev end to end. --- .forgejo/workflows/build.yml | 12 ++++ Dockerfile | 17 +++++ backend/app/api/extension.py | 26 ++++++- ci-requirements.md | 9 +++ extension/README.md | 19 +++++ extension/background/background.js | 27 ++++++- extension/popup/popup.js | 6 +- .../settings/BrowserExtensionCard.vue | 10 +++ .../components/browserExtensionCard.spec.js | 72 +++++++++++++++++++ tests/test_api_extension.py | 47 ++++++++++++ 10 files changed, 241 insertions(+), 4 deletions(-) create mode 100644 frontend/test/components/browserExtensionCard.spec.js diff --git a/.forgejo/workflows/build.yml b/.forgejo/workflows/build.yml index ca758ff..5551d68 100644 --- a/.forgejo/workflows/build.yml +++ b/.forgejo/workflows/build.yml @@ -426,13 +426,20 @@ jobs: # everywhere). Operator-flagged 2026-06-01 after first :c- # main-push build failed at this step. SHORT_SHA=$(printf '%s' "$GITHUB_SHA" | cut -c1-7) + # `channel` is baked into the image as FC_CHANNEL and reported by + # /api/extension/manifest (milestone 271 step 7). A tag-push counts as + # `main`: a vYY.MM.DD tag is cut from main, so that image is a + # main-channel artifact wearing an immutable name. if [ "${GITHUB_REF#refs/tags/}" != "${GITHUB_REF}" ]; then TAG_NAME="${GITHUB_REF#refs/tags/}" echo "tags=git.fabledsword.com/bvandeusen/fabledcurator:${TAG_NAME}" >> "$GITHUB_OUTPUT" + echo "channel=main" >> "$GITHUB_OUTPUT" elif [ "${GITHUB_REF##*/}" = "main" ]; then echo "tags=git.fabledsword.com/bvandeusen/fabledcurator:main,git.fabledsword.com/bvandeusen/fabledcurator:latest,git.fabledsword.com/bvandeusen/fabledcurator:c-${SHORT_SHA}" >> "$GITHUB_OUTPUT" + echo "channel=main" >> "$GITHUB_OUTPUT" else echo "tags=git.fabledsword.com/bvandeusen/fabledcurator:dev" >> "$GITHUB_OUTPUT" + echo "channel=dev" >> "$GITHUB_OUTPUT" fi - name: Login to Forgejo registry @@ -449,6 +456,11 @@ jobs: file: Dockerfile push: true tags: ${{ steps.tag.outputs.tags }} + # Only the web image carries a channel: it is the one that serves + # /api/extension/manifest. The ml and agent images have nothing to + # report it to. + build-args: | + FC_CHANNEL=${{ steps.tag.outputs.channel }} build-ml: runs-on: python-ci diff --git a/Dockerfile b/Dockerfile index 6c9da51..050cc04 100644 --- a/Dockerfile +++ b/Dockerfile @@ -47,6 +47,23 @@ RUN chmod +x entrypoint.sh COPY --from=frontend-builder /build/dist ./frontend/dist +# Which channel this image belongs to — `dev` or `main` (milestone 271 step 7). +# build.yml passes it; /api/extension/manifest reports it beside the version so +# an operator can tell which channel an install came from without the channel +# ever touching the version string. +# +# Empty by default, deliberately: a locally-built image then reports NO channel +# rather than claiming to be one, and the manifest omits the field entirely — +# indistinguishable from an image built before the field existed, which is +# exactly the shape every reader already has to handle. +# +# Declared LAST on purpose. An ARG/ENV invalidates every layer below it, and +# this is the one value that differs between the dev and main builds of +# identical source — put it any earlier and the two channels could never share +# a cached pip install. +ARG FC_CHANNEL="" +ENV FC_CHANNEL=${FC_CHANNEL} + EXPOSE 8080 ENTRYPOINT ["./entrypoint.sh"] diff --git a/backend/app/api/extension.py b/backend/app/api/extension.py index 082bc1e..868bab2 100644 --- a/backend/app/api/extension.py +++ b/backend/app/api/extension.py @@ -7,6 +7,7 @@ from __future__ import annotations import asyncio import hashlib import hmac +import os import re from pathlib import Path @@ -31,6 +32,12 @@ XPI_DIR = Path("/app/frontend/dist/extension") _XPI_VERSION_RE = re.compile(r"fabledcurator-(?P[\w.-]+)\.xpi$") +# Which channel this image belongs to — "dev" or "main" — baked in at build +# time from the FC_CHANNEL build arg (milestone 271 step 7). Empty for a local +# build, or for any image predating the field. Tests override by monkeypatching +# this constant, same as XPI_DIR above. +FC_CHANNEL = os.environ.get("FC_CHANNEL", "").strip() + async def _ext_key_required(session) -> bool: """Unlike /api/credentials (which accepts the browser path with no @@ -133,13 +140,30 @@ def _read_manifest_sync() -> dict | None: return None versioned.sort(key=lambda p: p.stat().st_mtime) latest = versioned[-1] - return { + info = { "installed": True, "version": _extract_version(latest.name), "xpi_url": f"/extension/{latest.name}", "latest_url": "/extension/fabledcurator-latest.xpi", "sha256": _sha256(latest), } + # The channel goes BESIDE the version, never inside it. A `-dev` suffix is + # what silently disabled the dev channel in the sibling project this design + # comes from: the comparator returned nothing for a non-integer segment, so + # every dev version compared equal and "no update available" became + # indistinguishable from "I cannot read this version". + # + # Omitted rather than defaulted when unset. Absence already has a meaning + # every reader must handle — an image built before this field existed says + # exactly the same thing by not having the key — so a blank channel reuses + # that path instead of inventing a second "unknown" spelling. + # + # Reported verbatim, not validated against {"dev", "main"}: if an image + # declares something else, showing what it actually claims is more useful + # to whoever is debugging it than dropping the value on the floor. + if FC_CHANNEL: + info["channel"] = FC_CHANNEL + return info @extension_bp.route("/manifest", methods=["GET"]) diff --git a/ci-requirements.md b/ci-requirements.md index 4d0f3c4..0be64a8 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -73,6 +73,15 @@ per `docs/process.md`'s "add deps to the image when used by >1 project". `extension-version`. A depth-1 clone sees one commit and derives a wrong, too-low value **rather than failing**, so the full-history checkout is load-bearing rather than incidental. +- **`FC_CHANNEL` is a build arg, not a runtime setting.** `build.yml` passes + `dev` / `main` to the web image only (the ml and agent images have nothing to + report it to), and `/api/extension/manifest` reports it beside the version so + an install can be traced to a channel. It is declared LAST in the Dockerfile + on purpose: an ARG invalidates every layer below it, and this is the one value + that differs between the dev and main builds of identical source, so placing + it earlier would stop the two channels ever sharing a cached `pip install`. + Empty by default — a local build then reports no channel at all rather than + claiming one. - Callers MUST `set -f` before substituting the script's output. Without it the shell expands `test/**` against the working tree and silently narrows the pattern to whatever files exist at that moment — a failure that looks like diff --git a/extension/README.md b/extension/README.md index 22db07c..6dc6db1 100644 --- a/extension/README.md +++ b/extension/README.md @@ -66,6 +66,25 @@ Commit time gives both branches the same number for the same source — which is exactly what lets one AMO signature serve both channels (family rule 149, FC issue #3092). +## Channels + +`dev` and `main` each build and sign their own extension, and an install is +tied to whichever FC instance it points at — Firefox's static `update_url` +cannot apply here, since every FC install is a different host, so the extension +asks its configured backend. **The channel therefore IS the instance.** +Switching channel means repointing the FC URL in options and reinstalling from +that host; there is no separate channel setting, and adding one would +contradict each server build shipping its own extension. + +The channel is reported *beside* the version, never inside it: +`/api/extension/manifest` answers `{"version": "...", "channel": "dev"}`. It is +optional — an instance that declares none simply omits the key, and the popup, +the toolbar tooltip and the Settings card all read exactly as they did before +the field existed. Do not be tempted to make it a `-dev` version suffix: the +comparator parses each dotted segment with `parseInt`, so a suffixed segment +reads as 0 and every dev build compares equal to every other, collapsing "no +update available" and "I cannot read this version" into one answer. + ## Release Nothing to do by hand. Push to `dev`: `build.yml` signs the extension if this diff --git a/extension/background/background.js b/extension/background/background.js index 9fc8f59..edbc2a0 100644 --- a/extension/background/background.js +++ b/extension/background/background.js @@ -37,7 +37,16 @@ ensureInitialized().catch(e => console.error('init failed:', e)); // configured backend for the latest published version and nudge the operator to // reinstall the freshly-signed XPI — surfaced as a popup banner (on demand) and // a toolbar badge (daily). /api/extension/manifest is public and returns -// {version, latest_url, sha256}; the XPI is served from the web root (not /api). +// {version, latest_url, sha256} plus an OPTIONAL {channel} naming which channel +// that instance serves ("dev"/"main", #3113); the XPI is served from the web +// root (not /api). +// +// The channel IS the instance: Firefox's static update_url cannot apply here +// because every FC install is a different host, so the extension asks its +// configured backend — which means switching channel is repointing apiUrl in +// options and reinstalling from that host. There is no separate channel +// setting to build, and building one would contradict each server build +// shipping its own extension. function versionIsNewer(candidate, current) { // Dotted numeric compare so 1.0.10 > 1.0.9 (a plain string compare wouldn't). @@ -60,12 +69,22 @@ async function checkForUpdateInfo() { } const currentVersion = browser.runtime.getManifest().version; const latestVersion = info && info.version ? info.version : null; + // Which channel the configured instance serves — reported ALONGSIDE the + // version, never folded into it. A `-dev` suffix would have to survive + // versionIsNewer's parseInt above, and it wouldn't: the segment would read + // as 0 and every dev build would compare equal to every other. + // + // null is a normal answer, not a failure — an instance built before the + // field existed, or one built locally with no channel declared. Nothing + // below branches on it except the label. + const channel = info && info.channel ? info.channel : null; // latest_url is served from the web root, not the JSON API. const base = api.webRoot(); return { updateAvailable: !!latestVersion && versionIsNewer(latestVersion, currentVersion), currentVersion, latestVersion, + channel, xpiUrl: info && info.latest_url ? `${base}${info.latest_url}` : null, }; } @@ -77,7 +96,11 @@ async function refreshUpdateBadge() { await browser.action.setBadgeText({ text: r.updateAvailable ? '↑' : '' }); if (r.updateAvailable) { await browser.action.setBadgeBackgroundColor({ color: '#F4BA7A' }); - await browser.action.setTitle({ title: `FabledCurator — update available (v${r.latestVersion})` }); + // Channel first, version second, and the channel dropped entirely when + // the instance doesn't report one — so the tooltip reads exactly as it + // did before the field existed rather than saying "(unknown ...)". + const label = r.channel ? `${r.channel} v${r.latestVersion}` : `v${r.latestVersion}`; + await browser.action.setTitle({ title: `FabledCurator — update available (${label})` }); } else { await browser.action.setTitle({ title: 'FabledCurator' }); } diff --git a/extension/popup/popup.js b/extension/popup/popup.js index 4ffcd8e..771bb6a 100644 --- a/extension/popup/popup.js +++ b/extension/popup/popup.js @@ -81,8 +81,12 @@ async function checkForUpdate() { } function showUpdateBanner(r) { + // The channel names itself beside the version, never inside it (#3113). + // Absent when the instance doesn't report one, and the banner then reads + // exactly as it did before the field existed. + const channel = r.channel ? ` (${r.channel})` : ''; document.getElementById('update-text').textContent = - `Update available — v${r.latestVersion} (installed v${r.currentVersion})`; + `Update available${channel} — v${r.latestVersion} (installed v${r.currentVersion})`; // Opening the signed XPI triggers Firefox's native install prompt. document.getElementById('update-btn').addEventListener('click', () => { browser.tabs.create({ url: r.xpiUrl }); diff --git a/frontend/src/components/settings/BrowserExtensionCard.vue b/frontend/src/components/settings/BrowserExtensionCard.vue index 2dfbd38..4226eba 100644 --- a/frontend/src/components/settings/BrowserExtensionCard.vue +++ b/frontend/src/components/settings/BrowserExtensionCard.vue @@ -4,6 +4,16 @@ · Firefox · v{{ manifest.version }} + + {{ manifest.channel }} diff --git a/frontend/test/components/browserExtensionCard.spec.js b/frontend/test/components/browserExtensionCard.spec.js new file mode 100644 index 0000000..b406bb5 --- /dev/null +++ b/frontend/test/components/browserExtensionCard.spec.js @@ -0,0 +1,72 @@ +// @vitest-environment happy-dom +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { nextTick } from 'vue' + +import BrowserExtensionCard from '../../src/components/settings/BrowserExtensionCard.vue' +import { freshPinia, mountComponent } from '../support/mountComponent.js' + +// useApi is a thin fetch wrapper, so the seam is fetch itself (same shape as +// showcase.spec.js) rather than a module mock. +function stubApi(manifest) { + globalThis.fetch = vi.fn(async (url) => { + const payload = String(url).includes('/api/extension/manifest') + ? manifest + : { key: 'test-key' } + return { + ok: true, status: 200, statusText: '200', + text: async () => JSON.stringify(payload), + } + }) +} + +async function mountCard(manifest) { + stubApi(manifest) + const w = mountComponent(BrowserExtensionCard, { pinia: freshPinia() }) + // onMounted fires two fetches (manifest + key) and each resolves through a + // chain of microtasks. Yielding to a macrotask drains the whole queue, which + // a fixed number of nextTicks would only do by luck. + await new Promise((resolve) => setTimeout(resolve, 0)) + await nextTick() + return w +} + +const INSTALLED = { + installed: true, + version: '1.0.3499884', + xpi_url: '/extension/fabledcurator-1.0.3499884.xpi', + latest_url: '/extension/fabledcurator-latest.xpi', + sha256: 'abc', +} + +describe('BrowserExtensionCard — channel', () => { + beforeEach(() => { vi.restoreAllMocks() }) + afterEach(() => { delete globalThis.fetch }) + + it('names the channel the instance reports', async () => { + // The point of the whole channel scheme: an operator can tell a dev + // instance from a main one without installing anything. + const w = await mountCard({ ...INSTALLED, channel: 'dev' }) + expect(w.text()).toContain('dev') + }) + + it('shows the version and the channel as SEPARATE text, never merged', async () => { + // Regression guard with teeth: the tempting shortcut is a `-dev` version + // suffix, and that is precisely what breaks the extension's comparator — + // it parses each dotted segment with parseInt, so a suffixed segment reads + // as 0 and every dev build compares equal to every other. If someone ever + // "simplifies" by folding the channel into the version, the version text + // stops being the bare derived number and this fails. + const w = await mountCard({ ...INSTALLED, channel: 'dev' }) + expect(w.text()).toContain('v1.0.3499884') + expect(w.text()).not.toContain('1.0.3499884-dev') + }) + + it('renders no channel when the instance declares none', async () => { + // A locally-built image, or one predating the field. The card must read + // exactly as it did before the channel existed rather than inventing an + // "unknown" badge — absence is a normal answer here, not a fault. + const w = await mountCard(INSTALLED) + expect(w.text()).toContain('v1.0.3499884') + expect(w.findAll('v-chip')).toHaveLength(0) + }) +}) diff --git a/tests/test_api_extension.py b/tests/test_api_extension.py index b2885de..65a8a77 100644 --- a/tests/test_api_extension.py +++ b/tests/test_api_extension.py @@ -383,6 +383,53 @@ async def test_extension_manifest_returns_metadata_when_xpi_present(client, monk assert body["sha256"] == hashlib.sha256(b"fake-xpi-content").hexdigest() +@pytest.mark.asyncio +async def test_extension_manifest_reports_the_channel_the_image_declares( + client, monkeypatch, tmp_path +): + """The channel travels BESIDE the version, never inside it. + + Folding it in as a `1.0.3499884-dev` suffix is the failure this design + exists to avoid: the extension's comparator parses each dotted segment as + an integer, so a suffixed segment collapses to 0 and every dev build + compares equal to every other — "no update available" and "I cannot read + this version" stop being distinguishable. Asserting the two are separate + keys is what keeps a future edit from merging them. + """ + (tmp_path / "fabledcurator-1.2.3.xpi").write_bytes(b"x") + monkeypatch.setattr(extension_module, "XPI_DIR", tmp_path) + monkeypatch.setattr(extension_module, "FC_CHANNEL", "dev") + resp = await client.get("/api/extension/manifest") + assert resp.status_code == 200 + body = await resp.get_json() + assert body["channel"] == "dev" + assert body["version"] == "1.2.3" + + +@pytest.mark.asyncio +async def test_extension_manifest_omits_the_channel_when_the_image_declares_none( + client, monkeypatch, tmp_path +): + """A local build, or any image from before the field existed. + + The key must be ABSENT rather than present-and-empty: absence is the state + every consumer already handles (an older image conveys it by not having the + key at all), so a blank channel reuses that path instead of introducing a + second spelling of "unknown" for each reader to special-case. + """ + (tmp_path / "fabledcurator-1.2.3.xpi").write_bytes(b"x") + monkeypatch.setattr(extension_module, "XPI_DIR", tmp_path) + monkeypatch.setattr(extension_module, "FC_CHANNEL", "") + resp = await client.get("/api/extension/manifest") + assert resp.status_code == 200 + body = await resp.get_json() + assert "channel" not in body + # Everything else still answers — an image with no channel is not a + # degraded one, it just cannot say which channel it came from. + assert body["installed"] is True + assert body["latest_url"] == "/extension/fabledcurator-latest.xpi" + + # --- /extension/ -----------------------------------------