diff --git a/docs/reviews/2026-06-15-NEO-138.md b/docs/reviews/2026-06-15-NEO-138.md new file mode 100644 index 0000000..e708257 --- /dev/null +++ b/docs/reviews/2026-06-15-NEO-138.md @@ -0,0 +1,67 @@ +# Code review — NEO-138 (E7M3-06) + +**Date:** 2026-06-15 +**Scope:** Branch `NEO-138-e7m3-reward-router-reputation-grants` — commits `599d87b` … `3466f1e` vs `main` +**Base:** `main` + +## Verdict + +**Request changes** + +## Summary + +This branch lands **E7M3-06**: **`RewardRouterOperations.TryDeliverQuestCompletion`** applies **`reputationGrants`** via **`ReputationOperations.TryApplyDelta`** after items and skill XP, with compensating rollback (including inverse rep deltas with `:rollback` suffix) and **`GrantedReputation`** on delivery event/result snapshots. Wiring threads **`IFactionStandingStore`** + **`IReputationDeltaStore`** through quest state, objective wiring, and gameplay APIs. Tests cover happy path, idempotent replay, unknown-faction deny with item/skill rollback, and quest-state integration (operator chain +15 standing, grid contract accept after organic chain completion). Bruno adds **`operator-chain-quest-flow-helper.js`** and **`Accept grid contract after operator chain.bru`**. **`808`** server tests pass locally. Core router logic matches NEO-127/NEO-136 precedent and the implementation plan. One **blocking** Bruno assertion contradicts server accept behavior (grid contract auto-completes when the player already holds **`survey_drone_kit`** from operator-chain rewards). Plan-listed race-rollback rep test and audit-row assertion are still missing. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-138-implementation-plan.md` | **Partially matches** — router apply order, idempotency, rollback, wiring, README, module/alignment updates, and most tests landed; Bruno quest-flow + grid accept success present. Gaps: no **`TryRecord`** race rep rollback unit test; operator-chain router test omits audit-row assertion; multi-row rep deny via **`audit_append_failed`** not covered (plan table row). | +| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-06 rep grants on quest completion bundle via router; HTTP standing / client HUD explicitly deferred. | +| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-06 landed (NEO-138); rep flow through E7.M2 router documented. | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — bundle apply + idempotent delivery store extended with rep rows (implicit via router). | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated for NEO-138 router extension. | +| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — no register row edit required beyond module doc status. | +| `server/README.md` | **Matches** — reward router section documents rep apply order, reason codes, rollback policy, Bruno seq **11** success sibling, telemetry hook comment for **`reputation_delta`**. | + +## Blocking issues + +1. **Bruno success smoke asserts wrong quest status after accept.** `bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru` expects `body.quest.status === "active"` and `currentStepIndex === 0`. After organic operator-chain completion the player receives **`survey_drone_kit`** in the completion bundle. **`prototype_quest_grid_contract`** has a single **`inventory_has_item`** objective for that item. **`QuestStateOperations.TryAccept`** runs **`QuestObjectiveWiring.TryCompleteStepIfSatisfied`** immediately after activation, so accept auto-completes the quest and returns **`completed`** (integration test `TryAccept_ShouldAcceptGridContract_WhenOperatorChainCompletedWithRepGrant` correctly asserts **`QuestProgressStatus.Completed`**). CI runs the full quest-progress collection with **`--bail`**; seq **11** will fail on the status assertion even when the gate and rep grant work. Fix: expect **`completed`** (and optionally assert Rust Collective +10 standing or delivery event), or change content/fixture so the kit is not held at accept time if **`active`** is the intended smoke signal. + +## Suggestions + +1. **Add rep rollback on `TryRecord` race loss.** Extend `TryDeliverQuestCompletion_ShouldRollbackGrants_WhenTryRecordLosesRace` (or add a sibling) with an operator-chain bundle including **`reputationGrants`** and assert standing returns to pre-call value after simulated race loss — listed explicitly in NEO-138 plan §6 tests table. +2. **Assert audit row on operator-chain router happy path.** Plan scenario “audit row queryable” — add `IReputationDeltaStore` lookup for `{idempotencyKey}:rep:{factionId}` in `TryDeliverQuestCompletion_ShouldApplyItemAndSkillXp_WhenOperatorChainBundle` (or the dedicated rep test). +3. **Document auto-complete on grid-contract accept in tests/README.** Integration test name says “Accept” but the meaningful gate check is standing ≥ 15; the returned snapshot is **`completed`** because the kit is already in inventory. A one-line comment in `QuestStateOperationsTests` and Bruno **docs** block would prevent future “active vs completed” confusion. +4. **Optional: multi-row rep deny rollback test.** Plan lists rep deny after items with audit append failure on the second rep row; only **`unknown_faction`** rollback is covered today. Consider a store stub that fails **`TryAppend`** on the second row to lock compensating rollback across grant types. + +## Nits + +- Nit: `TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull` does not assert `GrantedReputation` is empty — mirror item/skill empty assertions for snapshot parity. +- Nit: `CompensatingRevertReputation` skips `Amount == 0` rows; fine for prototype bundles but a clamp-to-zero edge case could leave standing changed while snapshot says 0 — defer unless content adds negative grants near floor. + +## Verification + +```bash +# Full server suite (808 green locally) +dotnet test NeonSprawl.sln + +# NEO-138-focused filter +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~RewardRouterOperations|FullyQualifiedName~GridContract|FullyQualifiedName~OperatorChain" + +# Bruno quest-progress (seq 11 will fail until status assertion fixed) +cd bruno/neon-sprawl-server +npx --yes @usebruno/cli@3.3.0 run quest-progress --env Local --sandbox=developer --bail +``` + +With Postgres + dev fixtures (matches CI): + +```bash +ConnectionStrings__NeonSprawl='Host=localhost;Port=5432;Database=neon_sprawl;Username=neon_sprawl;Password=neon_sprawl_dev' \ +Game__EnableCombatTargetFixtureApi=true \ +Game__EnableQuestFixtureApi=true \ +Game__EnableResourceNodeFixtureApi=true \ +dotnet run --project server/NeonSprawl.Server/NeonSprawl.Server.csproj --urls http://127.0.0.1:5253 +``` + +Manual: run **`Accept grid contract after operator chain.bru`** after fixing status expectation; confirm **`accepted: true`** and quest row **`completed`** when **`survey_drone_kit`** is already in bag.