diff --git a/docs/reviews/2026-05-31-NEO-105.md b/docs/reviews/2026-05-31-NEO-105.md new file mode 100644 index 0000000..d282648 --- /dev/null +++ b/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.