Merge pull request #184 from andreilungeanu/main

Always-on process isolation: eliminate conditional complexity

Author: ashleyhindle

src/Mcp/ToolExecutor.php

@@ -23,14 +23,10 @@ public function execute(string $toolClass, array $arguments = []): ToolResult
             return ToolResult::error("Tool not registered or not allowed: {$toolClass}");
         }
 
-        if ($this->shouldUseProcessIsolation()) {
-            return $this->executeInProcess($toolClass, $arguments);
-        }
-
-        return $this->executeInline($toolClass, $arguments);
+        return $this->executeInSubprocess($toolClass, $arguments);
     }
 
-    protected function executeInProcess(string $toolClass, array $arguments): ToolResult
+    protected function executeInSubprocess(string $toolClass, array $arguments): ToolResult
     {
         $command = $this->buildCommand($toolClass, $arguments);
 
@@ -45,7 +41,7 @@ protected function executeInProcess(string $toolClass, array $arguments): ToolRe
         $process = new Process(
             command: $command,
             env: $cleanEnv,
-            timeout: $this->getTimeout()
+            timeout: $this->getTimeout($arguments)
         );
 
         try {
@@ -62,7 +58,7 @@ protected function executeInProcess(string $toolClass, array $arguments): ToolRe
         } catch (ProcessTimedOutException $e) {
             $process->stop();
 
-            return ToolResult::error("Tool execution timed out after {$this->getTimeout()} seconds");
+            return ToolResult::error("Tool execution timed out after {$this->getTimeout($arguments)} seconds");
 
         } catch (ProcessFailedException $e) {
             $errorOutput = $process->getErrorOutput().$process->getOutput();
@@ -71,30 +67,11 @@ protected function executeInProcess(string $toolClass, array $arguments): ToolRe
         }
     }
 
