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

The XPI-content check added in 1c6452e did its job on its first run: the
archive carried empty `test/` and `scripts/` entries.

`test/**` matches the files inside a directory but not the directory entry
itself, and web-ext writes an entry for the directory separately -- so the
contents were correctly excluded while the empty directories shipped anyway.
Nothing harmful reached users (no dev code, just two empty entries), but our
single declaration claimed these do not ship and something was shipping.

Both forms are now listed per directory. The bare name alone would not do:
minimatch's `test` does not match `test/url.spec.js`, so dropping the glob
would ship the contents instead.

Verified locally: the derived version is unchanged at 1.0.19, confirming the
added entries are redundant for the git pathspec and only affect web-ext.

Refs #2400

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

.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
+          # The exclusion list is NOT written out here — it comes from
-          # scripts: these files are not packaged into the XPI, so touching them
+          # extension/scripts/packaging.sh, the one definition of what ships,
-          # (e.g. Renovate bumping the web-ext devDep, or editing a spec)
+          # shared with web-ext's --ignore-files and the derived-version patch
-          # changes nothing shipped and must not demand a version bump.
+          # count. Three hand-kept copies of that fact is how #2397 happened.
-          # 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
+          # `set -f` is required around the substitution: without it the shell
-          # prevent. test/version.spec.js pins the two lists' shared intent.
+          # globs `test/**` against the working tree and silently narrows it.
-          CHANGED=$(git diff --name-only "$BASE" HEAD -- extension/ \
+          set -f
-            ':(exclude)extension/package.json' \
+          CHANGED=$(git diff --name-only "$BASE" HEAD -- extension/ $(sh extension/scripts/packaging.sh pathspec))
-            ':(exclude)extension/package-lock.json' \
+          set +f
-            ':(exclude)extension/README.md' \
-            ':(exclude)extension/.gitignore' \
-            ':(exclude)extension/vitest.config.js' \
-            ':(exclude)extension/test/**')
           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/scripts/packaging.sh` is the single definition of what ships
-  `extension/package.json`, and the same paths are excluded from `ci.yml`'s
+  inside the XPI.** Three consumers read from it rather than keeping their own
-  `extension-version` guard. Those two lists must agree — `test/version.spec.js`
+  copy: web-ext's `--ignore-files` (`extension/package.json`), the `:(exclude)`
-  asserts the guard never ignores a file web-ext actually packages.
+  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/**\"",
+    "lint":  "set -f; web-ext lint --source-dir=. --no-config-discovery --ignore-files $(sh scripts/packaging.sh ignore)",
-    "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",
+    "start": "set -f; web-ext run --source-dir=. --no-config-discovery --ignore-files $(sh scripts/packaging.sh ignore) --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",
+    "build": "set -f; web-ext build --source-dir=. --no-config-discovery --ignore-files $(sh scripts/packaging.sh ignore) --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",
+    "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', () => {
+// Only the git-free subcommands are exercised here: `version`/`patch` shell out
-  // Duplicates check (1) of ci.yml's extension-version job, deliberately.
+// to git, and the extension lane runs on node:24-bookworm-slim which may not
-  // That job is the gate that can't be bypassed; this spec is the one that
+// ship it. Those two are covered where git is guaranteed — ci.yml and build.yml
-  // fails in a second on the developer's own CI lane with a readable diff.
+// run on ci-python.
-  // The two version strings feed different systems and nothing else reconciles
+const packaging = (cmd) =>
-  // them:
+  execFileSync('sh', [path.join(EXT_DIR, 'scripts', 'packaging.sh'), cmd], {
-  //   manifest.json  -> what `web-ext sign` signs, so what Firefox installs
+    cwd: EXT_DIR,
-  //                     (package.json is in --ignore-files, not in the XPI)
+    encoding: 'utf8'
-  //   package.json   -> build.yml's AMO cache key, the ext-<version> release
+  })
-  //                     tag, the XPI filename, and therefore the version
+    .trim()
-  //                     /api/extension/manifest reports to the update prompt
+    .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')
+    expect(read('manifest.json').version).toBe(read('package.json').version)
-    const pkg = read('package.json')
-    expect(manifest.version).toBe(pkg.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