Safe change: refactor behind a green suite, not faith

Your team already has the safe-refactor playbook — the worked recipe for establishing a green baseline, moving in small steps, and running the suite again after to prove behaviour is unchanged. This module is the layer above the recipe. It’s where you master what the safety discipline actually means — not just “run the tests twice” but the judgment that catches a refactor turning into a rewrite, the reading that separates a step-by-step diff from a wall of changes, and the PR description that makes the structural intent clear instead of leaving the reviewer to infer it from the diff.

“The green suite is your seatbelt — actually fasten it.” This is the line the playbook drives at, and it’s deceptively deep. It doesn’t mean “run the suite at the end”; it means run it before (establish the green baseline), after each step if the changes are significant, and at the end. And if anything goes red, that’s not a problem to paper over — it’s the system working correctly. The refactor is behaviour-preserving only if the suite says it is, not if it looks behaviour-preserving.

Behaviour unchanged — what it means technically

The playbook teaches you to run the tests before and after. Mastery is the judgment behind what those two green runs actually prove — and what they don’t.

• The suite proves the tests pass, not that the behaviour is unchanged. A suite that covers 40% of the code gives you a seatbelt that covers 40% of the seats. If your refactor touches an uncovered path — a rarely-triggered error case, the timeout handler, the Arabic locale string formatter — the suite can go green while the behaviour changed. Know what your coverage covers before you trust it as a proof.
• Characterization tests establish behaviour you don’t yet know. When the code you’re refactoring has thin coverage, the correct prerequisite is a characterization test pass: feed the current code a variety of inputs, record the current outputs, and write tests that assert those outputs. You’re not deciding whether the current behaviour is correct — you’re pinning it, so that any change the refactor makes to it shows up as a red test rather than a silent divergence. Characterization tests are how you turn “I hope nothing changed” into “I know nothing changed.”
• “Looks equivalent” and “is equivalent” are different claims. Claude will read two versions of a function and judge them equivalent based on their logic. It will sometimes be wrong — because equivalence in the presence of side effects, shared state, concurrency, or the specific data your production codebase runs is harder to reason about than it looks on the screen. The suite is the arbiter, not the look. If the suite is green, behaviour is unchanged. If it’s not, something changed, even if the logic looks equivalent.
• A red test after a behaviour-preserving change is a real signal — not an obstacle. The playbook says this and it’s worth emphasizing: never let Claude edit a test to make a refactor’s failure go away. A red test after a supposedly behaviour-preserving change either means the refactor changed behaviour (revert that step) or the test was wrong (which is important to know and fix separately, not to paper over). A refactor that passes a modified test is not a safe refactor.

Small steps — the discipline that makes failures locatable

The playbook asks you to do the refactor as discrete steps, not one giant rewrite. Mastery is the judgment that makes “discrete steps” meaningful rather than nominal.

• Each step should have one intent. “Extract the validation into its own function” is one step. “Extract the validation, collapse the duplicated branches, and rename the function” is three steps that should be three diffs, not one. When a step has one intent, a red test tells you exactly what the intent broke. When a step has three intents, a red test tells you to go looking.
• Show me the diff for each step before you move on. In Claude Desktop, each step lands as a visual diff you accept or reject in the file pane. The correct discipline is to read and accept (or reject) each step before the next one starts. This is not optional review at the end — it’s the review as you go, and it’s what makes each step’s intent visible rather than buried in a wall of changes.
• If a step surprises you, stop. A diff that touches a file you didn’t expect, changes a signature the plan didn’t mention, or introduces a new abstraction not in the plan should be questioned before it’s accepted. “Why did you touch src/lib/db.ts?” is a question to ask now, not after a suite run that went red.
• One concern, one commit, even in a refactor. The commit convention from Module 2 carries directly into refactor commits: extract the validation, commit it. Collapse the branches, commit it. Rename the function, commit it. Small commits with clear why-bodies are what make the refactor reviewable — and reversible if something goes wrong.

Refactor vs rewrite — the line and how to hold it

The hardest judgment in safe refactoring is catching a refactor turning into a rewrite before the test suite catches it.

• A behaviour change is always a different commit. If, while cleaning up a function, Claude says “while we’re in here, we could also fix the edge case where X” — stop. That edge case is either a bug fix (a different commit, possibly a different PR) or a behaviour change (outside the scope of a behaviour-preserving refactor). Accept the refactor, commit it, then address the edge case separately. Mixing a refactor with a behaviour change means a red test could be from either, and you’ve lost the seatbelt.
• The plan is your scope contract. The refactor plan you agreed on in step one is the boundary: “extract the validation, collapse the duplicated branches, rename data to invoicePayload.” Anything outside that plan is not part of the refactor — it’s a scope expansion. A refactor that grows beyond its plan is a rewrite starting to happen. Hold the plan.
• If the refactor is actually a rewrite, name it. Sometimes you open a function to refactor it and discover the real change needed is a structural redesign, not a cleanup. That’s important to know — and the right response is to stop, name the discovery, and treat the redesign as a separate piece of work (spec, plan, implementation, tests, clean commits). A refactor that becomes a rewrite unannounced is a diff nobody can review safely.

The PR description — explaining structural intent

The playbook ends with asking for a summary of what changed and why. Mastery is the judgment that makes that summary useful to a reviewer who can already read the diff.

