diff --git a/src/plugin_system/state_reconciliation.py b/src/plugin_system/state_reconciliation.py index e967fbdf..f2b55100 100644 --- a/src/plugin_system/state_reconciliation.py +++ b/src/plugin_system/state_reconciliation.py @@ -56,6 +56,71 @@ class ReconciliationResult: message: str +def secrets_top_level_keys(config_manager) -> Set[str]: + """Top-level keys load_config() merges in from the secrets file. + + Deliberately fail-safe: an unreadable, absent, malformed or non-path + secrets location narrows this set rather than raising, because a failure + here must never break reconciliation. + """ + try: + path = config_manager.get_secrets_path() + with open(path, 'r') as f: + secrets = json.load(f) + except (AttributeError, OSError, TypeError, ValueError): + return set() + return set(secrets) if isinstance(secrets, dict) else set() + + +def ignored_config_keys(config_manager) -> Set[str]: + """Config keys that are not plugin ids: system keys plus secrets keys.""" + return set(StateReconciliation._SYSTEM_CONFIG_KEYS) | secrets_top_level_keys(config_manager) + + +def config_plugin_ids(config: Dict[str, Any], ignored_keys: Set[str]) -> Set[str]: + """Plugin ids a loaded config declares. + + Shared with the web interface so a stored verdict is re-checked against the + same definition that produced it. ``set(config)`` is NOT equivalent: it also + contains system keys, the secrets-file keys load_config() merges in, and + non-dict values. Counting any of those as a plugin is exactly what turned a + 'data' key in the secrets file into a phantom plugin, and using the loose + set to re-check findings would clear ones that are still true. + """ + return {k for k, v in (config or {}).items() + if isinstance(v, dict) and k not in ignored_keys} + + +def disk_plugin_ids(plugins_dir) -> Set[str]: + """Plugin ids actually installed on disk. + + A directory counts only when it is not a standalone backup and its + manifest.json parses. A corrupt manifest must not read as installed, or a + live "in config but not on disk" finding gets cleared on the strength of an + unreadable file. + """ + ids: Set[str] = set() + root = Path(plugins_dir) + try: + if not root.exists(): + return ids + for entry in root.iterdir(): + if not entry.is_dir() or '.standalone-backup-' in entry.name: + continue + manifest = entry / "manifest.json" + if not manifest.exists(): + continue + try: + with open(manifest, 'r') as f: + json.load(f) + except (OSError, ValueError): + continue + ids.add(entry.name) + except OSError: + return ids + return ids + + def still_unresolved(entries: List[Dict[str, Any]], config_keys: Set[str], installed_ids: Set[str]) -> List[Dict[str, Any]]: @@ -240,16 +305,7 @@ class StateReconciliation: 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() + return secrets_top_level_keys(self.config_manager) def _get_config_state(self) -> Dict[str, Dict[str, Any]]: """Get plugin state from config file.""" @@ -257,11 +313,8 @@ class StateReconciliation: 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 ignored: - continue + for plugin_id in config_plugin_ids(config, ignored): + plugin_config = config[plugin_id] state[plugin_id] = { 'enabled': plugin_config.get('enabled', True), 'version': plugin_config.get('version'), @@ -275,25 +328,21 @@ class StateReconciliation: """Get plugin state from disk (installed plugins).""" state = {} try: - if self.plugins_dir.exists(): - for plugin_dir in self.plugins_dir.iterdir(): - if plugin_dir.is_dir(): - plugin_id = plugin_dir.name - if '.standalone-backup-' in plugin_id: - continue - manifest_path = plugin_dir / "manifest.json" - if manifest_path.exists(): - import json - try: - with open(manifest_path, 'r') as f: - manifest = json.load(f) - state[plugin_id] = { - 'exists_on_disk': True, - 'version': manifest.get('version'), - 'name': manifest.get('name') - } - except Exception: # nosec B110 - corrupt/unreadable manifest; skip this plugin, outer except logs - pass + # Membership comes from the shared extractor so the web interface + # re-checks stored findings against this same definition; the + # manifest is then re-read here only for version/name. + for plugin_id in disk_plugin_ids(self.plugins_dir): + manifest_path = self.plugins_dir / plugin_id / "manifest.json" + try: + with open(manifest_path, 'r') as f: + manifest = json.load(f) + except (OSError, ValueError): # nosec B112 - raced or corrupt; skip + continue + state[plugin_id] = { + 'exists_on_disk': True, + 'version': manifest.get('version'), + 'name': manifest.get('name') + } except Exception as e: self.logger.warning(f"Error reading disk state: {e}") return state diff --git a/test/test_reconciliation_status_endpoint.py b/test/test_reconciliation_status_endpoint.py index 78a744aa..f6b77897 100644 --- a/test/test_reconciliation_status_endpoint.py +++ b/test/test_reconciliation_status_endpoint.py @@ -42,15 +42,29 @@ def client(tmp_path, monkeypatch): d.mkdir() (d / "manifest.json").write_text(json.dumps({"id": plugin_id}), encoding="utf-8") - def _setup(verdict, config=None, installed=()): + def _setup(verdict, config=None, installed=(), secrets=None, corrupt=()): (tmp_path / "ledmatrix_reconciliation.json").write_text( json.dumps(verdict), encoding="utf-8") for pid in installed: _install(pid) - api_v3.config_manager = MagicMock() - api_v3.config_manager.load_config.return_value = dict(config or {}) - api_v3.plugin_manager = MagicMock() - api_v3.plugin_manager.plugins_dir = str(plugins_dir) + for pid in corrupt: + d = plugins_dir / pid + d.mkdir(exist_ok=True) + (d / "manifest.json").write_text("{ this is not json", encoding="utf-8") + + secrets_path = tmp_path / "config_secrets.json" + secrets_path.write_text(json.dumps(secrets or {}), encoding="utf-8") + + cm = MagicMock() + cm.load_config.return_value = dict(config or {}) + cm.get_secrets_path.return_value = str(secrets_path) + pm = MagicMock() + pm.plugins_dir = str(plugins_dir) + # setattr via monkeypatch: these live on a module-level blueprint + # singleton, so assigning them directly leaks mocks -- pointing at a + # deleted tmp_path -- into every later test that imports api_v3. + monkeypatch.setattr(api_v3, "config_manager", cm, raising=False) + monkeypatch.setattr(api_v3, "plugin_manager", pm, raising=False) app = Flask(__name__) app.config["TESTING"] = True @@ -110,3 +124,44 @@ class TestTheEndpointStaysRobust: def test_a_run_still_in_progress_is_reported_as_such(self, client): c = client({"done": False, "unresolved": []}) assert _get(c)["done"] is False + + +class TestTheFilterUsesTheReconcilersOwnRules: + """A looser definition of "in config" or "on disk" clears findings that are + still true. Both cases raised by CodeRabbit on #553.""" + + def test_a_secrets_key_does_not_clear_an_in_config_finding(self, client): + # load_config() merges the secrets file in, so 'data' appears in the + # config dict -- but it is not a plugin. A plain set(config) would treat + # it as one and clear this finding. + c = client({"done": True, "unresolved": [ + {"plugin_id": "data", "type": IN_CONFIG}]}, + config={"data": {"mode": "nfl_recent"}}, + secrets={"data": {"mode": "nfl_recent"}}) + assert [e["plugin_id"] for e in _get(c)["unresolved"]] == ["data"] + + def test_a_system_key_does_not_clear_an_in_config_finding(self, client): + c = client({"done": True, "unresolved": [ + {"plugin_id": "display", "type": IN_CONFIG}]}, + config={"display": {"hardware": {}}}) + assert [e["plugin_id"] for e in _get(c)["unresolved"]] == ["display"] + + def test_a_non_dict_value_does_not_clear_an_in_config_finding(self, client): + c = client({"done": True, "unresolved": [ + {"plugin_id": "timezone", "type": IN_CONFIG}]}, + config={"timezone": "America/Chicago"}) + assert [e["plugin_id"] for e in _get(c)["unresolved"]] == ["timezone"] + + def test_an_unparseable_manifest_does_not_count_as_installed(self, client): + # Otherwise a corrupt file clears a live "in config but not on disk" + # finding on the strength of something nothing can read. + c = client({"done": True, "unresolved": [ + {"plugin_id": "broken-plugin", "type": ON_DISK}]}, + corrupt=["broken-plugin"]) + assert [e["plugin_id"] for e in _get(c)["unresolved"]] == ["broken-plugin"] + + def test_a_standalone_backup_dir_does_not_count_as_installed(self, client): + c = client({"done": True, "unresolved": [ + {"plugin_id": "weather.standalone-backup-20260101", "type": ON_DISK}]}, + installed=["weather.standalone-backup-20260101"]) + assert len(_get(c)["unresolved"]) == 1 diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index a2bfaaa4..b6bb9f11 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -715,21 +715,25 @@ def _drop_stale_reconciliation_findings(unresolved): indefinitely. Best-effort: any failure here returns the list untouched, because showing a stale warning beats failing the endpoint. - The disk set is enumerated directly rather than through - _installed_plugin_ids(): this mirrors StateReconciliation._get_disk_state(), - whose verdict we are re-checking, and avoids triggering a discovery scan on - an endpoint the overview page polls. + Both sets come from the reconciliation module's own extractors rather than + being re-derived here. That matters for correctness, not tidiness: a plain + set(load_config()) also contains system keys, the secrets-file keys merged + in by load_config(), and non-dict values, and any directory holding a + manifest.json would count as installed even if that manifest does not + parse. Either looseness clears findings that are still true -- and a secrets + key read as a plugin is the very bug the filter exists to stop reporting. """ try: - from src.plugin_system.state_reconciliation import still_unresolved + from src.plugin_system.state_reconciliation import ( + config_plugin_ids, disk_plugin_ids, ignored_config_keys, + still_unresolved, + ) - config_keys = set(api_v3.config_manager.load_config() or {}) - installed = set() + cm = api_v3.config_manager + config_keys = config_plugin_ids(cm.load_config() or {}, + ignored_config_keys(cm)) 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) + installed = disk_plugin_ids(plugins_dir) if plugins_dir else set() return still_unresolved(unresolved, config_keys, installed) except Exception: logger.debug("[Reconciliation] Could not re-check stored findings", exc_info=True)