Fix bug in detecting already added paths. (#17430)

This commit is contained in:
Jacob Richman
2026-01-26 10:12:40 -08:00
committed by GitHub
parent 50f89e8a41
commit 3e1a377d78
2 changed files with 69 additions and 23 deletions
@@ -17,9 +17,18 @@ import type { CommandContext, OpenCustomDialogActionReturn } from './types.js';
import { MessageType } from '../types.js'; import { MessageType } from '../types.js';
import * as os from 'node:os'; import * as os from 'node:os';
import * as path from 'node:path'; import * as path from 'node:path';
import * as fs from 'node:fs';
import * as trustedFolders from '../../config/trustedFolders.js'; import * as trustedFolders from '../../config/trustedFolders.js';
import type { LoadedTrustedFolders } from '../../config/trustedFolders.js'; import type { LoadedTrustedFolders } from '../../config/trustedFolders.js';
vi.mock('node:fs', async (importOriginal) => {
const actual = await importOriginal<typeof import('node:fs')>();
return {
...actual,
realpathSync: vi.fn((p) => p),
};
});
vi.mock('../utils/directoryUtils.js', async (importOriginal) => { vi.mock('../utils/directoryUtils.js', async (importOriginal) => {
const actual = const actual =
await importOriginal<typeof import('../utils/directoryUtils.js')>(); await importOriginal<typeof import('../utils/directoryUtils.js')>();
@@ -42,13 +51,14 @@ describe('directoryCommand', () => {
beforeEach(() => { beforeEach(() => {
mockWorkspaceContext = { mockWorkspaceContext = {
targetDir: path.resolve('/test/dir'),
addDirectory: vi.fn(), addDirectory: vi.fn(),
addDirectories: vi.fn().mockReturnValue({ added: [], failed: [] }), addDirectories: vi.fn().mockReturnValue({ added: [], failed: [] }),
getDirectories: vi getDirectories: vi
.fn() .fn()
.mockReturnValue([ .mockReturnValue([
path.normalize('/home/user/project1'), path.resolve('/home/user/project1'),
path.normalize('/home/user/project2'), path.resolve('/home/user/project2'),
]), ]),
} as unknown as WorkspaceContext; } as unknown as WorkspaceContext;
@@ -58,7 +68,7 @@ describe('directoryCommand', () => {
getGeminiClient: vi.fn().mockReturnValue({ getGeminiClient: vi.fn().mockReturnValue({
addDirectoryContext: vi.fn(), addDirectoryContext: vi.fn(),
}), }),
getWorkingDir: () => '/test/dir', getWorkingDir: () => path.resolve('/test/dir'),
shouldLoadMemoryFromIncludeDirectories: () => false, shouldLoadMemoryFromIncludeDirectories: () => false,
getDebugMode: () => false, getDebugMode: () => false,
getFileService: () => ({}), getFileService: () => ({}),
@@ -91,9 +101,9 @@ describe('directoryCommand', () => {
expect(mockContext.ui.addItem).toHaveBeenCalledWith( expect(mockContext.ui.addItem).toHaveBeenCalledWith(
expect.objectContaining({ expect.objectContaining({
type: MessageType.INFO, type: MessageType.INFO,
text: `Current workspace directories:\n- ${path.normalize( text: `Current workspace directories:\n- ${path.resolve(
'/home/user/project1', '/home/user/project1',
)}\n- ${path.normalize('/home/user/project2')}`, )}\n- ${path.resolve('/home/user/project2')}`,
}), }),
); );
}); });
@@ -125,7 +135,7 @@ describe('directoryCommand', () => {
}); });
it('should call addDirectory and show a success message for a single path', async () => { it('should call addDirectory and show a success message for a single path', async () => {
const newPath = path.normalize('/home/user/new-project'); const newPath = path.resolve('/home/user/new-project');
vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({ vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({
added: [newPath], added: [newPath],
failed: [], failed: [],
@@ -144,8 +154,8 @@ describe('directoryCommand', () => {
}); });
it('should call addDirectory for each path and show a success message for multiple paths', async () => { it('should call addDirectory for each path and show a success message for multiple paths', async () => {
const newPath1 = path.normalize('/home/user/new-project1'); const newPath1 = path.resolve('/home/user/new-project1');
const newPath2 = path.normalize('/home/user/new-project2'); const newPath2 = path.resolve('/home/user/new-project2');
vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({ vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({
added: [newPath1, newPath2], added: [newPath1, newPath2],
failed: [], failed: [],
@@ -166,7 +176,7 @@ describe('directoryCommand', () => {
it('should show an error if addDirectory throws an exception', async () => { it('should show an error if addDirectory throws an exception', async () => {
const error = new Error('Directory does not exist'); const error = new Error('Directory does not exist');
const newPath = path.normalize('/home/user/invalid-project'); const newPath = path.resolve('/home/user/invalid-project');
vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({ vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({
added: [], added: [],
failed: [{ path: newPath, error }], failed: [{ path: newPath, error }],
@@ -184,7 +194,7 @@ describe('directoryCommand', () => {
it('should add directory directly when folder trust is disabled', async () => { it('should add directory directly when folder trust is disabled', async () => {
if (!addCommand?.action) throw new Error('No action'); if (!addCommand?.action) throw new Error('No action');
vi.spyOn(trustedFolders, 'isFolderTrustEnabled').mockReturnValue(false); vi.spyOn(trustedFolders, 'isFolderTrustEnabled').mockReturnValue(false);
const newPath = path.normalize('/home/user/new-project'); const newPath = path.resolve('/home/user/new-project');
vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({ vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({
added: [newPath], added: [newPath],
failed: [], failed: [],
@@ -198,7 +208,7 @@ describe('directoryCommand', () => {
}); });
it('should show an info message for an already added directory', async () => { it('should show an info message for an already added directory', async () => {
const existingPath = path.normalize('/home/user/project1'); const existingPath = path.resolve('/home/user/project1');
if (!addCommand?.action) throw new Error('No action'); if (!addCommand?.action) throw new Error('No action');
await addCommand.action(mockContext, existingPath); await addCommand.action(mockContext, existingPath);
expect(mockContext.ui.addItem).toHaveBeenCalledWith( expect(mockContext.ui.addItem).toHaveBeenCalledWith(
@@ -212,9 +222,33 @@ describe('directoryCommand', () => {
); );
}); });
it('should show an info message for an already added directory specified as a relative path', async () => {
const existingPath = path.resolve('/home/user/project1');
const relativePath = './project1';
const absoluteRelativePath = path.resolve(
path.resolve('/test/dir'),
relativePath,
);
vi.mocked(fs.realpathSync).mockImplementation((p) => {
if (p === absoluteRelativePath) return existingPath;
return p as string;
});
if (!addCommand?.action) throw new Error('No action');
await addCommand.action(mockContext, relativePath);
expect(mockContext.ui.addItem).toHaveBeenCalledWith(
expect.objectContaining({
type: MessageType.INFO,
text: `The following directories are already in the workspace:\n- ${relativePath}`,
}),
);
});
it('should handle a mix of successful and failed additions', async () => { it('should handle a mix of successful and failed additions', async () => {
const validPath = path.normalize('/home/user/valid-project'); const validPath = path.resolve('/home/user/valid-project');
const invalidPath = path.normalize('/home/user/invalid-project'); const invalidPath = path.resolve('/home/user/invalid-project');
const error = new Error('Directory does not exist'); const error = new Error('Directory does not exist');
vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({ vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({
added: [validPath], added: [validPath],
@@ -318,7 +352,7 @@ describe('directoryCommand', () => {
it('should add a trusted directory', async () => { it('should add a trusted directory', async () => {
if (!addCommand?.action) throw new Error('No action'); if (!addCommand?.action) throw new Error('No action');
mockIsPathTrusted.mockReturnValue(true); mockIsPathTrusted.mockReturnValue(true);
const newPath = path.normalize('/home/user/trusted-project'); const newPath = path.resolve('/home/user/trusted-project');
vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({ vi.mocked(mockWorkspaceContext.addDirectories).mockReturnValue({
added: [newPath], added: [newPath],
failed: [], failed: [],
@@ -334,7 +368,7 @@ describe('directoryCommand', () => {
it('should return a custom dialog for an explicitly untrusted directory (upgrade flow)', async () => { it('should return a custom dialog for an explicitly untrusted directory (upgrade flow)', async () => {
if (!addCommand?.action) throw new Error('No action'); if (!addCommand?.action) throw new Error('No action');
mockIsPathTrusted.mockReturnValue(false); // DO_NOT_TRUST mockIsPathTrusted.mockReturnValue(false); // DO_NOT_TRUST
const newPath = path.normalize('/home/user/untrusted-project'); const newPath = path.resolve('/home/user/untrusted-project');
const result = await addCommand.action(mockContext, newPath); const result = await addCommand.action(mockContext, newPath);
@@ -357,7 +391,7 @@ describe('directoryCommand', () => {
it('should return a custom dialog for a directory with undefined trust', async () => { it('should return a custom dialog for a directory with undefined trust', async () => {
if (!addCommand?.action) throw new Error('No action'); if (!addCommand?.action) throw new Error('No action');
mockIsPathTrusted.mockReturnValue(undefined); mockIsPathTrusted.mockReturnValue(undefined);
const newPath = path.normalize('/home/user/undefined-trust-project'); const newPath = path.resolve('/home/user/undefined-trust-project');
const result = await addCommand.action(mockContext, newPath); const result = await addCommand.action(mockContext, newPath);
@@ -385,7 +419,7 @@ describe('directoryCommand', () => {
source: 'file', source: 'file',
}); });
mockIsPathTrusted.mockReturnValue(undefined); mockIsPathTrusted.mockReturnValue(undefined);
const newPath = path.normalize('/home/user/new-project'); const newPath = path.resolve('/home/user/new-project');
const result = await addCommand.action(mockContext, newPath); const result = await addCommand.action(mockContext, newPath);
@@ -20,6 +20,7 @@ import {
} from '../utils/directoryUtils.js'; } from '../utils/directoryUtils.js';
import type { Config } from '@google/gemini-cli-core'; import type { Config } from '@google/gemini-cli-core';
import * as path from 'node:path'; import * as path from 'node:path';
import * as fs from 'node:fs';
async function finishAddingDirectories( async function finishAddingDirectories(
config: Config, config: Config,
@@ -100,7 +101,7 @@ export const directoryCommand: SlashCommand = {
const workspaceContext = const workspaceContext =
context.services.config.getWorkspaceContext(); context.services.config.getWorkspaceContext();
const existingDirs = new Set( const existingDirs = new Set(
workspaceContext.getDirectories().map((dir) => path.normalize(dir)), workspaceContext.getDirectories().map((dir) => path.resolve(dir)),
); );
filteredSuggestions = suggestions.filter((s) => { filteredSuggestions = suggestions.filter((s) => {
@@ -172,12 +173,23 @@ export const directoryCommand: SlashCommand = {
const pathsToProcess: string[] = []; const pathsToProcess: string[] = [];
for (const pathToAdd of pathsToAdd) { for (const pathToAdd of pathsToAdd) {
const expandedPath = expandHomeDir(pathToAdd.trim()); const trimmedPath = pathToAdd.trim();
if (currentWorkspaceDirs.includes(expandedPath)) { const expandedPath = expandHomeDir(trimmedPath);
alreadyAdded.push(pathToAdd.trim()); try {
} else { const absolutePath = path.resolve(
pathsToProcess.push(pathToAdd.trim()); workspaceContext.targetDir,
expandedPath,
);
const resolvedPath = fs.realpathSync(absolutePath);
if (currentWorkspaceDirs.includes(resolvedPath)) {
alreadyAdded.push(trimmedPath);
continue;
}
} catch (_e) {
// Path might not exist or be inaccessible.
// We'll let batchAddDirectories handle it later.
} }
pathsToProcess.push(trimmedPath);
} }
if (alreadyAdded.length > 0) { if (alreadyAdded.length > 0) {