# 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.~~ **Done.** — position gate runs first; whitespace-only path returns **404**.

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.~~ **Done.** — `[Theory]` for empty/whitespace **`templateId`** and **`seedBucket`**; whitespace path **404** test added.

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.~~ **Done.** — `ListForPlayer_ShouldPrependActiveAndCapCompletedRows_OnPostgres` in `ContractInstancePersistenceIntegrationTests`.

## 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.~~ **Done.** — active SELECT inlined on the list connection.
- ~~Nit: **`GetContracts_ShouldReturnCompletionRewardSummary_WhenContractCompletedAfterEncounterClear`** invokes **`TryCompleteOnEncounterClear`** under **`// Act`** before the GET — consider moving completion into **`// Arrange`** so Act is HTTP-only (AAA clarity).~~ **Done.**
- ~~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).~~ **Done.**

## Bugbot (PR #193)

| Severity | Location | Finding | Status |
|----------|----------|---------|--------|
| Low | `scripts/reset-contract-instances-helper.js` | Reset helper only cleared one capped GET page (active + 10 completed); older rows could survive pre-request cleanup | **Done.** — loop GET → reset until list empty (max 100 passes) |
| Low | `contracts/scripts/` helper path | Bruno resolves `./scripts/` from collection root, not per-folder — CI module not found | **Done.** — moved helper to `neon-sprawl-server/scripts/`; `.js` suffix on requires |
| Medium | `PostgresContractInstanceStore.ListForPlayer` | Active + completed read in two queries without snapshot — duplicate row if status flips mid-read | **Done.** — single CTE query for atomic list snapshot |

## 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`**.