safe-refactor
Guide safe refactoring of complex, untested code using a multi-phase hybrid approach: characterization tests → agent-assisted refactoring → manual review → integration testing → staged rollout. Use th
SKILL.mdskills/safe-refactor/SKILL.md
GitHubSafe Refactoring with Hybrid Approach
When to Use This Skill
✅ Use when:
- Refactoring complex, high-risk code with zero or minimal test coverage
- Delegating refactoring work to an agent or external developer
- Need to guarantee behavior preservation across changes
- Dealing with mission-critical functions where regressions are expensive
❌ Skip when:
- Code already has 80%+ test coverage (just keep tests green)
- Change is trivially small and isolated (rename a variable, extract a constant)
The Hybrid Approach: 5 Safety Layers
| Layer | Who | Goal |
|---|---|---|
| 1. Characterization tests | You | Lock current behavior before any changes |
| 2. Agent refactoring | Agent | Refactor while keeping all tests green |
| 3. Manual code review | You | Catch logic errors tests can't find |
| 4. Integration testing | You | Verify end-to-end flows still work |
| 5. Staged rollout | You | Monitor in staging before full production |
Phase 1: Generate Characterization Tests
Goal: Capture current behavior as a golden master — before touching a line of code.
⚠️ Run these tests against the original code first to confirm they pass. A characterization test that never passed is worthless.
1a: Identify What to Test
Focus on:
- Public API surface (functions called by other modules)
- Business-critical logic (money, auth, data transforms)
- Functions with side effects (DB writes, file I/O, external calls)
- Complex conditional trees
1b: Design Test Cases Per Function
For each function, capture 3–5 cases:
| Scenario | What to verify |
|---|---|
| Happy path | Correct output, expected side effects |
| Boundary / edge | Empty input, max values, zero, null |
| Error / invalid input | Exception type, error message, no partial writes |
| Concurrency (if relevant) | No race conditions, correct locking |
Template per test case:
Function: processPayment(amount, card, expiry)
Input: {amount: 100.00, card: "4532123456789010", expiry: "12/25"}
Output: {status: "SUCCESS", transaction_id: <non-null>, fee: 2.50}
Side fx: DB INSERT into payments table (verify row exists)
Timing: < 500ms
1c: When Characterization Tests Reveal Bugs
Sometimes you'll discover the current code is wrong. Don't blindly preserve bugs.
- Obvious bug (off-by-one, null dereference): Fix it separately before refactoring. Keep the fix in its own commit.
- Ambiguous behavior (undocumented edge case): Flag it as a known issue; preserve the behavior for now and file a follow-up.
- Intentional quirk (backward-compat hack): Document it clearly in the test with a comment explaining why.
Phase 2: Brief the Agent
Paste this template directly into your agent prompt — fill in the bracketed fields:
REFACTORING TASK
================
OBJECTIVE:
Refactor [FILE OR FUNCTION] to [SPECIFIC IMPROVEMENT — e.g., "reduce cyclomatic
complexity", "migrate from callbacks to async/await", "split into smaller functions"].
Do NOT add new features or change behavior.
HARD CONSTRAINT — ALL TESTS MUST PASS:
A characterization test suite is at [TEST PATH]. Run it with:
[TEST COMMAND — e.g., pytest tests/characterization/, npm run test:char]
Before submitting:
1. Make your changes
2. Run the full test suite
3. If any test fails:
a. Determine if it's a test bug or a regression you introduced
b. If it's a test bug, explain why in your submission notes
c. If it's your regression, fix it and retest
4. Do NOT submit until all tests are green
QUALITY GATES:
- No new lint warnings (run: [LINT COMMAND])
- No new type errors (if applicable)
- All functions still exported with same signatures
- Comments updated to reflect new logic
SUBMIT FORMAT:
Reply with:
1. Summary of changes made
2. Test results (paste output)
3. Any flagged test bugs with justification
4. Any concerns or questions about the codebase
WHAT NOT TO DO:
- Do not refactor things outside the specified scope
- Do not change function signatures
- Do not add dependencies
- Do not "improve" behavior — only improve structure
Handling Agent Escalations
The agent may come back with one of these situations:
| Agent says | Your response |
|---|---|
| "Test X seems wrong — the code clearly should return Y" | Review the test. If the agent is right, fix the test and re-run. |
| "I couldn't keep test X green without changing the signature" | This signals a deeper problem. Pause and discuss scope. |
| "I added a dependency to make this cleaner" | Reject unless you approved it. Ask for a version without it. |
| "Some tests are flaky" | Investigate before proceeding — flaky tests mask real regressions. |
Phase 3: Manual Code Review
Goal: Catch logic errors that tests can't detect — wrong algorithm, subtle off-by-one, silent data corruption.
Focus your review time on top 3–5 highest-risk functions only.
Review Checklist
For each critical function, compare before/after:
- Algorithm unchanged: Same logical steps, same order of operations?
- Conditionals preserved: Every
if/elsebranch still present and correct? - Loop termination: Conditions and exit logic unchanged?
- Side effects intact: All DB writes, API calls, file ops still happen?
- Error handling: Exceptions still caught and re-raised correctly?
- Type handling: No implicit coercions introduced?
- Variable mutation: Are assignments in the same order? (order matters for side effects)
Red Flags That Demand Deeper Investigation
- A
forloop became amap/filter— check edge cases on empty input - An
if/elsechain was restructured — verify all branches with a truth table - Multiple functions were merged or split — confirm all logic is still reachable
- Error handling was "cleaned up" — verify error paths aren't silently swallowed
Review Pattern
FUNCTION: validateUser
RISK: HIGH (auth logic)
BEFORE: checks role first, then expiry, then IP allowlist
AFTER: checks expiry first, then role, then IP allowlist
VERDICT: ⚠️ Order changed. Ask agent: does order matter here?
(If role check short-circuits on invalid role, expiry check was
never reached for invalid roles. New order changes that behavior.)
Phase 4: Integration Testing
Goal: Verify end-to-end flows that span multiple functions.
Integration Test Template
FLOW: [Name — e.g., "User checkout with payment"]
SETUP:
1. [Seed test data]
2. [Reset mocks / external stubs]
STEPS:
1. [User action 1]
2. [User action 2]
...
VERIFY:
✓ [Expected DB state]
✓ [Expected API response]
✓ [Expected file/email/queue output]
✓ No orphaned records
✓ No unhandled errors in logs
PERFORMANCE:
Baseline: [X ms, measured on original code]
Threshold: < [X * 1.10] ms (allow 10% variance)
How to establish your baseline if you don't have one:
- Run the integration flow 5 times on the original code
- Take the median execution time
- Set alert threshold at +10%
Phase 5: Staged Rollout
Deployment Gates
GATE 1 — STAGING
Deploy → Run full suite → Monitor 1–2 hours
Watch: error rate, latency, unexpected exceptions
→ Green? Proceed. Red? Rollback.
GATE 2 — PRODUCTION CANARY (10–20% traffic)
Monitor 30 minutes
Watch: error rate ≤ baseline + 5%, latency ≤ baseline + 10%
→ Green? Proceed. Red? Rollback.
GATE 3 — PRODUCTION FULL
Monitor 2–4 hours
Keep rollback ready
What to Monitor
| Metric | How to set baseline | Alert threshold |
|---|---|---|
| Error rate | Avg over last 7 days in production | Baseline + 5 percentage points |
| P95 latency | Measure on staging with original code | Baseline × 1.10 |
| DB query count | Log slow queries on staging | + 50% vs baseline |
| Business metric | Revenue / conversions / key actions | Any statistically significant drop |
Rollback Plan
Trigger rollback if:
- Any critical test failure in staging
- Error rate increases by more than 5 percentage points
- Latency degrades more than 10%
- User reports data loss or corruption
Procedure (target: < 15 minutes):
1. Alert on-call team
2. Pause new ingestion if safe to do so
3. Revert:
git revert <commit> # code rollback
kubectl rollout undo ... # if Kubernetes
4. Wait for deploy (~2–3 min)
5. Verify error rate returns to baseline
6. Check DB for consistency (orphaned records, partial writes)
7. Document: what failed, what the tests missed, how to prevent recurrence
Workflow Summary
Delegating to an Agent
1. Write characterization tests locally (1–2 hrs)
→ Confirm they pass against original code
2. Give agent: code + tests + Phase 2 brief
→ Agent refactors, returns code + test output
3. Manual review: top 3–5 functions (30–45 min)
→ Focus on algorithm correctness, not style
4. Run integration tests (15–30 min)
→ Verify end-to-end flows
5. Deploy to staging → monitor → canary → full
→ Keep rollback ready throughout
Doing It Yourself
Same sequence — skip the agent brief and do the refactoring yourself between steps 1 and 3.
Common Pitfalls
| Pitfall | Why it bites you | Prevention |
|---|---|---|
| Skip characterization tests | No way to know what broke | Non-negotiable: write them first |
| Add features during refactor | Mixes concerns, harder to debug | Separate commits: refactor, then feature |
| Use different test data for validation | Can't compare apples to apples | Same inputs, always |
| Ignore performance metrics | Silent 3× slowdown ships | Measure baseline before touching code |
| Deploy directly to production | First real test is on users | Always stage first |
| No rollback plan | Scrambling during an incident | Write it before you deploy |
| Trust flaky tests | They hide real regressions | Fix or skip flaky tests explicitly |