diff --git a/docs/plans/NEO-128-implementation-plan.md b/docs/plans/NEO-128-implementation-plan.md new file mode 100644 index 0000000..f111aca --- /dev/null +++ b/docs/plans/NEO-128-implementation-plan.md @@ -0,0 +1,158 @@ +# NEO-128 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEO-128 | +| **Title** | E7M2-05: Wire router into `QuestStateOperations.TryMarkComplete` | +| **Linear** | https://linear.app/neon-sprawl/issue/NEO-128/e7m2-05-wire-router-into-queststateoperationstrymarkcomplete | +| **Module** | [E7.M2 — RewardAndUnlockRouter](../decomposition/modules/E7_M2_RewardAndUnlockRouter.md) · Epic 7 Slice 2 · backlog **E7M2-05** | +| **Branch** | `NEO-128-e7m2-quest-complete-reward-wiring` | +| **Precursor** | [NEO-127](https://linear.app/neon-sprawl/issue/NEO-127) **`Done`** — `RewardRouterOperations.TryDeliverQuestCompletion` | +| **Pattern** | [NEO-105](NEO-105-implementation-plan.md) — deliver side effects before completion mark; [NEO-118](NEO-118-implementation-plan.md) — `QuestObjectiveWiring` → `TryMarkComplete` on terminal step | +| **Blocks** | [NEO-129](https://linear.app/neon-sprawl/issue/NEO-129) (`completionRewardSummary` HTTP), [NEO-130](https://linear.app/neon-sprawl/issue/NEO-130) (telemetry hook comments) | +| **Client counterpart** | [NEO-131](https://linear.app/neon-sprawl/issue/NEO-131) — HUD surfacing (blocked by E7M2-06 / NEO-129) | + +## Kickoff clarifications + +| Topic | Question | Agent recommendation | Answer | +|--------|----------|----------------------|--------| +| **Delivery vs mark ordering** | When should bundle apply run relative to `TryMarkComplete`? | **Deliver bundle first, then mark complete** — E7M2 “transactional equivalent” to fail-closed; mirrors NEO-105 (grants before completion mark); avoids new `TryRevertCompletion` store API. | **Adopted** — deliver-then-mark | +| **Bruno smoke** | Add Bruno requests for quest-complete reward delivery? | **Skip** — server-internal wiring only; no HTTP DTO changes (NEO-129); automated integration tests cover AC; Bruno optional per backlog because verification is store/inventory/skill assertions, not a new route. | **Adopted** — skip Bruno | + +**Additional defaults (no kickoff question — settled by backlog / landed code):** + +- **Idempotent quest replay** — when progress is already **`completed`**, return success immediately and **do not** call the router (NEO-121 `quest_complete` hook path unchanged; no double-grant). +- **Deny reason passthrough** — on delivery deny, return **`QuestStateOperationResult`** with **`Success: false`** and **`ReasonCode`** from **`RewardDeliveryResult.ReasonCode`** (`inventory_full`, etc.); quest row stays **`active`**. +- **`QuestStateOperationResult` shape** — unchanged; HTTP projection of grants is NEO-129. +- **Bundle source** — `definition.CompletionRewardBundle` from **`IQuestDefinitionRegistry`**; all four prototype quests have bundles (NEO-124/125 CI gates). +- **Wiring best-effort** — `QuestObjectiveWiring` continues to ignore `TryMarkComplete` deny envelopes (primary gather/craft/encounter ops unaffected); on inventory-full deny the quest remains active for retry. + +## Goal, scope, and out-of-scope + +**Goal:** First-time quest completion triggers **`completionRewardBundle`** delivery exactly once via **`RewardRouterOperations`**; idempotent complete replay does not double-grant items or XP. + +**In scope (from Linear + [E7M2-05](E7M2-prototype-backlog.md#e7m2-05--wire-router-into-queststateoperationstrymarkcomplete)):** + +- Call **`RewardRouterOperations.TryDeliverQuestCompletion`** from **`QuestStateOperations.TryMarkComplete`** on the first-time completion path (before store mark per adopted ordering). +- Skip router on idempotent **`completed`** early return. +- Thread router dependencies through **`QuestObjectiveWiring`** and upstream ops (`GatherOperations`, `CraftOperations`, `EncounterCompletionOperations`, `QuestProgressApi`) so objective-wiring completion exercises the same path. +- Integration tests (AAA): gather-intro wiring → salvage +25 + store record; idempotent `TryMarkComplete` replay → no duplicate XP; inventory-full deny → quest stays active, no store record; operator-chain → item + skill grants. +- `server/README.md` quest-state section update; E7.M2 module alignment anchor. + +**Out of scope (from Linear):** + +- HTTP **`completionRewardSummary`** ([NEO-129](https://linear.app/neon-sprawl/issue/NEO-129) E7M2-06). +- E9.M1 telemetry ingest ([NEO-130](https://linear.app/neon-sprawl/issue/NEO-130) E7M2-07) — existing comment-only hook sites stay. +- Encounter completion path; per-step bundles. +- Bruno smoke (deferred per kickoff). +- Godot client. + +## Acceptance criteria checklist + +- [ ] Completing a quest via objective wiring delivers bundle once. +- [ ] Idempotent complete replay does not double-grant items or XP. + +## Technical approach + +### 1. Extend `TryMarkComplete` (deliver-then-mark) + +Add router dependencies to the static signature (mirror **`RewardRouterOperations.TryDeliverQuestCompletion`** injectables): + +```csharp +public static QuestStateOperationResult TryMarkComplete( + string playerId, + string questId, + IQuestDefinitionRegistry questRegistry, + IPlayerQuestStateStore progressStore, + IItemDefinitionRegistry itemRegistry, + IPlayerInventoryStore inventoryStore, + ISkillDefinitionRegistry skillRegistry, + IPlayerSkillProgressionStore skillProgressionStore, + ISkillLevelCurve levelCurve, + PerkUnlockEngine perkUnlockEngine, + IPlayerPerkStateStore perkStore, + IRewardDeliveryStore deliveryStore, + TimeProvider timeProvider) +``` + +**First-time completion flow** (after existing registry / active guards): + +1. Load **`definition.CompletionRewardBundle`** from registry (already resolved in guard). +2. **`RewardRouterOperations.TryDeliverQuestCompletion(...)`** — includes store idempotent replay (`already_delivered` success when store hit). +3. If **`!delivery.Success`**, return **`Deny(delivery.ReasonCode!)`** — quest stays **`active`**. +4. **`progressStore.TryMarkComplete(...)`** — on success, emit existing NEO-121 **`quest_complete`** comment hook; return **`Success(snapshot)`**. +5. Preserve existing idempotent / race handling when **`TryMarkComplete`** returns false but progress is already **`completed`** (re-read paths unchanged). + +**Idempotent replay** (progress already **`completed`**): existing early return — **no router call**. + +### 2. Dependency threading + +| Layer | Change | +|-------|--------| +| **`QuestObjectiveWiring`** | Add reward-router parameters to **`TryCompleteStepIfSatisfied`**, **`TryCompleteAllActiveQuests`**, and all public entry points that call them (`TryProcessGatherSuccess`, `TryProcessCraftSuccess`, `TryProcessEncounterCompleteSuccess`, `TryRefreshInventoryProgressForPlayer`). | +| **`QuestStateOperations`** | Pass new deps into **`TryMarkComplete`** from **`TryAccept`** / **`TryAdvanceStep`** wiring calls. | +| **`GatherOperations`**, **`CraftOperations`**, **`EncounterCompletionOperations`** | Extend method signatures + call sites to pass skill/progress/perk/delivery deps into wiring (resolve from existing HTTP/ops injectables same as gather/craft already thread quest stores). | +| **`QuestProgressApi`** | Pass reward deps into **`TryRefreshInventoryProgressForPlayer`**. | + +Use **`InMemoryWebApplicationFactory`** DI resolution in tests — no new service registration (stores already in **`Program.cs`**). + +### 3. Tests + +| Test file | New / extended cases | +|-----------|---------------------| +| **`QuestStateOperationsTests`** | **`TryMarkComplete_ShouldDeliverGatherIntroBundle_WhenFirstCompletion`** — accept + complete → **`IRewardDeliveryStore.TryGet`** true, salvage XP +25. **`TryMarkComplete_ShouldNotDoubleGrant_WhenIdempotentReplay`** — second complete → same store event, XP unchanged. **`TryMarkComplete_ShouldDenyAndStayActive_WhenInventoryFull`** — fill bag, complete operator-chain quest setup → deny **`inventory_full`**, status **`active`**, no store record. | +| **`QuestObjectiveWiringTests`** | Extend **`GatherIntro_ShouldComplete_WhenScrapGatheredThreeTimes`** (or sibling) — assert salvage XP +25 and delivery store after wiring-driven complete. **`GatherIntro_ShouldNotDoubleGrantXp_WhenCompleteReplayed`** — manual second **`TryMarkComplete`** after wiring complete. | + +Reuse **`RewardRouterTestDependencies`**-style resolution or extend existing **`QuestTestDependencies`** / **`QuestWiringTestDependencies`** records with skill + delivery services. + +### 4. Docs + +- **`server/README.md`** — update **`QuestStateOperations.TryMarkComplete`** section: deliver-then-mark order, idempotent skip, deny passthrough, pointer to **`RewardRouterOperations`**. +- **`documentation_and_implementation_alignment.md`** — E7.M2 row: quest-state wiring (NEO-128). +- **`E7_M2_RewardAndUnlockRouter.md`** — implementation anchor for **`TryMarkComplete`** wiring. + +## Files to add + +| Path | Purpose | +|------|---------| +| `docs/plans/NEO-128-implementation-plan.md` | This plan. | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs` | Wire **`RewardRouterOperations`** into **`TryMarkComplete`**; extend signature; thread deps from accept/advance. | +| `server/NeonSprawl.Server/Game/Quests/QuestObjectiveWiring.cs` | Pass reward-router deps through completion wiring chain. | +| `server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs` | Thread reward deps into **`TryProcessGatherSuccess`**. | +| `server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs` | Thread reward deps into **`TryProcessCraftSuccess`**. | +| `server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs` | Thread reward deps into **`TryProcessEncounterCompleteSuccess`**. | +| `server/NeonSprawl.Server/Game/Quests/QuestProgressApi.cs` | Thread reward deps into inventory refresh wiring. | +| `server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs` | Bundle delivery, idempotent replay, inventory-full deny tests. | +| `server/NeonSprawl.Server.Tests/Game/Quests/QuestObjectiveWiringTests.cs` | Wiring path asserts store + skill XP after gather complete. | +| `server/NeonSprawl.Server.Tests/Game/Quests/QuestAcceptApiTests.cs` | Update **`TryMarkComplete`** call sites with new parameters. | +| `server/NeonSprawl.Server.Tests/Game/Quests/QuestProgressApiTests.cs` | Update **`TryMarkComplete`** / wiring dependency resolution. | +| `server/README.md` | Document quest-complete reward wiring on **`TryMarkComplete`**. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E7.M2 alignment — quest-state wiring (NEO-128). | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | NEO-128 implementation anchor. | + +## Tests + +| Target | Coverage | +|--------|----------| +| `server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs` | Direct **`TryMarkComplete`**: first completion delivers bundle + records store; idempotent replay no double XP; inventory-full deny leaves quest active without store record. | +| `server/NeonSprawl.Server.Tests/Game/Quests/QuestObjectiveWiringTests.cs` | Gather-intro wiring path delivers salvage +25 once; replay does not double-grant. | +| **Compile-fix only** — `QuestAcceptApiTests`, `QuestProgressApiTests` | Updated **`TryMarkComplete`** signatures; existing accept/progress behavior unchanged. | +| **Manual verification** | `dotnet test server/NeonSprawl.Server.Tests --filter "QuestStateOperationsTests|QuestObjectiveWiringTests"`; dev server boot unchanged. | + +## Open questions / risks + +| Question or risk | Agent recommendation | Status | +|------------------|----------------------|--------| +| Delivery succeeds but **`TryMarkComplete`** store write fails while quest still active | Extremely unlikely solo prototype; handle via existing re-read idempotent paths when another caller completed; defer explicit delivery rollback on mark-fail unless concurrency story requires it. | **deferred** | +| Orphan store record if mark never commits | **`TryDeliver`** idempotent replay on retry allows **`already_delivered`** success then mark on next complete attempt — self-healing. | **adopted** | +| Large signature expansion across wiring call graph | Accept explicit parameter threading (NEO-118 precedent); no new DI facade type in this story. | **adopted** | +| **`QuestStateOperationResult` grant projection** | Defer to NEO-129 HTTP read model. | **deferred** | + +None beyond the above.