From 96f5892b73d033a3fd7a3ccd0bfa914256ac9efc Mon Sep 17 00:00:00 2001 From: Dan Mordechay Date: Sun, 25 Jan 2026 15:24:02 +0200 Subject: [PATCH 1/2] improved flow and catch linting failures --- .../agents/go-code-reviewer.md | 3 +- .../agents/quality-analyzer.md | 16 ++ .../commands/go-ldd-analyze.md | 1 + .../commands/go-ldd-autopilot.md | 3 +- .../commands/go-ldd-quickfix.md | 11 +- .../commands/go-ldd-review.md | 1 + .../commands/go-ldd-status.md | 1 + .../skills/code-designing/SKILL.md | 106 ++++++++ .../skills/linter-driven-development/SKILL.md | 94 ++++++- .../skills/refactoring/SKILL.md | 255 +++++++++++++++++- .../skills/testing/SKILL.md | 2 +- 11 files changed, 479 insertions(+), 14 deletions(-) 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..9490642 100644 --- a/go-linter-driven-development/skills/refactoring/SKILL.md +++ b/go-linter-driven-development/skills/refactoring/SKILL.md @@ -8,6 +8,7 @@ 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 --- @@ -18,6 +19,18 @@ Operates autonomously - no user confirmation needed during execution. **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? @@ -92,9 +105,16 @@ The analysis produces a refactoring plan identifying: **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 +- **Cyclomatic Complexity** (`cyclop`): Too many decision points → Extract functions, simplify logic +- **Cognitive Complexity** (`gocognit`): Hard to understand → Storifying, reduce nesting +- **Maintainability Index** (`maintidx`): Hard to maintain → Break into smaller pieces +- **Deep Nesting** (`nestif`): >2 nesting levels → **Storify first** → Early returns → Extract function +- **Function Length** (`funlen`): Function too long → **Storify first** → Extract functions + +**Nesting Depth (`nestif`) - Priority Pattern:** +1. **Storify first** - Make code read like a story, reveals hidden structure +2. **Early returns** - Invert conditions, exit early +3. **Extract function** - Pull nested blocks into named functions **Architectural Issues:** - **noglobals/gochecknoglobals**: Global variable usage → Dependency rejection pattern @@ -105,6 +125,10 @@ The analysis produces a refactoring plan identifying: - **dupl**: Code duplication → Extract common logic/types - **goconst**: Magic strings/numbers → Extract constants or types - **ineffassign**: Ineffective assignments → Simplify logic + +**Error Handling:** +- **wrapcheck**: Unwrapped external errors → `fmt.Errorf("context: %w", err)` +- **early-return** (revive): Superfluous else → Invert condition, return early @@ -114,7 +138,33 @@ The analysis produces a refactoring plan identifying: - Unclear flow/purpose - Primitive obsession - Global variable access scattered throughout code +- **Files > 450 lines** (`file-length-limit` revive rule) - Route to @code-designing for file splitting + + +**When `file-length-limit` (revive) triggers (>450 lines):** + +Analyze file structure before refactoring: + +| File Pattern | Diagnosis | Pattern | +|--------------|-----------|---------| +| Multiple juicy types | Types scattered in one file | Route to @code-designing → Juicy type per file | +| Single god type | One type doing too much | Route to @code-designing → Extract via composition | +| Long functions, few types | Function bloat | **Storify → Extract functions** → Reassess | + +**"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): +- Simple enums (const block only) +- DTOs with no methods +- Type aliases + +**Key Insight:** Storification often reveals extraction opportunities - it's the first step in any refactoring. + @@ -137,12 +187,17 @@ AUTOMATED PROCESS: **For Complexity Failures** (cyclomatic, cognitive, maintainability): -1. Early Returns → Reduce nesting quickly -2. Extract Function → Break up long functions -3. Storifying → Improve abstraction levels +1. Storifying → **Always start here** - makes code read like a story, reveals hidden structure +2. Early Returns → Reduce nesting quickly +3. Extract Function → Break up long functions 4. Extract Type → Create domain types (only if "juicy") 5. Switch Extraction → Categorize switch cases +**For Nesting Depth Failures** (`nestif`): +1. Storify → Make code read like a story first +2. Early Returns → Invert conditions, exit early +3. Extract Function → Pull deeply nested blocks into named functions + **For Architectural Failures** (noglobals, singletons): 1. Dependency Rejection → Incremental bottom-up approach 2. Extract Type with dependency injection @@ -274,6 +329,57 @@ func ParseCommandResult(output string) (CommandResult, error) { See [Example 2](./examples.md#first-refactoring-attempt-the-over-abstraction-trap) for complete case study. + +**Type Cohesion Rule**: When extracting a type to its own file, **co-locate ALL related declarations**: + +```go +// WRONG - OrderStatus type in order.go but constants scattered elsewhere +// processor.go +type OrderStatus string // defined here but... +// ... later in another file or same file far away +const ( + OrderStatusPending OrderStatus = "pending" + OrderStatusShipped OrderStatus = "shipped" +) + +// CORRECT - Type and its constants in same file, together +// order.go +type OrderStatus string + +const ( + OrderStatusPending OrderStatus = "pending" + OrderStatusProcessing OrderStatus = "processing" + OrderStatusShipped OrderStatus = "shipped" + OrderStatusDelivered OrderStatus = "delivered" + OrderStatusCancelled OrderStatus = "cancelled" +) + +func (s OrderStatus) IsValid() bool { ... } +func (s OrderStatus) CanTransitionTo(target OrderStatus) error { ... } +``` + +**What to co-locate with a type:** +- Type definition +- Type constants (enum values) +- Type-specific errors (if not centralized in errors.go) +- Constructor function(s) (`New*`, `Parse*`) +- All methods on the type +- Related helper functions that operate on the type + +**Verification**: After extracting type, grep for type name in other files: +```bash +grep -r "TypeName" --include="*.go" . | grep -v "type_file.go" +``` +If found → move declaration to type's file + +**Process**: +1. Extract type to its own file (e.g., `order_status.go`) +2. Find all constants of that type → move to same file +3. Find all methods on that type → move to same file +4. Find constructor/parser functions → move to same file +5. Verify: only imports of the type remain in other files + + ```go // BEFORE - Long function @@ -425,6 +531,60 @@ func SetupMessaging() *EventPublisher { See [Example 3](./examples.md#example-3-dependency-rejection-pattern) for complete case study. + +**Strategy**: Break orchestrator into focused services using composition + +```go +// BEFORE - God object with 25+ methods +type DataProcessor struct { + users map[string]*User + orders map[string]*Order + cache map[string]any + httpClient *http.Client + notifications []Notification +} + +func (dp *DataProcessor) CreateUser(...) { } +func (dp *DataProcessor) GetUser(...) { } +func (dp *DataProcessor) UpdateUser(...) { } +func (dp *DataProcessor) DeleteUser(...) { } +func (dp *DataProcessor) CreateOrder(...) { } +func (dp *DataProcessor) GetOrder(...) { } +// ... 20+ more methods + +// AFTER - Composed services +type DataProcessor struct { + users *UserService + orders *OrderService + cache *CacheService + notifier *NotificationService +} + +func NewDataProcessor(config Config) (*DataProcessor, error) { + return &DataProcessor{ + users: NewUserService(), + orders: NewOrderService(), + cache: NewCacheService(), + notifier: NewNotificationService(config), + }, nil +} + +// Delegate to focused services +func (dp *DataProcessor) CreateUser(req CreateUserRequest) (*User, error) { + return dp.users.Create(req) +} +``` + +**Process**: +1. Group methods by noun (User, Order, Cache, Notification) +2. Extract each group into a focused service type +3. Move relevant fields to each service +4. Compose services in the orchestrator +5. Orchestrator delegates, doesn't implement + +**Threshold**: >15 methods = god object candidate + + @@ -451,12 +611,22 @@ When linter fails, ask these questions (see reference.md for details): When creating new types or extracting functions during refactoring: -**ALWAYS invoke @testing skill** to write tests for: +**MANDATORY: 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 + +**ENFORCEMENT**: Before marking refactoring complete: +1. List all types created: `grep -r "^type.*struct" internal/` +2. For each type, verify test file exists: `internal/[pkg]/[type]_test.go` +3. If missing: STOP and invoke @testing skill +4. Coverage check: `go test -cover ./...` - leaf types must show 100% + +**BLOCKING**: Do not proceed to next phase until tests exist for all extracted types. + + A type is an "island of clean code" if: - Dependencies are explicit (injected via constructor) @@ -505,6 +675,77 @@ EVEN IF you could theoretically extract more: STOP - Avoid abstraction bloat + +**NEVER use `//nolint` directives to avoid refactoring.** These are lazy shortcuts that hide problems. + + +```go +// FORBIDDEN - Hiding error handling +defer func() { + resp.Body.Close() //nolint:errcheck,gosec // Best effort +}() + +// FORBIDDEN - Hiding security concerns +data, err := os.ReadFile(path) //nolint:gosec // Trust caller + +// FORBIDDEN - Hiding complexity +func BigFunction() { //nolint:gocognit // It's fine +``` + + + +```go +// CORRECT - Handle the error (log as fallback) +defer func() { + if err := resp.Body.Close(); err != nil { + log.Printf("failed to close response body: %v", err) + } +}() + +// CORRECT - In tests, use t.Log +defer func() { + if err := resp.Body.Close(); err != nil { + t.Logf("failed to close response body: %v", err) + } +}() + +// CORRECT - Validate input at boundaries +func ParseConfig(path string) (*Config, error) { + if !filepath.IsAbs(path) { + return nil, errors.New("path must be absolute") + } + // Now gosec is satisfied because path is validated + data, err := os.ReadFile(path) +} + +// CORRECT - Refactor to reduce complexity +// Split BigFunction into smaller, focused functions +``` + + + +**When Nolint IS Acceptable** (rare): +Only use `//nolint` when ALL of these are true: +1. You have exhausted all refactoring options +2. The linter is genuinely wrong (false positive) +3. You add the nolint to `.golangci.yaml` exclusions, not inline +4. You get explicit user approval before adding + + + +**Verification**: After refactoring, scan changed files (both staged and unstaged) for nolint: +```bash +# Check unstaged changes +git diff --name-only | xargs grep "//nolint" 2>/dev/null +# Check staged changes +git diff --cached --name-only | xargs grep "//nolint" 2>/dev/null +``` +If found in any changed files → STOP and fix properly instead + +Note: Existing `//nolint` in unchanged files is acceptable (legacy code) + + + ``` CONTEXT ANALYSIS 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 From 278ff45ad661d09465b895a27f80040b1e0bfa74 Mon Sep 17 00:00:00 2001 From: Dan Mordechay Date: Mon, 2 Mar 2026 13:56:21 +0200 Subject: [PATCH 2/2] heal refactoring skill --- .../skills/refactoring/SKILL.md | 849 +++--------------- 1 file changed, 147 insertions(+), 702 deletions(-) diff --git a/go-linter-driven-development/skills/refactoring/SKILL.md b/go-linter-driven-development/skills/refactoring/SKILL.md index 9490642..ec25a95 100644 --- a/go-linter-driven-development/skills/refactoring/SKILL.md +++ b/go-linter-driven-development/skills/refactoring/SKILL.md @@ -15,7 +15,7 @@ allowed-tools: 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. @@ -34,7 +34,7 @@ Operates autonomously - no user confirmation needed during execution. 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 @@ -46,605 +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 + - -Proactively identify hidden types in the code: + +**When `file-length-limit` triggers (>450 lines):** -POTENTIAL TYPES TO DISCOVER: -1. Data being parsed from strings → Parse* types - Example: ParseCommandResult(), ParseLogEntry() +| 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 | -2. Scattered validation logic → Validated types - Example: Email, Port, IPAddress types +**"Juicy" types** (deserve own file): ≥2 methods, complex validation, transformations/parsing +**Anemic types** (can stay grouped): Simple enums, DTOs without methods, type aliases + + -3. Data that always travels together → Aggregate types - Example: UserCredentials, ServerConfig + +**Pattern Priority Order** (for complexity failures): -4. Complex conditions → State/status types - Example: DeploymentStatus with IsReady(), CanProceed() +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 -5. Repeated string manipulation → Types with methods - Example: FilePath with Dir(), Base(), Ext() - +See `reference.md` for detailed patterns with code examples. - -The analysis produces a refactoring plan identifying: -- Function's role in the system -- Potential domain types to extract -- Recommended refactoring approach -- Expected complexity reduction - - + +Before extracting a type, verify it's "juicy" (worth creating): - - -**Complexity Issues:** -- **Cyclomatic Complexity** (`cyclop`): Too many decision points → Extract functions, simplify logic -- **Cognitive Complexity** (`gocognit`): Hard to understand → Storifying, reduce nesting -- **Maintainability Index** (`maintidx`): Hard to maintain → Break into smaller pieces -- **Deep Nesting** (`nestif`): >2 nesting levels → **Storify first** → Early returns → Extract function -- **Function Length** (`funlen`): Function too long → **Storify first** → Extract functions - -**Nesting Depth (`nestif`) - Priority Pattern:** -1. **Storify first** - Make code read like a story, reveals hidden structure -2. **Early returns** - Invert conditions, exit early -3. **Extract function** - Pull nested blocks into named functions - -**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 - -**Error Handling:** -- **wrapcheck**: Unwrapped external errors → `fmt.Errorf("context: %w", err)` -- **early-return** (revive): Superfluous else → Invert condition, return early - - - -- Functions > 50 LOC -- Nesting > 2 levels -- Mixed abstraction levels -- Unclear flow/purpose -- Primitive obsession -- Global variable access scattered throughout code -- **Files > 450 lines** (`file-length-limit` revive rule) - Route to @code-designing for file splitting - +**BEHAVIORAL**: Complex validation, ≥2 meaningful methods, state transitions +**STRUCTURAL**: Parsing unstructured data, grouping related data +**USAGE**: Used in multiple places, simplifies calling code - -**When `file-length-limit` (revive) triggers (>450 lines):** +Need "yes" in at least ONE category. See `reference.md` section 2.5 for over-abstraction warnings. + -Analyze file structure before refactoring: + +When extracting a type to its own file, co-locate ALL related declarations: +- Type definition + constants + constructor + all methods -| File Pattern | Diagnosis | Pattern | -|--------------|-----------|---------| -| Multiple juicy types | Types scattered in one file | Route to @code-designing → Juicy type per file | -| Single god type | One type doing too much | Route to @code-designing → Extract via composition | -| Long functions, few types | Function bloat | **Storify → Extract functions** → Reassess | +Verify: `grep -r "TypeName" --include="*.go" . | grep -v "type_file.go"` +If found elsewhere → move to type's file + -**"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) + +**Trigger**: Type has >15 methods OR >500 LOC -**Anemic types** (can stay grouped): -- Simple enums (const block only) -- DTOs with no methods -- Type aliases +**Strategy** (in order): -**Key Insight:** Storification often reveals extraction opportunities - it's the first step in any refactoring. - - +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 - -This skill operates completely autonomously once invoked: + 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. Storifying → **Always start here** - makes code read like a story, reveals hidden structure -2. Early Returns → Reduce nesting quickly -3. Extract Function → Break up long functions -4. Extract Type → Create domain types (only if "juicy") -5. Switch Extraction → Categorize switch cases - -**For Nesting Depth Failures** (`nestif`): -1. Storify → Make code read like a story first -2. Early Returns → Invert conditions, exit early -3. Extract Function → Pull deeply nested blocks into named functions - -**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. - - - -**Type Cohesion Rule**: When extracting a type to its own file, **co-locate ALL related declarations**: - -```go -// WRONG - OrderStatus type in order.go but constants scattered elsewhere -// processor.go -type OrderStatus string // defined here but... -// ... later in another file or same file far away -const ( - OrderStatusPending OrderStatus = "pending" - OrderStatusShipped OrderStatus = "shipped" -) - -// CORRECT - Type and its constants in same file, together -// order.go -type OrderStatus string - -const ( - OrderStatusPending OrderStatus = "pending" - OrderStatusProcessing OrderStatus = "processing" - OrderStatusShipped OrderStatus = "shipped" - OrderStatusDelivered OrderStatus = "delivered" - OrderStatusCancelled OrderStatus = "cancelled" -) - -func (s OrderStatus) IsValid() bool { ... } -func (s OrderStatus) CanTransitionTo(target OrderStatus) error { ... } -``` - -**What to co-locate with a type:** -- Type definition -- Type constants (enum values) -- Type-specific errors (if not centralized in errors.go) -- Constructor function(s) (`New*`, `Parse*`) -- All methods on the type -- Related helper functions that operate on the type - -**Verification**: After extracting type, grep for type name in other files: -```bash -grep -r "TypeName" --include="*.go" . | grep -v "type_file.go" -``` -If found → move declaration to type's file - -**Process**: -1. Extract type to its own file (e.g., `order_status.go`) -2. Find all constants of that type → move to same file -3. Find all methods on that type → move to same file -4. Find constructor/parser functions → move to same file -5. Verify: only imports of the type remain in other files - - - -```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. - - - -**Strategy**: Break orchestrator into focused services using composition - -```go -// BEFORE - God object with 25+ methods -type DataProcessor struct { - users map[string]*User - orders map[string]*Order - cache map[string]any - httpClient *http.Client - notifications []Notification -} - -func (dp *DataProcessor) CreateUser(...) { } -func (dp *DataProcessor) GetUser(...) { } -func (dp *DataProcessor) UpdateUser(...) { } -func (dp *DataProcessor) DeleteUser(...) { } -func (dp *DataProcessor) CreateOrder(...) { } -func (dp *DataProcessor) GetOrder(...) { } -// ... 20+ more methods - -// AFTER - Composed services -type DataProcessor struct { - users *UserService - orders *OrderService - cache *CacheService - notifier *NotificationService -} - -func NewDataProcessor(config Config) (*DataProcessor, error) { - return &DataProcessor{ - users: NewUserService(), - orders: NewOrderService(), - cache: NewCacheService(), - notifier: NewNotificationService(config), - }, nil -} - -// Delegate to focused services -func (dp *DataProcessor) CreateUser(req CreateUserRequest) (*User, error) { - return dp.users.Create(req) -} -``` - -**Process**: -1. Group methods by noun (User, Order, Cache, Notification) -2. Extract each group into a focused service type -3. Move relevant fields to each service -4. Compose services in the orchestrator -5. Orchestrator delegates, doesn't implement - -**Threshold**: >15 methods = god object candidate - - - - - -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: - -**MANDATORY: 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 +**MANDATORY**: After creating new types or extracting functions, invoke @testing skill. -**ENFORCEMENT**: Before marking refactoring complete: +Before marking refactoring complete: 1. List all types created: `grep -r "^type.*struct" internal/` -2. For each type, verify test file exists: `internal/[pkg]/[type]_test.go` -3. If missing: STOP and invoke @testing skill +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 to next phase until tests exist for all extracted types. +**BLOCKING**: Do not proceed until tests exist AND no nolint directives in changed files. - -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! - - -REFACTORING → TESTING: 1. Extract type during refactoring 2. Immediately invoke @testing skill 3. @testing skill writes appropriate tests @@ -653,191 +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) - -**NEVER use `//nolint` directives to avoid refactoring.** These are lazy shortcuts that hide problems. - - -```go -// FORBIDDEN - Hiding error handling -defer func() { - resp.Body.Close() //nolint:errcheck,gosec // Best effort -}() - -// FORBIDDEN - Hiding security concerns -data, err := os.ReadFile(path) //nolint:gosec // Trust caller - -// FORBIDDEN - Hiding complexity -func BigFunction() { //nolint:gocognit // It's fine -``` - - - -```go -// CORRECT - Handle the error (log as fallback) -defer func() { - if err := resp.Body.Close(); err != nil { - log.Printf("failed to close response body: %v", err) - } -}() - -// CORRECT - In tests, use t.Log -defer func() { - if err := resp.Body.Close(); err != nil { - t.Logf("failed to close response body: %v", err) - } -}() - -// CORRECT - Validate input at boundaries -func ParseConfig(path string) (*Config, error) { - if !filepath.IsAbs(path) { - return nil, errors.New("path must be absolute") - } - // Now gosec is satisfied because path is validated - data, err := os.ReadFile(path) -} - -// CORRECT - Refactor to reduce complexity -// Split BigFunction into smaller, focused functions -``` - - - -**When Nolint IS Acceptable** (rare): -Only use `//nolint` when ALL of these are true: -1. You have exhausted all refactoring options -2. The linter is genuinely wrong (false positive) -3. You add the nolint to `.golangci.yaml` exclusions, not inline -4. You get explicit user approval before adding - - - -**Verification**: After refactoring, scan changed files (both staged and unstaged) for nolint: -```bash -# Check unstaged changes -git diff --name-only | xargs grep "//nolint" 2>/dev/null -# Check staged changes -git diff --cached --name-only | xargs grep "//nolint" 2>/dev/null -``` -If found in any changed files → STOP and fix properly instead - -Note: Existing `//nolint` in unchanged files is acceptable (legacy code) - - - ``` -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 Rejected (NOT JUICY): +- [Type]: [reason - good naming sufficient] -Types Considered but Rejected (NOT JUICY): -- UserID: Only empty check, good naming sufficient -- Status: Just string constants, enum adequate +Metrics: +- Cyclomatic: [before] → [after] +- LOC: [before] → [after] +- Nesting: [before] → [after] -METRICS - -Complexity Reduction: -- Cyclomatic: 18 → 7 -- Cognitive: 25 → 8 -- LOC: 120 → 45 -- Nesting: 4 → 2 - -FILES MODIFIED - -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 - -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 +- [file] (new) → Ready for @testing skill -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