# 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](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.