timothyandClaude Opus 5
46ec532745
Cross-family re-review of the previous fix commit. It did NOT pass, and it was right
not to: the round that fixed the reviewers' findings introduced two of its own, both in
the tests written to close them. That is this file's recurring shape, and it is the
reason the fix commit gets re-reviewed rather than the initial diff only.
TRUNCATION (High). `read_existing_verdict` asks for 100 statuses and never checked
whether the page was full. If a head ever carried more contexts than that, an existing
`review-verdict/h10` could fall off page 1, the job would conclude no verdict exists,
and it could post an exemption `success` over a human `failure` — the worst thing this
gate can do. Six contexts exist today, so this guards a future shape, not a live bug.
BUT THE PROPOSED GUARD WAS A NO-OP, and measuring is what showed it. The review asked
for `.statuses | length` compared against `.total_count`. On this instance `total_count`
is the count for the PAGE RETURNED, not for the commit: on 3aed43c6 (6 contexts),
`?limit=1` gives `len=1, total_count=1` and `?limit=3` gives `len=3, total_count=3`.
The two are equal by construction, so that check would have read as a completeness
proof while proving nothing — and it would have been the second guard in this file to
look like a check and not be one. What IS observable is a page at the requested limit,
which means "maybe more", so that is now treated as unreadable: post nothing, leave the
required check absent. The stub mirrors the per-page `total_count` deliberately, so the
new test cannot pass for the wrong reason either.
THE TESTS THAT CLOSED THE LAST ROUND'S FINDINGS:
* The behavioural guard test — added to answer "a bare `exit 1` substring is satisfiable
by dead code" — extracted the two marker lines BY TEXT and ran them alone. That
passes even if the write is moved into a function nobody calls: the extractor finds
the text, runs it at top level, the marker appears, and the test reports the guard
proven while production writes no marker. It now executes the classify body's real
PREFIX down to and including the write, which reproduces the production control flow
instead of a reconstruction of it. Mutation: move the write into an uncalled function
-> RED (it previously passed).
* The anti-vacuity check — added to replace an over-broad assertion — hand-counted
`run:` keys with a regex that only matched an indented `run:` starting `|` or `>`. It
false-redded legal spellings (`- run: |`, a single-line `run: echo ok`) and could
count a `run: |` sitting inside a heredoc. Hand-parsing YAML to validate a YAML parse
is the wrong shape: it adds a second, worse parser whose every disagreement is a
false alarm, and a red here blocks all merges. Now asserted on CONTENT — the walk
reached >=3 bodies and one over 5000 chars.
FALSE RED, THIRD INSTANCE IN THIS FILE. The repo-wide expression test scanned raw file
text, so a delimiter in an inert top-level YAML comment redded the repo even though the
runner never evaluates it. It now scans PARSED scalars: PyYAML drops YAML comments,
while a `run:` body is itself a scalar and keeps its SHELL comments — which is exactly
the distinction that matters, since inside a `run:` scalar a comment is not inert.
Verified in both directions: an inert top-level comment passes, the same payload in a
run-body comment still reds.
Also: the `total_count` zero check now requires the JSON TYPE to be number — `jq -r`
renders `0` and `"0"` identically, so a text compare accepted a corrupted
`"total_count": "0"` as "no statuses".
Verification: 455 green. Six further mutations, each landing as intended — the uncalled
function (red), an inert YAML comment (PASSES, no false red), the same payload in a
run-body comment (red), accepting a full status page (red), comparing total_count as
text (red), and breaking the YAML walk's job key (red). Seventeen mutations across the
three rounds.
Re-probe of both live controls follows on this body; the previous probe evidence was
taken before this commit and no longer describes what would merge.
Refs: #751
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/ — task-signal map
Purpose: route a fresh contributor/agent to the minimal set of docs for the task at hand, instead of a mandatory front-to-back read. Update this doc in the same PR that adds, removes, or retitles a doc below, or that changes which sections a task signal points to.
Start here, always
CLAUDE.md(repo root) — project intro: architecture, layout, dev commands, conventions, Task Completion Protocol.docs/contributing.md— established code patterns (CQRS/MediatR, LanguageExt, the ChicoryTV SPA, EF Core dual-provider migrations, FFmpeg pipeline, analyzers, testing). Read before any non-trivial change.
Task signal → minimal sections
| Signal | Read |
|---|---|
| Session startup / "what's next" (no issue named) | docs/handoffs/chicorytv-issue-queue.md (standing kickoff — two concurrent tracks: orientation ‖ scripts/select-queue.sh 5) |
| Named-issue pickup | Skip queue selection; go straight to focused retrieval — see "Knowledge retrieval" below, then the issue body |
Adding/changing a /api/* endpoint |
docs/api-conventions.md checklist + docs/endpoint-index.md |
| Adding a ChicoryTV SPA screen | docs/spa-conventions.md |
| Scheduling / playout engine work | docs/domain-model.md + decisions catalog rows keyed sched.* (docs/decisions/README.md) |
| Concurrency / optimistic-locking work | docs/api-conventions.md §7a/b/c + docs/decisions/optimistic-concurrency.md |
| Auth / security-surface work | docs/decisions/api-auth-security.md |
| CI / release pipeline work | docs/ci-cd.md + docs/decisions/release-ci-governance.md |
| Live local run / Playwright-MCP verification | docs/e2e-local.md + scripts/e2e-local.sh |
| Adding/changing a UI-E2E browser flow | docs/e2e-local.md → "UI-E2E harness" + scripts/e2e-ui.sh |
| What does a test suite cover | docs/testing.md |
| Legacy Blazor route lookup | docs/blazor-route-parity.md (historical #91 phase (b) inventory) |
| "Why do we do X this way" / challenging a convention | Catalog-first: docs/decisions/README.md (active rows) → follow the row's link to docs/decisions/records/<area>/<topic>.md for full rationale. docs/decisions/archive/<area>/ only for "what did the rule used to be." |
Knowledge retrieval (MemPalace + catalog + Gitea)
These four rules are the seam agreed with server-management#642 (the Gitea→MemPalace exporter). They apply whether the question comes up via MemPalace, a grep, or a stale comment:
- Current conventions/decisions → catalog-first. Start at
docs/decisions/README.md; discover via theErsatzTV-Decisionswing (active) /ErsatzTV-Decisions-Archive(superseded/retired). Resolve by topic/key, never by chasing a file path. - Issue history → evidence, not authority. The
Gitea-ErsatzTVwing is historical narrative that may be stale; it never overrides current Markdown. - The breadcrumb rule (the crux behavior change). A file path named inside a historical issue
comment (e.g. "grep
docs/decisions.md2026-07-17", "see …") is a breadcrumb, not a live pointer. Find the current rule via the catalog / active wing by concept; do not treat the named path as current. (Why it's safe: still-current → in the active wing, breadcrumb resolves; superseded → the active wing returns the successor and a literal follow lands on a record that announces its ownstatus: superseded; retired → the active wing returns nothing, which is itself the signal. The validator-enforced move-to-archive/is what prevents the catastrophic "superseded rule read as current" case.) - Fallback when MemPalace is stale/down:
docs/decisions/README.mdcatalog, thenrg '^`key: <dotted.key>`' docs/decisions/. MemPalace is never authority nor sole fallback.
MemPalace is candidate discovery only — every passage is verified against its cited Markdown/Gitea
source before use. Never derive live queue state from MemPalace, #237, or historical comments; queue
state is live Gitea state, retrieved via scripts/select-queue.sh (see
docs/handoffs/chicorytv-issue-queue.md). Full retrieval contract (altitude/precedence, staleness
bounds, what's mined per issue): docs/handoffs/chicorytv-issue-queue.md → "Knowledge retrieval".
Also present in docs/
docs/domain-model.md— what the app IS: entity glossary, channel→playout→schedule/block concept map, where each concept is edited in the SPA.docs/api-conventions.md— checklist for adding/changing a/api/*endpoint (controllers, DTOs, error mapping, auth, OpenAPI regen, tests).docs/spa-conventions.md— playbook for adding a screen to the ChicoryTV React SPA.docs/e2e-local.md(+scripts/e2e-local.sh) — how to run a live local instance for manual or Playwright-MCP verification.docs/testing.md— testing map: what each*.Testsproject /websuite covers, golden-file nets, the timezone-independence rule, how to run subsets, the per-PR verification gate.docs/blazor-route-parity.md— historical record of the completed #91 phase (b) cutover: the Blazor Server UI is removed and every legacy route now 302-redirects to its SPA equivalent (or falls through to the catch-all →/app). Read it for the full legacy→SPA route inventory.docs/decisions/records/<area>/<topic>.md— one active decision record per file, YAML frontmatter (key/title/status/since/supersedes/superseded-by, plus optionalstale-after/sources— ersatztv#603), rationale prose in the body. The filename is the key, so one-active-record-per-key is a filesystem property (ersatztv#610).docs/decisions.mdand the topic files remain as the lifecycle-schema narrative plus a "Records formerly in this file" index, which is what keeps older date-based pointers resolvable. Generated active view:docs/decisions/README.md(catalog / task router) — start there. Superseded/retired records live indocs/decisions/archive/and are read only for history, never for "what is the current rule."docs/ci-cd.md— build/test/release pipeline, versioning, dependency management.docs/rest-api.md— REST API design doc for ersatztv#2 (goals, conventions, per-slice plan). Largely superseded day-to-day bydocs/api-conventions.md; read this for the original rationale.docs/mcp.md— theErsatzTV.Mcpstdio JSON-RPC MCP server (#58): how it wraps/api/v1as read + cautious-write tools, its config/env vars, auth, security posture, and the tool catalog.docs/channels.md— Channel entity field reference.docs/m3u-xmltv.md— M3U/XMLTV generation overview (ChannelPlaylist,GetChannelGuideHandler).docs/fork-strategy.md— divergence policy vs upstream ErsatzTV.docs/design-sync.md— Claude Design ↔ repo screen workflow (#92).docs/endpoint-index.md— generated REST endpoint index (method/path/operationId/summary per OpenAPI tag). Do not edit by hand; regenerated byscripts/generate-endpoint-index.py/scripts/update-openapi.sh.docs/handoffs/chicorytv-issue-queue.md— static session kickoff prompt + workflow lore. Queue state is live Gitea state, retrieved each session viascripts/select-queue.sh— see that file's standing kickoff for the two concurrent tracks (orientation ‖ selection). ersatztv#237 is a closed, archival historical tracker (superseded bystartup.parallel-orientationindocs/decisions.md) — not a live pointer.docs/tracker-retrofit-triage-237.md— audit trail for the #524 triage of ersatztv#237's 111 comments (method, per-comment classification, totals). Evidence for thedocs.tracker-comment-retrofitdecision; read it only when triaging another over-cap tracker.docs/handoffs/rest-api.md— original handoff prompt for kicking off the REST API work (#2).