NEO-146: add code review for contract instance and outcome stores

docs/reviews/2026-06-24-NEO-146.md

@@ -0,0 +1,70 @@
+# Code 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`…`ae641a4` (2 commits)  
+**Base:** `main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-146 adds durable per-player contract instance state (`IContractInstanceStore`) and append-only completion audit rows (`IContractOutcomeStore`) with in-memory fallback and Postgres persistence (`V011`, `V012`) when `ConnectionStrings:NeonSprawl` is set. The implementation mirrors NEO-116/NEO-135 patterns: dev-player write gate, one-active enforcement (app logic + partial unique index), idempotent complete replay, outcome idempotency keys, grant snapshot reuse from `Game/Rewards/`, DI via `AddContractInstanceStores`, and test factory overrides. Unit tests cover happy/deny/idempotent paths with full AAA; Postgres integration tests include cross-factory persistence and a concurrent create race. `dotnet test` passes 862 tests. Risk is low — infrastructure-only, no HTTP or player-facing surface; primary consumers are NEO-147/NEO-149 orchestrators.
+
+## Documentation checked
+
+| Document | Alignment |
+|----------|-----------|
+| `docs/plans/NEO-146-implementation-plan.md` | **Matches** — AC checklist complete; reconciliation reflects shipped types, stores, migrations, DI, tests, README. Minor drift: plan lists `MakeActivePlayerKey` (not implemented) and defers Postgres `TryClear*` (implemented anyway — acceptable improvement). |
+| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-03) | **Matches** — dual stores, one-active policy, idempotency key shape, grant snapshots, NEO-116-style Postgres split; generator/payout/HTTP correctly out of scope. |
+| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — runtime stores paragraph + README links added (NEO-146). |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M4 row cites E7M4-01/NEO-144 and E7M4-02/NEO-145 only; should add E7M4-03/NEO-146 stores after merge (same pattern as NEO-145 follow-up). |
+| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E7.M4 In Progress spine unchanged; optional one-line NEO-146 note after merge. |
+| `docs/decomposition/modules/client_server_authority.md` | **N/A** — server-side persistence only; no authority boundary change. |
+| Full-stack epic decomposition | **N/A** — infrastructure-only; client counterpart correctly none (NEO-154 capstone). |
+
+Register/tracking tables should be updated for E7M4-03 after merge — **should fix** (non-blocking), mirroring NEO-145 alignment follow-up.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Update decomposition tracking for E7M4-03** — After merge, extend `documentation_and_implementation_alignment.md` E7.M4 row (and optionally `module_dependency_register.md`) with E7M4-03: `IContractInstanceStore` + `IContractOutcomeStore`, `V011`/`V012`, README sections — same pattern as NEO-145.
+
+2. **Postgres one-active concurrent race** — The existing `TryCreateActive_ShouldReturnOneTrueAndRestFalse_WhenCalledConcurrently` test races on the **same** instance id (PK dedup), matching the quest-store precedent. For contracts, the more important race is **different** instance ids for the same player competing for the one-active partial unique index. Consider a second `RequirePostgresFact` that launches concurrent `TryCreateActive` with distinct instance ids and asserts exactly one `active` row remains.
+
+3. **Postgres outcome deny paths** — In-memory tests cover duplicate row id / idempotency key; Postgres integration only covers happy-path append. Add `RequirePostgresFact` duplicate-append deny (and optionally idempotent complete replay on Postgres) to lock SQL `INSERT … WHERE NOT EXISTS` / complete `FOR UPDATE` behavior.
+
+4. **In-memory duplicate instance id** — Plan §7 lists duplicate instance id deny; no explicit unit test calls `TryCreateActive` twice with the same instance id (second call should return `false` with unchanged snapshot). Cheap coverage gap.
+
+5. **Postgres outcome FK parity** — `PostgresContractOutcomeStore.TryAppend` will throw on FK violation (missing `contract_instance` row) while in-memory accepts any normalized row. Orchestrators (NEO-149) will create the instance first, so this is low risk today; document in README or catch `PostgresException` and return `false` if you want strict bool-only store semantics across backends.
+
+## Nits
+
+- Nit: `ContractInstanceIds.MakeInstanceKey` is defined but unused; plan also lists `MakeActivePlayerKey` which was not added. Either wire keys into store indexing or trim plan/reconciliation to match shipped API.
+
+- Nit: `ContractInstanceReasonCodes` is defined for E7M4-04+ orchestrators but unused in this slice — correct deferral; no action until NEO-147.
+
+- Nit: Bruno folder `bruno/neon-sprawl-server/contract-stores/` (health smoke) is useful but not listed in plan §Implementation reconciliation — optional one-line note for doc parity (same as NEO-145 Bruno nit).
+
+- Nit: Plan open questions marked Postgres `TryClear*` as deferred; both Postgres stores implement clear helpers anyway — update plan risk row to `adopted` on next doc pass.
+
+## Verification
+
+Commands run during review (all green):
+
+```bash
+dotnet test NeonSprawl.sln
+```
+
+Author should run Postgres integration locally when `ConnectionStrings__NeonSprawl` is available:
+
+```bash
+# Requires local Postgres + connection string
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractInstancePersistence|FullyQualifiedName~ContractOutcomePersistence"
+```
+
+Before merge: skim decomposition alignment suggestion — functional code is merge-ready without it.