fix(gltest): support GenVM 0.3.0 runner-bundle asset rename#79
Closed
danieljrc888 wants to merge 5 commits into
Closed
fix(gltest): support GenVM 0.3.0 runner-bundle asset rename#79danieljrc888 wants to merge 5 commits into
danieljrc888 wants to merge 5 commits into
Conversation
The direct runner resolved GitHub's bare /releases/latest and always downloaded genvm-universal.tar.xz. GenVM v0.3.0-rc0 shipped as a non-prerelease and dropped that asset, so consumers (gltest direct mode and GLSim, which share this loader) broke with HTTP 404. - get_latest_version() now queries the releases API and returns the newest release that is not a pre-release and still ships genvm-universal.tar.xz, instead of trusting GitHub's "latest" pointer. - resolve_version() adds a GENVM_VERSION env var so callers can pin a deterministic version regardless of what GitHub marks as latest.
📝 WalkthroughWalkthroughReplaces /latest HEAD discovery with GitHub Releases iteration that skips drafts/prereleases and requires expected runner bundle assets, adds ENV-pinning and fallback, deterministic cached-version sorting, asset-fallback downloads with atomic streaming, and corresponding unit tests plus a CI step. ChangesSDK Version Resolution via GitHub API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
The sdk_loader version-resolution tests live in tests/gltest_direct/, which CI did not run. Add a step for the fast unit tests only; the integration tests there download the GenVM SDK and stay out of CI.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56e70b8b98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| req = urllib.request.Request( | ||
| f"{GITHUB_RELEASES_URL}/latest", | ||
| method="HEAD", | ||
| f"{GITHUB_API_RELEASES}?per_page=100", |
There was a problem hiding this comment.
get_latest_version() only fetches ?per_page=100 once and then falls back to FALLBACK_VERSION if no matching asset is found in that single page. Since GitHub release listing is paginated (max 100 per page), once there are more than 100 newer releases without genvm-universal.tar.xz, this code will incorrectly return the hardcoded fallback instead of the newest compatible release, leading the direct runner to use a stale pinned version (or fail if that fallback tag/asset is removed).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@gltest/direct/sdk_loader.py`:
- Around line 93-95: The cached selection assumes cached[0] is newest but
list_cached_versions() returns lexicographically-sorted strings, so change the
selection logic to parse and compare versions semantically (e.g., use
packaging.version.parse or semantic_version.Version) and pick the max by
semantic comparison instead of cached[0]; update the code that currently does
"cached = list_cached_versions(); if cached: return cached[0]" to compute the
semantically-largest entry (and optionally ignore unparsable entries or fall
back to lexicographic if parsing fails).
- Around line 78-80: The current broad "except Exception:" block around the
release resolution silently discards all errors and always returns
FALLBACK_VERSION; change it to either (A) catch only expected exceptions (e.g.,
network/HTTP and parsing errors such as requests.RequestException, ValueError)
and return FALLBACK_VERSION for those, or (B) if keeping a general except, at
minimum log the full exception and traceback (using logging.exception or
similar) before returning FALLBACK_VERSION, and re-raise truly unexpected
errors; update the except block that currently contains "except Exception: pass"
to implement one of these approaches so errors are not silently swallowed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7633d09-ca1f-462f-b430-92d3290b364a
📒 Files selected for processing (2)
gltest/direct/sdk_loader.pytests/gltest_direct/test_sdk_loader.py
GenVM 0.3.0 renamed the runner bundle from genvm-universal.tar.xz to
genvm-runners-all.tar.xz. Both archives have the identical internal
runners/{type}/{hash}.tar layout and the new bundle is a strict
superset of the old runner hashes, so a contract that pins a runner
hash resolves the same runner from either.
download_artifacts() now tries each known bundle asset name and uses
whichever the release publishes, normalizing the cache filename so the
rest of the loader is unaffected. get_latest_version() accepts a
release shipping either asset. This lets the direct runner work across
the asset rename without freezing on a pre-0.3.0 release.
danieljrc888
changed the title
Reduce comment noise: collapse multi-line docstrings to one line and drop comments that just restate the code.
…ilure Address review feedback on PR #79: - list_cached_versions() sorted lexicographically, so cached[0] could pick an older release (e.g. v0.2.9 ahead of v0.2.16). Sort by numeric components instead. - get_latest_version() swallowed every exception silently; emit a stderr warning before falling back so resolution failures are visible.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gltest/direct/sdk_loader.py (1)
90-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle prereleases explicitly in cached version sorting.
re.findall()makesv0.3.0-rc0sort afterv0.3.0, so once both tarballs are cachedresolve_version()will default back to the RC on the unpinned path.Suggested fix
def _version_sort_key(version: str) -> tuple: - """Numeric-component sort key so v0.2.16 ranks above v0.2.9.""" - return tuple(int(n) for n in re.findall(r"\d+", version)) + """Sort stable releases above prereleases for the same base version.""" + core, sep, suffix = version.partition("-") + core_parts = tuple(int(n) for n in re.findall(r"\d+", core)) + suffix_parts = tuple(int(n) for n in re.findall(r"\d+", suffix)) + return core_parts, 1 if not sep else 0, suffix_parts🤖 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 `@gltest/direct/sdk_loader.py` around lines 90 - 105, The current _version_sort_key uses re.findall to extract digits which treats prereleases like "v0.3.0-rc0" the same as "v0.3.0", causing RCs to be ordered after stables; update _version_sort_key to explicitly parse the main numeric version and an optional prerelease suffix (e.g., split with a regex capturing the numeric part and an optional "-..." prerelease), produce a sort key of (numeric_tuple, is_prerelease_flag, prerelease_tuple) where is_prerelease_flag is 0 for stable and 1 for prerelease so stables rank above prereleases when reverse=True, and parse prerelease components into comparable ints/strings (fallback to strings for non-numeric parts); keep list_cached_versions unchanged except it will now rely on the improved _version_sort_key to sort correctly.
🧹 Nitpick comments (1)
tests/gltest_direct/test_sdk_loader.py (1)
50-97: ⚡ Quick winAdd an explicit draft-release test for
get_latest_version().The suite verifies prerelease and asset filtering, but not draft filtering. Since draft skipping is part of the release-selection contract, it’s worth locking with one test to prevent regressions.
Proposed test addition
class TestGetLatestVersion: """get_latest_version() skips pre-releases and assetless releases.""" @@ def test_skips_prereleases(self, monkeypatch): releases = [ @@ assert sdk_loader.get_latest_version() == "v0.2.16" + + def test_skips_draft_releases(self, monkeypatch): + releases = [ + { + "tag_name": "v0.3.1", + "draft": True, + "prerelease": False, + "assets": [{"name": "genvm-runners-all.tar.xz"}], + }, + { + "tag_name": "v0.2.16", + "draft": False, + "prerelease": False, + "assets": [{"name": "genvm-universal.tar.xz"}], + }, + ] + monkeypatch.setattr( + "urllib.request.urlopen", lambda *a, **k: _FakeResponse(releases) + ) + assert sdk_loader.get_latest_version() == "v0.2.16"🤖 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 `@tests/gltest_direct/test_sdk_loader.py` around lines 50 - 97, Add a new test method on TestGetLatestVersion that verifies get_latest_version() skips draft releases: create a releases list where the newest tag (e.g., "v0.4.0") has "draft": True and an appropriate asset (e.g., "genvm-universal.tar.xz") and an older non-draft release (e.g., "v0.2.16") is present; monkeypatch urllib.request.urlopen to return _FakeResponse(releases) and assert sdk_loader.get_latest_version() returns the non-draft tag ("v0.2.16"). Ensure the new test follows the existing pattern (use monkeypatch, _FakeResponse) and name it something like test_skips_draft_releases to match the suite.
🤖 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.
Outside diff comments:
In `@gltest/direct/sdk_loader.py`:
- Around line 90-105: The current _version_sort_key uses re.findall to extract
digits which treats prereleases like "v0.3.0-rc0" the same as "v0.3.0", causing
RCs to be ordered after stables; update _version_sort_key to explicitly parse
the main numeric version and an optional prerelease suffix (e.g., split with a
regex capturing the numeric part and an optional "-..." prerelease), produce a
sort key of (numeric_tuple, is_prerelease_flag, prerelease_tuple) where
is_prerelease_flag is 0 for stable and 1 for prerelease so stables rank above
prereleases when reverse=True, and parse prerelease components into comparable
ints/strings (fallback to strings for non-numeric parts); keep
list_cached_versions unchanged except it will now rely on the improved
_version_sort_key to sort correctly.
---
Nitpick comments:
In `@tests/gltest_direct/test_sdk_loader.py`:
- Around line 50-97: Add a new test method on TestGetLatestVersion that verifies
get_latest_version() skips draft releases: create a releases list where the
newest tag (e.g., "v0.4.0") has "draft": True and an appropriate asset (e.g.,
"genvm-universal.tar.xz") and an older non-draft release (e.g., "v0.2.16") is
present; monkeypatch urllib.request.urlopen to return _FakeResponse(releases)
and assert sdk_loader.get_latest_version() returns the non-draft tag
("v0.2.16"). Ensure the new test follows the existing pattern (use monkeypatch,
_FakeResponse) and name it something like test_skips_draft_releases to match the
suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b54bc3c-b261-409b-95f7-78847b10d54d
📒 Files selected for processing (3)
.github/workflows/tests.ymlgltest/direct/sdk_loader.pytests/gltest_direct/test_sdk_loader.py
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.
Problem
The direct test runner (
gltest/direct/sdk_loader.py, also used by GLSim) resolved GitHub's baregenvm/releases/latestand unconditionally downloadedgenvm-universal.tar.xz.GenVM
v0.3.0-rc0renamed that asset togenvm-runners-all.tar.xz(and was published as a non-prerelease), so every consumer that resolved "latest" broke withHTTP Error 404: Not Found— gltest direct mode and GLSim alike.The two archives are otherwise identical: same internal
runners/{type}/{hash}.tarlayout, andgenvm-runners-all.tar.xzis a strict superset of the old runner hashes — a contract that pins a runner hash in itsDependsheader resolves the same runner from either bundle.Changes
download_artifacts()now tries each known bundle asset name (genvm-runners-all.tar.xz, thengenvm-universal.tar.xz) and uses whichever the release publishes. The local cache filename is normalized, solist_cached_versions()/extract_runner()are unaffected.get_latest_version()queries the releases API and returns the newest non-prerelease release shipping a known bundle asset, instead of trusting GitHub's/releases/latestpointer.resolve_version()(new) adds aGENVM_VERSIONenv var for deterministic pinning. Precedence:GENVM_VERSION> newest cached version > latest resolvable release.Tests
tests/gltest_direct/test_sdk_loader.py— 11 fast unit tests (no network): version-resolution precedence, pre-release / assetless-release skipping, the renamed-asset fallback, and cached-tarball reuse.Verified locally: unit tests pass;
test_deploy_storage_contractpasses end-to-end with bothGENVM_VERSION=v0.2.16(oldgenvm-universal.tar.xz) andGENVM_VERSION=v0.3.0-rc0(newgenvm-runners-all.tar.xz).Summary by CodeRabbit
New Features
Bug Fixes
Tests