---
name: refactor
description: Full-workspace code quality analysis and refactoring with validation
user-invocable: true
allowed-tools: Read, Grep, Glob, Bash(cargo check*), Bash(cargo clippy*), Bash(cargo test*), Bash(cargo fmt*), Bash(cargo tree*), Bash(cargo audit*), Bash(cargo outdated*), Bash(cargo machete*), Bash(cargo deny*), Bash(cargo mutants*), Bash(cargo tarpaulin*), Bash(cargo udeps*), Bash(git status*), Bash(git diff*), Bash(git stash*), Bash(git log*), Bash(wc *), Bash(grep*), Edit, Write, Task
---

# Refactor

Full-workspace code quality analysis and incremental refactoring with continuous validation.

This skill is intended for **periodic whole-codebase refactoring passes**, not per-branch or per-diff cleanup. It does not accept `--diff` / `--staged` / `--since` arguments.

Platform note: agents that support frontmatter permissions may use `allowed-tools` and `user-invocable`. Other agents should ignore unsupported fields and follow the workflow below. When platform-specific orchestration or memory features are unavailable, degrade gracefully and continue with the equivalent local workflow.

Arguments (optional):
- Path or crate: `/refactor crates/app` — limit scope to one directory
- No arguments — analyze the entire workspace

## Process

### Phase 1: Exploration

1. Read project instruction and planning files if present, for example `AGENTS.md`, `CLAUDE.md`, and `PRD.md`. Also read any project or agent memory/notes files if present for `intentional_placeholder_*`, `feedback_*`, or similar notes that inform what to skip.
2. Collect baseline metrics — run in parallel, each one optional (if a tool is absent, emit an info line in the report, do not fail):
   - `cargo clippy --workspace --all-targets 2>&1` — warning count.
   - `cargo test --workspace 2>&1` — green baseline? If tests fail or code doesn't compile, report and stop.
   - Glob + `wc -l` — LOC per crate, files > 400 LOC.
   - `cargo tree -d` — duplicated dependencies.
   - `cargo audit` — CVE list. If severity ≥ Medium, flag but continue.
   - `cargo outdated --root-deps-only` — major-behind deps.
   - `cargo machete` — unused `[dependencies]` entries.
   - `cargo udeps` — unused deps (secondary, if machete absent).
   - `git log --numstat --since="6 months ago"` → top-10 files by churn (hotspot candidates for splitting).
   - `grep -rn "TODO\|FIXME\|HACK\|XXX" --include="*.rs"` — **from repo root, not from `crates/`** (see Rules below).
   - Cyclomatic-complexity proxy: `grep -c "match\|if\|for\|while"` on the ten largest `.rs` files.

### Phase 2: Diagnosis

Analyze code across the categories below. For large scope, use parallel subagents or parallel analysis passes if the platform supports them, ideally one per category or related group of categories.

#### Analysis checklist

**Architecture (SOLID)**
- [ ] Files > 400 LOC with multiple public types — splitting candidates.
- [ ] Modules with > 1 responsibility.
- [ ] Hard dependencies between modules (concrete types where a trait would decouple).
- [ ] Open/Closed violations: `match` on types that are expected to grow.

Legitimate exceptions to "1 file = 1 type": Error+ErrorKind pairs, Builder+Target, small private helpers (< 30 LOC), related DTO families (Request/Response), typestate marker types, newtype wrapper collections.

**Module coupling / cohesion**
- [ ] Crate dependency graph is unidirectional (core ← mid ← app).
- [ ] No circular deps between modules within a crate.
- [ ] No excessive `pub` — types/functions public without need (downgrade to `pub(crate)` / `pub(super)`).
- [ ] Crate does not know implementation details of another crate (leaky abstraction).
- [ ] Changing one module does not cascade into unrelated modules.

**Duplication (DRY)**
- [ ] Repeated code blocks (> 5 lines, > 2 occurrences).
- [ ] Similar `match` patterns across different files.
- [ ] Copy-pasted error handling.

**Dead code**
- Grep usages **from repo root**, not from `crates/`. Binary crate in `src/` is a legitimate caller (previous incident with `check_git_available` / `ram_usage_percent` was caused by grepping only `crates/`).
- [ ] Unused `use`, functions, types, constants.
- [ ] Commented-out code.
- [ ] Unreachable branches.
- Confidence tagging:
  - `HIGH` — unused private / `pub(crate)` in leaf module, no callers.
  - `MEDIUM` — `pub` in inner crate, no callers found via grep.
  - `LOW` — enum variant, trait impl, or item with `#[allow(dead_code)]` nearby — likely intentional placeholder.
- Skip items already noted as intentional in project or agent memory/notes files (e.g. `ForceSave`, dead enum variants in `vfs_state.rs`/`keyboard.rs`, `format_bytes` x3, `ConflictResolution` x3 across layers).

**Performance**
- [ ] Unnecessary `.clone()` / `.to_string()` / `.to_owned()` where a reference suffices.
- [ ] O(n²) loops (nested iterations over collections).
- [ ] Repeated HashMap/Vec lookups inside a loop.
- [ ] Allocations in hot paths (see also Allocations category below).

**Error boundaries**
- [ ] Consistent error types at crate boundaries (`From` / `thiserror`).
- [ ] `unwrap()` / `expect()` in production code — replace with `?` or explicit handling.
- [ ] Lost error context (`.map_err(|_| ...)` without information).
- [ ] Panic instead of Result at input boundaries (user, DB, network).

**Security**
- Integrate `cargo audit` / `cargo deny check licenses` output here (do not split into a separate step).
- [ ] SQL queries via string concatenation instead of parameterized.
- [ ] Secrets / tokens hardcoded or logged.
- [ ] Unvalidated user input (path traversal, XSS).
- [ ] `fs::*(user_input_path)` without `canonicalize()` + `starts_with()` check — path traversal risk.
- [ ] `Command::new(user_input)` or shell-expansion of user input — command injection.
- [ ] Magic numbers and hardcoded config that should be parameters.

**Style and idioms**
- [ ] Inconsistent naming.
- [ ] Missing `#[must_use]` on pure / builder methods.
- [ ] Over-abstractions with no usage.
- [ ] Uncovered edge cases in tests (empty input, errors, overflow).

**Concurrency** *(Rust-specific)*
- [ ] `Arc<Mutex<T>>` where locked guard is used mostly for reads (grep the ratio of `.lock()` read accesses vs mutations) — `Arc<RwLock<T>>` candidate.
- [ ] `.await` inside a `.lock()` guard — deadlock risk.
- [ ] `tokio::spawn` without an abort handle for long-running tasks (leaks on shutdown).
- [ ] Shared mutable state without `Send` / `Sync` markers where the compiler would accept them.

**API stability**
- [ ] `pub` enum without `#[non_exhaustive]` on a cross-crate boundary.
- [ ] `pub` in a child module re-exported via `pub use` only for one caller — downgrade candidate to `pub(crate)`.
- [ ] Breaking changes to `pub` API without a version bump.

**Allocations / hot path** *(TUI-specific)*
- [ ] `.to_string()` / `format!` / `Vec::new()` / `vec![]` inside `render_*`, `tick_*`, `draw_*`, `update_*` functions.
- [ ] Config cloned per frame — verify `Arc<Config>` pattern is used.
- [ ] `Cow<str>` candidates: functions taking `&str` that sometimes need an owned `String`.

**Testing quality**
- [ ] Non-hermetic tests: `std::env::set_var`, global state mutation inside `#[test]`.
- [ ] `#[ignore]` without a comment explaining why.
- [ ] Assertions without a message on complex equality (`assert_eq!` on a large struct).
- [ ] Coverage gap in critical paths (if `cargo-tarpaulin` is available, highlight files with < 50%).

