feat(cli): add Windows support and automated CI test coverage by mbiernacik · Pull Request #2350 · dataform-co/dataform

mbiernacik · 2026-09-29T14:37:37Z

Summary

This PR brings full, first-class support for the Dataform CLI (@dataform/cli) and Core (@dataform/core) on Windows (supporting both PowerShell and Command Prompt cmd.exe). It resolves all known historical Windows issues (#1565, #510, #1813), fixes platform incompatibilities, and introduces an automated GitHub Actions CI workflow running on windows-latest to prevent future regressions.

What was introduced

Automated CI Workflow on Windows (.github/workflows/test.yaml):
- test-linux job: builds package tarballs and runs core unit tests via Bazel.
- test-windows job (windows-latest): downloads the packaged CLI tarball, installs it globally via npm, and runs an end-to-end smoke test suite under both PowerShell and cmd.exe.
Windows Smoke Test Suite (scripts/smoke_test_windows.js):
- Automated runner testing all 8 primary CLI flows (init, install with workflow_settings.yaml and package.json, compile, compile --json, JiT actions, compile --watch, run --dry-run, test, format, and init-creds).
- Verifies canonical forward slashes in compiled graphs, CRLF handling, and shell execution.
Core Path & Normalization Helpers (core/path.ts, core/session.ts, core/utils.ts):
- Centralized Path.toPosixPath() and canonical Path.separator = "/" across all platforms.
- Case-insensitive Windows drive letter handling (C:\ vs c:/) in relativePath and caller file resolution.
- Preserved leading slashes strictly for POSIX absolute paths in getContents.
Subprocess & Worker Sandbox Reliability (cli/api/commands/base_worker.ts, compile.ts):
- Fixed worker IPC on Windows (removed unsupported anonymous pipe in stdio).
- Replaced uncatchable SIGKILL with graceful process termination.
- Added retry backoff (maxRetries: 3) to fs.rmSync in compile.ts to prevent Windows file-locking race conditions (EBUSY/EPERM).
CRLF Line Ending Normalization (cli/commands/format_command.ts):
- Prevents dataform format --check false positives when Git checks out files with CRLF on Windows.
Documentation (contributing.md):
- Added Windows contributor guide detailing environment setup, path conventions, and how to run local smoke tests.

Issues Identified & Solutions

Category / Issue	Symptom on Windows	Root Cause	Implemented Solution
1. Path Separators (#1565)	`dataform compile` compiled 0 actions on Windows projects.	`Path.separator` was set to `\\` on Windows (`core/path.ts`), but file globbing in `cli/vm/compile.ts` always emits `/`. `file.path.startsWith("definitions\\")` never matched `definitions/my_table.sqlx`.	Standardized `Path.separator = "/"` globally; added `Path.toPosixPath()` to normalize backslashes.
2. Graph Target Paths (#510)	Compilation failed with `missing dependency detected`; `ref()` resolution broke.	Windows backslashes leaked into action `fileName` and target identifiers, creating mismatched keys in Dataform's dependency graph.	Enforced canonical forward slash `/` formatting across all action targets and proto fields.
3. CRLF Line Endings	`dataform format --check` exited with code `1` on cleanly formatted files.	Git for Windows checks out CRLF (`\r\n`), which differed from Prettier's LF (`\n`) output.	Added `normalizeLineEndings()` helper before comparing files in `format_command.ts`.
4. Spawn & Shell Execution	Spawning `.cmd`/`.bat` files in `cmd.exe` failed with `'\"C:\...\"' is not recognized`.	Executing shims on Windows requires `{ shell: true }`. Manually invoking `cmd.exe /c` with quoted arguments caused Node's argument serializer to escape quotes to `\"`.	Used native `{ shell: true }` on Windows in CLI runners and test harnesses to avoid argument double-escaping.
5. Worker Sandbox IPC (#1813)	Worker subprocess crash (`spawn EINVAL`) in VSCode extension / background compilation.	`stdio` was set to `[0, 1, 2, "ipc", "pipe"]`. Windows does not support anonymous pipes alongside an IPC channel. Windows also lacks `SIGKILL`.	Cleaned up `stdio` to `["ipc"]` and replaced `SIGKILL` with platform-neutral `child.kill()`.
6. Temp Directory File Locking	`fs.rmSync` failed with `EBUSY` / `EPERM` during cleanup of compilation temp directories.	Windows holds mandatory locks on recently closed file handles, causing race conditions on rapid directory removal.	Added `{ recursive: true, force: true, maxRetries: 3 }` to `fs.rmSync` in `cli/api/commands/compile.ts`.
7. Drive Letters & Root Prefix	`getCallerFile` and `session.getContents` failed with `Unable to find valid caller file`.	Absolute Windows paths were incorrectly prepended with `/` (`\C:\...`), and drive letter comparisons were case-sensitive (`C:` vs `c:`).	Added case-insensitive drive letter prefix matching and preserved leading slashes exclusively for POSIX absolute paths.
8. Local Dependency Testing	Testing unreleased or local tarballs in `workflow_settings.yaml` failed validation.	`dataformCoreVersion` validation only allowed semver strings.	Added support for `file:` tarball paths in `core/workflow_settings.ts`.

How it was tested

GitHub Actions: Verified via the new test-windows job running on windows-latest under both PowerShell and Command Prompt (cmd.exe).
Automated Smoke Tests: Ran scripts/smoke_test_windows.js (18/18 tests passed).

Note

After merge: Required Status Check
To ensure Windows regressions are permanently prevented, please enable CLI smoke & unit tests (Windows) as a Required Status Check in the repository settings

- Enforce canonical forward slash '/' separators across all platforms via Path.toPosixPath in core/path.ts - Use Path.toPosixPath in core/session.ts, core/utils.ts, and core/main.ts instead of inline regex replacements - Support case-insensitive Windows drive letters in relativePath and caller file detection - Allow file: tarball dependencies in workflow_settings.yaml in core/workflow_settings.ts - Remove unsupported stdio pipe and SIGKILL in cli/api/commands/base_worker.ts - Add maxRetries and recursive cleanup to cli/api/commands/compile.ts to prevent Windows file locking issues - Add CRLF normalization helper to cli/commands/format_command.ts - Add scripts/smoke_test_windows.js testing all primary CLI flows under PowerShell and cmd.exe - Add .github/workflows/test.yaml with windows-latest runner and status checks - Update contributing.md with Windows prerequisites and smoke testing instructions

apilaskowski · 2026-10-09T12:31:12Z

  return `dataform.sqlxAction({
  sqlxConfig: {
-    name: "${Path.escapedBasename(path)}",
+    name: "${Path.basename(path)}",


The basename is interpolated into a JS string literal. Backslashes can no longer appear here, but a " in a filename (legal on POSIX) would break the generated code. Consider name: ${JSON.stringify(Path.basename(path))}, to make it robust regardless of filename contents.

apilaskowski · 2026-10-09T12:31:12Z

+ * UNC root such as `\\server\share\proj` becomes `//server/share/proj` rather than a POSIX-looking
+ * `/server/share/proj`.
+ */
+export function toPosixPath(path: string): string {


[Nice to have] toPosixPath now runs on all platforms (via normalize, join, filename, relativePath). On POSIX, \\ is a legal filename character, so e.g. definitions/a\\b.sqlx would now be treated as a subdirectory. Probably acceptable, but worth making this an explicit, documented decision (or gating conversion to Windows-style inputs).

apilaskowski · 2026-10-09T12:31:12Z

+  return toPosixPath(path).split("/").slice(-1)[0];
 }

 export function basename(path: string) {


Behavior change: basename(".gitignore") was "" and is now ".gitignore". Seems more correct, but could you double-check callers (e.g. action naming in core/utils.ts) aren't relying on the old behavior?

apilaskowski · 2026-10-09T12:31:13Z

-  return basename(path).replace(/\\/g, "\\\\");
-}
-
 export function fileExtension(fullPath: string) {


Behavior change: fileExtension("noext") used to return "noext", now "". Worth verifying callers that dispatch on extension (sqlx/js detection) are unaffected.

Lets make sure we have a test for that.

apilaskowski · 2026-10-09T12:31:13Z

    }
-    if (!nextLastfile.includes(rootDir)) {
+    const nextLastfile = Path.toPosixPath(rawNextLastfile);
+    if (!Path.startsWithPath(nextLastfile, rootDir)) {


[Nice to have] This is stricter than the previous includes(rootDir): it now requires rootDir to be absolute and in the same form as stack-trace filenames. Can we confirm projectDir is always resolved to an absolute path before reaching core (incl. VSCode extension / JiT callers)? Otherwise caller resolution would silently fall back to __dataform_current_file or throw.

apilaskowski · 2026-10-09T12:31:13Z


  if (workflowSettingsDataformCoreVersion) {
-    fs.rmSync(temporaryProjectPath, { recursive: true });
+    fs.rmSync(temporaryProjectPath, { recursive: true, force: true, maxRetries: 3 });


[Nice to have] force: true will also mask genuine cleanup failures (not only the Windows EBUSY/EPERM races). Fine for a temp dir, just flagging; a debug log on failure might help diagnose leaks.

apilaskowski · 2026-10-09T12:31:13Z

    const forkScript = this.resolveScript();
    const child = fork(forkScript, [], {
-      stdio: [0, 1, 2, "ipc", "pipe"],
+      stdio: [0, 1, 2, "ipc"],


Removing the extra "pipe" looks safe from the parent side (nothing reads child.stdio[4]). Could you confirm the worker/child script doesn't use fd 4 either?

apilaskowski · 2026-10-09T12:31:13Z

@@ -0,0 +1 @@
+* text=auto eol=lf


[Nice to have] Repo-wide * text=auto eol=lf will likely trigger a one-time renormalization diff for existing files, and may need exceptions for any files that must keep CRLF. Maybe worth a note in the PR or a follow-up git add --renormalize ..

apilaskowski · 2026-10-09T12:31:13Z

+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+
+      - name: Setup Bazel
+        uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3.0.0


Not blocking here, but lets keep in mind that no Bazel cache is configured, so every PR does a cold build (60 min timeout). Let's consider adding a disk/repository cache (e.g. bazel-contrib/setup-bazel with disk-cache) to cut CI time.

apilaskowski · 2026-10-09T12:31:13Z

+      - name: Setup Node.js
+        uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
+        with:
+          node-version: 20


Is Node 20 the minimum supported version? Consider a small matrix (min + latest LTS). Also, dataform init in the smoke test installs @dataform/core from the npm registry, so that flow is network-dependent and doesn't exercise this PR's core; maybe use the local core tarball there too.

apilaskowski · 2026-10-09T13:28:10Z

One small request on the PR description, which seems to have drifted from final diff:

It says SIGKILL was replaced with child.kill(), but base_worker.ts still calls child.kill("SIGKILL"). That is fine, but desc should be updated.
It references scripts/smoke_test_windows.ts; the actual file is cli/test/windows/smoke_test/ts
It describes a test-linux job, which was changed
It may also be worth noting the basename and fileExtension changes for dotfiles and extensionless names.

mbiernacik added 5 commits September 28, 2026 18:10

Fix GitHub Actions workflow syntax for Bazel and Windows runners

1ab14ee

Preserve leading slash in getContents for POSIX absolute paths

344bd99

Fix cmd.exe command spawning in Windows smoke test suite

901b1d8

Clean up Windows contributor guide

bade732

mbiernacik requested review from Ford544, apilaskowski and rafal-hawrylak September 29, 2026 14:37

mbiernacik requested a review from a team as a code owner September 29, 2026 14:37

mbiernacik added 3 commits September 29, 2026 14:45

Pin GitHub actions to full commit SHAs for zizmor policy

00ca527

Format all modified files with prettier

8d26cb6

Set explicit read-only permissions and timeouts for zizmor

492b9d6

rafal-hawrylak requested changes Sep 30, 2026

View reviewed changes

apilaskowski requested changes Oct 1, 2026

View reviewed changes

Comment thread core/path.ts Outdated

Comment thread core/path.ts Outdated

Comment thread core/session.ts Outdated

Comment thread cli/api/commands/base_worker.ts

Comment thread contributing.md Outdated

mbiernacik added 2 commits October 1, 2026 21:25

Address review comments on VmRunner

953cb5a

Address review comments on VmRunner

ab44785

apilaskowski approved these changes Oct 6, 2026

View reviewed changes

rafal-hawrylak requested changes Oct 8, 2026

View reviewed changes

Comment thread cli/tests/windows/smoke_test.ts Outdated

Comment thread cli/tests/windows/smoke_test.ts

Comment thread core/path.ts Outdated

Comment thread contributing.md Outdated

Comment thread .github/workflows/test.yaml

Address review comments

ac88071

mbiernacik requested review from apilaskowski and rafal-hawrylak October 9, 2026 10:02

apilaskowski reviewed Oct 9, 2026

View reviewed changes

rafal-hawrylak approved these changes Oct 9, 2026

View reviewed changes

Conversation

mbiernacik commented Sep 29, 2026

Summary

What was introduced

Issues Identified & Solutions

How it was tested

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apilaskowski commented Oct 9, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants