From e3d8458a66d7943bacda7456bc33b6963412bce8 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 28 Jun 2026 09:56:37 -0400 Subject: [PATCH] NEO-150: Add code review for economy validation at issue --- docs/reviews/2026-06-28-NEO-150.md | 64 ++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 docs/reviews/2026-06-28-NEO-150.md diff --git a/docs/reviews/2026-06-28-NEO-150.md b/docs/reviews/2026-06-28-NEO-150.md new file mode 100644 index 0000000..15d7baa --- /dev/null +++ b/docs/reviews/2026-06-28-NEO-150.md @@ -0,0 +1,64 @@ +# Code review — NEO-150 economy validation at issue time + +**Date:** 2026-06-28 +**Scope:** Branch `NEO-150-e7m4-07-economy-validation-lint-at-issue-time` — commits `039542a`..`d5de4e8` (3 commits) +**Base:** `origin/main` +**Issue:** [NEO-150](https://linear.app/neon-sprawl/issue/NEO-150) — E7M4-07 economy validation lint at issue time + +## Verdict + +**Approve with nits** + +## Summary + +This branch adds defense-in-depth economy validation at contract issue time: `ContractEconomyValidation.TryValidateTemplate` delegates to existing `PrototypeE7M4ContractCatalogRules` band-cap and cross-ref helpers, wired into `ContractGeneratorOperations.TryIssue` after template selection and before persist. Two new reason codes (`economy_cap_exceeded`, `invalid_reward_bundle`) surface stable deny semantics for downstream HTTP (NEO-151) and client copy (NEO-153). + +The implementation matches the adopted kickoff plan: registry parameters on `TryIssue`, split reason codes, full loader-parity cross-ref scope, and no HTTP/client scope creep. All `TryIssue` call sites are updated; unit and integration tests cover happy path, over-cap deny, and unknown bundle ref deny. Full suite passes (911 tests). Risk is low — logic reuses proven CI/startup gates; main residual risk is per-issue registry snapshot allocation cost, acceptable at prototype scale. + +## Documentation checked + +| Document | Result | +|----------|--------| +| `docs/plans/NEO-150-implementation-plan.md` | **Matches** — AC checklist marked complete; shipped reconciliation aligns with diff | +| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — NEO-150 economy validation note + E6.M4/E3.M5 parity stub added | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M4 row updated with E7M4-07 / NEO-150 | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M4 note references NEO-150 | +| `server/README.md` | **Matches** — issue-time economy validation paragraph + reason-code table extended | +| `docs/plans/E7M4-pre-production-backlog.md` | **Not re-read in diff** — plan cites it; backlog intent satisfied by slice scope | + +Register/tracking table updates are already included in this branch. + +## Blocking issues + +None. + +## Suggestions + +1. **Deduplicate prototype registry test fixtures** — `ContractEconomyValidationTests.CreatePrototypeRegistries()` and `ContractGeneratorOperationsTests.CreatePrototypeCatalogRegistries()` are nearly identical (~50 lines). Consider a shared internal test helper (e.g. `ContractPrototypeTestFixtures`) to avoid drift when prototype catalog ids change. + +2. **Broaden deny-path test matrix (optional)** — Current tests exercise item qty over cap and unknown item id only. Band-cap and cross-ref helpers also cover skill XP, reputation grants, unknown encounter, and disallowed `mission_reward` skill sources (covered at loader level in `ContractTemplateCatalogLoaderTests`). One additional `[Theory]` or a skill-Xp-over-cap case in `ContractEconomyValidationTests` would document parity without much cost. + +3. **NEO-151 call-site checklist** — When HTTP lands, confirm the production DI path passes the same four registries already wired in integration tests. No code change needed here; worth a one-line note in NEO-151 plan. + +## Nits + +- Nit: `ContractEconomyValidation.TryValidateTemplate` does not null-check `template`; safe today because `TrySelectTemplate` only returns success with a non-null template, but `ArgumentNullException.ThrowIfNull(template)` would mirror the registry guards. +- Nit: Issue-time validation order is band-cap → cross-ref; startup loader runs cross-ref → band-cap. Intentional per plan (cap failures get `economy_cap_exceeded`), but worth keeping in mind if both gates ever fail on the same row during debugging. +- Nit: Per-issue allocation of encounter/faction hash sets and skill dictionary is fine for prototype; revisit only if issue volume becomes hot-path. + +## Verification + +Commands run during review: + +```bash +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractEconomyValidation|FullyQualifiedName~ContractGeneratorOperations" +dotnet test NeonSprawl.sln +``` + +Results: 20 filtered tests passed; **911/911** full suite passed. + +Author pre-merge checklist: + +- [x] `dotnet test NeonSprawl.sln` green +- [ ] Merge to `main` and proceed to NEO-151 (HTTP) with extended `TryIssue` signature +- [ ] No Godot manual QA required (server policy per plan)