Rename workspace objects with names illegal as local files in export-dir#5655
Merged
Merged
Conversation
Reproduces #5171: `workspace export-dir` aborts on Windows when a notebook name contains a ':' (e.g. an untitled "New Notebook <timestamp>"), because the colon is illegal in a local Windows filename and os.Create fails. The name is legal on Linux/macOS, so the test is gated to Windows via GOOS. output.txt is intentionally omitted: the golden cannot be generated on a non-Windows machine, so the Windows CI leg will surface the real output to commit verbatim.
Collaborator
Integration test reportCommit: ce5e23d
|
Captured verbatim from the Windows CI run of the test added in the previous commit. Documents the current (buggy) behavior of #5171: export-dir aborts on Windows when a notebook name contains ':'.
A workspace object name can be illegal as a local filename (e.g. the ':' in an untitled "New Notebook 2026-05-04 13:54:24" on Windows), which made os.Create fail and abort the entire export. Skip such files with a warning and continue, matching the existing skip paths for non-exportable and oversized objects. Detection uses a build-tagged helper checking the Windows ERROR_INVALID_NAME code; on other platforms no filename character (other than '/' and NUL, which cannot appear in a workspace object name) is illegal, so it is a no-op. Flips the Windows-gated acceptance test from asserting the abort to expecting skip-with-warning. Fixes #5171.
Locks in the narrow contract that only the Windows ERROR_INVALID_NAME error is treated as skippable, while permission, not-exist, generic, and nil errors are not. The acceptance test only exercises the positive (invalid-name) path, so this guards against accidentally broadening the check and silently dropping files on real I/O failures.
The test reproducing #5171 was gated to Windows only because a ':' in the notebook name is illegal as a local filename on Windows but legal on Linux/macOS, so export-dir skips it there and exports it everywhere else. Run it on every OS and capture the divergent output in out.<goos>.txt. Teach the acceptance runner to compare only the current OS's out.<goos>.txt and skip the others (extending the existing variant-skip machinery), since otherwise the non-current goldens would fail as "Missing output file". Add a fake WorkspaceList handler so the imported notebook is listed and actually exported, replacing the hardcoded [[Server]] list stub.
shreyas-goenka
approved these changes
Jun 19, 2026
…r GOOS filtering
Revert the per-GOOS output filtering added to the acceptance runner
(osVariants / preparePathFilter) and instead express the OS-specific behaviour
with two GOOS-gated leaf tests that share a single _script and notebook fixture:
export-dir-illegal-filename/
_script # import + export-dir, shared by both leaves
notebook.py # shared fixture, referenced via $TESTDIR/../notebook.py
posix/ # GOOS.windows = false -> notebook exported
windows/ # GOOS.{linux,darwin} = false -> skipped with a warning
The ':' in the notebook name is legal on Linux/macOS but illegal as a local
Windows filename, so export-dir skips it on Windows (#5171). The
two captured outputs differ, but everything else is shared, so the leaves only
carry their GOOS gate and golden. This keeps OS knowledge out of the shared
acceptance runner.
radakam
changed the title
…tead of skipping Previously export-dir skipped workspace objects whose names are not legal local filenames (e.g. the ':' in "New Notebook 2026-05-04 13:54:24" on Windows). Skipping silently dropped the file. Now the object is exported under a sanitized name (illegal characters replaced with '_') and a warning is emitted, so the content is preserved. Adds a build-tagged sanitizeLocalName helper (Windows replaces the reserved set plus control characters; no-op elsewhere) and a unit test covering it. Flips the Windows-gated acceptance golden from skip to rename-with-warning. Relates to #5171.
Rename the fixture notebook to "New Notebook a:b" so the acceptance timestamp redaction no longer applies and the golden stays literal. Drop the unreachable control-character branch in sanitizeLocalName, since workspace object names cannot contain control characters.
shreyas-goenka
approved these changes
Jun 23, 2026
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, leaving the Windows
golden unreproducible. Real #5171 names ("13:54:24") fail because they
contain two colons, which makes the stream type invalid. Switch the
fixture to "a:b:c" so it genuinely triggers the rename-with-warning path
on Windows, matching the unit test, while staying clear of the timestamp
redaction.
radakam
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A UI-created notebook is named like
New Notebook 2026-05-04 13:54:24. The:is legal on Linux/macOS but illegal on Windows, soos.Createfails withERROR_INVALID_NAMEand the whole recursive export aborts on that one file.Changes
export_dir.go: instead of aborting, sanitize the offending name (replace characters illegal in a local filename with_), export the object under the legal name, and emit aWarn:line so the rename is visible. OnlyERROR_INVALID_NAMEis handled this way — permission/disk/missing-path errors still propagate, so we never silently misplace files on real failures.export_dir_windows.go/export_dir_other.go) to avoiderr.Error()string-matching and stay cross-platform;ERROR_INVALID_NAMEhas no portable Go sentinel. On non-Windows platformssanitizeLocalNameis a no-op (the only bytes illegal in a filename,/and NUL, can't appear in a workspace object name).libs/testserver: added a fakeWorkspaceListhandler so the imported notebook is actually listed and exported.Changes to acceptance harness
posix/andwindows/), each GOOS-gated with its own captured output.Tests
export-dir-illegal-filename/): on Windows the object is exported asNew Notebook a_b.pywith aWarn:line and the export completes; on Linux/macOS the:is legal so it exports unchanged. The fixture uses a plainNew Notebook a:bname so the acceptance timestamp redaction doesn't obscure the golden (the real workspace export-dir failing on new notebook #5171 trigger is the auto-generatedNew Notebook <timestamp>name).export_dir_windows_test.go):TestIsInvalidLocalNameErrorasserts the narrow contract — onlyERROR_INVALID_NAMEis handled;ErrPermission/ErrNotExist/generic/nilare not.TestSanitizeLocalNamecovers reserved-character replacement and leaves legal names untouched.Fixes issue #5171.