chore: address Bruno self-reset review — README, Postgres test, nits.

Document resetQuestIds, resource-node fixture, and cooldown clear in server README; add TryClearProgress Postgres integration test; sync-review doc and Bruno require paths.
pull/177/head
VinPropane 2026-06-15 21:53:44 -04:00
parent 95cf5d1f83
commit 64920792ee
6 changed files with 111 additions and 4 deletions

View File

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

View File

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

View File

@ -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",

View File

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

View File

@ -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<IPlayerQuestStateStore>();
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<IPlayerQuestStateStore>();
readBackExists = store.TryGetProgress(PlayerId, questId, out _);
}
// Assert
Assert.False(readBackExists);
}
private async Task ResetQuestProgressTableAsync()
{
var cs = Environment.GetEnvironmentVariable("ConnectionStrings__NeonSprawl");

View File

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