From 22f99d96f349ee55ee22a1ef5ea9513b9f65cac6 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 15:17:20 +0200 Subject: [PATCH 01/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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.*" From dcaf3a67d7f97e23b8d97ebc4708fcc27d2e0646 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 15:19:15 +0200 Subject: [PATCH 16/25] Add local-env formatting-preserving pyproject.toml merge Fourth in the stacked local-env series. merge.go rewrites only the env-owned sections of a pyproject.toml and preserves every other byte (comments, ordering, whitespace, CRLF). It updates requires-python and the databricks-connect pin in place, and maintains a marker-bracketed managed [tool.uv] constraint block. The operation is idempotent: feeding its own output back in is byte-identical. RenderFreshPyproject produces a complete managed file for a greenfield project. Two correctness properties this file has to get right, both covered by tests that parse the result as TOML rather than asserting on strings: - When the user's pyproject.toml already has a [tool.uv] table with a non-constraint key, the managed constraint-dependencies nests header-less inside that table instead of emitting a second [tool.uv] header (two headers for one table is invalid TOML that uv rejects). - Single- vs multi-line constraint-dependencies detection tracks real bracket depth outside strings and comments, so an opening line that contains a "]" inside an element (e.g. "requests[security]~=2.0") or a trailing comment is not misread as single-line and mis-stripped. Depends on the constraints PR for the Constraints type. Still dormant. Co-authored-by: Isaac --- libs/localenv/merge.go | 445 ++++++++++++++++++++++++++++++++++++ libs/localenv/merge_test.go | 322 ++++++++++++++++++++++++++ 2 files changed, 767 insertions(+) create mode 100644 libs/localenv/merge.go create mode 100644 libs/localenv/merge_test.go diff --git a/libs/localenv/merge.go b/libs/localenv/merge.go new file mode 100644 index 00000000000..ca873b5deb0 --- /dev/null +++ b/libs/localenv/merge.go @@ -0,0 +1,445 @@ +package localenv + +import ( + "fmt" + "regexp" + "strings" +) + +// managedMarkerStart and managedMarkerEnd bracket the region of pyproject.toml that +// "databricks local-env python sync" owns. Everything between them is rewritten on each merge; +// everything outside is preserved byte-for-byte. +const ( + managedMarkerStart = "# managed by databricks local-env python sync — do not edit" + managedMarkerEnd = "# end managed by databricks local-env python sync" +) + +// Region names reported back to the caller via MergeManaged's regions return value. +const ( + regionRequiresPython = "requires-python" + regionDatabricksConnect = "databricks-connect" + regionToolUv = "tool.uv.constraint-dependencies" +) + +var ( + // tableHeaderRe matches a TOML table header line such as "[project]" or "[tool.uv]". + tableHeaderRe = regexp.MustCompile(`^\s*\[[^\]]+\]\s*$`) + // requiresPythonRe captures the leading whitespace of a requires-python assignment so it + // can be preserved when the value is replaced. + requiresPythonRe = regexp.MustCompile(`^(\s*)requires-python\s*=`) +) + +// MergeManaged applies the three managed transforms to target, preserving every other +// byte (comments, ordering, whitespace). It returns the merged bytes and the list of +// regions that actually changed. The operation is idempotent: feeding its own output +// back in produces identical bytes. +func MergeManaged(target []byte, c Constraints) (merged []byte, regions []string, err error) { + s := string(target) + + // Detect and normalize line endings. We process on "\n" and restore "\r\n" on exit. + crlf := strings.Contains(s, "\r\n") + if crlf { + s = strings.ReplaceAll(s, "\r\n", "\n") + } + + lines := strings.Split(s, "\n") + + lines, rpChanged := mergeRequiresPython(lines, c.RequiresPython) + if rpChanged { + regions = append(regions, regionRequiresPython) + } + + lines, dbcChanged := mergeDatabricksConnect(lines, c.DatabricksConnect) + if dbcChanged { + regions = append(regions, regionDatabricksConnect) + } + + lines, uvChanged := mergeToolUv(lines, c.ConstraintDeps) + if uvChanged { + regions = append(regions, regionToolUv) + } + + out := strings.Join(lines, "\n") + if crlf { + out = strings.ReplaceAll(out, "\n", "\r\n") + } + return []byte(out), regions, nil +} + +// tableBounds returns the line index of the header matching name (e.g. "[project]") and +// the index of the first line after the table body (the next table header or EOF). If the +// table is absent, found is false. +func tableBounds(lines []string, name string) (header, end int, found bool) { + header = -1 + for i, line := range lines { + if strings.TrimSpace(line) == name { + header = i + break + } + } + if header == -1 { + return -1, -1, false + } + end = len(lines) + for i := header + 1; i < len(lines); i++ { + if tableHeaderRe.MatchString(lines[i]) { + end = i + break + } + } + return header, end, true +} + +// mergeRequiresPython replaces the value of requires-python within [project], preserving +// the line's leading whitespace. If the key is absent, it is inserted directly under the +// [project] header. Returns whether the line slice changed. +func mergeRequiresPython(lines []string, value string) ([]string, bool) { + header, end, found := tableBounds(lines, "[project]") + if !found { + return lines, false + } + + want := func(indent string) string { + return fmt.Sprintf(`%srequires-python = "%s"`, indent, value) + } + + for i := header + 1; i < end; i++ { + m := requiresPythonRe.FindStringSubmatch(lines[i]) + if m == nil { + continue + } + replacement := want(m[1]) + if lines[i] == replacement { + return lines, false + } + lines[i] = replacement + return lines, true + } + + // Key absent: insert directly under the [project] header. + inserted := make([]string, 0, len(lines)+1) + inserted = append(inserted, lines[:header+1]...) + inserted = append(inserted, want("")) + inserted = append(inserted, lines[header+1:]...) + return inserted, true +} + +// dbconnectLineRe captures, for a line holding a databricks-connect dependency element: +// (1) the leading whitespace, and (3) any trailing comma (with optional trailing space), +// so that indentation and comma style are preserved when the quoted token is replaced. +var dbconnectLineRe = regexp.MustCompile(`^(\s*)"databricks-connect[^"]*"(\s*,?\s*)$`) + +// mergeDatabricksConnect replaces the databricks-connect element inside +// [dependency-groups].dev. It handles both the multi-line array form (one element per +// line) and the single-line array form (dev = ["databricks-connect~=..."]). +// An empty value (constraints-only mode) is a no-op: the user's dev group is left +// untouched rather than having its databricks-connect pin blanked out. +func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { + if value == "" { + return lines, false + } + header, end, found := tableBounds(lines, "[dependency-groups]") + if !found { + return lines, false + } + + for i := header + 1; i < end; i++ { + // Multi-line element form: a standalone line holding only the quoted token. + if m := dbconnectLineRe.FindStringSubmatch(lines[i]); m != nil { + replacement := fmt.Sprintf(`%s"%s"%s`, m[1], value, m[2]) + if lines[i] == replacement { + return lines, false + } + lines[i] = replacement + return lines, true + } + // Single-line array form: replace the quoted databricks-connect token in place. + if strings.Contains(lines[i], `"databricks-connect`) { + replaced := dbconnectTokenRe.ReplaceAllString(lines[i], `"`+value+`"`) + if replaced == lines[i] { + return lines, false + } + lines[i] = replaced + return lines, true + } + } + return lines, false +} + +// dbconnectTokenRe matches a quoted databricks-connect element anywhere in a line, used +// for the single-line array form. +var dbconnectTokenRe = regexp.MustCompile(`"databricks-connect[^"]*"`) + +// mergeToolUv rewrites the managed [tool.uv] constraint-dependencies block. If a +// marker-bracketed block already exists, its contents are replaced in place. Otherwise any +// plain [tool.uv] table is removed and a fresh marker-bracketed block is appended at EOF. +func mergeToolUv(lines, deps []string) ([]string, bool) { + start, stop, found := markerBounds(lines) + if found { + // Replace the existing managed region in place. Whether it owns a [tool.uv] + // header depends on whether it sits inside a user-authored [tool.uv] table: + // a header-less region attached to the user table stays header-less on + // re-merge, so idempotency holds. + block := renderToolUvBlock(deps, !markerAttachedToToolUv(lines, start)) + existing := lines[start : stop+1] + if equalLines(existing, block) { + return lines, false + } + out := make([]string, 0, len(lines)-(stop-start+1)+len(block)) + out = append(out, lines[:start]...) + out = append(out, block...) + out = append(out, lines[stop+1:]...) + return out, true + } + + // No managed block yet: reconcile any plain [tool.uv] table. + if header, end, ok := tableBounds(lines, "[tool.uv]"); ok { + if toolUvHasOnlyConstraintDeps(lines, header, end) { + // The table is effectively ours (only constraint-dependencies, from a + // pre-marker run): drop it whole and append a fresh standalone block. + out := make([]string, 0, len(lines)) + out = append(out, lines[:header]...) + out = append(out, lines[end:]...) + return appendManagedBlock(out, renderToolUvBlock(deps, true)), true + } + // The table holds user-authored keys: strip our stale constraint-dependencies, + // then insert a header-less managed block INSIDE the existing table. Emitting a + // second "[tool.uv]" header (as a standalone block would) is invalid TOML — + // toml.Decode and uv both reject a table defined twice. + lines = removeConstraintDeps(lines, header, end) + header, end, _ = tableBounds(lines, "[tool.uv]") + // Insert after the last non-blank line of the table body so the managed block + // stays under [tool.uv] and any blank line separating the next table is kept. + insertAt := end + for insertAt > header+1 && strings.TrimSpace(lines[insertAt-1]) == "" { + insertAt-- + } + block := renderToolUvBlock(deps, false) + out := make([]string, 0, len(lines)+len(block)) + out = append(out, lines[:insertAt]...) + out = append(out, block...) + out = append(out, lines[insertAt:]...) + return out, true + } + + // No [tool.uv] at all: append a fresh standalone managed block at EOF. + return appendManagedBlock(lines, renderToolUvBlock(deps, true)), true +} + +// markerAttachedToToolUv reports whether the managed marker region beginning at +// index start sits inside an existing [tool.uv] table — i.e. the nearest table +// header above it is [tool.uv]. When true, the managed block must omit its own +// [tool.uv] header, because a second header for the same table is invalid TOML. +func markerAttachedToToolUv(lines []string, start int) bool { + for i := start - 1; i >= 0; i-- { + if tableHeaderRe.MatchString(lines[i]) { + return strings.TrimSpace(lines[i]) == "[tool.uv]" + } + } + return false +} + +// constraintDepsRe matches the start of a constraint-dependencies assignment within a +// [tool.uv] table, capturing its leading whitespace. +var constraintDepsRe = regexp.MustCompile(`^\s*constraint-dependencies\s*=`) + +// bracketDepthDelta returns the net change in "[" nesting contributed by line. +// It scans outside TOML strings and stops at an unquoted "#" comment, so a "]" +// inside a string element (e.g. "requests[security]==2.0") or a trailing comment +// does not affect the count. It underpins single- vs multi-line array detection; +// testing strings.Contains(line, "]") instead misreads such lines and corrupts +// the merge. +func bracketDepthDelta(line string) int { + delta := 0 + var quote byte // 0 = outside a string; otherwise the opening quote rune + for i := 0; i < len(line); i++ { + c := line[i] + if quote != 0 { + if c == '\\' && quote == '"' { + i++ // skip the escaped char inside a basic string + continue + } + if c == quote { + quote = 0 + } + continue + } + switch c { + case '"', '\'': + quote = c + case '#': + return delta // comment tail: ignore the rest of the line + case '[': + delta++ + case ']': + delta-- + } + } + return delta +} + +// constraintDepsArrayEndDelta reports whether the constraint-dependencies array +// opening at lines[start] spans multiple lines, and if so the index of its last +// line. A single-line array (brackets balanced on the opening line) returns +// (start, false). +func constraintDepsArrayEnd(lines []string, start, limit int) (last int, multiline bool) { + depth := bracketDepthDelta(lines[start]) + if depth <= 0 { + return start, false + } + for j := start + 1; j < limit; j++ { + depth += bracketDepthDelta(lines[j]) + if depth <= 0 { + return j, true + } + } + return limit - 1, true +} + +// toolUvHasOnlyConstraintDeps reports whether the [tool.uv] table body spanning +// (header, end) contains no meaningful key other than constraint-dependencies. Blank lines +// and comment-only lines are ignored when deciding "only". +func toolUvHasOnlyConstraintDeps(lines []string, header, end int) bool { + for i := header + 1; i < end; i++ { + trimmed := strings.TrimSpace(lines[i]) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + if !constraintDepsRe.MatchString(lines[i]) { + return false + } + // Skip the continuation lines of a multi-line array so the whole managed + // key counts as ignorable (mirrors removeConstraintDeps). + last, multiline := constraintDepsArrayEnd(lines, i, end) + if multiline { + i = last + } + } + return true +} + +// removeConstraintDeps strips a constraint-dependencies key from the [tool.uv] table body +// spanning (header, end), leaving the table header and all other user keys in place. It +// handles both the single-line array form and the multi-line array form (the value spans +// several lines until the array's brackets balance). +func removeConstraintDeps(lines []string, header, end int) []string { + for i := header + 1; i < end; i++ { + if !constraintDepsRe.MatchString(lines[i]) { + continue + } + last, _ := constraintDepsArrayEnd(lines, i, end) + end2 := last + 1 + out := make([]string, 0, len(lines)-(end2-i)) + out = append(out, lines[:i]...) + out = append(out, lines[end2:]...) + return out + } + return lines +} + +// markerBounds returns the indices of the managed marker start and end lines, if present. +func markerBounds(lines []string) (start, stop int, found bool) { + start, stop = -1, -1 + for i, line := range lines { + if strings.TrimSpace(line) == managedMarkerStart { + start = i + break + } + } + if start == -1 { + return -1, -1, false + } + for i := start + 1; i < len(lines); i++ { + if strings.TrimSpace(lines[i]) == managedMarkerEnd { + stop = i + break + } + } + if stop == -1 { + return -1, -1, false + } + return start, stop, true +} + +// renderToolUvBlock builds the marker-bracketed managed block lines (no surrounding +// blank lines). When withHeader is true it emits its own "[tool.uv]" table header +// (standalone block appended at EOF); when false it omits the header so the block can +// be nested inside a user-authored [tool.uv] table without defining the table twice. +func renderToolUvBlock(deps []string, withHeader bool) []string { + block := []string{managedMarkerStart} + if withHeader { + block = append(block, "[tool.uv]") + } + block = append(block, "constraint-dependencies = [") + for _, d := range deps { + block = append(block, fmt.Sprintf(" %q,", d)) + } + block = append(block, "]", managedMarkerEnd) + return block +} + +// appendManagedBlock appends block to lines, ensuring exactly one blank line separates it +// from prior content and the file ends with a single trailing newline. +func appendManagedBlock(lines, block []string) []string { + // strings.Split on a trailing "\n" leaves a final empty element; drop trailing empty + // lines so we control the spacing precisely. + for len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + + out := make([]string, 0, len(lines)+len(block)+2) + out = append(out, lines...) + if len(out) > 0 { + out = append(out, "") // exactly one blank line before the managed block + } + out = append(out, block...) + out = append(out, "") // trailing newline after final join + return out +} + +// equalLines reports whether two line slices are identical. +func equalLines(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// freshProjectVersion is the placeholder version written into a greenfield +// [project] table. uv rejects a [project] table that has neither project.version +// nor project.dynamic containing "version", even for a non-distributed local +// environment, so a concrete value is required for `uv sync` to succeed. +const freshProjectVersion = "0.0.0" + +// RenderFreshPyproject produces a complete managed pyproject.toml for a project that has +// none, with [project], [dependency-groups].dev (carrying the databricks-connect pin), and +// the marker-bracketed [tool.uv] constraint block. When c.DatabricksConnect is empty +// (constraints-only mode) the dev group is emitted empty rather than with a blank entry. +func RenderFreshPyproject(projectName string, c Constraints) []byte { + var b strings.Builder + b.WriteString("[project]\n") + fmt.Fprintf(&b, "name = %q\n", projectName) + // uv requires project.version when a [project] table is present. + fmt.Fprintf(&b, "version = %q\n", freshProjectVersion) + fmt.Fprintf(&b, "requires-python = %q\n", c.RequiresPython) + b.WriteString("\n") + b.WriteString("[dependency-groups]\n") + if c.DatabricksConnect != "" { + b.WriteString("dev = [\n") + fmt.Fprintf(&b, " %q,\n", c.DatabricksConnect) + b.WriteString("]\n") + } else { + b.WriteString("dev = []\n") + } + b.WriteString("\n") + for _, line := range renderToolUvBlock(c.ConstraintDeps, true) { + b.WriteString(line) + b.WriteString("\n") + } + return []byte(b.String()) +} diff --git a/libs/localenv/merge_test.go b/libs/localenv/merge_test.go new file mode 100644 index 00000000000..ea273b68eb9 --- /dev/null +++ b/libs/localenv/merge_test.go @@ -0,0 +1,322 @@ +package localenv + +import ( + "strings" + "testing" + + "github.com/BurntSushi/toml" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// requireValidTOML fails the test if b is not parseable TOML. String-only +// assertions miss structural corruption such as a table header emitted twice +// ("Key 'tool.uv' has already been defined"), which uv also rejects on sync. +func requireValidTOML(t *testing.T, b []byte) { + t.Helper() + var v map[string]any + _, err := toml.Decode(string(b), &v) + require.NoError(t, err, "merged output must be valid TOML:\n%s", b) +} + +func testConstraints() Constraints { + return Constraints{ + RequiresPython: "==3.12.*", + DatabricksConnect: "databricks-connect~=17.2.0", + ConstraintDeps: []string{"pydantic~=2.10.6", "anyio~=4.6.2"}, + } +} + +func TestMergeReplacesRequiresPythonPreservingComments(t *testing.T) { + in := []byte(`[project] +name = "demo" +# keep this comment +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "databricks-connect~=16.0.0", + "pytest~=8.0", +] +`) + out, regions, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + assert.Contains(t, string(out), `requires-python = "==3.12.*"`) + assert.Contains(t, string(out), "# keep this comment") + assert.Contains(t, string(out), `"databricks-connect~=17.2.0",`) + assert.Contains(t, string(out), `"pytest~=8.0",`) + assert.Contains(t, regions, "requires-python") + assert.Contains(t, regions, "databricks-connect") + assert.Contains(t, regions, "tool.uv.constraint-dependencies") + assert.Contains(t, string(out), "pydantic~=2.10.6") +} + +func TestMergeIsIdempotent(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "databricks-connect~=16.0.0", +] +`) + once, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + twice, _, err := MergeManaged(once, testConstraints()) + require.NoError(t, err) + assert.Equal(t, string(once), string(twice)) +} + +func TestMergeInsertsRequiresPythonWhenMissing(t *testing.T) { + in := []byte(`[project] +name = "demo" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + assert.Contains(t, string(out), `requires-python = "==3.12.*"`) +} + +func TestMergeReplacesExistingManagedToolUvBlock(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +` + managedMarkerStart + ` +[tool.uv] +constraint-dependencies = [ + "stale~=1.0.0", +] +` + managedMarkerEnd + ` +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + assert.NotContains(t, string(out), "stale~=1.0.0") + assert.Contains(t, string(out), "pydantic~=2.10.6") + // Only one managed block remains. + assert.Equal(t, 1, countOccurrences(string(out), managedMarkerStart)) +} + +func TestMergePreservesCRLF(t *testing.T) { + in := []byte("[project]\r\nrequires-python = \">=3.10\"\r\n\r\n[dependency-groups]\r\ndev = [\"databricks-connect~=16.0.0\"]\r\n") + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + assert.Contains(t, string(out), "\r\n") + assert.Contains(t, string(out), `requires-python = "==3.12.*"`) + // Merging the CRLF output again must be byte-identical (idempotent under \r\n). + twice, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, string(out), string(twice)) +} + +func TestMergePreservesUserToolUvKeys(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.uv] +package = true +dev-dependencies = ["ruff"] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, "[tool.uv]") + assert.Contains(t, s, "package = true") + assert.Contains(t, s, `dev-dependencies = ["ruff"]`) + assert.Contains(t, s, managedMarkerStart) + assert.Contains(t, s, "pydantic~=2.10.6") + // The user's keys must live outside the managed marker block. + start := strings.Index(s, managedMarkerStart) + require.GreaterOrEqual(t, start, 0) + assert.NotContains(t, s[start:], "package = true") + assert.NotContains(t, s[start:], `dev-dependencies = ["ruff"]`) + // The result must be valid TOML: the managed constraint-dependencies nests + // inside the user's [tool.uv] rather than emitting a second [tool.uv] header. + requireValidTOML(t, out) + assert.Equal(t, 1, countOccurrences(s, "[tool.uv]")) + // Merge-twice is byte-identical (header-less managed region stays header-less). + twice, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(twice)) + requireValidTOML(t, twice) +} + +func TestMergeStripsStaleConstraintDepsFromUserToolUv(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.uv] +package = true +constraint-dependencies = ["old~=1.0"] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, "package = true") + // The stale constraint must be gone from the user table; the managed block has the new deps. + assert.NotContains(t, s, "old~=1.0") + assert.Contains(t, s, "pydantic~=2.10.6") + // Valid TOML with a single [tool.uv]: the managed deps nest in the user table. + requireValidTOML(t, out) + assert.Equal(t, 1, countOccurrences(s, "[tool.uv]")) + // Merge-twice is byte-identical. + twice, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, string(out), string(twice)) +} + +func TestMergeRemovesOwnedOnlyToolUv(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.uv] +constraint-dependencies = ["old~=1.0"] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.NotContains(t, s, "old~=1.0") + assert.Contains(t, s, "pydantic~=2.10.6") + // The plain table was removed and replaced by exactly one managed block. + assert.Equal(t, 1, countOccurrences(s, "[tool.uv]")) + assert.Equal(t, 1, countOccurrences(s, managedMarkerStart)) + requireValidTOML(t, out) +} + +func TestMergeRemovesOwnedOnlyMultiLineToolUv(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.uv] +constraint-dependencies = [ + "old~=1.0", +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.NotContains(t, s, "old~=1.0") + assert.Contains(t, s, "pydantic~=2.10.6") + // The multi-line owned-only table was removed whole, leaving exactly one + // [tool.uv] (inside the managed block) and no stray empty header. + assert.Equal(t, 1, countOccurrences(s, "[tool.uv]")) + assert.Equal(t, 1, countOccurrences(s, managedMarkerStart)) + requireValidTOML(t, out) + // Merge-twice is byte-identical. + twice, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, string(out), string(twice)) +} + +func TestMergeReplacesSingleLineDevArray(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0", "pytest~=8.0"] +`) + out, regions, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + // Sibling element and single-line array layout are preserved. + assert.Contains(t, string(out), `dev = ["databricks-connect~=17.2.0", "pytest~=8.0"]`) + assert.Contains(t, regions, "databricks-connect") +} + +func TestMergePreservesMultiLineTrailingComma(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "databricks-connect~=16.0.0", +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + // The trailing comma on the managed element is preserved. + assert.Contains(t, string(out), ` "databricks-connect~=17.2.0",`) +} + +func TestRenderFreshPyproject(t *testing.T) { + out := RenderFreshPyproject("demo", testConstraints()) + s := string(out) + assert.Contains(t, s, `name = "demo"`) + assert.Contains(t, s, `requires-python = "==3.12.*"`) + assert.Contains(t, s, `"databricks-connect~=17.2.0",`) + assert.Contains(t, s, managedMarkerStart) + assert.Contains(t, s, managedMarkerEnd) + assert.Contains(t, s, "pydantic~=2.10.6") + // A fresh render is itself a no-op under MergeManaged (already fully managed). + merged, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(merged)) +} + +func TestMergeStripsMultiLineConstraintDepsWithBracketInFirstElement(t *testing.T) { + // The user's stale constraint-dependencies is a multi-line array whose FIRST + // element line contains a "]" inside an extras spec. A naive + // strings.Contains(line, "]") check would misread this as single-line and + // strip only the first element, orphaning the rest and producing invalid TOML. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.uv] +package = true +constraint-dependencies = ["requests[security]~=2.0", + "old-dep~=1.0", +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + // The whole stale array is gone (both the bracket-bearing first element and + // the continuation), replaced by the managed deps. + assert.NotContains(t, s, "requests[security]") + assert.NotContains(t, s, "old-dep~=1.0") + assert.Contains(t, s, "package = true") + assert.Contains(t, s, "pydantic~=2.10.6") + requireValidTOML(t, out) + assert.Equal(t, 1, countOccurrences(s, "[tool.uv]")) + // Idempotent. + twice, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(twice)) +} + +func TestBracketDepthDeltaIgnoresStringsAndComments(t *testing.T) { + cases := map[string]int{ + `constraint-dependencies = [`: 1, // opens, no close + `constraint-dependencies = ["a", "b"]`: 0, // balanced single-line + `constraint-dependencies = ["requests[sec]~=2",`: 1, // ] inside string ignored + ` "old~=1.0",`: 0, // element line + `]`: -1, // close + `] # trailing note ]`: -1, // ] in comment ignored + `constraint-dependencies = ["x"] # [note]`: 0, // comment ] ignored + } + for line, want := range cases { + assert.Equal(t, want, bracketDepthDelta(line), "line: %q", line) + } +} + +func countOccurrences(s, substr string) int { + return strings.Count(s, substr) +} From babbd628cde70192daf6a139710b6e5fe0751f87 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 19:54:59 +0200 Subject: [PATCH 17/25] Scope pyproject merge to the dev group and preserve inline comments Review of the merge layer found the databricks-connect rewrite was not scoped to [dependency-groups].dev: - It walked every line of [dependency-groups] and rewrote the first databricks-connect element found, so a pin in a sibling group (docs/test) was clobbered instead of the dev entry. It now locates the dev assignment and edits only within that array's line span. - The single-line branch replaced the databricks-connect token anywhere on the dev line, including inside a trailing comment (user content). Replacement is now confined to the array portion (through its closing "]"); the trailing comment is preserved byte-for-byte. - mergeRequiresPython replaced the whole line, dropping an inline comment such as `requires-python = ">=3.10" # maintained by platform team`. It now reattaches the trailing comment, honoring the byte-preservation contract for everything outside the managed value. Adds tests for each: sibling-group untouched, comment not clobbered, inline comment preserved. Co-authored-by: Isaac --- libs/localenv/merge.go | 150 ++++++++++++++++++++++++++++++++---- libs/localenv/merge_test.go | 52 +++++++++++++ 2 files changed, 187 insertions(+), 15 deletions(-) diff --git a/libs/localenv/merge.go b/libs/localenv/merge.go index ca873b5deb0..965e73a788b 100644 --- a/libs/localenv/merge.go +++ b/libs/localenv/merge.go @@ -99,8 +99,8 @@ func mergeRequiresPython(lines []string, value string) ([]string, bool) { return lines, false } - want := func(indent string) string { - return fmt.Sprintf(`%srequires-python = "%s"`, indent, value) + want := func(indent, comment string) string { + return fmt.Sprintf(`%srequires-python = "%s"%s`, indent, value, comment) } for i := header + 1; i < end; i++ { @@ -108,7 +108,9 @@ func mergeRequiresPython(lines []string, value string) ([]string, bool) { if m == nil { continue } - replacement := want(m[1]) + // Preserve a trailing inline comment; only the value is managed, so + // "requires-python = \"...\" # note" keeps its note. + replacement := want(m[1], trailingComment(lines[i])) if lines[i] == replacement { return lines, false } @@ -119,21 +121,63 @@ func mergeRequiresPython(lines []string, value string) ([]string, bool) { // Key absent: insert directly under the [project] header. inserted := make([]string, 0, len(lines)+1) inserted = append(inserted, lines[:header+1]...) - inserted = append(inserted, want("")) + inserted = append(inserted, want("", "")) inserted = append(inserted, lines[header+1:]...) return inserted, true } +// trailingComment returns the inline TOML comment suffix of a line (including the +// leading whitespace and "#"), or "" if there is none. It ignores "#" characters +// inside a quoted string so a value like requires-python = ">=3.10 # x" is not +// mistaken for a comment. +func trailingComment(line string) string { + var quote byte + for i := 0; i < len(line); i++ { + c := line[i] + if quote != 0 { + if c == '\\' && quote == '"' { + i++ + continue + } + if c == quote { + quote = 0 + } + continue + } + switch c { + case '"', '\'': + quote = c + case '#': + // Include any whitespace immediately preceding the "#". + start := i + for start > 0 && (line[start-1] == ' ' || line[start-1] == '\t') { + start-- + } + return line[start:] + } + } + return "" +} + // dbconnectLineRe captures, for a line holding a databricks-connect dependency element: // (1) the leading whitespace, and (3) any trailing comma (with optional trailing space), // so that indentation and comma style are preserved when the quoted token is replaced. var dbconnectLineRe = regexp.MustCompile(`^(\s*)"databricks-connect[^"]*"(\s*,?\s*)$`) +// devKeyRe matches the start of the dev array assignment within [dependency-groups] +// (e.g. "dev = [" or "dev=["), capturing leading whitespace. Only this key is +// managed; sibling groups such as test/docs are user-owned and left untouched. +var devKeyRe = regexp.MustCompile(`^\s*dev\s*=`) + // mergeDatabricksConnect replaces the databricks-connect element inside -// [dependency-groups].dev. It handles both the multi-line array form (one element per -// line) and the single-line array form (dev = ["databricks-connect~=..."]). +// [dependency-groups].dev only. It handles both the multi-line array form (one +// element per line) and the single-line array form (dev = ["databricks-connect~=..."]). // An empty value (constraints-only mode) is a no-op: the user's dev group is left // untouched rather than having its databricks-connect pin blanked out. +// +// The rewrite is scoped to the dev array's own span (found via bracket depth), so a +// databricks-connect pin sitting in a sibling group (e.g. docs/test) or inside a +// trailing comment on some other line is never clobbered. func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { if value == "" { return lines, false @@ -143,8 +187,38 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { return lines, false } + // Locate the dev assignment and the line span of its array value. + devStart := -1 for i := header + 1; i < end; i++ { - // Multi-line element form: a standalone line holding only the quoted token. + if devKeyRe.MatchString(lines[i]) { + devStart = i + break + } + } + if devStart == -1 { + return lines, false + } + arrayLast, _ := arrayLineSpan(lines, devStart, end) + + // Single-line form: the whole array is on the dev line itself. Only rewrite + // within the array (through its closing "]"); a trailing comment after it is + // user content and must be left byte-for-byte intact. + if devStart == arrayLast { + line := lines[devStart] + arrayPart, commentPart := splitAtArrayClose(line) + if !strings.Contains(arrayPart, `"databricks-connect`) { + return lines, false + } + replaced := dbconnectTokenRe.ReplaceAllString(arrayPart, `"`+value+`"`) + commentPart + if replaced == line { + return lines, false + } + lines[devStart] = replaced + return lines, true + } + + // Multi-line form: one element per line, between the dev line and the closing "]". + for i := devStart + 1; i <= arrayLast; i++ { if m := dbconnectLineRe.FindStringSubmatch(lines[i]); m != nil { replacement := fmt.Sprintf(`%s"%s"%s`, m[1], value, m[2]) if lines[i] == replacement { @@ -153,17 +227,63 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { lines[i] = replacement return lines, true } - // Single-line array form: replace the quoted databricks-connect token in place. - if strings.Contains(lines[i], `"databricks-connect`) { - replaced := dbconnectTokenRe.ReplaceAllString(lines[i], `"`+value+`"`) - if replaced == lines[i] { - return lines, false + } + return lines, false +} + +// splitAtArrayClose splits a single-line array assignment into the part up to and +// including the array's closing "]" and the remainder (a trailing comment, if any), +// tracking bracket depth outside quoted strings. This keeps token replacement inside +// the array from touching a trailing comment. If no balanced close is found the whole +// line is returned as the array part. +func splitAtArrayClose(line string) (arrayPart, rest string) { + depth := 0 + var quote byte + opened := false + for i := 0; i < len(line); i++ { + c := line[i] + if quote != 0 { + if c == '\\' && quote == '"' { + i++ + continue + } + if c == quote { + quote = 0 + } + continue + } + switch c { + case '"', '\'': + quote = c + case '[': + depth++ + opened = true + case ']': + depth-- + if opened && depth == 0 { + return line[:i+1], line[i+1:] } - lines[i] = replaced - return lines, true } } - return lines, false + return line, "" +} + +// arrayLineSpan returns the index of the line on which the array opened at +// lines[start] closes (brackets balance), scanning outside strings/comments. A +// single-line array returns start. It bounds in-place edits of an array value to +// the array's own lines. +func arrayLineSpan(lines []string, start, limit int) (last int, multiline bool) { + depth := bracketDepthDelta(lines[start]) + if depth <= 0 { + return start, false + } + for j := start + 1; j < limit; j++ { + depth += bracketDepthDelta(lines[j]) + if depth <= 0 { + return j, true + } + } + return limit - 1, true } // dbconnectTokenRe matches a quoted databricks-connect element anywhere in a line, used diff --git a/libs/localenv/merge_test.go b/libs/localenv/merge_test.go index ea273b68eb9..81997245d62 100644 --- a/libs/localenv/merge_test.go +++ b/libs/localenv/merge_test.go @@ -317,6 +317,58 @@ func TestBracketDepthDeltaIgnoresStringsAndComments(t *testing.T) { } } +func TestMergeDatabricksConnectOnlyTouchesDevGroup(t *testing.T) { + // A databricks-connect pin in a sibling group (docs) must be left alone; only + // the dev group's entry is managed. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +docs = ["databricks-connect~=14.3"] +dev = [ + "databricks-connect~=16.0.0", +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + // docs untouched; dev updated to the managed pin. + assert.Contains(t, s, `docs = ["databricks-connect~=14.3"]`) + assert.Contains(t, s, `"databricks-connect~=17.2.0",`) + requireValidTOML(t, out) +} + +func TestMergeDatabricksConnectDoesNotClobberComment(t *testing.T) { + // A databricks-connect token inside a trailing comment is user content and + // must not be rewritten. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["pytest"] # keep "databricks-connect~=14.3" for docs +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + // The comment's databricks-connect~=14.3 is preserved verbatim. + assert.Contains(t, s, `# keep "databricks-connect~=14.3" for docs`) + requireValidTOML(t, out) +} + +func TestMergeRequiresPythonPreservesInlineComment(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" # maintained by platform team + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, `requires-python = "==3.12.*" # maintained by platform team`) + requireValidTOML(t, out) +} + func countOccurrences(s, substr string) int { return strings.Count(s, substr) } From 6480f0140d7c9234289573ac703a8c3c6b435a9d Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 20:14:44 +0200 Subject: [PATCH 18/25] Handle inline comments on TOML table headers in the pyproject merge Round-2 review found the merge did not tolerate a trailing comment on a table header line (e.g. "[project] # note"). Two consequences: mergeRequiresPython could not find a commented [project] header (managed value silently not updated), and worse, the [dependency-groups] end bound could run past a commented sibling header, so a dev key in a following table was mistaken for [dependency-groups].dev and rewritten. tableHeaderRe now allows a trailing comment and a new headerName helper matches a header by its bracketed name ignoring the comment; both the table lookup and the [tool.uv] attachment check use it. Also documents that line endings are a whole-file property (a CRLF-anywhere file is emitted entirely as CRLF), which is faithful for real single-ending pyproject.toml files. Co-authored-by: Isaac --- libs/localenv/merge.go | 34 ++++++++++++++++++++++++++++------ libs/localenv/merge_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/libs/localenv/merge.go b/libs/localenv/merge.go index 965e73a788b..6cbd463965d 100644 --- a/libs/localenv/merge.go +++ b/libs/localenv/merge.go @@ -22,8 +22,11 @@ const ( ) var ( - // tableHeaderRe matches a TOML table header line such as "[project]" or "[tool.uv]". - tableHeaderRe = regexp.MustCompile(`^\s*\[[^\]]+\]\s*$`) + // tableHeaderRe matches a TOML table header line such as "[project]" or + // "[tool.uv]", tolerating a trailing inline comment ("[project] # note"). Not + // tolerating the comment would both miss a commented [project] header and let + // a table's end bound run past a commented sibling header into the next table. + tableHeaderRe = regexp.MustCompile(`^\s*\[[^\]]+\]\s*(#.*)?$`) // requiresPythonRe captures the leading whitespace of a requires-python assignment so it // can be preserved when the value is replaced. requiresPythonRe = regexp.MustCompile(`^(\s*)requires-python\s*=`) @@ -36,7 +39,12 @@ var ( func MergeManaged(target []byte, c Constraints) (merged []byte, regions []string, err error) { s := string(target) - // Detect and normalize line endings. We process on "\n" and restore "\r\n" on exit. + // Detect and normalize line endings. We process on "\n" and restore "\r\n" on + // exit. Line endings are treated as a whole-file property: a file that uses + // CRLF anywhere is emitted entirely as CRLF. Real pyproject.toml files use one + // consistent ending, so this is faithful in practice; a file that deliberately + // mixes LF and CRLF would be normalized to CRLF (a benign whitespace-only + // change), which we accept rather than track a terminator per line. crlf := strings.Contains(s, "\r\n") if crlf { s = strings.ReplaceAll(s, "\r\n", "\n") @@ -66,13 +74,27 @@ func MergeManaged(target []byte, c Constraints) (merged []byte, regions []string return []byte(out), regions, nil } +// headerName returns the bracketed table name of a TOML table-header line (e.g. +// "[project]" from " [project] # note"), or "" if the line is not a table header. +// It strips a trailing inline comment so a commented header still matches by name. +func headerName(line string) string { + if !tableHeaderRe.MatchString(line) { + return "" + } + s := strings.TrimSpace(line) + if i := strings.Index(s, "]"); i >= 0 { + return s[:i+1] + } + return s +} + // tableBounds returns the line index of the header matching name (e.g. "[project]") and // the index of the first line after the table body (the next table header or EOF). If the // table is absent, found is false. func tableBounds(lines []string, name string) (header, end int, found bool) { header = -1 for i, line := range lines { - if strings.TrimSpace(line) == name { + if headerName(line) == name { header = i break } @@ -352,8 +374,8 @@ func mergeToolUv(lines, deps []string) ([]string, bool) { // [tool.uv] header, because a second header for the same table is invalid TOML. func markerAttachedToToolUv(lines []string, start int) bool { for i := start - 1; i >= 0; i-- { - if tableHeaderRe.MatchString(lines[i]) { - return strings.TrimSpace(lines[i]) == "[tool.uv]" + if name := headerName(lines[i]); name != "" { + return name == "[tool.uv]" } } return false diff --git a/libs/localenv/merge_test.go b/libs/localenv/merge_test.go index 81997245d62..b8b7602361b 100644 --- a/libs/localenv/merge_test.go +++ b/libs/localenv/merge_test.go @@ -369,6 +369,39 @@ dev = ["databricks-connect~=16.0.0"] requireValidTOML(t, out) } +func TestMergeHandlesTableHeaderInlineComments(t *testing.T) { + // Table headers may carry a trailing comment. requires-python under a + // commented [project] must still be updated, and the [dependency-groups] end + // bound must not run past a commented sibling header into another table's dev. + in := []byte(`[project] # package metadata +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.custom] # user table +dev = ["databricks-connect==1.0.0"] # must not be managed +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + // [project].requires-python was found and updated despite the header comment. + assert.Contains(t, s, `requires-python = "==3.12.*"`) + // The real dev group was updated... + assert.Contains(t, s, `"databricks-connect~=17.2.0"`) + // ...but the lookalike dev under [tool.custom] was left untouched. + assert.Contains(t, s, `dev = ["databricks-connect==1.0.0"] # must not be managed`) + requireValidTOML(t, out) +} + +func TestHeaderName(t *testing.T) { + assert.Equal(t, "[project]", headerName("[project]")) + assert.Equal(t, "[project]", headerName(" [project] # note")) + assert.Equal(t, "[tool.uv]", headerName("[tool.uv]#x")) + assert.Empty(t, headerName("requires-python = \"3.12\"")) + assert.Empty(t, headerName("dev = [\"a\"]")) +} + func countOccurrences(s, substr string) int { return strings.Count(s, substr) } From 5f9cbb28b9dd87aafd3c44154602d63f5f2fb8f6 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 20:26:41 +0200 Subject: [PATCH 19/25] Recognize array-of-tables headers so [tool.uv] bounds stop at its children Round-3 review found tableHeaderRe did not match TOML array-of-tables headers like "[[tool.uv.index]]". A [tool.uv] table's end bound therefore ran through its [[tool.uv.index]] children, and the header-less managed constraint block could be inserted inside the last index item instead of under [tool.uv], producing wrong or invalid uv config. tableHeaderRe now matches both "[...]" and "[[...]]"; headerName returns the full "[[...]]" token so an array-of-tables header is never treated as the same table as its "[...]" parent. Adds a merge test with a [[tool.uv.index]] child. Co-authored-by: Isaac --- libs/localenv/merge.go | 25 ++++++++++++++++++------- libs/localenv/merge_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/libs/localenv/merge.go b/libs/localenv/merge.go index 6cbd463965d..9de5f8358df 100644 --- a/libs/localenv/merge.go +++ b/libs/localenv/merge.go @@ -22,11 +22,13 @@ const ( ) var ( - // tableHeaderRe matches a TOML table header line such as "[project]" or - // "[tool.uv]", tolerating a trailing inline comment ("[project] # note"). Not - // tolerating the comment would both miss a commented [project] header and let - // a table's end bound run past a commented sibling header into the next table. - tableHeaderRe = regexp.MustCompile(`^\s*\[[^\]]+\]\s*(#.*)?$`) + // tableHeaderRe matches a TOML table header line: a standard table like + // "[project]" / "[tool.uv]" or an array-of-tables like "[[tool.uv.index]]", + // tolerating a trailing inline comment ("[project] # note"). Recognizing both + // forms matters for bounds: an unrecognized "[[...]]" header would let a + // table's end run past it (e.g. [tool.uv] swallowing its child [[tool.uv.index]] + // items), and a commented header would similarly be missed. + tableHeaderRe = regexp.MustCompile(`^\s*\[\[?[^\]]+\]\]?\s*(#.*)?$`) // requiresPythonRe captures the leading whitespace of a requires-python assignment so it // can be preserved when the value is replaced. requiresPythonRe = regexp.MustCompile(`^(\s*)requires-python\s*=`) @@ -75,13 +77,22 @@ func MergeManaged(target []byte, c Constraints) (merged []byte, regions []string } // headerName returns the bracketed table name of a TOML table-header line (e.g. -// "[project]" from " [project] # note"), or "" if the line is not a table header. -// It strips a trailing inline comment so a commented header still matches by name. +// "[project]" from " [project] # note", or "[[tool.uv.index]]" from an +// array-of-tables header), or "" if the line is not a table header. It strips a +// trailing inline comment so a commented header still matches by name, and keeps +// the full "[[...]]" form so an array-of-tables header is never treated as the +// same table as its "[...]" parent. func headerName(line string) string { if !tableHeaderRe.MatchString(line) { return "" } s := strings.TrimSpace(line) + // Array-of-tables: "[[name]]" — return through the closing "]]". + if strings.HasPrefix(s, "[[") { + if i := strings.Index(s, "]]"); i >= 0 { + return s[:i+2] + } + } if i := strings.Index(s, "]"); i >= 0 { return s[:i+1] } diff --git a/libs/localenv/merge_test.go b/libs/localenv/merge_test.go index b8b7602361b..f748934165c 100644 --- a/libs/localenv/merge_test.go +++ b/libs/localenv/merge_test.go @@ -398,10 +398,43 @@ func TestHeaderName(t *testing.T) { assert.Equal(t, "[project]", headerName("[project]")) assert.Equal(t, "[project]", headerName(" [project] # note")) assert.Equal(t, "[tool.uv]", headerName("[tool.uv]#x")) + // Array-of-tables headers are distinct from their parent table. + assert.Equal(t, "[[tool.uv.index]]", headerName("[[tool.uv.index]]")) + assert.Equal(t, "[[tool.uv.index]]", headerName(" [[tool.uv.index]] # note")) assert.Empty(t, headerName("requires-python = \"3.12\"")) assert.Empty(t, headerName("dev = [\"a\"]")) } +func TestMergeToolUvWithArrayOfTablesChild(t *testing.T) { + // A [tool.uv] table followed by its [[tool.uv.index]] array-of-tables child: + // the managed constraint block must attach to [tool.uv], not leak into the + // index item, and the result must be valid TOML. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.uv] + +[[tool.uv.index]] +name = "internal" +url = "https://packages.example/simple" +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + requireValidTOML(t, out) + // The index array-of-tables is preserved intact. + assert.Contains(t, s, `[[tool.uv.index]]`) + assert.Contains(t, s, `name = "internal"`) + assert.Contains(t, s, "pydantic~=2.10.6") + // Merge-twice is byte-identical. + twice, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(twice)) +} + func countOccurrences(s, substr string) int { return strings.Count(s, substr) } From 41d052a69727acb3343cfe75358ce344134fb314 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Mon, 6 Jul 2026 13:45:10 +0200 Subject: [PATCH 20/25] Address review: refuse to merge multi-line-string / no-[project] pyproject files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness gaps in the line-based merge, from review: - The scanner does not track TOML multi-line string state ("""...""" / '''...''') across lines, so a line inside such a string that looks like a table header, key, or bracket could mis-scope the managed-region edits and silently corrupt the file. MergeManaged now detects a multi-line string delimiter and returns an error (surfaced as E_MERGE) rather than risking corruption — the guarantee the merge exists to uphold. Multi-line strings are rare in a pyproject.toml. - A partial existing file with no [project] table would be "merged" with requires-python silently skipped. MergeManaged now errors when [project] is absent (greenfield goes through RenderFreshPyproject, which always writes it). Adds tests for both bail-outs. Co-authored-by: Isaac --- libs/localenv/merge.go | 72 +++++++++++++++++++++++++++++++++++++ libs/localenv/merge_test.go | 36 +++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/libs/localenv/merge.go b/libs/localenv/merge.go index 9de5f8358df..2dc36a25efc 100644 --- a/libs/localenv/merge.go +++ b/libs/localenv/merge.go @@ -1,11 +1,20 @@ package localenv import ( + "errors" "fmt" "regexp" "strings" ) +// errMultilineString and errNoProjectTable are the conditions under which +// MergeManaged refuses to merge rather than risk corrupting the file. The caller +// surfaces both as E_MERGE. +var ( + errMultilineString = errors.New("pyproject.toml uses a TOML multi-line string, which the formatting-preserving merge cannot safely edit; edit requires-python / [tool.uv] manually") + errNoProjectTable = errors.New("pyproject.toml has no [project] table to hold requires-python") +) + // managedMarkerStart and managedMarkerEnd bracket the region of pyproject.toml that // "databricks local-env python sync" owns. Everything between them is rewritten on each merge; // everything outside is preserved byte-for-byte. @@ -54,6 +63,24 @@ func MergeManaged(target []byte, c Constraints) (merged []byte, regions []string lines := strings.Split(s, "\n") + // The merge is line-based and does not track TOML multi-line string state + // ("""...""" / '''...''') across lines. A line inside such a string can look + // like a table header, a key assignment, or a bracket, which would mis-scope + // the managed-region edits and silently corrupt the file. Rather than risk + // that, bail out: this is the guarantee the merge exists to uphold. Multi-line + // strings are rare in a pyproject.toml, and the caller surfaces this as E_MERGE. + if containsMultilineString(lines) { + return nil, nil, errMultilineString + } + + // requires-python is a managed value; if there is no [project] table to hold + // it, this is not a file we can faithfully merge (greenfield goes through + // RenderFreshPyproject, which always writes [project]). Fail loudly rather + // than silently skip the version pin. + if _, _, ok := tableBounds(lines, "[project]"); !ok { + return nil, nil, errNoProjectTable + } + lines, rpChanged := mergeRequiresPython(lines, c.RequiresPython) if rpChanged { regions = append(regions, regionRequiresPython) @@ -396,6 +423,51 @@ func markerAttachedToToolUv(lines []string, start int) bool { // [tool.uv] table, capturing its leading whitespace. var constraintDepsRe = regexp.MustCompile(`^\s*constraint-dependencies\s*=`) +// containsMultilineString reports whether the input contains a TOML multi-line +// string delimiter (""" or ”'), taking a line-outside-comment view. The +// line-based merge cannot track such a string's body across lines, so its +// presence anywhere is treated as unmergeable rather than risking corruption. +// This is conservative: a single-line """x""" is also refused, but those are +// vanishingly rare in a pyproject.toml and refusing is safe. +func containsMultilineString(lines []string) bool { + for _, line := range lines { + // Ignore a delimiter that appears only within a "#" comment. + if i := commentStart(line); i >= 0 { + line = line[:i] + } + if strings.Contains(line, `"""`) || strings.Contains(line, "'''") { + return true + } + } + return false +} + +// commentStart returns the index of the "#" that begins an inline comment on +// line, or -1 if there is none. A "#" inside a quoted string is not a comment. +func commentStart(line string) int { + var quote byte + for i := 0; i < len(line); i++ { + c := line[i] + if quote != 0 { + if c == '\\' && quote == '"' { + i++ + continue + } + if c == quote { + quote = 0 + } + continue + } + switch c { + case '"', '\'': + quote = c + case '#': + return i + } + } + return -1 +} + // bracketDepthDelta returns the net change in "[" nesting contributed by line. // It scans outside TOML strings and stops at an unquoted "#" comment, so a "]" // inside a string element (e.g. "requests[security]==2.0") or a trailing comment diff --git a/libs/localenv/merge_test.go b/libs/localenv/merge_test.go index f748934165c..6e065d4722f 100644 --- a/libs/localenv/merge_test.go +++ b/libs/localenv/merge_test.go @@ -435,6 +435,42 @@ url = "https://packages.example/simple" assert.Equal(t, s, string(twice)) } +func TestMergeBailsOnMultilineString(t *testing.T) { + // A TOML multi-line string can contain lines that look like headers/keys/ + // brackets; the line-based merge can't track it, so it must refuse rather than + // risk corrupting the file. The body here contains a fake table header and a + // fake constraint-dependencies line. + in := []byte(`[project] +requires-python = ">=3.10" + +[tool.uv] +note = """ +[project] +constraint-dependencies = ["not really"] +""" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] +`) + _, _, err := MergeManaged(in, testConstraints()) + require.Error(t, err) + assert.ErrorIs(t, err, errMultilineString) +} + +func TestMergeBailsWhenNoProjectTable(t *testing.T) { + // A partial existing file with no [project] table can't receive requires-python; + // merging it would silently skip the pin, so it must error instead. + in := []byte(`[dependency-groups] +dev = ["databricks-connect~=16.0.0"] + +[tool.uv] +constraint-dependencies = ["old~=1.0"] +`) + _, _, err := MergeManaged(in, testConstraints()) + require.Error(t, err) + assert.ErrorIs(t, err, errNoProjectTable) +} + func countOccurrences(s, substr string) int { return strings.Count(s, substr) } From 892250fd85dfed555991a07f1680fdd0650213d7 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 15:20:11 +0200 Subject: [PATCH 21/25] Add local-env pipeline, detection, and package-manager interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth in the stacked local-env series. This completes the libs/localenv engine; the uv backend and the CLI command follow in later PRs. - pipeline.go: the six-phase orchestrator (preflight → resolve → fetch → merge → provision → validate) that ties the earlier layers together and records per-phase status into the Result. It drives everything through the PackageManager interface, so it is unit-tested end to end against a fake package manager and a stub compute client. This file also owns the filesystem/artifact constants and the canonical phase-order slice, which the foundation PR intentionally left to their consumer. - pkgmanager.go: the PackageManager interface the pipeline provisions through (implemented by uv in the next PR). - detect.go: package-manager detection (uv vs conda vs pip) and the writability preflight, biased toward uv since uv's native project file is the pyproject.toml this command already merges. A read error on an existing pyproject.toml that is not a not-exist error is treated as a failure rather than as greenfield, so a permission or transient I/O error can never cause the project to be overwritten with a fresh file and no backup. Depends on the foundation, target, constraints, and merge PRs. The package is still dormant: nothing under cmd/ imports it. Co-authored-by: Isaac --- libs/localenv/detect.go | 80 ++++++ libs/localenv/detect_test.go | 45 +++ libs/localenv/pipeline.go | 486 +++++++++++++++++++++++++++++++++ libs/localenv/pipeline_test.go | 369 +++++++++++++++++++++++++ libs/localenv/pkgmanager.go | 31 +++ 5 files changed, 1011 insertions(+) create mode 100644 libs/localenv/detect.go create mode 100644 libs/localenv/detect_test.go create mode 100644 libs/localenv/pipeline.go create mode 100644 libs/localenv/pipeline_test.go create mode 100644 libs/localenv/pkgmanager.go diff --git a/libs/localenv/detect.go b/libs/localenv/detect.go new file mode 100644 index 00000000000..8ccb84d1174 --- /dev/null +++ b/libs/localenv/detect.go @@ -0,0 +1,80 @@ +package localenv + +import ( + "os" + "path/filepath" +) + +// manager identifies the Python package manager a project uses. P0 only +// supports uv; every other value results in a clean E_MANAGER_UNSUPPORTED exit. +type manager string + +const ( + managerUv manager = "uv" + managerConda manager = "conda" + managerPip manager = "pip" +) + +// detectManager inspects projectDir for package-manager markers, only deep +// enough to branch uv vs. not-uv (spec §5). It emits no telemetry (spec §5). +// +// Detection is deliberately biased toward uv, because uv's native project file +// is pyproject.toml (PEP 621) — the same format this command writes and merges: +// - A uv marker (uv.lock or a [tool.uv] table) → uv. +// - A pyproject.toml with no competing marker → uv (a plain PEP 621 project is +// exactly the "existing project merge" case; uv can drive it). +// - conda (environment.yml) or pip (requirements.txt) with no pyproject.toml → +// that manager; automated setup is P1, so the caller exits cleanly. +// - Greenfield (no markers at all) → uv, the manager this command provisions. +// +// A conda/pip marker that sits alongside a pyproject.toml still resolves to uv: +// the project already has the file we drive, so we proceed rather than block. +func detectManager(projectDir string) manager { + // uv markers take precedence: an existing uv project or lockfile. + if fileExists(filepath.Join(projectDir, "uv.lock")) { + return managerUv + } + if fileExists(filepath.Join(projectDir, pyprojectFile)) { + // A pyproject.toml — with or without a [tool.uv] table — is uv-drivable. + return managerUv + } + + // No pyproject.toml: a conda or pip marker means a non-uv project we cannot + // yet automate. conda before pip (environment.yml is the more specific signal). + if fileExists(filepath.Join(projectDir, "environment.yml")) || + fileExists(filepath.Join(projectDir, "environment.yaml")) { + return managerConda + } + if fileExists(filepath.Join(projectDir, "requirements.txt")) { + return managerPip + } + + // Greenfield: nothing to disambiguate; this command provisions uv. + return managerUv +} + +// managerGuidance returns the actionable, non-blaming message shown when a +// non-uv manager is detected (spec §5). +func managerGuidance(m manager) string { + return "detected a " + string(m) + " project; automated setup for " + string(m) + + " is not yet available (P1). Use a uv project (add a pyproject.toml with a [tool.uv] table, or run `uv init`) to provision automatically" +} + +// fileExists reports whether path exists and is a regular file. +func fileExists(path string) bool { + info, err := os.Stat(path) + return err == nil && !info.IsDir() +} + +// ensureWritable verifies the process can create files in dir by creating and +// removing a temporary file. A permission failure is reported so preflight can +// stop before any real write (invariant 1). +func ensureWritable(dir string) error { + f, err := os.CreateTemp(dir, ".localenv-writecheck-*") + if err != nil { + return err + } + name := f.Name() + f.Close() + return os.Remove(name) +} diff --git a/libs/localenv/detect_test.go b/libs/localenv/detect_test.go new file mode 100644 index 00000000000..304a0e8118e --- /dev/null +++ b/libs/localenv/detect_test.go @@ -0,0 +1,45 @@ +package localenv + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDetectManager(t *testing.T) { + write := func(t *testing.T, files ...string) string { + dir := t.TempDir() + for _, f := range files { + require.NoError(t, os.WriteFile(filepath.Join(dir, f), []byte("x"), 0o644)) + } + return dir + } + + cases := []struct { + name string + files []string + want manager + }{ + {"greenfield", nil, managerUv}, + {"uv lock", []string{"uv.lock"}, managerUv}, + {"plain pyproject", []string{"pyproject.toml"}, managerUv}, + {"pyproject wins over requirements", []string{"pyproject.toml", "requirements.txt"}, managerUv}, + {"conda only", []string{"environment.yml"}, managerConda}, + {"conda yaml", []string{"environment.yaml"}, managerConda}, + {"pip only", []string{"requirements.txt"}, managerPip}, + {"conda before pip", []string{"environment.yml", "requirements.txt"}, managerConda}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, detectManager(write(t, tc.files...))) + }) + } +} + +func TestEnsureWritable(t *testing.T) { + assert.NoError(t, ensureWritable(t.TempDir())) + assert.Error(t, ensureWritable(filepath.Join(t.TempDir(), "does-not-exist"))) +} diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go new file mode 100644 index 00000000000..7b7320d4bd7 --- /dev/null +++ b/libs/localenv/pipeline.go @@ -0,0 +1,486 @@ +package localenv + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/databricks/cli/libs/log" + "github.com/hexops/gotextdiff" + "github.com/hexops/gotextdiff/myers" + "github.com/hexops/gotextdiff/span" +) + +// Filenames and directories the pipeline reads and writes. venvDir is also the +// virtualenv directory the package manager provisions. +const ( + pyprojectFile = "pyproject.toml" + backupFile = "pyproject.toml.bak" + venvDir = ".venv" +) + +// artifactSource values reported in --json resolved.artifactSource (spec §6). +const ( + artifactNetwork = "network" + artifactCache = "cache" +) + +// allPhases is the canonical phase order for the --json "phases" array. The +// pipeline seeds every phase pending from this slice and flips each to ok/error +// as the run progresses. +var allPhases = []PhaseName{ + PhasePreflight, + PhaseResolve, + PhaseFetch, + PhaseMerge, + PhaseProvision, + PhaseValidate, +} + +// Pipeline orchestrates the dbconnect sync phases against a project directory. +type Pipeline struct { + Mode Mode + Check bool + ProjectDir string + ConstraintBaseURL string + CacheDir string + Flags TargetFlags + Bundle BundleTarget + Compute ComputeClient + PM PackageManager + + // res accumulates phase statuses and result fields as the run progresses. + res *Result +} + +// Run executes all pipeline phases in order and returns a fully populated Result. +// On a phase error, Result.Error is set and the same error is also returned. The +// Result always carries the full canonical phase list: phases completed before a +// failure are "ok", the failing phase is "error", and the rest are "pending". +func (p *Pipeline) Run(ctx context.Context) (*Result, error) { + log.Debugf(ctx, "local-env: mode=%s check=%v project=%s cacheDir=%s constraintBaseURL=%s flags=%+v", + p.Mode, + p.Check, + filepath.ToSlash(p.ProjectDir), + filepath.ToSlash(p.CacheDir), + p.ConstraintBaseURL, + p.Flags, + ) + + p.res = &Result{ + SchemaVersion: SchemaVersion, + Command: CommandName, + Mode: p.Mode.String(), + DryRun: p.Check, + // Phases start as pending and flip to ok/error as the run progresses. + Phases: initialPhases(), + Warnings: []Warning{}, + } + + if err := p.run(ctx); err != nil { + return p.res, err + } + p.res.OK = true + return p.res, nil +} + +// run drives the phases and returns the first phase error. Result bookkeeping +// (phase status, error object) is handled by fail / markOK. +func (p *Pipeline) run(ctx context.Context) error { + // Phase: preflight — manager detection, writability, package-manager availability. + // P0 supports only uv; any other detected manager is a clean, non-blaming exit. + if m := detectManager(p.ProjectDir); m != managerUv { + return p.fail(PhasePreflight, false, NewError(ErrManagerUnsupported, nil, "%s", managerGuidance(m))) + } + if err := ensureWritable(p.ProjectDir); err != nil { + return p.fail(PhasePreflight, false, NewError(ErrNotWritable, err, "project directory %s is not writable", filepath.ToSlash(p.ProjectDir))) + } + version, err := p.PM.EnsureAvailable(ctx) + if err != nil { + return p.fail(PhasePreflight, false, asPipelineError(err, ErrUvMissing, "%s unavailable", p.PM.Name())) + } + p.markOK(PhasePreflight, p.PM.Name()+" "+version) + + // Phase: resolve — compute target → environment key. + target, err := p.resolve(ctx) + if err != nil { + return err + } + + // Phase: fetch — constraint artifact for the resolved env key. + c, err := p.fetch(ctx, target) + if err != nil { + return err + } + + // Parse the required Python minor from the artifact; a failure here reflects a + // bad artifact, reported at the fetch phase. + pyMinor, err := PythonMinorFromRequires(c.RequiresPython) + if err != nil { + return p.fail(PhaseFetch, false, NewError(ErrFetch, err, "cannot parse python version from constraints %q", c.RequiresPython)) + } + + dbcPin := c.DatabricksConnect + if p.Mode == ModeConstraintsOnly { + // constraints-only omits the databricks-connect dependency entirely. + dbcPin = "" + } + p.res.Resolved = &ResolvedInfo{ + PythonVersion: pyMinor, + DBConnectVersion: versionFromPin(dbcPin), + ArtifactSource: artifactSource(c.FromCache), + } + + // Phase: merge — compute the merged pyproject.toml (in-memory, no writes yet). + mergedBytes, greenfield, err := p.mergePlan(ctx, pyMinor, c, dbcPin) + if err != nil { + return err + } + p.res.Greenfield = greenfield + + // Check mode stops after planning — nothing below mutates disk. + if p.Check { + p.markOK(PhaseMerge, "") + p.markOK(PhaseProvision, "") + p.markOK(PhaseValidate, "") + return nil + } + + // Apply the merged content to disk (backup first for existing projects). + if err := p.applyMerge(ctx, mergedBytes, greenfield); err != nil { + return err + } + p.markOK(PhaseMerge, "") + + // Phase: provision — ensure Python, run uv sync, seed pip. + if err := p.provision(ctx, pyMinor); err != nil { + return err + } + + // Phase: validate — assert the venv matches the target. + return p.validate(ctx, pyMinor, dbcPin) +} + +// resolve runs ResolveTarget and records the resolve phase. +func (p *Pipeline) resolve(ctx context.Context) (*TargetInfo, error) { + target, err := ResolveTarget(ctx, p.Flags, p.Compute, p.Bundle) + if err != nil { + return nil, p.fail(PhaseResolve, false, asPipelineError(err, ErrResolve, "target resolution failed")) + } + p.res.Target = target + p.markOK(PhaseResolve, fmt.Sprintf("source=%s envKey=%s", target.Source, target.EnvKey)) + return target, nil +} + +// fetch fetches constraints for the resolved target and records the fetch phase. +func (p *Pipeline) fetch(ctx context.Context, target *TargetInfo) (*Constraints, error) { + c, err := FetchConstraints(ctx, p.ConstraintBaseURL, target.EnvKey, p.CacheDir) + if err != nil { + // FetchConstraints classifies the cause: E_ENV_UNSUPPORTED for a missing + // key (404) versus E_FETCH for transport failure with no cache. Both are + // discovered here, so both are reported at the fetch phase (the JSON keeps + // the failing phase and error.failurePhase in agreement). + return nil, p.fail(PhaseFetch, false, asPipelineError(err, ErrFetch, "fetch constraints failed")) + } + p.markOK(PhaseFetch, fmt.Sprintf("source=%s fromCache=%v", c.SourceURL, c.FromCache)) + return c, nil +} + +// pyprojectPath returns the path to pyproject.toml in the project directory. +func (p *Pipeline) pyprojectPath() string { + return filepath.Join(p.ProjectDir, pyprojectFile) +} + +// backupPath returns the path to the pyproject.toml backup file. +func (p *Pipeline) backupPath() string { + return filepath.Join(p.ProjectDir, backupFile) +} + +// mergePlan computes the merged pyproject.toml bytes (without writing to disk), +// decides greenfield vs. existing, and builds the Plan (populated only under +// --check). dbcPin is the databricks-connect pin to inject, or "" in +// constraints-only mode. +func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, dbcPin string) (merged []byte, greenfield bool, err error) { + pyproject := p.pyprojectPath() + backup := p.backupPath() + + // Determine base bytes for the merge. For an existing project with a backup, + // the backup is the canonical base so the merge starts from the original + // unmanaged state. + var baseBytes []byte + if data, rerr := os.ReadFile(backup); rerr == nil { + baseBytes = data + } else if !errors.Is(rerr, os.ErrNotExist) { + // A backup that exists but can't be read (permissions, I/O) must not be + // silently ignored: falling through would treat the project as greenfield + // and overwrite it. Fail loudly instead. + return nil, false, p.fail(PhaseMerge, false, NewError(ErrMerge, rerr, "read backup %s failed", filepath.ToSlash(backup))) + } + if baseBytes == nil { + data, rerr := os.ReadFile(pyproject) + switch { + case rerr == nil: + baseBytes = data + case !errors.Is(rerr, os.ErrNotExist): + // Only a genuine not-exist means greenfield. Any other read error on an + // existing pyproject.toml (permission change, transient I/O, delete race + // after detectManager saw it) must not be misread as greenfield — that + // would render a fresh file and overwrite the user's project with no + // backup. Fail instead of destroying unrecoverable state (invariant 2). + return nil, false, p.fail(PhaseMerge, false, NewError(ErrMerge, rerr, "read pyproject.toml %s failed", filepath.ToSlash(pyproject))) + } + } + greenfield = baseBytes == nil + + // The artifact drives the merge; in constraints-only mode we clear the + // databricks-connect pin so it is neither written nor asserted. + effective := *c + effective.DatabricksConnect = dbcPin + + var changedRegions []string + if greenfield { + // No existing pyproject.toml — render a fresh one. The project name comes + // from the directory name as a reasonable default. + projectName := filepath.Base(p.ProjectDir) + merged = RenderFreshPyproject(projectName, effective) + changedRegions = []string{regionRequiresPython, regionToolUv} + if dbcPin != "" { + changedRegions = append(changedRegions, regionDatabricksConnect) + } + } else { + merged, changedRegions, err = MergeManaged(baseBytes, effective) + if err != nil { + return nil, greenfield, p.fail(PhaseMerge, false, NewError(ErrMerge, err, "merge managed regions failed")) + } + } + + // Under --check, build the plan (with a diff) for reporting. A real run does + // not need the diff. + if p.Check { + oldStr := "" + newStr := string(merged) + oldName := pyprojectFile + newName := pyprojectFile + if !greenfield { + oldStr = string(baseBytes) + newName = pyprojectFile + ".new" + } + edits := myers.ComputeEdits(span.URIFromPath(oldName), oldStr, newStr) + diff := fmt.Sprint(gotextdiff.ToUnified(oldName, newName, oldStr, edits)) + + plan := &Plan{ + WouldWrite: filepath.ToSlash(pyproject), + Diff: diff, + ChangedRegions: changedRegions, + WouldInstallPython: pyMinor, + } + if !greenfield { + plan.WouldBackup = filepath.ToSlash(backup) + } + p.res.Plan = plan + } + return merged, greenfield, nil +} + +// applyMerge writes the merged bytes to disk, backing up an existing +// pyproject.toml first. From this point on, disk has been mutated. +func (p *Pipeline) applyMerge(_ context.Context, mergedBytes []byte, greenfield bool) error { + pyproject := p.pyprojectPath() + backup := p.backupPath() + + if !greenfield { + // Back up before modifying so the user's original is recoverable + // (invariant 2). Only create the backup when one does not already exist: + // on a re-run the existing .bak is the canonical original unmanaged state + // (mergePlan used it as the base), so overwriting it with the already-merged + // pyproject.toml would destroy that baseline. + if _, err := os.Stat(backup); err != nil { + if err := copyFile(pyproject, backup); err != nil { + return p.fail(PhaseMerge, false, NewError(ErrMerge, err, "backup pyproject.toml failed")) + } + } + p.res.BackupPath = filepath.ToSlash(backup) + } + + if err := os.WriteFile(pyproject, mergedBytes, 0o644); err != nil { + code := ErrMerge + if greenfield { + code = ErrWrite + } + return p.fail(PhaseMerge, true, NewError(code, err, "write pyproject.toml failed")) + } + return nil +} + +// provision ensures the required Python version is installed, runs uv sync, and +// seeds pip. All three are reported under the provision phase. +func (p *Pipeline) provision(ctx context.Context, pyMinor string) error { + if err := p.PM.EnsurePython(ctx, pyMinor); err != nil { + return p.fail(PhaseProvision, true, asPipelineError(err, ErrPythonInstall, "ensure python %s failed", pyMinor)) + } + if err := p.PM.Provision(ctx, p.ProjectDir); err != nil { + return p.fail(PhaseProvision, true, asPipelineError(err, ErrProvision, "provision failed")) + } + if err := p.PM.PostProvision(ctx, p.ProjectDir); err != nil { + return p.fail(PhaseProvision, true, asPipelineError(err, ErrProvision, "post-provision failed")) + } + p.markOK(PhaseProvision, "") + return nil +} + +// validate reads the Python and databricks-connect versions from the venv and +// populates the venv path. dbcPin is "" in constraints-only mode, where the DB +// Connect assertion is skipped. +func (p *Pipeline) validate(ctx context.Context, expectedPyMinor, dbcPin string) error { + pyVer, dbcVer, err := p.PM.Validate(ctx, p.ProjectDir) + if err != nil { + return p.fail(PhaseValidate, true, asPipelineError(err, ErrValidate, "validation failed")) + } + + // Assert the installed Python minor matches the target. + if pyVer != expectedPyMinor { + return p.fail(PhaseValidate, true, NewError(ErrValidate, nil, + "python version mismatch: want %s, got %s", expectedPyMinor, pyVer)) + } + + // In default mode, assert the installed databricks-connect major matches the + // pin's major. dbcPin is e.g. "databricks-connect~=17.2.0"; dbcVer is "17.2.0". + if dbcPin != "" { + pinMajor := dbcMajorFromPin(dbcPin) + if pinMajor == "" { + return p.fail(PhaseValidate, true, NewError(ErrValidate, nil, + "cannot determine databricks-connect major version from pin %q", dbcPin)) + } + installedMajor := majorVersion(dbcVer) + if installedMajor == "" { + return p.fail(PhaseValidate, true, NewError(ErrValidate, nil, + "cannot determine installed databricks-connect major version from %q", dbcVer)) + } + if pinMajor != installedMajor { + return p.fail(PhaseValidate, true, NewError(ErrValidate, nil, + "databricks-connect major version mismatch: want %s.x, got %s", pinMajor, dbcVer)) + } + } + + // Report the installed databricks-connect version only in default mode. In + // constraints-only mode databricks-connect is not a managed dependency, so the + // spec omits dbconnectVersion even if the package is present transitively. + defaultMode := dbcPin != "" + + detail := "python=" + pyVer + if defaultMode && dbcVer != "" { + detail += " databricks-connect=" + dbcVer + } + p.markOK(PhaseValidate, detail) + + p.res.VenvPath = filepath.ToSlash(filepath.Join(p.ProjectDir, venvDir)) + if p.res.Resolved != nil { + if defaultMode { + p.res.Resolved.DBConnectVersion = dbcVer + } else { + p.res.Resolved.DBConnectVersion = "" + } + } + return nil +} + +// initialPhases returns the canonical phase list with every phase pending. +func initialPhases() []PhaseStatus { + phases := make([]PhaseStatus, len(allPhases)) + for i, name := range allPhases { + phases[i] = PhaseStatus{Phase: name, Status: StatusPending} + } + return phases +} + +// markOK marks a phase ok with an optional human-readable detail. +func (p *Pipeline) markOK(name PhaseName, detail string) { + for i := range p.res.Phases { + if p.res.Phases[i].Phase == name { + p.res.Phases[i].Status = StatusOK + p.res.Phases[i].Detail = detail + return + } + } +} + +// fail marks the given phase as errored, attaches the error (with its phase and +// disk-mutation flag) to the Result, and returns it. Phases after the failing +// one remain pending. +func (p *Pipeline) fail(phase PhaseName, diskMutated bool, pe *PipelineError) error { + pe.FailurePhase = phase + pe.DiskMutated = diskMutated + for i := range p.res.Phases { + if p.res.Phases[i].Phase == phase { + p.res.Phases[i].Status = StatusError + p.res.Phases[i].Detail = pe.Error() + break + } + } + p.res.Error = pe + return pe +} + +// asPipelineError returns err as a *PipelineError if it already is one, otherwise +// wraps it with the fallback code and message. +func asPipelineError(err error, fallback ErrorCode, format string, args ...any) *PipelineError { + if pe, ok := errors.AsType[*PipelineError](err); ok { + return pe + } + return NewError(fallback, err, format, args...) +} + +// artifactSource maps the from-cache flag to the JSON artifactSource value. +func artifactSource(fromCache bool) string { + if fromCache { + return artifactCache + } + return artifactNetwork +} + +// versionFromPin extracts the bare version from a dependency pin such as +// "databricks-connect~=17.2.0" → "17.2.0". Returns "" when pin is empty or has +// no version component. +func versionFromPin(pin string) string { + for i, c := range pin { + if c >= '0' && c <= '9' { + return pin[i:] + } + } + return "" +} + +// dbcMajorFromPin extracts the major version number from a databricks-connect +// pin string such as "databricks-connect~=17.2.0". Returns "" if unparseable. +func dbcMajorFromPin(pin string) string { + return majorVersion(versionFromPin(pin)) +} + +// majorVersion returns the major portion of a version string (digits before the +// first dot), e.g. "17" from "17.2.0". A bare integer like "17" returns "17". +// Returns "" for an empty string. +func majorVersion(v string) string { + if v == "" { + return "" + } + before, _, ok := strings.Cut(v, ".") + if !ok { + return v + } + return before +} + +// copyFile copies src to dst, creating or overwriting dst. +func copyFile(src, dst string) error { + data, err := os.ReadFile(src) + if err != nil { + return fmt.Errorf("read %s: %w", src, err) + } + if err := os.WriteFile(dst, data, 0o644); err != nil { + return fmt.Errorf("write %s: %w", dst, err) + } + return nil +} diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go new file mode 100644 index 00000000000..a9eb80c5aab --- /dev/null +++ b/libs/localenv/pipeline_test.go @@ -0,0 +1,369 @@ +package localenv + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type fakePM struct{ py, dbc string } + +func (fakePM) Name() string { return "fake" } +func (fakePM) EnsureAvailable(context.Context) (string, error) { return "fake 1.0", nil } +func (fakePM) EnsurePython(context.Context, string) error { return nil } +func (fakePM) Provision(context.Context, string) error { return nil } +func (fakePM) PostProvision(context.Context, string) error { return nil } +func (f fakePM) Validate(context.Context, string) (string, string, error) { + return f.py, f.dbc, nil +} + +func writeProject(t *testing.T) string { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte(`[project] +name = "demo" +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] +`), 0o644)) + return dir +} + +func newTestServer(t *testing.T) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(sampleToml)) + })) +} + +func TestPipelineCheckMutatesNothing(t *testing.T) { + dir := writeProject(t) + before, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.DryRun) + assert.True(t, res.OK) + require.NotNil(t, res.Plan) + assert.Contains(t, res.Plan.Diff, "==3.12.*") + after, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + assert.Equal(t, string(before), string(after)) // unchanged +} + +func TestPipelineProvisionsAndValidatesExisting(t *testing.T) { + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.OK) + assert.False(t, res.Greenfield) + require.NotNil(t, res.Resolved) + assert.Equal(t, "3.12", res.Resolved.PythonVersion) + assert.Equal(t, "17.2.0", res.Resolved.DBConnectVersion) + assert.Equal(t, ".venv", filepath.Base(res.VenvPath)) + merged, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + assert.Contains(t, string(merged), `"databricks-connect~=17.2.0"`) + assert.FileExists(t, filepath.Join(dir, "pyproject.toml.bak")) +} + +func TestPipelineGreenfieldCreatesNewPyproject(t *testing.T) { + dir := t.TempDir() + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.OK) + assert.True(t, res.Greenfield) + data, readErr := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + require.NoError(t, readErr) + assert.Contains(t, string(data), `"databricks-connect~=17.2.0",`) + // No backup created when pyproject.toml did not previously exist. + assert.NoFileExists(t, filepath.Join(dir, "pyproject.toml.bak")) +} + +func TestPipelineExistingBacksUp(t *testing.T) { + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.OK) + assert.FileExists(t, filepath.Join(dir, "pyproject.toml.bak")) + assert.Equal(t, "pyproject.toml.bak", filepath.Base(res.BackupPath)) +} + +func TestPipelineConstraintsOnlyOmitsDBConnect(t *testing.T) { + dir := t.TempDir() + srv := newTestServer(t) + defer srv.Close() + + // Even though databricks-connect is present in the venv (fakePM reports a + // version), constraints-only must not assert it, not write the pin, and not + // report dbconnectVersion (spec §6 omits it in this mode). + p := &Pipeline{ + Mode: ModeConstraintsOnly, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.OK) + assert.Equal(t, "constraints-only", res.Mode) + require.NotNil(t, res.Resolved) + assert.Empty(t, res.Resolved.DBConnectVersion, "constraints-only must omit dbconnectVersion") + data, readErr := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + require.NoError(t, readErr) + assert.NotContains(t, string(data), "databricks-connect") + // The dev group is emitted empty, not with a blank-string entry. + assert.Contains(t, string(data), "dev = []") + assert.NotContains(t, string(data), `""`) +} + +func TestPipelineNoTarget(t *testing.T) { + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{}, + Compute: stubCompute{}, PM: fakePM{}, + } + res, err := p.Run(t.Context()) + require.Error(t, err) + require.NotNil(t, res) + require.NotNil(t, res.Error) + assert.False(t, res.OK) + assert.Equal(t, ErrNoTarget, res.Error.Code) + // No-target is detected during target resolution, so it is reported at the + // resolve phase (the failing phase in phases[] and error.failurePhase agree). + assert.Equal(t, PhaseResolve, res.Error.FailurePhase) + // Preflight succeeded, resolve errored, everything after stays pending. + assert.Equal(t, StatusOK, phaseStatus(res, PhasePreflight)) + assert.Equal(t, StatusError, phaseStatus(res, PhaseResolve)) + assert.Equal(t, StatusPending, phaseStatus(res, PhaseProvision)) +} + +func TestPipelineRestoresBackupBeforeMerge(t *testing.T) { + dir := t.TempDir() + // Write an original pyproject.toml and a pre-existing .bak. + original := []byte(`[project] +name = "demo" +requires-python = ">=3.9" + +[dependency-groups] +dev = ["databricks-connect~=15.0.0"] +`) + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml.bak"), original, 0o644)) + // Current pyproject.toml has been mutated by a previous run. + mutated := []byte(`[project] +name = "demo" +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect~=17.2.0"] +`) + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), mutated, 0o644)) + + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + require.NotNil(t, res) + // The bak content (requires-python = ">=3.9") was the base; merged result should + // contain the newly pinned version. + data, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + assert.Contains(t, string(data), `"databricks-connect~=17.2.0"`) + assert.Contains(t, string(data), `requires-python = "==3.12.*"`) +} + +func TestPipelineUnreadableExistingIsNotTreatedAsGreenfield(t *testing.T) { + if runtime.GOOS == "windows" || os.Getuid() == 0 { + // chmod 000 does not block reads for root or on Windows. + t.Skip("read-permission enforcement not available") + } + dir := t.TempDir() + pyproject := filepath.Join(dir, "pyproject.toml") + original := []byte(`[project] +name = "demo" +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] +`) + require.NoError(t, os.WriteFile(pyproject, original, 0o644)) + // Make it unreadable so os.ReadFile fails with a non-not-exist error after + // detectManager has already seen the file exists. + require.NoError(t, os.Chmod(pyproject, 0o000)) + t.Cleanup(func() { _ = os.Chmod(pyproject, 0o644) }) + + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + // The run must fail at merge rather than silently overwrite the user's file. + require.Error(t, err) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrMerge, pe.Code) + assert.Equal(t, PhaseMerge, pe.FailurePhase) + assert.False(t, res.Greenfield, "an unreadable existing file must not be treated as greenfield") + + // The original file must be untouched (still unreadable, not overwritten). + require.NoError(t, os.Chmod(pyproject, 0o644)) + after, readErr := os.ReadFile(pyproject) + require.NoError(t, readErr) + assert.Equal(t, string(original), string(after), "the user's pyproject.toml must not be overwritten") +} + +func TestPipelineResultPopulatesResolved(t *testing.T) { + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + require.NotNil(t, res.Resolved) + assert.Equal(t, "3.12", res.Resolved.PythonVersion) + assert.Equal(t, "17.2.0", res.Resolved.DBConnectVersion) + assert.Equal(t, "network", res.Resolved.ArtifactSource) +} + +// newServerWithDBC returns a test server that serves a constraints TOML with the +// given databricks-connect pin value in the dev dependency group. +func newServerWithDBC(t *testing.T, dbcPin string) *httptest.Server { + t.Helper() + body := `[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["` + dbcPin + `"] + +[tool.uv] +constraint-dependencies = [] +` + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(body)) + })) +} + +func TestPipelineValidateRejectsUnparseablePin(t *testing.T) { + dir := writeProject(t) + // Serve a TOML whose dev group has a malformed databricks-connect entry + // (no version digits after the package name). + srv := newServerWithDBC(t, "databricks-connect") + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.Error(t, err) + require.NotNil(t, res.Error) + assert.Equal(t, ErrValidate, res.Error.Code) + assert.Equal(t, PhaseValidate, res.Error.FailurePhase) +} + +func TestPipelineValidateRejectsUnparseableInstalledVersion(t *testing.T) { + dir := writeProject(t) + // sampleToml has databricks-connect~=17.2.0 as the pin; use an empty installed + // version string to simulate an installed version that can't be parsed. + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: ""}, + } + res, err := p.Run(t.Context()) + require.Error(t, err) + require.NotNil(t, res.Error) + assert.Equal(t, ErrValidate, res.Error.Code) +} + +func TestMajorVersion(t *testing.T) { + cases := []struct { + input string + want string + }{ + {"17.2.0", "17"}, + {"17", "17"}, + {"", ""}, + {"3.12", "3"}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, majorVersion(tc.input), "input=%q", tc.input) + } +} + +// phaseStatus returns the status recorded for the named phase in res. +func phaseStatus(res *Result, name PhaseName) string { + for _, ph := range res.Phases { + if ph.Phase == name { + return ph.Status + } + } + return "" +} diff --git a/libs/localenv/pkgmanager.go b/libs/localenv/pkgmanager.go new file mode 100644 index 00000000000..fff370c76fc --- /dev/null +++ b/libs/localenv/pkgmanager.go @@ -0,0 +1,31 @@ +package localenv + +import "context" + +// PackageManager manages the Python environment for a dbconnect project. +type PackageManager interface { + // Name returns the name of the package manager (e.g. "uv"). + Name() string + + // EnsureAvailable ensures the package manager binary is present, installing + // it if necessary. It returns the version string on success. + EnsureAvailable(ctx context.Context) (version string, err error) + + // EnsurePython ensures the requested Python minor version (e.g. "3.12") is + // available via the package manager. + EnsurePython(ctx context.Context, minor string) error + + // Provision installs the project dependencies inside projectDir. + Provision(ctx context.Context, projectDir string) error + + // PostProvision seeds pip into the virtual environment inside projectDir. + // This step is required because VS Code's ms-python.vscode-python-envs + // extension falls back to `python -m pip list` when its `uv --version` + // probe fails on the GUI PATH; uv venvs contain no pip; and `uv sync` + // strips pip, so seeding must run after every sync. + PostProvision(ctx context.Context, projectDir string) error + + // Validate reads the Python minor version and databricks-connect version + // from the virtual environment inside projectDir. + Validate(ctx context.Context, projectDir string) (pythonVersion, dbconnectVersion string, err error) +} From f727199c8a640f989a56cbc800114af10d2bf611 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 20:01:00 +0200 Subject: [PATCH 22/25] Harden pipeline backup safety and version-major parsing Review of the pipeline layer found: - applyMerge treated every os.Stat error on the backup as "does not exist" and proceeded to copyFile over it. An existing-but-unstattable backup (permission or I/O error) would thus be overwritten with the already-merged content, destroying the canonical original the merge base relies on. It now only creates the backup on os.ErrNotExist and fails on any other stat error. - A backup-copy failure was reported with DiskMutated=false even though copyFile creates/truncates the .bak path and can leave a partial file. That path now reports DiskMutated=true. - majorVersion accepted any non-empty prefix before the first dot, so a malformed version like "bad.version" yielded major "bad" and could be compared as a real major. It now returns "" for a non-numeric major so the validate phase rejects malformed versions. Adds a direct applyMerge test for the unstattable-backup branch and extends the majorVersion cases with non-numeric inputs. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 36 +++++++++++++++++++++++++++++---- libs/localenv/pipeline_test.go | 37 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 7b7320d4bd7..f7b20b5c1d4 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -297,10 +297,20 @@ func (p *Pipeline) applyMerge(_ context.Context, mergedBytes []byte, greenfield // on a re-run the existing .bak is the canonical original unmanaged state // (mergePlan used it as the base), so overwriting it with the already-merged // pyproject.toml would destroy that baseline. - if _, err := os.Stat(backup); err != nil { + _, statErr := os.Stat(backup) + switch { + case statErr == nil: + // Backup already exists — keep it as the canonical baseline. + case errors.Is(statErr, os.ErrNotExist): + // copyFile creates/truncates the backup path, so a failure mid-copy may + // leave a partial .bak: report disk as mutated. if err := copyFile(pyproject, backup); err != nil { - return p.fail(PhaseMerge, false, NewError(ErrMerge, err, "backup pyproject.toml failed")) + return p.fail(PhaseMerge, true, NewError(ErrMerge, err, "backup pyproject.toml failed")) } + default: + // An existing-but-unstattable backup must not be overwritten (that would + // destroy the recoverable original); fail before any write instead. + return p.fail(PhaseMerge, false, NewError(ErrMerge, statErr, "cannot stat backup %s", filepath.ToSlash(backup))) } p.res.BackupPath = filepath.ToSlash(backup) } @@ -461,18 +471,36 @@ func dbcMajorFromPin(pin string) string { // majorVersion returns the major portion of a version string (digits before the // first dot), e.g. "17" from "17.2.0". A bare integer like "17" returns "17". -// Returns "" for an empty string. +// Returns "" for an empty string or a non-numeric major component, so the +// validate phase rejects a malformed version rather than comparing arbitrary +// strings as major versions. func majorVersion(v string) string { if v == "" { return "" } before, _, ok := strings.Cut(v, ".") if !ok { - return v + before = v + } + if before == "" || !isAllDigits(before) { + return "" } return before } +// isAllDigits reports whether s is non-empty and every rune is an ASCII digit. +func isAllDigits(s string) bool { + if s == "" { + return false + } + for _, c := range s { + if c < '0' || c > '9' { + return false + } + } + return true +} + // copyFile copies src to dst, creating or overwriting dst. func copyFile(src, dst string) error { data, err := os.ReadFile(src) diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index a9eb80c5aab..52f7f297b34 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -128,6 +128,38 @@ func TestPipelineExistingBacksUp(t *testing.T) { assert.Equal(t, "pyproject.toml.bak", filepath.Base(res.BackupPath)) } +func TestApplyMergeFailsOnUnstattableBackupWithoutOverwrite(t *testing.T) { + if runtime.GOOS == "windows" || os.Getuid() == 0 { + // chmod-based stat blocking does not apply for root or on Windows. + t.Skip("stat-permission enforcement not available") + } + // Both pyproject.toml and its .bak live in a project dir that is made + // unsearchable, so os.Stat of the backup fails with a permission error rather + // than not-exist — isolating applyMerge's "can't stat" branch. applyMerge is + // called directly, bypassing the writability preflight. + dir := filepath.Join(t.TempDir(), "proj") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte("[project]\n"), 0o644)) + backup := filepath.Join(dir, "pyproject.toml.bak") + require.NoError(t, os.WriteFile(backup, []byte("ORIGINAL BACKUP\n"), 0o644)) + require.NoError(t, os.Chmod(dir, 0o000)) + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + + p := &Pipeline{ProjectDir: dir, res: &Result{Phases: initialPhases()}} + err := p.applyMerge(t.Context(), []byte("merged"), false) + require.Error(t, err) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, PhaseMerge, pe.FailurePhase) + assert.False(t, pe.DiskMutated, "no write should have happened before the stat check") + + // The original backup must be intact. + require.NoError(t, os.Chmod(dir, 0o755)) + got, readErr := os.ReadFile(backup) + require.NoError(t, readErr) + assert.Equal(t, "ORIGINAL BACKUP\n", string(got), "the canonical backup must not be overwritten") +} + func TestPipelineConstraintsOnlyOmitsDBConnect(t *testing.T) { dir := t.TempDir() srv := newTestServer(t) @@ -352,6 +384,11 @@ func TestMajorVersion(t *testing.T) { {"17", "17"}, {"", ""}, {"3.12", "3"}, + // Non-numeric major components are rejected (empty) so validation does + // not compare arbitrary strings as versions. + {"bad.version", ""}, + {"v17.2", ""}, + {"abc", ""}, } for _, tc := range cases { assert.Equal(t, tc.want, majorVersion(tc.input), "input=%q", tc.input) From f8bde02d2e8c32f707ae27dafdc4d62f4d12f6c8 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 21:13:56 +0200 Subject: [PATCH 23/25] Skip the writability probe under --check Round-4 review noted preflight ran ensureWritable even under --check, which creates and removes a temp file (a disk mutation in a dry run) and fails a read-only project the user only wants to inspect. The probe now runs only for a real (non-check) run; it exists to fail fast before a write that --check never performs. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 10 ++++++++-- libs/localenv/pipeline_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index f7b20b5c1d4..ddcb1204f1d 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -95,8 +95,14 @@ func (p *Pipeline) run(ctx context.Context) error { if m := detectManager(p.ProjectDir); m != managerUv { return p.fail(PhasePreflight, false, NewError(ErrManagerUnsupported, nil, "%s", managerGuidance(m))) } - if err := ensureWritable(p.ProjectDir); err != nil { - return p.fail(PhasePreflight, false, NewError(ErrNotWritable, err, "project directory %s is not writable", filepath.ToSlash(p.ProjectDir))) + // Skip the writability probe under --check: it creates and removes a temp + // file, which would be a disk mutation in a dry run, and a read-only project + // the user only wants to inspect must not fail. The probe exists to fail fast + // before a real write, which --check never performs. + if !p.Check { + if err := ensureWritable(p.ProjectDir); err != nil { + return p.fail(PhasePreflight, false, NewError(ErrNotWritable, err, "project directory %s is not writable", filepath.ToSlash(p.ProjectDir))) + } } version, err := p.PM.EnsureAvailable(ctx) if err != nil { diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 52f7f297b34..00db3d04829 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -64,6 +64,37 @@ func TestPipelineCheckMutatesNothing(t *testing.T) { assert.Equal(t, string(before), string(after)) // unchanged } +func TestPipelineCheckWorksOnReadOnlyDir(t *testing.T) { + if runtime.GOOS == "windows" || os.Getuid() == 0 { + t.Skip("read-only-dir enforcement not available") + } + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + // Make the project dir read-only: --check must still compute the plan without + // a writability probe (which would both mutate disk and fail here). + require.NoError(t, os.Chmod(dir, 0o555)) + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.OK) + require.NotNil(t, res.Plan) + // No writecheck temp file was left behind in the project dir. + entries, err := os.ReadDir(dir) + require.NoError(t, err) + for _, e := range entries { + assert.NotContains(t, e.Name(), "writecheck", "dry run must not create temp files") + } +} + func TestPipelineProvisionsAndValidatesExisting(t *testing.T) { dir := writeProject(t) srv := newTestServer(t) From 4c3802757307912b634205ef1e47e8ade2a8cf75 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 3 Jul 2026 21:22:15 +0200 Subject: [PATCH 24/25] Skip package-manager availability under --check too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 review noted that although --check now skips the writability probe, it still called PackageManager.EnsureAvailable, which may install the manager (uv) when missing — another disk mutation in a dry run. Preflight now performs neither write-side step under --check (neither is needed to compute the plan) and reports the phase as "check". Adds a test with a PackageManager that errors on every method, asserting --check still succeeds and produces a plan. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 25 ++++++++++-------- libs/localenv/pipeline_test.go | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index ddcb1204f1d..004271eb69d 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -95,20 +95,25 @@ func (p *Pipeline) run(ctx context.Context) error { if m := detectManager(p.ProjectDir); m != managerUv { return p.fail(PhasePreflight, false, NewError(ErrManagerUnsupported, nil, "%s", managerGuidance(m))) } - // Skip the writability probe under --check: it creates and removes a temp - // file, which would be a disk mutation in a dry run, and a read-only project - // the user only wants to inspect must not fail. The probe exists to fail fast - // before a real write, which --check never performs. - if !p.Check { + // Under --check the pipeline only reads and reports a plan, so it must not + // mutate anything at preflight. Two preflight steps can write: + // - ensureWritable creates and removes a temp file (and would fail a + // read-only project the user only wants to inspect); + // - PackageManager.EnsureAvailable may install the manager (uv) if missing. + // Both exist to fail fast before real writes, which --check never performs, so + // they are skipped in a dry run. Neither result is needed to compute the plan. + if p.Check { + p.markOK(PhasePreflight, "check") + } else { if err := ensureWritable(p.ProjectDir); err != nil { return p.fail(PhasePreflight, false, NewError(ErrNotWritable, err, "project directory %s is not writable", filepath.ToSlash(p.ProjectDir))) } + version, err := p.PM.EnsureAvailable(ctx) + if err != nil { + return p.fail(PhasePreflight, false, asPipelineError(err, ErrUvMissing, "%s unavailable", p.PM.Name())) + } + p.markOK(PhasePreflight, p.PM.Name()+" "+version) } - version, err := p.PM.EnsureAvailable(ctx) - if err != nil { - return p.fail(PhasePreflight, false, asPipelineError(err, ErrUvMissing, "%s unavailable", p.PM.Name())) - } - p.markOK(PhasePreflight, p.PM.Name()+" "+version) // Phase: resolve — compute target → environment key. target, err := p.resolve(ctx) diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 00db3d04829..fdc76fbd248 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -2,6 +2,7 @@ package localenv import ( "context" + "errors" "net/http" "net/http/httptest" "os" @@ -24,6 +25,32 @@ func (f fakePM) Validate(context.Context, string) (string, string, error) { return f.py, f.dbc, nil } +// noProvisionPM fails any method that could touch the machine (install the +// manager, install Python, sync, seed pip, validate). It asserts that --check +// never reaches those write-side operations. +type noProvisionPM struct{} + +func (noProvisionPM) Name() string { return "noprov" } +func (noProvisionPM) EnsureAvailable(context.Context) (string, error) { + return "", errors.New("EnsureAvailable must not be called under --check") +} + +func (noProvisionPM) EnsurePython(context.Context, string) error { + return errors.New("EnsurePython must not be called under --check") +} + +func (noProvisionPM) Provision(context.Context, string) error { + return errors.New("Provision must not be called under --check") +} + +func (noProvisionPM) PostProvision(context.Context, string) error { + return errors.New("PostProvision must not be called under --check") +} + +func (noProvisionPM) Validate(context.Context, string) (string, string, error) { + return "", "", errors.New("Validate must not be called under --check") +} + func writeProject(t *testing.T) string { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte(`[project] @@ -64,6 +91,26 @@ func TestPipelineCheckMutatesNothing(t *testing.T) { assert.Equal(t, string(before), string(after)) // unchanged } +func TestPipelineCheckDoesNotProvision(t *testing.T) { + // --check must not call any PackageManager method that could mutate the + // machine (EnsureAvailable may install uv). noProvisionPM errors on all of + // them; the dry run must still succeed and produce a plan. + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: TargetFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: noProvisionPM{}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.True(t, res.OK) + require.NotNil(t, res.Plan) +} + func TestPipelineCheckWorksOnReadOnlyDir(t *testing.T) { if runtime.GOOS == "windows" || os.Getuid() == 0 { t.Skip("read-only-dir enforcement not available") From be1b6b38081bf55b625181e5c2fbd8e46060f0d9 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Mon, 6 Jul 2026 13:45:58 +0200 Subject: [PATCH 25/25] Construct the pipeline Result via NewResult Use the NewResult constructor added in the foundation layer so the --json phases/warnings arrays are guaranteed non-nil (emit [] not null) at the real construction site, then override Phases with the canonical pending list. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 004271eb69d..c17d0d6fdb8 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -70,15 +70,15 @@ func (p *Pipeline) Run(ctx context.Context) (*Result, error) { p.Flags, ) - p.res = &Result{ - SchemaVersion: SchemaVersion, - Command: CommandName, - Mode: p.Mode.String(), - DryRun: p.Check, - // Phases start as pending and flip to ok/error as the run progresses. - Phases: initialPhases(), - Warnings: []Warning{}, - } + // NewResult seeds Phases/Warnings to non-nil slices so --json always emits + // [] not null; override Phases with the canonical pending phase list. + p.res = NewResult() + p.res.SchemaVersion = SchemaVersion + p.res.Command = CommandName + p.res.Mode = p.Mode.String() + p.res.DryRun = p.Check + // Phases start as pending and flip to ok/error as the run progresses. + p.res.Phases = initialPhases() if err := p.run(ctx); err != nil { return p.res, err