diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 58df386f4d..e73f69e64a 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/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'); - }); -}); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 8e3390bab9..86eae238d1 100644 --- a/packages/core/src/tools/shell.ts +++ b/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;