From e75ac3932f07cb59da00271acf01fb1d6ef45beb Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 27 Jun 2026 19:59:31 -0400 Subject: [PATCH] NEO-148: add code review for contract reward router --- docs/reviews/2026-06-27-NEO-148.md | 56 ++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 docs/reviews/2026-06-27-NEO-148.md diff --git a/docs/reviews/2026-06-27-NEO-148.md b/docs/reviews/2026-06-27-NEO-148.md new file mode 100644 index 0000000..8e3cc40 --- /dev/null +++ b/docs/reviews/2026-06-27-NEO-148.md @@ -0,0 +1,56 @@ +# Code review — NEO-148 contract completion reward router + +**Date:** 2026-06-27 +**Scope:** Branch `NEO-148-e7m4-05-reward-router-contract-completion` — commits `b20d7d0`..`76ce194` vs `origin/main` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-148 extends the E7.M2 reward delivery stack for contract completion: `RewardDeliverySourceKinds`, source-kind-aware `IRewardDeliveryStore` / `InMemoryRewardDeliveryStore` keys, `RewardDeliveryEvent.SourceKind` + `ContractInstanceId`, `ReputationDeltaSourceKinds.ContractCompletion`, and `RewardRouterOperations.TryDeliverContractCompletion` with a shared private `TryDeliverBundle` core extracted from the quest path. Grant order, compensating rollback, and `TryRecord` race handling mirror NEO-127/NEO-138. Five contract router tests plus one store isolation test cover happy path, idempotent replay, faction deny, inventory deny, and invalid instance id. Implementation plan reconciliation, `server/README.md`, and E7.M4 module anchor are updated. Server-only slice; no client work expected. Full suite: **897** tests passed locally. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-148-implementation-plan.md` | **Matches** — AC checklist marked complete; shipped reconciliation aligns with diff | +| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-05) | **Matches** — router + store extension, orchestrator/outcome store out of scope | +| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — NEO-148 router anchor added | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Partially matches** — store/router extended in code but module page still quest-centric (no NEO-148 cross-ref) | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M4 row stops at NEO-147; NEO-148 not yet recorded | +| `server/README.md` | **Matches** — delivery store fields, `TryGet` overloads, contract router + reason codes | +| Client counterpart (NEO-153) | **N/A** — explicitly out of scope; no prototype-complete claim | + +Register/tracking table update recommended for E7.M4 after merge. + +## Blocking issues + +None. + +## Suggestions + +1. **Update implementation alignment register** — Add **E7M4-05 / NEO-148 landed** to the E7.M4 row in `docs/decomposition/modules/documentation_and_implementation_alignment.md` (router + source-kind store extension), consistent with NEO-144–NEO-147 entries. + +2. **Optional E7.M2 module cross-ref** — A one-line anchor on `E7_M2_RewardAndUnlockRouter.md` noting contract completion reuse via `TryDeliverContractCompletion` (NEO-148) would help readers who land on E7.M2 first. + +3. **Store key format note for future Postgres** — Quest store keys changed from `{playerId}\0{questId}` to `{playerId}\0{sourceKind}\0{sourceId}`. Harmless for in-memory prototype (process restart clears state). When Postgres delivery persistence lands, document or migrate old rows explicitly so idempotent replay cannot double-grant after upgrade. + +4. **Contract fixture reset (follow-on)** — `QuestFixtureOperations` still clears quest rows via `TryClear(playerId, questId)` only. When E7M4-06+ adds Bruno contract loops, contract delivery rows will need a parallel reset path — track on NEO-149 or fixture story, not blocking here. + +## Nits + +- Nit: Plan test table lists “unknown item / full bag deny”; `TryDeliverContractCompletion_ShouldDenyInventoryFull_WithoutStoreRecord` covers full bag only. Shared `TryDeliverBundle` item path is quest-tested for inventory deny; an explicit `invalid_item` contract test would lock naming to AC but is low value given shared core. +- Nit: NEO-130 telemetry hook comments in `TryDeliverBundle` still reference `questId` in payload notes; fine until contract telemetry wiring — consider generalizing when E9.M1 touches contract completion. +- Nit: Plan mentions `MakeContractStoreKey`; implementation uses `MakeStoreKey(playerId, sourceKind, sourceId)` instead — equivalent, no action needed. + +## Verification + +```bash +dotnet test NeonSprawl.sln +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~RewardRouterOperationsTests|FullyQualifiedName~InMemoryRewardDeliveryStoreTests" +``` + +Manual Godot QA: none (server engine per plan).