NEO-128: add code review for quest-complete reward wiring.

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

@@ -0,0 +1,60 @@
+# Code review — NEO-128 (E7M2-05)
+
+**Date:** 2026-06-07  
+**Scope:** Branch `NEO-128-e7m2-quest-complete-reward-wiring` vs `origin/main` — commits `7abe7dc` … `da99769`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits** — deliver-then-mark wiring in `QuestStateOperations.TryMarkComplete`, idempotent skip, deny passthrough, and dependency threading through objective wiring and upstream ops match the adopted plan and NEO-105 precedent; tests cover the primary AC paths; risk is low (server-internal until NEO-129 HTTP projection).
+
+## Summary
+
+NEO-128 wires **`RewardRouterOperations.TryDeliverQuestCompletion`** into **`QuestStateOperations.TryMarkComplete`** on the first-time active→completed path: bundle delivery runs before **`IPlayerQuestStateStore.TryMarkComplete`**, idempotent **`completed`** replay returns early without calling the router, and delivery denies passthrough router **`reasonCode`** values while leaving the quest **active**. Reward-router dependencies are threaded through **`QuestObjectiveWiring`**, **`GatherOperations`**, **`CraftOperations`**, **`EncounterCompletionOperations`**, **`QuestProgressApi`**, **`QuestAcceptApi`**, **`AbilityCastApi`**, and related call sites so wiring-driven completion exercises the same path. Five new/extended AAA integration tests lock gather-intro bundle delivery, idempotent replay (direct and wiring), and inventory-full deny on operator-chain setup; compile-fix updates propagate the expanded signatures across quest/gather/craft/encounter test harnesses. Docs (plan reconciliation, E7.M2 anchor, alignment register, backlog AC, `server/README.md`) are updated. A minimal Bruno health smoke was added (host boot check only).
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-128-implementation-plan.md` | **Matches** — kickoff deliver-then-mark and idempotent skip adopted; reconciliation accurate; AC checked. |
+| `docs/plans/E7M2-prototype-backlog.md` (E7M2-05) | **Matches** — AC checkboxes complete; Bruno listed as optional in backlog (health-only smoke landed). |
+| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-128 implementation anchor added. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes E7M2-05 quest-state wiring; row correctly stays **Planned** until NEO-132 capstone. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register change required. |
+| `server/README.md` | **Matches** — `TryMarkComplete` table row and reward-router cross-ref document deliver-then-mark, idempotent skip, deny passthrough. |
+| Full-stack epic decomposition | **N/A** — E7M2-05 is server engine; client counterpart NEO-131 correctly deferred (blocked by NEO-129). |
+
+**Register / tracking:** E7.M2 correctly remains **Planned** until capstone NEO-132; alignment update is appropriate.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Operator-chain happy-path delivery assertions** — The plan lists integration coverage for “operator-chain → item + skill grants.” `TryMarkComplete_ShouldDenyAndStayActive_WhenInventoryFull` exercises chain quest at the operations layer (deny only). `ChainQuest_ShouldAdvanceThroughMixedObjectives_WhenPrerequisitesMet` completes via wiring but does not assert **`IRewardDeliveryStore`**, **`survey_drone_kit`**, or salvage +50. NEO-127 covers router apply in isolation; extending the wiring or operations happy-path test would lock end-to-end **`TryMarkComplete`** + item grant for the only bundle with **`itemGrants`**.
+
+2. **Inventory-full via wiring path** — Deny is tested via direct **`TryMarkComplete`** after manual chain setup. Optional: one wiring-driven test (e.g. fill bag then complete terminal **`inventory_has_item`** step) to prove **`QuestObjectiveWiring`** best-effort ignore does not mask the deny for primary ops while quest stays **active** for retry.
+
+## Nits
+
+- Nit: Kickoff adopted “skip Bruno” for bundle verification, but a **`GET /health`** smoke was added under `bruno/neon-sprawl-server/quest-progress/` with docs pointing to dotnet test for bundle asserts. Harmless and consistent with backlog “optional” wording; consider a one-line plan note if authors want kickoff vs backlog Bruno policy explicit.
+
+- Nit: **`TryCompleteStepIfSatisfied`** discards **`TryMarkComplete`** return value (by design per plan — wiring best-effort). Inventory-full deny therefore does not surface on gather/craft/encounter envelopes; acceptable for prototype, worth remembering for NEO-131 client surfacing.
+
+- Nit: Delivery succeeds then **`TryMarkComplete`** store write fails — orphan store record self-heals via **`already_delivered`** on retry (plan deferred). No action required for prototype.
+
+- Nit: Expanded parameter lists on **`TryAccept`** / **`TryAdvanceStep`** mirror NEO-118 threading precedent; a future facade/DI record could reduce call-site churn but is explicitly out of scope.
+
+## Verification
+
+```bash
+dotnet test server/NeonSprawl.Server.Tests --filter "QuestStateOperationsTests|QuestObjectiveWiringTests"
+dotnet test server/NeonSprawl.Server.Tests --filter "FullyQualifiedName~Quests|FullyQualifiedName~GatherOperations|FullyQualifiedName~CraftOperations|FullyQualifiedName~EncounterCompletion|FullyQualifiedName~EncounterCombatWiring"
+```
+
+All 30 quest-state/wiring tests and 141 related tests pass (verified 2026-06-07).
+
+Optional before merge:
+
+- Boot dev server; run Bruno `GET health (quest complete reward wiring NEO-128)` or `curl /health` — no new DI registration beyond existing reward stores.