From 64cad7af382ef5574f4a2e17c0716ab269c988af Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Fri, 27 Feb 2026 14:04:17 -0600 Subject: [PATCH 1/2] feat: add golangci-lint skill for Claude Code Creates a skill to parse, analyze, and fix golangci-lint JSON reports. Supports interactive and batch fix modes with test validation. Includes: - SKILL.md with JSON structure reference and fix workflows - references/linter-guide.md with detailed fix patterns per linter Co-Authored-By: Claude Opus 4.5 --- .claude/skills/golangci-lint/SKILL.md | 316 +++++++++++ .../golangci-lint/references/linter-guide.md | 503 ++++++++++++++++++ .gitignore | 4 +- 3 files changed, 822 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/golangci-lint/SKILL.md create mode 100644 .claude/skills/golangci-lint/references/linter-guide.md diff --git a/.claude/skills/golangci-lint/SKILL.md b/.claude/skills/golangci-lint/SKILL.md new file mode 100644 index 0000000..72db1f3 --- /dev/null +++ b/.claude/skills/golangci-lint/SKILL.md @@ -0,0 +1,316 @@ +--- +name: golangci-lint +description: This skill should be used when the user asks to "parse golangci-lint output", "fix lint errors", "review lint report", "analyze golangci-lint JSON", "fix go lint issues", "apply lint suggestions", "run golangci-lint" +version: 0.1.0 +--- + +# golangci-lint Skill + +Parse, analyze, and fix issues from golangci-lint JSON reports. + +## Overview + +This skill helps you: +1. Parse golangci-lint JSON output and present issues in a structured way +2. Fix lint issues interactively (one at a time) or in batch (by linter) +3. Apply suggested fixes when available +4. Validate fixes by running tests + +## Usage + +To generate a JSON report: +```bash +golangci-lint run --output-format json > tmp/golangci-lint-report.json 2>&1 +``` + +Common report locations: +- `tmp/golangci-lint-report.json` +- `golangci-lint-report.json` + +## JSON Structure Reference + +### Top-Level Object +```json +{ + "Issues": [...], + "Report": { "Linters": [...] } +} +``` + +### Issue Object +| Field | Type | Description | +|-------|------|-------------| +| `FromLinter` | string | Linter name (e.g., "err113", "gosec") | +| `Text` | string | Error message | +| `Severity` | string | "" (default), "medium", or "high" | +| `SourceLines` | []string | Source code lines at issue location | +| `Pos` | object | Position: `{Filename, Offset, Line, Column}` | +| `SuggestedFixes` | []object | Optional auto-fix data | +| `ExpectNoLint` | bool | Whether nolint was expected | +| `ExpectedNoLintLinter` | string | Expected nolint linter | + +### SuggestedFix Object +```json +{ + "Message": "Description of fix", + "TextEdits": [ + { + "Pos": 1234, // byte offset start + "End": 1290, // byte offset end + "NewText": "..." // base64-encoded replacement text (null = delete) + } + ] +} +``` + +## Fix Modes + +### Interactive Mode (Default) +Review and fix issues one at a time with full context: +1. Show issue details (linter, file, line, message) +2. Display source context +3. Present fix options: + - Apply suggested fix (if available) + - Generate fix based on linter rules + - Skip this issue + - Stop fixing + +### Batch Mode +Fix all issues from a specific linter at once: +1. Group issues by linter +2. User selects which linter to address +3. Apply fixes for all issues from that linter +4. Show summary of changes + +## Fix Workflow with Validation + +``` +1. Check if tests exist for affected code + └─ Look for *_test.go files in same package + +2. Apply fix + └─ Use suggested fix OR generate fix based on linter rules + +3. Run tests to validate + └─ go test ./path/to/package/... + +4. If tests fail: + └─ Prompt user: + - "Proceed anyway" (keep fix) + - "Revert and fix manually" (undo fix) +``` + +## Fix Strategies by Linter + +### err113 - Static Error Definitions +**Problem**: Dynamic errors with `fmt.Errorf()` without wrapping + +**Fix**: Define static sentinel errors and wrap them +```go +// Before +return fmt.Errorf("workload %q not found", name) + +// After +var errWorkloadNotFound = errors.New("workload not found") +// ... +return fmt.Errorf("%w: %q", errWorkloadNotFound, name) +``` + +### errcheck - Unchecked Error Returns +**Problem**: Error return values not checked + +**Fix Options**: +1. Check the error: +```go +// Before +auditor.Close() + +// After +if err := auditor.Close(); err != nil { + // handle or log +} +``` + +2. Explicitly ignore (for known-safe calls like `fmt.Fprintf` to stdout): +```go +_ = fmt.Fprintf(out, "message") +``` + +### errname - Sentinel Error Naming +**Problem**: Sentinel error names don't follow `errXxx` convention + +**Fix**: Rename to `errXxx` format +```go +// Before +var buildErr error + +// After +var errBuild error +``` + +### errorlint - Error Type Assertions +**Problem**: Direct type assertion on errors fails with wrapped errors + +**Fix**: Use `errors.As()`: +```go +// Before +if exitErr, ok := err.(*exec.ExitError); ok { + +// After +var exitErr *exec.ExitError +if errors.As(err, &exitErr) { +``` + +### gosec - Security Issues +**Problem**: Various security concerns + +**Fixes by code**: +- **G115 (integer overflow)**: Add bounds checking +```go +// Before +Cores: uint32(opts.CPUCores) + +// After +if opts.CPUCores < 0 || opts.CPUCores > math.MaxUint32 { + return nil, errors.New("CPU cores out of range") +} +Cores: uint32(opts.CPUCores) +``` + +- **G301 (directory permissions)**: Use 0750 or less +```go +os.MkdirAll(dir, 0o750) // instead of 0o755 +``` + +- **G306 (file permissions)**: Use 0600 or less +```go +os.WriteFile(path, data, 0o600) // instead of 0644 +``` + +- **G204 (command injection)**: Validate inputs or use allowlist + +### modernize - Modern Go Idioms +**rangeint** - Use range over int (Go 1.22+): +```go +// Before +for i := 0; i < count; i++ { + +// After +for i := range count { +``` + +**forvar** - Remove unnecessary loop variable capture (Go 1.22+): +```go +// Before +for _, p := range items { + p := p // no longer needed + go func() { use(p) }() +} + +// After +for _, p := range items { + go func() { use(p) }() +} +``` + +**any** - Replace `interface{}` with `any`: +```go +// Before +map[string]interface{} + +// After +map[string]any +``` + +**mapsloop** - Use `maps.Copy`: +```go +// Before +for k, v := range src { + dst[k] = v +} + +// After +maps.Copy(dst, src) +``` + +**minmax** - Use `min`/`max` builtins: +```go +// Before +if count < 1 { + count = 1 +} + +// After +count := max(w.Config.VMCount, 1) +``` + +### staticcheck - Static Analysis +**ST1023** - Omit redundant type in declaration: +```go +// Before +var name string = "" + +// After +var name = "" +// or just: name := "" +``` + +**SA4000** - Identical expressions: +```go +// Fix the logic error - expressions like x - x are always 0 +``` + +### gci / gofmt / golines - Formatting +These have suggested fixes - apply them directly using the base64-decoded `NewText`. + +## Applying SuggestedFixes + +When an issue has `SuggestedFixes`, decode and apply: + +```go +import "encoding/base64" + +// Decode NewText (if not null) +decoded, err := base64.StdEncoding.DecodeString(edit.NewText) + +// Apply: replace bytes from Pos to End with decoded text +// If NewText is null, delete the range +``` + +## Workflow Commands + +When invoked, I will: + +1. **Parse Report**: Load and parse the JSON file +2. **Summarize**: Show issue counts by linter, file, and severity +3. **Ask Mode**: Interactive or batch? +4. **Fix Loop**: + - Show issue details + - Apply fix (suggested or generated) + - Validate with tests if available + - Continue or stop based on results + +## Example Session + +``` +User: /golangci-lint tmp/golangci-lint-report.json + +Claude: Parsed golangci-lint report. Found 55 issues: + +By Linter: +- errcheck: 24 issues +- err113: 10 issues +- modernize: 9 issues +- gosec: 5 issues (2 high, 3 medium) +- gci: 2 issues +- staticcheck: 2 issues +- errname: 1 issue +- errorlint: 1 issue +- golines: 1 issue + +How would you like to proceed? +1. Interactive mode (review each issue) +2. Batch mode (fix all issues from one linter) +3. Show issues by file +4. Show high-severity issues first +``` diff --git a/.claude/skills/golangci-lint/references/linter-guide.md b/.claude/skills/golangci-lint/references/linter-guide.md new file mode 100644 index 0000000..46e9fc2 --- /dev/null +++ b/.claude/skills/golangci-lint/references/linter-guide.md @@ -0,0 +1,503 @@ +# Linter Guide - Detailed Fix Patterns + +Comprehensive reference for fixing issues from common golangci-lint linters. + +## Table of Contents + +- [err113 - Static Errors](#err113---static-errors) +- [errcheck - Error Checking](#errcheck---error-checking) +- [errname - Error Naming](#errname---error-naming) +- [errorlint - Error Handling](#errorlint---error-handling) +- [gosec - Security](#gosec---security) +- [modernize - Modern Go](#modernize---modern-go) +- [staticcheck - Static Analysis](#staticcheck---static-analysis) +- [gci - Import Ordering](#gci---import-ordering) +- [gofmt / gofumpt - Formatting](#gofmt--gofumpt---formatting) +- [golines - Line Length](#golines---line-length) + +--- + +## err113 - Static Errors + +**Purpose**: Enforce use of wrapped static errors instead of dynamic `fmt.Errorf()` calls. + +### Pattern 1: Simple Error Message +```go +// Before +return fmt.Errorf("connection failed") + +// After - at package level +var errConnectionFailed = errors.New("connection failed") + +// At call site +return errConnectionFailed +``` + +### Pattern 2: Error with Context +```go +// Before +return fmt.Errorf("workload %q not found", name) + +// After - at package level +var errWorkloadNotFound = errors.New("workload not found") + +// At call site - wrap with context +return fmt.Errorf("%w: %q", errWorkloadNotFound, name) +``` + +### Pattern 3: Multiple Dynamic Values +```go +// Before +return fmt.Errorf("timeout waiting for %s/%s", namespace, name) + +// After +var errTimeout = errors.New("timeout waiting for resource") + +return fmt.Errorf("%w: %s/%s", errTimeout, namespace, name) +``` + +### Pattern 4: In Test Files +For test files, consider whether static errors are needed. Options: +1. **nolint directive** if error is test-specific: + ```go + return fmt.Errorf("simulated error") //nolint:err113 + ``` +2. **Define test errors** if reused: + ```go + var errSimulatedDelete = errors.New("simulated delete error") + ``` + +### Import Required +```go +import "errors" +``` + +--- + +## errcheck - Error Checking + +**Purpose**: Ensure error return values are handled. + +### Pattern 1: Must Handle Error +```go +// Before +file.Close() + +// After +if err := file.Close(); err != nil { + return fmt.Errorf("closing file: %w", err) +} +``` + +### Pattern 2: Defer with Error +```go +// Before +defer file.Close() + +// After - option 1: named return +func doSomething() (err error) { + file, err := os.Open(path) + if err != nil { + return err + } + defer func() { + if cerr := file.Close(); cerr != nil && err == nil { + err = cerr + } + }() + // ... +} + +// After - option 2: log on failure (for cleanup) +defer func() { + if err := file.Close(); err != nil { + log.Printf("failed to close file: %v", err) + } +}() +``` + +### Pattern 3: Known-Safe Ignores +Some functions rarely fail when used correctly. Explicitly ignore: +```go +// Writing to stdout/stderr (rarely fails) +_ = fmt.Fprintf(os.Stdout, "message\n") + +// Closing read-only files +_ = file.Close() + +// Environment in tests +_ = os.Setenv("KEY", "value") +``` + +### Pattern 4: Flag Setting in Tests +```go +// Before +cmd.Flags().Set("config", path) + +// After +if err := cmd.Flags().Set("config", path); err != nil { + t.Fatalf("failed to set flag: %v", err) +} + +// Or if truly safe to ignore in test setup +_ = cmd.Flags().Set("config", path) +``` + +--- + +## errname - Error Naming + +**Purpose**: Sentinel errors should follow `errXxx` naming convention. + +### Pattern +```go +// Before +var buildErr error +var NotFoundError = errors.New("not found") +var ErrTimeout error // This is already correct + +// After +var errBuild error +var errNotFound = errors.New("not found") +``` + +### Exported vs Unexported +- **Unexported** (package-internal): `errXxx` +- **Exported** (public API): `ErrXxx` + +```go +// Unexported - use in same package +var errBuild = errors.New("build failed") + +// Exported - part of public API +var ErrNotFound = errors.New("not found") +``` + +--- + +## errorlint - Error Handling + +**Purpose**: Ensure proper error wrapping and type assertions for wrapped errors. + +### Pattern 1: Type Assertion +```go +// Before +if exitErr, ok := err.(*exec.ExitError); ok { + // use exitErr +} + +// After +var exitErr *exec.ExitError +if errors.As(err, &exitErr) { + // use exitErr +} +``` + +### Pattern 2: Error Comparison +```go +// Before +if err == io.EOF { + +// After +if errors.Is(err, io.EOF) { +``` + +### Pattern 3: Switch on Error Type +```go +// Before +switch e := err.(type) { +case *os.PathError: + // handle +} + +// After +var pathErr *os.PathError +if errors.As(err, &pathErr) { + // handle +} +``` + +### Import Required +```go +import "errors" +``` + +--- + +## gosec - Security + +**Purpose**: Identify security vulnerabilities. + +### G115 - Integer Overflow +```go +// Before +value := uint32(signedInt) + +// After +if signedInt < 0 || signedInt > math.MaxUint32 { + return fmt.Errorf("value %d out of uint32 range", signedInt) +} +value := uint32(signedInt) +``` + +### G204 - Command Injection +```go +// Before +cmd := exec.Command(userInput) + +// After - validate input +allowedCommands := map[string]bool{"ls": true, "cat": true} +if !allowedCommands[cmdName] { + return fmt.Errorf("command not allowed: %s", cmdName) +} +cmd := exec.Command(cmdName, args...) + +// Or use absolute paths +cmd := exec.Command("/usr/bin/ls", args...) +``` + +### G301 - Directory Permissions +```go +// Before +os.MkdirAll(dir, 0755) + +// After +os.MkdirAll(dir, 0750) // No world-readable +``` + +### G306 - File Permissions +```go +// Before +os.WriteFile(path, data, 0644) + +// After +os.WriteFile(path, data, 0600) // Owner only +``` + +### G401/G501 - Weak Crypto +```go +// Before +import "crypto/md5" +hash := md5.Sum(data) + +// After +import "crypto/sha256" +hash := sha256.Sum256(data) +``` + +### G104 - Unhandled Errors (see errcheck) + +--- + +## modernize - Modern Go + +**Purpose**: Use modern Go idioms (Go 1.21+/1.22+). + +### rangeint - Range Over Integer (Go 1.22+) +```go +// Before +for i := 0; i < count; i++ { + // use i +} + +// After +for i := range count { + // use i +} + +// If i unused +for range count { + // body +} +``` + +### forvar - Loop Variable Capture (Go 1.22+) +```go +// Before (needed pre-1.22) +for _, item := range items { + item := item // capture + go process(item) +} + +// After (1.22+ captures automatically) +for _, item := range items { + go process(item) +} +``` + +### any - Replace interface{} +```go +// Before +func process(data interface{}) interface{} {} +var m map[string]interface{} + +// After +func process(data any) any {} +var m map[string]any +``` + +### mapsloop - Use maps.Copy (Go 1.21+) +```go +// Before +for k, v := range src { + dst[k] = v +} + +// After +import "maps" +maps.Copy(dst, src) +``` + +### minmax - Use min/max Builtins (Go 1.21+) +```go +// Before +count := opts.Count +if count < 1 { + count = 1 +} + +// After +count := max(opts.Count, 1) +``` + +--- + +## staticcheck - Static Analysis + +### ST1023 - Redundant Type +```go +// Before +var name string = "" +var count int = 0 + +// After +var name = "" +var count = 0 +// Or: name := "" +``` + +### SA4000 - Identical Expressions +```go +// Before (always equals 0) +result := x - x + +// After - fix the logic error +result := x - y // probably meant different variable +``` + +### SA1019 - Deprecated +```go +// Follow deprecation notice to use replacement API +``` + +### SA4006 - Unused Value +```go +// Before +x = computeValue() +x = otherValue() // first assignment unused + +// After +_ = computeValue() // if side effects needed +x = otherValue() +``` + +--- + +## gci - Import Ordering + +**Purpose**: Enforce consistent import grouping and ordering. + +### Standard Order +```go +import ( + // 1. Standard library + "context" + "fmt" + + // 2. Third-party packages + "github.com/spf13/cobra" + "k8s.io/client-go/kubernetes" + + // 3. Local packages + "github.com/opdev/virtwork/internal/config" +) +``` + +### Auto-Fix +gci usually provides `SuggestedFixes` - apply them directly. + +--- + +## gofmt / gofumpt - Formatting + +**Purpose**: Standard Go formatting. + +### Auto-Fix +These always have `SuggestedFixes`. Apply directly or run: +```bash +gofmt -w file.go +gofumpt -w file.go +``` + +--- + +## golines - Line Length + +**Purpose**: Keep lines under configured length (default 120). + +### Pattern 1: Function Signature +```go +// Before +func ProcessItems(ctx context.Context, client Client, items []Item, options Options) (Result, error) { + +// After +func ProcessItems( + ctx context.Context, + client Client, + items []Item, + options Options, +) (Result, error) { +``` + +### Pattern 2: Struct Literal +```go +// Before +return &Config{Name: name, Value: value, Options: options, Enabled: true} + +// After +return &Config{ + Name: name, + Value: value, + Options: options, + Enabled: true, +} +``` + +### Auto-Fix +golines provides `SuggestedFixes`. Apply directly or run: +```bash +golines -w file.go +``` + +--- + +## Quick Reference: Severity Levels + +| Severity | Linters | Priority | +|----------|---------|----------| +| high | gosec (G115, G203, etc.) | Fix immediately | +| medium | gosec (G301, G306) | Fix soon | +| "" (default) | All others | Fix as convenient | + +## When to Use nolint + +Use `//nolint:linter` sparingly when: +1. False positive +2. Intentional deviation with good reason +3. Test-specific code + +Always include reason: +```go +//nolint:gosec // G204: command is hardcoded, not user input +cmd := exec.Command(binaryPath, args...) +``` diff --git a/.gitignore b/.gitignore index 30fde5a..78677ea 100644 --- a/.gitignore +++ b/.gitignore @@ -9,7 +9,9 @@ wheels/ # Virtual environments .venv -.claude +# Claude Code +.claude/* +!.claude/skills/ claude-logs docs/engineering-journals From 442d302da4fa25a260ff5da59e7d49641414f511 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Wed, 18 Mar 2026 17:22:44 -0500 Subject: [PATCH 2/2] fix: set buildx as default builder for attestation support GoReleaser's dockers_v2 uses --attest=type=sbom which requires a buildx driver that supports attestations. The default docker driver does not. Adding use: true ensures the docker-container driver is used instead. Signed-off-by: Melvin Hillsman --- .github/workflows/release.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 291b4f1..f07d961 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -22,6 +22,11 @@ jobs: with: go-version-file: "go.mod" + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + with: + use: true + - name: Login to Quay.io uses: docker/login-action@v4 with: