fix(mapping): endpoint hardening — actuator dupes, unknown-device fallback, type-edit guard (#58)#59
Merged
Merged
Conversation
…type guard (#58) Audit of the issue #56 bug class found three gaps between what Indigo permits and what the cluster→device pipeline assumed: - Duplicate actuators: OnOff/LevelControl only deferred within the lighting chain, so a Matter fan (OnOff is mandatory alongside FanControl since 1.2), a thermostat/lock/valve/covering exposing OnOff, or a colour light without LevelControl produced a duplicate relay/dimmer fighting over the same hardware. New ENDPOINT_OWNER_CLUSTERS rule: OnOff and LevelControl defer to any rich endpoint owner; OnOff also defers directly to ColorControl (found by the zoo harness on its first run). - Unknown devices vanished: an endpoint with only unhandled clusters created nothing, and the commission job reported SUCCESS with indigoDeviceIds: [] — Domio showed success, Indigo showed nothing. The matterUnknown placeholder (declared in Devices.xml since day one, never wired) is now created for device-bearing endpoints with supportedClusters populated and an INFO inviting a device-support report. When a later pass finds the endpoint supported (the Tapo firmware-update pattern) the real device is created and the user is told the placeholder can be deleted — never auto-deleted. - Type-edit guard: Indigo's Edit Device dialog always offers the plugin's full Type menu. createdTypeId is stamped at creation (and healed onto legacy devices at reconcile); validateDeviceConfigUi rejects type changes with an explanatory alert; deviceStartComm warns as backstop for edits made while the plugin was off. tests/test_device_zoo.py is a contract harness over cluster combinations: expected types, at most one actuator per endpoint, spec types exist in Devices.xml, initial states declared, sensor specs carry explicit display props (the #56 class). Add wild devices' cluster sets to ZOO and every invariant runs over them. 743 tests. Closes #58 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 23 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR addresses three critical gaps in Matter endpoint mapping: prevents duplicate actuators by deferring lighter handlers when richer ones are present, creates ChangesEndpoint-mapping hardening (issue
Sequence DiagramsequenceDiagram
participant User
participant EditUI as Indigo Edit Device UI
participant ValidateUI as validateDeviceConfigUi()
participant DeviceStartComm as deviceStartComm()
participant DeviceSync as device_sync
participant Logger as Logger
User->>EditUI: Try to change device typeId
EditUI->>ValidateUI: Validate typeId change
ValidateUI->>ValidateUI: Check if createdTypeId stamped
ValidateUI-->>EditUI: ❌ Reject with error message
EditUI-->>User: "Type cannot be changed, delete/reload plugin"
Note over User,Logger: Offline edit scenario (plugin not running)
User->>EditUI: Manually edit device typeId in config
Note over User,Logger: Plugin restart
DeviceStartComm->>DeviceStartComm: Load device, compare createdTypeId vs deviceTypeId
DeviceStartComm->>Logger: ⚠️ Log mismatch warning
DeviceStartComm->>DeviceSync: note_device() to flag for reconcile
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes This PR introduces a coordinated fix across handler ownership deferral, device creation/healing logic, UI validation, and comprehensive test coverage. The changes span multiple interdependent modules and require understanding the Matter endpoint-to-device mapping pipeline, the deferral chain, placeholder semantics, and the role of Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@indigo-matter.indigoPlugin/Contents/Server` Plugin/device_sync.py:
- Around line 663-667: The persistence check currently compares
bool(applied.get(key)) != bool(value), which treats any non-empty string as
equal and can miss wrong non-empty createdTypeId; update the verification to use
value equality for string fields (e.g., compare applied.get(key) == value) when
key == "createdTypeId" (and similarly for other string properties), and handle
missing vs None explicitly (check key in applied or applied.get(key) is None) so
that wrong non-empty persisted values are detected; modify the logic around
variables key, applied, value (and the construction of missing/createdTypeId
where type_id and current_props are used) to use direct equality instead of bool
coercion.
In `@tests/test_device_sync.py`:
- Line 1423: Tests currently unpack indigo_env into _indigo, devices but never
use devices (RUF059); update the unpacking in the test functions to drop the
unused variable by either assigning the second element to _ (e.g., "_indigo, _ =
indigo_env") or by only extracting the first element (e.g., "_indigo =
indigo_env[0]") wherever you see "_indigo, devices = indigo_env" at the reported
locations so that "devices" is no longer created but test behavior remains
unchanged.
🪄 Autofix (Beta)
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
Run ID: eb9c66df-6267-47bd-9455-3ee73c25564e
📒 Files selected for processing (10)
docs/HANDOVER.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/device_sync.pyindigo-matter.indigoPlugin/Contents/Server Plugin/matter_handlers/base.pyindigo-matter.indigoPlugin/Contents/Server Plugin/matter_handlers/level_control.pyindigo-matter.indigoPlugin/Contents/Server Plugin/matter_handlers/on_off.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pytests/test_device_sync.pytests/test_device_zoo.pytests/test_plugin_module.py
Comment on lines
+663
to
+667
| # Heal the type-edit guard's stamp onto devices created | ||
| # before it existed (issue #58). Records the CURRENT type as | ||
| # canonical — for a pristine fleet that is the created type. | ||
| if type_id and "createdTypeId" not in current_props: | ||
| missing["createdTypeId"] = type_id |
There was a problem hiding this comment.
Use value equality when verifying persisted createdTypeId.
At Line 666, createdTypeId (a string) is added to missing, but Lines 695-697 validate persistence with bool(applied.get(key)) != bool(value).
That treats any non-empty string as equivalent, so a wrong non-empty createdTypeId can be falsely reported as “stuck = []”.
Suggested fix
- stuck = [
- key for key, value in missing.items()
- if key not in applied or bool(applied.get(key)) != bool(value)
- ]
+ def _persisted(key: str, expected: Any) -> bool:
+ actual = applied.get(key)
+ if key == "createdTypeId":
+ return actual == expected
+ return bool(actual) == bool(expected)
+
+ stuck = [
+ key for key, value in missing.items()
+ if key not in applied or not _persisted(key, value)
+ ]Also applies to: 694-697
🤖 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/device_sync.py around
lines 663 - 667, The persistence check currently compares bool(applied.get(key))
!= bool(value), which treats any non-empty string as equal and can miss wrong
non-empty createdTypeId; update the verification to use value equality for
string fields (e.g., compare applied.get(key) == value) when key ==
"createdTypeId" (and similarly for other string properties), and handle missing
vs None explicitly (check key in applied or applied.get(key) is None) so that
wrong non-empty persisted values are detected; modify the logic around variables
key, applied, value (and the construction of missing/createdTypeId where type_id
and current_props are used) to use direct equality instead of bool coercion.
|
|
||
|
|
||
| def test_unknown_placeholder_is_idempotent(ds, indigo_env): | ||
| _indigo, devices = indigo_env |
There was a problem hiding this comment.
Remove unused unpacked devices variables in these tests.
Line 1423, Line 1438, and Line 1446 unpack devices but never use it; Ruff flags this as RUF059.
Suggested diff
def test_unknown_placeholder_is_idempotent(ds, indigo_env):
- _indigo, devices = indigo_env
+ _indigo, _devices = indigo_env
ds.create_from_raw(UNKNOWN_NODE, "Vacuum")
first = ds.lookup(0x50, 1)
result = ds.create_from_raw(UNKNOWN_NODE, "Vacuum")
assert result["indigoDeviceIds"] == [first]
def test_merge_only_endpoint_gets_no_placeholder(ds, indigo_env):
- _indigo, devices = indigo_env
+ _indigo, _devices = indigo_env
result = ds.create_from_raw(ELECTRICAL_ONLY_NODE, "Meter")
assert result["indigoDeviceIds"] == []
def test_placeholder_obsolete_when_endpoint_becomes_supported(ds, indigo_env, mock_logger):
@@
- _indigo, devices = indigo_env
+ _indigo, _devices = indigo_envAlso applies to: 1438-1438, 1446-1446
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 1423-1423: Unpacked variable devices is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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_device_sync.py` at line 1423, Tests currently unpack indigo_env
into _indigo, devices but never use devices (RUF059); update the unpacking in
the test functions to drop the unused variable by either assigning the second
element to _ (e.g., "_indigo, _ = indigo_env") or by only extracting the first
element (e.g., "_indigo = indigo_env[0]") wherever you see "_indigo, devices =
indigo_env" at the reported locations so that "devices" is no longer created but
test behavior remains unchanged.
Source: Linters/SAST tools
… on props replace Deployed the branch build to jarvis with the 15-device virtual fleet running: one reconcile healed all pre-fix sensors (verified persistence, no warnings); a second restart showed zero re-asserts and zero stale-display warnings, and the temp sensor's on_state went false→null. The #56 fix is fully self-healing — delete-and-recreate is never needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Live-rig verified on jarvis (see HANDOVER + issue #56 comment): one-pass heal of all 20 pre-fix devices with verified persistence, second reconcile clean — and the open hardware question is answered: Indigo re-derives |
#56) Live experiment on jarvis settled the open display question: for API-created devices a True Supports* prop wins, but with BOTH SupportsOnState and SupportsSensorValue explicitly False, Indigo falls back to the Devices.xml <UiDisplayStateId>. Verified by instrumented reconciles: button displayStateId became lastButtonEvent and air quality became airQuality (the human-readable string) instead of the on/off display they were stuck on. - GenericSwitchHandler and AirQualityHandler set display_props to the both-False pair (the existing reconcile self-heal upgrades fielded devices automatically — no recreation needed, as verified live). - The zoo invariant now allows (False, False) only when the XML declares a UiDisplayStateId pointing at a declared custom state — and it immediately caught that generic_switch never merged display_props into its spec props. - The stale-display nag covers any type that disclaims SupportsOnState (value sensors and UiDisplayStateId-fallback types alike). - FakeDev models the verified precedence rule. 747 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Devices created before the issue #18 address stamping show an empty Address column (spotted by Simon on the original five virtual devices, e.g. 'node-matter Thermostat') — and that address is the node id a user needs for decommission. The reconcile self-heal now writes it when absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 12, 2026
5 tasks
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.
Closes #58. Closes #56 (the display follow-ups are resolved here — see the issue comments for the live verification). Audit of the #56 bug class ("Indigo permits things the cluster→device pipeline never assumed").
1. Duplicate actuators for combined-cluster endpoints
The deferral chain was only OnOff→LevelControl→ColorControl (+ FanControl→Thermostat). Nothing deferred to Thermostat, FanControl, WindowCovering, Valve, or DoorLock — but the Matter Fan device type mandates OnOff alongside FanControl (1.2+), and thermostats/locks in the wild expose OnOff too. Each would have produced its proper device plus a duplicate
matterRelayfighting over the same hardware.Fix:
ENDPOINT_OWNER_CLUSTERS(Valve, DoorLock, WindowCovering, Thermostat, FanControl) inmatter_handlers/base.py; OnOff and LevelControl defer to any of them. The new zoo harness then immediately caught a fourth case on its first run: a colour light without LevelControl duplicated as colour + relay — OnOff now also defers directly to ColorControl.2. Unknown cluster combinations no longer vanish
An endpoint with only unhandled clusters created nothing, and a node of such endpoints commissioned to SUCCESS with
indigoDeviceIds: []— Domio showed success, Indigo showed nothing. ThematterUnknownplaceholder (declared in Devices.xml with asupportedClustersfield since day one, never wired) is now created for device-bearing endpoints ≥1, with the unhandled cluster ids populated and an INFO log inviting a GitHub device-support report. Endpoint 0 and utility/merge-only-cluster endpoints are excluded via_NON_DEVICE_CLUSTERS. When a later pass finds the endpoint supported (the Tapo firmware-update pattern), the real device is created and an INFO says the placeholder can be deleted — never auto-deleted.3. Type-edit guard
Indigo's Edit Device dialog always offers the plugin's full Type menu (platform behaviour; can't be removed). A user flipping a sensor to
matterRelaydesyncs the index → duplicate device next reconcile + a zombie whose actions target clusters the node doesn't implement. Now:createdTypeIdstamped into props at creation and healed onto legacy devices by the reconcile self-heal (one-time props replace per existing device on first reconcile after upgrade);validateDeviceConfigUirejects type changes with an explanatory alert;deviceStartCommwarns as backstop for edits made while the plugin was off.The zoo (
tests/test_device_zoo.py)A contract harness over cluster combinations run through the real registry: expected device types per endpoint, at most one actuator per endpoint, spec types exist in Devices.xml, initial states are XML-declared or built-in, and sensor specs carry explicit display props (the #56 invariant — that check alone would have caught CliveS's bug before he did). Adding a wild device's cluster set to
ZOOruns every invariant over it automatically.Testing
743 tests pass (+109: 96 zoo, 7 fallback/stamp, 6 guard). Not coverable without hardware (noted in HANDOVER): whether
replacePluginPropsOnServerre-derivesdisplayStateIdin real Indigo, and the #56 button/air-quality display follow-ups.Version: 2026.2.21 → 2026.2.22 (patch).
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests