don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-06-07-NEO-125.md

4.6 KiB
Raw Permalink Blame History

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). 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. 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; 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

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.