Skip to content

Skills · dev-team

refactor-plan

Plan paying down one piece of technical debt as a refactor that serves the goal it is blocking — characterisation tests on the riskiest behaviour first, then small steps each merged to main with main releasable after every one, pure refactors kept apart from the behaviour changes the goal needs, a scope fence, a stop point, a technique per step (extract, branch by abstraction, parallel change, strangler fig, mikado) and the clashes with in-flight feature work. Use when the user asks for a refactor plan, how to untangle, break up, replace or clean up a module without a rewrite, how to pay down a tech-debt item, "how do we refactor X without stopping feature work", "how do we get off X without a big bang", or wants a strangler-fig or mikado plan.

Pack dev-teamTier standardJob Build and review softwarev0.2.0

npx skilldrop-cli install refactor-plan
Other ways to install

With the sibling skills it hands off to:

npx skilldrop-cli install refactor-plan --with-related

The whole pack it belongs to:

npx skilldrop-cli install --pack dev-team

By hand, from a clone of the repo:

cp -R packs/dev-team/skills/refactor-plan ~/.claude/skills/

In Claude Code, with the dev-team plugin installed, invoke it as:

/dev-team:refactor-plan

Try it

A realistic prompt from the skill's acceptance evals:

This 1,800-line OrderService is where every change goes wrong; plan how we untangle it without stopping feature work. The code is order_service.py, the register row is debt-register-row.md, coverage is in test-coverage.txt, and what's in flight is in-flight-prs.md.

What a good result looks like

Example output

search ranker with a flag pile · the skill's own worked example, source

Input

TD-07 from our register got the go-ahead. search/ranker.py has one 600-line function, rank(query, docs, **flags), driven by 14 boolean flags listed in RANK_FLAGS at the top of the file. Register row: "5 of the last 8 search PRs changed rank(); INC-540 (Jul, legacy_tiebreak reversed the order of equal scores) and INC-552 (Aug, personalise is ignored when boost_recent is on — not fixed; the web client sends personalise only when boost_recent is off)." tests/search/test_ranker.py covers 4 of the 14 flags. PR #212 "Add boost_local flag" is open and edits rank(). Next quarter the mobile app needs its own ranking with different weights. Owner: Search team.

Output

Reply in chat (the default: the user asked how, not for a document)

Turn each flag into its own scoring function and build a ranking from a list of them, so the mobile ranking is a new list rather than an edit to rank(). First step: approval tests for every flag in a new test file, this week, because rank() has tests for only 4 of 14 flags.

What the code shows

  • search/ranker.py:rank reads all 14 RANK_FLAGS in one body; tests/search/test_ranker.py covers 4 of them.
  • INC-552 is live behaviour a client depends on: the web client sends personalise only when boost_recent is off, so the test pins it as-is.
  • #212 is editing rank() now; moves wait for it, tests don't.

Steps (each one PR to main; you can stop after any of them)

  1. [refactor] Approval tests in tests/search/test_ranker_characterisation.py: personalise + boost_recent together (INC-552, as-is), legacy_tiebreak on equal scores (INC-540), then each flag alone.
  2. [refactor] After #212 merges: extract the tie-break block into _tiebreak, unchanged; it caused INC-540.
  3. [refactor] Add rank_with_profile(query, docs, profile); rank(**flags) builds a profile and delegates. The mobile team can start from here.
  4. [refactor] Move the 15 flags (with boost_local) into scoring functions, five per PR, three PRs.
  5. [refactor] Move callers to rank_with_profile, delete **flags, add a CI check that forbids new flags.

Target: search PRs that edit rank(), 5 of the last 8 → the mobile ranking ships without editing it. Stop point: every flag is a scoring function (step 4); step 5 is scheduled but optional. Not touched: INC-552 behaviour, RANK_FLAGS names, weights, the API response. Anything found during the work that isn't a step goes to the debt register. To confirm: does anything outside this repo call rank() with flags? It decides whether step 5 deletes **flags or keeps a shim.

The same plan as a document (when the user asks for one)

Refactor plan: take the scoring rules out of rank() so new rankings stop editing it

