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);