**Observability**
- [ ] `println!` / `eprintln!` in production code (not `#[test]` / examples) — should be `log::*`.
- [ ] `let _ = <Result>` — errors swallowed without `log::warn!`.
- [ ] Long-running operations without tracing span or progress.

### Phase 3: Report and prioritisation

Each finding gets two tags:

```
Impact:  HIGH / MED / LOW
  HIGH  — > 100 LOC reduction OR removes a bug class OR unblocks a perf fix
  MED   — 20–100 LOC reduction OR consolidates a recurring pattern
  LOW   — < 20 LOC OR stylistic

Risk:    SAFE / CAREFUL / BREAKING
  SAFE      — private internals, no API surface change, covered by tests
  CAREFUL   — pub(crate) API, hot path, thread boundary
  BREAKING  — pub API, schema/config format, semantic change
```

Default execution order: `HIGH-SAFE → MED-SAFE → HIGH-CAREFUL → LOW-SAFE → MED-CAREFUL`. `BREAKING` findings are shown but never executed without explicit user approval.

Output format (terminal, do not create report files):

```
## Findings (ROI-sorted)

### 🟢 HIGH-SAFE (execute automatically)
1. **<short title>** — path/to/file.rs, path/to/other.rs
   Why: <one-sentence justification + evidence>.
   Effort: ~N min · Risk: SAFE (<reason>).

### 🟡 MED-CAREFUL (propose, require approval)
2. **<short title>** — path/to/file.rs
   Why: …
   Effort: ~N min · Risk: CAREFUL (<reason>).

### 🔴 BREAKING (report only, do not execute)
3. …

### 📦 Ecosystem
- cargo-audit: N advisories.
- cargo-outdated: N major-behind deps.
- cargo-machete: N unused crates.
- cargo-tarpaulin: workspace coverage X% (only if available).

## Category counts

| Category     | Found | HIGH | MED | LOW |
|--------------|------:|-----:|----:|----:|
| Architecture |   …   |  …   |  …  |  …  |
| …            |   …   |  …   |  …  |  …  |

Baseline: X clippy warnings, Y workspace LOC, Z tests passing.
```

**Auto-mode:**
- If auto-mode is detected by surrounding context, do not pause for interactive questions.
- Auto-execute all `HIGH-SAFE` and `MED-SAFE` findings.
- Leave `CAREFUL` and `BREAKING` in the report for the user to review.

**Non-auto:**
- Ask the user which categories to fix using the platform's interactive prompt mechanism, with `SAFE` findings preselected when supported.
- Ask for budget: quick cleanup / full refactoring.

### Phase 4: Execution

For each fix:
1. Read the file, understand context.
2. Make the change via `Edit`.
3. `cargo check` — still compiles?
4. If behaviour is affected — `cargo test --workspace`.
5. Move to the next fix.

On compilation or test failure — revert the change and move on.

Commit boundaries: do **not** auto-commit. Let the user review the final diff and commit.

### Phase 5: Validation

```bash
cargo fmt --check
cargo clippy --workspace --all-targets -- -D warnings
cargo test --workspace
```

Compare with baseline: warnings removed, LOC deleted/changed, tests still green.

## Rules

- Grep for dead code **from the repository root**, not from `crates/`. Binary crate in `src/` is a legitimate caller.
- Before proposing a dead-code or refactor finding, cross-check project or agent memory/notes files for `intentional_placeholder_*` entries and skip those silently.
- If an ecosystem tool (`cargo-machete`, `cargo-mutants`, `cargo-audit`, etc.) is not in PATH, emit an info line and continue — do not fail.
- `BREAKING` findings are reported but never executed without explicit user consent.
- In auto-mode, do not interrupt execution for `SAFE` actions.
- Do not create abstractions "for the future" — only when duplication already exists.
- Do not change public API without explicit consent.
- Do not touch files you haven't read.
- Do not expand refactoring beyond the agreed scope.
- Atomic changes: one fix at a time, verify after each.
- Output to terminal, not to report files.
- Communicate in the user's language.