From ff24b328b58a13bba9611710bb1d5024c1b67054 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Wed, 8 Jul 2026 13:01:45 +0200 Subject: [PATCH 1/2] Skip constraint cache writes under --check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A --check dry run must not mutate disk, but FetchConstraints wrote the fetched artifact into the cache dir on every successful live fetch — so a dry run still populated the cache whenever it was writable, even though the pipeline skips every other write-side step under --check. Thread a writeCache flag through FetchConstraints; the pipeline passes !p.Check. An existing cache is still read for offline fallback, since reading is not a mutation. Assert both directly (FetchConstraints with writeCache=false leaves the cache dir empty) and end-to-end (the --check pipeline test now checks the cache dir stays empty too). Reported by codex review of the pipeline PR. Co-authored-by: Isaac --- libs/localenv/constraints.go | 16 +++++++++++---- libs/localenv/constraints_test.go | 33 +++++++++++++++++++++++-------- libs/localenv/pipeline.go | 4 +++- libs/localenv/pipeline_test.go | 8 +++++++- 4 files changed, 47 insertions(+), 14 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index 22a2c49f66e..e103b86e5ff 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -137,7 +137,12 @@ func writeCacheAtomic(path string, data []byte) error { // 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) { +// +// writeCache controls whether a successful live fetch populates the on-disk +// cache. Callers pass false for a dry run (--check), which must not mutate +// disk; an existing cache is still read for offline fallback, since reading is +// not a mutation. +func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string, writeCache bool) (*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 @@ -158,9 +163,12 @@ func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string) (*C return nil, fmt.Errorf("parse constraints for %s: %w", envKey, err) } // 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) + // so a read-only cacheDir doesn't break the command. Skipped under a dry + // run so --check performs no disk writes at all. + if writeCache { + if err := writeCacheAtomic(cachePath, data); err != nil { + log.Debugf(ctx, "failed to write constraint cache %s: %v", filepath.ToSlash(cachePath), err) + } } return &Constraints{ EnvKey: envKey, diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index 8df363d943e..c330c4bba2f 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -30,7 +30,7 @@ func TestRepoConstraintBaseURL(t *testing.T) { 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()) + _, err := FetchConstraints(t.Context(), "", "serverless/serverless-v4", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrFetch, pe.Code) @@ -116,7 +116,7 @@ func TestFetchConstraintsCreatesCacheDir(t *testing.T) { })) defer srv.Close() - _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir) + _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir, true) require.NoError(t, err) // The cache file was written into the freshly created directory. written, err := os.ReadFile(filepath.Join(cacheDir, cacheFileName("serverless/serverless-v4"))) @@ -124,6 +124,23 @@ func TestFetchConstraintsCreatesCacheDir(t *testing.T) { assert.Equal(t, sampleToml, string(written)) } +func TestFetchConstraintsSkipsCacheWriteWhenDisabled(t *testing.T) { + // With writeCache=false (the --check dry-run path), a successful live fetch + // must not write anything to cacheDir. + cacheDir := t.TempDir() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(sampleToml)) + })) + defer srv.Close() + + c, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir, false) + require.NoError(t, err) + assert.False(t, c.FromCache) + entries, err := os.ReadDir(cacheDir) + require.NoError(t, err) + assert.Empty(t, entries, "no cache file should be written under --check") +} + 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. @@ -140,7 +157,7 @@ func TestFetchConstraintsHTTP(t *testing.T) { })) defer srv.Close() - c, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir()) + c, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir(), true) require.NoError(t, err) assert.False(t, c.FromCache) assert.Equal(t, "databricks-connect~=17.2.0", c.DatabricksConnect) @@ -155,7 +172,7 @@ func TestFetchConstraintsEnvKeyNotFound(t *testing.T) { })) defer srv.Close() - _, err := FetchConstraints(t.Context(), srv.URL, "dbr/99.9.x-scala2.12", t.TempDir()) + _, err := FetchConstraints(t.Context(), srv.URL, "dbr/99.9.x-scala2.12", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrEnvUnsupported, pe.Code) @@ -167,7 +184,7 @@ func TestFetchConstraintsTransportFailureNoCache(t *testing.T) { url := down.URL down.Close() - _, err := FetchConstraints(t.Context(), url, "serverless/serverless-v4", t.TempDir()) + _, err := FetchConstraints(t.Context(), url, "serverless/serverless-v4", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrFetch, pe.Code) @@ -182,7 +199,7 @@ func TestFetchConstraintsRejectsOversizedBody(t *testing.T) { })) defer srv.Close() - _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir()) + _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrFetch, pe.Code) @@ -194,12 +211,12 @@ func TestFetchConstraintsFallsBackToCache(t *testing.T) { 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) + _, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir, true) 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) + c, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir, true) require.NoError(t, err) assert.True(t, c.FromCache) } diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index e3f152d7304..3fa0737a911 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -191,8 +191,10 @@ func (p *Pipeline) resolve(ctx context.Context) (*TargetInfo, error) { } // fetch fetches constraints for the resolved target and records the fetch phase. +// Under --check the cache is not populated, so a dry run performs no disk writes +// (an existing cache is still read for offline fallback). func (p *Pipeline) fetch(ctx context.Context, target *TargetInfo) (*Constraints, error) { - c, err := FetchConstraints(ctx, p.ConstraintBaseURL, target.EnvKey, p.CacheDir) + c, err := FetchConstraints(ctx, p.ConstraintBaseURL, target.EnvKey, p.CacheDir, !p.Check) 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 diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 1a318cd1872..8d6286c47b6 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -81,12 +81,13 @@ func newTestServer(t *testing.T) *httptest.Server { func TestPipelineCheckMutatesNothing(t *testing.T) { dir := writeProject(t) before, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + cacheDir := t.TempDir() srv := newTestServer(t) defer srv.Close() p := &Pipeline{ Mode: ModeDefault, Check: true, ProjectDir: dir, - ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + ConstraintBaseURL: srv.URL, CacheDir: cacheDir, Flags: TargetFlags{Serverless: "v4"}, Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, } @@ -98,6 +99,11 @@ func TestPipelineCheckMutatesNothing(t *testing.T) { assert.Contains(t, res.Plan.Diff, "==3.12.*") after, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) assert.Equal(t, string(before), string(after)) // unchanged + + // --check must not populate the constraint cache either (no disk writes). + entries, err := os.ReadDir(cacheDir) + require.NoError(t, err) + assert.Empty(t, entries) } func TestPipelineCheckReRunPlanMatchesRealRun(t *testing.T) { From 0eb1236387a9de23cb4ae4aca38e669d9785c5ed Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Mon, 13 Jul 2026 16:22:04 +0200 Subject: [PATCH 2/2] Insert databricks-connect on merge; sanitize greenfield project name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two correctness nits from review of the pipeline PR (#5851). Since that PR was already queued to merge, the fixes land here in the follow-up. - mergeDatabricksConnect only ever *replaced* an existing databricks-connect element, so a default-mode sync of an existing project that didn't already pin databricks-connect wrote the file back with no pin; uv installed nothing and validate failed with E_VALIDATE. It now *inserts* the pin — into the dev array (single- or multi-line), creating the dev key and the [dependency-groups] table when absent — mirroring RenderFreshPyproject. Constraints-only (empty value) stays a no-op; the insert path is idempotent. - Greenfield rendered name = filepath.Base(ProjectDir), which is "." when run from the project's own directory (and "/" at root) — not a valid PEP 621 name, so uv sync rejected the file. projectName() now resolves via filepath.Abs and sanitizes to a valid PEP 508 identifier, falling back to a default when nothing usable remains. Co-authored-by: Isaac --- libs/localenv/merge.go | 153 ++++++++++++++++++++------ libs/localenv/merge_test.go | 190 +++++++++++++++++++++++++++++++++ libs/localenv/pipeline.go | 48 ++++++++- libs/localenv/pipeline_test.go | 33 ++++++ 4 files changed, 391 insertions(+), 33 deletions(-) diff --git a/libs/localenv/merge.go b/libs/localenv/merge.go index 2dc36a25efc..d0bc39ad1da 100644 --- a/libs/localenv/merge.go +++ b/libs/localenv/merge.go @@ -219,32 +219,33 @@ func trailingComment(line string) string { 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 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. +// mergeDatabricksConnect ensures [dependency-groups].dev pins databricks-connect +// to value. It replaces an existing databricks-connect element (multi-line or +// single-line array form) and, when none exists, inserts one — creating the dev +// key and the [dependency-groups] table when those are absent too, mirroring +// RenderFreshPyproject so an existing project without a pin provisions the same as +// a greenfield one. An empty value (constraints-only mode) is a no-op: the user's +// dev group is left untouched rather than having its pin blanked out or one added. // -// The rewrite is scoped to the dev array's own span (found via bracket depth), so a +// The edit 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. +// trailing comment on some other line is never clobbered. The insert path is +// idempotent: a subsequent merge finds the element and rewrites it in place. func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { if value == "" { return lines, false } + elem := `"` + value + `"` + header, end, found := tableBounds(lines, "[dependency-groups]") if !found { - return lines, false + // No [dependency-groups] table: append a fresh managed dev group. + return appendManagedBlock(lines, []string{"[dependency-groups]", "dev = [", " " + elem + ",", "]"}), true } // Locate the dev assignment and the line span of its array value. @@ -256,39 +257,133 @@ func mergeDatabricksConnect(lines []string, value string) ([]string, bool) { } } if devStart == -1 { - return lines, false + // The table exists but has no dev key: insert one right after the header. + insert := []string{"dev = [", " " + elem + ",", "]"} + out := make([]string, 0, len(lines)+len(insert)) + out = append(out, lines[:header+1]...) + out = append(out, insert...) + out = append(out, lines[header+1:]...) + return out, true } arrayLast, _ := arrayLineSpan(lines, devStart, end) - // Single-line form: the whole array is on the dev line itself. Only rewrite + // Single-line form: the whole array is on the dev line itself. Only edit // 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 + if replaced, ok := replaceDbconnectElement(arrayPart, elem); ok { + newLine := replaced + commentPart + if newLine == line { + return lines, false + } + lines[devStart] = newLine + return lines, true } - replaced := dbconnectTokenRe.ReplaceAllString(arrayPart, `"`+value+`"`) + commentPart - if replaced == line { + // No databricks-connect element: insert one as the first array element. + open := strings.Index(arrayPart, "[") + closeIdx := strings.LastIndex(arrayPart, "]") + if open < 0 || closeIdx < open { return lines, false } - lines[devStart] = replaced + inner := strings.TrimSpace(arrayPart[open+1 : closeIdx]) + newInner := elem + if inner != "" { + newInner = elem + ", " + inner + } + lines[devStart] = arrayPart[:open+1] + newInner + arrayPart[closeIdx:] + commentPart 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 { + // Multi-line form: the array spans devStart..arrayLast. An existing + // databricks-connect element may sit on a dedicated element line, share the + // opening "dev = [" line, or carry a trailing comment, and may be spelled with + // any PEP 503-equivalent separators/case — so detect it (outside comments, + // name-normalized) anywhere in the span and rewrite in place. Only when no + // element exists anywhere do we insert, which avoids duplicating a pin a raw + // substring match would miss. + lastElem := -1 + for i := devStart; i <= arrayLast; i++ { + code, comment := lines[i], "" + if c := commentStart(code); c >= 0 { + code, comment = code[:c], code[c:] + } + if replaced, ok := replaceDbconnectElement(code, elem); ok { + // Rewrite only the code portion; a trailing comment is user content and + // must be preserved byte-for-byte, even if it contains a quoted token. + newLine := replaced + comment + if newLine == lines[i] { return lines, false } - lines[i] = replacement + lines[i] = newLine return lines, true } + if dbconnectQuotedRe.MatchString(code) { + lastElem = i + } + } + // No databricks-connect element anywhere in the array: insert one before the + // closing "]", matching the indentation of the existing elements (default four + // spaces when empty). + indent := " " + for i := devStart + 1; i < arrayLast; i++ { + if strings.TrimSpace(lines[i]) != "" { + indent = leadingWhitespace(lines[i]) + break + } } - return lines, false + // TOML lets the final element omit its trailing comma; appending after it would + // then leave two adjacent elements with no separator. Ensure the current last + // element (if it is on its own line, not the "]" line) carries a comma first. + if lastElem >= 0 && lastElem < arrayLast { + lines[lastElem] = ensureTrailingComma(lines[lastElem]) + } + out := make([]string, 0, len(lines)+1) + out = append(out, lines[:arrayLast]...) + out = append(out, indent+elem+",") + out = append(out, lines[arrayLast:]...) + return out, true +} + +// dbconnectQuotedRe matches any double-quoted array element token. +var dbconnectQuotedRe = regexp.MustCompile(`"[^"]*"`) + +// replaceDbconnectElement replaces the first quoted element in code whose package +// name is databricks-connect (compared under PEP 503 normalization, so +// "databricks_connect" / "Databricks-Connect" / "databricks.connect" all match) +// with elem. It returns the rewritten code and whether a replacement was made. +// Matching mirrors the artifact side (isDatabricksConnectDep) so a differently +// spelled existing pin is rewritten in place rather than left for the insert path +// to duplicate. +func replaceDbconnectElement(code, elem string) (string, bool) { + for _, m := range dbconnectQuotedRe.FindAllStringIndex(code, -1) { + inner := code[m[0]+1 : m[1]-1] + if isDatabricksConnectDep(inner) { + return code[:m[0]] + elem + code[m[1]:], true + } + } + return code, false +} + +// ensureTrailingComma appends a "," after the last non-space code character of +// line when it lacks one, preserving any trailing whitespace and inline comment. +// A blank or comment-only line is returned unchanged. +func ensureTrailingComma(line string) string { + code, comment := line, "" + if c := commentStart(code); c >= 0 { + code, comment = code[:c], code[c:] + } + trimmed := strings.TrimRight(code, " \t") + if trimmed == "" || strings.HasSuffix(trimmed, ",") { + return line + } + return trimmed + "," + code[len(trimmed):] + comment +} + +// leadingWhitespace returns the run of spaces and tabs at the start of s. +func leadingWhitespace(s string) string { + return s[:len(s)-len(strings.TrimLeft(s, " \t"))] } // splitAtArrayClose splits a single-line array assignment into the part up to and @@ -346,10 +441,6 @@ func arrayLineSpan(lines []string, start, limit int) (last int, multiline bool) return limit - 1, true } -// 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. diff --git a/libs/localenv/merge_test.go b/libs/localenv/merge_test.go index 6e065d4722f..a2ad23babbb 100644 --- a/libs/localenv/merge_test.go +++ b/libs/localenv/merge_test.go @@ -253,6 +253,196 @@ dev = [ assert.Contains(t, string(out), ` "databricks-connect~=17.2.0",`) } +func TestMergeInsertsDatabricksConnectMultiLine(t *testing.T) { + // Existing project with a dev group that does not pin databricks-connect: the + // pin must be inserted (mirroring greenfield) rather than left absent. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "pytest~=8.0", +] +`) + out, regions, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, `"databricks-connect~=17.2.0",`) + assert.Contains(t, s, `"pytest~=8.0",`, "existing element preserved") + assert.Contains(t, regions, "databricks-connect") + // Idempotent: a second merge finds the element and rewrites in place. + out2, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(out2)) +} + +func TestMergeInsertsDatabricksConnectSingleLine(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["pytest~=8.0"] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + assert.Contains(t, string(out), `dev = ["databricks-connect~=17.2.0", "pytest~=8.0"]`) + out2, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, string(out), string(out2)) +} + +func TestMergeInsertsDatabricksConnectEmptyDev(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + assert.Contains(t, string(out), `dev = ["databricks-connect~=17.2.0"]`) +} + +func TestMergeInsertsDevKeyWhenAbsent(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +test = ["pytest~=8.0"] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, `"databricks-connect~=17.2.0",`) + assert.Contains(t, s, `test = ["pytest~=8.0"]`, "sibling group untouched") + out2, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(out2)) +} + +func TestMergeInsertsDependencyGroupsWhenAbsent(t *testing.T) { + in := []byte(`[project] +requires-python = ">=3.10" +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, "[dependency-groups]") + assert.Contains(t, s, `"databricks-connect~=17.2.0",`) + out2, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(out2)) +} + +func TestMergeReplacesDatabricksConnectOnDevLine(t *testing.T) { + // The pin shares the opening "dev = [" line of a multi-line array. It must be + // rewritten in place, not duplicated by the insert path. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0", + "pytest~=8.0", +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Equal(t, 1, strings.Count(s, "databricks-connect"), "must not duplicate the pin") + assert.Contains(t, s, "databricks-connect~=17.2.0") + assert.NotContains(t, s, "databricks-connect~=16.0.0") +} + +func TestMergeReplacesDatabricksConnectWithTrailingComment(t *testing.T) { + // The pin element carries a trailing comment. It must be rewritten in place + // (comment preserved), not duplicated. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "databricks-connect~=16.0.0", # keep me + "pytest~=8.0", +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Equal(t, 1, strings.Count(s, "databricks-connect"), "must not duplicate the pin") + assert.Contains(t, s, `"databricks-connect~=17.2.0", # keep me`) +} + +func TestMergePreservesCommentMentioningDatabricksConnect(t *testing.T) { + // A trailing comment that itself contains a quoted "databricks-connect..." + // token must be preserved byte-for-byte; only the code element is rewritten. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "databricks-connect~=16.0.0", # was "databricks-connect~=16.0.0" + "pytest~=8.0", +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + // Code element is rewritten; the comment mention is left verbatim. + assert.Contains(t, s, `"databricks-connect~=17.2.0", # was "databricks-connect~=16.0.0"`) + assert.Equal(t, 1, strings.Count(s, "databricks-connect~=16.0.0"), "only the comment mention of the old pin remains") +} + +func TestMergeRewritesNonCanonicalDatabricksConnectSpelling(t *testing.T) { + // A pin spelled with PEP 503-equivalent separators/case must be rewritten in + // place, not left undetected so the insert path adds a conflicting second pin. + for _, spelling := range []string{"databricks_connect", "Databricks-Connect", "databricks.connect"} { + in := []byte("[project]\nrequires-python = \">=3.10\"\n\n[dependency-groups]\ndev = [\n \"" + spelling + "~=16.0.0\",\n]\n") + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err, spelling) + s := string(out) + assert.Equal(t, 1, strings.Count(s, `"databricks-connect~=17.2.0"`), "spelling %q must be rewritten in place, not duplicated:\n%s", spelling, s) + assert.NotContains(t, s, "~=16.0.0", "old pin (any spelling) must be gone") + } +} + +func TestMergeInsertsCommaWhenLastElementHasNone(t *testing.T) { + // TOML permits the final array element to omit its trailing comma. Inserting + // after it must first add the separating comma, or the result is invalid TOML. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = [ + "pytest~=8.0" +] +`) + out, _, err := MergeManaged(in, testConstraints()) + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, `"pytest~=8.0",`, "previous last element gains a separating comma") + assert.Contains(t, s, `"databricks-connect~=17.2.0",`) + // Round-trips as valid TOML. + out2, _, err := MergeManaged(out, testConstraints()) + require.NoError(t, err) + assert.Equal(t, s, string(out2)) +} + +func TestMergeConstraintsOnlyDoesNotInsertDatabricksConnect(t *testing.T) { + // Empty DatabricksConnect (constraints-only) must not add a pin. + in := []byte(`[project] +requires-python = ">=3.10" + +[dependency-groups] +dev = ["pytest~=8.0"] +`) + c := testConstraints() + c.DatabricksConnect = "" + out, regions, err := MergeManaged(in, c) + require.NoError(t, err) + assert.NotContains(t, string(out), "databricks-connect") + assert.NotContains(t, regions, "databricks-connect") +} + func TestRenderFreshPyproject(t *testing.T) { out := RenderFreshPyproject("demo", testConstraints()) s := string(out) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 3fa0737a911..db8496f85fa 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -255,8 +255,7 @@ func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, 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) + merged = RenderFreshPyproject(projectName(p.ProjectDir), effective) changedRegions = []string{regionRequiresPython, regionToolUv} if dbcPin != "" { changedRegions = append(changedRegions, regionDatabricksConnect) @@ -518,6 +517,51 @@ func isAllDigits(s string) bool { return true } +// defaultProjectName is used for a fresh pyproject.toml when the project +// directory yields no usable PEP 508 name (e.g. filesystem root). +const defaultProjectName = "app" + +// projectName derives a PEP 508-valid project name from the project directory. +// filepath.Base(".") / ("") is "." and Base("/") is "/", none of which are valid +// [project].name values, so uv sync would reject the rendered file. Resolve to an +// absolute path first so "." picks up the real directory name, then sanitize to a +// valid identifier, falling back to defaultProjectName when nothing usable remains. +func projectName(dir string) string { + base := filepath.Base(dir) + if base == "." || base == string(filepath.Separator) || base == "" { + if abs, err := filepath.Abs(dir); err == nil { + base = filepath.Base(abs) + } + } + return sanitizeProjectName(base) +} + +// sanitizeProjectName maps an arbitrary directory name to a valid PEP 508 name: +// runs of non-alphanumeric characters collapse to a single "-", and leading and +// trailing separators are trimmed (a PEP 508 name must start and end with an +// alphanumeric). Returns defaultProjectName when nothing usable remains. +func sanitizeProjectName(name string) string { + var b strings.Builder + prevDash := false + for _, r := range name { + switch { + case (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9'): + b.WriteRune(r) + prevDash = false + default: + if b.Len() > 0 && !prevDash { + b.WriteByte('-') + prevDash = true + } + } + } + out := strings.Trim(b.String(), "-") + if out == "" { + return defaultProjectName + } + return out +} + // copyFile copies src to dst, creating or overwriting dst. dst is created with // src's permission bits: the backup preserves a locked-down pyproject.toml // (e.g. 0o600 because it carries a private index URL) rather than widening it to diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 8d6286c47b6..1823bcdcc90 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -233,6 +233,39 @@ func TestPipelineGreenfieldCreatesNewPyproject(t *testing.T) { assert.NoFileExists(t, filepath.Join(dir, "pyproject.toml.bak")) } +func TestProjectName(t *testing.T) { + // "." / "" / root resolve to the real directory name, not an invalid literal; + // non-alphanumeric runs collapse to "-"; unusable input falls back to a default. + cwd, err := filepath.Abs(".") + require.NoError(t, err) + assert.Equal(t, sanitizeProjectName(filepath.Base(cwd)), projectName(".")) + assert.Equal(t, sanitizeProjectName(filepath.Base(cwd)), projectName("")) + assert.Equal(t, "my-proj", projectName("/tmp/my proj")) + assert.Equal(t, "my-proj", projectName("/tmp/.my.proj.")) + assert.Equal(t, defaultProjectName, sanitizeProjectName(".")) + assert.Equal(t, defaultProjectName, sanitizeProjectName("///")) +} + +func TestPipelineGreenfieldFromDotDirRendersValidName(t *testing.T) { + // Running greenfield with ProjectDir="." must not render name = ".". + 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.Greenfield) + data, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + assert.NotContains(t, string(data), `name = "."`) + assert.Contains(t, string(data), `name = "`+sanitizeProjectName(filepath.Base(dir))+`"`) +} + func TestPipelineExistingBacksUp(t *testing.T) { dir := writeProject(t) srv := newTestServer(t)