fix(aqua): route versions host by registry repo#10341
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an allow-error-status HTTP path and propagates it through retries; rewrites versions-host JSON fetching and error-message formatting; scopes GitHub release cache by a versions-host flag; and routes Aqua GitHub-release lookups through backend-aware helpers with tests. ChangesVersions-host, HTTP client, GitHub release, and Aqua integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant AquaBackend
participant GitHub
participant Cache
participant VersionsHost
Client->>AquaBackend: request install/verify
AquaBackend->>GitHub: get_github_release(repo, tag) (maps to use_versions_host)
GitHub->>Cache: lookup release_cache_key(api_url, repo, tag, use_versions_host)
alt cache miss & use_versions_host=true
GitHub->>VersionsHost: fetch metadata (allow_error_status)
VersionsHost->>GitHub: return JSON or Ok(None)
GitHub->>Cache: store fetched release under namespaced key
else cache hit
Cache->>GitHub: return cached release
end
GitHub->>AquaBackend: return release metadata
AquaBackend->>Client: proceed with asset selection / verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/versions_host.rs`:
- Around line 426-437: The message formatter versions_host_error_message
currently always labels responses "client error" even for 5xx codes; change it
to inspect the parsed reqwest::StatusCode (using methods like
is_client_error/is_server_error) and choose the correct label ("client error"
for 4xx, "server error" for 5xx, or a generic "HTTP status" otherwise), then use
that label when composing the returned string while keeping the existing
truncation and status-to-string logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 830d86f9-ad98-43be-821c-eaf1e6016ef7
📒 Files selected for processing (3)
src/backend/aqua.rssrc/github.rssrc/versions_host.rs
Greptile SummaryFixes spurious
Confidence Score: 5/5Safe to merge; the routing logic is well-scoped, the HTTP flag propagates correctly through all retry and fallback paths, and the new cache-key scheme prevents cross-contamination between hosted and direct results. The three main change areas (routing predicate, cache keys, HTTP allow-error-status flag) are each covered by targeted tests. The No files require special attention. Important Files Changed
Reviews (7): Last reviewed commit: "style(http): format retry helper call" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jdx
force-pushed
the
fix/aqua-versions-host-warnings
branch
from
8e0cdc8 to
6cee613
Compare
jdx
changed the title
|
Correction: I initially pushed this too broadly by making Aqua release metadata skip I amended the branch so the behavior is now registry-aware:
Verified with debug smoke tests for both This comment was generated by an AI coding assistant. |
jdx
force-pushed
the
fix/aqua-versions-host-warnings
branch
from
6cee613 to
2014f10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/backend/aqua.rs`:
- Around line 1508-1510: The helper use_versions_host_for_github_metadata
currently only inspects self.ba and should be made repo-aware: change its
signature to accept the resolved repo identifier (e.g. owner/repo string or a
RepoInfo struct) and use backend_arg_matches_registry_backend(self.ba, repo) (or
otherwise consult the registry backend contract with the repo) so decisions
respect override/foreign repos; update all call sites that currently call
use_versions_host_for_github_metadata (including where resolve_repo_info(...) is
used, run_minisign_check(), and github_release_asset_matching()) to pass the
resolved repo_owner/repo_name (or RepoInfo) so registry-backed lookups route
correctly. Ensure any tests or override-package paths that call
github_release_asset_matching are adjusted to supply the repo parameter.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 466fef2d-753f-45f5-ab1c-e07a3fbafd90
📒 Files selected for processing (3)
src/backend/aqua.rssrc/github.rssrc/versions_host.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/versions_host.rs
- src/github.rs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
jdx
force-pushed
the
fix/aqua-versions-host-warnings
branch
from
2014f10 to
de68324
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit de68324. Configure here.
jdx
force-pushed
the
fix/aqua-versions-host-warnings
branch
from
de68324 to
ea35917
Compare
jdx
force-pushed
the
fix/aqua-versions-host-warnings
branch
from
ea35917 to
d23ee83
Compare
jdx
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/http.rs (2)
169-178: ⚡ Quick winAdd a regression test for the new allow-error-status contract.
This helper is now the boundary that
src/versions_host.rs:367-425relies on to inspect 404/429/5xx bodies directly. A small test here that asserts non-2xx returnsOk(Response)—while offline/network failures still returnErr—would lock that behavior down before someone reintroduceserror_for_status_ref()in the wrong layer.🤖 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/http.rs` around lines 169 - 178, Add a regression test that verifies the new allow-error-status contract: call get_async_with_headers_allow_error_status (which delegates to send_with_https_fallback_allow_error_status) against a mocked HTTP server that returns non-2xx responses (e.g., 404, 429, 500) and assert the call returns Ok(Response) and that the response body can be inspected; also include a separate case that simulates an offline/network failure (or Settings::get().offline() enabled) and assert it returns Err, ensuring the helper does not call error_for_status_ref() and preserves non-2xx responses for callers like versions_host.rs.
409-556: ⚡ Quick winAvoid threading another raw
boolthrough the send stack.
error_for_statusis now a second mode flag riding through a deep call chain, and thetrue/falseliterals at Lines 421-422, 439-440, and 492 are easy to flip by accident. A tiny enum or movingSendOnceOptionsup a layer would make the new status-mode contract self-describing.🤖 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/http.rs` around lines 409 - 556, Introduce a small enum (e.g., StatusMode {ErrorForStatus, AllowErrorStatus}) or fold the status mode into SendOnceOptions and stop passing the raw bool through the call chain; change the signatures of send_with_https_fallback, send_with_https_fallback_allow_error_status, send_with_https_fallback_with_retries, send_once_with_https_fallback, send_once_with_https_fallback_with_retry_headers, and send_once_with_retry_headers to accept the enum or SendOnceOptions instance instead of a bool named error_for_status, construct the correct mode in the top-level callers (send_with_https_fallback → ErrorForStatus, send_with_https_fallback_allow_error_status → AllowErrorStatus), and update send_once_with_retry_headers to derive the proper SendOnceOptions (or use the passed-in one) rather than branching on a boolean literal so the API becomes self-describing and eliminates the fragile true/false literals.
🤖 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/http.rs`:
- Around line 169-178: Add a regression test that verifies the new
allow-error-status contract: call get_async_with_headers_allow_error_status
(which delegates to send_with_https_fallback_allow_error_status) against a
mocked HTTP server that returns non-2xx responses (e.g., 404, 429, 500) and
assert the call returns Ok(Response) and that the response body can be
inspected; also include a separate case that simulates an offline/network
failure (or Settings::get().offline() enabled) and assert it returns Err,
ensuring the helper does not call error_for_status_ref() and preserves non-2xx
responses for callers like versions_host.rs.
- Around line 409-556: Introduce a small enum (e.g., StatusMode {ErrorForStatus,
AllowErrorStatus}) or fold the status mode into SendOnceOptions and stop passing
the raw bool through the call chain; change the signatures of
send_with_https_fallback, send_with_https_fallback_allow_error_status,
send_with_https_fallback_with_retries, send_once_with_https_fallback,
send_once_with_https_fallback_with_retry_headers, and
send_once_with_retry_headers to accept the enum or SendOnceOptions instance
instead of a bool named error_for_status, construct the correct mode in the
top-level callers (send_with_https_fallback → ErrorForStatus,
send_with_https_fallback_allow_error_status → AllowErrorStatus), and update
send_once_with_retry_headers to derive the proper SendOnceOptions (or use the
passed-in one) rather than branching on a boolean literal so the API becomes
self-describing and eliminates the fragile true/false literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c9f186c-65b9-457e-94e0-9c07e193a403
📒 Files selected for processing (4)
src/backend/aqua.rssrc/github.rssrc/http.rssrc/versions_host.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend/aqua.rs
- src/github.rs
- src/versions_host.rs
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.3 x -- echo |
18.8 ± 1.3 | 16.1 | 26.6 | 1.00 |
mise x -- echo |
18.8 ± 1.1 | 17.0 | 29.8 | 1.00 ± 0.09 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.3 env |
18.8 ± 1.6 | 16.4 | 23.8 | 1.00 |
mise env |
22.9 ± 4.6 | 17.2 | 37.4 | 1.22 ± 0.26 |
env measured 22% slower, but the relative uncertainty overlaps the 10% threshold. |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.3 hook-env |
21.3 ± 1.3 | 18.6 | 27.6 | 1.00 |
mise hook-env |
22.0 ± 2.0 | 17.9 | 29.8 | 1.03 ± 0.11 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.3 ls |
16.6 ± 1.3 | 13.7 | 22.1 | 1.03 ± 0.10 |
mise ls |
16.2 ± 1.0 | 14.6 | 20.9 | 1.00 |
xtasks/test/perf
| Command | mise-2026.6.3 | mise | Variance |
|---|---|---|---|
| install (cached) | 137ms | 136ms | +0% |
| ls (cached) | 59ms | 59ms | +0% |
| bin-paths (cached) | 62ms | 63ms | -1% |
| task-ls (cached) | 124ms | 125ms | +0% |
1 participant
Summary
mise-versionsfor Aqua tools that come from the mise registry, including explicitaqua:owner/repoforms that reverse-map to a registry backendmise-versionsfor fully qualified/non-registry Aqua GitHub metadata, including foreign override repos used by verification pathsContext
This fixes noisy
403warnings like the one reported in discussion #10339 foraqua:aws/session-manager-plugin@lateston 2026.6.2. That package is in the Aqua registry but not the mise registry, andmise-versionsintentionally serves GitHub metadata only for repos known to the mise registry.The fix is not to remove the versions host from Aqua. Registry-backed tools such as
actstill usemise-versionsfor shared GitHub metadata and unauthenticated GitHub rate-limit avoidance, whether invoked asactor explicitly asaqua:nektos/act. The client now makes that decision using the resolved GitHub repo plus the mise registry mapping, so non-registry and foreign override repos go directly to GitHub instead of producing avoidable versions-host warnings.Separately, versions-host warnings now include a capped response body. This keeps expected fallbacks visible without hiding useful server-side policy messages. The fetch path still uses the normal mise HTTP client retry/fallback behavior.
Tests
cargo test test_use_versions_host_for_github_metadata_only_for_registry_toolscargo test versions_host_error_messagecargo test versions_host_flag_splits_release_cachecargo checkMISE_DEBUG=1 cargo run --quiet -- latest aqua:aws/session-manager-plugin(direct GitHub, nomise-versions)MISE_DEBUG=1 cargo run --quiet -- latest act(mise-versionsis still used)Note
Medium Risk
Changes which network path Aqua uses for GitHub releases and attestations and alters release cache keys; mistakes could mis-route metadata or serve stale/wrong cached releases for some tools.
Overview
Limits mise-versions usage for Aqua so only mise-registry-backed tools whose resolved GitHub repo matches the Aqua package id (including subpackages) fetch releases and attestation metadata through the versions host; explicit non-registry Aqua tools and foreign override repos go straight to GitHub instead of triggering avoidable 403 warnings.
Aqua GitHub release lookups (latest, assets, minisign) now go through
get_github_release, which passes that per-repo flag intoget_release_for_url_with_versions_host. GitHub release caching keys are split byhostedvsdirectso versions-host and direct API results cannot overwrite each other.The HTTP client gains an opt-out from
error_for_statuson GET (retries and HTTPS fallback unchanged). Versions-host JSON fetches use it to treat non-2xx as soft fallbacks, respect offline mode, and log warnings with a capped response body for clearer policy messages.Reviewed by Cursor Bugbot for commit b29ad33. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests