70 lines
5.4 KiB
Markdown
70 lines
5.4 KiB
Markdown
# 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.
|