don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-24-NEO-63.md

6.6 KiB
Raw Permalink Blame History

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 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 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 Matches — E3M1-06 and E3M1-07 landed notes and checkboxes updated.
docs/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 Matches — E3.M1 row notes NEO-62/NEO-63 landed; NEO-64 telemetry still planned.
docs/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 Matches — server resolves gather outcomes; client sends interact intent only.
docs/manual-qa/NEO-63.md Matches — happy path, depletion deny, terminal control, Bruno pointers.
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 — 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

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