68 lines
6.4 KiB
Markdown
68 lines
6.4 KiB
Markdown
# Code review (re-review) — NEO-48 testing refactor + mastery fixture API
|
||
|
||
**Date:** 2026-05-17
|
||
**Scope:** Branch `NEO-48-perk-state-get-branch-selection-post-bruno` · commits `3ba8300`–`ffdbed2` (delta since [first review](2026-05-17-NEO-48.md)) · full branch vs `main` for merge readiness
|
||
**Base:** `main`
|
||
**Prior review:** [`2026-05-17-NEO-48.md`](2026-05-17-NEO-48.md) (perk-state HTTP + initial Bruno/tests)
|
||
**Follow-up:** R2 suggestions 1–3 and nits 1 + 3 + 4 addressed in `aba2644`; re-verified at **195** passing server tests.
|
||
|
||
## Verdict
|
||
|
||
**Approve**
|
||
|
||
## Summary
|
||
|
||
Follow-up work closes the first-review Bruno/test gaps and replaces a short-lived **fixture-player seeding** experiment with a cleaner **dev-only mastery fixture API**. **`POST /game/players/{id}/__dev/mastery-fixture`** (gated to Development, Testing, or `Game:EnableMasteryFixtureApi`) resets perk state and sets absolute skill XP without going through grant/level-up hooks—giving deterministic Bruno deny smokes on shared `dev-local-1`. **`PerkStateApiTests`** adds null-body **400** and HTTP deny smoke for `unknown_track` / `unknown_tier` / `unknown_branch`; **`MasteryFixtureApiTests`** verifies Production+flag-off returns **404** and that reset + XP zero yields **`level_too_low`** on the next branch POST. Bruno deny requests use `script:pre-request` + `axios` against the fixture endpoint; `{{playerId}}` from `Local.bru` is used consistently on deny flows. **191** server tests pass. Merge risk remains low; the new surface is explicitly non-production and tested for disable behavior.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-48-implementation-plan.md`](../plans/NEO-48-implementation-plan.md) | **Matches** — perk-state HTTP/Bruno acceptance unchanged; kickoff documents fixture API; **Files to add/modify** lists `MasteryFixture*` and store fixture methods. |
|
||
| [`docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md`](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) | **Matches** — NEO-48 handoff covers HTTP only; dev fixture correctly omitted from module contracts. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — production perk rules still flow through `PerkUnlockEngine`; fixture bypasses grant path by design (dev/QA only). |
|
||
| [`docs/manual-qa/NEO-48.md`](../manual-qa/NEO-48.md) | **Matches** — fixture curl examples + Bruno deny checklist updated. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — Perk state section documents fixture API, gating, and **404** when disabled. |
|
||
| Prior [`2026-05-17-NEO-48.md`](2026-05-17-NEO-48.md) | **Superseded for merge** by this re-review for the testing-refactor delta; first-review suggestions 1–3 marked done there remain valid. |
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Atomic fixture apply** — `MasteryFixtureOperations.TryApply` can leave mixed state if `resetPerkState` succeeds and a later `skillXp` entry fails (or an earlier XP write succeeds before a later one fails). For a dev endpoint this is unlikely with Bruno’s fixed payloads, but consider applying XP first then reset, a single transaction on Postgres, or returning **409**/**500** with a note when partial apply is detected.~~ **Done.** Batch **`TrySetSkillXpTotals`** then **`resetPerkState`**; XP rollback via previous snapshot if reset fails; **400** for invalid `skillXp` before writes.
|
||
2. ~~**Fixture API test coverage** — Add small tests for **400** (null body / bad `schemaVersion`) and **404** (unknown player, negative XP in `skillXp`) on `__dev/mastery-fixture` to mirror perk-state gate tests.~~ **Done.** Four new tests in `MasteryFixtureApiTests`.
|
||
3. ~~**Document fixture vs grant semantics** — README could note that **`skillXp` on the fixture sets absolute totals without running `SkillProgressionGrantOperations` / perk level-up reevaluation** (intentional for deny setup; use normal grant API when testing unlock side effects).~~ **Done.** README Perk state section.
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: `Get perk state.bru` still asserts `playerId === "dev-local-1"` while deny requests use `{{playerId}}` from the environment—harmless if `Local.bru` keeps the default; align assertions for copy-paste environments.~~ **Done.** GET uses `{{playerId}}` in URL and assertion.
|
||
- ~~Nit: `TryResetPerkState` / `TrySetSkillXpTotal` live on production store interfaces—acceptable for prototype; if the surface grows, consider a dev-only `IMasteryFixtureStore` facade later.~~ **Deferred.** Acceptable for E2.M3 prototype; revisit if more dev-only store methods accumulate.
|
||
- ~~Nit: Bruno deny flows mutate shared `dev-local-1`; folder `docs` explain fixture pre-requests—run deny requests before happy-path smokes or accept order sensitivity in manual runs.~~ **Done.** Folder `docs` note on run order.
|
||
- ~~Nit: `MasteryFixtureApiTests.PostMasteryFixture_ShouldResetSalvageXpAndPerkState_ThenLevelTooLowDenies` performs reset and deny POST both under **Act**; valid for an integration scenario, but heavy **Arrange** (grant + pick) could move to a private helper like `PerkStateApiTests.GrantSalvageLevel2Async`.~~ **Done.** `ArrangeSalvageProgressedWithTier1PickAsync`.
|
||
|
||
## Delta since first review (testing refactor)
|
||
|
||
| Area | Change |
|
||
|------|--------|
|
||
| **Bruno** | `Post branch select deny branch already chosen.bru`; `level_too_low` uses fixture pre-request (no skip-on-already-leveled); `{{playerId}}` + `baseUrl` resolution fixed. |
|
||
| **Server** | `MasteryFixtureApi` / `MasteryFixtureOperations` / DTOs; `Program.cs` conditional map; `EnableMasteryFixtureApi` in appsettings; store `TryResetPerkState` + `TrySetSkillXpTotal` (in-memory + Postgres). |
|
||
| **Tests** | `MasteryFixtureApiTests` (2); `PerkStateApiTests` +4 deny/null-body; `InMemoryWebApplicationFactory` enables fixture flag for Testing. |
|
||
| **Reverted approach** | `GamePlayerSeed` / extra fixture player ids (commit `b72201b`) removed in favor of fixture API (`ffdbed2`)—simpler ops story. |
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# From repo root
|
||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkState|MasteryFixture"
|
||
|
||
# Full server suite (195 tests after aba2644)
|
||
dotnet test NeonSprawl.sln
|
||
|
||
# Bruno (Development server, EnableMasteryFixtureApi on)
|
||
# bruno/neon-sprawl-server/perk-state/
|
||
```
|
||
|
||
Confirm deny Bruno requests pass on a fresh dev server; confirm `POST …/__dev/mastery-fixture` returns **404** when `ASPNETCORE_ENVIRONMENT=Production` and `Game:EnableMasteryFixtureApi=false`.
|