From 57215d401a92b1740dfde0affd447f04760850e9 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 15 Jun 2026 22:31:29 -0400 Subject: [PATCH] NEO-138: re-review confirms follow-ups addressed. --- docs/reviews/2026-06-15-NEO-138.md | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/docs/reviews/2026-06-15-NEO-138.md b/docs/reviews/2026-06-15-NEO-138.md index 80cf308..c43c9be 100644 --- a/docs/reviews/2026-06-15-NEO-138.md +++ b/docs/reviews/2026-06-15-NEO-138.md @@ -1,12 +1,13 @@ # Code review — NEO-138 (E7M3-06) **Date:** 2026-06-15 -**Scope:** Branch `NEO-138-e7m3-reward-router-reputation-grants` — commits `599d87b` … `3466f1e` vs `main` -**Base:** `main` +**Scope:** Branch `NEO-138-e7m3-reward-router-reputation-grants` — commits `599d87b` … `69c3ad4` vs `main` +**Base:** `main` +**Follow-up:** Re-reviewed after `69c3ad4` (code-review follow-ups). Prior blocking/suggestions verified in code; **810** tests green. ## Verdict -**Approve** (review follow-ups addressed) +**Approve** ## Summary @@ -39,6 +40,21 @@ This branch lands **E7M3-06**: **`RewardRouterOperations.TryDeliverQuestCompleti - ~~Nit: `TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull` does not assert `GrantedReputation` is empty — mirror item/skill empty assertions for snapshot parity.~~ **Done.** - Nit: `CompensatingRevertReputation` skips `Amount == 0` rows; fine for prototype bundles but a clamp-to-zero edge case could leave standing changed while snapshot says 0 — defer unless content adds negative grants near floor. +- Nit (re-review): `server/README.md` FactionGateOperations section still says grid-contract success Bruno is “deferred until NEO-138” — stale; seq **11** landed. One-line README tweak optional before merge. +- Nit (re-review): `TryDeliverQuestCompletion_ShouldRollbackReputation_WhenTryRecordLosesRace` asserts standing only on a full operator-chain bundle; item/skill rollback on the same path is covered by the older skill-only race test + shared `CompensatingRevertAll` — acceptable, not blocking. + +## Re-review (2026-06-15) + +| Original finding | Verified | +|------------------|----------| +| Bruno seq **11** status `active` vs `completed` | **Fixed** — `Accept grid contract after operator chain.bru` expects `completed`; docs note auto-complete when kit held. | +| Race rep rollback test | **Fixed** — `TryDeliverQuestCompletion_ShouldRollbackReputation_WhenTryRecordLosesRace`. | +| Audit row on operator-chain happy path | **Fixed** — delta id + row fields asserted in bundle test. | +| Grid-contract accept comment | **Fixed** — `QuestStateOperationsTests` Arrange comment + Bruno docs. | +| Multi-row audit-append rollback | **Fixed** — `AuditStoreFailingOnSecondForwardRepAppend` + full grant rollback test. | +| Empty `GrantedReputation` on null bundle | **Fixed** — result + stored event asserted. | + +No new blocking issues from follow-up commit. Test stubs (`AuditStoreFailingOnSecondForwardRepAppend`, race-loss store) are sound; AAA layout on new tests matches project conventions. ## Verification