From ab315f326b7af3751211f7a43ccdb71f543f533c Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 3 Jun 2026 22:22:44 -0400 Subject: [PATCH] NEO-115: address code review findings for quest definitions GET. Derive expected quest id order from PrototypeE7M1QuestCatalogRules, assert refine craft_recipe objective in integration test, and update module dependency register plus review doc strikethroughs. --- .../modules/module_dependency_register.md | 2 +- docs/reviews/2026-06-03-NEO-115.md | 8 ++++---- .../Quests/QuestDefinitionsWorldApiTests.cs | 20 +++++++++---------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index 86d7cf2..4692cac 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -109,7 +109,7 @@ Gameplay **content and curves** default to **boot load** with optional **dev-onl |---|---|---|---|---|---| | E7.M1 | QuestStateMachine | E3.M2, E5.M1 | QuestDef, QuestStepState, QuestStateTransition | Prototype | Planned | -**E7.M1 note:** Epic 7 **Slice 1** backlog in Linear ([Epic 7 — Questing, Narrative, and Faction Reputation](https://linear.app/neon-sprawl/project/epic-7-questing-narrative-and-faction-reputation-a9416783ceee)): [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) → [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123); label **`E7.M1`**. See [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md), [E7_M1_QuestStateMachine.md](E7_M1_QuestStateMachine.md). Upstream: E3.M1 gather **Ready**, E3.M2 craft **Ready**, E5.M3 encounter **Ready**. Client capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123). **E7M1-01 / NEO-112** catalog landed (schemas + CI); **E7M1-02 / NEO-113** server load landed; **E7M1-03 / NEO-114** registry landed ([NEO-114 plan](../../plans/NEO-114-implementation-plan.md)); register row stays **Planned** until quest runtime (E7M1-05+) lands. +**E7.M1 note:** Epic 7 **Slice 1** backlog in Linear ([Epic 7 — Questing, Narrative, and Faction Reputation](https://linear.app/neon-sprawl/project/epic-7-questing-narrative-and-faction-reputation-a9416783ceee)): [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) → [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123); label **`E7.M1`**. See [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md), [E7_M1_QuestStateMachine.md](E7_M1_QuestStateMachine.md). Upstream: E3.M1 gather **Ready**, E3.M2 craft **Ready**, E5.M3 encounter **Ready**. Client capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123). **E7M1-01 / NEO-112** catalog landed (schemas + CI); **E7M1-02 / NEO-113** server load landed; **E7M1-03 / NEO-114** registry landed ([NEO-114 plan](../../plans/NEO-114-implementation-plan.md)); **E7M1-04 / NEO-115** HTTP read landed ([NEO-115 plan](../../plans/NEO-115-implementation-plan.md)); register row stays **Planned** until quest runtime (E7M1-05+) lands. | E7.M2 | RewardAndUnlockRouter | E2.M2, E3.M3, E7.M1 | QuestRewardBundle, UnlockGrant, RewardDeliveryEvent | Prototype | Planned | | E7.M3 | FactionReputationLedger | E7.M1 | FactionStanding, ReputationDelta, FactionGateRule | Pre-production | Planned | | E7.M4 | ContractMissionGenerator | E4.M1, E5.M3, E7.M3 | ContractTemplate, ContractSeed, ContractOutcome | Pre-production | Planned | diff --git a/docs/reviews/2026-06-03-NEO-115.md b/docs/reviews/2026-06-03-NEO-115.md index 599d5d8..558c35d 100644 --- a/docs/reviews/2026-06-03-NEO-115.md +++ b/docs/reviews/2026-06-03-NEO-115.md @@ -20,7 +20,7 @@ NEO-115 adds **`GET /game/world/quest-definitions`**, a versioned read-only proj | `docs/plans/E7M1-prototype-backlog.md` (E7M1-04) | **Matches** — AC checked; in/out of scope aligned with shipped route. | | `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — HTTP read model bullet documents NEO-115 route + Bruno. | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-115 HTTP projection; register stays **Planned** until E7M1-05+ runtime. | -| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E7.M1 note still ends at NEO-114 registry; alignment doc covers NEO-115 (see Suggestion 2). | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note includes NEO-115 HTTP read (Suggestion 2 addressed). | | `server/README.md` | **Matches** — quest definitions section with curl, registry note, plan/Bruno links. | | Full-stack epic decomposition | **N/A** — server-only; client counterpart NEO-122 explicitly deferred. | @@ -30,13 +30,13 @@ NEO-115 adds **`GET /game/world/quest-definitions`**, a versioned read-only proj ## Suggestions -1. **Reuse frozen id ordering constant in tests** — Plan adopted reusing **`PrototypeE7M1QuestCatalogRules`** for ids. `QuestDefinitionsWorldApiTests` defines a parallel **`FrozenQuestIdsInOrdinalOrder`** array instead of deriving from **`ExpectedQuestIds.Order(StringComparer.Ordinal)`** (as **`QuestDefinitionRegistryTests`** already does). Consolidating avoids drift if the roster changes. +1. ~~**Reuse frozen id ordering constant in tests** — Plan adopted reusing **`PrototypeE7M1QuestCatalogRules`** for ids. `QuestDefinitionsWorldApiTests` defines a parallel **`FrozenQuestIdsInOrdinalOrder`** array instead of deriving from **`ExpectedQuestIds.Order(StringComparer.Ordinal)`** (as **`QuestDefinitionRegistryTests`** already does). Consolidating avoids drift if the roster changes.~~ **Done.** `QuestDefinitionsWorldApiTests` now asserts id order via **`ExpectedQuestIds.Order(StringComparer.Ordinal)`**. -2. **Optional: refresh `module_dependency_register` E7.M1 note** — E7.M1 module page and alignment register mention NEO-115; the register table note still stops at NEO-114. A one-line append (“**E7M1-04 / NEO-115** HTTP read landed”) would keep the dependency register consistent with NEO-114 follow-up style. +2. ~~**Optional: refresh `module_dependency_register` E7.M1 note** — E7.M1 module page and alignment register mention NEO-115; the register table note still stops at NEO-114. A one-line append (“**E7M1-04 / NEO-115** HTTP read landed”) would keep the dependency register consistent with NEO-114 follow-up style.~~ **Done.** E7.M1 note appended in **`module_dependency_register.md`**. ## Nits -- Nit: C# integration test asserts **`refine_intro`** prerequisite only, not the **`craft_recipe`** objective shape; Bruno likewise skips refine/combat spot-checks. Adequate for prototype freeze given gather + operator-chain + combat coverage, but a single **`craft_recipe`** assert would close the objective-kind loop in C#. +- ~~Nit: C# integration test asserts **`refine_intro`** prerequisite only, not the **`craft_recipe`** objective shape; Bruno likewise skips refine/combat spot-checks. Adequate for prototype freeze given gather + operator-chain + combat coverage, but a single **`craft_recipe`** assert would close the objective-kind loop in C#.~~ **Done.** `refine_intro` **`craft_recipe`** objective asserted in **`QuestDefinitionsWorldApiTests`**. - Nit: **`MapObjective`** default switch arm copies all optional fields for unknown kinds — unreachable for validated catalog content but reasonable defensive fallback. diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionsWorldApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionsWorldApiTests.cs index c5157df..bb1c333 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionsWorldApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionsWorldApiTests.cs @@ -7,15 +7,6 @@ namespace NeonSprawl.Server.Tests.Game.Quests; public class QuestDefinitionsWorldApiTests { - /// Frozen prototype quest ids in ascending ordinal order. Keep in sync with Bruno. - public static readonly string[] FrozenQuestIdsInOrdinalOrder = - [ - "prototype_quest_combat_intro", - "prototype_quest_gather_intro", - "prototype_quest_operator_chain", - "prototype_quest_refine_intro", - ]; - [Fact] public async Task GetQuestDefinitions_ShouldReturnSchemaV1_WithFourFrozenQuestsInIdOrder() { @@ -31,7 +22,9 @@ public class QuestDefinitionsWorldApiTests Assert.Equal(QuestDefinitionsListResponse.CurrentSchemaVersion, body!.SchemaVersion); Assert.NotNull(body.Quests); Assert.Equal(4, body.Quests.Count); - Assert.Equal(FrozenQuestIdsInOrdinalOrder, body.Quests.Select(q => q.Id).ToArray()); + Assert.Equal( + PrototypeE7M1QuestCatalogRules.ExpectedQuestIds.Order(StringComparer.Ordinal).ToArray(), + body.Quests.Select(q => q.Id).ToArray()); var gatherIntro = body.Quests.Single(q => q.Id == "prototype_quest_gather_intro"); Assert.Equal("Intro: Salvage Run", gatherIntro.DisplayName); @@ -51,6 +44,13 @@ public class QuestDefinitionsWorldApiTests var refineIntro = body.Quests.Single(q => q.Id == "prototype_quest_refine_intro"); Assert.Equal(["prototype_quest_gather_intro"], refineIntro.PrerequisiteQuestIds); + var refineObjective = refineIntro.Steps[0].Objectives[0]; + Assert.Equal("refine_intro_obj_recipe", refineObjective.Id); + Assert.Equal("craft_recipe", refineObjective.Kind); + Assert.Equal("refine_scrap_standard", refineObjective.RecipeId); + Assert.Equal(1, refineObjective.Quantity); + Assert.Null(refineObjective.ItemId); + Assert.Null(refineObjective.EncounterId); var combatIntro = body.Quests.Single(q => q.Id == "prototype_quest_combat_intro"); Assert.Single(combatIntro.Steps);