AI-Assisted Legacy ERP Refactor With No Written Spec

The module was a purchase requisition approval chain: one file, about nine hundred lines, and a nest of conditions on amount, branch, cost centre and the requester's own role. The only thing resembling a specification was a commit message from 2017 saying the thresholds were agreed with finance in the Tuesday meeting. The person who attended that meeting left two years later. The rule was still running every day, on every requisition, in three branches.

That is the normal condition of ERP maintenance rather than an unusual one, and it changes what a refactor even is: you cannot verify a change against a requirement you do not have. This post is the method I use instead — pin the behaviour, cut the seams, then hand an agent one narrow extraction at a time — and the four modules where I will not let an agent work unsupervised.

The specification was a Tuesday meeting

In an ERP built for one company, the requirements document is the shortest-lived artefact in the project. It describes the system as it was scoped, gets superseded by the first three weeks of user acceptance testing, and is then never edited again. What survives is the code, which makes the code the specification whether anyone intended that or not.

So before writing anything, work out where the surviving rules actually are. On this module there were four places, and none of them was a document:

• The code path itself, including branches that stopped being reachable when a warehouse closed and were never removed.
• The production data, which is a record of every rule that has ever applied, including the ones that were changed years ago.
• The users, whose workarounds are evidence. People raising two requisitions instead of one is a statement about where a threshold sits.
• The reports finance reconciles against, because those few totals are the real acceptance criteria and always have been.

The consequence is the one thing that makes this different from an ordinary refactor. The first job is not to improve the code, and not to understand it either. It is to make what the code does right now observable from the outside, so that a change stops being invisible.

Pin the behaviour before anything moves

Characterisation tests do that. The term is Michael Feathers', from Working Effectively with Legacy Code, and the definition is deliberately narrow: the test asserts what the code does, not what it should do, and its purpose is to make change detectable. Wikipedia's summary of the practice puts it well by calling these tests change detectors rather than correctness validators, and notes that they do not verify correct behaviour, which can be impossible to determine.

This is the one context in software where a test encoding current behaviour is the goal rather than the criticism. The technique that makes them fast to write is counter-intuitive: assert something you know is wrong, run it, and let the failure diff print the real value. Then paste the real value in and name the test after what you observed.

// Characterisation, not correctness. The test name says what the code DOES;
// the comment says what I think of it. Nobody should later mistake either
// assertion below for a statement of what finance asked for.
import { describe, it, expect } from "vitest";
import { resolveApprovalChain } from "../src/requisition/approval";

const requester = {
  id: 4821,
  role: "branch_manager",
  branchId: 7,
  costCentre: "OPS-JKT",
};

describe("resolveApprovalChain: observed behaviour, September 2026", () => {
  it("characterises: a branch manager's own requisition skips the branch step", () => {
    const chain = resolveApprovalChain(requester, { amountIdr: 25_000_000 });

    // I asserted [] on the first run on purpose. The failure diff printed the
    // real chain, and this is a paste of it. Reading 900 lines would have
    // given me my guess about the behaviour, not the behaviour.
    expect(chain.map((step) => step.role)).toEqual([
      "finance_controller",
      "director",
    ]);
  });

  it("characterises: an amount EQUAL to the band edge takes the lower band", () => {
    const chain = resolveApprovalChain(requester, { amountIdr: 20_000_000 });

    // I believe this is wrong. It is pinned anyway, because if an agent
    // "corrects" the comparison operator I want a red suite today, not a
    // finance email after month-end.
    expect(chain.map((step) => step.role)).toEqual(["finance_controller"]);
  });
});

Two rules about those names, because the names are what a future reader inherits. Prefix every one with characterises, so nobody mistakes it for a statement of intent. And pin at the module's public boundary rather than on its internals, or the tests will block the refactor they exist to enable — an assertion on a private helper is a lock on the very shape you are trying to change.

Seams: changing behaviour without editing the code that runs

A seam, in Feathers' terms and in chapter four of that book, is a place where you can alter behaviour without editing in that place. It sounds abstract until you go looking for them in an ERP module, where the same five keep appearing: the clock, the database query, the configuration table read, the document numbering service, and the currency rate lookup. Naming those five is most of the work, because everything else in the module is arithmetic on their results.

// Before: three dependencies buried mid-function, so the only way to test
// this is to have a database, a network and the right date.
export async function resolveApprovalChain(requester: Requester, doc: Doc) {
  const today = new Date();
  const bands = await bandRepo.forBranch(requester.branchId);
  const rate = await fxService.rateFor(doc.currency, today);
  // ... 900 lines of accumulated policy
}

// After: the same three, hoisted into one optional deps object. This is the
// seam. The default argument is what matters: every existing call site still
// compiles untouched, so introducing the seam changes no behaviour.
export interface ChainDeps {
  now: () => Date;
  loadBands: (branchId: number) => Promise<Band[]>;
  rateFor: (currency: string, on: Date) => Promise<number>;
}

const productionDeps: ChainDeps = {
  now: () => new Date(),
  loadBands: (branchId) => bandRepo.forBranch(branchId),
  rateFor: (currency, on) => fxService.rateFor(currency, on),
};

export async function resolveApprovalChain(
  requester: Requester,
  doc: Doc,
  deps: ChainDeps = productionDeps,
) {
  const today = deps.now();
  const bands = await deps.loadBands(requester.branchId);
  const rate = await deps.rateFor(doc.currency, today);
  // ...
}

In TypeScript the cheapest seam is a default parameter. The dependency object goes in, the production implementation becomes the default, and no call site changes — which is exactly what makes the step safe to take before you have any test worth trusting. The characterisation test then passes a frozen clock and a fixed band table, so it stops depending on today's date and on whatever the configuration table happens to hold this week.

There is a chicken-and-egg problem here worth stating, because it is where I have wasted the most time. Introducing a seam is itself a change, and you want tests before you change anything. The way out is to pin first at the outermost boundary you can reach with no edits at all — the HTTP handler or the service entry point, against a real database, slowly — cut the seam under that net, and only then move the pin inward to the function you actually wanted to work on. The slow test is scaffolding and gets deleted at the end.

One extraction, one test run, one commit

The unit of agent work here is smaller than it feels like it should be: one named function, out of one line range, with the suite run between each. Broad instructions produce a diff that moves eleven things, and a red suite then tells you only that one of the eleven is wrong. The loop I settled on has four steps.

1. Name the target precisely — the line range to extract, the function name you want, and the file the new function should live in.
2. Give the agent the suite command, and say explicitly that the test files are read-only for this task.
3. Run the suite yourself. Any red line means revert and re-scope, not repair, because a repaired extraction hides which assumption was wrong.
4. Commit the extraction on its own, with no behaviour change in the same commit, and paste the test summary into the message.

The read-only instruction has to be said out loud. The fastest way to turn a red suite green is to edit the assertion, and an agent asked to make the tests pass will do exactly that, cheerfully and with a plausible explanation. On a module where the tests are the only specification you have, that is not a shortcut, it is the deletion of the specification.

Where an agent must not go unsupervised

An agent must not touch posted financial periods, document numbering, tax calculation, or anything that changes historical figures, without a human reading every line of the diff. That is not a judgement about capability. It is about the shape of the failure. In a reporting or master data module a wrong diff produces a wrong number on a screen, somebody complains within the hour, and you re-run it. In posting, numbering and tax, a wrong diff produces figures that have already been reported, and it surfaces weeks later when someone reconciles at month-end.

Each of the four has its own reason. Posted periods are guarded so carefully by mature ERP products that Odoo ships two separate lock dates for them: a Lock Everything date that blocks both modification of posted journal entries and new postings dated on or before it, and a Hard Lock date that cannot be reversed, for jurisdictions requiring inalterability. That feature exists because this class of change has to be made impossible rather than merely discouraged.

Document numbering is the one that looks most refactorable and is not. Gapless numbering is a legal requirement in several jurisdictions, and it cannot be delegated to a database sequence: the PostgreSQL documentation states plainly that a value obtained by nextval is not reclaimed for re-use if the calling transaction aborts, so aborts and crashes leave gaps, and sequence objects therefore cannot be used to obtain gapless sequences. Which is why every ERP hand-rolls numbering with a counter table and a lock — verbose, awkward code that looks exactly like something an agent should tidy up. Tax calculation and landed cost complete the list: re-allocating landed cost changes inventory valuation retroactively, and with it the cost of goods sold on invoices that have already been issued.

The enforceable version of the rule is a deny list, because an instruction in a prompt is a preference while a permission rule is not. Claude Code evaluates deny rules before ask and allow, and the first match decides regardless of specificity, so a deny rule beats a session flag:

