From a7b835290cc7738de636a42620534d6bf1c79c94 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 31 May 2026 18:09:20 -0400 Subject: [PATCH] NEO-106: Add code review for combat encounter wiring. --- docs/reviews/2026-05-31-NEO-106.md | 58 ++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 docs/reviews/2026-05-31-NEO-106.md diff --git a/docs/reviews/2026-05-31-NEO-106.md b/docs/reviews/2026-05-31-NEO-106.md new file mode 100644 index 0000000..4ed4a7b --- /dev/null +++ b/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.