57 lines
4.2 KiB
Markdown
57 lines
4.2 KiB
Markdown
# 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).
|