From 33be1766085ea389643aefdad5625a83910eb7e5 Mon Sep 17 00:00:00 2001 From: Loki FastStart Date: Fri, 8 May 2026 23:02:18 +0000 Subject: [PATCH] feat: add module-organizer and refactoring skills - module-organizer: source tree layout discipline, placement checklist, anti-patterns - refactoring: Fowler/Feathers/Martin methodology, smell catalog, safe workflow - references/feathers-techniques.md: 25 dependency-breaking techniques - references/smell-priority.md: hotspot analysis, priority score formula --- module-organizer/SKILL.md | 162 ++++++++++++ refactoring/SKILL.md | 243 ++++++++++++++++++ refactoring/references/feathers-techniques.md | 133 ++++++++++ refactoring/references/smell-priority.md | 72 ++++++ 4 files changed, 610 insertions(+) create mode 100644 module-organizer/SKILL.md create mode 100644 refactoring/SKILL.md create mode 100644 refactoring/references/feathers-techniques.md create mode 100644 refactoring/references/smell-priority.md diff --git a/module-organizer/SKILL.md b/module-organizer/SKILL.md new file mode 100644 index 0000000..c0e0ce2 --- /dev/null +++ b/module-organizer/SKILL.md @@ -0,0 +1,162 @@ +--- +name: module-organizer +description: Prevent namespace drift in TypeScript projects. Use when adding new files, moving modules, reviewing structure, or planning reorganizations. Provides placement rules, a decision checklist, and anti-patterns to avoid. +--- + +# Module Organizer + +A discipline for keeping source tree layout coherent as projects grow. Prevents the drift that happens when files are placed by convenience (current caller, hurry, copy-paste) instead of by domain ownership. + +## When to Use This Skill + +- Adding a new source file or directory +- Moving/renaming modules during refactoring +- Reviewing PRs that add new files +- Planning a structural reorganization sprint +- Resolving "where does X live?" confusion + +## Core Principle + +> Place files where a new maintainer would look for the **capability itself**, not where the current caller lives. + +Test: "If someone asks 'where does Telegram pairing live?' does the file path answer that question?" + +--- + +## Domain Classification + +Every source file belongs to exactly one domain. Identify it first: + +| Domain | Description | Example Path | +|--------|-------------|--------------| +| **Core** | App-wide types, config, entrypoint | `src/types.ts`, `src/config.ts` | +| **Gateway/Runtime** | Request handling, streaming, lifecycle | `src/gateway/` | +| **Transport** | Platform-specific adapters (Telegram, Slack, etc.) | `src/transports/telegram/` | +| **Agent** | AI agent adapters and SDK integration | `src/agents/pi/` | +| **CLI** | Command-line interface, commands, TUI | `src/cli/commands/` | +| **CLI Support** | Setup, doctor, prompts — serves CLI | `src/cli/setup/` | +| **Feature** | Self-contained features (cron, memory, voice) | `src/cron/`, `src/memory/` | +| **Provisioning** | Install, update, bundle operations | `src/provisioning/` | +| **Shared** | Pure utilities used across 3+ domains | `src/shared/` | + +--- + +## Placement Checklist + +Run through these 10 checks for every new or moved file: + +### 1. Identify the owning domain +What capability does this file provide? Match to table above. + +### 2. Reject caller-based placement +❌ "Gateway calls it, so put it in gateway/" +✅ "It's Telegram formatting, so put it in transports/telegram/" + +### 3. Check for namespace collisions +Never create `foo.ts` beside `foo/` unless `foo.ts` IS the entrypoint (and should become `foo/index.ts`). + +### 4. Check for transport leakage +If the filename contains a platform name (`telegram`, `slack`, `discord`), it belongs under that transport's folder. + +### 5. Check for CLI ↔ runtime leakage +- If gateway/runtime imports it → it should NOT be under `cli/` +- If only CLI uses it → it should NOT be in root `src/` + +### 6. Check singleton-folder pressure +Don't create a folder for one file unless: +- It represents a stable abstraction boundary (e.g., `providers/`) +- You can name 2+ future files that will join it + +### 7. Check command metadata vs command behavior +- Bot command definitions (slash commands) → transport folder +- CLI command execution → `cli/commands/` +- Feature logic invoked by commands → feature folder + +### 8. Check for catch-all growth +If adding to a file that already has 3+ unrelated responsibilities → split first. + +### 9. Apply barrel (`index.ts`) policy +- Add `index.ts` only for **public module boundaries** +- It must export the folder's primary API +- Internal implementation folders skip barrels +- Never create partial barrels that omit the main export + +### 10. Verify import direction +``` +core ← gateway ← transport +core ← agents +core ← cli → cli-support +core ← features +shared ← everything +``` +Arrows show allowed dependency direction. Never invert. + +--- + +## Anti-Patterns + +| Anti-Pattern | Example | Fix | +|--------------|---------|-----| +| **Prefix Soup** | `telegram-format.ts`, `telegram-html.ts`, `telegram-progress.ts` in root | Move to `transports/telegram/` | +| **Orphan Namespace** | `notify/telegram.ts` (only file in folder) | Merge into `transports/telegram/notify.ts` | +| **Split Identity** | `gateway.ts` + `gateway/` | Move class into `gateway/index.ts` | +| **Caller Placement** | `cli/setup-telegram.ts` (it's setup code that touches Telegram API) | Move to `cli/setup/telegram.ts` | +| **Ambiguous Name** | `commands.ts` (Telegram bot commands? CLI commands? Both exist) | Qualify: `transports/telegram/bot-commands.ts` | +| **God Util** | `util.ts` with auth + messaging + ids + debug flags | Split by capability into `shared/` | +| **Hidden Dependency** | `gateway.ts` imports from `cli/doctor/` | Extract shared logic to feature-level module | + +--- + +## Decision Template + +When proposing a file location, document: + +``` +File: +Domain: +Path: +Justification: +New folder needed: +Barrel needed: +Prerequisite renames: +``` + +--- + +## Structural Review Process + +When reviewing an entire tree (e.g., quarterly cleanup): + +1. **List all root-level files** — each needs justification for being at root +2. **Find namespace collisions** — `foo.ts` beside `foo/` +3. **Find prefix clusters** — 3+ files with same prefix = missing folder +4. **Find singleton folders** — folder with 1 file = likely misplaced +5. **Check transport leakage** — platform names outside transport folder +6. **Check import inversions** — runtime importing from CLI namespace +7. **Check catch-all growth** — any file >8 exports or >3 unrelated concerns + +--- + +## Naming Conventions + +| Item | Convention | Example | +|------|-----------|---------| +| Feature folder | noun, singular | `memory/`, `voice/`, `cron/` | +| Adapter file | `-adapter.ts` | `pi-adapter.ts`, `kiro-adapter.ts` | +| CLI command | noun matching the command | `cli/commands/doctor.ts` | +| Types file | `types.ts` within owning folder | `cron/types.ts` | +| Barrel | `index.ts` | Only at public boundaries | +| Shared utility | capability name | `shared/auth.ts`, `shared/ids.ts` | +| Transport | platform name folder | `transports/telegram/` | + +--- + +## When NOT to Reorganize + +- Code is being deleted soon +- The project is <10 files (overhead exceeds benefit) +- You'd break published import paths without a major version bump +- You're on a deadline and the structure isn't blocking anyone +- A rename would touch >50% of files in one PR (break into phases) + +> Reorganization is investment. Invest when confusion is measurably slowing people down. diff --git a/refactoring/SKILL.md b/refactoring/SKILL.md new file mode 100644 index 0000000..136e547 --- /dev/null +++ b/refactoring/SKILL.md @@ -0,0 +1,243 @@ +--- +name: refactoring +description: Inspect, review, and refactor code using patterns from Working Effectively with Legacy Code (Feathers), Refactoring (Fowler), and Clean Code (Martin). Use when editing code, reviewing code for quality, planning refactoring, or assessing technical debt. Provides specific techniques, thresholds, and step-by-step safe refactoring workflows. +--- + +# Refactoring + +A disciplined approach to improving code structure without changing behavior, grounded in three canonical texts: + +- **Working Effectively with Legacy Code** (Michael Feathers) — safely changing code without tests +- **Refactoring: Improving the Design of Existing Code** (Martin Fowler) — catalog of transformations +- **Clean Code** (Robert C. Martin) — readability and design principles + +## When to Use This Skill + +- Reviewing code for quality issues (code review, PR review) +- Planning a refactoring campaign across a codebase +- Editing code and noticing adjacent smells worth fixing +- Assessing technical debt and prioritizing cleanup +- Breaking apart god classes, long methods, or tangled modules + +## Core Principles + +### The Boy Scout Rule +> Leave the code cleaner than you found it. + +When editing code for any reason, fix adjacent smells if the fix is safe and small. + +### Green Bar Discipline +> Never refactor on a red bar. + +1. Ensure tests pass before touching anything +2. Make ONE structural change +3. Run tests immediately +4. Commit if green +5. Repeat + +### Behavioral Preservation +Refactoring changes structure, never behavior. If you're tempted to fix a bug while refactoring — stop, commit the refactoring, THEN fix the bug in a separate commit. + +--- + +## Code Quality Thresholds + +Apply these when reviewing or inspecting code: + +| Metric | Threshold | Severity | Action | +|--------|-----------|----------|--------| +| File length | >400 lines | FAIL | Split module (SRP violation) | +| Function/method length | >50 lines | FAIL | Extract Method | +| Function/method length | >30 lines | WARN | Consider extraction | +| Class methods | >8 public methods | FAIL | God class — extract collaborators | +| Nesting depth | >3 levels | WARN | Extract, use guard clauses | +| Nested try/catch | >2 levels | FAIL | Replace with chain/pipeline | +| Parameters | >4 params | WARN | Introduce Parameter Object | +| Cyclomatic complexity | >10 per function | FAIL | Decompose conditionals | +| Identical patterns | 3+ occurrences | WARN | DRY — extract shared abstraction | +| Platform branching | >2× in same file | FAIL | Replace Conditional with Polymorphism | +| Mixed abstraction levels | orchestration + detail | WARN | Separate into layers | + +--- + +## Smell Catalog (Detection) + +When inspecting code, look for these smells in priority order: + +### Critical (fix immediately) +1. **God Class** — one class/module does everything; >8 methods, >400 lines +2. **Long Method** — can't describe in one sentence; >50 lines +3. **Feature Envy** — method uses another class's data more than its own +4. **Shotgun Surgery** — one change requires editing many files + +### Serious (fix soon) +5. **Primitive Obsession** — using strings/numbers where a type would clarify +6. **Data Clumps** — same 3+ params travel together across functions +7. **Divergent Change** — one module changes for multiple unrelated reasons +8. **Parallel Inheritance** — adding a subclass in one hierarchy forces another + +### Moderate (fix when nearby) +9. **Comments explaining what** — code should be self-documenting +10. **Dead Code** — unreachable branches, unused params, stale imports +11. **Message Chains** — `a.b().c().d()` — tight coupling to structure +12. **Speculative Generality** — abstractions for hypothetical future needs + +--- + +## Safe Refactoring Workflow (Feathers) + +When you lack tests or confidence, follow this sequence: + +### Step 1: Characterize +Write tests that pin current behavior (even if ugly): +``` +// Characterization test: documents ACTUAL behavior, not ideal behavior +test("handles null input by returning empty array", () => { + expect(processItems(null)).toEqual([]); // Even if this seems wrong +}); +``` + +### Step 2: Find Seams +A **seam** is a place where you can alter behavior without editing the code at that point. + +Types of seams: +- **Object seam** — pass a different implementation (dependency injection) +- **Link seam** — import from a different module (re-exports) +- **Preprocessing seam** — environment variables, feature flags + +### Step 3: Break Dependencies +Use these techniques to make code testable: + +| Technique | When to Use | +|-----------|------------| +| **Extract Interface** | Class has too many responsibilities | +| **Parameterize Constructor** | Hard-coded dependencies | +| **Extract and Override Call** | Can't test a method due to one line | +| **Introduce Instance Delegator** | Static method prevents testing | +| **Wrap Method** | Add behavior before/after existing method | +| **Sprout Method/Class** | New feature in untestable code | + +### Step 4: Refactor Under Test +Now that you have tests and seams, apply Fowler's catalog. + +--- + +## Refactoring Catalog (Key Techniques) + +### Extraction Family +- **Extract Method** — pull lines into a named function +- **Extract Variable** — name a complex expression +- **Extract Class** — split a class into two collaborators +- **Extract Module** — move functions to a new file (Sprout Module) + +### Movement Family +- **Move Method** — relocate to the class that uses the data +- **Move Field** — same, for data +- **Pull Up / Push Down** — move along inheritance hierarchy + +### Simplification Family +- **Replace Conditional with Polymorphism** — eliminate if/switch on type +- **Replace Nested Conditional with Guard Clauses** — early returns flatten logic +- **Decompose Conditional** — name the branches +- **Replace Parameter with Method** — caller computes what callee needs +- **Introduce Parameter Object** — group related params into a type +- **Replace Temp with Query** — eliminate intermediate variables + +### Organization Family +- **Inline Method** — when indirection adds no value +- **Rename** — the most powerful refactoring; names reveal intent +- **Replace Magic Number with Constant** — self-documenting literals +- **Encapsulate Collection** — don't expose raw arrays/maps + +--- + +## Patterns for Common Situations + +### Breaking a God Class +1. List all public methods and group by responsibility +2. For each group, create a new class/module +3. Use **Wrap Method** to delegate from old → new (preserves callers) +4. Once all callers migrate, remove delegation + +### Untangling a Long Method +1. Draw dependency arrows between statement groups +2. Identify "paragraphs" (groups separated by blank lines) +3. Name each paragraph — that's your extracted method name +4. Extract from bottom up (fewest dependencies first) +5. If a paragraph uses many locals → Introduce Parameter Object + +### Eliminating Platform Branching +```typescript +// Before: scattered if/else +if (process.platform === "darwin") { ... } +else { ... } + +// After: polymorphic interface +interface ServiceManager { start(); stop(); status(); } +class LaunchdManager implements ServiceManager { ... } +class SystemdManager implements ServiceManager { ... } +``` + +### Replacing Nested Conditionals +```typescript +// Before: 5-level nesting +function loadConfig() { + try { ... try { ... try { ... } } } +} + +// After: resolver chain (guard clauses) +function loadConfig() { + const fromEnv = resolveFromEnv(); + if (fromEnv) return fromEnv; + const fromFlag = resolveFromFlag(); + if (fromFlag) return fromFlag; + return DEFAULT_CONFIG; +} +``` + +### Adding Behavior to Untestable Code (Sprout) +When you can't refactor existing code safely: +1. Write the new behavior in a **new** function/class +2. Test the new code thoroughly +3. Call the new code from the old code (minimal edit) +4. Later, migrate old code's other responsibilities + +--- + +## Review Checklist + +When reviewing code (yours or others'), check in this order: + +1. **[ ] Naming** — Can I understand each function/variable without reading the body? +2. **[ ] Size** — Any file >400 lines? Any function >50 lines? +3. **[ ] SRP** — Does each module/class have one reason to change? +4. **[ ] DRY** — Any structural duplication (3+ similar patterns)? +5. **[ ] Abstraction levels** — Does each function operate at one level? +6. **[ ] Error handling** — Clean separation of happy path and error path? +7. **[ ] Dependencies** — Minimal coupling? Can I test this in isolation? +8. **[ ] Dead code** — Any unused imports, unreachable branches, stale comments? + +--- + +## Commit Strategy + +- One refactoring step per commit (never mix with behavioral changes) +- Commit message format: `refactor(scope): technique applied` +- Example: `refactor(gateway): extract command handlers (Extract Method)` +- Run tests between EVERY commit — never batch untested changes +- If tests break, `git reset --hard` and try a smaller step + +--- + +## When NOT to Refactor + +- Code is being deleted soon +- No tests exist AND you can't write characterization tests +- You're on a deadline and the refactoring isn't on the critical path +- The code works, is rarely changed, and isn't blocking anything +- You'd be refactoring speculatively (for hypothetical future needs) + +> Refactoring is investment. Invest where the code changes frequently. + +See [references/smell-priority.md](references/smell-priority.md) for a prioritization framework. +See [references/feathers-techniques.md](references/feathers-techniques.md) for all 25 Feathers dependency-breaking techniques. diff --git a/refactoring/references/feathers-techniques.md b/refactoring/references/feathers-techniques.md new file mode 100644 index 0000000..c0eb644 --- /dev/null +++ b/refactoring/references/feathers-techniques.md @@ -0,0 +1,133 @@ +# Feathers Dependency-Breaking Techniques + +Detailed reference for the 25 dependency-breaking techniques from "Working Effectively with Legacy Code." + +## The Core Problem + +Legacy code resists change because: +1. You can't test it (dependencies prevent isolation) +2. You can't understand it (too large, too tangled) +3. You can't change it safely (no safety net) + +The solution: **break dependencies to get code under test, then refactor freely.** + +--- + +## Techniques (Alphabetical) + +### Adapt Parameter +**When:** Method takes a parameter whose type is hard to instantiate in tests. +**How:** Create a simpler interface that wraps just what the method needs. + +### Break Out Method Object +**When:** A long method uses many instance variables making extraction hard. +**How:** Move the entire method to a new class; the old class's fields become constructor params. + +### Definition Completion +**When:** Can't instantiate a class because base class has unimplemented methods. +**How:** Create a test subclass that provides trivial implementations. + +### Encapsulate Global References +**When:** Functions depend on global state (singletons, module-level variables). +**How:** Wrap globals in a class/object; pass as dependency. + +### Expose Static Method +**When:** Method doesn't use instance state but is trapped in a class. +**How:** Make it static → now testable without instantiation. + +### Extract and Override Call +**When:** One problematic dependency call is buried in an otherwise good method. +**How:** Extract that call to a virtual method; override in test subclass. + +### Extract and Override Factory Method +**When:** Constructor creates dependencies that make testing hard. +**How:** Extract creation to a factory method; override in test subclass. + +### Extract and Override Getter +**When:** Instance variable initialization is hard to control in tests. +**How:** Access through a getter; override getter in test subclass. + +### Extract Implementer +**When:** A class mixes interface and implementation (making it hard to substitute). +**How:** Extract an interface, then the original becomes "the implementation." + +### Extract Interface +**When:** You need to substitute a dependency but it has no interface. +**How:** Create an interface from the methods you actually call. + +### Introduce Instance Delegator +**When:** Important behavior is in static methods (untestable with mocking). +**How:** Create instance methods that delegate to the static methods. + +### Introduce Static Setter +**When:** Singleton or global prevents test isolation. +**How:** Add a static setter for tests to inject a fake (use sparingly!). + +### Link Substitution +**When:** Need to replace a dependency at the module/import level. +**How:** Swap the import path (DI via module system). + +### Parameterize Constructor +**When:** Constructor hard-codes dependencies. +**How:** Accept dependencies as constructor parameters (classic DI). + +### Parameterize Method +**When:** Method hard-codes a dependency inside its body. +**How:** Accept the dependency as a parameter. + +### Primitivize Parameter +**When:** Method takes a complex object but only uses simple data from it. +**How:** Pass the primitives directly. + +### Pull Up Feature +**When:** Subclass has behavior that should be shared/testable independently. +**How:** Move to superclass or extract to a utility. + +### Push Down Dependency +**When:** Base class has a dependency that only some subclasses need. +**How:** Move dependency to the subclass that uses it. + +### Replace Function with Function Pointer +**When:** Need to substitute a free function for testing. +**How:** Accept the function as a parameter (higher-order function / callback). + +### Replace Global Reference with Getter +**When:** Code reads a global variable directly. +**How:** Access through a getter method that can be overridden in tests. + +### Subclass and Override Method +**When:** A method does something untestable (I/O, network, time). +**How:** Create test subclass that overrides just that method. + +### Supersede Instance Variable +**When:** Can't control an instance variable's initialization. +**How:** Add a setter or re-assignment point after construction (for tests only). + +### Template Redefinition +**When:** Behavior variation is needed but inheritance is impractical. +**How:** Use template method pattern; test via different template implementations. + +### Text Redefinition +**When:** In dynamic languages, redefine methods at runtime for tests. +**How:** Monkey-patch in test setup (use sparingly!). + +--- + +## Choosing a Technique + +| Situation | Best Technique | +|-----------|---------------| +| Can't instantiate class | Parameterize Constructor, Extract Interface | +| One bad line in a good method | Extract and Override Call | +| Static method dependency | Introduce Instance Delegator | +| Global state | Encapsulate Global References | +| Complex parameter type | Adapt Parameter, Primitivize Parameter | +| Hard-coded file/network I/O | Parameterize Method (inject callback) | +| Module-level coupling | Link Substitution (re-export from wrapper) | + +## The Safety Net Progression + +1. **No tests, no understanding** → Write characterization tests first +2. **Have tests, tangled code** → Break dependencies using techniques above +3. **Isolated code** → Apply Fowler's refactoring catalog freely +4. **Clean code** → Maintain with review thresholds diff --git a/refactoring/references/smell-priority.md b/refactoring/references/smell-priority.md new file mode 100644 index 0000000..5f51f74 --- /dev/null +++ b/refactoring/references/smell-priority.md @@ -0,0 +1,72 @@ +# Smell Priority Framework + +## How to Prioritize Refactoring + +Not all smells are equal. Prioritize based on **change frequency × pain level**. + +### Quadrant Model + +``` + HIGH CHANGE FREQUENCY + │ + Fix Now │ Fix Next Sprint + (blocking velocity) │ (accumulating debt) + │ + HIGH ────────────────────┼──────────────────── LOW + PAIN │ PAIN + │ + Fix When Nearby │ Leave Alone + (opportunistic) │ (stable, working) + │ + LOW CHANGE FREQUENCY +``` + +### Priority Score + +``` +Priority = (change_frequency × 3) + (bug_correlation × 2) + (comprehension_cost × 1) +``` + +Each factor scored 1-5: +- **Change frequency**: How often do we modify this code? (git log --since=3months) +- **Bug correlation**: How many bugs originate here? (issue tracker linkage) +- **Comprehension cost**: How long does a new developer need to understand it? + +### Score Interpretation + +| Score | Action | +|-------|--------| +| 25-30 | Emergency — blocking team velocity | +| 18-24 | Plan for this sprint | +| 12-17 | Add to backlog, fix when nearby | +| 6-11 | Leave alone (low impact) | + +## Hotspot Analysis + +Use git history to find refactoring targets: + +```bash +# Files changed most in last 3 months +git log --since="3 months ago" --pretty=format: --name-only | sort | uniq -c | sort -rn | head -20 + +# Files with most distinct authors (knowledge silos) +git log --since="6 months ago" --format="AUTHOR:%an" --name-only | awk '/^AUTHOR:/{author=substr($0,8);next} NF==0{next} {a[$0][author]=1} END{for(f in a) print length(a[f]), f}' | sort -rn | head -20 + +# Churn + complexity (lines × changes) +for f in $(git log --since="3 months ago" --pretty=format: --name-only | sort -u); do + changes=$(git log --since="3 months ago" --oneline -- "$f" | wc -l) + lines=$(wc -l < "$f" 2>/dev/null || echo 0) + echo "$((changes * lines)) $changes $lines $f" +done | sort -rn | head -20 +``` + +## When to Stop Refactoring + +A module is "good enough" when: +1. Each file ≤400 lines +2. Each function ≤50 lines with a clear name +3. Tests exist for public API surface +4. A new developer can understand it in <15 minutes +5. Changes to one responsibility don't risk breaking others + +Diminishing returns start when you're renaming things for aesthetics or extracting methods that are only called once and don't improve readability.