feat(export): E0 bridge-node skeleton (validation gate) - #119
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds a standalone ESM Matter bridge node with CLI configuration, persistent identity storage, Matter commissioning, a loopback WebSocket protocol, lifecycle handling, and comprehensive unit and integration tests. ChangesBridge node
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
simons-plugins
added a commit
that referenced
this pull request
Aug 4, 2026
Blockers - Guard double-open before touching DeviceCommissioner. matter.js 0.17.8's allowEnhancedCommissioning installs the new PASE commissioner and only then throws on a double-open, so a refusal that reached the stack invalidated the code the user was already holding. - Expiry timer never rethrows: sync and async failures from endCommissioning() racing close() are caught and logged. - SIGTERM: escape-hatch timer armed before the awaits, names the pending close and exits 1 (a forced exit is not a clean shutdown); ws and bridge close in separate try blocks so a ws failure cannot skip the Matter close. - Own the signals outright: BridgeNode.start() sets runtime.signals=false, so matter.js's ProcessManager no longer installs handlers that run first and tear the ServerNode down concurrently with our ordered shutdown. - Identity file: atomic write (temp in the same dir + rename, temp cleaned up on failure), a log line naming which branch was taken, ENOENT distinguished from corrupt, and a tracked TODO that E1 must refuse to start rather than regenerate. - Process-level uncaughtException/unhandledRejection handlers log with stack and exit 1 for launchd to restart; main().catch and config-parse failures route through the same log (USAGE on a parse error). - Serialize frames per socket. This was a live bug: open_commissioning_window genuinely awaits, so a pipelined get_pairing could overtake it, breaking the receipt-order guarantee of BRIDGE_PROTOCOL Section 1. - Implement window_closed (Sections 3.8/5) through an event seam on BridgeFacade, with the double announcement on expiry deduped. Hardening - durationSeconds bounded to Matter's 180-900s; ours is the only timer, since DeviceCommissioner builds its STANDARD_COMMISSIONING_TIMEOUT timer but never starts it. - One shared describeError/describeErrorWithStack; window-open failures are now logged at all, with the stack node-side and the message on the wire. - Per-phase startup framing so a launchd log says which step failed. - --mdns-interface validated against os.networkInterfaces() before matter.js silently advertises on nothing. - fabricsChanged handler cannot throw into matter.js's observable. - 0 added to INVALID_PASSCODES; endpointIdFor aliased to uniqueIdFor; terminate() on shutdown; socket error listener before the handshake write; not_attached checked before unknown_command; binary frames dropped. - Unknown flags reported as unknown rather than as missing a value. Tests: 27 -> 68 - Window bookkeeping extracted to CommissioningWindow with injected timer and clock, unit-tested across double-open, expiry, commission-complete, clear and throwing callbacks. - Protocol: throwing facade -> internal, ProtocolError passthrough, garbage frames, same-socket re-attach, pipelined ordering (verified failing before the fix), window_closed emission and drop-when-unattached. - Golden fixtures bound to protocol.ts types via a satisfies mirror module, plus the missing 4th get_pairing state. - Fixed `npm test`'s fixture copy, which nested on re-runs and so served stale golden frames forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
Adds `bridge-node/` — the npm package `indigo-matter-bridge`, a TypeScript matter.js device-role node that ADR-0006 makes the outbound twin of the existing controller. It lives alongside the plugin bundle, not inside it: the Indigo plugin stays pure Python and never links matter.js. E0 scope (PRD-indigo-matter-export.md §9): - ServerNode with an Aggregator endpoint at EP1 and one hard-coded bridged OnOff Plug-in Unit child at EP2 (`indigo-999001`), carrying Bridged Device Basic Information with a UniqueID derived from the Indigo device id. - Commissioning passcode and 12-bit discriminator randomised on first run and persisted to `identity.json`, kept outside matter.js's storage context so a later factory reset cannot scramble our identity. The spec's trivial passcode list is enforced; the matter.js example defaults are never used. - Storage defaults to ~/Library/Application Support/com.simons-plugins.indigo-matter/bridge-node/ rather than ~/.matter, overridable with --storage-path. Matter port (--matter-port, default 5540) and mDNS interface pinning (--mdns-interface) are CLI arguments. - Loopback WebSocket server on 127.0.0.1 (--ws-port, default 5581) implementing the BRIDGE_PROTOCOL.md envelope for the E0 command subset: the bare handshake frame, attach (with version_mismatch, not_attached, the 10s unattached-socket timeout and single-client supersede), get_status, get_pairing and open_commissioning_window. Endpoint CRUD returns unknown_command; the dispatch table is where E1 slots it in. - Clean SIGTERM/SIGINT shutdown that lets matter.js release its storage lock instead of racing it to process.exit. Tests: 27 under node's built-in runner, exercising the real ws-server against a stubbed bridge — no live Matter stack required — plus golden frames in test/fixtures/e0-frames.json per BRIDGE_PROTOCOL §7. One API deviation worth recording: matter.js 0.17.8's AdministratorCommissioning.openCommissioningWindow asserts a remote authenticated session, so it cannot be invoked from an offline agent. open_commissioning_window drives DeviceCommissioner directly instead; the cluster's windowStatus/adminFabricIndex attributes therefore do not reflect a locally-opened window, which E7 should close. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
Blockers - Guard double-open before touching DeviceCommissioner. matter.js 0.17.8's allowEnhancedCommissioning installs the new PASE commissioner and only then throws on a double-open, so a refusal that reached the stack invalidated the code the user was already holding. - Expiry timer never rethrows: sync and async failures from endCommissioning() racing close() are caught and logged. - SIGTERM: escape-hatch timer armed before the awaits, names the pending close and exits 1 (a forced exit is not a clean shutdown); ws and bridge close in separate try blocks so a ws failure cannot skip the Matter close. - Own the signals outright: BridgeNode.start() sets runtime.signals=false, so matter.js's ProcessManager no longer installs handlers that run first and tear the ServerNode down concurrently with our ordered shutdown. - Identity file: atomic write (temp in the same dir + rename, temp cleaned up on failure), a log line naming which branch was taken, ENOENT distinguished from corrupt, and a tracked TODO that E1 must refuse to start rather than regenerate. - Process-level uncaughtException/unhandledRejection handlers log with stack and exit 1 for launchd to restart; main().catch and config-parse failures route through the same log (USAGE on a parse error). - Serialize frames per socket. This was a live bug: open_commissioning_window genuinely awaits, so a pipelined get_pairing could overtake it, breaking the receipt-order guarantee of BRIDGE_PROTOCOL Section 1. - Implement window_closed (Sections 3.8/5) through an event seam on BridgeFacade, with the double announcement on expiry deduped. Hardening - durationSeconds bounded to Matter's 180-900s; ours is the only timer, since DeviceCommissioner builds its STANDARD_COMMISSIONING_TIMEOUT timer but never starts it. - One shared describeError/describeErrorWithStack; window-open failures are now logged at all, with the stack node-side and the message on the wire. - Per-phase startup framing so a launchd log says which step failed. - --mdns-interface validated against os.networkInterfaces() before matter.js silently advertises on nothing. - fabricsChanged handler cannot throw into matter.js's observable. - 0 added to INVALID_PASSCODES; endpointIdFor aliased to uniqueIdFor; terminate() on shutdown; socket error listener before the handshake write; not_attached checked before unknown_command; binary frames dropped. - Unknown flags reported as unknown rather than as missing a value. Tests: 27 -> 68 - Window bookkeeping extracted to CommissioningWindow with injected timer and clock, unit-tested across double-open, expiry, commission-complete, clear and throwing callbacks. - Protocol: throwing facade -> internal, ProtocolError passthrough, garbage frames, same-socket re-attach, pipelined ordering (verified failing before the fix), window_closed emission and drop-when-unattached. - Golden fixtures bound to protocol.ts types via a satisfies mirror module, plus the missing 4th get_pairing state. - Fixed `npm test`'s fixture copy, which nested on re-runs and so served stale golden frames forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
simons-plugins
force-pushed
the
feat/e0-bridge-node
branch
from
August 4, 2026 18:43
53399f1 to
0c7d642
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
bridge-node/test/storage.test.ts (2)
154-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the persisted file mode.
writeIdentitysetsmode: 0o600because the file holds the setup passcode. No test locks that in, so a later refactor can widen it silently. Add the assertion to this atomic-write test.💚 Proposed test addition
+import { chmodSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs";it("writes atomically and leaves no temp file behind", () => { const dir = scratch(); try { loadOrCreateIdentity(dir); assert.deepEqual(readdirSync(dir), ["identity.json"]); + if (process.platform !== "win32") { + assert.equal(statSync(join(dir, "identity.json")).mode & 0o777, 0o600); + } } finally { rmSync(dir, { recursive: true, force: true }); } });🤖 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/storage.test.ts` around lines 154 - 162, Update the atomic-write test around loadOrCreateIdentity to also inspect identity.json's filesystem permissions and assert its mode is 0o600, while preserving the existing directory-content and cleanup assertions.
190-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a short persisted
installId.This test only uses a minted UUID, so
serialNumberForandnodeUniqueIdForalways differ. It does not cover a persistedinstallIdof 16 or fewer characters, where both functions return the same value. See the comment on bridge-node/src/storage.ts Lines 68-83. Add the case together with the validation fix.🤖 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/storage.test.ts` around lines 190 - 205, Extend the test around serialNumberFor and nodeUniqueIdFor to persist an installId of 16 or fewer characters, then assert both generated values remain Matter-valid and distinct. Update the corresponding storage logic near loadOrCreateIdentity so nodeUniqueIdFor does not reuse a short persisted installId as the serial number, while preserving the existing behavior for valid longer identifiers.bridge-node/src/storage.ts (1)
116-118: 📐 Maintainability & Code Quality | 🔵 TrivialTODO(E1) tracks a pairing-loss risk.
The current branch regenerates a corrupt-but-present identity, which un-pairs every ecosystem. The TODO records the required refuse-to-start behavior for E1.
Do you want me to open an issue for the refuse-to-start branch so the E0 decision is not carried forward silently?
🤖 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 116 - 118, Update the identity-loading flow in storage.ts so a corrupt-but-present identity refuses to start instead of regenerating and minting a new identity; only the missing-file branch may create one. Preserve the existing pairing state and surface the failure through the established startup error path, and remove or update the TODO once this E1 behavior is implemented.bridge-node/src/main.ts (3)
73-80: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider closing the Matter node when the WebSocket listen fails.
bridge.start()on line 69 brings the Matter node online and takes the storage lock. Ifws.listen()then fails, for example withEADDRINUSEonconfig.wsPort,main()rejects into the handler on line 136 and callsprocess.exit(1)without callingbridge.close().The OS releases the file handles on exit, so this does not lose data. A matter.js storage lock artifact can survive on disk and make the next start noisier.
♻️ Proposed refactor
- await phase(`protocol WS listen failed (port ${config.wsPort})`, () => ws.listen()); + try { + await phase(`protocol WS listen (port ${config.wsPort})`, () => ws.listen()); + } catch (error) { + try { + await bridge.close(); + } catch (closeError) { + log(`Error closing Matter node after failed WS listen: ${describeErrorWithStack(closeError)}`); + } + throw 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/main.ts` around lines 73 - 80, Ensure the startup failure path after bridge.start() cleans up the Matter node before main() exits. Wrap the BridgeWsServer creation/listen phase around ws.listen() with cleanup that calls bridge.close() when listening fails, while preserving the existing error propagation and normal successful startup behavior.
127-141: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
process.exitcan truncate the final log line.
console.logwrites to a pipe asynchronously on POSIX. launchd captures stdout through a pipe.process.exiton lines 129, 133, and 140 therefore can discard the diagnostic that was just written, which is the one message needed to explain the exit.Line 98 in
shutdownhas the same exposure. If you want these messages to survive, write them withfs.writeSync(1, ...)before exiting, or set the exit code and let the process end naturally.🤖 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 127 - 141, Update the fatal-exit handlers in the uncaughtException, unhandledRejection, and main().catch flows, plus shutdown, to ensure diagnostic output is synchronously written before termination. Use fs.writeSync on stdout for these final messages, or replace process.exit with setting the exit code and allowing natural shutdown, while preserving the existing error text and exit status.
44-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
parsedsoconfigkeeps itsBridgeConfigtype.
let parsed;without an annotation gives an evolvingany.configon line 56 then inheritsany, and the later reads ofconfig.mdnsInterface,config.storagePath,config.matterPort, andconfig.wsPortlose type checking against theBridgeConfigcontract inbridge-node/src/config.ts.An explicit union annotation restores the check and still narrows correctly, because
process.exitreturnsnever.♻️ Proposed refactor
-import { assertMdnsInterface, parseArgs, USAGE } from "./config.js"; +import { assertMdnsInterface, type BridgeConfig, parseArgs, USAGE } from "./config.js";async function main(): Promise<void> { - let parsed; + let parsed: BridgeConfig | "help"; try { parsed = parseArgs(process.argv.slice(2));🤖 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 44 - 56, Annotate the `parsed` variable in the argument-parsing flow with the explicit union of `BridgeConfig` and the `"help"` sentinel. Keep the existing `parseArgs` try/catch and `parsed === "help"` narrowing unchanged so `config` is inferred as `BridgeConfig` after the early return and its property accesses remain type-checked.bridge-node/src/ws-server.ts (1)
199-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
sendErrorfor the missing-command reply.Line 200 builds the error frame inline.
sendErrorbuilds the same shape. Route this reply throughsendErrorso all error frames stay identical.♻️ Proposed refactor
if (typeof command !== "string") { - this.send(socket, { message_id: messageId, error_code: ErrorCode.malformedArgs, details: "Missing command" }); + this.sendError(socket, messageId, ErrorCode.malformedArgs, "Missing command"); return; }🤖 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 199 - 202, Update the missing-command branch in the WebSocket request handling to call the existing sendError method instead of constructing the error frame inline with this.send. Pass the socket, messageId, malformedArgs error code, and “Missing command” details so the reply remains identical while using the shared error path.bridge-node/test/protocol.test.ts (1)
175-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the shared
StubBridgeflags in afinallyblock.These two tests set
bridge.commissionedandbridge.windowOpen, then reset them on the last lines of the test body. If any assertion above the reset fails, the reset never runs. The sharedbridgeinstance then carries the commissioned state into later tests in this file and produces cascading failures that hide the original one.The same file already guards
openWindowError(lines 256-258, 273-275) anddelayOpenWindowMs(lines 332-334) withtry/finally. Apply the same pattern here.♻️ Proposed refactor
it("nulls the codes once commissioned with no window open", async () => { bridge.commissioned = true; - const client = await connect(); - await attach(client); - const response = await client.request(golden.get_pairing_commissioned.request); - assert.deepEqual(response, golden.get_pairing_commissioned.response); - - const result = response.result as Record<string, unknown>; - assert.equal(result.windowOpen, false); - assert.equal(result.manualPairingCode, null); - assert.equal(result.windowExpiresAt, null); - bridge.commissioned = false; - client.close(); + try { + const client = await connect(); + await attach(client); + const response = await client.request(golden.get_pairing_commissioned.request); + assert.deepEqual(response, golden.get_pairing_commissioned.response); + + const result = response.result as Record<string, unknown>; + assert.equal(result.windowOpen, false); + assert.equal(result.manualPairingCode, null); + assert.equal(result.windowExpiresAt, null); + client.close(); + } finally { + bridge.commissioned = false; + } }); it("reports codes and a non-null expiry while commissioned with a window open", async () => { bridge.commissioned = true; bridge.windowOpen = true; - const client = await connect(); - await attach(client); - const response = await client.request(golden.get_pairing_commissioned_window_open.request); - assert.deepEqual(response, golden.get_pairing_commissioned_window_open.response); - - const result = response.result as Record<string, unknown>; - assert.equal(result.commissioned, true); - assert.equal(result.windowOpen, true); - assert.notEqual(result.windowExpiresAt, null); - assert.notEqual(result.manualPairingCode, null); - bridge.commissioned = false; - bridge.windowOpen = false; - client.close(); + try { + const client = await connect(); + await attach(client); + const response = await client.request(golden.get_pairing_commissioned_window_open.request); + assert.deepEqual(response, golden.get_pairing_commissioned_window_open.response); + + const result = response.result as Record<string, unknown>; + assert.equal(result.commissioned, true); + assert.equal(result.windowOpen, true); + assert.notEqual(result.windowExpiresAt, null); + assert.notEqual(result.manualPairingCode, null); + client.close(); + } finally { + bridge.commissioned = false; + bridge.windowOpen = false; + } });🤖 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 175 - 206, Wrap the stateful setup and assertions in both tests around the shared bridge instance in try/finally blocks, and move the resets of bridge.commissioned and bridge.windowOpen into finally so they always execute. Preserve the existing assertions and client cleanup, ensuring each test restores the shared StubBridge flags even when an assertion or request fails.bridge-node/test/fixtures.test.ts (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test title with the three asserted states.
The title says "all four" states. The assertion at line 48 checks three states, and the comment explains that the fourth state cannot exist. Rename the title so a failure message does not contradict the assertion.
♻️ Proposed refactor
- it("covers all four §3.7 pairing states", () => { + it("covers the three reachable §3.7 pairing states", () => {🤖 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/fixtures.test.ts` at line 39, Rename the test case description in the “§3.7 pairing states” test to state that it covers the three asserted states, matching the assertions and the explanation that the fourth state cannot exist.
🤖 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 66-80: Update the phase labels passed to phase in the identity
load, Matter node start, and WebSocket listen calls to remove the trailing
“failed,” using noun-phrase labels consistent with the existing label near line
61. Keep phase’s “Startup failed —” formatting unchanged.
- Around line 95-118: Update the shutdown flow around the `escapeHatch` timer
and async close sequence: clear `escapeHatch` after `bridge.close()` completes
and before logging shutdown completion, so a completed shutdown cannot trigger
the timer later. Keep the existing exit-1 behavior for stalls during
`ws.close()` or `bridge.close()`, but make the timeout branch exit with code 0
when `pending` is `"event loop drain"`.
In `@bridge-node/src/storage.ts`:
- Around line 68-83: Update isUsableIdentity to require candidate.installId to
match the UUID format produced by randomUUID(), rather than accepting any
non-empty string; preserve the existing passcode and discriminator validation so
persisted identities always derive distinct serialNumberFor and nodeUniqueIdFor
values.
In `@bridge-node/src/window.ts`:
- Around line 86-92: Update CommissioningWindow.open to validate durationSeconds
at the boundary before computing expiresAt or scheduling the timer, accepting
only finite values in the inclusive 180–900 second range and rejecting all
others. Ensure direct callers cannot create immediate or non-finite expiry
states, while preserving the existing open behavior for valid durations.
---
Nitpick comments:
In `@bridge-node/src/main.ts`:
- Around line 73-80: Ensure the startup failure path after bridge.start() cleans
up the Matter node before main() exits. Wrap the BridgeWsServer creation/listen
phase around ws.listen() with cleanup that calls bridge.close() when listening
fails, while preserving the existing error propagation and normal successful
startup behavior.
- Around line 127-141: Update the fatal-exit handlers in the uncaughtException,
unhandledRejection, and main().catch flows, plus shutdown, to ensure diagnostic
output is synchronously written before termination. Use fs.writeSync on stdout
for these final messages, or replace process.exit with setting the exit code and
allowing natural shutdown, while preserving the existing error text and exit
status.
- Around line 44-56: Annotate the `parsed` variable in the argument-parsing flow
with the explicit union of `BridgeConfig` and the `"help"` sentinel. Keep the
existing `parseArgs` try/catch and `parsed === "help"` narrowing unchanged so
`config` is inferred as `BridgeConfig` after the early return and its property
accesses remain type-checked.
In `@bridge-node/src/storage.ts`:
- Around line 116-118: Update the identity-loading flow in storage.ts so a
corrupt-but-present identity refuses to start instead of regenerating and
minting a new identity; only the missing-file branch may create one. Preserve
the existing pairing state and surface the failure through the established
startup error path, and remove or update the TODO once this E1 behavior is
implemented.
In `@bridge-node/src/ws-server.ts`:
- Around line 199-202: Update the missing-command branch in the WebSocket
request handling to call the existing sendError method instead of constructing
the error frame inline with this.send. Pass the socket, messageId, malformedArgs
error code, and “Missing command” details so the reply remains identical while
using the shared error path.
In `@bridge-node/test/fixtures.test.ts`:
- Line 39: Rename the test case description in the “§3.7 pairing states” test to
state that it covers the three asserted states, matching the assertions and the
explanation that the fourth state cannot exist.
In `@bridge-node/test/protocol.test.ts`:
- Around line 175-206: Wrap the stateful setup and assertions in both tests
around the shared bridge instance in try/finally blocks, and move the resets of
bridge.commissioned and bridge.windowOpen into finally so they always execute.
Preserve the existing assertions and client cleanup, ensuring each test restores
the shared StubBridge flags even when an assertion or request fails.
In `@bridge-node/test/storage.test.ts`:
- Around line 154-162: Update the atomic-write test around loadOrCreateIdentity
to also inspect identity.json's filesystem permissions and assert its mode is
0o600, while preserving the existing directory-content and cleanup assertions.
- Around line 190-205: Extend the test around serialNumberFor and
nodeUniqueIdFor to persist an installId of 16 or fewer characters, then assert
both generated values remain Matter-valid and distinct. Update the corresponding
storage logic near loadOrCreateIdentity so nodeUniqueIdFor does not reuse a
short persisted installId as the serial number, while preserving the existing
behavior for valid longer identifiers.
🪄 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: d13862c1-e183-4199-bf08-7c3c1dab348c
⛔ Files ignored due to path filters (1)
bridge-node/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.gitignorebridge-node/package.jsonbridge-node/src/config.tsbridge-node/src/main.tsbridge-node/src/node.tsbridge-node/src/protocol.tsbridge-node/src/storage.tsbridge-node/src/window.tsbridge-node/src/ws-server.tsbridge-node/test/client.tsbridge-node/test/config.test.tsbridge-node/test/fixture-shapes.tsbridge-node/test/fixtures.test.tsbridge-node/test/fixtures/e0-frames.jsonbridge-node/test/protocol.test.tsbridge-node/test/storage.test.tsbridge-node/test/stub-bridge.tsbridge-node/test/timeout.test.tsbridge-node/test/window.test.tsbridge-node/tsconfig.jsonbridge-node/tsconfig.test.jsonindigo-matter.indigoPlugin/Contents/Info.plist
Comment on lines
+66
to
+80
| const identity = await phase("identity load failed", () => loadOrCreateIdentity(config.storagePath, log)); | ||
|
|
||
| const bridge = new BridgeNode(config, identity, bridgeVersion, log); | ||
| await phase(`Matter node start failed (matter port ${config.matterPort}, storage ${config.storagePath})`, () => | ||
| bridge.start(), | ||
| ); | ||
|
|
||
| const ws = new BridgeWsServer({ | ||
| port: config.wsPort, | ||
| bridge, | ||
| bridgeVersion, | ||
| matterJsVersion, | ||
| log, | ||
| }); | ||
| await phase(`protocol WS listen failed (port ${config.wsPort})`, () => ws.listen()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicated "failed" from the phase labels.
phase already formats its label as Startup failed — ${name}: ... at line 38. The labels on lines 66, 69, and 80 also end in "failed". The log line then reads Startup failed — identity load failed: .... Line 61 uses the correct noun-phrase form.
♻️ Proposed fix
- const identity = await phase("identity load failed", () => loadOrCreateIdentity(config.storagePath, log));
+ const identity = await phase("identity load", () => loadOrCreateIdentity(config.storagePath, log));
const bridge = new BridgeNode(config, identity, bridgeVersion, log);
- await phase(`Matter node start failed (matter port ${config.matterPort}, storage ${config.storagePath})`, () =>
+ await phase(`Matter node start (matter port ${config.matterPort}, storage ${config.storagePath})`, () =>
bridge.start(),
);
@@
- await phase(`protocol WS listen failed (port ${config.wsPort})`, () => ws.listen());
+ await phase(`protocol WS listen (port ${config.wsPort})`, () => ws.listen());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const identity = await phase("identity load failed", () => loadOrCreateIdentity(config.storagePath, log)); | |
| const bridge = new BridgeNode(config, identity, bridgeVersion, log); | |
| await phase(`Matter node start failed (matter port ${config.matterPort}, storage ${config.storagePath})`, () => | |
| bridge.start(), | |
| ); | |
| const ws = new BridgeWsServer({ | |
| port: config.wsPort, | |
| bridge, | |
| bridgeVersion, | |
| matterJsVersion, | |
| log, | |
| }); | |
| await phase(`protocol WS listen failed (port ${config.wsPort})`, () => ws.listen()); | |
| const identity = await phase("identity load", () => loadOrCreateIdentity(config.storagePath, log)); | |
| const bridge = new BridgeNode(config, identity, bridgeVersion, log); | |
| await phase(`Matter node start (matter port ${config.matterPort}, storage ${config.storagePath})`, () => | |
| bridge.start(), | |
| ); | |
| const ws = new BridgeWsServer({ | |
| port: config.wsPort, | |
| bridge, | |
| bridgeVersion, | |
| matterJsVersion, | |
| log, | |
| }); | |
| await phase(`protocol WS listen (port ${config.wsPort})`, () => ws.listen()); |
🤖 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 66 - 80, Update the phase labels passed
to phase in the identity load, Matter node start, and WebSocket listen calls to
remove the trailing “failed,” using noun-phrase labels consistent with the
existing label near line 61. Keep phase’s “Startup failed —” formatting
unchanged.
Comment on lines
+95
to
+118
| let pending = "protocol WS close"; | ||
| const escapeHatch = setTimeout(() => { | ||
| log(`Shutdown stalled at: ${pending}; forcing exit`); | ||
| process.exit(1); | ||
| }, SHUTDOWN_ESCAPE_MS); | ||
| escapeHatch.unref(); | ||
|
|
||
| void (async () => { | ||
| // Separate try blocks: a failing WS close must not skip the Matter | ||
| // close, which is what releases the storage lock. | ||
| try { | ||
| await ws.close(); | ||
| } catch (error) { | ||
| log(`Error closing protocol WS: ${describeErrorWithStack(error)}`); | ||
| } | ||
| pending = "Matter node close"; | ||
| try { | ||
| await bridge.close(); | ||
| } catch (error) { | ||
| log(`Error closing Matter node: ${describeErrorWithStack(error)}`); | ||
| } | ||
| pending = "event loop drain"; | ||
| log("Shutdown complete"); | ||
| })(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear escapeHatch after a completed shutdown, and separate a drain overrun from a stalled close.
escapeHatch is never cleared. After both closes succeed and line 117 logs "Shutdown complete", the timer stays armed. If any handle keeps the event loop alive for the remainder of SHUTDOWN_ESCAPE_MS, the callback fires and runs process.exit(1).
The log then reads Shutdown stalled at: event loop drain; forcing exit and the process exits non-zero after a clean shutdown. The file header states that a clean shutdown must not look like a crash to launchd, so this inverts the intended signal. The current code cannot distinguish a genuinely stalled ws.close() or bridge.close() from a slow event-loop drain, because both map to exit code 1.
Clear the timer once the closes finish. If a drain still does not complete, exit 0.
🐛 Proposed fix
let pending = "protocol WS close";
const escapeHatch = setTimeout(() => {
log(`Shutdown stalled at: ${pending}; forcing exit`);
process.exit(1);
}, SHUTDOWN_ESCAPE_MS);
escapeHatch.unref();
void (async () => {
// Separate try blocks: a failing WS close must not skip the Matter
// close, which is what releases the storage lock.
try {
await ws.close();
} catch (error) {
log(`Error closing protocol WS: ${describeErrorWithStack(error)}`);
}
pending = "Matter node close";
try {
await bridge.close();
} catch (error) {
log(`Error closing Matter node: ${describeErrorWithStack(error)}`);
}
- pending = "event loop drain";
+ clearTimeout(escapeHatch);
log("Shutdown complete");
+ // Both closes returned, so this is a clean shutdown. Give the loop
+ // a bounded chance to drain, then exit 0 — a slow drain is not a
+ // crash and launchd must not read it as one.
+ const drain = setTimeout(() => {
+ log("Event loop did not drain; exiting");
+ process.exit(0);
+ }, SHUTDOWN_ESCAPE_MS);
+ drain.unref();
})();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let pending = "protocol WS close"; | |
| const escapeHatch = setTimeout(() => { | |
| log(`Shutdown stalled at: ${pending}; forcing exit`); | |
| process.exit(1); | |
| }, SHUTDOWN_ESCAPE_MS); | |
| escapeHatch.unref(); | |
| void (async () => { | |
| // Separate try blocks: a failing WS close must not skip the Matter | |
| // close, which is what releases the storage lock. | |
| try { | |
| await ws.close(); | |
| } catch (error) { | |
| log(`Error closing protocol WS: ${describeErrorWithStack(error)}`); | |
| } | |
| pending = "Matter node close"; | |
| try { | |
| await bridge.close(); | |
| } catch (error) { | |
| log(`Error closing Matter node: ${describeErrorWithStack(error)}`); | |
| } | |
| pending = "event loop drain"; | |
| log("Shutdown complete"); | |
| })(); | |
| let pending = "protocol WS close"; | |
| const escapeHatch = setTimeout(() => { | |
| log(`Shutdown stalled at: ${pending}; forcing exit`); | |
| process.exit(1); | |
| }, SHUTDOWN_ESCAPE_MS); | |
| escapeHatch.unref(); | |
| void (async () => { | |
| // Separate try blocks: a failing WS close must not skip the Matter | |
| // close, which is what releases the storage lock. | |
| try { | |
| await ws.close(); | |
| } catch (error) { | |
| log(`Error closing protocol WS: ${describeErrorWithStack(error)}`); | |
| } | |
| pending = "Matter node close"; | |
| try { | |
| await bridge.close(); | |
| } catch (error) { | |
| log(`Error closing Matter node: ${describeErrorWithStack(error)}`); | |
| } | |
| clearTimeout(escapeHatch); | |
| log("Shutdown complete"); | |
| // Both closes returned, so this is a clean shutdown. Give the loop | |
| // a bounded chance to drain, then exit 0 — a slow drain is not a | |
| // crash and launchd must not read it as one. | |
| const drain = setTimeout(() => { | |
| log("Event loop did not drain; exiting"); | |
| process.exit(0); | |
| }, SHUTDOWN_ESCAPE_MS); | |
| drain.unref(); | |
| })(); |
🤖 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 95 - 118, Update the shutdown flow
around the `escapeHatch` timer and async close sequence: clear `escapeHatch`
after `bridge.close()` completes and before logging shutdown completion, so a
completed shutdown cannot trigger the timer later. Keep the existing exit-1
behavior for stalls during `ws.close()` or `bridge.close()`, but make the
timeout branch exit with code 0 when `pending` is `"event loop drain"`.
Comment on lines
+68
to
+83
| function isUsableIdentity(value: unknown): value is BridgeIdentity { | ||
| if (typeof value !== "object" || value === null) { | ||
| return false; | ||
| } | ||
| const candidate = value as Partial<BridgeIdentity>; | ||
| return ( | ||
| typeof candidate.installId === "string" && | ||
| candidate.installId.length > 0 && | ||
| typeof candidate.passcode === "number" && | ||
| isValidPasscode(candidate.passcode) && | ||
| typeof candidate.discriminator === "number" && | ||
| Number.isInteger(candidate.discriminator) && | ||
| candidate.discriminator >= 0 && | ||
| candidate.discriminator <= DISCRIMINATOR_MAX | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the installId shape, not only its length.
isUsableIdentity accepts any non-empty installId. If a persisted installId has 16 or fewer dash-stripped characters, serialNumberFor (Line 160) and nodeUniqueIdFor (Line 165) return the same string. node.ts Lines 123-124 then pass equal serialNumber and uniqueId to ServerNode.create, which the comment at Line 156 states Matter forbids.
Require the persisted value to match the UUID that randomUUID() mints, so the derived identifiers always differ.
🐛 Proposed fix to validate the persisted install id
+const INSTALL_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
+
function isUsableIdentity(value: unknown): value is BridgeIdentity {
if (typeof value !== "object" || value === null) {
return false;
}
const candidate = value as Partial<BridgeIdentity>;
return (
typeof candidate.installId === "string" &&
- candidate.installId.length > 0 &&
+ INSTALL_ID_PATTERN.test(candidate.installId) &&
typeof candidate.passcode === "number" &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function isUsableIdentity(value: unknown): value is BridgeIdentity { | |
| if (typeof value !== "object" || value === null) { | |
| return false; | |
| } | |
| const candidate = value as Partial<BridgeIdentity>; | |
| return ( | |
| typeof candidate.installId === "string" && | |
| candidate.installId.length > 0 && | |
| typeof candidate.passcode === "number" && | |
| isValidPasscode(candidate.passcode) && | |
| typeof candidate.discriminator === "number" && | |
| Number.isInteger(candidate.discriminator) && | |
| candidate.discriminator >= 0 && | |
| candidate.discriminator <= DISCRIMINATOR_MAX | |
| ); | |
| } | |
| const INSTALL_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; | |
| function isUsableIdentity(value: unknown): value is BridgeIdentity { | |
| if (typeof value !== "object" || value === null) { | |
| return false; | |
| } | |
| const candidate = value as Partial<BridgeIdentity>; | |
| return ( | |
| typeof candidate.installId === "string" && | |
| INSTALL_ID_PATTERN.test(candidate.installId) && | |
| typeof candidate.passcode === "number" && | |
| isValidPasscode(candidate.passcode) && | |
| typeof candidate.discriminator === "number" && | |
| Number.isInteger(candidate.discriminator) && | |
| candidate.discriminator >= 0 && | |
| candidate.discriminator <= DISCRIMINATOR_MAX | |
| ); | |
| } |
🤖 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 68 - 83, Update isUsableIdentity to
require candidate.installId to match the UUID format produced by randomUUID(),
rather than accepting any non-empty string; preserve the existing passcode and
discriminator validation so persisted identities always derive distinct
serialNumberFor and nodeUniqueIdFor values.
Comment on lines
+86
to
+92
| open(durationSeconds: number, manualPairingCode: string, qrPairingCode: string): Date { | ||
| this.assertClosed(); | ||
| const expiresAt = new Date(this.#now() + durationSeconds * 1000); | ||
| this.#open = { expiresAt, manualPairingCode, qrPairingCode }; | ||
| this.#cancel = this.#schedule(() => this.#expire(), durationSeconds * 1000); | ||
| return expiresAt; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find where the commissioning-window duration is validated or capped.
rg -nP -C4 '\b(durationSeconds|900|MAX_WINDOW|window_duration|duration)\b' bridge-node/src bridge-node/testRepository: simons-plugins/indigo-matter
Length of output: 17573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- protocol bounds ---\n'
sed -n '1,45p' bridge-node/src/protocol.ts
printf '\n--- node open method ---\n'
sed -n '260,330p' bridge-node/src/node.ts
printf '\n--- window implementation ---\n'
sed -n '60,105p' bridge-node/src/window.ts
printf '\n--- call sites for window.open and openCommissioningWindow ---\n'
rg -n '\.(open|openCommissioningWindow)\(' bridge-node/src bridge-node/testRepository: simons-plugins/indigo-matter
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- protocol bounds ---'
sed -n '1,45p' bridge-node/src/protocol.ts
echo
echo '--- node open method ---'
sed -n '260,330p' bridge-node/src/node.ts
echo
echo '--- window implementation ---'
sed -n '60,105p' bridge-node/src/window.ts
echo
echo '--- call sites for window.open and openCommissioningWindow ---'
rg -n '\.(open|openCommissioningWindow)\(' bridge-node/src bridge-node/testRepository: simons-plugins/indigo-matter
Length of output: 8602
Validate durationSeconds where the commissioning window is opened.
ws-server.ts validates the protocol interval, but node.ts still passes its unvalidated durationSeconds through to CommissioningWindow.open(), and that method does not check the value before arming the timer or returning expiresAt.toISOString(). Move the 180..900s validation into this boundary so direct callers cannot create an immediate expiry, a non-finite expiry, or an invalid ISO timestamp.
🤖 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/window.ts` around lines 86 - 92, Update
CommissioningWindow.open to validate durationSeconds at the boundary before
computing expiresAt or scheduling the timer, accepting only finite values in the
inclusive 180–900 second range and rejecting all others. Ensure direct callers
cannot create immediate or non-finite expiry states, while preserving the
existing open behavior for valid durations.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Milestone E0 of the Matter export build (PRD-indigo-matter-export.md §9, ADR-0006/0007): the
indigo-matter-bridgenpm package skeleton — a matter.js device-role node that will let ecosystems see exported Indigo devices.bridge-node/package (TypeScript, ESM, Node ≥22.13):@matter/main/@matter/nodejsexact-pinned at 0.17.8indigo-999001→ endpoint 2), Matter UDP default 5540attach(version_mismatch fails closed, 10s unattached timeout, single-client supersede),get_status,get_pairing,open_commissioning_window; endpoint CRUD deliberately returnsunknown_commanduntil E1BridgeFacadeseam so protocol tests run without a Matter stack; golden-frame JSON fixtures--mdns-interfacepinningVerification
Notable matter.js findings (documented for E7)
StorageService.locationis getter-only at 0.17.8 → storage set viaEnvironment.default.vars.set("storage.path", …)AdministratorCommissioning.openCommissioningWindowrequires a remote actor → local window opening drivesDeviceCommissioner.allowEnhancedCommissioningdirectly; the node owns the window timer (and the cluster path has an upstream bug that wedges the window state on a failed local call)Gate
E0's milestone gate — pairs into Apple Home — is executed on jarvis after this PR review (needs Simon's iPhone for the uncertified-accessory 'Add Anyway' step).
🤖 Generated with Claude Code
https://claude.ai/code/session_01JxqGhP3DcENf68AZK21U4S
Summary by CodeRabbit