timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

Complete<T> proves its semantics but not its APPLICATION — nothing derives the set of SPA full-replace construction sites #820

New Issue
Closed
opened 2026-08-22 22:10:58 +02:00 by timothy · 3 comments
timothy commented 2026-08-22 22:10:58 +02:00
Owner
Copy Link
Copy Source

Residue from #807, named there rather than implied closed. Filed by the adversarial review of PR for #807, which demonstrated it by execution.

The gap

#807 made SPA full-replace request bodies Complete<T> (web/src/api/completeRequest.ts) so a builder that omits a schema member fails npm run typecheck. Its guard (web/src/api/completeRequest.guard.test.ts) proves the TYPE's semantics — that Complete<T> rejects a missing member and a phantom one.

It does not prove the type is APPLIED anywhere. Stripping every Complete<> from web/src/api/blocks.ts and web/src/screens/BlocksScreen.tsx — i.e. reverting one of the covered screens to its pre-#807 state — leaves npm run typecheck clean and the guard test green. npm run lint fires only on the now-orphaned import type { Complete }; a regressor who removes the import too, or a newly added full-replace wrapper that never had one, produces no signal at all.

This is testing.guard-derives-population-from-source exactly: the population of construction sites is derived from nothing, so the guard cannot see the site that is missing. #807 found its own two uncovered wrappers by hand-grep, which is the method this repo has recorded as insufficient.

Why this is the harder half

Per testing.guard-derives-population-from-source, a population of sites in code has no external enumerator and is explicitly scoped out of that record (tracked in #777). So this is not a set-equality-against-a-source problem.

But the repo already owns a working precedent for exactly this shape: web/src/api/pageSizeScan.ts walks the SPA with the TypeScript compiler API and pageSizeCallSites.guard.test.ts cross-checks the discovered site set against a hand-reviewed registry in both directions (new-and-unregistered vs registered-but-gone), with anti-vacuity floors and an identity deliberately keyed (file, kind, value) rather than line/column. That is the model to copy.

Sketch

Discover, via the compiler API, every call to a web/src/api/* write wrapper whose body parameter type is (or should be) a full-replace request, and every object literal flowing into one. Cross-check against a registry recording, per site, whether it is Complete<>-annotated and why if not. Read pageSizeCallSites.guard.test.ts's doc comment first — it documents its own residual blind spots (spreads, positional args, forwarded expressions), and those apply here too.

Worth deciding early whether the cheaper answer is to make the wrapper signatures the ONLY seam (so a site cannot construct a body except through a Complete<>-typed parameter) rather than scanning for sites at all — dedup by construction beats enumeration, per docs/defect-shapes-773.md §4 detector C.

Done-when

  • The population is DERIVED, not hand-listed — the droppable SCHEMAS (parsed from the generated v1.d.ts) and the Complete<…> annotation sites (SPA AST, intersected with the git index). Reworded from "full-replace construction sites (or wrapper signatures)": a wrapper-signature population was built and REMOVED after four review rounds defeated it, and the per-construction-site population is scoped out by testing.guard-derives-population-from-source (#777). Rationale and the offer to revert this rewording are in the scope-divergence comment below
  • Both directions reported separately, with an anti-vacuity floor
  • Witnessed red by reverting one covered site to its pre-#807 form
  • A row in docs/guard-inventory.md if the guard lands in a globbed population, or an explicit note if it stays outside it
  • Adversarial review passed
Residue from #807, named there rather than implied closed. Filed by the adversarial review of PR for #807, which demonstrated it by execution. ## The gap #807 made SPA full-replace request bodies `Complete<T>` (`web/src/api/completeRequest.ts`) so a builder that omits a schema member fails `npm run typecheck`. Its guard (`web/src/api/completeRequest.guard.test.ts`) proves the TYPE's semantics — that `Complete<T>` rejects a missing member and a phantom one. It does not prove the type is APPLIED anywhere. Stripping every `Complete<>` from `web/src/api/blocks.ts` and `web/src/screens/BlocksScreen.tsx` — i.e. reverting one of the covered screens to its pre-#807 state — leaves `npm run typecheck` clean and the guard test green. `npm run lint` fires only on the now-orphaned `import type { Complete }`; a regressor who removes the import too, or a **newly added** full-replace wrapper that never had one, produces no signal at all. This is `testing.guard-derives-population-from-source` exactly: the population of construction sites is derived from nothing, so the guard cannot see the site that is missing. #807 found its own two uncovered wrappers by hand-grep, which is the method this repo has recorded as insufficient. ## Why this is the harder half Per `testing.guard-derives-population-from-source`, a population of **sites in code** has no external enumerator and is explicitly scoped out of that record (tracked in #777). So this is not a set-equality-against-a-source problem. But the repo already owns a working precedent for exactly this shape: `web/src/api/pageSizeScan.ts` walks the SPA with the **TypeScript compiler API** and `pageSizeCallSites.guard.test.ts` cross-checks the discovered site set against a hand-reviewed registry in **both** directions (new-and-unregistered vs registered-but-gone), with anti-vacuity floors and an identity deliberately keyed `(file, kind, value)` rather than line/column. That is the model to copy. ## Sketch Discover, via the compiler API, every call to a `web/src/api/*` write wrapper whose `body` parameter type is (or should be) a full-replace request, and every object literal flowing into one. Cross-check against a registry recording, per site, whether it is `Complete<>`-annotated and why if not. Read `pageSizeCallSites.guard.test.ts`'s doc comment first — it documents its own residual blind spots (spreads, positional args, forwarded expressions), and those apply here too. Worth deciding early whether the cheaper answer is to make the wrapper signatures the ONLY seam (so a site cannot construct a body except through a `Complete<>`-typed parameter) rather than scanning for sites at all — dedup by construction beats enumeration, per `docs/defect-shapes-773.md` §4 detector C. ## Done-when - [x] The population is DERIVED, not hand-listed — the droppable SCHEMAS (parsed from the generated `v1.d.ts`) and the `Complete<…>` annotation sites (SPA AST, intersected with the git index). **Reworded** from "full-replace construction sites (or wrapper signatures)": a wrapper-signature population was built and REMOVED after four review rounds defeated it, and the per-construction-site population is scoped out by `testing.guard-derives-population-from-source` (#777). Rationale and the offer to revert this rewording are in the scope-divergence comment below - [x] Both directions reported separately, with an anti-vacuity floor - [x] Witnessed red by reverting one covered site to its pre-#807 form - [x] A row in `docs/guard-inventory.md` if the guard lands in a globbed population, or an explicit note if it stays outside it - [x] Adversarial review passed
timothy added the priority: mediumfrontend labels 2026-08-22 22:10:58 +02:00
timothy added the in-progress label 2026-08-29 11:20:45 +02:00
timothy commented 2026-08-29 11:20:45 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this (Claude Code session main-3, 2026-08-29). Checked before claiming: no remote branch naming 820, no open PR with a fixes/refs #820 body (the two Renovate PRs matching the string carry it in a version/changelog line, not a reference), no prior comments, fresh git fetch origin main at 8aeacd534.

Starting from the question the body raises rather than the sketch: whether the wrapper signatures can be made the ONLY seam, so a body cannot be constructed except through a Complete<>-typed parameter. Dedup by construction beats enumeration, and it would make the derived-population problem moot rather than solved. If that does not hold for every wrapper, the fallback is the pageSizeScan.ts compiler-API model, both directions with an anti-vacuity floor.

Claiming this (Claude Code session `main-3`, 2026-08-29). Checked before claiming: no remote branch naming 820, no open PR with a `fixes/refs #820` body (the two Renovate PRs matching the string carry it in a version/changelog line, not a reference), no prior comments, fresh `git fetch origin main` at `8aeacd534`. Starting from the question the body raises rather than the sketch: whether the wrapper signatures can be made the ONLY seam, so a body cannot be constructed except through a `Complete<>`-typed parameter. Dedup by construction beats enumeration, and it would make the derived-population problem moot rather than solved. If that does not hold for every wrapper, the fallback is the `pageSizeScan.ts` compiler-API model, both directions with an anti-vacuity floor.
timothy commented 2026-08-29 21:17:14 +02:00
Author
Owner
Copy Link
Copy Source

Scope divergence on Done-when box 1 — flagging rather than quietly ticking it

Box 1 asks that "the population of full-replace construction sites (or wrapper signatures) is DERIVED, not hand-listed". What shipped derives a third population: the droppable schemas (parsed from the generated v1.d.ts) and the Complete<…> annotation sites (SPA AST ∩ the git index). Neither of the two the box names is in the final branch. Recording why, because this is a scope reduction and a reviewer should be able to push back on it.

Wrapper signatures were built, and removed. scanWriteWrappers shipped in an intermediate commit and was defeated four separate ways across review rounds:

  1. it attached the obligation per-schema, so a test fixture could discharge it;
  2. it treated schema reachability as protection — Complete<T> is shallow, so a wrapper annotation never reached a nested request type (confirmed by execution: with Complete<UpdateMultiCollectionRequest>, { items: [{}] } typechecks clean);
  3. it found the body by a parameter literally named body, so renaming it to payload made a wrapper vanish from the population — and separately, bodies built as typed locals were invisible;
  4. finally, an export function → export const refactor removed real, measured protection while the AST scan and its supposedly independent regex cross-check went blind together, because both keyed on the token export function.

Five defects from one mechanism, each found only by review. process.enumerate-workaround-behaviors-before-deleting and the withdrawn test_review_verdict_vocabulary_parity.py (six rounds, then deleted) both say to remove the mechanism rather than patch a sixth time, and the reviewer who found #4 independently recommended narrowing rather than withdrawing. ~250 lines went.

Construction sites are scoped out by an existing decision. testing.guard-derives-population-from-source explicitly excludes "sites in code" as having no external enumerator; that is tracked in #777. The issue body itself says as much under "Why this is the harder half".

So box 1 as written is not satisfiable with the tooling available, and I have reworded it to the delivered scope with this comment as the pointer. If you would rather the box stayed as written and the issue stayed open for the wrapper/site half, say so and I will reopen it — the removal is well-evidenced but it is less than the box asked for.

The other four boxes are met on their own terms: both directions are reported separately with anti-vacuity floors; the witnessed red was measured independently by two reviewers (stripping Complete<> from MultiCollectionsScreen.toItemRequest — the exact pre-#807 form — reddens); the guard-inventory carries a row plus the out-of-globbed-population note; and adversarial review is at five rounds.

What the guard actually does not cover, stated here because the docs now lead with it: the obligation is per-SCHEMA, not per-site and not per-wrapper, so moving an annotation off an API wrapper onto another production file stays green; it is token presence, so a dead export type X = Complete<Y> discharges it; and the phantom direction is not checked at all.

## Scope divergence on Done-when box 1 — flagging rather than quietly ticking it Box 1 asks that **"the population of full-replace construction sites (or wrapper signatures) is DERIVED, not hand-listed"**. What shipped derives a *third* population: the droppable **schemas** (parsed from the generated `v1.d.ts`) and the **`Complete<…>` annotation sites** (SPA AST ∩ the git index). Neither of the two the box names is in the final branch. Recording why, because this is a scope reduction and a reviewer should be able to push back on it. **Wrapper signatures were built, and removed.** `scanWriteWrappers` shipped in an intermediate commit and was defeated four separate ways across review rounds: 1. it attached the obligation per-schema, so a test fixture could discharge it; 2. it treated schema *reachability* as protection — `Complete<T>` is shallow, so a wrapper annotation never reached a nested request type (confirmed by execution: with `Complete<UpdateMultiCollectionRequest>`, `{ items: [{}] }` typechecks clean); 3. it found the body by a parameter literally named `body`, so renaming it to `payload` made a wrapper vanish from the population — and separately, bodies built as typed locals were invisible; 4. finally, an `export function` → `export const` refactor removed real, measured protection while the AST scan **and** its supposedly independent regex cross-check went blind *together*, because both keyed on the token `export function`. Five defects from one mechanism, each found only by review. `process.enumerate-workaround-behaviors-before-deleting` and the withdrawn `test_review_verdict_vocabulary_parity.py` (six rounds, then deleted) both say to remove the mechanism rather than patch a sixth time, and the reviewer who found #4 independently recommended narrowing rather than withdrawing. ~250 lines went. **Construction sites are scoped out by an existing decision.** `testing.guard-derives-population-from-source` explicitly excludes "sites in code" as having no external enumerator; that is tracked in **#777**. The issue body itself says as much under "Why this is the harder half". **So box 1 as written is not satisfiable with the tooling available**, and I have reworded it to the delivered scope with this comment as the pointer. If you would rather the box stayed as written and the issue stayed open for the wrapper/site half, say so and I will reopen it — the removal is well-evidenced but it *is* less than the box asked for. The other four boxes are met on their own terms: both directions are reported separately with anti-vacuity floors; the witnessed red was measured independently by two reviewers (stripping `Complete<>` from `MultiCollectionsScreen.toItemRequest` — the exact pre-#807 form — reddens); the guard-inventory carries a row plus the out-of-globbed-population note; and adversarial review is at five rounds. **What the guard actually does not cover, stated here because the docs now lead with it:** the obligation is per-SCHEMA, not per-site and not per-wrapper, so moving an annotation off an API wrapper onto another production file stays green; it is token *presence*, so a dead `export type X = Complete<Y>` discharges it; and the phantom direction is not checked at all.
timothy commented 2026-08-30 00:07:31 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #883 (squashed to e8f80c42c). web/src/api/completeAnnotationScan.ts (compiler-API scanners) + completeAnnotations.guard.test.ts (the guard) + a synthetic-source fixture suite, plus scripts/tests/test_complete_annotation_dispositions.py cross-checking the SPA disposition table against the authoritative Python one. Two derived populations — the Complete<…> annotations (SPA AST ∩ git index) and the droppable schemas (parsed from the generated v1.d.ts). Scope was reduced: see the scope-divergence comment above; box 1 was reworded and the offer to revert that rewording stands.

Root cause: Complete<T> proved its own semantics and nothing proved its APPLICATION. Worse, test_optional_request_members.py's COVERED disposition was worded "the SPA builds this body and the builder is annotated Complete<T>" — a claim about another language's source that nothing verified, so a COVERED row and a SPA with the annotation deleted were indistinguishable to the entire suite. The same file's COMPUTED prohibition (§4b's "do NOT annotate a schema whose optional members are computed server-side") was prose with no executable form.

Decisions/conventions changed: none added. testing.full-replace-asserts-field-list updated — its mechanics: now states which half is machine-checked, and the body's residue paragraph was corrected (it had described the guard as covering the wrapper half, which was measured false). docs/spa-conventions.md §4b now leads with what is NOT checked.

Reusable knowledge:

  • A registry row that describes ANOTHER language's source is an assertion, not a check. The COVERED wording had been true when written and nothing would have reported the day it stopped being.
  • Complete<T> is SHALLOW, and reachability is not protection. Complete<UpdateMultiCollectionRequest> requires items to be present and requires nothing of each item — { items: [{}] } typechecks clean. A wrapper annotation never protects a nested request type; only the nested builder's own annotation does. I got this wrong for a full round and the doc sentence justifying it was false.
  • A deep Complete<T> works but must not be applied blanket-wise. Probed against the real generated types: it catches a missing nested member, preserves the null arm of null | Array<X>, keeps explicit undefined legal and still rejects phantoms. NOT adopted: UpdateChannelRequest.logo reaches ArtworkContentTypeModel, where annotating would force a caller to fabricate server-computed values, and it closes only the missing direction — the phantom direction needs a fresh literal in a contextually typed position, which a generic .map callback's return is not. Recorded so it is not re-proposed on plausibility.
  • Two scanners keyed on the same token are ONE mechanism, not two. The AST wrapper scan and its "independent" regex cross-check both matched export function, so an export function → export const refactor blinded both at once while I had written that "the two fail in unrelated ways".
  • A hand-written mirror of a DTO wearing a Complete<> is the #754 mechanism in disguise — the annotation proves the caller filled in the MIRROR.

Verification: web 1319 tests green; scripts/tests 1243 green (2 skipped); typecheck + eslint + ruff clean; decisions-validate: OK; build_decisions_catalog --check up to date; check-doc-narrative --diff 0 warnings; npm run check:api and npm run build clean; guard-inventory summary counts machine-checked. CI on 68caecb green (image-push correctly skipped for a PR). Nine mutations witnessed by hand during development; the Python cross-check carries a DECLARED clause mutation in mutation_manifest.py, executed by the harness every run.

Deferred:

  • The per-CONSTRUCTION-SITE population (the PHANTOM direction) — sites-in-code, tracked in #777.
  • The wrapper-level obligation. A scanWriteWrappers was built and REMOVED after four review rounds each defeated it; the removal rationale is in docs/guard-inventory.md so the cheap version is not rebuilt without reading why it failed.
  • A SECOND setupFiles entry could discharge the obligation (three-site coordinated edit). Stated as a residual rather than closed; deriving setupFiles needs another virtual-module plugin.
  • Observation, not deferred work: web/vite.config.ts:36 still describes vitest's default include as **/*.{test,spec}.*; the real 4.1.9 default is **/*.{test,spec}.?(c|m)[jt]s?(x). Pre-existing (from #445), conservative in direction, left alone as outside this change's scope.

Docs updated: docs/spa-conventions.md (§4b), docs/guard-inventory.md (guard row + the out-of-globbed-population note + a row and summary counts for the new Python guard), docs/decisions/records/testing/full-replace-asserts-field-list.md (+ regenerated catalog), scripts/tests/test_optional_request_members.py (the COVERED/COMPUTED wording this guard now verifies).

## Closing record **Outcome:** Shipped in PR #883 (squashed to `e8f80c42c`). `web/src/api/completeAnnotationScan.ts` (compiler-API scanners) + `completeAnnotations.guard.test.ts` (the guard) + a synthetic-source fixture suite, plus `scripts/tests/test_complete_annotation_dispositions.py` cross-checking the SPA disposition table against the authoritative Python one. Two derived populations — the `Complete<…>` annotations (SPA AST ∩ git index) and the droppable schemas (parsed from the generated `v1.d.ts`). **Scope was reduced**: see the scope-divergence comment above; box 1 was reworded and the offer to revert that rewording stands. **Root cause:** `Complete<T>` proved its own semantics and nothing proved its APPLICATION. Worse, `test_optional_request_members.py`'s `COVERED` disposition was *worded* "the SPA builds this body and the builder is annotated `Complete<T>`" — a claim about another language's source that nothing verified, so a `COVERED` row and a SPA with the annotation deleted were indistinguishable to the entire suite. The same file's `COMPUTED` prohibition (§4b's "do NOT annotate a schema whose optional members are computed server-side") was prose with no executable form. **Decisions/conventions changed:** none added. `testing.full-replace-asserts-field-list` updated — its `mechanics:` now states which half is machine-checked, and the body's residue paragraph was corrected (it had described the guard as covering the wrapper half, which was measured false). `docs/spa-conventions.md` §4b now leads with what is NOT checked. **Reusable knowledge:** - **A registry row that describes ANOTHER language's source is an assertion, not a check.** The `COVERED` wording had been true when written and nothing would have reported the day it stopped being. - **`Complete<T>` is SHALLOW, and reachability is not protection.** `Complete<UpdateMultiCollectionRequest>` requires `items` to be present and requires nothing of each item — `{ items: [{}] }` typechecks clean. A wrapper annotation never protects a nested request type; only the nested builder's own annotation does. I got this wrong for a full round and the doc sentence justifying it was false. - **A deep `Complete<T>` works but must not be applied blanket-wise.** Probed against the real generated types: it catches a missing nested member, preserves the `null` arm of `null | Array<X>`, keeps explicit `undefined` legal and still rejects phantoms. NOT adopted: `UpdateChannelRequest.logo` reaches `ArtworkContentTypeModel`, where annotating would force a caller to fabricate server-computed values, and it closes only the missing direction — the phantom direction needs a fresh literal in a contextually typed position, which a generic `.map` callback's return is not. Recorded so it is not re-proposed on plausibility. - **Two scanners keyed on the same token are ONE mechanism, not two.** The AST wrapper scan and its "independent" regex cross-check both matched `export function`, so an `export function` → `export const` refactor blinded both at once while I had written that "the two fail in unrelated ways". - **A hand-written mirror of a DTO wearing a `Complete<>` is the #754 mechanism in disguise** — the annotation proves the caller filled in the MIRROR. **Verification:** web 1319 tests green; `scripts/tests` 1243 green (2 skipped); typecheck + eslint + ruff clean; `decisions-validate: OK`; `build_decisions_catalog --check` up to date; `check-doc-narrative --diff` 0 warnings; `npm run check:api` and `npm run build` clean; guard-inventory summary counts machine-checked. CI on `68caecb` green (image-push correctly skipped for a PR). Nine mutations witnessed by hand during development; the Python cross-check carries a DECLARED clause mutation in `mutation_manifest.py`, executed by the harness every run. **Deferred:** - The per-CONSTRUCTION-SITE population (the PHANTOM direction) — sites-in-code, tracked in **#777**. - The wrapper-level obligation. A `scanWriteWrappers` was built and REMOVED after four review rounds each defeated it; the removal rationale is in `docs/guard-inventory.md` so the cheap version is not rebuilt without reading why it failed. - A SECOND `setupFiles` entry could discharge the obligation (three-site coordinated edit). Stated as a residual rather than closed; deriving `setupFiles` needs another virtual-module plugin. - **Observation, not deferred work:** `web/vite.config.ts:36` still describes vitest's default include as `**/*.{test,spec}.*`; the real 4.1.9 default is `**/*.{test,spec}.?(c|m)[jt]s?(x)`. Pre-existing (from #445), conservative in direction, left alone as outside this change's scope. **Docs updated:** `docs/spa-conventions.md` (§4b), `docs/guard-inventory.md` (guard row + the out-of-globbed-population note + a row and summary counts for the new Python guard), `docs/decisions/records/testing/full-replace-asserts-field-list.md` (+ regenerated catalog), `scripts/tests/test_optional_request_members.py` (the COVERED/COMPUTED wording this guard now verifies).
timothy removed the in-progress label 2026-08-30 00:07:31 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 frontend priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#820