don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-138: address code review follow-ups. 

Fix Bruno grid-contract status expectation, add race rep rollback and
multi-row audit-failure tests, audit-row assertion, and doc comments.
pull/178/head
VinPropane 2026-06-15 22:29:36 -04:00
parent 245f0a84ed
commit 69c3ad4915
4 changed files with 163 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
bruno/neon-sprawl-server/quest-progress/Accept grid contract after operator chain.bru
View File

@ -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");
});
}

24
docs/reviews/2026-06-15-NEO-138.md
View File

@ -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.

3
server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs
View File

@ -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);

147
server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs
View File

@ -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;
}
}
/// <summary>Second forward rep audit append fails; rollback appends still succeed.</summary>
private sealed class AuditStoreFailingOnSecondForwardRepAppend : IReputationDeltaStore
{
private int forwardRepAppendCount;
private readonly List<ReputationDeltaRow> 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<ReputationDeltaRow> 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;
}
}
}