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.pull/79/head
parent
1f6fe86a1c
commit
4a9730c577
|
|
@ -38,10 +38,12 @@
|
||||||
"branches": [
|
"branches": [
|
||||||
{
|
{
|
||||||
"branchId": "scrap_efficiency",
|
"branchId": "scrap_efficiency",
|
||||||
|
"displayName": "Scrap Efficiency",
|
||||||
"perkIds": ["salvage_scrap_efficiency_1"]
|
"perkIds": ["salvage_scrap_efficiency_1"]
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
"branchId": "bulk_haul",
|
"branchId": "bulk_haul",
|
||||||
|
"displayName": "Bulk Haul",
|
||||||
"perkIds": ["salvage_bulk_haul_1"]
|
"perkIds": ["salvage_bulk_haul_1"]
|
||||||
}
|
}
|
||||||
]
|
]
|
||||||
|
|
|
||||||
|
|
@ -8,7 +8,7 @@
|
||||||
| **Epic** | [Epic 2 — Skills and Progression Framework](../epics/epic_02_skills_and_progression.md) |
|
| **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) |
|
| **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 |
|
| **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)
|
## 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) |
|
| **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) |
|
| **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):**
|
**`MasteryTrack` fields (prototype):**
|
||||||
|
|
||||||
| Field | Rule |
|
| Field | Rule |
|
||||||
|
|
|
||||||
|
|
@ -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.M1 | SkillDefinitionRegistry | None | SkillDef, SkillCategory, UnlockRequirement, allowedXpSourceKinds | Prototype | In Progress |
|
||||||
| E2.M2 | XpAwardAndLevelEngine | E2.M1 | XpGrantEvent, LevelCurve, LevelUpEvent | 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.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.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.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
|
### Epic 3 — Crafting Economy
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -51,9 +51,9 @@ Working backlog for **Epic 2 — Slice 4** ([mastery and pacing](../decompositio
|
||||||
|
|
||||||
**Acceptance criteria**
|
**Acceptance criteria**
|
||||||
|
|
||||||
- [ ] PR gate validates mastery JSON against schema.
|
- [x] PR gate validates mastery JSON against schema. (**Done** — NEO-45.)
|
||||||
- [ ] Only **`salvage`** has a track; `refine` / `intrusion` not required.
|
- [x] Only **`salvage`** has a track; `refine` / `intrusion` not required. (**Done** — NEO-45.)
|
||||||
- [ ] Stable `branchId` / `perkId` documented in [E2_M3](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) freeze box.
|
- [x] Stable `branchId` / `perkId` documented in [E2_M3](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) freeze box. (**Done** — NEO-45.)
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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.
|
- **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.
|
- **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.
|
- **None** blocking at kickoff after clarifications above.
|
||||||
|
|
||||||
## Decisions
|
## Decisions
|
||||||
|
|
|
||||||
|
|
@ -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).
|
||||||
|
|
@ -61,6 +61,7 @@ def _validate_mastery_catalogs(
|
||||||
errors = 0
|
errors = 0
|
||||||
all_track_skill_ids: list[str] = []
|
all_track_skill_ids: list[str] = []
|
||||||
global_perk_ids: dict[str, 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] = {}
|
global_referenced_perk_ids: dict[str, str] = {}
|
||||||
|
|
||||||
for path in mastery_files:
|
for path in mastery_files:
|
||||||
|
|
@ -83,6 +84,8 @@ def _validate_mastery_catalogs(
|
||||||
errors += 1
|
errors += 1
|
||||||
continue
|
continue
|
||||||
|
|
||||||
|
referenced_perk_ids_in_file: set[str] = set()
|
||||||
|
|
||||||
for perk_key, perk in perks.items():
|
for perk_key, perk in perks.items():
|
||||||
if not isinstance(perk, dict):
|
if not isinstance(perk, dict):
|
||||||
print(f"error: {rel}: perks[{perk_key!r}] must be an object", file=sys.stderr)
|
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] = (
|
global_referenced_perk_ids[perk_id] = (
|
||||||
f"{rel} tracks[{ti}].tiers[{tier_i}].branches[{bi}]"
|
f"{rel} tracks[{ti}].tiers[{tier_i}].branches[{bi}]"
|
||||||
)
|
)
|
||||||
|
referenced_perk_ids_in_file.add(perk_id)
|
||||||
|
|
||||||
tier_branch_set = frozenset(tier_branch_ids)
|
tier_branch_set = frozenset(tier_branch_ids)
|
||||||
if prev_tier_branch_ids is not None and tier_branch_set != prev_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:
|
if tier_branch_ids:
|
||||||
prev_tier_branch_ids = tier_branch_set
|
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
|
return errors, all_track_skill_ids
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue