From f6ef0a62272155a69855a3eee3938bd963280050 Mon Sep 17 00:00:00 2001
From: VinPropane <don@donmail.org>
Date: Sun, 28 Jun 2026 10:23:31 -0400
Subject: [PATCH] NEO-151: Add code review for contract issue and list HTTP API

---
 docs/reviews/2026-06-28-NEO-151.md | 60 ++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 docs/reviews/2026-06-28-NEO-151.md

diff --git a/docs/reviews/2026-06-28-NEO-151.md b/docs/reviews/2026-06-28-NEO-151.md
new file mode 100644
index 0000000..d87359f
--- /dev/null
+++ b/docs/reviews/2026-06-28-NEO-151.md
@@ -0,0 +1,60 @@
+# Code review — NEO-151 contract issue POST + per-player contract GET
+
+**Date:** 2026-06-28  
+**Scope:** Branch `NEO-151-e7m4-08-contract-issue-post-per-player-contract-get` — commits `9994aee`..`c0252ee` (2 commits)  
+**Base:** `main`  
+**Issue:** [NEO-151](https://linear.app/neon-sprawl/issue/NEO-151) — E7M4-08 contract issue POST + per-player contract GET
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+This branch lands client-readable contract HTTP: **`POST …/contracts/issue`** delegates to **`ContractGeneratorOperations.TryIssue`** with the quest-accept-style **`{ issued, reasonCode, contract }`** envelope; **`GET …/contracts`** lists the active instance (if any) plus up to 10 completed rows with **`encounterTemplateId`** and **`completionRewardSummary`** on completed rows via shared **`QuestProgressApi.MapCompletionRewardSummary`**.
+
+Store work adds **`IContractInstanceStore.ListForPlayer`** to both in-memory and Postgres implementations with active-first ordering and **`issuedAt` desc** cap semantics. Integration coverage is strong (10 HTTP tests + 5 store list tests); Bruno **`contracts/`** folder includes a full issue → encounter clear → GET capstone. Full suite passes (**927** tests). Risk is low for prototype scope; residual gaps are HTTP edge-case consistency and optional Postgres list persistence coverage.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| `docs/plans/NEO-151-implementation-plan.md` | **Matches** — AC checklist complete; shipped reconciliation matches diff; kickoff decisions (cap 10, issue envelope, reward summary reuse, store boundary) reflected in code |
+| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — NEO-151 HTTP anchor + README links added |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M4 row extended with E7M4-08 / NEO-151 |
+| `docs/decomposition/modules/module_dependency_register.md` | **Not re-read in diff** — no register change required; E7.M4 already tracked |
+| `server/README.md` | **Matches** — contract issue/list sections, **`ListForPlayer`** semantics, curl samples |
+| `docs/plans/E7M4-pre-production-backlog.md` | **Not re-read in diff** — plan cites E7M4-08; scope satisfied |
+| Client counterpart (NEO-153) | **N/A** — correctly out of scope; plan does not claim player-visible slice complete |
+
+Register/tracking table updates are included in this branch.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Align empty path `{id}` on POST with sibling player APIs** — `ContractIssueApi` returns **400** when the path id normalizes to empty (whitespace-only), while `ContractListApi`, `QuestAcceptApi`, and most other player routes return **404** for `trimmedId.Length == 0`. Recommend matching the **404** position-gate pattern for consistency.
+
+2. **Add POST 400 tests for missing seed fields** — Handler rejects empty **`templateId`** / **`seedBucket`** (lines 32–36 in `ContractIssueApi.cs`), but `ContractApiTests` only covers schema version and **`playerId`** mismatch. One test each (or a `[Theory]`) would lock the validation gate.
+
+3. **Optional Postgres `ListForPlayer` round-trip** — Plan marks this optional; if CI Postgres is stable, a `[RequirePostgresFact]` mirroring NEO-146 store tests would guard SQL ordering/cap behavior separately from in-memory parity.
+
+## Nits
+
+- Nit: **`PostgresContractInstanceStore.ListForPlayer`** opens a connection then calls **`TryGetActiveForPlayer`**, which opens a second connection. Fine at prototype volume; could inline the active SELECT on the existing connection later.
+- Nit: **`GetContracts_ShouldReturnCompletionRewardSummary_WhenContractCompletedAfterEncounterClear`** invokes **`TryCompleteOnEncounterClear`** under **`// Act`** before the GET — consider moving completion into **`// Arrange`** so Act is HTTP-only (AAA clarity).
+- Nit: Plan §1 Postgres snippet mentions **`completed_at DESC`** but kickoff + README + implementation use **`issuedAt` desc** — update the plan technical section on merge to remove the stale SQL note (implementation is correct).
+
+## Verification
+
+```bash
+dotnet test NeonSprawl.sln
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractApiTests"
+```
+
+Manual (dev server):
+
+- Run Bruno folder `bruno/neon-sprawl-server/contracts/` — especially **Get contracts after encounter clear** capstone.
+- Smoke **`POST …/contracts/issue`** deny path: second issue while active → **`issued: false`**, **`active_contract_exists`**.