61 lines
4.9 KiB
Markdown
61 lines
4.9 KiB
Markdown
# 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.**
|
||
|
||
## 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`**.
|