refactor: wrap proc_open in dedicated service class #16

New issue

Open

opened 2026-02-20 03:16:56 +00:00 by Clotho · 0 comments

Clotho commented

2026-02-20 03:16:56 +00:00

Member

Code Quality Issue

File: src/Api/Controllers/McpApiController.php
Lines: 459-481

Problem

Controller directly uses proc_open() for process management:

$process = proc_open(
    [
        'php',
        base_path('artisan'),
        'mcp:server',
        $server,
    ],
    $descriptors,
    $pipes,
    base_path(),
    null
);

Issues:

Low-level process handling in controller (violates SRP)
Potential resource leaks if process creation fails
Hard to test and mock
Error handling is minimal
Process cleanup logic scattered

Proposed Solution

Create ProcessExecutorService to encapsulate process management:

class ProcessExecutorService
{
    public function execute(array $command, ?string $stdin = null, int $timeout = 30): ProcessResult
    {
        // Encapsulate proc_open, stream handling, timeout, cleanup
        // Return structured result with stdout, stderr, exit code
    }
}

Benefits:

Testable via service mocking
Centralized error handling
Resource cleanup guaranteed
Reusable across application
Timeout handling in one place

Priority

Low - Works but not ideal architecture

Similar pattern might exist elsewhere - audit for other proc_open() usage.

## Code Quality Issue **File**: `src/Api/Controllers/McpApiController.php` **Lines**: 459-481 ### Problem Controller directly uses `proc_open()` for process management: ```php $process = proc_open( [ 'php', base_path('artisan'), 'mcp:server', $server, ], $descriptors, $pipes, base_path(), null ); ``` **Issues**: - Low-level process handling in controller (violates SRP) - Potential resource leaks if process creation fails - Hard to test and mock - Error handling is minimal - Process cleanup logic scattered ### Proposed Solution Create `ProcessExecutorService` to encapsulate process management: ```php class ProcessExecutorService { public function execute(array $command, ?string $stdin = null, int $timeout = 30): ProcessResult { // Encapsulate proc_open, stream handling, timeout, cleanup // Return structured result with stdout, stderr, exit code } } ``` **Benefits**: - Testable via service mocking - Centralized error handling - Resource cleanup guaranteed - Reusable across application - Timeout handling in one place ### Priority Low - Works but not ideal architecture ### Related Similar pattern might exist elsewhere - audit for other `proc_open()` usage.