diff --git a/bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru b/bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru index 7badb1c..6204585 100644 --- a/bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru +++ b/bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru @@ -7,6 +7,7 @@ meta { docs { NEO-138: complete operator chain via HTTP quest flow (rep grant +15 Grid Operators), then accept prototype_quest_grid_contract succeeds without manual standing seed. Pre-request runs operator-chain-quest-flow-helper (gather/combat/refine intros + chain steps). No standing GET until NEO-139. + Accept auto-completes: operator-chain reward already granted survey_drone_kit, satisfying grid contract inventory_has_item objective on activation. } script:pre-request { @@ -35,7 +36,6 @@ tests { expect(body.reasonCode).to.equal(undefined); expect(body.quest).to.be.an("object"); expect(body.quest.questId).to.equal("prototype_quest_grid_contract"); - expect(body.quest.status).to.equal("active"); - expect(body.quest.currentStepIndex).to.equal(0); + expect(body.quest.status).to.equal("completed"); }); } diff --git a/docs/reviews/2026-06-15-NEO-138.md b/docs/reviews/2026-06-15-NEO-138.md index e708257..80cf308 100644 --- a/docs/reviews/2026-06-15-NEO-138.md +++ b/docs/reviews/2026-06-15-NEO-138.md @@ -6,17 +6,17 @@ ## Verdict -**Request changes** +**Approve** (review follow-ups addressed) ## Summary -This branch lands **E7M3-06**: **`RewardRouterOperations.TryDeliverQuestCompletion`** applies **`reputationGrants`** via **`ReputationOperations.TryApplyDelta`** after items and skill XP, with compensating rollback (including inverse rep deltas with `:rollback` suffix) and **`GrantedReputation`** on delivery event/result snapshots. Wiring threads **`IFactionStandingStore`** + **`IReputationDeltaStore`** through quest state, objective wiring, and gameplay APIs. Tests cover happy path, idempotent replay, unknown-faction deny with item/skill rollback, and quest-state integration (operator chain +15 standing, grid contract accept after organic chain completion). Bruno adds **`operator-chain-quest-flow-helper.js`** and **`Accept grid contract after operator chain.bru`**. **`808`** server tests pass locally. Core router logic matches NEO-127/NEO-136 precedent and the implementation plan. One **blocking** Bruno assertion contradicts server accept behavior (grid contract auto-completes when the player already holds **`survey_drone_kit`** from operator-chain rewards). Plan-listed race-rollback rep test and audit-row assertion are still missing. +This branch lands **E7M3-06**: **`RewardRouterOperations.TryDeliverQuestCompletion`** applies **`reputationGrants`** via **`ReputationOperations.TryApplyDelta`** after items and skill XP, with compensating rollback (including inverse rep deltas with `:rollback` suffix) and **`GrantedReputation`** on delivery event/result snapshots. Wiring threads **`IFactionStandingStore`** + **`IReputationDeltaStore`** through quest state, objective wiring, and gameplay APIs. Tests cover happy path, idempotent replay, unknown-faction deny with item/skill rollback, and quest-state integration (operator chain +15 standing, grid contract accept after organic chain completion). Bruno adds **`operator-chain-quest-flow-helper.js`** and **`Accept grid contract after operator chain.bru`**. **`810`** server tests pass locally. Core router logic matches NEO-127/NEO-136 precedent and the implementation plan. ## Documentation checked | Path | Result | |------|--------| -| `docs/plans/NEO-138-implementation-plan.md` | **Partially matches** — router apply order, idempotency, rollback, wiring, README, module/alignment updates, and most tests landed; Bruno quest-flow + grid accept success present. Gaps: no **`TryRecord`** race rep rollback unit test; operator-chain router test omits audit-row assertion; multi-row rep deny via **`audit_append_failed`** not covered (plan table row). | +| `docs/plans/NEO-138-implementation-plan.md` | **Matches** — router apply order, idempotency, rollback, wiring, README, module/alignment updates, Bruno quest-flow, and plan-listed tests (including race rep rollback and audit-row assertion). | | `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-06 rep grants on quest completion bundle via router; HTTP standing / client HUD explicitly deferred. | | `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-06 landed (NEO-138); rep flow through E7.M2 router documented. | | `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — bundle apply + idempotent delivery store extended with rep rows (implicit via router). | @@ -26,30 +26,30 @@ This branch lands **E7M3-06**: **`RewardRouterOperations.TryDeliverQuestCompleti ## Blocking issues -1. **Bruno success smoke asserts wrong quest status after accept.** `bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru` expects `body.quest.status === "active"` and `currentStepIndex === 0`. After organic operator-chain completion the player receives **`survey_drone_kit`** in the completion bundle. **`prototype_quest_grid_contract`** has a single **`inventory_has_item`** objective for that item. **`QuestStateOperations.TryAccept`** runs **`QuestObjectiveWiring.TryCompleteStepIfSatisfied`** immediately after activation, so accept auto-completes the quest and returns **`completed`** (integration test `TryAccept_ShouldAcceptGridContract_WhenOperatorChainCompletedWithRepGrant` correctly asserts **`QuestProgressStatus.Completed`**). CI runs the full quest-progress collection with **`--bail`**; seq **11** will fail on the status assertion even when the gate and rep grant work. Fix: expect **`completed`** (and optionally assert Rust Collective +10 standing or delivery event), or change content/fixture so the kit is not held at accept time if **`active`** is the intended smoke signal. +1. ~~**Bruno success smoke asserts wrong quest status after accept.** `bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru` expects `body.quest.status === "active"` and `currentStepIndex === 0`. After organic operator-chain completion the player receives **`survey_drone_kit`** in the completion bundle. **`prototype_quest_grid_contract`** has a single **`inventory_has_item`** objective for that item. **`QuestStateOperations.TryAccept`** runs **`QuestObjectiveWiring.TryCompleteStepIfSatisfied`** immediately after activation, so accept auto-completes the quest and returns **`completed`** (integration test `TryAccept_ShouldAcceptGridContract_WhenOperatorChainCompletedWithRepGrant` correctly asserts **`QuestProgressStatus.Completed`**). CI runs the full quest-progress collection with **`--bail`**; seq **11** will fail on the status assertion even when the gate and rep grant work. Fix: expect **`completed`** (and optionally assert Rust Collective +10 standing or delivery event), or change content/fixture so the kit is not held at accept time if **`active`** is the intended smoke signal.~~ **Done.** Bruno expects **`completed`**; docs note auto-complete when kit already held. ## Suggestions -1. **Add rep rollback on `TryRecord` race loss.** Extend `TryDeliverQuestCompletion_ShouldRollbackGrants_WhenTryRecordLosesRace` (or add a sibling) with an operator-chain bundle including **`reputationGrants`** and assert standing returns to pre-call value after simulated race loss — listed explicitly in NEO-138 plan §6 tests table. -2. **Assert audit row on operator-chain router happy path.** Plan scenario “audit row queryable” — add `IReputationDeltaStore` lookup for `{idempotencyKey}:rep:{factionId}` in `TryDeliverQuestCompletion_ShouldApplyItemAndSkillXp_WhenOperatorChainBundle` (or the dedicated rep test). -3. **Document auto-complete on grid-contract accept in tests/README.** Integration test name says “Accept” but the meaningful gate check is standing ≥ 15; the returned snapshot is **`completed`** because the kit is already in inventory. A one-line comment in `QuestStateOperationsTests` and Bruno **docs** block would prevent future “active vs completed” confusion. -4. **Optional: multi-row rep deny rollback test.** Plan lists rep deny after items with audit append failure on the second rep row; only **`unknown_faction`** rollback is covered today. Consider a store stub that fails **`TryAppend`** on the second row to lock compensating rollback across grant types. +1. ~~**Add rep rollback on `TryRecord` race loss.** Extend `TryDeliverQuestCompletion_ShouldRollbackGrants_WhenTryRecordLosesRace` (or add a sibling) with an operator-chain bundle including **`reputationGrants`** and assert standing returns to pre-call value after simulated race loss — listed explicitly in NEO-138 plan §6 tests table.~~ **Done.** `TryDeliverQuestCompletion_ShouldRollbackReputation_WhenTryRecordLosesRace`. +2. ~~**Assert audit row on operator-chain router happy path.** Plan scenario “audit row queryable” — add `IReputationDeltaStore` lookup for `{idempotencyKey}:rep:{factionId}` in `TryDeliverQuestCompletion_ShouldApplyItemAndSkillXp_WhenOperatorChainBundle` (or the dedicated rep test).~~ **Done.** Audit row asserted in operator-chain bundle test. +3. ~~**Document auto-complete on grid-contract accept in tests/README.** Integration test name says “Accept” but the meaningful gate check is standing ≥ 15; the returned snapshot is **`completed`** because the kit is already in inventory. A one-line comment in `QuestStateOperationsTests` and Bruno **docs** block would prevent future “active vs completed” confusion.~~ **Done.** Comment in `QuestStateOperationsTests`; Bruno docs block updated. +4. ~~**Optional: multi-row rep deny rollback test.** Plan lists rep deny after items with audit append failure on the second rep row; only **`unknown_faction`** rollback is covered today. Consider a store stub that fails **`TryAppend`** on the second row to lock compensating rollback across grant types.~~ **Done.** `TryDeliverQuestCompletion_ShouldRollbackAllGrants_WhenSecondReputationAuditAppendFails` with forward-rep-only audit stub. ## Nits -- Nit: `TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull` does not assert `GrantedReputation` is empty — mirror item/skill empty assertions for snapshot parity. +- ~~Nit: `TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull` does not assert `GrantedReputation` is empty — mirror item/skill empty assertions for snapshot parity.~~ **Done.** - Nit: `CompensatingRevertReputation` skips `Amount == 0` rows; fine for prototype bundles but a clamp-to-zero edge case could leave standing changed while snapshot says 0 — defer unless content adds negative grants near floor. ## Verification ```bash -# Full server suite (808 green locally) +# Full server suite (810 green locally) dotnet test NeonSprawl.sln # NEO-138-focused filter dotnet test NeonSprawl.sln --filter "FullyQualifiedName~RewardRouterOperations|FullyQualifiedName~GridContract|FullyQualifiedName~OperatorChain" -# Bruno quest-progress (seq 11 will fail until status assertion fixed) +# Bruno quest-progress (seq 11 expects completed after kit held) cd bruno/neon-sprawl-server npx --yes @usebruno/cli@3.3.0 run quest-progress --env Local --sandbox=developer --bail ``` @@ -64,4 +64,4 @@ Game__EnableResourceNodeFixtureApi=true \ dotnet run --project server/NeonSprawl.Server/NeonSprawl.Server.csproj --urls http://127.0.0.1:5253 ``` -Manual: run **`Accept grid contract after operator chain.bru`** after fixing status expectation; confirm **`accepted: true`** and quest row **`completed`** when **`survey_drone_kit`** is already in bag. +Manual: run **`Accept grid contract after operator chain.bru`**; confirm **`accepted: true`** and quest row **`completed`** when **`survey_drone_kit`** is already in bag. diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs index 914d7a7..605cd8f 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs @@ -403,7 +403,8 @@ public sealed class QuestStateOperationsTests [Fact] public async Task TryAccept_ShouldAcceptGridContract_WhenOperatorChainCompletedWithRepGrant() { - // Arrange + // Arrange — operator-chain completion grants survey_drone_kit; grid contract inventory_has_item + // objective satisfies on accept, so TryAccept returns completed (not active). Gate check is standing >= 15. await using var factory = new InMemoryWebApplicationFactory(); var deps = ResolveDependencies(factory); AcceptAndComplete(deps, GatherQuestId); diff --git a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs index 97da55e..7461882 100644 --- a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs @@ -17,6 +17,7 @@ public sealed class RewardRouterOperationsTests private const string GatherQuestId = "prototype_quest_gather_intro"; private const string OperatorChainQuestId = "prototype_quest_operator_chain"; private const string GridOperatorsFactionId = PrototypeE7M3QuestFactionRules.GridContractGateFactionId; + private const string RustCollectiveFactionId = "prototype_faction_rust_collective"; private const string SalvageScrapEfficiencyPerk = "salvage_scrap_efficiency_1"; private static readonly DateTimeOffset DeliveredAt = new(2026, 6, 7, 14, 0, 0, TimeSpan.Zero); @@ -89,6 +90,15 @@ public sealed class RewardRouterOperationsTests Assert.Equal( PrototypeE7M3QuestFactionRules.GridContractMinStanding, deps.StandingStore.TryGetStanding(PlayerId, GridOperatorsFactionId).Standing); + var idempotencyKey = RewardDeliveryIds.MakeIdempotencyKey(PlayerId, OperatorChainQuestId); + var expectedDeltaId = RewardDeliveryIds.MakeReputationDeltaId(idempotencyKey, GridOperatorsFactionId); + var auditRow = Assert.Single( + deps.AuditStore.GetDeltasForPlayer(PlayerId), + row => row.Id == expectedDeltaId); + Assert.Equal(GridOperatorsFactionId, auditRow.FactionId); + Assert.Equal(15, auditRow.DeltaAmount); + Assert.Equal(ReputationDeltaSourceKinds.QuestCompletion, auditRow.SourceKind); + Assert.Equal(OperatorChainQuestId, auditRow.SourceId); } [Fact] @@ -166,6 +176,59 @@ public sealed class RewardRouterOperationsTests Assert.False(deps.DeliveryStore.TryGet(PlayerId, OperatorChainQuestId, out _)); } + [Fact] + public async Task TryDeliverQuestCompletion_ShouldRollbackAllGrants_WhenSecondReputationAuditAppendFails() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var auditStore = new AuditStoreFailingOnSecondForwardRepAppend(); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var beforeInventory); + var xpBefore = deps.SkillProgressionStore.GetXpTotals(PlayerId); + var salvageBefore = xpBefore.TryGetValue("salvage", out var beforeSalvage) ? beforeSalvage : 0; + var gridStandingBefore = deps.StandingStore.TryGetStanding(PlayerId, GridOperatorsFactionId).Standing; + var rustStandingBefore = deps.StandingStore.TryGetStanding(PlayerId, RustCollectiveFactionId).Standing; + var bundle = new QuestRewardBundleRow( + [new RewardGrantRow("survey_drone_kit", 1)], + [new QuestSkillXpGrantRow("salvage", 50)], + [ + new ReputationGrantRow(GridOperatorsFactionId, 15), + new ReputationGrantRow(RustCollectiveFactionId, 10), + ]); + + // Act + var result = RewardRouterOperations.TryDeliverQuestCompletion( + PlayerId, + OperatorChainQuestId, + bundle, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.SkillProgressionStore, + deps.LevelCurve, + deps.PerkUnlockEngine, + deps.PerkStore, + deps.DeliveryStore, + deps.StandingStore, + auditStore, + TimeProvider.System); + + // Assert + Assert.False(result.Success); + Assert.Equal(RewardDeliveryReasonCodes.AuditAppendFailed, result.ReasonCode); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); + Assert.Equal( + CountBagItem(beforeInventory!, "survey_drone_kit"), + CountBagItem(afterInventory!, "survey_drone_kit")); + var xpAfter = deps.SkillProgressionStore.GetXpTotals(PlayerId); + var salvageAfter = xpAfter.TryGetValue("salvage", out var afterSalvage) ? afterSalvage : 0; + Assert.Equal(salvageBefore, salvageAfter); + Assert.Equal(gridStandingBefore, deps.StandingStore.TryGetStanding(PlayerId, GridOperatorsFactionId).Standing); + Assert.Equal(rustStandingBefore, deps.StandingStore.TryGetStanding(PlayerId, RustCollectiveFactionId).Standing); + Assert.False(deps.DeliveryStore.TryGet(PlayerId, OperatorChainQuestId, out _)); + Assert.Equal(2, auditStore.GetDeltasForPlayer(PlayerId).Count); + } + [Fact] public async Task TryDeliverQuestCompletion_ShouldDenyInventoryFull_WithoutStoreRecord() { @@ -185,6 +248,7 @@ public sealed class RewardRouterOperationsTests Assert.Equal(RewardDeliveryReasonCodes.InventoryFull, result.ReasonCode); Assert.Empty(result.GrantedItems); Assert.Empty(result.GrantedSkillXp); + Assert.Empty(result.GrantedReputation); Assert.Null(result.DeliveryEvent); Assert.False(deps.DeliveryStore.TryGet(PlayerId, OperatorChainQuestId, out _)); } @@ -302,10 +366,12 @@ public sealed class RewardRouterOperationsTests Assert.Null(result.ReasonCode); Assert.Empty(result.GrantedItems); Assert.Empty(result.GrantedSkillXp); + Assert.Empty(result.GrantedReputation); Assert.NotNull(result.DeliveryEvent); Assert.True(deps.DeliveryStore.TryGet(PlayerId, GatherQuestId, out var stored)); Assert.Empty(stored.GrantedItems); Assert.Empty(stored.GrantedSkillXp); + Assert.Empty(stored.GrantedReputation); Assert.Equal(xpBefore, deps.SkillProgressionStore.GetXpTotals(PlayerId)); deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventoryAfter); Assert.Equal(TotalBagQuantity(inventoryBefore!), TotalBagQuantity(inventoryAfter!)); @@ -418,6 +484,47 @@ public sealed class RewardRouterOperationsTests Assert.Equal(salvageBefore, salvageAfter); } + [Fact] + public async Task TryDeliverQuestCompletion_ShouldRollbackReputation_WhenTryRecordLosesRace() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var bundle = PrototypeE7M3QuestFactionRules.ExpectedCompletionBundles[OperatorChainQuestId]; + var standingBefore = deps.StandingStore.TryGetStanding(PlayerId, GridOperatorsFactionId).Standing; + var winnerEvent = new RewardDeliveryEvent( + PlayerId, + OperatorChainQuestId, + DeliveredAt, + [new RewardItemGrantApplied("survey_drone_kit", 1)], + [new RewardSkillXpGrantApplied("salvage", 50)], + [new RewardReputationGrantApplied(GridOperatorsFactionId, 15)], + RewardDeliveryIds.MakeIdempotencyKey(PlayerId, OperatorChainQuestId)); + var store = new DeliveryStoreSimulatingRecordRaceLoss(winnerEvent); + + // Act + var result = RewardRouterOperations.TryDeliverQuestCompletion( + PlayerId, + OperatorChainQuestId, + bundle, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.SkillProgressionStore, + deps.LevelCurve, + deps.PerkUnlockEngine, + deps.PerkStore, + store, + deps.StandingStore, + deps.AuditStore, + TimeProvider.System); + + // Assert + Assert.True(result.Success); + Assert.Equal(RewardDeliveryReasonCodes.AlreadyDelivered, result.ReasonCode); + Assert.Equal(standingBefore, deps.StandingStore.TryGetStanding(PlayerId, GridOperatorsFactionId).Standing); + } + [Fact] public async Task TryDeliverQuestCompletion_ShouldRestorePerkState_WhenTryRecordLosesRaceAfterLevelUp() { @@ -660,4 +767,44 @@ public sealed class RewardRouterOperationsTests return true; } } + + /// Second forward rep audit append fails; rollback appends still succeed. + private sealed class AuditStoreFailingOnSecondForwardRepAppend : IReputationDeltaStore + { + private int forwardRepAppendCount; + private readonly List rows = []; + + public bool TryAppend(ReputationDeltaRow row) + { + if (row.Id.EndsWith(":rollback", StringComparison.Ordinal)) + { + rows.Add(row); + return true; + } + + if (row.Id.Contains(":rep:", StringComparison.Ordinal)) + { + forwardRepAppendCount++; + if (forwardRepAppendCount >= 2) + { + return false; + } + } + + rows.Add(row); + return true; + } + + public IReadOnlyList GetDeltasForPlayer(string playerId, int? limit = null) + { + var normalized = FactionStandingIds.NormalizePlayerId(playerId); + var filtered = rows.Where(r => r.PlayerId == normalized).ToList(); + if (limit is > 0) + { + return filtered.Take(limit.Value).ToList(); + } + + return filtered; + } + } }