From 2bb02d76e92c522ba680f7262a29a393ffce0e26 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 17 May 2026 20:34:36 -0400 Subject: [PATCH] NEO-48: add re-review for mastery fixture testing refactor --- docs/reviews/2026-05-17-NEO-48-r2.md | 66 ++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 docs/reviews/2026-05-17-NEO-48-r2.md diff --git a/docs/reviews/2026-05-17-NEO-48-r2.md b/docs/reviews/2026-05-17-NEO-48-r2.md new file mode 100644 index 0000000..98d4a0f --- /dev/null +++ b/docs/reviews/2026-05-17-NEO-48-r2.md @@ -0,0 +1,66 @@ +# 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) + +## Verdict + +**Approve with nits** + +## 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) | **Partially matches** — perk-state HTTP/Bruno acceptance unchanged; kickoff table documents **`__dev/mastery-fixture`** for Bruno deny setup. **Files to add/modify** tables do not list `MasteryFixture*.cs` or store `TryReset*` / `TrySetSkillXpTotal` (implementation detail worth a one-line plan add). | +| [`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. +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. +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). + +## 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. +- Nit: `TryResetPerkState` / `TrySetSkillXpTotal` live on production store interfaces—acceptable for prototype; if the surface grows, consider a dev-only `IMasteryFixtureStore` facade later. +- 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. +- 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`. + +## 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 (191 tests at re-review) +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`.