NEO-48: add code review for perk-state HTTP
parent
39c54c3881
commit
0838e661c9
|
|
@ -0,0 +1,58 @@
|
|||
# Code review — NEO-48 perk state GET + branch selection POST
|
||||
|
||||
**Date:** 2026-05-17
|
||||
**Scope:** Branch `NEO-48-perk-state-get-branch-selection-post-bruno` · commits `5290044`–`39c54c3` vs `main`
|
||||
**Base:** `main`
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits**
|
||||
|
||||
## Summary
|
||||
|
||||
NEO-48 adds thin HTTP mapping for versioned **`GET`/`POST /game/players/{id}/perk-state`**: read model (`branchPicks`, `unlockedPerkIds`), branch selection via **`PerkUnlockEngine.TrySelectBranch`**, and structured **`selected`** / **`reasonCode`** / **`perkState`** / **`unlockedEvents`** envelopes mirroring NEO-38 grant patterns. Eight integration tests cover 404/400 gates, happy tier-1 pick after XP grant, `level_too_low` / `branch_already_chosen` denies, and path-auto **`unlockedEvents`** at level 4+. Bruno folder, README, manual QA, and E2.M3 decomposition rows align with the implementation plan. **185** server tests pass locally. Low merge risk; no engine or persistence logic duplicated in the API layer.
|
||||
|
||||
## Documentation checked
|
||||
|
||||
| Document | Result |
|
||||
|----------|--------|
|
||||
| [`docs/plans/NEO-48-implementation-plan.md`](../plans/NEO-48-implementation-plan.md) | **Matches** — same-path GET/POST, schema v1, deny envelope, `unlockedEvents` on success, position-state 404 gate, Bruno/README/tests/manual QA; acceptance checklist complete. |
|
||||
| [`docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md`](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) | **Matches** — NEO-48 handoff section; HTTP responsibility listed under module responsibilities. |
|
||||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E2.M3 row cites NEO-48 landed + README/Bruno links. |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E2.M3 note includes NEO-48 perk-state HTTP. |
|
||||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — branch picks and unlock evaluation remain server-authoritative; no client changes (out of scope). |
|
||||
| [`docs/plans/NEO-47-implementation-plan.md`](../plans/NEO-47-implementation-plan.md) | **Matches** — API delegates to engine/store; no duplicate unlock rules; telemetry stays in `TryUnlockPerks` (NEO-49). |
|
||||
|
||||
Register/tracking: E2.M3 module doc, alignment row, and register note updated in implementation commit.
|
||||
|
||||
## Blocking issues
|
||||
|
||||
None.
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. **Bruno deny for `branch_already_chosen`** — Plan and manual QA describe a second POST to the same tier with a different branch; automated tests cover it (`PostPerkState_ShouldDenyBranchAlreadyChosen_OnSecondPick`), but Bruno only ships `Post branch select deny level too low.bru`. A fourth request (grant + first pick + conflicting second pick) would make the collection self-contained for CI/manual runs without relying on manual QA alone.
|
||||
2. **Optional thin HTTP deny smoke** — Engine tests lock `unknown_track`, `unknown_tier`, and `unknown_branch`; one API test each (bad `skillId`, bad `tierIndex`, bad `branchId` on dev player after level gate) would guard JSON field mapping if DTOs change. Non-blocking given `PerkUnlockEngineTests` coverage.
|
||||
3. **POST null body** — Handlers return **400** for `body is null` (same as skill-progression). Neither API suite asserts empty-body **400** today; add a one-liner in `PerkStateApiTests` if you want parity documentation in tests.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: `Post branch select deny level too low.bru` intentionally passes when salvage is already leveled (restart note) — acceptable for shared dev player; document in Bruno folder `docs` if operators hit false greens often.
|
||||
- Nit: Path-auto tier-2 unlocks surfaced in **`unlockedEvents`** use **`source: level_up`** (not `branch_pick`) — consistent with NEO-47/NEO-49 notes; clients/telemetry should treat `source` as unlock channel, not HTTP verb.
|
||||
- Nit: `PostPerkState_ShouldSelectTier1_AndGetReflectsPick_AfterSalvageLevel2` issues a follow-up **GET** inside **Assert** for end-to-end verification — valid AAA (verification in Assert); grant status checks in **Arrange** match other API tests in the repo.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# From repo root
|
||||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkStateApi"
|
||||
|
||||
# Full server suite (185 tests at review)
|
||||
dotnet test NeonSprawl.sln
|
||||
|
||||
# Manual / Bruno
|
||||
# docs/manual-qa/NEO-48.md
|
||||
# bruno/neon-sprawl-server/perk-state/
|
||||
```
|
||||
|
||||
Confirm **GET** empty state, **POST** tier-1 happy path after salvage grant (100 XP), **POST** deny codes, and **GET** reflects persisted pick; optional path-auto **`unlockedEvents`** at level 4+ (500 total XP before pick).
|
||||
Loading…
Reference in New Issue