neon-sprawl/docs/reviews/2026-05-31-NEO-104.md

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. Done. Note added to plan Implementation reconciliation.

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. Done. XML on IEncounterProgressStore.TryActivate.

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. Done. Two new tests in EncounterProgressOperationsTests plus mixed-case IsAllRequiredTargetsDefeated coverage.

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. Done. TryNormalizeKnown before lookup and progress read.

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. Done.

• 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. Done. IsCompleted uses per-key lock.

Verification

# 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.