Skip to content

Commit c985516

Browse files
Copilotdeyaaeldeenpelikhan
authored
Add Windows container support with osType option to fix Docker path issues (#1896)
* Initial plan * Fix Docker container CWD path issue #1873 - use POSIX paths for Linux containers Co-authored-by: deyaaeldeen <6074665+deyaaeldeen@users.noreply.github.com> * Add osType option to ContainerOptions for Windows container support - Add osType?: "unix" | "windows" option to ContainerOptions interface - Default to "unix" for backward compatibility with existing Linux containers - Use appropriate path separators in docker.ts based on osType - Update comprehensive tests for both Unix and Windows container path handling - Update all type definition files to include new option Addresses feedback: Windows containers need backslash path separators, while Unix containers need forward slash separators. Co-authored-by: deyaaeldeen <6074665+deyaaeldeen@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: deyaaeldeen <6074665+deyaaeldeen@users.noreply.github.com> Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>
1 parent 6b0bc7f commit c985516

6 files changed

Lines changed: 154 additions & 4 deletions

File tree

docs/public/genaiscript.d.ts

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/src/content/docs/guides/genaiscript.d.ts

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

genaiscript.d.ts

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/core/src/types.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5182,6 +5182,12 @@ export interface ContainerOptions {
51825182
* Commands to executes after the container is created
51835183
*/
51845184
postCreateCommands?: ElementOrArray<string>;
5185+
5186+
/**
5187+
* Container operating system type. Determines path separator used for working directories.
5188+
* Defaults to "unix" for compatibility with most Linux-based containers.
5189+
*/
5190+
osType?: "unix" | "windows";
51855191
}
51865192

51875193
export interface PromiseQueue {

packages/runtime/src/docker.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import type {
3535
ShellOutput,
3636
TraceOptions,
3737
} from "@genaiscript/core";
38-
import { basename, dirname, join, resolve } from "node:path";
38+
import { basename, dirname, join, resolve, posix, win32 } from "node:path";
3939
const dbg = genaiscriptDebug("docker");
4040

4141
type DockerodeType = import("dockerode");
@@ -261,10 +261,11 @@ export class DockerManager {
261261
postCreateCommands,
262262
env,
263263
networkEnabled,
264+
osType,
264265
} = options;
265266
let name = (userName || image).replace(/[^a-zA-Z0-9]+/g, "_");
266267
if (persistent) {
267-
name += `_${await hash({ image, name, ports, env, networkEnabled, postCreateCommands, CORE_VERSION }, { length: 12, version: true })}`;
268+
name += `_${await hash({ image, name, ports, env, networkEnabled, postCreateCommands, osType, CORE_VERSION }, { length: 12, version: true })}`;
268269
} else {
269270
name += `_${generateId()}`;
270271
}
@@ -368,7 +369,7 @@ export class DockerManager {
368369
name: string,
369370
hostPath: string,
370371
): Promise<ContainerHost> {
371-
const { trace, persistent } = options;
372+
const { trace, persistent, osType = "unix" } = options;
372373
const dbgc = name ? dbg.extend(name) : dbg;
373374
const runtimeHost = resolveRuntimeHost();
374375

@@ -436,7 +437,10 @@ export class DockerManager {
436437
}
437438

438439
const { cwd: userCwd, label } = options || {};
439-
const cwd = "/" + join(DOCKER_CONTAINER_VOLUME, userCwd || ".");
440+
// Use appropriate path separator based on container OS type
441+
const pathJoin = osType === "windows" ? win32.join : posix.join;
442+
const pathSeparator = osType === "windows" ? "\\" : "/";
443+
const cwd = pathSeparator + pathJoin(DOCKER_CONTAINER_VOLUME, userCwd || ".");
440444

441445
try {
442446
trace?.startDetails(`📦 ▶️ container exec: ${userCwd || ""}> ${label || command}`);
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
import { test, describe } from "vitest"
5+
import { expect } from "vitest"
6+
import { posix, win32 } from "node:path"
7+
8+
describe("docker path handling", () => {
9+
test("container working directory uses POSIX paths for Unix containers", () => {
10+
// Test that we're using POSIX path joining for container paths
11+
const DOCKER_CONTAINER_VOLUME = "app"
12+
const osType = "unix"
13+
14+
// Simulate various user cwd inputs that could cause issues on Windows
15+
const testCases = [
16+
{ userCwd: "tmp/project", expected: "/app/tmp/project" },
17+
{ userCwd: "tmp\\windows\\path", expected: "/app/tmp\\windows\\path" }, // Backslashes should be preserved as-is, not converted
18+
{ userCwd: "some/deep/path/structure", expected: "/app/some/deep/path/structure" },
19+
{ userCwd: "", expected: "/app" }, // posix.join normalizes empty string
20+
{ userCwd: undefined, expected: "/app" }, // posix.join("app", ".") normalizes to "app"
21+
]
22+
23+
testCases.forEach(({ userCwd, expected }) => {
24+
// This simulates the logic from docker.ts
25+
const pathJoin = osType === "windows" ? win32.join : posix.join;
26+
const pathSeparator = osType === "windows" ? "\\" : "/";
27+
const cwd = pathSeparator + pathJoin(DOCKER_CONTAINER_VOLUME, userCwd || ".");
28+
expect(cwd).toBe(expected)
29+
})
30+
})
31+
32+
test("container working directory uses Windows paths for Windows containers", () => {
33+
// Test that we're using Windows path joining for Windows containers
34+
const DOCKER_CONTAINER_VOLUME = "app"
35+
const osType = "windows"
36+
37+
// Simulate various user cwd inputs for Windows containers
38+
const testCases = [
39+
{ userCwd: "tmp/project", expected: "\\app\\tmp\\project" },
40+
{ userCwd: "tmp\\windows\\path", expected: "\\app\\tmp\\windows\\path" },
41+
{ userCwd: "some/deep/path/structure", expected: "\\app\\some\\deep\\path\\structure" },
42+
{ userCwd: "", expected: "\\app" },
43+
{ userCwd: undefined, expected: "\\app" },
44+
]
45+
46+
testCases.forEach(({ userCwd, expected }) => {
47+
// This simulates the logic from docker.ts for Windows containers
48+
const pathJoin = osType === "windows" ? win32.join : posix.join;
49+
const pathSeparator = osType === "windows" ? "\\" : "/";
50+
const cwd = pathSeparator + pathJoin(DOCKER_CONTAINER_VOLUME, userCwd || ".");
51+
expect(cwd).toBe(expected)
52+
})
53+
})
54+
55+
test("POSIX join vs regular join behavior", () => {
56+
// Verify that posix.join always uses forward slashes regardless of host OS
57+
const testPath = posix.join("app", "tmp", "project")
58+
expect(testPath).toBe("app/tmp/project")
59+
expect(testPath).not.toContain("\\") // Should never contain backslashes
60+
})
61+
62+
test("Windows join behavior", () => {
63+
// Verify that win32.join uses backslashes
64+
const testPath = win32.join("app", "tmp", "project")
65+
expect(testPath).toBe("app\\tmp\\project")
66+
expect(testPath).toContain("\\") // Should contain backslashes
67+
})
68+
69+
test("container path construction with various inputs - Unix", () => {
70+
const DOCKER_CONTAINER_VOLUME = "app"
71+
const osType = "unix"
72+
73+
// Test edge cases that could occur in real usage
74+
const edgeCases = [
75+
{ userCwd: "./", expected: "/app/" }, // posix.join normalizes "./" to "app/"
76+
{ userCwd: "../", expected: "/./" }, // posix.join("app", "../") resolves to "./"
77+
{ userCwd: " ", expected: "/app/ " }, // Spaces should be preserved
78+
{ userCwd: "folder with spaces", expected: "/app/folder with spaces" },
79+
]
80+
81+
edgeCases.forEach(({ userCwd, expected }) => {
82+
const pathJoin = osType === "windows" ? win32.join : posix.join;
83+
const pathSeparator = osType === "windows" ? "\\" : "/";
84+
const cwd = pathSeparator + pathJoin(DOCKER_CONTAINER_VOLUME, userCwd || ".");
85+
expect(cwd).toBe(expected)
86+
})
87+
})
88+
89+
test("container path construction with various inputs - Windows", () => {
90+
const DOCKER_CONTAINER_VOLUME = "app"
91+
const osType = "windows"
92+
93+
// Test edge cases that could occur in real usage for Windows containers
94+
const edgeCases = [
95+
{ userCwd: "./", expected: "\\app\\" }, // win32.join normalizes "./" to "app\\"
96+
{ userCwd: "../", expected: "\\.\\" }, // win32.join("app", "../") resolves to ".\\"
97+
{ userCwd: " ", expected: "\\app\\ " }, // Spaces should be preserved
98+
{ userCwd: "folder with spaces", expected: "\\app\\folder with spaces" },
99+
]
100+
101+
edgeCases.forEach(({ userCwd, expected }) => {
102+
const pathJoin = osType === "windows" ? win32.join : posix.join;
103+
const pathSeparator = osType === "windows" ? "\\" : "/";
104+
const cwd = pathSeparator + pathJoin(DOCKER_CONTAINER_VOLUME, userCwd || ".");
105+
expect(cwd).toBe(expected)
106+
})
107+
})
108+
109+
test("default osType should be unix for backward compatibility", () => {
110+
// Verify that when osType is undefined, it defaults to unix behavior
111+
const DOCKER_CONTAINER_VOLUME = "app"
112+
const osType = undefined // Simulate default case
113+
const userCwd = "tmp/project"
114+
115+
// This should behave like unix (default behavior)
116+
const pathJoin = (osType === "windows") ? win32.join : posix.join;
117+
const pathSeparator = (osType === "windows") ? "\\" : "/";
118+
const cwd = pathSeparator + pathJoin(DOCKER_CONTAINER_VOLUME, userCwd || ".");
119+
120+
expect(cwd).toBe("/app/tmp/project") // Should use POSIX paths by default
121+
})
122+
})

0 commit comments

Comments
 (0)