From 1333da29384ed12eec6f5bcd586f3a6d6080086c Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 22:49:00 -0400 Subject: [PATCH] NEO-130: Address review feedback on unlock_granted stub. Document E9.M1 durable-emit gating for rollback paths; reconcile plan bullet for class summary; strike through resolved review items. --- docs/plans/NEO-130-implementation-plan.md | 2 +- docs/reviews/2026-06-07-NEO-130.md | 9 +++++---- .../Game/Rewards/RewardRouterOperations.cs | 2 ++ 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/docs/plans/NEO-130-implementation-plan.md b/docs/plans/NEO-130-implementation-plan.md index a32b707..2322e3a 100644 --- a/docs/plans/NEO-130-implementation-plan.md +++ b/docs/plans/NEO-130-implementation-plan.md @@ -50,7 +50,7 @@ ## Implementation reconciliation (shipped) -- **Engine anchor:** **`RewardRouterOperations.TryDeliverQuestCompletion`** — two comment-only hook sites with **`TODO(E9.M1)`** (no runtime emit). +- **Engine anchor:** **`RewardRouterOperations.TryDeliverQuestCompletion`** — two comment-only hook sites with **`TODO(E9.M1)`** (no runtime emit); class xml summary cites NEO-130 telemetry anchor. - **`reward_delivery`:** after **`deliveryStore.TryRecord`** returns **`true`** on first-time delivery. - **`unlock_granted`:** stub after item + skill XP apply loops, before delivery event build — future **`UnlockGrant`** apply region; prototype bundles have no unlock rows. - **Docs:** `server/README.md` reward telemetry subsection; E7.M2 module + alignment register + E7M2 backlog updated. diff --git a/docs/reviews/2026-06-07-NEO-130.md b/docs/reviews/2026-06-07-NEO-130.md index 9da536e..3b19d21 100644 --- a/docs/reviews/2026-06-07-NEO-130.md +++ b/docs/reviews/2026-06-07-NEO-130.md @@ -2,7 +2,8 @@ **Date:** 2026-06-07 **Scope:** Branch `NEO-130-reward-telemetry-hooks` vs `origin/main` — commits `7493a6b` … `c461a82` -**Base:** `origin/main` +**Base:** `origin/main` +**Follow-up:** Review suggestions/nits addressed in a subsequent commit on the same branch. ## Verdict @@ -33,13 +34,13 @@ NEO-130 adds **comment-only** E9.M1 telemetry hook sites to **`RewardRouterOpera ## Suggestions -1. **E9.M1 `unlock_granted` wiring note** — The stub sits before **`TryRecord`**, so a future literal emit at this line would also run on the race-rollback path (grants applied then reverted). When **`UnlockGrant`** rows land, place per-row emits inside the apply loop and/or gate on durable commit (mirror **`reward_delivery`** after **`TryRecord`** success). The stub comment already says “once per applied UnlockGrant row”; consider adding one line that durable emit should not fire on rollback paths when E9.M1 implements the loop. +1. ~~**E9.M1 `unlock_granted` wiring note** — The stub sits before **`TryRecord`**, so a future literal emit at this line would also run on the race-rollback path (grants applied then reverted). When **`UnlockGrant`** rows land, place per-row emits inside the apply loop and/or gate on durable commit (mirror **`reward_delivery`** after **`TryRecord`** success). The stub comment already says “once per applied UnlockGrant row”; consider adding one line that durable emit should not fire on rollback paths when E9.M1 implements the loop.~~ **Done.** Stub comment now gates E9.M1 durable emit to apply-loop and/or post-**`TryRecord`** paths only (not race rollback). ## Nits -- Nit: **`unlock_granted`** stub runs even for empty bundles (no item/skill rows) — correct for a region marker; E9.M1 loop will simply emit nothing until unlock rows exist in content. +- ~~Nit: **`unlock_granted`** stub runs even for empty bundles (no item/skill rows) — correct for a region marker; E9.M1 loop will simply emit nothing until unlock rows exist in content.~~ **Done.** Acknowledged; no change. -- Nit: Plan technical approach lists class-summary update on **`RewardRouterOperations`**; shipped in xml summary — reconciliation bullet could mention it for parity with other stories (cosmetic doc only). +- ~~Nit: Plan technical approach lists class-summary update on **`RewardRouterOperations`**; shipped in xml summary — reconciliation bullet could mention it for parity with other stories (cosmetic doc only).~~ **Done.** Reconciliation bullet updated. ## Verification diff --git a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs index 96c9951..9090d58 100644 --- a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs +++ b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs @@ -140,6 +140,8 @@ public static class RewardRouterOperations // Prototype completionRewardBundle has item + skill XP rows only; no runtime unlock apply in Slice 2. // Planned payload fields: playerId, questId, unlock id / kind — finalize at E9.M1 wiring time. // Distinct from `perk_unlock` (PerkUnlockEngine, NEO-49) which may fire as a side effect of skill XP grants. + // E9.M1 wiring: place per-row emits inside the apply loop and/or after TryRecord success only — not on + // TryRecord race-rollback paths (grants applied then reverted below); mirror reward_delivery gating. var deliveredAt = timeProvider.GetUtcNow(); var deliveryEvent = new RewardDeliveryEvent(