fix(plugins): normalize legacy boolean settings before schema validation (#588)

The news plugin's schema turned global.dynamic_duration from a boolean
into an {enabled, min_duration_seconds, ...} object. Installs that have
not saved the news settings since still hold `true`, so every start
logged "Plugin news config does not match its schema (loading anyway):
Field 'global.dynamic_duration': Expected type object, got bool" and
flagged news degraded.

The settings form already reads such a boolean as {"enabled": <bool>}
(render_nested_section in plugin_config.html) and the next save writes
the object. The loader did not. It now applies the same rule before
merging schema defaults and validating, so the defaults fill in the rest
of the object and the plugin receives it in the new shape.

The rule lives in schema_manager.legacy_bool_as_object /
normalize_legacy_booleans. It applies at any depth of nested objects
but not inside arrays, matching the form, and only to a real bool under
an object-typed property with an `enabled` child. Every other mismatch
still warns. A parity test renders the template macro against the helper
so the two cannot drift.

Nothing is written to config.json at load: the normalization is in
memory, and the next save of the plugin's settings persists the object.

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

CHANGELOG.md

@@ -55,6 +55,17 @@ Web interface:
   re-sent with backoff instead of being counted as failed and skipped — that is
   how a disabled plugin with an update waiting was silently left out.
 
+Plugin system:
+
+- A plugin no longer starts with a schema warning and a degraded flag because
+  config.json still holds a boolean where its schema now has an object with an
+  `enabled` property (news' `global.dynamic_duration: true`). The loader reads
+  the boolean as `{"enabled": <bool>}` before merging schema defaults and
+  validating, the same rule the settings form already applies
+  (`legacy_bool_as_object` in `src/plugin_system/schema_manager.py`). Nothing
+  is written at load; the next save of that plugin's settings stores the object.
+  Other type mismatches still warn.
+
 ## 3.4.0
 
 Plugin-facing changes since 3.3.0 (tag `v3.3.1`) not covered further down:

src/plugin_system/plugin_manager.py

@@ -22,7 +22,7 @@ from src.logging_config import get_logger
 from src.plugin_system.plugin_loader import PluginLoader
 from src.plugin_system.plugin_executor import PluginExecutor
 from src.plugin_system.plugin_state import PluginStateManager, PluginState
-from src.plugin_system.schema_manager import SchemaManager
+from src.plugin_system.schema_manager import SchemaManager, normalize_legacy_booleans
 from src.common.permission_utils import (
     ensure_directory_permissions,
     get_plugin_dir_mode
@@ -351,6 +351,7 @@ class PluginManager:
                 config = {}
             
             # Check if plugin has a config schema
+            schema = None
             schema_path = self.schema_manager.get_schema_path(plugin_id)
             if schema_path is None:
                 # Schema file doesn't exist
@@ -368,6 +369,24 @@ class PluginManager:
                         f"The schema may be invalid. Please verify the schema file at: {schema_path}"
                     )
 
+            # A plugin that turned an on/off boolean into an {enabled, ...}
+            # object still finds the boolean in config.json until the user saves
+            # its settings form, which carries it over (plugin_config.html).
+            # Read it the same way here, before the defaults fill in the rest
+            # of the object and before schema validation, so the plugin doesn't
+            # start with a schema warning and a degraded flag. In memory only:
+            # config.json is written by saves, never by loading a plugin.
+            if schema:
+                upgraded: List[str] = []
+                config = normalize_legacy_booleans(config, schema, upgraded)
+                if upgraded:
+                    self.logger.info(
+                        "Plugin %s: reading legacy boolean setting %s as "
+                        "{\"enabled\": ...}; saving the plugin's settings "
+                        "stores the new shape",
+                        plugin_id, ", ".join(upgraded),
+                    )
+
             # Merge config with schema defaults to ensure all defaults are applied
             try:
                 defaults = self.schema_manager.generate_default_config(plugin_id, use_cache=True)

src/plugin_system/schema_manager.py

@@ -14,6 +14,85 @@ import jsonschema
 from jsonschema import Draft7Validator, ValidationError
 
 
+def _renders_as_object(prop: Dict[str, Any]) -> bool:
+    """``field_type == 'object'`` as ``plugin_config.html`` computes it.
+
+    The template takes a type list's *first* entry, so ``["object", "null"]``
+    is an object and ``["boolean", "object"]`` is a checkbox.
+    """
+    field_type = prop.get('type')
+    if isinstance(field_type, list):
+        field_type = field_type[0] if field_type else None
+    return field_type == 'object'
+
+
+def legacy_bool_as_object(value: Any, prop: Any) -> Any:
+    """Read a boolean stored where the schema now has an ``{enabled, ...}`` object.
+
+    Plugins turn an on/off switch into a settings object (news'
+    ``global.dynamic_duration: true`` became ``{enabled, min_duration_seconds,
+    ...}``), but config.json keeps the boolean until the user next saves that
+    plugin's form. The boolean was the switch the object's ``enabled`` now
+    holds, so it becomes ``{"enabled": value}`` and schema defaults fill the rest.
+
+    Returns ``value`` itself when the rule does not apply: anything that is not
+    a real ``bool`` (``1``, ``"true"``, ``None``) or a schema property that is not
+    an object with an ``enabled`` child. Those stay as they are, so validation
+    still reports a genuine mismatch.
+
+    This is the rule ``render_nested_section`` in
+    ``web_interface/templates/v3/partials/plugin_config.html`` applies when it
+    draws the form. ``test/test_legacy_boolean_config.py`` renders that macro
+    against this function, so change both together.
+    """
+    if not isinstance(value, bool) or not isinstance(prop, dict):
+        return value
+    properties = prop.get('properties')
+    if (_renders_as_object(prop) and isinstance(properties, dict)
+            and 'enabled' in properties):
+        return {'enabled': value}
+    return value
+
+
+def normalize_legacy_booleans(config: Any, schema: Any,
+                              changed_paths: Optional[List[str]] = None,
+                              _prefix: str = '') -> Any:
+    """Apply :func:`legacy_bool_as_object` at every depth of a plugin config.
+
+    Walks the config along the schema's ``properties`` the way the settings
+    form does: into nested objects, not into array items (the form hands arrays
+    to widgets and never applies the rule there).
+
+    Never mutates ``config``. Returns the same object when nothing changed, and
+    otherwise copies only the dicts on the path to each upgraded value. When
+    ``changed_paths`` is given, the dotted path of each upgraded value is
+    appended to it.
+    """
+    if not isinstance(config, dict) or not isinstance(schema, dict):
+        return config
+    properties = schema.get('properties')
+    if not isinstance(properties, dict):
+        return config
+
+    result = config
+    for key, value in config.items():
+        prop = properties.get(key)
+        if not isinstance(prop, dict) or not _renders_as_object(prop):
+            continue
+        path = f"{_prefix}.{key}" if _prefix else key
+        new_value = legacy_bool_as_object(value, prop)
+        if new_value is not value:
+            if changed_paths is not None:
+                changed_paths.append(path)
+        elif isinstance(value, dict):
+            new_value = normalize_legacy_booleans(value, prop, changed_paths, path)
+        if new_value is not value:
+            if result is config:
+                result = dict(config)
+            result[key] = new_value
+    return result
+
+
 class SchemaManager:
     """
     Manages plugin configuration schemas with caching and validation.

test/test_legacy_boolean_config.py

@@ -0,0 +1,394 @@
+"""A boolean the schema has since turned into an ``{enabled, ...}`` object.
+
+The news plugin's ``global.dynamic_duration`` went from ``true`` to an object
+with ``enabled``, ``min_duration_seconds``, ... An install that has not saved
+the news settings since still holds ``true``, and on every start the display
+service logged::
+
+    Plugin news config does not match its schema (loading anyway):
+    Field 'global.dynamic_duration': Expected type object, got bool
+
+and flagged news degraded. The settings form already reads such a boolean as
+``{"enabled": <bool>}`` (``render_nested_section`` in plugin_config.html); the
+loader did not. Both now follow ``legacy_bool_as_object``, and the parity tests
+below render the real macro against it so the two cannot drift.
+"""
+
+import json
+import sys
+from html.parser import HTMLParser
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+PROJECT_ROOT = Path(__file__).parent.parent
+if str(PROJECT_ROOT) not in sys.path:
+    sys.path.insert(0, str(PROJECT_ROOT))
+
+from src.plugin_system.plugin_manager import PluginManager
+from src.plugin_system.schema_manager import (
+    legacy_bool_as_object,
+    normalize_legacy_booleans,
+)
+
+
+DYNAMIC_DURATION = {
+    "type": "object",
+    "properties": {
+        "enabled": {"type": "boolean", "default": True},
+        "min_duration_seconds": {"type": "integer", "default": 30,
+                                 "minimum": 10, "maximum": 300},
+        "max_duration_seconds": {"type": "integer", "default": 300,
+                                 "minimum": 30, "maximum": 600},
+        "buffer_ratio": {"type": "number", "default": 0.1,
+                         "minimum": 0.01, "maximum": 1.0},
+    },
+    "additionalProperties": False,
+}
+
+# The shape of news' schema, cut down to what the tests need.
+NEWS_SCHEMA = {
+    "type": "object",
+    "properties": {
+        "enabled": {"type": "boolean", "default": True},
+        "global": {
+            "type": "object",
+            "properties": {
+                "font_size": {"type": "integer", "default": 12},
+                "dynamic_duration": DYNAMIC_DURATION,
+                "display": {
+                    "type": "object",
+                    "properties": {
+                        "scroll_speed": {"type": "number", "default": 1.0},
+                    },
+                    "additionalProperties": False,
+                },
+                "headline_paging": {
+                    "type": "object",
+                    "properties": {
+                        "enabled": {"type": "boolean", "default": False},
+                        "page_hold_seconds": {"type": "number", "default": 2.0},
+                    },
+                    "additionalProperties": False,
+                },
+            },
+            "additionalProperties": False,
+        },
+        "feeds": {
+            "type": "object",
+            "properties": {
+                "custom_feeds": {
+                    "type": "array",
+                    "items": {
+                        "type": "object",
+                        "properties": {
+                            "name": {"type": "string"},
+                            "enabled": {"type": "boolean", "default": True},
+                            "schedule": {
+                                "type": "object",
+                                "properties": {"enabled": {"type": "boolean"}},
+                            },
+                        },
+                    },
+                },
+            },
+        },
+    },
+    "additionalProperties": False,
+}
+
+
+# --------------------------------------------------------------------------
+# The helper
+# --------------------------------------------------------------------------
+
+class TestLegacyBoolAsObject:
+    def test_true_under_an_enabled_object_becomes_its_switch(self):
+        assert legacy_bool_as_object(True, DYNAMIC_DURATION) == {"enabled": True}
+
+    def test_false_stays_off(self):
+        """Upgrading must never turn a feature the user switched off back on."""
+        assert legacy_bool_as_object(False, DYNAMIC_DURATION) == {"enabled": False}
+
+    def test_nullable_object_type_list_counts_as_object(self):
+        prop = dict(DYNAMIC_DURATION, type=["object", "null"])
+        assert legacy_bool_as_object(True, prop) == {"enabled": True}
+
+    @pytest.mark.parametrize("value", [1, 0, "true", "false", None, [], 2.5])
+    def test_a_value_that_is_not_a_real_bool_is_untouched(self, value):
+        assert legacy_bool_as_object(value, DYNAMIC_DURATION) is value
+
+    def test_an_object_without_enabled_is_untouched(self):
+        prop = {"type": "object", "properties": {"speed": {"type": "number"}}}
+        assert legacy_bool_as_object(True, prop) is True
+
+    def test_a_boolean_or_object_union_is_untouched(self):
+        """The form draws ``["boolean", "object"]`` as a checkbox, and the
+        schema accepts the boolean, so there is nothing to upgrade."""
+        prop = dict(DYNAMIC_DURATION, type=["boolean", "object"])
+        assert legacy_bool_as_object(True, prop) is True
+
+    def test_a_non_object_schema_is_untouched(self):
+        assert legacy_bool_as_object(True, {"type": "boolean"}) is True
+        assert legacy_bool_as_object(True, None) is True
+
+
+class TestNormalizeLegacyBooleans:
+    def test_top_level(self):
+        schema = {"type": "object",
+                  "properties": {"dynamic_duration": DYNAMIC_DURATION}}
+        out = normalize_legacy_booleans({"dynamic_duration": True}, schema)
+        assert out == {"dynamic_duration": {"enabled": True}}
+
+    def test_nested(self):
+        cfg = {"global": {"font_size": 12, "dynamic_duration": True}}
+        out = normalize_legacy_booleans(cfg, NEWS_SCHEMA)
+        assert out == {"global": {"font_size": 12,
+                                  "dynamic_duration": {"enabled": True}}}
+
+    def test_several_depths_report_their_paths(self):
+        schema = {"type": "object", "properties": {
+            "dynamic_duration": DYNAMIC_DURATION,
+            "outer": {"type": "object", "properties": {
+                "inner": {"type": "object", "properties": {
+                    "mode": DYNAMIC_DURATION}}}},
+        }}
+        cfg = {"dynamic_duration": False, "outer": {"inner": {"mode": True}}}
+        changed = []
+        out = normalize_legacy_booleans(cfg, schema, changed)
+        assert out == {"dynamic_duration": {"enabled": False},
+                       "outer": {"inner": {"mode": {"enabled": True}}}}
+        assert changed == ["dynamic_duration", "outer.inner.mode"]
+
+    def test_array_items_are_left_alone_as_the_form_does(self):
+        """The form hands arrays to widgets and never upgrades inside them."""
+        cfg = {"feeds": {"custom_feeds": [{"name": "a", "schedule": True}]}}
+        out = normalize_legacy_booleans(cfg, NEWS_SCHEMA)
+        assert out is cfg
+
+    def test_already_an_object_is_untouched(self):
+        cfg = {"global": {"dynamic_duration": {"enabled": False,
+                                               "min_duration_seconds": 45}}}
+        assert normalize_legacy_booleans(cfg, NEWS_SCHEMA) is cfg
+
+    def test_other_mismatches_are_untouched(self):
+        cfg = {"global": {"font_size": True, "display": True,
+                          "dynamic_duration": "yes"}}
+        changed = []
+        assert normalize_legacy_booleans(cfg, NEWS_SCHEMA, changed) is cfg
+        assert changed == []
+
+    def test_keys_the_schema_does_not_declare_are_untouched(self):
+        cfg = {"global": {"mystery": True}, "vegas_width_pct": 50}
+        assert normalize_legacy_booleans(cfg, NEWS_SCHEMA) is cfg
+
+    def test_the_stored_config_is_not_mutated(self):
+        """load_config() may hand back a cached dict; it must stay as stored."""
+        cfg = {"enabled": True, "global": {"dynamic_duration": True}}
+        before = json.loads(json.dumps(cfg))
+        out = normalize_legacy_booleans(cfg, NEWS_SCHEMA)
+        assert cfg == before
+        assert out is not cfg and out["global"] is not cfg["global"]
+
+    @pytest.mark.parametrize("config,schema", [
+        (None, NEWS_SCHEMA), ([], NEWS_SCHEMA), ({"a": True}, None),
+        ({"a": True}, {"type": "object"}),
+    ])
+    def test_tolerates_odd_inputs(self, config, schema):
+        assert normalize_legacy_booleans(config, schema) is config
+
+
+# --------------------------------------------------------------------------
+# The load path
+# --------------------------------------------------------------------------
+
+PLUGIN_ID = "news-shaped"
+
+
+@pytest.fixture
+def load(tmp_path):
+    """Run the real ``PluginManager.load_plugin`` for a plugin with
+    ``NEWS_SCHEMA`` and the given stored section; the plugin class is stubbed.
+
+    Returns ``(loaded, config the plugin received, logger, health_tracker)``.
+    """
+    plugins_dir = tmp_path / "plugins"
+    plugin_dir = plugins_dir / PLUGIN_ID
+    plugin_dir.mkdir(parents=True)
+    manifest = {"id": PLUGIN_ID, "name": "News-shaped", "version": "1.0.0",
+                "entry_point": "manager.py", "class_name": "Plugin"}
+    (plugin_dir / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8")
+    (plugin_dir / "config_schema.json").write_text(json.dumps(NEWS_SCHEMA),
+                                                   encoding="utf-8")
+
+    def _load(stored_section):
+        config_manager = MagicMock()
+        config_manager.load_config.return_value = {
+            PLUGIN_ID: json.loads(json.dumps(stored_section))}
+        with patch('src.common.permission_utils.ensure_directory_permissions'):
+            manager = PluginManager(plugins_dir=str(plugins_dir),
+                                    config_manager=config_manager,
+                                    display_manager=MagicMock(),
+                                    cache_manager=MagicMock())
+        manager.logger = MagicMock()
+        manager.health_tracker = MagicMock()
+        manager.plugin_manifests[PLUGIN_ID] = manifest
+        manager.plugin_loader.find_plugin_directory = MagicMock(return_value=plugin_dir)
+
+        received = {}
+
+        def fake_load_plugin(**kwargs):
+            received.update(kwargs["config"])
+            instance = MagicMock()
+            instance.validate_config.return_value = True
+            return instance, MagicMock()
+
+        manager.plugin_loader.load_plugin = MagicMock(side_effect=fake_load_plugin)
+        loaded = manager.load_plugin(PLUGIN_ID)
+        return loaded, received, manager.logger, manager.health_tracker
+
+    return _load
+
+
+def _schema_warnings(logger):
+    return [c for c in logger.warning.call_args_list
+            if "does not match its schema" in str(c.args[0])]
+
+
+def _degraded_reason(health_tracker):
+    calls = [c.args for c in health_tracker.set_degraded.call_args_list
+             if c.args[0] == PLUGIN_ID]
+    assert calls, "schema validation never ran"
+    return calls[-1][1]
+
+
+class TestLoadPath:
+    def test_news_shaped_legacy_config_loads_without_warning_or_degraded(self, load):
+        stored = {"enabled": True,
+                  "global": {"font_size": 12, "dynamic_duration": True,
+                             "headline_paging": {"enabled": False}}}
+        loaded, received, logger, tracker = load(stored)
+
+        assert loaded is True
+        assert _schema_warnings(logger) == []
+        assert _degraded_reason(tracker) is None
+        # The plugin gets the object, with the schema defaults filled in.
+        assert received["global"]["dynamic_duration"] == {
+            "enabled": True, "min_duration_seconds": 30,
+            "max_duration_seconds": 300, "buffer_ratio": 0.1}
+        # And the upgrade is logged once, below warning level.
+        assert any("global.dynamic_duration" in str(c.args)
+                   for c in logger.info.call_args_list)
+
+    def test_a_legacy_false_loads_switched_off(self, load):
+        loaded, received, logger, tracker = load(
+            {"enabled": True, "global": {"dynamic_duration": False}})
+        assert loaded is True
+        assert received["global"]["dynamic_duration"]["enabled"] is False
+        assert _schema_warnings(logger) == []
+        assert _degraded_reason(tracker) is None
+
+    def test_a_real_mismatch_still_warns_and_degrades(self, load):
+        loaded, received, logger, tracker = load(
+            {"enabled": True, "global": {"dynamic_duration": True,
+                                         "font_size": "big"}})
+        assert loaded is True  # warn-only, as before
+        warnings = _schema_warnings(logger)
+        assert len(warnings) == 1
+        assert "font_size" in str(warnings[0].args)
+        assert "dynamic_duration" not in str(warnings[0].args)
+        reason = _degraded_reason(tracker)
+        assert reason and "font_size" in reason
+
+    def test_a_boolean_under_an_object_without_enabled_still_warns(self, load):
+        loaded, received, logger, tracker = load(
+            {"enabled": True, "global": {"display": True}})
+        assert loaded is True
+        assert received["global"]["display"] is True
+        warnings = _schema_warnings(logger)
+        assert len(warnings) == 1 and "global.display" in str(warnings[0].args)
+        assert _degraded_reason(tracker)
+
+
+# --------------------------------------------------------------------------
+# Parity with the settings form
+# --------------------------------------------------------------------------
+
+PARITY_SCHEMA = {
+    "type": "object",
+    "properties": {
+        "enabled": {"type": "boolean", "default": True},
+        "global": {"type": "object", "properties": {
+            "dynamic_duration": DYNAMIC_DURATION,
+            "outer": {"type": "object", "properties": {
+                "inner": {"type": "object", "properties": {
+                    "enabled": {"type": "boolean", "default": True},
+                    "level": {"type": "integer", "default": 1},
+                }},
+            }},
+            "union": {"type": ["boolean", "object"], "properties": {
+                "enabled": {"type": "boolean"}}},
+        }},
+    },
+}
+
+
+class _Checkboxes(HTMLParser):
+    def __init__(self):
+        super().__init__()
+        self.checked = {}
+
+    def handle_starttag(self, tag, attrs):
+        a = dict(attrs)
+        if tag == "input" and a.get("type") == "checkbox" and a.get("name"):
+            self.checked[a["name"]] = "checked" in a
+
+
+def _form_checkboxes(stored):
+    from jinja2 import Environment, FileSystemLoader, select_autoescape
+
+    env = Environment(
+        loader=FileSystemLoader(str(PROJECT_ROOT / "web_interface" / "templates")),
+        autoescape=select_autoescape(["html"]),
+    )
+    plugin = {"id": "demo", "name": "Demo", "description": "", "enabled": True,
+              "author": "me", "version": "1.0.0"}
+    html = env.get_template("v3/partials/plugin_config.html").render(
+        plugin=plugin, schema=PARITY_SCHEMA, config=json.loads(json.dumps(stored)))
+    parser = _Checkboxes()
+    parser.feed(html)
+    return parser.checked
+
+
+def _loader_enabled(normalized, *path):
+    node = normalized
+    for key in path:
+        node = node.get(key) if isinstance(node, dict) else None
+    return isinstance(node, dict) and node.get("enabled") is True
+
+
+@pytest.mark.parametrize("value", [True, False, 1, "true", None])
+def test_form_and_loader_agree_on_what_a_stored_value_means(value):
+    """Where the loader reads a stored value as ``{"enabled": true}``, the form
+    draws the object's ``enabled`` checkbox ticked, and nowhere else."""
+    stored = {"enabled": True,
+              "global": {"dynamic_duration": value, "outer": {"inner": value}}}
+    normalized = normalize_legacy_booleans(stored, PARITY_SCHEMA)
+    boxes = _form_checkboxes(stored)
+
+    for path in (("global", "dynamic_duration"), ("global", "outer", "inner")):
+        name = ".".join(path) + ".enabled"
+        assert name in boxes, f"form drew no {name} checkbox"
+        assert boxes[name] is _loader_enabled(normalized, *path), name
+
+
+@pytest.mark.parametrize("value", [True, False])
+def test_form_and_loader_agree_a_boolean_union_stays_a_boolean(value):
+    stored = {"enabled": True, "global": {"union": value}}
+    normalized = normalize_legacy_booleans(stored, PARITY_SCHEMA)
+    boxes = _form_checkboxes(stored)
+
+    assert normalized is stored
+    assert boxes.get("global.union") is value
+    assert "global.union.enabled" not in boxes

web_interface/templates/v3/partials/plugin_config.html

@@ -1166,7 +1166,11 @@
        TypeError that takes down the whole config page. A legacy boolean was
        the on/off switch the object's `enabled` now holds, so carry it over --
        rendering {} would draw that checkbox unchecked and the next save would
-       quietly turn the feature off. The save then writes the object. #}
+       quietly turn the feature off. The save then writes the object.
+       The plugin loader applies the same rule before validating
+       (legacy_bool_as_object in src/plugin_system/schema_manager.py), and
+       test/test_legacy_boolean_config.py renders this macro against it:
+       change the two together. #}
     {% if value is mapping %}
         {% set nested_value = value %}
     {% elif value is boolean and 'enabled' in (prop.properties or {}) %}