NEO-138: re-review confirms follow-ups addressed.
parent
69c3ad4915
commit
57215d401a
|
|
@ -1,12 +1,13 @@
|
||||||
# Code review — NEO-138 (E7M3-06)
|
# Code review — NEO-138 (E7M3-06)
|
||||||
|
|
||||||
**Date:** 2026-06-15
|
**Date:** 2026-06-15
|
||||||
**Scope:** Branch `NEO-138-e7m3-reward-router-reputation-grants` — commits `599d87b` … `3466f1e` vs `main`
|
**Scope:** Branch `NEO-138-e7m3-reward-router-reputation-grants` — commits `599d87b` … `69c3ad4` vs `main`
|
||||||
**Base:** `main`
|
**Base:** `main`
|
||||||
|
**Follow-up:** Re-reviewed after `69c3ad4` (code-review follow-ups). Prior blocking/suggestions verified in code; **810** tests green.
|
||||||
|
|
||||||
## Verdict
|
## Verdict
|
||||||
|
|
||||||
**Approve** (review follow-ups addressed)
|
**Approve**
|
||||||
|
|
||||||
## Summary
|
## 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: `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: `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
|
## Verification
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue