From 973d54bf8a8ecaad0344371eeaf5d2b60bdfb8a2 Mon Sep 17 00:00:00 2001 From: Dario Lipicar Date: Thu, 16 Jul 2026 15:35:49 -0300 Subject: [PATCH] fix: order the catalog by semver precedence, not by release date (#4) * fix: order the catalog by semver precedence, not by release date sort_versions ordered each package's versions[] by the `releasedAt` timestamp. Every client -- the downloader's resolver, the package-manager UI's row builder, `index.py list` -- reads versions[0] as "latest". A publish time is not a version, so: - publishing 2.0.0-alpha after 1.9.0 put the alpha at versions[0] and advertised an unreleased alpha to every user as the newest release; - a 1.2.1 backported after 2.0.0 shipped did the same; - so did a forced republish, which refreshes the asset's Last-Modified -- and that is what `releasedAt` actually records. Ordering is now SemVer 2.0.0 precedence, with releasedAt only breaking ties between entries that share a version (the same version republished with a different rootHash). It is computed by `lgx semver sort` rather than reimplemented here. lgx owns the single implementation the C++ clients also use, so the catalog cannot disagree with them about which version is newest -- which is exactly how the ordering drifted in the first place. This costs nothing: the script is stdlib-only but already requires lgx on PATH for the only two subcommands that sort (`build` / `add`). If lgx predates the subcommand we abort loudly rather than falling back to a date sort, which is invisible in the output. validate's ordering check had to change too, and not only because the sort did: asserting descending releasedAt would now actively REJECT a correctly ordered catalog, since a higher version legitimately carries an older timestamp. Adds the repo's first tests (stdlib unittest) and CI. Four of them fail against the old implementation. Requires the `lgx semver` subcommand from logos-package#30. CI builds lgx from logos-package master, so it stays red until that merges. Co-Authored-By: Claude Opus 4.8 * fix: guard validate against old lgx; correct the misleading ordering example Addresses Copilot review on #4. check_version_order only checked that lgx exists, not that it has the semver subcommand, so an older lgx made semver_rank_desc() raise and crashed validate. It now degrades to 'ordering not checked' both when lgx lacks the subcommand and if the sort raises mid-run. Factored the probe into _lgx_has_semver(), shared with the build/add preflight. The '2.0.0-alpha after 1.9.0' example (docstring, catalog-format.md, and the first case of test_orders_by_semver_not_by_release_date) was wrong: under semver 2.0.0-alpha OUTRANKS 1.9.0, so date and precedence AGREE there and it never demonstrated the bug. Replaced with cases where they genuinely disagree -- 1.2.1 backported after 2.0.0, and 2.0.0-alpha published after 2.0.0. The test now fails against the old date sort (verified), where before its first assert passed under both. Adds graceful-degradation tests that run without lgx. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .github/workflows/ci.yml | 37 ++++++++ docs/catalog-format.md | 46 +++++++--- index.py | 162 ++++++++++++++++++++++++++++---- test_index.py | 194 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 407 insertions(+), 32 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 test_index.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..7270a52 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,37 @@ +name: CI + +on: + push: + branches: [main] + pull_request: + workflow_dispatch: + +jobs: + test: + name: Python tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + + - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + with: + python-version: "3.12" + + # index.py delegates version ordering to `lgx semver` so the catalog can + # never disagree with the C++ clients about which version is newest. The + # ordering tests therefore need a real lgx — without one they'd skip, and + # the thing most worth testing would go untested. + - uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30 + with: + github_access_token: ${{ secrets.GITHUB_TOKEN }} + + - name: Build lgx + run: | + nix build github:logos-co/logos-package#lgx + echo "$PWD/result/bin" >> "$GITHUB_PATH" + + - name: Check lgx has the semver subcommand + run: lgx semver compare 1.0.0-rc.2 1.0.0-rc.11 + + - name: Run tests + run: python3 -m unittest discover -v -p 'test_*.py' diff --git a/docs/catalog-format.md b/docs/catalog-format.md index aae51ab..46f179e 100644 --- a/docs/catalog-format.md +++ b/docs/catalog-format.md @@ -323,16 +323,38 @@ generator never writes it. ## 6. Version ordering and selection -`versions[]` is stored **newest-first, sorted descending by `releasedAt`**. -Both the generator (`index.py` re-sorts on every `build`/`add`) and the -client (`getCatalogJson` stable-sorts on read) enforce this, so a -hand-mangled order is corrected at read time — but keep the file sorted so -it reads correctly raw. +`versions[]` is stored **newest-first by [SemVer 2.0.0](https://semver.org/spec/v2.0.0.html) +precedence**, with `releasedAt` breaking ties between entries that share a +version. Both the generator (`index.py` re-sorts on every `build`/`add`) and +the client (`getCatalogJson` stable-sorts on read) enforce this, so a +hand-mangled order is corrected at read time — but keep the file sorted so it +reads correctly raw. + +Ordering is computed by `lgx semver`, which is the single implementation the +C++ clients (`lgpm`, `lgpd`, the package-manager UI) also use. The catalog +therefore cannot disagree with them about which version is newest. + +> **This used to be "sorted descending by `releasedAt`", and that was a bug.** +> A publish time is not a version. A `1.2.1` backported after `2.0.0` shipped +> put the *lower* version at `versions[0]` — advertised to every client as the +> latest release; publishing `2.0.0-alpha` after `2.0.0` did the same, since +> the pre-release ranks below its own release but carried the newer timestamp. +> A forced republish had the same effect (it refreshes the asset's +> `Last-Modified`, which is what `releasedAt` records). The divergence only +> appears when semver and the timestamp disagree — `2.0.0-alpha` published +> after `1.9.0` lands first under *both* orderings, because `2.0.0-alpha` +> genuinely outranks `1.9.0`. + +Precedence follows the spec: a pre-release ranks below its own release +(`1.0.0-rc.1` < `1.0.0`), numeric pre-release identifiers compare *numerically* +(`1.0.0-rc.2` < `1.0.0-rc.11`), and build metadata is ignored. A version string +that isn't parseable ranks below every one that is, so junk can never win +"latest". Selection rules the client uses when resolving "which build": -- **Newest version**: `versions[0]` after the descending sort — the entry - with the latest `releasedAt`. +- **Newest version**: `versions[0]` after the descending sort — the entry with + the highest version, *not* the most recently published one. - **A specific version string** (e.g. a user picking `1.0.0`): the client filters by the embedded `manifest.version`. If several entries share a version string (different `rootHash`), the newest by `releasedAt` wins. @@ -342,11 +364,11 @@ Selection rules the client uses when resolving "which build": validate` flags it). `releasedAt` is a *made-available* time, not a build time. In the -GitHub-Actions flow it's the release's publish time; for a self-hosted -catalog built with `index.py` it's the `.lgx`'s HTTP `Last-Modified` (or -the local file's mtime, or the run time as a last resort). Minute-level -differences between two regenerations of the same catalog are expected and -harmless. +GitHub-Actions flow it's the `.lgx` asset's HTTP `Last-Modified`; for a +self-hosted catalog built with `index.py` it's the same, or the local file's +mtime, or the run time as a last resort. Minute-level differences between two +regenerations of the same catalog are expected and harmless — and, now that it +no longer drives the ordering, inconsequential. --- diff --git a/index.py b/index.py index 10a89a5..4805b05 100755 --- a/index.py +++ b/index.py @@ -36,8 +36,15 @@ # (build/add abort, validate reports per-entry) # # `build`, `add`, and `validate --full` require the `lgx` binary on PATH -# (every package is verified). `remove` / `list` / `show` / `validate` -# (light) are pure JSON ops — no `lgx`, no network. +# (every package is verified). `remove` / `list` / `show` need no network. +# +# `lgx` also owns VERSION ORDERING. `versions[]` is stored newest-first by +# SemVer 2.0.0 precedence (see sort_versions), and that order is computed by +# `lgx semver` rather than reimplemented here — it is the same implementation +# the C++ clients (lgpm, lgpd, the package-manager UI) use, so the catalog +# cannot disagree with them about which version is newest. `validate` (light) +# therefore also consults `lgx` when it is present, and reports the ordering as +# unchecked when it isn't. # # Install lgx: nix build github:logos-co/logos-package#lgx # @@ -119,6 +126,31 @@ def require_lgx() -> None: ) +def _lgx_has_semver() -> bool: + """True if an `lgx` with the `semver` subcommand is on PATH.""" + if shutil.which("lgx") is None: + return False + r = subprocess.run(["lgx", "semver", "compare", "1.0.0", "1.0.0"], + capture_output=True) + return r.returncode == 0 + + +def require_lgx_semver() -> None: + """Preflight: the `lgx` on PATH must have the `semver` subcommand. + + Catalog ordering is delegated to it (see semver_rank_desc). An older lgx + predates it — abort loudly rather than silently falling back to sorting by + release date, which is the bug this replaced and which is invisible in the + output.""" + if not _lgx_has_semver(): + die( + "the `lgx` on PATH has no `semver` subcommand (it predates it).\n" + " Version ordering is delegated to lgx so the catalog agrees\n" + " with the clients; refusing to fall back to a date sort.\n" + " Update with: nix build github:logos-co/logos-package#lgx" + ) + + def lgx_run(*args: str) -> bytes: """Run `lgx ` and return stdout bytes. Raises RuntimeError on non-zero exit, with the (decoded) stderr in the message.""" @@ -516,14 +548,71 @@ def merge_version(index: dict, name: str, entry: dict) -> bool: return True +def entry_version(entry: dict) -> str: + """The version string of an index entry. `manifest` is legally null in + catalogs produced by early action runs, hence the `or {}`.""" + manifest = entry.get("manifest") or {} + return manifest.get("version") or "" + + +def semver_rank_desc(versions: list[str]) -> dict[str, int]: + """Map each version to its rank, 0 = newest, by SemVer 2.0.0 precedence. + + Shells out to `lgx semver` rather than reimplementing semver here. `lgx` + owns the single implementation that the C++ clients (lgpm, lgpd, the + package-manager UI) also use, so the catalog cannot disagree with them + about which version is newest — which is exactly how the ordering drifted + before. It costs nothing: this script is stdlib-only but already requires + `lgx` on PATH for the only two subcommands that sort (`build` / `add`). + + Entries with no version string rank last. + """ + real = [v for v in versions if v] + ordered: list[str] = [] + if real: + out = lgx_run("semver", "sort", "--desc", *real).decode("utf-8", "replace") + ordered = [line.strip() for line in out.splitlines() if line.strip()] + if sorted(ordered) != sorted(real): + raise RuntimeError( + "lgx semver sort returned an unexpected set of versions " + f"(asked for {sorted(real)}, got {sorted(ordered)})" + ) + + rank = {v: i for i, v in enumerate(ordered)} + for v in versions: + if not v: + rank[v] = len(ordered) + return rank + + def sort_versions(index: dict) -> None: - """Sort each package's `versions` descending by releasedAt so the - client's "newest first" picker (`findBest` in the downloader) - matches the order the catalog actually intends.""" + """Order each package's `versions` newest-first by SemVer precedence, + tie-breaking on `releasedAt`. + + This used to sort on `releasedAt` alone. A release timestamp is not the + same thing as a version, and every client — the downloader's resolver, the + package-manager UI's row builder, `index.py list` — treats `versions[0]` as + "latest". Publishing `1.2.1` (a backport) after `2.0.0` put the *lower* + version at `versions[0]`; so did publishing `2.0.0-alpha` after `2.0.0`, + since the pre-release ranks below its own release but carried the newer + timestamp. A forced republish had the same effect (it refreshes the asset's + Last-Modified, which is what `releasedAt` actually records). Note it is only + a divergence when semver and the timestamp disagree: `2.0.0-alpha` published + after `1.9.0` lands first under *both* orderings, because `2.0.0-alpha` + genuinely outranks `1.9.0`. + + `releasedAt` still breaks ties *within* one version, e.g. the same version + republished with a different rootHash. + """ for pkg in index["packages"]: - pkg["versions"].sort( - key=lambda v: v.get("releasedAt", ""), reverse=True - ) + entries = pkg["versions"] + if len(entries) < 2: + continue + # Pre-sort by date; the rank sort below is stable, so this survives as + # the tiebreak between entries sharing a version. + entries.sort(key=lambda v: v.get("releasedAt", ""), reverse=True) + rank = semver_rank_desc([entry_version(v) for v in entries]) + entries.sort(key=lambda v: rank[entry_version(v)]) def bump_generated_at(index: dict) -> None: @@ -534,6 +623,7 @@ def bump_generated_at(index: dict) -> None: def cmd_build(args: argparse.Namespace) -> int: require_lgx() + require_lgx_semver() urls_file = pathlib.Path(args.urls_file) if not urls_file.exists(): die(f"urls file not found: {urls_file}") @@ -570,6 +660,7 @@ def cmd_build(args: argparse.Namespace) -> int: def cmd_add(args: argparse.Namespace) -> int: require_lgx() + require_lgx_semver() index_path = pathlib.Path(args.index) index = load_index(index_path) @@ -704,9 +795,46 @@ def cmd_show(args: argparse.Namespace) -> int: # ── subcommand: validate ───────────────────────────────────────────────── +def check_version_order(ctx: str, name: str, versions: list) -> list[str]: + """`versions` must be newest-first by SemVer precedence, since every client + reads `versions[0]` as "latest". + + This used to assert descending `releasedAt`. That check now has to go, and + not just because the sort changed: it would actively *reject* a correctly + ordered catalog, because a higher version legitimately carries an older + timestamp (a backport, or a forced republish refreshing Last-Modified). + + Deciding the order needs semver, and semver lives in `lgx`. Light validate + is documented as working without `lgx`, so when `lgx` is missing — or is too + old to have the `semver` subcommand — we report the ordering as unchecked + rather than crashing or quietly passing it. + """ + if not _lgx_has_semver(): + warn(f"{ctx} ({name}): version ordering not checked — " + "`lgx` is missing or has no `semver` subcommand") + return [] + + actual = [entry_version(v) for v in versions if isinstance(v, dict)] + if len(actual) < 2: + return [] + + try: + rank = semver_rank_desc(actual) + except RuntimeError as e: + warn(f"{ctx} ({name}): version ordering not checked — {e}") + return [] + for i in range(len(actual) - 1): + if rank[actual[i]] > rank[actual[i + 1]]: + return [ + f"{ctx} ({name}): out of order — versions must be sorted newest-first " + f"by semver precedence, but {actual[i]!r} precedes {actual[i + 1]!r}" + ] + return [] + + def _validate_light(index_path: pathlib.Path, index: dict) -> list[str]: - """Structural + internal consistency. No network, no `lgx` — just - walks the JSON tree and reports every inconsistency it finds. + """Structural + internal consistency. No network; uses `lgx` only to check + version ordering, and says so when it can't (see check_version_order). Returns a list of human-readable problems (empty = clean).""" issues: list[str] = [] @@ -742,7 +870,10 @@ def _validate_light(index_path: pathlib.Path, index: dict) -> list[str]: continue seen_keys: set[tuple[str, str]] = set() - previous_released: str | None = None + # Ordering is checked once per package, after the per-entry walk — it is + # a property of the sequence, not of any single entry, and it needs + # `lgx semver` (see check_version_order). + issues.extend(check_version_order(ctx, name, versions)) for vi, v in enumerate(versions): vctx = f"{ctx}.versions[{vi}] ({name})" if not isinstance(v, dict): @@ -764,15 +895,6 @@ def _validate_light(index_path: pathlib.Path, index: dict) -> list[str]: issues.append( f"{vctx}: manifest.name {mname!r} != package name {name!r}" ) - # Sort order - released = v.get("releasedAt") - if isinstance(released, str) and previous_released is not None: - if released > previous_released: - issues.append( - f"{vctx}: out of order — versions must be sorted " - f"descending by releasedAt" - ) - previous_released = released if isinstance(released, str) else previous_released # Dedup key key = (manifest.get("version", ""), v.get("rootHash", "")) if key in seen_keys: diff --git a/test_index.py b/test_index.py new file mode 100644 index 0000000..380ab05 --- /dev/null +++ b/test_index.py @@ -0,0 +1,194 @@ +#!/usr/bin/env python3 +"""Tests for index.py's catalog ordering. + +Standard library only (`python3 -m unittest`), matching index.py's own +no-third-party-deps contract. + +These need the `lgx` binary on PATH — ordering is deliberately delegated to +`lgx semver` so the catalog cannot disagree with the C++ clients about which +version is newest. Without lgx the ordering tests skip rather than silently +pass, which would defeat the point. + + nix build github:logos-co/logos-package#lgx # then put ./result/bin on PATH +""" + +import shutil +import subprocess +import unittest + +import index + + +def lgx_has_semver() -> bool: + if shutil.which("lgx") is None: + return False + r = subprocess.run(["lgx", "semver", "compare", "1.0.0", "1.0.0"], + capture_output=True) + return r.returncode == 0 + + +requires_lgx = unittest.skipUnless( + lgx_has_semver(), "needs an `lgx` with the `semver` subcommand on PATH") + + +def entry(version: str, released_at: str, root_hash: str = "h") -> dict: + """A minimal index version entry — only the fields ordering looks at.""" + return { + "releasedAt": released_at, + "rootHash": root_hash, + "manifest": {"name": "demo_module", "version": version}, + } + + +def catalog(*entries: dict) -> dict: + return {"packages": [{"name": "demo_module", "versions": list(entries)}]} + + +def ordered_versions(index_doc: dict) -> list: + return [index.entry_version(v) for v in index_doc["packages"][0]["versions"]] + + +@requires_lgx +class TestSortVersions(unittest.TestCase): + + def test_orders_by_semver_not_by_release_date(self): + """The bug this replaced. + + A pre-release published *after* its own stable release used to land at + versions[0], and every client reads versions[0] as "latest" — so + 2.0.0-alpha published after 2.0.0 advertised the alpha to everyone. + + The case has to be one where semver and the timestamp genuinely + disagree. 2.0.0-alpha vs 1.9.0 would NOT: 2.0.0-alpha outranks 1.9.0, + so it lands first under both the old date sort and the new one. + """ + doc = catalog( + entry("2.0.0-alpha", "2026-06-01T00:00:00Z"), # newest by DATE + entry("2.0.0", "2026-05-01T00:00:00Z"), # newest by VERSION + ) + index.sort_versions(doc) + self.assertEqual(ordered_versions(doc), ["2.0.0", "2.0.0-alpha"], + "a stable release must outrank its own later-published alpha") + + def test_backport_published_later_does_not_become_latest(self): + """A 1.2.1 hotfix cut after 2.0.0 shipped has a newer timestamp but is + an older version. It used to take versions[0].""" + doc = catalog( + entry("1.2.1", "2026-06-01T00:00:00Z"), + entry("2.0.0", "2026-01-01T00:00:00Z"), + ) + index.sort_versions(doc) + self.assertEqual(ordered_versions(doc), ["2.0.0", "1.2.1"]) + + def test_numeric_prerelease_identifiers_order_numerically(self): + """Spec §11: rc.11 is newer than rc.2. A string sort says otherwise.""" + doc = catalog( + entry("1.0.0-rc.2", "2026-01-01T00:00:00Z"), + entry("1.0.0-rc.11", "2026-01-02T00:00:00Z"), + entry("1.0.0", "2026-01-03T00:00:00Z"), + ) + index.sort_versions(doc) + self.assertEqual(ordered_versions(doc), + ["1.0.0", "1.0.0-rc.11", "1.0.0-rc.2"]) + + def test_ordering_is_independent_of_release_dates(self): + """Same versions, dates deliberately inverted — the order must not move.""" + ascending_dates = catalog( + entry("1.0.0", "2026-01-01T00:00:00Z"), + entry("2.0.0", "2026-02-01T00:00:00Z"), + entry("1.5.0", "2026-03-01T00:00:00Z"), + ) + descending_dates = catalog( + entry("1.0.0", "2026-03-01T00:00:00Z"), + entry("2.0.0", "2026-02-01T00:00:00Z"), + entry("1.5.0", "2026-01-01T00:00:00Z"), + ) + index.sort_versions(ascending_dates) + index.sort_versions(descending_dates) + self.assertEqual(ordered_versions(ascending_dates), ["2.0.0", "1.5.0", "1.0.0"]) + self.assertEqual(ordered_versions(descending_dates), ["2.0.0", "1.5.0", "1.0.0"]) + + def test_releasedAt_breaks_ties_within_one_version(self): + """The same version republished (different rootHash): newest publish wins.""" + doc = catalog( + entry("1.0.0", "2026-01-01T00:00:00Z", root_hash="old"), + entry("1.0.0", "2026-02-01T00:00:00Z", root_hash="new"), + ) + index.sort_versions(doc) + hashes = [v["rootHash"] for v in doc["packages"][0]["versions"]] + self.assertEqual(hashes, ["new", "old"]) + + def test_unparseable_versions_sort_last(self): + """A junk version string must never win "latest".""" + doc = catalog( + entry("banana", "2026-09-01T00:00:00Z"), + entry("1.0.0", "2026-01-01T00:00:00Z"), + ) + index.sort_versions(doc) + self.assertEqual(ordered_versions(doc), ["1.0.0", "banana"]) + + def test_single_and_empty_version_lists_are_untouched(self): + doc = catalog(entry("1.0.0", "2026-01-01T00:00:00Z")) + index.sort_versions(doc) + self.assertEqual(ordered_versions(doc), ["1.0.0"]) + + +@requires_lgx +class TestValidateVersionOrder(unittest.TestCase): + + def test_flags_a_semver_misordered_catalog(self): + issues = index.check_version_order( + "packages[0]", "demo_module", + [entry("1.0.0", "2026-02-01T00:00:00Z"), + entry("2.0.0", "2026-01-01T00:00:00Z")]) + self.assertTrue(issues) + self.assertIn("out of order", issues[0]) + + def test_accepts_a_correctly_ordered_catalog(self): + # Note the dates run "backwards" — that is legal and must not be flagged, + # which the old descending-releasedAt check got wrong. + issues = index.check_version_order( + "packages[0]", "demo_module", + [entry("2.0.0", "2026-01-01T00:00:00Z"), + entry("1.0.0", "2026-02-01T00:00:00Z")]) + self.assertEqual(issues, []) + + +class TestValidateWithoutLgx(unittest.TestCase): + """check_version_order must degrade gracefully when lgx can't rank — + missing, or too old to have the `semver` subcommand — rather than crash. + These run regardless of whether lgx is present.""" + + def test_reports_unchecked_when_lgx_lacks_semver(self): + # Simulate an lgx with no `semver` subcommand. + original = index._lgx_has_semver + index._lgx_has_semver = lambda: False + try: + issues = index.check_version_order( + "packages[0]", "demo_module", + [entry("1.0.0", "2026-02-01T00:00:00Z"), + entry("2.0.0", "2026-01-01T00:00:00Z")]) # genuinely misordered + finally: + index._lgx_has_semver = original + # Misordered, but unverifiable without semver: no crash, no false pass. + self.assertEqual(issues, []) + + def test_does_not_crash_if_ranking_raises(self): + # lgx claims semver support but the sort call blows up mid-run. + orig_has, orig_rank = index._lgx_has_semver, index.semver_rank_desc + index._lgx_has_semver = lambda: True + def boom(_): + raise RuntimeError("lgx semver sort failed: boom") + index.semver_rank_desc = boom + try: + issues = index.check_version_order( + "packages[0]", "demo_module", + [entry("1.0.0", "2026-01-01T00:00:00Z"), + entry("2.0.0", "2026-01-02T00:00:00Z")]) + finally: + index._lgx_has_semver, index.semver_rank_desc = orig_has, orig_rank + self.assertEqual(issues, []) + + +if __name__ == "__main__": + unittest.main()