fix: reuse an existing model property, and refuse a child that shadows an ancestor (#69)

* fix: reuse an existing model property, and refuse a child that shadows an ancestor

Two defects reported against 0.21.0's DeviceTreeBuilder, both silent.

lines later, added the PROPERTY unconditionally. GroupedPropertyDict
.add_property is a wholesale `self._properties[id] = property`, so a
producer handing over a model it had already populated got that property
swapped for a fresh one. The value was the least of it: _change_callbacks,
_set_callbacks and _entity_setter are instance state on the replaced
object, so a producer that wired inbound control lost the actuator while
$description kept advertising settable: true, and an arriving /set did
nothing.

Nor did it self-heal. The builder path seeds only a static initial_value,
and Property.set_value fires callbacks only on an actual change, so a
value written once at group creation never republished.

This is the exact case DeviceTreeBuilder documents as its reason for
accepting a model rather than creating one, which made it a documented
guarantee the code did not provide. An existing property is now reused,
and the builder records only properties it actually created, so remove()
deletes what it added and leaves what the producer owned. A spec whose
python type disagrees with the property already there raises rather than
binding a Homie twin to a mismatched observable.

no check, so a child carrying an ancestor's id made that ancestor name
itself in its own children and put two devices on the same topics, with
no exception and no warning. The obvious way to reach it was expressing
"capabilities on the root" as a DeviceSpec with parent=None and the
root's own id, which is a real thing to want and which the builder does
not yet support. Failing loudly beats materializing a malformed tree.
Ids still only need to be unique within a tree's ancestry.

Closes #66

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: refuse two children of one parent sharing an id

The same defect as the ancestor guard, one step sideways, and equally
silent. Two children of one parent with the same id derive the same base
topic, so their $description publishes overwrite each other on the broker
and whichever wrote last defines the device; the parent meanwhile names
that child twice in its own `children` list, which is malformed.

Observed before the guard:

    children_ids()        : ['circuit-1', 'circuit-1']
    a's nodes             : ['meter']
    b's nodes             : ['switch']
    ...both retained at   : ebus/5/circuit-1/$description

Costs no new state: the parent already tracks its children, so this is a
scan of parent.children() on a construction that already walks the
ancestor chain. delete() detaches a child, so recreating one after
deleting it is not a false positive, and DeviceTreeBuilder.add() already
handles re-fired lifecycles at its own level.

Refs #67

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: dcj

CHANGELOG.md

@@ -4,14 +4,20 @@ All notable changes to `ebus-sdk` are recorded here. Format follows [Keep a Chan
 
 ## [Unreleased]
 
-### Documentation
-
-- `DeviceTreeBuilder` now states two parts of its contract that the API alone did not convey, both reported by a consumer reconciling an existing multi-device builder against it. First, whether a producer is expected to *adapt its own model* to `GroupedPropertyDict` or to *own one*: it is the second, which is the observable-model pattern the proxy guide prescribes, and the builder accepts rather than creates one so a single model can span a tree and so a producer holding one already can hand it over. Second, the limit of `add()`'s ordering: it orders late-bound ids and builds an unbuilt parent it was handed, but `DeviceSpec` is frozen and `parent` is a direct reference, so a child spec cannot be constructed before its parent spec exists. A caller deriving specs from a source that names parents indirectly still owns that dependency ordering. `add()` reads as though ordering is handled generally; it is handled for ids. Thanks to [@cayossarian](https://github.com/cayossarian) ([#49](https://github.com/electrification-bus/python-sdk/issues/49)).
-
 ### Added
 
 - `node_id` on `build_from_declarations` and `DeviceTreeBuilder`: a callable mapping a capability to the Homie node id it materializes onto, defaulting to the capability itself. The node id was hardcoded to the capability name, which is right until one device carries two instances of the same capability (two lugs, two meters), at which point the second silently lands on the first one's node. `node_type` and `node_name` were already callables, so the id was the one part of a node a caller could not choose. Renaming is all it does: the declaration's vocabulary stays `capability`, the model group still comes from the spec, and the returned map is still keyed by the declared capability, so a caller who ignores it sees no change. Pairs with `PropertySpec.model_group` from 0.21.0, which separates the same two instances in the model the way this separates them on the wire; using one without the other moves the collision rather than removing it. ([#47](https://github.com/electrification-bus/python-sdk/issues/47))
 
+### Fixed
+
+- `build_from_declarations` and `DeviceTreeBuilder` now REUSE an observable property the model already holds instead of replacing it. `_materialize` guarded the model group with `has_group` and then, two lines later, added the property unconditionally, and `GroupedPropertyDict.add_property` is a wholesale `self._properties[property_id] = property`. So a producer handing over a model it had already populated got that property swapped for a fresh one, losing its value and, worse, every callback and `entity_setter` attached to it: `$description` kept advertising `settable: true` while the actuator behind it was gone, and an arriving `/set` did nothing. Nor was it self-healing, since the builder path seeds only a static `initial_value` and `Property.set_value` fires callbacks only on an actual change, so a value written once at group creation never republished. This is the exact case `DeviceTreeBuilder` documents as the reason it accepts a model rather than creating one, which made the gap a documented guarantee the code did not provide. The builder now records only properties it actually created, so removing a device deletes what it added and leaves what the producer owned. A spec whose python type disagrees with the property already in the model raises rather than binding a Homie twin to a mismatched observable. ([#66](https://github.com/electrification-bus/python-sdk/issues/66))
+
+- `Device` refuses a child whose id collides with any ancestor's. `Device.__init__` appended to `parent._children` with no check, so a child carrying the root's id made the root name itself in its own `children` and put two devices on the same topics, with no exception and no warning. The obvious way to reach it was trying to express "capabilities on the root" as a `DeviceSpec` with `parent=None` and the root's own id, which is a real thing to want and which the builder does not yet support; failing loudly beats materializing a malformed tree. Ids still only have to be unique within a tree's ancestry, so the same id under a different root is unaffected. The same defect one step sideways is refused too: two children of one parent sharing an id derived the same base topic, so their `$description` publishes overwrote each other on the broker while the parent named that child twice in its own `children` list. `delete()` detaches a child, so recreating one after deleting it is not affected. ([#67](https://github.com/electrification-bus/python-sdk/issues/67))
+
+### Documentation
+
+- `DeviceTreeBuilder` now states two parts of its contract that the API alone did not convey, both reported by a consumer reconciling an existing multi-device builder against it. First, whether a producer is expected to *adapt its own model* to `GroupedPropertyDict` or to *own one*: it is the second, which is the observable-model pattern the proxy guide prescribes, and the builder accepts rather than creates one so a single model can span a tree and so a producer holding one already can hand it over. Second, the limit of `add()`'s ordering: it orders late-bound ids and builds an unbuilt parent it was handed, but `DeviceSpec` is frozen and `parent` is a direct reference, so a child spec cannot be constructed before its parent spec exists. A caller deriving specs from a source that names parents indirectly still owns that dependency ordering. `add()` reads as though ordering is handled generally; it is handled for ids. Thanks to [@cayossarian](https://github.com/cayossarian) ([#49](https://github.com/electrification-bus/python-sdk/issues/49)).
+
 ## [0.21.0] — 2026-08-20
 
 ### Added

src/ebus_sdk/declaration.py

@@ -151,8 +151,10 @@ def build_from_declarations(
 
     Groups `specs` by capability (one Homie node each) and, for every spec,
     creates an observable `Property` in `model` and a Homie property on the node,
-    wired together with `bind_property_to_homie` (the outbound/report path). Runs
-    inside one `device.state_transition()`. Returns
+    wired together with `bind_property_to_homie` (the outbound/report path). An
+    observable property the model ALREADY holds is reused, never replaced, since
+    replacing it would discard the live value and every callback and
+    `entity_setter` attached to it. Runs inside one `device.state_transition()`. Returns
     `{(capability, prop_id): homie.Property}`, keyed by WIRE identity; a spec
     with `internal_only=True` creates no Homie property and so is absent from it.
 
@@ -238,6 +240,12 @@ def _materialize(
     devices, model groups keyed per device). Runs inside one
     `device.state_transition()`, so a device announces its structure once.
 
+    An observable property already present in `model` is REUSED rather than
+    replaced, and is not recorded in `model_keys`, so a later teardown removes
+    only what this call created. A spec whose python type disagrees with the
+    property already there raises instead of silently binding a Homie property to
+    a mismatched twin.
+
     `node_id` maps a capability to the Homie node id it materializes onto,
     defaulting to the capability itself. Only the id is renamed: the model group
     still comes from the spec, and the returned map is still keyed by the
@@ -273,9 +281,21 @@ def _materialize(
                     model.create_group(group)
                     created_groups.append(group)
                 py_type = spec.python_type if spec.python_type is not None else python_type_for(spec.datatype)
-                model.add_property(group, ObservableProperty(id=spec.model_key, type=py_type))
+                existing = model.get(group, spec.model_key)
+                if existing is None:
+                    model.add_property(group, ObservableProperty(id=spec.model_key, type=py_type))
+                    # Only what this call created, so a later remove() deletes what
+                    # it added and leaves anything the producer owned first.
+                    model_keys.append((group, spec.model_key))
+                elif existing.type() is not py_type:
+                    raise ValueError(
+                        f"{capability}/{spec.prop_id}: the model already holds "
+                        f"{group}/{spec.model_key} with type {existing.type()!r}, but this spec "
+                        f"declares {py_type!r}. Reusing it would publish values of one type through "
+                        "a property built for another; align the spec's datatype (or python_type) "
+                        "with the model, or give the spec its own source_id/model_group."
+                    )
                 declared[(capability, spec.prop_id)] = spec
-                model_keys.append((group, spec.model_key))
                 # An entity_setter is the translator toward the entity, so it is
                 # registered whenever one is given and the model can reach it.
                 if spec.entity_setter is not None and (spec.settable or spec.internal_only):

src/ebus_sdk/homie.py

@@ -1549,6 +1549,32 @@ def __init__(
             raise ValueError(
                 f"Device id={id}: cannot pass both parent= and mqttc=; children share the root's MQTT connection"
             )
+        # A device id that collides with an ancestor's is always a mistake, and a
+        # silent one: Device.__init__ appends to parent._children with no check, so
+        # a child carrying the root's id makes the root name itself as its own
+        # child in $description, and both publish to the same topics. Refuse it at
+        # construction, where the caller can still see which id was wrong.
+        if parent is not None:
+            ancestor: Optional[Device] = parent
+            while ancestor is not None:
+                if id == ancestor.id():
+                    raise ValueError(
+                        f"Device id={id}: a child cannot carry the same id as its "
+                        f"{'parent' if ancestor is parent else 'ancestor'}; both would publish to the "
+                        "same topics and the ancestor would name itself in its own children"
+                    )
+                ancestor = ancestor.parent()
+            # Same defect one step sideways: two children of one parent sharing an
+            # id derive the same base topic, so their $description publishes
+            # overwrite each other on the broker and the parent names the child
+            # twice in its own `children`. The parent already tracks its children,
+            # so this costs no new state.
+            if any(child.id() == id for child in parent.children()):
+                raise ValueError(
+                    f"Device id={id}: parent id={parent.id()} already has a child with this id; "
+                    "both would publish to the same topics and the parent would name it twice"
+                )
+
         # The domain is a per-TREE property, like the connection and the QoS: one
         # tree publishes under one prefix, and a child under a different domain
         # would sit outside its own root's subtree. Refuse it on a child rather

tests/test_declaration.py

@@ -7,6 +7,7 @@
     DeviceSpec,
     DeviceTreeBuilder,
     GroupedPropertyDict,
+    ObservableProperty,
     PropertyDatatype,
     PropertySpec,
     Unit,
@@ -502,3 +503,92 @@ def test_device_tree_builder_passes_node_id_through(mock_paho):
     )
     assert child.get_node("x-info") is not None
     assert child.get_node("info") is None
+
+
+# --- Reusing a model the builder does not own (GH #66) -----------------------
+
+
+def _live_model(group="dev-1", prop_id="serial-number"):
+    """A model a producer already populated, before any Homie tree existed."""
+    model = GroupedPropertyDict()
+    model.create_group(group)
+    model.add_property(group, ObservableProperty(id=prop_id, type=str))
+    model.set_value(group, prop_id, "SN-LIVE")
+    return model
+
+
+def test_an_existing_model_property_is_reused_not_replaced(mock_paho):
+    device = _device(mock_paho, "dev-reuse")
+    model = _live_model(group="info")
+    before = model.get("info", "serial-number")
+
+    build_from_declarations(device, model, [PropertySpec("info", "serial-number", PropertyDatatype.STRING)])
+
+    # The same object, so nothing attached to it was discarded.
+    assert model.get("info", "serial-number") is before
+    assert model.value("info", "serial-number") == "SN-LIVE"
+
+
+def test_reuse_preserves_callbacks_and_the_entity_setter(mock_paho):
+    device = _device(mock_paho, "dev-reuse-cb")
+    model = _live_model(group="info")
+    changed, commanded = [], []
+    model.add_property_on_change_callback("info", "serial-number", lambda p: changed.append(p.value()))
+    model.set_entity_setter("info", "serial-number", commanded.append)
+
+    build_from_declarations(device, model, [PropertySpec("info", "serial-number", PropertyDatatype.STRING)])
+
+    model.set_value("info", "serial-number", "SN-NEW")
+    assert changed == ["SN-NEW"], "the producer's on-change callback died with the replaced property"
+    model.set_entity("info", "serial-number", "CMD")
+    assert commanded == ["CMD"], "the producer's actuator died while $description still advertises settable"
+
+
+def test_the_tree_builder_reuses_a_producers_property_too(mock_paho):
+    root = _device(mock_paho, "enclosure-1")
+    model = _live_model(group="dev-1")
+    builder = DeviceTreeBuilder(root, model)
+    builder.add(
+        DeviceSpec("circuit", [PropertySpec("info", "serial-number", PropertyDatatype.STRING)], device_id="dev-1")
+    )
+    assert model.value("dev-1", "serial-number") == "SN-LIVE"
+
+
+def test_remove_deletes_what_the_builder_created_and_nothing_else(mock_paho):
+    root = _device(mock_paho, "enclosure-2")
+    model = _live_model(group="dev-1")
+    builder = DeviceTreeBuilder(root, model)
+    spec = DeviceSpec(
+        "circuit",
+        [
+            PropertySpec("info", "serial-number", PropertyDatatype.STRING),  # the producer's
+            PropertySpec("meter", "active-power", PropertyDatatype.FLOAT),  # the builder's
+        ],
+        device_id="dev-1",
+    )
+    builder.add(spec)
+    assert model.get("dev-1", "active-power") is not None
+
+    builder.remove(spec)
+    # What it created is gone; what it found is left where it was.
+    assert model.get("dev-1", "active-power") is None
+    assert model.get("dev-1", "serial-number") is not None
+    assert model.value("dev-1", "serial-number") == "SN-LIVE"
+
+
+def test_a_type_disagreement_with_an_existing_property_is_refused(mock_paho):
+    device = _device(mock_paho, "dev-mismatch")
+    model = _live_model(group="info")  # holds a str property
+    with pytest.raises(ValueError, match="already holds"):
+        build_from_declarations(device, model, [PropertySpec("info", "serial-number", PropertyDatatype.FLOAT)])
+
+
+def test_reuse_still_binds_and_publishes(mock_paho):
+    """Reusing the twin must not skip the wiring: a later model write still reaches Homie."""
+    device = _device(mock_paho, "dev-reuse-bind")
+    model = _live_model(group="info")
+    homie_props = build_from_declarations(
+        device, model, [PropertySpec("info", "serial-number", PropertyDatatype.STRING)]
+    )
+    model.set_value("info", "serial-number", "SN-NEW")
+    assert homie_props[("info", "serial-number")].value() == "SN-NEW"

tests/test_homie_device.py

@@ -3031,3 +3031,63 @@ def test_device_publish_keeps_its_own_severity_when_a_client_was_expected(self,
         assert [
             r for r in caplog.records if r.levelno == logging.INFO and "devicePublishNoMqttClient" in r.getMessage()
         ]
+
+
+class TestChildIdCollidesWithAnAncestor:
+    """A child carrying an ancestor's id is always a mistake, and used to be silent (GH #67).
+
+    Device.__init__ appends to parent._children with no check, so the ancestor
+    ended up naming itself in its own `children` and both devices published to
+    the same topics.
+    """
+
+    def test_child_carrying_its_parents_id_is_refused(self, mock_paho):
+        root, _ = _make_device(mock_paho, device_id="enclosure-1")
+        with pytest.raises(ValueError, match="same id as its parent"):
+            Device("enclosure-1", parent=root)
+
+    def test_grandchild_carrying_the_roots_id_is_refused(self, mock_paho):
+        root, _ = _make_device(mock_paho, device_id="enclosure-1")
+        bess = Device("bess-1", parent=root)
+        with pytest.raises(ValueError, match="same id as its ancestor"):
+            Device("enclosure-1", parent=bess)
+
+    def test_the_root_never_names_itself_as_its_own_child(self, mock_paho):
+        root, _ = _make_device(mock_paho, device_id="enclosure-1")
+        with pytest.raises(ValueError):
+            Device("enclosure-1", parent=root)
+        assert root.children_ids() == []
+        assert "enclosure-1" not in root.description()["children"]
+
+    def test_a_legitimate_child_is_unaffected(self, mock_paho):
+        root, _ = _make_device(mock_paho, device_id="enclosure-1")
+        child = Device("circuit-1", parent=root)
+        grandchild = Device("mid-1", parent=child)
+        assert root.children_ids() == ["circuit-1"]
+        assert grandchild.root() is root
+
+    def test_the_same_id_under_a_different_root_is_fine(self, mock_paho):
+        """Ids only have to be unique within a tree's ancestry, not globally."""
+        root_a, _ = _make_device(mock_paho, device_id="enclosure-a")
+        root_b, _ = _make_device(mock_paho, device_id="enclosure-b")
+        Device("circuit-1", parent=root_a)
+        Device("circuit-1", parent=root_b)
+        assert root_a.children_ids() == ["circuit-1"]
+        assert root_b.children_ids() == ["circuit-1"]
+
+    def test_two_children_of_one_parent_cannot_share_an_id(self, mock_paho):
+        """Same defect one step sideways: identical base topics, and a child named twice."""
+        root, _ = _make_device(mock_paho, device_id="enclosure-1")
+        Device("circuit-1", parent=root)
+        with pytest.raises(ValueError, match="already has a child with this id"):
+            Device("circuit-1", parent=root)
+        assert root.children_ids() == ["circuit-1"]
+
+    def test_recreating_a_deleted_child_is_allowed(self, mock_paho):
+        """delete() detaches, so rebuild-after-delete is not a false positive."""
+        root, _ = _make_device(mock_paho, device_id="enclosure-1")
+        child = Device("circuit-1", parent=root)
+        child.delete()
+        assert root.children_ids() == []
+        Device("circuit-1", parent=root)  # must not raise
+        assert root.children_ids() == ["circuit-1"]