# 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.~~ **Done.** — pre-gather loop moved to **Act**; **`Assert.All`** on successful responses; depletion + totals asserted in **Assert**.

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.~~ **Done.**

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.~~ **Done.** — **`PostInteract_ResourceNode_WhenBagFull_ShouldDenyWithInventoryFullWithoutDepletingNode`**.

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.~~ **Done.** — legacy file trimmed to terminal-only; **`InteractionApiTests`** summary points at NEO-63 integration tests.

## Nits

- ~~Nit: **`SkillProgressionGrantOperations`** summary still references “NEO-41 gather interact hook”; interact now routes through **`GatherOperations`** (NEO-63).~~ **Done.**

- ~~Nit: **`GatherOperations`** class comment says “Interact wiring is NEO-63” — wiring is landed; update to “wired from **`InteractionApi`** (NEO-63)”.~~ **Done.**

- 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).