feat(plugins): adaptive layout, element style, and dev preview tooling#396
Closed
ChuckBuilds wants to merge 4 commits into
Closed
feat(plugins): adaptive layout, element style, and dev preview tooling#396ChuckBuilds wants to merge 4 commits into
ChuckBuilds wants to merge 4 commits into
Conversation
Upstreams a substantial body of local work found on this device that had never been committed or pushed. Verified functionally sound (see test plan) before packaging; not authored this session. ## Adaptive layout (src/adaptive_layout.py, src/adaptive_images.py) Region/LayoutContext geometry system so plugins can carve their panel into regions and fit text/images to them instead of hardcoding coordinates per panel size. Wired into BasePlugin as self.layout (lazily built, cached, and invalidated on display-size or font-cache-generation change) and draw_fit(text, box, ladder=...) for size-fitted text. src/image_utils.py (the old fit-image helper) is marked deprecated in favor of adaptive_images.fit_image. ## Element style (src/element_style.py) Universal per-element style resolution for plugin customization (config['customization']), replacing four divergent hand-rolled implementations across the sports/music plugins. Centralizes font loading, x/y-offset reading, and the did the user actually override this? check -- subtle enough that it previously shipped broken twice, since a key being present in config never means the user set it (schema_manager. merge_with_defaults and the plugin manager both write full schema defaults into config before a plugin ever sees it). BasePlugin.element_style() wires this to a plugin's own config_schema.json defaults via SchemaManager. schema/manifest_schema.json and schema_manager.py gained the declarative generated element expansion (a customization block auto-populated with font/color/offset sub-keys from a short declaration) backing this. ## Testing harness - bounds_display_manager.py: records negative-coordinate (left/top overflow) draw calls -- previously undetectable, since PIL clips them silently. - harness.py/sizes.py: a design_size-driven fill check (a panel >= 2x a plugin's declared design size must not be left mostly empty -- the scale-up counterpart to the existing overflow check), a new 96x48 (non-64x32-grid) test size, and support for check_plugin.py variants (e.g. testing a plugin with adaptive-layout mode enabled alongside its classic default, each against its own golden dir). - render_service.py: shared single-plugin-render logic factored out for reuse by the new web UI preview endpoint below. ## Web UI live preview (web_interface/blueprints/api_v3.py, pages_v3.py, plugin_config.html; scripts/dev_server.py, templates/dev_preview.html) New POST /api/v3/plugins/<id>/preview endpoint (and dev_server.py's equivalent standalone route) renders a plugin's current in-progress config at a given panel size without saving it, so the Plugin Manager UI's config form can show a live preview. dev_preview.html gained a size-preset dropdown plus an All Sizes button that renders the current config across every harness size in a side-by-side gallery. api_v3.py's config-form parsing helpers (fix_array_structures, ensure_array_defaults) were promoted from nested closures to module-level functions so the new preview endpoint can reuse the exact same parsing the save endpoint uses. ## Test plan - Ran every new/touched test file on this device (which has the real RGBMatrixEmulator/plugin-repos environment, not a scratch subset): 187 passed across test_adaptive_layout.py, test_adaptive_images.py, test_element_style.py, test_schema_style_expansion.py, test_harness_fill.py, and test/web_interface/test_plugin_preview.py. - Ran the full existing test/ suite (1024+ tests) to check for regressions: no new failures. The failures present (test_circuit_breaker, test_save_double_sided_settings, test_save_double_sided_unchecked_disables, and two test_state_reconciliation.py cases) were confirmed to reproduce identically against a clean origin/main checkout in an isolated worktree, so they predate and are unrelated to this change. - Did not exhaustively line-by-line review all ~5000 lines as a fresh design review; relied on the above test evidence plus a structural read of each file's stated purpose.
📝 WalkthroughWalkthroughChangesAdaptive layout and plugin preview
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try: | ||
| plugin_dir, manifest, config, mock_data, skip_update = _parse_render_request(data) | ||
| except LookupError as e: | ||
| return jsonify({'error': str(e)}), 404 |
| }) | ||
| plugin_dir, manifest, config, mock_data, skip_update = _parse_render_request(data) | ||
| except LookupError as e: | ||
| return jsonify({'error': str(e)}), 404 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 313 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
CodeQL flagged 2 high-severity path-injection alerts and 7 medium stack-trace-exposure alerts introduced by this PR. Path injection (PluginLoader.find_plugin_directory, Strategy 2): replaced the resolve()+relative_to() containment check with find_trusted_subdir() (already used elsewhere in this file since #390) -- a name enumerated directly from plugins_dir via scandir() carries no taint regardless of what the caller's plugin_id string was, which CodeQL's path-injection query recognises; a post-hoc containment check on a path built from the tainted string apparently isn't, matching what #390 found for a similar pattern. Stack trace exposure: render_service.py's update()/display() exception handlers embedded the raw exception message directly into the response returned to the preview endpoint's caller. Showing *something* here is the point of the endpoint (it's the plugin author's own code failing, not a server secret), but the message was otherwise unbounded and could echo back the server's own directory layout (e.g. a FileNotFoundError embeds the full path it tried). Added _safe_exc_message(): collapses absolute paths to their basename and caps length, with the full exception (traceback included) now logged server-side via logger.warning(..., exc_info=True) before the sanitized version reaches the client. Applied the same helper in dev_server.py's two matching load-failure handlers; web_interface/blueprints/api_v3.py's HTML preview fragment (the other 2 flagged locations) consumes the same result['errors']/['warnings'] fields, so fixing the shared source in render_service.py covers it too -- confirmed it's the only other consumer. Verified: existing test/web_interface/test_plugin_preview.py assertions only check for an empty errors list on success, not exact failure-message content, so no test changes needed. Full test/test_plugin_loader.py + test/test_plugin_system.py: 31 passed, 1 pre-existing failure (test_circuit_breaker, unrelated).
…eQL sinks Round 2. The previous commit's _safe_exc_message() (regex-redact absolute paths, cap length) didn't clear the stack-trace-exposure alerts -- same lesson as PR #390's permission_utils.py fix: CodeQL doesn't trust a custom transformation function as a sanitiser, regardless of what it actually does. Removed the helper entirely; render_service.py/dev_server.py's update()/display()/load-failure handlers now surface only the exception's class name (a fixed, bounded string, never derived from the exception's own content) to the client, with the full exception (traceback included) still logged server-side via logger.warning(..., exc_info=True). Fixing the path-injection alert in find_plugin_directory also had a side effect: CodeQL's interprocedural analysis now traces further downstream through that changed function, surfacing 5 new clear-text-logging-of- secrets alerts in code that mostly predates this PR (scripts/render_plugin.py has zero other changes in this diff) -- logging plugin_id/file paths that originate from CLI args or discovered plugin directories. Not real secrets, but CodeQL's model doesn't distinguish "externally-influenced string" from "credential" here. Removed the path/plugin_id values from the 5 flagged log lines (check_plugin.py, render_plugin.py, plugin_loader.py x3), keeping a plugin_id-only or fully generic message where one was already available elsewhere in the same log line's context. Verified: full test/test_plugin_loader.py + test/test_plugin_system.py + the adaptive-layout/element-style/preview suites, no test asserts on the specific log/error message text that changed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/font_manager.py (1)
106-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
clear_cache()doesn't bumpcache_generation, breaking the stated invalidation contract.The docstring for
cache_generationsays it's "Bumped whenever cached font objects are invalidated, so holders of derived caches ... know to rebuild," but onlyreload_config()increments it.clear_cache()itself — called byset_override,remove_override,add_font, andremove_font— clearsfont_cache/metrics_cachewithout touchingcache_generation. Any of these runtime calls will silently leaveBasePlugin.layout's cachedLayoutContext(and its_fit_cache) unaware that fonts were invalidated, since its staleness check isgetattr(font_manager, "cache_generation", 0).🔧 Suggested fix — centralize the bump in `clear_cache()`
def clear_cache(self): """Clear font and metrics cache.""" self.font_cache.clear() self.metrics_cache.clear() + self.cache_generation += 1 logger.info("Font cache cleared")def reload_config(self, new_config: Dict[str, Any]): """Reload configuration and refresh font catalog.""" self.config = new_config self.fonts_config = new_config.get("fonts", {}) - self.font_cache.clear() # Clear cache to force reload - self.metrics_cache.clear() # Clear metrics cache - self.cache_generation += 1 + self.clear_cache() # clears font/metrics caches and bumps cache_generation self._initialize_fonts() logger.info("FontManager configuration reloaded successfully")Also applies to: 712-716
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/font_manager.py` around lines 106 - 120, Update FontManager.clear_cache() to increment cache_generation whenever it clears font_cache and metrics_cache, centralizing the invalidation bump there. Remove the duplicate increment from reload_config() so configuration reloads do not increment twice, while preserving its cache clearing and font reinitialization behavior.
🧹 Nitpick comments (6)
src/plugin_system/testing/harness.py (1)
328-341: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
histogram()instead of a Python-level pixel loop for the ink ratio.
sum(1 for p in lit.getdata() if p)iterates every pixel in pure Python. Sincelitis already binarized to 0/255 in "L" mode,histogram()[255](orhistogram()[-1]) gives the same count via Pillow's C implementation.⚡ Proposed optimization
extent_x = (bbox[2] - bbox[0]) / image.width extent_y = (bbox[3] - bbox[1]) / image.height - ink = sum(1 for p in lit.getdata() if p) / (image.width * image.height) + ink = lit.histogram()[-1] / (image.width * image.height) return (extent_x, extent_y, ink)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/testing/harness.py` around lines 328 - 341, Update fill_metrics to compute the lit-pixel count for ink using lit.histogram()[255] instead of the Python-level sum over lit.getdata(), while preserving the existing normalization by image.width * image.height and all other metric behavior.src/plugin_system/testing/bounds_display_manager.py (1)
62-76: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEither wire
negative_coordinate_callsinto a test assertion or document it as test-only state.
- The harness/tests only consume
check_overflow()→RenderResult.overflow/ok; nothing readsnegative_coordinate_calls.- Add type hints to
draw_text()anddraw_image()to match the rest of the file and the Python guidelines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/testing/bounds_display_manager.py` around lines 62 - 76, Update the bounds-display test harness around negative_coordinate_calls: either expose and consume it through a test assertion, or explicitly document the attribute as test-only state. Also add type annotations to draw_text and draw_image consistent with the surrounding methods and project Python guidelines, while preserving their existing recording and delegation behavior.Source: Coding guidelines
docs/ADAPTIVE_LAYOUT.md (1)
21-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuick-start example omits
display_manager.clear().The sample
display()implementation never callsself.display_manager.clear()before drawing, which contradicts the project's own convention (clear()before render,update_display()after). Plugin authors copying this snippet verbatim will skip clearing and risk stale-frame artifacts.📝 Suggested fix
def display(self, force_clear=False): from src.adaptive_layout import LADDER_ARCADE + self.display_manager.clear() b = self.layout.bounds.inset(1) # Region(0,0,W,H) minus 1px margin rows = b.split_v(3, 1, 1, gap=1) # 3/5 for time, 1/5 each for the restAs per coding guidelines, "Use display_manager.clear() before rendering and display_manager.update_display() after rendering in display() method."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ADAPTIVE_LAYOUT.md` around lines 21 - 32, Update the quick-start display method example to call self.display_manager.clear() before drawing any regions, while preserving the existing draw_fit calls and the final update_display() call.Source: Coding guidelines
src/font_manager.py (1)
514-539: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a debug log on parse failure in
_read_bdf_native_size.On
OSError/ValueErrorthis silently returnsNone, giving no trace for troubleshooting a malformed/missing BDF file on a deployed Pi.As per coding guidelines, "Implement comprehensive logging for remote debugging on Raspberry Pi" and "Provide clear error messages for troubleshooting."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/font_manager.py` around lines 514 - 539, Update _read_bdf_native_size to log a debug-level message when an OSError or ValueError occurs while opening or parsing the BDF file, including the bdf_path and exception details; preserve the existing None return behavior after logging.Source: Coding guidelines
src/common/logo_helper.py (1)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate default-size resolution logic.
The
max_width = int(self.display_width * 1.5)/max_height = int(self.display_height * 1.5)fallback is now duplicated inload_logo,_resize_logo, and_create_placeholder_logo. Consider extracting a small_default_max_size()helper to keep the three in sync going forward.Also applies to: 225-231, 285-289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/logo_helper.py` around lines 84 - 88, Extract the shared default max-size calculation from load_logo, _resize_logo, and _create_placeholder_logo into a _default_max_size() helper, returning the display dimensions scaled by 1.5. Replace each duplicated width/height fallback with this helper while preserving the existing explicit max_width and max_height behavior.test/test_adaptive_layout.py (1)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer unpacking over list concatenation.
- `@pytest.mark.parametrize`("w,h", DEFAULT_TEST_SIZES + [(8, 8)]) + `@pytest.mark.parametrize`("w,h", [*DEFAULT_TEST_SIZES, (8, 8)])As per static analysis hints (Ruff RUF005).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_adaptive_layout.py` at line 90, Update the parameter list in the test’s pytest.mark.parametrize decorator to use iterable unpacking instead of list concatenation, preserving all DEFAULT_TEST_SIZES entries and the additional (8, 8) case.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_plugin.py`:
- Around line 114-162: Make unnamed variants unique in the variant-processing
loop around `name = variant.get("name") or "variant"` by deriving a
deterministic identifier from each variant’s position, while preserving explicit
names unchanged. Use that identifier consistently for the default `golden_dir`
and the `@...` mode suffix so multiple unnamed variants cannot collide, and
ensure duplicate or otherwise invalid variant identities fail loudly rather than
overwriting outputs.
In `@scripts/dev_server.py`:
- Around line 195-212: Update _parse_render_request so data.get('skip_update',
...) defaults to True, ensuring requests that omit skip_update—including gallery
previews—skip live update calls. Preserve explicit caller-provided skip_update
values.
- Around line 264-276: Update the size-validation loop around parsed_sizes so
every entry must be a list (or expected array type) containing exactly two items
before indexing it; reject strings, non-list containers, and incorrect lengths
with the existing 400 invalid-size response. Preserve integer conversion and
dimension-bound checks for valid [w, h] entries.
In `@scripts/templates/dev_preview.html`:
- Around line 568-603: Update the gallery rendering in update() to read
r.warnings, display warning messages alongside each result, and include the
warning count in the status text. Keep failures based on r.errors or missing
images, while ensuring results with warnings are visibly marked as stale or
incomplete without being counted as hard failures.
In `@src/adaptive_layout.py`:
- Around line 429-452: Update _walk_ladder to explicitly reject an empty ladder
before iterating, raising a clear ValueError consistent with the existing
fit_text_proportional behavior. Preserve the current FitResult selection and
ellipsis logic for non-empty ladders, and ensure callers such as fit_text and
fit_lines no longer receive None.
In `@src/element_style.py`:
- Around line 167-198: Update expand_style_elements to preserve the original
input schema in a separate reference before rebinding schema to the deep copy,
and return that original reference from the except handler. Keep successful
expansion returning the copied schema while ensuring any exception during
_style_element_block or _expand_offset_blocks cannot expose partial
modifications.
In `@src/plugin_system/testing/mocks.py`:
- Around line 164-170: The test harness should fail fast when initializing the
real FontManager instead of silently assigning None. Update the FontManager
setup in the mock class initializer to remove the broad exception fallback,
allowing initialization errors from missing assets or dependencies to propagate
and preserve production-equivalent layout behavior.
In `@src/plugin_system/testing/render_service.py`:
- Around line 76-85: Update the exception handling around
plugin_instance.update() and plugin_instance.display() to log full tracebacks
server-side while replacing the returned exception text with stable, generic
user-facing messages. Preserve separate handling for update and display
failures, and use the existing server logging mechanism rather than exposing
exception details through warnings or errors.
In `@test/web_interface/test_plugin_preview.py`:
- Around line 156-161: Update the fixture plugin used by
test_disabled_plugin_still_previews so it raises or produces no output when
disabled, making the test sensitive to the preview forcing enabled behavior.
Keep the test’s successful 200 response and empty errors assertions, and add an
assertion that the preview rendered successfully.
In `@web_interface/blueprints/api_v3.py`:
- Around line 4321-4329: Update the bracket-array handling around
_set_nested_value to coerce each filtered value according to the array schema’s
items.type, matching the save-path normalization behavior. Reuse an existing
shared normalization helper if available; otherwise apply the schema-driven
conversion before assigning plugin_config so preview and saved results remain
consistent.
- Around line 4643-4648: Remove the schema-wide _set_missing_booleans_to_false
call from the form-processing flow. Update rendered checkbox inputs to submit an
explicit hidden false sentinel when unchecked, and make the request-handling
logic update only boolean fields whose corresponding form key is present,
preserving omitted values for partial, conditional, or disabled fields.
- Around line 5310-5328: Update _find_plugin_dir_for_preview to reject plugin_id
values containing path separators or traversal components before constructing
candidate paths. For each approved root, resolve the candidate and verify it is
exactly a direct child of the resolved base directory before accepting an
existing manifest.json, returning None for invalid or escaping identifiers.
- Around line 4285-4295: Update parse_plugin_config_form so it creates an
independent deep copy of existing_config before applying any form updates,
rather than assigning the caller’s object directly to plugin_config. Preserve
the existing merge, coercion, and return behavior while ensuring nested
configuration values remain unchanged when parsing previews or failed saves.
In `@web_interface/templates/v3/partials/plugin_config.html`:
- Around line 1013-1019: Add the supported 96×48 preset to the “My panel size”
select options in the plugin configuration template, keeping the existing size
choices unchanged; if a shared size-definition source is already available,
reuse it to keep validation and preview options synchronized.
---
Outside diff comments:
In `@src/font_manager.py`:
- Around line 106-120: Update FontManager.clear_cache() to increment
cache_generation whenever it clears font_cache and metrics_cache, centralizing
the invalidation bump there. Remove the duplicate increment from reload_config()
so configuration reloads do not increment twice, while preserving its cache
clearing and font reinitialization behavior.
---
Nitpick comments:
In `@docs/ADAPTIVE_LAYOUT.md`:
- Around line 21-32: Update the quick-start display method example to call
self.display_manager.clear() before drawing any regions, while preserving the
existing draw_fit calls and the final update_display() call.
In `@src/common/logo_helper.py`:
- Around line 84-88: Extract the shared default max-size calculation from
load_logo, _resize_logo, and _create_placeholder_logo into a _default_max_size()
helper, returning the display dimensions scaled by 1.5. Replace each duplicated
width/height fallback with this helper while preserving the existing explicit
max_width and max_height behavior.
In `@src/font_manager.py`:
- Around line 514-539: Update _read_bdf_native_size to log a debug-level message
when an OSError or ValueError occurs while opening or parsing the BDF file,
including the bdf_path and exception details; preserve the existing None return
behavior after logging.
In `@src/plugin_system/testing/bounds_display_manager.py`:
- Around line 62-76: Update the bounds-display test harness around
negative_coordinate_calls: either expose and consume it through a test
assertion, or explicitly document the attribute as test-only state. Also add
type annotations to draw_text and draw_image consistent with the surrounding
methods and project Python guidelines, while preserving their existing recording
and delegation behavior.
In `@src/plugin_system/testing/harness.py`:
- Around line 328-341: Update fill_metrics to compute the lit-pixel count for
ink using lit.histogram()[255] instead of the Python-level sum over
lit.getdata(), while preserving the existing normalization by image.width *
image.height and all other metric behavior.
In `@test/test_adaptive_layout.py`:
- Line 90: Update the parameter list in the test’s pytest.mark.parametrize
decorator to use iterable unpacking instead of list concatenation, preserving
all DEFAULT_TEST_SIZES entries and the additional (8, 8) case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 537e8a5f-8413-414b-b4d4-54e387e2f172
📒 Files selected for processing (37)
docs/ADAPTIVE_LAYOUT.mddocs/ADVANCED_PLUGIN_DEVELOPMENT.mddocs/DEVELOPER_QUICK_REFERENCE.mddocs/DEV_PREVIEW.mddocs/FONT_MANAGER.mddocs/PLUGIN_API_REFERENCE.mddocs/PLUGIN_DEVELOPMENT_GUIDE.mdschema/manifest_schema.jsonscripts/check_plugin.pyscripts/dev_server.pyscripts/templates/dev_preview.htmlsrc/adaptive_images.pysrc/adaptive_layout.pysrc/common/README.mdsrc/common/__init__.pysrc/common/logo_helper.pysrc/common/text_helper.pysrc/element_style.pysrc/font_manager.pysrc/image_utils.pysrc/plugin_system/base_plugin.pysrc/plugin_system/schema_manager.pysrc/plugin_system/testing/bounds_display_manager.pysrc/plugin_system/testing/harness.pysrc/plugin_system/testing/loading.pysrc/plugin_system/testing/mocks.pysrc/plugin_system/testing/render_service.pysrc/plugin_system/testing/sizes.pytest/test_adaptive_images.pytest/test_adaptive_layout.pytest/test_element_style.pytest/test_harness_fill.pytest/test_schema_style_expansion.pytest/web_interface/test_plugin_preview.pyweb_interface/blueprints/api_v3.pyweb_interface/blueprints/pages_v3.pyweb_interface/templates/v3/partials/plugin_config.html
Comment on lines
+114
to
+162
| # The plugin's declared design size drives the scale-up fill check | ||
| # (panels >= 2x the design size must not be left mostly empty). | ||
| declared = load_manifest(plugin_dir).get("display", {}).get("design_size", {}) | ||
| design_size = (int(declared.get("width", 128)), int(declared.get("height", 32))) | ||
| fill_strict = spec.get("fill_check") == "strict" | ||
|
|
||
| # Every run: the base config, plus one per harness.json "variant" — | ||
| # a config overlay with its own golden dir (e.g. adaptive layout mode | ||
| # tested alongside the classic default). | ||
| runs = [(None, {}, golden_dir_override or (plugin_dir / 'test' / 'golden'))] | ||
| for variant in spec.get("variants", []): | ||
| name = variant.get("name") or "variant" | ||
| vdir = plugin_dir / variant.get("golden_dir", f"test/golden-{name}") | ||
| runs.append((name, variant.get("config", {}), vdir)) | ||
|
|
||
| all_run_results: List[RenderResult] = [] | ||
| for variant_name, overlay, golden_dir in runs: | ||
| run_config = {**full_config, **overlay} | ||
| results = render_plugin_matrix( | ||
| plugin_id=plugin_id, plugin_dir=plugin_dir, config=run_config, | ||
| mock_data=effective_mock_data, sizes=effective_sizes, | ||
| run_update=effective_run_update, freeze_time=effective_freeze, | ||
| ) | ||
|
|
||
| golden_dir = golden_dir_override or (plugin_dir / 'test' / 'golden') | ||
| if update_golden: | ||
| written = write_goldens(results, golden_dir) | ||
| logger.info("Wrote %d golden image(s) for %s to %s", written, plugin_id, golden_dir) | ||
| else: | ||
| compare_to_goldens(results, golden_dir) | ||
| if update_golden: | ||
| written = write_goldens(results, golden_dir) | ||
| logger.info("Wrote %d golden image(s) for %s%s to %s", written, plugin_id, | ||
| f" [{variant_name}]" if variant_name else "", golden_dir) | ||
| else: | ||
| compare_to_goldens(results, golden_dir) | ||
|
|
||
| if out_dir: | ||
| for r in results: | ||
| if r.image is None: | ||
| continue | ||
| dest = out_dir / plugin_id / size_label(r.width, r.height) | ||
| dest.mkdir(parents=True, exist_ok=True) | ||
| r.image.save(dest / f"{safe_mode_filename(r.mode)}.png", format="PNG") | ||
| check_scale_up(results, design_size=design_size, strict=fill_strict) | ||
|
|
||
| # Tag variant runs so the report and PNG dumps stay distinguishable. | ||
| if variant_name: | ||
| for r in results: | ||
| r.mode = f"{r.mode}@{variant_name}" | ||
|
|
||
| return results | ||
| if out_dir: | ||
| for r in results: | ||
| if r.image is None: | ||
| continue | ||
| dest = out_dir / plugin_id / size_label(r.width, r.height) | ||
| dest.mkdir(parents=True, exist_ok=True) | ||
| r.image.save(dest / f"{safe_mode_filename(r.mode)}.png", format="PNG") | ||
|
|
||
| all_run_results.extend(results) | ||
|
|
||
| return all_run_results |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Unnamed variants collide on golden dir / mode suffix.
name = variant.get("name") or "variant" defaults every un-named entry to the literal "variant". If a plugin's harness.json declares two or more variants without an explicit "name", they all resolve to the same golden_dir (test/golden-variant) and the same mode suffix (@variant) — later runs silently overwrite the earlier run's golden images/PNG dumps (or compare against the wrong golden), instead of failing loudly the way load_harness_spec otherwise intends for misconfiguration.
🐛 Proposed fix to make unnamed variants unique
- for variant in spec.get("variants", []):
- name = variant.get("name") or "variant"
+ for idx, variant in enumerate(spec.get("variants", [])):
+ name = variant.get("name") or f"variant{idx}"
vdir = plugin_dir / variant.get("golden_dir", f"test/golden-{name}")
runs.append((name, variant.get("config", {}), vdir))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # The plugin's declared design size drives the scale-up fill check | |
| # (panels >= 2x the design size must not be left mostly empty). | |
| declared = load_manifest(plugin_dir).get("display", {}).get("design_size", {}) | |
| design_size = (int(declared.get("width", 128)), int(declared.get("height", 32))) | |
| fill_strict = spec.get("fill_check") == "strict" | |
| # Every run: the base config, plus one per harness.json "variant" — | |
| # a config overlay with its own golden dir (e.g. adaptive layout mode | |
| # tested alongside the classic default). | |
| runs = [(None, {}, golden_dir_override or (plugin_dir / 'test' / 'golden'))] | |
| for variant in spec.get("variants", []): | |
| name = variant.get("name") or "variant" | |
| vdir = plugin_dir / variant.get("golden_dir", f"test/golden-{name}") | |
| runs.append((name, variant.get("config", {}), vdir)) | |
| all_run_results: List[RenderResult] = [] | |
| for variant_name, overlay, golden_dir in runs: | |
| run_config = {**full_config, **overlay} | |
| results = render_plugin_matrix( | |
| plugin_id=plugin_id, plugin_dir=plugin_dir, config=run_config, | |
| mock_data=effective_mock_data, sizes=effective_sizes, | |
| run_update=effective_run_update, freeze_time=effective_freeze, | |
| ) | |
| golden_dir = golden_dir_override or (plugin_dir / 'test' / 'golden') | |
| if update_golden: | |
| written = write_goldens(results, golden_dir) | |
| logger.info("Wrote %d golden image(s) for %s to %s", written, plugin_id, golden_dir) | |
| else: | |
| compare_to_goldens(results, golden_dir) | |
| if update_golden: | |
| written = write_goldens(results, golden_dir) | |
| logger.info("Wrote %d golden image(s) for %s%s to %s", written, plugin_id, | |
| f" [{variant_name}]" if variant_name else "", golden_dir) | |
| else: | |
| compare_to_goldens(results, golden_dir) | |
| if out_dir: | |
| for r in results: | |
| if r.image is None: | |
| continue | |
| dest = out_dir / plugin_id / size_label(r.width, r.height) | |
| dest.mkdir(parents=True, exist_ok=True) | |
| r.image.save(dest / f"{safe_mode_filename(r.mode)}.png", format="PNG") | |
| check_scale_up(results, design_size=design_size, strict=fill_strict) | |
| # Tag variant runs so the report and PNG dumps stay distinguishable. | |
| if variant_name: | |
| for r in results: | |
| r.mode = f"{r.mode}@{variant_name}" | |
| return results | |
| if out_dir: | |
| for r in results: | |
| if r.image is None: | |
| continue | |
| dest = out_dir / plugin_id / size_label(r.width, r.height) | |
| dest.mkdir(parents=True, exist_ok=True) | |
| r.image.save(dest / f"{safe_mode_filename(r.mode)}.png", format="PNG") | |
| all_run_results.extend(results) | |
| return all_run_results | |
| # The plugin's declared design size drives the scale-up fill check | |
| # (panels >= 2x the design size must not be left mostly empty). | |
| declared = load_manifest(plugin_dir).get("display", {}).get("design_size", {}) | |
| design_size = (int(declared.get("width", 128)), int(declared.get("height", 32))) | |
| fill_strict = spec.get("fill_check") == "strict" | |
| # Every run: the base config, plus one per harness.json "variant" — | |
| # a config overlay with its own golden dir (e.g. adaptive layout mode | |
| # tested alongside the classic default). | |
| runs = [(None, {}, golden_dir_override or (plugin_dir / 'test' / 'golden'))] | |
| for idx, variant in enumerate(spec.get("variants", [])): | |
| name = variant.get("name") or f"variant{idx}" | |
| vdir = plugin_dir / variant.get("golden_dir", f"test/golden-{name}") | |
| runs.append((name, variant.get("config", {}), vdir)) | |
| all_run_results: List[RenderResult] = [] | |
| for variant_name, overlay, golden_dir in runs: | |
| run_config = {**full_config, **overlay} | |
| results = render_plugin_matrix( | |
| plugin_id=plugin_id, plugin_dir=plugin_dir, config=run_config, | |
| mock_data=effective_mock_data, sizes=effective_sizes, | |
| run_update=effective_run_update, freeze_time=effective_freeze, | |
| ) | |
| if update_golden: | |
| written = write_goldens(results, golden_dir) | |
| logger.info("Wrote %d golden image(s) for %s%s to %s", written, plugin_id, | |
| f" [{variant_name}]" if variant_name else "", golden_dir) | |
| else: | |
| compare_to_goldens(results, golden_dir) | |
| check_scale_up(results, design_size=design_size, strict=fill_strict) | |
| # Tag variant runs so the report and PNG dumps stay distinguishable. | |
| if variant_name: | |
| for r in results: | |
| r.mode = f"{r.mode}@{variant_name}" | |
| if out_dir: | |
| for r in results: | |
| if r.image is None: | |
| continue | |
| dest = out_dir / plugin_id / size_label(r.width, r.height) | |
| dest.mkdir(parents=True, exist_ok=True) | |
| r.image.save(dest / f"{safe_mode_filename(r.mode)}.png", format="PNG") | |
| all_run_results.extend(results) | |
| return all_run_results |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check_plugin.py` around lines 114 - 162, Make unnamed variants unique
in the variant-processing loop around `name = variant.get("name") or "variant"`
by deriving a deterministic identifier from each variant’s position, while
preserving explicit names unchanged. Use that identifier consistently for the
default `golden_dir` and the `@...` mode suffix so multiple unnamed variants
cannot collide, and ensure duplicate or otherwise invalid variant identities
fail loudly rather than overwriting outputs.
Comment on lines
+195
to
+212
| def _parse_render_request(data): | ||
| """Shared /api/render* request prep. Returns (plugin_dir, manifest, config, | ||
| mock_data, skip_update) or raises ValueError with a client message.""" | ||
| plugin_id = data['plugin_id'] | ||
| plugin_dir = find_plugin_dir(plugin_id) | ||
| if not plugin_dir: | ||
| raise LookupError(f'Plugin not found: {plugin_id}') | ||
|
|
||
| manifest_path = plugin_dir / 'manifest.json' | ||
| with open(manifest_path, 'r') as f: | ||
| manifest = json.load(f) | ||
|
|
||
| # Build config: schema defaults + user overrides | ||
| config = {'enabled': True} | ||
| config.update(load_config_defaults(plugin_dir)) | ||
| config.update(data.get('config', {})) | ||
|
|
||
| return plugin_dir, manifest, config, data.get('mock_data', {}), data.get('skip_update', False) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Default previews to skipping update().
Line 212 overrides the render service’s safe default. Since the gallery omits skip_update, one request can perform up to 12 sequential live API updates, causing long requests and rate limiting.
Proposed fix
- return plugin_dir, manifest, config, data.get('mock_data', {}), data.get('skip_update', False)
+ return plugin_dir, manifest, config, data.get('mock_data', {}), data.get('skip_update', True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _parse_render_request(data): | |
| """Shared /api/render* request prep. Returns (plugin_dir, manifest, config, | |
| mock_data, skip_update) or raises ValueError with a client message.""" | |
| plugin_id = data['plugin_id'] | |
| plugin_dir = find_plugin_dir(plugin_id) | |
| if not plugin_dir: | |
| raise LookupError(f'Plugin not found: {plugin_id}') | |
| manifest_path = plugin_dir / 'manifest.json' | |
| with open(manifest_path, 'r') as f: | |
| manifest = json.load(f) | |
| # Build config: schema defaults + user overrides | |
| config = {'enabled': True} | |
| config.update(load_config_defaults(plugin_dir)) | |
| config.update(data.get('config', {})) | |
| return plugin_dir, manifest, config, data.get('mock_data', {}), data.get('skip_update', False) | |
| def _parse_render_request(data): | |
| """Shared /api/render* request prep. Returns (plugin_dir, manifest, config, | |
| mock_data, skip_update) or raises ValueError with a client message.""" | |
| plugin_id = data['plugin_id'] | |
| plugin_dir = find_plugin_dir(plugin_id) | |
| if not plugin_dir: | |
| raise LookupError(f'Plugin not found: {plugin_id}') | |
| manifest_path = plugin_dir / 'manifest.json' | |
| with open(manifest_path, 'r') as f: | |
| manifest = json.load(f) | |
| # Build config: schema defaults + user overrides | |
| config = {'enabled': True} | |
| config.update(load_config_defaults(plugin_dir)) | |
| config.update(data.get('config', {})) | |
| return plugin_dir, manifest, config, data.get('mock_data', {}), data.get('skip_update', True) |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 203-203: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(manifest_path, 'r')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 GitHub Check: CodeQL
[failure] 209-209: Uncontrolled data used in path expression
This path depends on a user-provided value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/dev_server.py` around lines 195 - 212, Update _parse_render_request
so data.get('skip_update', ...) defaults to True, ensuring requests that omit
skip_update—including gallery previews—skip live update calls. Preserve explicit
caller-provided skip_update values.
Comment on lines
+264
to
+276
| from src.plugin_system.testing.sizes import DEFAULT_TEST_SIZES | ||
| sizes = data.get('sizes') or [list(s) for s in DEFAULT_TEST_SIZES] | ||
| if len(sizes) > MAX_MATRIX_SIZES: | ||
| return jsonify({'error': f'at most {MAX_MATRIX_SIZES} sizes per request'}), 400 | ||
| parsed_sizes = [] | ||
| for pair in sizes: | ||
| try: | ||
| plugin_instance.update() | ||
| except Exception as e: | ||
| warnings.append(f"update() raised: {e}") | ||
| w, h = int(pair[0]), int(pair[1]) | ||
| except (TypeError, ValueError, IndexError): | ||
| return jsonify({'error': f'invalid size entry {pair!r} (expected [w, h])'}), 400 | ||
| if not (MIN_WIDTH <= w <= MAX_WIDTH and MIN_HEIGHT <= h <= MAX_HEIGHT): | ||
| return jsonify({'error': f'size {w}x{h} out of bounds'}), 400 | ||
| parsed_sizes.append((w, h)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate each size as an exact two-item array.
A string such as "96x48" is accepted and interpreted as 9x6, while a non-list sizes value can raise an uncaught TypeError. Reject malformed containers with a 400 response.
Proposed fix
- sizes = data.get('sizes') or [list(s) for s in DEFAULT_TEST_SIZES]
+ sizes = data.get('sizes')
+ if sizes is None:
+ sizes = [list(s) for s in DEFAULT_TEST_SIZES]
+ if not isinstance(sizes, list):
+ return jsonify({'error': 'sizes must be an array of [width, height] pairs'}), 400
...
for pair in sizes:
+ if not isinstance(pair, (list, tuple)) or len(pair) != 2:
+ return jsonify({'error': f'invalid size entry {pair!r} (expected [w, h])'}), 400📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from src.plugin_system.testing.sizes import DEFAULT_TEST_SIZES | |
| sizes = data.get('sizes') or [list(s) for s in DEFAULT_TEST_SIZES] | |
| if len(sizes) > MAX_MATRIX_SIZES: | |
| return jsonify({'error': f'at most {MAX_MATRIX_SIZES} sizes per request'}), 400 | |
| parsed_sizes = [] | |
| for pair in sizes: | |
| try: | |
| plugin_instance.update() | |
| except Exception as e: | |
| warnings.append(f"update() raised: {e}") | |
| w, h = int(pair[0]), int(pair[1]) | |
| except (TypeError, ValueError, IndexError): | |
| return jsonify({'error': f'invalid size entry {pair!r} (expected [w, h])'}), 400 | |
| if not (MIN_WIDTH <= w <= MAX_WIDTH and MIN_HEIGHT <= h <= MAX_HEIGHT): | |
| return jsonify({'error': f'size {w}x{h} out of bounds'}), 400 | |
| parsed_sizes.append((w, h)) | |
| from src.plugin_system.testing.sizes import DEFAULT_TEST_SIZES | |
| sizes = data.get('sizes') | |
| if sizes is None: | |
| sizes = [list(s) for s in DEFAULT_TEST_SIZES] | |
| if not isinstance(sizes, list): | |
| return jsonify({'error': 'sizes must be an array of [width, height] pairs'}), 400 | |
| if len(sizes) > MAX_MATRIX_SIZES: | |
| return jsonify({'error': f'at most {MAX_MATRIX_SIZES} sizes per request'}), 400 | |
| parsed_sizes = [] | |
| for pair in sizes: | |
| if not isinstance(pair, (list, tuple)) or len(pair) != 2: | |
| return jsonify({'error': f'invalid size entry {pair!r} (expected [w, h])'}), 400 | |
| try: | |
| w, h = int(pair[0]), int(pair[1]) | |
| except (TypeError, ValueError, IndexError): | |
| return jsonify({'error': f'invalid size entry {pair!r} (expected [w, h])'}), 400 | |
| if not (MIN_WIDTH <= w <= MAX_WIDTH and MIN_HEIGHT <= h <= MAX_HEIGHT): | |
| return jsonify({'error': f'size {w}x{h} out of bounds'}), 400 | |
| parsed_sizes.append((w, h)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/dev_server.py` around lines 264 - 276, Update the size-validation
loop around parsed_sizes so every entry must be a list (or expected array type)
containing exactly two items before indexing it; reject strings, non-list
containers, and incorrect lengths with the existing 400 invalid-size response.
Preserve integer conversion and dimension-bound checks for valid [w, h] entries.
Comment on lines
+568
to
+603
| let failures = 0; | ||
| (data.results || []).forEach(r => { | ||
| const cell = document.createElement('div'); | ||
| cell.style.cssText = 'display:flex;flex-direction:column;gap:4px;'; | ||
| const failed = (r.errors || []).length > 0 || !r.image; | ||
| if (failed) failures++; | ||
|
|
||
| const label = document.createElement('span'); | ||
| label.className = 'text-xs font-mono'; | ||
| label.style.color = failed ? '#f87171' : 'var(--text-secondary)'; | ||
| label.textContent = `${r.width}x${r.height} · ${r.render_time_ms}ms`; | ||
| cell.appendChild(label); | ||
|
|
||
| if (r.image) { | ||
| const img = document.createElement('img'); | ||
| img.src = r.image; | ||
| // Small panels get 2x zoom so they stay legible in the grid | ||
| const zoom = r.height >= 128 ? 1 : 2; | ||
| img.style.cssText = | ||
| `image-rendering: pixelated; width:${r.width * zoom}px; ` + | ||
| `height:${r.height * zoom}px; ` + | ||
| `border:1px solid ${failed ? '#f87171' : 'var(--border-color)'};`; | ||
| cell.appendChild(img); | ||
| } | ||
| if (failed) { | ||
| const err = document.createElement('span'); | ||
| err.className = 'text-xs font-mono'; | ||
| err.style.color = '#f87171'; | ||
| err.textContent = (r.errors || ['render failed']).join('; '); | ||
| cell.appendChild(err); | ||
| } | ||
| grid.appendChild(cell); | ||
| }); | ||
| status.textContent = failures | ||
| ? `${failures} size(s) failed` | ||
| : `${(data.results || []).length} sizes rendered`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Surface render warnings in the gallery.
update() failures are returned through r.warnings, but the gallery ignores them and reports the render as fully successful. Display warnings and include their count in the status so stale or incomplete previews are visible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/templates/dev_preview.html` around lines 568 - 603, Update the
gallery rendering in update() to read r.warnings, display warning messages
alongside each result, and include the warning count in the status text. Keep
failures based on r.errors or missing images, while ensuring results with
warnings are visibly marked as stale or incomplete without being counted as hard
failures.
Comment on lines
+429
to
+452
| def _walk_ladder(self, text: str, ladder: Sequence[FontStep], | ||
| box_w: int, box_h: int, ellipsis: bool) -> FitResult: | ||
| """Shared by fit_text/fit_text_proportional: first ladder entry (in | ||
| the order given) whose rendered text fits, ellipsizing the last one | ||
| tried if none do.""" | ||
| result = None | ||
| for step in ladder: | ||
| font = self.font_manager.get_font(step.family, step.size_px) | ||
| width, height, baseline, y_offset = measure_ink(text, font) | ||
| result = FitResult(font, step.family, step.size_px, text, | ||
| width, height, baseline, y_offset, | ||
| fits=(width <= box_w and height <= box_h), | ||
| line_height=font_line_height(font)) | ||
| if result.fits: | ||
| break | ||
|
|
||
| if result is not None and not result.fits and ellipsis: | ||
| short = self.ellipsize(text, result.font, box_w) | ||
| width, height, baseline, y_offset = measure_ink(short, result.font) | ||
| result = FitResult(result.font, result.family, result.size_px, | ||
| short, width, height, baseline, y_offset, | ||
| fits=(width <= box_w and height <= box_h), | ||
| line_height=result.line_height) | ||
| return result |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_walk_ladder returns None for an empty ladder, violating the FitResult return contract.
When ladder is empty, the loop body never executes, result stays None, and it is cached and returned to callers that declare -> FitResult. Downstream code accessing fit.font or fit.width would get an AttributeError. fit_text_proportional already crashes with a clear ValueError from min() on an empty ladder — fit_text and fit_lines should fail consistently rather than silently returning None.
🛡️ Proposed guard for empty ladders
def _walk_ladder(self, text: str, ladder: Sequence[FontStep],
box_w: int, box_h: int, ellipsis: bool) -> FitResult:
"""Shared by fit_text/fit_text_proportional: first ladder entry (in
the order given) whose rendered text fits, ellipsizing the last one
tried if none do."""
+ if not ladder:
+ raise ValueError("Cannot fit text with an empty font ladder")
result = None
for step in ladder:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _walk_ladder(self, text: str, ladder: Sequence[FontStep], | |
| box_w: int, box_h: int, ellipsis: bool) -> FitResult: | |
| """Shared by fit_text/fit_text_proportional: first ladder entry (in | |
| the order given) whose rendered text fits, ellipsizing the last one | |
| tried if none do.""" | |
| result = None | |
| for step in ladder: | |
| font = self.font_manager.get_font(step.family, step.size_px) | |
| width, height, baseline, y_offset = measure_ink(text, font) | |
| result = FitResult(font, step.family, step.size_px, text, | |
| width, height, baseline, y_offset, | |
| fits=(width <= box_w and height <= box_h), | |
| line_height=font_line_height(font)) | |
| if result.fits: | |
| break | |
| if result is not None and not result.fits and ellipsis: | |
| short = self.ellipsize(text, result.font, box_w) | |
| width, height, baseline, y_offset = measure_ink(short, result.font) | |
| result = FitResult(result.font, result.family, result.size_px, | |
| short, width, height, baseline, y_offset, | |
| fits=(width <= box_w and height <= box_h), | |
| line_height=result.line_height) | |
| return result | |
| def _walk_ladder(self, text: str, ladder: Sequence[FontStep], | |
| box_w: int, box_h: int, ellipsis: bool) -> FitResult: | |
| """Shared by fit_text/fit_text_proportional: first ladder entry (in | |
| the order given) whose rendered text fits, ellipsizing the last one | |
| tried if none do.""" | |
| if not ladder: | |
| raise ValueError("Cannot fit text with an empty font ladder") | |
| result = None | |
| for step in ladder: | |
| font = self.font_manager.get_font(step.family, step.size_px) | |
| width, height, baseline, y_offset = measure_ink(text, font) | |
| result = FitResult(font, step.family, step.size_px, text, | |
| width, height, baseline, y_offset, | |
| fits=(width <= box_w and height <= box_h), | |
| line_height=font_line_height(font)) | |
| if result.fits: | |
| break | |
| if result is not None and not result.fits and ellipsis: | |
| short = self.ellipsize(text, result.font, box_w) | |
| width, height, baseline, y_offset = measure_ink(short, result.font) | |
| result = FitResult(result.font, result.family, result.size_px, | |
| short, width, height, baseline, y_offset, | |
| fits=(width <= box_w and height <= box_h), | |
| line_height=result.line_height) | |
| return result |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/adaptive_layout.py` around lines 429 - 452, Update _walk_ladder to
explicitly reject an empty ladder before iterating, raising a clear ValueError
consistent with the existing fit_text_proportional behavior. Preserve the
current FitResult selection and ellipsis logic for non-empty ladders, and ensure
callers such as fit_text and fit_lines no longer receive None.
Comment on lines
+4285
to
+4295
| def parse_plugin_config_form(form, schema, existing_config): | ||
| """Convert an HTMX config-form submission into a nested plugin config. | ||
|
|
||
| form is the werkzeug form MultiDict; existing_config is the saved | ||
| config to merge updates onto (mutated and returned). Handles dotted | ||
| field names, bracket/indexed array fields (color pickers), schema- | ||
| driven type coercion, and unchecked-checkbox fixup. | ||
|
|
||
| Shared by save_plugin_config and the plugin preview endpoint so the | ||
| two interpretations of the form can never drift apart.""" | ||
| plugin_config = existing_config |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mutate the caller’s saved configuration.
ConfigManager.load_config() returns its live config object, while callers only make shallow copies. Mutating nested values here can therefore apply unsaved preview changes—or failed save attempts—to in-memory configuration.
Proposed fix
def parse_plugin_config_form(form, schema, existing_config):
+ import copy
...
- plugin_config = existing_config
+ plugin_config = copy.deepcopy(existing_config)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def parse_plugin_config_form(form, schema, existing_config): | |
| """Convert an HTMX config-form submission into a nested plugin config. | |
| form is the werkzeug form MultiDict; existing_config is the saved | |
| config to merge updates onto (mutated and returned). Handles dotted | |
| field names, bracket/indexed array fields (color pickers), schema- | |
| driven type coercion, and unchecked-checkbox fixup. | |
| Shared by save_plugin_config and the plugin preview endpoint so the | |
| two interpretations of the form can never drift apart.""" | |
| plugin_config = existing_config | |
| def parse_plugin_config_form(form, schema, existing_config): | |
| """Convert an HTMX config-form submission into a nested plugin config. | |
| form is the werkzeug form MultiDict; existing_config is the saved | |
| config to merge updates onto (mutated and returned). Handles dotted | |
| field names, bracket/indexed array fields (color pickers), schema- | |
| driven type coercion, and unchecked-checkbox fixup. | |
| Shared by save_plugin_config and the plugin preview endpoint so the | |
| two interpretations of the form can never drift apart.""" | |
| import copy | |
| plugin_config = copy.deepcopy(existing_config) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 4285 - 4295, Update
parse_plugin_config_form so it creates an independent deep copy of
existing_config before applying any form updates, rather than assigning the
caller’s object directly to plugin_config. Preserve the existing merge,
coercion, and return behavior while ensuring nested configuration values remain
unchanged when parsing previews or failed saves.
Comment on lines
+4321
to
+4329
| for base_path, values in bracket_array_fields.items(): | ||
| # Get schema property to verify it's an array | ||
| base_prop = _get_schema_property(schema, base_path) | ||
| if base_prop and base_prop.get('type') == 'array': | ||
| # Filter out empty values and sentinel empty strings | ||
| filtered_values = [v for v in values if v and v.strip()] | ||
| # Set directly in plugin_config (values are already strings, no need to parse) | ||
| # Empty array (all unchecked) is represented as [] | ||
| _set_nested_value(plugin_config, base_path, filtered_values) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Coerce bracket-array items using their schema.
Fields such as integer checkbox arrays are stored as strings here. The save path later normalizes them, but preview does not, so preview and saved behavior can differ despite sharing this parser. Apply items.type coercion here or move normalization into a shared helper used by both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 4321 - 4329, Update the
bracket-array handling around _set_nested_value to coerce each filtered value
according to the array schema’s items.type, matching the save-path normalization
behavior. Reuse an existing shared normalization helper if available; otherwise
apply the schema-driven conversion before assigning plugin_config so preview and
saved results remain consistent.
Comment on lines
+4643
to
+4648
| # Fix unchecked boolean checkboxes: HTML checkboxes don't submit values | ||
| # when unchecked, so the existing config value (potentially True) persists. | ||
| # Walk the schema and set any boolean fields missing from form data to False. | ||
| if schema and 'properties' in schema: | ||
| form_keys = set(form.keys()) | ||
| _set_missing_booleans_to_false(plugin_config, schema['properties'], form_keys) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not treat every omitted boolean as unchecked.
A missing field may be absent because the form is partial, conditional, or disabled—not because its checkbox was unchecked. Walking the entire schema here can silently change unrelated saved True values to False. Submit an explicit hidden false sentinel for rendered checkboxes and only update fields present in the request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 4643 - 4648, Remove the
schema-wide _set_missing_booleans_to_false call from the form-processing flow.
Update rendered checkbox inputs to submit an explicit hidden false sentinel when
unchecked, and make the request-handling logic update only boolean fields whose
corresponding form key is present, preserving omitted values for partial,
conditional, or disabled fields.
Comment on lines
+5310
to
+5328
| def _find_plugin_dir_for_preview(plugin_id: str) -> 'Path | None': | ||
| """Locate an installed plugin's directory (store dir, then dev dirs) — | ||
| same search order the schema manager uses.""" | ||
| candidates = [] | ||
| active_pm = getattr(api_v3, 'plugin_manager', None) | ||
| if active_pm and getattr(active_pm, 'plugins_dir', None): | ||
| candidates.append(Path(active_pm.plugins_dir)) | ||
| else: | ||
| _cm = getattr(api_v3, 'config_manager', None) | ||
| _cfg = _cm.load_config() if _cm else {} | ||
| _dir_name = _cfg.get('plugin_system', {}).get('plugins_directory', 'plugin-repos') | ||
| candidates.append(Path(_dir_name) if os.path.isabs(_dir_name) | ||
| else PROJECT_ROOT / _dir_name) | ||
| candidates.append(PROJECT_ROOT / 'plugins') | ||
| candidates.append(PROJECT_ROOT / 'plugin-repos') | ||
| for base in candidates: | ||
| plugin_dir = base / plugin_id | ||
| if (plugin_dir / 'manifest.json').exists(): | ||
| return plugin_dir |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Constrain plugin lookup to direct children of approved roots.
base / plugin_id permits .. and path separators. A crafted identifier can escape the plugin root and cause the loader to import code from another directory containing a manifest. Validate the identifier and verify the resolved directory remains a direct child.
Proposed fix
def _find_plugin_dir_for_preview(plugin_id: str) -> 'Path | None':
+ if not plugin_id or Path(plugin_id).name != plugin_id:
+ return None
...
for base in candidates:
- plugin_dir = base / plugin_id
+ resolved_base = base.resolve()
+ plugin_dir = (resolved_base / plugin_id).resolve()
+ if plugin_dir.parent != resolved_base:
+ continue
if (plugin_dir / 'manifest.json').exists():
return plugin_dirAs per coding guidelines, “Validate inputs and handle errors early (Fail Fast principle).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _find_plugin_dir_for_preview(plugin_id: str) -> 'Path | None': | |
| """Locate an installed plugin's directory (store dir, then dev dirs) — | |
| same search order the schema manager uses.""" | |
| candidates = [] | |
| active_pm = getattr(api_v3, 'plugin_manager', None) | |
| if active_pm and getattr(active_pm, 'plugins_dir', None): | |
| candidates.append(Path(active_pm.plugins_dir)) | |
| else: | |
| _cm = getattr(api_v3, 'config_manager', None) | |
| _cfg = _cm.load_config() if _cm else {} | |
| _dir_name = _cfg.get('plugin_system', {}).get('plugins_directory', 'plugin-repos') | |
| candidates.append(Path(_dir_name) if os.path.isabs(_dir_name) | |
| else PROJECT_ROOT / _dir_name) | |
| candidates.append(PROJECT_ROOT / 'plugins') | |
| candidates.append(PROJECT_ROOT / 'plugin-repos') | |
| for base in candidates: | |
| plugin_dir = base / plugin_id | |
| if (plugin_dir / 'manifest.json').exists(): | |
| return plugin_dir | |
| def _find_plugin_dir_for_preview(plugin_id: str) -> 'Path | None': | |
| if not plugin_id or Path(plugin_id).name != plugin_id: | |
| return None | |
| """Locate an installed plugin's directory (store dir, then dev dirs) — | |
| same search order the schema manager uses.""" | |
| candidates = [] | |
| active_pm = getattr(api_v3, 'plugin_manager', None) | |
| if active_pm and getattr(active_pm, 'plugins_dir', None): | |
| candidates.append(Path(active_pm.plugins_dir)) | |
| else: | |
| _cm = getattr(api_v3, 'config_manager', None) | |
| _cfg = _cm.load_config() if _cm else {} | |
| _dir_name = _cfg.get('plugin_system', {}).get('plugins_directory', 'plugin-repos') | |
| candidates.append(Path(_dir_name) if os.path.isabs(_dir_name) | |
| else PROJECT_ROOT / _dir_name) | |
| candidates.append(PROJECT_ROOT / 'plugins') | |
| candidates.append(PROJECT_ROOT / 'plugin-repos') | |
| for base in candidates: | |
| resolved_base = base.resolve() | |
| plugin_dir = (resolved_base / plugin_id).resolve() | |
| if plugin_dir.parent != resolved_base: | |
| continue | |
| if (plugin_dir / 'manifest.json').exists(): | |
| return plugin_dir |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/blueprints/api_v3.py` around lines 5310 - 5328, Update
_find_plugin_dir_for_preview to reject plugin_id values containing path
separators or traversal components before constructing candidate paths. For each
approved root, resolve the candidate and verify it is exactly a direct child of
the resolved base directory before accepting an existing manifest.json,
returning None for invalid or escaping identifiers.
Source: Coding guidelines
Comment on lines
+1013
to
+1019
| <option value="">My panel size</option> | ||
| <option value="64x32">64 × 32</option> | ||
| <option value="128x32">128 × 32</option> | ||
| <option value="128x64">128 × 64</option> | ||
| <option value="192x48">192 × 48</option> | ||
| <option value="256x128">256 × 128</option> | ||
| </select> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the supported 96×48 preview size.
The PR explicitly adds 96×48 validation support, but users cannot select it in live preview. Add the preset or derive this list from the shared size definitions to prevent future drift.
<option value="64x32">64 × 32</option>
+ <option value="96x48">96 × 48</option>
<option value="128x32">128 × 32</option>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <option value="">My panel size</option> | |
| <option value="64x32">64 × 32</option> | |
| <option value="128x32">128 × 32</option> | |
| <option value="128x64">128 × 64</option> | |
| <option value="192x48">192 × 48</option> | |
| <option value="256x128">256 × 128</option> | |
| </select> | |
| <option value="">My panel size</option> | |
| <option value="64x32">64 × 32</option> | |
| <option value="96x48">96 × 48</option> | |
| <option value="128x32">128 × 32</option> | |
| <option value="128x64">128 × 64</option> | |
| <option value="192x48">192 × 48</option> | |
| <option value="256x128">256 × 128</option> | |
| </select> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web_interface/templates/v3/partials/plugin_config.html` around lines 1013 -
1019, Add the supported 96×48 preset to the “My panel size” select options in
the plugin configuration template, keeping the existing size choices unchanged;
if a shared size-definition source is already available, reuse it to keep
validation and preview options synchronized.
ChuckBuilds
added a commit
that referenced
this pull request
Jul 12, 2026
…ack traces from render responses CodeQL doesn't model Path.iterdir() as a taint-clearing enumeration the way it does os.scandir() -- _trusted_plugin_dir's iterdir-based rebuild still traced plugin_id through to the manifest.json open(). Switched to scandir, matching the pattern already verified clean on PR #396. Also stops surfacing raw exception text (update()/display() failures) in the JSON render response -- logs full detail server-side via exc_info instead, returning only the exception class name to the client. And drops path values from three plugin_loader debug/error logs that CodeQL flags as clear-text-logging of externally-influenced data, keeping plugin_id (not flagged) for context.
ChuckBuilds
added a commit
that referenced
this pull request
Jul 12, 2026
Union in adaptive_images.py and field in adaptive_layout.py are both imported but never used -- the last two Codacy findings on this PR, matching the same fix already applied on PR #396.
Owner
Author
|
Closing as a duplicate — this PR's full scope (adaptive layout + element style + dev preview + testing harness updates) was independently opened by @ChuckBuilds as two better-organized PRs: #393 (feat/adaptive-layout) and #394 (feat/element-style, stacked on #393). #393 is now green (CodeQL/Codacy fixes from this PR were ported over directly). Continuing there instead of maintaining a parallel duplicate. |
ChuckBuilds
added a commit
that referenced
this pull request
Jul 12, 2026
…, image fitting (#393) * feat(layout): adaptive layout & font scaling system for plugins Add src/adaptive_layout.py — opt-in core helpers so plugins render legibly on any panel size without hand-tuned per-display layouts: - Region: integer rect algebra (bands/columns/weighted splits/centering) that partitions space so text bands can't overlap by construction - Font ladders: ordered (family, size) steps known to render crisply (LADDER_GRID: X11 BDFs at native sizes; LADDER_ARCADE: PressStart2P at 8px multiples) — fitting walks the ladder instead of scaling pixel fonts fractionally - LayoutContext: breakpoint tiers, geometry scale vs. a declared design size, and cached fit_text/fit_lines/font_for_rows queries Generalizes the three patterns proven in the field: f1-scoreboard's scale factor, masters-tournament's tiers, baseball-scoreboard's font fallback ladder. Wiring: BasePlugin gains a lazy .layout property and draw_fit(); FontManager gains get_native_bdf_size() and a cache_generation counter; manifest schema gains display.design_size and requires.display_size max_width/max_height; 96x48 joins DEFAULT_TEST_SIZES; the bounds-check harness records negative-coordinate draws; TextHelper's broken measurement helpers are fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): adaptive image fitting + composite region helpers Add src/adaptive_images.py — the image counterpart to fit_text: - fit_image(img, box, mode=contain|cover|fill_height|stretch, crop_to_ink, anchor, resample, upscale) promoting the proven plugin patterns (football's crop-to-ink fill-height logos, masters' cover crop + NEAREST flags, static-image's letterbox). Upscales by default — thumbnail()'s downscale-only behavior is why imagery stays tiny on big panels. - draw_fitted_image() pastes aligned within a Region with alpha mask. - One central Pillow>=9.1 RESAMPLE shim replacing ~15 plugin copies. LayoutContext.fit_image() caches results per (identity, box size, options) with a 64-entry LRU; id()-keyed entries pin the source image. BasePlugin.draw_image() is the one-liner adoption path beside draw_fit. Composites in adaptive_layout.py: Region.offset() (user x/y-offset passthrough), scoreboard_regions() (the two-logos-plus-score card math duplicated across six sports plugins, logo_slot = min(H, W//2)), and media_row() (art-left/text-right). Fix LogoHelper's size-blind cache key (stale sizes on panel change); deprecation note on dead image_utils.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(harness): scale-up fill check, config variants, multi-size dev gallery Quality gates for adaptive layout: - fill_metrics()/check_scale_up() in the safety harness: overflow catches content too big for a panel, but nothing caught content that stays tiny on panels >= 2x the plugin's declared design size. The check measures lit-content extents and warns (or fails, when a plugin opts into "fill_check": "strict" in test/harness.json) below 50% coverage on the doubled axis. Warn-only by default so no existing plugin breaks. - harness.json "variants": extra runs with config overlays and their own golden dirs, so an opt-in mode (e.g. layout_mode: adaptive) is golden- tested beside the classic default. check_plugin.py loops base + variants and labels variant results mode@name. - Dev preview server: GET /api/sizes (harness size sample), POST /api/render-matrix (render at up to 12 sizes in one call), size-preset dropdown, and an "All Sizes" side-by-side gallery in the preview UI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(plugins): adaptive-lib discoverability + advisory version compat warning Discoverability: re-export the adaptive layout/image API from src.common (the blessed-helpers package plugin authors already know) — canonical paths stay src.adaptive_layout / src.adaptive_images so nothing breaks. Document it in src/common/README.md and cross-link ADAPTIVE_LAYOUT.md from the developer docs authors actually read (quick reference, API reference, advanced dev, font manager, dev preview, plugin dev guide); ADAPTIVE_LAYOUT.md gains adaptive-images, composite-layouts and preserving-user-customization sections. Compat: PluginLoader now logs one advisory warning (never raises) when a plugin's manifest declares a min LEDMatrix version newer than the running core, checking the min_ledmatrix_version / requires.* / versions[] spellings found in the wild. Guarded against stale core version numbers. src/__init__.py __version__ bumped 1.0.0 -> 3.1.0 to match the latest release tag (v3.1.0) — it had never been updated and the compat check needs a truthful number. NOTE: verify this matches the intended release numbering before the next tag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): add measure_font_crispness — verify a ladder rung isn't blurry PIL antialiases TTF outlines by default; a 'pixel-style' font only rasterizes without antialiasing at specific sizes (for PressStart2P: exact multiples of its 8px design grid). A ladder rung at an unverified size silently renders blurry on an LED panel — this exact bug shipped in both text-display's and football-scoreboard's custom TTF ladders (non-8-multiple PressStart2P sizes, and '5by7.regular'/'4x6-font' at sizes that were never actually crisp). measure_font_crispness(font, sample_text) renders the sample and reports the fraction of ink-bbox pixels that are neither pure black nor pure white. BDF fonts (real bitmaps) always score 0.0; TTF ladders should be verified against this before shipping — see the new TestFontFitting::test_ladder_arcade_is_crisp pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): add fit_text_proportional — proportional sizing vs. always-maximize fit_text always picks the largest ladder rung that fits its box. That's right when an element owns dedicated space, but wrong when several independently-fitted elements need to stay visually harmonious as the panel grows: a score's box might have generous room while a neighboring logo scales by a fixed geometry factor via px() — fit_text lets the score balloon out of proportion (even overlapping the logo) even though its individual pick is technically correct. fit_text_proportional(text, box, base_size_px, ladder) instead targets base_size_px * self.scale (the same scale factor px() already uses), picking the nearest ladder rung at or below that target, still capped to what fits the box, floored at the smallest rung when the target is below every rung. Refactored the shared largest-that-fits/ellipsize walk into _walk_ladder() so fit_text and fit_text_proportional don't duplicate it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(layout): fit_text_proportional gains an axis-specific scale override self.scale (min(width_ratio, height_ratio)) is the right conservative default for anything whose aspect ratio matters, but a caller whose surrounding composition already scales along a single axis — e.g. football-scoreboard's logo_slot = min(height, width // 2), which tracks height alone — needs text sized the same way, or it reads as under-scaled next to logos that grew on a panel that only got taller (128x32 -> 128x64: self.scale stays 1.0 since width didn't grow, but logos still double). fit_text_proportional(..., scale=None) now accepts an explicit override; None keeps the existing self.scale default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(layout): scoreboard_regions reserves real center space at 2:1 aspect ratios logo_slot = min(height, width // 2) has a blind spot: at exactly 2:1 aspect ratio (width == 2 * height -- a very common shape: two, four, or more square modules stacked into a taller panel) width // 2 and height are equal, so the two logo slots claim the ENTIRE width and leave zero pixels for a center column, no matter how large the panel gets. Not a 'small panel' problem -- 96x48, 128x64, and 256x128 (all exactly 2:1) hit it identically, while the 128x32 design baseline and panels like 192x48 or 256x32 never do, because height is already the tighter constraint there. Two new parameters fix it in the one shared helper every scoreboard-style plugin composes through: - min_center_fraction / min_center_design_px reserve at least max(width * fraction, design_px * ctx.scale) for the center column, capping logo_slot further when needed. The scaled design-px term matters on small panels where a flat fraction alone reserves too little absolute space. - score_bleed_fraction extends the score's own fit box (not the logo slots themselves) a controlled amount into each side -- the same way real broadcast scoreboards let a big score number's edges cross into the team marks flanking it. Without this the reserve alone can still be too narrow for a short score to render without truncating. score_area is now genuinely narrower than the full card width (previously identical to status_band/detail_band, which still span the full width and overlay the logos -- short text there was never the problem). Verified against the full harness size spread: a real game score like '17-21' never needs ellipsis at any tested 2:1-or-tighter aspect ratio (test_score_never_needs_ellipsis_for_a_short_score), and wide panels (128x32/192x48/256x32-style) are provably unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: document scoreboard_regions' center-reserve and score-bleed params Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address CodeRabbit review on PR #393 - docs: scope the self.layout note to BasePlugin subclasses (others build a LayoutContext directly) and make explicit that adaptive layout is opt-in — classic rendering stays unless a plugin adopts the APIs. - dev_server: broaden the render-request catch (a bad manifest.json now returns a clean 400 instead of an unhandled 500) and stop echoing raw exception text in the loader-failure responses — full tracebacks go to the dev server's console log instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): allowlist plugin_id before any path lookup CodeQL (py/path-injection): plugin_id arrives in request input and flows into filesystem paths via find_plugin_dir. Gate it with the same ^[a-zA-Z0-9_-]{1,64}$ allowlist the web UI's pages_v3 uses, at the single choke point every route resolves through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): lexical containment check on resolved plugin dirs CodeQL doesn't recognize the interprocedural allowlist as a path-injection barrier; add the canonical one — normalize (without following symlinks, since dev plugins are commonly symlinked into plugins/) and require the result to stay inside the search dir. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): inline normpath containment barrier before render CodeQL doesn't credit the sanitization inside find_plugin_dir along this flow; apply its documented barrier (normpath + startswith against the allowed roots) inline in _parse_render_request, on the exact path that reaches the render/load sinks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): derive plugin dir from trusted directory listings CodeQL's barrier-guard recognition doesn't see a startswith check inside an any() comprehension, so the normalize-and-prefix approach still flagged. Break the taint outright instead: after lookup, re-derive the directory by enumerating the search dirs (iterdir) and matching by path equality — the Path used for all downstream file access is built solely from trusted listings, never from request input. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(dev-server): use os.scandir for path-injection barrier, redact stack traces from render responses CodeQL doesn't model Path.iterdir() as a taint-clearing enumeration the way it does os.scandir() -- _trusted_plugin_dir's iterdir-based rebuild still traced plugin_id through to the manifest.json open(). Switched to scandir, matching the pattern already verified clean on PR #396. Also stops surfacing raw exception text (update()/display() failures) in the JSON render response -- logs full detail server-side via exc_info instead, returning only the exception class name to the client. And drops path values from three plugin_loader debug/error logs that CodeQL flags as clear-text-logging of externally-influenced data, keeping plugin_id (not flagged) for context. * fix(dev-server): remove conditional-reassignment ambiguity in plugin_dir resolution CodeQL's path-injection flow still traced through _parse_render_request after the scandir fix -- the tainted find_plugin_dir() result and the scandir-derived _trusted_plugin_dir() result shared the same variable name (plugin_dir), reassigned only on the truthy branch. That merge point apparently isn't treated as a barrier by the flow analysis, so it kept tracing the pre-reassignment value through to the manifest open(). Split into two distinct names -- candidate_dir (tainted, used only to call _trusted_plugin_dir) and trusted_dir (the only name used for any downstream file access) -- so there's no reassigned variable for the flow to walk through. * fix: remove unused imports flagged by Codacy Union in adaptive_images.py and field in adaptive_layout.py are both imported but never used -- the last two Codacy findings on this PR, matching the same fix already applied on PR #396. * fix(layout): bound the fit cache; never alias the source image in fits Two latent issues found in a self-review pass: - LayoutContext._fit_cache was an unbounded dict (the image cache got an LRU cap, the text-fit cache didn't). Cache keys embed the fitted TEXT, so a plugin fitting changing strings — a live game clock, a ticker — on a 24/7 service grows it forever. Now LRU-bounded at 512 entries via the same pattern as the image cache. - fit_image returned the caller's ORIGINAL image object when the source was already RGBA at target size (contain/fill_height, no ink crop). ImageFitResult is documented as an independent copy, and LayoutContext caches results — an aliased image lets later mutations of the source corrupt cached fits (or vice versa). Copy in that branch. Both covered by new regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Upstreams a substantial body of work found sitting uncommitted on a local
dev/test device — never pushed or opened as a PR. I verified it's
functionally sound before packaging (see test plan below); I did not author
this code this session, and did not do a from-scratch line-by-line design
review of all ~5000 lines — I relied on the test evidence plus a structural
read of each file's stated purpose.
What's in it
Adaptive layout (
src/adaptive_layout.py,src/adaptive_images.py) — aRegion/LayoutContext geometry system so plugins can carve their panel into
regions and fit text/images to them instead of hardcoding coordinates per
panel size. Wired into
BasePluginasself.layout(lazily built, cached,invalidated on display-size or font-cache-generation change) and
draw_fit(text, box, ladder=...).src/image_utils.py's old fit-imagehelper is marked deprecated in favor of
adaptive_images.fit_image.Element style (
src/element_style.py) — universal per-element styleresolution for plugin customization (
config['customization']), replacingfour divergent hand-rolled implementations across the sports/music plugins.
Centralizes font loading, x/y-offset reading, and the "did the user
actually override this?" check — subtle enough it shipped broken twice
before, since a key being present in config never means the user set it
(both
schema_manager.merge_with_defaultsand the plugin manager writefull schema defaults into config before a plugin sees it).
BasePlugin.element_style()wires this to a plugin's ownconfig_schema.jsondefaults.schema/manifest_schema.jsonandschema_manager.pygained the declarative "generated element" expansionbacking this.
Testing harness —
bounds_display_manager.pynow recordsnegative-coordinate (left/top overflow) draw calls, previously undetectable
since PIL clips them silently.
harness.py/sizes.pygained adesign-size-driven "fill" check (the scale-up counterpart to the existing
overflow check), a new 96×48 non-64×32-grid test size, and
check_plugin.py"variant" support (e.g. testing a plugin withadaptive-layout mode alongside its classic default, each against its own
golden dir).
render_service.pyfactors out shared single-plugin-renderlogic for reuse by the new web preview endpoint.
Web UI live preview — new
POST /api/v3/plugins/<id>/previewendpoint(plus
dev_server.py's standalone equivalent) renders a plugin'sin-progress config at a given panel size without saving, so the Plugin
Manager's config form can show a live preview.
dev_preview.htmlgained asize-preset dropdown and an "All Sizes" gallery button.
Test plan
RGBMatrixEmulator/plugin-repos environment): 187 passed across
test_adaptive_layout.py,test_adaptive_images.py,test_element_style.py,test_schema_style_expansion.py,test_harness_fill.py, andtest/web_interface/test_plugin_preview.py.test/suite (1024+ tests): no new failures. The5 failures present (
test_circuit_breaker+ 4 others intest_web_api.py/
test_state_reconciliation.py) were confirmed to reproduce identicallyagainst a clean
origin/maincheckout in an isolated git worktree, sothey predate and are unrelated to this change — worth a separate look,
but not introduced here.
isolating just this PR's file set into a clean branch off
origin/main(rather than the messy local working tree) to confirm nothing relied on
other uncommitted local state: still 187 passed.
🤖 Generated with Claude Code
Summary by CodeRabbit