fix(plugins): store and plugin-manager bugs; tidy src/plugin_system (#635)

* fix(store): don't read a ZIP-installed plugin's remote from the LEDMatrix repo update_plugin looked up remote.origin.url with `git -C <plugin> config --local` for plugins that are not git checkouts. Under plugin-repos/ git walks up to the enclosing LEDMatrix repository, so the lookup returned LEDMatrix's own URL and a plugin missing from the registry was "reinstalled" from the LEDMatrix repo. Only ask git when the plugin directory has its own .git, the test _get_local_git_info already uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(schema): report each missing required field once, by name validate_config_against_schema ran its own required-fields loop after Draft7Validator.iter_errors, which already yields one `required` error per missing field, so every missing top-level field was listed twice. The validator's copy also printed the schema's whole `required` list ("Missing required property '['api_key', 'city']'") instead of the field. Drop the loop and take the field name from the error itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(store): stop mangling repository URLs that contain ".git" install_from_url and fetch_registry_from_url cleaned URLs with `rstrip('/').replace('.git', '')`, which removes ".git" anywhere: https://github.com/user/my.github.io became .../myhub.io, so installing or browsing that repository asked GitHub for one that does not exist. Add src/plugin_system/repo_urls.py with one anchored normalize_repo_url(), same_repo() for comparisons, github_owner_repo() and github_api_headers(), and use them for the five copies of the owner/repo parsing and GitHub headers in the store and for saved repositories. GitHub URLs are now recognised by urlparse().hostname everywhere: _get_latest_commit_info used a substring test, and _install_from_monorepo_api parsed any host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(store): install a repository whose only branch is not main/master _install_via_git returned None both when every clone failed and when the last-resort clone of the repository's default branch succeeded. _install_plugin_impl papered over it with `and not plugin_path.exists()`; install_from_url did not, so a repository whose only branch is e.g. `develop` was cloned, then treated as a failure, then "downloaded" from main/master archives that do not exist. After a default-branch clone, return the branch the clone checked out (read from .git/HEAD), so None means failure and nothing else, and give both callers the same `branch_used is None` fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(plugins): judge the memory limit on each call's own growth monitor_call stores `metrics.memory_mb = max(previous, growth)`, and _check_limits compared that high-water mark with max_memory_mb. It never decreases, so once one update() grew the process past the limit every later call raised ResourceLimitExceeded and the circuit breaker kept reopening. Pass the call's own RSS growth to _check_limits; keep the high-water mark for reporting and document what it measures. Remove ResourceMetrics.update_average_execution_time: nothing called it, and it overwrote the running total with the average. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(plugins): reload_plugin re-reads the manifest from the discovered directory reload_plugin read `plugins_dir / plugin_id / "manifest.json"`, ignoring the discovery map and the plugin_dirs rules. For a plugin whose directory name differs from its manifest id the path did not exist, the re-read was skipped without a word, and the reload kept the stale manifest. Resolve the directory with find_plugin_directory, as load_plugin does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(plugins): drop the always-null last_display from plugin state info PluginStateManager reported `last_display` from `_last_display`, which nothing ever wrote, so it was null for every plugin. Recording it in PluginExecutor.execute_display would not help: get_state_info's only reader is the web process, whose PluginManager never calls display(). Remove the field, its dict and get_last_display() (no caller in core, the web UI or the plugin monorepo). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(store): share the rollback and requirements helpers, drop dead code - install_plugin and _reinstall_with_rollback set aside, discard and restore the old copy through _set_aside/_discard_backup/_restore_backup instead of two copies of the same blocks. - The loader and the store run the same pre-pip checks through contained_plugin_dir() and requirements_to_install() in plugin_loader. They still invoke pip differently (sys.executable -m pip vs. the sudo wrapper). `except (BrokenPipeError, OSError)` + `isinstance(e, OSError)` becomes `except OSError` checking errno.EPIPE. - load_module never returns None, so load_plugin's check is gone and the docstring says what it raises. - Remove the always-true JSONSCHEMA_AVAILABLE, the inline re-imports of re and permission_utils, the fake status_result object nobody reads, hasattr(git_error, 'cmd'), a redundant "merge conflict" test and `import traceback` (exc_info=True does it). - Correct comments: install_from_url names the directory for the caller's id when given (not always the manifest id), _get_local_git_info saves one git subprocess (not four), _enrich calls two helpers, search_plugins documents all its arguments, _find_plugin_path states its behaviour instead of a TODO, and history narration is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(plugins): tidy base_plugin, correct plugin_manager/state comments - base_plugin: drop the unused `import logging`; get_display_duration runs the instance value and the config value through one _positive_seconds() helper instead of two copies of the coercion; the 'static'/'none'/fallback branches of get_vegas_display_mode, which all returned FIXED_SEGMENT, are one; fix the mis-indented validate_config example; say that get_supported_vegas_modes/get_vegas_segment_width are not consulted by core (kept, plugins override them). - schema_manager: import expand_style_elements normally rather than swallowing an ImportError of a core module. - plugin_manager: the plugins directory is the configured one (plugin-repos/ by default), not plugins/; get_config() returns the live dict, not a copy, so the interval cache comments say what it saves. - state_manager: config_version and the file version are not used to detect corruption; say what they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(plugins): stop writing data/plugin_operations.json PluginOperationQueue wrote its finished-operation history to data/plugin_operations.json after every operation, and read it back only into its own in-memory list, which only get_operation_history() exposes -- and nothing calls that. The operation-history endpoint reads OperationHistory (data/operation_history.json). No code in src/, web_interface/, scripts/ or test/ reads the file. Drop the history_file/lazy_load parameters and the load/save code; the bounded in-memory history stays. web_interface/app.py and the integration test stop passing the removed arguments. An existing data/plugin_operations.json is left in place (data/* is gitignored). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(changelog): plugin-system Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 14:25:08 +00:00 · 2026-09-24 17:32:02 -04:00
parent 3967a6cffc
commit b11bcfa204
24 changed files with 958 additions and 756 deletions
@@ -299,3 +299,48 @@ class TestDefaultMerging:

        assert merged["enabled"] is False
        assert merged["display_duration"] == 60
+
+
+class TestMissingRequiredFields:
+    """One message per missing field, naming that field.
+
+    A manual ``required`` loop used to run after Draft7Validator, which already
+    reports ``required``, so every missing top-level field was listed twice --
+    and the validator's copy printed the schema's whole ``required`` list as
+    if it were the field name.
+    """
+
+    SCHEMA = {
+        "type": "object",
+        "properties": {
+            "api_key": {"type": "string"},
+            "city": {"type": "string"},
+            "units": {"type": "string"},
+        },
+        "required": ["api_key", "city", "units"],
+    }
+
+    def test_each_missing_field_is_reported_once_by_name(self):
+        ok, errors = SchemaManager().validate_config_against_schema(
+            {"units": "metric"}, self.SCHEMA, "test-plugin")
+
+        assert not ok
+        assert errors == [
+            "Field root: Missing required property 'api_key'",
+            "Field root: Missing required property 'city'",
+        ]
+
+    def test_nested_missing_field_names_the_field_and_its_parent(self):
+        schema = {
+            "type": "object",
+            "properties": {"nfl": {
+                "type": "object",
+                "properties": {"api_key": {"type": "string"}},
+                "required": ["api_key"],
+            }},
+        }
+        ok, errors = SchemaManager().validate_config_against_schema(
+            {"nfl": {}}, schema, "test-plugin")
+
+        assert not ok
+        assert errors == ["Field 'nfl': Missing required property 'api_key'"]
@@ -116,10 +116,14 @@ def test_values_of_the_wrong_type_fall_back_to_usable_defaults(bad):
    assert isinstance(getattr(metrics, field_name), (int, float)), \
        f"{field_name} came back as {getattr(metrics, field_name)!r}"

-    # The real proof: arithmetic on the loaded metrics must not explode.
+    # The real proof: the arithmetic monitor_call and get_metrics_summary do
+    # on the loaded metrics must not explode.
    metrics.call_count += 1
    metrics.total_execution_time += 0.5
-    metrics.update_average_execution_time()
+    metrics.max_execution_time = max(metrics.max_execution_time, 0.5)
+    metrics.min_execution_time = min(metrics.min_execution_time, 0.5)
+    metrics.memory_mb = max(metrics.memory_mb, 1.0)
+    assert metrics.total_execution_time / metrics.call_count >= 0


 def test_a_numeric_string_is_accepted_rather_than_discarded():
@@ -0,0 +1,36 @@
+"""reload_plugin re-reads the manifest from the plugin's actual directory.
+
+It read ``plugins_dir / plugin_id / manifest.json``, but a plugin directory's
+name need not be the id its manifest declares -- discovery maps ids to
+directories for exactly that reason. For such a plugin the path did not exist,
+the re-read was skipped silently, and the reload kept the stale manifest.
+"""
+
+import json
+
+import pytest
+
+from src.plugin_system.plugin_manager import PluginManager
+
+
+@pytest.fixture
+def pm_with_renamed_dir(tmp_path):
+    plugins_dir = tmp_path / "plugins"
+    plugin_dir = plugins_dir / "stock-ticker-v2"
+    plugin_dir.mkdir(parents=True)
+    manifest_path = plugin_dir / "manifest.json"
+    manifest_path.write_text(json.dumps({"id": "stocks", "version": "1.0.0"}))
+    pm = PluginManager(plugins_dir=str(plugins_dir))
+    assert pm.discover_plugins() == ["stocks"]
+    return pm, manifest_path
+
+
+def test_reload_picks_up_an_edited_manifest(pm_with_renamed_dir, monkeypatch):
+    pm, manifest_path = pm_with_renamed_dir
+    manifest_path.write_text(json.dumps({"id": "stocks", "version": "2.0.0"}))
+    loaded = []
+    monkeypatch.setattr(pm, "load_plugin", lambda pid: loaded.append(pid) or True)
+
+    assert pm.reload_plugin("stocks") is True
+    assert pm.plugin_manifests["stocks"]["version"] == "2.0.0"
+    assert loaded == ["stocks"]
@@ -115,5 +115,22 @@ def test_get_state_info_is_a_consistent_snapshot():
    assert not inconsistent, f"observed a torn snapshot: {inconsistent[:1]}"


+def test_state_info_reports_only_what_something_records():
+    """No field that is always null.
+
+    ``last_display`` was reported here, but nothing ever recorded a display()
+    call, so it was null for every plugin. Its only reader is the web process,
+    whose PluginManager never calls display(), so recording it in the display
+    process could not have filled it either.
+    """
+    manager = PluginStateManager()
+    manager.set_state("clock", PluginState.ENABLED)
+    manager.record_update("clock")
+
+    info = manager.get_state_info("clock")
+    assert "last_display" not in info
+    assert info["last_update"] is not None
+
+
 if __name__ == "__main__":
    sys.exit(pytest.main([__file__, "-v"]))
@@ -0,0 +1,95 @@
+"""Repository URL handling shared by the plugin store and saved repositories.
+
+The store cleaned URLs with ``url.rstrip('/').replace('.git', '')`` in two
+places, which removes ``.git`` anywhere in the URL:
+``https://github.com/user/my.github.io`` became ``.../myhub.io``, so installing
+or browsing such a repository asked GitHub for one that does not exist.
+"""
+
+from unittest.mock import MagicMock
+
+import pytest
+
+from src.plugin_system.repo_urls import (
+    github_api_headers, github_owner_repo, normalize_repo_url, same_repo,
+)
+from src.plugin_system.store_manager import PluginStoreManager
+
+PAGES_REPO = "https://github.com/user/my.github.io"
+
+
+class TestNormalizeRepoUrl:
+    @pytest.mark.parametrize("raw, expected", [
+        (PAGES_REPO, PAGES_REPO),
+        (PAGES_REPO + ".git", PAGES_REPO),
+        ("https://github.com/user/repo.git/", "https://github.com/user/repo"),
+        ("  https://github.com/user/repo/  ", "https://github.com/user/repo"),
+    ])
+    def test_only_a_trailing_dot_git_is_removed(self, raw, expected):
+        assert normalize_repo_url(raw) == expected
+
+    def test_same_repo_ignores_case_and_suffix(self):
+        assert same_repo("https://github.com/Owner/Repo.git",
+                         "https://github.com/owner/repo/")
+        assert not same_repo("https://github.com/owner/repo",
+                             "https://github.com/owner/other")
+
+
+class TestGithubOwnerRepo:
+    @pytest.mark.parametrize("url, expected", [
+        (PAGES_REPO + ".git", ("user", "my.github.io")),
+        ("https://www.github.com/owner/repo", ("owner", "repo")),
+        ("https://github.com/owner/repo/tree/main/plugins/x", ("owner", "repo")),
+    ])
+    def test_github_urls(self, url, expected):
+        assert github_owner_repo(url) == expected
+
+    @pytest.mark.parametrize("url", [
+        "https://github.com.example.org/owner/repo",
+        "https://gitlab.com/owner/repo",
+        "https://github.com/owner",
+        "github.com/owner/repo",
+    ])
+    def test_anything_else_is_not_a_github_repo(self, url):
+        assert github_owner_repo(url) is None
+
+    def test_headers_carry_the_token_only_when_given(self):
+        assert "Authorization" not in github_api_headers(None)
+        assert github_api_headers("abc")["Authorization"] == "token abc"
+
+
+@pytest.fixture
+def store(tmp_path):
+    return PluginStoreManager(
+        plugins_dir=str(tmp_path / "plugins"),
+        uninstalled_registry_path=str(tmp_path / "uninstalled.json"))
+
+
+def test_install_from_url_keeps_an_interior_dot_git(store, monkeypatch):
+    cloned_from = []
+    monkeypatch.setattr(store, "_install_via_git",
+                        lambda url, *a, **k: cloned_from.append(url))
+    downloaded = []
+    monkeypatch.setattr(store, "_install_via_download",
+                        lambda url, *a, **k: downloaded.append(url) or False)
+
+    result = store.install_from_url(PAGES_REPO + ".git")
+
+    assert result["success"] is False
+    assert cloned_from == [PAGES_REPO]
+    assert all(url.startswith(PAGES_REPO + "/archive/") for url in downloaded)
+
+
+def test_fetch_registry_from_url_asks_for_the_named_repository(store, monkeypatch):
+    requested = []
+
+    def fake_get(url, **kwargs):
+        requested.append(url)
+        return MagicMock(status_code=404)
+
+    monkeypatch.setattr(store, "_http_get_with_retries", fake_get)
+
+    assert store.fetch_registry_from_url(PAGES_REPO) is None
+    assert requested
+    assert all(url.startswith("https://raw.githubusercontent.com/user/my.github.io/")
+               for url in requested)
@@ -93,6 +93,27 @@ class TestResourceLimits:
        with pytest.raises(ResourceLimitExceeded):
            mon.monitor_call("p", lambda: time.sleep(0.02))

+    def test_memory_limit_judges_each_call_on_its_own_growth(self):
+        """One expensive call must not fail every call after it.
+
+        The check used to compare the stored high-water mark, which never
+        decreases, so after one call grew memory past the limit every later
+        call raised too and the plugin never updated again.
+        """
+        mon = PluginResourceMonitor(_cache(), enable_monitoring=False)
+        mon.enable_monitoring = True  # measure without needing psutil
+        readings = iter([100.0, 200.0,   # first call grows RSS by 100 MB
+                         200.0, 201.0])  # second call grows it by 1 MB
+        mon._get_process_memory_mb = lambda: next(readings)
+        mon._get_process_cpu_percent = lambda: 0.0
+        mon.set_limits("p", ResourceLimits(max_memory_mb=50))
+
+        with pytest.raises(ResourceLimitExceeded):
+            mon.monitor_call("p", lambda: None)
+        assert mon.monitor_call("p", lambda: "ok") == "ok"
+        # The high-water mark is still reported.
+        assert mon.get_metrics("p").memory_mb == 100.0
+
    def test_reset_metrics_clears_counts(self):
        cache = _cache()
        mon = PluginResourceMonitor(cache, enable_monitoring=False)
@@ -5,7 +5,7 @@ SavedRepositoriesManager contract.
 Covers: the three accepted on-disk load shapes (bare list, wrapped
 {"repositories": [...]}, anything else -> []) and that saves always write
 the bare-list form; add/remove/has round trips through a fresh manager;
-URL normalization post-fix (_clean_url strips only a TRAILING '.git' after
+URL normalization post-fix (normalize_repo_url strips only a TRAILING '.git' after
 trailing slashes — the old unanchored .replace('.git', '') mangled URLs
 like my.github.io); name derivation and registry-vs-single type
 classification (the ledmatrix-plugins check is lowercased, the
@@ -0,0 +1,77 @@
+"""A repository whose only branch is neither main nor master still installs.
+
+_install_via_git tries the candidate branches, then the repository's default
+branch -- but it returned None both for "every clone failed" and for "the
+default-branch clone succeeded". install_from_url took the None as failure,
+fell through to the archive download of main/master (which does not exist),
+and reported "Failed to clone or download repository" for a repository it had
+just cloned.
+"""
+
+import json
+import shutil
+import subprocess
+
+import pytest
+
+from src.plugin_system.store_manager import PluginStoreManager
+
+pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git not installed")
+
+MANIFEST = {
+    "id": "develop-only", "name": "Develop Only", "class_name": "P",
+    "display_modes": ["develop_only"], "version": "1.0.0",
+}
+
+
+def _git(*args, cwd):
+    subprocess.run(
+        ["git", "-c", "user.name=t", "-c", "user.email=t@example.invalid", *args],
+        cwd=cwd, check=True, capture_output=True)
+
+
+@pytest.fixture
+def develop_only_repo(tmp_path):
+    repo = tmp_path / "upstream"
+    repo.mkdir()
+    _git("init", "-q", "-b", "develop", cwd=repo)
+    (repo / "manifest.json").write_text(json.dumps(MANIFEST))
+    (repo / "manager.py").write_text("class P: pass\n")
+    _git("add", ".", cwd=repo)
+    _git("commit", "-q", "-m", "init", cwd=repo)
+    return repo.as_uri()
+
+
+@pytest.fixture
+def store(tmp_path, monkeypatch):
+    mgr = PluginStoreManager(
+        plugins_dir=str(tmp_path / "plugins"),
+        uninstalled_registry_path=str(tmp_path / "uninstalled.json"))
+    monkeypatch.setattr(mgr, "_install_dependencies", lambda *a, **k: True)
+    downloads = []
+    monkeypatch.setattr(mgr, "_install_via_download",
+                        lambda url, *a, **k: downloads.append(url) or False)
+    mgr.downloads = downloads
+    return mgr
+
+
+def test_a_default_branch_clone_reports_its_branch(store, develop_only_repo, tmp_path):
+    target = tmp_path / "clone"
+    assert store._install_via_git(develop_only_repo, target, ["main", "master"]) == "develop"
+    assert (target / "manifest.json").exists()
+
+
+def test_a_failed_clone_reports_none(store, tmp_path):
+    missing = (tmp_path / "no-such-repo").as_uri()
+    target = tmp_path / "clone"
+    assert store._install_via_git(missing, target, ["main"]) is None
+    assert not target.exists()
+
+
+def test_install_from_url_installs_a_develop_only_repository(store, develop_only_repo):
+    result = store.install_from_url(develop_only_repo)
+
+    assert result == {"success": True, "plugin_id": "develop-only",
+                      "name": "Develop Only", "branch": "develop"}
+    assert (store.plugins_dir / "develop-only" / "manifest.json").exists()
+    assert store.downloads == []
@@ -0,0 +1,62 @@
+"""update_plugin must not borrow the enclosing LEDMatrix checkout's remote.
+
+Plugins live in ``plugin-repos/`` inside the LEDMatrix git checkout. For a
+plugin installed from a ZIP (no ``.git`` of its own), ``git -C <plugin>``
+walks up to the LEDMatrix repository, and ``git config --local --get
+remote.origin.url`` answers with LEDMatrix's own URL. update_plugin then tried
+to "reinstall" the plugin from the LEDMatrix repository.
+"""
+
+import json
+import shutil
+import subprocess
+
+import pytest
+
+from src.plugin_system.store_manager import PluginStoreManager
+
+pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git not installed")
+
+PLUGIN_ID = "zip-installed"
+PARENT_REMOTE = "https://github.com/example/LEDMatrix"
+
+
+def _git(*args, cwd):
+    subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True)
+
+
+@pytest.fixture
+def store_inside_checkout(tmp_path):
+    checkout = tmp_path / "LEDMatrix"
+    checkout.mkdir()
+    _git("init", "-q", cwd=checkout)
+    _git("remote", "add", "origin", PARENT_REMOTE, cwd=checkout)
+
+    plugins_dir = checkout / "plugin-repos"
+    plugin_dir = plugins_dir / PLUGIN_ID
+    plugin_dir.mkdir(parents=True)
+    (plugin_dir / "manifest.json").write_text(json.dumps(
+        {"id": PLUGIN_ID, "name": "Zip", "version": "1.0.0"}))
+
+    store = PluginStoreManager(
+        plugins_dir=str(plugins_dir),
+        uninstalled_registry_path=str(tmp_path / "uninstalled.json"))
+    return store, plugin_dir
+
+
+def test_a_plugin_without_its_own_git_has_no_remote(store_inside_checkout, monkeypatch):
+    store, plugin_dir = store_inside_checkout
+    # The premise: git itself does report the parent's remote here.
+    parent_view = subprocess.run(
+        ["git", "-C", str(plugin_dir), "config", "--local", "--get", "remote.origin.url"],
+        capture_output=True, text=True)
+    assert parent_view.stdout.strip() == PARENT_REMOTE
+
+    monkeypatch.setattr(store, "fetch_registry", lambda *a, **k: {"plugins": []})
+    monkeypatch.setattr(store, "get_plugin_info", lambda *a, **k: None)
+    install_calls = []
+    monkeypatch.setattr(store, "install_from_url",
+                        lambda *a, **k: install_calls.append((a, k)) or {"success": True})
+
+    assert store.update_plugin(PLUGIN_ID) is False
+    assert install_calls == []
@@ -21,10 +21,7 @@ class TestPluginOperationsIntegration(unittest.TestCase):
        self.temp_dir = Path(tempfile.mkdtemp())
        
        # Initialize components
-        self.operation_queue = PluginOperationQueue(
-            history_file=str(self.temp_dir / "operations.json"),
-            max_history=100
-        )
+        self.operation_queue = PluginOperationQueue(max_history=100)
        
        self.state_manager = PluginStateManager(
            state_file=str(self.temp_dir / "state.json"),