From f8954c983136be179f6448c677ba6e9b1a4044d8 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Sat, 25 Jul 2026 00:30:00 +0200 Subject: [PATCH] fix: respawn on same-file reflection staleness and post-boot classes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.6.0 excluded the analysis target from the staleness check, reasoning that phpstan re-reads the analysed file. That holds for the file's AST and not for its reflection. Editing a class-level @extends generic left the worker resolving $this->options against the shape captured at first analysis, so a correct fix kept reporting the error it had just fixed — and a newly introduced shape error would have been missed just as silently. The target is now fingerprinted by structure: declarations, signatures, property types and docblocks, with function bodies stripped. A changed fingerprint respawns the worker; a body edit does not, so the edit-check-edit loop on one file stays warm (measured 0.0s warm, ~6s only when the structure actually moves). Second bug, same root: the worker's analysable file set is fixed at boot, so a class created afterwards reads as "unknown class" for a file that a cold run resolves fine, with phantom errors at stale line numbers on top. Content alone cannot separate that from a genuinely missing class, so a result containing class.notFound now buys one respawn and reports the fresh worker's verdict. Third, found while building the end-to-end harness: findPhpstanBin resolved phpstan from the server's own vendor directory, so a global install analyses with the wrong phpstan — no project extensions, no custom rules, far too few errors, and no complaint. MCP_PHPSTAN_PHPSTAN_BIN now takes precedence and fails loudly on a bad path. Verified against a real project (~14k PHP files): both failures reproduce on 0.6.0 and are clean after, with a control proving the harness actually analyses — the first version of it reported CLEAN for everything because it was silently using the wrong phpstan. Co-Authored-By: Max --- CHANGELOG.md | 14 ++ README.md | 4 + src/PhpstanRunner.php | 172 ++++++++++++++-- tests/Unit/PhpstanRunnerStalenessTest.php | 236 ++++++++++++++++++++++ 4 files changed, 411 insertions(+), 15 deletions(-) create mode 100644 tests/Unit/PhpstanRunnerStalenessTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e93d312..eb56f42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,20 @@ This project follows [Semantic Versioning](https://semver.org/). ## [Unreleased] +## [0.7.0] — 2026-07-25 + +### Fixed + +- **Same-file reflection staleness: a class-level generic edit kept being checked against the pre-edit shape.** 0.6.0 excluded the analysis target from the staleness check on the grounds that "phpstan re-reads the analysed file". That is true of the file's **AST** and false of its **reflection**: after editing a class-level `@extends`/`@template` generic, the warm worker kept resolving `$this->options` against the shape captured at first analysis, so a genuine fix kept reporting the error it had just fixed (and, symmetrically, a newly *introduced* shape error would be missed). `PhpstanRunner` now fingerprints each analysed file's **structure** — declarations, signatures, property types and docblocks, with function bodies stripped — and respawns when the target's fingerprint changes. Body-only edits leave the fingerprint untouched, so the edit-validate-edit loop on one file stays fully warm (measured: 0.0s per warm call, ~6s only when the structure actually changes). +- **Classes created after the worker booted were invisible.** The worker's analysable file set is fixed at boot, so a class added afterwards produced `class.notFound` ("extends unknown class …") for a file sitting on disk that a cold run resolves fine — plus knock-on phantom errors at stale line numbers. Indistinguishable from a genuinely missing class by content alone, so `analyse()` now pays one respawn when a result contains `class.notFound` and returns the fresh worker's verdict. Bounded to a single retry per call. +- **A globally-installed server analysed with its own phpstan.** `findPhpstanBin()` resolved phpstan from the server's own vendor directory, so a global install would analyse a project with the wrong phpstan — missing every extension and custom rule the project's config declares, and reporting far too little. `MCP_PHPSTAN_PHPSTAN_BIN` now takes precedence and fails loudly when it points at nothing. + +### Added + +- `MCP_PHPSTAN_PHPSTAN_BIN` env var — point the server at the project's phpstan binary explicitly. +- `PhpstanRunner::structuralSignature()` / `targetStructureChanged()` / `hasUnknownClassError()` / `sendAnalyse()` (private). +- Unit tests `PhpstanRunnerStalenessTest` (11 cases): body-only edits keep the signature stable, class-level generics / method signatures / property types change it, formatting and line comments do not, bodyless abstract methods do not derail the scanner, plus the binary-override contract. + ## [0.6.0] — 2026-06-21 ### Fixed diff --git a/README.md b/README.md index 5f65ad9..e25d713 100644 --- a/README.md +++ b/README.md @@ -132,10 +132,14 @@ Three things worth knowing: 3. **Worker death is handled transparently.** If `proc_get_status()` shows the worker died, the next `phpstan_analyse` call respawns it. The `warm_boot: false` flag in the response signals this happened. +4. **Reflection is memoised for the worker's lifetime, so staleness is managed by respawning.** A warm worker never rebuilds a class's reflection, and the analysed file set is fixed at boot — so the server respawns when it detects that the answer would otherwise be stale: an analysed dependency changed on disk, the target's own *structure* changed (declarations, signatures, property types, docblocks — bodies excluded), or a result mentions an unknown class that may have been created after boot. Editing method bodies never triggers a respawn, so the usual edit-check-edit loop stays warm. + ## FAQ **Does this replace `vendor/bin/phpstan`?** No. Use it from MCP clients. For one-off CLI runs the regular binary is simpler. +**Which phpstan does it analyse with?** The one it resolves from its own install — correct when the server is a dev dependency of the project it analyses. If it is installed globally, or you are testing a server build against another project, set `MCP_PHPSTAN_PHPSTAN_BIN=/path/to/project/vendor/phpstan/phpstan/phpstan.phar`. Getting this wrong is quiet rather than loud: the wrong phpstan loads none of your config's extensions or custom rules and simply reports less. + **Can I analyse multiple files at once?** The current tool accepts one file per call. The underlying protocol supports `"files":[...]` arrays — multi-file support can be added as a separate tool. **Memory?** The daemon sets `memory_limit = -1`. Idle worker ≈ 60-80MB resident depending on project size and PHPStan level. diff --git a/src/PhpstanRunner.php b/src/PhpstanRunner.php index e01a1e2..54f7565 100644 --- a/src/PhpstanRunner.php +++ b/src/PhpstanRunner.php @@ -80,13 +80,14 @@ final class PhpstanRunner private ?int $workerBootedAt = null; /** - * Set of files the caller has analysed through this worker (path => true). + * Files the caller has analysed through this worker, mapped to the structural + * signature they had when the worker reflected them ({@see structuralSignature}). * Bounds the per-call staleness check to the working set instead of stat-ing * the whole --paths tree (tens of thousands of files on a real project). A * file here whose mtime is newer than {@see $workerBootedAt} is stale in the * worker and forces a respawn before the next dependent is analysed. * - * @var array + * @var array */ private array $analysedFiles = []; @@ -114,15 +115,21 @@ public function analyse(string $path): array return []; } - // Correctness over warmth when a dependency moved: if any file we've - // analysed OTHER than the target has changed since the worker booted, the - // worker's memoised reflection of it is stale and re-analysing won't - // refresh it — only a fresh worker will. Respawn before analysing so the - // dependent is checked against current reflection. The target itself is - // excluded (phpstan re-reads the analysed file's own AST each call), so - // iterating on a single file never respawns and stays fully warm. Checked - // before ensureWorker() so the teardown + reboot happen in one step. - if ($this->isWarm() && $this->dependencyChangedSinceBoot($path)) { + // Correctness over warmth when reflection moved under us. Two ways that + // happens, both needing a respawn — re-analysing never refreshes memoised + // reflection, only a fresh worker does: + // + // 1. A dependency we've analysed changed since boot (mtime check). + // 2. The TARGET's own structure changed since we reflected it. PHPStan + // re-reads the analysed file's AST every call, so body edits are seen + // immediately — but the class's memoised *reflection* is not rebuilt, + // so an edit to a class-level @extends/@template generic keeps being + // checked against the shape captured at first analysis. + // + // Body-only edits leave the structural signature untouched, so the common + // edit-validate-edit loop on one file stays fully warm. Checked before + // ensureWorker() so the teardown + reboot happen in one step. + if ($this->isWarm() && ($this->dependencyChangedSinceBoot($path) || $this->targetStructureChanged($path))) { $this->teardown(); } @@ -140,6 +147,34 @@ public function analyse(string $path): array return []; } + $errors = $this->extractErrors($this->sendAnalyse($path)); + + // A class created after the worker booted is invisible to it: the analysable + // file set is fixed at boot, so the worker reports "unknown class" for a class + // sitting on disk that a cold run resolves fine. Indistinguishable from a real + // missing class by content alone — so pay one respawn to find out, and trust + // the fresh worker. Bounded to a single retry per call. + if ($this->hasUnknownClassError($errors)) { + $this->teardown(); + $this->ensureWorker(); + $errors = $this->extractErrors($this->sendAnalyse($path)); + } + + // Remember the structure we reflected, so a later edit to it registers as + // stale — for this file as a dependency, and for itself on re-analysis. + $this->analysedFiles[$path] = $this->structuralSignature($path); + + return $errors; + } + + /** + * One analyse round-trip on the live worker stream. + * + * @return array the worker's raw `result` payload + * @throws \RuntimeException on protocol errors + */ + private function sendAnalyse(string $path): array + { $request = json_encode(['action' => 'analyse', 'files' => [$path]]) . "\n"; $written = @fwrite($this->workerStream, $request); if ($written === false || $written === 0) { @@ -157,11 +192,26 @@ public function analyse(string $path): array throw new \RuntimeException('Unexpected worker response: ' . trim($line)); } - // Remember we've reflected this file so a later edit to it registers as a - // stale dependency for whatever analyses it next. - $this->analysedFiles[$path] = true; + $result = $decoded['result'] ?? []; - return $this->extractErrors($decoded['result'] ?? []); + return is_array($result) ? $result : []; + } + + /** + * Errors that mean "the worker does not know this class" — the signature of a + * class file created after boot, not of a genuine mistake in the analysed file. + * + * @param array{file: string, line: int, message: string, identifier: string|null}[] $errors + */ + private function hasUnknownClassError(array $errors): bool + { + foreach ($errors as $error) { + if (($error['identifier'] ?? null) === 'class.notFound') { + return true; + } + } + + return false; } /** @@ -187,6 +237,84 @@ private function dependencyChangedSinceBoot(string $target): bool return false; } + /** + * True when the target's structure changed since this worker reflected it. + * Only meaningful for a file we have already analysed — an unseen file has no + * memoised reflection to be stale. + */ + private function targetStructureChanged(string $target): bool + { + $reflected = $this->analysedFiles[$target] ?? null; + + return $reflected !== null && $reflected !== $this->structuralSignature($target); + } + + /** + * Hash of everything in a file that shapes its reflection: declarations, type + * hints, docblocks — with function bodies removed. Editing a method body leaves + * this untouched (worker stays warm); changing a class-level generic, a + * signature, a property type or a docblock changes it (worker respawns). + * + * Regular comments are dropped, docblocks are kept — PHPStan reads the latter + * as types. Unparseable input hashes to the empty string, which compares equal + * across calls and so never forces a respawn on its own; phplint catches those + * files before they reach here anyway. + */ + private function structuralSignature(string $path): string + { + $source = @file_get_contents($path); + if ($source === false) { + return ''; + } + + $tokens = @token_get_all($source); + $signature = ''; + $depth = 0; + $bodyDepth = null; + $pendingFunction = false; + + foreach ($tokens as $token) { + $id = is_array($token) ? $token[0] : null; + $text = is_array($token) ? $token[1] : $token; + + if ($id === T_WHITESPACE || $id === T_COMMENT) { + continue; + } + + if ($id === T_FUNCTION) { + $pendingFunction = true; + } + + // Abstract and interface methods end at `;` with no body to skip. + if ($text === ';' && $pendingFunction) { + $pendingFunction = false; + } + + if ($text === '{') { + $depth++; + if ($pendingFunction && $bodyDepth === null) { + $bodyDepth = $depth; + $pendingFunction = false; + } + } elseif ($text === '}') { + if ($bodyDepth !== null && $depth === $bodyDepth) { + $bodyDepth = null; + $depth--; + continue; + } + $depth--; + } + + if ($bodyDepth !== null) { + continue; + } + + $signature .= $text . '|'; + } + + return hash('sha256', $signature); + } + /** * Ensure the worker is running and the handshake is complete. * Respawns transparently if the previous worker died. @@ -369,6 +497,20 @@ private function spawnWorker(int $port, string $identifier): mixed private function findPhpstanBin(): string { + // Explicit override wins. Needed whenever the server and the analysed project + // resolve to different phpstan installs — a global install of this server would + // otherwise analyse with its own phpstan, missing every extension and custom + // rule the project's config declares, and silently reporting far too little. + $override = getenv('MCP_PHPSTAN_PHPSTAN_BIN') ?: ''; + if ($override !== '') { + $resolved = realpath($override); + if ($resolved === false || !is_file($resolved)) { + throw new \RuntimeException('MCP_PHPSTAN_PHPSTAN_BIN does not point at a file: ' . $override); + } + + return $resolved; + } + // phpstan/phpstan ships as a phar — classes inside it are not directly reflectable. // Use Composer's InstalledVersions to locate the package directory, then resolve the phar. if (class_exists(\Composer\InstalledVersions::class)) { diff --git a/tests/Unit/PhpstanRunnerStalenessTest.php b/tests/Unit/PhpstanRunnerStalenessTest.php new file mode 100644 index 0000000..baec748 --- /dev/null +++ b/tests/Unit/PhpstanRunnerStalenessTest.php @@ -0,0 +1,236 @@ + */ + private array $tempFiles = []; + + protected function tearDown(): void + { + foreach ($this->tempFiles as $file) { + @unlink($file); + } + $this->tempFiles = []; + } + + private function writeTemp(string $source): string + { + $path = sys_get_temp_dir() . '/mcp-phpstan-sig-' . bin2hex(random_bytes(6)) . '.php'; + file_put_contents($path, $source); + $this->tempFiles[] = $path; + + return $path; + } + + private function signatureOf(PhpstanRunner $runner, string $path): string + { + $method = (new ReflectionClass($runner))->getMethod('structuralSignature'); + + return (string) $method->invoke($runner, $path); + } + + public function testBodyOnlyEditKeepsTheSignatureStable(): void + { + $runner = new PhpstanRunner(); + + $before = $this->writeTemp(<<<'PHP' + writeTemp(<<<'PHP' + signatureOf($runner, $before), + $this->signatureOf($runner, $after), + 'A body-only edit must not respawn the worker — that is the warm path.' + ); + } + + public function testClassLevelGenericEditChangesTheSignature(): void + { + $runner = new PhpstanRunner(); + + $before = $this->writeTemp(<<<'PHP' + + */ + final class Probe extends Base + { + } + PHP); + + $after = $this->writeTemp(<<<'PHP' + + */ + final class Probe extends Base + { + } + PHP); + + self::assertNotSame( + $this->signatureOf($runner, $before), + $this->signatureOf($runner, $after), + 'The class-level generic is the exact case that served stale results before this guard.' + ); + } + + public function testMethodSignatureEditChangesTheSignature(): void + { + $runner = new PhpstanRunner(); + + $before = $this->writeTemp("writeTemp("signatureOf($runner, $before), $this->signatureOf($runner, $after)); + } + + public function testPropertyTypeEditChangesTheSignature(): void + { + $runner = new PhpstanRunner(); + + $before = $this->writeTemp("writeTemp("signatureOf($runner, $before), $this->signatureOf($runner, $after)); + } + + public function testFormattingAndLineCommentsDoNotChangeTheSignature(): void + { + $runner = new PhpstanRunner(); + + $before = $this->writeTemp("writeTemp("signatureOf($runner, $before), + $this->signatureOf($runner, $after), + 'Whitespace and line comments must not cost a respawn.' + ); + } + + public function testAbstractMethodWithoutBodyDoesNotSwallowLaterDeclarations(): void + { + $runner = new PhpstanRunner(); + + $before = $this->writeTemp("writeTemp("signatureOf($runner, $before), + $this->signatureOf($runner, $after), + 'A bodyless abstract method must not leave the scanner stuck skipping the rest of the class.' + ); + } + + public function testTargetStructureChangedIsFalseForAFileNeverAnalysed(): void + { + $runner = new PhpstanRunner(); + $path = $this->writeTemp("getMethod('targetStructureChanged'); + + self::assertFalse( + (bool) $method->invoke($runner, $path), + 'An unseen file has no memoised reflection, so there is nothing stale to respawn for.' + ); + } + + public function testTargetStructureChangedIsTrueAfterAStructuralEdit(): void + { + $runner = new PhpstanRunner(); + $path = $this->writeTemp("\n */\nfinal class Probe extends Base\n{\n}\n"); + + $reflection = new ReflectionClass($runner); + $analysed = $reflection->getProperty('analysedFiles'); + $analysed->setAccessible(true); + $analysed->setValue($runner, [$path => $this->signatureOf($runner, $path)]); + + $method = $reflection->getMethod('targetStructureChanged'); + + self::assertFalse((bool) $method->invoke($runner, $path)); + + file_put_contents($path, "\n */\nfinal class Probe extends Base\n{\n}\n"); + + self::assertTrue( + (bool) $method->invoke($runner, $path), + 'Editing the generic after the worker reflected the class must force a respawn.' + ); + } + + public function testPhpstanBinaryOverrideIsHonoured(): void + { + $fake = $this->writeTemp("getMethod('findPhpstanBin'); + + self::assertSame(realpath($fake), $method->invoke($runner)); + } finally { + putenv('MCP_PHPSTAN_PHPSTAN_BIN'); + } + } + + public function testPhpstanBinaryOverrideRejectsAMissingPath(): void + { + putenv('MCP_PHPSTAN_PHPSTAN_BIN=' . sys_get_temp_dir() . '/definitely-not-here-' . bin2hex(random_bytes(4))); + + try { + $runner = new PhpstanRunner(); + $method = (new ReflectionClass($runner))->getMethod('findPhpstanBin'); + + $this->expectException(\RuntimeException::class); + $method->invoke($runner); + } finally { + putenv('MCP_PHPSTAN_PHPSTAN_BIN'); + } + } + + public function testUnknownClassErrorIsDetected(): void + { + $runner = new PhpstanRunner(); + $method = (new ReflectionClass($runner))->getMethod('hasUnknownClassError'); + + $unknown = [['file' => 'a.php', 'line' => 3, 'message' => 'extends unknown class Base.', 'identifier' => 'class.notFound']]; + $ordinary = [['file' => 'a.php', 'line' => 3, 'message' => 'should return string but returns int.', 'identifier' => 'return.type']]; + + self::assertTrue((bool) $method->invoke($runner, $unknown)); + self::assertFalse((bool) $method->invoke($runner, $ordinary)); + self::assertFalse((bool) $method->invoke($runner, [])); + } +}