diff --git a/.claude/hooks/prepush-rebase-check.sh b/.claude/hooks/prepush-rebase-check.sh new file mode 100755 index 000000000..408589296 --- /dev/null +++ b/.claude/hooks/prepush-rebase-check.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash +# H11 (ersatztv#311) — refuse to push a branch that is BEHIND origin/main: rebase first, do NOT +# merge main in. A merge commit drags in files you never touched (e.g. the ~2500 legacy-BOM .cs), +# which then trips the pre-commit `dotnet format` hook on code that isn't yours (the #309 session). +# Rebasing keeps your diff to exactly what you changed. +# +# Fail-OPEN on anything we can't decide (a git pre-push hook has no "ask"): not a git repo, +# offline / fetch fails, no origin/main, HEAD unresolved -> allow the push. The only hard block is +# a positively-proven "behind origin/main". Deliberate exception: ETV_SKIP_REBASE_CHECK=1. +set -uo pipefail + +[ "${ETV_SKIP_REBASE_CHECK:-}" = "1" ] && exit 0 +git rev-parse --git-dir >/dev/null 2>&1 || exit 0 + +# Best-effort fetch of the latest main; offline / no network -> don't block. +git fetch origin main --quiet 2>/dev/null || exit 0 +git rev-parse --verify --quiet origin/main >/dev/null 2>&1 || exit 0 + +# Pushing main itself, or a branch already rebased on top of it, means origin/main is an ANCESTOR +# of HEAD -> nothing to rebase, allow. +if git merge-base --is-ancestor origin/main HEAD 2>/dev/null; then + exit 0 +fi + +behind=$(git rev-list --count HEAD..origin/main 2>/dev/null || echo '?') +branch=$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD) +echo "husky - push blocked (H11): '$branch' is behind origin/main by $behind commit(s)." +echo " Rebase before pushing — do NOT merge main in (a merge drags in files you didn't touch," +echo " e.g. legacy-BOM .cs, and trips the format hook on code that isn't yours):" +echo " git fetch origin main && git rebase origin/main" +echo " Deliberate exception: ETV_SKIP_REBASE_CHECK=1 git push" +exit 1 diff --git a/.gitea/workflows/docker-build.yml b/.gitea/workflows/docker-build.yml index bd5229117..665d19fc3 100644 --- a/.gitea/workflows/docker-build.yml +++ b/.gitea/workflows/docker-build.yml @@ -478,3 +478,64 @@ jobs: exit 1 fi echo "Generated API artifacts are in sync." + + # Formatting-as-you-touch gate (ersatztv#311): verify the .cs files THIS PR changed conform to + # .editorconfig (style + charset=utf-8, i.e. no UTF-8 BOM). Scoped to changed files so it enforces + # "normalize a legacy file when you touch it" WITHOUT a big-bang reformat of the ~2500 pre-existing + # BOM files. A PR that touches no .cs skips the expensive steps and passes trivially (always reports + # a status, so it is safe as a required check). + format: + name: Formatting (changed .cs conform to .editorconfig) + runs-on: ubuntu-latest + if: github.event_name == 'pull_request' + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Detect changed C# files + id: detect + run: | + base_ref="${{ github.base_ref }}" + git fetch --no-tags --depth=100 origin "$base_ref" || true + changed="$(git diff --name-only --diff-filter=ACM "origin/${base_ref}...HEAD" -- '*.cs' 2>/dev/null || true)" + echo "Changed .cs files in this PR:"; printf '%s\n' "$changed" + if [ -n "$changed" ]; then + printf '%s\n' "$changed" > /tmp/changed-cs.txt + echo "cs_changed=true" >> "$GITHUB_OUTPUT" + echo "-> will verify these files conform to .editorconfig." + else + echo "cs_changed=false" >> "$GITHUB_OUTPUT" + echo "No .cs change -> skipping format verify (job passes)." + fi + + - name: Setup .NET + if: steps.detect.outputs.cs_changed == 'true' + uses: actions/setup-dotnet@v4 + with: + dotnet-version: '10.0.x' + + - name: Cache NuGet packages + if: steps.detect.outputs.cs_changed == 'true' + uses: actions/cache@v4 + with: + path: ~/.nuget/packages + key: nuget-${{ runner.os }}-${{ hashFiles('Directory.Packages.props', 'global.json') }} + restore-keys: nuget-${{ runner.os }}- + + - name: Restore + if: steps.detect.outputs.cs_changed == 'true' + run: dotnet restore + + - name: Verify formatting of changed .cs files + if: steps.detect.outputs.cs_changed == 'true' + shell: bash + run: | + mapfile -t files < /tmp/changed-cs.txt + echo "Verifying ${#files[@]} changed .cs file(s) against .editorconfig..." + if ! dotnet format ErsatzTV.sln --no-restore --verify-no-changes --include "${files[@]}"; then + echo "::error::One or more .cs files this PR touches don't conform to .editorconfig (formatting or a UTF-8 BOM). Run 'dotnet format ErsatzTV.sln --include ' and commit the result in THIS PR — the fix-as-you-touch convention (docs/contributing.md §7; ersatztv#311). Legacy files you did NOT touch are unaffected." + exit 1 + fi + echo "All changed .cs files conform to .editorconfig." diff --git a/.husky/pre-push b/.husky/pre-push index 327f063c0..ef691778f 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -10,6 +10,11 @@ printf '%s\n' "$_prepush_refs" | ./.claude/hooks/prepush-donewhen.sh || exit 1 # diff" and lets drift through. Unset them so nested git rediscovers the repo normally. unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE +# H11 (ersatztv#311): refuse to push a branch that is BEHIND origin/main — rebase, don't merge +# main in (a merge drags in files you never touched, e.g. legacy-BOM .cs, and trips the format +# hook on code that isn't yours). Fail-open; escape with ETV_SKIP_REBASE_CHECK=1. +./.claude/hooks/prepush-rebase-check.sh || exit 1 + # CI-parity checks: catch "green locally, red in CI" before the push leaves the machine. # check:api guards the generated OpenAPI types (v1.json / v1.d.ts drift); the full # lint/typecheck/build catch a staged change that breaks an UNstaged file (lint-staged diff --git a/docs/contributing.md b/docs/contributing.md index 38ecad5a6..c4ddf6110 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -99,6 +99,15 @@ talks exclusively to the REST API. Every former Blazor route now 302-redirects t - **`.editorconfig` is the source of truth** for formatting + rule severities (plus `ErsatzTV.sln.DotSettings` for ReSharper). Run `dotnet format` before committing. +- **Fix formatting as you touch it (no big-bang).** ~2500 legacy `.cs` files inherited from upstream + carry a UTF-8 BOM, which violates `.editorconfig`'s `charset=utf-8`. We do **not** mass-reformat. + Instead, **when you modify a file for other work, normalize it in that same PR** — `dotnet format + ErsatzTV.sln --include `, which strips the BOM and fixes style. Files you did + not touch stay as-is. This is enforced two ways so it can't be silently skipped (ersatztv#311): the + pre-commit hook verifies staged `.cs`, and a blocking **`format`** CI job re-verifies the `.cs` this + PR changed against `.editorconfig` (a `.cs`-free PR passes trivially). Never `--no-verify` past a + format failure on a file you changed — de-BOM/format it instead. (A one-time repo-wide normalization + is a separate, unmade decision; the touched-file rule is the standing one.) - **`TreatWarningsAsErrors=true`** in the app projects — a warning fails the build. `NoWarn` carries a small, documented exemption list (e.g. `VSTHRD200`, `CA1873`); NuGet-audit `NU1901-1903` are demoted to warnings in `Directory.Build.props` while `NU1904` (critical) blocks. diff --git a/docs/decisions.md b/docs/decisions.md index afd1a9271..c1a95da0c 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -77,6 +77,7 @@ keep append-only from accreting stale, contradictory, or unreadably-large histor - [2026-07-12 — Review-verdict merge-gate: latest commit must be reviewed (#303 H10)](#2026-07-12--review-verdict-merge-gate-latest-commit-must-be-reviewed-303-h10) - [2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269)](#2026-07-12--cross-editor-etag-rotation-completed-for-collectionplayout-config-siblings-269) - [2026-07-12 — Live-E2E is a required step for API write-path handler changes (#303)](#2026-07-12--live-e2e-is-a-required-step-for-api-write-path-handler-changes-303) +- [2026-07-12 — Formatting-as-you-touch, enforced; rebase-not-merge for PR branches (#311 H11 + format CI)](#2026-07-12--formatting-as-you-touch-enforced-rebase-not-merge-for-pr-branches-311-h11--format-ci) --- @@ -1486,3 +1487,36 @@ silent: the PR/close comment states that live-E2E ran, or — for a non-write-pa wasn't required (the same stated-exemption discipline as the review skip rubric). Recipe + "When live-E2E is required": `docs/e2e-local.md`. This formalizes the #229 lore bullet ("live E2E remains the only net for this class") into a standing convention. + +--- + +## 2026-07-12 — Formatting-as-you-touch, enforced; rebase-not-merge for PR branches (#311 H11 + format CI) + +Two coupled process decisions, prompted when a stale docs branch *merged main in*, dragged ~17 +legacy-BOM `.cs` files it never touched into the merge commit, and the pre-commit `dotnet format` +hook then blocked on code that wasn't the author's (#309 session; the BOM backlog is #310). + +**1. Formatting-as-you-touch is the standing rule, and it is now *enforced* (not just prose).** ~2500 +of ~3900 `.cs` files carry a legacy UTF-8 BOM that violates `.editorconfig`'s `charset=utf-8`. We do +**not** mass-reformat (a repo-wide normalization stays an unmade, separate decision). Instead, a file +you modify for other work must be normalized (`dotnet format`, incl. BOM strip) in that same PR. +Enforcement — previously only the `--no-verify`-bypassable pre-commit hook, which is how #269 landed 17 +BOM files (CI never checked charset): +- a **blocking `format` CI job** runs `dotnet format --verify-no-changes` **scoped to the PR's changed + `.cs`** (vs the merge-base) — so it demands conformance only of files the PR touched, never the + untouched legacy 2500; a `.cs`-free PR skips the expensive steps and passes (always reports a status, + safe as a required check). This closes the "CI never verifies charset/format" gap. +- `docs/contributing.md` §7 documents the rule. + +**2. Keep a PR branch current by REBASING on `origin/main`, never merging main in (H11).** A merge +commit pulls in *every* file main changed — including files the author never touched — which then trip +the format hook/CI on code that isn't theirs; rebasing keeps the diff to exactly what changed. +Enforced by `.claude/hooks/prepush-rebase-check.sh` wired into `.husky/pre-push`: a push from a branch +that is behind `origin/main` (origin/main not an ancestor of HEAD) is **blocked** with +`git rebase origin/main` guidance. Fail-open (offline / no origin/main / not a repo → allow, since a +git hook has no "ask"); deliberate escape `ETV_SKIP_REBASE_CHECK=1`. This supersedes the old lore +guidance to "merge main into your PR branch." (After a rebase that conflicts in *generated* artifacts — +v1.json/v1.d.ts/endpoint-index — regenerate, don't hand-resolve; `npm run check:api` guards.) + +Rationale, as with the whole hook program: make the process rule a derivation/hook, not prose to +remember (#303 methodology review). Tracked: #311; sibling #312 (H12 issue-qualification audit). diff --git a/docs/handoffs/chicorytv-issue-queue.md b/docs/handoffs/chicorytv-issue-queue.md index 10543b61a..c58f0c900 100644 --- a/docs/handoffs/chicorytv-issue-queue.md +++ b/docs/handoffs/chicorytv-issue-queue.md @@ -62,9 +62,11 @@ Then work the queue: reviewer-repo audits are claimed by comment only. 4. Read the issue bodies (they carry the task context/evidence) and work the item under the HARD CONSTRAINTS below. -5. Finish by following the session-end protocol in #237: ONE session comment on the tracker - (template in the tracker body, incl. triage verdicts for any new issues), remove your - `in-progress` labels, and complete the per-issue Task Completion Protocol from CLAUDE.md. +5. Finish by following the session-end protocol in #237: run the **H12 qualification audit** + (`ETV_GITEA_BASICAUTH=user:pass scripts/issue-qualification-audit.sh`) and add a `priority:` + label to anything it lists (every issue you filed this session included); then ONE session + comment on the tracker (template in the tracker body, incl. triage verdicts for any new issues), + remove your `in-progress` labels, and complete the per-issue Task Completion Protocol from CLAUDE.md. HARD CONSTRAINTS: - Work in worktrees off origin/main. Copy web/node_modules from the main checkout. @@ -153,10 +155,22 @@ HARD CONSTRAINTS: survived a green fixed-point test because the handler returned a lazy `Map` the test never enumerated. Enumerate handler return values in tests; live E2E remains the only net for this class. (Write-path reload-through-read-path mechanics: api-conventions §7.) -- **Merging main into an open PR branch: REGENERATE generated artifacts** (v1.json, v1.d.ts, - endpoint-index) after resolving source conflicts — git text-merges them plausibly-but-wrong; - `npm run check:api` is the guard. Also expect other sessions to push to YOUR PR branch - (merge-main updates): `git pull` before push, treat non-FF as signal not error. +- **Keep a PR branch current by REBASING on origin/main — do NOT merge main in** (ersatztv#311, + enforced by the H11 pre-push hook: a push from a branch behind origin/main is blocked → `git fetch + origin main && git rebase origin/main`). A *merge* commit pulls in every file main changed, incl. + files you never touched (e.g. the ~2500 legacy-BOM `.cs`), which then trip the format hook/CI on code + that isn't yours (the #309 session). Rebasing keeps your diff to exactly what you changed. After a + rebase that hits conflicts in **generated artifacts** (v1.json, v1.d.ts, endpoint-index), REGENERATE + them (`./scripts/update-openapi.sh` + `npm run generate:api`) — never hand-resolve; git text-merges + them plausibly-but-wrong and `npm run check:api` is the guard. Escape hatch for a deliberate + non-rebased push: `ETV_SKIP_REBASE_CHECK=1 git push`. +- **H12 issue-qualification audit** (ersatztv#312, `scripts/issue-qualification-audit.sh`): a + session-end check that lists OPEN issues missing a `priority:` label — the #237 ranking keys off + `priority:`/gate labels, so an unlabeled issue is invisible to it. "Fully qualified" = has a + `priority: {high,medium,low}` label (that signals triage ran; gate-vs-backlog is then derivable + from the `review` label / milestone, and a milestone is NOT required — backlog is unmilestoned). + Run it at session end and label anything it flags (esp. issues you filed this session). Fail-open + without creds; advisory (exit 1 when any are unqualified). Sibling to H11 (both #311/#312). - **CI VM test timeouts**: heavy-render web tests (100+ item grids) need explicit vitest timeouts (e.g. 15s) — the CI VM hit the 5s default on a test that runs in ~1s locally (run 686). Bump per-test, don't raise the global default. diff --git a/scripts/issue-qualification-audit.sh b/scripts/issue-qualification-audit.sh new file mode 100755 index 000000000..6c4e7f61a --- /dev/null +++ b/scripts/issue-qualification-audit.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# H12 (ersatztv#312) — session-end audit: list OPEN issues that are not "fully qualified" for the +# #237 ranking protocol. An issue is qualified when it carries a `priority:` label — that is the +# signal triage happened; gate-vs-backlog is then derivable (a `review` label / open milestone = +# gate, otherwise backlog), and a milestone is deliberately NOT required (backlog is unmilestoned). +# So the one mandatory check is: does the issue have a `priority: {high,medium,low}` label? +# +# Run it at session end (kickoff session-end protocol) and qualify anything it lists before closing. +# Advisory: prints the under-qualified set and exits 1 if any exist, 0 if all clean. Fail-OPEN with +# no creds / Gitea unreachable (prints a notice, exits 0 — never a spurious signal). +# +# Creds from env: ETV_GITEA_TOKEN or ETV_GITEA_BASICAUTH (user:pass). ETV_GITEA_URL overrides the +# base (default: the LAN instance); ETV_GITEA_REPO overrides owner/repo (default timothy/ersatztv). +set -uo pipefail + +base_url="${ETV_GITEA_URL:-http://192.168.1.95:3000}/api/v1" +repo="${ETV_GITEA_REPO:-timothy/ersatztv}" + +gq() { + local path="$1" + if [ -n "${ETV_GITEA_TOKEN:-}" ]; then + curl -sf -H "Authorization: token $ETV_GITEA_TOKEN" "$base_url/$path" 2>/dev/null + elif [ -n "${ETV_GITEA_BASICAUTH:-}" ]; then + curl -sf -u "$ETV_GITEA_BASICAUTH" "$base_url/$path" 2>/dev/null + else + return 1 + fi +} + +if [ -z "${ETV_GITEA_TOKEN:-}" ] && [ -z "${ETV_GITEA_BASICAUTH:-}" ]; then + echo "H12 audit: no Gitea creds in env (ETV_GITEA_TOKEN or ETV_GITEA_BASICAUTH) — skipping (no-op)." + exit 0 +fi + +# Page through OPEN issues (type=issues excludes PRs; .pull_request guard is belt-and-suspenders). +# Emit "#Ntitle" for any issue with NO 'priority:' label. +missing="" +page=1 +while :; do + batch=$(gq "repos/$repo/issues?state=open&type=issues&limit=50&page=$page") || { + echo "H12 audit: Gitea unreachable — skipping (no-op)."; exit 0; } + count=$(printf '%s' "$batch" | jq 'length' 2>/dev/null || echo 0) + [ "${count:-0}" -eq 0 ] && break + rows=$(printf '%s' "$batch" | jq -r ' + .[] + | select(.pull_request == null) + | select(([ (.labels // [])[].name | select(startswith("priority:")) ] | length) == 0) + | "#\(.number)\t\(.title)"' 2>/dev/null || true) + [ -n "$rows" ] && missing="${missing}${rows}"$'\n' + page=$((page + 1)) +done + +if [ -z "$(printf '%s' "$missing" | tr -d '[:space:]')" ]; then + echo "H12 audit: every open issue carries a priority: label. ✓" + exit 0 +fi +echo "H12 audit: OPEN issues missing a 'priority:' label — qualify these before session end" +echo "(the #237 ranking keys off priority:/gate labels; an unlabeled issue is invisible to it):" +printf '%s' "$missing" | sed '/^[[:space:]]*$/d' | sed 's/^/ /' +exit 1