61 lines
5.5 KiB
Markdown
61 lines
5.5 KiB
Markdown
# 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)
|
|
|
|
**Follow-up:** Review suggestions and nits addressed in commit after `009fedf`.
|
|
|
|
## 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.~~ **Done.** `TryCompleteAndGrant_ShouldRollbackFirstGrant_WhenSecondApplyFails` uses **`InventoryStoreFullAfterFirstGrant`** to fill the bag after the first apply so pre-flight still passes.
|
|
|
|
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.~~ **Done.** `encounter_complete_denied` hook comment added on **`Deny`**.
|
|
|
|
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.~~ **Addressed.** Inline comment on empty-grants branch; dedicated reason deferred until zero-grant tables ship.
|
|
|
|
## 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.~~ **Done.** Unrecognized codes passthrough; **`null`** still maps to **`inventory_full`**.
|
|
|
|
- ~~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).~~ **Done.** Assert **`first.ReasonCode`** null and **`first.GrantsApplied.Count`** == 2 alongside second-call denies.
|
|
|
|
## 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.
|