From 302ab1da4f172857f7044833843ce648b96a58d1 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 21 May 2026 15:53:16 -0400 Subject: [PATCH] fix(plugin-config): handle missing type key in oneOf/anyOf schema fields (#344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web-ui): dedup registry fetches, surface reconciliation warnings, add check-update endpoint Story 1 — src/plugin_system/store_manager.py: Add threading.Lock (_registry_fetch_lock) to fetch_registry(). The outer cache check remains the hot path (no lock). When the cache is cold, only one thread hits the network; concurrent callers block on the lock then get the result from the warm cache (double-checked locking). Eliminates duplicate GitHub requests on every page load when the 15-minute cache expires. Story 2 — web_interface/app.py + api_v3.py + overview.html: _run_startup_reconciliation() now writes /tmp/ledmatrix_reconciliation.json (atomic tempfile+replace, mirrors hw_status pattern) so the result survives the background thread. New GET /api/v3/plugins/reconciliation-status reads that file. Overview page gains a dismissible yellow banner that shows stale plugin_id values (e.g. sync, github, youtube) and tells the user to remove them or reinstall from the Plugin Store. Banner is suppressed for the session after dismiss using sessionStorage keyed on the plugin_id list. Story 3 — web_interface/blueprints/api_v3.py: Add GET /api/v3/system/check-update. Does git fetch origin main then compares local HEAD vs origin/main to compute update_available, remote_sha, and commits_behind. Result is cached for 5 minutes so it doesn't run git on every page load. Falls back to {update_available: false} on any error. Eliminates the 404 logged on every page load. Story 4 (Pi 5 rgbmatrix rebuild) was already fixed in PR #341. Co-Authored-By: Claude Sonnet 4.6 * fix(plugin-config): handle missing `type` key in schema fields using oneOf/anyOf Jinja2's `prop.type` on a dict without a `type` key returns an Undefined object. Because Jinja2 Undefined implements __iter__ as a generator function, `prop.type is iterable` evaluates True, then `prop.type[0]` calls Undefined.__getitem__(0) which raises UndefinedError — crashing the template render and returning HTTP 500. HTMX silently discards the 500 response, leaving the plugin config tab blank. Fix: use `prop.get('type')` which returns None for missing keys instead of Undefined. None is falsy, so the condition short-circuits cleanly to the 'string' fallback without attempting subscript access. Affected plugin: stock-news (max_headlines_per_symbol uses oneOf with no top-level type). Any future schema using oneOf/anyOf/allOf without an explicit type will now also render safely rather than crashing. Co-Authored-By: Claude Sonnet 4.6 * fix(security): harden check-update, reconciliation status endpoint, and temp-file write api_v3.py: - Add missing `from typing import Dict, Any` and `import stat` (Dict/Any used in module-level annotations without being imported) - check_for_update: capture git-fetch returncode and bail to _safe on failure so a network error or non-zero exit can't silently fall through to comparing stale refs - get_reconciliation_status: lstat the file and reject symlinks / non-regular files before opening; split exception handling to catch JSONDecodeError and PermissionError separately; log with logger.exception; return a generic 'Status file unavailable' message instead of str(e) to avoid leaking internal details overview.html: - Replace one-shot reconciliation fetch with a polling loop (2 s interval via setTimeout) so the banner still appears when reconciliation finishes after the page first loads - dismissReconciliationBanner: write sessionStorage immediately using the key stored on the banner element (set at show time) so dismissal persists even if the background sync fetch fails; clear the polling timer on dismiss to avoid leaks app.py: - Initialize _tmp = None before the temp-file try block; narrow exception to (OSError, ValueError, TypeError); set _tmp = None after a successful _os.replace so the finally branch knows nothing needs unlinking; add finally clause to unlink the temp file if it was left behind by a mid-write failure Co-Authored-By: Claude Sonnet 4.6 * fix: reconciliation status errors return graceful not-done instead of HTTP 500; log fetch stderr get_reconciliation_status: symlink/non-regular-file, JSONDecodeError, and PermissionError all now return {'done': False, 'unresolved': []} so the polling loop in overview.html keeps retrying rather than stopping on a transient error. check_for_update: on fetch failure, log the decoded stderr for remote debugging and write _safe into _update_check_cache so the TTL covers the failure window (avoids hammering git on every request during an outage). Co-Authored-By: Claude Sonnet 4.6 * fix(bandit): replace hardcoded /tmp paths with tempfile.gettempdir() (B108) Codacy/Bandit B108 flagged two hardcoded '/tmp/' string literals in app.py (lines 737, 741). Replaced with _tempfile.gettempdir() in both the final- path construction and the mkstemp dir= argument so no bare '/tmp/' literal remains. Also updated the matching reader path in api_v3.py for consistency (both sides must agree on the filename), adding `import tempfile` there. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Chuck Co-authored-by: Claude Sonnet 4.6 --- src/plugin_system/store_manager.py | 80 +++++++++++-------- web_interface/app.py | 35 ++++++++ web_interface/blueprints/api_v3.py | 78 ++++++++++++++++++ .../templates/v3/partials/overview.html | 63 +++++++++++++++ .../templates/v3/partials/plugin_config.html | 3 +- 5 files changed, 225 insertions(+), 34 deletions(-) diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 5d4ba48a..d547fbf0 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -10,6 +10,7 @@ import json import stat import subprocess import shutil +import threading import zipfile import tempfile import requests @@ -100,6 +101,10 @@ class PluginStoreManager: # handlers. Bumping the cached-entry timestamp on failure serves # the stale payload cheaply until the backoff expires. self._failure_backoff_seconds = 60 + # Prevents concurrent callers from each firing a network request when + # the registry cache expires. Only one thread fetches; others wait and + # then get the result from the warm cache (double-checked locking). + self._registry_fetch_lock = threading.Lock() # Ensure plugins directory exists self.plugins_dir.mkdir(exist_ok=True) @@ -575,41 +580,50 @@ class PluginStoreManager: (current_time - self.registry_cache_time) < self.registry_cache_timeout): return self.registry_cache - try: - self.logger.info(f"Fetching plugin registry from {self.REGISTRY_URL}") - response = self._http_get_with_retries(self.REGISTRY_URL, timeout=10) - response.raise_for_status() - self.registry_cache = response.json() - self.registry_cache_time = current_time - self.logger.info(f"Fetched registry with {len(self.registry_cache.get('plugins', []))} plugins") - return self.registry_cache - except requests.RequestException as e: - self.logger.error(f"Error fetching registry: {e}") - if raise_on_failure: - raise - # Prefer stale cache over an empty list so the plugin list UI - # keeps working on a flaky connection (e.g. Pi on WiFi). Bump - # registry_cache_time into a short backoff window so the next - # request serves the stale payload cheaply instead of - # re-hitting the network on every request (matches the - # pattern used by github_cache / commit_info_cache). - if self.registry_cache: - self.logger.warning("Falling back to stale registry cache") - self.registry_cache_time = ( - time.time() + self._failure_backoff_seconds - self.registry_cache_timeout - ) + with self._registry_fetch_lock: + # Re-check inside the lock — a concurrent caller that was waiting + # may have already populated the cache while we blocked. + current_time = time.time() + if (self.registry_cache and self.registry_cache_time and + not force_refresh and + (current_time - self.registry_cache_time) < self.registry_cache_timeout): return self.registry_cache - return {"plugins": []} - except json.JSONDecodeError as e: - self.logger.error(f"Error parsing registry JSON: {e}") - if raise_on_failure: - raise - if self.registry_cache: - self.registry_cache_time = ( - time.time() + self._failure_backoff_seconds - self.registry_cache_timeout - ) + + try: + self.logger.info(f"Fetching plugin registry from {self.REGISTRY_URL}") + response = self._http_get_with_retries(self.REGISTRY_URL, timeout=10) + response.raise_for_status() + self.registry_cache = response.json() + self.registry_cache_time = current_time + self.logger.info(f"Fetched registry with {len(self.registry_cache.get('plugins', []))} plugins") return self.registry_cache - return {"plugins": []} + except requests.RequestException as e: + self.logger.error(f"Error fetching registry: {e}") + if raise_on_failure: + raise + # Prefer stale cache over an empty list so the plugin list UI + # keeps working on a flaky connection (e.g. Pi on WiFi). Bump + # registry_cache_time into a short backoff window so the next + # request serves the stale payload cheaply instead of + # re-hitting the network on every request (matches the + # pattern used by github_cache / commit_info_cache). + if self.registry_cache: + self.logger.warning("Falling back to stale registry cache") + self.registry_cache_time = ( + time.time() + self._failure_backoff_seconds - self.registry_cache_timeout + ) + return self.registry_cache + return {"plugins": []} + except json.JSONDecodeError as e: + self.logger.error(f"Error parsing registry JSON: {e}") + if raise_on_failure: + raise + if self.registry_cache: + self.registry_cache_time = ( + time.time() + self._failure_backoff_seconds - self.registry_cache_timeout + ) + return self.registry_cache + return {"plugins": []} def search_plugins(self, query: str = "", category: str = "", tags: List[str] = None, fetch_commit_info: bool = True, include_saved_repos: bool = True, saved_repositories_manager = None) -> List[Dict]: """ diff --git a/web_interface/app.py b/web_interface/app.py index 57286300..c87a7f21 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -716,6 +716,41 @@ def _run_startup_reconciliation() -> None: "manual 'Reconcile' action to resolve.", len(result.inconsistencies_manual), ) + + # Write status file so the web UI can surface unresolved issues as a + # banner without the user having to read journalctl. Mirrors the + # hw_status pattern (/tmp/led_matrix_hw_status.json). + import json as _json, tempfile as _tempfile, os as _os + _recon_status = { + "done": True, + "successful": result.reconciliation_successful, + "fixed_count": len(result.inconsistencies_fixed), + "unresolved": [ + { + "plugin_id": inc.plugin_id, + "type": inc.inconsistency_type.value, + "description": inc.description, + } + for inc in result.inconsistencies_manual + ], + } + _recon_path = _os.path.join(_tempfile.gettempdir(), "ledmatrix_reconciliation.json") + _tmp = None + try: + if not _os.path.islink(_recon_path): + _fd, _tmp = _tempfile.mkstemp(dir=_tempfile.gettempdir(), prefix=".led_recon_") + with _os.fdopen(_fd, "w") as _f: + _json.dump(_recon_status, _f) + _os.replace(_tmp, _recon_path) + _tmp = None # Rename succeeded; nothing to clean up + except (OSError, ValueError, TypeError) as _e: + _logger.warning("[Reconciliation] Could not write status file: %s", _e) + finally: + if _tmp is not None and _os.path.exists(_tmp): + try: + _os.unlink(_tmp) + except OSError: + pass except Exception as e: _logger.error("[Reconciliation] Error: %s", e, exc_info=True) finally: diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 58c4fe68..089b80e1 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -2,14 +2,17 @@ from flask import Blueprint, request, jsonify, Response import json import os import re +import stat import sys import subprocess +import tempfile import time import hashlib import uuid import logging from datetime import datetime from pathlib import Path +from typing import Dict, Any logger = logging.getLogger(__name__) @@ -1384,6 +1387,59 @@ def get_system_version(): except Exception as e: return jsonify({'status': 'error', 'message': str(e)}), 500 +_update_check_cache: Dict[str, Any] = {'result': None, 'ts': 0.0} +_UPDATE_CHECK_TTL = 300 # 5 minutes — avoids a git fetch on every page load + +@api_v3.route('/system/check-update', methods=['GET']) +def check_for_update(): + """Check whether a newer LEDMatrix commit is available on origin/main.""" + now = time.time() + if _update_check_cache['result'] and now - _update_check_cache['ts'] < _UPDATE_CHECK_TTL: + return jsonify(_update_check_cache['result']) + + _safe: Dict[str, Any] = {'update_available': False, 'remote_sha': 'unknown', 'commits_behind': 0} + try: + cwd = str(PROJECT_ROOT) + fetch_result = subprocess.run( + ['git', 'fetch', 'origin', 'main', '--quiet'], + capture_output=True, timeout=10, cwd=cwd, + ) + if fetch_result.returncode != 0: + logger.warning("check-update: git fetch failed (rc=%d): %s", + fetch_result.returncode, + fetch_result.stderr.decode(errors='replace').strip()) + _update_check_cache['result'] = _safe + _update_check_cache['ts'] = now + return jsonify(_safe) + local = subprocess.run( + ['git', 'rev-parse', 'HEAD'], + capture_output=True, text=True, timeout=5, cwd=cwd, + ).stdout.strip() + remote = subprocess.run( + ['git', 'rev-parse', 'origin/main'], + capture_output=True, text=True, timeout=5, cwd=cwd, + ).stdout.strip() + + if not local or not remote: + return jsonify(_safe) + + if local == remote: + result: Dict[str, Any] = {'update_available': False, 'remote_sha': remote, 'commits_behind': 0} + else: + count_str = subprocess.run( + ['git', 'rev-list', 'HEAD..origin/main', '--count'], + capture_output=True, text=True, timeout=5, cwd=cwd, + ).stdout.strip() + count = int(count_str) if count_str.isdigit() else 0 + result = {'update_available': count > 0, 'remote_sha': remote, 'commits_behind': count} + + _update_check_cache['result'] = result + _update_check_cache['ts'] = now + return jsonify(result) + except Exception as e: + logger.warning("check-update failed: %s", e) + return jsonify(_safe) + @api_v3.route('/system/action', methods=['POST']) def execute_system_action(): """Execute system actions (start/stop/reboot/etc)""" @@ -2433,6 +2489,28 @@ def reconcile_plugin_state(): status_code=500 ) +@api_v3.route('/plugins/reconciliation-status', methods=['GET']) +def get_reconciliation_status(): + """Return the result of the last startup reconciliation from /tmp status file.""" + _recon_path = os.path.join(tempfile.gettempdir(), "ledmatrix_reconciliation.json") + try: + st = os.lstat(_recon_path) + except FileNotFoundError: + return jsonify({'status': 'success', 'data': {'done': False, 'unresolved': []}}) + if stat.S_ISLNK(st.st_mode) or not stat.S_ISREG(st.st_mode): + logger.warning("[Reconciliation] Status file is not a regular file: %s", _recon_path) + return jsonify({'status': 'success', 'data': {'done': False, 'unresolved': []}}) + try: + with open(_recon_path) as _f: + data = json.load(_f) + return jsonify({'status': 'success', 'data': data}) + except json.JSONDecodeError: + logger.exception("[Reconciliation] Failed to parse status file: %s", _recon_path) + return jsonify({'status': 'success', 'data': {'done': False, 'unresolved': []}}) + except PermissionError: + logger.exception("[Reconciliation] Permission denied reading status file: %s", _recon_path) + return jsonify({'status': 'success', 'data': {'done': False, 'unresolved': []}}) + @api_v3.route('/plugins/config', methods=['GET']) def get_plugin_config(): """Get plugin configuration""" diff --git a/web_interface/templates/v3/partials/overview.html b/web_interface/templates/v3/partials/overview.html index 36af1300..f743d82c 100644 --- a/web_interface/templates/v3/partials/overview.html +++ b/web_interface/templates/v3/partials/overview.html @@ -1,3 +1,66 @@ + + + +

System Overview

diff --git a/web_interface/templates/v3/partials/plugin_config.html b/web_interface/templates/v3/partials/plugin_config.html index 5347fc84..9072497d 100644 --- a/web_interface/templates/v3/partials/plugin_config.html +++ b/web_interface/templates/v3/partials/plugin_config.html @@ -9,7 +9,8 @@ {% set field_id = (plugin_id ~ '-' ~ full_key)|replace('.', '-')|replace('_', '-') %} {% set label = prop.title if prop.title else key|replace('_', ' ')|title %} {% set description = prop.description if prop.description else '' %} - {% set field_type = prop.type if prop.type is string else (prop.type[0] if prop.type is iterable else 'string') %} + {% set _pt = prop.get('type') %} + {% set field_type = _pt if (_pt is string) else ((_pt | first) if (_pt and _pt is iterable and _pt is not string) else 'string') %} {# Handle nested objects - check for widget first #} {% if field_type == 'object' %}