From fcd44de4e4cd935bf252686cc69aa991417714c4 Mon Sep 17 00:00:00 2001 From: Frank Tovar Date: Sat, 7 Feb 2026 00:24:30 +0100 Subject: [PATCH] Updated review --- REVIEW.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/REVIEW.md b/REVIEW.md index 730228a..f04a21d 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -65,7 +65,7 @@ Worst coupling points: - `Endpoints/EndpointHelpers.cs` (84 call sites): de facto god module. - `wwwroot/js/ui.js` + global `state` object (131 direct state references across JS modules). - Suggestion and phase workflows span endpoint functions, helper functions, filters, and duplicated client-side checks. -- Operational behavior is split across `API.md`, `scripts/smoke.ps1`, and runtime filters with contradictory contracts. +- Operational behavior is split across `API.md` and runtime filters with contradictory contracts. ## C) Critical task list @@ -78,10 +78,10 @@ Worst coupling points: - Dependencies (if any): none. [P0] 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: smoke checks and runbooks are misleading during incidents/releases. -- Evidence: `API.md:3`, `scripts/smoke.ps1:46`, `scripts/smoke.ps1:48`, `scripts/smoke.ps1:51`, `Infrastructure/AdminOnlyFilter.cs:12`. -- Recommendation: Make one contract authoritative (cookie-based admin role), update docs and scripts to follow it, and add one integration smoke test path that validates real auth flow. -- Acceptance criteria (testable): `scripts/smoke.ps1` passes against local app without manual edits; `API.md` no longer references `X-Admin-Key` unless implemented; one automated test verifies admin route rejection/acceptance behavior. +- 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. +- Acceptance criteria (testable): `API.md` no longer references `X-Admin-Key`; one automated test verifies admin route rejection/acceptance behavior. - Effort / Risk: `S / Low`. - Dependencies (if any): none. @@ -136,7 +136,7 @@ Worst coupling points: [P1] Remove legacy/dead paths to reduce cognitive load - Problem: Severity `Medium`, Category `Other`. Legacy `Reveal` phase and dead UI hooks remain in active code, increasing confusion. - Evidence: `Domain/Phase.cs:6`, `Endpoints/StateEndpoints.cs:107`, `wwwroot/js/data.js:30`, `wwwroot/js/ui.js:156`, `wwwroot/js/ui.js:1191`. -- Recommendation: Remove obsolete phase enum/value handling and dead UI references (`all-suggestions`, `nav-vote-next`) or reintroduce missing elements intentionally. +- Recommendation: Remove obsolete phase enum/value handling and dead UI references (`all-suggestions`, `nav-vote-next`). - Acceptance criteria (testable): no references to removed phase/UI ids remain; tests validate expected phase transitions only (`Suggest`, `Vote`, `Results`). - Effort / Risk: `S / Low`. - Dependencies (if any): P0 phase cleanup. @@ -177,7 +177,6 @@ Worst coupling points: Quick wins (hours to 1 day each): - Remove stale admin key wording in `API.md` and align with actual auth behavior. -- Fix `scripts/smoke.ps1` to authenticate via register/login and remove duplicate factory reset call. - 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.