refactor-deep · diff
git:20260924.3aceaab to git:20260924.17234b5
157 added, 134 removed. Audit A to A.
---
name: refactor-deep
description: Read-only comprehensive analysis of the branch against the remote default branch for large features - derives conventions per layer, hunts for correctness defects in the new code paths, and writes a prioritized refactor plan to ./tmp/. Usually run by the refactor orchestrator alongside refactor-simple; use directly on 10+ file changes.
---
- # Deep refactor
+ # Deep Refactor
- Comprehensive read-only analysis for large features and architectural
- changes, safe to run anytime. It checks the branch against the conventions of
- the repository you are in, hunts for correctness defects in new code paths,
- and writes a prioritized plan to `./tmp/`:
+ **Comprehensive read-only analysis for large features and architectural changes.**
- 1. Classify the change with detailed metrics.
- 2. Learn the repository's conventions, per layer.
- 3. Check each layer the diff touches against those conventions.
- 4. Hunt for correctness defects in new code paths: unguarded I/O, bypassed
- guards, lifecycle and cleanup gaps. This is the part that finds real bugs.
- 5. Check cross-cutting concerns: SOLID, DRY, documentation, error handling.
- 6. Write a prioritized plan.
+ Safe to run anytime. Performs deep analysis against the target repository's
+ own conventions, hunts for correctness defects in the new code paths, and
+ writes a detailed refactor plan without modifying files.
- This skill never applies fixes. Standalone, ask the user before running
- `refactor-apply`; under `refactor`, the orchestrator owns that gate.
+ ## What This Does
- ## When to use
+ Thorough code quality analysis that:
+ 1. Classifies changes with detailed metrics
+ 2. Learns the conventions of the repository you are in, per layer
+ 3. Analyzes each layer the diff touches against those conventions
+ 4. Hunts for correctness defects in the new code paths (the part that finds
+ real bugs - unguarded I/O, bypassed guards, lifecycle and cleanup gaps)
+ 5. Checks cross-cutting concerns (SOLID, DRY, documentation, error handling)
+ 6. Writes a comprehensive, prioritized plan to `./tmp/`
+ ## When to Use
+
- **Large features** (10-20 files, 500-1000 lines)
- - **Huge features** (20+ files, over 1000 lines)
- - Architectural changes that need thorough validation
- - A thorough pre-PR check for complex work
+ - **Huge features** (20+ files, >1000 lines)
+ - **Architectural changes** requiring comprehensive validation
+ - **Pre-PR comprehensive check** for complex work
- For small and medium changes, use `refactor-simple`. On a large PR, running
- both is coverage: their findings overlap by about half, and the rest is
- complementary. Run this analysis once per diff; repeated runs converge on the
- same findings.
+ For small/medium changes, use `refactor-simple` instead. Simple and deep are the whole
+ set; on a large PR their findings overlap by about half and the rest is
+ complementary, so running both is coverage, not redundancy. Do not run this
+ command three times and merge the results - repeated runs on the same diff
+ converge on the same findings, and a merge step has been shown to lose the
+ severe ones.
## Process
- ### Phase 0: Classification and convention discovery
+ ### Phase 0: Classification & Convention Discovery
- Diff against the merge-base with the remote default branch. A stale local
- `main` pulls unrelated commits into the review, and every finding in them is a
- false positive.
+ Diff against the merge-base with the remote default branch, never a bare local
+ `main` - a stale local `main` pulls unrelated commits into the review and every
+ finding in them becomes a false positive.
+ Resolve the PR/current branch's relevant remote and that remote's default
+ branch before running these commands. If either is ambiguous or unavailable,
+ report the blocked comparison; do not guess a remote or silently review an
+ empty diff. Record the resolved remote, base ref, and merge-base for helpers.
+
```bash
- BASE=$(git symbolic-ref -q refs/remotes/origin/HEAD | sed 's|refs/remotes/||')
- [ -n "$BASE" ] || BASE=origin/$(git remote show origin | sed -n 's/.*HEAD branch: //p')
- git fetch origin "${BASE#origin/}"
+ # Resolve the relevant remote from the PR/current branch configuration first.
+ # Set REFACTOR_REMOTE to that verified name; do not assume origin.
+ git remote show "$REFACTOR_REMOTE"
+ # Resolve its actual default branch and set BASE to the verified remote ref.
+ # Fetch that branch explicitly if the ref is missing/stale, then verify it.
+ git rev-parse --verify "$BASE^{commit}"
MB=$(git merge-base "$BASE" HEAD)
git diff "$MB" --name-status
git diff "$MB" --numstat
git diff "$MB" --stat
```
- `$BASE` is the remote's real default branch (`main`, `master`, `develop`).
- Diffing from the merge-base to the working tree (one revision) covers
- committed, staged, and unstaged work, so a pre-PR run sees uncommitted edits.
+ Also enumerate non-ignored untracked files with
+ `git ls-files --others --exclude-standard -z`. Treat these as added files:
+ include their full contents and handwritten line counts in sizing and analysis,
+ applying the same generated/vendor/lockfile exclusions. Pass this file inventory
+ to every analyzer; plain `git diff` omits it. Do not stage files to inspect them.
- If the branch is behind `$BASE`, note "rebase before merge" once. It is
- neither a finding nor a score penalty.
- Classify:
+ `$BASE` is the remote's real default branch (`main`, `master`, `develop`),
+ never an assumed name. Diffing from the merge-base to the working tree - one
+ revision, not two - includes committed, staged, and unstaged work, so a
+ pre-PR run sees the edits that are not committed yet.
- - **Size**: Large (500-1000 lines) | Huge (over 1000 lines) of hand-written
- change. Exclude lockfiles, generated files, and vendored directories, and
- name what you excluded.
- - **Type**: New Feature | Major Refactor | Enhancement
- - **Complexity**: Complex | Very Complex, judged on the change itself. A huge
- diff that is one mechanical operation is simpler than its size; say so.
- - **Layers**: as this repository names them (for example main, preload, and
- renderer in an Electron app; api and webapp in a monorepo)
- - **Modules**: the affected modules
+ If the branch is behind `$BASE`, note it once as "rebase before merge"; it is
+ not a finding and does not lower the score.
- Discover conventions per layer, taking them only from the repository you are
- in:
+ **Classify:**
+ - **Size**: Large (500-1000 lines) | Huge (>1000 lines) - of hand-written
+ change. Lockfiles, generated files, and vendored directories are excluded
+ from the count and named as excluded.
+ - **Type**: New Feature | Major Refactor | Enhancement
+ - **Complexity**: Complex | Very Complex - judged on the change, not the line
+ count. A huge diff that is one mechanical operation is not Complex; say so.
+ - **Layers**: list them as this repository names them (for example
+ main/preload/renderer in an Electron app; api/webapp in a monorepo)
+ - **Modules**: affected modules
- 1. Read `CLAUDE.md` and `AGENTS.md` at the root and in every directory the
- diff touches.
- 2. For each layer, read two or three exemplar files next to the changed code.
- Note how they import, structure code, handle errors, test, and document.
- 3. Before flagging a convention violation, `grep` how many existing files
- already do the thing. If the codebase does it everywhere, it is the
- convention.
+ **Discover conventions, per layer.** Conventions come from the repository you
+ are in, never from a rule remembered from another repo:
- For example, Doozy's `CLAUDE.md` states "zero relative imports" and a grep
- finds none, so a `../` there is Critical. Pane has hundreds of `../` imports
- and no alias, so the same line in Pane is fine. The repository decides.
+ 1. Read `CLAUDE.md` / `AGENTS.md` at the root and in every directory the diff
+ touches.
+ 2. For each layer, read two or three exemplar files that neighbour the changed
+ code and note how they import, structure, handle errors, test, and document.
+ 3. Before flagging any convention violation, confirm the convention exists
+ here - `grep` how many existing files already do the thing. If the codebase
+ does it everywhere, it is the convention, not a violation.
- Show the classification:
+ An import style is a finding only when the target repository's guidance and
+ existing code establish that convention. Explain the actual impact rather than
+ assigning severity from a preference remembered from another project.
+ **Show classification to user:**
```
📊 Change Classification:
Size: Large (15 files, 742 hand-written lines; lockfile excluded)
Type: New Feature (12 added, 3 modified)
Complexity: Complex (multiple modules)
Layers: [as the repo names them, with file counts]
Modules: [list]
Diff base: merge-base with $BASE at [sha], to working tree
📋 Conventions sourced from:
[the guidance files and exemplars read in step 1-2]
Proceeding with comprehensive analysis...
```
- ### Phase 1: Per-layer convention analysis
+ ### Phase 1: Per-Layer Convention Analysis
For each layer the diff touches, check the changed code against the
- conventions from Phase 0, citing the file that states each one. Fill these
- typical dimensions from the repository:
+ conventions discovered in Phase 0, citing the file that states each one.
+ Typical dimensions - fill them from the repository, do not assume:
- - **Import style**: alias or relative, ordering, allowed cross-layer imports
- - **Layering**: where logic may live (handlers or services, pages or hooks,
- main or renderer), and what stays inside its layer
- - **Error handling**: the repo's error types and propagation; where try/catch
- is expected and where a wrapper handles it
- - **State and data**: the repo's server-state, IPC, and persistence patterns
- and their invariants (cache keys, invalidation, cleanup)
- - **Structure**: file and folder placement, local versus shared code, when a
- subfolder with an index is expected
- - **Tests**: what the repo tests and how; whether new surface has the test
+ - **Import style**: alias vs relative, ordering, allowed cross-layer imports
+ - **Layering**: where logic is allowed to live (handlers vs services, pages
+ vs hooks, main vs renderer), and what must not leak across
+ - **Error handling**: the error types and propagation the repo uses; where
+ try/catch is expected and where a wrapper handles it
+ - **State and data**: the repo's server-state / IPC / persistence patterns and
+ the invariants that come with them (cache keys, invalidation, cleanup)
+ - **Structure**: file and folder placement rules, local-vs-shared conventions,
+ when a subfolder with an index is expected
+ - **Tests**: what the repo tests and how; whether new surface has a test
neighbouring code would have
- As an illustration only, here is a filled-in checklist for Doozy's monorepo.
- Derive your own for the repo you are in:
-
- - controllers use `authenticatedHandler` and hold no business logic
- - services extend `BaseService`, throw `ApiError`, and own all business logic
- - validators are Zod schemas outside controllers
- - pages are thin JSX with all logic in an orchestration hook
- - hooks return no JSX and use TanStack Query with full dependency keys and
- mutation invalidation
- - `_components/`, `_hooks/`, and `_types/` mean local-only
-
- ### Phase 2: Correctness defects in new paths
+ For each checklist row, record the local rule, its source file/line, a nearby
+ example, and the changed code being evaluated. If the repository has no clear
+ rule for a row, mark it unestablished rather than inventing a framework,
+ service superclass, error type, validation library, or directory convention.
- This phase is the reason to run deep, and it keeps its full depth on big
- diffs. When budget is tight, drop a convention row and keep every correctness
- finding. On Huge diffs, do this phase before Phase 1.
+ ### Phase 2: Correctness Defects in New Paths
- For each new or materially changed code path, ask what happens when it goes
- wrong, and read far enough to answer:
+ This is where deep earns its keep, and it does not shrink when the diff is
+ big. If budget is tight, a convention row can be dropped; a correctness
+ finding cannot - on Huge diffs, do this phase before Phase 1. For each new or
+ materially changed code path, ask what happens when it goes wrong, and read
+ far enough to answer:
- - **Unguarded I/O**: child processes, sockets, streams, files.
- - Is every `write` and `spawn` paired with an error handler?
- - What happens on early exit, EPIPE, timeout, or a partial handshake?
- - Would an unhandled error reach the process's global handler, and does
- this process have one?
- - **Bypassed guards**: when one entry point enforces a check (a doctor, a
- validator, a platform gate, a permission check), does every other entry
- point that reaches the same operation enforce it too? Grep the guard's
- usages.
- - **Lifecycle and cleanup**: is everything started also stopped? Timers
- cleared, listeners removed, in-flight work cancelled on unmount or switch,
- and whole process trees killed along with the wrapper shell?
- - **Stale state**: does state reset when its key (session, id, account)
- changes? Can a failure leave old data rendered beside a new error?
- - **Boundary and platform assumptions**: paths, shells, environments (WSL,
- remote, browser), encodings, and anything hardcoded that another platform
+ - **Unguarded I/O**: child processes, sockets, streams, files. Is every
+ `write`/`spawn` paired with an error handler? What happens on early exit,
+ EPIPE, timeout, or a partial handshake? Would an unhandled error surface in
+ the process's global handler - and does this process even have one?
+ - **Bypassed guards**: a check enforced in one entry point (a doctor, a
+ validator, a platform gate, a permission check) - is it also enforced on
+ every other entry point that reaches the same operation? Grep for the
+ guard's usages.
+ - **Lifecycle and cleanup**: is every started thing stopped? Timers cleared,
+ listeners removed, in-flight work cancelled on unmount/switch, process trees
+ killed and not just the wrapper shell?
+ - **Stale state**: does state reset when its key changes (session, id,
+ account)? Can a failure leave old data rendered beside a new error?
+ - **Boundary and platform assumptions**: paths, shells, environment
+ (WSL/remote/browser), encodings; anything hardcoded that another platform
would break.
- - **Untested surface**: new logic without a test where neighbouring code has
- one. Name the specific case that would have caught the defect.
+ - **Untested surface**: new logic with no test where neighbouring code has
+ one - name the specific case that would have caught the defect above.
- Each finding cites `file:line` and states the concrete failure ("child exits
- after `initialize` → EPIPE → uncaught in main process → error dialog").
- Reproduce it where that is cheap. A reproduced defect is Critical; a
- plausible one is a Warning, listed with the reproduction it needs.
+ Every finding here cites `file:line`, states the concrete failure ("child
+ exits after `initialize` → EPIPE → uncaught in main process → error dialog"),
+ and - where cheap - is reproduced. A reproduced defect is Critical; a
+ plausible one is a Warning with the reproduction it needs.
- ### Phase 3: Cross-cutting concerns
+ ### Phase 3: Cross-Cutting Concerns
- - **SOLID and SRP**: each service, hook, and function has one clear purpose;
- no god objects.
- - **DRY**: no duplicate blocks; shared logic lives in utilities or base
- types; each type has a single source of truth.
- - **Documentation**: major files have a top-of-file comment where neighbouring
- files do; complex units have JSDoc; non-obvious logic has inline comments;
- TODOs carry context or an issue number.
- - **Error handling**: the repo's error types, with every failure and
- rejection surfaced.
+ **SOLID / SRP:** each service, hook, function has one clear purpose; no god
+ objects.
- **Configuration object pattern (Critical):**
+ **DRY:** no duplicate blocks; shared logic in utilities or base types; a
+ single source of truth for each type.
+ **Configuration Object Pattern (CRITICAL):**
- ❌ Multiple similar functions → consolidate with an options parameter
(`formatTime()`, `formatTimeCompact()` → `formatTime(date, { format })`)
- ❌ Similar hooks with variations → consolidate with options
- ❌ Two utilities imported for the same purpose → one with options
- - ❌ Duplicate interface or type definitions → single source of truth
+ - ❌ Duplicate interface/type definitions → single source of truth
- ❌ Similar services with minor config differences → consolidate
- - ✅ The same function called with different options is fine
+ - ✅ Same function called with different options is not duplication
- Only issues in lines this branch adds or changes count against the PR. You
- may list pre-existing debt in touched files under Info as "pre-existing, not
- against this PR"; it leaves the score alone. `git blame` settles it.
+ **Documentation:** major files have a top-of-file comment where neighbouring
+ files do; complex units have JSDoc; non-obvious logic has inline comments;
+ TODOs carry context or an issue number.
- ### Phase 4: Generate the report
+ **Error handling:** the repo's error types, no silent failures, no swallowed
+ rejections.
+ **Pre-existing vs introduced:** only issues in lines this branch adds or
+ changes count against the PR. Pre-existing debt in touched files may be
+ listed under Info as "pre-existing, not against this PR" and never lowers the
+ score. `git blame` settles it.
+
+ ### Phase 4: Generate Comprehensive Report
+
Write to `./tmp/deep-refactor-plan-[timestamp].md`:
```markdown
# Deep Refactor Plan
## Classification
- Size: [Large/Huge] ([N] hand-written lines; [M] generated/lockfile excluded)
- Type: [X]
- Complexity: [Complex/Very Complex]
- Layers: [as the repo names them]
- Modules: [list]
- Files Changed: X added, Y modified, Z deleted
- Diff base: merge-base with $BASE at [sha], to working tree
- Conventions sourced from: [files]
## Quality Score: X/10
## Issues Found
### Critical Issues (Must Fix Before Merge)
- [file:line] Issue description
→ Failure: the concrete thing that goes wrong, reproduced: yes/no
→ Fix: detailed instructions
→ Convention: [file that states it] | "correctness"
→ Auto-fixable: Yes/No
### Warnings (Should Fix)
- [file:line] Issue description
→ Suggestion / Impact / Auto-fixable
### Info (Nice to Have)
- [file:line] Suggestion → Benefit
- [file:line] Pre-existing, not against this PR: description
## Auto-Fixable Issues: X
## Manual Fixes Required: Y
**Priority 1 (Blocking):** ...
**Priority 2 (Important):** ...
**Priority 3 (Nice to Have):** ...
## Convention Compliance Matrix
[one row per convention actually checked, with its source file; omit rows
that do not apply to this repo rather than marking them N/A]
## Quality Score Breakdown
- Correctness (Phase 2): [X/10]
- Conventions (Phase 1): [X/10]
- Cross-cutting (Phase 3): [X/10]
- Documentation: [X/10]
**Overall: X/10** - target ≥ 9.8
## Recommendations
1. Hand this plan to `refactor-apply` (auto-fixable first)
2. Address Priority 1, then Priority 2
3. Re-run `refactor-deep` to verify
## References
- Exemplar files studied: [paths]
- Convention sources: [paths]
```
- An empty Critical section is a valid result. Report only real findings, and
- put only applicable rows in the compliance matrix. "No PR-introduced defects"
- is the honest baseline for a clean change and scores accordingly.
+ An empty Critical section is a valid result. Do not manufacture findings to
+ fill the template, and do not pad the compliance matrix with rows that do not
+ apply. "No PR-introduced defects" is the honest baseline for a clean change
+ and should score accordingly.
- ### Phase 5: Show the summary
+ ### Phase 5: Show Summary
```
📊 Deep Analysis Complete!
Plan: ./tmp/deep-refactor-plan-[timestamp].md
Quality Score: X/10 (target 9.8)
Files Analyzed: X · Critical: Y · Warnings: Z · Auto-fixable: W
Key Issues:
- [criticals, one line each - reproduced ones first]
- [warnings summary]
Return the plan path to the caller. When run standalone, ask before running `refactor-apply`.
```
- ## Arguments
+ **IMPORTANT:** This skill never applies fixes. Standalone, ask the user
+ before proceeding to `refactor-apply`; under `refactor`, the orchestrator
+ owns that gate.
+ ## Command Arguments
+
- `--force-all-patterns`: Check all conventions regardless of classification
- `--classify-as=<type>`: Override type classification
- `--size=<size>`: Override size classification
- - `--strict`: Warnings count as Critical for the score, and the target rises
- to 10/10. Strict is always at least as tight as the default 9.8.
+ - `--strict`: Warnings count as Critical for the score and the target rises to 10/10 - strict is never looser than the default 9.8
- ## Checklist
+ ## Success Checklist
- [ ] Diffed from the merge-base with the remote default branch to the working tree
- [ ] Classified with generated lines excluded; complexity judged on the change
- [ ] Conventions read from THIS repo's guidance files and exemplars, per layer
- [ ] Every convention finding confirmed by grep before it was written
- [ ] Phase 2 correctness hunt done on every new path, findings cite file:line
- [ ] Cross-cutting concerns checked
- [ ] Pre-existing debt separated from PR-introduced issues
- [ ] Issues prioritized; auto-fixable vs manual identified
- [ ] Plan written to ./tmp/
- [ ] No files modified (read-only)
+
+ ---
+
+ **This command is read-only and comprehensive.** It analyzes code against
+ the conventions of the repository you are in, hunts for correctness defects
+ in the new paths, and writes a detailed plan for you to review. For smaller
+ work, use `refactor-simple`. Run `refactor-apply` after reviewing the plan.