fix(web): stop reporting "no update" when the update check could not run

check-update returned update_available=False whenever git failed. The banner
is the only route to the update button, so a checkout git refuses to touch
looked exactly like a current one -- permanently, with nothing on screen to
act on and only a log line recording why.

The common cause is an install performed as root. scripts/install/one-shot-install.sh
clones into ${HOME}/LEDMatrix, never consults SUDO_USER, and contains no chown
at all, while its own error text suggests running the whole thing under sudo.
The result is a root-owned checkout, and on a rig this is what every git
command in it does:

    fatal: detected dubious ownership in repository at '...'

including the fetch this endpoint runs. Verified on real hardware rather than
assumed.

A failed check now reports check_failed with a message the user can act on --
for dubious ownership, the chown that fixes it. The banner shows that message
instead of hiding itself, with the update button suppressed since updating
cannot work until the cause is fixed. The success path is untouched.

This does not fix the installer, which is the real cause; it stops the symptom
being invisible. The installer needs SUDO_USER handling and a chown, and its
suggestion to run as root should go.

Reverting the endpoint change fails four of the five new tests; the fifth
guards the success path and correctly does not move.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

src/plugin_system/resource_monitor.py

@@ -9,7 +9,7 @@ import time
 import logging
 import threading
 from typing import Dict, Optional, Any, Callable
-from dataclasses import dataclass, field, fields
+from dataclasses import dataclass, field
 
 try:
     import psutil
@@ -102,50 +102,6 @@ class PluginResourceMonitor:
                 "psutil not available - resource monitoring will be limited to execution time only"
             )
     
-    def _metrics_from_cache(self, plugin_id: str, cached: Any) -> "ResourceMetrics":
-        """Build metrics from a cached record, ignoring anything unrecognised.
-
-        ResourceMetrics(**cached) raises TypeError on a single unexpected key,
-        and that exception escapes into plugin_manager, which reports it as
-        "plugin <id> operation failed". Every plugin fails, and the plugin
-        system never finishes initialising.
-
-        Seen on a live rig: every plugin failing with
-
-            ResourceMetrics.__init__() got an unexpected keyword argument
-            'consecutive_failures'
-
-        which is a plugin_health field, not a metrics one. How a health-shaped
-        record came to sit under a plugin_metrics key on that machine is not
-        established -- a restored backup that mixed two machines' caches is the
-        likeliest explanation -- but the loader should not be brittle enough for
-        it to matter. plugin_health already repairs its records field by field
-        rather than trusting whatever is on disk; this does the same.
-
-        Unknown keys are dropped and named once, so a genuine schema change is
-        visible in the log instead of silently discarded.
-        """
-        if not isinstance(cached, dict):
-            self.logger.warning(
-                "Ignoring cached metrics for %s: expected a mapping, got %s",
-                plugin_id, type(cached).__name__)
-            return ResourceMetrics()
-
-        known = {f.name for f in fields(ResourceMetrics)}
-        unknown = sorted(set(cached) - known)
-        if unknown:
-            self.logger.warning(
-                "Dropping unrecognised field(s) from cached metrics for %s: %s",
-                plugin_id, ", ".join(unknown))
-        usable = {k: v for k, v in cached.items() if k in known}
-        try:
-            return ResourceMetrics(**usable)
-        except (TypeError, ValueError) as e:
-            self.logger.warning(
-                "Cached metrics for %s unusable (%s); starting fresh",
-                plugin_id, e)
-            return ResourceMetrics()
-
     def _get_metrics_key(self, plugin_id: str) -> str:
         """Get cache key for plugin metrics."""
         return f"plugin_metrics:{plugin_id}"
@@ -170,7 +126,7 @@ class PluginResourceMonitor:
                     cache_key, max_age=None, memory_ttl=0 if force_reload else None
                 )
                 if cached:
-                    metrics = self._metrics_from_cache(plugin_id, cached)
+                    metrics = ResourceMetrics(**cached)
                 else:
                     metrics = ResourceMetrics()
                 self._metrics[plugin_id] = metrics

test/test_metrics_cache_unknown_fields.py

@@ -1,100 +0,0 @@
-"""A malformed metrics cache entry must not take every plugin down with it.
-
-`ResourceMetrics(**cached)` raises TypeError on a single unexpected key, and
-that exception escapes into plugin_manager, which reports it per plugin as
-"plugin <id> operation failed". Every plugin fails and the plugin system never
-finishes initialising -- the health endpoint reports
-`plugin_system: not_initialized` while the display itself keeps running.
-
-Seen on a live rig, once per plugin, continuously:
-
-    ERROR - src.plugin_system.plugin_manager - plugin geochron operation failed:
-    ResourceMetrics.__init__() got an unexpected keyword argument
-    'consecutive_failures'
-
-`consecutive_failures` belongs to plugin_health, not to metrics. How a
-health-shaped record came to sit under a plugin_metrics key on that machine is
-not established -- a restored backup that mixed two machines' caches is the
-likeliest explanation, and the same rig had one restored onto it -- but a
-loader that turns one bad cache entry into a total outage is the part worth
-fixing. plugin_health already repairs its own records field by field rather
-than trusting what is on disk.
-"""
-import logging
-from dataclasses import fields
-from unittest.mock import MagicMock
-
-import pytest
-
-from src.plugin_system.resource_monitor import PluginResourceMonitor, ResourceMetrics
-
-
-class _Cache:
-    def __init__(self, payload=None):
-        self.payload = payload
-
-    def get(self, key, max_age=None, memory_ttl=None, **kwargs):
-        return self.payload
-
-    def set(self, key, data, ttl=None, **kwargs):
-        pass
-
-
-def _monitor(payload):
-    m = PluginResourceMonitor(cache_manager=_Cache(payload))
-    m.logger = logging.getLogger("test")
-    return m
-
-
-#: What the rig actually had under the metrics key.
-HEALTH_SHAPED = {
-    "consecutive_failures": 0, "circuit_state": "closed",
-    "circuit_opened_time": None, "half_open_start_time": None,
-    "last_error": None, "last_failure_time": None,
-    "last_success_time": 1_700_000_000.0, "total_failures": 0,
-    "total_successes": 42,
-}
-
-
-def test_a_health_record_under_the_metrics_key_does_not_raise():
-    """The exact failure: it must degrade, not take the plugin system down."""
-    monitor = _monitor(HEALTH_SHAPED)
-    metrics = monitor.get_metrics(" plugin-a".strip())
-    assert isinstance(metrics, ResourceMetrics)
-
-
-def test_recognised_fields_in_a_mixed_record_are_kept():
-    """Dropping the record wholesale would lose real history unnecessarily."""
-    mixed = dict(HEALTH_SHAPED, call_count=7, memory_mb=12.5)
-    metrics = _monitor(mixed).get_metrics("plugin-b")
-    assert metrics.call_count == 7
-    assert metrics.memory_mb == 12.5
-
-
-def test_a_clean_record_still_loads_unchanged():
-    clean = {f.name: 3 for f in fields(ResourceMetrics)}
-    metrics = _monitor(clean).get_metrics("plugin-c")
-    for name in (f.name for f in fields(ResourceMetrics)):
-        assert getattr(metrics, name) == 3
-
-
-def test_unknown_fields_are_named_in_the_log(caplog):
-    """Silently discarding them would hide a real schema change."""
-    with caplog.at_level(logging.WARNING):
-        _monitor(HEALTH_SHAPED).get_metrics("plugin-d")
-    # getMessage(), not .message: the latter is only populated once a handler
-    # formats the record, so the obvious spelling silently never matches.
-    assert any("consecutive_failures" in r.getMessage() for r in caplog.records), \
-        caplog.text
-
-
-@pytest.mark.parametrize("payload", ["a string", 42, ["a", "list"]])
-def test_a_non_mapping_cache_entry_does_not_raise(payload):
-    metrics = _monitor(payload).get_metrics("plugin-e")
-    assert isinstance(metrics, ResourceMetrics)
-
-
-def test_values_of_the_wrong_type_do_not_raise():
-    """A dataclass will accept these, but a later float() on them would not."""
-    metrics = _monitor({"call_count": "not a number"}).get_metrics("plugin-f")
-    assert isinstance(metrics, ResourceMetrics)

test/test_update_check_reports_failure.py

@@ -0,0 +1,85 @@
+"""A check that could not run must not be reported as "up to date".
+
+check-update returned update_available=False whenever git failed. The banner
+is the only route to the update button, so a checkout git refuses to touch
+looked exactly like a current one -- permanently, and with nothing for the
+user to act on. The usual cause is an install performed as root, after which
+every git command fails with "detected dubious ownership".
+"""
+import subprocess
+import sys
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+from flask import Flask
+
+sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
+
+from web_interface.blueprints import api_v3 as mod  # noqa: E402
+from web_interface.blueprints.api_v3 import api_v3  # noqa: E402
+
+DUBIOUS = ("fatal: detected dubious ownership in repository at "
+           "'/home/pi/LEDMatrix'\nTo add an exception for this directory, call:\n"
+           "\tgit config --global --add safe.directory /home/pi/LEDMatrix\n")
+
+
+@pytest.fixture
+def client():
+    app = Flask(__name__)
+    app.config['TESTING'] = True
+    app.register_blueprint(api_v3, url_prefix='/api/v3')
+    mod._update_check_cache['result'] = None
+    mod._update_check_cache['ts'] = 0
+    return app.test_client()
+
+
+def _fetch_fails(stderr: bytes):
+    def fake_run(args, **kwargs):
+        if args[:2] == ['git', 'fetch']:
+            return subprocess.CompletedProcess(args, 1, stdout=b'', stderr=stderr)
+        return subprocess.CompletedProcess(args, 0, stdout='', stderr='')
+    return fake_run
+
+
+class TestFailedCheckIsNotSilence:
+    def test_dubious_ownership_is_reported_not_swallowed(self, client):
+        with patch.object(mod.subprocess, 'run', _fetch_fails(DUBIOUS.encode())):
+            data = client.get('/api/v3/system/check-update').get_json()
+        assert data['check_failed'] is True, (
+            "a git failure was reported as a successful 'no update' check")
+        assert data['update_available'] is False
+
+    def test_the_message_tells_the_user_what_to_do(self, client):
+        with patch.object(mod.subprocess, 'run', _fetch_fails(DUBIOUS.encode())):
+            data = client.get('/api/v3/system/check-update').get_json()
+        assert 'chown' in data['error'], (
+            "dubious ownership is unactionable without the fix command")
+        assert 'root' in data['error']
+
+    def test_an_ordinary_git_failure_still_surfaces(self, client):
+        with patch.object(mod.subprocess, 'run',
+                          _fetch_fails(b'fatal: some other git problem\n')):
+            data = client.get('/api/v3/system/check-update').get_json()
+        assert data['check_failed'] is True
+        assert 'some other git problem' in data['error']
+
+    def test_offline_reads_as_offline(self, client):
+        with patch.object(mod.subprocess, 'run',
+                          _fetch_fails(b'fatal: could not resolve host: github.com\n')):
+            data = client.get('/api/v3/system/check-update').get_json()
+        assert 'Could not reach GitHub' in data['error']
+
+
+class TestSuccessPathUnchanged:
+    def test_up_to_date_carries_no_failure_flag(self, client):
+        def fake_run(args, **kwargs):
+            if args[:2] == ['git', 'fetch']:
+                return subprocess.CompletedProcess(args, 0, stdout=b'', stderr=b'')
+            if args[:2] == ['git', 'rev-parse']:
+                return subprocess.CompletedProcess(args, 0, stdout='abc123\n', stderr='')
+            return subprocess.CompletedProcess(args, 0, stdout='0\n', stderr='')
+        with patch.object(mod.subprocess, 'run', fake_run):
+            data = client.get('/api/v3/system/check-update').get_json()
+        assert data['update_available'] is False
+        assert not data.get('check_failed'), "a healthy check must not look like a failure"

web_interface/blueprints/api_v3.py

@@ -1821,6 +1821,33 @@ def get_system_version():
 _update_check_cache: Dict[str, Any] = {'result': None, 'ts': 0.0}
 _UPDATE_CHECK_TTL = 300  # 5 minutes — avoids a git fetch on every page load
 
+def _update_check_failed(detail: str) -> Dict[str, Any]:
+    """A check that could not run is not the same as being up to date.
+
+    Reporting update_available=False on a git failure hides the banner, and
+    the banner is the only route to the update button -- so a checkout git
+    refuses to touch looks exactly like a current one, permanently. The most
+    common cause is an install performed as root: git then reports "dubious
+    ownership" and every command fails, including the fetch here.
+    """
+    return {'update_available': False, 'remote_sha': 'unknown',
+            'commits_behind': 0, 'check_failed': True, 'error': detail}
+
+
+def _describe_git_failure(stderr: str) -> str:
+    """Turn git's stderr into something the user can act on."""
+    text = (stderr or '').strip()
+    if 'dubious ownership' in text or 'detected dubious ownership' in text:
+        return ("This checkout is owned by a different user than the one "
+                "running the web interface, so git refuses to use it. It is "
+                "usually the result of installing as root. Fix the ownership "
+                "and the update will work: sudo chown -R $USER:$USER "
+                + str(PROJECT_ROOT))
+    if 'could not resolve host' in text.lower() or 'network is unreachable' in text.lower():
+        return "Could not reach GitHub to check for updates."
+    return "Could not check for updates: " + (text.splitlines()[0] if text else "git failed")
+
+
 @api_v3.route('/system/check-update', methods=['GET'])
 def check_for_update():
     """Check whether a newer LEDMatrix commit is available on origin/main."""
@@ -1836,12 +1863,13 @@ def check_for_update():
             capture_output=True, timeout=10, cwd=cwd,
         )
         if fetch_result.returncode != 0:
+            stderr = fetch_result.stderr.decode(errors='replace').strip()
             logger.warning("check-update: git fetch failed (rc=%d): %s",
-                           fetch_result.returncode,
-                           fetch_result.stderr.decode(errors='replace').strip())
-            _update_check_cache['result'] = _safe
+                           fetch_result.returncode, stderr)
+            failed = _update_check_failed(_describe_git_failure(stderr))
+            _update_check_cache['result'] = failed
             _update_check_cache['ts'] = now
-            return jsonify(_safe)
+            return jsonify(failed)
         local = subprocess.run(
             ['git', 'rev-parse', 'HEAD'],
             capture_output=True, text=True, timeout=5, cwd=cwd,
@@ -1869,7 +1897,8 @@ def check_for_update():
         return jsonify(result)
     except Exception as e:
         logger.warning("check-update failed: %s", e)
-        return jsonify(_safe)
+        return jsonify(_update_check_failed(
+            "Could not check for updates; see logs for details."))
 
 @api_v3.route('/system/action', methods=['POST'])
 def execute_system_action():

web_interface/templates/v3/base.html

@@ -1107,15 +1107,29 @@
         fetch('/api/v3/system/check-update')
             .then(function(r) { return r.json(); })
             .then(function(data) {
+                var banner = document.getElementById('update-banner');
+                var btn = document.getElementById('update-banner-btn');
+                if (data.check_failed) {
+                    // A check that could not run is not the same as being up
+                    // to date. Hiding the banner here made a checkout git
+                    // refuses to touch look permanently current, with no
+                    // route to the update button and nothing to act on.
+                    document.getElementById('update-banner-text').textContent =
+                        data.error || 'Could not check for updates.';
+                    if (btn) btn.style.display = 'none';
+                    banner.style.display = '';
+                    return;
+                }
+                if (btn) btn.style.display = '';
                 if (data.update_available && getDismissedSha() !== data.remote_sha) {
                     var n = data.commits_behind || 0;
                     var msg = 'A new LEDMatrix update is available';
                     if (n > 0) msg += ' (' + n + ' commit' + (n > 1 ? 's' : '') + ')';
                     document.getElementById('update-banner-text').textContent = msg;
-                    document.getElementById('update-banner').style.display = '';
+                    banner.style.display = '';
                     try { sessionStorage.setItem('update-sha', data.remote_sha); } catch(e) {}
                 } else {
-                    document.getElementById('update-banner').style.display = 'none';
+                    banner.style.display = 'none';
                 }
             })
             .catch(function() {});