74 lines
5.0 KiB
Markdown
74 lines
5.0 KiB
Markdown
# 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.
|