From e9c02eded794865341c3b2baf204e22b42c40137 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 17 May 2026 20:38:02 -0400 Subject: [PATCH] =?UTF-8?q?NEO-48:=20mark=20review=20items=20resolved=20?= =?UTF-8?q?=E2=80=94=20Approve=20verdict?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/reviews/2026-05-17-NEO-48-r2.md | 8 ++++---- docs/reviews/2026-05-17-NEO-48.md | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/reviews/2026-05-17-NEO-48-r2.md b/docs/reviews/2026-05-17-NEO-48-r2.md index 13a4f02..8256452 100644 --- a/docs/reviews/2026-05-17-NEO-48-r2.md +++ b/docs/reviews/2026-05-17-NEO-48-r2.md @@ -4,11 +4,11 @@ **Scope:** Branch `NEO-48-perk-state-get-branch-selection-post-bruno` · commits `3ba8300`–`ffdbed2` (delta since [first review](2026-05-17-NEO-48.md)) · full branch vs `main` for merge readiness **Base:** `main` **Prior review:** [`2026-05-17-NEO-48.md`](2026-05-17-NEO-48.md) (perk-state HTTP + initial Bruno/tests) -**Follow-up:** R2 suggestions 1–3 and nits 1 + 3 addressed in a follow-up commit on the same branch. +**Follow-up:** R2 suggestions 1–3 and nits 1 + 3 + 4 addressed in `aba2644`; re-verified at **195** passing server tests. ## Verdict -**Approve with nits** +**Approve** ## Summary @@ -38,7 +38,7 @@ None. ## Nits - ~~Nit: `Get perk state.bru` still asserts `playerId === "dev-local-1"` while deny requests use `{{playerId}}` from the environment—harmless if `Local.bru` keeps the default; align assertions for copy-paste environments.~~ **Done.** GET uses `{{playerId}}` in URL and assertion. -- Nit: `TryResetPerkState` / `TrySetSkillXpTotal` live on production store interfaces—acceptable for prototype; if the surface grows, consider a dev-only `IMasteryFixtureStore` facade later. +- ~~Nit: `TryResetPerkState` / `TrySetSkillXpTotal` live on production store interfaces—acceptable for prototype; if the surface grows, consider a dev-only `IMasteryFixtureStore` facade later.~~ **Deferred.** Acceptable for E2.M3 prototype; revisit if more dev-only store methods accumulate. - ~~Nit: Bruno deny flows mutate shared `dev-local-1`; folder `docs` explain fixture pre-requests—run deny requests before happy-path smokes or accept order sensitivity in manual runs.~~ **Done.** Folder `docs` note on run order. - ~~Nit: `MasteryFixtureApiTests.PostMasteryFixture_ShouldResetSalvageXpAndPerkState_ThenLevelTooLowDenies` performs reset and deny POST both under **Act**; valid for an integration scenario, but heavy **Arrange** (grant + pick) could move to a private helper like `PerkStateApiTests.GrantSalvageLevel2Async`.~~ **Done.** `ArrangeSalvageProgressedWithTier1PickAsync`. @@ -57,7 +57,7 @@ None. # From repo root dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkState|MasteryFixture" -# Full server suite (191 tests at re-review) +# Full server suite (195 tests after aba2644) dotnet test NeonSprawl.sln # Bruno (Development server, EnableMasteryFixtureApi on) diff --git a/docs/reviews/2026-05-17-NEO-48.md b/docs/reviews/2026-05-17-NEO-48.md index b2a376f..f29e369 100644 --- a/docs/reviews/2026-05-17-NEO-48.md +++ b/docs/reviews/2026-05-17-NEO-48.md @@ -3,11 +3,11 @@ **Date:** 2026-05-17 **Scope:** Branch `NEO-48-perk-state-get-branch-selection-post-bruno` · commits `5290044`–`39c54c3` vs `main` **Base:** `main` -**Follow-up:** Code-review suggestions 1–3 and nit 1 addressed in a follow-up commit on the same branch. +**Follow-up:** Suggestions 1–3 and nit 1 addressed on branch; r2 follow-up (`aba2644`) closed fixture/testing items — see [`2026-05-17-NEO-48-r2.md`](2026-05-17-NEO-48-r2.md). ## Verdict -**Approve with nits** +**Approve** ## Summary @@ -39,8 +39,8 @@ None. ## Nits - ~~Nit: `Post branch select deny level too low.bru` intentionally passes when salvage is already leveled (restart note) — acceptable for shared dev player; document in Bruno folder `docs` if operators hit false greens often.~~ **Done.** Folder `docs` block in `perk-state/folder.bru`. -- Nit: Path-auto tier-2 unlocks surfaced in **`unlockedEvents`** use **`source: level_up`** (not `branch_pick`) — consistent with NEO-47/NEO-49 notes; clients/telemetry should treat `source` as unlock channel, not HTTP verb. -- Nit: `PostPerkState_ShouldSelectTier1_AndGetReflectsPick_AfterSalvageLevel2` issues a follow-up **GET** inside **Assert** for end-to-end verification — valid AAA (verification in Assert); grant status checks in **Arrange** match other API tests in the repo. +- ~~Nit: Path-auto tier-2 unlocks surfaced in **`unlockedEvents`** use **`source: level_up`** (not `branch_pick`) — consistent with NEO-47/NEO-49 notes; clients/telemetry should treat `source` as unlock channel, not HTTP verb.~~ **Deferred.** Documented in NEO-47/NEO-49; no API change required for NEO-48. +- ~~Nit: `PostPerkState_ShouldSelectTier1_AndGetReflectsPick_AfterSalvageLevel2` issues a follow-up **GET** inside **Assert** for end-to-end verification — valid AAA (verification in Assert); grant status checks in **Arrange** match other API tests in the repo.~~ **Deferred.** Accepted integration-test pattern; no change. ## Verification @@ -48,7 +48,7 @@ None. # From repo root dotnet test NeonSprawl.sln --filter "FullyQualifiedName~PerkStateApi" -# Full server suite (185 tests at review) +# Full server suite (195 tests at final verification) dotnet test NeonSprawl.sln # Manual / Bruno