Skip to content
This repository was archived by the owner on Aug 23, 2026. It is now read-only.

chore(quality): Rector gate infrastructure, wave-based adoption (AID-528) - #186

Merged
abkrim merged 1 commit into
mainfrom
abdelkarim/aid-528-unblock-rector-pr0-gate
Jul 17, 2026
Merged

abkrim merged 1 commit into
mainfrom
abdelkarim/aid-528-unblock-rector-pr0-gate

Conversation

@abkrim

@abkrim abkrim commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Adds 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.

Code diff: 0 files. This PR is infrastructure only.

Where the gate bites

Inside composer check-full, which is what the quality job runs. That job is a required status check on main with strict: true (verified via gh api), so a pending refactoring turns CI red and blocks the merge — no reliance on ship-workflow convention.

This is the main divergence from openmizan, which has no branch protection (its AID-527) and therefore documents its gate as advisory.

Wave 0 carries exactly one rule

The pilot (isolated worktree, zero writes) measured three rules at zero-diff over app/, but only one ships:

Rule / Set Files Wave 0?
RemoveDumpDataDeadCodeRector 0 ✅ zero-diff and proven to bite
ReadOnlyClassRector 0 ❌ not proven to bite; forward risk (Filament/Livewire mutate by design)
LARAVEL_TYPE_DECLARATIONS 0 ❌ not proven to bite
deadCode (50) 3 wave 4
LARAVEL_COLLECTION 4 wave 4
codeQuality (50) 22 wave 3
LARAVEL_CODE_QUALITY 28 wave 3
AddOverrideAttribute 48 wave 2
DeclareStrictTypesRector 73 wave 1

Reporting 0 does not prove a rule detects anything. A rule that has not been shown to bite is noise in the gate, not protection.

Why DeclareStrictTypesRector is absent

openmizan's pint.json sets declare_strict_types, so Pint owns the convention repo-wide and its app/ sits at 597/597 — the rule there is an inert second lock. This repo's pint.json does not set it, and app/ sits at 109/182. Enabling it here rewrites 73 files with a semantic change (runtime type coercion), not a cosmetic one. Deferred to AID-529.

Cross-checked two ways: Rector reports 73; 182 PHP files − 109 already carrying the declare = 73.

Skips, justified against this codebase

  • ThrowIfRector / ThrowIfAndThrowUnless... — app/ holds 52 explicit throw new and zero throw_if(). The explicit guard is the convention.
  • AppToResolveRector — app() 2 vs resolve() 2. Pure churn.

Armed in advance so the decision lives in config rather than prose. Rector warns they are "never registered" until a wave enables a set that would register them; accepted deliberately.

Verification

  • Zero-diff over the untouched tree → rc=0
  • Violation probe, attributable → a new file with dd() yields rc=2 and Rector names both the file and the rule (app/ProbeVictim.php:6 / * RemoveDumpDataDeadCodeRector)
  • Returns clean after removing the probe → rc=0, no residue
  • composer check-full green, coverage 89.8%

Known limitation, out of scope here

The scripts/pre-commit step is wired but inert on this machine: core.hooksPath points at ~/.config/git/hooks (a global gitleaks guardrail), so Git ignores .git/hooks/ entirely and the repo's hook never runs — composer hook:install has been a no-op since that global config landed. This predates the PR and affects every repo with local hooks, so it is filed separately rather than fixed here. The CI gate is unaffected and is the one that enforces.

Refs: AID-529, AID-530, AID-531, AID-532

…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 <noreply@anthropic.com>
@abkrim
abkrim merged commit ea42fd6 into main Jul 17, 2026
2 checks passed
@abkrim
abkrim deleted the abdelkarim/aid-528-unblock-rector-pr0-gate branch July 17, 2026 09:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant