fix(cli): canonicalize paths in /directory remove guard by B-A-M-N · Pull Request #3867 · QwenLM/qwen-code

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

Security and robustness fix for directory management:

Canonicalize paths using fs.realpathSync before checking initial-dir guard.
Added test case for symlink/relative path bypass.

…tching in /directory remove # Conflicts: # .gitignore

The "WorkspaceContext with path expansion" test block was added in the wrong branch (feat/directory-remove instead of feat/skipped-dirs-warning). It references getSkippedDirectories() which doesn't exist here, and uses vi.spyOn(os, 'homedir') which fails under ESM. Remove it entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Commit d584b8a incorrectly added PR 3868's test changes to this branch: - removed debugLogger mock and skipped-directory warn test (PR 3868 feature) - fixed includeDirectories test to use real temp dirs instead of POSIX paths that break on Windows Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

wenshao

Missing test coverage & other issues not tied to a single diff line:

packages/core/src/utils/workspaceContext.test.ts — expandHomeDir integration in resolveAndValidateDir is untested. No test uses ~/~/path input with addDirectory.
packages/core/src/services/fileDiscoveryService.test.ts — 3 edge-case tests removed without replacement: empty file list, relative project root paths, filterFiles with undefined options.
packages/cli/src/ui/commands/directoryCommand.test.ts — completion function, config === null branch, and loadServerHierarchicalMemory failure path are all untested.
packages/cli/src/ui/commands/directoryCommand.test.ts — realpathSync mock fallback uses vi.importActual without await, making the fallback non-functional (swallowed TypeError).
packages/core/src/config/config.test.ts — temp directory cleanup (rmSync) is unreachable on assertion failure, causing resource leaks.
Memory-refresh logic (loadServerHierarchicalMemory + setUserMemory + setGeminiMdFileCount) is duplicated between add and remove subcommands. Extract into a shared helper.
packages/core/src/utils/workspaceContext.ts:141-146 — The removeDirectory/isInitialDirectory catch-block fallbacks don't call expandHomeDir, unlike the resolveAndValidateDir path now added. For consistency, apply expandHomeDir in fallback paths too.

— deepseek-v4-pro via Qwen Code /review

wenshao · 2026-05-08T02:23:13Z

+          return;
+        }
+
+        const removed = workspaceContext.removeDirectory(canonicalDirectory);


[Critical] Non-atomic mutation: workspaceContext.removeDirectory() (line 367) is called before settings.setValue() (line 412-421). If settings persistence fails, the in-memory workspace context is already mutated with no rollback. The next /directory remove attempt in the same session fails with "Directory not found in workspace" — only a restart recovers.

Swap the order: persist to settings first, then call removeDirectory on success. If persistence fails, the in-memory state stays intact and the user can retry.

— deepseek-v4-pro via Qwen Code /review

wenshao · 2026-05-08T02:23:13Z

+          // directory added at user scope would reappear on restart if we
+          // only clear the workspace-scoped list.
+          const targetDir = canonicalDirectory;
+          let targetScope: SettingScope | null = null;


[Critical] Silent success when targetScope remains null (lines 388-411). If the directory is not found in any settings scope's originalSettings.context.includeDirectories, the if (targetScope !== null) block at line 412 is skipped — setValue is never called. The directory is removed from in-memory WorkspaceContext (line 367) but remains in persisted settings. On restart, it reappears without warning.

This can happen when a directory is added via --include-directories CLI flag (not /directory add) or when the settings key exists but the array is empty. Show a warning when targetScope is null, or iterate all scopes regardless of match.

— deepseek-v4-pro via Qwen Code /review

wenshao · 2026-05-08T02:23:13Z

+            SettingScope.User,
+          ] as const) {
+            const scopeDirs =
+              settings.forScope(scope).originalSettings.context


[Suggestion] Settings matching uses Array.includes() strict equality (lines 396, 403). On case-insensitive filesystems (macOS) or when the stored path spelling differs from fs.realpathSync output (e.g., trailing slashes, .. segments, paths added via the UI dialog), the match fails and targetScope stays null — causing the silent-success bug described above.

Suggested change

settings.forScope(scope).originalSettings.context

// Normalize each scopeDir for comparison:

const normalizedTarget = canonicalDirectory;

if (scopeDirs.some(d => {

try { return fs.realpathSync(d) === normalizedTarget; }

catch { return d === targetDir || d === resolvedDirectory; }

})) {

targetScope = scope;

existingDirs = scopeDirs;

break;

}

— deepseek-v4-pro via Qwen Code /review

wenshao · 2026-05-08T02:23:13Z

+              settings.forScope(scope).originalSettings.context
+                ?.includeDirectories ?? [];
+            if (scopeDirs.includes(targetDir)) {
+              targetScope = scope;


[Suggestion] The scope-matching loop breaks on the first match (line 399). If the same directory is stored in both Workspace and User scopes (possible via manual settings editing or migration), only one scope is cleaned. The surviving scope's entry reappears via MergeStrategy.CONCAT on restart.

Remove the break and collect all matching scopes, then call setValue for each one.

— deepseek-v4-pro via Qwen Code /review

wenshao · 2026-05-08T02:23:13Z

+        })();
+
+        if (
+          workspaceContext.getInitialDirectories().includes(canonicalDirectory)


[Suggestion] The CLI manually performs expandHomeDir → path.resolve → fs.realpathSync and does a linear scan on getInitialDirectories() (line 352). WorkspaceContext already provides an isInitialDirectory() method that encapsulates this logic and handles error fallbacks. If isInitialDirectory's resolution logic changes later, this CLI guard won't pick up the change, creating inconsistent behavior.

Suggested change

workspaceContext.getInitialDirectories().includes(canonicalDirectory)

if (workspaceContext.isInitialDirectory(resolvedDirectory)) {

— deepseek-v4-pro via Qwen Code /review

wenshao · 2026-05-08T02:23:13Z

+            );
+            context.ui.setGeminiMdFileCount(fileCount);
+          }
+        } catch (error) {


[Suggestion] When loadServerHierarchicalMemory throws (catch block at line 456), the error message is displayed but execution falls through to the final addItem that shows "Removed directory" as success. The user sees both an error and a success message simultaneously — contradictory and confusing UX.

Add return; after the error addItem call to prevent the dual messages.

— deepseek-v4-pro via Qwen Code /review

wenshao · 2026-05-08T02:23:13Z

+      },
+      kind: CommandKind.BUILT_IN,
+      supportedModes: ['interactive'] as const,
+      completion: async (context: CommandContext) => {


[Suggestion] The completion function (line 297) declares only (context: CommandContext), omitting the partialArg: string parameter. Tab completion always returns ALL removable directories regardless of what the user has already typed. For example, /directory remove /home/u + Tab still shows /var/www instead of filtering to /home/u*. The add subcommand uses getDirPathCompletions(partialArg) for path-aware filtering — remove should do the same.

Suggested change

completion: async (context: CommandContext) => {

completion: async (context: CommandContext, partialArg: string) => {

// ... existing logic ...

const removable = dirs.filter((d) => !initialDirs.includes(d));

if (!partialArg.trim()) return removable;

return removable.filter((d) => d.startsWith(partialArg.trim()));

},

— deepseek-v4-pro via Qwen Code /review

B-A-M-N · 2026-05-08T21:02:16Z

Closing — superseded by #3975

This PR was an early implementation of /directory remove that has fallen behind main (currently CONFLICTING/DIRTY). PR #3975 is a clean single-commit rewrite of the same feature that is up-to-date with main and mergeable.

Closing in favor of #3975.

B-A-M-N added 3 commits May 6, 2026 18:20

feat(cli): add /directory remove subcommand

59f1a47

fix(cli): canonicalize paths in /directory remove guard

8cc21d7

fix(cli): implement scope-aware settings update and canonical path ma…

b018d09

…tching in /directory remove # Conflicts: # .gitignore

B-A-M-N force-pushed the feat/directory-remove branch from 78913ee to b018d09 Compare May 6, 2026 23:23

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

fix(core): move expandHomeDir to core to resolve import errors in CLI

89d9d48

fix(i18n): add missing /directory remove translations for zh and zh-TW

171671f

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

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

Closed

B-A-M-N and others added 3 commits May 7, 2026 00:52

test: refactor tests to avoid global mocks and fix CI failures

d584b8a

wenshao requested changes May 8, 2026

View reviewed changes

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

xaelistic pushed a commit to xaelistic/qwen-code that referenced this pull request Jun 7, 2026

Don't print MCP server logs. It's too noisy. (QwenLM#3867)

8d24e76

Navigation Menu

Uh oh!

fix(cli): canonicalize paths in /directory remove guard - #3867

fix(cli): canonicalize paths in /directory remove guard#3867
B-A-M-N wants to merge 8 commits into
QwenLM:mainfrom
B-A-M-N:feat/directory-remove

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

Uh oh!

wenshao left a comment

Uh oh!

wenshao May 8, 2026

Uh oh!

wenshao May 8, 2026

Uh oh!

wenshao May 8, 2026

Uh oh!

wenshao May 8, 2026

Uh oh!

wenshao May 8, 2026

Uh oh!

wenshao May 8, 2026

Uh oh!

wenshao May 8, 2026

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-              settings.forScope(scope).originalSettings.context
+// Normalize each scopeDir for comparison:
+const normalizedTarget = canonicalDirectory;
+if (scopeDirs.some(d => {
+  try { return fs.realpathSync(d) === normalizedTarget; }
+  catch { return d === targetDir || d === resolvedDirectory; }
+})) {
+  targetScope = scope;
+  existingDirs = scopeDirs;
+  break;
+}

	workspaceContext.getInitialDirectories().includes(canonicalDirectory)
	if (workspaceContext.isInitialDirectory(resolvedDirectory)) {

-      completion: async (context: CommandContext) => {
+completion: async (context: CommandContext, partialArg: string) => {
+  // ... existing logic ...
+  const removable = dirs.filter((d) => !initialDirs.includes(d));
+  if (!partialArg.trim()) return removable;
+  return removable.filter((d) => d.startsWith(partialArg.trim()));
+},

Uh oh!

Conversation

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

Uh oh!

wenshao left a comment

Choose a reason for hiding this comment

Uh oh!

wenshao May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao May 8, 2026

Choose a reason for hiding this comment

Uh oh!

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

Closing — superseded by #3975

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants