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

NEO-48: mark review items resolved — Approve verdict

pull/83/head
VinPropane 2026-05-17 20:38:02 -04:00
parent aba26446d3
commit e9c02eded7
2 changed files with 9 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
docs/reviews/2026-05-17-NEO-48-r2.md
View File

@ -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)

10
docs/reviews/2026-05-17-NEO-48.md
View File

@ -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