diff --git a/docs/reviews/2026-06-27-NEO-147.md b/docs/reviews/2026-06-27-NEO-147.md new file mode 100644 index 0000000..36644a2 --- /dev/null +++ b/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).