From 2520a80b6e8ac09ab43b0fc00980d3bc6586a01b Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 17 May 2026 18:42:28 -0400 Subject: [PATCH] NEO-47: add code review for perk unlock engine --- docs/reviews/2026-05-17-NEO-47.md | 59 +++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 docs/reviews/2026-05-17-NEO-47.md diff --git a/docs/reviews/2026-05-17-NEO-47.md b/docs/reviews/2026-05-17-NEO-47.md new file mode 100644 index 0000000..8dab73d --- /dev/null +++ b/docs/reviews/2026-05-17-NEO-47.md @@ -0,0 +1,59 @@ +# Code review — NEO-47 perk unlock engine + PerkState persistence + +**Date:** 2026-05-17 +**Scope:** Branch `NEO-47-perk-unlock-engine-perkstate-persistence` · commits `e297b3a`–`38774a5` vs `main` +**Base:** `main` + +## Verdict + +**Approve with nits** + +## Summary + +This change delivers server-authoritative perk state (`IPlayerPerkStateStore` in-memory + Postgres `V004`), a `PerkUnlockEngine` with explicit tier-1 branch picks and path-auto tier-2 unlocks for prototype salvage, and a level-up hook inside `SkillProgressionGrantOperations.TryApplyGrant` wired through HTTP POST, gather interact, and existing XP grant helpers. Tests cover deny paths, reevaluation idempotency, Postgres persistence across factories, and an HTTP grant smoke that crosses level 4 after a tier-1 pick. All **172** server tests pass locally. Overall risk is moderate-low: new persistence tables and grant-path side effects, but no public perk HTTP yet (NEO-48) and prototype catalog remains a single track. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-47-implementation-plan.md`](../plans/NEO-47-implementation-plan.md) | **Matches** — store, engine, V004, hook site, reason codes, acceptance checklist marked complete; tier-1 pick / tier-2 auto-path behavior implemented as adopted kickoff decisions. | +| [`docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md`](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) | **Partially matches** — responsibilities and contracts align; module **Status** / NEO-47 handoff bullets not yet updated (still “NEO-46 landed” only). | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Partially matches** — E2.M3 row still lists perk engine as “Still planned”; should note NEO-47 landed after merge. | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — unlock evaluation and persistence remain server-side; client unchanged until NEO-48. | +| [`docs/plans/NEO-46-implementation-plan.md`](../plans/NEO-46-implementation-plan.md) | **Matches** — engine consumes `IMasteryCatalogRegistry` and repo catalog; no loader regression in diff. | + +Register/tracking: update E2.M3 row and module doc when merging (non-blocking doc follow-up). + +## Blocking issues + +None. + +## Suggestions + +1. **Retroactive path-auto unlock after branch pick** — `TrySelectBranch` persists tier-1 picks but does not re-run path-auto evaluation. A player who reaches salvage **level 4+** before choosing tier 1 will not receive tier-2 perks until a **later** level-up, even though `requiredLevel` was already satisfied. NEO-48 branch POST will make this path easy to hit. After a successful pick, call `ReevaluateAfterLevelUp(playerId, skillId, currentLevel)` (level from `GetSkillLevel`) or extract shared “apply path-auto tiers ≤ level” logic. Add a test: XP to 450+, pick tier 1, assert tier-2 perk without another grant. +2. **Deny-code test gaps vs plan** — Plan table lists `unknown_track`, `unknown_tier`, and `player_not_found`. Tests cover `level_too_low`, `branch_already_chosen`, `unknown_branch`, and `tier_branch_not_selectable` but not the three above. Small engine unit tests (bad `skillId`, bad `tierIndex`, unknown player id on in-memory store without seed) would lock NEO-48 HTTP mapping. +3. **`UnlockPerks` store failure** — When `TryAddUnlockedPerks` returns `false`, the method returns an empty event list while `TrySelectBranch` may have already committed the branch pick. Harmless for prototype tier 1 (empty `perkIds`), but worth surfacing `player_not_found` on the outcome or rolling back the pick before NEO-48/expanded catalogs add perks on pick tiers. +4. **Decomposition doc refresh** — Mirror NEO-46 follow-up: add NEO-47 handoff to `E2_M3_MasteryAndPerkUnlocks.md` (engine + stores, no HTTP) and update the E2.M3 row in `documentation_and_implementation_alignment.md`. +5. **Gather-hook integration smoke** — Plan mentions `InteractionApi` grant path; implementation wires `perkUnlockEngine` correctly, but only `POST …/skill-progression` is smoke-tested for level-4 unlock. One interact test (or manual QA note in NEO-48) would close the loop for NEO-41 parity. + +## Nits + +- Nit: `PerkUnlockEngineTests.CreateEngine` opens a `IServiceScope` without disposing; use `using var scope = …` or resolve from a single test scope. +- Nit: `ReevaluateAfterLevelUp` calls `perkStore.GetSnapshot` inside the per-tier loop; a single snapshot per invocation is enough. +- Nit: Empty `skillId` / `branchId` on `TrySelectBranch` maps to `unknown_track` rather than a dedicated validation code — acceptable if NEO-48 trims inputs first. +- Nit: Plan risk “loader comment cross-ref to engine tier rule” not added; optional comment on `MasteryCatalogLoader` or `TierRequiresExplicitPick` pointing to `PerkUnlockEngine`. + +## Verification + +```bash +# From repo root +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkUnlock|FullyQualifiedName~PerkState" + +# Full server suite (recommended before merge) +dotnet test NeonSprawl.sln + +# Postgres integration (when ConnectionStrings__NeonSprawl is set) +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkStatePersistenceIntegration" +``` + +Manual: with Postgres, confirm `V004` tables exist after first perk write; grant salvage XP 100 → select tier-1 branch (future NEO-48) → grant to 450+ and confirm internal perk state via snapshot store or DB query until HTTP lands.