diff --git a/bruno/neon-sprawl-server/ability-cast/Post cast happy.bru b/bruno/neon-sprawl-server/ability-cast/Post cast happy.bru index 15b8fa2..53f6cee 100644 --- a/bruno/neon-sprawl-server/ability-cast/Post cast happy.bru +++ b/bruno/neon-sprawl-server/ability-cast/Post cast happy.bru @@ -5,7 +5,7 @@ meta { } script:pre-request { - const { resetPrototypeCombatTargets } = require("./scripts/combat-targets-reset-helper"); + const { resetPrototypeCombatTargets } = require("./scripts/combat-targets-reset-helper.js"); await resetPrototypeCombatTargets(bru); const axios = require("axios"); diff --git a/bruno/neon-sprawl-server/ability-cast/Post cast lock pulse defeat spine.bru b/bruno/neon-sprawl-server/ability-cast/Post cast lock pulse defeat spine.bru index 6bde75c..4d10c1f 100644 --- a/bruno/neon-sprawl-server/ability-cast/Post cast lock pulse defeat spine.bru +++ b/bruno/neon-sprawl-server/ability-cast/Post cast lock pulse defeat spine.bru @@ -9,7 +9,7 @@ docs { } script:pre-request { - const { resetPrototypeCombatTargets } = require("./scripts/combat-targets-reset-helper"); + const { resetPrototypeCombatTargets } = require("./scripts/combat-targets-reset-helper.js"); await resetPrototypeCombatTargets(bru); const axios = require("axios"); diff --git a/bruno/neon-sprawl-server/scripts/bruno-dev-fixture-helper.js b/bruno/neon-sprawl-server/scripts/bruno-dev-fixture-helper.js index d3a9973..b998ced 100644 --- a/bruno/neon-sprawl-server/scripts/bruno-dev-fixture-helper.js +++ b/bruno/neon-sprawl-server/scripts/bruno-dev-fixture-helper.js @@ -39,6 +39,7 @@ async function resetQuestProgress(bru, questIds) { } const ALL_PROTOTYPE_QUEST_IDS = [ + // Keep in sync with PrototypeE7M1QuestCatalogRules.ExpectedQuestIds (server/NeonSprawl.Server/Game/Quests/PrototypeE7M1QuestCatalogRules.cs). "prototype_quest_combat_intro", "prototype_quest_gather_intro", "prototype_quest_grid_contract", diff --git a/docs/reviews/2026-06-15-bruno-self-reset-fixtures.md b/docs/reviews/2026-06-15-bruno-self-reset-fixtures.md new file mode 100644 index 0000000..a481ee9 --- /dev/null +++ b/docs/reviews/2026-06-15-bruno-self-reset-fixtures.md @@ -0,0 +1,69 @@ +# Code review — Bruno self-reset dev fixtures + +**Date:** 2026-06-15 +**Scope:** Branch `chore/bruno-self-reset-fixtures` — commit `95cf5d1` vs `origin/main` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +This chore branch makes the Bruno server collection **order-independent** by adding dev-only fixture reset hooks and wiring them into quest, gather, and combat `.bru` pre-request scripts. Server-side changes extend existing fixture patterns: **`resetQuestIds`** on the quest fixture API, a new **`POST /game/__dev/resource-node-fixture`**, **`TrySetRemainingGathers`** on the resource-node instance store, and **`ClearPlayer`** on the ability cooldown store (called from the combat-target fixture). Bruno helpers centralize reset logic in `scripts/bruno-dev-fixture-helper.js`; quest and gather tests self-reset before asserting. Tests are solid (803 pass locally), CI enables the new resource-node fixture flag, and dev routes remain gated behind Development/Testing/config flags. Residual risk is low — dev-only surface, no player-facing client change. **Follow-up:** README, Postgres `TryClearProgress` integration test, and Bruno nit fixes applied post-review. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-116-implementation-plan.md` | **Partially matches** — open question “Dev fixture quest reset” was **deferred**; this branch implements that deferred hook (`resetQuestIds`). No dedicated plan for this chore; acceptable for infra work but README should record the landed behavior. | +| `docs/plans/NEO-137-implementation-plan.md` | **N/A (extended)** — quest fixture originally documented **`completedQuestIds` only**; this branch adds **`resetQuestIds`** without updating NEO-137 plan (fine for chore; README should be source of truth). | +| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **N/A change** — reset is dev fixture bypass, not quest state machine semantics. | +| `docs/decomposition/modules/E3_M1_GatheringAndSalvage.md` (gathering) | **Partially matches** — depletion store behavior unchanged for gameplay; fixture reset is dev-only upsert to catalog **`maxGathers`**. No module status update required. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **N/A change** — no tracking-table row for Bruno fixture infra. | +| `server/README.md` | **Matches** — quest fixture documents **`resetQuestIds`** + Bruno helper pointer; combat-target fixture documents cooldown clear; resource-node fixture documented. | + +## Blocking issues + +None. + +## Suggestions + +1. ~~**Update `server/README.md`** for the three fixture surfaces changed in this branch:~~ + - ~~Quest fixture: document optional **`resetQuestIds`** (clears progress rows, idempotent when absent; can combine with **`completedQuestIds`**).~~ + - ~~Combat-target fixture: note **`IPlayerAbilityCooldownStore.ClearPlayer`** for the dev player.~~ + - ~~New **`POST /game/__dev/resource-node-fixture`** section (`Game:EnableResourceNodeFixtureApi`, restores all catalog nodes to **`maxGathers`**, Bruno helper `resetPrototypeResourceNodes`).~~ + - ~~Brief pointer to **`scripts/bruno-dev-fixture-helper.js`** as the shared Bruno reset entry point for quest/gather/inventory flows.~~ **Done.** + +2. ~~**Add a Postgres integration test for `TryClearProgress`** (optional but valuable) — current reset coverage is in-memory + Bruno CI against Postgres; a focused persistence test would mirror other quest-store integration tests and catch store divergence early.~~ **Done.** (`PlayerQuestProgressPersistenceIntegrationTests.TryClearProgress_ShouldRemoveRowAndPersistAcrossNewFactory`) + +3. ~~**Keep prototype quest id lists in sync** — `ALL_PROTOTYPE_QUEST_IDS` in `bruno-dev-fixture-helper.js` duplicates `PrototypeE7M1QuestCatalogRules` in C#. Add a one-line comment in the JS file pointing at the C# source of truth, or extract a shared JSON manifest if the roster grows again.~~ **Done.** (comment points at `PrototypeE7M1QuestCatalogRules.ExpectedQuestIds`) + +## Nits + +- ~~Nit: New ability-cast pre-requests use `require("./scripts/combat-targets-reset-helper")` without the `.js` suffix; most sibling `.bru` files use `.js`. Harmless in Bruno developer sandbox but inconsistent.~~ **Done.** +- Nit: `clearInventory` caps at 32 removal passes — sufficient for prototype bag sizes; error message is clear if exceeded. +- Nit: `assertAllPrototypeQuestsNotStarted` verifies every row in the GET response, not just the five reset ids — correct for the default-state test but will fail loudly if the catalog gains quests not listed in `ALL_PROTOTYPE_QUEST_IDS`. + +## Verification + +```bash +dotnet test NeonSprawl.sln --configuration Release +``` + +With Postgres (matches CI Bruno step): + +```bash +# Terminal 1 — enable all dev fixtures +ConnectionStrings__NeonSprawl='Host=localhost;Port=5432;Database=neon_sprawl;Username=neon_sprawl;Password=neon_sprawl_dev' \ +Game__EnableCombatTargetFixtureApi=true \ +Game__EnableQuestFixtureApi=true \ +Game__EnableResourceNodeFixtureApi=true \ +dotnet run --project server/NeonSprawl.Server/NeonSprawl.Server.csproj --urls http://127.0.0.1:5253 + +# Terminal 2 +cd bruno/neon-sprawl-server +npx --yes @usebruno/cli@3.3.0 run --env Local --sandbox=developer --bail +``` + +Manual spot-check: run the quest-progress collection **twice** back-to-back without restarting the server — `Get quest progress default.bru` and `Accept duplicate.bru` should pass both times. diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs index 8f4d352..08b3063 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs @@ -90,6 +90,35 @@ public sealed class PlayerQuestProgressPersistenceIntegrationTests(PostgresInteg Assert.Equal(CompletedAt, readBack.CompletedAt); } + [RequirePostgresFact] + public async Task TryClearProgress_ShouldRemoveRowAndPersistAcrossNewFactory() + { + // Arrange + await ResetQuestProgressTableAsync(); + var questId = PrototypeE7M1QuestCatalogRules.GatherIntroQuestId; + + // Act + using (var firstScope = Factory.Services.CreateScope()) + { + var store = firstScope.ServiceProvider.GetRequiredService(); + Assert.True(store.TryActivate(PlayerId, questId, out _)); + Assert.True(store.TryMarkComplete(PlayerId, questId, CompletedAt, out _)); + Assert.True(store.TryClearProgress(PlayerId, questId)); + Assert.False(store.TryGetProgress(PlayerId, questId, out _)); + } + + var readBackExists = false; + await using (var secondFactory = new PostgresWebApplicationFactory()) + { + using var secondScope = secondFactory.Services.CreateScope(); + var store = secondScope.ServiceProvider.GetRequiredService(); + readBackExists = store.TryGetProgress(PlayerId, questId, out _); + } + + // Assert + Assert.False(readBackExists); + } + private async Task ResetQuestProgressTableAsync() { var cs = Environment.GetEnvironmentVariable("ConnectionStrings__NeonSprawl"); diff --git a/server/README.md b/server/README.md index c299a40..ec01ae3 100644 --- a/server/README.md +++ b/server/README.md @@ -108,7 +108,13 @@ Quest accept evaluates **`FactionGateRule`** rows through **`FactionGateOperatio Plan: [NEO-137 implementation plan](../../docs/plans/NEO-137-implementation-plan.md). -**Dev quest fixture (NEO-137, Bruno/manual QA):** When `Game:EnableQuestFixtureApi` is **true** (default in **Development** via `appsettings.Development.json`, **Testing**, or CI Bruno step) or the host environment is **Development** / **Testing**, **`POST /game/players/{id}/__dev/quest-fixture`** accepts `schemaVersion` **1** and **`completedQuestIds`** — marks each quest **completed** via store activate + mark-complete **without reward delivery** (standing unchanged). Idempotent when a quest is already completed. **400** for bad schema; **404** when disabled, player unknown, unknown quest id, or store write fails. Not for production. Bruno: `quest-progress/Accept grid contract faction gate deny.bru` pre-request. +**Dev quest fixture (NEO-137, Bruno/manual QA):** When `Game:EnableQuestFixtureApi` is **true** (default in **Development** via `appsettings.Development.json`, **Testing**, or CI Bruno step) or the host environment is **Development** / **Testing**, **`POST /game/players/{id}/__dev/quest-fixture`** accepts `schemaVersion` **1** with optional: +- **`completedQuestIds`** — marks each quest **completed** via store activate + mark-complete **without reward delivery** (standing unchanged). Idempotent when a quest is already completed. +- **`resetQuestIds`** — deletes each quest's progress row for the player (idempotent when absent). Can be combined with **`completedQuestIds`** in one request (resets run first). + +**400** for bad schema; **404** when disabled, player unknown, unknown quest id, or store write fails. Not for production. Bruno: `quest-progress/Accept grid contract faction gate deny.bru` ( **`completedQuestIds`** ); order-independent accept/gather tests use **`resetQuestIds`** via **`scripts/bruno-dev-fixture-helper.js`**. + +**Bruno self-reset helpers:** `bruno/neon-sprawl-server/scripts/bruno-dev-fixture-helper.js` centralizes quest progress reset, resource-node restore, inventory clear, and post-reset verification (`resetAllPrototypeQuestProgress`, `resetGatherIntroSpine`, `resetPrototypeResourceNodes`, `clearInventory`). Combat HP/encounter reset remains **`scripts/combat-targets-reset-helper.js`**. ## Resource-node catalog (`content/resource-nodes`, NEO-58) @@ -521,7 +527,7 @@ Plan: [NEO-80 implementation plan](../../docs/plans/NEO-80-implementation-plan.m curl -sS -i "http://localhost:5253/game/world/combat-targets" ``` -**Dev combat-target fixture (Bruno/manual QA):** When `Game:EnableCombatTargetFixtureApi` is **true** (default in **Development** via `appsettings.Development.json`, **Testing**, or CI Bruno step) or the host environment is **Development** / **Testing**, **`POST /game/__dev/combat-targets-fixture`** with `schemaVersion` **1** resets all **`PrototypeNpcRegistry`** instances to catalog full HP via **`TryResetToFull`**, clears all prototype aggro holders (**NEO-92**), resets NPC runtime behavior rows to **`idle`** (**NEO-93**), restores dev player combat HP to full (**NEO-95**), and clears in-memory encounter progress/completion/event rows for the dev player across catalog encounters (**NEO-108** Bruno). **404** when disabled. Bruno: `scripts/combat-targets-reset-helper.js` (request scripts use `./scripts/…` relative to the collection). +**Dev combat-target fixture (Bruno/manual QA):** When `Game:EnableCombatTargetFixtureApi` is **true** (default in **Development** via `appsettings.Development.json`, **Testing**, or CI Bruno step) or the host environment is **Development** / **Testing**, **`POST /game/__dev/combat-targets-fixture`** with `schemaVersion` **1** resets all **`PrototypeNpcRegistry`** instances to catalog full HP via **`TryResetToFull`**, clears all prototype aggro holders (**NEO-92**), resets NPC runtime behavior rows to **`idle`** (**NEO-93**), restores dev player combat HP to full (**NEO-95**), clears **`IPlayerAbilityCooldownStore`** slot cooldowns for the configured dev player, and clears in-memory encounter progress/completion/event rows for the dev player across catalog encounters (**NEO-108** Bruno). **404** when disabled. Bruno: `scripts/combat-targets-reset-helper.js` (request scripts use `./scripts/…` relative to the collection). ## Threat / aggro state (NEO-92) @@ -690,6 +696,8 @@ World-wide **remaining gathers** per **`interactableId`** live in **`Game/Gather **Persistence:** same **[NEO-29-style](#position-persistence-neo-8) split** as inventory — **`resource_node_instance`** in PostgreSQL when **`ConnectionStrings:NeonSprawl`** is set (DDL [`V006__resource_node_instance.sql`](../db/migrations/V006__resource_node_instance.sql)), otherwise in-memory fallback (empty at startup). Interact deny wiring: [NEO-63](#gather-via-interact-neo-63); plan: [NEO-61 implementation plan](../../docs/plans/NEO-61-implementation-plan.md). +**Dev resource-node fixture (Bruno/manual QA):** When `Game:EnableResourceNodeFixtureApi` is **true** (default in **Development** via `appsettings.Development.json`, **Testing**, or CI Bruno step) or the host environment is **Development** / **Testing**, **`POST /game/__dev/resource-node-fixture`** with `schemaVersion` **1** upserts every catalog node to **`maxGathers`** via **`TrySetRemainingGathers`**. **404** when disabled. Bruno: `scripts/bruno-dev-fixture-helper.js` (`resetPrototypeResourceNodes`). + ## Gather engine (NEO-62) **`GatherOperations.TryGather`** in **`Game/Gathering/`** resolves server-internal **`GatherResult`**: capacity pre-check (may **initialize** a depletion row without decrementing), inventory grant via **`PlayerInventoryOperations`**, **`salvage`** skill XP via **`SkillProgressionGrantOperations`** + **`GatherSkillXpConstants`**, then depletion commit via **`ResourceNodeInstanceOperations`** (decrement **last**, only when inventory + XP succeed). On XP failure after inventory add, the engine **rolls back** the item grant before denying.