Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import os from 'node:os';
import path from 'node:path';
import { mkdtempSync, rmSync } from 'node:fs';
import { afterAll, afterEach, describe, expect, it, vi } from 'vitest';

import * as fs from 'node:fs';

const throwMissingPath = vi.hoisted(() => () => {
const error = new Error('mocked missing path') as NodeJS.ErrnoException;
error.code = 'ENOENT';
throw error;
});

vi.mock('node:fs', async () => {
const actual = await vi.importActual<typeof import('node:fs')>('node:fs');
return {
...actual,
realpathSync: vi.fn(throwMissingPath),
};
});

import { createMockWorkspaceContext } from './mockWorkspaceContext.js';

const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));

describe('createMockWorkspaceContext filesystem fallback', () => {
it('uses lexical containment when canonicalization is unavailable', () => {
const workspace = createMockWorkspaceContext(rootDir);

expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
expect(workspace.isPathWithinWorkspace(`${rootDir}-sibling`)).toBe(false);
});

it('uses lexical containment when ENOENT has no path', () => {
const workspace = createMockWorkspaceContext(rootDir);

expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
});
Comment on lines +42 to +48

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] Fallback test 'uses lexical containment when ENOENT has no path' is redundant with the first test — both use the same mock setup (realpathSync mocked to throw throwMissingPath, which creates an ENOENT error with no error.path) and assert the same condition (path.join(rootDir, 'missing.txt') → true). The second test's assertion is a strict subset of the first test's assertions.

Failure scenario: No concrete bug — the tests are correct. But a reader would expect two different scenarios from two test names. The second test adds no new coverage.

Suggested change
it('uses lexical containment when ENOENT has no path', () => {
const workspace = createMockWorkspaceContext(rootDir);
expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
});
// Either remove this test, or change the mock to throw an ENOENT
// with an empty-string `path` (not `undefined`) to test a genuinely distinct case.
中文说明

[Suggestion] 回退测试 'uses lexical containment when ENOENT has no path' 与第一个测试冗余——两者使用相同的 mock 设置(realpathSync 模拟抛出不带 error.path 的 ENOENT),并断言相同的条件(path.join(rootDir, 'missing.txt') → true)。第二个测试的断言是第一个测试断言的严格子集。

失败场景:无具体 bug——测试正确。但读者会期望两个不同的测试名称对应两个不同的场景。第二个测试没有增加新的覆盖。

Suggested change
it('uses lexical containment when ENOENT has no path', () => {
const workspace = createMockWorkspaceContext(rootDir);
expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
});
// 删除此测试,或将 mock 改为抛出空字符串 `path`(而非 `undefined`)的 ENOENT,
// 以测试真正不同的情况。

— deepseek-v4-flash via Qwen Code /review (v0.21.8)


it('uses lexical containment for non-Node filesystem stubs', () => {
vi.mocked(fs.realpathSync).mockImplementation(() => {
throw new TypeError('mocked filesystem call');
});
const workspace = createMockWorkspaceContext(rootDir);

expect(workspace.isPathWithinWorkspace(`${rootDir}-sibling`)).toBe(false);
expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
});
});

afterEach(() => {
vi.mocked(fs.realpathSync).mockImplementation(throwMissingPath);
});

afterAll(() => {
rmSync(rootDir, { recursive: true, force: true });
});
145 changes: 145 additions & 0 deletions packages/core/src/test-utils/mockWorkspaceContext.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import os from 'node:os';
import path from 'node:path';
import { mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
import { describe, expect, it } from 'vitest';
import { createMockWorkspaceContext } from './mockWorkspaceContext.js';

describe('createMockWorkspaceContext', () => {
it('accepts missing descendants under a workspace root', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
try {
const workspace = createMockWorkspaceContext(rootDir);

expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
} finally {
rmSync(rootDir, { recursive: true, force: true });
}
});

it('does not treat a similarly prefixed sibling as inside the workspace', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
try {
const workspace = createMockWorkspaceContext(rootDir);

expect(
workspace.isPathWithinWorkspace(`${rootDir}-sibling/file.txt`),
).toBe(false);
} finally {
rmSync(rootDir, { recursive: true, force: true });
}
});

it('checks additional workspace directories', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
const additionalDir = mkdtempSync(
path.join(os.tmpdir(), 'qwen-workspace-'),
);
try {
const workspace = createMockWorkspaceContext(rootDir, [additionalDir]);

expect(
workspace.isPathWithinWorkspace(
path.join(additionalDir, 'missing.txt'),
),
).toBe(true);
} finally {
rmSync(additionalDir, { recursive: true, force: true });
rmSync(rootDir, { recursive: true, force: true });
}
});

it('canonicalizes workspace aliases for containment checks', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
const aliasDir = path.join(
os.tmpdir(),
`qwen-workspace-alias-${Date.now()}`,
);
symlinkSync(rootDir, aliasDir);

try {
const workspace = createMockWorkspaceContext(aliasDir);

expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
} finally {
rmSync(aliasDir, { recursive: true, force: true });
rmSync(rootDir, { recursive: true, force: true });
Comment thread
rbalachandar marked this conversation as resolved.
}
});

it('rejects dangling leaf symlinks', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
const danglingPath = path.join(rootDir, 'dangling');
symlinkSync(path.join(rootDir, 'missing-target'), danglingPath);

try {
const workspace = createMockWorkspaceContext(rootDir);

expect(workspace.isPathWithinWorkspace(danglingPath)).toBe(false);
} finally {
rmSync(rootDir, { recursive: true, force: true });
}
});

it('resolves existing candidate paths before checking containment', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
const outsideDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-outside-'));
const insidePath = path.join(rootDir, 'inside.txt');
const outsidePath = path.join(outsideDir, 'outside.txt');
const escapePath = path.join(rootDir, 'escape');
writeFileSync(insidePath, 'inside');
writeFileSync(outsidePath, 'outside');
symlinkSync(outsidePath, escapePath);

try {
const workspace = createMockWorkspaceContext(rootDir);

expect(workspace.isPathWithinWorkspace(insidePath)).toBe(true);
expect(workspace.isPathWithinWorkspace(escapePath)).toBe(false);
} finally {
rmSync(outsideDir, { recursive: true, force: true });
rmSync(rootDir, { recursive: true, force: true });
}
});

it('rejects paths through a symlink cycle', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
const cyclePath = path.join(rootDir, 'cycle');
symlinkSync('cycle', cyclePath);

try {
const workspace = createMockWorkspaceContext(rootDir);

expect(
workspace.isPathWithinWorkspace(path.join(cyclePath, 'file.txt')),
).toBe(false);
} finally {
rmSync(rootDir, { recursive: true, force: true });
}
});

it('ignores an invalid workspace root when another root is valid', () => {
const rootDir = mkdtempSync(path.join(os.tmpdir(), 'qwen-workspace-'));
const cyclePath = path.join(rootDir, 'cycle');
const candidatePath = path.join(rootDir, 'inside.txt');
symlinkSync('cycle', cyclePath);
writeFileSync(candidatePath, 'inside');

try {
const workspace = createMockWorkspaceContext(cyclePath, [rootDir]);

expect(workspace.isPathWithinWorkspace(candidatePath)).toBe(true);
} finally {
rmSync(rootDir, { recursive: true, force: true });
}
});
});
44 changes: 39 additions & 5 deletions packages/core/src/test-utils/mockWorkspaceContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
*/

import { vi } from 'vitest';
import { isNodeError } from '../utils/errors.js';
import {
isPathWithinRoot,
resolveWorkspacePath,
} from '../utils/workspaceContext.js';
import type { WorkspaceContext } from '../utils/workspaceContext.js';

/**
Expand All @@ -22,12 +27,41 @@ export function createMockWorkspaceContext(
const mockWorkspaceContext = {
addDirectory: vi.fn(),
getDirectories: vi.fn().mockReturnValue(allDirs),
isPathWithinWorkspace: vi
.fn()
.mockImplementation((path: string) =>
allDirs.some((dir) => path.startsWith(dir)),
),
isPathWithinWorkspace: vi.fn().mockImplementation((path: string) => {
try {
const canonicalPath = canonicalizeForContainment(path);
return allDirs.some((dir) => {
try {
return isPathWithinRoot(
canonicalPath,
canonicalizeForContainment(dir),
);
} catch {
return false;
}
});
Comment on lines +33 to +42

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] Inner try/catch wraps isPathWithinRoot but only canonicalizeForContainment is fallible — isPathWithinRoot is a pure string function (path.relative + three boolean checks) that never throws. The catch is only reachable through canonicalizeForContainment(dir), which is called as the second argument. A reader unfamiliar with the code cannot tell which operation is fallible.

Failure scenario: No crash — the code works correctly. But it obscures which part of the block is actually fallible, making the mock harder to reason about and maintain. The real WorkspaceContext.isPathWithinWorkspace (line 246 in the same diff) has no inner try/catch around isPathWithinRoot at all.

Suggested change
return allDirs.some((dir) => {
try {
return isPathWithinRoot(
canonicalPath,
canonicalizeForContainment(dir),
);
} catch {
return false;
}
});
return allDirs.some((dir) => {
const canonicalDir = canonicalizeForContainment(dir);
return isPathWithinRoot(canonicalPath, canonicalDir);
});
中文说明

[Suggestion] 内层 try/catch 包裹了 isPathWithinRoot,但只有 canonicalizeForContainment 可能抛出异常——isPathWithinRoot 是一个纯字符串函数(path.relative + 三个布尔检查),从不抛出。catch 只能通过第二个参数 canonicalizeForContainment(dir) 到达。不熟悉代码的读者无法判断哪个操作是可能失败的。

失败场景:不会崩溃——代码正确运行。但它掩盖了代码块中真正可能失败的部分,使 mock 更难理解和维护。真正的 WorkspaceContext.isPathWithinWorkspace(同一 diff 第 246 行)完全没有内层 try/catch。

Suggested change
return allDirs.some((dir) => {
try {
return isPathWithinRoot(
canonicalPath,
canonicalizeForContainment(dir),
);
} catch {
return false;
}
});
return allDirs.some((dir) => {
const canonicalDir = canonicalizeForContainment(dir);
return isPathWithinRoot(canonicalPath, canonicalDir);
});

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

} catch {
return false;
}
}),
} as unknown as WorkspaceContext;

return mockWorkspaceContext;
}

function canonicalizeForContainment(inputPath: string): string {
try {
return resolveWorkspacePath(inputPath);
} catch (error: unknown) {
if (isNodeError(error)) {
if (error.code === 'ENOENT' && !error.path) {
return inputPath;
}
throw error;
}

// Some tests stub filesystem calls; retain lexical behavior for those
// mocked environments.
return inputPath;
}
}
12 changes: 11 additions & 1 deletion packages/core/src/utils/workspaceContext.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { WorkspaceContext } from './workspaceContext.js';
import { resolveWorkspacePath, WorkspaceContext } from './workspaceContext.js';

describe('WorkspaceContext with real filesystem', () => {
let tempDir: string;
Expand Down Expand Up @@ -158,6 +158,16 @@ describe('WorkspaceContext with real filesystem', () => {
);
});

it('should preserve paths with missing intermediate components', () => {
const workspaceContext = new WorkspaceContext(cwd);
const nonExistentPath = path.join(cwd, 'missing', 'nested.txt');

expect(resolveWorkspacePath(nonExistentPath)).toBe(nonExistentPath);
expect(workspaceContext.isPathWithinWorkspace(nonExistentPath)).toBe(
true,
);
});
Comment on lines +161 to +169

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] Mutation-vacuous test — replacing resolveMissingPath with return pathToCheck (discarding the ancestor-walk loop entirely) still passes both assertions. The input is a simple subpath of the existing root, so the walked-up result is identical to the original input.

Failure scenario: If a future regression replaced the ancestor walk with a trivial identity return, this test would not catch it. The isPathWithinWorkspace assertion is the load-bearing one; the resolveWorkspacePath assertion is redundant for this input.

Suggested change
it('should preserve paths with missing intermediate components', () => {
const workspaceContext = new WorkspaceContext(cwd);
const nonExistentPath = path.join(cwd, 'missing', 'nested.txt');
expect(resolveWorkspacePath(nonExistentPath)).toBe(nonExistentPath);
expect(workspaceContext.isPathWithinWorkspace(nonExistentPath)).toBe(
true,
);
});
// Add a second test case where the missing portion sits below a symlink
// that maps to a different point in the filesystem, so that
// resolveMissingPath produces a *different* path from the input.
中文说明

[Suggestion] 变异无效的测试——将 resolveMissingPath 替换为 return pathToCheck(完全丢弃祖先遍历循环)后,两个断言仍然通过。输入是已存在根目录的简单子路径,遍历后的结果与原始输入相同。

失败场景:如果未来回归将祖先遍历替换为恒等返回,该测试不会捕获。isPathWithinWorkspace 断言才是真正负载的;resolveWorkspacePath 断言在此输入下是冗余的。

Suggested change
it('should preserve paths with missing intermediate components', () => {
const workspaceContext = new WorkspaceContext(cwd);
const nonExistentPath = path.join(cwd, 'missing', 'nested.txt');
expect(resolveWorkspacePath(nonExistentPath)).toBe(nonExistentPath);
expect(workspaceContext.isPathWithinWorkspace(nonExistentPath)).toBe(
true,
);
});
// 添加第二个测试用例,缺失部分坐在符号链接之下,
// 该符号链接映射到文件系统的不同位置,
// 这样 resolveMissingPath 会产生与输入*不同*的路径。

— deepseek-v4-flash via Qwen Code /review (v0.21.8)


describe('with symbolic link', () => {
describe('in the workspace', () => {
let realDir: string;
Expand Down
66 changes: 39 additions & 27 deletions packages/core/src/utils/workspaceContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,25 +273,7 @@ export class WorkspaceContext {
if (cached !== undefined) {
return cached;
}
let resolved: string;
try {
resolved = fs.realpathSync(pathToCheck);
} catch (e: unknown) {
if (
isNodeError(e) &&
e.code === 'ENOENT' &&
e.path &&
// realpathSync does not set e.path correctly for symlinks to
// non-existent files.
!this.isFileSymlink(e.path)
) {
// If it doesn't exist, e.path contains the fully resolved path.
resolved = e.path;
} else {
// Don't cache exceptions — the path may exist on retry.
throw e;
}
}
const resolved = resolveWorkspacePath(pathToCheck);
if (
this.resolvedPathCache.size >= WorkspaceContext.RESOLVED_PATH_CACHE_MAX
Comment thread
rbalachandar marked this conversation as resolved.
) {
Expand All @@ -302,16 +284,46 @@ export class WorkspaceContext {
this.resolvedPathCache.set(pathToCheck, resolved);
return resolved;
}
}

/**
* Checks if a file path is a symbolic link that points to a file.
*/
private isFileSymlink(filePath: string): boolean {
try {
return !fs.readlinkSync(filePath).endsWith('/');
} catch (_error) {
return false;
/**
* Resolves a workspace path using the same missing-path and symlink semantics
* used by WorkspaceContext containment checks.
*/
export function resolveWorkspacePath(pathToCheck: string): string {
try {
const resolved = fs.realpathSync(pathToCheck);
return typeof resolved === 'string' ? resolved : pathToCheck;
Comment thread
rbalachandar marked this conversation as resolved.
} catch (error: unknown) {
if (
isNodeError(error) &&
error.code === 'ENOENT' &&
error.path &&
// realpathSync does not set error.path correctly for symlinks to
// non-existent files.
!isFileSymlink(error.path)
) {
if (
error.path !== pathToCheck &&
pathToCheck.startsWith(`${error.path}${path.sep}`)
) {
Comment thread
rbalachandar marked this conversation as resolved.
Outdated
return pathToCheck;
}
Comment on lines +319 to +322

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] resolveMissingPath silently returns the original unresolved path when it walks up to the filesystem root without finding a resolvable ancestor — the parent === ancestor termination at the root boundary returns pathToCheck unchanged, with no documentation about this fallback.

Failure scenario: If a path like /a/b/c/d is supplied where every component from d up to / is non-existent or unreadable (ENOENT), the function returns the original /a/b/c/d unchanged. Downstream, isPathWithinRoot accepts the path via lexical containment. This is pre-existing behavior (the old e.path fallback did the same), but it is an undocumented implicit assumption.

Suggested change
const parent = path.dirname(ancestor);
if (parent === ancestor) {
return pathToCheck;
}
// Fall back to the original path when the entire hierarchy is missing.
// isPathWithinRoot will apply lexical containment in this case,
// matching the pre-existing behavior from the old e.path fallback.
return pathToCheck;
中文说明

[Suggestion] resolveMissingPath 在遍历到文件系统根目录仍未找到可解析的祖先时,会静默返回原始的未解析路径——parent === ancestor 在根目录边界的终止条件返回未修改的 pathToCheck,没有任何关于此回退行为的文档。

失败场景:如果路径 /a/b/c/d 的每个组件从 d 到 / 都不存在或不可读,函数返回原始的 /a/b/c/d。下游 isPathWithinRoot 会通过词法包含关系接受该路径。这是已有行为(旧的 e.path 回退也是如此),但这是一个未记录的隐式假设。

Suggested change
const parent = path.dirname(ancestor);
if (parent === ancestor) {
return pathToCheck;
}
// 整个路径层级都不存在时回退到原始路径。
// 此时 isPathWithinRoot 会使用词法包含判断,
// 与旧的 e.path 回退行为一致。
return pathToCheck;

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

return error.path;
}

throw error;
}
}

/**
* Checks if a file path is a symbolic link that points to a file.
*/
function isFileSymlink(filePath: string): boolean {
try {
return !fs.readlinkSync(filePath).endsWith('/');
} catch (_error) {
return false;
}
}
Comment on lines +343 to +349

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] isFileSymlink heuristic is misnamed — it detects symlinks whose target literal ends with /, not file symlinks. readlinkSync returns the raw link target, and a symlink to a directory does not normally have a trailing / unless created with ln -s target/ link. So isFileSymlink returns true for most directory symlinks, causing isResolvableMissingPathError to re-throw paths that are actually resolvable.

Failure scenario: No concrete bug — this is conservatively safe (the caller catches the error and returns false for containment). But the function name implies a different semantic, and the conservative re-throw means the function silently treats all symlinks to non-existent targets as non-resolvable, which is unnecessary for the common case of a symlink to a non-existent directory.

Suggested change
function isFileSymlink(filePath: string): boolean {
try {
return !fs.readlinkSync(filePath).endsWith('/');
} catch (_error) {
return false;
}
}
function isSymlinkTargetMarkedDirectory(filePath: string): boolean {
中文说明

[Suggestion] isFileSymlink 启发式函数命名有误——它检测的是符号链接目标字面量是否以 / 结尾,而不是"文件符号链接"。readlinkSync 返回原始链接目标,指向目录的符号链接通常不带尾部 /,除非用 ln -s target/ link 创建。所以 isFileSymlink 对大多数目录符号链接也返回 true,导致 isResolvableMissingPathError 抛出了本可解析的路径。

失败场景:无具体 bug——这是保守安全的(调用者捕获异常并返回 false)。但函数名暗示了不同的语义,保守的重新抛出意味着该函数将指向不存在目标的符号链接全部视为不可解析。

Suggested change
function isFileSymlink(filePath: string): boolean {
try {
return !fs.readlinkSync(filePath).endsWith('/');
} catch (_error) {
return false;
}
}
function isSymlinkTargetMarkedDirectory(filePath: string): boolean {

— deepseek-v4-flash via Qwen Code /review (v0.21.8)


Expand Down