fix(bridge-node): adopt a factory reset's own renumbering instead of alarming forever (#140) - #151
Merged
Merged
Conversation
…alarming forever (#140) Two deliberate designs collided: a preserving factory reset kept the endpoint map specifically to REPORT the renumbering the reset causes, and its log line told the user to clear the report with the §3.11 rebuild — which the plugin's M11 gate correctly refuses on a healthy node. Permanent alarm, locked door. Live on jarvis at every attach (indigo-459564566: expected 5, got 2). Fix is issue option (3), node-side only. Both reset sites — §3.10 factory_reset preserveEndpointNumbers:true and the last-fabric self-reset — now void every map entry's number (numberVoid: true); check() silently adopts the next live number for a void entry and clears the marker (even when the numbers happen to match: the marker is what must stop being true). Safe by construction: matter.js is always the allocator (the map is witness-only, even on restore) and both reset sites are reached with an empty fabric set, so no paired ecosystem exists that could hold the old numbers. A never-voided entry still drifts exactly as before — on a >=0.8.0 node surviving drift is a STRONGER signal, and the plugin's drift error now says so, naming the bridge-node version rather than the plugin's (adoption is node-side; a new plugin driving an old node still gets reset drift, and claiming it away would be #132 again). Schema stays v2: every reader takes fields by name, so an old build never looks at the new key — a version bump would guard nothing. No wire change; golden frames untouched. NOT in this PR, by the #141 precedent (never pin an unpublished version): the bridge-node 0.7.0->0.8.0 bump + npm publish + DEFAULT_INSTALL_SPEC pin, which is what actually ships the behaviour to installs. Mutation-verified: adopt-regardless-of-marker fails the non-void-still-drifts test; skipping voidNumbers at noteLastFabricGone fails its test. The reset's duplicate-number fingerprint heals under adoption (tested). Suites: 2265 Python (+1), 396 TS (+13). Version 2026.8.8 -> 2026.8.9. Closes #140 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesReset-aware endpoint renumbering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BridgeNode
participant MatterJS
participant EndpointMapStore
BridgeNode->>MatterJS: clear endpoint allocation during reset
BridgeNode->>EndpointMapStore: mark persisted numbers as void
MatterJS->>BridgeNode: recreate endpoints with new numbers
BridgeNode->>EndpointMapStore: reconcile live endpoints
EndpointMapStore->>EndpointMapStore: adopt numbers and clear markers
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: 1
🤖 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/node.ts`:
- Around line 665-668: Update the last-fabric handling around the commissioning
witness guard so the `#endpointMap.voidNumbers()` call and `#drift` reset
execute even when `identity.commissionedAt` is undefined. Preserve the existing
witness-clear guard for clearing commissioning data, but move only the
endpoint-number invalidation and drift reset after that guard and before
returning from the transition.
🪄 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: f2633922-74f5-4152-8e1a-6371ba7fd65f
📒 Files selected for processing (10)
bridge-node/src/endpoint-map.tsbridge-node/src/node.tsbridge-node/test/endpoint-map.test.tsbridge-node/test/persistence.test.tsdocs/BRIDGE_PROTOCOL.mddocs/HANDOVER.mddocs/PRD-indigo-matter-export.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.pytests/test_export_bridge.py
… a void baseline (#140) The review pass found the two claims-around-the-mechanism defects: - factoryReset discarded voidNumbers()'s return and unconditionally logged "…numbers are now VOID… §3.11 rebuild is not needed" — over a write that may have failed, leaving RAM-only markers that a restart loses, bringing #140's forever-drift back after the user was told otherwise. Same bug class the same function fixed for discard(). Both call sites now capture the result and branch the log; the empty-map no-op (also false) is resolved by gating on size > 0 — nothing to void is not a failure, and a bridge that never exported gets neither line. persist() already leaves #dirty set, so the retry is automatic and the failure line says so. - voidNumbers left #checked true, so between the reset and the first reconcile StatusReport claimed driftChecked: true ("checked, nothing moved" per §4.3) over a baseline the node itself just declared void — the exact over-claim the #dirty doc comment exists to prevent. Every sibling baseline-invalidating op recomputes it; now this one does too, and the plugin's "not checked yet" note becomes the honest post-reset rendering. Also: class comment bounds the marker's lifetime honestly (only LIVE entries adopt; a device offline through the first reconcile keeps its marker until it next exports — still safe, the argument anchors on "no fabric survived the reset", not "at the moment of adoption"); BRIDGE_PROTOCOL's three #140 passages now say "since bridge-node 0.8.0" per the doc's own convention, matching the plugin's message. Three new tests (399 TS): checked-drops-on-void, void-write-failure-reports-not-a-no-op, and the factoryReset truth-telling branch under the suite's chmod fault-injection pattern. Mutation-verified both important fixes fail-before/pass-after. Suites: 2265 Python, 399 TS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #140.
The defect: two deliberate designs colliding
A preserving factory reset keeps the endpoint map specifically to report the renumbering the reset causes, and its log line told the user to clear the report with the §3.11 rebuild — which the plugin's M11 gate correctly refuses on a healthy node. Permanent alarm, locked door. Live on jarvis at every attach:
indigo-459564566: expected 5, got 2.The fix — issue option (3), node-side only
At both reset sites — §3.10
factory_reset preserveEndpointNumbers: trueand the last-fabric self-reset (noteLastFabricGone) — the node now voids every map entry's number (numberVoid: true, per entry).check()silently adopts the next live number for a void entry and clears the marker (even when the numbers happen to match — the marker is what must stop being true).Safe by construction (now in the class comment): matter.js is always the allocator — the map is witness-only, even on the #141 restore path — and both reset sites are reached with an empty fabric set, so no paired ecosystem exists that could still hold the old numbers. Adoption is unobservable outside the node.
The never-auto-repaired rule survives: an entry that was never voided drifts exactly as before, and on a ≥0.8.0 node surviving drift is now a stronger signal. The plugin's drift error says so — naming the bridge-node version, not the plugin's: adoption is node-side, and a new plugin driving the published 0.7.0 node still gets reset drift; claiming it away would be #132's mistake over again.
Schema stays v2: every reader takes fields by name, so an old build reading a file carrying
numberVoidnever looks at the key — a version bump would guard nothing (documented atENDPOINT_MAP_VERSION).No wire change. Golden frames untouched;
StatusReport.driftshape unchanged.NOT in this PR (by the #141 precedent — never pin an unpublished version)
The bridge-node
0.7.0 → 0.8.0bump,npm publish, and theDEFAULT_INSTALL_SPECpin — the step that actually ships this behaviour to installs. jarvis also needs a one-time map edit for its pre-existing stale entry (predates the marker); recipe in HANDOVER §#140 — the safer variant is adding"numberVoid": trueto the stale entries and letting the new node adopt.Tests
voidNumbersatnoteLastFabricGone→ killed.persistence.test.ts"keeps endpoint-map.json by default" — entries now gain the marker after a preserving reset).Version
2026.8.8 → 2026.8.9.🤖 Generated with Claude Code
https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores