chore(586,594,485): PID-scoped E2E cleanup, ci-image-pin length guard, .gitignore core fix #598

Merged
timothy merged 1 commits from chore/586-594-485-hygiene into main 2026-07-25 13:51:57 +02:00
Owner

Session bundle of three independent CI/repo-hygiene fixes with disjoint file sets.

fixes #586
fixes #594
fixes #485


#586 — E2E cleanup is scoped by PID, never a pattern-wide pkill

  • New decision record testing.e2e-cleanup-scope-by-pid (+ regenerated catalog).
  • docs/e2e-local.md states the constraint in the teardown step, aimed explicitly at brief-writers — the #586 root cause was a delegation gap, not agent error.
  • docs/handoffs/chicorytv-issue-queue.md gets the one-line lore bullet, since that is what a live session actually reads at startup.

On the trap question (Done-when box 3): reviewed, and deliberately NOT adopted — recorded why. e2e-local.sh's contract is the inverse of e2e-ui.sh's: it hands a running instance back to its caller. Both callers use OUT="$(scripts/e2e-local.sh …)", so an EXIT trap would fire when the command substitution returns and kill the server before either could use it. e2e-ui.sh is the lifecycle owner and traps; this is the launcher and does not.

Instead the script gained the two things that actually prevent the incident:

  • An lsof pre-flight that names a foreign listener's PID rather than letting the run fail as process N exited before becoming ready + log tail — a framing that reads like a broken build. It reports, never reaps.
  • ETV_STREAMING_PORT now defaults to the given port.

The Blocker review caught — the escape hatch didn't work

Program.cs:167-172 binds two listeners, and SystemEnvironment.cs:23-27 defaults StreamingPort to 8409 independently of ETV_UI_PORT. So the obvious remedy ETV_UI_PORT=8420 still bound 8409, died against the foreign holder, and printed a log naming 8409 — dropping an agent right back into the confusion that produced the pkill. Documenting that remedy would have made #586 more likely to recur. Now the streaming port follows the UI port (explicit value still wins) and the pre-flight probes both.

A false premise from the issue body, corrected

#586 states "the CI step uses 8410 while local runs use 8409". On main, docker-build.yml:492 sets ETV_UI_PORT=8409 for functional-e2e — the same port as local. 8410 is the ersatztv-test container on jazz; only the unmerged #445 branch adds an 8410 step. The corrected fact is the stronger one: local and CI collide on the same port, so port choice was never separation.

#594 — ci-image-pin accepts any hex length

