fix: five defects in the declarative builder, all silent in production

CHANGELOG.md

@@ -4,6 +4,18 @@ All notable changes to `ebus-sdk` are recorded here. Format follows [Keep a Chan
 
 ## [Unreleased]
 
+### Fixed
+
+- `conditionally_settable` was inert: `_materialize` never read it. The half that looked right is that the property did come out not-settable; the half that bit is that the `entity_setter` was registered only when `settable` was true, so the caller's later `set_settable(True)` opened a `/set` topic with no translator behind it. The property then advertised that it accepts commands and silently discarded them, which is the exact failure the field was introduced to avoid, one step further along. It is the only route the API offers for per-instance settability decided at runtime, and it was the route that did not work. The translator is now wired at build time even though the property starts not-settable. The test that shipped with the feature asserted only the not-settable half, which is why this survived review: a test written from the design rationale checks the rationale rather than the feature. ([#72](https://github.com/electrification-bus/python-sdk/issues/72))
+
+- A value the model already held is now published when its Homie twin is built. The binding is on-change and a fresh twin starts empty, so a producer whose model predates the tree (the arrangement `DeviceTreeBuilder` documents as its reason for accepting an external model) announced its declared default instead of the live value, on every property. It did not self-heal, because `set_value` fires callbacks only on an actual change, so a value written once at group creation stayed wrong for the process lifetime. This became reachable in 0.22.0: before the reuse fix the model property was replaced, so twin and model started equally empty. Relatedly, a declared `initial_value` now SEEDS rather than overwrites, since a model already holding a value holds a fresher one than the declaration; an explicit `values` entry still wins, being a statement about this run. ([#77](https://github.com/electrification-bus/python-sdk/issues/77))
+
+- `DeviceTreeBuilder.remove()` no longer raises when the producer's model has already dropped the group. `GroupedPropertyDict.delete_group` removes the group BEFORE firing `GROUP_DELETED` and dispatch is synchronous, so a consumer driving `remove()` from that event was guaranteed to hit it. It raised after `device.delete()` and before the bookkeeping pop, leaving the device gone from the broker while the builder still held a corpse that short-circuited the next `add()`: unrecoverable for the process lifetime, and inside an observer callback it surfaced as a single swallowed warning. Bookkeeping is now dropped in a `finally`, and the per-property `deletePropertyGroupNotFound` warning burst is gone with it, which matters on bounded-disk fleet devices. ([#73](https://github.com/electrification-bus/python-sdk/issues/73))
+
+- `DeviceTreeBuilder.remove()` now prunes deferred descendants, not only the removed spec. A deferred child holds a frozen reference to its parent spec, so `resolve_deferred()` would rebuild a device that had been deliberately torn down. The shape most likely to hit it is a mandatory child deferred on a late identifier, which sits in the queue for exactly the window in which its parent might be removed. ([#75](https://github.com/electrification-bus/python-sdk/issues/75))
+
+- `DeviceTreeBuilder.add()` records its bookkeeping before materializing rather than after. The device is constructed, attached and broker-visible by then, so a raise left a live device the builder had no record of: `device_for()` returned None, `remove()` was a silent no-op, and the retained topics were stranded. The same window admitted re-entry, since the model's events dispatch synchronously and a producer observing its own model could call `add()` again before the cache entry existed. ([#76](https://github.com/electrification-bus/python-sdk/issues/76))
+
 ## [0.22.0] — 2026-08-20
 
 ### Added

src/ebus_sdk/declaration.py

@@ -81,8 +81,11 @@ class PropertySpec:
       runtime, per instance. The builder materializes it NOT settable, which
       keeps `$description` truthful and leaves no `/set` subscription open on a
       property that would reject the command; the caller enables it with
-      `homie.Property.set_settable(True)` inside a `state_transition()`. It is
-      mutually exclusive with `settable`, which means "settable now".
+      `homie.Property.set_settable(True)` inside a `state_transition()`. The
+      `entity_setter` is wired at build time even though the property starts
+      not-settable, because `set_settable(True)` subscribes immediately and a
+      `/set` topic with no translator behind it accepts commands and discards
+      them. It is mutually exclusive with `settable`, which means "settable now".
     * `source_id` / `model_group`: the observable-model identity, when it
       differs from the wire identity. `source_id` defaults to `prop_id` and
       `model_group` to `capability`, so they are fused unless split. Splitting
@@ -306,7 +309,9 @@ def _materialize(
                 declared[(capability, spec.prop_id)] = spec
                 # 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):
+                if spec.entity_setter is not None and (
+                    spec.settable or spec.conditionally_settable or spec.internal_only
+                ):
                     model.set_entity_setter(group, spec.model_key, spec.entity_setter)
                 if spec.internal_only or node is None:
                     continue
@@ -333,10 +338,23 @@ def _materialize(
                     continue
                 homie_prop = node.add_property_from_dict(prop_dict)
                 bind_property_to_homie(model, group, spec.model_key, homie_prop)
+                # The binding is on-change, and the twin starts empty, so a value
+                # the model was already holding would never reach the wire: a
+                # producer whose model predates the tree would publish its
+                # declared default forever, and it would not self-heal, because
+                # set_value fires callbacks only on an actual change.
+                current = model.value(group, spec.model_key)
+                if current is not None:
+                    homie_prop.set_value(current)
                 # Inbound/control path for a settable property with a translator:
                 # /set payload -> model.set_entity -> entity_setter. The /set
                 # subscription is already live from add_property -> set_subscribe.
-                if spec.settable and spec.entity_setter is not None:
+                # conditionally_settable too, and this is the whole point of it:
+                # the property is built not-settable, so no /set topic is open
+                # yet, but the caller flips it with set_settable(True) later and
+                # that subscribes immediately. Wiring the translator now is what
+                # stops that topic from accepting commands and discarding them.
+                if spec.entity_setter is not None and (spec.settable or spec.conditionally_settable):
                     homie_prop.set_set_callback(partial(model.set_entity, group, spec.model_key))
                 homie_props[(capability, spec.prop_id)] = homie_prop
 
@@ -381,8 +399,14 @@ def _seed(
     them: a caller passing a runtime map is being more specific than the static
     declaration. Entries naming an undeclared property are ignored.
     """
+    # A declared initial_value SEEDS, it does not overwrite: a model that already
+    # holds a value for this property holds a fresher one than the declaration.
+    # An explicit `values` entry still wins below, since that caller is being
+    # specific about this run rather than about the property in general.
     seed: dict[tuple, Any] = {
-        key: spec.initial_value for key, spec in declared.items() if spec.initial_value is not None
+        key: spec.initial_value
+        for key, spec in declared.items()
+        if spec.initial_value is not None and model.value(_group_for(spec, default_group), spec.model_key) is None
     }
     if values:
         seed.update({key: value for key, value in values.items() if key in declared})
@@ -560,6 +584,17 @@ def add(self, spec: DeviceSpec) -> Optional[Device]:
             type=spec.resolve_device_type(),
             parent=parent_device,
         )
+        # Record BEFORE materializing. The device is already constructed,
+        # attached and visible on the broker, so a raise below (or a re-entrant
+        # add() from a producer observing its own model, since the model's events
+        # dispatch synchronously) must not leave a live device the builder has no
+        # record of: device_for() would return None and remove() would be a
+        # silent no-op, stranding retained topics.
+        self._devices[spec] = device
+        self._homie_props[spec] = {}
+        self._model_keys[spec] = []
+        self._created_groups[spec] = []
+
         group = spec.resolve_model_group(device_id)
         built = _materialize(
             device,
@@ -572,10 +607,9 @@ def add(self, spec: DeviceSpec) -> Optional[Device]:
         )
         _seed(self._model, built.declared, default_group=group)
 
-        self._devices[spec] = device
-        self._homie_props[spec] = built.homie_props
-        self._model_keys[spec] = built.model_keys
-        self._created_groups[spec] = built.created_groups
+        self._homie_props[spec].update(built.homie_props)
+        self._model_keys[spec].extend(built.model_keys)
+        self._created_groups[spec].extend(built.created_groups)
         if spec in self._deferred:
             self._deferred.remove(spec)
         if spec.on_created is not None:
@@ -609,23 +643,38 @@ def remove(self, spec: DeviceSpec) -> None:
         too, along with any group it created that is now empty; a group the
         caller created, or one still in use, is left alone.
         """
+        # Deferred descendants go whether or not this spec was ever built: a
+        # deferred child holds a frozen reference to its parent spec, so leaving
+        # it in the queue lets resolve_deferred() rebuild a device that was
+        # deliberately torn down.
+        self._deferred = [s for s in self._deferred if not _descends_from(s, spec)]
+
         device = self._devices.get(spec)
         if device is None:
-            if spec in self._deferred:
-                self._deferred.remove(spec)  # never built, just stop waiting for it
-            return
+            return  # never built (or already removed); the queue is now clean
 
         doomed = {id(d) for d in _descendants(device)}
         removed = [s for s, d in self._devices.items() if id(d) in doomed]
         device.delete()
         for gone in removed:
-            for group, model_key in self._model_keys.pop(gone, []):
-                self._model.delete_property(group, model_key)
-            for group in self._created_groups.pop(gone, []):
-                if not self._model.items(group):
-                    self._model.delete_group(group)
-            self._devices.pop(gone, None)
-            self._homie_props.pop(gone, None)
+            # Bookkeeping is dropped whatever the model does, so a teardown can
+            # never leave a corpse in _devices that short-circuits the next
+            # add(). The model may legitimately have moved on already: a consumer
+            # driving remove() from a GROUP_DELETED observer is guaranteed to
+            # arrive after the group is gone, since delete_group removes it
+            # before firing and dispatch is synchronous.
+            try:
+                for group, model_key in self._model_keys.get(gone, []):
+                    if self._model.has_group(group) and self._model.get(group, model_key) is not None:
+                        self._model.delete_property(group, model_key)
+                for group in self._created_groups.get(gone, []):
+                    if self._model.has_group(group) and not self._model.items(group):
+                        self._model.delete_group(group)
+            finally:
+                self._model_keys.pop(gone, None)
+                self._created_groups.pop(gone, None)
+                self._devices.pop(gone, None)
+                self._homie_props.pop(gone, None)
 
     def add_root_capabilities(self, specs: Iterable[PropertySpec], *, model_group: Optional[str] = None) -> dict:
         """Materialize capabilities onto the tree's ROOT device.
@@ -724,6 +773,16 @@ def _defer(self, spec: DeviceSpec) -> None:
             self._deferred.append(spec)
 
 
+def _descends_from(spec: DeviceSpec, ancestor: DeviceSpec) -> bool:
+    """True when `spec` is `ancestor` or is declared beneath it."""
+    current: Optional[DeviceSpec] = spec
+    while current is not None:
+        if current is ancestor:
+            return True
+        current = current.parent
+    return False
+
+
 def _descendants(device: Device) -> list:
     """`device` and every device beneath it, parents before children."""
     found = [device]

tests/test_declaration.py

@@ -592,3 +592,105 @@ def test_reuse_still_binds_and_publishes(mock_paho):
     )
     model.set_value("info", "serial-number", "SN-NEW")
     assert homie_props[("info", "serial-number")].value() == "SN-NEW"
+
+
+# --- conditionally_settable reaches the inbound path (GH #72) ----------------
+
+
+def test_conditionally_settable_registers_its_entity_setter(mock_paho):
+    device = _device(mock_paho, "dev-cond-setter")
+    model = GroupedPropertyDict()
+    received = []
+    build_from_declarations(
+        device,
+        model,
+        [
+            PropertySpec(
+                "control",
+                "limit",
+                PropertyDatatype.FLOAT,
+                conditionally_settable=True,
+                entity_setter=received.append,
+            )
+        ],
+    )
+    model.set_entity("control", "limit", 42.0)
+    assert received == [42.0], "the translator was never registered on the model"
+
+
+def test_enabling_a_conditionally_settable_property_gives_a_working_set_topic(mock_paho):
+    """The whole point of the field: the /set topic opened later must have a handler."""
+    device = _device(mock_paho, "dev-cond-flip")
+    model = GroupedPropertyDict()
+    received = []
+    props = build_from_declarations(
+        device,
+        model,
+        [
+            PropertySpec(
+                "control",
+                "limit",
+                PropertyDatatype.FLOAT,
+                conditionally_settable=True,
+                entity_setter=received.append,
+            )
+        ],
+    )
+    hp = props[("control", "limit")]
+    assert hp.settable() is False  # still built not-settable
+
+    with device.state_transition():
+        hp.set_settable(True)
+
+    # Subscribed AND wired: a topic that accepts commands and discards them is
+    # the failure this field exists to avoid.
+    assert [c for c in mock_paho.subscribe.call_args_list if c.args and str(c.args[0]).endswith("/control/limit/set")]
+    assert hp.get_set_callback() is not None
+    hp.get_set_callback()("42.0")
+    assert received == ["42.0"]
+
+
+# --- a live model value reaches the wire (GH #77) ----------------------------
+
+
+def test_a_value_the_model_already_held_is_published(mock_paho):
+    device = _device(mock_paho, "dev-live")
+    model = _live_model(group="info")  # holds SN-LIVE before any tree exists
+    props = build_from_declarations(device, model, [PropertySpec("info", "serial-number", PropertyDatatype.STRING)])
+    # The binding is on-change and the twin starts empty, so without an explicit
+    # push the wire would show the declared default for the process lifetime.
+    assert props[("info", "serial-number")].value() == "SN-LIVE"
+    assert "SN-LIVE" in [c.args[1] for c in mock_paho.publish.call_args_list if len(c.args) > 1]
+
+
+def test_initial_value_seeds_but_does_not_clobber_a_live_value(mock_paho):
+    device = _device(mock_paho, "dev-live-2")
+    model = _live_model(group="info")
+    build_from_declarations(
+        device,
+        model,
+        [PropertySpec("info", "serial-number", PropertyDatatype.STRING, initial_value="DECLARED")],
+    )
+    assert model.value("info", "serial-number") == "SN-LIVE"
+
+
+def test_an_explicit_values_entry_still_wins_over_a_live_value(mock_paho):
+    """`values` is a statement about this run, so it is more specific than either."""
+    device = _device(mock_paho, "dev-live-3")
+    model = _live_model(group="info")
+    build_from_declarations(
+        device,
+        model,
+        [PropertySpec("info", "serial-number", PropertyDatatype.STRING)],
+        values={("info", "serial-number"): "RUNTIME"},
+    )
+    assert model.value("info", "serial-number") == "RUNTIME"
+
+
+def test_the_tree_builder_publishes_live_values_too(mock_paho):
+    root = _device(mock_paho, "enclosure-live")
+    model = _live_model(group="dev-1")
+    builder = DeviceTreeBuilder(root, model)
+    spec = DeviceSpec("circuit", [PropertySpec("info", "serial-number", PropertyDatatype.STRING)], device_id="dev-1")
+    builder.add(spec)
+    assert builder.homie_properties(spec)[("info", "serial-number")].value() == "SN-LIVE"