Add root mutation detection and shared test helpers#471
Merged
Conversation
- Add mutation detection for aggregate root state and process root state within handler scopes, panicking when mutations are detected outside of sanctioned operations - Extract compare package (internal/compare) for deep equality checks using unsafe reflection - Add shared test helpers to xtesting: ExpectPanicMatching and ExpectLocation - Eliminate package-local test helpers in aggregate and process packages, replacing them with shared xtesting functions - Add missing mutation detection tests for process scope - Rename test functions and fixtures for clarity
There was a problem hiding this comment.
Pull request overview
Adds root mutation detection for aggregate and process handler scopes, backed by a shared comparison helper and reusable testing assertions.
Changes:
- Introduces
internal/comparewith unsafe reflection support for comparing root/message state, including unexported fields and function values. - Adds aggregate/process shadow-root checks to detect direct mutation and non-deterministic sanctioned mutations.
- Refactors tests to use shared
xtestingpanic/location helpers and adds mutation-detection coverage.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test.go |
Uses shared comparison helper for fact value matching. |
compare.go |
Delegates default message comparison to shared comparison helper. |
fact/aggregate.go |
Adds snapshot offset metadata to aggregate loaded facts. |
internal/compare/doc.go |
Documents the new comparison package. |
internal/compare/compare.go |
Implements shared deep equality logic. |
internal/compare/compare_test.go |
Adds comparison behavior tests. |
internal/compare/internal/unsafereflect/value.go |
Adds unsafe reflection utilities for unexported fields. |
internal/compare/internal/unsafereflect/value_test.go |
Tests unsafe reflection helper behavior. |
internal/compare/internal/unsafereflect/LICENSE.credits |
Adds attribution for unsafe reflection approach. |
internal/x/xtesting/expect.go |
Adds shared panic and location assertion helpers. |
engine/internal/aggregate/controller.go |
Builds shadow aggregate roots and validates snapshot/event divergence. |
engine/internal/aggregate/controller_test.go |
Updates aggregate controller tests for shared helpers and snapshot behavior. |
engine/internal/aggregate/scope.go |
Adds aggregate mutation detection and non-deterministic ApplyEvent detection. |
engine/internal/aggregate/scope_test.go |
Adds aggregate mutation detection tests. |
engine/internal/process/controller.go |
Builds process shadow roots and checks for end-of-handler direct mutation. |
engine/internal/process/controller_test.go |
Updates process controller tests to shared helpers. |
engine/internal/process/scope.go |
Adds process mutation detection and non-deterministic Mutate detection. |
engine/internal/process/scope_test.go |
Adds process mutation detection tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Handle invalid (nil) reflect.Value in deepEqual to prevent panic when Equal is called with untyped nil arguments - Add reflect.Array to recursive traversal so arrays containing functions use definition-site comparison - Update DefaultMessageComparator doc comment to describe the actual compare.Equal semantics
- Only store aggregate instance in c.instances after events are recorded, preventing commands that record no events from leaving behind empty instances that change subsequent AggregateInstanceNotFound semantics - Add ExpectSet to xtesting for unordered slice comparison - Replace findFact+Expect with ExpectContains in aggregate tests - Inline expectNoError, expectFacts, and expectEnvelopeSet in process tests, removing package-local helper functions
- Fix 'ensure or' -> 'ensure our' in checkFlagValues comment - Fix 'Dave Collin's' -> 'Dave Collins'' in LICENSE.credits
jmalloc
force-pushed
the
root-mutation-checks
branch
from
c160af1 to
fce3a86
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add mutation detection for aggregate and process root state within handler scopes. The scope now panics with a descriptive
UnexpectedBehaviorerror when mutations are detected outside of sanctioned operations (e.g., mutating root state between handler method calls).Changes
internal/comparepackage: Deep equality checks using unsafe reflection, extracted for reuse across scope implementationsxtesting):ExpectPanicMatching(generic panic recovery with type assertion and match callback) andExpectLocation(source location assertions)xtestingfunctions