From 6b7e375057645b8e43bf3ffb60b90519c6b6d31f Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 07:19:14 -0400 Subject: [PATCH 01/13] fix(engine): deep-copy OpCopy source to avoid aliased reference types The reflection apply path for OpCopy resolved the source value and called Set with the live reflect.Value, which for reference types (slices, maps, pointers) made the destination share the source's backing storage. Subsequent mutations to either side leaked into the other. OpMove already handled this correctly by allocating a fresh value before Set; OpCopy now does the same via icore.DeepCopyValue so the destination owns its storage. Add TestOpCopyDeepCopies covering []int and map fields. --- engine_test.go | 28 ++++++++++++++++++++++++++++ internal/engine/apply_reflection.go | 2 +- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/engine_test.go b/engine_test.go index b4ecb0b..63e7040 100644 --- a/engine_test.go +++ b/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", Old: "/A"}, + {Kind: deep.OpCopy, Path: "/N", Old: "/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 diff --git a/internal/engine/apply_reflection.go b/internal/engine/apply_reflection.go index 18ac799..0c559bc 100644 --- a/internal/engine/apply_reflection.go +++ b/internal/engine/apply_reflection.go @@ -86,7 +86,7 @@ func ApplyOpReflectionValue(v reflect.Value, op Operation, logger *slog.Logger) var val reflect.Value val, err = icore.DeepPath(fromPath).Resolve(v) if err == nil { - err = icore.DeepPath(op.Path).Set(v, val) + err = icore.DeepPath(op.Path).Set(v, icore.DeepCopyValue(val)) } case OpLog: logger.Info("deep log", "message", op.New, "path", op.Path) From 8c58bc00b4bceaca70fe0dd0b83db7c1c1e7dfa7 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 07:20:03 -0400 Subject: [PATCH 02/13] fix(selector): RFC 6901-escape map keys in MapKey paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MapKey formatted keys with fmt.Sprintf("%s/%v", ...) directly into the path string, so a key containing '/' (e.g. "a/b") produced "/M/a/b" — the path navigator then treated it as two segments and failed with "map key a not found". '~' suffered the same RFC 6901 layering issue silently. Apply icore.EscapeKey to the key before joining so '/' becomes "~1" and '~' becomes "~0", matching ParsePath's unescaping. End-to-end test asserts both the produced path string and a successful Set through a key with '/'. --- selector.go | 5 ++++- selector_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/selector.go b/selector.go index b59fbee..269765f 100644 --- a/selector.go +++ b/selector.go @@ -5,6 +5,8 @@ import ( "reflect" "strings" "sync" + + icore "github.com/brunoga/deep/v5/internal/core" ) // selector is a function that retrieves a field from a struct of type T. @@ -40,8 +42,9 @@ func At[T any, S ~[]E, E any](p Path[T, S], i int) Path[T, E] { } // MapKey returns a type-safe path to the value at key k within a map field. +// Keys are RFC 6901-escaped so values containing '/' or '~' navigate correctly. func MapKey[T any, M ~map[K]V, K comparable, V any](p Path[T, M], k K) Path[T, V] { - return Path[T, V]{path: fmt.Sprintf("%s/%v", p.String(), k)} + return Path[T, V]{path: fmt.Sprintf("%s/%s", p.String(), icore.EscapeKey(fmt.Sprintf("%v", k)))} } // pathCache stores resolved paths keyed by selector function pointer. diff --git a/selector_test.go b/selector_test.go index beb9291..45c8a51 100644 --- a/selector_test.go +++ b/selector_test.go @@ -64,6 +64,46 @@ func TestSelectorNestedPointer(t *testing.T) { } } +// TestMapKeyEscapesJSONPointerSpecials verifies that map keys containing the +// JSON Pointer reserved characters '/' and '~' are RFC 6901-escaped so the +// resulting path navigates to the correct key. +func TestMapKeyEscapesJSONPointerSpecials(t *testing.T) { + type S struct { + M map[string]int `json:"m"` + } + + mPath := deep.Field(func(s *S) *map[string]int { return &s.M }) + + cases := []struct { + key string + wantPath string + }{ + {"a/b", "/m/a~1b"}, + {"c~d", "/m/c~0d"}, + {"~/", "/m/~0~1"}, + {"plain", "/m/plain"}, + } + for _, c := range cases { + got := deep.MapKey[S, map[string]int, string, int](mPath, c.key).String() + if got != c.wantPath { + t.Errorf("MapKey(%q) path = %q, want %q", c.key, got, c.wantPath) + } + } + + // End-to-end: a Set through MapKey on a slash-bearing key must hit the + // right entry. + s := &S{M: map[string]int{"a/b": 0, "other": 0}} + p := deep.Edit(s).With( + deep.Set(deep.MapKey[S, map[string]int, string, int](mPath, "a/b"), 42), + ).Build() + if err := deep.Apply(s, p); err != nil { + t.Fatalf("Apply: %v", err) + } + if s.M["a/b"] != 42 { + t.Errorf("expected M[\"a/b\"] == 42 after Apply, got %v", s.M) + } +} + // TestSelectorCircularType verifies that self-referential struct types do not // cause infinite recursion during path resolution. func TestSelectorCircularType(t *testing.T) { From 0604aefbb229bb44ce84a5d1a8d743640a3e49bb Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 07:21:49 -0400 Subject: [PATCH 03/13] fix(patch): drop OpLog operations during Reverse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Patch.Reverse switched on op.Kind but had no case for OpLog, so a reversed OpLog became a zero-valued Operation. Since OpAdd is the zero value of OpKind, the result was an OpAdd at the original log path with nil New — re-applying the reversed patch would attempt a bogus add. OpLog has no state effect, so the correct inverse is to drop it. Update TestPatchReverseExhaustive to expect 5 reversed ops (OpLog skipped) and add TestPatchReverseOpLogOnly covering the all-log case. --- patch.go | 5 +++++ patch_test.go | 30 ++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/patch.go b/patch.go index fd430bb..7da9906 100644 --- a/patch.go +++ b/patch.go @@ -122,6 +122,11 @@ func (p Patch[T]) Reverse() Patch[T] { } for i := len(p.Operations) - 1; i >= 0; i-- { op := p.Operations[i] + // OpLog has no state effect; its inverse is itself a no-op. Skip rather + // than emit a zero-valued Operation (which would default to OpAdd). + if op.Kind == OpLog { + continue + } rev := Operation{ Path: op.Path, } diff --git a/patch_test.go b/patch_test.go index 4dca040..582548d 100644 --- a/patch_test.go +++ b/patch_test.go @@ -168,8 +168,34 @@ func TestPatchReverseExhaustive(t *testing.T) { } rev := p.Reverse() - if len(rev.Operations) != 6 { - t.Errorf("expected 6 reversed ops, got %d", len(rev.Operations)) + // OpLog has no state effect; Reverse skips it instead of emitting a + // malformed op whose Kind defaults to OpAdd. + if len(rev.Operations) != 5 { + t.Errorf("expected 5 reversed ops (OpLog skipped), got %d", len(rev.Operations)) + } + for _, op := range rev.Operations { + if op.Kind == deep.OpLog { + t.Errorf("Reverse should drop OpLog, got %+v", op) + } + // Reversing OpLog used to emit {Kind:OpAdd, Path:"/h", New:nil}; guard + // against that exact regression. + if op.Path == "/h" { + t.Errorf("Reverse leaked OpLog at /h as an OpAdd: %+v", op) + } + } +} + +// TestPatchReverseOpLogOnly asserts that a patch containing only OpLog ops +// reverses to an empty patch rather than a sequence of malformed OpAdds. +func TestPatchReverseOpLogOnly(t *testing.T) { + p := deep.Patch[testmodels.User]{} + p.Operations = []deep.Operation{ + {Kind: deep.OpLog, Path: "/", New: "first"}, + {Kind: deep.OpLog, Path: "/", New: "second"}, + } + rev := p.Reverse() + if len(rev.Operations) != 0 { + t.Errorf("expected empty reverse of OpLog-only patch, got %+v", rev.Operations) } } From ebbe595b64f255e500cf8faa088809adc94a4e10 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 07:30:00 -0400 Subject: [PATCH 04/13] refactor(operation): split source path (From) from prior value (Old) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operation.Old was overloaded: prior value for OpRemove/OpReplace, but source path string for OpMove/OpCopy. The dual use blocked Reverse from restoring the displaced value when an OpCopy/OpMove had overwritten an existing destination — Old was already spoken for as the source path. Add Operation.From (json "f", omitempty) carrying the source path for OpMove/OpCopy and reserve Old to always mean "prior value". Move and Copy constructors, JSON round-trip, Walk-to-Operation flatten, String(), and the reflection apply path all switch to From. Reverse now produces OpReplace for OpCopy with a non-nil Old (restoring the displaced value) and a symmetric OpMove (Path<->From) for OpMove. Wire-format break: patches serialised with the previous "from" field encoding via the Old field will not round-trip. Generated *_deep.go files are unaffected (they never read Operation.From/Old for Move/Copy — those ops fall through to the reflection path). Tests cover OpCopy reverse with a captured prior value and the symmetric OpMove reverse. --- diff.go | 15 ++++++-- engine_test.go | 12 +++---- internal/engine/apply_reflection.go | 14 +++++--- internal/engine/operation.go | 13 +++++++ patch.go | 42 ++++++++++++++-------- patch_test.go | 54 +++++++++++++++++++++++++---- 6 files changed, 116 insertions(+), 34 deletions(-) diff --git a/diff.go b/diff.go index 4f2e029..816ff9a 100644 --- a/diff.go +++ b/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 }) diff --git a/engine_test.go b/engine_test.go index 63e7040..9d8cbc0 100644 --- a/engine_test.go +++ b/engine_test.go @@ -120,8 +120,8 @@ func TestOpCopyDeepCopies(t *testing.T) { } 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", Old: "/A"}, - {Kind: deep.OpCopy, Path: "/N", Old: "/M"}, + {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) @@ -145,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"}, } @@ -160,12 +160,12 @@ func TestEngineFailures(t *testing.T) { // Move from non-existent p1 := deep.Patch[testmodels.User]{} - p1.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id", Old: "/nonexistent"}} + p1.Operations = []deep.Operation{{Kind: deep.OpMove, Path: "/id", From: "/nonexistent"}} deep.Apply(u, p1) // Copy from non-existent p2 := deep.Patch[testmodels.User]{} - p2.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id", Old: "/nonexistent"}} + p2.Operations = []deep.Operation{{Kind: deep.OpCopy, Path: "/id", From: "/nonexistent"}} deep.Apply(u, p2) // Apply to nil diff --git a/internal/engine/apply_reflection.go b/internal/engine/apply_reflection.go index 0c559bc..13b1fbe 100644 --- a/internal/engine/apply_reflection.go +++ b/internal/engine/apply_reflection.go @@ -71,20 +71,24 @@ func ApplyOpReflectionValue(v reflect.Value, op Operation, logger *slog.Logger) case OpRemove: err = icore.DeepPath(op.Path).Delete(v) case OpMove: - fromPath := op.Old.(string) + if op.From == "" { + return fmt.Errorf("move at %s: missing From source path", op.Path) + } var val reflect.Value - val, err = icore.DeepPath(fromPath).Resolve(v) + val, err = icore.DeepPath(op.From).Resolve(v) if err == nil { copied := reflect.New(val.Type()).Elem() copied.Set(val) - if err = icore.DeepPath(fromPath).Delete(v); err == nil { + if err = icore.DeepPath(op.From).Delete(v); err == nil { err = icore.DeepPath(op.Path).Set(v, copied) } } case OpCopy: - fromPath := op.Old.(string) + if op.From == "" { + return fmt.Errorf("copy at %s: missing From source path", op.Path) + } var val reflect.Value - val, err = icore.DeepPath(fromPath).Resolve(v) + val, err = icore.DeepPath(op.From).Resolve(v) if err == nil { err = icore.DeepPath(op.Path).Set(v, icore.DeepCopyValue(val)) } diff --git a/internal/engine/operation.go b/internal/engine/operation.go index 70f0ed0..1440fdd 100644 --- a/internal/engine/operation.go +++ b/internal/engine/operation.go @@ -3,9 +3,22 @@ package engine import "github.com/brunoga/deep/v5/condition" // Operation represents a single change within a Patch. +// +// Field semantics by Kind: +// - OpAdd: Path = target; New = added value. +// - OpRemove: Path = target; Old = removed value (prior). +// - OpReplace: Path = target; Old = prior value; New = replacement. +// - OpMove: Path = destination; From = source path; Old = displaced value at Path (optional). +// - OpCopy: Path = destination; From = source path; Old = displaced value at Path (optional). +// - OpLog: Path = scope; New = log message. +// +// Old for OpMove/OpCopy was previously the source-path string; that role now +// belongs to From, freeing Old to carry the prior destination value +// (necessary for full Reverse fidelity when the destination was non-empty). type Operation struct { Kind OpKind `json:"k"` Path string `json:"p"` + From string `json:"f,omitempty"` Old any `json:"o,omitempty"` New any `json:"n,omitempty"` If *condition.Condition `json:"if,omitempty"` diff --git a/patch.go b/patch.go index 7da9906..0eda6d7 100644 --- a/patch.go +++ b/patch.go @@ -105,9 +105,9 @@ func (p Patch[T]) String() string { case OpReplace: b.WriteString(fmt.Sprintf("Replace %s: %v -> %v", op.Path, op.Old, op.New)) case OpMove: - b.WriteString(fmt.Sprintf("Move %v to %s", op.Old, op.Path)) + b.WriteString(fmt.Sprintf("Move %s to %s", op.From, op.Path)) case OpCopy: - b.WriteString(fmt.Sprintf("Copy %v to %s", op.Old, op.Path)) + b.WriteString(fmt.Sprintf("Copy %s to %s", op.From, op.Path)) case OpLog: b.WriteString(fmt.Sprintf("Log %s: %v", op.Path, op.New)) } @@ -143,14 +143,24 @@ func (p Patch[T]) Reverse() Patch[T] { rev.New = op.Old case OpMove: rev.Kind = OpMove - // op.Old for Move was the fromPath string. - // To reverse, we move back from current Path to op.Old Path. - rev.Path = fmt.Sprintf("%v", op.Old) - rev.Old = op.Path + rev.Path = op.From + rev.From = op.Path + // If the destination had a displaced value at apply-time, restore + // it via Old; reversing the move strands that value otherwise. + if op.Old != nil { + rev.New = op.Old + } case OpCopy: - // Undoing a copy means removing the copied value at the target path - rev.Kind = OpRemove - rev.Old = op.New + // If we know the prior destination value, restore it with Replace; + // otherwise the destination was empty pre-copy so Remove suffices. + if op.Old != nil { + rev.Kind = OpReplace + rev.Old = op.New + rev.New = op.Old + } else { + rev.Kind = OpRemove + rev.Old = op.New + } } res.Operations = append(res.Operations, rev) } @@ -182,7 +192,7 @@ func (p Patch[T]) ToJSONPatch() ([]byte, error) { case OpAdd, OpReplace: m["value"] = op.New case OpMove, OpCopy: - m["from"] = op.Old + m["from"] = op.From case OpLog: m["value"] = op.New // log message } @@ -241,10 +251,14 @@ func ParseJSONPatch[T any](data []byte) (Patch[T], error) { op.New = m["value"] case "move": op.Kind = OpMove - op.Old = m["from"] + if s, ok := m["from"].(string); ok { + op.From = s + } case "copy": op.Kind = OpCopy - op.Old = m["from"] + if s, ok := m["from"].(string); ok { + op.From = s + } case "log": op.Kind = OpLog op.New = m["value"] @@ -301,13 +315,13 @@ func Remove[T, V any](p Path[T, V]) Op { // Move returns a type-safe move operation that relocates the value at from to to. // Both paths must share the same value type V. func Move[T, V any](from, to Path[T, V]) Op { - return Op{op: Operation{Kind: OpMove, Path: to.String(), Old: from.String()}} + return Op{op: Operation{Kind: OpMove, Path: to.String(), From: from.String()}} } // Copy returns a type-safe copy operation that duplicates the value at from to to. // Both paths must share the same value type V. func Copy[T, V any](from, to Path[T, V]) Op { - return Op{op: Operation{Kind: OpCopy, Path: to.String(), Old: from.String()}} + return Op{op: Operation{Kind: OpCopy, Path: to.String(), From: from.String()}} } // Builder constructs a [Patch] via a fluent chain. diff --git a/patch_test.go b/patch_test.go index 582548d..7b73ab9 100644 --- a/patch_test.go +++ b/patch_test.go @@ -103,8 +103,8 @@ func TestPatchUtilities(t *testing.T) { {Kind: deep.OpAdd, Path: "/a", New: 1}, {Kind: deep.OpRemove, Path: "/b", Old: 2}, {Kind: deep.OpReplace, Path: "/c", Old: 3, New: 4}, - {Kind: deep.OpMove, Path: "/d", Old: "/e"}, - {Kind: deep.OpCopy, Path: "/f", Old: "/g"}, + {Kind: deep.OpMove, Path: "/d", From: "/e"}, + {Kind: deep.OpCopy, Path: "/f", From: "/g"}, {Kind: deep.OpLog, Path: "/h", New: "msg"}, } @@ -162,8 +162,8 @@ func TestPatchReverseExhaustive(t *testing.T) { {Kind: deep.OpAdd, Path: "/a", New: 1}, {Kind: deep.OpRemove, Path: "/b", Old: 2}, {Kind: deep.OpReplace, Path: "/c", Old: 3, New: 4}, - {Kind: deep.OpMove, Path: "/d", Old: "/e"}, - {Kind: deep.OpCopy, Path: "/f", Old: "/g"}, + {Kind: deep.OpMove, Path: "/d", From: "/e"}, + {Kind: deep.OpCopy, Path: "/f", From: "/g"}, {Kind: deep.OpLog, Path: "/h", New: "msg"}, } @@ -185,6 +185,48 @@ func TestPatchReverseExhaustive(t *testing.T) { } } +// TestPatchReverseOpCopyWithPriorValue asserts that when an OpCopy carries the +// displaced destination value in Old, Reverse emits an OpReplace that restores +// that value rather than an OpRemove that strands it. +func TestPatchReverseOpCopyWithPriorValue(t *testing.T) { + p := deep.Patch[testmodels.User]{} + p.Operations = []deep.Operation{ + // Pre-copy /dst held "before"; copy overwrote it with "after". + {Kind: deep.OpCopy, Path: "/dst", From: "/src", Old: "before", New: "after"}, + } + rev := p.Reverse() + if len(rev.Operations) != 1 { + t.Fatalf("expected 1 reversed op, got %d", len(rev.Operations)) + } + got := rev.Operations[0] + if got.Kind != deep.OpReplace { + t.Errorf("reverse of OpCopy with prior value should be OpReplace, got %v", got.Kind) + } + if got.Path != "/dst" { + t.Errorf("reverse target path = %q, want /dst", got.Path) + } + if got.Old != "after" || got.New != "before" { + t.Errorf("reverse should restore prior value: got Old=%v New=%v, want Old=after New=before", got.Old, got.New) + } +} + +// TestPatchReverseOpMoveSymmetric asserts OpMove reverses by swapping From and +// Path, restoring the original location. +func TestPatchReverseOpMoveSymmetric(t *testing.T) { + p := deep.Patch[testmodels.User]{} + p.Operations = []deep.Operation{ + {Kind: deep.OpMove, Path: "/dst", From: "/src"}, + } + rev := p.Reverse() + if len(rev.Operations) != 1 { + t.Fatalf("expected 1 reversed op, got %d", len(rev.Operations)) + } + got := rev.Operations[0] + if got.Kind != deep.OpMove || got.Path != "/src" || got.From != "/dst" { + t.Errorf("reverse OpMove: got Path=%s From=%s, want Path=/src From=/dst", got.Path, got.From) + } +} + // TestPatchReverseOpLogOnly asserts that a patch containing only OpLog ops // reverses to an empty patch rather than a sequence of malformed OpAdds. func TestPatchReverseOpLogOnly(t *testing.T) { @@ -305,8 +347,8 @@ func TestBuilderMoveCopy(t *testing.T) { if len(p.Operations) != 1 || p.Operations[0].Kind != deep.OpMove { t.Error("Move not added correctly") } - if p.Operations[0].Old != aPath.String() || p.Operations[0].Path != bPath.String() { - t.Errorf("Move paths wrong: from=%v to=%v", p.Operations[0].Old, p.Operations[0].Path) + if p.Operations[0].From != aPath.String() || p.Operations[0].Path != bPath.String() { + t.Errorf("Move paths wrong: from=%v to=%v", p.Operations[0].From, p.Operations[0].Path) } p2 := deep.Edit(&S{}).With(deep.Copy(aPath, bPath)).Build() From 34833e9cf168b4919fc14bc6f0a73b1b30c54a2c Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 07:55:47 -0400 Subject: [PATCH 05/13] test(engine): assert OpMove/OpCopy from missing source surface errors TestEngineFailures previously called deep.Apply for both cases without checking the returned error, so a regression that silently no-op'd these ops would have gone undetected. Tighten the assertions and add coverage for the empty-From early-reject path added in the From/Old split. --- engine_test.go | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/engine_test.go b/engine_test.go index 9d8cbc0..9ec5ed5 100644 --- a/engine_test.go +++ b/engine_test.go @@ -158,15 +158,32 @@ func TestReflectionEngineAdvanced(t *testing.T) { 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", From: "/nonexistent"}} - deep.Apply(u, p1) + 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", From: "/nonexistent"}} - deep.Apply(u, p2) + 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 { From 4ba995d61a993b541923c0b8c199a7069181c020 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 08:52:14 -0400 Subject: [PATCH 06/13] fix(deep-gen): comma-ok the root-level strict check Old type assertion The generated applyOperation's root-level strict check used an unchecked op.Old.({{.TypeName}}) assertion. A strict OpReplace at "/" whose Old carried any other concrete type would panic rather than report a strict-check failure. Switch the assertion to comma-ok and report the strict-check error on either a type mismatch or a value mismatch. Regenerate all in-tree examples and add TestStrictRootMismatchedOldType to lock in non-panicking behavior. --- cmd/deep-gen/main.go | 3 ++- engine_test.go | 21 +++++++++++++++++++ examples/atomic_config/proxyconfig_deep.go | 6 ++++-- examples/audit_logging/user_deep.go | 3 ++- examples/concurrent_updates/stock_deep.go | 3 ++- examples/config_manager/config_deep.go | 3 ++- examples/http_patch_api/resource_deep.go | 3 ++- examples/json_interop/uistate_deep.go | 3 ++- examples/keyed_inventory/inventory_deep.go | 6 ++++-- examples/multi_error/strictuser_deep.go | 3 ++- examples/policy_engine/employee_deep.go | 3 ++- examples/state_management/docstate_deep.go | 3 ++- examples/struct_map_keys/fleet_deep.go | 3 ++- examples/three_way_merge/systemconfig_deep.go | 3 ++- examples/websocket_sync/gameworld_deep.go | 6 ++++-- internal/testmodels/user_deep.go | 6 ++++-- 16 files changed, 59 insertions(+), 19 deletions(-) diff --git a/cmd/deep-gen/main.go b/cmd/deep-gen/main.go index d0367ab..ca5c2d2 100644 --- a/cmd/deep-gen/main.go +++ b/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) } } diff --git a/engine_test.go b/engine_test.go index 9ec5ed5..6093613 100644 --- a/engine_test.go +++ b/engine_test.go @@ -155,6 +155,27 @@ 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{} diff --git a/examples/atomic_config/proxyconfig_deep.go b/examples/atomic_config/proxyconfig_deep.go index 851414f..04ff5b4 100644 --- a/examples/atomic_config/proxyconfig_deep.go +++ b/examples/atomic_config/proxyconfig_deep.go @@ -62,7 +62,8 @@ func (t *ProxyConfig) applyOperation(op deep.Operation, logger *slog.Logger) (bo switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(ProxyConfig)) { + old, ok := op.Old.(ProxyConfig) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -340,7 +341,8 @@ func (t *SystemMeta) applyOperation(op deep.Operation, logger *slog.Logger) (boo switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(SystemMeta)) { + old, ok := op.Old.(SystemMeta) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/audit_logging/user_deep.go b/examples/audit_logging/user_deep.go index e970ee4..ef8cafe 100644 --- a/examples/audit_logging/user_deep.go +++ b/examples/audit_logging/user_deep.go @@ -63,7 +63,8 @@ func (t *User) applyOperation(op deep.Operation, logger *slog.Logger) (bool, err switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(User)) { + old, ok := op.Old.(User) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/concurrent_updates/stock_deep.go b/examples/concurrent_updates/stock_deep.go index 44c6cf8..3255ea8 100644 --- a/examples/concurrent_updates/stock_deep.go +++ b/examples/concurrent_updates/stock_deep.go @@ -62,7 +62,8 @@ func (t *Stock) applyOperation(op deep.Operation, logger *slog.Logger) (bool, er switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Stock)) { + old, ok := op.Old.(Stock) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/config_manager/config_deep.go b/examples/config_manager/config_deep.go index 4fded8b..f184bcc 100644 --- a/examples/config_manager/config_deep.go +++ b/examples/config_manager/config_deep.go @@ -63,7 +63,8 @@ func (t *Config) applyOperation(op deep.Operation, logger *slog.Logger) (bool, e switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Config)) { + old, ok := op.Old.(Config) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/http_patch_api/resource_deep.go b/examples/http_patch_api/resource_deep.go index b8bee12..def6ec9 100644 --- a/examples/http_patch_api/resource_deep.go +++ b/examples/http_patch_api/resource_deep.go @@ -62,7 +62,8 @@ func (t *Resource) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Resource)) { + old, ok := op.Old.(Resource) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/json_interop/uistate_deep.go b/examples/json_interop/uistate_deep.go index b2751f6..3b7630f 100644 --- a/examples/json_interop/uistate_deep.go +++ b/examples/json_interop/uistate_deep.go @@ -62,7 +62,8 @@ func (t *UIState) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(UIState)) { + old, ok := op.Old.(UIState) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/keyed_inventory/inventory_deep.go b/examples/keyed_inventory/inventory_deep.go index dc0235a..43c8196 100644 --- a/examples/keyed_inventory/inventory_deep.go +++ b/examples/keyed_inventory/inventory_deep.go @@ -62,7 +62,8 @@ func (t *Item) applyOperation(op deep.Operation, logger *slog.Logger) (bool, err switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Item)) { + old, ok := op.Old.(Item) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -340,7 +341,8 @@ func (t *Inventory) applyOperation(op deep.Operation, logger *slog.Logger) (bool switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Inventory)) { + old, ok := op.Old.(Inventory) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/multi_error/strictuser_deep.go b/examples/multi_error/strictuser_deep.go index 6b1f594..7ecefbc 100644 --- a/examples/multi_error/strictuser_deep.go +++ b/examples/multi_error/strictuser_deep.go @@ -62,7 +62,8 @@ func (t *StrictUser) applyOperation(op deep.Operation, logger *slog.Logger) (boo switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(StrictUser)) { + old, ok := op.Old.(StrictUser) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/policy_engine/employee_deep.go b/examples/policy_engine/employee_deep.go index ce03ed8..0402e87 100644 --- a/examples/policy_engine/employee_deep.go +++ b/examples/policy_engine/employee_deep.go @@ -62,7 +62,8 @@ func (t *Employee) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Employee)) { + old, ok := op.Old.(Employee) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/state_management/docstate_deep.go b/examples/state_management/docstate_deep.go index 7e77670..2e4bf50 100644 --- a/examples/state_management/docstate_deep.go +++ b/examples/state_management/docstate_deep.go @@ -63,7 +63,8 @@ func (t *DocState) applyOperation(op deep.Operation, logger *slog.Logger) (bool, switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(DocState)) { + old, ok := op.Old.(DocState) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/struct_map_keys/fleet_deep.go b/examples/struct_map_keys/fleet_deep.go index 1b84ff2..eeeda5d 100644 --- a/examples/struct_map_keys/fleet_deep.go +++ b/examples/struct_map_keys/fleet_deep.go @@ -61,7 +61,8 @@ func (t *Fleet) applyOperation(op deep.Operation, logger *slog.Logger) (bool, er switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Fleet)) { + old, ok := op.Old.(Fleet) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/three_way_merge/systemconfig_deep.go b/examples/three_way_merge/systemconfig_deep.go index 9436776..0f37e86 100644 --- a/examples/three_way_merge/systemconfig_deep.go +++ b/examples/three_way_merge/systemconfig_deep.go @@ -63,7 +63,8 @@ func (t *SystemConfig) applyOperation(op deep.Operation, logger *slog.Logger) (b switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(SystemConfig)) { + old, ok := op.Old.(SystemConfig) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/examples/websocket_sync/gameworld_deep.go b/examples/websocket_sync/gameworld_deep.go index 87888ba..6a93595 100644 --- a/examples/websocket_sync/gameworld_deep.go +++ b/examples/websocket_sync/gameworld_deep.go @@ -63,7 +63,8 @@ func (t *GameWorld) applyOperation(op deep.Operation, logger *slog.Logger) (bool switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(GameWorld)) { + old, ok := op.Old.(GameWorld) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -345,7 +346,8 @@ func (t *Player) applyOperation(op deep.Operation, logger *slog.Logger) (bool, e switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Player)) { + old, ok := op.Old.(Player) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } diff --git a/internal/testmodels/user_deep.go b/internal/testmodels/user_deep.go index e4d1589..5db52d7 100644 --- a/internal/testmodels/user_deep.go +++ b/internal/testmodels/user_deep.go @@ -64,7 +64,8 @@ func (t *User) applyOperation(op deep.Operation, logger *slog.Logger) (bool, err switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(User)) { + old, ok := op.Old.(User) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } @@ -595,7 +596,8 @@ func (t *Detail) applyOperation(op deep.Operation, logger *slog.Logger) (bool, e switch op.Path { case "/": if op.Strict && (op.Kind == deep.OpReplace || op.Kind == deep.OpRemove) { - if !deep.Equal(*t, op.Old.(Detail)) { + old, ok := op.Old.(Detail) + if !ok || !deep.Equal(*t, old) { return true, fmt.Errorf("strict check failed at root: expected %v, got %v", op.Old, *t) } } From ba192af0bfeb77043ea043e9d8fdf492cbfe02eb Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 08:54:00 -0400 Subject: [PATCH 07/13] fix(engine): nil-check logger in ApplyOpReflectionValue The pointer-variant ApplyOpReflection has always replaced a nil logger with slog.Default. The value-variant ApplyOpReflectionValue did not, so any caller invoking it directly with a nil logger and an OpLog op would NPE on logger.Info. Apply the same nil-check at the top of the value variant. Add TestApplyOpReflectionValueNilLogger covering the OpLog+nil-logger path. --- internal/engine/apply_reflection.go | 3 +++ internal/engine/apply_reflection_test.go | 27 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 internal/engine/apply_reflection_test.go diff --git a/internal/engine/apply_reflection.go b/internal/engine/apply_reflection.go index 13b1fbe..c6aa12a 100644 --- a/internal/engine/apply_reflection.go +++ b/internal/engine/apply_reflection.go @@ -21,6 +21,9 @@ func ApplyOpReflection[T any](target *T, op Operation, logger *slog.Logger) erro // ApplyOpReflectionValue applies op to the already-reflected value v. func ApplyOpReflectionValue(v reflect.Value, op Operation, logger *slog.Logger) error { + if logger == nil { + logger = slog.Default() + } // Strict check. if op.Strict && (op.Kind == OpReplace || op.Kind == OpRemove) { current, err := icore.DeepPath(op.Path).Resolve(v) diff --git a/internal/engine/apply_reflection_test.go b/internal/engine/apply_reflection_test.go new file mode 100644 index 0000000..275101b --- /dev/null +++ b/internal/engine/apply_reflection_test.go @@ -0,0 +1,27 @@ +package engine + +import ( + "reflect" + "testing" +) + +// TestApplyOpReflectionValueNilLogger asserts the value-variant entry point +// doesn't panic when a caller passes a nil logger and an OpLog op forces a +// log call. The pointer variant has always nil-checked the logger; the value +// variant must match so external callers (including future generated code) +// don't NPE on logger.Info. +func TestApplyOpReflectionValueNilLogger(t *testing.T) { + type S struct{ A int } + s := S{A: 1} + v := reflect.ValueOf(&s).Elem() + + defer func() { + if r := recover(); r != nil { + t.Errorf("nil logger caused panic: %v", r) + } + }() + + if err := ApplyOpReflectionValue(v, Operation{Kind: OpLog, Path: "/", New: "msg"}, nil); err != nil { + t.Errorf("OpLog with nil logger should succeed silently, got err=%v", err) + } +} From 4af9110b8a3023064ba4f5ec0776913902092af8 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 08:54:55 -0400 Subject: [PATCH 08/13] fix(hlc): add Clock.SetLatest for mutex-guarded rehydration Clock.Latest is exported for serialisation, but direct field writes bypass the clock's internal mutex and race with concurrent Now/Update. The CRDT package's UnmarshalJSON assigned Latest directly, leaving the race detector to fail any test that decoded a CRDT while another goroutine was editing it. Add Clock.SetLatest, which takes the mutex before assigning, and route CRDT.UnmarshalJSON through it. Document Latest as exposed only for serialisation. Add a race-targeted test that exercises SetLatest alongside concurrent Now calls. --- crdt/crdt.go | 2 +- crdt/hlc/hlc.go | 14 ++++++++++++++ crdt/hlc/hlc_test.go | 19 +++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/crdt/crdt.go b/crdt/crdt.go index 9eea3b2..4360b7c 100644 --- a/crdt/crdt.go +++ b/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 } diff --git a/crdt/hlc/hlc.go b/crdt/hlc/hlc.go index 446c2c0..0e93b1b 100644 --- a/crdt/hlc/hlc.go +++ b/crdt/hlc/hlc.go @@ -59,6 +59,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,6 +82,15 @@ 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) diff --git a/crdt/hlc/hlc_test.go b/crdt/hlc/hlc_test.go index 64a427f..bcfd215 100644 --- a/crdt/hlc/hlc_test.go +++ b/crdt/hlc/hlc_test.go @@ -97,3 +97,22 @@ 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 +} From 8c4da4726d3e7a82153da9bcb0a6717dadbe9a4e Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 08:56:07 -0400 Subject: [PATCH 09/13] fix(hlc): panic on Reserve overflow instead of silently truncating Clock.Reserve took n as int and assigned int32(n) into the Logical counter, so a reservation that would push Logical past math.MaxInt32 silently wrapped around. The wrap is invisible at the call site but breaks HLC monotonicity and therefore causal ordering. Pre-check n against the remaining int32 headroom and panic if Reserve would overflow; also reject negative n. Both states are programmer errors and far less harmful as a panic than as a silent wraparound. Add tests covering both panics. --- crdt/hlc/hlc.go | 14 ++++++++++++++ crdt/hlc/hlc_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/crdt/hlc/hlc.go b/crdt/hlc/hlc.go index 0e93b1b..6223c10 100644 --- a/crdt/hlc/hlc.go +++ b/crdt/hlc/hlc.go @@ -15,6 +15,7 @@ package hlc import ( "fmt" + "math" "sync" "time" ) @@ -97,7 +98,16 @@ func (c *Clock) Now() HLC { } // 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() @@ -108,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 diff --git a/crdt/hlc/hlc_test.go b/crdt/hlc/hlc_test.go index bcfd215..fd226b6 100644 --- a/crdt/hlc/hlc_test.go +++ b/crdt/hlc/hlc_test.go @@ -1,6 +1,7 @@ package hlc import ( + "math" "testing" "time" ) @@ -116,3 +117,29 @@ func TestSetLatestSafeConcurrentWithNow(t *testing.T) { } <-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) +} From 2e3b09116cb2e11fad490ea62836bbcf9e980d49 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 08:57:18 -0400 Subject: [PATCH 10/13] perf(crdt): make Set.Len avoid the Items slice allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Set.Len delegated to len(s.Items()), which materialised the full deduplicated slice of live elements just to take its length. Inline the dedup against a single map and return its size instead — same result, no slice allocation. Test asserts Len agrees with len(Items) across a churn pattern that includes duplicates and tombstones. --- crdt/set.go | 13 ++++++++++++- crdt/set_test.go | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/crdt/set.go b/crdt/set.go index 6e7204f..3a7c549 100644 --- a/crdt/set.go +++ b/crdt/set.go @@ -86,8 +86,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. diff --git a/crdt/set_test.go b/crdt/set_test.go index 3793d50..eaee4cd 100644 --- a/crdt/set_test.go +++ b/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") From 68294a362b9d70613d72fae7c23e62fbe08780d6 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 09:10:13 -0400 Subject: [PATCH 11/13] fix(patch): only lift leading test-op into Guard during ParseJSONPatch Any {"op":"test","path":"/","if":...} entry in the JSON Patch document was consumed as the global Guard, so a user-authored second such entry silently overwrote whatever Guard the leading entry already produced. Restrict the heuristic to the document's first entry, matching what ToJSONPatch emits. Document the wire convention on ParseJSONPatch. Test asserts a trailing same-shape entry leaves Guard untouched. --- patch.go | 20 ++++++++++++++++---- patch_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/patch.go b/patch.go index 0eda6d7..9e9ead9 100644 --- a/patch.go +++ b/patch.go @@ -212,22 +212,34 @@ func (p Patch[T]) ToJSONPatch() ([]byte, error) { // ParseJSONPatch parses a JSON Patch document (RFC 6902 plus deep extensions) // back into a Patch[T]. This is the inverse of Patch.ToJSONPatch(). +// +// Wire convention: a leading {"op":"test","path":"/","if":} entry +// is interpreted as the global Patch.Guard rather than a regular test op. +// This mirrors what ToJSONPatch emits; user-authored documents that happen +// to start with that exact triple will have it lifted into Guard. A test op +// on "/" without an "if" key is preserved as a regular operation (it has no +// special meaning in this format), and any subsequent test op is treated +// normally. To round-trip a regular test op at "/", attach it via Builder +// rather than serialising it as the document's first entry. func ParseJSONPatch[T any](data []byte) (Patch[T], error) { var ops []map[string]any if err := json.Unmarshal(data, &ops); err != nil { return Patch[T]{}, fmt.Errorf("ParseJSONPatch: %w", err) } res := Patch[T]{} - for _, m := range ops { + for i, m := range ops { opStr, _ := m["op"].(string) path, _ := m["path"].(string) - // Global condition is encoded as a test op on "/" with an "if" predicate. - if opStr == "test" && path == "/" { + // Global condition encoding: ONLY the leading entry (matched as + // op==test, path=="/", "if" present) is lifted into Guard. A later + // test op with the same shape is kept as a regular operation so a + // document with multiple "/" tests round-trips faithfully. + if i == 0 && opStr == "test" && path == "/" { if ifPred, ok := m["if"].(map[string]any); ok { res.Guard = condition.FromPredicate(ifPred) + continue } - continue } op := Operation{Path: path} diff --git a/patch_test.go b/patch_test.go index 7b73ab9..09d71cd 100644 --- a/patch_test.go +++ b/patch_test.go @@ -268,6 +268,35 @@ func TestPatchIsEmpty(t *testing.T) { } } +// TestParseJSONPatchGuardOnlyLeading asserts the Guard-extraction heuristic +// only fires on the leading entry. A later {"op":"test","path":"/","if":...} +// must not overwrite Guard or be re-interpreted as a second guard; deep does +// not model standalone test ops, so trailing tests are dropped as unknown. +func TestParseJSONPatchGuardOnlyLeading(t *testing.T) { + raw := []byte(`[ + {"op":"test","path":"/","if":{"op":"more","path":"/age","value":18}}, + {"op":"replace","path":"/name","value":"Alice"}, + {"op":"test","path":"/","if":{"op":"more","path":"/age","value":99}} + ]`) + type Doc struct { + Name string `json:"name"` + Age int `json:"age"` + } + p, err := deep.ParseJSONPatch[Doc](raw) + if err != nil { + t.Fatalf("ParseJSONPatch: %v", err) + } + if p.Guard == nil { + t.Fatal("expected leading test op to be lifted into Guard") + } + // The leading test specified Gt(/age, 18); the trailing one carries + // Gt(/age, 99). If the trailing entry were also lifted into Guard, the + // 99 value would clobber the 18. + if gv, ok := p.Guard.Value.(float64); !ok || gv != 18 { + t.Errorf("Guard value should remain 18 from leading test, got %v", p.Guard.Value) + } +} + func TestParseJSONPatchRoundTrip(t *testing.T) { type Doc struct { Name string `json:"name"` From 4759402d1b3b898fe890f9bb47ddabe9e4043be9 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 09:18:32 -0400 Subject: [PATCH 12/13] fix(condition): error on Not condition with empty Sub A Not condition with no Sub fell through the early-return branch and continued into the path-based comparison logic, which would silently mis-resolve the empty Path or produce a confusing error from a downstream operator. Convert the malformed-input case into an explicit error at the top of the Not branch so callers see a clear diagnostic. --- condition/condition.go | 13 +++++++------ condition/condition_test.go | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/condition/condition.go b/condition/condition.go index 78f98a8..312b64e 100644 --- a/condition/condition.go +++ b/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) diff --git a/condition/condition_test.go b/condition/condition_test.go index e17280b..b6b2185 100644 --- a/condition/condition_test.go +++ b/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) + } +} From 13fd7a0967cfe99d7146f9d452cbc49508f07628 Mon Sep 17 00:00:00 2001 From: Bruno Albuquerque Date: Mon, 1 Jun 2026 09:28:42 -0400 Subject: [PATCH 13/13] docs(crdt): explain Set.Add's two-clock-tick cost Investigating the review note about Set.Add advancing the HLC twice (once for the tag, once inside Edit for the Delta timestamp) showed neither order produces a single-tick path: Edit always ticks for any non-empty patch, and Add always produces a non-empty patch. The duplicate tick is harmless under the HLC mutex. Drop the open question by documenting the design on Add so future readers don't re-spend the analysis budget. --- crdt/set.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crdt/set.go b/crdt/set.go index 3a7c549..a289a13 100644 --- a/crdt/set.go +++ b/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]) {