Remove conflicting shell Directory checks (#7845)

Co-authored-by: gemini-cli-robot <gemini-cli-robot@google.com>
Co-authored-by: Sandy Tao <sandytao520@icloud.com>

packages/core/src/tools/shell.test.ts

@@ -56,7 +56,9 @@ describe('ShellTool', () => {
       getDebugMode: vi.fn().mockReturnValue(false),
       getTargetDir: vi.fn().mockReturnValue('/test/dir'),
       getSummarizeToolOutputConfig: vi.fn().mockReturnValue(undefined),
-      getWorkspaceContext: () => createMockWorkspaceContext('.'),
+      getWorkspaceContext: vi
+        .fn()
+        .mockReturnValue(createMockWorkspaceContext('/test/dir')),
       getGeminiClient: vi.fn(),
       getShouldUseNodePtyShell: vi.fn().mockReturnValue(false),
     } as unknown as Config;
@@ -107,13 +109,32 @@ describe('ShellTool', () => {
       );
     });
 
-    it('should throw an error for a non-existent directory', () => {
-      vi.mocked(fs.existsSync).mockReturnValue(false);
+    it('should throw an error for a relative directory path', () => {
       expect(() =>
         shellTool.build({ command: 'ls', directory: 'rel/path' }),
-      ).toThrow(
-        "Directory 'rel/path' is not a registered workspace directory.",
+      ).toThrow('Directory must be an absolute path.');
+    });
+
+    it('should throw an error for a directory outside the workspace', () => {
+      (mockConfig.getWorkspaceContext as Mock).mockReturnValue(
+        createMockWorkspaceContext('/test/dir', ['/another/workspace']),
       );
+      expect(() =>
+        shellTool.build({ command: 'ls', directory: '/not/in/workspace' }),
+      ).toThrow(
+        "Directory '/not/in/workspace' is not within any of the registered workspace directories.",
+      );
+    });
+
+    it('should return an invocation for a valid absolute directory path', () => {
+      (mockConfig.getWorkspaceContext as Mock).mockReturnValue(
+        createMockWorkspaceContext('/test/dir', ['/another/workspace']),
+      );
+      const invocation = shellTool.build({
+        command: 'ls',
+        directory: '/test/dir/subdir',
+      });
+      expect(invocation).toBeDefined();
     });
   });
 
@@ -151,7 +172,7 @@ describe('ShellTool', () => {
       const wrappedCommand = `{ my-command & }; __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`;
       expect(mockShellExecutionService).toHaveBeenCalledWith(
         wrappedCommand,
-        expect.any(String),
+        '/test/dir',
         expect.any(Function),
         mockAbortSignal,
         false,
@@ -161,6 +182,30 @@ describe('ShellTool', () => {
       expect(vi.mocked(fs.unlinkSync)).toHaveBeenCalledWith(tmpFile);
     });
 
+    it('should use the provided directory as cwd', async () => {
+      (mockConfig.getWorkspaceContext as Mock).mockReturnValue(
+        createMockWorkspaceContext('/test/dir'),
+      );
+      const invocation = shellTool.build({
+        command: 'ls',
+        directory: '/test/dir/subdir',
+      });
+      const promise = invocation.execute(mockAbortSignal);
+      resolveShellExecution();
+      await promise;
+
+      const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp');
+      const wrappedCommand = `{ ls; }; __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`;
+      expect(mockShellExecutionService).toHaveBeenCalledWith(
+        wrappedCommand,
+        '/test/dir/subdir',
+        expect.any(Function),
+        mockAbortSignal,
+        false,
+        {},
+      );
+    });
+
     it('should not wrap command on windows', async () => {
       vi.mocked(os.platform).mockReturnValue('win32');
       const invocation = shellTool.build({ command: 'dir' });
@@ -178,7 +223,7 @@ describe('ShellTool', () => {
       await promise;
       expect(mockShellExecutionService).toHaveBeenCalledWith(
         'dir',
-        expect.any(String),
+        '/test/dir',
         expect.any(Function),
         mockAbortSignal,
         false,
@@ -229,12 +274,9 @@ describe('ShellTool', () => {
     });
 
     it('should throw an error for invalid directory', () => {
-      vi.mocked(fs.existsSync).mockReturnValue(false);
       expect(() =>
         shellTool.build({ command: 'ls', directory: 'nonexistent' }),
-      ).toThrow(
-        `Directory 'nonexistent' is not a registered workspace directory.`,
-      );
+      ).toThrow('Directory must be an absolute path.');
     });
 
     it('should summarize output when configured', async () => {
@@ -383,38 +425,3 @@ describe('ShellTool', () => {
     });
   });
 });
-
-describe('build', () => {
-  it('should return an invocation for valid directory', () => {
-    const config = {
-      getCoreTools: () => undefined,
-      getExcludeTools: () => undefined,
-      getTargetDir: () => '/root',
-      getWorkspaceContext: () =>
-        createMockWorkspaceContext('/root', ['/users/test']),
-    } as unknown as Config;
-    const shellTool = new ShellTool(config);
-    const invocation = shellTool.build({
-      command: 'ls',
-      directory: 'test',
-    });
-    expect(invocation).toBeDefined();
-  });
-
-  it('should throw an error for directory outside workspace', () => {
-    const config = {
-      getCoreTools: () => undefined,
-      getExcludeTools: () => undefined,
-      getTargetDir: () => '/root',
-      getWorkspaceContext: () =>
-        createMockWorkspaceContext('/root', ['/users/test']),
-    } as unknown as Config;
-    const shellTool = new ShellTool(config);
-    expect(() =>
-      shellTool.build({
-        command: 'ls',
-        directory: 'test2',
-      }),
-    ).toThrow('is not a registered workspace directory');
-  });
-});

packages/core/src/tools/shell.ts

@@ -130,10 +130,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
             return `{ ${command} }; __code=$?; pgrep -g 0 >${tempFilePath} 2>&1; exit $__code;`;
           })();
 
-      const cwd = path.resolve(
-        this.config.getTargetDir(),
-        this.params.directory || '',
-      );
+      const cwd = this.params.directory || this.config.getTargetDir();
 
       let cumulativeOutput: string | AnsiOutput = '';
       let lastUpdateTime = Date.now();
@@ -361,7 +358,7 @@ export class ShellTool extends BaseDeclarativeTool<
           directory: {
             type: 'string',
             description:
-              '(OPTIONAL) Directory to run the command in, if not the project root directory. Must be relative to the project root directory and must already exist.',
+              '(OPTIONAL) The absolute path of the directory to run the command in. If not provided, the project root directory is used. Must be a directory within the workspace and must already exist.',
           },
         },
         required: ['command'],
@@ -391,20 +388,16 @@ export class ShellTool extends BaseDeclarativeTool<
       return 'Could not identify command root to obtain permission from user.';
     }
     if (params.directory) {
-      if (path.isAbsolute(params.directory)) {
-        return 'Directory cannot be absolute. Please refer to workspace directories by their name.';
+      if (!path.isAbsolute(params.directory)) {
+        return 'Directory must be an absolute path.';
       }
       const workspaceDirs = this.config.getWorkspaceContext().getDirectories();
-      const matchingDirs = workspaceDirs.filter(
-        (dir) => path.basename(dir) === params.directory,
+      const isWithinWorkspace = workspaceDirs.some((wsDir) =>
+        params.directory!.startsWith(wsDir),
       );
 
-      if (matchingDirs.length === 0) {
-        return `Directory '${params.directory}' is not a registered workspace directory.`;
-      }
-
-      if (matchingDirs.length > 1) {
-        return `Directory name '${params.directory}' is ambiguous as it matches multiple workspace directories.`;
+      if (!isWithinWorkspace) {
+        return `Directory '${params.directory}' is not within any of the registered workspace directories.`;
       }
     }
     return null;