-    protected function executeInline(string $toolClass, array $arguments): ToolResult
-    {
-        try {
-            /** @var \Laravel\Mcp\Server\Tool $tool */
-            $tool = app($toolClass);
-
-            return $tool->handle($arguments);
-        } catch (\Throwable $e) {
-            return ToolResult::error("Inline tool execution failed: {$e->getMessage()}");
-        }
-    }
-
-    protected function shouldUseProcessIsolation(): bool
+    protected function getTimeout(array $arguments): int
     {
-        if (app()->environment('testing')) {
-            return false;
-        }
+        $timeout = (int) ($arguments['timeout'] ?? 180);
 
-        return config('boost.process_isolation.enabled', true);
-    }
-
-    protected function getTimeout(): int
-    {
-        return config('boost.process_isolation.timeout', 180);
+        return max(1, min(600, $timeout));
     }
 
     /**

src/Mcp/Tools/Tinker.php

@@ -30,7 +30,7 @@ public function schema(ToolInputSchema $schema): ToolInputSchema
             ->description('PHP code to execute (without opening <?php tags)')
             ->required()
             ->integer('timeout')
-            ->description('Maximum execution time in seconds (default: 30)');
+            ->description('Maximum execution time in seconds (default: 180)');
     }
 
     /**
@@ -42,30 +42,14 @@ public function handle(array $arguments): ToolResult
     {
         $code = str_replace(['<?php', '?>'], '', (string) Arr::get($arguments, 'code'));
 
-        $timeout = min(180, (int) (Arr::get($arguments, 'timeout', 30)));
-        set_time_limit($timeout);
-        ini_set('memory_limit', '128M');
-
-        // Use PCNTL alarm for additional timeout control if available (Unix only)
-        if (function_exists('pcntl_async_signals') && function_exists('pcntl_signal')) {
-            pcntl_async_signals(true);
-            pcntl_signal(SIGALRM, function () {
-                throw new Exception('Code execution timed out');
-            });
-            pcntl_alarm($timeout);
-        }
+        ini_set('memory_limit', '256M');
 
         ob_start();
 
         try {
             $result = eval($code);
 
-            if (function_exists('pcntl_alarm')) {
-                pcntl_alarm(0);
-            }
-
             $output = ob_get_contents();
-            ob_end_clean();
 
             $response = [
                 'result' => $result,
@@ -79,20 +63,16 @@ public function handle(array $arguments): ToolResult
             }
 
             return ToolResult::json($response);
-
         } catch (Throwable $e) {
-            if (function_exists('pcntl_alarm')) {
-                pcntl_alarm(0);
-            }
-
-            ob_end_clean();
-
             return ToolResult::json([
                 'error' => $e->getMessage(),
                 'type' => get_class($e),
                 'file' => $e->getFile(),
                 'line' => $e->getLine(),
             ]);
+
+        } finally {
+            ob_end_clean();
         }
     }
 }

tests/Feature/Mcp/ToolExecutorTest.php

@@ -6,20 +6,7 @@
 use Laravel\Boost\Mcp\Tools\Tinker;
 use Laravel\Mcp\Server\Tools\ToolResult;
 
-test('can execute tool inline', function () {
-    // Disable process isolation for this test
-    config(['boost.process_isolation.enabled' => false]);
-
-    $executor = app(ToolExecutor::class);
-    $result = $executor->execute(ApplicationInfo::class, []);
-
-    expect($result)->toBeInstanceOf(ToolResult::class);
-});
-
-test('can execute tool with process isolation', function () {
-    // Enable process isolation for this test
-    config(['boost.process_isolation.enabled' => true]);
-
+test('can execute tool in subprocess', function () {
     // Create a mock that overrides buildCommand to work with testbench
     $executor = Mockery::mock(ToolExecutor::class)->makePartial()
         ->shouldAllowMockingProtectedMethods();
@@ -54,8 +41,6 @@
 });
 
 test('subprocess proves fresh process isolation', function () {
-    config(['boost.process_isolation.enabled' => true]);
-
     $executor = Mockery::mock(ToolExecutor::class)->makePartial()
         ->shouldAllowMockingProtectedMethods();
     $executor->shouldReceive('buildCommand')
@@ -76,8 +61,6 @@
 });
 
 test('subprocess sees modified autoloaded code changes', function () {
-    config(['boost.process_isolation.enabled' => true]);
-
     $executor = Mockery::mock(ToolExecutor::class)->makePartial()
         ->shouldAllowMockingProtectedMethods();
     $executor->shouldReceive('buildCommand')
@@ -149,3 +132,40 @@ function buildSubprocessCommand(string $toolClass, array $arguments): array
 
     return [PHP_BINARY, '-r', $testScript];
 }
+
+test('respects custom timeout parameter', function () {
+    $executor = Mockery::mock(ToolExecutor::class)->makePartial()
+        ->shouldAllowMockingProtectedMethods();
+    
+    $executor->shouldReceive('buildCommand')
+        ->andReturnUsing(fn ($toolClass, $arguments) => buildSubprocessCommand($toolClass, $arguments));
+
+    // Test with custom timeout - should succeed with fast code
+    $result = $executor->execute(Tinker::class, [
+        'code' => 'return "timeout test";',
+        'timeout' => 30
+    ]);
+
+    expect($result->isError)->toBeFalse();
+});
+
+test('clamps timeout values correctly', function () {
+    $executor = new ToolExecutor();
+    
+    // Test timeout clamping using reflection to access protected method
+    $reflection = new ReflectionClass($executor);
+    $method = $reflection->getMethod('getTimeout');
+    $method->setAccessible(true);
+    
+    // Test default
+    expect($method->invoke($executor, []))->toBe(180);
+    
+    // Test custom value
+    expect($method->invoke($executor, ['timeout' => 60]))->toBe(60);
+    
+    // Test minimum clamp
+    expect($method->invoke($executor, ['timeout' => 0]))->toBe(1);
+    
+    // Test maximum clamp
+    expect($method->invoke($executor, ['timeout' => 1000]))->toBe(600);
+});

tests/Feature/Mcp/Tools/TinkerTest.php

@@ -140,46 +140,3 @@
 
     expect($tool->shouldRegister())->toBeTrue();
 });
-
-test('uses custom timeout parameter', function () {
-    $tool = new Tinker;
-    $result = $tool->handle(['code' => 'return 2 + 2;', 'timeout' => 10]);
-
-    expect($result)->isToolResult()
-        ->toolJsonContentToMatchArray([
-            'result' => 4,
-            'type' => 'integer',
-        ]);
-});
-
-test('uses default timeout when not specified', function () {
-    $tool = new Tinker;
-    $result = $tool->handle(['code' => 'return 2 + 2;']);
-
-    expect($result)->isToolResult()
-        ->toolJsonContentToMatchArray([
-            'result' => 4,
-            'type' => 'integer',
-        ]);
-});
-
-test('times out when code takes too long', function () {
-    $tool = new Tinker;
-
-    // Code that will take more than 1 second to execute
-    $slowCode = '
-        $start = microtime(true);
-        while (microtime(true) - $start < 1.2) {
-            usleep(50000); // Don\'t waste entire CPU
-        }
-        return "should not reach here";
-    ';
-
-    $result = $tool->handle(['code' => $slowCode, 'timeout' => 1]);
-
-    expect($result)->isToolResult()
-        ->toolJsonContent(function ($data) {
-            expect($data)->toHaveKey('error')
-                ->and($data['error'])->toMatch('/(Maximum execution time|Code execution timed out)/');
-        });
-});