From 056ac88bf56415dc0c0d412fe3bed7a7dc8af8ca Mon Sep 17 00:00:00 2001 From: Simon Clark Date: Tue, 4 Aug 2026 22:28:53 +0100 Subject: [PATCH 1/3] =?UTF-8?q?feat(export):=20E2=20=E2=80=94=20allow-list?= =?UTF-8?q?=20store,=20device=20catalog,=20Manage=20Matter=20Exports=20dia?= =?UTF-8?q?log?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone E2 of PRD-indigo-matter-export: devices become selectable for export with a declared role, and the loop guard (XNG3) goes live. - export_store.py — the allow-list. ExportEntry (indigo device id + §4.2 role + name override + options), RLock'd store persisted as ONE schema-versioned JSON string in pluginPrefs["matterExports"]. A blob it cannot parse is moved aside to "matterExports.corrupt" and the store starts empty: user config is logged and preserved, never silently discarded. - export_catalog.py — PRD §5.2 mapping. Returns EligibleDevice(roles, default) or Excluded(reason). The loop guard is pluginId ONLY (XAC6), checked before any type reasoning; type dispatch walks the IOM class-name chain rather than isinstance, which is unusable against the MagicMock'd indigo module. - MenuItems.xml — "Manage Matter Exports…" (UI-D). No , so the dialog gets a single Close button and its buttons do the work. Master-detail is a filter textfield + Apply-filter button + single-select device menu with dynamicReload (a multi-select list has no CallbackMethod at all), role menu, name/polarity fields, Add/Remove buttons, a readonly status textfield (Indigo labels cannot change at runtime) and a readonly summary list. - plugin.py — builds the store in startup, seeds the dialog via get_menu_action_config_ui_values, and owns the pickers and callbacks. Excluded devices are listed with their reason (XAC9) under an "x-" id and rejected server-side; generators degrade to an empty list, never raise. Tests: 217 new (store CRUD/round-trip/corruption/locking, a §5.2 zoo with five invariants per row, dialog XML shape and every callback). Full suite 1504 green; pylint 9.24 repo-wide (9.21 on main), 10.00 for the two new modules. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S --- CLAUDE.md | 2 + docs/HANDOVER.md | 4 +- .../Contents/Info.plist | 2 +- .../Contents/Server Plugin/MenuItems.xml | 84 +++ .../Contents/Server Plugin/export_catalog.py | 356 +++++++++++ .../Contents/Server Plugin/export_store.py | 253 ++++++++ .../Contents/Server Plugin/plugin.py | 312 ++++++++++ tests/fakes.py | 119 ++++ tests/test_export_catalog.py | 323 ++++++++++ tests/test_export_menu.py | 551 ++++++++++++++++++ tests/test_export_store.py | 267 +++++++++ 11 files changed, 2271 insertions(+), 2 deletions(-) create mode 100644 indigo-matter.indigoPlugin/Contents/Server Plugin/export_catalog.py create mode 100644 indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py create mode 100644 tests/test_export_catalog.py create mode 100644 tests/test_export_menu.py create mode 100644 tests/test_export_store.py diff --git a/CLAUDE.md b/CLAUDE.md index 45eb2fb..66589d3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,6 +45,8 @@ loop→Indigo writes go straight through `device_sync.apply_states` (thread-safe | `matter_client.py` | matter-server client (inbound): the `serverLocation` URI, the `server_info` + `start_listening` handshake, controller command wrappers | | `bridge_protocol.py` | Bridge-node wire contract — envelope/commands/error codes/roles + normalised `BridgeCommand`/`StatusReport`/`PairingReport`/`FabricInfo`. **Not** a rename firewall (we own both ends); `protocolVersion` is what protects us. See `docs/BRIDGE_PROTOCOL.md` | | `bridge_client.py` | Bridge-node client (outbound export): hello+attach handshake that fails closed on version skew, endpoint CRUD, fire-and-forget `set_state`, §5 event callbacks. Attach refusals are triaged (§1.1): `version_mismatch`/`mass_removal_refused` halt with a `halted_reason`, `endpoint_map_invalid` holds the socket open un-attached in a `recovery` state so the §3.11 rebuild stays reachable, anything else reconnects on the normal backoff | +| `export_store.py` | The export allow-list (PRD-indigo-matter-export §5.1): `ExportEntry` (device id + role + name override + options) and an `RLock`'d store persisted as ONE JSON string in `pluginPrefs["matterExports"]`, schema-versioned. A blob it cannot parse is moved aside to `matterExports.corrupt` and the store starts empty — user config is never silently discarded | +| `export_catalog.py` | Indigo device → eligible Matter roles, or an `Excluded(reason)` shown in the picker (PRD §5.2, XAC9). The loop guard (XNG3/XAC6) is `pluginId` and nothing else, checked before any type reasoning. Type dispatch walks the IOM **class-name chain**, not `isinstance` — the indigo module is a MagicMock under test | | `launch_agent.py` | Generic launchd LaunchAgent machinery (npm/npx/node resolution, plist authoring, applied-plist digest, orphan/EADDRINUSE reaping), driven by a frozen `AgentSpec` that carries one agent's identity. Extracted so the Matter **bridge node** can be a second agent without duplicating it (PRD-indigo-matter-export §4.2 / XOQ3) | | `server_process.py` | `ServerProcess` = the matter-server (controller) specialisation of `LaunchAgent`: its prefs, its argv, its pinned version. Gated by the `serverLocation` pref — the config asks "is matter-server on this Mac?"; `local` (turnkey default) manages it here on loopback, `remote` connects to a server elsewhere. `manageLaunchAgent`/host/port are derived from that in `startup` (see `plugin.py:server_location`) | | `commission_jobs.py` | Commissioning job state machine (API.md §3.2/§3.3) | diff --git a/docs/HANDOVER.md b/docs/HANDOVER.md index 218ddff..22f0794 100644 --- a/docs/HANDOVER.md +++ b/docs/HANDOVER.md @@ -497,7 +497,9 @@ Domio no longer commissions; it relays a **share code** (Apple Home is admin 1; `plugin.py` lifecycle/glue + IWS `http_*` handlers + action bridge · `async_runtime.py` the loop · `protocol.py` **rename firewall** (MatterCommand/MatterWrite/MatterEvent; field names verified vs v0.6.2) · `ws_json_client.py` **shared transport** (run loop, backoff, message_id↔future correlation, disconnect + repeated-failure diagnostics; handshake/vocabulary are subclass hooks) · `matter_client.py` matter-server client on top of it (server_info + start_listening handshake, controller wrappers) · `commission_jobs.py` job state machine · `http_handlers.py` routing · `device_sync.py` reconcile + priming + state/command/write seams + node_added · `matter_model.py` parse get_node (flat `attributes` map → endpoints) · `fabric_backup.py` storage-dir backup/restore (issue #26; zip into sibling `backups/`, move-aside restore, prune) · `launch_agent.py` generic AgentSpec + LaunchAgent machinery (launchd/npm/node, XOQ3 extraction) · `server_process.py` the controller AgentSpec + prefs on top of it (launchd PM-B, off by default) · `matter_handlers/` one ClusterHandler per cluster + registry. -**Export side (E1, `docs/BRIDGE_PROTOCOL.md`):** `bridge_protocol.py` the wire contract (envelope, §3 commands, §1.1 error codes, §4.2 roles, normalised `BridgeCommand`/`StatusReport`/`PairingReport`/`FabricInfo`; **no** rename firewall — we own both ends) · `bridge_client.py` the client (hello+attach handshake, **fails closed** on `protocolVersion` skew via `on_version_skew` + halt, endpoint CRUD, fire-and-forget `set_state`, §5 event callbacks) · `bridge-node/` the TypeScript node · `tests/fixtures/bridge_protocol/frames.json` the ONE golden-frame file both suites read (§7; `npm test` copies it into the TS build). Not yet wired into `plugin.py` — that is E2+. +**Export side (E1, `docs/BRIDGE_PROTOCOL.md`):** `bridge_protocol.py` the wire contract (envelope, §3 commands, §1.1 error codes, §4.2 roles, normalised `BridgeCommand`/`StatusReport`/`PairingReport`/`FabricInfo`; **no** rename firewall — we own both ends) · `bridge_client.py` the client (hello+attach handshake, **fails closed** on `protocolVersion` skew via `on_version_skew` + halt, endpoint CRUD, fire-and-forget `set_state`, §5 event callbacks) · `bridge-node/` the TypeScript node · `tests/fixtures/bridge_protocol/frames.json` the ONE golden-frame file both suites read (§7; `npm test` copies it into the TS build). + +**Export side (E2, allow-list + UI):** `export_store.py` the allow-list model (`ExportEntry` = device id + §4.2 role + name override + options; `RLock`'d, one schema-versioned JSON string in `pluginPrefs["matterExports"]`, unparseable blobs moved aside to `matterExports.corrupt` rather than discarded) · `export_catalog.py` the PRD §5.2 mapping (eligible roles + safe default, or `Excluded(reason)`; loop guard is `pluginId` only — XNG3/XAC6 — and type dispatch walks the IOM class-name chain because `isinstance` is unusable against the MagicMock'd indigo module) · `MenuItems.xml` → **Manage Matter Exports…**, the UI-D dialog: no `` (so it gets a single Close button and the in-dialog buttons do the work), filter textfield + Apply-filter button + single-select device `menu` with `dynamicReload` (a multi-select `list` has NO CallbackMethod, so master-detail is impossible with one) + role menu + name/polarity fields + Add/Remove buttons + a readonly `exportStatus` textfield (Indigo labels cannot change at runtime) + a readonly summary list. `plugin.py` builds the store in `startup` and owns the callbacks. `bridge_client` is still **not** wired to the store — that is E3. **Key invariants:** node-details has NO `endpoints` key (derive from flat `attributes`); `attribute_updated` data is `[node_id,"ep/cl/at",value]`; `node_removed` is a bare id; `server_info` is a bare connect frame (`sdk_version`/`fabric_id`). Setpoints/modes are attribute **writes**, not commands. diff --git a/indigo-matter.indigoPlugin/Contents/Info.plist b/indigo-matter.indigoPlugin/Contents/Info.plist index c09bbe4..ac6ae95 100644 --- a/indigo-matter.indigoPlugin/Contents/Info.plist +++ b/indigo-matter.indigoPlugin/Contents/Info.plist @@ -20,7 +20,7 @@ IwsApiVersion 1.0.0 PluginVersion - 2026.7.25 + 2026.7.26 ServerApiVersion 3.6 diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml b/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml index 32ac67d..dc29937 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml @@ -57,6 +57,90 @@ + + + Manage Matter Exports… + + + + + + + + + + + Type part of a device name, then click Apply filter. Leave empty to list everything. + + + + Apply filter / refresh + exportReloadPicker + + + + + exportDeviceChanged + A ● marks a device that is already exported. + + + + + + + + + Indigo cannot tell a plug from a lamp or a lock, so you declare it. The first option is the safest default. + + + + Optional. Leave empty to use the Indigo device name. + + + + Matter's convention is 100% = fully open. Tick this if your Indigo device is the other way round. + + + + Add / update export + exportAddOrUpdate + + + + Remove export + exportRemove + + + + + + + + + + + + + + Export fabric backup… menuExportFabricBackup diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_catalog.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_catalog.py new file mode 100644 index 0000000..c808e8d --- /dev/null +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_catalog.py @@ -0,0 +1,356 @@ +"""Indigo device → exportable Matter role(s), per PRD-indigo-matter-export §5.2. + +This is the inverse of the inbound mapping and it is **not symmetric**. Inbound, +Matter tells us what a device is. Outbound, Indigo does not: a `relay` may be a +lamp, a plug, a lock, a valve, a fan or a garage door and nothing in the device +model distinguishes them. So this module answers a narrower question than "what +is it?" — it answers **"which roles may the user legitimately declare for it, +and which is the safest default?"** (§5.2: "the role is user-declared in the +§5.1 detail pane, defaulting to the safest interpretation rather than +guessing"). Roles the v1 bridge cannot honour are simply never offered, and a +device with no honourable role is :class:`Excluded` **with a reason** — XAC9 +requires the picker to show excluded devices, not hide them. + +Two structural notes: + +* **The loop guard is `pluginId`, nothing else** (XNG3/XAC6). A device this + plugin created is excluded before its type is even looked at. The comparison + is against a plugin id passed in by the caller, never a device-type-name + string, because type names are the sort of thing that gets refactored. +* **Type dispatch is by class-name chain**, not ``isinstance``. The Indigo + module is a MagicMock under test (``tests/conftest.py``) so ``isinstance`` + against ``indigo.RelayDevice`` is not merely wrong there, it raises. Walking + ``type(dev).__mro__`` for the documented IOM class names works identically + against real Indigo objects and the test doubles, and it resolves the + Dimmer-is-a-Relay ambiguity by most-specific-first ordering. + +Every role emitted here is in the BRIDGE_PROTOCOL §4.2 enum +(``bridge_protocol.ROLES``); ``tests/test_export_catalog.py`` pins that. +""" +from __future__ import annotations + +from dataclasses import dataclass +from typing import Optional, Union + +#: This plugin's bundle id (``Info.plist`` ``CFBundleIdentifier``). Only a +#: fallback: callers pass ``self.pluginId`` so the guard follows the running +#: plugin rather than a constant that could drift from the bundle. +DEFAULT_PLUGIN_ID = "com.simons-plugins.indigo-matter" + +# -------------------------------------------------------------------------- +# Roles (BRIDGE_PROTOCOL §4.2) +# -------------------------------------------------------------------------- +ROLE_ON_OFF_PLUG = "onOffPlugInUnit" +ROLE_ON_OFF_LIGHT = "onOffLight" +ROLE_DOOR_LOCK = "doorLock" +ROLE_DIMMABLE_LIGHT = "dimmableLight" +ROLE_COLOR_TEMPERATURE_LIGHT = "colorTemperatureLight" +ROLE_EXTENDED_COLOR_LIGHT = "extendedColorLight" +ROLE_WINDOW_COVERING = "windowCovering" +ROLE_OCCUPANCY_SENSOR = "occupancySensor" +ROLE_CONTACT_SENSOR = "contactSensor" +ROLE_TEMPERATURE_SENSOR = "temperatureSensor" +ROLE_HUMIDITY_SENSOR = "humiditySensor" +ROLE_LIGHT_SENSOR = "lightSensor" +ROLE_PRESSURE_SENSOR = "pressureSensor" +ROLE_FLOW_SENSOR = "flowSensor" +ROLE_THERMOSTAT = "thermostat" + +#: Human labels for the role picker. Keyed by the §4.2 role name. +ROLE_LABELS = { + ROLE_ON_OFF_PLUG: "Plug (On/Off Plug-in Unit)", + ROLE_ON_OFF_LIGHT: "Light (On/Off)", + ROLE_DOOR_LOCK: "Lock (Door Lock)", + ROLE_DIMMABLE_LIGHT: "Light (Dimmable)", + ROLE_COLOR_TEMPERATURE_LIGHT: "Light (Colour temperature)", + ROLE_EXTENDED_COLOR_LIGHT: "Light (Full colour)", + ROLE_WINDOW_COVERING: "Window covering", + ROLE_OCCUPANCY_SENSOR: "Occupancy sensor", + ROLE_CONTACT_SENSOR: "Contact sensor", + ROLE_TEMPERATURE_SENSOR: "Temperature sensor", + ROLE_HUMIDITY_SENSOR: "Humidity sensor", + ROLE_LIGHT_SENSOR: "Light (lux) sensor", + ROLE_PRESSURE_SENSOR: "Pressure sensor", + ROLE_FLOW_SENSOR: "Flow sensor", + ROLE_THERMOSTAT: "Thermostat", +} + +# -------------------------------------------------------------------------- +# Exclusion reasons — user-facing strings; they are shown in the picker (XAC9) +# -------------------------------------------------------------------------- +REASON_LOOP_GUARD = "created by this plugin (loop guard)" +REASON_FAN = "fan export is v2 (matter.js FanControl is a stub)" +REASON_SPRINKLER = "Matter has no irrigation type; per-zone valve export is v2" +REASON_MULTI_IO = "no coherent single-accessory representation" +REASON_NO_ROLE = "no resolvable Matter role" +REASON_SENSOR_UNITS = "no faithful Matter sensor type" +REASON_SENSOR_NO_VALUE = "sensor reports neither an on/off state nor a value" + +#: Roles a user might reasonably expect and the reason v1 does not offer them +#: (§5.2 "Explicitly not exportable in v1"). Indigo cannot tell us a relay is a +#: valve or a garage door, so these are declined as *roles*, not as device +#: classes — the device itself stays exportable as a plug/light/lock. +EXCLUDED_ROLES = { + "valve": "valve export is v2 (matter.js ValveConfigurationAndControl is an empty stub)", + "garageDoor": "garage-door export is v2 (needs the catalog's role/polarity data; " + "mis-mapping is a physical-safety issue)", + "fan": REASON_FAN, +} + +#: The numeric sensor roles, in §5.2 table order. All of them are offered for +#: any numeric sensor; the unit heuristic only picks the *default* (see +#: :func:`_numeric_sensor_role`). +NUMERIC_SENSOR_ROLES = ( + ROLE_TEMPERATURE_SENSOR, ROLE_HUMIDITY_SENSOR, ROLE_LIGHT_SENSOR, + ROLE_PRESSURE_SENSOR, ROLE_FLOW_SENSOR, +) + +#: Unit hints → role, **first match wins**. Ordered so unambiguous words beat +#: broad ones ("humidity" before "temp", which matches half the sensors in a +#: typical database). Indigo has no canonical sensor-unit property, so the +#: hints are gathered from pluginProps, the formatted UI value and the device's +#: own naming — see :func:`_unit_text`. +_UNIT_PATTERNS = ( + (ROLE_HUMIDITY_SENSOR, ("%rh", "rh%", "humidity", "moisture")), + (ROLE_LIGHT_SENSOR, ("lux", " lx", "illuminance", "light level", "luminance")), + (ROLE_PRESSURE_SENSOR, ("kpa", "hpa", "mbar", "millibar", "psi", "inhg", "pressure")), + (ROLE_FLOW_SENSOR, ("m3/h", "m³/h", "l/min", "lpm", "gpm", "flow rate", "flow")), + (ROLE_TEMPERATURE_SENSOR, ("°c", "°f", "degc", "degf", "celsius", "fahrenheit", + "temperature", "temp")), +) + +#: pluginProps keys plugins commonly use to record a sensor's unit. +_UNIT_PROP_KEYS = ("unit", "units", "sensorUnits", "unitOfMeasure", "uiUnits", "sensorType") + +# -------------------------------------------------------------------------- +# Type dispatch — IOM class names, most specific first +# -------------------------------------------------------------------------- +KIND_DIMMER = "DimmerDevice" +KIND_SENSOR = "SensorDevice" +KIND_THERMOSTAT = "ThermostatDevice" +KIND_SPRINKLER = "SprinklerDevice" +KIND_SPEED_CONTROL = "SpeedControlDevice" +KIND_MULTI_IO = "MultiIODevice" +KIND_RELAY = "RelayDevice" + +#: Resolution order. ``RelayDevice`` is last because several IOM subclasses +#: inherit from it — a dimmer, and on some Indigo versions a speed control, +#: are relays too, and the specific reading must win. +_KIND_PRIORITY = ( + KIND_DIMMER, KIND_SENSOR, KIND_THERMOSTAT, KIND_SPRINKLER, + KIND_SPEED_CONTROL, KIND_MULTI_IO, KIND_RELAY, +) + + +@dataclass(frozen=True) +class EligibleDevice: + """A device the user may export, and the roles they may declare for it. + + ``eligible_roles`` is ordered with ``default_role`` first, so the role + picker's first option is also its safest one. + """ + + eligible_roles: tuple[str, ...] + default_role: str + + +@dataclass(frozen=True) +class Excluded: + """A device v1 will not export, and the reason shown in the picker (XAC9).""" + + reason: str + + +Verdict = Union[EligibleDevice, Excluded] + + +def device_kind(dev) -> str: + """The most specific known IOM class name in ``dev``'s ancestry, or ``""``.""" + try: + names = {klass.__name__ for klass in type(dev).__mro__} + except (AttributeError, TypeError): + # An exotic proxy without a normal MRO is simply "unknown", not fatal. + return "" + for kind in _KIND_PRIORITY: + if kind in names: + return kind + return "" + + +def classify(dev, plugin_id: str = DEFAULT_PLUGIN_ID) -> Verdict: + """Classify one Indigo device for export (§5.2). + + ``plugin_id`` is the running plugin's own id — a device carrying it is + excluded first, before any type reasoning, which is what makes the loop + guard structural (XNG3/XAC6) rather than a downstream check that a future + edit could route around. + """ + if _plugin_id_of(dev) == plugin_id: + return Excluded(REASON_LOOP_GUARD) + handler = _BY_KIND.get(device_kind(dev)) + if handler is None: + return Excluded(REASON_NO_ROLE) + return handler(dev) + + +def is_exportable(dev, plugin_id: str = DEFAULT_PLUGIN_ID) -> bool: + """True if :func:`classify` yields an :class:`EligibleDevice`.""" + return isinstance(classify(dev, plugin_id), EligibleDevice) + + +def role_label(role: str) -> str: + """Human label for a §4.2 role (falls back to the role name itself).""" + return ROLE_LABELS.get(role, role) + + +# -------------------------------------------------------------------------- +# Per-class rules (§5.2 table) +# -------------------------------------------------------------------------- +def _relay(_dev) -> Verdict: + """Relay → plug (default), light or lock. + + Valve, garage door and fan are §5.2 exclusions and so are absent from the + offered roles; :data:`EXCLUDED_ROLES` carries the reasons for the docs and + for anyone re-deriving this list. + """ + return EligibleDevice( + eligible_roles=(ROLE_ON_OFF_PLUG, ROLE_ON_OFF_LIGHT, ROLE_DOOR_LOCK), + default_role=ROLE_ON_OFF_PLUG, + ) + + +def _dimmer(dev) -> Verdict: + """Dimmer → dimmable light, upgraded by colour capability; or a covering.""" + supports_rgb = _flag(dev, "supportsRGB") + supports_white_temp = _flag(dev, "supportsWhiteTemperature") + if supports_rgb: + default = ROLE_EXTENDED_COLOR_LIGHT + elif supports_white_temp: + default = ROLE_COLOR_TEMPERATURE_LIGHT + else: + default = ROLE_DIMMABLE_LIGHT + roles = [ROLE_DIMMABLE_LIGHT] + if supports_rgb: + roles.append(ROLE_EXTENDED_COLOR_LIGHT) + if supports_white_temp or supports_rgb: + # An Extended Color Light is a superset; offering the colour-temp-only + # role as well lets a user downgrade a bulb an ecosystem renders badly. + roles.append(ROLE_COLOR_TEMPERATURE_LIGHT) + roles.append(ROLE_WINDOW_COVERING) + return EligibleDevice(eligible_roles=_default_first(roles, default), default_role=default) + + +def _sensor(dev) -> Verdict: + """Sensor → binary (occupancy/contact) or numeric (by unit heuristic).""" + if _flag(dev, "supportsOnState"): + return EligibleDevice( + eligible_roles=(ROLE_OCCUPANCY_SENSOR, ROLE_CONTACT_SENSOR), + default_role=ROLE_OCCUPANCY_SENSOR, + ) + if _flag(dev, "supportsSensorValue"): + default = _numeric_sensor_role(dev) + if default is None: + return Excluded(REASON_SENSOR_UNITS) + return EligibleDevice( + eligible_roles=_default_first(list(NUMERIC_SENSOR_ROLES), default), + default_role=default, + ) + return Excluded(REASON_SENSOR_NO_VALUE) + + +def _thermostat(_dev) -> Verdict: + """Thermostat → Thermostat. No fan in v1 (the FanControl descope).""" + return EligibleDevice(eligible_roles=(ROLE_THERMOSTAT,), default_role=ROLE_THERMOSTAT) + + +def _speed_control(_dev) -> Verdict: + return Excluded(REASON_FAN) + + +def _sprinkler(_dev) -> Verdict: + return Excluded(REASON_SPRINKLER) + + +def _multi_io(_dev) -> Verdict: + return Excluded(REASON_MULTI_IO) + + +_BY_KIND = { + KIND_RELAY: _relay, + KIND_DIMMER: _dimmer, + KIND_SENSOR: _sensor, + KIND_THERMOSTAT: _thermostat, + KIND_SPEED_CONTROL: _speed_control, + KIND_SPRINKLER: _sprinkler, + KIND_MULTI_IO: _multi_io, +} + + +# -------------------------------------------------------------------------- +# Helpers +# -------------------------------------------------------------------------- +def _plugin_id_of(dev) -> str: + value = getattr(dev, "pluginId", "") + return value if isinstance(value, str) else "" + + +def _flag(dev, name: str) -> bool: + """Read an Indigo capability flag defensively (absent → False). + + Only ever ``True`` for a real boolean-ish value: a MagicMock attribute is + truthy for *everything*, so a bare ``bool()`` on an unset attribute of a + mocked device would silently classify it as capable. + """ + value = getattr(dev, name, False) + return value is True or value == 1 + + +def _default_first(roles: list[str], default: str) -> tuple[str, ...]: + """``roles`` with ``default`` moved to the front, duplicates removed.""" + ordered = [default] + [role for role in roles if role != default] + seen: list[str] = [] + for role in ordered: + if role not in seen: + seen.append(role) + return tuple(seen) + + +def _unit_text(dev) -> str: + """Everything about a device that might name its unit, lower-cased. + + Indigo carries no canonical unit property on ``SensorDevice`` — plugins + record it in their own props, and the only universal surface is the + formatted display value (``displayStateValUi``, e.g. ``"72.3 °F"``). So we + read the likely props, then the formatted value, then the device's own + naming, and match keywords over the lot. + """ + parts: list[str] = [] + props = getattr(dev, "pluginProps", None) + if isinstance(props, dict): + for key in _UNIT_PROP_KEYS: + value = props.get(key) + if isinstance(value, str): + parts.append(value) + for attr in ("displayStateValUi", "model", "subModel", "name"): + value = getattr(dev, attr, None) + if isinstance(value, str): + parts.append(value) + return " ".join(parts).lower() + + +def _numeric_sensor_role(dev) -> Optional[str]: + """Best-guess role for a numeric sensor, or ``None`` if nothing matches. + + ``None`` is the §5.2 "sensors with units outside the table" exclusion. The + guess is only a *default*: all five numeric roles are offered so a user can + correct it, which is the same "user-declared role" posture §5.2 takes for + relays. + """ + text = _unit_text(dev) + if not text: + return None + for role, needles in _UNIT_PATTERNS: + for needle in needles: + if needle in text: + return role + return None diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py new file mode 100644 index 0000000..3893438 --- /dev/null +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py @@ -0,0 +1,253 @@ +"""The Matter export allow-list — the model behind `Manage Matter Exports…`. + +Policy is fixed by ADR-0006 E2 and ``docs/PRD-indigo-matter-export.md`` §5.1: +an **explicit allow-list, default empty** (XG5). Export needs per-device +metadata beyond a boolean — role (§5.2 / BRIDGE_PROTOCOL §4.2), a display-name +override, and polarity for covering-like devices — so an entry is a small +record, not a device id. + +Persistence is one JSON string in ``pluginPrefs`` under ``matterExports`` +(PRD §4.3: "the plugin owns the allow-list and per-export metadata in plugin +prefs, backed up with Indigo's database"). Direct ``pluginPrefs[key] = value`` +writes persist immediately — the pattern the rest of this plugin already uses. + +Two disciplines worth knowing before editing: + +* **The lock is re-entrant on purpose.** From E3 ``deviceUpdated`` reads the + allow-list on Indigo's thread while the menu callbacks write it on the UI + thread, and the public methods call each other (``upsert`` re-reads through + ``all()`` to build the payload it persists). Same shape as + ``device_sync.DeviceSync``'s index lock. +* **Corrupt config is preserved, never discarded.** A blob we cannot parse is + moved aside to ``matterExports.corrupt`` and the store starts empty, so a + bad write (or a hand-edited ``.indiPref``) costs the user a rebuild, not a + silent, unrecoverable loss of every export they configured. + +No Indigo import: the store takes a prefs-like mapping so it unit-tests +against a plain dict. +""" +from __future__ import annotations + +import json +import threading +from dataclasses import dataclass, field +from typing import Iterable, Optional + +from bridge_protocol import ROLES + +#: pluginPrefs key holding the serialised allow-list. +PREF_KEY = "matterExports" +#: pluginPrefs key a blob we could not parse is moved aside to (forensics). +PREF_KEY_CORRUPT = "matterExports.corrupt" + +#: Schema version of the serialised payload. Bump when the entry shape changes. +SCHEMA_VERSION = 1 + +KEY_VERSION = "v" +KEY_EXPORTS = "exports" +KEY_DEVICE_ID = "indigoDeviceId" +KEY_ROLE = "role" +KEY_NAME_OVERRIDE = "nameOverride" +KEY_OPTIONS = "options" + +#: ``options`` key carrying window-covering polarity (PRD §5.2 / §4.1). +OPTION_INVERT = "invert" + + +@dataclass(frozen=True) +class ExportEntry: + """One allow-listed device and the metadata Indigo cannot supply. + + ``indigo_device_id`` is the identity key everywhere — in this store, in + the bridge protocol (§4.1) and in the node's endpoint map (PRD §4.3). It + is never re-keyed on name or list position. + """ + + indigo_device_id: int + role: str + name_override: Optional[str] = None + options: dict = field(default_factory=dict) + + def to_dict(self) -> dict: + """The persisted shape (one element of ``exports``).""" + return { + KEY_DEVICE_ID: int(self.indigo_device_id), + KEY_ROLE: self.role, + KEY_NAME_OVERRIDE: self.name_override, + KEY_OPTIONS: dict(self.options), + } + + @classmethod + def from_dict(cls, raw: object) -> "ExportEntry": + """Rebuild an entry, raising ``ValueError`` on anything unusable. + + Validation is deliberately strict — an entry with an unknown role + would be rejected by the bridge node with ``unknown_role`` (§1.1) + long after the user could connect it to what they did. + """ + if not isinstance(raw, dict): + raise ValueError(f"export entry is not an object: {raw!r}") + try: + device_id = int(raw[KEY_DEVICE_ID]) + except (KeyError, TypeError, ValueError) as exc: + raise ValueError(f"export entry has no usable {KEY_DEVICE_ID}: {raw!r}") from exc + role = raw.get(KEY_ROLE) + if role not in ROLES: + raise ValueError(f"export entry has unknown role {role!r} (device {device_id})") + name_override = raw.get(KEY_NAME_OVERRIDE) + if name_override is not None and not isinstance(name_override, str): + raise ValueError(f"export entry name override is not a string (device {device_id})") + options = raw.get(KEY_OPTIONS) or {} + if not isinstance(options, dict): + raise ValueError(f"export entry options are not an object (device {device_id})") + return cls( + indigo_device_id=device_id, + role=role, + name_override=name_override or None, + options=dict(options), + ) + + def label_for(self, device_name: str) -> str: + """The Bridged Device Basic Information ``NodeLabel`` for this export (§4.1).""" + return self.name_override or device_name + + +class ExportStore: + """Thread-safe CRUD over the allow-list, persisted to plugin prefs. + + ``prefs`` is any mutable mapping; in the plugin it is ``self.pluginPrefs``. + Every mutation writes through immediately — there is no flush to forget. + """ + + def __init__(self, prefs, logger) -> None: + self._prefs = prefs + self._logger = logger + # Re-entrant: public methods call one another, and E3's deviceUpdated + # reads from Indigo's thread while the menu writes from the UI's. + self._lock = threading.RLock() + self._entries: dict[int, ExportEntry] = {} + self._load() + + # ------------------------------------------------------------------ + # Reads + # ------------------------------------------------------------------ + def all(self) -> tuple[ExportEntry, ...]: + """Every entry, ordered by device id — an immutable snapshot.""" + with self._lock: + return tuple(self._entries[key] for key in sorted(self._entries)) + + def ids(self) -> frozenset[int]: + """The allow-listed device ids, for O(1) membership on the hot path.""" + with self._lock: + return frozenset(self._entries) + + def get(self, device_id: int) -> Optional[ExportEntry]: + """The entry for ``device_id``, or ``None`` if it is not exported.""" + with self._lock: + return self._entries.get(int(device_id)) + + def __len__(self) -> int: + with self._lock: + return len(self._entries) + + def __contains__(self, device_id: object) -> bool: + try: + key = int(device_id) # type: ignore[arg-type] + except (TypeError, ValueError): + return False + with self._lock: + return key in self._entries + + # ------------------------------------------------------------------ + # Writes + # ------------------------------------------------------------------ + def upsert(self, entry: ExportEntry) -> ExportEntry: + """Add ``entry`` or replace the existing one for the same device id.""" + with self._lock: + self._entries[int(entry.indigo_device_id)] = entry + self._save() + return entry + + def remove(self, device_id: int) -> bool: + """Drop ``device_id`` from the allow-list. True if it was there.""" + with self._lock: + existed = self._entries.pop(int(device_id), None) is not None + if existed: + self._save() + return existed + + def replace_all(self, entries: Iterable[ExportEntry]) -> None: + """Replace the whole allow-list in one persisted write.""" + with self._lock: + self._entries = {int(e.indigo_device_id): e for e in entries} + self._save() + + # ------------------------------------------------------------------ + # Persistence + # ------------------------------------------------------------------ + def _save(self) -> None: + with self._lock: + payload = { + KEY_VERSION: SCHEMA_VERSION, + KEY_EXPORTS: [entry.to_dict() for entry in self.all()], + } + self._prefs[PREF_KEY] = json.dumps(payload) + + def _load(self) -> None: + raw = self._prefs.get(PREF_KEY) + if not raw: + self._entries = {} + return + try: + payload = json.loads(raw) + except (TypeError, ValueError) as exc: + self._corrupt(raw, f"allow-list is not valid JSON ({exc})") + return + if not isinstance(payload, dict): + self._corrupt(raw, "allow-list is not a JSON object") + return + version = payload.get(KEY_VERSION) + if version != SCHEMA_VERSION: + # A future version is not ours to reinterpret, and neither is a + # missing one — both are moved aside rather than guessed at. + self._corrupt(raw, f"allow-list schema version {version!r} != {SCHEMA_VERSION}") + return + listed = payload.get(KEY_EXPORTS) + if not isinstance(listed, list): + self._corrupt(raw, f"allow-list {KEY_EXPORTS!r} is not a list") + return + entries: dict[int, ExportEntry] = {} + dropped = 0 + for item in listed: + try: + entry = ExportEntry.from_dict(item) + except ValueError as exc: + # One bad row must not cost the user every other export, but it + # is never silent, and the original blob is kept either way. + dropped += 1 + self._logger.error("Matter export allow-list: dropping an unusable entry — %s", exc) + continue + entries[entry.indigo_device_id] = entry + self._entries = entries + if dropped: + self._preserve(raw) + self._logger.debug("Matter export allow-list loaded: %d entries (%d dropped)", + len(entries), dropped) + + def _corrupt(self, raw, why: str) -> None: + """Start empty, but keep the blob — user config is never discarded.""" + self._entries = {} + self._logger.error( + "Matter export allow-list unreadable (%s). Starting with an EMPTY export list; " + "the previous value is preserved in the %r plugin pref for recovery.", + why, PREF_KEY_CORRUPT, + ) + self._preserve(raw) + + def _preserve(self, raw) -> None: + try: + self._prefs[PREF_KEY_CORRUPT] = raw if isinstance(raw, str) else repr(raw) + except Exception as exc: # pylint: disable=broad-except + # Preservation is best-effort: whatever the prefs mapping does, it + # must not turn an unreadable allow-list into a failed startup. + self._logger.exception(exc) diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py index 27f1caa..a554618 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py @@ -26,6 +26,8 @@ from async_runtime import AsyncRuntime from commission_jobs import CommissionJobs, node_id_to_str from device_sync import DeviceSync +import export_catalog +from export_store import ExportEntry, ExportStore, OPTION_INVERT from http_handlers import HttpApi, MatterUnavailable from matter_client import MatterClient from matter_handlers.boolean_state_config import ( @@ -41,6 +43,20 @@ COMMAND_TIMEOUT = 5.0 DECOMMISSION_TIMEOUT = 15.0 +#: Menu id of the export dialog (MenuItems.xml) — matched in +#: ``get_menu_action_config_ui_values`` so other menus are never seeded. +MENU_MANAGE_EXPORTS = "manageMatterExports" +#: Option-id prefix marking a picker row the user may look at but not choose +#: (PRD §5.2: excluded devices are shown *with a reason*, never hidden — XAC9). +EXCLUDED_OPTION_PREFIX = "x-" +#: The "nothing selected" / "list truncated" sentinel. Never "": Indigo rejects +#: an empty list id with "UI dynamic list function returned illegal ID string" +#: and silently drops the option. +NO_SELECTION_ID = "0" +#: Picker cap. Past this the tail row asks the user to narrow the filter — a +#: 2000-device database would otherwise build an unusable popup menu. +EXPORT_PICKER_LIMIT = 300 + def server_location(prefs: dict) -> str: """Resolve the one user-facing choice: is matter-server on this Mac? @@ -100,6 +116,10 @@ def __init__(self, plugin_id, plugin_display_name, plugin_version, plugin_prefs, self.jobs: CommissionJobs | None = None self.http: HttpApi | None = None self.server_process: ServerProcess | None = None + # The export allow-list (PRD §5.1). Built in startup, before anything + # can consult it; None means "the plugin has not started yet", which + # every export callback checks rather than assuming. + self.exports: ExportStore | None = None self._install_thread: threading.Thread | None = None self._stopping = False # When WE restart matter-server (menu / post-install), the client sees a brief @@ -145,6 +165,15 @@ def startup(self) -> None: self.pluginPrefs["serverLocation"] = location self.pluginPrefs["manageLaunchAgent"] = managed + # Load the export allow-list first: it is pure prefs I/O, and E3's + # bridge wiring will need it already populated when it starts. An + # unreadable list must not stop the (inbound) plugin starting, so + # ExportStore degrades to empty and preserves the blob rather than + # raising — see export_store._corrupt. + self.exports = ExportStore(self.pluginPrefs, self.logger) + if len(self.exports): + self.logger.info("Matter export allow-list: %d device(s) exported", len(self.exports)) + self.runtime = AsyncRuntime(self.logger) self.runtime.start() @@ -1033,6 +1062,289 @@ def menuDecommissionDevice(self, valuesDict, menuId=""): # noqa: N802, ARG002 "or reconnect). Retry once the device is powered and reachable.") return (False, valuesDict, errors) + # ------------------------------------------------------------------ + # Matter export allow-list — the "Manage Matter Exports…" dialog + # (PRD-indigo-matter-export §5.1 UI-D; roles per BRIDGE_PROTOCOL §4.2) + # ------------------------------------------------------------------ + def _export_plugin_id(self) -> str: + """This plugin's id, for the loop guard (XNG3/XAC6). + + Read from the running plugin rather than hardcoded, so the guard can + never drift from the bundle it is protecting; the catalog constant is + only the fallback for a plugin object built without one (tests). + """ + return getattr(self, "pluginId", "") or export_catalog.DEFAULT_PLUGIN_ID + + @staticmethod + def _truthy(value) -> bool: + """Indigo checkboxes arrive as bools or as "true"/"false" strings.""" + if isinstance(value, str): + return value.strip().lower() in ("true", "yes", "1") + return bool(value) + + @staticmethod + def _indigo_device(device_id): + """``indigo.devices[device_id]`` or None — a stale id is never fatal.""" + try: + return indigo.devices[int(device_id)] + except Exception: # noqa: BLE001 - KeyError, ValueError, or Indigo's own + return None + + def _export_selection(self, values_dict) -> tuple[str, int]: + """Decode the picker value into ``(kind, device_id)``. + + ``kind`` is ``"none"`` (nothing chosen, or the truncation row), + ``"excluded"`` (an ``x-`` row the user may see but not pick), or + ``"device"``. + """ + raw = str((values_dict or {}).get("exportDevice", "") or "") + if not raw or raw == NO_SELECTION_ID: + return ("none", 0) + excluded = raw.startswith(EXCLUDED_OPTION_PREFIX) + if excluded: + raw = raw[len(EXCLUDED_OPTION_PREFIX):] + try: + device_id = int(raw) + except (TypeError, ValueError): + return ("none", 0) + return ("excluded" if excluded else "device", device_id) + + def _export_summary(self) -> str: + if self.exports is None: + return "Plugin still starting — reopen this dialog in a moment." + count = len(self.exports) + if not count: + return "Nothing is exported yet." + return f"{count} device(s) exported." + + def get_menu_action_config_ui_values(self, menu_id): + """Seed the export dialog (menu dialogs never remember their values). + + Only the export menu is seeded — this callback fires for EVERY menu + item that has a ConfigUI, and returning values for another one would + overwrite its defaults. + """ + values = indigo.Dict() + if menu_id != MENU_MANAGE_EXPORTS: + return values + values["exportFilter"] = "" + values["exportDevice"] = NO_SELECTION_ID + values["exportRole"] = "" + values["exportName"] = "" + values["exportInvert"] = False + values["exportStatus"] = self._export_summary() + return values + + def getExportCandidates(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002, ARG002 + """Picker rows: every Indigo device, exportable or not (XAC9). + + Excluded devices are listed **with the reason in the label** and an + ``x-``-prefixed id so the callbacks can reject the pick cleanly — + hiding them would leave a user hunting for a device that will never + appear. ``filter`` here is the XML's static filter attribute, NOT the + user's text: textfields have no callbacks, so the typed filter arrives + in ``valuesDict`` and the Apply-filter button drives the reload. + """ + try: + text = str((valuesDict or {}).get("exportFilter", "") or "").strip().lower() + exported = self.exports.ids() if self.exports is not None else frozenset() + plugin_id = self._export_plugin_id() + options: list[tuple[str, str]] = [] + truncated = 0 + for dev in indigo.devices: + name = str(getattr(dev, "name", "") or "") + if text and text not in name.lower(): + continue + if len(options) >= EXPORT_PICKER_LIMIT: + truncated += 1 + continue + verdict = export_catalog.classify(dev, plugin_id) + if isinstance(verdict, export_catalog.Excluded): + options.append((f"{EXCLUDED_OPTION_PREFIX}{dev.id}", + f"{name} — not exportable: {verdict.reason}")) + else: + mark = "● " if dev.id in exported else "" + options.append((str(dev.id), f"{mark}{name}")) + if truncated: + options.append((NO_SELECTION_ID, + f"… {truncated} more — narrow the filter")) + if not options: + options.append((NO_SELECTION_ID, "(no devices match the filter)")) + return options + except Exception as exc: # noqa: BLE001 - never break the dialog + self.logger.exception(exc) + return [] + + def getExportRoles(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002, ARG002 + """Roles the picked device may legitimately be exported as (§5.2). + + Empty for no selection or an excluded pick — an empty role menu is the + honest rendering of "there is nothing you may choose here". + """ + try: + kind, device_id = self._export_selection(valuesDict) + if kind != "device": + return [] + dev = self._indigo_device(device_id) + if dev is None: + return [] + verdict = export_catalog.classify(dev, self._export_plugin_id()) + if isinstance(verdict, export_catalog.Excluded): + return [] + return [(role, export_catalog.role_label(role)) for role in verdict.eligible_roles] + except Exception as exc: # noqa: BLE001 - never break the dialog + self.logger.exception(exc) + return [] + + def getCurrentExports(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002, ARG002 + """Read-only summary of the allow-list (one row per export).""" + try: + if self.exports is None: + return [(NO_SELECTION_ID, "(plugin still starting)")] + options = [] + for entry in self.exports.all(): + dev = self._indigo_device(entry.indigo_device_id) + name = str(getattr(dev, "name", "") or "") if dev is not None else "" + if not name: + name = f"(deleted device {entry.indigo_device_id})" + label = f"{name} → {export_catalog.role_label(entry.role)}" + if entry.name_override: + label += f' · shown as "{entry.name_override}"' + if entry.options.get(OPTION_INVERT): + label += " · inverted" + options.append((str(entry.indigo_device_id), label)) + return options or [(NO_SELECTION_ID, "(nothing exported yet)")] + except Exception as exc: # noqa: BLE001 - never break the dialog + self.logger.exception(exc) + return [] + + def exportReloadPicker(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 + """Apply-filter button: the return trip is what reloads the lists.""" + values = valuesDict + text = str(values.get("exportFilter", "") or "").strip() + values["exportStatus"] = (f'Filtered on "{text}". {self._export_summary()}' if text + else self._export_summary()) + return values + + def exportDeviceChanged(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 + """Picker selection changed: load that device's saved export, or defaults. + + Menu callbacks return a valuesDict, not an error dict (the SDK's menu + contract), so an excluded pick is reported in the read-only status + field here and *rejected* — with a real field error — by the + Add/update button below. Both paths are covered by tests. + """ + values = valuesDict + kind, device_id = self._export_selection(values) + if kind == "none": + values["exportRole"] = "" + values["exportName"] = "" + values["exportInvert"] = False + values["exportStatus"] = self._export_summary() + return values + dev = self._indigo_device(device_id) + if kind == "excluded" or dev is None: + reason = "that device no longer exists" + if dev is not None: + verdict = export_catalog.classify(dev, self._export_plugin_id()) + reason = verdict.reason if isinstance(verdict, export_catalog.Excluded) \ + else "that device is not exportable" + values["exportRole"] = "" + values["exportName"] = "" + values["exportInvert"] = False + values["exportStatus"] = f"Not exportable: {reason}" + return values + verdict = export_catalog.classify(dev, self._export_plugin_id()) + if isinstance(verdict, export_catalog.Excluded): + values["exportRole"] = "" + values["exportStatus"] = f"Not exportable: {verdict.reason}" + return values + entry = self.exports.get(device_id) if self.exports is not None else None + if entry is not None: + values["exportRole"] = entry.role + values["exportName"] = entry.name_override or "" + values["exportInvert"] = bool(entry.options.get(OPTION_INVERT, False)) + values["exportStatus"] = f"{dev.name} is exported as {export_catalog.role_label(entry.role)}." + else: + values["exportRole"] = verdict.default_role + values["exportName"] = "" + values["exportInvert"] = False + values["exportStatus"] = f"{dev.name} is not exported yet." + return values + + def exportAddOrUpdate(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 + """Add or update one export. Validates the role against the catalog. + + A role the catalog does not offer for this device is refused here + rather than by the bridge node, which would only reject it with + ``unknown_role``/``role_change`` long after the user could connect the + failure to what they did (BRIDGE_PROTOCOL §1.1). + """ + values = valuesDict + errors = indigo.Dict() + if self.exports is None: + errors["exportDevice"] = "Plugin still starting — try again in a moment." + return (values, errors) + kind, device_id = self._export_selection(values) + if kind == "none": + errors["exportDevice"] = "Select a device to export." + return (values, errors) + dev = self._indigo_device(device_id) + if dev is None: + errors["exportDevice"] = "That device no longer exists — refresh the list." + return (values, errors) + verdict = export_catalog.classify(dev, self._export_plugin_id()) + if kind == "excluded" or isinstance(verdict, export_catalog.Excluded): + reason = verdict.reason if isinstance(verdict, export_catalog.Excluded) \ + else "not exportable" + errors["exportDevice"] = f"{dev.name} cannot be exported: {reason}" + values["exportStatus"] = f"Not exportable: {reason}" + return (values, errors) + role = str(values.get("exportRole", "") or "") + if role not in verdict.eligible_roles: + errors["exportRole"] = ("Choose how this device should appear " + f"({', '.join(verdict.eligible_roles)}).") + return (values, errors) + name_override = str(values.get("exportName", "") or "").strip() or None + options = {} + if role == export_catalog.ROLE_WINDOW_COVERING and self._truthy(values.get("exportInvert")): + options[OPTION_INVERT] = True + existed = device_id in self.exports + self.exports.upsert(ExportEntry( + indigo_device_id=device_id, role=role, + name_override=name_override, options=options, + )) + verb = "Updated" if existed else "Added" + self.logger.info("%s Matter export: %s (id %s) as %s%s", + verb, dev.name, device_id, role, + f' named "{name_override}"' if name_override else "") + values["exportStatus"] = f"{verb} {dev.name} as {export_catalog.role_label(role)}. " \ + f"{self._export_summary()}" + return values + + def exportRemove(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 + """Drop the picked device from the allow-list.""" + values = valuesDict + errors = indigo.Dict() + if self.exports is None: + errors["exportDevice"] = "Plugin still starting — try again in a moment." + return (values, errors) + kind, device_id = self._export_selection(values) + if kind == "none": + errors["exportDevice"] = "Select a device to remove from the export list." + return (values, errors) + if not self.exports.remove(device_id): + errors["exportDevice"] = "That device is not exported." + return (values, errors) + dev = self._indigo_device(device_id) + name = str(getattr(dev, "name", "") or "") if dev is not None else f"device {device_id}" + self.logger.info("Removed Matter export: %s (id %s)", name, device_id) + values["exportRole"] = "" + values["exportName"] = "" + values["exportInvert"] = False + values["exportStatus"] = f"Removed {name}. {self._export_summary()}" + return values + def _resolve_storage_path(self) -> str: """Storage dir path in BOTH managed and manual modes. diff --git a/tests/fakes.py b/tests/fakes.py index 8eb7ba5..0d79e20 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -116,3 +116,122 @@ def _respond(frame: dict) -> list: async def returns(value): """Await-able that yields ``value`` (for the client's connect factory).""" return value + + +# --------------------------------------------------------------------------- +# Fake Indigo devices for the export catalog / picker (PRD §5.1-§5.2) +# --------------------------------------------------------------------------- +# ``export_catalog`` dispatches on the IOM class-name chain, not isinstance +# (the indigo module is a MagicMock in tests — see conftest). So these classes +# are named exactly as Indigo's are and mirror its inheritance: DimmerDevice +# and SpeedControlDevice really are RelayDevice subclasses, which is precisely +# the ambiguity the catalog's most-specific-first ordering has to survive. +# +# Pessimistic discipline (docs/TESTING.md): each class declares only the +# capability flags its real counterpart carries, and ``minimal_device`` builds +# a device with none at all, so the catalog's getattr defaults are exercised +# rather than assumed. + +#: pluginId for a device that is NOT ours — the loop guard must let it through. +OTHER_PLUGIN_ID = "com.example.someone-else" + + +class FakeIndigoDevice: + """Base: the device attributes every Indigo device really has.""" + + def __init__(self, dev_id=1, name="Device", plugin_id=OTHER_PLUGIN_ID, **attrs): + self.id = dev_id + self.name = name + self.pluginId = plugin_id + self.pluginProps = dict(attrs.pop("pluginProps", None) or {}) + self.deviceTypeId = attrs.pop("deviceTypeId", "") + self.displayStateValUi = attrs.pop("displayStateValUi", "") + self.model = attrs.pop("model", "") + self.subModel = attrs.pop("subModel", "") + for key, value in attrs.items(): + setattr(self, key, value) + + +class RelayDevice(FakeIndigoDevice): + """indigo.RelayDevice — on/off only.""" + + def __init__(self, *args, **kwargs): + self.supportsOnState = kwargs.pop("supportsOnState", True) + super().__init__(*args, **kwargs) + + +class DimmerDevice(RelayDevice): + """indigo.DimmerDevice — a relay that also dims, and maybe colours.""" + + def __init__(self, *args, **kwargs): + self.supportsColor = kwargs.pop("supportsColor", False) + self.supportsRGB = kwargs.pop("supportsRGB", False) + self.supportsWhiteTemperature = kwargs.pop("supportsWhiteTemperature", False) + super().__init__(*args, **kwargs) + + +class SpeedControlDevice(RelayDevice): + """indigo.SpeedControlDevice — fans; excluded in v1.""" + + +class SensorDevice(FakeIndigoDevice): + """indigo.SensorDevice — binary (supportsOnState) or numeric (supportsSensorValue).""" + + def __init__(self, *args, **kwargs): + self.supportsOnState = kwargs.pop("supportsOnState", False) + self.supportsSensorValue = kwargs.pop("supportsSensorValue", False) + super().__init__(*args, **kwargs) + + +class ThermostatDevice(FakeIndigoDevice): + """indigo.ThermostatDevice.""" + + +class SprinklerDevice(FakeIndigoDevice): + """indigo.SprinklerDevice — no Matter irrigation type; excluded in v1.""" + + +class MultiIODevice(FakeIndigoDevice): + """indigo.MultiIODevice — no single-accessory representation; excluded.""" + + +class CustomDevice(FakeIndigoDevice): + """A plugin-defined `custom` device — no resolvable Matter role.""" + + +def minimal_device(class_name, dev_id=1, name="Device", plugin_id=OTHER_PLUGIN_ID): + """A device carrying ONLY id/name/pluginId, in a class of ``class_name``. + + Models the pessimistic case: an Indigo object whose capability flags we + must not assume exist. Everything the catalog reads beyond those three + attributes has to fall back cleanly. + """ + klass = type(class_name, (object,), {}) + dev = klass() + dev.id = dev_id + dev.name = name + dev.pluginId = plugin_id + return dev + + +class FakeIndigoDevices: + """Stands in for ``indigo.devices``: iterable, and subscriptable by id.""" + + def __init__(self, devices=()): + self._devices = list(devices) + + def add(self, device): + self._devices.append(device) + return device + + def __iter__(self): + return iter(self._devices) + + def __len__(self): + return len(self._devices) + + def __getitem__(self, device_id): + for device in self._devices: + if device.id == device_id: + return device + raise KeyError(device_id) diff --git a/tests/test_export_catalog.py b/tests/test_export_catalog.py new file mode 100644 index 0000000..914818b --- /dev/null +++ b/tests/test_export_catalog.py @@ -0,0 +1,323 @@ +"""Export-catalog contract harness — one row per PRD §5.2 mapping table row. + +Same shape as `test_device_zoo.py`, for the opposite direction: a table of +device shapes run through the REAL catalog, with structural invariants +asserted over every entry rather than only the individual expectations. + +Invariants: + 1. every row's verdict matches the §5.2 table (role or exclusion); + 2. every emitted role is in the BRIDGE_PROTOCOL §4.2 enum — a role the + bridge node would reject with `unknown_role` must never reach the UI; + 3. `default_role` is always the FIRST eligible role (the picker's first + option must be its safest one); + 4. every exclusion carries a non-empty reason (XAC9 shows it in the picker); + 5. the loop guard beats every other rule, for every device shape (XAC6). +""" +from __future__ import annotations + +import copy +import plistlib +from pathlib import Path + +import pytest + +import export_catalog +from bridge_protocol import ROLES +from export_catalog import EligibleDevice, Excluded, classify +from fakes import ( + OTHER_PLUGIN_ID, + CustomDevice, + DimmerDevice, + MultiIODevice, + RelayDevice, + SensorDevice, + SpeedControlDevice, + SprinklerDevice, + ThermostatDevice, + minimal_device, +) + +OURS = export_catalog.DEFAULT_PLUGIN_ID + +# name → (device, expected verdict). One entry per §5.2 row. +ZOO = { + # --- Relay rows ------------------------------------------------------- + "relay_plug_default": ( + RelayDevice(1, "Study Plug"), + EligibleDevice(("onOffPlugInUnit", "onOffLight", "doorLock"), "onOffPlugInUnit"), + ), + # --- Dimmer rows ------------------------------------------------------ + "dimmer_plain": ( + DimmerDevice(2, "Hall Dimmer"), + EligibleDevice(("dimmableLight", "windowCovering"), "dimmableLight"), + ), + "dimmer_full_colour": ( + DimmerDevice(3, "Lounge Bulb", supportsColor=True, supportsRGB=True, + supportsWhiteTemperature=True), + EligibleDevice(("extendedColorLight", "dimmableLight", "colorTemperatureLight", + "windowCovering"), "extendedColorLight"), + ), + "dimmer_colour_temp_only": ( + DimmerDevice(4, "Reading Lamp", supportsColor=True, supportsWhiteTemperature=True), + EligibleDevice(("colorTemperatureLight", "dimmableLight", "windowCovering"), + "colorTemperatureLight"), + ), + # --- Sensor rows ------------------------------------------------------ + "sensor_binary": ( + SensorDevice(5, "Hall Motion", supportsOnState=True), + EligibleDevice(("occupancySensor", "contactSensor"), "occupancySensor"), + ), + "sensor_temperature": ( + SensorDevice(6, "Study Temperature", supportsSensorValue=True, + displayStateValUi="19.5 °C"), + EligibleDevice(("temperatureSensor", "humiditySensor", "lightSensor", + "pressureSensor", "flowSensor"), "temperatureSensor"), + ), + "sensor_humidity": ( + SensorDevice(7, "Bath RH", supportsSensorValue=True, pluginProps={"unit": "%RH"}), + EligibleDevice(("humiditySensor", "temperatureSensor", "lightSensor", + "pressureSensor", "flowSensor"), "humiditySensor"), + ), + "sensor_lux": ( + SensorDevice(8, "Porch Light Level", supportsSensorValue=True, + pluginProps={"units": "lux"}), + EligibleDevice(("lightSensor", "temperatureSensor", "humiditySensor", + "pressureSensor", "flowSensor"), "lightSensor"), + ), + "sensor_pressure": ( + SensorDevice(9, "Barometer", supportsSensorValue=True, + displayStateValUi="1013 hPa"), + EligibleDevice(("pressureSensor", "temperatureSensor", "humiditySensor", + "lightSensor", "flowSensor"), "pressureSensor"), + ), + "sensor_flow": ( + SensorDevice(10, "Mains Meter", supportsSensorValue=True, + pluginProps={"unit": "l/min"}), + EligibleDevice(("flowSensor", "temperatureSensor", "humiditySensor", + "lightSensor", "pressureSensor"), "flowSensor"), + ), + "sensor_unmappable_units": ( + SensorDevice(11, "Solar Yield", supportsSensorValue=True, + displayStateValUi="4.2 kWh"), + Excluded(export_catalog.REASON_SENSOR_UNITS), + ), + "sensor_neither_state_nor_value": ( + SensorDevice(12, "Odd Sensor"), + Excluded(export_catalog.REASON_SENSOR_NO_VALUE), + ), + # --- Thermostat ------------------------------------------------------- + "thermostat": ( + ThermostatDevice(13, "Hall Stat"), + EligibleDevice(("thermostat",), "thermostat"), + ), + # --- Explicitly not exportable in v1 ---------------------------------- + "speed_control_fan": ( + SpeedControlDevice(14, "Bathroom Fan"), + Excluded(export_catalog.REASON_FAN), + ), + "sprinkler": ( + SprinklerDevice(15, "Irrigation"), + Excluded(export_catalog.REASON_SPRINKLER), + ), + "multi_io": ( + MultiIODevice(16, "IO Board"), + Excluded(export_catalog.REASON_MULTI_IO), + ), + "custom_no_role": ( + CustomDevice(17, "Matter Energy Meter"), + Excluded(export_catalog.REASON_NO_ROLE), + ), + "created_by_this_plugin": ( + RelayDevice(18, "Matter Plug", plugin_id=OURS), + Excluded(export_catalog.REASON_LOOP_GUARD), + ), +} + + +@pytest.mark.parametrize("name", sorted(ZOO)) +def test_zoo_row_matches_the_prd_table(name): + """Invariant 1 — the §5.2 row is what the catalog actually returns.""" + device, expected = ZOO[name] + assert classify(device, OURS) == expected + + +@pytest.mark.parametrize("name", sorted(ZOO)) +def test_zoo_roles_are_all_in_the_protocol_enum(name): + """Invariant 2 — no role the bridge node would reject with unknown_role.""" + device, _expected = ZOO[name] + verdict = classify(device, OURS) + if isinstance(verdict, EligibleDevice): + assert set(verdict.eligible_roles) <= ROLES + + +@pytest.mark.parametrize("name", sorted(ZOO)) +def test_zoo_default_role_is_first_and_eligible(name): + """Invariant 3 — the picker's first option is the safe default.""" + device, _expected = ZOO[name] + verdict = classify(device, OURS) + if isinstance(verdict, EligibleDevice): + assert verdict.eligible_roles[0] == verdict.default_role + assert len(set(verdict.eligible_roles)) == len(verdict.eligible_roles) + + +@pytest.mark.parametrize("name", sorted(ZOO)) +def test_zoo_exclusions_always_carry_a_reason(name): + """Invariant 4 — XAC9: excluded devices show *why*.""" + device, _expected = ZOO[name] + verdict = classify(device, OURS) + if isinstance(verdict, Excluded): + assert verdict.reason.strip() + + +# --------------------------------------------------------------------------- +# XAC6 — the loop guard. Required by name in the PRD's acceptance criteria. +# --------------------------------------------------------------------------- +@pytest.mark.parametrize("name", sorted(ZOO)) +def test_xac6_our_own_devices_are_excluded_whatever_their_type(name): + """Invariant 5 — a device carrying OUR pluginId is excluded, always. + + Run over every zoo shape: whatever a future edit does to the type rules, + the loop guard (XNG3) has to beat all of them. + """ + device = copy.copy(ZOO[name][0]) # never mutate the shared zoo + device.pluginId = OURS + assert classify(device, OURS) == Excluded(export_catalog.REASON_LOOP_GUARD) + + +def test_xac6_guard_follows_the_supplied_plugin_id_not_a_constant(): + """The guard is parameterised — tests (and a renamed bundle) can vary it.""" + device = RelayDevice(1, "Plug", plugin_id="com.example.other-plugin") + assert classify(device, OURS) == EligibleDevice( + ("onOffPlugInUnit", "onOffLight", "doorLock"), "onOffPlugInUnit") + assert classify(device, "com.example.other-plugin") == Excluded( + export_catalog.REASON_LOOP_GUARD) + + +def test_xac6_default_plugin_id_matches_the_bundle_identifier(): + """The fallback constant must never drift from Info.plist.""" + plist = (Path(__file__).parent.parent / "indigo-matter.indigoPlugin" + / "Contents" / "Info.plist") + with plist.open("rb") as handle: + assert plistlib.load(handle)["CFBundleIdentifier"] == export_catalog.DEFAULT_PLUGIN_ID + + +# --------------------------------------------------------------------------- +# Type dispatch — the isinstance-free discipline +# --------------------------------------------------------------------------- +def test_dimmer_wins_over_its_relay_base(): + """DimmerDevice subclasses RelayDevice; the specific reading must win.""" + assert export_catalog.device_kind(DimmerDevice(1, "D")) == "DimmerDevice" + + +def test_speed_control_wins_over_its_relay_base(): + assert export_catalog.device_kind(SpeedControlDevice(1, "F")) == "SpeedControlDevice" + assert classify(SpeedControlDevice(1, "F"), OURS) == Excluded(export_catalog.REASON_FAN) + + +def test_unknown_class_has_no_kind(): + assert export_catalog.device_kind(CustomDevice(1, "C")) == "" + + +def test_device_kind_tolerates_an_exotic_object(): + assert export_catalog.device_kind(object()) == "" + + +# --------------------------------------------------------------------------- +# Pessimistic Indigo: capability flags we cannot assume exist +# --------------------------------------------------------------------------- +def test_minimal_dimmer_degrades_to_plain_dimmable(): + dev = minimal_device("DimmerDevice", 1, "Bare Dimmer") + assert classify(dev, OURS) == EligibleDevice(("dimmableLight", "windowCovering"), + "dimmableLight") + + +def test_minimal_sensor_is_excluded_not_crashed(): + dev = minimal_device("SensorDevice", 1, "Bare Sensor") + assert classify(dev, OURS) == Excluded(export_catalog.REASON_SENSOR_NO_VALUE) + + +def test_minimal_relay_still_classifies(): + dev = minimal_device("RelayDevice", 1, "Bare Relay") + assert isinstance(classify(dev, OURS), EligibleDevice) + + +def test_device_without_a_plugin_id_is_not_our_own(): + dev = minimal_device("RelayDevice", 1, "Hardware Relay", plugin_id=None) + assert isinstance(classify(dev, OURS), EligibleDevice) + + +def test_truthy_but_non_boolean_flags_do_not_count_as_capability(): + """A MagicMock attribute is truthy for everything — only True/1 counts.""" + dev = DimmerDevice(1, "Mocked", supportsRGB=object()) + assert classify(dev, OURS).default_role == "dimmableLight" + + +# --------------------------------------------------------------------------- +# Sensor unit heuristics +# --------------------------------------------------------------------------- +@pytest.mark.parametrize("text,expected", [ + ("19.5 °C", "temperatureSensor"), + ("68.2 °F", "temperatureSensor"), + ("21 degC", "temperatureSensor"), + ("55 %RH", "humiditySensor"), + ("340 lux", "lightSensor"), + ("1013 hPa", "pressureSensor"), + ("2.4 l/min", "flowSensor"), + ("4.2 kWh", None), + ("", None), +]) +def test_unit_heuristic_from_the_formatted_ui_value(text, expected): + dev = SensorDevice(1, "", supportsSensorValue=True, displayStateValUi=text) + verdict = classify(dev, OURS) + if expected is None: + assert verdict == Excluded(export_catalog.REASON_SENSOR_UNITS) + else: + assert verdict.default_role == expected + + +def test_unit_heuristic_falls_back_to_the_device_name(): + dev = SensorDevice(1, "Greenhouse Temperature", supportsSensorValue=True) + assert classify(dev, OURS).default_role == "temperatureSensor" + + +def test_humidity_beats_temperature_in_a_combined_name(): + """First match wins, and 'humidity' is the unambiguous word.""" + dev = SensorDevice(1, "Temperature/Humidity — Humidity", supportsSensorValue=True, + pluginProps={"unit": "%RH"}) + assert classify(dev, OURS).default_role == "humiditySensor" + + +def test_binary_sensor_beats_the_unit_heuristic(): + dev = SensorDevice(1, "Door Temperature", supportsOnState=True, supportsSensorValue=True) + assert classify(dev, OURS).default_role == "occupancySensor" + + +# --------------------------------------------------------------------------- +# Module surface +# --------------------------------------------------------------------------- +def test_every_labelled_role_is_a_protocol_role(): + assert set(export_catalog.ROLE_LABELS) == set(ROLES) + + +def test_excluded_roles_are_documented_with_reasons(): + assert set(export_catalog.EXCLUDED_ROLES) == {"valve", "garageDoor", "fan"} + assert all(reason.strip() for reason in export_catalog.EXCLUDED_ROLES.values()) + + +def test_excluded_roles_are_never_offered(): + offered = set() + for device, _expected in ZOO.values(): + verdict = classify(device, OTHER_PLUGIN_ID) + if isinstance(verdict, EligibleDevice): + offered.update(verdict.eligible_roles) + assert offered.isdisjoint(export_catalog.EXCLUDED_ROLES) + + +def test_is_exportable_mirrors_classify(): + assert export_catalog.is_exportable(RelayDevice(1, "Plug"), OURS) is True + assert export_catalog.is_exportable(SprinklerDevice(2, "Zones"), OURS) is False + + +def test_role_label_falls_back_to_the_role_name(): + assert export_catalog.role_label("onOffLight") == "Light (On/Off)" + assert export_catalog.role_label("somethingNew") == "somethingNew" diff --git a/tests/test_export_menu.py b/tests/test_export_menu.py new file mode 100644 index 0000000..b15c1b5 --- /dev/null +++ b/tests/test_export_menu.py @@ -0,0 +1,551 @@ +"""The "Manage Matter Exports…" dialog — pickers, callbacks and XML shape. + +The dialog is the UI-D compromise (PRD-indigo-matter-export §5.1) forced by +what Indigo's XML dialogs can actually do: a multi-select `list` has no +CallbackMethod, and neither has a `textfield`, so master-detail is a filter +field + a reload button + a SINGLE-select `menu` whose callback loads the +picked device's saved settings. These tests pin the resulting contract, +including the two acceptance criteria the PRD names explicitly: + +* **XAC6** — a device this plugin created is excluded (also unit-tested at the + catalog level in `test_export_catalog.py`); +* **XAC9** — excluded devices appear in the picker *with reasons*, never + silently missing. +""" +from __future__ import annotations + +import importlib +import xml.etree.ElementTree as ET +from pathlib import Path +from unittest.mock import Mock + +import pytest + +import export_catalog +from export_store import OPTION_INVERT, PREF_KEY, ExportEntry, ExportStore +from fakes import ( + OTHER_PLUGIN_ID, + DimmerDevice, + FakeIndigoDevices, + RelayDevice, + SensorDevice, + SprinklerDevice, +) + +MENU_ITEMS_XML = ( + Path(__file__).parent.parent + / "indigo-matter.indigoPlugin" / "Contents" / "Server Plugin" / "MenuItems.xml" +) +OURS = export_catalog.DEFAULT_PLUGIN_ID + + +@pytest.fixture +def plugin_mod(mock_indigo_base): + import plugin as plugin_module + importlib.reload(plugin_module) + return plugin_module + + +@pytest.fixture +def devices(mock_indigo_base): + collection = FakeIndigoDevices([ + RelayDevice(101, "Study Plug"), + DimmerDevice(102, "Hall Dimmer"), + SensorDevice(103, "Hall Motion", supportsOnState=True), + SprinklerDevice(104, "Irrigation"), + RelayDevice(105, "Matter Plug", plugin_id=OURS), + ]) + mock_indigo_base.devices = collection + return collection + + +@pytest.fixture +def plug(plugin_mod, devices): # noqa: ARG001 - devices installs indigo.devices + p = plugin_mod.Plugin.__new__(plugin_mod.Plugin) + p.logger = Mock() + p.pluginId = OURS + p.pluginPrefs = {} + p.exports = ExportStore(p.pluginPrefs, p.logger) + return p + + +def _values(**kwargs): + base = {"exportFilter": "", "exportDevice": "0", "exportRole": "", + "exportName": "", "exportInvert": False, "exportStatus": ""} + base.update(kwargs) + return base + + +def _labels(options): + return {key: label for key, label in options} + + +# --------------------------------------------------------------------------- +# MenuItems.xml shape — the constraints the SDK actually imposes +# --------------------------------------------------------------------------- +def _menu_item(): + root = ET.parse(MENU_ITEMS_XML).getroot() + for item in root.findall("MenuItem"): + if item.get("id") == "manageMatterExports": + return item + raise AssertionError("manageMatterExports menu item missing") + + +def test_menu_item_exists_and_has_no_callback_method(): + """No CallbackMethod → a single Close button, so the buttons do the work.""" + item = _menu_item() + assert item.findtext("Name").startswith("Manage Matter Exports") + assert item.find("CallbackMethod") is None + assert item.find("ConfigUI") is not None + + +def test_dialog_fields_are_present_with_the_expected_types(): + fields = {f.get("id"): f.get("type") for f in _menu_item().findall("./ConfigUI/Field")} + assert fields["exportFilter"] == "textfield" + assert fields["exportReload"] == "button" + assert fields["exportDevice"] == "menu" # menus have CallbackMethod; lists do not + assert fields["exportRole"] == "menu" + assert fields["exportName"] == "textfield" + assert fields["exportInvert"] == "checkbox" + assert fields["exportAdd"] == "button" + assert fields["exportDelete"] == "button" + assert fields["exportStatus"] == "textfield" # readonly: labels cannot change at runtime + assert fields["exportList"] == "list" + + +def test_dynamic_lists_point_at_real_plugin_methods(plug): + for field in _menu_item().findall("./ConfigUI/Field"): + list_el = field.find("List") + if list_el is None or list_el.get("method") is None: + continue + assert list_el.get("class") == "self" + assert list_el.get("dynamicReload") == "true" + assert callable(getattr(plug, list_el.get("method"))) + + +def test_button_and_menu_callbacks_exist_on_the_plugin(plug): + for field in _menu_item().findall("./ConfigUI/Field"): + callback = field.findtext("CallbackMethod") + if callback: + assert callable(getattr(plug, callback)) + + +def test_conditional_field_is_counted_in_the_height_calculation(): + invert = [f for f in _menu_item().findall("./ConfigUI/Field") + if f.get("id") == "exportInvert"][0] + assert invert.get("visibleBindingId") == "exportRole" + assert invert.get("visibleBindingValue") == "windowCovering" + assert invert.get("alwaysUseInDialogHeightCalc") == "true" + + +def test_status_field_is_readonly(): + status = [f for f in _menu_item().findall("./ConfigUI/Field") + if f.get("id") == "exportStatus"][0] + assert status.get("readonly") == "true" + + +# --------------------------------------------------------------------------- +# Seeding (menu dialogs never remember their values) +# --------------------------------------------------------------------------- +def test_seed_values_only_for_the_export_menu(plug): + assert plug.get_menu_action_config_ui_values("manageMatterExports")["exportDevice"] == "0" + assert plug.get_menu_action_config_ui_values("decommissionDevice") == {} + + +def test_seed_reports_the_current_export_count(plug): + assert "Nothing is exported" in plug.get_menu_action_config_ui_values( + "manageMatterExports")["exportStatus"] + plug.exports.upsert(ExportEntry(101, "onOffPlugInUnit")) + assert "1 device(s) exported" in plug.get_menu_action_config_ui_values( + "manageMatterExports")["exportStatus"] + + +# --------------------------------------------------------------------------- +# Device picker — XAC9 and the id contract +# --------------------------------------------------------------------------- +def test_picker_lists_every_device(plug): + labels = _labels(plug.getExportCandidates(valuesDict=_values())) + assert labels["101"] == "Study Plug" + assert labels["102"] == "Hall Dimmer" + + +def test_xac9_excluded_devices_are_listed_with_their_reason(plug): + labels = _labels(plug.getExportCandidates(valuesDict=_values())) + assert "x-104" in labels + assert labels["x-104"].startswith("Irrigation — not exportable: ") + assert export_catalog.REASON_SPRINKLER in labels["x-104"] + + +def test_xac6_our_own_device_is_shown_as_excluded_not_hidden(plug): + """The loop-guarded device stays visible so the user can see WHY.""" + labels = _labels(plug.getExportCandidates(valuesDict=_values())) + assert "105" not in labels # not selectable + assert export_catalog.REASON_LOOP_GUARD in labels["x-105"] + + +def test_picker_ids_are_never_empty_strings(plug): + # Regression class: Indigo drops an option with an empty id + # ("UI dynamic list function returned illegal ID string"). + assert all(key != "" for key, _label in plug.getExportCandidates(valuesDict=_values())) + + +def test_picker_ids_carry_no_commas_or_semicolons(plug): + # Dynamic-list option values may not contain either (SDK dynamic-lists). + for key, _label in plug.getExportCandidates(valuesDict=_values()): + assert "," not in key and ";" not in key + + +def test_picker_marks_already_exported_devices(plug): + plug.exports.upsert(ExportEntry(101, "onOffPlugInUnit")) + labels = _labels(plug.getExportCandidates(valuesDict=_values())) + assert labels["101"] == "● Study Plug" + assert labels["102"] == "Hall Dimmer" + + +def test_picker_filter_is_case_insensitive_substring(plug): + labels = _labels(plug.getExportCandidates(valuesDict=_values(exportFilter="hAlL"))) + assert set(labels) == {"102", "103"} + + +def test_picker_filter_with_no_matches_still_returns_a_legal_option(plug): + options = plug.getExportCandidates(valuesDict=_values(exportFilter="zzz")) + assert options == [("0", "(no devices match the filter)")] + + +def test_picker_caps_the_list_and_offers_a_narrowing_tail(plug, devices, plugin_mod): + for device_id in range(1000, 1000 + plugin_mod.EXPORT_PICKER_LIMIT + 5): + devices.add(RelayDevice(device_id, f"Bulk {device_id}")) + options = plug.getExportCandidates(valuesDict=_values()) + assert len(options) == plugin_mod.EXPORT_PICKER_LIMIT + 1 + tail_id, tail_label = options[-1] + assert tail_id == "0" + assert "narrow the filter" in tail_label + + +def test_picker_degrades_to_empty_on_error(plug, mock_indigo_base): + boom = Mock() + boom.__iter__ = Mock(side_effect=RuntimeError("boom")) + mock_indigo_base.devices = boom + assert plug.getExportCandidates(valuesDict=_values()) == [] + plug.logger.exception.assert_called() + + +def test_picker_works_before_the_store_exists(plug): + plug.exports = None + assert _labels(plug.getExportCandidates(valuesDict=_values()))["101"] == "Study Plug" + + +def test_picker_tolerates_a_missing_values_dict(plug): + assert plug.getExportCandidates() + + +# --------------------------------------------------------------------------- +# Role picker +# --------------------------------------------------------------------------- +def test_role_picker_offers_the_catalog_roles_with_the_default_first(plug): + options = plug.getExportRoles(valuesDict=_values(exportDevice="101")) + assert [key for key, _ in options] == ["onOffPlugInUnit", "onOffLight", "doorLock"] + assert options[0][1] == export_catalog.role_label("onOffPlugInUnit") + + +def test_role_picker_is_empty_without_a_selection(plug): + assert plug.getExportRoles(valuesDict=_values()) == [] + + +def test_role_picker_is_empty_for_an_excluded_pick(plug): + assert plug.getExportRoles(valuesDict=_values(exportDevice="x-104")) == [] + + +def test_role_picker_is_empty_for_a_stale_device_id(plug): + assert plug.getExportRoles(valuesDict=_values(exportDevice="999999")) == [] + + +def test_role_picker_degrades_to_empty_on_error(plug, monkeypatch): + monkeypatch.setattr(export_catalog, "classify", + Mock(side_effect=RuntimeError("boom"))) + assert plug.getExportRoles(valuesDict=_values(exportDevice="101")) == [] + plug.logger.exception.assert_called() + + +# --------------------------------------------------------------------------- +# Current-exports summary list +# --------------------------------------------------------------------------- +def test_current_exports_summarises_role_name_and_polarity(plug): + plug.exports.upsert(ExportEntry(102, "windowCovering", name_override="Blind", + options={OPTION_INVERT: True})) + label = _labels(plug.getCurrentExports())["102"] + assert "Hall Dimmer" in label + assert export_catalog.role_label("windowCovering") in label + assert 'shown as "Blind"' in label + assert "inverted" in label + + +def test_current_exports_names_a_device_that_has_been_deleted(plug): + plug.exports.upsert(ExportEntry(999999, "onOffLight")) + assert "deleted device 999999" in _labels(plug.getCurrentExports())["999999"] + + +def test_current_exports_is_never_an_illegal_empty_option(plug): + assert plug.getCurrentExports() == [("0", "(nothing exported yet)")] + + +def test_current_exports_before_startup(plug): + plug.exports = None + assert plug.getCurrentExports() == [("0", "(plugin still starting)")] + + +# --------------------------------------------------------------------------- +# Picker-changed callback +# --------------------------------------------------------------------------- +def test_device_changed_loads_the_saved_entry(plug): + plug.exports.upsert(ExportEntry(102, "windowCovering", name_override="Blind", + options={OPTION_INVERT: True})) + values = plug.exportDeviceChanged(_values(exportDevice="102"), "manageMatterExports") + assert values["exportRole"] == "windowCovering" + assert values["exportName"] == "Blind" + assert values["exportInvert"] is True + assert "is exported as" in values["exportStatus"] + + +def test_device_changed_seeds_the_safe_default_for_a_new_export(plug): + values = plug.exportDeviceChanged(_values(exportDevice="101"), "manageMatterExports") + assert values["exportRole"] == "onOffPlugInUnit" + assert values["exportName"] == "" + assert values["exportInvert"] is False + assert "not exported yet" in values["exportStatus"] + + +def test_device_changed_reports_an_excluded_pick_in_the_status_field(plug): + values = plug.exportDeviceChanged(_values(exportDevice="x-104"), "manageMatterExports") + assert values["exportRole"] == "" + assert values["exportStatus"] == f"Not exportable: {export_catalog.REASON_SPRINKLER}" + + +def test_device_changed_reports_our_own_device_as_loop_guarded(plug): + values = plug.exportDeviceChanged(_values(exportDevice="x-105"), "manageMatterExports") + assert export_catalog.REASON_LOOP_GUARD in values["exportStatus"] + + +def test_device_changed_clears_the_detail_pane_on_no_selection(plug): + values = plug.exportDeviceChanged( + _values(exportDevice="0", exportRole="doorLock", exportName="X"), "manageMatterExports") + assert values["exportRole"] == "" + assert values["exportName"] == "" + + +def test_device_changed_handles_a_deleted_device(plug): + values = plug.exportDeviceChanged(_values(exportDevice="999999"), "manageMatterExports") + assert "no longer exists" in values["exportStatus"] + + +# --------------------------------------------------------------------------- +# Add / update +# --------------------------------------------------------------------------- +def test_add_persists_the_entry_and_reports_it(plug): + values = plug.exportAddOrUpdate( + _values(exportDevice="101", exportRole="onOffLight"), "manageMatterExports") + assert plug.exports.get(101) == ExportEntry(101, "onOffLight", None, {}) + assert "Added Study Plug" in values["exportStatus"] + plug.logger.info.assert_called() + + +def test_add_writes_through_to_prefs(plug): + plug.exportAddOrUpdate(_values(exportDevice="101", exportRole="onOffLight"), + "manageMatterExports") + assert "onOffLight" in plug.pluginPrefs[PREF_KEY] + + +def test_update_replaces_the_existing_entry(plug): + plug.exports.upsert(ExportEntry(101, "onOffLight")) + values = plug.exportAddOrUpdate( + _values(exportDevice="101", exportRole="doorLock"), "manageMatterExports") + assert plug.exports.get(101).role == "doorLock" + assert len(plug.exports) == 1 + assert "Updated" in values["exportStatus"] + + +def test_add_stores_the_name_override_trimmed(plug): + plug.exportAddOrUpdate( + _values(exportDevice="101", exportRole="onOffLight", exportName=" Desk Lamp "), + "manageMatterExports") + assert plug.exports.get(101).name_override == "Desk Lamp" + + +def test_add_treats_a_blank_name_override_as_none(plug): + plug.exportAddOrUpdate( + _values(exportDevice="101", exportRole="onOffLight", exportName=" "), + "manageMatterExports") + assert plug.exports.get(101).name_override is None + + +def test_add_records_covering_polarity_only_for_coverings(plug): + plug.exportAddOrUpdate( + _values(exportDevice="102", exportRole="windowCovering", exportInvert="true"), + "manageMatterExports") + assert plug.exports.get(102).options == {OPTION_INVERT: True} + plug.exportAddOrUpdate( + _values(exportDevice="102", exportRole="dimmableLight", exportInvert=True), + "manageMatterExports") + assert plug.exports.get(102).options == {} + + +def test_add_requires_a_selection(plug): + _values_out, errors = plug.exportAddOrUpdate(_values(), "manageMatterExports") + assert "exportDevice" in errors + assert len(plug.exports) == 0 + + +def test_add_rejects_an_excluded_pick_with_a_field_error(plug): + values, errors = plug.exportAddOrUpdate( + _values(exportDevice="x-104", exportRole="onOffLight"), "manageMatterExports") + assert export_catalog.REASON_SPRINKLER in errors["exportDevice"] + assert export_catalog.REASON_SPRINKLER in values["exportStatus"] + assert len(plug.exports) == 0 + + +def test_xac6_add_refuses_our_own_device_even_if_the_id_is_unprefixed(plug): + """Server-side rejection, not just a picker label — the guard is structural.""" + _out, errors = plug.exportAddOrUpdate( + _values(exportDevice="105", exportRole="onOffLight"), "manageMatterExports") + assert export_catalog.REASON_LOOP_GUARD in errors["exportDevice"] + assert len(plug.exports) == 0 + + +def test_add_rejects_a_role_the_device_is_not_eligible_for(plug): + _out, errors = plug.exportAddOrUpdate( + _values(exportDevice="101", exportRole="thermostat"), "manageMatterExports") + assert "exportRole" in errors + assert len(plug.exports) == 0 + + +def test_add_rejects_an_empty_role(plug): + _out, errors = plug.exportAddOrUpdate(_values(exportDevice="101"), "manageMatterExports") + assert "exportRole" in errors + + +def test_add_rejects_a_stale_device_id(plug): + _out, errors = plug.exportAddOrUpdate( + _values(exportDevice="999999", exportRole="onOffLight"), "manageMatterExports") + assert "no longer exists" in errors["exportDevice"] + + +def test_add_before_startup_is_a_dialog_error(plug): + plug.exports = None + _out, errors = plug.exportAddOrUpdate( + _values(exportDevice="101", exportRole="onOffLight"), "manageMatterExports") + assert "still starting" in errors["exportDevice"] + + +# --------------------------------------------------------------------------- +# Remove +# --------------------------------------------------------------------------- +def test_remove_drops_the_entry_and_clears_the_pane(plug): + plug.exports.upsert(ExportEntry(101, "onOffLight")) + values = plug.exportRemove( + _values(exportDevice="101", exportRole="onOffLight"), "manageMatterExports") + assert plug.exports.get(101) is None + assert values["exportRole"] == "" + assert "Removed Study Plug" in values["exportStatus"] + + +def test_remove_requires_a_selection(plug): + _out, errors = plug.exportRemove(_values(), "manageMatterExports") + assert "exportDevice" in errors + + +def test_remove_of_something_not_exported_is_a_field_error(plug): + _out, errors = plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") + assert errors["exportDevice"] == "That device is not exported." + + +def test_remove_works_for_a_deleted_indigo_device(plug): + plug.exports.upsert(ExportEntry(999999, "onOffLight")) + values = plug.exportRemove(_values(exportDevice="999999"), "manageMatterExports") + assert plug.exports.get(999999) is None + assert "device 999999" in values["exportStatus"] + + +def test_remove_before_startup_is_a_dialog_error(plug): + plug.exports = None + _out, errors = plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") + assert "still starting" in errors["exportDevice"] + + +# --------------------------------------------------------------------------- +# Filter button +# --------------------------------------------------------------------------- +def test_reload_button_echoes_the_filter_in_the_status(plug): + values = plug.exportReloadPicker(_values(exportFilter="hall"), "manageMatterExports") + assert 'Filtered on "hall"' in values["exportStatus"] + + +def test_reload_button_with_no_filter_shows_the_summary(plug): + plug.exports.upsert(ExportEntry(101, "onOffLight")) + values = plug.exportReloadPicker(_values(), "manageMatterExports") + assert values["exportStatus"] == "1 device(s) exported." + + +# --------------------------------------------------------------------------- +# Selection decoding + checkbox coercion +# --------------------------------------------------------------------------- +@pytest.mark.parametrize("raw,expected", [ + ("", ("none", 0)), + ("0", ("none", 0)), + ("junk", ("none", 0)), + ("101", ("device", 101)), + ("x-104", ("excluded", 104)), +]) +def test_selection_decoding(plug, raw, expected): + assert plug._export_selection({"exportDevice": raw}) == expected + + +@pytest.mark.parametrize("raw,expected", [ + (True, True), (False, False), ("true", True), ("YES", True), + ("1", True), ("false", False), ("", False), (None, False), +]) +def test_checkbox_coercion(plug, raw, expected): + assert plug._truthy(raw) is expected + + +def test_plugin_id_falls_back_to_the_bundle_constant(plug): + del plug.pluginId + assert plug._export_plugin_id() == export_catalog.DEFAULT_PLUGIN_ID + + +# --------------------------------------------------------------------------- +# startup wiring — the allow-list must exist before anything consults it +# --------------------------------------------------------------------------- +def test_startup_loads_the_allow_list_from_prefs(plugin_mod, monkeypatch): + class FakeRuntimeObj: + is_running = True + + def start(self): + pass + + def submit(self, coro): + if hasattr(coro, "close"): + coro.close() + return Mock() + + monkeypatch.setattr(plugin_mod, "MatterClient", + lambda *a, **k: Mock(run=lambda: None)) + monkeypatch.setattr(plugin_mod, "AsyncRuntime", lambda logger: FakeRuntimeObj()) + monkeypatch.setattr(plugin_mod, "CommissionJobs", lambda *a, **k: Mock()) + monkeypatch.setattr(plugin_mod, "HttpApi", lambda *a, **k: Mock()) + # MUST be patched — an unpatched ServerProcess touches the real $HOME (#104). + monkeypatch.setattr(plugin_mod, "ServerProcess", lambda *a, **k: Mock()) + + p = plugin_mod.Plugin.__new__(plugin_mod.Plugin) + p.logger = Mock() + p.pluginPrefs = {} + p.proto = object() + p.registry = object() + p.device_sync = Mock() + p.runtime = None + p.server_process = None + p.exports = None + ExportStore(p.pluginPrefs, p.logger).upsert(ExportEntry(101, "onOffLight")) + + p.startup() + assert p.exports is not None + assert p.exports.ids() == frozenset({101}) diff --git a/tests/test_export_store.py b/tests/test_export_store.py new file mode 100644 index 0000000..43ff629 --- /dev/null +++ b/tests/test_export_store.py @@ -0,0 +1,267 @@ +"""The export allow-list store (PRD-indigo-matter-export §5.1 / §4.3). + +Covers CRUD, the JSON round trip, schema versioning, snapshot immutability, +lock re-entrancy, and the property that matters most in the field: a blob we +cannot parse is **preserved**, never discarded — a user who has hand-built +twenty exports must be able to get them back. +""" +from __future__ import annotations + +import json +import threading + +import pytest + +from bridge_protocol import ROLES +from export_store import ( + OPTION_INVERT, + PREF_KEY, + PREF_KEY_CORRUPT, + SCHEMA_VERSION, + ExportEntry, + ExportStore, +) + + +@pytest.fixture +def prefs(): + return {} + + +@pytest.fixture +def store(prefs, mock_logger): + return ExportStore(prefs, mock_logger) + + +def _entry(device_id=101, role="onOffLight", **kwargs): + return ExportEntry(indigo_device_id=device_id, role=role, **kwargs) + + +# --------------------------------------------------------------------------- +# CRUD +# --------------------------------------------------------------------------- +def test_new_store_is_empty(store): + assert store.all() == () + assert store.ids() == frozenset() + assert len(store) == 0 + + +def test_upsert_then_get(store): + store.upsert(_entry()) + assert store.get(101) == _entry() + assert store.ids() == frozenset({101}) + assert 101 in store + + +def test_upsert_replaces_by_device_id(store): + store.upsert(_entry(role="onOffLight")) + store.upsert(_entry(role="doorLock")) + assert len(store) == 1 + assert store.get(101).role == "doorLock" + + +def test_remove_reports_whether_it_existed(store): + store.upsert(_entry()) + assert store.remove(101) is True + assert store.remove(101) is False + assert store.get(101) is None + + +def test_all_is_ordered_by_device_id(store): + for device_id in (300, 100, 200): + store.upsert(_entry(device_id)) + assert [e.indigo_device_id for e in store.all()] == [100, 200, 300] + + +def test_contains_tolerates_junk(store): + assert ("nonsense" in store) is False + assert (None in store) is False + + +def test_replace_all_swaps_the_whole_list(store): + store.upsert(_entry(1)) + store.replace_all([_entry(2), _entry(3)]) + assert store.ids() == frozenset({2, 3}) + + +# --------------------------------------------------------------------------- +# Snapshot immutability +# --------------------------------------------------------------------------- +def test_all_snapshot_does_not_track_later_writes(store): + store.upsert(_entry(1)) + snapshot = store.all() + store.upsert(_entry(2)) + assert [e.indigo_device_id for e in snapshot] == [1] + + +def test_ids_snapshot_is_frozen(store): + store.upsert(_entry(1)) + ids = store.ids() + store.remove(1) + assert ids == frozenset({1}) + with pytest.raises(AttributeError): + ids.add(2) # frozenset, by contract + + +def test_entry_is_immutable(store): + entry = _entry() + with pytest.raises(AttributeError): + entry.role = "doorLock" # frozen dataclass + + +# --------------------------------------------------------------------------- +# Persistence / round trip / schema version +# --------------------------------------------------------------------------- +def test_write_persists_immediately_to_prefs(prefs, store): + store.upsert(_entry(7, "windowCovering", name_override="Blind", + options={OPTION_INVERT: True})) + payload = json.loads(prefs[PREF_KEY]) + assert payload["v"] == SCHEMA_VERSION + assert payload["exports"] == [{ + "indigoDeviceId": 7, "role": "windowCovering", + "nameOverride": "Blind", "options": {OPTION_INVERT: True}, + }] + + +def test_round_trips_through_a_second_store(prefs, mock_logger): + first = ExportStore(prefs, mock_logger) + first.upsert(_entry(7, "windowCovering", name_override="Blind", + options={OPTION_INVERT: True})) + first.upsert(_entry(8, "thermostat")) + second = ExportStore(prefs, mock_logger) + assert second.all() == first.all() + + +def test_remove_persists(prefs, store, mock_logger): + store.upsert(_entry(1)) + store.upsert(_entry(2)) + store.remove(1) + assert ExportStore(prefs, mock_logger).ids() == frozenset({2}) + + +def test_blank_pref_is_simply_empty(mock_logger): + assert ExportStore({PREF_KEY: ""}, mock_logger).all() == () + mock_logger.error.assert_not_called() + + +# --------------------------------------------------------------------------- +# Corrupt-blob preservation — never silently discard user config +# --------------------------------------------------------------------------- +def test_unparseable_json_is_preserved_and_store_starts_empty(mock_logger): + prefs = {PREF_KEY: "{not json at all"} + store = ExportStore(prefs, mock_logger) + assert store.all() == () + assert prefs[PREF_KEY_CORRUPT] == "{not json at all" + mock_logger.error.assert_called() + + +def test_wrong_schema_version_is_preserved_not_reinterpreted(mock_logger): + blob = json.dumps({"v": 99, "exports": [{"indigoDeviceId": 1, "role": "onOffLight"}]}) + prefs = {PREF_KEY: blob} + store = ExportStore(prefs, mock_logger) + assert store.all() == () + assert prefs[PREF_KEY_CORRUPT] == blob + + +def test_non_object_payload_is_preserved(mock_logger): + prefs = {PREF_KEY: "[1, 2, 3]"} + assert ExportStore(prefs, mock_logger).all() == () + assert prefs[PREF_KEY_CORRUPT] == "[1, 2, 3]" + + +def test_exports_not_a_list_is_preserved(mock_logger): + blob = json.dumps({"v": SCHEMA_VERSION, "exports": {"nope": True}}) + prefs = {PREF_KEY: blob} + assert ExportStore(prefs, mock_logger).all() == () + assert prefs[PREF_KEY_CORRUPT] == blob + + +def test_one_bad_entry_drops_only_itself_and_keeps_the_blob(mock_logger): + blob = json.dumps({"v": SCHEMA_VERSION, "exports": [ + {"indigoDeviceId": 1, "role": "onOffLight"}, + {"indigoDeviceId": 2, "role": "teleporter"}, # role not in §4.2 + {"role": "onOffLight"}, # no device id + {"indigoDeviceId": 3, "role": "thermostat"}, + ]}) + prefs = {PREF_KEY: blob} + store = ExportStore(prefs, mock_logger) + assert store.ids() == frozenset({1, 3}) + assert prefs[PREF_KEY_CORRUPT] == blob + assert mock_logger.error.call_count == 2 + + +def test_entry_from_dict_rejects_unknown_role(): + with pytest.raises(ValueError): + ExportEntry.from_dict({"indigoDeviceId": 1, "role": "notARole"}) + + +def test_entry_from_dict_accepts_every_protocol_role(): + for role in ROLES: + entry = ExportEntry.from_dict({"indigoDeviceId": 1, "role": role}) + assert entry.role == role + + +def test_entry_from_dict_rejects_non_object(): + with pytest.raises(ValueError): + ExportEntry.from_dict("nope") + + +def test_entry_from_dict_rejects_bad_options(): + with pytest.raises(ValueError): + ExportEntry.from_dict({"indigoDeviceId": 1, "role": "onOffLight", "options": "nope"}) + + +def test_entry_from_dict_rejects_non_string_name_override(): + with pytest.raises(ValueError): + ExportEntry.from_dict({"indigoDeviceId": 1, "role": "onOffLight", "nameOverride": 7}) + + +def test_preservation_failure_does_not_raise(mock_logger): + class HostileMapping(dict): + def __setitem__(self, key, value): + if key == PREF_KEY_CORRUPT: + raise RuntimeError("prefs are read-only") + super().__setitem__(key, value) + + prefs = HostileMapping({PREF_KEY: "{broken"}) + store = ExportStore(prefs, mock_logger) # must not raise + assert store.all() == () + mock_logger.exception.assert_called() + + +# --------------------------------------------------------------------------- +# Locking +# --------------------------------------------------------------------------- +def test_lock_is_reentrant(store): + # An RLock, not a Lock: upsert() persists via all(), which re-acquires. + assert isinstance(store._lock, type(threading.RLock())) + + +def test_public_calls_nest_without_deadlocking(store): + with store._lock: + store.upsert(_entry(1)) + assert store.get(1) is not None + assert store.all() + assert store.ids() == frozenset({1}) + assert store.remove(1) is True + + +def test_concurrent_writers_all_land(prefs, mock_logger): + store = ExportStore(prefs, mock_logger) + + def writer(base): + for offset in range(20): + store.upsert(_entry(base + offset)) + + threads = [threading.Thread(target=writer, args=(base,)) for base in (1000, 2000, 3000)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + assert len(store) == 60 + assert len(json.loads(prefs[PREF_KEY])["exports"]) == 60 + + +def test_label_for_prefers_the_override(): + assert _entry().label_for("Kitchen Lamp") == "Kitchen Lamp" + assert _entry(name_override="Hall").label_for("Kitchen Lamp") == "Hall" From 2656dc4627ce9dd6d09b1cb56abff6a62f62b31a Mon Sep 17 00:00:00 2001 From: Simon Clark Date: Wed, 5 Aug 2026 08:05:10 +0100 Subject: [PATCH 2/3] fix(export): harden E2 store/dialog/catalog per PR #122 three-review batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Store integrity - S1 persist-then-commit: _commit writes the pref, flushes through an injected save_prefs (indigo.server.savePluginPrefs), and only then adopts the new map in memory, rolling the pref back if the flush raises. Pre-fix a failed remove-save left memory and prefs divergent — the device vanished from the dialog and resurrected on the next restart. - S2 live prefs binding: the store takes a prefs_getter callable, not the pluginPrefs object, so a PluginConfig save that rebinds pluginPrefs cannot strand writes on an orphan mapping. - S3 load_error: a corrupt blob or dropped rows are carried on the store and surfaced by the dialog, which previously said "Nothing is exported yet." over an unreadable list — inviting a rebuild whose first save clobbers the rescue. - S4 first rescue wins: matterExports.corrupt is never overwritten; a second corruption is logged as NOT preserved. - S5 restored-entry validation: an injected entry_validator re-runs the loop guard over entries loaded from prefs (the one write path the dialog's guards never see), and from_dict enforces the options shape per role — `invert` only on windowCovering, and only as a bool. - S6 the Add/Remove callbacks contain a failing store write and report "FAILED to save the export list", never a stale success. Dialog - D1 button callbacks return the values dict only. The (valuesDict, errorsDict) tuple is the documented shape for validation methods, not for button CallbackMethods (SDK ConfigUI > Button: "returns a dictionary … containing any field changes", and the button's own field is read-only). Every refusal now lands in exportStatus, including the early returns that previously returned without setting it at all. - D2 the picker always leads with a real ("0", "— select a device —") row for the seeded value, and the truncation tail has its own id so "0" is unique. - D3 per-device error containment in all three list callbacks: one unreadable device costs one row, not the whole dialog (pre-fix a single raising proxy made getExportCandidates return []). An outright failure returns a labelled error row rather than an empty list. - D4 excluded-but-exported coherence: the "●" marker survives on excluded rows, the status names the incoherence, and the second excluded branch clears the stale name/polarity it used to leave behind. - D5 removed from the two textfields (undocumented for that type; it is documented for menus and checkboxes, which keep theirs) into adjacent label fields. Catalog - C1 classify() never propagates: a device whose attribute access raises yields Excluded("error reading device") with one logged stack. Fail-closed — an unreadable pluginId cannot prove the loop guard passed. - C2 unit heuristic matches WORDS: "Fluxcapacitor" was a lightSensor, "Attempt Counter" a temperatureSensor, "Epsilon Meter" a pressureSensor and "Overflow Alarm" a flowSensor. Prose needles are \b-anchored; symbol needles keep the matching that suits them. The device NAME is now the weakest tier, below declared units and displayStateValUi. - C3 RGB-only dimmer zoo row (a surviving mutation until now). - C4 reverse-coverage invariant: the union of eligible roles across the zoo IS bridge_protocol.ROLES, so a new role cannot ship UI-unreachable. Fixing it exposed that two zoo sweeps classified against OTHER_PLUGIN_ID — the id the zoo devices themselves carry — making them near-vacuous. Reconcile + docs - R1 report-only startup reconcile: warns per entry whose device is gone, no longer exportable, or no longer offers its saved role. Never edits the list. - R2 PRD §5.2 said excluded devices must be "absent from the picker with a reason shown", contradicting XAC9 ("appear in the picker as excluded, with reasons"). Aligned to XAC9 — the picker has always shown them. - R3 XAC10 test: walks every .py under Server Plugin/ and asserts no matter.js import, matching module names from real import statements against an npm-shaped denylist (AST, not grep — "matter.js" appears in prose all over this repo, and matter_client/matter_model/matter_handlers are ours). - R4 ruff-style noqa in the E2 modules replaced with pylint-readable disables. HANDOVER.md records the new store contract and, in bold, that the store is NOT the guard: E3 must re-classify at endpoint-build time. Tests 1504 -> 1630, all green. pylint 9.31/10 (was 9.24). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S --- docs/HANDOVER.md | 4 + docs/PRD-indigo-matter-export.md | 4 +- .../Contents/Server Plugin/MenuItems.xml | 15 +- .../Contents/Server Plugin/export_catalog.py | 130 ++++-- .../Contents/Server Plugin/export_store.py | 193 ++++++-- .../Contents/Server Plugin/plugin.py | 350 +++++++++++---- tests/fakes.py | 32 ++ tests/test_export_catalog.py | 127 +++++- tests/test_export_menu.py | 411 ++++++++++++++++-- tests/test_export_store.py | 246 ++++++++++- tests/test_xac10_no_matter_js.py | 106 +++++ 11 files changed, 1404 insertions(+), 214 deletions(-) create mode 100644 tests/test_xac10_no_matter_js.py diff --git a/docs/HANDOVER.md b/docs/HANDOVER.md index 22f0794..e971073 100644 --- a/docs/HANDOVER.md +++ b/docs/HANDOVER.md @@ -501,6 +501,10 @@ Domio no longer commissions; it relays a **share code** (Apple Home is admin 1; **Export side (E2, allow-list + UI):** `export_store.py` the allow-list model (`ExportEntry` = device id + §4.2 role + name override + options; `RLock`'d, one schema-versioned JSON string in `pluginPrefs["matterExports"]`, unparseable blobs moved aside to `matterExports.corrupt` rather than discarded) · `export_catalog.py` the PRD §5.2 mapping (eligible roles + safe default, or `Excluded(reason)`; loop guard is `pluginId` only — XNG3/XAC6 — and type dispatch walks the IOM class-name chain because `isinstance` is unusable against the MagicMock'd indigo module) · `MenuItems.xml` → **Manage Matter Exports…**, the UI-D dialog: no `` (so it gets a single Close button and the in-dialog buttons do the work), filter textfield + Apply-filter button + single-select device `menu` with `dynamicReload` (a multi-select `list` has NO CallbackMethod, so master-detail is impossible with one) + role menu + name/polarity fields + Add/Remove buttons + a readonly `exportStatus` textfield (Indigo labels cannot change at runtime) + a readonly summary list. `plugin.py` builds the store in `startup` and owns the callbacks. `bridge_client` is still **not** wired to the store — that is E3. +**E2 hardening (PR #122) — read before building E3.** The store persists *then* commits: `_commit` writes the pref, flushes through the injected `save_prefs` (`indigo.server.savePluginPrefs`), and only then adopts the new map in memory, rolling the pref back if the flush raises — so memory and prefs can never disagree. It holds a `prefs_getter` callable, not the `pluginPrefs` object, because Indigo may rebind that on a PluginConfig save. A load failure is carried in `store.load_error` and shown in the dialog instead of "Nothing is exported yet.", and `matterExports.corrupt` is **first-rescue-wins** (a second corruption never overwrites it). + +> **The store is NOT the guard. E3 MUST re-classify at endpoint-build time.** The injected `entry_validator` re-runs the loop guard over entries restored from prefs, and `ExportEntry.from_dict` enforces the options shape per role (`invert` only on `windowCovering`) — but both run at *load*, against the database as it was then. A device can change type, gain our `pluginId`, or be replaced between load and endpoint build. Every endpoint E3 builds must call `export_catalog.classify` again and refuse anything that comes back `Excluded`; treating a store hit as proof of eligibility reintroduces exactly the loop (XNG3/XAC6) the guard exists to prevent. + **Key invariants:** node-details has NO `endpoints` key (derive from flat `attributes`); `attribute_updated` data is `[node_id,"ep/cl/at",value]`; `node_removed` is a bare id; `server_info` is a bare connect frame (`sdk_version`/`fabric_id`). Setpoints/modes are attribute **writes**, not commands. ## 7. Cross-repo (uncommitted / coordination — left intentionally) diff --git a/docs/PRD-indigo-matter-export.md b/docs/PRD-indigo-matter-export.md index 69fa88c..1722321 100644 --- a/docs/PRD-indigo-matter-export.md +++ b/docs/PRD-indigo-matter-export.md @@ -235,8 +235,8 @@ them into a PRD where they can rot. | Garage doors | Needs the polarity handling the catalog doesn't yet carry (`onState` true = closed, turnOn = close), and mis-mapping is a physical-safety issue. Blocked on the catalog role/polarity work | | Sensors with units outside the table | No faithful Matter sensor type | -Excluded devices must be **absent from the picker with a reason shown**, not -silently missing. +Excluded devices must **appear in the picker as excluded, with reasons** (XAC9), +not silently missing. ### 5.3 Bridge node diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml b/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml index dc29937..213c357 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml @@ -84,9 +84,18 @@ + - Type part of a device name, then click Apply filter. Leave empty to list everything. + + + @@ -110,7 +119,9 @@ - Optional. Leave empty to use the Indigo device name. + + + Verdict: excluded first, before any type reasoning, which is what makes the loop guard structural (XNG3/XAC6) rather than a downstream check that a future edit could route around. + + Never raises. Indigo device objects are live proxies, and one being deleted + while the picker walks the database can raise from any attribute access — + which would otherwise take out the whole dialog. Such a device is + :data:`REASON_DEVICE_ERROR`, never eligible: if ``pluginId`` could not be + read we cannot prove the loop guard passed, so the fail-safe answer is no. """ - if _plugin_id_of(dev) == plugin_id: - return Excluded(REASON_LOOP_GUARD) - handler = _BY_KIND.get(device_kind(dev)) - if handler is None: - return Excluded(REASON_NO_ROLE) - return handler(dev) + try: + if _plugin_id_of(dev) == plugin_id: + return Excluded(REASON_LOOP_GUARD) + handler = _BY_KIND.get(device_kind(dev)) + if handler is None: + return Excluded(REASON_NO_ROLE) + return handler(dev) + except Exception as exc: # pylint: disable=broad-except + _LOG.error("Matter export: could not classify a device for export — %s", exc, + exc_info=True) + return Excluded(REASON_DEVICE_ERROR) def is_exportable(dev, plugin_id: str = DEFAULT_PLUGIN_ID) -> bool: @@ -315,27 +363,43 @@ def _default_first(roles: list[str], default: str) -> tuple[str, ...]: return tuple(seen) -def _unit_text(dev) -> str: - """Everything about a device that might name its unit, lower-cased. +def _unit_texts(dev) -> tuple[str, str]: + """``(strong, weak)`` lower-cased haystacks for the unit heuristic. Indigo carries no canonical unit property on ``SensorDevice`` — plugins record it in their own props, and the only universal surface is the - formatted display value (``displayStateValUi``, e.g. ``"72.3 °F"``). So we - read the likely props, then the formatted value, then the device's own - naming, and match keywords over the lot. + formatted display value (``displayStateValUi``, e.g. ``"72.3 °F"``). Those + are the **strong** tier: a declared unit is a statement about the unit. + The device's own **name** is the weak tier, consulted only when nothing + declared a unit, because a name is prose the user wrote for themselves. It + is kept rather than dropped because plenty of real Indigo sensors carry + their unit nowhere else ("Greenhouse Temperature"). """ - parts: list[str] = [] + strong: list[str] = [] props = getattr(dev, "pluginProps", None) if isinstance(props, dict): for key in _UNIT_PROP_KEYS: value = props.get(key) if isinstance(value, str): - parts.append(value) - for attr in ("displayStateValUi", "model", "subModel", "name"): + strong.append(value) + for attr in _UNIT_ATTRS_STRONG: value = getattr(dev, attr, None) if isinstance(value, str): - parts.append(value) - return " ".join(parts).lower() + strong.append(value) + weak = [value for value in (getattr(dev, attr, None) for attr in _UNIT_ATTRS_WEAK) + if isinstance(value, str)] + return " ".join(strong).lower(), " ".join(weak).lower() + + +def _role_for_text(text: str) -> Optional[str]: + """First :data:`_UNIT_PATTERNS` role whose regex matches ``text``.""" + if not text: + return None + for role, needles in _UNIT_PATTERNS: + for needle in needles: + if needle.search(text): + return role + return None def _numeric_sensor_role(dev) -> Optional[str]: @@ -346,11 +410,5 @@ def _numeric_sensor_role(dev) -> Optional[str]: correct it, which is the same "user-declared role" posture §5.2 takes for relays. """ - text = _unit_text(dev) - if not text: - return None - for role, needles in _UNIT_PATTERNS: - for needle in needles: - if needle in text: - return role - return None + strong, weak = _unit_texts(dev) + return _role_for_text(strong) or _role_for_text(weak) diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py index 3893438..c6c25f4 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_store.py @@ -8,30 +8,42 @@ Persistence is one JSON string in ``pluginPrefs`` under ``matterExports`` (PRD §4.3: "the plugin owns the allow-list and per-export metadata in plugin -prefs, backed up with Indigo's database"). Direct ``pluginPrefs[key] = value`` -writes persist immediately — the pattern the rest of this plugin already uses. +prefs, backed up with Indigo's database"). -Two disciplines worth knowing before editing: +Four disciplines worth knowing before editing: * **The lock is re-entrant on purpose.** From E3 ``deviceUpdated`` reads the allow-list on Indigo's thread while the menu callbacks write it on the UI - thread, and the public methods call each other (``upsert`` re-reads through - ``all()`` to build the payload it persists). Same shape as + thread, and the public methods call each other. Same shape as ``device_sync.DeviceSync``'s index lock. -* **Corrupt config is preserved, never discarded.** A blob we cannot parse is - moved aside to ``matterExports.corrupt`` and the store starts empty, so a - bad write (or a hand-edited ``.indiPref``) costs the user a rebuild, not a - silent, unrecoverable loss of every export they configured. - -No Indigo import: the store takes a prefs-like mapping so it unit-tests -against a plain dict. +* **Persist first, commit second.** :meth:`ExportStore._commit` builds the + payload, writes it to prefs, *flushes* through the injected ``save_prefs`` + callable, and only then adopts the new mapping in memory — rolling the prefs + key back if the flush raises. Mutating memory first (the pre-#122 shape) let + a failed save leave the two out of step: a removed device reappeared on the + next restart while the dialog swore it was gone. +* **Prefs are resolved late, every time.** The store holds a ``prefs_getter`` + callable, not the mapping object. Indigo may rebind ``self.pluginPrefs`` when + the user saves a PluginConfig dialog, and a store holding the old object + would write to an orphan nobody ever persists. +* **Corrupt config is preserved, never discarded — and the first rescue wins.** + A blob we cannot parse is moved aside to ``matterExports.corrupt`` and the + store starts empty, so a bad write (or a hand-edited ``.indiPref``) costs the + user a rebuild, not a silent loss of every export they configured. A *second* + corruption never overwrites the first rescue copy: the oldest surviving blob + is the one most likely to still hold the user's real list. The failure is + also carried in :attr:`ExportStore.load_error` so the dialog can say so + rather than cheerfully reporting "nothing is exported yet". + +No Indigo import: the store takes a prefs-getter and an optional entry +validator so it unit-tests against a plain dict. """ from __future__ import annotations import json import threading from dataclasses import dataclass, field -from typing import Iterable, Optional +from typing import Callable, Iterable, Optional from bridge_protocol import ROLES @@ -53,6 +65,16 @@ #: ``options`` key carrying window-covering polarity (PRD §5.2 / §4.1). OPTION_INVERT = "invert" +#: Roles for which :data:`OPTION_INVERT` means anything. Polarity is a covering +#: concept (§5.2); on any other role it is either a hand-edit or a stale write, +#: and honouring it would silently invert a lock or a plug. +INVERTIBLE_ROLES = ("windowCovering",) + +#: The message the dialog shows when the whole blob was unreadable (S3). The +#: store must never let the UI say "nothing is exported yet" after this. +LOAD_ERROR_UNREADABLE = ("Export list could not be read — starting empty. " + "The previous list is preserved (see Event Log).") + @dataclass(frozen=True) class ExportEntry: @@ -100,6 +122,17 @@ def from_dict(cls, raw: object) -> "ExportEntry": options = raw.get(KEY_OPTIONS) or {} if not isinstance(options, dict): raise ValueError(f"export entry options are not an object (device {device_id})") + if OPTION_INVERT in options: + # Shape is enforced per ROLE, not just per key: a restored or + # hand-edited blob that carries `invert` on a lock or a plug would + # otherwise ride into the endpoint build as a silent polarity flip. + if not isinstance(options[OPTION_INVERT], bool): + raise ValueError( + f"export entry {OPTION_INVERT!r} option is not a boolean (device {device_id})") + if role not in INVERTIBLE_ROLES: + raise ValueError( + f"export entry has the {OPTION_INVERT!r} option on role {role!r}, which has " + f"no polarity (device {device_id})") return cls( indigo_device_id=device_id, role=role, @@ -115,19 +148,42 @@ def label_for(self, device_name: str) -> str: class ExportStore: """Thread-safe CRUD over the allow-list, persisted to plugin prefs. - ``prefs`` is any mutable mapping; in the plugin it is ``self.pluginPrefs``. - Every mutation writes through immediately — there is no flush to forget. + :param prefs_getter: callable returning the *current* prefs mapping — in + the plugin ``lambda: self.pluginPrefs``. A callable, not the mapping, + because Indigo may rebind ``pluginPrefs`` on a PluginConfig save. + :param logger: the plugin logger. + :param save_prefs: callable that flushes prefs to Indigo's database (in the + plugin, ``indigo.server.savePluginPrefs``). Defaults to a no-op so the + store unit-tests against a plain dict. + :param entry_validator: optional callable taking an :class:`ExportEntry` + loaded from prefs and returning a rejection reason (or ``None`` to + accept). Load is the one write path the dialog's guards do not cover — + a restored or hand-edited blob can name a device the loop guard would + refuse — so the plugin injects that check here. """ - def __init__(self, prefs, logger) -> None: - self._prefs = prefs + def __init__(self, prefs_getter: Callable[[], object], logger, + save_prefs: Optional[Callable[[], None]] = None, + entry_validator: Optional[Callable[[ExportEntry], Optional[str]]] = None) -> None: + self._prefs_getter = prefs_getter self._logger = logger + self._save_prefs = save_prefs + self._entry_validator = entry_validator # Re-entrant: public methods call one another, and E3's deviceUpdated # reads from Indigo's thread while the menu writes from the UI's. self._lock = threading.RLock() self._entries: dict[int, ExportEntry] = {} + #: Human-readable reason the last load did not produce a faithful list, + #: or ``None``. The dialog shows it instead of claiming an empty list + #: is an intentionally empty one (S3). + self.load_error: Optional[str] = None self._load() + @property + def _prefs(self): + """The prefs mapping as it is *right now* — never a captured object.""" + return self._prefs_getter() + # ------------------------------------------------------------------ # Reads # ------------------------------------------------------------------ @@ -162,36 +218,70 @@ def __contains__(self, device_id: object) -> bool: # Writes # ------------------------------------------------------------------ def upsert(self, entry: ExportEntry) -> ExportEntry: - """Add ``entry`` or replace the existing one for the same device id.""" + """Add ``entry`` or replace the existing one for the same device id. + + Raises whatever the prefs write or flush raised, having changed + nothing — see :meth:`_commit`. + """ with self._lock: - self._entries[int(entry.indigo_device_id)] = entry - self._save() + pending = dict(self._entries) + pending[int(entry.indigo_device_id)] = entry + self._commit(pending) return entry def remove(self, device_id: int) -> bool: """Drop ``device_id`` from the allow-list. True if it was there.""" with self._lock: - existed = self._entries.pop(int(device_id), None) is not None - if existed: - self._save() - return existed + key = int(device_id) + if key not in self._entries: + return False + pending = dict(self._entries) + del pending[key] + self._commit(pending) + return True def replace_all(self, entries: Iterable[ExportEntry]) -> None: """Replace the whole allow-list in one persisted write.""" with self._lock: - self._entries = {int(e.indigo_device_id): e for e in entries} - self._save() + self._commit({int(e.indigo_device_id): e for e in entries}) # ------------------------------------------------------------------ # Persistence # ------------------------------------------------------------------ - def _save(self) -> None: + def _commit(self, pending: dict[int, ExportEntry]) -> None: + """Persist ``pending``, flush, and only then adopt it in memory. + + The order is the whole point. Writing the pref, flushing it through + Indigo, and *then* replacing ``self._entries`` means a failure at any + step leaves memory and prefs saying the same (old) thing. The reverse + order resurrects removed devices on the next restart while the dialog + reports success. + """ with self._lock: payload = { KEY_VERSION: SCHEMA_VERSION, - KEY_EXPORTS: [entry.to_dict() for entry in self.all()], + KEY_EXPORTS: [pending[key].to_dict() for key in sorted(pending)], } - self._prefs[PREF_KEY] = json.dumps(payload) + blob = json.dumps(payload) + prefs = self._prefs + had_previous = PREF_KEY in prefs + previous = prefs.get(PREF_KEY) + prefs[PREF_KEY] = blob + try: + if self._save_prefs is not None: + self._save_prefs() + except Exception: + # Put the pref back the way we found it: a half-written key the + # in-memory list disagrees with is worse than a failed write. + try: + if had_previous: + prefs[PREF_KEY] = previous + else: + del prefs[PREF_KEY] + except Exception as rollback_exc: # pylint: disable=broad-except + self._logger.exception(rollback_exc) + raise + self._entries = pending def _load(self) -> None: raw = self._prefs.get(PREF_KEY) @@ -227,16 +317,43 @@ def _load(self) -> None: dropped += 1 self._logger.error("Matter export allow-list: dropping an unusable entry — %s", exc) continue + rejection = self._reject(entry) + if rejection: + dropped += 1 + self._logger.error( + "Matter export allow-list: dropping the entry for device %s — %s", + entry.indigo_device_id, rejection) + continue entries[entry.indigo_device_id] = entry self._entries = entries if dropped: + self.load_error = ( + f"{dropped} saved export(s) could not be read and were dropped. " + "The previous list is preserved (see Event Log).") self._preserve(raw) self._logger.debug("Matter export allow-list loaded: %d entries (%d dropped)", len(entries), dropped) + def _reject(self, entry: ExportEntry) -> Optional[str]: + """The injected validator's verdict on a restored entry, fail-safe. + + Load is an unguarded write path: the dialog's loop guard never sees a + blob restored from a backup or edited by hand. A validator that itself + blows up must not take the whole allow-list down with it, so its own + failure is logged and the entry kept — the E3 endpoint build re-checks. + """ + if self._entry_validator is None: + return None + try: + return self._entry_validator(entry) + except Exception as exc: # pylint: disable=broad-except + self._logger.exception(exc) + return None + def _corrupt(self, raw, why: str) -> None: """Start empty, but keep the blob — user config is never discarded.""" self._entries = {} + self.load_error = LOAD_ERROR_UNREADABLE self._logger.error( "Matter export allow-list unreadable (%s). Starting with an EMPTY export list; " "the previous value is preserved in the %r plugin pref for recovery.", @@ -245,8 +362,24 @@ def _corrupt(self, raw, why: str) -> None: self._preserve(raw) def _preserve(self, raw) -> None: + """Move the unreadable blob aside — but never over an earlier rescue. + + First rescue wins. A second corruption is usually a *derivative* of the + first (the user restarted, we wrote an empty list, that got mangled + too); overwriting would trade the blob that still holds twenty real + exports for one that holds none. + """ try: - self._prefs[PREF_KEY_CORRUPT] = raw if isinstance(raw, str) else repr(raw) + prefs = self._prefs + if prefs.get(PREF_KEY_CORRUPT): + self._logger.error( + "Matter export allow-list: an earlier rescue copy already exists in the %r " + "plugin pref and was KEPT — this newer unreadable value was NOT preserved. " + "Recover from the existing copy, then clear it.", + PREF_KEY_CORRUPT, + ) + return + prefs[PREF_KEY_CORRUPT] = raw if isinstance(raw, str) else repr(raw) except Exception as exc: # pylint: disable=broad-except # Preservation is best-effort: whatever the prefs mapping does, it # must not turn an unreadable allow-list into a failed startup. diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py index a554618..644b9d2 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py @@ -49,10 +49,25 @@ #: Option-id prefix marking a picker row the user may look at but not choose #: (PRD §5.2: excluded devices are shown *with a reason*, never hidden — XAC9). EXCLUDED_OPTION_PREFIX = "x-" -#: The "nothing selected" / "list truncated" sentinel. Never "": Indigo rejects -#: an empty list id with "UI dynamic list function returned illegal ID string" -#: and silently drops the option. +#: The "nothing selected" sentinel. Never "": Indigo rejects an empty list id +#: with "UI dynamic list function returned illegal ID string" and silently +#: drops the option. The picker always emits a REAL row carrying this id +#: (:data:`NO_SELECTION_LABEL`), because the dialog is seeded with it — a +#: seeded value with no matching row renders as a blank first item. NO_SELECTION_ID = "0" +NO_SELECTION_LABEL = "— select a device —" +#: Informational rows. They get their own ids so :data:`NO_SELECTION_ID` stays +#: unique, and the ``x-`` prefix keeps them unpickable through the same door +#: excluded devices use. +TRUNCATED_OPTION = (f"{EXCLUDED_OPTION_PREFIX}truncated", + "…too many matches — narrow the filter") +NO_MATCH_OPTION = (f"{EXCLUDED_OPTION_PREFIX}nomatch", "(no devices match the filter)") +#: What a list callback returns when it fails outright. An empty list would +#: render as an empty popup the user cannot tell from "nothing to choose". +LIST_ERROR_OPTION = (NO_SELECTION_ID, "(error building list — see Event Log)") +#: One unreadable device inside an otherwise fine list (D3): the row is kept so +#: the count is honest, but it is not selectable. +ROW_ERROR_LABEL = "(error reading device — see Event Log)" #: Picker cap. Past this the tail row asks the user to narrow the filter — a #: 2000-device database would otherwise build an unusable popup menu. EXPORT_PICKER_LIMIT = 300 @@ -170,9 +185,18 @@ def startup(self) -> None: # unreadable list must not stop the (inbound) plugin starting, so # ExportStore degrades to empty and preserves the blob rather than # raising — see export_store._corrupt. - self.exports = ExportStore(self.pluginPrefs, self.logger) + # prefs are read through a getter, not captured: Indigo can rebind + # self.pluginPrefs when the user saves the PluginConfig dialog, and a + # store holding the old mapping would write to an orphan. savePluginPrefs + # is the flush the store commits through before it trusts a write. + self.exports = ExportStore( + lambda: self.pluginPrefs, self.logger, + save_prefs=self._save_plugin_prefs, + entry_validator=self._reject_unexportable_entry, + ) if len(self.exports): self.logger.info("Matter export allow-list: %d device(s) exported", len(self.exports)) + self._reconcile_exports() self.runtime = AsyncRuntime(self.logger) self.runtime.start() @@ -1087,18 +1111,19 @@ def _indigo_device(device_id): """``indigo.devices[device_id]`` or None — a stale id is never fatal.""" try: return indigo.devices[int(device_id)] - except Exception: # noqa: BLE001 - KeyError, ValueError, or Indigo's own + except Exception: # pylint: disable=broad-except # KeyError/ValueError/Indigo's own return None def _export_selection(self, values_dict) -> tuple[str, int]: """Decode the picker value into ``(kind, device_id)``. - ``kind`` is ``"none"`` (nothing chosen, or the truncation row), - ``"excluded"`` (an ``x-`` row the user may see but not pick), or + ``kind`` is ``"none"`` (nothing chosen, or one of the informational + rows — the "select a device" seed, the truncation tail, the no-match + note), ``"excluded"`` (an ``x-`` row the user may see but not pick), or ``"device"``. """ raw = str((values_dict or {}).get("exportDevice", "") or "") - if not raw or raw == NO_SELECTION_ID: + if not raw or raw == NO_SELECTION_ID or raw in (TRUNCATED_OPTION[0], NO_MATCH_OPTION[0]): return ("none", 0) excluded = raw.startswith(EXCLUDED_OPTION_PREFIX) if excluded: @@ -1109,10 +1134,80 @@ def _export_selection(self, values_dict) -> tuple[str, int]: return ("none", 0) return ("excluded" if excluded else "device", device_id) + def _save_plugin_prefs(self) -> None: + """Flush pluginPrefs to Indigo's database (the store's commit step).""" + indigo.server.savePluginPrefs() + + def _reject_unexportable_entry(self, entry) -> str | None: + """Validator for entries restored from prefs — the loop guard, re-run. + + Load is the one write path the dialog's guards never see: a blob + restored from a backup, or hand-edited in the ``.indiPref``, can name a + device this plugin created. Only the loop guard is enforced here. + Ordinary ineligibility is *reported* by the startup reconcile and left + alone, because a device can be temporarily odd (a plugin still + starting) and silently deleting the user's export would be worse than + an accessory that fails to build. + """ + dev = self._indigo_device(entry.indigo_device_id) + if dev is None: + return None + verdict = export_catalog.classify(dev, self._export_plugin_id()) + if isinstance(verdict, export_catalog.Excluded) \ + and verdict.reason == export_catalog.REASON_LOOP_GUARD: + return export_catalog.REASON_LOOP_GUARD + return None + + def _reconcile_exports(self) -> None: + """Report-only startup sweep of the allow-list (never edits it). + + An export whose device has been deleted, or which no longer classifies + as exportable, is a real problem the user should hear about at startup + rather than discovering as a missing accessory. It is NOT auto-removed: + the allow-list is the user's declaration, and E3 re-classifies at + endpoint-build time anyway. + """ + if self.exports is None: + return + try: + plugin_id = self._export_plugin_id() + for entry in self.exports.all(): + dev = self._indigo_device(entry.indigo_device_id) + if dev is None: + self.logger.warning( + "Matter export allow-list: device %s is exported as %s but no longer " + "exists in Indigo — it will not be bridged. Remove it in " + "'Manage Matter Exports…'.", + entry.indigo_device_id, entry.role) + continue + verdict = export_catalog.classify(dev, plugin_id) + if isinstance(verdict, export_catalog.Excluded): + self.logger.warning( + "Matter export allow-list: %s (id %s) is exported as %s but is no longer " + "exportable: %s. It will not be bridged.", + getattr(dev, "name", ""), entry.indigo_device_id, entry.role, + verdict.reason) + elif entry.role not in verdict.eligible_roles: + self.logger.warning( + "Matter export allow-list: %s (id %s) is exported as %s, which this " + "device no longer offers (%s). Re-pick its role in " + "'Manage Matter Exports…'.", + getattr(dev, "name", ""), entry.indigo_device_id, entry.role, + ", ".join(verdict.eligible_roles)) + except Exception as exc: # pylint: disable=broad-except + # A diagnostic sweep must never be the thing that fails startup. + self.logger.exception(exc) + def _export_summary(self) -> str: if self.exports is None: return "Plugin still starting — reopen this dialog in a moment." count = len(self.exports) + # A load failure has to lead. Reporting "Nothing is exported yet." over + # a blob we could not read invites the user to rebuild the list from + # scratch, and the rebuild's first save overwrites the rescue copy. + error = self.exports.load_error + if error: + return error if not count else f"{error} {count} device(s) exported." if not count: return "Nothing is exported yet." return f"{count} device(s) exported." @@ -1135,7 +1230,34 @@ def get_menu_action_config_ui_values(self, menu_id): values["exportStatus"] = self._export_summary() return values - def getExportCandidates(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002, ARG002 + def _log_row_failure(self, exc, first: bool) -> None: + """Log one unreadable picker row — stack for the first, one line after. + + A database with fifty broken proxies must not write fifty tracebacks + into the event log, but the first one has to carry enough to debug. + """ + if first: + self.logger.exception(exc) + else: + self.logger.error("Matter export: another device could not be read — %s", exc) + + @staticmethod + def _candidate_row(dev, name: str, plugin_id: str, exported) -> tuple[str, str]: + """One picker row for ``dev``. May raise — the caller contains it.""" + device_id = dev.id + # An excluded device that IS exported keeps its marker: the pair + # "excluded" + "exported" is exactly the state the user has to know + # about, and hiding half of it reads as a picker bug rather than the + # stale export it actually is. + mark = "● " if device_id in exported else "" + verdict = export_catalog.classify(dev, plugin_id) + if isinstance(verdict, export_catalog.Excluded): + return (f"{EXCLUDED_OPTION_PREFIX}{device_id}", + f"{mark}{name} — not exportable: {verdict.reason}") + return (str(device_id), f"{mark}{name}") + + def getExportCandidates(self, filter="", valuesDict=None, typeId="", targetId=0): + # pylint: disable=redefined-builtin, unused-argument """Picker rows: every Indigo device, exportable or not (XAC9). Excluded devices are listed **with the reason in the label** and an @@ -1144,42 +1266,54 @@ def getExportCandidates(self, filter="", valuesDict=None, typeId="", targetId=0) appear. ``filter`` here is the XML's static filter attribute, NOT the user's text: textfields have no callbacks, so the typed filter arrives in ``valuesDict`` and the Apply-filter button drives the reload. + + One device that cannot be read costs one row, not the whole list: the + try/except is INSIDE the loop, because the alternative is a dialog that + renders empty the moment any device in the database misbehaves. """ try: text = str((valuesDict or {}).get("exportFilter", "") or "").strip().lower() exported = self.exports.ids() if self.exports is not None else frozenset() plugin_id = self._export_plugin_id() - options: list[tuple[str, str]] = [] + # Always a real row for the seeded value, and always first. + options: list[tuple[str, str]] = [(NO_SELECTION_ID, NO_SELECTION_LABEL)] + matched = 0 truncated = 0 + failures = 0 for dev in indigo.devices: - name = str(getattr(dev, "name", "") or "") - if text and text not in name.lower(): - continue - if len(options) >= EXPORT_PICKER_LIMIT: - truncated += 1 - continue - verdict = export_catalog.classify(dev, plugin_id) - if isinstance(verdict, export_catalog.Excluded): - options.append((f"{EXCLUDED_OPTION_PREFIX}{dev.id}", - f"{name} — not exportable: {verdict.reason}")) - else: - mark = "● " if dev.id in exported else "" - options.append((str(dev.id), f"{mark}{name}")) + try: + name = str(getattr(dev, "name", "") or "") + if text and text not in name.lower(): + continue + matched += 1 + if matched > EXPORT_PICKER_LIMIT: + truncated += 1 + continue + options.append(self._candidate_row(dev, name, plugin_id, exported)) + except Exception as exc: # pylint: disable=broad-except + self._log_row_failure(exc, first=not failures) + failures += 1 + # Position-keyed id: the device's own id is one of the + # things we could not read. + options.append((f"{EXCLUDED_OPTION_PREFIX}err{len(options)}", + f"— {ROW_ERROR_LABEL}")) if truncated: - options.append((NO_SELECTION_ID, - f"… {truncated} more — narrow the filter")) - if not options: - options.append((NO_SELECTION_ID, "(no devices match the filter)")) + options.append(TRUNCATED_OPTION) + if len(options) == 1: + options.append(NO_MATCH_OPTION) return options - except Exception as exc: # noqa: BLE001 - never break the dialog + except Exception as exc: # pylint: disable=broad-except self.logger.exception(exc) - return [] + return [LIST_ERROR_OPTION] - def getExportRoles(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002, ARG002 + def getExportRoles(self, filter="", valuesDict=None, typeId="", targetId=0): + # pylint: disable=redefined-builtin, unused-argument """Roles the picked device may legitimately be exported as (§5.2). Empty for no selection or an excluded pick — an empty role menu is the - honest rendering of "there is nothing you may choose here". + honest rendering of "there is nothing you may choose here". An outright + failure is NOT empty: it says so, so the user does not read a broken + callback as "this device offers no roles". """ try: kind, device_id = self._export_selection(valuesDict) @@ -1191,34 +1325,60 @@ def getExportRoles(self, filter="", valuesDict=None, typeId="", targetId=0): # verdict = export_catalog.classify(dev, self._export_plugin_id()) if isinstance(verdict, export_catalog.Excluded): return [] - return [(role, export_catalog.role_label(role)) for role in verdict.eligible_roles] - except Exception as exc: # noqa: BLE001 - never break the dialog + options = [] + for role in verdict.eligible_roles: + try: + options.append((role, export_catalog.role_label(role))) + except Exception as exc: # pylint: disable=broad-except + self._log_row_failure(exc, first=not options) + return options + except Exception as exc: # pylint: disable=broad-except self.logger.exception(exc) - return [] + return [LIST_ERROR_OPTION] - def getCurrentExports(self, filter="", valuesDict=None, typeId="", targetId=0): # noqa: N802, A002, ARG002 + def getCurrentExports(self, filter="", valuesDict=None, typeId="", targetId=0): + # pylint: disable=redefined-builtin, unused-argument """Read-only summary of the allow-list (one row per export).""" try: if self.exports is None: return [(NO_SELECTION_ID, "(plugin still starting)")] options = [] + failures = 0 for entry in self.exports.all(): - dev = self._indigo_device(entry.indigo_device_id) - name = str(getattr(dev, "name", "") or "") if dev is not None else "" - if not name: - name = f"(deleted device {entry.indigo_device_id})" - label = f"{name} → {export_catalog.role_label(entry.role)}" - if entry.name_override: - label += f' · shown as "{entry.name_override}"' - if entry.options.get(OPTION_INVERT): - label += " · inverted" - options.append((str(entry.indigo_device_id), label)) + try: + dev = self._indigo_device(entry.indigo_device_id) + name = str(getattr(dev, "name", "") or "") if dev is not None else "" + if not name: + name = f"(deleted device {entry.indigo_device_id})" + label = f"{name} → {export_catalog.role_label(entry.role)}" + if entry.name_override: + label += f' · shown as "{entry.name_override}"' + if entry.options.get(OPTION_INVERT): + label += " · inverted" + options.append((str(entry.indigo_device_id), label)) + except Exception as exc: # pylint: disable=broad-except + self._log_row_failure(exc, first=not failures) + failures += 1 + options.append((f"{EXCLUDED_OPTION_PREFIX}err{len(options)}", + f"— {ROW_ERROR_LABEL}")) return options or [(NO_SELECTION_ID, "(nothing exported yet)")] - except Exception as exc: # noqa: BLE001 - never break the dialog + except Exception as exc: # pylint: disable=broad-except self.logger.exception(exc) - return [] + return [LIST_ERROR_OPTION] + + def _exported_warning(self, device_id: int) -> str: + """Suffix warning shown when an EXCLUDED device is nonetheless exported. + + This is the incoherent state worth naming out loud: the allow-list says + export it, the catalog says it cannot be. Left alone it becomes an + accessory that never appears, with no visible cause. + """ + if self.exports is not None and device_id in self.exports: + return " — but this device IS currently exported — remove it or it will fail to bridge" + return "" - def exportReloadPicker(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 + def exportReloadPicker(self, valuesDict, typeId="", devId=0): + # pylint: disable=unused-argument """Apply-filter button: the return trip is what reloads the lists.""" values = valuesDict text = str(values.get("exportFilter", "") or "").strip() @@ -1226,13 +1386,14 @@ def exportReloadPicker(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG else self._export_summary()) return values - def exportDeviceChanged(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 + def exportDeviceChanged(self, valuesDict, typeId="", devId=0): + # pylint: disable=unused-argument """Picker selection changed: load that device's saved export, or defaults. Menu callbacks return a valuesDict, not an error dict (the SDK's menu contract), so an excluded pick is reported in the read-only status - field here and *rejected* — with a real field error — by the - Add/update button below. Both paths are covered by tests. + field here — and refused again by the Add/update button below. Both + paths are covered by tests. """ values = valuesDict kind, device_id = self._export_selection(values) @@ -1252,12 +1413,16 @@ def exportDeviceChanged(self, valuesDict, typeId="", devId=0): # noqa: N802, AR values["exportRole"] = "" values["exportName"] = "" values["exportInvert"] = False - values["exportStatus"] = f"Not exportable: {reason}" + values["exportStatus"] = (f"Not exportable: {reason}" + f"{self._exported_warning(device_id)}") return values verdict = export_catalog.classify(dev, self._export_plugin_id()) if isinstance(verdict, export_catalog.Excluded): values["exportRole"] = "" - values["exportStatus"] = f"Not exportable: {verdict.reason}" + values["exportName"] = "" + values["exportInvert"] = False + values["exportStatus"] = (f"Not exportable: {verdict.reason}" + f"{self._exported_warning(device_id)}") return values entry = self.exports.get(device_id) if self.exports is not None else None if entry is not None: @@ -1272,48 +1437,64 @@ def exportDeviceChanged(self, valuesDict, typeId="", devId=0): # noqa: N802, AR values["exportStatus"] = f"{dev.name} is not exported yet." return values - def exportAddOrUpdate(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 + def exportAddOrUpdate(self, valuesDict, typeId="", devId=0): + # pylint: disable=unused-argument """Add or update one export. Validates the role against the catalog. A role the catalog does not offer for this device is refused here rather than by the bridge node, which would only reject it with ``unknown_role``/``role_change`` long after the user could connect the failure to what they did (BRIDGE_PROTOCOL §1.1). + + Returns **the values dict only**. A ``(valuesDict, errorsDict)`` tuple + is the documented contract for *validation* methods, not for button + ``CallbackMethod``s — the SDK's button reference says a button callback + returns a dictionary of field changes, and the field carrying a button's + outcome is read-only, so it cannot hold an error message anyway. Every + refusal therefore lands in ``exportStatus``, which is what the dialog + actually shows. """ values = valuesDict - errors = indigo.Dict() if self.exports is None: - errors["exportDevice"] = "Plugin still starting — try again in a moment." - return (values, errors) + values["exportStatus"] = "Plugin still starting — try again in a moment." + return values kind, device_id = self._export_selection(values) if kind == "none": - errors["exportDevice"] = "Select a device to export." - return (values, errors) + values["exportStatus"] = "Select a device to export." + return values dev = self._indigo_device(device_id) if dev is None: - errors["exportDevice"] = "That device no longer exists — refresh the list." - return (values, errors) + values["exportStatus"] = "That device no longer exists — refresh the list." + return values verdict = export_catalog.classify(dev, self._export_plugin_id()) if kind == "excluded" or isinstance(verdict, export_catalog.Excluded): reason = verdict.reason if isinstance(verdict, export_catalog.Excluded) \ else "not exportable" - errors["exportDevice"] = f"{dev.name} cannot be exported: {reason}" - values["exportStatus"] = f"Not exportable: {reason}" - return (values, errors) + values["exportStatus"] = (f"{dev.name} cannot be exported: {reason}" + f"{self._exported_warning(device_id)}") + return values role = str(values.get("exportRole", "") or "") if role not in verdict.eligible_roles: - errors["exportRole"] = ("Choose how this device should appear " - f"({', '.join(verdict.eligible_roles)}).") - return (values, errors) + values["exportStatus"] = ("Choose how this device should appear " + f"({', '.join(verdict.eligible_roles)}).") + return values name_override = str(values.get("exportName", "") or "").strip() or None options = {} if role == export_catalog.ROLE_WINDOW_COVERING and self._truthy(values.get("exportInvert")): options[OPTION_INVERT] = True existed = device_id in self.exports - self.exports.upsert(ExportEntry( - indigo_device_id=device_id, role=role, - name_override=name_override, options=options, - )) + try: + self.exports.upsert(ExportEntry( + indigo_device_id=device_id, role=role, + name_override=name_override, options=options, + )) + except Exception as exc: # pylint: disable=broad-except + # The store rolled back, so nothing was saved — say so rather than + # reporting the success the old code reported unconditionally. + self.logger.error("Matter export: saving the export list FAILED — %s", exc) + self.logger.exception(exc) + values["exportStatus"] = "FAILED to save the export list — see Event Log" + return values verb = "Updated" if existed else "Added" self.logger.info("%s Matter export: %s (id %s) as %s%s", verb, dev.name, device_id, role, @@ -1322,20 +1503,27 @@ def exportAddOrUpdate(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG0 f"{self._export_summary()}" return values - def exportRemove(self, valuesDict, typeId="", devId=0): # noqa: N802, ARG002 - """Drop the picked device from the allow-list.""" + def exportRemove(self, valuesDict, typeId="", devId=0): + # pylint: disable=unused-argument + """Drop the picked device from the allow-list. Returns values only (see above).""" values = valuesDict - errors = indigo.Dict() if self.exports is None: - errors["exportDevice"] = "Plugin still starting — try again in a moment." - return (values, errors) + values["exportStatus"] = "Plugin still starting — try again in a moment." + return values kind, device_id = self._export_selection(values) if kind == "none": - errors["exportDevice"] = "Select a device to remove from the export list." - return (values, errors) - if not self.exports.remove(device_id): - errors["exportDevice"] = "That device is not exported." - return (values, errors) + values["exportStatus"] = "Select a device to remove from the export list." + return values + try: + removed = self.exports.remove(device_id) + except Exception as exc: # pylint: disable=broad-except + self.logger.error("Matter export: saving the export list FAILED — %s", exc) + self.logger.exception(exc) + values["exportStatus"] = "FAILED to save the export list — see Event Log" + return values + if not removed: + values["exportStatus"] = "That device is not exported." + return values dev = self._indigo_device(device_id) name = str(getattr(dev, "name", "") or "") if dev is not None else f"device {device_id}" self.logger.info("Removed Matter export: %s (id %s)", name, device_id) diff --git a/tests/fakes.py b/tests/fakes.py index 0d79e20..5e80c78 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -199,6 +199,38 @@ class CustomDevice(FakeIndigoDevice): """A plugin-defined `custom` device — no resolvable Matter role.""" +class HostileDevice: + """A device proxy where EVERY attribute access raises. + + Models an Indigo device object being deleted underneath us: the proxy is + still in the list, but reading anything off it blows up. The catalog must + turn this into an ``Excluded`` verdict, never an exception — the picker + walks the whole database, so one of these would otherwise empty the dialog. + ``id``/``name`` raise too, deliberately: the picker cannot even label it. + """ + + def __getattr__(self, name): + raise RuntimeError(f"device proxy is gone (reading {name!r})") + + +class HostilePluginIdDevice(RelayDevice): + """Readable enough to be *listed*, but classification detonates. + + The other half of the class: ``id``/``name`` work, so the picker can build + a row, and then reading ``pluginId`` — the loop guard's only input — + raises. The verdict must be excluded, never eligible: an unreadable + ``pluginId`` is precisely the case where we cannot prove the guard passed. + """ + + @property + def pluginId(self): + raise RuntimeError("pluginId blew up") + + @pluginId.setter + def pluginId(self, value): + pass # the base __init__ assigns it; only the READ has to raise + + def minimal_device(class_name, dev_id=1, name="Device", plugin_id=OTHER_PLUGIN_ID): """A device carrying ONLY id/name/pluginId, in a class of ``class_name``. diff --git a/tests/test_export_catalog.py b/tests/test_export_catalog.py index 914818b..0b8d9c7 100644 --- a/tests/test_export_catalog.py +++ b/tests/test_export_catalog.py @@ -11,7 +11,10 @@ 3. `default_role` is always the FIRST eligible role (the picker's first option must be its safest one); 4. every exclusion carries a non-empty reason (XAC9 shows it in the picker); - 5. the loop guard beats every other rule, for every device shape (XAC6). + 5. the loop guard beats every other rule, for every device shape (XAC6); + 6. reverse coverage — the union of every eligible role across the zoo IS + `bridge_protocol.ROLES`, so a role added to the protocol cannot ship + UI-unreachable (and `classify` never raises, whatever the device does). """ from __future__ import annotations @@ -25,9 +28,10 @@ from bridge_protocol import ROLES from export_catalog import EligibleDevice, Excluded, classify from fakes import ( - OTHER_PLUGIN_ID, CustomDevice, DimmerDevice, + HostileDevice, + HostilePluginIdDevice, MultiIODevice, RelayDevice, SensorDevice, @@ -57,6 +61,14 @@ EligibleDevice(("extendedColorLight", "dimmableLight", "colorTemperatureLight", "windowCovering"), "extendedColorLight"), ), + # RGB WITHOUT white-temperature — the shape a plain colour bulb actually + # ships in. Its own row because the mixed-capability row above cannot tell + # "rgb implies extendedColorLight" apart from "whiteTemp implies it". + "dimmer_rgb_only": ( + DimmerDevice(19, "Party Bulb", supportsColor=True, supportsRGB=True), + EligibleDevice(("extendedColorLight", "dimmableLight", "colorTemperatureLight", + "windowCovering"), "extendedColorLight"), + ), "dimmer_colour_temp_only": ( DimmerDevice(4, "Reading Lamp", supportsColor=True, supportsWhiteTemperature=True), EligibleDevice(("colorTemperatureLight", "dimmableLight", "windowCovering"), @@ -169,6 +181,63 @@ def test_zoo_exclusions_always_carry_a_reason(name): assert verdict.reason.strip() +def test_zoo_offers_every_protocol_role_somewhere(): + """Invariant 6 (reverse coverage) — no role can land UI-unreachable. + + The forward invariant says every role we emit is a real §4.2 role. This is + its mirror: every §4.2 role must be reachable from SOME device shape in the + zoo. Adding a role to `bridge_protocol.ROLES` (and to the node) without a + device that can ever be declared as it would ship a role the picker never + offers — dead protocol surface nobody notices for a milestone. + """ + offered = set() + for device, _expected in ZOO.values(): + # OURS, not OTHER_PLUGIN_ID: the zoo devices themselves carry + # OTHER_PLUGIN_ID, so classifying against it would loop-guard the + # entire zoo and make this invariant vacuous. + verdict = classify(device, OURS) + if isinstance(verdict, EligibleDevice): + offered.update(verdict.eligible_roles) + assert offered == set(ROLES), ( + f"unreachable roles: {sorted(set(ROLES) - offered)}; " + f"non-protocol roles offered: {sorted(offered - set(ROLES))}" + ) + + +# --------------------------------------------------------------------------- +# C1 — classify() never propagates. Indigo proxies die mid-iteration. +# --------------------------------------------------------------------------- +def test_a_device_whose_every_attribute_raises_is_excluded_not_fatal(): + assert classify(HostileDevice(), OURS) == Excluded(export_catalog.REASON_DEVICE_ERROR) + + +def test_an_unreadable_plugin_id_fails_closed(): + """If the loop guard's input cannot be read, the answer is NOT eligible.""" + verdict = classify(HostilePluginIdDevice(1, "Flaky Plug"), OURS) + assert verdict == Excluded(export_catalog.REASON_DEVICE_ERROR) + assert not isinstance(verdict, EligibleDevice) + + +def test_classification_failure_is_logged_once(caplog): + with caplog.at_level("ERROR"): + classify(HostileDevice(), OURS) + assert len([r for r in caplog.records if "could not classify" in r.message]) == 1 + + +def test_a_raising_capability_flag_is_excluded_not_fatal(): + class ExplodingSensor(SensorDevice): + @property + def supportsOnState(self): # noqa: N802 - mirrors Indigo's own naming + raise RuntimeError("state read failed") + + @supportsOnState.setter + def supportsOnState(self, value): + pass + + assert classify(ExplodingSensor(1, "Odd"), OURS) == Excluded( + export_catalog.REASON_DEVICE_ERROR) + + # --------------------------------------------------------------------------- # XAC6 — the loop guard. Required by name in the PRD's acceptance criteria. # --------------------------------------------------------------------------- @@ -280,6 +349,57 @@ def test_unit_heuristic_falls_back_to_the_device_name(): assert classify(dev, OURS).default_role == "temperatureSensor" +# --------------------------------------------------------------------------- +# C2 — the needles are WORDS, not substrings +# --------------------------------------------------------------------------- +# Substring matching turned ordinary English into sensor units: every name +# below contains a needle ("flux" ⊃ lux, "attempt" ⊃ temp, "epsilon" ⊃ psi, +# "overflow" ⊃ flow, "moist"…), and each was silently mis-defaulted. +@pytest.mark.parametrize("name", [ + "Fluxcapacitor", # lux + "Attempt Counter", # temp + "Epsilon Meter", # psi + "Overflow Alarm", # flow + "Deluxe Panel", # lux + "Contempt Index", # temp +]) +def test_a_word_that_merely_contains_a_unit_is_not_a_unit(name): + dev = SensorDevice(1, name, supportsSensorValue=True) + assert classify(dev, OURS) == Excluded(export_catalog.REASON_SENSOR_UNITS), name + + +@pytest.mark.parametrize("text,expected", [ + ("19.5 °C", "temperatureSensor"), + ("Humidity 47%RH", "humiditySensor"), + ("Flow (l/min)", "flowSensor"), + ("340 lux", "lightSensor"), + ("1013 hPa", "pressureSensor"), + ("Tank Pressure", "pressureSensor"), + ("Soil moisture", "humiditySensor"), + ("Light level", "lightSensor"), +], ids=lambda v: str(v)) +def test_genuine_units_still_classify_after_the_word_boundaries(text, expected): + dev = SensorDevice(1, "", supportsSensorValue=True, displayStateValUi=text) + assert classify(dev, OURS).default_role == expected + + +def test_a_declared_unit_outranks_the_device_name(): + """Names are the weakest evidence: a declared unit must win outright. + + "Flow" in a name would otherwise beat a props-declared "%RH" purely + because flow sorts before temperature in the pattern table. + """ + dev = SensorDevice(1, "Bathroom Flow Sensor", supportsSensorValue=True, + pluginProps={"unit": "%RH"}) + assert classify(dev, OURS).default_role == "humiditySensor" + + +def test_the_formatted_value_outranks_the_device_name(): + dev = SensorDevice(1, "Kitchen Temperature", supportsSensorValue=True, + displayStateValUi="1013 hPa") + assert classify(dev, OURS).default_role == "pressureSensor" + + def test_humidity_beats_temperature_in_a_combined_name(): """First match wins, and 'humidity' is the unambiguous word.""" dev = SensorDevice(1, "Temperature/Humidity — Humidity", supportsSensorValue=True, @@ -307,7 +427,8 @@ def test_excluded_roles_are_documented_with_reasons(): def test_excluded_roles_are_never_offered(): offered = set() for device, _expected in ZOO.values(): - verdict = classify(device, OTHER_PLUGIN_ID) + # OURS for the same reason as the reverse-coverage invariant above. + verdict = classify(device, OURS) if isinstance(verdict, EligibleDevice): offered.update(verdict.eligible_roles) assert offered.isdisjoint(export_catalog.EXCLUDED_ROLES) diff --git a/tests/test_export_menu.py b/tests/test_export_menu.py index b15c1b5..b1b608a 100644 --- a/tests/test_export_menu.py +++ b/tests/test_export_menu.py @@ -11,10 +11,17 @@ catalog level in `test_export_catalog.py`); * **XAC9** — excluded devices appear in the picker *with reasons*, never silently missing. + +Two contracts hardened in PR #122 are pinned here too: button `CallbackMethod`s +return **the values dict only** (the SDK documents a dict, not a +`(values, errors)` tuple — that shape belongs to validation methods), so every +refusal has to be visible in `exportStatus`; and one unreadable device costs one +picker row, never the whole list. """ from __future__ import annotations import importlib +import json import xml.etree.ElementTree as ET from pathlib import Path from unittest.mock import Mock @@ -27,6 +34,8 @@ OTHER_PLUGIN_ID, DimmerDevice, FakeIndigoDevices, + HostileDevice, + HostilePluginIdDevice, RelayDevice, SensorDevice, SprinklerDevice, @@ -65,7 +74,7 @@ def plug(plugin_mod, devices): # noqa: ARG001 - devices installs indigo.devices p.logger = Mock() p.pluginId = OURS p.pluginPrefs = {} - p.exports = ExportStore(p.pluginPrefs, p.logger) + p.exports = ExportStore(lambda: p.pluginPrefs, p.logger) return p @@ -202,34 +211,110 @@ def test_picker_marks_already_exported_devices(plug): assert labels["102"] == "Hall Dimmer" -def test_picker_filter_is_case_insensitive_substring(plug): +def test_picker_filter_is_case_insensitive_substring(plug, plugin_mod): labels = _labels(plug.getExportCandidates(valuesDict=_values(exportFilter="hAlL"))) - assert set(labels) == {"102", "103"} + assert set(labels) == {plugin_mod.NO_SELECTION_ID, "102", "103"} + + +def test_picker_always_leads_with_a_real_row_for_the_seeded_value(plug, plugin_mod): + """The dialog is SEEDED with exportDevice="0", so "0" must be a real row. + + Without it the menu opens on a value that matches no option, which Indigo + renders as a blank first item the user cannot get back to. + """ + options = plug.getExportCandidates(valuesDict=_values()) + assert options[0] == (plugin_mod.NO_SELECTION_ID, plugin_mod.NO_SELECTION_LABEL) + +def test_picker_option_ids_are_unique(plug, devices, plugin_mod): + """"0" belongs to the seed row alone — the tail may not reuse it.""" + for device_id in range(1000, 1000 + plugin_mod.EXPORT_PICKER_LIMIT + 5): + devices.add(RelayDevice(device_id, f"Bulk {device_id}")) + ids = [key for key, _label in plug.getExportCandidates(valuesDict=_values())] + assert len(ids) == len(set(ids)) -def test_picker_filter_with_no_matches_still_returns_a_legal_option(plug): + +def test_picker_filter_with_no_matches_still_returns_a_legal_option(plug, plugin_mod): options = plug.getExportCandidates(valuesDict=_values(exportFilter="zzz")) - assert options == [("0", "(no devices match the filter)")] + assert options == [(plugin_mod.NO_SELECTION_ID, plugin_mod.NO_SELECTION_LABEL), + plugin_mod.NO_MATCH_OPTION] def test_picker_caps_the_list_and_offers_a_narrowing_tail(plug, devices, plugin_mod): for device_id in range(1000, 1000 + plugin_mod.EXPORT_PICKER_LIMIT + 5): devices.add(RelayDevice(device_id, f"Bulk {device_id}")) options = plug.getExportCandidates(valuesDict=_values()) - assert len(options) == plugin_mod.EXPORT_PICKER_LIMIT + 1 - tail_id, tail_label = options[-1] - assert tail_id == "0" - assert "narrow the filter" in tail_label + # seed row + the cap + the tail + assert len(options) == plugin_mod.EXPORT_PICKER_LIMIT + 2 + assert options[-1] == plugin_mod.TRUNCATED_OPTION + assert options[-1][0] != plugin_mod.NO_SELECTION_ID + +def test_the_truncation_tail_is_not_a_selectable_device(plug, plugin_mod): + assert plug._export_selection( + {"exportDevice": plugin_mod.TRUNCATED_OPTION[0]}) == ("none", 0) + assert plug._export_selection( + {"exportDevice": plugin_mod.NO_MATCH_OPTION[0]}) == ("none", 0) -def test_picker_degrades_to_empty_on_error(plug, mock_indigo_base): + +def test_picker_says_so_when_it_fails_outright(plug, mock_indigo_base, plugin_mod): boom = Mock() boom.__iter__ = Mock(side_effect=RuntimeError("boom")) mock_indigo_base.devices = boom - assert plug.getExportCandidates(valuesDict=_values()) == [] + assert plug.getExportCandidates(valuesDict=_values()) == [plugin_mod.LIST_ERROR_OPTION] plug.logger.exception.assert_called() +# --------------------------------------------------------------------------- +# D3 — one bad device costs one row, not the whole dialog +# --------------------------------------------------------------------------- +def test_one_unreadable_device_costs_one_row(plug, devices, plugin_mod): + devices.add(HostileDevice()) + options = plug.getExportCandidates(valuesDict=_values()) + labels = _labels(options) + assert labels["101"] == "Study Plug" # the rest of the list survives + broken = [label for label in labels.values() if plugin_mod.ROW_ERROR_LABEL in label] + assert len(broken) == 1 + plug.logger.exception.assert_called() + + +def test_many_unreadable_devices_log_one_stack_trace(plug, devices): + for _ in range(5): + devices.add(HostileDevice()) + plug.getExportCandidates(valuesDict=_values()) + assert plug.logger.exception.call_count == 1 + assert plug.logger.error.call_count == 4 + + +def test_an_unclassifiable_device_is_an_excluded_row_not_a_crash(plug, devices): + """C1 end-to-end: classify() absorbs it, so the picker shows a reason.""" + devices.add(HostilePluginIdDevice(555, "Flaky Plug")) + labels = _labels(plug.getExportCandidates(valuesDict=_values())) + assert "555" not in labels # not selectable + assert "error reading device" in labels["x-555"] + + +def test_current_exports_contains_a_bad_row(plug, plugin_mod, monkeypatch): + plug.exports.upsert(ExportEntry(101, "onOffPlugInUnit")) + plug.exports.upsert(ExportEntry(102, "dimmableLight")) + real = export_catalog.role_label + + def flaky(role): + if role == "onOffPlugInUnit": + raise RuntimeError("boom") + return real(role) + + monkeypatch.setattr(export_catalog, "role_label", flaky) + labels = _labels(plug.getCurrentExports()) + assert "102" in labels + assert any(plugin_mod.ROW_ERROR_LABEL in label for label in labels.values()) + + +def test_current_exports_says_so_when_it_fails_outright(plug, plugin_mod, monkeypatch): + monkeypatch.setattr(plug.exports, "all", Mock(side_effect=RuntimeError("boom"))) + assert plug.getCurrentExports() == [plugin_mod.LIST_ERROR_OPTION] + + def test_picker_works_before_the_store_exists(plug): plug.exports = None assert _labels(plug.getExportCandidates(valuesDict=_values()))["101"] == "Study Plug" @@ -260,10 +345,13 @@ def test_role_picker_is_empty_for_a_stale_device_id(plug): assert plug.getExportRoles(valuesDict=_values(exportDevice="999999")) == [] -def test_role_picker_degrades_to_empty_on_error(plug, monkeypatch): +def test_role_picker_says_so_when_it_fails_outright(plug, plugin_mod, monkeypatch): + """An empty role menu means "nothing you may choose"; a failure must not + borrow that meaning.""" monkeypatch.setattr(export_catalog, "classify", Mock(side_effect=RuntimeError("boom"))) - assert plug.getExportRoles(valuesDict=_values(exportDevice="101")) == [] + assert plug.getExportRoles( + valuesDict=_values(exportDevice="101")) == [plugin_mod.LIST_ERROR_OPTION] plug.logger.exception.assert_called() @@ -326,6 +414,45 @@ def test_device_changed_reports_our_own_device_as_loop_guarded(plug): assert export_catalog.REASON_LOOP_GUARD in values["exportStatus"] +# --------------------------------------------------------------------------- +# D4 — excluded AND exported is a real state, and it has to be legible +# --------------------------------------------------------------------------- +def test_picker_keeps_the_exported_marker_on_an_excluded_device(plug): + """A device that became unexportable AFTER it was exported keeps its dot. + + Dropping the marker makes the picker disagree with the "Currently exported" + list, which reads as a picker bug rather than the stale export it is. + """ + plug.exports.upsert(ExportEntry(104, "onOffPlugInUnit")) + label = _labels(plug.getExportCandidates(valuesDict=_values()))["x-104"] + assert label.startswith("● ") + assert export_catalog.REASON_SPRINKLER in label + + +def test_device_changed_warns_when_an_excluded_device_is_still_exported(plug): + plug.exports.upsert(ExportEntry(104, "onOffPlugInUnit")) + values = plug.exportDeviceChanged(_values(exportDevice="x-104"), "manageMatterExports") + assert "IS currently exported" in values["exportStatus"] + assert "fail to bridge" in values["exportStatus"] + + +def test_device_changed_does_not_cry_wolf_for_an_unexported_exclusion(plug): + values = plug.exportDeviceChanged(_values(exportDevice="x-104"), "manageMatterExports") + assert "IS currently exported" not in values["exportStatus"] + + +def test_device_changed_clears_the_whole_pane_for_an_excluded_pick(plug, devices): + """The second excluded branch used to leave a stale name/polarity behind.""" + devices.add(HostilePluginIdDevice(555, "Flaky Plug")) + values = plug.exportDeviceChanged( + _values(exportDevice="555", exportRole="onOffLight", exportName="Stale", + exportInvert=True), + "manageMatterExports") + assert values["exportRole"] == "" + assert values["exportName"] == "" + assert values["exportInvert"] is False + + def test_device_changed_clears_the_detail_pane_on_no_selection(plug): values = plug.exportDeviceChanged( _values(exportDevice="0", exportRole="doorLock", exportName="X"), "manageMatterExports") @@ -390,50 +517,49 @@ def test_add_records_covering_polarity_only_for_coverings(plug): def test_add_requires_a_selection(plug): - _values_out, errors = plug.exportAddOrUpdate(_values(), "manageMatterExports") - assert "exportDevice" in errors + values = plug.exportAddOrUpdate(_values(), "manageMatterExports") + assert "Select a device to export" in values["exportStatus"] assert len(plug.exports) == 0 -def test_add_rejects_an_excluded_pick_with_a_field_error(plug): - values, errors = plug.exportAddOrUpdate( +def test_add_rejects_an_excluded_pick_in_the_status_field(plug): + values = plug.exportAddOrUpdate( _values(exportDevice="x-104", exportRole="onOffLight"), "manageMatterExports") - assert export_catalog.REASON_SPRINKLER in errors["exportDevice"] assert export_catalog.REASON_SPRINKLER in values["exportStatus"] assert len(plug.exports) == 0 def test_xac6_add_refuses_our_own_device_even_if_the_id_is_unprefixed(plug): """Server-side rejection, not just a picker label — the guard is structural.""" - _out, errors = plug.exportAddOrUpdate( + values = plug.exportAddOrUpdate( _values(exportDevice="105", exportRole="onOffLight"), "manageMatterExports") - assert export_catalog.REASON_LOOP_GUARD in errors["exportDevice"] + assert export_catalog.REASON_LOOP_GUARD in values["exportStatus"] assert len(plug.exports) == 0 def test_add_rejects_a_role_the_device_is_not_eligible_for(plug): - _out, errors = plug.exportAddOrUpdate( + values = plug.exportAddOrUpdate( _values(exportDevice="101", exportRole="thermostat"), "manageMatterExports") - assert "exportRole" in errors + assert "Choose how this device should appear" in values["exportStatus"] assert len(plug.exports) == 0 def test_add_rejects_an_empty_role(plug): - _out, errors = plug.exportAddOrUpdate(_values(exportDevice="101"), "manageMatterExports") - assert "exportRole" in errors + values = plug.exportAddOrUpdate(_values(exportDevice="101"), "manageMatterExports") + assert "Choose how this device should appear" in values["exportStatus"] def test_add_rejects_a_stale_device_id(plug): - _out, errors = plug.exportAddOrUpdate( + values = plug.exportAddOrUpdate( _values(exportDevice="999999", exportRole="onOffLight"), "manageMatterExports") - assert "no longer exists" in errors["exportDevice"] + assert "no longer exists" in values["exportStatus"] -def test_add_before_startup_is_a_dialog_error(plug): +def test_add_before_startup_says_so(plug): plug.exports = None - _out, errors = plug.exportAddOrUpdate( + values = plug.exportAddOrUpdate( _values(exportDevice="101", exportRole="onOffLight"), "manageMatterExports") - assert "still starting" in errors["exportDevice"] + assert "still starting" in values["exportStatus"] # --------------------------------------------------------------------------- @@ -449,13 +575,13 @@ def test_remove_drops_the_entry_and_clears_the_pane(plug): def test_remove_requires_a_selection(plug): - _out, errors = plug.exportRemove(_values(), "manageMatterExports") - assert "exportDevice" in errors + values = plug.exportRemove(_values(), "manageMatterExports") + assert "Select a device to remove" in values["exportStatus"] -def test_remove_of_something_not_exported_is_a_field_error(plug): - _out, errors = plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") - assert errors["exportDevice"] == "That device is not exported." +def test_remove_of_something_not_exported_is_reported(plug): + values = plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") + assert values["exportStatus"] == "That device is not exported." def test_remove_works_for_a_deleted_indigo_device(plug): @@ -465,10 +591,99 @@ def test_remove_works_for_a_deleted_indigo_device(plug): assert "device 999999" in values["exportStatus"] -def test_remove_before_startup_is_a_dialog_error(plug): +def test_remove_before_startup_says_so(plug): + plug.exports = None + values = plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") + assert "still starting" in values["exportStatus"] + + +# --------------------------------------------------------------------------- +# D1 — button callbacks return the values dict ONLY +# --------------------------------------------------------------------------- +# The SDK's button reference documents a button CallbackMethod as returning "a +# dictionary back to the dialog containing any field changes"; the +# (valuesDict, errorsDict) tuple belongs to the VALIDATION methods. It also says +# a button's own field is read-only and so cannot carry an error message. Every +# refusal therefore has to reach the user through exportStatus. +BUTTON_CALLS = [ + ("exportReloadPicker", _values()), + ("exportAddOrUpdate", _values()), # no selection + ("exportAddOrUpdate", _values(exportDevice="x-104")), # excluded + ("exportAddOrUpdate", _values(exportDevice="999999")), # stale id + ("exportAddOrUpdate", _values(exportDevice="101")), # no role + ("exportAddOrUpdate", _values(exportDevice="101", exportRole="thermostat")), + ("exportAddOrUpdate", _values(exportDevice="101", exportRole="onOffLight")), + ("exportRemove", _values()), + ("exportRemove", _values(exportDevice="101")), +] + + +@pytest.mark.parametrize("callback,values", BUTTON_CALLS, + ids=[f"{name}-{i}" for i, (name, _v) in enumerate(BUTTON_CALLS)]) +def test_button_callbacks_return_a_bare_values_dict(plug, callback, values): + result = getattr(plug, callback)(dict(values), "manageMatterExports") + assert not isinstance(result, tuple), f"{callback} returned the undocumented tuple" + assert "exportStatus" in result + + +@pytest.mark.parametrize("callback,values", BUTTON_CALLS, + ids=[f"{name}-{i}" for i, (name, _v) in enumerate(BUTTON_CALLS)]) +def test_every_button_path_leaves_a_non_empty_status(plug, callback, values): + """Including the early returns: a silent dialog is indistinguishable from + a dead button.""" + result = getattr(plug, callback)(dict(values), "manageMatterExports") + assert str(result["exportStatus"]).strip() + + +@pytest.mark.parametrize("callback", ["exportAddOrUpdate", "exportRemove"]) +def test_button_callbacks_before_startup_still_return_a_dict(plug, callback): plug.exports = None - _out, errors = plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") - assert "still starting" in errors["exportDevice"] + result = getattr(plug, callback)( + _values(exportDevice="101", exportRole="onOffLight"), "manageMatterExports") + assert not isinstance(result, tuple) + + +# --------------------------------------------------------------------------- +# S6 — a failed save is never reported as success +# --------------------------------------------------------------------------- +def test_add_reports_a_failed_save_instead_of_claiming_success(plug, monkeypatch): + monkeypatch.setattr(plug.exports, "upsert", Mock(side_effect=RuntimeError("prefs died"))) + values = plug.exportAddOrUpdate( + _values(exportDevice="101", exportRole="onOffLight"), "manageMatterExports") + assert values["exportStatus"] == "FAILED to save the export list — see Event Log" + plug.logger.exception.assert_called() + + +def test_remove_reports_a_failed_save_instead_of_claiming_success(plug, monkeypatch): + plug.exports.upsert(ExportEntry(101, "onOffLight")) + monkeypatch.setattr(plug.exports, "remove", Mock(side_effect=RuntimeError("prefs died"))) + values = plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") + assert values["exportStatus"] == "FAILED to save the export list — see Event Log" + plug.logger.exception.assert_called() + + +# --------------------------------------------------------------------------- +# S3 — a load failure must never render as "Nothing is exported yet." +# --------------------------------------------------------------------------- +def test_the_dialog_reports_an_unreadable_export_list(plug): + plug.pluginPrefs = {PREF_KEY: "{broken"} + plug.exports = ExportStore(lambda: plug.pluginPrefs, plug.logger) + status = plug.get_menu_action_config_ui_values("manageMatterExports")["exportStatus"] + assert "could not be read" in status + assert "preserved" in status + assert "Nothing is exported yet." not in status + + +def test_the_dialog_reports_dropped_rows_alongside_the_count(plug): + blob = json.dumps({"v": 1, "exports": [ + {"indigoDeviceId": 101, "role": "onOffLight"}, + {"indigoDeviceId": 102, "role": "teleporter"}, + ]}) + plug.pluginPrefs = {PREF_KEY: blob} + plug.exports = ExportStore(lambda: plug.pluginPrefs, plug.logger) + status = plug._export_summary() + assert "could not be read" in status + assert "1 device(s) exported" in status # --------------------------------------------------------------------------- @@ -515,7 +730,9 @@ def test_plugin_id_falls_back_to_the_bundle_constant(plug): # --------------------------------------------------------------------------- # startup wiring — the allow-list must exist before anything consults it # --------------------------------------------------------------------------- -def test_startup_loads_the_allow_list_from_prefs(plugin_mod, monkeypatch): +@pytest.fixture +def started(plugin_mod, devices, monkeypatch): # noqa: ARG001 - devices installs indigo.devices + """A Plugin whose startup() can be driven without touching the real host.""" class FakeRuntimeObj: is_running = True @@ -535,17 +752,115 @@ def submit(self, coro): # MUST be patched — an unpatched ServerProcess touches the real $HOME (#104). monkeypatch.setattr(plugin_mod, "ServerProcess", lambda *a, **k: Mock()) - p = plugin_mod.Plugin.__new__(plugin_mod.Plugin) - p.logger = Mock() - p.pluginPrefs = {} - p.proto = object() - p.registry = object() - p.device_sync = Mock() - p.runtime = None - p.server_process = None - p.exports = None - ExportStore(p.pluginPrefs, p.logger).upsert(ExportEntry(101, "onOffLight")) + def build(prefs=None): + p = plugin_mod.Plugin.__new__(plugin_mod.Plugin) + p.logger = Mock() + p.pluginId = OURS + p.pluginPrefs = {} if prefs is None else prefs + p.proto = object() + p.registry = object() + p.device_sync = Mock() + p.runtime = None + p.server_process = None + p.exports = None + return p + return build + + +def _seed(prefs, *entries): + store = ExportStore(lambda: prefs, Mock()) + for entry in entries: + store.upsert(entry) + return prefs + + +def test_startup_loads_the_allow_list_from_prefs(started): + p = started() + _seed(p.pluginPrefs, ExportEntry(101, "onOffLight")) p.startup() assert p.exports is not None assert p.exports.ids() == frozenset({101}) + + +def test_startup_wires_the_store_to_indigos_prefs_flush(started, mock_indigo_base): + """S1 — the store's commit step must be a REAL flush, not the default no-op.""" + p = started() + p.startup() + mock_indigo_base.server.savePluginPrefs.reset_mock() + p.exports.upsert(ExportEntry(101, "onOffLight")) + mock_indigo_base.server.savePluginPrefs.assert_called_once() + + +def test_startup_wires_the_store_to_a_live_prefs_binding(started): + """S2 — a PluginConfig save can hand the plugin a NEW pluginPrefs object.""" + p = started() + p.startup() + p.pluginPrefs = {} # Indigo rebinds it + p.exports.upsert(ExportEntry(101, "onOffLight")) + assert PREF_KEY in p.pluginPrefs # the write followed the rebinding + + +def test_startup_drops_a_restored_entry_for_our_own_device(started): + """S5 — load is an unguarded write path; the loop guard is re-run there. + + Device 105 carries OUR pluginId. A blob restored from a backup (or edited + by hand) that exports it must not survive into the store, or E3 would build + an endpoint for a device this plugin created. + """ + prefs = _seed({}, ExportEntry(101, "onOffLight"), ExportEntry(105, "onOffLight")) + p = started(prefs) + p.startup() + assert p.exports.ids() == frozenset({101}) + assert p.exports.load_error is not None + + +def test_startup_keeps_a_merely_unexportable_entry(started): + """Only the loop guard deletes. Ordinary ineligibility is reported, not acted on.""" + prefs = _seed({}, ExportEntry(104, "onOffPlugInUnit")) # sprinkler + p = started(prefs) + p.startup() + assert p.exports.ids() == frozenset({104}) + + +# --------------------------------------------------------------------------- +# R1 — startup reconcile is REPORT-ONLY +# --------------------------------------------------------------------------- +def test_reconcile_warns_about_a_deleted_device_but_keeps_it(started): + prefs = _seed({}, ExportEntry(999999, "onOffLight")) + p = started(prefs) + p.startup() + assert p.exports.ids() == frozenset({999999}) + assert any("no longer exists" in str(call) + for call in p.logger.warning.call_args_list) + + +def test_reconcile_warns_about_a_now_unexportable_device(started): + prefs = _seed({}, ExportEntry(104, "onOffPlugInUnit")) # sprinkler + p = started(prefs) + p.startup() + assert any("no longer exportable" in str(call) + for call in p.logger.warning.call_args_list) + + +def test_reconcile_warns_about_a_role_the_device_no_longer_offers(started): + prefs = _seed({}, ExportEntry(101, "windowCovering")) # relay: no covering role + p = started(prefs) + p.startup() + assert any("no longer offers" in str(call) + for call in p.logger.warning.call_args_list) + + +def test_reconcile_is_silent_when_everything_is_fine(started): + prefs = _seed({}, ExportEntry(101, "onOffPlugInUnit")) + p = started(prefs) + p.startup() + assert p.logger.warning.call_args_list == [] + + +def test_reconcile_never_fails_startup(started, monkeypatch): + prefs = _seed({}, ExportEntry(101, "onOffPlugInUnit")) + p = started(prefs) + monkeypatch.setattr(export_catalog, "classify", Mock(side_effect=RuntimeError("boom"))) + p.startup() # must not raise + assert p.exports.ids() == frozenset({101}) diff --git a/tests/test_export_store.py b/tests/test_export_store.py index 43ff629..208cda0 100644 --- a/tests/test_export_store.py +++ b/tests/test_export_store.py @@ -14,6 +14,7 @@ from bridge_protocol import ROLES from export_store import ( + LOAD_ERROR_UNREADABLE, OPTION_INVERT, PREF_KEY, PREF_KEY_CORRUPT, @@ -30,7 +31,7 @@ def prefs(): @pytest.fixture def store(prefs, mock_logger): - return ExportStore(prefs, mock_logger) + return ExportStore(lambda: prefs, mock_logger) def _entry(device_id=101, role="onOffLight", **kwargs): @@ -124,11 +125,11 @@ def test_write_persists_immediately_to_prefs(prefs, store): def test_round_trips_through_a_second_store(prefs, mock_logger): - first = ExportStore(prefs, mock_logger) + first = ExportStore(lambda: prefs, mock_logger) first.upsert(_entry(7, "windowCovering", name_override="Blind", options={OPTION_INVERT: True})) first.upsert(_entry(8, "thermostat")) - second = ExportStore(prefs, mock_logger) + second = ExportStore(lambda: prefs, mock_logger) assert second.all() == first.all() @@ -136,20 +137,134 @@ def test_remove_persists(prefs, store, mock_logger): store.upsert(_entry(1)) store.upsert(_entry(2)) store.remove(1) - assert ExportStore(prefs, mock_logger).ids() == frozenset({2}) + assert ExportStore(lambda: prefs, mock_logger).ids() == frozenset({2}) def test_blank_pref_is_simply_empty(mock_logger): - assert ExportStore({PREF_KEY: ""}, mock_logger).all() == () + assert ExportStore(lambda: {PREF_KEY: ""}, mock_logger).all() == () mock_logger.error.assert_not_called() +# --------------------------------------------------------------------------- +# S1 — persist-then-commit: memory and prefs may never diverge +# --------------------------------------------------------------------------- +class _Flush: + """A save_prefs stand-in that can be made to fail, and records its order.""" + + def __init__(self, prefs, fail=False): + self._prefs = prefs + self.fail = fail + self.saw = [] + + def __call__(self): + # Captured at flush time: proves the pref was written BEFORE the flush. + self.saw.append(self._prefs.get(PREF_KEY)) + if self.fail: + raise RuntimeError("Indigo refused the write") + + +def test_commit_writes_the_pref_before_flushing(prefs, mock_logger): + flush = _Flush(prefs) + store = ExportStore(lambda: prefs, mock_logger, save_prefs=flush) + store.upsert(_entry(1)) + assert len(flush.saw) == 1 + assert "indigoDeviceId" in flush.saw[0] + + +def test_upsert_that_fails_to_flush_changes_nothing(prefs, mock_logger): + flush = _Flush(prefs) + store = ExportStore(lambda: prefs, mock_logger, save_prefs=flush) + store.upsert(_entry(1)) + good_blob = prefs[PREF_KEY] + + flush.fail = True + with pytest.raises(RuntimeError): + store.upsert(_entry(2)) + # Memory unchanged... + assert store.ids() == frozenset({1}) + # ...and so is the pref, so a restart agrees with the dialog. + assert prefs[PREF_KEY] == good_blob + assert ExportStore(lambda: prefs, mock_logger).ids() == frozenset({1}) + + +def test_remove_that_fails_to_flush_does_not_resurrect_the_device(prefs, mock_logger): + """The probe case: a failed remove-save must not delete it from memory only. + + Pre-#122 the entry vanished from the dialog and came back on restart. + """ + flush = _Flush(prefs) + store = ExportStore(lambda: prefs, mock_logger, save_prefs=flush) + store.upsert(_entry(1)) + store.upsert(_entry(2)) + + flush.fail = True + with pytest.raises(RuntimeError): + store.remove(2) + assert store.ids() == frozenset({1, 2}) + assert ExportStore(lambda: prefs, mock_logger).ids() == frozenset({1, 2}) + + +def test_first_ever_write_that_fails_leaves_no_pref_key(prefs, mock_logger): + flush = _Flush(prefs, fail=True) + store = ExportStore(lambda: prefs, mock_logger, save_prefs=flush) + with pytest.raises(RuntimeError): + store.upsert(_entry(1)) + assert PREF_KEY not in prefs + assert store.all() == () + + +def test_replace_all_that_fails_to_flush_keeps_the_old_list(prefs, mock_logger): + flush = _Flush(prefs) + store = ExportStore(lambda: prefs, mock_logger, save_prefs=flush) + store.replace_all([_entry(1), _entry(2)]) + flush.fail = True + with pytest.raises(RuntimeError): + store.replace_all([_entry(9)]) + assert store.ids() == frozenset({1, 2}) + + +def test_removing_something_absent_does_not_flush(prefs, mock_logger): + flush = _Flush(prefs) + store = ExportStore(lambda: prefs, mock_logger, save_prefs=flush) + assert store.remove(42) is False + assert flush.saw == [] + + +# --------------------------------------------------------------------------- +# S2 — prefs are resolved late (Indigo can rebind pluginPrefs) +# --------------------------------------------------------------------------- +def test_writes_follow_a_rebound_prefs_mapping(mock_logger): + """A PluginConfig save can hand the plugin a NEW pluginPrefs object. + + The store must write to whatever ``prefs_getter`` returns now, not to the + mapping it happened to see at startup — otherwise every export lands on an + orphan dict nobody persists. + """ + holder = {"prefs": {}} + store = ExportStore(lambda: holder["prefs"], mock_logger) + store.upsert(_entry(1)) + original = holder["prefs"] + + holder["prefs"] = {} # Indigo rebinds self.pluginPrefs + store.upsert(_entry(2)) + + assert PREF_KEY in holder["prefs"] + assert len(json.loads(holder["prefs"][PREF_KEY])["exports"]) == 2 + assert json.loads(original[PREF_KEY])["exports"][0]["indigoDeviceId"] == 1 + + +def test_load_reads_through_the_getter_too(mock_logger): + prefs = {} + ExportStore(lambda: prefs, mock_logger).upsert(_entry(5)) + assert ExportStore(lambda: prefs, mock_logger).ids() == frozenset({5}) + + # --------------------------------------------------------------------------- # Corrupt-blob preservation — never silently discard user config # --------------------------------------------------------------------------- def test_unparseable_json_is_preserved_and_store_starts_empty(mock_logger): prefs = {PREF_KEY: "{not json at all"} - store = ExportStore(prefs, mock_logger) + store = ExportStore(lambda: prefs, mock_logger) assert store.all() == () assert prefs[PREF_KEY_CORRUPT] == "{not json at all" mock_logger.error.assert_called() @@ -158,21 +273,21 @@ def test_unparseable_json_is_preserved_and_store_starts_empty(mock_logger): def test_wrong_schema_version_is_preserved_not_reinterpreted(mock_logger): blob = json.dumps({"v": 99, "exports": [{"indigoDeviceId": 1, "role": "onOffLight"}]}) prefs = {PREF_KEY: blob} - store = ExportStore(prefs, mock_logger) + store = ExportStore(lambda: prefs, mock_logger) assert store.all() == () assert prefs[PREF_KEY_CORRUPT] == blob def test_non_object_payload_is_preserved(mock_logger): prefs = {PREF_KEY: "[1, 2, 3]"} - assert ExportStore(prefs, mock_logger).all() == () + assert ExportStore(lambda: prefs, mock_logger).all() == () assert prefs[PREF_KEY_CORRUPT] == "[1, 2, 3]" def test_exports_not_a_list_is_preserved(mock_logger): blob = json.dumps({"v": SCHEMA_VERSION, "exports": {"nope": True}}) prefs = {PREF_KEY: blob} - assert ExportStore(prefs, mock_logger).all() == () + assert ExportStore(lambda: prefs, mock_logger).all() == () assert prefs[PREF_KEY_CORRUPT] == blob @@ -184,7 +299,7 @@ def test_one_bad_entry_drops_only_itself_and_keeps_the_blob(mock_logger): {"indigoDeviceId": 3, "role": "thermostat"}, ]}) prefs = {PREF_KEY: blob} - store = ExportStore(prefs, mock_logger) + store = ExportStore(lambda: prefs, mock_logger) assert store.ids() == frozenset({1, 3}) assert prefs[PREF_KEY_CORRUPT] == blob assert mock_logger.error.call_count == 2 @@ -216,6 +331,113 @@ def test_entry_from_dict_rejects_non_string_name_override(): ExportEntry.from_dict({"indigoDeviceId": 1, "role": "onOffLight", "nameOverride": 7}) +# --------------------------------------------------------------------------- +# S3 — load_error: the dialog must not claim an empty list is intentional +# --------------------------------------------------------------------------- +def test_clean_load_sets_no_load_error(prefs, mock_logger): + ExportStore(lambda: prefs, mock_logger).upsert(_entry(1)) + assert ExportStore(lambda: prefs, mock_logger).load_error is None + + +def test_corrupt_blob_sets_the_load_error(mock_logger): + prefs = {PREF_KEY: "{not json at all"} + store = ExportStore(lambda: prefs, mock_logger) + assert store.load_error == LOAD_ERROR_UNREADABLE + assert "preserved" in store.load_error + + +def test_dropped_rows_set_a_counted_load_error(mock_logger): + blob = json.dumps({"v": SCHEMA_VERSION, "exports": [ + {"indigoDeviceId": 1, "role": "onOffLight"}, + {"indigoDeviceId": 2, "role": "teleporter"}, + ]}) + store = ExportStore(lambda: {PREF_KEY: blob}, mock_logger) + assert store.load_error is not None + assert "1 saved export(s)" in store.load_error + + +# --------------------------------------------------------------------------- +# S4 — the rescue copy: first one wins, and nothing later clobbers it +# --------------------------------------------------------------------------- +def test_rescue_copy_survives_a_later_successful_save(mock_logger): + prefs = {PREF_KEY: "{broken"} + store = ExportStore(lambda: prefs, mock_logger) + rescued = prefs[PREF_KEY_CORRUPT] + store.upsert(_entry(1)) # the user rebuilds one export + assert prefs[PREF_KEY_CORRUPT] == rescued + assert "indigoDeviceId" in prefs[PREF_KEY] + + +def test_second_corruption_does_not_overwrite_the_first_rescue(mock_logger): + prefs = {PREF_KEY: "ORIGINAL-twenty-exports"} + ExportStore(lambda: prefs, mock_logger) + assert prefs[PREF_KEY_CORRUPT] == "ORIGINAL-twenty-exports" + + prefs[PREF_KEY] = "second-mangled-blob" + mock_logger.reset_mock() + ExportStore(lambda: prefs, mock_logger) + assert prefs[PREF_KEY_CORRUPT] == "ORIGINAL-twenty-exports" + # Never silent: the user is told the newer blob was NOT kept. + assert any("NOT preserved" in str(call) for call in mock_logger.error.call_args_list) + + +# --------------------------------------------------------------------------- +# S5 — restored entries are re-validated (load is an unguarded write path) +# --------------------------------------------------------------------------- +def _blob(*entries): + return json.dumps({"v": SCHEMA_VERSION, "exports": list(entries)}) + + +def test_validator_drops_a_rejected_entry_and_preserves_the_blob(mock_logger): + blob = _blob({"indigoDeviceId": 1, "role": "onOffLight"}, + {"indigoDeviceId": 2, "role": "onOffLight"}) + prefs = {PREF_KEY: blob} + store = ExportStore( + lambda: prefs, mock_logger, + entry_validator=lambda e: "created by this plugin (loop guard)" + if e.indigo_device_id == 2 else None, + ) + assert store.ids() == frozenset({1}) + assert prefs[PREF_KEY_CORRUPT] == blob # recoverable, as with a bad row + assert store.load_error is not None + mock_logger.error.assert_called() + + +def test_validator_that_raises_keeps_the_entry_and_logs(mock_logger): + """A broken validator must not silently empty the user's allow-list.""" + def boom(_entry): + raise RuntimeError("indigo.devices exploded") + + store = ExportStore(lambda: {PREF_KEY: _blob({"indigoDeviceId": 1, "role": "onOffLight"})}, + mock_logger, entry_validator=boom) + assert store.ids() == frozenset({1}) + mock_logger.exception.assert_called() + + +def test_no_validator_accepts_everything(mock_logger): + store = ExportStore(lambda: {PREF_KEY: _blob({"indigoDeviceId": 1, "role": "onOffLight"})}, + mock_logger) + assert store.ids() == frozenset({1}) + + +def test_from_dict_rejects_invert_on_a_role_with_no_polarity(): + with pytest.raises(ValueError, match="polarity"): + ExportEntry.from_dict({"indigoDeviceId": 1, "role": "doorLock", + "options": {OPTION_INVERT: True}}) + + +def test_from_dict_accepts_invert_on_a_window_covering(): + entry = ExportEntry.from_dict({"indigoDeviceId": 1, "role": "windowCovering", + "options": {OPTION_INVERT: True}}) + assert entry.options == {OPTION_INVERT: True} + + +def test_from_dict_rejects_a_non_boolean_invert(): + with pytest.raises(ValueError, match="boolean"): + ExportEntry.from_dict({"indigoDeviceId": 1, "role": "windowCovering", + "options": {OPTION_INVERT: "yes"}}) + + def test_preservation_failure_does_not_raise(mock_logger): class HostileMapping(dict): def __setitem__(self, key, value): @@ -224,7 +446,7 @@ def __setitem__(self, key, value): super().__setitem__(key, value) prefs = HostileMapping({PREF_KEY: "{broken"}) - store = ExportStore(prefs, mock_logger) # must not raise + store = ExportStore(lambda: prefs, mock_logger) # must not raise assert store.all() == () mock_logger.exception.assert_called() @@ -247,7 +469,7 @@ def test_public_calls_nest_without_deadlocking(store): def test_concurrent_writers_all_land(prefs, mock_logger): - store = ExportStore(prefs, mock_logger) + store = ExportStore(lambda: prefs, mock_logger) def writer(base): for offset in range(20): diff --git a/tests/test_xac10_no_matter_js.py b/tests/test_xac10_no_matter_js.py new file mode 100644 index 0000000..6709f5a --- /dev/null +++ b/tests/test_xac10_no_matter_js.py @@ -0,0 +1,106 @@ +"""XAC10 — no matter.js import exists anywhere in the Python. + +The PRD names this as an acceptance criterion enforced by a test +(`docs/PRD-indigo-matter-export.md` §8, XAC10, guarding XG6). The architecture +it protects: matter.js lives **only** in `bridge-node/`, behind the WebSocket +contract in `docs/BRIDGE_PROTOCOL.md`. The moment a Python module reaches for a +matter.js package directly — via a Node shim, a transpiler bridge, a +`js2py`-style loader — the plugin has two Matter stacks, two versions of the +spec, and a second place for endpoint identity to drift. + +**Why AST and not grep.** A raw text search for "matter.js" fires on every +sentence of prose in this repo that explains the split ("matter.js FanControl is +a stub", and so on), and a search for "matter" hits `matter_client`, +`matter_model`, `matter_handlers`, `matter_protocol` — all of which are OURS. +So the check parses each file and inspects only the module names in real +`import` / `from ... import` statements, against a denylist of the shapes an +npm package would take. That is precise enough to be trusted, which is the +whole point of an acceptance-criterion test: one that cries wolf gets deleted. +""" +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +SERVER_PLUGIN_DIR = ( + Path(__file__).parent.parent + / "indigo-matter.indigoPlugin" / "Contents" / "Server Plugin" +) + +#: Top-level module names that would mean a matter.js package. `matter` itself +#: is here because `from matter.js import ...` and `import matter` both root +#: there; every module WE own is prefixed (`matter_client`, `matter_model`, +#: `matter_handlers`, …) and so never matches. +DENIED_ROOTS = frozenset({ + "matter", "matterjs", "matter_js", "matternode", "matter_node", + "node_matter", "matterbridge", "chip", "chip_tool", +}) + +#: Substrings that can only occur in an npm-style specifier. +DENIED_SUBSTRINGS = ("@matter", "matter.js", "matter-js") + + +def _python_files() -> list[Path]: + return sorted(SERVER_PLUGIN_DIR.rglob("*.py")) + + +def _imported_modules(tree: ast.AST) -> list[str]: + """Every module named by an import statement in ``tree``.""" + modules: list[str] = [] + for node in ast.walk(tree): + if isinstance(node, ast.Import): + modules.extend(alias.name for alias in node.names) + elif isinstance(node, ast.ImportFrom): + # node.module is None for `from . import x` — a relative import, + # which by construction cannot name an npm package. + if node.module: + modules.append(node.module) + return modules + + +def test_there_are_python_files_to_check(): + """A silent zero-file sweep would pass XAC10 forever without checking it.""" + assert len(_python_files()) > 5 + + +@pytest.mark.parametrize("path", _python_files(), ids=lambda p: p.name) +def test_xac10_no_matter_js_import(path): + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for module in _imported_modules(tree): + root = module.split(".")[0] + assert root not in DENIED_ROOTS, ( + f"{path.name} imports {module!r}: matter.js belongs in bridge-node/, " + "behind the BRIDGE_PROTOCOL WebSocket contract (XAC10/XG6)." + ) + lowered = module.lower() + for needle in DENIED_SUBSTRINGS: + assert needle not in lowered, ( + f"{path.name} imports {module!r}, which names an npm matter.js " + "package (XAC10/XG6)." + ) + + +def test_our_own_matter_prefixed_modules_are_not_flagged(): + """The denylist must not be so broad it bans the plugin's own modules. + + `matter_client`, `matter_model` and `matter_handlers` are ours and are + imported all over `plugin.py`; a check that flagged them would be turned + off within a week. + """ + ours = ["matter_client", "matter_model", "matter_handlers.registry", + "matter_protocol", "bridge_client", "export_catalog"] + for module in ours: + assert module.split(".")[0] not in DENIED_ROOTS, module + assert not any(needle in module for needle in DENIED_SUBSTRINGS), module + + +def test_the_denylist_would_actually_catch_a_matter_js_import(tmp_path): + """Pin the check itself: a planted violation must fail.""" + planted = tmp_path / "planted.py" + for source in ("import matter_js", "from matter.js import Endpoint", + "import matter", "from matterbridge import x"): + planted.write_text(source, encoding="utf-8") + with pytest.raises(AssertionError): + test_xac10_no_matter_js_import(planted) From 52f2c482b6968ad09710d84f9848549a661fd9b8 Mon Sep 17 00:00:00 2001 From: Simon Clark Date: Wed, 5 Aug 2026 08:07:46 +0100 Subject: [PATCH 3/3] fix(export): loop-guard devices are absent from the picker (XAC6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit XAC6 requires devices created by this plugin to be ABSENT from the picker, verified by unit test — not merely unpickable. They previously rendered as excluded-with-reason rows like every other exclusion, which was both a criterion violation and pure noise (each one shadows a real device already listed). All other exclusions keep their visible reasons (XAC9). classify() still returns Excluded for our own devices, so the store validator and both callbacks keep rejecting crafted picks — absence is presentation, the guard stays structural. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S --- .../Contents/Server Plugin/plugin.py | 20 ++++++++++++++++--- tests/test_export_menu.py | 15 +++++++++++--- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py index 644b9d2..dd40f3e 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py @@ -1242,8 +1242,16 @@ def _log_row_failure(self, exc, first: bool) -> None: self.logger.error("Matter export: another device could not be read — %s", exc) @staticmethod - def _candidate_row(dev, name: str, plugin_id: str, exported) -> tuple[str, str]: - """One picker row for ``dev``. May raise — the caller contains it.""" + def _candidate_row(dev, name: str, plugin_id: str, exported) -> Optional[tuple[str, str]]: + """One picker row for ``dev``, or None to omit it. May raise — the caller contains it. + + Loop-guard devices (created by this plugin) return None: XAC6 requires + them ABSENT from the picker, not merely unpickable — every one of them + shadows a device the user already sees, so listing them as excluded + would only add noise. Every OTHER exclusion is listed with its reason + (XAC9); hiding those would leave a user hunting for a device that never + appears. + """ device_id = dev.id # An excluded device that IS exported keeps its marker: the pair # "excluded" + "exported" is exactly the state the user has to know @@ -1252,6 +1260,8 @@ def _candidate_row(dev, name: str, plugin_id: str, exported) -> tuple[str, str]: mark = "● " if device_id in exported else "" verdict = export_catalog.classify(dev, plugin_id) if isinstance(verdict, export_catalog.Excluded): + if verdict.reason == export_catalog.REASON_LOOP_GUARD: + return None return (f"{EXCLUDED_OPTION_PREFIX}{device_id}", f"{mark}{name} — not exportable: {verdict.reason}") return (str(device_id), f"{mark}{name}") @@ -1289,7 +1299,11 @@ def getExportCandidates(self, filter="", valuesDict=None, typeId="", targetId=0) if matched > EXPORT_PICKER_LIMIT: truncated += 1 continue - options.append(self._candidate_row(dev, name, plugin_id, exported)) + row = self._candidate_row(dev, name, plugin_id, exported) + if row is None: # loop guard: absent, not excluded (XAC6) + matched -= 1 # our own devices never consume the cap + continue + options.append(row) except Exception as exc: # pylint: disable=broad-except self._log_row_failure(exc, first=not failures) failures += 1 diff --git a/tests/test_export_menu.py b/tests/test_export_menu.py index b1b608a..538437a 100644 --- a/tests/test_export_menu.py +++ b/tests/test_export_menu.py @@ -185,11 +185,20 @@ def test_xac9_excluded_devices_are_listed_with_their_reason(plug): assert export_catalog.REASON_SPRINKLER in labels["x-104"] -def test_xac6_our_own_device_is_shown_as_excluded_not_hidden(plug): - """The loop-guarded device stays visible so the user can see WHY.""" +def test_xac6_our_own_device_is_absent_from_the_picker(plug): + """XAC6: loop-guarded devices are ABSENT, not listed-as-excluded. + + Every device this plugin created shadows a real device the user already + sees in the picker, so a visible "not exportable" row would be pure noise. + All OTHER exclusions stay visible with reasons (XAC9) — absence is reserved + for the loop guard. Defense in depth is unchanged: classify() still returns + Excluded for these ids, so the store validator and the exportDeviceChanged / + exportAddOrUpdate callbacks reject a crafted pick anyway. + """ labels = _labels(plug.getExportCandidates(valuesDict=_values())) assert "105" not in labels # not selectable - assert export_catalog.REASON_LOOP_GUARD in labels["x-105"] + assert "x-105" not in labels # and not even shown + assert not any("105" in option_id for option_id in labels) def test_picker_ids_are_never_empty_strings(plug):