feat(export): E5 — endpoint-map drift detection, refuse-to-start, replace-all persistence - #126
Conversation
…lace-all persistence Endpoint identity must survive everything, and its loss must be loud (XAC5, PRD §4.3/§7). Bridge node: - endpoint-map.json, a persisted UniqueID → endpoint-number map kept OUTSIDE matter.js's storage context so §3.10's factory reset cannot wipe the witness of the very event it exists to survive. Written atomically, checked on every reconcile/upsert, report-only: drift populates StatusReport.drift, emits §5 drift_detected, and never moves the baseline. - driftChecked is now real — true once the detector has run against a persisted map, false before. - Refuse-to-start (§1.1 endpoint_map_invalid): fabrics with no usable map, a commissioning witness with no fabrics (PRD §7 "storage missing but previously commissioned"), or an unusable identity.json. The Matter stack still starts so get_pairing answers; attach is refused, so nothing is ever created. rebuild_endpoint_map (§3.11) is the way out. - identity.json gains commissionedAt, stamped on the first fabric observed. - §3.9 remove_fabric via FabricManager (the cluster command asserts a remote actor), §3.10 factory_reset via ServerNode.erase() with both flavours. - main.ts exits 0 after an ordered shutdown: erase() leaves a ref'd timer that close() does not clear, so a clean stop after a reset exited 1. - All five remaining pending golden frames graduated; pending is now empty and both suites assert it. Plugin: - device_updated diffs against the last state PUSHED, not against orig_dev — a ±1° hue tolerance against the previous Indigo reading let a 1°/step ramp push nothing at all, forever. - pendingReplaceAll persisted in prefs (XAC7): an un-export whose attach never landed reconnects on its own and is discharged by the next successful one. - §5 commands dispatch through a single-worker executor, off the shared loop. - start() is gated while an un-export is in flight. - subscribeToChanges gets a bounded re-issue watchdog. - Fabric backup covers the bridge-node storage dir; restore reports and skips it until E7 provides a stop seam. - _set_color_temp no longer turns a whiteLevel of 0 into 100. PluginVersion 2026.7.30, bridge-node 0.4.0. 1978 Python / 291 TS tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
📝 WalkthroughWalkthroughThis PR adds persisted endpoint maps, drift detection, refusal and recovery flows in ChangesBridge-node endpoint-map recovery
Plugin export and backup hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Process as bridge-node main
participant Storage as identity and map storage
participant Node as BridgeNode
participant Client as WebSocket client
Process->>Storage: validate identity and load endpoint map
Process->>Node: construct with refusal state
Client->>Node: attach or recovery command
Node->>Storage: persist or rebuild endpoint baseline
Storage-->>Node: status and warnings
Node-->>Client: status, drift, or refusal response
sequenceDiagram
participant Plugin
participant ExportBridge
participant BridgeClient
participant BridgeNode
Plugin->>ExportBridge: start or process export change
ExportBridge->>BridgeClient: provide replacement debt
BridgeClient->>BridgeNode: attach with replacement intent
BridgeNode-->>BridgeClient: status report
ExportBridge->>ExportBridge: compare against pushed snapshot
ExportBridge->>BridgeNode: dispatch serialized command
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (13)
tests/test_bridge_client.py (1)
1083-1102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the fail-closed warning.
_replace_alllogs a warning when the provider raises. That log line is the only signal an operator gets that the pending un-export flag was unreadable. Asserting it locks in the diagnostic, not just the safe outcome.♻️ Proposed addition
assert bridge_protocol.ARG_INTENT not in sent(fake, bridge_protocol.CMD_ATTACH)["args"] + assert any("pending un-export flag" in str(call.args[0]) + for call in mock_logger.warning.call_args_list) await client.close()🤖 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_bridge_client.py` around lines 1083 - 1102, Update test_a_raising_provider_attaches_without_the_intent to assert that mock_logger records the warning emitted by _replace_all when replace_all_provider raises, while preserving the existing assertion that the attach command omits bridge_protocol.ARG_INTENT.tests/test_export_wiring.py (1)
547-572: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the tick counts from the plugin constants.
The literals
4and3duplicateRESUBSCRIBE_TICKSandMAX_RESUBSCRIBE_ATTEMPTSinplugin.py. If either constant changes, these tests fail for a reason unrelated to the behaviour they cover. Referencing the constants keeps the intent explicit.♻️ Proposed tweak
def test_it_re_issues_after_a_minute_with_no_device_updates_at_all( - self, plug, mock_indigo_base): + self, plug, mock_indigo_base, plugin_mod): subscribe = self._exporting(plug, mock_indigo_base) - for _ in range(4): + for _ in range(plugin_mod.RESUBSCRIBE_TICKS): plug._resubscribe_tick() subscribe.assert_called_once_with()Apply the same substitution in
test_it_gives_up_rather_than_nagging_forever, usingplugin_mod.MAX_RESUBSCRIBE_ATTEMPTSfor the expected call count.🤖 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` around lines 547 - 572, Update the resubscription tests around _resubscribe_tick to derive retry counts from plugin_mod.RESUBSCRIBE_TICKS instead of the literal 4, and assert the final call count with plugin_mod.MAX_RESUBSCRIBE_ATTEMPTS instead of 3. Preserve the existing test behavior and loop structure.tests/fakes.py (1)
177-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a join timeout so a stuck nested coroutine fails instead of wedging the suite.
thread.join()waits without a bound. If a future nested coroutine ever awaits something only the outer loop can complete, the test run hangs with no output rather than failing. A timeout plus an explicit error keeps the failure diagnosable.♻️ Proposed tweak
thread = threading.Thread(target=_worker, name="fake-runtime-nested") thread.start() - thread.join() + thread.join(timeout=10) + if thread.is_alive(): + raise RuntimeError("nested coroutine did not finish; it is awaiting the outer loop") if "error" in outcome: raise outcome["error"] return outcome["value"]🤖 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/fakes.py` around lines 177 - 182, Update the nested coroutine execution flow around the thread.join call to use a bounded timeout, then detect whether the thread is still alive and raise a clear diagnostic error if it failed to finish. Preserve the existing outcome error propagation and value return for threads that complete normally.bridge-node/test/protocol.test.ts (1)
909-921: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
as DriftEntry[]overas neverat line 915.
as neveris assignable to every parameter type, so it removes the type check on theemitDriftargument.GoldenFramestypesdrift_detected.data.driftasunknown[], so a cast is needed, butas DriftEntry[]keeps the shape contract and still compiles.🤖 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 `@bridge-node/test/protocol.test.ts` around lines 909 - 921, Update the emitDrift call in the “drift_detected event (§5)” test to cast golden.drift_detected.data.drift as DriftEntry[] instead of never, preserving compile-time validation of the drift payload shape.bridge-node/test/fixture-shapes.ts (1)
222-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
satisfiesclause to the two empty-result fixtures.Every other fixture in this file carries one, including
rebuiltStatusandendpointMapInvalidbelow. Without it, these two constants assert no shape. If a future §3.9 or §3.10 result gains a field, the fixture stays silently empty.♻️ Proposed change
-export const removeFabricResult = {}; -export const factoryResetResult = {}; +export const removeFabricResult = {} satisfies Record<string, never>; +export const factoryResetResult = {} satisfies Record<string, never>;🤖 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 `@bridge-node/test/fixture-shapes.ts` around lines 222 - 224, Add explicit `satisfies` clauses to the `removeFabricResult` and `factoryResetResult` empty fixtures, using the response shape types expected for §3.9 and §3.10. Match the established fixture typing pattern used by `rebuiltStatus` and `endpointMapInvalid` so future shape changes are validated.bridge-node/src/endpoint-map.ts (1)
70-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn
RefuseReasonValueinstead ofstring.
protocol.tsline 174 definesRefuseReasonValuefor exactly this purpose. The widerstringreturn type lets any caller pass an arbitrary reason throughendpointMapInvalidDetails, so the union adds no compile-time protection today.♻️ Proposed narrowing
-import { describeError, type DriftEntry, RefuseReason } from "./protocol.js"; +import { describeError, type DriftEntry, RefuseReason, type RefuseReasonValue } from "./protocol.js";-export function refuseReasonFor(state: EndpointIdentityState): string | undefined { +export function refuseReasonFor(state: EndpointIdentityState): RefuseReasonValue | undefined {🤖 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 `@bridge-node/src/endpoint-map.ts` around lines 70 - 78, Update refuseReasonFor to return RefuseReasonValue | undefined instead of string | undefined, importing or referencing the existing RefuseReasonValue type from protocol.ts. Preserve the current branching and returned RefuseReason values so endpointMapInvalidDetails receives the narrowed union and rejects arbitrary reason strings at compile time.bridge-node/package.json (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why
--test-force-exitis present.
--test-force-exitexits after tests finish before the event loop drains and can hide unclosed WebSocket/server handles. Add a short comment explaining the failure mode that required it, or close the leaking handle if a test is keeping a server/socket open.🤖 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 `@bridge-node/package.json` at line 18, Add a brief explanation near the test script documenting which unclosed WebSocket or server handle necessitates --test-force-exit and the failure mode it prevents; alternatively, identify and close that leaking handle so the flag can be removed while preserving reliable test completion.bridge-node/src/main.ts (1)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe phase label duplicates the wrapper's own prefix.
phaselogsStartup failed — ${name}, so this reads "Startup failed — identity check failed" on a throw. The other call sites pass a noun phrase, for example"mDNS interface check". Use"identity check"and"identity load"for consistency.♻️ Proposed wording fix
- const identityFault = await phase("identity check failed", () => identityProblem(config.storagePath)); + const identityFault = await phase("identity check", () => identityProblem(config.storagePath));- const identity = await phase("identity load failed", () => loadOrCreateIdentity(config.storagePath, log)); + const identity = await phase("identity load", () => loadOrCreateIdentity(config.storagePath, log));🤖 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 `@bridge-node/src/main.ts` at line 70, Update the phase label in the identity check call around identityProblem to use the noun phrase "identity check" instead of "identity check failed", and use "identity load" for the corresponding identity load phase call. Keep the phase wrapper responsible for adding failure wording.bridge-node/src/node.ts (1)
529-534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the live-endpoint projection shared with
checkDrift.Lines 530-533 repeat lines 349-352 exactly. Both build the same
LiveEndpointNumber[]from#registry?.summaries(). A change to the projection — a new field onLiveEndpointNumber, or a filter for endpoints that have no number yet — has to be made in both places, and a rebuild that disagrees with the detector writes a baseline the detector then reports as drift.♻️ Proposed refactor
+ /** The live `UniqueID → endpoint number` set, as both §4.3 and §3.11 see it. */ + private liveEndpointNumbers(): LiveEndpointNumber[] { + return (this.#registry?.summaries() ?? []).map(summary => ({ + uniqueId: uniqueIdFor(summary.indigoDeviceId), + endpointNumber: summary.endpointNumber, + })); + } + async rebuildEndpointMap(): Promise<StatusReport> { - const live: LiveEndpointNumber[] = (this.#registry?.summaries() ?? []).map(summary => ({ - uniqueId: uniqueIdFor(summary.indigoDeviceId), - endpointNumber: summary.endpointNumber, - })); + const live = this.liveEndpointNumbers(); this.#endpointMap.rebuild(live);Apply the same substitution in
checkDrift.🤖 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 `@bridge-node/src/node.ts` around lines 529 - 534, Extract the duplicated `LiveEndpointNumber[]` projection from `rebuildEndpointMap` and `checkDrift` into a shared helper, then use that helper in both methods before rebuilding or comparing the endpoint map. Preserve the existing `#registry?.summaries()` fallback and projection behavior exactly.bridge-node/src/storage.ts (2)
207-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe documented precondition is not enforced by the code.
The comment states that the replace branch is safe only because
main.tscallsidentityProblemfirst.loadOrCreateIdentityis exported, and the guard lives in another module. A second caller that omits the check re-mintsinstallId, which changesserialNumberanduniqueIdand un-pairs every ecosystem — the exact loss this milestone prevents.Consider making the precondition structural rather than documentary. One option is a required option, for example
loadOrCreateIdentity(storagePath, log, { replaceUnusable: true }), so a caller has to state that it accepts the loss. Another is to return the problem from a single combined function and let the caller branch, which also removes the duplicated parse inidentityProblem.🤖 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 `@bridge-node/src/storage.ts` around lines 207 - 214, Make the replacement behavior in loadOrCreateIdentity structurally opt-in instead of relying on main.ts calling identityProblem first. Add a required option or equivalent explicit authorization that callers must provide before replacing an unreadable or invalid identity file, and ensure callers without that authorization preserve the existing identity-problem handling rather than re-minting values.
103-125: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
writeJsonAtomicdurable across power loss.
writeFileSyncplusrenameSyncdoes not guarantee the temp file contents are on stable storage before the rename completes; a crash can leaveendpoint-map.jsonempty, missing, or still the old corrupt file. Flush the temp file beforerenameSync, and flush the parent directory afterward before claiming durability. Platform support for directoryfsyncSyncis implementation-defined, so the directory flush must handle unsupported cases such as Windows or network filesystems.🤖 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 `@bridge-node/src/storage.ts` around lines 103 - 125, Update writeJsonAtomic to flush the temporary file’s contents with fsyncSync before renameSync, then open and fsyncSync the parent directory after the rename to persist the directory entry. Handle unsupported directory fsync cases, including Windows or network filesystems, without failing the successful atomic write; preserve cleanup and error propagation for write, flush, or rename failures.bridge-node/test/persistence.test.ts (1)
158-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA mid-test failure leaks a live
ServerNodeand can cascade into later tests.Only the test at line 317 wraps its session in
try/finally. Every other test callsclose()on the success path only. The file header explains that matter.js takes an exclusive lock per storage path and that the restart tests rebuild aServerNodeon the same path. So a failed assertion betweenbootandcloseleaves the node holding that lock and theafterhook then removes the directory underneath it. The first genuine failure in this file is likely to be followed by unrelated failures that hide it.Apply the same
try/finallytreatment used at line 320, or giveboota helper that owns the lifetime.💚 Proposed refactor
+/** Run `body` against a booted session and always close it. */ +async function withSession<T>( + storagePath: string, + body: (session: Session) => Promise<T>, + commissionedAt?: string, +): Promise<T> { + const session = await boot(storagePath, commissionedAt); + try { + return await body(session); + } finally { + await session.close(); + } +}Several tests read state after
close()—readMapat line 205, for example — so each conversion needs the assertion order checked rather than a mechanical wrap.🤖 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 `@bridge-node/test/persistence.test.ts` around lines 158 - 183, Ensure every test that creates a ServerNode through boot, including the restart test, closes it in a finally block so assertion failures cannot leak live nodes or storage locks. Apply the existing try/finally pattern used by the test around line 320, preserving operations that intentionally read state after close by placing only the appropriate assertions before or after cleanup.bridge-node/test/endpoint-map.test.ts (1)
165-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion can pass without proving anything.
Line 178 filters the log for the prefix
"Endpoint map recorded". That prefix is produced bypersistasEndpoint map ${why}, andwhyis chosen insideEndpointMapStore.check. The test does not control that string. If the wording changes to anything else, the filter becomes empty for every input and the test passes whether or not the file was rewritten — including for the regression it exists to catch.Assert on the observable effect instead of the log wording.
💚 Proposed fix
- quiet.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); - - // `get_status`/attach run this on every reconnect; a steady state that - // wrote to disk each time would be a needless write per watchdog cycle. - assert.deepEqual(logged.filter(line => line.startsWith("Endpoint map recorded")), []); + const before = statSync(join(dir, ENDPOINT_MAP_FILE)).mtimeMs; + + quiet.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + + // `get_status`/attach run this on every reconnect; a steady state that + // wrote to disk each time would be a needless write per watchdog cycle. + assert.equal(statSync(join(dir, ENDPOINT_MAP_FILE)).mtimeMs, before, "no write in a steady state"); + assert.deepEqual(logged, [], "and nothing to say about it");
mtimeMsresolution can be coarse on some filesystems. If that proves flaky, assertloggedis empty and count writes through an injected writer instead.🤖 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 `@bridge-node/test/endpoint-map.test.ts` around lines 165 - 179, Update the “does not rewrite the file when nothing was added” test to verify the observable no-write behavior rather than filtering for the “Endpoint map recorded” log prefix. Capture the loaded map file’s modification state or, preferably, inject/count the writer used by EndpointMapStore.persist and assert that quiet.check performs no write; use logged only if needed as a secondary check and avoid relying on message wording.
🤖 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 `@bridge-node/src/main.ts`:
- Around line 131-141: Update the clean shutdown path after the “Shutdown
complete” log to flush stdout before terminating: await one stdout drain step if
needed, or allow the event loop to finish naturally after clearing the escape
hatch. Preserve process.exit(0) for paths where shutdown does not complete.
In `@bridge-node/src/node.ts`:
- Around line 457-469: Update removeFabric to clear the commissioning witness
after fabric.leave() when no fabrics remain, matching the witness cleanup
performed by factoryReset. Preserve the existing behavior for remaining fabrics
and ensure the cleanup occurs only after the last fabric is removed.
- Around line 235-257: Update assertEndpointIdentity so
noteCommissioningWitness() runs whenever the server is commissioned and the
endpoint registry/map state is available, before evaluating the refusal reason.
Keep refusal decision and fabricStorageLost logging unchanged, and allow
independent witness-storage failures to remain non-fatal.
In `@bridge-node/src/storage.ts`:
- Around line 207-214: Make loadOrCreateIdentity in
bridge-node/src/storage.ts:207-214 require an explicit replacement decision or
return the validation problem, eliminating duplicated identityProblem parsing
and preventing silent replacement by default. In bridge-node/src/main.ts:70-83,
when identityFault is present, rename the existing identity.json to an
identity.json.unusable-<timestamp> backup before loadOrCreateIdentity mints a
replacement, and log the backup path so recoverable installId data is preserved.
In `@bridge-node/src/ws-server.ts`:
- Around line 247-272: Update the refusal-state recovery handling in the
WebSocket command flow to require protocolVersion === PROTOCOL_VERSION for
recovery frames. Preserve pre-attach access for rebuild_endpoint_map, while
allowing attach to perform its existing version validation and rejecting other
recovery commands when the version is missing or mismatched.
In `@bridge-node/test/endpoint-map.test.ts`:
- Around line 266-273: Update the test case for “serves an uncommissioned bridge
even with an unreadable map” to pass mapPresent: true while retaining
commissioned: false and mapProblem: "corrupt", so refuseReasonFor exercises the
unreadable-map branch and still returns undefined.
- Around line 181-199: Update the write-failure test around
EndpointMapStore.check to avoid relying on chmod-based permission denial, which
is bypassed when running as root. Prefer adding or using an injectable
map-writer dependency in EndpointMapStore so the test can deterministically
simulate a write failure while preserving the expected empty drift result and
warning assertion.
In `@bridge-node/test/persistence.test.ts`:
- Around line 290-313: Update the test named “clears the commissioning
witness...” so factory_reset is the operation being tested: remove the preceding
rebuild_endpoint_map request, since factory_reset is admitted while the node
refuses. Preserve the initial refusal assertion, then verify after factory_reset
that identity.json no longer contains commissionedAt and that restart has no
refusal.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/bridge_client.py:
- Around line 229-233: Update the recovery flow in start() to retain the pending
un-export count from PREF_PENDING_REPLACE_ALL rather than only the boolean from
_owes_replace_all(). Carry that count through the provider into _replace_all(),
and when the empty-handshake attach enables replace_all, pass an explicit
deadline sized for the pending removals to _attach(). Preserve the existing
timeout behavior for non-replace-all attaches.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py:
- Around line 341-348: Update the `_un_exporting` handling in the coroutine
scheduling flow around `_fire` so a failed schedule cannot leave the flag set
permanently; either assign it only after scheduling succeeds or clear it when
`_fire` reports failure. Apply the same correction to the related path around
lines 366–371, preserving the existing cleanup behavior when scheduling
succeeds.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/fabric_backup.py:
- Around line 161-165: Update the backup logging branch around bridge_members to
distinguish a supplied but missing or empty bridge_storage_path from a
controller-only backup. When bridge_storage_path is configured but no bridge
members are written, emit a clear log message stating that bridge storage was
omitted; retain the existing messages for backups containing bridge members and
for configurations without bridge storage.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/plugin.py:
- Around line 371-390: Update the watchdog guard in the health-tick method
around _issue_device_subscription so an initial subscribeToChanges failure can
enter the bounded retry path even when _subscribed_to_devices is false. Preserve
the existing early return for cases with no exported devices, and keep the
current retry counter, tick threshold, and MAX_RESUBSCRIBE_ATTEMPTS limit
unchanged.
In `@tests/test_fabric_backup.py`:
- Around line 519-523: Update the assertions in the backup test to iterate over
the archive-derived names rather than the hardcoded path list. Verify that every
non-bridge member in names lacks fabric_backup.BRIDGE_MEMBER_PREFIX, while also
asserting the controller members exist under their plain paths, including config
and certificates/root.pem.
---
Nitpick comments:
In `@bridge-node/package.json`:
- Line 18: Add a brief explanation near the test script documenting which
unclosed WebSocket or server handle necessitates --test-force-exit and the
failure mode it prevents; alternatively, identify and close that leaking handle
so the flag can be removed while preserving reliable test completion.
In `@bridge-node/src/endpoint-map.ts`:
- Around line 70-78: Update refuseReasonFor to return RefuseReasonValue |
undefined instead of string | undefined, importing or referencing the existing
RefuseReasonValue type from protocol.ts. Preserve the current branching and
returned RefuseReason values so endpointMapInvalidDetails receives the narrowed
union and rejects arbitrary reason strings at compile time.
In `@bridge-node/src/main.ts`:
- Line 70: Update the phase label in the identity check call around
identityProblem to use the noun phrase "identity check" instead of "identity
check failed", and use "identity load" for the corresponding identity load phase
call. Keep the phase wrapper responsible for adding failure wording.
In `@bridge-node/src/node.ts`:
- Around line 529-534: Extract the duplicated `LiveEndpointNumber[]` projection
from `rebuildEndpointMap` and `checkDrift` into a shared helper, then use that
helper in both methods before rebuilding or comparing the endpoint map. Preserve
the existing `#registry?.summaries()` fallback and projection behavior exactly.
In `@bridge-node/src/storage.ts`:
- Around line 207-214: Make the replacement behavior in loadOrCreateIdentity
structurally opt-in instead of relying on main.ts calling identityProblem first.
Add a required option or equivalent explicit authorization that callers must
provide before replacing an unreadable or invalid identity file, and ensure
callers without that authorization preserve the existing identity-problem
handling rather than re-minting values.
- Around line 103-125: Update writeJsonAtomic to flush the temporary file’s
contents with fsyncSync before renameSync, then open and fsyncSync the parent
directory after the rename to persist the directory entry. Handle unsupported
directory fsync cases, including Windows or network filesystems, without failing
the successful atomic write; preserve cleanup and error propagation for write,
flush, or rename failures.
In `@bridge-node/test/endpoint-map.test.ts`:
- Around line 165-179: Update the “does not rewrite the file when nothing was
added” test to verify the observable no-write behavior rather than filtering for
the “Endpoint map recorded” log prefix. Capture the loaded map file’s
modification state or, preferably, inject/count the writer used by
EndpointMapStore.persist and assert that quiet.check performs no write; use
logged only if needed as a secondary check and avoid relying on message wording.
In `@bridge-node/test/fixture-shapes.ts`:
- Around line 222-224: Add explicit `satisfies` clauses to the
`removeFabricResult` and `factoryResetResult` empty fixtures, using the response
shape types expected for §3.9 and §3.10. Match the established fixture typing
pattern used by `rebuiltStatus` and `endpointMapInvalid` so future shape changes
are validated.
In `@bridge-node/test/persistence.test.ts`:
- Around line 158-183: Ensure every test that creates a ServerNode through boot,
including the restart test, closes it in a finally block so assertion failures
cannot leak live nodes or storage locks. Apply the existing try/finally pattern
used by the test around line 320, preserving operations that intentionally read
state after close by placing only the appropriate assertions before or after
cleanup.
In `@bridge-node/test/protocol.test.ts`:
- Around line 909-921: Update the emitDrift call in the “drift_detected event
(§5)” test to cast golden.drift_detected.data.drift as DriftEntry[] instead of
never, preserving compile-time validation of the drift payload shape.
In `@tests/fakes.py`:
- Around line 177-182: Update the nested coroutine execution flow around the
thread.join call to use a bounded timeout, then detect whether the thread is
still alive and raise a clear diagnostic error if it failed to finish. Preserve
the existing outcome error propagation and value return for threads that
complete normally.
In `@tests/test_bridge_client.py`:
- Around line 1083-1102: Update
test_a_raising_provider_attaches_without_the_intent to assert that mock_logger
records the warning emitted by _replace_all when replace_all_provider raises,
while preserving the existing assertion that the attach command omits
bridge_protocol.ARG_INTENT.
In `@tests/test_export_wiring.py`:
- Around line 547-572: Update the resubscription tests around _resubscribe_tick
to derive retry counts from plugin_mod.RESUBSCRIBE_TICKS instead of the literal
4, and assert the final call count with plugin_mod.MAX_RESUBSCRIBE_ATTEMPTS
instead of 3. Preserve the existing test behavior and loop structure.
🪄 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: e1145b9a-15ae-44b3-8950-3462d0116aa2
⛔ Files ignored due to path filters (1)
bridge-node/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
bridge-node/package.jsonbridge-node/src/endpoint-map.tsbridge-node/src/main.tsbridge-node/src/node.tsbridge-node/src/protocol.tsbridge-node/src/reconcile.tsbridge-node/src/storage.tsbridge-node/src/ws-server.tsbridge-node/test/client.tsbridge-node/test/endpoint-map.test.tsbridge-node/test/fixture-shapes.tsbridge-node/test/fixtures.test.tsbridge-node/test/integration.test.tsbridge-node/test/persistence.test.tsbridge-node/test/protocol.test.tsbridge-node/test/storage.test.tsbridge-node/test/stub-bridge.tsdocs/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/fabric_backup.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pytests/fakes.pytests/fixtures/bridge_protocol/frames.jsontests/test_bridge_client.pytests/test_export_bridge.pytests/test_export_handlers.pytests/test_export_wiring.pytests/test_fabric_backup.py
| clearTimeout(escapeHatch); | ||
| log("Shutdown complete"); | ||
| // Both closes returned, in order, so everything that is ours is | ||
| // down — and what is left on the loop is not ours to wait for. | ||
| // matter.js 0.17.8's `ServerNode.erase()` (§3.10) leaves a ref'd | ||
| // timer behind that `close()` does not clear, measured at 0.17.8: | ||
| // without this, a perfectly clean shutdown *after a factory reset* | ||
| // would sit until the escape hatch fired and then exit 1, telling | ||
| // launchd a successful stop was a crash. Exit 0 here; the escape | ||
| // hatch still owns every path where a close does NOT return. | ||
| process.exit(0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js process.exit stdout pipe not flushed truncated output macOS
💡 Result:
The truncation of stdout when using process.exit occurs because process.exit terminates the Node.js process immediately, forcing it to stop even if there are asynchronous I/O operations pending [1][2]. In Node.js, writes to stdout and stderr are often asynchronous; when process.exit is called, these operations are interrupted before the data can be fully flushed to the underlying stream or pipe [1][2]. This issue is particularly noticeable on macOS and other Unix-like systems when piping output, as the buffering behavior differs from TTY output, increasing the likelihood that data remains in the buffer when the process is killed [3][4][5]. The recommended approach to avoid this truncation is to avoid using process.exit entirely [1][2]. Instead, you should: 1. Set the process.exitCode property to your desired exit code [1][2]. 2. Allow the process to exit naturally by ensuring no further asynchronous work is scheduled [1][2]. The Node.js event loop will then terminate the process gracefully once all remaining operations, including stdout/stderr flushes, are completed [1][2]. If you must terminate due to an error, throwing an uncaught error is generally safer than calling process.exit, as it allows the process to handle the termination more predictably [1][2]. While some developers have used workarounds such as setting stdout._handle.setBlocking(true) or manually draining streams with a promise before calling process.exit, these are not officially supported or guaranteed methods and may lead to other issues [3][6][7]. Top results: [1], [2], [8], [5]
Citations:
- 1: https://nodejs.org/api/process.html
- 2: https://nodejs.org/docs/latest-v26.x/api/process.html
- 3: stdio buffered writes (chunked) issues & process.exit() truncation nodejs/node#6456
- 4: Stdout is not completely flushed on process exit nodejs/node#2972
- 5: Inconsistent output depending on if it's piped somewhere or not nodejs/node#53242
- 6: process.stdout/.stderr might loose data when calling process.exit() nodejs/node#3669
- 7: https://stackoverflow.com/questions/42047997/flush-drain-stdout-stderr-in-node-js-process-before-exiting
- 8: aedneth/magnus-cli@c8fb260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Target file context"
wc -l bridge-node/src/main.ts
sed -n '1,190p' bridge-node/src/main.ts
echo
echo "## Runtime/package Node config"
if [ -f bridge-node/package.json ]; then
cat bridge-node/package.json
fi
if [ -f package.json ]; then
grep -n '"node":' package.json || true
fi
if [ -f .nvmrc ]; then cat .nvmrc; fi
if [ -f bridge-node/.nvmrc ]; then cat bridge-node/.nvmrc; fi
node -p "process.version" 2>/dev/null || true
echo
echo "## stdout drain behavioral probe"
node - <<'JS'
const { spawn } = require('child_process');
const script = `
process.exitCode = 0;
let n = 0;
function writeUntilFull() {
if (++n > 10000) return;
if (process.stdout.write('x'.repeat(4096))) {
writeUntilFull();
} else {
process.stdout.once('drain', function drain() {
process.stdout.once('drain', function drain2() {
process.stdout.write('exit now');
process.exit(0);
});
});
}
}
writeUntilFull();
`;
const child = spawn(process.execPath, ['-e', script], { stdio: ['ignore', 'pipe', 'pipe'] });
const out = [];
child.stdout.on('data', (d) => out.push(d.toString()));
child.stderr.on('data', (d) => process.stderr.write(d));
child.on('close', (code) => {
const text = out.join('');
console.log(JSON.stringify({
code,
stdoutLength: text.length,
endsWithExitNow: text.endsWith('exit now'),
expectedMinLength: 40960
}));
});
JSRepository: simons-plugins/indigo-matter
Length of output: 7856
Flush stdout before calling process.exit(0).
The clean shutdown path now prints Shutdown complete and then calls process.exit(0). When launchd redirects stdout to a pipe, process.exit can terminate before the buffered writes flush, so the final log can be cut off. Await one drain step before exiting, or avoid process.exit on this clean path if the loop is allowed to complete after the escapes are cleared.
🤖 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 `@bridge-node/src/main.ts` around lines 131 - 141, Update the clean shutdown
path after the “Shutdown complete” log to flush stdout before terminating: await
one stdout drain step if needed, or allow the event loop to finish naturally
after clearing the escape hatch. Preserve process.exit(0) for paths where
shutdown does not complete.
| private assertEndpointIdentity(): void { | ||
| if (this.#refusal !== undefined) { | ||
| this.refuse(this.#refusal); | ||
| return; | ||
| } | ||
| const reason = refuseReasonFor({ | ||
| commissioned: this.server.lifecycle.isCommissioned, | ||
| mapPresent: this.#endpointMap.present, | ||
| mapProblem: this.#endpointMap.problem, | ||
| commissionedAt: this.identity.commissionedAt, | ||
| }); | ||
| if (reason === undefined) { | ||
| this.noteCommissioningWitness(); | ||
| return; | ||
| } | ||
| if (reason === RefuseReason.fabricStorageLost) { | ||
| this.log( | ||
| `Identity records commissioning at ${this.identity.commissionedAt}, but the ` + | ||
| "Matter stack reports no fabrics", | ||
| ); | ||
| } | ||
| this.refuse(reason); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
matter.js 0.17.8 ServerNode events commissioning fabricsChanged emitted on start for existing fabrics
💡 Result:
In matter.js (including version 0.17.8), the server.events.commissioning.fabricsChanged event is designed to notify you of changes to commissioned fabrics (addition, removal, or update) that occur during the runtime of the server node [1][2]. It is not standard behavior for the fabricsChanged event to be emitted automatically upon the initial startup of a ServerNode for existing fabrics [1][2]. The fabricsChanged event is reactive, triggered by specific actions performed on the node, such as a fabric being added, removed, or updated via a controller interaction [1][2]. If you need to retrieve or inspect the list of existing fabrics when the server node starts, you should access the server.state.commissioning.fabrics object directly during or after the server's initialization (for example, in a server.lifecycle.online event listener) [2][3]. The fabrics property is dynamically maintained and will contain the current state of commissioned fabrics upon startup [4][3]. For identifying when a node is generally ready or has completed its initial state load, it is recommended to use the server.lifecycle.online event, which ensures the node is up and the state is available [3].
Citations:
- 1: https://github.com/project-chip/matter.js/blob/main/docs/MIGRATION_GUIDE_08.md
- 2: https://github.com/matter-js/matter.js/blob/HEAD/docs/MIGRATION_GUIDE_08.md
- 3: https://context7.com/matter-js/matter.js/llms.txt
- 4: Release 0.12 from 23.01.2025 matter-js/matter.js#1654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate bridge-node files =="
git ls-files 'bridge-node/**/*.{ts,tsx,js,jsx}' | sed -n '1,120p'
echo "== outline node.ts =="
ast-grep outline bridge-node/src/node.ts || true
echo "== relevant node.ts ranges =="
sed -n '1,90p' bridge-node/src/node.ts
sed -n '160,275p' bridge-node/src/node.ts
sed -n '500,550p' bridge-node/src/node.ts
echo "== package versions =="
for f in package.json bridge-node/package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat "$f" | sed -n '1,220p'
fi
done
echo "== refs to fabricsChanged and noteCommissioningWitness =="
rg -n "fabricsChanged|noteCommissioningWitness|commissionedAt|isCommissioned|commissioning.fabrics" bridge-node/src -SRepository: simons-plugins/indigo-matter
Length of output: 15232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bridge-node package lock / material locations =="
git ls-files | rg '(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lockb|node_modules|\.test-build|package\.json|src/node\.ts|src/endpoint-map\.ts|src/storage\.ts|tests/fixtures/bridge_protocol|package\.lock)' | sed -n '1,200p'
echo "== lockfile material for matter 0.17.8 =="
for f in package-lock.json bridge-node/package-lock.json pnpm-lock.yaml yarn.lock bun.lockb; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n '"`@matter/`(main|nodejs)"|`@matter/`(main|nodejs)|0\.17\.8' "$f" -C 2 -S | sed -n '1,200p'
fi
done
echo "== endpoint-map and storage relevant implementations =="
sed -n '35,90p' bridge-node/src/endpoint-map.ts
sed -n '155,205p' bridge-node/src/storage.ts
sed -n '270,295p' bridge-node/src/node.tsRepository: simons-plugins/indigo-matter
Length of output: 9407
Record the commissioning witness independently of the refusal decision.
noteCommissioningWitness() currently runs only after refuseReasonFor() returns undefined. If a commissioned bridge has a present but unreadable endpoint-map.json, the node refuses without setting commissionedAt, and rebuildEndpointMap() skips both clearing and setting the witness because isCommissioned is still true.
Record the witness when the server state and endpoint registry are available, then decide whether to refuse. Independent storage failures can still be tolerated, but placing the write inside the successful path leaves the §7 detection disabled for commissioned installs with map incidents.
🤖 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 `@bridge-node/src/node.ts` around lines 235 - 257, Update
assertEndpointIdentity so noteCommissioningWitness() runs whenever the server is
commissioned and the endpoint registry/map state is available, before evaluating
the refusal reason. Keep refusal decision and fabricStorageLost logging
unchanged, and allow independent witness-storage failures to remain non-fatal.
| // The refuse-to-start gate comes before everything, including the | ||
| // attach gate. In this state there is nothing to attach *to* — the node | ||
| // will not create an endpoint until its map is rebuilt — so answering | ||
| // `not_attached` would send the plugin round a reconnect loop that can | ||
| // never succeed, and hide the one error that names the remedy. The | ||
| // three §1.1 recovery commands are exempt from the attach requirement | ||
| // for the same reason: the client holding this socket open never got to | ||
| // attach, and `rebuild_endpoint_map` is the only way it ever will. | ||
| const refusal = this.options.bridge.endpointMapRefusal(); | ||
| if (refusal !== undefined) { | ||
| if (!RECOVERY_COMMANDS.has(command)) { | ||
| this.sendError( | ||
| socket, | ||
| messageId, | ||
| ErrorCode.endpointMapInvalid, | ||
| endpointMapInvalidDetails(refusal), | ||
| ); | ||
| return; | ||
| } | ||
| } else if (command !== "attach" && !state.attached) { | ||
| // Gating: before `attach` the node has said nothing about which | ||
| // commands it knows, so `not_attached` is the honest answer even for | ||
| // a name it would otherwise reject (§1.1). | ||
| this.sendError(socket, messageId, ErrorCode.notAttached, `${command} requires a successful attach first`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Resolve RECOVERY_COMMANDS membership and every protocolVersion check.
set -euo pipefail
echo "== RECOVERY_COMMANDS definition =="
rg -n -A 12 'RECOVERY_COMMANDS' bridge-node/src/protocol.ts
echo "== every protocolVersion check in the node =="
rg -n --type=ts -C 3 'protocolVersion' bridge-node/src
echo "== tests exercising recovery commands before attach =="
rg -n --type=ts -C 5 'rebuild_endpoint_map|factory_reset|remove_fabric' bridge-node/testRepository: simons-plugins/indigo-matter
Length of output: 23085
Add a protocol-version check to rebuild_endpoint_map in the refusal state.
RECOVERY_COMMANDS contains get_status, get_pairing, and rebuild_endpoint_map. The refusal branch admits all three without the normal protocolVersion comparison, and it skips setting state.attached, so handleAttach is the only version check on that socket. Require protocolVersion === PROTOCOL_VERSION for recovery frames, and still allow rebuild_endpoint_map before attach; only the attach flow itself cannot omit the version.
🤖 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 `@bridge-node/src/ws-server.ts` around lines 247 - 272, Update the
refusal-state recovery handling in the WebSocket command flow to require
protocolVersion === PROTOCOL_VERSION for recovery frames. Preserve pre-attach
access for rebuild_endpoint_map, while allowing attach to perform its existing
version validation and rejecting other recovery commands when the version is
missing or mismatched.
| if bridge_members: | ||
| log.info("Fabric backup written: %s (%d member(s), including %d from the Matter " | ||
| "export bridge node)", archive_path, members_written, bridge_members) | ||
| else: | ||
| log.info("Fabric backup written: %s (%d member(s))", archive_path, members_written) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Report when the bridge storage is omitted.
If bridge_storage_path is supplied but missing or empty, bridge_members remains zero and Line 165 logs the same message as a controller-only backup. Plugin._bridge_storage_path() documents that this case produces a log line, but no branch emits one. The user can then assume the bridge identity was backed up.
Proposed fix
if bridge_members:
log.info("Fabric backup written: %s (%d member(s), including %d from the Matter "
"export bridge node)", archive_path, members_written, bridge_members)
+ elif bridge_storage_path:
+ log.info("Fabric backup written: %s (%d member(s)); bridge-node storage was not included from %s",
+ archive_path, members_written, bridge_storage_path)
else:
log.info("Fabric backup written: %s (%d member(s))", archive_path, members_written)📝 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.
| if bridge_members: | |
| log.info("Fabric backup written: %s (%d member(s), including %d from the Matter " | |
| "export bridge node)", archive_path, members_written, bridge_members) | |
| else: | |
| log.info("Fabric backup written: %s (%d member(s))", archive_path, members_written) | |
| if bridge_members: | |
| log.info("Fabric backup written: %s (%d member(s), including %d from the Matter " | |
| "export bridge node)", archive_path, members_written, bridge_members) | |
| elif bridge_storage_path: | |
| log.info("Fabric backup written: %s (%d member(s)); bridge-node storage was not included from %s", | |
| archive_path, members_written, bridge_storage_path) | |
| else: | |
| log.info("Fabric backup written: %s (%d member(s))", archive_path, members_written) |
🤖 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/fabric_backup.py around
lines 161 - 165, Update the backup logging branch around bridge_members to
distinguish a supplied but missing or empty bridge_storage_path from a
controller-only backup. When bridge_storage_path is configured but no bridge
members are written, emit a clear log message stating that bridge storage was
omitted; retain the existing messages for backups containing bridge members and
for configurations without bridge storage.
| if not self._exported_ids or not self._subscribed_to_devices: | ||
| return | ||
| if self._device_updates_seen or self._resubscribe_attempts >= MAX_RESUBSCRIBE_ATTEMPTS: | ||
| return | ||
| self._no_update_ticks += 1 | ||
| if self._no_update_ticks < RESUBSCRIBE_TICKS: | ||
| return | ||
| self._no_update_ticks = 0 | ||
| self._resubscribe_attempts += 1 | ||
| # **Bounded, because "no updates" is also what a quiet house looks | ||
| # like.** A device that nobody touches genuinely produces no callback, | ||
| # so this cannot be a permanent retry loop without being permanent | ||
| # noise. A handful of re-issues covers the case it is for — a | ||
| # subscription that never registered — and after that the evidence is | ||
| # indistinguishable from nothing having happened. | ||
| self.logger.debug( | ||
| "Matter export: no device updates since subscribing; re-issuing " | ||
| "subscribeToChanges (attempt %d of %d)", | ||
| self._resubscribe_attempts, MAX_RESUBSCRIBE_ATTEMPTS) | ||
| self._issue_device_subscription() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retry a failed initial subscription.
If subscribeToChanges() raises at Lines 347-353, _subscribed_to_devices remains False. Line 371 then returns on every health tick. Exported device state remains stale until an allow-list change or plugin restart retries the call.
Allow the bounded watchdog path to run after an initial failure.
Proposed fix
- if not self._exported_ids or not self._subscribed_to_devices:
+ if not self._exported_ids:
return📝 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.
| if not self._exported_ids or not self._subscribed_to_devices: | |
| return | |
| if self._device_updates_seen or self._resubscribe_attempts >= MAX_RESUBSCRIBE_ATTEMPTS: | |
| return | |
| self._no_update_ticks += 1 | |
| if self._no_update_ticks < RESUBSCRIBE_TICKS: | |
| return | |
| self._no_update_ticks = 0 | |
| self._resubscribe_attempts += 1 | |
| # **Bounded, because "no updates" is also what a quiet house looks | |
| # like.** A device that nobody touches genuinely produces no callback, | |
| # so this cannot be a permanent retry loop without being permanent | |
| # noise. A handful of re-issues covers the case it is for — a | |
| # subscription that never registered — and after that the evidence is | |
| # indistinguishable from nothing having happened. | |
| self.logger.debug( | |
| "Matter export: no device updates since subscribing; re-issuing " | |
| "subscribeToChanges (attempt %d of %d)", | |
| self._resubscribe_attempts, MAX_RESUBSCRIBE_ATTEMPTS) | |
| self._issue_device_subscription() | |
| if not self._exported_ids: | |
| return | |
| if self._device_updates_seen or self._resubscribe_attempts >= MAX_RESUBSCRIBE_ATTEMPTS: | |
| return | |
| self._no_update_ticks += 1 | |
| if self._no_update_ticks < RESUBSCRIBE_TICKS: | |
| return | |
| self._no_update_ticks = 0 | |
| self._resubscribe_attempts += 1 | |
| # **Bounded, because "no updates" is also what a quiet house looks | |
| # like.** A device that nobody touches genuinely produces no callback, | |
| # so this cannot be a permanent retry loop without being permanent | |
| # noise. A handful of re-issues covers the case it is for — a | |
| # subscription that never registered — and after that the evidence is | |
| # indistinguishable from nothing having happened. | |
| self.logger.debug( | |
| "Matter export: no device updates since subscribing; re-issuing " | |
| "subscribeToChanges (attempt %d of %d)", | |
| self._resubscribe_attempts, MAX_RESUBSCRIBE_ATTEMPTS) | |
| self._issue_device_subscription() |
🤖 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 371
- 390, Update the watchdog guard in the health-tick method around
_issue_device_subscription so an initial subscribeToChanges failure can enter
the bounded retry path even when _subscribed_to_devices is false. Preserve the
existing early return for cases with no exported devices, and keep the current
retry counter, tick threshold, and MAX_RESUBSCRIBE_ATTEMPTS limit unchanged.
| # The prefix is what keeps two sibling directories apart in one archive — | ||
| # and what lets an old, controller-only backup still restore cleanly. | ||
| assert "config" in names | ||
| assert not any(name.startswith(fabric_backup.BRIDGE_MEMBER_PREFIX) | ||
| for name in ["config", "certificates/root.pem"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This assertion cannot fail — assert against the archive names.
The comprehension iterates the literal list ["config", "certificates/root.pem"], not names. It checks that two hardcoded strings do not start with the prefix, which is true independently of the archive. Assert that every non-bridge member of names is unprefixed, and that the controller members are present under their plain paths.
🐛 Proposed fix
assert "config" in names
- assert not any(name.startswith(fabric_backup.BRIDGE_MEMBER_PREFIX)
- for name in ["config", "certificates/root.pem"])
+ assert "certificates/root.pem" in names
+ # Controller members keep their plain, storage-root-relative paths.
+ assert {name for name in names
+ if not name.startswith(fabric_backup.BRIDGE_MEMBER_PREFIX)} >= {
+ "config", "certificates/root.pem"}📝 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.
| # The prefix is what keeps two sibling directories apart in one archive — | |
| # and what lets an old, controller-only backup still restore cleanly. | |
| assert "config" in names | |
| assert not any(name.startswith(fabric_backup.BRIDGE_MEMBER_PREFIX) | |
| for name in ["config", "certificates/root.pem"]) | |
| # The prefix is what keeps two sibling directories apart in one archive — | |
| # and what lets an old, controller-only backup still restore cleanly. | |
| assert "config" in names | |
| assert "certificates/root.pem" in names | |
| # Controller members keep their plain, storage-root-relative paths. | |
| assert {name for name in names | |
| if not name.startswith(fabric_backup.BRIDGE_MEMBER_PREFIX)} >= { | |
| "config", "certificates/root.pem"} |
🤖 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_fabric_backup.py` around lines 519 - 523, Update the assertions in
the backup test to iterate over the archive-derived names rather than the
hardcoded path list. Verify that every non-bridge member in names lacks
fabric_backup.BRIDGE_MEMBER_PREFIX, while also asserting the controller members
exist under their plain paths, including config and certificates/root.pem.
Two independent expert reviews plus a test-coverage review: 5 Criticals, ~10 Highs, 5 Mediums, 9 coverage gaps. The deploy blocker first. A1 — a commissioned bridge with NO endpoint-map.json refused to serve, and that is the state of every install commissioned before E5, because the file did not exist yet. jarvis today: 2 fabrics, 4 endpoints, no map — deploying E5 as written would have refused the attach and taken all four accessories offline. The refusal bought nothing either: matter.js OWNS the numbers, keyed on Endpoint.id in its own store, and this map is only the witness, so a missing witness renumbers precisely nothing. Absent-on-commissioned is now a BOOTSTRAP — seed the baseline from matter.js's persisted allocation (read through ServerNodeStore, guarded, falling back to the first attach's live set), log it as a migration, serve. Only a present-but-unreadable map refuses. A2 — rebuild_endpoint_map had no caller anywhere in the plugin while three user-facing strings told the user to confirm the rebuild "in the plugin". Ships the exits: "Rebuild Matter Endpoint Map…" (§3.11, confirm + duplication warning) and "Reset Matter Export Pairings…" (§3.10 preserve=true, two confirms). Both gate on connected, not attached — §1.1 holds the socket open un-attached and that is the only state a rebuild is ever needed in. remove_fabric still has no UI; it needs E6's fabric readout to pick from. A3 — main.ts called identityProblem() then loadOrCreateIdentity() unconditionally, and the mint writes through rename: an unreadable identity.json was destroyed one line BEFORE the refusal that exists to protect it, taking the SerialNumber and UniqueID every paired ecosystem knows us by. Now moved aside to identity.json.unreadable-<stamp>, minted in memory only, nothing written. rebuildEndpointMap refuses to clear an identityUnreadable refusal — different loss, different remedy. A4 — pending debt plus a disjoint re-add was a permanent halt. _owes_replace_all was ANDed with an empty allow-list, so emptying the list while the node was down and then exporting a different device sent an attach with no intent; the node's guard saw N removals against zero survivors, answered mass_removal_refused — which HALTS — and blamed an allow-list that was never the problem. The debt now answers independently of store emptiness, and mass_removal_refused retries once with the intent while a debt is recorded instead of halting. A5 — remove_fabric on the last fabric: matter.js self-factory-resets when the set empties, and our witness was left set, so the next boot refused with fabricStorageLost and blamed lost storage for a deliberate unpairing. Handled in noteFabrics/noteLastFabricGone, so an ecosystem unpairing US is covered too. Visibility of persistence failures — the milestone's whole point: - StatusReport gains warnings[] (§4.3). The node's only other channel is a stdout nobody is watching while it is hand-started. persist(), markCommissioned() and clearCommissioned() report whether the write landed; rebuild_endpoint_map FAILS rather than saying "serving normally again" over a map that never reached disk; factory_reset re-reads identity.json to verify the witness is gone before reporting completion. - driftChecked no longer asserts an all-clear over a RAM-only baseline (#dirty), and check() retries the failed write even when it added nothing. - The §5 command executor gets a 30s deadline naming command+device, submitted/completed counters, a health_tick queue-depth warning and a dropped-count line at stop(). - fabric_backup WARNS, naming the path, when the bridge storage dir is missing or empty — and names it on success too. - _set_color_temp returns the E4 tri-state reason instead of a bare None, so a no-op stops being reported as success. - The node stops closing an un-attached socket while it is refusing; the discharge attach is sized from the debt count, not len([]); the debt is discharged on what the attach CARRIED, not on live state; and fabrics_changed/commissioned/decommissioned are finally emitted. Medium: factory_reset re-checks drift and says what preservation bought (erase() wipes matter.js's own allocation, so it preserves the ability to NOTICE, not the numbers); _on_drift_detected is latched per drift-set; an unusable map is copied to .corrupt before a rebuild; removeEndpoint runs the detector; _indigo_device narrows to KeyError/IndexError and reports anything else distinctly; stop() shuts the executor down with cancel_futures and latches so a queued coroutine cannot rebuild it; _un_exporting cannot latch forever on a submit failure; the plugin parses and surfaces driftChecked/drift/warnings in the export dialog. Docs: §3.9/§3.10/§3.11/§4.3/§5 corrected — what preserveEndpointNumbers actually preserves, what §3.11 actually does, warnings[], when drift is checked, and the RECOVERY_COMMANDS design call (factory_reset stays gated; §3.11 is the non-destructive exit and already covers every refusal state). HANDOVER carries the corrected refuse-to-start table, the real test counts, the process-scoped stopped-keys caveat, and the three paths that need live hardware. CLAUDE.md gains the bridge-node module table. PluginVersion 2026.7.31. 2025 Python / 316 TS tests, pylint 9.39. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
Verified against a copy of jarvis's real pre-E5 bridge storage (2 fabrics, 4 exported endpoints, no endpoint-map.json): the bootstrap correctly SERVES rather than refuses, but seeded 0 numbers while logging that it had adopted them from matter.js — a false success in a migration path. The reader walked root->parts->aggregator->parts and called contexts(), which enumerates only subcontexts already materialised in memory; at bootstrap none are. The numbers live in flat on-disk keys whose layout is matter.js-internal. Deleted rather than re-derived: the fallback records the live set on the first reconcile, and those numbers ARE matter.js's persisted ones (it restores each Endpoint.id's number as the endpoint is created), so the witness is identical for everything we export. An endpoint we do not export gains nothing from an entry — its number is decided by its id whenever it is created. Removes the coupling to matter.js's internal storage-context names that the review flagged, and the log now states what actually happens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/fakes.py (1)
146-182: 🩺 Stability & Availability | 🟡 MinorHandle loop-bound work in nested
RecordingRuntime.submit.
_run_to_completionoffloads tothreading.Threadfrom an event loop path, then joins that thread synchronously. If the submitted coroutine awaits objects bound to the caller’s loop, execution can deadlock or raise a non-deterministic cross-loop error. Run loop-bound submissions on the owning loop, or reject them for fake tests and add coverage forasyncio.Future,create_future, and transport callbacks.[low_effort和high_reward]
🤖 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/fakes.py` around lines 146 - 182, The nested path in _run_to_completion must not execute coroutines on a new worker loop when they await objects owned by the caller’s event loop. Update RecordingRuntime.submit and its completion flow to run loop-bound submissions on the owning loop, or explicitly reject unsupported loop-bound work in the fake; add coverage for asyncio.Future, create_future, and transport-callback cases.bridge-node/test/endpoint-map.test.ts (1)
351-379: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThese two tests assert the same call, so one of the two named cases is uncovered.
Line 358 and Line 376 pass identical arguments —
{ commissioned: true, commissionedAt: "2026-08-01T00:00:00.000Z" }— and assert the sameundefined. The names claim two different states: "no map at all" and "a usable map".refuseReasonForcannot tell them apart, becausemapProblemis now its only map input and both states leave itundefined.The distinction is real, and it lives one level up:
assertEndpointIdentityinbridge-node/src/node.tsreadsthis.#endpointMap.presentat Line 270 and callsbootstrapEndpointMap()only for the no-map case. Merge these two tests into one, and keep the no-map-versus-usable-map distinction where a branch actually depends on it. The bootstrap tests at Lines 382-421 already cover the store side of it.🤖 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 `@bridge-node/test/endpoint-map.test.ts` around lines 351 - 379, Remove the redundant “serves a commissioned bridge with a usable map” test from the refuseReasonFor cases and retain the commissioned no-map upgrade-path assertion. Preserve the distinction between missing and usable endpoint maps in assertEndpointIdentity, where endpointMap.present controls whether bootstrapEndpointMap() is called; do not represent that distinction through identical refuseReasonFor inputs.
♻️ Duplicate comments (1)
bridge-node/src/node.ts (1)
258-283: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe commissioning witness is still recorded only on the non-refusing path.
noteCommissioningWitness()at Line 273 runs only whenreason === undefined. A commissioned bridge that refuses formapUnreadabletherefore never recordscommissionedAton that boot.rebuildEndpointMapdoes not record it either: Line 850 callsclearCommissionedonly whenisCommissionedis false, and nothing callsmarkCommissioned. The witness stays absent until the next restart or the nextfabricsChangedobservation.During that window a loss of matter.js storage is undetectable: the next start sees no witness and no fabrics, so
refuseReasonForreturnsundefinedand the node serves and re-creates every accessory. The precondition is narrow — the map must become unusable before the witness was ever written, which is the shape of a pre-E5 upgrade whose first map write is corrupted — but the remedy is one line.Record the witness before the refusal decision. The write is already non-fatal and already reports its own failure through
applyWitness.🛡️ Proposed fix
const commissioned = this.server.lifecycle.isCommissioned; + // Before the decision, not after it. The witness records "endpoint + // numbers now matter to somebody", which is true whether or not the map + // is readable — and a refusal that skips it leaves the §7 storage-loss + // detection disabled until the next restart. + this.noteCommissioningWitness(); const reason = refuseReasonFor({ commissioned, mapProblem: this.#endpointMap.problem, commissionedAt: this.identity.commissionedAt, }); if (reason === undefined) { if (commissioned && !this.#endpointMap.present) { this.bootstrapEndpointMap(); } - this.noteCommissioningWitness(); return; }
refuseReasonForreadscommissionedAtonly on the uncommissioned branch, so recording it first does not change any refusal outcome.🤖 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 `@bridge-node/src/node.ts` around lines 258 - 283, Move noteCommissioningWitness() in assertEndpointIdentity() to execute after the initial refusal check but before refuseReasonFor evaluates the endpoint state, so commissioned bridges record commissionedAt even when refusing for mapUnreadable. Preserve the existing bootstrap behavior and refusal handling, including the early return for this.#refusal.
🧹 Nitpick comments (1)
indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py (1)
1185-1213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider latching the read-failure warning.
The narrow/broad split is right. The broad-catch warning is not deduplicated, though.
endpoint_specscalls_device_getteronce per allow-list entry on every reconnect, so a broken IPC connection writes one warning per exported device per attach. Every other persistent condition in this module is latched for that reason (_skipped,_update_failed,_node_warnings,_drift_reported).The module-level function has no state to latch on. One option is to move the reporting into a small
ExportBridgehelper that owns the latch and clears it on the first successful read.🤖 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 1185 - 1213, Latch broad-catch read-failure warnings instead of logging once per allow-list entry on each reconnect. Move the warning responsibility from _indigo_device into an ExportBridge-owned helper/state, suppress repeated warnings while reads continue failing, and clear the latch after the first successful device read; preserve None for genuinely absent device IDs.
🤖 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 `@bridge-node/test/endpoint-map.test.ts`:
- Around line 259-301: Add an injectable writer seam to EndpointMapStore and use
it in the persistence path exercised by check and rebuild, allowing tests to
deterministically simulate write failure without relying on directory
permissions or process privileges. Update all three write-failure tests to
inject a failing writer, then retain the assertions that check leaves checked
false and rebuild returns false with a warning before verifying successful
retries.
In `@bridge-node/test/persistence.test.ts`:
- Around line 375-396: Update the test setup around rebuild_endpoint_map so it
explicitly verifies commissionedAt is present before factory_reset runs. Create
a witness-present, non-refusing state, such as commissioning a fabric so
rebuildEndpointMap does not invoke clearCommissioned, then assert the identity
witness remains before checking the factory-reset warnings and removal.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py:
- Around line 329-346: Update indigo-matter.indigoPlugin/Contents/Server
Plugin/export_bridge.py:329-346 so _owes_replace_all returns the integer from
_pending_replace_all() and is annotated as int; exports_changed requires no
direct change. Leave indigo-matter.indigoPlugin/Contents/Server
Plugin/bridge_client.py:478-485 unchanged, as it will consume the corrected debt
count. Update tests/test_bridge_client.py:1049 to provide a count such as 1
instead of True, and update tests/test_export_bridge.py:1057-1065 to assert the
expected integer count for both _owes_replace_all() and the provider.
---
Outside diff comments:
In `@bridge-node/test/endpoint-map.test.ts`:
- Around line 351-379: Remove the redundant “serves a commissioned bridge with a
usable map” test from the refuseReasonFor cases and retain the commissioned
no-map upgrade-path assertion. Preserve the distinction between missing and
usable endpoint maps in assertEndpointIdentity, where endpointMap.present
controls whether bootstrapEndpointMap() is called; do not represent that
distinction through identical refuseReasonFor inputs.
In `@tests/fakes.py`:
- Around line 146-182: The nested path in _run_to_completion must not execute
coroutines on a new worker loop when they await objects owned by the caller’s
event loop. Update RecordingRuntime.submit and its completion flow to run
loop-bound submissions on the owning loop, or explicitly reject unsupported
loop-bound work in the fake; add coverage for asyncio.Future, create_future, and
transport-callback cases.
---
Duplicate comments:
In `@bridge-node/src/node.ts`:
- Around line 258-283: Move noteCommissioningWitness() in
assertEndpointIdentity() to execute after the initial refusal check but before
refuseReasonFor evaluates the endpoint state, so commissioned bridges record
commissionedAt even when refusing for mapUnreadable. Preserve the existing
bootstrap behavior and refusal handling, including the early return for
this.#refusal.
---
Nitpick comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py:
- Around line 1185-1213: Latch broad-catch read-failure warnings instead of
logging once per allow-list entry on each reconnect. Move the warning
responsibility from _indigo_device into an ExportBridge-owned helper/state,
suppress repeated warnings while reads continue failing, and clear the latch
after the first successful device read; preserve None for genuinely absent
device IDs.
🪄 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: 1ae383cb-313f-4fb4-9d52-1ab78c1ff43f
📒 Files selected for processing (34)
CLAUDE.mdbridge-node/package.jsonbridge-node/src/endpoint-map.tsbridge-node/src/main.tsbridge-node/src/node.tsbridge-node/src/protocol.tsbridge-node/src/storage.tsbridge-node/src/ws-server.tsbridge-node/test/endpoint-map.test.tsbridge-node/test/fixture-shapes.tsbridge-node/test/main.test.tsbridge-node/test/persistence.test.tsbridge-node/test/protocol.test.tsbridge-node/test/storage.test.tsbridge-node/test/stub-bridge.tsdocs/BRIDGE_PROTOCOL.mddocs/HANDOVER.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xmlindigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.pyindigo-matter.indigoPlugin/Contents/Server Plugin/bridge_protocol.pyindigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.pyindigo-matter.indigoPlugin/Contents/Server Plugin/export_handlers.pyindigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pytests/fakes.pytests/fixtures/bridge_protocol/frames.jsontests/test_bridge_client.pytests/test_bridge_protocol_frames.pytests/test_export_bridge.pytests/test_export_handlers.pytests/test_export_menu.pytests/test_export_wiring.pytests/test_fabric_backup.py
🚧 Files skipped from review as they are similar to previous changes (13)
- indigo-matter.indigoPlugin/Contents/Info.plist
- bridge-node/package.json
- bridge-node/test/storage.test.ts
- bridge-node/src/main.ts
- bridge-node/test/fixture-shapes.ts
- bridge-node/src/ws-server.ts
- tests/fixtures/bridge_protocol/frames.json
- docs/HANDOVER.md
- bridge-node/test/stub-bridge.ts
- bridge-node/src/protocol.ts
- tests/test_export_handlers.py
- indigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.py
- tests/test_fabric_backup.py
| describe("a write that did not land (E5 B1/B2)", () => { | ||
| it("never claims driftChecked over a RAM-only baseline", () => { | ||
| // ⊗ `#checked` used to be set unconditionally. A `driftChecked: true` | ||
| // over a map that never reached disk asserts an all-clear about a | ||
| // baseline the next restart cannot find — so that device's reallocated | ||
| // number gets recorded as the truth and the real renumbering is never | ||
| // reported by anybody, ever. | ||
| const dir = storage(); | ||
| const store = new EndpointMapStore(dir); | ||
| store.load(); | ||
| chmodSync(dir, 0o500); | ||
| try { | ||
| assert.deepEqual(store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]), []); | ||
| assert.equal(store.checked, false, "the write failed, so nothing durable was checked"); | ||
| } finally { | ||
| chmodSync(dir, 0o700); | ||
| } | ||
|
|
||
| // And the retry: the next check re-attempts the write even though it | ||
| // adds nothing, because the map still owes the disk what it holds. | ||
| assert.deepEqual(store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]), []); | ||
| assert.equal(store.checked, true); | ||
| assert.deepEqual(mapFileIn(dir).endpoints, { "indigo-1": 2 }); | ||
| }); | ||
|
|
||
| it("tells the caller a rebuild did not persist, and warns", () => { | ||
| const dir = storage(); | ||
| const store = new EndpointMapStore(dir); | ||
| store.load(); | ||
| chmodSync(dir, 0o500); | ||
| try { | ||
| assert.equal(store.rebuild([{ uniqueId: "indigo-1", endpointNumber: 2 }]), false); | ||
| // §4.3 `warnings`: the node's log is a stdout nobody is watching in | ||
| // this milestone, so a failed write has to reach get_status. | ||
| assert.equal(store.warnings.length, 1); | ||
| assert.match(store.warnings[0]!, /Could not write the endpoint map/); | ||
| } finally { | ||
| chmodSync(dir, 0o700); | ||
| } | ||
| assert.equal(store.rebuild([{ uniqueId: "indigo-1", endpointNumber: 2 }]), true); | ||
| assert.deepEqual(store.warnings, [], "a warning is current, not historical"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both new write-failure tests break when the suite runs as root.
chmodSync(dir, 0o500) at Lines 269 and 288 removes write permission for ordinary users only. Root bypasses the mode bits, so the write succeeds. Then store.checked is true and the assertion at Line 272 fails, and store.rebuild(...) returns true and the assertion at Line 290 fails. Container CI images commonly run as root, so these are false failures rather than silently passing tests.
This is the same mechanism raised on the earlier check write-failure test. One injected writer seam on EndpointMapStore would fix all three deterministically. A uid skip is the smaller change but proves nothing on the machine where it matters.
💚 Minimal guard, if the seam is deferred
-describe("a write that did not land (E5 B1/B2)", () => {
- it("never claims driftChecked over a RAM-only baseline", () => {
+// Root bypasses the mode bits below, so the read-only directory is not a write
+// failure for that user and every assertion here would run against a write that
+// actually succeeded.
+const asRoot = typeof process.getuid === "function" && process.getuid() === 0;
+
+describe("a write that did not land (E5 B1/B2)", { skip: asRoot ? "file modes do not restrict root" : false }, () => {
+ it("never claims driftChecked over a RAM-only baseline", () => {🤖 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 `@bridge-node/test/endpoint-map.test.ts` around lines 259 - 301, Add an
injectable writer seam to EndpointMapStore and use it in the persistence path
exercised by check and rebuild, allowing tests to deterministically simulate
write failure without relying on directory permissions or process privileges.
Update all three write-failure tests to inject a failing writer, then retain the
assertions that check leaves checked false and rebuild returns false with a
warning before verifying successful retries.
Three whole-PR expert reviews (code correctness, silent failures, test coverage via 136 mutations — 37 survived). The survivors share one shape, worth naming because it will recur: the helper is covered, the real caller is not. `fabric_backup` has a thorough suite and its only call site was unpinned; `_resubscribe_tick` had four tests and nothing called it from `_health_tick`; the queue counters were set by hand and nothing drove them through a dispatch. Deleting the wiring left the suite green every time. The five that would have hurt a real user: * The confirm gates were inert IN THE TESTS, live in production. Deleting either recovery menu's confirm check kept the suite green, and with a live client the destructive action really ran unticked — the tests never set `export_bridge`, so the connection check below filled the same errors key. Now asserted against the client, which is the only real question. * `replace_all_provider` was a bool where a count is required. The client declares `Callable[[], int]` and sizes the discharge attach's deadline from it; `int(True) == 1` gave every discharge the 8s floor, so an 80-accessory un-export timed out mid-reconcile and retried forever — the regression the formula exists to prevent. The wrapper is gone. * The §4.3 warnings channel had no reader. `get_status` had zero production callers while four docstrings and the protocol said it was polled, so the three faults that happen after the attach reached the user as nothing. `health_tick` now polls it. * The identity refusal lasted exactly one restart: the next start read the quarantined-aside file as a first run and minted a new SerialNumber. `identityProblem` now also refuses on a leftover quarantine marker, and `identityUnreadable` gets its own remedy text — a rebuild cannot fix it. * `rebuild_endpoint_map` reported the opposite of what happened. The node writes the map and stops refusing before answering, so a failed re-attach was reported as "unchanged and still refusing", left recovery set, and invited the user to repeat an irreversible operation. Also: the un-export debt accumulates and is only cleared by a watched discharge (an add-then-remove cycle could erase it); the resubscribe watchdog says so when it gives up; `noteFabrics` no longer swallows three failures at once; ignored `seed`/`discard`/witness returns are checked; `driftChecked` no longer goes true off an empty comparison; corrupt-map quarantine covers `persist()` and is timestamped; the rebuild menu requires a refusal to be in force. Docs: PRD §7 split into the four real cases and §4.3's drift wording corrected; BRIDGE_PROTOCOL §1.1 gains the reason→remedy table and §4.3 makes polling mandatory. Version 2026.8.0 (two user-facing menu items plus a persistence subsystem). Suites: 2056 Python (from 2025), 344 TS (from 316). pylint 9.40. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bridge-node/src/endpoint-map.ts (1)
404-417: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSet
#quarantinedonly after the copy succeeds.Line 408 marks the file quarantined before
copyFileSyncruns. If the copy fails, the flag staystrue, so no later attempt is made. The source file still exists at that moment, andpersistretries on the nextcheckwhile#dirtyis set — that retry is the one remaining chance to preserve the bytes. Moving the assignment into the success path keeps the duplicate-suppression intent and restores the retry.🛡️ Proposed fix
- this.#quarantined = true; const file = join(this.storagePath, ENDPOINT_MAP_FILE); const target = `${file}.corrupt-${this.now().toISOString().replace(/[:.]/g, "-")}`; try { copyFileSync(file, target); + this.#quarantined = true; this.log(`Unusable endpoint map copied to ${target} before it is overwritten`); } catch (error) { this.log(`Could not copy the unusable endpoint map aside: ${describeError(error)}`); }🤖 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 `@bridge-node/src/endpoint-map.ts` around lines 404 - 417, Update quarantineUnusable so `#quarantined` is assigned true only after copyFileSync succeeds. Keep the early-return duplicate suppression and existing success logging unchanged; leave the flag unset when copying fails so later persist/check retries can attempt preservation again.
♻️ Duplicate comments (1)
bridge-node/src/node.ts (1)
261-286: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA commissioned bridge that refuses never records the commissioning witness, and the rebuild does not repair it.
noteCommissioningWitness()runs only whenreason === undefined(Line 276). On a commissioned bridge with an unusableendpoint-map.json,refuseReasonForanswersmapUnreadable, so the witness is never stamped.rebuildEndpointMapthen clears the refusal, but its witness branch at Line 886 runs only when!isCommissioned, so it neither sets nor clearscommissionedAt. The bridge ends up serving while paired with no witness on disk.That disarms the §7 check the witness exists for: on a later boot where matter.js storage is gone,
refuseReasonForseescommissioned: falseandcommissionedAt: undefined, returnsundefined, and the node serves — re-creating every accessory in every paired ecosystem without refusing.
fabricsChangedat Line 209 does not close the gap. It fires on a fabric change, not for fabrics that already exist when the node starts, so a bridge that simply stays paired never reaches it.Record the witness once the fabric table is readable, before the refusal decision, and keep the write failure non-fatal.
🛡️ Proposed fix
private assertEndpointIdentity(): void { if (this.#refusal !== undefined) { this.refuse(this.#refusal); return; } const commissioned = this.server.lifecycle.isCommissioned; + // Before the decision: the witness records "endpoint numbers now + // matter to somebody", which is true whether or not we go on to + // refuse — and a refusal that suppresses it disarms the §7 check for + // every later boot. + this.noteCommissioningWitness(); const reason = refuseReasonFor({ commissioned, mapProblem: this.#endpointMap.problem, commissionedAt: this.identity.commissionedAt, }); if (reason === undefined) { if (commissioned && !this.#endpointMap.present) { this.bootstrapEndpointMap(); } - this.noteCommissioningWitness(); return; }Note that
refuseReasonForreadscommissionedAtonly on the un-commissioned branch, so stamping it first does not change this boot's decision.🤖 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 `@bridge-node/src/node.ts` around lines 261 - 286, Update assertEndpointIdentity to call noteCommissioningWitness once the fabric table is readable, before evaluating refuseReasonFor, while keeping witness-write failures non-fatal. Ensure this also occurs when the endpoint map is unusable, and remove or adjust the rebuildEndpointMap witness logic so a commissioned bridge preserves the recorded commissionedAt witness when refusal is cleared.
🧹 Nitpick comments (5)
tests/test_plugin_behaviour.py (1)
562-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Pylint-compliant test name.
Line [562] contains uppercase
BRIDGEin a function identifier. Rename it totest_menu_export_fabric_backup_archives_the_bridge_storage_too, unless the repository Pylint configuration explicitly permits uppercase acronyms. Otherwise, Pylint can reportinvalid-name.As per coding guidelines, Python files must follow the project's Pylint standards.
Proposed rename
-def test_menu_export_fabric_backup_archives_the_BRIDGE_storage_too( +def test_menu_export_fabric_backup_archives_the_bridge_storage_too(🤖 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_plugin_behaviour.py` around lines 562 - 563, Rename the test function test_menu_export_fabric_backup_archives_the_BRIDGE_storage_too to the Pylint-compliant lowercase form test_menu_export_fabric_backup_archives_the_bridge_storage_too, unless the repository configuration explicitly allows uppercase acronyms.Source: Coding guidelines
tests/test_bridge_client.py (2)
787-790: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiscard the unused
fakebinding.Ruff reports RUF059 here because
fakeis never read in this test. If RUF059 is enforced in CI, this fails lint.♻️ Proposed fix
- fake, client = self._recovering(mock_logger, overrides={ + _, client = self._recovering(mock_logger, overrides={🤖 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_bridge_client.py` around lines 787 - 790, Remove the unused fake binding from the _recovering call in this test, retaining only the client value that is read and preserving the existing overrides configuration.Source: Linters/SAST tools
1352-1372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the module's
runhelper for consistency.Every other test in this file drives its coroutine through
run(...). This helper callsasyncio.rundirectly. Ifrunperforms any shared setup or cleanup, this test does not get it.♻️ Proposed fix
- asyncio.run(scenario()) + run(scenario())🤖 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_bridge_client.py` around lines 1352 - 1372, Update the _sizes test helper to execute scenario through the module’s run helper instead of calling asyncio.run directly, preserving the existing coroutine setup and assertions.tests/test_export_wiring.py (1)
631-660: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrive the tick counts from the plugin constants.
Both tests hard-code
4and4 * 10, while_resubscribe_tickreadsRESUBSCRIBE_TICKSandMAX_RESUBSCRIBE_ATTEMPTS. IfRESUBSCRIBE_TICKSchanges,test_the_re_issues_are_a_minute_apart_not_back_to_backruns fewer ticks than one window and passes without ever reaching the re-issue it exists to pin.♻️ Proposed refactor
subscribe = self._exporting(plug, mock_indigo_base) - for _ in range(4): + for _ in range(plugin_mod.RESUBSCRIBE_TICKS): plug._resubscribe_tick() assert subscribe.call_count == 1 plug._resubscribe_tick() # one tick later — must NOT re-issue assert subscribe.call_count == 1Add
plugin_modto the test signature, and apply the same substitution to the give-up test's loop bound.🤖 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` around lines 631 - 660, Update both tests, test_the_re_issues_are_a_minute_apart_not_back_to_back and test_giving_up_says_so_once_and_names_the_consequence, to accept plugin_mod and derive loop counts from plugin_mod.RESUBSCRIBE_TICKS and plugin_mod.MAX_RESUBSCRIBE_ATTEMPTS instead of hard-coded 4 and 4 * 10 values. Preserve the existing assertions and timing semantics while ensuring the tests reach the configured reissue and give-up thresholds.tests/test_export_bridge.py (1)
1415-1479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
monkeypatch.setattrfor the handler patches.Both tests replace
handler.dispatchby hand and restore it in afinally.handleris a module-level singleton fromexport_handlers.HANDLERS, so a failure between the assignment and thetryleaks the patched dispatch into every later test in the session.monkeypatchrestores the attribute even then, and it removes thereal_dispatch/try/finallyboilerplate.The same tests also use a local
import export_handlers, while the sibling test at line 906 reaches the module throughbridge_mod.export_handlers. Pick one form for both.♻️ Proposed refactor for lines 1430-1444
- # The command half-applies: dispatch raises, but the relay really moved. - import export_handlers - handler = export_handlers.handler_for("onOffLight") - real_dispatch = handler.dispatch - - def boom(command, args, device, options): - device.onState = True # the device DID move - raise RuntimeError("driver blew up after switching") - - handler.dispatch = boom - try: - h.bridge.on_command( - bridge_protocol.parse_command(FRAMES["command_on_off"]["data"])) - finally: - handler.dispatch = real_dispatch + # The command half-applies: dispatch raises, but the relay really moved. + handler = bridge_mod.export_handlers.handler_for("onOffLight") + + def boom(command, args, device, options): + device.onState = True # the device DID move + raise RuntimeError("driver blew up after switching") + + monkeypatch.setattr(handler, "dispatch", boom) + h.bridge.on_command( + bridge_protocol.parse_command(FRAMES["command_on_off"]["data"]))Add
monkeypatchto both test signatures.🤖 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` around lines 1415 - 1479, Update both tests, test_a_CORRECTION_folds_into_the_snapshot_it_just_pushed and test_a_correction_is_gated_on_a_LIVE_client_like_every_other_push, to accept pytest’s monkeypatch fixture and use monkeypatch.setattr on the shared handler.dispatch instead of manual save/restore with try/finally. Use the same export_handlers access style in both tests, matching the neighboring bridge_mod.export_handlers usage, and remove the real_dispatch and restoration boilerplate.
🤖 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 `@bridge-node/test/endpoint-map.test.ts`:
- Around line 330-336: Guard both chmod-based write-failure tests in
bridge-node/test/endpoint-map.test.ts:330-336 and
bridge-node/test/endpoint-map.test.ts:352-358 by accepting the test context
argument in “clears the persistence warnings a successful discard has settled”
and “drops a stale warning rather than carrying it across a re-read,” then
skipping when process.getuid?.() === 0 before each chmodSync call.
In `@docs/HANDOVER.md`:
- Around line 101-104: Update the M11 documentation to replace “retained
endpoint-number allocations” with “retained endpoint-map entries,” clarifying
that rebuilding the witness can discard non-live map entries but does not affect
matter.js endpoint-number allocations or endpoint restoration after re-addition.
In `@tests/test_bridge_client.py`:
- Around line 840-876: Extend rebuild_endpoint_map to handle
asyncio.TimeoutError from the re-attach request the same way as a
BridgeProtocolError after a successful rebuild: clear client.recovery, preserve
and return the rebuild status, and log the re-attach refusal warning instead of
propagating the exception. Update the test scenario around rebuild_endpoint_map
to simulate a timed-out attach response and verify these outcomes.
---
Outside diff comments:
In `@bridge-node/src/endpoint-map.ts`:
- Around line 404-417: Update quarantineUnusable so `#quarantined` is assigned
true only after copyFileSync succeeds. Keep the early-return duplicate
suppression and existing success logging unchanged; leave the flag unset when
copying fails so later persist/check retries can attempt preservation again.
---
Duplicate comments:
In `@bridge-node/src/node.ts`:
- Around line 261-286: Update assertEndpointIdentity to call
noteCommissioningWitness once the fabric table is readable, before evaluating
refuseReasonFor, while keeping witness-write failures non-fatal. Ensure this
also occurs when the endpoint map is unusable, and remove or adjust the
rebuildEndpointMap witness logic so a commissioned bridge preserves the recorded
commissionedAt witness when refusal is cleared.
---
Nitpick comments:
In `@tests/test_bridge_client.py`:
- Around line 787-790: Remove the unused fake binding from the _recovering call
in this test, retaining only the client value that is read and preserving the
existing overrides configuration.
- Around line 1352-1372: Update the _sizes test helper to execute scenario
through the module’s run helper instead of calling asyncio.run directly,
preserving the existing coroutine setup and assertions.
In `@tests/test_export_bridge.py`:
- Around line 1415-1479: Update both tests,
test_a_CORRECTION_folds_into_the_snapshot_it_just_pushed and
test_a_correction_is_gated_on_a_LIVE_client_like_every_other_push, to accept
pytest’s monkeypatch fixture and use monkeypatch.setattr on the shared
handler.dispatch instead of manual save/restore with try/finally. Use the same
export_handlers access style in both tests, matching the neighboring
bridge_mod.export_handlers usage, and remove the real_dispatch and restoration
boilerplate.
In `@tests/test_export_wiring.py`:
- Around line 631-660: Update both tests,
test_the_re_issues_are_a_minute_apart_not_back_to_back and
test_giving_up_says_so_once_and_names_the_consequence, to accept plugin_mod and
derive loop counts from plugin_mod.RESUBSCRIBE_TICKS and
plugin_mod.MAX_RESUBSCRIBE_ATTEMPTS instead of hard-coded 4 and 4 * 10 values.
Preserve the existing assertions and timing semantics while ensuring the tests
reach the configured reissue and give-up thresholds.
In `@tests/test_plugin_behaviour.py`:
- Around line 562-563: Rename the test function
test_menu_export_fabric_backup_archives_the_BRIDGE_storage_too to the
Pylint-compliant lowercase form
test_menu_export_fabric_backup_archives_the_bridge_storage_too, unless the
repository configuration explicitly allows uppercase acronyms.
🪄 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: 5d9fc004-d662-45a7-9353-18b7c990b7a4
📒 Files selected for processing (24)
bridge-node/src/endpoint-map.tsbridge-node/src/node.tsbridge-node/src/storage.tsbridge-node/src/ws-server.tsbridge-node/test/endpoint-map.test.tsbridge-node/test/main.test.tsbridge-node/test/persistence.test.tsbridge-node/test/protocol.test.tsbridge-node/test/storage.test.tsdocs/BRIDGE_PROTOCOL.mddocs/HANDOVER.mddocs/PRD-indigo-matter-export.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xmlindigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.pyindigo-matter.indigoPlugin/Contents/Server Plugin/bridge_protocol.pyindigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pytests/fakes.pytests/test_bridge_client.pytests/test_export_bridge.pytests/test_export_menu.pytests/test_export_wiring.pytests/test_plugin_behaviour.py
🚧 Files skipped from review as they are similar to previous changes (9)
- indigo-matter.indigoPlugin/Contents/Info.plist
- indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_protocol.py
- indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml
- bridge-node/test/protocol.test.ts
- tests/fakes.py
- bridge-node/src/ws-server.ts
- docs/BRIDGE_PROTOCOL.md
- tests/test_export_menu.py
- indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py
| chmodSync(dir, 0o500); | ||
| try { | ||
| store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); | ||
| assert.equal(store.warnings.length, 1, "the failed write must warn first"); | ||
| } finally { | ||
| chmodSync(dir, 0o700); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two chmod-based write-failure tests have no effective-uid guard. Both tests use chmodSync(dir, 0o500) to force a write failure. Root bypasses the mode bits, so the write succeeds, no warning is recorded, and the "must warn first" assertion fails. bridge-node/test/persistence.test.ts already guards the equivalent tests with process.getuid?.() === 0.
bridge-node/test/endpoint-map.test.ts#L330-L336: take the test context argument in "clears the persistence warnings a successful discard has settled" and skip whenprocess.getuid?.() === 0, before thechmodSyncat Line 330.bridge-node/test/endpoint-map.test.ts#L352-L358: apply the same skip in "drops a stale warning rather than carrying it across a re-read", before thechmodSyncat Line 352.
📍 Affects 1 file
bridge-node/test/endpoint-map.test.ts#L330-L336(this comment)bridge-node/test/endpoint-map.test.ts#L352-L358
🤖 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 `@bridge-node/test/endpoint-map.test.ts` around lines 330 - 336, Guard both
chmod-based write-failure tests in bridge-node/test/endpoint-map.test.ts:330-336
and bridge-node/test/endpoint-map.test.ts:352-358 by accepting the test context
argument in “clears the persistence warnings a successful discard has settled”
and “drops a stale warning rather than carrying it across a re-read,” then
skipping when process.getuid?.() === 0 before each chmodSync call.
| - **M11:** the rebuild menu gated only on `client.connected`. Run against a | ||
| healthy node it silently discarded the retained endpoint-number allocations of | ||
| every non-live export — the ones that make re-adding a device restore the same | ||
| accessory. It now requires a refusal to be in force, and the dialog says so. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify which component preserves endpoint numbers.
matter.js owns persisted endpoint-number allocation. endpoint-map.json is an independent report-only witness. Rebuilding the witness can discard non-live map entries, but it does not discard the allocations that preserve a device's endpoint after re-addition.
Replace “retained endpoint-number allocations” with “retained endpoint-map entries” or name the matter.js allocation store. This prevents operators from treating rebuild_endpoint_map as an endpoint-allocation repair.
Based on the report-only endpoint-map rule and the architecture contract in docs/PRD-indigo-matter-export.md, Lines 108-116 and 219-221.
🤖 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 `@docs/HANDOVER.md` around lines 101 - 104, Update the M11 documentation to
replace “retained endpoint-number allocations” with “retained endpoint-map
entries,” clarifying that rebuilding the witness can discard non-live map
entries but does not affect matter.js endpoint-number allocations or endpoint
restoration after re-addition.
Source: Coding guidelines
| def test_a_rebuild_whose_re_attach_FAILS_is_still_a_completed_rebuild(self, mock_logger): | ||
| """⊗ The node writes the map and stops refusing BEFORE it answers us. | ||
|
|
||
| Letting a re-attach failure after that propagate made the menu report | ||
| "the bridge node is unchanged and still refusing to export" — both | ||
| clauses false — leave `recovery` set so every state push went on being | ||
| dropped, and invite the user to repeat an operation that duplicates | ||
| accessories in every paired ecosystem. | ||
| """ | ||
| async def scenario(): | ||
| answers = {bridge_protocol.CMD_ATTACH: error_response( | ||
| bridge_protocol.ERR_ENDPOINT_MAP_INVALID, "map unreadable")} | ||
|
|
||
| def responder(frame): | ||
| command = frame["command"] | ||
| if command == bridge_protocol.CMD_REBUILD_ENDPOINT_MAP: | ||
| # The rebuild worked; the re-attach after it does not. | ||
| answers[bridge_protocol.CMD_ATTACH] = error_response( | ||
| bridge_protocol.ERR_INTERNAL, "node is busy") | ||
| body = {**RESPONSES, **answers}.get(command) | ||
| return [{**body, "message_id": frame["message_id"]}] | ||
|
|
||
| fake = _fake(responder=responder) | ||
| client = _client(mock_logger, fake) | ||
| task = asyncio.create_task(client.run()) | ||
| await settle(lambda: client.recovery) | ||
|
|
||
| status = await client.rebuild_endpoint_map() | ||
|
|
||
| assert status is not None, "the rebuild's own report, not an exception" | ||
| assert not client.recovery, ( | ||
| "the node has stopped refusing; leaving this set drops every push") | ||
| assert not client.attached | ||
| assert "re-attaching was refused" in logged(mock_logger, "warning") | ||
| await client.close() | ||
| task.cancel() | ||
| run(scenario()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the exception type raised on a request timeout and what rebuild_endpoint_map catches.
rg -n -C10 -P '\basync def _request_frame\s*\(' --glob '*.py'
rg -n -C6 -P 'TimeoutError|_triage_rebuilt_reattach' --glob '*.py'Repository: simons-plugins/indigo-matter
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -u
echo "Files:"
git ls-files | rg '(^|/)(tests/test_bridge_client|bridge_client|fakes)\.py$|py$' | sed -n '1,120p'
echo
echo "Search relevant symbols:"
rg -n -C4 'rebuild_endpoint_map|attach\(\)|_triage_rebuilt_reattach|BridgeProtocolError|_request_frame|CMD_ATTACH|ERR_INTERNAL' . --glob '*.py' | sed -n '1,240p'Repository: simons-plugins/indigo-matter
Length of output: 26770
🏁 Script executed:
#!/bin/bash
set -u
echo "bridge_client.py outline:"
ast-grep outline 'indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py' --view compact | sed -n '1,220p'
echo
echo "bridge_client.py relevant sections:"
sed -n '460,550p' 'indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py' | cat -n
sed -n '130,175p' 'indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py' | cat -n
sed -n '260,290p' 'indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py' | cat -n
echo
echo "ws_json_client.py request timeout handling:"
sed -n '440,510p' 'indigo-matter.indigoPlugin/Contents/Server Plugin/ws_json_client.py' | cat -nRepository: simons-plugins/indigo-matter
Length of output: 13956
Cover re-attach timeout in rebuild_endpoint_map.
_request_frame raises asyncio.TimeoutError when an attach answer does not arrive; this test only covers BridgeProtocolError from an error-frame response. A timeout after a successful rebuild should still clear recovery and report the rebuild, not surface as an exception that makes menuRebuildEndpointMap say “the bridge node is unchanged and still refusing.”
🤖 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_bridge_client.py` around lines 840 - 876, Extend
rebuild_endpoint_map to handle asyncio.TimeoutError from the re-attach request
the same way as a BridgeProtocolError after a successful rebuild: clear
client.recovery, preserve and return the rebuild status, and log the re-attach
refusal warning instead of propagating the exception. Update the test scenario
around rebuild_endpoint_map to simulate a timed-out attach response and verify
these outcomes.
1 participant
Summary
Milestone E5 (PRD's highest-risk correctness requirement) + the late-#124-review carry-overs.
Node:
endpoint-map.jsonbeside identity (outside matter.js storage — survives factory_reset), drift detector on every reconcile/upsert (report-only per §4.3,driftCheckednow real), refuse-to-start as a protocol state (get_pairing answers; attach refused;rebuild_endpoint_mapexits it), §3.9–3.11 implemented (ServerNode.erase()with both preserve flavours), all 5 remaining fixtures graduated — pending is empty and both suites assert it.Plugin:
pendingReplaceAllpersisted + discharged on reconnect (fixes orphaned-accessories-forever, XAC7), diff against last-pushed snapshot (kills unbounded ramp drift),on_commandon a single-worker executor, bounded re-subscribe watchdog,start()gated during un-export, fabric_backup archives the bridge dir (restore deliberately manual until E7's stop seam),whiteLevel or 100falsy-zero fixed.Notable: matter.js
erase()leaves a ref'd timerclose()never clears — without the ordered-shutdown fix a clean stop after factory reset exits 1 and launchd reads success as a crash.Verification
Python 1978, node 291/0 skipped, pylint 9.38, drift/diff mutations verified failing. PluginVersion 2026.7.30, bridge-node 0.4.0.
Full XAC5 jarvis script (reboot + forced-drift + refuse-to-start + storage-loss sequence) in the PR discussion after review.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
Summary by CodeRabbit
New Features
Bug Fixes