52 lines
4.2 KiB
Markdown
52 lines
4.2 KiB
Markdown
# Code review — NEO-33 (prototype SkillDef trio + CI catalog contract)
|
|
|
|
**Date:** 2026-05-02
|
|
|
|
**Scope:** Branch `NEO-33-e2m1-lock-prototype-skilldef-trio-ci-catalog-contract` vs `origin/main` (commits `66150ea`, `583201c`: implementation plan, `validate_content.py` Slice 1 gate, E2.M1 / CT.M1 / alignment / register / `content/README` / `skills.md`).
|
|
|
|
**Base:** `origin/main` at merge-base `807812148ffa445d3628e6fed1337d53790c3c77`.
|
|
|
|
## Verdict
|
|
|
|
**Approve with nits**
|
|
|
|
## Summary
|
|
|
|
The branch implements NEO-33 as documented: a post-schema **prototype Slice 1** gate in `scripts/validate_content.py` enforces the frozen id set (`salvage`, `refine`, `intrusion`) and category coverage (gather, tech, process or make), with decomposition and authoring docs updated so designers see `allowedXpSourceKinds` semantics and the freeze table. `prototype_skills.json` on `main` already matched the trio; no catalog diff is expected. Risk is low (CI + Python script only); one small validator behavior affects error messaging in an edge case, not happy-path correctness.
|
|
|
|
## Documentation checked
|
|
|
|
| Document | Result |
|
|
|----------|--------|
|
|
| `docs/plans/NEO-33-implementation-plan.md` | **Matches** — plan, shipped notes, acceptance checklist, and “Files to modify” align with the diff (catalog unchanged on branch because it was already correct; gate + docs carry the story). |
|
|
| `docs/decomposition/modules/E2_M1_SkillDefinitionRegistry.md` | **Matches** — Status **In Progress**, designer note, freeze table, CI pointer, `allowedXpSourceKinds` table + narrative; schema link remains in **Data contract** above the new subsection. |
|
|
| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **Matches** — CI paragraph describes duplicate-id check and NEO-33 trio + category enforcement. |
|
|
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E2.M1 **In Progress** and E2.M1 note describe NEO-33 → NEO-34+ flow. |
|
|
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E2.M1 row documents NEO-33 landed state and follow-on issues. |
|
|
| `docs/game-design/skills.md` | **Matches** — Prototype trio bullet points at frozen ids and E2.M1 / catalog anchors. |
|
|
| `content/README.md` | **Matches** — Brief Slice 1 pointer and anchor to E2.M1 designer section. |
|
|
| `docs/decomposition/modules/contracts.md` | **N/A** — no new contract kind beyond existing JSON Schema + CI story; consistent with CT.M1 wording. |
|
|
| `docs/decomposition/modules/client_server_authority.md` | **N/A** — no runtime authority change. |
|
|
|
|
**Register / tracking:** Branch already updates E2.M1 to **In Progress** and the alignment row; no further register change required for merge.
|
|
|
|
## Blocking issues
|
|
|
|
(none)
|
|
|
|
## Suggestions
|
|
|
|
1. ~~**`validate_content.py` — reserve `seen_ids` only for schema-clean rows** — Today, a string `id` is inserted into `seen_ids` even when `row_schema_errors > 0`, while `id_to_category` is gated on `row_schema_errors == 0`. If an author fixes a broken row and reuses an `id` that appeared only on an invalid earlier row, the validator can report **duplicate skill id** instead of focusing on the remaining schema errors. Consider only updating `seen_ids` when `row_schema_errors == 0` (mirror the `id_to_category` guard) so duplicate detection applies to validated rows only.~~ **Done.**
|
|
|
|
2. ~~**E2.M1 acceptance criteria (optional doc polish)** — The bullet that says “when Slice 1 locks three skills” could name **`salvage` / `refine` / `intrusion`** explicitly now that NEO-33 shipped, so the acceptance section matches the freeze subsection without reading both.~~ **Done.**
|
|
|
|
## Nits
|
|
|
|
- **Nit:** Ensure the PR description mentions that **`content/skills/prototype_skills.json`** was already compliant on `main` so reviewers do not hunt for a missing catalog diff.
|
|
|
|
## Verification
|
|
|
|
- `pip install -r scripts/requirements-content.txt && python3 scripts/validate_content.py` (expect `content OK` and exit code 0).
|
|
- Confirm **PR gate** on the PR runs the same script (`.github/workflows/pr-gate.yml` already invokes `python scripts/validate_content.py`).
|
|
- Optional: add a second skills JSON under `content/skills/` in a throwaway branch to confirm duplicate-id and “exactly three ids” failures read clearly in CI logs.
|