diff --git a/docs/reviews/2026-06-24-NEO-146-r2.md b/docs/reviews/2026-06-24-NEO-146-r2.md new file mode 100644 index 0000000..7afb6c1 --- /dev/null +++ b/docs/reviews/2026-06-24-NEO-146-r2.md @@ -0,0 +1,73 @@ +# Code review (re-review) — NEO-146 (E7M4-03 contract instance + outcome stores) + +**Date:** 2026-06-24 +**Scope:** Branch `NEO-146-e7m4-03-contract-instance-outcome-stores` — commits `82c4594`…`01e82f3` (10 commits); supersedes [2026-06-24-NEO-146.md](2026-06-24-NEO-146.md) initial review +**Base:** `main` + +## Verdict + +**Approve with nits** + +## Summary + +Follow-up review after the initial NEO-146 review and a broad C# style/analyzer cleanup pass. All prior **Suggestions** and actionable **Nits** from the first review are addressed: decomposition alignment updated, Postgres one-active race + deny-path tests added, duplicate instance id unit test, FK-safe `PostgresContractOutcomeStore.TryAppend`, `MakeInstanceKey` / player-level locking wired in the in-memory instance store, README orchestrator ordering documented, and plan reconciliation brought current. Subsequent commits apply collection spread, `await using` / `Lock` schema gates, and related analyzer conventions repo-wide (documented in `.cursor/rules/csharp-style.md`). Contract store behavior matches the plan; `dotnet test` passes **867** tests with **0** build warnings. Risk remains low — infrastructure-only, no HTTP surface. + +## Documentation checked + +| Document | Alignment | +|----------|-----------| +| `docs/plans/NEO-146-implementation-plan.md` | **Matches** — AC complete; reconciliation includes Bruno smoke, `MakeInstanceKey` locking, Postgres clear adopted, review follow-up tests. Test count says `866`; suite now **867** (minor drift). | +| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-03) | **Matches** | +| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — runtime stores paragraph (NEO-146). | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7M4-03 / NEO-146 stores row added. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7M4-03 / NEO-146 note added. | +| `.cursor/rules/csharp-style.md` | **Matches** — new sections document collection spread, `using`/`await using`, `Lock` schema gates, CA1859, null-check style adopted in cleanup commits. | +| Full-stack epic decomposition | **N/A** — infrastructure-only. | + +## Prior review resolution + +| Item (from `2026-06-24-NEO-146.md`) | Status | +|-------------------------------------|--------| +| Decomposition tracking (alignment + register) | **Done** | +| Postgres distinct-instance-id one-active race test | **Done** — `TryCreateActive_ShouldAllowOnlyOneActive_WhenDistinctInstanceIdsRace` | +| Postgres outcome deny paths + idempotent complete | **Done** | +| In-memory duplicate instance id test | **Done** | +| Postgres FK parity on outcome append | **Done** — catch + README | +| `MakeInstanceKey` wiring / plan trim | **Done** | +| Bruno + plan reconciliation | **Done** | +| Postgres `TryClear*` plan status | **Done** — adopted | + +## Blocking issues + +None. + +## Suggestions + +1. **PR / commit scope for future stories** — Commits `b455ff5`…`01e82f3` touch ~50 files outside `Game/Contracts/` (catalog loaders, integration-test `using` layout, bootstrap `Lock` gates, mutation-outcome docs). Mechanical and green, but it widens review/merge-conflict surface for an infrastructure ticket. Consider a separate `chore:` PR for repo-wide analyzer/style passes when the diff is this large. + +2. **In-memory outcome instance guard (optional parity)** — Postgres `TryAppend` returns `false` when `contract_instance` is missing (FK); in-memory still accepts a normalized row without checking instance existence. README documents Postgres ordering; orchestrators (NEO-149) will create the instance first. If strict cross-backend parity matters before NEO-149, add an optional `IContractInstanceStore` lookup in `InMemoryContractOutcomeStore.TryAppend` or a shared guard helper — otherwise document the asymmetry explicitly in README (one line under in-memory storage). + +## Nits + +- Nit: Plan reconciliation cites **866** tests; re-review run reports **867** — update the count on next doc touch. + +- Nit: `ContractInstanceReasonCodes` remains unused until NEO-147 orchestrators — expected deferral; no action in E7M4-03. + +- Nit: `InMemoryContractInstanceStore` uses `PlayerLockKey` for create/active reads and `MakeInstanceKey` for get/complete/clear on a specific instance. One-active policy holds under analysis (create path serializes on player lock; active index checked before insert), but the split lock granularity is slightly harder to reason about than a single player-level mutex — acceptable for prototype; no change required unless concurrency tests expand. + +## Verification + +Commands run during re-review (all green): + +```bash +dotnet build NeonSprawl.sln +dotnet test NeonSprawl.sln +``` + +Optional Postgres integration (when `ConnectionStrings__NeonSprawl` is set): + +```bash +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractInstancePersistence|FullyQualifiedName~ContractOutcomePersistence" +``` + +Merge-ready: prior review feedback is closed; remaining items are process/parity nits only.