feat(cli): add /directory remove subcommand - #3975
Conversation
|
The lint failure was from an older commit that had merge conflict markers in locale files. These have been resolved in the latest commit (fb6263a). Could you re-trigger CI or I can push an empty commit to kick it? |
| workspace: { | ||
| settings: {}, | ||
| originalSettings: {}, | ||
| } as SettingsFile, |
There was a problem hiding this comment.
[Critical] TypeScript errors: SettingsFile type used at lines 73, 77 without import, and this implicitly has type any at line 81 (noImplicitThis).
Add the missing import and convert forScope to an arrow function:
| } as SettingsFile, | |
| import type { SettingsFile } from '../../config/settings.js'; |
| } as SettingsFile, | |
| forScope: vi.fn((scope: string) => { | |
| if (scope === 'user') return mockSettings.user; | |
| return mockSettings.workspace; | |
| }), |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| 'This directory is already in the workspace.', | ||
| 'Already covered by existing directory: {{dir}}': | ||
| 'Already covered by existing directory: {{dir}}', | ||
|
|
There was a problem hiding this comment.
[Critical] The i18n key 'Already covered by existing directory: {{dir}}' was deleted from en.js, zh.js, and zh-TW.js but is still referenced by PermissionsDialog.tsx:343. This will cause the raw key text to display instead of a translated message when a directory is covered by an existing workspace entry.
Restore the key to all three files:
| 'Already covered by existing directory: {{dir}}': | |
| 'Already covered by existing directory: {{dir}}', |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Critical] removeDirectory() is called at line 390 (removes from memory) before the try block at line 408 that persists to settings. If settings.setValue() throws (disk full, permissions), the catch block displays an error and returns — but the directory is already gone from memory while still present in persisted settings. On restart, the directory silently reappears.
Swap the order (persist first, then remove) or roll back in the catch block:
| } catch (error) { | |
| // Rollback: re-add the directory to in-memory context | |
| workspaceContext.addDirectory(expandedDir); | |
| addItem( | |
| { | |
| type: MessageType.ERROR, | |
| text: t( | |
| 'Directory removed from workspace but error updating settings: {{error}}', | |
| { error: (error as Error).message }, | |
| ), | |
| }, | |
| Date.now(), | |
| ); | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| if ( | ||
| workspaceContext.isInitialDirectory(expandedDir) ?? |
There was a problem hiding this comment.
[Suggestion] ?? (nullish coalescing) is used instead of || (logical OR) for the initial-directory guard. isInitialDirectory() returns boolean (never null/undefined), so getInitialDirectories().includes(expandedDir) is dead code — the fallback check can never execute. If isInitialDirectory() returns false due to a path resolution edge case, the user sees a misleading "Directory not found in workspace" instead of "Cannot remove initial workspace directory".
| workspaceContext.isInitialDirectory(expandedDir) ?? | |
| if ( | |
| workspaceContext.isInitialDirectory(expandedDir) || | |
| workspaceContext.getInitialDirectories().includes(expandedDir) | |
| ) |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } catch { | ||
| return d === targetDir; | ||
| } | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Settings-cleanup uses findIndex + filter(i !== matchingIndex) which only removes the first matching entry per scope. If context.includeDirectories contains duplicate entries for the same directory, duplicates survive and the directory reappears on restart via MergeStrategy.CONCAT.
Use filter to remove all matches:
| }); | |
| const includeDirectories = scopeDirs.filter((d: string) => { | |
| try { | |
| return fs.realpathSync(expandHomeDir(d)) !== targetDir; | |
| } catch { | |
| return d !== targetDir; | |
| } | |
| }); | |
| if (includeDirectories.length < scopeDirs.length) { | |
| found = true; | |
| settings.setValue(scope, 'context.includeDirectories', includeDirectories); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| settings.forScope(scope).originalSettings.context | ||
| ?.includeDirectories ?? []; | ||
| const matchingIndex = scopeDirs.findIndex((d: string) => { | ||
| try { |
There was a problem hiding this comment.
[Suggestion] When realpathSync fails in the findIndex callback, the fallback compares d === targetDir — but d is the raw persisted path (possibly ~/project or $HOME/project) while targetDir is the realpath-resolved canonical form. Path format mismatch causes false negatives: the directory exists in persisted settings but can't be matched.
Expand the raw path before comparing:
| try { | |
| } catch { | |
| return expandHomeDir(d) === targetDir || d === targetDir; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[Suggestion] getSkippedDirectories() and the skippedDirectories array are added with full test coverage but have zero production consumers. This dead code increases the public API surface without providing value. Either add a consumer (e.g., surface skipped directories in /directory show or at CLI startup) or remove it from this PR.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| settings.setValue( | ||
| scope, | ||
| 'context.includeDirectories', | ||
| includeDirectories, |
There was a problem hiding this comment.
[Critical] settings.setValue() mutates the loaded settings before the write is completed, but this catch path only reports the error and returns. If saveSettings() throws, LoadedSettings.setValue() has already changed settingsFile.settings, settingsFile.originalSettings, and _merged, so the in-memory settings no longer match the file on disk. A retry can then behave as if the persisted entry was removed even though it will reappear after restart. The same loop can also partially update one scope before a later scope throws, leaving workspace/user settings inconsistent.
Please make this update transactional for this command: either restore the affected in-memory settings on catch / on later-scope failure, or use a settings API that writes successfully before mutating the loaded settings state.
— gpt-5.5 via Qwen Code /review
| }), | ||
| }, | ||
| Date.now(), | ||
| ); |
There was a problem hiding this comment.
[Critical] This only treats the directory as missing when both removed and found are false. If a persisted settings entry was found and deleted, but workspaceContext.removeDirectory(expandedDir) returns false, the command still refreshes memory and reports success even though the active workspace entry was not removed.
That can leave the directory accessible for the rest of the current session while telling the user it was removed. Please treat a failed in-memory removal as an error even when a persisted entry was found, or remove by the same canonical target used for settings matching and only report success after the active workspace entry is actually removed.
— gpt-5.5 via Qwen Code /review
| addItem( | ||
| { | ||
| type: MessageType.ERROR, | ||
| text: t('Error updating settings: {{error}}', { |
There was a problem hiding this comment.
[Suggestion] This new i18n key is not added to the locale files. The previous translated key still exists, but Error updating settings: {{error}} is missing from en.js, zh.js, and zh-TW.js, so localized sessions will fall back to the raw English key on this failure path.
Please add this key to all locale files, or reuse an existing localized error key.
— gpt-5.5 via Qwen Code /review
| text: `Removed directory: ${removableDir}`, | ||
| }), | ||
| expect.any(Number), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The new removal loop handles both SettingScope.Workspace and SettingScope.User, but the tests only cover the workspace-scoped removal path and a single failing setValue() call. There is no coverage for a directory stored only in user settings, stored in both scopes, or a partial failure where one scope update succeeds and a later scope throws.
Please add focused tests for user-only removal, both-scope removal, and partial-failure behavior so regressions in the new multi-scope logic are caught.
— gpt-5.5 via Qwen Code /review
| let mockConfig: Config; | ||
| let mockWorkspaceContext: WorkspaceContext; | ||
| let mockWorkspaceDirectories: string[]; | ||
| let mockSettings: Record<string, unknown>; |
There was a problem hiding this comment.
[Critical] 15 TS4111 type errors: mockSettings is typed Record<string, unknown> but the test code accesses nested properties (forScope, user, workspace, setValue) with dot notation. With noPropertyAccessFromIndexSignature enabled, tsc requires bracket notation. This blocks the build.
Affected lines in the test: 82, 83, 84, 395, 457, 458, 459, 502, 510, 555, 566, 622, 630, 640, 678.
| let mockSettings: Record<string, unknown>; | |
| // Option 1: use bracket notation for all index-signature accesses | |
| mockSettings['forScope'](scope) | |
| mockSettings['user'] | |
| mockSettings['workspace'] |
— deepseek-v4-pro via Qwen Code /review
| let found = false; | ||
|
|
||
| // Snapshot in-memory state for rollback. | ||
| const workspaceBefore = { |
There was a problem hiding this comment.
[Critical] Shallow copy rollback is ineffective. The snapshot { settings: { ...settings.workspace.settings } } only copies the top-level keys. setValue('context.includeDirectories', ...) internally calls setNestedPropertySafe which mutates the nested context object in-place. Since the snapshot's .context reference points to the same object, the rollback settings.workspace.settings = workspaceBefore.settings cannot restore the original context.includeDirectories — the mutation is already visible in the snapshot.
Combined with C3 (below): this means multi-scope failures cause silent data corruption where the first scope's settings are lost in memory.
| const workspaceBefore = { | |
| // Use structuredClone for true deep copy | |
| const workspaceBefore = { | |
| settings: structuredClone(settings.workspace.settings), | |
| originalSettings: structuredClone(settings.workspace.originalSettings), | |
| }; | |
| const userBefore = { | |
| settings: structuredClone(settings.user.settings), | |
| originalSettings: structuredClone(settings.user.originalSettings), | |
| }; |
— deepseek-v4-pro via Qwen Code /review
| }; | ||
|
|
||
| try { | ||
| for (const scope of [ |
There was a problem hiding this comment.
[Critical] settings.setValue() writes to disk immediately (internally calls saveSettings() → writeWithBackupSync). In the [Workspace, User] loop, if Workspace succeeds (disk committed) but User fails, the catch block only restores memory — the disk file for Workspace scope is already mutated with no reversal. On restart, the directory silently disappears from Workspace scope despite the error message.
| for (const scope of [ | |
| // Option: collect changes in memory first, commit atomically after all scopes succeed | |
| const pendingChanges: Array<{scope: SettingScope, dirs: string[]}> = []; | |
| for (const scope of [SettingScope.Workspace, SettingScope.User] as const) { | |
| // ... compute new includeDirectories array ... | |
| if (includeDirectories.length < scopeDirs.length) { | |
| found = true; | |
| pendingChanges.push({ scope, dirs: includeDirectories }); | |
| } | |
| } | |
| // Only persist after all scopes validated | |
| for (const change of pendingChanges) { | |
| settings.setValue(change.scope, 'context.includeDirectories', change.dirs); | |
| } |
— deepseek-v4-pro via Qwen Code /review
| ?.includeDirectories ?? []; | ||
| const includeDirectories = scopeDirs.filter((d: string) => { | ||
| try { | ||
| const resolved = fs.realpathSync(expandHomeDir(d)); |
There was a problem hiding this comment.
[Critical] fs.realpathSync called inside scopeDirs.filter() closure for each entry produces N+1 sync disk I/O that blocks the event loop. In a monorepo with many includeDirectories, /directory remove will freeze the UI.
| const resolved = fs.realpathSync(expandHomeDir(d)); | |
| // Use async parallel resolution | |
| const resolutions = await Promise.all( | |
| scopeDirs.map(async (d: string) => { | |
| try { | |
| return { original: d, resolved: await fs.promises.realpath(expandHomeDir(d)) }; | |
| } catch { | |
| return { original: d, resolved: null }; | |
| } | |
| }), | |
| ); | |
| const includeDirectories = resolutions | |
| .filter((r) => r.resolved !== targetDir && r.original !== targetDir) | |
| .map((r) => r.original); |
— deepseek-v4-pro via Qwen Code /review
| addItem( | ||
| { | ||
| type: MessageType.INFO, | ||
| text: t('Removed directory: {{directory}}', { directory }), |
There was a problem hiding this comment.
[Critical] The loadServerHierarchicalMemory call (line 497) is wrapped in try/catch that appends an error message — but addItem({ type: INFO, text: 'Removed directory: ...' }) at line 532 executes unconditionally after the catch block. If memory refresh fails, the user sees a success message while stale QWEN.md rules from the removed directory remain active for the rest of the session.
| text: t('Removed directory: {{directory}}', { directory }), | |
| // Move success message inside the try block, or guard it: | |
| if (config.shouldLoadMemoryFromIncludeDirectories()) { | |
| try { | |
| // ... refresh memory ... | |
| addItem({ type: MessageType.INFO, text: t('Removed directory: {{directory}}', { directory }) }, Date.now()); | |
| } catch (error) { | |
| addItem({ type: MessageType.ERROR, text: t('Error refreshing memory: {{error}}', ...) }, Date.now()); | |
| addItem({ type: MessageType.WARNING, text: t('Directory removed from settings but memory may still contain stale content.') }, Date.now()); | |
| } | |
| } else { | |
| addItem({ type: MessageType.INFO, text: t('Removed directory: {{directory}}', { directory }) }, Date.now()); | |
| } |
— deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| // Now remove from memory — persisted settings are already updated. | ||
| const removed = workspaceContext.removeDirectory(expandedDir); |
There was a problem hiding this comment.
[Suggestion] canonicalDirectory is computed at line 370 via fs.realpathSync(expandedDir) but removeDirectory(expandedDir) and the initial directory guard both pass expandedDir instead. While both methods re-resolve internally, the inconsistency means the three code paths can resolve to different canonical forms if a symlink changes between calls (TOCTOU).
| const removed = workspaceContext.removeDirectory(expandedDir); | |
| const removed = workspaceContext.removeDirectory(canonicalDirectory); |
— deepseek-v4-pro via Qwen Code /review
| }, | ||
| kind: CommandKind.BUILT_IN, | ||
| supportedModes: ['interactive'] as const, | ||
| completion: async (context: CommandContext) => { |
There was a problem hiding this comment.
[Suggestion] Two issues in the completion function:
- The filter
dirs.filter((d) => !initialDirs.includes(d))is O(n×m) — convertinitialDirsto a Set for O(1) lookup:const initialSet = new Set(initialDirs); return dirs.filter(d => !initialSet.has(d)); - The completion doesn't check
isRestrictiveSandbox()— in sandbox mode it will suggest directories that the action will reject. Add an early return[]whenservices.config?.isRestrictiveSandbox()is true.
— deepseek-v4-pro via Qwen Code /review
| }; | ||
|
|
||
| let callCount = 0; | ||
| mockSettings.setValue = vi.fn().mockImplementation(() => { |
There was a problem hiding this comment.
[Suggestion] The rollback test replaces setValue with a mock that throws without ever modifying settings objects, so the test doesn't verify actual state restoration. The shallow copy rollback bug (C2) is masked by this mock. Modify the mock to actually mutate workspace.originalSettings.context.includeDirectories before throwing, and assert the value is restored after rollback.
— deepseek-v4-pro via Qwen Code /review
| // Refresh hierarchical memory to drop QWEN.md content and | ||
| // conditional rules that were loaded from the removed directory, | ||
| // mirroring what the add path already does. | ||
| if (config.shouldLoadMemoryFromIncludeDirectories()) { |
There was a problem hiding this comment.
[Critical] This code path (shouldLoadMemoryFromIncludeDirectories → loadServerHierarchicalMemory) has zero test coverage — both success and error cases. All remove tests mock shouldLoadMemoryFromIncludeDirectories to return false. Add tests for: (a) refresh succeeds — verify setUserMemory, setGeminiMdFileCount, setConditionalRulesRegistry are called; (b) refresh throws — verify error message appears and success message is not shown.
— deepseek-v4-pro via Qwen Code /review
| return; | ||
| } | ||
|
|
||
| if (config.isRestrictiveSandbox()) { |
There was a problem hiding this comment.
[Critical] The isRestrictiveSandbox() early-return path has zero test coverage. Since sandbox is a security mode, a regression here could allow /directory remove to execute when it shouldn't. Add a test with isRestrictiveSandbox returning true and verify the sandbox error message is emitted and no settings mutation occurs.
— deepseek-v4-pro via Qwen Code /review
Address 15 review comments from three review rounds: Critical fixes: - Replace shallow copy rollback with structuredClone for deep copy (nested context.includeDirectories was mutated in-place, making rollback ineffective) - Two-phase settings commit: compute all scope changes first, then atomically commit. Track committed scopes for partial rollback. - Guard memory refresh error path: return after error so success message is not shown when refresh fails - Replace N+1 sync fs.realpathSync with async parallel fs.promises.realpath - Remove redundant dual initial-directory guard (isInitialDirectory already handles realpath resolution) - Add sandbox check to completion function - Convert O(n×m) completion filter to Set-based O(n) lookup Test fixes: - Change mockSettings type from Record<string, unknown> to any (resolves TS4111 noPropertyAccessFromIndexSignature errors) - Mock loadServerHierarchicalMemory for memory refresh tests - Add mockContext.config updates in tests that create new mockConfig - Add setGeminiMdFileCount to mockContext.ui New tests: - Sandbox mode removal: verifies error and no settings mutation - Memory refresh success: verifies setUserMemory, setGeminiMdFileCount, setConditionalRulesRegistry called - Memory refresh error: verifies error message and no success message - Deep copy rollback: verifies committed scopes are rolled back on later-scope failure
| } | ||
|
|
||
| // Snapshot deep copies for rollback before any mutations. | ||
| const workspaceBefore = { |
There was a problem hiding this comment.
[Critical] Phase 2 two-scope commit loop writes to disk immediately via setValue() for each scope. If the first scope (Workspace) succeeds and persists to disk, but the second scope (User) throws, the catch block restores in-memory settings/originalSettings snapshots and calls recomputeMerged(), but the workspace disk file has already been modified and is not rolled back. The user sees an error suggesting the operation failed, but the workspace-scope settings file on disk has been permanently mutated — creating an inconsistent state.
Impact: On next restart, the directory is silently missing from workspace settings. This is a data integrity issue — the error message is misleading and the system enters a state that contradicts what the UI reported.
| const workspaceBefore = { | |
| // Option A: batch all mutations in-memory first, then write all disk files in a | |
| // second pass with file-level rollback (restore from backup if any write fails). | |
| // Option B: write each scope's file to a temp location first, rename atomically | |
| // after all scopes succeed. | |
| // At minimum, document the partial-commit risk and consider a single-scope | |
| // rollback for the scope that already committed. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| 'Cannot remove initial workspace directory: {{directory}}', | ||
| 'Directory not found in workspace: {{directory}}': | ||
| 'Directory not found in workspace: {{directory}}', | ||
| 'Directory removed from workspace but error updating settings: {{error}}': |
There was a problem hiding this comment.
[Suggestion] Three i18n keys are added to all locale files (en.js, zh.js, zh-TW.js) but have zero references in production code:
Directory removed from workspace but error updating settings: {{error}}(en.js:1561, zh.js:1481, zh-TW.js:1322)Directory removed from workspace memory but no matching persisted entry was found. It may reappear on restart if stored under a different path format.(en.js:1569, zh.js:1489, zh-TW.js:1330)
These appear to be leftovers from an earlier iteration. They increase maintenance burden, bloat the i18n surface, and will confuse future translators.
| 'Directory removed from workspace but error updating settings: {{error}}': | |
| // Remove the four unused entries from all three locale files. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| canonicalDirectory = path.isAbsolute(expandedDir) | ||
| ? expandedDir | ||
| : path.resolve(expandedDir); | ||
| } |
There was a problem hiding this comment.
[Suggestion] When fs.realpathSync(expandedDir) fails (e.g., permission change, NFS hiccup), the initial-directory guard falls back to raw string comparison: initialDirectories.has(canonicalDirectory). However, initialDirectories is populated from WorkspaceContext.getInitialDirectories(), which stores realpath-resolved paths. If the initial directory's realpath differs from its raw path (common with symlinks), the fallback comparison returns false, bypassing the guard.
Impact: Under specific conditions (symlinked cwd + temporarily inaccessible directory), the initial directory can be removed from settings.
| } | |
| // When realpathSync fails, normalize both sides through path.resolve + | |
| // normalization rather than relying on realpathSync alone: | |
| const normalized = path.normalize(expandedDir); | |
| if (getInitialDirectories().some(d => path.normalize(d) === normalized)) return; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const resolutions = await Promise.all( | ||
| scopeDirs.map(async (d: string) => { | ||
| try { | ||
| return { |
There was a problem hiding this comment.
[Suggestion] The Phase 1 persistence filter condition r.resolved !== targetDir && r.original !== targetDir compares r.original (raw settings string) against targetDir (canonical path). When realpath fails for both input and settings entry (directory deleted from disk), the raw string comparison may miss entries stored in settings with ~ shorthand or non-normalized paths (e.g., /home/user/./project2).
Impact: Users who manually edit settings to use ~ or non-normalized paths cannot remove those entries via /directory remove. The command reports "not found" even though the entry exists in settings.
| return { | |
| .filter((r) => { | |
| const normalized = path.normalize(expandHomeDir(r.original)); | |
| return r.resolved !== targetDir && r.original !== targetDir && normalized !== targetDir; | |
| }) |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Now remove from memory — persisted settings are already updated. | ||
| const removed = workspaceContext.removeDirectory(expandedDir); | ||
| if (!removed) { | ||
| addItem( |
There was a problem hiding this comment.
[Suggestion] When loadServerHierarchicalMemory throws (memory refresh fails), the catch block shows an error and returns — skipping the success message Removed directory: {{directory}}. At this point, the directory has already been successfully removed from both settings and WorkspaceContext memory. The user only sees an error, not knowing the core operation succeeded.
Impact: If the user re-runs the command after seeing the error, the next attempt shows "not found" — creating confusion because the previous attempt "failed" yet left the system in a correct state. This is misleading UX, especially during debugging at odd hours.
| addItem( | |
| // Move the success message before the memory refresh, or make refresh best-effort: | |
| addItem({ type: MessageType.INFO, text: t('Removed directory: {{directory}}', { directory }) }, Date.now()); | |
| if (config.shouldLoadMemoryFromIncludeDirectories()) { | |
| try { /* refresh */ } catch (error) { | |
| addItem({ type: MessageType.WARNING, text: t('Directory removed but memory refresh failed: {{error}}', { error: (error as Error).message }) }, Date.now()); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
- Fix C3: Add disk-level rollback via setValueFullSave when partial scope commit fails - Fix C5: Move success message before memory refresh; make refresh best-effort with WARNING - Fix S1: Use canonicalDirectory in removeDirectory() call - Fix S4: Add normalized path fallback for initial-directory guard - Fix S5: Expand home dir and normalize raw paths in Phase 1 filter - Fix S6: Verify actual in-memory state restoration and disk rollback in rollback test - Add i18n key 'Directory removed but memory refresh failed' to en/zh/zh-TW - Add setValueFullSave mock to test setup and rollback tests - Update memory refresh failure test: expects WARNING + success instead of ERROR - Remove duplicate rollback test and fix describe block closing
|
Fixed the CI failure — LoadedSettings doesn't have a setValueFullSave method (I made a wrong assumption). Replaced it with the exported saveSettings function which directly writes the settings file to disk. In the catch block: restore in-memory state, then call saveSettings(settingsFile, originalSettings) to re-write the disk file for committed scopes. Mock saveSettings in tests via vi.mock. Typecheck and all 25 tests pass clean. |
| // setValue() writes to disk immediately, so on partial failure we | ||
| // must also restore the disk files for already-committed scopes. | ||
| const committed: SettingScope[] = []; | ||
| try { |
There was a problem hiding this comment.
[Critical] Incomplete rollback when setValue() throws — leaves in-memory settings corrupted.
LoadedSettings.setValue() (settings.ts:434-438) modifies settingsFile.settings and settingsFile.originalSettings in memory before calling saveSettings(). If saveSettings() fails (disk full, permissions), setValue throws — but the failing scope's memory is already dirty. The catch block only restores scopes in the committed array (pushed only after setValue returns successfully), so the failed scope is never restored. recomputeMerged() then reads inconsistent state.
If Workspace succeeds but User scope's saveSettings() fails, settings.user remains partially modified for the rest of the session.
| try { | |
| } catch (error) { | |
| // Always restore both scopes — setValue() modifies memory before | |
| // saveSettings(), so the failing scope is also dirty. | |
| settings.workspace.settings = workspaceBefore.settings; | |
| settings.workspace.originalSettings = workspaceBefore.originalSettings; | |
| settings.user.settings = userBefore.settings; | |
| settings.user.originalSettings = userBefore.originalSettings; | |
| // Rewrite disk for scopes that were actually committed | |
| for (const scope of committed) { | |
| if (scope === SettingScope.Workspace) { | |
| saveSettings(settings.workspace, workspaceBefore.originalSettings); | |
| } else { | |
| saveSettings(settings.user, userBefore.originalSettings); | |
| } | |
| } | |
| settings.recomputeMerged(); | |
| addItem( | |
| { | |
| type: MessageType.ERROR, | |
| text: t('Error updating settings: {{error}}', { | |
| error: (error as Error).message, | |
| }), | |
| }, | |
| Date.now(), | |
| ); | |
| return; | |
| } |
— deepseek-v4-pro via Qwen Code /review
| committed.push(change.scope); | ||
| } | ||
| } catch (error) { | ||
| // Roll back in-memory state and disk for committed scopes. |
There was a problem hiding this comment.
[Suggestion] saveSettings() in rollback catch has no error handling — double failure causes silent abort.
If the original error was caused by a full disk, the rollback saveSettings() calls will also throw. The second throw bypasses recomputeMerged() and the error message — the user sees no outcome.
| // Roll back in-memory state and disk for committed scopes. | |
| try { saveSettings(settings.workspace, workspaceBefore.originalSettings); } catch { /* best-effort rollback */ } |
— deepseek-v4-pro via Qwen Code /review
| } catch { | ||
| return { original: d, resolved: null }; | ||
| } | ||
| }), |
There was a problem hiding this comment.
[Suggestion] Dead filter clauses in persistence matching: r.original !== targetDir and partially normalized !== targetDir.
r.original is a raw persisted string (may contain ~, $HOME, relative paths); targetDir is a realpathSync-resolved absolute path. These will never match. The normalized comparison doesn't resolve symlinks. Only r.resolved !== targetDir (async realpath) produces correct matches.
Not a runtime bug (the r.resolved clause covers correct matching), but the dead clauses mislead readers into thinking multi-dimensional matching is happening.
| }), | |
| .filter((r) => { | |
| // Only realpath-resolved comparison reliably matches canonicalDirectory. | |
| return r.resolved !== targetDir; | |
| }) |
— deepseek-v4-pro via Qwen Code /review
| Date.now(), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] Unsafe (error as Error).message cast — displays "undefined" for non-Error throws.
If code throws a non-Error value (valid in JS: throw "string"), (error as Error).message evaluates to undefined. Same pattern at line 567 in the memory refresh catch.
| } | |
| error: error instanceof Error ? error.message : String(error), |
Apply the same fix at line 567.
— deepseek-v4-pro via Qwen Code /review
| const removableDir = path.normalize('/home/user/project2'); | ||
| mockWorkspaceContext = { | ||
| ...mockWorkspaceContext, | ||
| removeDirectory: vi.fn().mockReturnValue(true), |
There was a problem hiding this comment.
[Suggestion] Rollback test mock doesn't accurately simulate setValue() behavior — masks the Critical rollback bug.
The mock throws immediately without modifying mockSettings memory state. The real setValue() modifies settingsFile.settings and settingsFile.originalSettings before calling saveSettings(). The test passes but the production code fails to restore the failing scope's dirty memory — the test cannot detect this because it doesn't replicate the memory-before-disk mutation pattern.
Make the mock modify mockSettings.*.settings and mockSettings.*.originalSettings before throwing for the failing scope, then add assertions verifying the failing scope's state is fully restored.
— deepseek-v4-pro via Qwen Code /review
…18n keys. Heartfelt apologies for the delays in addressing the final suggestions!
|
All reviewer feedback has been addressed, including the final suggestions for i18n cleanup. Heartfelt apologies for the delays in getting these final pieces submitted! |
| ); | ||
|
|
||
| // Refresh hierarchical memory to drop QWEN.md content and | ||
| // conditional rules that were loaded from the removed directory, |
There was a problem hiding this comment.
[Critical] Model context is not updated after directory removal. The /directory add handler calls gemini.addDirectoryContext() (line 267) to refresh the model's awareness of workspace directories after adding paths. The remove handler has no equivalent call — it refreshes hierarchical memory (loadServerHierarchicalMemory) but never notifies the Gemini client that the workspace layout has changed.
After removal, the model's most recent directory-context history entry still contains the pre-removal workspace layout, so the model may continue referencing or attempting to access files from the removed directory.
| // conditional rules that were loaded from the removed directory, | |
| // Refresh the model's directory context so it's aware the | |
| // directory has been removed (mirrors the add path at ~L267). | |
| const gemini = config.getGeminiClient(); | |
| if (gemini) { | |
| await gemini.addDirectoryContext(); | |
| } | |
| // Refresh hierarchical memory to drop QWEN.md content and |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ); | ||
| }); | ||
|
|
||
| it('should roll back committed scopes when later scope fails', async () => { |
There was a problem hiding this comment.
[Suggestion] The rollback test does not assert that settings.recomputeMerged() is called. The implementation calls settings.recomputeMerged() in the catch block to rebuild the merged settings cache after rollback, but the test only verifies in-memory state restoration and saveSettings disk writes.
If recomputeMerged() were accidentally removed or broken, the test would still pass while the merged settings layer retained stale values for the rest of the session.
| it('should roll back committed scopes when later scope fails', async () => { | |
| // Verify merged settings were recomputed after rollback | |
| expect(mockSettings.recomputeMerged).toHaveBeenCalled(); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| expect(mockWorkspaceContext.removeDirectory).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should refresh memory after successful removal', async () => { |
There was a problem hiding this comment.
[Suggestion] The memory refresh test verifies config.setGeminiMdFileCount is called but does not assert context.ui.setGeminiMdFileCount is called. The implementation updates both the config and the TUI status bar (context.ui.setGeminiMdFileCount(fileCount) at ~line 490), but only the config-side call is verified in the test.
If the context.ui.setGeminiMdFileCount line is accidentally removed, the TUI status bar would display a stale file count after directory removal without any test catching it.
| it('should refresh memory after successful removal', async () => { | |
| expect(mockContext.ui.setGeminiMdFileCount).toHaveBeenCalledWith(0); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -0,0 +1,14 @@ | |||
| { | |||
There was a problem hiding this comment.
[Critical] A ~96MB binary file (.gitnexus/lbug) and this metadata file are accidentally committed to the PR. meta.json leaks the contributor's local filesystem path (/home/bamn/qwen-code). Once merged, the binary blob permanently inflates repo history for all future clones.
Add .gitnexus/ to .gitignore and remove both files from the branch:
git rm --cached -r .gitnexus/— qwen-latest-series-invite-beta-v28 via Qwen Code /review
|
|
||
| // Now remove from memory — persisted settings are already updated. | ||
| const removed = workspaceContext.removeDirectory(canonicalDirectory); | ||
| if (!removed) { |
There was a problem hiding this comment.
[Critical] removeDirectory() is called after Phase 2 commits settings to disk. If it returns false, the error message is shown but settings are already permanently modified — no rollback occurs. The directory is silently removed from persisted settings while remaining in the active workspace. On next session start, it disappears without explanation.
Fix: call removeDirectory() before Phase 2 commit (validate in-memory removal first), or roll back settings when it returns false using the existing snapshot mechanism:
| if (!removed) { | |
| // Now remove from memory — validate before persisting. | |
| const removed = workspaceContext.removeDirectory(canonicalDirectory); | |
| if (!removed) { | |
| addItem( | |
| { | |
| type: MessageType.ERROR, | |
| text: t( | |
| 'Directory removed from settings but could not be removed from the active workspace. It may still be accessible in this session.', | |
| ), | |
| }, | |
| Date.now(), | |
| ); | |
| // Roll back settings changes since in-memory removal failed | |
| for (const scope of committed) { | |
| if (scope === SettingScope.Workspace) { | |
| settings.workspace.settings = workspaceBefore.settings; | |
| settings.workspace.originalSettings = workspaceBefore.originalSettings; | |
| saveSettings(settings.workspace, workspaceBefore.originalSettings); | |
| } else { | |
| settings.user.settings = userBefore.settings; | |
| settings.user.originalSettings = userBefore.originalSettings; | |
| saveSettings(settings.user, userBefore.originalSettings); | |
| } | |
| } | |
| settings.recomputeMerged(); | |
| return; | |
| } |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| scope: SettingScope; | ||
| dirs: string[]; | ||
| }> = []; | ||
| for (const scope of [ |
There was a problem hiding this comment.
[Critical] The remove command iterates both Workspace and User scopes, but the add subcommand (line ~206) only writes to Workspace scope. If a directory exists in user-scope settings (~/.qwen/settings.json), /directory remove silently deletes it from the global config with no user-facing indication of which scope(s) were modified.
This violates the principle of least surprise — a workspace-level command modifies global settings without warning. Fix: either (a) inform the user which scope(s) were modified in the success message, or (b) restrict remove to the same scope(s) add writes to.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| }, | ||
| kind: CommandKind.BUILT_IN, | ||
| supportedModes: ['interactive'] as const, | ||
| completion: async (context: CommandContext) => { |
There was a problem hiding this comment.
[Suggestion] The completion function ignores the partialArg parameter — it always returns the full list of removable directories regardless of what the user has typed. Compare with add's completion (line 113) which uses getDirPathCompletions(partialArg) for prefix-filtered completions.
| completion: async (context: CommandContext) => { | |
| completion: async (context: CommandContext, partialArg: string) => { | |
| const { services } = context; | |
| if (!services.config) return []; | |
| if (services.config.isRestrictiveSandbox()) return []; | |
| const dirs = services.config.getWorkspaceContext().getDirectories(); | |
| const initialSet = new Set( | |
| services.config.getWorkspaceContext().getInitialDirectories(), | |
| ); | |
| return dirs | |
| .filter((d) => !initialSet.has(d)) | |
| .filter((d) => !partialArg || d.includes(partialArg)); | |
| }, |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| resolved: await fs.promises.realpath(expandHomeDir(d)), | ||
| }; | ||
| } catch { | ||
| return { original: d, resolved: null }; |
There was a problem hiding this comment.
[Suggestion] When fs.promises.realpath() fails for a stored settings entry (e.g., directory deleted from disk, permission change), the error is silently swallowed — resolved: null with no logging. Combined with zero debug logging in this command, path-matching failures are completely unobservable.
If the stored entry's raw string doesn't textually match targetDir and realpath fails, the entry silently survives the filter — the directory is not removed from that scope, with no warning to the user.
| return { original: d, resolved: null }; | |
| } catch (error) { | |
| config.getDebugLogger?.()?.warn( | |
| 'realpath failed for stored directory entry', | |
| { original: d, error: String(error) }, | |
| ); | |
| return { original: d, resolved: null }; | |
| } |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| ); | ||
| return dirs.filter((d) => !initialSet.has(d)); | ||
| }, | ||
| action: async (context: CommandContext, args: string) => { |
There was a problem hiding this comment.
[Suggestion] The entire remove action has zero debugLogger calls. Every other non-trivial command in commands/ (setupGithubCommand, extensionsCommand, skillsCommand, languageCommand, arenaCommand, clearCommand) uses debug logging at multiple decision points.
This command mutates persisted settings files on disk via setValue() and saveSettings(). At minimum, log: (1) input args and expandedDir, (2) canonicalDirectory and whether realpathSync succeeded, (3) each scope's before/after directory list, (4) committed scopes, (5) rollback trigger, (6) removeDirectory return value, (7) memory refresh outcome.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
55ead30 to
6f91cf9
Compare
Address 15 review comments from three review rounds: Critical fixes: - Replace shallow copy rollback with structuredClone for deep copy (nested context.includeDirectories was mutated in-place, making rollback ineffective) - Two-phase settings commit: compute all scope changes first, then atomically commit. Track committed scopes for partial rollback. - Guard memory refresh error path: return after error so success message is not shown when refresh fails - Replace N+1 sync fs.realpathSync with async parallel fs.promises.realpath - Remove redundant dual initial-directory guard (isInitialDirectory already handles realpath resolution) - Add sandbox check to completion function - Convert O(n×m) completion filter to Set-based O(n) lookup Test fixes: - Change mockSettings type from Record<string, unknown> to any (resolves TS4111 noPropertyAccessFromIndexSignature errors) - Mock loadServerHierarchicalMemory for memory refresh tests - Add mockContext.config updates in tests that create new mockConfig - Add setGeminiMdFileCount to mockContext.ui New tests: - Sandbox mode removal: verifies error and no settings mutation - Memory refresh success: verifies setUserMemory, setGeminiMdFileCount, setConditionalRulesRegistry called - Memory refresh error: verifies error message and no success message - Deep copy rollback: verifies committed scopes are rolled back on later-scope failure
- Fix C3: Add disk-level rollback via setValueFullSave when partial scope commit fails - Fix C5: Move success message before memory refresh; make refresh best-effort with WARNING - Fix S1: Use canonicalDirectory in removeDirectory() call - Fix S4: Add normalized path fallback for initial-directory guard - Fix S5: Expand home dir and normalize raw paths in Phase 1 filter - Fix S6: Verify actual in-memory state restoration and disk rollback in rollback test - Add i18n key 'Directory removed but memory refresh failed' to en/zh/zh-TW - Add setValueFullSave mock to test setup and rollback tests - Update memory refresh failure test: expects WARNING + success instead of ERROR - Remove duplicate rollback test and fix describe block closing
…18n keys. Heartfelt apologies for the delays in addressing the final suggestions!
| # code graph skills | ||
| .venv | ||
| .codegraph No newline at end of file | ||
| tmp/ No newline at end of file |
There was a problem hiding this comment.
[Suggestion] This PR removes the .venv and .codegraph entries (and the # code graph skills comment) from .gitignore, but these entries exist on origin/main. This change is unrelated to the /directory remove feature and appears to be a stale-branch artifact.
After merge, developers with .venv/ (Python virtual environments from the codegraph skill) or .codegraph/ index directories will see them as untracked files in git status, risking accidental commits of large binary index files.
| tmp/ | |
| tmp/ | |
| # code graph skills | |
| .venv | |
| .codegraph |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
|
|
||
| if (config.isRestrictiveSandbox()) { | ||
| addItem( | ||
| { |
There was a problem hiding this comment.
[Suggestion] The remove subcommand's sandbox guard uses addItem() + bare return, while the add subcommand (lines 150-158) returns a structured message object ({ type: 'message', messageType: 'error', content }). Both are in the same file, guarded by the same config.isRestrictiveSandbox() check, yet use different error-reporting patterns.
Consider aligning with the add pattern for consistency:
if (config.isRestrictiveSandbox()) {
return {
type: 'message' as const,
messageType: 'error' as const,
content: t(
'The /directory remove command is not supported in restrictive sandbox profiles.',
),
};
}— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| const expandedDir = expandHomeDir(directory); | ||
| let canonicalDirectory: string; | ||
| try { | ||
| canonicalDirectory = fs.realpathSync(expandedDir); |
There was a problem hiding this comment.
[Suggestion] fs.realpathSync is called at line 371, but all test paths (/home/user/project1, /home/user/project2, etc.) don't exist on disk, so realpathSync always throws and the catch fallback (path.isAbsolute(expandedDir) ? expandedDir : path.resolve(expandedDir)) always runs.
The symlink-resolution success path — which is the primary purpose of calling realpathSync — has zero test coverage. If there's a bug in how canonical paths are computed or compared, no test would catch it.
Consider mocking fs.realpathSync in at least one test to return a canonical path that differs from the input, and verify the persistence filter correctly matches by resolved path.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| // form (e.g. symlinked cwd). | ||
| const normalizedExpanded = path.normalize(expandedDir); | ||
| const initialDirs = workspaceContext.getInitialDirectories(); | ||
| if (initialDirs.some((d) => path.normalize(d) === normalizedExpanded)) { |
There was a problem hiding this comment.
[Suggestion] This second initial-directory guard (normalized-path fallback) has no test coverage. All existing tests that block initial-directory removal trigger the first guard (isInitialDirectory() returning true). No test exercises the scenario where isInitialDirectory() returns false but the normalized-path comparison matches.
This is a security/safety guard preventing removal of initial workspace directories. A regression here would silently allow users to remove the initial workspace directory when symlinks are involved.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| { | ||
| type: MessageType.ERROR, | ||
| text: t( | ||
| 'Directory not found in the active workspace or could not be removed.', |
There was a problem hiding this comment.
[Suggestion] The i18n key 'Directory not found in the active workspace or could not be removed.' used here is absent from all locale files (en.js, zh.js, zh-TW.js). Meanwhile, the locale files contain a different key — 'Directory removed from settings but could not be removed from the active workspace. It may still be accessible in this session.' — which has zero references in production code. The code and locale files have drifted apart during revisions.
Either update the code to use the existing locale key, or update the locale files to include this key (and remove the orphan entry).
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| const workspaceBefore = { | ||
| settings: structuredClone(settings.workspace.settings), | ||
| originalSettings: structuredClone( | ||
| settings.workspace.originalSettings, |
There was a problem hiding this comment.
[Suggestion] Four structuredClone() calls deep-clone the entire settings trees for both scopes, but setValue() only mutates context.includeDirectories. Cloning the full tree (which may contain rules, model configs, telemetry settings, etc.) is overkill.
Consider snapshotting only the array that setValue will mutate:
const workspaceDirsBefore = structuredClone(
settings.workspace.originalSettings.context?.includeDirectories ?? [],
);
const userDirsBefore = structuredClone(
settings.user.originalSettings.context?.includeDirectories ?? [],
);On rollback, restore just that property and call recomputeMerged().
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
6f91cf9 to
55ead30
Compare
Add /directory remove subcommand with tab-completion, initial directory guards, workspace settings persistence, and restrictive sandbox check. Warn on startup when --add-dir paths don't exist or aren't readable. Changes: - directoryCommand.tsx: new 'remove' subcommand (action, completion, error handling) - directoryCommand.tsx: remove persists to context.includeDirectories in settings - directoryCommand.tsx: scope-aware settings lookup (User + Workspace) - directoryCommand.tsx: resolve persisted raw entries via expandHomeDir/realpath - directoryCommand.tsx: warn if no persisted entry found after removal - directoryCommand.tsx: restrictive sandbox check (consistent with add) - directoryCommand.test.tsx: 5 new tests for remove subcommand - config.ts (cli): improved --add-dir help text description - config.ts (core): startup warning via process.stderr for invalid --add-dir paths - workspaceContext.ts: track skipped directories, expose getSkippedDirectories() - workspaceContext.test.ts: 4 new tests for getSkippedDirectories() - en.js, zh.js, zh-TW.js: 8 new i18n strings for remove subcommand
Critical fixes: - Fix noImplicitThis in test by converting forScope to arrow function - Restore deleted i18n key 'Already covered by existing directory' in en/zh/zh-TW - Persist settings before in-memory removal; skip removal if persist fails - Replace ?? with || for initial-directory guard (dead code fix) - Use filter instead of findIndex to remove all duplicate entries - Expand raw path in realpathSync fallback for path format mismatch Suggestion fixes: - Remove dead code getSkippedDirectories (zero production consumers)
- Make multi-scope settings update transactional: snapshot in-memory
state before the loop and rollback on partial failure so settings
stay consistent with disk.
- Treat failed in-memory removal as error even when a persisted
settings entry was found, so the user knows the directory is still
accessible in the current session.
- Add missing i18n keys: 'Error updating settings: {{error}}' and
'Directory removed from settings but could not be removed from the
active workspace...' in en/zh/zh-TW.
- Add tests for user-only removal, both-scope removal, partial-failure
rollback, and failed in-memory removal with settings updated.
Address 15 review comments from three review rounds: Critical fixes: - Replace shallow copy rollback with structuredClone for deep copy (nested context.includeDirectories was mutated in-place, making rollback ineffective) - Two-phase settings commit: compute all scope changes first, then atomically commit. Track committed scopes for partial rollback. - Guard memory refresh error path: return after error so success message is not shown when refresh fails - Replace N+1 sync fs.realpathSync with async parallel fs.promises.realpath - Remove redundant dual initial-directory guard (isInitialDirectory already handles realpath resolution) - Add sandbox check to completion function - Convert O(n×m) completion filter to Set-based O(n) lookup Test fixes: - Change mockSettings type from Record<string, unknown> to any (resolves TS4111 noPropertyAccessFromIndexSignature errors) - Mock loadServerHierarchicalMemory for memory refresh tests - Add mockContext.config updates in tests that create new mockConfig - Add setGeminiMdFileCount to mockContext.ui New tests: - Sandbox mode removal: verifies error and no settings mutation - Memory refresh success: verifies setUserMemory, setGeminiMdFileCount, setConditionalRulesRegistry called - Memory refresh error: verifies error message and no success message - Deep copy rollback: verifies committed scopes are rolled back on later-scope failure
- Fix C3: Add disk-level rollback via setValueFullSave when partial scope commit fails - Fix C5: Move success message before memory refresh; make refresh best-effort with WARNING - Fix S1: Use canonicalDirectory in removeDirectory() call - Fix S4: Add normalized path fallback for initial-directory guard - Fix S5: Expand home dir and normalize raw paths in Phase 1 filter - Fix S6: Verify actual in-memory state restoration and disk rollback in rollback test - Add i18n key 'Directory removed but memory refresh failed' to en/zh/zh-TW - Add setValueFullSave mock to test setup and rollback tests - Update memory refresh failure test: expects WARNING + success instead of ERROR - Remove duplicate rollback test and fix describe block closing
…ollback - Replace non-existent setValueFullSave with exported saveSettings function - Import saveSettings from config/settings.js - In catch block: restore in-memory state then call saveSettings() to re-write disk file for committed scopes - Mock saveSettings in tests via vi.mock - Update rollback test assertions to verify saveSettings called with correct scope snapshot - Remove setValueFullSave mock from test setup
…18n keys. Heartfelt apologies for the delays in addressing the final suggestions!
55ead30 to
d2f2a48
Compare
| addItem( | ||
| { | ||
| type: MessageType.INFO, | ||
| text: t('Removed directory: {{directory}}', { directory }), |
There was a problem hiding this comment.
[Critical] Missing gemini.addDirectoryContext() call after successful removal.
The add subcommand (line ~267) calls config.getGeminiClient()?.addDirectoryContext() after a successful add to update the LLM's chat history with the new workspace layout. The remove subcommand never makes this call. After /directory remove, the LLM's conversation history still reflects the old workspace — it may reference, read, or suggest files from the removed directory.
| text: t('Removed directory: {{directory}}', { directory }), | |
| const gemini = config.getGeminiClient(); | |
| if (gemini) { | |
| await gemini.addDirectoryContext(); | |
| } | |
| addItem( | |
| { | |
| type: MessageType.INFO, | |
| text: t('Removed directory: {{directory}}', { directory }), | |
| }, | |
| Date.now(), | |
| ); |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| } | ||
|
|
||
| // Now remove from memory — persisted settings are already updated. | ||
| const removed = workspaceContext.removeDirectory(canonicalDirectory); |
There was a problem hiding this comment.
[Critical] No rollback when removeDirectory() returns false.
At this point, Phase 2 has already committed settings changes to disk. If removeDirectory() returns false (e.g., race condition where the directory is deleted between Phase 1's async realpath and Phase 2's sync call), the code shows an error but doesn't roll back the persisted settings. This leaves disk and memory inconsistent: the settings file says the directory is removed, but the in-memory workspace still contains it. On next restart, the directory silently disappears.
The workspaceBefore/userBefore snapshots are still in scope — reuse the same rollback logic:
| const removed = workspaceContext.removeDirectory(canonicalDirectory); | |
| const removed = workspaceContext.removeDirectory(canonicalDirectory); | |
| if (!removed) { | |
| for (const scope of committed) { | |
| if (scope === SettingScope.Workspace) { | |
| settings.workspace.settings = workspaceBefore.settings; | |
| settings.workspace.originalSettings = workspaceBefore.originalSettings; | |
| saveSettings(settings.workspace, workspaceBefore.originalSettings); | |
| } else { | |
| settings.user.settings = userBefore.settings; | |
| settings.user.originalSettings = userBefore.originalSettings; | |
| saveSettings(settings.user, userBefore.originalSettings); | |
| } | |
| } | |
| settings.recomputeMerged(); | |
| addItem( | |
| { | |
| type: MessageType.ERROR, | |
| text: t('Could not remove directory from the active workspace. Settings were not changed.'), | |
| }, | |
| Date.now(), | |
| ); | |
| return; | |
| } |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| settings.workspace.originalSettings = | ||
| workspaceBefore.originalSettings; | ||
| // Re-write the disk file to match the restored in-memory state. | ||
| saveSettings( |
There was a problem hiding this comment.
[Suggestion] saveSettings() calls in the rollback catch block are not wrapped in try/catch. If saveSettings throws during rollback (e.g., disk I/O error), the for loop aborts mid-iteration: remaining scopes aren't rolled back, recomputeMerged() is skipped, and no error feedback is shown. Wrap each saveSettings call in its own try/catch to ensure the rollback always completes as much as possible.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| @@ -0,0 +1,14 @@ | |||
| { | |||
There was a problem hiding this comment.
[Suggestion] .gitnexus/lbug (binary) and .gitnexus/meta.json (containing local path /home/bamn/qwen-code) are artifacts from a code-graph indexing tool and should not be committed to the PR. Please remove with git rm -r .gitnexus/.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| }, | ||
| kind: CommandKind.BUILT_IN, | ||
| supportedModes: ['interactive'] as const, | ||
| completion: async (context: CommandContext) => { |
There was a problem hiding this comment.
[Suggestion] Two issues with the completion function:
-
No
partialArgfiltering: Unlike theaddsubcommand's completion (which usesgetDirPathCompletions(partialArg)for prefix matching), this function ignores the second argument and always returns all removable directories. Tab-completion won't narrow results as the user types. -
Zero test coverage: All three branches (no config, sandbox mode, normal filtering) are untested.
| completion: async (context: CommandContext) => { | |
| completion: async (context: CommandContext, partialArg: string) => { | |
| const { services } = context; | |
| if (!services.config) return []; | |
| if (services.config.isRestrictiveSandbox()) return []; | |
| const dirs = services.config.getWorkspaceContext().getDirectories(); | |
| const initialSet = new Set( | |
| services.config.getWorkspaceContext().getInitialDirectories(), | |
| ); | |
| const candidates = dirs.filter((d) => !initialSet.has(d)); | |
| const prefix = partialArg?.trim() ?? ''; | |
| if (!prefix) return candidates; | |
| return candidates.filter((d) => d.startsWith(prefix)); | |
| }, |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| // This is best-effort: a failure here does not roll back the | ||
| // directory removal, but the user is warned that stale content | ||
| // may remain for the rest of the session. | ||
| if (config.shouldLoadMemoryFromIncludeDirectories()) { |
There was a problem hiding this comment.
[Suggestion] The memory-refresh block (~25 lines: shouldLoadMemoryFromIncludeDirectories check, loadServerHierarchicalMemory call, 4 setter calls) is nearly line-for-line duplicated with the add subcommand (lines ~220-243). The only differences are the directory list argument and error handling style. Extract a shared helper to prevent drift:
async function refreshMemoryFromDirectories(
config: Config,
settings: LoadedSettings,
context: CommandContext,
): Promise<void> { ... }— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| 'Successfully added QWEN.md files from the following directories if there are:\n- {{directories}}': | ||
| 'Successfully added QWEN.md files from the following directories if there are:\n- {{directories}}', | ||
| 'Error refreshing memory: {{error}}': 'Error refreshing memory: {{error}}', | ||
| 'Directory removed but memory refresh failed: {{error}}': |
There was a problem hiding this comment.
[Suggestion] 6 locale files (ca.js, de.js, fr.js, ja.js, pt.js, ru.js) all have translations for the existing /directory add command strings but receive zero entries for the 8 new /directory remove i18n keys added here. Users of these locales will see English fallback strings mixed with their translated UI. Consider adding translations (even placeholder ones) to maintain consistency.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
| ); | ||
| }); | ||
|
|
||
| it('should correctly expand a Windows-style home directory path', () => { |
There was a problem hiding this comment.
[Suggestion] This expandHomeDir test was moved from the top-level describe('directoryCommand') into describe('remove'). It tests a general utility function used by both add and remove — its nesting under remove is misleading in test output (directoryCommand > remove > should correctly expand...). Consider moving it back to the top-level describe block.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds a /directory remove subcommand to complement the existing /directory add command. While the implementation shows good intent with two-phase commit, rollback logic, and comprehensive error handling, there are several critical issues that must be addressed before merging.
Note: This review acknowledges the extensive feedback already provided by @wenshao (57 inline comments). The concerns below are high-level architectural issues that warrant blocking the PR until resolved.
Critical Blocking Issues
-
Accidentally committed build artifacts (
.gitnexus/lbug- 96MB binary,.gitnexus/meta.json)- These files expose local filesystem paths (
/home/bamn/qwen-code) and will permanently bloat the repository - Must be removed with
git rm -r .gitnexusand force-pushed
- These files expose local filesystem paths (
-
Missing model context update after removal
- The
/directory addcommand callsgemini.addDirectoryContext()to update the LLM's awareness - The
removecommand lacks this call, leaving the model unaware that the directory was removed - This creates a disconnect between what the user sees and what the model knows about the workspace
- The
-
Incomplete rollback logic in two-phase commit
- Multiple issues with the rollback strategy when
setValue()throws:- Shallow copy snapshots don't capture nested mutations
saveSettings()in rollback has no error handling (double-failure scenario)- Order of operations is inconsistent (removeDirectory called before/after persistence)
- Multiple issues with the rollback strategy when
-
Test coverage gaps for critical paths
- Sandbox mode early-return has zero test coverage
- User-scope removal path is not tested
- Memory refresh error handling is not verified
Architectural Concerns
The implementation attempts a sophisticated two-phase commit pattern with rollback, but this complexity introduces subtle bugs:
- Phase 1 computes changes asynchronously with parallel realpath resolution
- Phase 2 commits scopes sequentially with immediate disk writes
- Rollback must restore both in-memory state AND disk files for already-committed scopes
This pattern is error-prone and the current implementation has multiple edge cases where partial failures leave the system in an inconsistent state. Consider whether a simpler approach (e.g., compute all changes, validate, then commit all at once) would be more maintainable.
Positive Aspects
- Good use of async realpath resolution to avoid blocking the event loop
- Proper sandbox guard
- Initial directory protection prevents accidental removal
- Comprehensive i18n coverage (en, zh, zh-TW)
- Memory refresh is best-effort with appropriate warnings
Recommendation
Request Changes - The PR must address the critical blocking issues above before merging. Specifically:
- Remove
.gitnexus/directory from the commit - Add
gemini.addDirectoryContext()call after successful removal - Simplify or fix the rollback logic
- Add test coverage for sandbox mode and user-scope removal
The feature itself is valuable and the implementation approach is reasonable, but the current state has too many critical issues to merge safely.
Deterministic analysis: 0 findings (tsc: 0 errors, eslint: 0 errors)
Existing review context: 57 inline comments already posted by @wenshao
— qwen-code via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds a /directory remove subcommand with a two-phase commit approach for settings persistence. The feature is well-structured and CI passes (17/17 checks green). However, several Critical issues from prior review rounds remain unaddressed in the latest commit.
Outstanding blockers
-
Accidental commit of
.gitnexus/directory — A ~96MB binary (.gitnexus/lbug) and a metadata file containing a local filesystem path (/home/bamn/qwen-code) are committed. These must be removed from the branch and added to.gitignore. -
Missing
gemini.addDirectoryContext()after removal — Theaddsubcommand callsgemini.addDirectoryContext()to update the model's context, but theremovesubcommand does not. This means the model retains stale directory context for the rest of the session after a removal. -
Incomplete rollback on partial
setValue()failure — The two-phase commit approach has rollback gaps:saveSettings()in the rollback catch block is not wrapped in its own try/catch, so a double-failure silently aborts rollback for remaining scopes. Additionally, whenremoveDirectory()returnsfalseafter settings have already been committed to disk, there is no rollback of the persisted changes. -
Sandbox early-return uses inconsistent pattern — The
removesubcommand usesaddItem()+ barereturnfor the sandbox guard, while theaddsubcommand returns a structured message object. This inconsistency could cause downstream handling differences.
What looks good
- The two-phase approach (compute pending changes, then commit) is a sound design for atomic multi-scope updates.
- Async parallel
realpathresolution in Phase 1 avoids N+1 sync I/O. - Comprehensive test suite covering: empty input, initial directory protection, not-found case, workspace/user/both-scope removal, rollback on partial failure, sandbox mode, memory refresh success, and memory refresh failure.
- i18n keys are properly added to
en.js,zh.js, andzh-TW.jswith correct translations. - The
--add-dirhelp text improvement inconfig.tsis a nice polish.
Recommendation
Please address the four blockers above (especially items 1 and 2, which are straightforward fixes). The remaining items from prior reviews are Suggestions that can be addressed in follow-up work.
— qwen-code via Qwen Code /review
- Remove .gitnexus/ artifacts from commit - Add gemini.addDirectoryContext() after removal (mirrors add subcommand) - Wrap saveSettings() rollback in try/catch to prevent silent abort on double-failure - Roll back persisted settings when removeDirectory() returns false - Fix unsafe error casts to use instanceof Error - Update i18n keys (remove dead keys, add rollback message, update sandbox text) - Add partialArg filtering to completion function
Local real-run verification report (maintainer, macOS)Verified Environment
Static checks
Behavioral matrix (real TUI)
E8 and E11 are the two that matter most and both held up: the realpath resolution makes removal match the stored canonical form even via a different spelling, and the scope-aware two-phase commit edits the correct scope file (User vs Workspace) without touching the other. Implementation note (not a defect)The remove path is unusually defensive for a list edit: it resolves each stored entry via async Two things to address before merge (neither is a code bug)
ConclusionThe actual code — the 中文版（Chinese version）本地真实运行验证报告（维护者,macOS）在真实交互式 TUI(tmux、隔离 环境
静态检查
行为矩阵（真实 TUI）
E8 与 E11 是最关键的两项,都站住了:realpath 解析让移除即使用不同拼写也能匹配存储的规范形式;scope-aware 的两阶段提交编辑了正确的 scope 文件(User vs Workspace),不波及另一个。 实现说明（非缺陷）remove 路径对于一次列表编辑而言异常防御性:并行 async 合并前需处理的两件事（都不是代码 bug）
结论实际代码—— |
|
@B-A-M-N heads up — this PR currently has merge conflicts with Conflicting files:
The rest merges cleanly. Thanks! 中文@B-A-M-N 提个醒 —— 这个 PR 目前和 冲突文件:
其余文件可以自动合并。谢谢! |
|
@B-A-M-N Thanks for this
return { type: MessageType.ERROR, content: '…' };— was replaced by string-literal fields: return { type: 'message' as const, messageType: 'error' as const, content: '…' };So after merging Could you:
Once that's in it should merge cleanly. (There were also a couple of earlier review points worth a second look.) 中文说明@B-A-M-N 感谢这个
麻烦你:
改完应该就能干净合并了。(另外早先 review 里还有几点也值得再看一下。) |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
No new blocking issues at this commit. The /directory remove subcommand is a careful two-phase-commit implementation with rollback, sandbox/initial-directory guards, memory refresh, and i18n. The substantive items — two-phase commit/rollback correctness, disk-rollback wrapping, the removeDirectory()===false path, sandbox-guard inconsistency, missing addDirectoryContext() after removal, test-coverage gaps, and the accidentally committed .gitnexus/ artifact — are already covered by the extensive existing review and are not repeated here.
— Qwen Code /review
Generated by Claude Code
| const candidates = dirs.filter((d) => !initialSet.has(d)); | ||
| const prefix = partialArg?.trim() ?? ''; | ||
| if (!prefix) return candidates; | ||
| return candidates.filter((d) => d.includes(prefix)); |
There was a problem hiding this comment.
[Bug — Correctness] The completion filter uses d.includes(prefix), which performs substring matching anywhere within the directory path. This is inconsistent with the add subcommand's completion function (getDirPathCompletions, line 86) which uses e.name.startsWith(namePrefix) for proper prefix-based filtering.
With substring matching, typing tmp would match /home/user/attempt-old (because tmp appears inside attempt), and typing project would match /home/user/my-project-backup as well as /home/user/project1. This leads to noisy and confusing completion results.
Suggested fix — use startsWith for consistent prefix matching:
return candidates.filter((d) => d.startsWith(prefix));Or, to keep substring flexibility, at least anchor to the basename like getDirPathCompletions does:
return candidates.filter((d) => path.basename(d).startsWith(path.basename(prefix)));
3 participants
Summary
Adds a
/directory removesubcommand to remove directories from the workspace context, complementing the existing/directory addcommand.Changes
packages/cli/src/ui/commands/directoryCommand.tsx: Added remove subcommand with path resolution, sandbox check, scope-aware settings update, and memory refreshpackages/cli/src/ui/commands/directoryCommand.test.tsx: Added 5 new tests for remove functionalitypackages/cli/src/config/config.ts: Improved --add-dir help textpackages/core/src/utils/workspaceContext.ts: AddedskippedDirectoriestracking andgetSkippedDirectories()methodpackages/core/src/utils/workspaceContext.test.ts: Added 4 new testspackages/cli/src/i18n/locales/en.js,zh.js,zh-TW.js: Added 8 new i18n keysSafety
realpathSyncbefore comparison