Repository navigation
feat: load Haskell entities from snapshots - #36
Conversation
WalkthroughChangesThe pull request adds snapshot persistence to the Rust engine, exposes snapshot operations through the version-4 C ABI and Haskell bindings, and updates Haskell entity loading to resume from snapshots. It also adds SQLx metadata and adjusts development hooks. Snapshot support
Development hook configuration
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3000d3f to
836022f
Compare
11e048e to
eb04fd1
Compare
836022f to
0fd170e
Compare
eb04fd1 to
83941ab
Compare
0fd170e to
7c03431
Compare
8ac8b94 to
5ab9e26
Compare
3497c35 to
836f740
Compare
5ab9e26 to
1159168
Compare
836f740 to
397b89c
Compare
1159168 to
3be4428
Compare
397b89c to
b184079
Compare
3be4428 to
3de31a7
Compare
b184079 to
b1428a2
Compare
3de31a7 to
06c14c3
Compare
b1428a2 to
bd1e561
Compare
06c14c3 to
a3da8a6
Compare
bd1e561 to
20bd850
Compare
a3da8a6 to
0f21746
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/haskell/src/EventSorcery/Snapshot.hs`:
- Around line 85-93: Remove the ineffective linear SnapshotWrite wrapper:
replace SnapshotWrite and snapshotWrite with a plain storeSnapshot signature
accepting StreamIdentity, Word64, and ByteString and returning IO (Either
EngineError SnapshotVersion). Remove the SnapshotWrite GADT, Ur import, and
related pattern-matching indirection, following the ordinary argument style used
by EventSorcery.Stream.commit.
- Around line 81-82: Change StoredSnapshot to a named-field record, introducing
or reusing a StreamSequence newtype for the sequence alongside SnapshotVersion,
and update its construction and access sites to use record fields. In
EventSorcery.Store.resumeSnapshot, replace positional binding with field-name
access while preserving the existing snapshot behavior.
- Around line 157-198: Add dedicated Haskell tests for the snapshot wire codec,
exercising encodeIdentity and encodeWrite through a test-facing boundary or
codec suite. Include independent literal inputs that verify decodeStoredSnapshot
rejects trailing bytes, unsupported format versions, and incorrect CBOR list
lengths, while preserving valid snapshot and null-snapshot decoding behavior.
In `@bindings/haskell/src/EventSorcery/Store.hs`:
- Around line 105-123: Update loadCurrent to return both the current entity and
the sequence at which the existing snapshot was read, then adjust snapshotEntity
to compare that loaded snapshot sequence with the current sequence before
calling Snapshot.storeSnapshot. Only write a snapshot when the current sequence
has advanced past the loaded snapshot sequence; preserve the existing result
handling and allow concurrent commits between loadCurrent and storeSnapshot.
- Around line 170-176: Update the snapshot persistence and resume flow centered
on loadCurrent and resumeSnapshot to include EventSourced.schemaVersion in
StoredSnapshot and the EventSorcery.Snapshot/C ABI payload. Compare the stored
schema version with Aggregate.schemaVersion (Proxy `@entity`) before decoding;
treat mismatches as stale checkpoints and replay the stream, while preserving
StoreSnapshotDecodeFailed for decode failures at a matching version. Document
the wire-format and recovery decision in an ADR under adrs/.
In `@bindings/haskell/test/StoreSpec.hs`:
- Around line 92-129: Replace the single conjunction-driven conditional around
the test expectations with separate `shouldBe` assertions for each invariant,
including `initially`, `emptySnapshot`, `openedAccount`, `deposited`,
`snapshotted`, `afterSnapshotDeposit`, `reloaded`, `invalidEvent`,
`afterInvalidEvent`, `dispatched`, `jobs`, `rejected`, `afterRejection`,
`corrupted`, `corruptedLoad`, `discarded`, and `recovered`, preserving each
existing expected value.
- Around line 215-218: Update the Account snapshot codec in encodeSnapshot and
decodeSnapshot to use an encoding width that represents the full balance domain
without truncation, keeping malformed payloads such as the existing corrupted
snapshot rejected by decodeSnapshot. Ensure round-tripping balances above 255
preserves the original Account value.
In `@crates/event-sorcery-ffi/src/lib.rs`:
- Around line 545-547: The snapshot path currently serializes payload bytes as a
JSON number array; update the snapshot aggregate construction near sequence
conversion to reuse the existing opaque payload envelope via
encode_opaque_payload. Change snapshot_payload_bytes to decode that
representation with the matching decoder, preserving payload limits and the
established provenance protection used by the event path.
In `@crates/event-sorcery/src/engine.rs`:
- Around line 1063-1074: Update store_snapshot_in_transaction to use the
existing committed_stream_version oracle instead of loading events through
load_events_on and reading events.last().sequence. Preserve the empty-stream
behavior when no committed version exists, while allowing compacted streams to
validate snapshot.last_sequence against the version reported by
committed_stream_version.
- Around line 1982-2055: Extend the snapshot tests in
crates/event-sorcery/src/engine.rs:1982-2055 around
snapshot_operations_use_the_existing_snapshot_table to exercise
SnapshotBeyondCurrentVersion and EmptySnapshotUpdate through store_snapshot,
asserting each rejected write leaves the persisted snapshot unchanged. Also
extend the FFI snapshot tests in crates/event-sorcery-ffi/src/lib.rs:1455-1467
to write a JSON-object snapshot through the leased engine and assert
es_snapshot_load returns ES_ERR_STORAGE for
SnapshotPayloadFault::NotOpaqueBytes, plus add coverage for ByteOutOfRange.
- Around line 639-668: Update store_snapshot to acquire a transaction through
the pool API using the existing immediate-transaction pattern, rather than
acquiring a PoolConnection and issuing raw BEGIN IMMEDIATE. Pass the transaction
to store_snapshot_in_transaction, commit it on success, and rely on
sqlx::Transaction’s drop rollback behavior on failure or cancellation; remove
the manual COMMIT/ROLLBACK handling and any now-unused query metadata.
In `@flake.nix`:
- Around line 165-168: Update the hook migration logic around the
generated-by-prek and GITBUTLER_MANAGED_HOOK_V1 checks to resolve the active
hooks directory via git rev-parse --git-path hooks, then use hooks_dir for every
marker lookup and mv operation instead of .git/hooks. Add coverage for standard
repositories, linked worktrees, and repositories with configured core.hooksPath.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a204bb7-a2c3-47be-b08a-ca2105f58dce
📒 Files selected for processing (15)
.sqlx/query-587fa628a9fe4642ac0e22825a97e81d388af674f7251a9fb699f96901dcb710.json.sqlx/query-79663c1d3b43ceaf9ee728e501c64b57ae2e4d6cb36ff5c65eddcda1de27342f.json.sqlx/query-930a7770399087898ae6ac96ce5375048117486e06b21da4523d2c3c75113c32.json.sqlx/query-d8d7bee8e77c496d9510ccb0a632d70da8f647b4ce32ca7526a5e18d709d1d65.jsonbindings/haskell/event-sorcery.cabalbindings/haskell/src/EventSorcery/Engine/Internal.hsbindings/haskell/src/EventSorcery/Engine/Internal/FFI.hsbindings/haskell/src/EventSorcery/Snapshot.hsbindings/haskell/src/EventSorcery/Store.hsbindings/haskell/test/StoreSpec.hscrates/event-sorcery-ffi/src/lib.rscrates/event-sorcery/src/engine.rscrates/event-sorcery/src/lib.rsflake.nixgit-hooks.nix
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: fmt
- GitHub Check: test
- GitHub Check: examples
- GitHub Check: haskell
- GitHub Check: check
- GitHub Check: clippy
- GitHub Check: hooks
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Before work, read SPEC.md and docs/domain.md; read relevant supplemental documentation before implementation.
New features must be documented in SPEC.md before implementation and must follow the hierarchy SPEC.md -> issue -> plan -> tests -> implementation.
Fix all known problems immediately, complete all tasks, and do not allow warnings or errors to pass through.
Keep a granular task list and clear completed tasks from the active list.
All new or modified logic must have corresponding test coverage.
Understand relevant documentation and source code before implementation, keep diffs small, and review the approach critically.
When changing direction or making an important undocumented architectural decision, obtain confirmation; record significant decisions as ADRs under adrs/.
Before handover, review the diff, revert unjustified changes, and check for scope creep.
Each aggregate in a consuming application must use exactly one SqliteCqrs instance constructed at startup; per-request construction is forbidden.
Never read secret or credential files such as .env*, credentials.json, *.key, *.pem, *.p12, *.pfx, or sensitive database files without explicit permission.
Never bypass, disable, suppress, or obscure quality-control mechanisms without explicit permission; fix lint and test issues at their root.
Use cargo check, cargo nextest, and cargo clippy for verification; never use cargo build unless build artifacts are required.
Files:
bindings/haskell/event-sorcery.cabalcrates/event-sorcery/src/lib.rsbindings/haskell/src/EventSorcery/Engine/Internal/FFI.hsbindings/haskell/src/EventSorcery/Engine/Internal.hsbindings/haskell/src/EventSorcery/Snapshot.hsbindings/haskell/src/EventSorcery/Store.hsbindings/haskell/test/StoreSpec.hsgit-hooks.nixflake.nixcrates/event-sorcery/src/engine.rscrates/event-sorcery-ffi/src/lib.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.rs: Organize code by business feature rather than technical layer; avoid catch-all modules such as types.rs, error.rs, models.rs, utils.rs, helpers.rs, and services.rs.
Never write directly to the events table; emit events through CqrsFramework::execute() or execute_with_metadata().
Use cqrs-es Services for side effects in handle() and follow the {Action}er -> {Domain}Service -> {Domain}Manager naming pattern.
Place command-execution logging in aggregate handle() methods rather than callers.
Model invalid states with enums, ADTs, newtypes, and typestate rather than relying on runtime validation.
Use domain newtypes at APIs and convert to SDK primitives inside the callee, except at cross-crate boundaries where conversion at the call site is necessary.
Keep visibility as restrictive as possible: private over pub(crate) over pub.
Use a three-group import order: external crates, workspace crates, then crate-internal imports; do not use function-level imports except enum variants.
Do not use unwrap() or expect() in production Rust code; they are permitted in #[cfg(test)] code.
Never create error variants containing opaque String values; prefer #[from], ?, #[source], and preserve error chains.
Log a warning or error before silent early returns such as let-else failures.
Never silently mask numeric failures with caps, fallback defaults, precision truncation, unwrap_or(), or unwrap_or_default(); use explicit checked conversions and errors.
Prefer functional patterns, pattern matching, combinators, type-driven design, and iterators over imperative loops unless complexity increases.
Use ASCII in identifiers, comments, log messages, and configuration keys; Unicode is preferred only in user-facing rendered output.
Do not use single-letter variables, arguments, closure parameters, or generic type parameters except an unambiguous lone type parameter or short unambiguous closure.
Every module must have a //! docstring and should order public API, private implementati...
Files:
crates/event-sorcery/src/lib.rscrates/event-sorcery/src/engine.rscrates/event-sorcery-ffi/src/lib.rs
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
git-hooks.nixflake.nix
🧠 Learnings (6)
📚 Learning: 2026-07-16T15:10:47.551Z
Learnt from: 0xgleb
Repo: dataclique/event-sorcery PR: 24
File: crates/event-sorcery/src/lib.rs:88-88
Timestamp: 2026-07-16T15:10:47.551Z
Learning: In `crates/event-sorcery/src/lib.rs`, the crate-root `engine` module should remain private (`mod engine;`). Rust permits descendant modules, including `crates/event-sorcery/src/job_sqlite.rs` and `crates/event-sorcery/src/sqlite_event_repository.rs`, to access it through `crate::engine`; it does not need `pub(crate)` visibility for that internal access.
Applied to files:
bindings/haskell/event-sorcery.cabalcrates/event-sorcery/src/lib.rscrates/event-sorcery/src/engine.rscrates/event-sorcery-ffi/src/lib.rs
📚 Learning: 2026-07-16T21:10:49.512Z
Learnt from: 0xgleb
Repo: dataclique/event-sorcery PR: 0
File: :0-0
Timestamp: 2026-07-16T21:10:49.512Z
Learning: In `crates/event-sorcery/src/job_backend.rs`, `JobClaimHandle` is deliberately non-serializable. It contains private fencing identity, so foreign-language bindings must retain it in trusted process memory and expose only a binding-owned opaque token; accepting caller-provided serialized handles could permit forged claim state.
Applied to files:
crates/event-sorcery/src/lib.rscrates/event-sorcery/src/engine.rscrates/event-sorcery-ffi/src/lib.rs
📚 Learning: 2026-07-16T09:05:57.710Z
Learnt from: CR
Repo: dataclique/event-sorcery PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-16T09:05:57.710Z
Learning: Applies to crates/**/*.rs : Use in-memory SQLite pools via sqlite_es::testing::create_test_pool() for database test isolation.
Applied to files:
crates/event-sorcery/src/engine.rscrates/event-sorcery-ffi/src/lib.rs
📚 Learning: 2026-07-16T17:19:57.719Z
Learnt from: 0xgleb
Repo: dataclique/event-sorcery PR: 26
File: crates/event-sorcery-ffi/build.rs:0-0
Timestamp: 2026-07-16T17:19:57.719Z
Learning: For the `event-sorcery-ffi` C ABI, `event_sorcery.h` is a generated build artifact defined by `SPEC.md` and must not be hand-maintained or written into the checked-in source tree during builds. `crates/event-sorcery-ffi/build.rs` should generate it under `OUT_DIR`; publishing it for consumers is deferred to a separately defined release/package destination.
Applied to files:
crates/event-sorcery-ffi/src/lib.rs
📚 Learning: 2026-07-16T09:05:57.710Z
Learnt from: CR
Repo: dataclique/event-sorcery PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-16T09:05:57.710Z
Learning: Applies to crates/**/*.rs : Never write directly to the events table; emit events through CqrsFramework::execute() or execute_with_metadata().
Applied to files:
crates/event-sorcery-ffi/src/lib.rs
📚 Learning: 2026-07-16T09:05:57.710Z
Learnt from: CR
Repo: dataclique/event-sorcery PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-16T09:05:57.710Z
Learning: Applies to crates/**/*.rs : Serialization tests must compare output with independent literals such as json!(), not re-serialized domain values.
Applied to files:
crates/event-sorcery-ffi/src/lib.rs
🔇 Additional comments (16)
git-hooks.nix (1)
8-16: 🩺 Stability & AvailabilityThe pinned
git-hooks.nixrustfmt hook already setspass_filenames = false; no change is required.> Likely an incorrect or invalid review comment.crates/event-sorcery/src/engine.rs (1)
158-172: LGTM!Also applies to: 197-200, 396-397, 1236-1238
.sqlx/query-79663c1d3b43ceaf9ee728e501c64b57ae2e4d6cb36ff5c65eddcda1de27342f.json (1)
1-12: LGTM!.sqlx/query-d8d7bee8e77c496d9510ccb0a632d70da8f647b4ce32ca7526a5e18d709d1d65.json (1)
1-12: LGTM!crates/event-sorcery/src/lib.rs (1)
130-130: LGTM!crates/event-sorcery-ffi/src/lib.rs (1)
19-19: LGTM!Also applies to: 30-34, 78-80, 471-512, 573-594, 1402-1403, 1732-1733, 2375-2376, 4378-4460
bindings/haskell/test/StoreSpec.hs (1)
31-39: LGTM!Also applies to: 68-90
.sqlx/query-587fa628a9fe4642ac0e22825a97e81d388af674f7251a9fb699f96901dcb710.json (1)
1-12: LGTM!.sqlx/query-930a7770399087898ae6ac96ce5375048117486e06b21da4523d2c3c75113c32.json (1)
1-12: LGTM!bindings/haskell/event-sorcery.cabal (1)
58-58: LGTM!bindings/haskell/src/EventSorcery/Engine/Internal.hs (1)
143-147: LGTM!bindings/haskell/src/EventSorcery/Snapshot.hs (2)
96-104: LGTM!Also applies to: 122-130
133-154: LGTM!bindings/haskell/src/EventSorcery/Store.hs (2)
7-7: LGTM!Also applies to: 30-43, 67-67, 313-317
205-205: 🎯 Functional CorrectnessNo change needed:
loadStream ... (Just sequence)usessequence > ?3, so it excludes the snapshot event and matchesresume'snextPosition.> Likely an incorrect or invalid review comment.bindings/haskell/src/EventSorcery/Engine/Internal/FFI.hs (1)
20-22: 🗄️ Data Integrity & IntegrationNo header change is required.
event_sorcery.his generated from the exported Rust ABI and packaged under$out/include; the three snapshot functions are declared for the Haskellcapiimports.> Likely an incorrect or invalid review comment.
| data StoredSnapshot = StoredSnapshot Word64 SnapshotVersion ByteString | ||
| deriving stock (Eq, Show) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use named fields for StoredSnapshot to remove the positional swap hazard.
StoredSnapshot Word64 SnapshotVersion ByteString carries a sequence and a version next to each other. The sequence stays a bare Word64 while the version is a newtype. A caller that swaps the two numeric concepts gets no type error once both are unwrapped.
EventSorcery.Stream.ProposedEvent uses record syntax, and the package enables NoFieldSelectors and OverloadedRecordDot. Follow that convention here. Consider a StreamSequence newtype as well, so the sequence and the version cannot be confused at any call site.
♻️ Proposed record form
-data StoredSnapshot = StoredSnapshot Word64 SnapshotVersion ByteString
- deriving stock (Eq, Show)
+data StoredSnapshot = StoredSnapshot
+ { sequence :: Word64
+ , version :: SnapshotVersion
+ , payload :: ByteString
+ }
+ deriving stock (Eq, Show)EventSorcery.Store.resumeSnapshot at line 200 then binds by field name instead of by position.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data StoredSnapshot = StoredSnapshot Word64 SnapshotVersion ByteString | |
| deriving stock (Eq, Show) | |
| data StoredSnapshot = StoredSnapshot | |
| { sequence :: Word64 | |
| , version :: SnapshotVersion | |
| , payload :: ByteString | |
| } | |
| deriving stock (Eq, Show) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/haskell/src/EventSorcery/Snapshot.hs` around lines 81 - 82, Change
StoredSnapshot to a named-field record, introducing or reusing a StreamSequence
newtype for the sequence alongside SnapshotVersion, and update its construction
and access sites to use record fields. In EventSorcery.Store.resumeSnapshot,
replace positional binding with field-name access while preserving the existing
snapshot behavior.
Source: Path instructions
| data SnapshotWrite where | ||
| SnapshotWrite | ||
| :: Ur (StreamIdentity, Word64, ByteString) | ||
| %1 -> SnapshotWrite | ||
|
|
||
|
|
||
| snapshotWrite :: StreamIdentity -> Word64 -> ByteString -> SnapshotWrite | ||
| snapshotWrite identity sequence payload = | ||
| SnapshotWrite (Ur (identity, sequence, payload)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
The linear SnapshotWrite wrapper over Ur gives no linearity guarantee.
SnapshotWrite takes its payload linearly, but the payload is Ur (StreamIdentity, Word64, ByteString). Ur marks the contents as unrestricted, so pattern matching at line 111 releases them into an unrestricted context immediately. The only invariant enforced is that the SnapshotWrite token is consumed once, and snapshotWrite constructs that token freely from unrestricted arguments. No resource is protected.
Two consistent options:
- Drop the linear machinery. Give
storeSnapshota plainStreamIdentity -> Word64 -> ByteString -> IO (Either EngineError SnapshotVersion)signature. This removes the GADT, theUrimport, and the indirection. - Keep linearity, but make it mean something. Hold the payload linearly rather than through
Ur, so the encoded bytes cannot be duplicated or dropped before the FFI call.
Option 1 matches the surrounding modules. EventSorcery.Stream.commit takes ordinary arguments for the same kind of engine call.
If the linear signature is a deliberate architectural choice for the binding, record the rationale as an ADR. Otherwise the type surface suggests a guarantee that the implementation does not provide.
Also applies to: 107-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/haskell/src/EventSorcery/Snapshot.hs` around lines 85 - 93, Remove
the ineffective linear SnapshotWrite wrapper: replace SnapshotWrite and
snapshotWrite with a plain storeSnapshot signature accepting StreamIdentity,
Word64, and ByteString and returning IO (Either EngineError SnapshotVersion).
Remove the SnapshotWrite GADT, Ur import, and related pattern-matching
indirection, following the ordinary argument style used by
EventSorcery.Stream.commit.
Source: Path instructions
| decodeStoredSnapshot :: ByteString -> Either String (Maybe StoredSnapshot) | ||
| decodeStoredSnapshot bytes = | ||
| case deserialiseFromBytes | ||
| decodeStoredSnapshotWire | ||
| (LazyByteString.fromStrict bytes) of | ||
| Left failure -> Left (show failure) | ||
| Right (remaining, snapshot) | ||
| | LazyByteString.null remaining -> Right snapshot | ||
| | otherwise -> Left "trailing bytes after stored snapshot" | ||
|
|
||
|
|
||
| decodeStoredSnapshotWire :: Decoder s (Maybe StoredSnapshot) | ||
| decodeStoredSnapshotWire = do | ||
| expectListLength 2 | ||
| expectFormatVersion | ||
| token <- peekTokenType | ||
|
|
||
| case token of | ||
| TypeNull -> Nothing <$ decodeNull | ||
| _ -> do | ||
| expectListLength 3 | ||
| sequence <- decodeWord64 | ||
| version <- SnapshotVersion <$> decodeWord64 | ||
| Just . StoredSnapshot sequence version <$> decodeBytes | ||
|
|
||
|
|
||
| expectFormatVersion :: Decoder s () | ||
| expectFormatVersion = do | ||
| version <- decodeWord | ||
|
|
||
| if version == 1 | ||
| then pure () | ||
| else fail "unsupported snapshot format version" | ||
|
|
||
|
|
||
| expectListLength :: Int -> Decoder s () | ||
| expectListLength expected = do | ||
| actual <- decodeListLen | ||
|
|
||
| if actual == expected | ||
| then pure () | ||
| else fail "unexpected CBOR list length" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing snapshot codec coverage in the Haskell test suites.
fd -t f -e hs . bindings/haskell/test --exec rg -n 'Snapshot|snapshotWrite|decodeStoredSnapshot|StoredSnapshot' {}
# Show which modules WireSpec currently covers.
fd -t f 'WireSpec.hs' --exec rg -n '^import|testCase' {}Repository: dataclique/event-sorcery
Length of output: 3334
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- documentation files ---'
fd -t f 'SPEC.md' 'domain.md' . | sort
for f in $(fd -t f 'SPEC.md' 'domain.md' . | sort); do
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
done
printf '%s\n' '--- Snapshot module ---'
wc -l bindings/haskell/src/EventSorcery/Snapshot.hs
sed -n '1,220p' bindings/haskell/src/EventSorcery/Snapshot.hs
printf '%s\n' '--- WireSpec snapshot-related context ---'
WIRE=$(fd -t f 'WireSpec.hs' . | head -n 1)
printf 'file=%s\n' "$WIRE"
sed -n '1,240p' "$WIRE"
printf '%s\n' '--- all codec test references ---'
rg -n -C 3 'encodeIdentity|encodeWrite|decodeStoredSnapshot|stored snapshot|snapshot' bindings/haskell/test bindings/haskell/srcRepository: dataclique/event-sorcery
Length of output: 39821
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- domain documentation ---'
sed -n '1,220p' docs/domain.md
printf '%s\n' '--- snapshot specification references ---'
rg -n -C 4 'snapshot|Serialization tests|literal|wire-level|CBOR' SPEC.md docs bindings/haskell
printf '%s\n' '--- Haskell test configuration ---'
fd -t f . bindings/haskell | rg '(cabal|package|Makefile|flake|nix)$' | sort
for f in $(fd -t f . bindings/haskell | rg '(cabal|package|Makefile|flake|nix)$' | sort); do
printf '\n--- %s ---\n' "$f"
rg -n -C 3 'WireSpec|StoreSpec|test-suite|exposed-modules|other-modules' "$f" || true
done
printf '%s\n' '--- Rust snapshot wire tests ---'
rg -n -C 8 'StoredSnapshotWire|snapshot.*(None|Some)|snapshot_load|snapshot_store|snapshot wire' crates bindingsRepository: dataclique/event-sorcery
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Haskell package and test-suite declarations ---'
for f in $(fd -t f . bindings/haskell | rg '\.(cabal|yaml|yml|mk|nix)$|Makefile$' | sort); do
rg -n -C 5 'WireSpec|StoreSpec|test-suite|hs-source-dirs|other-modules|EventSorcery.Snapshot' "$f" || true
done
printf '%s\n' '--- Rust snapshot test matches only ---'
rg -n 'StoredSnapshotWire|snapshot_load|snapshot_store|snapshot.*None|snapshot.*Some' crates bindings --glob '*.rs' --glob '*.hs' | head -n 120
printf '%s\n' '--- Rust snapshot test region ---'
MATCH=$(rg -l 'StoredSnapshotWire' crates bindings --glob '*.rs' | head -n 1 || true)
if [ -n "$MATCH" ]; then
printf 'file=%s\n' "$MATCH"
LINE=$(rg -n 'StoredSnapshotWire' "$MATCH" | head -n 1 | cut -d: -f1)
START=$((LINE-30)); [ "$START" -lt 1 ] && START=1
END=$((LINE+100))
sed -n "${START},${END}p" "$MATCH"
fi
printf '%s\n' '--- Snapshot exports and test imports ---'
rg -n -C 2 'import EventSorcery.Snapshot|module EventSorcery.Snapshot|decodeStoredSnapshot|encodeIdentity|encodeWrite' bindings/haskell --glob '*.hs'Repository: dataclique/event-sorcery
Length of output: 15650
Add independent-literal tests for the snapshot wire codec.
The Haskell tests cover snapshots only through the store. They do not cover encodeIdentity, encodeWrite, or the decoder rejection paths for trailing bytes, unsupported format versions, and incorrect list lengths. Add a dedicated codec test suite or test the helpers through an explicit test-facing boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/haskell/src/EventSorcery/Snapshot.hs` around lines 157 - 198, Add
dedicated Haskell tests for the snapshot wire codec, exercising encodeIdentity
and encodeWrite through a test-facing boundary or codec suite. Include
independent literal inputs that verify decodeStoredSnapshot rejects trailing
bytes, unsupported format versions, and incorrect CBOR list lengths, while
preserving valid snapshot and null-snapshot decoding behavior.
Source: Coding guidelines
| snapshotEntity store@(Store engine _) key = do | ||
| loaded <- loadCurrent store key | ||
|
|
||
| case loaded of | ||
| Left failure -> pure (Left failure) | ||
| Right (Nothing, _) -> pure (Right Nothing) | ||
| Right (Just entity, sequence) -> do | ||
| stored <- | ||
| Snapshot.storeSnapshot | ||
| engine | ||
| ( Snapshot.snapshotWrite | ||
| (streamKeyIdentity key) | ||
| sequence | ||
| (Aggregate.encodeSnapshot entity) | ||
| ) | ||
|
|
||
| pure case stored of | ||
| Left failure -> Left (StoreEngineFailed failure) | ||
| Right _ -> Right (Just entity) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
snapshotEntity writes a redundant snapshot when the stream has not advanced.
loadCurrent returns the sequence reached after resume. If a snapshot already exists at sequence N and no event has been committed since, resumeSnapshot loads an empty event list and latestSequenceAfter N [] = N. snapshotEntity then stores a snapshot at the same sequence N.
The engine accepts this write. store_snapshot_in_transaction rejects only when stored.current_sequence > snapshot.last_sequence, and N > N is false. It writes a new row and increments snapshot_version. Repeated snapshotEntity calls with no intervening events increment the version without bound.
Each such call also costs a full transactional write plus, inside the engine, a load of the entire event stream to determine the current sequence. A periodic snapshotting job over idle streams pays that cost for no benefit.
Skip the write when the sequence has not advanced past the loaded snapshot. That requires loadCurrent to report the sequence the snapshot was read at, so snapshotEntity can compare.
Note also that loadCurrent and storeSnapshot are two separate engine calls. A concurrent commit between them causes a snapshot at an older sequence to be written. That is safe, because resume replays the events after the recorded sequence. No fix is needed for that case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/haskell/src/EventSorcery/Store.hs` around lines 105 - 123, Update
loadCurrent to return both the current entity and the sequence at which the
existing snapshot was read, then adjust snapshotEntity to compare that loaded
snapshot sequence with the current sequence before calling
Snapshot.storeSnapshot. Only write a snapshot when the current sequence has
advanced past the loaded snapshot sequence; preserve the existing result
handling and allow concurrent commits between loadCurrent and storeSnapshot.
| loadCurrent (Store engine _) key = do | ||
| loadedSnapshot <- Snapshot.loadSnapshot engine (streamKeyIdentity key) | ||
|
|
||
| case loadedSnapshot of | ||
| Left failure -> pure (Left (StoreEngineFailed failure)) | ||
| Right Nothing -> replayFullStream engine key | ||
| Right (Just snapshot) -> resumeSnapshot engine key snapshot |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A snapshot decode failure permanently blocks every read of the stream, and there is no schema-version guard.
loadCurrent routes any stored snapshot to resumeSnapshot. If Aggregate.decodeSnapshot rejects the payload, the call returns StoreSnapshotDecodeFailed. loadEntity, executeCommand, and snapshotEntity all go through loadCurrent, so all three fail for that stream. The event stream is still intact, and a full replay would succeed.
The realistic trigger is not corruption. It is schema evolution. When an entity changes its encodeSnapshot/decodeSnapshot representation, every previously stored snapshot becomes undecodable at once. Recovery then requires an out-of-band discardSnapshot call for every affected stream.
EventSourced already declares schemaVersion, but StoredSnapshot carries only the sequence, the monotonic snapshot version, and the payload. Nothing records which aggregate schema produced the payload. A stale-schema snapshot is therefore indistinguishable from a corrupt one.
Record the aggregate schemaVersion with the snapshot payload. resumeSnapshot can then compare it against Aggregate.schemaVersion (Proxy @entity) before decoding, and treat a mismatch as a stale checkpoint rather than a failure. A decode failure at a matching schema version stays a hard error, which keeps genuine corruption loud.
This crosses the wire format, so it also affects EventSorcery.Snapshot and the C ABI payload. Confirm the intended behavior before merge, and record the decision as an ADR under adrs/.
Also applies to: 194-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/haskell/src/EventSorcery/Store.hs` around lines 170 - 176, Update
the snapshot persistence and resume flow centered on loadCurrent and
resumeSnapshot to include EventSourced.schemaVersion in StoredSnapshot and the
EventSorcery.Snapshot/C ABI payload. Compare the stored schema version with
Aggregate.schemaVersion (Proxy `@entity`) before decoding; treat mismatches as
stale checkpoints and replay the stream, while preserving
StoreSnapshotDecodeFailed for decode failures at a matching version. Document
the wire-format and recovery decision in an ADR under adrs/.
Source: Path instructions
| require_payload_limit(payload.len())?; | ||
| let sequence = usize::try_from(sequence).map_err(AbiError::InputInteger)?; | ||
| let aggregate = serde_json::Value::Array(payload.into_iter().map(Into::into).collect()); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Reuse the opaque payload envelope instead of a JSON number array.
Line 547 stores an N-byte snapshot as a serde_json::Value::Array of N JSON numbers. Three consequences follow:
- Storage cost grows roughly four times. A byte such as
255occupies four characters in the stored JSON text. - Every load rebuilds the
Vec<u8>element by element throughsnapshot_payload_bytes, so decoding is proportional to the element count rather than a byte copy. require_payload_limit(payload.len())at Line 545 bounds the request bytes, not the stored bytes. The persisted row can exceed that bound by a large factor.
The event path already solves this problem. encode_opaque_payload and decode_opaque_event in crates/event-sorcery/src/engine.rs wrap binding-owned bytes in the $event-sorcery-engine-payload envelope, and native_metadata_cannot_claim_the_opaque_payload_envelope protects the provenance. The snapshot path introduces a second, incompatible representation for the same concern.
Apply the existing envelope to snapshot payloads, and derive snapshot_payload_bytes from the matching decoder. That keeps one opaque-bytes contract across events and snapshots.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/event-sorcery-ffi/src/lib.rs` around lines 545 - 547, The snapshot
path currently serializes payload bytes as a JSON number array; update the
snapshot aggregate construction near sequence conversion to reuse the existing
opaque payload envelope via encode_opaque_payload. Change snapshot_payload_bytes
to decode that representation with the matching decoder, preserving payload
limits and the established provenance protection used by the event path.
| let mut connection = self.pool.acquire().await?; | ||
| load_snapshot_on(&mut connection, stream).await | ||
| } | ||
|
|
||
| pub async fn store_snapshot( | ||
| &self, | ||
| stream: &StreamIdentity, | ||
| snapshot: SnapshotWrite, | ||
| ) -> Result<usize, EngineError> { | ||
| let mut connection = self.pool.acquire().await?; | ||
| sqlx::query!("BEGIN IMMEDIATE") | ||
| .execute(&mut *connection) | ||
| .await?; | ||
| let result = store_snapshot_in_transaction(&mut connection, stream, snapshot).await; | ||
| let commit = result.is_ok(); | ||
| let close = if commit { | ||
| sqlx::query!("COMMIT").execute(&mut *connection).await | ||
| } else { | ||
| sqlx::query!("ROLLBACK").execute(&mut *connection).await | ||
| }; | ||
|
|
||
| match close { | ||
| Err(error) if commit => Err(EngineError::Sql(error)), | ||
| Err(error) => { | ||
| tracing::warn!(target: "cqrs", ?error, "snapshot rollback failed"); | ||
| result | ||
| } | ||
| Ok(_) => result, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Use the pool transaction API instead of raw BEGIN IMMEDIATE on a pooled connection.
store_snapshot runs BEGIN IMMEDIATE directly on a PoolConnection. If the future is cancelled between the BEGIN IMMEDIATE and the COMMIT/ROLLBACK, the connection returns to the pool with an open write transaction. This crate already treats that as a known hazard: commit_serialized uses self.pool.begin_with("BEGIN IMMEDIATE") at Line 829, and claim_job uses the immediate_transaction helper. The test cancelled_immediate_transaction_does_not_poison_the_pooled_connection exists for exactly this failure mode.
sqlx::Transaction rolls back on drop, so the pool-level API removes the cancellation hole and the manual commit/rollback branch.
♻️ Proposed fix using the existing transaction API
pub async fn store_snapshot(
&self,
stream: &StreamIdentity,
snapshot: SnapshotWrite,
) -> Result<usize, EngineError> {
- let mut connection = self.pool.acquire().await?;
- sqlx::query!("BEGIN IMMEDIATE")
- .execute(&mut *connection)
- .await?;
- let result = store_snapshot_in_transaction(&mut connection, stream, snapshot).await;
- let commit = result.is_ok();
- let close = if commit {
- sqlx::query!("COMMIT").execute(&mut *connection).await
- } else {
- sqlx::query!("ROLLBACK").execute(&mut *connection).await
- };
-
- match close {
- Err(error) if commit => Err(EngineError::Sql(error)),
- Err(error) => {
- tracing::warn!(target: "cqrs", ?error, "snapshot rollback failed");
- result
- }
- Ok(_) => result,
- }
+ let mut transaction = self.pool.begin_with("BEGIN IMMEDIATE").await?;
+ let version = store_snapshot_in_transaction(&mut transaction, stream, snapshot).await?;
+ transaction.commit().await?;
+
+ Ok(version)
}This also removes the now-unused .sqlx/query-79663c1d…json (COMMIT) metadata entry.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut connection = self.pool.acquire().await?; | |
| load_snapshot_on(&mut connection, stream).await | |
| } | |
| pub async fn store_snapshot( | |
| &self, | |
| stream: &StreamIdentity, | |
| snapshot: SnapshotWrite, | |
| ) -> Result<usize, EngineError> { | |
| let mut connection = self.pool.acquire().await?; | |
| sqlx::query!("BEGIN IMMEDIATE") | |
| .execute(&mut *connection) | |
| .await?; | |
| let result = store_snapshot_in_transaction(&mut connection, stream, snapshot).await; | |
| let commit = result.is_ok(); | |
| let close = if commit { | |
| sqlx::query!("COMMIT").execute(&mut *connection).await | |
| } else { | |
| sqlx::query!("ROLLBACK").execute(&mut *connection).await | |
| }; | |
| match close { | |
| Err(error) if commit => Err(EngineError::Sql(error)), | |
| Err(error) => { | |
| tracing::warn!(target: "cqrs", ?error, "snapshot rollback failed"); | |
| result | |
| } | |
| Ok(_) => result, | |
| } | |
| } | |
| let mut connection = self.pool.acquire().await?; | |
| load_snapshot_on(&mut connection, stream).await | |
| } | |
| pub async fn store_snapshot( | |
| &self, | |
| stream: &StreamIdentity, | |
| snapshot: SnapshotWrite, | |
| ) -> Result<usize, EngineError> { | |
| let mut transaction = self.pool.begin_with("BEGIN IMMEDIATE").await?; | |
| let version = store_snapshot_in_transaction(&mut transaction, stream, snapshot).await?; | |
| transaction.commit().await?; | |
| Ok(version) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/event-sorcery/src/engine.rs` around lines 639 - 668, Update
store_snapshot to acquire a transaction through the pool API using the existing
immediate-transaction pattern, rather than acquiring a PoolConnection and
issuing raw BEGIN IMMEDIATE. Pass the transaction to
store_snapshot_in_transaction, commit it on success, and rely on
sqlx::Transaction’s drop rollback behavior on failure or cancellation; remove
the manual COMMIT/ROLLBACK handling and any now-unused query metadata.
| let events = load_events_on(connection, stream, None).await?; | ||
| let current_sequence = events | ||
| .last() | ||
| .map(|event| event.sequence) | ||
| .ok_or(EngineError::EmptySnapshotUpdate)?; | ||
|
|
||
| if snapshot.last_sequence > current_sequence { | ||
| return Err(EngineError::SnapshotBeyondCurrentVersion { | ||
| proposed: snapshot.last_sequence, | ||
| current: current_sequence, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not load the whole event stream to read one sequence number.
store_snapshot_in_transaction calls load_events_on(connection, stream, None) and then uses only events.last().sequence. This materializes and deserializes every event in the stream on each snapshot write. Snapshots exist to avoid that cost, so the write path scales with the exact quantity the snapshot removes.
This also produces a wrong result after compaction. committed_stream_version (Line 915) already reports MAX(events.sequence, snapshots.last_sequence), and its doc states that a compacted stream reports the version its snapshot covers. load_events_on sees an empty events table after compaction, so store_snapshot returns EmptySnapshotUpdate for a stream that has a valid version.
♻️ Proposed fix using the existing version oracle
- let events = load_events_on(connection, stream, None).await?;
- let current_sequence = events
- .last()
- .map(|event| event.sequence)
- .ok_or(EngineError::EmptySnapshotUpdate)?;
-
- if snapshot.last_sequence > current_sequence {
+ let current_sequence = committed_stream_version(connection, stream).await?;
+ if current_sequence == 0 {
+ return Err(EngineError::EmptySnapshotUpdate);
+ }
+
+ if snapshot.last_sequence > current_sequence {
return Err(EngineError::SnapshotBeyondCurrentVersion {
proposed: snapshot.last_sequence,
current: current_sequence,
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let events = load_events_on(connection, stream, None).await?; | |
| let current_sequence = events | |
| .last() | |
| .map(|event| event.sequence) | |
| .ok_or(EngineError::EmptySnapshotUpdate)?; | |
| if snapshot.last_sequence > current_sequence { | |
| return Err(EngineError::SnapshotBeyondCurrentVersion { | |
| proposed: snapshot.last_sequence, | |
| current: current_sequence, | |
| }); | |
| } | |
| let current_sequence = committed_stream_version(connection, stream).await?; | |
| if current_sequence == 0 { | |
| return Err(EngineError::EmptySnapshotUpdate); | |
| } | |
| if snapshot.last_sequence > current_sequence { | |
| return Err(EngineError::SnapshotBeyondCurrentVersion { | |
| proposed: snapshot.last_sequence, | |
| current: current_sequence, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/event-sorcery/src/engine.rs` around lines 1063 - 1074, Update
store_snapshot_in_transaction to use the existing committed_stream_version
oracle instead of loading events through load_events_on and reading
events.last().sequence. Preserve the empty-stream behavior when no committed
version exists, while allowing compacted streams to validate
snapshot.last_sequence against the version reported by committed_stream_version.
|
|
||
| #[tokio::test] | ||
| async fn snapshot_operations_use_the_existing_snapshot_table() { | ||
| let pool = create_test_pool().await.unwrap(); | ||
| let engine = Engine::new(pool); | ||
| let stream = StreamIdentity::new("engine-snapshot-test", "one"); | ||
| let event = SerializedEvent { | ||
| aggregate_type: "engine-snapshot-test".to_string(), | ||
| aggregate_id: "one".to_string(), | ||
| sequence: 1, | ||
| event_type: "Created".to_string(), | ||
| event_version: "1.0".to_string(), | ||
| payload: serde_json::json!({}), | ||
| metadata: serde_json::json!({}), | ||
| }; | ||
| engine | ||
| .commit(CommitRequest::new(stream.clone(), &[event])) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let version = engine | ||
| .store_snapshot( | ||
| &stream, | ||
| SnapshotWrite::new(serde_json::json!({ "value": 42 }), 1), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(version, 1); | ||
| let snapshot = engine.load_snapshot(&stream).await.unwrap().unwrap(); | ||
| assert_eq!(snapshot.current_sequence, 1); | ||
| assert_eq!(snapshot.current_snapshot, 1); | ||
| assert_eq!(snapshot.aggregate, serde_json::json!({ "value": 42 })); | ||
|
|
||
| let second_event = SerializedEvent { | ||
| aggregate_type: "engine-snapshot-test".to_string(), | ||
| aggregate_id: "one".to_string(), | ||
| sequence: 2, | ||
| event_type: "Changed".to_string(), | ||
| event_version: "1.0".to_string(), | ||
| payload: serde_json::json!({}), | ||
| metadata: serde_json::json!({}), | ||
| }; | ||
| engine | ||
| .commit(CommitRequest::new(stream.clone(), &[second_event])) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!( | ||
| engine | ||
| .store_snapshot( | ||
| &stream, | ||
| SnapshotWrite::new(serde_json::json!({ "value": 84 }), 2), | ||
| ) | ||
| .await | ||
| .unwrap(), | ||
| 2 | ||
| ); | ||
| assert!(matches!( | ||
| engine | ||
| .store_snapshot( | ||
| &stream, | ||
| SnapshotWrite::new(serde_json::json!({ "value": 21 }), 1), | ||
| ) | ||
| .await, | ||
| Err(EngineError::OptimisticLock) | ||
| )); | ||
| let current = engine.load_snapshot(&stream).await.unwrap().unwrap(); | ||
| assert_eq!(current.current_sequence, 2); | ||
| assert_eq!(current.current_snapshot, 2); | ||
| assert_eq!(current.aggregate, serde_json::json!({ "value": 84 })); | ||
|
|
||
| engine.discard_snapshot(&stream).await.unwrap(); | ||
| assert_eq!(engine.load_snapshot(&stream).await.unwrap(), None); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Snapshot tests cover only the accepted path at both layers. This PR adds new rejection paths in the engine and in the C ABI, but each new test exercises a successful store, load, and discard sequence. Every new failure variant is unreachable from the suite, so a regression in snapshot validation ships silently.
crates/event-sorcery/src/engine.rs#L1982-L2055: add cases that triggerEngineError::SnapshotBeyondCurrentVersionandEngineError::EmptySnapshotUpdatethroughstore_snapshot, and assert the stored snapshot is unchanged after each rejection.crates/event-sorcery-ffi/src/lib.rs#L1455-L1467: add a case that writes a JSON object snapshot through the leased engine, then assertses_snapshot_loadreturnsES_ERR_STORAGEforSnapshotPayloadFault::NotOpaqueBytes, and a case coveringByteOutOfRange.
As per coding guidelines: "All new or modified logic must have corresponding test coverage."
📍 Affects 2 files
crates/event-sorcery/src/engine.rs#L1982-L2055(this comment)crates/event-sorcery-ffi/src/lib.rs#L1455-L1467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/event-sorcery/src/engine.rs` around lines 1982 - 2055, Extend the
snapshot tests in crates/event-sorcery/src/engine.rs:1982-2055 around
snapshot_operations_use_the_existing_snapshot_table to exercise
SnapshotBeyondCurrentVersion and EmptySnapshotUpdate through store_snapshot,
asserting each rejected write leaves the persisted snapshot unchanged. Also
extend the FFI snapshot tests in crates/event-sorcery-ffi/src/lib.rs:1455-1467
to write a JSON-object snapshot through the leased engine and assert
es_snapshot_load returns ES_ERR_STORAGE for
SnapshotPayloadFault::NotOpaqueBytes, plus add coverage for ByteOutOfRange.
Source: Coding guidelines
| if grep -q 'generated by prek' .git/hooks/pre-commit 2>/dev/null; then | ||
| if grep -q GITBUTLER_MANAGED_HOOK_V1 .git/hooks/pre-commit.legacy 2>/dev/null; then | ||
| mv .git/hooks/pre-commit .git/hooks/pre-commit-user | ||
| mv .git/hooks/pre-commit.legacy .git/hooks/pre-commit |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Active hooks directory: '
git rev-parse --git-path hooks
git config --show-origin --get core.hooksPath || true
git worktree list
for path in flake.nix SPEC.md docs/domain.md; do
if test -f "$path"; then
rg -n -C 4 \
'GITBUTLER_MANAGED_HOOK_V1|generated by prek|pre-commit(\.legacy|-user)?' \
"$path" || true
fi
doneRepository: dataclique/event-sorcery
Length of output: 751
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- required documentation ---'
for path in SPEC.md docs/domain.md; do
if test -f "$path"; then
printf '\n### %s (%s lines)\n' "$path" "$(wc -l < "$path")"
sed -n '1,220p' "$path"
else
printf '\n### %s: missing\n' "$path"
fi
done
printf '\n%s\n' '--- flake.nix structure and relevant context ---'
ast-grep outline flake.nix --lang nix 2>/dev/null || true
sed -n '130,190p' flake.nix
printf '\n%s\n' '--- hook-related references ---'
rg -n -C 5 \
'shellHook|git rev-parse|core\.hooksPath|\.git/hooks|pre-commit\.legacy|pre-commit-user|generated by prek|GITBUTLER_MANAGED_HOOK_V1' \
--glob '!result' --glob '!node_modules' .
printf '\n%s\n' '--- repository status and tracked paths ---'
git status --short
git ls-files SPEC.md docs/domain.md flake.nixRepository: dataclique/event-sorcery
Length of output: 23012
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- hook definition and flake inputs ---'
sed -n '1,140p' flake.nix
rg -n -C 8 \
'hooks\s*=|prek|git-hooks|shellHook|pre-commit|GITBUTLER_MANAGED_HOOK_V1' \
flake.nix README.md docs SPEC.md .github 2>/dev/null || true
printf '\n%s\n' '--- Git path resolution probes ---'
printf 'default hooks path: '
git rev-parse --git-path hooks
printf 'overridden hooks path: '
git -c core.hooksPath=.custom-hooks rev-parse --git-path hooks
printf 'absolute hooks path: '
git -c core.hooksPath=/tmp/event-sorcery-hooks rev-parse --git-path hooks
printf 'git directory: '
git rev-parse --git-dir
printf 'common git directory: '
git rev-parse --git-common-dir
printf '\n%s\n' '--- current path types ---'
test -e .git && stat -c '%F %n' .git || true
test -e .git/hooks && stat -c '%F %n' .git/hooks || trueRepository: dataclique/event-sorcery
Length of output: 9525
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- git-hooks.nix ---'
wc -l git-hooks.nix
sed -n '1,260p' git-hooks.nix
printf '\n%s\n' '--- repository metadata relevant to worktrees and hooks ---'
git config --show-origin --get-regexp '^(core\.hooksPath|extensions\.worktreeConfig|worktree\.)' || true
git worktree list --porcelain
git rev-parse --is-inside-work-tree
git rev-parse --is-bare-repository
git rev-parse --show-toplevel
printf '\n%s\n' '--- all hook path references ---'
rg -n -C 6 \
'git-path hooks|core\.hooksPath|\.git/hooks|hooksPath|pre-commit\.legacy|pre-commit-user|GITBUTLER_MANAGED_HOOK_V1' \
--glob '!flake.lock' .Repository: dataclique/event-sorcery
Length of output: 1800
🌐 Web query:
git githooks core.hooksPath git rev-parse --git-path hooks linked worktree official documentation
💡 Result:
Git hooks are generally managed via the core.hooksPath configuration variable, which allows you to specify a custom directory for hook scripts instead of the default $GIT_DIR/hooks [1][2]. Regarding your query about worktrees and path resolution: core.hooksPath By default, Git looks for hooks in $GIT_DIR/hooks [1][2]. Setting core.hooksPath overrides this globally or per-repository [1]. The path can be absolute or relative (relative to the directory where the hooks are run) [1][3]. You can disable hooks entirely by setting this to /dev/null [1][3]. git rev-parse --git-path This command is the standard way to resolve paths within the Git repository structure, including the hooks directory [4][5]. It respects overrides, including core.hooksPath [6][5]. If you run git rev-parse --git-path hooks in a repository, it will return the configured hooks directory [6]. Linked Worktrees and Hooks Git worktrees are managed by a main repository ($GIT_COMMON_DIR) and private directories for each worktree (
Citations:
- 1: https://github.com/git/git/blob/master/Documentation/config/core.adoc
- 2: https://git-scm.com/docs/githooks
- 3: https://github.com/git/git/blob/e9019fca/Documentation/config/core.adoc
- 4: https://git.github.io/htmldocs/git-worktree.html
- 5: https://public-inbox.org/git/2f55d6fb2a158c5b26b93ddb9c144ce1af5d9c32.1570534405.git.gitgitgadget@gmail.com/
- 6: https://public-inbox.org/git/594839500.1229861.1471423409171.JavaMail.zimbra@ensimag.grenoble-inp.fr/T/
- 7: https://man7.org/linux/man-pages/man1/git-worktree.1.html
- 8: https://git-scm.com/docs/git-worktree.html
- 9: https://public-inbox.org/git/49862352.x4qs2Yizgb@thunderbird/T/
- 10: https://stackoverflow.com/questions/79186993/using-git-hooks-with-worktree
Resolve the active Git hooks directory before moving hooks.
When core.hooksPath is configured or the repository uses a linked worktree, .git/hooks may be inactive. Resolve hooks_dir with git rev-parse --git-path hooks and use it for every marker check and mv operation. Add coverage for normal repositories, linked worktrees, and configured core.hooksPath.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@flake.nix` around lines 165 - 168, Update the hook migration logic around the
generated-by-prek and GITBUTLER_MANAGED_HOOK_V1 checks to resolve the active
hooks directory via git rev-parse --git-path hooks, then use hooks_dir for every
marker lookup and mv operation instead of .git/hooks. Add coverage for standard
repositories, linked worktrees, and repositories with configured core.hooksPath.
Source: Coding guidelines
1 participant
Verified with Rust engine and FFI tests, the Nix Haskell build, HLint, rustfmt, and Clippy.
Closes #72.
This is part 18 of 32 in a stack made with GitButler:
Summary by CodeRabbit