Refactor

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.