Address CodeRabbit review: prevent encoding/read-failure wedges

- McpServer: encode responses with JSON_INVALID_UTF8_SUBSTITUTE via a new
  encodeResponse() that never lets a bad encode terminate the STDIN loop
  (non-UTF-8 bytes from exec() tool output no longer wedge it on the response
  side); harden debugLog() the same way.
- DebugServer::readDbgpFrame(): don't consume the length header until the whole
  frame is buffered, so a mid-payload read failure leaves the buffer as an
  intact, resumable frame instead of desyncing later reads.
- DebugServer::setBreakpoint(): throw BreakpointException on an empty response
  (read failure) instead of returning the 'unknown' sentinel and recording a
  breakpoint that was never set.
- CompareRunner worktree test: assert the child exits non-zero so it pins the
  fatal-error path, not just shutdown-on-exit.

Author: koriym

src/DebugServer.php

@@ -1100,6 +1100,16 @@ private function setBreakpoint(string $filename, int $line, string|null $conditi
 
         $response = $this->sendCommand('breakpoint_set', $params);
 
+        // sendCommand() returns '' on a read failure/connection drop. Without
+        // this guard an empty response would slip past the <error> check and
+        // the id="..." regex and return the 'unknown' sentinel, recording a
+        // breakpoint that was never actually set.
+        if ($response === '') {
+            throw new BreakpointException(
+                "No response from Xdebug when setting breakpoint at {$filename}:{$line}",
+            );
+        }
+
         if (str_contains($response, '<error')) {
             $message = 'Xdebug rejected breakpoint_set';
             if (preg_match('/<message>([^<]+)<\/message>/', $response, $matches)) {
@@ -2554,18 +2564,24 @@ private function readDbgpFrame(Socket $socket): string
             // ASCII digits, so the first NUL is always the header delimiter.
             $nulPos = $this->fillDbgpBufferUntilNul($socket, $timeout);
             $length = (int) substr($this->dbgpReadBuffer, 0, $nulPos);
-            $this->dbgpReadBuffer = substr($this->dbgpReadBuffer, $nulPos + 1);
 
             if ($length <= 0) {
                 throw new RuntimeException("Invalid response length: {$length}");
             }
 
-            // Need $length data bytes plus the trailing NUL.
-            $this->fillDbgpBufferTo($socket, $timeout, $length + 1);
+            // Do NOT consume the header until the whole frame (header + NUL +
+            // payload + trailing NUL) is buffered. If the payload read fails
+            // partway (readTimeout fires, or the connection drops), the buffer
+            // must still look like an intact, resumable frame — otherwise the
+            // next read would misparse leftover payload bytes as a new length
+            // header and desync every subsequent frame on this socket.
+            $headerLength = $nulPos + 1;
+            $frameLength = $headerLength + $length + 1;
+            $this->fillDbgpBufferTo($socket, $timeout, $frameLength);
 
-            $response = substr($this->dbgpReadBuffer, 0, $length);
-            $trailingNull = $this->dbgpReadBuffer[$length];
-            $this->dbgpReadBuffer = substr($this->dbgpReadBuffer, $length + 1);
+            $response = substr($this->dbgpReadBuffer, $headerLength, $length);
+            $trailingNull = $this->dbgpReadBuffer[$headerLength + $length];
+            $this->dbgpReadBuffer = substr($this->dbgpReadBuffer, $frameLength);
 
             if ($trailingNull !== "\0") {
                 $this->log('Warning: Expected trailing NULL byte, got: ' . bin2hex($trailingNull));

src/McpServer.php

@@ -51,6 +51,7 @@
 use function trim;
 use function unlink;
 
+use const JSON_INVALID_UTF8_SUBSTITUTE;
 use const JSON_THROW_ON_ERROR;
 use const JSON_UNESCAPED_SLASHES;
 use const STDIN;
@@ -87,7 +88,10 @@ private function debugLog(string $message, array $data = []): void
             'message' => $message,
             'data' => $data,
         ];
-        error_log('MCP Debug: ' . json_encode($logData, JSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES));
+        // Substitute (don't throw on) non-UTF-8 bytes in the logged payload —
+        // an unset MCP_DEBUG already skips this, but with it set a non-UTF-8
+        // request line must not be able to wedge the loop from here either.
+        error_log('MCP Debug: ' . json_encode($logData, JSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES | JSON_INVALID_UTF8_SUBSTITUTE));
     }
 
     private function initializeTools(): void
@@ -276,14 +280,40 @@ public function __invoke(): void
                 }
 
                 $this->debugLog('Sending response', ['id' => $response->id]);
-                echo json_encode($response, JSON_THROW_ON_ERROR) . "\n";
+                echo $this->encodeResponse($response) . "\n";
                 fflush(STDOUT);
             }
         } catch (Throwable $e) {
             error_log('MCP Server Fatal Error: ' . $e->getMessage() . "\nStack trace: " . $e->getTraceAsString());
         }
     }
 
+    /**
+     * Encode a response for the wire without letting an encoding failure
+     * terminate the STDIN loop.
+     *
+     * Tool results embed the raw output of arbitrary scripts (exec()), which
+     * may contain non-UTF-8 bytes; those are substituted rather than thrown on,
+     * so the response is still delivered. On any other, unexpected encoding
+     * error we emit a protocol-valid error for the same id instead of dropping
+     * the response — a dropped response would leave the client waiting forever.
+     */
+    private function encodeResponse(JsonRpcResponse $response): string
+    {
+        try {
+            // JSON_INVALID_UTF8_SUBSTITUTE keeps non-UTF-8 tool output from
+            // throwing; the flag set otherwise matches the previous wire output.
+            return json_encode($response, JSON_THROW_ON_ERROR | JSON_INVALID_UTF8_SUBSTITUTE);
+        } catch (JsonException $e) {
+            error_log('MCP Server Error: failed to encode response: ' . $e->getMessage());
+
+            return json_encode(
+                JsonRpcResponse::error($response->id, -32603, 'Failed to encode response'),
+                JSON_THROW_ON_ERROR | JSON_INVALID_UTF8_SUBSTITUTE,
+            );
+        }
+    }
+
     /**
      * Parse and dispatch a single JSON-RPC request line.
      *

tests/Integration/CompareRunnerWorktreeTest.php

@@ -144,6 +144,7 @@ public function testWorktreeIsRemovedOnFatalErrorViaShutdownHandler(): void
             $worktreePath = trim(implode('', $output));
 
             $this->assertNotSame('', $worktreePath, 'child should print the worktree path before the fatal error');
+            $this->assertNotSame(0, $exit, 'child must exit non-zero via the fatal error this test exercises');
             clearstatcache(true, $worktreePath);
             $this->assertDirectoryDoesNotExist(
                 $worktreePath,

tests/Unit/McpServerTest.php

@@ -4,15 +4,20 @@
 
 namespace Koriym\XdebugMcp\Tests\Unit;
 
+use Koriym\XdebugMcp\DTO\GenericResult;
+use Koriym\XdebugMcp\DTO\JsonRpcResponse;
 use Koriym\XdebugMcp\Exceptions\InvalidArgumentException;
 use Koriym\XdebugMcp\McpServer;
 use PHPUnit\Framework\TestCase;
 use ReflectionClass;
 use Throwable;
 
 use function array_column;
+use function json_decode;
 use function putenv;
 
+use const JSON_THROW_ON_ERROR;
+
 class McpServerTest extends TestCase
 {
     private McpServer $server;
@@ -122,6 +127,23 @@ public function testHandleLineValidRequestAfterMalformedLineIsNotWedged(): void
         $this->assertArrayHasKey('tools', $second['result']);
     }
 
+    public function testEncodeResponseWithNonUtf8ToolOutputDoesNotThrow(): void
+    {
+        // Regression: tool results embed raw exec() output that may contain
+        // non-UTF-8 bytes. encodeResponse() must not throw (which would
+        // propagate to __invoke()'s outer catch and wedge the STDIN loop) — the
+        // bytes are substituted and a valid JSON response is still produced.
+        $response = JsonRpcResponse::success(7, new GenericResult([
+            'content' => [['type' => 'text', 'text' => "trace output \xff\xfe not utf-8"]],
+        ]));
+
+        $encoded = $this->invokePrivateMethod($this->server, 'encodeResponse', [$response]);
+
+        $decoded = json_decode($encoded, true, 512, JSON_THROW_ON_ERROR);
+        $this->assertSame(7, $decoded['id']);
+        $this->assertArrayHasKey('result', $decoded);
+    }
+
     public function testHandleLineNonObjectJsonReturnsInvalidRequest(): void
     {
         $responseObj = $this->invokePrivateMethod($this->server, 'handleLine', ['42']);