diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 1f46a0a57c6..4ad8ebccffd 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -5,6 +5,7 @@ ### Notable Changes ### CLI +* `workspace export-dir` no longer aborts when a workspace object's name is not a legal local filename (e.g. a notebook named `New Notebook 2026-05-04 13:54:24` whose `:` is illegal on Windows). Such files are now exported under a sanitized name with a warning and the export completes ([#5171](https://github.com/databricks/cli/issues/5171)). ### Bundles * `bundle run` now prints the modern job run URL (`/jobs//runs/`) so that non-admin users permitted to view the run are taken to the run instead of the workspace homepage. diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/_script b/acceptance/cmd/workspace/export-dir-illegal-filename/_script new file mode 100644 index 00000000000..8515606362c --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/_script @@ -0,0 +1,15 @@ +# Shared by the posix/ and windows/ leaf tests, which differ only in their GOOS +# gate and golden output. The notebook name contains ':', which is a legal local +# filename character on Linux/macOS but illegal on Windows, so export-dir renames +# it to a legal name with a warning on Windows and exports it normally elsewhere. +# The real #5171 trigger is the auto-generated "New Notebook " name. +# We use two colons ("a:b:c") rather than one: a single ':' is the Windows +# alternate-data-stream separator, so os.Create("New Notebook a:b.py") silently +# succeeds (writing stream "b.py") instead of failing with ERROR_INVALID_NAME. +# Two colons make the stream type invalid, which is what real timestamp names +# ("13:54:24") hit. A plain name keeps the timestamp redaction out of the golden. +$CLI workspace import "/test-dir/New Notebook a:b:c.py" --file "$TESTDIR/../notebook.py" --format AUTO --language PYTHON + +mkdir -p "$TEST_TMP_DIR/export" + +trace $CLI workspace export-dir /test-dir "$TEST_TMP_DIR/export" diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/notebook.py b/acceptance/cmd/workspace/export-dir-illegal-filename/notebook.py new file mode 100644 index 00000000000..4914a7436d9 --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/notebook.py @@ -0,0 +1,2 @@ +# Databricks notebook source +print("hello") diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/posix/out.test.toml b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/out.test.toml new file mode 100644 index 00000000000..1baaa898c5b --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = false +GOOS.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/posix/output.txt b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/output.txt new file mode 100644 index 00000000000..c540d77622e --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/output.txt @@ -0,0 +1,5 @@ + +>>> [CLI] workspace export-dir /test-dir [TEST_TMP_DIR]/export +Exporting files from /test-dir +/test-dir/New Notebook a:b:c -> [TEST_TMP_DIR]/export/New Notebook a:b:c.py +Export complete diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/posix/script b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/script new file mode 100644 index 00000000000..6125f11b143 --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/script @@ -0,0 +1 @@ +source "$TESTDIR/../_script" diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/posix/test.toml b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/test.toml new file mode 100644 index 00000000000..96be3f28dce --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/posix/test.toml @@ -0,0 +1,3 @@ +# The ':' in the notebook name is a legal local filename on Linux/macOS, so the +# notebook is exported. Windows diverges and is covered by ../windows. +GOOS.windows = false diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/test.toml b/acceptance/cmd/workspace/export-dir-illegal-filename/test.toml new file mode 100644 index 00000000000..20b44bad782 --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/test.toml @@ -0,0 +1,14 @@ +Local = true +Cloud = false + +# This reproduces databricks/cli#5171. The behaviour is OS-specific (see _script), +# so the actual test cases live in the posix/ and windows/ leaf directories, each +# GOOS-gated with its own golden. This directory only holds the shared _script, +# the notebook fixture, and the config they inherit. + +# The exported notebook lands under $TEST_TMP_DIR/export and is not part of the +# comparison; only the captured command output is. +Ignore = ["export/"] + +[Env] +MSYS_NO_PATHCONV = "1" diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/windows/out.test.toml b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/out.test.toml new file mode 100644 index 00000000000..8471d88c7f3 --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false +GOOS.darwin = false +GOOS.linux = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/windows/output.txt b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/output.txt new file mode 100644 index 00000000000..e2d1f680ddf --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/output.txt @@ -0,0 +1,6 @@ + +>>> [CLI] workspace export-dir /test-dir [TEST_TMP_DIR]/export +Exporting files from /test-dir +Warn: /test-dir/New Notebook a:b:c: name is not valid for the local file system, exporting as "New Notebook a_b_c.py" +/test-dir/New Notebook a:b:c -> [TEST_TMP_DIR]\export\New Notebook a_b_c.py +Export complete diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/windows/script b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/script new file mode 100644 index 00000000000..6125f11b143 --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/script @@ -0,0 +1 @@ +source "$TESTDIR/../_script" diff --git a/acceptance/cmd/workspace/export-dir-illegal-filename/windows/test.toml b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/test.toml new file mode 100644 index 00000000000..9893864def7 --- /dev/null +++ b/acceptance/cmd/workspace/export-dir-illegal-filename/windows/test.toml @@ -0,0 +1,5 @@ +# The ':' in the notebook name is illegal as a local Windows filename, so +# export-dir renames it to a legal name with a warning. Linux/macOS are covered +# by ../posix. +GOOS.linux = false +GOOS.darwin = false diff --git a/cmd/workspace/workspace/export_dir.go b/cmd/workspace/workspace/export_dir.go index e7965bcda91..77056f4d28c 100644 --- a/cmd/workspace/workspace/export_dir.go +++ b/cmd/workspace/workspace/export_dir.go @@ -16,6 +16,7 @@ import ( "github.com/databricks/cli/libs/cmdctx" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/filer" + "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/notebook" "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/service/workspace" @@ -125,7 +126,18 @@ func (opts *exportDirOptions) callback(ctx context.Context, workspaceFiler filer // create the file f, err := os.Create(targetPath) if err != nil { - return err + // A workspace name can be illegal as a local filename (e.g. a ':' + // in "New Notebook 2026-05-04 13:54:24" on Windows). Rename it to a + // legal name with a warning rather than aborting the export (#5171). + if !isInvalidLocalNameError(err) { + return err + } + targetPath = filepath.Join(filepath.Dir(targetPath), sanitizeLocalName(filepath.Base(targetPath))) + log.Warnf(ctx, "%s: name is not valid for the local file system, exporting as %q", sourcePath, filepath.Base(targetPath)) + f, err = os.Create(targetPath) + if err != nil { + return err + } } defer f.Close() diff --git a/cmd/workspace/workspace/export_dir_other.go b/cmd/workspace/workspace/export_dir_other.go new file mode 100644 index 00000000000..3ada8e8d3eb --- /dev/null +++ b/cmd/workspace/workspace/export_dir_other.go @@ -0,0 +1,18 @@ +//go:build !windows + +package workspace + +// isInvalidLocalNameError reports whether err means the workspace object could +// not be written because its name is not a legal filename on the local OS. On +// non-Windows platforms the only bytes illegal in a filename are '/' and NUL, +// neither of which can appear in a workspace object name, so this never fires. +func isInvalidLocalNameError(err error) bool { + return false +} + +// sanitizeLocalName is a no-op on non-Windows platforms. It exists to satisfy +// the call in export_dir.go, which is only reached when isInvalidLocalNameError +// returns true, so this is never invoked here. +func sanitizeLocalName(name string) string { + return name +} diff --git a/cmd/workspace/workspace/export_dir_windows.go b/cmd/workspace/workspace/export_dir_windows.go new file mode 100644 index 00000000000..358846ca1e3 --- /dev/null +++ b/cmd/workspace/workspace/export_dir_windows.go @@ -0,0 +1,38 @@ +//go:build windows + +package workspace + +import ( + "errors" + "strings" + "syscall" +) + +// errorInvalidName is the Windows ERROR_INVALID_NAME code. The file APIs return +// it when a path contains characters that are illegal in a local filename, such +// as the ':' in a notebook named "New Notebook 2026-05-04 13:54:24". It is not +// declared in the standard syscall package, so we use the well-known code. +// https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499- +const errorInvalidName = syscall.Errno(0x7b) + +// isInvalidLocalNameError reports whether err means the workspace object could +// not be written because its name is not a legal filename on the local OS. +func isInvalidLocalNameError(err error) bool { + return errors.Is(err, errorInvalidName) +} + +// reservedNameChars are the characters that are illegal in a Windows filename. +// https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file +const reservedNameChars = `<>:"/\|?*` + +// sanitizeLocalName replaces characters that are illegal in a Windows filename +// with '_' so an object whose name is invalid locally can still be written under +// a legal name. +func sanitizeLocalName(name string) string { + return strings.Map(func(r rune) rune { + if strings.ContainsRune(reservedNameChars, r) { + return '_' + } + return r + }, name) +} diff --git a/cmd/workspace/workspace/export_dir_windows_test.go b/cmd/workspace/workspace/export_dir_windows_test.go new file mode 100644 index 00000000000..1a26f47383e --- /dev/null +++ b/cmd/workspace/workspace/export_dir_windows_test.go @@ -0,0 +1,86 @@ +//go:build windows + +package workspace + +import ( + "errors" + "io/fs" + "os" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" +) + +// The narrow contract: only the Windows "invalid file name" error is treated as +// skippable. Genuine failures (permission, missing path, anything else) must not +// be swallowed, otherwise export-dir would silently drop files on real errors. +func TestIsInvalidLocalNameError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "invalid name wrapped in PathError", + err: &os.PathError{Op: "open", Path: `C:\tmp\New Notebook 13:54:24.py`, Err: syscall.Errno(0x7b)}, + want: true, + }, + { + name: "permission denied is not skipped", + err: fs.ErrPermission, + want: false, + }, + { + name: "not exist is not skipped", + err: fs.ErrNotExist, + want: false, + }, + { + name: "generic error is not skipped", + err: errors.New("boom"), + want: false, + }, + { + name: "nil is not skipped", + err: nil, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isInvalidLocalNameError(tt.err)) + }) + } +} + +func TestSanitizeLocalName(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + { + name: "colons replaced", + in: "New Notebook 2026-05-04 13:54:24.py", + want: "New Notebook 2026-05-04 13_54_24.py", + }, + { + name: "all reserved characters replaced", + in: `ac:d"e|f?g*h`, + want: "a_b_c_d_e_f_g_h", + }, + { + name: "legal name unchanged", + in: "hello world.py", + want: "hello world.py", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sanitizeLocalName(tt.in)) + }) + } +} diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go index d2925fb1e38..9b7a6fcc91c 100644 --- a/libs/testserver/fake_workspace.go +++ b/libs/testserver/fake_workspace.go @@ -8,6 +8,7 @@ import ( "os" "path" "path/filepath" + "slices" "strings" "sync" "time" @@ -370,6 +371,31 @@ func (s *FakeWorkspace) WorkspaceGetStatus(path string) Response { } } +func (s *FakeWorkspace) WorkspaceList(listPath string) Response { + defer s.LockUnlock()() + + var objects []workspace.ObjectInfo + + for filePath, entry := range s.files { + if path.Dir(filePath) == listPath { + objects = append(objects, entry.Info) + } + } + for dirPath, dirInfo := range s.directories { + if dirPath != listPath && path.Dir(dirPath) == listPath { + objects = append(objects, dirInfo) + } + } + + slices.SortFunc(objects, func(a, b workspace.ObjectInfo) int { + return strings.Compare(a.Path, b.Path) + }) + + return Response{ + Body: workspace.ListResponse{Objects: objects}, + } +} + func (s *FakeWorkspace) WorkspaceMkdirs(request workspace.Mkdirs) { defer s.LockUnlock()() s.directories[request.Path] = workspace.ObjectInfo{ diff --git a/libs/testserver/handlers.go b/libs/testserver/handlers.go index 8f611a7c7c9..0756af5d0ff 100644 --- a/libs/testserver/handlers.go +++ b/libs/testserver/handlers.go @@ -79,6 +79,11 @@ func AddDefaultHandlers(server *Server) { return req.Workspace.WorkspaceGetStatus(path) }) + server.Handle("GET", "/api/2.0/workspace/list", func(req Request) any { + path := req.URL.Query().Get("path") + return req.Workspace.WorkspaceList(path) + }) + server.Handle("POST", "/api/2.0/workspace/mkdirs", func(req Request) any { var request workspace.Mkdirs if err := json.Unmarshal(req.Body, &request); err != nil {