diff --git a/.gitignore b/.gitignore index fea80d19..86909a99 100644 --- a/.gitignore +++ b/.gitignore @@ -81,10 +81,10 @@ assets/stocks/crypto_icons/ # Plugin operation state written at runtime. # -# web_interface/app.py writes data/plugin_operations.json, data/plugin_state.json -# and data/operation_history.json as the web interface runs, into a directory that -# ships tracked (data/.gitkeep) and was otherwise unignored. So every rig that ever -# opened the web UI -- and every test run that constructs the app -- left three +# web_interface/app.py writes data/plugin_state.json and data/operation_history.json +# (older releases also data/plugin_operations.json) as the web interface runs, into +# a directory that ships tracked (data/.gitkeep). Unignored, every rig that ever +# opened the web UI -- and every test run that constructs the app -- would leave # untracked files behind and a permanently dirty `git status`. Same reasoning as # the logo rule above: a checkout that is always dirty is a checkout nobody reads. data/* diff --git a/CHANGELOG.md b/CHANGELOG.md index 81da2d21..90421aac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,15 @@ accepts both, but the store flags the old spelling as deprecated - `src/common/README.md` covers every module. - Stale setup, service and troubleshooting claims are corrected. +- Plugin store and plugin manager fixes: + - Updating a plugin that was installed from a ZIP no longer tries to reinstall it from the LEDMatrix repository's own URL. + - Repository URLs with `.git` in the middle are no longer mangled. The URL helpers now live in `src/plugin_system/repo_urls.py`. + - Installing from a URL works when the repository's only branch isn't `main` or `master`. + - A missing required config field is reported once, by name. + - A plugin that went over `max_memory_mb` once is no longer refused on every call after that. + - `reload_plugin` reads the manifest from the plugin's discovered directory. + - Removed: `last_display` from plugin state info and `get_last_display()` (nothing recorded them); `PluginOperationQueue`'s `history_file` and `lazy_load` arguments; and `data/plugin_operations.json`, which nothing read. + - The web service (`ledmatrix-web`) logs through `src.logging_config` like the display service, so `journalctl -p err -u ledmatrix-web` works. Successful GET/HEAD/OPTIONS requests (the UI's polling) are logged at DEBUG instead of diff --git a/src/plugin_system/base_plugin.py b/src/plugin_system/base_plugin.py index 8bd62635..5af7f7fe 100644 --- a/src/plugin_system/base_plugin.py +++ b/src/plugin_system/base_plugin.py @@ -11,7 +11,6 @@ Stability: Stable - maintains backward compatibility from abc import ABC, abstractmethod from enum import Enum from typing import Dict, Any, Optional, List -import logging import os import sys from src.logging_config import get_logger @@ -511,104 +510,53 @@ class BasePlugin(ABC): """ Get the display duration for this plugin instance. - Automatically detects duration from: - 1. self.display_duration instance variable (if exists) - 2. self.config.get("display_duration", 15.0) (fallback) + Uses, in order, the first positive number among: + 1. ``self.display_duration`` (a common pattern in scoreboard plugins) + 2. ``self.config["display_duration"]`` + 3. 15.0 - Can be overridden by plugins to provide dynamic durations based - on content (e.g., longer duration for more complex displays). + Numeric strings count as numbers. Can be overridden by plugins to + provide dynamic durations based on content (e.g., longer duration for + more complex displays). Returns: Duration in seconds to display this plugin's content """ - # Check for instance variable first (common pattern in scoreboard plugins) - if hasattr(self, 'display_duration'): - try: - duration = getattr(self, 'display_duration') - # Handle None case - if duration is None: - pass # Fall through to config - # Try to convert to float if it's a number or numeric string. - # bool is excluded: it's an int subclass, and True would - # otherwise read as a 1-second duration. - elif isinstance(duration, (int, float)) and not isinstance(duration, bool): - if duration > 0: - return float(duration) - else: - self.logger.debug( - "display_duration instance variable is non-positive (%s), using config fallback", - duration - ) - # Try converting string representations of numbers - elif isinstance(duration, str): - try: - duration_float = float(duration) - if duration_float > 0: - return duration_float - else: - self.logger.debug( - "display_duration string value is non-positive (%s), using config fallback", - duration - ) - except (ValueError, TypeError): - self.logger.warning( - "display_duration instance variable has invalid string value '%s', using config fallback", - duration - ) - else: - self.logger.warning( - "display_duration instance variable has unexpected type %s (value: %s), using config fallback", - type(duration).__name__, duration - ) - except (TypeError, ValueError, AttributeError) as e: - self.logger.warning( - "Error reading display_duration instance variable: %s, using config fallback", - e - ) - - # Fall back to config - config_duration = self.config.get("display_duration", 15.0) try: - # Ensure config value is also a valid float (bool excluded — an - # int subclass that would otherwise read True as 1 second) - if isinstance(config_duration, (int, float)) and not isinstance(config_duration, bool): - if config_duration > 0: - return float(config_duration) - else: - self.logger.debug( - "Config display_duration is non-positive (%s), using default 15.0", - config_duration - ) - return 15.0 - elif isinstance(config_duration, str): - try: - duration_float = float(config_duration) - if duration_float > 0: - return duration_float - else: - self.logger.debug( - "Config display_duration string is non-positive (%s), using default 15.0", - config_duration - ) - return 15.0 - except ValueError: - self.logger.warning( - "Config display_duration has invalid string value '%s', using default 15.0", - config_duration - ) - return 15.0 - else: - self.logger.warning( - "Config display_duration has unexpected type %s (value: %s), using default 15.0", - type(config_duration).__name__, config_duration - ) - except (ValueError, TypeError) as e: + duration = getattr(self, 'display_duration', None) + except (TypeError, ValueError, AttributeError) as e: + # A plugin may define display_duration as a property that raises. self.logger.warning( - "Error processing config display_duration: %s, using default 15.0", - e - ) + "Error reading display_duration instance variable: %s, using config fallback", e) + duration = None + if duration is not None: + seconds = self._positive_seconds(duration, "display_duration instance variable") + if seconds is not None: + return seconds - return 15.0 + seconds = self._positive_seconds( + self.config.get("display_duration", 15.0), "config display_duration") + return seconds if seconds is not None else 15.0 + + def _positive_seconds(self, value: Any, source: str) -> Optional[float]: + """``value`` as a positive float, or None (with a log line) if it is not one. + + bool is rejected although it is an int subclass: True would otherwise + read as a 1-second duration. + """ + if isinstance(value, bool) or not isinstance(value, (int, float, str)): + self.logger.warning("%s has unexpected type %s (value: %s), ignoring it", + source, type(value).__name__, value) + return None + try: + seconds = float(value) + except ValueError: + self.logger.warning("%s has invalid value %r, ignoring it", source, value) + return None + if seconds > 0: + return seconds + self.logger.debug("%s is non-positive (%s), ignoring it", source, value) + return None # --------------------------------------------------------------------- # Dynamic duration support hooks @@ -926,25 +874,20 @@ class BasePlugin(ABC): config_mode, self.plugin_id ) - # Fall back to mapping legacy content_type - content_type = self.get_vegas_content_type() - if content_type == 'multi': + # Fall back to mapping legacy content_type. 'none' (excluded from + # Vegas) also maps to FIXED_SEGMENT: exclusion is decided by checking + # get_vegas_content_type() separately. + if self.get_vegas_content_type() == 'multi': return VegasDisplayMode.SCROLL - elif content_type == 'static': - return VegasDisplayMode.FIXED_SEGMENT - elif content_type == 'none': - # 'none' means excluded - return FIXED_SEGMENT as default - # The exclusion is handled by checking get_vegas_content_type() separately - return VegasDisplayMode.FIXED_SEGMENT - return VegasDisplayMode.FIXED_SEGMENT def get_supported_vegas_modes(self) -> List[VegasDisplayMode]: """ Return list of Vegas display modes this plugin supports. - Used by the web UI to show available mode options for user configuration. - Override to customize which modes are available for this plugin. + Not currently consulted by core: neither Vegas mode nor the web UI + calls it. It is kept, and plugins override it, as the declared set of + modes a future mode picker would offer. By default: - 'multi' content type plugins support SCROLL and FIXED_SEGMENT @@ -972,6 +915,10 @@ class BasePlugin(ABC): """ Get the preferred width for this plugin in Vegas FIXED_SEGMENT mode. + Not currently consulted by core: Vegas mode sizes a card from the + ``vegas_width_pct`` / ``vegas_scroll.render_width_pct`` settings + (see get_vegas_render_width()). Kept because plugins override it. + Returns the number of panels this plugin should occupy when displayed as a fixed segment. The actual pixel width is calculated as: width = panels * single_panel_width @@ -1025,7 +972,7 @@ class BasePlugin(ABC): required_fields = ['api_key', 'city'] for field in required_fields: if field not in self.config: - self.logger.error("Missing required field: %s", field) + self.logger.error("Missing required field: %s", field) return False return True """ diff --git a/src/plugin_system/operation_queue.py b/src/plugin_system/operation_queue.py index 8ac18080..b0e421b8 100644 --- a/src/plugin_system/operation_queue.py +++ b/src/plugin_system/operation_queue.py @@ -9,8 +9,6 @@ import threading import queue from typing import Dict, Optional, List, Callable, Any from datetime import datetime -from pathlib import Path -import json from src.plugin_system.operation_types import ( PluginOperation, OperationType, OperationStatus @@ -28,28 +26,22 @@ class PluginOperationQueue: - Prevents concurrent operations on same plugin - Operation status tracking - Operation cancellation - - Operation history + - In-memory history of finished operations + + The history is not persisted. The web UI's operation history comes from + OperationHistory (operation_history.py), which has its own file; a copy + written here was never read back by anything. """ - - def __init__( - self, - history_file: Optional[str] = None, - max_history: int = 100, - lazy_load: bool = False - ): + + def __init__(self, max_history: int = 100): """ Initialize operation queue. - + Args: - history_file: Optional path to file for persisting operation history max_history: Maximum number of operations to keep in history - lazy_load: If True, defer loading history file until first access """ self.logger = get_logger(__name__) - self.history_file = Path(history_file) if history_file else None self.max_history = max_history - self._lazy_load = lazy_load - self._history_loaded = False # Operation tracking self._operations: Dict[str, PluginOperation] = {} @@ -62,20 +54,8 @@ class PluginOperationQueue: self._worker_thread: Optional[threading.Thread] = None self._stop_event = threading.Event() - # Load history from file if it exists (unless lazy loading) - if not self._lazy_load and self.history_file and self.history_file.exists(): - self._load_history() - self._history_loaded = True - - # Start worker thread self._start_worker() - - def _ensure_loaded(self) -> None: - """Ensure history is loaded (for lazy loading).""" - if not self._history_loaded and self.history_file and self.history_file.exists(): - self._load_history() - self._history_loaded = True - + def enqueue_operation( self, operation_type: OperationType, @@ -139,7 +119,6 @@ class PluginOperationQueue: Returns: PluginOperation if found, None otherwise """ - self._ensure_loaded() with self._lock: return self._operations.get(operation_id) @@ -184,7 +163,6 @@ class PluginOperationQueue: Returns: List of operations, sorted by creation time (newest first) """ - self._ensure_loaded() with self._lock: # Sort by creation time (newest first) history = sorted( @@ -314,11 +292,7 @@ class PluginOperationQueue: if self._active_operations[operation.plugin_id].operation_id == operation.operation_id: del self._active_operations[operation.plugin_id] - # Add to history self._add_to_history(operation) - - # Save history to file - self._save_history() def _add_to_history(self, operation: PluginOperation) -> None: """Add operation to history, maintaining max_history limit.""" @@ -330,46 +304,6 @@ class PluginOperationQueue: self._operation_history.sort(key=lambda op: op.created_at) self._operation_history = self._operation_history[-self.max_history:] - def _save_history(self) -> None: - """Save operation history to file.""" - if not self.history_file: - return - - try: - with self._lock: - # Convert operations to dicts - history_data = [op.to_dict() for op in self._operation_history] - - # Ensure directory exists - self.history_file.parent.mkdir(parents=True, exist_ok=True) - - # Write to file - with open(self.history_file, 'w') as f: - json.dump(history_data, f, indent=2) - - except Exception as e: - self.logger.warning(f"Error saving operation history: {e}") - - def _load_history(self) -> None: - """Load operation history from file.""" - if not self.history_file or not self.history_file.exists(): - return - - try: - with open(self.history_file, 'r') as f: - history_data = json.load(f) - - with self._lock: - self._operation_history = [ - PluginOperation.from_dict(op_data) - for op_data in history_data - ] - - self.logger.info(f"Loaded {len(self._operation_history)} operations from history") - - except Exception as e: - self.logger.warning(f"Error loading operation history: {e}") - def shutdown(self) -> None: """Shutdown the operation queue and worker thread.""" self.logger.info("Shutting down plugin operation queue") @@ -377,7 +311,4 @@ class PluginOperationQueue: if self._worker_thread and self._worker_thread.is_alive(): self._worker_thread.join(timeout=5.0) - - # Save history one last time - self._save_history() diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 66feabab..7547c8f0 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -5,6 +5,7 @@ Handles plugin module imports, dependency installation, and class instantiation. Extracted from PluginManager to improve separation of concerns. """ +import errno import importlib import importlib.metadata import importlib.util @@ -184,6 +185,46 @@ def find_trusted_subdir(trusted_dir: str, name: str) -> Optional[str]: return None +def contained_plugin_dir(plugin_dir: Path, plugins_dir: Path) -> Optional[str]: + """``plugin_dir`` rebuilt from an entry enumerated under ``plugins_dir``. + + Returns None when ``plugin_dir`` is not a subdirectory of ``plugins_dir``. + Callers derive ``plugin_dir`` from a manifest-declared id, so the path is + rebuilt from :func:`find_trusted_subdir`'s answer rather than trusted: a + name that came out of ``os.scandir()`` on the trusted root carries no + taint, which is a real containment guarantee (and one CodeQL's + path-injection query can follow), not a string sanitiser. + """ + plugin_dir_real = os.path.realpath(str(plugin_dir)) + plugins_dir_real = os.path.realpath(str(plugins_dir)) + matched_name = find_trusted_subdir(plugins_dir_real, os.path.basename(plugin_dir_real)) + if matched_name is None: + return None + return os.path.join(plugins_dir_real, matched_name) + + +def requirements_to_install(plugin_dir: str, logger: logging.Logger, + label: str) -> Optional[str]: + """The plugin's requirements.txt if pip has work to do, else None. + + None when there is no requirements.txt, when it lists nothing (plugins + whose dependencies ship with core often keep an all-comments file), or + when every requirement is already installed. Shared by the loader and the + store so both skip pip for the same reasons; they differ only in how they + run it. + """ + requirements_file = os.path.join(plugin_dir, "requirements.txt") + if not os.path.isfile(requirements_file): + return None + if not requirements_has_real_deps(requirements_file): + logger.debug("requirements.txt for %s has no real dependencies, skipping pip", label) + return None + if requirements_are_satisfied(requirements_file): + logger.debug("Dependencies for %s already satisfied, skipping pip", label) + return None + return requirements_file + + class PluginLoader: """Handles plugin module loading and class instantiation.""" @@ -273,43 +314,15 @@ class PluginLoader: if not plugin_id: return False - # Resolve to a canonical absolute path (normalises .. and symlinks) - plugin_dir_real = os.path.realpath(str(plugin_dir)) - plugins_dir_real = os.path.realpath(str(plugins_dir)) - requested_name = os.path.basename(plugin_dir_real) - - # Match the requested directory against an entry actually enumerated - # from the trusted plugins_dir, and build the path from that entry -- - # not from requested_name. A name that came out of os.scandir() on a - # trusted root carries no taint regardless of what the caller asked - # for, so this is a real containment guarantee (an allowlist check - # against a trusted source), not a string-sanitisation of untrusted - # input that a static analyzer has to trust blindly. - matched_name = find_trusted_subdir(plugins_dir_real, requested_name) - if matched_name is None: + safe_plugin_dir = contained_plugin_dir(plugin_dir, plugins_dir) + if safe_plugin_dir is None: self.logger.error( "Plugin directory for %s not found inside plugins dir", plugin_id ) return False - safe_plugin_dir = os.path.join(plugins_dir_real, matched_name) - requirements_file = os.path.join(safe_plugin_dir, "requirements.txt") - - if not os.path.isfile(requirements_file): - return True # No dependencies needed - - if not requirements_has_real_deps(requirements_file): - self.logger.debug( - "requirements.txt for %s has no real dependencies (comments/blank only), skipping pip", - plugin_id - ) - return True - - if requirements_are_satisfied(requirements_file): - self.logger.debug( - "Dependencies for %s already satisfied in current environment, skipping pip", - plugin_id - ) + requirements_file = requirements_to_install(safe_plugin_dir, self.logger, plugin_id) + if requirements_file is None: return True try: @@ -348,8 +361,8 @@ class PluginLoader: # below). try: # sys.executable is this process's own interpreter (not - # attacker-influenced), and requirements_file is a path - # built internally by find_plugin_directory, never raw + # attacker-influenced), and requirements_file is rebuilt + # by contained_plugin_dir() from a trusted listing, never raw # external input. retry_result = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep [sys.executable, "-m", "pip", "install", "--break-system-packages", @@ -384,10 +397,10 @@ class PluginLoader: except FileNotFoundError: self.logger.warning("pip not found. Skipping dependency installation for %s", plugin_id) return True - except (BrokenPipeError, OSError) as e: - # Handle broken pipe errors (errno 32) which can occur during pip downloads - # Often caused by network interruptions or output buffer issues - if isinstance(e, OSError) and e.errno == 32: + except OSError as e: + # A broken pipe (EPIPE) happens when pip's output pipe closes + # mid-download, usually a network interruption. + if e.errno == errno.EPIPE: self.logger.error( "Broken pipe error during dependency installation for %s. " "This usually indicates a network interruption or pip output buffer issue. " @@ -528,7 +541,7 @@ class PluginLoader: plugin_id: str, plugin_dir: Path, entry_point: str - ) -> Optional[Any]: + ) -> Any: """ Load a plugin module from file. @@ -547,7 +560,12 @@ class PluginLoader: entry_point: Entry point filename (e.g., 'manager.py') Returns: - Loaded module or None on error + The loaded module + + Raises: + PluginError: If the plugin id, directory or entry point is + invalid. Whatever the module raises while executing + propagates unchanged. """ plugin_id = os.path.basename(plugin_id or '') if not plugin_id: @@ -782,9 +800,7 @@ class PluginLoader: # Load module entry_point = manifest.get('entry_point', 'manager.py') module = self.load_module(plugin_id, plugin_dir, entry_point) - if module is None: - raise PluginError(f"Failed to load module for plugin {plugin_id}", plugin_id=plugin_id) - + # Get plugin class class_name = manifest.get('class_name') if not class_name: diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 866e99eb..97aef580 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -2,7 +2,8 @@ Plugin Manager Manages plugin discovery, loading, and lifecycle for the LEDMatrix system. -Handles dynamic plugin loading from the plugins/ directory. +Loads plugins from the configured plugins directory +(``plugin_system.plugins_directory``, ``plugin-repos/`` by default). API Version: 1.0.0 """ @@ -40,7 +41,7 @@ class PluginManager: Manages plugin discovery, loading, and lifecycle. The PluginManager is responsible for: - - Discovering plugins in the plugins/ directory + - Discovering plugins in the configured plugins directory - Loading plugin modules and instantiating plugin classes - Managing plugin lifecycle (load, unload, reload) - Providing access to loaded plugins @@ -99,10 +100,9 @@ class PluginManager: self.plugin_directories: Dict[str, Path] = {} self.plugin_last_update: Dict[str, float] = {} - # Cached data-fetch intervals per plugin_id. - # _get_plugin_update_interval falls back to config_manager.get_config() - # (a full dict copy) when the manifest lacks an interval — caching avoids - # that copy on every 30-fps tick. Cleared on load/unload. + # Cached static data-fetch intervals per plugin_id, so the render + # loop's scheduling tick does not repeat the manifest/config lookup + # for every plugin. Cleared on load/unload. self._update_interval_cache: Dict[str, Optional[float]] = {} # Health tracking (optional, set by display_controller if available) @@ -110,14 +110,12 @@ class PluginManager: self.resource_monitor = None # --- Asynchronous plugin updates ------------------------------- - # update() used to run inline in the render loop (execute_update's - # internal thread.join(timeout=30) blocked it), so one slow plugin - # HTTP fetch froze scrolling for the whole fetch. Scheduling still - # happens on the render thread (run_scheduled_updates), but - # execution moves to this single background worker. Per-plugin - # locks keep a plugin's update() and display() mutually exclusive — - # today's implicit guarantee, now explicit (and, unlike today, - # also held across the post-timeout window). + # Run inline in the render loop, one slow plugin HTTP fetch in + # update() freezes scrolling for the whole fetch. Scheduling happens + # on the render thread (run_scheduled_updates); execution happens on + # this single background worker. Per-plugin locks keep a plugin's + # update() and display() mutually exclusive, including across the + # post-timeout window. # Kill switch: plugin_system.synchronous_updates: true restores the # inline path. self._update_queue: "queue.Queue[Optional[Tuple[str, float]]]" = queue.Queue() @@ -661,9 +659,15 @@ class PluginManager: if not self.unload_plugin(plugin_id): return False - # Re-discover to get updated manifest - manifest_path = self.plugins_dir / plugin_id / "manifest.json" - if manifest_path.exists(): + # Re-read the manifest so an edit to it takes effect, from the + # directory discovery found the plugin in: a directory's name need not + # be the id its manifest declares. + with self._discovery_lock: + directories = dict(self.plugin_directories) + plugin_dir = self.plugin_loader.find_plugin_directory( + plugin_id, self.plugins_dir, directories) + manifest_path = plugin_dir / "manifest.json" if plugin_dir is not None else None + if manifest_path is not None and manifest_path.exists(): try: with open(manifest_path, 'r', encoding='utf-8') as f: manifest = json.load(f) @@ -881,11 +885,12 @@ class PluginManager: updating, since a scheduler that propagates a plugin bug stops every other plugin too. - The static result is cached per plugin_id after the first lookup to - avoid calling config_manager.get_config() — which returns a full dict - copy — on every tick of the 30-fps display loop. The cache is - invalidated when a plugin is loaded or unloaded. The dynamic hook is - deliberately *not* cached: caching it would defeat its only purpose. + The static result is cached per plugin_id after the first lookup, so + the manifest/config resolution is not repeated on every scheduling + tick of the display loop. A change to ``update_interval`` in + config.json therefore takes effect when the plugin is next loaded or + unloaded, which clears the cache. The dynamic hook is deliberately + *not* cached: caching it would defeat its only purpose. """ dynamic = self._dynamic_update_interval(plugin_id, plugin_instance) if dynamic is not None: diff --git a/src/plugin_system/plugin_state.py b/src/plugin_system/plugin_state.py index 68087e1d..e1e26db1 100644 --- a/src/plugin_system/plugin_state.py +++ b/src/plugin_system/plugin_state.py @@ -41,7 +41,6 @@ class PluginStateManager: self._state_transition_counts: Dict[str, int] = {} self._error_info: Dict[str, Dict[str, Any]] = {} self._last_update: Dict[str, datetime] = {} - self._last_display: Dict[str, datetime] = {} def _record_transition(self, plugin_id: str) -> None: """Count a state transition. Callers must already hold ``_lock``.""" @@ -181,11 +180,7 @@ class PluginStateManager: def get_last_update(self, plugin_id: str) -> Optional[datetime]: """Get timestamp of last update() call.""" return self._last_update.get(plugin_id) - - def get_last_display(self, plugin_id: str) -> Optional[datetime]: - """Get timestamp of last display() call.""" - return self._last_display.get(plugin_id) - + def get_state_info(self, plugin_id: str) -> Dict[str, Any]: """ Get comprehensive state information for a plugin. @@ -212,7 +207,6 @@ class PluginStateManager: 'is_error': self.is_error(plugin_id), 'can_execute': self.can_execute(plugin_id), 'last_update': self.get_last_update(plugin_id), - 'last_display': self.get_last_display(plugin_id), 'error_info': self.get_error_info(plugin_id), 'state_history_count': self._state_transition_counts.get(plugin_id, 0) } @@ -231,5 +225,4 @@ class PluginStateManager: self._state_transition_counts.pop(plugin_id, None) self._error_info.pop(plugin_id, None) self._last_update.pop(plugin_id, None) - self._last_display.pop(plugin_id, None) diff --git a/src/plugin_system/repo_urls.py b/src/plugin_system/repo_urls.py new file mode 100644 index 00000000..0bfad8ae --- /dev/null +++ b/src/plugin_system/repo_urls.py @@ -0,0 +1,70 @@ +""" +Repository URL helpers shared by the plugin store and saved repositories. + +One definition of "the same repository URL", of how a GitHub URL maps to +``owner/repo``, and of the headers sent to the GitHub API. +""" + +from typing import Dict, Optional, Tuple +from urllib.parse import urlparse + +#: Hosts whose URLs name a GitHub repository. Matched against +#: ``urlparse(url).hostname``, never by substring: a substring test accepts +#: ``https://github.com.example.org/...`` as GitHub. +GITHUB_HOSTS = frozenset({'github.com', 'www.github.com'}) + +#: Sent on every request the store makes to GitHub. +USER_AGENT = 'LEDMatrix-Plugin-Manager/1.0' + + +def normalize_repo_url(url: str) -> str: + """``url`` without surrounding whitespace, trailing slashes or a trailing ``.git``. + + Only a *trailing* ``.git`` is removed. The unanchored + ``url.replace('.git', '')`` this replaces turned + ``https://github.com/user/my.github.io`` into ``.../myhub.io``. + Case is preserved; compare with :func:`same_repo`. + """ + url = url.strip().rstrip('/') + if url.endswith('.git'): + url = url[:-4] + return url + + +def same_repo(url_a: str, url_b: str) -> bool: + """Whether two URLs name the same repository. + + GitHub owner and repository names are case-insensitive, so + ``ChuckBuilds/LEDMatrix-Plugins`` and ``chuckbuilds/ledmatrix-plugins`` + are one repository. + """ + return normalize_repo_url(url_a).lower() == normalize_repo_url(url_b).lower() + + +def github_owner_repo(url: str) -> Optional[Tuple[str, str]]: + """``(owner, repo)`` for a github.com repository URL, else None. + + The first two path segments, so a URL that points inside the repository + (``.../owner/repo/tree/main/plugins/x``) still names ``owner/repo``. + """ + parsed = urlparse(normalize_repo_url(url)) + if parsed.hostname not in GITHUB_HOSTS: + return None + parts = [part for part in parsed.path.split('/') if part] + if len(parts) < 2: + return None + return parts[0], normalize_repo_url(parts[1]) + + +def github_api_headers(token: Optional[str] = None) -> Dict[str, str]: + """Headers for a GitHub REST API request, authenticated when ``token`` is set. + + An authenticated request gets 5000 requests an hour instead of 60. + """ + headers = { + 'Accept': 'application/vnd.github.v3+json', + 'User-Agent': USER_AGENT, + } + if token: + headers['Authorization'] = f'token {token}' + return headers diff --git a/src/plugin_system/resource_monitor.py b/src/plugin_system/resource_monitor.py index b024e726..ed11e9f3 100644 --- a/src/plugin_system/resource_monitor.py +++ b/src/plugin_system/resource_monitor.py @@ -33,7 +33,14 @@ class ResourceLimits: @dataclass class ResourceMetrics: - """Resource usage metrics for a plugin.""" + """Resource usage metrics for a plugin. + + ``memory_mb`` is the largest growth in this *process's* resident memory + seen across a single monitored call -- a high-water mark, not current + usage, and not the plugin's own footprint (another thread allocating + during the call counts too). ``cpu_percent`` is the whole process's CPU + use since the previous sample. + """ memory_mb: float = 0.0 cpu_percent: float = 0.0 execution_time: float = 0.0 @@ -42,11 +49,6 @@ class ResourceMetrics: max_execution_time: float = 0.0 min_execution_time: float = float('inf') last_update_time: float = field(default_factory=time.time) - - def update_average_execution_time(self): - """Update average execution time.""" - if self.call_count > 0: - self.total_execution_time = self.total_execution_time / self.call_count #: How often a plugin's metrics are written to the cache, in seconds. @@ -287,7 +289,8 @@ class PluginResourceMonitor: # Calculate execution time execution_time = time.time() - start_time - + memory_growth_mb = 0.0 + # Update metrics with self._lock: metrics.execution_time = execution_time @@ -302,17 +305,17 @@ class PluginResourceMonitor: # Update memory and CPU if monitoring enabled if self.enable_monitoring: - end_memory = self._get_process_memory_mb() - metrics.memory_mb = max(metrics.memory_mb, end_memory - start_memory) + memory_growth_mb = self._get_process_memory_mb() - start_memory + metrics.memory_mb = max(metrics.memory_mb, memory_growth_mb) # CPU is harder to measure per-call, so we track it separately metrics.cpu_percent = self._get_process_cpu_percent() - + # Persist metrics, at most once per interval per plugin. self._persist_metrics(plugin_id, metrics) - - # Check limits + if limits: - self._check_limits(plugin_id, metrics, limits, execution_time) + self._check_limits(plugin_id, metrics, limits, execution_time, + memory_growth_mb) return result @@ -326,9 +329,17 @@ class PluginResourceMonitor: metrics.last_update_time = time.time() raise - def _check_limits(self, plugin_id: str, metrics: ResourceMetrics, - limits: ResourceLimits, execution_time: float) -> None: - """Check if plugin has exceeded resource limits.""" + def _check_limits(self, plugin_id: str, metrics: ResourceMetrics, + limits: ResourceLimits, execution_time: float, + memory_growth_mb: float) -> None: + """Raise ResourceLimitExceeded if this call went over a limit. + + Execution time and memory growth are this call's own; CPU is the + latest process sample. Judging memory by the stored high-water mark + (``metrics.memory_mb``) instead would fail every call after the first + expensive one, so the health tracker's circuit breaker would reopen + on every recovery probe and the plugin would never update again. + """ warnings = [] errors = [] @@ -343,13 +354,13 @@ class PluginResourceMonitor: ) # Check memory - if limits.max_memory_mb and metrics.memory_mb > limits.max_memory_mb: + if limits.max_memory_mb and memory_growth_mb > limits.max_memory_mb: errors.append( - f"Memory usage {metrics.memory_mb:.2f}MB exceeds limit {limits.max_memory_mb:.2f}MB" + f"Memory growth {memory_growth_mb:.2f}MB exceeds limit {limits.max_memory_mb:.2f}MB" ) - elif limits.max_memory_mb and metrics.memory_mb > limits.max_memory_mb * limits.warning_threshold: + elif limits.max_memory_mb and memory_growth_mb > limits.max_memory_mb * limits.warning_threshold: warnings.append( - f"Memory usage {metrics.memory_mb:.2f}MB approaching limit {limits.max_memory_mb:.2f}MB" + f"Memory growth {memory_growth_mb:.2f}MB approaching limit {limits.max_memory_mb:.2f}MB" ) # Check CPU diff --git a/src/plugin_system/saved_repositories.py b/src/plugin_system/saved_repositories.py index c8da3c5b..64794d8c 100644 --- a/src/plugin_system/saved_repositories.py +++ b/src/plugin_system/saved_repositories.py @@ -10,6 +10,8 @@ import os from pathlib import Path from typing import List, Dict, Optional +from src.plugin_system.repo_urls import normalize_repo_url + class SavedRepositoriesManager: """Manages saved GitHub repository URLs.""" @@ -71,18 +73,6 @@ class SavedRepositoriesManager: pass return False - @staticmethod - def _clean_url(repo_url: str) -> str: - """Normalize a repo URL: strip whitespace, trailing slashes, and a - trailing ``.git`` suffix ONLY. (The old ``.replace('.git', '')`` - was an unanchored substring replace that mangled URLs merely - containing ``.git``, e.g. ``https://github.com/user/my.github.io``.) - """ - repo_url = repo_url.strip().rstrip('/') - if repo_url.endswith('.git'): - repo_url = repo_url[:-4] - return repo_url - def get_all(self) -> List[Dict[str, str]]: """Get all saved repositories.""" return self.repositories.copy() @@ -98,7 +88,7 @@ class SavedRepositoriesManager: Returns: True if added successfully """ - repo_url = self._clean_url(repo_url) + repo_url = normalize_repo_url(repo_url) # Check if already exists for repo in self.repositories: @@ -138,7 +128,7 @@ class SavedRepositoriesManager: Returns: True if removed successfully """ - repo_url = self._clean_url(repo_url) + repo_url = normalize_repo_url(repo_url) previous = self.repositories remaining = [r for r in previous if r.get('url') != repo_url] @@ -156,7 +146,7 @@ class SavedRepositoriesManager: def has(self, repo_url: str) -> bool: """Check if a repository is already saved.""" - repo_url = self._clean_url(repo_url) + repo_url = normalize_repo_url(repo_url) return any(r.get('url') == repo_url for r in self.repositories) def get_registry_repositories(self) -> List[Dict[str, str]]: diff --git a/src/plugin_system/schema_manager.py b/src/plugin_system/schema_manager.py index 0042f75e..8b267e7c 100644 --- a/src/plugin_system/schema_manager.py +++ b/src/plugin_system/schema_manager.py @@ -14,6 +14,7 @@ import jsonschema from jsonschema import Draft7Validator, ValidationError from src.core_config_keys import CORE_CONFIG_KEYS +from src.element_style import expand_style_elements def _renders_as_object(prop: Dict[str, Any]) -> bool: @@ -452,11 +453,7 @@ class SchemaManager: # full per-element style blocks (font/size/color + layout # offsets) the web-UI config form renders. No-op for schemas # without the declaration; never raises. - try: - from src.element_style import expand_style_elements - schema = expand_style_elements(schema) - except ImportError: - pass + schema = expand_style_elements(schema) # Cache the schema self._schema_cache[plugin_id] = schema @@ -643,20 +640,12 @@ class SchemaManager: [name for name in CORE_PLUGIN_PROPERTIES if name not in declared] ) - # Create validator with enhanced schema + # iter_errors reports every violation, including one ``required`` + # error per missing field at every depth. validator = Draft7Validator(enhanced_schema) - - # Collect all validation errors for error in validator.iter_errors(config): - error_msg = self._format_validation_error(error, plugin_id) - errors.append(error_msg) - - # Check required fields - required_fields = enhanced_schema.get('required', []) - for field in required_fields: - if field not in config: - errors.append(f"Missing required field: '{field}'") - + errors.append(self._format_validation_error(error, plugin_id)) + if errors: return False, errors @@ -687,7 +676,15 @@ class SchemaManager: field_path = f"'{path}'" if path else "root" if error.validator == 'required': - missing = error.validator_value + # validator_value is the schema's whole ``required`` list; the + # error itself is about one field, which jsonschema names only in + # its message ("'api_key' is a required property"). + missing = next( + (name for name in error.validator_value + if error.message.startswith(f"{name!r} ")), + None) + if missing is None: + return f"Field {field_path}: {error.message}" return f"Field {field_path}: Missing required property '{missing}'" elif error.validator == 'type': expected = error.validator_value diff --git a/src/plugin_system/state_manager.py b/src/plugin_system/state_manager.py index 16151f74..da8ea449 100644 --- a/src/plugin_system/state_manager.py +++ b/src/plugin_system/state_manager.py @@ -34,7 +34,9 @@ class PluginState: version: Optional[str] = None installed_at: Optional[datetime] = None last_updated: Optional[datetime] = None - config_version: int = 1 # For detecting state corruption + # Bumped on every update_plugin_state(). Nothing reads it; it stays so + # plugin_state.json keeps the shape older releases load with cls(**data). + config_version: int = 1 metadata: Dict[str, Any] = None def __post_init__(self): @@ -100,6 +102,8 @@ class PluginStateManager: # State storage self._states: Dict[str, PluginState] = {} + # The file's top-level "version", written back as read. Nothing + # checks it yet; it is there for a future format change to branch on. self._state_version = 1 # Threading @@ -193,7 +197,6 @@ class PluginStateManager: current_state.metadata = {} current_state.metadata.update(updates['metadata']) - # Increment config version current_state.config_version += 1 # Store updated state diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 5e543b1e..1a2a2168 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -5,6 +5,7 @@ Handles plugin discovery, installation, updates, and uninstallation from both the official registry and custom GitHub repositories. """ +import errno import os import re import json @@ -22,21 +23,19 @@ from pathlib import Path from typing import List, Dict, Optional, Any, Tuple, Set import logging -from urllib.parse import urlparse +from jsonschema import Draft7Validator, ValidationError -from src.common.permission_utils import sudo_remove_directory, install_requirements_file -from src.plugin_system.plugin_loader import ( - requirements_has_real_deps, requirements_are_satisfied, find_trusted_subdir +from src.common.permission_utils import ( + ensure_directory_permissions, get_plugin_dir_mode, install_requirements_file, + sudo_remove_directory, ) +from src.plugin_system.plugin_loader import contained_plugin_dir, requirements_to_install from src.plugin_system.plugin_dirs import ( BACKUP_MARKER, PluginDirectoryIndex, resolve_plugin_dir, store_search_dirs, ) - -try: - from jsonschema import Draft7Validator, ValidationError - JSONSCHEMA_AVAILABLE = True -except ImportError: - JSONSCHEMA_AVAILABLE = False +from src.plugin_system.repo_urls import ( + USER_AGENT, github_api_headers, github_owner_repo, normalize_repo_url, same_repo, +) class PluginStoreManager: @@ -93,11 +92,11 @@ class PluginStoreManager: self.registry_cache_timeout = 900 self.commit_info_cache = {} # Cache for latest commit info: {key: (timestamp, data)} # 30 minutes for commit/manifest caches. Plugin Store users browse - # the catalog via /plugins/store/list which fetches commit info and - # manifest data per plugin. 5-min TTLs meant every fresh browse on - # a Pi4 paid for ~3 HTTP requests x N plugins (30-60s serial). 30 - # minutes keeps the cache warm across a realistic session while - # still picking up upstream updates within a reasonable window. + # the catalog via /plugins/store/list, which fetches commit info per + # plugin; with a 5-minute TTL nearly every browse on a Pi4 paid for + # an HTTP request per plugin again. 30 minutes keeps the cache warm + # across a realistic session while still picking up upstream updates + # within a reasonable window. self.commit_cache_timeout = 1800 self.manifest_cache = {} # Cache for GitHub manifest fetches: {key: (timestamp, data)} self.manifest_cache_timeout = 1800 @@ -127,9 +126,9 @@ class PluginStoreManager: # where ``signature`` is a tuple of (head_mtime, resolved_ref_mtime, # head_contents) so a fast-forward update to the current branch # (which touches .git/refs/heads/ but NOT .git/HEAD) still - # invalidates the cache. Before this cache, every - # /plugins/installed request fired 4 git subprocesses per plugin, - # which pegged the CPU on a Pi4 with a dozen plugins. The cached + # invalidates the cache. Without it every /plugins/installed request + # runs a git subprocess per plugin, which adds up on a Pi4 with a + # dozen plugins. The cached # ``data`` dict is the same shape returned by ``_get_local_git_info`` # itself (sha / short_sha / branch / optional remote_url, date_iso, # date) — all string-keyed strings. @@ -368,13 +367,7 @@ class PluginStoreManager: # Validate token by making a lightweight API call to /user endpoint try: api_url = "https://api.github.com/user" - headers = { - 'Accept': 'application/vnd.github.v3+json', - 'User-Agent': 'LEDMatrix-Plugin-Manager/1.0', - 'Authorization': f'token {token}' - } - - response = requests.get(api_url, headers=headers, timeout=5) + response = requests.get(api_url, headers=github_api_headers(token), timeout=5) if response.status_code == 200: # Token is valid @@ -502,9 +495,6 @@ class PluginStoreManager: Returns: List of validation error messages (empty if valid or schema unavailable) """ - if not JSONSCHEMA_AVAILABLE: - return [] - try: # Load manifest schema schema_path = Path(__file__).parent.parent.parent / "schema" / "manifest_schema.json" @@ -535,134 +525,107 @@ class PluginStoreManager: self.logger.debug(f"Error validating manifest schema for {plugin_id}: {e}") return [] + _EMPTY_REPO_INFO: Dict[str, Any] = { + 'stars': 0, + 'forks': 0, + 'open_issues': 0, + 'updated_at_iso': '', + 'last_commit_iso': '', + 'last_commit_date': '', + 'language': '', + 'license': '', + 'default_branch': 'main', + } + def _get_github_repo_info(self, repo_url: str) -> Dict[str, Any]: - """Fetch GitHub repository information (stars, etc.)""" - # Extract owner/repo from URL + """GitHub metadata for a repository (stars, default branch, last push). + + Returns zeroed defaults (``_EMPTY_REPO_INFO``) for a non-GitHub URL or + when GitHub cannot be asked and nothing is cached. + """ try: - # Handle different URL formats - _parsed_url = urlparse(repo_url) - if _parsed_url.hostname in ('github.com', 'www.github.com'): - parts = repo_url.strip('/').split('/') - if len(parts) >= 2: - owner = parts[-2] - repo = parts[-1] - if repo.endswith('.git'): - repo = repo[:-4] + owner_repo = github_owner_repo(repo_url) + if owner_repo is None: + return dict(self._EMPTY_REPO_INFO) + owner, repo = owner_repo + cache_key = f"{owner}/{repo}" - cache_key = f"{owner}/{repo}" + if cache_key in self.github_cache: + cached_time, cached_data = self.github_cache[cache_key] + if time.time() - cached_time < self.cache_timeout: + return cached_data - # Check cache first - if cache_key in self.github_cache: - cached_time, cached_data = self.github_cache[cache_key] - if time.time() - cached_time < self.cache_timeout: - return cached_data + api_url = f"https://api.github.com/repos/{owner}/{repo}" + try: + response = requests.get( + api_url, headers=github_api_headers(self.github_token), timeout=10) + except requests.RequestException as req_err: + # Network error: prefer a stale cache hit over an empty + # default so the UI keeps working on a flaky Pi WiFi link. + # Bump the cached entry's timestamp into a short backoff + # window so subsequent requests serve the stale payload + # cheaply instead of re-hitting the network on every request. + if cache_key in self.github_cache: + _, stale = self.github_cache[cache_key] + self._record_cache_backoff(self.github_cache, cache_key, self.cache_timeout, stale) + self.logger.warning( + "GitHub repo info fetch failed for %s (%s); serving stale cache.", + cache_key, req_err, + ) + return stale + raise - # Fetch from GitHub API - api_url = f"https://api.github.com/repos/{owner}/{repo}" - headers = { - 'Accept': 'application/vnd.github.v3+json', - 'User-Agent': 'LEDMatrix-Plugin-Manager/1.0' - } - - # Add authentication if token is available - if self.github_token: - headers['Authorization'] = f'token {self.github_token}' + if response.status_code == 200: + data = response.json() + pushed_at = data.get('pushed_at', '') or data.get('updated_at', '') + repo_info = { + 'stars': data.get('stargazers_count', 0), + 'forks': data.get('forks_count', 0), + 'open_issues': data.get('open_issues_count', 0), + 'updated_at_iso': data.get('updated_at', ''), + 'last_commit_iso': pushed_at, + 'last_commit_date': self._iso_to_date(pushed_at), + 'language': data.get('language', ''), + 'license': data.get('license', {}).get('name', '') if data.get('license') else '', + 'default_branch': data.get('default_branch', 'main') + } + self.github_cache[cache_key] = (time.time(), repo_info) + return repo_info - try: - response = requests.get(api_url, headers=headers, timeout=10) - except requests.RequestException as req_err: - # Network error: prefer a stale cache hit over an - # empty default so the UI keeps working on a flaky - # Pi WiFi link. Bump the cached entry's timestamp - # into a short backoff window so subsequent - # requests serve the stale payload cheaply instead - # of re-hitting the network on every request. - if cache_key in self.github_cache: - _, stale = self.github_cache[cache_key] - self._record_cache_backoff(self.github_cache, cache_key, self.cache_timeout, stale) - self.logger.warning( - "GitHub repo info fetch failed for %s (%s); serving stale cache.", - cache_key, req_err, - ) - return stale - raise + if response.status_code == 403: + # Rate limit or authentication issue. A stale star count is + # better than a reset to zero, and the backoff bump stops the + # store hammering the API while rate-limited. + if cache_key in self.github_cache: + _, stale = self.github_cache[cache_key] + self._record_cache_backoff(self.github_cache, cache_key, self.cache_timeout, stale) + self.logger.warning( + "GitHub API 403 for %s; serving stale cache.", cache_key, + ) + return stale + if not self.github_token: + self.logger.warning( + "GitHub API rate limit likely exceeded (403). " + "Add a GitHub personal access token to config/config_secrets.json " + "under 'github.api_token' to increase rate limits from 60 to 5000/hour." + ) + else: + self.logger.warning( + f"GitHub API request failed: 403 for {api_url}. " + f"Your token may have insufficient permissions or rate limit exceeded." + ) + else: + self.logger.warning(f"GitHub API request failed: {response.status_code} for {api_url}") + if cache_key in self.github_cache: + _, stale = self.github_cache[cache_key] + self._record_cache_backoff(self.github_cache, cache_key, self.cache_timeout, stale) + return stale - if response.status_code == 200: - data = response.json() - pushed_at = data.get('pushed_at', '') or data.get('updated_at', '') - repo_info = { - 'stars': data.get('stargazers_count', 0), - 'forks': data.get('forks_count', 0), - 'open_issues': data.get('open_issues_count', 0), - 'updated_at_iso': data.get('updated_at', ''), - 'last_commit_iso': pushed_at, - 'last_commit_date': self._iso_to_date(pushed_at), - 'language': data.get('language', ''), - 'license': data.get('license', {}).get('name', '') if data.get('license') else '', - 'default_branch': data.get('default_branch', 'main') - } - - # Cache the result - self.github_cache[cache_key] = (time.time(), repo_info) - return repo_info - elif response.status_code == 403: - # Rate limit or authentication issue. If we have a - # previously-cached value, serve it rather than - # returning empty defaults — a stale star count is - # better than a reset to zero. Apply the same - # failure-backoff bump as the network-error path - # so we don't hammer the API with repeat requests - # while rate-limited. - if cache_key in self.github_cache: - _, stale = self.github_cache[cache_key] - self._record_cache_backoff(self.github_cache, cache_key, self.cache_timeout, stale) - self.logger.warning( - "GitHub API 403 for %s; serving stale cache.", cache_key, - ) - return stale - if not self.github_token: - self.logger.warning( - "GitHub API rate limit likely exceeded (403). " - "Add a GitHub personal access token to config/config_secrets.json " - "under 'github.api_token' to increase rate limits from 60 to 5000/hour." - ) - else: - self.logger.warning( - f"GitHub API request failed: 403 for {api_url}. " - f"Your token may have insufficient permissions or rate limit exceeded." - ) - else: - self.logger.warning(f"GitHub API request failed: {response.status_code} for {api_url}") - if cache_key in self.github_cache: - _, stale = self.github_cache[cache_key] - self._record_cache_backoff(self.github_cache, cache_key, self.cache_timeout, stale) - return stale - - return { - 'stars': 0, - 'forks': 0, - 'open_issues': 0, - 'updated_at_iso': '', - 'last_commit_iso': '', - 'last_commit_date': '', - 'language': '', - 'license': '', - 'default_branch': 'main' - } + return dict(self._EMPTY_REPO_INFO) except Exception as e: self.logger.error(f"Error fetching GitHub repo info for {repo_url}: {e}") - return { - 'stars': 0, - 'forks': 0, - 'open_issues': 0, - 'updated_at_iso': '', - 'last_commit_iso': '', - 'last_commit_date': '', - 'language': '', - 'license': '', - 'default_branch': 'main' - } + return dict(self._EMPTY_REPO_INFO) def _http_get_with_retries(self, url: str, *, timeout: int = 10, stream: bool = False, headers: Dict[str, str] = None, max_retries: int = 3, backoff_sec: float = 0.75): """ @@ -697,27 +660,17 @@ class PluginStoreManager: Registry dict with plugins list, or None if not found/invalid """ try: - # Clean up URL - repo_url = repo_url.rstrip('/').replace('.git', '') - - # Try to find plugins.json in common locations - # First try root directory + repo_url = normalize_repo_url(repo_url) + + # plugins.json or registry.json at the root of main, then master. registry_urls = [] + owner_repo = github_owner_repo(repo_url) + if owner_repo is not None: + owner, repo = owner_repo + for branch in ['main', 'master']: + registry_urls.append(f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/plugins.json") + registry_urls.append(f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/registry.json") - # Extract owner/repo from URL - _parsed_repo_url = urlparse(repo_url) - if _parsed_repo_url.hostname in ('github.com', 'www.github.com'): - parts = repo_url.split('/') - if len(parts) >= 2: - owner = parts[-2] - repo = parts[-1] - - # Try common branch names - for branch in ['main', 'master']: - registry_urls.append(f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/plugins.json") - registry_urls.append(f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/registry.json") - - # Try each URL for url in registry_urls: try: response = self._http_get_with_retries(url, timeout=10) @@ -815,15 +768,20 @@ class PluginStoreManager: """ Search for plugins in the registry with enhanced metadata. - GitHub is now treated as the source of truth for live metadata like - stars and last commit timestamps. The registry provides descriptive - information (name, description, repo URL, etc.). + GitHub supplies live metadata such as stars and last commit + timestamps; the registry supplies descriptive information (name, + description, repo URL, etc.). Args: - query: Search query string (searches name, description, id) + query: Search query string (searches name, description, id, author) category: Filter by category (e.g., 'sports', 'weather', 'time') tags: Filter by tags (matches any tag in list) fetch_commit_info: If True (default), fetch commit metadata from GitHub. + include_saved_repos: If True (default), also search the + registry-style repositories the user saved. + saved_repositories_manager: The SavedRepositoriesManager holding + those repositories; without it only the official registry is + searched. Returns: List of matching plugin metadata enriched with GitHub information @@ -877,11 +835,11 @@ class PluginStoreManager: def _enrich(plugin: Dict) -> Dict: """Enrich a single plugin with GitHub metadata. - Called concurrently from a ThreadPoolExecutor. Each underlying - HTTP helper (``_get_github_repo_info`` / ``_get_latest_commit_info`` - / ``_fetch_manifest_from_github``) is thread-safe — they use - ``requests`` and write their own cache keys on Python dicts, - which is atomic under the GIL for single-key assignments. + Called concurrently from a ThreadPoolExecutor. Both HTTP helpers + (``_get_github_repo_info`` / ``_get_latest_commit_info``) are + thread-safe -- they use ``requests`` and write their own cache + keys on Python dicts, which is atomic under the GIL for + single-key assignments. """ enhanced_plugin = plugin.copy() repo_url = plugin.get('repo', '') @@ -912,24 +870,21 @@ class PluginStoreManager: # The registry's plugins.json already carries ``description`` # (it is generated from each plugin's manifest by # ``update_registry.py``), and ``last_updated`` is filled in - # from the commit info above. An earlier implementation - # fetched manifest.json per plugin anyway, which meant one - # extra HTTPS round trip per result; on a Pi4 with a flaky - # WiFi link the tail retries of that one extra call + # from the commit info above. Fetching manifest.json per + # plugin costs one extra HTTPS round trip per result; on a Pi4 + # with a flaky WiFi link the tail retries of that one call # (_http_get_with_retries does 3 attempts with exponential - # backoff) dominated wall time even after parallelization. + # backoff) dominate wall time even with the thread pool. return enhanced_plugin - # Fan out the per-plugin GitHub enrichment. The previous - # implementation did this serially, which on a Pi4 with ~15 plugins - # and a fresh cache meant 30+ HTTP requests in strict sequence (the - # "connecting to display" hang reported by users). With a thread + # Fan out the per-plugin GitHub enrichment. Serially, a Pi4 with ~15 + # plugins and a cold cache makes 30+ HTTP requests in strict sequence + # (the "connecting to display" hang users reported). With a thread # pool, latency is dominated by the slowest request rather than # their sum. Workers capped at 10 to stay well under the # unauthenticated GitHub rate limit burst and avoid overwhelming a - # Pi's WiFi link. For a small number of plugins the pool is - # essentially free. + # Pi's WiFi link. if not filtered: return [] @@ -959,46 +914,34 @@ class PluginStoreManager: Manifest data or None if not found """ try: - # Convert repo URL to raw content URL - # https://github.com/user/repo -> https://raw.githubusercontent.com/user/repo/branch/manifest.json - _parsed_manifest_url = urlparse(repo_url) - if _parsed_manifest_url.hostname in ('github.com', 'www.github.com'): - # Handle different URL formats - repo_url = repo_url.rstrip('/') - if repo_url.endswith('.git'): - repo_url = repo_url[:-4] + owner_repo = github_owner_repo(repo_url) + if owner_repo is None: + return None + owner, repo = owner_repo - parts = repo_url.split('/') - if len(parts) >= 2: - owner = parts[-2] - repo = parts[-1] + cache_key = f"{owner}/{repo}:{branch}:{manifest_path}" + if not force_refresh and cache_key in self.manifest_cache: + cached_time, cached_data = self.manifest_cache[cache_key] + if time.time() - cached_time < self.manifest_cache_timeout: + return cached_data - # Check cache first - cache_key = f"{owner}/{repo}:{branch}:{manifest_path}" - if not force_refresh and cache_key in self.manifest_cache: - cached_time, cached_data = self.manifest_cache[cache_key] - if time.time() - cached_time < self.manifest_cache_timeout: - return cached_data + raw_url = f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/{manifest_path}" + response = self._http_get_with_retries(raw_url, timeout=10) + if response.status_code == 200: + result = response.json() + self.manifest_cache[cache_key] = (time.time(), result) + return result + if response.status_code == 404 and branch != "main": + raw_url = f"https://raw.githubusercontent.com/{owner}/{repo}/main/{manifest_path}" + response = self._http_get_with_retries(raw_url, timeout=10) + if response.status_code == 200: + result = response.json() + self.manifest_cache[cache_key] = (time.time(), result) + return result - raw_url = f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/{manifest_path}" - - response = self._http_get_with_retries(raw_url, timeout=10) - if response.status_code == 200: - result = response.json() - self.manifest_cache[cache_key] = (time.time(), result) - return result - elif response.status_code == 404: - # Try main branch instead - if branch != "main": - raw_url = f"https://raw.githubusercontent.com/{owner}/{repo}/main/{manifest_path}" - response = self._http_get_with_retries(raw_url, timeout=10) - if response.status_code == 200: - result = response.json() - self.manifest_cache[cache_key] = (time.time(), result) - return result - - # Cache negative result - self.manifest_cache[cache_key] = (time.time(), None) + # Cache the miss too, so a plugin without a manifest at this path + # is not re-fetched on every browse. + self.manifest_cache[cache_key] = (time.time(), None) except Exception as e: self.logger.debug(f"Could not fetch manifest from GitHub for {repo_url}: {e}") @@ -1007,21 +950,11 @@ class PluginStoreManager: def _get_latest_commit_info(self, repo_url: str, branch: str = "main", force_refresh: bool = False) -> Optional[Dict[str, Any]]: """Return metadata about the latest commit on the given branch.""" try: - if 'github.com' not in repo_url: + owner_repo = github_owner_repo(repo_url) + if owner_repo is None: return None + owner, repo = owner_repo - repo_url = repo_url.rstrip('/') - if repo_url.endswith('.git'): - repo_url = repo_url[:-4] - - parts = repo_url.split('/') - if len(parts) < 2: - return None - - owner = parts[-2] - repo = parts[-1] - - # Check cache first cache_key = f"{owner}/{repo}:{branch}" if not force_refresh and cache_key in self.commit_info_cache: cached_time, cached_data = self.commit_info_cache[cache_key] @@ -1029,14 +962,7 @@ class PluginStoreManager: return cached_data branches_to_try = self._distinct_sequence([branch, 'main', 'master']) - - headers = { - 'Accept': 'application/vnd.github.v3+json', - 'User-Agent': 'LEDMatrix-Plugin-Manager/1.0' - } - - if self.github_token: - headers['Authorization'] = f'token {self.github_token}' + headers = github_api_headers(self.github_token) last_error = None for branch_name in branches_to_try: @@ -1254,46 +1180,57 @@ class PluginStoreManager: backup_path = plugin_path.with_name( 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 - # existed. + problem = self._set_aside(plugin_path, backup_path) + if problem: + # Can't stage a safety net. Attempting the install anyway is + # what callers got before the net existed; refusing would be + # a new failure mode for a direct install. self.logger.warning( - "Could not clear stale pre-install backup for %s at %s; " - "installing without a rollback net", plugin_id, backup_path) - return self._install_plugin_impl(plugin_id, branch) - - try: - plugin_path.rename(backup_path) - except OSError as e: - self.logger.warning( - "Could not set aside existing install of %s (%s); " - "installing without a rollback net", plugin_id, e) + "Installing %s without a rollback net: %s", plugin_id, problem) return self._install_plugin_impl(plugin_id, branch) try: installed = self._install_plugin_impl(plugin_id, branch) except Exception: - self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) + self._restore_backup(plugin_id, plugin_path, backup_path, "Install") raise if installed: - if not self._safe_remove_directory(backup_path): - self.logger.warning( - "Install of %s succeeded but the previous copy at %s " - "could not be removed; it will be cleared on the next " - "install", plugin_id, backup_path) + self._discard_backup(plugin_id, backup_path, "install") return True - self._restore_preinstall_backup(plugin_id, plugin_path, backup_path) + self._restore_backup(plugin_id, plugin_path, backup_path, "Install") return False - def _restore_preinstall_backup( - self, plugin_id: str, plugin_path: Path, backup_path: Path + def _set_aside(self, plugin_path: Path, backup_path: Path) -> Optional[str]: + """Rename an installed plugin to ``backup_path`` so a failed + (re)install can put it back. + + A stale backup left by a crash is cleared first, since it would block + the rename. Returns None on success, otherwise why it could not. + """ + if backup_path.exists() and not self._safe_remove_directory(backup_path): + return f"could not clear stale backup at {backup_path}" + try: + plugin_path.rename(backup_path) + except OSError as e: + return f"could not set aside {plugin_path}: {e}" + return None + + def _discard_backup(self, plugin_id: str, backup_path: Path, action: str) -> None: + """Remove the set-aside copy after a successful (re)install.""" + if not self._safe_remove_directory(backup_path): + self.logger.warning( + "%s of %s succeeded but the previous copy at %s could not be " + "removed; it will be cleared on the next %s", + action.capitalize(), plugin_id, backup_path, action) + + def _restore_backup( + self, plugin_id: str, plugin_path: Path, backup_path: Path, action: str ) -> None: - """Put the previous install back after a failed (re)install.""" + """Put the set-aside copy back after a failed (re)install.""" self.logger.error( - "Install of %s failed; restoring the previous version", plugin_id) + "%s of %s failed; restoring the previous version", action, plugin_id) try: if plugin_path.exists(): # Partial download debris from the failed install. @@ -1375,8 +1312,7 @@ class PluginStoreManager: return False else: branch_used = self._install_via_git(repo_url, plugin_path, branch_candidates) - if branch_used is None and not plugin_path.exists(): - # Git failed entirely; fall back to zip download + if branch_used is None: self.logger.info("Git not available or clone failed, attempting archive download...") for candidate in branch_candidates: download_url = f"{repo_url}/archive/refs/heads/{candidate}.zip" @@ -1384,7 +1320,7 @@ class PluginStoreManager: branch_used = candidate break - if branch_used is None and not plugin_path.exists(): + if branch_used is None: self.logger.error(f"Failed to install plugin {plugin_id} via git or archive download") return False @@ -1523,8 +1459,7 @@ class PluginStoreManager: branch_info = f" (branch: {branch})" if branch else "" self.logger.info(f"Installing plugin from custom URL: {repo_url}{branch_info}" + (f" (subpath: {plugin_path})" if plugin_path else "")) - # Clean up URL (remove .git suffix if present) - repo_url = repo_url.rstrip('/').replace('.git', '') + repo_url = normalize_repo_url(repo_url) temp_dir = None try: @@ -1549,24 +1484,22 @@ class PluginStoreManager: 'error': f'Failed to download or extract plugin from monorepo subdirectory: {plugin_path}' } else: - # Try git clone for direct plugin repos branch_used = self._install_via_git(repo_url, temp_dir, branch_candidates) - if branch_used: + if branch_used is not None: self.logger.info(f"Cloned via git (branch: {branch_used})") else: - # Git failed; try downloading as zip - branch_used = None + self.logger.info("Git not available or clone failed, attempting archive download...") for candidate in branch_candidates: download_url = f"{repo_url}/archive/refs/heads/{candidate}.zip" if self._install_via_download(download_url, temp_dir): branch_used = candidate break - - if branch_used is None: - return { - 'success': False, - 'error': 'Failed to clone or download repository' - } + + if branch_used is None: + return { + 'success': False, + 'error': 'Failed to clone or download repository' + } # Read manifest to get plugin ID manifest_path = temp_dir / "manifest.json" @@ -1634,8 +1567,10 @@ class PluginStoreManager: json.dump(manifest, f, indent=2) self.logger.info(f"Added missing entry_point field to {plugin_id} manifest (defaulted to manager.py)") - # Move to plugins directory - use manifest ID as source of truth - # This ensures directory name always matches manifest ID + # The directory is named for the caller's plugin_id when one was + # given (update_plugin passes the installed id), else for the + # manifest's id -- so it can differ from the manifest id, which + # discovery tolerates by reading the manifest. final_path = self.plugins_dir / plugin_id if final_path.exists(): self.logger.warning(f"Plugin {plugin_id} already exists, removing existing copy") @@ -1647,9 +1582,7 @@ class PluginStoreManager: shutil.move(str(temp_dir), str(final_path)) temp_dir = None # Prevent cleanup since we moved it - - # Note: plugin_id here is already from manifest (line 749), so directory name matches manifest ID - + # Install dependencies self._install_dependencies(final_path) @@ -1701,7 +1634,6 @@ class PluginStoreManager: Class name if found, None otherwise """ try: - import re with open(manager_file, 'r', encoding='utf-8') as f: content = f.read() @@ -1723,7 +1655,18 @@ class PluginStoreManager: return None def _install_via_git(self, repo_url: str, target_path: Path, branches: Optional[List[str]] = None) -> Optional[str]: - """Clone a repository into ``target_path``. Returns the branch name on success.""" + """Clone a repository into ``target_path``. + + Tries each of ``branches`` (default ``main``, ``master``), then the + repository's own default branch, so a repository whose only branch + is e.g. ``develop`` still installs. + + Returns: + The branch that was cloned, or None when every clone failed and + ``target_path`` has been removed. After a default-branch clone + this is the branch the clone checked out (``'HEAD'`` if the + remote's HEAD is detached), never None. + """ branches_to_try = self._distinct_sequence(branches or []) if not branches_to_try: branches_to_try = ['main', 'master'] @@ -1758,7 +1701,7 @@ class PluginStoreManager: timeout=60 ) self.logger.debug(f"Successfully cloned {repo_url} (git default branch) to {target_path}") - return None # Unknown branch name, git default used + return self._checked_out_branch(target_path) except (subprocess.CalledProcessError, subprocess.TimeoutExpired, FileNotFoundError) as e: last_error = e if target_path.exists(): @@ -1766,7 +1709,20 @@ class PluginStoreManager: self.logger.error(f"Git clone failed for all attempted branches: {last_error}") return None - + + @staticmethod + def _checked_out_branch(checkout: Path) -> str: + """The branch a fresh clone has checked out, read from ``.git/HEAD``. + + ``'HEAD'`` when HEAD is detached or unreadable. + """ + try: + head = (checkout / '.git' / 'HEAD').read_text(encoding='utf-8').strip() + except OSError: + return 'HEAD' + prefix = 'ref: refs/heads/' + return head[len(prefix):] if head.startswith(prefix) else 'HEAD' + def _install_from_monorepo(self, download_url: str, plugin_subpath: str, target_path: Path) -> bool: """ Install a plugin from a monorepo by downloading only the target subdirectory. @@ -1815,14 +1771,6 @@ class PluginStoreManager: pass return None, None - @staticmethod - def _normalize_repo_url(url: str) -> str: - """Normalize a GitHub repo URL for comparison (strip trailing / and .git).""" - url = url.rstrip('/') - if url.endswith('.git'): - url = url[:-4] - return url.lower() - def _install_from_monorepo_api(self, repo_url: str, branch: str, plugin_subpath: str, target_path: Path) -> bool: """ Install a plugin subdirectory using the GitHub Git Trees API. @@ -1841,25 +1789,15 @@ class PluginStoreManager: True if successful, False to trigger ZIP fallback """ try: - # Parse owner/repo from URL - clean_url = repo_url.rstrip('/') - if clean_url.endswith('.git'): - clean_url = clean_url[:-4] - parts = clean_url.split('/') - if len(parts) < 2: + owner_repo = github_owner_repo(repo_url) + if owner_repo is None: return False - owner, repo = parts[-2], parts[-1] + owner, repo = owner_repo # Step 1: Get the recursive tree listing (1 API call) api_url = f"https://api.github.com/repos/{owner}/{repo}/git/trees/{branch}?recursive=true" - headers = { - 'Accept': 'application/vnd.github.v3+json', - 'User-Agent': 'LEDMatrix-Plugin-Manager/1.0' - } - if self.github_token: - headers['Authorization'] = f'token {self.github_token}' - - tree_response = self._http_get_with_retries(api_url, timeout=15, headers=headers) + tree_response = self._http_get_with_retries( + api_url, timeout=15, headers=github_api_headers(self.github_token)) if tree_response.status_code != 200: self.logger.debug(f"Trees API returned {tree_response.status_code} for {owner}/{repo}") return False @@ -1892,10 +1830,6 @@ class PluginStoreManager: self.logger.info(f"Downloading {len(file_entries)} files for {plugin_subpath} via API") # Step 3: Create target directory and download each file - from src.common.permission_utils import ( - ensure_directory_permissions, - get_plugin_dir_mode - ) ensure_directory_permissions(target_path.parent, get_plugin_dir_mode()) target_path.mkdir(parents=True, exist_ok=True) @@ -1991,10 +1925,6 @@ class PluginStoreManager: source_plugin_dir = temp_extract / root_dir / plugin_subpath - from src.common.permission_utils import ( - ensure_directory_permissions, - get_plugin_dir_mode - ) ensure_directory_permissions(target_path.parent, get_plugin_dir_mode()) # Ensure target doesn't exist to prevent shutil.move nesting if target_path.exists(): @@ -2028,7 +1958,7 @@ class PluginStoreManager: try: self.logger.info(f"Downloading from: {download_url}") # Allow redirects (GitHub archive URLs redirect to codeload.github.com) - response = self._http_get_with_retries(download_url, timeout=60, stream=True, headers={'User-Agent': 'LEDMatrix-Plugin-Manager/1.0'}) + response = self._http_get_with_retries(download_url, timeout=60, stream=True, headers={'User-Agent': USER_AGENT}) response.raise_for_status() # Download to temporary file @@ -2065,10 +1995,6 @@ class PluginStoreManager: # Move contents from root_dir to target source_dir = temp_extract / root_dir if source_dir.exists(): - from src.common.permission_utils import ( - ensure_directory_permissions, - get_plugin_dir_mode - ) ensure_directory_permissions(target_path.parent, get_plugin_dir_mode()) shutil.move(str(source_dir), str(target_path)) else: @@ -2094,42 +2020,23 @@ class PluginStoreManager: """ Install Python dependencies from requirements.txt. + ``plugin_path`` is ultimately derived from a plugin-supplied manifest + ``id``, so it is only used after contained_plugin_dir() has rebuilt it + from a listing of ``self.plugins_dir``. + Args: plugin_path: Path to plugin directory Returns: True if successful or no requirements file """ - # Reconstruct the plugin path from the trusted self.plugins_dir base + - # an entry actually enumerated from it, rather than trusting - # plugin_path directly -- callers ultimately derive it from a - # plugin-supplied manifest "id" field (see install_plugin_from_url), - # so without this a malicious manifest could point requirements_file - # outside plugins_dir. find_trusted_subdir()'s return value always - # comes from os.scandir() on the trusted root, so building the path - # from it (not from the caller's string) is a real containment - # guarantee, matching the pattern in PluginLoader.install_dependencies(). - plugin_dir_real = os.path.realpath(str(plugin_path)) - plugins_dir_real = os.path.realpath(str(self.plugins_dir)) - requested_name = os.path.basename(plugin_dir_real) - matched_name = find_trusted_subdir(plugins_dir_real, requested_name) - if matched_name is None: + safe_plugin_dir = contained_plugin_dir(plugin_path, self.plugins_dir) + if safe_plugin_dir is None: self.logger.error("Plugin directory not found inside plugins dir for dependency install") return False - safe_plugin_path = Path(os.path.join(plugins_dir_real, matched_name)) - requirements_file = safe_plugin_path / "requirements.txt" - - if not requirements_file.exists(): - self.logger.debug(f"No requirements.txt found in {plugin_path.name}") - return True - - if not requirements_has_real_deps(str(requirements_file)): - self.logger.debug(f"requirements.txt for {plugin_path.name} has no real dependencies, skipping pip") - return True - - if requirements_are_satisfied(str(requirements_file)): - self.logger.debug(f"Dependencies for {plugin_path.name} already satisfied, skipping pip") + requirements_file = requirements_to_install(safe_plugin_dir, self.logger, plugin_path.name) + if requirements_file is None: return True try: @@ -2141,7 +2048,7 @@ class PluginStoreManager: # ledmatrix.service, so pip reports success while the package # stays invisible to the running plugin (e.g. missing `astral` # for the weather plugin even though "install" succeeded). - result = install_requirements_file(requirements_file, timeout=300) + result = install_requirements_file(Path(requirements_file), timeout=300) if result.returncode != 0: self.logger.error( f"Error installing dependencies for {plugin_path.name}: {result.stderr}" @@ -2153,10 +2060,10 @@ class PluginStoreManager: except subprocess.TimeoutExpired: self.logger.error("Dependency installation timed out") return False - except (BrokenPipeError, OSError) as e: - # Handle broken pipe errors (errno 32) which can occur during pip downloads - # Often caused by network interruptions or output buffer issues - if isinstance(e, OSError) and e.errno == 32: + except OSError as e: + # A broken pipe (EPIPE) happens when pip's output pipe closes + # mid-download, usually a network interruption. + if e.errno == errno.EPIPE: self.logger.error( f"Broken pipe error during dependency installation for {plugin_path.name}. " f"This usually indicates a network interruption or pip output buffer issue. " @@ -2166,7 +2073,6 @@ class PluginStoreManager: self.logger.error(f"OS error during dependency installation: {e}") return False except Exception as e: - # Catch any other unexpected errors self.logger.error(f"Unexpected error installing dependencies for {plugin_path.name}: {e}", exc_info=True) return False @@ -2241,9 +2147,9 @@ class PluginStoreManager: Results are cached keyed on a signature that includes HEAD contents plus the mtime of HEAD AND the resolved ref (or - packed-refs). Repeated calls skip the four ``git`` subprocesses - when nothing has changed, and a ``git pull`` that fast-forwards - the branch correctly invalidates the cache. + packed-refs). Repeated calls skip the ``git log`` subprocess when + nothing has changed, and a ``git pull`` that fast-forwards the + branch correctly invalidates the cache. """ git_dir = plugin_path / '.git' if not git_dir.exists(): @@ -2412,12 +2318,11 @@ class PluginStoreManager: 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. + directory that names the id exactly or declares it. So a registry id + such as `stocks` does not resolve to an installed `ledmatrix-stocks/` + declaring `ledmatrix-stocks` (the monorepo's leaderboard, music, + stocks and weather); callers pass the installed id, and + update_plugin() maps it back to the registry id itself. Args: plugin_id: Plugin identifier @@ -2545,11 +2450,9 @@ class PluginStoreManager: The old install is renamed aside (not deleted) until the new install succeeds, then removed; on ANY install failure the old directory is - restored. This is the difference between a failed update and a - destroyed plugin: the previous delete-then-install flow permanently - removed plugins whenever the download failed mid-update (seen in the - field during the monorepo migration on a Pi with broken DNS — every - old-remote plugin was deleted and none could be re-downloaded). + restored. Deleting first turns a failed download into a destroyed + plugin: during the monorepo migration a Pi with broken DNS lost every + old-remote plugin that way, with none able to be re-downloaded. The aside name embeds BACKUP_MARKER ('.standalone-backup-') so every plugin directory lookup (src/plugin_system/plugin_dirs.py) ignores it @@ -2564,18 +2467,11 @@ class PluginStoreManager: with self._get_reinstall_lock(plugin_id): backup_path = plugin_path.with_name( 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): - self.logger.error( - f"Could not clear stale backup for {plugin_id} at " - f"{backup_path}; leaving old install in place") - return False - try: - plugin_path.rename(backup_path) - except OSError as e: + problem = self._set_aside(plugin_path, backup_path) + if problem: self.logger.error( - f"Could not set aside old plugin directory for {plugin_id}: {e}") + "Not updating %s: %s; the installed version is left in place", + plugin_id, problem) return False try: @@ -2585,27 +2481,11 @@ class PluginStoreManager: installed = False if installed: - if not self._safe_remove_directory(backup_path): - self.logger.warning( - f"Update of {plugin_id} succeeded but the old backup " - f"at {backup_path} could not be removed; it will be " - f"cleared on the next update") + self._discard_backup(plugin_id, backup_path, "update") return True - # Install failed (bad network, registry error...) — put the old - # version back so the user still has a working plugin. - self.logger.error( - f"Reinstall of {plugin_id} failed; restoring previous version") - try: - if plugin_path.exists(): - # partial download debris from the failed install - self._safe_remove_directory(plugin_path) - backup_path.rename(plugin_path) - self.logger.info(f"Restored previous install of {plugin_id}") - except OSError as e: - self.logger.error( - f"CRITICAL: could not restore {plugin_id} from {backup_path}: {e}. " - f"The previous install is preserved there — rename it back manually.") + # Bad network, registry error...: the user keeps a working plugin. + self._restore_backup(plugin_id, plugin_path, backup_path, "Reinstall") return False def update_plugin(self, plugin_id: str) -> bool: @@ -2665,7 +2545,7 @@ class PluginStoreManager: # while the registry now points to the monorepo. Detect this and reinstall. registry_repo = plugin_info_remote.get('repo', '') local_remote = git_info.get('remote_url', '') - if local_remote and registry_repo and self._normalize_repo_url(local_remote) != self._normalize_repo_url(registry_repo): + if local_remote and registry_repo and not same_repo(local_remote, registry_repo): self.logger.info( f"Plugin {resolved_id} git remote ({local_remote}) differs from registry ({registry_repo}). " f"Reinstalling from registry to migrate to new source." @@ -2814,7 +2694,6 @@ class PluginStoreManager: # If status check times out, assume there might be changes and proceed self.logger.warning(f"Git status check timed out for {plugin_id}, proceeding with update") has_changes = True - status_result = type('obj', (object,), {'stdout': '', 'stderr': 'Status check timed out'})() stash_info = "" # Whether the pull can be undone without destroying work. @@ -2898,7 +2777,7 @@ class PluginStoreManager: except subprocess.CalledProcessError as git_error: error_output = git_error.stderr or git_error.stdout or "Unknown error" - cmd_str = ' '.join(git_error.cmd) if hasattr(git_error, 'cmd') else 'unknown' + cmd_str = ' '.join(git_error.cmd) self.logger.error(f"Git update failed for {plugin_id}") self.logger.error(f"Command: {cmd_str}") self.logger.error(f"Return code: {git_error.returncode}") @@ -2914,7 +2793,7 @@ class PluginStoreManager: self.logger.error(f"Authentication failed for {plugin_id}. Check git credentials or repository permissions.") elif "not found" in error_lower or "does not exist" in error_lower: self.logger.error(f"Remote branch or repository not found for {plugin_id}. Check repository URL and branch name.") - elif "merge conflict" in error_lower or "conflict" in error_lower: + elif "conflict" in error_lower: self.logger.error(f"Merge conflict detected for {plugin_id}. Resolve conflicts manually or reinstall plugin.") return False @@ -2922,24 +2801,28 @@ class PluginStoreManager: self.logger.warning(f"Git update timed out for {plugin_id}") return False - # Not a git repository - try to get repo URL from git config if it exists - # (in case .git directory was removed but remote URL is still in config) + # A plugin with its own .git that _get_local_git_info could not + # read (e.g. no commits yet) may still name a remote to reinstall + # from. Without its own .git, `git -C ` walks up and finds + # the enclosing LEDMatrix checkout when plugins live in + # plugin-repos/ -- `--local` does not prevent that -- and the + # "plugin's" remote would be LEDMatrix itself. repo_url = None - try: - # Use --local to avoid inheriting the parent LEDMatrix repo's git config - # when the plugin directory lives inside the main repo (e.g. plugin-repos/). - remote_url_result = subprocess.run( - ['git', '-C', str(plugin_path), 'config', '--local', '--get', 'remote.origin.url'], - capture_output=True, - text=True, - timeout=10, - check=False - ) - if remote_url_result.returncode == 0: - repo_url = remote_url_result.stdout.strip() - self.logger.info(f"Found git remote URL for {plugin_id}: {repo_url}") - except Exception as e: - self.logger.debug(f"Could not get git remote URL: {e}") + if (plugin_path / '.git').exists(): + try: + remote_url_result = subprocess.run( + ['git', '-C', str(plugin_path), 'config', '--local', '--get', 'remote.origin.url'], + capture_output=True, + text=True, + timeout=10, + check=False + ) + if remote_url_result.returncode == 0: + repo_url = remote_url_result.stdout.strip() or None + if repo_url: + self.logger.info(f"Found git remote URL for {plugin_id}: {repo_url}") + except (OSError, subprocess.SubprocessError) as e: + self.logger.debug(f"Could not get git remote URL: {e}") # Try registry-based update self.logger.info(f"Plugin {plugin_id} is not a git repository, checking registry...") @@ -3027,9 +2910,7 @@ class PluginStoreManager: return self._reinstall_with_rollback(registry_id, plugin_path) except Exception as e: - import traceback - self.logger.error(f"Error updating plugin {plugin_id}: {e}") - self.logger.debug(traceback.format_exc()) + self.logger.error(f"Error updating plugin {plugin_id}: {e}", exc_info=True) return False def list_installed_plugins(self) -> List[str]: diff --git a/test/test_config_validation_edge_cases.py b/test/test_config_validation_edge_cases.py index 040902cd..fa1fce13 100644 --- a/test/test_config_validation_edge_cases.py +++ b/test/test_config_validation_edge_cases.py @@ -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'"] diff --git a/test/test_metrics_cache_unknown_fields.py b/test/test_metrics_cache_unknown_fields.py index 729893a3..3dd4e1d3 100644 --- a/test/test_metrics_cache_unknown_fields.py +++ b/test/test_metrics_cache_unknown_fields.py @@ -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(): diff --git a/test/test_plugin_manager_reload.py b/test/test_plugin_manager_reload.py new file mode 100644 index 00000000..be8f25db --- /dev/null +++ b/test/test_plugin_manager_reload.py @@ -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"] diff --git a/test/test_plugin_state_transition_count.py b/test/test_plugin_state_transition_count.py index ae2ba05f..90ea40dd 100644 --- a/test/test_plugin_state_transition_count.py +++ b/test/test_plugin_state_transition_count.py @@ -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"])) diff --git a/test/test_repo_urls.py b/test/test_repo_urls.py new file mode 100644 index 00000000..3e960bf7 --- /dev/null +++ b/test/test_repo_urls.py @@ -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) diff --git a/test/test_resource_monitor.py b/test/test_resource_monitor.py index 2f651d1d..5e913210 100644 --- a/test/test_resource_monitor.py +++ b/test/test_resource_monitor.py @@ -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) diff --git a/test/test_saved_repositories.py b/test/test_saved_repositories.py index 39bed1e4..0736c44c 100644 --- a/test/test_saved_repositories.py +++ b/test/test_saved_repositories.py @@ -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 diff --git a/test/test_store_install_default_branch.py b/test/test_store_install_default_branch.py new file mode 100644 index 00000000..8997e444 --- /dev/null +++ b/test/test_store_install_default_branch.py @@ -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 == [] diff --git a/test/test_store_update_non_git_remote.py b/test/test_store_update_non_git_remote.py new file mode 100644 index 00000000..21be96b7 --- /dev/null +++ b/test/test_store_update_non_git_remote.py @@ -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 `` +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 == [] diff --git a/test/web_interface/integration/test_plugin_operations.py b/test/web_interface/integration/test_plugin_operations.py index 35de83cf..128f14ed 100644 --- a/test/web_interface/integration/test_plugin_operations.py +++ b/test/web_interface/integration/test_plugin_operations.py @@ -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"), diff --git a/web_interface/app.py b/web_interface/app.py index 9c75a62c..2a8822fa 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -144,12 +144,7 @@ schema_manager = SchemaManager( ) # Initialize operation queue for plugin operations -# Use lazy_load=True to defer file loading until first use (improves startup time) -operation_queue = PluginOperationQueue( - history_file=str(project_root / "data" / "plugin_operations.json"), - max_history=500, - lazy_load=True -) +operation_queue = PluginOperationQueue(max_history=500) # Initialize plugin state manager # Use lazy_load=True to defer file loading until first use (improves startup time)