don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-47: re-review — follow-ups verified, approve for merge

pull/81/head
VinPropane 2026-05-17 18:46:25 -04:00
parent 0a2a6fe45b
commit 76e710b491
1 changed files with 22 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
docs/reviews/2026-05-17-NEO-47.md
View File

@ -1,18 +1,20 @@
# Code review — NEO-47 perk unlock engine + PerkState persistence # Code review — NEO-47 perk unlock engine + PerkState persistence
**Date:** 2026-05-17 **Date:** 2026-05-17
**Scope:** Branch `NEO-47-perk-unlock-engine-perkstate-persistence` · commits `e297b3a`–`38774a5` vs `main` **Scope:** Branch `NEO-47-perk-unlock-engine-perkstate-persistence` · commits `e297b3a`–`0a2a6fe` vs `main`
**Base:** `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). **Follow-up:** Addressed review suggestions in `0a2a6fe` (retroactive path-auto on branch pick, deny-code tests, unlock rollback, decomposition docs, gather-hook smoke, nits).
**Re-review (2026-05-17):** Verified follow-up commit against prior suggestions; no blocking regressions; **177** server tests pass.
## Verdict ## Verdict
**Approve with nits** **Approve**
## Summary ## 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. Server-authoritative perk state (`IPlayerPerkStateStore` in-memory + Postgres `V004`), `PerkUnlockEngine` (tier-1 picks, path-auto tier-2, retroactive path-auto after pick at current level), and level-up re-evaluation in `SkillProgressionGrantOperations` (HTTP POST, gather interact, mission/refine helpers). Follow-up commit adds `ApplyPathAutoTiersAtOrBelowLevel`, branch-pick rollback on perk write failure, `CanWritePlayer` / `TryRemoveBranchPick`, expanded deny tests, gather-hook smoke, and E2.M3 doc alignment. All **177** server tests pass locally. Ready for merge from a code perspective; perk HTTP remains **NEO-48**.
## Documentation checked ## Documentation checked
@ -24,11 +26,24 @@ This change delivers server-authoritative perk state (`IPlayerPerkStateStore` in
| [`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/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. | | [`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). Register/tracking: E2.M3 module doc and alignment row updated in follow-up (`0a2a6fe`).
## Blocking issues ## Blocking issues
None. None (initial review and re-review).
## Re-review (2026-05-17)
| Prior item | Status |
|------------|--------|
| Retroactive path-auto after branch pick | **Verified** — `ApplyPathAutoTiersAtOrBelowLevel` called from `TrySelectBranch`; test `TrySelectBranch_AtLevel4AfterXpGate_ShouldUnlockTier2PerkWithoutExtraGrant`. |
| Deny-code tests (`unknown_track`, `unknown_tier`, `player_not_found`) | **Verified** — three new `[Fact]` methods in `PerkUnlockEngineTests.cs`. |
| Unlock rollback on store failure | **Verified** — `TryRemoveBranchPick` + deny `player_not_found` when `TryUnlockPerks` fails after pick. |
| Decomposition docs | **Verified** — `E2_M3_MasteryAndPerkUnlocks.md` NEO-47 handoff; alignment table row updated. |
| Gather-hook smoke | **Verified** — `PostInteract_ResourceNode_AfterTier1PickNearLevel4_ShouldUnlockPerkViaGatherHook` (440 + 10 XP gather). |
| Test scope / nits | **Verified** — singleton resolve from `factory.Services`; loader cross-ref on `TryGetTierIndexGateError`. |
**New issues from follow-up:** None blocking. Optional nit retained below (`unknown_track` for empty ids). Minor note: path-auto unlock events emitted from `TrySelectBranch` use `PerkUnlockSource.LevelUp` (same as reevaluate path) — acceptable for NEO-49 to refine if telemetry needs a distinct source for retroactive unlocks.
## Suggestions ## Suggestions
@ -51,7 +66,7 @@ None.
# From repo root # From repo root
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkUnlock|FullyQualifiedName~PerkState" dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkUnlock|FullyQualifiedName~PerkState"
# Full server suite (recommended before merge) # Full server suite (177 tests at re-review)
dotnet test NeonSprawl.sln dotnet test NeonSprawl.sln
# Postgres integration (when ConnectionStrings__NeonSprawl is set) # Postgres integration (when ConnectionStrings__NeonSprawl is set)