Repository navigation
fix(acp-bridge): map Windows-shaped workspace paths to their sandbox mount #7228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
1dd5393
4ab9f67
3046e3b
59f11f0
b646a41
60705e1
e9c3b00
3675d96
88a5fc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /** | ||
| * @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', | ||
| () => { | ||
| const result = canonicalizeWorkspace('C:\\qwen-repro'); | ||
| // 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); | ||
| }, | ||
| ); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,55 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { realpathSync } from 'node:fs'; | ||
| import { existsSync, realpathSync } from 'node:fs'; | ||
| import * as path from 'node:path'; | ||
|
|
||
| const WINDOWS_ABSOLUTE_PATH_RE = /^([A-Za-z]):[\\/](.*)$/; | ||
|
|
||
| /** | ||
| * Maps a Windows-shaped absolute path to the container mount produced by the | ||
| * host-side sandbox launcher (`C:\work\proj` → `/c/work/proj`, mirroring | ||
| * `getContainerPath` in `cli/src/utils/sandbox.ts`). | ||
| * | ||
| * A Windows host relaunching `qwen serve` into a Linux Docker/Podman sandbox | ||
| * translates the bind mount and `--workdir`, but path-valued CLI arguments | ||
| * (`--workspace C:\…`), client-registered workspaces, and persisted | ||
| * registrations reach the in-container daemon in host shape. Left alone, | ||
| * `path.resolve` on Linux mangles them further (prepends the cwd) and every | ||
| * ACP child spawn fails with `chdir(2) ENOENT` before running anything | ||
| * (#7139). | ||
| * | ||
| * Deliberately conservative — the input is returned unchanged unless ALL of: | ||
| * - the daemon is running on POSIX inside a container sandbox (`SANDBOX` env | ||
| * set by the launcher; macOS `sandbox-exec` does not remap paths and is | ||
| * excluded), | ||
| * - the path is Windows-absolute (`<drive>:\…` or `<drive>:/…`), | ||
| * - the translated candidate actually exists (i.e. the drive really is | ||
| * mounted the way the launcher mounts workspaces). | ||
| * | ||
| * The `opts` seams exist for tests; production callers use the defaults. | ||
| */ | ||
| export function translateWindowsWorkspaceForPosixSandbox( | ||
| p: string, | ||
| opts: { | ||
| platform?: NodeJS.Platform; | ||
| sandboxEnv?: string | undefined; | ||
| exists?: (candidate: string) => boolean; | ||
| } = {}, | ||
| ): string { | ||
| const platform = opts.platform ?? process.platform; | ||
| const sandboxEnv = | ||
| 'sandboxEnv' in opts ? opts.sandboxEnv : process.env['SANDBOX']; | ||
| const exists = opts.exists ?? existsSync; | ||
| if (platform === 'win32' || !sandboxEnv || sandboxEnv === 'sandbox-exec') { | ||
| return p; | ||
| } | ||
| const match = WINDOWS_ABSOLUTE_PATH_RE.exec(p); | ||
| if (!match) return p; | ||
| const translated = `/${match[1]!.toLowerCase()}/${match[2]!.replace(/\\/g, '/')}`; | ||
| return exists(translated) ? translated : p; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 60705e1 — the candidate is resolved and must stay under its drive prefix before the existence probe runs; |
||
| } | ||
|
|
||
| /** | ||
| * Canonicalize a workspace path so the boot-time bound path and every | ||
| * request's `workspaceCwd` collapse to the same key. `path.resolve` | ||
|
|
@@ -40,7 +86,11 @@ import * as path from 'node:path'; | |
| * at the original location. | ||
| */ | ||
| export function canonicalizeWorkspace(p: string): string { | ||
| const resolved = path.resolve(p); | ||
| // #7139: inside a Linux container sandbox, host-shaped Windows workspace | ||
| // paths must be mapped to their bind-mount location BEFORE resolution — | ||
| // `path.resolve('C:\\x')` on POSIX treats the whole string as relative | ||
| // and prepends the cwd. | ||
| const resolved = path.resolve(translateWindowsWorkspaceForPosixSandbox(p)); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in 3046e3b — the finding was exactly right: I verified
Also added the wiring test you asked for: 中文:发现完全正确——已核实五处调用点都在 |
||
| try { | ||
| // FIXME(stage-2): switch to `fs.promises.realpath` once the | ||
| // bridge call sites become async-friendly. This sync syscall | ||
|
|
||
There was a problem hiding this comment.
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
SANDBOXto '' explicitly, exactly for the inherited-env case you describe. 中文:两处 outside-a-sandbox 测试都显式 stubEnv('SANDBOX',''),覆盖套件本身跑在项目 Docker 沙箱内继承真实 SANDBOX 的情形。