60 lines
5.5 KiB
Markdown
60 lines
5.5 KiB
Markdown
# 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.
|