Clears the still-live findings from #172 (a #197 gate-cluster item). Most of the 2026-07-07 list was already resolved or ratified deliberate — this PR fixes the three that were genuinely live and dispositions the rest.
Unreachable 404 ProducesResponseType on create-group
FIXED
2
Negative-id "(none)" synthesized rows
Deliberate (api-conventions §8)
4
Deep-FK non-existence-check in ReplaceBlockItems
Deliberate (api-conventions §3b)
7
Name= missing on PlayoutController Create/Delete/Update
Moot — OperationIdOpenApiTransformer (#197 Bundle C) synthesizes stable operationIds; adding Name= would risk renaming generated SPA client methods
5
No lightweight exists query
Deferred (perf refactor, not a bug)
tail
unauth /api/logs+/api/troubleshoot/info; Trakt link; serial /api/search
Already fixed on main
Fixes
1 — Null/empty name → 500 (10 create/replace handlers).CreateBlock, ReplaceBlockItems, CreateTemplate, ReplaceTemplateItems, CreateDecoTemplate, ReplaceDecoTemplateItems, CreateDeco, UpdateDeco, UpdateFFmpegProfile (9 genuine 500s) + CreatePlaylist (empty-name persist; its DTO coalesces null→"") all did if (request.Name.Length > 50) on a client-nullable string Name. Request DTOs carry no #nullable context (api-conventions §2) and there's no implicit [Required] and no global exception filter, so name: null bound through and threw NullReferenceException → HTTP 500. Guard is now string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50 → 422, which also rejects empty/whitespace names (matching the group-create handlers' null-safe NotEmpty, closing a latent "entity named ''" gap). Chose the one-line guard over refactoring each handler onto the NotEmpty/NotLongerThan combinator to keep the blast radius minimal and preserve each handler's existing error message + 422 mapping.
3 — Duplicate template items.ReplaceTemplateItemsHandler's O(n²) overlap loop skipped on item == otherItem, but BlockTemplateItem is a record, so two value-identical items (same BlockId + StartTime → same computed EndTime) were value-equal and skipped — both persisted unvalidated. Now iterates by index (i != j) so identical items at distinct positions are compared and register as a (self-)intersection → 422. (The SPA's index-based check already caught this client-side; API-only gap.)
6 — Unreachable 404.POST /api/blocks/groups / POST /api/templates/groups declared a 404 copied from precedent, but a create has no parent lookup that can 404 (only 201/422). Trimmed; v1.json regenerated (diff = exactly the two 404 removals).
Tests
Issue172HardeningTests.cs (19 cases) — a null + whitespace case per handler for all 10 name-guard paths (incl. UpdateDeco), plus FFmpegProfileHandlerTests.Update_Should_Reject_Null_Name, plus the duplicate-items rejection (non-vacuous: the two items are the same value-identical record, so the old value-equality skip would have persisted both — the test fails against the old logic). Full ErsatzTV.Tests suite green locally.
Docs
docs/decisions.md entry + docs/api-conventions.md §3b handler-hardening bullet (dereferencing a request string is a latent 500 — validate null-safe).
Review scope note
Independent adversarial review pending (per the skip rubric this diff — API write-path handler validation, ~11 files — is above the pure-SPA/docs leaf threshold, so a real independent pass is warranted, not skippable).
Clears the still-live findings from #172 (a #197 gate-cluster item). Most of the 2026-07-07 list was already resolved or ratified deliberate — this PR fixes the three that were genuinely live and dispositions the rest.
## What was live vs stale (verified against `main`)
| # | Finding | Disposition |
|---|---|---|
| 1 | Null `name` → NRE → 500 | **FIXED** (10 handlers) |
| 3 | Exact-duplicate template items bypass overlap validation | **FIXED** |
| 6 | Unreachable 404 `ProducesResponseType` on create-group | **FIXED** |
| 2 | Negative-id "(none)" synthesized rows | Deliberate (api-conventions §8) |
| 4 | Deep-FK non-existence-check in ReplaceBlockItems | Deliberate (api-conventions §3b) |
| 7 | `Name=` missing on PlayoutController Create/Delete/Update | Moot — `OperationIdOpenApiTransformer` (#197 Bundle C) synthesizes stable operationIds; adding `Name=` would risk renaming generated SPA client methods |
| 5 | No lightweight exists query | Deferred (perf refactor, not a bug) |
| tail | unauth `/api/logs`+`/api/troubleshoot/info`; Trakt link; serial `/api/search` | Already fixed on `main` |
## Fixes
**1 — Null/empty name → 500 (10 create/replace handlers).** `CreateBlock`, `ReplaceBlockItems`, `CreateTemplate`, `ReplaceTemplateItems`, `CreateDecoTemplate`, `ReplaceDecoTemplateItems`, `CreateDeco`, `UpdateDeco`, `UpdateFFmpegProfile` (9 genuine 500s) + `CreatePlaylist` (empty-name persist; its DTO coalesces `null`→`""`) all did `if (request.Name.Length > 50)` on a client-nullable `string Name`. Request DTOs carry no `#nullable` context (api-conventions §2) and there's no implicit `[Required]` and **no global exception filter**, so `name: null` bound through and threw `NullReferenceException` → HTTP 500. Guard is now `string.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50` → 422, which also rejects empty/whitespace names (matching the group-create handlers' null-safe `NotEmpty`, closing a latent "entity named ''" gap). Chose the one-line guard over refactoring each handler onto the `NotEmpty`/`NotLongerThan` combinator to keep the blast radius minimal and preserve each handler's existing error message + 422 mapping.
**3 — Duplicate template items.** `ReplaceTemplateItemsHandler`'s O(n²) overlap loop skipped on `item == otherItem`, but `BlockTemplateItem` is a `record`, so two value-identical items (same BlockId + StartTime → same computed EndTime) were value-equal and skipped — both persisted unvalidated. Now iterates by index (`i != j`) so identical items at distinct positions are compared and register as a (self-)intersection → 422. (The SPA's index-based check already caught this client-side; API-only gap.)
**6 — Unreachable 404.** `POST /api/blocks/groups` / `POST /api/templates/groups` declared a 404 copied from precedent, but a create has no parent lookup that can 404 (only 201/422). Trimmed; `v1.json` regenerated (diff = exactly the two 404 removals).
## Tests
`Issue172HardeningTests.cs` (19 cases) — a null + whitespace case per handler for all 10 name-guard paths (incl. `UpdateDeco`), plus `FFmpegProfileHandlerTests.Update_Should_Reject_Null_Name`, plus the duplicate-items rejection (non-vacuous: the two items are the same value-identical record, so the old value-equality skip would have persisted both — the test fails against the old logic). Full `ErsatzTV.Tests` suite green locally.
## Docs
`docs/decisions.md` entry + `docs/api-conventions.md` §3b handler-hardening bullet (dereferencing a request `string` is a latent 500 — validate null-safe).
## Review scope note
Independent adversarial review pending (per the skip rubric this diff — API write-path handler validation, ~11 files — is above the pure-SPA/docs leaf threshold, so a real independent pass is warranted, not skippable).
fixes #172
Ran a cold review over the initial diff, which found one Blocker: Fix 3's 404 removal from Block/Template CreateGroup left ApiErrorResponseMetadataTests asserting those ops still document 404 → Build & test would go red. (My own earlier full-suite run reported exit 0 — a stale-.dll false-pass; the reviewer's clean run caught it.)
Fixed in 9b5d1244: removed the stale metadata assertions and, for spec consistency, extended the unreachable-404 trim to Deco + DecoTemplate CreateGroup (same handler shape — NotEmpty → NotLongerThan → duplicate-name AnyAsync, all → 422, no lookup that 404s). All four CreateGroup 404s now gone (422 siblings kept); v1.json regenerated (40 deletions, 2 hunks). Full ErsatzTV.Tests green locally (1570/0).
The reviewer re-reviewed the fix commit and confirmed: B1 fully resolved, the Deco/DecoTemplate expansion sound, no dangling test/SPA consumers, v1.json diff is exactly the removals. One Low (ReplacePlaylistItems does no name validation) is a pre-existing create-vs-update asymmetry, not the #172 NRE class — deferred to #327 (reviewer agreed).
Independent-review rubric note: the fix commit is purely subtractive (unreachable-ProducesResponseType attributes + stale test assertions + spec lines, zero logic/runtime-behavior change), and the same independent reviewer verified it — no correlated-blindspot risk.
Review-verdict: MERGEABLE @ 9b5d1244
## Independent adversarial review — cold, zero-context
Ran a cold review over the initial diff, which found one **Blocker**: Fix 3's 404 removal from Block/Template `CreateGroup` left `ApiErrorResponseMetadataTests` asserting those ops still document 404 → `Build & test` would go red. (My own earlier full-suite run reported exit 0 — a stale-`.dll` false-pass; the reviewer's clean run caught it.)
Fixed in `9b5d1244`: removed the stale metadata assertions and, for spec consistency, extended the unreachable-404 trim to Deco + DecoTemplate `CreateGroup` (same handler shape — `NotEmpty → NotLongerThan → duplicate-name AnyAsync`, all → 422, no lookup that 404s). All four `CreateGroup` 404s now gone (422 siblings kept); v1.json regenerated (40 deletions, 2 hunks). Full `ErsatzTV.Tests` green locally (1570/0).
The reviewer re-reviewed the fix commit and confirmed: B1 fully resolved, the Deco/DecoTemplate expansion sound, no dangling test/SPA consumers, v1.json diff is exactly the removals. One Low (`ReplacePlaylistItems` does no name validation) is a pre-existing create-vs-update asymmetry, not the #172 NRE class — deferred to **#327** (reviewer agreed).
Independent-review rubric note: the fix commit is purely subtractive (unreachable-`ProducesResponseType` attributes + stale test assertions + spec lines, zero logic/runtime-behavior change), and the same independent reviewer verified it — no correlated-blindspot risk.
Review-verdict: MERGEABLE @ 9b5d1244
Follow-up cd3b5102 is formatting-only: the #311 fix-as-you-touch gate checks the whole of every touched .cs, so trimming DecoController's 404 subjected the whole file to it and it flagged 3 pre-existing [Required] [FromBody] → [Required][FromBody] spacing violations (matching the rest of the API controllers). 3-line whitespace/style delta, no logic change — CI caught it because dotnet format --include no-ops on the local macOS box.
No independent re-review warranted for a whitespace-only delta (skip rubric); the logic verdict from 9b5d1244 stands.
Review-verdict: MERGEABLE @ cd3b5102
Follow-up `cd3b5102` is **formatting-only**: the #311 fix-as-you-touch gate checks the whole of every touched `.cs`, so trimming DecoController's 404 subjected the whole file to it and it flagged 3 pre-existing `[Required] [FromBody]` → `[Required][FromBody]` spacing violations (matching the rest of the API controllers). 3-line whitespace/style delta, no logic change — CI caught it because `dotnet format --include` no-ops on the local macOS box.
No independent re-review warranted for a whitespace-only delta (skip rubric); the logic verdict from 9b5d1244 stands.
Review-verdict: MERGEABLE @ cd3b5102
Rebased onto origin/main after #286 (the /api/v1 versioning sweep) merged — new head 2e4e07a2.
The 4 controllers auto-merged cleanly: my create-group 404 trims sit on top of #286's /api/v1 routes (verified all 4 CreateGroup actions = /api/v1/.../groups + 201/422, no 404).
Handler guards + the ApiErrorResponseMetadataTests changes replayed intact.
decisions.md had an append/append conflict — resolved by keeping both 2026-07-13 entries (#286's and #172's).
Regeneratedv1.json/endpoint-index/v1.d.ts (rebase-artifact discipline) — regen produced zero changes over the rebased tree, and the v1.json diff vs main is exactly the 4 create-group 404 removals (now on /api/v1 paths); endpoint-index/.d.ts unaffected.
Full ErsatzTV.Tests green post-rebase: 1583/0 (up from 1570 — #286's added tests coexist).
The dropped 3rd commit ([Required][FromBody] spacing) was already applied upstream by #286's sweep — git dropped it as redundant.
No logic changed by the rebase (same reviewed diff replayed; clean auto-merge + a trivial keep-both docs resolution), so the prior fix-commit review carries.
Rebased onto `origin/main` after **#286** (the `/api/v1` versioning sweep) merged — new head `2e4e07a2`.
- The 4 controllers auto-merged cleanly: my create-group 404 trims sit on top of #286's `/api/v1` routes (verified all 4 `CreateGroup` actions = `/api/v1/.../groups` + `201`/`422`, no `404`).
- Handler guards + the `ApiErrorResponseMetadataTests` changes replayed intact.
- `decisions.md` had an append/append conflict — resolved by keeping **both** 2026-07-13 entries (#286's and #172's).
- **Regenerated** `v1.json`/`endpoint-index`/`v1.d.ts` (rebase-artifact discipline) — regen produced **zero** changes over the rebased tree, and the `v1.json` diff vs `main` is exactly the 4 create-group `404` removals (now on `/api/v1` paths); endpoint-index/`.d.ts` unaffected.
- Full `ErsatzTV.Tests` green post-rebase: **1583/0** (up from 1570 — #286's added tests coexist).
- The dropped 3rd commit (`[Required][FromBody]` spacing) was already applied upstream by #286's sweep — git dropped it as redundant.
No logic changed by the rebase (same reviewed diff replayed; clean auto-merge + a trivial keep-both docs resolution), so the prior fix-commit review carries.
Review-verdict: MERGEABLE @ 2e4e07a2
timothy
merged commit 2e4e07a207 into main2026-07-13 01:27:38 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Clears the still-live findings from #172 (a #197 gate-cluster item). Most of the 2026-07-07 list was already resolved or ratified deliberate — this PR fixes the three that were genuinely live and dispositions the rest.
What was live vs stale (verified against
main)name→ NRE → 500ProducesResponseTypeon create-groupName=missing on PlayoutController Create/Delete/UpdateOperationIdOpenApiTransformer(#197 Bundle C) synthesizes stable operationIds; addingName=would risk renaming generated SPA client methods/api/logs+/api/troubleshoot/info; Trakt link; serial/api/searchmainFixes
1 — Null/empty name → 500 (10 create/replace handlers).
CreateBlock,ReplaceBlockItems,CreateTemplate,ReplaceTemplateItems,CreateDecoTemplate,ReplaceDecoTemplateItems,CreateDeco,UpdateDeco,UpdateFFmpegProfile(9 genuine 500s) +CreatePlaylist(empty-name persist; its DTO coalescesnull→"") all didif (request.Name.Length > 50)on a client-nullablestring Name. Request DTOs carry no#nullablecontext (api-conventions §2) and there's no implicit[Required]and no global exception filter, soname: nullbound through and threwNullReferenceException→ HTTP 500. Guard is nowstring.IsNullOrWhiteSpace(request.Name) || request.Name.Length > 50→ 422, which also rejects empty/whitespace names (matching the group-create handlers' null-safeNotEmpty, closing a latent "entity named ''" gap). Chose the one-line guard over refactoring each handler onto theNotEmpty/NotLongerThancombinator to keep the blast radius minimal and preserve each handler's existing error message + 422 mapping.3 — Duplicate template items.
ReplaceTemplateItemsHandler's O(n²) overlap loop skipped onitem == otherItem, butBlockTemplateItemis arecord, so two value-identical items (same BlockId + StartTime → same computed EndTime) were value-equal and skipped — both persisted unvalidated. Now iterates by index (i != j) so identical items at distinct positions are compared and register as a (self-)intersection → 422. (The SPA's index-based check already caught this client-side; API-only gap.)6 — Unreachable 404.
POST /api/blocks/groups/POST /api/templates/groupsdeclared a 404 copied from precedent, but a create has no parent lookup that can 404 (only 201/422). Trimmed;v1.jsonregenerated (diff = exactly the two 404 removals).Tests
Issue172HardeningTests.cs(19 cases) — a null + whitespace case per handler for all 10 name-guard paths (incl.UpdateDeco), plusFFmpegProfileHandlerTests.Update_Should_Reject_Null_Name, plus the duplicate-items rejection (non-vacuous: the two items are the same value-identical record, so the old value-equality skip would have persisted both — the test fails against the old logic). FullErsatzTV.Testssuite green locally.Docs
docs/decisions.mdentry +docs/api-conventions.md§3b handler-hardening bullet (dereferencing a requeststringis a latent 500 — validate null-safe).Review scope note
Independent adversarial review pending (per the skip rubric this diff — API write-path handler validation, ~11 files — is above the pure-SPA/docs leaf threshold, so a real independent pass is warranted, not skippable).
fixes #172
Independent adversarial review — cold, zero-context
Ran a cold review over the initial diff, which found one Blocker: Fix 3's 404 removal from Block/Template
CreateGroupleftApiErrorResponseMetadataTestsasserting those ops still document 404 →Build & testwould go red. (My own earlier full-suite run reported exit 0 — a stale-.dllfalse-pass; the reviewer's clean run caught it.)Fixed in
9b5d1244: removed the stale metadata assertions and, for spec consistency, extended the unreachable-404 trim to Deco + DecoTemplateCreateGroup(same handler shape —NotEmpty → NotLongerThan → duplicate-name AnyAsync, all → 422, no lookup that 404s). All fourCreateGroup404s now gone (422 siblings kept); v1.json regenerated (40 deletions, 2 hunks). FullErsatzTV.Testsgreen locally (1570/0).The reviewer re-reviewed the fix commit and confirmed: B1 fully resolved, the Deco/DecoTemplate expansion sound, no dangling test/SPA consumers, v1.json diff is exactly the removals. One Low (
ReplacePlaylistItemsdoes no name validation) is a pre-existing create-vs-update asymmetry, not the #172 NRE class — deferred to #327 (reviewer agreed).Independent-review rubric note: the fix commit is purely subtractive (unreachable-
ProducesResponseTypeattributes + stale test assertions + spec lines, zero logic/runtime-behavior change), and the same independent reviewer verified it — no correlated-blindspot risk.Review-verdict: MERGEABLE @ 9b5d1244
Follow-up
cd3b5102is formatting-only: the #311 fix-as-you-touch gate checks the whole of every touched.cs, so trimming DecoController's 404 subjected the whole file to it and it flagged 3 pre-existing[Required] [FromBody]→[Required][FromBody]spacing violations (matching the rest of the API controllers). 3-line whitespace/style delta, no logic change — CI caught it becausedotnet format --includeno-ops on the local macOS box.No independent re-review warranted for a whitespace-only delta (skip rubric); the logic verdict from 9b5d1244 stands.
Review-verdict: MERGEABLE @ cd3b5102
cd3b51025ato2e4e07a207Rebased onto
origin/mainafter #286 (the/api/v1versioning sweep) merged — new head2e4e07a2./api/v1routes (verified all 4CreateGroupactions =/api/v1/.../groups+201/422, no404).ApiErrorResponseMetadataTestschanges replayed intact.decisions.mdhad an append/append conflict — resolved by keeping both 2026-07-13 entries (#286's and #172's).v1.json/endpoint-index/v1.d.ts(rebase-artifact discipline) — regen produced zero changes over the rebased tree, and thev1.jsondiff vsmainis exactly the 4 create-group404removals (now on/api/v1paths); endpoint-index/.d.tsunaffected.ErsatzTV.Testsgreen post-rebase: 1583/0 (up from 1570 — #286's added tests coexist).[Required][FromBody]spacing) was already applied upstream by #286's sweep — git dropped it as redundant.No logic changed by the rebase (same reviewed diff replayed; clean auto-merge + a trivial keep-both docs resolution), so the prior fix-commit review carries.
Review-verdict: MERGEABLE @
2e4e07a2