# 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 **867**. |
| `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**~~ — **Intentional.** Repo-wide analyzer/style cleanup (`b455ff5`…`01e82f3`) was bundled on this branch by author request; no split `chore:` PR for E7M4-03.

2. ~~**In-memory outcome instance guard (optional parity)**~~ — **Done.** `InMemoryContractOutcomeStore` injects **`IContractInstanceStore`** and denies **`TryAppend`** when **`TryGet`** is false; unit test + README/plan updated (Postgres FK parity).

## Nits

- ~~Nit: Plan reconciliation cites **866** tests; re-review run reports **867**~~ — **Done.** Plan reconciliation updated to **867**.

- 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: all suggestions addressed or intentionally accepted; remaining nits are expected deferrals only.