From c7e87dfac721bfe4310cd3798ec0f35e3ab54847 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Sat, 22 Aug 2026 14:48:31 +0200 Subject: [PATCH] fix: keep the forked child's output out of the MCP transport (#3) stdout is the protocol stream, and the child inherits it across the fork. runInProcess() buffered PHPUnit's own output, but a test that fatals or calls exit() never reaches the matching ob_get_clean(): PHP flushes every active buffer during shutdown, straight to fd 1. Whatever user code had printed landed in front of the next JSON-RPC frame and, unterminated, glued itself to it -- `{"jsonrpc":...}`. Clients that frame on newlines cannot parse that line, so they block until their own timeout on a result that already arrived. Measured at five minutes per call, and no verdict, for a run that took two seconds. The child now seals stdout immediately after the fork with a never-ended output buffer whose callback returns nothing, so any phase -- destructors and shutdown included -- writes zero bytes. Sealing alone would trade a loud bug for a silent one: a debug echo, and the error page a framework renders on its way down, would both vanish. So the seal also registers a shutdown hook that ships the buffered output over the result socket, and the crash payload carries it under the existing `echo` key together with a message naming the fatal when PHP recorded one. The bytes that used to corrupt the channel are now the diagnostic that explains the crash. The hook fires only on the crash path: a completed run writes its payload and dies by SIGKILL, which runs no shutdown function. Transport purity is asserted in tearDown for every integration test rather than only the one written for it -- a leak is a property of the transport, so it should fail wherever it appears, not only where someone thought to look. Co-Authored-By: Max --- CHANGELOG.md | 11 ++ src/PhpunitRunner.php | 103 +++++++++++++- tests/Integration/ServerStdioTest.php | 192 ++++++++++++++++++++++++++ 3 files changed, 303 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48d7b4a..825cad8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,17 @@ This project follows [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Fixed + +- **Child output no longer reaches the MCP transport (#3).** The forked test child inherits stdout, which for this server *is* the protocol stream. `runInProcess()` buffered PHPUnit's own output, but a test that fatals or calls `exit()` never reaches the matching `ob_get_clean()` — PHP flushes every active buffer during shutdown, straight to fd 1. Whatever user code had printed then landed in front of the next JSON-RPC frame and, being unterminated, glued itself to it (`{"jsonrpc":...}`). Clients that frame on newlines could not parse that line and blocked until their own timeout on a result that had already arrived — measured at five minutes per call against a host application that renders an HTML error page on fatals. The child now seals stdout immediately after the fork with a never-ended output buffer whose callback returns nothing, so output from any phase, shutdown included, writes zero bytes. Results are unaffected: the child ships them over its socket pair. + +- **A dying child now reports what it printed, instead of losing it (#3).** Sealing stdout alone would have traded a loud failure for a silent one — a debug `echo`, or the error page a framework rendered on its way down, would simply vanish. The seal installs a shutdown hook that ships the buffered output over the result socket instead, so the crash payload carries it under the existing `echo` key, alongside a message naming the fatal (`... died before shipping a result: in :`) when PHP recorded one. The bytes that used to corrupt the channel are now the diagnostic that explains the crash. The hook fires only on the crash path: a completed run writes its payload and dies by `SIGKILL`, which runs no shutdown function. + +### Added + +- Integration test `ServerStdioTest::testChildOutputNeverReachesTheProtocolStream`: a fixture project whose test echoes and then exits, asserting that the transport carries only parseable JSON-RPC frames *and* that the crash result reports the child's output and cause of death. +- Transport purity is now asserted in `tearDown` for every integration test, not only the one written for it — a leak is a property of the transport, so it should fail wherever it appears rather than only where someone thought to look. + ## [0.3.0] — 2026-05-23 ### Security diff --git a/src/PhpunitRunner.php b/src/PhpunitRunner.php index bb541f2..97b8592 100644 --- a/src/PhpunitRunner.php +++ b/src/PhpunitRunner.php @@ -39,6 +39,13 @@ final class PhpunitRunner private static ?self $shared = null; + /** + * Set in the forked child once its result is on the wire, so the shutdown hook + * installed by {@see sealChildStdout()} does not ship a second, contradictory + * payload behind it. + */ + private static bool $childResultSent = false; + private bool $warm = false; private InMemorySubscriber $subscriber; @@ -140,6 +147,7 @@ public function run(array $argv): array if ($pid === 0) { // ---- CHILD ---- fclose($parentSock); + self::sealChildStdout($childSock); try { $payload = $this->runInProcess($argv); @@ -153,6 +161,7 @@ public function run(array $argv): array $json = json_encode($payload, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES); fwrite($childSock, (string) $json); fflush($childSock); + self::$childResultSent = true; fclose($childSock); $this->terminateChild(); @@ -209,6 +218,82 @@ public function run(array $argv): array return $payload; } + /** + * Make the forked child incapable of reaching stdout, and give whatever it + * printed somewhere better to go. + * + * stdout is the MCP transport. The child inherits it across the fork, so every + * byte user code prints lands in the parent's protocol stream -- in front of + * the next frame, and glued to it when the print is unterminated: + * `{"jsonrpc":...}`. A client that frames on newlines cannot parse that + * line, so it blocks until its own timeout on a result that already arrived. + * Five minutes per call, and no verdict, for a run that took two seconds (#3). + * + * `runInProcess()` already wraps Application::run in ob_start, but a buffer is + * only as good as its ending: a test that fatals or calls exit() never reaches + * the matching ob_get_clean(), and PHP flushes every active buffer on the way + * out -- to fd 1. So the seal is a buffer that is never ended and whose + * callback returns nothing: the shutdown flush still runs, and still writes + * zero bytes. Any phase is covered, destructors and shutdown functions included. + * + * Discarding alone would trade a loud bug for a silent one: the debug echo + * someone left in a test, and the fatal that killed the run, would both vanish. + * So the same hook ships them over the socket instead, and the parent reports + * them as the result. The bytes that used to corrupt the channel become the + * diagnostic that explains the crash. + * + * This only ever fires on the crash path. A run that completes writes its + * payload and dies by SIGKILL in {@see terminateChild()}, which runs no + * shutdown function at all; the flag covers the posix-less fallback there, + * where exit(0) does run them. + * + * @param resource $childSock + */ + private static function sealChildStdout($childSock): void + { + ob_start(static fn (string $buffer, int $phase): string => ''); + + register_shutdown_function(static function () use ($childSock): void { + if (self::$childResultSent) { + return; + } + + $printed = ob_get_contents(); + $payload = [ + 'exit_code' => 255, + 'output' => self::errorOutput( + self::deathMessage(error_get_last()), + is_string($printed) ? $printed : '', + ), + ]; + + @fwrite($childSock, (string) json_encode($payload, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES)); + @fflush($childSock); + }); + } + + /** + * Name what killed the child, when PHP recorded it. + * + * `error_get_last()` also returns warnings and notices that were survived, so + * only the fatal classes are quoted -- a warning from early in the run would + * name the wrong culprit with total confidence. + * + * @param array{type: int, message: string, file: string, line: int}|null $lastError + */ + private static function deathMessage(?array $lastError): string + { + $message = 'phpunit child died before shipping a result'; + + $fatal = [E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR]; + if ($lastError !== null && \in_array($lastError['type'], $fatal, true)) { + return $message . ': ' . $lastError['message'] + . ' in ' . $lastError['file'] . ':' . $lastError['line']; + } + + return $message . ' (no fatal recorded -- exit(), a signal, or a crash in an extension)'; + } + /** * Execute PHPUnit in the current process and collect the in-memory result. * @@ -288,10 +373,16 @@ private function terminateChild(): never /** * Build an InMemorySubscriber-shaped result JSON carrying a single error, so * the validator adapter renders the failure instead of choking on empty output. + * + * `$echoed` carries whatever the run printed, under the same `echo` key + * {@see runInProcess()} uses on the success path. It is the only route out for + * output produced by a run that died -- stdout is the MCP transport and the + * child is sealed off from it (#3) -- so a debug echo, or the error page a + * framework rendered on its way down, still reaches whoever reads the result. */ - private function errorOutput(string $message): string + private static function errorOutput(string $message, string $echoed = ''): string { - return (string) json_encode([ + $result = [ 'tests' => 0, 'assertions' => 0, 'failures' => [], @@ -304,7 +395,13 @@ private function errorOutput(string $message): string ]], 'skipped' => [], 'time' => 0.0, - ], JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES); + ]; + + if ($echoed !== '') { + $result['echo'] = $echoed; + } + + return (string) json_encode($result, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES); } private static function prewarmProbePath(): string diff --git a/tests/Integration/ServerStdioTest.php b/tests/Integration/ServerStdioTest.php index d35d788..da72f06 100644 --- a/tests/Integration/ServerStdioTest.php +++ b/tests/Integration/ServerStdioTest.php @@ -12,20 +12,52 @@ */ final class ServerStdioTest extends TestCase { + /** Printed by the child in the #3 fixture; must never appear on the transport. */ + private const CHILD_MARKER = ''; + private static string $bin; private static string $fixtureDir; /** @var list temp project dirs created per test, removed in tearDown */ private array $tmpDirs = []; + /** @var list every byte each spawned server wrote to stdout this test */ + private array $transcripts = []; + protected function tearDown(): void { + // Checked for every test rather than only the one that targets it (#3): + // a leak is a property of the transport, so the suite should fail wherever + // it appears, not only where someone thought to look for it. + foreach ($this->transcripts as $transcript) { + $this->assertTransportCarriesOnlyFrames($transcript); + } + $this->transcripts = []; + foreach ($this->tmpDirs as $dir) { $this->removeDir($dir); } $this->tmpDirs = []; } + /** + * Every non-empty line the server wrote must be a JSON-RPC frame and nothing + * else. Anything a client cannot parse belongs nowhere near this stream. + */ + private function assertTransportCarriesOnlyFrames(string $transcript): void + { + foreach (explode("\n", trim($transcript)) as $line) { + $line = trim($line); + if ($line === '') { + continue; + } + self::assertIsArray( + json_decode($line, true), + 'every line on the transport must be a JSON-RPC frame, got: ' . substr($line, 0, 200), + ); + } + } + private function removeDir(string $dir): void { if (!is_dir($dir)) { @@ -234,6 +266,164 @@ public function testEditedSourceIsReloadedAcrossCalls(): void } } + /** + * Regression for #3: nothing the forked child prints may reach stdout. + * + * stdout IS the MCP transport. A test that echoes and then dies leaves PHP's + * shutdown path to flush every active output buffer to fd 1, so the echoed + * bytes land in front of the next protocol frame and glue themselves to it: + * `{"jsonrpc":...}`. A client that frames on newlines cannot parse + * that line and waits out its entire timeout on an answer it already holds. + * Measured against a host application that renders an HTML error page on + * fatals: 45KB of it in the stream, and five minutes per call for a verdict + * the server had produced in two seconds. + * + * The assertion is deliberately about the transport rather than about the + * response: a fix that merely reordered the frames would still leave a + * client parsing HTML. + */ + public function testChildOutputNeverReachesTheProtocolStream(): void + { + $project = $this->makeEchoingCrashProject(); + + $stdout = $this->rawStdout($project, [ + ['jsonrpc' => '2.0', 'id' => 1, 'method' => 'initialize', 'params' => [ + 'protocolVersion' => '2024-11-05', + 'capabilities' => new \stdClass(), + 'clientInfo' => ['name' => 'phpunit', 'version' => '1.0.0'], + ]], + ['jsonrpc' => '2.0', 'method' => 'notifications/initialized'], + $this->runCall(2), + ]); + + // Deliberately NOT a substring check for the marker: once the child's output + // is reported rather than discarded, the marker travels inside the frame as + // data, which is the whole point. What must never happen is the marker + // reaching the stream as bytes of its own -- and that is exactly what frame + // purity states. Before the fix this failed on + // `...{"jsonrpc":...}`, which decodes as nothing. + $this->assertTransportCarriesOnlyFrames($stdout); + + // Sealed, not swallowed: the crash still has to be reported, and what the + // child printed on its way down is the most useful thing in the report. + $call = $this->frame($stdout, 2); + $output = json_decode($call['result']['structuredContent']['output'] ?? '', true); + self::assertIsArray($output, 'crash must still produce a result payload'); + + self::assertStringContainsString( + self::CHILD_MARKER, + $output['echo'] ?? '', + 'output printed by the dying child must be reported in the result', + ); + self::assertStringContainsString( + 'died before shipping a result', + $output['errors'][0]['message'] ?? '', + 'the result must say the child died rather than report an empty run', + ); + } + + /** + * The frame with $id, parsed out of a raw transcript. + * + * @return array + */ + private function frame(string $transcript, int $id): array + { + foreach (explode("\n", $transcript) as $line) { + $line = trim($line); + if ($line === '' || $line[0] !== '{') { + continue; + } + $decoded = json_decode($line, true); + if (is_array($decoded) && ($decoded['id'] ?? null) === $id) { + return $decoded; + } + } + + self::fail("no frame for id={$id} in: " . substr($transcript, 0, 500)); + } + + /** + * A project whose only test echoes and then ends the process, reproducing + * both halves of #3 at once: output produced by user code, and a child that + * dies before it can ship its result. + */ + private function makeEchoingCrashProject(): string + { + $dir = sys_get_temp_dir() . '/phpunit_mcp_leak_' . bin2hex(random_bytes(6)); + mkdir($dir . '/tests', 0777, true); + $this->tmpDirs[] = $dir; + + $marker = self::CHILD_MARKER; + file_put_contents($dir . '/tests/EchoingCrashTest.php', <<error page'; + exit(1); + } + } + PHP); + file_put_contents($dir . '/phpunit.xml', << + + + + tests + + + + XML); + + return $dir; + } + + /** + * The server's stdout verbatim — unfiltered, because the filtering is what + * this test exists to check. + * + * @param list> $messages + */ + private function rawStdout(string $project, array $messages): string + { + $cmd = [ + self::$bin, + '--no-prewarm', + '--working-dir=' . $project, + '--config=' . $project . '/phpunit.xml', + ]; + + $stdin = ''; + foreach ($messages as $message) { + $stdin .= json_encode($message) . "\n"; + } + + $proc = proc_open( + $cmd, + [0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes, + ); + self::assertIsResource($proc); + fwrite($pipes[0], $stdin); + fclose($pipes[0]); + $stdout = stream_get_contents($pipes[1]) ?: ''; + stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + proc_close($proc); + + $this->transcripts[] = $stdout; + + return $stdout; + } + /** * @return array{handle: resource, stdin: resource, stdout: resource, stderr: string} */ @@ -385,6 +575,8 @@ private function invoke(array $messages, bool $withProject): array fclose($pipes[2]); proc_close($proc); + $this->transcripts[] = $stdout; + $responses = []; foreach (explode("\n", $stdout) as $line) { $line = trim($line);