Harness Refactoring

# Harness Refactoring

> Safe refactoring with constraint verification at every step. Change structure without changing behavior, with harness checks as your safety net.

## When to Use

- When improving code structure, readability, or maintainability without changing behavior
- When reducing duplication (DRY refactoring)
- When moving code to the correct architectural layer
- When splitting large files or functions into smaller, focused ones
- When renaming for clarity across the codebase
- After completing a feature (post-TDD cleanup beyond single-cycle refactoring)
- NOT when adding new behavior (use harness-tdd instead)
- NOT when fixing bugs (use harness-tdd — write a failing test first)
- NOT when the test suite is already failing — fix the tests before refactoring

## Process

### Iron Law

**All tests must pass BEFORE you start refactoring and AFTER every single change.**

If tests are not green before you start, you are not refactoring — you are debugging. Fix the tests first. If tests break during refactoring, undo the last change immediately. Do not try to fix forward.

### Phase 1: Prepare — Verify Starting State

1. **Run the full test suite.** Every test must pass. Record the count of passing tests — this number must not decrease at any point.

2. **Run `harness validate`** and **`harness check-deps`**. Both must pass. You are establishing a clean baseline. If either reports issues, fix those first (that is a separate task, not part of this refactoring).

3. **Run `compute_blast_radius` on target files** to understand downstream impact before refactoring. This reveals which modules, tests, and consumers will be affected by structural changes.

4. **Identify the refactoring target.** Be specific: which file, function, class, or module? What is wrong with the current structure? What will be better after refactoring?

5. **Plan the steps.** Break the refactoring into the smallest possible individual changes. Each step should be independently committable and verifiable. If you cannot describe a step in one sentence, it is too large.

### Graph-Enhanced Context (when available)

When a knowledge graph exists at `.harness/graph/`, use graph queries for faster, more accurate context:

- `get_impact` — precise impact analysis: "if I move this function, what breaks?"
- `query_graph` — find all transitive consumers, not just direct importers

Catches indirect consumers that grep misses. Fall back to file-based commands if no graph is available.

### Uncertainty Surfacing

When you encounter an unknown during refactoring, classify it immediately:

- **Blocking:** Cannot determine if the change is purely structural without resolving this (e.g., unclear whether callers rely on implementation detail). STOP and surface to human.
- **Assumption:** Can proceed if assumption is stated (e.g., "no external consumers of this internal API"). Document the assumption and continue. If wrong, revert.
- **Deferrable:** Does not affect the current refactoring step (e.g., whether a further refactoring would be beneficial). Note for future consideration.

Do not guess whether a change is behavioral or structural. If you are unsure, it is blocking.

### Phase 2: Execute — One Small Change at a Time

For EACH step in the plan:

1. **Make ONE small change.** Examples of "one small change":
   - Rename one variable or function
   - Extract one function from a larger function
   - Move one function to a different file
   - Inline one unnecessary abstraction
   - Replace one conditional with polymorphism
   - Remove one instance of duplication

2. **Run the full test suite.** All tests must pass. If any test fails:
   - **STOP immediately.**
   - **Undo the change** (git checkout the file or revert manually).
   - **Analyze why it broke.** Either the change was not purely structural (it changed behavior) or the tests are coupled to implementation details.
   - **Try a smaller step** or a different approach.

3. **Run `harness validate` and `harness check-deps`.** Both must pass. A refactoring that fixes code structure but violates architectural constraints is not safe.

4. **Commit the step.** Each step gets its own commit. The commit message describes the structural change: "extract validateInput from processOrder" or "move UserRepository to data-access layer."

5. **Repeat** for the next step in the plan.

### Phase 3: Verify — Confirm the Refactoring is Complete

1. **Run the full test suite one final time.** Same number of passing tests as Phase 1.

2. **Run `harness validate` and `harness check-deps` one final time.** Clean output.

3. **Run `detect_stale_constraints`** to verify architectural constraints are still valid after structural changes. Refactoring can render existing constraints obsolete or misaligned.

