From a3071a754907db6109ff58336c088c0738fb5020 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 28 Aug 2026 22:01:58 -0400 Subject: [PATCH] fix(ci): the file that decides FC_VERSION now moves it (#3202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scripts/artifacts.sh` decides both values the web image carries — the `fc.revision` label the reuse check compares and the `FC_VERSION` baked into the image — and was in no artifact's path set. So a change to `cmd_version` alone left every revision untouched, the reuse check hit, the build was skipped, and the published image went on reporting the OLD version format, indefinitely, until some unrelated commit forced a rebuild. Nothing goes red; the footer just shows a well-formed string of the wrong shape. Milestone 318 step 5 is the worked instance: b3989d0 -> rev=fb2c4d5b80be ver=2026.8.28.1249 5771fd5 -> rev=fb2c4d5b80be ver=2026.08.28.1249 bce894b -> rev=bce894ba2499 ver=2026.08.28.2208 Same revision across the zero-pad commit, so web's build was skipped. It cost nothing only by timing: FC_VERSION did not exist until step 6 landed one commit later. Web only, and that is the interesting part. Every artifact stamps a revision, but only web also stamps a version. A revision-only artifact needs no entry here, because changing how a revision is COMPUTED changes the derived value, which then disagrees with the label on the published image and forces a rebuild — the mechanism self-corrects, since it compares against a string stamped into a real artifact. Nothing compares a version to anything. That asymmetry is why this was invisible and is now written down in both files. Named as a file rather than `scripts`: release_notes.py sits beside it and only reads derived values, so it decides nothing and must not re-version web. This is #3156 one level up — packaging.sh excluded from the version it derives — so the guard is generalised rather than duplicated: one DERIVERS table naming each deriver and the artifacts whose identity it decides. The too-wide test gains a note saying where the line is, since "copied into no image" no longer settles it on its own. --- scripts/artifacts.sh | 34 ++++++++++++++++++++- tests/test_artifact_paths.py | 57 ++++++++++++++++++++++++++++-------- 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/scripts/artifacts.sh b/scripts/artifacts.sh index 234b09d..a7cfcdf 100755 --- a/scripts/artifacts.sh +++ b/scripts/artifacts.sh @@ -3,6 +3,11 @@ # version derived from it. Milestone 313; generalises the shape # extension/scripts/packaging.sh established for the extension alone. # +# "Built from" is deliberately wider than "copied into". A file that DECIDES an +# artifact's identity is part of what that artifact is built from even though it +# never reaches the image — see DERIVER below, and #3156 for the same finding +# about packaging.sh. +# # Four artifacts, four independent versions. An artifact whose shipped files # did not change keeps its version and does not rebuild — that is the whole # point, and it is why each path set must match its Dockerfile rather than @@ -56,6 +61,33 @@ ML_PATHS='Dockerfile.ml requirements-ml.txt requirements.txt backend alembic ale # it: this is deliberately NOT `agent/`. AGENT_PATHS='agent/Dockerfile agent/requirements.txt agent/fc_agent' +# This file. It is copied into no image and it is still part of what the web +# image is built from, because it DECIDES the FC_VERSION baked into that image +# (#3202). Same finding as #3156 about packaging.sh, one level up. +# +# Why web and nothing else. Every artifact stamps `fc.revision`, but only web +# also stamps a version (build.yml line ~488 feeds `version web` to the +# FC_VERSION build arg; ml and agent ask for `revision` alone, and the +# extension takes its version from packaging.sh). For a revision-only artifact +# this file needs no entry: any change to how the revision is COMPUTED changes +# the derived value, which then disagrees with the label on the published image +# and forces a rebuild. That mechanism is self-correcting because it compares +# against a string stamped into a real artifact. +# +# The version is compared against nothing, so it has no such backstop. Before +# this entry, a change to cmd_version alone left every artifact's revision +# untouched, the reuse check hit, the build was skipped, and the published +# image went on reporting the OLD version format — silently, until some +# unrelated commit happened to force a rebuild. Milestone 318 step 5 is the +# worked instance: b3989d0 and 5771fd5 share revision fb2c4d5b80be while the +# version moved 2026.8.28.1249 -> 2026.08.28.1249. It cost nothing only because +# FC_VERSION did not exist until one commit later. +# +# Named as a file, not as `scripts`: release_notes.py lives beside it and only +# READS derived values, so it decides nothing and must not re-version anything. +# A future script that derives an identity belongs here explicitly. +DERIVER='scripts/artifacts.sh' + usage() { echo "usage: artifacts.sh {paths|revision|version} {web|ml|agent|extension}" >&2 @@ -71,7 +103,7 @@ ext_paths() { cmd_paths() { case "$1" in - web) echo "$WEB_PATHS $(ext_paths)" ;; + web) echo "$WEB_PATHS $DERIVER $(ext_paths)" ;; ml) echo "$ML_PATHS" ;; agent) echo "$AGENT_PATHS" ;; extension) ext_paths ;; diff --git a/tests/test_artifact_paths.py b/tests/test_artifact_paths.py index 8063517..888d4c3 100644 --- a/tests/test_artifact_paths.py +++ b/tests/test_artifact_paths.py @@ -13,6 +13,10 @@ being right, and both ways of being wrong are silent: Nothing else notices either. The version still derives, CI still goes green, and the mismatch only surfaces as "I pinned that build and got the wrong bytes". So the Dockerfiles are read here and compared against the declaration. + +The COPY list is not the whole answer, though. A file that DECIDES what an +artifact reports belongs in its set even though it is copied into nothing — +see DERIVERS below, where the same finding is recorded twice (#3156, #3202). """ from __future__ import annotations @@ -110,26 +114,48 @@ 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. +# A file that DECIDES an artifact's identity is part of what that artifact is +# built from, even though it is copied into no image. Both entries here are the +# same finding twice — #3156 for packaging.sh, #3202 for artifacts.sh — and +# both were latent for the same reason: the version has no backstop. +# +# The revision does. Change how a REVISION is computed and the derived value +# stops matching the label on the published image, which forces a rebuild; the +# mechanism self-corrects because it compares against a string stamped into a +# real artifact. Nothing compares a version to anything, so a version-only +# derivation change is invisible unless the deriver is in the set. +DERIVERS = [ + # packaging.sh decides the version build.yml stamps into the packaged + # manifest.json, so changing it changes the shipped bytes. Left out, + # milestone 313 step 4 turns 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 too, since web bundles what the extension makes. + ("extension/scripts/packaging.sh", ("extension", "web")), + # artifacts.sh decides the FC_VERSION baked into the web image (#3202). + # Web only, and deliberately: ml and agent ask this script for `revision` + # alone, so they are covered by the self-correcting path above, and the + # extension takes its version from packaging.sh. Milestone 318 step 5 is + # the worked instance — b3989d0 and 5771fd5 share revision fb2c4d5b80be + # while the version moved 2026.8.28.1249 -> 2026.08.28.1249. It was + # harmless only because FC_VERSION did not exist until one commit later. + ("scripts/artifacts.sh", ("web",)), +] - 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"): + +@pytest.mark.parametrize("path, artifacts", DERIVERS, ids=lambda v: str(v)) +def test_a_version_deriver_is_in_the_set_of_what_it_decides(path, artifacts): + for artifact in artifacts: 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" + f"{path} decides the version {artifact} reports, but is not in the " + f"{artifact} path set. A change to the derivation would leave the " + f"revision untouched, the build skipped, and the published image " + f"reporting the old version — with nothing to disagree with it." ) assert not any( covered_by(path, e.rstrip("*").rstrip("/")) for e in excluded @@ -146,6 +172,11 @@ def test_the_web_image_versions_on_a_version_derivation_change(): # Deliberate exclusions — the too-wide direction. Each of these lives # beside shipped code but never reaches an image, and including it # would re-version the artifact for a change it does not carry. + # + # "Never reaches an image" is the test, not "is not source": DERIVERS + # above are also copied into nothing and DO belong in their sets, + # because they decide what the image reports. The line between the two + # lists is whether the file has a say in the artifact's identity. ("agent", "agent/README.md"), ("agent", "agent/ruff.toml"), ("agent", "agent/docker-compose.yml"),