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

Code review — NEO-127 (E7M2-04)

Date: 2026-06-07
Scope: Branch NEO-127-e7m2-reward-router-operations vs origin/main — commits 35b8904 … 65ca111
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. Ten AAA integration tests cover happy paths, inventory deny, skill deny rollback, store replay, unknown-skill deny, null-bundle empty record, and invalid-id 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)	Matches — AC checkboxes complete; in-scope skill XP path aligned with TryApplyGrant.
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. Done. TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull.

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. Done. TryDeliverQuestCompletion_ShouldDenyInvalidPlayerId_WhenPlayerIdIsEmpty and …InvalidQuestId….

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. Done. Gather intro test re-fetches XP after 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

dotnet test server/NeonSprawl.Server.Tests --filter RewardRouterOperationsTests

All 14 tests pass (verified 2026-06-07; includes concurrent level-up winner perk preservation on TryRecord race loss).

Optional before merge:

• Boot dev server; confirm /health 200 — no new DI registration; behavior unchanged until NEO-128.