# 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` | **Matches** — new **Encounter combat wiring (NEO-106)** section is accurate; encounter progress section defers HTTP read to NEO-108 only. |

## 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.~~ **Done.** `TryProcessCastOutcome_ShouldNotComplete_WhenOnlyTwoTargetsDefeatedAndThirdHitNonLethal`.

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.~~ **Done.** `TryProcessCastOutcome_ShouldRetainProgressWithoutCompletion_WhenBagFullOnThirdDefeat`.

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.~~ **Done.** Hook comments on **`EncounterCombatWiring`** success path and **`EncounterCompletionOperations.TryCompleteAndGrant`**.

## 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).~~ **Done.**

- ~~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.~~ **Done.** Inventory GET and completion store resolve moved to **Assert** in both encounter-loot integration tests.

## Verification

```bash
# NEO-106-focused (11 tests — passed locally)
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~EncounterCombatWiring|FullyQualifiedName~PostAbilityCast_ShouldGrantEncounterLoot|FullyQualifiedName~PostAbilityCast_ShouldNotGrantEncounterLoot|FullyQualifiedName~PostAbilityCast_ShouldGrantGigXpPerDefeat"

# Broader encounter slice (71 tests — passed locally)
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"

# CI parity
python3 scripts/validate_content.py
```

**Local run:** 11/11 NEO-106-focused tests passed; 71/71 `Encounters` filter tests passed.