From 84847e20f05ec1f2ef05e461bbbc668d06c793d4 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 6 Jun 2026 19:29:10 -0400 Subject: [PATCH] NEO-117: Add code review for QuestStateOperations branch. --- docs/reviews/2026-06-06-NEO-117.md | 67 ++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 docs/reviews/2026-06-06-NEO-117.md diff --git a/docs/reviews/2026-06-06-NEO-117.md b/docs/reviews/2026-06-06-NEO-117.md new file mode 100644 index 0000000..9e3f05c --- /dev/null +++ b/docs/reviews/2026-06-06-NEO-117.md @@ -0,0 +1,67 @@ +# Code review — NEO-117 (E7M1-06) + +**Date:** 2026-06-06 +**Scope:** Branch `NEO-117-quest-state-operations` vs `origin/main` — commits `44ef5f2` … `f053ffb` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-117 adds **`QuestStateOperations`** — static accept, step advance, and complete wrappers over **`IPlayerQuestStateStore`** with **`QuestStateReasonCodes`** and **`QuestStateOperationResult`**, mirroring the NEO-104/105 encounter store + operations pattern. Prerequisite enforcement reads catalog **`prerequisiteQuestIds`**; complete is **idempotent at the operations layer** (replay → `Success: true` with unchanged snapshot), including a defensive re-read after store denial for concurrent complete. Fifteen AAA tests cover accept prerequisites/denials, advance bounds, idempotent complete, and host DI smoke via **`InMemoryWebApplicationFactory`**. **`server/README.md`** documents operations and reason codes. Server-only infrastructure — correct scope; HTTP and objective wiring remain deferred. Risk is low; main follow-ups are test helper consistency, minor coverage gaps, and post-merge decomposition register updates. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-117-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. | +| `docs/plans/E7M1-prototype-backlog.md` (E7M1-06) | **Partially matches** — implementation satisfies AC; backlog checkboxes still `[ ]` (expected until merge/story end). | +| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Partially matches** — module page still lists **`QuestStateOperations`** as future E7M1-06; should note NEO-117 landed after merge. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M1 row ends at E7M1-05 / NEO-116; plan defers register update until story lands — append E7M1-06 / NEO-117 on merge. | +| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — no NEO-117 note yet; same deferred update as NEO-116 review. | +| `server/README.md` | **Matches** — quest operations section, reason-code table, idempotent complete semantics, HTTP deferral to NEO-119/NEO-120. | +| Full-stack epic decomposition | **N/A** — server-only; client counterpart explicitly NEO-120/NEO-122; plan does not claim prototype slice complete. | + +**Register / tracking:** E7.M1 register row and module **Status** should gain an **E7M1-06 / NEO-117 landed** bullet after merge (register stays **Planned** until E7M1-07+ runtime and client capstone). + +## Blocking issues + +(none) + +## Suggestions + +1. **Test `ResolveDependencies` pattern** — `QuestStateOperationsTests.ResolveDependencies` creates an `IServiceScope` that is never disposed. **`EncounterCompletionOperationsTests`** resolves from `factory.Services` root (singleton stores). Align to avoid undisposed scopes and match the established encounter test pattern. + +2. **`unknown_quest` coverage on advance/complete** — Accept path has `TryAccept_ShouldDenyUnknownQuest_WhenQuestIdNotInCatalog`; advance and complete lack parity. Add two small `[Fact]` cases (mirror **`EncounterCompletionOperationsTests`** unknown-encounter test) for registry gate consistency. + +3. **Post-merge decomposition docs** — Plan defers register/module updates; on story end append **E7M1-06 / NEO-117** to `documentation_and_implementation_alignment.md`, `E7_M1_QuestStateMachine.md` (operations bullet + README link), and check E7M1-06 AC boxes in `E7M1-prototype-backlog.md`. + +## Nits + +- Nit: **`TryAccept`** progress switch default `_ => Deny(UnknownQuest)` is unreachable while **`QuestProgressStatus`** is only `Active`/`Completed`; if kept for exhaustiveness, a more accurate code (or `NotActive`) would aid future enum extensions. + +- Nit: Chain-quest setup tests hardcode **`prototype_quest_combat_intro`** and **`prototype_quest_refine_intro`**; plan deferred **`RefineIntroQuestId`** / **`CombatIntroQuestId`** constants — acceptable for now, but NEO-118 wiring will reuse these ids. + +- Nit: **`TryMarkComplete`** re-calls **`TryGetProgress`** after the active-row read (lines 117–118) where `active` could be reused on the `not_active` deny path — minor readability only. + +## Verification + +```bash +# Quest state operations (primary signal for this story) +cd server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \ + --filter "FullyQualifiedName~QuestStateOperationsTests" + +# Quest store regression (precursor) +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \ + --filter "FullyQualifiedName~InMemoryPlayerQuestStateStoreTests|FullyQualifiedName~PlayerQuestProgressPersistence" + +# Full server suite (CI) +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +``` + +**Reviewer note:** All 15 `QuestStateOperationsTests` and full server suite (663 tests) passed locally during review. + +**Manual:** None required — no HTTP or player-visible surface in this story.