NEO-105: Add code review for encounter completion grants.

docs/reviews/2026-05-31-NEO-105.md

@@ -0,0 +1,58 @@
+# Code review — NEO-105 (E5M3-06)
+
+**Date:** 2026-05-31  
+**Scope:** Branch `NEO-105-encounter-completion-inventory-reward-apply` vs `719a14f` (merge-base on `main`) — commits `b5306aa` … `009fedf`  
+**Base:** `719a14f` (main at branch point, post NEO-104 merge)
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-105 adds **`EncounterCompletionOperations.TryCompleteAndGrant`**, which resolves an encounter and reward table, pre-flights inventory via **`PlayerInventoryOperations.TrySimulateAddStack`**, applies **`fixedGrants`** sequentially with compensating rollback on partial failure or failed **`TryMarkCompleted`**, and returns an internal **`EncounterCompleteEvent`** payload on success (persistence deferred to NEO-107). Supporting types mirror established patterns (`GatherOperations` / `CraftOperations`): **`EncounterCompletionResult`**, **`EncounterCompletionReasonCodes`**, **`EncounterGrantApplied`**. Six AAA unit tests cover grant-once, idempotent replay, `not_ready`, bag-full fail-closed, mark-fail rollback, and unknown encounter. Docs (plan, backlog, E5.M3 snapshot, alignment register, module register, `server/README.md`) are updated. Combat wiring and HTTP read remain correctly deferred to NEO-106 / NEO-108; client counterpart NEO-110 is out of scope. Risk is low: static ops with no new DI or player-visible surface until NEO-106.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-105-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate; combat/HTTP/client deferred. |
+| `docs/plans/E5M3-prototype-backlog.md` (E5M3-06) | **Matches** — acceptance criteria checked; landed note cites plan. |
+| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — Status marks **E5M3-06 / NEO-105** landed; implementation snapshot bullet added. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-105** completion grants; NEO-106+ combat/HTTP still outstanding. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-105** completion ops landed. |
+| Full-stack epic decomposition | **N/A** — plan explicitly defers client counterpart to **NEO-110** via NEO-108; no prototype slice completion claim. |
+| `server/README.md` | **Matches** — encounter completion section documents ops, reason codes, frozen grants, and deferrals. |
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Apply-phase partial rollback test** — The plan test table mentions compensating rollback when the bag cannot fit both grants, but **`FillBag`** fills every slot so pre-flight denies before any apply; the mark-fail test covers rollback after both grants succeed. Consider an AAA test where pre-flight passes but the second **`TryAddStack`** fails during apply (e.g. stub inventory store that allows simulate/add for the first grant only) to exercise **`CompensatingRemoveGrants`** on the apply path, not only the mark-complete race path.
+
+2. **Denial telemetry hook comment** — **`EncounterProgressOperations`** documents an E9.M1 hook on activation; **`CraftOperations`** / **`GatherOperations`** comment deny sites. **`EncounterCompletionOperations`** only has the E7.M2 success TODO. A brief `// TODO(E9.M1): catalog emit — encounter_complete_denied` (or similar) on **`Deny`** would align with NEO-109 without adding runtime code.
+
+3. **`unknown_reward_table` for empty `FixedGrants`** — Empty grants return **`unknown_reward_table`** (also documented in README). Prototype catalog gates prevent this at startup; if a future table legitimately has zero fixed grants, a dedicated reason code may be clearer. Fine for prototype; note for catalog expansion.
+
+## Nits
+
+- Nit: **`MapInventoryReason`** default branch maps unrecognized inventory reason codes to **`inventory_full`**. Safe given startup validation; passthrough of the raw code (or a dedicated **`inventory_denied`**) would aid debugging if invalid catalog rows slip through in tests.
+
+- Nit: **`TryCompleteAndGrant_ShouldDenyAlreadyCompleted_WhenCalledTwice`** — first successful completion is correctly in **Arrange**; consider asserting **`first.ReasonCode`** is null and **`second.GrantsApplied`** is empty in one block for symmetry with the success test (already partially covered).
+
+## Verification
+
+```bash
+# NEO-105-focused (6 tests — passed locally)
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~EncounterCompletionOperations"
+
+# Broader encounter slice (62 tests — passed locally; one flaky failure observed on first full run, passed on retry)
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"
+
+# CI parity
+python3 scripts/validate_content.py
+```
+
+**Local run:** 6/6 `EncounterCompletionOperations` tests passed; 62/62 `Encounters` filter tests passed on retry.