feat(export): E3b — device sync, export handlers, bridge client wiring - #124
Merged
Conversation
Wires the whole outbound pipeline: Indigo device changes → the bridge node, and ecosystem commands → Indigo devices (PRD-indigo-matter-export §5.4). export_handlers.py — the per-ROLE outbound handler table, the mirror of matter_handlers/ (inbound is keyed by cluster; outbound there is no cluster, only a user-declared role). Three methods per role: states_for, diff and dispatch. E3 roles only — handler_for answers None for the E4 roles the §5.1 dialog can already write into the allow-list, because an unknown role fails the WHOLE attach on the node. Hue diffs carry a ±1° tolerance (Matter's 0-254 hue round-trips ±1°); saturation deliberately has none. export_bridge.py — the engine. Owns the BridgeClient and starts one only while the allow-list is non-empty (XG5), stopping it via the deliberate §3.1 replace_all attach when it empties. The endpoint provider re-runs export_catalog.classify on EVERY attach — the store is a past user declaration, not a guard — and skips-with-warning anything deleted, excluded, re-typed or E4-roled. State pushes are fire-and-forget; on_command dispatches indigo.* on the loop thread, the discipline device_sync.apply_states already uses. plugin.py — deviceUpdated/deviceDeleted, the _exports_changed seam, the watchdog branch and shutdown ordering. subscribeToChanges is conditional (it is a server-wide firehose and the default posture is inert) and one-way (no unsubscribe exists in the canonical reference); deviceUpdated's guard is a plain frozenset attribute, so a non-exported device costs one hash lookup. bridge_client.py — attach_timeout_for(n) = max(8.0, 2.0 + 0.15n). The node answers an attach only after its ~100ms-paced removals, so ~80 endpoints spend 8s in pacing alone and the flat deadline timed out on exactly the databases that need export most. Zero protocol change. A role change in the dialog is now remove+re-add (§4.1 refuses it in place) and exportStatus warns that the accessory loses its Home-app name and room. bridgeWsPort stays out of PluginConfig.xml: PRD §5.5's Export section is a whole panel and belongs with E6/E7. Tests: 1784 (was 1633). pylint 9.33 (was 9.30). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
📝 WalkthroughWalkthroughThis change adds outbound E3 export handlers and an ChangesRole-based export handlers
Bridge lifecycle and plugin integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Indigo
participant Plugin
participant ExportBridge
participant BridgeClient
participant BridgeNode
Plugin->>ExportBridge: synchronize persisted exports
ExportBridge->>Indigo: fetch and classify devices
ExportBridge->>BridgeClient: attach endpoint specifications
BridgeClient->>BridgeNode: attach endpoints
Indigo->>Plugin: report device update
Plugin->>ExportBridge: forward device update
ExportBridge->>BridgeClient: push state diff
BridgeNode->>ExportBridge: send device command
ExportBridge->>Indigo: dispatch command
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The attach deadline, the colour handlers, and every path that could stop exporting without saying so. X1 the un-export deadline is sized by the REMOVALS. attach latency is dominated by the node's ~100ms-paced removals (§3.3), not by what we send — and _replace_all_then_stop sends nothing while removing everything, so it was taking the 8s floor. Over ~60 exports that times out mid-reconcile, warns that accessories "may linger", then close() yanks the socket out from under a node that was working. exports_changed captures the size before it empties the store; attach_timeout_for documents the asymmetry. X2 _set_color_temp clamps mireds to [153, 500] before converting, and rounds rather than truncates. The node clamps too, but that is the other process: unclamped, 1 mired writes 1,000,000 K. X3 hue is omitted below saturation 20 (SATURATION_HUE_FLOOR). Indigo recovers hue from three integer 0-100 channels, and the round-trip error is 30° at sat 1 and 180° at sat 0 — far past the ±1° tolerance, so every pastel change pushed a spurious set_state and the Home wheel jumped. The measured error table is in the constant's docstring. X4 colour-mode exclusivity: _set_color zeroes whiteLevel, _set_color_temp zeroes RGB, each only where the device has that channel; a device with no white channel skips setColorTemp entirely rather than inventing 100. Matter colorMode is one-of and the node already picks hue/sat over CT. *** RGBW driver behaviour needs the live E2E — see docs/HANDOVER.md. *** X5 endpoint_specs (N blocking indigo.devices[id] IPC copies) moves to an executor; the loop is shared with the inbound matter-server client. X6 on_command's thread-safety claim is now "unverified from docs; single seam so it can move to an executor" — device_sync's precedent covers state writes on our own devices, not device commands. Silent failures: not-attached drops name the device at debug and warn once per streak with the real reason when halted/in recovery (F1); run-loop and un-export scheduling failures warn and name what was lost (F2); a failed dispatch fires a corrective set_state so the ecosystem snaps back to truth (F5); the per-event catches in plugin.deviceUpdated and export_bridge.device_updated name the device and latch per streak (F6/F7); non-terminal attach refusals report once per streak (F8c); the skip dedupe key no longer contains str(exc) (F8b); the watchdog's halted/recovery branches latch (F9); deviceDeleted refreshes the id cache in a finally (F9a); the dialog says when the bridge is halted, rebuilding, or not connected, with load_error still leading (F10); _un_export closes the socket in a finally so CancelledError cannot leak it (F4). Tests: startup with a non-empty allow-list (T1, verified by deleting the startup _exports_changed() call); the fast-path guard now fails on any store ATTRIBUTE access, not just a call (T2); MIREDS_MAX clamp, the n=41 timeout crossover, setLevel 0, re-enable -> set_reachable(True), E4-role silent skip, attach-failure still closes (T3). 1826 pass, pylint 9.35. Also fixes the pre-existing E0602 (Optional undefined in plugin.py). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
tests/test_export_handlers.py (2)
128-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative-path coverage for
onOffarguments.
TestOnOffcoversTrue,False, and an unknown command. It does not cover a missing or non-booleanvalue. That is the path where_on_offcurrently turns the device off instead of refusing the frame (see the comment onexport_handlers.pylines 222-227).Add the case with the handler fix so the behaviour is pinned:
💚 Proposed test
def test_an_unknown_command_is_refused_not_guessed(self, handlers, mock_indigo_base): dev = RelayDevice(1, "Plug") assert handlers.handler_for("onOffPlugInUnit").dispatch("setLevel", {"level": 5}, dev) \ is False mock_indigo_base.device.turnOn.assert_not_called() + + `@pytest.mark.parametrize`("args", [{}, {"value": None}, {"value": "on"}]) + def test_a_malformed_on_off_is_refused_not_read_as_off(self, handlers, mock_indigo_base, + args): + """A frame we cannot read must not be the thing that switches the load off.""" + with pytest.raises(ValueError): + handlers.handler_for("onOffPlugInUnit").dispatch("onOff", args, RelayDevice(1, "P")) + mock_indigo_base.device.turnOff.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_export_handlers.py` around lines 128 - 144, Add negative-path tests in TestOnOff for onOff requests with a missing value and with a non-boolean value, asserting dispatch returns False and neither turnOn nor turnOff is called. Update the _on_off handler in export_handlers.py to reject these invalid arguments instead of treating them as a request to turn the device off, while preserving the existing True and False behavior.
443-448: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare hue on the circle, not on the line.
abs(back - hue)is not wrap-safe. At hue 359 a one-degree drift to 0 reports an error of 359°, which hides the real cause. The test passes today because the conversion is exact at that boundary.♻️ Proposed change
red, green, blue = handlers.hue_saturation_to_rgb(hue, 100) back, _saturation = handlers.rgb_to_hue_saturation(red, green, blue) - assert abs(back - hue) <= handlers.HUE_TOLERANCE_DEGREES + drift = min((back - hue) % 360, (hue - back) % 360) + assert drift <= handlers.HUE_TOLERANCE_DEGREES🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_export_handlers.py` around lines 443 - 448, Update test_hue_survives_a_round_trip_through_rgb to compare hue values using the shortest circular distance, normalizing the difference across the 0/360 boundary before applying handlers.HUE_TOLERANCE_DEGREES. Preserve the existing parameterized cases and tolerance assertion.indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py (1)
436-445: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
_recreateperforms Indigo IPC on the loop thread.
_spec_forcallsself._device_getter, which is a blockingindigo.devices[id]copy. Inside_recreatethat call runs on the asyncio loop, and this loop is shared with the inbound matter-server client. The module docstring on lines 38-43 sets the rule and nameson_commandas "the single remaining seam";replaceadds a second one that the docstring does not cover.
upsertalready shows the correct shape: it resolves the spec on the calling thread and schedules only the coroutine. The spec does not depend on the removal, so hoist it.♻️ Proposed change
client = self._live_client("the role change", device_id) if client is None: return + entry = self._store.get(device_id) + spec = self._spec_for(entry) if entry is not None else None async def _recreate() -> None: await client.remove_endpoint(device_id) - entry = self._store.get(device_id) - if entry is None: - return - spec = self._spec_for(entry) if spec is not None: await client.upsert_endpoint(spec)This also removes the re-read of the store from the loop thread, which the store's
RLockcurrently has to serialise against the Indigo thread.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py around lines 436 - 445, Update the replace/recreation flow around _recreate so _spec_for and the associated store lookup execute before scheduling the coroutine, on the calling thread. Pass the resolved spec into _recreate, leaving the coroutine responsible only for client.remove_endpoint and conditional client.upsert_endpoint; preserve the existing behavior when no entry or spec exists.indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py (1)
363-379: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the endpoint-provider call.
_gather_endpointsawaits the executor with no deadline. The provider does one synchronousindigo.devices[id]IPC per exported device. If IndigoServer stops answering, the handshake blocks at Line 219 forever:_mark_connectedis never reached, no attach is sent, and the run loop cannot fall through to its reconnect backoff. The client then reports "not connected" without a bounded failure.An
asyncio.wait_foraround the executor call converts that hang into a normal reconnect. Note that the worker thread itself stays blocked, so pick a deadline that is generous relative to the export count.♻️ Proposed bound on the provider read
loop = asyncio.get_running_loop() - return await loop.run_in_executor(None, self._endpoint_provider) + return await asyncio.wait_for( + loop.run_in_executor(None, self._endpoint_provider), + timeout=ENDPOINT_READ_TIMEOUT, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/bridge_client.py around lines 363 - 379, Update _gather_endpoints to wrap the executor-backed self._endpoint_provider call in asyncio.wait_for with a generous timeout appropriate for the number of exported devices. Allow the timeout to propagate as a normal endpoint-gathering failure so the reconnect loop can apply its existing backoff, while preserving the executor offloading and synchronous provider interface.tests/test_export_bridge.py (1)
125-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRuff flags
×(MULTIPLICATION SIGN) in two new prose lines. Both lines were added in this PR and use×where Ruff's ambiguous-character rules expectx. If RUF002 and RUF003 are enforced in CI, both fail.
tests/test_export_bridge.py#L125-L125: replacepacing×countwithpacing x countin the class docstring (RUF002).indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py#L59-L59: replace0.1s × removalswith0.1s x removalsin the comment (RUF003).If these rules are intentionally suppressed for this repository, no change is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_export_bridge.py` at line 125, Replace the ambiguous multiplication sign in both prose sites so Ruff’s ambiguous-character checks pass: update the class docstring in tests/test_export_bridge.py at lines 125-125 from pacing×count to pacing x count, and update the comment in indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py at lines 59-59 from 0.1s × removals to 0.1s x removals; keep the surrounding wording unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 51: Update the CLAUDE.md entry for export_bridge.py to remove the
assertion that on_command dispatches indigo.* from the loop thread as
established discipline. Describe this behavior as an unverified assumption or
explicitly defer to the qualification in export_bridge.py and HANDOVER.md, while
preserving the surrounding lifecycle and attach-provider documentation.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py:
- Around line 156-164: Update the stop method to store client.close() in a
coroutine variable, then separately call _runtime.submit and result so only
submit failures close the coroutine with coro.close(). Preserve the existing
shutdown logging and idempotent behavior, and do not close a coroutine after it
has been successfully submitted.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_handlers.py:
- Around line 222-227: Update _on_off to validate args["value"] as a boolean
before acting, raising ValueError for missing or malformed values so
export_bridge.on_command handles the correction path; replace the bare "value"
literal with the module’s established constant for the §4.2 argument name, or
define and reuse one if none exists.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/plugin.py:
- Around line 1397-1412: Update _export_bridge_note to capture bridge.client
once before checking bridge.active, then return the existing empty note when the
captured client is unavailable. Use that captured client for the halted,
recovery, and attached checks so a client unset between property reads cannot
cause an AttributeError.
In `@tests/test_export_wiring.py`:
- Line 496: Replace the ineffective uninstall assertion in the shutdown test
with assertions for the actual contract: verify shutdown calls stop and does not
invoke any bridge teardown method. Configure the bridge mock with a spec based
on the real ExportBridge so unexpected uninstall calls fail immediately.
---
Nitpick comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/bridge_client.py:
- Around line 363-379: Update _gather_endpoints to wrap the executor-backed
self._endpoint_provider call in asyncio.wait_for with a generous timeout
appropriate for the number of exported devices. Allow the timeout to propagate
as a normal endpoint-gathering failure so the reconnect loop can apply its
existing backoff, while preserving the executor offloading and synchronous
provider interface.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py:
- Around line 436-445: Update the replace/recreation flow around _recreate so
_spec_for and the associated store lookup execute before scheduling the
coroutine, on the calling thread. Pass the resolved spec into _recreate, leaving
the coroutine responsible only for client.remove_endpoint and conditional
client.upsert_endpoint; preserve the existing behavior when no entry or spec
exists.
In `@tests/test_export_bridge.py`:
- Line 125: Replace the ambiguous multiplication sign in both prose sites so
Ruff’s ambiguous-character checks pass: update the class docstring in
tests/test_export_bridge.py at lines 125-125 from pacing×count to pacing x
count, and update the comment in indigo-matter.indigoPlugin/Contents/Server
Plugin/bridge_client.py at lines 59-59 from 0.1s × removals to 0.1s x removals;
keep the surrounding wording unchanged.
In `@tests/test_export_handlers.py`:
- Around line 128-144: Add negative-path tests in TestOnOff for onOff requests
with a missing value and with a non-boolean value, asserting dispatch returns
False and neither turnOn nor turnOff is called. Update the _on_off handler in
export_handlers.py to reject these invalid arguments instead of treating them as
a request to turn the device off, while preserving the existing True and False
behavior.
- Around line 443-448: Update test_hue_survives_a_round_trip_through_rgb to
compare hue values using the shortest circular distance, normalizing the
difference across the 0/360 boundary before applying
handlers.HUE_TOLERANCE_DEGREES. Preserve the existing parameterized cases and
tolerance assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14c90224-6e86-495a-bf23-cdc9ee67679c
📒 Files selected for processing (16)
CLAUDE.mddocs/HANDOVER.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.pyindigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.pyindigo-matter.indigoPlugin/Contents/Server Plugin/export_handlers.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pypyproject.tomltests/conftest.pytests/fakes.pytests/test_export_bridge.pytests/test_export_handlers.pytests/test_export_menu.pytests/test_export_wiring.pytests/test_plugin_behaviour.pytests/test_ws_json_client.py
| | `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 | | ||
| | `export_handlers.py` | The **outbound** handler table, keyed by §4.2 **role** (the inbound registry is keyed by cluster; outbound there is no cluster, only a user-declared role) — `states_for` / `diff` / `dispatch` per role. E3 roles only: plug, on/off light, dimmable, colour-temp, extended colour. `handler_for` returns `None` for the E4 roles the §5.1 dialog can already put in the allow-list. Hue diffs carry a ±1° tolerance (Matter's 0–254 hue round-trips ±1°); saturation deliberately has none | | ||
| | `export_bridge.py` | The outbound engine: owns the `BridgeClient` and everything the Indigo callbacks *mean* for it. The client exists **only** while the allow-list is non-empty (XG5) and starts/stops on the dialog's empty↔non-empty transitions. The attach endpoint provider **re-runs `export_catalog.classify` on every attach** — the store is a past user declaration, not a guard — and skips-with-warning anything deleted/excluded/re-typed or carrying an E4 role (an unknown role fails the *whole* attach). State pushes are fire-and-forget onto the loop; `on_command` dispatches `indigo.*` from the loop thread, the same discipline `device_sync.apply_states` already uses | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line 51 contradicts the code and HANDOVER.md on the loop-thread claim.
The entry ends with "on_command dispatches indigo.* from the loop thread, the same discipline device_sync.apply_states already uses". The two other places that describe this say the opposite:
export_bridge.pylines 453-459: issuingindigo.*device commands from a non-Indigo thread is "unverified from the docs", and theapply_statesprecedent "covers state writes on our own devices, which is not the same claim".HANDOVER.mdline 508: "Not 'existing house discipline'".
CLAUDE.md is the architecture map a contributor reads first. Presenting an acknowledged unverified assumption as settled discipline is how the hedge gets dropped in the next module.
📝 Proposed wording
-State pushes are fire-and-forget onto the loop; `on_command` dispatches `indigo.*` from the loop thread, the same discipline `device_sync.apply_states` already uses |
+State pushes are fire-and-forget onto the loop; `on_command` dispatches `indigo.*` from the loop thread, which is **unverified from the docs** (`device_sync.apply_states` only sets a precedent for state *writes* on our own devices). It is kept as a single seam so it can move to `run_in_executor` if the loop is seen stalling |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `export_bridge.py` | The outbound engine: owns the `BridgeClient` and everything the Indigo callbacks *mean* for it. The client exists **only** while the allow-list is non-empty (XG5) and starts/stops on the dialog's empty↔non-empty transitions. The attach endpoint provider **re-runs `export_catalog.classify` on every attach** — the store is a past user declaration, not a guard — and skips-with-warning anything deleted/excluded/re-typed or carrying an E4 role (an unknown role fails the *whole* attach). State pushes are fire-and-forget onto the loop; `on_command` dispatches `indigo.*` from the loop thread, the same discipline `device_sync.apply_states` already uses | | |
| | `export_bridge.py` | The outbound engine: owns the `BridgeClient` and everything the Indigo callbacks *mean* for it. The client exists **only** while the allow-list is non-empty (XG5) and starts/stops on the dialog's empty↔non-empty transitions. The attach endpoint provider **re-runs `export_catalog.classify` on every attach** — the store is a past user declaration, not a guard — and skips-with-warning anything deleted/excluded/re-typed or carrying an E4 role (an unknown role fails the *whole* attach). State pushes are fire-and-forget onto the loop; `on_command` dispatches `indigo.*` from the loop thread, which is **unverified from the docs** (`device_sync.apply_states` only sets a precedent for state *writes* on our own devices). It is kept as a single seam so it can move to `run_in_executor` if the loop is seen stalling | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 51, Update the CLAUDE.md entry for export_bridge.py to
remove the assertion that on_command dispatches indigo.* from the loop thread as
established discipline. Describe this behavior as an unverified assumption or
explicitly defer to the qualification in export_bridge.py and HANDOVER.md, while
preserving the surrounding lifecycle and attach-provider documentation.
Comment on lines
+156
to
+164
| def stop(self, timeout: float = 4.0) -> None: | ||
| """Close the client. Idempotent; never raises at shutdown.""" | ||
| client, self.client = self.client, None | ||
| if client is None: | ||
| return | ||
| try: | ||
| self._runtime.submit(client.close()).result(timeout=timeout) | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| self._logger.debug("bridge client close error: %s", exc) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the coroutine when submit fails in stop.
client.close() builds a coroutine object before self._runtime.submit receives it. If submit raises — the likely case at shutdown, when the runtime is already stopping — the coroutine is never awaited. Python then emits a RuntimeWarning: coroutine 'close' was never awaited, and the socket is left to the garbage collector.
_fire already handles this exact case with coro.close() on line 641. Apply the same treatment here.
🐛 Proposed fix
def stop(self, timeout: float = 4.0) -> None:
"""Close the client. Idempotent; never raises at shutdown."""
client, self.client = self.client, None
if client is None:
return
+ coro = client.close()
try:
- self._runtime.submit(client.close()).result(timeout=timeout)
+ self._runtime.submit(coro).result(timeout=timeout)
except Exception as exc: # pylint: disable=broad-except
+ coro.close()
self._logger.debug("bridge client close error: %s", exc)Note that coro.close() on an already-scheduled coroutine is unsafe, so this fix is only correct while the sole failure mode is submit itself raising. If result(timeout=...) can also raise here, split the two calls so only the submit failure closes the coroutine.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 163-163: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py around
lines 156 - 164, Update the stop method to store client.close() in a coroutine
variable, then separately call _runtime.submit and result so only submit
failures close the coroutine with coro.close(). Preserve the existing shutdown
logging and idempotent behavior, and do not close a coroutine after it has been
successfully submitted.
Comment on lines
+222
to
+227
| @staticmethod | ||
| def _on_off(args: dict, dev: Any) -> None: | ||
| if bool(args.get("value")): | ||
| indigo.device.turnOn(dev) | ||
| else: | ||
| indigo.device.turnOff(dev) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_on_off turns the device off when value is missing or malformed.
bool(args.get("value")) maps an absent key, None, and a non-boolean to "off". Every other command in this module rejects a bad argument: _set_level, _set_color_temp and _set_color all raise ValueError, which export_bridge.on_command catches, logs against the device, and answers with a corrective set_state. A malformed onOff frame instead switches the load off silently.
Also, "value" is the only §4.2 argument name in this module spelled as a bare literal. _set_level uses STATE_LEVEL and _set_color uses STATE_HUE/STATE_SATURATION, so those are pinned by the zoo test. A typo in "value" would be an every-command-turns-off bug that no test catches.
🐛 Proposed fix: validate the argument and pin its name
COMMAND_ON_OFF = "onOff"
+#: The §4.2 argument name carried by ``onOff``.
+ARG_VALUE = "value" `@staticmethod`
def _on_off(args: dict, dev: Any) -> None:
- if bool(args.get("value")):
+ value = args.get(ARG_VALUE)
+ if not isinstance(value, bool):
+ raise ValueError(f"onOff without a boolean value: {args!r}")
+ if value:
indigo.device.turnOn(dev)
else:
indigo.device.turnOff(dev)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_handlers.py around
lines 222 - 227, Update _on_off to validate args["value"] as a boolean before
acting, raising ValueError for missing or malformed values so
export_bridge.on_command handles the correction path; replace the bare "value"
literal with the module’s established constant for the §4.2 argument name, or
define and reuse one if none exists.
Comment on lines
+1397
to
+1412
| bridge = self.export_bridge | ||
| if bridge is None or not bridge.active: | ||
| # No client is the CORRECT state for an empty allow-list (XG5), and | ||
| # the count above already says the list is not empty — so this is a | ||
| # plugin still starting, which its own log line covers. | ||
| return "" | ||
| client = bridge.client | ||
| if client.halted: | ||
| return (f" Bridge client halted ({client.halted_reason or 'no reason recorded'}) " | ||
| "— restart the bridge node.") | ||
| if client.recovery: | ||
| return (" The bridge node is waiting for an endpoint-map rebuild — exports are not " | ||
| "live until it is done.") | ||
| if not client.attached: | ||
| return " Not connected to the bridge node — exports are not live." | ||
| return "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate every write to ExportBridge.client and every caller of stop()/exports_changed().
set -euo pipefail
fd -e py . | xargs rg -n -C 6 'self\.client\s*=|def stop\(|def exports_changed\(|\.exports_changed\(\)|\.stop\('Repository: simons-plugins/indigo-matter
Length of output: 16797
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
git ls-files | fd -e py . | sed -n '1,80p'
printf '\n--- plugin files with client references ---\n'
rg -n -C 7 'self\.client\s*=|def stop\(|def exports_changed\(|\.exports_changed\(\)|\.stop\(|_export_summary|get_menu_action_config_ui_values|export_bridge' --glob '*.py' .Repository: simons-plugins/indigo-matter
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- export_bridge.py relevant methods ---'
sed -n '120,230p' 'indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py'| cat -n
printf '%s\n' '--- plugin.py _export_summary and dialog config ---'
sed -n '1360,1430p' 'indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py'| cat -n
printf '%s\n' '--- exports changed callers ---'
rg -n -C 4 'exports_changed\(' --glob '*.py'Repository: simons-plugins/indigo-matter
Length of output: 10743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'exports_changed|\.stop\(' --glob '*.py' .
printf '%s\n' '--- plugin methods around export dialogs ---'
rg -n -C 7 'MENU_MANAGE_EXPORTS|get_menu|exportStatus|manageMatterExports|dialog' 'indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py'Repository: simons-plugins/indigo-matter
Length of output: 29918
Capture bridge.client before checking active.
active and client both read self.client. If an un-export unsets the client after active returns true, _export_bridge_note() reads None and raises AttributeError. Capture client = bridge.client once before the active check and handle None.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/plugin.py around lines
1397 - 1412, Update _export_bridge_note to capture bridge.client once before
checking bridge.active, then return the existing empty note when the captured
client is unavailable. Use that captured client for the halted, recovery, and
attached checks so a client unset between property reads cannot cause an
AttributeError.
| plug._stopping = False | ||
| plug.shutdown() | ||
| bridge.stop.assert_called_once() | ||
| assert not hasattr(bridge, "uninstall") or not bridge.uninstall.called |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This assertion can never fail.
bridge is a Mock, so hasattr(bridge, "uninstall") is always True and bridge.uninstall auto-creates a child mock whose .called is always False. Both operands of the or are therefore satisfied on every run. The test states it protects PM-B — "a plugin reload must not un-pair anyone's ecosystems" — but it asserts nothing about that.
Assert on the real contract instead: shutdown must call stop and must not call any teardown method on the bridge. Mock(spec=...) against the actual ExportBridge would also make an accidental uninstall call raise.
💚 Proposed fix
plug.shutdown()
bridge.stop.assert_called_once()
- assert not hasattr(bridge, "uninstall") or not bridge.uninstall.called
+ # `stop` is the ONLY thing shutdown may do to the bridge — anything that
+ # tears the agent down would un-pair every ecosystem (PM-B).
+ assert [name for name, *_ in bridge.method_calls] == ["stop"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert not hasattr(bridge, "uninstall") or not bridge.uninstall.called | |
| plug.shutdown() | |
| bridge.stop.assert_called_once() | |
| # `stop` is the ONLY thing shutdown may do to the bridge — anything that | |
| # tears the agent down would un-pair every ecosystem (PM-B). | |
| assert [name for name, *_ in bridge.method_calls] == ["stop"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_export_wiring.py` at line 496, Replace the ineffective uninstall
assertion in the shutdown test with assertions for the actual contract: verify
shutdown calls stop and does not invoke any bridge teardown method. Configure
the bridge mock with a spec based on the real ExportBridge so unexpected
uninstall calls fail immediately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second half of milestone E3 — the full outbound pipeline: Indigo device changes → bridge node → ecosystems, and ecosystem commands →
indigo.device.*.export_handlers.py— per-role table (states_for / diff / dispatch) for the five E3 roles; real Indigo API only (indigo.dimmer.setColorLevelswith KelvinwhiteTemperatureriding the current white level — there is nosetColorTemp); hue ±1° tolerance in diffexport_bridge.py— owns the BridgeClient; endpoint provider re-classifies every store entry at build (the store is not the guard); E4-roled entries skip-with-warning; fire-and-forget pushes; on_command dispatch; attach-refused/halt surfacing; watchdog branchsubscribeToChangesonly on first export (it's server-wide and a one-way door — no unsubscribe exists);deviceUpdatedfast path = one attribute load + one frozenset hash for non-exported devices (asserted by a test that makes classify/handler_for raise if touched); rename→label, disable→reachable, delete→store+remove; empty↔non-empty client lifecycle from the dialogmax(8.0, 2.0 + 0.15 × endpoints): the zero-protocol-change answer to E3a's pacing×count interactionTests
+151 (suite 1784 green). pylint 9.33 (baseline 9.30). XAC10 green. PluginVersion 2026.7.28.
Live E2E
The report's jarvis runbook follows after review: fresh bundle install (jarvis is generations behind), hand-started bridge node, pick a plug in Manage Matter Exports, pair Apple Home, verify XAC4/XAC7/XAC8 + reconnect-without-duplicates.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
Summary by CodeRabbit
New Features
Bug Fixes