feat(store): evaluate compatible_versions, not just the floor - #433
Merged
Conversation
Closes the gap CodeRabbit surfaced on #427. `compatible_versions` is the canonical compatibility contract -- schema/manifest_schema.json marks it required, all 42 published manifests carry it -- and it is the only field that can express an *upper* bound. `ledmatrix_min_version` is a floor and cannot say "not compatible with 4.x". The gate read only the floor, so a plugin declaring ["2.0.0 - 2.9.9"] would be installed on 3.2.0 regardless of having said it stops at 2.x. check() now evaluates both and the more restrictive wins. The array is a set of alternatives (satisfying any one entry suffices), supporting every form the schema permits: >=, <=, >, <, ~, ^, a bare exact version, and an inclusive "A - B" range, with prerelease/build suffixes tolerated. Refusal still requires evidence. Anything unparseable, absent, or below TRUSTWORTHY_FLOOR resolves to compatible. That last point needed a new strict parser. parse_semver is deliberately lenient -- it strips non-digits and yields (0, 0, 0) for a string with no numbers at all. Harmless for a floor (0.0.0 never blocks) but wrong for a range, where the same leniency turned an unreadable spec into a *refusal*: a manifest whose only entry was garbage got compared against 0.0.0 and refused. Range specs are now shape-checked first, so garbage reads as "no evidence". parse_semver itself is unchanged, since the loader depends on its behaviour. Verified: 815 core unit tests pass, 18 of them new. Swept the real registry -- all 42 published manifests, at cores 1.0.0 / 2.0.0 / 3.1.0 / 3.2.0 / 4.0.0 -- and nothing is refused at any of them. The gate stays inert for shipped plugins, which is the property that makes it safe to land ahead of B5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Contributor
|
Warning Review limit reached
Next review available in: 8 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 (2)
📝 WalkthroughWalkthroughThe plugin compatibility gate parses ChangesPlugin compatibility evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PluginCompatibilityCheck
participant CompatibleVersions
participant MinimumVersionFloor
participant CoreVersion
PluginCompatibilityCheck->>CoreVersion: parse and assess trust
PluginCompatibilityCheck->>CompatibleVersions: evaluate declared semver range
CompatibleVersions-->>PluginCompatibilityCheck: compatible, incompatible, or uncertain
PluginCompatibilityCheck->>MinimumVersionFloor: evaluate minimum-version constraint
MinimumVersionFloor-->>PluginCompatibilityCheck: compatibility result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/test_plugin_compatibility_gate.py (2)
260-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd return type annotations to the new test methods.
Annotate each new test method with
-> None. Annotate the parameterized test inputs at Line 260.Proposed change
- def test_range_forms(self, spec, core, expected): + def test_range_forms( + self, spec: str, core: str, expected: bool + ) -> None: ... - def test_array_is_alternatives_not_conjunction(self): + def test_array_is_alternatives_not_conjunction(self) -> None: ... - def test_absent_or_unparseable_is_no_evidence(self): + def test_absent_or_unparseable_is_no_evidence(self) -> None: ... - def test_upper_bound_blocks_a_core_that_clears_the_floor(self): + def test_upper_bound_blocks_a_core_that_clears_the_floor(self) -> None:Apply the same
-> Noneannotation to the remaining new test methods. As per coding guidelines, "Use type hints for function parameters and return values."Also applies to: 265-265, 272-272, 285-285, 295-295, 303-303, 308-308
🤖 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_plugin_compatibility_gate.py` at line 260, Add -> None return annotations to the new test methods, including test_range_forms and the methods at the referenced locations; preserve their existing parameters and test behavior while applying the repository’s typing style consistently.Source: Coding guidelines
240-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstrings to the new test classes.
TestCompatibleVersionsandTestMoreRestrictiveWinshave no class docstrings.Proposed change
class TestCompatibleVersions: + """Test compatible-version range evaluation.""" + ... class TestMoreRestrictiveWins: + """Test range and minimum-version precedence.""" +As per coding guidelines, "Use docstrings for classes and complex functions."
Also applies to: 284-284
🤖 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_plugin_compatibility_gate.py` at line 240, Add concise class docstrings to the new test classes TestCompatibleVersions and TestMoreRestrictiveWins in test_plugin_compatibility_gate.py, describing the behavior each test class covers. Do not alter the test logic.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.
Nitpick comments:
In `@test/test_plugin_compatibility_gate.py`:
- Line 260: Add -> None return annotations to the new test methods, including
test_range_forms and the methods at the referenced locations; preserve their
existing parameters and test behavior while applying the repository’s typing
style consistently.
- Line 240: Add concise class docstrings to the new test classes
TestCompatibleVersions and TestMoreRestrictiveWins in
test_plugin_compatibility_gate.py, describing the behavior each test class
covers. Do not alter the test logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7651668b-3509-4975-9ddf-32247a53933a
📒 Files selected for processing (2)
src/plugin_system/compatibility.pytest/test_plugin_compatibility_gate.py
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 32 |
| 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.
The B6 sunset deletes each plugin's guarded-import fallback, so a plugin that
floors at 3.2.0 must never reach a core that lacks the 3.2.0 modules. The gate
could not stop that for the population most at risk.
A device installed from the v3.1.0 release reports __version__ = "1.0.0". The
gate treated anything below TRUSTWORTHY_FLOOR as "unknown, do not block" --
correct while every manifest floors at 2.0.0, because blocking would have
emptied the plugin store for those users. But after the sunset it hands them a
3.2.0-floored plugin with no fallback, which fails to load with one log line.
Nothing else in the system protects them: they cannot be told apart from a
genuine 1.0.0 install.
On an untrustworthy core the gate now refuses a floor ABOVE 2.0.0 and still
allows anything at or below it. A floor above the ecosystem baseline says the
plugin needs modules that arrived after 2.0.0, and a core reporting below that
-- whether it is the v3.1.0 release or something genuinely ancient -- will not
have them. Refusing leaves the user on the version they already run instead of
one that cannot load.
Measured against all 42 published manifests:
today (every manifest floors at 2.0.0)
core 1.0.0 / 2.0.0 / 3.1.0 / 3.2.0 / unparseable -> 0 of 42 refused
after B6 (same manifests floored at 3.2.0)
core 1.0.0 -> 38 refused, core 3.1.0 -> 38 refused, core 3.2.0 -> 0
So nobody loses the store today, and the sunset cannot reach a core that
cannot run it.
Two older tests asserted the previous "allow everything" behaviour; they now
express the new rule with a 2.0.0 floor, which is what their no-lockout intent
was actually about.
The 38-of-42 in that measurement surfaced a separate B6 trap, recorded here
because it will bite whoever raises the floors: four plugins (flights,
leaderboard, music, stocks) declare the floor as a TOP-LEVEL
`min_ledmatrix_version`, a third spelling, which declared_min_version checks
before the versions[] array. For those, editing versions[0] is a silent no-op
and the floor stays at 2.0.0.
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: 2
🧹 Nitpick comments (3)
test/test_plugin_compatibility_gate.py (2)
339-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
UNTRUSTWORTHYimmutable.Ruff reports RUF012 because
UNTRUSTWORTHYis a mutable class attribute. Use a tuple so test parameters cannot share mutable state.As per coding guidelines, “Keep tests independent and ensure they do not depend on each other.”
Proposed fix
- UNTRUSTWORTHY = ["1.0.0", "0.9.0", "1.9.9"] + UNTRUSTWORTHY = ("1.0.0", "0.9.0", "1.9.9")🤖 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_plugin_compatibility_gate.py` at line 339, Change the UNTRUSTWORTHY class attribute from a list to an immutable tuple, preserving the existing version values and their order so the test behavior remains unchanged.Sources: Coding guidelines, Linters/SAST tools
84-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the new test methods.
Add
-> Noneto each new test method. Addcore: strto the parametrized methods.As per coding guidelines, “Use type hints for function parameters and return values.”
Also applies to: 342-371
🤖 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_plugin_compatibility_gate.py` around lines 84 - 101, Add type annotations to the new compatibility tests: annotate both test methods with a None return type, and update the parametrized test methods in the referenced area to declare the core parameter as str. Preserve the existing test behavior and assertions.Source: Coding guidelines
src/plugin_system/compatibility.py (1)
187-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the low-core exception.
The contract says every unparseable or below-
TRUSTWORTHY_FLOORcore resolves as compatible. This branch rejects floors above2.0.0. State this exception in the docstring.As per coding guidelines, “Make intentions clear through naming and structure (Explicit over Implicit principle).”
🤖 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/compatibility.py` around lines 187 - 209, Add or update the docstring for the function containing the TRUSTWORTHY_FLOOR check to explicitly document the exception: low-core versions (below TRUSTWORTHY_FLOOR or unparseable) are normally treated as compatible, but this exception does not apply when the manifest's declared minimum version (evaluated via parse_semver and declared_min_version) exceeds TRUSTWORTHY_FLOOR. Make the intended behavior clear by stating that such high-floor declarations are rejected for low-core systems to prevent incompatible plugin installations.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 202-214: Update the compatibility reject paths around
declared_min_version, satisfies_compatible_versions, and sunset handling to use
the strict SemVer token parser for both current and needed versions before any
comparison can return False. Preserve None for malformed declarations, avoid
rejecting valid 3.2.0 values with build metadata or release prefixes, and add
regression tests covering both inputs.
- Around line 202-203: Update the manifest-version handling around
declared_min_version(manifest) and parse_semver(declared) to validate the
requires and versions container types before accessing them. Treat malformed or
unsupported shapes—including non-empty requires lists and mappings in
versions—as having no declared floor, and continue without raising exceptions.
---
Nitpick comments:
In `@src/plugin_system/compatibility.py`:
- Around line 187-209: Add or update the docstring for the function containing
the TRUSTWORTHY_FLOOR check to explicitly document the exception: low-core
versions (below TRUSTWORTHY_FLOOR or unparseable) are normally treated as
compatible, but this exception does not apply when the manifest's declared
minimum version (evaluated via parse_semver and declared_min_version) exceeds
TRUSTWORTHY_FLOOR. Make the intended behavior clear by stating that such
high-floor declarations are rejected for low-core systems to prevent
incompatible plugin installations.
In `@test/test_plugin_compatibility_gate.py`:
- Line 339: Change the UNTRUSTWORTHY class attribute from a list to an immutable
tuple, preserving the existing version values and their order so the test
behavior remains unchanged.
- Around line 84-101: Add type annotations to the new compatibility tests:
annotate both test methods with a None return type, and update the parametrized
test methods in the referenced area to declare the core parameter as str.
Preserve the existing test behavior and assertions.
🪄 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: 8989f9aa-1212-4ba6-8f0c-d28e44d8cc49
📒 Files selected for processing (2)
src/plugin_system/compatibility.pytest/test_plugin_compatibility_gate.py
Both CodeRabbit findings on #433 verified against the code and fixed. 1. Malformed manifest sections raised instead of degrading. `requires` as a list hit AttributeError ('list' object has no attribute 'get') and `versions` as a mapping hit KeyError: 0. Both reproduced. This got worse with the sunset rule in the previous commit: that branch resolves the floor for *every* manifest on an untrustworthy core, where the old code returned early. One hand-edited or third-party file with the wrong shape would have taken down the whole install path rather than just itself. Container types are now validated and an unrecognised shape reads as "no declared floor". 2. Prerelease and build metadata leaked into the version numbers. The digit scrape parsed "3.2.0+build42" as (3, 2, 42) and "3.2.0-rc1" as (3, 2, 1) -- a release candidate ranking above its own release. Both fed reject decisions, and the consequence was demonstrable: a plugin pinned to exactly "3.2.0" refused a core running 3.2.0+build42, which is that same version. The suggested remedy -- use the strict token parser -- would not have fixed it. _parse_strict validates the shape but delegates the numbers to parse_semver, so it returned the same (3, 2, 42). The bug is in the scrape, so suffixes are now dropped before it. Prereleases compare equal to their release rather than below it; full prerelease ordering is more than any caller needs and equal is far closer to right than what it did before. parse_semver is shared with PluginLoader, so its suite was re-run: unchanged, and it only ever gets more correct here. Verified: 839 core unit tests pass, 21 of them new -- six malformed shapes, five suffixed forms, and the two demonstrated regressions. The real-registry sweep is unchanged at 0 of 42 refused across cores 1.0.0, 3.1.0, 3.2.0, 3.2.0+build42 and an unparseable string. 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 11, 2026
A line wrapped onto "#433), the newest manifest entry ...", which markdownlint reads as a malformed ATX heading (MD018). Reflowed so the line starts with "(#431, #433)" instead. Not the suggested fix: adding a space after the hash would have turned the PR reference into "# 433". The B5 safety claim raised alongside this was already corrected in ac44b5a. 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 21, 2026
The heading read "B5 — adoption is safe by construction", which this same document disproves two sections later: four of the eight adopted plugins shipped with scroll mode broken on a 3.2.0 core and were repaired in plugins #251. The body was already careful -- it says fallback compatibility is what is guaranteed, and that correctness on a core which *does* ship the module needs object-level and scroll-mode validation. The heading was not, and a heading is what a reader scanning the plan actually takes away. Retitled to name both halves, with a sentence up front saying why the unqualified claim is false and pointing at the retrospective that shows it. The phase intro said "one of them is safe by construction and the other is not"; that now says what it actually means -- one cannot break a user on an old core, the other can. The second review point, MD018 on the ATX heading at line 409, does not reproduce: that line now begins "(#431, #433)" rather than "#433)", so there is no bare-hash heading. `grep -cE '^#+[^ #]'` returns 0 for the whole file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
ChuckBuilds
added a commit
that referenced
this pull request
Sep 1, 2026
Sideloading a plugin from a URL validated required manifest fields, warned on version-field and schema problems, and then installed whatever it had found -- never once comparing the core version. With this, all three routes in refuse on the same rule: install_plugin (#431/#433), the git-pull update path (the commit before this one), and now sideloading. The argument for gating it is not the same as for the other two, and worth stating. An automatic store update is something that happens TO a user; a sideload is something they chose. But the floor is not advice about intent -- it is a statement that the plugin cannot run on this core. Letting it through produces exactly the silent PluginState.ERROR at load that the store gate exists to prevent, and someone who pasted a URL is no better placed to diagnose that than someone who pressed Update. Checked after the download, like the others, because the manifest is the only place the floor is written. Placed before the move into plugins/, so the existing `finally` removes the temp tree and nothing half-installed survives a refusal. The reason string is returned to the caller, so the web UI shows the same actionable message the store path already gives. TestSideloadGate pins the three cases the other routes pin: refuses a floor above this core and leaves nothing behind, still allows a compatible plugin (the guard against a gate that refuses everything and passes the first test), and does not block a 2.0.0 floor on a core reporting an untrustworthy version -- the v3.1.0-reports-1.0.0 population, who must not be locked out. TestInstallGate's docstring said this route was still ungated. It no longer is, so it now points at TestSideloadGate rather than describing a gap. Verified the tests bite: removing the check fails the refusal case. Full suite 3723 passed, 6 skipped. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChuckBuilds
added a commit
that referenced
this pull request
Sep 2, 2026
This PR was opened to say B6 was deliberately held. It has since run, so the document said the opposite of the truth -- which is the exact failure it was written to fix: "a plan that misreports which phase it is in is worse than no plan". Merges current main first (53 commits), which brings in #508's corrections to this same file, then replaces the hold with what happened. **Why the hold lifted is worth recording, because the stated gate was never met.** It asked for evidence of 3.2.0 uptake, and that evidence could not arrive: the core updates by `git pull --rebase`, so release-asset counts cannot measure it, and no store-side telemetry exists. What changed is that the risk the gate protected against was closed directly -- the store now refuses a plugin whose floor exceeds the running core on all three routes in: install_plugin (#431/#433), update_plugin's git branch (#508), and install_from_url (#510). A pre-3.2.0 user cannot receive a sunset plugin at all, so they keep the version they run. Refusal replaced the bundled copy, which is what the copy stood in for. Records what shipped (eight plugins, ~5,800 lines, plugins #346/#349/#350/#351) and the two findings worth carrying to the next module: baseball's fallback was the only one holding orchestration logic the core lacked, and two tests had been leaning on the guard -- soccer's stubbed `src` in a way that shadowed the core, so it had been exercising the frozen copy rather than the shipping class since B5. The remaining-work list is replaced too. Its first item was "nothing on the critical path, B6 is waiting on calendar time", which is no longer true. What remains: hardware soaks (with a note to check the rig's display_mode first, or a board in switch mode tells you nothing about the scroll code), cutting 3.3.0 -- not required by B6, whose floors are 3.2.0, but calendar 1.2.3 floors at 3.3.0 and is un-installable until it exists -- and reconsidering the modules held back during the sunset. Keeps the pre-B6 sections as history. The reasoning still applies to the next module; it is just no longer in force for this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
ChuckBuilds
added a commit
that referenced
this pull request
Sep 2, 2026
…511) #510 shows as merged, but into fix/gate-git-pull-updates -- #508's branch -- rather than main. #508 reached main first, so the sideload gate was left behind on a branch. Same failure as plugins #350/#351, which merged into each other's bases; worth knowing the pattern, because GitHub reports these as MERGED and `gh pr list` shows nothing outstanding. main today has two of the three routes gated: install_plugin (#431/#433) and update_plugin's git branch (#508). install_from_url validates required manifest fields and then installs whatever it found, never comparing the core version. Cherry-picked unchanged from the orphaned branch -- it applies to main with no conflict. TestSideloadGate pins the three cases the other routes pin: refuses a floor above this core leaving nothing behind, still allows a compatible plugin (the guard against a gate that refuses everything), and does not block a 2.0.0 floor on a core reporting an untrustworthy version. Full suite 3725 passed, 6 skipped. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChuckBuilds
added a commit
that referenced
this pull request
Sep 2, 2026
* docs(sports): record where B6 stands, and why it is waiting
The phase table had B4 as "next" and B5 as "after B4" while both had shipped,
and described B6 as blocked on B4's gate — which is now merged and released. A
plan that misreports which phase it is in is worse than no plan: the next
person reads it and repeats finished work.
Corrected, and three things that were only ever decided in conversation are now
written down:
* **B6 is deliberately held.** 3.2.0 published 2026-08-03; 3.1.0 ran nine
months before it. B6's premise is that cores without the module are gone,
and there is no release-asset count or install telemetry to show that.
Running it now strands users on their current plugin versions. The gate
that makes it safe is already built and tested — it is the calendar that is
missing, and no amount of further code changes that.
* **Stop adopting further shared modules** (data_sources, game_renderer,
base_odds_manager) until B6 closes. Each adoption adds a copy to keep in
step against a payoff contingent on B6.
* **A B5 retrospective**, because "the adoption went fine" is not what
happened: four of eight shipped with scroll mode broken on a 3.2.0 core.
The bundled fallback did not protect against it — the break was on the
modern path — which is an argument for the sunset, not against it. Records
the ledger too: net negative on disk until B6 runs.
Also replaces the "what's next" list, whose first five items were all done,
with what actually remains.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
* fix: apply CodeRabbit auto-fixes
Fixed 1 file(s) based on 2 unresolved review comments.
Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
* docs(sports): stop a wrapped PR reference reading as a heading
A line wrapped onto "#433), the newest manifest entry ...", which
markdownlint reads as a malformed ATX heading (MD018). Reflowed so the
line starts with "(#431, #433)" instead.
Not the suggested fix: adding a space after the hash would have turned
the PR reference into "# 433". The B5 safety claim raised alongside this
was already corrected in ac44b5a.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
* docs(sports): scope the B5 safety claim to the fallback
The heading read "B5 — adoption is safe by construction", which this same
document disproves two sections later: four of the eight adopted plugins
shipped with scroll mode broken on a 3.2.0 core and were repaired in
plugins #251.
The body was already careful -- it says fallback compatibility is what is
guaranteed, and that correctness on a core which *does* ship the module
needs object-level and scroll-mode validation. The heading was not, and a
heading is what a reader scanning the plan actually takes away.
Retitled to name both halves, with a sentence up front saying why the
unqualified claim is false and pointing at the retrospective that shows
it. The phase intro said "one of them is safe by construction and the
other is not"; that now says what it actually means -- one cannot break a
user on an old core, the other can.
The second review point, MD018 on the ATX heading at line 409, does not
reproduce: that line now begins "(#431, #433)" rather than "#433)", so
there is no bare-hash heading. `grep -cE '^#+[^ #]'` returns 0 for the
whole file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
* docs(sports): re-check the hold, and close two items that are already done
The remaining-work list had two entries that finished without the doc noticing,
which is the failure mode this file exists to prevent.
- The stale plugin-test tranche is gone. run_plugin_tests.py --all now reports
174 passed, 2 skipped, 0 failed across the whole fleet. Recorded how to
re-check it too: these are standalone scripts, not a pytest suite, and one
calls sys.exit(1) at import, so pointing pytest at a plugin directory
collapses into an INTERNALERROR that looks nothing like the real state.
- CLAUDE.md already says eight panel sizes.
That leaves the hardware soaks as the only open item needing work rather than
calendar time.
B6's prerequisite is now built -- core test/test_sports_sunset_matrix.py
(#505) -- so the phase table and the regression-test section say so, and the
two modelling traps it had to work through are recorded for whoever touches it
next: the copy-removed shape must be an unguarded import or the failure names
scroll_display_legacy instead of the core module, and only the leaf module may
be hidden because a pre-3.2.0 core still ships src/common/.
The hold itself is re-checked and unchanged: v3.2.0 is still latest,
__version__ is still 3.2.0, no 3.3.0, 23 days rather than the few months the
gate asks for. Also worth stating plainly -- the core updates by git pull, not
by downloading a release, so release-asset counts would not measure uptake even
if we had them. Whatever unblocks this has to come from the store side.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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
1 participant
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.
Closes the gap CodeRabbit surfaced reviewing #427, and recorded in
docs/SPORTS_UNIFICATION.mdas something that must land before B6.The gap
compatible_versionsis the canonical contract —schema/manifest_schema.jsonmarks it required, and all 42 published manifests carry it. It is also the only field that can express an upper bound;ledmatrix_min_versionis a floor and cannot say "not compatible with 4.x".The gate read only the floor. A plugin declaring
["2.0.0 - 2.9.9"]— meaning it stops at 2.x — would be installed on 3.2.0 anyway.The change
check()now evaluates both, and the more restrictive wins. The array is a set of alternatives (satisfying any one entry suffices), covering every form the schema permits:>=,<=,>,<,~,^, a bare exact version, and an inclusiveA - Brange, with prerelease/build suffixes tolerated.Refusal still requires evidence: anything unparseable, absent, or below
TRUSTWORTHY_FLOORresolves to compatible.A flaw my own test caught
That last principle needed a new strict parser.
parse_semveris deliberately lenient — it strips non-digits and yields(0, 0, 0)for a string with no numbers at all. Harmless for a floor (a floor of0.0.0never blocks), but wrong for a range, where the same leniency turned an unreadable spec into a refusal: a manifest whose only entry was garbage got compared against0.0.0and refused.Range specs are now shape-checked before parsing, so garbage reads as "no evidence".
parse_semveritself is unchanged — the loader depends on its current behaviour and its test only pins non-string inputs.Verified
1.0.0,2.0.0,3.1.0,3.2.0and4.0.0.The gate stays inert for shipped plugins — the property that makes it safe to land ahead of B5.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit