mutation fixes

pkg/cart/cart-mutation-helper.go

@@ -73,6 +73,33 @@ func (c *CartMutationContext) ReleaseItem(ctx context.Context, cartId CartId, sk
 	return c.reservationService.Release(ctx, inventory.SKU(sku), l, inventory.CartID(cartId.String()))
 }
 
+// equalOptional is the canonical nullable-equality primitive used by the
+// cart mutations when deciding whether two identifier fields (parent/child
+// links, store ids, anything else nullable) refer to the same thing.
+//
+// Returns true when both pointers are nil, when both alias the same
+// address, or when both are non-nil and dereference to equal values. The
+// same-address short-circuit is a no-op for correctness but lets a caller
+// who passes the same variable get back true without traversing the
+// generic's comparison path.
+//
+// One tested primitive covers every *T the cart currently compares —
+// `*uint32` for ParentId, `*string` for StoreId — and any future
+// comparable pointer type. Hand-rolled "both nil OR both non-nil and
+// equal" expressions were duplicated at AddItem's merge site and were a
+// footgun whenever a new field was added (which axis of the predicate
+// was the relevant one?). Keeping a single helper makes future merges
+// safer to copy.
+func equalOptional[T comparable](a, b *T) bool {
+	if a == b { // both nil OR same address
+		return true
+	}
+	if a == nil || b == nil {
+		return false
+	}
+	return *a == *b
+}
+
 // CascadeChildQuantities scales every direct child's quantity
 // proportionally when a parent's quantity is bumped (ChangeQuantity or
 // the AddItem-merge path).

pkg/cart/cart-mutation-helper_test.go

@@ -0,0 +1,68 @@
+package cart
+
+import "testing"
+
+// equalOptional is the nullable-equality primitive shared by the AddItem
+// merge site and any future field-comparison. Two call sites use it today
+// with two distinct types — *uint32 (ParentId) and *string (StoreId) — so
+// the test pins both via generic instantiation.
+func TestEqualOptional(t *testing.T) {
+	t.Run("both nil is equal", func(t *testing.T) {
+		if !equalOptional[string](nil, nil) {
+			t.Error("both nil should be equal")
+		}
+		if !equalOptional[uint32](nil, nil) {
+			t.Error("both nil should be equal (uint32)")
+		}
+	})
+
+	t.Run("same address alias is equal", func(t *testing.T) {
+		s := "x"
+		if !equalOptional(&s, &s) {
+			t.Error("same-address *string should be equal")
+		}
+		v := uint32(5)
+		if !equalOptional(&v, &v) {
+			t.Error("same-address *uint32 should be equal")
+		}
+	})
+
+	t.Run("distinct addresses with equal values are equal", func(t *testing.T) {
+		a, b := "x", "x"
+		if !equalOptional(&a, &b) {
+			t.Error("equal strings at distinct addresses should be equal")
+		}
+		u, w := uint32(5), uint32(5)
+		if !equalOptional(&u, &w) {
+			t.Error("equal uint32 at distinct addresses should be equal")
+		}
+	})
+
+	t.Run("one nil, one set is not equal", func(t *testing.T) {
+		a := "x"
+		if equalOptional(&a, nil) {
+			t.Error("non-nil vs nil should differ (string)")
+		}
+		if equalOptional[string](nil, &a) {
+			t.Error("nil vs non-nil should differ (string)")
+		}
+		u := uint32(5)
+		if equalOptional(&u, nil) {
+			t.Error("non-nil vs nil should differ (uint32)")
+		}
+		if equalOptional[uint32](nil, &u) {
+			t.Error("nil vs non-nil should differ (uint32)")
+		}
+	})
+
+	t.Run("differing values are not equal", func(t *testing.T) {
+		a, b := "x", "y"
+		if equalOptional(&a, &b) {
+			t.Error("differing strings should differ")
+		}
+		u, w := uint32(5), uint32(6)
+		if equalOptional(&u, &w) {
+			t.Error("differing uint32 should differ")
+		}
+	})
+}

pkg/cart/mutation_add_item_test.go

@@ -68,3 +68,125 @@ func TestAddItem_MergeCascadesToChildren(t *testing.T) {
 		}
 	}
 }
+
+// Item identity for merge purposes includes ParentId: a standalone root
+// line (ParentId == nil) and a child line (ParentId == &someParentLine) that
+// happen to share an ItemId are different roles and MUST stay as distinct
+// lines. Merging would silently turn an accessory into a +N on the root
+// (or vice versa), corrupting the parent-child link.
+func TestAddItem_DoesNotMergeWhenParentIdDiffers(t *testing.T) {
+	reg := NewCartMultationRegistry(NewCartMutationContext(nil))
+	g := NewCartGrain(1, time.Now())
+	ctx := context.Background()
+
+	// Standalone root (no ParentId).
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P", Quantity: 1, Price: 1000}); err != nil {
+		t.Fatalf("add standalone: %v", err)
+	}
+	parentLine := g.Items[0].Id
+
+	// Same ItemId+StoreId but with ParentId set — must create a NEW line,
+	// not bump the standalone's qty.
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P", Quantity: 1, Price: 1000, ParentId: &parentLine}); err != nil {
+		t.Fatalf("add child-form: %v", err)
+	}
+
+	if len(g.Items) != 2 {
+		t.Fatalf("items = %d, want 2 (different ParentId => distinct lines)", len(g.Items))
+	}
+	// Standalone untouched at qty 1; child-form is its own line at qty 1.
+	for _, it := range g.Items {
+		if it.Quantity != 1 {
+			t.Errorf("line %d (parentId=%v) qty = %d, want 1 (no merge)", it.Id, it.ParentId, it.Quantity)
+		}
+		// One of them has nil ParentId and the other has &parentLine — guard
+		// against an accidental merge ever flipping both to non-nil.
+	}
+	var seenNil, seenSet bool
+	for _, it := range g.Items {
+		if it.ParentId == nil {
+			seenNil = true
+		} else if *it.ParentId == parentLine {
+			seenSet = true
+		}
+	}
+	if !seenNil || !seenSet {
+		t.Errorf("expected one line with nil ParentId and one with &parentLine; got nil=%v set=%v", seenNil, seenSet)
+	}
+}
+
+// Positive control: when BOTH sides have ParentId set to the SAME parent
+// line, the merge proceeds (this guards the new sameParent guard against
+// accidentally rejecting a legitimate "re-add same child of same parent"
+// case — e.g. an idempotent retry from a flaky request).
+func TestAddItem_MergesWhenBothParentIdMatch(t *testing.T) {
+	reg := NewCartMultationRegistry(NewCartMutationContext(nil))
+	g := NewCartGrain(1, time.Now())
+	ctx := context.Background()
+
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P", Quantity: 1, Price: 1000}); err != nil {
+		t.Fatalf("add parent: %v", err)
+	}
+	parentLine := g.Items[0].Id
+
+	// First child: ItemId 200 under parent.
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 200, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentLine}); err != nil {
+		t.Fatalf("add child: %v", err)
+	}
+	// Re-add the SAME child (same ItemId+StoreId+ParentId) — must merge into
+	// qty 2, not create a second accessory line.
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 200, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentLine}); err != nil {
+		t.Fatalf("re-add child: %v", err)
+	}
+
+	if len(g.Items) != 2 {
+		t.Fatalf("items = %d, want 2 (parent + merged child)", len(g.Items))
+	}
+	for _, it := range g.Items {
+		switch it.Id {
+		case parentLine:
+			if it.Quantity != 1 {
+				t.Errorf("parent qty = %d, want 1 (untouched)", it.Quantity)
+			}
+		default:
+			if it.Quantity != 2 {
+				t.Errorf("child qty = %d, want 2 (merged on same-PARENT re-add)", it.Quantity)
+			}
+		}
+	}
+}
+
+// Same ItemId+StoreId+both with ParentId set to *different* parent lines
+// must also stay distinct — children belong to their own parent.
+func TestAddItem_DoesNotMergeAcrossDifferentParentLines(t *testing.T) {
+	reg := NewCartMultationRegistry(NewCartMutationContext(nil))
+	g := NewCartGrain(1, time.Now())
+	ctx := context.Background()
+
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 100, Sku: "P1", Quantity: 1, Price: 500}); err != nil {
+		t.Fatalf("add parent A: %v", err)
+	}
+	parentA := g.Items[0].Id
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 200, Sku: "P2", Quantity: 1, Price: 500}); err != nil {
+		t.Fatalf("add parent B: %v", err)
+	}
+	parentB := g.Items[1].Id
+
+	// Two children of different parents sharing an ItemId — distinct lines.
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 999, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentA}); err != nil {
+		t.Fatalf("add child of A: %v", err)
+	}
+	if _, err := reg.Apply(ctx, g, &cart_messages.AddItem{ItemId: 999, Sku: "C", Quantity: 1, Price: 100, ParentId: &parentB}); err != nil {
+		t.Fatalf("add child of B: %v", err)
+	}
+
+	// 2 parent lines + 2 child lines = 4 total; no merges across.
+	if len(g.Items) != 4 {
+		t.Errorf("items = %d, want 4 (parents A,B + their distinct children)", len(g.Items))
+	}
+	for _, it := range g.Items {
+		if it.Quantity != 1 {
+			t.Errorf("line %d qty = %d, want 1 (no accidental merge)", it.Id, it.Quantity)
+		}
+	}
+}

pkg/cart/mutation_items.go

@@ -87,15 +87,25 @@ func (c *CartMutationContext) AddItem(g *CartGrain, m *cart_messages.AddItem) er
 		return fmt.Errorf("AddItem: invalid quantity %d", m.Quantity)
 	}
 
-	// Merge with any existing item having the same item id and matching StoreId
-	// (including both nil). Identity is the id; SKU is reference-only.
+	// Merge with any existing item having the same item id, matching
+	// StoreId, and matching ParentId (all three including both nil).
+	// Identity is ItemId; SKU is reference-only.
+	//
+	// ParentId MUST match: a standalone root line (ParentId == nil) and
+	// a child line (ParentId == &someParentLine) that happen to share an
+	// ItemId are different roles in the cart, even though the catalog
+	// item is the same. Merging them would silently convert an accessory
+	// into a +N on the root (or vice versa), corrupting the parent-child
+	// link. Both checks are delegated to equalOptional so the nullable-
+	// equality idiom stays in one place (cart-mutation-helper.go).
 	for _, existing := range g.Items {
 		if existing.ItemId != m.ItemId {
 			continue
 		}
-		sameStore := (existing.StoreId == nil && m.StoreId == nil) ||
-			(existing.StoreId != nil && m.StoreId != nil && *existing.StoreId == *m.StoreId)
-		if !sameStore {
+		if !equalOptional(existing.StoreId, m.StoreId) {
+			continue
+		}
+		if !equalOptional(existing.ParentId, m.ParentId) {
 			continue
 		}
 		if g.Type == cart_messages.CartType_REGULAR && c.UseReservations(existing) {