Refresh review status and line references

REVIEW.md

@@ -4,16 +4,24 @@
 
 This codebase is functional and reasonably tested on backend behavior, but long-term change safety is currently limited by concentration risk (god modules), hidden side effects in read paths, and drifting operational contracts.
 
+Progress update (as of February 6, 2026):
+- Completed: phase reads are side-effect free with explicit reconciliation on write routes (`Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:62`).
+- Completed: admin auth docs aligned to account-based admin sessions (`API.md:3`).
+- Completed: build/test guardrails added (`.github/workflows/ci.yml`) and root ownership/setup docs added (`README.md:1`).
+- Completed: backend validators centralized for suggestions and auth (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`).
+- Completed: request safety hardened for redirects and forwarded headers (`Program.cs:40`, `Program.cs:104`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`).
+
 Top 5 maintainability risks (priority order):
 
-1. Hidden state mutation in phase reads (Critical)
+1. Frontend module concentration and global coupling (Critical)
-- `Endpoints/EndpointHelpers.cs:37` (`GetPhase`) reads and mutates player phase, then writes via `SaveChangesAsync` (`Endpoints/EndpointHelpers.cs:69`).
+- `wwwroot/js/ui.js` is still the dominant hotspot and owns rendering, validation, modal orchestration, admin flows, and vote logic.
-- This function is called from GET paths (`Endpoints/StateEndpoints.cs:19`, `Endpoints/StateEndpoints.cs:42`) and filters (`Infrastructure/PhaseRequirementFilter.cs:17`), creating non-obvious write behavior and duplicate checks.
+- Hidden module coupling through globals: `wwwroot/js/data.js:131`-`wwwroot/js/data.js:134`, plus `window` callbacks consumed in `wwwroot/js/ui.js:473`, `wwwroot/js/ui.js:696`, `wwwroot/js/ui.js:1009`.
-- Impact: hard-to-debug regressions, surprising side effects, and fragile refactors.
+- Impact: hard-to-debug regressions and fragile refactors in UI workflows.
 
-2. Operational contract drift between docs/scripts/code (High)
+2. Rule duplication still present between backend/frontend validations (High)
-- `API.md:3` claims admin auth via `X-Admin-Key`/`key`; admin runtime actually checks authenticated admin user (`Infrastructure/AdminOnlyFilter.cs:12`).
+- Suggestion validation is centralized on the backend (`Endpoints/SuggestEndpoints.cs:45`, `Endpoints/SuggestEndpoints.cs:133`, `Endpoints/SuggestionValidator.cs:7`) but frontend still duplicates parts (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`).
-- Impact: incident response and deployment validation are unreliable.
+- Auth validation is centralized on the backend (`Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`) while frontend length checks remain duplicated (`wwwroot/app.js:92`, `wwwroot/app.js:121`).
+- Impact: inconsistent behavior and repeated fixes across server/client.
 
 3. High-change, high-complexity frontend hotspots (High)
 - Git churn: `wwwroot/app.js` (76 changes), `wwwroot/js/ui.js` (55), `wwwroot/js/i18n.js` (50).
@@ -21,15 +29,13 @@ Top 5 maintainability risks (priority order):
 - Hidden module coupling through globals: `wwwroot/js/data.js:131`-`wwwroot/js/data.js:134`, plus `window` callbacks consumed in `wwwroot/js/ui.js:473`, `wwwroot/js/ui.js:696`, `wwwroot/js/ui.js:1009`.
 - Impact: every UI change risks regressions outside its feature area.
 
-4. Rule duplication and divergence across backend/frontend (High)
+4. Service-layer extraction is still pending in large endpoint files (High)
-- Suggestion validation duplicated in create and update (`Endpoints/SuggestEndpoints.cs:45`, `Endpoints/SuggestEndpoints.cs:152`) plus separate JS copy (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`).
+- Endpoint lambdas still own orchestration and persistence logic in `Endpoints/SuggestEndpoints.cs`, `Endpoints/AdminEndpoints.cs`, `Endpoints/VoteEndpoints.cs`, and `Endpoints/ResultsEndpoints.cs`.
-- Auth length constraints duplicated backend (`Endpoints/AuthEndpoints.cs:19`, `Endpoints/AuthEndpoints.cs:25`) and frontend (`wwwroot/app.js:92`, `wwwroot/app.js:121`).
+- Impact: high cognitive load and slower, riskier feature changes.
-- Impact: inconsistent behavior, repeated bug fixes, and shotgun surgery.
 
-5. Tooling guardrails are weak and build hygiene is unstable (Medium)
+5. Static-analysis and frontend lint guardrails remain incomplete (Medium)
-- `dotnet build GameList.sln` currently reports MSB3026 copy warnings involving recursive test output paths under `GameList.Tests`.
+- Build/test CI exists (`.github/workflows/ci.yml`) and project content rules are fixed (`GameList.csproj:17`-`GameList.csproj:21`), but analyzers/lint/format gates are still absent.
-- No CI workflow found (`.github/workflows` absent), no root `README.md`, and no analyzer config files detected (`.editorconfig`, `Directory.Build.props`, `global.json` absent).
+- Impact: regressions and style drift can still slip through.
-- Impact: regressions and process drift are caught late and inconsistently.
 
 Where to start (recommended sequence):
 - Start with P0 tasks that reduce surprise and operational risk: phase-read side effects, auth contract drift, build hygiene, and validation centralization.
@@ -69,15 +75,15 @@ Worst coupling points:
 
 ## C) Critical task list
 
-[P0] Make phase reads side-effect free and move reconciliation to explicit writes
+[P0][Done] Make phase reads side-effect free and move reconciliation to explicit writes
-- Problem: Severity `Critical`, Category `Architecture`. `GetPhase` mutates and persists state during read operations, which causes hidden writes and duplicated logic in endpoints/filters. Impact: unsafe refactors and non-deterministic behavior.
+- Problem: Severity `Critical`, Category `Architecture`. Read endpoints/filters previously relied on mutating phase reads. Impact: unsafe refactors and non-deterministic behavior.
-- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:69`, `Endpoints/StateEndpoints.cs:19`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsEndpoints.cs:26`.
+- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:20`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsEndpoints.cs:26`, `GameList.Tests/StateTests.cs:236`, `GameList.Tests/FiltersTests.cs:55`.
 - Recommendation: Split into `GetCurrentPhaseAsync` (pure read) and explicit `ReconcilePhaseAsync` (write command). Run reconciliation only on intentional transition points (admin toggle, phase change commands, migration job), not on GET paths.
 - Acceptance criteria (testable): GET `/api/state` and GET `/api/me` never call `SaveChangesAsync`; integration tests verify no phase mutations occur during read-only requests; filters perform one phase check path without side effects.
 - Effort / Risk: `M / Med`.
 - Dependencies (if any): none.
 
-[P0] Repair admin auth contract drift across code, docs, and smoke automation
+[P0][Done] Repair admin auth contract drift across code, docs, and smoke automation
 - Problem: Severity `High`, Category `Documentation/Tooling`. Documentation and smoke script specify header-based admin auth, runtime requires authenticated admin cookie. Impact: runbooks are misleading during incidents/releases.
 - Evidence: `API.md:3`, `Infrastructure/AdminOnlyFilter.cs:12`.
 - Recommendation: Make one contract authoritative (account-based admin role), update docs to follow it, and add one integration smoke test path that validates real auth flow.
@@ -85,25 +91,25 @@ Worst coupling points:
 - Effort / Risk: `S / Low`.
 - Dependencies (if any): none.
 
-[P0] Eliminate build instability and warning noise from project layout/content rules
+[P0][Done] Eliminate build instability and warning noise from project layout/content rules
 - Problem: Severity `High`, Category `Tooling`. Current build emits MSB3026 warnings and recursive copy paths under test output, reducing trust in build outputs and masking real issues.
-- Evidence: `dotnet build GameList.sln` warning output referencing `GameList.Tests\bin\Debug\net10.0\GameList.Tests\bin\Debug\...`; current exclusions only in `GameList.csproj:17`-`GameList.csproj:21`.
+- Evidence: content exclusions now include `Content` in `GameList.csproj:17`-`GameList.csproj:21`; CI gate added in `.github/workflows/ci.yml`.
 - Recommendation: Ensure test assets are fully excluded from web content pipeline (or move tests outside web project root), then enforce clean build (`0 warnings`) in CI.
 - Acceptance criteria (testable): `dotnet build GameList.sln` emits zero warnings; publish output contains only expected runtime artifacts; CI fails on warning regressions.
 - Effort / Risk: `M / Med`.
 - Dependencies (if any): none.
 
-[P0] Centralize validation rules to stop backend/frontend drift
+[P0][Partial] Centralize validation rules to stop backend/frontend drift
 - Problem: Severity `High`, Category `Complexity/Duplication`. Validation rules are duplicated in multiple backend endpoints and frontend forms. Impact: inconsistent behavior and repeated fixes.
-- Evidence: `Endpoints/SuggestEndpoints.cs:45`, `Endpoints/SuggestEndpoints.cs:152`, `Endpoints/SuggestEndpoints.cs:298`, `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `Endpoints/AuthEndpoints.cs:19`, `wwwroot/app.js:92`.
+- Evidence: backend centralized in `Endpoints/SuggestEndpoints.cs:45`, `Endpoints/SuggestEndpoints.cs:133`, `Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`; frontend duplicates remain in `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `wwwroot/app.js:92`.
 - Recommendation: Extract backend validators (e.g., `SuggestionValidator`, `AuthValidator`) and reuse in create/update paths; simplify frontend to UX-only prechecks and rely on server responses for source-of-truth.
 - Acceptance criteria (testable): create/update share one backend validator path; tests cover validator once and both endpoints; frontend no longer re-implements server-only security rules.
 - Effort / Risk: `M / Med`.
 - Dependencies (if any): none.
 
-[P0] Harden request safety defaults (forwarded headers and redirect handling)
+[P0][Done] Harden request safety defaults (forwarded headers and redirect handling)
 - Problem: Severity `High`, Category `Security`. Forwarded headers are trusted without explicit proxy/network allowlist, and image validation likely follows redirects despite "no redirects" policy.
-- Evidence: `Program.cs:69`, `Endpoints/EndpointHelpers.cs:108`, `Endpoints/EndpointHelpers.cs:113`, `Endpoints/EndpointHelpers.cs:133`, `Endpoints/SuggestEndpoints.cs:57`.
+- Evidence: `Program.cs:40`, `Program.cs:70`, `Program.cs:104`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`, `IIS.md:17`.
 - Recommendation: Configure known proxies/networks for forwarded headers; enforce `AllowAutoRedirect = false` in image validation client and add tests for redirect-chain and private-host edge cases.
 - Acceptance criteria (testable): integration tests prove redirected URLs are rejected; forwarded header spoofing test fails when source is untrusted; documentation updated with trusted proxy requirements.
 - Effort / Risk: `M / Med`.
@@ -151,7 +157,7 @@ Worst coupling points:
 
 [P1] Strengthen test quality for flaky/time-sensitive cases and security edges
 - Problem: Severity `Medium`, Category `Testing`. Some tests depend on sleeps and do not cover realistic redirect behavior or overlapping refresh flows.
-- Evidence: `GameList.Tests/SuggestionTests.cs:379`, `GameList.Tests/SuggestionTests.cs:575`, `GameList.Tests/HelperTests.cs:95`.
+- Evidence: `GameList.Tests/SuggestionTests.cs:379`, `GameList.Tests/SuggestionTests.cs:575`, `GameList.Tests/HelperTests.cs:121`.
 - Recommendation: replace `Task.Delay` ordering checks with deterministic seeded timestamps where feasible; add explicit redirect-follow tests and concurrency-path tests.
 - Acceptance criteria (testable): no timing sleeps in endpoint tests for ordering; new tests cover redirect-chain rejection and race-sensitive refresh logic.
 - Effort / Risk: `M / Low`.
@@ -179,11 +185,11 @@ Quick wins (hours to 1 day each):
 - Remove stale admin key wording in `API.md` and align with actual auth behavior.
 - Add `Content Remove="GameList.Tests\**\*"` (or equivalent) and verify clean build output.
 - Add a build check script that fails on warnings and run it locally/CI.
-- Delete dead `DeletePlayerRequest` in `Contracts/Dtos.cs:23` if unused.
+- Completed: removed dead `DeletePlayerRequest` from `Contracts/Dtos.cs`.
-- Remove unused endpoint handler parameters (`IConfiguration _`) in `Endpoints/StateEndpoints.cs:55`, `Endpoints/StateEndpoints.cs:79`.
+- Completed: removed unused endpoint handler parameters in `Endpoints/StateEndpoints.cs`.
 - Remove dead UI references (`all-suggestions`, `nav-vote-next`) or add explicit TODO with owner/date.
 - Replace `Task.Delay` test ordering hacks with deterministic setup in affected tests.
-- Add a short "module ownership" section to `README.md` once created.
+- Completed: added module ownership section in `README.md`.
 
 Strategic refactors (multi-day/week), staged: