fix: address PR review findings

Fix (10 of 15 findings):

plugin-repos/march-madness/requirements.txt:
  Add urllib3>=1.26.0 — manager.py directly imports from urllib3; it was
  an undeclared transitive dependency via requests.

scripts/dev/dev_plugin_setup.sh:
  Restore subshell form (cd "$target_dir" && git pull --rebase) || true
  so the shell's working directory is not permanently changed after the
  if-cd block. Previous fix for SC2015 leaked cwd into the remainder of
  the script.

src/base_classes/sports.py:
  Narrow 'except Exception' to 'except RuntimeError as e' and log via
  self.logger.debug — Path.home() raises only RuntimeError for service
  users; other exceptions should not be silently swallowed.

src/config_service.py:
  Fix stale "MD5 checksum" in ConfigVersion.__init__ docstring (line 40);
  the implementation uses SHA-256 since the Codacy fix.

src/wifi_manager.py:
  Log the last-resort AP enable failure with exc_info=True instead of
  silently passing — failure here means the device may be unreachable.

web_interface/blueprints/pages_v3.py:
  Log the outer metadata pre-load exception at debug level instead of
  swallowing it silently; schema still loads fully below.

src/background_data_service.py:
  Remove unused 'timeout' parameter from shutdown() — executor.shutdown()
  does not accept timeout; update __del__ caller accordingly.

