Refactor

---
name: refactor
description: Refactor and optimize [code]
argument-hint: [file-or-function, a named area (e.g. "the auth module"), or empty for the fix ledger]
allowed-tools: Read, Edit, Grep, Glob, Bash(npm test:*), Bash(npx jest:*), Bash(npx vitest:*), Bash(pnpm test:*), Bash(yarn test:*), Bash(pytest:*), Bash(python:*), Bash(go test:*), Bash(cargo test:*), Bash(make test:*), Bash(git diff:*), Bash(git grep:*), Bash(git status:*), Bash(git rev-parse:*), Bash(git switch:*)
disable-model-invocation: true
---
Refactor $ARGUMENTS. A targeted refactor includes the performance pass
below — it is on by default, not a separate command.

## Guardrails
- **Spawn a worker, mid tier stated explicitly** (omitted inherits the
  parent's, not the balanced one; not cheapest/fastest either — judgment
  degrades there; never a vendor model name). Fall back to running inline if
  your tool has no subagent mechanism.
- **Escalate, never assume.** Anything you cannot decide, cannot verify, or
  that this spec does not cover → **stop and report it to the orchestrator**
  (the main session). Never improvise, never widen scope, never fix a side
  issue you noticed along the way.
- **The worker does the work itself — no delegation.** The fixer must **not**
  spawn subagents of its own. Every edit it reports, and every test run it
  cites, has to be one it made or ran with its own tool calls: a relayed "I
  fixed it and the suite is green" from a sub-worker is hearsay, and this
  command's whole output is the claim that a change landed and the tests still
  pass. A fix that delegates its work is a report about a report.
- **The HITL gates below belong to the orchestrator, not the worker.** A
  subagent cannot hold a conversation with the user, so it cannot run a gate
  that ends in *stop and ask*. When one trips — a failing test, a crossed
  public API boundary, a change bigger than the bullet asked for — the worker
  **stops there and hands the situation back**, with the options and its
  reasoning but no choice made. The orchestrator asks. A worker that picks
  revert / patch / update-test on the user's behalf has answered a question it
  was never allowed to ask.
- **Edit only what a surviving bullet names.** Ledger mode's scope is the
  bullets that survive revalidation, one change per bullet — not the
  neighbouring code, not the formatting, not a second finding noticed on the
  way past. Anything else goes back to the orchestrator to become a new
  bullet.
- **Prove the blast radius with two checks, because neither sees what the
  other does.** `git status --porcelain` at exit must list only files a
  surviving bullet named — that is this command's scope guarantee, and unlike
  `/branch-review` it is not expected to be empty. It cannot police the
  memory directory: `.amp/` is normally gitignored, so porcelain stays
  empty whether you deleted a fixed bullet, wrote nothing, or overwrote
  `MEMORY.md`. So also hash the files there before you start and again before
  you report:
  ```
  find .amp/remember -maxdepth 1 -type f -exec md5sum {} + | sort -k2
  ```
  and show the comparison: only `fix-ledger.md` may differ. `last-review.md` in
  particular is `/branch-review`'s to write —
  a fixer that touches it forges the gate that judges its own work.

## Ledger mode — `$ARGUMENTS` empty
Work through `.amp/remember/fix-ledger.md`, the non-blocking findings
`/self-review` and `/branch-review` have accumulated. Everything below (goals, constraints,
verification, HITL gates) still applies; this section only says what to
refactor and how to close each item.

1. **Tree must be clean and not on `main`.** The orchestrator runs this check
   before spawning the worker, so a dirty tree costs no worker; the worker
   then re-runs it as its own first act. `git status --porcelain` non-empty
   → stop, say what is uncommitted. On `main` → `git switch -c chore/fix-ledger`.
2. **Ledger missing or has zero bullets** → say so and stop. Nothing to do.
3. **Revalidate every bullet first, fix nothing yet.** For each: `git grep -F
   "<snippet>" -- <path>`. **No hit → delete the bullet** and list it as
   "cleaned by other work". Hit → re-read the surrounding code; if the finding
   no longer holds, delete the bullet with a one-line reason. What survives is
   the work list.
4. **Fix only surviving `nit` bullets** (untagged bullets count as `nit`),
   one bullet per change, under the constraints below. Delete each bullet as
   its fix lands. **Surviving `change` and `idea` bullets: the worker lists
   them in its report ("left: change" / "left: idea") and changes nothing
   about them** — they need a behaviour change, a redesign or a build, not a
   refactor, and the worker cannot ask. A `nit` that turns out to need one is
   **retagged `change` in place**, not fixed and not left silently. The
   **orchestrator** then asks the user, per `change`/`idea` item: **keep**
   (stays in the ledger), **drop** (the orchestrator deletes the bullet) or
   **spec it** (the user describes it; it becomes its own task on its own
   branch *after* this run — never built in ledger mode, whose diff must have
   NO behavior changes).
5. Run the tests as described below. Then report: **fixed / dropped / left**
   with the reason per left item, ending with **N nits, K changes, I ideas** remaining
   — counted the same mechanical way `/branch-review` does:
   ```
   grep -c '^- ' .amp/remember/fix-ledger.md
   grep -cE '@ [0-9a-f]{7,40} · change$' .amp/remember/fix-ledger.md
   grep -cE '@ [0-9a-f]{7,40} · idea$' .amp/remember/fix-ledger.md
   ```
   First is the total bullet count, second is K, third is I; N = total − K − I.
6. **Hand it back; do not chain it.** Say plainly: **commit, then run
   `/branch-review`** on this branch — ledger mode is a fixer, not a review,
   and its diff gets the ordinary gate. That is a sentence you *say*, not a
   sequence you *run*. They are two separate calls and both are the user's:
   an answer of "commit", "yes" or "go" authorizes the commit and nothing
   after it. Never start `/branch-review` off the back of it. Observed in the
   field: a run chained the review onto the owner's "commit" and the owner
   objected.