4. **Run `check_traceability`** to confirm refactoring didn't break requirement-to-implementation mappings. Moved or renamed code may orphan traceability links.

### Graph Refresh

If a knowledge graph exists at `.harness/graph/`, refresh it after code changes to keep graph queries accurate:

```
harness scan [path]
```

Skipping this step means subsequent graph queries (impact analysis, dependency health, test advisor) may return stale results.

5. **Review the cumulative diff.** Does the final state match the intended improvement? Is the code genuinely better, or just different?

6. **If the refactoring introduced no improvement,** revert the entire sequence. Refactoring for its own sake is churn.

## Common Refactoring Patterns

### Extract Function

**When:** A function is doing too many things, or a block of code is reused in multiple places.
**How:** Identify the block. Ensure all variables it uses are either parameters or local. Cut the block into a new function with a descriptive name. Replace the original block with a call to the new function.
**Harness guidance:** If the extracted function belongs in a different layer, move it there AND update the import. Run `harness check-deps` to verify the new import respects layer boundaries.

### Move to Layer

**When:** Code is in the wrong architectural layer (e.g., business logic in a UI component, database queries in a service).
**How:** Create the function in the correct layer. Update all callers to import from the new location. Delete the old function. Run `harness check-deps` after each step.
**Harness guidance:** This is where `harness check-deps` is most valuable. Moving code between layers changes the dependency graph. The tool will tell you immediately if the move created a violation.

### Split File

**When:** A file has grown too large or contains unrelated responsibilities.
**How:** Identify the cohesive groups within the file. Create new files, one per group. Move functions/classes to their new files. Update the original file to re-export from the new files (for backward compatibility) or update all callers.
**Harness guidance:** Run `harness validate` after splitting to ensure the new files follow naming conventions and are properly structured. Run `harness check-deps` to verify no new boundary violations.

### Inline Abstraction

**When:** An abstraction (class, interface, wrapper function) adds complexity without value. It has only one implementation, is never extended, and obscures what the code actually does.
**How:** Replace uses of the abstraction with the concrete implementation. Delete the abstraction. Run tests.
**Harness guidance:** Removing an abstraction may expose a layer violation that the abstraction was hiding. Run `harness check-deps` to check.

### Rename for Clarity

**When:** A name is misleading, ambiguous, or no longer reflects what the code does.
**How:** Use your editor's rename/refactor tool to change the name everywhere it appears. If the name is part of a public API, check for external consumers first.
**Harness guidance:** Run `harness check-docs` after renaming to detect documentation that still uses the old name. AGENTS.md, inline comments, and doc pages may all need updating.

## Harness Integration

- **`harness validate`** — Run before starting, after each step, and at the end. Catches structural issues, naming violations, and configuration drift.
- **`harness check-deps`** — Run after each step, especially when moving code between files or layers. Catches dependency violations introduced by structural changes.
- **`harness check-docs`** — Run after renaming or moving public APIs. Catches documentation that references old names or locations.
- **`harness cleanup`** — Run after completing a refactoring sequence. Detects dead code that the refactoring may have created (unused exports, orphaned files).
- **`compute_blast_radius`** — Run in Phase 1 PREPARE before making changes. Reveals downstream impact of target files so you understand what the refactoring will affect.
- **`detect_stale_constraints`** — Run in Phase 3 VERIFY after refactoring. Checks whether architectural constraints are still valid after structural changes.
- **`check_traceability`** — Run in Phase 3 VERIFY after refactoring. Confirms requirement-to-implementation mappings were not broken by moved or renamed code.

## Success Criteria

- All tests pass before, during, and after refactoring (same count, same results)
- `harness validate` passes at every step
- `harness check-deps` passes at every step
- Each step is an atomic commit with a clear structural description
- The code is measurably better after refactoring (clearer names, less duplication, correct layering, smaller functions)
- No behavioral changes were introduced (the test suite is the proof)
- No dead code was left behind (run `harness cleanup` to verify)

## Red Flags

