From d9e253b2e14c30a726d3371e781ddf7f6522ec9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Say=C3=A3o=20Lobato=20Abreu?= Date: Mon, 26 Oct 2026 20:11:53 -0300 Subject: [PATCH 1/7] feat: add explicit native Rector preview and apply command --- .changelog/explicit-rector-command.md | 9 ++ AGENTS.md | 1 + README.md | 7 +- composer.json | 2 + composer.lock | 115 ++++++++++++++- docs/architecture.md | 6 +- docs/instruction-map.md | 4 + docs/policy.md | 4 + docs/rector.md | 87 +++++++++++ .../fast-forward-code-standards/SKILL.md | 6 +- src/Console/Application.php | 6 +- src/Console/Command/RefactorCommand.php | 59 ++++++++ src/Process/NativeProcessRunner.php | 13 +- src/Refactoring/RectorRunner.php | 56 +++++++ .../StandardsServiceProvider.php | 6 + tests/Integration/RectorTest.php | 138 ++++++++++++++++++ tests/Refactoring/RectorRunnerTest.php | 80 ++++++++++ tests/Refactoring/RefactorCommandTest.php | 62 ++++++++ tests/ServiceProvider/CompositionTest.php | 3 + 19 files changed, 649 insertions(+), 15 deletions(-) create mode 100644 .changelog/explicit-rector-command.md create mode 100644 docs/rector.md create mode 100644 src/Console/Command/RefactorCommand.php create mode 100644 src/Refactoring/RectorRunner.php create mode 100644 tests/Integration/RectorTest.php create mode 100644 tests/Refactoring/RectorRunnerTest.php create mode 100644 tests/Refactoring/RefactorCommandTest.php diff --git a/.changelog/explicit-rector-command.md b/.changelog/explicit-rector-command.md new file mode 100644 index 0000000..3bcd2ca --- /dev/null +++ b/.changelog/explicit-rector-command.md @@ -0,0 +1,9 @@ +--- +category: added +type: minor +--- + +Add a separate opt-in native Rector command with dry-run by default, explicit +configuration and apply authority, empty-rule initialization protection and +post-apply verification. Keep target PHP, rules and caches consumer-owned; +formatting check/fix never invoke semantic refactoring. diff --git a/AGENTS.md b/AGENTS.md index d7ab52c..96d1bc8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,6 +66,7 @@ for the exact local base/head before publication; do not claim remote CI evidenc - [README.md](README.md): library/CLI adoption, explicit fix and tool scope. - [Policy comparison](docs/policy.md): evidence, rule differences and extraction gates. +- [Explicit Rector](docs/rector.md): opt-in native semantic preview/application; executable configuration owns target PHP, rules, source paths and caches. - [Resolved preset](docs/resolved-preset.md): compiled rules, golden contracts and dependency-update review. - [Legacy fixer comparison](docs/legacy-fixer-comparison.md): fixed-source inventory, loaded dependencies and remaining profile decisions. - [PHPDoc comparison](docs/phpdoc-comparison.md): actual custom-rule registration, source contracts and remaining golden decisions. diff --git a/README.md b/README.md index f690a8d..edc6958 100644 --- a/README.md +++ b/README.md @@ -104,5 +104,8 @@ See [policy and differences](docs/policy.md), [workflow/ref adoption](docs/autom and the [consumer skill](resources/skills/fast-forward-code-standards/SKILL.md). The [architecture decision](docs/architecture.md) defines library/tool dependency directions and CPX/autoload limits. -Rector/custom PHPDoc transformations and deeper static analysis are separate, -unimplemented increments; this package does not assert extraction parity yet. +The separate opt-in [`refactor` command](docs/rector.md) previews an explicit +native Rector configuration and requires `--apply` for semantic writes. It is +never invoked by formatting `check` or `fix`; target PHP and cache remain native +consumer configuration decisions. Custom PHPDoc transformations and deeper +static analysis remain separate; no DevTools extraction parity is asserted. diff --git a/composer.json b/composer.json index 967ad8d..3b9e1b8 100644 --- a/composer.json +++ b/composer.json @@ -41,7 +41,9 @@ "composer-runtime-api": "^2.2", "container-interop/service-provider": "^0.4.1", "fast-forward/container": "~1.10.1", + "phpstan/phpstan": "~2.1.51", "psr/container": "^1.1 || ^2.0", + "rector/rector": "~2.4.2", "symfony/console": "^7.4 || ^8.0", "symfony/process": "^7.4 || ^8.0", "symplify/easy-coding-standard": "^13.0" diff --git a/composer.lock b/composer.lock index e181612..a173f6c 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "d4501df051e73a0498012c3f268844ba", + "content-hash": "66bba742dc3bb603e0387d2083fd882c", "packages": [ { "name": "brick/varexporter", @@ -420,6 +420,59 @@ }, "time": "2026-09-13T18:51:52+00:00" }, + { + "name": "phpstan/phpstan", + "version": "2.1.51", + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/phpstan/phpstan/zipball/dc3b523c45e714c70de2ac5113b958223b55dc59", + "reference": "dc3b523c45e714c70de2ac5113b958223b55dc59", + "shasum": "" + }, + "require": { + "php": "^7.4|^8.0" + }, + "conflict": { + "phpstan/phpstan-shim": "*" + }, + "bin": [ + "phpstan", + "phpstan.phar" + ], + "type": "library", + "autoload": { + "files": [ + "bootstrap.php" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "description": "PHPStan - PHP Static Analysis Tool", + "keywords": [ + "dev", + "static analysis" + ], + "support": { + "docs": "https://phpstan.org/user-guide/getting-started", + "forum": "https://github.com/phpstan/phpstan/discussions", + "issues": "https://github.com/phpstan/phpstan/issues", + "security": "https://github.com/phpstan/phpstan/security/policy", + "source": "https://github.com/phpstan/phpstan-src" + }, + "funding": [ + { + "url": "https://github.com/ondrejmirtes", + "type": "github" + }, + { + "url": "https://github.com/phpstan", + "type": "github" + } + ], + "time": "2026-04-21T18:22:01+00:00" + }, { "name": "psr/container", "version": "2.0.2", @@ -524,6 +577,66 @@ }, "time": "2021-10-29T13:26:27+00:00" }, + { + "name": "rector/rector", + "version": "2.4.2", + "source": { + "type": "git", + "url": "https://github.com/rectorphp/rector.git", + "reference": "e645b6463c6a88ea5b44b17d3387d35a912c7946" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/rectorphp/rector/zipball/e645b6463c6a88ea5b44b17d3387d35a912c7946", + "reference": "e645b6463c6a88ea5b44b17d3387d35a912c7946", + "shasum": "" + }, + "require": { + "php": "^7.4|^8.0", + "phpstan/phpstan": "^2.1.48" + }, + "conflict": { + "rector/rector-doctrine": "*", + "rector/rector-downgrade-php": "*", + "rector/rector-phpunit": "*", + "rector/rector-symfony": "*" + }, + "suggest": { + "ext-dom": "To manipulate phpunit.xml via the custom-rule command" + }, + "bin": [ + "bin/rector" + ], + "type": "library", + "autoload": { + "files": [ + "bootstrap.php" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "description": "Instant Upgrade and Automated Refactoring of any PHP code", + "homepage": "https://getrector.com/", + "keywords": [ + "automation", + "dev", + "migration", + "refactoring" + ], + "support": { + "issues": "https://github.com/rectorphp/rector/issues", + "source": "https://github.com/rectorphp/rector/tree/2.4.2" + }, + "funding": [ + { + "url": "https://github.com/tomasvotruba", + "type": "github" + } + ], + "time": "2026-04-16T13:07:34+00:00" + }, { "name": "symfony/console", "version": "v7.4.21", diff --git a/docs/architecture.md b/docs/architecture.md index 9b9094d..98789eb 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -33,8 +33,10 @@ explicit compatibility tests or a real process boundary. A separate Composer graph alone must never be described as runtime isolation. Analyzer/autoload fixtures must prove that the code under analysis comes from the consumer checkout, rather than another copy installed inside the tool. -This initial syntax parser executes no application autoload; future PHPUnit, -Rector and type-analysis adapters must add those compatibility tests. +This initial syntax parser executes no application autoload; future PHPUnit and +type-analysis adapters must add those compatibility tests. The separate Rector +command's executable configuration and child-autoload limits are documented in +[the native refactoring contract](rector.md). ## Bootstrap and compatibility diff --git a/docs/instruction-map.md b/docs/instruction-map.md index 096a778..db73a9e 100644 --- a/docs/instruction-map.md +++ b/docs/instruction-map.md @@ -10,5 +10,9 @@ consumer procedure. It is not copied to `.agents/skills` and needs no host alias Read root instructions, then only the linked domain guide or skill needed by the task. The root Child DOX Index provides the reading route. +The opt-in refactoring guide remains under this same root authority. It records +the native Rector invocation and executable-config boundary; it introduces no +new instruction subtree, automatic semantic profile or consumer installer. + No previous instruction tree is migrated. Rollback restores the root contract, its index and this map together through Git; it changes no host configuration. diff --git a/docs/policy.md b/docs/policy.md index 4e866c4..08a6f15 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -57,6 +57,10 @@ Static quality in this first increment means parseable PHP and formatting conformance. Type analysis, dead-code analysis and semantic refactors are not claimed. Application tests, coverage and complete reports remain outside this domain. +The separately opted-in [native Rector command](rector.md) is a future minor +increment. It selects no legacy semantic profile and is never invoked by +formatting check/fix; the consumer explicitly owns rules, target PHP and caches. + The [PHPDoc comparison](phpdoc-comparison.md) records actual custom Rector activation and the fixtures required before adopting a documentation policy. The [static-analysis evaluation](static-analysis-evaluation.md) distinguishes diff --git a/docs/rector.md b/docs/rector.md new file mode 100644 index 0000000..87d9187 --- /dev/null +++ b/docs/rector.md @@ -0,0 +1,87 @@ +# Explicit semantic refactoring + +This opt-in increment targets a future minor release. It does not change the +0.1 syntax/style contract, select a migration profile, or authorize publication. +`check` and `fix` never invoke the semantic runner. The separate invocable +`refactor` command uses the existing container factories and process transport. + +The qualified lock selects published Rector 2.4.2, reference +`e645b6463c6a88ea5b44b17d3387d35a912c7946`, with PHPStan 2.1.51. The manifest +restricts both dependencies to their respective 2.4/2.1 patch lines. Rector's +upstream PHPStan constraint also permits 2.3.2, but that combination reproduced +a fatal missing `RichParser::$container` error before rule execution. This +compatibility restriction applies to the shared Composer graph; a consumer +requiring a newer PHPStan must use a separate compatible tool installation. +PHPStan is an engine dependency here, not a newly exposed analysis command. +Dependency updates require the real integration fixtures below. Native configuration is the only +extension point; no PHP parser, configuration translator or engine framework is +introduced. Configuration paths resolve from `--directory` or the current +directory. This command deliberately uses the configuration's whole-project +paths rather than borrowing the formatter's changed-file list. + +```php +withPhpVersion(PhpVersion::PHP_74) + ->withPaths([__DIR__ . '/src']) + ->withRules([LongArrayToShortArrayRector::class]) + ->withCache(sys_get_temp_dir() . '/my-project-rector/' . hash('sha256', __DIR__)) + ->withoutParallel(); +``` + +The target is a consumer decision, independent of the host PHP. `withPhpVersion` +limits supported transformations; it does not downgrade existing code or select +PHP migration sets. Review any later `withPhpSets` selection separately. The +command does not infer or overwrite target, rule, cache, bootstrap, parallelism +or path choices. Configure generated caches separately from owned source, with +a directory unique to the checkout. No consumer dependency/lock update occurs. + +```sh +php bin/code-standards refactor --config=rector.php --directory=/project +# After reviewing the proposed semantic diff: +php bin/code-standards refactor --config=rector.php --directory=/project --apply +``` + +Preview passes native `--dry-run`. Explicit apply performs one write pass and +then a dry-run verification; remaining differences fail. Native exits are 0 for +clean/success, 1 for processing errors and 2 for proposed changes. Controlled +invocation, empty-rule and invalid-inventory errors also return 2, with an error +diagnostic. Other native fatal exits propagate unchanged. Rector 2.4.2 uses `-n` for dry-run and does not expose the ECS +`--no-interaction` option; its native flags are not translated blindly. + +## Executable configuration and initialization boundary + +The command first asks native `list-rules --output-format=json` for the selected +configuration's rules. An empty or invalid inventory is rejected without calling +`process`: native Rector otherwise can generate `rector.php` and return success +even during dry-run. Missing explicit configuration is rejected before launch. + +This is a contract for **trusted, deterministic configuration**. Preview loads +it twice; successful apply and verification load it three times. A configuration +that changes its rules between invocations can defeat that preflight. Arbitrary +configuration, Composer autoload files, custom rules and configured bootstraps +can execute and mutate files even in dry-run. The command is not a filesystem or +security sandbox, and cannot promise read-only behavior for such executable +extensions. Do not use them with privileged events, secrets or an untrusted +consumer. Changes to the native initialization contract require regression +review before changing the pinned Rector version. + +The subprocess prevents Rector from sharing the parent CLI's in-memory state. +It does not guarantee namespace isolation in the child: Rector loads its +installation autoloader and may load the consumer's `vendor/autoload.php` too. +The collision regression requires failure rather than a false clean result; +configuration/bootstrap compatibility remains a consumer qualification gate. +Captured preflight and streamed processing use the same credential stripping. +No generic promise of semantic safety follows from a zero result. + +The integration fixtures cover no-write preview, explicit apply/verification, empty custom +configuration, a PHP 7.4 target under a newer host, configured cache ownership, +native bootstrap failures and consumer/tool autoload collisions. Run the +consumer's affected behavioral tests and review API/side-effect changes before +accepting any semantic patch. Type analysis and application testing are not +implicitly added to this command. diff --git a/resources/skills/fast-forward-code-standards/SKILL.md b/resources/skills/fast-forward-code-standards/SKILL.md index 8255eb1..039c127 100644 --- a/resources/skills/fast-forward-code-standards/SKILL.md +++ b/resources/skills/fast-forward-code-standards/SKILL.md @@ -24,7 +24,11 @@ metadata: privileged token or on a privileged target event. No incidental dependency update. 5. Only when fixing is requested run `code-standards fix --all`, inspect the diff and rerun check. Do not commit/push automatically. Semantic migrations need a - separate reviewed procedure; none are enabled by this initial package. + separate reviewed procedure: the opt-in `refactor` command requires explicit + native Rector configuration, consumer target PHP/cache selection and reviewed + rules. Preview is default; only explicit `--apply` grants semantic writes. + Read `docs/rector.md` for executable-config/autoload limits. Never substitute + this command for ordinary formatting `fix` or automatically migrate consumers. 6. For requested CI adoption copy `resources/workflows/code-standards.yml` from the selected ref after v0 points to a confirmed 0.x release, and use `pull_request` with read-only permissions and a hosted runner. No secrets/inherit, write token, diff --git a/src/Console/Application.php b/src/Console/Application.php index 031fef0..0bdbbe4 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -6,6 +6,7 @@ use FastForward\CodeStandards\Console\Command\CheckCommand; use FastForward\CodeStandards\Console\Command\FixCommand; +use FastForward\CodeStandards\Console\Command\RefactorCommand; use Symfony\Component\Console\Application as SymfonyApplication; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Exception\ExceptionInterface; @@ -16,13 +17,14 @@ /** Native Symfony application; standard binding errors retain the CLI's invalid-input status. */ final class Application extends SymfonyApplication { - /** Registers the two explicitly composed operations without resolving project state or executing tools. */ - public function __construct(CheckCommand $check, FixCommand $fix) + /** Registers independent operations without resolving project state or executing tools. */ + public function __construct(CheckCommand $check, FixCommand $fix, RefactorCommand $refactor) { parent::__construct('Fast Forward Code Standards'); $this->addCommand($check); $this->addCommand($fix); + $this->addCommand($refactor); $this->setAutoExit(false); } diff --git a/src/Console/Command/RefactorCommand.php b/src/Console/Command/RefactorCommand.php new file mode 100644 index 0000000..03c3f5b --- /dev/null +++ b/src/Console/Command/RefactorCommand.php @@ -0,0 +1,59 @@ +projects->resolve(true, null, null, $directory, $config); + + return $this->rector->run($scope->directory, $scope->config, $apply); + } catch (ProcessFailedException $error) { + $io->getErrorStyle()->error($error->getMessage()); + + return $error->getProcess()->getExitCode() ?? Command::FAILURE; + } catch (InvalidArgumentException|RuntimeException $error) { + $io->getErrorStyle()->error($error->getMessage()); + + return Command::INVALID; + } + } +} diff --git a/src/Process/NativeProcessRunner.php b/src/Process/NativeProcessRunner.php index 2090c9c..140fa5c 100644 --- a/src/Process/NativeProcessRunner.php +++ b/src/Process/NativeProcessRunner.php @@ -9,15 +9,18 @@ /** Native transport; callers own selection and operation policy. */ final class NativeProcessRunner implements ProcessRunner { + /** Both captured and streamed configuration subprocesses receive the same credential policy. */ + private const array ENVIRONMENT = ['GH_TOKEN' => false, 'GITHUB_TOKEN' => false, 'COMPOSER_AUTH' => false]; + /** @param \Closure(string,string):void|null $write Explicit output sink; null keeps this library transport silent. */ public function __construct(private readonly ?\Closure $write = null) { } - /** Captures Git output as bytes, including NUL-delimited paths. */ + /** Captures native output as bytes, including Git paths and Rector's JSON inventory. */ public function capture(array $arguments, string $directory): string { - $process = new Process($arguments, $directory, timeout: 120); + $process = new Process($arguments, $directory, self::ENVIRONMENT, timeout: 120); $process->mustRun(); return $process->getOutput(); @@ -26,11 +29,7 @@ public function capture(array $arguments, string $directory): string /** Runs one tool, streaming its output without changing its failure status. */ public function run(array $arguments, string $directory): int { - $process = new Process($arguments, $directory, [ - 'GH_TOKEN' => false, - 'GITHUB_TOKEN' => false, - 'COMPOSER_AUTH' => false, - ], timeout: 300); + $process = new Process($arguments, $directory, self::ENVIRONMENT, timeout: 300); return $process->run($this->write); } diff --git a/src/Refactoring/RectorRunner.php b/src/Refactoring/RectorRunner.php new file mode 100644 index 0000000..7ed6f82 --- /dev/null +++ b/src/Refactoring/RectorRunner.php @@ -0,0 +1,56 @@ +assertRules($directory, $config); + $check = [PHP_BINARY, $this->rector, 'process', '--config=' . $config, '--no-progress-bar']; + $status = $this->process->run($apply ? $check : [...$check, '--dry-run'], $directory); + if (!$apply || 0 !== $status) { + return $status; + } + + return $this->process->run([...$check, '--dry-run'], $directory); + } + + /** Uses Rector's own JSON inventory instead of evaluating or parsing consumer PHP in the parent process. */ + private function assertRules(string $directory, string $config): void + { + $output = $this->process->capture([ + PHP_BINARY, $this->rector, 'list-rules', '--config=' . $config, '--output-format=json', + ], $directory); + try { + $inventory = json_decode($output, true, flags: JSON_THROW_ON_ERROR); + } catch (JsonException $error) { + throw new InvalidArgumentException('Rector did not return a valid native rule inventory.', previous: $error); + } + $rules = is_array($inventory) ? ($inventory['rectors'] ?? null) : null; + if (!is_array($rules) || [] === $rules || !array_is_list($rules) + || count(array_filter($rules, static fn (mixed $rule): bool => is_string($rule) && '' !== $rule)) !== count($rules)) { + throw new InvalidArgumentException('Select a native Rector configuration with at least one rule; automatic initialization is disabled.'); + } + } +} diff --git a/src/ServiceProvider/StandardsServiceProvider.php b/src/ServiceProvider/StandardsServiceProvider.php index c75cbea..ddd44a4 100644 --- a/src/ServiceProvider/StandardsServiceProvider.php +++ b/src/ServiceProvider/StandardsServiceProvider.php @@ -7,6 +7,7 @@ use FastForward\CodeStandards\Process\NativeProcessRunner; use FastForward\CodeStandards\Process\ProcessRunner; use FastForward\CodeStandards\Project\ProjectScopeResolver; +use FastForward\CodeStandards\Refactoring\RectorRunner; use FastForward\CodeStandards\Standards\StandardsRunner; use FastForward\Container\Factory\CallableFactory; use FastForward\Container\Factory\FactoryInterface; @@ -48,6 +49,11 @@ public function getFactories(): array ProcessRunner::class, $this->vendorDirectory . '/symplify/easy-coding-standard/bin/ecs', ), + RectorRunner::class => new InvokableFactory( + RectorRunner::class, + ProcessRunner::class, + $this->vendorDirectory . '/rector/rector/bin/rector', + ), ]; } diff --git a/tests/Integration/RectorTest.php b/tests/Integration/RectorTest.php new file mode 100644 index 0000000..e062af1 --- /dev/null +++ b/tests/Integration/RectorTest.php @@ -0,0 +1,138 @@ +directory = realpath(sys_get_temp_dir()) . '/rector-consumer-' . bin2hex(random_bytes(8)); + mkdir($this->directory); + file_put_contents($this->directory . '/composer.lock', '{"fixture":"do not update"}'); + } + + protected function tearDown(): void + { + $files = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->directory, \FilesystemIterator::SKIP_DOTS), \RecursiveIteratorIterator::CHILD_FIRST); + foreach ($files as $file) { + $file->isDir() && !$file->isLink() ? rmdir($file->getPathname()) : unlink($file->getPathname()); + } + rmdir($this->directory); + } + + /** Native preview leaves source/config/lock bytes intact; explicit apply is verified and repeatable. */ + public function testPreviewApplyAndSecondPassWithExplicitNativeConfiguration(): void + { + $source = "directory . '/source.php', $source); + $config = $this->configuration('\\Rector\\Php54\\Rector\\Array_\\LongArrayToShortArrayRector', 70400); + $before = $this->ownedBytes(); + + $preview = $this->cli(); + self::assertSame(2, $preview->getExitCode(), $preview->getOutput() . $preview->getErrorOutput()); + self::assertSame($before, $this->ownedBytes()); + self::assertDirectoryExists($this->directory . '/.cache'); + self::assertSame(0, $this->cli(['--apply'])->getExitCode()); + self::assertStringContainsString('[1, 2]', file_get_contents($this->directory . '/source.php')); + $after = $this->ownedBytes(); + self::assertSame(0, $this->cli()->getExitCode()); + self::assertSame(0, $this->cli(['--apply'])->getExitCode()); + self::assertSame($after, $this->ownedBytes()); + self::assertSame($config, file_get_contents($this->directory . '/policy.php')); + } + + /** Empty native custom config is rejected before Rector's successful-but-writing initializer. */ + public function testEmptyConfigDoesNotGenerateRectorFile(): void + { + file_put_contents($this->directory . '/source.php', 'directory . '/policy.php', 'ownedBytes(); + $result = $this->cli(); + + self::assertSame(2, $result->getExitCode()); + self::assertStringContainsString('at least one rule', $result->getErrorOutput()); + self::assertFileDoesNotExist($this->directory . '/rector.php'); + self::assertSame($before, $this->ownedBytes()); + } + + /** A target below the executing PHP suppresses a versioned transformation without selecting migration sets. */ + public function testConsumerTargetControlsVersionedRule(): void + { + $source = 'name = $name; } }'; + file_put_contents($this->directory . '/source.php', $source); + $rule = '\\Rector\\Php80\\Rector\\Class_\\ClassPropertyAssignToConstructorPromotionRector'; + $this->configuration($rule, 70400); + self::assertSame(0, $this->cli()->getExitCode()); + $this->configuration($rule, 80000); + self::assertSame(2, $this->cli()->getExitCode()); + self::assertSame($source, file_get_contents($this->directory . '/source.php')); + } + + /** The subprocess boundary protects the parent, but a consumer autoload collision still fails in the child. */ + public function testConsumerAutoloadCollisionIsNotClaimedToBeIsolated(): void + { + file_put_contents($this->directory . '/source.php', 'configuration('\\Rector\\Php54\\Rector\\Array_\\LongArrayToShortArrayRector', 70400); + mkdir($this->directory . '/vendor'); + file_put_contents($this->directory . '/vendor/autoload.php', 'ownedBytes(); + $result = $this->cli(); + + self::assertNotSame(0, $result->getExitCode()); + self::assertSame($before, $this->ownedBytes()); + unlink($this->directory . '/vendor/autoload.php'); + self::assertSame(2, $this->cli()->getExitCode()); + } + + /** Native bootstrap failures preserve their status instead of being mistaken for no changes. */ + public function testExecutableConfigFailureAndRequiredOption(): void + { + file_put_contents($this->directory . '/policy.php', 'cli(); + self::assertSame(1, $result->getExitCode()); + self::assertStringContainsString('native config failure', $result->getErrorOutput()); + $missing = new Process([PHP_BINARY, dirname(__DIR__, 2) . '/bin/code-standards', 'refactor'], $this->directory, timeout: 20); + self::assertSame(2, $missing->run()); + } + + /** Returns native builder PHP unchanged; target and cache remain consumer-owned options. */ + private function configuration(string $rule, int $target): string + { + $config = 'withPhpVersion(' . $target . ')' + . '->withoutParallel()->withCache(__DIR__."/.cache")->withPaths([__DIR__."/source.php"])' + . '->withRules([' . $rule . '::class]);'; + file_put_contents($this->directory . '/policy.php', $config); + + return $config; + } + + /** @return array Bytes owned by the consumer; configured generated cache is deliberately separate. */ + private function ownedBytes(): array + { + $bytes = []; + foreach (['source.php', 'policy.php', 'composer.lock'] as $name) { + if (is_file($this->directory . '/' . $name)) { + $bytes[$name] = file_get_contents($this->directory . '/' . $name); + } + } + + return $bytes; + } + + /** @param list $options Additional explicit command options, never shell fragments. */ + private function cli(array $options = []): Process + { + $process = new Process([PHP_BINARY, dirname(__DIR__, 2) . '/bin/code-standards', 'refactor', '--config=policy.php', ...$options], $this->directory, timeout: 90); + $process->run(); + + return $process; + } +} diff --git a/tests/Refactoring/RectorRunnerTest.php b/tests/Refactoring/RectorRunnerTest.php new file mode 100644 index 0000000..6bfb602 --- /dev/null +++ b/tests/Refactoring/RectorRunnerTest.php @@ -0,0 +1,80 @@ +run('/consumer', '/consumer/my policy.php', false)); + self::assertCount(1, $process->commands); + self::assertContains('--dry-run', $process->commands[0]); + self::assertContains('--config=/consumer/my policy.php', $process->commands[0]); + self::assertContains('list-rules', $process->captures[0]); + self::assertNotContains('--no-interaction', $process->commands[0]); + } + + /** Apply is a single write pass followed by a read-only verification whose failure remains observable. */ + public function testApplyVerifiesAndPropagatesResidualChanges(): void + { + $process = new RectorTransport(statuses: [0, 2]); + self::assertSame(2, (new RectorRunner($process, '/tool/rector'))->run('/consumer', '/consumer/policy.php', true)); + self::assertCount(2, $process->commands); + self::assertNotContains('--dry-run', $process->commands[0]); + self::assertContains('--dry-run', $process->commands[1]); + + $failed = new RectorTransport(statuses: [1]); + self::assertSame(1, (new RectorRunner($failed, '/tool/rector'))->run('/consumer', '/consumer/policy.php', true)); + self::assertCount(1, $failed->commands); + } + + /** Empty or malformed inventory must prevent process from auto-generating rector.php. */ + public function testEmptyOrMalformedRulesNeverReachProcess(): void + { + foreach (['{"rectors":[]}', '{}', 'not JSON', '{"rectors":[1]}'] as $inventory) { + $process = new RectorTransport($inventory); + try { + (new RectorRunner($process, '/tool/rector'))->run('/consumer', '/consumer/policy.php', false); + self::fail('Invalid rule inventory must fail closed.'); + } catch (InvalidArgumentException) { + self::assertSame([], $process->commands); + } + } + } +} + +/** Records only the execution boundary; no native configuration or source is evaluated here. */ +final class RectorTransport implements ProcessRunner +{ + public array $commands = []; + public array $captures = []; + + /** @param list $statuses Native process/verification outcomes in execution order. */ + public function __construct(private string $inventory = '{"rectors":["ReviewedRule"]}', private array $statuses = [0]) + { + } + + public function capture(array $arguments, string $directory): string + { + $this->captures[] = $arguments; + + return $this->inventory; + } + + public function run(array $arguments, string $directory): int + { + $this->commands[] = $arguments; + + return array_shift($this->statuses); + } +} diff --git a/tests/Refactoring/RefactorCommandTest.php b/tests/Refactoring/RefactorCommandTest.php new file mode 100644 index 0000000..e5fdb43 --- /dev/null +++ b/tests/Refactoring/RefactorCommandTest.php @@ -0,0 +1,62 @@ +createMock(ProcessRunner::class); + $process->expects(self::once())->method('capture')->willReturn('{"rectors":["ReviewedRule"]}'); + $process->expects(self::once())->method('run')->with(self::callback(static fn (array $arguments): bool => in_array('--dry-run', $arguments, true)), $directory)->willReturn(2); + $application = new Application(); + $application->setAutoExit(false); + $application->addCommand(new RefactorCommand(new ProjectScopeResolver('/unused-default'), new RectorRunner($process, '/tool/rector'))); + $tester = new ApplicationTester($application); + + try { + self::assertSame(2, $tester->run(['command' => 'refactor'])); + self::assertStringContainsString('explicit native Rector configuration', $tester->getDisplay()); + self::assertSame(2, $tester->run(['command' => 'refactor', '--config' => $config, '--directory' => $directory])); + self::assertStringNotContainsString('ERROR', $tester->getDisplay()); + } finally { + unlink($config); + } + } + + /** Captured preflight configuration receives the same credential stripping as streamed execution. */ + public function testCapturedConfigurationDoesNotReceiveToolCredentials(): void + { + $previous = []; + foreach (['GH_TOKEN', 'GITHUB_TOKEN', 'COMPOSER_AUTH'] as $name) { + $previous[$name] = getenv($name); + putenv($name . '=synthetic-test-value'); + } + + try { + $output = (new NativeProcessRunner())->capture([ + PHP_BINARY, '-r', 'foreach (["GH_TOKEN", "GITHUB_TOKEN", "COMPOSER_AUTH"] as $name) { echo getenv($name) === false ? "absent\n" : "present\n"; }', + ], sys_get_temp_dir()); + self::assertSame("absent\nabsent\nabsent\n", $output); + } finally { + foreach ($previous as $name => $value) { + putenv(false === $value ? $name : $name . '=' . $value); + } + } + } +} diff --git a/tests/ServiceProvider/CompositionTest.php b/tests/ServiceProvider/CompositionTest.php index 93ffd8a..e66044c 100644 --- a/tests/ServiceProvider/CompositionTest.php +++ b/tests/ServiceProvider/CompositionTest.php @@ -7,6 +7,7 @@ use FastForward\CodeStandards\Console\Application; use FastForward\CodeStandards\Console\Command\CheckCommand; use FastForward\CodeStandards\Console\Command\FixCommand; +use FastForward\CodeStandards\Console\Command\RefactorCommand; use FastForward\CodeStandards\Process\NativeProcessRunner; use FastForward\CodeStandards\Process\ProcessRunner; use FastForward\CodeStandards\ServiceProvider\StandardsServiceProvider; @@ -49,6 +50,8 @@ public function testAutowiringAndTransportOverrideComposeDistinctCommands(): voi self::assertInstanceOf(FixCommand::class, $container->get(FixCommand::class)); self::assertFalse(is_subclass_of(CheckCommand::class, Command::class)); self::assertFalse(is_subclass_of(FixCommand::class, Command::class)); + self::assertInstanceOf(RefactorCommand::class, $container->get(RefactorCommand::class)); + self::assertFalse(is_subclass_of(RefactorCommand::class, Command::class)); $tester = new ApplicationTester($application); self::assertSame(23, $tester->run(['command' => $name, '--all' => true, '--directory' => dirname(__DIR__, 2)])); From 82a435ac1476a4f2d9ac1dc1b3789e791a2651b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Say=C3=A3o=20Lobato=20Abreu?= Date: Mon, 26 Oct 2026 20:20:26 -0300 Subject: [PATCH 2/7] docs: describe explicit refactoring in the present tense --- docs/policy.md | 4 ++-- docs/rector.md | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/policy.md b/docs/policy.md index 08a6f15..461b928 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -57,8 +57,8 @@ Static quality in this first increment means parseable PHP and formatting conformance. Type analysis, dead-code analysis and semantic refactors are not claimed. Application tests, coverage and complete reports remain outside this domain. -The separately opted-in [native Rector command](rector.md) is a future minor -increment. It selects no legacy semantic profile and is never invoked by +The separately opted-in [native Rector command](rector.md) selects no legacy +semantic profile and is never invoked by formatting check/fix; the consumer explicitly owns rules, target PHP and caches. The [PHPDoc comparison](phpdoc-comparison.md) records actual custom Rector diff --git a/docs/rector.md b/docs/rector.md index 87d9187..24594d6 100644 --- a/docs/rector.md +++ b/docs/rector.md @@ -1,9 +1,10 @@ # Explicit semantic refactoring -This opt-in increment targets a future minor release. It does not change the -0.1 syntax/style contract, select a migration profile, or authorize publication. -`check` and `fix` never invoke the semantic runner. The separate invocable -`refactor` command uses the existing container factories and process transport. +The opt-in `refactor` command previews native semantic transformations and +applies them only when explicitly requested. It does not select a migration +profile or authorize publication. `check` and `fix` never invoke the semantic +runner. The separate invocable command uses the existing container factories +and process transport. The qualified lock selects published Rector 2.4.2, reference `e645b6463c6a88ea5b44b17d3387d35a912c7946`, with PHPStan 2.1.51. The manifest From 4f54ea8efb2cd11e6950937e45c1bffd61d8c872 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Say=C3=A3o=20Lobato=20Abreu?= Date: Mon, 26 Oct 2026 20:27:55 -0300 Subject: [PATCH 3/7] style: align explicit Rector sources with PER --- src/Console/Command/RefactorCommand.php | 4 +--- src/Refactoring/RectorRunner.php | 6 ++---- tests/Refactoring/RectorRunnerTest.php | 4 +--- tests/Refactoring/RefactorCommandTest.php | 2 +- 4 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/Console/Command/RefactorCommand.php b/src/Console/Command/RefactorCommand.php index 03c3f5b..d1a53dc 100644 --- a/src/Console/Command/RefactorCommand.php +++ b/src/Console/Command/RefactorCommand.php @@ -22,9 +22,7 @@ final readonly class RefactorCommand { /** Reuses project/config path validation and the injected subprocess boundary. */ - public function __construct(private ProjectScopeResolver $projects, private RectorRunner $rector) - { - } + public function __construct(private ProjectScopeResolver $projects, private RectorRunner $rector) {} /** Configured native rules own target PHP and source scope; no migration set is inferred from the host. */ public function __invoke( diff --git a/src/Refactoring/RectorRunner.php b/src/Refactoring/RectorRunner.php index 7ed6f82..02d5662 100644 --- a/src/Refactoring/RectorRunner.php +++ b/src/Refactoring/RectorRunner.php @@ -12,9 +12,7 @@ final readonly class RectorRunner { /** @param string $rector Absolute Rector entrypoint in this tool's Composer installation. */ - public function __construct(private ProcessRunner $process, private string $rector) - { - } + public function __construct(private ProcessRunner $process, private string $rector) {} /** * Rejects an empty rule set before Rector can initialize configuration, then previews or explicitly applies. @@ -49,7 +47,7 @@ private function assertRules(string $directory, string $config): void } $rules = is_array($inventory) ? ($inventory['rectors'] ?? null) : null; if (!is_array($rules) || [] === $rules || !array_is_list($rules) - || count(array_filter($rules, static fn (mixed $rule): bool => is_string($rule) && '' !== $rule)) !== count($rules)) { + || count(array_filter($rules, static fn(mixed $rule): bool => is_string($rule) && '' !== $rule)) !== count($rules)) { throw new InvalidArgumentException('Select a native Rector configuration with at least one rule; automatic initialization is disabled.'); } } diff --git a/tests/Refactoring/RectorRunnerTest.php b/tests/Refactoring/RectorRunnerTest.php index 6bfb602..5d977af 100644 --- a/tests/Refactoring/RectorRunnerTest.php +++ b/tests/Refactoring/RectorRunnerTest.php @@ -60,9 +60,7 @@ final class RectorTransport implements ProcessRunner public array $captures = []; /** @param list $statuses Native process/verification outcomes in execution order. */ - public function __construct(private string $inventory = '{"rectors":["ReviewedRule"]}', private array $statuses = [0]) - { - } + public function __construct(private string $inventory = '{"rectors":["ReviewedRule"]}', private array $statuses = [0]) {} public function capture(array $arguments, string $directory): string { diff --git a/tests/Refactoring/RefactorCommandTest.php b/tests/Refactoring/RefactorCommandTest.php index e5fdb43..aaba331 100644 --- a/tests/Refactoring/RefactorCommandTest.php +++ b/tests/Refactoring/RefactorCommandTest.php @@ -23,7 +23,7 @@ public function testNativeBindingAndRequiredConfiguration(): void $directory = realpath(sys_get_temp_dir()); $process = $this->createMock(ProcessRunner::class); $process->expects(self::once())->method('capture')->willReturn('{"rectors":["ReviewedRule"]}'); - $process->expects(self::once())->method('run')->with(self::callback(static fn (array $arguments): bool => in_array('--dry-run', $arguments, true)), $directory)->willReturn(2); + $process->expects(self::once())->method('run')->with(self::callback(static fn(array $arguments): bool => in_array('--dry-run', $arguments, true)), $directory)->willReturn(2); $application = new Application(); $application->setAutoExit(false); $application->addCommand(new RefactorCommand(new ProjectScopeResolver('/unused-default'), new RectorRunner($process, '/tool/rector'))); From 2f10fee418e830ea2836fa7477f77df88c9bf236 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Say=C3=A3o=20Lobato=20Abreu?= Date: Mon, 26 Oct 2026 20:42:01 -0300 Subject: [PATCH 4/7] test: preserve native Rector memory cache in CI --- docs/rector.md | 4 ++++ tests/Integration/RectorTest.php | 15 ++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/docs/rector.md b/docs/rector.md index 24594d6..d495ba3 100644 --- a/docs/rector.md +++ b/docs/rector.md @@ -41,6 +41,10 @@ PHP migration sets. Review any later `withPhpSets` selection separately. The command does not infer or overwrite target, rule, cache, bootstrap, parallelism or path choices. Configure generated caches separately from owned source, with a directory unique to the checkout. No consumer dependency/lock update occurs. +Rector 2.4.2 defaults to memory cache storage when it detects CI, so setting a +cache directory does not guarantee that directory will be created. Use native +`withCache` backend options if persistent file storage is explicitly required; +the command preserves Rector's selection. ```sh php bin/code-standards refactor --config=rector.php --directory=/project diff --git a/tests/Integration/RectorTest.php b/tests/Integration/RectorTest.php index e062af1..438bd2b 100644 --- a/tests/Integration/RectorTest.php +++ b/tests/Integration/RectorTest.php @@ -36,10 +36,11 @@ public function testPreviewApplyAndSecondPassWithExplicitNativeConfiguration(): $config = $this->configuration('\\Rector\\Php54\\Rector\\Array_\\LongArrayToShortArrayRector', 70400); $before = $this->ownedBytes(); - $preview = $this->cli(); + $preview = $this->cli(environment: ['CI' => 'true', 'GITHUB_ACTIONS' => 'true']); self::assertSame(2, $preview->getExitCode(), $preview->getOutput() . $preview->getErrorOutput()); self::assertSame($before, $this->ownedBytes()); - self::assertDirectoryExists($this->directory . '/.cache'); + // Native Rector chooses memory storage in CI even with a configured cache directory. + self::assertDirectoryDoesNotExist($this->directory . '/.cache'); self::assertSame(0, $this->cli(['--apply'])->getExitCode()); self::assertStringContainsString('[1, 2]', file_get_contents($this->directory . '/source.php')); $after = $this->ownedBytes(); @@ -127,10 +128,14 @@ private function ownedBytes(): array return $bytes; } - /** @param list $options Additional explicit command options, never shell fragments. */ - private function cli(array $options = []): Process + /** + * @param list $options Additional explicit command options, never shell fragments. + * + * @param array $environment Explicit fixture environment, inherited by the native child. + */ + private function cli(array $options = [], array $environment = []): Process { - $process = new Process([PHP_BINARY, dirname(__DIR__, 2) . '/bin/code-standards', 'refactor', '--config=policy.php', ...$options], $this->directory, timeout: 90); + $process = new Process([PHP_BINARY, dirname(__DIR__, 2) . '/bin/code-standards', 'refactor', '--config=policy.php', ...$options], $this->directory, $environment, timeout: 90); $process->run(); return $process; From 1ecf009b25fc77a17f2d55db0e814c9caa04d6e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Say=C3=A3o=20Lobato=20Abreu?= Date: Mon, 26 Oct 2026 23:10:59 -0300 Subject: [PATCH 5/7] fix: require explicit native Rector target before processing --- .changelog/explicit-rector-command.md | 3 +- docs/rector.md | 25 +++++-- src/Refactoring/RectorRunner.php | 62 ++++++++++++++-- tests/Integration/RectorTest.php | 97 +++++++++++++++++++++++++- tests/Refactoring/RectorRunnerTest.php | 34 +++++++++ 5 files changed, 204 insertions(+), 17 deletions(-) diff --git a/.changelog/explicit-rector-command.md b/.changelog/explicit-rector-command.md index 3bcd2ca..5ce39c0 100644 --- a/.changelog/explicit-rector-command.md +++ b/.changelog/explicit-rector-command.md @@ -5,5 +5,6 @@ type: minor Add a separate opt-in native Rector command with dry-run by default, explicit configuration and apply authority, empty-rule initialization protection and -post-apply verification. Keep target PHP, rules and caches consumer-owned; +post-apply verification. Require an explicit native consumer PHP target before +processing; keep target PHP, rules and caches consumer-owned; formatting check/fix never invoke semantic refactoring. diff --git a/docs/rector.md b/docs/rector.md index d495ba3..f0b5288 100644 --- a/docs/rector.md +++ b/docs/rector.md @@ -35,7 +35,10 @@ return RectorConfig::configure() ->withoutParallel(); ``` -The target is a consumer decision, independent of the host PHP. `withPhpVersion` +The target is a required consumer decision, independent of the host PHP. Declare +it with native `withPhpVersion()` or `RectorConfig::phpVersion()`, directly or in +an imported native configuration. Composer constraints and Rector's runtime +fallback do not satisfy this explicit target requirement. `withPhpVersion` limits supported transformations; it does not downgrade existing code or select PHP migration sets. Review any later `withPhpSets` selection separately. The command does not infer or overwrite target, rule, cache, bootstrap, parallelism @@ -62,13 +65,20 @@ diagnostic. Other native fatal exits propagate unchanged. Rector 2.4.2 uses `-n` ## Executable configuration and initialization boundary The command first asks native `list-rules --output-format=json` for the selected -configuration's rules. An empty or invalid inventory is rejected without calling +configuration's rules. A temporary, tool-owned native PHP wrapper imports that +configuration into Rector's own initialized container and checks its public +parameter API for an explicit PHP target. The wrapper is removed after preflight, +including failures; selected source and consumer configuration are not rewritten. +The actual `process` commands keep the original config path and cache identity. +Missing or invalid native targets fail without semantic writes. An empty or invalid inventory is rejected without calling `process`: native Rector otherwise can generate `rector.php` and return success even during dry-run. Missing explicit configuration is rejected before launch. -This is a contract for **trusted, deterministic configuration**. Preview loads -it twice; successful apply and verification load it three times. A configuration -that changes its rules between invocations can defeat that preflight. Arbitrary +This is a contract for **trusted, deterministic configuration**. Preview runs +one inventory and one processing invocation; successful apply adds a verification +invocation. Native Rector may evaluate configuration multiple times within an +invocation. The guard does not add another configuration-evaluation process. +A configuration that changes its target or rules between invocations can defeat that preflight. Arbitrary configuration, Composer autoload files, custom rules and configured bootstraps can execute and mutate files even in dry-run. The command is not a filesystem or security sandbox, and cannot promise read-only behavior for such executable @@ -85,8 +95,9 @@ Captured preflight and streamed processing use the same credential stripping. No generic promise of semantic safety follows from a zero result. The integration fixtures cover no-write preview, explicit apply/verification, empty custom -configuration, a PHP 7.4 target under a newer host, configured cache ownership, -native bootstrap failures and consumer/tool autoload collisions. Run the +configuration, missing/invalid targets, a PHP 7.4 target under a newer host, +imported targets and native service bindings, configured cache ownership, +configuration context/evaluation counts, native bootstrap failures and consumer/tool autoload collisions. Run the consumer's affected behavioral tests and review API/side-effect changes before accepting any semantic patch. Type analysis and application testing are not implicitly added to this command. diff --git a/src/Refactoring/RectorRunner.php b/src/Refactoring/RectorRunner.php index 02d5662..9f6ec8e 100644 --- a/src/Refactoring/RectorRunner.php +++ b/src/Refactoring/RectorRunner.php @@ -7,6 +7,7 @@ use FastForward\CodeStandards\Process\ProcessRunner; use InvalidArgumentException; use JsonException; +use RuntimeException; /** Runs the installed native CLI; configuration owns rules, target PHP, paths and cache. */ final readonly class RectorRunner @@ -15,10 +16,11 @@ public function __construct(private ProcessRunner $process, private string $rector) {} /** - * Rejects an empty rule set before Rector can initialize configuration, then previews or explicitly applies. + * Requires an explicit native PHP target and rules before previewing or explicitly applying. * - * Configuration is executable PHP and must be trusted/deterministic: preview evaluates it twice; - * apply with verification evaluates it three times. This transport is not a filesystem sandbox. + * Configuration is executable PHP and must be trusted/deterministic: preview uses inventory and process; + * successful apply adds verification. Native Rector may evaluate configuration more than once per invocation. + * This transport is not a filesystem sandbox. * * @return int Native process status, including 2 when dry-run proposes changes. */ @@ -37,18 +39,64 @@ public function run(string $directory, string $config, bool $apply): int /** Uses Rector's own JSON inventory instead of evaluating or parsing consumer PHP in the parent process. */ private function assertRules(string $directory, string $config): void { - $output = $this->process->capture([ - PHP_BINARY, $this->rector, 'list-rules', '--config=' . $config, '--output-format=json', - ], $directory); + $output = $this->nativeInventory($directory, $config); try { $inventory = json_decode($output, true, flags: JSON_THROW_ON_ERROR); } catch (JsonException $error) { throw new InvalidArgumentException('Rector did not return a valid native rule inventory.', previous: $error); } $rules = is_array($inventory) ? ($inventory['rectors'] ?? null) : null; - if (!is_array($rules) || [] === $rules || !array_is_list($rules) + if (!is_array($rules) + || [] === $rules + || !array_is_list($rules) || count(array_filter($rules, static fn(mixed $rule): bool => is_string($rule) && '' !== $rule)) !== count($rules)) { throw new InvalidArgumentException('Select a native Rector configuration with at least one rule; automatic initialization is disabled.'); } } + + /** Imports the original config in Rector's native container; no application/config source is copied or parsed. */ + private function nativeInventory(string $directory, string $config): string + { + $temporary = sys_get_temp_dir() . '/code-standards-rector-' . bin2hex(random_bytes(16)); + if (!mkdir($temporary, 0700)) { + throw new RuntimeException('Cannot create the native Rector preflight configuration.'); + } + $preflight = $temporary . '/preflight.php'; + try { + $source = $this->targetGuard($config); + if (strlen($source) !== file_put_contents($preflight, $source)) { + throw new RuntimeException('Cannot write the native Rector preflight configuration.'); + } + + return $this->process->capture([ + PHP_BINARY, $this->rector, 'list-rules', '--config=' . $preflight, '--output-format=json', + ], $directory); + } finally { + if (is_file($preflight)) { + unlink($preflight); + } + rmdir($temporary); + } + } + + /** Uses public native configuration/parameter APIs; actual process commands retain the original config identity. */ + private function targetGuard(string $config): string + { + return sprintf(<<<'PHP' + import(%s); + if ($implicitTarget + || !SimpleParameterProvider::hasParameter(Option::PHP_VERSION_FEATURES) + || SimpleParameterProvider::provideIntParameter(Option::PHP_VERSION_FEATURES) <= 0) { + throw new InvalidArgumentException('Select an explicit PHP target with native RectorConfig::phpVersion() or withPhpVersion().'); + } + }; + PHP, var_export($config, true)); + } } diff --git a/tests/Integration/RectorTest.php b/tests/Integration/RectorTest.php index 438bd2b..be83e60 100644 --- a/tests/Integration/RectorTest.php +++ b/tests/Integration/RectorTest.php @@ -5,6 +5,7 @@ namespace FastForward\CodeStandards\Tests\Integration; use PHPUnit\Framework\TestCase; +use PHPUnit\Framework\Attributes\DataProvider; use Symfony\Component\Process\Process; /** Real installed CLI and native rules; only disposable consumer files are touched. */ @@ -54,7 +55,7 @@ public function testPreviewApplyAndSecondPassWithExplicitNativeConfiguration(): public function testEmptyConfigDoesNotGenerateRectorFile(): void { file_put_contents($this->directory . '/source.php', 'directory . '/policy.php', 'directory . '/policy.php', 'withPhpVersion(70400);'); $before = $this->ownedBytes(); $result = $this->cli(); @@ -64,6 +65,30 @@ public function testEmptyConfigDoesNotGenerateRectorFile(): void self::assertSame($before, $this->ownedBytes()); } + /** @return iterable}> Both preview and application require a declared target. */ + public static function executionModes(): iterable + { + yield 'preview' => [[]]; + yield 'apply' => [['--apply']]; + } + + /** Valid rules must not silently inherit the executing runtime as the consumer PHP target. */ + #[DataProvider('executionModes')] + public function testMissingExplicitTargetRejectsBeforeProcessing(array $options): void + { + file_put_contents($this->directory . '/source.php', 'directory . '/composer.json', '{"require":{"php":"^7.4"}}'); + $config = $this->configuration('\\Rector\\Php54\\Rector\\Array_\\LongArrayToShortArrayRector', 70400); + file_put_contents($this->directory . '/policy.php', str_replace('->withPhpVersion(70400)', '', $config)); + $before = $this->ownedBytes(); + $result = $this->cli($options); + + self::assertSame(1, $result->getExitCode(), $result->getOutput() . $result->getErrorOutput()); + self::assertStringContainsString('explicit PHP target', $result->getErrorOutput()); + self::assertFileDoesNotExist($this->directory . '/rector.php'); + self::assertSame($before, $this->ownedBytes()); + } + /** A target below the executing PHP suppresses a versioned transformation without selecting migration sets. */ public function testConsumerTargetControlsVersionedRule(): void { @@ -77,6 +102,74 @@ public function testConsumerTargetControlsVersionedRule(): void self::assertSame($source, file_get_contents($this->directory . '/source.php')); } + /** Native import and service bindings preserve cwd/config context and evaluation counts through the preflight. */ + public function testImportedExplicitTargetAndNativeServicesPreserveConfigurationContext(): void + { + file_put_contents($this->directory . '/source.php', 'directory . '/relative.txt', 'consumer context'); + file_put_contents($this->directory . '/parts.php', <<<'PHP' + phpVersion(70400); + $config->paths([__DIR__ . '/source.php']); + $config->rule(\Rector\Php54\Rector\Array_\LongArrayToShortArrayRector::class); + $config->disableParallel(); + }; + PHP); + file_put_contents($this->directory . '/policy.php', <<<'PHP' + import(__DIR__ . '/parts.php'); + $config->make(\PHPStan\Parser\Parser::class); + file_put_contents(__DIR__ . '/context.jsonl', json_encode([ + 'cwd' => getcwd(), + 'directory' => __DIR__, + 'relative' => file_get_contents('relative.txt'), + ], JSON_THROW_ON_ERROR) . "\n", FILE_APPEND); + }; + PHP); + $before = $this->ownedBytes(); + $native = [PHP_BINARY, dirname(__DIR__, 2) . '/vendor/rector/rector/bin/rector']; + $inventory = new Process([...$native, 'list-rules', '--config=policy.php', '--output-format=json'], $this->directory, timeout: 90); + $inventory->mustRun(); + $inventoryLoads = count(file($this->directory . '/context.jsonl', FILE_IGNORE_NEW_LINES)); + unlink($this->directory . '/context.jsonl'); + $processing = new Process([...$native, 'process', '--config=policy.php', '--dry-run', '--no-progress-bar'], $this->directory, timeout: 90); + self::assertSame(2, $processing->run(), $processing->getOutput() . $processing->getErrorOutput()); + $processLoads = count(file($this->directory . '/context.jsonl', FILE_IGNORE_NEW_LINES)); + unlink($this->directory . '/context.jsonl'); + $preview = $this->cli(); + self::assertSame(2, $preview->getExitCode(), $preview->getOutput() . $preview->getErrorOutput()); + self::assertSame($before, $this->ownedBytes()); + $lines = file($this->directory . '/context.jsonl', FILE_IGNORE_NEW_LINES); + self::assertCount($inventoryLoads + $processLoads, $lines); + foreach ($lines as $line) { + self::assertSame([ + 'cwd' => $this->directory, + 'directory' => $this->directory, + 'relative' => 'consumer context', + ], json_decode($line, true, flags: JSON_THROW_ON_ERROR)); + } + $apply = $this->cli(['--apply']); + self::assertSame(0, $apply->getExitCode(), $apply->getOutput() . $apply->getErrorOutput()); + self::assertCount(2 * $inventoryLoads + 3 * $processLoads, file($this->directory . '/context.jsonl', FILE_IGNORE_NEW_LINES)); + self::assertStringContainsString('[1]', file_get_contents($this->directory . '/source.php')); + } + + /** Native invalid target/config errors must fail without processing or mutating selected source. */ + public function testInvalidExplicitTargetFailsWithoutWriting(): void + { + file_put_contents($this->directory . '/source.php', 'configuration('\\Rector\\Php54\\Rector\\Array_\\LongArrayToShortArrayRector', $target); + $before = $this->ownedBytes(); + $result = $this->cli(['--apply']); + self::assertNotSame(0, $result->getExitCode(), $result->getOutput() . $result->getErrorOutput()); + self::assertSame($before, $this->ownedBytes()); + self::assertFileDoesNotExist($this->directory . '/rector.php'); + } + } + /** The subprocess boundary protects the parent, but a consumer autoload collision still fails in the child. */ public function testConsumerAutoloadCollisionIsNotClaimedToBeIsolated(): void { @@ -119,7 +212,7 @@ private function configuration(string $rule, int $target): string private function ownedBytes(): array { $bytes = []; - foreach (['source.php', 'policy.php', 'composer.lock'] as $name) { + foreach (['source.php', 'policy.php', 'composer.json', 'composer.lock'] as $name) { if (is_file($this->directory . '/' . $name)) { $bytes[$name] = file_get_contents($this->directory . '/' . $name); } diff --git a/tests/Refactoring/RectorRunnerTest.php b/tests/Refactoring/RectorRunnerTest.php index 5d977af..7fda479 100644 --- a/tests/Refactoring/RectorRunnerTest.php +++ b/tests/Refactoring/RectorRunnerTest.php @@ -8,6 +8,7 @@ use FastForward\CodeStandards\Refactoring\RectorRunner; use InvalidArgumentException; use PHPUnit\Framework\TestCase; +use RuntimeException; /** Checks write authority and the native empty-configuration initialization boundary. */ final class RectorRunnerTest extends TestCase @@ -51,6 +52,30 @@ public function testEmptyOrMalformedRulesNeverReachProcess(): void } } } + + /** Only the temporary preflight is removed; original config identity stays on every process command. */ + public function testTargetGuardEscapesOriginalPathAndCleansItsOwnFile(): void + { + $process = new RectorTransport(); + $config = "/consumer/quote'line\npolicy.php"; + (new RectorRunner($process, '/tool/rector'))->run('/consumer', $config, false); + + self::assertCount(1, $process->preflights); + self::assertStringContainsString('$config->import(' . var_export($config, true) . ');', $process->guards[0]); + self::assertFileDoesNotExist($process->preflights[0]); + self::assertContains('--config=' . $config, $process->commands[0]); + + $failure = new RectorTransport(); + $failure->failCapture = true; + try { + (new RectorRunner($failure, '/tool/rector'))->run('/consumer', $config, true); + self::fail('Native bootstrap failure must remain observable.'); + } catch (RuntimeException $exception) { + self::assertSame('fixture capture failure', $exception->getMessage()); + self::assertFileDoesNotExist($failure->preflights[0]); + self::assertSame([], $failure->commands); + } + } } /** Records only the execution boundary; no native configuration or source is evaluated here. */ @@ -58,6 +83,9 @@ final class RectorTransport implements ProcessRunner { public array $commands = []; public array $captures = []; + public array $preflights = []; + public array $guards = []; + public bool $failCapture = false; /** @param list $statuses Native process/verification outcomes in execution order. */ public function __construct(private string $inventory = '{"rectors":["ReviewedRule"]}', private array $statuses = [0]) {} @@ -65,6 +93,12 @@ public function __construct(private string $inventory = '{"rectors":["ReviewedRu public function capture(array $arguments, string $directory): string { $this->captures[] = $arguments; + $preflight = substr($arguments[3], strlen('--config=')); + $this->preflights[] = $preflight; + $this->guards[] = file_get_contents($preflight); + if ($this->failCapture) { + throw new RuntimeException('fixture capture failure'); + } return $this->inventory; } From d989a15c1357adda9e62b43d54fb8ec9d7d66208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Say=C3=A3o=20Lobato=20Abreu?= Date: Tue, 27 Oct 2026 00:28:05 -0300 Subject: [PATCH 6/7] fix: bound Rector timeouts and clarify opt-in contracts --- .changelog/explicit-rector-command.md | 2 + docs/policy.md | 18 ++-- docs/rector.md | 12 ++- .../fast-forward-code-standards/SKILL.md | 5 +- src/Console/Command/RefactorCommand.php | 27 ++++- src/Process/NativeProcessRunner.php | 20 +++- src/Process/ProcessRunner.php | 14 ++- src/Refactoring/RectorRunner.php | 16 +-- tests/Integration/RectorTest.php | 24 +++++ tests/Process/NativeProcessRunnerTest.php | 51 ++++++++++ tests/Refactoring/RectorRunnerTest.php | 19 +++- tests/Refactoring/RefactorCommandTest.php | 99 ++++++++++++++++++- tests/ServiceProvider/CompositionTest.php | 6 +- tests/Standards/StandardsRunnerTest.php | 4 +- 14 files changed, 278 insertions(+), 39 deletions(-) create mode 100644 tests/Process/NativeProcessRunnerTest.php diff --git a/.changelog/explicit-rector-command.md b/.changelog/explicit-rector-command.md index 5ce39c0..25372e5 100644 --- a/.changelog/explicit-rector-command.md +++ b/.changelog/explicit-rector-command.md @@ -8,3 +8,5 @@ configuration and apply authority, empty-rule initialization protection and post-apply verification. Require an explicit native consumer PHP target before processing; keep target PHP, rules and caches consumer-owned; formatting check/fix never invoke semantic refactoring. +Allow an explicit positive per-subprocess timeout without disabling bounds; +execution timeouts fail with status 1, while cache configuration remains optional. diff --git a/docs/policy.md b/docs/policy.md index dedb051..2f7d653 100644 --- a/docs/policy.md +++ b/docs/policy.md @@ -27,7 +27,7 @@ from blocked PR #376 or composer-installers #15 was executed or changed. | Scope | source resolver excludes generated/resources/public/vendor | consumer Finder, hidden files, VCS ignores, path exclusions | Tracked PHP/bin candidates filtered by root/nested consumer .gitignore (including tracked matches); only .git structurally excluded; PHP symlinks rejected | | Overrides | customize callback last | extraRules last | customize callback last; explicit config selection | | Check | combined orchestration has independent mutation risks | dry-run fixer | syntax parse then ECS check, no source writes | -| Semantic transformations | Rector seven sets, PHP migration, Safe migration, Faker and custom PHPDoc | mixed into risky fixer | Not implemented; requires explicit target, opt-in and independent tests | +| Semantic transformations | Rector seven sets, PHP migration, Safe migration, Faker and custom PHPDoc | mixed into risky fixer | Not implemented in the initial extraction snapshot; required explicit target, opt-in and independent tests | The explicit rule deltas above are intentional. Transitive upstream preset membership is dependency-version specific and fixed for contributor builds by @@ -53,13 +53,17 @@ exercise headers and overrides in golden fixtures and decide which semantic transformations belong in an opt-in profile. Optional Twig/magic-value policy, Composer metadata normalization and existing public DevTools APIs stay untouched. -Static quality in this first increment means parseable PHP and formatting -conformance. Type analysis, dead-code analysis and semantic refactors are not -claimed. Application tests, coverage and complete reports remain outside this domain. +The initial extraction snapshot limited static quality to parseable PHP and +formatting conformance; it did not claim type analysis, dead-code analysis or +semantic refactors. Application tests, coverage and complete reports remain +outside this domain. -The separately opted-in [native Rector command](rector.md) selects no legacy -semantic profile and is never invoked by -formatting check/fix; the consumer explicitly owns rules, target PHP and caches. +The current default formatter does not select a semantic profile. The separately +opted-in [native Rector command](rector.md) exists and requires explicit native +configuration, reviewed rules and a consumer PHP target; formatting check/fix +never invoke it. Native cache configuration is optional and remains consumer-owned, +including Rector's in-memory cache selection in CI. This command does not claim +parity with the three legacy custom PHPDoc transformations; #4 remains partial. The [PHPDoc comparison](phpdoc-comparison.md) records actual custom Rector activation and the fixtures required before adopting a documentation policy. diff --git a/docs/rector.md b/docs/rector.md index f0b5288..b9886cb 100644 --- a/docs/rector.md +++ b/docs/rector.md @@ -53,15 +53,25 @@ the command preserves Rector's selection. php bin/code-standards refactor --config=rector.php --directory=/project # After reviewing the proposed semantic diff: php bin/code-standards refactor --config=rector.php --directory=/project --apply +# For a larger project, choose an explicit finite per-subprocess budget: +php bin/code-standards refactor --config=rector.php --directory=/project --timeout=900 ``` Preview passes native `--dry-run`. Explicit apply performs one write pass and then a dry-run verification; remaining differences fail. Native exits are 0 for -clean/success, 1 for processing errors and 2 for proposed changes. Controlled +clean/success, 1 for processing errors or subprocess timeouts and 2 for proposed changes. Controlled invocation, empty-rule and invalid-inventory errors also return 2, with an error diagnostic. Other native fatal exits propagate unchanged. Rector 2.4.2 uses `-n` for dry-run and does not expose the ECS `--no-interaction` option; its native flags are not translated blindly. +`--timeout` accepts a positive whole number of seconds and defaults to 300. +It bounds each Rector subprocess separately: preflight inventory, processing and +post-apply verification receive the same limit. Fractional, negative, zero, +nonfinite and overflowing values are rejected; no unlimited mode is exposed. +The option changes this refactor invocation, leaving Git's 120-second captured +transport default and formatting's 300-second default unchanged. Native Rector +process/parallel-worker limits remain native configuration decisions. + ## Executable configuration and initialization boundary The command first asks native `list-rules --output-format=json` for the selected diff --git a/resources/skills/fast-forward-code-standards/SKILL.md b/resources/skills/fast-forward-code-standards/SKILL.md index 039c127..034b01c 100644 --- a/resources/skills/fast-forward-code-standards/SKILL.md +++ b/resources/skills/fast-forward-code-standards/SKILL.md @@ -25,8 +25,9 @@ metadata: 5. Only when fixing is requested run `code-standards fix --all`, inspect the diff and rerun check. Do not commit/push automatically. Semantic migrations need a separate reviewed procedure: the opt-in `refactor` command requires explicit - native Rector configuration, consumer target PHP/cache selection and reviewed - rules. Preview is default; only explicit `--apply` grants semantic writes. + native Rector configuration, an explicit consumer PHP target and reviewed + rules. Native cache configuration is optional; Rector may use memory in CI. + Preview is default; only explicit `--apply` grants semantic writes. Read `docs/rector.md` for executable-config/autoload limits. Never substitute this command for ordinary formatting `fix` or automatically migrate consumers. 6. For requested CI adoption copy `resources/workflows/code-standards.yml` from diff --git a/src/Console/Command/RefactorCommand.php b/src/Console/Command/RefactorCommand.php index d1a53dc..439be77 100644 --- a/src/Console/Command/RefactorCommand.php +++ b/src/Console/Command/RefactorCommand.php @@ -13,6 +13,7 @@ use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Style\SymfonyStyle; use Symfony\Component\Process\Exception\ProcessFailedException; +use Symfony\Component\Process\Exception\ProcessTimedOutException; /** Explicit semantic refactoring is independent of the check/fix formatting operations. */ #[AsCommand( @@ -28,7 +29,7 @@ public function __construct(private ProjectScopeResolver $projects, private Rect public function __invoke( SymfonyStyle $io, - #[Option(description: 'Required existing native Rector PHP configuration, including explicit target PHP and cache.')] + #[Option(description: 'Required existing native Rector PHP configuration with an explicit target PHP; cache configuration is optional.')] ?string $config = null, #[Option(description: 'Project directory; relative configuration paths resolve here.', shortcut: 'C')] @@ -36,22 +37,44 @@ public function __invoke( #[Option(description: 'Explicitly apply the selected semantic changes, then verify with a dry-run.')] bool $apply = false, + + #[Option(description: 'Positive whole seconds per Rector subprocess, including preflight and verification.')] + string $timeout = '300', ): int { try { if (null === $config) { throw new InvalidArgumentException('Select an explicit native Rector configuration with --config.'); } + + $seconds = $this->timeoutSeconds($timeout); $scope = $this->projects->resolve(true, null, null, $directory, $config); - return $this->rector->run($scope->directory, $scope->config, $apply); + return $this->rector->run($scope->directory, $scope->config, $apply, $seconds); } catch (ProcessFailedException $error) { $io->getErrorStyle()->error($error->getMessage()); return $error->getProcess()->getExitCode() ?? Command::FAILURE; + } catch (ProcessTimedOutException $error) { + $io->getErrorStyle()->error($error->getMessage()); + + return Command::FAILURE; } catch (InvalidArgumentException|RuntimeException $error) { $io->getErrorStyle()->error($error->getMessage()); return Command::INVALID; } } + + /** Preserves the literal option contract instead of silently coercing decimal, exponent or overflowing input. */ + private function timeoutSeconds(string $timeout): int + { + $seconds = (int) $timeout; + + if (1 !== preg_match('/^[1-9][0-9]*$/D', $timeout) + || (string) $seconds !== $timeout) { + throw new InvalidArgumentException('--timeout must be a positive whole number of seconds.'); + } + + return $seconds; + } } diff --git a/src/Process/NativeProcessRunner.php b/src/Process/NativeProcessRunner.php index ac2d3fb..d987717 100644 --- a/src/Process/NativeProcessRunner.php +++ b/src/Process/NativeProcessRunner.php @@ -4,6 +4,7 @@ namespace FastForward\CodeStandards\Process; +use InvalidArgumentException; use Symfony\Component\Process\Process; /** Native transport; callers own selection and operation policy. */ @@ -16,19 +17,30 @@ final class NativeProcessRunner implements ProcessRunner public function __construct(private readonly ?\Closure $write = null) {} /** Captures native output as bytes, including Git paths and Rector's JSON inventory. */ - public function capture(array $arguments, string $directory): string + public function capture(array $arguments, string $directory, float $timeout = 120): string { - $process = new Process($arguments, $directory, self::ENVIRONMENT, timeout: 120); + $process = $this->createProcess($arguments, $directory, $timeout); $process->mustRun(); return $process->getOutput(); } /** Runs one tool, streaming its output without changing its failure status. */ - public function run(array $arguments, string $directory): int + public function run(array $arguments, string $directory, float $timeout = 300): int { - $process = new Process($arguments, $directory, self::ENVIRONMENT, timeout: 300); + $process = $this->createProcess($arguments, $directory, $timeout); return $process->run($this->write); } + + /** @param list $arguments Positive finite bounds never become Symfony's zero/null unlimited mode. */ + private function createProcess(array $arguments, string $directory, float $timeout): Process + { + if (!is_finite($timeout) + || $timeout <= 0) { + throw new InvalidArgumentException('Subprocess timeout must be a positive finite number of seconds.'); + } + + return new Process($arguments, $directory, self::ENVIRONMENT, timeout: $timeout); + } } diff --git a/src/Process/ProcessRunner.php b/src/Process/ProcessRunner.php index 698523b..a4e7bda 100644 --- a/src/Process/ProcessRunner.php +++ b/src/Process/ProcessRunner.php @@ -7,9 +7,15 @@ /** Executes bounded argument arrays without shell interpolation. */ interface ProcessRunner { - /** @param list $arguments Returns stdout or throws on failure. */ - public function capture(array $arguments, string $directory): string; + /** + * @param list $arguments Returns stdout or throws on failure. + * @param positive-float $timeout Finite maximum seconds for this subprocess; defaults retain Git's existing limit. + */ + public function capture(array $arguments, string $directory, float $timeout = 120): string; - /** @param list $arguments Streams diagnostics and returns the actual exit code. */ - public function run(array $arguments, string $directory): int; + /** + * @param list $arguments Streams diagnostics and returns the actual exit code. + * @param positive-float $timeout Finite maximum seconds for this subprocess; default formatting limit is unchanged. + */ + public function run(array $arguments, string $directory, float $timeout = 300): int; } diff --git a/src/Refactoring/RectorRunner.php b/src/Refactoring/RectorRunner.php index 9f6ec8e..adbc67d 100644 --- a/src/Refactoring/RectorRunner.php +++ b/src/Refactoring/RectorRunner.php @@ -24,22 +24,22 @@ public function __construct(private ProcessRunner $process, private string $rect * * @return int Native process status, including 2 when dry-run proposes changes. */ - public function run(string $directory, string $config, bool $apply): int + public function run(string $directory, string $config, bool $apply, float $timeout = 300): int { - $this->assertRules($directory, $config); + $this->assertRules($directory, $config, $timeout); $check = [PHP_BINARY, $this->rector, 'process', '--config=' . $config, '--no-progress-bar']; - $status = $this->process->run($apply ? $check : [...$check, '--dry-run'], $directory); + $status = $this->process->run($apply ? $check : [...$check, '--dry-run'], $directory, $timeout); if (!$apply || 0 !== $status) { return $status; } - return $this->process->run([...$check, '--dry-run'], $directory); + return $this->process->run([...$check, '--dry-run'], $directory, $timeout); } /** Uses Rector's own JSON inventory instead of evaluating or parsing consumer PHP in the parent process. */ - private function assertRules(string $directory, string $config): void + private function assertRules(string $directory, string $config, float $timeout): void { - $output = $this->nativeInventory($directory, $config); + $output = $this->nativeInventory($directory, $config, $timeout); try { $inventory = json_decode($output, true, flags: JSON_THROW_ON_ERROR); } catch (JsonException $error) { @@ -55,7 +55,7 @@ private function assertRules(string $directory, string $config): void } /** Imports the original config in Rector's native container; no application/config source is copied or parsed. */ - private function nativeInventory(string $directory, string $config): string + private function nativeInventory(string $directory, string $config, float $timeout): string { $temporary = sys_get_temp_dir() . '/code-standards-rector-' . bin2hex(random_bytes(16)); if (!mkdir($temporary, 0700)) { @@ -70,7 +70,7 @@ private function nativeInventory(string $directory, string $config): string return $this->process->capture([ PHP_BINARY, $this->rector, 'list-rules', '--config=' . $preflight, '--output-format=json', - ], $directory); + ], $directory, $timeout); } finally { if (is_file($preflight)) { unlink($preflight); diff --git a/tests/Integration/RectorTest.php b/tests/Integration/RectorTest.php index be83e60..85df401 100644 --- a/tests/Integration/RectorTest.php +++ b/tests/Integration/RectorTest.php @@ -197,6 +197,30 @@ public function testExecutableConfigFailureAndRequiredOption(): void self::assertSame(2, $missing->run()); } + /** A real native preflight timeout is an execution failure and removes its owned guard without source writes. */ + public function testNativePreflightTimeoutPreservesConsumerAndCleansGuard(): void + { + file_put_contents($this->directory . '/source.php', 'configuration('\\Rector\\Php54\\Rector\\Array_\\LongArrayToShortArrayRector', 70400); + $slow = 'directory . '/policy.php', $slow); + $before = $this->ownedBytes(); + $result = $this->cli(['--timeout=2']); + + self::assertSame(1, $result->getExitCode(), $result->getOutput() . $result->getErrorOutput()); + self::assertStringContainsString('exceeded the timeout', $result->getErrorOutput()); + self::assertSame($before, $this->ownedBytes()); + $preflight = file_get_contents($this->directory . '/preflight.path'); + self::assertStringContainsString('/code-standards-rector-', $preflight); + self::assertFileDoesNotExist($preflight); + self::assertDirectoryDoesNotExist(dirname($preflight)); + + file_put_contents($this->directory . '/policy.php', $config); + $accepted = $this->cli(['--timeout=5']); + self::assertSame(2, $accepted->getExitCode(), $accepted->getOutput() . $accepted->getErrorOutput()); + } + /** Returns native builder PHP unchanged; target and cache remain consumer-owned options. */ private function configuration(string $rule, int $target): string { diff --git a/tests/Process/NativeProcessRunnerTest.php b/tests/Process/NativeProcessRunnerTest.php new file mode 100644 index 0000000..5aa9371 --- /dev/null +++ b/tests/Process/NativeProcessRunnerTest.php @@ -0,0 +1,51 @@ +{$method}([PHP_BINARY, '-r', 'usleep(250000);'], sys_get_temp_dir(), 0.01); + self::fail('A controlled long subprocess must time out.'); + } catch (ProcessTimedOutException $exception) { + self::assertSame(0.01, $exception->getExceededTimeout()); + } + } + $arguments = [PHP_BINARY, '-r', 'usleep(50000); echo "completed";']; + self::assertSame('completed', $process->capture($arguments, sys_get_temp_dir(), 2)); + $output = ''; + $streamed = new NativeProcessRunner(static function (string $type, string $bytes) use (&$output): void { + $output .= $bytes; + }); + self::assertSame(0, $streamed->run($arguments, sys_get_temp_dir(), 2)); + self::assertSame('completed', $output); + } + + /** Zero, negative and nonfinite values cannot silently become Symfony's unlimited mode. */ + public function testInvalidBoundsFailBeforeLaunchingEitherChannel(): void + { + foreach (['capture', 'run'] as $method) { + foreach ([0.0, -0.1, INF, -INF, NAN] as $timeout) { + try { + (new NativeProcessRunner())->{$method}([PHP_BINARY, '-r', 'echo "unexpected";'], sys_get_temp_dir(), $timeout); + self::fail('Invalid subprocess limits must be rejected.'); + } catch (InvalidArgumentException $exception) { + self::assertStringContainsString('positive finite', $exception->getMessage()); + } + } + } + } +} diff --git a/tests/Refactoring/RectorRunnerTest.php b/tests/Refactoring/RectorRunnerTest.php index 7fda479..7aec365 100644 --- a/tests/Refactoring/RectorRunnerTest.php +++ b/tests/Refactoring/RectorRunnerTest.php @@ -23,6 +23,8 @@ public function testPreviewPreservesNativeDifferencesWithoutWriteAuthority(): vo self::assertContains('--config=/consumer/my policy.php', $process->commands[0]); self::assertContains('list-rules', $process->captures[0]); self::assertNotContains('--no-interaction', $process->commands[0]); + self::assertSame([300.0], $process->captureTimeouts); + self::assertSame([300.0], $process->runTimeouts); } /** Apply is a single write pass followed by a read-only verification whose failure remains observable. */ @@ -76,6 +78,15 @@ public function testTargetGuardEscapesOriginalPathAndCleansItsOwnFile(): void self::assertSame([], $failure->commands); } } + + /** One genuine operation parameter bounds inventory, apply and verification without changing global defaults. */ + public function testExplicitTimeoutReachesEveryRectorSubprocess(): void + { + $process = new RectorTransport(statuses: [0, 0]); + self::assertSame(0, (new RectorRunner($process, '/tool/rector'))->run('/consumer', '/consumer/policy.php', true, 600)); + self::assertSame([600.0], $process->captureTimeouts); + self::assertSame([600.0, 600.0], $process->runTimeouts); + } } /** Records only the execution boundary; no native configuration or source is evaluated here. */ @@ -86,13 +97,16 @@ final class RectorTransport implements ProcessRunner public array $preflights = []; public array $guards = []; public bool $failCapture = false; + public array $captureTimeouts = []; + public array $runTimeouts = []; /** @param list $statuses Native process/verification outcomes in execution order. */ public function __construct(private string $inventory = '{"rectors":["ReviewedRule"]}', private array $statuses = [0]) {} - public function capture(array $arguments, string $directory): string + public function capture(array $arguments, string $directory, float $timeout = 120): string { $this->captures[] = $arguments; + $this->captureTimeouts[] = $timeout; $preflight = substr($arguments[3], strlen('--config=')); $this->preflights[] = $preflight; $this->guards[] = file_get_contents($preflight); @@ -103,9 +117,10 @@ public function capture(array $arguments, string $directory): string return $this->inventory; } - public function run(array $arguments, string $directory): int + public function run(array $arguments, string $directory, float $timeout = 300): int { $this->commands[] = $arguments; + $this->runTimeouts[] = $timeout; return array_shift($this->statuses); } diff --git a/tests/Refactoring/RefactorCommandTest.php b/tests/Refactoring/RefactorCommandTest.php index aaba331..88e0537 100644 --- a/tests/Refactoring/RefactorCommandTest.php +++ b/tests/Refactoring/RefactorCommandTest.php @@ -10,8 +10,11 @@ use FastForward\CodeStandards\Project\ProjectScopeResolver; use FastForward\CodeStandards\Refactoring\RectorRunner; use PHPUnit\Framework\TestCase; +use PHPUnit\Framework\Attributes\DataProvider; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\ApplicationTester; +use Symfony\Component\Process\Exception\ProcessTimedOutException; +use Symfony\Component\Process\Process; /** Exercises real attribute binding without loading Rector into the parent process. */ final class RefactorCommandTest extends TestCase @@ -39,8 +42,8 @@ public function testNativeBindingAndRequiredConfiguration(): void } } - /** Captured preflight configuration receives the same credential stripping as streamed execution. */ - public function testCapturedConfigurationDoesNotReceiveToolCredentials(): void + /** Both channels preserve credential filtering when a caller supplies a finite timeout override. */ + public function testConfigurationSubprocessesDoNotReceiveToolCredentials(): void { $previous = []; foreach (['GH_TOKEN', 'GITHUB_TOKEN', 'COMPOSER_AUTH'] as $name) { @@ -49,14 +52,102 @@ public function testCapturedConfigurationDoesNotReceiveToolCredentials(): void } try { - $output = (new NativeProcessRunner())->capture([ + $arguments = [ PHP_BINARY, '-r', 'foreach (["GH_TOKEN", "GITHUB_TOKEN", "COMPOSER_AUTH"] as $name) { echo getenv($name) === false ? "absent\n" : "present\n"; }', - ], sys_get_temp_dir()); + ]; + $output = (new NativeProcessRunner())->capture($arguments, sys_get_temp_dir(), 2); self::assertSame("absent\nabsent\nabsent\n", $output); + $streamed = ''; + $process = new NativeProcessRunner(static function (string $type, string $bytes) use (&$streamed): void { + $streamed .= $bytes; + }); + self::assertSame(0, $process->run($arguments, sys_get_temp_dir(), 2)); + self::assertSame("absent\nabsent\nabsent\n", $streamed); } finally { foreach ($previous as $name => $value) { putenv(false === $value ? $name : $name . '=' . $value); } } } + + /** An actual short subprocess timeout is an execution failure, with the preflight cleaned on the same exception path. */ + public function testTimeoutIsFailureAndCleansThePreflight(): void + { + $native = new Process([PHP_BINARY, '-r', 'usleep(250000);'], timeout: 0.01); + try { + $native->mustRun(); + self::fail('The controlled subprocess must time out.'); + } catch (ProcessTimedOutException $failure) { + $timeout = $failure; + } + $config = tempnam(sys_get_temp_dir(), 'rector-policy-'); + $preflight = null; + $process = $this->createMock(ProcessRunner::class); + $process->expects(self::once())->method('capture')->willReturnCallback(static function (array $arguments) use (&$preflight, $timeout): never { + $preflight = substr($arguments[3], strlen('--config=')); + throw $timeout; + }); + $process->expects(self::never())->method('run'); + $application = new Application(); + $application->setAutoExit(false); + $application->addCommand(new RefactorCommand(new ProjectScopeResolver('/unused-default'), new RectorRunner($process, '/tool/rector'))); + $tester = new ApplicationTester($application); + + try { + self::assertSame(1, $tester->run(['command' => 'refactor', '--config' => $config, '--directory' => realpath(sys_get_temp_dir())])); + self::assertStringContainsString('exceeded the timeout', $tester->getDisplay()); + self::assertIsString($preflight); + self::assertFileDoesNotExist($preflight); + self::assertDirectoryDoesNotExist(dirname($preflight)); + } finally { + unlink($config); + } + } + + /** @return iterable Inputs that must never be coerced to a disabled or unintended limit. */ + public static function invalidTimeouts(): iterable + { + foreach (['bad', '1.5', '0.5', '0', '-1', '1e3', 'INF', 'NAN', ' 5', '5 ', '+5', '01', '999999999999999999999'] as $input) { + yield $input => [$input]; + } + } + + /** Rejects malformed bounds through the actual Symfony option binding before touching transport. */ + #[DataProvider('invalidTimeouts')] + public function testInvalidTimeoutIsRejectedWithoutLaunchingProcesses(string $timeout): void + { + $config = tempnam(sys_get_temp_dir(), 'rector-policy-'); + $process = $this->createMock(ProcessRunner::class); + $process->expects(self::never())->method('capture'); + $process->expects(self::never())->method('run'); + $application = new Application(); + $application->setAutoExit(false); + $application->addCommand(new RefactorCommand(new ProjectScopeResolver('/unused-default'), new RectorRunner($process, '/tool/rector'))); + $tester = new ApplicationTester($application); + try { + self::assertSame(2, $tester->run(['command' => 'refactor', '--config' => $config, '--timeout' => $timeout])); + self::assertStringContainsString('--timeout must be a positive whole number', $tester->getDisplay()); + } finally { + unlink($config); + } + } + + /** The actual option binding forwards the requested positive limit to the native preflight and dry-run. */ + public function testPositiveTimeoutOptionIsForwardedWithoutCoercion(): void + { + $config = tempnam(sys_get_temp_dir(), 'rector-policy-'); + $directory = realpath(sys_get_temp_dir()); + $process = $this->createMock(ProcessRunner::class); + $process->expects(self::once())->method('capture')->with(self::isArray(), $directory, 600.0)->willReturn('{"rectors":["ReviewedRule"]}'); + $process->expects(self::once())->method('run')->with(self::isArray(), $directory, 600.0)->willReturn(0); + $application = new Application(); + $application->setAutoExit(false); + $application->addCommand(new RefactorCommand(new ProjectScopeResolver('/unused-default'), new RectorRunner($process, '/tool/rector'))); + $tester = new ApplicationTester($application); + try { + self::assertSame(0, $tester->run(['command' => 'refactor', '--config' => $config, '--directory' => $directory, '--timeout' => '600'])); + } finally { + unlink($config); + } + } } diff --git a/tests/ServiceProvider/CompositionTest.php b/tests/ServiceProvider/CompositionTest.php index 82815d0..73571e4 100644 --- a/tests/ServiceProvider/CompositionTest.php +++ b/tests/ServiceProvider/CompositionTest.php @@ -113,13 +113,13 @@ final class RecordingTransport implements ProcessRunner public array $commands = []; /** Reads only Git metadata through the production shell-free transport. */ - public function capture(array $arguments, string $directory): string + public function capture(array $arguments, string $directory, float $timeout = 120): string { - return (new NativeProcessRunner())->capture($arguments, $directory); + return (new NativeProcessRunner())->capture($arguments, $directory, $timeout); } /** Returns a synthetic style failure to verify end-to-end exit-code propagation. */ - public function run(array $arguments, string $directory): int + public function run(array $arguments, string $directory, float $timeout = 300): int { $this->commands[] = $arguments; diff --git a/tests/Standards/StandardsRunnerTest.php b/tests/Standards/StandardsRunnerTest.php index c024387..91af4c8 100644 --- a/tests/Standards/StandardsRunnerTest.php +++ b/tests/Standards/StandardsRunnerTest.php @@ -71,13 +71,13 @@ public function __construct( ) {} /** Fails if standards orchestration unexpectedly performs a capture operation. */ - public function capture(array $arguments, string $directory): string + public function capture(array $arguments, string $directory, float $timeout = 120): string { throw new \LogicException('Not expected.'); } /** Records argument boundaries and returns the selected synthetic tool status. */ - public function run(array $arguments, string $directory): int + public function run(array $arguments, string $directory, float $timeout = 300): int { $this->commands[] = $arguments; From 5700df9274540bdfb6b03c42a40b223f25223b89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Say=C3=A3o=20Lobato=20Abreu?= Date: Tue, 27 Oct 2026 00:38:57 -0300 Subject: [PATCH 7/7] test: accept wrapped timeout diagnostics across terminal widths --- tests/Refactoring/RefactorCommandTest.php | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/tests/Refactoring/RefactorCommandTest.php b/tests/Refactoring/RefactorCommandTest.php index 88e0537..96f4d7f 100644 --- a/tests/Refactoring/RefactorCommandTest.php +++ b/tests/Refactoring/RefactorCommandTest.php @@ -70,8 +70,17 @@ public function testConfigurationSubprocessesDoNotReceiveToolCredentials(): void } } - /** An actual short subprocess timeout is an execution failure, with the preflight cleaned on the same exception path. */ - public function testTimeoutIsFailureAndCleansThePreflight(): void + /** @return iterable Widths exercise narrow, hosted-CI and wide rendering without private terminal state. */ + public static function timeoutTerminalWidths(): iterable + { + yield 'narrow' => [20]; + yield 'hosted CI' => [80]; + yield 'wide' => [160]; + } + + /** An actual short subprocess timeout remains a failure with its complete diagnostic and owned preflight cleanup. */ + #[DataProvider('timeoutTerminalWidths')] + public function testTimeoutIsFailureAndCleansThePreflight(int $columns): void { $native = new Process([PHP_BINARY, '-r', 'usleep(250000);'], timeout: 0.01); try { @@ -93,15 +102,19 @@ public function testTimeoutIsFailureAndCleansThePreflight(): void $application->addCommand(new RefactorCommand(new ProjectScopeResolver('/unused-default'), new RectorRunner($process, '/tool/rector'))); $tester = new ApplicationTester($application); + $previousColumns = getenv('COLUMNS'); try { + putenv('COLUMNS=' . $columns); self::assertSame(1, $tester->run(['command' => 'refactor', '--config' => $config, '--directory' => realpath(sys_get_temp_dir())])); - self::assertStringContainsString('exceeded the timeout', $tester->getDisplay()); + self::assertMatchesRegularExpression('/exceeded\s+the\s+timeout\s+of\s+0\.01\s+seconds\./', $tester->getDisplay()); self::assertIsString($preflight); self::assertFileDoesNotExist($preflight); self::assertDirectoryDoesNotExist(dirname($preflight)); } finally { + putenv(false === $previousColumns ? 'COLUMNS' : 'COLUMNS=' . $previousColumns); unlink($config); } + self::assertSame($previousColumns, getenv('COLUMNS')); } /** @return iterable Inputs that must never be coerced to a disabled or unintended limit. */