From fb2c4d5b80be1daa98d6f7d4f7da5b99b9faf4ba Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 28 Aug 2026 08:49:06 -0400 Subject: [PATCH] fix(extension): packaging.sh decides the version, so it must move the version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `packaging.sh pathspec` excluded `extension/scripts/**` — the same list web-ext ignores. But the two lists answer different questions, and this is the one place they disagree. packaging.sh is not packaged into the XPI. It does decide the version string, and build.yml stamps that string into the manifest.json that IS packaged. Changing how the version is computed therefore changes the shipped bytes, and the derivation has to see it. Harmless while every push rebuilt the web image. Step 4 made the rebuild conditional on the derived revision moving, which turns it into a silent failure: a packaging.sh change yields a NEW version, so sign-extension misses its ext- cache and signs — while build-web sees an unmoved revision, reuses the published image and ships the OLD XPI. One orphaned AMO signature, and an instance serving code the registry calls current. Found while checking the ground under step 5, which changes the version format and is exactly the commit that would have hit it. Split the list rather than widening the shared one: NOT_VERSION_RELEVANT drives the pathspec, NOT_PACKAGED_TRACKED still drives web-ext's ignore list, and scripts/ stays out of the XPI. The two directions are not symmetric, which is why the version list is the narrower one — too wide costs a re-sign and a rebuild for a change that ships nothing new, too narrow serves stale bytes and says nothing. No version churn: the last packaging.sh commit predates the current extension revision, so the derived version is unchanged at 1.0.3500147 and web's revision stays a7e626a67a79. Both suites now pin the disagreement from their own side, because the tempting fix for either half is to make the lists one again — version.spec.js asserts the pathspec does NOT exclude scripts while the ignore list still does, and test_artifact_paths.py asserts packaging.sh is inside the extension and web path sets. --- extension/scripts/packaging.sh | 35 ++++++++++++++++++++++++++++++---- extension/test/version.spec.js | 32 ++++++++++++++++++++++++++++++- tests/test_artifact_paths.py | 30 +++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/extension/scripts/packaging.sh b/extension/scripts/packaging.sh index a8aa969..399aa43 100755 --- a/extension/scripts/packaging.sh +++ b/extension/scripts/packaging.sh @@ -6,7 +6,9 @@ # 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) +# 2. the :(exclude) pathspec (what moves the version — a WIDER +# set than the ignore list; see +# NOT_VERSION_RELEVANT) # 3. the git-log pathspec (the derived version, below) # # They now all read from here. POSIX sh only — CI's run shell is busybox. @@ -35,6 +37,28 @@ set -euf 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' +# Paths under extension/ that cannot change the SHIPPED BYTES, and so must not +# move the derived version. +# +# Deliberately NOT the same list as NOT_PACKAGED_TRACKED, and the whole +# difference is `scripts/`. packaging.sh is not packaged into the XPI — but it +# DECIDES the version string, and build.yml stamps that string into the +# manifest.json that is packaged. A change to how the version is computed is +# therefore a change to the shipped bytes. +# +# Excluding it was harmless only while every push rebuilt the web image. +# Milestone 313 step 4 made the rebuild conditional on the derived revision +# moving, which turned it into a silent failure: a packaging.sh change gives a +# NEW version, so sign-extension misses its ext- cache and signs — +# while build-web sees an unmoved revision, reuses the published image, and +# ships the OLD XPI. An orphaned AMO signature, and an instance quietly serving +# code the registry says is current. +# +# The two directions are not symmetric, which is why this list is the narrower +# one. Too wide costs a re-sign and a rebuild for a change that ships nothing +# new. Too narrow serves stale bytes and says nothing. +NOT_VERSION_RELEVANT='package.json package-lock.json README.md .gitignore vitest.config.js test test/**' + usage() { echo "usage: packaging.sh {ignore|pathspec|version|major-minor|patch}" >&2 exit 2 @@ -49,11 +73,14 @@ 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/** +# git pathspec excluding the tracked files that cannot change the shipped +# bytes, e.g. :(exclude)extension/package.json :(exclude)extension/test/** +# +# This answers "what moves the version?", NOT "what goes in the XPI?" — see +# NOT_VERSION_RELEVANT for why those differ. cmd_ignore answers the other one. # Same `set -f` requirement as above. cmd_pathspec() { - for entry in $NOT_PACKAGED_TRACKED; do + for entry in $NOT_VERSION_RELEVANT; do printf ':(exclude)extension/%s ' "$entry" done echo diff --git a/extension/test/version.spec.js b/extension/test/version.spec.js index 4e69a69..3b75088 100644 --- a/extension/test/version.spec.js +++ b/extension/test/version.spec.js @@ -44,9 +44,39 @@ describe('packaging.sh — the single definition of what ships', () => { // 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) + + const ignore = packaging('ignore') + expect(ignore).toContain('test/**') + expect(ignore).toContain('scripts/**') + expect(ignore.some((e) => e.includes('.spec.js'))).toBe(false) + }) + + it('lets packaging.sh move the version, though it never ships in the XPI', () => { + // The two lists answer different questions and this is the one place they + // disagree. scripts/ is ignored by web-ext — it is repo tooling, not addon + // code — but packaging.sh DECIDES the version string, and build.yml stamps + // that string into the manifest.json that does ship. So changing how the + // version is computed changes the shipped bytes. + // + // Excluding it from the pathspec was invisible while every push rebuilt the + // web image. Milestone 313 step 4 made that rebuild conditional on the + // derived revision moving, and the omission turned into a silent failure: + // a new version means sign-extension misses its ext- cache and + // signs, while build-web sees an unmoved revision, reuses the published + // image and ships the OLD XPI. An orphaned signature, and an instance + // serving code the registry calls current. + const pathspec = packaging('pathspec') + expect( + pathspec.some((e) => e.startsWith(':(exclude)extension/scripts')), + 'the pathspec excludes scripts/, so a change to how the version is ' + + 'derived would not move the version it derives', + ).toBe(false) + + // ...and it is still kept out of the package itself. Both must hold: the + // tempting "fix" for either half is to make the two lists one again. + expect(packaging('ignore')).toContain('scripts') }) it('keeps its own scripts and specs out of the XPI', () => { diff --git a/tests/test_artifact_paths.py b/tests/test_artifact_paths.py index c226f5b..8063517 100644 --- a/tests/test_artifact_paths.py +++ b/tests/test_artifact_paths.py @@ -110,6 +110,36 @@ def test_the_web_image_versions_on_an_extension_change(): ) +def test_the_web_image_versions_on_a_version_derivation_change(): + """packaging.sh ships in no image, yet it belongs in the sets that bundle + the XPI — because it decides the version string build.yml stamps into the + packaged manifest.json. Changing the derivation changes the shipped bytes. + + Left out, milestone 313 step 4 turns it silent: the new version misses the + ext- cache and gets signed, while web's revision has not moved, so + the reuse path republishes the old image and the fresh signature is + orphaned. Guarded for web and the extension both, since web bundles what + the extension produces. + """ + for artifact in ("extension", "web"): + inc = includes(artifact) + excluded = [ + p[len(":(exclude)"):] for p in declared_paths(artifact) + if p.startswith(":(exclude)") + ] + path = "extension/scripts/packaging.sh" + assert any(covered_by(path, i) for i in inc), ( + f"{path} is not in the {artifact} path set" + ) + assert not any( + covered_by(path, e.rstrip("*").rstrip("/")) for e in excluded + ), ( + f"{path} is excluded from the {artifact} path set, so a change to " + f"how the version is derived would not move the version — and " + f"step 4 would reuse the image that carries the old one" + ) + + @pytest.mark.parametrize( "artifact, path", [