-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): align mock workspace path containment #8759
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 all commits
1415118
3ef14cc
27e1098
fbbb471
6b799ee
3e25aa7
76e8224
af13809
bc31217
a7f9b8b
815f883
6b5ca00
5bfb406
f2fc21c
5ec9f62
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,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); | ||
| }); | ||
|
|
||
| it('uses lexical containment for non-Node filesystem stubs', () => { | ||
| vi.mocked(fs.realpathSync).mockImplementation(() => { | ||
| throw new TypeError('mocked filesystem call'); | ||
| }); | ||
|
rbalachandar marked this conversation as resolved.
|
||
| 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 }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| /** | ||
| * @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( | ||
|
rbalachandar marked this conversation as resolved.
|
||
| 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 }); | ||
| } | ||
| }); | ||
|
|
||
| it('does not collapse missing paths below a symlinked ancestor', () => { | ||
| 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 workspaceRoot = path.join(aliasDir, 'ghost', 'workspace'); | ||
| const siblingPath = path.join( | ||
| aliasDir, | ||
| 'ghost', | ||
| 'completely-different', | ||
| 'file.txt', | ||
| ); | ||
| const workspace = createMockWorkspaceContext(workspaceRoot); | ||
|
|
||
| expect(workspace.isPathWithinWorkspace(siblingPath)).toBe(false); | ||
| } finally { | ||
| rmSync(aliasDir, { recursive: true, force: true }); | ||
| rmSync(rootDir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| 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 }); | ||
| } | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
rbalachandar marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
rbalachandar marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+33
to
+42
Collaborator
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. [Suggestion] Inner try/catch wraps 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
Suggested change
中文说明[Suggestion] 内层 try/catch 包裹了 失败场景:不会崩溃——代码正确运行。但它掩盖了代码块中真正可能失败的部分,使 mock 更难理解和维护。真正的
Suggested change
— deepseek-v4-flash via Qwen Code /review (v0.21.8) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } as unknown as WorkspaceContext; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return mockWorkspaceContext; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function canonicalizeForContainment(inputPath: string): string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
rbalachandar marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
Collaborator
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. [Suggestion] Mutation-vacuous test — replacing Failure scenario: If a future regression replaced the ancestor walk with a trivial identity return, this test would not catch it. The
Suggested change
中文说明[Suggestion] 变异无效的测试——将 失败场景:如果未来回归将祖先遍历替换为恒等返回,该测试不会捕获。
Suggested change
— deepseek-v4-flash via Qwen Code /review (v0.21.8) |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('with symbolic link', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| describe('in the workspace', () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let realDir: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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.
[Suggestion] Fallback test
'uses lexical containment when ENOENT has no path'is redundant with the first test — both use the same mock setup (realpathSyncmocked to throwthrowMissingPath, which creates an ENOENT error with noerror.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.
中文说明
[Suggestion] 回退测试
'uses lexical containment when ENOENT has no path'与第一个测试冗余——两者使用相同的 mock 设置(realpathSync模拟抛出不带error.path的 ENOENT),并断言相同的条件(path.join(rootDir, 'missing.txt')→true)。第二个测试的断言是第一个测试断言的严格子集。失败场景:无具体 bug——测试正确。但读者会期望两个不同的测试名称对应两个不同的场景。第二个测试没有增加新的覆盖。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)