159 lines
12 KiB
Markdown
159 lines
12 KiB
Markdown
# 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.
|