don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-06-15-NEO-133.md

4.9 KiB
Raw Permalink Blame History

Code review — NEO-133 (E7M3-01)

Date: 2026-06-15
Scope: Branch NEO-133-e7m3-factiondef-catalog-quest-gate-rep-schemas-ci — commits 7183461 … 0948d9d vs main
Base: main (merge-base at 33cfc62)

Verdict

Approve with nits

Summary

This branch lands Epic 7 Slice 3 content contracts ahead of server faction load: three new JSON schemas (FactionDef, FactionGateRule, ReputationGrantRow), prototype_factions.json, quest JSON extensions (factionGateRules, reputationGrants), a fifth quest (prototype_quest_grid_contract), and matching CI gates in scripts/validate_content.py. Server changes are intentionally minimal—five-quest roster constants, rep-only grid-contract E7M2 bundle freeze entry, and schema $ref registration so startup validation accepts the new quest JSON. Documentation and decomposition tracking (E7.M3 backlog, module register, alignment table) are updated coherently. Risk is low: no runtime faction/rep behavior yet; validate_content.py and dotnet test both pass locally.

Documentation checked

Path Result
docs/plans/NEO-133-implementation-plan.md Matches — shipped items reconcile with plan (schemas, catalog, CI, server parity, docs).
docs/plans/E7M3-pre-production-backlog.md Matches — E7M3-01 AC checked; client capstone NEO-142/143 documented.
docs/decomposition/modules/E7_M3_FactionReputationLedger.md Matches — Slice 3 freeze table and CI line updated.
docs/decomposition/modules/E7_M1_QuestStateMachine.md Matches — Slice 1 freeze table preserved; CI paragraph cross-links E7.M3 five-quest roster.
docs/decomposition/modules/module_dependency_register.md Matches — E7.M3 status In Progress, E7.M2 dependency added.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E7.M3 row added.
docs/decomposition/epics/epic_07_quest_faction.md Matches — Slice 3 backlog link and Godot capstone AC.
content/README.md Matches — factions folder, Slice 3 paragraph.
server/README.md N/A for NEO-133 — no faction section yet (planned NEO-134).

Register/tracking updates for E7.M3 are appropriately landed in this PR.

Blocking issues

None.

Suggestions

  1. E7.M1 module doc vs five-quest CI — E7_M1_QuestStateMachine.md still states CI requires exactly the four Slice 1 quest ids. Add a one-line cross-link to E7.M3 (PROTOTYPE_E7M3_QUEST_IDS) so readers do not assume the four-quest gate is still the active roster check. Done. CI paragraph cross-links E7.M3 roster expansion.

  2. Server constant naming — PrototypeE7M1QuestCatalogRules.ExpectedQuestIds and TryGetE7M1GateError now enforce five ids while labels/messages still say “E7M1”. When NEO-134 lands, consider PrototypeE7M3QuestCatalogRules (or rename + update error strings) to avoid onboarding confusion. Done (partial). Error string now says “prototype quest roster”; class/XML docs note NEO-134 rename. Full type rename deferred to NEO-134.

  3. Remove dead CI helper — _prototype_e7m1_quest_gate in scripts/validate_content.py is no longer called (superseded by _prototype_e7m3_quest_roster_gate). Delete or mark deprecated to prevent accidental reintroduction of the four-quest gate. Done. Dead helper removed.

  4. Faction standing band schema — faction-def.schema.json does not constrain minStanding <= neutralStanding <= maxStanding. Optional JSON Schema minimum/maximum cross-field checks (or a CI gate mirroring faction freeze) would catch designer mistakes before NEO-134 server load. Done. _prototype_e7m3_faction_standing_band_gate enforces ordering on all faction rows.

Nits

  • Nit: PROTOTYPE_E7M1_QUEST_IDS (four ids) remains beside PROTOTYPE_E7M3_QUEST_IDS (five ids); fine for historical Slice 1 reference, but a short comment on E7M1 constant (“Slice 1 roster subset; CI roster gate is E7M3”) would clarify intent. Done.
  • Nit: QuestDefinitionCatalogLoaderTests.Load_ShouldSucceed_WhenCatalogMatchesPrototypeContract asserts gather/chain rows but does not assert grid-contract gate/bundle fields; acceptable per plan (“no new Fact unless regression needs it”), but a single assert on grid-contract prereq/gate would cheaply lock the fifth quest shape server-side. Done. Asserts grid-contract prereq and rep-only bundle (item/skill empty at server parse layer).

Verification

Commands run during review (passed):

python3 scripts/validate_content.py
dotnet test server/NeonSprawl.Server.Tests

Author should confirm PR CI content gate green before merge.

Known intentional gap (not a blocker for E7M3-01): Server QuestDefRow / QuestRewardBundleRow do not model factionGateRules or reputationGrants; JSON is schema-validated at load but ignored until NEO-134–NEO-138. Grid contract can be accepted without standing check until NEO-137/NEO-138.