## Where to look — broad targets only
Empty `$ARGUMENTS` is ledger mode; the surviving bullets are the scope, so skip
this. When `$ARGUMENTS` names a specific file or function, that is the scope;
skip this too. When it names a whole area ("the auth module", "clean this
up"), let
recent change decide where inside it to start: `git log --oneline -- <path>`
and weight the files that keep coming back. A refactor is an investment in the
*next* change to that code, so code nobody edits pays the worst return — say
which files you picked and what churn you saw.

**Do not edit yet.** After weighting by churn, list candidates — file(s) ·
what is wrong · the change proposed · strength (strong / worth exploring /
speculative) — then **stop and hand the list back**. The worker cannot ask;
the orchestrator asks the user which to do, and only picked candidates are
edited.

## Goals
- Reduce complexity
- Improve readability
- Apply DRY
- Better naming
- **Shrink the interface, not the pieces.** A caller should have to learn
  *less* after the refactor than before — fewer entry points, fewer
  parameters, fewer ordering rules and error modes to keep in mind. Splitting
  one messy function into five that the caller must now sequence itself makes
  the code worse: the same complexity, spread thinner, behind a bigger surface.
  Push complexity *inward*; private helpers inside the thing are fine, they are
  not part of what a caller must learn. This targets the surface callers
  *inside the change* see — narrowing something outside callers depend on is an
  API change, which is the public-API gate below, not a free win.
- **Apply the deletion test before you create anything.** Imagine the new
  function, class, file, or wrapper already deleted. If the complexity
  reappears, spread across its callers, it earns its place — create it. If the
  complexity simply vanishes, it was a pass-through: do not create it. Run the
  same test on what is already there; a pass-through you find is a deletion,
  not a refactor target. Deleting one that callers outside this change can see
  is the same public-API gate.
- Remove needless work — the performance pass below

In ledger mode these two are a *filter on the bullet's own fix*, never a licence
to hunt: shape the change a surviving bullet asked for so it shrinks the
interface, and do not create something the deletion test rejects. A
pass-through or fat interface you spot elsewhere goes back to the orchestrator
as a new bullet, exactly like a side perf finding.

## Performance — part of every targeted refactor
When `$ARGUMENTS` names a target, look for wasted work as well as messy
work: time and space complexity, N+1 queries, I/O inside a loop, needless
allocations, the same value recomputed repeatedly.

**Ground every finding before you touch it.** Performance claims are easy
to invent. A finding counts as **confirmed** only with at least one of:
- a profile, benchmark or log line showing call frequency or duration,
- the path sits on an obvious hot loop or per-request handler with real
  volume,
- the user supplied evidence in the request.

Without one of those it is **uncertain — report it, do not optimise it.**
Speculative optimisation is scope creep with a stopwatch.

Fix confirmed findings under the same constraints as any other refactor:
minimal change, one obvious shape, no behaviour change, no API change.
After each such edit, re-read the changed region and confirm it still
computes the same answer — a perf change that quietly alters semantics is
the worst kind. Report per finding: **location** (`file:line`), **cost**
(concrete — "N+1 over ~1k rows on every page load", not "could be
faster"), **change**, **expected improvement**, **trade-off**
(readability / memory / consistency).

In ledger mode the surviving bullets are the whole scope — do not add
perf findings of your own. One you notice goes back to the orchestrator
as a new bullet, like any other side finding.

## Constraints
- **NO behavior changes**
- Keep public API intact
- Existing tests must pass

Explain each change.

## After the refactor — verify it didn't break anything

"Existing tests must pass" is the load-bearing constraint, and the only
honest way to know is to run them.

1. **Detect the project's test command** (look for `package.json`
   scripts, `pytest.ini` / `pyproject.toml`, `go.mod`, `Cargo.toml`,
   `Makefile`). If none is found, **stop and ask** before claiming the
   refactor is done — silent green isn't acceptable.
2. **Run the tests.** Scope to the affected area when possible (`-t`,
   `--testPathPattern`, `pytest path/`, `go test ./pkg`); otherwise run
   the suite.
3. **Report** pass / fail counts and any failure's name + `file:line`.

**Stop and ask** when (HITL gates — not all the time, only here):
- `$ARGUMENTS` names a whole area — the candidate list above is handed back
  before any edit.
- a test **fails** after the refactor. Don't auto-revert (destroys
  work-in-progress) and don't push forward (the no-behavior-change
  constraint is broken). Present the failure and the options:
  **revert**, **patch the refactor**, or **update the test** (with
  reasoning).
- the refactor crossed a **public API boundary** that callers depend
  on — even if tests pass, downstream consumers may break.
- the change is **bigger than the user asked for** (scope creep —
  unrelated cleanups, formatting, comment edits). Confirm before
  applying.
- a perf fix has **multiple reasonable shapes** (cache vs precompute vs
  batch vs paginate vs index) — present the options with trade-offs, not
  a chosen path.
- a perf fix trades **correctness for speed** (lossy approximation,
  weaker or eventual consistency) — even when it is "obviously" faster.
- a perf fix touches **concurrency primitives** (locks, atomics,
  ordering) — easy to introduce a race.
- a perf fix changes a **DB schema, response shape or caller contract**.

Final report:
- **refactor done** — ready, OR
- **refactor done, but K tests fail** — awaiting direction (revert /
  patch / update test).

Either way the report carries this line, filled from the run:
```
tests: <cmd> exit <code> <totals> (scoped | full) | NOT RUN: <reason>
```

Plus the performance pass: **confirmed-and-fixed** · **confirmed-but-asking**
(why + options) · **uncertain** (what profiling or data would settle it) ·
**none found**.