NEO-144: add code review for contract catalog CI slice
parent
2afd29e4d9
commit
fe9b706092
|
|
@ -0,0 +1,64 @@
|
||||||
|
# Code review — NEO-144 (E7M4-01 contract catalog + schemas + CI)
|
||||||
|
|
||||||
|
**Date:** 2026-06-20
|
||||||
|
**Scope:** Branch `NEO-144-e7m4-01-contracttemplate-catalog-schemas-ci` — commits `53122f9`…`2afd29e` (6 commits)
|
||||||
|
**Base:** `main`
|
||||||
|
|
||||||
|
## Verdict
|
||||||
|
|
||||||
|
**Approve with nits**
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
NEO-144 delivers the E7M4-01 content slice: `contract-template` and `contract-seed` JSON Schemas, a single-row `prototype_contract_templates.json` catalog, and ~320 lines of CI gates in `validate_content.py` (schema validation, roster/freeze, encounter/item/skill/faction cross-refs, band-1 economy caps). The work mirrors established NEO-133/NEO-124 quest and faction patterns, stays infrastructure-only (no server loader), and updates decomposition docs, `content/README.md`, and CT.M1 consistently. Risk is low: no runtime behavior change until NEO-145; the primary surface is `python3 scripts/validate_content.py`, which passes locally along with 823 server tests.
|
||||||
|
|
||||||
|
## Documentation checked
|
||||||
|
|
||||||
|
| Document | Alignment |
|
||||||
|
|----------|-----------|
|
||||||
|
| `docs/plans/NEO-144-implementation-plan.md` | **Matches** — schemas, catalog, gates, docs, and out-of-scope boundaries (no server/Godot) are all shipped; AC checklist marked complete. |
|
||||||
|
| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-01) | **Matches** — acceptance criteria satisfied; client counterpart correctly none. |
|
||||||
|
| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — freeze table, band policy, and CI landed note present. |
|
||||||
|
| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **Matches** — contract-catalog PR gate paragraph added. |
|
||||||
|
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M4 row updated to In Progress with E7M4-01 catalog landed. |
|
||||||
|
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M4 status and NEO-144 spine note updated. |
|
||||||
|
| `docs/decomposition/modules/client_server_authority.md` | **N/A** — content-only; no authority boundary change. |
|
||||||
|
| Full-stack epic decomposition | **N/A** — infrastructure-only; plan explicitly defers playable loop to NEO-154. |
|
||||||
|
|
||||||
|
Register/tracking tables were updated in this branch; no further alignment edits required for merge.
|
||||||
|
|
||||||
|
## Blocking issues
|
||||||
|
|
||||||
|
None.
|
||||||
|
|
||||||
|
## Suggestions
|
||||||
|
|
||||||
|
1. **Freeze gate ignores `reputationGrants` today** — `_prototype_e7m4_template_freeze_gate` compares bundles via `_normalize_completion_bundle` (item + skill XP only), same as E7M2 quest bundles. Prototype row has no rep grants, so this is fine for E7M4-01. When rep grants land on contract templates (or a second template is added), extend normalization akin to `_normalize_e7m3_completion_bundle` and expand `PROTOTYPE_E7M4_TEMPLATE_FREEZE` so silent rep drift cannot pass CI.
|
||||||
|
|
||||||
|
2. **`contract-seed.schema.json` is not exercised in CI** — intentional per plan (NEO-147+). When HTTP DTO wiring starts, add at least a schema parse/registry smoke check (or a tiny fixture) so `$ref` breakage is caught before server integration.
|
||||||
|
|
||||||
|
3. **Port band-cap constants to C# in NEO-145/NEO-147** — `PROTOTYPE_E7M4_BAND_CAPS` in Python should stay name-aligned with the future server `ContractEconomyValidation` (plan already flags this); call it out in NEO-145 kickoff so drift does not reappear at issue time.
|
||||||
|
|
||||||
|
## Nits
|
||||||
|
|
||||||
|
- Nit: The implementation plan §5 names `PROTOTYPE_E7M4_BAND1_*` constants; shipped code uses `PROTOTYPE_E7M4_BAND_CAPS` keyed by band — the dict form is better for E7M4-07; consider a one-line reconciliation in the plan (cosmetic only; plan reconciliation section already describes shipped shape).
|
||||||
|
|
||||||
|
- Nit: `_prototype_e7m4_cross_ref_gate` iterates only `PROTOTYPE_E7M4_TEMPLATE_IDS`. Correct for the single-template roster; generalize to all catalog rows when the roster gate relaxes in a later story.
|
||||||
|
|
||||||
|
## Verification
|
||||||
|
|
||||||
|
Commands run during review (all green):
|
||||||
|
|
||||||
|
```bash
|
||||||
|
python3 scripts/validate_content.py
|
||||||
|
dotnet test
|
||||||
|
```
|
||||||
|
|
||||||
|
Author should also confirm band-cap negative AC manually (plan documents this; not committed):
|
||||||
|
|
||||||
|
```bash
|
||||||
|
# Temporarily set scrap_metal_bulk quantity to 11 in prototype_contract_templates.json
|
||||||
|
python3 scripts/validate_content.py # expect non-zero exit
|
||||||
|
```
|
||||||
|
|
||||||
|
Optional before merge: skim PR diff for doc-only commits (`E7M4-pre-production-backlog.md`, epic alignment) — no functional concern, but keeps reviewer focus on the three implementation commits (`58d2c32`, `7ecc874`, `2afd29e`).
|
||||||
Loading…
Reference in New Issue