From ad80ed8760af1947865a279b3128650a176cbbf8 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 15:26:51 -0400 Subject: [PATCH] NEO-125: add code review for quest bundle catalog load --- docs/reviews/2026-06-07-NEO-125.md | 54 ++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 docs/reviews/2026-06-07-NEO-125.md diff --git a/docs/reviews/2026-06-07-NEO-125.md b/docs/reviews/2026-06-07-NEO-125.md new file mode 100644 index 0000000..2dd0883 --- /dev/null +++ b/docs/reviews/2026-06-07-NEO-125.md @@ -0,0 +1,54 @@ +# Code review — NEO-125 (E7M2-02) + +**Date:** 2026-06-07 +**Scope:** Branch `NEO-125-e7m2-server-quest-catalog-reward-bundle-validation` vs `origin/main` — commits `47ddde6` … `8d026bd` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** — implementation matches plan and CI parity; tests pass; minor test-naming/coverage notes only. + +## Summary + +NEO-125 closes the server-side gap left by NEO-124: the quest catalog loader now registers bundle-related JSON Schemas (`reward-grant-row`, `quest-skill-xp-grant`, `quest-reward-bundle`), parses **`completionRewardBundle`** onto **`QuestDefRow`**, and runs **`PrototypeE7M2QuestCatalogRules`** (presence → freeze → cross-ref + **`mission_reward`** allowlist) at startup — mirroring **`scripts/validate_content.py`**. DI injects **`SkillDefinitionCatalog`** so skill allowlist checks use loaded defs. Five new loader tests extend the NEO-113 fixture pattern; host boot via **`WebApplicationFactory`** still resolves the catalog. Risk is low: infrastructure-only, fail-fast at startup, no HTTP or delivery changes. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-125-implementation-plan.md` | **Matches** — kickoff decisions, acceptance criteria checked, reconciliation accurate. | +| `docs/plans/E7M2-prototype-backlog.md` (E7M2-02) | **Matches** — AC checkboxes complete; infrastructure-only client counterpart noted. | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — server load paragraph added; freeze table unchanged. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes NEO-125 server load; row stays **Planned** until NEO-132 capstone. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M2 note references NEO-125; register row correctly **Planned**. | +| `server/README.md` | **Matches** — quest catalog section documents E7M2 gates, bundle schemas, skill catalog dependency. | +| Full-stack epic decomposition | **N/A** — E7M2-02 is infrastructure-only; no client counterpart required. | + +**Register / tracking:** E7.M2 correctly remains **Planned** until capstone NEO-132; alignment updates are appropriate. + +## Blocking issues + +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). + +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. + +## 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. + +## Verification + +```bash +dotnet test server/NeonSprawl.Server.Tests --filter QuestDefinitionCatalogLoaderTests +python3 scripts/validate_content.py +``` + +Optional before merge: + +- Boot dev server; confirm Information log for quest catalog load (4 quests) with no startup exception. +- If renaming tests (Suggestion **1**), re-run the filter above.