fix(menu): rebuild dialog no longer promises duplication it cannot cause (#132) - #148
Conversation
…use (#132) The Rebuild Matter Endpoint Map… dialog warned that rebuilding WILL duplicate accessories in already-paired ecosystems. It cannot: rebuild() adopts the live endpoint numbers and renumbers nothing, so any duplication belongs to the storage-loss event that already happened — observed live 2026-08-06, where post-reset drift made the rebuild the correct and harmless action while the dialog read as though it was about to break the house. The same false claim lived in seven user-facing places, including the attach-refusal log the user reads immediately before opening the dialog. All now distinguish the issue's two cases: a damaged map file with Matter storage intact (no ecosystem sees any change) vs lost Matter storage (the duplicates already exist; rebuilding accepts that rather than causing it). - MenuItems.xml: dialog warning + confirm-tick label - plugin.py: tick error, REBUILT log, callback docstring - bridge_client.py: TERMINAL_ATTACH_ERRORS map remedy - export_bridge.py: map-unreadable refusal log (now also names the menu path) - bridge_protocol.py: build_rebuild_endpoint_map docstring ("reallocate from scratch" was mechanically wrong too) - node.ts: refuse-to-serve remedy line - BRIDGE_PROTOCOL.md §1.1 remedy table + §3.11; INSTALL.md destructive-actions section (drops the "worse one first / UNRECOVERABLE" framing) Kept, deliberately: the confirm tick (the rebuild still irreversibly discards the only repairable record — quarantined aside, not deleted); the healthy-node refusal (that one genuinely would discard §3.3's retained numbers); and every claim that SILENT reallocation duplicates accessories (DriftEntry, PRD §7 table) — that is about renumbering, which drift auto-repair would do and the confirmed rebuild does not. Tests pin the new claim rather than the absence of the old phrase; the identity-vs-map branch test's leak canary moves from "duplicate accessories" to "renumbers nothing", which the map remedy actually says. Suites: 2256 Python, 383 TS, both green. Version 2026.8.5 → 2026.8.6. Closes #132 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change corrects endpoint-map recovery wording across bridge messages, plugin UI, logs, tests, and documentation. Rebuilding now adopts live endpoint numbers without renumbering. Storage-loss consequences remain separate from map-file damage. ChangesEndpoint Map Recovery
Estimated code review effort: 2 (Simple) | ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/BRIDGE_PROTOCOL.md`:
- Around line 308-310: Update the explanatory sentence near the endpoint-map
quarantine command so existing duplication is attributed specifically to the
earlier loss of Matter storage, not to loss or damage of the endpoint map.
Preserve the distinction that the command discards the surviving mapping record
without itself duplicating accessories.
In `@docs/INSTALL.md`:
- Line 555: In the documentation text near “accessories,” replace the British
English word “afterwards” with the American English form “afterward,” leaving
the surrounding wording unchanged.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/export_bridge.py:
- Around line 1355-1358: Update the unreadable-map warning in the export bridge
to remove the assertion that duplication has already happened. Use the
established storage-loss wording from the corresponding MenuItems.xml and
INSTALL.md guidance, describing the potential for paired ecosystems to retain
dead accessories under old endpoint numbers without claiming duplication is
guaranteed.
🪄 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: 0f161f93-05ac-4ac7-9990-91a2001f94c0
📒 Files selected for processing (14)
bridge-node/src/node.tsbridge-node/test/persistence.test.tsdocs/BRIDGE_PROTOCOL.mddocs/HANDOVER.mddocs/INSTALL.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/test_bridge_client.pytests/test_export_bridge.pytests/test_export_menu.py
| which Matter accessory number belongs to which Indigo device; rebuilding it | ||
| discards the unreadable record and adopts whatever numbers exist now as the new | ||
| one. It renumbers nothing, so **the rebuild itself cannot duplicate | ||
| accessories** — what you see afterwards depends on which fault brought you |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the American English form.
Line 555 uses afterwards. Replace it with afterward.
Proposed fix
- accessories** — what you see afterwards depends on which fault brought you
+ accessories** — what you see afterward depends on which fault brought youThe static analysis hint identifies afterwards as the non-preferred American English variant.
📝 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.
| accessories** — what you see afterwards depends on which fault brought you | |
| accessories** — what you see afterward depends on which fault brought you |
🧰 Tools
🪛 LanguageTool
[locale-violation] ~555-~555: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... duplicate accessories** — what you see afterwards depends on which fault brought you here...
(AFTERWARDS_US)
🤖 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/INSTALL.md` at line 555, In the documentation text near “accessories,”
replace the British English word “afterwards” with the American English form
“afterward,” leaving the surrounding wording unchanged.
Source: Linters/SAST tools
| "map is unreadable (%s). Nothing will be exported until it is rebuilt (Plugins " | ||
| "▸ Matter ▸ Rebuild Matter Endpoint Map…). The rebuild renumbers nothing: if " | ||
| "only the map file was damaged no paired ecosystem will see any change, and if " | ||
| "the bridge's Matter storage was lost the duplication has already happened.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not assert that duplication has already happened.
At Line 1358, the node is still refusing to serve the rebuilt endpoint set. Lost Matter storage can leave paired ecosystems with dead accessories under old endpoint numbers, but duplication is not guaranteed at this point. Use the same storage-loss wording as indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml Lines 346-352 and docs/INSTALL.md Lines 558-568.
Proposed wording
- "the bridge's Matter storage was lost the duplication has already happened.",
+ "the bridge's Matter storage was lost, paired ecosystems may already contain "
+ "dead accessories under the old numbers; rebuilding accepts that state.",📝 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.
| "map is unreadable (%s). Nothing will be exported until it is rebuilt (Plugins " | |
| "▸ Matter ▸ Rebuild Matter Endpoint Map…). The rebuild renumbers nothing: if " | |
| "only the map file was damaged no paired ecosystem will see any change, and if " | |
| "the bridge's Matter storage was lost the duplication has already happened.", | |
| "map is unreadable (%s). Nothing will be exported until it is rebuilt (Plugins " | |
| "▸ Matter ▸ Rebuild Matter Endpoint Map…). The rebuild renumbers nothing: if " | |
| "only the map file was damaged no paired ecosystem will see any change, and if " | |
| "the bridge's Matter storage was lost, paired ecosystems may already contain " | |
| "dead accessories under the old numbers; rebuilding accepts that state.", |
🤖 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 1355 - 1358, Update the unreadable-map warning in the export bridge to
remove the assertion that duplication has already happened. Use the established
storage-loss wording from the corresponding MenuItems.xml and INSTALL.md
guidance, describing the potential for paired ecosystems to retain dead
accessories under old endpoint numbers without claiming duplication is
guaranteed.
…tible string (#132) The review-pr pass (code-reviewer, comment-analyzer, pr-test-analyzer) on PR #148 found that the phrase-keyed sweep missed the false model stated in different words: - bridge_client.rebuild_endpoint_map() docstring still said "Reallocate endpoint numbers from scratch… This DUPLICATES accessories" — the docstring of the very method that executes the confirmed rebuild - two test comments taught "§3.11 exists to hand out NEW endpoint numbers" (test_bridge_protocol_frames, test_bridge_client) — the fixture's differing numbers depict a post-storage-loss node, not §3.11 reallocating Also from review: - pin the TERMINAL_ATTACH_ERRORS map remedy ("renumbers nothing") — the one corrected string a revert could silently restore — plus the symmetric not-in canary on the identity branch - pin the confirm-tick label and tick-error text; shorten the tick label from 108 chars to one-line length - "sees any change" → "sees any further change" (dialog + INSTALL): a damaged-map user may already have watched accessories go unavailable while the node refused (the #141 condition) - reflow the orphaned line in menuRebuildEndpointMap's docstring - BRIDGE_PROTOCOL.md: return the two stranded §1.1 rows (commissioning_window_failed, internal) to the error-code table (pre-existing breakage in the passage being edited); "a lost map implies" → "lost Matter storage implies" here and in endpoint-map.ts - INSTALL.md: Reset header "recoverable, at the cost of pairing again" dangled once its "UNRECOVERABLE" counterpart died → "recoverable by pairing again" Suites: 2256 Python, 383 TS, both green. pylint on bridge_client+plugin 9.32, measured identical on the pre-fix baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
[no-release] docs(handover): #148 merged, v2026.8.6 released
1 participant
Closes #132.
What
The Rebuild Matter Endpoint Map… dialog warned that rebuilding WILL duplicate accessories in already-paired ecosystems. It cannot:
rebuild()adopts the live endpoint numbers as the new baseline and renumbers nothing, so it cannot itself change what any ecosystem sees. The duplication risk belongs to the drift/storage-loss event that already happened — rebuilding only stops reporting it. Observed live 2026-08-06: after a last-fabric factory reset, rebuilding was the correct and harmless action while the dialog read as though it was about to break the house.Where
The issue named four files; the same false claim actually lived in seven user-facing places — including the attach-refusal log line the user reads immediately before opening the dialog. All rewritten to distinguish the issue's two cases:
MenuItems.xmlplugin.pybridge_client.pyTERMINAL_ATTACH_ERRORSmap remedyexport_bridge.pybridge_protocol.pybuild_rebuild_endpoint_mapdocstring — "reallocate from scratch" was mechanically wrong toonode.tsBRIDGE_PROTOCOL.md§1.1 + §3.11,INSTALL.mdDeliberately kept
DriftEntrydocstring, PRD §7 table) — that claim is about renumbering, which drift auto-repair would do and the confirmed rebuild does not.Tests
Pins moved to the new claim rather than the absence of the old phrase: the dialog test asserts "cannot itself duplicate accessories" / "renumbers NOTHING"; the refusal-log tests assert "renumbers nothing"; the identity-vs-map branch test's map-remedy-leak canary moves from
"duplicate accessories" not in said(which would now pass trivially) to"renumbers nothing" not in said.Suites: 2256 Python / 383 TS, both green. pylint 9.52 on touched modules. Version
2026.8.5 → 2026.8.6. Strings only — no behaviour change.🤖 Generated with Claude Code
https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Summary by CodeRabbit
Bug Fixes
Documentation
Chores