From a6039f11ef48708e5cc5770c00c2288fab55aefb Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:59:33 -0400 Subject: [PATCH] fix(api-v3): re-check reconciliation findings by the reconciler's own rules Both CodeRabbit findings on the merge commit, verified against the code first. Major, plugins.py: the stale-findings filter derived its own notion of "in config" and "on disk", and both were looser than the reconciliation module's. set(load_config()) also contains system keys, the secrets-file keys load_config() merges in, and non-dict values; and any directory holding a manifest.json counted as installed even when that manifest does not parse. Either looseness clears a finding that is still true -- and a secrets key read as a plugin is the precise bug the filter exists to stop reporting, so reintroducing that asymmetry while re-checking was the wrong way round. The two extractions now live in state_reconciliation.py as config_plugin_ids() and disk_plugin_ids(), with ignored_config_keys() and secrets_top_level_keys() alongside. _get_config_state() and _get_disk_state() use them too, so there is one definition rather than two that can drift. _get_disk_state() re-reads each manifest for version/name after taking membership from the shared extractor; that costs one extra small read per plugin on a path that runs once per boot. Minor, the new test: the fixture assigned api_v3.config_manager and api_v3.plugin_manager directly. Those live on a module-level blueprint singleton, so the mocks leaked into every later test that imports api_v3 -- pointing at a tmp_path already deleted. Both now go through monkeypatch.setattr, which restores them. This is the same pollution class that made an earlier test in this session break seven unrelated ones, so it is worth getting right. Five cases added for the parity itself: a secrets key, a system key and a non-dict value must not clear an "installed but missing from config" finding, and neither an unparseable manifest nor a .standalone-backup- directory may count as installed. All five fail against the looser version. Linux CI on the preceding commit: Core unit tests, plugin harness, CodeQL and CodeRabbit all pass. Codacy reads action_required on every commit of this branch including the first, so it is pre-existing and not from this work. Co-Authored-By: Claude Opus 5 --- src/plugin_system/state_reconciliation.py | 117 ++++++++++++++------ test/test_reconciliation_status_endpoint.py | 65 ++++++++++- web_interface/blueprints/api_v3/plugins.py | 26 +++-- 3 files changed, 158 insertions(+), 50 deletions(-) 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)