diff --git a/docs/reviews/2026-06-07-NEO-127.md b/docs/reviews/2026-06-07-NEO-127.md new file mode 100644 index 0000000..625aea7 --- /dev/null +++ b/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.