The Migration Review Checklist

Part II · Module 28 · Chapter 28.9 · The Migration Review Checklist

1. What a Migration Review Is For

An ordinary code review asks whether the new code is correct, clear and well tested. Those questions still apply to a migration, but they are not the ones that decide whether it is safe to land. The new code can be excellent and the migration still break the flow. Chapter 28.3's rewrite was good Python and behaved differently in eleven of eleven scenarios.

A migration review asks a different question: will anyone who depends on the old script notice the switch? And if they will, has that been decided, announced and made reversible? The answer is almost never in the diff. It is in the artefacts the earlier chapters of this module produce:

• the reading record and inventory (Chapters 28.1 and 28.2);
• the strategy and its reasons (Chapter 28.4);
• the shadow log for an incremental conversion (Chapter 28.5);
• the validation corpus and the harness transcript (Chapter 28.8).

The checklist is a way of asking for those artefacts and checking that they say what they need to say.

2. The Checklist

Eight items can be checked by a script, because each is a property of the artefacts. Four need a person. Each item is traced to the chapter whose failure it prevents.

The split matters. The mechanical checks are cheap, so run them on every migration and never argue about them. The manual ones are where the reviewer's time should go, because the mechanical ones have already cleared away the easy failures.

3. The Summary a Migration Carries

A script can only check what is written down in a fixed form. This chapter proposes a convention: every migration merge request carries a short MIGRATION summary, partly produced by the tools of Chapters 28.2 and 28.8, in a line-oriented format. It is a convention for this module, not an industry standard. Any format works if it states the same facts. Here is the summary for the migration of sta_summary.pl:

# MIGRATION summary: sta_summary.pl -> tools/sta_summary.py
owner: sample.owner (one named person)
rollback: make signoff STA_SUMMARY="perl tools/sta_summary.pl"
inventory: B1 B2 B3 B4 B5 B6 B7 B8 B9 B10 B11 B12
case night_0911     covers B1 B4 B5 B6 B12
case night_0912     covers B1 B6 B10 B12
case night_0913     covers B1 B4 B6 B12
case verbose        covers B2
case opt_after_dir  covers B2 B11
case upper_V        covers B2 B11
case unknown_opt    covers B11
case opt_with_value covers B11
case empty_dir      covers B7
case missing_dir    covers B8
case dir_named_0    covers B9
case unreadable     covers B10
case 256_corners    covers B12
case no_argument    covers B3
validated night_0911 same
validated night_0912 same
validated night_0913 same
validated verbose same
validated opt_after_dir same
validated upper_V same
validated unknown_opt same
validated opt_with_value same
validated empty_dir same
validated missing_dir same
validated dir_named_0 same
validated unreadable same
validated 256_corners same
validated no_argument same
channels: stdout stderr exit files
changed: flow/Makefile (1 line)
added: tools/sta_summary.py

Every line is a claim a reviewer can check against an artefact. The inventory: line names the twelve rows of Chapter 28.2. Each case ... covers line maps a case from Chapter 28.8's cases.list to the rows it exercises. Each validated line is a result from the harness. channels: states what the harness compared. And changed:, added: and deleted: describe the commit.

The case mapping is where most of the thinking goes. night_0911 covers B1, B4, B5, B6 and B12, because a normal night with one clean corner prints the summary, shows the 0.000 WNS, writes tmp.csv with empty fields, and exits with a count. Writing the mapping is itself a review step: it forces the question "which row does this case exist for?", and a case with no answer is not earning its place in the corpus.

4. The Checker

use strict;
use warnings;

# review_migration.pl - the mechanical half of the migration review.
# usage: perl review_migration.pl SUBMISSION...
# A SUBMISSION is the MIGRATION summary a migration merge request carries
# (format below). Each check prints PASS or FAIL with its reason; the items
# a script cannot judge are listed once at the end as MANUAL.
#
#   owner: NAME                        rollback: COMMAND
#   inventory: B1 B2 ...               case NAME covers B.. B..
#   validated NAME same|DIFF           channels: stdout stderr exit files
#   changed: PATH (N lines)            deleted: PATH
#   decision: Bn OWNER: TEXT
my @CHANNELS = qw(stdout stderr exit files);

sub load {
    my ($file) = @_;
    my %s = (rows => [], cases => {}, validated => {}, changed => [],
             deleted => [], decision => {});
    open my $fh, '<', $file or die "cannot read $file: $!\n";
    while (my $l = <$fh>) {
        next if $l =~ /^\s*(?:#|$)/;
        if    ($l =~ /^(owner|rollback|channels):\s*(.*?)\s*$/) { $s{$1} = $2 }
        elsif ($l =~ /^inventory:\s*(.*)/)              { $s{rows} = [split ' ', $1] }
        elsif ($l =~ /^case\s+(\S+)\s+covers\s+(.*)/)   { $s{cases}{$1} = [split ' ', $2] }
        elsif ($l =~ /^validated\s+(\S+)\s+(\S+)/)       { $s{validated}{$1} = $2 }
        elsif ($l =~ /^changed:\s*(\S+)\s+\((\d+) lines?\)/) { push @{ $s{changed} }, [$1, $2] }
        elsif ($l =~ /^deleted:\s*(\S+)/)                { push @{ $s{deleted} }, $1 }
        elsif ($l =~ /^decision:\s*(B\d+)\s+(\S+):/)     { $s{decision}{$1} = $2 }
    }
    return \%s;
}

my @CHECKS = (
  [ 'inventory exists' => sub {
        my $s = shift;
        @{ $s->{rows} } ? '' : 'no inventory rows listed' } ],
  [ 'every row has a case' => sub {
        my $s = shift;
        my %hit = map { $_ => 1 } map { @$_ } values %{ $s->{cases} };
        my @miss = grep { !$hit{$_} } @{ $s->{rows} };
        @miss ? "no case covers @miss" : '' } ],
  [ 'every case was validated' => sub {
        my $s = shift;
        my @all  = sort keys %{ $s->{cases} };
        my @miss = grep { !exists $s->{validated}{$_} } @all;
        return '' unless @miss;
        my @show = @miss > 3 ? (@miss[0 .. 2], '...') : @miss;
        return sprintf "%d of %d cases never ran: %s", scalar @miss, scalar @all, "@show";
    } ],
  [ 'all four channels compared' => sub {
        my $s = shift;
        my %got = map { $_ => 1 } split ' ', ($s->{channels} // '');
        my @miss = grep { !$got{$_} } @CHANNELS;
        @miss ? "not compared: @miss" : '' } ],
  [ 'every difference decided' => sub {
        my $s = shift;
        my @diff = grep { $s->{validated}{$_} ne 'same' } sort keys %{ $s->{validated} };
        my @undecided = grep {
            my $case = $_;
            !grep { exists $s->{decision}{$_} } @{ $s->{cases}{$case} || [] }
        } @diff;
        @undecided ? "DIFF with no recorded decision: @undecided" : '' } ],
  [ 'one revertible change' => sub {
        my $s = shift;
        my @c = @{ $s->{changed} };
        my $lines = 0; $lines += $_->[1] for @c;
        @c > 1 || $lines > 1 ? scalar(@c) . " existing files changed, $lines lines" : '' } ],
  [ 'old implementation kept' => sub {
        my $s = shift;
        my @d = grep { /\.pl$/ } @{ $s->{deleted} };
        my $rb = $s->{rollback} // '';
        @d ? "deletes @d" : $rb eq '' ? 'no rollback command' : '' } ],
  [ 'a named owner' => sub {
        my $s = shift;
        my $o = $s->{owner} // '';
        $o eq '' || $o =~ /^(?:team|tbd|n\/a|everyone)$/i ? "owner is '" . ($o || 'missing') . "'" : '' } ],
);

for my $file (@ARGV) {
    my $s = load($file);
    my $fails = 0;
    print "== $file\n";
    for my $c (@CHECKS) {
        my ($name, $test) = @$c;
        my $why = $test->($s);
        $fails++ if $why;
        printf "  %-4s %-28s %s\n", $why ? 'FAIL' : 'PASS', $name, $why;
    }
    printf "  => %s\n", $fails ? "NOT READY ($fails of " . scalar(@CHECKS) . " checks fail)" : 'ready for the manual items';
}
print <<'MANUAL';

MANUAL (a person must judge these; no script can):
  - is the inventory complete, or only what the author thought of?
  - are the "not probed" limits written down (timing, environment, concurrency)?
  - is every recorded decision agreed by the owner of the affected consumer?
  - has the rollback command actually been run once, before the switch?
MANUAL

Each check is a small function that returns an empty string for a pass and a reason for a failure. The reason matters as much as the verdict: "no case covers B3" tells the author exactly what to write next, where "coverage check failed" would not.

Three of the checks deserve a closer look.

Check 3 compares cases listed against cases validated. A summary can list fourteen cases and validate three. The check counts both and names the missing ones.

Check 5 accepts a difference if it is decided. A DIFF is not a failure in itself. It is a failure when no decision: line covers any inventory row that the differing case exercises. Section 9 shows a migration with two decided differences that passes.

Check 6 counts changed existing files and lines. New files (the port itself) are not counted. The switch should be one line in one existing file, so that undoing it is one line too.

5. Six Submissions

make_specimens.sh derives five more submissions from ready.mr, each by the edits that define it, so the defect under review is exactly what the edit shows:

#!/bin/bash
# make_specimens.sh - five more submissions, each derived from ready.mr by
# the edits that define it. The edit IS the defect under review.
mkdir -p mr
cp ready.mr mr/ready.mr

# the Chapter 28.8 work as it stood: nobody wrote a case for B3
grep -v 'no_argument' ready.mr > mr/as_built.mr

# "validated against six weeks of production runs"
grep -v '^validated' ready.mr > mr/prod_only.mr
grep '^validated night_' ready.mr >> mr/prod_only.mr

# every case "same" -- because the harness compared two channels
sed 's/^channels: .*/channels: stdout files/' ready.mr > mr/exit_blind.mr

# port, consumer edits and deletion in one commit; a fix nobody recorded
sed -e 's/^owner: .*/owner: team/' \
    -e 's/^rollback: .*/rollback: git revert/' \
    -e 's/^validated night_0911 same/validated night_0911 DIFF/' ready.mr > mr/big_bang.mr
printf '%s\n' 'changed: flow/bin/post_wns.sh (3 lines)' \
              'changed: flow/bin/weekly_digest.pl (2 lines)' \
              'deleted: tools/sta_summary.pl' >> mr/big_bang.mr

# a deliberate fix of B4, recorded before the switch
sed -e 's/^validated night_0911 same/validated night_0911 DIFF/' \
    -e 's/^validated night_0913 same/validated night_0913 DIFF/' ready.mr > mr/decided.mr
echo 'decision: B4 dashboard-owner: clean corners report their true WNS from the switch on' >> mr/decided.mr

#!/bin/bash
bash make_specimens.sh
perl review_migration.pl mr/*.mr

== mr/as_built.mr
  PASS inventory exists             
  FAIL every row has a case         no case covers B3
  PASS every case was validated     
  PASS all four channels compared   
  PASS every difference decided     
  PASS one revertible change        
  PASS old implementation kept      
  PASS a named owner                
  => NOT READY (1 of 8 checks fail)
== mr/big_bang.mr
  PASS inventory exists             
  PASS every row has a case         
  PASS every case was validated     
  PASS all four channels compared   
  FAIL every difference decided     DIFF with no recorded decision: night_0911
  FAIL one revertible change        3 existing files changed, 6 lines
  FAIL old implementation kept      deletes tools/sta_summary.pl
  FAIL a named owner                owner is 'team'
  => NOT READY (4 of 8 checks fail)
== mr/decided.mr
  PASS inventory exists             
  PASS every row has a case         
  PASS every case was validated     
  PASS all four channels compared   
  PASS every difference decided     
  PASS one revertible change        
  PASS old implementation kept      
  PASS a named owner                
  => ready for the manual items
== mr/exit_blind.mr
  PASS inventory exists             
  PASS every row has a case         
  PASS every case was validated     
  FAIL all four channels compared   not compared: stderr exit
  PASS every difference decided     
  PASS one revertible change        
  PASS old implementation kept      
  PASS a named owner                
  => NOT READY (1 of 8 checks fail)
== mr/prod_only.mr
  PASS inventory exists             
  PASS every row has a case         
  FAIL every case was validated     11 of 14 cases never ran: 256_corners dir_named_0 empty_dir ...
  PASS all four channels compared   
  PASS every difference decided     
  PASS one revertible change        
  PASS old implementation kept      
  PASS a named owner                
  => NOT READY (1 of 8 checks fail)
== mr/ready.mr
  PASS inventory exists             
  PASS every row has a case         
  PASS every case was validated     
  PASS all four channels compared   
  PASS every difference decided     
  PASS one revertible change        
  PASS old implementation kept      
  PASS a named owner                
  => ready for the manual items

MANUAL (a person must judge these; no script can):
  - is the inventory complete, or only what the author thought of?
  - are the "not probed" limits written down (timing, environment, concurrency)?
  - is every recorded decision agreed by the owner of the affected consumer?
  - has the rollback command actually been run once, before the switch?

Two submissions are ready (ready and decided). Four are not, and each fails in a different way. The next four sections take them one at a time.

6. The Review Found a Gap in This Module

as_built.mr is the migration exactly as Chapter 28.8 left it: thirteen cases, all same, all four channels compared, one line changed, the Perl kept, an owner named. It fails one check: no case covers B3, the behaviour that with no argument the script reads the current directory.

That is not an invented defect. Chapter 28.8's cases.list really does have no no-argument case, and the gap went unnoticed while that chapter was written, until the mapping in section 3 made every row ask for its case. A no_argument case was then added and run through the same harness, and it passed:

  same  no_argument

So the port was right and the evidence was incomplete, which is exactly the state a reviewer cannot tell apart from a wrong port without this check. ready.mr is as_built.mr plus that one case.

This is the checklist doing its real job. It did not find a bug in the code. It found a behaviour that nothing had tested, which is where the next bug would have been.

7. The Passing Submission That Compared Two Channels

exit_blind.mr is the specimen to remember. Every one of its fourteen cases is same. A reviewer skimming it sees a clean validation: validated ... same, fourteen times.

It fails check 4: not compared: stderr exit. Its harness compared STDOUT and the files and nothing else, so same means "same on two channels". Run Chapter 28.8's first port through this harness and the missing_dir case comes out same, although the port got both its message and its exit status wrong there (1 instead of 2): both differences are on the two channels this harness never looked at.

The lesson is about what same means. A validation result is only as strong as the list of things it compared, and that list has to be stated in the summary, because the word "same" does not carry it. A harness that cannot report what it compared is not evidence.

8. The Big-Bang Submission

big_bang.mr fails four checks, and they all come from one decision: to do the whole migration in one commit.

• "3 existing files changed, 6 lines". The Makefile switch, plus edits to post_wns.sh and weekly_digest.pl to adapt them to the new output. Each edit may be correct, but now the switch cannot be undone without undoing all three.
• "deletes tools/sta_summary.pl". There is nothing to roll back to. rollback: git revert is a command, but it reverts the consumer edits along with the switch, and restores a Perl script that the flow no longer calls in the same way.
• "DIFF with no recorded decision: night_0911". Somebody fixed B4 (the clean corner's WNS) during the port. It may well be right, but no row says so, and the dashboard owner has not been asked.
• "owner is 'team'". When the dashboard shows something strange at 2 am, nobody is the person to call.

Each of those is reasonable on its own, and together they make a change that cannot be reviewed or reversed. The fix is the sequence this module teaches: switch first with everything else unchanged, fix the defects one at a time with a recorded decision each, adapt the consumers in their own commits, and delete the Perl later, on purpose (Chapter 28.10).

9. A Difference Is Fine When It Is Decided

decided.mr has two cases that differ, night_0911 and night_0913, and it passes all eight checks. The differences come from a deliberate fix of B4, and the summary carries the line that makes it reviewable:

decision: B4 dashboard-owner: clean corners report their true WNS from the switch on

Check 5 sees that both differing cases exercise B4 and that B4 has a decision, and passes. That is the right outcome. The checklist does not demand that nothing change. It demands that everything that changes was meant to, and that the person on the receiving end is named. Whether the dashboard owner actually agreed is manual question M3. The script can check that a decision is recorded, not that it was agreed.

10. The Manual Half

The checker prints four questions at the end because no script can answer them. They are where a reviewer earns their place.

M1: Is the inventory complete? The checker can see that every row has a case, not that every behaviour has a row. Read the reading record and the inventory side by side, and ask the author which branches of the original they probed and how.

M2: Are the limits written down? Chapter 28.8 listed what "same" does not prove: timing, environment, concurrency, inputs not tried. The summary should say which of those matter for this script and what was done about them, even if the answer is "nothing, and here is why".

M3: Has each decision been agreed by the affected owner? A decision: line names a person. Ask them.

M4: Has the rollback been run once? A rollback command that has only been written is a guess. Run it in a scratch run directory before approving, and see the old output come back. Chapter 28.10 does exactly that.

11. Mistakes in Migration Review

Reviewing the diff. The diff of a well-planned migration is one new file and one changed line. Everything that matters is in the artefacts.

Reading "same" without reading "channels". Measured: fourteen cases same with two channels compared.

Accepting "validated on production". That is a statement about which inputs ran, and production inputs exercised only 3 of the 14 cases.

Treating every difference as a failure. A decided, announced difference is a planned change. An undecided one is the problem.

Approving one commit that does everything. It fails on rollback whether or not anything goes wrong.

Letting "team" own it. Ownership is a person who will answer the phone.

12. Exercises

1. Break the checker. Write a seventh specimen that is unsafe to land but passes all eight mechanical checks. (Hints: a case line that claims to cover a row it does not exercise; a rollback: command for a target that does not exist.) Which manual question would catch it?

2. A ninth check. Add a check that every decision: names a row that is actually in the inventory, and another that the summary states its limits (limits: lines). Run all six specimens again.

3. Map your own corpus. Take Chapter 28.8's cases.list and write the covers mapping for it yourself before reading section 3's version. Do you also find that nothing covers B3? Is there any row you would map differently?

4. Review a real migration. Find a migration in your own repository's history (any language change, or a rewrite of a tool). Write its MIGRATION summary after the fact from whatever artefacts exist, and run the checker on it. Which checks fail, and did any of them correspond to a problem that actually happened after the switch?

5. The incremental version. Chapter 28.5 converted one function at a time. Design the summary lines a function-level conversion should carry: the seam, the shadow log's period and its disagreement count. Add checks for them.

13. What To Remember

• A migration review asks whether anyone will notice the switch, and if so, whether that was decided, announced and made reversible.
• The evidence is in the artefacts, not the diff: inventory, corpus, harness transcript, decisions.
• Eight checks are mechanical: inventory, a case per row, every case run, four channels, every difference decided, one revertible change, the old implementation kept, a named owner.
• Four are manual: is the inventory complete, are the limits written, are the decisions agreed, has the rollback been run.
• Measured: the checklist found a real gap in this module's own validation. Nothing covered B3, and a no-argument case was added.
• Measured: a submission with fourteen same cases compared only two channels. "Same" is only as strong as the list of things compared.
• "Validated on production" ran 3 of 14 cases.
• A big-bang commit fails four checks at once, all from one decision.
• A decided difference passes. The checklist demands intent, not stasis.
• Write the summary in a fixed form so a script can check it, and spend the reviewer's time on the manual half.

Chapter 28.10 lands the migration that ready.mr describes. It switches the flow behind a one-line change, runs the rollback before it is needed, keeps the Perl for a defined period, and then deletes it on purpose.