// .claude/settings.json
// deny is evaluated before ask and allow, and the first match decides, so
// these beat any allow rule and any --allowedTools flag for the session.
{
  "permissions": {
    "deny": [
      "Edit(./src/accounting/posting/**)",
      "Edit(./src/accounting/period-close/**)",
      "Edit(./src/tax/**)",
      "Edit(./src/numbering/**)",
      "Edit(./db/migrations/**)"
    ]
  }
}

Be honest about what that buys you. The documentation is explicit that Read and Edit deny rules cover the built-in file tools and the file commands Claude Code recognises inside Bash, such as cat, head, tail and sed, but not arbitrary subprocesses — for operating-system-level enforcement it points you at the sandbox instead. So a deny list is a guard rail against the accidental edit, not a security boundary. The working rule underneath it is simpler: if the worst outcome of a wrong diff is that somebody re-runs a report, the agent can work alone behind the tests; if the worst outcome is that a number already printed on a document changes, a human reads every line.

Write the rule down the moment it becomes legible

Somewhere around the third or fourth extraction, a condition stops being noise and becomes a sentence. On this module it was the branch-manager skip: managers used to own their cost centres under an older org chart, so a requisition one of them raised was already approved by the person the chain would have routed it to. The rule was not arbitrary at all. It was correct in 2017 and had simply outlived its reason.

That sentence is worth more than the refactor, and it has nowhere to live unless you put it somewhere. So every recovered rule gets three artefacts before I move on:

• A comment beside the condition saying why it exists, when that was learned and from whom — or, honestly, that the origin is unknown and the behaviour is merely pinned.
• A test renamed from characterises to a statement of intent, which is the moment a characterisation test is promoted into a real unit test.
• An issue, if the rule now looks wrong, describing the current behaviour and naming who has to decide. Never a fix folded into a refactoring commit.

The second artefact is also the only progress metric on a module like this that means anything. Not lines removed, not files split, not coverage. How many tests you have been able to rename, because that number is the size of the specification you have actually recovered.

Refactoring the guarded module anyway

The boundary is not a permanent exemption, which is awkward, because the posting code usually needs the work more than anything else in the system. The way in is replay: take a period that is already closed and audited, re-run the posting logic into a scratch schema, and diff the resulting trial balance against what was actually posted. That makes the guarded module observable in the same way a characterisation test makes a function observable, without writing a single row to the ledger.

# Replay one closed month into a scratch schema, then diff the trial balance.
# Nothing writes to public.*; the real ledger is read-only for the whole run.
psql -v ON_ERROR_STOP=1 -c 'CREATE SCHEMA replay_2026_07'
node ./scripts/replay-posting.mjs --period 2026-07 --target replay_2026_07

psql -v ON_ERROR_STOP=1 <<'SQL'
-- One row per account where the refactored posting path disagrees with what
-- was actually posted and already audited. An empty result is the whole point.
SELECT COALESCE(p.account_code, r.account_code)         AS account_code,
       COALESCE(p.balance, 0)                            AS posted,
       COALESCE(r.balance, 0)                            AS replayed,
       COALESCE(r.balance, 0) - COALESCE(p.balance, 0)   AS delta
FROM   (SELECT account_code, sum(debit - credit) AS balance
        FROM   public.gl_entry
        WHERE  period = '2026-07'
        GROUP  BY account_code) p
FULL JOIN
       (SELECT account_code, sum(debit - credit) AS balance
        FROM   replay_2026_07.gl_entry
        GROUP  BY account_code) r
  ON   r.account_code = p.account_code
WHERE  COALESCE(p.balance, 0) <> COALESCE(r.balance, 0)
ORDER  BY abs(COALESCE(r.balance, 0) - COALESCE(p.balance, 0)) DESC;
SQL

A delta of zero on every account means the refactored path reproduces a month that has already been signed off, which in the absence of a specification is the strongest evidence available anywhere in the building. A non-zero delta on one account is better still: it is the rule you had not found yet, and it has just told you which account it lives on.

Two limits, stated because I have been caught by both. Replay proves the past, not the future, so a rule that legitimately changed mid-period shows up as a delta that is correct and has to be excluded by hand. And replay is only honest if the scratch schema is genuinely separate, which means confirming that nothing in the posting path writes outside its target schema before you run it — one hard-coded table name in a rarely-used branch is enough to turn the exercise into a live posting.

The method is not sophisticated. Make current behaviour observable, then change it in units small enough that a red test names which change did it. What an agent adds is that the small units become cheap, so the extraction you would have postponed for a quarter takes an afternoon instead. What it does not change is who answers for a posted figure that moved — which is why the modules that can move one get a human on every diff, and why the tests stay read-only while the agent is working.