1 files changed, 52 insertions, 83 deletions

diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts
index 55720af5..96ff49a1 100644
--- a/packages/core/src/tools/shell.test.ts
+++ b/packages/core/src/tools/shell.test.ts
@@ -25,7 +25,6 @@ vi.mock('../utils/summarizer.js');
 
 import { isCommandAllowed } from '../utils/shell-utils.js';
 import { ShellTool } from './shell.js';
-import { ToolErrorType } from './tool-error.js';
 import { type Config } from '../config/config.js';
 import {
   type ShellExecutionResult,
@@ -93,22 +92,25 @@ describe('ShellTool', () => {
     });
   });
 
-  describe('validateToolParams', () => {
-    it('should return null for a valid command', () => {
-      expect(shellTool.validateToolParams({ command: 'ls -l' })).toBeNull();
+  describe('build', () => {
+    it('should return an invocation for a valid command', () => {
+      const invocation = shellTool.build({ command: 'ls -l' });
+      expect(invocation).toBeDefined();
     });
 
-    it('should return an error for an empty command', () => {
-      expect(shellTool.validateToolParams({ command: ' ' })).toBe(
+    it('should throw an error for an empty command', () => {
+      expect(() => shellTool.build({ command: ' ' })).toThrow(
         'Command cannot be empty.',
       );
     });
 
-    it('should return an error for a non-existent directory', () => {
+    it('should throw an error for a non-existent directory', () => {
       vi.mocked(fs.existsSync).mockReturnValue(false);
-      expect(
-        shellTool.validateToolParams({ command: 'ls', directory: 'rel/path' }),
-      ).toBe("Directory 'rel/path' is not a registered workspace directory.");
+      expect(() =>
+        shellTool.build({ command: 'ls', directory: 'rel/path' }),
+      ).toThrow(
+        "Directory 'rel/path' is not a registered workspace directory.",
+      );
     });
   });
 
@@ -134,10 +136,8 @@ describe('ShellTool', () => {
     };
 
     it('should wrap command on linux and parse pgrep output', async () => {
-      const promise = shellTool.execute(
-        { command: 'my-command &' },
-        mockAbortSignal,
-      );
+      const invocation = shellTool.build({ command: 'my-command &' });
+      const promise = invocation.execute(mockAbortSignal);
       resolveShellExecution({ pid: 54321 });
 
       vi.mocked(fs.existsSync).mockReturnValue(true);
@@ -159,8 +159,9 @@ describe('ShellTool', () => {
 
     it('should not wrap command on windows', async () => {
       vi.mocked(os.platform).mockReturnValue('win32');
-      const promise = shellTool.execute({ command: 'dir' }, mockAbortSignal);
-      resolveExecutionPromise({
+      const invocation = shellTool.build({ command: 'dir' });
+      const promise = invocation.execute(mockAbortSignal);
+      resolveShellExecution({
         rawOutput: Buffer.from(''),
         output: '',
         stdout: '',
@@ -182,10 +183,8 @@ describe('ShellTool', () => {
 
     it('should format error messages correctly', async () => {
       const error = new Error('wrapped command failed');
-      const promise = shellTool.execute(
-        { command: 'user-command' },
-        mockAbortSignal,
-      );
+      const invocation = shellTool.build({ command: 'user-command' });
+      const promise = invocation.execute(mockAbortSignal);
       resolveShellExecution({
         error,
         exitCode: 1,
@@ -204,40 +203,19 @@ describe('ShellTool', () => {
       expect(result.llmContent).not.toContain('pgrep');
     });
 
-    it('should return error with error property for invalid parameters', async () => {
-      const result = await shellTool.execute(
-        { command: '' }, // Empty command is invalid
-        mockAbortSignal,
-      );
-
-      expect(result.llmContent).toContain(
-        'Could not execute command due to invalid parameters:',
+    it('should throw an error for invalid parameters', () => {
+      expect(() => shellTool.build({ command: '' })).toThrow(
+        'Command cannot be empty.',
       );
-      expect(result.returnDisplay).toBe('Command cannot be empty.');
-      expect(result.error).toEqual({
-        message: 'Command cannot be empty.',
-        type: ToolErrorType.INVALID_TOOL_PARAMS,
-      });
     });
 
-    it('should return error with error property for invalid directory', async () => {
+    it('should throw an error for invalid directory', () => {
       vi.mocked(fs.existsSync).mockReturnValue(false);
-      const result = await shellTool.execute(
-        { command: 'ls', directory: 'nonexistent' },
-        mockAbortSignal,
-      );
-
-      expect(result.llmContent).toContain(
-        'Could not execute command due to invalid parameters:',
+      expect(() =>
+        shellTool.build({ command: 'ls', directory: 'nonexistent' }),
+      ).toThrow(
+        `Directory 'nonexistent' is not a registered workspace directory.`,
       );
-      expect(result.returnDisplay).toBe(
-        "Directory 'nonexistent' is not a registered workspace directory.",
-      );
-      expect(result.error).toEqual({
-        message:
-          "Directory 'nonexistent' is not a registered workspace directory.",
-        type: ToolErrorType.INVALID_TOOL_PARAMS,
-      });
     });
 
     it('should summarize output when configured', async () => {
@@ -248,7 +226,8 @@ describe('ShellTool', () => {
         'summarized output',
       );
 
-      const promise = shellTool.execute({ command: 'ls' }, mockAbortSignal);
+      const invocation = shellTool.build({ command: 'ls' });
+      const promise = invocation.execute(mockAbortSignal);
       resolveExecutionPromise({
         output: 'long output',
         rawOutput: Buffer.from('long output'),
@@ -280,9 +259,8 @@ describe('ShellTool', () => {
       });
       vi.mocked(fs.existsSync).mockReturnValue(true); // Pretend the file exists
 
-      await expect(
-        shellTool.execute({ command: 'a-command' }, mockAbortSignal),
-      ).rejects.toThrow(error);
+      const invocation = shellTool.build({ command: 'a-command' });
+      await expect(invocation.execute(mockAbortSignal)).rejects.toThrow(error);
 
       const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp');
       expect(vi.mocked(fs.unlinkSync)).toHaveBeenCalledWith(tmpFile);
@@ -299,11 +277,8 @@ describe('ShellTool', () => {
       });
 
       it('should throttle text output updates', async () => {
-        const promise = shellTool.execute(
-          { command: 'stream' },
-          mockAbortSignal,
-          updateOutputMock,
-        );
+        const invocation = shellTool.build({ command: 'stream' });
+        const promise = invocation.execute(mockAbortSignal, updateOutputMock);
 
         // First chunk, should be throttled.
         mockShellOutputCallback({
@@ -342,11 +317,8 @@ describe('ShellTool', () => {
       });
 
       it('should immediately show binary detection message and throttle progress', async () => {
-        const promise = shellTool.execute(
-          { command: 'cat img' },
-          mockAbortSignal,
-          updateOutputMock,
-        );
+        const invocation = shellTool.build({ command: 'cat img' });
+        const promise = invocation.execute(mockAbortSignal, updateOutputMock);
 
         mockShellOutputCallback({ type: 'binary_detected' });
         expect(updateOutputMock).toHaveBeenCalledOnce();
@@ -394,8 +366,8 @@ describe('ShellTool', () => {
   describe('shouldConfirmExecute', () => {
     it('should request confirmation for a new command and whitelist it on "Always"', async () => {
       const params = { command: 'npm install' };
-      const confirmation = await shellTool.shouldConfirmExecute(
-        params,
+      const invocation = shellTool.build(params);
+      const confirmation = await invocation.shouldConfirmExecute(
         new AbortController().signal,
       );
 
@@ -408,25 +380,21 @@ describe('ShellTool', () => {
       );
 
       // Should now be whitelisted
-      const secondConfirmation = await shellTool.shouldConfirmExecute(
-        { command: 'npm test' },
+      const secondInvocation = shellTool.build({ command: 'npm test' });
+      const secondConfirmation = await secondInvocation.shouldConfirmExecute(
         new AbortController().signal,
       );
       expect(secondConfirmation).toBe(false);
     });
 
-    it('should skip confirmation if validation fails', async () => {
-      const confirmation = await shellTool.shouldConfirmExecute(
-        { command: '' },
-        new AbortController().signal,
-      );
-      expect(confirmation).toBe(false);
+    it('should throw an error if validation fails', () => {
+      expect(() => shellTool.build({ command: '' })).toThrow();
     });
   });
 });
 
-describe('validateToolParams', () => {
-  it('should return null for valid directory', () => {
+describe('build', () => {
+  it('should return an invocation for valid directory', () => {
     const config = {
       getCoreTools: () => undefined,
       getExcludeTools: () => undefined,
@@ -435,14 +403,14 @@ describe('validateToolParams', () => {
         createMockWorkspaceContext('/root', ['/users/test']),
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const result = shellTool.validateToolParams({
+    const invocation = shellTool.build({
       command: 'ls',
       directory: 'test',
     });
-    expect(result).toBeNull();
+    expect(invocation).toBeDefined();
   });
 
-  it('should return error for directory outside workspace', () => {
+  it('should throw an error for directory outside workspace', () => {
     const config = {
       getCoreTools: () => undefined,
       getExcludeTools: () => undefined,
@@ -451,10 +419,11 @@ describe('validateToolParams', () => {
         createMockWorkspaceContext('/root', ['/users/test']),
     } as unknown as Config;
     const shellTool = new ShellTool(config);
-    const result = shellTool.validateToolParams({
-      command: 'ls',
-      directory: 'test2',
-    });
-    expect(result).toContain('is not a registered workspace directory');
+    expect(() =>
+      shellTool.build({
+        command: 'ls',
+        directory: 'test2',
+      }),
+    ).toThrow('is not a registered workspace directory');
   });
 });