Safe Refactoring skill for Codex

A refactor is safe when the existing tests pass unmodified after every single step, the public behaviour (outputs, errors, side effects, API shape) is identical, and the work is a series of small commits any one of which can be reverted alone. If a test had to change to stay green, either it tested internals or behaviour changed; find out which.

Preconditions

1. Read AGENTS.md for test, type-check and lint commands. You will run them constantly.
2. Confirm the suite is green on the starting commit. Never refactor on red.
3. Measure coverage of the code you are about to move, not the whole repo:

pytest --cov=billing.invoice --cov-report=term-missing tests/
npx vitest run --coverage --coverage.include='src/billing/**'
go test ./billing/... -coverprofile=cover.out && go tool cover -func=cover.out

4. Decide:

Coverage of target	Action
Branches you will touch are covered by behavioural tests	Proceed
Partially covered	Add tests for the uncovered paths first, as separate commits
Barely covered, or legacy code nobody understands	Stop; characterise it first (legacy-code-characterization)
Tests assert on internals you are about to change	Rewrite those tests against public behaviour first, separate commit

5. State the goal and the end shape in two sentences to the user. Refactoring without a destination drifts into rewriting.

The step loop

pick the smallest named refactoring -> apply it -> type-check -> run focused tests
  -> run the wider suite -> commit -> next
on any red: revert the step (git checkout -- . or git restore), make it smaller, retry

• A step is small enough when you can describe it in a commit subject with one verb: “Extract tax calculation from Invoice.total”.
• Do not debug a red refactor step. Revert it. Debugging means you took a step too large to understand.
• Commit every green step. Ten tiny commits can be squashed later; a broken big one cannot be bisected.
• Never mix a behaviour change, bug fix or reformat into a refactor commit. If you find a bug, note it and fix it in its own commit before or after, with its own test.

Choosing the refactoring

Smell	Refactoring	Watch out for
Name lies or is vague	Rename	String references, serialised names, public API
Function does several things	Extract function	Captured mutable locals; early returns
Indirection adds nothing	Inline function or variable	Side effects evaluated a different number of times
Code lives in the wrong module	Move function / class	Import cycles; re-export from the old path for callers
Long parameter lists	Introduce parameter object	Default values; keyword-argument call sites
One function mixes parsing and computing	Split phase	Error handling that spans both phases
Type switches repeated in many places	Replace conditional with polymorphism	Only worth it at three or more switch sites
Signature must change and has many callers	Parallel change (below)	Callers outside this repo

Parallel change for signatures and public API

Never change a widely used signature in one step. Expand, migrate, contract:

# 1. Expand: add the new form next to the old one; old delegates to new.
def charge(order, amount_cents: int, *, currency: str = "USD"): ...

def charge_amount(order, amount):  # old API
    warnings.warn("charge_amount is deprecated; use charge()", DeprecationWarning, stacklevel=2)
    return charge(order, round(amount * 100))

# 2. Migrate callers one module per commit.
# 3. Contract: delete the old form once rg finds no callers (and, for libraries,
#    after the deprecation window the user agrees).

For anything consumed outside the repo (published package, HTTP API, DB column, event schema), contracting is a breaking change and needs the user’s decision.

Find every reference, not just the compiler’s

Type checkers and IDE renames miss dynamic references. Before renaming or moving, search the whole repo including non-code files:

rg -n --hidden -g '!.git' -w "invoice_total"
rg -n "invoice_total|invoiceTotal|InvoiceTotal|INVOICE_TOTAL"

Look specifically for:

• Reflection and string dispatch: getattr(obj, name), obj[methodName](), DI container keys, Rails/Django conventions that derive names from classes.
• Serialised names: JSON fields, ORM column names, GraphQL fields, protobuf, cache keys, queue message types. Renaming the Python attribute may rename the wire field.
• Templates, SQL files, YAML/JSON config, feature flag names, CLI flags, env vars.
• Observability names dashboards depend on: log fields, metric names, span names.
• Mocks patched by string path: mock.patch("billing.invoice.send_email") breaks silently after a move (the test still passes but patches nothing).

Use tooling for mechanical edits at scale: language-server renames when available, or structural search and replace (ast-grep, jscodeshift) through codemods rather than regex sed across the tree.

Verifying behaviour is unchanged

Run after every step: the type checker (tsc --noEmit, mypy, pyright, go vet, cargo check) and focused tests. At the end:

• Full suite green with zero test-file edits other than import paths and renamed symbols: git diff --stat <start>..HEAD -- tests/ '**/*.test.*' '**/*_test.go'.
• Lint clean, no new warnings.
• Public surface unchanged unless agreed: compare exported symbols, e.g. git diff <start>..HEAD -- '**/index.ts' '**/__init__.py', or an API report tool.
• For code paths with weak tests, compare outputs on real inputs before and after (golden-master comparison; see legacy-code-characterization for a script).
• For hot paths, a quick benchmark shows no regression (extracting into closures or adding indirection in tight loops can cost real time).
• Every commit in the series passes: git rebase --exec "<test cmd>" <start>.

Hard rules

• No behaviour change hidden in a refactor. Not even “obvious” fixes.
• No editing test assertions to keep the suite green.
• No large moves plus edits in one commit: move first (pure move, git detects the rename), then edit, so reviewers see a clean diff.
• Stop and ask when the refactor would change public API, database schema or anything another team consumes.

Report

### Refactor: <goal>
Steps (each committed and green):
1. a1b2c3d Extract tax calculation from Invoice.total
2. d4e5f6a Move tax module to billing/tax.py (pure move)
3. 0718293 Introduce TaxContext parameter object
Tests: unchanged except 2 import paths; `pytest -q` 412 passed at every commit
Public API: unchanged (Invoice.total signature identical)
Found but not fixed (separate work): rounding bug in discount.py:77, reported in #1601