diff --git a/docs/plans/NEO-125-implementation-plan.md b/docs/plans/NEO-125-implementation-plan.md index 125f859..08cfec1 100644 --- a/docs/plans/NEO-125-implementation-plan.md +++ b/docs/plans/NEO-125-implementation-plan.md @@ -53,7 +53,7 @@ - **DTOs:** `QuestSkillXpGrantRow`, `QuestRewardBundleRow`; optional **`CompletionRewardBundle`** on **`QuestDefRow`**. - **Rules:** `PrototypeE7M2QuestCatalogRules` — presence, freeze-table, cross-ref + **`mission_reward`** allowlist gates synced to `validate_content.py`. - **Loader:** registers bundle-related JSON Schemas; parses **`completionRewardBundle`**; accepts **`SkillDefinitionCatalog.ById`** from DI. -- **Tests:** 25 AAA tests in **`QuestDefinitionCatalogLoaderTests`** (5 new E7M2 cases + updated prototype fixture with bundles). +- **Tests:** 25 AAA tests in **`QuestDefinitionCatalogLoaderTests`** (5 E7M2 loader cases + updated prototype fixture with bundles); **`PrototypeE7M2QuestCatalogRulesTests`** (4 direct cross-ref / allowlist cases). - **Docs:** `server/README.md` quest catalog E7M2 paragraph; E7M2-02 backlog AC checked. ## Technical approach @@ -148,8 +148,9 @@ Extend **`EnsureQuestSchemasLoaded`** signature to accept bundle schema paths; v | Target | Coverage | |--------|----------| -| `server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs` | Happy path (repo + temp fixture with bundles); E7M2 presence, freeze, unknown item/skill, missing **`mission_reward`** allowlist; existing E7M1 tests unchanged; host DI resolve. | -| **No new test files** | Extend existing loader test class per NEO-113 precedent. | +| `server/NeonSprawl.Server.Tests/Game/Quests/PrototypeE7M2QuestCatalogRulesTests.cs` | Direct unit tests for **`TryGetCompletionBundleCrossRefError`** (item/skill catalog + allowlist paths). | +| `server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs` | Happy path (repo + temp fixture with bundles); E7M2 presence, freeze, allowlist deny; host DI resolve. | +| **No other new test files** | Loader + rules classes per NEO-113 precedent. | | **Manual verification** | `dotnet test server/NeonSprawl.Server.Tests --filter QuestDefinitionCatalogLoaderTests`; boot dev server and confirm quest catalog Information log after NEO-124 content. | ## Open questions / risks diff --git a/docs/reviews/2026-06-07-NEO-125.md b/docs/reviews/2026-06-07-NEO-125.md index 2dd0883..4ec10ae 100644 --- a/docs/reviews/2026-06-07-NEO-125.md +++ b/docs/reviews/2026-06-07-NEO-125.md @@ -32,14 +32,14 @@ None. ## Suggestions -1. **Rename misleading bundle cross-ref tests** — `Load_ShouldThrow_WhenUnknownItemIdInCompletionRewardBundle` and `Load_ShouldThrow_WhenUnknownSkillIdInCompletionRewardBundle` mutate bundle ids so the **freeze** gate fails first (assertions correctly expect `"completionRewardBundle must match E7M2 freeze table"`). Consider renaming to `…WhenCompletionRewardBundleItemIdDoesNotMatchFreezeTable` / `…SkillIdDoesNotMatchFreezeTable`, or add a focused unit test on **`PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError`** with a matching-freeze bundle to assert the cross-ref message paths (`is not in the frozen prototype item catalog` / skill catalog). +1. ~~**Rename misleading bundle cross-ref tests** — `Load_ShouldThrow_WhenUnknownItemIdInCompletionRewardBundle` and `Load_ShouldThrow_WhenUnknownSkillIdInCompletionRewardBundle` mutate bundle ids so the **freeze** gate fails first (assertions correctly expect `"completionRewardBundle must match E7M2 freeze table"`). Consider renaming to `…WhenCompletionRewardBundleItemIdDoesNotMatchFreezeTable` / `…SkillIdDoesNotMatchFreezeTable`, or add a focused unit test on **`PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError`** with a matching-freeze bundle to assert the cross-ref message paths (`is not in the frozen prototype item catalog` / skill catalog).~~ **Done.** Renamed loader tests; added **`PrototypeE7M2QuestCatalogRulesTests`** with direct cross-ref coverage. -2. **Manual boot smoke (plan §Tests)** — Plan lists booting the dev server and confirming the quest catalog Information log; worth a quick run before merge since NEO-124 content now depends on bundle schema registration at host startup. +2. ~~**Manual boot smoke (plan §Tests)** — Plan lists booting the dev server and confirming the quest catalog Information log; worth a quick run before merge since NEO-124 content now depends on bundle schema registration at host startup.~~ **Done.** Dev server boot logged `Loaded quest catalog from …/content/quests: 4 quest(s) across 1 JSON catalog file(s).` with no startup exception. ## Nits -- Nit: Bundle schema paths use sibling `content/schemas/` resolution only; quest def/step/objective schemas support **`Content:*`** overrides. Plan marked overrides as optional — fine for prototype, but document if custom content layouts need the same override pattern later. -- Nit: In **`TryGetCompletionBundleCrossRefError`**, a skill id present in **`PrototypeSlice1SkillCatalogRules.ExpectedSkillIds`** but missing from **`skillDefsById`** returns the “not in the frozen prototype skill catalog” message; Python’s cross-ref gate would report a **`mission_reward`** allowlist failure instead. Unreachable in production DI (skills load before quests) but slightly divergent for isolated unit tests. +- Nit: Bundle schema paths use sibling `content/schemas/` resolution only; quest def/step/objective schemas support **`Content:*`** overrides. Plan marked overrides as optional — fine for prototype; **`server/README.md`** now documents that bundle schemas have no separate override keys yet. +- ~~Nit: In **`TryGetCompletionBundleCrossRefError`**, a skill id present in **`PrototypeSlice1SkillCatalogRules.ExpectedSkillIds`** but missing from **`skillDefsById`** returns the “not in the frozen prototype skill catalog” message; Python’s cross-ref gate would report a **`mission_reward`** allowlist failure instead. Unreachable in production DI (skills load before quests) but slightly divergent for isolated unit tests.~~ **Done.** Missing loaded skill defs now mirror Python — empty **`allowedXpSourceKinds`** → **`mission_reward`** allowlist error. ## Verification diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/PrototypeE7M2QuestCatalogRulesTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/PrototypeE7M2QuestCatalogRulesTests.cs new file mode 100644 index 0000000..6e0f1b7 --- /dev/null +++ b/server/NeonSprawl.Server.Tests/Game/Quests/PrototypeE7M2QuestCatalogRulesTests.cs @@ -0,0 +1,95 @@ +using NeonSprawl.Server.Game.Encounters; +using NeonSprawl.Server.Game.Quests; +using NeonSprawl.Server.Game.Skills; +using Xunit; + +namespace NeonSprawl.Server.Tests.Game.Quests; + +public class PrototypeE7M2QuestCatalogRulesTests +{ + private static readonly IReadOnlyList EmptySteps = + [new QuestStepDefRow("step", "Step", [])]; + + private static QuestDefRow QuestWithBundle(string questId, QuestRewardBundleRow bundle) => + new(questId, "Test", [], EmptySteps, bundle); + + private static readonly IReadOnlyDictionary ValidSkillDefs = + new Dictionary(StringComparer.Ordinal) + { + ["salvage"] = new("salvage", "gather", "Salvage", ["activity", "mission_reward"]), + ["refine"] = new("refine", "process", "Refine", ["activity", "mission_reward", "trainer"]), + }; + + [Fact] + public void TryGetCompletionBundleCrossRefError_ShouldReturnError_WhenItemIdNotInFrozenCatalog() + { + // Arrange + var rows = new Dictionary(StringComparer.Ordinal) + { + [PrototypeE7M1QuestCatalogRules.ChainQuestId] = QuestWithBundle( + PrototypeE7M1QuestCatalogRules.ChainQuestId, + new QuestRewardBundleRow( + [new RewardGrantRow("unknown_item_id", 1)], + [new QuestSkillXpGrantRow("salvage", 50)])), + }; + // Act + var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError(rows, ValidSkillDefs); + // Assert + Assert.NotNull(error); + Assert.Contains("completionRewardBundle.itemGrants[0]: itemId 'unknown_item_id'", error, StringComparison.Ordinal); + Assert.Contains("is not in the frozen prototype item catalog", error, StringComparison.Ordinal); + } + + [Fact] + public void TryGetCompletionBundleCrossRefError_ShouldReturnError_WhenSkillIdNotInFrozenCatalog() + { + // Arrange + var rows = new Dictionary(StringComparer.Ordinal) + { + [PrototypeE7M1QuestCatalogRules.GatherIntroQuestId] = QuestWithBundle( + PrototypeE7M1QuestCatalogRules.GatherIntroQuestId, + new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("unknown_skill_id", 25)])), + }; + // Act + var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError(rows, ValidSkillDefs); + // Assert + Assert.NotNull(error); + Assert.Contains("completionRewardBundle.skillXpGrants[0]: skillId 'unknown_skill_id'", error, StringComparison.Ordinal); + Assert.Contains("is not in the frozen prototype skill catalog", error, StringComparison.Ordinal); + } + + [Fact] + public void TryGetCompletionBundleCrossRefError_ShouldReturnError_WhenSkillMissingFromLoadedDefs() + { + // Arrange + var rows = new Dictionary(StringComparer.Ordinal) + { + [PrototypeE7M1QuestCatalogRules.GatherIntroQuestId] = QuestWithBundle( + PrototypeE7M1QuestCatalogRules.GatherIntroQuestId, + new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)])), + }; + // Act + var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError( + rows, + new Dictionary(StringComparer.Ordinal)); + // Assert + Assert.NotNull(error); + Assert.Contains("must allow sourceKind 'mission_reward' in allowedXpSourceKinds (got [])", error, StringComparison.Ordinal); + } + + [Fact] + public void TryGetCompletionBundleCrossRefError_ShouldReturnNull_WhenBundlesMatchFreezeAndAllowlists() + { + // Arrange + var rows = new Dictionary(StringComparer.Ordinal); + foreach (var (qid, bundle) in PrototypeE7M2QuestCatalogRules.ExpectedCompletionBundles) + { + rows[qid] = QuestWithBundle(qid, bundle); + } + + // Act + var error = PrototypeE7M2QuestCatalogRules.TryGetCompletionBundleCrossRefError(rows, ValidSkillDefs); + // Assert + Assert.Null(error); + } +} diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs index d74eafc..37ead2a 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestDefinitionCatalogLoaderTests.cs @@ -645,7 +645,7 @@ public class QuestDefinitionCatalogLoaderTests } [Fact] - public void Load_ShouldThrow_WhenUnknownItemIdInCompletionRewardBundle() + public void Load_ShouldThrow_WhenCompletionRewardBundleItemIdDoesNotMatchFreezeTable() { // Arrange var (_, questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath) = CreateTempContentLayout(); @@ -667,7 +667,7 @@ public class QuestDefinitionCatalogLoaderTests } [Fact] - public void Load_ShouldThrow_WhenUnknownSkillIdInCompletionRewardBundle() + public void Load_ShouldThrow_WhenCompletionRewardBundleSkillIdDoesNotMatchFreezeTable() { // Arrange var (_, questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath) = CreateTempContentLayout(); diff --git a/server/NeonSprawl.Server/Game/Quests/PrototypeE7M2QuestCatalogRules.cs b/server/NeonSprawl.Server/Game/Quests/PrototypeE7M2QuestCatalogRules.cs index ad74153..959425f 100644 --- a/server/NeonSprawl.Server/Game/Quests/PrototypeE7M2QuestCatalogRules.cs +++ b/server/NeonSprawl.Server/Game/Quests/PrototypeE7M2QuestCatalogRules.cs @@ -103,17 +103,13 @@ public static class PrototypeE7M2QuestCatalogRules $"'{grant.SkillId}' is not in the frozen prototype skill catalog"; } - if (!skillDefsById.TryGetValue(grant.SkillId, out var skill)) - { - return - $"error: quest '{qid}' completionRewardBundle.skillXpGrants[{gi}]: skillId " + - $"'{grant.SkillId}' is not in the frozen prototype skill catalog"; - } - - var allowedKinds = skill.AllowedXpSourceKinds; + skillDefsById.TryGetValue(grant.SkillId, out var skill); + var allowedKinds = skill?.AllowedXpSourceKinds ?? []; if (!allowedKinds.Contains(MissionRewardSourceKind, StringComparer.Ordinal)) { - var kindsDisplay = $"[{string.Join(", ", allowedKinds.Select(k => $"'{k}'"))}]"; + var kindsDisplay = allowedKinds.Count == 0 + ? "[]" + : $"[{string.Join(", ", allowedKinds.Select(k => $"'{k}'"))}]"; return $"error: quest '{qid}' completionRewardBundle.skillXpGrants[{gi}]: skillId " + $"'{grant.SkillId}' must allow sourceKind '{MissionRewardSourceKind}' in allowedXpSourceKinds " + diff --git a/server/README.md b/server/README.md index 5d82864..179a294 100644 --- a/server/README.md +++ b/server/README.md @@ -144,6 +144,8 @@ On startup the host loads every **`*_quests.json`** under the quests directory * | **`Content:QuestStepDefSchemaPath`** | Optional override for **`quest-step-def.schema.json`**. When unset, **`{parent of quests directory}/schemas/quest-step-def.schema.json`**. | | **`Content:QuestObjectiveDefSchemaPath`** | Optional override for **`quest-objective-def.schema.json`**. When unset, **`{parent of quests directory}/schemas/quest-objective-def.schema.json`**. | +Bundle-related schemas (**`quest-reward-bundle.schema.json`**, **`quest-skill-xp-grant.schema.json`**, **`reward-grant-row.schema.json`**) resolve as siblings under **`{parent of quests directory}/schemas/`** only — there are no separate **`Content:*`** override keys yet; custom layouts must keep those three files next to the quest schemas (NEO-125). + **Docker / CI:** include **`content/quests`**, upstream catalogs (**`content/items`**, **`content/recipes`**, **`content/encounters`**, **`content/skills`**), and quest/bundle schemas in the mounted **`content/`** tree; set **`Content__QuestsDirectory`** when layout differs. On success, **Information** logs include the resolved quests directory path, distinct quest count, and catalog file count. Game code should use **`IQuestDefinitionRegistry`** for lookups (`TryGetDefinition`, `TryNormalizeKnown`, `GetDefinitionsInIdOrder`; NEO-114). The catalog singleton remains for fail-fast startup only; do not inject **`QuestDefinitionCatalog`** in new game code. **`TryGetDefinition`** is case-sensitive on catalog keys; HTTP and game callers validating wire ids should use **`TryNormalizeKnown`** (trim + lowercase + fail-closed lookup), same as encounter/ability routes.