From b428ae3eae57d218a7baf34cbe71cf00eb0a250d Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Thu, 6 Aug 2026 01:32:23 +0000
Subject: [PATCH] fix: address CodeRabbit review findings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Verified each finding against the code; fixes for the valid ones:

- install_dependencies_apt.py: the installer listed 'freetype', but the
  declared dependency is freetype-py — an apt miss would pip-install the
  wrong PyPI package. Now installs freetype-py with an import-name
  mapping (pre-existing bug, surfaced by the review).
- api_v3.py: pixel_mapper_config is validated as a string before being
  saved to display.hardware (JSON callers could previously store an
  object/list the matrix library can't use).
- .cursorrules: the Plugin Loading Process and File Organization
  sections still said discovery scans plugins/ — now consistent with the
  corrected overview (configured directory, default plugin-repos/).
- README.md: removed the stale '(except the core calendar)' claim — no
  core calendar exists in src/ — and qualified the plugin inventory
  (official plugins in the monorepo; third-party from their own repos).
- CONFIG_REFERENCE.md: hardware_mapping now shows the code fallback
  (adafruit-hat-pwm) alongside the template value.
- PLUGIN_REGISTRY_SETUP_GUIDE.md: check_plugin.py takes --plugin, not a
  positional id.
- scripts/fix_perms/fix_*.sh: exec bits set so the documented
  'sudo ./...' invocations work.
- Guard tests hardened: template guard now catches multi-line
  render_template() calls; widget guard parses actual <script> src
  values and fails if the widgets dir goes missing; type hints and
  docstrings added per repo coding guidelines.

Skipped with reasons (noted on the PR): limit_refresh_rate_hz 100-vs-90
is documented as intentional in CONFIG_REFERENCE.md; the psutil comment
already names the enforcing manifest; docs/archive/ findings are out of
scope per the docs policy (archive may rot).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
---
 .cursorrules                                |  4 +-
 README.md                                   | 11 +++---
 docs/CONFIG_REFERENCE.md                    |  2 +-
 docs/PLUGIN_REGISTRY_SETUP_GUIDE.md         |  2 +-
 scripts/fix_perms/fix_assets_permissions.sh |  0
 scripts/fix_perms/fix_cache_permissions.sh  |  0
 scripts/fix_perms/fix_plugin_permissions.sh |  0
 scripts/fix_perms/fix_web_permissions.sh    |  0
 scripts/install_dependencies_apt.py         |  8 ++--
 test/test_doc_links.py                      |  7 +++-
 test/test_template_targets.py               | 22 ++++++-----
 test/test_widget_scripts.py                 | 42 +++++++++++++++------
 test/web_interface/test_cache.py            | 22 +++++++----
 web_interface/blueprints/api_v3.py          |  4 ++
 14 files changed, 82 insertions(+), 42 deletions(-)
 mode change 100644 => 100755 scripts/fix_perms/fix_assets_permissions.sh
 mode change 100644 => 100755 scripts/fix_perms/fix_cache_permissions.sh
 mode change 100644 => 100755 scripts/fix_perms/fix_plugin_permissions.sh
 mode change 100644 => 100755 scripts/fix_perms/fix_web_permissions.sh

diff --git a/.cursorrules b/.cursorrules
index b89202dd..9b8ec283 100644
--- a/.cursorrules
+++ b/.cursorrules
@@ -227,7 +227,7 @@ Common optional fields:
 
 ## Plugin Loading Process
 
-1. **Discovery**: PluginManager scans `plugins/` directory for directories containing `manifest.json`
+1. **Discovery**: PluginManager scans the configured `plugin_system.plugins_directory` (default `plugin-repos/`) for directories containing `manifest.json`
 2. **Validation**: Validates manifest structure and required fields
 3. **Loading**: Imports plugin module and instantiates plugin class
 4. **Configuration**: Loads plugin config from `config/config.json`
@@ -301,7 +301,7 @@ Common optional fields:
 ## File Organization
 
 ```
-plugins/
+plugin-repos/            # default install dir (plugins/ when using dev symlinks)
   <plugin-id>/
     manifest.json          # Plugin metadata
     manager.py             # Main plugin class
diff --git a/README.md b/README.md
index c0791461..ccc12332 100644
--- a/README.md
+++ b/README.md
@@ -54,10 +54,11 @@ LEDMatrix is a plugin platform: the displays below are plugins installed
 from the built-in Plugin Store (web interface → Plugins), where each can be
 individually enabled, ordered, and configured — display durations, teams,
 stocks, weather, timezones, and more. The core repo ships with just two
-bundled plugins (`starlark-apps` and `web-ui-info`); everything else lives
-in the [ledmatrix-plugins](https://github.com/ChuckBuilds/ledmatrix-plugins)
-monorepo and installs with one click. Displays available in the store
-include:
+bundled plugins (`starlark-apps` and `web-ui-info`); the official plugins
+live in the [ledmatrix-plugins](https://github.com/ChuckBuilds/ledmatrix-plugins)
+monorepo and install with one click, and third-party plugins can be
+installed from their own GitHub repositories. Displays available in the
+store include:
 
 ### Time and Weather
 - Real-time clock display (2x 64x32 Displays 4mm Pixel Pitch)
@@ -428,7 +429,7 @@ I recommend using the web-ui "Quick Actions" to control the Display.
 ## Plugins
 
 <details>
-LEDMatrix uses a plugin-based architecture where all display functionality (except the core calendar) is implemented as plugins. All managers that were previously built into the core system are now available as plugins through the Plugin Store.
+LEDMatrix uses a plugin-based architecture where all display functionality is implemented as plugins. All managers that were previously built into the core system are now available as plugins through the Plugin Store.
 
 ### Plugin Store
 See the [Plugin Store documentation](https://github.com/ChuckBuilds/ledmatrix-plugins) for detailed installation instructions.
diff --git a/docs/CONFIG_REFERENCE.md b/docs/CONFIG_REFERENCE.md
index dbe15681..a7bd81df 100644
--- a/docs/CONFIG_REFERENCE.md
+++ b/docs/CONFIG_REFERENCE.md
@@ -55,7 +55,7 @@ in `DisplayManager` (`src/display_manager.py`, ~lines 270–295).
 | `chain_length` | int, `2` |
 | `parallel` | int, `1` |
 | `brightness` | int, `90` |
-| `hardware_mapping` | string, `"adafruit-hat"` |
+| `hardware_mapping` | string, `"adafruit-hat"` (code default `"adafruit-hat-pwm"`) |
 | `scan_mode` | int, `0` |
 | `pwm_bits` | int, `9` (code default 10) |
 | `pwm_dither_bits` | int, `1` |
diff --git a/docs/PLUGIN_REGISTRY_SETUP_GUIDE.md b/docs/PLUGIN_REGISTRY_SETUP_GUIDE.md
index ffaeda18..d1426979 100644
--- a/docs/PLUGIN_REGISTRY_SETUP_GUIDE.md
+++ b/docs/PLUGIN_REGISTRY_SETUP_GUIDE.md
@@ -333,7 +333,7 @@ python3 scripts/setup_plugin_repos.py
 python3 scripts/audit_plugins.py
 
 # Validate a single plugin
-python3 scripts/check_plugin.py <plugin-id>
+python3 scripts/check_plugin.py --plugin <plugin-id>
 ```
 
 Registry regeneration (`update_registry.py`) lives in the
diff --git a/scripts/fix_perms/fix_assets_permissions.sh b/scripts/fix_perms/fix_assets_permissions.sh
old mode 100644
new mode 100755
diff --git a/scripts/fix_perms/fix_cache_permissions.sh b/scripts/fix_perms/fix_cache_permissions.sh
old mode 100644
new mode 100755
diff --git a/scripts/fix_perms/fix_plugin_permissions.sh b/scripts/fix_perms/fix_plugin_permissions.sh
old mode 100644
new mode 100755
diff --git a/scripts/fix_perms/fix_web_permissions.sh b/scripts/fix_perms/fix_web_permissions.sh
old mode 100644
new mode 100755
diff --git a/scripts/install_dependencies_apt.py b/scripts/install_dependencies_apt.py
index a913b6ee..52631e8d 100644
--- a/scripts/install_dependencies_apt.py
+++ b/scripts/install_dependencies_apt.py
@@ -44,7 +44,7 @@ def install_via_apt(package_name: str) -> Tuple[bool, str]:
     apt_package_map = {
         'flask': 'python3-flask',
         'PIL': 'python3-pil',
-        'freetype': 'python3-freetype',
+        'freetype-py': 'python3-freetype',
         'psutil': 'python3-psutil',
         'werkzeug': 'python3-werkzeug',
         'numpy': 'python3-numpy',
@@ -95,7 +95,9 @@ def install_via_pip(package_name: str) -> Tuple[bool, str]:
 
 
 # Distribution (pip/apt) names whose importable module name differs.
-IMPORT_NAME_MAP = {}
+IMPORT_NAME_MAP = {
+    'freetype-py': 'freetype',
+}
 
 
 def check_package_installed(package_name: str) -> bool:
@@ -139,7 +141,7 @@ def main():
     required_packages = [
         'flask',
         'PIL',
-        'freetype',
+        'freetype-py',
         'psutil',
         'werkzeug',
         'numpy',
diff --git a/test/test_doc_links.py b/test/test_doc_links.py
index ac95ea37..08f904af 100644
--- a/test/test_doc_links.py
+++ b/test/test_doc_links.py
@@ -6,20 +6,23 @@ links inside fenced code blocks.
 """
 import re
 from pathlib import Path
+from typing import Iterator
 
 PROJECT_ROOT = Path(__file__).resolve().parent.parent
 LINK_RE = re.compile(r'\[[^\]]*\]\(([^)\s]+)\)')
 FENCE_RE = re.compile(r'^(```|~~~)')
 
 
-def _md_files():
+def _md_files() -> Iterator[Path]:
+    """Yield active markdown files (repo root + docs/, excluding docs/archive/)."""
     yield from PROJECT_ROOT.glob('*.md')
     for path in PROJECT_ROOT.glob('docs/**/*.md'):
         if 'archive' not in path.parts:
             yield path
 
 
-def test_relative_markdown_links_resolve():
+def test_relative_markdown_links_resolve() -> None:
+    """Every relative markdown link outside code fences must resolve on disk."""
     broken = []
     for md in _md_files():
         in_fence = False
diff --git a/test/test_template_targets.py b/test/test_template_targets.py
index 4a26d9ff..a720e92e 100644
--- a/test/test_template_targets.py
+++ b/test/test_template_targets.py
@@ -6,28 +6,32 @@ templates were removed when those displays became plugins).
 """
 import re
 from pathlib import Path
+from typing import Iterator
 
 PROJECT_ROOT = Path(__file__).resolve().parent.parent
 TEMPLATE_ROOT = PROJECT_ROOT / 'web_interface' / 'templates'
 RENDER_RE = re.compile(r"""render_template\(\s*['"]([^'"]+)['"]""")
 
 
-def _python_sources():
+def _python_sources() -> Iterator[Path]:
+    """Yield every Python source that can call render_template()."""
     yield PROJECT_ROOT / 'web_interface' / 'app.py'
     yield from (PROJECT_ROOT / 'web_interface' / 'blueprints').glob('*.py')
 
 
-def test_all_literal_render_template_targets_exist():
+def test_all_literal_render_template_targets_exist() -> None:
+    """Every string-literal render_template() target must exist on disk."""
     missing = []
     for source in _python_sources():
         text = source.read_text(encoding='utf-8')
-        for lineno, line in enumerate(text.splitlines(), 1):
-            for target in RENDER_RE.findall(line):
-                if not (TEMPLATE_ROOT / target).is_file():
-                    missing.append(
-                        f'{source.relative_to(PROJECT_ROOT)}:{lineno} -> {target}'
-                    )
+        for match in RENDER_RE.finditer(text):
+            target = match.group(1)
+            if not (TEMPLATE_ROOT / target).is_file():
+                lineno = text.count('\n', 0, match.start()) + 1
+                missing.append(
+                    f'{source.relative_to(PROJECT_ROOT)}:{lineno} -> {target}'
+                )
     assert not missing, (
         'render_template() references templates that do not exist under '
-        f'web_interface/templates/:\n' + '\n'.join(missing)
+        'web_interface/templates/:\n' + '\n'.join(missing)
     )
diff --git a/test/test_widget_scripts.py b/test/test_widget_scripts.py
index 8c07a8b0..58e949b3 100644
--- a/test/test_widget_scripts.py
+++ b/test/test_widget_scripts.py
@@ -6,12 +6,19 @@ config schema declares that widget (the field renders as an empty container
 that polls the registry forever). base.html's widget list is maintained by
 hand, so this test keeps it honest.
 """
+import re
 from pathlib import Path
+from typing import Set
 
 PROJECT_ROOT = Path(__file__).resolve().parent.parent
 WIDGETS_DIR = PROJECT_ROOT / 'web_interface' / 'static' / 'v3' / 'js' / 'widgets'
 BASE_HTML = PROJECT_ROOT / 'web_interface' / 'templates' / 'v3' / 'base.html'
 
+# Matches url_for('static', filename='...') inside actual <script> tags.
+SCRIPT_SRC_RE = re.compile(
+    r"""<script\s[^>]*src="\{\{\s*url_for\(\s*'static'\s*,\s*filename='([^']+)'\s*\)\s*\}\}[^"]*"""
+)
+
 # Files that must NOT be script-included, with the reason.
 ALLOWLIST = {
     # Documentation example (docs/widget-guide.md); registers the name
@@ -20,14 +27,26 @@ ALLOWLIST = {
 }
 
 
-def test_every_widget_script_is_included_in_base_html():
+def _included_widget_scripts() -> Set[str]:
+    """Return widget JS basenames referenced by real <script> tags in base.html."""
     base_html = BASE_HTML.read_text(encoding='utf-8')
-    missing = []
-    for js_file in sorted(WIDGETS_DIR.glob('*.js')):
-        if js_file.name in ALLOWLIST:
-            continue
-        if f'v3/js/widgets/{js_file.name}' not in base_html:
-            missing.append(js_file.name)
+    return {
+        Path(filename).name
+        for filename in SCRIPT_SRC_RE.findall(base_html)
+        if filename.startswith('v3/js/widgets/')
+    }
+
+
+def test_every_widget_script_is_included_in_base_html() -> None:
+    """Every non-allowlisted widget file must be loaded by a <script> tag."""
+    assert WIDGETS_DIR.is_dir(), f'Widget directory missing: {WIDGETS_DIR}'
+    included = _included_widget_scripts()
+    assert included, 'No widget <script> tags found in base.html — regex or template drift?'
+    missing = [
+        js_file.name
+        for js_file in sorted(WIDGETS_DIR.glob('*.js'))
+        if js_file.name not in ALLOWLIST and js_file.name not in included
+    ]
     assert not missing, (
         'Widget files exist but are never <script>-included in base.html '
         '(plugins declaring these widgets get blank config fields): '
@@ -37,11 +56,10 @@ def test_every_widget_script_is_included_in_base_html():
     )
 
 
-def test_allowlisted_widgets_are_not_included():
-    base_html = BASE_HTML.read_text(encoding='utf-8')
-    wrongly_included = [
-        name for name in ALLOWLIST if f'v3/js/widgets/{name}' in base_html
-    ]
+def test_allowlisted_widgets_are_not_included() -> None:
+    """Allowlisted (must-not-load) widget files must stay out of base.html."""
+    included = _included_widget_scripts()
+    wrongly_included = [name for name in ALLOWLIST if name in included]
     assert not wrongly_included, (
         'Allowlisted (must-not-load) widget files are script-included in '
         'base.html: ' + ', '.join(wrongly_included)
diff --git a/test/web_interface/test_cache.py b/test/web_interface/test_cache.py
index 98986b0c..46f6b0ee 100644
--- a/test/web_interface/test_cache.py
+++ b/test/web_interface/test_cache.py
@@ -1,36 +1,44 @@
 """Tests for the web interface's in-memory cache helpers."""
+from typing import Iterator
+
 import pytest
 
 from web_interface.cache import delete_cached, get_cached, invalidate_cache, set_cached
 
 
 @pytest.fixture(autouse=True)
-def clean_cache():
+def clean_cache() -> Iterator[None]:
+    """Start and finish every test with an empty cache."""
     invalidate_cache()
     yield
     invalidate_cache()
 
 
-def test_set_and_get():
+def test_set_and_get() -> None:
+    """A cached value is returned before its TTL expires."""
     set_cached('key', 'value')
     assert get_cached('key') == 'value'
 
 
-def test_get_missing_returns_none():
+def test_get_missing_returns_none() -> None:
+    """Reading an unknown key returns None."""
     assert get_cached('missing') is None
 
 
-def test_delete_cached_removes_key():
+def test_delete_cached_removes_key() -> None:
+    """delete_cached removes exactly the named key."""
     set_cached('fonts_catalog', ['a-font'])
     delete_cached('fonts_catalog')
     assert get_cached('fonts_catalog') is None
 
 
-def test_delete_cached_missing_key_is_noop():
-    delete_cached('never-set')  # must not raise
+def test_delete_cached_missing_key_is_noop() -> None:
+    """Deleting a key that was never set must not raise."""
+    delete_cached('never-set')
 
 
-def test_invalidate_cache_pattern():
+def test_invalidate_cache_pattern() -> None:
+    """Pattern invalidation removes matching keys and keeps the rest."""
     set_cached('fonts_catalog', 1)
     set_cached('plugins_list', 2)
     invalidate_cache('fonts')
diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py
index cd3a8cd2..aea7d592 100644
--- a/web_interface/blueprints/api_v3.py
+++ b/web_interface/blueprints/api_v3.py
@@ -838,6 +838,10 @@ def save_main_config():
                 except (ValueError, TypeError):
                     return jsonify({'status': 'error', 'message': f"Invalid multiplexing value '{data['multiplexing']}'. Must be an integer from 0 to 22."}), 400
 
+            # Validate pixel_mapper_config (free-form mapper string, e.g. "U-mapper;Rotate:90")
+            if 'pixel_mapper_config' in data and not isinstance(data['pixel_mapper_config'], str):
+                return jsonify({'status': 'error', 'message': 'pixel_mapper_config must be a string (e.g. "U-mapper;Rotate:90" or empty)'}), 400
+
             # Validate row_address_type
             if 'row_address_type' in data:
                 try: