Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 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
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,4 @@ storybook-static

# Dev symlink: qc-helper bundled skill docs (created by scripts/dev.js)
packages/core/src/skills/bundled/qc-helper/docs
tmp/
tmp/
68 changes: 64 additions & 4 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { Mock } from 'vitest';
import type { ConfigParameters, SandboxConfig } from './config.js';
import { Config, ApprovalMode } from './config.js';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { setGeminiMdFilename as mockSetGeminiMdFilename } from '../memory/const.js';
import {
Expand Down Expand Up @@ -44,6 +45,21 @@ import { readAutoMemoryIndex } from '../memory/store.js';
import { ExtensionManager } from '../extension/extensionManager.js';
import { SkillManager } from '../skills/skill-manager.js';
import { HookSystem } from '../hooks/index.js';
import * as debugLoggerModule from '../utils/debugLogger.js';

vi.mock('../utils/debugLogger.js', async (importOriginal) => {
const actual =
await importOriginal<typeof import('../utils/debugLogger.js')>();
return {
...actual,
createDebugLogger: vi.fn().mockReturnValue({
warn: vi.fn(),
info: vi.fn(),
error: vi.fn(),
debug: vi.fn(),
}),
};
});

function createToolMock(toolName: string) {
const ToolMock = vi.fn();
Expand Down Expand Up @@ -898,10 +914,15 @@ describe('Server Config (config.ts)', () => {
});

it('should initialize WorkspaceContext with includeDirectories', () => {
const includeDirectories = ['/path/to/dir1', '/path/to/dir2'];
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'config-test-'));
const dir1 = path.join(tmpDir, 'dir1');
const dir2 = path.join(tmpDir, 'dir2');
fs.mkdirSync(dir1);
fs.mkdirSync(dir2);

const paramsWithIncludeDirs: ConfigParameters = {
...baseParams,
includeDirectories,
includeDirectories: [dir1, dir2],
};
const config = new Config(paramsWithIncludeDirs);
const workspaceContext = config.getWorkspaceContext();
Expand All @@ -910,8 +931,47 @@ describe('Server Config (config.ts)', () => {
// Should include the target directory plus the included directories
expect(directories).toHaveLength(3);
expect(directories).toContain(path.resolve(baseParams.targetDir));
expect(directories).toContain('/path/to/dir1');
expect(directories).toContain('/path/to/dir2');
expect(directories).toContain(dir1);
expect(directories).toContain(dir2);

fs.rmSync(tmpDir, { recursive: true, force: true });
});

it('should warn when includeDirectories paths are skipped', () => {
const nonExistentDir = path.join(
fs.mkdtempSync(path.join(os.tmpdir(), 'config-skip-test-')),
'nonexistent',
);

// Override existsSync to return false only for the non-existent dir,
// while keeping the default true for everything else.
vi.mocked(fs.existsSync).mockImplementation(
(p) => p.toString() !== nonExistentDir,

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.

[Critical] 测试 should warn when includeDirectories paths are skipped 中通过 fs.mkdtempSync() 创建了临时目录,但测试结束时从未调用 fs.rmSync() 清理。同一 diff 中前一个测试(should initialize WorkspaceContext with includeDirectories)正确执行了清理。每次运行该测试都会在 /tmp 泄漏一个空目录。

Suggested change
(p) => p.toString() !== nonExistentDir,
it('should warn when includeDirectories paths are skipped', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'config-skip-test-'));
const nonExistentDir = path.join(tmpDir, 'nonexistent');
// ... rest of test ...
fs.rmSync(tmpDir, { recursive: true, force: true });
});

同时,该测试通过 vi.mocked(fs.existsSync).mockImplementation(...) 覆盖了 existsSync,但测试结束后未恢复(vi.clearAllMocks() 不清除实现)。建议测试末尾添加 vi.mocked(fs.existsSync).mockReturnValue(true) 防止泄漏到后续测试。

— deepseek-v4-pro via Qwen Code /review

);

const mockWarn = vi.fn();
vi.mocked(debugLoggerModule.createDebugLogger).mockReturnValue({
warn: mockWarn,
info: vi.fn(),
error: vi.fn(),
debug: vi.fn(),
} as unknown as debugLoggerModule.DebugLogger);

const paramsWithInvalidDir: ConfigParameters = {
...baseParams,
includeDirectories: [nonExistentDir],
};

new Config(paramsWithInvalidDir);

expect(mockWarn).toHaveBeenCalledWith(
expect.stringContaining(
'The following --include-directories paths were skipped',
),
);
expect(mockWarn).toHaveBeenCalledWith(
expect.stringContaining(nonExistentDir),
);
});

it('Config constructor should set telemetry to true when provided as true', () => {
Expand Down
9 changes: 9 additions & 0 deletions packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ import { CronScheduler } from '../services/cronScheduler.js';
import type { SendSdkMcpMessage } from '../tools/mcp-client.js';
import { setGeminiMdFilename } from '../memory/const.js';
import { canUseRipgrep } from '../utils/ripgrepUtils.js';
import { expandHomeDir } from '../utils/paths.js';

import { ToolRegistry, type ToolFactory } from '../tools/tool-registry.js';
import { ToolNames } from '../tools/tool-names.js';
import type { LspClient } from '../lsp/types.js';
Expand Down Expand Up @@ -731,7 +733,14 @@ export class Config {
this.targetDir,
this.explicitIncludeDirectories,
);
const skippedDirs = this.workspaceContext.getSkippedDirectories();

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] The new skipped-directory warning branch is user-visible but currently has no direct test coverage. Existing config tests cover valid includeDirectories, but not the case where getSkippedDirectories() is non-empty and debugLogger.warn emits the summary. A regression could silently drop the warning or report the wrong path without failing tests.

Please add a Config constructor test with an invalid includeDirectories entry and assert debugLogger.warn includes the skipped-directory warning and path.

— gpt-5.5 via Qwen Code /review

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] 重复警告:WorkspaceContext.addDirectory() 的 catch 块已为每个跳过目录输出 debugLogger.warn("Skipping unreadable directory: ..."),而 Config 构造函数又新增了第二条聚合警告 "The following --include-directories paths were skipped..."。每个跳过目录会产生两条日志。

Suggested change
const skippedDirs = this.workspaceContext.getSkippedDirectories();
// 方案 A:移除 Config 的聚合警告,仅保留 addDirectory 中的单条 warn
// 方案 B:将 addDirectory 中的 warn 降级为 debug,仅保留 Config 的聚合警告

— deepseek-v4-pro via Qwen Code /review

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] Dual warning for the same skipped directory

Each skipped directory produces two debug log warnings: (1) per-directory "Skipping unreadable directory: X (reason)" from WorkspaceContext.addDirectory (line ~102 of workspaceContext.ts), and (2) this batched summary. Both go to the debug log.

For N skipped directories, the user sees N+1 warnings with overlapping information. Consider removing or gating the per-directory warning in WorkspaceContext.addDirectory() since this batched summary is the authoritative overview.

— glm-5.1 via Qwen Code /review

if (skippedDirs.length > 0) {

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] 警告消息硬编码了 --include-directories,但 getSkippedDirectories() 也包含构造函数的第一个参数 targetDir(如果不可读)。当 targetDir 是问题所在时,调试信息会产生误导。

Suggested change
if (skippedDirs.length > 0) {
const skippedDirs = this.workspaceContext.getSkippedDirectories()
.filter((d) => path.resolve(d) !== this.targetDir);
if (skippedDirs.length > 0) {
this.debugLogger.warn(
`The following workspace directories were skipped because they do not exist or are not readable:\n${skippedDirs.map((d) => ` - ${expandHomeDir(d)}`).join('\n')}`,
);
}

— deepseek-v4-pro via Qwen Code /review

this.debugLogger.warn(

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.

[Critical] Warning is invisible to users — goes to debug log only, not to this.warnings

The skipped-directory warning uses this.debugLogger.warn(...) which writes to the debug log file. It does NOT push into this.warnings (the array surfaced via config.getWarnings()). Users passing --include-directories /nonexistent see nothing in the TUI — the only trace is in a debug log file they likely don't know about.

Downstream tool calls targeting those directories fail with "Path is not within workspace" with no clear root cause.

Suggested change
this.debugLogger.warn(
const skippedDirs = this.workspaceContext.getSkippedDirectories();
if (skippedDirs.length > 0) {
const message =
`The following --include-directories paths were skipped because they do not exist or are not readable:\n${skippedDirs.map((d) => ` - ${expandHomeDir(d)}`).join('\n')}`;
this.debugLogger.warn(message);
this.warnings.push(message);
}

Note: this requires moving the block to after this.warnings = params.warnings ?? [];, or constructing a local array and assigning later.

— glm-5.1 via Qwen Code /review

`The following --include-directories paths were skipped because they do not exist or are not readable:\n${skippedDirs.map((d) => ` - ${expandHomeDir(d)}`).join('\n')}`,

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.

[Critical] 被跳过目录的聚合警告仅通过 debugLogger.warn() 写入 ~/.qwen/debug/ 下的调试日志文件——不会输出到终端(stdout/stderr)。当用户通过 --include-directories 指定目录被静默跳过时,完全看不到任何反馈。debugLogger 日志文件对排查来说难以发现(需要知道其存在和位置),在线上故障排查时尤其致命。

建议同时对被跳过的关键路径输出 console.warn() 到 stderr,调试日志写入可作为补充。

另外,警告消息将其原因统一归为 "do not exist or are not readable",但 resolveAndValidateDir 还可能因"路径是文件而非目录"(ENOTDIR) 而抛出异常。三种失败原因各有不同的排查路径,聚合警告将原因抹平会误导排查方向。

— deepseek-v4-pro via Qwen Code /review

);
}
this.debugMode = params.debugMode;

this.inputFormat = params.inputFormat ?? InputFormat.TEXT;
const normalizedOutputFormat = normalizeConfigOutputFormat(
params.outputFormat ?? params.output?.format,
Expand Down
49 changes: 49 additions & 0 deletions packages/core/src/utils/paths.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
isSubpath,
shortenPath,
tildeifyPath,
expandHomeDir,
getProjectHash,
_resetValidatePathCacheForTest,
} from './paths.js';
Expand Down Expand Up @@ -911,3 +912,51 @@ describe('getProjectHash', () => {
platformSpy.mockRestore();
});
});

