68 lines
5.3 KiB
Markdown
68 lines
5.3 KiB
Markdown
# 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) | **Matches** — E7M1-06 acceptance checkboxes checked. |
|
||
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — NEO-117 operations bullet + README link added. |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7M1-06 / NEO-117 landed note appended. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — NEO-117 note appended. |
|
||
| `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.~~ **Done.** `ResolveDependencies` and host smoke test resolve from `factory.Services`.
|
||
|
||
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.~~ **Done.** Added `TryAdvanceStep_ShouldDenyUnknownQuest_WhenQuestIdNotInCatalog` and `TryMarkComplete_ShouldDenyUnknownQuest_WhenQuestIdNotInCatalog`.
|
||
|
||
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`.~~ **Done.** Alignment register, module doc, dependency register note, and E7M1-06 backlog checkboxes updated.
|
||
|
||
## 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.~~ **Done.** Replaced switch with explicit `Completed` / `AlreadyActive` branches.
|
||
|
||
- ~~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.~~ **Done.** Added **`RefineIntroQuestId`** and **`CombatIntroQuestId`** to **`PrototypeE7M1QuestCatalogRules`**; tests updated.
|
||
|
||
- ~~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.~~ **Done.** Single **`TryGetProgress`** read at start of **`TryMarkComplete`**.
|
||
|
||
## 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.
|