fix(807): SPA full-replace bodies are built as Complete<T>, with a derived guard over droppable request members #827

Merged

timothy merged 21 commits from issue-807-spa-field-set into main 2026-08-23 02:58:21 +02:00

Conversation 1 Commits 21 Files Changed 32

+1343 -59

timothy commented 2026-08-23 02:22:36 +02:00

Owner

Copy Link

Copy Source

fixes #807

What this does

Two halves.

The type: Complete<T> (web/src/api/completeRequest.ts) maps a generated request type so every member is required, making an omission a hard tsc error however the schema was modelled. Applied at the full-replace API-wrapper parameters, so later callers inherit it, and at each construction site, which is what keeps TypeScript's excess-property check (the phantom direction) alive.

The guard: scripts/tests/test_optional_request_members.py derives, every run, every schema that can silently drop a member on a write — from ErsatzTV/wwwroot/openapi/v1.json, transitively from any operation's request body, inline bodies included — and asserts set equality both directions against a registry of per-schema dispositions. A new optional member anywhere in that reach fails until someone writes down what should happen about it.

The issue's premise was wrong, and the correction is the useful half

#807 said an optional DTO field added and not carried through would compile clean and drop silently. Verifying that first turned out to matter:

• Most nullable properties emit as required-and-nullable ("name": null | string), so most builders were already checked and the gap read as closed on inspection.
• But two full-replace PUTs carried a live optional member. Deleting weight: clampWeight(item.weight) from MultiCollectionsScreen.toItemRequest typechecked clean — and that PUT replaces the item list, so every weight would have reset to 1 on the next save. The screen carries a prose comment warning about exactly that. UpdateFFmpegProfileRequest.qsvPreferNativeDecoder had the same exposure.

A member is omittable exactly when it sits outside its schema's required array in the ASP.NET-produced OpenAPI document; the generator only passes that through.

Nine review rounds, and what they were all about

Rounds 1–7 returned BLOCKED. Every blocker was the same class: a hand-written claim about a population that was false. Prose exempting "create/update" (the two live drops). A table that replaced it and omitted ArtworkContentTypeModel, sorted out on its …Model name. "Anywhere in the request graph", while inline bodies were invisible. A record claiming construction-site coverage the tree lacked. Five resolver mutations that left the suite green.

So the fix stopped being "correct the list" and became "remove the mechanism":