| Flag                                                                                    | Corrective Action                                                                                                                                                            |
| --------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| "This refactoring is safe, I don't need to run tests after this small change"           | STOP. The Iron Law is absolute: tests after EVERY change. "Small" and "safe" are the changes that introduce subtle bugs.                                                     |
| "I'll combine these two renames into one commit since they're related"                  | STOP. One change per commit. Combined changes make it impossible to isolate which change caused a regression.                                                                |
| "The failing test is testing implementation details, so I'll fix the test"              | STOP. Changing tests during refactoring is a warning sign. Verify the test is actually testing implementation details — not behavior your refactoring inadvertently changed. |
| `// removed old implementation` or `// TODO: move back later` replacing functional code | STOP. Either the code lives in its new location or it doesn't. Comments are not migration plans. Keep the code or delete it with a test proving the deletion is safe.        |

## Rationalizations to Reject

| Rationalization                                                                                                     | Reality                                                                                                                                                     |
| ------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- |
| "The tests are mostly passing, so I can start refactoring and fix the remaining failures as I go"                   | All tests must pass BEFORE refactoring starts. If tests are not green before you start, you are not refactoring -- you are debugging.                       |
| "This refactoring changes a small amount of behavior, but it is a clear improvement"                                | Refactoring must not change behavior. The test suite is the proof. If the refactoring requires changing tests, you may be changing behavior.                |
| "I will make several changes at once and run tests at the end since each change is small"                           | Tests must run after EVERY single change. If a test breaks, you must undo the LAST change immediately.                                                      |
| "The refactoring did not produce a measurable improvement, but the code is different so it must be somewhat better" | If the refactoring introduced no measurable improvement, revert the entire sequence. Refactoring for its own sake is churn.                                 |
| "I will refactor this and add the new feature in the same pass to be efficient"                                     | Refactoring and feature work are separate tasks. Mixing them means test failures could be from the refactoring OR the new behavior — you cannot tell which. |
| "The test suite is slow, so I will run tests only at the end of the refactoring sequence"                           | Each step must be independently verified. A slow test suite is a separate problem to solve — it is not a reason to skip the safety net.                     |

## Examples

### Example: Moving business logic out of a UI component

**Target:** `src/components/OrderSummary.tsx` contains a `calculateDiscount()` function with complex business rules. This logic belongs in the service layer.

**Step 1:** Create `src/services/discount-service.ts` with the `calculateDiscount` function copied from the component.

- Run tests: pass
- Run `harness check-deps`: pass (new file, no violations)
- Commit: "extract calculateDiscount to discount-service"

**Step 2:** Update `OrderSummary.tsx` to import `calculateDiscount` from `discount-service` instead of using the local function.

- Run tests: pass
- Run `harness check-deps`: pass (UI importing from service is allowed)
- Commit: "update OrderSummary to use discount-service"

**Step 3:** Delete the original `calculateDiscount` function from `OrderSummary.tsx`.

- Run tests: pass
- Run `harness check-deps`: pass
- Run `harness cleanup`: no dead code detected
- Commit: "remove duplicate calculateDiscount from OrderSummary"

**Final verification:** 3 steps, 3 commits, all tests green throughout, all harness checks passing. The business logic is now in the correct layer.

## Escalation

- **When tests fail during refactoring and you cannot figure out why:** Revert to the last green commit. The test failure means the change was not purely structural. Analyze the test to understand what behavioral assumption it depends on, then plan a different approach.
- **When `harness check-deps` fails after a move:** The code you moved may have dependencies that are not allowed in its new layer. You may need to refactor the moved code itself (remove forbidden imports) before it can live in the new layer.
- **When a refactoring requires changing tests:** This is a warning sign. If the tests need to change, the refactoring may be changing behavior. The only valid reason to change tests during refactoring is if the tests were testing implementation details (not behavior) — and in that case, fix the tests first as a separate step before refactoring.
- **When the refactoring scope keeps growing:** Stop. Commit what you have (if it is clean). Re-plan with a smaller scope. Large refactorings should be broken into multiple sessions, each leaving the code in a better state.