diff --git a/src/config_manager.py b/src/config_manager.py index 453e73b1..f544f35c 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -29,6 +29,7 @@ import os import logging from pathlib import Path from typing import Dict, Any, Optional, List +from src.core_config_keys import CORE_CONFIG_KEYS, CORE_SECRETS_KEYS from src.exceptions import ConfigError from src.logging_config import get_logger from src.config_manager_atomic import ( @@ -756,12 +757,13 @@ class ConfigManager: valid_set = set(valid_plugin_ids) - # Find orphaned plugins in main config - main_plugins = set(main_config.keys()) + # Find orphaned plugins in main config. Core sections (display, + # schedule, auto_update, ...) are not plugins and never orphans. + main_plugins = set(main_config.keys()) - CORE_CONFIG_KEYS orphaned_main = main_plugins - valid_set # Find orphaned plugins in secrets config - secrets_plugins = set(secrets_config.keys()) + secrets_plugins = set(secrets_config.keys()) - CORE_CONFIG_KEYS - CORE_SECRETS_KEYS orphaned_secrets = secrets_plugins - valid_set all_orphaned = orphaned_main | orphaned_secrets @@ -815,8 +817,8 @@ class ConfigManager: if not isinstance(plugin_config, dict): continue - # Skip non-plugin config sections - if plugin_id in ['display', 'schedule', 'timezone', 'plugin_system']: + # Skip core config sections + if plugin_id in CORE_CONFIG_KEYS: continue schema = plugin_schema_manager.load_schema(plugin_id, use_cache=True) diff --git a/src/core_config_keys.py b/src/core_config_keys.py index d68e785f..8d8a07fa 100644 --- a/src/core_config_keys.py +++ b/src/core_config_keys.py @@ -41,3 +41,13 @@ CORE_CONFIG_KEYS = frozenset({ 'vegas_excluded_plugins', 'vegas_scroll_enabled', }) + +#: Top-level keys of ``config_secrets.json`` that belong to the core rather than +#: to a plugin: the GitHub token the Plugin Store reads, and the historical +#: ``youtube`` section. Plugin secrets are namespaced by plugin id, so anything +#: deciding whether a secrets section is a plugin's needs this as well as +#: ``CORE_CONFIG_KEYS``. +CORE_SECRETS_KEYS = frozenset({ + 'github', + 'youtube', +}) diff --git a/src/plugin_system/schema_manager.py b/src/plugin_system/schema_manager.py index b56e6126..2be222ef 100644 --- a/src/plugin_system/schema_manager.py +++ b/src/plugin_system/schema_manager.py @@ -13,6 +13,8 @@ from typing import Any, Dict, List, Optional, Tuple import jsonschema from jsonschema import Draft7Validator, ValidationError +from src.core_config_keys import CORE_CONFIG_KEYS + def _renders_as_object(prop: Dict[str, Any]) -> bool: """``field_type == 'object'`` as ``plugin_config.html`` computes it. @@ -698,10 +700,11 @@ class SchemaManager: """ collisions = [] - # Reserved top-level config keys that plugins should not use as IDs - reserved_keys = { - 'display', 'schedule', 'timezone', 'plugin_system', - 'display_modes', 'system', 'hardware', 'debug', + # Reserved top-level config keys that plugins should not use as IDs: + # every core section (src/core_config_keys.py), plus a few names that + # read as core even though no current section uses them. + reserved_keys = set(CORE_CONFIG_KEYS) | { + 'display_modes', 'hardware', 'debug', 'log_level', 'emulator', 'web_interface' } diff --git a/src/startup_validator.py b/src/startup_validator.py index 7bf15a11..954c751f 100644 --- a/src/startup_validator.py +++ b/src/startup_validator.py @@ -8,6 +8,7 @@ Fails fast with clear error messages to prevent runtime issues. import os from typing import Any, List, Optional, Tuple from pathlib import Path +from src.core_config_keys import CORE_CONFIG_KEYS from src.exceptions import ConfigError, PluginError, CacheError from src.logging_config import get_logger @@ -275,8 +276,9 @@ class StartupValidator: # Check for enabled plugins that don't exist for plugin_id, plugin_config in config.items(): - # Skip non-plugin config sections - if plugin_id in ['display', 'schedule', 'timezone', 'plugin_system']: + # Skip core sections: auto_update and dim_schedule have an + # 'enabled' key too, and are not plugins that went missing. + if plugin_id in CORE_CONFIG_KEYS: continue if not isinstance(plugin_config, dict): diff --git a/test/test_core_config_key_adopters.py b/test/test_core_config_key_adopters.py new file mode 100644 index 00000000..c93e3b0d --- /dev/null +++ b/test/test_core_config_key_adopters.py @@ -0,0 +1,108 @@ +"""Every "is this top-level key a plugin?" decision uses src/core_config_keys.py. + +#589 moved plugin-state reconciliation onto the shared list, but three private +copies stayed behind and did not know about auto_update, dim_schedule, sync, +location, target_fps, ...: + +- StartupValidator warned "Plugin 'auto_update' is enabled but not found in + plugins directory" on every display start with auto-update or a dim + schedule switched on; +- SchemaManager.detect_config_key_collisions let a plugin id collide with + those sections silently; +- ConfigManager.cleanup_orphaned_plugin_configs deleted display, schedule, + auto_update, ... as orphans, and validate_all_plugin_configs validated core + sections as plugins. +""" + +import json +import re +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from src.config_manager import ConfigManager +from src.core_config_keys import CORE_CONFIG_KEYS, CORE_SECRETS_KEYS +from src.plugin_system.schema_manager import SchemaManager +from src.startup_validator import StartupValidator + +REPO = Path(__file__).resolve().parent.parent + + +class TestStartupValidator: + def test_enabled_core_sections_are_not_missing_plugins(self): + config = { + 'display': {'hardware': {}}, + 'auto_update': {'enabled': True}, + 'dim_schedule': {'enabled': True}, + 'schedule': {'enabled': True}, + 'gone-plugin': {'enabled': True}, + } + cm = MagicMock() + cm.get_config.return_value = config + pm = MagicMock() + pm.discover_plugins.return_value = [] + validator = StartupValidator(cm, plugin_manager=pm, cache_manager=MagicMock()) + validator.warnings = [] + validator.errors = [] + validator._validate_plugins() + missing = [w for w in validator.warnings if 'not found in plugins directory' in w] + assert missing == ["Plugin 'gone-plugin' is enabled but not found in plugins directory"] + + +class TestReservedPluginIds: + @pytest.mark.parametrize('key', sorted(CORE_CONFIG_KEYS)) + def test_every_core_key_is_reserved(self, key): + collisions = SchemaManager().detect_config_key_collisions([key]) + assert [c['type'] for c in collisions] == ['reserved_key_collision'] + + def test_an_ordinary_plugin_id_is_not(self): + assert SchemaManager().detect_config_key_collisions(['clock-simple']) == [] + + +class TestConfigManagerHelpers: + @pytest.fixture + def manager(self, tmp_path): + config = {key: {'enabled': True} for key in ('display', 'schedule', 'auto_update', + 'dim_schedule', 'sync', 'location')} + config.update({'timezone': 'UTC', 'installed': {'enabled': True}, + 'orphan': {'enabled': True}}) + secrets = {'github': {'token': 't'}, 'youtube': {'api_key': 'k'}, + 'installed': {'api_key': 'a'}, 'orphan': {'api_key': 'o'}} + (tmp_path / 'config.json').write_text(json.dumps(config)) + (tmp_path / 'secrets.json').write_text(json.dumps(secrets)) + manager = ConfigManager(config_path=str(tmp_path / 'config.json'), + secrets_path=str(tmp_path / 'secrets.json')) + manager.template_path = str(tmp_path / 'no-template.json') + return manager + + def test_cleanup_removes_only_real_orphans(self, manager, tmp_path): + removed = manager.cleanup_orphaned_plugin_configs(['installed']) + assert removed == ['orphan'] + config = json.loads((tmp_path / 'config.json').read_text()) + assert {'display', 'schedule', 'auto_update', 'dim_schedule', 'sync', + 'location', 'timezone', 'installed'} == set(config) + secrets = json.loads((tmp_path / 'secrets.json').read_text()) + assert set(secrets) == {'github', 'youtube', 'installed'} + + def test_validate_all_skips_core_sections(self, manager): + schema_manager = MagicMock() + schema_manager.load_schema.return_value = None + results = manager.validate_all_plugin_configs(schema_manager) + assert set(results) == {'installed', 'orphan'} + + +def test_core_secrets_keys_are_not_core_config_keys(): + assert not (CORE_SECRETS_KEYS & CORE_CONFIG_KEYS) + + +@pytest.mark.parametrize('relpath', [ + 'src/startup_validator.py', + 'src/config_manager.py', + 'src/plugin_system/schema_manager.py', +]) +def test_no_private_copy_of_the_list_remains(relpath): + """The old copies all started with this literal; a new copy likely would too.""" + source = (REPO / relpath).read_text(encoding='utf-8') + assert not re.search(r"""\[\s*['"]display['"]\s*,\s*['"]schedule['"]""", source), \ + f'{relpath} has a private core-key list again; use src/core_config_keys.py'