timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

Adopt a C# lint/format/static-analysis stack (the ruff equivalent) and enforce in CI #15

New Issue
Closed
opened 2026-06-27 00:06:42 +02:00 by timothy · 7 comments
timothy commented 2026-06-27 00:06:42 +02:00
Owner
Copy Link
Copy Source

Goal

Stand up the C#/.NET equivalent of Python's ruff — consistent formatting + static analysis — and actually enforce it in CI. The pieces exist; they're just not turned up or applied uniformly.

What's already in place

  • .editorconfig — the config (formatting + rule severities), analogous to ruff.toml.
  • Roslyn / .NET SDK analyzers — active (the NoWarn for CA1873 proves CAxxxx rules run).
  • Microsoft.VisualStudio.Threading.Analyzers — wired in Directory.Build.targets, but only when EnableThreadingAnalyzers=true.
  • TreatWarningsAsErrors — on in the app projects (so findings already block their build).

What to add / enforce

  1. Formatting in CI — dotnet format --verify-no-changes (whitespace + style + analyzers) so .editorconfig is enforced, not aspirational. (The Docker build now copies .editorconfig too, so local/CI/image agree.) Optionally evaluate CSharpier for opinionated, zero-config formatting.
  2. Turn up the analyzers centrally via Directory.Build.props (so every project gets them, pinned once — pairs with Central Package Management in #14):
    • SDK analyzers: <EnableNETAnalyzers>true</EnableNETAnalyzers>, raise <AnalysisMode> (Recommended→All), enable threading analyzers repo-wide.
    • Curated packs: Roslynator (largest set + CLI), SonarAnalyzer.CSharp (bugs/security/smells), Meziantou.Analyzer, StyleCop.Analyzers (style), AsyncFixer.
    • Configure all severities centrally in .editorconfig.
  3. Enforce in CI — a lint step (or warnings-as-errors) so violations block, like ruff in CI.
  4. Coverage — collect with coverlet and publish a summary; decide on a floor later.

Approach (important)

Introduce incrementally — turning every rule to error at once would bury us in a wall of pre-existing findings. Start new rules at suggestion/warning, fix or baseline, then promote to error. Keep changes consistent with the established style (the "follow established patterns" principle / contributors guide #10) rather than letting a tool reformat the whole tree at once.

Payoff

Style and a broad class of bugs/smells/security issues get caught automatically and uniformly — the regression/consistency net Python gets from ruff. Backlog. Related: #10 (contributors guide), #14 (Central Package Management).

Done-when

  • Curated analyzer packs are centrally configured and default to non-blocking suggestion severity.
  • SDK analyzers run centrally at latest-All and the threading analyzer is enabled repo-wide.
  • Formatting is enforced for changed C# files locally and in blocking CI.
  • CI collects authored-code coverage and publishes a human-readable summary without a floor.
  • A high-value analyzer rule is promoted centrally to a CI-blocking severity.
  • Every current occurrence of that promoted rule is fixed with regression coverage where behavior changed.
  • Static-analysis and formatting documentation reflects the shipped posture and explicit deferrals.
  • Full restore, build, and test verification passes.
  • Adversarial review passes on the final PR head.
## Goal Stand up the C#/.NET equivalent of Python's `ruff` — consistent formatting + static analysis — and actually **enforce** it in CI. The pieces exist; they're just not turned up or applied uniformly. ## What's already in place - **`.editorconfig`** — the config (formatting + rule severities), analogous to `ruff.toml`. - **Roslyn / .NET SDK analyzers** — active (the `NoWarn` for `CA1873` proves CAxxxx rules run). - **`Microsoft.VisualStudio.Threading.Analyzers`** — wired in `Directory.Build.targets`, but only when `EnableThreadingAnalyzers=true`. - **`TreatWarningsAsErrors`** — on in the app projects (so findings already block their build). ## What to add / enforce 1. **Formatting in CI** — `dotnet format --verify-no-changes` (whitespace + style + analyzers) so `.editorconfig` is enforced, not aspirational. (The Docker build now copies `.editorconfig` too, so local/CI/image agree.) Optionally evaluate **CSharpier** for opinionated, zero-config formatting. 2. **Turn up the analyzers** centrally via `Directory.Build.props` (so every project gets them, pinned once — pairs with Central Package Management in #14): - SDK analyzers: `<EnableNETAnalyzers>true</EnableNETAnalyzers>`, raise `<AnalysisMode>` (Recommended→All), enable threading analyzers repo-wide. - Curated packs: **Roslynator** (largest set + CLI), **SonarAnalyzer.CSharp** (bugs/security/smells), **Meziantou.Analyzer**, **StyleCop.Analyzers** (style), **AsyncFixer**. - Configure all severities centrally in `.editorconfig`. 3. **Enforce in CI** — a lint step (or warnings-as-errors) so violations block, like ruff in CI. 4. **Coverage** — collect with coverlet and publish a summary; decide on a floor later. ## Approach (important) Introduce **incrementally** — turning every rule to `error` at once would bury us in a wall of pre-existing findings. Start new rules at `suggestion`/`warning`, fix or baseline, then promote to `error`. Keep changes consistent with the established style (the "follow established patterns" principle / contributors guide #10) rather than letting a tool reformat the whole tree at once. ## Payoff Style and a broad class of bugs/smells/security issues get caught automatically and uniformly — the regression/consistency net Python gets from ruff. Backlog. Related: #10 (contributors guide), #14 (Central Package Management). ## Done-when - [x] Curated analyzer packs are centrally configured and default to non-blocking suggestion severity. - [x] SDK analyzers run centrally at `latest-All` and the threading analyzer is enabled repo-wide. - [x] Formatting is enforced for changed C# files locally and in blocking CI. - [x] CI collects authored-code coverage and publishes a human-readable summary without a floor. - [x] A high-value analyzer rule is promoted centrally to a CI-blocking severity. - [x] Every current occurrence of that promoted rule is fixed with regression coverage where behavior changed. - [x] Static-analysis and formatting documentation reflects the shipped posture and explicit deferrals. - [x] Full restore, build, and test verification passes. - [x] Adversarial review passes on the final PR head.
timothy changed title from CI code-quality gate: dotnet format (+ coverage report) to Adopt a C# lint/format/static-analysis stack (the ruff equivalent) and enforce in CI 2026-06-27 00:08:07 +02:00
timothy added the ci-cdenhancementpriority: medium labels 2026-07-11 19:03:41 +02:00
timothy added the in-progress label 2026-07-16 18:39:25 +02:00
timothy commented 2026-07-16 18:39:28 +02:00
Author
Owner
Copy Link
Copy Source

Claiming for a bounded, single increment — this issue stays open as an increments tracker.

Live state of #15's scope:

  • ✅ Curated analyzer packs (Roslynator, SonarAnalyzer, Meziantou, AsyncFixer) wired repo-wide in Directory.Build.targets (StyleCop deliberately omitted, rationale in-file).
  • ✅ Formatting enforced in CI — dotnet format --verify-no-changes, fix-as-you-touch scoped to changed .cs (#311).
  • ✅ AnalysisLevel=latest-Recommended set per project.
  • ⬜ Coverage (scope item 4) — coverlet.collector is referenced by the test projects but the CI Test step never passes --collect, and nothing publishes a summary.
  • ⬜ Rule promotions (suggestion→warning/error) — ongoing, out of scope for this increment (higher-risk; see #25 for the razor Sonar burn-down).

This increment: wire coverage collection into the CI test step (Cobertura via coverlet) and publish a human-readable summary (no floor yet, per "decide on a floor later"). CI-only, low risk. Will land as a follow-up-friendly PR referencing (not closing) #15.

Claiming for a **bounded, single increment** — this issue stays open as an increments tracker. Live state of #15's scope: - ✅ Curated analyzer packs (Roslynator, SonarAnalyzer, Meziantou, AsyncFixer) wired repo-wide in `Directory.Build.targets` (StyleCop deliberately omitted, rationale in-file). - ✅ Formatting enforced in CI — `dotnet format --verify-no-changes`, fix-as-you-touch scoped to changed `.cs` (#311). - ✅ `AnalysisLevel=latest-Recommended` set per project. - ⬜ **Coverage (scope item 4)** — `coverlet.collector` is referenced by the test projects but the CI `Test` step never passes `--collect`, and nothing publishes a summary. - ⬜ Rule promotions (suggestion→warning/error) — ongoing, out of scope for this increment (higher-risk; see #25 for the razor Sonar burn-down). **This increment:** wire coverage collection into the CI test step (Cobertura via coverlet) and publish a human-readable summary (no floor yet, per "decide on a floor later"). CI-only, low risk. Will land as a follow-up-friendly PR referencing (not closing) #15.
timothy removed the in-progress label 2026-07-16 19:29:49 +02:00
timothy commented 2026-07-16 19:30:08 +02:00
Author
Owner
Copy Link
Copy Source

Increment landed: coverage collection + summary (PR #370, merged 3654fad1)

Scope item 4 ("collect with coverlet and publish a summary; decide on a floor later") is done:

  • CI Test step now runs --collect:"XPlat Code Coverage" --settings coverlet.runsettings; a best-effort Coverage summary step publishes a ReportGenerator summary (TextSummary → log, MarkdownSummaryGithub → job step summary). No floor enforced yet, continue-on-error so it never blocks a build.
  • Added coverlet.collector to ErsatzTV.Core.Tests + ErsatzTV.Tests (now uniform across all six *.Tests).
  • Gotcha for future coverage work (root cause of a CI OOM caught mid-PR): the whole-solution Architecture.Tests process instruments both EF migration folders (~2.59M generated lines vs ~200k authored), which OOM-killed the shared job (exit 137). coverlet.runsettings excludes **/Migrations/*.cs + generated code → instrumented surface dropped 2,510,185 → 19,963 coverable lines (~126×). Any future coverage tuning must keep that exclusion.
  • Documented in docs/ci-cd.md (test job).

Still open on this tracker (leaving #15 open):

  • Rule promotions suggestion→warning/error (ongoing; razor Sonar burn-down tracked separately as #25).
  • Full-tree dotnet format normalization (deliberately deferred; fix-as-you-touch via #311 in the meantime).
  • Coverage floor — deferred by design ("decide on a floor later"); revisit once the summary has produced a few baselines.

Removing my in-progress claim.

### Increment landed: coverage collection + summary (PR #370, merged `3654fad1`) Scope item 4 ("collect with coverlet and publish a summary; decide on a floor later") is **done**: - CI `Test` step now runs `--collect:"XPlat Code Coverage" --settings coverlet.runsettings`; a best-effort `Coverage summary` step publishes a ReportGenerator summary (TextSummary → log, MarkdownSummaryGithub → job step summary). No floor enforced yet, `continue-on-error` so it never blocks a build. - Added `coverlet.collector` to `ErsatzTV.Core.Tests` + `ErsatzTV.Tests` (now uniform across all six `*.Tests`). - **Gotcha for future coverage work** (root cause of a CI OOM caught mid-PR): the whole-solution `Architecture.Tests` process instruments **both EF migration folders (~2.59M generated lines vs ~200k authored)**, which OOM-killed the shared job (`exit 137`). `coverlet.runsettings` excludes `**/Migrations/*.cs` + generated code → instrumented surface dropped **2,510,185 → 19,963 coverable lines (~126×)**. Any future coverage tuning must keep that exclusion. - Documented in `docs/ci-cd.md` (test job). **Still open on this tracker** (leaving #15 open): - Rule promotions suggestion→warning/error (ongoing; razor Sonar burn-down tracked separately as #25). - Full-tree `dotnet format` normalization (deliberately deferred; fix-as-you-touch via #311 in the meantime). - Coverage **floor** — deferred by design ("decide on a floor later"); revisit once the summary has produced a few baselines. Removing my `in-progress` claim.
timothy added the in-progress label 2026-07-16 19:33:37 +02:00
timothy commented 2026-07-16 19:33:37 +02:00
Author
Owner
Copy Link
Copy Source

Claiming #15 for this session. I’ll first reconcile the original checklist against the analyzers, formatting hooks, CI jobs, and coverage support that have landed since the issue was filed, then implement and verify only the remaining enforceable gap(s) in a worktree off origin/main.

Claiming #15 for this session. I’ll first reconcile the original checklist against the analyzers, formatting hooks, CI jobs, and coverage support that have landed since the issue was filed, then implement and verify only the remaining enforceable gap(s) in a worktree off `origin/main`.
timothy commented 2026-07-16 19:43:52 +02:00
Author
Owner
Copy Link
Copy Source

Reconciliation found one clean final increment: Sonar S3981 reports the production condition Workers.Count >= 0, which is always true. Root cause: the non-empty check used >= instead of >, so scheduled non-forced memory releases always classified the process as busy and never selected the intended aggressive idle-GC branch. Scope is now fixed: change that predicate, add idle/active-worker regression coverage, promote S3981 to warning repo-wide (there is exactly one current occurrence), and update the static-analysis/formatting docs. Async lifetime findings were reviewed and excluded as behaviorally broader. Once this increment passes full verification and adversarial review, the original four #15 outcomes are complete; full-tree normalization and a coverage floor remain explicitly deferred, so #15 can close rather than remain a perpetual automatic pickup.

Reconciliation found one clean final increment: Sonar `S3981` reports the production condition `Workers.Count >= 0`, which is always true. Root cause: the non-empty check used `>=` instead of `>`, so scheduled non-forced memory releases always classified the process as busy and never selected the intended aggressive idle-GC branch. Scope is now fixed: change that predicate, add idle/active-worker regression coverage, promote `S3981` to `warning` repo-wide (there is exactly one current occurrence), and update the static-analysis/formatting docs. Async lifetime findings were reviewed and excluded as behaviorally broader. Once this increment passes full verification and adversarial review, the original four #15 outcomes are complete; full-tree normalization and a coverage floor remain explicitly deferred, so #15 can close rather than remain a perpetual automatic pickup.
timothy commented 2026-07-16 20:21:59 +02:00
Author
Owner
Copy Link
Copy Source

Scope correction after inspecting the evaluated build configuration: the earlier reconciliation missed an explicit original checklist gap. EnableThreadingAnalyzers still defaulted to false, and latest-Recommended was duplicated only in six production csproj files. This increment now also moves the analyzer posture to Directory.Build.props (EnableNETAnalyzers=true, AnalysisLevel=latest-All, EnableThreadingAnalyzers=true) for every project, removes the six narrower duplicates, and verifies the evaluated properties/build. Findings still default to suggestion; only the independently reviewed S3981 promotion blocks CI.

Scope correction after inspecting the evaluated build configuration: the earlier reconciliation missed an explicit original checklist gap. `EnableThreadingAnalyzers` still defaulted to `false`, and `latest-Recommended` was duplicated only in six production csproj files. This increment now also moves the analyzer posture to `Directory.Build.props` (`EnableNETAnalyzers=true`, `AnalysisLevel=latest-All`, `EnableThreadingAnalyzers=true`) for every project, removes the six narrower duplicates, and verifies the evaluated properties/build. Findings still default to `suggestion`; only the independently reviewed `S3981` promotion blocks CI.
timothy commented 2026-07-16 20:40:54 +02:00
Author
Owner
Copy Link
Copy Source

Analyzer-baseline validation found and resolved an SDK precedence trap before commit:

  • A direct latest-All build injected the SDK's exact per-ID severities, which outrank the bulk .editorconfig suggestion setting, and activated 455 errors in TWAE projects (1,651 analyzer warnings solution-wide).
  • The final design keeps AnalysisLevel=latest-All and checks in the .NET 10 All inventory as an explicit higher-priority suggestion baseline: 281 SDK IDs, with the SDK's future-rule CA1516=none preserved. Findings remain visible; none are suppressed.
  • The baseline does not skip package configs. Root .editorconfig per-ID promotions override it.
  • S3981 is also in the central WarningsAsErrors list, so the promotion blocks in test projects as well as TWAE app projects.

Evidence so far: evaluated properties are latest-All / SDK analyzers enabled / threading analyzers enabled on a test project; the full solution build succeeds with only 20 pre-existing compiler nullability warnings; and a negative-control build that temporarily restored Workers.Count >= 0 failed exactly on S3981 (0 warnings, 1 error). The source fix was immediately restored. Full tests and final adversarial review are still pending.

Analyzer-baseline validation found and resolved an SDK precedence trap before commit: - A direct `latest-All` build injected the SDK's exact per-ID severities, which outrank the bulk `.editorconfig` suggestion setting, and activated 455 errors in TWAE projects (1,651 analyzer warnings solution-wide). - The final design keeps `AnalysisLevel=latest-All` and checks in the .NET 10 `All` inventory as an explicit higher-priority suggestion baseline: 281 SDK IDs, with the SDK's future-rule `CA1516=none` preserved. Findings remain visible; none are suppressed. - The baseline does not skip package configs. Root `.editorconfig` per-ID promotions override it. - `S3981` is also in the central `WarningsAsErrors` list, so the promotion blocks in test projects as well as TWAE app projects. Evidence so far: evaluated properties are `latest-All` / SDK analyzers enabled / threading analyzers enabled on a test project; the full solution build succeeds with only 20 pre-existing compiler nullability warnings; and a negative-control build that temporarily restored `Workers.Count >= 0` failed exactly on `S3981` (0 warnings, 1 error). The source fix was immediately restored. Full tests and final adversarial review are still pending.
timothy removed the in-progress label 2026-07-16 22:19:31 +02:00
timothy commented 2026-07-16 22:19:50 +02:00
Author
Owner
Copy Link
Copy Source

Closed by PR #375 (merged to main as dbd4bb43, Fixes #15). Completing the close ritual that was interrupted when the Codex session dropped mid-finish.

Merge-gate state at merge (all derived, not asserted):

  • CI green on head 10ae885b — all 8 checks (Build & test 3,688 pass, Functional E2E, EF migration integrity SQLite+MySql, Formatting, API-docs-in-sync, decisions.md, docs-reminder; image-push skipped as normal for a PR).
  • Review-verdict: MERGEABLE @ 10ae885b — a fresh exact-head adversarial re-review (the fourth pass, after rebasing onto merged PR #374; the reviewer confirmed the base-to-head patch was byte-for-byte identical to the previously-reviewed diff and that #374's IPTV code compiles cleanly under the promoted analyzer posture).
  • All 9 ## Done-when boxes ticked.

What shipped (per PR #375): SDK analyzers centralized at latest-All + threading analyzers enabled repo-wide, a checked-in 281-rule SDK globalconfig baseline (exact match to .NET SDK 10.0.301) kept at suggestion severity so All-only rules stay visible without a wall of errors, Sonar S3981 promoted to a repo-wide CI blocker, and the real bug it caught — ReleaseMemoryHandler's always-true Workers.Count >= 0 predicate (which made the aggressive idle-GC branch unreachable) fixed with idle/active regression tests. Docker restore/publish layers now copy the analyzer baseline; the gitignored non-CPM .mcp tool is preserved via a ManagePackageVersionsCentrally guard (fixing an NU1015). Static-analysis + formatting docs updated.

Deferred (documented in the PR): full-tree formatting normalization (format-as-you-touch stays enforced), a coverage floor (until CI summaries establish a baseline), SDK-globalconfig regeneration on a future .NET SDK major, and the active-process/forced-override memory-handler branches (unchanged, low-severity coverage debt). No local Docker image build claimed (Docker unavailable on the workstation; the Dockerfile now supplies the exact previously-missing build input).

Note: this completes the analyzer-foundation work (#15 increment 1). Housekeeping: the orphaned Codex worktree at /private/tmp/ersatztv-15 (branch feat/15-analyzer-promotion, now merged + remote-deleted) can be pruned at leisure — left untouched since it's another session's checkout.

Closed by PR #375 (merged to `main` as `dbd4bb43`, `Fixes #15`). Completing the close ritual that was interrupted when the Codex session dropped mid-finish. **Merge-gate state at merge (all derived, not asserted):** - CI green on head `10ae885b` — all 8 checks (Build & test 3,688 pass, Functional E2E, EF migration integrity SQLite+MySql, Formatting, API-docs-in-sync, decisions.md, docs-reminder; image-push skipped as normal for a PR). - `Review-verdict: MERGEABLE @ 10ae885b` — a fresh exact-head adversarial re-review (the fourth pass, after rebasing onto merged PR #374; the reviewer confirmed the base-to-head patch was byte-for-byte identical to the previously-reviewed diff and that #374's IPTV code compiles cleanly under the promoted analyzer posture). - All 9 `## Done-when` boxes ticked. **What shipped (per PR #375):** SDK analyzers centralized at `latest-All` + threading analyzers enabled repo-wide, a checked-in 281-rule SDK globalconfig baseline (exact match to .NET SDK 10.0.301) kept at suggestion severity so All-only rules stay visible without a wall of errors, Sonar **`S3981` promoted to a repo-wide CI blocker**, and the real bug it caught — `ReleaseMemoryHandler`'s always-true `Workers.Count >= 0` predicate (which made the aggressive idle-GC branch unreachable) fixed with idle/active regression tests. Docker restore/publish layers now copy the analyzer baseline; the gitignored non-CPM `.mcp` tool is preserved via a `ManagePackageVersionsCentrally` guard (fixing an `NU1015`). Static-analysis + formatting docs updated. **Deferred (documented in the PR):** full-tree formatting normalization (format-as-you-touch stays enforced), a coverage floor (until CI summaries establish a baseline), SDK-globalconfig regeneration on a future .NET SDK major, and the active-process/forced-override memory-handler branches (unchanged, low-severity coverage debt). No local Docker image build claimed (Docker unavailable on the workstation; the Dockerfile now supplies the exact previously-missing build input). Note: this completes the analyzer-foundation work (#15 increment 1). Housekeeping: the orphaned Codex worktree at `/private/tmp/ersatztv-15` (branch `feat/15-analyzer-promotion`, now merged + remote-deleted) can be pruned at leisure — left untouched since it's another session's checkout.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels ci-cd enhancement priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#15