Merge pull request 'Extension packaging: one source of truth, shadow-mode versioning, real XPI verification' (#235) from dev into main

.forgejo/workflows/build.yml

@@ -40,6 +40,11 @@ jobs:
       image: git.fabledsword.com/bvandeusen/ci-python:3.14
     steps:
       - uses: actions/checkout@v4
+        with:
+          # Full history: the shadow-mode step below derives a version from a
+          # commit count, which a depth-1 clone cannot produce. Harmless for
+          # everything else in this job.
+          fetch-depth: 0
 
       - name: Resolve extension version
         id: extver
@@ -48,6 +53,25 @@ jobs:
           echo "version=$VERSION" >> "$GITHUB_OUTPUT"
           echo "Resolved extension version: $VERSION"
 
+      # --- shadow mode (milestone #271, step 2) ---------------------------
+      # Informational ONLY — nothing downstream reads this, and it must never
+      # fail the build. This is THE place the derived formula gets validated:
+      # `sign-extension` only runs on main, so main pushes are the sole source
+      # of truth for whether the derived version moves exactly when the shipped
+      # extension changes. Compare these lines across several main builds
+      # before step 4 lets the derived value control publishing.
+      - name: Shadow — derived version (informational)
+        run: |
+          set -u
+          DERIVED=$(sh extension/scripts/packaging.sh version 2>&1 || echo "UNAVAILABLE")
+          MANUAL=${{ steps.extver.outputs.version }}
+          echo "shadow: manual=$MANUAL derived=$DERIVED sha=$GITHUB_SHA"
+          if [ "$MANUAL" = "$DERIVED" ]; then
+            echo "shadow: manual and derived agree"
+          else
+            echo "shadow: DIVERGENT — expected until step 4 cuts over; derived is authoritative-to-be"
+          fi
+
       - name: Check Forgejo release-asset cache
         id: cache
         env:

.forgejo/workflows/ci.yml

@@ -77,6 +77,17 @@ jobs:
           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; }
 
+          # --- shadow mode (milestone #271, step 2) -------------------------
+          # Informational ONLY: nothing below reads DERIVED, and this must never
+          # fail the job. `web-ext sign` is one-shot per version (AMO 409s on a
+          # repeat), so a wrong formula would burn a real version number that
+          # can't be reclaimed. Logging it against real pushes first is the only
+          # way to validate it at zero cost.
+          # Placed before every early-exit path so it reports on all runs.
+          DERIVED=$(sh extension/scripts/packaging.sh version 2>&1 || echo "UNAVAILABLE")
+          echo "shadow: manual=$PKG derived=$DERIVED"
+          # -----------------------------------------------------------------
+
           # (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.
@@ -121,20 +132,16 @@ jobs:
             echo "OK: extension version $PKG"
             exit 0
           fi
-          # Exclusions mirror --ignore-files in extension/package.json's web-ext
-          # scripts: these files are not packaged into the XPI, so touching them
-          # (e.g. Renovate bumping the web-ext devDep, or editing a spec)
-          # changes nothing shipped and must not demand a version bump.
-          # KEEP IN SYNC with --ignore-files — a file packaged into the XPI but
-          # excluded here is exactly the silent-stale-ship this job exists to
-          # prevent. test/version.spec.js pins the two lists' shared intent.
-          CHANGED=$(git diff --name-only "$BASE" HEAD -- extension/ \
-            ':(exclude)extension/package.json' \
-            ':(exclude)extension/package-lock.json' \
-            ':(exclude)extension/README.md' \
-            ':(exclude)extension/.gitignore' \
-            ':(exclude)extension/vitest.config.js' \
-            ':(exclude)extension/test/**')
+          # 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"

.forgejo/workflows/extension.yml

@@ -38,3 +38,45 @@ jobs:
       # package version-consistency checks. No browser, no network.
       - name: Unit tests
         run: cd extension && npm run test:unit
