From aba3b5e9ebe4275d3bd49e2edae5b32d5b84b48b Mon Sep 17 00:00:00 2001 From: Anthony James Padavano Date: Wed, 28 Oct 2026 03:25:30 -0400 Subject: [PATCH 1/3] fix(container): enforce acyclic, single-parent relationships --- src/rules_system_bound/container.py | 55 ++++++++++++-- tests/test_container_relationships.py | 100 ++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 7 deletions(-) create mode 100644 tests/test_container_relationships.py diff --git a/src/rules_system_bound/container.py b/src/rules_system_bound/container.py index 5718d44..b4e0bd8 100644 --- a/src/rules_system_bound/container.py +++ b/src/rules_system_bound/container.py @@ -39,23 +39,64 @@ 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. + + Re-adding the same child object is a no-op, never a duplicate. + + 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 + if any(existing is child for existing in self.children): + return + 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) 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.""" diff --git a/tests/test_container_relationships.py b/tests/test_container_relationships.py new file mode 100644 index 0000000..6a53dde --- /dev/null +++ b/tests/test_container_relationships.py @@ -0,0 +1,100 @@ +"""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_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] From b051a98698f7c9f27eb07ef98ad9cd9cf43a5dc9 Mon Sep 17 00:00:00 2001 From: Anthony James Padavano Date: Wed, 28 Oct 2026 07:19:40 -0400 Subject: [PATCH 2/3] fix(container): reconcile parent link on re-add of an existing child (CodeRabbit review on #19) --- src/rules_system_bound/container.py | 11 +++++++---- tests/test_container_relationships.py | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/rules_system_bound/container.py b/src/rules_system_bound/container.py index b4e0bd8..0c8883a 100644 --- a/src/rules_system_bound/container.py +++ b/src/rules_system_bound/container.py @@ -58,7 +58,11 @@ def add_child(self, child: Container) -> None: since the child is then an ancestor of this container), raising a deterministic ``ValueError`` with both trees unchanged. - Re-adding the same child object is a no-op, never a duplicate. + 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 @@ -75,13 +79,12 @@ def add_child(self, child: Container) -> None: if current is child: raise ValueError("adding an ancestor as a child would create a cycle") current = current.parent - if any(existing is child for existing in self.children): - return 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. diff --git a/tests/test_container_relationships.py b/tests/test_container_relationships.py index 6a53dde..2208957 100644 --- a/tests/test_container_relationships.py +++ b/tests/test_container_relationships.py @@ -88,6 +88,31 @@ def test_envelops_uses_identity_not_dataclass_equality(): 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_normal_lineage_and_descendants_with_unrelated_unchanged(): root = make("root") child1, child2 = make("c1"), make("c2") From b7f8ab34f7f5aaddcbf07de1173ba759acc6036a Mon Sep 17 00:00:00 2001 From: Anthony James Padavano Date: Wed, 28 Oct 2026 07:27:49 -0400 Subject: [PATCH 3/3] fix(container): reconcile parent link on re-add of an existing child (CodeRabbit review on #19) --- src/rules_system_bound/container.py | 22 +++++++++++++++++++++- tests/test_container_relationships.py | 15 +++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/rules_system_bound/container.py b/src/rules_system_bound/container.py index 0c8883a..4ebd317 100644 --- a/src/rules_system_bound/container.py +++ b/src/rules_system_bound/container.py @@ -56,7 +56,13 @@ def add_child(self, child: Container) -> None: 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. + 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 @@ -79,6 +85,20 @@ def add_child(self, child: Container) -> 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] diff --git a/tests/test_container_relationships.py b/tests/test_container_relationships.py index 2208957..494c6cc 100644 --- a/tests/test_container_relationships.py +++ b/tests/test_container_relationships.py @@ -113,6 +113,21 @@ def test_readd_via_direct_list_detaches_old_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")