feat(refactor): require a test net and SRP, SPOT, KISS, DRY
Before editing, the skill now establishes a safety net: run existing coverage for a green baseline, or write characterization tests against the current behaviour where the code is testable logic. Those tests must still pass unchanged afterwards, so a test that had to be edited means the refactor moved behaviour and gets reverted. The refactor itself is now governed by SRP, SPOT, KISS and DRY, and no file containing program code may exceed 250 lines. Description and markup languages are exempt, as is a file the language or framework genuinely forbids splitting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -28,12 +28,51 @@ Never `git stash` and never revert a working-tree file to compare - other agents
|
||||
and the operator share this tree. Read the committed version with
|
||||
`git show HEAD:<path>` into a temporary file instead.
|
||||
|
||||
## Step 2: refactor each file
|
||||
## Step 2: get a safety net first
|
||||
|
||||
A refactor is only provably behaviour-preserving if something failed before it
|
||||
and passes after it. Before editing, check what covers the change set:
|
||||
|
||||
- **Coverage exists**: run it now and record the green baseline. A suite that
|
||||
was already red gets reported, not refactored around.
|
||||
- **No coverage and the code is testable logic**: write the characterization
|
||||
tests first, against the current behaviour - including the behaviour you
|
||||
consider wrong, so the refactor cannot silently change it. Keep them small
|
||||
and framework-free unless the project already has a framework. Fix the
|
||||
captured wrongness in a separate change, never in this one.
|
||||
- **Not sensibly testable**: pure glue, declarative config, a thin wrapper over
|
||||
a service the test would have to mock into meaninglessness, or a project with
|
||||
no test setup at all. Say so in one line and refactor without a net; do not
|
||||
invent a test harness the project never asked for.
|
||||
|
||||
The new tests are part of the deliverable and must still pass unchanged after
|
||||
the refactor. A test that had to be edited to stay green means the behaviour
|
||||
moved: revert the refactor, not the test.
|
||||
|
||||
## Step 3: refactor each file
|
||||
|
||||
Read the whole file first, then apply the standards that the surrounding code
|
||||
and the project's own config already establish (formatter config, linter rules,
|
||||
existing idioms) over any generic preference of your own:
|
||||
existing idioms) over any generic preference of your own.
|
||||
|
||||
Four principles govern the result:
|
||||
|
||||
- **SRP**: one file, one responsibility; one function, one reason to change. A
|
||||
unit that needs an "and" to describe it is two units.
|
||||
- **SPOT**: every fact - a constant, a path, a schema, a rule - lives in exactly
|
||||
one place and everything else references it.
|
||||
- **KISS**: the simplest construct that works. No abstraction without a second
|
||||
caller today, no configuration for a value that never varies.
|
||||
- **DRY**: the same logic is written once. A literal repeated three times is a
|
||||
missing constant.
|
||||
|
||||
Concretely:
|
||||
|
||||
- **File size**: no file containing program code exceeds 250 lines. Split the
|
||||
overflow along the SRP seam - it is the symptom, not the cause. Description
|
||||
and markup languages are exempt (Markdown, YAML, JSON, HTML, translation and
|
||||
data files), as is the rare file the language or framework genuinely forbids
|
||||
splitting: name that reason in the report, "it was easier" does not qualify.
|
||||
- **Duplication**: fold repeated logic into the helper that already exists;
|
||||
create a new one only when two or more call sites need it now.
|
||||
- **Dead weight**: unused imports, variables, parameters, branches that cannot
|
||||
@@ -50,8 +89,9 @@ Do not reformat regions you have no substantive fix for - cosmetic churn buries
|
||||
the real change in the diff. If a file's problem is architectural and cannot be
|
||||
fixed inside the change set, report it instead of half-doing it.
|
||||
|
||||
## Step 3: keep behaviour provable
|
||||
## Step 4: keep behaviour provable
|
||||
|
||||
- Re-run the step 2 net and require the same green result as the baseline.
|
||||
- Never fold a bug fix, a feature, or a dependency change into a refactor.
|
||||
Found a real bug? Name it in the report and leave it for a separate change.
|
||||
- Run the cheap checks the project already has for the touched files (formatter,
|
||||
@@ -60,7 +100,7 @@ fixed inside the change set, report it instead of half-doing it.
|
||||
- State plainly what was verified and what was not. An unverified refactor is
|
||||
reported as unverified.
|
||||
|
||||
## Step 4: report
|
||||
## Step 5: report
|
||||
|
||||
One line per file: what changed and why it is safe. Then one line naming what
|
||||
was deliberately left alone (architectural findings, bugs, files skipped) and
|
||||
|
||||
Reference in New Issue
Block a user