feat/review-rigor
needs attentionviewing older commitdb5fe05 · fullpre-PRreviewed 2026-07-07 03:59 UTC2H · 2M · 2L · 4I- Purpose
- Add review-rigor infrastructure: codify three bars a fix must clear (root-cause, blast-radius, regression test) and prove them with property-based tests on the two highest-stakes decision cores.
- Goal
- Encode the 'definition of done for a fix' into loops.md + review-panel SKILL.md (so the correctness and tests lenses enforce it), document property-based testing in testing.md, add fast-check, and ship two example property-test files covering the last-owner invariant and site status-machine invariants.
- Sub-goals
- SG-1: loops.md — definition-of-done section (root-cause / blast-radius / regression-test)
- SG-2: testing.md — property-based tests section with fast-check patterns
- SG-3: review-panel SKILL.md — blast-radius sub-check + fix-gate additions
- SG-4: fast-check devDep + property tests for membership.decisions (6 properties) and site.decisions (5 properties)
- What
- Added 'definition of done for a fix' section to loops.md; property-based testing section to testing.md; blast-radius sub-check and fix-gate mandatory items to review-panel SKILL.md correctness and tests lenses; fast-check 3.23.2 as devDependency in domains/core; two new property-test files totalling 202 lines of fast-check properties.
- Why
- The loops review panel was commenting-only with no enforcement of root-cause vs symptom, no blast-radius naming, and no regression-test requirement. This branch adds that enforcement and provides reference examples so future implementations follow the pattern.
- Areas
- .claude/rules+53−0.claude/skills/review-panel+2−2domains/core (package.json + 2 test files)+203−0
- Blast
- 3 areas, 6 files (excluding lockfile), +258/−2. Pure documentation and devDependency changes — zero production code, zero API contracts, zero shared types or decision signatures modified.
Findings · 10
correctness1
Test 5 (idempotency) is correct but slightly coupled to implementation ordering
domains/core/src/membership/__tests__/membership.decisions.property.test.ts:124
The test verifies decideRemoveMember returns ok for status='removed' — which is true due to the early status check before the owner check. The property holds, but it implicitly relies on the status check being ordered before the owner guard. A purer framing would be: 'idempotent removal never errors for any already-removed membership.' Minor and no functional bug.
conventions1
Conventions: all clean
File naming ({entity}.decisions.property.test.ts), placement (__tests__/), import style (fc default import from fast-check, vitest named imports), fc.constantFrom over domain unions, and devDependency placement are all correct.
tests6
decideLeaveMembership lacks idempotency check — asymmetry with decideRemoveMember is untested
domains/core/src/membership/__tests__/membership.decisions.property.test.ts:124
Test 5 verifies decideRemoveMember is idempotent for status='removed', but does not test decideLeaveMembership with the same state. Code inspection shows decideRemoveMember short-circuits on status='removed' (returns ok before owner check), while decideLeaveMembership has no such guard — so calling leaveMembership on an already-removed owner with activeOwnersCount=1 returns LastOwner, not ok. This asymmetry is either a silent bug or intentional design, but either way the property tests don't expose or document it. A property test should cover this case for both functions.
Null-membership NotFound property only covers decideRemoveMember — not decideLeaveMembership or decideUpdateRole
domains/core/src/membership/__tests__/membership.decisions.property.test.ts:133
Test 6 asserts that decideRemoveMember with membership=null always returns MembershipNotFound. All three decision functions have a null guard (membership.decisions.ts lines 85–86, 112–114, 134–136), but only one is covered by the property. A regression in null handling in decideLeaveMembership or decideUpdateRole would pass the current test suite silently.
site.decisions.property.test.ts Test 5 is a hardcoded example, not a property test
domains/core/src/site/__tests__/site.decisions.property.test.ts:56
'active and inactive are mutually reachable' uses two bare expect() assertions rather than fc.property() + generators. It's a valid correctness check but doesn't belong in a .property.test.ts file — move to site.decisions.test.ts (example tests) or convert to fc.property(statusArb, ...) to stay consistent with the file's purpose.
No property test for decideCreateMembership → UserAlreadyMember invariant
domains/core/src/membership/__tests__/membership.decisions.property.test.ts:47
decideCreateMembership has a simple but critical invariant: existingMembershipInOrg !== null always yields UserAlreadyMember. This is the access-control gate for membership creation and deserves a property test alongside the remove/leave/update invariants already covered.
Membership Tests 1–4 and 6 are well-formed invariant tests
domains/core/src/membership/__tests__/membership.decisions.property.test.ts:48
Tests correctly use fc.constantFrom over real MemberRole/SiteStatus unions, don't re-implement decision logic, use fc.pre() appropriately for version-mismatch filtering, and would fail under plausible regressions. The last-owner and version-conflict invariants are solidly covered.
Site state-machine property tests 1–4 are comprehensive and correct
domains/core/src/site/__tests__/site.decisions.property.test.ts:18
Reflexivity, totality, terminal-state, and no-escape-sequence properties correctly test the isValidStatusTransition invariants without re-implementing the state machine. Falsifiable: introducing a 'decommissioned'→'active' bug would fail test 3 and test 4.
improvement2
File-level JSDoc blocks are 7 lines; existing example tests use 4–5
domains/core/src/membership/__tests__/membership.decisions.property.test.ts:1
Both property-test files open with 7-line JSDoc blocks. Existing example tests in the same domain use 4–5 lines. The explanations are useful but could be condensed by cutting the repetitive narrative and keeping only the invariant name + testing.md reference.
File-level JSDoc blocks are 7 lines; existing example tests use 4–5
domains/core/src/site/__tests__/site.decisions.property.test.ts:1
Same as membership: 7-line block comment where 4–5 lines is the local norm. Minor.