+
+      # Everything else about packaging is asserted against our own declaration
+      # of what ships. This is the only check that asks web-ext what it ACTUALLY
+      # put in the archive. Until now that was an unverified assumption about
+      # glob semantics — and a fragile one: `test/**` reaches web-ext intact
+      # only because callers `set -f` first, so losing that quoting would
+      # silently start shipping dev files with no other signal.
+      - name: Verify XPI contents
+        run: |
+          set -eu
+          command -v unzip >/dev/null 2>&1 || { apt-get update -qq && apt-get install -y -qq unzip; }
+          cd extension
+          npm run build
+          ZIP=$(ls web-ext-artifacts/*.zip | head -1)
+          echo "=== packaged entries in $ZIP ==="
+          unzip -Z1 "$ZIP" | sort
+          echo "=== end ==="
+          ENTRIES=$(unzip -Z1 "$ZIP")
+          fail=0
+          # Must NOT ship: repo infrastructure with no business in a user's browser.
+          for pat in 'test/' 'scripts/' 'vitest.config.js' 'package.json' 'package-lock.json' 'README.md' 'node_modules/' 'web-ext-artifacts/'; do
+            if echo "$ENTRIES" | grep -q "^$pat"; then
+              echo "ERROR: '$pat' was packaged into the XPI but must not be"
+              fail=1
+            fi
+          done
+          # Must ship: if an exclusion pattern ever over-matches, the extension
+          # breaks at runtime rather than at build time, so assert presence too.
+          for req in 'manifest.json' 'lib/url.js' 'lib/api.js' 'lib/platforms.js' 'lib/cookies.js'; do
+            if ! echo "$ENTRIES" | grep -q "^$req$"; then
+              echo "ERROR: '$req' is missing from the XPI"
+              fail=1
+            fi
+          done
+          for dir in 'background/' 'popup/' 'options/' 'content/' 'icons/'; do
+            if ! echo "$ENTRIES" | grep -q "^$dir"; then
+              echo "ERROR: nothing from '$dir' was packaged"
+              fail=1
+            fi
+          done
+          [ "$fail" -eq 0 ] || exit 1
+          echo "XPI contents verified."

ci-requirements.md

@@ -27,6 +27,10 @@ per `docs/process.md`'s "add deps to the image when used by >1 project".
 - `pip install -r requirements.txt pytest pytest-asyncio` — in `backend-lint-and-test` and `integration` jobs
 - `npm install --no-audit --no-fund` — in `frontend-build` job
 - `npm install --no-audit --no-fund` — in `extension.yml`'s `lint` job (web-ext + vitest)
+- `unzip` — in `extension.yml`'s "Verify XPI contents" step, installed via apt
+  only when absent (`node:24-bookworm-slim` may or may not carry it). Debian
+  package, ~2s. Not worth baking into a shared image for a single consumer, per
+  `docs/process.md`'s ">1 project" rule.
 
 ## Notes
 
@@ -49,7 +53,17 @@ per `docs/process.md`'s "add deps to the image when used by >1 project".
   classic script (`test/helpers/loadLib.js`) rather than adding `module.exports`
   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 test files are excluded from the XPI via `--ignore-files` in
-  `extension/package.json`, and the same paths are excluded from `ci.yml`'s
-  `extension-version` guard. Those two lists must agree — `test/version.spec.js`
-  asserts the guard never ignores a file web-ext actually packages.
+- **`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 commit count that
+  derives the extension version. Three hand-kept copies of that one fact is
+  what allowed issue #2397.
+- `build.yml`'s `sign-extension` checks out with `fetch-depth: 0` — the derived
+  extension version is a commit count, which a shallow clone cannot produce.
+- 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
+  nothing until dev files start appearing in the XPI. `test/version.spec.js`
+  asserts every `--ignore-files` consumer sets it, and that no consumer has
+  quietly reinstated a hardcoded list.

extension/package.json

@@ -3,11 +3,12 @@
   "version": "1.0.10",
   "private": true,
   "description": "Firefox extension for FabledCurator",
+  "comment_ignore_files": "The --ignore-files list comes from scripts/packaging.sh, the single source of truth shared with ci.yml's guard and the derived-version patch count. `set -f` is REQUIRED before the substitution: without it the shell globs `test/**` against the working tree and silently narrows the pattern to whatever files happen to exist.",
   "scripts": {
-    "lint":  "web-ext lint --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\"",
-    "start": "web-ext run --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\" --firefox=firefox",
-    "build": "web-ext build --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\" --overwrite-dest",
-    "sign":  "web-ext sign --source-dir=. --no-config-discovery --ignore-files package.json package-lock.json web-ext-artifacts node_modules README.md .gitignore vitest.config.js \"test/**\" --channel=unlisted --api-key=$WEB_EXT_API_KEY --api-secret=$WEB_EXT_API_SECRET",
+    "lint":  "set -f; web-ext lint --source-dir=. --no-config-discovery --ignore-files $(sh scripts/packaging.sh ignore)",
+    "start": "set -f; web-ext run --source-dir=. --no-config-discovery --ignore-files $(sh scripts/packaging.sh ignore) --firefox=firefox",
+    "build": "set -f; web-ext build --source-dir=. --no-config-discovery --ignore-files $(sh scripts/packaging.sh ignore) --overwrite-dest",
+    "sign":  "set -f; web-ext sign --source-dir=. --no-config-discovery --ignore-files $(sh scripts/packaging.sh ignore) --channel=unlisted --api-key=$WEB_EXT_API_KEY --api-secret=$WEB_EXT_API_SECRET",
     "test:unit": "vitest run"
   },
   "devDependencies": {

extension/scripts/packaging.sh

@@ -0,0 +1,99 @@
+#!/bin/sh
+# Single source of truth for "what ships inside the XPI", plus the version
+# derived from it.
+#
+# Three consumers used to hand-maintain their own copy of this list, and
+# keeping three copies of one fact in sync by hand is how issue #2397 happened:
+#
+#   1. web-ext's --ignore-files      (extension/package.json's four scripts)
+#   2. the :(exclude) pathspec       (ci.yml's extension-version guard)
+#   3. the rev-list pathspec         (the derived version, below)
+#
+# They now all read from here. POSIX sh only — CI's run shell is busybox.
+#
+# -f (no pathname expansion) is set for the whole script and is load-bearing:
+# the lists below are iterated with deliberate word-splitting, and without -f
+# the shell would also GLOB them, expanding `test/**` into whatever files
+# happen to exist and corrupting the output. A caller's own `set -f` does not
+# help here — this runs as a separate sh process and does not inherit it.
+# Callers still need their own `set -f` for the substituted result; the two
+# guards protect different expansions.
+set -euf
+
+# Paths under extension/ that are NOT packaged into the XPI.
+#
+# Split by whether git tracks them: node_modules and web-ext-artifacts are
+# build/dependency output that never appears in a commit, so they belong in
+# web-ext's ignore list but would be meaningless in a git pathspec.
+#
+# Directories need BOTH forms. `test/**` matches the files inside, but not the
+# directory entry itself — web-ext writes an entry for the directory too, so
+# with only the glob the XPI ends up carrying empty `test/` and `scripts/`
+# entries (caught by the XPI-content check on 2026-08-03). The bare name alone
+# is not enough either: minimatch's `test` does not match `test/url.spec.js`,
+# so dropping the glob would ship the contents. Keep both.
+NOT_PACKAGED_TRACKED='package.json package-lock.json README.md .gitignore vitest.config.js scripts scripts/** test test/**'
+NOT_PACKAGED_BUILD='web-ext-artifacts node_modules'
+
+usage() {
+  echo "usage: packaging.sh {ignore|pathspec|version|major-minor|patch}" >&2
+  exit 2
+}
+
+# web-ext --ignore-files values, space-separated.
+#
+# Callers MUST disable pathname expansion first (`set -f`), or the shell will
+# glob `test/**` against the working tree before web-ext ever sees the pattern
+# and silently narrow it to whatever happens to exist right now.
+cmd_ignore() {
+  echo "$NOT_PACKAGED_TRACKED $NOT_PACKAGED_BUILD"
+}
+
+# git pathspec excluding the non-packaged tracked files, e.g.
+#   :(exclude)extension/package.json :(exclude)extension/test/**
+# Same `set -f` requirement as above.
+cmd_pathspec() {
+  for entry in $NOT_PACKAGED_TRACKED; do
+    printf ':(exclude)extension/%s ' "$entry"
+  done
+  echo
+}
+
+# MAJOR.MINOR stays hand-set in manifest.json — it's the part that carries
+# deliberate meaning. Only the patch component is derived.
+cmd_major_minor() {
+  root=$(git rev-parse --show-toplevel)
+  grep -E '"version"' "$root/extension/manifest.json" \
+    | head -1 \
+    | sed -E 's/.*"version"[[:space:]]*:[[:space:]]*"([0-9]+)\.([0-9]+).*/\1.\2/'
+}
+
+# Count of commits that touched a PACKAGED extension file. Monotonic on a
+# branch (the count only grows), which is a correctness requirement, not a
+# nicety: Firefox refuses to install a version lower than the one present.
+#
+# Merge commits need no special handling — git's history simplification already
+# prunes merges that don't change the pathspec, so --no-merges is a no-op here
+# (verified on main: both forms return the same count).
+cmd_patch() {
+  root=$(git rev-parse --show-toplevel)
+  # Unquoted on purpose: the pathspec must word-split into separate args.
+  # Globbing is already off script-wide (set -euf above).
+  # shellcheck disable=SC2046
+  count=$(cd "$root" && git rev-list --count HEAD -- extension/ $(cmd_pathspec))
+  echo "$count"
+}
+
+cmd_version() {
+  echo "$(cmd_major_minor).$(cmd_patch)"
+}
+
+[ $# -ge 1 ] || usage
+case "$1" in
+  ignore)      cmd_ignore ;;
+  pathspec)    cmd_pathspec ;;
+  version)     cmd_version ;;
+  major-minor) cmd_major_minor ;;
+  patch)       cmd_patch ;;
+  *)           usage ;;
+esac

extension/test/version.spec.js

@@ -1,32 +1,97 @@
 import { describe, it, expect } from 'vitest'
 import { readFileSync } from 'node:fs'
+import { execFileSync } from 'node:child_process'
 import { fileURLToPath } from 'node:url'
 import path from 'node:path'
 
 const EXT_DIR = path.join(path.dirname(fileURLToPath(import.meta.url)), '..')
 const read = (name) => JSON.parse(readFileSync(path.join(EXT_DIR, name), 'utf8'))
+const readText = (...seg) => readFileSync(path.join(EXT_DIR, ...seg), 'utf8')
 
-describe('extension version consistency', () => {
-  // Duplicates check (1) of ci.yml's extension-version job, deliberately.
-  // That job is the gate that can't be bypassed; this spec is the one that
-  // fails in a second on the developer's own CI lane with a readable diff.
-  // The two version strings feed different systems and nothing else reconciles
-  // them:
-  //   manifest.json  -> what `web-ext sign` signs, so what Firefox installs
-  //                     (package.json is in --ignore-files, not in the XPI)
-  //   package.json   -> build.yml's AMO cache key, the ext-<version> release
-  //                     tag, the XPI filename, and therefore the version
-  //                     /api/extension/manifest reports to the update prompt
+// Only the git-free subcommands are exercised here: `version`/`patch` shell out
+// to git, and the extension lane runs on node:24-bookworm-slim which may not
+// ship it. Those two are covered where git is guaranteed — ci.yml and build.yml
+// run on ci-python.
+const packaging = (cmd) =>
+  execFileSync('sh', [path.join(EXT_DIR, 'scripts', 'packaging.sh'), cmd], {
+    cwd: EXT_DIR,
+    encoding: 'utf8'
+  })
+    .trim()
+    .split(/\s+/)
+    .filter(Boolean)
+
+describe('packaging.sh — the single definition of what ships', () => {
+  it('emits an ignore list and a pathspec that agree on the tracked files', () => {
+    const ignore = packaging('ignore')
+    const pathspec = packaging('pathspec').map((e) => e.replace(':(exclude)extension/', ''))
+
+    // Every git-excluded path must also be hidden from web-ext. The reverse is
+    // not required: node_modules and web-ext-artifacts are build output git
+    // never tracks, so they appear only in the ignore list.
+    for (const entry of pathspec) {
+      expect(ignore, `pathspec has "${entry}" but --ignore-files does not`).toContain(entry)
+    }
+    expect(pathspec.length).toBeGreaterThan(0)
+    expect(ignore).toContain('node_modules')
+  })
+
+  it('emits glob patterns literally, never expanded against the working tree', () => {
+    // The script iterates its lists with deliberate word-splitting, so it must
+    // run with pathname expansion off. Without that, invoking it from a cwd
+    // where test/ exists (exactly how ci.yml and vitest call it) expands
+    // `test/**` into the individual spec files, and the pathspec silently stops
+    // covering anything added later.
+    const pathspec = packaging('pathspec')
+    expect(pathspec).toContain(':(exclude)extension/test/**')
+    expect(pathspec).toContain(':(exclude)extension/scripts/**')
+    expect(pathspec.some((e) => e.includes('.spec.js'))).toBe(false)
+    expect(pathspec.some((e) => e.includes('helpers'))).toBe(false)
+  })
+
+  it('keeps its own scripts and specs out of the XPI', () => {
+    // Both are repo infrastructure. web-ext packages everything not ignored, so
+    // omitting either would ship dev tooling to users -- and `test/**` in
+    // particular only survives because callers `set -f` before substituting it.
+    const ignore = packaging('ignore')
+    expect(ignore).toContain('vitest.config.js')
+    // Both forms per directory. The glob covers the contents; the bare name
+    // covers the directory ENTRY, which web-ext writes separately — with only
+    // the glob, the XPI carries an empty `test/` and `scripts/`.
+    for (const dir of ['test', 'scripts']) {
+      expect(ignore, `${dir} contents`).toContain(`${dir}/**`)
+      expect(ignore, `${dir} directory entry`).toContain(dir)
+    }
+  })
+})
+
+describe('consumers delegate rather than keeping their own copy', () => {
+  // These assertions are the actual anti-regression value: it is easy for a
+  // future edit to "simplify" by inlining a literal list again, which silently
+  // reintroduces the drift that issue #2397 was about.
+  it('package.json derives --ignore-files from the script', () => {
+    for (const [name, script] of Object.entries(read('package.json').scripts)) {
+      if (!script.includes('--ignore-files')) continue
+      expect(script, `${name} should call packaging.sh`).toContain('scripts/packaging.sh ignore')
+      expect(script, `${name} must set -f before the substitution`).toMatch(/set -f;/)
+    }
+  })
+
+  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\//)
+  })
+})
+
+describe('extension version', () => {
   it('keeps manifest.json and package.json in lockstep', () => {
-    const manifest = read('manifest.json')
-    const pkg = read('package.json')
-    expect(manifest.version).toBe(pkg.version)
+    expect(read('manifest.json').version).toBe(read('package.json').version)
   })
 
   it('uses a plain dotted numeric version AMO will accept', () => {
-    // AMO rejects exotic version strings, and build.yml embeds this value in a
-    // release tag and a filename — so anything needing escaping breaks the
-    // publish path rather than the extension.
     expect(read('package.json').version).toMatch(/^\d+(\.\d+)*$/)
   })
 
@@ -34,30 +99,6 @@ describe('extension version consistency', () => {
     expect(read('manifest.json').manifest_version).toBe(3)
   })
 
-  it('never lets the CI guard ignore a file that actually ships', () => {
-    // ci.yml's extension-version job skips its bump check for paths it deems
-    // non-shipping. If it excludes something web-ext DOES package, a real
-    // change to shipped code passes the guard unnoticed — precisely the
-    // silent-stale-ship the guard exists to stop. The reverse drift (guard
-    // stricter than web-ext) only costs a needless bump, so it isn't asserted.
-    const ci = readFileSync(path.join(EXT_DIR, '..', '.forgejo', 'workflows', 'ci.yml'), 'utf8')
-    const lint = read('package.json').scripts.lint
-    const after = lint.split('--ignore-files')[1] ?? ''
-    const ignored = new Set(
-      after
-        .split(/\s+/)
-        .filter((tok) => tok && !tok.startsWith('--'))
-        .map((tok) => tok.replace(/^["']|["']$/g, ''))
-    )
-    expect(ignored.size, 'parsed --ignore-files from the lint script').toBeGreaterThan(0)
-
-    const guarded = [...ci.matchAll(/:\(exclude\)extension\/(\S+?)'/g)].map((m) => m[1])
-    expect(guarded.length, 'parsed :(exclude) entries from ci.yml').toBeGreaterThan(0)
-    for (const entry of guarded) {
-      expect(ignored, `ci.yml excludes "${entry}" but web-ext packages it`).toContain(entry)
-    }
-  })
-
   it('lists every background script that exists, in dependency order', () => {
     // url.js must load BEFORE api.js: api.js calls normalizeApiUrl at
     // init()-time, and these are classic scripts sharing one scope, so a