From 22f99d96f349ee55ee22a1ef5ea9513b9f65cac6 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 15:17:20 +0200 Subject: [PATCH 01/15] Add local-env foundation: result types and env-key mapping First of a stacked series adding `databricks local-env python sync`, which provisions a local Python environment matched to a Databricks compute target. The feature lands across small, single-concern PRs; each layer is independently reviewable and adds no user-facing surface until the final PR wires the command in. This PR is the foundation the rest of the stack builds on: - result.go: the result types and the --json / E_* error contract that every phase reports through (Result, PipelineError, ErrorCode, PhaseName, PhaseStatus, Mode, TargetInfo, ResolvedInfo, Plan, Warning), plus the command-path constants (local-env / python / sync) defined once. - envkey.go: mapping a compute target to an environment key and parsing the Python minor from a requires-python specifier. Nothing imports this package yet, so the CLI is unchanged. The unexported filesystem/artifact constants and the canonical phase-order slice live with the pipeline that consumes them (a later PR in the stack). Co-authored-by: Isaac --- libs/localenv/envkey.go | 34 +++++++ libs/localenv/envkey_test.go | 34 +++++++ libs/localenv/result.go | 180 +++++++++++++++++++++++++++++++++++ libs/localenv/result_test.go | 27 ++++++ 4 files changed, 275 insertions(+) create mode 100644 libs/localenv/envkey.go create mode 100644 libs/localenv/envkey_test.go create mode 100644 libs/localenv/result.go create mode 100644 libs/localenv/result_test.go diff --git a/libs/localenv/envkey.go b/libs/localenv/envkey.go new file mode 100644 index 00000000000..1e9171b35b3 --- /dev/null +++ b/libs/localenv/envkey.go @@ -0,0 +1,34 @@ +package localenv + +import ( + "fmt" + "regexp" + "strings" +) + +var pythonVersionRe = regexp.MustCompile(`(\d+)\.(\d+)`) + +// NormalizeServerless returns the canonical "vN" spelling of a serverless +// version accepting "4", "v4", or "V4". +func NormalizeServerless(version string) string { + return "v" + strings.TrimPrefix(strings.ToLower(version), "v") +} + +// EnvKeyForServerless returns the environment key for a serverless version. +func EnvKeyForServerless(version string) string { + return "serverless/serverless-" + NormalizeServerless(version) +} + +// EnvKeyForSparkVersion returns the environment key for a Spark version. +func EnvKeyForSparkVersion(sparkVersion string) string { + return "dbr/" + sparkVersion +} + +// PythonMinorFromRequires parses a PEP 440 requires-python string and extracts MAJOR.MINOR. +func PythonMinorFromRequires(requiresPython string) (string, error) { + match := pythonVersionRe.FindStringSubmatch(requiresPython) + if match == nil { + return "", fmt.Errorf("cannot parse python version from %q", requiresPython) + } + return fmt.Sprintf("%s.%s", match[1], match[2]), nil +} diff --git a/libs/localenv/envkey_test.go b/libs/localenv/envkey_test.go new file mode 100644 index 00000000000..cb276af4ea2 --- /dev/null +++ b/libs/localenv/envkey_test.go @@ -0,0 +1,34 @@ +package localenv + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEnvKeyForServerless(t *testing.T) { + for _, in := range []string{"4", "v4", "V4"} { + assert.Equal(t, "serverless/serverless-v4", EnvKeyForServerless(in)) + } +} + +func TestEnvKeyForSparkVersion(t *testing.T) { + assert.Equal(t, "dbr/15.4.x-scala2.12", EnvKeyForSparkVersion("15.4.x-scala2.12")) +} + +func TestPythonMinorFromRequires(t *testing.T) { + cases := map[string]string{ + "==3.12.*": "3.12", + ">=3.12": "3.12", + "==3.12.3": "3.12", + "~=3.11": "3.11", + } + for in, want := range cases { + got, err := PythonMinorFromRequires(in) + require.NoError(t, err) + assert.Equal(t, want, got) + } + _, err := PythonMinorFromRequires("garbage") + assert.Error(t, err) +} diff --git a/libs/localenv/result.go b/libs/localenv/result.go new file mode 100644 index 00000000000..82faed1b1dd --- /dev/null +++ b/libs/localenv/result.go @@ -0,0 +1,180 @@ +package localenv + +import "fmt" + +// Command path components, defined once so a rename touches a single place +// (spec §0 / invariant 8 / scenario 21). The cmd layer builds the Cobra +// command tree from CommandGroup/CommandSubgroup/CommandVerb; the --json +// "command" field uses CommandName. No other string re-spells the command path. +const ( + CommandGroup = "local-env" + CommandSubgroup = "python" + CommandVerb = "sync" + CommandName = CommandGroup + " " + CommandSubgroup + " " + CommandVerb + + // SchemaVersion is the version of the --json output contract (spec §6). + // Bump it on any breaking change to the JSON shape. + SchemaVersion = 1 +) + +// Mode is the provisioning mode: a full environment (default) or the +// constraints-only variant that omits the databricks-connect dependency. +type Mode int + +const ( + ModeDefault Mode = iota + ModeConstraintsOnly +) + +// String returns the JSON/text spelling of the mode ("default" | "constraints-only"). +func (m Mode) String() string { + if m == ModeConstraintsOnly { + return "constraints-only" + } + return "default" +} + +// PhaseName is a canonical execution phase (spec §3 / §6). The set is fixed and +// ordered; the --json "phases" array reports every phase in this order. +type PhaseName string + +const ( + PhasePreflight PhaseName = "preflight" + PhaseResolve PhaseName = "resolve" + PhaseFetch PhaseName = "fetch" + PhaseMerge PhaseName = "merge" + PhaseProvision PhaseName = "provision" + PhaseValidate PhaseName = "validate" +) + +// Phase status values (spec §6.2). +const ( + StatusOK = "ok" + StatusError = "error" + StatusPending = "pending" +) + +// ErrorCode is a stable failure-class identifier surfaced in --json error.code +// (spec §7). Values are compared via the ErrorCode constants, never by +// string-matching messages, and are defined once here. +type ErrorCode string + +const ( + ErrNoTarget ErrorCode = "E_NO_TARGET" + ErrManagerUnsupported ErrorCode = "E_MANAGER_UNSUPPORTED" + ErrUvMissing ErrorCode = "E_UV_MISSING" + ErrNotWritable ErrorCode = "E_NOT_WRITABLE" + ErrResolve ErrorCode = "E_RESOLVE" + ErrEnvUnsupported ErrorCode = "E_ENV_UNSUPPORTED" + ErrFetch ErrorCode = "E_FETCH" + ErrWrite ErrorCode = "E_WRITE" + ErrMerge ErrorCode = "E_MERGE" + ErrPythonInstall ErrorCode = "E_PYTHON_INSTALL" + ErrProvision ErrorCode = "E_PROVISION" + ErrValidate ErrorCode = "E_VALIDATE" +) + +// PipelineError is a failure carrying a stable code, the phase at which it +// occurred, and whether disk was mutated before the failure. It marshals to the +// --json error object (spec §6.2). Code and FailurePhase are the stable +// contract; Err holds the wrapped cause for errors.Is/As and is not serialized. +type PipelineError struct { + Code ErrorCode `json:"code"` + FailurePhase PhaseName `json:"failurePhase"` + Msg string `json:"message"` + DiskMutated bool `json:"diskMutated"` + Err error `json:"-"` +} + +func (e *PipelineError) Error() string { + if e.Err != nil { + return e.Msg + ": " + e.Err.Error() + } + return e.Msg +} + +func (e *PipelineError) Unwrap() error { + return e.Err +} + +// NewError creates a PipelineError with a code and message. FailurePhase and +// DiskMutated are filled in by the pipeline when it records the failure. The +// message is formatted with fmt.Sprintf(format, args...); err may be nil. +func NewError(code ErrorCode, err error, format string, args ...any) *PipelineError { + return &PipelineError{ + Code: code, + Msg: fmt.Sprintf(format, args...), + Err: err, + } +} + +// TargetInfo is the resolved compute target (spec §6 "target"). Source records +// which of the four precedence sources was used. SparkVersion is the raw cluster +// runtime string the resolver read; it is folded into EnvKey (dbr/) +// and is not part of the JSON contract, kept only as intermediate resolver state. +type TargetInfo struct { + Source string `json:"source"` + ClusterID string `json:"clusterId,omitempty"` + ServerlessVersion string `json:"serverlessVersion,omitempty"` + EnvKey string `json:"envKey"` + + SparkVersion string `json:"-"` +} + +// ResolvedInfo is the resolved environment definition (spec §6 "resolved"). +// DBConnectVersion is omitted in constraints-only mode. +type ResolvedInfo struct { + PythonVersion string `json:"pythonVersion"` + DBConnectVersion string `json:"dbconnectVersion,omitempty"` + ArtifactSource string `json:"artifactSource"` +} + +// Plan describes the changes a --check run would apply (spec §6.3). +// ChangedRegions is retained for text output only and is not serialized. +type Plan struct { + WouldWrite string `json:"wouldWrite"` + WouldBackup string `json:"wouldBackup,omitempty"` + WouldInstallPython string `json:"wouldInstallPython,omitempty"` + Diff string `json:"diff"` + + ChangedRegions []string `json:"-"` +} + +// PhaseStatus is one entry in the --json "phases" array (spec §6). Detail is +// used for human-readable text output only and is not serialized. +type PhaseStatus struct { + Phase PhaseName `json:"phase"` + Status string `json:"status"` + + Detail string `json:"-"` +} + +// Warning is a non-fatal advisory surfaced in --json "warnings" (spec §6). +type Warning struct { + Code string `json:"code"` + Message string `json:"message"` +} + +// Result is the full outcome of a sync run and the root of the --json object +// (spec §6). Field order matches the spec's schema so JSON key order is stable. +type Result struct { + SchemaVersion int `json:"schemaVersion"` + Command string `json:"command"` + OK bool `json:"ok"` + Mode string `json:"mode"` + DryRun bool `json:"dryRun"` + Target *TargetInfo `json:"target,omitempty"` + Resolved *ResolvedInfo `json:"resolved,omitempty"` + Greenfield bool `json:"greenfield"` + Plan *Plan `json:"plan,omitempty"` + VenvPath string `json:"venvPath,omitempty"` + Phases []PhaseStatus `json:"phases"` + Warnings []Warning `json:"warnings"` + Error *PipelineError `json:"error"` + BackupPath string `json:"backupPath,omitempty"` + // DurationMs is part of the §6 contract but reserved for now: the pipeline + // does not measure wall time (a real clock would make acceptance goldens + // non-deterministic), so it is always emitted as 0 until timing is wired + // through a clock the tests can control. + DurationMs int64 `json:"durationMs"` +} diff --git a/libs/localenv/result_test.go b/libs/localenv/result_test.go new file mode 100644 index 00000000000..76d30ec22bf --- /dev/null +++ b/libs/localenv/result_test.go @@ -0,0 +1,27 @@ +package localenv + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPipelineErrorWrapsAndExposesCode(t *testing.T) { + base := errors.New("boom") + err := NewError(ErrFetch, base, "fetch %s", "x") + assert.Equal(t, "fetch x: boom", err.Error()) + assert.Equal(t, ErrFetch, err.Code) + assert.ErrorIs(t, err, base) +} + +func TestModeString(t *testing.T) { + assert.Equal(t, "default", ModeDefault.String()) + assert.Equal(t, "constraints-only", ModeConstraintsOnly.String()) +} + +func TestCommandName(t *testing.T) { + // The --json "command" field and all help text derive from these; the + // three-part path must join to the full command a user types. + assert.Equal(t, "local-env python sync", CommandName) +} From 22c2902b0351285a8285fb5a31a1e660c2139ca0 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 17:25:43 +0200 Subject: [PATCH 02/15] Fix PythonMinorFromRequires to pick the lower bound in multi-clause specifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the foundation layer flagged that PythonMinorFromRequires took the first MAJOR.MINOR in the string via first-match regex. For a multi-clause requires-python where the exclusive upper bound comes first — e.g. "<3.13,>=3.10" — it returned 3.13, the version the "<3.13" clause forbids, because PEP 440 clause order is arbitrary. The result feeds PM.EnsurePython, so the tool could target a Python the constraint excludes. Prefer a lower-bound / pinning clause (>=, >, ==, ~=, ===) and only fall back to the first version when none is present. Adds multi-clause test coverage; the prior tests exercised only single-bound specifiers. Co-authored-by: Isaac --- libs/localenv/envkey.go | 20 +++++++++++++++++++- libs/localenv/envkey_test.go | 10 +++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/libs/localenv/envkey.go b/libs/localenv/envkey.go index 1e9171b35b3..a7c0612f69c 100644 --- a/libs/localenv/envkey.go +++ b/libs/localenv/envkey.go @@ -8,6 +8,13 @@ import ( var pythonVersionRe = regexp.MustCompile(`(\d+)\.(\d+)`) +// lowerBoundClauseRe matches a single requires-python clause that establishes +// the Python floor to install: a lower-bound or pinning operator (>=, >, ==, +// ~=, ===) followed by a MAJOR.MINOR version. Upper-bound (<, <=) and exclusion +// (!=) clauses are deliberately not matched — they must not be chosen as the +// version to install. +var lowerBoundClauseRe = regexp.MustCompile(`(?:>=|===|==|~=|>)\s*(\d+)\.(\d+)`) + // NormalizeServerless returns the canonical "vN" spelling of a serverless // version accepting "4", "v4", or "V4". func NormalizeServerless(version string) string { @@ -24,8 +31,19 @@ func EnvKeyForSparkVersion(sparkVersion string) string { return "dbr/" + sparkVersion } -// PythonMinorFromRequires parses a PEP 440 requires-python string and extracts MAJOR.MINOR. +// PythonMinorFromRequires parses a PEP 440 requires-python string and extracts +// the MAJOR.MINOR of the Python version to install. +// +// A requires-python may hold several comma-separated clauses in any order +// (e.g. "<3.13,>=3.10"). The version to install is the lower bound, so a +// lower-bound / pinning clause (>=, >, ==, ~=, ===) is preferred; taking the +// first number in the string would pick an upper bound like "<3.13" — a +// version the specifier forbids. Only when no lower-bound clause is present do +// we fall back to the first MAJOR.MINOR found. func PythonMinorFromRequires(requiresPython string) (string, error) { + if m := lowerBoundClauseRe.FindStringSubmatch(requiresPython); m != nil { + return fmt.Sprintf("%s.%s", m[1], m[2]), nil + } match := pythonVersionRe.FindStringSubmatch(requiresPython) if match == nil { return "", fmt.Errorf("cannot parse python version from %q", requiresPython) diff --git a/libs/localenv/envkey_test.go b/libs/localenv/envkey_test.go index cb276af4ea2..ea8b7c6a737 100644 --- a/libs/localenv/envkey_test.go +++ b/libs/localenv/envkey_test.go @@ -23,11 +23,19 @@ func TestPythonMinorFromRequires(t *testing.T) { ">=3.12": "3.12", "==3.12.3": "3.12", "~=3.11": "3.11", + // Multi-clause specifiers: the lower bound is the version to install, + // regardless of clause order. Taking the first number would pick the + // excluded upper bound (e.g. 3.13 from "<3.13"). + "<3.13,>=3.10": "3.10", + ">=3.10,<3.13": "3.10", + ">=3.10, <3.13": "3.10", + "<4.0,>=3.9": "3.9", + "===3.11": "3.11", } for in, want := range cases { got, err := PythonMinorFromRequires(in) require.NoError(t, err) - assert.Equal(t, want, got) + assert.Equal(t, want, got, "input %q", in) } _, err := PythonMinorFromRequires("garbage") assert.Error(t, err) From 5b0e0cce7073bbe9b4a039ffc5429a358a7c5387 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 20:13:35 +0200 Subject: [PATCH 03/15] Reject requires-python with no lower bound instead of picking a forbidden version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review of the foundation layer noted that when a requires-python has no lower-bound/pin clause at all (only upper-bound or exclusion, e.g. "<3.13,!=3.12"), PythonMinorFromRequires fell back to the first number and returned 3.13 — a version the specifier forbids. Such a spec has no floor to install from, so it now errors rather than guessing. A bare "3.12" (no operator) is still accepted as a valid floor. Co-authored-by: Isaac --- libs/localenv/envkey.go | 20 ++++++++++++++++---- libs/localenv/envkey_test.go | 14 ++++++++++++-- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/libs/localenv/envkey.go b/libs/localenv/envkey.go index a7c0612f69c..eee1022d26f 100644 --- a/libs/localenv/envkey.go +++ b/libs/localenv/envkey.go @@ -15,6 +15,12 @@ var pythonVersionRe = regexp.MustCompile(`(\d+)\.(\d+)`) // version to install. var lowerBoundClauseRe = regexp.MustCompile(`(?:>=|===|==|~=|>)\s*(\d+)\.(\d+)`) +// boundedClauseRe matches a MAJOR.MINOR that is governed by an upper-bound or +// exclusion operator (<, <=, !=). Such a version is forbidden or capped, never a +// version to install, so a spec whose only version is bounded this way has no +// usable floor. +var boundedClauseRe = regexp.MustCompile(`(?:<=|<|!=)\s*(\d+)\.(\d+)`) + // NormalizeServerless returns the canonical "vN" spelling of a serverless // version accepting "4", "v4", or "V4". func NormalizeServerless(version string) string { @@ -36,10 +42,11 @@ func EnvKeyForSparkVersion(sparkVersion string) string { // // A requires-python may hold several comma-separated clauses in any order // (e.g. "<3.13,>=3.10"). The version to install is the lower bound, so a -// lower-bound / pinning clause (>=, >, ==, ~=, ===) is preferred; taking the -// first number in the string would pick an upper bound like "<3.13" — a -// version the specifier forbids. Only when no lower-bound clause is present do -// we fall back to the first MAJOR.MINOR found. +// lower-bound / pinning clause (>=, >, ==, ~=, ===) is preferred. A bare +// MAJOR.MINOR with no operator (e.g. "3.12") is also accepted as the floor. +// But a spec whose only version is governed by an upper-bound/exclusion +// operator (e.g. "<3.13" or "!=3.12") has no usable floor: picking that number +// would select a version the specifier forbids, so we error instead of guessing. func PythonMinorFromRequires(requiresPython string) (string, error) { if m := lowerBoundClauseRe.FindStringSubmatch(requiresPython); m != nil { return fmt.Sprintf("%s.%s", m[1], m[2]), nil @@ -48,5 +55,10 @@ func PythonMinorFromRequires(requiresPython string) (string, error) { if match == nil { return "", fmt.Errorf("cannot parse python version from %q", requiresPython) } + // A version exists but not behind a lower-bound/pin operator. If it is behind + // an upper-bound/exclusion operator, there is no floor to install. + if boundedClauseRe.MatchString(requiresPython) { + return "", fmt.Errorf("requires-python %q has no lower bound to install from", requiresPython) + } return fmt.Sprintf("%s.%s", match[1], match[2]), nil } diff --git a/libs/localenv/envkey_test.go b/libs/localenv/envkey_test.go index ea8b7c6a737..5230fe16305 100644 --- a/libs/localenv/envkey_test.go +++ b/libs/localenv/envkey_test.go @@ -37,6 +37,16 @@ func TestPythonMinorFromRequires(t *testing.T) { require.NoError(t, err) assert.Equal(t, want, got, "input %q", in) } - _, err := PythonMinorFromRequires("garbage") - assert.Error(t, err) + + // A bare version with no operator is a valid floor. + got, err := PythonMinorFromRequires("3.12") + require.NoError(t, err) + assert.Equal(t, "3.12", got) + + // No usable floor: only upper-bound / exclusion clauses. Must error rather + // than select a forbidden/capped version. + for _, in := range []string{"<3.13", "<=3.12", "!=3.12", "<3.13,!=3.12", "garbage"} { + _, err := PythonMinorFromRequires(in) + assert.Error(t, err, "input %q must error", in) + } } From 7af0d4840b5876214982910d3a5570849cf59d35 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 20:25:56 +0200 Subject: [PATCH 04/15] Parse requires-python by clause and install the effective (highest) lower bound Round-3 review found two edge cases in the regex-based PythonMinorFromRequires: with multiple lower bounds (">=3.8,>=3.11") it returned the first (3.8) rather than the effective floor (3.11), and a bare floor alongside an exclusion ("!=3.11,3.12") was wrongly rejected as having no floor. Replaced the layered regexes with a small clause parser: split on commas, classify each clause by operator (>=,>,==,~=,=== and bare = floor; <,<=,!= never a floor), and return the highest floor. A spec with no floor clause ("<3.13", "!=3.12") still errors. Covers multi-lower-bound, bare-floor + exclusion, ordering, and whitespace. Co-authored-by: Isaac --- libs/localenv/envkey.go | 74 ++++++++++++++++++++---------------- libs/localenv/envkey_test.go | 17 ++++++--- 2 files changed, 53 insertions(+), 38 deletions(-) diff --git a/libs/localenv/envkey.go b/libs/localenv/envkey.go index eee1022d26f..47043324df5 100644 --- a/libs/localenv/envkey.go +++ b/libs/localenv/envkey.go @@ -3,23 +3,13 @@ package localenv import ( "fmt" "regexp" + "strconv" "strings" ) -var pythonVersionRe = regexp.MustCompile(`(\d+)\.(\d+)`) - -// lowerBoundClauseRe matches a single requires-python clause that establishes -// the Python floor to install: a lower-bound or pinning operator (>=, >, ==, -// ~=, ===) followed by a MAJOR.MINOR version. Upper-bound (<, <=) and exclusion -// (!=) clauses are deliberately not matched — they must not be chosen as the -// version to install. -var lowerBoundClauseRe = regexp.MustCompile(`(?:>=|===|==|~=|>)\s*(\d+)\.(\d+)`) - -// boundedClauseRe matches a MAJOR.MINOR that is governed by an upper-bound or -// exclusion operator (<, <=, !=). Such a version is forbidden or capped, never a -// version to install, so a spec whose only version is bounded this way has no -// usable floor. -var boundedClauseRe = regexp.MustCompile(`(?:<=|<|!=)\s*(\d+)\.(\d+)`) +// clauseRe splits a single requires-python clause into its operator (optional) +// and MAJOR.MINOR version. A clause with no operator is a bare floor. +var clauseRe = regexp.MustCompile(`^(>=|<=|===|==|~=|!=|<|>)?\s*(\d+)\.(\d+)`) // NormalizeServerless returns the canonical "vN" spelling of a serverless // version accepting "4", "v4", or "V4". @@ -37,28 +27,48 @@ func EnvKeyForSparkVersion(sparkVersion string) string { return "dbr/" + sparkVersion } -// PythonMinorFromRequires parses a PEP 440 requires-python string and extracts -// the MAJOR.MINOR of the Python version to install. +// PythonMinorFromRequires parses a PEP 440 requires-python string and returns +// the MAJOR.MINOR of the Python version to install: the effective lower bound. +// +// A requires-python is a comma-separated list of clauses in any order (e.g. +// "<3.13,>=3.10"). Each clause is classified by operator: +// - lower-bound / pinning (>=, >, ==, ~=, ===) or a bare MAJOR.MINOR with no +// operator establishes a floor; +// - upper-bound / exclusion (<, <=, !=) does not — those versions are capped +// or forbidden and must never be installed. // -// A requires-python may hold several comma-separated clauses in any order -// (e.g. "<3.13,>=3.10"). The version to install is the lower bound, so a -// lower-bound / pinning clause (>=, >, ==, ~=, ===) is preferred. A bare -// MAJOR.MINOR with no operator (e.g. "3.12") is also accepted as the floor. -// But a spec whose only version is governed by an upper-bound/exclusion -// operator (e.g. "<3.13" or "!=3.12") has no usable floor: picking that number -// would select a version the specifier forbids, so we error instead of guessing. +// The result is the highest floor across all floor clauses (so ">=3.8,>=3.11" +// yields 3.11, the version that satisfies every clause). A spec with no floor +// clause at all (e.g. "<3.13" or "!=3.12") is an error rather than a guess. func PythonMinorFromRequires(requiresPython string) (string, error) { - if m := lowerBoundClauseRe.FindStringSubmatch(requiresPython); m != nil { - return fmt.Sprintf("%s.%s", m[1], m[2]), nil + bestMajor, bestMinor := -1, -1 + sawClause := false + for clause := range strings.SplitSeq(requiresPython, ",") { + clause = strings.TrimSpace(clause) + if clause == "" { + continue + } + m := clauseRe.FindStringSubmatch(clause) + if m == nil { + continue + } + sawClause = true + op := m[1] + // Upper-bound and exclusion operators never establish a floor. + if op == "<" || op == "<=" || op == "!=" { + continue + } + major, _ := strconv.Atoi(m[2]) + minor, _ := strconv.Atoi(m[3]) + if major > bestMajor || (major == bestMajor && minor > bestMinor) { + bestMajor, bestMinor = major, minor + } } - match := pythonVersionRe.FindStringSubmatch(requiresPython) - if match == nil { - return "", fmt.Errorf("cannot parse python version from %q", requiresPython) + if bestMajor >= 0 { + return fmt.Sprintf("%d.%d", bestMajor, bestMinor), nil } - // A version exists but not behind a lower-bound/pin operator. If it is behind - // an upper-bound/exclusion operator, there is no floor to install. - if boundedClauseRe.MatchString(requiresPython) { + if sawClause { return "", fmt.Errorf("requires-python %q has no lower bound to install from", requiresPython) } - return fmt.Sprintf("%s.%s", match[1], match[2]), nil + return "", fmt.Errorf("cannot parse python version from %q", requiresPython) } diff --git a/libs/localenv/envkey_test.go b/libs/localenv/envkey_test.go index 5230fe16305..f9f334a2740 100644 --- a/libs/localenv/envkey_test.go +++ b/libs/localenv/envkey_test.go @@ -31,6 +31,16 @@ func TestPythonMinorFromRequires(t *testing.T) { ">=3.10, <3.13": "3.10", "<4.0,>=3.9": "3.9", "===3.11": "3.11", + // The effective floor is the HIGHEST lower bound, regardless of order. + ">=3.8,>=3.11": "3.11", + ">=3.11,>=3.8": "3.11", + // A bare floor alongside an exclusion is still a floor. + "!=3.11,3.12": "3.12", + "3.12,!=3.12.4": "3.12", + // Bare version with no operator. + "3.12": "3.12", + // Whitespace and patch components tolerated. + ">= 3.10 , < 3.13": "3.10", } for in, want := range cases { got, err := PythonMinorFromRequires(in) @@ -38,14 +48,9 @@ func TestPythonMinorFromRequires(t *testing.T) { assert.Equal(t, want, got, "input %q", in) } - // A bare version with no operator is a valid floor. - got, err := PythonMinorFromRequires("3.12") - require.NoError(t, err) - assert.Equal(t, "3.12", got) - // No usable floor: only upper-bound / exclusion clauses. Must error rather // than select a forbidden/capped version. - for _, in := range []string{"<3.13", "<=3.12", "!=3.12", "<3.13,!=3.12", "garbage"} { + for _, in := range []string{"<3.13", "<=3.12", "!=3.12", "<3.13,!=3.12", "garbage", ""} { _, err := PythonMinorFromRequires(in) assert.Error(t, err, "input %q must error", in) } From be61b7d31aded4d600d7acf4197e34627548df3c Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 21:13:27 +0200 Subject: [PATCH 05/15] Treat a strict ">" python bound as excluding the whole minor series Round-4 review noted PythonMinorFromRequires returned 3.10 for ">3.10", but PEP 440's ">" excludes the entire given release series (neither 3.10 nor any 3.10.x satisfies ">3.10"), so the lowest installable minor is 3.11. The clause parser now bumps the minor by one for a strict ">" bound. Co-authored-by: Isaac --- libs/localenv/envkey.go | 6 ++++++ libs/localenv/envkey_test.go | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/libs/localenv/envkey.go b/libs/localenv/envkey.go index 47043324df5..39cff1a8331 100644 --- a/libs/localenv/envkey.go +++ b/libs/localenv/envkey.go @@ -60,6 +60,12 @@ func PythonMinorFromRequires(requiresPython string) (string, error) { } major, _ := strconv.Atoi(m[2]) minor, _ := strconv.Atoi(m[3]) + // A strict ">" excludes the whole given minor series (PEP 440: ">3.10" + // matches neither 3.10 nor any 3.10.x), so the lowest installable minor is + // the next one up. + if op == ">" { + minor++ + } if major > bestMajor || (major == bestMajor && minor > bestMinor) { bestMajor, bestMinor = major, minor } diff --git a/libs/localenv/envkey_test.go b/libs/localenv/envkey_test.go index f9f334a2740..8c5f3be671f 100644 --- a/libs/localenv/envkey_test.go +++ b/libs/localenv/envkey_test.go @@ -41,6 +41,11 @@ func TestPythonMinorFromRequires(t *testing.T) { "3.12": "3.12", // Whitespace and patch components tolerated. ">= 3.10 , < 3.13": "3.10", + // Strict ">" excludes the whole minor series (PEP 440), so the floor is + // the next minor up. + ">3.10": "3.11", + ">3.10,<3.13": "3.11", + ">=3.9,>3.10": "3.11", } for in, want := range cases { got, err := PythonMinorFromRequires(in) From 900518bcf616320ca0e3d9b3c4793cf00144fc1c Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Mon, 6 Jul 2026 13:39:37 +0200 Subject: [PATCH 06/15] Address review: fix patch-qualified strict python bound; guarantee --json arrays Two items from review of the foundation layer: - PythonMinorFromRequires bumped the minor for any strict ">" bound, but a patch-qualified bound like ">3.10.5" is still satisfied by 3.10.6, so the floor should stay 3.10 (only a bare ">3.10" excludes the whole 3.10.x series). clauseRe now captures the patch component and the minor is bumped only when it is absent. Adds >3.10.5 / >=3.10.2 test cases. - Result.Phases and Result.Warnings are non-omitempty slices, so a bare Result{} would marshal them as "null" rather than "[]", an ambiguity in the --json contract. Added NewResult() which seeds both to empty slices, a doc note on the invariant, and a test asserting the JSON emits [] not null. The pipeline (later in the stack) constructs its Result through this. Co-authored-by: Isaac --- libs/localenv/envkey.go | 19 ++++++++++++------- libs/localenv/envkey_test.go | 9 +++++++-- libs/localenv/result.go | 15 +++++++++++++++ libs/localenv/result_test.go | 18 ++++++++++++++++++ 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/libs/localenv/envkey.go b/libs/localenv/envkey.go index 39cff1a8331..070668ba826 100644 --- a/libs/localenv/envkey.go +++ b/libs/localenv/envkey.go @@ -7,9 +7,12 @@ import ( "strings" ) -// clauseRe splits a single requires-python clause into its operator (optional) -// and MAJOR.MINOR version. A clause with no operator is a bare floor. -var clauseRe = regexp.MustCompile(`^(>=|<=|===|==|~=|!=|<|>)?\s*(\d+)\.(\d+)`) +// clauseRe splits a single requires-python clause into its operator (optional), +// MAJOR.MINOR version, and an optional patch component. A clause with no operator +// is a bare floor. The patch capture (group 4) is needed to interpret a strict +// ">" correctly: ">3.10" excludes all of 3.10.x, but ">3.10.5" is still satisfied +// by 3.10.6, so only the former bumps the minor. +var clauseRe = regexp.MustCompile(`^(>=|<=|===|==|~=|!=|<|>)?\s*(\d+)\.(\d+)(\.\d+)?`) // NormalizeServerless returns the canonical "vN" spelling of a serverless // version accepting "4", "v4", or "V4". @@ -60,10 +63,12 @@ func PythonMinorFromRequires(requiresPython string) (string, error) { } major, _ := strconv.Atoi(m[2]) minor, _ := strconv.Atoi(m[3]) - // A strict ">" excludes the whole given minor series (PEP 440: ">3.10" - // matches neither 3.10 nor any 3.10.x), so the lowest installable minor is - // the next one up. - if op == ">" { + hasPatch := m[4] != "" + // A strict ">" with no patch excludes the whole given minor series (PEP 440: + // ">3.10" matches neither 3.10 nor any 3.10.x), so the lowest installable + // minor is the next one up. But ">3.10.5" is still satisfied by 3.10.6, so a + // patch-qualified strict bound leaves the minor unchanged. + if op == ">" && !hasPatch { minor++ } if major > bestMajor || (major == bestMajor && minor > bestMinor) { diff --git a/libs/localenv/envkey_test.go b/libs/localenv/envkey_test.go index 8c5f3be671f..898e93434b6 100644 --- a/libs/localenv/envkey_test.go +++ b/libs/localenv/envkey_test.go @@ -41,11 +41,16 @@ func TestPythonMinorFromRequires(t *testing.T) { "3.12": "3.12", // Whitespace and patch components tolerated. ">= 3.10 , < 3.13": "3.10", - // Strict ">" excludes the whole minor series (PEP 440), so the floor is - // the next minor up. + // Strict ">" with no patch excludes the whole minor series (PEP 440), so + // the floor is the next minor up. ">3.10": "3.11", ">3.10,<3.13": "3.11", ">=3.9,>3.10": "3.11", + // Strict ">" WITH a patch does not exclude the minor series: 3.10.6 + // satisfies ">3.10.5", so the floor stays 3.10. + ">3.10.5": "3.10", + ">3.10.5,<3.13": "3.10", + ">=3.10.2": "3.10", } for in, want := range cases { got, err := PythonMinorFromRequires(in) diff --git a/libs/localenv/result.go b/libs/localenv/result.go index 82faed1b1dd..1de8f66ba50 100644 --- a/libs/localenv/result.go +++ b/libs/localenv/result.go @@ -157,6 +157,11 @@ type Warning struct { // Result is the full outcome of a sync run and the root of the --json object // (spec §6). Field order matches the spec's schema so JSON key order is stable. +// +// Phases and Warnings are non-omitempty slices, so they must always be non-nil +// before marshalling or the --json contract would emit "null" instead of "[]" — +// a distinction that trips JSON consumers and golden diffs. Construct a Result +// with NewResult (or otherwise seed both) rather than a bare Result{} literal. type Result struct { SchemaVersion int `json:"schemaVersion"` Command string `json:"command"` @@ -178,3 +183,13 @@ type Result struct { // through a clock the tests can control. DurationMs int64 `json:"durationMs"` } + +// NewResult returns a Result with the non-omitempty slice fields initialized to +// empty (non-nil) slices, so the --json output always renders "phases": [] and +// "warnings": [] rather than "null". Callers fill in the remaining fields. +func NewResult() *Result { + return &Result{ + Phases: []PhaseStatus{}, + Warnings: []Warning{}, + } +} diff --git a/libs/localenv/result_test.go b/libs/localenv/result_test.go index 76d30ec22bf..75fe60ea66d 100644 --- a/libs/localenv/result_test.go +++ b/libs/localenv/result_test.go @@ -1,10 +1,12 @@ package localenv import ( + "encoding/json" "errors" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestPipelineErrorWrapsAndExposesCode(t *testing.T) { @@ -25,3 +27,19 @@ func TestCommandName(t *testing.T) { // three-part path must join to the full command a user types. assert.Equal(t, "local-env python sync", CommandName) } + +func TestNewResultEmitsEmptyArraysNotNull(t *testing.T) { + // The --json contract requires phases/warnings to render as [] not null; + // NewResult seeds them so consumers and golden diffs see a stable shape. + b, err := json.Marshal(NewResult()) + require.NoError(t, err) + s := string(b) + assert.Contains(t, s, `"phases":[]`) + assert.Contains(t, s, `"warnings":[]`) + assert.NotContains(t, s, `"phases":null`) + assert.NotContains(t, s, `"warnings":null`) + // A bare Result{} literal is the shape NewResult exists to avoid. + bare, err := json.Marshal(&Result{}) + require.NoError(t, err) + assert.Contains(t, string(bare), `"phases":null`, "sanity: bare literal is the null case") +} From 28469e175c43fea3eb64ab195db72cbc3d0899da Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 15:17:50 +0200 Subject: [PATCH 07/15] Add local-env compute-target resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second in the stacked local-env series (builds on the foundation types). target.go resolves a compute target to a TargetInfo (and its environment key) using ordered precedence: --cluster flag → --serverless flag → --job flag → bundle target. Compute lookups go through the narrow ComputeClient seam so the resolver is unit-tested against a stub with no SDK dependency. ValidateTargetFlags guards the library path against more than one target flag being set. The classic-compute job branch reads the Spark version from the first return of GetJobSparkVersion, per that method's documented contract, rather than the recorded-version third return. Depends on the foundation PR for NewError, the E_RESOLVE / E_NO_TARGET codes, TargetInfo, and the EnvKeyFor* helpers. Still dormant. Co-authored-by: Isaac --- libs/localenv/target.go | 140 +++++++++++++++++++++++++++++++++++ libs/localenv/target_test.go | 96 ++++++++++++++++++++++++ 2 files changed, 236 insertions(+) create mode 100644 libs/localenv/target.go create mode 100644 libs/localenv/target_test.go diff --git a/libs/localenv/target.go b/libs/localenv/target.go new file mode 100644 index 00000000000..336cf1e5667 --- /dev/null +++ b/libs/localenv/target.go @@ -0,0 +1,140 @@ +package localenv + +import ( + "context" + "fmt" + "strings" +) + +// ComputeClient is a narrow seam over the SDK so tests can stub it. +type ComputeClient interface { + // GetClusterSparkVersion returns the Spark version string for a cluster. + GetClusterSparkVersion(ctx context.Context, clusterID string) (string, error) + // GetJobSparkVersion returns either a Spark version (isServerless=false) or a + // serverless marker (isServerless=true) for a job, plus a recorded version string. + GetJobSparkVersion(ctx context.Context, jobID string) (sparkVersion string, isServerless bool, version string, err error) +} + +// TargetFlags holds the mutually-exclusive compute target flags from the CLI. +type TargetFlags struct { + Cluster string + Serverless string + Job string +} + +// BundleTarget is the three-state result of reading the bundle's configured +// target. Selected=false means nothing was configured. +type BundleTarget struct { + ClusterID string + Serverless bool + Selected bool +} + +// noTargetMessage is the actionable message shown when no target is selected, +// matching spec §2.3. +const noTargetMessage = "No compute target is selected. Select a cluster or serverless target, or pass --cluster / --serverless / --job" + +// ValidateTargetFlags returns an error if more than one of the three flags is set. +// Cobra marks them mutually exclusive too; this guards the library path. +func ValidateTargetFlags(f TargetFlags) error { + var set []string + if f.Cluster != "" { + set = append(set, "--cluster") + } + if f.Serverless != "" { + set = append(set, "--serverless") + } + if f.Job != "" { + set = append(set, "--job") + } + if len(set) > 1 { + return fmt.Errorf("flags %s are mutually exclusive; specify at most one", strings.Join(set, " and ")) + } + return nil +} + +// ResolveTarget resolves the compute target using ordered precedence: +// --cluster flag → --serverless flag → --job flag → bundle target. +// PythonVersion is left empty; it is filled later from constraint data. +func ResolveTarget(ctx context.Context, f TargetFlags, c ComputeClient, bt BundleTarget) (*TargetInfo, error) { + if f.Cluster != "" { + v, err := c.GetClusterSparkVersion(ctx, f.Cluster) + if err != nil { + return nil, NewError(ErrResolve, err, "resolving cluster %s", f.Cluster) + } + return &TargetInfo{ + Source: "cluster", + ClusterID: f.Cluster, + SparkVersion: v, + EnvKey: EnvKeyForSparkVersion(v), + }, nil + } + + if f.Serverless != "" { + return &TargetInfo{ + Source: "serverless", + ServerlessVersion: NormalizeServerless(f.Serverless), + EnvKey: EnvKeyForServerless(f.Serverless), + }, nil + } + + if f.Job != "" { + sparkVersion, isServerless, version, err := c.GetJobSparkVersion(ctx, f.Job) + if err != nil { + return nil, NewError(ErrResolve, err, "resolving job %s", f.Job) + } + if isServerless { + // Default to v4 when the job is serverless; the serverless env version + // is not recorded in the bundle/project (documented stand-in from the + // original script, spec §4.3). + v := version + if v == "" { + v = "v4" + } + return &TargetInfo{ + Source: "job", + ServerlessVersion: NormalizeServerless(v), + EnvKey: EnvKeyForServerless(v), + }, nil + } + // Classic compute: the Spark version is the first return per the + // GetJobSparkVersion contract, not the recorded-version third return. + return &TargetInfo{ + Source: "job", + SparkVersion: sparkVersion, + EnvKey: EnvKeyForSparkVersion(sparkVersion), + }, nil + } + + // Fall back to bundle target. + if !bt.Selected { + return nil, NewError(ErrNoTarget, nil, "%s", noTargetMessage) + } + + if bt.Serverless { + // Default to serverless-v4: the serverless env version is not recorded + // in the bundle/project (documented stand-in from the original script). + return &TargetInfo{ + Source: "bundle", + ServerlessVersion: "v4", + EnvKey: EnvKeyForServerless("v4"), + }, nil + } + + if bt.ClusterID != "" { + v, err := c.GetClusterSparkVersion(ctx, bt.ClusterID) + if err != nil { + return nil, NewError(ErrResolve, err, "resolving bundle cluster %s", bt.ClusterID) + } + return &TargetInfo{ + Source: "bundle", + ClusterID: bt.ClusterID, + SparkVersion: v, + EnvKey: EnvKeyForSparkVersion(v), + }, nil + } + + // Bundle target is selected but has neither serverless nor a cluster ID — + // treat this the same as nothing selected so the user gets a clear message. + return nil, NewError(ErrNoTarget, nil, "%s", noTargetMessage) +} diff --git a/libs/localenv/target_test.go b/libs/localenv/target_test.go new file mode 100644 index 00000000000..b2cc2a55df7 --- /dev/null +++ b/libs/localenv/target_test.go @@ -0,0 +1,96 @@ +package localenv + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type stubCompute struct { + clusterVersion string + clusterErr error +} + +func (s stubCompute) GetClusterSparkVersion(_ context.Context, _ string) (string, error) { + return s.clusterVersion, s.clusterErr +} + +func (s stubCompute) GetJobSparkVersion(_ context.Context, _ string) (string, bool, string, error) { + return "", false, "", nil +} + +func TestResolveServerlessFlag(t *testing.T) { + ti, err := ResolveTarget(t.Context(), TargetFlags{Serverless: "v4"}, stubCompute{}, BundleTarget{}) + require.NoError(t, err) + assert.Equal(t, "serverless", ti.Source) + assert.Equal(t, "v4", ti.ServerlessVersion) + assert.Equal(t, "serverless/serverless-v4", ti.EnvKey) +} + +func TestResolveClusterFlag(t *testing.T) { + c := stubCompute{clusterVersion: "15.4.x-scala2.12"} + ti, err := ResolveTarget(t.Context(), TargetFlags{Cluster: "abc"}, c, BundleTarget{}) + require.NoError(t, err) + assert.Equal(t, "cluster", ti.Source) + assert.Equal(t, "15.4.x-scala2.12", ti.SparkVersion) + assert.Equal(t, "dbr/15.4.x-scala2.12", ti.EnvKey) + assert.Equal(t, "abc", ti.ClusterID) +} + +func TestResolveClusterFlagError(t *testing.T) { + c := stubCompute{clusterErr: errors.New("cluster not found")} + _, err := ResolveTarget(t.Context(), TargetFlags{Cluster: "abc"}, c, BundleTarget{}) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrResolve, pe.Code) +} + +func TestResolveBundleNothingSelected(t *testing.T) { + _, err := ResolveTarget(t.Context(), TargetFlags{}, stubCompute{}, BundleTarget{Selected: false}) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrNoTarget, pe.Code) +} + +func TestResolveBundleServerless(t *testing.T) { + ti, err := ResolveTarget(t.Context(), TargetFlags{}, stubCompute{}, BundleTarget{Selected: true, Serverless: true}) + require.NoError(t, err) + assert.Equal(t, "bundle", ti.Source) + assert.Equal(t, "serverless/serverless-v4", ti.EnvKey) +} + +// jobStubCompute returns distinct values for the first (sparkVersion) and third +// (recorded version) results of GetJobSparkVersion so the classic-compute branch +// can be checked against the documented contract (it must use the first). +type jobStubCompute struct { + sparkVersion string + isServerless bool + version string +} + +func (jobStubCompute) GetClusterSparkVersion(_ context.Context, _ string) (string, error) { + return "", nil +} + +func (s jobStubCompute) GetJobSparkVersion(_ context.Context, _ string) (string, bool, string, error) { + return s.sparkVersion, s.isServerless, s.version, nil +} + +func TestResolveJobClassicUsesSparkVersionReturn(t *testing.T) { + // Contract: for a classic-compute job the Spark version is the FIRST return. + // The third "recorded version" return differs here to catch use of the wrong one. + c := jobStubCompute{sparkVersion: "15.4.x-scala2.12", isServerless: false, version: "wrong-recorded"} + ti, err := ResolveTarget(t.Context(), TargetFlags{Job: "42"}, c, BundleTarget{}) + require.NoError(t, err) + assert.Equal(t, "job", ti.Source) + assert.Equal(t, "15.4.x-scala2.12", ti.SparkVersion) + assert.Equal(t, "dbr/15.4.x-scala2.12", ti.EnvKey) +} + +func TestValidateTargetFlagsMutuallyExclusive(t *testing.T) { + assert.Error(t, ValidateTargetFlags(TargetFlags{Cluster: "a", Serverless: "v4"})) + assert.NoError(t, ValidateTargetFlags(TargetFlags{Cluster: "a"})) +} From 42a7988900a9467bd4994a23758eb47cd198a74e Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 17:30:40 +0200 Subject: [PATCH 08/15] Validate mutually exclusive target flags inside ResolveTarget Review of the target layer noted that ResolveTarget accepted incompatible flags on the library path: called directly with e.g. TargetFlags{Cluster: "c", Serverless: "v4"} it silently took the first precedence branch and ignored the rest, resolving a different target than requested. Cobra and the cmd layer already reject this, but ResolveTarget is exported and ValidateTargetFlags exists specifically to guard callers that bypass Cobra, so the resolver now runs that check first and returns E_RESOLVE on conflicting flags. Co-authored-by: Isaac --- libs/localenv/target.go | 9 +++++++++ libs/localenv/target_test.go | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/libs/localenv/target.go b/libs/localenv/target.go index 336cf1e5667..b3ccb3171e1 100644 --- a/libs/localenv/target.go +++ b/libs/localenv/target.go @@ -56,7 +56,16 @@ func ValidateTargetFlags(f TargetFlags) error { // ResolveTarget resolves the compute target using ordered precedence: // --cluster flag → --serverless flag → --job flag → bundle target. // PythonVersion is left empty; it is filled later from constraint data. +// +// Incompatible flags are rejected up front: without this a library caller that +// bypasses Cobra (which also marks the flags mutually exclusive) and passes more +// than one target flag would have all but the first precedence branch silently +// ignored, resolving a different target than requested. func ResolveTarget(ctx context.Context, f TargetFlags, c ComputeClient, bt BundleTarget) (*TargetInfo, error) { + if err := ValidateTargetFlags(f); err != nil { + return nil, NewError(ErrResolve, err, "invalid compute target flags") + } + if f.Cluster != "" { v, err := c.GetClusterSparkVersion(ctx, f.Cluster) if err != nil { diff --git a/libs/localenv/target_test.go b/libs/localenv/target_test.go index b2cc2a55df7..22beaee3382 100644 --- a/libs/localenv/target_test.go +++ b/libs/localenv/target_test.go @@ -94,3 +94,13 @@ func TestValidateTargetFlagsMutuallyExclusive(t *testing.T) { assert.Error(t, ValidateTargetFlags(TargetFlags{Cluster: "a", Serverless: "v4"})) assert.NoError(t, ValidateTargetFlags(TargetFlags{Cluster: "a"})) } + +func TestResolveTargetRejectsConflictingFlags(t *testing.T) { + // ResolveTarget must reject incompatible flags rather than silently taking + // the first precedence branch, so a library caller bypassing Cobra can't + // resolve a different target than it asked for. + _, err := ResolveTarget(t.Context(), TargetFlags{Cluster: "c", Serverless: "v4"}, stubCompute{}, BundleTarget{}) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrResolve, pe.Code) +} From b621532d34fa3b74d5775dc34803320ffae6a30b Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 15:18:28 +0200 Subject: [PATCH 09/15] Add local-env constraint fetch with offline cache Third in the stacked local-env series. constraints.go fetches the per-environment pyproject.toml for a resolved env key and parses out requires-python, the databricks-connect pin, and the [tool.uv] constraint-dependencies. It caches each fetch on disk and classifies failures: a 404 is E_ENV_UNSUPPORTED (a resolvable target with no published environment, no cache fallback), while a transport or non-404 HTTP failure is E_FETCH and falls back to the last-good cached copy. The fetched body is validated by parseConstraints before it is written to the cache, so a malformed 2xx response cannot poison the cache and break a later transport-failure run that would otherwise serve the bad copy. Depends on the foundation PR for NewError and the E_FETCH / E_ENV_UNSUPPORTED codes. Still dormant. Co-authored-by: Isaac --- libs/localenv/constraints.go | 166 ++++++++++++++++++++++++++++++ libs/localenv/constraints_test.go | 90 ++++++++++++++++ 2 files changed, 256 insertions(+) create mode 100644 libs/localenv/constraints.go create mode 100644 libs/localenv/constraints_test.go diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go new file mode 100644 index 00000000000..d4925d4ac22 --- /dev/null +++ b/libs/localenv/constraints.go @@ -0,0 +1,166 @@ +package localenv + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strings" + + "github.com/BurntSushi/toml" + "github.com/databricks/cli/libs/log" +) + +// errEnvKeyNotFound is returned by fetchURL when the constraint artifact does +// not exist for the requested env key (HTTP 404). It is distinct from a +// transport failure so FetchConstraints can classify it as E_ENV_UNSUPPORTED +// (a resolvable target with no published environment) rather than E_FETCH. +var errEnvKeyNotFound = errors.New("environment key not found") + +// Constraints holds the parsed contents of a per-environment pyproject.toml. +type Constraints struct { + // EnvKey is the environment key used to look up the constraints. + EnvKey string + // SourceURL is the URL from which the constraints were fetched. + SourceURL string + // FromCache is true when the data came from the on-disk cache rather than a live fetch. + FromCache bool + // RequiresPython is the PEP 440 python version specifier from [project].requires-python. + RequiresPython string + // DatabricksConnect is the full dependency string for databricks-connect from [dependency-groups].dev. + DatabricksConnect string + // ConstraintDeps is the list of entries from [tool.uv].constraint-dependencies. + ConstraintDeps []string +} + +// sanitizeEnvKey replaces path separators with double-underscores to produce a flat filename. +func sanitizeEnvKey(envKey string) string { + return strings.ReplaceAll(envKey, "/", "__") +} + +// FetchConstraints fetches the pyproject.toml for envKey from baseURL and caches it in +// cacheDir. On a transport or non-404 HTTP failure it falls back to the cached copy if one +// exists (E_FETCH otherwise). A 404 means the env key is not published (E_ENV_UNSUPPORTED) +// and does not fall back to cache — a resolvable target with no environment is a distinct, +// non-transient condition. +// +// Constraint files are hosted at: +// https://github.com/rugpanov/databricks-environments +func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string) (*Constraints, error) { + url := baseURL + "/" + envKey + "/pyproject.toml" + cachePath := filepath.Join(cacheDir, sanitizeEnvKey(envKey)+".toml") + + data, fetchErr := fetchURL(ctx, url) + if fetchErr == nil { + // Parse before caching: a malformed 2xx body must not overwrite a valid + // cached copy, or a later transport-failure run would serve the poisoned + // cache and fail to parse instead of falling back to the last-good file. + rp, dbc, deps, err := parseConstraints(data) + if err != nil { + return nil, fmt.Errorf("parse constraints for %s: %w", envKey, err) + } + // Write the cache copy; non-fatal so a read-only cacheDir doesn't break the command. + if err := os.WriteFile(cachePath, data, 0o600); err != nil { + log.Debugf(ctx, "failed to write constraint cache %s: %v", filepath.ToSlash(cachePath), err) + } + return &Constraints{ + EnvKey: envKey, + SourceURL: url, + FromCache: false, + RequiresPython: rp, + DatabricksConnect: dbc, + ConstraintDeps: deps, + }, nil + } + + // A missing env key (404) is not a transport failure and has no useful cache + // fallback: the target resolved to an environment that isn't published. + if errors.Is(fetchErr, errEnvKeyNotFound) { + return nil, NewError(ErrEnvUnsupported, fetchErr, + "no published environment for %q. If this is a new runtime, try the latest LTS target (e.g. --serverless v4 or a supported --cluster DBR)", envKey) + } + + // Network or HTTP failure: attempt to serve from cache. + cached, readErr := os.ReadFile(cachePath) + if readErr != nil { + return nil, NewError(ErrFetch, fetchErr, "fetch constraints for %s", envKey) + } + + log.Warnf(ctx, "constraint fetch failed, using cached copy: %v", fetchErr) + rp, dbc, deps, err := parseConstraints(cached) + if err != nil { + return nil, fmt.Errorf("parse cached constraints for %s: %w", envKey, err) + } + return &Constraints{ + EnvKey: envKey, + SourceURL: url, + FromCache: true, + RequiresPython: rp, + DatabricksConnect: dbc, + ConstraintDeps: deps, + }, nil +} + +// fetchURL performs an HTTP GET and returns the body bytes, or an error on non-2xx or transport failure. +func fetchURL(ctx context.Context, url string) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("build request for %s: %w", url, err) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return nil, fmt.Errorf("GET %s: %w", url, err) + } + defer resp.Body.Close() + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("GET %s: %w", url, errEnvKeyNotFound) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil, fmt.Errorf("GET %s: unexpected status %s", url, resp.Status) + } + data, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("read body from %s: %w", url, err) + } + return data, nil +} + +// pyprojectTOML mirrors the pyproject.toml fields we care about. +type pyprojectTOML struct { + Project struct { + RequiresPython string `toml:"requires-python"` + } `toml:"project"` + DependencyGroups struct { + Dev []string `toml:"dev"` + } `toml:"dependency-groups"` + Tool struct { + UV struct { + ConstraintDependencies []string `toml:"constraint-dependencies"` + } `toml:"uv"` + } `toml:"tool"` +} + +// parseConstraints parses a pyproject.toml byte slice and extracts requires-python, +// the databricks-connect entry from dependency-groups.dev, and constraint-dependencies. +func parseConstraints(data []byte) (requiresPython, dbconnect string, deps []string, err error) { + var p pyprojectTOML + if err = toml.Unmarshal(data, &p); err != nil { + return "", "", nil, fmt.Errorf("unmarshal pyproject.toml: %w", err) + } + + requiresPython = p.Project.RequiresPython + + for _, entry := range p.DependencyGroups.Dev { + // Despace before matching so whitespace variants like "databricks-connect ~=17" also match. + if strings.HasPrefix(strings.ReplaceAll(entry, " ", ""), "databricks-connect") { + dbconnect = entry + break + } + } + + deps = p.Tool.UV.ConstraintDependencies + return requiresPython, dbconnect, deps, nil +} diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go new file mode 100644 index 00000000000..bced735bc45 --- /dev/null +++ b/libs/localenv/constraints_test.go @@ -0,0 +1,90 @@ +package localenv + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const sampleToml = `[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [ + "databricks-connect~=17.2.0", + "pytest~=8.0", +] + +[tool.uv] +constraint-dependencies = [ + "pydantic~=2.10.6", + "anyio~=4.6.2", +] +` + +func TestParseConstraints(t *testing.T) { + rp, dbc, deps, err := parseConstraints([]byte(sampleToml)) + require.NoError(t, err) + assert.Equal(t, "==3.12.*", rp) + assert.Equal(t, "databricks-connect~=17.2.0", dbc) + assert.Equal(t, []string{"pydantic~=2.10.6", "anyio~=4.6.2"}, deps) +} + +func TestFetchConstraintsHTTP(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/serverless/serverless-v4/pyproject.toml", r.URL.Path) + _, _ = w.Write([]byte(sampleToml)) + })) + defer srv.Close() + + c, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir()) + require.NoError(t, err) + assert.False(t, c.FromCache) + assert.Equal(t, "databricks-connect~=17.2.0", c.DatabricksConnect) + assert.Len(t, c.ConstraintDeps, 2) +} + +func TestFetchConstraintsEnvKeyNotFound(t *testing.T) { + // A 404 for a resolved env key means the environment is not published; this + // must classify as E_ENV_UNSUPPORTED, not E_FETCH, and not fall back to cache. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not found", http.StatusNotFound) + })) + defer srv.Close() + + _, err := FetchConstraints(t.Context(), srv.URL, "dbr/99.9.x-scala2.12", t.TempDir()) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrEnvUnsupported, pe.Code) +} + +func TestFetchConstraintsTransportFailureNoCache(t *testing.T) { + // A transport failure with no cache classifies as E_FETCH. + down := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + url := down.URL + down.Close() + + _, err := FetchConstraints(t.Context(), url, "serverless/serverless-v4", t.TempDir()) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrFetch, pe.Code) +} + +func TestFetchConstraintsFallsBackToCache(t *testing.T) { + cacheDir := t.TempDir() + // First, a successful fetch populates the cache. + good := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(sampleToml)) + })) + _, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir) + require.NoError(t, err) + good.Close() + + // Now the server is down; fetch must serve the cache. + c, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir) + require.NoError(t, err) + assert.True(t, c.FromCache) +} From 53ed3a107dd884d58e154f550d6fa90a969fdba6 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 17:41:45 +0200 Subject: [PATCH 10/15] Harden constraint fetch: validate artifact, create+atomically write cache, exact dep match Review of the constraint-fetch layer surfaced several defects, all fixed here: - parseConstraints accepted valid-but-empty TOML: a 200 body with no [project].requires-python returned an empty result that would be cached and only fail confusingly later. It now errors when requires-python is absent. - The cache write assumed cacheDir already existed. On a fresh machine the first fetch succeeded but os.WriteFile failed (no such directory), so the cache never populated and offline runs got E_FETCH. writeCacheAtomic now MkdirAll's the parent. - The cache write was non-atomic (os.WriteFile truncates in place), so a concurrent transport-failure reader could observe a partial file. writeCacheAtomic writes a temp file and renames it into place. - databricks-connect detection used a bare string prefix, so a sibling like "databricks-connectors==1.0" matched first and was returned instead of the real pin. isDatabricksConnectDep now requires a package-name boundary. - sanitizeEnvKey collapsed only "/", leaving a Windows "\" to be treated as a separator by filepath.Join; it now collapses both. Co-authored-by: Isaac --- libs/localenv/constraints.go | 84 ++++++++++++++++++++++++++++--- libs/localenv/constraints_test.go | 43 ++++++++++++++++ 2 files changed, 121 insertions(+), 6 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index d4925d4ac22..ffd93140b35 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -36,9 +36,48 @@ type Constraints struct { ConstraintDeps []string } -// sanitizeEnvKey replaces path separators with double-underscores to produce a flat filename. +// sanitizeEnvKey flattens an env key to a single filename component by replacing +// every path separator (both "/" and the OS separator, e.g. "\\" on Windows) with +// double-underscores. Collapsing both keeps the cache file inside cacheDir even on +// Windows, where filepath.Join would otherwise treat a backslash as a separator. func sanitizeEnvKey(envKey string) string { - return strings.ReplaceAll(envKey, "/", "__") + s := strings.ReplaceAll(envKey, "/", "__") + s = strings.ReplaceAll(s, "\\", "__") + return s +} + +// writeCacheAtomic writes data to path via a temp file and rename, creating the +// parent directory first. The rename is atomic on the same filesystem, so a +// concurrent reader never observes a truncated or partial cache file (os.WriteFile +// truncates in place, which a fallback reader could catch mid-write). +func writeCacheAtomic(path string, data []byte) error { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + tmp, err := os.CreateTemp(dir, ".constraints-*.tmp") + if err != nil { + return err + } + tmpName := tmp.Name() + if _, err := tmp.Write(data); err != nil { + tmp.Close() + os.Remove(tmpName) + return err + } + if err := tmp.Close(); err != nil { + os.Remove(tmpName) + return err + } + if err := os.Chmod(tmpName, 0o600); err != nil { + os.Remove(tmpName) + return err + } + if err := os.Rename(tmpName, path); err != nil { + os.Remove(tmpName) + return err + } + return nil } // FetchConstraints fetches the pyproject.toml for envKey from baseURL and caches it in @@ -62,8 +101,9 @@ func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string) (*C if err != nil { return nil, fmt.Errorf("parse constraints for %s: %w", envKey, err) } - // Write the cache copy; non-fatal so a read-only cacheDir doesn't break the command. - if err := os.WriteFile(cachePath, data, 0o600); err != nil { + // Write the cache copy (creating cacheDir if needed, atomically); non-fatal + // so a read-only cacheDir doesn't break the command. + if err := writeCacheAtomic(cachePath, data); err != nil { log.Debugf(ctx, "failed to write constraint cache %s: %v", filepath.ToSlash(cachePath), err) } return &Constraints{ @@ -145,6 +185,9 @@ type pyprojectTOML struct { // parseConstraints parses a pyproject.toml byte slice and extracts requires-python, // the databricks-connect entry from dependency-groups.dev, and constraint-dependencies. +// A body that is valid TOML but carries no requires-python is rejected: it is not a +// usable constraint artifact, and silently accepting it would cache an empty result +// and only surface a confusing failure later in the pipeline. func parseConstraints(data []byte) (requiresPython, dbconnect string, deps []string, err error) { var p pyprojectTOML if err = toml.Unmarshal(data, &p); err != nil { @@ -152,10 +195,12 @@ func parseConstraints(data []byte) (requiresPython, dbconnect string, deps []str } requiresPython = p.Project.RequiresPython + if strings.TrimSpace(requiresPython) == "" { + return "", "", nil, errors.New("constraint artifact has no [project].requires-python") + } for _, entry := range p.DependencyGroups.Dev { - // Despace before matching so whitespace variants like "databricks-connect ~=17" also match. - if strings.HasPrefix(strings.ReplaceAll(entry, " ", ""), "databricks-connect") { + if isDatabricksConnectDep(entry) { dbconnect = entry break } @@ -164,3 +209,30 @@ func parseConstraints(data []byte) (requiresPython, dbconnect string, deps []str deps = p.Tool.UV.ConstraintDependencies return requiresPython, dbconnect, deps, nil } + +// isDatabricksConnectDep reports whether a dependency-group entry is the +// databricks-connect requirement. It matches on a package-name boundary rather +// than a bare prefix so a sibling package such as "databricks-connectors" (whose +// name merely starts with "databricks-connect") is not mistaken for it. The next +// character after the name must be a PEP 508 version/extra/marker delimiter or the +// end of the string. +func isDatabricksConnectDep(entry string) bool { + const name = "databricks-connect" + // Despace so whitespace variants like "databricks-connect ~=17" also match. + s := strings.ReplaceAll(entry, " ", "") + rest, ok := strings.CutPrefix(s, name) + if !ok { + return false + } + if rest == "" { + return true + } + // A real requirement continues with a version specifier, extra, marker, or + // separator — never an identifier character (which would mean a longer name). + switch rest[0] { + case '=', '<', '>', '!', '~', '[', ';', '@', ',', '(': + return true + default: + return false + } +} diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index bced735bc45..c5b54a56785 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -3,6 +3,8 @@ package localenv import ( "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -33,6 +35,47 @@ func TestParseConstraints(t *testing.T) { assert.Equal(t, []string{"pydantic~=2.10.6", "anyio~=4.6.2"}, deps) } +func TestParseConstraintsRejectsMissingRequiresPython(t *testing.T) { + // Valid TOML but no requires-python is not a usable artifact; it must error + // rather than return an empty result that would be cached and fail later. + _, _, _, err := parseConstraints([]byte("[project]\nname = \"x\"\n")) + require.Error(t, err) +} + +func TestParseConstraintsDatabricksConnectNameBoundary(t *testing.T) { + // A sibling package whose name merely starts with "databricks-connect" must + // not be mistaken for the databricks-connect requirement. + toml := `[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "databricks-connectors==1.0", + "databricks-connect~=17.2.0", +] +` + _, dbc, _, err := parseConstraints([]byte(toml)) + require.NoError(t, err) + assert.Equal(t, "databricks-connect~=17.2.0", dbc) +} + +func TestFetchConstraintsCreatesCacheDir(t *testing.T) { + // The cache directory may not exist yet on a fresh machine; the fetch must + // create it so the cache actually populates (and offline fallback works). + cacheDir := filepath.Join(t.TempDir(), "does", "not", "exist", "yet") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(sampleToml)) + })) + defer srv.Close() + + _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir) + require.NoError(t, err) + // The cache file was written into the freshly created directory. + written, err := os.ReadFile(filepath.Join(cacheDir, "serverless__serverless-v4.toml")) + require.NoError(t, err) + assert.Equal(t, sampleToml, string(written)) +} + func TestFetchConstraintsHTTP(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/serverless/serverless-v4/pyproject.toml", r.URL.Path) From 4783518f00d20f603919d4b8679e30682b1830fe Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 20:14:19 +0200 Subject: [PATCH 11/15] Make the constraint cache filename collision-free Round-2 review noted that mapping an env key to a cache filename by replacing "/" with "__" was not injective: distinct keys like "a/b" and "a__b" collided on the same file, so a cached copy for one environment could be served for another on a transport-failure fallback. cacheFileName now appends a short sha256 of the raw env key to the readable slug, guaranteeing distinct keys get distinct files (env keys are internally generated and never actually collide today, but the cache should not depend on that). Co-authored-by: Isaac --- libs/localenv/constraints.go | 23 ++++++++++++++--------- libs/localenv/constraints_test.go | 11 ++++++++++- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index ffd93140b35..06d84e64b0a 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -2,6 +2,8 @@ package localenv import ( "context" + "crypto/sha256" + "encoding/hex" "errors" "fmt" "io" @@ -36,14 +38,17 @@ type Constraints struct { ConstraintDeps []string } -// sanitizeEnvKey flattens an env key to a single filename component by replacing -// every path separator (both "/" and the OS separator, e.g. "\\" on Windows) with -// double-underscores. Collapsing both keeps the cache file inside cacheDir even on -// Windows, where filepath.Join would otherwise treat a backslash as a separator. -func sanitizeEnvKey(envKey string) string { - s := strings.ReplaceAll(envKey, "/", "__") - s = strings.ReplaceAll(s, "\\", "__") - return s +// cacheFileName maps an env key to a single, collision-free cache filename. +// It keeps a readable slug (path separators flattened to double-underscores so +// the file stays inside cacheDir on every OS) and appends a short hash of the +// raw env key. The hash guarantees injectivity: distinct env keys that would +// otherwise flatten to the same slug (e.g. "a/b" and "a__b") get distinct +// filenames, so a cache hit can never serve another environment's constraints. +func cacheFileName(envKey string) string { + slug := strings.ReplaceAll(envKey, "/", "__") + slug = strings.ReplaceAll(slug, "\\", "__") + sum := sha256.Sum256([]byte(envKey)) + return fmt.Sprintf("%s-%s.toml", slug, hex.EncodeToString(sum[:8])) } // writeCacheAtomic writes data to path via a temp file and rename, creating the @@ -90,7 +95,7 @@ func writeCacheAtomic(path string, data []byte) error { // https://github.com/rugpanov/databricks-environments func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string) (*Constraints, error) { url := baseURL + "/" + envKey + "/pyproject.toml" - cachePath := filepath.Join(cacheDir, sanitizeEnvKey(envKey)+".toml") + cachePath := filepath.Join(cacheDir, cacheFileName(envKey)) data, fetchErr := fetchURL(ctx, url) if fetchErr == nil { diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index c5b54a56785..dfe86968d10 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -71,11 +71,20 @@ func TestFetchConstraintsCreatesCacheDir(t *testing.T) { _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir) require.NoError(t, err) // The cache file was written into the freshly created directory. - written, err := os.ReadFile(filepath.Join(cacheDir, "serverless__serverless-v4.toml")) + written, err := os.ReadFile(filepath.Join(cacheDir, cacheFileName("serverless/serverless-v4"))) require.NoError(t, err) assert.Equal(t, sampleToml, string(written)) } +func TestCacheFileNameInjective(t *testing.T) { + // Distinct env keys that flatten to the same slug must not collide, so a + // cache hit can never serve another environment's constraints. + assert.NotEqual(t, cacheFileName("a/b"), cacheFileName("a__b")) + // The filename stays inside cacheDir (no separators leak through). + assert.NotContains(t, cacheFileName("a/b"), "/") + assert.NotContains(t, cacheFileName("a\\b"), "\\") +} + func TestFetchConstraintsHTTP(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/serverless/serverless-v4/pyproject.toml", r.URL.Path) From f7e3e3f1b3c4dd3cc93cb0a31898db617830df74 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 20:26:20 +0200 Subject: [PATCH 12/15] Match databricks-connect case-insensitively Round-3 review noted isDatabricksConnectDep was case-sensitive, but Python package names are case-insensitive (PEP 503): a valid entry like "Databricks-Connect==16.4.0" went undetected, leaving the pin empty. The match now lowercases the entry before comparing; the caller still stores the original casing. Co-authored-by: Isaac --- libs/localenv/constraints.go | 6 ++++-- libs/localenv/constraints_test.go | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index 06d84e64b0a..a75813fdff6 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -223,8 +223,10 @@ func parseConstraints(data []byte) (requiresPython, dbconnect string, deps []str // end of the string. func isDatabricksConnectDep(entry string) bool { const name = "databricks-connect" - // Despace so whitespace variants like "databricks-connect ~=17" also match. - s := strings.ReplaceAll(entry, " ", "") + // Despace so whitespace variants like "databricks-connect ~=17" also match, + // and lowercase because Python package names are case-insensitive (PEP 503), + // so "Databricks-Connect==16.4.0" is the same requirement. + s := strings.ToLower(strings.ReplaceAll(entry, " ", "")) rest, ok := strings.CutPrefix(s, name) if !ok { return false diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index dfe86968d10..df61427f298 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -59,6 +59,20 @@ dev = [ assert.Equal(t, "databricks-connect~=17.2.0", dbc) } +func TestParseConstraintsDatabricksConnectCaseInsensitive(t *testing.T) { + // Python package names are case-insensitive (PEP 503), so a differently-cased + // entry must still be detected; the original casing is preserved in the result. + toml := `[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["Databricks-Connect==16.4.0"] +` + _, dbc, _, err := parseConstraints([]byte(toml)) + require.NoError(t, err) + assert.Equal(t, "Databricks-Connect==16.4.0", dbc) +} + func TestFetchConstraintsCreatesCacheDir(t *testing.T) { // The cache directory may not exist yet on a fresh machine; the fetch must // create it so the cache actually populates (and offline fallback works). From 6dcf041100c6194a142eea24626f4bde66f0cfc8 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 21:13:53 +0200 Subject: [PATCH 13/15] Normalize databricks-connect dependency name per PEP 503 Round-4 review found isDatabricksConnectDep matched only case, missing PEP 503 name equivalence: "databricks_connect" and "databricks.connect" (and other whitespace) were not recognized. The check now extracts the leading package name up to the first PEP 508 delimiter and compares it under PEP 503 normalization (lowercase, collapse runs of -, _, . to a single -), so all spellings match while a distinct package like databricks-connectors does not. Co-authored-by: Isaac --- libs/localenv/constraints.go | 47 +++++++++++++++---------------- libs/localenv/constraints_test.go | 28 +++++++++++------- 2 files changed, 41 insertions(+), 34 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index a75813fdff6..9673d5ece89 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "path/filepath" + "regexp" "strings" "github.com/BurntSushi/toml" @@ -215,31 +216,29 @@ func parseConstraints(data []byte) (requiresPython, dbconnect string, deps []str return requiresPython, dbconnect, deps, nil } +// depNameSepRe matches the first PEP 508 delimiter that ends a requirement's +// package name: a version specifier, extra, marker, url, or list separator. +var depNameSepRe = regexp.MustCompile(`[<>=!~;,@\[( \t]`) + // isDatabricksConnectDep reports whether a dependency-group entry is the -// databricks-connect requirement. It matches on a package-name boundary rather -// than a bare prefix so a sibling package such as "databricks-connectors" (whose -// name merely starts with "databricks-connect") is not mistaken for it. The next -// character after the name must be a PEP 508 version/extra/marker delimiter or the -// end of the string. +// databricks-connect requirement. It extracts the leading package name (up to +// the first PEP 508 delimiter) and compares it under PEP 503 normalization, so +// case, and runs of "-", "_", or "." are all treated as equivalent: +// "Databricks-Connect", "databricks_connect", and "databricks.connect" all match, +// while a distinct package like "databricks-connectors" does not. func isDatabricksConnectDep(entry string) bool { - const name = "databricks-connect" - // Despace so whitespace variants like "databricks-connect ~=17" also match, - // and lowercase because Python package names are case-insensitive (PEP 503), - // so "Databricks-Connect==16.4.0" is the same requirement. - s := strings.ToLower(strings.ReplaceAll(entry, " ", "")) - rest, ok := strings.CutPrefix(s, name) - if !ok { - return false - } - if rest == "" { - return true - } - // A real requirement continues with a version specifier, extra, marker, or - // separator — never an identifier character (which would mean a longer name). - switch rest[0] { - case '=', '<', '>', '!', '~', '[', ';', '@', ',', '(': - return true - default: - return false + name := strings.TrimSpace(entry) + if i := depNameSepRe.FindStringIndex(name); i != nil { + name = name[:i[0]] } + return normalizePackageName(name) == "databricks-connect" +} + +// pep503SepRe matches runs of "-", "_", or "." for PEP 503 name normalization. +var pep503SepRe = regexp.MustCompile(`[-_.]+`) + +// normalizePackageName applies PEP 503 normalization: lowercase and collapse any +// run of "-", "_", or "." to a single "-". +func normalizePackageName(name string) string { + return pep503SepRe.ReplaceAllString(strings.ToLower(strings.TrimSpace(name)), "-") } diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index df61427f298..9af68c97869 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -59,18 +59,26 @@ dev = [ assert.Equal(t, "databricks-connect~=17.2.0", dbc) } -func TestParseConstraintsDatabricksConnectCaseInsensitive(t *testing.T) { - // Python package names are case-insensitive (PEP 503), so a differently-cased - // entry must still be detected; the original casing is preserved in the result. - toml := `[project] -requires-python = ">=3.10" - -[dependency-groups] -dev = ["Databricks-Connect==16.4.0"] -` +func TestParseConstraintsDatabricksConnectPEP503(t *testing.T) { + // PEP 503: package names are case-insensitive and runs of -, _, . are + // equivalent. Every spelling of databricks-connect must be detected, with the + // original entry preserved verbatim in the result. + for _, entry := range []string{ + "Databricks-Connect==16.4.0", + "databricks_connect==16.4.0", + "databricks.connect==16.4.0", + "databricks-connect ~= 17.2", + } { + toml := "[project]\nrequires-python = \">=3.10\"\n\n[dependency-groups]\ndev = [\"" + entry + "\"]\n" + _, dbc, _, err := parseConstraints([]byte(toml)) + require.NoError(t, err, entry) + assert.Equal(t, entry, dbc, "entry %q", entry) + } + // A distinct sibling package must NOT match. + toml := "[project]\nrequires-python = \">=3.10\"\n\n[dependency-groups]\ndev = [\"databricks-connectors==1.0\"]\n" _, dbc, _, err := parseConstraints([]byte(toml)) require.NoError(t, err) - assert.Equal(t, "Databricks-Connect==16.4.0", dbc) + assert.Empty(t, dbc) } func TestFetchConstraintsCreatesCacheDir(t *testing.T) { From 0ef2220bc1e12984711b23b26f2db93140f18a4b Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Mon, 6 Jul 2026 13:42:03 +0200 Subject: [PATCH 14/15] Address review: bound the constraint fetch (timeout + size cap) Hardening the fetch of untrusted remote constraint artifacts, per review: - Use a dedicated http.Client with a 30s timeout instead of http.DefaultClient (which has none), so the request is bounded even if the caller's context carries no deadline. - Cap the response body with io.LimitReader (1 MiB, far above any real pyproject.toml) so a misbehaving or hostile host can't read an unbounded body into memory; an over-cap body is rejected. Adds a test for the oversized-body rejection. Note: the separate (blocking) review point about the production artifact host being a personal GitHub repo is tracked on the PR and gated on the unveil PR; it is a hosting/ownership decision, not a code change here. Co-authored-by: Isaac --- libs/localenv/constraints.go | 23 +++++++++++++++++++++-- libs/localenv/constraints_test.go | 16 ++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index 9673d5ece89..c4a09e0aa13 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -12,6 +12,7 @@ import ( "path/filepath" "regexp" "strings" + "time" "github.com/BurntSushi/toml" "github.com/databricks/cli/libs/log" @@ -23,6 +24,15 @@ import ( // (a resolvable target with no published environment) rather than E_FETCH. var errEnvKeyNotFound = errors.New("environment key not found") +// maxConstraintBytes caps the constraint artifact read. The body is untrusted +// remote content and a pyproject.toml is small; 1 MiB is far above any real +// artifact while preventing a misbehaving or hostile host from exhausting memory. +const maxConstraintBytes int64 = 1 << 20 + +// constraintHTTPClient fetches constraint artifacts with an explicit timeout, so +// the request is bounded even when the caller's context has no deadline. +var constraintHTTPClient = &http.Client{Timeout: 30 * time.Second} + // Constraints holds the parsed contents of a per-environment pyproject.toml. type Constraints struct { // EnvKey is the environment key used to look up the constraints. @@ -156,7 +166,10 @@ func fetchURL(ctx context.Context, url string) ([]byte, error) { if err != nil { return nil, fmt.Errorf("build request for %s: %w", url, err) } - resp, err := http.DefaultClient.Do(req) + // Use a client with an explicit timeout rather than http.DefaultClient, which + // has none: the fetch of remote content must be bounded even if the caller's + // context carries no deadline. + resp, err := constraintHTTPClient.Do(req) if err != nil { return nil, fmt.Errorf("GET %s: %w", url, err) } @@ -167,10 +180,16 @@ func fetchURL(ctx context.Context, url string) ([]byte, error) { if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("GET %s: unexpected status %s", url, resp.Status) } - data, err := io.ReadAll(resp.Body) + // Cap the read: the body is untrusted remote content and a pyproject.toml is + // small, so an oversized (or hostile) response must not be read into memory + // unbounded. Read one byte past the cap to detect an over-limit body. + data, err := io.ReadAll(io.LimitReader(resp.Body, maxConstraintBytes+1)) if err != nil { return nil, fmt.Errorf("read body from %s: %w", url, err) } + if int64(len(data)) > maxConstraintBytes { + return nil, fmt.Errorf("constraint artifact from %s exceeds %d bytes", url, maxConstraintBytes) + } return data, nil } diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index 9af68c97869..66ed3675b04 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -147,6 +148,21 @@ func TestFetchConstraintsTransportFailureNoCache(t *testing.T) { assert.Equal(t, ErrFetch, pe.Code) } +func TestFetchConstraintsRejectsOversizedBody(t *testing.T) { + // An over-cap response body must be rejected (classified E_FETCH here, with no + // cache to fall back to) rather than read unbounded into memory. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + big := strings.Repeat("x", int(maxConstraintBytes)+100) + _, _ = w.Write([]byte(big)) + })) + defer srv.Close() + + _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir()) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrFetch, pe.Code) +} + func TestFetchConstraintsFallsBackToCache(t *testing.T) { cacheDir := t.TempDir() // First, a successful fetch populates the cache. From 4f83973fd5262f47d0d9cbf823b8c9646ed6e34b Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Mon, 6 Jul 2026 17:38:50 +0200 Subject: [PATCH 15/15] Address review: source the constraint repo from an env var, no personal-repo default Per review, the constraint artifacts must not be sourced from a hardcoded personal GitHub repo. This parameterizes the host: RepoConstraintBaseURL reads the hosting repo ("owner/name") from the DATABRICKS_LOCALENV_CONSTRAINT_REPO environment variable and builds a raw.githubusercontent.com main-branch URL. The built-in default is intentionally empty and resolution errors when no repo is configured, so no untrusted default controls what the CLI installs. This is temporary: once the Databricks-owned databricks/environments repo can publish the artifacts (its GitHub Actions are currently disabled), defaultConstraintRepo becomes that constant and the env var is no longer required. Tracked as a follow-up; the command stays hidden until the unveil PR regardless. Co-authored-by: Isaac --- libs/localenv/constraints.go | 44 +++++++++++++++++++++++++++++-- libs/localenv/constraints_test.go | 25 ++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index c4a09e0aa13..22a2c49f66e 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -15,9 +15,41 @@ import ( "time" "github.com/BurntSushi/toml" + "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" ) +// EnvConstraintRepo names the environment variable that supplies the GitHub repo +// ("owner/name") hosting the environment constraint artifacts. +const EnvConstraintRepo = "DATABRICKS_LOCALENV_CONSTRAINT_REPO" + +// defaultConstraintRepo is the GitHub repo that hosts the constraint artifacts. +// It is intentionally empty for now: the artifacts must move to a +// Databricks-owned repo (databricks/environments) before this ships, but that +// repo can't publish them yet (its GitHub Actions are disabled). Until then the +// repo is supplied via the EnvConstraintRepo environment variable rather than +// hardcoding a personal repo, so no untrusted default controls what the CLI +// installs. Once databricks/environments is ready this becomes that constant. +const defaultConstraintRepo = "" + +// RepoConstraintBaseURL resolves the base URL for constraint artifacts from the +// hosting GitHub repo: EnvConstraintRepo overrides the (currently empty) built-in +// default, and the repo is turned into a raw.githubusercontent.com main-branch +// URL. It returns "" when no repo is configured; the caller must not fall back to +// any other source, and FetchConstraints reports the missing configuration as a +// fetch-phase error so it surfaces through the normal phase/JSON reporting rather +// than aborting the command before the phase table is rendered. +func RepoConstraintBaseURL(ctx context.Context) string { + repo := defaultConstraintRepo + if v, ok := env.Lookup(ctx, EnvConstraintRepo); ok && strings.TrimSpace(v) != "" { + repo = strings.TrimSpace(v) + } + if repo == "" { + return "" + } + return "https://raw.githubusercontent.com/" + repo + "/main" +} + // errEnvKeyNotFound is returned by fetchURL when the constraint artifact does // not exist for the requested env key (HTTP 404). It is distinct from a // transport failure so FetchConstraints can classify it as E_ENV_UNSUPPORTED @@ -102,9 +134,17 @@ func writeCacheAtomic(path string, data []byte) error { // and does not fall back to cache — a resolvable target with no environment is a distinct, // non-transient condition. // -// Constraint files are hosted at: -// https://github.com/rugpanov/databricks-environments +// baseURL points at the repo hosting the constraint artifacts (see +// RepoConstraintBaseURL); it is empty when no source is configured, which is +// reported below as a fetch-phase error. func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string) (*Constraints, error) { + if baseURL == "" { + // No constraint host is configured. This is reported at the fetch phase (as + // E_FETCH) rather than aborting earlier, so the failure flows through the + // same phase/JSON reporting as any other fetch error. + return nil, NewError(ErrFetch, nil, + "no constraint source configured: set %s to the GitHub repo (owner/name) that hosts the environment constraints", EnvConstraintRepo) + } url := baseURL + "/" + envKey + "/pyproject.toml" cachePath := filepath.Join(cacheDir, cacheFileName(envKey)) diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index 66ed3675b04..8df363d943e 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -8,10 +8,35 @@ import ( "strings" "testing" + "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestRepoConstraintBaseURL(t *testing.T) { + // With no repo configured (empty built-in default), it returns "" so the caller + // can report the missing source at the fetch phase rather than aborting early. + assert.Empty(t, RepoConstraintBaseURL(t.Context())) + + // The env var supplies the repo and is turned into a raw main-branch URL. + ctx := env.Set(t.Context(), EnvConstraintRepo, "databricks/environments") + assert.Equal(t, "https://raw.githubusercontent.com/databricks/environments/main", RepoConstraintBaseURL(ctx)) + + // Whitespace-only is treated as unset. + ctx = env.Set(t.Context(), EnvConstraintRepo, " ") + assert.Empty(t, RepoConstraintBaseURL(ctx)) +} + +func TestFetchConstraintsNoSourceConfigured(t *testing.T) { + // An empty base URL means no constraint host is configured; it must classify as + // E_FETCH (surfaced at the fetch phase) and name the env var to set. + _, err := FetchConstraints(t.Context(), "", "serverless/serverless-v4", t.TempDir()) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrFetch, pe.Code) + assert.Contains(t, pe.Error(), EnvConstraintRepo) +} + const sampleToml = `[project] requires-python = "==3.12.*"