Test suite overhaul + fixes for the three bugs it uncovered - #441
Merged
Conversation
…ething real The unit-tests CI job ran an explicit 24-file allowlist that had rotted: 63 of 90 test files (display, vegas, store manager, web API, web_interface) never ran on a PR. The job now runs all of test/ (minus test/plugins, which the plugin-safety job owns) so new test files are enrolled by default and any exclusion needs a visible, commented --ignore. The plugin-safety job was a green no-op: plugins/ is empty in CI, so every test skipped with 'Manifest not found'. It now renders a bundled deterministic fixture plugin (test/fixtures/plugins/ci-fixture-plugin, golden images included for all 8 default sizes) via LEDMATRIX_PLUGINS_DIR, and sets LEDMATRIX_REQUIRE_PLUGINS=1 so discovering zero plugins fails loudly instead of skipping green. The per-plugin suites document that they target dev machines with real plugins installed. Coverage is now measured and enforced in exactly one place — the CI unit-tests step (--cov=src --cov=web_interface --cov-fail-under=45, from a measured 47% baseline). pytest.ini previously declared --cov-fail-under=30 but CI always passed --no-cov, so the gate had never run anywhere; local pytest is now coverage-free and fast. Enabling the 63 unenrolled files surfaced three cases of test rot, fixed here: test_display_controller_vegas_tick.py could not collect without the hardware rgbmatrix module (now uses the emulator convention), the state-reconciliation unrecoverable-cache tests broke when production added the is_plugin_uninstalled tombstone check (bare Mock returned truthy), and test_get_system_status assumed the optional psutil dependency (now installed via requirements-test.txt and guarded by importorskip). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
test_font_manager.py was 5 of 6 tests shaped as 'try: call(); assert True / except: assert True' — running in CI while unable to fail on any regression. Rewritten against the real FontManager API and the bundled assets/fonts: returned font types, cache-hit identity, distinct entries per size, default-font fallback for unknown families and corrupt files (recorded in failed_loads), BDF native-size reading, text measurement, and cache lifecycle. test_display_manager.py's test_draw_text ended in 'assert True'; it now renders onto a known-black canvas and asserts pixels were actually lit — which required un-breaking the fixture's freetype MagicMock so draw_text's isinstance check doesn't silently swallow the draw. test_display_controller.py carried a permanently-skipped test whose skip reason already declared it redundant; deleted. Both display test files now set EMULATOR=true before importing display_manager (the same convention as test_display_dirty_tracking.py) so they collect standalone instead of depending on which test module imports display_manager first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
…config merges, durations, skin cards) New unit tests for pure or filesystem-only logic that previously had zero direct coverage: - test_compatibility.py: the semver install gate (parse_semver suffix handling, every range operator, TRUSTWORTHY_FLOOR behavior for cores reporting untrustworthy versions, 'more restrictive wins', and the malformed-manifest shapes that used to raise). - test/web_interface/test_secret_helpers.py: the canonical x-secret helpers — find/separate/mask/remove, array-item secrets, no input mutation, and a separate->recombine round-trip. - test/web_interface/test_api_v3_helpers.py: the module-level helpers behind the plugin config save endpoint (_is_plugin_update_available, _coerce_to_bool including the int==1 quirk, deep_merge including its shared-subtree shallowness, _parse_form_value, dotted-key-aware _get_schema_property/_set_nested_value). - test_base_plugin_duration.py: get_display_duration's full coercion ladder (instance attr -> config -> 15.0), including the bool-is-int quirk where display_duration=True means one second. - test_config_manager_secrets.py: the secrets round-trip — deep-merge on load, strip on save, group pruning, the load fast path — and two characterized sharp edges marked SUSPECTED BUG: an unreadable secrets file at save time writes secrets into config.json in plaintext, and a same-mtime-same-size content swap is served stale. - test_schema_manager_merge.py: merge_with_defaults branch behavior (None replacement vs falsey preservation, dict-vs-scalar mismatches, arrays replaced wholesale, defaults never mutated). - test_skin_system.py (extended): render_skin_card shares _render_game's 3-strike counter but never resets it on success — the asymmetry is pinned in both directions, along with card fallthrough and the disable interaction between the two paths. Suspected bugs are characterized, not fixed — each carries a comment so a future behavior change is deliberate rather than accidental. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
Three guard suites that pin contracts spanning multiple files, where one side changing unilaterally breaks the other silently: - test_version_comparison_consistency.py: the repo's four version comparators (compatibility.parse_semver, api_v3's packaging-based _is_plugin_update_available, store_manager update_plugin's raw string equality, skin_runtime._major) answer differently on the same inputs. A table pins each one's verdict; update_plugin is driven through its real code path to show the SUSPECTED BUGs: 'v1.2.0' vs '1.2.0' triggers a full reinstall the UI calls unnecessary, and a locally-ahead plugin gets downgraded. A pairwise-ordering check keeps parse_semver agreeing with packaging on plain X.Y.Z. - test/web_interface/test_secret_separation_parity.py: api_v3.py carries three inline copies of find_secret_fields/separate_secrets that lack the canonical module's array-item support. The copy count is asserted exact (it may only go down; new copies must import src/web_interface/secret_helpers), the missing-array-support gap is asserted so it can't grow silently, and the canonical behavior that migration will adopt is documented executably. - test_discovery_path_contract.py: the three 'where is plugin X' resolvers (PluginManager discovery, StoreManager._find_plugin_path, SchemaManager.get_schema_path) agree on the configured directory, and their divergent fallback chains are characterized. Also pins the .standalone-backup- naming contract shared by store rollback and discovery, and _resolve_skin_target's path-traversal rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request expands CI test execution, adds a deterministic fixture plugin, centralizes plugin version comparison, hardens configuration secret saves, resets skin-render failure counts, and adds broad core and web-interface test coverage. ChangesTest validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 3 |
| Duplication | -2 |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
test/test_discovery_path_contract.py (1)
36-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type hints to the affected test helpers, fixtures, and test methods.
- Type annotate helper/fixture functions and test function parameters/returns in
test_discovery_path_contract.py,test_font_manager.py,test_schema_manager_merge.py,test_skin_system.py,test_version_comparison_consistency.py,test_secret_helpers.py, andtest_secret_separation_parity.py.- Use
str | Nonefor_write_plugin(..., dir_name=None)on this Python target.🤖 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_discovery_path_contract.py` around lines 36 - 167, Apply type hints to all affected helpers, fixtures, and test methods in test/test_discovery_path_contract.py:36-167, test/test_font_manager.py:18-126, test/test_schema_manager_merge.py:14-104, test/test_skin_system.py:464-561, test/test_version_comparison_consistency.py:49-148, test/web_interface/test_secret_helpers.py:51-241, and test/web_interface/test_secret_separation_parity.py:30-119. Annotate parameters and return types using the concrete types implied by each helper, fixture, and test; specifically update _write_plugin’s dir_name parameter to str | None and add appropriate return annotations throughout.Sources: Coding guidelines, Linters/SAST tools
test/web_interface/test_api_v3_helpers.py (1)
156-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark
SCHEMAas a class variable.Ruff reports RUF012 because
SCHEMAis mutable at class scope. Add aClassVarannotation or move the schema into a fixture. This prevents accidental shared-state mutation between tests.🤖 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/web_interface/test_api_v3_helpers.py` around lines 156 - 171, Annotate the class-level mutable SCHEMA definition with typing.ClassVar in the test class, preserving its existing schema contents and behavior while resolving Ruff RUF012.Sources: Coding guidelines, 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 `@test/fixtures/plugins/ci-fixture-plugin/manager.py`:
- Line 18: Add a class docstring to CIFixturePlugin stating that it is a
deterministic, CI-only fixture plugin, while leaving the class behavior
unchanged.
- Around line 22-38: Update display() to call self.display_manager.clear()
before accessing self.display_manager.image and performing any drawing. Preserve
the existing rendering operations and ensure
self.display_manager.update_display() remains after rendering.
In `@test/fixtures/plugins/ci-fixture-plugin/requirements.txt`:
- Line 1: Add the existing LEDMatrix/Pillow-compatible pinned Pillow dependency
to the fixture’s requirements.txt, preserving the explanatory comment and
ensuring the plugin loader detects and installs it for manager.py’s
PIL.ImageDraw import.
In `@test/test_base_plugin_duration.py`:
- Around line 16-110: Add repository-required type hints to all affected test
callables: in test/test_base_plugin_duration.py lines 16-110, annotate
_MinimalPlugin methods, make_plugin(), and every test method; in
test/test_compatibility.py lines 25-271, annotate parameterized test parameters
and test method returns; in test/test_config_manager_secrets.py lines 21-161,
annotate make_manager(), tmp_path, and test method returns; and in
test/web_interface/test_api_v3_helpers.py lines 30-231, annotate test methods
and helper inputs with concrete types, using -> None for tests and preserving
existing behavior.
- Around line 81-84: Update BasePlugin.get_display_duration to exclude bool
values from numeric duration handling, including both True and False, so they
use the configured duration or 15.0 default. Replace
test_bool_true_is_one_second with an assertion covering the expected fallback
behavior for instance_duration=True.
In `@test/test_compatibility.py`:
- Around line 219-227: Rename test_unparseable_core_version_is_compatible in
test/test_compatibility.py:219-227 to describe that an untrustworthy core with a
high declared floor is blocked; rename the corresponding secrets test in
test/test_config_manager_secrets.py:102-120 to state that the secrets file is
corrupt rather than unreadable. No assertion or behavior changes are needed.
In `@test/test_config_manager_secrets.py`:
- Around line 102-120: Update save_config() to detect parsing failures in
config_secrets.json and raise a clear ConfigError before writing config.json.
Ensure the failure path performs no plaintext secret write, and preserve the
regression test’s expectation that the secret is not saved in the main
configuration.
In `@test/test_skin_system.py`:
- Around line 521-550: Update SportsCore.render_skin_card and its successful
card-render path so _skin_failures resets to zero when render_vegas_card returns
an image and when the mode renderer returns True. Revise
test_card_success_does_not_reset_strikes to assert successes clear the counter
and that a later failure starts from the reset state rather than accumulated
strikes.
In `@test/test_version_comparison_consistency.py`:
- Around line 101-115: Update PluginStoreManager.update_plugin() to use one
shared semantic version comparator that normalizes equivalent forms such as a
leading “v”, and reinstall only when the registry version is newer than the
installed version. Change
test_v_prefix_triggers_reinstall_despite_semantic_equality and
test_locally_ahead_version_triggers_downgrade_reinstall to assert no reinstall
and preserve the successful update result.
---
Nitpick comments:
In `@test/test_discovery_path_contract.py`:
- Around line 36-167: Apply type hints to all affected helpers, fixtures, and
test methods in test/test_discovery_path_contract.py:36-167,
test/test_font_manager.py:18-126, test/test_schema_manager_merge.py:14-104,
test/test_skin_system.py:464-561,
test/test_version_comparison_consistency.py:49-148,
test/web_interface/test_secret_helpers.py:51-241, and
test/web_interface/test_secret_separation_parity.py:30-119. Annotate parameters
and return types using the concrete types implied by each helper, fixture, and
test; specifically update _write_plugin’s dir_name parameter to str | None and
add appropriate return annotations throughout.
In `@test/web_interface/test_api_v3_helpers.py`:
- Around line 156-171: Annotate the class-level mutable SCHEMA definition with
typing.ClassVar in the test class, preserving its existing schema contents and
behavior while resolving Ruff RUF012.
🪄 Autofix
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 Plus
Run ID: 7d9d9b8d-e215-4d9c-a0c3-5fb01494f887
⛔ Files ignored due to path filters (8)
test/fixtures/plugins/ci-fixture-plugin/test/golden/128x32/ci-fixture.pngis excluded by!**/*.pngtest/fixtures/plugins/ci-fixture-plugin/test/golden/128x64/ci-fixture.pngis excluded by!**/*.pngtest/fixtures/plugins/ci-fixture-plugin/test/golden/128x96/ci-fixture.pngis excluded by!**/*.pngtest/fixtures/plugins/ci-fixture-plugin/test/golden/256x128/ci-fixture.pngis excluded by!**/*.pngtest/fixtures/plugins/ci-fixture-plugin/test/golden/256x32/ci-fixture.pngis excluded by!**/*.pngtest/fixtures/plugins/ci-fixture-plugin/test/golden/64x32/ci-fixture.pngis excluded by!**/*.pngtest/fixtures/plugins/ci-fixture-plugin/test/golden/64x64/ci-fixture.pngis excluded by!**/*.pngtest/fixtures/plugins/ci-fixture-plugin/test/golden/96x48/ci-fixture.pngis excluded by!**/*.png
📒 Files selected for processing (30)
.github/workflows/test.ymlpytest.inirequirements-test.txttest/fixtures/plugins/ci-fixture-plugin/config_schema.jsontest/fixtures/plugins/ci-fixture-plugin/manager.pytest/fixtures/plugins/ci-fixture-plugin/manifest.jsontest/fixtures/plugins/ci-fixture-plugin/requirements.txttest/plugins/conftest.pytest/plugins/test_basketball_scoreboard.pytest/plugins/test_calendar.pytest/plugins/test_clock_simple.pytest/plugins/test_odds_ticker.pytest/plugins/test_soccer_scoreboard.pytest/plugins/test_text_display.pytest/test_base_plugin_duration.pytest/test_compatibility.pytest/test_config_manager_secrets.pytest/test_discovery_path_contract.pytest/test_display_controller.pytest/test_display_controller_vegas_tick.pytest/test_display_manager.pytest/test_font_manager.pytest/test_schema_manager_merge.pytest/test_skin_system.pytest/test_version_comparison_consistency.pytest/test_web_api.pytest/web_interface/test_api_v3_helpers.pytest/web_interface/test_secret_helpers.pytest/web_interface/test_secret_separation_parity.pytest/web_interface/test_state_reconciliation.py
💤 Files with no reviewable changes (1)
- test/test_display_controller.py
- ci-fixture-plugin: call display_manager.clear() before rendering (per plugin guidelines — the fixture should model a well-behaved plugin), add a class docstring, and document why Pillow is deliberately not pinned in its requirements.txt (core dependency; harness installs nothing). - Rename two tests whose names contradicted their assertions: test_unparseable_core_version_is_compatible -> test_unparseable_core_with_high_floor_is_blocked, and test_unreadable_secrets_file... -> test_corrupt_secrets_file... - Annotate TestGetSchemaProperty.SCHEMA as ClassVar (RUF012). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
Owner
Author
|
Addressed the CodeRabbit review in 71b665e (or latest head): Applied
Declined, with reasons
Generated by Claude Code |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
…n strikes on card success Fixes the three suspected bugs this PR's characterization tests pinned, flipping those tests to assert the corrected behavior: - plugins/store: ONE shared update comparator. New compatibility.is_update_available() (PEP 440 via packaging) is now used by both the web UI's update badge (api_v3._is_plugin_update_available is a thin alias) and store_manager.update_plugin's reinstall decision. Previously update_plugin used raw string equality: 'v1.2.0' vs '1.2.0' triggered a full reinstall the UI called unnecessary, and a locally- ahead plugin (2.0.0 installed, registry 1.9.0) was silently DOWNGRADED. Now equivalent spellings skip the reinstall and locally-ahead versions are never downgraded; unparseable versions still reconcile by reinstalling from the registry. - config: save_config and save_config_atomic now refuse (ConfigError) when config_secrets.json exists but cannot be loaded. Both previously proceeded without stripping, writing the merged secrets into config.json in plaintext. The shared _load_secrets_for_save() helper raises with an actionable message instead; a missing secrets file is still fine (nothing to strip), and _migrate_config's catch-all keeps boot resilient. - skins: render_skin_card resets _skin_failures on both success paths (vegas card returned, or mode renderer handled), mirroring _render_game. Transient card failures no longer accumulate across a session until they permanently disable a working skin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
ChuckBuilds
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/config_manager.py (1)
288-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatch expected load failures and log the traceback.
Line 304 catches all
Exceptionvalues. This can hide an implementation defect as a secrets-file error. Catch the expected file and JSON parsing exceptions instead.Line 311 logs an error without
exc_info=True. Add the traceback and the[Config]context prefix for remote diagnosis.Proposed fix
- except Exception as e: + except (OSError, UnicodeDecodeError, json.JSONDecodeError, RecursionError) as e: error_msg = ( f"Refusing to save config: secrets file {self.secrets_path} exists " f"but could not be loaded ({e}). Saving without it would write " f"merged secret values into config.json in plaintext. Fix or " f"remove the secrets file, then retry." ) - self.logger.error(error_msg) + self.logger.error("[Config] %s", error_msg, exc_info=True) raise ConfigError(error_msg, config_path=self.secrets_path) from e🤖 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/config_manager.py` around lines 288 - 312, Update _load_secrets_for_save to catch only the expected file-access and JSON-parsing exceptions instead of all Exception values, allowing unexpected implementation errors to propagate. Change the corresponding logger.error call to include the “[Config]” context prefix and enable traceback logging with exc_info=True, while preserving the existing ConfigError and secret-protection behavior.Source: Coding guidelines
🤖 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 `@src/plugin_system/compatibility.py`:
- Around line 199-215: Update is_update_available() to reject truthy non-string
installed_version or latest_version values immediately after the missing-value
check, returning True for these malformed inputs. Keep valid string comparison
unchanged and retain the existing InvalidVersion handling for unparseable
strings.
In `@src/plugin_system/store_manager.py`:
- Around line 2981-2988: Update the version comparison in the store manager’s
manifest-present path to call is_update_available(local_version, remote_version)
without requiring either version to be truthy, while preserving the
manifest-exists check so missing manifests still use _reinstall_with_rollback().
Add store integration cases covering an empty local version and an empty
registry version, confirming both follow the shared comparator result.
---
Nitpick comments:
In `@src/config_manager.py`:
- Around line 288-312: Update _load_secrets_for_save to catch only the expected
file-access and JSON-parsing exceptions instead of all Exception values,
allowing unexpected implementation errors to propagate. Change the corresponding
logger.error call to include the “[Config]” context prefix and enable traceback
logging with exc_info=True, while preserving the existing ConfigError and
secret-protection behavior.
🪄 Autofix
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 Plus
Run ID: 0f439bb1-f2b8-4e9b-8aaa-b762ba40d4b5
📒 Files selected for processing (13)
.github/workflows/test.ymlsrc/base_classes/sports/core.pysrc/config_manager.pysrc/plugin_system/compatibility.pysrc/plugin_system/store_manager.pytest/fixtures/plugins/ci-fixture-plugin/manager.pytest/fixtures/plugins/ci-fixture-plugin/requirements.txttest/test_compatibility.pytest/test_config_manager_secrets.pytest/test_skin_system.pytest/test_version_comparison_consistency.pytest/web_interface/test_api_v3_helpers.pyweb_interface/blueprints/api_v3.py
🚧 Files skipped from review as they are similar to previous changes (7)
- test/fixtures/plugins/ci-fixture-plugin/requirements.txt
- .github/workflows/test.yml
- test/fixtures/plugins/ci-fixture-plugin/manager.py
- test/test_version_comparison_consistency.py
- test/test_skin_system.py
- test/test_compatibility.py
- test/web_interface/test_api_v3_helpers.py
- is_update_available: reject truthy non-string versions (a malformed manifest can carry a number; packaging raises TypeError on those) by surfacing the mismatch instead of raising. - store_manager.update_plugin: drop the truthiness gate around the comparator so a missing version on either side follows the shared 'no update' verdict, keeping the store consistent with the UI badge; a missing manifest still uses the reinstall recovery path. - config_manager._load_secrets_for_save: catch only expected read/parse failures (OSError/ValueError/RecursionError) so implementation bugs propagate as themselves, and log with traceback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
This was referenced Aug 7, 2026
23 tasks
ChuckBuilds
added a commit
that referenced
this pull request
Aug 7, 2026
…d coverage for every remaining untested module (#444) * refactor(web): use canonical secret helpers in api_v3; make ConfigManager secret strip/merge array-aware api_v3.py carried three inline nested copies of find_secret_fields/ separate_secrets (main-config save, plugin-config save, plugin-config reset). They drifted from each other (one lacked isinstance guards) and none supported the canonical module's array-item secrets (accounts[].token). All three endpoints now import from src/web_interface/secret_helpers. Adopting the canonical behavior makes array-item secrets reachable, and their parallel-placeholder shape ([{'token': ...}, {}] alongside the regular list) was not survivable by ConfigManager's round-trip: _strip_secrets_recursive dropped the whole key (losing the regular fields from config.json) and _deep_merge replaced the regular list wholesale on load. Both are now array-aware: - strip removes the secret fields from each item and ALWAYS keeps the list so indices survive for merge-on-load; whole-key secrets (scalar lists, shape mismatches) still drop the key entirely — never leak. - merge folds each secrets item into the config item at the same index, skipping {} placeholders. The regular list's length is authoritative in both directions: a user deleting an array item never has it resurrected from a stale secrets entry (extras warn and are ignored). api_v3's own deep_merge intentionally still replaces lists wholesale — form posts carry complete arrays and index-merging would resurrect deleted items; a comment now documents that. Tests: the parity guard flips from 'exactly 3 inline copies' to 'zero, and the canonical import must exist'; TestArraySecretStripAndMerge covers the new strip/merge semantics incl. length-mismatch contracts; new test_api_v3_secret_roundtrip.py drives all three endpoints through a Flask client with a REAL ConfigManager+SchemaManager over tmp_path, proving secrets land in config_secrets.json, config.json stays clean, and a fresh load merges them back into the right array items. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * fix: repair broken helper paths across display, cache, odds, logging, resolver, repos, config, validator Nine fixes for bugs surfaced while writing coverage for previously untested modules (plus the bool-duration quirk pinned in PR #441): - base_plugin.get_display_duration: exclude bools from both numeric branches — display_duration=True no longer reads as a 1-second slot; it falls through to config, then the 15.0 default. - display_helper: draw_error_message/draw_no_data_message called _draw_centered_text with the wrong arguments and crashed with AttributeError — both now delegate to draw_centered_text. draw_scorebug_layout drew status and clock at the same y, overprinting each other — they now share one combined top line. draw_ticker_layout drew its text starting at x=display_width (fully off-canvas), returning a blank frame every time — now draws at x=0; scroll_speed stays accepted-but-unused and is documented as such. - api_helper.clear_cache guarded on a nonexistent CacheManager.clear() method, silently never clearing anything; it now uses the real surface (clear_cache/delete/list_cache_files) and no-ops safely otherwise. - base_odds_manager._extract_espn_data raised AttributeError when ESPN sent explicit JSON nulls ("homeTeamOdds": null) — every level now null-safes with 'or {}'. format_odds_summary gated on is_odds_available, which deliberately ignores money lines, so ML-only odds formatted as "No odds available" — it now gates only on empty/no_odds data and formats money lines. - logging_config.ContextualFormatter mutated record.msg in place, so a second handler prepended the context prefix twice; it now formats a copy. log_error hardcoded exc_info=True and raised TypeError when the caller passed exc_info — now kwargs.setdefault. - dynamic_team_resolver wrote its "shared" class cache through self, creating instance shadows — the cache was per-instance and every scoreboard refetched rankings. Writes now go through the class. - saved_repositories cleaned URLs with an unanchored .replace('.git','') that mangled URLs merely containing '.git' (my.github.io -> myhub.io); now strips only a trailing suffix. add/remove also roll back the in-memory list when the save fails, so memory always matches disk. - config_helper.merge_configs shallow-copied the base, aliasing every un-overridden nested dict into the result — now deep-copies. - startup_validator.validate_all accumulated errors/warnings across calls — now resets both lists per run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: cover the previously untested modules Nine new suites plus an extension, asserting the Phase-1b fixed behavior and pinning the quirks deliberately left alone: - test_logging_config.py: formatters (JSON shape, no record mutation, single prefix through two handlers), PluginLoggerAdapter precedence, setup_logging handler hygiene and LEDMATRIX_DEBUG, log_error exc_info. - test_startup_validator.py: exact messages, error-vs-warning split, accessor split (load_config vs get_config), cache-dir branches with os.access monkeypatched (root can write anything in CI), idempotence, raise_on_errors classification precedence. - test_config_helper.py (full): load/save round trips, dot-notation get/set incl. silent-failure contract, post-fix no-aliasing merge, schema validation branches, the '{id}_config' key pin, default-enabled pin. - test_saved_repositories.py: three load shapes, bare-list rewrite pin, trailing-only .git strip (my.github.io regression), save-failure rollback, type-classification case-sensitivity pin. - test_api_helper.py: rate-limit math, cache-hit short circuit, ESPN URL/key formats, exact User-Agent guard, retry adapter, post-fix clear_cache against the real CacheManager surface, ttl-dropped pin. - test_base_odds_manager.py: cache-key/URL construction, no_odds sentinel round trip, stale-cache fallback, null-safe extraction, ML-only formatting, is_odds_available truth table (ML-blind by contract), config key/attr mismatch pin. - test_dynamic_team_resolver.py: expansion/dedup/slicing, dropped unknown-dynamic names (TOP_ substring hazard pinned), genuinely shared class cache (second instance: zero HTTP), TTL expiry, failure degradation without raising. - test_display_helper.py (full): the fixed error/no-data renders, combined scorebug top line, non-blank ticker with scroll_speed no-op pin, composite upconversion, logo bleed positions, square orientation pin. - test_skin_runtime_cache.py: discovery-cache hit/invalidation semantics (manifest mtime, .py edits pinned as non-invalidating), sys.modules namespacing contract incl. bare-name restore and stdlib shadowing, entry-module execute-once, API minor-version tolerance, skin_matches_target table. - test_sports_capabilities.py (extended): _draw_celebration_layout executed for real (flash window, matrix-dims fallback, highlight alternation, logo-failure isolation), _should_celebrate_for direct, strict duration boundary, score_to_int edges, both-teams-score precedence, expired-coalesce refire, disabled-win baseline preservation, id-less prune. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * test: real schedule/dim coverage for DisplayController; fix two vacuous schedule tests New test_display_controller_schedule.py drives _check_schedule and _check_dim_schedule on a bare controller stub: same-day and midnight-crossing windows with inclusive boundaries, global vs per-day vs legacy-inferred modes (and dim's global-only default — no legacy inference), per-day disabled days, invalid %H:%M fallbacks, unknown timezone -> UTC, dim_brightness default 30, inactive-display short circuit, and the _was_display_active/_was_dimmed transition flags. test_display_controller.py's test_schedule_disabled and test_active_hours patched config_service.get_config — which _check_schedule never reads — so both asserted the init-default value and could not fail. Rewritten on the test_inactive_hours pattern (inject controller.config['schedule'], reset the minute gate, flip the flag to the opposite state first so the assertion has teeth). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * ci: raise coverage floor to 48% Measured 50% with the new suites in place (was 47% baseline when the gate was introduced at 45); floor stays two points under measured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh * fix: address CodeQL alert and review findings - config_manager: the "secrets list longer than config list" warning now interpolates only config-side data (no key name or secrets-derived values), resolving the CodeQL clear-text-logging alert. - base_plugin: validate_config rejects bool display_duration, matching get_display_duration (bool is an int subclass and would otherwise pass as a positive number). - config_helper: merge_configs deep-copies override values in the non-recursive branch so mutating the merged result cannot reach back into override_config. - saved_repositories: saves are atomic (temp file + fsync + os.replace), so a failed write can no longer truncate saved_repositories.json. - tests: regression cases for each fix, plus a pin that whole-item array secrets (key[] + key[].field both marked) strip to empty {} skeletons — no secret values can reach config.json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh --------- Co-authored-by: Claude <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.
Pull Request
Summary
An audit of the test suite found that CI was giving false confidence: only 24 of 90 test files ran on PRs, the plugin-safety job silently skipped everything (no plugins present in CI), several enrolled tests were
assert Trueshells that could not fail, and the declared coverage gate had never executed. This PR makes CI honest (full-tree run, a bundled fixture plugin with goldens, one real coverage gate at 45%), replaces the can't-fail tests with real assertions, adds ~180 new tests covering the riskiest untested logic, and adds drift guards that pin cross-file contracts. The new tests surfaced three real production bugs, which are fixed in the final commit (see below).Type of change
Related issues
None pre-existing — the three bugs below were discovered by this PR's new tests and are fixed in commit
206eca0:store_manager.update_pluginused raw string equality while the UI's update badge usedpackaging— sov1.2.0vs1.2.0triggered a full reinstall the UI called unnecessary, and a locally-ahead plugin got silently downgraded. Both now share one comparator:compatibility.is_update_available()(PEP 440). Equivalent spellings skip the reinstall; locally-ahead versions are never downgraded; unparseable versions still reconcile via reinstall.config.json. Ifconfig_secrets.jsonexisted but couldn't be parsed at save time, bothsave_configandsave_config_atomicproceeded without stripping — writing merged secrets toconfig.jsonin plaintext. Both now raiseConfigErrorwith an actionable message instead. A missing secrets file is still fine, and_migrate_config's catch-all keeps boot resilient.render_skin_cardnow resets the shared 3-strike counter on success (both the vegas-card and mode-renderer paths), mirroring_render_game— transient card failures no longer accumulate across a session until they permanently disable a working skin.Also documented executably (not changed): the three inline
find_secret_fields/separate_secretscopies inapi_v3.pylack the canonical module's array-item secret support (test_secret_separation_parity.pyguards the copy count).Test plan
EMULATOR=true python3 run.py) — via the plugin-safety harness, which renders the new fixture plugin at all 8 default panel sizes underRGBMatrixEmulatorand compares against committed goldensscripts/dev_server.py)pytest) — full mirror of both new CI jobs locally:pytest -m "not hardware" test/ --ignore=test/plugins --cov=src --cov=web_interface --cov-fail-under=45(2,200+ tests) andLEDMATRIX_PLUGINS_DIR=test/fixtures/plugins LEDMATRIX_REQUIRE_PLUGINS=1 pytest test/plugins/(65 passed). Enabling the 63 previously-unenrolled files surfaced exactly 4 rotted tests, all fixed here. The three production fixes flip their characterization tests to assert the corrected behavior.Documentation
README.mdif user-facing behavior changeddocs/if developer behavior changedcompatibility.is_update_available,ConfigManager._load_secrets_for_save, and every new test module carries a docstring explaining what contract it pins and whyPlugin compatibility
The production changes are strictly less destructive for plugins: fewer unnecessary reinstalls, no downgrades of locally-ahead versions, and working skins stop getting disabled by stale strike counts. The new
test/fixtures/plugins/ci-fixture-pluginis a test fixture only — it lives outside the configured plugins directory and is invisible to normal discovery.Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.mdconfig_schema.json) — N/A, no config keys addedNotes for reviewer
ConfigError) instead of silently writing secrets intoconfig.json. Web routes surface it as a 500 with the actionable message; boot is unaffected (_migrate_configcatches it).pytest.inino longer carries--cov*flags; coverage lives in exactly one place, the CI unit-tests step. The unit-tests job runstest/wholesale instead of an allowlist (the allowlist rotted once — 63 files silently unenrolled); the current ignore list is empty.src/web_interface/secret_helpers,skin_runtimediscovery-cache tests,_check_schedule/_check_dim_scheduleunder freezegun, unit tests forlogging_config,startup_validator,common/*_helper,dynamic_team_resolver,base_odds_manager,saved_repositories, theget_display_durationbool-as-int quirk, and a coverage ratchet.🤖 Generated with Claude Code
https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
Summary by CodeRabbit
Bug Fixes
Tests
Chores