test(sync): probe for a free port on loopback, not every interface

CodeQL flagged the ephemeral-port probe in the handshake test for
binding to all interfaces. The probe only needs a free port number, so
loopback is both sufficient and correct — a test should not open a port
to the network to discover one.

The manager under test still binds to all interfaces, which is
deliberate and already marked nosec: a follower has to receive the
leader's UDP broadcast.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh

.github/workflows/test.yml

@@ -72,4 +72,4 @@ jobs:
             --ignore=test/plugins \
             --cov=src --cov=web_interface \
             --cov-report=term \
-            --cov-fail-under=48
+            --cov-fail-under=52

src/common/logo_helper.py

@@ -19,6 +19,10 @@ from src.common.permission_utils import (
 )
 
 
+# Well above any real team logo; bounds what a remote URL can write to disk.
+MAX_LOGO_BYTES = 10 * 1024 * 1024
+
+
 class LogoHelper:
     """
     Helper class for logo loading, caching, and resizing.
@@ -226,7 +230,10 @@ class LogoHelper:
         return {
             'cached_logos': len(self._logo_cache),
             'cache_size_limit': self.cache_size,
-            'cache_usage_percent': (len(self._logo_cache) / self.cache_size) * 100
+            'cache_usage_percent': (
+                (len(self._logo_cache) / self.cache_size) * 100
+                if self.cache_size else 0
+            ),
         }
     
     def _resize_logo(self, logo: Image.Image, max_width: Optional[int] = None, 
@@ -258,21 +265,43 @@ class LogoHelper:
         self._cache_order.append(cache_key)
     
     def _download_logo(self, url: str, file_path: Path) -> None:
-        """Download logo from URL."""
+        """Download logo from URL.
+
+        The response size is capped and the saved file is verified as a
+        decodable image before it is left on disk: a logo URL is remote
+        input, and without this an oversized or malformed response would
+        be cached for every later load_logo() call to trip over.
+        """
         # Ensure directory exists with proper permissions
         ensure_directory_permissions(file_path.parent, get_assets_dir_mode())
-        
+
         # Download with timeout
         response = self.session.get(url, timeout=30)
         response.raise_for_status()
-        
+
+        content = response.content
+        if len(content) > MAX_LOGO_BYTES:
+            raise ValueError(
+                f"Logo at {url} is {len(content)} bytes, over the "
+                f"{MAX_LOGO_BYTES}-byte limit; not saved")
+
         # Save to file
         with open(file_path, 'wb') as f:
-            f.write(response.content)
-        
+            f.write(content)
+
+        # Verify it decodes before leaving it on disk. PIL raises
+        # DecompressionBombError past its own pixel limit; a partial or
+        # non-image response raises UnidentifiedImageError/OSError.
+        try:
+            with Image.open(file_path) as probe:
+                probe.load()
+        except Exception:
+            file_path.unlink(missing_ok=True)
+            raise
+
         # Set proper file permissions after saving
         ensure_file_permissions(file_path, get_assets_file_mode())
-        
+
         self.logger.debug(f"Downloaded logo to {file_path}")
     
     def _create_placeholder_logo(self, team_abbr: str, 

src/common/sync_manager.py

@@ -101,6 +101,7 @@ class DisplaySyncManager:
         self._peer_chain: int = 0
         self._last_heartbeat_time: float = 0.0
         self._leader_width: int = 0  # set by display_controller after init
+        self._oversized_frame_warned: bool = False
 
         # Follower state
         self._follower_state = FollowerState.STANDALONE
@@ -174,6 +175,10 @@ class DisplaySyncManager:
                 continue
             except Exception as exc:
                 self.logger.debug("Sync leader recv error: %s", exc)
+                # Brief backoff: a socket left in a bad state raises
+                # immediately, which would otherwise spin this thread at
+                # 100% CPU logging the same error.
+                time.sleep(0.1)
 
     def _handle_hello(self, msg: dict, sender_ip: str) -> None:
         hw = self._hw_config
@@ -396,7 +401,7 @@ class DisplaySyncManager:
             data = header + arr.tobytes()
             if len(data) <= 65000:
                 self._send_sock.sendto(data, (self._peer_ip, self.port))
-            elif not getattr(self, '_oversized_frame_warned', False):
+            elif not self._oversized_frame_warned:
                 self._oversized_frame_warned = True
                 self.logger.warning(
                     "Sync: frame too large for UDP (%d bytes, max 65000) — "
@@ -451,41 +456,44 @@ class DisplaySyncManager:
         )
         self.write_status_file()
 
+    def _handle_received_frame(self, img: Image.Image, sender_ip: str) -> None:
+        """Record a decoded leader frame and enter follower mode if needed."""
+        with self._frame_lock:
+            self._latest_frame = img
+        self._last_leader_frame_time = time.time()
+        self._leader_ip = sender_ip
+
+        if self._follower_state == FollowerState.STANDALONE:
+            self._follower_state = FollowerState.FOLLOWER
+            self.logger.info(
+                "Sync: leader active at %s — switching to follower mode",
+                sender_ip,
+            )
+            self.write_status_file()
+
     def _follower_recv_loop(self) -> None:
         while self._running:
             try:
                 data, addr = self._recv_sock.recvfrom(65535)
                 sender_ip = addr[0]
 
-                if data[:8] == _RAW_MAGIC or len(data) > 512:
-                    # Frame data: prefer magic-tagged raw RGB; fall back to legacy PNG
+                if data[:8] == _RAW_MAGIC:
+                    # Magic-tagged raw RGB frame — self-describing, no guessing.
                     try:
-                        if data[:8] == _RAW_MAGIC:
-                            w, h = _RAW_HEADER.unpack(data[8:12])
-                            raw = data[12:]
-                            img = Image.frombuffer(
-                                "RGB", (w, h), raw, "raw", "RGB", 0, 1
-                            )
-                        else:
-                            # Fallback: try legacy PNG
-                            img = Image.open(io.BytesIO(data))
-                            img.load()
-                        with self._frame_lock:
-                            self._latest_frame = img
-                        self._last_leader_frame_time = time.time()
-                        self._leader_ip = sender_ip
-
-                        if self._follower_state == FollowerState.STANDALONE:
-                            self._follower_state = FollowerState.FOLLOWER
-                            self.logger.info(
-                                "Sync: leader active at %s — switching to follower mode",
-                                sender_ip,
-                            )
-                            self.write_status_file()
+                        w, h = _RAW_HEADER.unpack(data[8:12])
+                        raw = data[12:]
+                        img = Image.frombuffer(
+                            "RGB", (w, h), raw, "raw", "RGB", 0, 1
+                        )
+                        self._handle_received_frame(img, sender_ip)
                     except Exception as exc:
                         self.logger.debug("Sync: frame decode error: %s", exc)
                 else:
-                    # Control message
+                    # No magic prefix: try control-message JSON, and treat a
+                    # parse failure as a legacy (pre-magic) PNG frame. Both
+                    # wire formats are self-describing, so no size heuristic
+                    # is needed — a >512-byte control message used to be
+                    # misrouted into image decode and silently dropped.
                     try:
                         msg = json.loads(data.decode("utf-8"))
                         t = msg.get("t")
@@ -518,12 +526,19 @@ class DisplaySyncManager:
                             if self._on_new_cycle:
                                 self._on_new_cycle()
                     except (json.JSONDecodeError, UnicodeDecodeError, KeyError):
-                        pass
+                        # Not a control message — try legacy PNG frame.
+                        try:
+                            img = Image.open(io.BytesIO(data))
+                            img.load()
+                            self._handle_received_frame(img, sender_ip)
+                        except Exception as exc:
+                            self.logger.debug("Sync: frame decode error: %s", exc)
 
             except socket.timeout:
                 continue
             except Exception as exc:
                 self.logger.debug("Sync follower recv error: %s", exc)
+                time.sleep(0.1)
 
     def _follower_announce_loop(self) -> None:
         hw = self._hw_config

src/web_interface/api_helpers.py

@@ -29,18 +29,16 @@ def success_response(
         Flask jsonify response
     """
     response_data = create_success_response(data, message, metadata)
-    
-    # Add request metadata if available
-    if metadata is None:
-        metadata = {}
-    
-    # Add timing if request start time is available
+
+    # Timing is merged into whatever the caller passed, without inventing a
+    # metadata block for responses that have neither.
+    enriched = dict(metadata) if metadata is not None else {}
     if hasattr(request, 'start_time'):
-        metadata['response_time_ms'] = int((time.time() - request.start_time) * 1000)
-    
-    if metadata:
-        response_data['metadata'] = metadata
-    
+        enriched['response_time_ms'] = int((time.time() - request.start_time) * 1000)
+
+    if metadata is not None or enriched:
+        response_data['metadata'] = enriched
+
     return jsonify(response_data)
 
 

src/web_interface/error_handler.py

@@ -142,14 +142,17 @@ def create_success_response(
         "status": "success"
     }
     
+    # All three use `is not None` rather than truthiness: "" and {} are
+    # values a caller chose to send, and dropping them silently would make
+    # the response shape depend on the data.
     if data is not None:
         response["data"] = data
-    
-    if message:
+
+    if message is not None:
         response["message"] = message
-    
-    if metadata:
+
+    if metadata is not None:
         response["metadata"] = metadata
-    
+
     return response
 

src/web_interface/errors.py

@@ -89,7 +89,11 @@ class WebInterfaceError:
         self.category = category or self._infer_category(error_code)
         self.details = details
         self.context = context or {}
-        self.suggested_fixes = suggested_fixes or self._get_default_suggestions(error_code)
+        # `is None`, not truthiness: an explicit [] means "this caller has
+        # no suggestions to offer", which the default list would override.
+        self.suggested_fixes = (
+            suggested_fixes if suggested_fixes is not None
+            else self._get_default_suggestions(error_code))
         self.original_error = original_error
     
     def _infer_category(self, error_code: ErrorCode) -> ErrorCategory:

src/web_interface/validators.py

@@ -43,10 +43,15 @@ def validate_image_url(url: str) -> Tuple[bool, Optional[str]]:
     if any(handler in url_lower for handler in ['onerror=', 'onload=', 'onclick=']):
         return False, "Event handlers not allowed in URLs"
     
+    # Reject directory traversal anywhere, not only in relative paths:
+    # http://host/../secret is as much a traversal attempt as /../secret.
+    if '..' in url:
+        return False, "Invalid path: directory traversal not allowed"
+
     # Allow relative paths starting with /
     if url.startswith('/'):
-        # Validate it's a safe relative path (no directory traversal)
-        if '..' in url or url.startswith('//'):
+        # // would be a protocol-relative URL, not a local path
+        if url.startswith('//'):
             return False, "Invalid relative path"
         return True, None
     