describe('expandHomeDir', () => {
const homeDir = os.homedir();

it('should return empty string for empty input', () => {
expect(expandHomeDir('')).toBe('');
});

it('should expand ~ to home directory', () => {
expect(expandHomeDir('~')).toBe(path.normalize(homeDir));
});

it('should expand ~/path to home directory path', () => {
expect(expandHomeDir('~/documents')).toBe(path.join(homeDir, 'documents'));
});

it('should not expand ~path (no slash)', () => {
expect(expandHomeDir('~documents')).toBe('~documents');
});

it('should expand %userprofile% (case-insensitive) to home directory', () => {
expect(expandHomeDir('%userprofile%')).toBe(path.normalize(homeDir));
expect(expandHomeDir('%USERPROFILE%')).toBe(path.normalize(homeDir));
});

it('should expand %userprofile%\\path to home directory path', () => {
// We use path.join to get the correct separator for the current platform
const result = expandHomeDir('%userprofile%/documents');
expect(result).toBe(path.join(homeDir, 'documents'));

if (process.platform === 'win32') {
const resultWin = expandHomeDir('%userprofile%\\documents');
expect(resultWin).toBe(path.join(homeDir, 'documents'));
}
});

it('should return regular absolute path unchanged (but normalized)', () => {
const absPath =
process.platform === 'win32' ? 'C:\\absolute\\path' : '/absolute/path';
expect(expandHomeDir(absPath)).toBe(path.normalize(absPath));
});

it('should return relative path unchanged (but normalized)', () => {
expect(expandHomeDir('relative/path')).toBe(
path.normalize('relative/path'),
);
});
});
18 changes: 18 additions & 0 deletions packages/core/src/utils/paths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,24 @@ export function tildeifyPath(path: string): string {
return path;
}

/**
* Expands tilde (~) and Windows-style %userprofile% to the full home directory path.
* @param p - The path to expand.
* @returns The expanded path.
*/
export function expandHomeDir(p: string): string {

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] expandHomeDir is duplicated across 3 locations

Identical tilde/home-expansion logic exists in:

  • packages/core/src/utils/paths.ts (this new copy)
  • packages/cli/src/ui/commands/directoryCommand.tsx
  • packages/cli/src/utils/resolvePath.ts

All three expand ~, ~/, and %userprofile% to os.homedir(). Any fix to one copy (e.g., the %userprofile% matching issue flagged in other comments) must be applied to all three. Consider keeping the canonical implementation here in core and having CLI code import from @qwen-code/qwen-code-core.

— glm-5.1 via Qwen Code /review

if (!p) {

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] expandHomeDir() is newly exported and implements several explicit path-formatting behaviors ('', ~, ~/..., case-insensitive %userprofile%, and normalization), but none of those cases are covered by unit tests. Since this helper is now used for skipped-directory warnings and mixes POSIX/Windows-style path handling, it would be easy to regress cross-platform formatting without coverage.

Please add expandHomeDir tests near the existing path-helper tests in paths.test.ts for those edge cases.

— gpt-5.5 via Qwen Code /review

return '';
}
let expandedPath = p;

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] expandHomeDir 中 %userprofile% 的匹配过于宽松:1) 未检查 process.platform === 'win32'——非 Windows 上包含字面 %userprofile% 命名的目录会被错误展开;2) 未检查分隔符——%userprofile%foo(无 \ 或 /)也会展开,但 ~ 分支正确地要求了 ~/ 分隔符。同一逻辑在 packages/cli/src/utils/resolvePath.ts 和 packages/cli/src/ui/commands/directoryCommand.tsx 中重复了两次,建议收敛到 core 单一实现。