src/font_manager.py:
  Validate URL scheme before urlretrieve — reject non-http/https schemes
  (e.g. file://) to prevent reading local files from config-supplied URLs.

src/plugin_system/plugin_executor.py:
  Simplify redundant except tuple: (PluginTimeoutError, PluginError,
  Exception) → Exception, which already covers the others.

test/test_display_controller.py:
  Mark empty test_plugin_discovery_and_loading as @pytest.mark.skip with
  reason. Move duplicate 'from datetime import datetime' to module header
  and remove the stray mid-module copy.

Skip (5 of 15 findings, with reasons):
  - pytest 9.0.3 concerns: full suite already verified (467 pass, 18 pre-existing)
  - Pillow 12.2.0 API concerns: no deprecated APIs in codebase; tests + Pi smoke test pass
  - diagnose_web_ui.sh sudo validation: set -e already ensures fail-fast on any sudo failure
  - app.py request-logging except: must stay silent (recursive logging risk); annotated
  - app.py SSE file-read except: genuinely transient I/O; annotated

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

plugin-repos/march-madness/requirements.txt

@@ -1,4 +1,5 @@
 requests>=2.33.0
+urllib3>=1.26.0
 Pillow>=12.2.0
 pytz>=2022.1
 numpy>=1.24.0

scripts/dev/dev_plugin_setup.sh

@@ -203,9 +203,7 @@ link_github_plugin() {
         log_info "Repository already exists at $target_dir"
         if [[ -d "$target_dir/.git" ]]; then
             log_info "Updating repository..."
-            if cd "$target_dir"; then
-                git pull --rebase || true
-            fi
+            (cd "$target_dir" && git pull --rebase) || true
         fi
     else
         # Clone the repository

src/background_data_service.py

@@ -549,13 +549,12 @@ class BackgroundDataService:
             if to_remove:
                 logger.info(f"Cleared {len(to_remove)} old completed requests")
     
-    def shutdown(self, wait: bool = True, timeout: int = 30):
+    def shutdown(self, wait: bool = True):
         """
         Shutdown the background data service.
-        
+
         Args:
             wait: Whether to wait for active requests to complete
-            timeout: Maximum time to wait for shutdown
         """
         logger.info("Shutting down BackgroundDataService...")
         
@@ -573,7 +572,7 @@ class BackgroundDataService:
     def __del__(self):
         """Cleanup when service is destroyed."""
         if not self._shutdown:
-            self.shutdown(wait=False, timeout=None)
+            self.shutdown(wait=False)
 
 # Global service instance
 _background_service: Optional[BackgroundDataService] = None

src/base_classes/sports.py

@@ -172,8 +172,8 @@ class SportsCore(ABC):
 
         try:
             fallbacks.append(Path.home() / ".ledmatrix" / "logos" / self.sport_key)
-        except Exception:  # nosec B110 - Path.home() raises RuntimeError for service users; fallback list continues
-            pass
+        except RuntimeError as e:
+            self.logger.debug("Could not resolve home directory (expected for service users): %s", e)
 
         fallbacks.append(Path(tempfile.gettempdir()) / "ledmatrix_logos" / self.sport_key)
 

src/config_service.py

@@ -37,7 +37,7 @@ class ConfigVersion:
             config: Configuration dictionary
             version: Version number
             timestamp: When this version was created
-            checksum: MD5 checksum of the config
+            checksum: SHA-256 hex digest of the config (for change detection)
         """
         self.config: Dict[str, Any] = config
         self.version: int = version

src/font_manager.py

@@ -3,6 +3,7 @@ import logging
 import freetype
 import json
 import hashlib
+import urllib.parse
 import urllib.request
 import zipfile
 import tempfile
@@ -265,9 +266,12 @@ class FontManager:
                 logger.info(f"Using cached font: {cache_path}")
                 return str(cache_path)
 
-            # Download font
+            # Download font — restrict to http/https to prevent file:// reads
+            parsed = urllib.parse.urlparse(url)
+            if parsed.scheme not in ('http', 'https'):
+                raise ValueError(f"Font URL must use http or https, got: {parsed.scheme!r}")
             logger.info(f"Downloading font from {url}")
-            urllib.request.urlretrieve(url, cache_path)  # nosec B310 - URL from user's own config file on local device
+            urllib.request.urlretrieve(url, cache_path)  # nosec B310 - scheme validated above
 
             # Handle zip files
             if url.endswith('.zip'):

src/plugin_system/plugin_executor.py

@@ -245,7 +245,7 @@ class PluginExecutor:
                 timeout=timeout,
                 plugin_id=plugin_id
             )
-        except (PluginTimeoutError, PluginError, Exception) as e:
+        except Exception as e:  # covers PluginTimeoutError, PluginError, and unexpected errors
             self.logger.warning(
                 "Plugin %s %s failed, using default return: %s",
                 plugin_id,

src/wifi_manager.py

@@ -1401,8 +1401,8 @@ class WiFiManager:
                     # Last resort: enable AP mode
                     try:
                         self.enable_ap_mode()
-                    except Exception:  # nosec B110 - last-resort recovery; if AP enable fails there's nothing left to try
-                        pass
+                    except Exception as ap_error:  # nosec B110 - last-resort; do not re-raise, but log for debugging
+                        logger.error("Last-resort AP mode enable failed in recovery path: %s", ap_error, exc_info=True)
             return False, str(e)
     
     def _restore_original_connection(self, connection_name: str, ssid: str) -> bool:

test/test_display_controller.py

@@ -1,5 +1,7 @@
 import time
+from datetime import datetime
 from unittest.mock import MagicMock, patch
+import pytest
 
 class TestDisplayControllerInitialization:
     """Test DisplayController initialization and setup."""
@@ -13,18 +15,12 @@ class TestDisplayControllerInitialization:
         assert test_display_controller.plugin_manager is not None
         assert test_display_controller.available_modes == []
 
+    @pytest.mark.skip(reason="No assertions; init logic is covered by test_init_success and fixture setup")
     def test_plugin_discovery_and_loading(self, test_display_controller):
         """Test plugin discovery and loading during initialization."""
-        # Mock plugin manager behavior
         pm = test_display_controller.plugin_manager
         pm.discover_plugins.return_value = ["plugin1", "plugin2"]
         pm.get_plugin.return_value = MagicMock()
-        
-        # Manually trigger the plugin loading logic that happens in __init__
-        # Since we're using a fixture that mocks __init__ partially, we need to verify 
-        # the interactions or simulate the loading if we want to test that specific logic
-        # Note: Testing __init__ logic is tricky with the fixture. 
-        # We rely on the fixture to give us a usable controller.
 
 
 class TestDisplayControllerModeRotation:
@@ -248,5 +244,3 @@ class TestDisplayControllerSchedule:
             with patch.object(controller.config_service, 'get_config', return_value=schedule_config):
                 controller._check_schedule()
                 assert controller.is_display_active is False
-            
-from datetime import datetime

web_interface/blueprints/pages_v3.py

@@ -395,8 +395,8 @@ def _load_plugin_config_partial(plugin_id):
                                             config['images'] = config.get('images', []) + new_images
                             except Exception as e:
                                 print(f"Warning: Could not load metadata for {plugin_id}: {e}")
-            except Exception:  # nosec B110 - metadata pre-load is optional; schema loads fully below
-                pass  # Will load schema properly below
+            except Exception as e:  # nosec B110 - metadata pre-load is optional; schema loads fully below
+                logger.debug("Metadata pre-load skipped for plugin %s: %s", plugin_id, e)
         
         # Get plugin schema
         schema = {}