diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f2d790..9bc92f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ This project follows [Semantic Versioning](https://semver.org/). ## [Unreleased] +## [0.4.0] — 2026-06-21 + +### Fixed + +- **Warm reflection state no longer leaks between files (claude-supertool#273).** The warm container reused one Rector `Application` across every call but reset nothing between them. Rector's `DynamicSourceLocatorProvider` caches its `AggregateSourceLocator` for every non-PHPUnit run, so the second (and every later) file was analysed with a source locator that only knew the *first* file — and Rector emitted `System error: "ClassReflection must be resolved for class X"` on test classes whose hierarchy the stale locator could not resolve. A fresh `rector` CLI process never hit this; the warm daemon did, and the failure was being content-hash-cached and replayed (2100 poisoned entries). `RectorRunner::run()` now resets every service tagged `ResettableInterface` before each warm call — the same flush `AbstractRectorTestCase` performs between fixtures — so the warm daemon matches a cold CLI boot. The expensive bootstrap stays warm; only per-run reflection state is flushed (~no measurable per-call cost). This makes the rector-mcp adapter's `System error:` suppression defense-in-depth rather than load-bearing. + +### Added + +- **`testWarmReflectionMatchesColdForSequence` regression (env-gated).** Drives a sequence of files through one warm server and asserts no `System error` / stale-reflection failure. Self-contained synthetic classes cannot reproduce #273 (in-process PHPUnit disables the locator cache via `isPHPUnitRun()`, and the trigger needs real framework base classes extended from outside the configured paths), so the test points at a real project via `MCP_RECTOR_WARM_REPRO_DIR` / `MCP_RECTOR_WARM_REPRO_FILES` / `MCP_RECTOR_WARM_REPRO_BIN` and skips otherwise. `tools/repro-273.py` discovers a triggering sequence. + ## [0.3.0] — 2026-05-23 ### Security diff --git a/src/RectorRunner.php b/src/RectorRunner.php index 059945a..1ce7bbe 100644 --- a/src/RectorRunner.php +++ b/src/RectorRunner.php @@ -14,6 +14,7 @@ final class RectorRunner { private ?object $application = null; + private ?object $container = null; private ?string $appClass = null; private ?string $inputClass = null; private ?string $outputClass = null; @@ -36,6 +37,16 @@ public function run(array $argv): array $warmBoot = $this->isWarm(); if (!$warmBoot) { $this->boot(); + } else { + // Warm reuse: flush per-run reflection state before analysing the next + // file. Rector's DynamicSourceLocatorProvider caches its + // AggregateSourceLocator for every non-PHPUnit run, so a second, different + // file gets analysed with a locator that only knows the first file and + // Rector emits 'System error: ClassReflection must be resolved for class X' + // (claude-supertool#273). A fresh CLI process never hits this; the warm + // daemon must reset the same services AbstractRectorTestCase resets between + // fixtures to behave identically to cold. + $this->resetReflectionState(); } $inputClass = $this->inputClass; @@ -118,6 +129,31 @@ private function boot(): void $app->setCatchExceptions(true); $this->application = $app; + $this->container = $container; + } + + /** + * Reset Rector's per-run reflection state between warm calls. Mirrors + * AbstractRectorTestCase::setUp(), which resets every service tagged + * ResettableInterface so each fixture analyses with a fresh source locator. + * The warm daemon reuses one container across files and needs the same flush; + * without it the cached AggregateSourceLocator from the previous file poisons + * the next one (claude-supertool#273). + */ + private function resetReflectionState(): void + { + $container = $this->container; + if ($container === null || !method_exists($container, 'tagged')) { + return; + } + + /** @var iterable $resettables */ + $resettables = $container->tagged(\Rector\Contract\DependencyInjection\ResettableInterface::class); + foreach ($resettables as $resettable) { + if (method_exists($resettable, 'reset')) { + $resettable->reset(); + } + } } /** diff --git a/tests/Integration/ServerStdioTest.php b/tests/Integration/ServerStdioTest.php index 9892d35..2bbfb88 100644 --- a/tests/Integration/ServerStdioTest.php +++ b/tests/Integration/ServerStdioTest.php @@ -196,6 +196,84 @@ public function testEditedSourceIsReprocessedAcrossCalls(): void } } + /** + * Regression for claude-supertool#273: the warm container must return the same + * result as a cold CLI for every file in a sequence — no spurious + * "System error: ClassReflection must be resolved for class X". + * + * Why this is env-gated and not a self-contained fixture: + * 1. The bug only manifests when Rector's DynamicSourceLocatorProvider caches + * its AggregateSourceLocator, which it does ONLY for non-PHPUnit runs + * (StaticPHPUnitEnvironment::isPHPUnitRun() === false). An in-process + * PHPUnit test therefore can never reproduce it — the cache is bypassed. + * The subprocess server is non-PHPUnit, which is why it triggers there. + * 2. The failing reflection needs real framework base classes (DVSI's + * SiTestCase / SiModuleTestCase) extended from files outside the configured + * paths. Trivial synthetic classes resolve cleanly and never trip it. + * + * Point this at a project that reproduces it (e.g. a DVSI checkout): + * MCP_RECTOR_WARM_REPRO_DIR=/path/to/project \ + * MCP_RECTOR_WARM_REPRO_FILES=relA.php,relB.php \ # >=2 files, B fails warm pre-fix + * MCP_RECTOR_WARM_REPRO_CONFIG=rector.php \ # optional, default rector.php + * vendor/bin/phpunit --filter testWarmReflectionMatchesColdForSequence + */ + public function testWarmReflectionMatchesColdForSequence(): void + { + $project = getenv('MCP_RECTOR_WARM_REPRO_DIR'); + $filesEnv = getenv('MCP_RECTOR_WARM_REPRO_FILES'); + if ($project === false || $filesEnv === false) { + self::markTestSkipped( + 'Set MCP_RECTOR_WARM_REPRO_DIR + MCP_RECTOR_WARM_REPRO_FILES (>=2 comma-separated ' + . 'paths relative to the dir) to run the #273 warm-reflection regression. ' + . 'See tools/repro-273.py to discover a triggering sequence.', + ); + } + + $project = realpath($project) ?: $project; + $config = getenv('MCP_RECTOR_WARM_REPRO_CONFIG') ?: 'rector.php'; + // The bundled bin only boots its own rector + autoload; point this at the + // project's installed bin (e.g. DVSI's libs/bin/mcp-rector-warm) when the + // config references project-specific custom rules. + $bin = getenv('MCP_RECTOR_WARM_REPRO_BIN') ?: self::$bin; + $files = array_values(array_filter(array_map('trim', explode(',', $filesEnv)))); + self::assertGreaterThanOrEqual(2, count($files), 'need at least two files to warm then re-use'); + + $proc = $this->spawnServer($project, $config, $bin); + + try { + $this->send($proc['stdin'], ['jsonrpc' => '2.0', 'id' => 1, 'method' => 'initialize', 'params' => [ + 'protocolVersion' => '2024-11-05', + 'capabilities' => new \stdClass(), + 'clientInfo' => ['name' => 'phpunit', 'version' => '1.0.0'], + ]]); + $this->send($proc['stdin'], ['jsonrpc' => '2.0', 'method' => 'notifications/initialized']); + + $id = 1; + foreach ($files as $rel) { + $id++; + $abs = $project . '/' . ltrim($rel, '/'); + $this->send($proc['stdin'], $this->processCall($id, $abs)); + $resp = $this->readResponse($proc['stdout'], $id); + $blob = (string) json_encode($resp['result']['structuredContent'] ?? []); + self::assertStringNotContainsString( + 'System error', + $blob, + "warm container emitted a System error on '{$rel}' (#273)" . $this->stderrTail($proc['stderr']), + ); + self::assertStringNotContainsString( + 'must be resolved', + $blob, + "warm container served a stale reflection locator on '{$rel}' (#273)" . $this->stderrTail($proc['stderr']), + ); + } + } finally { + fclose($proc['stdin']); + stream_get_contents($proc['stdout']); + fclose($proc['stdout']); + proc_close($proc['handle']); + } + } + /** * @param array $response */ @@ -210,14 +288,17 @@ private function changedFiles(array $response): int /** * @return array{handle: resource, stdin: resource, stdout: resource, stderr: string} */ - private function spawnServer(string $project): array + private function spawnServer(string $project, string $config = 'rector.php', ?string $bin = null): array { // Capture stderr to a file (not /dev/null) so a CI failure has diagnostics. $stderr = $project . '/server.stderr'; + // Absolute config path passes through unchanged; a relative one resolves + // against the project (working) dir, matching the production daemon. + $configPath = str_starts_with($config, '/') ? $config : $project . '/' . $config; $cmd = [ - self::$bin, + $bin ?? self::$bin, '--working-dir=' . $project, - '--config=' . $project . '/rector.php', + '--config=' . $configPath, ]; $proc = proc_open( $cmd, diff --git a/tools/repro-273.py b/tools/repro-273.py new file mode 100755 index 0000000..6f78e1e --- /dev/null +++ b/tools/repro-273.py @@ -0,0 +1,148 @@ +#!/usr/bin/env python3 +""" +Deterministic-repro harness for claude-supertool #273. + +Spawns ONE warm mcp-rector-warm server and feeds it a long sequence of real +project files over a single connection, hunting for the first `System error:` +— the warm-only ClassReflection failure (a stale AggregateSourceLocator from an +earlier file poisoning a later one). On a hit it cold-checks the same file +(fresh server, single call) to prove the failure is warm-only, not file-local. + +The trigger needs real framework base classes (e.g. a test-case base) extended +from files outside the configured Rector paths; trivial synthetic classes +resolve cleanly. Point at such a project (e.g. a DVSI checkout) +and feed its *Test.php files. + +Usage: repro-273.py [start] +File list on stdin (one path per line, relative to working_dir). +""" +import json +import os +import subprocess +import sys +import time + + +def spawn(bin_path, working_dir, config): + proc = subprocess.Popen( + [bin_path, "--working-dir=" + working_dir, "--config=" + config], + cwd=working_dir, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + bufsize=0, + ) + return proc + + +def send(proc, msg): + proc.stdin.write((json.dumps(msg) + "\n").encode()) + proc.stdin.flush() + + +def read_id(proc, want_id, timeout=120): + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + line = proc.stdout.readline() + if not line: + return None + line = line.strip() + if not line or line[:1] != b"{": + continue + try: + d = json.loads(line) + except Exception: + continue + if d.get("id") == want_id: + return d + return None + + +def init(proc): + send(proc, {"jsonrpc": "2.0", "id": 1, "method": "initialize", "params": { + "protocolVersion": "2024-11-05", "capabilities": {}, + "clientInfo": {"name": "repro", "version": "1.0.0"}}}) + send(proc, {"jsonrpc": "2.0", "method": "notifications/initialized"}) + read_id(proc, 1) + + +def call(proc, mid, abs_path): + send(proc, {"jsonrpc": "2.0", "id": mid, "method": "tools/call", "params": { + "name": "rector_process", "arguments": {"path": abs_path, "dryRun": True}}}) + return read_id(proc, mid) + + +def structured(resp): + if not resp: + return {} + return (resp.get("result", {}) or {}).get("structuredContent", {}) or {} + + +def has_system_error(sc): + blob = json.dumps(sc) + return "System error" in blob or "must be resolved" in blob + + +def main(): + bin_path, working_dir, config, max_files = sys.argv[1:5] + start = int(sys.argv[5]) if len(sys.argv) > 5 else 0 + max_files = int(max_files) + files = [l.strip() for l in sys.stdin if l.strip()][start:start + max_files] + + proc = spawn(bin_path, working_dir, config) + init(proc) + print(f"warm server up, feeding {len(files)} files (start={start})", flush=True) + + hit = None + mid = 1 + for i, rel in enumerate(files): + mid += 1 + ap = os.path.join(working_dir, rel) + if not os.path.isfile(ap): + continue + resp = call(proc, mid, ap) + sc = structured(resp) + if resp is None: + print(f"[{i}] NO RESPONSE on {rel} (server died?)", flush=True) + print((proc.stderr.read() or b"")[-2000:].decode(errors="replace"), flush=True) + break + if has_system_error(sc): + hit = (i, rel, sc) + print(f"\n*** HIT at #{i} (warm_boot={sc.get('warm_boot')}): {rel}", flush=True) + print(json.dumps(sc, indent=2)[:3000], flush=True) + break + if i % 50 == 0: + print(f"[{i}] ok warm={sc.get('warm_boot')} exit={sc.get('exit_code')} {rel}", flush=True) + try: + proc.stdin.close() + proc.terminate() + except Exception: + pass + + if not hit: + print("\nNo System error across the fed sequence.", flush=True) + return 0 + + # Cold-confirm: fresh server, single call on the culprit file. + i, rel, _ = hit + ap = os.path.join(working_dir, rel) + cold = spawn(bin_path, working_dir, config) + init(cold) + csc = structured(call(cold, 2, ap)) + try: + cold.stdin.close() + cold.terminate() + except Exception: + pass + print(f"\n=== COLD check on culprit {rel} ===", flush=True) + print(f"cold warm_boot={csc.get('warm_boot')} exit={csc.get('exit_code')} " + f"system_error={has_system_error(csc)}", flush=True) + if not has_system_error(csc): + print(">>> CONFIRMED WARM-ONLY: fails warm, clean cold. Deterministic repro found.", flush=True) + else: + print(">>> Fails cold too — not warm-specific; keep looking.", flush=True) + return 0 + + +if __name__ == "__main__": + sys.exit(main())