From 87dff6e3f41573860866b6368f3c4b4e85679153 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 31 May 2026 16:36:42 -0400 Subject: [PATCH] NEO-104: Add code review for encounter progress stores. --- docs/reviews/2026-05-31-NEO-104.md | 60 ++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 docs/reviews/2026-05-31-NEO-104.md diff --git a/docs/reviews/2026-05-31-NEO-104.md b/docs/reviews/2026-05-31-NEO-104.md new file mode 100644 index 0000000..9cac928 --- /dev/null +++ b/docs/reviews/2026-05-31-NEO-104.md @@ -0,0 +1,60 @@ +# Code review — NEO-104 (E5M3-05) + +**Date:** 2026-05-31 +**Scope:** Branch `NEO-104-encounter-progress-completion-stores` vs `aa81914` (merge-base on `main`) — commits `727ce47` … `aaccbbb` +**Base:** `aa81914` (main at branch point, post NEO-103 merge) + +## Verdict + +**Approve with nits** + +## Summary + +NEO-104 adds server-owned per-player encounter **activation**, **defeat progress** (order-independent subset of `requiredNpcInstanceIds`), and an **idempotent completion flag** at the store layer, plus static **`EncounterProgressOperations`** that auto-resolve encounters from NPC ids via **`IEncounterDefinitionRegistry`**. Implementation mirrors established patterns (`InMemoryResourceNodeInstanceStore` per-key locks, `GatherOperations` / `AggroOperations` static orchestration): split **`IEncounterProgressStore`** / **`IEncounterCompletionStore`**, `EncounterProgressIds` normalization, DI in **`AddEncounterAndRewardCatalogs`**, telemetry hook comment on activation, and ten new AAA unit tests plus a host DI smoke in **`EncounterDefinitionRegistryTests`**. Docs (plan, backlog, alignment register, module register, E5.M3 snapshot, `server/README.md`) are updated. Combat cast and HTTP read are correctly deferred to NEO-106 / NEO-108; no client counterpart required. Risk is low: in-memory stores with no player-visible surface until NEO-108 / NEO-110. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-104-implementation-plan.md` | **Matches** — split stores, operations API, in-memory DI, deferred combat/HTTP, kickoff decisions recorded, acceptance checklist checked, reconciliation section accurate. | +| `docs/plans/E5M3-prototype-backlog.md` (E5M3-05) | **Matches** — acceptance criteria checked; landed note cites plan. | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — Summary **Status** marks **E5M3-05 / NEO-104** landed; **Implementation snapshot** progress bullet added. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-104** stores; **NEO-105+** grants/combat/HTTP still outstanding. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-104** store landing; **NEO-105+** outstanding. | +| Full-stack epic decomposition | **N/A** — plan explicitly excludes client counterpart until NEO-110 via NEO-108. | +| `server/README.md` | **Matches** — encounter progress section documents stores, operations, and deferred NEO-106/108 wiring. | + +## Blocking issues + +None. + +## Suggestions + +1. **Plan file list vs shipped diff** — The plan lists **`InMemoryWebApplicationFactory.cs`** and **`Program.cs`** as files to modify; neither changed. This is acceptable: stores register in the existing **`AddEncounterAndRewardCatalogs`** extension (already invoked at startup), and encounter stores are always in-memory (no Postgres swap needed in the test factory, unlike position/inventory stores). Consider a one-line note in the plan **Implementation reconciliation** that factory/Program edits were N/A. + +2. **Completion guard on `TryActivate`** — The plan describes **`TryActivate`** returning `false` when the completion store marks the encounter done. Gating lives only in **`EncounterProgressOperations.TryActivateOnFirstEngagement`**; a direct **`IEncounterProgressStore.TryActivate`** call after completion would still create a started row. Acceptable while NEO-106/105 route exclusively through operations; add a brief XML note on **`IEncounterProgressStore.TryActivate`** that completion gating is the caller's responsibility if direct store use is anticipated. + +3. **Optional operations coverage** — Store and core happy-path operations are well covered. Consider adding AAA tests for: **`TryActivateOnFirstEngagement`** returns `false` when encounter is already completed; resolve/activate returns `false` for an NPC id not in any encounter definition. + +4. **`IsAllRequiredTargetsDefeated` id normalization** — The method passes **`encounterId`** to case-sensitive **`TryGetDefinition`**. NEO-102 precedent documents that only **`TryNormalizeKnown`** lowercases. Internal callers today use catalog **`row.Id`** (lowercase); NEO-108 HTTP should normalize before calling — optionally harden now with **`TryNormalizeKnown`** or **`EncounterProgressIds.NormalizeEncounterId`** + lookup for defense in depth. + +## Nits + +- Nit: **`Host_ShouldResolveEncounterProgressStoresFromDi_WhenStartupSucceeds`** — **`Assert.NotNull`** on values from **`GetRequiredService`** is redundant (missing registration throws). Minor AAA nit: DI resolution fits **Arrange**; **Act** could be only **`TryActivate`** / **`TryMarkCompleted`**. + +- Nit: **`InMemoryEncounterCompletionStore.IsCompleted`** reads **`ConcurrentDictionary`** without the per-key lock used by **`TryMarkCompleted`** / **`TryGetCompletedAt`**. Benign for prototype bool checks; align with lock if strict read-your-writes under concurrency matters later. + +## Verification + +```bash +# NEO-104-focused (10 new tests + host DI smoke — passed locally) +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~EncounterProgressOperations|FullyQualifiedName~InMemoryEncounterProgressStore|FullyQualifiedName~InMemoryEncounterCompletionStore|Host_ShouldResolveEncounterProgressStoresFromDi" + +# Broader encounter slice (catalog + registries + HTTP + progress) +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters" + +# CI parity +python3 scripts/validate_content.py +``` + +**Local run:** 52/52 `Encounters` filter tests passed.