From 7ddfb53219b264656af96d965d19de559a293cf4 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Thu, 30 Apr 2026 21:52:14 -0400 Subject: [PATCH] NEO-32: add pre-merge code review (docs/reviews/2026-04-30-NEO-32.md) --- docs/reviews/2026-04-30-NEO-32.md | 47 +++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 docs/reviews/2026-04-30-NEO-32.md diff --git a/docs/reviews/2026-04-30-NEO-32.md b/docs/reviews/2026-04-30-NEO-32.md new file mode 100644 index 0000000..5e82ffc --- /dev/null +++ b/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.