diff --git a/go-linter-driven-development/agents/go-code-reviewer.md b/go-linter-driven-development/agents/go-code-reviewer.md index e9cc6e5..ee126cb 100644 --- a/go-linter-driven-development/agents/go-code-reviewer.md +++ b/go-linter-driven-development/agents/go-code-reviewer.md @@ -8,6 +8,7 @@ tools: - Read - Grep - Skill(go-linter-driven-development:pre-commit-review) # Auto-loaded for design analysis guidance + - mcp__ide__getDiagnostics --- You are a Go Code Design Reviewer specializing in detecting design patterns and architectural issues that linters cannot catch. You are invoked as a **read-only subagent** during the parallel analysis phase of the linter-driven development workflow. @@ -27,7 +28,7 @@ Your job: Analyze the code and return a **structured report** that the orchestra -Automatically use the @pre-commit-review skill to guide your analysis. This skill contains: +**Use the Skill tool** to invoke `Skill(go-linter-driven-development:pre-commit-review)` to load design analysis guidance. This skill contains: - Detection checklist for 8 design issue categories - Juiciness scoring algorithm for primitive obsession - Examples of good vs bad patterns diff --git a/go-linter-driven-development/agents/quality-analyzer.md b/go-linter-driven-development/agents/quality-analyzer.md index 5416449..4924e19 100644 --- a/go-linter-driven-development/agents/quality-analyzer.md +++ b/go-linter-driven-development/agents/quality-analyzer.md @@ -6,6 +6,7 @@ description: | tools: - Bash - Task + - mcp__ide__getDiagnostics --- You are a Quality Analyzer Agent that orchestrates parallel quality analysis for Go projects. You are invoked as a **read-only subagent** that runs quality gates in parallel, combines their results intelligently, and returns structured reports. @@ -126,6 +127,21 @@ normalized_issue: message: "Cognitive complexity 18 (>15)" raw_output: "..." ``` + +**Linter Categorization Reference:** + +| Linter | Category | Severity | Routes To | +|--------|----------|----------|-----------| +| `nestif` | complexity | high | @refactoring (storify → early returns) | +| `cyclop`, `gocognit` | complexity | high | @refactoring (storify → extract type) | +| `funlen` | complexity | medium | @refactoring (storify → extract function) | +| `argument-limit` (revive) | design | high | @code-designing (options struct) | +| `function-result-limit` (revive) | design | high | @code-designing (result type) | +| `confusing-results` (revive) | design | medium | @code-designing (named result type) | +| `early-return` (revive) | style | low | @refactoring (early return pattern) | +| `file-length-limit` (revive) | design | high | @code-designing (file splitting) | +| `wrapcheck` | bug | medium | Direct fix (error wrapping) | +| `varnamelen` | style | low | Direct fix (rename variable) | diff --git a/go-linter-driven-development/commands/go-ldd-analyze.md b/go-linter-driven-development/commands/go-ldd-analyze.md index f3c948b..b9190b9 100644 --- a/go-linter-driven-development/commands/go-ldd-analyze.md +++ b/go-linter-driven-development/commands/go-ldd-analyze.md @@ -7,6 +7,7 @@ allowed-tools: - Grep - Bash - Task + - mcp__ide__getDiagnostics --- Run comprehensive quality analysis with intelligent combining of test results, linter findings, and code review feedback. diff --git a/go-linter-driven-development/commands/go-ldd-autopilot.md b/go-linter-driven-development/commands/go-ldd-autopilot.md index 83c6456..8963a64 100644 --- a/go-linter-driven-development/commands/go-ldd-autopilot.md +++ b/go-linter-driven-development/commands/go-ldd-autopilot.md @@ -4,9 +4,10 @@ description: Start complete linter-driven autopilot workflow (Phase 1-5) argument-hint: "" allowed-tools: - Skill(go-linter-driven-development:linter-driven-development) + - mcp__ide__getDiagnostics --- -Invoke the @linter-driven-development skill to run the complete autopilot workflow from design through commit-ready. +**Use the Skill tool** to invoke `Skill(go-linter-driven-development:linter-driven-development)` to run the complete autopilot workflow from design through commit-ready. ⏱️ **Estimated Duration**: 5-15 minutes (depends on feature complexity and issues found) diff --git a/go-linter-driven-development/commands/go-ldd-quickfix.md b/go-linter-driven-development/commands/go-ldd-quickfix.md index 767ad16..89e261b 100644 --- a/go-linter-driven-development/commands/go-ldd-quickfix.md +++ b/go-linter-driven-development/commands/go-ldd-quickfix.md @@ -4,13 +4,14 @@ description: Run quality gates loop until all green (tests+linter+review → fix argument-hint: "[file_pattern]" allowed-tools: - Skill(go-linter-driven-development:linter-driven-development) + - mcp__ide__getDiagnostics --- Execute the quality gates loop for already-implemented code that needs cleanup. ⏱️ **Estimated Duration**: 2-5 minutes (depends on number of issues found) -Run these phases from @linter-driven-development skill: +**Use the Skill tool** to invoke `Skill(go-linter-driven-development:linter-driven-development)` and run these phases: **Phase 2**: Parallel Analysis - Discover project test/lint commands @@ -29,8 +30,14 @@ Run these phases from @linter-driven-development skill: - Re-verify with parallel analysis (incremental review mode) - Repeat until all green +**Phase 5**: Orchestrator Review (after linter clean) +- Check types with >15 methods (god object threshold) +- If found: Apply @refactoring for storification first +- Then apply @code-designing for composition (service extraction) +- Re-verify with linter + **Loop until**: -✅ Tests pass | ✅ Linter clean | ✅ Review clean +✅ Tests pass | ✅ Linter clean | ✅ Review clean | ✅ No god objects (≤15 methods per type) Use this when code is already written but needs to pass quality gates. Skip the implementation phase (Phase 1) and go straight to fixing issues. diff --git a/go-linter-driven-development/commands/go-ldd-review.md b/go-linter-driven-development/commands/go-ldd-review.md index 9b1dea5..857b141 100644 --- a/go-linter-driven-development/commands/go-ldd-review.md +++ b/go-linter-driven-development/commands/go-ldd-review.md @@ -7,6 +7,7 @@ allowed-tools: - Grep - Bash - Task + - mcp__ide__getDiagnostics --- Run final verification checks **without** the auto-fix loop. diff --git a/go-linter-driven-development/commands/go-ldd-status.md b/go-linter-driven-development/commands/go-ldd-status.md index 4a90e9b..358e9fa 100644 --- a/go-linter-driven-development/commands/go-ldd-status.md +++ b/go-linter-driven-development/commands/go-ldd-status.md @@ -5,6 +5,7 @@ argument-hint: "" allowed-tools: - Read - Bash(git *) + - mcp__ide__getDiagnostics --- !`git status --porcelain` diff --git a/go-linter-driven-development/skills/code-designing/SKILL.md b/go-linter-driven-development/skills/code-designing/SKILL.md index ea8767d..65bf672 100644 --- a/go-linter-driven-development/skills/code-designing/SKILL.md +++ b/go-linter-driven-development/skills/code-designing/SKILL.md @@ -6,6 +6,7 @@ description: | Focuses on vertical slice architecture and type safety. allowed-tools: - Skill(go-linter-driven-development:testing) + - mcp__ide__getDiagnostics --- @@ -15,6 +16,16 @@ Use when planning new features or identifying need for new types during refactor **Reference**: See `reference.md` for complete design principles and examples. + +**CRITICAL**: When this skill says "Use @skill-name" or routes to "@skill-name", you MUST use the **Skill tool** explicitly. + +| Notation | Skill Tool Call | +|----------|-----------------| +| @testing | `Skill(go-linter-driven-development:testing)` | + +**DO NOT** just reference the skill - actually invoke it using the Skill tool. + + 1. **Analyze Architecture**: Check for vertical vs horizontal slicing 2. **Understand Domain**: Identify problem domain, concepts, invariants @@ -31,6 +42,10 @@ Ready to implement? Use @testing skill for test structure. - Refactoring reveals need for new types (complexity extraction) - Linter failures suggest types should be introduced - When you need to think through domain modeling +- **`argument-limit`** linter failure (>4 parameters) → Design options struct +- **`function-result-limit`** linter failure (>3 returns) → Design result type +- **`confusing-results`** linter failure → Design named result type +- **`file-length-limit`** linter failure (>450 lines) → Analyze and split juicy types to own files @@ -176,6 +191,97 @@ Check design against (see reference.md): - [ ] Clear separation of concerns + +**When invoked by linter failures, apply these patterns:** + + +```go +// BEFORE - Too many parameters +func CreateUser(name string, email string, age int, role string, dept string) (*User, error) + +// AFTER - Options struct +type CreateUserOptions struct { + Name string + Email string + Age int + Role string + Dept string +} + +func CreateUser(opts CreateUserOptions) (*User, error) +``` +**Design Tip**: Add validation in a constructor: `NewCreateUserOptions(...) (CreateUserOptions, error)` + + + +```go +// BEFORE - Too many return values +func ParseConfig(path string) (config Config, warnings []string, version int, error) + +// AFTER - Result type +type ParseConfigResult struct { + Config Config + Warnings []string + Version int +} + +func ParseConfig(path string) (ParseConfigResult, error) +``` + + + +```go +// BEFORE - Confusing (string, string, error) +func ParseAddress(raw string) (string, string, error) // Which is host? Which is port? + +// AFTER - Named result type +type ParsedAddress struct { + Host string + Port string +} + +func ParseAddress(raw string) (ParsedAddress, error) +``` + + + +**Step 1: Analyze file structure** + +| File Pattern | Action | +|--------------|--------| +| Multiple juicy types | Move each juicy type to its own file | +| Single god type | Extract method clusters via composition OR extract juicy logic from methods | +| Long functions, few types | Route to @refactoring first (storify → extract functions) | + +**Step 2: Apply juiciness test** + +"Juicy" types (deserve their own file): +- Types with ≥2 methods +- Types with complex validation +- Types with transformations/parsing +- Enums WITH methods (behavior makes them juicy) + +"Anemic" types (can stay grouped in types.go or similar): +- Simple enums (const block only) +- DTOs with no methods +- Type aliases + +**Step 3: For god types** (>15 methods) + +| Option | When to Use | Pattern | +|--------|-------------|---------| +| **Storify first** | Methods are hard to read | Apply storification → reveals hidden structure | +| **Extract via composition** | Method clusters exist | Identify cluster → extract to new type → compose | +| **Extract juicy logic** | Primitive obsession inside methods | Find logic on primitives → extract to self-validating type | + +**Routing**: God types require two-phase refactoring: +1. **@refactoring** (first): Storify methods to reveal structure +2. **@code-designing** (then): Design service composition + +See @refactoring skill → `god_object_decomposition` pattern for detailed mechanics. + + + diff --git a/go-linter-driven-development/skills/linter-driven-development/SKILL.md b/go-linter-driven-development/skills/linter-driven-development/SKILL.md index 9c5bba0..0450a85 100644 --- a/go-linter-driven-development/skills/linter-driven-development/SKILL.md +++ b/go-linter-driven-development/skills/linter-driven-development/SKILL.md @@ -10,6 +10,7 @@ allowed-tools: - Skill(go-linter-driven-development:refactoring) - Skill(go-linter-driven-development:documentation) - Task + - mcp__ide__getDiagnostics --- @@ -28,6 +29,25 @@ Use for any commit: features, bug fixes, refactors. - Working directory contains Go project (go.mod or .go files) + +**CRITICAL**: When this skill says "Invoke @skill-name" or routes to "@skill-name", you MUST use the **Skill tool** explicitly. + +| Notation | Skill Tool Call | +|----------|-----------------| +| @code-designing | `Skill(go-linter-driven-development:code-designing)` | +| @testing | `Skill(go-linter-driven-development:testing)` | +| @refactoring | `Skill(go-linter-driven-development:refactoring)` | +| @documentation | `Skill(go-linter-driven-development:documentation)` | + +**DO NOT** just reference the skill in your response - actually invoke it using the Skill tool. +**DO NOT** read the skill file directly - use the Skill tool to load and execute it. + +Example: When Phase 1 says "Invoke @testing skill to WRITE tests", you must call: +``` +Skill(go-linter-driven-development:testing) +``` + + **Immediate Action**: Run Pre-Flight Check, then execute phases sequentially until commit-ready. @@ -81,8 +101,9 @@ Scan conversation history (last 50 messages) for step-by-step plan and which ste - Invoke @code-designing skill - Output: Type design plan with self-validating domain types -**Write Tests First**: -- Invoke @testing skill for guidance +**Write Tests First** (MANDATORY): +- Invoke @testing skill to WRITE tests (not just guidance) +- Create test files for all new types/functions - Write table-driven tests or testify suites - Target: 100% coverage on new leaf types @@ -90,6 +111,18 @@ Scan conversation history (last 50 messages) for step-by-step plan and which ste - Follow coding principles from coding_rules.md - Keep functions <50 LOC, max 2 nesting levels - Use self-validating types, prevent primitive obsession + +**Test Verification** (before proceeding): +1. For each new type file created: + - Verify corresponding `*_test.go` exists + - Run: `go test -cover ./path/to/package` + - Verify: coverage > 0% (tests actually exercise code) +2. For leaf types: warn if coverage < 80% + +**GATE**: DO NOT proceed to Phase 2 until: +- [ ] Test files exist for all new types +- [ ] `go test -cover` shows > 0% coverage for new packages +- [ ] No "no test files" or "[no tests to run]" messages @@ -120,9 +153,45 @@ Max 10 iterations. If stuck, ask user for guidance. + + +**Linter Error → Skill Routing Table** + +Route linter failures to the correct skill based on error type: + +| Linter Error | Route To | Pattern Priority | +|--------------|----------|------------------| +| `nestif` (deep nesting) | @refactoring | 1. Storify, 2. Early returns, 3. Extract function | +| `argument-limit` (>4 params) | @code-designing | Create options struct type | +| `function-result-limit` (>3 returns) | @code-designing | Create result type | +| `confusing-results` | @code-designing | Create named result type | +| `cyclop`/`gocognit` (complexity) | @refactoring | 1. Storifying, 2. Extract type | +| `funlen` (function too long) | @refactoring | 1. Storify, 2. Extract function | +| `wrapcheck` (unwrapped error) | Direct fix | `fmt.Errorf("context: %w", err)` | +| `varnamelen` (short var name) | Direct fix | Rename variable to be descriptive | +| `early-return` (revive) | @refactoring | Apply early return pattern | +| `file-length-limit` (revive) | Analyze first → route | See file-level concerns below | + +**File-Level Concerns** (`file-length-limit` triggers at >450 lines): +When files exceed the limit, analyze structure first: + +| File Pattern | Route To | Pattern | +|--------------|----------|---------| +| Multiple juicy types in one file | @code-designing | **Juicy type per file** - move each to own file | +| Single god type (>15 methods) | @refactoring → @code-designing | 1. Storify (refactoring), 2. Decompose (code-designing) | +| Long functions, few types | @refactoring | **Storify → Extract functions** | + +**"Juicy" types** (deserve their own file): +- Types with ≥2 methods +- Types with complex validation +- Types with transformations/parsing +- Enums WITH methods (behavior makes them juicy) + + **For each prioritized fix** (from agent's report): 1. **Apply Fix**: + - Use routing table above to select correct skill - Invoke @refactoring skill with file, function, issues, and root cause - @refactoring applies patterns: early returns, extract function, storifying, extract type, switch extraction, extract constant @@ -140,6 +209,25 @@ Max 10 iterations. If stuck, ask user for guidance. - Max 10 iterations per fix loop - If stuck after 3 attempts → show status, ask user +5. **Orchestrator Check** (after CLEAN_STATE): + - Count methods per type in modified files + - If any type has >15 methods: + - Flag as potential god object + - Apply @refactoring for storification (make it read like a story) + - Apply @code-designing for composition (extract services) + - Re-run quality-analyzer to verify + +6. **Test Extracted Types** (mandatory after type extraction): + - Track all new types created during refactoring + - For each leaf type (no external dependencies): + - Invoke @testing skill + - Write table-driven tests for constructor validation + - Write tests for all public methods + - Target: 100% coverage on leaf types + - For orchestrating types: + - Write integration-style tests covering seams + - Re-run `task test` to verify all pass + **Loop until agent returns CLEAN_STATE**. @@ -198,6 +286,8 @@ Workflow is complete when ALL of the following are true: - [ ] Tests pass - [ ] Linter passes (0 errors) - [ ] Code review clean (0 findings) +- [ ] All extracted leaf types have tests (100% coverage) +- [ ] No god objects (all types have ≤15 methods) - [ ] Phase 4 complete (documentation added/updated) - [ ] Commit summary presented to user with options - [ ] User has chosen commit action (or deferred) diff --git a/go-linter-driven-development/skills/refactoring/SKILL.md b/go-linter-driven-development/skills/refactoring/SKILL.md index 33995a7..ec25a95 100644 --- a/go-linter-driven-development/skills/refactoring/SKILL.md +++ b/go-linter-driven-development/skills/refactoring/SKILL.md @@ -8,20 +8,33 @@ allowed-tools: - Skill(go-linter-driven-development:code-designing) - Skill(go-linter-driven-development:testing) - Skill(go-linter-driven-development:pre-commit-review) + - mcp__ide__getDiagnostics --- Linter-driven refactoring patterns to reduce complexity and improve code quality. Operates autonomously - no user confirmation needed during execution. -**Reference**: See `reference.md` for complete decision tree and all patterns. +**Reference**: See `reference.md` for complete decision tree, patterns, and code examples. **Examples**: See `examples.md` for real-world refactoring case studies. + +**CRITICAL**: When this skill says "Invoke @skill-name" or routes to "@skill-name", you MUST use the **Skill tool** explicitly. + +| Notation | Skill Tool Call | +|----------|-----------------| +| @code-designing | `Skill(go-linter-driven-development:code-designing)` | +| @testing | `Skill(go-linter-driven-development:testing)` | +| @pre-commit-review | `Skill(go-linter-driven-development:pre-commit-review)` | + +**DO NOT** just reference the skill - actually invoke it using the Skill tool. + + 1. **Receive linter failures** from @linter-driven-development 2. **Analyze root cause** - Does it read like a story? Can it be broken down? -3. **Apply patterns** in priority order (early returns → extract function → storifying → extract type) +3. **Apply patterns** in priority order (storify → early returns → extract function → extract type) 4. **Verify** - Re-run linter automatically 5. **Iterate** until linter passes @@ -33,448 +46,151 @@ Operates autonomously - no user confirmation needed during execution. - **Automatically invoked** by @pre-commit-review when design issues detected - **Complexity failures**: cyclomatic, cognitive, maintainability index - **Architectural failures**: noglobals, gochecknoinits, gochecknoglobals -- **Design smell failures**: dupl (duplication), goconst (magic strings), ineffassign +- **Design smell failures**: dupl, goconst, ineffassign - Functions > 50 LOC or nesting > 2 levels - Mixed abstraction levels in functions - Manual invocation when code feels hard to read/maintain -Choose your learning path: -- **Quick Start**: Use the patterns below for common refactoring cases +- **Quick Start**: Use patterns below for common cases - **Complete Reference**: See [reference.md](./reference.md) for full decision tree and all patterns -- **Real-World Examples**: See [examples.md](./examples.md) to learn the refactoring thought process - - [Example 1](./examples.md#example-1-storifying-mixed-abstractions-and-extracting-logic-into-leaf-types): Storifying and extracting a single leaf type - - [Example 2](./examples.md#example-2-primitive-obsession-with-multiple-types-and-storifying-switch-statements): Primitive obsession with multiple types and switch elimination +- **Real-World Examples**: See [examples.md](./examples.md) for case studies: + - **Example 1**: Storifying mixed abstractions + extracting leaf types (fat function → lean orchestration) + - **Example 2**: Primitive obsession with multiple types + switch elimination (includes over-abstraction trap!) + - **Example 3**: Dependency rejection pattern (globals → clean testable islands, bottom-up approach) - -Before applying any refactoring patterns, automatically analyze the context: + - -AUTOMATICALLY ANALYZE: -1. Find all callers of the failing function -2. Identify which flows/features depend on it -3. Determine primary responsibility -4. Check for similar functions revealing patterns -5. Spot potential refactoring opportunities - + +| Linter Error | Pattern | +|--------------|---------| +| `nestif` (deep nesting) | Storify → Early returns → Extract function | +| `cyclop`/`gocognit` | Storify → Extract type | +| `funlen` (too long) | Storify → Extract function | +| `noglobals` | Dependency rejection | +| `dupl` | Extract common logic/types | +| `goconst` | Extract constants or types | +| `wrapcheck` | Direct fix: `fmt.Errorf("context: %w", err)` | +| `early-return` (revive) | Invert condition, return early | +| `file-length-limit` | Route to @code-designing for file splitting | + +**Pattern Documentation**: +- Storifying, Early Returns, Extract Function, Extract Type → `reference.md` +- Dependency Rejection → `examples.md` (Example 3) +- Over-abstraction warnings → `reference.md` section 2.5 + + + +**When `file-length-limit` triggers (>450 lines):** + +| File Pattern | Route To | Action | +|--------------|----------|--------| +| Multiple juicy types | @code-designing | Juicy type per file | +| Single god type (>15 methods) | @refactoring → @code-designing | Storify, then decompose | +| Long functions, few types | @refactoring | Storify → Extract functions | + +**"Juicy" types** (deserve own file): ≥2 methods, complex validation, transformations/parsing +**Anemic types** (can stay grouped): Simple enums, DTOs without methods, type aliases + + - -Proactively identify hidden types in the code: + +**Pattern Priority Order** (for complexity failures): -POTENTIAL TYPES TO DISCOVER: -1. Data being parsed from strings → Parse* types - Example: ParseCommandResult(), ParseLogEntry() +1. **Storifying** - Make code read like a story, reveals hidden structure +2. **Early Returns** - Reduce nesting by inverting conditions +3. **Extract Function** - Break up long functions by responsibility +4. **Extract Type** - Create domain types (only if "juicy") +5. **Switch Extraction** - Extract case handlers to separate functions +6. **Dependency Rejection** - Push globals up call chain incrementally -2. Scattered validation logic → Validated types - Example: Email, Port, IPAddress types +See `reference.md` for detailed patterns with code examples. -3. Data that always travels together → Aggregate types - Example: UserCredentials, ServerConfig + +Before extracting a type, verify it's "juicy" (worth creating): -4. Complex conditions → State/status types - Example: DeploymentStatus with IsReady(), CanProceed() +**BEHAVIORAL**: Complex validation, ≥2 meaningful methods, state transitions +**STRUCTURAL**: Parsing unstructured data, grouping related data +**USAGE**: Used in multiple places, simplifies calling code -5. Repeated string manipulation → Types with methods - Example: FilePath with Dir(), Base(), Ext() - +Need "yes" in at least ONE category. See `reference.md` section 2.5 for over-abstraction warnings. + - -The analysis produces a refactoring plan identifying: -- Function's role in the system -- Potential domain types to extract -- Recommended refactoring approach -- Expected complexity reduction - - + +When extracting a type to its own file, co-locate ALL related declarations: +- Type definition + constants + constructor + all methods - - -**Complexity Issues:** -- **Cyclomatic Complexity**: Too many decision points → Extract functions, simplify logic -- **Cognitive Complexity**: Hard to understand → Storifying, reduce nesting -- **Maintainability Index**: Hard to maintain → Break into smaller pieces - -**Architectural Issues:** -- **noglobals/gochecknoglobals**: Global variable usage → Dependency rejection pattern -- **gochecknoinits**: Init function usage → Extract initialization logic -- **Static/singleton patterns**: Hidden dependencies → Inject dependencies - -**Design Smells:** -- **dupl**: Code duplication → Extract common logic/types -- **goconst**: Magic strings/numbers → Extract constants or types -- **ineffassign**: Ineffective assignments → Simplify logic - - - -- Functions > 50 LOC -- Nesting > 2 levels -- Mixed abstraction levels -- Unclear flow/purpose -- Primitive obsession -- Global variable access scattered throughout code - - +Verify: `grep -r "TypeName" --include="*.go" . | grep -v "type_file.go"` +If found elsewhere → move to type's file + - -This skill operates completely autonomously once invoked: + +**Trigger**: Type has >15 methods OR >500 LOC + +**Strategy** (in order): + +1. **Extract generic logic first** (creates reusable leaf types): + - String manipulation → `StringParser`, `Formatter` types + - URL/path handling → `URL`, `FilePath` types with validation + - Retry/timeout logic → `Retrier`, `TimeoutHandler` types + - Date/time formatting → `DateFormatter` type + - Validation patterns → Self-validating domain types + + These become **testable islands** and may be useful elsewhere. + +2. **Then group remaining methods by noun** (domain services): + - User methods → `UserService` + - Order methods → `OrderService` + - Cache methods → `CacheService` + +3. **Extract each group into focused service type** +4. **Compose services in orchestrator** (delegates, doesn't implement) + +**Key insight**: Generic logic extraction often reveals the god object was mixing infrastructure concerns with domain logic. +See `reference.md` for detailed example. + + + + -AUTOMATED PROCESS: -1. Receive trigger: - - From @linter-driven-development (linter failures) - - From @pre-commit-review (design debt/readability debt) +1. Receive trigger (automatic from other skills, or manual user request) 2. Apply refactoring pattern (start with least invasive) 3. Run linter immediately (no user confirmation) -4. If linter still fails OR review finds more issues: - - Try next pattern in priority order - - Repeat until both linter and review pass -5. If patterns exhausted and still failing: - - Report what was tried - - Suggest file splitting or architectural changes +4. If linter still fails → try next pattern in priority order +5. Repeat until linter passes +6. If patterns exhausted → report what was tried, escalate to user for architectural guidance - -**For Complexity Failures** (cyclomatic, cognitive, maintainability): -1. Early Returns → Reduce nesting quickly -2. Extract Function → Break up long functions -3. Storifying → Improve abstraction levels -4. Extract Type → Create domain types (only if "juicy") -5. Switch Extraction → Categorize switch cases - -**For Architectural Failures** (noglobals, singletons): -1. Dependency Rejection → Incremental bottom-up approach -2. Extract Type with dependency injection -3. Push global access up call chain one level -4. Iterate until globals only at entry points (main, handlers) - -**For Design Smells** (dupl, goconst): -1. Extract Type → For repeated values or validation -2. Extract Function → For code duplication -3. Extract Constant → For magic strings/numbers - - - **NO** asking for confirmation between patterns - **NO** waiting for user input -- **NO** manual linter runs - **AUTOMATIC** progression through patterns - **ONLY** report results at the end - - - -```go -// BEFORE - Mixed abstractions -func ProcessOrder(order Order) error { - // Validation - if order.ID == "" { - return errors.New("invalid") - } - // Low-level DB setup - db, err := sql.Open("postgres", connStr) - if err != nil { return err } - defer db.Close() - // SQL construction - query := "INSERT INTO..." - // ... many lines - return nil -} - -// AFTER - Story-like -func ProcessOrder(order Order) error { - if err := validateOrder(order); err != nil { - return err - } - if err := saveToDatabase(order); err != nil { - return err - } - return notifyCustomer(order) -} - -func validateOrder(order Order) error { /* ... */ } -func saveToDatabase(order Order) error { /* ... */ } -func notifyCustomer(order Order) error { /* ... */ } -``` - - - - -**BEHAVIORAL JUICINESS** (rich behavior): -- Complex validation rules (regex, ranges, business rules) -- Multiple meaningful methods (≥2 methods) -- State transitions or transformations -- Format conversions (different representations) - -**STRUCTURAL JUICINESS** (organizing complexity): -- Parsing unstructured data into fields -- Grouping related data that travels together -- Making implicit structure explicit -- Replacing map[string]interface{} with typed fields - -**USAGE JUICINESS** (simplifies code): -- Used in multiple places -- Significantly simplifies calling code -- Makes tests cleaner and more focused - -**Score**: Need "yes" in at least ONE category to create the type - - -```go -// NOT JUICY - Don't create type -func ValidateUserID(id string) error { - if id == "" { - return errors.New("empty id") - } - return nil -} -// Just use: if userID == "" - -// JUICY (Behavioral) - Complex validation -type Email string - -func ParseEmail(s string) (Email, error) { - if s == "" { - return "", errors.New("empty email") - } - if !emailRegex.MatchString(s) { - return "", errors.New("invalid format") - } - if len(s) > 255 { - return "", errors.New("too long") - } - return Email(s), nil -} - -func (e Email) Domain() string { /* extract domain */ } -func (e Email) LocalPart() string { /* extract local */ } - -// JUICY (Structural) - Parsing complex data -type CommandResult struct { - FailedFiles []string - SuccessFiles []string - Message string - ExitCode int -} - -func ParseCommandResult(output string) (CommandResult, error) { - // Parse unstructured output into structured fields -} -``` - -**Warning Signs of Over-Engineering:** -- Type with only one trivial method -- Simple validation (just empty check) -- Type that's just a wrapper without behavior -- Good variable naming would be clearer - -See [Example 2](./examples.md#first-refactoring-attempt-the-over-abstraction-trap) for complete case study. - - - -```go -// BEFORE - Long function -func CreateUser(data map[string]interface{}) error { - // Validation (15 lines) - // Database operations (20 lines) - // Email notification (10 lines) - // Logging (5 lines) - return nil -} - -// AFTER - Extracted functions -func CreateUser(data map[string]interface{}) error { - user, err := validateAndParseUser(data) - if err != nil { - return err - } - if err := saveUser(user); err != nil { - return err - } - if err := sendWelcomeEmail(user); err != nil { - return err - } - logUserCreation(user) - return nil -} -``` - - - -```go -// BEFORE - Deeply nested -func ProcessItem(item Item) error { - if item.IsValid() { - if item.IsReady() { - if item.HasPermission() { - // Process - return nil - } else { - return errors.New("no permission") - } - } else { - return errors.New("not ready") - } - } else { - return errors.New("invalid") - } -} - -// AFTER - Early returns -func ProcessItem(item Item) error { - if !item.IsValid() { - return errors.New("invalid") - } - if !item.IsReady() { - return errors.New("not ready") - } - if !item.HasPermission() { - return errors.New("no permission") - } - // Process - return nil -} -``` - - - -```go -// BEFORE - Long switch in one function -func HandleRequest(reqType string, data interface{}) error { - switch reqType { - case "create": - // 20 lines of creation logic - case "update": - // 20 lines of update logic - case "delete": - // 15 lines of delete logic - default: - return errors.New("unknown type") - } - return nil -} - -// AFTER - Extracted handlers -func HandleRequest(reqType string, data interface{}) error { - switch reqType { - case "create": - return handleCreate(data) - case "update": - return handleUpdate(data) - case "delete": - return handleDelete(data) - default: - return errors.New("unknown type") - } -} - -func handleCreate(data interface{}) error { /* ... */ } -func handleUpdate(data interface{}) error { /* ... */ } -func handleDelete(data interface{}) error { /* ... */ } -``` - - - -**Goal**: Create "islands of clean code" by incrementally pushing dependencies up the call chain - -**Strategy**: Work from bottom-up, rejecting dependencies one level at a time -- DON'T do massive refactoring all at once -- Start at deepest level (furthest from main) -- Extract clean type with dependency injected -- Push global access up one level -- Repeat until global only at entry points - -```go -// BEFORE - Global accessed deep in code -func PublishEvent(event Event) error { - conn, err := nats.Connect(env.Configs.NATsAddress) - // ... complex logic -} - -// AFTER - Dependency rejected up one level -type EventPublisher struct { - natsAddress string // injected, not global -} - -func NewEventPublisher(natsAddress string) *EventPublisher { - return &EventPublisher{natsAddress: natsAddress} -} - -func (p *EventPublisher) Publish(event Event) error { - conn, err := nats.Connect(p.natsAddress) - // ... same logic, now testable -} - -// Caller pushed up (closer to main) -func SetupMessaging() *EventPublisher { - return NewEventPublisher(env.Configs.NATsAddress) // Global only here -} -``` - -**Result**: EventPublisher is now 100% testable without globals - -**Key Principles**: -- **Incremental**: One type at a time, one level at a time -- **Bottom-up**: Start at deepest code, work toward main -- **Pragmatic**: Accept globals at entry points (main, handlers) -- **Testability**: Each extracted type is an island (testable in isolation) - -See [Example 3](./examples.md#example-3-dependency-rejection-pattern) for complete case study. - - - - - -When linter fails, ask these questions (see reference.md for details): - -1. **Does this read like a story?** - - No → Extract functions for different abstraction levels - -2. **Can this be broken into smaller pieces?** - - By responsibility? → Extract functions - - By task? → Extract functions - - By category? → Extract functions - -3. **Does logic run on primitives?** - - Yes → Is this primitive obsession? → Extract type - -4. **Is function long due to switch statement?** - - Yes → Extract case handlers - -5. **Are there deeply nested if/else?** - - Yes → Use early returns or extract functions - - -When creating new types or extracting functions during refactoring: - -**ALWAYS invoke @testing skill** to write tests for: -- **Isolated types**: Types with injected dependencies (testable islands) -- **Value object types**: Types that may depend on other value objects but are still isolated -- **Extracted functions**: New functions created during refactoring -- **Parse functions**: Functions that transform unstructured data - - -A type is an "island of clean code" if: -- Dependencies are explicit (injected via constructor) -- No global or static dependencies -- Can be tested in isolation -- Has 100% testable public API - -**Examples of testable islands:** -- `NATSClient` with injected `natsAddress` string (no other dependencies) -- `Email` type with validation logic (no dependencies) -- `ServiceEndpoint` that uses `Port` value object (both are testable islands) -- `OrderService` with injected `Repository` and `EventPublisher` (all testable) - -**Note**: Islands can depend on other value objects and still be isolated! - +**MANDATORY**: After creating new types or extracting functions, invoke @testing skill. + + +Before marking refactoring complete: +1. List all types created: `grep -r "^type.*struct" internal/` +2. Verify test file exists for each type +3. If missing: STOP and invoke @testing skill to write tests +4. Coverage check: `go test -cover ./...` - leaf types must show 100% +5. Scan for nolint in all uncommitted files (staged + unstaged): + ```bash + { git diff --name-only; git diff --cached --name-only; } | sort -u | xargs grep "//nolint" 2>/dev/null + ``` + If found → remove directive and fix properly (see `` section) + +**BLOCKING**: Do not proceed until tests exist AND no nolint directives in changed files. + -REFACTORING → TESTING: 1. Extract type during refactoring 2. Immediately invoke @testing skill 3. @testing skill writes appropriate tests @@ -483,120 +199,90 @@ REFACTORING → TESTING: + +**NEVER use `//nolint` directives to avoid refactoring.** + +Instead: +- Handle errors (log as fallback, use t.Log in tests) +- Validate input at boundaries +- Refactor to reduce complexity + +**Verification**: After refactoring, scan for nolint in all uncommitted files (staged + unstaged): +```bash +{ git diff --name-only; git diff --cached --name-only; } | sort -u | xargs grep "//nolint" 2>/dev/null +``` +If found → STOP and fix properly + +See `reference.md` for acceptable exceptions (rare, requires user approval). + + -**STOP when ALL of these are met:** -- Linter passes +**STOP when ALL are met:** +- Linter passes (0 issues) - All functions < 50 LOC - Nesting ≤ 2 levels - Code reads like a story - No more "juicy" abstractions to extract **Warning Signs of Over-Engineering:** -- Creating types with only one method +- Types with only one method - Functions that just call another function - More abstraction layers than necessary - Code becomes harder to understand -- Diminishing returns on complexity reduction -**Pragmatic Approach:** -IF linter passes AND code is readable: - STOP - Don't extract more -EVEN IF you could theoretically extract more: - STOP - Avoid abstraction bloat +IF linter passes AND code is readable → STOP (avoid abstraction bloat) ``` -CONTEXT ANALYSIS - -Function: CreateUser (user/service.go:45) -Role: Core user creation orchestration -Called by: -- api/handler.go:89 (HTTP endpoint) -- cmd/user.go:34 (CLI command) - -Potential types spotted: -- Email: Complex validation logic scattered -- UserID: Generation and validation logic - REFACTORING APPLIED -Patterns Successfully Applied: -1. Early Returns: Reduced nesting from 4 to 2 levels -2. Storifying: Extracted validate(), save(), notify() -3. Extract Type: Created Email and PhoneNumber types - -Patterns Tried but Insufficient: -- Extract Function alone: Still too complex, needed types +Patterns Applied: +1. [Pattern]: [What changed] +2. [Pattern]: [What changed] Types Created (with Juiciness Score): +- [Type] (JUICY - [reason]): [methods] + → Invoke @testing skill -Email type (JUICY - Behavioral + Usage): -- Behavioral: ParseEmail(), Domain(), LocalPart() methods -- Usage: Used in 5+ places across codebase -- Island: Testable in isolation -- → Invoke @testing skill to write tests - -Types Considered but Rejected (NOT JUICY): -- UserID: Only empty check, good naming sufficient -- Status: Just string constants, enum adequate - -METRICS - -Complexity Reduction: -- Cyclomatic: 18 → 7 -- Cognitive: 25 → 8 -- LOC: 120 → 45 -- Nesting: 4 → 2 +Types Rejected (NOT JUICY): +- [Type]: [reason - good naming sufficient] -FILES MODIFIED +Metrics: +- Cyclomatic: [before] → [after] +- LOC: [before] → [after] +- Nesting: [before] → [after] -Modified: -- user/service.go (+15, -75 lines) +Files Modified: +- [file] (+X, -Y lines) Created (Islands of Clean Code): -- user/email.go (new, +45 lines) → Ready for @testing skill +- [file] (new) → Ready for @testing skill -AUTOMATION COMPLETE - -Stopping Criteria Met: -- Linter passes (0 issues) -- All functions < 50 LOC -- Max nesting = 2 levels -- Code reads like a story -- No more juicy abstractions - -Ready for: @pre-commit-review phase +STATUS: [Linter passes / Still failing: X issues] ``` -**Invoked By (Automatic Triggering)**: -- **@linter-driven-development**: Automatically invokes when linter fails (Phase 3) -- **@pre-commit-review**: Automatically invokes when design issues detected (Phase 4) - -**Iterative Loop**: -1. Linter fails → invoke @refactoring -2. Refactoring complete → re-run linter -3. Linter passes → invoke @pre-commit-review -4. Review finds design debt → invoke @refactoring again -5. Repeat until both linter AND review pass - -**Invokes (When Needed)**: -- **@code-designing**: When refactoring creates new types, validate design -- **@testing**: Automatically invoked to write tests for new types/functions -- **@pre-commit-review**: Validates design quality after linting passes - -See [reference.md](./reference.md) for complete refactoring patterns and decision tree. +**Invoked By**: +- @linter-driven-development: When linter fails (Phase 3) +- @pre-commit-review: When design issues detected + +**Invokes**: +- @code-designing: When file splitting needed or new types require design validation +- @testing: After creating new types/functions (MANDATORY) +- @pre-commit-review: After linting passes (validates design quality) + +**Loop**: Linter fails → @refactoring → re-run linter → @pre-commit-review → repeat until both pass -Refactoring is complete when ALL of the following are true: +Refactoring is complete when ALL are true: - [ ] Linter passes (0 issues) - [ ] All functions < 50 LOC - [ ] Max nesting ≤ 2 levels -- [ ] Code reads like a story (single abstraction level per function) +- [ ] Code reads like a story - [ ] No more "juicy" abstractions to extract - [ ] Tests written for new types/functions (via @testing skill) - [ ] Ready for @pre-commit-review phase diff --git a/go-linter-driven-development/skills/testing/SKILL.md b/go-linter-driven-development/skills/testing/SKILL.md index c7339e6..94a73b3 100644 --- a/go-linter-driven-development/skills/testing/SKILL.md +++ b/go-linter-driven-development/skills/testing/SKILL.md @@ -26,7 +26,7 @@ Ready after tests? Run linter: `task lintwithfix` -- **Automatically invoked** by @linter-driven-development during Phase 2 (Implementation) +- **Automatically invoked** by @linter-driven-development during Phase 1 (Implementation Foundation) - **Automatically invoked** by @refactoring when new isolated types are created - **Automatically invoked** by @code-designing after designing new types - **After creating new leaf types** - Types that should have 100% unit test coverage