Repository navigation
feat(github): support additional release assets - #11272
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesAdditional GitHub asset overlays
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant GitHubBackend
participant Lockfile
participant InstallDirectory
Config->>GitHubBackend: provide primary and additional_asset_patterns
GitHubBackend->>Lockfile: resolve and record additional_artifacts
GitHubBackend->>GitHubBackend: download and verify supplemental archives
GitHubBackend->>InstallDirectory: overlay supplemental contents
Lockfile-->>GitHubBackend: provide recorded artifacts for --locked install
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryAdds supplemental release-asset overlays for GitHub, GitLab, and Forgejo backends.
Confidence Score: 5/5The PR appears safe to merge because no blocking failures eligible for this follow-up review remain. No blocking failure remains. Important Files Changed
Reviews (8): Last reviewed commit: "fix(github): count overridden supplement..." | Re-trigger Greptile |
![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
🧹 Nitpick comments (1)
src/lockfile.rs (1)
236-236: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
deny_unknown_fieldson a lockfile-persisted struct is a forward-compat trap.The rest of the lockfile (
PlatformInfo::try_from) parses leniently and ignores unknown keys (e.g. legacyprovenance_url).ArtifactInfowith#[serde(deny_unknown_fields)]will instead hard-fail the whole lockfile read if a newer mise version adds any field to anadditional_artifactsentry. Consider droppingdeny_unknown_fieldsto keep older clients able to read newer lockfiles.🤖 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/lockfile.rs` at line 236, Remove the #[serde(deny_unknown_fields)] attribute from the lockfile-persisted ArtifactInfo struct so deserialization ignores fields added by newer mise versions, matching the lenient behavior of PlatformInfo::try_from and preserving older-client compatibility.
🤖 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 `@docs/dev-tools/backends/github.md`:
- Around line 84-86: Update the lockfile documentation around supplemental
artifacts to qualify provenance: state that verification and recorded provenance
apply only to the current platform, while cross-platform entries are
detection-only and failed verification records no provenance. Remove the
unconditional claim that every supplemental artifact has verified provenance,
and retain the existing --locked behavior description.
In `@e2e/backend/test_github_additional_assets`:
- Around line 26-30: Extend the locked-install test around mise install --locked
with a temporary incomplete supplemental artifact entry, then assert that the
locked installation fails rather than resolving the missing artifact. Restore
the original configuration afterward and retain the existing successful install
assertion to verify complete lockfile entries still work.
---
Nitpick comments:
In `@src/lockfile.rs`:
- Line 236: Remove the #[serde(deny_unknown_fields)] attribute from the
lockfile-persisted ArtifactInfo struct so deserialization ignores fields added
by newer mise versions, matching the lenient behavior of PlatformInfo::try_from
and preserving older-client compatibility.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a6e86b1a-570d-4e36-b80b-3dd2b5b65710
📒 Files selected for processing (6)
docs/dev-tools/backends/github.mde2e/backend/test_github_additional_assetssrc/backend/github.rssrc/backend/options.rssrc/backend/static_helpers.rssrc/lockfile.rs
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.13 x -- echo |
19.5 ± 1.0 | 17.8 | 27.3 | 1.00 |
mise x -- echo |
20.9 ± 1.0 | 19.2 | 25.1 | 1.07 ± 0.08 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.13 env |
18.3 ± 0.9 | 16.6 | 21.4 | 1.00 |
mise env |
19.7 ± 0.9 | 18.0 | 24.5 | 1.08 ± 0.07 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.13 hook-env |
19.1 ± 0.8 | 17.5 | 22.1 | 1.00 |
mise hook-env |
20.6 ± 1.0 | 18.8 | 25.0 | 1.08 ± 0.07 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.13 ls |
16.3 ± 0.9 | 14.7 | 21.4 | 1.00 |
mise ls |
17.6 ± 1.0 | 16.1 | 23.2 | 1.08 ± 0.08 |
xtasks/test/perf
| Command | mise-2026.7.13 | mise | Variance |
|---|---|---|---|
| install (cached) | 154ms | 166ms | -7% |
| ls (cached) | 58ms | 60ms | -3% |
| bin-paths (cached) | 65ms | 68ms | -4% |
| task-ls (cached) | 135ms | 138ms | -2% |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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)
src/lockfile.rs (1)
250-252: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not treat detected provenance as verified provenance.
src/backend/github.rs:808-860stores detected provenance for every target but verifies it only when the target is current. A foreign-target entry therefore has a checksum andSome(provenance), making this predicate true;src/backend/github.rs:1374-1457then skips provenance verification on the target host unless forced. Persist an explicit verified state (defaulting to false) and gate the skip on that state; add a cross-platform lock/install regression test.🤖 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/lockfile.rs` around lines 250 - 252, Update Lockfile’s has_checksum_and_verified_provenance predicate to require an explicit persisted verified state, defaulting that state to false for existing or newly detected provenance; ensure GitHub lock/install handling only skips provenance verification when this state is true, while detected provenance alone remains unverified. Add a cross-platform regression test covering a foreign-target entry through lock/install and confirming verification is performed.
🤖 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 `@src/lockfile.rs`:
- Around line 250-252: Update Lockfile’s has_checksum_and_verified_provenance
predicate to require an explicit persisted verified state, defaulting that state
to false for existing or newly detected provenance; ensure GitHub lock/install
handling only skips provenance verification when this state is true, while
detected provenance alone remains unverified. Add a cross-platform regression
test covering a foreign-target entry through lock/install and confirming
verification is performed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e60ad4bd-eaec-4ef5-8d20-e42784b01961
📒 Files selected for processing (1)
src/lockfile.rs
|
Addressed the outside-diff provenance feedback in 0240d8a. Lock entries now persist an explicit Validation: all 66 lockfile tests, all 33 GitHub backend tests, the supplemental-assets e2e, and full project lint pass. This comment was generated by an AI coding assistant. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lockfile.rs (1)
236-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct test for
merge_provenance_state's verified-over-unverified precedence.The new merge tests (
test_additional_artifacts_merge_integrity_for_same_urls,test_provenance_merge_preserves_existing,test_detected_cross_platform_provenance_is_not_verified) all useprovenance_verified: false(default) on both sides of a merge, so the actual precedence rules inmerge_provenance_state(verified wins over unverified regardless of which side is "current"/"other") are never exercised through a mismatched-flags merge. This is exactly the guarantee the PR's follow-up fix targets ("Lockfile merge behavior preserves verified provenance without promoting detection-only metadata"), so a focused unit test would lock in the regression protection at the source of truth rather than only at the higher-level consumers.♻️ Suggested test
#[test] fn test_merge_provenance_state_prefers_verified() { let verified = Some(ProvenanceType::GithubAttestations); let unverified = verified.clone(); // Verified current beats unverified other. assert_eq!( merge_provenance_state(verified.clone(), true, unverified.clone(), false), (verified.clone(), true) ); // Verified other beats unverified current. assert_eq!( merge_provenance_state(unverified, false, verified.clone(), true), (verified, true) ); }🤖 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/lockfile.rs` around lines 236 - 257, Add a focused unit test for merge_provenance_state that covers both argument orders: verified current with unverified other, and unverified current with verified other. Assert each merge preserves the verified ProvenanceType and returns true for the verification flag.
🤖 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 `@src/lockfile.rs`:
- Around line 236-257: Add a focused unit test for merge_provenance_state that
covers both argument orders: verified current with unverified other, and
unverified current with verified other. Assert each merge preserves the verified
ProvenanceType and returns true for the verification flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 099c86e7-2bb5-4f5a-ba2a-99acaef06000
📒 Files selected for processing (3)
e2e/backend/test_github_additional_assetssrc/backend/github.rssrc/lockfile.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/github.rs
|
Also addressed the CodeRabbit test-coverage nit in 3c5b315: This comment was generated by an AI coding assistant. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d00be26. Configure here.
Summary
additional_asset_patternsto the GitHub/GitLab/Forgejo release backendMotivation
Discussion #11259 needs both Ollama's normal Linux archive and its optional ROCm archive in one installation. The aqua package model selects one release asset, while changing the existing registry entry would make the large ROCm payload unconditional. An explicit GitHub backend overlay keeps the accelerator choice user-controlled and models the upstream release layout directly.
Validation
cargo test additional_mise run test:e2e e2e/backend/test_github_additional_assetsmise run lint-fixmise run lintThis pull request was generated by an AI coding assistant.
Note
Medium Risk
Touches install satisfaction, lockfile merge semantics, and provenance skip paths for multi-artifact installs; mistakes could cause wrong overlays or overly strict/lenient locked installs.
Overview
Adds
additional_asset_patternson the GitHub/GitLab/Forgejo backend so one tool install can pull extra release archives from the same tag and overlay them into the primary asset’s install directory (later files win on path conflicts). Supplemental archives skip primary-only layout options (strip_components,bin,rename_exe).Install and lock behavior: Each supplemental asset is resolved, downloaded, checksummed, and provenance-verified on its own. Lockfiles gain ordered
additional_artifacts(ArtifactInfoper file) plusprovenance_verifiedso “detected” vs cryptographically verified provenance is explicit.--lockedrequires the lock’s supplemental list to match config; changing patterns after install is tracked via.mise-additional-assets.tomlso adding ROCm-style extras to an already-installed version is not skipped.Docs distinguish one composed installation (this option, e.g. Ollama + ROCm) from separate tools (aliases /
matchingper binary). E2E covers overlay install, lock pinning, and locked-mode strictness.Reviewed by Cursor Bugbot for commit 75c0575. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
additional_asset_patterns, overlaying them into the primary install directory.--lockedfails if the recorded additional set/provenance doesn’t fully match.asset_patternis provided, it takes precedence over overlay pattern resolution.additional_asset_patterns, including overlay behavior and--lockedexpectations.--lockedstrictness.