NEO-127: add code review for reward router operations

docs/reviews/2026-06-07-NEO-127.md

@@ -0,0 +1,61 @@
+# Code review — NEO-127 (E7M2-04)
+
+**Date:** 2026-06-07  
+**Scope:** Branch `NEO-127-e7m2-reward-router-operations` vs `origin/main` — commits `35b8904` … `5b84ffd`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits** — router apply, compensating rollback, idempotent replay, and store record flow mirror the NEO-105 encounter-completion precedent; plan AC and module contracts are satisfied; risk is low (server-internal only until NEO-128 wiring).
+
+## Summary
+
+NEO-127 adds **`RewardRouterOperations.TryDeliverQuestCompletion`** under `Game/Rewards/` to apply quest **`completionRewardBundle`** rows: item grants via **`PlayerInventoryOperations`** (simulate-all pre-flight, sequential apply, compensating rollback) and skill XP via **`SkillProgressionGrantOperations.TryApplyGrant`** with fixed **`mission_reward`** source kind. **`RewardDeliveryResult`** and **`RewardDeliveryReasonCodes`** provide structured outcomes; **`IRewardDeliveryStore`** records **`RewardDeliveryEvent`** on success with idempotent replay via **`TryGet`** before apply and race handling when **`TryRecord`** returns `false`. Seven AAA integration tests cover happy paths, inventory deny, skill deny rollback, store replay, and unknown-skill deny. Docs (`NEO-127` plan reconciliation, E7.M2 module anchor, alignment register, `server/README.md`) are updated. No player-visible behavior until NEO-128 wires quest completion.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-127-implementation-plan.md` | **Matches** — kickoff decisions adopted; AC checked; reconciliation accurate. |
+| `docs/plans/E7M2-prototype-backlog.md` (E7M2-04) | **Partially matches** — AC checkboxes complete; **In scope** still names **`MissionRewardSkillXpGrant`** while implementation correctly uses **`TryApplyGrant`** per plan kickoff (stale backlog wording only). |
+| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-127 implementation anchor documents router apply path. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes E7M2-04 router apply; row correctly stays **Planned** until NEO-132 capstone. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register change required for this slice. |
+| `server/README.md` | **Matches** — reward router section documents entry point, reason codes, apply order, idempotency + store race behavior. |
+| Full-stack epic decomposition | **N/A** — E7M2-04 is server engine only; no client counterpart required. |
+
+**Register / tracking:** E7.M2 correctly remains **Planned** until capstone NEO-132; alignment update is appropriate.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Empty / null bundle test** — Kickoff decision and plan both require success with empty grant snapshots and **`TryRecord`** for null or both-empty bundles. Code handles this (skips inventory pre-flight, records empty arrays), but no test locks the behavior. A single AAA test with `bundle: null` or `new QuestRewardBundleRow([], [])` would close the gap.
+
+2. **Invalid id deny tests** — `invalid_player_id` / `invalid_quest_id` are implemented and documented in `server/README.md`, but untested. Two small deny-path tests (empty/whitespace ids) would match the fail-closed store precedent from NEO-126.
+
+3. **Replay XP assertion** — In `TryDeliverQuestCompletion_ShouldApplyGatherIntroSkillXp_WhenBundleIsSkillOnly`, the second-call assertion reuses `salvageXp` captured before the replay `Deliver` call. Re-fetch **`GetXpTotals`** after the second call to prove no double XP grant on idempotent replay.
+
+## Nits
+
+- Nit: Pre-flight **`TrySimulateAddStack`** failure always denies with **`inventory_full`**, even when the underlying cause would be **`invalid_item`** or **`invalid_quantity`** at apply time. Same as **`EncounterCompletionOperations`** (NEO-105); acceptable for prototype catalog-gated content; apply-phase denies still return correct passthrough codes.
+
+- Nit: When **`TryRecord`** returns `false` and a subsequent **`TryGet`** also misses, the router still returns success with the locally built **`deliveryEvent`** (line 149 fallback). Extremely unlikely with the in-memory store; mirrors “retain grants, treat as idempotent success” policy from the plan. Consider a debug log if this path ever surfaces in production persistence.
+
+- Nit: **`CompensatingRemoveItems`** ignores **`TryRemoveStack`** outcomes (same as encounter compensating helper). Acceptable for prototype; multi-item bundles with partial rollback failure would be a future hardening item.
+
+- Nit: Optional plan test for **`source_kind_not_allowed`** negative path not implemented; catalog startup gates make this low priority.
+
+## Verification
+
+```bash
+dotnet test server/NeonSprawl.Server.Tests --filter RewardRouterOperationsTests
+```
+
+All 7 tests pass (verified 2026-06-07).
+
+Optional before merge:
+
+- Boot dev server; confirm `/health` 200 — no new DI registration; behavior unchanged until NEO-128.