NEO-113: address code review — docs, schema and chain terminal tests.
parent
38310d6ed3
commit
9c2ab5ae4d
|
|
@ -7,7 +7,7 @@
|
||||||
| **Module ID** | E7.M1 |
|
| **Module ID** | E7.M1 |
|
||||||
| **Epic** | [Epic 7 — Quest / Faction](../epics/epic_07_quest_faction.md) |
|
| **Epic** | [Epic 7 — Quest / Faction](../epics/epic_07_quest_faction.md) |
|
||||||
| **Stage target** | Prototype |
|
| **Stage target** | Prototype |
|
||||||
| **Status** | Planned — Slice 1 backlog [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md): **E7M1-01** [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) catalog **landed** (schemas + CI); runtime from **E7M1-02** [NEO-113](https://linear.app/neon-sprawl/issue/NEO-113) → capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123) |
|
| **Status** | Planned — Slice 1 backlog [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md): **E7M1-01** [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) catalog **landed** (schemas + CI); **E7M1-02** [NEO-113](https://linear.app/neon-sprawl/issue/NEO-113) server load **landed**; registry + runtime from **E7M1-03** [NEO-114](https://linear.app/neon-sprawl/issue/NEO-114) → capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123) |
|
||||||
| **Linear** | Label **`E7.M1`** · [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md) |
|
| **Linear** | Label **`E7.M1`** · [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md) |
|
||||||
|
|
||||||
## Purpose
|
## Purpose
|
||||||
|
|
@ -78,6 +78,8 @@ The **first shipped quest spine** is **frozen** for prototype tuning until a del
|
||||||
|
|
||||||
**CI enforcement (NEO-112):** `scripts/validate_content.py` requires **exactly** these four quest ids; objective cross-refs to frozen item/recipe/encounter catalogs; acyclic **`prerequisiteQuestIds`**; chain quest terminal step uses **`contract_handoff_token`** from [E5.M3 encounter loot](E5_M3_EncounterAndRewardTables.md#prototype-slice-3-freeze-e5m3-01).
|
**CI enforcement (NEO-112):** `scripts/validate_content.py` requires **exactly** these four quest ids; objective cross-refs to frozen item/recipe/encounter catalogs; acyclic **`prerequisiteQuestIds`**; chain quest terminal step uses **`contract_handoff_token`** from [E5.M3 encounter loot](E5_M3_EncounterAndRewardTables.md#prototype-slice-3-freeze-e5m3-01).
|
||||||
|
|
||||||
|
**Server load (NEO-113):** Host fail-fast load of `content/quests/*_quests.json` with the same gates — [server README — Quest catalog](../../../server/README.md#quest-catalog-contentquests-neo-113).
|
||||||
|
|
||||||
**Reward policy (Slice 1):** Quest completion updates **`QuestStepState` only** — no duplicate item/XP grants. Gather/craft/encounter paths keep existing payouts; [E7.M2](E7_M2_RewardAndUnlockRouter.md) adds **`QuestRewardBundle`** apply in Slice 2.
|
**Reward policy (Slice 1):** Quest completion updates **`QuestStepState` only** — no duplicate item/XP grants. Gather/craft/encounter paths keep existing payouts; [E7.M2](E7_M2_RewardAndUnlockRouter.md) adds **`QuestRewardBundle`** apply in Slice 2.
|
||||||
|
|
||||||
## Risks and telemetry
|
## Risks and telemetry
|
||||||
|
|
|
||||||
|
|
@ -51,8 +51,9 @@
|
||||||
- **Catalog:** `QuestDefinitionCatalog` singleton; eager resolve in `Program.cs` after encounter catalogs.
|
- **Catalog:** `QuestDefinitionCatalog` singleton; eager resolve in `Program.cs` after encounter catalogs.
|
||||||
- **DI:** `AddQuestDefinitionCatalog` — loads after item, recipe, and encounter catalogs for cross-refs.
|
- **DI:** `AddQuestDefinitionCatalog` — loads after item, recipe, and encounter catalogs for cross-refs.
|
||||||
- **Config:** `ContentPathsOptions` quest path keys; `InMemoryWebApplicationFactory` pins repo `content/quests`.
|
- **Config:** `ContentPathsOptions` quest path keys; `InMemoryWebApplicationFactory` pins repo `content/quests`.
|
||||||
- **Tests:** 18 AAA tests in `QuestDefinitionCatalogLoaderTests` (loader + host boot).
|
- **Tests:** 20 AAA tests in `QuestDefinitionCatalogLoaderTests` (loader + host boot, incl. row schema violation + chain terminal quantity).
|
||||||
- **Docs:** `server/README.md` quest catalog section; alignment register E7.M1 row updated.
|
- **Docs:** `server/README.md` quest catalog section; alignment register E7.M1 row updated; `E7_M1_QuestStateMachine.md` Status + server-load line.
|
||||||
|
- **Bruno:** `bruno/neon-sprawl-server/quest-catalog/Health after quest catalog load.bru` (boot smoke; no quest HTTP in this story).
|
||||||
|
|
||||||
## Technical approach
|
## Technical approach
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -18,7 +18,7 @@ NEO-113 delivers fail-fast server startup loading for `content/quests/*_quests.j
|
||||||
|------|--------|
|
|------|--------|
|
||||||
| `docs/plans/NEO-113-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. |
|
| `docs/plans/NEO-113-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. |
|
||||||
| `docs/plans/E7M1-prototype-backlog.md` (E7M1-02) | **Matches** — AC checked; client counterpart none. |
|
| `docs/plans/E7M1-prototype-backlog.md` (E7M1-02) | **Matches** — AC checked; client counterpart none. |
|
||||||
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Partially matches** — freeze table and CI enforcement line correct; **Status** still reads “runtime from E7M1-02 NEO-113” without noting server load **landed** (see Suggestion 1). |
|
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — freeze table, CI enforcement + server-load line, Status notes NEO-113 landed. |
|
||||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-113 server load; register stays Planned until NEO-114. |
|
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-113 server load; register stays Planned until NEO-114. |
|
||||||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note updated for NEO-113 server load. |
|
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note updated for NEO-113 server load. |
|
||||||
| `server/README.md` | **Matches** — quest catalog section (config keys, discovery, fail-fast, load order, NEO-114 forward reference). |
|
| `server/README.md` | **Matches** — quest catalog section (config keys, discovery, fail-fast, load order, NEO-114 forward reference). |
|
||||||
|
|
@ -30,15 +30,15 @@ NEO-113 delivers fail-fast server startup loading for `content/quests/*_quests.j
|
||||||
|
|
||||||
## Suggestions
|
## Suggestions
|
||||||
|
|
||||||
1. **Refresh `E7_M1_QuestStateMachine.md` Status line** — After merge, note that **E7M1-02 / NEO-113** server load is **landed** and runtime/registry work continues from **NEO-114** (mirror the alignment register wording). Optionally add a one-line “server load (NEO-113)” bullet under **CI enforcement** pointing at `server/README.md#quest-catalog-contentquests-neo-113`.
|
1. ~~**Refresh `E7_M1_QuestStateMachine.md` Status line** — After merge, note that **E7M1-02 / NEO-113** server load is **landed** and runtime/registry work continues from **NEO-114** (mirror the alignment register wording). Optionally add a one-line “server load (NEO-113)” bullet under **CI enforcement** pointing at `server/README.md#quest-catalog-contentquests-neo-113`.~~ **Done.**
|
||||||
|
|
||||||
2. **Add `Load_ShouldThrow_WhenRowViolatesSchema` test** — The implementation plan and NEO-101 precedent (`EncounterDefinitionCatalogLoaderTests.Load_ShouldThrow_WhenRowViolatesSchema`) call for a row-level schema failure case (e.g. empty `displayName` or missing required objective field). Other negative paths are well covered; this is the main gap vs the plan’s test table.
|
2. ~~**Add `Load_ShouldThrow_WhenRowViolatesSchema` test** — The implementation plan and NEO-101 precedent (`EncounterDefinitionCatalogLoaderTests.Load_ShouldThrow_WhenRowViolatesSchema`) call for a row-level schema failure case (e.g. empty `displayName` or missing required objective field). Other negative paths are well covered; this is the main gap vs the plan’s test table.~~ **Done.**
|
||||||
|
|
||||||
3. **Chain terminal quantity negative test** — Plan lists “wrong chain terminal objective (kind / item / quantity)”; kind and item are covered; adding a quantity ≠ 1 case would close the loop on `TryGetChainTerminalGateError`.
|
3. ~~**Chain terminal quantity negative test** — Plan lists “wrong chain terminal objective (kind / item / quantity)”; kind and item are covered; adding a quantity ≠ 1 case would close the loop on `TryGetChainTerminalGateError`.~~ **Done.**
|
||||||
|
|
||||||
## Nits
|
## Nits
|
||||||
|
|
||||||
- Nit: Bruno `bruno/neon-sprawl-server/quest-catalog/` is not listed in the implementation plan file table — harmless smoke; consider a one-line mention in plan reconciliation if you want docs to mirror NEO-103/NEO-78 Bruno folders.
|
- ~~Nit: Bruno `bruno/neon-sprawl-server/quest-catalog/` is not listed in the implementation plan file table — harmless smoke; consider a one-line mention in plan reconciliation if you want docs to mirror NEO-103/NEO-78 Bruno folders.~~ **Done.** — plan reconciliation lists Bruno boot smoke.
|
||||||
|
|
||||||
- Nit: E7M1 four-id gate error strings differ slightly from Python (`'id'` quoted list vs `repr` list) — acceptable; both are actionable.
|
- Nit: E7M1 four-id gate error strings differ slightly from Python (`'id'` quoted list vs `repr` list) — acceptable; both are actionable.
|
||||||
|
|
||||||
|
|
@ -47,7 +47,7 @@ NEO-113 delivers fail-fast server startup loading for `content/quests/*_quests.j
|
||||||
## Verification
|
## Verification
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
# Quest catalog tests (18 — primary signal for this story)
|
# Quest catalog tests (20 — primary signal for this story)
|
||||||
cd server
|
cd server
|
||||||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
|
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
|
||||||
--filter "FullyQualifiedName~QuestDefinitionCatalogLoaderTests"
|
--filter "FullyQualifiedName~QuestDefinitionCatalogLoaderTests"
|
||||||
|
|
@ -60,4 +60,4 @@ python3 -m pip install -r scripts/requirements-content.txt
|
||||||
python3 scripts/validate_content.py
|
python3 scripts/validate_content.py
|
||||||
```
|
```
|
||||||
|
|
||||||
**Reviewer note:** All 18 `QuestDefinitionCatalogLoaderTests` passed locally. Full server suite: 602 passed; 12 Postgres/docker integration tests skipped/failed in this environment (no Docker daemon) — unrelated to NEO-113.
|
**Reviewer note:** All 20 `QuestDefinitionCatalogLoaderTests` passed locally after review follow-up. Full server suite: 602 passed; 12 Postgres/docker integration tests skipped/failed in this environment (no Docker daemon) — unrelated to NEO-113.
|
||||||
|
|
|
||||||
|
|
@ -264,6 +264,46 @@ public class QuestDefinitionCatalogLoaderTests
|
||||||
Assert.Contains("expected schemaVersion 1", ioe.Message, StringComparison.Ordinal);
|
Assert.Contains("expected schemaVersion 1", ioe.Message, StringComparison.Ordinal);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Load_ShouldThrow_WhenRowViolatesSchema()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
var (_, questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath) = CreateTempContentLayout();
|
||||||
|
const string bad = """
|
||||||
|
{
|
||||||
|
"schemaVersion": 1,
|
||||||
|
"quests": [
|
||||||
|
{
|
||||||
|
"id": "prototype_quest_gather_intro",
|
||||||
|
"displayName": "",
|
||||||
|
"prerequisiteQuestIds": [],
|
||||||
|
"steps": [
|
||||||
|
{
|
||||||
|
"id": "gather_intro_step_salvage",
|
||||||
|
"displayName": "Gather scrap metal",
|
||||||
|
"objectives": [
|
||||||
|
{
|
||||||
|
"id": "gather_intro_obj_scrap",
|
||||||
|
"kind": "gather_item",
|
||||||
|
"itemId": "scrap_metal_bulk",
|
||||||
|
"quantity": 3
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
|
""";
|
||||||
|
File.WriteAllText(Path.Combine(questsDir, "bad_quests.json"), bad, Encoding.UTF8);
|
||||||
|
// Act
|
||||||
|
var ex = Record.Exception(() => LoadCatalog(questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath));
|
||||||
|
// Assert
|
||||||
|
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||||
|
Assert.Contains("bad_quests.json", ioe.Message, StringComparison.Ordinal);
|
||||||
|
Assert.Contains("Quest catalog validation failed", ioe.Message, StringComparison.Ordinal);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Load_ShouldThrow_WhenDuplicateQuestIdAcrossFiles()
|
public void Load_ShouldThrow_WhenDuplicateQuestIdAcrossFiles()
|
||||||
{
|
{
|
||||||
|
|
@ -506,6 +546,27 @@ public class QuestDefinitionCatalogLoaderTests
|
||||||
Assert.Contains("terminal itemId must be 'contract_handoff_token'", ioe.Message, StringComparison.Ordinal);
|
Assert.Contains("terminal itemId must be 'contract_handoff_token'", ioe.Message, StringComparison.Ordinal);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Load_ShouldThrow_WhenChainTerminalQuantityIsWrong()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
var (_, questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath) = CreateTempContentLayout();
|
||||||
|
var root = JsonNode.Parse(ValidPrototypeCatalogJson) as JsonObject
|
||||||
|
?? throw new InvalidOperationException("expected object");
|
||||||
|
var chain = GetQuestRow(root, "prototype_quest_operator_chain");
|
||||||
|
var steps = chain["steps"] as JsonArray ?? throw new InvalidOperationException("expected steps");
|
||||||
|
var lastStep = steps[^1] as JsonObject ?? throw new InvalidOperationException("expected step");
|
||||||
|
var objectives = lastStep["objectives"] as JsonArray ?? throw new InvalidOperationException("expected objectives");
|
||||||
|
var obj = objectives[0] as JsonObject ?? throw new InvalidOperationException("expected objective");
|
||||||
|
obj["quantity"] = 2;
|
||||||
|
WriteCatalog(questsDir, root.ToJsonString());
|
||||||
|
// Act
|
||||||
|
var ex = Record.Exception(() => LoadCatalog(questsDir, defSchemaPath, stepSchemaPath, objectiveSchemaPath));
|
||||||
|
// Assert
|
||||||
|
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||||
|
Assert.Contains("terminal quantity must be 1", ioe.Message, StringComparison.Ordinal);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void Load_ShouldThrow_WhenMissingSchemaFile()
|
public void Load_ShouldThrow_WhenMissingSchemaFile()
|
||||||
{
|
{
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue