From 76e710b49136f1d8474e2ec8fa9975a8df879182 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 17 May 2026 18:46:25 -0400 Subject: [PATCH] =?UTF-8?q?NEO-47:=20re-review=20=E2=80=94=20follow-ups=20?= =?UTF-8?q?verified,=20approve=20for=20merge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/reviews/2026-05-17-NEO-47.md | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/docs/reviews/2026-05-17-NEO-47.md b/docs/reviews/2026-05-17-NEO-47.md index b05cade..0c52fc7 100644 --- a/docs/reviews/2026-05-17-NEO-47.md +++ b/docs/reviews/2026-05-17-NEO-47.md @@ -1,18 +1,20 @@ # 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` +**Scope:** Branch `NEO-47-perk-unlock-engine-perkstate-persistence` · commits `e297b3a`–`0a2a6fe` 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). +**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 -**Approve with nits** +**Approve** ## 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 @@ -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/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 -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 @@ -51,7 +66,7 @@ None. # From repo root 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 # Postgres integration (when ConnectionStrings__NeonSprawl is set)