fix(backup): make a restored device match the one that was backed up - #439
Merged
Conversation
|
Warning Review limit reached
Next review available in: 44 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds YouTube Music authentication backup and restore support, improves backup export and failure reporting, resolves plugins by registry ID or installed path, updates installer ownership and reboot documentation, and adds the related test modules to the CI suite. ChangesBackup updates
Plugin registry lookup
Installer updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BackupAPI
participant BackupManager
participant BackupArchive
participant ConfigFilesystem
BackupAPI->>BackupManager: create or restore backup
BackupManager->>ConfigFilesystem: read or write config/ytm_auth.json
BackupManager->>BackupArchive: include or validate ytm_auth
BackupManager-->>BackupAPI: return contents or restore status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 high |
🟢 Metrics 0 duplication
Metric Results Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/test_backup_manager.py (1)
250-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-default YTM paths.
The new assertion covers only a successful restore with
restore_wifi=True. Add focused tests forpreview_backup_contents()["has_ytm_auth"],validate_backup(...)[2]["detected_contents"], andRestoreOptions(restore_wifi=False)returningytm_authinskipped.As per coding guidelines, test files must test component interactions and edge cases, and each test should verify a single responsibility.
🤖 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 `@test/test_backup_manager.py` around lines 250 - 252, Extend the backup-manager tests around the existing YTM restore assertion to cover the non-default paths separately: verify preview_backup_contents() reports has_ytm_auth, validate_backup(...)[2]["detected_contents"] includes YTM authentication, and RestoreOptions(restore_wifi=False) places ytm_auth in skipped. Keep each scenario as a focused single-responsibility test.Source: Coding guidelines
web_interface/blueprints/api_v3.py (1)
7866-7867: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winLog the fallback export path.
When
preferredis not writable, this branch silently selects the reinstall-deleted fallback. A Raspberry Pi operator cannot tell that an on-device backup may be removed by the next reinstall. Log the preferred path, fallback path, andOSErrorbefore returning.As per coding guidelines,
**/*.pyfiles must implement comprehensive logging for remote debugging on Raspberry Pi and provide clear error messages for troubleshooting.🤖 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 `@web_interface/blueprints/api_v3.py` around lines 7866 - 7867, Update the OSError handler in the export-path selection flow to log the preferred path, fallback path, and caught exception before returning the fallback. Preserve the existing fallback value PROJECT_ROOT / "config" / "backups" / "exports" and use the module’s established logging mechanism.Source: Coding guidelines
🤖 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 `@first_time_install.sh`:
- Around line 1500-1513: Update the secrets-file ownership and permission setup
around the chown branches and chmod command to stop suppressing failures,
require both operations to succeed, and verify the final owner, group, and mode
before reporting success. Preserve the root-service and regular-user ownership
targets, and exit with a clear troubleshooting error if chown, chmod, or final
verification fails.
In `@test/test_registry_id_resolution.py`:
- Around line 77-107: Add direct coverage for PluginStoreManager.get_plugin_info
using "ledmatrix-weather" with fetch_latest_from_github=False, and assert it
resolves to the expected weather plugin information. Keep the test network-free
and use the existing store fixture or setup patterns so it validates delegation
to get_registry_info without changing the current registry lookup tests.
In `@web_interface/blueprints/api_v3.py`:
- Around line 8030-8037: Ensure plugin IDs from restore_backup() are validated
as strings before reaching the failed_plugins formatting in the restore
response, preferably rejecting non-string IDs at the restore boundary; otherwise
convert each ID to a string before ', '.join(failed_plugins). Preserve the
existing detailed error response for valid IDs.
- Around line 8024-8039: Before the restore result is serialized with
result.to_dict(), update the endpoint flow to set result.success to False
whenever result.plugins_failed is non-empty. Keep the existing filesystem-error
handling and response construction unchanged so plugin reinstall failures
produce the existing restore-failure response instead of HTTP 200 success.
- Around line 7861-7864: Update the temporary writability probe near
preferred.mkdir to use a uniquely named, exclusively created temporary file
rather than the predictable preferred / ".writetest" path. Ensure the probe is
created within preferred, cleanup only removes the file created by this call,
and preserve the existing writability check behavior.
---
Nitpick comments:
In `@test/test_backup_manager.py`:
- Around line 250-252: Extend the backup-manager tests around the existing YTM
restore assertion to cover the non-default paths separately: verify
preview_backup_contents() reports has_ytm_auth,
validate_backup(...)[2]["detected_contents"] includes YTM authentication, and
RestoreOptions(restore_wifi=False) places ytm_auth in skipped. Keep each
scenario as a focused single-responsibility test.
In `@web_interface/blueprints/api_v3.py`:
- Around line 7866-7867: Update the OSError handler in the export-path selection
flow to log the preferred path, fallback path, and caught exception before
returning the fallback. Preserve the existing fallback value PROJECT_ROOT /
"config" / "backups" / "exports" and use the module’s established logging
mechanism.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f40aaa1-3cee-44eb-adc9-d782bce14d3f
📒 Files selected for processing (8)
.github/workflows/test.ymlREADME.mdfirst_time_install.shsrc/backup_manager.pysrc/plugin_system/store_manager.pytest/test_backup_manager.pytest/test_registry_id_resolution.pyweb_interface/blueprints/api_v3.py
Found by wiping a working device and reinstalling from scratch. Every problem here is invisible until you actually do that, which is why a green test suite and eleven hours of uptime had not surfaced any of them. **Four enabled plugins vanished on restore.** Weather, stocks, music and leaderboard have a registry `id` that differs from the `id` in their own manifest: the registry calls them `weather`, everything else calls them `ledmatrix-weather`. Installation already prefers the manifest id for the directory name and warns when the two disagree, so on disk, in config.json and in a backup they are `ledmatrix-weather` -- but nothing resolved that in reverse. Restore asked the store for `ledmatrix-weather` and got "Plugin not found in registry", four times, and the device came back missing four plugins the user had enabled. Registry lookup now falls back to matching `plugin_path`, which already records `plugins/ledmatrix-weather`. Renaming the published ids would have orphaned `plugin_state.json` entries keyed on the old ones. Exact id still wins, so a path that collides with another entry's id cannot shadow it. Against the live registry and a real 28-plugin install this takes unresolvable directories from five to one -- the one being starlark-apps, which is genuinely not in the registry. **Secrets could not be restored at all.** A fresh install left config_secrets.json group-readable but not group-writable, and the web interface -- which is what performs a restore -- does not necessarily run as the owner. Every other file in the backup restored; secrets failed with EACCES. Now group-writable, so the account running the web UI can put them back. **A partial restore reported "Restore had errors" and nothing else.** That is the same message whether the whole thing failed or it quietly dropped your API keys. It now names what was restored, what failed, and which plugins were not reinstalled. **ytm_auth.json was never in the backup.** It sits in config/ beside the three files that are, and is pure device-local auth: losing it silently signs the user out of YouTube Music. Backed up and restored with the wifi config, which it resembles. **Backups were written inside the directory a reinstall deletes.** config/backups/exports is destroyed by the reinstall the user was told to make it before. Exports now go beside the install, falling back to the old path when that is not writable. **The installer reboots without asking in non-interactive mode**, which the README did not mention -- easy to hit when piping the install, and alarming when a device you are installing onto disappears. Documented, with --no-reboot-prompt. Its log also claimed root:ledmatrix while printing a hardcoded group name rather than the one it used. Tests: registry resolution gets its own suite, including the collision case and third-party entries with an empty plugin_path. The existing round-trip test passed throughout this because its fixture plugin has a directory name equal to its id -- the one shape that cannot fail -- so it now carries ytm_auth too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
test_backup_manager.py existed but was never enrolled, so the tests that should have guarded backup and restore have not run on a pull request. That is part of why the restore bugs in the previous commit reached a device: the suite was there, it just was not watching. Adds it alongside the new registry-resolution tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Follow-up from testing the previous commit on real hardware, where the secrets fix turned out to be both too narrow and slightly wrong. Too narrow: config.json, wifi_config.json and ytm_auth.json are installed root-owned and group-readable exactly like the secrets file, so all four were unrestorable by the web service, not just one. `shutil.copy2` opens the destination for writing, which needs permission on the *existing file*; the web user could create files in that directory all day and still not replace them. Slightly wrong: the previous commit loosened the secrets file to group-writable. That was treating the symptom. The real error was deciding ownership from `ledmatrix.service` -- the display service, which runs as root and only ever *reads* secrets -- when the account that *writes* them is the web interface, which deliberately does not run as root. Ownership now follows the web service's user and the mode stays 640. `_copy_file` writes a temporary file alongside the target and renames over it. That needs only directory permission, so a restore no longer cares who owns the destination, and it is atomic: a crash mid-restore can no longer leave a half-written config. The destination's mode is carried across so restoring secrets does not widen them to the umask, and its owner is carried across too when the OS allows it -- only root can hand a file to another user, so a restore run by the web service keeps its own ownership rather than pretending to preserve root's. Verified on a device with all four config files set root-owned 640 and unwritable by the web user: before, every one failed with EACCES; after, the restore reports success with no errors and all four sections restored, mode still 640, root still able to read them and the web service still able to write them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
ChuckBuilds
force-pushed
the
fix/backup-restore-fidelity
branch
from
August 6, 2026 19:33
cbe6240 to
7d83ca7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/backup_manager.py (1)
616-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog YouTube Music restore failures.
Line 620 records the failure only in
RestoreResult. The downstream API returns that result to the caller, but it does not log partial restore failures. Add an error log with a[Backup]prefix andexc_info=Truebefore appending the user-facing error.As per coding guidelines, “Implement comprehensive logging for remote debugging on Raspberry Pi.”
🤖 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 `@src/backup_manager.py` around lines 616 - 621, Add an error log in the restore_wifi exception handler around _copy_file, using the [Backup] prefix and exc_info=True, before appending the existing user-facing message to result.errors. Preserve the current RestoreResult behavior.Source: Coding guidelines
🤖 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 `@test/test_backup_manager.py`:
- Around line 251-253: Extend test_restore_honors_options to verify the disabled
YouTube Music restore path: assert that "ytm_auth" is included in result.skipped
and that config/ytm_auth.json is absent when RestoreOptions.restore_wifi=False.
Keep the existing enabled-path assertions unchanged.
---
Nitpick comments:
In `@src/backup_manager.py`:
- Around line 616-621: Add an error log in the restore_wifi exception handler
around _copy_file, using the [Backup] prefix and exc_info=True, before appending
the existing user-facing message to result.errors. Preserve the current
RestoreResult behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d8c5a94-f351-4a07-a170-757732002fd5
📒 Files selected for processing (8)
.github/workflows/test.ymlREADME.mdfirst_time_install.shsrc/backup_manager.pysrc/plugin_system/store_manager.pytest/test_backup_manager.pytest/test_registry_id_resolution.pyweb_interface/blueprints/api_v3.py
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/test.yml
- README.md
- test/test_registry_id_resolution.py
- web_interface/blueprints/api_v3.py
- src/plugin_system/store_manager.py
3 tasks
- first_time_install.sh: verify chown/chmod succeed and the final owner/group/mode on config_secrets.json before reporting success; exit with a clear error otherwise instead of swallowing failures. - api_v3.py: replace the predictable .writetest probe with an exclusive NamedTemporaryFile to avoid a race with concurrent resolvers; log the preferred/fallback export path and OSError when falling back to the reinstall-deleted directory. - api_v3.py: mark a restore as failed when plugin reinstalls fail, even if file restoration itself succeeded, so the endpoint no longer reports HTTP 200 success on a partial restore. - api_v3.py: stringify plugin IDs before joining them into the error message so a malformed backup's non-string plugin_id can't raise a TypeError and mask the detailed response. - backup_manager.py / api_v3.py: stop putting raw exception text (originating from a user-controlled backup file) into restore results returned to the client; log full details server-side instead. Addresses the CodeQL "stack trace information exposure" alert. - test coverage: add a test for get_plugin_info() resolving a manifest id, and assert the disabled restore_wifi path also skips and omits ytm_auth.json. Co-authored-by: Claude <noreply@anthropic.com>
…delity # Conflicts: # .github/workflows/test.yml
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
3 participants
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.
Found by wiping a working device and reinstalling from scratch, then restoring its backup. Every problem here is invisible until you do that — which is why a green suite and eleven hours of uptime had surfaced none of them.
1. Four enabled plugins vanished on restore
Weather, stocks, music and leaderboard have a registry
idthat differs from theidin their own manifest:ididledmatrix-weatherweatherledmatrix-stocksstocksledmatrix-musicmusicledmatrix-leaderboardleaderboardledmatrix-flightsledmatrix-flights✓Installation already knows this — it deliberately names the directory after the manifest id and warns when they disagree. So on disk, in
config.jsonand in a backup, they'reledmatrix-weather. Only the registry saysweather, and nothing resolved it in reverse:The device came back missing four plugins the user had enabled, with nothing surfaced in the UI.
Lookup now falls back to matching
plugin_path, which already recordsplugins/ledmatrix-weather. Renaming the published ids would orphanplugin_state.jsonentries keyed on the old ones. Exact id still wins, so a path colliding with another entry's id can't shadow it.Verified against the live registry and a real 28-plugin install: unresolvable directories go from 5 → 1, the one being
starlark-apps, which genuinely isn't in the registry.2. Secrets could not be restored at all
A fresh install leaves
config_secrets.jsongroup-readable but not group-writable, and the web interface — which performs restores — doesn't necessarily run as the owner. Every other file restored; secrets failed withPermission denied. Now group-writable.3. A partial restore said only "Restore had errors"
Same message whether the whole thing failed or it quietly dropped your API keys. It now names what restored, what failed, and which plugins weren't reinstalled.
4.
ytm_auth.jsonwas never backed upIt sits in
config/beside the three files that are, and is pure device-local auth — losing it silently signs you out of YouTube Music. Now backed up and restored alongside the wifi config it resembles.5. Backups were written inside the directory a reinstall deletes
config/backups/exportsis destroyed by the reinstall you were told to make it before. Exports now land beside the install, falling back to the old path if that isn't writable.6. Non-interactive install reboots without asking
Undocumented, and alarming when the device you're installing onto vanishes mid-SSH. Documented, with
--no-reboot-prompt. The installer's log also claimedroot:ledmatrixwhile printing a hardcoded group rather than the one it used.Tests
New
test_registry_id_resolution.pycovers resolution by manifest id, the id-vs-path collision case, and third-party entries with an emptyplugin_path(which must not match""). It fails onmainwith exactly the production symptom.Worth calling out why the existing suite didn't catch this:
test_restore_roundtripuses a fixture plugin whose directory name equals its id — the one shape that cannot exhibit the bug. It now carriesytm_authtoo.Separately,
test_backup_manager.pyexisted but was never enrolled in CI, so the tests meant to guard backup and restore weren't running on PRs. Both suites are now enrolled.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
New Features
Bug Fixes
Documentation