diff --git a/docker/Dockerfile b/docker/Dockerfile index dd8eac5f3..abe2d1a78 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -9,37 +9,35 @@ WORKDIR /source COPY design-system/. ./design-system/ COPY web/. ./web/ WORKDIR /source/web -# The SPA suite runs here except for two files (ersatztv#819), excluded for OVERLAPPING reasons — -# one needs the git binary, the other needs the binary AND a checkout. Reading them as two separate -# reasons is what broke this stage once already: -# * `web/src/api/pageSizeCallSites.guard.test.ts` needs a git CHECKOUT — and, through it, the -# binary. It derives its file population from `git ls-files` rather than a directory walk, and -# refuses rather than falling back. This stage's context is `web/` + `design-system/` only, so -# there is no `.git`. -# * `web/vite-plugins/trackedSourceFiles.realgit.test.ts` needs the BINARY but no checkout: it -# builds its own temp repository to prove the derivation by executing it. -# `node:22-bookworm-slim` ships no git (`command -v git` -> not found), so it dies with -# `spawnSync git ENOENT`. -# So this is NOT checkout-versus-binary, and adding a `COPY .git` would not let either run here — -# the binary would still be missing. Excluding only the first is not enough either, and a replica -# that merely deletes `.git` cannot show that: verify any change here with the git binary off -# `PATH`, not just with the directory absent. -# Everything else — all but those two files — runs fine gitless and is kept, rather than dropping -# the whole suite for one file as an earlier fix here did. -# The excluded pair is not skipped overall: `docker-build.yml`'s `Build & test (.NET)` job runs the -# whole suite on a real checkout, and `build` (the job that invokes this Dockerfile) carries -# `needs: [test, migrations, scan]`. State that chain precisely, because the `needs:` edge is not -# all of it: `Test SPA` is also gated on `docs_only` and on the #420 revalidate skip, and `build` is -# not gated on `revalidate`. On a push whose tree is byte-identical to an already-green head the -# suite is skipped and the image still builds — carried there by #420's byte-identical-tree -# argument. The `docs_only` arm cannot ship an image at all (`Build and push` is gated on it too), -# and `ci-detect-docs-only.sh` classifies by PATH SHAPE (`docs/` or `*.md`), not by directory, so a -# `web/*.md` would count as docs — there are none today, but do not restate it as "any `web/**`". -RUN npm run lint && npm run typecheck && \ - npm test -- --run \ - --exclude 'src/api/pageSizeCallSites.guard.test.ts' \ - --exclude 'vite-plugins/trackedSourceFiles.realgit.test.ts' && \ - npm run build +# THE VITEST SUITE IS NOT RUN IN THIS STAGE, AND MUST NOT BE RE-ADDED (ersatztv#887). +# This stage is gitless in both senses at once: the build context is `web/` + `design-system/`, so +# there is no `.git`, and `node:22-bookworm-slim` ships no git binary (`command -v git` -> not +# found). Members of the SPA suite need one or the other — `web/src/api/*.guard.test.ts` derive +# their file population from `git ls-files` and refuse to fall back to a directory walk +# (`testing.guard-derives-population-from-source`), and `web/vite-plugins/*.realgit.test.ts` builds +# its own temp repository. So a `COPY .git` alone would not make the suite runnable here; the +# binary would still be missing. +# +# Running the suite here anyway therefore costs a hand-maintained list of the members that cannot +# run — and that list is a population nothing derives. It went stale the first time a guard was +# added without updating it, and because `Build & push image (amd64)` is `if: github.event_name != +# 'pull_request'`, the resulting red is unreachable on a PR and lands on `main` and on the `v*` tag +# path: every image build failed and `:latest` stopped being republished. Excluding one more file +# re-arms that; the list is removed instead. +# +# WHAT STILL VALIDATES THE SUITE. `docker-build.yml`'s `test` job runs it UNFILTERED on a real +# checkout, and `build` — the job that invokes this Dockerfile — carries `needs: [test, migrations, +# scan]`, so an image cannot be published past a red suite. Two skips exist inside `test` and +# neither leaves an image unvalidated: `docs_only` also gates `Build and push`, so that arm ships +# nothing; the #420 revalidate skip fires only on a tree byte-identical to a head that already +# carried a green combined status, i.e. this exact source already passed. Held by +# `scripts/tests/test_image_build_delegates_the_spa_suite.py`, which derives both halves rather +# than restating them. +# +# `lint` and `typecheck` stay. They are gitless-safe with no member that is not, so they carry no +# list and no trap; whether the image build should run them at all is a separate question this +# does not answer. +RUN npm run lint && npm run typecheck && npm run build FROM --platform=linux/amd64 192.168.1.95:3000/timothy/ersatztv-ffmpeg:8.1.2 AS runtime-base COPY --from=dotnet-runtime /usr/share/dotnet /usr/share/dotnet diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 320f9a6bd..3b8bb9f3e 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -54,6 +54,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class. Gitea injects `GITEA_TOKEN` into EVERY job and it USED to be write-capable everywhere, so any ref-resolved workflow could forge `review-verdict/h10`; since #748 (2026-08-27) all six workflows here declare `permissions:` — five `code: read`, and `review-verdict.yml`'s gate job `code: write`, deliberately, because it IS the gate (the five rest on the TOP-LEVEL form, which is INFERRED rather than probed — the gate job's JOB-LEVEL form is the one measured; see `ci.actions-credential-scoping`). That narrows the INJECTED token and only that: deliberately NO "only X can post a status" summary is stated here, because any such summary generalizes from the injected token, which `permissions:` scopes, to every credential a job can reach, which it does not touch — `RENOVATE_TOKEN` is in the same secret store and is referenced by `renovate.yml`, one of the five holding `code: read`. `ci-image.yml` was the last holdout — its unfiltered `push:` made it the worst-placed of the six — and #744 closed that trigger route (`ci.toolchain-image-publish-is-a-dispatch`) while #748 scoped its token. So the route is NARROWED, not closed, and the SHAPE is open — a newly added workflow declaring write, or omitting a declaration and inheriting the owner default, is a route again, which is why `test_pr_changed_files.py` asserts every tracked workflow declares a block. And a collaborator's own API token still can, since branch protection binds the context and not its issuer. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (an ALLOW-LISTED user credential's forgery carries a matching `creator` and is inherited as a verdict; an Actions job's carries `creator: null` and is re-derived, as since #742 is every account outside `H10_REVIEWERS` — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The owner-level Restricted default (server-management#714) remains a separate OPEN lever — its effect on a declared `code: write` is unmeasured, and the version half of that condition has been satisfied since the 1.25.4 -> 1.27.1 upgrade on 2026-08-05, so it is not something to wait on a Gitea release for. A collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | | `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) | | `ci.grep-q-pipefail-inversion` | In any script running under `set -o pipefail`, a security or classification predicate of the form `producer \| grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf '%s\n' "$data" \| grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep's status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`. | 2026-07-29 | [link](records/ci/grep-q-pipefail-inversion.md) | +| `ci.image-build-delegates-the-spa-suite` | `docker/Dockerfile` runs no vitest suite. Its web-build stage lints, typechecks and BUILDS the SPA; the suite runs once, unfiltered, in `docker-build.yml`'s `test` job on a real checkout, and `build` carries `needs: [test, migrations, scan]` so no image is published past a red suite. Do NOT restore a filtered in-image run by naming the specs that cannot run there: that list is a population nothing derives, and the red it produces when it goes stale is unreachable on a PR — `Build & push image (amd64)` is `if: github.event_name != 'pull_request'` — so it lands on `main` and on the `v*` tag path, where it fails the release cut. Held in both directions by `scripts/tests/test_image_build_delegates_the_spa_suite.py`. | 2026-08-30 | [link](records/ci/image-build-delegates-the-spa-suite.md) | | `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) | | `ci.jq-version-contract` | Every shell gate that shells out to `jq` is authored to the jq 1.6-compatible subset, because the CI runner ships jq 1.6 while every developer Mac ships 1.8.x. `scripts/jq-preflight.sh` (no args) prints the parsed version and asserts a floor of 1.6 in every gate job's log; `scripts/jq-preflight.sh --expect 1.6` additionally pins and fails loudly, but ONLY in the `script-tests` job. `review-verdict.yml` never pins — it writes the branch-protection-required `review-verdict/h10` status, so a hard pin there would turn any jq bump into a repo-wide merge deadlock. | 2026-07-26 | [link](records/ci/jq-version-contract.md) | | `ci.killed-job-triage` | Never trust a job's `conclusion` field alone — read the log tail and require an `❌ Failure - Main …` marker before treating a red as a real failure. | 2026-07-21 | [link](records/ci/killed-job-triage.md) | diff --git a/docs/decisions/records/ci/image-build-delegates-the-spa-suite.md b/docs/decisions/records/ci/image-build-delegates-the-spa-suite.md new file mode 100644 index 000000000..ff07195b4 --- /dev/null +++ b/docs/decisions/records/ci/image-build-delegates-the-spa-suite.md @@ -0,0 +1,57 @@ +--- +key: ci.image-build-delegates-the-spa-suite +title: '2026-08-30 — the image build builds the SPA and does not test it (#887)' +status: active +since: '2026-08-30' +supersedes: none +superseded-by: none +rule: '`docker/Dockerfile` runs no vitest suite. Its web-build stage lints, typechecks and BUILDS the SPA; the suite runs once, unfiltered, in `docker-build.yml`''s `test` job on a real checkout, and `build` carries `needs: [test, migrations, scan]` so no image is published past a red suite. Do NOT restore a filtered in-image run by naming the specs that cannot run there: that list is a population nothing derives, and the red it produces when it goes stale is unreachable on a PR — `Build & push image (amd64)` is `if: github.event_name != ''pull_request''` — so it lands on `main` and on the `v*` tag path, where it fails the release cut. Held in both directions by `scripts/tests/test_image_build_delegates_the_spa_suite.py`.' +signals: 'image build fails on main only · every image build fails · latest not republished · release cut fails at the image build · gitless build context · node:22-bookworm-slim ships no git · virtual:etv-tracked-source-files could not read the git index · hand-maintained exclude list · exclusion list is the underived population · suite members needing the git binary vs a checkout · COPY .git would not help · needs edge is the only gate · unfiltered gating run · paths: `docker/Dockerfile`, `.gitea/workflows/docker-build.yml`, `scripts/tests/test_image_build_delegates_the_spa_suite.py`, `web/vite-plugins/trackedSourceFiles.ts` · issues: #887, #883, #819, #806, #420' +mechanics: 'Guard derives three populations — tracked Dockerfiles, tracked workflows, and the vitest script names read out of `web/package.json` — then asserts no Dockerfile runs the suite, every publishing job whose Dockerfile carries the SPA source transitively `needs:` a job that does, and every such run is unfiltered. Publishing jobs come from the `docker/build-push-action` step; the Dockerfile each builds comes from that step''s `file:` input.' +--- + +The web-build stage is gitless twice over: its build context is `web/` + `design-system/`, so there +is no `.git`, and `node:22-bookworm-slim` ships no git binary. Members of the SPA suite need one or +the other — the `web/src/api/*.guard.test.ts` guards derive their file population from `git ls-files` +and refuse to fall back to a directory walk (`testing.guard-derives-population-from-source`), and +`trackedSourceFiles.realgit.test.ts` builds a real temporary repository. Because the two +prerequisites OVERLAP rather than divide, adding a `COPY .git` fixes neither: the binary is still +missing. + +So running the suite there at all costs a list of the members that cannot run, maintained by hand +beside a suite that grows. That list went stale the first time a guard was added without updating +it, and the failure it produced was invisible where it could have been cheap and fatal where it was +not: green on every PR, red on `main` and on the release tag. + +**Why not the alternatives.** Each was considered and rejected on its own evidence, not by analogy: + +* **Add a third `--exclude`.** Re-arms the same trap for the next guard, which is what the issue was + filed to prevent. +* **Derive the exclusions from source.** Buildable, but it requires a predicate for "needs git" over + spec text — two unrelated shapes today (a virtual-module import, a `git` spawn) and no reason to + think that stays two. A string predicate over source is the wrong instrument for a set whose + members are defined by what they DO at runtime. +* **Give the container git** (`apt-get install git` plus a real `.git`). It works, and it costs a + cache-busting `COPY` of VCS metadata on every commit, so the whole lint/typecheck/test/build layer + re-runs for changes that do not touch `web/`. Synthesising a repository in the image instead — + `git init && git add -A` — is worse than it looks: the resulting index is a filesystem walk with + extra steps, which is precisely the population the guards refuse. + +**What the removal gives up, enumerated rather than waved through** +(`process.enumerate-workaround-behaviors-before-deleting`): + +1. *Blocking a publish on a red suite* — retained, by the `needs:` edge. Two skips exist inside + `test` and neither leaves an image unvalidated: `docs_only` also gates `Build and push`, so that + arm ships nothing, and the #420 revalidate skip fires only on a tree byte-identical to a head + that already carried a green combined status. +2. *Running the suite under `node:22-bookworm-slim` specifically* — **lost**, and this is the real + cost. It is small because the suite is jsdom and pure JS, and because the artifact the image + actually ships is the output of `vite build`, which still runs there: a test that passes in the + CI toolchain image and fails in bookworm-slim would be telling us about the test environment, not + about the image. +3. *`docker build` running the suite for a developer locally* — lost; `npm test -- --run` is the + direct route and is what `docs/testing.md` documents. + +`lint` and `typecheck` stay in the stage. They are gitless-safe with no member that is not, so they +carry no list and no trap. Whether the image build should run them at all is a separate question +this does not answer, and leaving them is not an argument that it should. diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 133280191..2fefe5284 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -74,7 +74,9 @@ with have sat in these gaps. Treat anything not named here as unassessed, not as and the pair `web/vite-plugins/trackedSourceFiles.test.ts` + its `.realgit.test.ts` sibling, which together prove the population `pageSizeCallSites.guard.test.ts` derives (split because the `realgit` half - needs the git BINARY, which the Docker web-build stage has not got). Note what that costs: this + needs the git BINARY and its thirteen injected-runner siblings do not, so the prerequisite is + confined to one file — it is NOT the Docker exclusion it was originally written for, which + ersatztv#887 removed along with the whole in-image suite run). Note what that costs: this list is a HAND-WRITTEN mirror of a population nothing derives, so it goes stale silently and CI stays green — #807 added the third entry, and nothing mechanical caught that `pageSizeCallSites.guard.test.ts` had become the only one named. @@ -171,6 +173,7 @@ The audit #806 asked for, recorded whichever way it came out, because "we looked | `test_workflow_job_guards.py` | `.gitea/workflows/*.y*ml` | **yes** — every tracked workflow's jobs must each declare a `CI_JOB_ROLE`, and the guard-declaring ones must match `## Workflow-job guards` both ways | index-derived from the start (ersatztv#786); registered in `test_guard_populations_derive_from_git.py` so the shared proof covers it | | `test_pr_changed_files.py` | `.gitea/workflows/*.y*ml` | **yes** — "no OTHER workflow writes the review-verdict status", and since ersatztv#748 "every tracked workflow declares a `permissions:` block", with NO exemption list (the one `ci-image.yml` briefly needed was deleted when #744 landed, per #835) | converted. Not on #806's list: an untracked `.yaml` dropped in `.gitea/workflows/` reddened two guards while absent from the index — the issue's list of files to assess was a starting point, not the population | | `test_workflow_persist_credentials.py` | `.gitea/workflows/*.yml` + `*.yaml` | **yes** — every `actions/checkout` in every workflow must drop the persisted credential | derived from the index from the start (#835). Shipped with NO exemption list, which is why it waited for #744: the one non-compliant checkout would otherwise have bought a permanent entry that keeps passing after its reason expires | +| `test_image_build_delegates_the_spa_suite.py` | `.gitea/workflows/*.y*ml` + every tracked `Dockerfile*`/`*.Dockerfile`, plus the vitest SCRIPT NAMES read out of `web/package.json` | **yes, in both directions** — no Dockerfile in the population may run the SPA suite, and every image-publishing job whose Dockerfile carries the SPA source must transitively `needs:` a job that runs it unfiltered | ersatztv#887, index-derived from the start. THREE populations rather than one, because a single one leaves a hole the other two close: the Dockerfile set alone cannot see a deleted `needs:` edge, and the job set alone cannot see a suite run added to a gitless stage. The publishing jobs are derived from the `docker/build-push-action` step and the Dockerfile each builds is read from that step's own `file:` input, so `ci-image.yml` — which publishes from `docker/ci/Dockerfile`, a Dockerfile carrying no SPA source — is out of scope by DERIVATION and holds no exemption entry that could outlive its reason. Which npm scripts count as the suite is derived too, from `web/package.json`: `test` is in because `"test": "vitest"` is, and the sibling `test:ui-e2e` is out because it runs Playwright — a hardcoded name would go quiet on a rename, and a `test:`-prefix guess would demand an unfiltered gate on a browser suite. RESIDUALS, stated: a `RUN` that calls a SCRIPT which runs the suite is invisible (the parser reads commands, not what they call — no such script exists today, and the miss is a missed DETECTION, not a false green about the gate, which is derived separately); only `RUN` is read in a Dockerfile, since nothing else executes during a build; and a line whose first non-space character is `#` is inert, which is what lets `docker/Dockerfile` name the rule it is subject to — only a LEADING `#`, so an invocation with a trailing note is still seen | | `web/src/api/pageSizeCallSites.guard.test.ts` | `import.meta.glob` over `web/src/**/*.{ts,tsx,mts,cts}`, INTERSECTED with the git index | **yes** — an unregistered discovered site fails | converted under #819. The population is the glob INTERSECTED with the git index, and the walk/index divergence is asserted in BOTH directions WITHIN a shared scope: a key the index does not carry is dropped, and a tracked, on-disk, in-scope path that never reached the scanner FAILS rather than shrinking the population silently (that direction catches a dotfile, which the glob cannot match, and a disk/index spelling divergence, which `core.ignorecase` and NFD/NFC normalisation make permanent). The SCOPE itself cancels out of that equality — both sides call one `isInScopeSourcePath` — so narrowing it moves both sets together and no assertion comparing the two sides can see it. That is closed separately, by re-deriving the predicate from its two components over the whole tracked index (`the shared scope predicate is EXACTLY its two documented components`). Two mechanisms carry that. FIRST, the scope predicate is proved by a CLOSED-FORM restatement — it may share no helper, at any depth, with the predicate it checks, since anything shared sits on both sides of the comparison and cancels. FOUR earlier attempts were each measured going green while removing real files: a table of example paths (four of `src/`'s eight directories, missed a 23-file narrowing), a decomposition delegating its filename half to `isScannableSourceFileName` (11 files), one still sharing a `basename` helper (15 files, and blind to a planted call site), and one still sharing the tracked-file array every comparison was derived from (8 files, also blind). SECOND — because that last one showed a restatement cannot police the population it reads — the plugin runs a separate `git ls-files --others` query and the guard requires every in-scope walked path to appear in `tracked ∪ others`; narrowing `tracked` cannot suppress that, since it adds nothing to `others`. SIX residuals, NOT a claim of closure, each with its direction MEASURED by planting a real `pageSize` call site rather than reasoned about. FAIL-NOISY (they redden a checkout; they cannot hide a call site): (2) a scope term matching no tracked path today survives until the day it first matches one; (4) the WIRING in `listSourceFiles`, indistinguishable on a clean checkout where the walk set and the index set agree on every in-scope key (they are not literally equal — the index also carries `.css`, which the glob never yields), so no assertion COMPARING THE TWO POPULATIONS can tell which is passed; (5) `ABSENT_FROM_DISK` emptied. BLIND — each hid a planted `pageSize: 100` with the whole suite green: (1) a COORDINATED edit of the scope predicate and BOTH closed-form restatements, which is three sites and not two, since a second restatement guards `expectedTrackedSources` (measured: the two-site edit reddens, the three-site edit does not) — tolerated because a three-site policy edit is review-visible in a way a one-line slip is not; and (3) a misdescription by the plugin's THREE outputs — not only a mispartition of `tracked`/`others`, which preserves the union any consumer compares, but a FATTENED `absentFromDisk`, which subtracts real files from the hole-detection direction. The fattening is now caught wherever it overlaps a file the walk found (a walked path is on disk by construction, so it can never be legitimately absent); restricted to paths the walk cannot see anyway — a dotfile, a case-divergent name — it stays invisible from the consuming side and is answerable only by testing the derivation directly, and the real-git tests that do so catch an UNCONDITIONAL misdescription, not one keyed on a path pattern. Neither blind residual is tolerated for being harmless. A sixth is specific to WATCH mode and is stale in BOTH directions, so it belongs to neither group: the virtual module has no backing file, so the index is read once per dev-server lifetime while the glob refreshes. A file CREATED mid-session reddens the cross-check misleadingly (noisy); a file already UNTRACKED when the watcher started keeps that classification when staged mid-session, so its call sites are never scanned — MEASURED green across both phases while `npx vitest run` on the same tree reports `UNREGISTERED`, i.e. blind, and a window that did not exist before #819 because the population was then the walk. A watch-mode green is therefore not authoritative for this guard; restarting the watcher clears the noisy case and opens the blind one, so the remedy is `npm test -- --run`. Invalidating the module from `configureServer` was implemented, measured and REJECTED: it fixes the created-mid-session red and additionally blinds the create-then-stage sequence, while the already-untracked-then-staged sequence is blind either way, since `git add` fires no watcher event in either design. The index is read by `web/vite-plugins/trackedSourceFiles.ts` in Vite's own Node context and handed to the app project as the virtual module `virtual:etv-tracked-source-files`. That is what reaches the index without admitting `@types/node` to `tsconfig.app.json` — the obstacle that deferred this, since wiring those types in was tried and reverted (it leaked Node's `setTimeout` into the app project and broke three unrelated tests). The plugin throws rather than falling back to the unfiltered walk when git fails, reports zero files, or cannot learn the Vite root. Residual, stated: a file tracked but DELETED in the working tree is subtracted deliberately, since an unstaged deletion is a normal developer state and a guard red on one gets ignored (#806); it is distinguished from the hole cases above by an on-disk existence check, not conflated with them | | `web/src/api/completeAnnotations.guard.test.ts` | TWO derived populations: the `Complete<…>` annotations across `web/src/**/*.{ts,tsx,mts,cts}` INTERSECTED with the git index, and the droppable SCHEMAS parsed out of the generated `src/api/generated/v1.d.ts` | **partial, and the split is stated** — set equality holds for the SCHEMA population (a new optional member fails until dispositioned) and every tracked in-scope path must be supplied by the glob; there is NO closed-form restatement of the scope predicate, so a coordinated edit to it is caught by five named path pins AND a 0.95 population-ratio floor, not by a completeness proof | ersatztv#820. Turns `test_optional_request_members.py`'s `COVERED` disposition — worded "the builder is annotated `Complete`", a claim about ANOTHER LANGUAGE'S source that nothing checked — into a check, and makes `docs/spa-conventions.md` §4b's prohibitions executable as asserted ABSENCES. It found one live defect: `playouts.ts` declared two request types as hand-written mirrors SHADOWING generated schemas of the same name, so their `Complete<>` was checking a local copy rather than the contract. **NARROWED after four BLOCKED review rounds, and the narrowing is the point.** It originally also derived the write WRAPPERS and required the annotation on the wrapper parameter. Every blocker across those rounds came out of that one mechanism — the obligation attached to the wrong population; reachability mistaken for protection (`Complete` is shallow, so a wrapper annotation never reached a nested schema); body discovery keyed on a parameter NAME, then on parameter-versus-local; and finally an `export function` -> `export const` refactor that removed real protection while both the AST scan and its supposedly independent regex cross-check stayed blind TOGETHER, because both keyed on the same token. Five defects from one mechanism is the `process.enumerate-workaround-behaviors-before-deleting` signal to remove it rather than patch a sixth time, and ~250 lines went with it. WHAT IS NOT COVERED, listed because the earlier version of this row stated residuals in the direction that was actually covered: (1) the obligation is per-SCHEMA, not per-SITE and not per-WRAPPER — deleting the `Complete<>` from an API wrapper stays green as long as some production file still names that schema, so the wrapper half of §4b rests on review; (2) it is a TOKEN-PRESENCE check, so a dead `export type X = Complete` that nothing uses discharges the obligation as well as a live builder does — it catches deletion, which is the failure actually observed in #807, not substitution; (3) the PHANTOM direction needs a fresh literal in a contextually typed position and is not checked at all (sites-in-code, #777); (4) the test-file exclusion covers `*.test.*`, `*.spec.*` and the ONE setup file `vite.config.ts` names, whose path is pinned so a rename reddens — a SECOND `setupFiles` entry is loaded by vitest every run and WOULD discharge the obligation; that needs a three-site coordinated edit and is tolerated on the terms the sibling guard states for its own; (5) a MUST-NOT-ANNOTATE violation inside a `*.guard.test.ts` is not seen, since that file class is excluded to keep `completeRequest.guard.test.ts`'s synthetic `Complete<{…}>` fixtures out of the resolver. The disposition VALUES are cross-checked against the authoritative Python table by `scripts/tests/test_complete_annotation_dispositions.py` — without it, flipping one row from `ANNOTATED` to `CREATE` silently retired the requirement, which cold review demonstrated. Nine mutations were witnessed by hand across development, NOT re-executed per suite, so this guard claims no standing `MUTATION` grade — the same footing as `pageSizeCallSites.guard.test.ts`. (Its Python cross-check `test_complete_annotation_dispositions.py` DOES carry a declared harness-executed mutation and is graded accordingly.) A SIXTH residual, shared with its sibling: every population here derives from `trackedSources.tracked`, so a narrowing inside `web/vite-plugins/trackedSourceFiles.ts` cancels out of every comparison — this guard never reads the plugin's separate `others` query, and relies on `pageSizeCallSites.guard.test.ts` and `trackedSourceFiles.test.ts` policing the plugin. A coordinated scope edit is caught by five named path pins AND a 0.95 population-ratio floor whose denominator is computed by a different expression, not by a completeness proof | | `test_ci_release_path_scan_job.py` | `.gitea/workflows/*.y*ml` + `scripts/**` | **no** — a fixture assembling a tmp harness, asserted about behaviour not membership | takes its file LIST from the index anyway, for hermeticity not completeness: `shutil.copytree` copied whatever was on disk, including untracked files and `scripts/__pycache__`, into a tree whose behaviour the probes then measure. Content still comes from the working tree. The copy is not a git repo, so the two files this step RUNS may not use the helper — see the fixture docstring | @@ -249,6 +252,7 @@ recorded as unexamined rather than as cleared. | `scripts/tests/test_guard_inventory.py` | the `script-tests` job, on an unclassified guard or a stale proof ref | GUARD | MUTATION | `test_guard_inventory.py::test_the_inventory_covers_exactly_the_guards_that_exist` | | `scripts/tests/test_guard_populations_derive_from_git.py` | the `script-tests` job, on a guard whose file population admits a file git does not track | GUARD | MUTATION | `test_guard_populations_derive_from_git.py::test_no_derivation_admits_an_untracked_file` | | `scripts/tests/test_hook_fire_log.py` | the `script-tests` job, on a hook that stops reporting that it fired, or whose reporting changes what the harness sees | GUARD | MUTATION | `test_hook_fire_log.py::test_a_hook_that_LOSES_its_instrumentation_is_DETECTED` | +| `scripts/tests/test_image_build_delegates_the_spa_suite.py` | the `script-tests` job, on a Dockerfile that runs the SPA vitest suite, on an image-publishing job that stops being gated on a job that runs it, or on a gating run that is narrowed by a spec filter | GUARD | MUTATION | `test_image_build_delegates_the_spa_suite.py::test_no_DOCKERFILE_runs_the_spa_suite` | | `scripts/tests/test_jq_preflight.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_mcp_smoke.py` | the `script-tests` job, when the MCP smoke checker accepts a server it should refuse — an impostor identity, a reply carrying only an id, a missing expected tool, or an `initialize` pre-answered before the request (the `tools/list` twin is held by two mechanisms jointly and is covered behaviourally, not by the declared clause) | GUARD | MUTATION | `test_mcp_smoke.py::test_MUTATION_a_PRE_ANSWERED_id_is_refused_because_the_request_ids_are_UNGUESSABLE` | | `scripts/tests/test_merge_consent_base_change.py` | the `script-tests` job | PROOF | NONE | — | @@ -375,7 +379,7 @@ clause-provable and the entry is regraded. ## What the numbers say -45 guards, 6 tooling scripts, 23 proof files. **25 guards carry a mutation proof; 5 are +46 guards, 6 tooling scripts, 23 proof files. **26 guards carry a mutation proof; 5 are behaviour-only; 15 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table`, because a hand-maintained summary of a table is a second copy of it — the duplication family this change argues against, and one that has already drifted diff --git a/docs/testing.md b/docs/testing.md index 0d93d1685..dbc04b5d5 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -20,7 +20,7 @@ before adding tests, not just `docs/contributing.md` §8 (which now just points | `ErsatzTV.Scanner.Tests` | Library scanning: scan handlers, folder scanners, NFO readers | Handler tests substitute the folder scanners + `ILibraryRepository` and assert the resulting repository writes (e.g. `ScanLocalLibraryHandlerTests` pins which `LastScan` levels a scan records — ersatztv#264). Fakes/`Testably` back the file-system-facing scanners. ~1,485 tests (approximate on purpose — an exact count goes stale on every PR that adds one). Additionally contains `Core/FFmpeg/TranscodingTests` — `[Explicit]` + `[Combinatorial]`, so it never runs in CI or a plain `dotnet test` (it needs real ffmpeg/hardware) and contributes 0 to that count; run it by name when touching the transcoding pipeline. | | `ErsatzTV.Architecture.Tests` | Layering rules via NetArchTest.eNhancedEdition | Core↛Infra/App/EF; FFmpeg↛all; App↛concrete providers. 5 tests. See `docs/contributing.md` §1. | | `ErsatzTV.FFmpeg.Tests` | FFmpeg command construction | Build a pipeline, assert the exact rendered arg string (`PipelineBuilderBaseTests.cs`). | -| `web/` (vitest) | React SPA unit tests | Run alongside typecheck + build (see below). Collects `src/**`, `web/scripts/**` *and* `web/vite-plugins/**`, but deliberately **excludes** `web/e2e/**` (the Playwright specs — vitest's default `**/*.spec.*` glob would otherwise run them under jsdom). Two files have git prerequisites since ersatztv#819. `web/src/api/pageSizeCallSites.guard.test.ts` needs a git **checkout** AND, through it, the **binary** — it derives its file population from `git ls-files` via `web/vite-plugins/trackedSourceFiles.ts` rather than a directory walk, and refuses rather than falling back. `web/vite-plugins/trackedSourceFiles.realgit.test.ts` needs the **binary** but no checkout: it builds its own temp repository to prove that derivation by executing it. So it is not checkout-versus-binary — supplying a `.git` alone would not let either run. Every other file runs fine with neither. `docker/Dockerfile`'s web-build stage runs the suite with exactly those two `--exclude`d, because its context carries no `.git` and `node:22-bookworm-slim` ships no git — and the two exclusions overlap rather than divide — both files need the binary. | +| `web/` (vitest) | React SPA unit tests | Run alongside typecheck + build (see below). Collects `src/**`, `web/scripts/**` *and* `web/vite-plugins/**`, but deliberately **excludes** `web/e2e/**` (the Playwright specs — vitest's default `**/*.spec.*` glob would otherwise run them under jsdom). Some files have git prerequisites since ersatztv#819, and the set is not fixed — ersatztv#883 added one. `web/src/api/pageSizeCallSites.guard.test.ts` and `web/src/api/completeAnnotations.guard.test.ts` need a git **checkout** AND, through it, the **binary**: they derive their file population from `git ls-files` via `web/vite-plugins/trackedSourceFiles.ts` rather than a directory walk, and refuse rather than falling back. `web/vite-plugins/trackedSourceFiles.realgit.test.ts` needs the **binary** but no checkout — it builds its own temp repository to prove that derivation by executing it. So it is not checkout-versus-binary: supplying a `.git` alone would not let any of them run. **The suite therefore runs only where git is present, and `docker/Dockerfile` is not such a place** — its web-build stage builds the SPA and does not test it (ersatztv#887). Enumerating the git-dependent files as Docker `--exclude`s was tried and REVERSED: that list is a population nothing derives, it went stale the first time a guard was added, and the resulting red is unreachable on a PR — `Build & push image (amd64)` is `if: github.event_name != 'pull_request'` — so it landed on `main` and on the release tag instead. The image is gated on `docker-build.yml`'s `test` job running the whole suite on a real checkout, held by `scripts/tests/test_image_build_delegates_the_spa_suite.py`. | | `web/e2e/` (Playwright) | UI-interactive E2E flows against a **live** instance | Not a unit suite and **not** part of `npm test` — needs a running server, so it runs via `scripts/e2e-ui.sh` (boots its own fresh instance) and in CI as a step of the `functional-e2e` job. Headless Chromium, `serial`, `retries: 0`. Scope rule: assert only what the curl harness structurally cannot. See `docs/e2e-local.md` → "UI-E2E harness". | ## Golden-file nets diff --git a/scripts/tests/mutation_manifest.py b/scripts/tests/mutation_manifest.py index d09fde233..e6ff8f6d5 100644 --- a/scripts/tests/mutation_manifest.py +++ b/scripts/tests/mutation_manifest.py @@ -403,6 +403,24 @@ MUTATIONS: tuple[Mutation, ...] = ( "that it moved). Disarming the comparison inside the script instead would prove the second " "and say nothing about the first, which is the half that was missing.", ), + Mutation( + guard="scripts/tests/test_image_build_delegates_the_spa_suite.py", + target="docker/Dockerfile", + clause="RUN npm run lint && npm run typecheck && npm run build", + replacement="RUN npm run lint && npm run typecheck && npm test -- --run && npm run build", + proof="test_image_build_delegates_the_spa_suite.py::test_no_DOCKERFILE_runs_the_spa_suite", + granularity=CLAUSE, + expect="runs the SPA vitest suite, but its build context has no git", + why="The target is not the guard, per the checker-guard exception: the guard IS a test, so " + "disarming it makes it absent rather than red, and the mutation goes into the guarded " + "ARTIFACT. The clause is the web-build stage's whole command line, and the replacement is " + "the defect itself rather than a caricature of it — ersatztv#883 put a vitest run back into " + "a gitless stage, and every image build failed from that commit until #887. Mutating the " + "detector's own token list instead would prove only that the detector matches its own " + "constants; this proves it reads the real Dockerfile and reports the real shape. Note the " + "mutant is UNFILTERED: a filtered run is what actually broke, but the guard forbids the " + "class, and mutating to the narrower case would leave the broader one unproven.", + ), ) diff --git a/scripts/tests/test_guard_populations_derive_from_git.py b/scripts/tests/test_guard_populations_derive_from_git.py index 12fc69450..ecc437bb5 100644 --- a/scripts/tests/test_guard_populations_derive_from_git.py +++ b/scripts/tests/test_guard_populations_derive_from_git.py @@ -43,6 +43,7 @@ from scripts.tests import test_ci_image_pin_population as image_pin from scripts.tests import test_ci_status_context_uniqueness as ctx_unique from scripts.tests import test_guard_inventory as guard_inventory from scripts.tests import test_hook_fire_log as hook_fire +from scripts.tests import test_image_build_delegates_the_spa_suite as image_build from scripts.tests import test_pr_changed_files as pr_changed from scripts.tests import test_remote_state_inventory as remote_state from scripts.tests import test_workflow_job_guards as job_guards @@ -108,6 +109,8 @@ def _as_relative_strings(members) -> set[str]: DERIVATIONS = ( ("test_guard_inventory.derived_guard_files", guard_inventory.derived_guard_files, 25), ("test_hook_fire_log.hook_scripts", hook_fire.hook_scripts, 10), + ("test_image_build_delegates_the_spa_suite.dockerfiles", image_build.dockerfiles, 4), + ("test_image_build_delegates_the_spa_suite.workflow_files", image_build.workflow_files, 5), ("test_ci_image_pin_population.workflow_files", image_pin.workflow_files, 5), ("test_ci_status_context_uniqueness.workflow_files", ctx_unique.workflow_files, 5), ("test_remote_state_inventory.derived_population", remote_state.derived_population, 40), diff --git a/scripts/tests/test_image_build_delegates_the_spa_suite.py b/scripts/tests/test_image_build_delegates_the_spa_suite.py new file mode 100644 index 000000000..37735732a --- /dev/null +++ b/scripts/tests/test_image_build_delegates_the_spa_suite.py @@ -0,0 +1,738 @@ +"""The SPA vitest suite never runs inside an image build, and the image is gated on a job that does +run it, unfiltered (ersatztv#887). + +WHAT THIS IS PROTECTING. `docker/Dockerfile`'s `web-build` stage is gitless twice over: its build +context is `web/` + `design-system/`, so there is no `.git`, and `node:22-bookworm-slim` ships no +git binary. Members of the SPA suite need one or the other — the `web/src/api/*.guard.test.ts` +guards derive their file population from `git ls-files` and refuse to fall back to a directory walk +(`testing.guard-derives-population-from-source`), and `trackedSourceFiles.realgit.test.ts` builds a +real temporary repository. Running the suite there anyway therefore requires a list of the members +that cannot run, and that list is a population nothing derives: ersatztv#883 added a third member +without updating a hand-written pair of `--exclude`s, and every image build failed from that commit +on. The red was structurally unreachable on a PR — `Build & push image (amd64)` carries +`if: github.event_name != 'pull_request'` — so it landed on `main` and on the `v*` tag path, where a +release cut fails at the image build and `:latest` stops being republished. + +The fix removed the list rather than extending it. This guard is what keeps it removed, and what +holds up the safety argument that licensed removing it, because that argument is a claim about two +OTHER files and would otherwise rot silently in a comment. + +THE INVARIANT, in the two halves it actually has: + + * NEGATIVE — no Dockerfile runs the suite. Not "no `--exclude` appears": an unfiltered suite run + in a gitless stage fails just as hard, and a filtered one is the trap itself. The whole class is + excluded rather than a spelling of it. + * POSITIVE — every job that publishes an image built from a Dockerfile CARRYING THE SPA SOURCE is + transitively gated on a job that runs the suite, and that run is unfiltered. Without this half + the negative half is a hole: deleting the suite run from the Dockerfile and deleting + `needs: [test, ...]` from `build` would both be green. + +POPULATIONS, all derived — never a filesystem walk and never a hand-written list +(`testing.guard-derives-population-from-source`, ersatztv#806): + + * Dockerfiles and workflows: the git index, via `tracked_files`. Both the `*.yml` and `*.yaml` + workflow spellings, for the reason `test_workflow_persist_credentials.py` states — Gitea accepts + either, so a `.yaml` workflow would otherwise be structurally invisible. + * WHICH npm SCRIPTS ARE THE SUITE: read out of `web/package.json`, by finding the scripts whose + body invokes the vitest binary. `npm test` is a suite run because `"test": "vitest"` is; the + sibling `"test:ui-e2e": "playwright test"` is not, and no exemption says so. A guess in either + direction here is a live defect — treating `test:ui-e2e` as the suite would demand a gate on a + Playwright run, and hardcoding the name `test` would go quiet the day the script is renamed. + * Image-publishing jobs: every job with a `docker/build-push-action` step, and the Dockerfile it + builds is read out of that step's `file:` input rather than hardcoded here. That is what keeps + a renamed Dockerfile red instead of silently uncovered, and it is why `ci-image.yml` — which + publishes the CI toolchain image from `docker/ci/Dockerfile`, a Dockerfile that carries no SPA + source — is correctly out of scope WITHOUT an exemption entry. An exemption outlives its reason; + a derivation does not. + +WHY A SHELL-POSITION PARSER RATHER THAN A REGEX. The first version matched `vitest` anywhere in a +command and reported `echo the vitest suite does not run here` as an invocation. A predicate that +cannot tell a command from prose about a command produces false reds in exactly the files that +explain the rule, and those get silenced. Commands are split on shell separators and only the HEAD +of each segment is consulted, so a runner named in an argument or in prose is not a run. + +WHAT IS NOT COVERED, stated rather than implied: + + * A line whose FIRST non-space character is `#` is treated as inert, in both file kinds. That is + right for a Dockerfile comment and for a shell comment at the start of a line inside a `run:` + body, and it is what lets `docker/Dockerfile`'s own explanatory comment name the thing it + forbids. A suite invocation appended after a `#` on a line that also carries code is NOT a + comment and is still seen, because only a leading `#` is stripped. + * In a Dockerfile only `RUN` executes at build time, so only `RUN` is read. A `CMD`/`ENTRYPOINT` + that runs the suite would run in the shipped container, which is a different (and absent) + problem; it is out of scope rather than overlooked. + * A `RUN` that invokes a SCRIPT which in turn runs the suite is invisible to this — the parser + reads commands, not what they call. No such script exists today; the failure mode is a missed + detection, not a false green about the gate, because the positive half is derived independently. + * An image published by something other than `docker/build-push-action` — a raw `docker push` in a + `run:` body — is outside the publishing-job derivation. `test_no_run_BODY_builds_or_pushes_an_image` + asserts that shape does not exist rather than leaving it uncovered. + * A genuinely new value-taking vitest flag is reported as a spec filter, because every non-flag + token that is not the value of a KNOWN value-taking flag is treated as one. That is a false red + a reader fixes by naming the flag here — the safe direction. +""" + +from __future__ import annotations + +import json +import re +import shlex +from pathlib import Path + +import yaml + +from scripts.tests import tracked_files +from scripts.tests.tracked_files import tracked_paths + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOW_DIR = ".gitea/workflows" +WORKFLOWS = (WORKFLOW_DIR, ("*.yml", "*.yaml")) +PACKAGE_JSON = "web/package.json" + +# The action every image publish in this repo goes through. Matched on the last two path segments +# for the reason `test_workflow_persist_credentials.py` gives: Gitea accepts a full action URL, and +# a host is not required to contain a dot, so no hostname heuristic is safe. +PUBLISH_ACTION = "docker/build-push-action" + +# Shell tokens that end one command and start another. `shlex.split` returns each of these as its +# own token, which is what makes segmenting possible without a shell grammar. +SEPARATORS = frozenset({"&&", "||", ";", "|", "&"}) + +# Prefixes that wrap a command without being one. Skipped so `env FOO=1 npm test` is still a run. +WRAPPERS = frozenset({"env", "sudo", "time", "exec", "command", "nice", "ionice"}) + +# Dockerfile instructions. Only `RUN` executes during a build; the rest are read as not-a-command so +# a `COPY` whose argument happens to be a runner name is not mistaken for one. +DOCKERFILE_INSTRUCTIONS = frozenset( + { + "ADD", + "ARG", + "CMD", + "COPY", + "ENTRYPOINT", + "ENV", + "EXPOSE", + "FROM", + "HEALTHCHECK", + "LABEL", + "MAINTAINER", + "ONBUILD", + "RUN", + "SHELL", + "STOPSIGNAL", + "USER", + "VOLUME", + "WORKDIR", + } +) + +# Flags that narrow WHICH specs run. Every one of these reintroduces the defect: the set of specs +# that ran becomes a thing someone maintains by hand, and nothing compares it to the set that exists. +NARROWING_FLAGS = frozenset( + {"--exclude", "--include", "--dir", "--project", "--shard", "--changed", "--related", "--testNamePattern", "-t"} +) + +# Flags whose NEXT token is a value rather than a spec filter, so `--reporter verbose` is not read as +# a filter. Deliberately short — see the docstring's last residual. +VALUE_TAKING_FLAGS = frozenset( + { + "--reporter", + "--outputFile", + "--config", + "--root", + "--environment", + "--pool", + "--maxWorkers", + "--minWorkers", + "--testTimeout", + "--hookTimeout", + "--retry", + "--coverage.reporter", + "--coverage.provider", + } +) + +_ENV_ASSIGNMENT = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") + + +def _canonical_action(uses: str) -> str: + """The last two path segments of a `uses:` value, without its `@version`. + + Same normalisation as `test_workflow_persist_credentials._canonical_action`, and deliberately a + second copy rather than an import: that guard's population is checkout steps, this one's is + publish steps, and sharing the helper would mean a narrowing there silently narrows here too. + """ + ref = uses.strip().strip("'\"").rsplit("@", 1)[0].strip().lower() + ref = re.sub(r"^https?://", "", ref) + segments = [seg for seg in ref.split("/") if seg] + if segments: + segments[-1] = re.sub(r"\.git$", "", segments[-1]) + return "/".join(segments[-2:]) if len(segments) >= 2 else ref + + +def logical_commands(text: str) -> list[tuple[int, str]]: + """`(1-based line number, command)` per command line: comments dropped, `\\` continuations joined. + + Both normalisations are load-bearing. Dropping leading-`#` lines is what lets a Dockerfile + explain the rule it enforces without tripping it. Joining backslash continuations is what makes + a multi-line invocation ONE command with its flags attached — the shape the removed `--exclude` + pair had, and the shape a re-added one would have. Scanning raw lines instead would read + `npm test -- --run` as unfiltered and report it clean. + """ + commands: list[tuple[int, str]] = [] + buffer = "" + start = 0 + for number, raw in enumerate(text.splitlines(), start=1): + if raw.lstrip().startswith("#"): + continue + if not buffer: + start = number + stripped = raw.rstrip() + if stripped.endswith("\\"): + buffer += stripped[:-1] + " " + continue + buffer += stripped + if buffer.strip(): + commands.append((start, buffer.strip())) + buffer = "" + if buffer.strip(): + commands.append((start, buffer.strip())) + return commands + + +def command_segments(command: str) -> list[list[str]]: + """The shell segments of one command, as token lists, or `[]` if nothing executes. + + A leading Dockerfile instruction is consumed: `RUN` yields its command, every other instruction + yields nothing, since only `RUN` runs during a build. + """ + try: + tokens = shlex.split(command) + except ValueError: + # An unbalanced quote is not a command anyone can reason about. Returning the raw split + # keeps it VISIBLE to the detectors rather than silently dropping it. + tokens = command.split() + if not tokens: + return [] + # A YAML step written on one line — `- run: npm test -- --run` — is a command, and roughly half + # the `run:` steps in this repo are written that way. Without this the raw-text cross-check below + # reads `run:` as the program and finds nothing, so a single-line invocation would be invisible + # to every text-based assertion while the block form was caught. + if tokens[0] == "-": + tokens = tokens[1:] + if tokens and tokens[0] == "run:": + tokens = tokens[1:] + if not tokens: + return [] + head = tokens[0] + if head.isupper() and head in DOCKERFILE_INSTRUCTIONS: + if head != "RUN": + return [] + tokens = tokens[1:] + + segments: list[list[str]] = [] + current: list[str] = [] + for token in tokens: + if token in SEPARATORS: + if current: + segments.append(current) + current = [] + continue + current.append(token) + if current: + segments.append(current) + return segments + + +def _strip_prefixes(tokens: list[str]) -> list[str]: + index = 0 + while index < len(tokens) and (_ENV_ASSIGNMENT.match(tokens[index]) or tokens[index] in WRAPPERS): + index += 1 + return tokens[index:] + + +def direct_vitest_args(tokens: list[str]) -> list[str] | None: + """The arguments of a segment that invokes the vitest BINARY, or `None`. + + Covers `vitest …` (npm puts `node_modules/.bin` on PATH for a script body) and + `npx/npm exec/pnpm exec/yarn dlx vitest …`. Kept separate from the npm-script route below so the + `web/package.json` scan can use it without recursing through the script table it is building. + """ + tokens = _strip_prefixes(tokens) + if not tokens: + return None + if tokens[0] == "vitest": + return tokens[1:] + if tokens[0] in {"npx", "pnpm", "npm", "yarn"}: + index = 1 + if tokens[0] != "npx": + if index >= len(tokens) or tokens[index] not in {"exec", "dlx"}: + return None + index += 1 + while index < len(tokens) and tokens[index] in {"--", "-y", "--yes", "--no-install"}: + index += 1 + if index < len(tokens) and tokens[index] == "vitest": + return tokens[index + 1 :] + return None + + +def vitest_scripts() -> set[str]: + """The `web/package.json` script names whose body runs the vitest binary. + + This is the derivation that decides what `npm test` and `npm run X` mean. `"test": "vitest"` is + in; `"test:ui-e2e": "playwright test"` is not, and needs no exemption to be left out. + """ + scripts = json.loads((REPO_ROOT / PACKAGE_JSON).read_text(encoding="utf-8")).get("scripts", {}) + named: set[str] = set() + for name, body in scripts.items(): + if not isinstance(body, str): + continue + if any(direct_vitest_args(segment) is not None for segment in command_segments(body)): + named.add(str(name)) + return named + + +def suite_args(tokens: list[str], scripts: set[str]) -> list[str] | None: + """The arguments of a segment that runs the SPA suite, or `None` if it does not run it.""" + direct = direct_vitest_args(tokens) + if direct is not None: + return direct + tokens = _strip_prefixes(tokens) + if not tokens or tokens[0] not in {"npm", "pnpm", "yarn", "bun"}: + return None + index = 1 + if index < len(tokens) and tokens[index] in {"run", "run-script"}: + index += 1 + if index >= len(tokens) or tokens[index] not in scripts: + return None + return tokens[index + 1 :] + + +def suite_invocations(text: str, scripts: set[str] | None = None) -> list[tuple[int, str]]: + """Every command in `text` that runs the SPA vitest suite.""" + scripts = vitest_scripts() if scripts is None else scripts + found: list[tuple[int, str]] = [] + for number, command in logical_commands(text): + if any(suite_args(segment, scripts) is not None for segment in command_segments(command)): + found.append((number, command)) + return found + + +def narrowing_arguments(command: str, scripts: set[str] | None = None) -> list[str]: + """The arguments in `command` that restrict WHICH specs run, or `[]` when it runs all of them.""" + scripts = vitest_scripts() if scripts is None else scripts + found: list[str] = [] + for segment in command_segments(command): + args = suite_args(segment, scripts) + if args is None: + continue + index = 0 + while index < len(args): + argument = args[index] + index += 1 + if argument == "--": + continue + base = argument.split("=", 1)[0] + if base in NARROWING_FLAGS: + found.append(argument) + if "=" not in argument and index < len(args) and not args[index].startswith("-"): + found.append(args[index]) + index += 1 + continue + if base in VALUE_TAKING_FLAGS and "=" not in argument: + index += 1 # its value is not a filter + continue + if argument.startswith("-"): + continue + found.append(argument) + return found + + +def dockerfiles() -> list[str]: + """Tracked Dockerfiles, by repo-relative path, in every spelling docker recognises. + + `tracked_files._git_ls_files()` through the MODULE, never a `from … import` binding. The shared + proof in `test_guard_populations_derive_from_git.py` narrows the index by patching that module + attribute, and a name bound at import time is not reachable from there — the derivation would go + on returning every Dockerfile after git stopped tracking them, and the proof would say so. A + binding captured before the patch is the same shape that file warns about one block down. + """ + return sorted( + path + for path in tracked_files._git_ls_files() + if path.rpartition("/")[2].startswith("Dockerfile") or path.endswith(".Dockerfile") + ) + + +def workflow_files() -> list[Path]: + return tracked_paths(*WORKFLOWS) + + +def _read(rel: str) -> str: + return (REPO_ROOT / rel).read_text(encoding="utf-8") + + +def carries_spa_source(rel: str) -> bool: + """Does this Dockerfile copy the SPA source tree into the build? + + This is what makes a suite run there possible at all, and it is what separates + `docker/Dockerfile` from `docker/ci/Dockerfile` — which installs npm packages globally but never + copies `web/`. Derived from the `COPY` instructions rather than from a list of Dockerfile paths. + + Two shapes count. `COPY web/… ` names the tree; `COPY . …` carries it without naming it, and the + question is what the stage HAS, not what it says. No Dockerfile here uses the second form today — + it is matched anyway because the gate assertion SKIPS every Dockerfile this answers False for, so + a miss here drops an image out of the check silently rather than reddening. + + A `COPY --from=` is not a context copy: its source is another stage, so `--from=build .` + would not bring the repo in. Flags are dropped before the source argument is read. + """ + for _, command in logical_commands(_read(rel)): + tokens = command.split() + if not tokens or tokens[0].upper() != "COPY": + continue + arguments = [token for token in tokens[1:] if not token.startswith("--")] + if len(arguments) < 2: + continue + if any(token.startswith("--from=") for token in tokens[1:]): + continue + sources = arguments[:-1] + if any(source in {".", "./"} or re.match(r"^\.?/?web/", source) for source in sources): + return True + return False + + +def _jobs(doc: object) -> dict[str, dict]: + jobs = doc.get("jobs") if isinstance(doc, dict) else None + if not isinstance(jobs, dict): + return {} + return {str(key): value for key, value in jobs.items() if isinstance(value, dict)} + + +def _steps(job: dict) -> list[dict]: + steps = job.get("steps") + return [step for step in steps if isinstance(step, dict)] if isinstance(steps, list) else [] + + +def _run_bodies(job: dict) -> list[str]: + return [step["run"] for step in _steps(job) if isinstance(step.get("run"), str)] + + +def _needs(job: dict) -> list[str]: + needs = job.get("needs") + if isinstance(needs, str): + return [needs] + return [str(item) for item in needs] if isinstance(needs, list) else [] + + +def _transitive_needs(job_id: str, jobs: dict[str, dict]) -> set[str]: + seen: set[str] = set() + frontier = list(_needs(jobs.get(job_id, {}))) + while frontier: + current = frontier.pop() + if current in seen: + continue + seen.add(current) + frontier.extend(_needs(jobs.get(current, {}))) + return seen + + +def publishing_jobs() -> list[tuple[str, str, str, dict[str, dict]]]: + """`(workflow rel path, job id, Dockerfile rel path, that workflow's jobs)` per publishing job. + + The Dockerfile comes from the step's own `file:` input, defaulting to docker's own default + (`Dockerfile` at the context root) when the input is absent, so a step that stops naming one is + still resolved rather than dropped from the population. + """ + found: list[tuple[str, str, str, dict[str, dict]]] = [] + for path in workflow_files(): + rel = path.relative_to(REPO_ROOT).as_posix() + jobs = _jobs(yaml.safe_load(path.read_text(encoding="utf-8"))) + for job_id, job in jobs.items(): + for step in _steps(job): + uses = step.get("uses") + if not isinstance(uses, str) or _canonical_action(uses) != PUBLISH_ACTION: + continue + with_block = step.get("with") + named = with_block.get("file") if isinstance(with_block, dict) else None + dockerfile = str(named or "Dockerfile").strip() + found.append((rel, job_id, re.sub(r"^\./", "", dockerfile), jobs)) + return found + + +# ------------------------------------------------------------------------------------------------ +# The invariant +# ------------------------------------------------------------------------------------------------ + + +def test_no_DOCKERFILE_runs_the_spa_suite() -> None: + """The negative half. A gitless stage runs no vitest suite, filtered or not.""" + scripts = vitest_scripts() + faults: list[str] = [] + for rel in dockerfiles(): + for number, command in suite_invocations(_read(rel), scripts): + faults.append( + f"{rel}:{number} runs the SPA vitest suite, but its build context has no git and " + f"`node:22-bookworm-slim` ships no git binary, so the suite's git-dependent members " + f"cannot run there: `{command.strip()}`. Excluding them by name is the defect this " + f"guard exists to prevent — that list is a population nothing derives, it went " + f"stale in ersatztv#883, and the resulting red is unreachable on a PR, so it lands " + f"on `main` and on the release tag. The suite runs in `docker-build.yml`'s `test` " + f"job, which gates the image; delete this invocation rather than filtering it " + f"(ersatztv#887)." + ) + assert not faults, "\n".join(faults) + + +def test_every_IMAGE_PUBLISHING_job_is_gated_on_the_job_that_runs_the_suite() -> None: + """The positive half, without which deleting `needs:` is silently green.""" + scripts = vitest_scripts() + known = set(dockerfiles()) + faults: list[str] = [] + for rel, job_id, dockerfile, jobs in publishing_jobs(): + if dockerfile not in known: + faults.append( + f"{rel}: job `{job_id}` builds `{dockerfile}`, which is not a tracked Dockerfile. " + f"The population here is derived from the git index, so this job's image is " + f"UNCHECKED — fix the path or widen `dockerfiles()`." + ) + continue + if not carries_spa_source(dockerfile): + continue + gates = _transitive_needs(job_id, jobs) + runners = sorted( + gate for gate in gates if any(suite_invocations(body, scripts) for body in _run_bodies(jobs.get(gate, {}))) + ) + if not runners: + faults.append( + f"{rel}: job `{job_id}` publishes an image built from `{dockerfile}`, which carries " + f"the SPA source, but none of the jobs it transitively `needs:` ({sorted(gates)}) " + f"runs the SPA suite. Since ersatztv#887 the image build deliberately does not run " + f"the suite itself, so this `needs:` edge is the ONLY thing standing between a red " + f"suite and a published image." + ) + assert not faults, "\n".join(faults) + + +def test_every_spa_suite_invocation_is_UNFILTERED() -> None: + """One list, and it is empty. A narrowed run is the hand-maintained population coming back. + + Driven off the YAML walk rather than the file text, so it reads exactly what the gate assertion + above reads. `test_the_yaml_walk_finds_every_suite_invocation_the_TEXT_does` is what stops the + walk from silently narrowing. + """ + scripts = vitest_scripts() + faults: list[str] = [] + for path in workflow_files(): + rel = path.relative_to(REPO_ROOT).as_posix() + jobs = _jobs(yaml.safe_load(path.read_text(encoding="utf-8"))) + for job_id, job in jobs.items(): + for body in _run_bodies(job): + for _, command in suite_invocations(body, scripts): + narrowing = narrowing_arguments(command, scripts) + if narrowing: + faults.append( + f"{rel}: job `{job_id}` runs the SPA suite but restricts which specs run " + f"({narrowing}): `{command.strip()}`. The gating run must be the WHOLE " + f"suite — a filtered one re-creates the hand-maintained population " + f"ersatztv#887 removed." + ) + assert not faults, "\n".join(faults) + + +def test_the_yaml_walk_finds_every_suite_invocation_the_TEXT_does() -> None: + """The walk and a plain text scan must agree per file, or the walk has a blind spot. + + Both assertions above read `jobs..steps[].run`. A step list the walk cannot reach makes a + suite run structurally invisible while the guard still reports a clean pass, and absence of + output is not evidence. The two shapes that would escape BOTH halves — a local composite action + and a `jobs..uses:` reusable workflow — are asserted not to exist AT ALL by + `test_workflow_persist_credentials.py`, repo-wide and for the same reason; they are not + re-asserted here, because two guards on one condition mask each other (ersatztv#685). + """ + scripts = vitest_scripts() + disagreements: list[str] = [] + for path in workflow_files(): + text = path.read_text(encoding="utf-8") + jobs = _jobs(yaml.safe_load(text)) + walked = sum(len(suite_invocations(body, scripts)) for job in jobs.values() for body in _run_bodies(job)) + scanned = len(suite_invocations(text, scripts)) + if walked != scanned: + disagreements.append( + f"{path.name}: the jobs..steps walk found {walked} SPA-suite invocation(s) but " + f"the text scan found {scanned}. The walk is what both assertions above run over, so " + f"the difference is suite runs this guard cannot see — widen `_run_bodies`." + ) + assert not disagreements, "\n".join(disagreements) + + +def test_no_run_BODY_builds_or_pushes_an_image() -> None: + """The publishing-job derivation keys on the action; assert nothing publishes around it. + + Both halves would agree on zero if an image were built by a raw `docker push` in a shell body, + so this reports the shape rather than leaving it uncovered. + """ + stray: list[str] = [] + for path in workflow_files(): + rel = path.relative_to(REPO_ROOT).as_posix() + jobs = _jobs(yaml.safe_load(path.read_text(encoding="utf-8"))) + for job_id, job in jobs.items(): + for body in _run_bodies(job): + for number, command in logical_commands(body): + for segment in command_segments(command): + head = _strip_prefixes(segment) + publishes = head[:3] == ["docker", "buildx", "build"] or ( + len(head) >= 2 and head[0] == "docker" and head[1] in {"push", "build"} + ) + if publishes: + stray.append(f"{rel}: job `{job_id}`, line {number} of a run body: `{command.strip()}`") + assert not stray, ( + f"image build/push outside `{PUBLISH_ACTION}`: {stray}. The publishing-job population above " + f"is derived from that action, so these jobs are NOT checked for the suite gate — extend " + f"`publishing_jobs()` before adding one." + ) + + +# ------------------------------------------------------------------------------------------------ +# Anti-vacuity and negative controls +# ------------------------------------------------------------------------------------------------ + + +def test_the_populations_are_NOT_empty() -> None: + """Every assertion above passes over an empty set, which is how a completeness guard dies.""" + assert dockerfiles(), "`git ls-files` reported no Dockerfile — the derivation is broken, not the repo." + assert workflow_files(), f"`git ls-files` reported no {WORKFLOW_DIR}/*.yml — the derivation is broken." + + scripts = vitest_scripts() + assert scripts, ( + f"no script in {PACKAGE_JSON} was read as running vitest, so every assertion in this file " + "measures an empty set: no command can be recognised as a suite run. `direct_vitest_args` " + "stopped matching the script body, or the scripts moved." + ) + + spa_carrying = [rel for rel in dockerfiles() if carries_spa_source(rel)] + assert spa_carrying, ( + f"none of {dockerfiles()} was read as carrying the SPA source, so " + "`test_every_IMAGE_PUBLISHING_job_is_gated_on_the_job_that_runs_the_suite` skipped every " + "job and asserted nothing. `carries_spa_source` stopped matching the `COPY web/` it derives." + ) + + published = publishing_jobs() + assert published, ( + f"no job in {WORKFLOW_DIR} uses `{PUBLISH_ACTION}`, so the gate assertion ran over an empty " + "population. Either the repo stopped publishing images or `_canonical_action` stopped " + "recognising the action." + ) + + runs = [ + (path.name, number) + for path in workflow_files() + for number, _ in suite_invocations(path.read_text(encoding="utf-8"), scripts) + ] + assert runs, ( + "no workflow runs the SPA suite at all. The image gate then rests on a job that runs " + "nothing, and `test_every_spa_suite_invocation_is_UNFILTERED` asserts over an empty set." + ) + + +def test_the_SCRIPT_derivation_separates_vitest_from_playwright() -> None: + """Pinned in both directions: this table is what every other assertion here keys on. + + `test` in and `test:ui-e2e` out is the whole reason there is no exemption list. If the + derivation drifted to include `test:ui-e2e`, the guard would demand an unfiltered gate on a + Playwright run; if it drifted to exclude `test`, nothing in the repo would be recognised as a + suite run and every assertion above would pass vacuously. + """ + scripts = vitest_scripts() + assert "test" in scripts, scripts + assert "test:ui-e2e" not in scripts, scripts + assert "build" not in scripts and "lint" not in scripts, scripts + + +def test_the_detector_recognises_the_SPELLINGS_it_claims_to() -> None: + """Negative control on the predicates, driven directly rather than through the tree. + + The mutation proof drives the real Dockerfile; this covers the spellings the tree does not + contain, so a detector that quietly stopped matching one is caught without waiting for someone + to write it. + """ + scripts = {"test", "test:unit"} + for command in ( + "npm test -- --run", + "npm run test", + "npm run test:unit", + "npx vitest run", + "npm exec vitest run", + "vitest run", + "cd web && npm test -- --run", + "npm run lint && npm test -- --run && npm run build", + "RUN npm test -- --run", + "env CI=1 npm test -- --run", + # The one-line YAML step form, which roughly half this repo's `run:` steps use. + "run: npm test -- --run", + "- run: npm test -- --run", + ): + assert suite_invocations(command, scripts), command + + for command in ( + "dotnet test --configuration Release", + "npm ci", + "npm run build", + "npm run lint", + "npm run typecheck", + "npm run test:ui-e2e", # not in `scripts`, i.e. not a vitest script + "playwright test", + # Prose ABOUT the suite is not a run of it. The first version of this guard reported both of + # these, which is a false red in exactly the files that explain the rule. + "echo the vitest suite does not run here", + "echo skipping npm test in this stage", + # A non-RUN Dockerfile instruction does not execute during a build. + "CMD npm test -- --run", + "COPY vitest.config.ts ./", + ): + assert not suite_invocations(command, scripts), command + + assert narrowing_arguments("npm test -- --run", scripts) == [] + assert narrowing_arguments("npm test -- --run --reporter verbose", scripts) == [] + assert narrowing_arguments("npm test -- --run --exclude 'src/api/x.guard.test.ts'", scripts) == [ + "--exclude", + "src/api/x.guard.test.ts", + ] + assert narrowing_arguments("npm test -- --run --project=spa", scripts) == ["--project=spa"] + assert narrowing_arguments("npm test -- --run src/api", scripts) == ["src/api"] + assert narrowing_arguments("npm test -- --run --changed", scripts) == ["--changed"] + + +def test_a_COMMENT_is_inert_but_only_a_LEADING_hash_is() -> None: + """`docker/Dockerfile` names the rule it enforces; that must not trip the rule. + + The second half is the one that matters: a `#` mid-line does not make what precedes it a + comment, and treating it as one would let a real invocation hide behind a trailing note. + """ + scripts = {"test"} + assert not suite_invocations("# npm test -- --run\n", scripts) + assert not suite_invocations(" # the vitest suite is not run in this stage\n", scripts) + assert suite_invocations("RUN npm test -- --run # left over from before\n", scripts) + + +def test_the_continuation_JOIN_keeps_flags_with_their_command() -> None: + """A multi-line invocation is one command; its flags must not read as separate lines. + + This is the exact shape the removed exclusion pair had, so a guard that scanned raw lines would + see `npm test -- --run` as unfiltered and report it clean. + """ + scripts = {"test"} + multiline = "RUN npm test -- --run \\\n --exclude 'src/api/a.test.ts' \\\n --exclude 'b.test.ts'\n" + found = suite_invocations(multiline, scripts) + assert len(found) == 1, found + assert narrowing_arguments(found[0][1], scripts) == ["--exclude", "src/api/a.test.ts", "--exclude", "b.test.ts"] + + +def test_the_spa_carrying_predicate_separates_the_two_real_DOCKERFILES() -> None: + """Pinned both ways, because the gate assertion is SKIPPED for everything it answers False for. + + A predicate that drifted to False for `docker/Dockerfile` would make the positive half vacuous + while `test_the_populations_are_NOT_empty` still passed on some other file. + """ + assert carries_spa_source("docker/Dockerfile") + assert not carries_spa_source("docker/ci/Dockerfile") diff --git a/web/vite-plugins/trackedSourceFiles.realgit.test.ts b/web/vite-plugins/trackedSourceFiles.realgit.test.ts index f911f11f9..9a81861be 100644 --- a/web/vite-plugins/trackedSourceFiles.realgit.test.ts +++ b/web/vite-plugins/trackedSourceFiles.realgit.test.ts @@ -13,10 +13,11 @@ import { type GitRunner, resolveTrackedSourceFiles } from './trackedSourceFiles' * plugin's refusal. Reading the two exclusions as checkout-versus-binary is what broke the Docker * stage once already; see `docker/Dockerfile`, which retracts that framing. * - * `docker/Dockerfile`'s web-build stage runs on `node:22-bookworm-slim`, which ships no git at all, - * so this file is excluded there alongside the guard. Keeping them apart means - * that exclusion costs two files rather than the thirteen injected-runner tests in - * `trackedSourceFiles.test.ts`, which run fine with no git and no repository. + * Keeping these apart from `trackedSourceFiles.test.ts` confines the git prerequisite to one file: + * its thirteen injected-runner siblings run with no git and no repository. The suite as a whole is + * therefore run only where git is present. `docker/Dockerfile`'s web-build stage is not such a + * place — `node:22-bookworm-slim` ships no git at all — and since ersatztv#887 it does not run the + * suite there rather than naming the files that cannot run, which was a list nothing derived. * * Do NOT make these skip when git is missing. A check that answers "could not tell" by passing is a * permanent no-op, which is the whole failure mode this plugin exists to avoid.