69 lines
5.1 KiB
Markdown
69 lines
5.1 KiB
Markdown
# Code review — NEO-112 (E7M1-01)
|
||
|
||
**Date:** 2026-05-31
|
||
**Scope:** Branch `NEO-112-e7m1-prototype-questdef-catalog-schemas-ci` vs `47105cc` (merge-base on `main`) — commits `be1dae3` … `765075f`
|
||
**Base:** `47105cc` (main at branch point, post NEO-111 merge)
|
||
|
||
**Follow-up:** Suggestions and the prerequisites nit below are **done** (strikethrough + **Done.**).
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-112 delivers the Epic 7 Slice 1 **content foundation**: three JSON Schemas (`quest-def`, `quest-step-def`, `quest-objective-def`), a four-row `prototype_quests.json` catalog aligned to the E7.M1 freeze table, and ~300 lines of CI gates in `validate_content.py` (four-id allowlist, acyclic prerequisites, item/recipe/encounter cross-refs, operator-chain terminal token gate, duplicate quest/step/objective ids). Documentation is thorough—implementation plan reconciliation, CT.M1 paragraph, `content/README.md`, alignment register, and the full E7M1 prototype backlog land together. No server, C#, or Godot changes (correct for E7M1-01). Risk is low: pattern mirrors NEO-100/NEO-87; happy-path validation passes locally.
|
||
|
||
## Documentation checked
|
||
|
||
| Path | Result |
|
||
|------|--------|
|
||
| `docs/plans/NEO-112-implementation-plan.md` | **Matches** — acceptance checklist checked; reconciliation section accurate. |
|
||
| `docs/plans/E7M1-prototype-backlog.md` (E7M1-01) | **Matches** — AC checked; scope/out-of-scope correct; client counterpart none. |
|
||
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — freeze table, CI enforcement line, and Status note NEO-112 catalog landed / runtime from NEO-113. |
|
||
| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **Matches** — quest catalog PR gate paragraph describes shipped validators. |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-112 catalog; register stays Planned until NEO-113. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note references NEO-112 catalog landed; register **Planned** until NEO-113 server load. |
|
||
| `content/README.md` | **Matches** — `quests/` table row + E7M1 prototype paragraph. |
|
||
| `docs/decomposition/epics/epic_07_quest_faction.md` | **Matches** — Slice 1 backlog link + Godot DoD clarification. |
|
||
| Full-stack epic decomposition | **N/A** — infrastructure-only story; no client counterpart required until NEO-122/NEO-123. |
|
||
|
||
## Blocking issues
|
||
|
||
(none)
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Refresh `module_dependency_register.md` E7.M1 note** — Change “Register row stays **Planned** until **NEO-112** lands” to “until **NEO-113** server load lands” so it matches `documentation_and_implementation_alignment.md` after this merge.~~ **Done.** — `module_dependency_register.md` E7.M1 note updated.
|
||
|
||
2. ~~**Clarify E7.M1 module Status line** — Replace “implementation not started” with wording that E7M1-01 catalog (**NEO-112**) is landed and runtime work starts at **NEO-113**.~~ **Done.** — `E7_M1_QuestStateMachine.md` Status row updated.
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: E7M1 kickoff table row “Onboarding order: gather → refine → combat → chain” oversimplifies prerequisites — **`prototype_quest_combat_intro`** has no prereq (parallel path); freeze table and catalog are correct.~~ **Done.** — `E7M1-prototype-backlog.md` kickoff Prerequisites row clarified.
|
||
|
||
- Nit: `quest-objective-def.schema.json` lists all kind-specific fields in `properties` plus `oneOf` branches, so schema validation may accept wrong-field-at-wrong-kind (e.g. `recipeId` on `encounter_complete`) if both are present — plan adopted this tradeoff; CI cross-ref gate still catches unknown ids.
|
||
|
||
- ~~Nit: Duplicate step/objective ids are enforced **within** a quest only, not globally across quests — acceptable for prototype; E7M1-04 HTTP projection should namespace by quest id.~~ **Superseded (Bugbot):** objective duplicate check was incorrectly scoped per-step; fixed in `validate_content.py` to quest-level (step ids remain per-quest; objective ids now per-quest too). Cross-quest uniqueness still not enforced (acceptable for prototype).
|
||
|
||
- Nit: Branch bundles the full E7M1 backlog decomposition (~390 lines) with E7M1-01 implementation — large diff but intentional and documented.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# Happy path (primary regression signal — also runs in pr-gate.yml)
|
||
python3 -m pip install -r scripts/requirements-content.txt
|
||
python3 scripts/validate_content.py
|
||
|
||
# Manual negative cases (from NEO-112 plan)
|
||
# 1. Duplicate quest id across or within files → non-zero exit
|
||
# 2. Remove a frozen quest id from catalog → four-id gate error
|
||
# 3. Unknown itemId/recipeId/encounterId in objective → cross-ref error
|
||
# 4. Cyclic prerequisiteQuestIds → prerequisite gate error
|
||
# 5. Unknown prerequisite id → prerequisite gate error
|
||
# 6. Chain terminal step not inventory_has_item contract_handoff_token ×1 → chain gate error
|
||
# 7. Invalid kind or missing required field per kind → schema error
|
||
```
|
||
|
||
**Reviewer note:** Happy path executed successfully in review environment (`content OK` … `4 unique quest id(s)`).
|