don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-31-NEO-107.md

4.6 KiB
Raw Permalink Blame History

Code review — NEO-107 (E5M3-09)

Date: 2026-05-31
Scope: Branch NEO-107-encounter-complete-event-e7m2-hook vs 856dc8d (merge-base on main) — commits 986cf55 … 555c323
Base: 856dc8d (main at branch point, post NEO-106 merge)

Verdict

Approve with nits

Summary

NEO-107 adds IEncounterCompleteEventStore and InMemoryEncounterCompleteEventStore — a dedicated, idempotent per-player+encounter record for EncounterCompleteEvent, mirroring the NEO-104 split-store pattern (ConcurrentDictionary + per-key locks). EncounterCompletionOperations.TryCompleteAndGrant records the event immediately after a successful TryMarkCompleted, with expanded E7.M2 reward_delivery hook comments and no runtime router behavior. The store is registered in AddEncounterAndRewardCatalogs and threaded through EncounterCombatWiring → AbilityCastApi. Tests cover store idempotency, all completion deny paths (store stays empty), wiring third-defeat success, and host DI resolution. Docs (plan, backlog E5M3-09, E5.M3 snapshot, E7.M2 anchor, alignment register, module register, server/README.md) are updated. HTTP projection and Godot HUD remain correctly deferred to NEO-108 / NEO-110. Risk is low: append-only record on an already-idempotent completion path; TryRecord false-after-fresh-mark is documented as benign idempotent success.

Documentation checked

Path Result
docs/plans/NEO-107-implementation-plan.md Matches — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate; frozen completion→event flow table reflected in code and tests.
docs/plans/E5M3-prototype-backlog.md (E5M3-09) Matches — acceptance criteria checked; landed note cites plan.
docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md Matches — implementation snapshot bullet for NEO-107 added; summary Status row includes E5M3-09 / NEO-107 landed; NEO-105 bullet cross-links NEO-107 persistence (review follow-up).
docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md Matches — NEO-107 producer hook anchor added with correct consumer contract vocabulary.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E5.M3 row notes NEO-107 event record.
docs/decomposition/modules/module_dependency_register.md Matches — E5.M3 note cites NEO-107 event store 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 — new Encounter complete event record (NEO-107) section is accurate; NEO-105 section updated to reflect persistence.

Blocking issues

None.

Suggestions

  1. E5.M3 summary Status row — Update the module summary table (line 10) to include E5M3-09 / NEO-107 alongside the other landed slices so the header matches the implementation snapshot bullets below. Done. Status row updated in E5_M3_EncounterAndRewardTables.md.

  2. Stale NEO-105 deferral line — In E5_M3_EncounterAndRewardTables.md, the NEO-105 bullet still reads “event persistence deferred to NEO-107.” Reword to point at the NEO-107 snapshot (or drop the deferral phrase) now that persistence is landed. Done. NEO-105 bullet now cross-links IEncounterCompleteEventStore (NEO-107).

  3. Empty encounter id fail-closed test — Plan test table lists “empty key / invalid ids fail closed.” InMemoryEncounterCompleteEventStoreTests covers whitespace PlayerId only; a symmetric case with empty EncounterId would close the gap (store layer only). Done. Added TryRecord_ShouldFailClosed_WhenEncounterIdIsEmpty; split empty PlayerId into its own test.

Nits

  • Nit: InMemoryEncounterCompleteEventStoreTests — both test methods call TryGet under Act; move verification reads to Assert for full AAA per csharp-style.md. Done. TryGet moved to Assert in all store tests.

  • Nit: EncounterDefinitionRegistryTests resolves IEncounterCompleteEventStore in Arrange but only asserts IsType<InMemoryEncounterCompleteEventStore> — sufficient for DI smoke; no functional TryRecord assertion needed unless you want parity with progress/completion store tests.

Verification

# NEO-107-focused (73 tests — passed locally)
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"

# CI parity
python3 scripts/validate_content.py

Local run: 73/73 Encounters filter tests passed.