CORE-1: Op envelope + per-zone Lamport + UUID v7#4
Merged
Conversation
Subtask 1 of CORE-1: thin wrapper over Uuidm pinning the v7 variant.
All client_id values in the wire envelope are values of this type.
Module: core/lib/crdt/uuid_v7.{ml,mli}
- type t (abstract outside the module; Uuidm.t inside)
- v / to_bytes / of_bytes / to_string / of_string / compare / pp
- of_bytes + of_string validate length AND that the version nibble is 0x7
Tests: core/tests/test_uuid_v7.ml — 16 alcotest cases incl. 3 qcheck
properties (round-trips + compare antisymmetry) and a Slow stress test
that generates 100k UUIDs in a tight loop to probabilistically exercise
the per-millisecond counter overflow / None-retry path.
Wrapper modules patched to re-export submodules:
- core/lib/crdt/crdtsync_crdt.ml now `module Uuid_v7 = Uuid_v7`
- Reminder comment added to crdtsync_wire / persist / auth / blob /
server / sdk wrappers explaining the dune wrapped-library pattern
(matching-name file = wrapper; siblings must be re-exported here).
Build deps:
- opam: crdtsync_core now depends on (uuidm (>= 0.9.10))
- dune: core/lib/crdt now links uuidm + unix (Unix for the time
source; portability note documented for the WASM work in v0.3)
- test deps: qcheck-alcotest declared :with-test in crdtsync_core.opam
Abstract (client_id, client_seq) op identity. - mli: make / client_id / client_seq / compare / equal / pp - Total order: client_id first, client_seq on tie - 13 tests: alcotest unit + qcheck properties (reflexive, antisymmetric, equal<=>compare=0, round-trip)
Per-zone logical clock, nonneg int64 internally. - mli: zero / tick / merge / compare / equal / pp / to_int64 / of_int64 - merge ~recv ~local = max(recv, local) + 1 - of_int64 raises Failure on negative input (codec validates upstream) - 20 tests: alcotest unit + qcheck properties (round-trip, reflexive, antisymmetric, equal<=>compare=0, tick monotone, merge dominates+symmetric)
- Op.kind: single Placeholder constructor (CORE-8 fills full closed enum) - Envelope: immutable record (private) with op_id, actor_id, room, branch, zone, schema_version, lamport, wall_time, op, and optional (tx_id, tx_role) - tx_id typed as Uuid_v7.t (client-generated); tx_id + tx_role folded into one option pair so invalid state is unrepresentable - Wall_time: nonneg int64 ms since Unix epoch; now / of_ms / to_ms / compare / equal / pp - 23 new tests: 12 envelope (op kind, make, equal, tx_role, pp) + 11 wall_time (of_ms round-trip + reject, compare, equal, pp, now sanity, qcheck props) Closes CORE-1.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="core/tests/test_envelope.ml">
<violation number="1" location="core/tests/test_envelope.ml:120">
P0: Variant constructors `Member` and `Commit` need the `Envelope.` prefix. Without the qualification they are unbound, causing a compilation error.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+120
to
+122
| Alcotest.(check bool) "Member = Member" true (Envelope.equal_tx_role Member Member); | ||
| Alcotest.(check bool) "Commit = Commit" true (Envelope.equal_tx_role Commit Commit); | ||
| Alcotest.(check bool) "Member <> Commit" false (Envelope.equal_tx_role Member Commit) |
There was a problem hiding this comment.
P0: Variant constructors Member and Commit need the Envelope. prefix. Without the qualification they are unbound, causing a compilation error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At core/tests/test_envelope.ml, line 120:
<comment>Variant constructors `Member` and `Commit` need the `Envelope.` prefix. Without the qualification they are unbound, causing a compilation error.</comment>
<file context>
@@ -0,0 +1,167 @@
+(* ── tx_role ──────────────────────────────────────────────────────────────── *)
+
+let test_equal_tx_role () =
+ Alcotest.(check bool) "Member = Member" true (Envelope.equal_tx_role Member Member);
+ Alcotest.(check bool) "Commit = Commit" true (Envelope.equal_tx_role Commit Commit);
+ Alcotest.(check bool) "Member <> Commit" false (Envelope.equal_tx_role Member Commit)
</file context>
Suggested change
| Alcotest.(check bool) "Member = Member" true (Envelope.equal_tx_role Member Member); | |
| Alcotest.(check bool) "Commit = Commit" true (Envelope.equal_tx_role Commit Commit); | |
| Alcotest.(check bool) "Member <> Commit" false (Envelope.equal_tx_role Member Commit) | |
| Alcotest.(check bool) "Member = Member" true (Envelope.equal_tx_role Envelope.Member Envelope.Member); | |
| Alcotest.(check bool) "Commit = Commit" true (Envelope.equal_tx_role Envelope.Commit Envelope.Commit); | |
| Alcotest.(check bool) "Member <> Commit" false (Envelope.equal_tx_role Envelope.Member Envelope.Commit) |
Member
Author
There was a problem hiding this comment.
Not a bug — OCaml does type-directed constructor disambiguation. Envelope.equal_tx_role has signature tx_role -> tx_role -> bool, so the expected type is known at the call site and the compiler resolves unqualified Member/Commit to Envelope.Member/Envelope.Commit. This has been part of OCaml since 4.01 (2013).
The file compiles and tests pass both with dune build locally and in CI.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Uuidm.of_binary_string reads the first 16 bytes regardless of input length, so longer inputs were silently truncated and accepted. CI hit this on a 17-byte case; local was a no-op by uuidm version luck. Reject anything that isn't exactly 16 bytes before parsing.
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
1 participant
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.
Summary
Uuid_v7,Op_id,Lamport,Wall_time,Op,Envelopemodules undercrdtsync_core.crdt.Modules
Uuid_v7v,to_bytes/of_bytes,to_string/of_string,compare,ppuuidmv7 monotonic generator.Op_idmake ~client_id ~client_seq, accessors,compare/equal/ppclient_idfirst,client_seqon tie.Lamportzero,tick,merge ~recv ~local,compare,equal,pp,to_int64,of_int64int64;of_int64raises on negative.Wall_timenow,of_ms,to_ms,compare,equal,ppOpkind = Placeholder,pp_kind,equal_kindEnvelopemake,equal,pp;tx_role = Member | Committx_id+tx_rolefolded into one(Uuid_v7.t * tx_role) optionso invalid state is unrepresentable.Commits
de3f299CORE-1.1:Uuid_v7wrapper + 16 testse846325CORE-1.2:Op_id+ 13 testsc2c6c62CORE-1.3:Lamport+ 20 tests0890ad7CORE-1.4:Opplaceholder +Envelope+Wall_time+ 23 tests727e3f7docs: tick CORE-1 in KANBANTest plan
Summary by cubic
Adds an operation envelope, per-zone Lamport clock, and UUID v7 support to the core CRDT library to provide stable IDs, causal ordering, and transaction metadata. Also fixes the v7 binary parser to reject non-16-byte inputs. Implements CORE-1 and exposes new modules under
crdtsync_core.crdt.New Features
Uuid_v7,Op_id,Lamport,Wall_time,Op(placeholder), andEnvelopemodules.Envelopecarries op metadata and encodes transaction membership as(Uuid_v7 * role) optionto avoid invalid states (Member|Commit).Crdtsync_crdt.*; added unit/property tests for round-trips, ordering, and clock invariants; linkeduuidm/unix; addedqcheck-alcotestfor tests.Bug Fixes
Uuid_v7.of_bytesnow rejects inputs that are not exactly 16 bytes to prevent silent truncation byuuidm.Written for commit c629523. Summary will update on new commits. Review in cubic