ci: require the newest version entry to use ledmatrix_min_version - #244
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 23 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 (5)
📝 WalkthroughWalkthroughThe PR consolidates and throttles live-content logging for baseball and football scoreboards. It adds regression tests, updates release metadata, and introduces manifest version-field validation in the plugin workflow. ChangesScoreboard and manifest updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 | 39 |
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plugins/baseball-scoreboard/manifest.json`:
- Around line 33-37: Replace the ledmatrix_min key with ledmatrix_min_version in
the release entry at plugins/baseball-scoreboard/manifest.json lines 33-37 and
the corresponding release entry at plugins/football-scoreboard/manifest.json
lines 27-31, preserving each existing version value.
In `@scripts/check_manifest_version_fields.py`:
- Around line 61-66: Update the validation around the versions[0] check so the
manifest is rejected when the newest entry contains neither OLD nor NEW.
Preserve the existing rename diagnostic when OLD is present without NEW, and
ensure manifests pass only when versions[0] includes the required NEW field.
🪄 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 Plus
Run ID: b684c2ed-3333-4120-bd0e-fe18b67a9c0d
📒 Files selected for processing (11)
.github/workflows/test-plugins.ymlplugins.jsonplugins/baseball-scoreboard/CHANGELOG.mdplugins/baseball-scoreboard/manager.pyplugins/baseball-scoreboard/manifest.jsonplugins/baseball-scoreboard/test_live_content_log_throttle.pyplugins/football-scoreboard/CHANGELOG.mdplugins/football-scoreboard/manager.pyplugins/football-scoreboard/manifest.jsonplugins/football-scoreboard/test_live_content_log_throttle.pyscripts/check_manifest_version_fields.py
ChuckBuilds
pushed a commit
that referenced
this pull request
Aug 4, 2026
…ies I wrote Both CodeRabbit findings on #244, verified against the real manifests. 1. baseball 1.21.1 and football 2.10.1 use the deprecated `ledmatrix_min` in versions[0], so they would fail this very gate on their next change. I wrote both entries earlier in this same session and matched the neighbouring (deprecated) spelling rather than the new one. Migrated. 2. An entry with NEITHER spelling passed silently. That is a real gap: the core's declared_min_version() then resolves to None, so the install gate has no floor to enforce and the plugin can reach a core that cannot run it -- the failure the B6 sunset turns fatal. Now rejected. The suggested fix for (2) -- `if NEW not in head: fail` -- would have broken four manifests that are already correct. flights, leaderboard, music and stocks declare the floor as a TOP-LEVEL `min_ledmatrix_version`, which `declared_min_version` checks BEFORE the versions[] array. Requiring the key in versions[0] unconditionally would have reported them as missing a floor they do declare, and pushed them into declaring it twice. So the missing-floor rule resolves the floor the way the core does -- top-level, then requires{}, then versions[0] -- and only fires when none of them has one. The deprecated-spelling rule is unchanged and still applies to those four, which is correct: their versions[0] key really is deprecated. Verified across all five shapes: versions[0] ledmatrix_min_version -> clean versions[0] ledmatrix_min (deprecated) -> deprecated-spelling no floor in versions[0], top-level has it -> clean no floor in versions[0], requires{} has it -> clean no floor anywhere -> missing-floor And across the real fleet: 0 of 42 plugins fail the new missing-floor rule, so it adds no breakage today. The audit is down to 37 still to migrate, from 39. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_manifest_version_fields.py`:
- Around line 84-93: Update the minimum-version validation around the newest
entry check in the manifest version-field validation flow to require a non-empty
value, not merely the presence of the NEW key. Reuse the same non-empty-value
semantics as _declares_floor_elsewhere so empty strings and null values fall
through to the existing rejection path, and add regression cases covering both
values when no top-level or requires.min_ledmatrix_version floor exists.
🪄 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: f6fe0a09-bda4-4877-8866-3c9d43ee51cb
📒 Files selected for processing (3)
plugins/baseball-scoreboard/manifest.jsonplugins/football-scoreboard/manifest.jsonscripts/check_manifest_version_fields.py
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/football-scoreboard/manifest.json
- plugins/baseball-scoreboard/manifest.json
Phase B4's manifest migration, scoped to what it is actually worth. 39 of 42 plugins still spell the floor `ledmatrix_min` in versions[0]. The tempting move is a sweep, but it would change no behaviour and cost 39 version bumps -- 39 store updates pushed to every user for a field rename: - compatibility.declared_min_version reads versions[0] and accepts either spelling, so the install gate already behaves identically for both. - store_manager's own deprecation check is warnings-only, and reachable only from the sideload path (install_from_url). So instead of rewriting 42 manifests, CI now asks each plugin to migrate the one entry that is actually read, at a moment when it is already being bumped for other reasons. The migration completes as plugins release and cannot slide backwards, at zero cost to users. The nine sports plugins get it free during B5 adoption, since they add a fresh versions[0] entry flooring at 3.2.0 anyway. Historical entries (229 of them) are deliberately left alone: nothing reads them, and rewriting shipped release records to satisfy a linter is worse than the inconsistency. The check also requires a non-empty `compatible_versions` -- the core's schema marks it required, and it is the only field that can express an upper bound, which the gate now evaluates (LEDMatrix#433). scripts/check_manifest_version_fields.py runs per changed plugin in CI (failing) and has an `--all` audit mode (reporting, non-failing) for tracking what is left. Verified: exit 1 on an unmigrated changed plugin, 0 on a migrated one, 0 for `--all` and for no arguments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
…ies I wrote Both CodeRabbit findings on #244, verified against the real manifests. 1. baseball 1.21.1 and football 2.10.1 use the deprecated `ledmatrix_min` in versions[0], so they would fail this very gate on their next change. I wrote both entries earlier in this same session and matched the neighbouring (deprecated) spelling rather than the new one. Migrated. 2. An entry with NEITHER spelling passed silently. That is a real gap: the core's declared_min_version() then resolves to None, so the install gate has no floor to enforce and the plugin can reach a core that cannot run it -- the failure the B6 sunset turns fatal. Now rejected. The suggested fix for (2) -- `if NEW not in head: fail` -- would have broken four manifests that are already correct. flights, leaderboard, music and stocks declare the floor as a TOP-LEVEL `min_ledmatrix_version`, which `declared_min_version` checks BEFORE the versions[] array. Requiring the key in versions[0] unconditionally would have reported them as missing a floor they do declare, and pushed them into declaring it twice. So the missing-floor rule resolves the floor the way the core does -- top-level, then requires{}, then versions[0] -- and only fires when none of them has one. The deprecated-spelling rule is unchanged and still applies to those four, which is correct: their versions[0] key really is deprecated. Verified across all five shapes: versions[0] ledmatrix_min_version -> clean versions[0] ledmatrix_min (deprecated) -> deprecated-spelling no floor in versions[0], top-level has it -> clean no floor in versions[0], requires{} has it -> clean no floor anywhere -> missing-floor And across the real fleet: 0 of 42 plugins fail the new missing-floor rule, so it adds no breakage today. The audit is down to 37 still to migrate, from 39. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Valid finding. The gate tested key presence, so a manifest carrying
`"ledmatrix_min_version": ""` or `null` passed while the core resolved no
floor at all -- confirmed against the merged compatibility module, which
resolves both to None. That is precisely the manifest this gate exists to
catch: one that looks migrated and leaves the install gate nothing to enforce.
It was also inconsistent within this file. _declares_floor_elsewhere already
used truthiness; only the versions[0] check did not. Both now use values,
matching how the core resolves the floor: `head.get(NEW) or head.get(OLD)`.
That `or` also decides the classification. An empty NEW alongside a real OLD
is what the core falls through to, so it is a spelling problem rather than a
missing floor, and is reported as such.
Adds scripts/test_check_manifest_version_fields.py -- 12 cases pinning both
things that are easy to get wrong here: values rather than key presence
(including the empty-string and null cases this finding names, with and
without a floor above versions[]), and that a floor declared at the top level
or in requires{} is legitimate, since the core checks there first and four
published plugins rely on it.
Verified the suite bites: reverted to the presence check and 5 of the 12 cases
fail, exit 1; restored, exit 0. Wired into the same CI step as the gate, whose
failure mode is silent enough to deserve it.
Real fleet unchanged: 0 of 42 plugins fail the missing-floor rule, audit still
reports 37 to migrate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
ChuckBuilds
force-pushed
the
ci/manifest-version-field-gate
branch
from
August 4, 2026 18:29
f3698d5 to
570220f
Compare
ChuckBuilds
pushed a commit
that referenced
this pull request
Aug 4, 2026
Extends the resolvability check to the manager as well as the display, on both import paths. It only looked at the display class, which is how a manager referencing something its module cannot supply slipped through. The extension immediately found it, independently of the review: all five legacy managers still annotate `Dict[str, ScrollDisplay]`, a class that no longer exists under that name after the rename. Corrected. On Python 3.13 that annotation is not evaluated, so the reported NameError does not reproduce there -- measured, not assumed. But it names a class that is gone, this repo supports 3.10 through 3.13, and I cannot test the older three, so it is fixed rather than argued about. Not changed, with reasons: - lacrosse's changelog panel count is correct. Measured from the harness output: 8 panel sizes x 2 screens = 16 renders, which is what it says. - hockey's versions[0] floor already uses ledmatrix_min_version. That is the spelling the migration in #244 moves toward, and the core reads both. Verified: all five pass, and the harness is still byte-for-byte identical to the pre-adoption baselines -- hockey 16/16, afl 24/24, basketball 24/24, lacrosse 16/16, nrl 24/24. Module-collision check clean across 42 plugins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
ChuckBuilds
added a commit
that referenced
this pull request
Aug 5, 2026
…ve (#249) * feat(sports): adopt the core scroll orchestration in the remaining five Completes B5's scroll adoption for every plugin that shares the shape: hockey, afl, basketball, lacrosse and nrl, following the three pilots. hockey 691 -> 308 afl 694 -> 347 basketball 730 -> 334 lacrosse 672 -> 295 nrl 733 -> 368 ufc and f1 are deliberately NOT included. They are documented forks -- ufc has no ScrollDisplay class at all, just a single 405-line ScrollDisplayManager with _determine_fight_type, and f1's is a reduced rewrite. The docs call both out as genuine forks and they stay that way. Each follows its lineage's pilot: - afl and nrl mirror soccer: one `scroll_mode` config block rather than per-league keys, a 24px gap, min/max duration bounds of 30/300 (core's own max default is 600, so leaving it implicit would double the cap), and the extra plugin_dir constructor argument derived in the subclass so core's get_scroll_display() constructs them unchanged. - hockey, basketball and lacrosse mirror baseball/football: per-league key ladders, game cards pinned at 128px where core sizes them to the panel. Hockey's ladder is five keys (nhl, ncaa_mens, ncaam_hockey, ncaa_womens, ncaaw_hockey), not the three in core's docstring example -- that example is illustrative, not hockey's real ladder. The restructure was mechanised rather than done by hand, lifting content methods verbatim from each legacy source. Transcription error is exactly what the byte-identical gate exists to catch and there was no reason to risk it across five more plugins. Verified: - byte-for-byte identical harness output for all five: afl 24/24, basketball 24/24, hockey 16/16, lacrosse 16/16, nrl 24/24. `diff -r` of before/after is empty in every case. - test_core_fallback.py added to each, and every one checked that it bites: sabotaging the guard to match the unguarded recipe makes it exit 1, the healthy tree exits 0. - module-collision check clean across 42 plugins. - suites: afl 3/3, lacrosse 5/5, nrl 3/3 pass. hockey and basketball each have two failures -- test_hockey_emulator, test_recent_games, test_plugin_syntax, test_score_fix_verification -- all four confirmed failing identically on main, in a worktree, before this branch existed. Floors stay at 2.0.0 throughout: with a working fallback these plugins do not require 3.2.0, they prefer it. The floors rise at B6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(sports): the core-backed modules could not draw CodeRabbit caught this on #249 and it is a real bug I shipped. The mechanised restructure lifted the content methods out of each legacy module but left the definitions they reach for behind: GameRenderer, ScrollHelper, RESAMPLE_FILTER, LEAGUE_NAMES and the separator-path constants. On a core that ships src.common.sports_scroll -- the primary path -- _load_separator_icons and prepare_scroll_content raise NameError. Scroll mode was broken in all five. Neither gate caught it, and that is the more important half: - The safety harness renders the scoreboard screens, not scroll mode, so "byte-for-byte identical" was true and said nothing about this code. - test_core_fallback.py checked class identity and hasattr. The methods existed; what they reached for did not. It never constructed anything. I ran a sabotage check on those tests and concluded they "bite" -- they bite for the one failure mode they model, and I generalised that into confidence they had not earned. So the test now resolves, for every method on the display class, the globals it reads, and fails on any the module cannot supply. It walks the AST for Name loads rather than bytecode co_names, which mixes in attribute names -- Image.Resampling.LANCZOS read as a missing global -- and excludes locals, arguments, imports and except-handler names. Checking the source rather than calling the method is deliberate: building a real display needs a display manager, fonts and assets, but an unresolvable global is a load-time fact. Verified the check catches the bug it was written for: on the unfixed tree all five fail naming exactly the missing symbols. After the fix all five pass, and the harness is still byte-for-byte identical to the pre-adoption baseline -- hockey 16/16, afl 24/24, basketball 24/24, lacrosse 16/16, nrl 24/24. Module-collision check clean across 42 plugins. soccer needs the same fix; baseball and football were hand-written and are already clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * test(sports): cover the manager class, and fix the stale annotations Extends the resolvability check to the manager as well as the display, on both import paths. It only looked at the display class, which is how a manager referencing something its module cannot supply slipped through. The extension immediately found it, independently of the review: all five legacy managers still annotate `Dict[str, ScrollDisplay]`, a class that no longer exists under that name after the rename. Corrected. On Python 3.13 that annotation is not evaluated, so the reported NameError does not reproduce there -- measured, not assumed. But it names a class that is gone, this repo supports 3.10 through 3.13, and I cannot test the older three, so it is fixed rather than argued about. Not changed, with reasons: - lacrosse's changelog panel count is correct. Measured from the harness output: 8 panel sizes x 2 screens = 16 renders, which is what it says. - hockey's versions[0] floor already uses ledmatrix_min_version. That is the spelling the migration in #244 moves toward, and the core reads both. Verified: all five pass, and the harness is still byte-for-byte identical to the pre-adoption baselines -- hockey 16/16, afl 24/24, basketball 24/24, lacrosse 16/16, nrl 24/24. Module-collision check clean across 42 plugins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- 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.
Phase B4's manifest migration — scoped to what it is actually worth.
Why not just sweep all 42
39 of 42 plugins still spell the floor
ledmatrix_mininversions[0]. The obvious move is a bulk rename, but I checked what it would buy and it is nothing:compatibility.declared_min_versionreadsversions[0]and accepts either spelling, so the install gate behaves identically today.store_manager's own deprecation check is warnings-only, and reachable only from the sideload path (install_from_url).Meanwhile the cost is real: CI requires a version bump whenever a plugin's non-test files change, so a sweep means 39 version bumps — 39 store updates pushed to every user for a field rename with no behaviour change.
What this does instead
CI asks each plugin to migrate the one entry that is actually read, at a moment when it is already being bumped for other reasons. The migration completes as plugins release and cannot slide backwards, at zero cost to users.
The nine sports plugins get it free during B5 adoption — they add a fresh
versions[0]entry flooring at 3.2.0 anyway.The 229 historical entries are deliberately left alone. Nothing reads them, and rewriting shipped release records to satisfy a linter is worse than the inconsistency.
Also checked
A non-empty
compatible_versions— the core's schema marks it required, and it is the only field that can express an upper bound, which the gate now evaluates (ChuckBuilds/LEDMatrix#433).Verified
scripts/check_manifest_version_fields.pyruns per changed plugin in CI (failing) and has an--allaudit mode (reporting, non-failing) for tracking what is left:news)7-segment-clock)--allaudit (reports 39 remaining)The workflow parses, and the new step sits alongside the existing version-bump gate using the same validated
IDShandling.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
New Features
Bug Fixes
Documentation