61 lines
5.7 KiB
Markdown
61 lines
5.7 KiB
Markdown
# Code review — NEO-76 prototype AbilityDef catalog + CI
|
||
|
||
**Date:** 2026-05-24
|
||
**Scope:** Branch `NEO-76-e5m1-prototype-backlog` · commits `510d711`–`9b11a52` vs `origin/main`
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Request changes** — schema, `validate_content.py` gates, and docs align with the adopted plan, but the shipped catalog contains a duplicate `prototype_pulse` row and **PR gate validation fails**. Fix the catalog (or remove the stray row) before merge.
|
||
|
||
## Summary
|
||
|
||
The branch decomposes Epic 5 Slice 1 into the E5M1 backlog (NEO-76–NEO-86, NEO-44), adds kickoff + implementation plans, and lands content-only E5M1-01: `ability-def.schema.json`, `prototype_abilities.json`, and ability validation in `scripts/validate_content.py` following NEO-50/NEO-65 patterns. Documentation updates (E5.M1 freeze table, `content/README.md`, CT.M1, alignment register, epic backlog link) are thorough and match kickoff decisions. No server/C#/client code — appropriate for this story. Risk is low once the catalog is corrected; the duplicate-id gate works as designed and currently blocks CI.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-76-implementation-plan.md`](../plans/NEO-76-implementation-plan.md) | **Partially matches** — kickoff table, schema shape, CI constant name, and file list match; reconciliation marks AC complete but catalog currently fails duplicate-id validation. |
|
||
| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-01** | **Partially matches** — E5M1-01 AC checked and landed note present; content file must pass CI before merge. |
|
||
| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Partially matches** — freeze table, Linear slug table, and CI rules match plan; module **Status** row still **Planned** while alignment register says **In Progress**. |
|
||
| [`docs/decomposition/modules/CT_M1_ContentValidationPipeline.md`](../decomposition/modules/CT_M1_ContentValidationPipeline.md) | **Matches** — ability catalog PR gate paragraph added. |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E5.M1 row added **In Progress** / NEO-76 landed. Register update appropriate when work starts. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E5.M1 Linear note content is correct but inserted **inside** the Epic 5 table, breaking markdown table rendering. |
|
||
| [`docs/decomposition/epics/epic_05_pve_combat.md`](../decomposition/epics/epic_05_pve_combat.md) | **Matches** — Slice 1 backlog link to E5M1-prototype-backlog.md. |
|
||
| [`content/README.md`](../../content/README.md) | **Matches** — `abilities/` row + E5 Slice 1 freeze paragraph. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — no runtime authority changes; content-only. |
|
||
| Full-stack epic decomposition | **Matches** — E5M1-01 is **server/content** only; client counterparts **NEO-85** / **NEO-86** documented in backlog (not required for this issue). |
|
||
|
||
## Blocking issues
|
||
|
||
1. **Duplicate `prototype_pulse` in `content/abilities/prototype_abilities.json`** — The catalog has **five** rows; `prototype_pulse` appears twice (lines 4–9 and 32–37). Running `python scripts/validate_content.py` exits non-zero with `duplicate ability id 'prototype_pulse'`. This violates E5M1-01 AC (“exactly four prototype ability ids; duplicate `id` fails CI”) and will fail `.github/workflows/pr-gate.yml`. Remove the trailing duplicate row so the file contains exactly the four frozen ids.
|
||
|
||
## Suggestions
|
||
|
||
1. **Fix `module_dependency_register.md` Epic 5 table layout** — The **E5.M1 note** paragraph sits between the E5.M1 and E5.M2 table rows, which breaks the markdown table. Move the note below the Epic 5 table (same pattern as other epic footnotes) so E5.M2–E5.M4 rows render in the table.
|
||
|
||
2. **Align E5.M1 module Status with alignment register** — [`documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) lists E5.M1 as **In Progress**; [`E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) summary **Status** still reads **Planned**. Mirror the E3.M* pattern (module page status note when catalog lands).
|
||
|
||
## Nits
|
||
|
||
- Nit: Duplicate-id error message cites the same file twice (`… in content/abilities/prototype_abilities.json and content/abilities/prototype_abilities.json`) — acceptable; same-file duplicates are rare in normal authoring.
|
||
- Nit: [`NEO-76-implementation-plan.md`](../plans/NEO-76-implementation-plan.md) reconciliation and backlog checkboxes are marked complete while CI currently fails — update after catalog fix or note “pending catalog fix” until green.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# From repo root (venv recommended)
|
||
python3 -m venv .venv-content && .venv-content/bin/pip install -r scripts/requirements-content.txt
|
||
.venv-content/bin/python scripts/validate_content.py
|
||
# Expect: content OK … 1 ability catalog file(s) … 4 unique ability id(s)
|
||
|
||
# Negative: duplicate id (should fail — already present until fixed)
|
||
# Remove duplicate row first, then re-run happy path above.
|
||
|
||
# CI parity
|
||
# pr-gate.yml runs the same script after pip install -r scripts/requirements-content.txt
|
||
```
|
||
|
||
After fixing the catalog: confirm four unique ids match `PROTOTYPE_E5M1_ABILITY_IDS` and [`PrototypeAbilityRegistry`](../../server/NeonSprawl.Server/Game/AbilityInput/PrototypeAbilityRegistry.cs) allowlist (`prototype_pulse`, `prototype_guard`, `prototype_dash`, `prototype_burst`).
|