65 lines
5.2 KiB
Markdown
65 lines
5.2 KiB
Markdown
# Code review — NEO-88 server NPC behavior catalog load (fail-fast)
|
||
|
||
**Date:** 2026-05-25
|
||
**Scope:** Branch `NEO-88-e5m2-server-npc-behavior-catalog-load` · commits `7730bde`–`c56ad22` vs `origin/main`
|
||
**Follow-up:** Doc updates per suggestions 1–2 (module register E5.M2 note + E5.M2 freeze Rules paragraph).
|
||
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-88 delivers **E5M2-02**: fail-fast startup load of `content/npc-behaviors/*_npc_behaviors.json` under `server/NeonSprawl.Server/Game/Npc/`, mirroring the NEO-77 ability-catalog pattern. The loader validates Draft 2020-12 row schema, `schemaVersion`, duplicate `id`, the frozen E5M2 three-id gate (`PrototypeE5M2NpcBehaviorCatalogRules`), and the `leashRadius > aggroRadius` numeric gate; registers `NpcBehaviorDefinitionCatalog` in DI; and eagerly resolves it in `Program.cs`. Sixteen AAA loader/host tests pass. Docs (plan, E5M2 backlog, E5.M2 module status, alignment register, `server/README.md`) are updated. Bruno adds a health smoke request with `folder.bru`. `INpcBehaviorDefinitionRegistry` is correctly deferred to NEO-89. No client or NPC HTTP API — appropriately scoped. Risk is low: infrastructure-only; CI already gates content via NEO-87.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-88-implementation-plan.md`](../plans/NEO-88-implementation-plan.md) | **Matches** — kickoff decisions, loader/catalog/DI scope, acceptance checklist, reconciliation section; registry correctly out of scope. |
|
||
| [`docs/plans/E5M2-prototype-backlog.md`](../plans/E5M2-prototype-backlog.md) · **E5M2-02** | **Matches** — AC checkboxes + landed note. |
|
||
| [`docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md) | **Matches** — Status line cites NEO-88 server load; freeze **Rules** cites CI + server startup (NEO-88). |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) · **E5.M2** | **Matches** — NEO-87 + NEO-88 landed notes; next backlog item NEO-89. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E5.M2 **Status** **In Progress**; **E5.M2 note** cites NEO-87 + NEO-88 server load. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server startup catalog only; no client mutation or NPC runtime. |
|
||
| [`scripts/validate_content.py`](../../scripts/validate_content.py) | **Matches** — C# loader mirrors `_validate_npc_behavior_catalogs` + E5M2 id/numeric gates; sync comment on `PrototypeE5M2NpcBehaviorCatalogRules.ExpectedBehaviorIds`. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — NPC behavior catalog section (config keys, discovery, fail-fast, E5M2 parity, NEO-89 registry deferral). |
|
||
| Full-stack epic decomposition | **Matches** — E5M2-02 is **server/content** only; kickoff defers Godot to E5M2-04+ / NEO-98; no false “prototype complete” claim. |
|
||
| Manual QA | **N/A** — plan documents skip rationale: automated loader/host tests + Bruno health smoke. |
|
||
|
||
Register/tracking: alignment table E5.M2 **In Progress** with NEO-88 note is correct; module register **E5.M2 note** updated with NEO-88.
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Update `module_dependency_register.md` E5.M2 note** — Append **NEO-88 landed:** fail-fast server load of `content/npc-behaviors/*_npc_behaviors.json` with link to [NEO-88 plan](../plans/NEO-88-implementation-plan.md) and [server README — NPC behavior catalog](../../server/README.md#npc-behavior-catalog-contentnpc-behaviors-neo-88) (same pattern as E5.M1 note after NEO-77).~~ **Done.** E5.M2 note extended in `module_dependency_register.md`.
|
||
|
||
2. ~~**E5.M2 freeze Rules paragraph** — In [`E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md), extend the CI sentence to cite server startup enforcement via NEO-88 (mirror E5.M1 freeze rules referencing NEO-77).~~ **Done.** Rules paragraph now cites CI + server startup (NEO-88).
|
||
|
||
## Nits
|
||
|
||
- Nit: `ValidPrototypeCatalogJson` duplicates `content/npc-behaviors/prototype_npc_behaviors.json` inline — fine for isolated temp-dir fixtures (same pattern as NEO-77 ability tests); optional future refactor to copy repo file.
|
||
|
||
- Nit: E5M2 gate error string format differs slightly from Python (`'prototype_melee_pressure', …` vs `sorted(...)!r`); behavior is equivalent and messages are actionable.
|
||
|
||
- Nit: Repeated `services.AddOptions<ContentPathsOptions>().Bind(...)` in each catalog extension — established repo pattern (NEO-51/NEO-66/NEO-77); harmless duplicate binds.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd /home/don/neon-sprawl/server
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~NpcBehaviorDefinitionCatalogLoaderTests"
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
|
||
|
||
# Content CI parity (repo root)
|
||
python3 scripts/validate_content.py
|
||
# Expect: … 1 npc behavior catalog file(s) … 3 unique npc behavior id(s)
|
||
```
|
||
|
||
Bruno: `bruno/neon-sprawl-server/npc-behavior-catalog/` — GET `/health` smoke after catalog boot.
|