Fix hotwater_lock key-collision hazard, address Opus review of triage fixes

An Opus review of the previous commit found that giving the switchHotwater
fallback and LOCK.hotwater_lock the same key introduced a real bug:
adapter.flatten() (the source of coordinator.data, which every switch's
is_on reads) only ever honours exists_fn, never entity.py's implicit
own-field-presence default that gates plain registration. With only one
side of the pair gated, both descriptors still wrote the same key into the
flattened state dict, and whichever was processed last -- decided by
device-reported href order, not correctness -- silently won. Reproduced
with the existing coffee fixture: reversing resource order flipped
hotwater_lock from correct (False) to a stuck True.

Fixed by gating both sides symmetrically via a shared tri-state helper that
also treats an unfetched /status/lock/vs/0 stub as "outcome pending" rather
than "confirmed absent" -- otherwise the stub window let both descriptors
pass exists_fn at once, which would have registered two switch entities
with the same unique_id. The fallback also re-asserts its own field's
presence, a check it used to get for free before it shared LOCK's key.

Also addresses two smaller findings from the same review, both in the
range-hood after-run capability (#147): runningProgress's unit='%' was a
guess from a single "0" sample with no supported-values/range field to
confirm the domain -- inconsistent with treating activationState as
read-only for the same "don't guess" reason -- so it's now a bare
passthrough sensor; and entity_category='diagnostic' was dropped from the
two read entities since after-run is a feature the user actively watches
and cancels via the (correctly uncategorized) button, not passive
diagnostics.
This commit is contained in:
Marc Billow
2026-07-28 02:25:42 +00:00
parent 5055d6bb72
commit 0640ac3477
4 changed files with 120 additions and 21 deletions
@@ -181,12 +181,18 @@ HOOD_FILTER = Capability(
# After Run (issue #147): the hood keeps the fan running at low speed for a
# while after it's switched off, to clear residual cooking smoke. No
# while after it's switched off, to clear residual cooking smoke -- a
# feature a user actively watches and cancels, not passive diagnostics, so
# none of the three entities below carry entity_category. No
# supported-values list is advertised for activationState, so it's modeled
# read-only (monitoring, not an invented "enable" write) per the 'don't
# guess' rule; runningCancel's only observed value is the command name
# itself ('Cancel'), the same self-describing command-field shape as
# operational.STOP_BUTTON.
# operational.STOP_BUTTON. runningProgress is left a bare passthrough for the
# same 'don't guess' reason -- the only observed sample is "0", with no
# range or supported-values field to confirm it's a percentage rather than a
# minutes/seconds count, and unit + state_class='measurement' would commit
# it to long-term statistics under a possibly-wrong unit.
AFTER_RUN = Capability(
href='/afterrun/vs/0',
poll_tier='warm',
@@ -195,17 +201,12 @@ AFTER_RUN = Capability(
key='after_run_active',
field='x.com.samsung.da.activationState',
icon='mdi:fan-clock',
entity_category='diagnostic',
value_fn=lambda value: str(value).lower() == 'on',
),
SensorDesc(
key='after_run_progress',
field='x.com.samsung.da.runningProgress',
unit='%',
state_class='measurement',
icon='mdi:fan-clock',
entity_category='diagnostic',
value_fn=int_or_none,
),
ButtonDesc(
key='after_run_cancel',
@@ -105,6 +105,29 @@ FAVORITE_CAPACITY = Capability(
# Coffee-capable variant (issue #107) -- a "favorite" supported-list select
# for the hot water dispensed alongside brewing, same shape as
# FAVORITE_CAPACITY above.
def _status_lock_definitely_lacks_hotwater_field(resources: dict) -> bool:
"""Three-way read of /status/lock/vs/0's hotwaterLock field, favouring
LOCK.hotwater_lock (the primary descriptor) whenever the outcome is
still ambiguous:
- href entirely absent from this device -> definitely no clash, the
switchHotwater fallback below may claim the entity.
- href present but an unfetched stub ({}) -> outcome pending, *not* a
confirmed absence. LOCK's own exists_fn optimistically includes itself
through a stub (matching entity.py's default), so returning True here
too would register both descriptors -- as SwitchDescs sharing one key,
with identical unique_ids -- until the next poll resolves it.
- href present and fetched -> the real answer."""
rep = resources.get('/status/lock/vs/0')
if rep is None:
return True
if not rep:
return False
return 'x.com.samsung.da.hotwaterLock' not in rep
FAVORITE_HOTWATER = Capability(
href='/favorite/hotwater/vs/0',
poll_tier='cold',
@@ -114,15 +137,26 @@ FAVORITE_HOTWATER = Capability(
# hot-water lock as LOCK.hotwater_lock below, just surfaced through
# this href on boards that don't populate /status/lock/vs/0's
# hotwaterLock field. Shares that descriptor's key so only one "Hot
# water lock" entity ever appears; exists_fn activates this fallback
# only when the primary field is absent, so a board reporting both
# can't collide.
# water lock" entity ever appears.
#
# Both halves of this fallback pair need an exists_fn, not just this
# one: adapter.flatten() (the coordinator.data source every entity's
# is_on reads) only ever honours exists_fn, never entity.py's
# implicit "require own field present" default that gates plain
# registration. Two same-keyed descriptors with only one of them
# gated still both land in flatten()'s output dict -- whichever is
# processed last silently wins, decided by device-reported href
# order, not by which one is actually correct. So this exists_fn
# also re-asserts its own field's presence (switchHotwater), the
# gate a bare `field=` used to get for free before it had to share a
# key with LOCK's descriptor.
SwitchDesc(key='hotwater_lock', field='x.com.samsung.da.switchHotwater',
device_class='lock',
entity_category='config',
value_fn=lambda v: v != 'Unlocked',
exists_fn=lambda rep, resources: 'x.com.samsung.da.hotwaterLock' not in (
resources.get('/status/lock/vs/0') or {}),
exists_fn=lambda rep, resources: (
'x.com.samsung.da.switchHotwater' in rep
and _status_lock_definitely_lacks_hotwater_field(resources)),
write_fn=lambda p, rep, href=None: (
['favorite', 'hotwater', 'vs', '0'],
{'x.com.samsung.da.switchHotwater': 'Locked' if p == 'On' else 'Unlocked'})),
@@ -160,10 +194,19 @@ LOCK = Capability(
href='/status/lock/vs/0',
poll_tier='warm',
entities=(
# Shares its key with FAVORITE_HOTWATER's switchHotwater fallback
# above (issue #144); see the comment there for why this half also
# needs an explicit exists_fn now that the two share a key in
# adapter.flatten()'s output. A stub rep ({}) still counts as
# "present" here (matches entity.py's own default for a field-less
# gate) since the alternative -- treating an unfetched resource as
# confirmed-absent -- is what let both descriptors register at once.
SwitchDesc(key='hotwater_lock', field='x.com.samsung.da.hotwaterLock',
device_class='lock',
entity_category='config',
value_fn=lambda v: v != 'Unlocked',
exists_fn=lambda rep, resources: (
not rep or 'x.com.samsung.da.hotwaterLock' in rep),
write_fn=lambda p, rep, href=None: (
['status', 'lock', 'vs', '0'],
{'x.com.samsung.da.hotwaterLock': 'Locked' if p == 'On' else 'Unlocked'})),
+1 -1
View File
@@ -62,7 +62,7 @@ def test_range_hood_fixture_values():
assert state['air_sensing_state'] == 'NonProcessing'
assert state['last_air_sensing_level'] == 'Kr2'
assert state['after_run_active'] is False
assert state['after_run_progress'] == 0
assert state['after_run_progress'] == '0'
def test_one_composite_fan_is_bound():
+63 -8
View File
@@ -216,14 +216,69 @@ def test_favorite_hotwater_switch_is_a_lock_not_an_enable_flag():
assert lock.value_fn('Locked') is True
def test_favorite_hotwater_lock_fallback_only_activates_when_primary_absent():
"""The switchHotwater-based lock and LOCK's hotwaterLock-based lock share
the 'hotwater_lock' key so only one entity is ever registered (issue
#144). This fixture has no hotwaterLock field, so the fallback must be
active; a board reporting both must not."""
lock = _desc_coffee_by_href('hotwater_lock', '/favorite/hotwater/vs/0')
assert lock.exists_fn({}, {'/status/lock/vs/0': {}}) is True
assert lock.exists_fn({}, {'/status/lock/vs/0': {'x.com.samsung.da.hotwaterLock': 'Unlocked'}}) is False
def test_favorite_hotwater_lock_wins_in_flattened_state():
"""adapter.flatten() -- the actual source of coordinator.data every
switch's is_on reads -- only honours exists_fn, never entity.py's
implicit own-field-presence default. So it's not enough for the
*registered* entity to resolve correctly (test_expected_state_keys_present
territory); the shared 'hotwater_lock' key in the flattened dict itself
must reflect the live switchHotwater value, not a stale phantom from
LOCK's ungated hotwaterLock read (issue #144). This fixture's
switchHotwater reads 'Unlocked'; without exists_fn on *both* sides of the
pair, LOCK's descriptor computes None != 'Unlocked' == True regardless,
and flatten() would pick whichever of the two entities happens to be
processed last."""
state = _state_coffee()
assert state['hotwater_lock'] is False
def test_exactly_one_hotwater_lock_descriptor_exists_per_resource_state():
"""Both LOCK.hotwater_lock and FAVORITE_HOTWATER's switchHotwater
fallback are always bound on this fixture (their hrefs are both always
present) -- discrimination happens entirely in exists_fn. Exactly one of
the two must ever pass, regardless of iteration order, or two switch
entities would be registered with the same unique_id."""
bound, resources = _bound_coffee()
candidates = [b for b in bound if b.desc.key == 'hotwater_lock']
assert len(candidates) == 2
included = [b for b in candidates
if b.desc.exists_fn(resources.get(b.href) or {}, resources)]
assert len(included) == 1
assert included[0].href == '/favorite/hotwater/vs/0'
def test_hotwater_lock_fallback_gating_across_status_lock_states():
"""The fallback (FAVORITE_HOTWATER's switchHotwater descriptor) must
activate only once /status/lock/vs/0 is confirmed to lack hotwaterLock --
never while that resource is an unfetched stub ({}), and never when it
does carry the field. LOCK's own descriptor is the mirror image."""
fallback = _desc_coffee_by_href('hotwater_lock', '/favorite/hotwater/vs/0')
primary = _desc_coffee_by_href('hotwater_lock', '/status/lock/vs/0')
own_rep = {'x.com.samsung.da.switchHotwater': 'Unlocked'}
# /status/lock/vs/0 absent entirely -- device genuinely lacks it.
assert fallback.exists_fn(own_rep, {}) is True
# /status/lock/vs/0 present but not yet fetched (a stub): outcome
# pending, so the fallback must defer to LOCK rather than assume absence.
stub_resources = {'/status/lock/vs/0': {}}
assert fallback.exists_fn(own_rep, stub_resources) is False
assert primary.exists_fn({}, stub_resources) is True
# /status/lock/vs/0 fetched and confirmed to lack hotwaterLock (this
# fixture's actual shape) -- the fallback wins.
absent_resources = {'/status/lock/vs/0': {'x.com.samsung.da.coldwaterLock': 'Unlocked'}}
assert fallback.exists_fn(own_rep, absent_resources) is True
assert primary.exists_fn(absent_resources['/status/lock/vs/0'], absent_resources) is False
# /status/lock/vs/0 fetched and does carry hotwaterLock -- primary wins.
present_resources = {'/status/lock/vs/0': {'x.com.samsung.da.hotwaterLock': 'Unlocked'}}
assert fallback.exists_fn(own_rep, present_resources) is False
assert primary.exists_fn(present_resources['/status/lock/vs/0'], present_resources) is True
# The fallback also re-asserts its own field, since it no longer gets
# that check for free once it shares LOCK's key (issue #144 review).
assert fallback.exists_fn({}, {}) is False
def test_favorite_hotwater_temperature_options_come_from_live_supported_list():