fix(plugins): stop reconciliation inventing plugins and telling users to delete real config (#557)

On a device running four installed, configured, working plugins, the overview
banner read:

  Stale plugin config entries found: football-scoreboard, odds-ticker, data,
  ledmatrix-weather, starlark-apps. Remove them from config.json or reinstall
  via the Plugin Store.

Every claim in that sentence was wrong, and following its advice would have
deleted 4.9KB of working league settings. Four separate defects combined.

1. Secrets keys became phantom plugins. load_config() merges
   config_secrets.json into the config it returns, and the ignore list named
   only 'github' and 'youtube'. A 'data' key in that file therefore read as a
   plugin id and was reported as "in config but not on disk" forever. Read the
   secrets file's own top-level keys instead of hardcoding two of them.

2. The auto-fix clobbered real config. The handler for "on disk but not in
   config" assigned `config[plugin_id] = {'enabled': False}` unconditionally,
   so whenever detection was wrong it replaced a plugin's entire configuration
   with a stub. On the reported device it only failed to do so because the
   write hit EACCES. Now it refuses to overwrite an entry that already exists.

3. The banner gave backwards advice. plugin_missing_in_config ("on disk, not in
   config") and plugin_missing_on_disk ("in config, not on disk") are opposite
   problems, and both were rendered as "stale config entries ... remove them
   from config.json" -- which is correct for the second and destructive for the
   first. They are now reported separately, each with the advice that fits.

4. A stale verdict was served indefinitely. The result is a snapshot written
   once per run to a status file, and a run that fails to apply a fix also
   declares it will not retry. A condition that had since resolved kept being
   reported for hours. The status endpoint now re-checks stored findings
   against current state, dropping only what it can prove stale and keeping
   any kind it cannot re-verify.

The secrets-key lookup is deliberately fail-safe: an unreadable, absent,
malformed or non-path secrets location narrows the ignore set rather than
raising. An earlier revision let TypeError escape, which the broad handler in
_get_config_state() swallowed as "Error reading config state" -- emptying the
config state and making every downstream detection wrong. The existing
reconciliation tests caught it; there is now a regression test for it too.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

src/plugin_system/state_reconciliation.py

@@ -8,6 +8,7 @@ Detects and fixes inconsistencies between:
 - State manager state
 """
 
+import json
 from typing import Dict, Any, List, Set
 from dataclasses import dataclass
 from enum import Enum
@@ -55,6 +56,35 @@ class ReconciliationResult:
     message: str
 
 
+def still_unresolved(entries: List[Dict[str, Any]],
+                     config_keys: Set[str],
+                     installed_ids: Set[str]) -> List[Dict[str, Any]]:
+    """Drop stored reconciliation findings that are no longer true.
+
+    The verdict is written once to a status file and served to the web UI from
+    there, and a run that fails to apply a fix also declares it "will not retry
+    automatically". Together those froze a single moment forever: a device whose
+    plugins were all present in config kept being told, for hours, that four of
+    them were missing and should be removed from config.json.
+
+    Entry kinds this cannot re-check are kept, so filtering only ever removes
+    findings that are provably stale.
+    """
+    live: List[Dict[str, Any]] = []
+    for entry in entries:
+        kind = entry.get('type')
+        plugin_id = entry.get('plugin_id')
+        if kind == InconsistencyType.PLUGIN_MISSING_IN_CONFIG.value:
+            if plugin_id not in config_keys:
+                live.append(entry)
+        elif kind == InconsistencyType.PLUGIN_MISSING_ON_DISK.value:
+            if plugin_id not in installed_ids:
+                live.append(entry)
+        else:
+            live.append(entry)
+    return live
+
+
 class StateReconciliation:
     """
     State reconciliation system.
@@ -200,15 +230,37 @@ class StateReconciliation:
         'github', 'youtube',
     })
 
+    def _secrets_top_level_keys(self) -> Set[str]:
+        """Top-level keys that load_config() merges in from the secrets file.
+
+        load_config() merges config_secrets.json into the config it returns, so
+        those keys sit alongside plugin ids. _SYSTEM_CONFIG_KEYS named them
+        individually ('github', 'youtube'), which broke the moment anything else
+        was written there: a 'data' key became a phantom plugin, permanently
+        reported as "in config but not on disk". Reading the file keeps this
+        correct no matter what it holds.
+        """
+        try:
+            path = self.config_manager.get_secrets_path()
+            with open(path, 'r') as f:
+                secrets = json.load(f)
+        except (AttributeError, OSError, TypeError, ValueError):
+            # Deliberately broad: an unreadable, absent, malformed or
+            # non-path secrets location must narrow this set, never break
+            # reconciliation. ValueError covers json.JSONDecodeError.
+            return set()
+        return set(secrets) if isinstance(secrets, dict) else set()
+
     def _get_config_state(self) -> Dict[str, Dict[str, Any]]:
         """Get plugin state from config file."""
         state = {}
         try:
             config = self.config_manager.load_config()
+            ignored = self._SYSTEM_CONFIG_KEYS | self._secrets_top_level_keys()
             for plugin_id, plugin_config in config.items():
                 if not isinstance(plugin_config, dict):
                     continue
-                if plugin_id in self._SYSTEM_CONFIG_KEYS:
+                if plugin_id in ignored:
                     continue
                 state[plugin_id] = {
                     'enabled': plugin_config.get('enabled', True),
@@ -367,8 +419,18 @@ class StateReconciliation:
         """Attempt to fix an inconsistency."""
         try:
             if inconsistency.inconsistency_type == InconsistencyType.PLUGIN_MISSING_IN_CONFIG:
-                # Add plugin to config with default disabled state
                 config = self.config_manager.load_config()
+                if inconsistency.plugin_id in config:
+                    # Detection said "not in config" but it is there -- the
+                    # config changed under us, or the id came from a key merged
+                    # in from elsewhere. Assigning the stub below would replace
+                    # the real entry: one reported case would have traded 4.9KB
+                    # of league settings for {'enabled': False}. Nothing to fix.
+                    self.logger.info(
+                        "Skipped: %s is already in config; not overwriting it",
+                        inconsistency.plugin_id)
+                    return True
+                # Add plugin to config with default disabled state
                 config[inconsistency.plugin_id] = {
                     'enabled': False
                 }

test/test_reconciliation_phantom_plugins.py

@@ -0,0 +1,170 @@
+"""Tests for plugin-state reconciliation reporting phantom inconsistencies.
+
+Observed on a device running four installed, configured, working plugins: the
+web UI showed "Stale plugin config entries found: football-scoreboard,
+odds-ticker, data, ledmatrix-weather, starlark-apps. Remove them from
+config.json" for hours. Three separate defects combined to produce that:
+
+1. load_config() merges config_secrets.json into the config it returns, and the
+   ignore list named only 'github' and 'youtube'. A 'data' key in the secrets
+   file therefore became a phantom plugin, reported as in-config-not-on-disk.
+
+2. The auto-fix for "on disk but not in config" assigned
+   ``config[plugin_id] = {'enabled': False}`` unconditionally. When detection was
+   wrong, that traded a plugin's real configuration (4.9KB of league settings in
+   the reported case) for a stub. It only avoided firing there because the write
+   failed with EACCES.
+
+3. The verdict is a snapshot written once per run and served to the UI from a
+   status file, and a run that fails to apply a fix also declines to retry. So a
+   condition that had since resolved was reported indefinitely.
+"""
+
+import json
+from pathlib import Path
+from unittest.mock import Mock
+
+import pytest
+
+from src.plugin_system.state_reconciliation import (
+    InconsistencyType,
+    StateReconciliation,
+    still_unresolved,
+)
+
+
+class _ConfigManager:
+    """Minimal config manager with the secrets path the real one exposes."""
+
+    def __init__(self, config, secrets_path=None):
+        self._config = config
+        self._secrets_path = secrets_path
+        self.saved = []
+
+    def load_config(self):
+        return dict(self._config)
+
+    def save_config(self, config):
+        self.saved.append(config)
+        self._config = dict(config)
+
+    def get_secrets_path(self):
+        return self._secrets_path
+
+
+def _reconciler(config, plugins_dir, secrets_path=None):
+    return StateReconciliation(
+        state_manager=Mock(),
+        config_manager=_ConfigManager(config, secrets_path),
+        plugin_manager=Mock(),
+        plugins_dir=plugins_dir,
+    )
+
+
+class TestSecretsKeysAreNotPlugins:
+    def test_secrets_key_is_not_treated_as_a_plugin(self, tmp_path):
+        secrets = tmp_path / "config_secrets.json"
+        # The shape actually found on the device: display state written here.
+        secrets.write_text(json.dumps({"data": {"mode": "nfl_recent"},
+                                       "timestamp": 1.0}), encoding="utf-8")
+        config = {
+            "data": {"mode": "nfl_recent"},
+            "football-scoreboard": {"enabled": True, "nfl": {"favorite_teams": ["TB"]}},
+        }
+        state = _reconciler(config, tmp_path, secrets).\
+            _get_config_state()
+
+        assert "data" not in state, "secrets key leaked in as a phantom plugin"
+        assert "football-scoreboard" in state
+
+    def test_missing_secrets_file_is_survivable(self, tmp_path):
+        state = _reconciler({"odds-ticker": {"enabled": True}}, tmp_path,
+                            tmp_path / "does-not-exist.json")._get_config_state()
+        assert "odds-ticker" in state
+
+    def test_non_path_secrets_location_is_survivable(self, tmp_path):
+        """Regression: a non-path return raised TypeError out of the helper,
+        which the broad handler in _get_config_state() swallowed as "Error
+        reading config state" -- emptying the config state and making every
+        downstream detection wrong."""
+        r = _reconciler({"odds-ticker": {"enabled": True}}, tmp_path,
+                        secrets_path=Mock())
+        assert "odds-ticker" in r._get_config_state()
+
+    def test_config_manager_without_secrets_path_is_survivable(self, tmp_path):
+        r = StateReconciliation(state_manager=Mock(),
+                                config_manager=Mock(spec=["load_config", "save_config"]),
+                                plugin_manager=Mock(), plugins_dir=tmp_path)
+        r.config_manager.load_config.return_value = {"odds-ticker": {"enabled": True}}
+        assert "odds-ticker" in r._get_config_state()
+
+
+class TestFixNeverClobbersRealConfig:
+    def _missing_in_config(self, plugin_id):
+        inc = Mock()
+        inc.inconsistency_type = InconsistencyType.PLUGIN_MISSING_IN_CONFIG
+        inc.plugin_id = plugin_id
+        return inc
+
+    def test_existing_entry_is_not_overwritten(self, tmp_path):
+        rich = {"enabled": True, "nfl": {"favorite_teams": ["TB"], "show_odds": True}}
+        r = _reconciler({"football-scoreboard": dict(rich)}, tmp_path)
+
+        assert r._fix_inconsistency(self._missing_in_config("football-scoreboard")) is True
+        # The regression: this wrote {'enabled': False} over the real settings.
+        assert r.config_manager.saved == []
+        assert r.config_manager.load_config()["football-scoreboard"] == rich
+
+    def test_genuinely_absent_plugin_is_still_added(self, tmp_path):
+        r = _reconciler({"other-plugin": {"enabled": True}}, tmp_path)
+
+        assert r._fix_inconsistency(self._missing_in_config("new-plugin")) is True
+        assert len(r.config_manager.saved) == 1
+        assert r.config_manager.load_config()["new-plugin"] == {"enabled": False}
+        # and it must not disturb what was already there
+        assert r.config_manager.load_config()["other-plugin"] == {"enabled": True}
+
+
+class TestStaleFindingsAreDropped:
+    IN_CONFIG = InconsistencyType.PLUGIN_MISSING_IN_CONFIG.value
+    ON_DISK = InconsistencyType.PLUGIN_MISSING_ON_DISK.value
+
+    def test_resolved_missing_in_config_is_dropped(self):
+        entries = [{"plugin_id": "football-scoreboard", "type": self.IN_CONFIG}]
+        assert still_unresolved(entries, {"football-scoreboard"}, set()) == []
+
+    def test_genuinely_missing_in_config_is_kept(self):
+        entries = [{"plugin_id": "football-scoreboard", "type": self.IN_CONFIG}]
+        assert still_unresolved(entries, set(), set()) == entries
+
+    def test_resolved_missing_on_disk_is_dropped(self):
+        entries = [{"plugin_id": "odds-ticker", "type": self.ON_DISK}]
+        assert still_unresolved(entries, set(), {"odds-ticker"}) == []
+
+    def test_genuinely_missing_on_disk_is_kept(self):
+        entries = [{"plugin_id": "data", "type": self.ON_DISK}]
+        assert still_unresolved(entries, set(), {"odds-ticker"}) == entries
+
+    def test_unrecheckable_kinds_are_kept(self):
+        # Filtering must only ever remove what it can prove stale.
+        entries = [{"plugin_id": "x", "type": "plugin_version_mismatch"}]
+        assert still_unresolved(entries, set(), set()) == entries
+
+    def test_the_reported_device_state_clears_every_false_finding(self):
+        """The exact verdict the device served, against its real state."""
+        entries = [
+            {"plugin_id": "football-scoreboard", "type": self.IN_CONFIG},
+            {"plugin_id": "odds-ticker", "type": self.IN_CONFIG},
+            {"plugin_id": "data", "type": self.ON_DISK},
+            {"plugin_id": "ledmatrix-weather", "type": self.IN_CONFIG},
+            {"plugin_id": "starlark-apps", "type": self.IN_CONFIG},
+        ]
+        installed = {"football-scoreboard", "ledmatrix-weather", "odds-ticker",
+                     "starlark-apps", "web-ui-info"}
+        live = still_unresolved(entries, installed, installed)
+
+        # All four "installed but not in config" findings were false and clear.
+        # 'data' legitimately is not installed, so this filter keeps it; it stops
+        # being reported because _get_config_state() no longer invents it from the
+        # secrets file, which takes effect on the next reconciliation run.
+        assert [e["plugin_id"] for e in live] == ["data"]

web_interface/blueprints/api_v3.py

@@ -3464,6 +3464,30 @@ def reconcile_plugin_state():
             status_code=500
         )
 
+def _drop_stale_reconciliation_findings(unresolved):
+    """Re-check a stored reconciliation verdict against current state.
+
+    The verdict is a snapshot written once per run, and a run that could not
+    apply a fix also refuses to retry -- so a resolved condition was reported
+    indefinitely. Best-effort: any failure here returns the list untouched,
+    because showing a stale warning beats failing the endpoint.
+    """
+    try:
+        from src.plugin_system.state_reconciliation import still_unresolved
+
+        config_keys = set(api_v3.config_manager.load_config() or {})
+        installed = set()
+        plugins_dir = getattr(api_v3.plugin_manager, 'plugins_dir', None)
+        if plugins_dir:
+            for entry in Path(plugins_dir).iterdir():
+                if entry.is_dir() and (entry / 'manifest.json').exists():
+                    installed.add(entry.name)
+        return still_unresolved(unresolved, config_keys, installed)
+    except Exception:
+        logger.debug("[Reconciliation] Could not re-check stored findings", exc_info=True)
+        return unresolved
+
+
 @api_v3.route('/plugins/reconciliation-status', methods=['GET'])
 def get_reconciliation_status():
     """Return the result of the last startup reconciliation from /tmp status file."""
@@ -3478,6 +3502,8 @@ def get_reconciliation_status():
     try:
         with open(_recon_path) as _f:
             data = json.load(_f)
+        if data.get('unresolved'):
+            data['unresolved'] = _drop_stale_reconciliation_findings(data['unresolved'])
         return jsonify({'status': 'success', 'data': data})
     except json.JSONDecodeError:
         logger.exception("[Reconciliation] Failed to parse status file: %s", _recon_path)

web_interface/templates/v3/partials/overview.html

@@ -30,10 +30,33 @@
                 if (!d.unresolved || d.unresolved.length === 0) return;
                 var key = d.unresolved.map(function (i) { return i.plugin_id; }).sort().join(',');
                 if (sessionStorage.getItem(DISMISS_KEY) === key) return;
-                var ids = d.unresolved.map(function (i) { return i.plugin_id; }).join(', ');
-                document.getElementById('reconciliation-banner-text').textContent =
-                    'Stale plugin config entries found: ' + ids +
-                    '. Remove them from config.json or reinstall via the Plugin Store.';
+                // These are opposite problems and need opposite advice. Lumping
+                // them together told users to delete config.json entries for
+                // plugins that were installed and correctly configured.
+                function idsOfType(t) {
+                    return d.unresolved.filter(function (i) { return i.type === t; })
+                                       .map(function (i) { return i.plugin_id; });
+                }
+                var notConfigured = idsOfType('plugin_missing_in_config');
+                var notInstalled = idsOfType('plugin_missing_on_disk');
+                var other = d.unresolved.filter(function (i) {
+                    return i.type !== 'plugin_missing_in_config'
+                        && i.type !== 'plugin_missing_on_disk';
+                }).map(function (i) { return i.plugin_id; });
+
+                var parts = [];
+                if (notConfigured.length) {
+                    parts.push('Installed but missing from config: ' + notConfigured.join(', ')
+                        + '. Configure them in the Plugin Store - do not remove anything from config.json.');
+                }
+                if (notInstalled.length) {
+                    parts.push('In config but not installed: ' + notInstalled.join(', ')
+                        + '. Reinstall via the Plugin Store, or remove these entries from config.json.');
+                }
+                if (other.length) {
+                    parts.push('Needs attention: ' + other.join(', ') + '.');
+                }
+                document.getElementById('reconciliation-banner-text').textContent = parts.join(' ');
                 var banner = document.getElementById('reconciliation-banner');
                 banner.dataset.dismissKey = key;
                 banner.style.setProperty('display', 'flex', 'important');