From 57ee5d433504f1af0c61846b4c6ba9474c5ef572 Mon Sep 17 00:00:00 2001 From: Abdelkarim Mateos Sanchez Date: Fri, 17 Jul 2026 11:38:13 +0200 Subject: [PATCH] chore(quality): Rector gate infrastructure, wave-based adoption (AID-528) Add rector/rector + driftingly/rector-laravel as a code-quality gate, following the wave-based pattern from openmizan (AID-520) but adapted to this repo's measured reality rather than copied. The gate lives inside `composer check-full`, which is what the `quality` job runs. That job is a required status check on main with strict mode, so a pending refactoring turns CI red and blocks the merge. The pre-commit hook does not call check-full, so the step is wired there explicitly too. Wave 0 carries exactly one rule. Three rules measured zero-diff over app/ in the pilot (RemoveDumpDataDeadCodeRector, ReadOnlyClassRector and the LARAVEL_TYPE_DECLARATIONS set), but only the first is proven to bite; a rule that reports nothing and has not been shown to detect anything is noise in the gate, not protection. ReadOnlyClassRector is additionally a forward risk here, since Filament/Livewire components mutate properties by design. Both are deferred to their own wave. DeclareStrictTypesRector is deliberately absent: openmizan's pint.json sets declare_strict_types so its app/ sits at 597/597 and the rule is an inert second lock, but this repo's pint.json does not and app/ sits at 109/182. Enabling it here is a semantic change over 73 files (runtime type coercion), not a cosmetic one, so it belongs to its own wave. The withSkip() entries are armed in advance, justified against this codebase: app/ holds 52 explicit `throw new` and zero `throw_if()`, so the explicit guard is the convention; app() and resolve() are tied at 2 each, making that rewrite pure churn. Verified: zero-diff over the untouched tree (rc=0); violation probe bites and is attributable (a new file with dd() yields rc=2 with Rector naming both the file and the rule); tree returns clean afterwards. check-full green, coverage 89.8%. Refs: AID-529, AID-530, AID-531, AID-532 Co-Authored-By: Claude Fable 5 --- composer.json | 15 ++- composer.lock | 98 ++++++++++++++- .../specs/2026-07-17-rector-laravel-design.md | 113 ++++++++++++++++++ rector.php | 61 ++++++++++ scripts/pre-commit | 20 +++- 5 files changed, 302 insertions(+), 5 deletions(-) create mode 100644 docs/superpowers/specs/2026-07-17-rector-laravel-design.md create mode 100644 rector.php diff --git a/composer.json b/composer.json index 5b791d1..ad589f4 100644 --- a/composer.json +++ b/composer.json @@ -22,6 +22,7 @@ }, "require-dev": { "barryvdh/laravel-debugbar": "^4.0", + "driftingly/rector-laravel": "^2.5", "fakerphp/faker": "^1.23", "infection/infection": "^0.32.0", "larastan/larastan": "^3.6", @@ -35,7 +36,8 @@ "pestphp/pest-plugin-browser": "^4.0", "pestphp/pest-plugin-laravel": "^4.0", "pestphp/pest-plugin-livewire": "^4.0", - "phpstan/phpstan": "^2.1" + "phpstan/phpstan": "^2.1", + "rector/rector": "^2.5" }, "autoload": { "psr-4": { @@ -88,6 +90,15 @@ "syntax-check": [ "bash ./scripts/check-syntax.sh" ], + "refactor": [ + "Composer\\Config::disableProcessTimeout", + "./vendor/bin/rector process", + "./vendor/bin/pint --dirty" + ], + "refactor:dry": [ + "Composer\\Config::disableProcessTimeout", + "./vendor/bin/rector process --dry-run" + ], "test": [ "@php artisan config:clear --ansi", "@php artisan test" @@ -116,6 +127,7 @@ "@validate-lock", "@syntax-check", "@pint-test", + "@refactor:dry", "@phpstan", "@test" ], @@ -123,6 +135,7 @@ "@validate-lock", "@syntax-check", "@pint-test", + "@refactor:dry", "@phpstan", "@test-coverage" ], diff --git a/composer.lock b/composer.lock index 7260b30..8afa20a 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": "d19acfc6961bac096ede3f0b8c93eaad", + "content-hash": "d3b212e3be291c8f448f9f6ce7a84233", "packages": [ { "name": "anourvalar/eloquent-serialize", @@ -11083,6 +11083,42 @@ }, "time": "2026-02-07T07:09:04+00:00" }, + { + "name": "driftingly/rector-laravel", + "version": "2.5.0", + "source": { + "type": "git", + "url": "https://github.com/driftingly/rector-laravel.git", + "reference": "0dbdd842dfdbc821cfe5f47ca7326e2502b2a107" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/driftingly/rector-laravel/zipball/0dbdd842dfdbc821cfe5f47ca7326e2502b2a107", + "reference": "0dbdd842dfdbc821cfe5f47ca7326e2502b2a107", + "shasum": "" + }, + "require": { + "php": "^7.4 || ^8.0", + "rector/rector": "^2.2.7", + "webmozart/assert": "^1.11 || ^2.0" + }, + "type": "rector-extension", + "autoload": { + "psr-4": { + "RectorLaravel\\": "src" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "description": "Rector upgrades rules for Laravel Framework", + "support": { + "issues": "https://github.com/driftingly/rector-laravel/issues", + "source": "https://github.com/driftingly/rector-laravel/tree/2.5.0" + }, + "time": "2026-06-02T16:22:14+00:00" + }, { "name": "ergebnis/agent-detector", "version": "1.2.0", @@ -15402,6 +15438,66 @@ ], "time": "2024-06-11T12:45:25+00:00" }, + { + "name": "rector/rector", + "version": "2.5.7", + "source": { + "type": "git", + "url": "https://github.com/rectorphp/rector.git", + "reference": "ba22f8c087848278fed6b4910d4cf1108096d8d3" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/rectorphp/rector/zipball/ba22f8c087848278fed6b4910d4cf1108096d8d3", + "reference": "ba22f8c087848278fed6b4910d4cf1108096d8d3", + "shasum": "" + }, + "require": { + "php": "^7.4|^8.0", + "phpstan/phpstan": "^2.2.2" + }, + "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.5.7" + }, + "funding": [ + { + "url": "https://github.com/tomasvotruba", + "type": "github" + } + ], + "time": "2026-07-13T15:24:18+00:00" + }, { "name": "revolt/event-loop", "version": "v1.0.9", diff --git a/docs/superpowers/specs/2026-07-17-rector-laravel-design.md b/docs/superpowers/specs/2026-07-17-rector-laravel-design.md new file mode 100644 index 0000000..28adb74 --- /dev/null +++ b/docs/superpowers/specs/2026-07-17-rector-laravel-design.md @@ -0,0 +1,113 @@ +# Rector adoption — gate infrastructure (PR 0) + +- **Ticket:** AID-528 +- **Date:** 2026-07-17 +- **Status:** approved, pending implementation + +## Goal + +Adopt `rector/rector` + `driftingly/rector-laravel` in unblock as a code-quality support tool, wired to a gate that actually blocks bad merges. Follow the wave-based adoption pattern proven in openmizan (AID-520), **adapted to unblock's measured reality** rather than copied. + +Wave-based means: PR 0 ships infrastructure only, with a rule set verified zero-diff. Each subsequent wave enables curated rules in its own mechanical PR, backed by the full suite. + +## Measured baseline (pilot, 2026-07-17) + +Run in an isolated worktree, zero writes to the repo. `rector/rector 2.5.7` + `driftingly/rector-laravel 2.5.0`, over `app/` (182 PHP files), target `PHP_83`: + +| Rule / Set | Files | +|---|---| +| `RemoveDumpDataDeadCodeRector` | **0** ← Wave 0 | +| `ReadOnlyClassRector` | 0 | +| `LARAVEL_TYPE_DECLARATIONS` | 0 | +| `deadCode` (50) | 3 | +| `LARAVEL_COLLECTION` | 4 | +| `codeQuality` (50) | 22 | +| `LARAVEL_CODE_QUALITY` | 28 | +| `AddOverrideAttributeToOverriddenMethodsRector` | 48 | +| `DeclareStrictTypesRector` | 73 | + +`DeclareStrictTypesRector`'s 73 is cross-checked independently: `app/` has 182 PHP files, 109 of which already carry `declare(strict_types=1)` — 182 − 109 = 73. + +## Why this is not a copy of openmizan's config + +Two premises behind openmizan's `rector.php` do not hold here, and both were measured: + +1. **`DeclareStrictTypesRector` is not zero-diff in unblock.** openmizan's `pint.json` sets `declare_strict_types: true`, so Pint owns strict-types insertion repo-wide and `app/` sits at 597/597 — the Rector rule there is an inert second lock. unblock's `pint.json` does not set it, and `app/` sits at 109/182. Enabling the same rule here would rewrite 73 files with a **semantic** change (runtime type coercion), not a cosmetic one. Deferred to its own wave. + +2. **unblock already has the enforcement openmizan lacks.** openmizan's `main` has no branch protection (its AID-527), which is why its `rector.php` documents enforcement as "ship-workflow convention — a red job does not block a merge by itself". unblock's `main` has `required_status_checks: ["quality"]` with `strict: true` (verified via `gh api`), and the `quality` job runs `composer check-full`. Placing the gate inside `check-full` therefore inherits real enforcement for free: a pending diff turns CI red and blocks the merge. + +## Design + +### `rector.php` + +```php +->withPaths([__DIR__.'/app']) +->withPhpVersion(PhpVersion::PHP_83) +->withRules([RemoveDumpDataDeadCodeRector::class]) +->withSkip([ + ThrowIfRector::class, + ThrowIfAndThrowUnlessExceptionsToUseClassStringRector::class, + AppToResolveRector::class, +]) +``` + +**Path scope.** `app/` only. `routes/`, `bootstrap/`, `config/` and `tests/` are wave candidates, not oversights. `database/` must never be added: migrations are append-only history. + +**PHP version.** `PHP_83` follows the `composer.json` floor (`^8.3|^8.4`). Bump both together so gate semantics never drift implicitly. The CI runtime is PHP 8.4; `withPhpVersion` sets the transformation target, not the runtime, so the two are independent by design. + +**Skips**, justified against unblock's own code rather than inherited: + +- `ThrowIfRector` — `app/` has 52 explicit `throw new` and **zero** `throw_if()`. The explicit guard *is* the convention here; `throw_if()` would also flatten guards and evaluate its exception argument eagerly even when nothing throws. +- `ThrowIfAndThrowUnlessExceptionsToUseClassStringRector` — inert today (no `throw_if()` exists), armed in advance so the decision lives in config rather than in remembered prose. Rector warns such rules are "never registered" until a set enables them; that warning is accepted deliberately. +- `AppToResolveRector` — `app()` 2 vs `resolve()` 2. Naming churn with no semantic value. + +### Wave 0 carries exactly one rule + +Three rules measured zero-diff, but only `RemoveDumpDataDeadCodeRector` ships. The other two are excluded on purpose: + +- Neither has been proven to bite. A rule that reports nothing and has not been shown to detect anything is noise in the gate, not protection. +- `ReadOnlyClassRector` is a forward risk in a Filament/Livewire codebase, where components mutate properties by design. + +`RemoveDumpDataDeadCodeRector` earns its place: it stops a forgotten `dd()` from reaching main, and it is proven to catch one. + +### Gate wiring + +``` +"refactor": rector process && pint --dirty # write path +"refactor:dry": rector process --dry-run # gate path + +"check" += @refactor:dry +"check-full" += @refactor:dry # job `quality` (required + strict) => red blocks merge +scripts/pre-commit += rector --dry-run step +``` + +The pre-commit hook does **not** call `check-full` — it runs its own steps — so the gate must be added there explicitly or the hook will not inherit it. The hook runs Rector over all of `app/`, matching how Pint and PHPStan already behave in that hook; at 182 files the cost is negligible next to the test run it already performs. + +Ordering inside `check-full`: after `@pint-test`, before `@phpstan`. Style is settled first, so a Rector failure is never a formatting artifact. + +## Verification + +**Expected code diff: 0 files.** `composer refactor:dry` must exit 0 on the untouched tree. + +**Violation probe — the gate must be proven to bite, or it is decorative:** + +1. Clean baseline → `rc=0`. +2. Add a new isolated file containing a `dd()` → `rc=2`, and Rector must **name the file and the rule** (`app/ProbeVictim.php:6` / `* RemoveDumpDataDeadCodeRector`). +3. Remove the probe → `rc=0`, tree clean. + +The probe must be attributable. During the pilot, a first attempt reported a false "bites" result: the script had corrupted a file with stray whitespace and the `rc=2` came from that, not from the `dd()` — the injection had in fact failed. Asserting on the exit code alone is insufficient; assert that Rector names the file and the rule, and confirm the tree is clean afterwards. + +**Full gate:** `composer check-full` green before opening the PR. + +## Waves (separate tickets) + +| Wave | Content | Files | Nature | +|---|---|---|---| +| 1 | `strict_types` + `pint.json` | 73 in `app/` | **Semantic** — changes runtime coercion | +| 2 | `AddOverrideAttribute` | 48 | Mechanical | +| 3 | `LARAVEL_CODE_QUALITY` + `codeQuality` 50 | 28 + 22 | Mechanical | +| 4 | `LARAVEL_COLLECTION` + `deadCode` 50 | 4 + 3 | Manual review of public signatures | + +Wave 1 carries a scope trap worth recording now: unblock's `pint.json` excludes `database`, `elk`, `config` and `notas`, but **not** `tests/`. Setting `declare_strict_types: true` in Pint would therefore rewrite the 175 files under `tests/` and `routes/` as well, not just the 73 under `app/`. Size that wave before starting it. + +Each wave must re-prove its new rules bite before landing. diff --git a/rector.php b/rector.php new file mode 100644 index 0000000..59195f4 --- /dev/null +++ b/rector.php @@ -0,0 +1,61 @@ +withPaths([ + __DIR__.'/app', + ]) + // Follows the composer.json "php" floor (^8.3|^8.4). Bump together with the + // composer constraint so gate semantics never drift implicitly. This sets + // the transformation target, not the runtime — CI runs PHP 8.4. + ->withPhpVersion(PhpVersion::PHP_83) + ->withRules([ + RemoveDumpDataDeadCodeRector::class, + ]) + ->withSkip([ + // Excluded by design decision (AID-528), armed BEFORE any wave enables + // a set that would register them — config, not remembered prose. + // Rector warns these rules are "never registered" until such a set + // lands; that warning is accepted deliberately. + // + // app/ holds 52 explicit `throw new` and zero `throw_if()`: the + // explicit guard is the convention. throw_if() also flattens guards and + // evaluates its exception argument eagerly even when nothing is thrown. + ThrowIfRector::class, + ThrowIfAndThrowUnlessExceptionsToUseClassStringRector::class, + // app() vs resolve() is naming churn with no semantic value (2 vs 2). + AppToResolveRector::class, + ]); diff --git a/scripts/pre-commit b/scripts/pre-commit index e961ad9..63b9f78 100755 --- a/scripts/pre-commit +++ b/scripts/pre-commit @@ -1,7 +1,7 @@ #!/bin/bash # Pre-commit hook for unblock project # Install: composer run hook:install -# Checks: lock file sync, syntax, lint, PHPStan, tests +# Checks: lock file sync, syntax, lint, Rector, PHPStan, tests set -e @@ -91,7 +91,21 @@ else pass "No PHP files to lint" fi -# ── 4. PHPStan static analysis ────────────────────────────────────── +# ── 4. Rector (refactoring gate) ──────────────────────────────────── +# +# Runs over all of app/, matching how Pint and PHPStan already behave here. +# Rules live in rector.php and are enabled in curated waves (AID-528). +# `composer refactor` applies the pending changes. + +echo "Checking pending refactorings..." + +vendor/bin/rector process --dry-run --no-progress-bar --no-diffs > /dev/null 2>&1 || { + fail "Rector found pending refactorings. Run 'composer refactor:dry' to review, 'composer refactor' to apply." +} + +pass "Rector OK" + +# ── 5. PHPStan static analysis ────────────────────────────────────── echo "Running static analysis..." @@ -101,7 +115,7 @@ vendor/bin/phpstan analyse --memory-limit=1G --no-progress --quiet 2>/dev/null | pass "PHPStan OK" -# ── 5. Tests ───────────────────────────────────────────────────────── +# ── 6. Tests ───────────────────────────────────────────────────────── echo "Running tests..."