Skip to content

fix(nix): parse Nix versions that omit the patch component - #2924

Merged
mikeland73 merged 1 commit into
mainfrom
claude/focused-goldberg-3jrvvr
Sep 15, 2026
Merged

mikeland73 merged 1 commit into
mainfrom
claude/focused-goldberg-3jrvvr

Conversation

@mikeland73

Copy link
Copy Markdown
Collaborator

Summary

Fixes #2766.

Some Nix builds report a two-component version followed directly by a prerelease, e.g. nix (Nix) 2.33pre20251107_479b6b73 (note: 2.33pre…, not 2.33.0pre…). versionRegexp required a full major.minor.patch triple, so it failed to match these strings. Info.Version was then left empty and Devbox aborted every command with a confusing error where the version renders as empty:

Error: Devbox requires nix of version >= 2.12.0. Your version is . Please upgrade nix and try again.

Fix

  • Make the patch component optional in versionRegexp.
  • Normalize the parsed version by inserting a .0 patch when Nix omits it, so 2.33pre20251107_479b6b73 becomes 2.33.0pre20251107_479b6b73 (and a bare 2.33 becomes 2.33.0).

The normalization is necessary because a patch component is required for the version to be comparable as a semver — both directly and via AtLeast's prerelease coercion. Without it, AtLeast would coerce the version to 2.33-pre.20251107+479b6b73, which is not a valid semver (a prerelease requires a patch), so AtLeast(2.12.0) would still return false and the version would keep being rejected. Inserting the .0 makes the coerced form 2.33.0-pre.20251107+479b6b73, a valid semver that compares correctly.

All previously-supported version formats (2.21.2, 2.23.0pre20240526_7de033d6, 2.90.0-beta.1, …) are unchanged.

How was it tested?

  • Added cases to TestParseVersionInfoShort asserting 2.33pre20251107_479b6b732.33.0pre20251107_479b6b73 and 2.332.33.0.
  • Extended TestVersionInfoAtLeast so a patch-less prerelease (2.33.0pre20251107_479b6b73) is treated as >= 2.12.0 and >= MinVersion, and < 2.34.0.
  • go test ./nix/ passes; go vet ./nix/, go build ./..., and gofmt -l are clean.

cc @Electrenator (issue reporter) — thanks for the detailed debug logs that pinpointed this.

Community Contribution License

All community contributions in this pull request are licensed to the project
maintainers under the terms of the
Apache 2 License.

By creating this pull request, I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 License as stated in
the
Community Contribution License.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VeatwKxvuw66rgQUnekx7J


Generated by Claude Code

Nix builds can report a two-component version followed directly by a
prerelease, e.g. "nix (Nix) 2.33pre20251107_479b6b73". versionRegexp
required a major.minor.patch triple, so it failed to match these strings.
Info.Version was left empty and devbox aborted with:

    Error: Devbox requires nix of version >= 2.12.0. Your version is .

Make the patch component optional and normalize the parsed version by
inserting a ".0" patch when Nix omits it ("2.33pre..." becomes
"2.33.0pre..."). The patch is required for the version to be comparable
as a semver, both directly and via AtLeast's prerelease coercion (a
prerelease without a patch is not a valid semver, so AtLeast would
otherwise still reject the version).

Fixes #2766

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VeatwKxvuw66rgQUnekx7J
Copilot AI review requested due to automatic review settings July 16, 2026 14:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes Devbox failing to detect certain Nix versions whose version string omits the patch component (e.g. 2.33pre... instead of 2.33.0pre...), which previously caused Info.Version to be empty and led to confusing “Your version is .” errors.

Changes:

  • Updated the versionRegexp parser to allow an optional patch component in the Nix version.
  • Added normalizeVersion to insert a .0 patch when Nix omits it, ensuring versions remain semver-comparable (including prerelease coercion in AtLeast).
  • Extended unit tests to cover patch-less versions and patch-less prerelease normalization behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
nix/nix.go Makes patch optional in version parsing and normalizes missing patch to .0 so version comparison works correctly.
nix/nix_test.go Adds/extends test cases to validate parsing and AtLeast behavior for patch-less versions and prereleases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mikeland73

Copy link
Copy Markdown
Collaborator Author

@claude please review this PR for correctness, and flag anything that would block merging.

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @mikeland73's task in 1m 39s —— View job


Review: fix(nix): parse Nix versions that omit the patch component

  • Gather context (diff, related files)
  • Review nix/nix.go changes for correctness
  • Review nix/nix_test.go changes for coverage
  • Check for edge cases / blocking issues
  • Post final review

No blocking issues found. This is a clean, well-scoped fix.

Correctness

  • versionRegexp (nix/nix.go:180): making the patch group (?:\.(?P<patch>0|[1-9]\d*))? optional is a minimal, correct change — group numbering/matches[2] (the full version substring) is unaffected, so the existing len(matches) < 3 check and info.Name = matches[1] logic still hold.
  • normalizeVersion (nix/nix.go:291-301): since major/minor are captured verbatim from the start of matches[2], majorMinor is always a true prefix of version, so strings.TrimPrefix can never silently no-op on a mismatch — there's no ambiguity here even for multi-digit major/minor values.
  • Traced through both new cases by hand:
    • 2.33pre20251107_479b6b73 → patch empty → "2.33" + ".0" + "pre20251107_479b6b73" = 2.33.0pre20251107_479b6b73 ✓.
    • 2.33 → patch empty → "2.33" + ".0" + "" = 2.33.0 ✓.
  • The AtLeast reasoning in the PR description is correct: preReleaseRegexp requires a semver-valid base to coerce to -pre.$date+$commit, and without the inserted patch, 2.33-pre.20251107+479b6b73 is not a valid semver (per semver.IsValid), so AtLeast would keep returning false. Inserting .0 fixes this.
  • Checked all other call sites of Info.Version/nix.Version() (internal/nix/install.go, internal/telemetry/{telemetry,segment}.go) — they just display or log the string, so normalizing 2.33pre...2.33.0pre... is a harmless, arguably more-correct cosmetic change, not a behavioral regression.
  • All previously-supported formats (2.21.2, 2.23.0pre..., 2.90.0-beta.1) are unaffected since patch is non-empty for them and normalizeVersion returns early.

Tests

  • TestParseVersionInfoShort and TestVersionInfoAtLeast additions directly cover the new normalization and the AtLeast comparison against Version2_12, MinVersion, and an upper bound (2.34.0) — good coverage of the reported bug and the fix's correctness claim.

Note

I was not able to execute go test/go vet/go build in this sandboxed environment (commands required approval that wasn't available), so I verified correctness by manual trace/regex analysis instead of running the suite. The PR description states go test ./nix/, go vet ./nix/, go build ./..., and gofmt -l were run locally and pass — if you'd like me to actually execute them here, the --allowedTools for this job would need to permit go test/go vet/go build.

Minor (non-blocking) observations

  • normalizeVersion calls versionRegexp.SubexpIndex("patch") (and again for major/minor) on every parse; this is a linear name lookup but happens once per nix --version invocation (memoized via sync.Once), so it's not a real perf concern.
  • Nothing else stood out — the doc comments are accurate and the fix is narrowly targeted at the reported issue (Devbox unable to detect version #2766).

Copy link
Copy Markdown
Collaborator Author

Review: correct, and I don't see a correctness blocker

I re-reviewed the change (nix/nix.go, nix/nix_test.go) against the PR head. Summary of what I verified:

Correctness — the fix is complete, not just the regex half. The bug has two layers, and both are handled:

  1. versionRegexp required a major.minor.patch triple, so 2.33pre20251107_479b6b73 didn't match → Info.Version empty → the gate in internal/nix/install.go (nix.AtLeast(MinVersion), MinVersion = 2.18.0) rejected it. Making patch optional fixes matching.
  2. But an extracted 2.33pre… still wouldn't compare: AtLeast coerces prereleases to 2.33-pre.20251107+479b6b73, which is not a valid semver (a prerelease requires a patch), so semver.Compare would return -1 and the version would keep being rejected. normalizeVersion inserting .0 (→ 2.33.0pre… → coerced 2.33.0-pre.…, valid) is what actually makes the gate pass. I confirmed this against MinVersion (2.18.0), not just the 2.12.0 in the error string.

Edge cases checked:

  • normalizeVersion's TrimPrefix(version, major.minor) is safe: it only runs when the patch group is empty (so there's no numeric .patch after the minor to confuse it), and capture group 2 always begins with exactly major.minor.
  • Bare 2.332.33.0; all previously-supported formats (2.21.2, 2.23.0pre…, 2.90.0-beta.1) are unchanged. Tests cover these plus the two new patch-less cases.

Local verification: go test ./nix/ passes, go vet ./nix/ and go build ./... clean, gofmt -l empty.

Non-blocking notes:

  • The branch is behind main (which has advanced since this was opened, including "ci: test against Nix 2.35.2"), but main hasn't touched nix/nix.go/nix/nix_test.go, so there's no merge conflict. A rebase is optional — happy to rebase if you'd prefer the branch current before merge.
  • This PR is currently in draft. I've left it as-is — I won't mark it ready for review unless you ask me to.

Net: I'd consider this mergeable on correctness grounds. The only merge gate is the draft status (a maintainer decision).


Generated by Claude Code

@mikeland73 mikeland73 added the ready for human review Automated review passed; needs a maintainer to review and merge label Sep 14, 2026
@mikeland73
mikeland73 marked this pull request as ready for review September 14, 2026 16:18
@mikeland73
mikeland73 merged commit 38dec4b into main Sep 15, 2026
28 checks passed
@mikeland73
mikeland73 deleted the claude/focused-goldberg-3jrvvr branch September 15, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for human review Automated review passed; needs a maintainer to review and merge

Development

Successfully merging this pull request may close these issues.

Devbox unable to detect version

3 participants