NEO-125: add code review for quest bundle catalog load
parent
8d026bdca4
commit
ad80ed8760
|
|
@ -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.
|
||||
Loading…
Reference in New Issue