Target: search PRs that edit rank() — baseline 5 of the last 8 (TD-07) → the mobile ranking ships without editing rank(). Stop point: every flag in RANK_FLAGS is its own scoring function, and a ranking is built from a list of them. Removing **flags from rank() is scheduled but outside the stop point (step 7). **First step: characterisation tests for all 14 flags, then the expand step for a profile-based rank — first because the mobile ranking next quarter is the work this unblocks. Type: every step is a refactor; ranking output is identical throughout. The mobile weights are feature work built on step 3. Not touched:** INC-552 behaviour, RANK_FLAGS names, the search API response shape. [assumption] rank() is called only from inside this repo; see To confirm.

Steps

#ChangeTechniqueVerified byRollback (live side)RiskSizeOwner
1Approval tests in a new tests/search/test_ranker_characterisation.py: personalise + boost_recent together, legacy_tiebreak on equal scores, then each flag alonecharacterisation testsnew tests pass on current coderevert PRnone, tests onlyMSearch team
—Wait for #212 to merge——————
2Extract the tie-break block into _tiebreak(docs, legacy) in ranker.py, unchangedextractstep 1 testsrevert PRordering of equal scores (INC-540)SSearch team
3Add rank_with_profile(query, docs, profile); rank(**flags) builds a profile from the flags and delegatesparallel change (expand)step 1 tests; rank output identicalrevert PR; rank is the only callerflag-to-profile mapping misses a flagMSearch team
4–6Move flags into scoring functions, five, five and five per PR (15 with boost_local)extract, behind step 3step 1 tests + #212's testsrevert that batch's PRinteraction between flags in one batchM eachSearch team
7Move callers to rank_with_profile; delete **flags; CI check forbids new flagsparallel change (contract)suite + checkrevert PRcaller outside the repoSSearch team

Stop after any step: each leaves rank() shorter and better tested. Two entry points exist from step 3 to step 7 and nowhere else; step 7 is scheduled for the sprint after step 6.

Step detail (step 3 shown)

  • Change: new rank_with_profile in search/ranker.py; rank keeps its signature and builds a profile from **flags.
  • Technique: parallel change — callers move over later, so the old entry point stays live.
  • Verified by: every step 1 approval test, run through rank (unchanged callers).
  • Rollback: revert the PR; nothing calls rank_with_profile yet.
  • Risk: a flag dropped from the profile mapping changes ranking silently; the per-flag approval tests catch it.
  • Size: M. Owner: Search team.
  • Clashes: none once #212 has merged.

Characterisation tests

FunctionPinIncludes current bug?Lives in
rankeach of the 14 flags alone over a recorded query setnotests/search/test_ranker_characterisation.py (new, so it can't conflict with #212)
rankequal scores with legacy_tiebreak on and offno (INC-540 is fixed)same
rankpersonalise + boost_recent togetheryes: personalise ignored, pinned as-is (INC-552)same

Clashes with feature work

In-flight workShared files / functionsDecisionEffect on the plan
#212 Add boost_localsearch/ranker.py::rankwaitstep 2 starts after #212 merges; boost_local joins batch 4–6
Mobile ranking (next quarter)rankfold inthe mobile team builds its profile on rank_with_profile once step 6 lands

Scope fence

  • Not touched: search API response shape, RANK_FLAGS names, weights.
  • Pulled out of this plan: fixing INC-552 is a behaviour change; it is a separate ticket, and the web client workaround has to be removed in the same change.
  • Rule: anything found during the work that isn't in a step goes to the debt register as a new item, not into this plan.

To confirm

  • Does anything outside this repo call rank() with flags? It decides whether step 7 can delete **flags or must keep a shim.

How it's checked

Measured against plain Claude Code (2026-10-23): it met 57% of its checks, against 38% without it, and a blind judge preferred it. How skills are checked

55 assertions across 7 acceptance evals, and 11 trigger queries. Every skill's checks

What the output must do
  • Opens with the approach and the first step in its first lines, and states the target with its baseline taken from the register row (9 of the last 12 order PRs touching order_service.py, or the 3.1-day against 1.2-day review time) and a checkable stop point
  • Characterisation tests come before any step that touches calculate_tax, refund or send_confirmation_email (0% coverage), and they pin the per-line tax rounding and the zero-price negative-quantity lines as they behave today rather than fixing them
  • No step is a rewrite or a long-lived branch: every step is one PR merged to main, and moves are separated from changes
  • Each step says what changes, why (naming the technique where it matters), which tests verify it, and whether it is a pure refactor or a behaviour change; any step that adds a flag or an abstraction says which side is live for rollback
  • Names the clash with #481 (apply_discounts, charge_payment) and #486 (send_confirmation_email) and decides how each is handled (land first, wait, fold in, route through the new seam, or a short freeze window), rather than moving that code under the open PRs; characterisation tests are not delayed by either PR
  • The scope fence keeps the confirmation-email-on-failed-payment behaviour (INC-715) and the rounding out of the refactor as behaviour changes, and says new findings go to the debt register
  • Says the plan can stop after any step with the code better than before, and gives a reason the first real step goes first (for example the planned partial refunds or the in-flight gift-card work)
  • Does not invent modules, metrics or numbers not in the input; every file and function cited exists in the four files
  • Answers at the size of the ask: the plan is in the reply as a numbered sequence of steps of a line or two each, not a long multi-section document, since no document was requested
  • Produces the plan without asking questions first, with its assumptions tagged [assumption] at the top
  • Pulls the env var rename out of the refactor as a behaviour change, and either sends it to its own later parallel change (read both names, then drop the old) or to a separate ticket, instead of folding it into the steps
  • Uses 'about 40 direct os.environ reads' as the baseline and does not invent other measured numbers such as test times or file counts; anything else to measure is marked not measured
  • Introduces the settings object first and moves call sites over in batches of one PR each, so main stays releasable between batches
  • Characterisation tests pin how values are parsed today (defaults when unset, how strings like 'false' or '0' are read) before call sites move
  • The stop point is zero direct os.environ reads outside the settings object, enforced by a check in CI or a test
  • Does not invent file names, module names or settings that the prompt did not mention
  • Builds the order from imports.txt as a prerequisite graph (mikado-style or explicit dependency order): the goal, the import edges blocking it, and which edges change cleanly first, with the reason
  • States the target as the count of billing-to-web imports in imports.txt (seven) going to zero, or the four in invoices.py and dunning.py that block the worker as the first milestone, with the final stop point enforced by an import check in CI
  • Puts characterisation tests for billing/proration.py before any step that changes it, because the register row says it has no tests, and relies on tests/billing/ for invoices and dunning
  • Sequences the edges that block worker/nightly.py (in billing/invoices.py and billing/dunning.py) before the proration and tax edges the worker doesn't need, and says the plan can stop with the worker unblocked and the code better
  • Pulls the invoices.total_cents column change out of the refactor as a schema change with data to migrate, to be planned separately as a migration, and does not fold it into any step
  • Every step cites the specific import line or file from imports.txt it removes, and states its rollback
  • Does not invent modules, functions, metrics or numbers beyond imports.txt and register.md
  • Spots that src/partners/statement.py depends on build_invoice mutating the caller-supplied line_items list (it reads the appended tax line back from its own lines list, not from the returned invoice), and the plan either keeps that side effect working or moves that caller off it in its own step before the mutation is removed. It does not quietly make build_invoice pure.
  • Spots that reports/monthly_revenue.py has its own copied TAX_RATES and tax logic, which has already drifted (no US-TX, no EU reverse-charge rule). The plan puts it explicitly inside or outside the scope fence, and does not claim the refactor removes duplicated tax logic if the report is left out.
  • Puts characterisation tests for build_invoice first, before any extraction, and says they must cover the existing branches (short-period proration, annual anniversary month, included seats/annual add-on discount, usage tiers, each coupon type including expiry, tax-exempt and reverse charge, non-GBP conversion, credit notes, rounding). They pin current output, float-rounding quirks included. Notes that test_payments.py gives no coverage of invoice.py.
  • Deals with the clash with PR #412 (tax block and TAX_RATES, committed to customers for 1 Nov): it sequences the tax-block work after #412 merges, or otherwise avoids conflicting edits to that block while #412 is open. It also mentions draft PR #420 touching the add-ons block.
  • Keeps DEBT-15 (float to Decimal money) and any change to rounding or tax results out of scope as separate work, and states that every step preserves behaviour, i.e. invoice output is unchanged.
  • Breaks the work into small, separately mergeable PRs, each leaving main releasable. Each step names a specific technique that fits it, e.g. extract function per section (plan, add-ons, usage, coupons, tax, currency, credit note), or parallel change / expand-contract for the line_items parameter or the signature with 9 parameters.
  • Sets a measurable target with a baseline taken from the files (e.g. build_invoice is about 165 lines with 9 parameters and 0 tests; or the register's '2 incidents in Q3' / 'most of a sprint per tax region') or marked as 'to measure'. It also defines an explicit stop point where the refactor is finished.
  • Invents no facts: no made-up cyclomatic-complexity numbers, coverage percentages, incident counts, team sizes or dates beyond those in the inputs, and no claims that callers, modules or tests exist beyond those shown. New modules it proposes are clearly labelled as proposed. Unknowns, such as other callers of build_invoice, appear as checks to run (e.g. grep for build_invoice and line_items=), not as stated facts.
  • Flags that LegacyMailer.send returns true even when sending fails (the catch block returns true), and that src/orders/onOrderShipped.ts has a retryQueue branch on !sent that has never fired as a result. Warns that a new client which honestly returns false (or throws) would change behaviour, with possible retries or duplicate emails. Keeps the current semantics at the seam and treats surfacing errors as a separate, deliberate change.
  • Finds that scripts/nightly-digest.js has its own copy of the SMTP send and renderer, bypasses LegacyMailer, and skips the suppression list. Brings it into the plan, since the relay can't be switched off while it exists, and does not treat deleting legacyMailer.ts as finishing the migration.
  • Proposes an incremental migration through a seam (branch by abstraction and/or strangler fig): an interface the callers depend on, the new HTTP-provider implementation behind it, a per-template or per-caller switch via flag/config with a quick rollback, and legacy code deleted only once traffic on it is zero.
  • Lists the LegacyMailer behaviours that need characterisation tests before the swap: comma-splitting and lowercasing recipients, suppression-file filtering (and returning true when all recipients are suppressed), bccOps, per-template subjects including the 'there' fallback, missing vars rendering as empty, sendBulk's sequential 200ms pacing. Notes the existing test only covers render().
  • Handles each in-flight PR that matters: #88 (adds a template and subject inside legacyMailer.ts; must land for peak) is allowed to land first or is accommodated, not blocked. #91 (rewrites loadTemplate/templates to MJML) is flagged as a clash with the template-rendering step. #93 (new mailer.send caller for magic_link) is told to go through the new seam.
  • Sizes and sequences the steps against ci-notes.md (about 38 min e2e that flakes about 1 in 6, manual prod approval, about 2 deploys a week) and the end-of-Q1 deadline. Each step is its own small PR leaving main releasable. Includes a measurable target and stop point, e.g. templates or call sites still on the relay going from a baseline counted in the files to 0, or relay send volume reaching 0, before the contract ends.
  • Invents no facts: no named email vendor or specific API endpoints, no email volumes, error rates, team size or calendar dates beyond 'end of Q1', and no claim of call sites beyond those in the files. Unknowns (other callers of mailer, the provider's suppression handling, which templates are highest volume) are raised as questions or checks.
  • Delivers a usable plan in this reply despite the vague request, asking at most a few targeted questions, and recommends neither a big-bang cutover nor a feature freeze.
  • Flags that DunningJob.run() mutates invoices returned by findOverdue (status, dunningLevel, lastDunnedOn) without saving them, relying on Hibernate dirty checking inside @Transactional, and makes the plan handle it (an explicit save step or a test pinning that the changes persist) before findOverdue moves behind a non-JPA interface or the ORM changes.
  • Notices that InvoicePdfRenderer.taxFor is a copy of computeLineTax but rounds HALF_UP instead of HALF_EVEN, and does not silently merge the two as a behaviour-preserving step: it either fences the renderer out of scope or raises the mismatch as a decision for finance/the team (ADR-007 says tax rules are not to change).
  • Puts characterisation tests ahead of moving the untested logic, naming at least applyLateFees (grace days, exemption, the daysLate/30 multiplier, only raising the fee) and per-line tax rounding, since the existing tests only cover createInvoice; and says these tests should not need a database once the rules are extracted, or explains how they move from @SpringBootTest to plain unit tests.
  • Sequences around PR #412: does not start moving applyLateFees until #412 (approved, expected to merge this week) has landed, or explicitly rebases onto it; and names PR #415 (credit notes, changes void, about 3 weeks out) as a clash to coordinate with, e.g. by agreeing where issueCreditNote should live.
  • Sets a target taken from ADR-007's precondition, such as: invoice rule code has no jakarta.persistence imports and no EntityManager dependency, with the baseline read from the current file and a stop point at that precondition; the jOOQ migration itself is explicitly outside the scope fence, as are tax and late-fee rule changes.
  • Each step is a separate, small PR that leaves main releasable and behaviour unchanged, and each names a concrete technique that fits it (e.g. extract a pure InvoiceCalculator / late-fee policy, introduce a repository port in front of the EntityManager queries as branch by abstraction, parallel change for the findOverdue callers). It does not propose a rewrite or a long-lived branch.
  • Treats markPaid's native customer balance update (Customer.balance is mapped read-only) as persistence that has to stay atomic with the invoice save, and keeps it on the persistence side rather than moving it into the rules code.
  • Invents no modules, files, metrics, dates or numbers that are not in the provided files (no made-up coverage percentages, line counts, team sizes or PR numbers); anything new it proposes, such as a class name, is clearly labelled as a proposal.
  • The answer is short and easy to scan, matching a casual one-line question: a brief plan of a handful of steps, not a long formal document with many sections, and it does not ask a string of clarifying questions before giving a plan.
  • Respects the release: nothing beyond adding tests or trivial, zero-risk changes goes into CheckoutPanel before the 30 Oct code freeze / 3 Nov v4.8 release, and the structural refactor starts after release (or the plan explains why a specific step is safe to land before freeze).
  • Puts gift cards PR #231 (in v4.8, rewrites the totals block and submit(), expected merge by 28 Oct) ahead of any refactor of the same code, so the refactor does not conflict with it; and mentions #236 (address autocomplete, parked until after release) as touching the address section the plan would move.
  • Spots that MiniCart.tsx duplicates the totals maths but differs from CheckoutPanel (it rounds the discount per line instead of on the subtotal, and its free-shipping test is >= 5000 where CheckoutPanel uses > 5000), links this to WEB-512's 1p mismatch, and treats unifying them as a deliberate behaviour change needing a decision on which is correct, not a behaviour-preserving step.
  • Spots that OrderReview.tsx reads the 'checkout_draft' localStorage entry that CheckoutPanel writes (including total), and keeps that contract unchanged (or pins it with a test) when draft or totals logic is moved into a hook or module.
  • Asks for characterisation tests before moving logic, because the only existing test is a single snapshot: at least totals with a promo and the free-shipping threshold, and the draft write, as plain tests on behaviour rather than more snapshots.
  • Gives a target and stop point tied to something real in the input, for example WEB-530 (pay-by-bank in Q1 needs the payment step to stop assuming card fields) or pulling the totals and validation into pure functions, and says what is out of scope (e.g. not a redesign of the checkout flow).
  • Invents no files, metrics, dates or numbers not in the provided files; any count it quotes (e.g. how many useState calls there are or the file's length) matches the actual file, and proposed new hooks or modules are clearly labelled as proposals.
When it should and shouldn't activate

Should activate:

  • This 1,800-line OrderService is where every change goes wrong; plan how we untangle it without stopping feature work
  • TD-04 got approved for this quarter. Plan the refactor as small PRs we can ship one at a time
  • How do we break billing's dependency on the web package without a big-bang rewrite?
  • Give me a strangler-fig plan for replacing our legacy report builder module piece by piece
  • We want to stop calling the SMTP client from 14 places — plan the refactor with characterisation tests first

Should leave to another skill:

  • Go through our TODOs and incident notes and tell me which tech debt to pay down this quarter
  • Plan the cutover from MySQL to Postgres with a backfill and no downtime
  • Implement step 2 of the refactor plan: move calculate_tax into its own module with tests
  • Write an ADR for splitting the monolith into an orders service and a billing service
  • Review this refactoring PR before I merge it
  • Split the gift-card checkout epic into user stories

Hands off to

Related skills

adr-generator, design-doc, migration-plan, pre-merge-review, user-story-splitter

Source

SKILL.md · the whole folder on GitHub.