The migration Rector wrote and we rewrote

An automated refactor produced a diff of eight thousand lines across four hundred and twelve files, in one pull request, with three genuinely wrong changes in it. Approving that is not review, and rejecting it means doing the work by hand.

The symptom

$ vendor/bin/rector process src --dry-run
  412 files would be changed

$ vendor/bin/rector process src
$ git diff --stat | tail -1
 412 files changed, 4,102 insertions(+), 3,908 deletions(-)

$ git diff | wc -l
8218

the review, honestly: four hours to read 8,218 lines at
review pace, against about twenty minutes of attention
available for a diff that is 95% mechanical.

and hunk 3,881 changed behaviour.

Why it happens

A tool applies a rule uniformly and every rule has exceptions the rule cannot see. The output is correct in proportion to how well the rule matches the codebase, and the exceptions are distributed randomly through a diff that is otherwise noise.

The fix

Splitting the run by rule

// rector-01-imports.php
return RectorConfig::configure()
    ->withPaths([__DIR__ . '/src'])
    ->withRules([
        RemoveUnusedImportsRector::class,
    ]);

// rector-02-null-coalesce.php, rector-03-attributes.php, ...
// nine configurations, nine commits

the same 412 files, split:

  rule                          files   lines   review
  unused imports                  188     412   glance
  docblock to native type          94   1,208   sample
  ternary to null coalesce         41     188   READ
  annotations to attributes       144   2,102   sample
  short closures                   88     404   glance
  ...

nine commits, each reviewable on its own terms.

Splitting by rule is the whole technique and it takes ten minutes to set up. The value is not that each diff is smaller — it is that each diff has one failure mode, so the reviewer knows what to look for rather than reading everything with equal attention.

Which rules are safe to accept unreviewed

the question per rule: what does a WRONG application
look like?

  unused imports   a parse error or an undefined class,
    caught by the analyser → accept, verify with phpstan
  short closures   syntactic only; a wrong one does not
    compile → accept
  docblock to native type   a narrower type than the code
    accepts, so a TypeError on a rare path → SAMPLE
  ternary to null coalesce   a SILENT behaviour change
    when the operand is falsy-but-set → READ EVERY HUNK

Classifying by failure mode rather than by diff size is what makes this tractable, and the answer is usually that two or three rules need real attention and the rest do not. The classification is a judgement about the rule, made once, and it is the part worth writing down.

The rule that changed behaviour

// what Rector produced
$value = $this->cache->get($key) ?? $this->compute($key);

// what it replaced
$value = isset($cached) ? $cached : $this->compute($key);

// which is fine — except the original was
$cached = $this->cache->get($key);
$value = $cached !== false ? $cached : $this->compute($key);

// the cache returns FALSE on a miss and NULL is a
// legitimately cached value. the rewrite recomputes on
// every cached null, and there were 41,000 a day.

The rewrite is a correct application of the rule and is wrong for this code, because ?? tests for null and the original tested for false. Nothing failed — the values were recomputed and were the same — so the only symptom was a cache hit rate that dropped from ninety-one per cent to sixty-two and nobody was watching that graph.

Sampling, and what sample size means

for a rule classified as "sample":

144 files. reading 30 at random and finding nothing puts
the upper bound at roughly 10% wrong, at 95% confidence —
which is 14 files. is that acceptable?

  annotations-to-attributes  yes. a wrong one is a test
    that does not run, and the test count is asserted.
  docblock-to-native-type    no. a wrong one is a
    TypeError on an uncommon path → applied, then checked
    by the analyser at level 9, which reads every file.

The useful move is noticing when a tool can replace the sampling entirely. A static analyser reads all four hundred files without getting tired, so for any rule whose failures are type errors the review is “run the analyser” rather than “read thirty files” — and that is a stronger guarantee than the full manual review would have been.

A behavioural diff

// captured against the pre-refactor code
$baseline = [];

foreach ($this->corpus() as $case) {
    $baseline[$case->id] = $this->runIsolated($case->input);
}

// and asserted after
public function testBehaviourIsUnchanged(): void
{
    foreach ($this->corpus() as $case) {
        self::assertEquals(
            $this->baseline[$case->id],
            $this->runIsolated($case->input),
            "case {$case->id}",
        );
    }
}

Four thousand cases captured from a week of production traffic, with identifiers stripped, is a stronger assertion than the test suite and took an afternoon to build. It caught the null-coalescing change — not as a wrong value, but as a case that took forty times longer, which the harness records alongside the result.

Verifying it worked

$ git log --oneline HEAD~9..HEAD
  9c1f4a7 rector: short closures
  8b2d0e6 rector: annotations to attributes
  7a3f5c7 rector: ternary to null coalesce (3 hunks reverted)
  ...

$ vendor/bin/phpstan analyse --level=9
 [OK] No errors

$ vendor/bin/phpunit
  Tests: 1,414 passed

$ ./bin/behaviour-diff --baseline=pre-refactor.json
  4,102 cases, 4,102 identical

# and a fortnight of production
  cache hit rate  91.4%    # unchanged
  error rate      unchanged

The cache hit rate is the metric that would have caught the bug if it had shipped, and watching it for a fortnight afterwards is the confirmation. Three hunks reverted out of eight thousand lines is the actual defect rate, and it is small enough to be invisible and large enough to matter.

What this costs

A habit of trusting the tool, which is the thing that will bite. Nine commits reviewed carefully this time establishes that the process works, and the next refactor will be one commit because the process worked last time — and the classification by failure mode is the part that gets skipped first.

The behavioural corpus is also a fixture with production data in it, stripped of identifiers, which is a thing to be careful about. It lives in the repository, it is four megabytes, and the stripping was done by a script that somebody has to trust — a field added to the input shape later would be captured without anybody re-examining that decision.