Add devices/archive, devices/unarchive, devices/list_archived#105
Merged
Conversation
Mirrors the legacy dashboard's archive flow (``esphome/dashboard/web_server.py:ArchiveRequestHandler`` / ``UnArchiveRequestHandler``) — a soft-delete that moves the YAML to ``<config_dir>/archive/`` and wipes the per-device PlatformIO build tree, but keeps the YAML on disk so the user can ``unarchive`` later. Three new WS commands: - ``devices/archive`` — move + wipe build, scanner re-fires ``DEVICE_REMOVED`` so the dashboard's active list refreshes. - ``devices/unarchive`` — move back to ``config_dir``; refuses to clobber an active YAML of the same name and surfaces a ``CommandError(INVALID_ARGS)`` so the dialog can prompt for a rename or explicit overwrite. - ``devices/list_archived`` — read ``<config_dir>/archive/``, parse each YAML's ``esphome:`` block, return name / friendly_name / comment so the dashboard's "Show archived devices" toggle can render rows + Unarchive / Delete-permanently controls. Edge cases pinned by tests: - Re-archiving a device with the same name appends a numeric suffix (``kitchen.yaml`` → ``kitchen (2).yaml``) rather than clobbering the earlier copy. - Archiving a never-compiled device (no StorageJSON, no build tree) succeeds — the archive flow is the natural "I just made this YAML and don't want it" escape hatch. - ``list_archived`` skips non-YAML and hidden files (``.DS_Store``, ``notes.txt``, ``.hidden.yaml``) so a single stray file in a user-managed archive directory doesn't poison the listing. - A YAML whose ``esphome:`` block doesn't parse falls back to surfacing the filename — the user's only handle on a legacy archive entry.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 64.30% 65.25% +0.95%
==========================================
Files 36 36
Lines 4572 4686 +114
==========================================
+ Hits 2940 3058 +118
+ Misses 1632 1628 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds backend support for archiving devices in the dashboard: moving YAMLs into an archive area, restoring them later, and listing archived entries for future UI integration. This fits into the existing DevicesController WebSocket API by extending device lifecycle operations without permanently deleting user configs.
Changes:
- Add new WebSocket commands:
devices/archive,devices/unarchive, anddevices/list_archived. - Implement archive/unarchive/list logic in
DevicesController, including build-directory cleanup on archive. - Add focused tests and API documentation for the new archive flow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
esphome_device_builder/controllers/devices.py |
Adds the new archive/unarchive/list command handlers and their filesystem logic. |
tests/test_archive_device.py |
Adds unit tests for archive moves, build cleanup, collisions, unarchive behavior, and archived listing. |
docs/API.md |
Documents the three new WebSocket API commands and their payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…g, StorageJSON fallback - Drop the suffix-bump dance in _archive_single. Renaming the archive copy to '<name> (2).yaml' would orphan it from the StorageJSON sidecar (still keyed on the original filename), so a later unarchive of the suffixed copy would lose the cached address / version / loaded_integrations. Refuse the collision with CommandError(INVALID_ARGS) and let the user resolve it explicitly. - Wrap archive_device / unarchive_device so a missing YAML surfaces as CommandError(NOT_FOUND) over the WS rather than bubbling up as a generic INTERNAL_ERROR. - _list_archived_sync now falls back to StorageJSON.name / friendly_name / comment when the archived YAML's esphome: block is sparse. Friendly name often only lives in the sidecar (the dashboard's edit-name dialog writes it there rather than mutating the YAML), so without this the archived listing regressed to bare filenames for those devices. - Tests updated: collision now expects CommandError, plus new cases for archive_device / unarchive_device NOT_FOUND translation and the StorageJSON-fallback path.
Path-traversal audit: archive / unarchive / delete_archived all build paths from the user-supplied filename (`<config_dir>/archive/<configuration>`, `ext_storage_path(configuration)`) that don't all flow through `Settings.rel_path`. A configuration like `../etc/passwd` or `subdir/../foo` could resolve outside the archive tree and be unlinked or overwritten — for delete_archived nothing else interposed. Reject anything that isn't a pure basename at the public-command boundary via `_validate_archive_configuration` (checks empty, `.` / `..`, path separators, NUL byte, and both POSIX + Windows .name round-trip). 11 traversal cases x 3 commands all rejected with INVALID_ARGS. Adds `devices/delete_archived` for the frontend's Delete-permanently button — symmetric with archive: removes the YAML + StorageJSON sidecar + device-metadata sidecar. FileNotFoundError translates to CommandError(NOT_FOUND). Tests + docs updated. 30 archive tests, 512 total.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
12 tasks
Address Copilot review on PR #105: 1. _archive_single now wipes the StorageJSON sidecar AND the device-metadata entry alongside the build dir. Previously left them in place so unarchive could restore cached state (address / version / hash / loaded_integrations), but per-filename keying means a future same-name configuration would inherit the archived device's stale state until recompiled or edited. Trade a few seconds of "unknown state" after unarchive (the scanner + monitor refill from the next mDNS broadcast) for full isolation between archive and any future same-name device. Updated docstrings to reflect. 2. _delete_archived_single now skips sidecar removal when an active config of the same filename exists. Defensive — with #1 in place this collision shouldn't arise, but if the archive predates this PR (or was written by the legacy dashboard which kept sidecars), the StorageJSON / metadata belong to the live device and removing them would wipe its cached IP / hash / loaded_integrations. Path-traversal verification: 11 traversal payloads x 3 archive commands all still rejected with INVALID_ARGS. The new code paths (sidecar wipe in archive, active-config check in delete_archived) all run after _validate_archive_configuration at the public boundary; the metadata file is hardcoded (.device-builder.json) and configuration is used only as a dict key. Tests: 33 archive-specific (3 new), 515 backend total.
Same shape lived in three places: _archive_single, _delete_archived_single, and _delete_single. The build-dir wipe (StorageJSON.build_path -> shutil.rmtree) and the sidecar cleanup (storage_path.unlink + remove_device_metadata with logging) were copy-pasted across them. Refactor pointed out during PR review. Pull both into module-level helpers near _validate_archive_configuration: - _wipe_device_build_dir(configuration) — load StorageJSON, rmtree build_path. No-op when sidecar missing or never built. - _remove_device_sidecars(config_dir, configuration) — unlink StorageJSON sidecar + remove device-metadata entry, both best-effort with warning logs. Net: -28 lines. Behavior identical (515 backend tests still green).
PR coverage was at 90.59% with 11 missing lines. Added 5 tests to cover the gaps: - test_archive_device_full_flow_calls_scanner — end-to-end archive_device() (the WS-command wrapper, not just the helper) + verifies the trailing _scanner.scan() call. - test_unarchive_device_full_flow_calls_scanner — same shape for unarchive_device(). - test_list_archived_full_flow — runs list_archived() through its loop.run_in_executor() wrapper; helper-level tests had been calling _list_archived_sync directly. - test_remove_device_sidecars_logs_oserror_on_storage_unlink — monkeypatch Path.unlink to raise OSError; assert the warning is logged and the helper doesn't propagate. - test_remove_device_sidecars_logs_exception_on_metadata_remove — same shape for remove_device_metadata raising RuntimeError. 38 archive-specific tests (5 new), 520 backend total.
set_device_metadata / get_device_metadata go through metadata_transaction which calls tempfile.mkstemp; that calls os.path.abspath under the hood which blockbuster (CI's blocking-call detector) flags from an async context. The local pytest run passed because blockbuster fires only in CI. Wrap the test's metadata setup + assertion in asyncio.to_thread so the blocking calls run off the event loop.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- delete_archived docstring now describes archive sidecars as 'usually already gone' (archive wipes them on the way in), with delete_archived's cleanup framed as covering legacy archives + the active-same-name guard. - _list_archived_sync docstring frames the StorageJSON fallback as a legacy-only path: server-created archives no longer have sidecars, so the fallback only matters for upstream-dashboard archives or pre-sidecar-wipe entries. - docs/API.md spells out the full archive teardown (build dir, StorageJSON, device-metadata) and notes that cached state refills from mDNS after unarchive. Same file's list_archived row drops the 'Show archived devices toggle' reference and uses 'archived-devices dialog' to match the frontend's actual UI shape.
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
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
Adds backend support for the dashboard's archive flow — soft-delete that's reversible, mirrors the legacy ESPHome dashboard's
ArchiveRequestHandler/UnArchiveRequestHandler. Pairs with frontend PR esphome/device-builder-frontend#86.Commands
devices/archive{configuration}<config_dir>/archive/, wipe build dir, wipe StorageJSON + metadata sidecars. Reversible viaunarchive.devices/unarchive{configuration}INVALID_ARGSif an active config of the same name already exists.devices/list_archived[{configuration, name, friendly_name, comment}]devices/delete_archived{configuration}archive.Design decisions
Sidecar wipe on archive
_archive_singleremoves the StorageJSON sidecar and device-metadata entry alongside the build dir. A previous iteration left them in place so unarchive could restore the cached IP / version / config_hash / loaded_integrations, but per-filename keying meant a future same-nameconfigurationwould inherit the archived device's stale state until recompiled. Wiping on archive trades a few seconds of "unknown state" after unarchive (the scanner + monitor refill from the next mDNS broadcast) for full isolation between archive and any future same-name device.Same-name collision: refuse, don't suffix-bump
_archive_singlerejects withCommandError(INVALID_ARGS)when an archive entry of the same name already exists, instead of renaming the new archive to<name> (2).yaml. The earlier suffix-bump approach orphaned the suffixed copy from its sidecars (sidecars stay keyed on the original filename), so unarchive of the bumped copy would surface without its cached state. Safer to make the user resolve the collision explicitly (unarchive the existing copy or permanently delete it first).Path-traversal validator at the public boundary
_validate_archive_configurationruns at the top of every archive command and rejects anything that isn't a pure basename (no path separators, no.., no NUL byte, no./.., fails POSIX or Windows.nameround-trip). Each helper builds paths from the user-supplied filename (<config_dir>/archive/<configuration>,ext_storage_path(configuration)→data_dir/storage/<configuration>.json) that don't all flow throughSettings.rel_path, so structural protection alone wasn't sufficient. 11 traversal payloads × 3 commands all rejected withINVALID_ARGS. Defense-in-depth follow-up for other handlers tracked at #107.delete_archiveddefensive guard_delete_archived_singleskips sidecar removal when an active config of the same filename exists. With archive-side sidecar wipe in place, this collision shouldn't arise from new archives, but archives created by the legacy dashboard or before this PR could still hit it — and removing the sidecars would wipe the live device's cached state.List fallback
_list_archived_syncfalls back toStorageJSON.name/friendly_name/commentwhen the archived YAML'sesphome:block is sparse (e.g. friendly name only ever lived in the sidecar because the user wrote it via the dashboard's edit-name dialog rather than mutating the YAML). Bare-filename is the third fallback.Error code translation
archive_device,unarchive_device,delete_archivedwrap the corresponding helpers and translateFileNotFoundErrortoCommandError(NOT_FOUND). Unarchive additionally translatesFileExistsError(active config already at the target filename) toCommandError(INVALID_ARGS). Without these the WS layer would surface them as genericINTERNAL_ERROR.Test plan