• The reviewer can see what moved; they can’t see why. “Extracted the validation into invoice-validate.ts” is something the reviewer can read in the diff. “Extracted the validation so the invoice-generation function has a single responsibility and can be tested without a running Supabase client” is why — the reasoning behind the structural decision, the intent the diff doesn’t carry on its own.
• State what behaviour was preserved, not just what was cleaned. “No behaviour was changed: suite ran green before and after, with baseline established against the current output” is the sentence that turns a refactor PR description into a safe one. It’s the seatbelt in prose form — the reviewer knows that the author established a green baseline, not just hoped for one.
• Name the specific structural problems that were fixed. “This function had grown to 380 lines handling validation, calculation, PDF generation, and email in a single flow — impossible to test any piece of it in isolation. It’s now four focused functions that can be tested and changed independently.” That’s the structural intent — the reviewer now understands not just what moved but what problem the move solved.

Your assignment

Complete the safe-refactor loop for one real file or module that has test coverage — your own codebase (recommended) or the Mizan invoice generation function worked through this module. Open the project folder in Claude Desktop with your CLAUDE.md in place and work in the chat — running the suite before and after is the Power Track step.

Module 4 deliverable — safe refactor

1. Before baseline  (suite results or paste of the green output)
   - the test command run, the output showing green
   - the coverage level noted (so the rubric can judge what the green proves)

2. Refactor as separate steps  (git log or session notes)
   - each structural move as its own accepted diff
   - each committed separately with a why body
   - evidence that each step was reviewed before the next started

3. After results  (suite results)
   - the same suite run again, same green
   - if anything went red: the step that caused it, named explicitly, and what
     you did (reverted the step / fixed the implementation / confirmed it was
     a test that was wrong)

4. PR description  (one page)
   - what structural problems existed before
   - what each structural move did to fix them (not what the diff shows —
     why the move was made)
   - explicit statement that behaviour was preserved: green before, green after

How it’s graded — the rubric

Safe-refactor rubric

1. Green baseline        The suite was run before any change and the result
   established           is shown. Coverage level is noted. Not just asserted.

2. Steps are discrete    Each structural move is a separate accepted diff, not
   and reviewed          a wall of changes. Evidence of step-by-step review
                         (accepted each diff before moving on).

3. Green after, same     The same suite run after all steps. Same result.
   coverage              If a test went red: the step named, the fix shown,
                         and no test edited to paper over a real signal.

4. No behaviour change   No step included a bug fix, an edge-case addition,
   mixed in              or a feature change alongside the structural cleanup.
                         One concern per commit, including the refactor steps.

5. PR description        Explains the structural problem that was fixed (the
   explains intent       why), not the diff (the what). States explicitly
                         that green before + green after was the proof.

The bar, shown — a worked model answer (Mizan)

Refactor target: src/lib/invoice.ts — 380 lines, four responsibilities
(validation, calculation, PDF generation, email) in one function.

Before baseline:
  $ npm run test:unit
  ✓ 47 tests passed (src/lib/invoice.ts: 4 tests — happy path, totals,
    zero-line-items, currency). Coverage: 62% of invoice.ts statements.
  Note: the PDF-generation and email paths have no unit tests (they're
  covered by two E2E tests that require Playwright + Resend sandbox).

Step 1 — extract validation:
  Moved payload validation into src/lib/invoice-validate.ts.
  Diff: invoice.ts (-82 lines), invoice-validate.ts (new, +76 lines).
  Suite: green.
  Commit: "refactor(invoice): extract payload validation to invoice-validate.ts
           The validation logic (field checks, client lookup, amount constraints)
           had no way to be tested without generating a full invoice. Now it
           can be unit-tested in isolation with a mock client lookup."

Step 2 — extract calculation:
  Moved the totals, VAT, and rounding logic into src/lib/invoice-calc.ts.
  Diff: invoice.ts (-94 lines), invoice-calc.ts (new, +89 lines).
  Suite: green.
  Commit: "refactor(invoice): extract calculation to invoice-calc.ts
           The AED rounding fix from #201 highlighted that the calculation
           logic needed its own test surface. Extracted so the rounding,
           VAT, and multi-line-item totals can be tested without touching
           the DB or the PDF renderer."

[Step 3 and 4 follow the same pattern for PDF generation and email]

After: $ npm run test:unit → ✓ 67 tests passed (20 new tests for the
extracted functions; the 4 original invoice.ts tests still pass unchanged).

PR description excerpt:
  invoice.ts had four responsibilities in one function (validation,
  calculation, PDF rendering, email dispatch) — impossible to test any
  piece in isolation, and the 380-line function was the most-edited file
  in the last quarter. This refactor extracts each responsibility into
  its own module without changing any behaviour.

  What the extraction fixed: the AED rounding fix (#201) required
  running a full invoice to test. The new invoice-calc.ts has 12 unit
  tests that cover the rounding, VAT, and multi-currency cases without
  a DB or PDF renderer.

  Behaviour preserved: suite green before (47 tests) and after (67 tests,
  20 new). No test was modified. No E2E test changed.

What you’ve proven — and what’s next

Clear the rubric and you’ve demonstrated the discipline that separates a refactor you can stand behind from a rewrite you’re quietly nervous about: a green baseline, discrete steps, and a before/after proof that behaviour never moved.

Module 5 — the big move is next: the migration approach that converts the mechanical majority across the whole repo and surfaces the judgment-requiring cases for you to decide rather than silently guessing at them.