• The population is derived, not enumerated. The dispositions are the reviewed scope, forced to exist by the equality assertion.
• The decision record no longer asserts present coverage at all — it states the rule, says plainly that nothing enforces the construction-site half (#820), and tells the reader to read the code rather than a summary. A set phrased in words rots exactly like a count.
• Every resolver branch, and every keyword whose required is unioned or discarded, is pinned by a constructed-schema case. That discipline is what would have caught the then/else polarity error, which was the silent-miss direction and shipped with a comment asserting the semantics had been considered.

Also fixed along the way: ruff format was CI-red and absent from my local gate for six commits.

Verification

typecheck 0 · eslint 0 · 1200/1200 web tests · build 0 · 956 passed, 2 skipped in scripts/tests · ruff check 0 · ruff format --check 0 (CI's git ls-files -z form) · catalog check clean · doc-narrative 0 new.

Rebased onto 7966e0816 (#826) and re-run there — the guard-inventory summary line collided with #825 and the merged truth is neither side's number (38/20/16), read off test_the_summary_counts_match_the_table rather than chosen.

Docs

docs/spa-conventions.md §4b (new), docs/decisions/records/testing/full-replace-asserts-field-list.md, docs/guard-inventory.md (row + scope note), docs/README.md task-signal map, catalog regenerated.

Deferred

• #820 — this guard proves Complete<T>'s semantics, not that it is applied; reverting one screen leaves it green. Named as residue in three places rather than implied closed.
• #822 — test_hook_fire_log.py's oracle is the shared production hook-fire directory, so any concurrent session reddens it with a message blaming the suite.

fixes #807 ## What this does Two halves. **The type**: `Complete<T>` (`web/src/api/completeRequest.ts`) maps a generated request type so every member is required, making an omission a hard `tsc` error however the schema was modelled. Applied at the full-replace API-wrapper parameters, so later callers inherit it, and at each construction site, which is what keeps TypeScript's excess-property check (the *phantom* direction) alive. **The guard**: `scripts/tests/test_optional_request_members.py` derives, every run, every schema that can silently drop a member on a write — from `ErsatzTV/wwwroot/openapi/v1.json`, transitively from any operation's request body, inline bodies included — and asserts set equality both directions against a registry of per-schema dispositions. A new optional member anywhere in that reach fails until someone writes down what should happen about it. ## The issue's premise was wrong, and the correction is the useful half #807 said an optional DTO field added and not carried through would compile clean and drop silently. Verifying that first turned out to matter: - Most nullable properties emit as **required-and-nullable** (`"name": null | string`), so most builders were already checked and the gap read as closed on inspection. - But two full-replace PUTs carried a **live** optional member. Deleting `weight: clampWeight(item.weight)` from `MultiCollectionsScreen.toItemRequest` typechecked **clean** — and that PUT replaces the item list, so every weight would have reset to 1 on the next save. The screen carries a prose comment warning about exactly that. `UpdateFFmpegProfileRequest.qsvPreferNativeDecoder` had the same exposure. A member is omittable exactly when it sits outside its schema's `required` array in the ASP.NET-produced OpenAPI document; the generator only passes that through. ## Nine review rounds, and what they were all about Rounds 1–7 returned BLOCKED. **Every blocker was the same class: a hand-written claim about a population that was false.** Prose exempting "create/update" (the two live drops). A table that replaced it and omitted `ArtworkContentTypeModel`, sorted out on its `…Model` name. "Anywhere in the request graph", while inline bodies were invisible. A record claiming construction-site coverage the tree lacked. Five resolver mutations that left the suite green. So the fix stopped being "correct the list" and became "remove the mechanism": - The population is **derived**, not enumerated. The dispositions are the reviewed scope, forced to exist by the equality assertion. - The decision record **no longer asserts present coverage at all** — it states the rule, says plainly that nothing enforces the construction-site half (#820), and tells the reader to read the code rather than a summary. A set phrased in words rots exactly like a count. - Every resolver branch, and every keyword whose `required` is unioned or discarded, is pinned by a constructed-schema case. That discipline is what would have caught the `then`/`else` polarity error, which was the silent-miss direction and shipped with a comment asserting the semantics had been considered. Also fixed along the way: `ruff format` was CI-red and absent from my local gate for six commits. ## Verification typecheck 0 · eslint 0 · 1200/1200 web tests · build 0 · **956 passed, 2 skipped** in `scripts/tests` · ruff check 0 · `ruff format --check` 0 (CI's `git ls-files -z` form) · catalog check clean · doc-narrative 0 new. Rebased onto `7966e0816` (#826) and re-run there — the guard-inventory summary line collided with #825 and the merged truth is neither side's number (38/20/16), read off `test_the_summary_counts_match_the_table` rather than chosen. ## Docs `docs/spa-conventions.md` §4b (new), `docs/decisions/records/testing/full-replace-asserts-field-list.md`, `docs/guard-inventory.md` (row + scope note), `docs/README.md` task-signal map, catalog regenerated. ## Deferred - **#820** — this guard proves `Complete<T>`'s semantics, not that it is *applied*; reverting one screen leaves it green. Named as residue in three places rather than implied closed. - **#822** — `test_hook_fire_log.py`'s oracle is the shared production hook-fire directory, so any concurrent session reddens it with a message blaming the suite.

timothy added 21 commits 2026-08-23 02:22:37 +02:00

feat(807): SPA full-replace bodies are built as Complete<T> ... 539fcde902

The six SPA screens that hand-build write-request objects field-by-field could
silently drop a DTO field on a full-replace write — #754's mechanism narrowed to
optional fields, filed as residue by `testing.full-replace-asserts-field-list`.

The premise that residue was filed on turned out to be wrong, and the correction
is the more useful half. The mechanism was LATENT, not live: the OpenAPI type
generator renders a nullable DTO property as a required member of a nullable type
(`"name": null | string`), not as an optional one, and none of the eleven request
schemas these builders target declares a single `?:` member. tsc was already
checking all of them. But it was doing so contingently — on a generator behaviour
nothing asserts, for a set of DTOs none of which had yet acquired an optional
property. Optional members already occur elsewhere in the same generated file, so
the contingency was one modelling decision away from lapsing silently.

`Complete<T>` removes the contingency rather than asserting the generator's
current behaviour: it maps a request type so every member is required, making an
omission a hard error however the generator emitted it. Applied at the
full-replace wrapper boundary so future callers inherit it without knowing it
exists, and at each construction site, which is what keeps the excess-property
(phantom-field) direction alive — two sites had lost it by building through an
inferred local or a spread rather than a fresh annotated literal.

Measured, not argued: injecting an optional member into `ScheduleItemRequest`
reddens `normalizeForSave` with TS2741 under `Complete<T>` and compiles CLEAN
without it. Mutating `Complete<T>` to drop `Required` reddens the guard's case 2;
`Partial<T>` reddens cases 2 and 4. Case 4 does not discriminate against the
realistic mutation and the guard says so, because no real request type can today
— which is the contingency itself.

The proof ships as `@ts-expect-error` cases re-executed on every `npm run
typecheck` (a marked CI step), not witnessed once by hand.

Verification: typecheck clean, eslint clean, 1200 web tests pass (new guard test
confirmed executed by name), 833 scripts/tests pass.

fixes #807

Decisions-Edit: yes

fix(807): cover the two LIVE uncovered full-replace PUTs, correct four prose claims ... b8c1de3368

Cold adversarial review returned BLOCKED. All findings confirmed by execution here
before acting on them.

H1 (blocker) — the change shipped with a live instance of the defect it claimed
was latent. `MultiCollectionItemRequest.weight` and
`UpdateFFmpegProfileRequest.qsvPreferNativeDecoder` are absent from their schemas'
`required` arrays, so both emit optional, and both are the body of a full-replace
PUT with a hand-built SPA body that the first commit did not cover. Deleting
`weight: clampWeight(item.weight)` from `MultiCollectionsScreen.toItemRequest`
typechecked CLEAN before this commit; the screen carries a prose comment warning
that this exact drop resets every weight to 1 on save. It now reddens with TS2741.

The bad reasoning is worth naming because it is a shape this repo keeps
recording: "create/update" vs "replace" was sorted on the verb in the wrapper
name rather than on what the endpoint does, and a property verified across the
eleven schemas the six named builders touch was then reported as a property of
the SPA. Coverage extended to `updateMultiCollection`, `updateFFmpegProfile`,
`replaceDeco`, and both `mediaSources` replace wrappers.

The FFmpeg fix surfaced a second instance one level down: the screen's `Draft`
type and the `sampleRequest` test fixture both omitted `qsvPreferNativeDecoder`,
so 30 green tests were exercising only the default. Both now `Complete<…>`.

H3 — "two sites had lost the phantom direction" is five, and the stated cause was
wrong. Measured: a fresh literal returned from a generic `.map` callback accepts a
phantom field even with no spread and no inferred local, because `map<U>` infers
`U` from the callback rather than from the target element type. Three of the five
contain neither a spread nor a local, so the documented rule would have taught a
reader that those are safe.

M4 — the generator was named as the cause of nullable-emits-as-required. It is a
pass-through (`required.has(name) ? '' : '?'`); the determinant is the `required`
array in the ASP.NET-produced `ErsatzTV/wwwroot/openapi/v1.json`. Pointed three
durable places at the right file.

M5 — the guard's header claimed every case reddens under every weakening, which
its own case-4 comment contradicted. Replaced with the measured table.

L6/L7/L8/N10 — record frontmatter now carries #807 so catalog-first lookup finds
it (catalog regenerated); `guard-inventory.md`'s hand-written out-of-population
list gains the third TS guard and now says why such a mirror goes stale silently;
`Complete` exported from the api barrel; §4b moved after §4a.

H2 is not fixed and is not implied closed: this guard proves `Complete<T>`'s
semantics, not that it is APPLIED — reverting one screen leaves it green. Filed
as #820 and named as residue on the record.

Verification: typecheck 0, eslint 0, 1200/1200 web tests, 833 passed 2 skipped in
scripts/tests, catalog regenerates to a zero diff, doc-narrative 0 warnings.

refs #820

Decisions-Edit: yes

docs(807): enumerate the coverage boundary instead of describing it ... c218daec74

"Full-replace gets `Complete<T>`, create/update does not" is a category sentence,
and it is the sentence that mis-sorted `updateMultiCollection` and
`updateFFmpegProfile` into the exempt bucket on the verb in their names. Replaced
with the enumeration it was standing in for.

Differencing `properties` against `required` across
`ErsatzTV/wwwroot/openapi/v1.json` yields thirteen schemas carrying at least one
optional property. Six reach a SPA write path; the record now lists each with its
disposition and the reason, and states the test that decides the column — does
this write replace a whole entity or collection — rather than the wrapper's name.

The three Auto-Tune schemas are exempt on a stated ground, not an omission: both
are POST creates (`/channels/auto-tune`, `/channels/from-lineup`) where an omitted
option correctly means "use the default". Requiring a caller to name 25 advanced
options would make the annotation noise, and a noisy annotation gets deleted.

Rebased onto 29d7a06e6 (#818) — the generated catalog conflicted and was
regenerated rather than hand-resolved. Re-ran the whole gate against the new base:
874 passed 2 skipped (up from 833; #818 added tests), typecheck 0, eslint 0,
1200/1200 web tests, catalog check clean.

Decisions-Edit: yes

docs(807): cite the inherit semantics behind the Auto-Tune exemption ... 247cdd9efe

The exemption was resting on my reading that "optional means use the default" on
a create. It rests on the code now: both endpoints return `CreatedResult` (201),
and `CreateChannelFromLineupClearField` documents that for the
template-inheritable advanced fields a null/omitted override means INHERIT the
template value, with a separate explicit `clear` list to force NONE.

So omission there is a THIRD STATE, not an oversight, and annotating those
requests `Complete<T>` would collapse inherit into explicit-null — a bug, not
missing coverage. Worth stating in the record because the exemption otherwise
reads as the same category-sort that produced the two live misses.

Also corrects two lines in the guard's own comment that survived the M4 fix: one
still attributed optionality to the generator script rather than to the schema's
`required` array, and one described the synthetic type's `optionalMember?` as a
shape the generator "does NOT currently emit" — it does, for the two live cases.

Adds the SPA pointer to the `docs/README.md` task-signal map row for a
hand-built request object, so the type-system half is reachable from the router
rather than only from the record.

Verification on this tree: typecheck 0, eslint 0, web build 0, 1200/1200 web
tests, 874 passed 2 skipped in scripts/tests, catalog check clean.

Decisions-Edit: yes

fix(807): derive the coverage boundary instead of writing it down a third time ... 685926a0d0

Round 2 returned BLOCKED. The enumeration added in the previous commit was
itself incomplete: it omitted `ArtworkContentTypeModel`, which carries three
members outside its `required` array and is reachable from the full-replace
`PUT /channels/{id}` via `UpdateChannelRequest.logo`. It was sorted out on its
name — `...Model` reads as a response model — which is the same failure the
enumeration was written to eliminate one level up, and it closed with a false
universal negative ("the remaining schemas ... are never request bodies") sixty
lines below this document's own warning against exactly that.

Two misses from one mechanism, so the mechanism goes rather than the list
getting a third patch.

`scripts/tests/test_optional_request_members.py` derives the population every
run from `ErsatzTV/wwwroot/openapi/v1.json`: every schema with a property
outside `required` that is TRANSITIVELY reachable from a POST/PUT/PATCH request
body, asserted set-equal in both directions against a registry of per-schema
dispositions. Transitivity is load-bearing — `MultiCollectionItemRequest` and
`ArtworkContentTypeModel` are both nested, so a top-level-only check would have
reproduced both misses. Population derived, dispositions hand-written as the
reviewed SCOPE, per testing.guard-derives-population-from-source.

Witnessed red: removing the `ArtworkContentTypeModel` disposition reports it as
MISSING with its three members. That mutation is DECLARED in
`mutation_manifest.py` and re-applied every suite, so the row grades MUTATION
rather than resting on this message. Renaming the key rather than deleting the
entry keeps the module importable, so the red is the MISSING direction and not
an ImportError reddening for the wrong reason.

ArtworkContentTypeModel's real disposition is COMPUTED, not COVERED: the three
members are get-only properties derived from `Path` and are never deserialized,
so omitting them drops nothing — and `Complete<T>` must NOT be applied there,
because it would force a caller to fabricate server-computed values in an
outbound request. That caveat is now in spa-conventions §4b, where a reader
following "annotate each construction site" would otherwise have produced a
wrong payload.

Also reconciles the contradiction round 2 found between the wrapper-boundary
rationale and the snapshot: 13 of ~33 full-replace PUT wrappers carry
`Complete<>` today. The rest are stated as still contingent — the difference
from before is that the contingency is now MONITORED by the derived test rather
than assumed.

L5 the phantom denominator is ten sites, not nine (five accepted, three of those
without a spread or a local). N6 fixture field placed alphabetically. N7 the
record's three-act framing trimmed to the current statement plus the rejected
one, so a cold reader does not have to work out which is live.

Verification: 879 passed 2 skipped in scripts/tests (up from 874), typecheck 0,
eslint 0, 1200/1200 web tests, web build 0, catalog check clean, doc-narrative 0.

Decisions-Edit: yes

fix(807): drop the verb allow-list from the derivation ... 35e5d42ad2

Self-review of the new guard, before round 3 reported: `_request_reachable`
seeded its walk from POST/PUT/PATCH request bodies only. That reads as obviously
right and already had a counterexample in the document — `DELETE
/api/v1/media-items` carries a request body.

A DELETE body probably cannot cause a full-replace drop. "Probably" is what
produced both of this issue's earlier misses, and a verb allow-list is another
hand-drawn edge on a population that has now been drawn wrongly twice. Every
operation carrying a request body now seeds the walk, so anything it surfaces
must acquire a stated disposition rather than being argued away in advance.

Effect measured: reachable schemas 141 -> 142, droppable set unchanged at 8, set
equality against the registry still holds. So this buys no new rows today; it
removes a place to be wrong later.

Also states in the docstring why the walk is transitive and why `_schema_refs`
recurses over every dict value and list element rather than naming composition
keywords — `oneOf` appears 23 times in the document today, and naming keywords
is the same frozen-filter mistake one level down.

879 passed, 2 skipped.

test(807): state what the anti-vacuity floors do NOT catch ... eff4bdfa22

Probed the new guard against a partially-broken derivation rather than only the
total-failure case the floors were written for: deleting the transitive step from
`_request_reachable`, so only schemas named directly on a request body resolve.

Both floors stay satisfied and `test_the_derivation_reached_a_real_population`
stays GREEN. What reddens is the planted-member test — its plant is reached
through a nested `$ref` exactly so that it can — and the disposition test, since
the nested rows vanish and report as PHANTOM.

So the floors are the crude backstop against a parse that reached nothing at all,
and the planted-member test is what actually holds the walk honest. Recorded in
the docstring, because a floor described as anti-vacuity reads as coverage it does
not have, and the next reader deciding which of these to keep working should know
which one is load-bearing.

test(807): assert the two document properties the walk silently depends on ... f878831e8f

`testing.guard-derives-population-from-source`: when a guard's scope mirrors an
authoritative source, the mirror needs its own check or it goes stale silently.
`_request_reachable` mirrors two properties of the OpenAPI document, both true
today and guaranteed by nothing:

1. Request bodies are declared inline on the operation. If ASP.NET emits a
   `components.requestBodies` bucket and operations `$ref` into it, the seed walk
   finds no `#/components/schemas/` name at all and every schema behind that
   shared body drops out of the population — silently, with the guard still green.
2. Every `$ref` points into `#/components/schemas/`. `_schema_refs` matches that
   prefix, so a ref into any other bucket is invisible to it.

Both are now asserted, with a failure message that says what to fix rather than
just that a fact changed. Neither was asserted anywhere else, so a change in the
emitter would have shrunk this guard's reach without failing anything — the
"complete within a scope that has silently gone stale" case.

880 passed, 2 skipped.

fix(807): ruff format, and re-pin case 4 to a member that discriminates ... c31c3cc556

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

docs(807): the fifth stale population claim, found by sweeping for set-sentences ... e8d4766ed9

Every one of this branch's three blockers was a hand-written claim about a
population that was false. Rather than wait for round 4 to find the next one, I
swept all four artifacts for sentences asserting something about a SET — a count,
a "none"/"every"/"only" — and checked each against what was measured.

Two were wrong:

- `spa-conventions.md` §4b still said "five of nine construction sites" after the
  denominator was corrected to ten in the record and in `completeRequest.ts`. The
  numerator was right, so nothing failed; the doc simply disagreed with the other
  two artifacts describing the same measurement.
- `completeRequest.guard.test.ts` said the derived guard "lists eight schemas" —
  a hand-written count of a DERIVED population, in the comment that exists to
  warn against exactly that. Replaced with a pointer to the guard and no number.

Neither is a behaviour change and neither would have failed a gate, which is the
point: these are the claims that read as checked and are not, and this branch has
now produced five of them.

ruff format 0, typecheck 0.

fix(807): cover inline request bodies; drop three unverifiable counts ... 5371b664e0

Round 4 returned BLOCKED. Its blocker is R2's failure mode repeated: a boundary
drawn by hand, stated as a universal, missing a case already present in the
document.

The guard's docstring claimed "a new optional member ANYWHERE in the request
graph" fails the test. It did not. `_optional_members` iterates
`components.schemas` and `_request_reachable` seeds from `$ref`s, so a request
body declared INLINE is invisible in both directions — and
`POST /api/v1/artwork/uploads` declares one, whose `file` and `target` sit
outside any `required` array and had no disposition. Planting a third optional
member into that body left the guard GREEN.

Rather than narrow the claim, `_inline_body_members` now walks inline bodies and
keys them `"<VERB> <path> (inline body)"` — there is no schema name to use, which
is precisely why the component walk could not see them. Composition arms are
unioned, because this body splits its properties across `allOf` and would
otherwise report none. The population goes 8 -> 9 and the uploads endpoint
acquires a TRIGGER disposition. The planted-member proof now plants into the
inline body too: deleting the inline branch reddens two tests, where before the
branch could have been deleted wholesale with everything green — the same gap
`oneOf` was in one round ago.

Live impact of the hole was nil (`artwork.ts` builds that body as hand-rolled
FormData, no generated type, create-not-replace), which is why it is worth fixing
rather than shrugging at: it was invisible, not harmless.

`createFFmpegProfile` was dispositioned COVERED while its wrapper was
unannotated, contradicting the vocabulary's own words and spa-conventions §4b
rule 1. Annotated; coverage no longer rests on the single `Draft` alias.

Three hand-written counts removed rather than corrected. "The eleven schemas the
six named builders target" is unverifiable — no natural cut reproduces eleven.
"The ten construction sites" has no derived definition of a construction site and
was already wrong once on this branch. And the record offered
`rg 'Complete<' web/src/api` as a reproduction for "most wrappers do not carry
it" — a command that counts the helper and its own test file and cannot produce
the claim. A command that does not reproduce is worse than none: it reads as
checked.

NITs: inventory row moved into alphabetical position; the manifest `why` now says
the rename fires BOTH directions, since `expect` names only the MISSING half; two
over-120-char comment lines wrapped.

Filed #822 for the `test_hook_fire_log` race surfaced while running the suite —
its oracle is the shared production hook-fire directory, so any concurrent
session reddens it with a message that blames the suite.

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.

refs #822

Decisions-Edit: yes

fix(807): resolve schema composition recursively, and by its actual semantics ... 9445330cb6

Self-attack on the inline-body walk added one commit ago, before round 5
reported. Probing it with constructed schemas rather than reading it found two
defects, and the second is the more serious:

  nested allOf inside allOf   -> reported NO properties (one-level handling)
  oneOf, x required in ONE arm -> reported x as REQUIRED

The second is backwards, not merely incomplete. `allOf` arms all apply, so their
`required` sets union. `oneOf`/`anyOf` arms are ALTERNATIVES, so a member is
genuinely required only when EVERY arm requires it — the sets intersect. Unioning
them marks a member required because one arm requires it, which hides the arm
that lets a client omit it. That is exactly the drop this guard exists to catch,
so the resolver was capable of concealing its own subject matter.

`_properties_and_required` now recurses and distinguishes conjunction from
disjunction, and `_optional_members` uses it too, so named component schemas and
inline bodies resolve composition identically instead of the named path silently
keeping the old flat behaviour.

Effect on today's document: none. Population stays 9, set equality holds. The
document exercises exactly one composition shape (a two-arm `allOf` in the
artwork upload body), which is precisely why every other branch was unexercised
prose — a resolver whose correctness rested on a case the corpus does not
contain.

So the six probe cases are now parametrised tests, pinned against constructed
schemas rather than against today's API. Witnessed: reverting the allOf recursion
reddens the nested case.

886 passed, 2 skipped (up from 880), ruff format 0, ruff check 0, catalog clean.

test(807): pin that the component walk and the inline walk COMPOSE ... 3758cb9112

`_properties_and_required` does not resolve `$ref`, so an `allOf` arm that is a
`$ref` contributes nothing to the inline key. I flagged that as a suspected hole
and it is not one: the referenced schema is a named component, so
`_request_reachable` seeds on it (`_schema_refs` finds a `$ref` anywhere, arms
included) and `_optional_members` walks it, and its optional members surface
under their own key. Verified by construction — a probe schema behind a `$ref`
arm reports `['probeOptional']` under `RefArmProbeSchema` while the sibling
inline arm reports `['probeInline']`.

Pinned as a test rather than left as a comment for two reasons. Resolving refs in
this resolver as well is the obvious "fix" and would report the same member under
two keys, which is worse than either. And a reader checking only one of the two
walks would reasonably conclude the case is uncovered and go build that fix.

887 passed, 2 skipped. ruff format 0.

fix(807): close the phantom gap at two sites, pin every resolver branch, stop ... d194091536

claiming coverage in prose

Round 5 returned BLOCKED. Its blocker was a durable coverage claim the tree
contradicted, and its other findings were the same class one layer down.

F1, the blocker. The record said `Complete<T>` "is applied … at each construction
site, which is what keeps the phantom direction alive." This PR annotated
`replacePathReplacements` and `replaceRemoteLibraryPreferences` at the wrapper
but left their construction sites as bare generic `.map` callbacks — the exact
shape spa-conventions §4b rule 2 says is unchecked. Injecting `phantomFieldZZZ`
into both literals typechecked CLEAN. Both sites are annotated now and both
reject the phantom with TS2353.

F2. Five mutations of the composition resolver left the whole file GREEN:
dropping `anyOf` entirely, making the alternatives loop non-recursive, taking
only the first arm's properties, ignoring top-level `required` when alternatives
are present, and dropping `items` descent. Three of those are silent-miss
direction — a droppable member the guard reports nothing about. The six existing
cases pinned the `allOf` path and the `oneOf` intersection and nothing else, so
the previous commit's "witnessed" claim covered one branch of five. All five now
redden; fourteen parametrised cases instead of six.

F3. "A new optional member anywhere in the request graph fails this test" was
false in three constructed shapes. Two are now handled — an inline body that is
an ARRAY of objects, and properties under `if`/`then` — and the third was a real
collision: `_inline_body_members` keyed on verb+path with no media type, so a
second `[Consumes]` on one action made the later body silently overwrite the
earlier. Keyed by media type now. The universal is replaced in both copies by the
actual boundary.

`if` is treated as a condition, not a constraint: its properties are collected (a
client may send them) and its `required` is discarded, because that `required`
selects a branch rather than obliging anyone to send anything.

F4/F5. The record characterised the annotated wrappers as "the ones reachable
from a droppable schema, plus the six builders" — five fall outside both. And
`oneOf` and `anyOf` were concatenated into one alternatives list, which
intersects `required` ACROSS the two keywords; they are conjunctive with each
other, so each intersects within itself and the results union. Safe-direction but
wrong, and untested.

The prose change is the one that matters. Five rounds, five blockers, every one a
hand-written claim about a population. So the record no longer asserts present
coverage at all: it states the RULE, says plainly that nothing enforces the
second half (#820), and tells the reader to read the code rather than a summary.
A set phrased in words rots exactly like a count — deleting the counts in an
earlier commit while leaving the worded set was treating the symptom.

895 passed 2 skipped (up from 887), ruff format 0, ruff check 0, typecheck 0,
eslint 0, 1200/1200 web, build 0, catalog clean.

fix(807): the last unannotated item builder, and three more resolver blind spots ... aa24e57343

Self-attack ahead of round 6, on the two things every round has found defective:
the newest hand-written code, and the completeness of an enumeration.

CONSTRUCTION SITES, enumerated rather than sampled. 15 wrappers now take
`Complete<>`; walking each one's nested item type to its builder leaves exactly
one unannotated — `DecosScreen.breakToRequest` / `toReplaceRequest`, feeding the
`Complete<>`-annotated `replaceDeco`. Round 5 called this non-blocking because a
declared return type is already a contextual position, so the phantom direction
fires there. True, but the MISSING direction does not: a new optional member on
`DecoBreakContentRequest` would have been droppable. Both annotated.

RESOLVER, probed with constructed schemas rather than read. Three more shapes it
could not see, all silent misses:

  items in the 3.0 TUPLE form (a list)   -> nothing
  prefixItems (the 2020-12 spelling)     -> nothing
  dependentSchemas                       -> nothing

`isinstance(items, dict)` skipped the list form silently. All three are handled
now; `dependentSchemas` gets the `if` treatment — properties collected because a
client may send them, conditional `required` discarded because it binds only when
the trigger key is present.

Two shapes probed and confirmed already CORRECT, pinned so they stay that way:
`additionalProperties` contributes no members (it describes values under
arbitrary keys, not named members), and `not` cannot make a member required.
`else` was already handled; it now has a case of its own rather than riding on
`then`'s.

Seven new parametrised cases, 27 in the file. Population unchanged at 9 with set
equality — every one of these is a latent hole, not a live miss, which is the
point: "it does not appear in today's document" is the reasoning that has been
wrong five times on this issue.

902 passed 2 skipped (up from 895), ruff format 0, typecheck 0, eslint 0,
1200/1200 web, build 0.

docs(807): record what happens on a cyclic schema, measured both ways ... 4ce590662a

The resolver has no cycle guard and that is deliberate, but the reasoning was
nowhere, so the next reader would either add a redundant guard or wonder.

Measured 2026-08-23:

- `_properties_and_required` raises `RecursionError` on a dict that contains
  itself by object identity. That input cannot come from `json.load` — JSON has
  no back-references — and the failure is a loud red rather than a silent wrong
  answer, so it needs no defence.
- The cycle a real document CAN express is a mutual `$ref`. `_request_reachable`
  handles it: A -> B -> A resolves to {A, B} and terminates, because of its
  `seen` set. This resolver never follows `$ref` at all, which is what makes the
  first case the only one available.

Stated where the recursion is, since "why is there no cycle guard here" is
answerable only by knowing both halves.

fix(807): then/else are ALTERNATIVES — stop unioning their required ... 4d7fd26a3e

Round 6 returned BLOCKED on a polarity error I committed in the block that fixed
the previous polarity error.

`then` and `else` were in the conjunctive list with `allOf`, so their `required`
unioned. They are MUTUALLY EXCLUSIVE: a member required only under `then` is
omittable whenever `if` does not match. The guard reported it as required — the
silent-miss direction, which is the one this file exists to catch. Twelve lines
below the defect the same function warns, about `oneOf`, that getting exactly
this backwards "is not a detail".

The comment made it worse, not better: it asserted the semantics had been
considered, which is what stops the next reader checking. Its reasoning was right
for properties (union them — a client may send them) and silently inverted for
`required`.

`if`, `then`, `else` and `dependentSchemas` now share one loop with one rule:
collect properties, discard `required`. They discard for three different reasons
— a branch selector, a branch that may not be taken, a trigger key that may be
absent — but the rule is identical, so one loop beats three arguments.

Why fix a keyword the emitter has never produced: this head added `prefixItems`
and `dependentSchemas` handling on the stated grounds that "it does not appear in
today's document" has been wrong repeatedly here. That standard cannot apply to
three keywords and not the fourth — and a wrongly-handled keyword is worse than
an unhandled one, because it ships a comment claiming it was thought about.

The real lesson is narrower than "handle more keywords": for EVERY keyword whose
`required` is unioned or discarded, ship a case that puts a `required` under it.
None of the 27 cases did, which is why both this and the unpinned `if` discard
were invisible. Three such cases added.

Also closes the last blind spot with a mechanism behind it: an inline object
under `properties`. The generator recurses into it and emits `"b"?:`, so it is
genuinely droppable; the resolver returned nothing. Now reported with a DOTTED
path, so `logo.contentType` cannot be mistaken for a top-level `contentType` in a
MISSING diagnostic. `patternProperties` is correctly unhandled (no fixed members,
like `additionalProperties`) and now has a case saying so rather than being
silently absent.

The docstring now names the resolver's exact reach instead of gesturing at it,
and the `$ref` skip in the inline walk is marked as belt-and-braces rather than
reading as protective — deleting it changes no result.

Population unchanged at 9, set equality holds. 33 resolver cases, 908 passed 2
skipped, ruff 0/0, typecheck 0, eslint 0, 1200/1200 web, build 0, catalog clean.

fix(807): the dotted path I just added could MASK a nested member ... 021f3a4bb9

Self-attack on the previous commit's new code, in the function every round has
found defective — including, twice now, in the block that fixed the last defect.

Qualifying a nested inline object's members as `parent.child` assumes no property
name contains a dot. JSON permits one. Measured: a schema with a literal `a.b`
member alongside a nested `a` -> `b` collides, and when the literal is `required`
the nested optional member is MASKED — it vanishes from the population entirely.
Silent-miss direction again, introduced by the fix for a silent miss.

Zero dotted property names in the document today, which is exactly the reasoning
that has been wrong five times on this issue, so the assumption is asserted
rather than relied on. The separator is a named constant, the assumptions test
now carries a third clause, and the failure message says what to change instead
of only reporting that a fact moved. Witnessed: planting a schema with an `a.b`
member fires it.

908 passed 2 skipped, ruff format 0.

fix(807): the collision check did not walk the tree it was checking ... 161c788a70

Third round of the same shape in three consecutive commits, and the cleanest
statement of it yet.

The assertion added one commit ago — no property name may contain the separator —
iterated only the top-level `properties` of each named component schema. The
resolver it protects walks the WHOLE tree. So the two disagreed exactly where it
matters: a dotted name on a nested inline object, or inside an inline request
body, was undetected, and those are precisely the places the resolver qualifies a
name and can therefore collide. A guard for a hole, with the same hole.

`_property_names_containing_the_separator` now recurses over the parsed document
the way `_schema_refs` does. Witnessed across five shapes — top-level on a named
schema, a nested inline object, an `allOf` arm, under `items`, and inside an
inline request body — all detected; the middle three were not, before.

The transferable rule, which this issue has now paid for three times in a row: a
check must walk the SAME tree as the thing it checks. Not a similar tree, not the
obvious subset — the same one. Each of these three defects was introduced by the
commit fixing the previous one, so the pattern is not carelessness about the
subject, it is that a fix written to close one shape gets tested against that
shape only.

908 passed 2 skipped, ruff check 0, ruff format 0.

fix(807): remove the collision class instead of asserting it away ... 424ffb573b

Round 7 reviewed the assertion added two commits ago and found it covered 1 of
the 5 nodes where the collision fires — the four it missed being the ones that
matter, since they are where the walk actually qualifies a name. I had already
widened it to walk the whole tree, but the reviewer's recommendation is better
than either version and is taken here: restructure so there is nothing to assert.

A nested inline object lives in its OWN namespace. The defect was qualifying its
members as `parent.child` BEFORE subtracting `required`, which merges the two
namespaces and lets a literal `a.b` member mask a nested `a` -> `b` one. Now
`_optional_of` subtracts inside each namespace and qualifies only the survivors,
so the sets are never mixed. No separator to defend, no assumption to assert, and
`NESTED_SEPARATOR` and its whole-tree checker are gone.

Verified at all five sites — top level of a named schema, one level down, inside
an `allOf` arm, under `items`, and inside an inline request body — every one now
surfaces (`a.b`, `outer.a.b`, `a.b`, `a.b`, `a.b`); four were masked before.

The reviewer flagged a trap in this fix and it is real: keeping a 2-tuple
compatibility shim (`return properties | nested, required`) re-creates the mask
at the merge boundary while the suite stays green. So `_resolve` returns three
sets and every caller changed. There is now a case that reddens exactly that
shim.

Residual, stated rather than hidden: a literal `a.b` and a nested `a` -> `b` that
are BOTH optional conflate into one reported string. That is a diagnostic
ambiguity, not a miss — the schema still enters the population and still forces a
disposition.

Also from round 7: `prefixItems` was the one keyword whose `required` handling no
case pinned (its only case carried none), so discarding it stayed green; it has a
case now. And the assumptions docstring is back to describing two assertions
rather than three, which the separator removal resolved on its own.

REBASED onto ad31a0285 (#825). That change edits the same guard-inventory summary
line this branch does, and the merged truth is neither side's number: 38 guards /
20 proof files / 16 with a mutation proof. Resolved by reading it off
`test_the_summary_counts_match_the_table` rather than by picking a side.

One local trap worth recording: a stale `scripts/**/__pycache__` made pytest
report this commit's new case failing while the source could not produce that
result — `_optional_of` returned the right value when called directly. Clearing
the cache resolved it. I nearly "fixed" correct code on the strength of a red.

946 passed 2 skipped on the new base, ruff check 0, ruff format 0, typecheck 0,
eslint 0, 1200/1200 web, build 0, catalog clean, doc-narrative 0 new.

test(807): make "threaded through every branch" a checked property ... dafa56452c

`_resolve` returns three sets and the third has to reach every composition site.
That was a claim in a docstring; each site is a separate opportunity to forget,
and forgetting is silent — no error, just a smaller population.

Ten sites enumerated and asserted: top level, `allOf` arm, `if`, `then`, `else`,
`dependentSchemas`, `oneOf` arm, `anyOf` arm, `items`, `prefixItems`. All ten
currently thread it. Witnessed load-bearing by dropping the threading at three of
them — `allOf` reddens 1 case, `oneOf`/`anyOf` 2, `items` 2.

The value is not today's ten passes; it is that a NEW composition keyword has one
visible place to be added to, instead of the omission being spread across the
resolver where nothing would report it.

956 passed 2 skipped, ruff check 0, ruff format 0.

timothy changed title from fix(807): SPA full-replace bodies are built as Complete&lt;T&gt;, with a derived guard over droppable request members to fix(807): SPA full-replace bodies are built as Complete<T>, with a derived guard over droppable request members 2026-08-23 02:40:02 +02:00

timothy commented 2026-08-23 02:40:20 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ dafa564

Nine cold adversarial rounds (Opus, worktree-isolated). Rounds 1-7 BLOCKED, every blocker real and every one the same class: a hand-written claim about a population that was false. Two LIVE silent drops found and fixed (MultiCollectionItemRequest.weight, UpdateFFmpegProfileRequest.qsvPreferNativeDecoder). Final round could not construct a wrong answer, find an unpinned clause, or falsify a prose claim.

Review-verdict: MERGEABLE @ dafa564 Nine cold adversarial rounds (Opus, worktree-isolated). Rounds 1-7 BLOCKED, every blocker real and every one the same class: a hand-written claim about a population that was false. Two LIVE silent drops found and fixed (MultiCollectionItemRequest.weight, UpdateFFmpegProfileRequest.qsvPreferNativeDecoder). Final round could not construct a wrong answer, find an unpinned clause, or falsify a prose claim.

timothy merged commit 3e2c07b42f into main 2026-08-23 02:58:21 +02:00

timothy deleted branch issue-807-spa-field-set 2026-08-23 02:58:27 +02:00

timothy referenced this pull request 2026-08-23 02:59:13 +02:00

The SPA's hand-built request objects can silently drop an OPTIONAL DTO field #807

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#827