diff --git a/docs/reviews/2026-06-15-NEO-137.md b/docs/reviews/2026-06-15-NEO-137.md new file mode 100644 index 0000000..2924a11 --- /dev/null +++ b/docs/reviews/2026-06-15-NEO-137.md @@ -0,0 +1,55 @@ +# Code review — NEO-137 (E7M3-05) + +**Date:** 2026-06-15 +**Scope:** Branch `NEO-137-e7m3-faction-gate-operations-tryaccept` — commits `1c3d075` … `0241cc3` vs `main` +**Base:** `main` + +## Verdict + +**Request changes** + +## Summary + +This branch lands **E7M3-05**: **`FactionGateOperations.TryEvaluate`** evaluates quest **`FactionGateRule`** rows with vacuous-AND semantics (`standing >= minStanding`), wired into **`QuestStateOperations.TryAccept`** after prerequisite checks and before activation. Quest-level denies surface **`faction_gate_blocked`** via **`QuestStateReasonCodes.FactionGateBlocked`**; ops-level **`FactionGateEvaluateOutcome`** retains failing-rule context for future telemetry/HUD. **`IFactionStandingStore`** threads through **`QuestAcceptApi`** and test helpers. Coverage is solid: five unit tests, three integration tests on grid contract (deny at 0/14, success at 15), HTTP deny test, and host DI smoke. **`790`** server tests pass locally. Core server logic matches the plan and NEO-136 ops precedent. One **blocking** gap: the new Bruno deny smoke runs in CI without completing **`prototype_quest_operator_chain`**, so the collection will assert the wrong **`reasonCode`** (`prerequisite_incomplete` instead of **`faction_gate_blocked`**). + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-137-implementation-plan.md` | **Matches** — ops, types, wire site, reason code, tests, README, module/alignment updates; AC checklist marked complete; success Bruno deferred to NEO-138 per kickoff. | +| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-05 scope (quest accept gate eval, no HTTP standing read, no client counterpart). | +| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-05 `FactionGateOperations` landed (NEO-137). | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated for E7M3-05 / NEO-137. | +| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — register row already lists `FactionGateRule` contract. | +| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — `TryAccept` gate hook site behavior aligns with module responsibilities. | +| `server/README.md` | **Matches** — new FactionGateOperations section + quest accept POST table includes `faction_gate_blocked`. | + +## Blocking issues + +1. **Bruno CI will fail on faction-gate deny smoke.** `bruno/neon-sprawl-server/quest-progress/Accept grid contract faction gate deny.bru` (seq **10**) asserts `reasonCode === "faction_gate_blocked"`, but the quest-progress collection never completes **`prototype_quest_operator_chain`** before that request (prior steps only accept/deny gather/refine through seq **8**). **`TryAccept`** checks prerequisites before gate eval, so a fresh dev player receives **`prerequisite_incomplete`**, not **`faction_gate_blocked`**. `.github/workflows/dotnet.yml` runs the full Bruno collection with **`--bail`** on every push touching `bruno/neon-sprawl-server/**`. Fix options (pick one): add prerequisite Bruno steps (accept + complete operator chain, or a dev fixture seed if/when available), add a **`script:pre-request`** that marks the chain completed the way integration tests do, or defer/remove this request from the CI collection until NEO-138 makes the spine reachable. The `.bru` **docs** block acknowledges the prerequisite but the collection does not implement it. + +## Suggestions + +1. **Multi-rule AND unit test.** Prototype grid contract has one gate today; add a **`TryEvaluate`** test with two rules (one pass, one fail) to lock AND semantics if content adds multi-faction gates before E4.M1 travel gates reuse the ops. +2. **Bruno seq documentation in README.** `server/README.md` notes success Bruno is deferred to NEO-138; add one line that deny Bruno requires operator-chain-complete setup (or list the intended seq after prerequisite steps land) so manual runs do not surprise operators. + +## Nits + +- Nit: **`TryAccept`** maps all gate failures to **`faction_gate_blocked`**, discarding ops-level **`unknown_faction`** from the standing store — intentional per plan (quest-level code only in HTTP), but worth a one-line XML remark on the gate deny branch if NEO-141 telemetry needs to distinguish store vs threshold denies later. +- Nit: **`FactionGateOperationsTests`** host DI smoke asserts **`gate_blocked`** at standing 0 without seeding standing — correct behavior, but a brief comment that missing standing reads as **0** would aid future readers. + +## Verification + +```bash +# Full server suite (author reported 790 green) +dotnet test NeonSprawl.sln + +# Faction-gate-focused filter +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~FactionGate|FullyQualifiedName~GridContract" + +# Bruno (will fail on seq 10 until prerequisite setup is added) +cd bruno/neon-sprawl-server +npx --yes @usebruno/cli@3.3.0 run quest-progress --env Local --sandbox=developer --bail +``` + +Manual: after fixing Bruno prerequisites, run **`Accept grid contract faction gate deny.bru`** only when **`prototype_quest_operator_chain`** is **completed** and Grid Operators standing is **< 15**.