fix(887): the image build builds the SPA and does not test it #899
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#899
Reference in New Issue
Block a user
fixes #887
What was broken
docker/Dockerfile'sweb-buildstage ran the SPA vitest suite with two hand-written--excludedspec paths. That stage is gitless twice over — it copies only
web/+design-system/, so it holdsno
.git, andnode:22-bookworm-slimships no git binary — while the suite has members needing oneor the other. So the exclusions were a population nothing derives: a hand-maintained list of "the
specs that cannot run here", beside a suite that grows.
#883 added a third such spec without updating the list. Because
Build & push image (amd64)carriesif: github.event_name != 'pull_request', the resulting red is structurally unreachable on a PR — itappears only on pushes to
mainand on thev*tag path. Every image build has failed since,:latesthas stopped being republished, and a release cut would fail at the image build.What this does
Removes the list rather than extending it. The stage now runs
npm run lint && npm run typecheck && npm run build— it builds the SPA and does not test it. Thesuite runs once, unfiltered, in
docker-build.yml'stestjob on a real checkout, andbuildcarries
needs: [test, migrations, scan].Adding a third
--excludewould have re-armed the same trap for the next guard, which is what theissue asked not to do.
The guard, and why it looks the way it does
scripts/tests/test_image_build_delegates_the_spa_suite.pyholds three parts — the third existsbecause the first two together still certify a publish on which the suite never ran:
needs:the jobholding the pinned gating step, in that workflow.
run:body,if:andworking-directoryare pinned; it and its jobcarry no
continue-on-errorin any spelling and no job-levelif:; the publish step keeps its owndocs_onlygate; andweb/package.json's script map andweb/vite.config.ts'stest:block arepinned, because a pinned
RUN npm …executes whatever those say.The mechanism was replaced once, and that is the part worth reading. Three earlier versions
PARSED shell text to decide "does this run the suite, and can its failure be swallowed?". That
predicate was wrong nine times across three review rounds — executed heredocs treated as data,
#truncating a command mid-word, compound punctuation welding commands,
npm t/./node_modules/.bin/vitest/
timeout/su -cunrecognised,true || npm testcounted as a run it never performs — and twicea clause added to remove a false red opened a false green. It was withdrawn. Nothing decides what
a command means any more; the risky command lines are compared as TEXT against a pin, and no spelling
has to be recognised in order to be rejected.
The taxonomy that came out of it is in
docs/guard-inventory.md: a population decides what isCHECKED, so a hand-written one goes silently short; a pin decides what is EXPECTED, so a stale one
goes loudly red; a selector decides which members to pin, and going short there is silent — the
worst of the three, and the one that produced this PR's last blocker.
Verification
development battery deliberately: it is not in the repo and nothing re-derives it.
scripts/tests/mutation_manifest.py, harness-executed everysuite — that is the standing proof; the other 63 were witnessed during development and are not.
mutant-verified; findings that were overclaiming prose are corrected in place. Every reviewer
independently confirmed the CI path itself: no route publishes an image on which the suite has not
run.
v*tag,ci-detect-docs-only.shemitsdocs_only=falseandci-detect-already-validated.shemitsskip=false, sotestruns the fullsuite before
build. The release path is strictly better off: it no longer fails at the imagebuild, and the suite still gates it.
scripts/tests(1411 passed / 2 skipped),npm run lint,npm run typecheck,npm test -- --run(121 files / 1319 tests), ruff check + format, catalog--check.workflow_dispatchrun ofdocker-build.ymlon this branch, wherebuildruns (gated only on
!= 'pull_request') but publishes nothing (push:is gated onmain/v*).Docs
New decision record
ci.image-build-delegates-the-spa-suite(catalog regenerated), plus thestale-passage sweep:
docs/testing.md'sweb/row,docs/guard-inventory.md's note on therealgitsplit, and the module comment in
web/vite-plugins/trackedSourceFiles.realgit.test.tsall describedthe two
--excludes as current.The record enumerates what removing the in-image run gives up rather than waving it through — the one
real loss is running the suite under
node:22-bookworm-slimspecifically, which is small because theartifact the image ships is
vite build's output and that still runs there.One correction worth flagging, since it appeared in five places including a mutation
expectstring: the original claim "the build context is
web/+design-system/, so there is no.git" isfalse. The context is the repository root (
context: .) and.dockerignoredoes not exclude.git.The true statement is about the STAGE. The conclusion survives — bookworm-slim has no git binary
either — but a reader who checked would have found
.gitin the context and concluded the note wasstale.
Round 4, and the third cold review found the same mechanism failing again, so it is removed rather than patched a tenth time. WHAT KEPT BREAKING. Three versions of this guard asked "does this command RUN the suite, and can its failure be swallowed?" of arbitrary shell text. That predicate was wrong NINE times across three review rounds, and twice a clause added to remove a FALSE RED opened a FALSE GREEN on the guard's headline assertion: * heredoc bodies were skipped as data, but BuildKit EXECUTES `RUN <<EOF` — and the opener regex also fired inside quotes (`echo "tags<<__EOT__"`), which blinded the whole-file scan over the last 303 lines of docker-build.yml. Wrong in both directions at once, and measurably live on this tree. * `shlex.shlex` does not clear `commenters` the way `shlex.split` does, so `#` truncated a command mid-word — including the live `${#reports[@]}` idiom — and made this file's own stated residual false. * compound punctuation (`);`) welded two commands into one segment. * `npm t`, `./node_modules/.bin/vitest`, `pnpm vitest`, `yarn vitest`, `node …/vitest.mjs`, `timeout …`, `su -c …`, `if npm test; then` — all invisible. * `true || npm test` counted as the gating run while never executing it. * `continue-on-error: ${{ … }}` passed a check written against two literals — a presence test that cannot see polarity, fail-OPEN in the one direction that matters. WHAT REPLACES IT. Nothing in the file decides what a command means any more. The commands that may run in the two risky places are PINNED as text: the `RUN` lines of every SPA-carrying Dockerfile stage, and the gating step's `run:` body and `if:`. A suite run re-added in ANY spelling is simply not equal to its pin — the pin does not have to recognise a spelling in order to reject it. A pin cannot produce a false green, only a false red, and a false red is a human reading a diff they should have read anyway. The population/pin split is the load-bearing distinction, and it is now stated in the inventory: a POPULATION decides what is CHECKED, so a hand-written one goes silently short; a PIN decides what is EXPECTED, so a stale one goes loudly red. Only the second is safe to write by hand. Populations stay derived from the git index. Two premises that were prose are now assertions: the publish step keeps its own `docs_only` gate (without it, a docs-only push skips the suite and publishes anyway), and no step other than the pinned one mentions the suite — a SUBSTRING sweep, deliberately not a predicate, whose failure mode is a false red asking someone to look. 41 mutants, 0 missed, including all nine spellings above and the three from the previous round. Exactly ONE is declared in `mutation_manifest.py` and re-executed every suite; the other 40 were witnessed during development and are NOT standing — stated in the row rather than left to be assumed. Also fixed: the truncated sentence the round-2 rewrite left in the Dockerfile comment, and the `web/src/api/*.guard.test.ts` glob, which over-claimed — it matches three files and only two of them need git. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkFA pin rejects any command it does not equal, so the remaining attack is to change what the pinned text MEANS. Four such vectors, found by attacking the new mechanism rather than reading it; the first was MEASURED escaping and the rest are the same class: * A NEW stage taking the SPA source across with `COPY --from=web-build /source/web` and running the suite there. `copies_spa_source` excluded every `--from=` copy on the grounds that a stage copy is not a context copy — true, but it can still carry the SOURCE TREE from a stage that has it. The receiving stage was therefore unpinned and unchecked, which is exactly the false-NEGATIVE direction this file's own residual warns about. A stage copy now counts when its SOURCE has a whole `web` path segment, which keeps the built-artifact copy this repo actually makes (`/source/ErsatzTV/wwwroot/app/.`) correctly out. * `working-directory:` moved off `web` — `npm test` somewhere else runs a different package, or none, with the pinned command text unchanged. Now pinned. * `defaults.run.shell` changed from `bash` to `sh`. `bash` here is `bash -e`, which is what makes a failing command fail the step; changing it changes whether a red suite blocks the image without touching the step at all. Now pinned. * A `SHELL` instruction in a pinned stage, which redefines what every later `RUN` executes. Refused outright rather than modelled — there is none in this repo, so the honest move is to reject the construct, not to reason about a replacement interpreter. Battery 41 -> 45, 0 missed; the stage-copy mutant reddens three assertions. The residual list gains the two cases that remain in this class and are NOT covered: a stage copy that RENAMES the tree on the way in (no `web` segment in its source), and an `ENV` altering `PATH` so a pinned `RUN` resolves a different `npm`. Naming them is the point — the previous rounds' residual lists read as exhaustive while omitting the largest holes. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkFA fourth cold review attacked the pin itself. BLOCKER empty, the mechanism upheld, and every finding was prose claiming more than I had measured — plus one one-token gap that was live. THE ABSOLUTES, refuted by execution and now corrected in all three places they appeared (guard docstring, `docs/guard-inventory.md`, the decision record's `rule:`): * "A pin cannot produce a false green." True only in the trivial reading. A pin is immune to a different SPELLING of the command — the entire class that defeated the parser nine times — and is NOT immune to the same text MEANING something else. Two mutants re-armed ersatztv#887 through `web/package.json` alone: `RUN npm run build` executes whatever that file says, so `"build": "vitest run && …"` puts the suite back into the gitless stage with every pin still matching, and `"prepare"` does it via `npm ci`. Now pinned: exactly one script may mention vitest, and its body is fixed. * Residual (1), "a stage that does not carry the SPA source is unpinned — correct, since without `web/` there is no suite there". False. The boundary is what `copies_spa_source` RECOGNISES, which is narrower than "has the suite available". Restated, with the case still outside it named: a stage copy that RENAMES the tree. * The substring sweep's "never a false green". Its reported failures are false reds; what it fails to REPORT is not. `SUITE_MENTIONS` is a hand-written SELECTOR — a third category beside population and pin, and the worst-behaved, because a population going short is caught by an equality and a stale pin reddens loudly, while a selector going short is silent. It was short by exactly one entry: `npm t`, npm's own alias, which this guard already names among the spellings that defeated the parser. A stage running `npm t -- --run` escaped it. Fixed, and the category is now named. ALSO CLOSED: `run: |` -> `run: >` folded the two-line body into one command whose whitespace-normalised text was byte-identical to the pin, so the marker script swallowed the suite as its arguments — the body is now compared LINE BY LINE, since a newline separates two commands. A SECOND step named `Test SPA` inherited the exemption both the pin lookup and the sweep key on; exactly one is now required. And `COPY web*/` — a glob that matches `web/` — was read as not carrying the source, leaving the receiving stage unpinned. Battery 45 -> 51, 0 missed. The remaining meaning-change route, an `ENV` rewriting `PATH` so a pinned `RUN` resolves a different `npm`, is not modelled and is recorded as a residual rather than implied away. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkFRound 6 found three more false greens and named the class they share, which is worth more than any of the three fixes: * `web/vitest.config.ts` OUTRANKS the pinned `vite.config.ts` — closed in the previous commit, found by probing vitest rather than reading about it. * A DECOY first `test: {` block. The comparison took `text.index("test: {")`, so a copy of the pin placed above `defineConfig` satisfied it while the real block was narrowed. Exactly one is now required — the same assertion this file already made about the gating step's NAME, for the same reason, not carried across. * A `needs:` edge matched by bare job id. `needs:` resolves within its own workflow, so a SECOND workflow publishing this Dockerfile while needing its own unrelated job called `test` satisfied it. Now bound to `GATING_WORKFLOW`. (The reviewer downgraded this to MEDIUM on measuring that `test_remote_state_inventory.py` forces a human to classify any new workflow — so the hole is "the guard is blind", not "silent". The forced review asks about remote state, not about whether the image is gated, so the one-line fix stands.) * A vite PLUGIN can shell out to the suite from `buildStart()`. The plugin ARRAY is pinned; the plugin BODIES are a stated residual, mitigated because `trackedSourceFilesPlugin` is deliberately lazy — a fact its own comment now marks as LOAD-BEARING for the image build rather than leaving as an optimisation note. THE CLASS: **a pin assumes it is pinning the artifact that still decides.** Every route found so far is authority moving where the pin is not looking — to another FILE, another OCCURRENCE in the same file, another WORKFLOW, or a HOOK the pinned command invokes. That question is now written down for the next person adding a pin, because a list of four instances is not what generalises. Prose, all refuted by execution: the residual naming the uncovered COPY shapes was wrong a THIRD time at the same site (`/source/web /elsewhere` IS recognised — only the destination is renamed — and the file's own test 700 lines below said so); "only an `ENV` is unmodelled" was an absolute and is now a list; "Reach: N mutants, 0 missed" is restated as a DEVELOPMENT BATTERY, since it is not in the repo, nothing re-derives it, and an independent battery found misses against an earlier head; and `PUBLISH_ACTION` was claimed covered by anti-vacuity, which proves the selector is non-empty and cannot prove it complete. Battery 61 -> 64, 0 missed. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkFHunting a fifth instance of "the pin assumes it is pinning the thing that still decides" turned up two candidates that look like routes and are not, both measured rather than argued: * `setupFiles` is pinned by NAME while its CONTENT is not, which reads like the package.json hole one level down. It is fail-NOISY: `process.exit(0)` at the top of `src/setupTests.ts` makes vitest report `121 failed (121)`, not a green. * `tsconfig*.json` shapes what `tsc -b` compiles, not what vitest collects. Recorded because a reader who spots either will otherwise spend the same probe to reach the same answer — and because the honest residual beside them is the one that IS open: a dependency's own install script, reached through `npm ci` and `web/package-lock.json`. That is a supply-chain concern wider than this guard, and it is named rather than claimed covered. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkFRound 7. BLOCKER empty; three real holes, two of them introduced by round six's own fixes, which is this repo's recorded `each-fix-round-introduces-an-adjacent-defect` happening inside a single commit. **`web/vite.config.js` outranks the pinned `.ts` for `vite build`.** Read from the pinned tarball rather than remembered: vite 8.1.3's `DEFAULT_CONFIG_FILES` is `["vite.config.js", "vite.config.mjs", "vite.config.ts", …]`. So a `vite.config.js` whose plugin shells out to the suite runs it in the gitless stage — measured with all 1411 tests green. The governing rule was already written down one family over ("pinning one config is worthless while a second can outrank it") and had been applied to `vitest.config.*` and not to this one. The refusal now covers both families. **The decoy fix counted one SPELLING of the marker.** `text.count("test: {")` is defeated by writing the real block `test: {` with two spaces beside a decoy that matches exactly: count is 1, the comparison takes the decoy, and the live block filters out precisely the specs #883 broke on. Now `re.finditer(r"\btest:\s*\{")`. **The plugin pin added in that same commit shipped the identical decoy hole** it was written next door to fix — a raw `text.count(PINNED_VITE_PLUGINS) == 1` with a decoy above `defineConfig`. Both `vite.config.ts` pins now share ONE bracket walk and ONE whitespace-tolerant uniqueness rule, so they cannot drift apart again. PROSE, and this one is a false completion claim in my own previous commit message: I said the `PUBLISH_ACTION`/anti-vacuity sentence and the singular "only an `ENV`" residual were corrected. They were — in the record and the inventory row, and NOT in the guard docstring, which is the artifact a code reader hits first. Both are now fixed there too, the route COUNT is removed from the docstring and the record and kept in ONE place, and the residual that stated its own false version before retracting it now states the boundary once. Also: the `--from=` branch never reached the JSON exec-form parser, so `COPY --from=web-build ["/source/web", "/dest"]` left the receiving stage unpinned; the revalidate arm of the gating `if:` is now described as a DEPENDENCY on `ci-detect-already-validated.sh` (graded `MUTATION: NONE`) rather than as something asserted here, since only the `docs_only` arm is; and the plugin-bodies residual now says there are TWO plugins, `react()`'s being third-party and unmitigated. Battery 64 -> 68, 0 missed. One of those four exists because the battery itself briefly reported NOTHING and exited 0 after a bad splice deleted its `main()` — it now carries an anti-vacuity assert on its own mutant count. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkFweb/vite.config.tsis pinned WHOLE 58def5faa1Round 8. Three consecutive rounds had each closed one SPELLING of the same marker match, which is `every-blocker-was-one-mechanism-so-delete-it` at the count where it says withdraw. Round 6 pinned a block; round 7 fixed `test: {` (two spaces); round 8 defeated the repaired matcher four more ways — `test : {`, `"test": {`, and the same two for `plugins:` — plus two that never touched the marker at all: plugins: [react(), trackedSourceFilesPlugin()].concat([evil]) test: { …pinned… }, ...moreTest `defineConfig` is the identity function in BOTH vite and vitest (read from the installed tree), so a spread AFTER the pinned span simply replaces what the pin matched. No respelling of the marker could ever have caught those: the defect was partial matching, not the pattern. So the file is pinned whole. 48 lines, nothing generates it, no marker to respell and nothing after the span. One assertion replaces a bracket walk, a block extractor and two uniqueness assertions — and catches all seven measured routes. Stated cost, which is the same one every other pin here carries: any edit to that file reddens, a comment included. This also retires a claim I made in a commit message AND in the inventory row: that the two pins "share one bracket walk and cannot drift apart again". It was false when written — the block extractor had its own inline copy and never called the shared helper. Verified by spying on the call: the `test:` pin made zero calls to it. Also closed: the second exec-form branch in `copies_spa_source` was DEAD code (measured by poisoning it with a `raise` — every test stayed green), since the round-7 reorder rewrites `remainder` before it; the exec-form `--from` case that reorder was written for had no STANDING test, only the development battery, and now has three; and `OUTRANKING_CONFIG_PREFIXES` matched nested paths, so a `web/e2e/vite.config.ts` — and `web/e2e/` is a real directory — would have reddened with a message asserting it outranks the pinned config, which is false. Direct children of `web/` only. Battery 68 -> 73, 0 missed. Refs: #887 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019T79beF1Ufid3dXju4yqkFReview-verdict: MERGEABLE @
9da0020Nine independent cold-review rounds (eight BLOCKED, round 9 MERGEABLE with BLOCKER and HIGH empty). Every measured false green closed and re-verified; 73-mutant development battery, 0 missed; one declared clause mutation harness-executed per suite. Local gate green: scripts/tests 1410 passed/2 skipped, web lint+typecheck+1319 tests, ruff, catalog --check.