fix: correctness bugs across engine, generator, CRDT, and conditions

cmd/deep-gen/main.go

@@ -545,7 +545,8 @@ var applyOpTmpl = template.Must(template.New("applyOp").Funcs(tmplFuncs).Parse(
 	switch op.Path {
 	case "/":
 		if op.Strict && (op.Kind == {{.P}}OpReplace || op.Kind == {{.P}}OpRemove) {
-			if !{{.P}}Equal(*t, op.Old.({{.TypeName}})) {
+			old, ok := op.Old.({{.TypeName}})
+			if !ok || !{{.P}}Equal(*t, old) {
 				return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t)
 			}
 		}

condition/condition.go

@@ -58,13 +58,14 @@ func Evaluate(root reflect.Value, c *Condition) (bool, error) {
 		return false, nil
 	}
 	if c.Op == Not {
-		if len(c.Sub) > 0 {
-			ok, err := Evaluate(root, c.Sub[0])
-			if err != nil {
-				return false, err
-			}
-			return !ok, nil
+		if len(c.Sub) == 0 {
+			return false, fmt.Errorf("malformed Not condition: missing sub-condition")
+		}
+		ok, err := Evaluate(root, c.Sub[0])
+		if err != nil {
+			return false, err
 		}
+		return !ok, nil
 	}
 
 	val, err := icore.DeepPath(c.Path).Resolve(root)

condition/condition_test.go

@@ -76,3 +76,17 @@ func TestEvaluate(t *testing.T) {
 		}
 	}
 }
+
+// TestNotEmptySubReturnsError asserts that a malformed Not condition
+// (no Sub-condition) yields an explicit error rather than silently
+// falling through to the path-based comparison that would compare a
+// nil-path value against c.Value.
+func TestNotEmptySubReturnsError(t *testing.T) {
+	type S struct{ N int }
+	root := reflect.ValueOf(&S{N: 1}).Elem()
+	c := &Condition{Op: Not}
+	got, err := Evaluate(root, c)
+	if err == nil {
+		t.Errorf("Evaluate(Not{empty Sub}) = %v, nil; want error", got)
+	}
+}

crdt/crdt.go

@@ -392,6 +392,6 @@ func (c *CRDT[T]) UnmarshalJSON(data []byte) error {
 	c.tombstones = m.Tombstones
 	c.nodeID = m.NodeID
 	c.clock = hlc.NewClock(m.NodeID)
-	c.clock.Latest = m.Latest
+	c.clock.SetLatest(m.Latest)
 	return nil
 }

crdt/hlc/hlc.go

@@ -15,6 +15,7 @@ package hlc
 
 import (
 	"fmt"
+	"math"
 	"sync"
 	"time"
 )
@@ -59,6 +60,11 @@ func (h HLC) String() string {
 }
 
 // Clock manages the local HLC state.
+//
+// Latest is exposed for serialisation but must not be mutated by callers
+// after the clock is in use — direct writes bypass the internal mutex and
+// race with concurrent Now/Update. Use [Clock.SetLatest] for explicit
+// rehydration (e.g. from snapshots).
 type Clock struct {
 	mu     sync.Mutex
 	Latest HLC
@@ -77,13 +83,31 @@ func NewClock(nodeID string) *Clock {
 	}
 }
 
+// SetLatest rehydrates the clock from a previously observed timestamp under
+// the clock's mutex, so it is safe to call alongside concurrent Now/Update.
+// Subsequent Now/Update calls advance from at least h.
+func (c *Clock) SetLatest(h HLC) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	c.Latest = h
+}
+
 // Now returns the current HLC timestamp.
 func (c *Clock) Now() HLC {
 	return c.Reserve(1)
 }
 
 // Reserve returns the current HLC timestamp and reserves n logical ticks.
+//
+// n must be non-negative and small enough that c.Latest.Logical + n fits in
+// int32; otherwise Reserve panics. Practical text inserts and similar uses
+// fall well under that bound; overflow would silently break causal ordering,
+// so an explicit panic is preferred to a wraparound bug.
 func (c *Clock) Reserve(n int) HLC {
+	if n < 0 {
+		panic("hlc: Reserve called with negative n")
+	}
+
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
@@ -94,6 +118,10 @@ func (c *Clock) Reserve(n int) HLC {
 		c.Latest.Logical = 0
 	}
 
+	if int64(c.Latest.Logical)+int64(n) > int64(math.MaxInt32) {
+		panic("hlc: Reserve would overflow Logical (int32)")
+	}
+
 	start := c.Latest
 	c.Latest.Logical += int32(n)
 	return start

crdt/hlc/hlc_test.go

@@ -1,6 +1,7 @@
 package hlc
 
 import (
+	"math"
 	"testing"
 	"time"
 )
@@ -97,3 +98,48 @@ func TestClock_UpdateMore(t *testing.T) {
 		t.Errorf("expected logical %d, got %d", remote2.Logical+1, c.Latest.Logical)
 	}
 }
+
+// TestSetLatestSafeConcurrentWithNow stresses SetLatest against concurrent
+// Now calls and checks that the race detector reports no data race on
+// Clock.Latest. Direct field assignment would race here; SetLatest must
+// take the clock mutex to be safe.
+func TestSetLatestSafeConcurrentWithNow(t *testing.T) {
+	c := NewClock("n1")
+	done := make(chan struct{})
+	go func() {
+		for i := 0; i < 1000; i++ {
+			_ = c.Now()
+		}
+		close(done)
+	}()
+	for i := 0; i < 1000; i++ {
+		c.SetLatest(HLC{WallTime: int64(i), Logical: 0, NodeID: "n1"})
+	}
+	<-done
+}
+
+// TestReserveOverflowPanics ensures Reserve panics rather than silently
+// wrapping when the requested reservation would overflow Logical (int32).
+func TestReserveOverflowPanics(t *testing.T) {
+	c := NewClock("n1")
+	// Use a far-future wall time so Reserve doesn't reset Logical to 0 before
+	// the overflow check fires.
+	c.SetLatest(HLC{WallTime: math.MaxInt64, Logical: math.MaxInt32 - 10, NodeID: "n1"})
+	defer func() {
+		if r := recover(); r == nil {
+			t.Error("expected panic on int32 overflow, got nil")
+		}
+	}()
+	c.Reserve(100)
+}
+
+// TestReserveNegativePanics rejects nonsensical reservations.
+func TestReserveNegativePanics(t *testing.T) {
+	c := NewClock("n1")
+	defer func() {
+		if r := recover(); r == nil {
+			t.Error("expected panic on negative n")
+		}
+	}()
+	c.Reserve(-1)
+}

crdt/set.go

@@ -37,6 +37,12 @@ func (s *Set[T]) NodeID() string { return s.inner.NodeID() }
 
 // Add appends a new uniquely-tagged entry for elem.
 // The tag is the current HLC timestamp serialised as a string map key.
+//
+// Two clock ticks occur per Add: one to mint the entry's tag and a second
+// inside the underlying Edit for the resulting Delta's Timestamp. Both
+// values are monotonic per the HLC mutex, so the extra tick is harmless;
+// keeping the tag-mint outside Edit means the tag is fixed before the
+// inner closure runs, which keeps the data-flow easy to follow.
 func (s *Set[T]) Add(elem T) {
 	id := s.inner.Clock().Now()
 	s.inner.Edit(func(si *setInner[T]) {
@@ -86,8 +92,19 @@ func (s *Set[T]) Items() []T {
 }
 
 // Len returns the number of distinct live elements.
+//
+// Cost is O(n) in the number of entries (live + tombstoned) because OR-Set
+// duplicates require dedup; the prior implementation built a full slice via
+// Items just to take its length, which this avoids.
 func (s *Set[T]) Len() int {
-	return len(s.Items())
+	state := s.inner.View()
+	seen := make(map[T]struct{}, len(state.Entries))
+	for _, e := range state.Entries {
+		if !e.Deleted {
+			seen[e.Elem] = struct{}{}
+		}
+	}
+	return len(seen)
 }
 
 // Merge performs a full state-based OR-Set merge with another Set node.

crdt/set_test.go

@@ -50,6 +50,27 @@ func TestSet_Items(t *testing.T) {
 	}
 }
 
+// TestSet_LenMatchesItemsAfterChurn asserts Len agrees with len(Items) after a
+// mix of adds, duplicates, and removes. The previous Len implementation called
+// Items() and took its length; the new direct dedup must produce the same
+// distinct-live count without going through the allocation.
+func TestSet_LenMatchesItemsAfterChurn(t *testing.T) {
+	s := NewSet[string]("node-a")
+	s.Add("a")
+	s.Add("b")
+	s.Add("a") // duplicate live add
+	s.Add("c")
+	s.Remove("b") // tombstone
+
+	want := len(s.Items())
+	if got := s.Len(); got != want {
+		t.Errorf("Len()=%d, len(Items())=%d", got, want)
+	}
+	if want != 2 {
+		t.Errorf("expected 2 distinct live items, got %d", want)
+	}
+}
+
 func TestSet_DuplicateAdd(t *testing.T) {
 	s := NewSet[string]("node-a")
 	s.Add("dup")

diff.go

@@ -35,12 +35,21 @@ func Diff[T any](a, b T) (Patch[T], error) {
 
 	res := Patch[T]{}
 	p.Walk(func(path string, op engine.OpKind, old, new any) error {
-		res.Operations = append(res.Operations, Operation{
+		o := Operation{
 			Kind: op,
 			Path: path,
-			Old:  old,
 			New:  new,
-		})
+		}
+		// Internal walk emits the source path in `old` for Move/Copy; lift it
+		// into the typed From field so Old stays free for prior values.
+		if op == engine.OpMove || op == engine.OpCopy {
+			if s, ok := old.(string); ok {
+				o.From = s
+			}
+		} else {
+			o.Old = old
+		}
+		res.Operations = append(res.Operations, o)
 		return nil
 	})
 

engine_test.go

@@ -108,6 +108,34 @@ func TestNilMapDiff(t *testing.T) {
 	}
 }
 
+// TestOpCopyDeepCopies asserts OpCopy on a reference-typed field gives the
+// destination its own backing storage, so mutations to the source no longer
+// leak into the destination.
+func TestOpCopyDeepCopies(t *testing.T) {
+	type S struct {
+		A []int
+		B []int
+		M map[string]int
+		N map[string]int
+	}
+	s := &S{A: []int{1, 2, 3}, M: map[string]int{"k": 1}}
+	p := deep.Patch[S]{Operations: []deep.Operation{
+		{Kind: deep.OpCopy, Path: "/B", From: "/A"},
+		{Kind: deep.OpCopy, Path: "/N", From: "/M"},
+	}}
+	if err := deep.Apply(s, p); err != nil {
+		t.Fatalf("Apply: %v", err)
+	}
+	s.A[0] = 99
+	s.M["k"] = 99
+	if s.B[0] == 99 {
+		t.Errorf("OpCopy on []int aliases source: B=%v", s.B)
+	}
+	if s.N["k"] == 99 {
+		t.Errorf("OpCopy on map aliases source: N=%v", s.N)
+	}
+}
+
 func TestReflectionEngineAdvanced(t *testing.T) {
 	type Data struct {
 		A int
@@ -117,8 +145,8 @@ func TestReflectionEngineAdvanced(t *testing.T) {
 
 	p := deep.Patch[Data]{}
 	p.Operations = []deep.Operation{
-		{Kind: deep.OpMove, Path: "/B", Old: "/A"},
-		{Kind: deep.OpCopy, Path: "/A", Old: "/B"},
+		{Kind: deep.OpMove, Path: "/B", From: "/A"},
+		{Kind: deep.OpCopy, Path: "/A", From: "/B"},
 		{Kind: deep.OpRemove, Path: "/A"},
 	}
 
@@ -127,18 +155,56 @@ func TestReflectionEngineAdvanced(t *testing.T) {
 	}
 }
 
+// TestStrictRootMismatchedOldType asserts that a strict OpReplace at root
+// whose Old value carries the wrong concrete type returns an error rather
+// than panicking on the type assertion.
+func TestStrictRootMismatchedOldType(t *testing.T) {
+	u := &testmodels.User{Name: "alice"}
+	p := deep.Patch[testmodels.User]{
+		Strict: true,
+		Operations: []deep.Operation{
+			{Kind: deep.OpReplace, Path: "/", Old: "not-a-User", New: testmodels.User{Name: "bob"}},
+		},
+	}
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("Apply panicked on mismatched Old type: %v", r)
+		}
+	}()
+	if err := deep.Apply(u, p); err == nil {
+		t.Error("expected strict check error on mismatched Old type, got nil")
+	}
+}
+
 func TestEngineFailures(t *testing.T) {
 	u := &testmodels.User{}
 
-	// Move from non-existent
+	// Move from non-existent path must surface an error rather than silently
+	// no-op (previously this test ignored the return value).
 	p1 := deep.Patch[testmodels.User]{}
-	p1.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id", Old: "/nonexistent"}}
-	deep.Apply(u, p1)
+	p1.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id", From: "/nonexistent"}}
+	if err := deep.Apply(u, p1); err == nil {
+		t.Error("OpMove from non-existent source should return an error")
+	}
 
-	// Copy from non-existent
+	// Copy from non-existent path must also surface an error.
 	p2 := deep.Patch[testmodels.User]{}
-	p2.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id", Old: "/nonexistent"}}
-	deep.Apply(u, p2)
+	p2.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id", From: "/nonexistent"}}
+	if err := deep.Apply(u, p2); err == nil {
+		t.Error("OpCopy from non-existent source should return an error")
+	}
+
+	// Move/Copy with empty From must reject early with a clear error.
+	p3 := deep.Patch[testmodels.User]{}
+	p3.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id"}}
+	if err := deep.Apply(u, p3); err == nil {
+		t.Error("OpMove with empty From should return an error")
+	}
+	p4 := deep.Patch[testmodels.User]{}
+	p4.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id"}}
+	if err := deep.Apply(u, p4); err == nil {
+		t.Error("OpCopy with empty From should return an error")
+	}
 
 	// Apply to nil
 	if err := deep.Apply((*testmodels.User)(nil), p1); err == nil {