fix(container): enforce acyclic, single-parent relationships

src/rules_system_bound/container.py

@@ -39,23 +39,87 @@ def can_contain(self, other: Container, rules: ContainmentRules) -> bool:
         return rules.check_containment(self, other)
 
     def envelops(self, other: Container) -> bool:
-        """Check if this container envelops another."""
-        return other in self.children
+        """Check if this container envelops another.
+
+        Membership is by object identity, not dataclass equality: a
+        distinct but equal-valued container is not enveloped here.
+        """
+        return any(existing is other for existing in self.children)
 
     def enveloped_by(self) -> Optional[Container]:
         """Get the container that envelops this one."""
         return self.parent
 
     def add_child(self, child: Container) -> None:
-        """Add a child container."""
+        """Add a child container.
+
+        Rejects, before any mutation, adding a container to itself or
+        to one of its own descendants (either would create a cycle,
+        since the child is then an ancestor of this container), raising
+        a deterministic ``ValueError`` with both trees unchanged. The
+        check covers both directions: the parent-pointer chain above
+        this container, and the children graph below the candidate
+        child — a directly-supplied children list can claim a
+        descendant relationship the parent pointers do not show, and
+        adding it back would close a cycle in the children lists even
+        though the pointers look acyclic.
+
+        Re-adding the same child object is a no-op, never a duplicate —
+        except that it still reconciles the child's parent pointer: a
+        children list supplied directly at construction can leave the
+        pointer unset or pointing elsewhere, so the pointer is set to
+        this container even when the child is already listed here.
+
+        Moving a child from another parent atomically reparents it:
+        it is detached from the previous parent's children list and
+        attached here, so the child has exactly one parent and appears
+        in exactly one parent's children list afterwards. This is the
+        explicit reparenting behaviour; rejection without mutation is
+        not used, so existing ``add_child`` callers that move a child
+        keep working with a consistent tree instead of a stale one.
+        """
+        if child is self:
+            raise ValueError("a container cannot be its own child")
+        current = self.parent
+        while current is not None:
+            if current is child:
+                raise ValueError("adding an ancestor as a child would create a cycle")
+            current = current.parent
+        # Children-graph direction: reject if this container is reachable
+        # from the candidate child's children lists. The seen set keeps
+        # the walk terminating even on an already-cyclic supplied graph.
+        pending = [child]
+        seen: set[int] = set()
+        while pending:
+            node = pending.pop()
+            if node is self:
+                raise ValueError("adding an ancestor as a child would create a cycle")
+            marker = id(node)
+            if marker in seen:
+                continue
+            seen.add(marker)
+            pending.extend(node.children)
+        old_parent = child.parent
+        if old_parent is not None and old_parent is not self:
+            old_parent.children = [e for e in old_parent.children if e is not child]
         child.parent = self
-        self.children.append(child)
+        if not any(existing is child for existing in self.children):
+            self.children.append(child)
 
     def remove_child(self, child: Container) -> None:
-        """Remove a child container."""
-        if child in self.children:
-            self.children.remove(child)
-            child.parent = None
+        """Remove a child container.
+
+        Removal is by object identity, not dataclass equality: removing
+        a distinct but equal-valued container must not detach the real
+        child, and must not clear the real child's parent pointer.
+        Removing a container that is not a child here is a no-op.
+        """
+        for index, existing in enumerate(self.children):
+            if existing is child:
+                del self.children[index]
+                if child.parent is self:
+                    child.parent = None
+                return
 
     def get_lineage(self) -> list[Container]:
         """Get the full lineage from this container to the top."""

tests/test_container_relationships.py

