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.

Feature	Minimum Go
Generics	1.18
Typed atomics (`atomic.Int64`, `atomic.Bool`)	1.19
`slog`, `slices`/`maps` packages, `min`/`max` builtins, `sync.OnceValue`/`OnceFunc`	1.21
Range-over-func, enhanced loop variable semantics	1.22
`iter.Seq`, `unique` package	1.23

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¶

Define scope — files/diff under review. Apply Generated Code Exclusion Gate.
Check Go version from go.mod — gate all modern Go recommendations.
Run static analysis — follow execution protocol above. Record output.
Gather evidence — read changed files, identify quality/style patterns.
Load references — always load go-error-and-quality.md (quality sections); load go-modern-practices.md when modern Go features relevant.
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¶

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

#	Item	Pattern	Grep Pattern
1	Function too long	> 50 lines → extract helper (see anti-example for flat switch exception)	Semantic-Only (function length requires counting lines — no grep pattern)
2	Excessive nesting	> 4 levels → early return pattern ("happy path left-aligned")	Semantic-Only (nesting depth requires counting indent levels)
3	Naked return	In functions > 5 lines → explicit returns for clarity	`return$\\|return\s*$` (naked return — grep for `return` with nothing after)
4	Mutable global variable	Mutable `var` at package level → `const`, getter, or functional options	`^var\s+\w` (package-level var declarations — grep at file scope)
5	Interface bloat	Interface > 3 methods or defined at implementation site → small interfaces at consumer site	Semantic-Only (interface bloat requires counting methods)
6	Type assertion without ok	`x.(T)` without comma-ok → `x, ok := x.(T)` — panics on wrong type	`\.\(\w` (type assertion without comma-ok check)
7	defer in loop	Defer accumulates until function return → extract to helper function	`defer\s+` (compound: inside `for` loop — semantic confirmation)
8	init() misuse	`init()` for non-registration logic → explicit initialization	`func init`
9	Inconsistent receiver type	Mixed pointer/value receivers on same type → consistent choice	Semantic-Only (receiver consistency requires checking all methods on type)
10	Modern Go alternatives	Outdated patterns when modern alternatives exist (version-gated): `log` → `slog`, `atomic.AddInt64` → `atomic.Int64`, `sort.Slice` → `slices.SortFunc`	`log\.\\|atomic\.Add\\|sort\.Slice` (outdated patterns when modern alternatives exist)
11	Generics vs interfaces	Wrong choice — type operations (containers, transforms) → generics; behavior contracts → interfaces	`interface\s*\{\\|interface\{\\|any\b`
12	Naming / package structure (Low)	Stuttering (`user.UserService`), package `utils`/`helpers`/`common`, unexported type in exported return	`\.User\wService\\|\.User\wHandler\\|package\s+utils\\|package\s+helpers\\|package\s+common`
13	Missing context.Context on I/O function	Function performing DB / HTTP / cache / Redis I/O but signature lacks `ctx context.Context` as first parameter — queries cannot be cancelled on client disconnect or upstream timeout. Fix: add `ctx context.Context` first, then `.WithContext(ctx)` (GORM), `http.NewRequestWithContext(ctx, ...)`, `redis.*Cmd(ctx, ...)` etc.	Semantic-Only (absence-of-pattern: requires reading both the signature and the body to identify I/O calls without ctx propagation)

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¶

Reference	Load When
`references/go-error-and-quality.md`	Always (code quality sections)
`references/go-modern-practices.md`	Code uses or could benefit from modern Go features
`references/go-review-anti-examples.md`	Always

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