Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions packages/acp-bridge/src/bridge.sandbox.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* @license
* Copyright 2025 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { afterEach, describe, expect, it, vi } from 'vitest';
import { makeBridge, makeChannel } from './internal/testUtils.js';
import { _setSandboxMountExistsForTest } from './workspacePaths.js';

// #7139 wiring for the bridge ingestion site: `resolveWorkspaceKey` guards
// with `path.isAbsolute` before canonicalizing, so a Windows-shaped
// `workspaceCwd` arriving via spawnOrAttach (clients, persisted
// registrations) must be translated to its bind mount first — the sibling
// of the dispatch/request-helpers/route/boot-validator wiring tests.
describe('bridge spawnOrAttach inside a POSIX container sandbox (#7139)', () => {
afterEach(() => {
vi.unstubAllEnvs();
_setSandboxMountExistsForTest(undefined);
});

it.skipIf(process.platform === 'win32')(
'accepts a Windows-shaped workspaceCwd bound to its mount location',
async () => {
vi.stubEnv('SANDBOX', 'qwen-code-sandbox-0');
_setSandboxMountExistsForTest((p) => p === '/c/qwen-repro');
const handle = makeChannel();
const bridge = makeBridge({
boundWorkspace: '/c/qwen-repro',
channelFactory: vi.fn().mockResolvedValue(handle.channel),
});
try {
const session = await bridge.spawnOrAttach({
workspaceCwd: 'C:\\qwen-repro',
});
expect(session.sessionId).toBeTruthy();
} finally {
await bridge.shutdown();
}
},
);

it.skipIf(process.platform === 'win32')(
'still rejects a Windows-shaped workspaceCwd outside a sandbox',
async () => {
vi.stubEnv('SANDBOX', '');
const handle = makeChannel();
const bridge = makeBridge({
boundWorkspace: '/c/qwen-repro',
channelFactory: vi.fn().mockResolvedValue(handle.channel),
});
try {
await expect(
bridge.spawnOrAttach({ workspaceCwd: 'C:\\qwen-repro' }),
).rejects.toThrow('workspaceCwd must be an absolute path');
} finally {
await bridge.shutdown();
}
},
);
});
16 changes: 12 additions & 4 deletions packages/acp-bridge/src/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ import {
SessionBusyError,
InvalidRewindTargetError,
} from './bridgeErrors.js';
import { canonicalizeWorkspace } from './workspacePaths.js';
import {
canonicalizeWorkspace,
translateAndCheckAbsoluteWorkspacePath,
} from './workspacePaths.js';
import { parseSessionSource } from './session-source.js';
import {
CHANNEL_STARTUP_PROFILE_META_KEY,
Expand Down Expand Up @@ -2845,10 +2848,15 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge {
return entry.transportClosedReject;
};

const resolveWorkspaceKey = (workspaceCwd: string): string => {
if (!path.isAbsolute(workspaceCwd)) {
const resolveWorkspaceKey = (rawWorkspaceCwd: string): string => {
// #7139: host-shaped Windows paths reach the in-container bridge via
// clients and persisted registrations; the shared helper maps them to
// the bind mount before the absolute-path check.
const workspaceCwd =
translateAndCheckAbsoluteWorkspacePath(rawWorkspaceCwd);
if (workspaceCwd === null) {
throw new Error(
`workspaceCwd must be an absolute path; got "${workspaceCwd}"`,
`workspaceCwd must be an absolute path; got "${rawWorkspaceCwd}"`,
);
}
const workspaceKey =
Expand Down
52 changes: 52 additions & 0 deletions packages/acp-bridge/src/workspacePaths.sandbox.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* @license
* Copyright 2025 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { afterEach, describe, expect, it, vi } from 'vitest';
import { canonicalizeWorkspace } from './workspacePaths.js';

// Isolated from workspacePaths.test.ts because it mocks node:fs — the other
// file drives real filesystem fixtures.
vi.mock('node:fs', async (importOriginal) => {
const actual = (await importOriginal()) as typeof import('node:fs');
return {
...actual,
existsSync: (p: unknown) =>
p === '/c/qwen-repro' ? true : actual.existsSync(p as never),
};
});

describe('canonicalizeWorkspace inside a POSIX container sandbox (#7139)', () => {
afterEach(() => {
vi.unstubAllEnvs();
});

it.skipIf(process.platform === 'win32')(
'resolves a Windows-shaped workspace to its bind-mount location',
() => {
vi.stubEnv('SANDBOX', 'qwen-code-sandbox-0');
// The mount exists (mocked), realpath on it ENOENTs on this host, so
// the fallback returns the translated absolute path — NOT the cwd
// concatenation `path.resolve` alone would produce.
expect(canonicalizeWorkspace('C:\\qwen-repro')).toBe('/c/qwen-repro');
},
);

it.skipIf(process.platform === 'win32')(
'keeps Windows-shaped input untouched outside a sandbox',
() => {
// Explicitly clear SANDBOX: when this suite itself runs inside the
// project's Docker sandbox the launcher sets it, and unstubAllEnvs
// would not remove a genuinely inherited value.
vi.stubEnv('SANDBOX', '');
const result = canonicalizeWorkspace('C:\\qwen-repro');
Comment on lines +37 to +44

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This "outside a sandbox" test never stubs SANDBOX, but the file-level vi.mock('node:fs') makes existsSync('/c/qwen-repro') return true for every test in the file. If SANDBOX happens to be set in the environment — e.g. running this suite inside the project's Docker sandbox container, where the launcher sets SANDBOX=qwen-code-sandbox-0 — the translation fires (truthy sandboxEnv, regex match, mocked-true mount probe) and returns /c/qwen-repro, so result.endsWith('C:\\qwen-repro') is false and the test fails spuriously. afterEach(vi.unstubAllEnvs) only restores variables a prior test stubbed; it does not unset a real inherited SANDBOX. — Concrete cost: a flaky, misleading failure that looks like a translation bug but is a test-isolation gap.

Suggested change
it.skipIf(process.platform === 'win32')(
'keeps Windows-shaped input untouched outside a sandbox',
() => {
const result = canonicalizeWorkspace('C:\\qwen-repro');
it.skipIf(process.platform === 'win32')(
'keeps Windows-shaped input untouched outside a sandbox',
() => {
vi.stubEnv('SANDBOX', '');
const result = canonicalizeWorkspace('C:\\qwen-repro');

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 60705e1 — the outside-a-sandbox tests (both this file and the REST-helper sibling) now stub SANDBOX to '' explicitly, exactly for the inherited-env case you describe. 中文:两处 outside-a-sandbox 测试都显式 stubEnv('SANDBOX',''),覆盖套件本身跑在项目 Docker 沙箱内继承真实 SANDBOX 的情形。

// Unsandboxed POSIX behavior is unchanged: the string resolves
// relative to the cwd (and stays broken — which is what the
// pre-#7139 sandbox path produced too).
expect(result.endsWith('C:\\qwen-repro')).toBe(true);
expect(result.startsWith('/')).toBe(true);
},
);
});
134 changes: 132 additions & 2 deletions packages/acp-bridge/src/workspacePaths.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,13 @@
import { promises as fsp, realpathSync } from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { afterEach, describe, expect, it } from 'vitest';
import { canonicalizeWorkspaces } from './workspacePaths.js';
import { afterEach, describe, expect, it, vi } from 'vitest';
import {
_setSandboxMountExistsForTest,
canonicalizeWorkspaces,
translateAndCheckAbsoluteWorkspacePath,
translateWindowsWorkspaceForPosixSandbox,
} from './workspacePaths.js';

const scratches: string[] = [];

Expand Down Expand Up @@ -46,3 +51,128 @@ describe('canonicalizeWorkspaces', () => {
expect(canonicalizeWorkspaces([])).toEqual([]);
});
});

// The single ingestion-ordering enforcement point: translation before the
// absolute-path check, shared by all five workspace-ingestion sites.
describe('translateAndCheckAbsoluteWorkspacePath', () => {
it('returns the translated mount for Windows-shaped input in a sandbox', () => {
vi.stubEnv('SANDBOX', 'qwen-code-sandbox-0');
_setSandboxMountExistsForTest((p) => p === '/c/qwen-repro');
try {
expect(translateAndCheckAbsoluteWorkspacePath('C:\\qwen-repro')).toBe(
'/c/qwen-repro',
);
} finally {
vi.unstubAllEnvs();
_setSandboxMountExistsForTest(undefined);
}
});

it('returns null for non-absolute input (untranslated Windows shape included)', () => {
vi.stubEnv('SANDBOX', '');
try {
expect(translateAndCheckAbsoluteWorkspacePath('C:\\qwen-repro')).toBe(
null,
);
expect(translateAndCheckAbsoluteWorkspacePath('relative/dir')).toBe(null);
} finally {
vi.unstubAllEnvs();
}
});

it('passes POSIX absolute paths through unchanged', () => {
expect(translateAndCheckAbsoluteWorkspacePath('/work/a')).toBe('/work/a');
});
});

// Regression for #7139: a Windows host relaunching `qwen serve` into a Linux
// Docker sandbox forwards `--workspace C:\…` (and client/persisted workspace
// registrations) in host shape; every ACP child then failed with
// `chdir(2) ENOENT`. These tests pin the container-side translation.
describe('translateWindowsWorkspaceForPosixSandbox', () => {
const sandboxOpts = (exists: boolean) => ({
platform: 'linux' as NodeJS.Platform,
sandboxEnv: 'qwen-code-sandbox-0',
exists: () => exists,
});

it('maps a Windows-absolute path to its bind-mount location', () => {
expect(
translateWindowsWorkspaceForPosixSandbox(
'C:\\qwen-repro',
sandboxOpts(true),
),
).toBe('/c/qwen-repro');
expect(
translateWindowsWorkspaceForPosixSandbox(
'D:/Work/proj sub',
sandboxOpts(true),
),
).toBe('/d/Work/proj sub');
expect(
translateWindowsWorkspaceForPosixSandbox(
'C:\\nested\\dir\\leaf',
sandboxOpts(true),
),
).toBe('/c/nested/dir/leaf');
});

it('leaves the path alone when the translated mount does not exist', () => {
expect(
translateWindowsWorkspaceForPosixSandbox(
'C:\\qwen-repro',
sandboxOpts(false),
),
).toBe('C:\\qwen-repro');
});

it('refuses ..-laden input that escapes the drive mount', () => {
// existsSync would resolve /c/../../etc to /etc and return true — the
// guard must refuse before the probe can bless an out-of-mount path.
expect(
translateWindowsWorkspaceForPosixSandbox(
'C:\\..\\..\\etc',
sandboxOpts(true),
),
).toBe('C:\\..\\..\\etc');
// In-mount .. that stays under the drive prefix is still fine.
expect(
translateWindowsWorkspaceForPosixSandbox(
'C:\\work\\..\\proj',
sandboxOpts(true),
),
).toBe('/c/work/../proj');
});

it('leaves non-Windows-shaped paths alone', () => {
for (const p of ['/c/qwen-repro', 'relative/dir', 'C:', 'CC:\\x', '']) {
expect(
translateWindowsWorkspaceForPosixSandbox(p, sandboxOpts(true)),
).toBe(p);
}
});

it('is inert on Windows hosts, outside sandboxes, and under seatbelt', () => {
expect(
translateWindowsWorkspaceForPosixSandbox('C:\\qwen-repro', {
platform: 'win32',
sandboxEnv: 'qwen-code-sandbox-0',
exists: () => true,
}),
).toBe('C:\\qwen-repro');
expect(
translateWindowsWorkspaceForPosixSandbox('C:\\qwen-repro', {
platform: 'linux',
sandboxEnv: undefined,
exists: () => true,
}),
).toBe('C:\\qwen-repro');
expect(
translateWindowsWorkspaceForPosixSandbox('C:\\qwen-repro', {
platform: 'darwin',
sandboxEnv: 'sandbox-exec',
exists: () => true,
}),
).toBe('C:\\qwen-repro');
});
});
Loading
Loading