diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index 5f17846..71e5caa 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -121,20 +121,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" diff --git a/ci-requirements.md b/ci-requirements.md index c7a5438..36d0119 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -49,7 +49,15 @@ 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. +- 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. diff --git a/extension/package.json b/extension/package.json index c441e82..66fa304 100644 --- a/extension/package.json +++ b/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": { diff --git a/extension/scripts/packaging.sh b/extension/scripts/packaging.sh new file mode 100755 index 0000000..fce6ba2 --- /dev/null +++ b/extension/scripts/packaging.sh @@ -0,0 +1,92 @@ +#!/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. +NOT_PACKAGED_TRACKED='package.json package-lock.json README.md .gitignore vitest.config.js scripts/** 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 diff --git a/extension/test/version.spec.js b/extension/test/version.spec.js index f5cdf59..7f62ea3 100644 --- a/extension/test/version.spec.js +++ b/extension/test/version.spec.js @@ -1,32 +1,92 @@ 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- 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('test/**') + expect(ignore).toContain('scripts/**') + expect(ignore).toContain('vitest.config.js') + }) +}) + +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 +94,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