fix(plugins): warn once per directory, not once per scan

Self-review catch. Discovery runs on every web UI page load and every config
reconcile, so warning unconditionally about an unloadable directory would put
a line in the journal each time someone opened a page -- the same log-volume
problem this change exists to help diagnose.

The skip is now reported once per directory per process. The diagnostic value
is unchanged: the reason a plugin is missing still appears in the journal,
once, where before it appeared nowhere.

Test added covering five consecutive scans producing one warning.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

scripts/install/configure_wifi_permissions.sh

@@ -37,10 +37,6 @@ echo "  systemctl: $SYSTEMCTL_PATH"
 echo ""
 echo "Step 1: Configuring sudo permissions for nmcli..."
 SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi"
-SYSCTL_PATH=$(command -v sysctl || echo /usr/sbin/sysctl)
-NFT_PATH=$(command -v nft || echo /usr/sbin/nft)
-RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill)
-MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir)
 
 # Create a temporary sudoers file using mktemp (handles permissions better)
 TEMP_SUDOERS=$(mktemp) || {
@@ -66,36 +62,6 @@ $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH start dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager
-# The captive portal turns IP forwarding on while the access point is up and
-# restores the previous value when it comes down (wifi_manager._setup_iptables_
-# redirect / _teardown_iptables_redirect). Without this rule that sudo call
-# needs a password, so forwarding stays off and clients associate to the AP but
-# cannot route. It goes unnoticed on a stock Raspberry Pi image, where
-# /etc/sudoers.d/010_pi-nopasswd grants the default user blanket NOPASSWD and
-# masks every gap in this file -- it only bites once that blanket rule is
-# removed.
-$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=0
-$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=1
-# The portal's redirect lives in its own nftables table, created when the AP
-# comes up and deleted when it goes down, and the radio has to be unblocked
-# before the AP can start at all. Same story as the sysctl rules above: called
-# with sudo, never granted here, and invisible on a stock Pi image.
-$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH add table ip ledmatrix
-$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH delete table ip ledmatrix
-$WEB_USER ALL=(ALL) NOPASSWD: $RFKILL_PATH unblock wifi
-# NetworkManager's dnsmasq drop-in directory, exact path.
-$WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d
-#
-# iptables is deliberately NOT granted here. Its rules are built from the live
-# interface name and port, so a rule covering them needs a trailing wildcard --
-# and `iptables --modprobe=/path/to/anything` runs that path as root, so
-# `NOPASSWD: iptables *` is a root shell for the web user by another name. That
-# is a worse outcome than the gap it would close, which today is masked anyway
-# by the blanket NOPASSWD rule on stock Pi images.
-#
-# Closing it safely means a wrapper script that builds the rules itself and
-# takes only an interface and a port, granted the way safe_plugin_rm.sh already
-# is. That belongs in its own change rather than being smuggled into this one.
 
 # Allow copying hostapd and dnsmasq config files into place
 $WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf

src/plugin_system/plugin_manager.py

@@ -76,6 +76,9 @@ class PluginManager:
         # Lock protecting plugin_manifests and plugin_directories from
         # concurrent mutation (background reconciliation) and reads (requests).
         self._discovery_lock = threading.RLock()
+        #: Directories already reported as unloadable, so the warning is
+        #: emitted once rather than on every discovery scan.
+        self._skip_reported: set = set()
 
         # Lock protecting plugin_last_update from concurrent mutation/iteration.
         # It's written from run_scheduled_updates()/update_all_plugins() (main
@@ -195,18 +198,45 @@ class PluginManager:
                     continue
 
                 manifest_path = item / "manifest.json"
-                if manifest_path.exists():
-                    try:
-                        with open(manifest_path, 'r', encoding='utf-8') as f:
-                            manifest = json.load(f)
-                            plugin_id = manifest.get('id')
-                            if plugin_id:
-                                plugin_ids.append(plugin_id)
-                                new_manifests[plugin_id] = manifest
-                                new_directories[plugin_id] = item
-                    except (json.JSONDecodeError, PermissionError, OSError) as e:
-                        self.logger.warning("Error reading manifest from %s: %s", manifest_path, e, exc_info=True)
-                        continue
+                if not manifest_path.exists():
+                    # Once per directory per process. Discovery runs on every
+                    # web UI page load and every config reconcile, so warning
+                    # unconditionally would put a line in the journal each
+                    # time someone opened a page -- the same log-volume
+                    # problem this is meant to help diagnose.
+                    # A directory here that carries no manifest is not a
+                    # plugin. Said once, because the alternative is a plugin
+                    # that is enabled in config, enabled in plugin state,
+                    # present on disk, and simply absent from the running
+                    # process with nothing anywhere to say why. Working that
+                    # out afterwards means reading cache-file mtimes.
+                    if item.name not in self._skip_reported:
+                        self._skip_reported.add(item.name)
+                        self.logger.warning(
+                            "Skipping %s: no manifest.json, so it cannot be "
+                            "loaded as a plugin", item.name)
+                    continue
+                try:
+                    with open(manifest_path, 'r', encoding='utf-8') as f:
+                        manifest = json.load(f)
+                except (json.JSONDecodeError, PermissionError, OSError) as e:
+                    self.logger.warning("Error reading manifest from %s: %s", manifest_path, e, exc_info=True)
+                    continue
+
+                plugin_id = manifest.get('id')
+                if not plugin_id:
+                    # Parsed but unusable. This was the quietest path of all:
+                    # the manifest is read successfully and then dropped.
+                    if item.name not in self._skip_reported:
+                        self._skip_reported.add(item.name)
+                        self.logger.warning(
+                            "Skipping %s: its manifest.json has no \"id\", so "
+                            "there is nothing to register it under", item.name)
+                    continue
+
+                plugin_ids.append(plugin_id)
+                new_manifests[plugin_id] = manifest
+                new_directories[plugin_id] = item
         except (OSError, PermissionError) as e:
             self.logger.error("Error scanning directory %s: %s", directory, e, exc_info=True)
 

test/test_plugin_discovery_reports_skips.py

@@ -0,0 +1,81 @@
+#!/usr/bin/env python3
+"""Discovery must say when it skips a directory.
+
+A plugin can be enabled in config, enabled in plugin state, present on disk
+with a valid entry point -- and simply absent from the running process, with
+nothing in the journal to say why. Working that out afterwards meant comparing
+cache-file mtimes to find when it had last run.
+
+Two paths were silent. A directory with no manifest.json was ignored, and --
+quieter still -- a manifest that parsed but carried no "id" was read
+successfully and then dropped on the floor.
+"""
+import json
+import logging
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock
+
+sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
+
+from src.plugin_system.plugin_manager import PluginManager  # noqa: E402
+
+
+def _manager(tmp_path):
+    pm = PluginManager.__new__(PluginManager)
+    pm.plugins_dir = tmp_path
+    pm.logger = logging.getLogger("test.discovery")
+    pm.plugin_manifests = {}
+    pm.plugin_directories = {}
+    pm._discovery_lock = __import__("threading").RLock()
+    pm._skip_reported = set()
+    pm.schema_manager = MagicMock()
+    return pm
+
+
+def test_a_directory_without_a_manifest_is_reported(tmp_path, caplog):
+    (tmp_path / "not-a-plugin").mkdir()
+    pm = _manager(tmp_path)
+    with caplog.at_level(logging.WARNING, logger="test.discovery"):
+        pm._scan_directory_for_plugins(tmp_path)
+    joined = " ".join(r.message for r in caplog.records)
+    assert "not-a-plugin" in joined and "manifest" in joined, (
+        f"skip was silent; log said: {joined!r}")
+
+
+def test_a_manifest_without_an_id_is_reported(tmp_path, caplog):
+    d = tmp_path / "idless"
+    d.mkdir()
+    (d / "manifest.json").write_text(json.dumps({"name": "No Id", "version": "1.0.0"}))
+    pm = _manager(tmp_path)
+    with caplog.at_level(logging.WARNING, logger="test.discovery"):
+        pm._scan_directory_for_plugins(tmp_path)
+    joined = " ".join(r.message for r in caplog.records)
+    assert "idless" in joined and "id" in joined, (
+        f"a parsed-but-unusable manifest vanished silently; log said: {joined!r}")
+
+
+def test_a_good_plugin_still_registers(tmp_path, caplog):
+    d = tmp_path / "real-plugin"
+    d.mkdir()
+    (d / "manifest.json").write_text(json.dumps(
+        {"id": "real-plugin", "name": "Real", "version": "1.0.0"}))
+    pm = _manager(tmp_path)
+    pm._scan_directory_for_plugins(tmp_path)
+    assert "real-plugin" in pm.plugin_manifests, "a valid plugin was not registered"
+
+
+def test_the_warning_does_not_repeat_on_every_scan(tmp_path, caplog):
+    """Discovery runs on every web UI page load and every config reconcile.
+
+    Warning unconditionally would put a line in the journal each time someone
+    opened a page -- the same log-volume problem this is meant to help
+    diagnose.
+    """
+    (tmp_path / "not-a-plugin").mkdir()
+    pm = _manager(tmp_path)
+    with caplog.at_level(logging.WARNING, logger="test.discovery"):
+        for _ in range(5):
+            pm._scan_directory_for_plugins(tmp_path)
+    hits = [r for r in caplog.records if "not-a-plugin" in r.message]
+    assert len(hits) == 1, f"warned {len(hits)} times across 5 scans"

test/test_sudo_allowlist_covers_calls.py

@@ -1,120 +0,0 @@
-"""The captive portal's fixed-argument sudo calls must be granted.
-
-The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and
-ledmatrix_wifi. A sudo call absent from both needs a password, which a service
-cannot supply, so it fails.
-
-Four such calls were ungranted, all of them captive-portal teardown/setup:
-
-    sysctl -w net.ipv4.ip_forward=0|1     wifi_manager.py:788, 883
-    nft add|delete table ip ledmatrix     wifi_manager.py:835, 895
-    rfkill unblock wifi                   wifi_manager.py:1811
-    mkdir -p .../dnsmasq-shared.d         wifi_manager.py:922
-
-It goes unnoticed because a stock Raspberry Pi image ships
-/etc/sudoers.d/010_pi-nopasswd granting the default user
-`ALL=(ALL) NOPASSWD: ALL`, which satisfies every gap in both files. It only
-bites once that blanket rule is removed or the service runs as another user.
-
-Scope, deliberately narrow: this pins the four commands above, each of which
-can be written out literally. The portal makes further sudo calls whose
-arguments are built at runtime -- iptables and nft rules carrying an interface
-name and a port, `ip addr`, `ip link` -- and those cannot be granted safely
-here. A rule covering them needs a trailing wildcard, and
-`iptables --modprobe=/path/to/anything` runs that path as root, so
-`NOPASSWD: iptables *` is a root shell for the web user by another name.
-Closing that half needs a privileged helper that builds the rules itself and
-takes only an interface and a port, granted the way safe_plugin_rm.sh already
-is. That is a design decision, not a one-line grant, and belongs in its own
-change.
-"""
-import re
-from pathlib import Path
-
-import pytest
-
-ROOT = Path(__file__).resolve().parent.parent
-INSTALLERS = (
-    ROOT / "first_time_install.sh",
-    ROOT / "scripts" / "install" / "configure_wifi_permissions.sh",
-)
-
-#: Commands this change grants, each fully literal in the source.
-REQUIRED = (
-    ("sysctl", "-w", "net.ipv4.ip_forward=0"),
-    ("sysctl", "-w", "net.ipv4.ip_forward=1"),
-    ("nft", "add", "table", "ip", "ledmatrix"),
-    ("nft", "delete", "table", "ip", "ledmatrix"),
-    ("rfkill", "unblock", "wifi"),
-    ("mkdir", "-p", "/etc/NetworkManager/dnsmasq-shared.d"),
-)
-
-#: Tools with an option that executes a program of the caller's choosing.
-#: A trailing wildcard on any of these is a privilege escalation.
-EXEC_CAPABLE = ("iptables", "ip6tables", "nft", "tcpdump", "find", "awk",
-                "sed", "perl", "python", "python3", "env")
-
-
-def _grant_lines():
-    lines = []
-    for installer in INSTALLERS:
-        if not installer.is_file():
-            continue
-        for line in installer.read_text(encoding="utf-8", errors="replace").splitlines():
-            if "NOPASSWD:" in line:
-                lines.append(line.split("NOPASSWD:", 1)[1])
-    return lines
-
-
-def _normalised_grants():
-    """Grants with binary-path variables reduced to tool names.
-
-    Rules are written as `$SYSCTL_PATH -w ...`, so matching the literal
-    "sysctl" finds nothing and every rule looks absent -- which is exactly how
-    an earlier version of this test reported six gaps that did not exist.
-    Only NOPASSWD lines are considered, because taking the whole script let a
-    variable definition such as NFT_PATH=$(command -v nft) satisfy the check on
-    its own while the grant itself had been deleted.
-    """
-    text = "\n".join(_grant_lines())
-    text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", lambda m: m.group(1).lower(), text)
-    return re.sub(r"/usr/(?:s?bin)/", "", text)
-
-
-def test_the_installers_are_present():
-    missing = [str(p.relative_to(ROOT)) for p in INSTALLERS if not p.is_file()]
-    assert not missing, f"installer(s) missing: {missing}"
-
-
-@pytest.mark.parametrize("command", REQUIRED, ids=lambda c: " ".join(c))
-def test_the_command_is_granted(command):
-    """Whole command, not just the binary.
-
-    Checking only the binary made this far weaker than it looked: with
-    `sysctl` present anywhere, deleting the ip_forward=0 grant still passed,
-    and the portal would then be unable to restore forwarding on teardown.
-    """
-    pattern = r"\s+".join(re.escape(word) for word in command)
-    assert re.search(pattern, _normalised_grants()), (
-        f"no installer grants `{' '.join(command)}`")
-
-
-def test_no_wildcard_on_a_tool_that_can_exec():
-    """`NOPASSWD: iptables *` hands the web user root.
-
-    iptables --modprobe=/path runs that path as root. This caught a grant added
-    in this very change, which is why it is here.
-    """
-    offenders = []
-    for rule in _grant_lines():
-        rule = rule.strip()
-        if not rule.endswith("*"):
-            continue
-        haystack = rule.replace("_PATH", "").lower()
-        for tool in EXEC_CAPABLE:
-            if re.search(rf"(^|/|\s|\$){tool}(\s|$)", haystack):
-                offenders.append(rule)
-                break
-    assert not offenders, (
-        "wildcard grant on a tool that can execute another program:\n  "
-        + "\n  ".join(offenders))