Round 3 returned BLOCKED on a CI-red I would have shipped. BLOCKER — `ruff format --check` reds on the new guard file. Two implicitly concatenated strings fit in 120 chars when joined. `ruff check` was green, which is why this survived; the format step is a separate gate and the merge gate reads COMBINED status, so an advisory-looking red still blocks. The real defect is that ruff was absent from my local gate for six commits while I was reporting "verification: ..." lines that read as complete. Both ruff steps are now run, in the CI's own `git ls-files` form under `bash -c` (zsh has no `mapfile`). FALSE NEGATIVE, third of its kind on this branch — the guard's own comment said "No real request type here can discriminate today, because none has an optional member". That was false when written, and this PR's own derived guard lists eight schemas that contradict it. It also cost the proof its most valuable case. Case 4 is re-pinned from `ReplaceDecoTemplateRequest.name` (already REQUIRED, so it errored with or without `Complete<T>`) to `MultiCollectionItemRequest.weight`, which is genuinely optional and is the exact field whose loss was live before #807. Measured: case 4 now reddens under ALL THREE weakenings including the realistic `{ [K in keyof T]: T[K] }`, where before it reddened only under `Partial<T>`. The table is updated to what was measured, and the file now says that the set of schemas with optional members is DERIVED and must not be restated in prose — three hand-written versions of that list have now been wrong. STALE AFTER THE VERB WIDENING — the MISSING diagnostic still told the reader "reachable from a POST/PUT/PATCH body" one commit after the walk started seeding from every verb, so it could name a DELETE-reached schema while misdescribing where it came from; `expect` matched the tail of the message, so no test caught it. Same sentence fixed in the decision record. `oneOf` COVERAGE WAS CLAIMED, NOT HELD — `_schema_refs` has a dict branch and a list branch, and only the dict one was exercised: deleting list descent (making all 23 `oneOf` refs invisible) left the file GREEN. The planted fixture now goes in through a `oneOf` list, and deleting that branch reddens it. Also: deterministic host selection in the mutation test (set iteration over strings varies per process, so the proof's depth varied run to run); the `#/components/` bucket regex no longer assumes alphabetic bucket names; the hand-maintained "thirteen of thirty-three wrappers" count is replaced by its reproduction command; the record now reconciles its own pin-vs-floor argument against this guard's floors; 141 -> 142; generator path corrected to `web/scripts/`. Verification: ruff format 0, ruff check 0, 880 passed 2 skipped, typecheck 0, eslint 0, 1200/1200 web tests, build 0, catalog clean, doc-narrative 0 new (9 pre-existing warnings, none in files this branch touches). Decisions-Edit: yes
336 lines
17 KiB
Python
336 lines
17 KiB
Python
"""#807 guard: every schema that can SILENTLY DROP a member on a SPA write has a stated disposition.
|
|
|
|
WHAT THIS BLOCKS. A request-body property that is absent from its schema's `required` array emits
|
|
into `web/src/api/generated/v1.d.ts` as an OPTIONAL member (`"weight"?: number`). A SPA builder may
|
|
then omit it, `tsc` says nothing — optional means omittable, by design — and on a FULL-REPLACE write
|
|
the server stores the field's default. That is #754's mechanism and it is what #807 found live in
|
|
`MultiCollectionItemRequest.weight` and `UpdateFFmpegProfileRequest.qsvPreferNativeDecoder`.
|
|
|
|
WHY IT IS A DERIVED GUARD AND NOT A TABLE IN A DOC. #807 shipped the disposition list by hand
|
|
TWICE and got it wrong BOTH times, each time by sorting a schema on its NAME rather than on what its
|
|
endpoint does:
|
|
|
|
round 1 a prose sentence exempted "create/update" — `updateMultiCollection` and
|
|
`updateFFmpegProfile` are full replaces, and both were live silent drops.
|
|
round 2 a hand-written table replaced that sentence and omitted `ArtworkContentTypeModel`,
|
|
because `…Model` reads as a response model. It is reachable from `PUT /channels/{id}`.
|
|
|
|
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.
|
|
|
|
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
|
|
are the SCOPE: a reviewed policy choice per schema, legitimately hand-written, and each one is
|
|
FORCED to exist by the set-equality assertion. A new optional member anywhere in the request graph
|
|
fails this test until someone writes down what should happen about it.
|
|
|
|
Set equality is asserted in BOTH directions and reported separately, because they are opposite
|
|
defects: `missing` is a schema the API can drop and nobody has ruled on (the #807 defect), `phantom`
|
|
is a disposition for a schema that no longer has an optional member reachable from a request body
|
|
(the registry claiming coverage of something gone).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import json
|
|
import re
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
REPO_ROOT = Path(__file__).resolve().parents[2]
|
|
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`).
|
|
# 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
|
|
# drop.
|
|
# 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.
|
|
COVERED = "COVERED"
|
|
CREATE = "CREATE"
|
|
TRIGGER = "TRIGGER"
|
|
COMPUTED = "COMPUTED"
|
|
|
|
DISPOSITIONS: dict[str, tuple[str, str]] = {
|
|
"UpdateFFmpegProfileRequest": (
|
|
COVERED,
|
|
"PUT /ffmpeg/profiles/{id} is a full replace. `qsvPreferNativeDecoder` is a defaulted ctor "
|
|
"param so ASP.NET drops it from `required`. Was a LIVE silent drop before #807.",
|
|
),
|
|
"CreateFFmpegProfileRequest": (
|
|
COVERED,
|
|
"FFmpegProfilesScreen's `Draft` feeds BOTH the POST and the full-replace PUT, so the draft "
|
|
"type itself is `Complete<…>` rather than only the update wrapper.",
|
|
),
|
|
"MultiCollectionItemRequest": (
|
|
COVERED,
|
|
"Nested in the full-replace PUT /multi-collections/{id}. `weight` is a defaulted ctor param "
|
|
"(`CreateMultiCollectionRequest.cs`). Was a LIVE silent drop before #807: the screen carried "
|
|
"a prose comment warning that dropping it resets every weight to 1, and a comment is not a "
|
|
"check.",
|
|
),
|
|
"ArtworkContentTypeModel": (
|
|
COMPUTED,
|
|
"Reachable from the full-replace PUT /channels/{id} (via `UpdateChannelRequest.logo`), so "
|
|
"the endpoint test alone would put it in COVERED. It is not: `IsExternalUrl`, "
|
|
"`HasContentType` and `UrlWithContentType` are computed get-only properties on the record "
|
|
"`ArtworkContentTypeModel(string Path, string ContentType)`, never deserialized, so a "
|
|
"client omitting them drops nothing. Annotating the SPA site `Complete<…>` would force it "
|
|
"to fabricate server-computed values. This row exists because #807's hand-written table "
|
|
"omitted this schema on the strength of its `…Model` name.",
|
|
),
|
|
"AutoTunedChannelRequest": (
|
|
CREATE,
|
|
"POST /channels/auto-tune. `CreateChannelFromLineupHandler` does `Channels.Add(...)` and "
|
|
"rejects a duplicate number; it never overwrites an existing channel.",
|
|
),
|
|
"CreateChannelFromLineupAdvancedOptionsRequest": (
|
|
CREATE,
|
|
"POST /channels/from-lineup. Omission is LOAD-BEARING here: `CreateChannelFromLineupClearField` "
|
|
"documents that for the template-inheritable fields a null/omitted override means INHERIT the "
|
|
"template value, with a separate explicit `clear` list to force NONE. `Complete<T>` would "
|
|
"collapse that third state into explicit-null.",
|
|
),
|
|
"AutoTuneSourceWeightRequest": (
|
|
CREATE,
|
|
"Nested in the auto-tune POST body; same create-time semantics as its parent.",
|
|
),
|
|
"ScanShowRequest": (
|
|
TRIGGER,
|
|
"POST /libraries/{id}/scan-show starts a scan. `deepScan` parameterises the action; no entity is replaced.",
|
|
),
|
|
}
|
|
|
|
# Anti-vacuity floors. A completeness check whose population came back empty must not report that it
|
|
# proved everything (`testing.guard-ships-with-mutation-proof`). These are LOWER bounds on the
|
|
# derived intermediates, deliberately well under today's values (142 reachable, 13 with optional
|
|
# members) so ordinary schema churn does not trip them — they exist to catch a broken parse or a
|
|
# `$ref` walk that reached nothing, not to pin the corpus.
|
|
MIN_REQUEST_REACHABLE_SCHEMAS = 40
|
|
MIN_SCHEMAS_WITH_OPTIONAL_MEMBERS = 5
|
|
|
|
|
|
def _load() -> dict:
|
|
if not OPENAPI.is_file():
|
|
pytest.fail(f"{OPENAPI.relative_to(REPO_ROOT)} is missing — run scripts/update-openapi.sh")
|
|
return json.loads(OPENAPI.read_text())
|
|
|
|
|
|
def _schema_refs(node: object, out: set[str]) -> None:
|
|
"""Collect every `#/components/schemas/X` name anywhere under `node`."""
|
|
if isinstance(node, dict):
|
|
ref = node.get("$ref")
|
|
if isinstance(ref, str) and ref.startswith("#/components/schemas/"):
|
|
out.add(ref.rsplit("/", 1)[1])
|
|
for value in node.values():
|
|
_schema_refs(value, out)
|
|
elif isinstance(node, list):
|
|
for value in node:
|
|
_schema_refs(value, out)
|
|
|
|
|
|
def _request_reachable(doc: dict) -> set[str]:
|
|
"""Every schema reachable from ANY operation's request body, transitively.
|
|
|
|
Two deliberate non-restrictions, both because this population has now been drawn by hand wrongly
|
|
twice and every hand-drawn edge is a place to be wrong again:
|
|
|
|
NO VERB ALLOW-LIST. An earlier draft scanned POST/PUT/PATCH, which reads as obviously right and
|
|
already had an exception: `DELETE /api/v1/media-items` carries a request body. Rather than argue
|
|
that a DELETE body cannot cause a full-replace drop — probably true, and exactly the kind of
|
|
"probably" that produced this record's two live misses — every operation carrying a request body
|
|
seeds the walk, and anything it surfaces must acquire a stated disposition.
|
|
|
|
TRANSITIVE, and that is load-bearing rather than thorough: `MultiCollectionItemRequest` and
|
|
`ArtworkContentTypeModel` are both NESTED, so a check reading only top-level request bodies
|
|
would have missed both of the schemas this guard exists because of. `_schema_refs` walks every
|
|
value of every dict and every list element, so `oneOf` (23 occurrences today), `items`,
|
|
`additionalProperties` and any future composition keyword are covered without naming them.
|
|
"""
|
|
schemas = doc["components"]["schemas"]
|
|
seeds: set[str] = set()
|
|
for operations in doc["paths"].values():
|
|
for operation in operations.values():
|
|
if isinstance(operation, dict) and operation.get("requestBody"):
|
|
_schema_refs(operation["requestBody"], seeds)
|
|
|
|
seen: set[str] = set()
|
|
stack = list(seeds)
|
|
while stack:
|
|
name = stack.pop()
|
|
if name in seen or name not in schemas:
|
|
continue
|
|
seen.add(name)
|
|
nested: set[str] = set()
|
|
_schema_refs(schemas[name], nested)
|
|
stack.extend(nested - seen)
|
|
return seen
|
|
|
|
|
|
def _optional_members(doc: dict) -> dict[str, list[str]]:
|
|
"""Schema -> its properties that sit OUTSIDE `required`, i.e. the ones that emit `?:`."""
|
|
out: dict[str, list[str]] = {}
|
|
for name, schema in doc["components"]["schemas"].items():
|
|
properties = schema.get("properties") or {}
|
|
if not properties:
|
|
continue
|
|
optional = sorted(set(properties) - set(schema.get("required") or []))
|
|
if optional:
|
|
out[name] = optional
|
|
return out
|
|
|
|
|
|
def _droppable(doc: dict) -> dict[str, list[str]]:
|
|
reachable = _request_reachable(doc)
|
|
return {n: m for n, m in _optional_members(doc).items() if n in reachable}
|
|
|
|
|
|
def test_the_derivation_reached_a_real_population() -> None:
|
|
"""Anti-vacuity: a broken `$ref` walk or parse must not read as 'nothing to rule on'.
|
|
|
|
What this test does NOT do, measured 2026-08-22 rather than assumed: it does not catch a
|
|
PARTIALLY broken walk. Deleting the transitive step from `_request_reachable` — so only the
|
|
schemas named directly on a request body resolve — leaves both floors satisfied and this test
|
|
GREEN. What reddens is `test_MUTATION_a_planted_optional_member_is_reported_as_MISSING`, whose
|
|
planted schema is reached through a nested `$ref` precisely so that it can, plus the disposition
|
|
test (the nested rows vanish and report as PHANTOM). So the floors below are the crude backstop
|
|
against a parse that reached nothing at all; the planted-member test is what actually holds the
|
|
walk honest, and it should be the one kept working if these two ever conflict.
|
|
"""
|
|
doc = _load()
|
|
reachable = _request_reachable(doc)
|
|
optional = _optional_members(doc)
|
|
assert len(reachable) >= MIN_REQUEST_REACHABLE_SCHEMAS, (
|
|
f"only {len(reachable)} schemas reachable from a request body — the $ref walk is broken, not the API"
|
|
)
|
|
assert len(optional) >= MIN_SCHEMAS_WITH_OPTIONAL_MEMBERS, (
|
|
f"only {len(optional)} schemas have a property outside `required` — suspect the parse"
|
|
)
|
|
|
|
|
|
def test_the_walks_ASSUMPTIONS_about_the_document_still_hold() -> None:
|
|
"""The derivation's scope mirrors two properties of the OpenAPI document. Check them.
|
|
|
|
`testing.guard-derives-population-from-source`: "When the scope itself MIRRORS an authoritative
|
|
source, the mirror needs its own equality check or a dated staleness marker, or the guard is
|
|
complete within a scope that has silently gone stale." Two such assumptions are baked into
|
|
`_request_reachable`, and both are true of the document today (2026-08-22) rather than
|
|
guaranteed by anything:
|
|
|
|
1. Request bodies are declared INLINE on the operation. If ASP.NET ever emits a
|
|
`components.requestBodies` bucket and operations `$ref` into it, the seed walk still finds
|
|
the `$ref` — but only because `_schema_refs` collects `#/components/schemas/...` names, so a
|
|
body referencing `#/components/requestBodies/X` would seed NOTHING and the schemas under it
|
|
would drop out of the population silently.
|
|
2. Every `$ref` in the document points into `#/components/schemas/`. `_schema_refs` matches on
|
|
that prefix, so a ref into any other bucket is invisible to it.
|
|
|
|
Both are cheap to assert and neither is asserted anywhere else, so a change in the emitter
|
|
would otherwise shrink this guard's population without failing anything.
|
|
"""
|
|
doc = _load()
|
|
buckets = set(doc.get("components", {}))
|
|
assert "requestBodies" not in buckets, (
|
|
"the OpenAPI document now declares components.requestBodies — `_request_reachable` seeds "
|
|
"only from inline operation bodies and `_schema_refs` only follows #/components/schemas/, "
|
|
"so schemas behind a shared request body are now INVISIBLE to this guard. Teach the walk "
|
|
"to resolve that bucket before deleting this assertion."
|
|
)
|
|
ref_buckets = set(re.findall(r'"#/components/([^/"]+)/', json.dumps(doc)))
|
|
assert ref_buckets <= {"schemas"}, (
|
|
f"$refs now point into {sorted(ref_buckets - {'schemas'})} as well as schemas; "
|
|
"`_schema_refs` matches only the schemas prefix and silently ignores the rest"
|
|
)
|
|
|
|
|
|
def test_every_droppable_request_schema_has_a_stated_disposition() -> None:
|
|
"""Set equality, both directions, accumulated into ONE message.
|
|
|
|
Failing fast on the first mismatch hands back one schema at a time and invites fixing them one
|
|
at a time, which is how #754's twin stayed hidden.
|
|
"""
|
|
droppable = _droppable(_load())
|
|
|
|
unruled = sorted(set(droppable) - set(DISPOSITIONS))
|
|
phantom = sorted(set(DISPOSITIONS) - set(droppable))
|
|
|
|
problems: list[str] = []
|
|
if unruled:
|
|
problems.append(
|
|
"MISSING — reachable from a request body with a member outside `required`, and "
|
|
"no disposition written down. Decide what happens to each and add a row:\n"
|
|
+ "\n".join(f" {n}: optional members {droppable[n]}" for n in unruled)
|
|
)
|
|
if phantom:
|
|
problems.append(
|
|
"PHANTOM — a disposition for a schema that is no longer request-reachable with an "
|
|
"optional member. Delete the row rather than leaving it claiming coverage:\n"
|
|
+ "\n".join(f" {n}" for n in phantom)
|
|
)
|
|
assert not problems, "\n\n".join(problems)
|
|
|
|
|
|
def test_every_disposition_uses_the_closed_vocabulary() -> None:
|
|
"""A free-text disposition would let a row read as considered while saying nothing."""
|
|
allowed = {COVERED, CREATE, TRIGGER, COMPUTED}
|
|
for name, (disposition, why) in DISPOSITIONS.items():
|
|
assert disposition in allowed, f"{name}: {disposition!r} is not one of {sorted(allowed)}"
|
|
assert len(why.strip()) >= 40, f"{name}: the reason is too thin to be a decision"
|
|
|
|
|
|
def test_MUTATION_a_planted_optional_member_is_reported_as_MISSING() -> None:
|
|
"""The checker-guard mutation proof (`testing.guard-ships-with-mutation-proof`).
|
|
|
|
Disarming a checker makes it ABSENT rather than red, so the mutation goes into the guarded
|
|
ARTIFACT: plant a request-reachable schema carrying a member outside `required` and require the
|
|
derivation to surface it. Mutating the checker's own population instead would be the trap that
|
|
record names — a shrunken population makes every real row report PHANTOM, so the proof would go
|
|
red on a false positive while saying nothing about the MISSING detection this row claims.
|
|
|
|
The planted schema is reached through a real request body, so this also exercises the
|
|
transitive `$ref` walk that the hand-written list twice failed to do by eye.
|
|
"""
|
|
doc = _load()
|
|
schemas = doc["components"]["schemas"]
|
|
|
|
# Deterministic, because `set` iteration order over strings varies per process: picking the
|
|
# host with `next(iter(...))` would silently vary WHICH walk depth the proof exercises from run
|
|
# to run, so a green run would not mean the same thing twice.
|
|
candidates = sorted(
|
|
n for n in _request_reachable(doc) if isinstance(schemas.get(n), dict) and schemas[n].get("properties")
|
|
)
|
|
assert candidates, "no request-reachable schema with properties — the walk is broken"
|
|
host = candidates[0]
|
|
|
|
schemas["PlantedDroppableRequest"] = {
|
|
"type": "object",
|
|
"properties": {"plantedRequired": {"type": "string"}, "plantedOptional": {"type": "string"}},
|
|
"required": ["plantedRequired"],
|
|
}
|
|
# Planted through a `oneOf` LIST rather than a bare dict `$ref`, because `_schema_refs` has two
|
|
# descent branches and only the dict one was exercised: deleting its list descent — which makes
|
|
# every `oneOf` reference invisible, 23 of them in the document today — left the whole file
|
|
# GREEN when measured 2026-08-22. A proof that cannot see half its own walk is the "green for
|
|
# the wrong reason" shape this repo keeps recording.
|
|
schemas[host]["properties"]["plantedLink"] = {
|
|
"oneOf": [{"type": "null"}, {"$ref": "#/components/schemas/PlantedDroppableRequest"}]
|
|
}
|
|
|
|
droppable = _droppable(doc)
|
|
assert "PlantedDroppableRequest" in droppable, (
|
|
"the planted schema was NOT surfaced — the derivation cannot see a droppable member, so a "
|
|
"green run of this file proves nothing"
|
|
)
|
|
assert droppable["PlantedDroppableRequest"] == ["plantedOptional"]
|
|
assert "PlantedDroppableRequest" not in DISPOSITIONS
|