Length is an invariant separate from correctness: the resolve/staleness checks compare resolved shas, so an 8-char pin of the right commit passes green while matching no registry tag — and all five container: jobs then die at image-pull with manifest unknown, which reads like a registry outage.

  • Guard fails at the gate and prints the exact tag to use.
  • Literal 7, not a derived --short=7: in a full clone git may widen an ambiguous abbreviation, demanding a pin ci-image.yml can never publish — the same clone-depth asymmetry the existing comment warns about. Escape hatch documented inline.
  • Also fixes a pre-existing misdiagnosis: zero pins reported "MORE THAN ONE".
  • docs/ci-cd.md documents the 7-char rule, git rev-parse --short=7 HEAD, and the caveat that 7 is currently empirical (see #597).

#485 — .gitignore core swallowed */Core/ source files

A bare core matched any path component named core; case-insensitively on macOS that included every */Core/ source directory, so new untracked files were dropped by git add -A while tracked ones stayed fine — a clean local build and a CI checkout that fails to compile. Now /core + /core.[0-9]*, both anchored (an unanchored core.[0-9]* would re-introduce the very class this fixes). Subdirectory-dump coverage is a written-down accepted tradeoff.


Verification

What How
.gitignore Full ignored-file-set diff before/after: identical — nothing lost its ignore. The three real dirs (ErsatzTV.Scanner/Core, ErsatzTV.Scanner.Tests/Core, ErsatzTV.Tests/Core) are trackable without -f.
ci-image-pin Step body extracted from the YAML by real parse (not hand-copied), run against doctored pins: 7 green; 6 / 8 / 10 red, each naming the correct tag. docker-build.yml restored clean.
Pre-flight Decoy listeners: UI port busy → red naming it; 8409 busy + ETV_UI_PORT=8420 → now passes (previously a dead end); ETV_STREAMING_PORT=8409 pinned busy → red naming 8409.
Port guard 84o9 / abc → red; empty → correctly falls back to the 8409 default; CI shape (ETV_UI_PORT=8409) unaffected.
Gates bash -n clean · decisions_validate.py OK · catalog regenerates byte-identically · kickoff-guard OK · pr-checks.yml parses.

Live-E2E: not required and not run (release.live-e2e-required scopes it to API write-path handlers; there is no C# change). The executable diff to e2e-local.sh is the pre-flight plus the env default, both tested directly above; every other line in that script is untouched.

Independent review: done, two rounds — cold-context reviewer, no implementation role. Round 1 returned BLOCKED and found the ETV_STREAMING_PORT Blocker plus the 8410 factual error. Round 2 re-reviewed the fix commit and returned MERGEABLE; its remaining Medium/Low/Nit items are all folded in.

Known, pre-existing

decisions_validate.py emits active-corpus 5682 lines (budget 5600). main is already over budget; this record widens it. Non-blocking ::warning:: ("schedule a consolidation"), not a gate.

Follow-ups filed

  • #596 — testing.playwright-mcp-download-and-recovery prescribes pkill -f ms-playwright-mcp, the same shared-host reap this PR forbids. Deferred rather than line-edited, because reversing a record is a supersede-and-relocate lifecycle operation. Flags the honest open question: a safe PID-scoped MCP recovery may not exist, in which case the rule is "restart via the client, never the shell."
  • #597 — make ci-image.yml tag with --short=7 so the 7-char length is enforced rather than observed. Needs its own publish-then-pin two-step, since editing ci-image.yml re-points ci-image-pin's expected at that commit.

🤖 Generated with Claude Code

Session bundle of three independent CI/repo-hygiene fixes with disjoint file sets. fixes #586 fixes #594 fixes #485 --- ## #586 — E2E cleanup is scoped by PID, never a pattern-wide `pkill` - New decision record `testing.e2e-cleanup-scope-by-pid` (+ regenerated catalog). - `docs/e2e-local.md` states the constraint in the teardown step, aimed explicitly at **brief-writers** — the #586 root cause was a delegation gap, not agent error. - `docs/handoffs/chicorytv-issue-queue.md` gets the one-line lore bullet, since that is what a live session actually reads at startup. **On the trap question (Done-when box 3): reviewed, and deliberately NOT adopted — recorded why.** `e2e-local.sh`'s contract is the inverse of `e2e-ui.sh`'s: it hands a *running* instance back to its caller. Both callers use `OUT="$(scripts/e2e-local.sh …)"`, so an `EXIT` trap would fire when the command substitution returns and kill the server before either could use it. `e2e-ui.sh` is the lifecycle owner and traps; this is the launcher and does not. Instead the script gained the two things that actually prevent the incident: - An `lsof` pre-flight that **names a foreign listener's PID** rather than letting the run fail as `process N exited before becoming ready` + log tail — a framing that reads like a broken build. It reports, never reaps. - `ETV_STREAMING_PORT` now defaults to the given port. ### The Blocker review caught — the escape hatch didn't work `Program.cs:167-172` binds **two** listeners, and `SystemEnvironment.cs:23-27` defaults `StreamingPort` to **8409 independently of `ETV_UI_PORT`**. So the obvious remedy `ETV_UI_PORT=8420` *still bound 8409*, died against the foreign holder, and printed a log naming 8409 — dropping an agent right back into the confusion that produced the `pkill`. Documenting that remedy would have made #586 **more** likely to recur. Now the streaming port follows the UI port (explicit value still wins) and the pre-flight probes both. ### A false premise from the issue body, corrected #586 states "the CI step uses 8410 while local runs use 8409". On `main`, `docker-build.yml:492` sets `ETV_UI_PORT=8409` for `functional-e2e` — the **same** port as local. 8410 is the `ersatztv-test` container on jazz; only the unmerged #445 branch adds an 8410 step. The corrected fact is the stronger one: local and CI collide on the same port, so port choice was never separation. ## #594 — `ci-image-pin` accepts any hex length Length is an invariant **separate from correctness**: the resolve/staleness checks compare resolved shas, so an 8-char pin of the right commit passes green while matching no registry tag — and all five `container:` jobs then die at image-pull with `manifest unknown`, which reads like a registry outage. - Guard fails at the gate and prints the exact tag to use. - Literal `7`, not a derived `--short=7`: in a full clone git may widen an ambiguous abbreviation, demanding a pin `ci-image.yml` can never publish — the same clone-depth asymmetry the existing comment warns about. Escape hatch documented inline. - Also fixes a **pre-existing** misdiagnosis: zero pins reported "MORE THAN ONE". - `docs/ci-cd.md` documents the 7-char rule, `git rev-parse --short=7 HEAD`, and the caveat that 7 is currently empirical (see #597). ## #485 — `.gitignore` `core` swallowed `*/Core/` source files A bare `core` matched any path component named `core`; case-insensitively on macOS that included every `*/Core/` **source** directory, so new untracked files were dropped by `git add -A` while tracked ones stayed fine — a clean local build and a CI checkout that fails to compile. Now `/core` + `/core.[0-9]*`, **both anchored** (an unanchored `core.[0-9]*` would re-introduce the very class this fixes). Subdirectory-dump coverage is a written-down accepted tradeoff. --- ## Verification | What | How | |---|---| | `.gitignore` | Full **ignored-file-set diff** before/after: identical — nothing lost its ignore. The three real dirs (`ErsatzTV.Scanner/Core`, `ErsatzTV.Scanner.Tests/Core`, `ErsatzTV.Tests/Core`) are trackable without `-f`. | | `ci-image-pin` | Step body extracted from the YAML by real parse (not hand-copied), run against doctored pins: **7 green; 6 / 8 / 10 red**, each naming the correct tag. `docker-build.yml` restored clean. | | Pre-flight | Decoy listeners: UI port busy → red naming it; **8409 busy + `ETV_UI_PORT=8420` → now passes** (previously a dead end); `ETV_STREAMING_PORT=8409` pinned busy → red naming 8409. | | Port guard | `84o9` / `abc` → red; empty → correctly falls back to the 8409 default; CI shape (`ETV_UI_PORT=8409`) unaffected. | | Gates | `bash -n` clean · `decisions_validate.py` OK · catalog regenerates byte-identically · `kickoff-guard` OK · `pr-checks.yml` parses. | **Live-E2E: not required and not run** (`release.live-e2e-required` scopes it to API write-path handlers; there is no C# change). The executable diff to `e2e-local.sh` is the pre-flight plus the env default, both tested directly above; every other line in that script is untouched. **Independent review: done, two rounds** — cold-context reviewer, no implementation role. Round 1 returned BLOCKED and found the `ETV_STREAMING_PORT` Blocker plus the 8410 factual error. Round 2 re-reviewed the fix commit and returned MERGEABLE; its remaining Medium/Low/Nit items are all folded in. ## Known, pre-existing `decisions_validate.py` emits `active-corpus 5682 lines (budget 5600)`. **`main` is already over budget**; this record widens it. Non-blocking `::warning::` ("schedule a consolidation"), not a gate. ## Follow-ups filed - **#596** — `testing.playwright-mcp-download-and-recovery` prescribes `pkill -f ms-playwright-mcp`, the same shared-host reap this PR forbids. Deferred rather than line-edited, because reversing a record is a supersede-and-relocate lifecycle operation. Flags the honest open question: a safe PID-scoped MCP recovery may not exist, in which case the rule is "restart via the client, never the shell." - **#597** — make `ci-image.yml` tag with `--short=7` so the 7-char length is enforced rather than observed. Needs its own publish-then-pin two-step, since editing `ci-image.yml` re-points `ci-image-pin`'s `expected` at that commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
timothy added 1 commit 2026-07-25 12:47:26 +02:00
chore(586,594,485): PID-scoped E2E cleanup, ci-image-pin length guard, .gitignore core fix
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 11s
PR Gates / decisions lifecycle (pull_request) Successful in 12s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 21s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m22s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 21m9s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 5m55s
c8e79f49f4
Three independent CI/repo-hygiene fixes swept together; disjoint file sets.

fixes #586 — E2E cleanup is scoped by PID, never a pattern-wide pkill
  - New decision record `testing.e2e-cleanup-scope-by-pid`.
  - docs/e2e-local.md states the constraint where a BRIEF-WRITER sees it (the
    #586 root cause was a delegation gap, not agent error).
  - scripts/e2e-local.sh: reviewed against e2e-ui.sh's trap lifecycle and
    deliberately does NOT adopt it — its contract is to hand a running instance
    back to its caller, so an EXIT trap would kill the server the instant the
    launcher returned (both callers use `OUT="$(e2e-local.sh ...)"`). Recorded.
  - Instead it gains what actually prevents the incident: an lsof pre-flight
    that NAMES a foreign listener's PID rather than letting Kestrel fail its
    bind and surface as "process N exited before becoming ready".
  - Pre-flight probes BOTH bound ports, and ETV_STREAMING_PORT now defaults to
    ETV_UI_PORT. Program.cs binds a second listener whose port defaults to 8409
    independently of ETV_UI_PORT, so `ETV_UI_PORT=8420` alone still bound 8409
    and died against a foreign holder — i.e. the documented escape hatch was a
    dead end that led straight back to the confusion behind the pattern kill.

fixes #594 — ci-image-pin accepts any hex length
  - Length is a separate invariant from correctness: the resolve/staleness
    checks compare resolved shas, so an 8-char pin of the right commit passes
    green while matching NO registry tag, and all five container: jobs then die
    at image-pull with `manifest unknown` (reads like a registry outage).
  - Guard fails at the gate and prints the exact tag to use. Verified against
    doctored pins: 7 green; 6/8/10 red.
  - Uses a literal 7 rather than a derived `--short=7`: in a full clone git may
    widen an ambiguous abbreviation, demanding a pin ci-image.yml can never
    publish. Escape hatch documented inline.
  - Also fixes a pre-existing misdiagnosis: zero pins reported "MORE THAN ONE".
  - docs/ci-cd.md documents the 7-char rule and `git rev-parse --short=7 HEAD`.

fixes #485 — .gitignore `core` silently ignored `*/Core/` files
  - A bare `core` matched any path component named `core`; case-insensitively
    on macOS that swallowed every `*/Core/` SOURCE dir, so new untracked files
    were dropped by `git add -A` while tracked ones stayed fine — a clean local
    build and a CI checkout that fails to compile.
  - Now `/core` + `/core.[0-9]*`, both anchored (an unanchored `core.[0-9]*`
    would re-introduce the same silent-exclusion class this fixes).
  - Verified by diffing the full ignored-file set before/after: identical, and
    the three real Core/ dirs are trackable without -f.

Docs updated in-PR: docs/e2e-local.md, docs/ci-cd.md, docs/decisions/
workflow-process.md (+ regenerated catalog), docs/handoffs/chicorytv-issue-queue.md.

Follow-ups filed: #596 (the same shared-host reap in the Playwright-MCP
recovery record) and the ci-image.yml `--short=7` publisher-side fix, which
cannot ride this PR — editing ci-image.yml re-points ci-image-pin's `expected`
at this commit and reds the gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
Owner

Independent adversarial review, two rounds, cold-context reviewer with no implementation role (process.independent-review-rubric).

Round 1 — BLOCKED. Found a genuine Blocker plus two High findings:

  • Blocker: the documented escape hatch ETV_UI_PORT=<other> did not work. Program.cs binds a second listener and SystemEnvironment.cs defaults StreamingPort to 8409 independently of ETV_UI_PORT, so the remedy still bound 8409 and died against the foreign holder — making #586 more likely to recur, not less.
  • High: the pre-flight probed only the UI port, so the one case a caller reaches it was a false negative.
  • High: the "CI uses 8410, local uses 8409" claim (taken from #586's body) is false on main — functional-e2e uses 8409.

Also flagged that the branch base was 2 commits behind origin/main, showing a spurious PR #588 revert in the diff.

Round 2 — re-reviewed the fix commit (release.review-verdict-gate: the fix gets reviewed, not just the original diff). Verified the streaming-port default is a literal no-op for every existing caller (CI docker-build.yml, e2e-ui.sh on the #445 branch, e2e-functional.sh), that no streaming-URL construction regresses, and that the dedup loop is safe under set -euo pipefail. Remaining Medium/Low/Nit items — a self-contradiction in the record about the 8420 run, an overstated deferral reason for #597, a missing caveat in docs/ci-cd.md, a non-numeric-port hole, and two prose nits — are all folded into the head commit.

One reviewer suggestion was declined with reason: changing ci-image.yml to --short=7. ci-image-pin derives expected from the last commit touching ci-image.yml itself, so that edit re-points the gate at its own commit and needs a publish-then-pin cycle. Split to #597 as a scope call, not a blocker — and #597's body was corrected after round 2 pointed out my first justification overstated it as impossible.

Review-verdict: MERGEABLE @ c8e79f49

Independent adversarial review, two rounds, cold-context reviewer with no implementation role (`process.independent-review-rubric`). **Round 1 — BLOCKED.** Found a genuine Blocker plus two High findings: - **Blocker**: the documented escape hatch `ETV_UI_PORT=<other>` did not work. `Program.cs` binds a second listener and `SystemEnvironment.cs` defaults `StreamingPort` to 8409 *independently* of `ETV_UI_PORT`, so the remedy still bound 8409 and died against the foreign holder — making #586 more likely to recur, not less. - **High**: the pre-flight probed only the UI port, so the one case a caller reaches it was a false negative. - **High**: the "CI uses 8410, local uses 8409" claim (taken from #586's body) is false on `main` — `functional-e2e` uses 8409. Also flagged that the branch base was 2 commits behind `origin/main`, showing a spurious PR #588 revert in the diff. **Round 2 — re-reviewed the fix commit** (`release.review-verdict-gate`: the fix gets reviewed, not just the original diff). Verified the streaming-port default is a literal no-op for every existing caller (CI `docker-build.yml`, `e2e-ui.sh` on the #445 branch, `e2e-functional.sh`), that no streaming-URL construction regresses, and that the dedup loop is safe under `set -euo pipefail`. Remaining Medium/Low/Nit items — a self-contradiction in the record about the 8420 run, an overstated deferral reason for #597, a missing caveat in `docs/ci-cd.md`, a non-numeric-port hole, and two prose nits — are all folded into the head commit. One reviewer suggestion was **declined with reason**: changing `ci-image.yml` to `--short=7`. `ci-image-pin` derives `expected` from the last commit touching `ci-image.yml` itself, so that edit re-points the gate at its own commit and needs a publish-then-pin cycle. Split to #597 as a scope call, not a blocker — and #597's body was corrected after round 2 pointed out my first justification overstated it as impossible. Review-verdict: MERGEABLE @ c8e79f49
Author
Owner

Live boot verification — closing the gap the advisory job left

The Functional E2E job (advisory) failed on infrastructure, before any of this PR's code ran, so it did not exercise the scripts/e2e-local.sh change. Root cause is inside actions/cache's bundled restore script:

ReferenceError: __dirname is not defined in ES module scope
  ❌  Failure - Main Cache NuGet packages

Step-outcome list confirms Cache NuGet packages was the only step to report; the three e2e-local strings in that log are filenames in a changed-files listing, not execution — no "Copying wwwroot", no "Launching dotnet". Not a known flake either: runs 1345/1344/1347 all had this job green with zero __dirname errors. Per ci.infra-shaped-red-under-load I'm not filing a CI bug off one sample under load, and per ci.batch-pushes-no-cancel-route/dont-repush-to-clear-a-killed-ci-job I'm not forcing a fresh run to paper over it.

Since that left my own change unexercised end-to-end, I built and booted the app locally instead. Results:

Launch Listeners bound Outcome
Direct dotnet ErsatzTV.dll, ETV_UI_PORT=8420, no ETV_STREAMING_PORT — the old documented remedy *:8420 and *:8409 ← the Blocker, reproduced
Via fixed e2e-local.sh, ETV_UI_PORT=8420 *:8420 only escape hatch now actually works
Via fixed e2e-local.sh, CI shape ETV_UI_PORT=8409 *:8409 only ready line reached, GET / → 302, kill "$PID" stopped it cleanly, port freed

The first row is the important one: it is a negative control proving the defect was real rather than inferred from reading Program.cs. An agent following the previously-documented ETV_UI_PORT=<other> advice would have bound 8409 anyway and died against the foreign holder — which is why this PR fixes the escape hatch instead of merely documenting it.

The third row is the regression check that matters for CI: the ETV_STREAMING_PORT line I added to the launch command is a literal no-op at ETV_UI_PORT=8409, exactly as claimed, so functional-e2e is unaffected once the runner recovers.

All test processes were torn down by PID (never pkill -f) — dogfooding the rule this PR adds. No strays; 8409/8420 free.

## Live boot verification — closing the gap the advisory job left The `Functional E2E` job (advisory) failed on infrastructure, **before** any of this PR's code ran, so it did not exercise the `scripts/e2e-local.sh` change. Root cause is inside `actions/cache`'s bundled restore script: ``` ReferenceError: __dirname is not defined in ES module scope ❌ Failure - Main Cache NuGet packages ``` Step-outcome list confirms `Cache NuGet packages` was the only step to report; the three `e2e-local` strings in that log are **filenames in a changed-files listing**, not execution — no "Copying wwwroot", no "Launching dotnet". Not a known flake either: runs 1345/1344/1347 all had this job green with zero `__dirname` errors. Per `ci.infra-shaped-red-under-load` I'm not filing a CI bug off one sample under load, and per `ci.batch-pushes-no-cancel-route`/`dont-repush-to-clear-a-killed-ci-job` I'm not forcing a fresh run to paper over it. Since that left my own change unexercised end-to-end, I built and booted the app locally instead. Results: | Launch | Listeners bound | Outcome | |---|---|---| | Direct `dotnet ErsatzTV.dll`, `ETV_UI_PORT=8420`, no `ETV_STREAMING_PORT` — **the old documented remedy** | `*:8420` **and `*:8409`** | ← the Blocker, reproduced | | Via fixed `e2e-local.sh`, `ETV_UI_PORT=8420` | `*:8420` only | escape hatch now actually works | | Via fixed `e2e-local.sh`, CI shape `ETV_UI_PORT=8409` | `*:8409` only | ready line reached, `GET /` → 302, `kill "$PID"` stopped it cleanly, port freed | The first row is the important one: it is a **negative control** proving the defect was real rather than inferred from reading `Program.cs`. An agent following the previously-documented `ETV_UI_PORT=<other>` advice would have bound 8409 anyway and died against the foreign holder — which is why this PR fixes the escape hatch instead of merely documenting it. The third row is the regression check that matters for CI: the `ETV_STREAMING_PORT` line I added to the launch command is a literal no-op at `ETV_UI_PORT=8409`, exactly as claimed, so `functional-e2e` is unaffected once the runner recovers. All test processes were torn down **by PID** (never `pkill -f`) — dogfooding the rule this PR adds. No strays; 8409/8420 free.
timothy merged commit 67e53b1304 into main 2026-07-25 13:51:57 +02:00
timothy deleted branch chore/586-594-485-hygiene 2026-07-25 13:51:59 +02:00
Sign in to join this conversation.