don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-17-NEO-47.md

6.2 KiB
Raw Blame History

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

Follow-up: Addressed review suggestions (retroactive path-auto on branch pick, deny-code tests, unlock rollback, decomposition docs, gather-hook smoke, nits).

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 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 Matches — NEO-47 handoff + Status updated after follow-up.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E2.M3 row notes NEO-47 landed after follow-up.
docs/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 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. Done. ApplyPathAutoTiersAtOrBelowLevel shared by pick + level-up; test TrySelectBranch_AtLevel4AfterXpGate_ShouldUnlockTier2PerkWithoutExtraGrant.
  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. Done. Added TrySelectBranch_WhenUnknownTrack_*, WhenUnknownTier_*, WhenPlayerNotInStore_*.
  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. Done. TryUnlockPerks returns false → TryRemoveBranchPick rollback + player_not_found deny.
  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. Done.
  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. Done. PostInteract_ResourceNode_AfterTier1PickNearLevel4_ShouldUnlockPerkViaGatherHook.

Nits

  • Nit: PerkUnlockEngineTests.CreateEngine opens a IServiceScope without disposing; use using var scope = … or resolve from a single test scope. Done. Resolve singletons from factory.Services.
  • Nit: ReevaluateAfterLevelUp calls perkStore.GetSnapshot inside the per-tier loop; a single snapshot per invocation is enough. Done. ApplyPathAutoTiersAtOrBelowLevel refreshes snapshot only after successful unlock batches.
  • 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. Done. Cross-ref on TryGetTierIndexGateError and PerkUnlockEngine class summary.

Verification

# 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.