Tutorials · 2026-09-05 · 975 words · 4 min read

Refactoring with Claude Code: how do you keep behaviour unchanged?

Restructuring a legacy script with a coding agent: the same characterization tests run against old and new code, one refactor per turn, fixes in their own commit.

tutorial refactoring characterization tests pure functions review

You get a refactored version of the legacy logreport script from Add a test suite to a legacy script, a test file that runs the same characterization tests against both versions, and the rules I would give an agent so that a refactor stays a refactor. No agent ran in this session. What ran here was Python 3.13.12 on Windows: both suites, and a byte comparison of the two scripts' output on the same log. The prompts are the ones I would send.

The rule that makes this safe

Feathers' definition is the contract: characterization tests "document your system's actual behavior". A refactor is any change under which those tests stay green without being edited. The moment a test has to change, you are no longer refactoring; you are changing behaviour, and that goes in its own commit with its own explanation.

From that one rule come the three I would put at the top of the session:

1. One refactor per turn. Extract a function, or remove a global, or rename; never two of those in one diff.
2. Tests green before and after every turn, with no edits to the test file in a refactor turn.
3. A behaviour change never shares a commit with a structure change. If the refactor exposes a bug, write it down and keep going.

The common-workflows page says the same thing in fewer words in its refactoring recipe: "Do refactoring in small, testable increments."

The safety net, run against both versions

The interesting part of the shipped test file is not the tests but the layout. The characterization tests live in a mixin with no base class, and two concrete classes bind it to an implementation:

class Characterization:
    """Mixin: subclasses define report(lines) -> text."""

    def test_report_for_five_lines_is_exactly_this(self):
        self.assertEqual(self.report(LINES), EXPECTED_FIVE)
    # ... five more, including the month-boundary bug pinned on purpose


class LegacyBehaviour(Characterization, unittest.TestCase):
    @staticmethod
    def report(lines):
        out = io.StringIO()
        legacy.run(lines, out)
        return out.getvalue()


class RefactoredBehaviour(Characterization, unittest.TestCase):
    @staticmethod
    def report(lines):
        return new.run(lines)

If a refactor changes any output byte, RefactoredBehaviour goes red while LegacyBehaviour stays green, and the failing test name says which behaviour moved. That asymmetry is the signal. It is cheap to set up and I would ask for it in the first turn, before any restructuring:

Create logreport_new.py as an exact copy of logreport.py. Write a test
module that loads both files and runs the existing characterization
tests against each through a shared mixin. Run it. Both classes must be
green before we touch anything.

Two things I learned making this work, both worth telling the agent up front. The helper must not be called run, because unittest.TestCase.run already exists and the runner will call your helper with a result object; the traceback is confusing for about a minute. And when loading a module from a file whose name contains hyphens, register it in sys.modules before executing it, or a dataclass with postponed annotations fails at import with an AttributeError about NoneType. The shipped loader does both.

The refactor, one turn at a time

The prompts I would send, each one a single structural move:

Turn 1: Replace the five module globals with a Summary dataclass that
summarise(lines) builds and returns. No printing in summarise. Tests
green before and after; do not edit the tests.

Turn 2: Split parse_line so it returns an Entry or None and has no side
effects. Move the warning text into Summary.warnings.

Turn 3: Add render(summary) -> str and make run(lines) -> str compose
summarise and render. main() writes run(fh) to stdout.

Turn 4: Run both suites and the CLI on app.log; show the byte comparison.

Here is the diff for the function that changed most, written by hand in this session. Before, parse_line did four jobs: validate, count, compare, and print.

-def parse_line(line, out=sys.stdout):
-    global FIRST, LAST, BAD, TOTAL
-    TOTAL += 1
-    parts = line.rstrip("\n").split(" ", 3)
-    if len(parts) < 4 or len(parts[0]) != 10 or len(parts[1]) != 8:
-        BAD += 1
-        print("WARN skipping line %d: %r" % (TOTAL, line.rstrip("\n")), file=out)
-        return
-    stamp = fmt_date(parts[0]) + " " + parts[1]
-    level = parts[2]
-    COUNTS[level] = COUNTS.get(level, 0) + 1
-    if FIRST is None or stamp < FIRST:
-        FIRST = stamp
-    if LAST is None or stamp > LAST:
-        LAST = stamp
+def parse_line(line: str) -> Entry | None:
+    """Return an Entry for a well-formed line, None for a malformed one."""
+    parts = line.rstrip("\n").split(" ", 3)
+    if len(parts) < 4 or len(parts[0]) != 10 or len(parts[1]) != 8:
+        return None
+    return Entry(stamp=fmt_date(parts[0]) + " " + parts[1], level=parts[2])

Counting, comparing, and the warning text moved into summarise, which folds entries into a Summary. The comparison that picks first and last is still a string comparison of the DD/MM/YY form, and the comment beside it says why:

        # Legacy behaviour preserved: lexical comparison of the DD/MM/YY form.
        if s.first is None or entry.stamp < s.first:
            s.first = entry.stamp

Keeping a known bug is the uncomfortable part of rule three, and it is the part an agent most wants to skip. Fixing it here would be a one-line change that flips one test, and it would also mean the commit titled "extract pure functions" changes what users see. The fix is the next commit, on its own, with the pinned test's expectation flipped in the same diff.

What ran here

The combined suite, seventeen tests: six characterization tests times two implementations, plus five unit tests that only the refactored code can support because it has functions with no streams and no globals.

$ python -m unittest refactor-with-an-agent-characterization-tests_test.py
.................
----------------------------------------------------------------------
Ran 17 tests in 0.001s

OK

The legacy suite, unchanged from the previous article, still passes on its own (8 tests, OK). And the byte comparison on the five-line sample log, using cmp on the two outputs:

$ python add-tests-to-a-legacy-script-with-an-agent.py app.log > a.txt
$ python refactor-with-an-agent-characterization-tests.py app.log > b.txt
$ cmp a.txt b.txt && echo IDENTICAL
IDENTICAL

The new unit tests are the payoff. test_summarise_counts_without_rendering checks counts as a dictionary without parsing text. test_summarise_is_pure_across_calls is the test that could not exist before: calling summarise twice gives independent results because there is no state to reset.

Where to run it

A refactor session touches every function in a file, which is the worst possible neighbour for a feature branch in the same checkout. The worktrees page describes claude --worktree <name> as creating a checkout "under .claude/worktrees// at your repository root, on a new branch named worktree-", and says that on exit, if the worktree is clean, "Claude removes the worktree and its branch automatically". For a one-file refactor that is cheap isolation: the feature work in the main checkout never sees a half-extracted function. Small commits, worktrees, and branches covers the habits around that.

Review before merge

The review is where rule two gets enforced by a human. The checklist from The review loop has three items that apply to every refactor diff: was the test file touched (it must not be), did any exception handling get added or widened (a refactor should not), and did the public surface change (main(argv) still takes the same arguments and returns the same codes). On this diff the honest answer to the first is that the test file changed, but as a new file that imports both implementations, which is the point of turn one and is the reason to do it as its own commit.

Limits

This is a 90-line script with one input and one output, and the refactor was mine, so the claim is narrower than it looks: the shipped tests prove the two versions agree on the cases I chose, and the byte comparison proves they agree on one log. Neither proves the agent-driven version of turns one to four would land in the same place, only that the net would catch it if it did not. The bug is still in there, on purpose, and the commit that removes it is a one-liner I have not made.