go-quality-review

Go Quality Review

Purpose

Audit Go code for structural quality, style conformance, and modern Go practices. This skill is the designated lint-tool runner — other vertical review skills do NOT run golangci-lint/staticcheck/go vet, avoiding duplicate execution.

This skill does NOT cover: security, concurrency, performance, error handling, test quality, or business logic — those belong to sibling vertical skills.

When To Use

• Reviewing code structure and readability
• Checking Go naming conventions and package organization
• Evaluating use of modern Go features
• Need to run golangci-lint / staticcheck / go vet

When NOT To Use

• Security vulnerabilities → go-security-review
• Concurrency safety → go-concurrency-review
• Error handling correctness → go-error-review
• Performance optimization → go-performance-review
• Business logic → go-logic-review

Mandatory Gates

1) Go Version Gate

Read go.mod for go directive. Do NOT recommend features above project version.

2) Anti-Example Suppression Gate

MUST quote specific code evidence. Category match alone insufficient.

Embedded anti-examples:

• "Should use generics" — when only one concrete type used throughout codebase. Generics add complexity without benefit for single-type usage.
• "Should use slog" — when project targets Go < 1.21. Version-inappropriate recommendation.
• "Should use typed atomics" — when project targets Go < 1.19.
• "Exported function missing godoc" — when symbol is in internal/ package not intended for external consumers.
• "Function too long (>50 lines)" — when body is a straightforward table-driven switch, sequential pipeline with no nesting, or single select/case block. These are long but simple.
• "interface{} should be any" — pure cosmetic alias rename. Report only as part of broader modernization effort, never as standalone finding.
• "Should extract helper function" — when the code would only be called from one place and extraction reduces readability by splitting context.

3) Static Analysis Execution Protocol

Run lint tools in this priority order:

1. Check for .golangci.yml / .golangci.yaml — respect project config
2. golangci-lint run (config-aware)
3. staticcheck ./... (if golangci-lint unavailable)
4. go vet ./... (minimal fallback) Report tool output in Execution Status. If no tools available, state Not available. Dedup rule: same location + same issue from multiple tools = report once.

4) Generated Code Exclusion Gate

Exclude: *.pb.go, *_gen.go, mock_*.go, *_string.go, *_enumer.go. Note excluded files in Execution Status.

Workflow

1. Define scope — files/diff under review. Apply Generated Code Exclusion Gate.
2. Check Go version from go.mod — gate all modern Go recommendations.
3. Run static analysis — follow execution protocol above. Record output.
4. Gather evidence — read changed files, identify quality/style patterns.
5. Load references — always load go-error-and-quality.md (quality sections); load go-modern-practices.md when modern Go features relevant.
6. Evaluate ALL 13 checklist items → cross-reference with lint output → suppress anti-examples → format.

Grep-Gated Execution Protocol

This skill uses mechanical grep pre-scanning to guarantee zero missed checklist items. 8 of 13 items are grep-gated; 5 are semantic-only.

Execution Order

1. Identify target files (from dispatch prompt, or write raw snippet to $TMPDIR/review_snippet.go)
2. Run grep for all grep-gated checklist items against target files
3. HIT → run semantic analysis to confirm or reject
4. MISS → auto-mark NOT FOUND, skip semantic analysis
5. For semantic-only items (items 1, 2, 5, 9, 13): full model reasoning — these require structural analysis
6. Report only FOUND items

Grep Audit Line

Include in Execution Status: Grep pre-scan: X/8 items hit, Z confirmed as findings (5 semantic-only)

Quality Checklist (12 Items)

All Medium severity unless marked (Low).

Severity Rubric

Medium — Maintainability/readability issue increasing cognitive load or bug risk.

Low — Style preference or minor inconsistency.

Evidence Rules

• For each finding: show current pattern vs idiomatic alternative
• For lint findings: include tool name and rule ID when available
• Merge rule: same issue at ≥3 locations → one finding with location list
• Dedup with lint: if lint flagged it, use lint output as evidence

Output Format

Findings

[Medium|Low] Short Title

• ID: QUAL-NNN
• Location: path:line
• Impact: Maintenance/readability consequence
• Evidence: Current pattern vs idiomatic alternative
• Recommendation: Specific refactoring suggestion
• Action: follow-up

Suppressed Items

[Suppressed] Short Title

• Reason: Anti-example matched + evidence cited

Execution Status

• Go version: X.Y
• Grep pre-scan: X/8 items hit, Z confirmed as findings (5 semantic-only)
• golangci-lint: PASS | FAIL | Not available
• staticcheck: PASS | FAIL | Covered by golangci-lint | Not available
• go vet: PASS | FAIL | Covered by golangci-lint | Not available
• Excluded (generated): list or None
• References loaded: list

Summary

1-2 lines. Count by severity + lint status.

Example Output

### Findings

#### [Medium] Function Exceeds 50 Lines with Deep Nesting
- **ID:** QUAL-001
- **Location:** `internal/service/order.go:45-120`
- **Impact:** 75-line function with 5 nesting levels — high cognitive load, hard to test individual branches
- **Evidence:** `ProcessOrder()` has nested if/for/if/switch/case structure. Unlike a flat switch (anti-example), this has complex branching.
- **Recommendation:** Extract inner switch into `classifyOrderType()` and validation into `validateOrderItems()`
- **Action:** follow-up

#### [Medium] Mutable Package-Level Variable
- **ID:** QUAL-002
- **Location:** `internal/config/defaults.go:8`
- **Impact:** `var DefaultTimeout = 30 * time.Second` — any package can mutate, non-deterministic in tests
- **Evidence:** Written at L8, read from 4 packages. golangci-lint: `gochecknoglobals`
- **Recommendation:** Change to `const` or getter: `func DefaultTimeout() time.Duration { return 30 * time.Second }`
- **Action:** follow-up

### Suppressed Items
#### [Suppressed] Long Function — Table-Driven Switch
- **Reason:** `routeRequest()` at router.go:30 is 60 lines but flat switch on HTTP method, no nesting. Anti-example: "straightforward table-driven switch"

### Execution Status
- Go version: 1.21
- golangci-lint: PASS (2 warnings reported above)
- staticcheck: Covered by golangci-lint config
- go vet: Covered by golangci-lint config
- Excluded (generated): None
- References loaded: go-error-and-quality.md, go-modern-practices.md

### Summary
2 Medium findings (function length, mutable global). Lint clean except reported items.

No-Finding Case

If no issues found: state No code quality findings identified. Still output Execution Status (lint results always reported).

Load References Selectively

Review Discipline

• Code quality, style, modern practices, and lint only — not security, concurrency, errors, performance, tests, or logic
• Designated lint runner — this skill runs golangci-lint; other skills reference its output
• Execute ALL 13 checklist items
• Version-gate all modern Go recommendations — check go.mod before recommending