diff --git a/docs/reviews/2026-05-10-NEO-41.md b/docs/reviews/2026-05-10-NEO-41.md new file mode 100644 index 0000000..cbeeb32 --- /dev/null +++ b/docs/reviews/2026-05-10-NEO-41.md @@ -0,0 +1,49 @@ +# 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` + +--- + +## Verdict + +**Approve with nits** — Implementation matches the adopted plan and server tests pass; one documentation alignment nit should be fixed before or immediately after merge. + +## 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 135 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` — **partially matches** (prototype anchor paragraph added per plan; **Summary** table **Status** still says **Planned** while the dependency register lists E3.M1 as **In Progress** — conflicts with [documentation_and_implementation_alignment.md](documentation_and_implementation_alignment.md) rule that Summary **Status** should match the register). +- `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:** This PR already updates the implementation tracking table and register; no further tracking update required once the E3.M1 module page Summary **Status** is aligned (see nits). + +## 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. + +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](documentation_and_implementation_alignment.md) (“Summary table Status field should match the register row”). + +## 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). + +## Verification + +- `dotnet test NeonSprawl.sln` — **passed** (135 tests) on review date. +- 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.