@@ -0,0 +1,140 @@
+"""Focused regressions for acyclic, single-parent container relationships.
+
+Covers rules-system-bound#17: self-parenting and ancestor cycles must be
+rejected before mutation, re-adding must not duplicate, reparenting must
+be atomic, and removal/membership must use object identity.
+"""
+
+import pytest
+from rules_system_bound.container import Container
+
+
+def make(identifier):
+    return Container(id=identifier, function="f", purpose="p")
+
+
+def test_self_parenting_rejected_and_state_unchanged():
+    container = make("a")
+    with pytest.raises(ValueError):
+        container.add_child(container)
+    assert container.parent is None
+    assert container.children == []
+
+
+def test_ancestor_cycle_rejected_and_state_unchanged():
+    root, mid, leaf = make("root"), make("mid"), make("leaf")
+    root.add_child(mid)
+    mid.add_child(leaf)
+    with pytest.raises(ValueError):
+        leaf.add_child(root)
+    assert root.parent is None
+    assert root.children == [mid]
+    assert mid.children == [leaf]
+    assert leaf.children == []
+    assert leaf.get_lineage() == [leaf, mid, root]
+
+
+def test_readding_same_child_does_not_duplicate():
+    parent, child = make("p"), make("ch")
+    parent.add_child(child)
+    parent.add_child(child)
+    assert parent.children == [child]
+    assert child.parent is parent
+
+
+def test_reparenting_moves_atomically():
+    old_parent, new_parent, child = make("p1"), make("p2"), make("ch")
+    old_parent.add_child(child)
+    new_parent.add_child(child)
+    assert old_parent.children == []
+    assert new_parent.children == [child]
+    assert child.parent is new_parent
+    assert child.get_lineage() == [child, new_parent]
+
+
+def test_descendant_promoted_to_direct_child_without_cycle():
+    root, mid, leaf = make("root"), make("mid"), make("leaf")
+    root.add_child(mid)
+    mid.add_child(leaf)
+    root.add_child(leaf)
+    assert mid.children == []
+    assert root.children == [mid, leaf]
+    assert leaf.parent is root
+    assert leaf.get_lineage() == [leaf, root]
+
+
+def test_remove_uses_identity_not_dataclass_equality():
+    parent, child = make("p"), make("ch")
+    parent.add_child(child)
+    twin = Container(id="ch", function="f", purpose="p")
+    twin.parent = parent
+    assert twin == child
+    parent.remove_child(twin)
+    assert parent.children == [child]
+    assert child.parent is parent
+    assert twin.parent is parent
+    parent.remove_child(child)
+    assert parent.children == []
+    assert child.parent is None
+
+
+def test_envelops_uses_identity_not_dataclass_equality():
+    parent, child = make("p"), make("ch")
+    parent.add_child(child)
+    twin = Container(id="ch", function="f", purpose="p")
+    twin.parent = parent
+    assert twin == child
+    assert parent.envelops(child)
+    assert not parent.envelops(twin)
+
+
+def test_readd_reconciles_parent_when_children_list_supplied_directly():
+    # CodeRabbit review on PR #19: a directly-constructed children list
+    # can leave child.parent unset while membership claims otherwise.
+    child = make("ch")
+    parent = Container(id="p", function="f", purpose="p", children=[child])
+    assert child.parent is None
+    parent.add_child(child)
+    assert parent.children == [child]
+    assert child.parent is parent
+    assert child.get_lineage() == [child, parent]
+
+
+def test_readd_via_direct_list_detaches_old_parent():
+    # Same construction path, but the pointer names another parent that
+    # still lists the child: re-adding must move it, not duplicate it.
+    old_parent, child = make("p1"), make("ch")
+    old_parent.add_child(child)
+    new_parent = Container(id="p2", function="f", purpose="p", children=[child])
+    new_parent.add_child(child)
+    assert old_parent.children == []
+    assert new_parent.children == [child]
+    assert child.parent is new_parent
+    assert child.get_lineage() == [child, new_parent]
+
+
+def test_children_graph_cycle_rejected_and_state_unchanged():
+    # Second CodeRabbit review on PR #19: a directly-constructed list
+    # (parent lists child, child.parent unset) must not let
+    # child.add_child(parent) close a cycle in the children lists.
+    child = make("ch")
+    parent = Container(id="p", function="f", purpose="p", children=[child])
+    with pytest.raises(ValueError):
+        child.add_child(parent)
+    assert parent.parent is None
+    assert parent.children == [child]
+    assert child.parent is None
+    assert child.children == []
+    assert parent.get_descendants() == [child]
+
+
+def test_normal_lineage_and_descendants_with_unrelated_unchanged():
+    root = make("root")
+    child1, child2 = make("c1"), make("c2")
+    grandchild = make("gc")
+    root.add_child(child1)
+    root.add_child(child2)
+    child1.add_child(grandchild)
+    assert grandchild.get_lineage() == [grandchild, child1, root]
+    assert root.get_descendants() == [child1, grandchild, child2]
+    assert child2.get_lineage() == [child2, root]