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