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-134.md

4.3 KiB
Raw Blame History

Code review — NEO-134 (E7M3-02)

Date: 2026-06-15
Scope: Branch NEO-134-e7m3-server-faction-catalog-load-fail-fast — commits edb3351 … 48bca98 vs main
Base: main

Verdict

Approve with nits

Summary

This branch lands E7M3-02: fail-fast startup load of content/factions/*_factions.json with CI-parity roster/freeze/standing-band gates, IFactionDefinitionRegistry, and quest-loader extensions that parse factionGateRules / reputationGrants and run E7M3 cross-ref, completion-bundle freeze (including rep rows), and grid-contract shape gates. Load order is correct (skills → factions → quests via DI dependency). The implementation mirrors established catalog patterns (SkillDefinitionCatalogLoader, NEO-125 quest gates) and keeps Python/C# gate constants in sync. Test coverage is strong (faction loader negatives, registry + host boot, four new quest-loader negatives); 746 server tests and validate_content.py pass locally. Risk is low — infrastructure-only, no player-facing runtime behavior yet. Minor gaps: decomposition tracking docs not updated for E7M3-02, and two plan-listed loader negative cases are only partially covered.

Documentation checked

Path Result
docs/plans/NEO-134-implementation-plan.md Matches — shipped items reconcile with plan (faction catalog, quest extensions, DI/load order, tests, README). AC checklist marked complete.
docs/plans/E7M3-pre-production-backlog.md Matches — E7M3-02 scope (server load, CI parity, no client counterpart) aligns with implementation.
docs/decomposition/modules/E7_M3_FactionReputationLedger.md Partially matches — contracts and freeze table still accurate; Status line still cites E7M3-01 only (should note E7M3-02 server load landed).
docs/decomposition/modules/documentation_and_implementation_alignment.md Partially matches — E7.M3 row still describes E7M3-01; should mention E7M3-02 faction catalog load on server.
docs/decomposition/modules/module_dependency_register.md N/A change — no register update required for this slice.
server/README.md Matches — new faction catalog section; quest catalog paragraph updated for E7M3 gates and faction dependency.
content/README.md N/A — no content changes in this branch.

Register/tracking table updates for E7M3-02 are a should fix (non-blocking).

Blocking issues

None.

Suggestions

  1. Decomposition status — Update E7_M3_FactionReputationLedger.md Status (one line) and documentation_and_implementation_alignment.md E7.M3 row to record E7M3-02 server faction catalog load landed (NEO-134). Plan §7 lists this as optional but it keeps module tracking honest for NEO-135 consumers.

  2. Standing-band loader integration test — Plan §6 lists “Invalid standing band ordering” under FactionDefinitionCatalogLoaderTests. TryGetStandingBandGateError_ShouldReturnError_WhenOrderingFails covers the rule in isolation; add a Load_ShouldThrow_WhenStandingBandOrderingFails that writes invalid minStanding/neutralStanding JSON through the full loader path (cheap regression lock).

  3. Missing factions directory test — Plan §6 also lists “Missing factions directory / schema.” Schema-missing is covered; add Load_ShouldThrow_WhenFactionsDirectoryMissing for parity with other catalog loader test suites.

Nits

  • Nit: FactionDefinitionCatalogLoaderTests.cs imports Microsoft.Extensions.DependencyInjection but does not use it.
  • Nit: FactionDefinitionCatalogLoader logs unconditionally on success while QuestDefinitionCatalogLoader guards with logger.IsEnabled(LogLevel.Information) — harmless, but inconsistent with sibling loaders.

Verification

Commands run during review (passed):

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

Author should confirm PR CI green before merge. Bruno smoke (bruno/neon-sprawl-server/faction-catalog/) is appropriate — no manual QA doc required per plan (infrastructure-only).

Intentional follow-on (not blockers): Standing store, gate evaluation on accept, HTTP projection, and Godot HUD remain NEO-135–NEO-143. PrototypeE7M1QuestCatalogRules full rename remains deferred per plan.