Merge pull request #160 from ViPro-Technologies/NEO-121-quest-telemetry-hooks
NEO-121: Quest telemetry hook sites (comment-only)pull/162/head
GitHub
commit
a3932752b8
|
|
@ -7,7 +7,7 @@
|
|||
| **Module ID** | E7.M1 |
|
||||
| **Epic** | [Epic 7 — Quest / Faction](../epics/epic_07_quest_faction.md) |
|
||||
| **Stage target** | Prototype |
|
||||
| **Status** | Planned — Slice 1 backlog [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md): **E7M1-01** [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) catalog **landed** (schemas + CI); **E7M1-02** [NEO-113](https://linear.app/neon-sprawl/issue/NEO-113) server load **landed**; **E7M1-03** [NEO-114](https://linear.app/neon-sprawl/issue/NEO-114) registry **landed**; **E7M1-04** [NEO-115](https://linear.app/neon-sprawl/issue/NEO-115) HTTP read **landed**; **E7M1-05** [NEO-116](https://linear.app/neon-sprawl/issue/NEO-116) player quest state store **landed**; **E7M1-06** [NEO-117](https://linear.app/neon-sprawl/issue/NEO-117) **`QuestStateOperations`** **landed**; **E7M1-07** [NEO-118](https://linear.app/neon-sprawl/issue/NEO-118) **`QuestObjectiveWiring`** **landed**; **E7M1-08** [NEO-119](https://linear.app/neon-sprawl/issue/NEO-119) per-player GET **landed**; **E7M1-09** [NEO-120](https://linear.app/neon-sprawl/issue/NEO-120) accept POST **landed**; client from **E7M1-11** [NEO-122](https://linear.app/neon-sprawl/issue/NEO-122) → capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123) |
|
||||
| **Status** | Planned — Slice 1 backlog [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md): **E7M1-01** [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) catalog **landed** (schemas + CI); **E7M1-02** [NEO-113](https://linear.app/neon-sprawl/issue/NEO-113) server load **landed**; **E7M1-03** [NEO-114](https://linear.app/neon-sprawl/issue/NEO-114) registry **landed**; **E7M1-04** [NEO-115](https://linear.app/neon-sprawl/issue/NEO-115) HTTP read **landed**; **E7M1-05** [NEO-116](https://linear.app/neon-sprawl/issue/NEO-116) player quest state store **landed**; **E7M1-06** [NEO-117](https://linear.app/neon-sprawl/issue/NEO-117) **`QuestStateOperations`** **landed**; **E7M1-07** [NEO-118](https://linear.app/neon-sprawl/issue/NEO-118) **`QuestObjectiveWiring`** **landed**; **E7M1-08** [NEO-119](https://linear.app/neon-sprawl/issue/NEO-119) per-player GET **landed**; **E7M1-09** [NEO-120](https://linear.app/neon-sprawl/issue/NEO-120) accept POST **landed**; **E7M1-10** [NEO-121](https://linear.app/neon-sprawl/issue/NEO-121) telemetry hooks **landed**; client from **E7M1-11** [NEO-122](https://linear.app/neon-sprawl/issue/NEO-122) → capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123) |
|
||||
| **Linear** | Label **`E7.M1`** · [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md) |
|
||||
|
||||
## Purpose
|
||||
|
|
@ -92,12 +92,15 @@ The **first shipped quest spine** is **frozen** for prototype tuning until a del
|
|||
|
||||
**Quest accept POST (NEO-120):** **`POST /game/players/{id}/quests/{questId}/accept`** — `QuestAcceptApi` + DTOs; wires **`QuestStateOperations.TryAccept`**; structured **`accepted`** / **`reasonCode`** / optional **`quest`** row ([NEO-120](../../plans/NEO-120-implementation-plan.md)); [server README — Quest accept POST (NEO-120)](../../../server/README.md#quest-accept-post-neo-120); Bruno accept spine in `bruno/neon-sprawl-server/quest-progress/`.
|
||||
|
||||
**Quest telemetry hooks (NEO-121):** comment-only **`quest_start`**, **`quest_step_complete`**, **`quest_complete`**, **`quest_accept_denied`** hook sites in **`QuestStateOperations`** ([NEO-121](../../plans/NEO-121-implementation-plan.md)); [server README — Quest telemetry hooks (NEO-121)](../../../server/README.md#quest-telemetry-hooks-neo-121).
|
||||
|
||||
**Reward policy (Slice 1):** Quest completion updates **`QuestStepState` only** — no duplicate item/XP grants. Gather/craft/encounter paths keep existing payouts; [E7.M2](E7_M2_RewardAndUnlockRouter.md) adds **`QuestRewardBundle`** apply in Slice 2.
|
||||
|
||||
## Risks and telemetry
|
||||
|
||||
- Scripting weight: keep `QuestDef` data-first; templates in decomposition/tooling path per epic.
|
||||
- Double-claim: design for idempotency before E7.M2; lint bundles vs E6.M4 / E3.M5.
|
||||
- Epic 7 Slice 1 catalog events (comment-only until E9.M1): **`quest_start`**, **`quest_step_complete`**, **`quest_complete`**, **`quest_accept_denied`** — anchored in **`QuestStateOperations`** (NEO-121).
|
||||
|
||||
## Source anchors
|
||||
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -109,7 +109,7 @@ Gameplay **content and curves** default to **boot load** with optional **dev-onl
|
|||
|---|---|---|---|---|---|
|
||||
| E7.M1 | QuestStateMachine | E3.M2, E5.M1 | QuestDef, QuestStepState, QuestStateTransition | Prototype | Planned |
|
||||
|
||||
**E7.M1 note:** Epic 7 **Slice 1** backlog in Linear ([Epic 7 — Questing, Narrative, and Faction Reputation](https://linear.app/neon-sprawl/project/epic-7-questing-narrative-and-faction-reputation-a9416783ceee)): [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) → [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123); label **`E7.M1`**. See [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md), [E7_M1_QuestStateMachine.md](E7_M1_QuestStateMachine.md). Upstream: E3.M1 gather **Ready**, E3.M2 craft **Ready**, E5.M3 encounter **Ready**. Client capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123). **E7M1-01 / NEO-112** catalog landed (schemas + CI); **E7M1-02 / NEO-113** server load landed; **E7M1-03 / NEO-114** registry landed ([NEO-114 plan](../../plans/NEO-114-implementation-plan.md)); **E7M1-04 / NEO-115** HTTP read landed ([NEO-115 plan](../../plans/NEO-115-implementation-plan.md)); **E7M1-05 / NEO-116** player quest state store landed ([NEO-116 plan](../../plans/NEO-116-implementation-plan.md)); **E7M1-06 / NEO-117** **`QuestStateOperations`** landed ([NEO-117 plan](../../plans/NEO-117-implementation-plan.md)); **E7M1-07 / NEO-118** **`QuestObjectiveWiring`** landed ([NEO-118 plan](../../plans/NEO-118-implementation-plan.md)); **E7M1-08 / NEO-119** per-player GET landed ([NEO-119 plan](../../plans/NEO-119-implementation-plan.md)); **E7M1-09 / NEO-120** accept POST landed ([NEO-120 plan](../../plans/NEO-120-implementation-plan.md)); register row stays **Planned** until client capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123).
|
||||
**E7.M1 note:** Epic 7 **Slice 1** backlog in Linear ([Epic 7 — Questing, Narrative, and Faction Reputation](https://linear.app/neon-sprawl/project/epic-7-questing-narrative-and-faction-reputation-a9416783ceee)): [NEO-112](https://linear.app/neon-sprawl/issue/NEO-112) → [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123); label **`E7.M1`**. See [E7M1-prototype-backlog.md](../../plans/E7M1-prototype-backlog.md), [E7_M1_QuestStateMachine.md](E7_M1_QuestStateMachine.md). Upstream: E3.M1 gather **Ready**, E3.M2 craft **Ready**, E5.M3 encounter **Ready**. Client capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123). **E7M1-01 / NEO-112** catalog landed (schemas + CI); **E7M1-02 / NEO-113** server load landed; **E7M1-03 / NEO-114** registry landed ([NEO-114 plan](../../plans/NEO-114-implementation-plan.md)); **E7M1-04 / NEO-115** HTTP read landed ([NEO-115 plan](../../plans/NEO-115-implementation-plan.md)); **E7M1-05 / NEO-116** player quest state store landed ([NEO-116 plan](../../plans/NEO-116-implementation-plan.md)); **E7M1-06 / NEO-117** **`QuestStateOperations`** landed ([NEO-117 plan](../../plans/NEO-117-implementation-plan.md)); **E7M1-07 / NEO-118** **`QuestObjectiveWiring`** landed ([NEO-118 plan](../../plans/NEO-118-implementation-plan.md)); **E7M1-08 / NEO-119** per-player GET landed ([NEO-119 plan](../../plans/NEO-119-implementation-plan.md)); **E7M1-09 / NEO-120** accept POST landed ([NEO-120 plan](../../plans/NEO-120-implementation-plan.md)); **E7M1-10 / NEO-121** comment-only quest telemetry hook sites landed ([NEO-121 plan](../../plans/NEO-121-implementation-plan.md)); register row stays **Planned** until client capstone **E7M1-12** [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123).
|
||||
| E7.M2 | RewardAndUnlockRouter | E2.M2, E3.M3, E7.M1 | QuestRewardBundle, UnlockGrant, RewardDeliveryEvent | Prototype | Planned |
|
||||
| E7.M3 | FactionReputationLedger | E7.M1 | FactionStanding, ReputationDelta, FactionGateRule | Pre-production | Planned |
|
||||
| E7.M4 | ContractMissionGenerator | E4.M1, E5.M3, E7.M3 | ContractTemplate, ContractSeed, ContractOutcome | Pre-production | Planned |
|
||||
|
|
|
|||
|
|
@ -0,0 +1,35 @@
|
|||
# Manual QA — NEO-121 (quest telemetry hook sites)
|
||||
|
||||
Reference: [implementation plan](../plans/NEO-121-implementation-plan.md), [NEO-117 implementation plan](../plans/NEO-117-implementation-plan.md) (quest operations), [NEO-118 implementation plan](../plans/NEO-118-implementation-plan.md) (objective wiring).
|
||||
|
||||
## Preconditions
|
||||
|
||||
- Run `NeonSprawl.Server` (`dotnet run` from `server/NeonSprawl.Server`; default `http://localhost:5253`) if exercising Bruno; not required for code review only.
|
||||
- This story adds **comments only**; quest behavior matches NEO-117/118/119/120.
|
||||
|
||||
## Code review check
|
||||
|
||||
- [ ] Open `server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs`.
|
||||
- [ ] Confirm **`quest_start`** comment block immediately after successful **`TryActivate`** in **`TryAccept`** (before objective wiring).
|
||||
- [ ] Confirm **`quest_step_complete`** comment block after successful **`TryAdvanceStep`** store commit.
|
||||
- [ ] Confirm **`quest_complete`** comment block after first-time **`TryMarkComplete`** commit (not on idempotent **`completed`** early return).
|
||||
- [ ] Confirm **`quest_accept_denied`** comment in **`DenyAccept`** (used by all **`TryAccept`** deny paths).
|
||||
- [ ] Confirm **`TODO(E9.M1)`** markers and planned payload fields; **no** new `ILogger` or metrics calls.
|
||||
- [ ] Confirm **no** duplicate hook comments in **`QuestAcceptApi`**, **`QuestProgressApi`**, or **`QuestObjectiveWiring`**.
|
||||
- [ ] Open [server README — Quest telemetry hooks (NEO-121)](../../server/README.md#quest-telemetry-hooks-neo-121) — NEO-121 subsection present with event table.
|
||||
|
||||
## Regression (optional)
|
||||
|
||||
```bash
|
||||
cd server && dotnet test NeonSprawl.sln
|
||||
```
|
||||
|
||||
Expect all tests pass (comments-only change).
|
||||
|
||||
## Bruno (optional sanity)
|
||||
|
||||
From `bruno/neon-sprawl-server/quest-progress/` against a running server:
|
||||
|
||||
- **Get quest progress default** — all quests **`not_started`** unchanged.
|
||||
- **Post accept gather intro** — **`accepted: true`** unchanged.
|
||||
- **Post accept duplicate** — **`accepted: false`**, **`reasonCode: already_active`** unchanged.
|
||||
|
|
@ -315,8 +315,10 @@ Working backlog for **Epic 7 — Slice 1** ([quest core and persistence](../deco
|
|||
|
||||
**Acceptance criteria**
|
||||
|
||||
- [ ] Hook sites exist at accept, step complete, quest complete paths.
|
||||
- [ ] Epic 7 Slice 1 telemetry vocabulary documented.
|
||||
- [x] Hook sites exist at accept, step complete, quest complete paths.
|
||||
- [x] Epic 7 Slice 1 telemetry vocabulary documented.
|
||||
|
||||
**Landed (NEO-121):** comment-only **`quest_start`**, **`quest_step_complete`**, **`quest_complete`**, **`quest_accept_denied`** in **`QuestStateOperations`**; [server README — Quest telemetry hooks (NEO-121)](../../server/README.md#quest-telemetry-hooks-neo-121); plan [NEO-121](../../plans/NEO-121-implementation-plan.md).
|
||||
|
||||
**Client counterpart:** none (infrastructure-only).
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,126 @@
|
|||
# NEO-121 — Implementation plan
|
||||
|
||||
## Story reference
|
||||
|
||||
| Field | Value |
|
||||
|--------|--------|
|
||||
| **Key** | NEO-121 |
|
||||
| **Title** | E7M1-10: Quest telemetry hook sites (comment-only) |
|
||||
| **Linear** | https://linear.app/neon-sprawl/issue/NEO-121/e7m1-10-quest-telemetry-hook-sites-comment-only |
|
||||
| **Module** | [E7.M1 — QuestStateMachine](../decomposition/modules/E7_M1_QuestStateMachine.md) · Epic 7 Slice 1 · backlog **E7M1-10** |
|
||||
| **Branch** | `NEO-121-quest-telemetry-hooks` |
|
||||
| **Precursor** | [NEO-117](https://linear.app/neon-sprawl/issue/NEO-117) — `QuestStateOperations` (**landed on `main`**); [NEO-118](https://linear.app/neon-sprawl/issue/NEO-118) — `QuestObjectiveWiring` (**landed on `main`**); [NEO-119](https://linear.app/neon-sprawl/issue/NEO-119) / [NEO-120](https://linear.app/neon-sprawl/issue/NEO-120) — quest progress GET + accept POST (**landed on `main`**) |
|
||||
| **Pattern** | [NEO-64](NEO-64-implementation-plan.md) / [NEO-56](NEO-56-implementation-plan.md) / [NEO-49](NEO-49-implementation-plan.md) — comment-only engine anchors + `TODO(E9.M1)`; engine-only (no duplicate API-layer hooks) |
|
||||
| **Client counterpart** | none (infrastructure-only per [E7M1-10](E7M1-prototype-backlog.md#e7m1-10--quest-telemetry-hook-sites-comment-only)) |
|
||||
|
||||
## Kickoff clarifications
|
||||
|
||||
**No clarifications needed.** [E7M1-10](E7M1-prototype-backlog.md#e7m1-10--quest-telemetry-hook-sites-comment-only) backlog, Linear AC, landed quest operations/wiring (NEO-117/118), and comment-only telemetry precedents (NEO-49/56/64/71) settle scope:
|
||||
|
||||
- **Runtime behavior:** comments-only + `TODO(E9.M1)` — no production ingest, no `ILogger` (NEO-49/56 precedent).
|
||||
- **Hook anchor:** **`QuestStateOperations`** only for **`quest_start`**, **`quest_step_complete`**, **`quest_complete`** — HTTP accept POST and objective wiring delegate to the engine; no duplicate API-layer or wiring-layer hooks (NEO-56 engine-only precedent). **`QuestObjectiveWiring`** triggers step advance/complete by calling **`QuestStateOperations`**; hooks live at mutation commit sites.
|
||||
- **`quest_start` timing:** after successful **`TryActivate`** in **`TryAccept`** (first activation only; deny paths excluded).
|
||||
- **`quest_step_complete` timing:** after successful **`TryAdvanceStep`** store commit (covers wiring-driven multi-step chains — one emit per committed advance).
|
||||
- **`quest_complete` timing:** after first-time **`TryMarkComplete`** store commit; **not** on idempotent replay when row is already **`completed`** (NEO-117 idempotency + NEO-49 “idempotent reevaluation produces none” precedent).
|
||||
- **Optional deny hook:** include **`quest_accept_denied`** comment in **`TryAccept`** **`Deny`** returns with stable **`reasonCode`** (NEO-56 **`inventory_transfer_denied`** pattern; backlog lists as optional — adopt for accept funnel parity).
|
||||
- **Manual QA doc:** **`docs/manual-qa/NEO-121.md`** — NEO-64/49 comment-only verification pattern (no behavior/API change).
|
||||
|
||||
## Goal, scope, and out-of-scope
|
||||
|
||||
**Goal:** Document and place **comment-only hook sites** on the server-authoritative quest state path for future E9.M1 catalog events **`quest_start`**, **`quest_step_complete`**, and **`quest_complete`**. Align with Epic 7 Slice 1 telemetry vocabulary ([E7_M1 module](../decomposition/modules/E7_M1_QuestStateMachine.md#risks-and-telemetry)).
|
||||
|
||||
**In scope (from Linear + [E7M1-10](E7M1-prototype-backlog.md#e7m1-10--quest-telemetry-hook-sites-comment-only)):**
|
||||
|
||||
- Hook sites in **`QuestStateOperations`** at accept, step advance, and complete commit paths.
|
||||
- Reserved deny hook **`quest_accept_denied`** (comment-only) on **`TryAccept`** deny paths.
|
||||
- **`server/README.md`** quest telemetry subsection.
|
||||
- **`E7_M1_QuestStateMachine.md`** implementation snapshot note.
|
||||
- **`docs/manual-qa/NEO-121.md`** — verify comments + no behavior change.
|
||||
- E7M1 backlog + alignment register updates when implementation completes.
|
||||
|
||||
**Out of scope (from Linear + backlog):**
|
||||
|
||||
- E9.M1 ingest pipeline; runtime telemetry emit.
|
||||
- `ILogger` / metrics / dev-only log lines.
|
||||
- Duplicate hook comments in **`QuestAcceptApi`**, **`QuestProgressApi`**, or **`QuestObjectiveWiring`** (engine-only precedent).
|
||||
- Client HUD ([NEO-122](https://linear.app/neon-sprawl/issue/NEO-122)).
|
||||
|
||||
## Acceptance criteria checklist
|
||||
|
||||
- [x] Hook sites exist at accept, step complete, quest complete paths.
|
||||
- [x] Epic 7 Slice 1 telemetry vocabulary documented in `server/README.md`.
|
||||
|
||||
## Implementation reconciliation (shipped)
|
||||
|
||||
- **Engine anchor:** **`QuestStateOperations`** — four comment-only hook sites with **`TODO(E9.M1)`** (no runtime emit).
|
||||
- **`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(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`.
|
||||
|
||||
## Technical approach
|
||||
|
||||
1. **Authoritative surface:** **`QuestStateOperations`** — all quest lifecycle mutations funnel here. **`QuestObjectiveWiring`** calls **`TryAdvanceStep`** / **`TryMarkComplete`**; **`QuestAcceptApi`** calls **`TryAccept`**.
|
||||
|
||||
2. **Hook site A — `quest_start`:** In **`TryAccept`**, immediately after **`progressStore.TryActivate`** returns **`true`** (before objective wiring refresh):
|
||||
- Comment block names future E9.M1 event **`quest_start`**.
|
||||
- **`TODO(E9.M1): catalog emit`** (NEO-49 style).
|
||||
- Planned payload fields: `playerId`, `questId`, `currentStepIndex` (0), optional `schemaVersion`.
|
||||
|
||||
3. **Hook site B — `quest_step_complete`:** In **`TryAdvanceStep`**, immediately after **`progressStore.TryAdvanceStep`** returns **`true`**:
|
||||
- Comment block names future E9.M1 event **`quest_step_complete`**.
|
||||
- **`TODO(E9.M1): catalog emit`** — emit once per committed step advance (chain quests may emit multiple in one wiring pass).
|
||||
- Planned payload fields: `playerId`, `questId`, `completedStepIndex` (prior index), `newStepIndex`, optional step id from catalog at E9.M1 wiring time.
|
||||
|
||||
4. **Hook site C — `quest_complete`:** In **`TryMarkComplete`**, immediately after **`progressStore.TryMarkComplete`** returns **`true`** on the first-time completion path (after the early idempotent **`completed`** return):
|
||||
- Comment block names future E9.M1 event **`quest_complete`**.
|
||||
- **`TODO(E9.M1): catalog emit`** — not on idempotent replay.
|
||||
- Planned payload fields: `playerId`, `questId`, `completedAt`, optional total step count.
|
||||
|
||||
5. **Hook site D (optional) — `quest_accept_denied`:** On **`TryAccept`** deny returns (central **`Deny`** helper and/or each deny branch before return):
|
||||
- Comment block names future E9.M1 event **`quest_accept_denied`**.
|
||||
- **`TODO(E9.M1): catalog emit`** with stable **`reasonCode`** (`unknown_quest`, `prerequisite_incomplete`, `already_active`, `already_completed`, etc.).
|
||||
- Planned payload fields: `playerId`, `questId`, `reasonCode`.
|
||||
|
||||
6. **Cross-references (minimal):**
|
||||
- Update **`QuestStateOperations`** class summary → NEO-121 telemetry hook anchor.
|
||||
- **`server/README.md`:** new **Quest telemetry hooks (NEO-121)** subsection after quest accept POST (NEO-120).
|
||||
- **`E7_M1_QuestStateMachine.md`:** NEO-121 landed line under Risks and telemetry.
|
||||
- **`documentation_and_implementation_alignment.md`** + **`module_dependency_register.md`:** E7.M1 row — E7M1-10 when complete.
|
||||
- **`E7M1-prototype-backlog.md`:** E7M1-10 checkboxes + landed note.
|
||||
|
||||
7. **Accept + instant wiring:** **`TryAccept`** may trigger **`quest_start`**, then wiring may emit **`quest_step_complete`** / **`quest_complete`** in the same call stack — order is preserved by code flow (activate → wiring → advance/complete).
|
||||
|
||||
## Files to add
|
||||
|
||||
| Path | Purpose |
|
||||
|------|---------|
|
||||
| `docs/manual-qa/NEO-121.md` | Manual QA: verify comment blocks + no behavior/API change. |
|
||||
| `docs/plans/NEO-121-implementation-plan.md` | This plan. |
|
||||
|
||||
## Files to modify
|
||||
|
||||
| Path | Rationale |
|
||||
|------|-----------|
|
||||
| `server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs` | **NEO-121:** comment-only **`quest_start`**, **`quest_step_complete`**, **`quest_complete`**, **`quest_accept_denied`** hook sites at mutation/deny commit paths. |
|
||||
| `server/README.md` | Document quest telemetry hook placement and Epic 7 Slice 1 vocabulary (NEO-121). |
|
||||
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | Implementation snapshot: NEO-121 telemetry hooks landed. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E7.M1 row: NEO-121 hook sites. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | E7.M1 note — E7M1-10 complete when story lands. |
|
||||
| `docs/plans/E7M1-prototype-backlog.md` | E7M1-10 checkboxes + landed note. |
|
||||
|
||||
## Tests
|
||||
|
||||
| Test file | What it covers |
|
||||
|-----------|----------------|
|
||||
| *(none added or changed)* | Comments-only change; no behavior or wire contract change. Regression: `dotnet test NeonSprawl.sln` (existing **`QuestStateOperationsTests`**, **`QuestObjectiveWiringTests`**, quest HTTP integration tests unchanged). Manual verification: **`docs/manual-qa/NEO-121.md`**. |
|
||||
|
||||
## Open questions / risks
|
||||
|
||||
| Question / risk | Agent recommendation | Status |
|
||||
|-----------------|----------------------|--------|
|
||||
| **Duplicate emits on accept + wiring same tick** | Expected — **`quest_start`** then zero or more **`quest_step_complete`** / **`quest_complete`**; E9.M1 can correlate by session/time or sample funnels. | **adopted** |
|
||||
| **`quest_accept_denied` scope** | Include on all **`TryAccept`** deny paths with **`reasonCode`** (NEO-56 deny pattern). | **adopted** |
|
||||
| **Module Ready vs In Progress** | Leave E7.M1 **In Progress** until client capstones NEO-122/123 land; only note E7M1-10 complete in register. | **adopted** |
|
||||
|
|
@ -0,0 +1,57 @@
|
|||
# Code review — NEO-121 (E7M1-10)
|
||||
|
||||
**Date:** 2026-06-07
|
||||
**Scope:** Branch `NEO-121-quest-telemetry-hooks` vs `origin/main` — commits `72b98a6` … `705a7cd`
|
||||
**Base:** `origin/main`
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits**
|
||||
|
||||
## Summary
|
||||
|
||||
NEO-121 adds **comment-only** E9.M1 telemetry hook sites to **`QuestStateOperations`** for Epic 7 Slice 1 events **`quest_start`**, **`quest_step_complete`**, **`quest_complete`**, and **`quest_accept_denied`**. Hooks are placed at mutation commit sites (after successful store writes) and accept-deny paths funnel through a new **`DenyAccept`** helper (mirroring NEO-56 **`inventory_transfer_denied`**). **`DenyFromProgressSnapshotAccept`** ensures activate-failure rereads also hit the deny hook. No runtime behavior, wire contract, or API surface changes — only comments plus accept-path deny routing that still delegates to **`Deny`**. Documentation is thorough: implementation plan reconciliation, **`server/README.md`** event table, E7.M1 module snapshot, alignment register, backlog checkboxes, and **`docs/manual-qa/NEO-121.md`**. Server-only infrastructure slice; Godot HUD correctly deferred to NEO-122. Risk is negligible.
|
||||
|
||||
## Documentation checked
|
||||
|
||||
| Path | Result |
|
||||
|------|--------|
|
||||
| `docs/plans/NEO-121-implementation-plan.md` | **Matches** — kickoff decisions adopted (engine-only anchors, deny helper, idempotent complete exclusion); acceptance checklist checked; reconciliation accurate. |
|
||||
| `docs/plans/E7M1-prototype-backlog.md` (E7M1-10) | **Matches** — acceptance checkboxes checked; landed note + README link. |
|
||||
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — NEO-121 implementation snapshot + Risks and telemetry bullet. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7M1-10 / NEO-121 landed note appended. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7M1-10 note appended; row stays **Planned** until NEO-123 (per plan). |
|
||||
| `server/README.md` | **Matches** — Quest telemetry hooks subsection with event table and delegation note. |
|
||||
| `docs/manual-qa/NEO-121.md` | **Matches** — comment-only verification checklist; optional Bruno/regression steps. |
|
||||
| Full-stack epic decomposition | **N/A** — infrastructure-only; plan explicitly documents no client counterpart. |
|
||||
|
||||
**Register / tracking:** E7.M1 correctly remains **Planned** until client capstone NEO-123; E7M1-10 completion noted in register note only — no further status change required.
|
||||
|
||||
## Blocking issues
|
||||
|
||||
(none)
|
||||
|
||||
## Suggestions
|
||||
|
||||
(none)
|
||||
|
||||
## 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.~~ **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).~~ **Done.** Merged into **`DenyFromProgressSnapshot`** with optional accept-deny context; **`DenyAccept`** now takes **`playerId`** and **`questId`** at all call sites.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# Full regression (691 tests passed at review time)
|
||||
dotnet test NeonSprawl.sln
|
||||
|
||||
# Quest-focused subset
|
||||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Quest"
|
||||
```
|
||||
|
||||
Manual (per `docs/manual-qa/NEO-121.md`):
|
||||
|
||||
- Walk **`QuestStateOperations.cs`** hook comment blocks and confirm no duplicate hooks in **`QuestAcceptApi`**, **`QuestProgressApi`**, or **`QuestObjectiveWiring`**.
|
||||
- Optional Bruno sanity in `bruno/neon-sprawl-server/quest-progress/` — accept happy path + duplicate deny unchanged.
|
||||
|
|
@ -5,6 +5,8 @@ namespace NeonSprawl.Server.Game.Quests;
|
|||
/// <summary>
|
||||
/// Server-authoritative quest accept, step advance, and complete with reason codes (NEO-117).
|
||||
/// Objective wiring: E7M1-07 (NEO-118). HTTP: E7M1-08/09 (NEO-119/NEO-120).
|
||||
/// Telemetry hook sites: E7M1-10 (NEO-121) — comment-only <c>quest_start</c>, <c>quest_step_complete</c>,
|
||||
/// <c>quest_complete</c>, <c>quest_accept_denied</c> for future E9.M1.
|
||||
/// </summary>
|
||||
public static class QuestStateOperations
|
||||
{
|
||||
|
|
@ -23,22 +25,22 @@ public static class QuestStateOperations
|
|||
if (!questRegistry.TryNormalizeKnown(questId, out var normalizedQuestId) ||
|
||||
!questRegistry.TryGetDefinition(normalizedQuestId, out var definition))
|
||||
{
|
||||
return Deny(QuestStateReasonCodes.UnknownQuest);
|
||||
return DenyAccept(playerId, questId, QuestStateReasonCodes.UnknownQuest);
|
||||
}
|
||||
|
||||
if (!progressStore.CanWritePlayer(playerId))
|
||||
{
|
||||
return Deny(QuestStateReasonCodes.UnknownPlayer);
|
||||
return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.UnknownPlayer);
|
||||
}
|
||||
|
||||
if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var existing))
|
||||
{
|
||||
if (existing.Status == QuestProgressStatus.Completed)
|
||||
{
|
||||
return Deny(QuestStateReasonCodes.AlreadyCompleted, existing);
|
||||
return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.AlreadyCompleted, existing);
|
||||
}
|
||||
|
||||
return Deny(QuestStateReasonCodes.AlreadyActive, existing);
|
||||
return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.AlreadyActive, existing);
|
||||
}
|
||||
|
||||
foreach (var prerequisiteId in definition.PrerequisiteQuestIds)
|
||||
|
|
@ -46,12 +48,17 @@ public static class QuestStateOperations
|
|||
if (!progressStore.TryGetProgress(playerId, prerequisiteId, out var prerequisite) ||
|
||||
prerequisite.Status != QuestProgressStatus.Completed)
|
||||
{
|
||||
return Deny(QuestStateReasonCodes.PrerequisiteIncomplete);
|
||||
return DenyAccept(playerId, normalizedQuestId, QuestStateReasonCodes.PrerequisiteIncomplete);
|
||||
}
|
||||
}
|
||||
|
||||
if (progressStore.TryActivate(playerId, normalizedQuestId, out var snapshot))
|
||||
{
|
||||
// --- 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), optional schemaVersion.
|
||||
// No ingest or ILogger here (comments-only).
|
||||
|
||||
QuestObjectiveWiring.TryProcessInventoryHasItemForQuest(
|
||||
playerId,
|
||||
normalizedQuestId,
|
||||
|
|
@ -120,6 +127,11 @@ public static class QuestStateOperations
|
|||
|
||||
if (progressStore.TryAdvanceStep(playerId, normalizedQuestId, newStepIndex, out var snapshot))
|
||||
{
|
||||
// --- Telemetry hook site (NEO-121): future E9.M1 catalog event `quest_step_complete` ---
|
||||
// TODO(E9.M1): catalog emit — once per committed step advance (chain quests may emit multiple per wiring pass).
|
||||
// Planned payload fields: playerId, questId, completedStepIndex (prior index), newStepIndex;
|
||||
// optional step id from catalog at E9.M1 wiring time. No ingest or ILogger here (comments-only).
|
||||
|
||||
QuestObjectiveWiring.TryProcessInventoryHasItemForQuest(
|
||||
playerId,
|
||||
normalizedQuestId,
|
||||
|
|
@ -180,6 +192,11 @@ public static class QuestStateOperations
|
|||
|
||||
if (progressStore.TryMarkComplete(playerId, normalizedQuestId, timeProvider.GetUtcNow(), out var snapshot))
|
||||
{
|
||||
// --- Telemetry hook site (NEO-121): future E9.M1 catalog event `quest_complete` ---
|
||||
// TODO(E9.M1): catalog emit — first-time completion only; idempotent replay returns above without hook.
|
||||
// Planned payload fields: playerId, questId, completedAt; optional total step count.
|
||||
// No ingest or ILogger here (comments-only).
|
||||
|
||||
return Success(snapshot);
|
||||
}
|
||||
|
||||
|
|
@ -208,11 +225,11 @@ public static class QuestStateOperations
|
|||
string playerId,
|
||||
string questId,
|
||||
QuestStepState attemptSnapshot) =>
|
||||
DenyFromProgressSnapshot(attemptSnapshot, QuestStateReasonCodes.AlreadyActive)
|
||||
DenyFromProgressSnapshot(attemptSnapshot, QuestStateReasonCodes.AlreadyActive, playerId, questId)
|
||||
?? (progressStore.TryGetProgress(playerId, questId, out var reread)
|
||||
? DenyFromProgressSnapshot(reread, QuestStateReasonCodes.AlreadyActive)
|
||||
? DenyFromProgressSnapshot(reread, QuestStateReasonCodes.AlreadyActive, playerId, questId)
|
||||
: null)
|
||||
?? Deny(QuestStateReasonCodes.UnknownPlayer);
|
||||
?? DenyAccept(playerId, questId, QuestStateReasonCodes.UnknownPlayer);
|
||||
|
||||
private static QuestStateOperationResult DenyAdvanceFailure(
|
||||
IPlayerQuestStateStore progressStore,
|
||||
|
|
@ -228,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)
|
||||
{
|
||||
|
|
@ -237,12 +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;
|
||||
|
|
@ -251,6 +274,20 @@ public static class QuestStateOperations
|
|||
private static QuestStateOperationResult Success(QuestStepState snapshot) =>
|
||||
new(Success: true, ReasonCode: null, Snapshot: snapshot);
|
||||
|
||||
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 (caller context threaded for E9.M1 wiring).
|
||||
// No ingest or ILogger here (comments-only).
|
||||
|
||||
return Deny(reasonCode, snapshot);
|
||||
}
|
||||
|
||||
private static QuestStateOperationResult Deny(string reasonCode, QuestStepState? snapshot = null) =>
|
||||
new(Success: false, ReasonCode: reasonCode, Snapshot: snapshot);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -250,6 +250,19 @@ Sample success:
|
|||
{"schemaVersion":1,"accepted":true,"quest":{"questId":"prototype_quest_gather_intro","status":"active","currentStepIndex":0,"objectiveCounters":{}}}
|
||||
```
|
||||
|
||||
### Quest telemetry hooks (NEO-121)
|
||||
|
||||
Comment-only hook sites in **`QuestStateOperations`** for future E9.M1 catalog events ([Epic 7 Slice 1](../../docs/decomposition/modules/E7_M1_QuestStateMachine.md#risks-and-telemetry)). **`TODO(E9.M1)`** — no production ingest. HTTP **`POST …/accept`** and **`QuestObjectiveWiring`** delegate to the engine only (no duplicate API-layer or wiring-layer hooks).
|
||||
|
||||
| Event | Anchor | When |
|
||||
|-------|--------|------|
|
||||
| **`quest_start`** | **`TryAccept`** | After successful **`TryActivate`** (first activation only). |
|
||||
| **`quest_step_complete`** | **`TryAdvanceStep`** | After successful store commit (one emit per advance; chain quests may emit multiple per wiring pass). |
|
||||
| **`quest_complete`** | **`TryMarkComplete`** | After first-time completion commit (not idempotent replay when already **`completed`**). |
|
||||
| **`quest_accept_denied`** | **`TryAccept`** deny paths | Every structured accept deny with stable **`reasonCode`**. |
|
||||
|
||||
Manual QA: [`docs/manual-qa/NEO-121.md`](../../docs/manual-qa/NEO-121.md); plan: [NEO-121 implementation plan](../../docs/plans/NEO-121-implementation-plan.md).
|
||||
|
||||
## Encounter definitions (NEO-103)
|
||||
|
||||
**`GET /game/world/encounter-definitions`** returns a versioned JSON body (`schemaVersion` **1**, **`encounters`**) backed by **`IEncounterDefinitionRegistry`** and **`IRewardTableDefinitionRegistry`** — the same prototype rows loaded at startup (no second source of truth). Each row includes **`id`**, **`displayName`**, nested **`completionCriteria`** (`kind`), **`requiredNpcInstanceIds`**, and nested **`rewardTable`** (`id`, `displayName`, **`fixedGrants`** with `itemId` + `quantity`). Plan: [NEO-103 implementation plan](../../docs/plans/NEO-103-implementation-plan.md); Bruno: `bruno/neon-sprawl-server/encounter-definitions/`.
|
||||
|
|
|
|||
Loading…
Reference in New Issue