diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index 71e5caa..17a6c9f 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -2,7 +2,7 @@ name: CI # CI lanes per FabledRulebook/forgejo.md "CI philosophy": # - lint: ruff only, no dep install — fast-fail for the common lint bounce. -# - extension-version: guards the extension publish path (see the job). +# - extension-version: the derived version resolves and MAJOR.MINOR agrees. # - backend-lint-and-test: `pytest -m "not integration"`, no service containers. # - frontend-build: vitest unit + vite build. # - integration: pgvector + redis service containers; alembic + `pytest -m integration`. @@ -42,18 +42,29 @@ jobs: # catching syntax errors before the image build. run: python -m compileall -q agent/fc_agent - # Guards the extension publish path, which has no self-correcting behavior. + # The extension version is DERIVED, not hand-maintained (milestone 271 step + # 4): build.yml computes it from the commit TIME of the newest packaged + # extension change and stamps it into manifest.json / package.json at build + # time. The guard that used to live here — "packaged files changed but nobody + # bumped the version" — was therefore checking a fact that had stopped + # existing. Worse than useless: it would have failed this lane on every real + # extension change, demanding a bump that decides nothing. Retired 2026-08-27 + # rather than left running beside the new mechanism (rule 22). # - # build.yml's sign-extension job keys its AMO-signing cache purely on the - # version string in extension/package.json: if an `ext-` Forgejo - # release already carries an XPI, signing is SKIPPED and that old signed XPI - # is what build-web bakes into `:latest`. Nothing in that path inspects - # whether extension/ actually changed — so a forgotten version bump ships a - # stale extension on a fully green build, silently. (AMO can't help: it 409s - # on re-signing a version, which is exactly why the cache exists.) + # Two things are still worth asserting, and this is the only lane that can: + # the extension.yml suite runs on node:24-slim, which is exactly why + # version.spec.js sticks to packaging.sh's git-free subcommands. + # 1. the derivation actually resolves on this commit + # 2. MAJOR.MINOR agrees between the two files — the one part still hand-set, + # and packaging.sh reads it from manifest.json ALONE, so a divergence + # ships a version package.json disagrees with # - # This job makes that case loud, on the dev push, instead of invisible at - # merge-to-main. It is pure git + text work — no deps, no services. + # Deliberately NOT checked here: that the derived value beats what has already + # been signed. That guard belongs in build.yml, where it compares against the + # real ext-* releases. Comparing against origin/main here would be wrong — + # dev legitimately derives a LOWER value whenever main is ahead on the + # extension, and a lane that fails for being behind is a lane people learn to + # ignore. extension-version: runs-on: python-ci container: @@ -61,97 +72,37 @@ jobs: steps: - uses: actions/checkout@v4 with: - # Full history: the check diffs against the push's `before` SHA (or - # the PR base), which a depth-1 clone wouldn't contain. + # The derivation needs real history: a depth-1 clone sees one commit + # and produces a wrong, too-low value RATHER THAN FAILING. Checking + # that here is half the point of the lane. fetch-depth: 0 - - name: Extension version guard - env: - BEFORE: ${{ github.event.before }} - PR_BASE: ${{ github.event.pull_request.base.sha }} + - name: Extension version derives cleanly run: | set -eu # busybox sh on the act_runner — no bashisms (family rule). - ver() { grep -E '"version"' "$1" | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([^"]+)".*/\1/'; } - PKG=$(ver extension/package.json) - MAN=$(ver extension/manifest.json) - test -n "$PKG" || { echo "ERROR: no version found in extension/package.json"; exit 1; } - test -n "$MAN" || { echo "ERROR: no version found in extension/manifest.json"; exit 1; } - - # (1) Unconditional: the two version strings must agree. `web-ext sign` - # reads manifest.json (package.json sits in --ignore-files and isn't - # even inside the XPI), so AMO signs MAN and Firefox installs MAN. - # build.yml keys its cache, release tag, XPI filename — and therefore - # the version /api/extension/manifest reports to the update prompt — - # on PKG. Divergence either hard-fails at AMO or ships a mislabelled - # XPI whose update prompt lies about what's installed. + VERSION=$(sh extension/scripts/packaging.sh version) + echo "derived: $VERSION" + # The shape AMO accepts, and the shape build.yml will stamp. + if ! echo "$VERSION" | grep -qE '^[0-9]+(\.[0-9]+)*$'; then + echo "ERROR: derived version '$VERSION' is not plain dotted-numeric." + echo "AMO would reject it, and build.yml stamps it verbatim." + exit 1 + fi + mm() { grep -E '"version"' "$1" | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([0-9]+\.[0-9]+).*/\1/'; } + MAN=$(mm extension/manifest.json) + PKG=$(mm extension/package.json) + test -n "$MAN" || { echo "ERROR: no parseable version in extension/manifest.json"; exit 1; } + test -n "$PKG" || { echo "ERROR: no parseable version in extension/package.json"; exit 1; } if [ "$MAN" != "$PKG" ]; then - echo "ERROR: extension version mismatch." - echo " extension/manifest.json = $MAN <- what AMO signs / Firefox installs" - echo " extension/package.json = $PKG <- what CI caches, names, and reports" - echo "Set both to the same value." + echo "ERROR: MAJOR.MINOR disagrees between the two files." + echo " extension/manifest.json = $MAN <- packaging.sh reads MAJOR.MINOR from here" + echo " extension/package.json = $PKG" + echo "Only MAJOR.MINOR is hand-set. The patch component is derived from" + echo "commit time and overwritten at build time, so the committed patch" + echo "numbers are inert — but MAJOR.MINOR still ships. Set both the same." exit 1 fi - - # (2) If the SHIPPED extension changed, the version must have moved. - # - # Compare against MAIN, not against the previous push. The publish - # decision is made at merge-to-main against whatever ext- - # already exists, so "differs from main" is the question that matters. - # Diffing against the previous dev push instead would demand a fresh - # bump on every iteration — push, tweak the extension again, and CI - # would insist on a second bump that buys nothing, inflating the - # version for no reason. On a main push there is no "main to compare - # to" yet, so fall back to that push's own before-SHA. - if [ "${GITHUB_REF##*/}" = "main" ]; then - BASE="${BEFORE:-}" - else - BASE=$(git rev-parse --verify -q origin/main 2>/dev/null || git rev-parse --verify -q main 2>/dev/null || echo "") - # PR base is the fallback when main isn't in the clone at all. - [ -n "$BASE" ] || BASE="${PR_BASE:-}" - fi - case "$BASE" in - ''|0000000000000000000000000000000000000000) - echo "No usable base ref (no main in clone / first push) — skipping the bump check." - echo "OK: extension version $PKG" - exit 0 - ;; - esac - if ! git cat-file -e "$BASE^{commit}" 2>/dev/null; then - echo "Base commit $BASE not in this clone — skipping the bump check." - echo "OK: extension version $PKG" - exit 0 - fi - # The exclusion list is NOT written out here — it comes from - # extension/scripts/packaging.sh, the one definition of what ships, - # shared with web-ext's --ignore-files and the derived-version patch - # count. Three hand-kept copies of that fact is how #2397 happened. - # - # `set -f` is required around the substitution: without it the shell - # globs `test/**` against the working tree and silently narrows it. - set -f - CHANGED=$(git diff --name-only "$BASE" HEAD -- extension/ $(sh extension/scripts/packaging.sh pathspec)) - set +f - if [ -z "$CHANGED" ]; then - echo "No packaged extension files changed since $BASE — nothing to guard." - echo "OK: extension version $PKG" - exit 0 - fi - echo "Packaged extension files changed since $BASE:" - echo "$CHANGED" | sed 's/^/ /' - PKG_OLD=$(git show "$BASE:extension/package.json" 2>/dev/null | grep -E '"version"' | head -1 | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([^"]+)".*/\1/') - if [ -z "$PKG_OLD" ]; then - echo "Could not read the base version — skipping the bump check." - echo "OK: extension version $PKG" - exit 0 - fi - if [ "$PKG_OLD" = "$PKG" ]; then - echo "ERROR: packaged extension files changed but the version is still $PKG." - echo "build.yml would find the existing ext-$PKG release, skip AMO signing," - echo "and bake the OLD signed XPI into :latest — a green build shipping stale code." - echo "Bump the version in BOTH extension/package.json and extension/manifest.json." - exit 1 - fi - echo "OK: extension version $PKG_OLD -> $PKG" + echo "OK: MAJOR.MINOR $MAN, derived version $VERSION" backend-lint-and-test: runs-on: python-ci diff --git a/.forgejo/workflows/extension.yml b/.forgejo/workflows/extension.yml index c933028..f29cad9 100644 --- a/.forgejo/workflows/extension.yml +++ b/.forgejo/workflows/extension.yml @@ -10,15 +10,20 @@ on: paths: - 'extension/**' - '.forgejo/workflows/extension.yml' - # test/version.spec.js asserts ci.yml's extension-version guard never - # ignores a file web-ext actually packages, so a ci.yml-only edit can - # break this suite and must trigger it. + # test/version.spec.js asserts things ABOUT the other two workflows — + # that neither inlines the packaged-file set, and that build.yml derives + # the shipped version rather than reading it out of the repo. A + # workflow-only edit can therefore break this suite, so it has to trigger + # it. build.yml joined the list at milestone 271 step 5, when the spec + # started asserting against it. - '.forgejo/workflows/ci.yml' + - '.forgejo/workflows/build.yml' pull_request: branches: [main] paths: - 'extension/**' - '.forgejo/workflows/ci.yml' + - '.forgejo/workflows/build.yml' workflow_dispatch: jobs: diff --git a/README.md b/README.md index c13f2f6..915ffa5 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ Five deployable pieces, built by `.forgejo/workflows/build.yml`: | **Web / workers** | `Dockerfile` | `fabledcurator` | Quart API + the built Vue SPA in one image. `entrypoint.sh` picks the role: `web`, `worker`, `scheduler`. The `maintenance-long` service is a second `worker` pinned to the long-running maintenance queue. | | **ML worker** | `Dockerfile.ml` | `fabledcurator-ml` | Same app, plus `requirements-ml.txt` — tagging and embedding models that run in-container. | | **GPU agent** | `agent/Dockerfile` | `fabledcurator-agent` | Optional desktop-GPU worker (`agent/`). Leases jobs over **HTTP only** — never touches the database or Redis. Run it for a burst, stop it to reclaim the card. See `agent/README.md`. | -| **Firefox extension** | `extension/` | signed XPI | MV3 extension: pushes platform session cookies into FC and adds a creator as a Source in one click. AMO-signed on `main` only, then bundled into the web image and served from Settings → Maintenance. See `extension/README.md`. | +| **Firefox extension** | `extension/` | signed XPI | MV3 extension: pushes platform session cookies into FC and adds a creator as a Source in one click. AMO-signed on both `dev` and `main` (one signature per extension change, shared by the two channels), bundled into that channel's web image and served from Settings → Maintenance. See `extension/README.md`. | | **Data** | — | `pgvector/pgvector:pg16`, `redis:7-alpine` | Postgres with pgvector for embeddings; Redis as the Celery broker. | ## Quick start @@ -52,7 +52,7 @@ FabledCurator is designed to run inside a self-hosted homelab environment over p ## CI / Forgejo setup -Three workflows: `ci.yml` (lint, extension-version guard, backend unit tests, +Three workflows: `ci.yml` (lint, extension-version check, backend unit tests, frontend build, integration), `extension.yml` (extension lint, vitest, XPI content verification), and `build.yml` (sign + publish). diff --git a/ci-requirements.md b/ci-requirements.md index 620da37..4d0f3c4 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -54,17 +54,25 @@ per `docs/process.md`'s "add deps to the image when used by >1 project". shims to production code — the libs ship as `background.scripts`, not ES modules, so the specs exercise exactly the bytes packaged into the XPI. - **`extension/scripts/packaging.sh` is the single definition of what ships - inside the XPI.** Three consumers read from it rather than keeping their own - copy: web-ext's `--ignore-files` (`extension/package.json`), the `:(exclude)` - pathspec in `ci.yml`'s `extension-version` guard, and the `git log` pathspec - that derives the extension version. Three hand-kept copies of that one fact - is what allowed issue #2397. -- Jobs that derive the extension version check out with `fetch-depth: 0`. The - version is the commit TIME of the newest packaged-extension change (minutes - since 2020-01-01, per family rule 149 — never a commit count, which orders - by branch rather than by recency). 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 wherever `packaging.sh version` is called. + inside the XPI.** Two consumers read from it rather than keeping their own + copy: web-ext's `--ignore-files` (`extension/package.json`), and the `git log` + pathspec inside the script's own version derivation. It was three until + 2026-08-27 — `ci.yml`'s `extension-version` guard held the third and went when + the manual bump it guarded did (milestone 271 step 5). Hand-kept copies of + that one fact is what allowed issue #2397, so `extension/test/version.spec.js` + asserts no workflow has reintroduced a literal `:(exclude)extension/…`. +- **The shipped extension version is derived, not committed.** It is the commit + TIME of the newest packaged-extension change (minutes since 2020-01-01, per + family rule 149 — never a commit count, which orders by branch rather than by + recency). `build.yml`'s `sign-extension` computes it and stamps it into + `extension/manifest.json` + `package.json` in the working tree before signing; + the stamp is never committed. Treat the version in the repo as a base: only + its MAJOR.MINOR is read, and its patch component is inert. +- Every job that calls `packaging.sh version` checks out with `fetch-depth: 0` — + `build.yml`'s `sign-extension` and `build-web`, and `ci.yml`'s + `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. - 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 e780d95..22db07c 100644 --- a/extension/README.md +++ b/extension/README.md @@ -7,7 +7,8 @@ page in one click. ## Install (operator) -The signed XPI is bundled into the FC Docker image. Open FC → +The signed XPI is bundled into the FC Docker image — `:dev` and +`:latest` each carry their own channel's build. Open FC → Settings → Maintenance → Browser extension → click "Install Firefox extension". Firefox shows its native install prompt. After installing, open the extension's options page (about:addons → FabledCurator → @@ -20,6 +21,7 @@ same card. cd extension/ npm install --no-save # web-ext only npm run lint # web-ext lint +npm run test:unit # vitest — lib/ logic + packaging/version checks npm run start # launches Firefox with extension loaded npm run build # unsigned XPI in web-ext-artifacts/ ``` @@ -36,10 +38,42 @@ npm run build # unsigned XPI in web-ext-artifacts/ - [ ] Subscriptions list: popup → "Sources" tab → list renders - [ ] Check now: click play icon on source row → no error toast +## Versioning — don't hand-edit the patch number + +The shipped version is **derived**, not committed. `scripts/packaging.sh +version` returns `MAJOR.MINOR` from `manifest.json` plus a patch component +that is the commit *time* of the newest change to a packaged extension file, +in minutes since 2020-01-01. `build.yml` computes it and stamps it into both +`manifest.json` and `package.json` at build time. The stamp is never +committed — the commit carrying it would itself be a change to the extension, +which would move the version again. + +So: + +- **Editing the patch number does nothing.** It is overwritten before web-ext + ever reads it. There is no bump to make, and none to forget. +- **MAJOR.MINOR is still yours.** It carries the deliberate meaning, it is read + from `manifest.json` alone, and CI fails the `extension-version` lane if the + two files disagree on it. +- `npm run build` locally produces an XPI labelled with the *committed* + version, since nothing stamped it. Fine for loading into a test profile; not + what ships. + +Why commit time and not a commit count: a count is per-branch, so `dev` and +`main` count different histories of the same code and their versions end up +ordered by which branch accumulated more commits rather than by which is newer. +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). + ## Release -Bump `manifest.json` + `package.json` SemVer (both files) and commit -under `extension/**`. The `.forgejo/workflows/extension.yml` workflow -runs `web-ext sign` on main, commits the signed XPI to -`frontend/public/extension/`, and the next FC server build bundles it -into the Docker image. +Nothing to do by hand. Push to `dev`: `build.yml` signs the extension if this +change moved the version, caches the signed XPI as a Forgejo `ext-` +release, and bundles it into `fabledcurator:dev`. Merging to `main` derives the +same version, hits that cache, and bundles the byte-identical XPI into +`:latest` with no second AMO call. + +AMO refuses to re-sign a version it has already issued, so signing is one-shot +per version — which is why the cache exists and why the version must never move +backwards. diff --git a/extension/test/version.spec.js b/extension/test/version.spec.js index d39a41e..4e69a69 100644 --- a/extension/test/version.spec.js +++ b/extension/test/version.spec.js @@ -77,21 +77,56 @@ describe('consumers delegate rather than keeping their own copy', () => { } }) - it('ci.yml derives its pathspec from the script and hardcodes none', () => { - const ci = readText('..', '.forgejo', 'workflows', 'ci.yml') - expect(ci).toContain('extension/scripts/packaging.sh pathspec') - // A literal :(exclude)extension/... in the workflow means someone bypassed - // the shared definition. - expect(ci).not.toMatch(/:\(exclude\)extension\//) + const WORKFLOWS = ['ci.yml', 'build.yml', 'extension.yml'] + + it('no workflow hardcodes the packaged-file set', () => { + // ci.yml used to substitute `packaging.sh pathspec` directly, for the + // manual-bump guard that milestone 271 step 5 retired. Nothing inlines the + // set today, and nothing should start to: a literal :(exclude)extension/... + // in a workflow means someone bypassed the shared definition, which is + // exactly the drift #2397 was about. Asserted across all three rather than + // against one named consumer, so it keeps holding as consumers come and go. + for (const wf of WORKFLOWS) { + const text = readText('..', '.forgejo', 'workflows', wf) + expect(text, `${wf} inlines an :(exclude) literal`).not.toMatch(/:\(exclude\)extension\//) + } + }) + + it('build.yml takes the shipped version from the script, not from the repo', () => { + // The version is DERIVED from commit time (#3092, milestone 271 step 4). + // Going back to reading the committed value is not a style regression, it + // is the bug: a hand-set version makes dev and main sign the same number + // for different code, and the ext- cache then serves one channel + // the other's XPI. + const build = readText('..', '.forgejo', 'workflows', 'build.yml') + expect(build).toContain('packaging.sh version') + expect(build, 'build.yml re-reads the committed version instead of deriving it') + .not.toMatch(/grep[^\n]*'"version"'[^\n]*package\.json/) }) }) describe('extension version', () => { - it('keeps manifest.json and package.json in lockstep', () => { - expect(read('manifest.json').version).toBe(read('package.json').version) + const majorMinor = (v) => v.split('.').slice(0, 2).join('.') + + it('keeps the hand-set MAJOR.MINOR in lockstep across both files', () => { + // Narrowed from full-string equality at milestone 271 step 5. Since step 4 + // the patch component is derived from commit time and stamped into both + // files at build time, so the committed patch numbers are inert — nothing + // reads them and they are not what ships. Asserting on them would fail for + // a difference that changes nothing. + // + // MAJOR.MINOR is the opposite: still hand-set, still shipped, and + // packaging.sh reads it from manifest.json ALONE. Let the two diverge and + // the extension ships a version package.json disagrees with, with no other + // signal. + expect(majorMinor(read('manifest.json').version)) + .toBe(majorMinor(read('package.json').version)) }) it('uses a plain dotted numeric version AMO will accept', () => { + // The committed value seeds MAJOR.MINOR, so it still has to parse even + // though its patch component never ships. ci.yml asserts the same shape on + // the DERIVED value, which is the one AMO actually sees. expect(read('package.json').version).toMatch(/^\d+(\.\d+)*$/) })