@@ -104,10 +109,11 @@ def validate_file_upload(filename: str, max_size_mb: int = 10,
     if '..' in filename or '/' in filename or '\\' in filename:
         return False, "Filename contains invalid characters"
     
-    # Check extension if specified
+    # Check extension if specified. Both sides are lowercased: the caller's
+    # list is as likely to hold '.TTF' as the filename is.
     if allowed_extensions:
         file_ext = Path(filename).suffix.lower()
-        if file_ext not in allowed_extensions:
+        if file_ext not in [ext.lower() for ext in allowed_extensions]:
             return False, f"File extension must be one of: {', '.join(allowed_extensions)}"
     
     return True, None
@@ -147,7 +153,8 @@ def validate_numeric_range(value: float, min_val: Optional[float] = None,
     Returns:
         Tuple of (is_valid, error_message)
     """
-    if not isinstance(value, (int, float)):
+    # bool is an int subclass, so True would otherwise validate as 1.
+    if not isinstance(value, (int, float)) or isinstance(value, bool):
         return False, "Value must be a number"
     
     if min_val is not None and value < min_val:
@@ -183,11 +190,19 @@ def validate_string_length(text: str, min_length: Optional[int] = None,
 
 def sanitize_plugin_config(config: dict) -> dict:
     """
-    Sanitize plugin configuration input to prevent injection.
-    
+    Restrict a plugin config to safe key names and value types.
+
+    Drops keys that are not plain identifiers and values that are not
+    JSON-ish scalars, lists, or dicts, recursing into the latter two.
+
+    String values are returned **unescaped**: output escaping is the
+    template layer's job, and escaping here would store the escaped form
+    in config.json. Do not read this function as XSS protection for
+    rendered output.
+
     Args:
         config: Configuration dictionary
-    
+
     Returns:
         Sanitized configuration dictionary
     """

test/_api_v3_test_helpers.py

@@ -0,0 +1,75 @@
+"""
+Shared scaffolding for api_v3 blueprint tests.
+
+Not a test module (the leading underscore keeps pytest from collecting
+it). It is the pytest-fixture equivalent of ``_make_client()`` in
+test_uninstall_and_reconcile_endpoint.py, which is unittest-style and
+requires ``self.addCleanup``.
+
+The api_v3 blueprint keeps its managers as attributes on a module-level
+singleton, not in Flask app state, so replacing them with mocks leaks
+into every later test that imports api_v3 unless the originals are put
+back. ``api_v3_client`` snapshots and restores them around each test.
+"""
+
+from unittest.mock import MagicMock
+
+import pytest
+from flask import Flask
+
+
+# Every manager attribute the blueprint reads. Anything missing here keeps
+# whatever a previously-run test left on the singleton.
+API_V3_MANAGER_ATTRS = (
+    'config_manager', 'plugin_manager', 'plugin_store_manager',
+    'plugin_state_manager', 'saved_repositories_manager', 'schema_manager',
+    'operation_queue', 'operation_history', 'cache_manager',
+)
+
+_SENTINEL = object()
+
+
+def build_app(blueprint):
+    app = Flask(__name__)
+    app.config['TESTING'] = True
+    app.config['SECRET_KEY'] = 'test'
+    app.register_blueprint(blueprint, url_prefix='/api/v3')
+    return app
+
+
+@pytest.fixture
+def api_v3_module():
+    """The api_v3 module with every manager replaced by a MagicMock.
+
+    Restores the original attributes afterwards. Tests point individual
+    managers at real objects (a ConfigManager over tmp_path, say) or set
+    them to None to exercise the not-initialized branches.
+    """
+    from web_interface.blueprints import api_v3 as module
+
+    originals = {
+        name: getattr(module.api_v3, name, _SENTINEL)
+        for name in API_V3_MANAGER_ATTRS
+    }
+    for name in API_V3_MANAGER_ATTRS:
+        setattr(module.api_v3, name, MagicMock())
+    # Default to the direct path; queue tests opt in explicitly.
+    module.api_v3.operation_queue = None
+
+    yield module
+
+    for name, original in originals.items():
+        if original is _SENTINEL:
+            if hasattr(module.api_v3, name):
+                try:
+                    delattr(module.api_v3, name)
+                except AttributeError:
+                    pass
+        else:
+            setattr(module.api_v3, name, original)
+
+
+@pytest.fixture
+def api_v3_client(api_v3_module):
+    """Flask test client wired to the mocked blueprint."""
+    return build_app(api_v3_module.api_v3).test_client()

test/test_api_v3_calendar_credentials.py

@@ -0,0 +1,214 @@
+"""
+Endpoint tests for POST /plugins/calendar/upload-credentials.
+
+The endpoint takes an uploaded Google OAuth credentials file, writes it
+into the calendar plugin's directory as credentials.json at mode 0600, and
+copies any previous file aside first. It had no tests.
+
+Regression coverage for two fixed bugs:
+- The OAuth-shape check sat inside `except Exception: pass`, so a valid
+  JSON document that is not an object — a bare `42`, a list, a string —
+  raised TypeError on the membership test, was swallowed, and got saved
+  as credentials.json anyway.
+- Each overwrite created a timestamped backup and nothing ever removed
+  them, so every re-upload left another complete copy of the user's OAuth
+  client credentials in the plugin directory, indefinitely.
+"""
+
+import io
+import json
+import os
+import stat
+import sys
+import time
+from pathlib import Path
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+URL = "/api/v3/plugins/calendar/upload-credentials"
+
+VALID_CREDENTIALS = {
+    "installed": {
+        "client_id": "abc.apps.googleusercontent.com",
+        "client_secret": "shh",
+        "redirect_uris": ["http://localhost"],
+    }
+}
+
+
+@pytest.fixture
+def plugin_dir(tmp_path, api_v3_module):
+    directory = tmp_path / "plugins" / "calendar"
+    directory.mkdir(parents=True)
+    api_v3_module.api_v3.plugin_manager.get_plugin_directory.return_value = str(directory)
+    return directory
+
+
+def upload(client, content, filename="credentials.json"):
+    # bytes are sent verbatim (to exercise malformed input); anything else
+    # is serialized, so None becomes the JSON literal null rather than an
+    # empty body.
+    payload = content if isinstance(content, bytes) else json.dumps(content).encode()
+    return client.post(
+        URL,
+        data={"file": (io.BytesIO(payload), filename)},
+        content_type="multipart/form-data",
+    )
+
+
+def backups(plugin_dir):
+    return sorted(plugin_dir.glob("credentials.json.backup.*"))
+
+
+class TestRequestValidation:
+    def test_no_file_part_is_a_400(self, api_v3_client, plugin_dir):
+        response = api_v3_client.post(URL, data={}, content_type="multipart/form-data")
+        assert response.status_code == 400
+        assert "No file provided" in response.get_json()["message"]
+
+    def test_empty_filename_is_a_400(self, api_v3_client, plugin_dir):
+        response = upload(api_v3_client, VALID_CREDENTIALS, filename="")
+        assert response.status_code == 400
+
+    @pytest.mark.parametrize("filename", ["creds.txt", "creds.pem", "creds"])
+    def test_non_json_extension_is_a_400(self, api_v3_client, plugin_dir, filename):
+        response = upload(api_v3_client, VALID_CREDENTIALS, filename=filename)
+        assert response.status_code == 400
+        assert "JSON file" in response.get_json()["message"]
+
+    def test_uppercase_json_extension_accepted(self, api_v3_client, plugin_dir):
+        assert upload(api_v3_client, VALID_CREDENTIALS,
+                      filename="CREDENTIALS.JSON").status_code == 200
+
+    def test_oversized_file_is_a_400(self, api_v3_client, plugin_dir):
+        response = upload(api_v3_client, b"x" * (1024 * 1024 + 1))
+        assert response.status_code == 400
+        assert "1MB" in response.get_json()["message"]
+        assert not (plugin_dir / "credentials.json").exists()
+
+    def test_invalid_json_is_a_400(self, api_v3_client, plugin_dir):
+        response = upload(api_v3_client, b"{not json")
+        assert response.status_code == 400
+        assert "not valid JSON" in response.get_json()["message"]
+        assert not (plugin_dir / "credentials.json").exists()
+
+    def test_missing_plugin_directory_is_a_404(self, api_v3_client, api_v3_module, tmp_path):
+        api_v3_module.api_v3.plugin_manager.get_plugin_directory.return_value = str(
+            tmp_path / "not-installed")
+        assert upload(api_v3_client, VALID_CREDENTIALS).status_code == 404
+
+
+class TestOAuthShapeValidation:
+    def test_installed_key_accepted(self, api_v3_client, plugin_dir):
+        assert upload(api_v3_client, VALID_CREDENTIALS).status_code == 200
+
+    def test_web_key_accepted(self, api_v3_client, plugin_dir):
+        assert upload(api_v3_client, {"web": {"client_id": "x"}}).status_code == 200
+
+    def test_object_without_oauth_keys_is_a_400(self, api_v3_client, plugin_dir):
+        response = upload(api_v3_client, {"something": "else"})
+        assert response.status_code == 400
+        assert "valid Google OAuth" in response.get_json()["message"]
+        assert not (plugin_dir / "credentials.json").exists()
+
+    @pytest.mark.parametrize("content", [42, "a string", [1, 2, 3], True, None])
+    def test_valid_json_that_is_not_an_object_is_rejected(
+            self, api_v3_client, plugin_dir, content):
+        # Regression: `'installed' not in 42` raises TypeError, which the
+        # bare `except Exception: pass` swallowed — the file was then saved
+        # as credentials.json despite being unusable as credentials.
+        response = upload(api_v3_client, content)
+        assert response.status_code == 400
+        assert "valid Google OAuth" in response.get_json()["message"]
+        assert not (plugin_dir / "credentials.json").exists()
+
+
+class TestSaving:
+    def test_file_written_with_contents_intact(self, api_v3_client, plugin_dir):
+        response = upload(api_v3_client, VALID_CREDENTIALS)
+        assert response.status_code == 200
+        saved = json.loads((plugin_dir / "credentials.json").read_text())
+        assert saved == VALID_CREDENTIALS
+
+    def test_response_reports_the_path(self, api_v3_client, plugin_dir):
+        body = upload(api_v3_client, VALID_CREDENTIALS).get_json()
+        assert body["path"].endswith("credentials.json")
+
+    def test_permissions_are_owner_only(self, api_v3_client, plugin_dir):
+        upload(api_v3_client, VALID_CREDENTIALS)
+        mode = stat.S_IMODE((plugin_dir / "credentials.json").stat().st_mode)
+        assert mode == 0o600
+
+    def test_first_upload_creates_no_backup(self, api_v3_client, plugin_dir):
+        upload(api_v3_client, VALID_CREDENTIALS)
+        assert backups(plugin_dir) == []
+
+    def test_overwrite_backs_up_the_previous_file(self, api_v3_client, plugin_dir):
+        (plugin_dir / "credentials.json").write_text(json.dumps({"installed": {"old": 1}}))
+        upload(api_v3_client, VALID_CREDENTIALS)
+        assert len(backups(plugin_dir)) == 1
+        assert json.loads(backups(plugin_dir)[0].read_text()) == {"installed": {"old": 1}}
+        assert json.loads((plugin_dir / "credentials.json").read_text()) == VALID_CREDENTIALS
+
+
+class TestBackupPruning:
+    def _seed(self, plugin_dir, count):
+        """Create `count` backups with distinct, increasing mtimes."""
+        now = int(time.time())
+        for i in range(count):
+            path = plugin_dir / f"credentials.json.backup.{now - (count - i) * 10}"
+            path.write_text(json.dumps({"installed": {"gen": i}}))
+            os.utime(path, (now - (count - i) * 10, now - (count - i) * 10))
+
+    def test_old_backups_are_pruned(self, api_v3_client, plugin_dir):
+        # Regression: nothing ever removed these, so a plugin directory
+        # accumulated one full copy of the user's OAuth credentials per
+        # re-upload, forever.
+        (plugin_dir / "credentials.json").write_text(json.dumps({"installed": {"cur": 1}}))
+        self._seed(plugin_dir, 7)
+        assert len(backups(plugin_dir)) == 7
+
+        upload(api_v3_client, VALID_CREDENTIALS)
+        assert len(backups(plugin_dir)) == 5
+
+    def test_the_newest_backups_are_the_ones_kept(self, api_v3_client, plugin_dir):
+        (plugin_dir / "credentials.json").write_text(json.dumps({"installed": {"cur": 1}}))
+        self._seed(plugin_dir, 7)
+
+        upload(api_v3_client, VALID_CREDENTIALS)
+        remaining = backups(plugin_dir)
+        # The just-created backup (of "cur") plus the four newest seeds.
+        contents = [json.loads(p.read_text()) for p in remaining]
+        assert {"installed": {"cur": 1}} in contents
+        assert {"installed": {"gen": 0}} not in contents  # oldest seed gone
+
+    def test_under_the_limit_nothing_is_removed(self, api_v3_client, plugin_dir):
+        (plugin_dir / "credentials.json").write_text(json.dumps({"installed": {"cur": 1}}))
+        self._seed(plugin_dir, 2)
+        upload(api_v3_client, VALID_CREDENTIALS)
+        assert len(backups(plugin_dir)) == 3  # 2 seeded + 1 new
+
+    def test_repeated_uploads_stay_bounded(self, api_v3_client, plugin_dir):
+        for i in range(10):
+            upload(api_v3_client, {"installed": {"round": i}})
+            # Distinct mtimes so ordering is well-defined between rounds.
+            for path in backups(plugin_dir):
+                os.utime(path, (path.stat().st_mtime, path.stat().st_mtime))
+            time.sleep(0.01)
+        assert len(backups(plugin_dir)) <= 5
+
+    def test_unremovable_backup_does_not_fail_the_upload(
+            self, api_v3_client, plugin_dir, monkeypatch):
+        (plugin_dir / "credentials.json").write_text(json.dumps({"installed": {"cur": 1}}))
+        self._seed(plugin_dir, 7)
+
+        def refuse(self):
+            raise OSError("read-only filesystem")
+        monkeypatch.setattr(Path, "unlink", refuse)
+
+        # Pruning is housekeeping; failing it must not lose the upload.
+        assert upload(api_v3_client, VALID_CREDENTIALS).status_code == 200

test/test_api_v3_music_auth_endpoints.py

@@ -0,0 +1,302 @@
+"""
+Endpoint tests for /plugins/authenticate/spotify and .../ytm.
+
+The Spotify step-2 handler writes a Python wrapper script to a temp file
+with the user's redirect URL embedded in it, then runs that file through
+subprocess. That is the most dangerous shape in the blueprint and had no
+tests: the URL is user input reaching generated source code.
+
+The two endpoints are NOT symmetrical, despite the matching names. Only
+Spotify has a two-step flow, a wrapper script, and a redirect_url; YTM
+just runs its script directly.
+
+Regression coverage for one fixed bug: the wrapper file was unlinked in
+the success/failure branch and again in the TimeoutExpired handler, so
+any other failure from subprocess.run — the interpreter missing, a fork
+failure, an interrupted call — left a temp file containing the user's
+redirect URL behind.
+"""
+
+import ast
+import json
+import os
+import subprocess
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+
+@pytest.fixture
+def plugin_dir(tmp_path, api_v3_module):
+    """A plugin directory containing both auth scripts."""
+    directory = tmp_path / "plugins" / "ledmatrix-music"
+    directory.mkdir(parents=True)
+    (directory / "authenticate_spotify.py").write_text("print('spotify')\n")
+    (directory / "authenticate_ytm.py").write_text("print('ytm')\n")
+    api_v3_module.api_v3.plugin_manager.get_plugin_directory.return_value = str(directory)
+    return directory
+
+
+def completed(returncode=0, stdout="ok", stderr=""):
+    return subprocess.CompletedProcess(
+        args=["python3"], returncode=returncode, stdout=stdout, stderr=stderr)
+
+
+class TestSpotifyPreconditions:
+    URL = "/api/v3/plugins/authenticate/spotify"
+
+    def test_missing_plugin_directory_is_404(self, api_v3_client, api_v3_module, tmp_path):
+        api_v3_module.api_v3.plugin_manager.get_plugin_directory.return_value = str(
+            tmp_path / "not-installed")
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 404
+        assert response.get_json()["message"] == "Plugin not found"
+
+    def test_none_plugin_directory_is_404(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_manager.get_plugin_directory.return_value = None
+        assert api_v3_client.post(self.URL, json={}).status_code == 404
+
+    def test_missing_auth_script_is_404(self, api_v3_client, plugin_dir):
+        (plugin_dir / "authenticate_spotify.py").unlink()
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 404
+        assert "script not found" in response.get_json()["message"]
+
+
+class TestSpotifyStepTwo:
+    """redirect_url present — the wrapper-script path."""
+
+    URL = "/api/v3/plugins/authenticate/spotify"
+
+    def test_success(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed(0, "done")):
+            response = api_v3_client.post(self.URL, json={"redirect_url": "http://cb/?code=x"})
+        assert response.status_code == 200
+        body = response.get_json()
+        assert body["status"] == "success"
+        assert body["output"] == "done"
+
+    def test_script_failure_is_a_400_with_combined_output(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed(1, "out", "err")):
+            response = api_v3_client.post(self.URL, json={"redirect_url": "http://cb/?code=x"})
+        assert response.status_code == 400
+        assert response.get_json()["output"] == "outerr"
+
+    def test_timeout_is_a_408(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run",
+                          side_effect=subprocess.TimeoutExpired("python3", 120)):
+            response = api_v3_client.post(self.URL, json={"redirect_url": "http://cb/?code=x"})
+        assert response.status_code == 408
+        assert "timed out" in response.get_json()["message"]
+
+    def test_runs_a_list_argv_never_a_shell(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed()) as run:
+            api_v3_client.post(self.URL, json={"redirect_url": "http://cb/?code=x"})
+        args, kwargs = run.call_args
+        assert isinstance(args[0], list)
+        assert args[0][0] == "python3"
+        assert kwargs.get("shell") in (None, False)
+
+    def test_timeout_is_bounded(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed()) as run:
+            api_v3_client.post(self.URL, json={"redirect_url": "http://cb/?code=x"})
+        assert run.call_args.kwargs["timeout"] == 120
+
+
+class TestSpotifyWrapperCleanup:
+    URL = "/api/v3/plugins/authenticate/spotify"
+
+    def _wrapper_paths_after(self, api_v3_client, run_mock):
+        """Run the endpoint and return the wrapper path subprocess saw."""
+        seen = {}
+
+        def capture(args, **kwargs):
+            seen["path"] = args[1]
+            return run_mock(args, **kwargs)
+
+        with patch.object(subprocess, "run", side_effect=capture):
+            api_v3_client.post(self.URL, json={"redirect_url": "http://cb/?code=x"})
+        return seen["path"]
+
+    def test_removed_after_success(self, api_v3_client, plugin_dir):
+        path = self._wrapper_paths_after(api_v3_client, lambda *a, **kw: completed())
+        assert not os.path.exists(path)
+
+    def test_removed_after_script_failure(self, api_v3_client, plugin_dir):
+        path = self._wrapper_paths_after(
+            api_v3_client, lambda *a, **kw: completed(1, "out", "err"))
+        assert not os.path.exists(path)
+
+    def test_removed_after_timeout(self, api_v3_client, plugin_dir):
+        def raise_timeout(*a, **kw):
+            raise subprocess.TimeoutExpired("python3", 120)
+        path = self._wrapper_paths_after(api_v3_client, raise_timeout)
+        assert not os.path.exists(path)
+
+    def test_removed_when_subprocess_cannot_start(self, api_v3_client, plugin_dir):
+        # Regression: cleanup lived in the success/failure branch and in the
+        # TimeoutExpired handler only. An OSError from subprocess.run itself
+        # — no interpreter, fork failure — skipped both and left the wrapper,
+        # which contains the user's redirect URL, on disk.
+        def raise_oserror(*a, **kw):
+            raise OSError("[Errno 12] Cannot allocate memory")
+        path = self._wrapper_paths_after(api_v3_client, raise_oserror)
+        assert not os.path.exists(path)
+
+
+class TestSpotifyRedirectUrlIsNotInjectable:
+    """The wrapper embeds redirect_url into generated Python source."""
+
+    URL = "/api/v3/plugins/authenticate/spotify"
+
+    ADVERSARIAL = [
+        '''http://cb/?code=x"''',
+        """http://cb/?code=x'""",
+        'http://cb/?code=x\\',
+        'http://cb/?code=x\nimport os; os.system("id")',
+        'http://cb/?code=x"""\nimport os\n"""',
+        "http://cb/?code=x'''",
+        'http://cb/?code=x\\"\\n',
+        '"; import os; os.system("id"); "',
+    ]
+
+    def _wrapper_source(self, api_v3_client, redirect_url):
+        captured = {}
+
+        def capture(args, **kwargs):
+            captured["source"] = Path(args[1]).read_text()
+            return completed()
+
+        with patch.object(subprocess, "run", side_effect=capture):
+            api_v3_client.post(self.URL, json={"redirect_url": redirect_url})
+        return captured["source"]
+
+    @pytest.mark.parametrize("redirect_url", ADVERSARIAL)
+    def test_wrapper_is_still_valid_python(self, api_v3_client, plugin_dir, redirect_url):
+        # If escaping failed, the generated file would not parse at all.
+        source = self._wrapper_source(api_v3_client, redirect_url)
+        ast.parse(source)
+
+    @pytest.mark.parametrize("redirect_url", ADVERSARIAL)
+    def test_url_survives_as_one_string_literal(
+            self, api_v3_client, plugin_dir, redirect_url):
+        # Stronger than "it parses": the URL must still be a single string
+        # assigned to redirect_url, not code that escaped into statements.
+        source = self._wrapper_source(api_v3_client, redirect_url)
+        tree = ast.parse(source)
+        assigned = [
+            node.value.value for node in ast.walk(tree)
+            if isinstance(node, ast.Assign)
+            and isinstance(node.value, ast.Constant)
+            and any(getattr(t, "id", None) == "redirect_url" for t in node.targets)
+        ]
+        assert assigned == [redirect_url.strip()]
+
+    def test_injected_call_does_not_become_a_statement(self, api_v3_client, plugin_dir):
+        source = self._wrapper_source(
+            api_v3_client, 'http://cb/\nimport os; os.system("id")')
+        tree = ast.parse(source)
+        imported = {
+            alias.name for node in ast.walk(tree)
+            if isinstance(node, ast.Import) for alias in node.names
+        }
+        # The wrapper legitimately imports sys, subprocess and os; what it
+        # must not gain is a *call* smuggled in through the URL.
+        calls = [
+            node for node in ast.walk(tree)
+            if isinstance(node, ast.Call)
+            and isinstance(node.func, ast.Attribute)
+            and node.func.attr == "system"
+        ]
+        assert calls == []
+
+
+class TestSpotifyStepOne:
+    """No redirect_url — the OAuth-URL path, which imports the script."""
+
+    URL = "/api/v3/plugins/authenticate/spotify"
+
+    def test_script_without_credentials_helper_is_an_error(
+            self, api_v3_client, plugin_dir):
+        # The stub script defines neither get_auth_url nor
+        # load_spotify_credentials, so no URL can be produced.
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code in (400, 500)
+        assert response.get_json()["status"] == "error"
+
+    def test_unusable_credentials_do_not_leak_into_the_response(
+            self, api_v3_client, plugin_dir):
+        (plugin_dir / "authenticate_spotify.py").write_text(
+            "def load_spotify_credentials():\n"
+            "    return ('id-abc', 'super-secret-value', None)\n"
+        )
+        response = api_v3_client.post(self.URL, json={})
+        assert "super-secret-value" not in response.get_data(as_text=True)
+
+    def test_script_raising_on_import_is_handled(self, api_v3_client, plugin_dir):
+        (plugin_dir / "authenticate_spotify.py").write_text("raise RuntimeError('boom')\n")
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 500
+        assert response.get_json()["status"] == "error"
+
+    def test_bodyless_post_reaches_step_one(self, api_v3_client, plugin_dir):
+        # Covered by the silent=True fix: previously a 500 from body parsing.
+        response = api_v3_client.post(self.URL)
+        assert response.status_code in (400, 500)
+        assert response.get_json()["status"] == "error"
+
+    def test_whitespace_redirect_url_is_treated_as_absent(
+            self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed()) as run:
+            api_v3_client.post(self.URL, json={"redirect_url": "   "})
+        # Step 2 never runs, so no wrapper is executed.
+        run.assert_not_called()
+
+
+class TestYouTubeMusic:
+    """No wrapper script and no redirect_url — deliberately not symmetric."""
+
+    URL = "/api/v3/plugins/authenticate/ytm"
+
+    def test_missing_plugin_directory_is_404(self, api_v3_client, api_v3_module, tmp_path):
+        api_v3_module.api_v3.plugin_manager.get_plugin_directory.return_value = str(
+            tmp_path / "not-installed")
+        assert api_v3_client.post(self.URL).status_code == 404
+
+    def test_missing_script_is_404(self, api_v3_client, plugin_dir):
+        (plugin_dir / "authenticate_ytm.py").unlink()
+        response = api_v3_client.post(self.URL)
+        assert response.status_code == 404
+        assert "script not found" in response.get_json()["message"]
+
+    def test_success(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed(0, "authorized")):
+            response = api_v3_client.post(self.URL)
+        assert response.status_code == 200
+        assert response.get_json()["output"] == "authorized"
+
+    def test_failure_is_a_400_with_combined_output(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed(1, "out", "err")):
+            response = api_v3_client.post(self.URL)
+        assert response.status_code == 400
+        assert response.get_json()["output"] == "outerr"
+
+    def test_timeout_is_a_408(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run",
+                          side_effect=subprocess.TimeoutExpired("python3", 60)):
+            assert api_v3_client.post(self.URL).status_code == 408
+
+    def test_runs_the_script_directly_without_a_shell(self, api_v3_client, plugin_dir):
+        with patch.object(subprocess, "run", return_value=completed()) as run:
+            api_v3_client.post(self.URL)
+        args, kwargs = run.call_args
+        assert args[0][0] == "python3"
+        assert args[0][1].endswith("authenticate_ytm.py")
+        assert kwargs.get("shell") in (None, False)
+        assert kwargs["timeout"] == 60

test/test_api_v3_optional_body.py

@@ -0,0 +1,136 @@
+"""
+Regression tests: POST endpoints whose body is optional must accept a
+request that has no body at all.
+
+Six handlers in api_v3 read their body as ``request.get_json() or {}``.
+The ``or {}`` states the intent plainly — every field is optional, so a
+bodyless POST should fall back to defaults. But ``get_json()`` without
+``silent=True`` raises ``UnsupportedMediaType`` when the request carries
+no JSON Content-Type, and it raises *before* ``or {}`` is evaluated. Each
+handler's catch-all then turned that into a 500.
+
+So the natural way to call these endpoints — a POST with no body, which
+is what curl, a fetch() without options, and most HTTP clients send by
+default — failed on every one of them. The shipped UI always sends a JSON
+object, which is why this went unnoticed.
+
+This file covers the endpoints whose bodyless behaviour is not already
+tested in their own suite.
+"""
+
+import re
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+
+class TestOnDemandStart:
+    URL = "/api/v3/display/on-demand/start"
+
+    def test_bodyless_post_is_not_a_server_error(self, api_v3_client, api_v3_module):
+        response = api_v3_client.post(self.URL)
+        # The endpoint may still reject the request on its own terms (no
+        # plugin_id, nothing to display); what it must not do is fail with
+        # a 500 raised out of body parsing.
+        assert response.status_code != 500
+
+    def test_json_body_still_works(self, api_v3_client, api_v3_module):
+        assert api_v3_client.post(self.URL, json={}).status_code != 500
+
+
+class TestResetPluginConfig:
+    URL = "/api/v3/plugins/config/reset"
+
+    def test_bodyless_post_is_not_a_server_error(self, api_v3_client, api_v3_module):
+        assert api_v3_client.post(self.URL).status_code != 500
+
+    def test_json_body_still_works(self, api_v3_client, api_v3_module):
+        assert api_v3_client.post(self.URL, json={}).status_code != 500
+
+
+class TestDeleteOfTheDayJson:
+    URL = "/api/v3/plugins/of-the-day/json/delete"
+
+    def test_bodyless_post_is_not_a_server_error(self, api_v3_client, api_v3_module):
+        assert api_v3_client.post(self.URL).status_code != 500
+
+    def test_json_body_still_works(self, api_v3_client, api_v3_module):
+        assert api_v3_client.post(self.URL, json={}).status_code != 500
+
+
+class TestPluginLimits:
+    URL = "/api/v3/plugins/clock/limits"
+
+    def test_bodyless_post_is_not_a_server_error(self, api_v3_client, api_v3_module):
+        assert api_v3_client.post(self.URL).status_code != 500
+
+
+class TestMissingBodyGivesTheDeclaredError:
+    """Handlers that answer "No data provided" must actually be able to.
+
+    A second group of handlers reads `data = request.get_json()` and then
+    guards with `if not data: return 400`. That guard is unreachable for a
+    request with no JSON body, because get_json() raises first — so the
+    caller got a 500 "an error occurred; see logs for details" instead of
+    the 400 the handler plainly intends to send.
+    """
+
+    @pytest.mark.parametrize("url", [
+        "/api/v3/plugins/install",
+        "/api/v3/plugins/install-from-url",
+        "/api/v3/plugins/registry-from-url",
+        "/api/v3/config/raw/main",
+        "/api/v3/config/raw/secrets",
+        "/api/v3/cache/delete",
+    ])
+    def test_bodyless_post_gets_a_400_not_a_500(self, api_v3_client, api_v3_module, url):
+        response = api_v3_client.post(url)
+        assert response.status_code == 400, (
+            f"{url} answered {response.status_code}: "
+            f"{response.get_data(as_text=True)[:200]}")
+
+    @pytest.mark.parametrize("url", [
+        "/api/v3/plugins/install",
+        "/api/v3/config/raw/main",
+    ])
+    def test_malformed_json_gets_a_400_not_a_500(self, api_v3_client, api_v3_module, url):
+        response = api_v3_client.post(
+            url, data="{not json", content_type="application/json")
+        assert response.status_code == 400
+
+
+class TestNoBodyReadContradictsItsOwnGuard:
+    SOURCE = Path(__file__).parent.parent / "web_interface/blueprints/api_v3.py"
+
+    def test_no_or_default_read_is_unguarded(self):
+        """`get_json() or <default>` is a contradiction without silent=True.
+
+        Writing `or {}` declares the body optional; omitting silent=True
+        means the call raises before the default can apply.
+        """
+        offenders = [
+            line.strip() for line in self.SOURCE.read_text().splitlines()
+            if "request.get_json()" in line and " or " in line
+        ]
+        assert offenders == [], (
+            "these reads declare a default but raise before reaching it; "
+            f"use get_json(silent=True): {offenders}")
+
+    def test_no_not_data_guard_is_unreachable(self):
+        """A `if not data:` guard needs a read that can actually return None."""
+        lines = self.SOURCE.read_text().splitlines()
+        offenders = []
+        for i, line in enumerate(lines):
+            if re.search(r"=\s*request\.get_json\(\)\s*$", line):
+                window = "\n".join(lines[i + 1:i + 3])
+                if re.search(r"if\s+(not\s+data\b|data\s+is\s+None)", window):
+                    offenders.append(f"line {i + 1}: {line.strip()}")
+        assert offenders == [], (
+            "these handlers guard on a missing body but raise before the "
+            f"guard runs; use get_json(silent=True): {offenders}")

test/test_api_v3_plugin_install_endpoints.py

@@ -0,0 +1,302 @@
+"""
+Endpoint tests for POST /plugins/install and POST /plugins/install-from-url.
+
+Both were only ever tested at the PluginStoreManager layer, so the route
+logic — the queue-vs-direct branch, schema invalidation, plugin discovery,
+state and history recording — was unexercised.
+
+/plugins/install carries the same install logic twice: once inside the
+operation-queue callback and once in the direct fallback. The paired
+tests below assert both branches produce the same side effects, so the
+duplication cannot quietly drift.
+"""
+
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+INSTALL = "/api/v3/plugins/install"
+FROM_URL = "/api/v3/plugins/install-from-url"
+
+
+@pytest.fixture
+def queued(api_v3_module):
+    """Enable the operation queue and run its callback synchronously."""
+    queue = MagicMock()
+
+    def enqueue(operation_type, plugin_id, operation_callback=None):
+        queue.callback_result = operation_callback(MagicMock())
+        return "op-123"
+
+    queue.enqueue_operation.side_effect = enqueue
+    api_v3_module.api_v3.operation_queue = queue
+    return queue
+
+
+def side_effects(module):
+    """The manager calls a successful install is expected to make."""
+    api = module.api_v3
+    return {
+        "schema_invalidated": api.schema_manager.invalidate_cache.call_args_list,
+        "discovered": api.plugin_manager.discover_plugins.call_count,
+        "loaded": api.plugin_manager.load_plugin.call_args_list,
+        "state_set": api.plugin_state_manager.set_plugin_installed.call_args_list,
+        "history": api.operation_history.record_operation.call_args_list,
+    }
+
+
+class TestInstallValidation:
+    def test_uninitialized_store_manager_is_a_500(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager = None
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert response.status_code == 500
+        assert "not initialized" in response.get_json()["message"]
+
+    def test_missing_plugin_id_is_a_400(self, api_v3_client, api_v3_module):
+        response = api_v3_client.post(INSTALL, json={})
+        assert response.status_code == 400
+        assert "plugin_id required" in response.get_json()["message"]
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.assert_not_called()
+
+    def test_empty_body_is_a_400(self, api_v3_client, api_v3_module):
+        assert api_v3_client.post(INSTALL, json=None).status_code == 400
+
+
+class TestInstallDirectPath:
+    """operation_queue is None — the fallback branch."""
+
+    def test_success(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert response.status_code == 200
+        assert response.get_json()["status"] == "success"
+
+    def test_success_side_effects(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        effects = side_effects(api_v3_module)
+        assert effects["schema_invalidated"] == [(("clock",), {})]
+        assert effects["discovered"] == 1
+        assert effects["loaded"] == [(("clock",), {})]
+        assert effects["state_set"] == [(("clock",), {})]
+        assert effects["history"][0].kwargs["status"] == "success"
+
+    def test_branch_forwarded_to_the_manager(self, api_v3_client, api_v3_module):
+        manager = api_v3_module.api_v3.plugin_store_manager
+        manager.install_plugin.return_value = True
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock", "branch": "dev"})
+        manager.install_plugin.assert_called_once_with("clock", branch="dev")
+
+    def test_branch_named_in_the_message(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock", "branch": "dev"})
+        assert "(branch: dev)" in response.get_json()["message"]
+
+    def test_failure_is_a_500(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = False
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert response.status_code == 500
+        assert "Failed to install" in response.get_json()["message"]
+
+    def test_failure_mentions_missing_registry_entry(self, api_v3_client, api_v3_module):
+        manager = api_v3_module.api_v3.plugin_store_manager
+        manager.install_plugin.return_value = False
+        manager.get_plugin_info.return_value = None
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "ghost"})
+        assert "not found in registry" in response.get_json()["message"]
+
+    def test_failure_omits_registry_note_when_plugin_is_known(
+            self, api_v3_client, api_v3_module):
+        manager = api_v3_module.api_v3.plugin_store_manager
+        manager.install_plugin.return_value = False
+        manager.get_plugin_info.return_value = {"id": "clock"}
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert "not found in registry" not in response.get_json()["message"]
+
+    def test_failure_recorded_in_history(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = False
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        record = api_v3_module.api_v3.operation_history.record_operation.call_args
+        assert record.kwargs["status"] == "failed"
+
+    def test_no_side_effects_on_failure(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = False
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        effects = side_effects(api_v3_module)
+        assert effects["schema_invalidated"] == []
+        assert effects["loaded"] == []
+        assert effects["state_set"] == []
+
+
+class TestInstallQueuedPath:
+    """operation_queue present — the callback branch."""
+
+    def test_returns_an_operation_id(self, api_v3_client, api_v3_module, queued):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert response.status_code == 200
+        assert response.get_json()["data"]["operation_id"] == "op-123"
+
+    def test_message_says_queued(self, api_v3_client, api_v3_module, queued):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert "queued" in response.get_json()["message"]
+
+    def test_callback_success_side_effects(self, api_v3_client, api_v3_module, queued):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        effects = side_effects(api_v3_module)
+        assert effects["schema_invalidated"] == [(("clock",), {})]
+        assert effects["discovered"] == 1
+        assert effects["loaded"] == [(("clock",), {})]
+        assert effects["state_set"] == [(("clock",), {})]
+        assert effects["history"][0].kwargs["status"] == "success"
+
+    def test_callback_reports_success(self, api_v3_client, api_v3_module, queued):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert queued.callback_result["success"] is True
+
+    def test_callback_failure_raises_for_the_queue(self, api_v3_client, api_v3_module, queued):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = False
+        # The callback signals failure by raising, so the queue can mark the
+        # operation failed; the route's catch-all turns it into a 500.
+        response = api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        assert response.status_code == 500
+
+    def test_callback_failure_recorded_in_history(self, api_v3_client, api_v3_module, queued):
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = False
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock"})
+        record = api_v3_module.api_v3.operation_history.record_operation.call_args
+        assert record.kwargs["status"] == "failed"
+
+    def test_branch_forwarded_from_the_callback(self, api_v3_client, api_v3_module, queued):
+        manager = api_v3_module.api_v3.plugin_store_manager
+        manager.install_plugin.return_value = True
+        api_v3_client.post(INSTALL, json={"plugin_id": "clock", "branch": "dev"})
+        manager.install_plugin.assert_called_once_with("clock", branch="dev")
+
+
+class TestInstallPathsAgree:
+    """The queue callback and the direct fallback duplicate the same logic."""
+
+    def _run(self, client, module, install_ok, queue):
+        module.api_v3.plugin_store_manager.install_plugin.return_value = install_ok
+        client.post(INSTALL, json={"plugin_id": "clock", "branch": "dev"})
+        return side_effects(module)
+
+    def test_success_side_effects_match(self, api_v3_client, api_v3_module):
+        direct = self._run(api_v3_client, api_v3_module, True, None)
+
+        # Reset and re-run through the queue.
+        for mock in (api_v3_module.api_v3.schema_manager,
+                     api_v3_module.api_v3.plugin_manager,
+                     api_v3_module.api_v3.plugin_state_manager,
+                     api_v3_module.api_v3.operation_history):
+            mock.reset_mock()
+        queue = MagicMock()
+        queue.enqueue_operation.side_effect = (
+            lambda t, p, operation_callback=None: operation_callback(MagicMock()) and "op")
+        api_v3_module.api_v3.operation_queue = queue
+        queued = self._run(api_v3_client, api_v3_module, True, queue)
+
+        assert direct["schema_invalidated"] == queued["schema_invalidated"]
+        assert direct["discovered"] == queued["discovered"]
+        assert direct["loaded"] == queued["loaded"]
+        assert direct["state_set"] == queued["state_set"]
+        assert (direct["history"][0].kwargs["status"]
+                == queued["history"][0].kwargs["status"])
+        assert (direct["history"][0].kwargs["details"]
+                == queued["history"][0].kwargs["details"])
+
+    def test_only_the_message_wording_differs(self, api_v3_client, api_v3_module):
+        # Characterized: the direct path says "Plugin installed
+        # successfully" while the queue callback says "Plugin clock
+        # installed successfully". Cosmetic, and the queue's text is
+        # internal to the operation record rather than the HTTP response.
+        api_v3_module.api_v3.plugin_store_manager.install_plugin.return_value = True
+        direct = api_v3_client.post(INSTALL, json={"plugin_id": "clock"}).get_json()
+        assert direct["message"] == "Plugin installed successfully"
+
+
+class TestInstallFromUrl:
+    def test_uninitialized_store_manager_is_a_500(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager = None
+        assert api_v3_client.post(FROM_URL, json={"repo_url": "http://x"}).status_code == 500
+
+    def test_missing_repo_url_is_a_400(self, api_v3_client, api_v3_module):
+        response = api_v3_client.post(FROM_URL, json={})
+        assert response.status_code == 400
+        assert "repo_url required" in response.get_json()["message"]
+
+    def test_success(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_from_url.return_value = {
+            "success": True, "plugin_id": "clock", "name": "Clock"}
+        response = api_v3_client.post(FROM_URL, json={"repo_url": "https://github.com/o/r"})
+        assert response.status_code == 200
+        body = response.get_json()
+        assert body["plugin_id"] == "clock"
+        assert body["name"] == "Clock"
+
+    def test_all_optional_arguments_forwarded(self, api_v3_client, api_v3_module):
+        manager = api_v3_module.api_v3.plugin_store_manager
+        manager.install_from_url.return_value = {"success": True, "plugin_id": "clock"}
+        api_v3_client.post(FROM_URL, json={
+            "repo_url": "  https://github.com/o/r  ",
+            "plugin_id": "clock",
+            "plugin_path": "plugins/clock",
+            "branch": "dev",
+        })
+        manager.install_from_url.assert_called_once_with(
+            repo_url="https://github.com/o/r",
+            plugin_id="clock",
+            plugin_path="plugins/clock",
+            branch="dev",
+        )
+
+    def test_success_invalidates_schema_and_loads_plugin(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_from_url.return_value = {
+            "success": True, "plugin_id": "clock"}
+        api_v3_client.post(FROM_URL, json={"repo_url": "http://x"})
+        api_v3_module.api_v3.schema_manager.invalidate_cache.assert_called_once_with("clock")
+        api_v3_module.api_v3.plugin_manager.load_plugin.assert_called_once_with("clock")
+
+    def test_success_without_plugin_id_skips_discovery(self, api_v3_client, api_v3_module):
+        # install_from_url can succeed without naming the plugin; there is
+        # then nothing to invalidate or load.
+        api_v3_module.api_v3.plugin_store_manager.install_from_url.return_value = {
+            "success": True, "plugin_id": None}
+        api_v3_client.post(FROM_URL, json={"repo_url": "http://x"})
+        api_v3_module.api_v3.schema_manager.invalidate_cache.assert_not_called()
+        api_v3_module.api_v3.plugin_manager.load_plugin.assert_not_called()
+
+    def test_branch_from_result_included(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_from_url.return_value = {
+            "success": True, "plugin_id": "clock", "branch": "dev"}
+        body = api_v3_client.post(FROM_URL, json={"repo_url": "http://x"}).get_json()
+        assert body["branch"] == "dev"
+        assert "(branch: dev)" in body["message"]
+
+    def test_failure_reports_the_managers_error(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_from_url.return_value = {
+            "success": False, "error": "repo not found"}
+        response = api_v3_client.post(FROM_URL, json={"repo_url": "http://x"})
+        assert response.status_code == 500
+        assert response.get_json()["message"] == "repo not found"
+
+    def test_failure_without_error_uses_fallback_text(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_from_url.return_value = {
+            "success": False}
+        response = api_v3_client.post(FROM_URL, json={"repo_url": "http://x"})
+        assert "Failed to install plugin from URL" in response.get_json()["message"]
+
+    def test_manager_exception_is_a_500(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.install_from_url.side_effect = (
+            RuntimeError("boom"))
+        assert api_v3_client.post(FROM_URL, json={"repo_url": "http://x"}).status_code == 500

test/test_api_v3_registry_endpoints.py

@@ -0,0 +1,174 @@
+"""
+Endpoint tests for the plugin-registry routes in api_v3:
+POST /plugins/store/refresh and POST /plugins/registry-from-url.
+
+Both reach out to the network through PluginStoreManager (mocked here) and
+had no endpoint-level coverage; registry-from-url in particular takes a
+user-supplied URL and hands it straight to the manager.
+"""
+
+import sys
+from pathlib import Path
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+
+class TestRefreshPluginStore:
+    URL = "/api/v3/plugins/store/refresh"
+
+    def test_uninitialized_manager_is_a_500(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager = None
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 500
+        assert "not initialized" in response.get_json()["message"]
+
+    def test_success_reports_plugin_count(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.return_value = {
+            "plugins": [{"id": "a"}, {"id": "b"}, {"id": "c"}]}
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 200
+        assert response.get_json()["plugin_count"] == 3
+
+    def test_forces_a_refresh_rather_than_using_cache(self, api_v3_client, api_v3_module):
+        manager = api_v3_module.api_v3.plugin_store_manager
+        manager.fetch_registry.return_value = {"plugins": []}
+        api_v3_client.post(self.URL, json={})
+        manager.fetch_registry.assert_called_once_with(force_refresh=True)
+
+    def test_empty_registry_reports_zero(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.return_value = {}
+        response = api_v3_client.post(self.URL, json={})
+        assert response.get_json()["plugin_count"] == 0
+
+    def test_no_body_is_accepted(self, api_v3_client, api_v3_module):
+        # Regression: `request.get_json() or {}` says a missing body is
+        # fine, but get_json() raises UnsupportedMediaType before `or {}`
+        # is reached, so a bodyless POST — the natural way to call a
+        # refresh endpoint — came back 500.
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.return_value = {"plugins": []}
+        assert api_v3_client.post(self.URL).status_code == 200
+
+    def test_body_without_json_content_type_is_accepted(
+            self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.return_value = {"plugins": []}
+        response = api_v3_client.post(self.URL, data="", content_type="text/plain")
+        assert response.status_code == 200
+
+    def test_malformed_json_body_falls_back_to_defaults(
+            self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.return_value = {"plugins": []}
+        response = api_v3_client.post(
+            self.URL, data="{not json", content_type="application/json")
+        assert response.status_code == 200
+
+    @pytest.mark.parametrize("key", ["fetch_commit_info", "fetch_latest_versions"])
+    def test_either_commit_info_key_extends_the_message(
+            self, api_v3_client, api_v3_module, key):
+        # fetch_latest_versions is the older spelling; both must work.
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.return_value = {"plugins": []}
+        response = api_v3_client.post(self.URL, json={key: True})
+        assert "commit metadata" in response.get_json()["message"]
+
+    def test_message_stays_plain_without_the_flag(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.return_value = {"plugins": []}
+        response = api_v3_client.post(self.URL, json={})
+        assert response.get_json()["message"] == "Plugin store refreshed"
+
+    def test_network_failure_is_a_500(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.side_effect = (
+            ConnectionError("github unreachable"))
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 500
+        assert response.get_json()["message"] == "An error occurred; see logs for details"
+
+    def test_failure_body_carries_no_traceback_or_paths(
+            self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry.side_effect = (
+            RuntimeError("failed at /home/user/LEDMatrix/src/secret.py line 42"))
+        body = api_v3_client.post(self.URL, json={}).get_json()
+        assert "Traceback" not in str(body)
+        # `details` is describe_exception output: one line, type-named,
+        # credential-redacted. It may quote the message, but never a stack.
+        assert body["details"].startswith("RuntimeError:")
+        assert "\n" not in body["details"]
+
+
+class TestRegistryFromUrl:
+    URL = "/api/v3/plugins/registry-from-url"
+
+    def test_uninitialized_manager_is_a_500(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager = None
+        response = api_v3_client.post(self.URL, json={"repo_url": "http://x"})
+        assert response.status_code == 500
+
+    def test_missing_repo_url_is_a_400(self, api_v3_client, api_v3_module):
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 400
+        assert "repo_url required" in response.get_json()["message"]
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.assert_not_called()
+
+    def test_success_returns_the_plugin_list(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.return_value = {
+            "plugins": [{"id": "clock"}]}
+        response = api_v3_client.post(
+            self.URL, json={"repo_url": "https://github.com/o/r"})
+        assert response.status_code == 200
+        body = response.get_json()
+        assert body["plugins"] == [{"id": "clock"}]
+        assert body["registry_url"] == "https://github.com/o/r"
+
+    def test_url_is_trimmed_before_use(self, api_v3_client, api_v3_module):
+        manager = api_v3_module.api_v3.plugin_store_manager
+        manager.fetch_registry_from_url.return_value = {"plugins": []}
+        api_v3_client.post(self.URL, json={"repo_url": "  https://github.com/o/r  "})
+        manager.fetch_registry_from_url.assert_called_once_with("https://github.com/o/r")
+
+    def test_registry_without_plugins_key_returns_empty_list(
+            self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.return_value = {
+            "other": 1}
+        response = api_v3_client.post(self.URL, json={"repo_url": "http://x"})
+        assert response.get_json()["plugins"] == []
+
+    def test_no_registry_found_is_a_400(self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.return_value = None
+        response = api_v3_client.post(self.URL, json={"repo_url": "http://x/not-a-registry"})
+        assert response.status_code == 400
+        assert "Failed to fetch registry" in response.get_json()["message"]
+
+    @pytest.mark.parametrize("url", [
+        "not a url",
+        "javascript:alert(1)",
+        "file:///etc/passwd",
+        "http://localhost:8080/admin",
+    ])
+    def test_unusable_urls_fail_cleanly(self, api_v3_client, api_v3_module, url):
+        # Characterization: the handler performs no URL validation of its
+        # own — whatever the manager makes of the URL decides the outcome.
+        # What is pinned here is that a rejected URL produces a clean 400
+        # rather than a traceback or a 500.
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.return_value = None
+        response = api_v3_client.post(self.URL, json={"repo_url": url})
+        assert response.status_code == 400
+        assert "Traceback" not in str(response.get_json())
+
+    def test_fetch_exception_is_a_500_without_internals(
+            self, api_v3_client, api_v3_module):
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.side_effect = (
+            ValueError("parse failed in /srv/app/internal.py"))
+        response = api_v3_client.post(self.URL, json={"repo_url": "http://x"})
+        assert response.status_code == 500
+        body = response.get_json()
+        assert body["message"] == "An error occurred; see logs for details"
+        assert "Traceback" not in str(body)
+
+    def test_non_string_repo_url_is_a_500_not_a_crash(
+            self, api_v3_client, api_v3_module):
+        # .strip() on a non-string raises; the handler's catch-all turns
+        # that into a 500 rather than propagating.
+        response = api_v3_client.post(self.URL, json={"repo_url": 12345})
+        assert response.status_code == 500

test/test_api_v3_wifi_endpoints.py

@@ -0,0 +1,240 @@
+"""
+Endpoint tests for the /wifi/* routes in api_v3.
+
+These routes drive the host's actual networking — connecting, dropping a
+connection, switching the radio off — and had no endpoint-level tests at
+all. WiFiManager is mocked throughout; nothing here may touch real
+networking.
+
+Each handler does `from src.wifi_manager import WiFiManager` inside the
+function body, so the patch target is the class at its definition site.
+"""
+
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+
+@pytest.fixture
+def wifi_manager():
+    """Patch WiFiManager where it is defined; yield the instance mock."""
+    with patch("src.wifi_manager.WiFiManager") as cls:
+        instance = MagicMock()
+        cls.return_value = instance
+        yield instance
+
+
+class TestConnect:
+    URL = "/api/v3/wifi/connect"
+
+    def test_success(self, api_v3_client, wifi_manager):
+        wifi_manager.connect_to_network.return_value = (True, "Connected to HomeNet")
+        response = api_v3_client.post(self.URL, json={"ssid": "HomeNet", "password": "pw"})
+        assert response.status_code == 200
+        assert response.get_json()["message"] == "Connected to HomeNet"
+        wifi_manager.connect_to_network.assert_called_once_with("HomeNet", "pw")
+
+    def test_missing_body_rejected(self, api_v3_client, wifi_manager):
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 400
+        wifi_manager.connect_to_network.assert_not_called()
+
+    def test_missing_ssid_rejected(self, api_v3_client, wifi_manager):
+        response = api_v3_client.post(self.URL, json={"password": "pw"})
+        assert response.status_code == 400
+        assert "SSID is required" in response.get_json()["message"]
+        wifi_manager.connect_to_network.assert_not_called()
+
+    @pytest.mark.parametrize("ssid", ["", "   ", "\t"])
+    def test_blank_ssid_rejected(self, api_v3_client, wifi_manager, ssid):
+        response = api_v3_client.post(self.URL, json={"ssid": ssid})
+        assert response.status_code == 400
+        wifi_manager.connect_to_network.assert_not_called()
+
+    def test_ssid_is_trimmed(self, api_v3_client, wifi_manager):
+        wifi_manager.connect_to_network.return_value = (True, "ok")
+        api_v3_client.post(self.URL, json={"ssid": "  HomeNet  "})
+        wifi_manager.connect_to_network.assert_called_once_with("HomeNet", "")
+
+    def test_missing_password_becomes_empty_string(self, api_v3_client, wifi_manager):
+        wifi_manager.connect_to_network.return_value = (True, "ok")
+        api_v3_client.post(self.URL, json={"ssid": "OpenNet"})
+        wifi_manager.connect_to_network.assert_called_once_with("OpenNet", "")
+
+    def test_null_password_becomes_empty_string(self, api_v3_client, wifi_manager):
+        wifi_manager.connect_to_network.return_value = (True, "ok")
+        api_v3_client.post(self.URL, json={"ssid": "OpenNet", "password": None})
+        wifi_manager.connect_to_network.assert_called_once_with("OpenNet", "")
+
+    def test_failure_reports_the_managers_reason(self, api_v3_client, wifi_manager):
+        wifi_manager.connect_to_network.return_value = (False, "Bad password")
+        response = api_v3_client.post(self.URL, json={"ssid": "HomeNet"})
+        assert response.status_code == 400
+        assert response.get_json()["message"] == "Bad password"
+
+    def test_failure_without_reason_uses_fallback_text(self, api_v3_client, wifi_manager):
+        wifi_manager.connect_to_network.return_value = (False, None)
+        response = api_v3_client.post(self.URL, json={"ssid": "HomeNet"})
+        assert response.status_code == 400
+        assert response.get_json()["message"] == "Failed to connect to network"
+
+    def test_manager_exception_is_a_500_without_leaking_internals(
+            self, api_v3_client, wifi_manager):
+        wifi_manager.connect_to_network.side_effect = RuntimeError(
+            "/usr/lib/secret/path blew up")
+        response = api_v3_client.post(self.URL, json={"ssid": "HomeNet"})
+        assert response.status_code == 500
+        body = response.get_json()
+        assert body["message"] == "An error occurred; see logs for details"
+        # `details` comes from describe_exception, which is deliberately
+        # safe to return (redacted, capped) — it names the type.
+        assert "RuntimeError" in body["details"]
+
+
+class TestDisconnect:
+    URL = "/api/v3/wifi/disconnect"
+
+    def test_success(self, api_v3_client, wifi_manager):
+        wifi_manager.disconnect_from_network.return_value = (True, "Disconnected")
+        response = api_v3_client.post(self.URL)
+        assert response.status_code == 200
+        assert response.get_json()["message"] == "Disconnected"
+
+    def test_failure(self, api_v3_client, wifi_manager):
+        wifi_manager.disconnect_from_network.return_value = (False, "Not connected")
+        response = api_v3_client.post(self.URL)
+        assert response.status_code == 400
+        assert response.get_json()["message"] == "Not connected"
+
+    def test_failure_without_reason_uses_fallback(self, api_v3_client, wifi_manager):
+        wifi_manager.disconnect_from_network.return_value = (False, "")
+        response = api_v3_client.post(self.URL)
+        assert response.get_json()["message"] == "Failed to disconnect from network"
+
+    def test_exception_is_a_500(self, api_v3_client, wifi_manager):
+        wifi_manager.disconnect_from_network.side_effect = OSError("nmcli missing")
+        assert api_v3_client.post(self.URL).status_code == 500
+
+
+class TestApMode:
+    ENABLE = "/api/v3/wifi/ap/enable"
+    DISABLE = "/api/v3/wifi/ap/disable"
+
+    def test_enable_success(self, api_v3_client, wifi_manager):
+        wifi_manager.enable_ap_mode.return_value = (True, "AP enabled")
+        response = api_v3_client.post(self.ENABLE, json={})
+        assert response.status_code == 200
+        wifi_manager.enable_ap_mode.assert_called_once_with(force=False)
+
+    @pytest.mark.parametrize("raw,expected", [
+        (True, True), (False, False),
+        ("true", True), ("TRUE", True), ("1", True),
+        ("false", False), ("no", False), ("yes", False),
+        (1, False),  # only real True or the listed strings count
+    ])
+    def test_force_coercion(self, api_v3_client, wifi_manager, raw, expected):
+        wifi_manager.enable_ap_mode.return_value = (True, "ok")
+        api_v3_client.post(self.ENABLE, json={"force": raw})
+        wifi_manager.enable_ap_mode.assert_called_once_with(force=expected)
+
+    def test_enable_without_body(self, api_v3_client, wifi_manager):
+        wifi_manager.enable_ap_mode.return_value = (True, "ok")
+        assert api_v3_client.post(self.ENABLE).status_code == 200
+
+    def test_enable_failure(self, api_v3_client, wifi_manager):
+        wifi_manager.enable_ap_mode.return_value = (False, "hostapd missing")
+        response = api_v3_client.post(self.ENABLE, json={})
+        assert response.status_code == 400
+        assert response.get_json()["message"] == "hostapd missing"
+
+    def test_disable_success(self, api_v3_client, wifi_manager):
+        wifi_manager.disable_ap_mode.return_value = (True, "AP disabled")
+        assert api_v3_client.post(self.DISABLE).status_code == 200
+
+    def test_disable_failure(self, api_v3_client, wifi_manager):
+        wifi_manager.disable_ap_mode.return_value = (False, "not running")
+        assert api_v3_client.post(self.DISABLE).status_code == 400
+
+    def test_enable_exception_is_a_500(self, api_v3_client, wifi_manager):
+        wifi_manager.enable_ap_mode.side_effect = RuntimeError("boom")
+        assert api_v3_client.post(self.ENABLE, json={}).status_code == 500
+
+
+class TestRadio:
+    URL = "/api/v3/wifi/radio"
+
+    def test_get_state(self, api_v3_client, wifi_manager):
+        wifi_manager.get_wifi_radio_state.return_value = {
+            "enabled": True, "ethernet_connected": False}
+        response = api_v3_client.get(self.URL)
+        assert response.status_code == 200
+        assert response.get_json()["data"]["enabled"] is True
+
+    def test_get_state_exception_is_a_500(self, api_v3_client, wifi_manager):
+        wifi_manager.get_wifi_radio_state.side_effect = OSError("rfkill missing")
+        assert api_v3_client.get(self.URL).status_code == 500
+
+    def test_enabled_is_required(self, api_v3_client, wifi_manager):
+        response = api_v3_client.post(self.URL, json={})
+        assert response.status_code == 400
+        assert "enabled is required" in response.get_json()["message"]
+        wifi_manager.set_wifi_radio.assert_not_called()
+
+    def test_enable_success(self, api_v3_client, wifi_manager):
+        wifi_manager.set_wifi_radio.return_value = (True, "Radio on", None)
+        wifi_manager.get_wifi_radio_state.return_value = {"enabled": True}
+        response = api_v3_client.post(self.URL, json={"enabled": True})
+        assert response.status_code == 200
+        wifi_manager.set_wifi_radio.assert_called_once_with(True, force=False)
+
+    @pytest.mark.parametrize("raw,expected", [
+        (True, True), ("true", True), ("1", True), ("yes", True),
+        (False, False), ("false", False), ("off", False), (0, False),
+    ])
+    def test_enabled_coercion_is_string_aware(
+            self, api_v3_client, wifi_manager, raw, expected):
+        # bool("false") is True, so the endpoint parses strings explicitly
+        # rather than trusting truthiness — it is a public contract, not
+        # only the shipped UI which always sends real JSON booleans.
+        wifi_manager.set_wifi_radio.return_value = (True, "ok", None)
+        wifi_manager.get_wifi_radio_state.return_value = {}
+        api_v3_client.post(self.URL, json={"enabled": raw})
+        wifi_manager.set_wifi_radio.assert_called_once_with(expected, force=False)
+
+    def test_force_passed_through(self, api_v3_client, wifi_manager):
+        wifi_manager.set_wifi_radio.return_value = (True, "ok", None)
+        wifi_manager.get_wifi_radio_state.return_value = {}
+        api_v3_client.post(self.URL, json={"enabled": False, "force": "true"})
+        wifi_manager.set_wifi_radio.assert_called_once_with(False, force=True)
+
+    def test_refusal_reports_reason(self, api_v3_client, wifi_manager):
+        # Disabling the radio without Ethernet would lock the user out of
+        # this very interface, so the manager can refuse with a reason.
+        wifi_manager.set_wifi_radio.return_value = (
+            False, "Refusing: no wired fallback", "no_ethernet")
+        response = api_v3_client.post(self.URL, json={"enabled": False})
+        assert response.status_code == 400
+        body = response.get_json()
+        assert body["reason"] == "no_ethernet"
+        assert "Refusing" in body["message"]
+
+    def test_exception_is_a_500(self, api_v3_client, wifi_manager):
+        wifi_manager.set_wifi_radio.side_effect = RuntimeError("boom")
+        assert api_v3_client.post(self.URL, json={"enabled": True}).status_code == 500
+
+
+class TestNoRealNetworking:
+    def test_wifi_manager_is_never_constructed_for_real(self, api_v3_client):
+        # Guard against a future refactor moving the import to module level,
+        # where the fixture's patch of the definition site would stop
+        # applying and the tests would start driving real networking.
+        with patch("src.wifi_manager.WiFiManager") as cls:
+            cls.return_value.disconnect_from_network.return_value = (True, "ok")
+            api_v3_client.post("/api/v3/wifi/disconnect")
+            assert cls.called

test/test_logo_helper.py

@@ -0,0 +1,315 @@
+"""
+Tests for src/common/logo_helper.py — logo loading, LRU caching, resizing,
+and download-with-fallback. Previously untested: nothing in test/ referenced
+this module at all.
+
+Real PIL images under tmp_path are used rather than mocked ones, since
+load_logo() does real Path.exists() and Image.open() calls; only the HTTP
+session and the permission helpers are patched.
+
+Regression coverage for two fixed bugs:
+- _download_logo wrote response.content to disk with no size cap and no
+  check that the bytes decoded as an image, so a hostile or broken URL
+  could leave arbitrary/oversized content cached in the assets directory.
+- get_cache_stats() divided by self.cache_size unguarded, raising
+  ZeroDivisionError for a helper constructed with cache_size=0.
+"""
+
+import logging
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+import requests
+from PIL import Image
+
+from src.common.logo_helper import MAX_LOGO_BYTES, LogoHelper
+
+
+@pytest.fixture(autouse=True)
+def _no_real_chmod(monkeypatch):
+    # Keep the permission helpers out of the way: their own env detection
+    # is not what these tests are about.
+    monkeypatch.setattr("src.common.logo_helper.ensure_directory_permissions", MagicMock())
+    monkeypatch.setattr("src.common.logo_helper.ensure_file_permissions", MagicMock())
+
+
+@pytest.fixture
+def helper():
+    return LogoHelper(display_width=64, display_height=32,
+                      logger=logging.getLogger("test.logo_helper"))
+
+
+def write_logo(path: Path, size=(20, 20), color=(255, 0, 0), fmt="PNG") -> Path:
+    path.parent.mkdir(parents=True, exist_ok=True)
+    Image.new("RGB", size, color).save(path, format=fmt)
+    return path
+
+
+def fake_response(content: bytes):
+    response = MagicMock()
+    response.content = content
+    response.raise_for_status = MagicMock()
+    return response
+
+
+def png_bytes(size=(20, 20), color=(0, 128, 0)) -> bytes:
+    import io
+    buf = io.BytesIO()
+    Image.new("RGB", size, color).save(buf, format="PNG")
+    return buf.getvalue()
+
+
+class TestLoadLogo:
+    def test_loads_and_converts_to_rgba(self, helper, tmp_path):
+        path = write_logo(tmp_path / "PHI.png")
+        logo = helper.load_logo("PHI", path)
+        assert logo is not None
+        assert logo.mode == "RGBA"
+
+    def test_missing_file_returns_none(self, helper, tmp_path, caplog):
+        with caplog.at_level(logging.WARNING):
+            assert helper.load_logo("NOPE", tmp_path / "missing.png") is None
+        assert "Logo not found" in caplog.text
+
+    def test_second_load_is_served_from_cache(self, helper, tmp_path):
+        path = write_logo(tmp_path / "PHI.png")
+        first = helper.load_logo("PHI", path)
+        path.unlink()  # cache hit must not touch the filesystem
+        assert helper.load_logo("PHI", path) is first
+
+    def test_cache_key_includes_requested_size(self, helper, tmp_path):
+        # A panel-size change must not hand back a logo sized for the old
+        # dimensions, so the two sizes get separate cache entries.
+        path = write_logo(tmp_path / "PHI.png", size=(100, 100))
+        small = helper.load_logo("PHI", path, max_width=10, max_height=10)
+        large = helper.load_logo("PHI", path, max_width=50, max_height=50)
+        assert small is not large
+        assert small.size != large.size
+        assert len(helper._logo_cache) == 2
+
+    def test_default_size_is_one_and_a_half_display(self, helper, tmp_path):
+        path = write_logo(tmp_path / "PHI.png", size=(500, 500))
+        logo = helper.load_logo("PHI", path)
+        assert logo.width <= int(64 * 1.5)
+        assert logo.height <= int(32 * 1.5)
+
+    def test_smaller_image_is_not_upscaled(self, helper, tmp_path):
+        path = write_logo(tmp_path / "PHI.png", size=(8, 8))
+        assert helper.load_logo("PHI", path, max_width=64, max_height=64).size == (8, 8)
+
+    def test_larger_image_is_downscaled_preserving_aspect(self, helper, tmp_path):
+        path = write_logo(tmp_path / "PHI.png", size=(200, 100))
+        logo = helper.load_logo("PHI", path, max_width=50, max_height=50)
+        assert logo.width <= 50 and logo.height <= 50
+        assert logo.width == 50 and logo.height == 25  # 2:1 preserved
+
+    def test_string_path_accepted(self, helper, tmp_path):
+        path = write_logo(tmp_path / "PHI.png")
+        assert helper.load_logo("PHI", str(path)) is not None
+
+    def test_corrupt_file_returns_none(self, helper, tmp_path, caplog):
+        bad = tmp_path / "bad.png"
+        bad.write_bytes(b"not an image")
+        with caplog.at_level(logging.ERROR):
+            assert helper.load_logo("BAD", bad) is None
+        assert "Error loading logo" in caplog.text
+
+
+class TestCacheManagement:
+    def test_lru_evicts_oldest(self, tmp_path):
+        helper = LogoHelper(64, 32, cache_size=2, logger=MagicMock())
+        paths = [write_logo(tmp_path / f"T{i}.png") for i in range(3)]
+        for i, path in enumerate(paths):
+            helper.load_logo(f"T{i}", path)
+        assert len(helper._logo_cache) == 2
+        assert not any(k.startswith("T0_") for k in helper._logo_cache)
+
+    def test_cache_hit_refreshes_lru_position(self, tmp_path):
+        helper = LogoHelper(64, 32, cache_size=2, logger=MagicMock())
+        a, b, c = [write_logo(tmp_path / f"{n}.png") for n in ("A", "B", "C")]
+        helper.load_logo("A", a)
+        helper.load_logo("B", b)
+        helper.load_logo("A", a)   # A is now most-recently used
+        helper.load_logo("C", c)   # evicts B, not A
+        assert any(k.startswith("A_") for k in helper._logo_cache)
+        assert not any(k.startswith("B_") for k in helper._logo_cache)
+
+    def test_clear_cache_empties_both_structures(self, helper, tmp_path):
+        helper.load_logo("PHI", write_logo(tmp_path / "PHI.png"))
+        helper.clear_cache()
+        assert helper._logo_cache == {}
+        assert helper._cache_order == []
+
+    def test_cache_stats(self, tmp_path):
+        helper = LogoHelper(64, 32, cache_size=4, logger=MagicMock())
+        helper.load_logo("PHI", write_logo(tmp_path / "PHI.png"))
+        stats = helper.get_cache_stats()
+        assert stats["cached_logos"] == 1
+        assert stats["cache_size_limit"] == 4
+        assert stats["cache_usage_percent"] == 25
+
+    def test_zero_cache_size_does_not_divide_by_zero(self):
+        # Regression: this raised ZeroDivisionError.
+        stats = LogoHelper(64, 32, cache_size=0, logger=MagicMock()).get_cache_stats()
+        assert stats["cache_usage_percent"] == 0
+        assert stats["cache_size_limit"] == 0
+
+
+class TestLoadLogoWithDownload:
+    def test_existing_file_skips_download(self, helper, tmp_path):
+        path = write_logo(tmp_path / "PHI.png")
+        helper.session.get = MagicMock()
+        assert helper.load_logo_with_download("PHI", path, "http://x/logo.png") is not None
+        helper.session.get.assert_not_called()
+
+    def test_downloads_then_loads(self, helper, tmp_path):
+        path = tmp_path / "PHI.png"
+        helper.session.get = MagicMock(return_value=fake_response(png_bytes()))
+        logo = helper.load_logo_with_download("PHI", path, "http://x/logo.png")
+        assert logo is not None
+        assert path.exists()
+        helper.session.get.assert_called_once_with("http://x/logo.png", timeout=30)
+
+    def test_download_failure_falls_back_to_placeholder(self, helper, tmp_path):
+        helper.session.get = MagicMock(
+            side_effect=requests.RequestException("connection reset"))
+        logo = helper.load_logo_with_download(
+            "PHI", tmp_path / "PHI.png", "http://x/logo.png",
+            max_width=20, max_height=20)
+        assert logo is not None and logo.size == (20, 20)  # placeholder
+
+    def test_http_error_falls_back_to_placeholder(self, helper, tmp_path):
+        response = fake_response(b"")
+        response.raise_for_status.side_effect = requests.HTTPError("404")
+        helper.session.get = MagicMock(return_value=response)
+        logo = helper.load_logo_with_download(
+            "PHI", tmp_path / "PHI.png", "http://x/logo.png",
+            max_width=20, max_height=20)
+        assert logo is not None and logo.size == (20, 20)
+
+    def test_no_url_and_no_file_gives_placeholder(self, helper, tmp_path):
+        logo = helper.load_logo_with_download(
+            "PHI", tmp_path / "missing.png", None, max_width=16, max_height=16)
+        assert logo is not None and logo.size == (16, 16)
+
+
+class TestDownloadLogo:
+    def test_writes_file_and_sets_permissions(self, helper, tmp_path):
+        path = tmp_path / "assets" / "PHI.png"
+        # Directory creation is ensure_directory_permissions' job, and the
+        # autouse fixture stubs it out — so make the directory here.
+        path.parent.mkdir()
+        helper.session.get = MagicMock(return_value=fake_response(png_bytes()))
+        with patch("src.common.logo_helper.ensure_directory_permissions") as dirs, \
+             patch("src.common.logo_helper.ensure_file_permissions") as files:
+            helper._download_logo("http://x/logo.png", path)
+        assert path.exists()
+        dirs.assert_called_once()
+        files.assert_called_once()
+        assert dirs.call_args[0][0] == path.parent
+
+    def test_oversized_response_is_rejected_without_writing(self, helper, tmp_path):
+        # Regression: an unbounded response.content was written straight to
+        # disk, so a hostile URL chose how many bytes landed in assets/.
+        path = tmp_path / "huge.png"
+        helper.session.get = MagicMock(
+            return_value=fake_response(b"\x00" * (MAX_LOGO_BYTES + 1)))
+        with pytest.raises(ValueError, match="over the"):
+            helper._download_logo("http://x/huge.png", path)
+        assert not path.exists()
+
+    def test_non_image_response_is_deleted_and_raises(self, helper, tmp_path):
+        # Regression: undecodable bytes stayed on disk, so every later
+        # load_logo() call hit the corrupt file instead of re-downloading.
+        path = tmp_path / "bad.png"
+        helper.session.get = MagicMock(return_value=fake_response(b"<html>404</html>"))
+        with pytest.raises(Exception):
+            helper._download_logo("http://x/bad.png", path)
+        assert not path.exists()
+
+    def test_decompression_bomb_is_deleted_and_raises(self, helper, tmp_path, monkeypatch):
+        path = tmp_path / "bomb.png"
+        helper.session.get = MagicMock(return_value=fake_response(png_bytes()))
+
+        class Bomb:
+            def __enter__(self):
+                return self
+
+            def __exit__(self, *a):
+                return False
+
+            def load(self):
+                raise Image.DecompressionBombError("too many pixels")
+
+        monkeypatch.setattr("src.common.logo_helper.Image.open", lambda *a, **kw: Bomb())
+        with pytest.raises(Image.DecompressionBombError):
+            helper._download_logo("http://x/bomb.png", path)
+        assert not path.exists()
+
+    def test_bad_download_surfaces_as_placeholder_not_crash(self, helper, tmp_path):
+        # The new guards raise, and load_logo_with_download's existing
+        # broad except turns that into the placeholder path.
+        helper.session.get = MagicMock(return_value=fake_response(b"garbage"))
+        logo = helper.load_logo_with_download(
+            "PHI", tmp_path / "PHI.png", "http://x/bad.png",
+            max_width=12, max_height=12)
+        assert logo is not None and logo.size == (12, 12)
+
+
+class TestLogoVariations:
+    def test_plain_abbreviation_returns_itself(self, helper):
+        assert helper.get_logo_variations("PHI") == ["PHI"]
+
+    def test_ampersand_expanded(self, helper):
+        assert "TAAND M" in helper.get_logo_variations("TA& M")
+
+    def test_and_contracted(self, helper):
+        assert "T&M" in helper.get_logo_variations("TANDM")
+
+    def test_special_case_appends_known_aliases(self, helper):
+        variations = helper.get_logo_variations("TA&M")
+        assert "TAMU" in variations and "TEXASAM" in variations
+        assert "TAANDM" in variations  # the generic & rule still applies
+
+
+class TestNormalizeAbbreviation:
+    def test_uppercases_and_strips(self, helper):
+        assert helper.normalize_abbreviation("  phi  ") == "PHI"
+
+    def test_ampersand_becomes_and(self, helper):
+        assert helper.normalize_abbreviation("TA&M") == "TAANDM"
+
+    def test_internal_spaces_removed(self, helper):
+        assert helper.normalize_abbreviation("New York") == "NEWYORK"
+
+    def test_deliberately_differs_from_logo_downloader(self, helper):
+        # Pinned, not a bug: LogoDownloader.normalize_abbreviation replaces
+        # filesystem-unsafe characters but keeps spaces, and plugins call
+        # that one. Changing either changes which logo filenames resolve on
+        # existing installs. Both docstrings say so explicitly.
+        from src.logo_downloader import LogoDownloader
+        assert helper.normalize_abbreviation("New York") == "NEWYORK"
+        assert LogoDownloader.normalize_abbreviation("New York") == "NEW YORK"
+
+
+class TestPlaceholderLogo:
+    def test_uses_requested_dimensions(self, helper):
+        assert helper._create_placeholder_logo("PHI", 30, 20).size == (30, 20)
+
+    def test_defaults_to_one_and_a_half_display(self, helper):
+        assert helper._create_placeholder_logo("PHI").size == (96, 48)
+
+    def test_is_rgba(self, helper):
+        assert helper._create_placeholder_logo("PHI", 10, 10).mode == "RGBA"
+
+    def test_invalid_dimensions_return_none(self, helper, caplog):
+        with caplog.at_level(logging.ERROR):
+            assert helper._create_placeholder_logo("PHI", -5, -5) is None
+        assert "Error creating placeholder" in caplog.text
+
+
+class TestSessionConfiguration:
+    def test_user_agent_and_accept_headers(self, helper):
+        assert helper.session.headers["User-Agent"] == "LEDMatrix-Common/1.0"
+        assert helper.session.headers["Accept"] == "image/*"

test/test_sync_manager.py

@@ -0,0 +1,829 @@
+"""
+Tests for src/common/sync_manager.py — the UDP leader/follower protocol
+that synchronizes scrolling content across two LED matrix displays.
+
+This module had zero coverage: it only ever appeared in the suite as a
+MagicMock() stand-in (test_vegas_continuous_refresh.py,
+test_display_controller_vegas_tick.py), so none of its real framing,
+handshake, or socket logic was exercised.
+
+Most tests build the manager via object.__new__() + manual attribute
+assignment (the test_display_controller_vegas_tick.py bare-stub pattern)
+so no real sockets open and no background threads start. Receive loops are
+driven synchronously by once_then_stop(): the mocked socket call returns
+one crafted packet, then flips _running False and raises socket.timeout,
+so `while self._running:` exits after exactly one real iteration.
+
+Regression coverage for three fixed bugs:
+- Both recv loops' generic `except Exception` retried with no delay, so a
+  socket stuck raising a non-timeout error spun the thread at 100% CPU.
+- _follower_recv_loop dispatched on `data[:8] == _RAW_MAGIC or
+  len(data) > 512`, which routed any control message over 512 bytes into
+  the image decoder (dropping it) and any raw frame under 512 bytes into
+  the JSON parser.
+- _oversized_frame_warned was read via getattr(self, ..., False) instead of
+  being initialized in __init__.
+"""
+
+import io
+import json
+import socket
+import threading
+import time
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import numpy as np
+import pytest
+from PIL import Image
+
+from src.common import sync_manager
+from src.common.sync_manager import (
+    DisplaySyncManager,
+    FollowerState,
+    LeaderState,
+    SyncRole,
+)
+
+
+@pytest.fixture(autouse=True)
+def _isolated_status_file(tmp_path, monkeypatch):
+    # STATUS_FILE is a module-level fixed path under tempfile.gettempdir() —
+    # genuinely shared state between tests and even between processes.
+    monkeypatch.setattr(
+        sync_manager, "STATUS_FILE", str(tmp_path / "led_matrix_sync_status.json"))
+
+
+def make_manager(role=SyncRole.STANDALONE, hw_config=None):
+    """Bare stub bypassing __init__'s socket/thread setup."""
+    mgr = object.__new__(DisplaySyncManager)
+    mgr.role = role
+    mgr.logger = MagicMock()
+    mgr.port = sync_manager.SYNC_PORT
+    mgr._hw_config = hw_config or {"rows": 32, "cols": 64, "chain_length": 1}
+
+    mgr._leader_state = LeaderState.NO_PEER
+    mgr._peer_ip = None
+    mgr._peer_compatible = False
+    mgr._peer_chain = 0
+    mgr._last_heartbeat_time = 0.0
+    mgr._leader_width = 0
+    mgr._oversized_frame_warned = False
+
+    mgr._follower_state = FollowerState.STANDALONE
+    mgr._latest_frame = None
+    mgr._latest_scroll_x = None
+    mgr._last_leader_frame_time = 0.0
+    mgr._frame_lock = threading.Lock()
+    mgr._leader_ip = None
+    mgr._on_new_cycle = None
+    mgr._on_scroll_image = None
+    mgr._pending_scroll_image = None
+    mgr._scroll_image_lock = threading.Lock()
+    mgr._img_server_sock = None
+
+    mgr._on_follower_connected = None
+    mgr._error_message = None
+    mgr._running = False
+    mgr._recv_sock = None
+    mgr._send_sock = None
+    return mgr
+
+
+def once_then_stop(mgr, value):
+    """side_effect returning `value` once, then stopping the enclosing loop."""
+    state = {"served": False}
+
+    def _side_effect(*args, **kwargs):
+        if not state["served"]:
+            state["served"] = True
+            return value
+        mgr._running = False
+        raise socket.timeout()
+
+    return _side_effect
+
+
+def raise_n_then_stop(mgr, exc, count):
+    """side_effect raising `exc` `count` times, then stopping the loop."""
+    state = {"n": 0}
+
+    def _side_effect(*args, **kwargs):
+        state["n"] += 1
+        if state["n"] <= count:
+            raise exc
+        mgr._running = False
+        raise socket.timeout()
+
+    return _side_effect
+
+
+def run_watchdog_once(monkeypatch, mgr, watchdog, now):
+    """Run exactly one watchdog iteration at a frozen wall-clock time."""
+    monkeypatch.setattr(sync_manager.time, "time", lambda: now)
+    monkeypatch.setattr(
+        sync_manager.time, "sleep", lambda _: setattr(mgr, "_running", False))
+    mgr._running = True
+    watchdog()
+
+
+class FakeConn:
+    """Minimal TCP connection stand-in whose recv() drains a byte buffer."""
+
+    def __init__(self, payload: bytes):
+        self._buf = payload
+        self.closed = False
+
+    def settimeout(self, _):
+        pass
+
+    def recv(self, n):
+        chunk, self._buf = self._buf[:n], self._buf[n:]
+        return chunk
+
+    def close(self):
+        self.closed = True
+
+
+def png_bytes(size=(10, 10), color=(1, 2, 3)) -> bytes:
+    buf = io.BytesIO()
+    Image.new("RGB", size, color).save(buf, format="PNG")
+    return buf.getvalue()
+
+
+def raw_frame_packet(width, height, color=(10, 20, 30)) -> bytes:
+    arr = np.asarray(Image.new("RGB", (width, height), color), dtype=np.uint8)
+    return _magic_header(width, height) + arr.tobytes()
+
+
+def _magic_header(width, height) -> bytes:
+    return sync_manager._RAW_MAGIC + sync_manager._RAW_HEADER.pack(width, height)
+
+
+def length_prefixed(payload: bytes) -> bytes:
+    return len(payload).to_bytes(4, "big") + payload
+
+
+class TestRoleParsing:
+    def test_leader_role(self, monkeypatch):
+        monkeypatch.setattr(DisplaySyncManager, "_start_leader", lambda self: None)
+        assert DisplaySyncManager("leader", {}, {}, MagicMock()).role is SyncRole.LEADER
+
+    def test_follower_role(self, monkeypatch):
+        monkeypatch.setattr(DisplaySyncManager, "_start_follower", lambda self: None)
+        assert DisplaySyncManager("follower", {}, {}, MagicMock()).role is SyncRole.FOLLOWER
+
+    def test_standalone_starts_nothing(self):
+        mgr = DisplaySyncManager("standalone", {}, {}, MagicMock())
+        assert mgr.role is SyncRole.STANDALONE
+        assert mgr._running is False
+        assert mgr._recv_sock is None
+
+    def test_invalid_role_warns_and_falls_back(self):
+        logger = MagicMock()
+        assert DisplaySyncManager("bogus", {}, {}, logger).role is SyncRole.STANDALONE
+        assert logger.warning.called
+
+    def test_role_matching_is_case_sensitive(self):
+        # Pinned: SyncRole's values are lowercase, so "LEADER" is not
+        # normalized — it is simply invalid and falls back to standalone.
+        logger = MagicMock()
+        assert DisplaySyncManager("LEADER", {}, {}, logger).role is SyncRole.STANDALONE
+        assert logger.warning.called
+
+    def test_port_defaults_to_module_constant(self):
+        assert DisplaySyncManager("standalone", {}, {}, MagicMock()).port == sync_manager.SYNC_PORT
+
+    def test_port_read_from_config(self):
+        assert DisplaySyncManager("standalone", {"port": 9999}, {}, MagicMock()).port == 9999
+
+    def test_oversized_frame_warned_initialized_in_init(self, monkeypatch):
+        # Regression: this attribute was only ever created on first use via
+        # getattr(self, '_oversized_frame_warned', False).
+        monkeypatch.setattr(DisplaySyncManager, "_start_leader", lambda self: None)
+        mgr = DisplaySyncManager("leader", {}, {}, MagicMock())
+        assert mgr._oversized_frame_warned is False
+
+
+class TestHandleHello:
+    def test_matching_panels_connect(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._send_sock = MagicMock()
+        mgr._handle_hello({"t": "hello", "rows": 32, "cols": 64, "chain": 3}, "10.0.0.5")
+        assert mgr._leader_state is LeaderState.CONNECTED
+        assert mgr._peer_ip == "10.0.0.5"
+        assert mgr._peer_compatible is True
+        assert mgr._peer_chain == 3
+        assert mgr._error_message is None
+
+    def test_ack_reports_compatibility(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._send_sock = MagicMock()
+        mgr._leader_width = 128
+        mgr._handle_hello({"t": "hello", "rows": 32, "cols": 64, "chain": 1}, "10.0.0.5")
+        payload, dest = mgr._send_sock.sendto.call_args[0]
+        ack = json.loads(payload.decode("utf-8"))
+        assert ack["compatible"] is True
+        assert ack["leader_width"] == 128
+        assert dest == ("10.0.0.5", mgr.port)
+
+    def test_mismatched_panels_are_incompatible(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._send_sock = MagicMock()
+        mgr._handle_hello({"t": "hello", "rows": 16, "cols": 32, "chain": 1}, "10.0.0.5")
+        assert mgr._leader_state is LeaderState.INCOMPATIBLE
+        assert "Incompatible panels" in mgr._error_message
+        ack = json.loads(mgr._send_sock.sendto.call_args[0][0].decode("utf-8"))
+        assert ack["compatible"] is False
+        assert ack["error"] == mgr._error_message
+
+    def test_chain_length_may_differ(self):
+        # Documented rule: rows/cols must match, chain_length need not.
+        mgr = make_manager(role=SyncRole.LEADER, hw_config={"rows": 32, "cols": 64, "chain_length": 1})
+        mgr._send_sock = MagicMock()
+        mgr._handle_hello({"t": "hello", "rows": 32, "cols": 64, "chain": 4}, "10.0.0.5")
+        assert mgr._leader_state is LeaderState.CONNECTED
+
+    def test_connect_callback_fires_only_on_first_transition(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._send_sock = MagicMock()
+        fired = threading.Event()
+        calls = []
+        mgr._on_follower_connected = lambda: (calls.append(1), fired.set())
+
+        hello = {"t": "hello", "rows": 32, "cols": 64, "chain": 1}
+        mgr._handle_hello(hello, "10.0.0.5")
+        assert fired.wait(timeout=1)
+        assert len(calls) == 1
+
+        fired.clear()
+        mgr._handle_hello(hello, "10.0.0.5")  # already CONNECTED
+        assert not fired.wait(timeout=0.2)
+        assert len(calls) == 1
+
+    def test_ack_send_failure_is_swallowed(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._send_sock = MagicMock()
+        mgr._send_sock.sendto.side_effect = OSError("network unreachable")
+        mgr._handle_hello({"t": "hello", "rows": 32, "cols": 64, "chain": 1}, "10.0.0.5")
+        assert mgr._leader_state is LeaderState.CONNECTED  # state still updated
+        assert mgr.logger.debug.called
+
+
+class TestWatchdogs:
+    def test_leader_drops_peer_after_heartbeat_timeout(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        mgr._peer_ip = "10.0.0.1"
+        mgr._peer_compatible = True
+        mgr._last_heartbeat_time = 0.0
+        run_watchdog_once(monkeypatch, mgr, mgr._leader_watchdog,
+                          now=sync_manager.PEER_TIMEOUT + 1)
+        assert mgr._leader_state is LeaderState.NO_PEER
+        assert mgr._peer_ip is None
+        assert mgr._peer_compatible is False
+
+    def test_leader_keeps_peer_within_timeout(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        mgr._peer_ip = "10.0.0.1"
+        mgr._last_heartbeat_time = 100.0
+        run_watchdog_once(monkeypatch, mgr, mgr._leader_watchdog, now=101.0)
+        assert mgr._leader_state is LeaderState.CONNECTED
+        assert mgr._peer_ip == "10.0.0.1"
+
+    def test_leader_watchdog_ignores_disconnected_state(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.INCOMPATIBLE
+        mgr._last_heartbeat_time = 0.0
+        run_watchdog_once(monkeypatch, mgr, mgr._leader_watchdog, now=10_000)
+        assert mgr._leader_state is LeaderState.INCOMPATIBLE
+
+    def test_follower_returns_to_standalone_after_frame_timeout(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._follower_state = FollowerState.FOLLOWER
+        mgr._last_leader_frame_time = 0.0
+        mgr._latest_frame = Image.new("RGB", (2, 2))
+        run_watchdog_once(monkeypatch, mgr, mgr._follower_watchdog,
+                          now=sync_manager.LEADER_TIMEOUT + 1)
+        assert mgr._follower_state is FollowerState.STANDALONE
+        assert mgr.get_latest_frame() is None
+
+    def test_follower_keeps_frames_within_timeout(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._follower_state = FollowerState.FOLLOWER
+        mgr._last_leader_frame_time = 100.0
+        mgr._latest_frame = Image.new("RGB", (2, 2))
+        run_watchdog_once(monkeypatch, mgr, mgr._follower_watchdog, now=101.0)
+        assert mgr._follower_state is FollowerState.FOLLOWER
+        assert mgr.get_latest_frame() is not None
+
+
+class TestLeaderRecvLoop:
+    def _drive(self, mgr, payload, sender="10.0.0.8"):
+        mgr._recv_sock = MagicMock()
+        mgr._recv_sock.recvfrom.side_effect = once_then_stop(mgr, (payload, (sender, 1)))
+        mgr._running = True
+        mgr._leader_recv_loop()
+
+    def test_hello_is_dispatched(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._send_sock = MagicMock()
+        self._drive(mgr, json.dumps(
+            {"t": "hello", "rows": 32, "cols": 64, "chain": 1}).encode())
+        assert mgr._leader_state is LeaderState.CONNECTED
+        assert mgr._peer_ip == "10.0.0.8"
+
+    def test_heartbeat_from_known_peer_refreshes_timer(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._peer_ip = "10.0.0.8"
+        with patch.object(sync_manager.time, "time", return_value=12345.0):
+            self._drive(mgr, json.dumps({"t": "hb"}).encode())
+        assert mgr._last_heartbeat_time == 12345.0
+
+    def test_heartbeat_from_stranger_is_ignored(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._peer_ip = "10.0.0.8"
+        mgr._last_heartbeat_time = 5.0
+        self._drive(mgr, json.dumps({"t": "hb"}).encode(), sender="10.0.0.99")
+        assert mgr._last_heartbeat_time == 5.0
+
+    def test_unknown_message_type_ignored(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        self._drive(mgr, json.dumps({"t": "who-knows"}).encode())
+        assert mgr._leader_state is LeaderState.NO_PEER
+
+    def test_malformed_json_is_swallowed(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        self._drive(mgr, b"{not json")
+        assert mgr._leader_state is LeaderState.NO_PEER
+
+    def test_undecodable_bytes_are_swallowed(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        self._drive(mgr, b"\xff\xfe\x00bad")
+        assert mgr._leader_state is LeaderState.NO_PEER
+
+    def test_backs_off_between_repeated_errors(self, monkeypatch):
+        # Regression: without a sleep this loop spun at 100% CPU whenever
+        # the socket raised a non-timeout error on every call.
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._recv_sock = MagicMock()
+        mgr._recv_sock.recvfrom.side_effect = raise_n_then_stop(mgr, OSError("boom"), 3)
+        sleeps = MagicMock()
+        monkeypatch.setattr(sync_manager.time, "sleep", sleeps)
+        mgr._running = True
+        mgr._leader_recv_loop()
+        assert sleeps.call_count == 3
+        sleeps.assert_called_with(0.1)
+
+
+class TestFollowerRecvLoop:
+    def _drive(self, mgr, payload, sender="10.0.0.2"):
+        mgr._recv_sock = MagicMock()
+        mgr._recv_sock.recvfrom.side_effect = once_then_stop(mgr, (payload, (sender, 1)))
+        mgr._running = True
+        mgr._follower_recv_loop()
+
+    def test_small_raw_frame_is_decoded(self):
+        # Regression: a raw frame under the old 512-byte threshold was sent
+        # to the JSON parser and dropped.
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        packet = raw_frame_packet(4, 3)
+        assert len(packet) <= 512
+        self._drive(mgr, packet)
+        frame = mgr.get_latest_frame()
+        assert frame is not None and frame.size == (4, 3)
+        assert mgr._follower_state is FollowerState.FOLLOWER
+
+    def test_large_raw_frame_is_decoded(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        packet = raw_frame_packet(64, 32)
+        assert len(packet) > 512
+        self._drive(mgr, packet)
+        assert mgr.get_latest_frame().size == (64, 32)
+
+    def test_large_control_message_is_not_routed_to_image_decode(self):
+        # Regression: the old `len(data) > 512` branch treated any large
+        # control message as frame data and silently discarded it.
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        long_error = "x" * 600
+        payload = json.dumps(
+            {"t": "hello_ack", "compatible": False, "error": long_error}).encode()
+        assert len(payload) > 512
+        self._drive(mgr, payload, sender="10.0.0.9")
+        assert mgr._leader_ip == "10.0.0.9"
+        assert mgr._peer_compatible is False
+        assert mgr._error_message == long_error
+        assert mgr.get_latest_frame() is None
+        assert mgr.logger.error.called
+
+    def test_legacy_png_frame_without_magic_is_decoded(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        self._drive(mgr, png_bytes(size=(5, 5)))
+        frame = mgr.get_latest_frame()
+        assert frame is not None and frame.size == (5, 5)
+        assert mgr._follower_state is FollowerState.FOLLOWER
+
+    def test_truncated_raw_frame_is_swallowed(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        self._drive(mgr, _magic_header(64, 32) + b"\x00" * 10)  # far too short
+        assert mgr.get_latest_frame() is None
+        assert mgr.logger.debug.called
+
+    def test_garbage_payload_is_swallowed(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        self._drive(mgr, b"neither json nor a png, just bytes 1234567890")
+        assert mgr.get_latest_frame() is None
+
+    def test_hello_ack_updates_peer_state(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        self._drive(mgr, json.dumps(
+            {"t": "hello_ack", "compatible": True, "error": None}).encode(),
+            sender="10.0.0.6")
+        assert mgr._leader_ip == "10.0.0.6"
+        assert mgr._peer_compatible is True
+        assert mgr.logger.error.called is False
+
+    def test_scroll_x_switches_to_follower_and_builds_cycle(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        calls = []
+        mgr._on_new_cycle = lambda: calls.append(1)
+        self._drive(mgr, json.dumps({"t": "sx", "x": 12.34}).encode())
+        assert mgr._follower_state is FollowerState.FOLLOWER
+        assert mgr.get_latest_scroll_x() == 12.34
+        assert calls == [1]
+
+    def test_scroll_x_while_already_following_does_not_rebuild(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._follower_state = FollowerState.FOLLOWER
+        calls = []
+        mgr._on_new_cycle = lambda: calls.append(1)
+        self._drive(mgr, json.dumps({"t": "sx", "x": 5.0}).encode())
+        assert mgr.get_latest_scroll_x() == 5.0
+        assert calls == []
+
+    def test_new_cycle_message_triggers_callback(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._follower_state = FollowerState.FOLLOWER
+        calls = []
+        mgr._on_new_cycle = lambda: calls.append(1)
+        self._drive(mgr, json.dumps({"t": "nc"}).encode())
+        assert calls == [1]
+
+    def test_scroll_x_missing_key_is_swallowed(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        self._drive(mgr, json.dumps({"t": "sx"}).encode())  # no "x"
+        assert mgr.get_latest_scroll_x() is None
+
+    def test_backs_off_between_repeated_errors(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._recv_sock = MagicMock()
+        mgr._recv_sock.recvfrom.side_effect = raise_n_then_stop(mgr, OSError("boom"), 3)
+        sleeps = MagicMock()
+        monkeypatch.setattr(sync_manager.time, "sleep", sleeps)
+        mgr._running = True
+        mgr._follower_recv_loop()
+        assert sleeps.call_count == 3
+        sleeps.assert_called_with(0.1)
+
+
+class TestSendFrame:
+    def _connected_leader(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        mgr._peer_ip = "10.0.0.1"
+        mgr._send_sock = MagicMock()
+        return mgr
+
+    def test_frame_sent_with_magic_header(self):
+        mgr = self._connected_leader()
+        mgr.send_frame(Image.new("RGB", (8, 8)))
+        packet = mgr._send_sock.sendto.call_args[0][0]
+        assert packet[:8] == sync_manager._RAW_MAGIC
+        assert sync_manager._RAW_HEADER.unpack(packet[8:12]) == (8, 8)
+
+    def test_oversized_frame_warns_once_and_is_dropped(self):
+        mgr = self._connected_leader()
+        big = Image.new("RGB", (300, 300))  # 270000 bytes > 65000 UDP cap
+
+        mgr.send_frame(big)
+        assert mgr._oversized_frame_warned is True
+        assert mgr.logger.warning.call_count == 1
+        assert not mgr._send_sock.sendto.called
+
+        mgr.send_frame(big)
+        assert mgr.logger.warning.call_count == 1  # still warned only once
+
+    def test_not_sent_when_no_peer(self):
+        mgr = self._connected_leader()
+        mgr._leader_state = LeaderState.NO_PEER
+        mgr.send_frame(Image.new("RGB", (8, 8)))
+        assert not mgr._send_sock.sendto.called
+
+    def test_follower_never_sends(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._send_sock = MagicMock()
+        mgr.send_frame(Image.new("RGB", (8, 8)))
+        assert not mgr._send_sock.sendto.called
+
+    def test_send_error_is_swallowed(self):
+        mgr = self._connected_leader()
+        mgr._send_sock.sendto.side_effect = OSError("no route")
+        mgr.send_frame(Image.new("RGB", (8, 8)))  # must not raise
+        assert mgr.logger.debug.called
+
+
+class TestSendControlMessages:
+    def _connected_leader(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        mgr._peer_ip = "10.0.0.1"
+        mgr._send_sock = MagicMock()
+        return mgr
+
+    def test_send_scroll_x_rounds_to_two_places(self):
+        mgr = self._connected_leader()
+        mgr.send_scroll_x(3.14159)
+        msg = json.loads(mgr._send_sock.sendto.call_args[0][0].decode())
+        assert msg == {"t": "sx", "x": 3.14}
+
+    def test_send_new_cycle(self):
+        mgr = self._connected_leader()
+        mgr.send_new_cycle()
+        msg = json.loads(mgr._send_sock.sendto.call_args[0][0].decode())
+        assert msg == {"t": "nc"}
+
+    def test_control_messages_noop_when_disconnected(self):
+        mgr = self._connected_leader()
+        mgr._leader_state = LeaderState.NO_PEER
+        mgr.send_scroll_x(1.0)
+        mgr.send_new_cycle()
+        assert not mgr._send_sock.sendto.called
+
+    def test_set_leader_width(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr.set_leader_width(256)
+        assert mgr._leader_width == 256
+
+
+class TestImageServerLoop:
+    def _drive(self, mgr, conn):
+        mgr._img_server_sock = MagicMock()
+        mgr._img_server_sock.accept.side_effect = once_then_stop(
+            mgr, (conn, ("10.0.0.1", 1)))
+        mgr._running = True
+        mgr._image_server_loop()
+
+    def test_rejects_non_positive_length(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._on_scroll_image = MagicMock()
+        self._drive(mgr, FakeConn((0).to_bytes(4, "big")))
+        assert mgr.logger.warning.called
+        mgr._on_scroll_image.assert_not_called()
+
+    def test_rejects_oversized_length(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._on_scroll_image = MagicMock()
+        self._drive(mgr, FakeConn((11 * 1024 * 1024).to_bytes(4, "big")))
+        assert mgr.logger.warning.called
+        mgr._on_scroll_image.assert_not_called()
+
+    def test_rejects_oversized_dimensions(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._on_scroll_image = MagicMock()
+        self._drive(mgr, FakeConn(length_prefixed(png_bytes(size=(300, 300)))))
+        assert mgr.logger.warning.called
+        mgr._on_scroll_image.assert_not_called()
+
+    def test_rejects_decompression_bomb(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._on_scroll_image = MagicMock()
+
+        class BombImage:
+            width = height = 10
+
+            def load(self):
+                raise Image.DecompressionBombError("too many pixels")
+
+        monkeypatch.setattr(sync_manager.Image, "open", lambda *a, **kw: BombImage())
+        self._drive(mgr, FakeConn(length_prefixed(png_bytes())))
+        assert mgr.logger.warning.called
+        mgr._on_scroll_image.assert_not_called()
+
+    def test_valid_image_invokes_callback(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        received = []
+        mgr._on_scroll_image = received.append
+        self._drive(mgr, FakeConn(length_prefixed(png_bytes(size=(10, 10)))))
+        assert len(received) == 1
+        assert received[0].size == (10, 10)
+
+    def test_image_cached_when_callback_not_yet_registered(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._on_scroll_image = None
+        self._drive(mgr, FakeConn(length_prefixed(png_bytes(size=(6, 6)))))
+        assert mgr._pending_scroll_image is not None
+        assert mgr._pending_scroll_image.size == (6, 6)
+
+    def test_short_header_is_skipped(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._on_scroll_image = MagicMock()
+        self._drive(mgr, FakeConn(b"\x00\x01"))  # under the 4-byte prefix
+        mgr._on_scroll_image.assert_not_called()
+
+    def test_connection_always_closed(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        conn = FakeConn(length_prefixed(png_bytes()))
+        self._drive(mgr, conn)
+        assert conn.closed is True
+
+
+class TestScrollImageCallback:
+    def test_pending_image_delivered_on_late_registration(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        img = Image.new("RGB", (3, 3))
+        mgr._pending_scroll_image = img
+        received = []
+        mgr.set_on_scroll_image(received.append)
+        assert received == [img]
+        assert mgr._pending_scroll_image is None
+
+    def test_no_pending_image_means_no_immediate_call(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        received = []
+        mgr.set_on_scroll_image(received.append)
+        assert received == []
+
+
+class TestFollowerConnectedCallback:
+    def test_fires_immediately_when_already_connected(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        fired = threading.Event()
+        mgr.set_on_follower_connected(fired.set)
+        assert fired.wait(timeout=1)
+
+    def test_does_not_fire_when_no_peer(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        fired = threading.Event()
+        mgr.set_on_follower_connected(fired.set)
+        assert not fired.wait(timeout=0.2)
+
+
+class TestSendScrollImage:
+    def test_noop_when_not_connected(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.NO_PEER
+        with patch.object(sync_manager.socket, "socket") as sock:
+            mgr.send_scroll_image(Image.new("RGB", (4, 4)))
+            sock.assert_not_called()
+
+    def test_noop_for_follower_role(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        with patch.object(sync_manager.socket, "socket") as sock:
+            mgr.send_scroll_image(Image.new("RGB", (4, 4)))
+            sock.assert_not_called()
+
+    def test_sends_length_prefixed_png(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        mgr._peer_ip = "10.0.0.1"
+        fake_sock = MagicMock()
+        fake_sock.__enter__ = lambda s: s
+        fake_sock.__exit__ = lambda s, *a: False
+        with patch.object(sync_manager.socket, "socket", return_value=fake_sock):
+            mgr.send_scroll_image(Image.new("RGB", (4, 4)))
+        payload = fake_sock.sendall.call_args[0][0]
+        assert int.from_bytes(payload[:4], "big") == len(payload) - 4
+        assert payload[4:8] == b"\x89PNG"
+
+    def test_connection_error_is_swallowed(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        mgr._peer_ip = "10.0.0.1"
+        with patch.object(sync_manager.socket, "socket", side_effect=OSError("refused")):
+            mgr.send_scroll_image(Image.new("RGB", (4, 4)))  # must not raise
+        assert mgr.logger.debug.called
+
+
+class TestGetStatus:
+    def test_standalone_shape(self):
+        status = make_manager(role=SyncRole.STANDALONE).get_status()
+        assert status["role"] == "standalone"
+        assert status["state"] == "standalone"
+        assert status["local_rows"] == 32 and status["local_cols"] == 64
+
+    def test_leader_shape(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._leader_state = LeaderState.CONNECTED
+        mgr._peer_ip = "10.0.0.1"
+        mgr._peer_compatible = True
+        mgr._peer_chain = 2
+        mgr._leader_width = 128
+        status = mgr.get_status()
+        assert status["role"] == "leader"
+        assert status["state"] == "connected"
+        assert status["peer_ip"] == "10.0.0.1"
+        assert status["peer_chain"] == 2
+        assert status["leader_width"] == 128
+
+    def test_follower_shape(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        mgr._follower_state = FollowerState.FOLLOWER
+        mgr._leader_ip = "10.0.0.2"
+        status = mgr.get_status()
+        assert status["role"] == "follower"
+        assert status["state"] == "follower"
+        assert status["leader_ip"] == "10.0.0.2"
+        assert "peer_chain" not in status
+
+    def test_is_follower_active(self):
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        assert mgr.is_follower_active() is False
+        mgr._follower_state = FollowerState.FOLLOWER
+        assert mgr.is_follower_active() is True
+
+    def test_leader_is_never_follower_active(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._follower_state = FollowerState.FOLLOWER
+        assert mgr.is_follower_active() is False
+
+
+class TestWriteStatusFile:
+    def test_writes_status_and_cleans_up_temp(self):
+        mgr = make_manager(role=SyncRole.STANDALONE)
+        mgr.write_status_file()
+        data = json.loads(Path(sync_manager.STATUS_FILE).read_text())
+        assert data["role"] == "standalone"
+        assert "ts" in data
+        assert not Path(sync_manager.STATUS_FILE + ".tmp").exists()
+
+    def test_write_failure_is_swallowed(self, monkeypatch):
+        mgr = make_manager(role=SyncRole.STANDALONE)
+        monkeypatch.setattr("builtins.open", MagicMock(side_effect=OSError("disk full")))
+        mgr.write_status_file()  # must not raise
+        assert mgr.logger.debug.called
+
+
+class TestStop:
+    def _stub_with_sockets(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._recv_sock = MagicMock()
+        mgr._send_sock = MagicMock()
+        mgr._img_server_sock = MagicMock()
+        return mgr
+
+    def test_closes_every_socket(self):
+        mgr = self._stub_with_sockets()
+        mgr.stop()
+        assert mgr._running is False
+        mgr._recv_sock.close.assert_called_once()
+        mgr._send_sock.close.assert_called_once()
+        mgr._img_server_sock.close.assert_called_once()
+
+    def test_is_idempotent(self):
+        mgr = self._stub_with_sockets()
+        mgr.stop()
+        mgr.stop()  # must not raise
+
+    def test_close_failure_is_swallowed(self):
+        mgr = make_manager(role=SyncRole.LEADER)
+        mgr._recv_sock = MagicMock()
+        mgr._recv_sock.close.side_effect = OSError("already closed")
+        mgr.stop()  # must not raise
+        assert mgr.logger.debug.called
+
+    def test_handles_unset_sockets(self):
+        make_manager(role=SyncRole.STANDALONE).stop()  # all sockets None
+
+
+class TestLoopbackHandshake:
+    def test_leader_and_follower_negotiate_over_real_sockets(self, monkeypatch):
+        # One end-to-end check that the wire format actually round-trips:
+        # every other test drives the loops with mocked sockets.
+        monkeypatch.setattr(sync_manager, "HELLO_INTERVAL", 0.02)
+        monkeypatch.setattr(sync_manager, "HEARTBEAT_INTERVAL", 0.02)
+
+        # Pick a free port by binding one on loopback and releasing it.
+        # Loopback, not "", so this test never opens a port to the network.
+        probe = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+        probe.bind(("127.0.0.1", 0))
+        port = probe.getsockname()[1]
+        probe.close()
+
+        hw = {"rows": 32, "cols": 64, "chain_length": 1}
+        leader = DisplaySyncManager("leader", {"port": port}, hw, MagicMock())
+        follower = DisplaySyncManager("follower", {"port": port}, hw, MagicMock())
+        try:
+            deadline = time.time() + 5.0
+            while time.time() < deadline:
+                if (leader._leader_state is LeaderState.CONNECTED
+                        and follower._peer_compatible):
+                    break
+                time.sleep(0.02)
+            assert leader._leader_state is LeaderState.CONNECTED
+            assert follower._peer_compatible is True
+            assert follower._leader_ip is not None
+        finally:
+            leader.stop()
+            follower.stop()

test/web_interface/test_api_v3_backup_paths.py

@@ -0,0 +1,220 @@
+"""
+Path-containment tests for the backup file routes:
+GET /backup/download/<filename>, DELETE /backup/<filename>, and the
+listing/validation routes alongside them.
+
+Both filename routes take user input straight from the URL and turn it
+into a filesystem path, one to read and one to unlink. `_safe_backup_path`
+is what stops that from reaching outside the export directory, and it had
+no tests.
+
+This is verification of existing containment, not a fix: no bypass was
+found. The tests exist so that a later "just let dots through" change has
+to argue with something.
+"""
+
+import io
+import json
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+from flask import Flask
+
+project_root = Path(__file__).parent.parent.parent
+sys.path.insert(0, str(project_root))
+
+from web_interface.blueprints import api_v3 as api_v3_module  # noqa: E402
+from web_interface.blueprints.api_v3 import api_v3  # noqa: E402
+
+_MANAGER_ATTRS = (
+    'config_manager', 'plugin_manager', 'plugin_store_manager',
+    'plugin_state_manager', 'saved_repositories_manager', 'schema_manager',
+    'operation_queue', 'operation_history', 'cache_manager',
+)
+_SENTINEL = object()
+
+# Anything that tries to name a file outside the export directory, or that
+# is not a plain <name>.zip.
+TRAVERSAL_ATTEMPTS = [
+    "../../etc/passwd",
+    "../config.json",
+    "..%2f..%2fetc%2fpasswd",
+    "....//....//etc/passwd",
+    "/etc/passwd",
+    "..\\..\\config.json",
+    "backup.zip/../../../etc/passwd",
+    ".hidden.zip",
+    "backup.txt",
+    "backup.zip.exe",
+    "",
+    ".",
+    "..",
+]
+
+
+@pytest.fixture
+def env(tmp_path, monkeypatch):
+    export_dir = tmp_path / "backups"
+    export_dir.mkdir()
+    monkeypatch.setattr(api_v3_module, "_BACKUP_EXPORT_DIR", export_dir)
+
+    # A file outside the export dir that a traversal would be reaching for.
+    secret = tmp_path / "config.json"
+    secret.write_text(json.dumps({"secret": "do not touch"}))
+
+    originals = {name: getattr(api_v3, name, _SENTINEL) for name in _MANAGER_ATTRS}
+    for name in _MANAGER_ATTRS:
+        setattr(api_v3, name, MagicMock())
+
+    app = Flask(__name__)
+    app.config["TESTING"] = True
+    app.register_blueprint(api_v3, url_prefix="/api/v3")
+
+    class Env:
+        pass
+
+    e = Env()
+    e.client = app.test_client()
+    e.export_dir = export_dir
+    e.secret = secret
+    yield e
+
+    for name, original in originals.items():
+        if original is _SENTINEL:
+            if hasattr(api_v3, name):
+                delattr(api_v3, name)
+        else:
+            setattr(api_v3, name, original)
+
+
+def make_backup(export_dir, name="backup-2026-01-01.zip"):
+    path = export_dir / name
+    path.write_bytes(b"PK\x03\x04fake zip")
+    return path
+
+
+class TestSafeBackupPath:
+    """The containment helper itself."""
+
+    @pytest.mark.parametrize("filename", TRAVERSAL_ATTEMPTS)
+    def test_rejects_unsafe_names(self, env, filename):
+        assert api_v3_module._safe_backup_path(filename) is None
+
+    def test_rejects_none(self, env):
+        assert api_v3_module._safe_backup_path(None) is None
+
+    @pytest.mark.parametrize("filename", [
+        "backup.zip",
+        "backup-2026-01-01.zip",
+        "backup_2026.01.01-v2.zip",
+        "a.zip",
+    ])
+    def test_accepts_plain_zip_names(self, env, filename):
+        resolved = api_v3_module._safe_backup_path(filename)
+        assert resolved is not None
+        assert resolved.parent == env.export_dir.resolve()
+
+    def test_result_is_always_inside_the_export_dir(self, env):
+        resolved = api_v3_module._safe_backup_path("backup.zip")
+        resolved.relative_to(env.export_dir.resolve())  # raises if outside
+
+    def test_overlong_name_rejected(self, env):
+        assert api_v3_module._safe_backup_path("a" * 250 + ".zip") is None
+
+
+class TestDownload:
+    def test_downloads_an_existing_backup(self, env):
+        make_backup(env.export_dir)
+        response = env.client.get("/api/v3/backup/download/backup-2026-01-01.zip")
+        assert response.status_code == 200
+        assert response.data == b"PK\x03\x04fake zip"
+
+    def test_missing_file_is_a_404(self, env):
+        response = env.client.get("/api/v3/backup/download/never-made.zip")
+        assert response.status_code == 404
+
+    @pytest.mark.parametrize("filename", TRAVERSAL_ATTEMPTS)
+    def test_traversal_attempts_are_refused(self, env, filename):
+        response = env.client.get(f"/api/v3/backup/download/{filename}")
+        # However the request is turned away — 404 from the containment
+        # check, or 308/405 from routing never matching at all — what
+        # matters is that no file outside the export directory is served.
+        assert response.status_code != 200
+        assert b"do not touch" not in response.data
+
+
+class TestDelete:
+    def test_deletes_an_existing_backup(self, env):
+        path = make_backup(env.export_dir)
+        response = env.client.delete("/api/v3/backup/backup-2026-01-01.zip")
+        assert response.status_code == 200
+        assert not path.exists()
+
+    def test_missing_file_is_a_404(self, env):
+        response = env.client.delete("/api/v3/backup/never-made.zip")
+        assert response.status_code == 404
+
+    @pytest.mark.parametrize("filename", TRAVERSAL_ATTEMPTS)
+    def test_traversal_attempts_delete_nothing(self, env, filename):
+        response = env.client.delete(f"/api/v3/backup/{filename}")
+        assert response.status_code != 200
+        assert env.secret.exists()  # the file a traversal was aiming at
+
+    def test_only_the_named_backup_is_removed(self, env):
+        keep = make_backup(env.export_dir, "keep.zip")
+        drop = make_backup(env.export_dir, "drop.zip")
+        env.client.delete("/api/v3/backup/drop.zip")
+        assert keep.exists()
+        assert not drop.exists()
+
+    def test_directory_with_a_matching_name_is_not_removed(self, env):
+        # The delete loop matches by name but requires a regular file.
+        (env.export_dir / "sneaky.zip").mkdir()
+        response = env.client.delete("/api/v3/backup/sneaky.zip")
+        assert response.status_code == 404
+        assert (env.export_dir / "sneaky.zip").is_dir()
+
+
+class TestList:
+    def test_lists_only_zip_files(self, env):
+        make_backup(env.export_dir, "one.zip")
+        (env.export_dir / "notes.txt").write_text("ignore me")
+        response = env.client.get("/api/v3/backup/list")
+        assert response.status_code == 200
+        names = [entry["filename"] for entry in response.get_json()["data"]]
+        assert names == ["one.zip"]
+
+    def test_empty_directory_lists_nothing(self, env):
+        response = env.client.get("/api/v3/backup/list")
+        assert response.get_json()["data"] == []
+
+    def test_entries_carry_size_and_timestamp(self, env):
+        make_backup(env.export_dir, "one.zip")
+        entry = env.client.get("/api/v3/backup/list").get_json()["data"][0]
+        assert entry["size"] == len(b"PK\x03\x04fake zip")
+        assert entry["created_at"]
+
+
+class TestValidate:
+    def test_missing_file_is_a_400(self, env):
+        response = env.client.post("/api/v3/backup/validate", data={},
+                                   content_type="multipart/form-data")
+        assert response.status_code == 400
+        assert "No backup_file" in response.get_json()["message"]
+
+    def test_invalid_archive_is_a_400(self, env):
+        response = env.client.post(
+            "/api/v3/backup/validate",
+            data={"backup_file": (io.BytesIO(b"not a zip"), "bad.zip")},
+            content_type="multipart/form-data")
+        assert response.status_code == 400
+        assert "Invalid or corrupted" in response.get_json()["message"]
+
+    def test_validation_does_not_leave_temp_files_in_the_export_dir(self, env):
+        env.client.post(
+            "/api/v3/backup/validate",
+            data={"backup_file": (io.BytesIO(b"not a zip"), "bad.zip")},
+            content_type="multipart/form-data")
+        assert list(env.export_dir.iterdir()) == []

test/web_interface/test_api_v3_backup_restore.py

@@ -0,0 +1,262 @@
+"""
+Endpoint tests for POST /backup/restore.
+
+Restore is the most destructive operation the web interface exposes: it
+overwrites config, secrets, WiFi settings and fonts, and reinstalls
+plugins. It had no tests.
+
+restore_backup itself is mocked — this file is about what the route does
+with the request and with the result, not about ZIP handling, which
+belongs to backup_manager's own tests.
+
+Regression coverage for one fixed bug: a malformed `options` field fell
+back to {}, and since every RestoreOptions flag defaults to True, that
+turned a mis-serialized narrow restore into a full one — secrets
+included — with no indication anything had been ignored.
+"""
+
+import io
+import json
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+from flask import Flask
+
+project_root = Path(__file__).parent.parent.parent
+sys.path.insert(0, str(project_root))
+
+from web_interface.blueprints.api_v3 import api_v3  # noqa: E402
+
+URL = "/api/v3/backup/restore"
+
+_MANAGER_ATTRS = (
+    'config_manager', 'plugin_manager', 'plugin_store_manager',
+    'plugin_state_manager', 'saved_repositories_manager', 'schema_manager',
+    'operation_queue', 'operation_history', 'cache_manager',
+)
+_SENTINEL = object()
+
+
+class FakeResult:
+    """Stand-in for backup_manager.RestoreResult."""
+
+    def __init__(self, success=True, restored=None, errors=None,
+                 plugins_to_install=None):
+        self.success = success
+        self.restored = restored if restored is not None else ["config"]
+        self.errors = errors or []
+        self.plugins_to_install = plugins_to_install or []
+        self.plugins_installed = []
+        self.plugins_failed = []
+
+    def to_dict(self):
+        return {
+            "success": self.success,
+            "restored": self.restored,
+            "errors": self.errors,
+            "plugins_installed": self.plugins_installed,
+            "plugins_failed": self.plugins_failed,
+        }
+
+
+@pytest.fixture
+def client():
+    originals = {name: getattr(api_v3, name, _SENTINEL) for name in _MANAGER_ATTRS}
+    for name in _MANAGER_ATTRS:
+        setattr(api_v3, name, MagicMock())
+
+    app = Flask(__name__)
+    app.config["TESTING"] = True
+    app.register_blueprint(api_v3, url_prefix="/api/v3")
+    yield app.test_client()
+
+    for name, original in originals.items():
+        if original is _SENTINEL:
+            if hasattr(api_v3, name):
+                delattr(api_v3, name)
+        else:
+            setattr(api_v3, name, original)
+
+
+@pytest.fixture
+def restore():
+    """Patch backup_manager.restore_backup (imported inside the handler)."""
+    with patch("src.backup_manager.restore_backup") as mock:
+        mock.return_value = FakeResult()
+        yield mock
+
+
+def post(client, options=None, filename="backup.zip", content=b"PK\x03\x04fake"):
+    data = {"backup_file": (io.BytesIO(content), filename)}
+    if options is not None:
+        data["options"] = options
+    return client.post(URL, data=data, content_type="multipart/form-data")
+
+
+class TestRequestValidation:
+    def test_missing_file_is_a_400(self, client, restore):
+        response = client.post(URL, data={}, content_type="multipart/form-data")
+        assert response.status_code == 400
+        assert "No backup_file" in response.get_json()["message"]
+        restore.assert_not_called()
+
+    def test_absent_options_defaults_to_a_full_restore(self, client, restore):
+        # Documented default, not the bug: omitting options entirely means
+        # "restore everything".
+        post(client)
+        options = restore.call_args[0][2]
+        assert options.restore_config is True
+        assert options.restore_secrets is True
+        assert options.reinstall_plugins is True
+
+    def test_partial_options_are_honoured(self, client, restore):
+        post(client, options=json.dumps({
+            "restore_secrets": False, "reinstall_plugins": False}))
+        options = restore.call_args[0][2]
+        assert options.restore_secrets is False
+        assert options.reinstall_plugins is False
+        assert options.restore_config is True  # unspecified stays default
+
+    @pytest.mark.parametrize("raw", ["{not json", "", "{'single': 'quotes'}"])
+    def test_malformed_options_are_refused(self, client, restore, raw):
+        # Regression: this fell back to {}, and every flag defaults to
+        # True, so a caller asking for a narrow restore and mis-serializing
+        # it got a full one — secrets overwritten — and no warning.
+        response = post(client, options=raw)
+        assert response.status_code == 400
+        assert "Invalid options" in response.get_json()["message"]
+        restore.assert_not_called()
+
+    @pytest.mark.parametrize("raw", ["[1,2,3]", '"a string"', "42", "true", "null"])
+    def test_options_that_are_not_an_object_are_refused(self, client, restore, raw):
+        response = post(client, options=raw)
+        assert response.status_code == 400
+        restore.assert_not_called()
+
+    def test_empty_object_is_accepted_as_all_defaults(self, client, restore):
+        assert post(client, options="{}").status_code == 200
+        assert restore.call_args[0][2].restore_config is True
+
+
+class TestSuccess:
+    def test_success_returns_the_result(self, client, restore):
+        restore.return_value = FakeResult(success=True, restored=["config", "secrets"])
+        response = post(client)
+        assert response.status_code == 200
+        body = response.get_json()
+        assert body["status"] == "success"
+        assert body["data"]["restored"] == ["config", "secrets"]
+
+    def test_temp_file_is_cleaned_up(self, client, restore):
+        seen = {}
+
+        def capture(path, project_root, options):
+            seen["path"] = Path(path)
+            assert seen["path"].exists()  # present while restoring
+            return FakeResult()
+
+        restore.side_effect = capture
+        post(client)
+        assert not seen["path"].exists()
+
+    def test_temp_file_cleaned_up_even_when_restore_raises(self, client, restore):
+        seen = {}
+
+        def blow_up(path, project_root, options):
+            seen["path"] = Path(path)
+            raise RuntimeError("corrupt archive")
+
+        restore.side_effect = blow_up
+        response = post(client)
+        assert response.status_code == 500
+        assert not seen["path"].exists()
+
+
+class TestPluginReinstall:
+    def test_plugins_are_reinstalled_when_requested(self, client, restore):
+        restore.return_value = FakeResult(
+            plugins_to_install=[{"plugin_id": "clock"}, {"plugin_id": "weather"}])
+        api_v3.plugin_store_manager.install_plugin.return_value = True
+        response = post(client)
+        assert response.status_code == 200
+        assert response.get_json()["data"]["plugins_installed"] == ["clock", "weather"]
+
+    def test_reinstall_skipped_when_not_requested(self, client, restore):
+        restore.return_value = FakeResult(plugins_to_install=[{"plugin_id": "clock"}])
+        post(client, options=json.dumps({"reinstall_plugins": False}))
+        api_v3.plugin_store_manager.install_plugin.assert_not_called()
+
+    def test_entries_without_a_plugin_id_are_skipped(self, client, restore):
+        restore.return_value = FakeResult(plugins_to_install=[{}, {"plugin_id": "clock"}])
+        api_v3.plugin_store_manager.install_plugin.return_value = True
+        post(client)
+        assert api_v3.plugin_store_manager.install_plugin.call_count == 1
+
+    def test_failed_reinstall_turns_the_whole_restore_into_an_error(
+            self, client, restore):
+        # Pinned as intentional: file restoration succeeded and does not
+        # touch result.errors, but a user whose plugins did not come back
+        # should not be told the restore was a success.
+        restore.return_value = FakeResult(
+            success=True, plugins_to_install=[{"plugin_id": "clock"}])
+        api_v3.plugin_store_manager.install_plugin.return_value = False
+        response = post(client)
+        assert response.status_code == 500
+        body = response.get_json()
+        assert body["status"] == "error"
+        assert "clock" in body["message"]
+
+    def test_message_names_what_landed_and_what_did_not(self, client, restore):
+        restore.return_value = FakeResult(
+            success=True, restored=["config", "fonts"],
+            plugins_to_install=[{"plugin_id": "clock"}])
+        api_v3.plugin_store_manager.install_plugin.return_value = False
+        message = post(client).get_json()["message"]
+        assert "restored: config, fonts" in message
+        assert "plugins not reinstalled: clock" in message
+
+    def test_install_exception_is_recorded_without_leaking_details(
+            self, client, restore):
+        restore.return_value = FakeResult(plugins_to_install=[{"plugin_id": "clock"}])
+        api_v3.plugin_store_manager.install_plugin.side_effect = RuntimeError(
+            "/srv/internal/path exploded")
+        body = post(client).get_json()
+        failures = body["data"]["plugins_failed"]
+        assert failures[0]["plugin_id"] == "clock"
+        assert "/srv/internal/path" not in json.dumps(body)
+
+    def test_missing_store_manager_is_reported_per_plugin(self, client, restore):
+        restore.return_value = FakeResult(plugins_to_install=[{"plugin_id": "clock"}])
+        api_v3.plugin_store_manager = None
+        with patch("web_interface.blueprints.api_v3.plugin_store_manager", None):
+            body = post(client).get_json()
+        assert body["data"]["plugins_failed"][0]["error"] == "Store manager unavailable"
+
+
+class TestFailureReporting:
+    def test_restore_errors_produce_a_500(self, client, restore):
+        restore.return_value = FakeResult(
+            success=False, restored=[], errors=["config: permission denied"])
+        response = post(client)
+        assert response.status_code == 500
+        assert "permission denied" in response.get_json()["message"]
+
+    def test_partial_restore_names_both_sides(self, client, restore):
+        restore.return_value = FakeResult(
+            success=False, restored=["config"], errors=["secrets: unwritable"])
+        message = post(client).get_json()["message"]
+        assert "restored: config" in message
+        assert "failed: secrets: unwritable" in message
+
+    def test_failure_without_detail_still_says_something(self, client, restore):
+        restore.return_value = FakeResult(success=False, restored=[], errors=[])
+        message = post(client).get_json()["message"]
+        assert "Restore incomplete" in message
+
+    def test_unexpected_exception_is_a_500(self, client, restore):
+        restore.side_effect = RuntimeError("boom")
+        response = post(client)
+        assert response.status_code == 500
+        assert response.get_json()["status"] == "error"

test/web_interface/test_api_v3_config_raw.py

@@ -0,0 +1,199 @@
+"""
+Endpoint tests for POST /config/raw/main and POST /config/raw/secrets.
+
+These write whatever JSON they are given straight to config.json and
+config_secrets.json, bypassing the secret-separation path that
+/config/main and the plugin-config endpoints go through. Given how much
+care the rest of the config surface takes to keep secrets out of
+config.json, an untested pair of endpoints that writes it verbatim is
+worth pinning precisely.
+
+Like test_api_v3_secret_roundtrip.py, these run a REAL ConfigManager over
+tmp_path so the assertions are against files on disk rather than mock
+calls.
+"""
+
+import json
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+from flask import Flask
+
+project_root = Path(__file__).parent.parent.parent
+sys.path.insert(0, str(project_root))
+
+from src.config_manager import ConfigManager  # noqa: E402
+from src.exceptions import ConfigError  # noqa: E402
+from web_interface.blueprints.api_v3 import api_v3  # noqa: E402
+
+MAIN = "/api/v3/config/raw/main"
+SECRETS = "/api/v3/config/raw/secrets"
+
+
+@pytest.fixture
+def env(tmp_path):
+    config_file = tmp_path / "config.json"
+    config_file.write_text(json.dumps({"timezone": "UTC"}))
+    secrets_file = tmp_path / "config_secrets.json"
+
+    config_manager = ConfigManager(
+        config_path=str(config_file), secrets_path=str(secrets_file))
+    config_manager.template_path = str(tmp_path / "no-template.json")
+
+    _SENTINEL = object()
+    attrs = ('config_manager', 'plugin_manager', 'plugin_store_manager',
+             'plugin_state_manager', 'saved_repositories_manager',
+             'schema_manager', 'operation_queue', 'operation_history',
+             'cache_manager')
+    originals = {name: getattr(api_v3, name, _SENTINEL) for name in attrs}
+
+    for name in attrs:
+        setattr(api_v3, name, MagicMock())
+    api_v3.config_manager = config_manager
+
+    app = Flask(__name__)
+    app.config["TESTING"] = True
+    app.register_blueprint(api_v3, url_prefix="/api/v3")
+
+    class Env:
+        pass
+
+    e = Env()
+    e.client = app.test_client()
+    e.config_manager = config_manager
+    e.config_file = config_file
+    e.secrets_file = secrets_file
+    yield e
+
+    for name, original in originals.items():
+        if original is _SENTINEL:
+            if hasattr(api_v3, name):
+                delattr(api_v3, name)
+        else:
+            setattr(api_v3, name, original)
+
+
+class TestSaveRawMain:
+    def test_writes_the_body_to_config_json(self, env):
+        response = env.client.post(MAIN, json={"timezone": "America/Chicago"})
+        assert response.status_code == 200
+        assert json.loads(env.config_file.read_text()) == {"timezone": "America/Chicago"}
+
+    def test_replaces_rather_than_merges(self, env):
+        env.client.post(MAIN, json={"only": "this"})
+        assert json.loads(env.config_file.read_text()) == {"only": "this"}
+
+    def test_does_not_touch_the_secrets_file(self, env):
+        env.secrets_file.write_text(json.dumps({"weather": {"api_key": "k"}}))
+        env.client.post(MAIN, json={"timezone": "UTC"})
+        assert json.loads(env.secrets_file.read_text()) == {"weather": {"api_key": "k"}}
+
+    def test_uninitialized_manager_is_a_500(self, env):
+        api_v3.config_manager = None
+        response = env.client.post(MAIN, json={"timezone": "UTC"})
+        assert response.status_code == 500
+        assert "not initialized" in response.get_json()["message"]
+
+    def test_empty_object_is_a_400(self, env):
+        response = env.client.post(MAIN, json={})
+        assert response.status_code == 400
+        assert "No data provided" in response.get_json()["message"]
+
+    def test_bodyless_post_is_a_400(self, env):
+        response = env.client.post(MAIN)
+        assert response.status_code == 400
+        assert "No data provided" in response.get_json()["message"]
+
+    def test_malformed_json_is_a_400_in_the_app_shape(self, env):
+        response = env.client.post(MAIN, data="{not json",
+                                   content_type="application/json")
+        assert response.status_code == 400
+        body = response.get_json()
+        assert body["status"] == "error"
+
+    def test_config_error_is_a_500_with_context(self, env, monkeypatch):
+        def refuse(kind, data):
+            raise ConfigError("cannot write", config_path="/etc/x.json")
+        monkeypatch.setattr(env.config_manager, "save_raw_file_content", refuse)
+        response = env.client.post(MAIN, json={"timezone": "UTC"})
+        assert response.status_code == 500
+        assert "/etc/x.json" in json.dumps(response.get_json())
+
+    def test_unexpected_error_is_a_500(self, env, monkeypatch):
+        def boom(kind, data):
+            raise RuntimeError("disk on fire")
+        monkeypatch.setattr(env.config_manager, "save_raw_file_content", boom)
+        response = env.client.post(MAIN, json={"timezone": "UTC"})
+        assert response.status_code == 500
+        assert response.get_json()["status"] == "error"
+
+
+class TestSaveRawSecrets:
+    def test_writes_only_to_the_secrets_file(self, env):
+        response = env.client.post(SECRETS, json={"weather": {"api_key": "s3cret"}})
+        assert response.status_code == 200
+        assert json.loads(env.secrets_file.read_text()) == {"weather": {"api_key": "s3cret"}}
+
+    def test_secret_values_never_reach_config_json(self, env):
+        env.client.post(SECRETS, json={"weather": {"api_key": "s3cret"}})
+        assert "s3cret" not in env.config_file.read_text()
+
+    def test_existing_main_config_is_untouched(self, env):
+        before = env.config_file.read_text()
+        env.client.post(SECRETS, json={"weather": {"api_key": "k"}})
+        assert env.config_file.read_text() == before
+
+    def test_github_token_is_reloaded_for_the_store_manager(self, env):
+        store = MagicMock()
+        store._load_github_token.return_value = "ghp_new"
+        api_v3.plugin_store_manager = store
+        env.client.post(SECRETS, json={"github": {"token": "ghp_new"}})
+        store._load_github_token.assert_called_once()
+        assert store.github_token == "ghp_new"
+
+    def test_absent_store_manager_is_fine(self, env):
+        api_v3.plugin_store_manager = None
+        assert env.client.post(SECRETS, json={"a": 1}).status_code == 200
+
+    def test_uninitialized_manager_is_a_500(self, env):
+        api_v3.config_manager = None
+        assert env.client.post(SECRETS, json={"a": 1}).status_code == 500
+
+    def test_empty_object_is_a_400(self, env):
+        assert env.client.post(SECRETS, json={}).status_code == 400
+
+    def test_bodyless_post_is_a_400(self, env):
+        assert env.client.post(SECRETS).status_code == 400
+
+    def test_error_is_a_500(self, env, monkeypatch):
+        def boom(kind, data):
+            raise RuntimeError("nope")
+        monkeypatch.setattr(env.config_manager, "save_raw_file_content", boom)
+        assert env.client.post(SECRETS, json={"a": 1}).status_code == 500
+
+
+class TestRawEndpointsBypassSecretSeparation:
+    """Pinned behaviour, deliberately not "fixed".
+
+    These endpoints are the escape hatch for editing the config files
+    directly from the web UI's raw JSON editor. They write what they are
+    given, so a secret typed into the main-config editor lands in
+    config.json in plain text — unlike /config/main and the plugin-config
+    endpoints, which route x-secret fields into config_secrets.json.
+
+    That is the point of a raw editor, but it is a sharp edge worth
+    stating out loud: anyone adding a "convenience" that posts plugin
+    config through this endpoint would silently lose secret separation.
+    """
+
+    def test_secret_shaped_keys_are_written_verbatim_to_main(self, env):
+        env.client.post(MAIN, json={"weather": {"api_key": "PLAINTEXT-KEY"}})
+        on_disk = json.loads(env.config_file.read_text())
+        assert on_disk["weather"]["api_key"] == "PLAINTEXT-KEY"
+
+    def test_no_separation_happens_on_the_raw_path(self, env):
+        env.client.post(MAIN, json={"weather": {"api_key": "PLAINTEXT-KEY"}})
+        # Nothing was moved aside into the secrets file.
+        assert not env.secrets_file.exists() or "PLAINTEXT-KEY" not in env.secrets_file.read_text()

test/web_interface/test_error_handler.py

@@ -0,0 +1,149 @@
+"""
+Tests for the response builders in src/web_interface/error_handler.py and
+the success path in src/web_interface/api_helpers.py.
+
+describe_exception() in the same module is already covered by
+test/test_web_error_detail.py and is not duplicated here.
+
+Regression coverage for one fixed bug: create_success_response used
+truthiness for `message` and `metadata` while using `is not None` for
+`data`, so an explicitly-passed "" or {} was silently dropped —
+api_helpers.success_response() repeated the same gate, which is the path
+every api_v3 endpoint actually calls.
+"""
+
+import pytest
+from flask import Flask
+
+from src.web_interface.api_helpers import success_response
+from src.web_interface.error_handler import (
+    create_error_response,
+    create_success_response,
+)
+from src.web_interface.errors import ErrorCode, WebInterfaceError
+
+
+@pytest.fixture
+def app():
+    return Flask(__name__)
+
+
+class TestCreateErrorResponse:
+    def test_returns_response_and_status_tuple(self, app):
+        with app.test_request_context():
+            response, status = create_error_response(
+                ErrorCode.CONFIG_SAVE_FAILED, "could not save")
+        assert status == 500
+        assert response.get_json()["message"] == "could not save"
+
+    def test_status_code_passthrough(self, app):
+        with app.test_request_context():
+            _, status = create_error_response(
+                ErrorCode.INVALID_INPUT, "bad", status_code=400)
+        assert status == 400
+
+    def test_body_matches_the_error_dataclass(self, app):
+        with app.test_request_context():
+            response, _ = create_error_response(
+                ErrorCode.NETWORK_ERROR, "offline",
+                details="connection refused", context={"url": "http://x"})
+        expected = WebInterfaceError(
+            error_code=ErrorCode.NETWORK_ERROR, message="offline",
+            details="connection refused", context={"url": "http://x"}).to_dict()
+        assert response.get_json() == expected
+
+    def test_none_context_produces_no_context_key(self, app):
+        with app.test_request_context():
+            response, _ = create_error_response(ErrorCode.SYSTEM_ERROR, "boom")
+        assert "context" not in response.get_json()
+
+    def test_suggested_fixes_passed_through(self, app):
+        with app.test_request_context():
+            response, _ = create_error_response(
+                ErrorCode.SYSTEM_ERROR, "boom", suggested_fixes=["Try again"])
+        assert response.get_json()["suggested_fixes"] == ["Try again"]
+
+
+class TestCreateSuccessResponse:
+    def test_bare_success(self):
+        assert create_success_response() == {"status": "success"}
+
+    def test_data_included(self):
+        assert create_success_response(data={"a": 1})["data"] == {"a": 1}
+
+    @pytest.mark.parametrize("falsy", [0, "", False, {}, []])
+    def test_falsy_data_is_still_included(self, falsy):
+        assert create_success_response(data=falsy)["data"] == falsy
+
+    def test_none_data_omitted(self):
+        assert "data" not in create_success_response(data=None)
+
+    def test_message_included(self):
+        assert create_success_response(message="done")["message"] == "done"
+
+    def test_empty_message_is_still_included(self):
+        # Regression: `if message:` dropped an explicitly-passed "".
+        assert create_success_response(message="")["message"] == ""
+
+    def test_none_message_omitted(self):
+        assert "message" not in create_success_response(message=None)
+
+    def test_metadata_included(self):
+        assert create_success_response(metadata={"v": 1})["metadata"] == {"v": 1}
+
+    def test_empty_metadata_is_still_included(self):
+        # Regression: `if metadata:` dropped an explicitly-passed {}.
+        assert create_success_response(metadata={})["metadata"] == {}
+
+    def test_none_metadata_omitted(self):
+        assert "metadata" not in create_success_response(metadata=None)
+
+
+class TestSuccessResponseHelper:
+    """api_helpers.success_response — the wrapper every endpoint calls."""
+
+    def test_plain_response_has_no_metadata_block(self, app):
+        with app.test_request_context():
+            body = success_response(data={"a": 1}).get_json()
+        assert body == {"status": "success", "data": {"a": 1}}
+
+    def test_explicit_empty_metadata_survives_the_wrapper(self, app):
+        # Regression: the wrapper re-gated metadata on truthiness after
+        # create_success_response had already included it, so {} was
+        # dropped again on the way out.
+        with app.test_request_context():
+            body = success_response(data=None, metadata={}).get_json()
+        assert body["metadata"] == {}
+
+    def test_caller_metadata_preserved(self, app):
+        with app.test_request_context():
+            body = success_response(metadata={"version": "1.2"}).get_json()
+        assert body["metadata"]["version"] == "1.2"
+
+    def test_timing_added_when_request_has_start_time(self, app):
+        with app.test_request_context() as ctx:
+            ctx.request.start_time = 0.0
+            body = success_response(data={"a": 1}).get_json()
+        assert "response_time_ms" in body["metadata"]
+
+    def test_timing_merges_with_caller_metadata(self, app):
+        with app.test_request_context() as ctx:
+            ctx.request.start_time = 0.0
+            body = success_response(metadata={"version": "1.2"}).get_json()
+        assert body["metadata"]["version"] == "1.2"
+        assert "response_time_ms" in body["metadata"]
+
+    def test_caller_metadata_dict_is_not_mutated(self, app):
+        # The helper used to add response_time_ms straight into the dict the
+        # caller passed, so a module-level or reused metadata dict would
+        # accumulate timings from previous requests.
+        caller_metadata = {"version": "1.2"}
+        with app.test_request_context() as ctx:
+            ctx.request.start_time = 0.0
+            success_response(metadata=caller_metadata)
+        assert caller_metadata == {"version": "1.2"}
+
+    def test_message_passed_through(self, app):
+        with app.test_request_context():
+            body = success_response(message="saved").get_json()
+        assert body["message"] == "saved"

test/web_interface/test_errors.py

@@ -0,0 +1,208 @@
+"""
+Tests for src/web_interface/errors.py — the structured error type behind
+every API error response (category inference, default suggestions, the
+JSON shape, and exception conversion).
+
+Pure logic; no Flask context needed.
+
+Regression coverage for one fixed bug: suggested_fixes used `or`, so a
+caller passing [] to mean "no suggestions" silently got the default list.
+"""
+
+import pytest
+
+from src.web_interface.errors import ErrorCategory, ErrorCode, WebInterfaceError
+
+
+class TestCategoryInference:
+    @pytest.mark.parametrize("code,expected", [
+        (ErrorCode.CONFIG_SAVE_FAILED, ErrorCategory.CONFIGURATION),
+        (ErrorCode.CONFIG_ROLLBACK_FAILED, ErrorCategory.CONFIGURATION),
+        (ErrorCode.PLUGIN_NOT_FOUND, ErrorCategory.PLUGIN),
+        (ErrorCode.PLUGIN_OPERATION_CONFLICT, ErrorCategory.PLUGIN),
+        (ErrorCode.VALIDATION_ERROR, ErrorCategory.VALIDATION),
+        (ErrorCode.SCHEMA_VALIDATION_FAILED, ErrorCategory.VALIDATION),
+        (ErrorCode.INVALID_INPUT, ErrorCategory.VALIDATION),
+        (ErrorCode.NETWORK_ERROR, ErrorCategory.NETWORK),
+        (ErrorCode.API_ERROR, ErrorCategory.NETWORK),
+        (ErrorCode.TIMEOUT, ErrorCategory.NETWORK),
+        (ErrorCode.PERMISSION_DENIED, ErrorCategory.PERMISSION),
+        (ErrorCode.FILE_PERMISSION_ERROR, ErrorCategory.PERMISSION),
+        (ErrorCode.SYSTEM_ERROR, ErrorCategory.SYSTEM),
+        (ErrorCode.SERVICE_UNAVAILABLE, ErrorCategory.SYSTEM),
+        (ErrorCode.UNKNOWN_ERROR, ErrorCategory.UNKNOWN),
+    ])
+    def test_every_code_prefix_maps_to_its_category(self, code, expected):
+        assert WebInterfaceError(code, "msg").category is expected
+
+    def test_explicit_category_overrides_inference(self):
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", category=ErrorCategory.SYSTEM)
+        assert error.category is ErrorCategory.SYSTEM
+
+    def test_every_error_code_gets_a_category(self):
+        # No code may fall through uncategorized as the enum grows.
+        for code in ErrorCode:
+            assert isinstance(WebInterfaceError(code, "msg").category, ErrorCategory)
+
+
+class TestDefaultSuggestions:
+    def test_mapped_code_gets_specific_suggestions(self):
+        fixes = WebInterfaceError(ErrorCode.CONFIG_SAVE_FAILED, "msg").suggested_fixes
+        assert "Check available disk space" in fixes
+
+    def test_unmapped_code_gets_generic_fallback(self):
+        # PLUGIN_UPDATE_FAILED has no entry in suggestions_map.
+        fixes = WebInterfaceError(ErrorCode.PLUGIN_UPDATE_FAILED, "msg").suggested_fixes
+        assert fixes == ["Review error details and try again"]
+
+    def test_explicit_suggestions_win(self):
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=["Do the thing"])
+        assert error.suggested_fixes == ["Do the thing"]
+
+    def test_explicit_empty_list_is_respected(self):
+        # Regression: `suggested_fixes or default` treated [] as "unset",
+        # so a caller could not express "I have no suggestions".
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=[])
+        assert error.suggested_fixes == []
+
+    def test_none_still_gets_defaults(self):
+        error = WebInterfaceError(
+            ErrorCode.CONFIG_SAVE_FAILED, "msg", suggested_fixes=None)
+        assert len(error.suggested_fixes) > 0
+
+
+class TestToDict:
+    def test_base_keys_always_present(self):
+        result = WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom").to_dict()
+        assert result["status"] == "error"
+        assert result["error_code"] == "SYSTEM_ERROR"
+        assert result["error_category"] == "system"
+        assert result["message"] == "boom"
+
+    def test_details_included_when_set(self):
+        result = WebInterfaceError(
+            ErrorCode.SYSTEM_ERROR, "boom", details="disk full").to_dict()
+        assert result["details"] == "disk full"
+
+    def test_details_omitted_when_absent(self):
+        assert "details" not in WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom").to_dict()
+
+    def test_context_included_when_non_empty(self):
+        result = WebInterfaceError(
+            ErrorCode.SYSTEM_ERROR, "boom", context={"path": "/tmp/x"}).to_dict()
+        assert result["context"] == {"path": "/tmp/x"}
+
+    def test_empty_context_is_omitted(self):
+        # Pinned as intentional, not a bug: __init__ normalizes context to
+        # {}, and an empty context carries no information, so it is left out
+        # rather than padding every error body with "context": {}.
+        result = WebInterfaceError(ErrorCode.SYSTEM_ERROR, "boom", context={}).to_dict()
+        assert "context" not in result
+
+    def test_empty_suggestions_omitted(self):
+        result = WebInterfaceError(
+            ErrorCode.SYSTEM_ERROR, "boom", suggested_fixes=[]).to_dict()
+        assert "suggested_fixes" not in result
+
+    def test_is_json_serializable(self):
+        import json
+        error = WebInterfaceError(
+            ErrorCode.NETWORK_ERROR, "boom",
+            details="timeout", context={"url": "http://x"})
+        assert json.loads(json.dumps(error.to_dict()))["error_code"] == "NETWORK_ERROR"
+
+
+class TestFromException:
+    @pytest.mark.parametrize("exc_name,expected", [
+        ("ConfigError", ErrorCode.CONFIG_LOAD_FAILED),
+        ("PluginError", ErrorCode.PLUGIN_LOAD_FAILED),
+        ("PermissionError", ErrorCode.PERMISSION_DENIED),
+        ("AccessDenied", ErrorCode.PERMISSION_DENIED),
+        ("ValidationError", ErrorCode.VALIDATION_ERROR),
+        ("SchemaError", ErrorCode.VALIDATION_ERROR),
+        ("NetworkError", ErrorCode.NETWORK_ERROR),
+        ("ConnectionError", ErrorCode.NETWORK_ERROR),
+        ("TimeoutError", ErrorCode.TIMEOUT),
+        ("SomethingElse", ErrorCode.UNKNOWN_ERROR),
+    ])
+    def test_code_inferred_from_exception_class_name(self, exc_name, expected):
+        exc = type(exc_name, (Exception,), {})("boom")
+        assert WebInterfaceError.from_exception(exc).error_code is expected
+
+    def test_explicit_code_skips_inference(self):
+        error = WebInterfaceError.from_exception(
+            ValueError("boom"), error_code=ErrorCode.PLUGIN_NOT_FOUND)
+        assert error.error_code is ErrorCode.PLUGIN_NOT_FOUND
+
+    def test_message_is_the_safe_one_not_the_exception_text(self):
+        # The raw exception text is not echoed into `message`; that field is
+        # a fixed, user-facing string per code.
+        error = WebInterfaceError.from_exception(ValueError("secret-ish detail"))
+        assert error.message == "An unexpected error occurred"
+        assert "secret-ish" not in error.message
+
+    def test_exception_type_recorded_in_context(self):
+        error = WebInterfaceError.from_exception(ValueError("boom"))
+        assert error.context["exception_type"] == "ValueError"
+
+    def test_caller_context_is_preserved_alongside_type(self):
+        error = WebInterfaceError.from_exception(
+            ValueError("boom"), context={"plugin_id": "clock"})
+        assert error.context["plugin_id"] == "clock"
+        assert error.context["exception_type"] == "ValueError"
+
+    def test_caller_supplied_exception_type_is_overwritten(self):
+        error = WebInterfaceError.from_exception(
+            ValueError("boom"), context={"exception_type": "Fake"})
+        assert error.context["exception_type"] == "ValueError"
+
+    def test_original_error_retained(self):
+        exc = ValueError("boom")
+        assert WebInterfaceError.from_exception(exc).original_error is exc
+
+    def test_every_code_has_a_safe_message(self):
+        for code in ErrorCode:
+            assert WebInterfaceError._safe_message(code)
+
+
+class TestExceptionDetails:
+    def test_context_dict_is_flattened(self):
+        exc = ValueError("boom")
+        exc.context = {"config_path": "/etc/x.json", "line": 4}
+        details = WebInterfaceError._get_exception_details(exc)
+        assert "config_path: /etc/x.json" in details
+        assert "line: 4" in details
+        assert "; " in details
+
+    def test_exception_type_key_excluded(self):
+        exc = ValueError("boom")
+        exc.context = {"exception_type": "ValueError", "path": "/tmp/x"}
+        details = WebInterfaceError._get_exception_details(exc)
+        assert "exception_type" not in details
+        assert details == "path: /tmp/x"
+
+    def test_context_with_only_exception_type_gives_none(self):
+        exc = ValueError("boom")
+        exc.context = {"exception_type": "ValueError"}
+        assert WebInterfaceError._get_exception_details(exc) is None
+
+    def test_no_context_attribute_gives_none(self):
+        assert WebInterfaceError._get_exception_details(ValueError("boom")) is None
+
+    def test_non_dict_context_gives_none(self):
+        exc = ValueError("boom")
+        exc.context = "not a dict"
+        assert WebInterfaceError._get_exception_details(exc) is None
+
+    def test_empty_context_gives_none(self):
+        exc = ValueError("boom")
+        exc.context = {}
+        assert WebInterfaceError._get_exception_details(exc) is None
+
+    def test_details_flow_into_from_exception(self):
+        exc = ValueError("boom")
+        exc.context = {"config_path": "/etc/x.json"}
+        assert "config_path" in WebInterfaceError.from_exception(exc).details

test/web_interface/test_validators.py

@@ -0,0 +1,284 @@
+"""
+Tests for src/web_interface/validators.py.
+
+dedup_unique_arrays is already covered by test_dedup_unique_arrays.py and
+is not repeated here; this file covers the other eight functions, none of
+which had any tests.
+
+Regression coverage for three fixed bugs:
+- validate_numeric_range accepted True/False, since bool subclasses int.
+- validate_file_upload lowercased the filename's extension but not the
+  caller's allowed_extensions list, so ['.TTF'] rejected 'font.ttf'.
+- validate_image_url only checked for '..' inside the relative-path
+  branch, so http://host/../secret passed validation untouched.
+"""
+
+import pytest
+
+from src.web_interface.validators import (
+    escape_html,
+    sanitize_plugin_config,
+    validate_file_upload,
+    validate_font_awesome_class,
+    validate_image_url,
+    validate_mime_type,
+    validate_numeric_range,
+    validate_string_length,
+)
+
+
+class TestEscapeHtml:
+    def test_escapes_all_five_entities(self):
+        assert escape_html("""<a href="x">O'Neill & co</a>""") == (
+            "&lt;a href=&quot;x&quot;&gt;O&#x27;Neill &amp; co&lt;/a&gt;")
+
+    def test_ampersand_is_escaped_first_so_nothing_double_escapes(self):
+        # If '<' were replaced before '&', the '&' of '&lt;' would be
+        # escaped again into '&amp;lt;'.
+        assert escape_html("<") == "&lt;"
+        assert escape_html("&") == "&amp;"
+        assert escape_html("&<") == "&amp;&lt;"
+
+    def test_plain_text_unchanged(self):
+        assert escape_html("hello world") == "hello world"
+
+    def test_non_string_is_coerced(self):
+        assert escape_html(42) == "42"
+        assert escape_html(None) == "None"
+
+    def test_script_tag_neutralized(self):
+        assert "<script>" not in escape_html("<script>alert(1)</script>")
+
+
+class TestValidateImageUrl:
+    @pytest.mark.parametrize("url", [
+        "javascript:alert(1)",
+        "JavaScript:alert(1)",
+        "JAVASCRIPT:alert(1)",
+        "data:text/html;base64,PHNjcmlwdD4=",
+        "vbscript:msgbox(1)",
+        "file:///etc/passwd",
+    ])
+    def test_dangerous_protocols_rejected(self, url):
+        valid, error = validate_image_url(url)
+        assert valid is False and "protocol" in error.lower()
+
+    @pytest.mark.parametrize("url", [
+        "http://x/a.png?onerror=alert(1)",
+        "http://x/a.png#onload=alert(1)",
+        "http://x/onclick=alert(1).png",
+    ])
+    def test_event_handlers_rejected(self, url):
+        valid, error = validate_image_url(url)
+        assert valid is False and "Event handlers" in error
+
+    @pytest.mark.parametrize("url", ["", None, 123, []])
+    def test_empty_or_non_string_rejected(self, url):
+        assert validate_image_url(url)[0] is False
+
+    def test_http_and_https_allowed(self):
+        assert validate_image_url("http://example.com/logo.png") == (True, None)
+        assert validate_image_url("https://example.com/logo.png") == (True, None)
+
+    def test_other_schemes_rejected(self):
+        valid, error = validate_image_url("ftp://example.com/logo.png")
+        assert valid is False and "http://" in error
+
+    def test_relative_path_allowed(self):
+        assert validate_image_url("/static/logo.png") == (True, None)
+
+    def test_protocol_relative_url_rejected(self):
+        assert validate_image_url("//evil.com/logo.png")[0] is False
+
+    def test_relative_traversal_rejected(self):
+        assert validate_image_url("/static/../../etc/passwd")[0] is False
+
+    def test_absolute_url_traversal_rejected(self):
+        # Regression: the '..' check used to sit inside the leading-slash
+        # branch, so an absolute URL skipped it entirely.
+        valid, error = validate_image_url("http://example.com/../secret")
+        assert valid is False and "traversal" in error.lower()
+
+    def test_bare_traversal_rejected(self):
+        assert validate_image_url("../../etc/passwd")[0] is False
+
+
+class TestValidateFontAwesomeClass:
+    @pytest.mark.parametrize("cls", ["fa-star", "fas fa-star", "fa-solid fa-house"])
+    def test_valid_classes_accepted(self, cls):
+        assert validate_font_awesome_class(cls) == (True, None)
+
+    @pytest.mark.parametrize("cls", ["star", "glyphicon-star", ""])
+    def test_classes_without_fa_prefix_rejected(self, cls):
+        assert validate_font_awesome_class(cls)[0] is False
+
+    def test_injection_attempt_rejected(self):
+        assert validate_font_awesome_class('fa-star" onload="alert(1)')[0] is False
+
+    def test_angle_brackets_rejected(self):
+        assert validate_font_awesome_class("<script>fa-star</script>")[0] is False
+
+    def test_non_string_rejected(self):
+        valid, error = validate_font_awesome_class(None)
+        assert valid is False and "string" in error
+
+    def test_explicit_fa_check_is_unreachable_but_harmless(self):
+        # Characterized, not fixed: the regex already requires 'fa-', so the
+        # follow-up `if 'fa-' not in class_name` can never fire. Anything
+        # lacking 'fa-' is rejected by the pattern first, with the pattern's
+        # own message.
+        valid, error = validate_font_awesome_class("star")
+        assert valid is False
+        assert error == "Invalid Font Awesome class name format"
+
+
+class TestValidateFileUpload:
+    def test_plain_filename_accepted(self):
+        assert validate_file_upload("logo.png") == (True, None)
+
+    @pytest.mark.parametrize("filename", [
+        "../etc/passwd", "dir/file.png", "dir\\file.png", "..\\..\\secrets",
+    ])
+    def test_traversal_characters_rejected(self, filename):
+        valid, error = validate_file_upload(filename)
+        assert valid is False and "invalid characters" in error
+
+    @pytest.mark.parametrize("filename", ["", None, 123])
+    def test_empty_or_non_string_rejected(self, filename):
+        assert validate_file_upload(filename)[0] is False
+
+    def test_allowed_extension_accepted(self):
+        assert validate_file_upload("font.ttf", allowed_extensions=[".ttf", ".otf"]) == (True, None)
+
+    def test_disallowed_extension_rejected(self):
+        valid, error = validate_file_upload("evil.exe", allowed_extensions=[".ttf"])
+        assert valid is False and "extension" in error
+
+    def test_uppercase_filename_extension_matches(self):
+        assert validate_file_upload("FONT.TTF", allowed_extensions=[".ttf"]) == (True, None)
+
+    def test_uppercase_allowed_list_matches(self):
+        # Regression: only the filename side was lowercased, so a caller
+        # passing ['.TTF'] rejected every valid .ttf upload.
+        assert validate_file_upload("font.ttf", allowed_extensions=[".TTF"]) == (True, None)
+
+    def test_no_extension_list_skips_the_check(self):
+        assert validate_file_upload("anything.xyz") == (True, None)
+
+
+class TestValidateMimeType:
+    def test_known_type_accepted(self):
+        assert validate_mime_type("logo.png", ["image/png"]) == (True, None)
+
+    def test_mismatched_type_rejected(self):
+        valid, error = validate_mime_type("logo.png", ["image/jpeg"])
+        assert valid is False and "not allowed" in error
+
+    def test_undeterminable_type_rejected(self):
+        valid, error = validate_mime_type("mystery.zzz", ["image/png"])
+        assert valid is False and "Could not determine" in error
+
+    def test_guess_type_failure_is_caught(self, monkeypatch):
+        import mimetypes
+        monkeypatch.setattr(mimetypes, "guess_type",
+                            lambda *a, **kw: (_ for _ in ()).throw(RuntimeError("boom")))
+        valid, error = validate_mime_type("logo.png", ["image/png"])
+        assert valid is False and "Error validating MIME type" in error
+
+
+class TestValidateNumericRange:
+    def test_value_in_range(self):
+        assert validate_numeric_range(5, min_val=0, max_val=10) == (True, None)
+
+    def test_boundaries_are_inclusive(self):
+        assert validate_numeric_range(0, min_val=0, max_val=10) == (True, None)
+        assert validate_numeric_range(10, min_val=0, max_val=10) == (True, None)
+
+    def test_below_minimum_rejected(self):
+        valid, error = validate_numeric_range(-1, min_val=0)
+        assert valid is False and "at least" in error
+
+    def test_above_maximum_rejected(self):
+        valid, error = validate_numeric_range(11, max_val=10)
+        assert valid is False and "at most" in error
+
+    def test_floats_accepted(self):
+        assert validate_numeric_range(2.5, min_val=0, max_val=10) == (True, None)
+
+    def test_no_bounds_accepts_any_number(self):
+        assert validate_numeric_range(-9999) == (True, None)
+
+    @pytest.mark.parametrize("value", ["5", None, [], {}])
+    def test_non_numeric_rejected(self, value):
+        valid, error = validate_numeric_range(value, min_val=0, max_val=10)
+        assert valid is False and error == "Value must be a number"
+
+    @pytest.mark.parametrize("value", [True, False])
+    def test_booleans_rejected(self, value):
+        # Regression: bool subclasses int, so True passed the isinstance
+        # check and then compared as 1 against the range.
+        valid, error = validate_numeric_range(value, min_val=0, max_val=10)
+        assert valid is False and error == "Value must be a number"
+
+
+class TestValidateStringLength:
+    def test_within_range(self):
+        assert validate_string_length("hello", min_length=1, max_length=10) == (True, None)
+
+    def test_boundaries_are_inclusive(self):
+        assert validate_string_length("abc", min_length=3, max_length=3) == (True, None)
+
+    def test_too_short_rejected(self):
+        valid, error = validate_string_length("", min_length=1)
+        assert valid is False and "at least" in error
+
+    def test_too_long_rejected(self):
+        valid, error = validate_string_length("abcdef", max_length=3)
+        assert valid is False and "at most" in error
+
+    def test_non_string_rejected(self):
+        valid, error = validate_string_length(123, max_length=10)
+        assert valid is False and "must be a string" in error
+
+    def test_no_bounds_accepts_anything(self):
+        assert validate_string_length("") == (True, None)
+
+
+class TestSanitizePluginConfig:
+    def test_valid_keys_and_scalars_kept(self):
+        config = {"enabled": True, "count": 3, "ratio": 1.5, "name": "clock"}
+        assert sanitize_plugin_config(config) == config
+
+    @pytest.mark.parametrize("key", ["has space", "has-dash", "has.dot", "has/slash", ""])
+    def test_invalid_key_names_dropped(self, key):
+        assert sanitize_plugin_config({key: "value", "good": 1}) == {"good": 1}
+
+    def test_non_string_keys_dropped(self):
+        assert sanitize_plugin_config({1: "a", "good": 2}) == {"good": 2}
+
+    def test_nested_dicts_recursed(self):
+        result = sanitize_plugin_config({"outer": {"inner": 1, "bad key": 2}})
+        assert result == {"outer": {"inner": 1}}
+
+    def test_list_of_scalars_preserved(self):
+        assert sanitize_plugin_config({"teams": ["PHI", "NYG"]})["teams"] == ["PHI", "NYG"]
+
+    def test_list_of_dicts_recursed(self):
+        result = sanitize_plugin_config({"items": [{"ok": 1, "bad key": 2}]})
+        assert result["items"] == [{"ok": 1}]
+
+    def test_unknown_value_types_dropped(self):
+        assert sanitize_plugin_config({"weird": {1, 2, 3}, "good": 1}) == {"good": 1}
+
+    def test_none_values_dropped(self):
+        assert sanitize_plugin_config({"nothing": None, "good": 1}) == {"good": 1}
+
+    def test_strings_are_not_html_escaped(self):
+        # Pinned, not a bug: escaping here would persist the escaped form in
+        # config.json. Output escaping belongs to the template layer, which
+        # the function's docstring now says explicitly.
+        payload = "<script>alert(1)</script>"
+        assert sanitize_plugin_config({"title": payload})["title"] == payload
+
+    def test_empty_config(self):
+        assert sanitize_plugin_config({}) == {}

web_interface/blueprints/api_v3.py

@@ -328,7 +328,7 @@ def save_schedule_config():
         if not api_v3.config_manager:
             return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data:
             return jsonify({'status': 'error', 'message': 'No data provided'}), 400
 
@@ -536,7 +536,7 @@ def save_dim_schedule_config():
         if not api_v3.config_manager:
             return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data:
             return jsonify({'status': 'error', 'message': 'No data provided'}), 400
 
@@ -1345,7 +1345,7 @@ def save_raw_main_config():
         if not api_v3.config_manager:
             return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data:
             return jsonify({'status': 'error', 'message': 'No data provided'}), 400
 
@@ -1391,7 +1391,7 @@ def save_raw_secrets_config():
         if not api_v3.config_manager:
             return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data:
             return jsonify({'status': 'error', 'message': 'No data provided'}), 400
 
@@ -2411,7 +2411,7 @@ def get_on_demand_status():
 def start_on_demand_display():
     """Request the display controller to run a specific plugin on-demand."""
     try:
-        data = request.get_json() or {}
+        data = request.get_json(silent=True) or {}
         plugin_id = data.get('plugin_id')
         mode = data.get('mode')
         duration = data.get('duration')
@@ -2935,7 +2935,7 @@ def manage_plugin_limits(plugin_id):
                 })
         else:
             # POST - Set limits
-            data = request.get_json() or {}
+            data = request.get_json(silent=True) or {}
             from src.plugin_system.resource_monitor import ResourceLimits
 
             limits = ResourceLimits(
@@ -2966,7 +2966,7 @@ def toggle_plugin():
         content_type = request.content_type or ''
 
         if 'application/json' in content_type:
-            data = request.get_json()
+            data = request.get_json(silent=True)
             if not data or 'plugin_id' not in data or 'enabled' not in data:
                 return jsonify({'status': 'error', 'message': 'plugin_id and enabled required'}), 400
             plugin_id = data['plugin_id']
@@ -3837,7 +3837,7 @@ def install_plugin():
         if not api_v3.plugin_store_manager:
             return jsonify({'status': 'error', 'message': 'Plugin store manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data or 'plugin_id' not in data:
             return jsonify({'status': 'error', 'message': 'plugin_id required'}), 400
 
@@ -3971,7 +3971,7 @@ def install_plugin_from_url():
         if not api_v3.plugin_store_manager:
             return jsonify({'status': 'error', 'message': 'Plugin store manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data or 'repo_url' not in data:
             return jsonify({'status': 'error', 'message': 'repo_url required'}), 400
 
@@ -4026,7 +4026,7 @@ def get_registry_from_url():
         if not api_v3.plugin_store_manager:
             return jsonify({'status': 'error', 'message': 'Plugin store manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data or 'repo_url' not in data:
             return jsonify({'status': 'error', 'message': 'repo_url required'}), 400
 
@@ -4071,7 +4071,7 @@ def add_saved_repository():
         if not api_v3.saved_repositories_manager:
             return jsonify({'status': 'error', 'message': 'Saved repositories manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data or 'repo_url' not in data:
             return jsonify({'status': 'error', 'message': 'repo_url required'}), 400
 
@@ -4102,7 +4102,7 @@ def remove_saved_repository():
         if not api_v3.saved_repositories_manager:
             return jsonify({'status': 'error', 'message': 'Saved repositories manager not initialized'}), 500
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data or 'repo_url' not in data:
             return jsonify({'status': 'error', 'message': 'repo_url required'}), 400
 
@@ -4236,7 +4236,7 @@ def refresh_plugin_store():
         if not api_v3.plugin_store_manager:
             return jsonify({'status': 'error', 'message': 'Plugin store manager not initialized'}), 500
 
-        data = request.get_json() or {}
+        data = request.get_json(silent=True) or {}
         fetch_commit_info = data.get('fetch_commit_info', data.get('fetch_latest_versions', False))
 
         # Force refresh the registry
@@ -5822,7 +5822,7 @@ def reset_plugin_config():
         if not api_v3.config_manager:
             return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500
 
-        data = request.get_json() or {}
+        data = request.get_json(silent=True) or {}
         plugin_id = data.get('plugin_id')
         preserve_secrets = data.get('preserve_secrets', True)
 
@@ -6209,7 +6209,7 @@ sys.exit(proc.returncode)
 def authenticate_spotify():
     """Run Spotify authentication script"""
     try:
-        data = request.get_json() or {}
+        data = request.get_json(silent=True) or {}
         redirect_url = data.get('redirect_url', '').strip()
 
         # Get plugin directory
@@ -6272,7 +6272,6 @@ sys.exit(proc.returncode)
                     timeout=120,
                     env=env
                 )
-                os.unlink(wrapper_path)
 
                 if result.returncode == 0:
                     return jsonify({
@@ -6287,9 +6286,13 @@ sys.exit(proc.returncode)
                         'output': result.stdout + result.stderr
                     }), 400
             except subprocess.TimeoutExpired:
+                return jsonify({'status': 'error', 'message': 'Authentication timed out'}), 408
+            finally:
+                # The wrapper carries the user's redirect URL, so it must not
+                # survive the request on any path — including a failure to
+                # launch, which the previous per-branch unlinks missed.
                 if os.path.exists(wrapper_path):
                     os.unlink(wrapper_path)
-                return jsonify({'status': 'error', 'message': 'Authentication timed out'}), 408
         else:
             # Step 1: Get authorization URL
             # Import the script's functions directly to get the auth URL
@@ -6526,7 +6529,7 @@ def get_fonts_overrides():
 def save_fonts_overrides():
     """Save font overrides"""
     try:
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data:
             return jsonify({'status': 'error', 'message': 'No data provided'}), 400
 
@@ -7146,7 +7149,7 @@ def upload_of_the_day_json():
 def delete_of_the_day_json():
     """Delete a JSON file from of-the-day plugin"""
     try:
-        data = request.get_json() or {}
+        data = request.get_json(silent=True) or {}
         file_id = data.get('file_id')  # This is the category_name
 
         if not file_id:
@@ -7236,6 +7239,29 @@ def serve_plugin_static(plugin_id, file_path):
         return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500
 
 
+_MAX_CREDENTIAL_BACKUPS = 5
+
+
+def _prune_credential_backups(plugin_dir: Path) -> None:
+    """Keep only the newest _MAX_CREDENTIAL_BACKUPS credential backups.
+
+    Every re-upload copies the previous credentials.json aside. Without
+    pruning those accumulate for the life of the install — each one a
+    complete set of OAuth client credentials sitting in the plugin
+    directory.
+    """
+    backups = sorted(
+        plugin_dir.glob('credentials.json.backup.*'),
+        key=lambda p: p.stat().st_mtime,
+        reverse=True,
+    )
+    for stale in backups[_MAX_CREDENTIAL_BACKUPS:]:
+        try:
+            stale.unlink()
+        except OSError:
+            logger.warning("Could not remove old credential backup %s", stale.name)
+
+
 @api_v3.route('/plugins/calendar/upload-credentials', methods=['POST'])
 def upload_calendar_credentials():
     """Upload credentials.json file for calendar plugin"""
@@ -7267,20 +7293,25 @@ def upload_calendar_credentials():
         except json.JSONDecodeError:
             return jsonify({'status': 'error', 'message': 'File is not valid JSON'}), 400
 
-        # Validate it looks like Google OAuth credentials
+        # Validate it looks like Google OAuth credentials. The content
+        # already parsed as JSON above, so anything raising here means it is
+        # not credentials-shaped — a bare scalar, for instance, where the
+        # membership test raises TypeError. Reject rather than swallow: a
+        # file saved as credentials.json but not usable as credentials only
+        # fails later, somewhere less obvious.
         try:
             file.seek(0)
             creds_data = json.loads(file.read())
             file.seek(0)
-
-            # Check for required Google OAuth fields
-            if 'installed' not in creds_data and 'web' not in creds_data:
-                return jsonify({
-                    'status': 'error',
-                    'message': 'File does not appear to be a valid Google OAuth credentials file'
-                }), 400
+            is_oauth_shaped = 'installed' in creds_data or 'web' in creds_data
         except Exception:
-            pass  # Continue even if validation fails
+            is_oauth_shaped = False
+
+        if not is_oauth_shaped:
+            return jsonify({
+                'status': 'error',
+                'message': 'File does not appear to be a valid Google OAuth credentials file'
+            }), 400
 
         # Get plugin directory
         plugin_id = 'calendar'
@@ -7300,6 +7331,7 @@ def upload_calendar_credentials():
             backup_path = Path(plugin_dir) / f'credentials.json.backup.{int(time.time())}'
             import shutil
             shutil.copy2(credentials_path, backup_path)
+            _prune_credential_backups(Path(plugin_dir))
 
         # Save new file
         file.save(str(credentials_path))
@@ -7603,7 +7635,7 @@ def connect_wifi():
     try:
         from src.wifi_manager import WiFiManager
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data:
             return jsonify({
                 'status': 'error',
@@ -7757,7 +7789,7 @@ def set_auto_enable_ap_mode():
     try:
         from src.wifi_manager import WiFiManager
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if data is None or 'auto_enable_ap_mode' not in data:
             return jsonify({
                 'status': 'error',
@@ -7886,7 +7918,7 @@ def delete_cache_file():
             from src.cache_manager import CacheManager
             api_v3.cache_manager = CacheManager()
 
-        data = request.get_json()
+        data = request.get_json(silent=True)
         if not data or 'key' not in data:
             return jsonify({'status': 'error', 'message': 'cache key is required'}), 400
 
@@ -8149,7 +8181,16 @@ def backup_restore():
         try:
             opts_dict = json.loads(options_raw)
         except json.JSONDecodeError:
-            opts_dict = {}
+            opts_dict = None
+        if not isinstance(opts_dict, dict):
+            # Every option defaults to True, so falling back to {} on a
+            # parse failure would silently perform a FULL restore —
+            # secrets and all — for a caller who asked for a narrow one
+            # and mis-serialized it. Refuse instead of guessing.
+            return jsonify({
+                'status': 'error',
+                'message': 'Invalid options: expected a JSON object',
+            }), 400
         options = RestoreOptions(
             restore_config=bool(opts_dict.get('restore_config', True)),
             restore_secrets=bool(opts_dict.get('restore_secrets', True)),