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-10-NEO-41.md

4.2 KiB
Raw Permalink Blame History

Code review: NEO-41 (gather interact → skill XP)

Date: 2026-05-10

Scope: Branch NEO-41-gather-loop-awards-skill-xp-activity (commits through c1e7728) vs origin/main. Linear issue NEO-41.

Base: origin/main

Follow-up: Suggestions and nits below are done (strikethrough + Done.).

Verdict

Approve — Implementation matches the adopted plan; review nits and optional test suggestion addressed in follow-up commit.

Summary

This work introduces shared SkillProgressionGrantOperations.TryApplyGrant (extracted from NEO-38 POST handling), wires POST /game/players/{id}/interact to call it when the resolved prototype entry has kind: resource_node, using fixed salvage + activity + 10 XP via GatherSkillXpConstants. Denied grants and missing progression store are intentionally silent on the interact path (outcome remains allowed), matching the plan. NEO-40 telemetry hook comments now live on the shared grant path so both HTTP POST and interact-triggered grants see the same hook sites. Docs, Bruno requests, manual QA, and decomposition updates accompany the code. Risk: Low; behavior is additive, authority remains server-side, and 136 server tests pass (dotnet test NeonSprawl.sln).

Documentation checked

  • docs/plans/NEO-41-implementation-plan.md — matches (prototype resource_node interact, shared grant path, silent no-op on deny/store miss, 10 XP, salvage + activity, constants/registry usage, tests and manual QA listed).
  • docs/decomposition/modules/module_dependency_register.md — matches (E3.M1 row moved to In Progress; E2.M2 note consistent with NEO-41).
  • docs/decomposition/modules/documentation_and_implementation_alignment.md — matches (E2.M2 / E3.M1 tracking rows updated for NEO-41).
  • docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md — matches (NEO-41 implementation snapshot paragraph).
  • docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md — Matches — Summary Status set to In Progress (aligned with register); prototype anchor paragraph retained.
  • docs/manual-qa/NEO-41.md — matches intent (move → interact → GET progression).
  • server/README.md (diff) — matches plan (prototype gather XP pointer).
  • Cross-cutting client_server_authority.md / contracts.md — N/A for this slice (no new client authority; HTTP spikes unchanged in contract “Ready” sense).

Register / tracking: Implementation tracking table + register updated; E3.M1 module Summary Status aligned with register in follow-up.

Blocking issues

None.

Suggestions

  1. Optional test from plan: The implementation plan called out an optional unit/integration test for a “corrupt catalog” / disallowed sourceKind path ensuring the interact handler does not throw and still returns Allowed: true. The current tests cover terminal (no XP) and resource node (XP) happy paths; a focused test with a stub registry that denies activity for salvage would fully close the stated defensive scenario without needing production catalog corruption. Done. (SalvageActivityDeniedRegistryWebApplicationFactory + PostInteract_ResourceNode_WhenSalvageDisallowsActivity_ShouldStillAllowInteract_AndNotApplyXp.)

  2. E3.M1 Summary table: Update the Status field in docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md from Planned to In Progress so it matches module_dependency_register.md and documentation_and_implementation_alignment.md (“Summary table Status field should match the register row”). Done.

Nits

  • Nit: _ = SkillProgressionGrantOperations.TryApplyGrant(...) is clear enough; if the team prefers explicit fire-and-forget documentation, a one-line comment that the outcome is intentionally ignored on the interact path could mirror the plan wording (optional). Done. (Comment on interact path in InteractionApi.cs.)

Verification

  • dotnet test NeonSprawl.sln — passed (136 tests) after follow-up.
  • Manual: follow docs/manual-qa/NEO-41.md and/or Bruno flows under bruno/neon-sprawl-server/position/ and bruno/neon-sprawl-server/interaction/ for end-to-end smoke.