From 4a9730c577544f5e38a431e745c67cc474996314 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 16 May 2026 20:05:51 -0400 Subject: [PATCH] NEO-45: address code review doc alignment and CI nits. Align E2.M3 In Progress status, backlog checkboxes, perks map doc; reject unreferenced PerkDef rows; tier-2 branch displayNames; review Done. --- .../mastery/prototype_salvage_mastery.json | 2 + .../modules/E2_M3_MasteryAndPerkUnlocks.md | 4 +- .../modules/module_dependency_register.md | 4 +- docs/plans/E2M3-pre-production-backlog.md | 6 +- docs/plans/NEO-45-implementation-plan.md | 2 +- docs/reviews/2026-05-16-NEO-45-code-review.md | 70 +++++++++++++++++++ scripts/validate_content.py | 12 ++++ 7 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 docs/reviews/2026-05-16-NEO-45-code-review.md diff --git a/content/mastery/prototype_salvage_mastery.json b/content/mastery/prototype_salvage_mastery.json index 2c92459..ea8165d 100644 --- a/content/mastery/prototype_salvage_mastery.json +++ b/content/mastery/prototype_salvage_mastery.json @@ -38,10 +38,12 @@ "branches": [ { "branchId": "scrap_efficiency", + "displayName": "Scrap Efficiency", "perkIds": ["salvage_scrap_efficiency_1"] }, { "branchId": "bulk_haul", + "displayName": "Bulk Haul", "perkIds": ["salvage_bulk_haul_1"] } ] diff --git a/docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md b/docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md index 47bc7ea..df40124 100644 --- a/docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md +++ b/docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md @@ -8,7 +8,7 @@ | **Epic** | [Epic 2 — Skills and Progression Framework](../epics/epic_02_skills_and_progression.md) | | **Linear (Epic 2 project)** | [Epic 2 — Classless Skill and Progression Framework](https://linear.app/neon-sprawl/project/epic-2-classless-skill-and-progression-framework-5200f84bf8b6) | | **Stage target** | Pre-production | -| **Status** | Planned (see [dependency register](module_dependency_register.md)) | +| **Status** | In Progress (see [dependency register](module_dependency_register.md); **NEO-45** catalog + CI landed) | ## Linear backlog (Epic 2 Slice 4 — E2.M3) @@ -45,6 +45,8 @@ Mastery tracks and perk unlock state so **skill** builds have expressive identit | **JSON Schema** | [`content/schemas/mastery-catalog.schema.json`](../../../content/schemas/mastery-catalog.schema.json) | | **Prototype catalog** | [`content/mastery/prototype_salvage_mastery.json`](../../../content/mastery/prototype_salvage_mastery.json) | +Each catalog file has top-level **`schemaVersion`**, a **`perks`** map (`perkId` → **`PerkDef`**), and a **`tracks`** array. Branches reference perks by id string only (no inline perk objects). + **`MasteryTrack` fields (prototype):** | Field | Rule | diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index b7c8ac1..2a619ca 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -29,14 +29,14 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen |---|---|---|---|---|---| | E2.M1 | SkillDefinitionRegistry | None | SkillDef, SkillCategory, UnlockRequirement, allowedXpSourceKinds | Prototype | In Progress | | E2.M2 | XpAwardAndLevelEngine | E2.M1 | XpGrantEvent, LevelCurve, LevelUpEvent | Prototype | In Progress | -| E2.M3 | MasteryAndPerkUnlocks | E2.M2 | MasteryTrack, PerkDef, PerkUnlockEvent, PerkState | Pre-production | Planned | +| E2.M3 | MasteryAndPerkUnlocks | E2.M1, E2.M2 | MasteryTrack, PerkDef, PerkUnlockEvent, PerkState | Pre-production | In Progress | | E2.M4 | ProgressionPacingControls | E2.M2, E9.M2 | XpModifierProfile, CatchUpRule, PacingPolicy | Pre-production | Planned | **E2.M1 note:** Epic 2 Slice 1 backlog in Linear ([Epic 2 — Classless Skill and Progression Framework](https://linear.app/neon-sprawl/project/epic-2-classless-skill-and-progression-framework-5200f84bf8b6)): [NEO-33](https://linear.app/neon-sprawl/issue/NEO-33) → [NEO-34](https://linear.app/neon-sprawl/issue/NEO-34) → [NEO-35](https://linear.app/neon-sprawl/issue/NEO-35) → [NEO-36](https://linear.app/neon-sprawl/issue/NEO-36); label **`E2.M1`**. See [E2_M1_SkillDefinitionRegistry.md](E2_M1_SkillDefinitionRegistry.md). **NEO-33** (content + CI + docs) moves the register row to **In Progress**; **NEO-34+** cover server registry and HTTP. Update [documentation and implementation alignment](documentation_and_implementation_alignment.md) when slices land. **E2.M2 note:** Epic 2 Slice 2 — [NEO-37](https://linear.app/neon-sprawl/issue/NEO-37) (progression snapshot **GET**) and [NEO-38](https://linear.app/neon-sprawl/issue/NEO-38) (grant **POST**, persistence, level-up payloads); label **`E2.M2`**. Module doc [E2_M2_XpAwardAndLevelEngine.md](E2_M2_XpAwardAndLevelEngine.md). Slice 3 integration: [NEO-41](https://linear.app/neon-sprawl/issue/NEO-41)–[NEO-43](https://linear.app/neon-sprawl/issue/NEO-43) landed; **NEO-44** (gig XP, Epic 5) remains backlog. Keep aligned with [documentation and implementation alignment](documentation_and_implementation_alignment.md). -**E2.M3 note:** Epic 2 Slice 4 — [NEO-45](https://linear.app/neon-sprawl/issue/NEO-45) → [NEO-49](https://linear.app/neon-sprawl/issue/NEO-49); label **`E2.M3`**. Flagship **`salvage`** mastery track; dependency order and story text in [E2_M3_MasteryAndPerkUnlocks.md](E2_M3_MasteryAndPerkUnlocks.md) and [E2M3-pre-production-backlog.md](../../plans/E2M3-pre-production-backlog.md). **NEO-47** blocked by [NEO-46](https://linear.app/neon-sprawl/issue/NEO-46) and [NEO-38](https://linear.app/neon-sprawl/issue/NEO-38) (skill level source). +**E2.M3 note:** Epic 2 Slice 4 — [NEO-45](https://linear.app/neon-sprawl/issue/NEO-45) → [NEO-49](https://linear.app/neon-sprawl/issue/NEO-49); label **`E2.M3`**. **NEO-45 landed:** prototype **`salvage`** mastery catalog + schema + CI (`content/mastery/`, `scripts/validate_content.py`); register row **In Progress**. Flagship track; dependency order and story text in [E2_M3_MasteryAndPerkUnlocks.md](E2_M3_MasteryAndPerkUnlocks.md) and [E2M3-pre-production-backlog.md](../../plans/E2M3-pre-production-backlog.md). **NEO-47** blocked by [NEO-46](https://linear.app/neon-sprawl/issue/NEO-46) and [NEO-38](https://linear.app/neon-sprawl/issue/NEO-38) (skill level source). ### Epic 3 — Crafting Economy diff --git a/docs/plans/E2M3-pre-production-backlog.md b/docs/plans/E2M3-pre-production-backlog.md index 38bc23a..e1d983b 100644 --- a/docs/plans/E2M3-pre-production-backlog.md +++ b/docs/plans/E2M3-pre-production-backlog.md @@ -51,9 +51,9 @@ Working backlog for **Epic 2 — Slice 4** ([mastery and pacing](../decompositio **Acceptance criteria** -- [ ] PR gate validates mastery JSON against schema. -- [ ] Only **`salvage`** has a track; `refine` / `intrusion` not required. -- [ ] Stable `branchId` / `perkId` documented in [E2_M3](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) freeze box. +- [x] PR gate validates mastery JSON against schema. (**Done** — NEO-45.) +- [x] Only **`salvage`** has a track; `refine` / `intrusion` not required. (**Done** — NEO-45.) +- [x] Stable `branchId` / `perkId` documented in [E2_M3](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) freeze box. (**Done** — NEO-45.) --- diff --git a/docs/plans/NEO-45-implementation-plan.md b/docs/plans/NEO-45-implementation-plan.md index 2bfdef1..0b0f23f 100644 --- a/docs/plans/NEO-45-implementation-plan.md +++ b/docs/plans/NEO-45-implementation-plan.md @@ -103,7 +103,7 @@ - **Tier index convention:** Use explicit `tierIndex` (1-based) in JSON for stable server persistence keys in NEO-47; document in schema description. - **Catalog growth:** Slice 4 gate mirrors NEO-33 — relaxing “salvage only” requires a new Linear issue to change `validate_content.py` constants. -- **Drift vs NEO-46:** Server loader must mirror Python rules; NEO-46 plan should reference this plan’s gate constants. +- **Drift vs NEO-46:** Server loader must mirror Python rules; NEO-46 plan should reference this plan’s gate constants and validate **`tierIndex`** uniqueness / sequential 1..N per track (schema allows gaps today). - **None** blocking at kickoff after clarifications above. ## Decisions diff --git a/docs/reviews/2026-05-16-NEO-45-code-review.md b/docs/reviews/2026-05-16-NEO-45-code-review.md new file mode 100644 index 0000000..a1b4150 --- /dev/null +++ b/docs/reviews/2026-05-16-NEO-45-code-review.md @@ -0,0 +1,70 @@ +# Code review — NEO-45 (salvage mastery catalog + schemas + CI) + +**Date:** 2026-05-16 + +**Scope:** Branch `NEO-45-salvage-mastery-catalog-schemas-ci`, commit `f5f04d8` (`NEO-45: lock prototype salvage mastery catalog, schemas, and CI.`). Issue **NEO-45**. + +**Base:** `main` (parent of `f5f04d8`) + +## Verdict + +**Approve with nits** + +## Summary + +The change delivers NEO-45 as a **content-only** slice: `mastery-catalog.schema.json`, prototype `prototype_salvage_mastery.json` (one **salvage** track, tier 1 branch pick @ level 2 with empty `perkIds`, tier 2 perks @ level 4), extended `validate_content.py` with schema validation, `skillId` cross-check, perk map key/`id` alignment, duplicate `perkId` / reference guards, branch-set continuity across tiers, strictly increasing `requiredLevel`, and a Slice 4 **salvage-only / exactly-one-track** gate. Documentation (E2.M3 module, CT.M1, epic Slice 4, alignment table, implementation plan, backlog) is thorough and matches kickoff decisions. Risk is low: no server/C#/client surface; PR gate already runs the script. Main gap is **status field drift** between the alignment row (**In Progress**) and the register / E2.M3 Summary (**Planned**). + +## Documentation checked + +| Document | Assessment | +|----------|------------| +| `docs/plans/NEO-45-implementation-plan.md` | **Matches** — schema shape, catalog layout, validator gates, frozen ids, out-of-scope (NEO-46+), verification steps align with the implementation. | +| `docs/plans/E2M3-pre-production-backlog.md` | **Matches** — E2M3-01 acceptance criteria marked done (NEO-45). | +| `docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md` | **Matches** — data contract (`perks` map + `tracks`), designer note, freeze table, **In Progress** status. | +| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **Matches** — CI paragraph documents mastery validation alongside skills. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E2.M3 row aligned with register and module **In Progress**. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E2.M3 **In Progress**, Depends On **E2.M1, E2.M2**, NEO-45 landed in note. | +| `docs/decomposition/epics/epic_02_skills_and_progression.md` | **Matches** — Slice 4 scope, NEO-45→49 chain, E2.M4 separation. | +| `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` | **Matches** — NEO-43 landed line + E2.M3 level-up hook cross-link. | +| `content/README.md` | **Matches** — Slice 4 freeze reminder and links to E2.M3. | + +**Register / tracking:** ~~Update **Planned → In Progress** for E2.M3~~ **Done** (register, module Summary, backlog checkboxes). + +## Blocking issues + +_None._ + +## Suggestions + +1. ~~**Align E2.M3 status across docs** — `documentation_and_implementation_alignment.md` marks E2.M3 **In Progress** with NEO-45 landed, but `module_dependency_register.md` and `E2_M3_MasteryAndPerkUnlocks.md` Summary still say **Planned**. Pick one convention (recommended: **In Progress** on register + module Summary when catalog merges, matching E2.M1 after NEO-33).~~ **Done.** Register + module Summary → **In Progress**; Depends On → **E2.M1, E2.M2**; E2.M3 note cites NEO-45 landed. + +2. ~~**Backlog checkboxes** — In `docs/plans/E2M3-pre-production-backlog.md`, flip E2M3-01 acceptance criteria to `[x]` (or add a “Done in NEO-45” note) so the backlog does not contradict `NEO-45-implementation-plan.md`.~~ **Done.** + +3. ~~**Optional CI gate: unreferenced perks** — `perks` map entries with no `perkIds` reference currently pass validation (orphan perk → exit 0). Consider rejecting dead `PerkDef` rows in a follow-up if designers should not ship unused defs; not required by NEO-45 AC.~~ **Done.** `validate_content.py` rejects unreferenced `perks` map entries per file. + +4. ~~**NEO-46 parity** — Plan already flags mirroring Python gate constants in C# loader; when implementing NEO-46, also consider validating **`tierIndex`** uniqueness / sequential 1..N per track (schema allows gaps today; catalog is correct).~~ **Done.** Noted in `NEO-45-implementation-plan.md` open questions for NEO-46. + +## Nits + +- ~~**Nit:** Tier 2 branches in `prototype_salvage_mastery.json` omit `displayName` while tier 1 includes it; valid per schema but slightly uneven for authoring.~~ **Done.** Tier 2 branches include `displayName`. +- ~~**Nit:** `global_referenced_perk_ids` treats a perk as globally single-use across branches/files — correct for mutually exclusive v1 trees, but document if a future catalog ever needs the same `perkId` on two branches (unlikely).~~ **Done.** Comment in `validate_content.py`. +- **Nit:** Adding a second **salvage** track with duplicated perk references fails on duplicate reference errors before the Slice 4 “exactly one track” message; harmless, but authors adding a second track with no perk refs get the clearer Slice 4 error. + +## Verification + +Run before merge: + +```bash +pip install -r scripts/requirements-content.txt # if needed +python3 scripts/validate_content.py +python3 scripts/check_decomposition_language.py +``` + +Manual negative cases (restore catalog after each): + +- Unknown `skillId` → non-zero, actionable stderr. +- Unknown `perkIds` reference → non-zero. +- Duplicate perk reference across branches → non-zero. +- Second `salvage` track (no perk refs) → Slice 4 gate non-zero. + +No `dotnet test` or Godot run required for this story (content-only per plan). diff --git a/scripts/validate_content.py b/scripts/validate_content.py index e39c138..e001328 100644 --- a/scripts/validate_content.py +++ b/scripts/validate_content.py @@ -61,6 +61,7 @@ def _validate_mastery_catalogs( errors = 0 all_track_skill_ids: list[str] = [] global_perk_ids: dict[str, str] = {} + # Each perkId may appear in at most one branch reference catalog-wide (v1 mutually exclusive trees). global_referenced_perk_ids: dict[str, str] = {} for path in mastery_files: @@ -83,6 +84,8 @@ def _validate_mastery_catalogs( errors += 1 continue + referenced_perk_ids_in_file: set[str] = set() + for perk_key, perk in perks.items(): if not isinstance(perk, dict): print(f"error: {rel}: perks[{perk_key!r}] must be an object", file=sys.stderr) @@ -186,6 +189,7 @@ def _validate_mastery_catalogs( global_referenced_perk_ids[perk_id] = ( f"{rel} tracks[{ti}].tiers[{tier_i}].branches[{bi}]" ) + referenced_perk_ids_in_file.add(perk_id) tier_branch_set = frozenset(tier_branch_ids) if prev_tier_branch_ids is not None and tier_branch_set != prev_tier_branch_ids: @@ -199,6 +203,14 @@ def _validate_mastery_catalogs( if tier_branch_ids: prev_tier_branch_ids = tier_branch_set + for perk_key in perks: + if isinstance(perk_key, str) and perk_key not in referenced_perk_ids_in_file: + print( + f"error: {rel}: perks[{perk_key!r}] is not referenced by any branch perkIds", + file=sys.stderr, + ) + errors += 1 + return errors, all_track_skill_ids