diff --git a/docs/reviews/2026-05-24-NEO-63.md b/docs/reviews/2026-05-24-NEO-63.md new file mode 100644 index 0000000..1e4ae7e --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-63.md @@ -0,0 +1,68 @@ +# Code review — NEO-63 interact path integration + NEO-62 gather engine + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-63-e3m1-interact-path-integration-registry-expansion-bruno` · commits `acf9fe1`–`01dc48a` vs `origin/main` (includes stacked **NEO-62** gather engine commits) +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +This branch completes Epic 3 Slice 2 gather wiring: **NEO-62** adds **`GatherOperations.TryGather`** and **`GatherResult`** (inventory → XP → depletion commit, compensating rollback on XP failure); **NEO-63** replaces the NEO-41 silent inline XP path in **`InteractionApi`** with gather-engine delegation, expands **`PrototypeInteractableRegistry`** to four resource-node anchors, and adds HTTP integration tests, Bruno requests, and manual QA. Denies now surface as **`allowed: false`** with stable **`reasonCode`** passthrough (`node_depleted`, `inventory_full`, skill deny codes, etc.). Risk is moderate but well-contained: the orchestration touches inventory, progression, and depletion stores with ordered commits and rollback, but coverage is strong (6 engine tests, 4 new HTTP integration tests, registry/world API updates, Bruno depletion loop). Recommend merging **NEO-62** first or accepting a stacked PR with both stories called out in the PR title/body. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-62-implementation-plan.md`](../plans/NEO-62-implementation-plan.md) | **Matches** — commit order (inventory → XP → depletion), rollback on XP failure, reason-code passthrough, **`GatherResult`** shape, acceptance checklist complete. | +| [`docs/plans/NEO-63-implementation-plan.md`](../plans/NEO-63-implementation-plan.md) | **Matches** — **`InteractionApi`** delegates **`resource_node`** to gather engine; NEO-41 inline XP removed; four registry anchors at kickoff coords; integration + Bruno + manual QA; acceptance checklist complete. | +| [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-06 and E3M1-07 landed notes and checkboxes updated. | +| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Matches** — four-node freeze table anchors, NEO-62 engine + NEO-63 interact slices, **`GatherResult`** field table, E1.M3 boundary (no separate gather HTTP). | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-62/NEO-63 landed; NEO-64 telemetry still planned. | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — **E3.M1 note** extended through E3M1-07; row **In Progress** until NEO-64. | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server resolves gather outcomes; client sends interact intent only. | +| [`docs/manual-qa/NEO-63.md`](../manual-qa/NEO-63.md) | **Matches** — happy path, depletion deny, terminal control, Bruno pointers. | +| [`server/README.md`](../../server/README.md) | **Matches** — gather engine subsection, gather-via-interact reason codes, four anchors. | + +Register/tracking: alignment table and **E3.M1 note** updated for NEO-62/NEO-63. + +## Blocking issues + +None. + +## Suggestions + +1. **AAA layout in depletion integration test** — `InteractionResourceNodeGatherIntegrationTests.PostInteract_ResourceNode_WhenDepleted_ShouldDenyWithNodeDepletedWithoutGrants` asserts **`Allowed`** inside the Arrange loop (10 pre-gathers). Move loop invariants to **Act** (fire-and-forget or collect last response) and keep all expectations in **Assert**, per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert) — same pattern flagged on NEO-61. + +2. **Tighten skill-deny integration assertion** — `PostInteract_ResourceNode_WhenSalvageDisallowsActivity_ShouldDenyWithoutGrants` only checks `ReasonCode` is non-empty. Assert the expected passthrough **`source_kind_not_allowed`** (from `SkillProgressionSnapshotApi.ReasonSourceKindNotAllowed`) so regressions in XP rollback + reason mapping are caught explicitly. + +3. **Optional HTTP `inventory_full` deny** — Engine test covers full-bag deny without depletion; no interact-level test yet. Low priority since **`GatherOperationsTests`** already locks the rule; add one HTTP test if you want parity with depletion deny at the wire boundary. + +4. **Test duplication (optional cleanup)** — `InteractionResourceNodeGatherXpTests` overlaps with `InteractionResourceNodeGatherIntegrationTests` on terminal control and alpha XP smoke. Plan kept both for regression; consider trimming the legacy file to terminal-only once NEO-63 integration tests are trusted. + +## Nits + +- Nit: **`SkillProgressionGrantOperations`** summary still references “NEO-41 gather interact hook”; interact now routes through **`GatherOperations`** (NEO-63). + +- Nit: **`GatherOperations`** class comment says “Interact wiring is NEO-63” — wiring is landed; update to “wired from **`InteractionApi`** (NEO-63)”. + +- Nit: Stacked branch — five commits include NEO-62 plan + engine before NEO-63 wiring. Call out both issue keys in the PR title/description so reviewers know scope. + +- Nit: Beta/gamma/delta per-node yield quantities (2/3/5) are validated in **`GatherOperationsTests`** (delta) but not via HTTP interact on the new anchors; acceptable for prototype, optional Bruno spot-check on delta. + +## Verification + +```bash +cd server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~Gather|FullyQualifiedName~Interaction" +# Passed! Failed: 0, Passed: 70 + +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +# Passed! Failed: 0, Passed: 292 +``` + +Manual / Bruno: run `docs/manual-qa/NEO-63.md` curl steps; Bruno folder `bruno/neon-sprawl-server/interaction/` — **`Post interact gather then get inventory.bru`**, **`Post interact depleted node deny.bru`**. + +Postgres persistence (when `ConnectionStrings__NeonSprawl` is set): gather depletion rows should persist across restarts via existing NEO-61 store (not re-tested in this diff).