Conversation
`parse_goproxy()` used `env::var("GOPROXY").unwrap_or_else(...)`, which
only falls back to DEFAULT_GOPROXY when the variable is unset — not when
it is set to the empty string. The e2e harness passes `GOPROXY=` into
the mise process, so proxies came back empty, `fetch_proxy_versions`
returned None, and the `go list -m -versions` fallback returned no
versions for submodules without tagged releases (e.g.
`github.com/go-kratos/kratos/cmd/kratos/v2`).
PR #9276 exposed this: resolution now errors with "no versions found"
instead of falling through to `resolve_prefix("latest")`, so the kratos
`mise x @latest` test in `test_go_install_slow` failed with exit 127.
Match `go env GOPROXY`, which treats `GOPROXY=` the same as unset.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes a regression where Confidence Score: 5/5Safe to merge — the fix is minimal, correct, and well-validated; the only finding is a P2 style note on the test. The production code change is a targeted, idiomatic Rust fix that directly matches the documented Go tooling behaviour. All remaining feedback is P2 (unsafe env mutation in tests), which does not block merge. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[parse_goproxy called] --> B{std::env::var GOPROXY}
B -- Err: unset --> C[use DEFAULT_GOPROXY]
B -- Ok empty string --> D{.filter: is_empty?}
B -- Ok non-empty value --> E[use env value]
D -- true: empty --> C
D -- false: non-empty --> E
C --> F[parse_goproxy_value DEFAULT_GOPROXY]
E --> G[parse_goproxy_value env value]
F --> H[Vec of GoProxy entries]
G --> H
Reviews (1): Last reviewed commit: "fix(go): treat empty GOPROXY as default" | Re-trigger Greptile |
| // SAFETY: test is single-threaded; matches `go env GOPROXY` behavior for GOPROXY=. | ||
| // The prev guard restores the previous value so parallel test runs stay stable. | ||
| struct EnvGuard(Option<String>); | ||
| impl Drop for EnvGuard { | ||
| fn drop(&mut self) { | ||
| match &self.0 { | ||
| Some(v) => unsafe { std::env::set_var("GOPROXY", v) }, | ||
| None => unsafe { std::env::remove_var("GOPROXY") }, | ||
| } | ||
| } | ||
| } | ||
| let _g = EnvGuard(std::env::var("GOPROXY").ok()); | ||
| unsafe { std::env::set_var("GOPROXY", "") }; | ||
| let proxies = parse_goproxy(); | ||
| assert_eq!(proxies.len(), 1); | ||
| assert_eq!(proxies[0].url, "https://proxy.golang.org"); | ||
| } |
There was a problem hiding this comment.
unsafe env mutation in parallel test runner
The comment says "test is single-threaded," but Rust's default test harness runs tests concurrently across threads. Calling std::env::set_var / remove_var while other threads may be reading env vars is a data race (undefined behavior in the OS layer), which is why these functions were made unsafe in Rust 1.81. The EnvGuard ensures cleanup but does not prevent concurrent readers from seeing a torn state.
In practice the risk here is low because all sibling tests call parse_goproxy_value(...) directly and never touch the env, but the safety comment is misleading and the pattern is fragile. A safer approach is to test the underlying parse_goproxy_value("") path instead, or isolate the env test with #[serial_test::serial] if env mutation is truly needed.
There was a problem hiding this comment.
Code Review
This pull request updates the parse_goproxy function to handle empty environment variables by falling back to the default value and introduces a test case for this scenario. The review feedback suggests refactoring the implementation to use a helper function, which would allow for safer, thread-safe testing by avoiding direct manipulation of global environment variables.
| fn parse_goproxy() -> Vec<GoProxy> { | ||
| let goproxy = std::env::var("GOPROXY").unwrap_or_else(|_| DEFAULT_GOPROXY.to_string()); | ||
| // Treat unset or empty GOPROXY as the default, matching `go env GOPROXY`. | ||
| let goproxy = std::env::var("GOPROXY") | ||
| .ok() | ||
| .filter(|s| !s.is_empty()) | ||
| .unwrap_or_else(|| DEFAULT_GOPROXY.to_string()); | ||
| parse_goproxy_value(&goproxy) | ||
| } |
There was a problem hiding this comment.
Modifying the global environment in tests using std::env::set_var is generally discouraged in Rust because tests run in parallel by default. This can lead to flakiness and potential undefined behavior if other tests access the environment simultaneously.
Consider refactoring parse_goproxy to use a helper function that takes the environment value as an argument. This allows testing the logic without side effects, removes the need for unsafe blocks in tests, and also avoids unnecessary string allocations in the default case.
| fn parse_goproxy() -> Vec<GoProxy> { | |
| let goproxy = std::env::var("GOPROXY").unwrap_or_else(|_| DEFAULT_GOPROXY.to_string()); | |
| // Treat unset or empty GOPROXY as the default, matching `go env GOPROXY`. | |
| let goproxy = std::env::var("GOPROXY") | |
| .ok() | |
| .filter(|s| !s.is_empty()) | |
| .unwrap_or_else(|| DEFAULT_GOPROXY.to_string()); | |
| parse_goproxy_value(&goproxy) | |
| } | |
| fn parse_goproxy() -> Vec<GoProxy> { | |
| let val = std::env::var("GOPROXY").ok(); | |
| parse_goproxy_from_val(val.as_deref()) | |
| } | |
| fn parse_goproxy_from_val(val: Option<&str>) -> Vec<GoProxy> { | |
| // Treat unset or empty GOPROXY as the default, matching go env GOPROXY. | |
| let goproxy = val.filter(|s| !s.is_empty()).unwrap_or(DEFAULT_GOPROXY); | |
| parse_goproxy_value(goproxy) | |
| } |
| fn parse_goproxy_empty_uses_default() { | ||
| // SAFETY: test is single-threaded; matches `go env GOPROXY` behavior for GOPROXY=. | ||
| // The prev guard restores the previous value so parallel test runs stay stable. | ||
| struct EnvGuard(Option<String>); | ||
| impl Drop for EnvGuard { | ||
| fn drop(&mut self) { | ||
| match &self.0 { | ||
| Some(v) => unsafe { std::env::set_var("GOPROXY", v) }, | ||
| None => unsafe { std::env::remove_var("GOPROXY") }, | ||
| } | ||
| } | ||
| } | ||
| let _g = EnvGuard(std::env::var("GOPROXY").ok()); | ||
| unsafe { std::env::set_var("GOPROXY", "") }; | ||
| let proxies = parse_goproxy(); | ||
| assert_eq!(proxies.len(), 1); | ||
| assert_eq!(proxies[0].url, "https://proxy.golang.org"); | ||
| } |
There was a problem hiding this comment.
With the refactoring of parse_goproxy to use a helper function, this test can be simplified to avoid environment manipulation and unsafe blocks, making it thread-safe and more robust.
fn parse_goproxy_empty_uses_default() {
let proxies = parse_goproxy_from_val(Some(""));
assert_eq!(proxies.len(), 1);
assert_eq!(proxies[0].url, "https://proxy.golang.org");
}
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.18 x -- echo |
22.7 ± 0.6 | 21.5 | 24.8 | 1.00 |
mise x -- echo |
23.3 ± 0.6 | 22.1 | 30.0 | 1.03 ± 0.04 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.18 env |
22.0 ± 0.6 | 21.0 | 27.8 | 1.00 |
mise env |
22.4 ± 0.6 | 21.1 | 24.1 | 1.02 ± 0.04 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.18 hook-env |
22.8 ± 0.7 | 21.7 | 29.4 | 1.00 |
mise hook-env |
23.5 ± 0.6 | 22.2 | 25.0 | 1.03 ± 0.04 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.4.18 ls |
19.9 ± 0.5 | 18.8 | 21.7 | 1.00 |
mise ls |
20.6 ± 0.5 | 19.5 | 22.0 | 1.03 ± 0.04 |
xtasks/test/perf
| Command | mise-2026.4.18 | mise | Variance |
|---|---|---|---|
| install (cached) | 145ms | -13% | |
| ls (cached) | 76ms | 79ms | -3% |
| bin-paths (cached) | 81ms | 83ms | -2% |
| task-ls (cached) | 847ms | 803ms | +5% |
Summary
Fixes the
test_go_install_slowfailure on the 2026.4.19 release PR wheremise x go:github.com/go-kratos/kratos/cmd/kratos/v2@latest -- bash -c 'kratos --help 2>&1'exits 127 (kratos: command not found).Root Cause
parse_goproxy()in src/backend/go.rs usedenv::var(\"GOPROXY\").unwrap_or_else(...).unwrap_or_elseonly fires when the variable is unset; it does not fire when the variable is set to the empty string.The e2e harness forwards
GOPROXY=\"\${GOPROXY:-}\"in e2e/run_test:84, which evaluates to an empty string inside the docker container. That empty string feeds intoparse_goproxy_value(\"\"), which returns no proxies, so:fetch_proxy_versionsreturnsNone.go list -m -versionsfallback returns an emptyVersionslist for submodules without tagged releases (e.g.github.com/go-kratos/kratos/cmd/kratos/v2)._list_remote_versionsreturns[]and mise printsWARN No versions found for go:….This bug was latent. Before #9276,
resolve_versionfell through toresolve_prefix(\"latest\"), which resolved to the literal string\"latest\"— andgo install …@latesthandled it natively. After #9276, resolution errors withno versions foundandmise xnever installs the tool, sokratosis not on PATH →bash -c 'kratos --help'exits 127.Go's own tooling treats
GOPROXY=identically to unset:```
$ env GOPROXY= go env GOPROXY
https://proxy.golang.org,direct
```
Fix
Treat both unset and empty
GOPROXYas the default.```rust
let goproxy = std::env::var("GOPROXY")
.ok()
.filter(|s| !s.is_empty())
.unwrap_or_else(|| DEFAULT_GOPROXY.to_string());
```
Added unit test
parse_goproxy_empty_uses_default.Validation
Failing run this fixes: https://github.com/jdx/mise/actions/runs/24803479152/job/72592666111
This PR was generated by an AI coding assistant.
Note
Low Risk
Low risk: small, localized change to Go backend proxy parsing plus a unit test; behavior only changes when
GOPROXYis set to an empty string.Overview
Fixes Go backend proxy resolution to treat
GOPROXY=(empty) the same as an unsetGOPROXY, defaulting tohttps://proxy.golang.org,directto matchgo envbehavior.Adds a unit test (
parse_goproxy_empty_uses_default) that temporarily setsGOPROXYto empty and asserts the default proxy is used.Reviewed by Cursor Bugbot for commit 61d7c9f. Bugbot is set up for automated code reviews on this repo. Configure here.