Suggested change
let expandedPath = p;
if (process.platform === 'win32' && (p.toLowerCase() === '%userprofile%' || p.toLowerCase().startsWith('%userprofile%\\') || p.toLowerCase().startsWith('%userprofile%/'))) {
expandedPath = os.homedir() + p.substring('%userprofile%'.length);
} else if (p === '~' || p.startsWith('~/')) {

— deepseek-v4-pro via Qwen Code /review

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] os.homedir() 在 expandHomeDir 中未被 try/catch 包裹。当 HOME 未设置时(精简容器环境),会抛出异常导致整个 Config 构造函数崩溃,且堆栈中没有明确指向 expandHomeDir 的信息。

Suggested change
let expandedPath = p;
let homeDir: string;
try {
homeDir = os.homedir();
} catch {
return p;
}
let expandedPath = p;
if (p.toLowerCase().startsWith('%userprofile%')) {
expandedPath = homeDir + p.substring('%userprofile%'.length);
} else if (p === '~' || p.startsWith('~/')) {
expandedPath = homeDir + p.substring(1);
}

— deepseek-v4-pro via Qwen Code /review

if (p.toLowerCase().startsWith('%userprofile%')) {

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] expandHomeDir() 使用 path.normalize() 处理结果,这会解析 .. 段,允许路径逃逸出家目录。例如 expandHomeDir('~/../../etc/passwd') 会返回 /etc/passwd。当前仅用于警告消息中的路径展示,实际不可被利用,但作为导出的公共工具函数,未来调用者可能将其用于文件操作。

Suggested change
if (p.toLowerCase().startsWith('%userprofile%')) {
return path.normalize(expandedPath);
// 或:在 normalize 后验证结果仍在家目录内
// 并在 JSDoc 中标注调用者需自行验证路径安全性

— deepseek-v4-pro via Qwen Code /review

expandedPath = os.homedir() + p.substring('%userprofile%'.length);
} else if (p === '~' || p.startsWith('~/')) {
expandedPath = os.homedir() + p.substring(1);
}
return path.normalize(expandedPath);
}

/**
* Shortens a path string if it exceeds maxLen, prioritizing the start and end segments.
* Shows root + first segment + "..." + end segments when middle segments are omitted.
Expand Down
12 changes: 12 additions & 0 deletions packages/core/src/utils/workspaceContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ export type Unsubscribe = () => void;
export class WorkspaceContext {
private directories = new Set<string>();
private initialDirectories: Set<string>;
private readonly skippedDirectories = new Set<string>();
private onDirectoriesChangedListeners = new Set<() => void>();

/**
* Memoized realpath results. Every workspace-bounded tool call ultimately
* routes through {@link fullyResolvedPath} → `fs.realpathSync`; without
Expand Down Expand Up @@ -50,8 +52,17 @@ export class WorkspaceContext {
}
}

/**
* Returns directories that were skipped during construction because they

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] 新增的 getSkippedDirectories() 公共 API 在 workspaceContext.test.ts 中没有直接的单元测试。该方法仅在 config.test.ts 中通过警告消息副作用被间接覆盖。建议添加测试:1) 构造后返回空;2) 无效目录构造后包含被跳过的目录;3) 运行时 addDirectory 失败后正确跟踪。

— deepseek-v4-pro via Qwen Code /review

* did not exist or were not readable.
*/
getSkippedDirectories(): readonly string[] {
return Array.from(this.skippedDirectories);
}

/**
* Registers a listener that is called when the workspace directories change.

* @param listener The listener to call.
* @returns A function to unsubscribe the listener.
*/
Expand Down Expand Up @@ -88,6 +99,7 @@ export class WorkspaceContext {
this.directories.add(resolved);

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] skippedDirectories Set 只增不减——目录首次失败时被加入,但后续成功时不会移除。如果 addDirectory 在构造后被重试调用(如 filesystem watcher 重新添加),getSkippedDirectories() 仍返回已变为有效的目录。

Suggested change
this.directories.add(resolved);
this.directories.add(resolved);
this.skippedDirectories.delete(directory);
this.notifyDirectoriesChanged();

— deepseek-v4-pro via Qwen Code /review

this.notifyDirectoriesChanged();
} catch (err) {

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] skippedDirectories stores raw input string — inconsistent with directories

directories stores fully-resolved canonical paths (via fs.realpathSync), but skippedDirectories stores the raw unresolved directory parameter. This means getSkippedDirectories() returns paths that cannot be directly compared with getDirectories() results — any code correlating the two sets would get false negatives.

Suggested change
} catch (err) {
} catch (err) {
const absolutePath = path.isAbsolute(directory)
? directory
: path.resolve(basePath, directory);
this.skippedDirectories.add(absolutePath);

— glm-5.1 via Qwen Code /review

this.skippedDirectories.add(directory);
debugLogger.warn(
`Skipping unreadable directory: ${directory} (${err instanceof Error ? err.message : String(err)})`,
);
Expand Down