# 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.