7 Commits

Include renames

Author	SHA1	Message	Date
timothyandtimothy	45b17e58e1	fix(744,835): ci-image.yml publishes from main only; guard persist-credentials with no exemption (#857) ... Closes the push route into ci-image.yml (#744) and ships the persist-credentials guard that was waiting on it (#835). ci-image.yml's push trigger had no branches: filter and was path-scoped to docker/ci/** AND to the workflow file itself. Gitea resolves a push workflow's definition from the pushed ref, so any branch push touching those paths ran that branch's own YAML on a docker-capable runner holding the credential that writes ersatztv:prod and the ersatztv-ci:<sha> five container: jobs execute. Be precise about what the filter buys: it is loaded from the pushed ref like the rest of the file, so a branch that deletes it re-enables the route. This closes the DRIVE-BY case - publication as a side effect of an ordinary push - and is not a boundary against a writer who intends to run their own YAML. The wider class is #853. The self-reference left both paths: and ci-image-pin's expected in the same change - a decided tradeoff with both prices stated, not a necessity. Branch publishing moves to workflow_dispatch, probed live: run 2340 on this branch published ersatztv-ci:43b1e45 and left :latest unchanged. With both mechanical blockers gone, ci-image.yml's checkout takes persist-credentials: false (16 of 16) and scripts/tests/test_workflow_persist_credentials.py holds the convention with NO exemption list - git-index population, declared clause mutation re-run every suite, guard-inventory rows. fixes #744 fixes #835 Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-27 20:44:44 +00:00
timothy	352aa70634	refactor(445): drop e2e-ui.sh's port pre-flight, now owned by e2e-local.sh (#586/#598) ... main's #598 added a port pre-flight to scripts/e2e-local.sh while this branch was in review, and theirs is strictly better than the one I had here: - it probes BOTH bound ports. The app binds a SECOND (streaming) listener that defaults to 8409 regardless of ETV_UI_PORT, so my single-port check could certify a port free while the run still died binding 8409. That also means this PR's CI step (ETV_UI_PORT=8410) was only working because the curl step's server had already been killed — #598's fix, defaulting ETV_STREAMING_PORT to the given port, is what makes it correct rather than lucky. A real latent bug in my work, surfaced by their change. - it REPORTS rather than reaps, which is the #586 rule. Keeping mine would leave two divergent port checks on the same concern. Also records the app's SINGLE-INSTANCE guard, which is independent of ports: a stray instance blocks a run whatever port you choose, so 'pick another port' is not a workaround. Kill the leftover BY PID (#586). Refs #445 #586	2026-07-25 14:29:27 +02:00
timothy	54919e4730	fix(445): pidfile-first cleanup ordering + clear BOOT_PID after wait ... Codex round-3 verdict was BLOCKED @ 29bc5119 with three High findings. All three are real; all three are fixed. They are narrower than the previous rounds — two are sub-millisecond races between adjacent statements — but the fixes are cheap and make the ordering correct rather than lucky. **Recycled-PID kill (the one that genuinely mattered).** `BOOT_PID` stayed set after `wait` had already reaped the launcher. A stale PID is not merely useless: the OS can RECYCLE that number during a long spec run, and cleanup would then SIGTERM an unrelated process. This is the same "never kill something you did not prove is yours" failure that got the earlier port-based reap deleted — it came back in a different disguise. Cleared immediately after `wait` returns. **Cleanup ordering.** cleanup() killed e2e-local.sh BEFORE reading the pidfile, so a signal landing between the server fork and the pidfile write could kill the launcher and lose the only handle on an already-running server. The pidfile is now read FIRST; stopping the launcher is best-effort and never the primary handle. **Fork/publish sliver.** A signal can also land between `... &` and `BOOT_PID=$!`, or between e2e-local.sh's fork and its pidfile write — the pidfile reads empty, then populates microseconds later, and the server is orphaned. cleanup now does a bounded grace re-read (~1s) when it has no PID. Verified this does NOT cost anything on a genuinely server-less abort: a bad ETV_BUILD_CONFIG still exits in 1s rather than sitting out the loop. ## Verification - passing run 3/3; failing run exits 1 - server-less abort exits in 1s (grace loop stays bounded) - targeted SIGTERM: exit 143, no listener left - zero orphaned processes after the full gate Refs #445	2026-07-25 14:04:52 +02:00
timothy	e8ad79623f	fix(445): make the boot wait interruptible so a mid-boot signal can't orphan the server ... Codex round-2 verdict was BLOCKED @ 2a5f26c0 on one High finding, which was correct and is the subtlest defect in this whole change. ## The defect Bash DEFERS a trapped signal while it waits on a FOREGROUND command. The boot was OUT="$(ETV_UI_PORT=... scripts/e2e-local.sh "$CONFIG_DIR")" so a TERM arriving during the (up to 120s) readiness wait did not run `on_signal` until boot completed — and a supervisor escalating TERM->KILL means the trap never runs at all, orphaning the server on its port. The pidfile added in the previous commit only helps IF the trap runs; this is the case where it doesn't. Proved the mechanism in isolation rather than asserting it, since the real boot is only ~2s locally and kept winning the race: foreground command substitution : TERM at t=1s -> trap ran at t=6s (deferred) background job + `wait` : TERM at t=1s -> trap ran at t=1s (prompt) ## The fix Boot in the background and `wait` on it. `wait` is interruptible, so the handler runs immediately; by then e2e-local.sh has already written $ETV_PIDFILE, so cleanup can reap a server whose PID this script has not yet parsed. Cleanup also now stops the backgrounded e2e-local.sh itself, so it cannot sit waiting on a server we just killed, and removes its temp output file. Boot diagnostics are preserved on the failure path (verified with a deliberately bad ETV_BUILD_CONFIG: the underlying "bin/<config> does not exist" error is surfaced, along with the exit status, which is now propagated rather than flattened to 1). ## Verification - mechanism: deferred-vs-prompt trap proven as above - passing run 3/3 green; failing run (--grep ZZZ_NOPE) exits 1 - boot-failure path prints e2e-local.sh's real error and its exit status - targeted SIGTERM: exit 143, no listener, no stray process - curl harness unaffected: 45/45 PASS - no orphaned processes after the full gate (an earlier count of 1 was a graceful shutdown still in flight, not a leak — re-checked clean) Refs #445	2026-07-25 14:04:52 +02:00
timothy	71706f3849	fix(445): silent-pass on spec failure + prove server ownership via pidfile [decisions-edit] ... 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	2026-07-25 14:04:52 +02:00
timothy	0f1951340e	fix(445,533): harden e2e-ui lifecycle + retire stale #533 decision record [decisions-edit] ... Addresses the adversarial review round. Codex returned BLOCKED on the harness lifecycle contract; the second (cold) reviewer independently flagged the same trap-ordering defect, which is what made it credible. **Trap installed AFTER boot -> signal mid-boot orphans the server.** The window between `e2e-local.sh` returning and `trap ... EXIT INT TERM` had no handler, so a Ctrl-C/TERM there (or the PID-parse bail-out) left dotnet holding the port — violating the script's own "always kills the server" contract. The trap is now installed BEFORE boot. When the PID is not yet known, cleanup falls back to reaping whatever LISTENS on our port; attribution is sound because the pre-flight proved that port free moments earlier. **Signals were not re-raised.** A TERM landing beside a passing Playwright run could exit 0, reporting success for a cancelled run. INT/TERM now clean up and re-raise, so the wrapper dies BY the signal. Verified, not assumed: SIGTERM mid-run -> wrapper exits 143 (128+15) and leaves zero listeners and zero stray ErsatzTV processes. `e2e-local.sh` backgrounded a MULTI-command subshell, so `$!` is the subshell — not dotnet — wherever bash does not collapse it. Measured both ways: bash 3.2.57 (stock macOS /bin/bash), 2-command subshell : $! = SUBSHELL bash 5.3.15 (homebrew) : $! = leaf with `exec` (both versions) : $! = leaf Consequence on stock-macOS bash: every PID-based kill/liveness check targeted the wrong process, the escalation silently no-opped, and the server leaked. Fixed at the source with `exec`, which benefits all consumers (the CI step's trap and the e2e-functional.sh pairing), not just e2e-ui.sh. My first attempt to test this was WRONG and would have cleared the finding: a single-command subshell is collapsed on both versions. Only the 2-command form reproduces it. `testing.e2e-local-fresh-config-dir` was still `status: active` asserting the bug 'wait for either line' option today", "widening the probe ... is tracked as #533". `READY_LINE` existed ONLY in that record; nowhere in code. Amended rather than superseded: the operative rule (use a fresh config dir) is unchanged and still correct — only its RATIONALE moved from "the probe hangs" to "state bleed". Heading deliberately left alone: the validator matches records by `## HEADING`, so renaming fails CI as an "unlogged removal"; an explicit note now tells the reader the heading is historical. - Boot-failure diagnostics were lost: `OUT="$(...)"` aborts under `set -e` before the print. Now `if ! OUT=$(...)` so the output is shown. - Playwright failures surfaced no server-side evidence (nothing uploads the traces in web/e2e/.output). The server log tail is now printed on failure. - `lsof -ti :PORT` also matched outbound/TIME_WAIT sockets -> false positives. Now `-sTCP:LISTEN`. Documented honestly that the CI image ships no lsof, so the pre-flight is a local-developer guard only. - Predictable `/tmp/etv-pw-probe.$$` -> `mktemp` (symlink-redirect on a shared host). - Suggested escape-hatch port was 8411, which is scripts/security-scan.sh's default; 8409/8410 are prod/ersatztv-test on jazz. Now suggests 8419 and names the conflicts. - boot-gate.spec.ts comment overclaimed: after logout the browser holds NO cookie, so that assertion cannot prove stamp rotation (the curl harness does, by replaying the same cookie). Comment corrected to what it actually proves. Deliberately NOT changed: `ci-image-pin`'s regex accepts any hex length rather than the exact 7 chars ci-image.yml publishes (pre-existing guard weakness, not introduced here — filed as a follow-up rather than widened in this PR). Also trimmed the two records I had bloated: the active decisions corpus went over its 5600-line budget as a result of this PR (baseline on main was under), so the overflow was mine to pay down, not to pass on. - SIGTERM mid-run: exit 143, no orphan listener/process - curl harness unaffected by the `exec` change: 45/45 PASS - UI-E2E 2x clean; typecheck + lint clean; decisions validator OK, no size warning Refs #445 #533	2026-07-25 14:04:52 +02:00
timothy	d8c0b3e752	feat(445,533): headless Playwright UI-E2E flows + fix e2e-local readiness probe [decisions-edit] ... Adds the last deferred #299/#363 follow-up: the flows that CANNOT be expressed as curl calls. Scope rule (the durable part) — assert only what the curl harness structurally cannot reach: 1. client-side form validation (the Setup confirm-password gate is pure React state and makes no request, so there is no HTTP contract to assert) 2. AuthGate's RENDERED states (Setup vs Login vs app) 3. the session cookie authenticating the SPA's OWN /api XHRs — curl proves the cookie works for curl, not that the app sends it 4. sign-out through the UserMenu back to the login gate New: web/e2e/boot-gate.spec.ts, web/playwright.config.ts, scripts/e2e-ui.sh (owns the whole lifecycle: fresh config dir -> boot -> specs -> always kill). Runs as a second step of the EXISTING advisory `functional-e2e` job rather than a new job: the dominant cost there is `npm ci` + the Release build, both already done, so this adds ~5s instead of duplicating a heavy job. It boots its own fresh instance on port 8410 because the first spec asserts the one-shot Setup gate that the curl step has already claimed on its config dir. Determinism (the issue asked for it explicitly): `serial`, `workers: 1`, `retries: 0` even in CI — a retry would let a flaky flow merge looking green. Measured 5 consecutive clean runs, ~2s each. Pins all five `container:` jobs to the toolchain image built by the preceding commit, which bakes `chromium-headless-shell`. Non-obvious coupling fixed: vitest's default include glob would have collected web/e2e/*.spec.ts and run it under jsdom. Excluded `e2e/**` by spreading `configDefaults.exclude` rather than narrowing `include` to `src/**`, because web/scripts/ holds a real vitest test an src-only include would silently stop running. `RebuildSearchIndexHandler` logs one of two mutually-exclusive lines just before `SystemStartup.SearchIndexIsReady()`: fresh config -> "Done migrating search index in {Duration}" reused config -> "Search index is already version {Version}" The probe watched only the first, so a reused dir waited out the full 120s timeout and then killed a perfectly healthy server. Widened to a `grep -Eq` alternation; the handler's if/else is exhaustive, so the pair covers every path to readiness. Verified with a negative control: on a reused dir the server is ready in 2s via the "already version" line, and the OLD probe string is genuinely ABSENT from that run's log — so the old code would have hung, i.e. the fix is load-bearing rather than incidentally passing. The "prefer a fresh config dir" guidance stays: that guards state bleed, which is a separate concern from the probe hanging. - `wait "$PID"` in the cleanup trap was a NO-OP: the server is a grandchild (launched in e2e-local.sh's subshell, which then exits), so `wait` fails instantly and was swallowed by `|| true` — cleanup did not actually ensure the port was released, exactly what its comment claimed. Replaced with a bounded `kill -0` poll, then SIGKILL. - Added a port pre-flight check: previously an occupied port surfaced as a 120s readiness timeout that reads like a broken build. Now fails in 0s naming the PIDs, and warns against blanket-killing `dotnet ErsatzTV.dll` (that reaps other sessions' servers). - UI-E2E: 5x clean (3 specs, ~2s); back-to-back runs pass with no manual cleanup - curl harness unaffected by the boot-script change: 45/45 PASS - web: 983 tests / 105 files green; typecheck + lint clean - vitest collection verified: excludes web/e2e, still collects web/scripts - Dockerfile sequence + browser launch validated verbatim in a container on the real amd64 base before committing; chromium launches as root with NO sandbox opt-out needed - decisions validator green; catalog regenerated - docs/decisions.md TOC repaired: it had drifted to 69 of 97 records and held a dangling anchor to the #72 record that #415 superseded into archive/. Regenerated with a generator validated against the 68 existing anchors (0 mismatches) -> 97/97, no dangling, no duplicates. Docs: docs/e2e-local.md (new "UI-E2E harness" section), docs/ci-cd.md (toolchain image + UI-E2E step), docs/testing.md, docs/README.md, docs/decisions.md (new `ci.ui-e2e-harness` record; `ci.functional-e2e-harness` amended — its Rule said "curl-only", now accurate). Refs #445 #533	2026-07-25 14:03:59 +02:00