From 0838e661c9109ad8f5ce5708b67f7c7fdf7e9a92 Mon Sep 17 00:00:00 2001
From: VinPropane <don@donmail.org>
Date: Sun, 17 May 2026 20:16:46 -0400
Subject: [PATCH] NEO-48: add code review for perk-state HTTP

---
 docs/reviews/2026-05-17-NEO-48.md | 58 +++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 docs/reviews/2026-05-17-NEO-48.md

diff --git a/docs/reviews/2026-05-17-NEO-48.md b/docs/reviews/2026-05-17-NEO-48.md
new file mode 100644
index 0000000..f25ae3f
--- /dev/null
+++ b/docs/reviews/2026-05-17-NEO-48.md
@@ -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).