NEO-121: Address review nits for quest telemetry hook sites.

Add schemaVersion to quest_start comment, thread playerId/questId into
DenyAccept, and merge duplicate deny snapshot helpers.

docs/plans/NEO-121-implementation-plan.md

@@ -56,7 +56,7 @@
 - **`quest_start`:** after successful **`TryActivate`** in **`TryAccept`**.
 - **`quest_step_complete`:** after successful **`TryAdvanceStep`** store commit.
 - **`quest_complete`:** after first-time **`TryMarkComplete`** commit (not idempotent replay).
-- **`quest_accept_denied`:** **`DenyAccept`** helper on all **`TryAccept`** deny paths.
+- **`quest_accept_denied`:** **`DenyAccept(playerId, questId, …)`** on all **`TryAccept`** deny paths; optional accept context on **`DenyFromProgressSnapshot`** for activate-failure rereads.
 - **Docs:** `server/README.md` quest telemetry subsection; E7.M1 module + alignment register + E7M1 backlog updated.
 - **Manual QA:** `docs/manual-qa/NEO-121.md`.
 

docs/reviews/2026-06-07-NEO-121.md

@@ -37,9 +37,9 @@ NEO-121 adds **comment-only** E9.M1 telemetry hook sites to **`QuestStateOperati
 
 ## Nits
 
-1. **Nit:** Plan technical approach lists optional **`schemaVersion`** in **`quest_start`** planned payload; the inline comment omits it. Harmless for comment-only work; consider adding for parity when touching the file again.
+1. ~~**Nit:** Plan technical approach lists optional **`schemaVersion`** in **`quest_start`** planned payload; the inline comment omits it. Harmless for comment-only work; consider adding for parity when touching the file again.~~ **Done.** Added optional **`schemaVersion`** to **`quest_start`** inline comment.
 
-2. **Nit:** **`DenyFromProgressSnapshotAccept`** duplicates **`DenyFromProgressSnapshot`** except for **`DenyAccept`** vs **`Deny`** — intentional funneling, but E9.M1 wiring may want **`playerId`** / **`questId`** threaded into **`DenyAccept`** (comment already documents those fields; implementer will need caller context).
+2. ~~**Nit:** **`DenyFromProgressSnapshotAccept`** duplicates **`DenyFromProgressSnapshot`** except for **`DenyAccept`** vs **`Deny`** — intentional funneling, but E9.M1 wiring may want **`playerId`** / **`questId`** threaded into **`DenyAccept`** (comment already documents those fields; implementer will need caller context).~~ **Done.** Merged into **`DenyFromProgressSnapshot`** with optional accept-deny context; **`DenyAccept`** now takes **`playerId`** and **`questId`** at all call sites.
 
 ## Verification
 

server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs

@@ -25,22 +25,22 @@ public static class QuestStateOperations
         if (!questRegistry.TryNormalizeKnown(questId, out var normalizedQuestId) ||
             !questRegistry.TryGetDefinition(normalizedQuestId, out var definition))
         {
-            return DenyAccept(QuestStateReasonCodes.UnknownQuest);
+            return DenyAccept(playerId, questId, QuestStateReasonCodes.UnknownQuest);
         }
 
         if (!progressStore.CanWritePlayer(playerId))
         {
-            return DenyAccept(QuestStateReasonCodes.UnknownPlayer);
+            return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.UnknownPlayer);
         }
 
         if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var existing))
         {
             if (existing.Status == QuestProgressStatus.Completed)
             {
-                return DenyAccept(QuestStateReasonCodes.AlreadyCompleted, existing);
+                return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.AlreadyCompleted, existing);
             }
 
-            return DenyAccept(QuestStateReasonCodes.AlreadyActive, existing);
+            return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.AlreadyActive, existing);
         }
 
         foreach (var prerequisiteId in definition.PrerequisiteQuestIds)
@@ -48,7 +48,7 @@ public static class QuestStateOperations
             if (!progressStore.TryGetProgress(playerId, prerequisiteId, out var prerequisite) ||
                 prerequisite.Status != QuestProgressStatus.Completed)
             {
-                return DenyAccept(QuestStateReasonCodes.PrerequisiteIncomplete);
+                return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.PrerequisiteIncomplete);
             }
         }
 
@@ -56,7 +56,8 @@ public static class QuestStateOperations
         {
             // --- Telemetry hook site (NEO-121): future E9.M1 catalog event `quest_start` ---
             // TODO(E9.M1): catalog emit — first activation only (deny paths excluded).
-            // Planned payload fields: playerId, questId, currentStepIndex (0). No ingest or ILogger here (comments-only).
+            // Planned payload fields: playerId, questId, currentStepIndex (0), optional schemaVersion.
+            // No ingest or ILogger here (comments-only).
 
             QuestObjectiveWiring.TryProcessInventoryHasItemForQuest(
                 playerId,
@@ -224,11 +225,11 @@ public static class QuestStateOperations
         string playerId,
         string questId,
         QuestStepState attemptSnapshot) =>
-        DenyFromProgressSnapshotAccept(attemptSnapshot, QuestStateReasonCodes.AlreadyActive)
+        DenyFromProgressSnapshot(attemptSnapshot, QuestStateReasonCodes.AlreadyActive, playerId, questId)
         ?? (progressStore.TryGetProgress(playerId, questId, out var reread)
-            ? DenyFromProgressSnapshotAccept(reread, QuestStateReasonCodes.AlreadyActive)
+            ? DenyFromProgressSnapshot(reread, QuestStateReasonCodes.AlreadyActive, playerId, questId)
             : null)
-        ?? DenyAccept(QuestStateReasonCodes.UnknownPlayer);
+        ?? DenyAccept(playerId, questId, QuestStateReasonCodes.UnknownPlayer);
 
     private static QuestStateOperationResult DenyAdvanceFailure(
         IPlayerQuestStateStore progressStore,
@@ -244,7 +245,9 @@ public static class QuestStateOperations
 
     private static QuestStateOperationResult? DenyFromProgressSnapshot(
         QuestStepState? snapshot,
-        string activeOrIntermediateReasonCode)
+        string activeOrIntermediateReasonCode,
+        string? acceptDenyPlayerId = null,
+        string? acceptDenyQuestId = null)
     {
         if (snapshot is null)
         {
@@ -253,34 +256,16 @@ public static class QuestStateOperations
 
         if (snapshot.Status == QuestProgressStatus.Completed)
         {
-            return Deny(QuestStateReasonCodes.AlreadyCompleted, snapshot);
+            return acceptDenyPlayerId is not null
+                ? DenyAccept(acceptDenyPlayerId, acceptDenyQuestId ?? string.Empty, QuestStateReasonCodes.AlreadyCompleted, snapshot)
+                : Deny(QuestStateReasonCodes.AlreadyCompleted, snapshot);
         }
 
         if (snapshot.Status == QuestProgressStatus.Active)
         {
-            return Deny(activeOrIntermediateReasonCode, snapshot);
+            return acceptDenyPlayerId is not null
-        }
+                ? DenyAccept(acceptDenyPlayerId, acceptDenyQuestId ?? string.Empty, activeOrIntermediateReasonCode, snapshot)
-
+                : Deny(activeOrIntermediateReasonCode, snapshot);
-        return null;
-    }
-
-    private static QuestStateOperationResult? DenyFromProgressSnapshotAccept(
-        QuestStepState? snapshot,
-        string activeOrIntermediateReasonCode)
-    {
-        if (snapshot is null)
-        {
-            return null;
-        }
-
-        if (snapshot.Status == QuestProgressStatus.Completed)
-        {
-            return DenyAccept(QuestStateReasonCodes.AlreadyCompleted, snapshot);
-        }
-
-        if (snapshot.Status == QuestProgressStatus.Active)
-        {
-            return DenyAccept(activeOrIntermediateReasonCode, snapshot);
         }
 
         return null;
@@ -289,11 +274,16 @@ public static class QuestStateOperations
     private static QuestStateOperationResult Success(QuestStepState snapshot) =>
         new(Success: true, ReasonCode: null, Snapshot: snapshot);
 
-    private static QuestStateOperationResult DenyAccept(string reasonCode, QuestStepState? snapshot = null)
+    private static QuestStateOperationResult DenyAccept(
+        string playerId,
+        string questId,
+        string reasonCode,
+        QuestStepState? snapshot = null)
     {
         // --- Telemetry hook site (NEO-121): future E9.M1 catalog event `quest_accept_denied` ---
         // TODO(E9.M1): catalog emit — reasonCode from QuestStateReasonCodes (unknown_quest, prerequisite_incomplete, etc.).
-        // Planned payload fields: playerId, questId, reasonCode. No ingest or ILogger here (comments-only).
+        // Planned payload fields: playerId, questId, reasonCode (caller context threaded for E9.M1 wiring).
+        // No ingest or ILogger here (comments-only).
 
         return Deny(reasonCode, snapshot);
     }