NEO-133: Add code review for E7M3-01 catalog branch.
parent
0948d9d4a8
commit
92c078b0e3
|
|
@ -0,0 +1,61 @@
|
|||
# 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` | **Partially matches** — Slice 1 freeze table still documents **four** quests; CI enforcement paragraph still says “exactly these four quest ids” without noting E7M3 roster expansion (see Suggestions). |
|
||||
| `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.
|
||||
|
||||
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.
|
||||
|
||||
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.
|
||||
|
||||
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.
|
||||
|
||||
## 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.
|
||||
- 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.
|
||||
|
||||
## Verification
|
||||
|
||||
Commands run during review (passed):
|
||||
|
||||
```bash
|
||||
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.
|
||||
Loading…
Reference in New Issue