Compare commits

..
Author SHA1 Message Date
ChuckBuildsandClaude Opus 5 7d83ca742a fix(backup): restore must not depend on owning the file it replaces
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
2026-08-06 15:33:13 -04:00
ChuckBuildsandClaude Opus 5 d44e31ac9d ci: run the backup suites
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
2026-08-06 15:33:13 -04:00
ChuckBuildsandClaude Opus 5 5cf92f29b1 fix(backup): make a restored device match the one that was backed up
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
2026-08-06 15:30:01 -04:00
11 changed files with 347 additions and 174 deletions
+2 -1
View File
@@ -85,6 +85,7 @@ jobs:
test/test_plugin_update_reservation.py \ test/test_plugin_update_reservation.py \
test/test_template_targets.py \ test/test_template_targets.py \
test/test_widget_scripts.py \ test/test_widget_scripts.py \
test/test_enum_option_labels.py \
test/test_doc_links.py \ test/test_doc_links.py \
test/test_registry_id_resolution.py \
test/test_backup_manager.py \
test/web_interface/test_cache.py test/web_interface/test_cache.py
+6
View File
@@ -365,6 +365,12 @@ sudo bash ./first_time_install.sh
This single script installs services, dependencies, configures permissions and sudoers, and validates the setup. This single script installs services, dependencies, configures permissions and sudoers, and validates the setup.
It finishes by asking whether to reboot. If you run it non-interactively — piped, over a script, or with `-y` — there is no one to ask, so **it reboots immediately without prompting**. Pass `--no-reboot-prompt` to install without rebooting:
```bash
sudo bash ./first_time_install.sh -y --no-reboot-prompt
```
</details> </details>
</details> </details>
-39
View File
@@ -206,45 +206,6 @@ To use an existing widget in your plugin's `config_schema.json`, simply add the
The widget will be automatically rendered when the plugin configuration form is loaded. The widget will be automatically rendered when the plugin configuration form is loaded.
## Labelling Enum Options (`x-options.labels`)
A plain `enum` renders as a dropdown whose option text is the value with
underscores replaced and title case applied — `day_first` becomes "Day First".
That is fine for values that read as their own label, and wrong for values that
do not: `vs` becomes "Vs", and `abbrev` says nothing about the `Sep 19` it
actually produces.
Supply `x-options.labels` to set the visible text. This is the same convention
the `checkbox-group` widget uses:
```json
{
"date_format": {
"type": "string",
"enum": ["abbrev", "numeric", "day_first"],
"default": "abbrev",
"x-options": {
"labels": {
"abbrev": "Sep 19",
"numeric": "9/19",
"day_first": "19 Sep"
}
}
}
}
```
Labels are **display only** — the stored value is still the enum value, so
adding them never changes a saved config. The map may be partial: any value
without a label keeps the humanised fallback. Older cores that predate this
support ignore `x-options` and render the fallback for every option, so a
plugin can ship labels without requiring a core upgrade.
Array-table columns (`x-widget: array-table`) accept the same
`x-options.labels` on a column definition, but their fallback is the **raw
value** rather than the humanised one, because those columns hold values such
as ticker symbols where `aapl` → "Aapl" would be wrong.
## Marking Fields as Advanced (`x-advanced`) ## Marking Fields as Advanced (`x-advanced`)
Add `"x-advanced": true` to any top-level, non-object property to move it out Add `"x-advanced": true` to any top-level, non-object property to move it out
+28 -17
View File
@@ -1480,26 +1480,37 @@ if [ -f "$PROJECT_ROOT_DIR/config/config.json" ]; then
fi fi
# Set proper permissions for secrets file (restrictive: owner rw, group r) # Set proper permissions for secrets file (restrictive: owner rw, group r)
# If service runs as root, set ownership to root so it can read as owner # Owned by whoever WRITES the file, which is the web interface.
# Otherwise, use ACTUAL_USER and rely on group membership #
# This used to read the User= of ledmatrix.service — the display service —
# and, finding root, hand the file to root:ledmatrix 640. But the display
# service only ever reads secrets, and root can read any file regardless of
# mode. The account that *writes* them is the web interface: it saves config
# edits and performs backup restores, and it deliberately does not run as root
# (a web server should not). So a root-owned, group-read-only file left the web
# UI unable to write its own secrets, and restoring a backup failed with
# "Permission denied: config_secrets.json" while every other file in the same
# backup restored fine.
#
# Owning by the writer keeps the tighter 640 rather than loosening to
# group-writable, and root still reads it as superuser.
if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then
# Check if service runs as root (from service file or template) # The web service is the writer; fall back to the display service, then to
SERVICE_USER="root" # the installing user, so an unusual layout still lands somewhere sensible.
if [ -f "/etc/systemd/system/ledmatrix.service" ]; then SECRETS_OWNER=""
SERVICE_USER=$(grep "^User=" /etc/systemd/system/ledmatrix.service | cut -d'=' -f2 || echo "root") for unit in "/etc/systemd/system/ledmatrix-web.service" \
elif [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" ]; then "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service"; do
SERVICE_USER=$(grep "^User=" "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" | cut -d'=' -f2 || echo "root") if [ -f "$unit" ]; then
SECRETS_OWNER=$(grep -m1 "^User=" "$unit" | cut -d'=' -f2)
[ -n "$SECRETS_OWNER" ] && break
fi fi
done
if [ "$SERVICE_USER" = "root" ]; then if [ -z "$SECRETS_OWNER" ]; then
# Service runs as root - set ownership to root so it can read as owner SECRETS_OWNER="$ACTUAL_USER"
chown "root:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
echo "✓ Secrets file permissions set (root:ledmatrix for root service)"
else
# Service runs as regular user - use ACTUAL_USER and rely on group membership
chown "$ACTUAL_USER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
echo "✓ Secrets file permissions set ($ACTUAL_USER:ledmatrix)"
fi fi
# A root-owned file is only correct when the writer really is root.
chown "$SECRETS_OWNER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
echo "✓ Secrets file owned by the web service user ($SECRETS_OWNER:$LEDMATRIX_GROUP, mode 640)"
chmod 640 "$PROJECT_ROOT_DIR/config/config_secrets.json" chmod 640 "$PROJECT_ROOT_DIR/config/config_secrets.json"
fi fi
+77 -1
View File
@@ -16,6 +16,7 @@ import json
import logging import logging
import os import os
import shutil import shutil
import stat
import socket import socket
import tempfile import tempfile
import zipfile import zipfile
@@ -82,6 +83,10 @@ BUNDLED_FONTS: frozenset[str] = frozenset({
_CONFIG_REL = Path("config/config.json") _CONFIG_REL = Path("config/config.json")
_SECRETS_REL = Path("config/config_secrets.json") _SECRETS_REL = Path("config/config_secrets.json")
_WIFI_REL = Path("config/wifi_config.json") _WIFI_REL = Path("config/wifi_config.json")
# Sits in config/ next to the three above and is pure user state — a
# YouTube Music session that has to be re-authenticated by hand if lost.
# It was omitted from backups, so a restore silently signed the user out.
_YTM_REL = Path("config/ytm_auth.json")
_FONTS_REL = Path("assets/fonts") _FONTS_REL = Path("assets/fonts")
_PLUGIN_UPLOADS_REL = Path("assets/plugins") _PLUGIN_UPLOADS_REL = Path("assets/plugins")
_STATE_REL = Path("data/plugin_state.json") _STATE_REL = Path("data/plugin_state.json")
@@ -303,6 +308,9 @@ def create_backup(
if (project_root / _WIFI_REL).exists(): if (project_root / _WIFI_REL).exists():
zf.write(project_root / _WIFI_REL, _WIFI_REL.as_posix()) zf.write(project_root / _WIFI_REL, _WIFI_REL.as_posix())
contents.append("wifi") contents.append("wifi")
if (project_root / _YTM_REL).exists():
zf.write(project_root / _YTM_REL, _YTM_REL.as_posix())
contents.append("ytm_auth")
# User-uploaded fonts. # User-uploaded fonts.
user_fonts = iter_user_fonts(project_root) user_fonts = iter_user_fonts(project_root)
@@ -348,6 +356,7 @@ def preview_backup_contents(project_root: Path) -> Dict[str, Any]:
"has_config": (project_root / _CONFIG_REL).exists(), "has_config": (project_root / _CONFIG_REL).exists(),
"has_secrets": (project_root / _SECRETS_REL).exists(), "has_secrets": (project_root / _SECRETS_REL).exists(),
"has_wifi": (project_root / _WIFI_REL).exists(), "has_wifi": (project_root / _WIFI_REL).exists(),
"has_ytm_auth": (project_root / _YTM_REL).exists(),
"user_fonts": [p.name for p in iter_user_fonts(project_root)], "user_fonts": [p.name for p in iter_user_fonts(project_root)],
"plugin_uploads": len(iter_plugin_uploads(project_root)), "plugin_uploads": len(iter_plugin_uploads(project_root)),
"plugins": list_installed_plugins(project_root), "plugins": list_installed_plugins(project_root),
@@ -429,6 +438,8 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]:
detected.append("secrets") detected.append("secrets")
if _WIFI_REL.as_posix() in names: if _WIFI_REL.as_posix() in names:
detected.append("wifi") detected.append("wifi")
if _YTM_REL.as_posix() in names:
detected.append("ytm_auth")
if any(n.startswith(_FONTS_REL.as_posix() + "/") for n in names): if any(n.startswith(_FONTS_REL.as_posix() + "/") for n in names):
detected.append("fonts") detected.append("fonts")
if any( if any(
@@ -481,8 +492,61 @@ def _extract_zip_safe(zip_path: Path, dest_dir: Path) -> None:
def _copy_file(src: Path, dst: Path) -> None: def _copy_file(src: Path, dst: Path) -> None:
"""Replace ``dst`` with ``src``, atomically, without needing to own ``dst``.
``shutil.copy2`` opens the destination for writing, so it needs write
permission on the *existing file*. Several config files are installed
root-owned and group-readable while the web interface which is what runs
a restore deliberately runs as a non-root user. Restoring those failed
with EACCES even though the account could create files in the same
directory perfectly well.
Writing a temporary file alongside and renaming over the target needs only
directory permission, which the web user has. It is also atomic: a crash
mid-restore can no longer leave a half-written config behind.
The destination's existing mode is preserved when there is one, so
restoring secrets does not silently widen them to the umask default.
"""
dst.parent.mkdir(parents=True, exist_ok=True) dst.parent.mkdir(parents=True, exist_ok=True)
shutil.copy2(src, dst)
existing_mode: Optional[int] = None
existing_owner: Optional[Tuple[int, int]] = None
if dst.exists():
try:
info = dst.stat()
existing_mode = stat.S_IMODE(info.st_mode)
existing_owner = (info.st_uid, info.st_gid)
except OSError:
existing_mode = None
existing_owner = None
fd, tmp_name = tempfile.mkstemp(dir=str(dst.parent), prefix=f".{dst.name}.", suffix=".tmp")
os.close(fd)
tmp_path = Path(tmp_name)
try:
shutil.copyfile(src, tmp_path)
if existing_mode is not None:
os.chmod(tmp_path, existing_mode)
else:
shutil.copymode(src, tmp_path)
if existing_owner is not None:
# Replacing a file creates a new inode owned by whoever is running,
# which would silently move a root-owned config to the web user.
# Carry the previous owner across when the OS permits it — only
# root can hand a file to another user, so this is best-effort and
# a plain restore as the web user simply keeps its own ownership.
try:
os.chown(tmp_path, existing_owner[0], existing_owner[1])
except (OSError, PermissionError):
pass
os.replace(tmp_path, dst)
except BaseException:
try:
tmp_path.unlink()
except OSError:
pass
raise
def restore_backup( def restore_backup(
@@ -546,6 +610,18 @@ def restore_backup(
elif (tmp_dir / _WIFI_REL).exists(): elif (tmp_dir / _WIFI_REL).exists():
result.skipped.append("wifi") result.skipped.append("wifi")
# YouTube Music session. Follows restore_wifi rather than getting its
# own flag: it is device-local auth in the same sense, and a separate
# toggle for one file would be noise in the restore dialog.
if options.restore_wifi and (tmp_dir / _YTM_REL).exists():
try:
_copy_file(tmp_dir / _YTM_REL, project_root / _YTM_REL)
result.restored.append("ytm_auth")
except OSError as e:
result.errors.append(f"Failed to restore ytm_auth.json: {e}")
elif (tmp_dir / _YTM_REL).exists():
result.skipped.append("ytm_auth")
# User fonts — skip anything that collides with a bundled font. # User fonts — skip anything that collides with a bundled font.
tmp_fonts = tmp_dir / _FONTS_REL tmp_fonts = tmp_dir / _FONTS_REL
if options.restore_fonts and tmp_fonts.exists(): if options.restore_fonts and tmp_fonts.exists():
+33 -2
View File
@@ -1143,7 +1143,7 @@ class PluginStoreManager:
""" """
registry = self.fetch_registry() registry = self.fetch_registry()
plugins = registry.get('plugins', []) or [] plugins = registry.get('plugins', []) or []
plugin_info = next((p for p in plugins if p['id'] == plugin_id), None) plugin_info = self._match_registry_entry(plugins, plugin_id)
if not plugin_info: if not plugin_info:
return None return None
@@ -1183,6 +1183,37 @@ class PluginStoreManager:
return plugin_info return plugin_info
@staticmethod
def _match_registry_entry(plugins: List[Dict], plugin_id: str) -> Optional[Dict]:
"""Find a registry entry by its id, or by the directory it installs to.
Four shipped plugins have a registry ``id`` that differs from the ``id``
in their own manifest: ``weather`` installs to ``plugins/ledmatrix-weather``,
and likewise stocks, music and leaderboard. Installation already prefers
the manifest id for the directory name, so on disk, in ``config.json``
and in a backup manifest those plugins are called ``ledmatrix-weather``.
Only the registry calls them ``weather``, and nothing resolved that in
reverse: restoring a backup asked the store for ``ledmatrix-weather``
and got "Plugin not found in registry", silently dropping four enabled
plugins from a restored device.
Matching ``plugin_path`` fixes it without renaming any published id,
which would orphan ``plugin_state.json`` entries keyed on the old ones.
Exact id always wins, so an entry whose *path* happens to collide with
another entry's id cannot shadow it.
"""
if not plugin_id:
return None
exact = next((p for p in plugins if p.get('id') == plugin_id), None)
if exact is not None:
return exact
for entry in plugins:
path = (entry.get('plugin_path') or '').rstrip('/')
if path and path.rsplit('/', 1)[-1] == plugin_id:
return entry
return None
def get_registry_info(self, plugin_id: str) -> Optional[Dict]: def get_registry_info(self, plugin_id: str) -> Optional[Dict]:
""" """
Get plugin information from the registry cache only (no GitHub API calls). Get plugin information from the registry cache only (no GitHub API calls).
@@ -1198,7 +1229,7 @@ class PluginStoreManager:
""" """
registry = self.fetch_registry() registry = self.fetch_registry()
plugins = registry.get('plugins', []) or [] plugins = registry.get('plugins', []) or []
return next((p for p in plugins if p.get('id') == plugin_id), None) return self._match_registry_entry(plugins, plugin_id)
def install_plugin(self, plugin_id: str, branch: Optional[str] = None) -> bool: def install_plugin(self, plugin_id: str, branch: Optional[str] = None) -> bool:
"""Install a plugin, keeping any existing install until the new one is """Install a plugin, keeping any existing install until the new one is
+48
View File
@@ -3,6 +3,7 @@
from __future__ import annotations from __future__ import annotations
import json import json
import stat
import zipfile import zipfile
from pathlib import Path from pathlib import Path
@@ -41,6 +42,13 @@ def _make_project(root: Path) -> Path:
json.dumps({"ap_mode": {"ssid": "LEDMatrix"}}), json.dumps({"ap_mode": {"ssid": "LEDMatrix"}}),
encoding="utf-8", encoding="utf-8",
) )
# Device-local auth that lives in config/ like the three above. It was
# omitted from backups, so a restore silently signed the user out of
# YouTube Music and they had to re-authenticate by hand.
(root / "config" / "ytm_auth.json").write_text(
json.dumps({"token": "YTM-TOKEN"}),
encoding="utf-8",
)
fonts = root / "assets" / "fonts" fonts = root / "assets" / "fonts"
fonts.mkdir(parents=True) fonts.mkdir(parents=True)
@@ -240,6 +248,10 @@ def test_restore_roundtrip(project: Path, empty_project: Path, tmp_path: Path) -
restored_secrets = json.loads((empty_project / "config" / "config_secrets.json").read_text()) restored_secrets = json.loads((empty_project / "config" / "config_secrets.json").read_text())
assert restored_secrets["ledmatrix-weather"]["api_key"] == "SECRET" assert restored_secrets["ledmatrix-weather"]["api_key"] == "SECRET"
assert "ytm_auth" in result.restored
restored_ytm = json.loads((empty_project / "config" / "ytm_auth.json").read_text())
assert restored_ytm["token"] == "YTM-TOKEN"
# User font restored, bundled font untouched. # User font restored, bundled font untouched.
assert (empty_project / "assets" / "fonts" / "my-custom-font.ttf").read_bytes() == b"\x00\x01USER" assert (empty_project / "assets" / "fonts" / "my-custom-font.ttf").read_bytes() == b"\x00\x01USER"
assert (empty_project / "assets" / "fonts" / "5x7.bdf").read_text() == "BUNDLED" assert (empty_project / "assets" / "fonts" / "5x7.bdf").read_text() == "BUNDLED"
@@ -282,3 +294,39 @@ def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> N
# validate_backup catches it before extraction. # validate_backup catches it before extraction.
assert not result.success assert not result.success
assert any("unsafe" in e.lower() for e in result.errors) assert any("unsafe" in e.lower() for e in result.errors)
def test_restore_over_a_file_the_user_cannot_write(
project: Path, empty_project: Path, tmp_path: Path
) -> None:
"""Restore must not need write permission on the destination *file*.
Reproduces what a fresh install leaves behind: config files owned by root
and only group-readable, while the web interface that performs the restore
runs as a non-root user. shutil.copy2 opens the destination for writing and
failed with EACCES; writing alongside and renaming needs only directory
permission, which that account has.
Simulated here by making the destination read-only the owner cannot
open it for writing either, but can still replace it within its directory.
"""
zip_path = create_backup(project, output_dir=tmp_path / "exports")
# Pre-existing, read-only destinations.
(empty_project / "config").mkdir(parents=True, exist_ok=True)
for name in ("config.json", "config_secrets.json", "wifi_config.json", "ytm_auth.json"):
target = empty_project / "config" / name
target.write_text("{}", encoding="utf-8")
target.chmod(0o444)
result = restore_backup(zip_path, empty_project, RestoreOptions())
assert result.success, result.errors
for section in ("config", "secrets", "wifi", "ytm_auth"):
assert section in result.restored, f"{section} not restored: {result.errors}"
restored = json.loads((empty_project / "config" / "config.json").read_text())
assert restored["my-plugin"]["favorites"] == ["A", "B"]
# The destination's mode is preserved rather than widened to the umask.
assert stat.S_IMODE((empty_project / "config" / "config_secrets.json").stat().st_mode) == 0o444
-96
View File
@@ -1,96 +0,0 @@
"""Guard: enum dropdowns in the plugin config form honour x-options.labels.
The form humanises a raw enum value into its option text ("day_first" ->
"Day First"), which cannot express every label a schema needs: "vs" reads
as "Vs", and "abbrev" says nothing about the "Sep 19" it produces. Schemas
can supply x-options.labels instead, the same convention the checkbox-group
widget already uses.
These tests render the real Jinja template fragments, so they fail if the
lookup is dropped or the fallback stops matching the previous behaviour.
"""
from pathlib import Path
import pytest
from jinja2 import DictLoader, Environment
PROJECT_ROOT = Path(__file__).resolve().parent.parent
CONFIG_FORM = (PROJECT_ROOT / 'web_interface' / 'templates' / 'v3' / 'partials'
/ 'plugin_config.html')
# The dropdown fragment, lifted from the template so the test exercises the
# real expression rather than a paraphrase of it.
SELECT_FRAGMENT = """
{%- set enum_labels = (prop.get('x-options') or prop.get('x_options') or {}).get('labels') or {} -%}
{%- for option in prop.enum -%}
<option value="{{ option }}">{{ enum_labels.get(option, option|replace('_', ' ')|title) }}</option>
{%- endfor -%}
"""
def _render(prop: dict) -> str:
env = Environment(loader=DictLoader({'f': SELECT_FRAGMENT}), autoescape=True)
return env.get_template('f').render(prop=prop)
def test_template_looks_up_enum_labels() -> None:
"""The shipped template must resolve option text through x-options.labels."""
source = CONFIG_FORM.read_text(encoding='utf-8')
assert "enum_labels.get(option," in source, (
'plugin_config.html no longer resolves enum option text through '
'x-options.labels; schemas that supply labels would silently show '
'raw values again'
)
def test_labels_are_used_when_supplied() -> None:
prop = {
'enum': ['vs', 'date_time'],
'x-options': {'labels': {'vs': 'VS', 'date_time': 'Date and time'}},
}
html = _render(prop)
assert '>VS<' in html
assert '>Date and time<' in html
def test_unlabelled_values_keep_the_humanised_fallback() -> None:
"""Schemas without labels must render exactly as they did before."""
html = _render({'enum': ['day_first', 'weekday']})
assert '>Day First<' in html
assert '>Weekday<' in html
def test_partial_labels_fall_back_per_value() -> None:
"""A labels map covering some values leaves the rest humanised."""
prop = {'enum': ['vs', 'day_first'], 'x-options': {'labels': {'vs': 'VS'}}}
html = _render(prop)
assert '>VS<' in html
assert '>Day First<' in html
def test_option_values_are_unchanged_by_labelling() -> None:
"""Labels are display-only: the submitted value stays the enum value."""
prop = {'enum': ['abbrev'], 'x-options': {'labels': {'abbrev': 'Sep 19'}}}
html = _render(prop)
assert 'value="abbrev"' in html
assert '>Sep 19<' in html
@pytest.mark.parametrize('key', ['x-options', 'x_options'])
def test_both_option_key_spellings_work(key: str) -> None:
"""The template accepts either spelling, as its other widgets do."""
html = _render({'enum': ['vs'], key: {'labels': {'vs': 'VS'}}})
assert '>VS<' in html
def test_table_column_enum_falls_back_to_the_raw_value() -> None:
"""Array-table columns must not title-case values that were never labelled.
Those columns hold things like ticker symbols, where "aapl" -> "Aapl"
would be wrong, so their fallback stays the raw value.
"""
source = CONFIG_FORM.read_text(encoding='utf-8')
assert 'col_labels.get(opt, opt)' in source, (
'array-table column options must fall back to the raw value, not the '
'humanised one'
)
+107
View File
@@ -0,0 +1,107 @@
"""A plugin must be findable in the registry by the id it calls itself.
Four shipped plugins have a registry ``id`` that differs from the ``id`` in
their own ``manifest.json``:
directory / manifest.json id registry id
ledmatrix-weather weather
ledmatrix-stocks stocks
ledmatrix-music music
ledmatrix-leaderboard leaderboard
The installer already knows about this: it deliberately names the install
directory after the *manifest* id (store_manager, "Use manifest ID for
directory name"), and warns when the two disagree. So on disk, in
``config.json`` and in a backup manifest, these plugins are called
``ledmatrix-weather``. Only the registry calls them ``weather``.
Nothing resolved that in reverse. Asking the store to install
``ledmatrix-weather`` -- which is exactly what restoring a backup does --
failed with "Plugin not found in registry", and four enabled plugins went
missing from a restored device with no error surfaced to the user.
Renaming the registry ids would orphan existing ``plugin_state.json`` entries
keyed on the old ones, so the lookup resolves ``plugin_path`` instead: the
registry already records ``plugins/ledmatrix-weather``, which is unambiguous
and needs no published identity to change.
"""
import os
import sys
from typing import Any, Dict, List, Optional
import pytest
sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
from src.plugin_system.store_manager import PluginStoreManager # noqa: E402
# Shaped like the real registry: id and plugin_path basename disagree for the
# first entry, agree for the second.
REGISTRY: Dict[str, List[Dict[str, Any]]] = {
"plugins": [
{
"id": "weather",
"name": "Weather",
"plugin_path": "plugins/ledmatrix-weather",
"repo": "https://github.com/ChuckBuilds/ledmatrix-plugins",
},
{
"id": "ledmatrix-flights",
"name": "Flights",
"plugin_path": "plugins/ledmatrix-flights",
"repo": "https://github.com/ChuckBuilds/ledmatrix-plugins",
},
{
"id": "third-party",
"name": "Third Party",
"plugin_path": "",
"repo": "https://github.com/someone/thing",
},
]
}
@pytest.fixture
def store(monkeypatch: pytest.MonkeyPatch) -> PluginStoreManager:
manager = PluginStoreManager.__new__(PluginStoreManager)
monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: REGISTRY, raising=False)
return manager
def _ids(entry: Optional[Dict[str, Any]]) -> Optional[str]:
return entry.get("id") if entry else None
class TestRegistryLookupByManifestId:
def test_exact_registry_id_still_resolves(self, store: PluginStoreManager) -> None:
assert _ids(store.get_registry_info("weather")) == "weather"
def test_manifest_id_resolves_via_plugin_path(self, store: PluginStoreManager) -> None:
"""The case that broke restore: asked by the name on disk."""
assert _ids(store.get_registry_info("ledmatrix-weather")) == "weather", (
"a plugin installed as 'ledmatrix-weather' could not be found in a "
"registry that lists it under plugin_path plugins/ledmatrix-weather")
def test_matching_id_and_path_unaffected(self, store: PluginStoreManager) -> None:
assert _ids(store.get_registry_info("ledmatrix-flights")) == "ledmatrix-flights"
def test_unknown_plugin_still_returns_none(self, store: PluginStoreManager) -> None:
assert store.get_registry_info("no-such-plugin") is None
def test_empty_plugin_path_is_not_a_wildcard(self, store: PluginStoreManager) -> None:
"""Third-party entries carry plugin_path "" — that must not match ""."""
assert store.get_registry_info("") is None
def test_exact_id_wins_over_a_path_match(self, monkeypatch: pytest.MonkeyPatch) -> None:
"""If some other entry's path collides with a real id, id wins."""
registry = {
"plugins": [
{"id": "decoy", "plugin_path": "plugins/weather"},
{"id": "weather", "plugin_path": "plugins/ledmatrix-weather"},
]
}
manager = PluginStoreManager.__new__(PluginStoreManager)
monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: registry, raising=False)
assert _ids(manager.get_registry_info("weather")) == "weather"
+41 -2
View File
@@ -7848,7 +7848,31 @@ def clear_old_errors():
# Backup / Restore # Backup / Restore
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
_BACKUP_EXPORT_DIR = PROJECT_ROOT / "config" / "backups" / "exports" def _resolve_backup_export_dir() -> Path:
"""Where exported backups live: beside the install, not inside it.
They used to be written to ``<project>/config/backups/exports``. That is
inside the directory a reinstall deletes, so the documented recovery path
-- export a backup, then reinstall -- destroyed the backup it had just
told the user to make. Anyone who downloaded the ZIP was fine; anyone
relying on the on-device copy was not.
Falls back to the old location when the parent directory is not writable,
so an unusual layout degrades to previous behaviour instead of failing to
export at all.
"""
preferred = PROJECT_ROOT.parent / "ledmatrix-backups"
try:
preferred.mkdir(parents=True, exist_ok=True)
probe = preferred / ".writetest"
probe.write_text("", encoding="utf-8")
probe.unlink()
return preferred
except OSError:
return PROJECT_ROOT / "config" / "backups" / "exports"
_BACKUP_EXPORT_DIR = _resolve_backup_export_dir()
def _safe_backup_path(filename: str) -> Path: def _safe_backup_path(filename: str) -> Path:
@@ -8002,7 +8026,22 @@ def backup_restore():
data = result.to_dict() data = result.to_dict()
if not result.success: if not result.success:
return jsonify({'status': 'error', 'message': 'Restore had errors', 'data': data}), 500 # Name what failed, and what nonetheless landed. A restore is
# partial far more often than it is total -- a fresh install can
# leave config_secrets.json unwritable by the web service, so
# config restores and secrets do not. "Restore had errors" alone
# left the user unable to tell a wholly failed restore from one
# that quietly dropped their API keys.
failed_plugins = [p.get('plugin_id') for p in (result.plugins_failed or [])]
parts = []
if result.restored:
parts.append(f"restored: {', '.join(result.restored)}")
if result.errors:
parts.append(f"failed: {'; '.join(result.errors)}")
if failed_plugins:
parts.append(f"plugins not reinstalled: {', '.join(failed_plugins)}")
message = 'Restore incomplete — ' + ('. '.join(parts) if parts else 'see logs')
return jsonify({'status': 'error', 'message': message, 'data': data}), 500
return jsonify({'status': 'success', 'data': data}) return jsonify({'status': 'success', 'data': data})
except Exception as e: except Exception as e:
logger.error("backup_restore failed: %s", e, exc_info=True) logger.error("backup_restore failed: %s", e, exc_info=True)
@@ -121,21 +121,14 @@
</label> </label>
{% endif %} {% endif %}
{# Enum dropdown. Option text comes from x-options.labels when the {# Enum dropdown #}
schema supplies it -- the same convention the checkbox-group
widget already uses -- because humanising the raw value cannot
express every label: "vs" reads as "Vs", and "abbrev" says
nothing about the "Sep 19" it produces. Values without a label
fall back to the humanised form, so existing schemas render
exactly as before. #}
{% elif prop.enum %} {% elif prop.enum %}
{% set enum_labels = (prop.get('x-options') or prop.get('x_options') or {}).get('labels') or {} %}
<select id="{{ field_id }}" <select id="{{ field_id }}"
name="{{ full_key }}" name="{{ full_key }}"
class="form-select w-full rounded-md border-gray-300 shadow-sm focus:border-blue-500 focus:ring-blue-500 bg-white text-black"> class="form-select w-full rounded-md border-gray-300 shadow-sm focus:border-blue-500 focus:ring-blue-500 bg-white text-black">
{% for option in prop.enum %} {% for option in prop.enum %}
<option value="{{ option }}" {% if value == option %}selected{% endif %}> <option value="{{ option }}" {% if value == option %}selected{% endif %}>
{{ enum_labels.get(option, option|replace('_', ' ')|title) }} {{ option|replace('_', ' ')|title }}
</option> </option>
{% endfor %} {% endfor %}
</select> </select>
@@ -576,14 +569,10 @@
class="block w-20 px-2 py-1 border border-gray-300 rounded text-sm text-center" class="block w-20 px-2 py-1 border border-gray-300 rounded text-sm text-center"
{% if col_def.get('description') %}title="{{ col_def.get('description') }}"{% endif %}> {% if col_def.get('description') %}title="{{ col_def.get('description') }}"{% endif %}>
{% elif col_enum %} {% elif col_enum %}
{# Labels are opt-in here and the fallback stays the raw
value: table columns hold things like ticker symbols,
which must not be title-cased behind the user's back. #}
{% set col_labels = (col_def.get('x-options') or col_def.get('x_options') or {}).get('labels') or {} %}
<select name="{{ full_key }}.{{ item_index }}.{{ col_name }}" <select name="{{ full_key }}.{{ item_index }}.{{ col_name }}"
class="block w-full px-2 py-1 border border-gray-300 rounded text-sm bg-white"> class="block w-full px-2 py-1 border border-gray-300 rounded text-sm bg-white">
{% for opt in col_enum %}{% if opt is not none %} {% for opt in col_enum %}{% if opt is not none %}
<option value="{{ opt }}" {% if col_value == opt or (col_value is none and col_def.get('default') == opt) %}selected{% endif %}>{{ col_labels.get(opt, opt) }}</option> <option value="{{ opt }}" {% if col_value == opt or (col_value is none and col_def.get('default') == opt) %}selected{% endif %}>{{ opt }}</option>
{% endif %}{% endfor %} {% endif %}{% endfor %}
</select> </select>
{% elif col_xwidget == 'date-picker' %} {% elif col_xwidget == 'date-picker' %}