diff --git a/docs/decisions/records/testing/full-replace-asserts-field-list.md b/docs/decisions/records/testing/full-replace-asserts-field-list.md index 4d5cf4602..b49c9fecc 100644 --- a/docs/decisions/records/testing/full-replace-asserts-field-list.md +++ b/docs/decisions/records/testing/full-replace-asserts-field-list.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: 'Any path that writes a WHOLE entity or a WHOLE child collection — a PUT-replace handler, a hand-built request object, a test comparer standing in for one — derives its field list from the authoritative type and asserts SET EQUALITY against it, rather than enumerating the fields by hand. A hand-written list is correct on the day it is written and structurally unable to report the day it stops being: the field that drifts is the one nobody wrote a line for, so no amount of care in the existing lines can reach it. The failure is silent by construction — a full replace with a field omitted returns HTTP 200 and destroys that field''s value (#754 drifted from a 28-property DTO by one and cleared it; the symptom arrived hours later as missing pixels). SECOND CLAUSE, separable from the first: where a replaced child row carries state keyed to its identity — progression, ordering, an enumerator position — the handler RECONCILES BY ID rather than delete-and-reinsert, because reinsertion silently resets state a client never asked to touch (#252: a schedule PUT reset fill-group progression; #500: a dedup fix became permanent data loss because the add filter and the remove filter used different keys, so the two halves must agree on the key). Delete-and-reinsert is acceptable ONLY where no such state exists, and that emptiness is a fact about today''s schema that a later feature can silently invalidate — so record it where the handler is, dated, rather than leaving it to be re-derived. The canonical worked example is `ToolCatalogTests.Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`, which reads the accepted fields from the generated OpenAPI document and compares both directions. ON THE SPA SIDE the same rule is enforced by the TYPE SYSTEM rather than by a test: a full-replace body is built as `Complete` (`web/src/api/completeRequest.ts`), a mapped type that makes every member of a generated request type required, so a builder that omits one fails `npm run typecheck`. Annotate BOTH the API-wrapper parameter (so later callers inherit it) AND each construction site including every `.map` callback return type, because the excess-property check that catches a PHANTOM field fires only on a fresh literal in a contextually typed position and a generic `.map` callback is not one. Do not infer from "most builders already typecheck" that the gap is closed: a member is omittable exactly when it is absent from the schema `required` array in the ASP.NET-produced OpenAPI document, and two live cases (#807) sat unchecked inside a large majority of checked ones.' signals: 'full replace asserts its field list · hand-maintained mirror drifts by one field · reconcile by id not delete and reinsert · 200 and the field is gone · add filter and remove filter must share a key · fill-group progression reset by a PUT · derive the comparer from the DTO · a lossless round-trip test that is itself a hand-copied list · anti-vacuity PIN not a floor on a reflective walk · stale exemption must still name a real property · a complete comparer over a hand-written fixture · universal negative in a record is a trap · the SPA builds a full-replace body as `Complete` · a member is omittable only when it is absent from the OpenAPI `required` array · excess-property checking does not fire inside a generic `.map` callback · a comment warning about a silent reset is not a check · `weight` · `qsvPreferNativeDecoder` · paths: `ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs`, `web/src/api/completeRequest.ts`, `web/src/api/completeRequest.guard.test.ts` · issues: #807, #779, #773, #757, #754, #500, #252' -mechanics: 'Detector G of `docs/defect-shapes-773.md` §4. Enforced per-site by a reflective comparison over the DTO plus a written-down count pin, not by a repo-wide check — see the record body for why a global one is not proposed. The exemption set is empty today; the machinery guarding exemptions remains for the first one that earns its place.' +mechanics: 'Detector G of `docs/defect-shapes-773.md` §4. Enforced per-site by a reflective comparison over the DTO plus a written-down count pin, not by a repo-wide check — see the record body for why a global one is not proposed. The exemption set is empty today; the machinery guarding exemptions remains for the first one that earns its place. ON THE SPA SIDE the same rule is carried by the type system (`Complete`, `spa.full-replace-complete` in `docs/spa-conventions.md` §4b), and since ersatztv#820 its APPLICATION is checked rather than assumed: `web/src/api/completeAnnotations.guard.test.ts` derives the droppable schemas from the generated `v1.d.ts` and the annotations from the SPA AST, asserts set equality between the droppable SCHEMAS and a reviewed disposition table, requires a PRODUCTION annotation naming each schema DISPOSITIONED as needing one — three of the thirteen that can drop a member, the other ten being creates, triggers, read models and two MUST-NOT-ANNOTATE rows, each ruled on individually. A test-file annotation does not discharge that: `*.test.*`, `*.spec.*` and the one setup file named in `vite.config.ts` are excluded, and the setup path is pinned so a rename reddens. NOT covered, stated rather than implied — a SECOND `setupFiles` entry, which vitest would load every run; deriving that list would need another virtual-module plugin and is not proportionate to a three-site coordinated edit. The guard also forbids an annotation on the schemas where annotating is a bug, cross-checks its disposition values against this file''s table from the Python side, and is per-SCHEMA rather than per-site or per-wrapper — a wrapper-level rule was built and REMOVED before merge after four review rounds each defeated it. It also rejects a `Complete` over a hand-written mirror rather than a generated alias. Before it, `scripts/tests/test_optional_request_members.py`''s COVERED disposition ASSERTED the annotation existed and nothing verified it. Residual: the per-CONSTRUCTION-SITE population is not derived (sites in code, #777), so the phantom direction still rests on the §4b site rule and on review.' --- **This is `testing.guard-derives-population-from-source` applied to a write path, and it is worth its @@ -160,11 +160,16 @@ in two places: at the full-replace API-wrapper boundary, so later callers inheri it exists, and at each construction site, which is what keeps TypeScript's excess-property check (the *phantom* direction) alive. -That is the rule, not a claim about how much of the tree currently follows it. Nothing enforces the -second half — #820 — so any sentence here asserting present coverage would be falsifiable by one -`tsc` run and would go stale on the next screen anyone adds. Two wrappers annotated under #807 -initially shipped with unannotated construction sites for exactly that reason. To find out what is -actually annotated, read the code; do not read a count here. +That is the rule, not a claim about how much of the tree currently follows it. The two halves are +enforced UNEQUALLY, and the split is the thing to know. Since #820 the SCHEMA half is checked: +`web/src/api/completeAnnotations.guard.test.ts` derives the droppable schemas from the generated +types and the annotations from the SPA AST, and fails when a schema that can drop a member is named +by no PRODUCTION `Complete<…>`. It is per-SCHEMA: it catches the annotations being deleted, not one +being moved off an API wrapper onto some other production file. The +CONSTRUCTION-SITE half is still enforced by review only — that population is sites in code (#777). +So a count of annotated sites here would still be falsifiable by one `tsc` run and would go stale on +the next screen anyone adds. To find out what is actually annotated, read the code; do not read a +count here. **Why the second half of the rule is the one that gets skipped.** The wrapper annotation catches a MISSING member anywhere. The PHANTOM direction — a field the schema does not accept — relies on @@ -247,11 +252,26 @@ contingency is now MONITORED — the derived test above reddens the moment one o optional member — instead of being assumed. Annotating the rest is cheap and should happen when each is next touched. -**Residue, named rather than implied closed.** `Complete` proves its own semantics but NOT that -it is applied: reverting one screen to its pre-#807 form leaves the guard green, because the -population of construction sites is derived from nothing. The repo already owns the tool for that -— `web/src/api/pageSizeScan.ts` (TypeScript compiler API) plus a registry cross-checked in both -directions — and building it is tracked in #820. Until then this convention is enforced by review, -which is the weaker thing this record exists to warn about. Separately: `Complete` is shallow, -so a new nested item request type needs its own annotation; and requiring a field to be *named* is -not the same as requiring it to be *populated* correctly. +**Residue, named rather than implied closed.** #820 built the half that had an enumerable +population: `web/src/api/completeAnnotations.guard.test.ts` derives the droppable schemas from the +generated `v1.d.ts` and the `Complete<…>` annotations from the SPA AST, so deleting every production +annotation for a schema dispositioned as needing one now reddens. It found one live hole on landing: two +`playouts.ts` request types were hand-written mirrors SHADOWING generated schemas of the same name, +so their `Complete<>` was checking a local copy rather than the contract. + +What is NOT closed: the per-CONSTRUCTION-SITE population, which is sites in code (#777), so the +PHANTOM direction still rests on review, as does the wrapper half of the rule. A test-file annotation +cannot discharge the obligation in any spelling vitest COLLECTS (`*.test.*`, `*.spec.*`) nor from the +one setup file `vite.config.ts` names, whose path is pinned — but a SECOND `setupFiles` entry would, +and that direction is open — +but a screen that builds a body inline without annotating its `.map` callback is still only caught by +a reader, and so is an annotation moved off a wrapper. A guard that DID enforce the wrapper half was +built for #820 and removed before merge: four review rounds each found a fresh way past it, the last +being an `export function` to `export const` refactor invisible to both the AST scan and its +cross-check at once. Five defects from one mechanism, so the mechanism went. +Separately: `Complete` is shallow, so a new nested item request type needs its own annotation. A +DEEP variant was probed under #820 and works against the real generated types, but it was rejected: +it cannot be applied blanket-wise, because `UpdateChannelRequest.logo` reaches +`ArtworkContentTypeModel`, where annotating would force a caller to fabricate server-computed +values — and it closes only the missing direction, not the phantom one. Finally, requiring a field +to be *named* is not the same as requiring it to be *populated* correctly. diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index a9448242b..0cb6b17ef 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -64,12 +64,14 @@ with have sat in these gaps. Treat anything not named here as unassessed, not as the check proves each job was classified, never that the classification is CORRECT; and an `inline` assertion cannot be proven to work without running the job, so those rows carry `Proof: NONE` honestly instead of borrowing a neighbouring file's proof. -2. **C# and TypeScript guards** — six files, none with a row: +2. **C# and TypeScript guards** — seven files, none with a row: `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs` (ersatztv#824 — derives the `ISearchIndex` population from the declaring assembly and asserts set equality against a hand-written covered set), `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `web/src/api/pageSizeCallSites.guard.test.ts`, - `web/src/api/completeRequest.guard.test.ts`, and the pair + `web/src/api/completeRequest.guard.test.ts`, `web/src/api/completeAnnotations.guard.test.ts` + (added by ersatztv#820 — the second time this hand-written list has had to be extended by hand), + 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 @@ -170,6 +172,7 @@ The audit #806 asked for, recorded whichever way it came out, because "we looked | `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 | | `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 | | `test_ci_dropped_step_guard.py` | the parsed workflow document (its OWN subject only — the release-path `scan` job runs it in a non-git copy, so it may not derive from the index) | **no filesystem population at all** | unchanged by #806; the SCOPE residual it left open — `MARKED_JOBS` as a hand-written mirror of the required contexts on `main` — was closed by #787, which derives it from `.gitea/required-status-contexts.json` and reconciles that snapshot against the server in `scripts/check-required-contexts.sh`. The cross-workflow half of that claim lives in `test_ci_status_context_uniqueness.py`, which CAN use the index | @@ -235,6 +238,7 @@ recorded as unexamined rather than as cleared. | `scripts/tests/test_check_review_verdict.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_check_doc_narrative.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_ci_dropped_step_guard.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_complete_annotation_dispositions.py` | the `script-tests` job, when the SPA guard's `DISPOSITIONS` table disagrees with `test_optional_request_members.py` about a schema both rule on | GUARD | MUTATION | `test_complete_annotation_dispositions.py::test_the_two_dispositions_AGREE` | | `scripts/tests/test_ci_image_pin_population.py` | the `script-tests` job, when a container job loses its pin | GUARD | MUTATION | `test_ci_image_pin_population.py::test_a_single_job_losing_its_pin_is_DETECTED` | | `scripts/tests/test_ci_release_path_scan_job.py` | the `script-tests` job, on a weakened release-path scan job | GUARD | NONE | — | | `scripts/tests/test_ci_status_context_uniqueness.py` | the `script-tests` job, when two CI jobs synthesize the same status-check context (which branch protection cannot tell apart) | GUARD | MUTATION | `test_ci_status_context_uniqueness.py::test_no_two_jobs_synthesize_the_SAME_status_context` | @@ -370,7 +374,7 @@ clause-provable and the entry is regraded. ## What the numbers say -44 guards, 6 tooling scripts, 22 proof files. **22 guards carry a mutation proof; 6 are +45 guards, 6 tooling scripts, 22 proof files. **23 guards carry a mutation proof; 6 are behaviour-only; 16 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/spa-conventions.md b/docs/spa-conventions.md index c20ac74a4..25a7b4dd4 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -514,6 +514,35 @@ and fails until each has a stated disposition. See `docs/decisions/records/testing/full-replace-asserts-field-list.md`, and `web/src/api/completeRequest.guard.test.ts` for the executed proof. +**What is machine-checked, exactly (#820).** +`web/src/api/completeAnnotations.guard.test.ts` scans the SPA with the TypeScript compiler API and +asserts, per SCHEMA: every schema that can silently drop a member and is dispositioned as needing an +annotation carries a `Complete<…>` naming it in some **production** file — a `*.test.*` or +`*.spec.*` file does not count, nor does the setup file `vite.config.ts` names (a SECOND +`setupFiles` entry would, and is a stated residual); the schemas where annotating would be a bug carry none; and every `Complete` resolves to +an alias of `components['schemas'][…]` rather than a hand-written mirror — annotating a mirror proves +only that the caller filled in the mirror, which is the #754 mechanism wearing the annotation meant to +prevent it. Its disposition table is cross-checked against +`scripts/tests/test_optional_request_members.py` by `test_complete_annotation_dispositions.py`, so the +two cannot quietly disagree about a schema they both rule on. + +**What is NOT checked — read this before assuming the guard has your back.** + +- **Neither of the two rules above is enforced per SITE.** The check is that *a* production + annotation naming the schema exists. Deleting the `Complete<…>` from an API wrapper stays green as + long as some other production file still names that schema. Write both annotations anyway; the + guard catches wholesale deletion, not relocation. +- **It is token presence, not liveness.** A `Complete` that nothing uses satisfies it. +- **The phantom direction is not checked at all.** That needs a fresh object literal in a + contextually typed position, which a generic `.map` callback's return is not — the population is + sites-in-code (#777). + +A guard that derived the write WRAPPERS and required the annotation on the wrapper parameter was +built for #820 and REMOVED before merge, after four review rounds each found a fresh way past it — +the last being an `export function` to `export const` refactor that removed real protection while the +scanner and its cross-check went blind together. Do not rebuild it without reading that history in +`docs/guard-inventory.md`; the cheap version of that idea does not work. + ## 5. Artwork rendering Render `item.artwork` / `item.poster` (or whatever the DTO field is named) **directly as an ` from MultiCollectionsScreen.toItemRequest then leaves every suite green with " + "#807's silent weight reset live again. The row is named in full rather than by the bare " + "disposition line, which occurs three times and would identify none of them.", + ), ) diff --git a/scripts/tests/test_complete_annotation_dispositions.py b/scripts/tests/test_complete_annotation_dispositions.py new file mode 100644 index 000000000..cc6370020 --- /dev/null +++ b/scripts/tests/test_complete_annotation_dispositions.py @@ -0,0 +1,158 @@ +"""ersatztv#820: the SPA guard's disposition table may not silently disagree with this one. + +WHAT THIS BLOCKS. `web/src/api/completeAnnotations.guard.test.ts` carries a `DISPOSITIONS` table +saying, per schema, whether the SPA must annotate it `Complete`, must not, or need not. It derives +its POPULATION (which schemas can drop a member) from the generated types and asserts set equality +against those keys — so a new optional member cannot ship undispositioned. + +It did not check the VALUE, and cold review demonstrated that the value is a kill switch: flip +`MultiCollectionItemRequest` from `ANNOTATED` to `CREATE` and delete the `Complete<…>` from +`MultiCollectionsScreen.toItemRequest`, and the whole suite stays green while the defect #807 exists +to prevent — every weight silently reset to 1 on save — is live again. The row's own note still said +"a prose comment warned about it, and a comment is not a check", which is what the flip made of it. + +WHY THE CHECK LIVES HERE. `testing.guard-derives-population-from-source`: "When the scope itself +MIRRORS an authoritative source, the mirror needs its own equality check against that source." +`test_optional_request_members.py` IS that source — it derives from `ErsatzTV/wwwroot/openapi/v1.json` +every run and its dispositions are reviewed. The TS table mirrors nine of its rows. Putting the +comparison on the Python side is what makes it cheap: this process already holds the authoritative +table and can read a file, whereas the SPA guard runs under Vite with no filesystem access to +`scripts/` and would need a virtual-module plugin to see it. + +THE MAPPING IS ASYMMETRIC ON PURPOSE, and stating why is half the check: + + COVERED -> ANNOTATED exact. This is the row whose claim the SPA guard verifies. + COMPUTED -> MUST-NOT-ANNOTATE exact. Annotating would force a caller to invent server-computed + values, so the prohibition must survive on both sides. + CREATE -> CREATE | MUST-NOT-ANNOTATE + TRIGGER -> TRIGGER | MUST-NOT-ANNOTATE + a STRICTER call on the SPA side is allowed, because forbidding an + annotation is never the unsafe direction. This is not hypothetical + slack: `CreateChannelFromLineupAdvancedOptionsRequest` is CREATE + here and MUST-NOT-ANNOTATE there, because an omitted override means + INHERIT the template value and `Complete` would collapse that + third state into an explicit null. + +Schemas the TS table holds that this one does not are NOT an error: that table scopes to every +generated schema with an optional member, this one to schemas reachable from a request body, so it +additionally sees response models. The reverse IS an error — a schema ruled on here and absent there +means the SPA guard is not covering something the API can drop. +""" + +from __future__ import annotations + +import importlib.util +import re +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +TS_GUARD = REPO_ROOT / "web" / "src" / "api" / "completeAnnotations.guard.test.ts" + +# `Name: {` then an optional run of `//` comment lines, then `disposition: '...'`. Anchored on the +# two-space indent the file's formatter produces for a top-level entry, so a `disposition:` appearing +# in prose or in a nested object cannot be mistaken for a row. +_TS_ROW = re.compile( + r"^ (?P\w+): \{\n(?:\s*//[^\n]*\n)*\s*disposition: '(?P[A-Z-]+)'", + re.MULTILINE, +) + +# The compact single-line form the formatter emits for short rows. +_TS_ROW_INLINE = re.compile(r"^ (?P\w+): \{ disposition: '(?P[A-Z-]+)'", re.MULTILINE) + +ALLOWED = { + "COVERED": {"ANNOTATED"}, + "COMPUTED": {"MUST-NOT-ANNOTATE"}, + "CREATE": {"CREATE", "MUST-NOT-ANNOTATE"}, + "TRIGGER": {"TRIGGER", "MUST-NOT-ANNOTATE"}, +} + + +# A key that is not a bare identifier does not name a generated schema: `test_optional_request_members` +# also rules on INLINE request bodies, which it keys by endpoint (`POST /api/v1/artwork/uploads +# (multipart/form-data inline body)`). Those have no `components['schemas'][…]` entry, so the SPA +# guard — whose population is parsed out of the generated types — cannot carry a row for them and +# their absence is not a hole. `_comparable` is what keeps that exemption from widening into one: it +# is a SHAPE test, not a name list, so a new inline body is exempt automatically while a new named +# schema can never be. +_SCHEMA_NAME = re.compile(r"^\w+$") + + +def _comparable(schema: str) -> bool: + return _SCHEMA_NAME.match(schema) is not None + + +def _python_dispositions() -> dict[str, str]: + spec = importlib.util.spec_from_file_location( + "_optional_request_members", Path(__file__).with_name("test_optional_request_members.py") + ) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return {schema: value[0] for schema, value in module.DISPOSITIONS.items()} + + +def _ts_dispositions() -> dict[str, str]: + text = TS_GUARD.read_text(encoding="utf-8") + found = {m.group("schema"): m.group("disposition") for m in _TS_ROW.finditer(text)} + found.update({m.group("schema"): m.group("disposition") for m in _TS_ROW_INLINE.finditer(text)}) + return found + + +def test_the_TS_table_is_parseable_at_all() -> None: + """Anti-vacuity, and the only thing standing between a formatting change and a silent no-op. + + Every comparison below is over an intersection. If the regex stops matching — a prettier setting, + a rename, a move — the intersection empties and every other test here passes having compared + nothing. This is the check that turns that into a failure. + """ + ts = _ts_dispositions() + assert len(ts) >= 10, ( + f"parsed only {len(ts)} rows out of {TS_GUARD.relative_to(REPO_ROOT)}; the table has more than " + f"that, so the parse is broken rather than the table being small. Every comparison in this " + f"file is over an intersection and would pass vacuously." + ) + assert set(ts.values()) <= {"ANNOTATED", "MUST-NOT-ANNOTATE", "CREATE", "TRIGGER", "RESPONSE-ONLY"} + + +def test_the_PYTHON_table_is_non_empty_too() -> None: + """The TS side has a floor; without one here the comparisons are still vacuous from the other end. + + Every check below iterates the Python table. An empty one collects zero parametrised cases and + makes the missing-schema check pass over an empty set — green, having compared nothing. The + neighbour file has its own floor, but a guard that depends on a neighbour's floor for its own + anti-vacuity is borrowing the property rather than holding it. + """ + assert len(_python_dispositions()) >= 5 + + +def test_the_inline_body_exemption_stays_small_and_shaped() -> None: + """The exemption above is a shape test; this bounds how much it can be carrying.""" + exempt = sorted(s for s in _python_dispositions() if not _comparable(s)) + assert len(exempt) <= 2, f"more inline-body keys than expected: {exempt}" + assert all(" " in s or "/" in s for s in exempt), f"an exempt key looks like a schema name: {exempt}" + + +def test_every_schema_ruled_on_HERE_is_also_ruled_on_in_the_SPA_guard() -> None: + missing = sorted(s for s in _python_dispositions() if _comparable(s) and s not in _ts_dispositions()) + assert not missing, ( + f"schema(s) dispositioned in test_optional_request_members.py with no row in the SPA guard: " + f"{missing}. The API can drop a member of each, and the guard that checks the SPA's " + f"annotations is not covering it." + ) + + +@pytest.mark.parametrize("schema", sorted(s for s in _python_dispositions() if _comparable(s))) +def test_the_two_dispositions_AGREE(schema: str) -> None: + """The bypass this closes: the TS value alone decided whether an annotation was required.""" + python_disposition = _python_dispositions()[schema] + ts = _ts_dispositions() + if schema not in ts: + pytest.skip("absence is asserted separately, so it is not re-reported per schema here") + allowed = ALLOWED[python_disposition] + assert ts[schema] in allowed, ( + f"{schema}: this file says {python_disposition}, the SPA guard says {ts[schema]}, and " + f"{python_disposition} admits only {sorted(allowed)}. Flipping the SPA-side value is how a " + f"required `Complete<…>` annotation stops being required — decide which table is wrong." + ) diff --git a/scripts/tests/test_optional_request_members.py b/scripts/tests/test_optional_request_members.py index ea54191a2..37e6eb10f 100644 --- a/scripts/tests/test_optional_request_members.py +++ b/scripts/tests/test_optional_request_members.py @@ -18,8 +18,14 @@ endpoint does: Two misses from one mechanism, so the mechanism goes rather than the list getting a third patch. `testing.guard-derives-population-from-source` is explicit that a hand-written list is "a filter frozen at authoring time, correct on the day it was written and unable to report the day it stopped -being" — and unlike #820's population (sites in code, which needs compiler-API tooling), THIS -population has an authoritative machine-readable source: the OpenAPI document. +being" — and unlike the per-CONSTRUCTION-SITE population (sites in code, which #777 tracks), THIS +population has an authoritative machine-readable source: the OpenAPI document. #820 closed the +SCHEMA-level half of that gap the same way, from the generated types rather than the sites: +`web/src/api/completeAnnotations.guard.test.ts` checks that the SPA actually annotates what a +disposition here CLAIMS it annotates. The two tables are independent and can drift — that one scopes +to every generated schema with an optional member, this one to schemas reachable from a request body +— so neither reddens when the other is re-dispositioned. What neither can miss is a schema unnoticed +by BOTH, since both derive from the same `required` arrays. SCOPE vs POPULATION, per that same record. The POPULATION — which schemas can drop a member on a write — is DERIVED here, every run, from `ErsatzTV/wwwroot/openapi/v1.json`. The DISPOSITIONS below @@ -55,7 +61,11 @@ OPENAPI = REPO_ROOT / "ErsatzTV" / "wwwroot" / "openapi" / "v1.json" # --- the closed disposition vocabulary ------------------------------------------------------- # # COVERED the SPA builds this body and the builder is annotated `Complete`, so omitting a -# member is a typecheck error (`web/src/api/completeRequest.ts`). +# member is a typecheck error (`web/src/api/completeRequest.ts`). Since ersatztv#820 that +# is CHECKED rather than asserted: `web/src/api/completeAnnotations.guard.test.ts` fails +# when no `Complete<…>` in the SPA names a schema dispositioned here as COVERED. Until +# then this row was a claim about another language's source that nothing verified, so a +# COVERED row and a SPA with the annotation deleted were indistinguishable. # CREATE a POST that creates a new entity, where an omitted member correctly means "use the # default". Annotating it would be a BUG, not coverage — see the from-lineup note. # TRIGGER the body parameterises an ACTION and replaces no stored entity, so there is nothing to @@ -63,7 +73,8 @@ OPENAPI = REPO_ROOT / "ErsatzTV" / "wwwroot" / "openapi" / "v1.json" # COMPUTED the optional members are get-only computed properties on the C# record. System.Text.Json # never deserializes them, so the client cannot drop a stored value by omitting them — # and `Complete` must NOT be applied here, because it would force a caller to invent -# server-computed values in an outbound request. +# server-computed values in an outbound request. The prohibition is executable since +# ersatztv#820, asserted as an ABSENCE by the same TS guard. COVERED = "COVERED" CREATE = "CREATE" TRIGGER = "TRIGGER" diff --git a/web/src/api/completeAnnotationScan.test.ts b/web/src/api/completeAnnotationScan.test.ts new file mode 100644 index 000000000..8b56998ab --- /dev/null +++ b/web/src/api/completeAnnotationScan.test.ts @@ -0,0 +1,205 @@ +import { describe, expect, it } from 'vitest'; +import { + declaresLocalComplete, + scanCompleteAnnotations, + scanOptionalSchemaMembers, + scanSchemaAliases +} from './completeAnnotationScan'; + +/** + * Fixture proofs for `completeAnnotationScan.ts` (ersatztv#820), over SYNTHETIC source strings. + * + * Split from `completeAnnotations.guard.test.ts` for the reason `pageSizeScan.test.ts` is split + * from its guard: the guard asserts over today's repo, so it can only witness an input class that + * happens to be committed right now. A scanner defect for a shape the repo does not currently + * contain — a `Complete<…>` inside a template literal, say — would ship green and start mattering + * the day someone writes one. These pin the scanner's behaviour per INPUT CLASS instead. + */ + +describe('scanCompleteAnnotations', () => { + it('finds a Complete<…> in a parameter type position', () => { + const source = 'export function f(body: Complete): void {}'; + expect(scanCompleteAnnotations(source, 'a.ts')).toEqual([{ line: 1, column: 25, typeName: 'ReplaceBlockRequest' }]); + }); + + it('finds a Complete<…> in a return type and in a variable annotation', () => { + const source = ['const x: Complete = y;', 'function g(): Complete { return z; }'].join('\n'); + expect(scanCompleteAnnotations(source, 'a.ts').map((a) => `${a.line}:${a.typeName}`)).toEqual(['1:A', '2:B']); + }); + + it('finds the annotation on a .map callback return type, the form §4b says gets skipped', () => { + const source = 'const r = items.map((i): Complete => ({ ...i }));'; + expect(scanCompleteAnnotations(source, 'a.ts').map((a) => a.typeName)).toEqual(['BlockItemRequest']); + }); + + // The three exclusions below are why this is a compiler-API scan and not a regex. Each is a + // DIFFERENT node kind, so none of them is a `TypeReferenceNode` and none needs special-casing. + it('does NOT match Complete<…> written inside a comment', () => { + const source = ['/** Build the body as Complete before sending. */', 'const a = 1;'].join('\n'); + expect(scanCompleteAnnotations(source, 'a.ts')).toEqual([]); + }); + + it('does NOT match Complete<…> inside a string or template literal', () => { + const source = ['const s = "Complete";', 'const t = `Complete`;'].join('\n'); + expect(scanCompleteAnnotations(source, 'a.ts')).toEqual([]); + }); + + it('does NOT match the import specifier that brings Complete into scope', () => { + const source = "import type { Complete } from './completeRequest';"; + expect(scanCompleteAnnotations(source, 'a.ts')).toEqual([]); + }); + + it('does NOT record a Complete reference with no type argument or with two', () => { + // Neither compiles — `Complete` takes exactly one parameter — so recording them would put a + // shape the compiler already rejects into a set the guard asserts set-equality over. + const source = ['let a: Complete;', 'let b: Complete;'].join('\n'); + expect(scanCompleteAnnotations(source, 'a.ts')).toEqual([]); + }); + + it('records the type argument VERBATIM rather than flattening it to an identifier', () => { + const source = "let a: Complete;"; + expect(scanCompleteAnnotations(source, 'a.ts').map((a) => a.typeName)).toEqual(["components['schemas']['X']"]); + }); + + it('parses a .tsx file with JSX rather than choking on the angle brackets', () => { + const source = ['const el =
;', 'export const f = (b: Complete) => b;'].join('\n'); + expect(scanCompleteAnnotations(source, 'a.tsx').map((a) => a.typeName)).toEqual(['A']); + }); + + it('reports sites in source order', () => { + const source = ['let b: Complete;', 'let a: Complete;'].join('\n'); + expect(scanCompleteAnnotations(source, 'a.ts').map((a) => a.typeName)).toEqual(['B', 'A']); + }); +}); + +describe('scanSchemaAliases', () => { + it('resolves an exported components schema alias', () => { + const source = "export type ReplaceBlockRequest = components['schemas']['ReplaceBlockRequest'];"; + expect(scanSchemaAliases(source, 'a.ts')).toEqual([ + { localName: 'ReplaceBlockRequest', schemaName: 'ReplaceBlockRequest' } + ]); + }); + + it('resolves an alias whose LOCAL name differs from the schema name', () => { + // The case that makes resolution worth deriving rather than assuming: `Block` is not + // `BlockResponseModel`, and a guard that matched on name equality would silently miss it. + const source = "export type Block = components['schemas']['BlockResponseModel'];"; + expect(scanSchemaAliases(source, 'a.ts')).toEqual([{ localName: 'Block', schemaName: 'BlockResponseModel' }]); + }); + + it('ignores an indexed-access alias that is not into components.schemas', () => { + const source = "type A = Foo['bar'];\ntype B = components['responses']['X'];"; + expect(scanSchemaAliases(source, 'a.ts')).toEqual([]); + }); + + it('ignores a HAND-WRITTEN object type, which is the shape the guard rejects', () => { + const source = 'export type ReplacePlayoutTemplatesRequest = { items: PlayoutTemplateItemRequest[] };'; + expect(scanSchemaAliases(source, 'a.ts')).toEqual([]); + }); +}); + +describe('scanOptionalSchemaMembers', () => { + const dts = [ + 'export interface components {', + ' schemas: {', + ' "A": {', + ' "req": number;', + ' "opt"?: string;', + ' };', + ' "B": {', + ' "one": boolean;', + ' };', + ' };', + '}' + ].join('\n'); + + it('records only the OPTIONAL members, per schema', () => { + const parsed = scanOptionalSchemaMembers(dts); + expect(parsed.get('A')).toEqual(['opt']); + expect(parsed.get('B')).toEqual([]); + }); + + it('records every schema, so a schema with no optional member is present-and-empty rather than absent', () => { + // The guard filters on `optionals.length > 0`. If a no-optionals schema were simply missing, + // that filter would be indistinguishable from a parse that dropped it. + expect([...scanOptionalSchemaMembers(dts).keys()].sort()).toEqual(['A', 'B']); + }); + + it('returns nothing for a d.ts with no schemas block rather than throwing', () => { + // The guard's own anti-vacuity floor is what turns an empty parse into a failure; the scanner + // returning empty is the input to that check, not a silent pass on its own. + expect(scanOptionalSchemaMembers('export interface components { paths: {} }').size).toBe(0); + }); +}); + +describe('scanOptionalSchemaMembers — unreadable shapes FAIL rather than silently shrinking the population', () => { + const wrap = (schemas: string) => `export interface components {\n schemas: {\n${schemas}\n };\n}`; + + it('throws on a duplicate schema declaration instead of letting the later one win', () => { + expect(() => scanOptionalSchemaMembers(wrap(' "A": { "x"?: string };\n "A": { "y": number };'))).toThrow(/declared twice/); + }); + + it('throws on an optional member with a computed key rather than dropping it', () => { + expect(() => scanOptionalSchemaMembers(wrap(' "A": { ["computed"]?: string };'))).toThrow(/computed key/); + }); + + // Every shape below was MEASURED hiding members by cold review, against the real scanner, after an + // earlier fix threw on intersections alone and claimed in a comment that `allOf` was "the one + // non-object shape that can hide members". `generateTypes` emits a union for `oneOf`/`anyOf` and + // for a plain nullable object (`type: ["object","null"]`), so none of these is hypothetical. + it.each([ + ['intersection (allOf)', ' "A": ComponentA & ComponentB;'], + ['union of refs (oneOf)', ' "A": ComponentA | ComponentB;'], + ['union of object literals', ' "A": { "x"?: string } | { "y"?: number };'], + ['a $ref alias', ' "A": components["schemas"]["B"];'], + ['an array shorthand', ' "A": { "x"?: string }[];'], + // `typeFromSchema` emits `Array` (a TypeReference), not `T[]`; pin the shape the generator + // actually produces rather than only the one that reads like an array. + ['an Array as the generator emits it', ' "A": Array;'] + ])('throws on %s rather than dropping its members', (_label, schema) => { + expect(() => scanOptionalSchemaMembers(wrap(schema))).toThrow(/cannot enumerate/); + }); + + it.each([ + ['a string enum', ' "A": "x" | "y";'], + ['a single literal', ' "A": "only";'], + ['a bare keyword', ' "A": string;'], + ['a nullable enum', ' "A": null | "x";'] + ])('still skips %s quietly, which has no members to drop', (_label, schema) => { + expect(scanOptionalSchemaMembers(wrap(schema)).size).toBe(0); + }); + + it('ignores a `schemas` member on an interface that is not `components`', () => { + // The unconstrained form let a `declare module` block overwrite a real schema with an empty + // optional list — a droppable schema removed from the population with nothing reddening. + const noise = 'declare module "n" { interface Other { schemas: { "A": { "y": number } } } }'; + const parsed = scanOptionalSchemaMembers(`${wrap(' "A": { "x"?: string };')}\n${noise}`); + expect(parsed.get('A')).toEqual(['x']); + }); +}); + +describe('declaresLocalComplete', () => { + it('detects a local type alias, an interface, and an aliased import binding the name', () => { + expect(declaresLocalComplete('type Complete = T;', 'a.ts')).toBe(true); + expect(declaresLocalComplete('interface Complete { x?: keyof T }', 'a.ts')).toBe(true); + expect(declaresLocalComplete("import { Other as Complete } from './x';", 'a.ts')).toBe(true); + }); + + it('does NOT fire on the real import, which binds the mapped type itself', () => { + expect(declaresLocalComplete("import type { Complete } from './completeRequest';", 'a.ts')).toBe(false); + }); + + it('does NOT fire on the words appearing inside a comment or a string', () => { + // The regression that moved this off a regex: this repo's own fixture file contains + // `Build the body as Complete<…>` inside a quoted synthetic source, and a text predicate + // reported the whole file as shadowing. + expect(declaresLocalComplete('/** the body as Complete before sending */\nconst a = 1;', 'a.ts')).toBe(false); + expect(declaresLocalComplete("const s = 'type Complete = T;';", 'a.ts')).toBe(false); + }); +}); + +describe('declaresLocalComplete — the default-import form', () => { + it('detects a default import binding the name Complete', () => { + expect(declaresLocalComplete("import Complete from './x';", 'a.ts')).toBe(true); + }); +}); diff --git a/web/src/api/completeAnnotationScan.ts b/web/src/api/completeAnnotationScan.ts new file mode 100644 index 000000000..9a6f08759 --- /dev/null +++ b/web/src/api/completeAnnotationScan.ts @@ -0,0 +1,328 @@ +import * as ts from 'typescript'; + +/** + * ersatztv#820 — AST scanners that let a guard check where `Complete` is APPLIED, not only what + * it means. + * + * ## The gap this exists to close + * + * `completeRequest.guard.test.ts` proves the TYPE's semantics: `Complete` rejects a missing + * member and a phantom one. It proves nothing about application. Separately, + * `scripts/tests/test_optional_request_members.py` derives — from `ErsatzTV/wwwroot/openapi/v1.json`, + * every run — the set of schemas that can silently drop a member on a write, and files each under a + * disposition. One of those dispositions, `COVERED`, is worded "the SPA builds this body and the + * builder is annotated `Complete`". + * + * That sentence is an ASSERTION, not a check. Nothing verified it. A `COVERED` row and a SPA with + * the annotation deleted are indistinguishable to every test in the repo, which is the + * `testing.guard-derives-population-from-source` shape one level up: the population of schemas is + * derived and the population of ANNOTATIONS is not, so the registry can claim coverage it does not + * have. `docs/spa-conventions.md` §4b's opposite rule — do NOT annotate a schema whose optional + * members are computed server-side — was prose only for the same reason. + * + * ## Why the compiler API and not a string match + * + * `Complete` is a narrow token, so a regex looks adequate here in a way it did not for + * `check-doc-narrative`. It is not: the text appears in doc comments (this file's own comment + * block says `Complete` a dozen times), in string literals, and in `import type { Complete }`. + * A `TypeReferenceNode` is a structurally different AST node from all three, so they are excluded + * by NODE KIND rather than by a preceding-character heuristic — the same argument + * `pageSizeScan.ts` makes, and this repo has a withdrawn guard (six review rounds, then deleted) + * recording what happens when a source-shaped predicate is built out of string matching instead. + * + * ## The alias indirection, and why it is resolved rather than assumed + * + * The SPA never writes `Complete`. Each `web/src/api/*.ts` module + * re-exports the schema under a local alias — `export type ReplaceBlockRequest = + * components['schemas']['ReplaceBlockRequest']` — and screens import that. The alias name usually + * equals the schema name, and assuming that would work today; it is derived anyway, because the + * assumption is exactly the kind that holds until someone renames one alias and the guard silently + * stops matching a schema it believes it is covering. + * + * ## What this resolution does NOT follow, and which way each one fails + * + * Written down because "resolved" would otherwise read as complete. None of these is silent: each + * yields an annotation the guard cannot resolve, and an unresolved annotation FAILS. + * + * - A renamed import or re-export (`import type { X as Y }`, `export type { X as Y }`) is not + * followed, so `Complete` is unresolved — a false RED, which is noisy but safe. + * - A nested generic (`Complete>`) is recorded verbatim and matches no alias — also a false + * RED. + * - The alias map is built across the whole population and would otherwise be last-write-wins, so + * two modules mapping the same local name to different schemas could credit the wrong one. That + * is a false GREEN, so it is not tolerated: the guard rejects a colliding local name outright + * rather than resolving it. + * - `Complete` is matched by NAME. A file declaring its own `type Complete = T` would make every + * annotation in it an identity type while still reading as covered — the one genuinely dangerous + * gap, closed by asserting no such declaration exists rather than by resolving the symbol through + * a full `Program` + type checker, which this scanner deliberately does not build. + */ + +/** A `Complete` type reference, as written. */ +export interface CompleteAnnotation { + /** 1-based source line of the `Complete` reference, matching editor line numbers. */ + line: number; + /** 1-based source column. */ + column: number; + /** + * The type argument as WRITTEN — the local alias (`ReplaceBlockRequest`), not the schema it + * resolves to. Resolution is the caller's job, so that an UNRESOLVABLE alias stays visible as + * itself rather than being silently dropped here. + */ + typeName: string; +} + +/** A `export type X = components['schemas']['Y']` alias declaration. */ +export interface SchemaAlias { + /** The local name the SPA writes, e.g. `ReplaceBlockRequest`. */ + localName: string; + /** The schema key it indexes, e.g. `ReplaceBlockRequest`. */ + schemaName: string; +} + +function scriptKindFor(fileName: string): ts.ScriptKind { + // `.mts`/`.cts` parse as plain TS; only `.tsx` needs the JSX grammar. Same rule as + // `pageSizeScan.ts` — stated here rather than shared, because a helper on both sides of a + // comparison cancels out of it (the #819 round-5 finding recorded in the pageSize guard). + return fileName.endsWith('.tsx') ? ts.ScriptKind.TSX : ts.ScriptKind.TS; +} + +function parse(sourceText: string, fileName: string): ts.SourceFile { + return ts.createSourceFile(fileName, sourceText, ts.ScriptTarget.Latest, true, scriptKindFor(fileName)); +} + +/** + * Every `Complete` TYPE REFERENCE in one source file. + * + * A reference with no type argument, or with more than one, is NOT recorded: `Complete` takes + * exactly one parameter, so those forms do not compile, and recording them would put a shape the + * compiler already rejects into a set the guard asserts over. + */ +export function scanCompleteAnnotations(sourceText: string, fileName: string): CompleteAnnotation[] { + const sourceFile = parse(sourceText, fileName); + const found: CompleteAnnotation[] = []; + + function visit(node: ts.Node): void { + if (ts.isTypeReferenceNode(node) && ts.isIdentifier(node.typeName) && node.typeName.text === 'Complete') { + if (node.typeArguments?.length === 1) { + const argument = node.typeArguments[0]; + const { line, character } = sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)); + found.push({ + line: line + 1, + column: character + 1, + // `getText()` on a type node returns the argument verbatim, so a nested or qualified form + // is preserved as written instead of being flattened into an identifier that was never + // there. + typeName: argument.getText(sourceFile).trim() + }); + } + } + ts.forEachChild(node, visit); + } + + visit(sourceFile); + return found.sort((a, b) => (a.line === b.line ? a.column - b.column : a.line - b.line)); +} + +/** + * Every `type X = components['schemas']['Y']` alias in one source file. + * + * Both the exported and the local form are recorded. Only a STRING-literal index is resolvable; a + * computed one (`components['schemas'][someKey]`) names no fixed schema and is correctly skipped, + * which is why the guard asserts that every annotation resolves rather than assuming this returns + * everything. + */ +export function scanSchemaAliases(sourceText: string, fileName: string): SchemaAlias[] { + const sourceFile = parse(sourceText, fileName); + const found: SchemaAlias[] = []; + + function visit(node: ts.Node): void { + if (ts.isTypeAliasDeclaration(node) && ts.isIndexedAccessTypeNode(node.type)) { + const outer = node.type; + const schemaKey = outer.indexType; + const container = outer.objectType; + // Shape: components['schemas']['Name'] — an indexed access whose OBJECT is itself an indexed + // access keyed 'schemas'. Checking the inner 'schemas' key is what keeps an unrelated + // `Foo['bar']` alias out of the map. + if ( + ts.isLiteralTypeNode(schemaKey) && + ts.isStringLiteral(schemaKey.literal) && + ts.isIndexedAccessTypeNode(container) && + ts.isLiteralTypeNode(container.indexType) && + ts.isStringLiteral(container.indexType.literal) && + container.indexType.literal.text === 'schemas' + ) { + found.push({ localName: node.name.text, schemaName: schemaKey.literal.text }); + } + } + ts.forEachChild(node, visit); + } + + visit(sourceFile); + return found; +} + +/** + * Schema name → the names of its OPTIONAL members, read off the generated `v1.d.ts`. + * + * The generated file is the artifact the SPA actually compiles against, and + * `web/scripts/generate-openapi-types.mjs` is a pure pass-through of the OpenAPI `required` array + * (`const optional = required.has(name) ? '' : '?'`). So "has an optional member here" and "sits + * outside `required` in `v1.json`" are the same fact read at different points, NOT a second + * derivation that could disagree with the Python guard's. + */ +/** + * A schema type with no members to drop: a string/number enum, a single literal, or a bare keyword. + * + * The generated file holds 54 of these today (52 unions of string literals, one literal, one + * `IFormFile`-shaped keyword). They are skipped rather than thrown on because there is nothing in + * them a builder could omit — which is a property this predicate CHECKS, not one the caller asserts. + */ +function isEnumShaped(type: ts.TypeNode): boolean { + if (ts.isLiteralTypeNode(type)) { + return true; + } + if (ts.isUnionTypeNode(type)) { + // One level deep in practice: TS flattens `A | B | C` into a single node, and a parenthesised + // group parses as a `ParenthesizedTypeNode`, which the keyword check below rejects — so nesting + // is refused rather than mis-admitted. The recursion is what lets each MEMBER be judged on its + // own kind instead of the union being waved through as a whole. + return type.types.every((member) => isEnumShaped(member)); + } + return ( + type.kind === ts.SyntaxKind.StringKeyword || + type.kind === ts.SyntaxKind.NumberKeyword || + type.kind === ts.SyntaxKind.BooleanKeyword || + type.kind === ts.SyntaxKind.UnknownKeyword || + type.kind === ts.SyntaxKind.AnyKeyword + ); +} + +export function scanOptionalSchemaMembers(dtsText: string): Map { + const sourceFile = parse(dtsText, 'v1.d.ts'); + const optionalsBySchema = new Map(); + + function memberName(member: ts.TypeElement): string | undefined { + if (!member.name) { + return undefined; + } + if (ts.isIdentifier(member.name) || ts.isStringLiteral(member.name)) { + return member.name.text; + } + return undefined; + } + + function visit(node: ts.Node): void { + // Constrained to `interface components { schemas: … }` at the TOP LEVEL of the file, not "any + // property named `schemas` anywhere". Cold review demonstrated the unconstrained form: a + // `declare module` block elsewhere carrying its own `schemas` member silently OVERWROTE a real + // schema's optional list with an empty one, which removes a droppable schema from the + // population without reddening anything. + if (!ts.isInterfaceDeclaration(node) || node.name.text !== 'components') { + ts.forEachChild(node, visit); + return; + } + for (const member of node.members) { + if (!ts.isPropertySignature(member) || memberName(member) !== 'schemas') { + continue; + } + if (!member.type || !ts.isTypeLiteralNode(member.type)) { + throw new Error('completeAnnotationScan: `components.schemas` is not a type literal — the generated format changed.'); + } + for (const schema of member.type.members) { + if (!ts.isPropertySignature(schema)) { + continue; + } + const name = memberName(schema); + if (name === undefined) { + // A computed key names a schema this scanner cannot read. Skipping it would drop a + // droppable schema from the population silently; UNKNOWN must fail rather than pass. + throw new Error( + `completeAnnotationScan: a schema key at position ${schema.getStart(sourceFile)} is not a plain or quoted name.` + ); + } + if (schema.type && !ts.isTypeLiteralNode(schema.type) && !isEnumShaped(schema.type)) { + // Anything that is neither an object literal nor enum-shaped may CARRY members this + // scanner cannot enumerate, and skipping it drops them from the droppable population with + // nothing reddening. An earlier version threw on intersections only, on the stated ground + // that `allOf` was "the one non-object shape that can hide members". Review measured that + // false by execution: a union of refs, a union of object literals, a `$ref` alias and an + // array each hid members too, and `generateTypes` emits a union for `oneOf`/`anyOf` AND for + // a plain nullable object (`type: ["object","null"]`). So the test is now positive — is it + // a shape we can enumerate, or one we have shown carries nothing — rather than a list of + // shapes to reject, which is the enumeration that was already wrong once. + throw new Error( + `completeAnnotationScan: schema \`${name}\` has a type this scanner cannot enumerate ` + + `(kind ${ts.SyntaxKind[schema.type.kind]}). Teach it the shape rather than letting the ` + + `schema leave the droppable population silently.` + ); + } + if (!schema.type || !ts.isTypeLiteralNode(schema.type)) { + continue; + } + if (optionalsBySchema.has(name)) { + throw new Error(`completeAnnotationScan: schema \`${name}\` is declared twice; the later one would silently win.`); + } + const optionals: string[] = []; + for (const m of schema.type.members) { + if (!ts.isPropertySignature(m) || m.questionToken === undefined) { + continue; + } + const memberKey = memberName(m); + if (memberKey === undefined) { + throw new Error( + `completeAnnotationScan: schema \`${name}\` has an OPTIONAL member with a computed key, which this scanner cannot read.` + ); + } + optionals.push(memberKey); + } + optionalsBySchema.set(name, optionals); + } + } + } + + visit(sourceFile); + return optionalsBySchema; +} + +/** + * Whether this file BINDS the name `Complete` to something of its own. + * + * `scanCompleteAnnotations` matches `Complete` by name, so a file declaring `type Complete = T` + * (or an `interface`, or importing something else under that alias) would read as fully covered + * while every annotation in it was an identity type — the one genuinely dangerous gap in a + * name-based match. + * + * Done with the AST rather than a regex over the text, and not for symmetry: the regex version + * false-positived on this repo's own fixture file, which contains `type Complete = T` inside a + * synthetic source STRING. A string predicate cannot tell a declaration from a quoted example, + * which is the argument this whole module is built on. + */ +export function declaresLocalComplete(sourceText: string, fileName: string): boolean { + const sourceFile = parse(sourceText, fileName); + let found = false; + + function visit(node: ts.Node): void { + if (found) { + return; + } + if ((ts.isTypeAliasDeclaration(node) || ts.isInterfaceDeclaration(node)) && node.name.text === 'Complete') { + found = true; + return; + } + // `import { Something as Complete }` — the local binding is `Complete`, the imported name is not. + if (ts.isImportSpecifier(node) && node.name.text === 'Complete' && node.propertyName !== undefined) { + found = true; + return; + } + // `import Complete from './x'` — a default import binding the same name. + if (ts.isImportClause(node) && node.name?.text === 'Complete') { + found = true; + return; + } + ts.forEachChild(node, visit); + } + + visit(sourceFile); + return found; +} diff --git a/web/src/api/completeAnnotations.guard.test.ts b/web/src/api/completeAnnotations.guard.test.ts new file mode 100644 index 000000000..ce8170094 --- /dev/null +++ b/web/src/api/completeAnnotations.guard.test.ts @@ -0,0 +1,469 @@ +import { describe, expect, it } from 'vitest'; +import trackedSources from 'virtual:etv-tracked-source-files'; +import { + declaresLocalComplete, + scanCompleteAnnotations, + scanOptionalSchemaMembers, + scanSchemaAliases, + type CompleteAnnotation +} from './completeAnnotationScan'; + +/** + * ersatztv#820 — `Complete` is APPLIED where the contract says it must be, and nowhere it must + * not be. + * + * ## What was missing + * + * Two guards existed and neither covered this. `completeRequest.guard.test.ts` proves the TYPE's + * semantics — `Complete` rejects a missing member and a phantom one — and would stay green with + * every annotation in the SPA deleted. `scripts/tests/test_optional_request_members.py` derives, + * from `ErsatzTV/wwwroot/openapi/v1.json` on every run, which schemas can silently drop a member on + * a write, and files each under a disposition; one of those, `COVERED`, is worded "the SPA builds + * this body and the builder is annotated `Complete`". + * + * That wording is a CLAIM ABOUT ANOTHER LANGUAGE'S SOURCE that nothing checked. A `COVERED` row + * and a SPA with the annotation deleted were indistinguishable to the whole suite. So was the + * opposite error: `docs/spa-conventions.md` §4b's "do NOT apply `Complete` to a schema whose + * optional members are computed server-side" was prose with no executable form. + * + * ## The population is derived; the dispositions are the reviewed scope + * + * The POPULATION — which schemas can drop a member — is read every run off + * `src/api/generated/v1.d.ts` with the compiler API. That file is a pure pass-through of the + * OpenAPI `required` array (`web/scripts/generate-openapi-types.mjs` does + * `const optional = required.has(name) ? '' : '?'`), so "optional here" and "outside `required` in + * `v1.json`" are ONE fact read at two points, not two derivations that could disagree. + * `DISPOSITIONS` below is the SCOPE: a reviewed policy choice per schema, hand-written on purpose, + * and FORCED to exist by the set-equality assertion — a new optional member fails this guard until + * someone writes down what should happen about it. + * + * RELATIONSHIP TO THE PYTHON GUARD, stated because "they check the same thing" would be wrong. + * That guard scopes to schemas REACHABLE FROM A REQUEST BODY (9 rows today); this one scopes to + * every generated schema with an optional member (13), so it additionally sees response models. The + * two tables therefore overlap rather than coincide, and they can drift: re-dispositioning a schema + * on one side does not redden the other. What is NOT possible is a schema going unnoticed by both, + * because both derive their population from the same `required` arrays. + * + * ## Residual, stated rather than left to be discovered + * + * This guard proves an annotation EXISTS for a schema; it does not prove that every construction + * site of that schema carries one. The missing-member direction does not need it — the wrapper + * parameter's annotation reaches every caller — but the PHANTOM direction does, because + * TypeScript's excess-property check fires only on a fresh object literal in a contextually typed + * position, which a generic `.map` callback's return is not (§4b). That per-site population is + * "sites in code", the case `testing.guard-derives-population-from-source` explicitly scopes out + * and #777 tracks. A deep `Complete` would close the missing direction into nested types but + * NOT the phantom one, and it cannot be applied blanket-wise anyway — see `MUST-NOT-ANNOTATE`. + */ + +// --- the reviewed scope --------------------------------------------------------------------- +// +// ANNOTATED a write body that can drop a member, so the SPA MUST annotate it `Complete`. +// Equivalent to the Python guard's COVERED, and this is the row that turns that +// registry's claim into a check. +// MUST-NOT-ANNOTATE the optional members are computed get-only properties on the C# record. Nothing +// deserializes them, so omitting them drops nothing — and annotating WOULD BE A +// BUG, forcing a caller to fabricate server-computed values (§4b). Asserted as an +// absence, so the prohibition is executable rather than advisory. +// CREATE a POST that creates an entity, where an omitted member correctly means "use the +// default". +// TRIGGER the body parameterises an action and replaces no stored entity. +// RESPONSE-ONLY never sent as a request body, so there is nothing for a builder to drop. +// +// The last three are PERMISSIVE by design: they require no annotation and forbid none. That is +// deliberate rather than an omission — annotating one is harmless (it costs a caller nothing to name +// a field it was going to set anyway), while FORBIDDING it would make a future full-replace endpoint +// over the same schema harder to protect. Where annotating is actively wrong, the row is +// MUST-NOT-ANNOTATE and the prohibition is asserted; `CreateChannelFromLineupAdvancedOptionsRequest` +// is such a row despite being a create. +type Disposition = 'ANNOTATED' | 'MUST-NOT-ANNOTATE' | 'CREATE' | 'TRIGGER' | 'RESPONSE-ONLY'; + +const DISPOSITIONS: Record = { + CreateFFmpegProfileRequest: { + disposition: 'ANNOTATED', + note: "FFmpegProfilesScreen's `Draft` feeds BOTH the POST and the full-replace PUT, so the draft type itself is `Complete<…>`. `qsvPreferNativeDecoder` was a live silent drop before #807." + }, + UpdateFFmpegProfileRequest: { + disposition: 'ANNOTATED', + note: 'PUT /ffmpeg/profiles/{id} is a full replace; `qsvPreferNativeDecoder` is a defaulted ctor param so ASP.NET drops it from `required`.' + }, + MultiCollectionItemRequest: { + disposition: 'ANNOTATED', + note: 'Nested in the full-replace PUT /multi-collections/{id}. Dropping `weight` resets every weight to 1 — a prose comment warned about it, and a comment is not a check.' + }, + ArtworkContentTypeModel: { + disposition: 'MUST-NOT-ANNOTATE', + note: 'Reachable from the full-replace PUT /channels/{id} via `UpdateChannelRequest.logo`, so an endpoint-shaped rule would demand an annotation. `isExternalUrl`/`hasContentType`/`urlWithContentType` are computed get-only properties, never deserialized.' + }, + AutoTunedChannelRequest: { + disposition: 'CREATE', + note: 'POST /channels/auto-tune. The handler adds a channel and rejects a duplicate number; it never overwrites an existing one.' + }, + AutoTuneSourceWeightRequest: { + disposition: 'CREATE', + note: 'Nested in the auto-tune POST body; same create-time semantics as its parent.' + }, + CreateChannelFromLineupAdvancedOptionsRequest: { + // MUST-NOT-ANNOTATE rather than CREATE, so the prohibition is asserted rather than merely + // described: annotating this one is a BUG for the same reason as the server-computed case, just + // arrived at differently. Cold review pointed out that CREATE was permissive here while the note + // said annotating would break the semantics. + disposition: 'MUST-NOT-ANNOTATE', + note: 'POST /channels/from-lineup. Omission is LOAD-BEARING: an omitted override means INHERIT the template value, with a separate `clear` list to force NONE. `Complete` would collapse that third state into explicit-null.' + }, + ScanShowRequest: { + disposition: 'TRIGGER', + note: 'POST /libraries/{id}/scan-show starts a scan; `deepScan` parameterises the action and replaces no entity.' + }, + ChannelLogoResponseModel: { + disposition: 'RESPONSE-ONLY', + note: 'Read model. Same computed get-only trio as ArtworkContentTypeModel.' + }, + LibraryBrowseItemResponseModel: { + disposition: 'RESPONSE-ONLY', + note: 'Read model for the library browser; `subtitle`/`seasonId` are present only for the item kinds that have them. The SPA never sends it.' + }, + MultiCollectionItemResponseModel: { + disposition: 'RESPONSE-ONLY', + note: 'Read model. The REQUEST twin of this schema is dispositioned ANNOTATED above.' + }, + ProblemDetails: { disposition: 'RESPONSE-ONLY', note: 'RFC 7807 error payload; never sent by the SPA.' }, + ValidationProblemDetails: { + disposition: 'RESPONSE-ONLY', + note: 'RFC 7807 validation error payload returned by the API on a 400; never constructed or sent by the SPA.' + } +}; + +// --- discovery ------------------------------------------------------------------------------ + +const rawSourceModules = import.meta.glob('/src/**/*.{ts,tsx,mts,cts}', { + query: '?raw', + import: 'default', + eager: true +}) as Record; + +const GENERATED_DTS_PATH = 'src/api/generated/v1.d.ts'; + +/** Glob keys are root-absolute (`/src/x.ts`); the git index spells them root-relative. */ +function globKeyToRootRelativePath(key: string): string { + return key.startsWith('/') ? key.slice(1) : key; +} + +/** + * In scope for the ANNOTATION scan: tracked SPA source, excluding generated output and excluding + * this guard itself (whose `DISPOSITIONS` notes and doc comment mention `Complete<…>` in prose — + * harmless, since the scanner matches type-reference NODES, but scanning the file that defines the + * expectation is the "check walks the same tree as its subject" shape and is avoided by construction). + * + * Test files are deliberately IN scope, unlike the pageSize guard's population: a `Complete<…>` in a + * test fixture is a real application of the type, and #807's own regression fixture lives in + * `ffmpegProfiles.test.ts`. They are scanned but cannot DISCHARGE a production obligation — see + * `isProductionSource`. + * + * The `*.guard.test.ts` exclusion is broader than this file and has a stated cost: + * `completeRequest.guard.test.ts` annotates synthetic inline types (`Complete<{ a: number }>`) to + * prove the type's semantics, which resolve to no schema and would fail the unresolved assertion for + * no benefit. The consequence to accept: a MUST-NOT-ANNOTATE violation written inside a + * `*.guard.test.ts` would not be caught. That is a test-only file annotating a schema it must not, + * which changes no production behaviour — recorded rather than fixed. + */ +function isInScopeForAnnotations(rootRelativePath: string): boolean { + if (!/\.(ts|tsx|mts|cts)$/.test(rootRelativePath)) { + return false; + } + return !rootRelativePath.includes('/generated/') && !rootRelativePath.endsWith('.guard.test.ts'); +} + +interface DiscoveredAnnotation extends CompleteAnnotation { + file: string; + /** The generated schema the written alias resolves to, or `undefined` when nothing maps it. */ + schemaName: string | undefined; +} + +// Each of these parses the whole tracked population with the compiler API, and several tests need +// the same result, so they are computed ONCE. Not a micro-optimisation: unmemoised, the annotation +// scan walked the population twice per call (directly, and again through the alias map) across four +// tests, and the guard passed in isolation while timing out at vitest's 5s default under full-suite +// load — green alone, red in `npm test`, which is the worst way for a guard to fail. The inputs are +// module-level immutable strings, so caching cannot mask a mutation: each `vitest run` is a fresh +// process that re-reads the tree. +let cachedSources: { file: string; text: string }[] | undefined; +let cachedAliases: Map | undefined; +let cachedAnnotations: DiscoveredAnnotation[] | undefined; +let cachedOptionalMembers: Map | undefined; + +function trackedInScopeSources(): { file: string; text: string }[] { + if (cachedSources === undefined) { + const tracked = new Set(trackedSources.tracked); + cachedSources = Object.entries(rawSourceModules) + .map(([key, text]) => ({ file: globKeyToRootRelativePath(key), text })) + .filter(({ file }) => tracked.has(file) && isInScopeForAnnotations(file)) + .sort((a, b) => a.file.localeCompare(b.file)); + } + return cachedSources; +} + +/** localName -> generated schema key, built across the WHOLE population (aliases cross files). */ +function schemaAliasMap(): Map { + if (cachedAliases === undefined) { + const map = new Map(); + for (const { file, text } of trackedInScopeSources()) { + for (const alias of scanSchemaAliases(text, file)) { + map.set(alias.localName, alias.schemaName); + } + } + cachedAliases = map; + } + return cachedAliases; +} + +function discoverAnnotations(): DiscoveredAnnotation[] { + if (cachedAnnotations === undefined) { + const aliases = schemaAliasMap(); + const found: DiscoveredAnnotation[] = []; + for (const { file, text } of trackedInScopeSources()) { + for (const annotation of scanCompleteAnnotations(text, file)) { + found.push({ ...annotation, file, schemaName: aliases.get(annotation.typeName) }); + } + } + cachedAnnotations = found; + } + return cachedAnnotations; +} + +/** + * A source file whose annotations can DISCHARGE a production obligation. + * + * Test files are scanned (a `Complete<…>` there is still an application, and #807's own regression + * fixture lives in one) but they cannot satisfy the requirement that a schema be annotated in + * production. Cold review demonstrated why with a working bypass: strip `Complete<>` off the + * `updateFFmpegProfile` WRAPPER and retype `ffmpegProfiles.test.ts`'s fixture to name + * `UpdateFFmpegProfileRequest`, and a per-schema "annotated somewhere" rule stays green while the + * wrapper stops protecting every future caller. + */ +function isProductionSource(rootRelativePath: string): boolean { + // `.spec.` and `setupTests.ts` are here because cold review measured the `.test.`-only form false: + // vitest's own default include is `**/*.{test,spec}.?(c|m)[jt]s?(x)` (measured on 4.1.9), so a `*.spec.ts` fixture was + // a TEST to the runner and PRODUCTION to this predicate, and could discharge the obligation while + // the real builder lost its annotation. Same for the setup file, which vitest loads for every run. + // + // WHAT THIS COVERS, bounded rather than universal — an earlier version of this comment said it had + // to be true "in all of the spellings vitest accepts", and review measured THAT false in turn: + // `*.test.*`, `*.spec.*`, and the ONE setup file named in `vite.config.ts`, whose path is pinned by + // the scope-pin test so a rename reddens. NOT covered: a SECOND `setupFiles` entry. vitest would + // load it every run, so an annotation there discharges the obligation — measured, with the real + // wrapper unprotected and this guard green. Deriving `setupFiles` from the vite config needs + // another virtual-module plugin and is not proportionate to a three-site coordinated edit; the + // residual is stated instead, which is the same tolerance `pageSizeCallSites.guard.test.ts` takes. + return !/\.(test|spec)\.(ts|tsx|mts|cts)$/.test(rootRelativePath) && rootRelativePath !== 'src/setupTests.ts'; +} + +function generatedDtsText(): string { + const text = rawSourceModules[`/${GENERATED_DTS_PATH}`]; + if (text === undefined) { + throw new Error( + `completeAnnotations guard: the glob supplied no content for ${GENERATED_DTS_PATH}. The ` + + `population of droppable schemas is read from that file, so an empty read must fail rather ` + + `than report every schema as having no optional member.` + ); + } + return text; +} + +function allSchemaOptionalMembers(): Map { + if (cachedOptionalMembers === undefined) { + cachedOptionalMembers = scanOptionalSchemaMembers(generatedDtsText()); + } + return cachedOptionalMembers; +} + +function schemasWithOptionalMembers(): Map { + return new Map([...allSchemaOptionalMembers().entries()].filter(([, optionals]) => optionals.length > 0)); +} + +// --- the guard ------------------------------------------------------------------------------ + +describe('Complete application guard (#820)', () => { + it('scans a healthy number of TRACKED source files (anti-vacuity: a broken glob or index read must not pass on zero input)', () => { + expect(trackedInScopeSources().length).toBeGreaterThan(200); + }, 20000); + + it('parses a healthy number of schemas out of the generated types (anti-vacuity: a broken d.ts parse must not report zero droppable schemas)', () => { + expect(allSchemaOptionalMembers().size).toBeGreaterThan(100); + }, 20000); + + it('discovers a healthy number of Complete<…> annotations (anti-vacuity: a broken scan must not pass on zero matches)', () => { + expect(discoverAnnotations().length).toBeGreaterThan(25); + }, 20000); + + it('matches the schemas that can DROP A MEMBER exactly against the reviewed dispositions (not a non-empty check)', () => { + const discovered = new Set(schemasWithOptionalMembers().keys()); + const registered = new Set(Object.keys(DISPOSITIONS)); + + const undispositioned = [...discovered].filter((s) => !registered.has(s)).sort(); + const stale = [...registered].filter((s) => !discovered.has(s)).sort(); + + if (undispositioned.length > 0 || stale.length > 0) { + const optionals = schemasWithOptionalMembers(); + throw new Error( + [ + `UNDISPOSITIONED (${undispositioned.length}) — schema(s) with an optional member and no row in DISPOSITIONS.`, + 'A member outside the OpenAPI `required` array can be silently dropped by a SPA builder; say what should happen about it.', + ...undispositioned.map((s) => ` + ${s} (optional: ${optionals.get(s)?.join(', ')})`), + `STALE (${stale.length}) — DISPOSITIONS row(s) whose schema no longer has any optional member:`, + ...stale.map((s) => ` - ${s}`) + ].join('\n') + ); + } + }, 20000); + + it('every schema dispositioned ANNOTATED actually carries a Complete<…> annotation in the SPA', () => { + // THE point of this file: it converts the Python registry's `COVERED` wording — "the builder is + // annotated `Complete`" — from a claim into a check. Reverting a covered site to its + // pre-#807 form reddens here. + const annotatedSchemas = new Set( + discoverAnnotations() + .filter((a) => isProductionSource(a.file)) + .map((a) => a.schemaName) + .filter((s): s is string => s !== undefined) + ); + const required = Object.entries(DISPOSITIONS) + .filter(([, v]) => v.disposition === 'ANNOTATED') + .map(([schema]) => schema) + .sort(); + + const unannotated = required.filter((s) => !annotatedSchemas.has(s)); + expect( + unannotated, + `Dispositioned ANNOTATED but no \`Complete<…>\` names them anywhere in the SPA: ${unannotated.join(', ')}. ` + + `Either the annotation was removed (the member can now be silently dropped on a full replace) ` + + `or the disposition is wrong.` + ).toEqual([]); + // Anti-vacuity for THIS assertion specifically: an empty `required` would satisfy it trivially. + expect(required.length).toBeGreaterThan(0); + }, 20000); + + it('no schema dispositioned MUST-NOT-ANNOTATE is annotated (§4b: it would force a caller to invent server-computed values)', () => { + const forbidden = new Set( + Object.entries(DISPOSITIONS) + .filter(([, v]) => v.disposition === 'MUST-NOT-ANNOTATE') + .map(([schema]) => schema) + ); + const violations = discoverAnnotations() + .filter((a) => a.schemaName !== undefined && forbidden.has(a.schemaName)) + .map((a) => `${a.file}:${a.line}:${a.column} Complete<${a.typeName}>`); + + expect(violations, `Annotated a schema whose optional members are computed server-side: ${violations.join(', ')}`).toEqual( + [] + ); + expect(forbidden.size).toBeGreaterThan(0); + }, 20000); + + it('every Complete<…> resolves to a GENERATED schema, so it checks the contract and not a local mirror', () => { + // The finding that produced this assertion: `playouts.ts` declared + // `ReplacePlayoutTemplatesRequest` and `ReplacePlayoutAlternateSchedulesRequest` as hand-written + // object types that SHADOWED generated schemas of the same name. They dropped no field — they + // narrowed `null | Array` to `T[]` — but `Complete` over a hand-maintained mirror proves + // only that the caller filled in the MIRROR, which is the #754 mechanism wearing the annotation + // that is supposed to prevent it. + const unresolved = discoverAnnotations() + .filter((a) => a.schemaName === undefined) + .map((a) => `${a.file}:${a.line}:${a.column} Complete<${a.typeName}>`); + + expect( + unresolved, + `\`Complete<…>\` over a type that is not an alias of \`components['schemas'][…]\`: ${unresolved.join(', ')}. ` + + `Alias the generated schema instead of re-declaring its shape.` + ).toEqual([]); + }, 20000); + + it('scans nearly ALL tracked TypeScript, so a scope edit cannot quietly drop a quarter of it', () => { + // Named pins catch a narrowing that happens to hit one of five files; cold review dropped 65 of + // 257 files with every pin and floor still green. This bounds the whole population instead. + // + // The denominator is deliberately NOT computed through `isInScopeForAnnotations`: a helper on + // both sides of a comparison cancels out of it, which is the #819 finding recorded at length in + // the pageSize guard. It is a different and cruder expression — every tracked TypeScript file — + // so the two can actually disagree. The legitimate gap between them is only `/generated/` and + // `*.guard.test.ts`, about 1.5% today, hence the 95% floor. + const trackedTypeScript = trackedSources.tracked.filter((p) => /\.(ts|tsx|mts|cts)$/.test(p)); + const scanned = trackedInScopeSources().length; + expect(trackedTypeScript.length).toBeGreaterThan(200); + expect( + scanned / trackedTypeScript.length, + `only ${scanned} of ${trackedTypeScript.length} tracked TypeScript files reached the scanner. ` + + `The scope predicate excludes \`/generated/\` and \`*.guard.test.ts\` and nothing else, so a ` + + `larger shortfall means it was narrowed.` + ).toBeGreaterThan(0.95); + }, 20000); + + it('keeps the modules carrying the motivating defects inside the scanned population (scope pin)', () => { + // A floor cannot see SELECTIVE narrowing: cold review showed that excluding one path from + // `isInScopeForAnnotations` stays comfortably above every count while erasing this guard's reach + // over exactly the file whose defect motivated it. These pin the paths rather than the count. + // `isProductionSource` hardcodes 'src/setupTests.ts', mirroring `vite.config.ts`'s `setupFiles` + // with nothing coupling them. Renaming the setup file would leave the exclusion stale, vitest + // would still load the new one for every run, and the round-5 `.spec.ts` bypass would reopen + // green. Pinning it turns that silent staleness into a red — the same remedy as the paths below. + expect( + trackedSources.tracked, + "src/setupTests.ts moved or was renamed; isProductionSource still excludes the old path" + ).toContain('src/setupTests.ts'); + + const files = new Set(trackedInScopeSources().map((s) => s.file)); + for (const pinned of [ + 'src/api/playouts.ts', + 'src/api/ffmpegProfiles.ts', + 'src/api/multiCollections.ts', + 'src/screens/MultiCollectionsScreen.tsx', + 'src/screens/ffmpegProfileDraft.ts' + ]) { + expect(files.has(pinned), `${pinned} dropped out of the scanned population`).toBe(true); + } + }, 20000); + + it('supplies content for every TRACKED in-scope path (a shortfall is a hole, not a skip)', () => { + // The glob/index operation is an inner join, so a tracked in-scope path the glob never yielded + // would vanish from the population silently. + const globbed = new Set(Object.keys(rawSourceModules).map(globKeyToRootRelativePath)); + const missing = trackedSources.tracked + .filter((p) => isInScopeForAnnotations(p) && !globbed.has(p)) + .filter((p) => !trackedSources.absentFromDisk.includes(p)); + expect(missing, `Tracked, on-disk, in-scope path(s) the scanner never saw: ${missing.join(', ')}`).toEqual([]); + }, 20000); + + it('no file declares its OWN Complete, which would make an annotation an identity type', () => { + // `Complete` is matched by NAME. A local `type Complete = T` would satisfy every + // annotation assertion here while providing none of the checking — a false green cold review + // demonstrated. Nothing declares one today; this keeps it that way. + const shadows = trackedInScopeSources() + .filter(({ file }) => file !== 'src/api/completeRequest.ts') + .filter(({ file, text }) => declaresLocalComplete(text, file)) + .map(({ file }) => file); + expect(shadows, `Local \`Complete\` declaration(s) shadowing the mapped type: ${shadows.join(', ')}`).toEqual([]); + }, 20000); + + it('resolves every alias unambiguously — two files may not map the same local name to different schemas', () => { + // The alias map is global and last-write-wins, so a collision would silently credit the wrong + // schema. Detected rather than tolerated. + const byLocal = new Map>(); + for (const { file, text } of trackedInScopeSources()) { + for (const alias of scanSchemaAliases(text, file)) { + const set = byLocal.get(alias.localName) ?? new Set(); + set.add(alias.schemaName); + byLocal.set(alias.localName, set); + } + } + const collisions = [...byLocal.entries()] + .filter(([, schemas]) => schemas.size > 1) + .map(([local, schemas]) => `${local} -> {${[...schemas].join(', ')}}`); + expect(collisions, `Ambiguous local alias name(s): ${collisions.join('; ')}`).toEqual([]); + }, 20000); + + it('every disposition row states a reason', () => { + const unexplained = Object.entries(DISPOSITIONS) + .filter(([, v]) => v.note.trim().length < 40) + .map(([schema]) => schema); + expect(unexplained, `DISPOSITIONS rows with no usable note: ${unexplained.join(', ')}`).toEqual([]); + }, 20000); +}); diff --git a/web/src/api/multiCollections.ts b/web/src/api/multiCollections.ts index 635e17af1..ff80d217f 100644 --- a/web/src/api/multiCollections.ts +++ b/web/src/api/multiCollections.ts @@ -44,7 +44,14 @@ export function getMultiCollectionWithMeta(id: number): Promise(`/api/v1/multi-collections/${id}`); } -export function createMultiCollection(body: CreateMultiCollectionRequest): Promise { +// `Complete<…>` matches the full-replace update wrapper below, so both entry points to this screen's +// body are the same shape. Be precise about what it buys, because an earlier comment here claimed +// more: `Complete` is SHALLOW, so this requires `name` and `items` to be NAMED and requires +// nothing of each item — `{ items: [{}] }` typechecks. What protects `MultiCollectionItemRequest.weight` +// is the `Complete` on `toItemRequest` in `MultiCollectionsScreen`, not +// this. Today the schema has no optional members of its own, so this annotation is a no-op that +// exists to be already in place the day one appears (ersatztv#820). +export function createMultiCollection(body: Complete): Promise { return request('/api/v1/multi-collections', { body, method: 'POST' }); } diff --git a/web/src/api/playouts.ts b/web/src/api/playouts.ts index 00678e98e..cc131b88a 100644 --- a/web/src/api/playouts.ts +++ b/web/src/api/playouts.ts @@ -45,15 +45,21 @@ export type PlayoutAlternateScheduleItemRequest = components['schemas']['Playout export type PlayoutTemplate = components['schemas']['PlayoutTemplateResponseModel']; export type PlayoutTemplateItemRequest = components['schemas']['PlayoutTemplateItemRequest']; -// Declared as `type` (not `interface`) so they satisfy the client's `Record` -// RequestBody bound, matching the generated request DTOs. -export type ReplacePlayoutAlternateSchedulesRequest = { - items: PlayoutAlternateScheduleItemRequest[]; -}; +// Aliased from the generated schema rather than re-declared, so `Complete<…>` on the wrapper below +// checks against the API contract instead of a local copy of it (ersatztv#820). These were +// hand-written object types until then; they dropped no field, but a hand-maintained mirror of a +// DTO is the #754 mechanism, and a `Complete` over a mirror proves only that the caller filled +// in the MIRROR. Declared as `type` (not `interface`) so they satisfy the client's +// `Record` RequestBody bound — an alias of the generated type is already a `type`, +// so that constraint never required re-declaring the shape. Note what the alias ADMITS that the +// mirror did not: `items` is `null | Array`, and the server reads `null` as an empty list — +// clearing every template, or being rejected outright for alternate schedules, which require at +// least one item. That is the real contract; the mirror was narrower than the API, not safer +// than it, and locally re-narrowing a generated type hides the case rather than preventing it. +export type ReplacePlayoutAlternateSchedulesRequest = + components['schemas']['ReplacePlayoutAlternateSchedulesRequest']; -export type ReplacePlayoutTemplatesRequest = { - items: PlayoutTemplateItemRequest[]; -}; +export type ReplacePlayoutTemplatesRequest = components['schemas']['ReplacePlayoutTemplatesRequest']; export interface PlayoutsScreenData { channelStates: PlayoutChannelState[];