NEO-32: add pre-merge code review (docs/reviews/2026-04-30-NEO-32.md)

docs/reviews/2026-04-30-NEO-32.md

@@ -0,0 +1,47 @@
+# Code review — NEO-32 (CooldownSnapshot sync + slot presentation)
+
+**Date:** 2026-04-30
+
+**Scope:** Branch `NEO-32-cooldown-snapshot-sync-slot-presentation` vs merge-base with `origin/main` (commits through `56f7ae8`: cooldown snapshot API, cast `on_cooldown`, client HUD/sync, optional `NEON_SPRAWL_API_LOG`, HUD anchor fix for bad `serverTimeUtc`).
+
+**Base:** `origin/main` at merge-base `0790a3398d1b7138643e8e91f4b28db4fd7c554e`.
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+The branch delivers the planned NEO-32 slice: authoritative in-memory per-player slot cooldowns after an accepted cast, JSON deny `on_cooldown`, `GET /game/players/{id}/cooldown-snapshot` with v1 DTOs, `FakeTimeProvider` in the test host for deterministic time, Godot `CooldownSnapshotClient` + `CooldownState` with poll-while-cooling and a client-side cast guard aligned with the same reason string, plus Bruno and manual QA. Server tests pass (`dotnet test`). Overall risk is low for a prototype vertical slice; remaining notes are test-structure polish and a minor store hygiene detail.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| `docs/plans/NEO-32-implementation-plan.md` | **Matches** — dedicated GET, global duration, `on_cooldown`, in-memory store, client poll + guard, `FakeTimeProvider`, reconciliation section matches shipped filenames (`cooldown_state_test.gd` vs earlier table mentioning `cooldown_snapshot_client_test.gd`; reconciliation already reflects state tests). |
+| `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` | **Matches** — current state paragraph describes route, DTOs, client scripts, HUD, and plan link. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — NEO-32 note alongside prior E1.M4 slices. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E1.M4 row includes NEO-32 pointers. |
+| `docs/decomposition/modules/client_server_authority.md` | **Matches** — cooldown enforced on server; client guard is optional spam/UX layer, consistent with “server validates every intent.” |
+| `docs/decomposition/modules/E5_M1_CombatRulesEngine.md` | **N/A** for conflict — still notes prototype accept does not imply full combat resolution; NEO-32 does not claim engine-level cooldown catalog. |
+
+**Register / tracking:** No change required beyond what the branch already updates; E1.M4 correctly stays **In Progress**.
+
+## Blocking issues
+
+(none)
+
+## Suggestions
+
+1. **C# AAA layout on new integration tests** — `CooldownSnapshotApiTests.GetCooldownSnapshot_ShouldExposeSlotEnd_WhenCastAccepted_ThenClear_WhenAdvancedPastDuration` and the two new cooldown tests in `AbilityCastApiTests` bundle HTTP calls and `FakeClock.Advance` inside a single `// Act` block. Per [csharp-style](.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert), split so **Act** is only the invocations under assertion (or use clearly separated Act steps per phase) and keep clock advances in Arrange or interleaved with labeled sections. Low priority but keeps CI lint/review consistency for future edits.
+
+2. **Stale cooldown entries in the in-memory store** — `TryGetCooldownEndUtc` does not prune expired ends; `BuildSnapshot` omits them from JSON, so wire behavior is correct. Expired rows linger until `IsOnCooldown` runs (e.g. next cast to that slot). For a long-lived prototype process this is negligible; if you ever extend GET-heavy paths, consider pruning in `TryGetCooldownEndUtc` or snapshot build for symmetry with `IsOnCooldown`.
+
+## Nits
+
+- **Nit:** Untracked Godot `.uid` files for new scripts appeared in the working tree (`cooldown_snapshot_client.gd.uid`, etc.). Decide whether to commit them (team Godot 4 convention) or ignore via `.gitignore` so CI/agents do not see noise.
+
+## Verification
+
+- `cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj` — all tests passed (96) at review time.
+- Manual: follow `docs/manual-qa/NEO-32.md` (HUD countdown, server deny, client guard, rehydrate); optional `NEON_SPRAWL_API_LOG=1` / `http+apiLog` profile for stdout tracing.