feat(backup): restore extracts the bridge node's storage too (#136) - #169
Merged
Conversation
restore_backup() now accepts optional bridge_storage_path/bridge_control kwargs and, when the archive carries bridge-node/ members and both are given, stops the bridge node before the controller, extracts its half of the archive (prefix stripped) alongside the controller fabric, and restarts it afterwards if it was running. A bridge-restart failure never rolls back an otherwise-good controller restore (XG5); a bridge that was not running before the restore is left stopped. Falls back to the pre-#136 controller-only behaviour (loud WARNING naming the manual recipe) whenever there's no bridge control, no usable bridge storage path, or the bridge path overlaps the controller's. An old, controller-only archive restores exactly as it always did, silently. Rollback is now symmetric: a failure during either extract undoes both sides (bridge first, controller second, controller restarted first on the way back), and the zip-slip pre-flight now checks both sides before either process is stopped. Adds 15 tests in tests/test_fabric_backup.py covering the two-control stop/start ordering, the bridge extraction/prefix-strip, every controller-only fallback path, and rollback symmetry. Refs #136 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Adds _bridge_restore_control(), the stop()/start() twin of _bridge_storage_path(): reuses self.bridge_process if one already exists, otherwise builds one from current prefs exactly as menuStopBridgeNode does, and falls back to None (bridge files reported and skipped, controller restores normally) if construction raises. menuRestoreFabricBackup now passes both to fabric_backup.restore_backup and reports the bridge outcome from its result: an ERROR naming the bridge diagnosis when the controller restored but the bridge failed to come back, an INFO with the drift-honesty note when it restored and started successfully. The XAC1 latch (export_bridge.note_agent_stopped) is deliberately never touched from this path in either direction — see the call-site comment for both branches' reasoning. plug fixture in tests/test_plugin_behaviour.py gains bridge_process=None (was previously unset, which the new code path needs). Adds 6 tests covering the control being passed/reused/rebuilt/falling back, the untouched XAC1 latch on both success and failure, and the honest bridge-outcome reporting. Refs #136 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
INSTALL.md's fabric-backup section, MenuItems.xml's restore dialog warning/checkbox labels, and CLAUDE.md's fabric_backup.py row now describe restore extracting the bridge node's storage too (stop, then extract, then restart), instead of the pre-#136 report-and-skip-only behaviour. Strikes the #136 row in HANDOVER.md's open-issues table. Bumps PluginVersion 2026.8.24 -> 2026.8.25 (Info.plist). Refs #136 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
…r predated the restore (#136) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
📝 WalkthroughWalkthroughThe restore flow now handles controller and Matter bridge storage. It validates archive contents, coordinates bridge and controller lifecycle operations, preserves existing directories for rollback, reports restart outcomes, and integrates bridge controls into the plugin. ChangesBridge storage restore
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin
participant restore_backup
participant BridgeProcess
participant ControllerStorage
participant BridgeStorage
User->>Plugin: Confirm fabric restore
Plugin->>restore_backup: Pass storage paths and bridge control
restore_backup->>BridgeProcess: Stop bridge
restore_backup->>ControllerStorage: Move aside and restore controller files
restore_backup->>BridgeStorage: Move aside and restore bridge files
restore_backup->>BridgeProcess: Restart previously active bridge
restore_backup-->>Plugin: Return restoration and restart status
Plugin-->>User: Log restore outcome
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
…136) F1 (HIGH, confirmed by 2 review agents + live repro): _move_aside_for_restore performed two renames outside restore_backup's try block. A controller rename that succeeded followed by a bridge rename that raised (e.g. a leftover same-stamp .pre-restore- dir) let a raw OSError escape: no rollback, the controller fabric stranded aside, both daemons left stopped, and the UI dialog claiming "your existing fabric was preserved" while it sat outside the live path. _move_aside_for_restore is now all-or-nothing: a failing bridge rename undoes the controller rename before re-raising, so after the function returns or raises, either both sides are aside or neither is. If the undo itself also fails (a double fault), both paths are named explicitly for manual recovery. restore_backup wraps the move-aside call in its own except OSError: it best-effort restarts matter-server and (if the bridge was alive) the bridge node, then raises a wrapped RuntimeError — the disk is unchanged in the single-fault case, and this failure never reaches the existing _rollback path (whose contract is unchanged: it can now assume both sides moved, or the call never returned). Tests (TDD — written first, watched fail against the raw OSError, then fixed): - test_a_failing_bridge_move_aside_undoes_the_controller_move_and_restarts_both - test_a_failing_bridge_move_aside_restart_failures_are_logged Mutation-verified: removing the undo rename, or removing either restart, independently fails these tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
… coverage (#136) Remaining review-fix batch on top of the F1 move-aside fix: A2 (F2, MED): _stop_for_restore's abort path discarded the bridge restart result when the controller failed to stop. Now logs an ERROR naming the bridge (matching the wording R4/A1 already use) instead of swallowing it. A3 (F3, LOW): _control_is_alive silently treated a raising is_alive() as alive with no trace. Takes a log param and logs a debug line, so a persistently-raising probe is distinguishable from one never called. A4 (F4, LOW): _rollback's bridge-rollback-mechanics failure message always said "previous storage is PRESERVED at %s" even when moved_aside_to was None (no previous bridge dir to preserve). Now two message shapes for the same failure, keyed on whether a previous copy actually exists (_bridge_rollback_failure_message, extracted to keep _rollback's branch count under the pylint gate). A5: PluginVersion 2026.8.25 -> 2026.9.0 (minor: user-visible restore behaviour + dialog text + doc changes). Docstring/comment precision (comment-analyzer findings): - Zip-slip docstring example changed from a two-dot escape (which also escapes under the raw archive-relative name, so it doesn't demonstrate the point) to the one-dot discriminating case. - Dropped three dangling "§3.3" references to a section that doesn't exist in this repo. - "escalated at CRITICAL" reworded to "at ERROR with a CRITICAL: message prefix" throughout (logging.CRITICAL is never actually used). - Plan-private labels (D, R1-R4) replaced with descriptive phrases. - _restart_bridge's docstring now names all four call sites (a fourth, the A1 move-aside path, exists since the previous commit). - plugin.py's XAC1 latch comment: case 3 now states the latch is left EXACTLY as it was, not unconditionally "stays SET"; case 2's "an agent this session never started" (false once restore's start() succeeds) reworded to "an agent whose lifecycle this session does not own". - MenuItems.xml restore warning hedges "stopped first and restarted afterwards" to cover the not-currently-running bridge case. - CLAUDE.md fabric_backup row: names all three bridge-skip reasons, not just "no control". - HANDOVER.md: single bracketed annotation on the now-wired deferred item. Test additions (test-analyzer, all rated >= 3): - FakeLogger gains a debug() recorder. - R1 bridge-rollback-mechanics failure (rename-back raises) no longer aborts the controller's own rollback; message shape asserted. - _restart_bridge's raise path after an otherwise-successful restore is reported (ERROR + debug), not swallowed. - Bridge control given but no storage path -> warns, controller-only. - Bridge-side H4 (emptied extraction) refuses and rolls both dirs back. - Both remaining bridge/controller storage-path overlap clauses. - Directory entries (not just files) in the archive extract cleanly on the prefix-stripping bridge path. - First-ever test of the pre-existing controller rollback-mechanics CRITICAL branch, with the bridge_note appended. - _control_is_alive edge cases: no is_alive() method, and a raising probe. - Zip-slip archive strengthened with the one-dot variant alongside the two-dot one; both existing zip-slip tests assert the control was never touched. - _SKIP_MSG reason-clause text pinned for the no-control and overlap cases. - Plugin-side: bridge_started=False leg asserts the diagnosis-fallback text; new bridge_moved_aside_to=None/bridge_started=None leg asserts no "None"/"preserved"/"restarted" claims (cb395f8 pin, tightened). - A2's abort-path restart-failure ERROR test. Mutation-verified (in addition to the F1 pair already verified): letting the R1 bridge-rollback exception propagate, removing _restart_bridge's try/except, and deleting the bridge H4 empty-check each independently fail their new test. Re-confirmed two of the original PR's mutations still kill (the _safe_extract prefix strip, the aliveness gate). Suite: 2384 passed (baseline 2371 + 13). pylint: fabric_backup 9.97/10 (baseline 9.96), plugin.py 9.23/10 (unchanged), combined 9.36/10 (baseline 9.35). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
![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
🧹 Nitpick comments (2)
tests/test_fabric_backup.py (1)
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse these constants in
_make_bridge_storageso the fixture and the assertions cannot diverge.
_make_bridge_storageat Lines 550-553 repeats the same two JSON literals. The round-trip tests at Lines 754-757 compare against these constants. If someone edits one copy, the tests fail with a value mismatch rather than pointing at the fixture.♻️ Proposed change
files = files or { - "identity.json": '{"installId": "abc", "passcode": 20202021, "discriminator": 3840}', - "endpoint-map.json": '{"version": 1, "endpoints": {"indigo-101": 2}}', + "identity.json": _IDENTITY_JSON, + "endpoint-map.json": _ENDPOINT_MAP_JSON, "node-indigo-matter-bridge/state.json": "{}", }🤖 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 18 - 19, Update _make_bridge_storage to use the existing _IDENTITY_JSON and _ENDPOINT_MAP_JSON constants instead of duplicating their JSON literals, keeping the fixture data aligned with the round-trip assertions.indigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.py (1)
395-395: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
# noqa: BLE001to match the module's existing suppression style.Ruff reports BLE001 on both new broad excepts. Elsewhere in this module (lines 698 and 821) the same construct carries
# noqa: BLE001. These two sites carry only the pylint pragma, so Ruff still flags them.♻️ Proposed change
- except Exception as exc: # pylint: disable=broad-except + except Exception as exc: # noqa: BLE001 # pylint: disable=broad-exceptAlso applies to: 420-420
🤖 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 at line 395, Update both broad exception handlers in the relevant error-handling blocks, including the handlers around `except Exception as exc`, to add the module’s existing `# noqa: BLE001` suppression alongside the current pylint pragma. Keep the exception behavior unchanged and apply the same annotation consistently at both sites.Source: Linters/SAST tools
🤖 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/HANDOVER.md`:
- Line 1503: Update the deferred-list entry around “skips” in the handover
documentation to remove the stale “reports and skips” status. Reflect that
fabric_backup.restore_backup extracts bridge members when bridge control and
storage are usable, or remove the item entirely from the deferred list.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/fabric_backup.py:
- Around line 664-690: Wrap server_control.start() in the move-aside failure
handler with the same exception handling used by _rollback, preserving the
existing CRITICAL recovery log and allowing the bridge restart logic to execute
when start() raises. Ensure the handler still raises the contextual RuntimeError
from the original OSError, including the start failure as appropriate, rather
than letting the start exception replace it.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/MenuItems.xml:
- Around line 328-335: Qualify the bridge-restore confirmation in MenuItems.xml
so bridge storage replacement is conditional on controller availability, a
usable non-overlapping storage path, and archive presence. Update
docs/INSTALL.md to document every fallback condition and the manual-copy
procedure; update docs/HANDOVER.md to remove or correct the stale deferred
“reports and skips” item.
---
Nitpick comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/fabric_backup.py:
- Line 395: Update both broad exception handlers in the relevant error-handling
blocks, including the handlers around `except Exception as exc`, to add the
module’s existing `# noqa: BLE001` suppression alongside the current pylint
pragma. Keep the exception behavior unchanged and apply the same annotation
consistently at both sites.
In `@tests/test_fabric_backup.py`:
- Around line 18-19: Update _make_bridge_storage to use the existing
_IDENTITY_JSON and _ENDPOINT_MAP_JSON constants instead of duplicating their
JSON literals, keeping the fixture data aligned with the round-trip assertions.
🪄 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: 1d8644e3-6e7d-4456-886c-b0a25886a177
📒 Files selected for processing (9)
CLAUDE.mddocs/HANDOVER.mddocs/INSTALL.mdindigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xmlindigo-matter.indigoPlugin/Contents/Server Plugin/fabric_backup.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pytests/test_fabric_backup.pytests/test_plugin_behaviour.py
| 3. **Restoring the bridge-node storage from a backup** needs a bridge `stop()`/ | ||
| `start()` control — E7's launchd agent. Backup is done; restore reports and | ||
| skips (above). | ||
| skips (above). [wired by #136, v2026.9.0] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale “reports and skips” status.
Line 1503 still describes the pre-#136 behavior. fabric_backup.restore_backup now extracts bridge members when bridge control and storage are usable. Replace the stale sentence with the conditional behavior, or remove this item from the deferred list.
🤖 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` at line 1503, Update the deferred-list entry around “skips”
in the handover documentation to remove the stale “reports and skips” status.
Reflect that fabric_backup.restore_backup extracts bridge members when bridge
control and storage are usable, or remove the item entirely from the deferred
list.
Comment on lines
664
to
+690
| try: | ||
| os.makedirs(storage_path, exist_ok=True) | ||
| _safe_extract(archive_path, storage_path) | ||
| # H4: a validly-zipped but empty restore is a silent wipe. Refuse it. | ||
| if not _is_nonempty_dir(storage_path): | ||
| raise RuntimeError( | ||
| f"Restored storage dir is empty after extracting {archive_path}; " | ||
| "refusing to leave the fabric wiped" | ||
| moved_aside_to = _move_aside_for_restore(storage_path, plan, now, log) | ||
| except OSError as exc: | ||
| # Best-effort: put both daemons back the way we found them. This is | ||
| # NOT routed through _rollback — there is nothing for it to undo, the | ||
| # move-aside itself already did (or explicitly failed to, and logged | ||
| # why) — this only needs to bring the daemons back up. | ||
| if not server_control.start(): | ||
| log.error( | ||
| "CRITICAL: matter-server is DOWN after a failed move-aside during restore. " | ||
| "To recover manually: check ~/Library/Logs/indigo-matter/matter-server.err.log " | ||
| "and start matter-server (it should pick up the original fabric at %s).", | ||
| storage_path, | ||
| ) | ||
| # C1: start() returns a bool; False (launchctl failed) is a failure that | ||
| # must trigger rollback exactly like an exception would. | ||
| if plan is not None and plan.was_alive: | ||
| if not _restart_bridge(plan, log): | ||
| log.error( | ||
| "The Matter bridge node did not restart after a failed move-aside during " | ||
| "restore. Its pairings are intact; exported accessories are unavailable " | ||
| "until it starts — check the bridge node's error log, then reload the " | ||
| "plugin." | ||
| ) | ||
| raise RuntimeError( | ||
| f"Fabric restore from {archive_path} failed while moving the existing storage " | ||
| "aside; the disk is unchanged unless the error above says otherwise — both " | ||
| "daemons were restarted" | ||
| ) from exc |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard server_control.start() in the move-aside failure handler.
server_control.start() at Line 671 can raise. _rollback wraps the same call in try/except at Lines 819-825 for that reason; this handler does not. If start() raises here, three things follow:
- The bridge that this restore stopped is never restarted, because Lines 678-685 are skipped.
- The raised exception replaces the wrapped
RuntimeError, so the caller loses the "moving the existing storage aside" context thatmenuRestoreFabricBackuprelies on. - The
CRITICAL:recovery guidance for a down controller is never logged.
The existing tests exercise a start() that returns False on this path, not one that raises.
🛡️ Proposed fix
- if not server_control.start():
+ try:
+ started = server_control.start()
+ except Exception as start_exc: # noqa: BLE001 # pylint: disable=broad-except
+ started = False
+ log.error("matter-server restart raised after a failed move-aside: %s", start_exc)
+ if not started:
log.error(
"CRITICAL: matter-server is DOWN after a failed move-aside during restore. "
"To recover manually: check ~/Library/Logs/indigo-matter/matter-server.err.log "
"and start matter-server (it should pick up the original fabric at %s).",
storage_path,
)📝 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
| try: | |
| os.makedirs(storage_path, exist_ok=True) | |
| _safe_extract(archive_path, storage_path) | |
| # H4: a validly-zipped but empty restore is a silent wipe. Refuse it. | |
| if not _is_nonempty_dir(storage_path): | |
| raise RuntimeError( | |
| f"Restored storage dir is empty after extracting {archive_path}; " | |
| "refusing to leave the fabric wiped" | |
| moved_aside_to = _move_aside_for_restore(storage_path, plan, now, log) | |
| except OSError as exc: | |
| # Best-effort: put both daemons back the way we found them. This is | |
| # NOT routed through _rollback — there is nothing for it to undo, the | |
| # move-aside itself already did (or explicitly failed to, and logged | |
| # why) — this only needs to bring the daemons back up. | |
| if not server_control.start(): | |
| log.error( | |
| "CRITICAL: matter-server is DOWN after a failed move-aside during restore. " | |
| "To recover manually: check ~/Library/Logs/indigo-matter/matter-server.err.log " | |
| "and start matter-server (it should pick up the original fabric at %s).", | |
| storage_path, | |
| ) | |
| # C1: start() returns a bool; False (launchctl failed) is a failure that | |
| # must trigger rollback exactly like an exception would. | |
| if plan is not None and plan.was_alive: | |
| if not _restart_bridge(plan, log): | |
| log.error( | |
| "The Matter bridge node did not restart after a failed move-aside during " | |
| "restore. Its pairings are intact; exported accessories are unavailable " | |
| "until it starts — check the bridge node's error log, then reload the " | |
| "plugin." | |
| ) | |
| raise RuntimeError( | |
| f"Fabric restore from {archive_path} failed while moving the existing storage " | |
| "aside; the disk is unchanged unless the error above says otherwise — both " | |
| "daemons were restarted" | |
| ) from exc | |
| try: | |
| moved_aside_to = _move_aside_for_restore(storage_path, plan, now, log) | |
| except OSError as exc: | |
| # Best-effort: put both daemons back the way we found them. This is | |
| # NOT routed through _rollback — there is nothing for it to undo, the | |
| # move-aside itself already did (or explicitly failed to, and logged | |
| # why) — this only needs to bring the daemons back up. | |
| try: | |
| started = server_control.start() | |
| except Exception as start_exc: # noqa: BLE001 # pylint: disable=broad-except | |
| started = False | |
| log.error("matter-server restart raised after a failed move-aside: %s", start_exc) | |
| if not started: | |
| log.error( | |
| "CRITICAL: matter-server is DOWN after a failed move-aside during restore. " | |
| "To recover manually: check ~/Library/Logs/indigo-matter/matter-server.err.log " | |
| "and start matter-server (it should pick up the original fabric at %s).", | |
| storage_path, | |
| ) | |
| if plan is not None and plan.was_alive: | |
| if not _restart_bridge(plan, log): | |
| log.error( | |
| "The Matter bridge node did not restart after a failed move-aside during " | |
| "restore. Its pairings are intact; exported accessories are unavailable " | |
| "until it starts — check the bridge node's error log, then reload the " | |
| "plugin." | |
| ) | |
| raise RuntimeError( | |
| f"Fabric restore from {archive_path} failed while moving the existing storage " | |
| "aside; the disk is unchanged unless the error above says otherwise — both " | |
| "daemons were restarted" | |
| ) from exc |
🤖 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 664 - 690, Wrap server_control.start() in the move-aside failure handler
with the same exception handling used by _rollback, preserving the existing
CRITICAL recovery log and allowing the bridge restart logic to execute when
start() raises. Ensure the handler still raises the contextual RuntimeError from
the original OSError, including the start failure as appropriate, rather than
letting the start exception replace it.
Comment on lines
+328
to
+335
| <Label>Restoring REPLACES the current fabric and restarts matter-server. If the backup includes the Matter bridge node's storage, that is restored too — if the bridge node is running it is stopped and started around the swap; a stopped bridge stays stopped. The current fabric (and bridge storage, if restored) is moved aside, not deleted, so a bad restore is reversible. Requires 'Manage LaunchAgent' to be on.</Label> | ||
| </Field> | ||
| <Field id="backup" type="menu"> | ||
| <Label>Backup to restore:</Label> | ||
| <List class="self" method="getFabricBackups"/> | ||
| </Field> | ||
| <Field id="confirm" type="checkbox" defaultValue="false"> | ||
| <Label>I understand this replaces the current fabric and will restart matter-server.</Label> | ||
| <Label>I understand this replaces the current fabric (and the Matter bridge node's storage, if the backup has it) and will restart matter-server.</Label> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep bridge-restore documentation consistent with the runtime contract.
The implementation can restore the controller while skipping bridge storage when bridge control is unavailable, the bridge storage path is unusable, or the paths overlap. The confirmation and documentation currently do not state this complete fallback behavior.
indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml#L328-L335: qualify the confirmation text so archive presence does not imply unconditional bridge replacement.docs/INSTALL.md#L637-L648: document all fallback conditions and the manual-copy procedure.docs/HANDOVER.md#L1503-L1503: replace the stale “reports and skips” status or remove the item from the deferred list.
Proposed wording
- If the backup includes the Matter bridge node's storage, that is restored too.
+ If the backup includes the Matter bridge node's storage, it is restored when
+ bridge control and a usable, non-overlapping storage path are available.
+ Otherwise, the bridge files are reported and skipped for manual recovery.📍 Affects 3 files
indigo-matter.indigoPlugin/Contents/Server Plugin/MenuItems.xml#L328-L335(this comment)docs/INSTALL.md#L637-L648docs/HANDOVER.md#L1503-L1503
🤖 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/MenuItems.xml around lines
328 - 335, Qualify the bridge-restore confirmation in MenuItems.xml so bridge
storage replacement is conditional on controller availability, a usable
non-overlapping storage path, and archive presence. Update docs/INSTALL.md to
document every fallback condition and the manual-copy procedure; update
docs/HANDOVER.md to remove or correct the stale deferred “reports and skips”
item.
…e fault (#136) Closing repair pass from five final-review agents: - F1: restore_backup's move-aside except handler had the ONE unguarded server_control.start() in the module — a raise there skipped the CRITICAL log, the bridge restart, and the wrapped RuntimeError. Wrapped it like _rollback's start() already is. Also hardened _stop_for_restore: a RAISING server_control.stop() used to leave a just-stopped bridge down with no restart attempt and no log line. - F2: the double-fault path (bridge rename fails, then undoing the controller rename ALSO fails) used to re-raise the same bare OSError as the single fault, so restore_backup couldn't tell them apart and unconditionally restarted matter-server — recreating the missing storage_path as a fresh empty fabric that then blocks the manual `mv` the error message prescribes. _move_aside_for_restore now signals the double fault via a private _MoveAsideDoubleFault(OSError); the handler skips the controller start on that path (nothing to come up on) but still best-effort restarts the bridge (its dir was never touched), and names the stranded aside path in the wrapped RuntimeError. - F3: the single-fault message's "both daemons were restarted" overclaimed (either restart may have failed) — reworded to say a restart was attempted and to point at the log. - F4: added the missing test for _bridge_rollback_failure_message's no-previous-copy shape — deleting that branch left the suite green before this. - F5: :func: -> :meth: for LaunchAgent.is_alive; stripped the leftover plan-private labels (F4/A1/A2) from prose, replaced with descriptive wording. 63 fabric_backup tests (was 60), full suite 2387 (was 2384). pylint fabric_backup.py 9.97/10.
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.
Fixes #136. Plugin
2026.9.0(minor: user-visible feature per workspace versioning rule). Suites 2384 Python (from 2350, +34 across build + review passes), 405 TS untouched. pylint: fabric_backup 9.94 → 9.97, plugin 9.23 held, combined 9.30 → 9.36.What changes
Since E5, Back up the Matter fabric… has archived the export bridge node's storage (
identity.json,endpoint-map.json, the matter.js store) under the reservedbridge-node/prefix — and restore reported-and-skipped it, because the bridge stop seam didn't exist yet. E7 shipped that seam (LaunchAgent.stop()/.start(), whose docstrings already named fabric restore as their caller). This PR wires it: Restore a fabric backup… now puts the bridge storage back too, stopping and starting the bridge node around the swap.The sequence, and why it's shaped this way
Stop bridge (only if alive) → stop controller → move BOTH dirs aside (all-or-nothing) → extract both → start controller → start bridge (only if we stopped it, outside the rollback scope):
_bootout()returns False for "no such job" as well as real failure, so without it a never-started bridge would abort every restore; and XG5 forbids leaving a bridge running on a system with nothing exported — a bridge that was down stays down (it returns on the next export).start()returning False for "package never installed" is by design; rolling back a good controller-fabric restore to protect a secondary daemon would destroy the thing the user came for. The gate means that branch is only reachable for a bridge that was demonstrably alive minutes earlier, so a False there is a real regression and is reported as one.bridge-node/../escape.txtresolves insidedestunder its raw archive name but escapes once the prefix is stripped — the raw-name check would have passed it.The XAC1 latch is untouched in every path — restore uses
stop()/start()(plist survives), unlikemenuStopBridgeNode'suninstall(); the call-site comment carries the case analysis, and a wiring test pinsnote_agent_stoppedis never called. Restored-content honesty: the restored triple is one internally-consistent snapshot but may be older than what paired ecosystems hold — the success log names the designed signal (endpoint-map drift is REPORTED, renumbered never; #141's restore-before-serve means an older map serves safely).The /review-pr pass (4 agents), and what it caught
Run post-build per the every-PR review policy. Headline finding, fixed in
ee8c624: the move-aside step sat outside the rollback scope and the PR had doubled it to two renames — controller rename succeeding then bridge rename failing (e.g. a leftover same-stamp.pre-restore-dir) stranded the controller fabric aside with both daemons stopped and no rollback, while the dialog claimed the fabric was preserved. Reproduced live by the reviewer; now all-or-nothing with a double-fault escape hatch, mutation-pinned both ways.Also from the pass (
cfa301a): the abort-path bridge restart's result is no longer discarded (a failed restart now logs the consequence); a raisingis_alive()probe is debug-logged;_rollback's bridge error text no longer claims a "previous copy preserved at None"; ~10 docstring precision fixes (step-numbering truth, the zip-slip example corrected to the discriminating one-..case, dangling plan-private section refs removed, the XAC1 comment's third case made conditional, CRITICAL-vs-ERROR grep-honesty); the restore dialog's warning now hedges for a not-running bridge; and 13 new tests closing the coverage review's gaps — including the previously-untested R1 rollback swallow, the bridge-side empty-extract wipe guard,_control_is_aliveedges, directory-entry members from hand-zipped archives, and a pin on every commit in the PR.Tests
34 new total (15 build + 13 review + 6 plugin wiring). 16 mutations verified across build and review passes; the coordinator independently re-ran four (aliveness gate, prefix strip, A1 undo-rename, late-error conversion class). The E5 invariant — nothing named
bridge-nodeever nested under the controller's storage — keeps its own test.Docs
INSTALL.md's manual-recipe paragraph replaced with the wired behavior; restore dialog warning + confirm labels updated (ids unchanged); CLAUDE.md module row updated; HANDOVER's open-items row struck + one bracketed annotation on the E5 deferred item.
For Simon
🤖 Generated with Claude Code
https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL