From 06657de428c9b23042db782515895f32887559f9 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Tue, 4 Aug 2026 13:29:01 -0400 Subject: [PATCH] =?UTF-8?q?fix(store):=20address=20review=20=E2=80=94=20ma?= =?UTF-8?q?lformed=20manifests,=20and=20suffixed=20versions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --- src/plugin_system/compatibility.py | 36 +++++++++++++--- test/test_plugin_compatibility_gate.py | 59 ++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/src/plugin_system/compatibility.py b/src/plugin_system/compatibility.py index aeb8d351..784a6b6a 100644 --- a/src/plugin_system/compatibility.py +++ b/src/plugin_system/compatibility.py @@ -40,7 +40,20 @@ def parse_semver(value: Any) -> Optional[Tuple[int, int, int]]: 3-tuple, or ``None`` when unparseable. A leading ``v`` is tolerated.""" if not isinstance(value, str): return None - parts = value.strip().lstrip('v').split('.') + text = value.strip().lstrip('v') + # Drop the prerelease/build suffix before scraping digits. Without this the + # scrape pulls them into the numbers: "3.2.0+build42" parsed as (3, 2, 42) + # and "3.2.0-rc1" as (3, 2, 1) -- a release candidate ranking *above* its + # own release, and a build of 3.2.0 failing an exact "3.2.0" match. + # + # Prereleases compare equal to their release here rather than below it. + # Full prerelease ordering is more than any caller needs, and equal is far + # closer to right than the old behaviour. + for sep in ('+', '-'): + head, found, _tail = text.partition(sep) + if found: + text = head + parts = text.split('.') try: nums = [int(''.join(ch for ch in p if ch.isdigit()) or 0) for p in parts[:3]] except ValueError: @@ -143,16 +156,25 @@ def declared_min_version(manifest: Dict[str, Any]) -> Optional[str]: of `ledmatrix_min_version` (`store_manager._validate_manifest_fields` flags it); both are read because a large share of published manifests still carry the old one. + + Container types are validated rather than assumed. A hand-edited or + third-party manifest can carry `requires` as a list or `versions` as a + mapping, and both used to raise out of here (`AttributeError` and + `KeyError` respectively). That now matters far more than it did: the + untrustworthy-core branch of :func:`check` calls this for *every* manifest, + so one malformed file would take down the install path rather than just + itself. A shape we do not recognise means "no declared floor". """ - declared = ( - manifest.get('min_ledmatrix_version') - or (manifest.get('requires') or {}).get('min_ledmatrix_version') - ) + declared = manifest.get('min_ledmatrix_version') + if not declared: + requires = manifest.get('requires') + if isinstance(requires, dict): + declared = requires.get('min_ledmatrix_version') if declared: return declared - versions = manifest.get('versions') or [] - if versions and isinstance(versions[0], dict): + versions = manifest.get('versions') + if isinstance(versions, list) and versions and isinstance(versions[0], dict): return (versions[0].get('ledmatrix_min_version') or versions[0].get('ledmatrix_min')) return None diff --git a/test/test_plugin_compatibility_gate.py b/test/test_plugin_compatibility_gate.py index 0d9c356b..26834ffa 100644 --- a/test/test_plugin_compatibility_gate.py +++ b/test/test_plugin_compatibility_gate.py @@ -369,3 +369,62 @@ class TestUntrustworthyCoreAndTheSunset: assert "too old to identify" not in reason, ( "a believable version should get the ordinary message" ) + + +class TestMalformedManifests: + """A manifest we cannot parse must read as "no declared floor", not raise. + + This matters more since the untrustworthy-core branch of check() began + resolving the floor for *every* manifest: one hand-edited or third-party + file with the wrong shape would take down the whole install path rather + than just itself. + """ + + @pytest.mark.parametrize("manifest", [ + {"requires": ["python>=3.9"]}, # a list, not a mapping + {"requires": "python>=3.9"}, # a bare string + {"versions": {"a": 1}}, # a mapping, not a list + {"versions": "1.0.0"}, # a bare string + {"versions": [None]}, # a list of the wrong thing + {"versions": []}, + ]) + def test_shape_errors_read_as_no_floor(self, manifest): + assert compatibility.declared_min_version(manifest) is None + assert compatibility.check(manifest, "1.0.0") == (True, None) + assert compatibility.check(manifest, "3.2.0") == (True, None) + + def test_a_valid_requires_block_still_works(self): + assert compatibility.declared_min_version( + {"requires": {"min_ledmatrix_version": "3.2.0"}}) == "3.2.0" + + +class TestSuffixedVersions: + """Prerelease and build metadata must not leak into the numbers. + + The digit scrape used to pull them in: "3.2.0+build42" became (3, 2, 42) + and "3.2.0-rc1" became (3, 2, 1) — a release candidate ranking above its + own release. Both fed reject decisions. + """ + + @pytest.mark.parametrize("text,expected", [ + ("3.2.0", (3, 2, 0)), + ("3.2.0+build42", (3, 2, 0)), + ("3.2.0-rc1", (3, 2, 0)), + ("3.2.0-rc.1+build.9", (3, 2, 0)), + ("v3.2.0+build42", (3, 2, 0)), + ]) + def test_suffixes_are_dropped(self, text, expected): + assert compatibility.parse_semver(text) == expected + + def test_a_build_of_the_pinned_version_is_not_refused(self): + """The regression: an exact "3.2.0" pin refused a core running + 3.2.0+build42, which is that same version.""" + ok, reason = compatibility.check( + {"compatible_versions": ["3.2.0"]}, "3.2.0+build42") + assert ok is True, f"refused a build of the pinned version: {reason}" + + def test_a_release_candidate_does_not_outrank_its_release(self): + m = {"versions": [{"ledmatrix_min_version": "3.2.0"}]} + assert compatibility.check(m, "3.2.0-rc1")[0] is True + # ...and still refuses something genuinely older. + assert compatibility.check(m, "3.1.0-rc1")[0] is False