From 2645f4404f58a532e8ea57c80c073d3303edbb3f Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Sun, 21 Jun 2026 23:37:54 +0200 Subject: [PATCH] fix: recover from warm-state scope corruption (reboot + retry) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warm Rector container can carry stale PHPStan scope/reflection state across edits — a class whose shape changed on disk between calls yields a null scope deep in PHPStanNodeScopeResolver ("Call to a member function toMutatingScope() on null"). It is not a real finding: a cold run on the same file passes. resetReflectionState() only flushes ResettableInterface services (source locators); PHPStan's scope/reflection caches are not resettable, so it cannot clear this. RectorTool::process now detects these internal corruption errors (toMutatingScope / "must be resolved" / "System error"), reboots the container (RectorRunner::reboot()) and retries once — a fresh container is the only guaranteed reset. Result is cold-quality instead of a false rector.error surfacing to the caller. Extract RunnerInterface so the recovery path is unit-testable with a double that simulates the corruption; RectorTool::withRunner() keeps __construct's concrete type for MCP SDK autowiring. Bump 0.3.0 -> 0.4.0. Co-Authored-By: Max --- bin/mcp-rector-warm | 2 +- src/RectorRunner.php | 16 ++- src/RectorTool.php | 50 ++++++++- src/RunnerInterface.php | 27 +++++ tests/Unit/RectorToolRecoveryTest.php | 143 ++++++++++++++++++++++++++ 5 files changed, 235 insertions(+), 3 deletions(-) create mode 100644 src/RunnerInterface.php create mode 100644 tests/Unit/RectorToolRecoveryTest.php diff --git a/bin/mcp-rector-warm b/bin/mcp-rector-warm index 3a55ca5..e783c0b 100755 --- a/bin/mcp-rector-warm +++ b/bin/mcp-rector-warm @@ -47,7 +47,7 @@ if ($config !== null) { // not here — to avoid polluting the MCP SDK before the server starts. $server = Server::builder() - ->setServerInfo('mcp-rector-warm', '0.3.0') + ->setServerInfo('mcp-rector-warm', '0.4.0') ->setDiscovery(dirname(__DIR__), ['src']) ->build(); diff --git a/src/RectorRunner.php b/src/RectorRunner.php index 1ce7bbe..2413fe5 100644 --- a/src/RectorRunner.php +++ b/src/RectorRunner.php @@ -11,7 +11,7 @@ * Holds a warm Rector container + Application across multiple analyse calls. * Boot happens lazily on first call; subsequent calls reuse the live container. */ -final class RectorRunner +final class RectorRunner implements RunnerInterface { private ?object $application = null; private ?object $container = null; @@ -25,6 +25,20 @@ public function isWarm(): bool return $this->application !== null; } + /** + * Drop the warm container + application so the next run() boots fresh. Used to + * recover from warm-state corruption that resetReflectionState() cannot flush: + * PHPStan's NodeScopeResolver/reflection caches are not ResettableInterface + * services, so a class whose shape changed on disk between warm calls can yield + * a null scope deep in PHPStanNodeScopeResolver ("Call to a member function + * toMutatingScope() on null"). A fresh container is the only guaranteed reset. + */ + public function reboot(): void + { + $this->application = null; + $this->container = null; + } + /** * Run a rector command. Returns ['exit_code' => int, 'output' => string, 'warm_boot' => bool]. * diff --git a/src/RectorTool.php b/src/RectorTool.php index 0956468..de40822 100644 --- a/src/RectorTool.php +++ b/src/RectorTool.php @@ -8,13 +8,28 @@ final class RectorTool { - private RectorRunner $runner; + private RunnerInterface $runner; public function __construct(?RectorRunner $runner = null) { $this->runner = $runner ?? new RectorRunner(); } + /** + * Test seam: build a tool around an arbitrary runner (e.g. a double that + * simulates warm-state corruption). Kept separate from __construct so the MCP + * SDK's constructor autowiring still sees a concrete RectorRunner type. + * + * @internal + */ + public static function withRunner(RunnerInterface $runner): self + { + $tool = new self(); + $tool->runner = $runner; + + return $tool; + } + /** * Run Rector on a path. Config + working dir are pinned at server startup (--working-dir, --config flags). * @@ -55,6 +70,21 @@ public function process(string $path, bool $dryRun = true): array try { return $this->runner->run($argv); } catch (\Throwable $e) { + // A warm container can corrupt across edits: PHPStan's scope/reflection + // caches are not ResettableInterface, so a class whose shape changed on + // disk yields a null scope deep in PHPStanNodeScopeResolver + // ("toMutatingScope() on null"). It is not a real finding — a cold run on + // the same file passes. Reboot the container and retry once so the caller + // gets the correct (cold-quality) result instead of a false rector.error. + if ($this->runner->isWarm() && self::isRecoverableWarmCorruption($e)) { + $this->runner->reboot(); + try { + return $this->runner->run($argv); + } catch (\Throwable $retryError) { + $e = $retryError; + } + } + return [ 'exit_code' => -1, 'output' => '', @@ -65,4 +95,22 @@ public function process(string $path, bool $dryRun = true): array ]; } } + + /** + * True for internal Rector/PHPStan errors that signal corrupted warm state + * rather than a genuine problem with the analysed code — recoverable by + * rebooting the container. Matched on message because the underlying type is a + * plain \Error (null method call) or Rector "System error". + */ + private static function isRecoverableWarmCorruption(\Throwable $error): bool + { + $message = $error->getMessage(); + foreach (['toMutatingScope', 'must be resolved', 'System error'] as $needle) { + if (str_contains($message, $needle)) { + return true; + } + } + + return false; + } } diff --git a/src/RunnerInterface.php b/src/RunnerInterface.php new file mode 100644 index 0000000..025f530 --- /dev/null +++ b/src/RunnerInterface.php @@ -0,0 +1,27 @@ + $argv Rector CLI args including the binary name as $argv[0]. + * @return array{exit_code: int, output: string, warm_boot: bool} + */ + public function run(array $argv): array; + + public function isWarm(): bool; + + /** + * Drop the warm container so the next run() boots a fresh one. Used to recover + * from warm-state corruption (stale PHPStan scope/reflection across edits). + */ + public function reboot(): void; +} diff --git a/tests/Unit/RectorToolRecoveryTest.php b/tests/Unit/RectorToolRecoveryTest.php new file mode 100644 index 0000000..f472e25 --- /dev/null +++ b/tests/Unit/RectorToolRecoveryTest.php @@ -0,0 +1,143 @@ +cwdBackup = getcwd() ?: '/'; + $this->workDir = sys_get_temp_dir() . '/mcp-rector-recover-' . bin2hex(random_bytes(4)); + mkdir($this->workDir, 0o700, true); + chdir($this->workDir); + } + + protected function tearDown(): void + { + chdir($this->cwdBackup); + @unlink($this->workDir . '/inside.php'); + @rmdir($this->workDir); + } + + private function insideFile(): string + { + $inside = $this->workDir . '/inside.php'; + file_put_contents($inside, "runs; + if ($this->runs === 1) { + throw new \Error('Call to a member function toMutatingScope() on null'); + } + return ['exit_code' => 0, 'output' => '{"totals":{"errors":0}}', 'warm_boot' => false]; + } + + public function isWarm(): bool + { + return true; + } + + public function reboot(): void + { + ++$this->reboots; + } + }; + + $tool = RectorTool::withRunner($fake); + $result = $tool->process($this->insideFile(), true); + + self::assertSame(0, $result['exit_code'], 'recovered run should succeed'); + self::assertSame(2, $fake->runs, 'should retry exactly once'); + self::assertSame(1, $fake->reboots, 'should reboot before retry'); + self::assertArrayNotHasKey('error', $result, 'no false error surfaced after recovery'); + } + + public function testDoesNotRetryUnrelatedError(): void + { + $fake = new class implements RunnerInterface { + public int $runs = 0; + public int $reboots = 0; + + public function run(array $argv): array + { + ++$this->runs; + throw new \RuntimeException('disk full'); + } + + public function isWarm(): bool + { + return true; + } + + public function reboot(): void + { + ++$this->reboots; + } + }; + + $tool = RectorTool::withRunner($fake); + $result = $tool->process($this->insideFile(), true); + + self::assertSame(-1, $result['exit_code']); + self::assertSame(1, $fake->runs, 'unrelated error must not be retried'); + self::assertSame(0, $fake->reboots); + self::assertSame('disk full', $result['error'] ?? null); + } + + public function testSurfacesErrorWhenRetryAlsoFails(): void + { + $fake = new class implements RunnerInterface { + public int $runs = 0; + public int $reboots = 0; + + public function run(array $argv): array + { + ++$this->runs; + throw new \Error('Call to a member function toMutatingScope() on null'); + } + + public function isWarm(): bool + { + return true; + } + + public function reboot(): void + { + ++$this->reboots; + } + }; + + $tool = RectorTool::withRunner($fake); + $result = $tool->process($this->insideFile(), true); + + self::assertSame(-1, $result['exit_code']); + self::assertSame(2, $fake->runs, 'recoverable error retried once then gives up'); + self::assertSame(1, $fake->reboots); + self::assertStringContainsString('toMutatingScope', $result['error'] ?? ''); + } +}