localenv: fall back to installed Python - #6457
Conversation
83ef594 to
3191670
Compare
Approval status: pending
|
|
Reviewed the full diff. Two of these I'd treat as blocking; the rest are ordinary review comments. 🔴 Blocking1.
|
| request | result |
|---|---|
cpython@>=3.12,==3.12.* |
/usr/bin/python3.12 ✅ |
cpython@>=3.12,<3.13,==3.12.* |
/usr/bin/python3.12 ✅ |
cpython@3.12,==3.12.* |
[], exit 0 ❌ |
cpython@==3.12,==3.12.* |
[], exit 0 ❌ |
cpython@===3.11,==3.11.* |
[], exit 0 ❌ |
cpython@==3.12.* (control) |
/usr/bin/python3.12 ✅ |
All three failing forms are ones TestPythonMinorFromRequires explicitly accepts (3.12, ===3.11, !=3.11,3.12, 3.12,!=3.12.4).
Suggested fix: drop constraint and request "==" + minor + ".*" on its own. It isn't buying anything — pyMinor is already the floor and validate only asserts the minor. It also removes a second problem for free: constraint is the untrimmed raw TOML value (parseConstraints trims only for its emptiness check), so requires-python = " >=3.12 " forwards cpython@ >=3.12 ,==3.12.* into argv.
2. Missing .nextchanges/ fragment
Per .agents/rules/changelog.md. This changes provisioning behavior, adds a line to text output, and adds a pythonResolution key to the --output json contract — user-visible on all three counts. #6445 added .nextchanges/cli/setup-local-multiline-toml.md for a smaller change.
Repo rules — worth settling before merge
3. uv.go:32 — runFn reimplements process.WithStub, and it short-circuits the path under test. process.WithStub(ctx) (libs/process/stub.go:16) already hooks runCmd inside process.Background with WithStdoutFor/WithFailureFor/WithCallback; it's used in cmd/labs/project/installer_test.go, bundle/config/mutator/python/python_mutator_test.go, and libs/aitools/installer/plugin_test.go. Beyond the duplication (CLAUDE.md: search for an existing helper first), runUvOutput returns at 151-153 before resolveIndexURL, so the argv-to-subprocess wiring for the new uv python list calls and the UV_INDEX_URL bridge have zero coverage — and that index-URL path is the whole restricted-network motivation for the feature.
4. No doc comments on any new symbol. selectInstalledPython (51), newerInstalledPython (81), runUvOutput (150), listInstalledPython (184), and the types uvRunFn (35), uvPython (37), installedPython (46) — in a file where runUv, venvPython, lineWithPrefix, pipConfPaths, and redactURLCredentials each carry several sentences. Relatedly, pythonListArgs encodes uv-specific grammar (--only-installed, --managed-python, cpython@<specifiers>) with no source link, while the same file links docs.astral.sh three times elsewhere — and finding 1 is exactly the assumption a link would have let a reader check.
5. uv.go:193 — deleted rationale in the Provision doc comment. The removed text named the concrete repro and tied it to the validate phase: "uv sync selects the newest installed interpreter satisfying requires-python (e.g. 3.13 for a >=3.12 floor), which then fails validation against the 3.12 target." The replacement ("can select a newer interpreter than the target") drops both, so the next reader can't tell the guard is load-bearing for pipeline.validate. Also syncArgs (311-315) still says "pinning the interpreter to pyMinor via --python" after the parameter was renamed and widened to accept a path.
Correctness / behavior
6. uv.go:93 — the managed-vs-system tie-break is dead code. selectInstalledPython always walks the managed group first, so candidate.managed && !current.managed is true && !true in the managed group and false && … in the system group. It always returns false. TestSelectInstalledPython/"managed wins an equal-version tie" passes purely on iteration order, so the documented preference is unenforced and will vanish silently if anyone reorders the two groups or merges them into one uv call.
7. uv.go:81 — patch is compared before managed, inverting uv's own default. uv defaults to python-preference = "managed": prefer uv-managed installs over system ones even when the system one is newer. Here a uv-managed 3.12.10 loses to a distro 3.12.11, so the venv gets built on a distro-patched interpreter uv would never have picked — while a self-contained managed one sat right there. Given the premise is "the managed download path is broken on this network", an already-downloaded managed interpreter is the more reliable pick. Suggest preferring managed unconditionally and using patch only to break ties within a group. (Happy to be argued out of this one.)
8. uv.go:67 — one empty path aborts the whole selection. The return "", errors.New(...) guard sits inside the scan loop, after the managed group has been walked, so a single entry with a null/empty path discards a viable candidate best is already holding. uv types these fields as nullable in its real JSON ("symlink":null, "url":null). continue is the right action here.
9. uv.go:163 — a cancelled context still shells out twice. Ctrl-C during a slow download makes installErr a signal failure; the code then runs uv python list twice against the dead ctx purely to fail, and the recorded error and phase Detail carry two spurious "list failed" causes for what the user experienced as a clean interrupt. An early if ctx.Err() != nil return keeps the diagnostic clean.
10. pipeline.go:504 — selection.Executable is forwarded unchecked. The third arg used to be pyMinor, validated upstream. A PythonSelection{} with a nil error now yields uv sync --python "", surfacing as an opaque E_PROVISION. The parameter is also now stringly overloaded ("either a minor request or an absolute path") while the caller already holds selection.Resolution and could disambiguate.
Diagnosability of the fallback path
This is the cluster I'd most like to see addressed together — for a feature whose entire reason to exist is "the network broke the download", none of the three is recoverable from a --debug transcript today.
11. uv.go:171 — the list command's stderr is structurally unreachable. uvFailure uses errors.AsType[*process.ProcessError], which walks the errors.Join(installErr, err) tree depth-first and so always matches installErr first; the list error is found second and never consulted. ProcessError.Error() is only Command: Err — it omits Stderr. So error: unexpected argument '--managed-python' found, or an EACCES on the uv binary, appears nowhere in the output or in --output json.
12. uv.go:181 — the chosen interpreter is never logged. The file logs uv: discovered binary at %s and uv: not found; running installer: %s for far less consequential events, but the fact that decides whether the resulting venv is trustworthy goes unrecorded. One log.Debugf before the return matches the existing convention.
13. cmd/environments/output.go:114 — the fallback is surfaced only on success. renderResult returns at the pipelineErr != nil branch long before renderSuccess, so a user whose uv sync then fails on a distro python (missing headers, patched ssl) sees only "✗ Setup failed while provisioning" with no hint that Python came from the system. pipeline.go:500-503 deliberately sets PythonResolution before Provision for exactly this; text output just doesn't read it.
Questions / discretionary
14. uv.go:325 — no minimum-uv-version check. --managed-python/--no-managed-python replaced --python-preference only-managed/only-system in uv 0.5.x–0.6, and --output-format json plus the positional REQUEST are similarly recent. On an older uv both list calls fail and the feature is silently inert (made worse by finding 11 hiding the reason). How much this matters depends on whether EnsureAvailable always lands a recent uv — if it does, feel free to wave this off. EnsureAvailable already captures the version string at uv.go:130-134 if a gate is wanted.
15. uv.go:177 — the comparator may be re-deriving what uv already gives you. uv python list emits entries best-first (verified: 3.13.7, 3.12.11, 3.11.13, 3.10.18 descending), so --all-versions plus newerInstalledPython, the installedPython wrapper type, the anonymous []struct{pythons; managed}, and the best pointer collapse to "first element of managed, else first of system". Separately, the same three-line return PythonSelection{}, uvFailure(ErrPythonInstall, errors.Join(installErr, err), …) is copy-pasted verbatim at 171, 175, and 179 — one named wrapper would also be the single place to fix finding 11.
16. On testing pythonResolution: it's emitted as "uv_install_succeeded" on every real run and omitted on every dry run — and all 10 acceptance goldens are --dry-run, which is why none needed regeneration. TestResultOmitsUnknownPythonResolution pins only the bare-NewResult case, so the value consumers will actually branch on is unpinned, and there's no PythonResolutionUnspecified constant giving them a documented zero. Minor: the comment at pkgmanager.go:37-39 justifies the field against telemetry, but buildSetupLocalEvent never reads it.
anton-107
left a comment
There was a problem hiding this comment.
Thanks for the detailed review. The following findings need to be addressed before this can merge:
- #1 —
uv.go:167, constraint concatenation silently disabling fallback. Real bug: the accepted bare/==/===requires-pythonforms produce a request uv resolves to[](exit 0), so a valid installed interpreter is missed. One caveat on the remedy — dropping the constraint and requesting onlycpython@==<minor>.*is lossy: the validate phase comparesmajor.minoronly, so it will not catch an excluded/capped patch, anduv sync --python <path>would then reject it. Prefer normalizingrequires-pythoninto a valid specifier (trim whitespace + rewrite the bare/exact forms) and keep the==<minor>.*pin. - #2 — missing
.nextchanges/fragment. Required by.agents/rules/changelog.mdfor this user-visible behavior + JSON-contract change. - #3 —
runFnreimplementsprocess.WithStub. Switch to the existing seam; it also gives coverage to the newuv python listargv wiring and theUV_INDEX_URLbridge, which are currently untested (and that bridge is the whole restricted-network motivation). - #6 — dead managed-vs-system tie-break. Because the managed group is always iterated first, the
candidate.managed && !current.managedbranch never fires. Remove the always-false expression and document that managed-first ordering provides the preference, or make the preference actually reachable. - #8 — one empty
pathaborts the entire selection.continuepast unusable entries and fail only if none remain, or keep the strict check with a one-line comment documenting the--only-installedinvariant.
Requesting changes on these.
a1287f7 to
51443bf
Compare
51443bf to
8dbb696
Compare
Changes
uv python installas the first attemptWhy
Python downloads can fail on restricted networks even when a compatible interpreter is already installed. Reusing that interpreter lets setup complete without retrying the failed download.
If discovery or provisioning still fails, the structured result preserves the fallback state so clients can offer manual interpreter selection.
Tests
GOTOOLCHAIN=go1.26.5 GOPROXY=direct go test ./libs/localenv/...