NEO-147: add code review for contract generator TryIssue

docs/reviews/2026-06-27-NEO-147.md

@@ -0,0 +1,61 @@
+# Code review — NEO-147 (E7M4-04 contract generator TryIssue)
+
+**Date:** 2026-06-27  
+**Scope:** Branch `NEO-147-e7m4-04-contract-generator-try-issue` — commits `7f62f43`…`c37c3c7` (5 commits)  
+**Base:** `main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-147 adds `ContractGeneratorOperations.TryIssue`, a server-authoritative orchestrator that selects a contract template (explicit id or auto-select by band + faction standing), mints a deterministic instance id (`ci_{32 hex}`), and persists via `IContractInstanceStore.TryCreateActive`. Reason codes align with NEO-146 store codes where shared; generator-specific codes cover unknown template and no eligible template. The flow mirrors `QuestStateOperations` / NEO-117 patterns: early player/active gates, delegated faction evaluation, structured deny envelopes with optional snapshots, and race-safe create-failure handling. Eleven AAA unit tests, one DI integration test, and one optional Postgres persistence test cover happy paths and primary deny gates; `dotnet test` passes **888** tests. Risk is low — orchestrator-only, no HTTP or client surface; downstream NEO-149/NEO-151 consume this layer.
+
+## Documentation checked
+
+| Document | Alignment |
+|----------|-----------|
+| `docs/plans/NEO-147-implementation-plan.md` | **Matches** — AC checklist complete; reconciliation reflects shipped types, orchestrator, tests (11 unit + DI + Postgres), README, module note. Minor drift: plan “Files to add” omits `ContractGeneratorPersistenceIntegrationTests.cs` (listed under Tests section only). |
+| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-04) | **Matches** — template selection, active cap, deterministic ids, structured denies; economy lint, HTTP, completion wiring correctly out of scope. |
+| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — NEO-147 generator operations paragraph + README link added. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M4 row stops at E7M4-03/NEO-146; should add E7M4-04/NEO-147 generator ops after merge (same follow-up pattern as NEO-146). |
+| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E7.M4 note ends at NEO-146; optional one-line NEO-147 after merge. |
+| `docs/decomposition/modules/client_server_authority.md` | **N/A** — server orchestrator; authority unchanged. |
+| Full-stack epic decomposition | **N/A** — server-only; client counterpart NEO-153 correctly deferred until NEO-151 HTTP. |
+
+Register/tracking tables should be updated for E7M4-04 after merge — **should fix** (non-blocking), mirroring NEO-146 alignment follow-up.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Update decomposition tracking for E7M4-04** — After merge, extend `documentation_and_implementation_alignment.md` E7.M4 row (and optionally `module_dependency_register.md`) with E7M4-04: `ContractGeneratorOperations.TryIssue`, reason codes, deterministic ids, README section — same pattern as NEO-145/NEO-146.
+
+2. **Faction standing deny path** — Plan and README document faction floor via `FactionGateOperations`; unit tests cover band mismatch but not standing failure → `no_eligible_template`. Prototype template min standing is **0** (always passes with default standing), so add a unit test with a synthetic template row (`minStanding: 1`) and zero player standing to lock the orchestrator wiring (distinct from `FactionGateOperationsTests`).
+
+3. **Duplicate instance id after complete** — Plan adopted: re-issue with the same `seedBucket` after completion hits store duplicate-id deny; orchestrator maps to `invalid_ids` via `DenyCreateFailure`. Add a unit test (complete first issue, re-issue same seed) to document caller contract (“rotate `seedBucket` for a new contract”).
+
+## Nits
+
+- Nit: `MakeDeterministicInstanceId_ShouldMatchForSameInputs_AndDifferForDifferentSeedBucket` has an empty `// Act` block — move equality assertions into Act or fold into Arrange per AAA convention.
+- Nit: `ContractGeneratorPersistenceIntegrationTests` mixes second-factory read into Act; acceptable but slightly blurs Act vs Assert compared to other integration tests.
+- Nit: `DenyCreateFailure` fallback `invalid_ids` for duplicate completed instance id is correct per plan but coarse for future HTTP mappers — consider a dedicated reason code if NEO-151 needs clearer client messaging (defer unless HTTP design requires it).
+
+## Verification
+
+```bash
+cd /Users/don/neon-sprawl && dotnet test NeonSprawl.sln
+cd /Users/don/neon-sprawl && dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractGenerator"
+```
+
+Optional (Postgres configured):
+
+```bash
+export ConnectionStrings__NeonSprawl='…'
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractGeneratorPersistence"
+```
+
+Manual Godot QA: **none** (server orchestrator per plan).