NEO-106: Add code review for combat encounter wiring.

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

@@ -0,0 +1,58 @@
+# Code review — NEO-106 (E5M3-07)
+
+**Date:** 2026-05-31  
+**Scope:** Branch `NEO-106-wire-combat-defeat-encounter-progress-completion` vs `7656fde` (merge-base on `main`) — commits `fc09808` … `8306f92`  
+**Base:** `7656fde` (main at branch point, post NEO-105 merge)
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-106 adds **`EncounterCombatWiring.TryProcessCastOutcome`**, a static orchestration helper (mirroring **`CombatDefeatGigXpGrant`**) that activates encounter progress on first damaging hit, marks defeats on lethal hits, and invokes **`EncounterCompletionOperations.TryCompleteAndGrant`** when all required prototype NPCs are defeated. **`AbilityCastApi`** injects encounter/inventory dependencies and calls the helper on every successful combat resolve, preserving the frozen call order: aggro → cooldown → NPC runtime stop → gig XP grant → encounter wiring. Six AAA unit tests and three in-proc integration tests cover activation, partial progress, full completion loot, gig XP preservation (**75** breach XP after three defeats), and idempotent replay on already-completed encounters. Docs (plan, backlog, E5.M3 snapshot, alignment register, module register, `server/README.md`) are updated. HTTP progress read and client counterpart remain correctly deferred to NEO-108 / NEO-110. Risk is low: best-effort wiring with no cast JSON changes; inventory deny fail-closed behavior inherited from NEO-105.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-106-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate; frozen call order and cast→encounter flow table reflected in code. |
+| `docs/plans/E5M3-prototype-backlog.md` (E5M3-07) | **Matches** — acceptance criteria checked; landed note cites plan. |
+| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — Status marks **E5M3-07 / NEO-106** landed; implementation snapshot bullet added. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-106** combat wiring; NEO-108+ HTTP still outstanding. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-106** wiring 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` | **Partially matches** — new **Encounter combat wiring (NEO-106)** section is accurate; encounter progress section still has one stale “land in NEO-106 / NEO-108” line (see Nits). |
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Unit test for 2/3 defeated** — Plan test table lists “completion not called when only 2/3 defeated.” Integration test **`PostAbilityCast_ShouldNotGrantEncounterLoot_WhenOnlyTwoNpcDefeated`** covers this at HTTP level; a wiring-level AAA test (two **`DefeatNpc`** calls, third **`TryProcessCastOutcome`** with `targetDefeated: false`) would mirror the plan table and isolate completion gating without HTTP.
+
+2. **Inventory-full on third defeat (wiring path)** — README and plan document fail-closed retry semantics; NEO-105 ops tests cover grant deny. Consider a wiring or integration test that fills the bag before the third lethal cast and asserts progress all-defeated but **`IsCompleted`** false and zero loot delta — exercises the documented retry path end-to-end.
+
+3. **`encounter_complete` success hook comment** — **`EncounterCompletionOperations`** has NEO-109 deny hook + E7.M2 success TODO; **`EncounterCombatWiring`** logs completion deny at Debug. A brief `// TODO(E9.M1): catalog emit — encounter_complete` on the success branch (or in **`TryCompleteAndGrant`**) would align with NEO-109 scope without runtime code.
+
+## Nits
+
+- Nit: **`server/README.md`** encounter progress section (line ~154) still reads “Combat cast wiring … land in NEO-106 / NEO-108” — combat wiring is landed; reword to “HTTP progress read lands in NEO-108” (the next line already links the NEO-106 section).
+
+- Nit: **`PostAbilityCast_ShouldGrantEncounterLootOnce_WhenAllThreeNpcDefeated`** reads **`inventoryAfter`** and resolves **`completionStore`** after the defeat loop (still under **Act**). Moving those reads into **Assert** would tighten AAA; behavior is unchanged.
+
+## Verification
+
+```bash
+# NEO-106-focused (9 tests — passed locally)
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~EncounterCombatWiring|FullyQualifiedName~PostAbilityCast_ShouldGrantEncounterLoot|FullyQualifiedName~PostAbilityCast_ShouldNotGrantEncounterLoot|FullyQualifiedName~PostAbilityCast_ShouldGrantGigXpPerDefeat"
+
+# Broader encounter slice (69 tests — passed locally)
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"
+
+# CI parity
+python3 scripts/validate_content.py
+```
+
+**Local run:** 9/9 NEO-106-focused tests passed; 69/69 `Encounters` filter tests passed.