timothy
71706f3849
Second review round. Codex returned BLOCKED @ e50a2624 with three High findings;
all three were real and all three are fixed. One of them was severe and was
introduced by my OWN previous "fix" commit.
## HIGH 1 — a FAILING spec run exited 0, silently passing CI
`if ! npx playwright test; then status=$?; ... exit "$status"; fi`
Under `!` negation bash sets `$?` to the LOGICAL NEGATION of the command's
status, so inside the failure branch `$?` reads 0 — the script exited 0 on a
failing run. Verified: `if ! (exit 42); then echo $?; fi` prints 0.
A UI-E2E harness that reports success when its specs fail is worse than no
harness. My five green local runs could never have caught this: the bug lives
only on the failure path. Introduced by the log-tail improvement in e50a2624.
Fixed with `set +e` / read `$?` / `set -e`, then exit that status explicitly.
Verified: a deliberately-failing run (`--grep ZZZ_NOPE`) now exits 1.
## HIGH 2 + 3 — the pre-PID fallback needed lsof and only GUESSED ownership
The mid-boot fallback reaped "whatever LISTENS on $PORT", which was wrong twice:
- it needed `lsof`, which is ABSENT from the CI toolchain image (verified
directly in the published image) — so it silently no-opped precisely where
it was needed;
- it INFERRED ownership from the earlier pre-flight rather than proving it, so
a process that grabbed the port after the pre-flight — or a real instance on
a shared host — could be killed. Reaping someone else's server is worse than
the leak it was meant to fix.
Replaced with an opt-in `ETV_PIDFILE`: e2e-local.sh writes the PID the instant
it forks, BEFORE its readiness wait, which is exactly the window a mid-boot
signal lands in. A pidfile we asked for PROVES ownership, needs no external
tool, and works in CI. The port-based kill is gone; the lsof pre-flight remains
as a friendly local check only.
Verified: the pidfile is populated while still mid-boot (readiness not yet
reached), names the real `dotnet ErsatzTV` process (not a subshell — which also
re-confirms the `exec` fix), and killing that PID alone frees the port.
Also dropped the `seq` dependency inside the trap (shell arithmetic instead),
addressing the other reviewer's busybox concern.
## Docs-reviewer finding — my stated reasoning was wrong
I justified amending `testing.e2e-local-fresh-config-dir` rather than superseding
it partly on "renaming the heading trips CI". That's a true statement that does
NOT bear on the choice: a supersession relocates to `archive/` with the heading
INTACT (verified: archive/api.md keeps the #72 heading verbatim). Corrected to
the actual reasons — the Rule never reversed, and the key is cited from
docs/handoffs/chicorytv-issue-queue.md plus two docs/superpowers/ files, which a
supersession would aim at an archived, stale-labelled record.
## Gotcha found by accident, now documented
A flawed test of mine booted two e2e-local.sh instances concurrently and the
first mysteriously failed to become ready. Cause: every run `rm -rf`s and
re-copies the SAME build-output wwwroot, so a second run yanks the static files
out from under a still-starting first instance. Documented in both the script
header and docs/e2e-local.md, because the symptom (readiness timeout, or /app
404ing) looks nothing like a shared-directory race. CI is unaffected — its curl
and UI-E2E steps are sequential.
## Budget: filed, not shaved
This PR pushes the active decisions corpus 7 lines past its 5600-line soft
budget (main was under). I trimmed my records repeatedly and each rewrite
recovered ~1 line, because the content is load-bearing; continuing would have
meant deleting useful rationale from a new convention record to hit an arbitrary
cap. The validator's own remedy is "schedule a consolidation", so that is filed
as #595 rather than paid for by starving the record. Non-blocking warning.
Also filed #594 for the pre-existing `ci-image-pin` any-hex-length weakness.
## Verification
- failing run exits 1 (was 0); passing run still 3/3 green
- pidfile written mid-boot, names the real dotnet proc, reap frees the port
- SIGTERM mid-run: exit 143, no orphan listener or process
- curl harness unaffected: 45/45 PASS; ETV_PIDFILE unset => unchanged behaviour
- decisions validator OK; zero orphaned processes after the full gate
Refs #445 #533 #594 #595
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) → docs/decisions.md + docs/decisions/*.md for full rationale. docs/decisions/archive/ 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.md+docs/decisions/*.md— active decision records (lifecycle schema: key/status/since/supersedes/superseded-by). 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).