From ece416c4e5d2359b3e14baf0dab8eaf5796a6096 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 24 Sep 2026 15:52:52 -0400 Subject: [PATCH] refactor(plugins): one plugin-directory resolver (#623) * refactor(plugins): one resolver for plugin id -> directory Five places mapped a plugin id to its directory, each with its own rules and each re-reading manifests per lookup: PluginManager discovery and get_plugin_directory, PluginLoader.find_plugin_directory, PluginStoreManager._find_plugin_path / list_installed_plugins, and state_reconciliation.disk_plugin_ids. They disagreed on backup dirs, on whether the manifest id or the directory name is the id, on duplicate ids and on path safety. src/plugin_system/plugin_dirs.py now holds the rules once: PluginDirectoryIndex scans one directory and reads each manifest once; resolve_plugin_dir() searches directories in order. What legitimately differs per caller is an explicit argument: search dirs (discovery and the loader: configured dir only; the store: configured then sibling plugins/), ledmatrix- prefix (not for the store), case folding (loader only), manifest pass (not for get_plugin_directory, whose discovery map already holds it). Behaviour changes, all for layouts installs do not produce: - a directory whose manifest declares the id beats one merely named for it (discovery already worked this way; the loader and store now agree) - the store searches the configured dir completely before plugins/ - backup and hidden dirs are skipped everywhere (the loader's case and manifest scans and list_installed_plugins used to return them) - duplicate ids resolve deterministically (exact name, then ledmatrix-, then by name) with a one-time warning; discovery no longer lists the id twice - disk_plugin_ids / list_installed_plugins report manifest ids, falling back to the directory name; auto-update looks the directory up - ids that are not one plain path segment resolve to nothing in every caller (the loader used to truncate them, the store to join them) The .standalone-backup- marker is one constant, BACKUP_MARKER, used by store_manager's rename-aside names and every lookup. Co-Authored-By: Claude Opus 5.5 * docs(changelog): one plugin-directory resolver Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 8 + src/plugin_system/plugin_dirs.py | 345 ++++++++++++++++++++++ src/plugin_system/plugin_loader.py | 87 ++---- src/plugin_system/plugin_manager.py | 159 +++++----- src/plugin_system/state_reconciliation.py | 58 ++-- src/plugin_system/store_manager.py | 145 +++------ test/test_discovery_path_contract.py | 32 +- test/test_plugin_dirs.py | 343 +++++++++++++++++++++ web_interface/auto_update.py | 13 +- 9 files changed, 895 insertions(+), 295 deletions(-) create mode 100644 src/plugin_system/plugin_dirs.py create mode 100644 test/test_plugin_dirs.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1eb67063..1ef00eff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,14 @@ accepts both, but the store flags the old spelling as deprecated (`web_interface/cache.py`) now honours the TTL a value was stored with and is thread-safe. +- One plugin-directory resolver, `src/plugin_system/plugin_dirs.py`, behind + discovery, `PluginManager.get_plugin_directory`, `PluginLoader`, the store and + state reconciliation. A manifest's `id` wins over a directory merely named for + the id; hidden and `.standalone-backup-` directories are never treated as + plugins (auto-update could previously try to update a backup); ids like + `a/b` or `..` resolve to nothing everywhere. Installs where each directory is + named for its manifest id, the installer's layout, behave as before. + - `FontManager.get_font()` returns a BDF font at its native size when asked for a size the file doesn't contain (5x7.bdf at 8 or 10px, say). It used to return PIL's default font, a different typeface, so a plugin that relied on diff --git a/src/plugin_system/plugin_dirs.py b/src/plugin_system/plugin_dirs.py new file mode 100644 index 00000000..46837b58 --- /dev/null +++ b/src/plugin_system/plugin_dirs.py @@ -0,0 +1,345 @@ +""" +One answer to "which directory holds plugin X?". + +Five places used to answer it, each with its own rules and each re-parsing +every manifest per lookup: ``PluginManager`` discovery and +``get_plugin_directory``, ``PluginLoader.find_plugin_directory``, +``PluginStoreManager._find_plugin_path`` / ``list_installed_plugins`` and +``state_reconciliation.disk_plugin_ids``. The rules now live here once; what +still legitimately differs between callers (which directories to search, +whether a ``ledmatrix-`` prefix or a case difference counts as a match) is a +keyword argument at the call site, so a difference is always a visible choice +rather than an accident of which copy you read. + +The rules +--------- +* A directory is a *candidate* when it is a directory (a symlink to one counts: + dev plugins are symlinked in) and its name is neither hidden (leading ``.``) + nor carries :data:`BACKUP_MARKER`. store_manager renames a plugin aside with + that marker during install/rollback; the aside still holds a manifest, so + treating it as a plugin would resurrect a ghost. +* A plugin's id is its manifest ``id``. The directory name is only a fallback, + for callers that must still see a plugin whose manifest is missing an id. +* Resolving an id within one directory: a directory whose manifest declares + the id wins; among several, the one named exactly for the id, then + ``ledmatrix-``, then by name. Only when no manifest claims the id do + directory names count: ````, then ``ledmatrix-`` (``prefix=True``), + then either of those ignoring case (``case_insensitive=True``). The name + fallback still returns a directory whose manifest is unreadable -- that is + how a broken plugin gets uninstalled or reinstalled. ``by_manifest=False`` + (``PluginManager.get_plugin_directory``, whose discovery map already holds + the manifest answer) skips straight to the names. +* Several directories are searched one at a time, in the order given: the + first directory that resolves the id at all wins, by manifest or by name. +* The id must be one plain path segment (``safe_path_component``); anything + else resolves to nothing rather than being joined or truncated. +* Returned paths are ``search_dir / name`` and are not resolved, so a + symlinked dev plugin keeps the path that lies inside the search directory. + +Manifests are read at most once per :class:`PluginDirectoryIndex`; one index is +one scan. +""" + +from __future__ import annotations + +import json +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Dict, Iterable, List, Optional, Set, Union + +from src.common.path_safety import safe_path_component + +__all__ = [ + 'BACKUP_MARKER', + 'PLUGIN_DIR_PREFIX', + 'ManifestStatus', + 'PluginDirEntry', + 'PluginDirectoryIndex', + 'is_ignored_dir_name', + 'resolve_plugin_dir', + 'store_search_dirs', +] + +#: Substring store_manager embeds in a plugin directory it has set aside +#: (``.standalone-backup-preinstall`` / ``-migrating``). Existing debris on +#: devices carries exactly this text, so it must never change. +BACKUP_MARKER = '.standalone-backup-' + +#: Legacy repository naming (``ledmatrix-``); some installs still use it +#: as the directory name. +PLUGIN_DIR_PREFIX = 'ledmatrix-' + +PathLike = Union[str, Path] + + +class ManifestStatus: + """What reading ``manifest.json`` in a candidate directory produced.""" + OK = 'ok' # a JSON object with a non-empty "id" + MISSING = 'missing' # no manifest.json + UNREADABLE = 'unreadable' # I/O error or invalid JSON + NOT_OBJECT = 'not_object' # valid JSON, but not an object + NO_ID = 'no_id' # an object without a usable "id" + + +def is_ignored_dir_name(name: str) -> bool: + """True for names that are never a plugin: hidden, or set aside mid-install.""" + return name.startswith('.') or BACKUP_MARKER in name + + +@dataclass +class PluginDirEntry: + """One candidate directory and its manifest, read once.""" + path: Path + status: str + manifest: Optional[Any] = None + error: Optional[BaseException] = None + + @property + def name(self) -> str: + return self.path.name + + @property + def manifest_id(self) -> Optional[str]: + """The manifest's ``id`` when the manifest is usable, else None.""" + if self.status != ManifestStatus.OK: + return None + return self.manifest['id'] + + @property + def manifest_parses(self) -> bool: + """The manifest exists and is valid JSON (of any shape).""" + return self.status in (ManifestStatus.OK, ManifestStatus.NOT_OBJECT, + ManifestStatus.NO_ID) + + @property + def installed_id(self) -> str: + """The manifest id, falling back to the directory name.""" + return self.manifest_id or self.name + + +def _read_entry(path: Path) -> PluginDirEntry: + manifest_path = path / 'manifest.json' + if not manifest_path.is_file(): + return PluginDirEntry(path, ManifestStatus.MISSING) + try: + with open(manifest_path, 'r', encoding='utf-8') as handle: + manifest = json.load(handle) + except (OSError, ValueError) as exc: # ValueError covers JSON + decode errors + return PluginDirEntry(path, ManifestStatus.UNREADABLE, error=exc) + if not isinstance(manifest, dict): + return PluginDirEntry(path, ManifestStatus.NOT_OBJECT, manifest) + plugin_id = manifest.get('id') + if not plugin_id or not isinstance(plugin_id, str): + return PluginDirEntry(path, ManifestStatus.NO_ID, manifest) + return PluginDirEntry(path, ManifestStatus.OK, manifest) + + +def _preference(plugin_id: str, name: str) -> tuple: + """Sort key among directories that all claim ``plugin_id``.""" + if name == plugin_id: + rank = 0 + elif name == PLUGIN_DIR_PREFIX + plugin_id: + rank = 1 + else: + rank = 2 + return (rank, name) + + +@dataclass +class PluginDirectoryIndex: + """Every candidate directory directly under ``root``, manifests read once. + + Build one with :meth:`scan`. It is a snapshot: a directory added or + removed afterwards is not seen until the next scan. + """ + root: Path + entries: List[PluginDirEntry] = field(default_factory=list) + #: Set when ``root`` exists but could not be listed. + error: Optional[BaseException] = None + _plugins: Optional[Dict[str, PluginDirEntry]] = field( + default=None, init=False, repr=False, compare=False) + + @classmethod + def scan(cls, root: PathLike) -> 'PluginDirectoryIndex': + root = Path(root) + index = cls(root) + try: + children = sorted(root.iterdir(), key=lambda p: p.name) + except FileNotFoundError: + return index + except OSError as exc: + index.error = exc + return index + for child in children: + if is_ignored_dir_name(child.name): + continue + try: + if not child.is_dir(): + continue + except OSError: + continue + index.entries.append(_read_entry(child)) + return index + + # -- listing ---------------------------------------------------------- + + def plugins(self) -> Dict[str, PluginDirEntry]: + """Manifest id -> entry, one entry per id. + + When several directories declare the same id, the one named for it + wins, then ``ledmatrix-``, then the first by name; see + :meth:`duplicates` for the losers. + """ + if self._plugins is not None: + return self._plugins + chosen: Dict[str, PluginDirEntry] = {} + for entry in self.entries: + plugin_id = entry.manifest_id + if plugin_id is None: + continue + current = chosen.get(plugin_id) + if current is None or (_preference(plugin_id, entry.name) + < _preference(plugin_id, current.name)): + chosen[plugin_id] = entry + self._plugins = chosen + return chosen + + def duplicates(self) -> Dict[str, List[PluginDirEntry]]: + """Ids declared by more than one directory -> every such entry.""" + seen: Dict[str, List[PluginDirEntry]] = {} + for entry in self.entries: + if entry.manifest_id is not None: + seen.setdefault(entry.manifest_id, []).append(entry) + return {k: v for k, v in seen.items() if len(v) > 1} + + def installed_ids(self, *, require_parseable_manifest: bool) -> Set[str]: + """Ids of everything that counts as installed. + + A directory counts when it has a manifest.json -- which must also be + valid JSON when ``require_parseable_manifest``. Its id is the manifest + id, or the directory name when the manifest does not carry one. + """ + ids: Set[str] = set() + for entry in self.entries: + if entry.status == ManifestStatus.MISSING: + continue + if require_parseable_manifest and not entry.manifest_parses: + continue + ids.add(entry.installed_id) + return ids + + def entry_for_installed_id(self, plugin_id: str) -> Optional[PluginDirEntry]: + """The entry :meth:`installed_ids` reported as ``plugin_id``.""" + entry = self.plugins().get(plugin_id) + if entry is not None: + return entry + for entry in self.entries: + if entry.manifest_id is None and entry.name == plugin_id: + return entry + return None + + # -- lookup ----------------------------------------------------------- + + def find(self, plugin_id: str, *, prefix: bool, case_insensitive: bool, + by_manifest: bool = True) -> Optional[Path]: + """Resolve ``plugin_id`` within this directory (rules in the module doc).""" + plugin_id = _lookup_id(plugin_id) + if plugin_id is None: + return None + + if by_manifest: + entry = self.plugins().get(plugin_id) + if entry is not None: + return entry.path + + names = _candidate_names(plugin_id, prefix) + by_name = {e.name: e for e in self.entries} + for name in names: + if name in by_name: + return by_name[name].path + if case_insensitive: + for low in (n.lower() for n in names): + for entry in self.entries: + if entry.name.lower() == low: + return entry.path + return None + + +def _lookup_id(plugin_id: Any) -> Optional[str]: + """``plugin_id`` if it can name a plugin directory at all, else None.""" + plugin_id = safe_path_component(plugin_id) + if plugin_id is None or is_ignored_dir_name(plugin_id): + return None + return plugin_id + + +def _candidate_names(plugin_id: str, prefix: bool) -> List[str]: + names = [plugin_id] + if prefix: + names.append(PLUGIN_DIR_PREFIX + plugin_id) + return names + + +def _is_dir(path: Path) -> bool: + try: + return path.is_dir() + except OSError: + return False + + +def resolve_plugin_dir(plugin_id: Any, search_dirs: Iterable[PathLike], *, + prefix: bool, case_insensitive: bool = False, + by_manifest: bool = True) -> Optional[Path]: + """The directory holding ``plugin_id``, searching ``search_dirs`` in order. + + Each search directory is scanned once and each manifest in it read once. + ``by_manifest=False`` skips the manifest pass and matches directory names + only, which reads no manifests at all. + + Names are compared against the directory listing, never by probing + ``search_dir / name``: on a case-insensitive filesystem that probe says + ``Demo`` exists when the directory is ``demo``, which made the answer + depend on the platform. + """ + plugin_id = _lookup_id(plugin_id) + if plugin_id is None: + return None + for search_dir in search_dirs: + search_dir = Path(search_dir) + if by_manifest or case_insensitive: + found = PluginDirectoryIndex.scan(search_dir).find( + plugin_id, prefix=prefix, case_insensitive=case_insensitive, + by_manifest=by_manifest) + else: + found = _find_by_name(search_dir, _candidate_names(plugin_id, prefix)) + if found is not None: + return found + return None + + +def _find_by_name(search_dir: Path, names: List[str]) -> Optional[Path]: + try: + present = {child.name for child in search_dir.iterdir()} + except OSError: + return None + for name in names: + if name in present and _is_dir(search_dir / name): + return search_dir / name + return None + + +def store_search_dirs(plugins_dir: PathLike) -> List[Path]: + """Directories the plugin store searches: the configured one, then a + sibling ``plugins/`` (the legacy/dev location) when that is a different + directory. Discovery deliberately does NOT use this -- it scans only the + configured directory (see CLAUDE.md, test_discovery_path_contract.py).""" + plugins_dir = Path(plugins_dir) + dirs = [plugins_dir] + try: + base = plugins_dir if plugins_dir.is_absolute() else plugins_dir.resolve() + sibling = base.parent / 'plugins' + if sibling != base: + dirs.append(sibling) + except (OSError, ValueError): + pass + return dirs diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 61e80afc..66feabab 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -8,7 +8,6 @@ Extracted from PluginManager to improve separation of concerns. import importlib import importlib.metadata import importlib.util -import json import os import sys import subprocess @@ -21,6 +20,7 @@ from packaging.requirements import InvalidRequirement, Requirement from src.exceptions import PluginError from src.logging_config import get_logger +from src.plugin_system.plugin_dirs import resolve_plugin_dir def requirements_has_real_deps(requirements_file: str) -> bool: @@ -214,85 +214,36 @@ class PluginLoader: ) -> Optional[Path]: """ Find the plugin directory for a given plugin ID. - - Tries multiple strategies: - 1. Use plugin_directories mapping if available - 2. Direct path matching - 3. Case-insensitive directory matching - 4. Manifest-based search - + + 1. The discovery mapping, when it has the id and the path exists. + 2. ``plugins_dir`` only, by the shared rules in + ``src/plugin_system/plugin_dirs.py``: a directory whose manifest + declares the id wins; otherwise ```` or ``ledmatrix-``, + matched case-insensitively. Backup and hidden directories are + never matched. + Args: plugin_id: Plugin identifier plugins_dir: Base plugins directory plugin_directories: Optional mapping of plugin_id to directory - - Returns: - Path to plugin directory or None if not found - """ - # Sanitize plugin_id — os.path.basename is a CodeQL-recognized path sanitizer - plugin_id = os.path.basename(plugin_id or '') - if not plugin_id: - return None + Returns: + Path to plugin directory or None if not found. An id that is not + one plain path segment finds nothing. + """ # Strategy 1: Use mapping from discovery if plugin_directories and plugin_id in plugin_directories: plugin_dir = plugin_directories[plugin_id] if plugin_dir.exists(): self.logger.debug("Using plugin directory from discovery mapping: %s", plugin_dir) return plugin_dir - - # Strategy 2: Direct paths — resolve and validate they stay within plugins_dir - plugins_dir_resolved = plugins_dir.resolve() - for _candidate_name in (plugin_id, f"ledmatrix-{plugin_id}"): - _candidate = (plugins_dir_resolved / _candidate_name).resolve() - try: - _candidate.relative_to(plugins_dir_resolved) - except ValueError: - continue - if _candidate.exists(): - return _candidate - - # Strategy 3: Case-insensitive search - normalized_id = plugin_id.lower() - for item in plugins_dir.iterdir(): - if not item.is_dir(): - continue - - item_name = item.name - if item_name.lower() == normalized_id: - return item - - if item_name.lower() == f"ledmatrix-{plugin_id}".lower(): - return item - - # Strategy 4: Manifest-based search - self.logger.debug("Directory name search failed for %s, searching by manifest...", plugin_id) - for item in plugins_dir.iterdir(): - if not item.is_dir(): - continue - - # Skip if already checked - if item.name.lower() == normalized_id or item.name.lower() == f"ledmatrix-{plugin_id}".lower(): - continue - - manifest_path = item / "manifest.json" - if manifest_path.exists(): - try: - with open(manifest_path, 'r', encoding='utf-8') as f: - item_manifest = json.load(f) - item_manifest_id = item_manifest.get('id') - if item_manifest_id == plugin_id: - self.logger.info( - "Found plugin %s in directory %s (manifest ID matches)", - plugin_id, - item.name - ) - return item - except (json.JSONDecodeError, Exception) as e: - self.logger.debug("Skipping %s due to manifest error: %s", item.name, e) - continue - return None + plugin_dir = resolve_plugin_dir( + plugin_id, [plugins_dir], prefix=True, case_insensitive=True) + if plugin_dir is not None and plugin_dir.name != plugin_id: + self.logger.debug("Found plugin %s in directory %s", + plugin_id, plugin_dir.name) + return plugin_dir def install_dependencies( self, diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 27502f02..866e99eb 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -25,7 +25,9 @@ from src.plugin_system.plugin_state import PluginStateManager, PluginState from src.plugin_system.schema_manager import ( CORE_VEGAS_TUNING_KEYS, SchemaManager, normalize_legacy_booleans, ) -from src.common.path_safety import safe_path_component +from src.plugin_system.plugin_dirs import ( + ManifestStatus, PluginDirectoryIndex, resolve_plugin_dir, +) from src.deprecation import deprecated from src.common.permission_utils import ( ensure_directory_permissions, @@ -175,89 +177,88 @@ class PluginManager: self.logger.error("Could not create plugins directory %s: %s", self.plugins_dir, e, exc_info=True) raise PluginError(f"Could not create plugins directory: {self.plugins_dir}", context={'error': str(e)}) from e + def _report_skip_once(self, key: str, message: str, *args: Any) -> None: + """Warn about a skipped directory once per process, not per scan. + + 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. + """ + reported = self.__dict__.setdefault('_skip_reported', set()) + if key in reported: + return + reported.add(key) + self.logger.warning(message, *args) + def _scan_directory_for_plugins(self, directory: Path) -> List[str]: """ Scan a directory for plugins. + Which directories count and how an id maps to one is decided by + :class:`PluginDirectoryIndex` (``src/plugin_system/plugin_dirs.py``), + shared with the loader, the store and reconciliation. Only + ``directory`` is scanned: discovery has no fallback to ``plugins/``. + Directories set aside mid-install (``BACKUP_MARKER`` in the name) are + skipped so they don't overwrite live entries. + Args: directory: Directory to scan Returns: List of plugin IDs found """ - plugin_ids = [] - if not directory.exists(): - return plugin_ids + return [] # Build new state locally before acquiring lock - new_manifests: Dict[str, Dict[str, Any]] = {} - new_directories: Dict[str, Path] = {} - - try: - for item in directory.iterdir(): - if not item.is_dir(): - continue - # Skip backup directories so they don't overwrite live entries - if '.standalone-backup-' in item.name: - continue - - manifest_path = item / "manifest.json" - 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 + index = PluginDirectoryIndex.scan(directory) + if index.error is not None: + self.logger.error("Error scanning directory %s: %s", directory, + index.error, exc_info=index.error) + for entry in index.entries: + if entry.status == ManifestStatus.MISSING: + # 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. + self._report_skip_once( + entry.name, "Skipping %s: no manifest.json, so it cannot be " + "loaded as a plugin", entry.name) + elif entry.status == ManifestStatus.UNREADABLE: + self.logger.warning("Error reading manifest from %s: %s", + entry.path / "manifest.json", entry.error, + exc_info=entry.error) + elif entry.status == ManifestStatus.NOT_OBJECT: # json.load accepts any JSON value, so a manifest holding - # null, [] or "text" parses and then raises AttributeError on - # .get(). Nothing here catches that -- the outer handler takes - # OSError/PermissionError only -- so a single malformed - # manifest aborted the whole scan and every other plugin on + # null, [] or "text" parses. It once raised AttributeError on + # .get() and aborted the whole scan, so every other plugin on # disk, however healthy, silently failed to register. - if not isinstance(manifest, dict): - if item.name not in self._skip_reported: - self._skip_reported.add(item.name) - self.logger.warning( - "Skipping %s: its manifest.json is %s, not a JSON " - "object", item.name, type(manifest).__name__) - continue + self._report_skip_once( + entry.name, "Skipping %s: its manifest.json is %s, not a " + "JSON object", entry.name, type(entry.manifest).__name__) + elif entry.status == ManifestStatus.NO_ID: + # Parsed but unusable. This was the quietest path of all: the + # manifest is read successfully and then dropped. + self._report_skip_once( + entry.name, "Skipping %s: its manifest.json has no \"id\", " + "so there is nothing to register it under", entry.name) - 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 + plugins = index.plugins() + for plugin_id, entries in index.duplicates().items(): + self._report_skip_once( + "duplicate:" + plugin_id, + "Plugin id %r is declared by %d directories (%s); using %s", + plugin_id, len(entries), ", ".join(e.name for e in entries), + plugins[plugin_id].name) - 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) + new_manifests: Dict[str, Dict[str, Any]] = { + plugin_id: entry.manifest for plugin_id, entry in plugins.items()} + new_directories: Dict[str, Path] = { + plugin_id: entry.path for plugin_id, entry in plugins.items()} # Replace shared state under lock so uninstalled plugins don't linger with self._discovery_lock: @@ -266,8 +267,8 @@ class PluginManager: self.plugin_directories.clear() self.plugin_directories.update(new_directories) - return plugin_ids - + return list(plugins) + def discover_plugins(self) -> List[str]: """ Discover all plugins in the plugins directory. @@ -772,24 +773,20 @@ class PluginManager: not one plain path segment (``..``, ``a/b``, an absolute path) is refused instead of being joined onto ``plugins_dir``. The join is not resolved further: dev plugins are symlinks into ``plugins_dir``. + + The discovery map is authoritative. For an id discovery has not seen, + only directory names are tried -- ```` then ``ledmatrix-``, + in ``plugins_dir`` only -- so a miss on a web request never reads + every manifest on disk. Rules: ``src/plugin_system/plugin_dirs.py``. """ with self._discovery_lock: if plugin_id in self.plugin_directories: return str(self.plugin_directories[plugin_id]) - plugin_id = safe_path_component(plugin_id) - if plugin_id is None: - return None - - plugin_dir = self.plugins_dir / plugin_id - if plugin_dir.exists(): - return str(plugin_dir) - - plugin_dir = self.plugins_dir / f"ledmatrix-{plugin_id}" - if plugin_dir.exists(): - return str(plugin_dir) - - return None + plugin_dir = resolve_plugin_dir( + plugin_id, [self.plugins_dir], prefix=True, case_insensitive=False, + by_manifest=False) + return str(plugin_dir) if plugin_dir is not None else None def get_plugin_display_modes(self, plugin_id: str) -> List[str]: """ diff --git a/src/plugin_system/state_reconciliation.py b/src/plugin_system/state_reconciliation.py index 0359e460..865b8425 100644 --- a/src/plugin_system/state_reconciliation.py +++ b/src/plugin_system/state_reconciliation.py @@ -15,6 +15,7 @@ from enum import Enum from pathlib import Path from src.core_config_keys import CORE_CONFIG_KEYS +from src.plugin_system.plugin_dirs import PluginDirectoryIndex from src.plugin_system.state_manager import PluginStateManager from src.logging_config import get_logger @@ -102,31 +103,25 @@ def config_plugin_ids(config: Dict[str, Any], ignored_keys: Set[str]) -> Set[str 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. + A directory counts only when it is not a standalone backup (or hidden) + 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. + + The id is the manifest's ``id`` -- what discovery registers and what the + config is keyed by -- and the directory name only when the manifest has + none. Directory names alone made a plugin living in ``ledmatrix-stocks/`` + with id ``stocks`` read as both "stocks in config but not on disk" and + "ledmatrix-stocks on disk but not in config". """ - 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) + return _disk_index(plugins_dir).installed_ids(require_parseable_manifest=True) except OSError: - return ids - return ids + return set() + + +def _disk_index(plugins_dir) -> PluginDirectoryIndex: + return PluginDirectoryIndex.scan(Path(plugins_dir)) def still_unresolved(entries: List[Dict[str, Any]], @@ -326,16 +321,15 @@ class StateReconciliation: """Get plugin state from disk (installed plugins).""" state = {} try: - # 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 + # Membership uses the same index and rule as disk_plugin_ids, so + # the web interface re-checks stored findings against this same + # definition; each manifest is read once, by the scan. + index = _disk_index(self.plugins_dir) + for plugin_id in index.installed_ids(require_parseable_manifest=True): + entry = index.entry_for_installed_id(plugin_id) + manifest = entry.manifest if entry is not None else None + if not isinstance(manifest, dict): + manifest = {} state[plugin_id] = { 'exists_on_disk': True, 'version': manifest.get('version'), diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 12e74212..5e543b1e 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -28,6 +28,9 @@ from src.common.permission_utils import sudo_remove_directory, install_requireme from src.plugin_system.plugin_loader import ( requirements_has_real_deps, requirements_are_satisfied, find_trusted_subdir ) +from src.plugin_system.plugin_dirs import ( + BACKUP_MARKER, PluginDirectoryIndex, resolve_plugin_dir, store_search_dirs, +) try: from jsonschema import Draft7Validator, ValidationError @@ -1233,9 +1236,9 @@ class PluginStoreManager: Pass-through when nothing is installed, and when called from `_reinstall_with_rollback`, which has already moved the old copy aside. - The aside name embeds '.standalone-backup-' so plugin discovery - (`plugin_manager._scan_directory_for_plugins`) skips it even though it - still holds a manifest.json. + The aside name embeds BACKUP_MARKER ('.standalone-backup-') so every + plugin directory lookup (src/plugin_system/plugin_dirs.py) skips it + even though it still holds a manifest.json. Held under the per-plugin reinstall lock for the same reason `_reinstall_with_rollback` is: the web UI runs Flask with @@ -1250,7 +1253,7 @@ class PluginStoreManager: return self._install_plugin_impl(plugin_id, branch) backup_path = plugin_path.with_name( - f"{plugin_path.name}.standalone-backup-preinstall") + f"{plugin_path.name}{BACKUP_MARKER}preinstall") if backup_path.exists() and not self._safe_remove_directory(backup_path): # Can't stage a safety net. Better to attempt the install than # to refuse outright, which is what callers got before this @@ -2393,95 +2396,43 @@ class PluginStoreManager: def _find_plugin_path(self, plugin_id: str) -> Optional[Path]: """ Find the plugin path by checking the configured directory and standard plugins directory. - + + Searches the configured directory, then a sibling ``plugins/`` (the + case where plugins sit in plugins/ but config says plugin-repos/) -- + a store-only fallback; discovery scans the configured directory only. + Each directory is searched completely before the next, by the shared + rules in ``src/plugin_system/plugin_dirs.py``: a directory whose + manifest declares the id wins, then a directory named exactly for it. + + The manifest match matters because a directory name can differ from + the id its manifest declares (a hand-made or legacy layout such as + `ledmatrix-stocks/` holding id `stocks`); a lookup by directory name + alone reported such a plugin as not installed, so update_plugin() + silently did nothing. + + No ``ledmatrix-`` prefix and no case folding here, unlike the loader: + a store operation may delete what this returns, so it only accepts a + directory that names the id exactly or declares it. Note that this + leaves registry ids like `stocks` unresolved when the installed + plugin is `ledmatrix-stocks/` declaring `ledmatrix-stocks` (the + monorepo's leaderboard, music, stocks and weather); passing + ``prefix=True`` would resolve them, but update_plugin()'s reinstall + path has not been checked against that yet. + Args: plugin_id: Plugin identifier - + Returns: Path to plugin directory if found, None otherwise """ - # First check the configured plugins directory - plugin_path = self.plugins_dir / plugin_id - if plugin_path.exists(): - return plugin_path - - # Also check the standard 'plugins/' directory if it's different - # This handles the case where plugins are in plugins/ but config says plugin-repos/ - try: - if self.plugins_dir.is_absolute(): - project_root = self.plugins_dir.parent - else: - project_root = self.plugins_dir.resolve().parent - - standard_plugins_dir = project_root / 'plugins' - if standard_plugins_dir.exists() and standard_plugins_dir != self.plugins_dir: - plugin_path = standard_plugins_dir / plugin_id - if plugin_path.exists(): - return plugin_path - except (OSError, ValueError): - pass - - # Last resort: the directory name may differ from the id being looked - # up. install_plugin() deliberately renames a plugin's directory to the - # MANIFEST id when it differs from the REGISTRY id (see the rename near - # "doesn't match registry ID" above), so `stocks` in the registry lands - # in `ledmatrix-stocks/`. Every lookup above is by directory name, so - # update_plugin("stocks") found nothing and reported the plugin as not - # installed -- silently, and for good: the user sees no error and stays - # on a stale version. Four installed plugins hit this in practice - # (leaderboard, music, stocks, weather). - # - # Deliberately last so the two lookups above keep their exact meaning; - # this only runs when a direct hit already failed. See - # test_discovery_path_contract.py, which pins that ordering. - for search_dir in self._candidate_plugin_dirs(): - match = self._find_by_manifest_id(search_dir, plugin_id) - if match is not None: - self.logger.debug( - "Resolved plugin '%s' to %s via its manifest id " - "(directory name differs from the id)", plugin_id, match) - return match - - return None + return resolve_plugin_dir( + plugin_id, self._candidate_plugin_dirs(), prefix=False, + case_insensitive=False) def _candidate_plugin_dirs(self) -> List[Path]: """Directories that may hold installed plugins, configured one first.""" - dirs = [self.plugins_dir] - try: - base = self.plugins_dir if self.plugins_dir.is_absolute() else self.plugins_dir.resolve() - sibling = base.parent / 'plugins' - if sibling != self.plugins_dir: - dirs.append(sibling) - except (OSError, ValueError): - pass - return [d for d in dirs if d.exists()] + return [d for d in store_search_dirs(self.plugins_dir) if d.exists()] - @staticmethod - def _find_by_manifest_id(search_dir: Path, plugin_id: str) -> Optional[Path]: - """A subdirectory of `search_dir` whose manifest declares `plugin_id`. - - Skips half-finished installs: store_manager renames a directory aside - with '.standalone-backup-' during install and rollback, and treating - one as installed would resurrect a ghost plugin. - """ - try: - entries = sorted(search_dir.iterdir()) - except (OSError, ValueError): - return None - for entry in entries: - if not entry.is_dir() or '.standalone-backup-' in entry.name: - continue - manifest = entry / 'manifest.json' - if not manifest.is_file(): - continue - try: - with open(manifest, 'r', encoding='utf-8') as handle: - if json.load(handle).get('id') == plugin_id: - return entry - except (OSError, ValueError): - continue - return None - def uninstall_plugin(self, plugin_id: str) -> bool: """ Uninstall a plugin by removing its directory. @@ -2600,9 +2551,9 @@ class PluginStoreManager: field during the monorepo migration on a Pi with broken DNS — every old-remote plugin was deleted and none could be re-downloaded). - The aside name embeds '.standalone-backup-' so plugin discovery - (plugin_manager._scan_directory_for_plugins) ignores it even though - it still contains a manifest.json. + The aside name embeds BACKUP_MARKER ('.standalone-backup-') so every + plugin directory lookup (src/plugin_system/plugin_dirs.py) ignores it + even though it still contains a manifest.json. Held for the whole operation under a per-plugin_id lock: two overlapping requests for the same plugin (double-click, two @@ -2612,7 +2563,7 @@ class PluginStoreManager: """ with self._get_reinstall_lock(plugin_id): backup_path = plugin_path.with_name( - f"{plugin_path.name}.standalone-backup-migrating") + f"{plugin_path.name}{BACKUP_MARKER}migrating") # A stale aside from a previous crash would block the rename if backup_path.exists(): if not self._safe_remove_directory(backup_path): @@ -3084,16 +3035,16 @@ class PluginStoreManager: def list_installed_plugins(self) -> List[str]: """ Get list of installed plugin IDs. - + + One entry per plugin directory in the configured directory that has a + manifest.json, named by the manifest's id (the directory name when + the manifest carries none, e.g. because it does not parse). Backup + and hidden directories are not plugins. + Returns: - List of plugin IDs + List of plugin IDs, sorted """ if not self.plugins_dir.exists(): return [] - - installed = [] - for item in self.plugins_dir.iterdir(): - if item.is_dir() and (item / "manifest.json").exists(): - installed.append(item.name) - - return installed + index = PluginDirectoryIndex.scan(self.plugins_dir) + return sorted(index.installed_ids(require_parseable_manifest=False)) diff --git a/test/test_discovery_path_contract.py b/test/test_discovery_path_contract.py index 6e8e96ca..f10e50a0 100644 --- a/test/test_discovery_path_contract.py +++ b/test/test_discovery_path_contract.py @@ -16,9 +16,13 @@ change to the fallback chains is a deliberate one. The `.standalone-backup-` contract: store_manager renames a plugin dir aside with that substring during install/rollback; discovery MUST skip such dirs -or a half-finished install would surface a ghost plugin. The substring is -duplicated as a literal in both files — this test breaks if either side -changes it unilaterally. +or a half-finished install would surface a ghost plugin. The substring now +lives once, as plugin_dirs.BACKUP_MARKER, which both sides import; its value +is pinned because debris already on devices carries exactly that text. + +All of them now resolve through src/plugin_system/plugin_dirs.py; the +per-caller differences pinned here are explicit arguments there. The rules +themselves are covered table-style in test_plugin_dirs.py. """ import json @@ -198,14 +202,16 @@ class TestStandaloneBackupContract: found = _scanner()._scan_directory_for_plugins(plugins_dir) assert found == ["real-plugin"] - def test_backup_substring_literal_matches_across_files(self): - """The substring is duplicated in plugin_manager (skip check) and - store_manager (rename-aside names). If either side changes it, the - other silently stops honoring the contract — this test is the - tripwire.""" + def test_backup_marker_is_shared_and_unchanged(self): + """store_manager (rename-aside names) and every lookup (skip check) + must agree on the marker. Both now import one constant; the value is + pinned because renaming it would make existing debris on devices + visible as plugins again.""" + from src.plugin_system import plugin_dirs + assert plugin_dirs.BACKUP_MARKER == '.standalone-backup-' root = Path(__file__).resolve().parents[1] - pm_text = (root / "src/plugin_system/plugin_manager.py").read_text() - sm_text = (root / "src/plugin_system/store_manager.py").read_text() - assert "'.standalone-backup-'" in pm_text.replace('"', "'") - assert ".standalone-backup-" in sm_text - + sm_text = (root / "src/plugin_system/store_manager.py").read_text(encoding="utf-8") + assert "{BACKUP_MARKER}preinstall" in sm_text + assert "{BACKUP_MARKER}migrating" in sm_text + assert plugin_dirs.is_ignored_dir_name( + "demo" + plugin_dirs.BACKUP_MARKER + "preinstall") diff --git a/test/test_plugin_dirs.py b/test/test_plugin_dirs.py new file mode 100644 index 00000000..f80d5e5d --- /dev/null +++ b/test/test_plugin_dirs.py @@ -0,0 +1,343 @@ +""" +Every "which directory holds plugin X?" answer, from one tree, per caller. + +src/plugin_system/plugin_dirs.py holds the rules; the callers differ only in +explicit arguments (search dirs, ``ledmatrix-`` prefix, case folding, whether +the manifest pass runs). This file builds one project tree that exercises +every rule and pins each caller's answer for each id, so a change to either +the shared rules or a caller's arguments shows up as a table row. + +Callers: + discovery PluginManager._scan_directory_for_plugins -> plugin_directories + pm_get PluginManager.get_plugin_directory before discovery has run + loader PluginLoader.find_plugin_directory (no discovery mapping) + store PluginStoreManager._find_plugin_path +""" + +import json +import logging +import os +import sys +import threading +from pathlib import Path + +import pytest + +from src.plugin_system import plugin_dirs +from src.plugin_system.plugin_dirs import ( + ManifestStatus, PluginDirectoryIndex, resolve_plugin_dir, +) +from src.plugin_system.plugin_loader import PluginLoader +from src.plugin_system.plugin_manager import PluginManager +from src.plugin_system.state_reconciliation import ( + StateReconciliation, disk_plugin_ids, +) +from src.plugin_system.store_manager import PluginStoreManager + +CONFIGURED = "plugin-repos" +SIBLING = "plugins" + + +def _write(base: Path, dir_name: str, manifest) -> Path: + d = base / dir_name + d.mkdir(parents=True) + if manifest is not None: + text = manifest if isinstance(manifest, str) else json.dumps(manifest) + (d / "manifest.json").write_text(text, encoding="utf-8") + return d + + +def _plugin(base: Path, dir_name: str, plugin_id: str, **extra) -> Path: + return _write(base, dir_name, dict({"id": plugin_id, "name": plugin_id, + "version": "1.0.0"}, **extra)) + + +def _link_dir(link: Path, target: Path) -> None: + """A symlink where the OS allows one; on Windows without the privilege, + a directory junction, which the code under test sees the same way + (is_dir() follows it, iterdir() lists it under the link's name).""" + try: + os.symlink(target, link, target_is_directory=True) + except OSError: + if sys.platform != "win32": + raise + import _winapi + _winapi.CreateJunction(str(target), str(link)) + + +@pytest.fixture +def tree(tmp_path): + repos = tmp_path / CONFIGURED + legacy = tmp_path / SIBLING + repos.mkdir() + legacy.mkdir() + + # configured dir (plugin-repos/) + _plugin(repos, "exact", "exact") # id == dir name + _plugin(repos, "ledmatrix-stocks", "stocks") # manifest id != dir name + _plugin(repos, "ledmatrix-legacy", "ledmatrix-legacy") # prefix only by name + _plugin(repos, "MixedCase", "MixedCase") # case differences + _plugin(repos, "renamed-dir", "other-id") # dir name belongs to no id + _plugin(repos, "shadow", "not-shadow") # name says one id ... + _plugin(repos, "real-shadow", "shadow") # ... manifest says it's here + _plugin(repos, "ghost.standalone-backup-preinstall", "ghost") # set aside + _plugin(repos, "exact.standalone-backup-migrating", "exact") # set aside, dup id + _plugin(repos, ".hidden", "hidden") # hidden / staging + _plugin(repos, "zz-dupe", "dupe") # duplicate ids: + _plugin(repos, "ledmatrix-dupe", "dupe") # prefix beats other, + _plugin(repos, "dupe", "dupe") # exact beats prefix + _write(repos, "broken", "{ not json") # unreadable manifest + _write(repos, "noid", {"name": "No id"}) # parses, no id + _write(repos, "listy", [1, 2]) # parses, not an object + _write(repos, "nomanifest", None) # not a plugin + _plugin(repos, "both", "both") # also in plugins/ + _plugin(repos, "ledmatrix-weather", "weather") # manifest hit here vs + (repos / "README.md").write_text("not a dir") # a file, never a match + + dev_target = _plugin(tmp_path / "dev-checkouts", "devplug-src", "devplug") + _link_dir(repos / "dev-link", dev_target) # symlinked dev plugin + + # sibling dir (plugins/): store fallback only + _plugin(legacy, "both", "both") + _plugin(legacy, "legacy-only", "legacy-only") + _plugin(legacy, "ledmatrix-sibling", "sibling") + _plugin(legacy, "weather", "weather") # ... a name hit here + return tmp_path + + +def _discovery(root: Path) -> PluginManager: + pm = object.__new__(PluginManager) + pm.logger = logging.getLogger("test_plugin_dirs") + pm._discovery_lock = threading.RLock() + pm._skip_reported = set() + pm.plugin_manifests = {} + pm.plugin_directories = {} + pm.plugins_dir = root / CONFIGURED + return pm + + +def _answers(root: Path, plugin_id: str) -> dict: + repos = root / CONFIGURED + discovered = _discovery(root) + discovered._scan_directory_for_plugins(repos) + fresh = _discovery(root) # discovery not run: exercises the disk rules + store = PluginStoreManager(plugins_dir=str(repos), + uninstalled_registry_path=str(root / "u.json")) + + def rel(p): + return None if p is None else Path(p).relative_to(root).as_posix() + + return { + "discovery": rel(discovered.plugin_directories.get(plugin_id)), + "pm_get": rel(fresh.get_plugin_directory(plugin_id)), + "loader": rel(PluginLoader().find_plugin_directory(plugin_id, repos)), + "store": rel(store._find_plugin_path(plugin_id)), + } + + +R = CONFIGURED + "/" +S = SIBLING + "/" + +# id discovery pm_get loader store +TABLE = [ + ("exact", R + "exact", R + "exact", R + "exact", R + "exact"), + # manifest id != dir name: the manifest finds it; pm_get by prefix + ("stocks", R + "ledmatrix-stocks", R + "ledmatrix-stocks", + R + "ledmatrix-stocks", R + "ledmatrix-stocks"), + # ledmatrix- prefix: name-only callers with prefix=True; the store has none + ("legacy", None, R + "ledmatrix-legacy", R + "ledmatrix-legacy", None), + ("ledmatrix-legacy", R + "ledmatrix-legacy", R + "ledmatrix-legacy", + R + "ledmatrix-legacy", R + "ledmatrix-legacy"), + # case: only the loader folds case + ("mixedcase", None, None, R + "MixedCase", None), + ("MixedCase", R + "MixedCase", R + "MixedCase", R + "MixedCase", R + "MixedCase"), + # a directory name no manifest claims still resolves by name + ("renamed-dir", None, R + "renamed-dir", R + "renamed-dir", R + "renamed-dir"), + ("other-id", R + "renamed-dir", None, R + "renamed-dir", R + "renamed-dir"), + # manifest id wins over directory name (pm_get has no manifest pass) + ("shadow", R + "real-shadow", R + "shadow", R + "real-shadow", R + "real-shadow"), + # backups and hidden dirs are never plugins, by id or by name + ("ghost", None, None, None, None), + ("ghost.standalone-backup-preinstall", None, None, None, None), + ("hidden", None, None, None, None), + (".hidden", None, None, None, None), + # duplicate ids: exact name, then ledmatrix-, then by name + ("dupe", R + "dupe", R + "dupe", R + "dupe", R + "dupe"), + # unreadable manifest: found by name so it can be repaired/removed + ("broken", None, R + "broken", R + "broken", R + "broken"), + ("noid", None, R + "noid", R + "noid", R + "noid"), + ("nomanifest", None, R + "nomanifest", R + "nomanifest", R + "nomanifest"), + ("README.md", None, None, None, None), + # symlinked dev plugin: found through the link, path kept inside the dir + ("devplug", R + "dev-link", None, R + "dev-link", R + "dev-link"), + ("dev-link", None, R + "dev-link", R + "dev-link", R + "dev-link"), + # search order: only the store looks in plugins/, and configured first + ("both", R + "both", R + "both", R + "both", R + "both"), + ("legacy-only", None, None, None, S + "legacy-only"), + ("sibling", None, None, None, S + "ledmatrix-sibling"), + # configured dir searched completely (manifest hit) before plugins/ (name hit) + ("weather", R + "ledmatrix-weather", R + "ledmatrix-weather", + R + "ledmatrix-weather", R + "ledmatrix-weather"), + # not one plain path segment: nothing, never a join or a truncation + ("../plugins/both", None, None, None, None), + ("plugin-repos/exact", None, None, None, None), + ("", None, None, None, None), +] + + +@pytest.mark.parametrize("plugin_id,discovery,pm_get,loader,store", TABLE, + ids=[row[0] or "" for row in TABLE]) +def test_each_caller_resolves_each_id(tree, plugin_id, discovery, pm_get, loader, store): + assert _answers(tree, plugin_id) == { + "discovery": discovery, "pm_get": pm_get, "loader": loader, "store": store, + } + + +class TestListings: + def test_discovery_registers_manifest_ids_only(self, tree): + pm = _discovery(tree) + found = pm._scan_directory_for_plugins(tree / CONFIGURED) + assert sorted(found) == sorted([ + "exact", "stocks", "ledmatrix-legacy", "MixedCase", "other-id", + "not-shadow", "shadow", "dupe", "both", "weather", "devplug", + ]) + assert len(found) == len(set(found)), "a duplicate id was listed twice" + assert set(pm.plugin_manifests) == set(found) + + def test_store_lists_every_dir_with_a_manifest(self, tree): + store = PluginStoreManager(plugins_dir=str(tree / CONFIGURED), + uninstalled_registry_path=str(tree / "u.json")) + assert store.list_installed_plugins() == sorted([ + "exact", "stocks", "ledmatrix-legacy", "MixedCase", "other-id", + "not-shadow", "shadow", "dupe", "both", "weather", "devplug", + # manifest present but no usable id: listed by directory name + "broken", "noid", "listy", + ]) + + def test_reconciliation_counts_parseable_manifests(self, tree): + assert disk_plugin_ids(tree / CONFIGURED) == { + "exact", "stocks", "ledmatrix-legacy", "MixedCase", "other-id", + "not-shadow", "shadow", "dupe", "both", "weather", "devplug", + "noid", "listy", + } + + def test_reconciliation_disk_state_is_keyed_like_config(self, tree): + recon = object.__new__(StateReconciliation) + recon.plugins_dir = tree / CONFIGURED + recon.logger = logging.getLogger("test_plugin_dirs") + state = recon._get_disk_state() + assert state["stocks"] == {"exists_on_disk": True, "version": "1.0.0", + "name": "stocks"} + assert "ledmatrix-stocks" not in state + # A manifest that is valid JSON but not an object used to abort the + # whole disk state with AttributeError. + assert state["listy"] == {"exists_on_disk": True, "version": None, "name": None} + + def test_all_listings_skip_backups_and_hidden(self, tree): + store = PluginStoreManager(plugins_dir=str(tree / CONFIGURED), + uninstalled_registry_path=str(tree / "u.json")) + pm = _discovery(tree) + listings = { + "discovery": set(pm._scan_directory_for_plugins(tree / CONFIGURED)), + "store": set(store.list_installed_plugins()), + "reconciliation": disk_plugin_ids(tree / CONFIGURED), + } + for name, ids in listings.items(): + assert not {"ghost", "hidden", ".hidden"} & ids, name + assert not any(plugin_dirs.BACKUP_MARKER in i for i in ids), name + # the backup of `exact` did not replace the live one + assert pm.plugin_directories["exact"] == tree / CONFIGURED / "exact" + + +class TestIndex: + def test_each_manifest_is_read_once_per_scan(self, tree, monkeypatch): + reads = [] + real = plugin_dirs._read_entry + monkeypatch.setattr(plugin_dirs, "_read_entry", + lambda p: reads.append(p.name) or real(p)) + index = PluginDirectoryIndex.scan(tree / CONFIGURED) + for plugin_id in ("exact", "stocks", "dupe", "shadow", "nope"): + index.find(plugin_id, prefix=True, case_insensitive=True) + index.plugins() + index.installed_ids(require_parseable_manifest=True) + assert len(reads) == len(set(reads)) == len(index.entries) + + def test_manifest_statuses(self, tree): + index = PluginDirectoryIndex.scan(tree / CONFIGURED) + status = {e.name: e.status for e in index.entries} + assert status["exact"] == ManifestStatus.OK + assert status["broken"] == ManifestStatus.UNREADABLE + assert status["noid"] == ManifestStatus.NO_ID + assert status["listy"] == ManifestStatus.NOT_OBJECT + assert status["nomanifest"] == ManifestStatus.MISSING + assert "README.md" not in status + + def test_duplicates_are_reported_with_every_claimant(self, tree): + dupes = PluginDirectoryIndex.scan(tree / CONFIGURED).duplicates() + assert sorted(e.name for e in dupes["dupe"]) == \ + ["dupe", "ledmatrix-dupe", "zz-dupe"] + # the backup of `exact` is not a claimant + assert "exact" not in dupes + + def test_duplicate_preference_without_an_exact_name(self, tmp_path): + _plugin(tmp_path, "zz-dupe", "dupe") + _plugin(tmp_path, "aa-dupe", "dupe") + _plugin(tmp_path, "ledmatrix-dupe", "dupe") + assert resolve_plugin_dir("dupe", [tmp_path], prefix=False) == \ + tmp_path / "ledmatrix-dupe" + (tmp_path / "ledmatrix-dupe" / "manifest.json").unlink() + assert resolve_plugin_dir("dupe", [tmp_path], prefix=False) == \ + tmp_path / "aa-dupe" + + def test_exact_name_beats_prefix_even_when_it_sorts_later(self, tmp_path): + _plugin(tmp_path, "ledmatrix-zeta", "zeta") + _plugin(tmp_path, "zeta", "zeta") + index = PluginDirectoryIndex.scan(tmp_path) + assert index.plugins()["zeta"].name == "zeta" + assert resolve_plugin_dir("zeta", [tmp_path], prefix=True) == tmp_path / "zeta" + + @pytest.mark.parametrize("bad", [None, 5, b"exact", ["exact"]]) + def test_non_string_ids_resolve_to_nothing(self, tree, bad): + for kwargs in ({"prefix": True}, {"prefix": True, "by_manifest": False}, + {"prefix": False, "case_insensitive": True}): + assert resolve_plugin_dir(bad, [tree / CONFIGURED], **kwargs) is None + + def test_missing_search_dir_is_empty_not_an_error(self, tmp_path): + index = PluginDirectoryIndex.scan(tmp_path / "absent") + assert index.entries == [] and index.error is None + assert resolve_plugin_dir("x", [tmp_path / "absent"], prefix=True) is None + + def test_discovery_warns_once_about_a_duplicate(self, tree, caplog): + pm = _discovery(tree) + with caplog.at_level(logging.WARNING, logger="test_plugin_dirs"): + for _ in range(3): + pm._scan_directory_for_plugins(tree / CONFIGURED) + hits = [r for r in caplog.records if "'dupe'" in r.getMessage()] + assert len(hits) == 1 + assert "zz-dupe" in hits[0].getMessage() + + +class TestAutoUpdateUsesManifestIds: + def test_update_targets_manifest_id_and_its_directory(self, tree, monkeypatch): + from web_interface import auto_update + + store = PluginStoreManager(plugins_dir=str(tree / CONFIGURED), + uninstalled_registry_path=str(tree / "u.json")) + calls = [] + + def fake_update(plugin_id): + calls.append(plugin_id) + if plugin_id == "stocks": + path = tree / CONFIGURED / "ledmatrix-stocks" / "manifest.json" + m = json.loads(path.read_text(encoding="utf-8")) + m["version"] = "2.0.0" + path.write_text(json.dumps(m), encoding="utf-8") + return True + + monkeypatch.setattr(store, "update_plugin", fake_update) + monkeypatch.setattr(store, "_get_local_git_info", lambda p: None) + updated, failed = auto_update.update_plugins(store) + assert "stocks" in calls and "ledmatrix-stocks" not in calls + assert not any(plugin_dirs.BACKUP_MARKER in c for c in calls) + # the version change was seen in ledmatrix-stocks/, not a missing stocks/ + assert updated == ["stocks"] and failed == [] diff --git a/web_interface/auto_update.py b/web_interface/auto_update.py index 610391a1..1ac3085a 100644 --- a/web_interface/auto_update.py +++ b/web_interface/auto_update.py @@ -172,9 +172,10 @@ def _plugin_fingerprint(store_manager, plugin_dir): try: with open(plugin_dir / 'manifest.json', 'r', encoding='utf-8') as f: manifest = json.load(f) - if manifest.get('local_only'): - return None - version = manifest.get('version') + if isinstance(manifest, dict): # valid JSON need not be an object + if manifest.get('local_only'): + return None + version = manifest.get('version') except (OSError, ValueError): pass sha = None @@ -190,8 +191,12 @@ def update_plugins(store_manager, operation_history=None): """Update every installed plugin that has an update. Returns (updated, failed).""" updated, failed = [], [] plugins_dir = Path(store_manager.plugins_dir) + # list_installed_plugins() reports manifest ids, and a plugin's directory + # may be named differently (ledmatrix-stocks/ holding id "stocks"), so + # the directory comes from the store's own lookup, not a join. + find_dir = getattr(store_manager, '_find_plugin_path', None) for plugin_id in sorted(store_manager.list_installed_plugins()): - plugin_dir = plugins_dir / plugin_id + plugin_dir = (find_dir(plugin_id) if find_dir else None) or plugins_dir / plugin_id before = _plugin_fingerprint(store_manager, plugin_dir) if before is None: continue # local_only: managed by hand, never from the registry