don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-06-15-bruno-self-reset...

5.4 KiB
Raw Permalink Blame History

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

dotnet test NeonSprawl.sln --configuration Release

With Postgres (matches CI Bruno step):

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