fix(820): derive where Complete<T> is APPLIED, not just what it means (#883)
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 9s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 28s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 14m22s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 11m1s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 9m58s
Build ErsatzTV Image / Build & push image (amd64) (push) Failing after 1m58s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 9s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 28s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 14m22s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 11m1s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 9m58s
Build ErsatzTV Image / Build & push image (amd64) (push) Failing after 1m58s
`Complete<T>` (#807) makes SPA full-replace bodies fail typecheck when a builder omits a schema member. Nothing checked it was APPLIED: `completeRequest.guard.test.ts` proves the type's semantics and would stay green with every annotation deleted, and `test_optional_request_members.py`'s COVERED disposition — "the builder is annotated `Complete<T>`" — was a claim about another language's source that nothing verified. Adds `completeAnnotationScan.ts` (compiler-API scanners) + `completeAnnotations.guard.test.ts`, with a synthetic-source fixture suite. Two derived populations: the `Complete<…>` annotations (SPA AST ∩ git index) and the droppable schemas (parsed from the generated `v1.d.ts`, a pass-through of the OpenAPI `required` array). It asserts a production annotation per schema dispositioned as needing one, NO annotation on the server-computed and load-bearing-omission schemas, that every `Complete<X>` resolves to a generated schema rather than a hand-written mirror, and set equality between droppable schemas and the reviewed dispositions. `test_complete_annotation_dispositions.py` cross-checks that table against the authoritative Python one and ships a declared, harness-executed mutation. 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 — the #754 mechanism wearing the annotation meant to prevent it. Eight review rounds, seven BLOCKED, two independent cold reviewers. A wrapper-signature scanner was built and REMOVED: every blocker traced to that one mechanism (obligation on the wrong population; reachability mistaken for protection, since `Complete<T>` is shallow; body discovery keyed on a parameter name, then parameter-vs-local; and finally `export function` → `export const` blinding the scanner and its cross-check together). Five defects from one mechanism, so the mechanism went rather than a sixth patch. Residuals stated in §4b, the guard-inventory row and the record: per-SCHEMA not per-site or per-wrapper; token presence not liveness; the phantom direction unchecked (#777); a second `setupFiles` entry could discharge; and plugin-level population integrity borrowed from the sibling guard. fixes #820 Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>
This commit was merged in pull request #883.
This commit is contained in:
@@ -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<T>` (`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<T>` · 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<T>`, `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<X>` 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<T>` 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<T>` 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<T>` 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.
|
||||
|
||||
File diff suppressed because one or more lines are too long
@@ -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<X>` 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<X>` 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 `<img
|
||||
|
||||
@@ -356,6 +356,22 @@ MUTATIONS: tuple[Mutation, ...] = (
|
||||
"pre-answer IS singly exploitable, which is why it is the declared clause; the tools stage is "
|
||||
"covered behaviourally in the same file.",
|
||||
),
|
||||
Mutation(
|
||||
guard="scripts/tests/test_complete_annotation_dispositions.py",
|
||||
target="web/src/api/completeAnnotations.guard.test.ts",
|
||||
clause="MultiCollectionItemRequest: {\n disposition: 'ANNOTATED',",
|
||||
replacement="MultiCollectionItemRequest: {\n disposition: 'CREATE',",
|
||||
proof="test_complete_annotation_dispositions.py::test_the_two_dispositions_AGREE",
|
||||
granularity=CLAUSE,
|
||||
expect="this file says COVERED, the SPA guard says CREATE",
|
||||
why="This guard IS a test, so disarming it makes it absent rather than red — the checker-guard "
|
||||
"exception applies and the mutation goes into the guarded ARTIFACT, the SPA guard's disposition "
|
||||
"table. The VALUE is the clause: cold review demonstrated that flipping this row from ANNOTATED "
|
||||
"to CREATE retires the requirement that MultiCollectionItemRequest be annotated, so deleting the "
|
||||
"Complete<...> 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.",
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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<T>`, 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<T>` 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<schema>\w+): \{\n(?:\s*//[^\n]*\n)*\s*disposition: '(?P<disposition>[A-Z-]+)'",
|
||||
re.MULTILINE,
|
||||
)
|
||||
|
||||
# The compact single-line form the formatter emits for short rows.
|
||||
_TS_ROW_INLINE = re.compile(r"^ (?P<schema>\w+): \{ disposition: '(?P<disposition>[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."
|
||||
)
|
||||
@@ -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<T>`, 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<T>` 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"
|
||||
|
||||
@@ -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<ReplaceBlockRequest>): 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<A> = y;', 'function g(): Complete<B> { 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<BlockItemRequest> => ({ ...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<ReplaceBlockRequest> 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<A>";', 'const t = `Complete<B>`;'].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<X, Y>;'].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<components['schemas']['X']>;";
|
||||
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 = <div className="x" />;', 'export const f = (b: Complete<A>) => 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<B>;', 'let a: Complete<A>;'].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<T>` (a TypeReference), not `T[]`; pin the shape the generator
|
||||
// actually produces rather than only the one that reads like an array.
|
||||
['an Array<T> as the generator emits it', ' "A": Array<ComponentB>;']
|
||||
])('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> = T;', 'a.ts')).toBe(true);
|
||||
expect(declaresLocalComplete('interface Complete<T> { 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<X> before sending */\nconst a = 1;', 'a.ts')).toBe(false);
|
||||
expect(declaresLocalComplete("const s = 'type Complete<T> = 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);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,328 @@
|
||||
import * as ts from 'typescript';
|
||||
|
||||
/**
|
||||
* ersatztv#820 — AST scanners that let a guard check where `Complete<T>` is APPLIED, not only what
|
||||
* it means.
|
||||
*
|
||||
* ## The gap this exists to close
|
||||
*
|
||||
* `completeRequest.guard.test.ts` proves the TYPE's semantics: `Complete<T>` 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<T>`".
|
||||
*
|
||||
* 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<Foo>` 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<T>` 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<components['schemas']['X']>`. 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<Y>` is unresolved — a false RED, which is noisy but safe.
|
||||
* - A nested generic (`Complete<Box<S>>`) 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> = 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<X>` 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<X>` 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<string, string[]> {
|
||||
const sourceFile = parse(dtsText, 'v1.d.ts');
|
||||
const optionalsBySchema = new Map<string, string[]>();
|
||||
|
||||
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> = 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> = 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;
|
||||
}
|
||||
@@ -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<T>` 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<T>` 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<T>`".
|
||||
*
|
||||
* 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<T>` 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<T>` 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<T>`.
|
||||
// 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<string, { disposition: Disposition; note: string }> = {
|
||||
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<T>` 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<string, string>;
|
||||
|
||||
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<string, string> | undefined;
|
||||
let cachedAnnotations: DiscoveredAnnotation[] | undefined;
|
||||
let cachedOptionalMembers: Map<string, string[]> | 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<string, string> {
|
||||
if (cachedAliases === undefined) {
|
||||
const map = new Map<string, string>();
|
||||
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<string, string[]> {
|
||||
if (cachedOptionalMembers === undefined) {
|
||||
cachedOptionalMembers = scanOptionalSchemaMembers(generatedDtsText());
|
||||
}
|
||||
return cachedOptionalMembers;
|
||||
}
|
||||
|
||||
function schemasWithOptionalMembers(): Map<string, string[]> {
|
||||
return new Map([...allSchemaOptionalMembers().entries()].filter(([, optionals]) => optionals.length > 0));
|
||||
}
|
||||
|
||||
// --- the guard ------------------------------------------------------------------------------
|
||||
|
||||
describe('Complete<T> 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<T>`" — 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<T>` to `T[]` — but `Complete<T>` 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<X>` is matched by NAME. A local `type Complete<T> = 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<string, Set<string>>();
|
||||
for (const { file, text } of trackedInScopeSources()) {
|
||||
for (const alias of scanSchemaAliases(text, file)) {
|
||||
const set = byLocal.get(alias.localName) ?? new Set<string>();
|
||||
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);
|
||||
});
|
||||
@@ -44,7 +44,14 @@ export function getMultiCollectionWithMeta(id: number): Promise<ResponseWithMeta
|
||||
return requestWithMeta<MultiCollection>(`/api/v1/multi-collections/${id}`);
|
||||
}
|
||||
|
||||
export function createMultiCollection(body: CreateMultiCollectionRequest): Promise<MultiCollection> {
|
||||
// `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<T>` 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<MultiCollectionItemRequest>` 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<CreateMultiCollectionRequest>): Promise<MultiCollection> {
|
||||
return request<MultiCollection>('/api/v1/multi-collections', { body, method: 'POST' });
|
||||
}
|
||||
|
||||
|
||||
+14
-8
@@ -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<string, unknown>`
|
||||
// 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<T>` over a mirror proves only that the caller filled
|
||||
// in the MIRROR. Declared as `type` (not `interface`) so they satisfy the client's
|
||||
// `Record<string, unknown>` 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<T>`, 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[];
|
||||
|
||||
Reference in New Issue
Block a user