Chapter 22 of 28, in Part IV, Tutorials
A refactor plan for a coding agent: pin the behaviour with tests first
Tests first, then structure: pin a legacy script's behaviour with characterization tests, run them against old and new code, one refactor per turn.
You get the legacy logreport script with a suite that pins what it does today, a refactored version of it, a test file that runs the same characterization tests against both versions, and the rules we 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 we would send.
Merged, 29 October 2026: the first half of the method, getting tests onto the untested script, was a separate article until today. Its survey, seam and pinning steps are now the next section, and its old address redirects here. Also on 29 October 2026 the title changed: it was "Refactoring with Claude Code: how do you keep behaviour unchanged?", which read as a record of a Claude Code session, and no agent ran for this article.
Before the refactor: the first tests
The subject is logreport, a 90-line log summariser we wrote to have the problems legacy scripts have: module-level state, printing from inside the parser, a date format done by string slicing, and one latent bug. It ships as add-tests-to-a-legacy-script-with-an-agent.py with its suite beside it. Feathers names the hard part of testing code like this: "breaking dependencies around a piece of code well enough to be able to exercise it in a test harness." So the agent's first job is not to write tests. It is to find the seams.
Survey, read-only. The permission-modes page describes plan as a mode where "Claude reads files and runs read-only shell commands to explore but doesn't edit your source files", which is the right shape while you are still learning what the script does:
Read logreport.py. Do not change anything. Tell me: (1) every piece of
module-level state and who writes it, (2) every print call and which
stream it uses, (3) the exact input format it expects, (4) anything that
looks like a bug or a quirk. Quote line numbers.
On this script a correct survey names five globals (COUNTS, FIRST, LAST, BAD, TOTAL), two print sites (a warning inside parse_line, the report in report), the YYYY-MM-DD HH:MM:SS LEVEL message line shape, and two quirks: dates are re-formatted to DD/MM/YY, and the first/last timestamps are picked by comparing those DD/MM/YY strings. The second quirk is the bug. Nobody fixes it yet.
Seams that change no output byte. The smallest change that lets a test pass lines in and capture the output: every printing function gains an out parameter defaulting to sys.stdout, the globals gain a reset(), and one new function composes them.
+def reset():
+ global COUNTS, FIRST, LAST, BAD, TOTAL
+ COUNTS = {}
+ FIRST = None
+ LAST = None
+ BAD = 0
+ TOTAL = 0
+
+def run(lines, out=sys.stdout):
+ """Seam: process `lines` and write the report to `out`. Resets the globals first."""
+ reset()
+ for line in lines:
+ parse_line(line, out)
+ report(out)
The reset() is not cosmetic: without it a second test in the same process inherits the first test's counts, and the suite passes or fails depending on test order. There is a test for exactly that.
Pin the behaviour, the bug included. The tests are written the Feathers way: run with a wrong expected value, paste the actual output in, name the test after what it showed. The one that matters most pins the bug:
def test_last_seen_is_wrong_across_a_month_boundary_documented_bug(self):
# 1 September sorts before 31 August once both are DD/MM/YY strings.
out = run(["2026-08-31 23:59:59 INFO x\n", "2026-09-01 00:00:01 INFO y\n"])
self.assertIn("first: 01/09/26 00:00:01\n", out) # wrong, pinned on purpose
self.assertIn("last: 31/08/26 23:59:59\n", out) # wrong, pinned on purpose
When somebody fixes the comparison, this test fails, they read its name, and they flip the expectation in the same commit as the fix. The suite has eight tests: the exact seven-line report for a five-line sample with one garbage line, empty input, sort order (count descending then name), case-sensitive levels, three malformed stamps via subTest, state reset between runs, the DD/MM/YY table, and the month-boundary bug. Against the shipped script:
$ python -m unittest add-tests-to-a-legacy-script-with-an-agent_test.py
........
----------------------------------------------------------------------
Ran 8 tests in 0.000s
OK
Only with that suite green does the conversation about bugs start, and even then as a list rather than a change: "List the behaviours the tests pinned that you think are bugs, one per line, with the test name that would need to change. Do not change code or tests." The best-practices page asks for the same kind of thing at this stage: "have Claude show evidence rather than asserting success". The rest of this article keeps those eight tests as the net and restructures the script underneath them.
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 we would put at the top of the session:
- One refactor per turn. Extract a function, or remove a global, or rename; never two of those in one diff.
- Tests green before and after every turn, with no edits to the test file in a refactor turn.
- 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 we 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 we 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 we 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, refactor-with-an-agent-characterization-tests_test.py, 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 from the first section, unchanged, 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/
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 ours, so the claim is narrower than it looks: the shipped tests prove the two versions agree on the cases we 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 we have not made.