fix(core): use debugLogger for skipped directory warnings by B-A-M-N · Pull Request #3868 · QwenLM/qwen-code

B-A-M-N · 2026-05-06T06:42:08Z

Logging and tracking improvements:

Switched to Set for skipped directory tracking.
Replaced raw stderr writes with debugLogger.warn.
Included expanded home directory paths in warnings.

…ith Set # Conflicts: # packages/core/src/config/config.ts # packages/core/src/utils/workspaceContext.ts

wenshao · 2026-05-07T02:27:24Z

      this.targetDir,
      this.explicitIncludeDirectories,
    );
+    const skippedDirs = this.workspaceContext.getSkippedDirectories();


[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

wenshao · 2026-05-07T02:27:24Z

+ * @returns The expanded path.
+ */
+export function expandHomeDir(p: string): string {
+  if (!p) {


[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

…rnings

wenshao · 2026-05-07T14:28:46Z

+  if (!p) {
+    return '';
+  }
+  let expandedPath = p;


[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

wenshao · 2026-05-07T14:28:46Z

@@ -88,6 +99,7 @@ export class WorkspaceContext {
      this.directories.add(resolved);


[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

wenshao · 2026-05-07T14:28:46Z

      this.explicitIncludeDirectories,
    );
+    const skippedDirs = this.workspaceContext.getSkippedDirectories();
+    if (skippedDirs.length > 0) {


[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

wenshao · 2026-05-07T14:28:46Z

+  if (!p) {
+    return '';
+  }
+  let expandedPath = p;


[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

wenshao · 2026-05-07T14:28:46Z

  }

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


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

— deepseek-v4-pro via Qwen Code /review

Both tests assumed POSIX path resolution and broke on Windows: - includeDirectories test now uses real temp dirs with path.join - skipped-directory warn test now mocks existsSync for the specific non-existent path instead of relying on path.resolve matching Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

B-A-M-N · 2026-05-07T22:53:59Z

CI not triggering on latest commits. Attempting to re-trigger.

- Add process.platform check for %userprofile% expansion (Windows only) - Require separator after %userprofile% to avoid matching %userprofile%foo - Wrap os.homedir() in try/catch to handle missing HOME env var - Filter targetDir from skipped directories warning - Clear skippedDirectories entry when addDirectory succeeds later - Add getSkippedDirectories() unit tests - Add expandHomeDir edge-case tests (non-Windows, no separator, homedir failure) - Update config test to match new warning message text

wenshao · 2026-05-08T01:24:27Z

+    // 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,


[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

wenshao · 2026-05-08T01:24:27Z

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


[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

wenshao · 2026-05-08T01:24:27Z

+    return '';
+  }
+  let expandedPath = p;
+  if (p.toLowerCase().startsWith('%userprofile%')) {


[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

wenshao · 2026-05-08T01:24:27Z

      this.targetDir,
      this.explicitIncludeDirectories,
    );
+    const skippedDirs = this.workspaceContext.getSkippedDirectories();


[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

wenshao · 2026-05-08T01:43:06Z

    );
+    const skippedDirs = this.workspaceContext.getSkippedDirectories();
+    if (skippedDirs.length > 0) {
+      this.debugLogger.warn(


[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

wenshao · 2026-05-08T01:43:06Z

      this.targetDir,
      this.explicitIncludeDirectories,
    );
+    const skippedDirs = this.workspaceContext.getSkippedDirectories();


[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

wenshao · 2026-05-08T01:43:06Z

+ * @param p - The path to expand.
+ * @returns The expanded path.
+ */
+export function expandHomeDir(p: string): string {


[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

wenshao · 2026-05-08T01:43:06Z

@@ -88,6 +99,7 @@ export class WorkspaceContext {
      this.directories.add(resolved);
      this.notifyDirectoriesChanged();
    } catch (err) {


[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

- config.test.ts: Add fs.rmSync cleanup for temp dir and restore existsSync mock in finally block to prevent leaking mock state to subsequent tests - config.ts: Add process.stderr.write so users see skipped-dir warnings on stderr (not just in debug logs). Distinguish failure reasons: 'does not exist', 'is not a directory', 'is not readable' - workspaceContext.ts: Change skippedDirectories from Set to Map to store failure reasons. Remove per-directory debugLogger.warn (was duplicating the aggregate warning in Config constructor) - paths.ts: Guard expandHomeDir against path traversal via .. segments. If normalization escapes the home directory, return the original input. Add JSDoc noting callers must validate for file operations. - Add getSkippedDirectoryReason() method and tests - Add expandHomeDir path traversal test

Resolved conflicts in paths.ts and paths.test.ts: kept hardened expandHomeDir with Windows-only %userprofile% gating, separator check, os.homedir() try/catch, and path traversal guard. Kept expanded test suite with platform-conditional and edge-case coverage.

- Push skipped-directory warning to this.warnings so it's visible in TUI via getWarnings() - Move skipped dirs warning block after this.warnings initialization - Store absolute paths in skippedDirectories Map (consistent with directories Set) - Add test assertion for warnings array containing skipped directory warning

When addDirectory succeeds after a previous failure, the skipped entry may have been stored under a non-canonical absolute path (via path.resolve) while the success path attempted deletion using the canonical path (via fs.realpathSync). This caused a silent leak in skippedDirectories when the two paths differed (e.g. symlinks). Fix: compute the non-canonical absolute path up front and attempt deletion under both the canonical and non-canonical keys. Also adds tests to verify cleanup and path consistency.

@returns

- Align expandHomeDir path traversal guard with expansion conditions (platform check + exact/separator match for %userprofile%) - Return p instead of '' for empty input in expandHomeDir (consistent with os.homedir() catch block and @returns doc) - Remove unnecessary expandHomeDir() call on already-absolute paths in Config warning block; remove unused import - Remove duplicate workspaceContext test (identical to existing test)

B-A-M-N · 2026-05-09T00:06:39Z

Closing this PR. The branch has accumulated broad CI failures across all platforms (including unrelated test failures in StandaloneSessionPicker and relaunch tests) and has gone through multiple review rounds with significant churn. The core idea (skipped directory warnings via debugLogger) is worth revisiting as a clean, focused PR, but this branch is too tangled to salvage efficiently.

B-A-M-N added 2 commits May 6, 2026 18:27

fix(core): use debugLogger for skipped directory warnings and track w…

662f0cd

…ith Set # Conflicts: # packages/core/src/config/config.ts # packages/core/src/utils/workspaceContext.ts

fix(core): remove .prforge junk from .gitignore

8590a44

B-A-M-N force-pushed the feat/skipped-dirs-warning branch from 7fb9991 to 8590a44 Compare May 6, 2026 23:28

fix(core): move expandHomeDir to core and fix import in config

0311a7f

B-A-M-N mentioned this pull request May 7, 2026

feat(cli): polish --add-dir / --include-directories feature #3856

Closed

wenshao reviewed May 7, 2026

View reviewed changes

test(core): add unit tests for expandHomeDir and skipped-directory wa…

411cf41

…rnings

B-A-M-N force-pushed the feat/skipped-dirs-warning branch from 6813635 to 411cf41 Compare May 7, 2026 12:30

wenshao reviewed May 7, 2026

View reviewed changes

B-A-M-N closed this May 7, 2026

B-A-M-N reopened this May 7, 2026

ci: retrigger

d2f7caa

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

wenshao requested changes May 8, 2026

View reviewed changes

B-A-M-N added 5 commits May 7, 2026 20:47

B-A-M-N closed this May 9, 2026

-  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('~/')) {

		@@ -88,6 +99,7 @@ export class WorkspaceContext {
		this.directories.add(resolved);

-  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);
+  }

-      (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 });
+  });

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

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

-      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);
+    }

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

Uh oh!

Conversation

B-A-M-N commented May 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

B-A-M-N commented May 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

B-A-M-N commented May 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants