diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 1181ab736..f6dc55de5 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -173,7 +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. A THIRD assertion exists because the first two together still certify a publish on which the suite never ran: the gating step must not be advisory. Two independent cold reviews each demonstrated that gap by execution rather than argument, and between them produced twelve mutants the guard passed on arrival — `continue-on-error: true`, `if: false`, a job-level `if:`, an or-true suffix on the suite command, a pipeline into `tee`, `set +e`, `--root src/api`, `--config other.ts`, an unspaced `lint&&npm test`, a `;`-separated re-add, `sh -c "npm test …"`, and `npm --prefix web test`. All twelve now redden; each was re-measured after the fix. THE REACH IS MEASURED, NOT ASSERTED, and the residuals below are what survived that measurement rather than what seemed likely. (1) A `RUN` or step that calls a SCRIPT which runs the suite is invisible — the parser reads commands, not what they call, and `sh -c` is expanded only because its argument IS a command line; no such script exists today, and the miss is a missed DETECTION on the negative assertion, never a false green on the positive one, which is derived from the job graph instead. (2) Only `RUN` is read in a Dockerfile, since nothing else executes during a build. (3) A line whose FIRST non-space character is `#` is inert — that is what lets `docker/Dockerfile` name the rule it is subject to — and only a LEADING `#`, so an invocation with a trailing note is still seen. (4) The step `if:` is checked against a HAND-WRITTEN set of one condition, which is a deliberate review trigger rather than a derivation: a legitimate change reddens and its author states the reason. (5) `test_the_SCRIPT_derivation_separates_vitest_from_playwright` pins the two script NAMES, so renaming the `test` script is a false red — measured, and the derivation itself adapts correctly; the pin is the only thing that catches a wrong-but-non-empty script table, which the anti-vacuity check cannot. (6) A new value-taking vitest flag is reported as a spec filter, a false red fixed by naming it | +| `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. A THIRD assertion exists because the first two together still certify a publish on which the suite never ran: the gating step must not be advisory. Two independent cold reviews each demonstrated that gap by execution rather than argument. A 24-mutant battery now stands behind the reach: **24 applied, 0 missed**, each caught by the assertion intended for it, re-measured after every fix round. Twelve came from the two reviews (`continue-on-error: true`, `if: false`, a job-level `if:`, an or-true suffix, a pipeline into `tee`, `set +e`, `--root`, `--config`, an unspaced `lint&&npm test`, a `;`-separated re-add, `sh -c`, `npm --prefix`); two more were found by the author attacking the fix (`npm test && echo ok || true`, where the or-true is not adjacent to the suite yet still swallows its status, and a backgrounded `&`); the rest cover `xargs`, `npx -p`, `sh -euxc`, the JSON/`ADD`/no-slash copy spellings, and a heredoc standing in for the run — that last one reddens TWO assertions, since the gate loses its runner and anti-vacuity fires. **GRADING, stated rather than implied: exactly ONE of those 24 is DECLARED in `mutation_manifest.py` and re-executed every suite; the other 23 were witnessed by hand during development and are NOT standing** — the same footing `pageSizeCallSites.guard.test.ts` states for its nine. The battery is development evidence for the reach, not a per-run proof of it. THE REACH IS MEASURED, NOT ASSERTED, and the residuals below are what survived that measurement rather than what seemed likely. (1) A `RUN` or step that calls a SCRIPT which runs the suite is invisible — the parser reads commands, not what they call, and `sh -c` is expanded only because its argument IS a command line; no such script exists today, and the miss is a missed DETECTION on the negative assertion, never a false green on the positive one, which is derived from the job graph instead. (2) Only `RUN` is read in a Dockerfile, since nothing else executes during a build. (3) A line whose FIRST non-space character is `#` is inert — that is what lets `docker/Dockerfile` name the rule it is subject to — and only a LEADING `#`, so an invocation with a trailing note is still seen. (4) The step `if:` is checked against a HAND-WRITTEN set of one condition, which is a deliberate review trigger rather than a derivation: a legitimate change reddens and its author states the reason. (5) `test_the_SCRIPT_derivation_separates_vitest_from_playwright` pins the two script NAMES, so renaming the `test` script is a false red — measured, and the derivation itself adapts correctly; the pin is the only thing that catches a wrong-but-non-empty script table, which the anti-vacuity check cannot. (6) A new value-taking vitest flag is reported as a spec filter, a false red fixed by naming it | | `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 | diff --git a/scripts/tests/test_image_build_delegates_the_spa_suite.py b/scripts/tests/test_image_build_delegates_the_spa_suite.py index cca679e85..ee5941be9 100644 --- a/scripts/tests/test_image_build_delegates_the_spa_suite.py +++ b/scripts/tests/test_image_build_delegates_the_spa_suite.py @@ -461,35 +461,14 @@ def suite_invocations(text: str, scripts: Mapping[str, list[str]] | None = None) return found -def _strip_redirections(args: list[str]) -> list[str]: - """Drop redirection operators, their targets, and any file descriptor written before them. - - `2>&1` tokenises as `['2', '>&', '1']`, so the descriptor and the target are bare tokens that - would otherwise both be reported as positional spec filters. - """ - kept: list[str] = [] - index = 0 - while index < len(args): - argument = args[index] - if _REDIRECTION.match(argument): - index += 2 # the operator and its target - continue - if ( - argument.isdigit() - and index + 1 < len(args) - and _REDIRECTION.match(args[index + 1]) - and not args[index + 1][:1].isdigit() - ): - index += 3 # descriptor, operator, target - continue - kept.append(argument) - index += 1 - return kept - - def _narrowing_in(args: list[str]) -> list[str]: - """The subset of `args` that restricts WHICH specs run.""" - args = _strip_redirections(args) + """The subset of `args` that restricts WHICH specs run. + + Redirections are consumed HERE rather than in a pre-pass, so a flag's value is claimed before the + file-descriptor rule can see it. As a pre-pass, `--exclude 2 > log` lost the `2`: the digit was + read as the descriptor of the following `>` and dropped with it, and the fault message then named + a filter without its value. Measured. + """ found: list[str] = [] index = 0 while index < len(args): @@ -497,6 +476,17 @@ def _narrowing_in(args: list[str]) -> list[str]: index += 1 if argument == "--": continue + if _REDIRECTION.match(argument): + index += 1 # the operator's target + continue + if ( + argument.isdigit() + and index < len(args) + and _REDIRECTION.match(args[index]) + and not args[index][:1].isdigit() + ): + index += 2 # the operator and its target; `argument` was its descriptor + continue base = argument.split("=", 1)[0] if base in NARROWING_FLAGS: found.append(argument) @@ -551,6 +541,13 @@ def failure_suppressions(command: str, scripts: Mapping[str, list[str]], depth: tokens = tokens[1:] if tokens[0] == "RUN" else [] current: list[str] = [] + # Whether the SUITE has run earlier in this `;`-delimited list. `&&`/`||` chain across a + # whole list, so `npm test && echo ok || true` runs `true` when the suite fails and exits 0 — + # the suite's failure is swallowed even though the `||` does not sit immediately after it. + # Looking only at the NEXT separator missed that; measured. A `;` (or a newline, since each + # line is walked separately) ends the list and resets, because a `||` in the next list is + # about the next command. + suite_in_list = False for word in [*tokens, ";"]: if word not in SEPARATORS: current.append(word) @@ -559,17 +556,21 @@ def failure_suppressions(command: str, scripts: Mapping[str, list[str]], depth: head = _strip_prefixes(current) if head[:2] == ["set", "+e"]: found.append("`set +e` disarms the step's own `set -e`") - if suite_args(current, scripts) is not None and word in {"||", "|"}: - found.append( - "a trailing or-true swallows the exit status" - if word == "||" - else "a pipeline reports the LAST command's status" - ) + if suite_args(current, scripts) is not None: + suite_in_list = True + if word == "|": + found.append("a pipeline reports the LAST command's status") if len(head) >= 3 and head[0] in SHELLS: for offset, inner in enumerate(head[1:-1], start=1): if inner.startswith("-") and not inner.startswith("--") and "c" in inner.lstrip("-"): found.extend(failure_suppressions(head[offset + 1], scripts, depth + 1)) break + if word == "||" and suite_in_list: + found.append("an `||` later in the same list swallows the exit status") + if word == "&" and suite_in_list: + found.append("`&` backgrounds the command, so its status is never awaited") + if word == ";": + suite_in_list = False current = [] return found @@ -1041,10 +1042,19 @@ def test_a_SUPPRESSED_failure_is_advisory_by_another_spelling() -> None: assert failure_suppressions("npm test -- --run || true", scripts) assert failure_suppressions("npm test -- --run | tee /tmp/spa.log", scripts) assert failure_suppressions("set +e\nnpm test -- --run", scripts) + # A `||` LATER in the same list still swallows it: if the suite fails, the `&&` right-hand side + # is skipped and the `||` right-hand side runs, so the list exits 0. Measured missed before the + # detector became list-scoped rather than next-separator-scoped. + assert failure_suppressions("npm test -- --run && echo ok || true", scripts) + assert failure_suppressions("npm test -- --run && npm run build || true", scripts) + assert failure_suppressions("npm test -- --run &", scripts) + assert failure_suppressions("( npm test -- --run ) || true", scripts) # An ordinary run, and a `||` on something that is NOT the suite, are both clean. assert failure_suppressions(marker + "npm test -- --run", scripts) == [] assert failure_suppressions("npm run lint && npm test -- --run", scripts) == [] assert failure_suppressions("npm run lint || true\nnpm test -- --run", scripts) == [] + # A `;` ends the list, so a `||` after it is about the NEXT command, not the suite. + assert failure_suppressions("npm test -- --run; npm run other || true", scripts) == [] def test_a_REDIRECTION_is_not_a_spec_filter() -> None: @@ -1059,8 +1069,11 @@ def test_a_REDIRECTION_is_not_a_spec_filter() -> None: assert narrowing_arguments("npm test -- --run >> test.log", scripts) == [] assert narrowing_arguments("npm test -- --run 2>&1", scripts) == [] assert narrowing_arguments("npm test -- --run > test.log 2>&1", scripts) == [] - # A real filter beside a redirection is still reported. + # A real filter beside a redirection is still reported, INCLUDING a numeric value: stripping + # redirections as a pre-pass ate the `2` in `--exclude 2 > log` by reading it as a descriptor. assert narrowing_arguments("npm test -- --run --exclude a.ts > log", scripts) == ["--exclude", "a.ts"] + assert narrowing_arguments("npm test -- --run --exclude 2 > log", scripts) == ["--exclude", "2"] + assert narrowing_arguments("npm test -- --run 2 --exclude a.ts", scripts) == ["2", "--exclude", "a.ts"] def test_the_flags_that_move_the_COLLECTED_SET_count_as_filters() -> None: