refactor(extension): one definition of what ships in the XPI
CI / lint (push) Successful in 3s
CI / extension-version (push) Successful in 4s
extension / lint (push) Successful in 20s
CI / frontend-build (push) Successful in 23s
CI / backend-lint-and-test (push) Successful in 44s
CI / integration (push) Successful in 3m59s

Milestone #271 step 1. Groundwork for deriving the extension version from git;
no behavior change yet -- nothing consumes `version` so far.

"Which files end up in the XPI" was stated in two places and about to become
three. Three hand-kept copies of one fact is what allowed #2397, where the
publish path could republish a stale XPI because its cache key had no link to
the content it stood for.

New extension/scripts/packaging.sh holds the single declaration and exposes:
  ignore       web-ext --ignore-files values
  pathspec     :(exclude)extension/... for git
  version      <MAJOR.MINOR from manifest>.<commit count over packaged files>
  major-minor  /  patch

Consumers now delegate instead of restating it:
- extension/package.json -- all four web-ext scripts
- .forgejo/workflows/ci.yml -- the extension-version guard's exclusions
- (step 4) the rev-list that derives the version

scripts/** joins the non-packaged set; the script must not ship to users.

Two shell hazards, both load-bearing:

The script runs `set -euf`. Its lists are iterated with deliberate word
splitting, and without -f the shell ALSO globs them -- invoking `pathspec`
from a directory where test/ exists (exactly how ci.yml calls it) would expand
`test/**` into the individual spec files and silently stop covering anything
added later. A caller's own `set -f` cannot prevent this: the script is a
separate sh process and does not inherit it.

Callers additionally need their own `set -f` for the substituted RESULT, which
is a different expansion. version.spec.js asserts every --ignore-files caller
sets it, that the pathspec comes through with `test/**` literal and no
.spec.js paths, and that neither consumer has reinstated a hardcoded list --
the easy future regression is "simplifying" by inlining one again.

Verified: all five subcommands plus the usage/exit-2 path. Derived version on
main (8300029) is 1.0.19, matching dev. Last published is 1.0.10, so the
eventual cutover moves strictly upward and needs no offset -- Firefox refuses
downgrades. (An earlier note recorded 18; that was measured against a stale
origin/main from before the PR #234 merge.)

Refs #2398

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude
2026-08-03 15:33:11 -04:00
parent f9111c06a7
commit 597b91d29b
5 changed files with 196 additions and 63 deletions
+77 -41
View File
@@ -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-<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('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