Go Code Reviewer

Go Code Reviewer

Purpose

Use this skill to review Go code for real defects and risk, not just style. The review must be evidence-based, policy-aligned, and actionable.

Quick Reference

When To Use

Trigger this skill when the user asks for:

• Go code review / PR review / diff review
• Code quality or best-practice checks
• Risk or regression analysis
• "Is this code compliant with project rules?"

Review Policy Sources (in order)

1. constitution.md (highest repository policy for this skill)
2. AGENTS.md (repo workflow/testing/style constraints)
3. Local package conventions (tests, interfaces, dependency patterns)
4. Go language/runtime best practices If constitution.md is missing, explicitly state that and continue with AGENTS.md + Go best practices.

Execution Modes (Lite / Standard / Strict)

Choose a mode before starting review and state it in the report.

• Default mode: Standard
• Declare the selected mode in a dedicated Review Mode section. Mode selection rules:
• Choose Lite only when scope is small (typically <=3 files), low-risk, and no security/auth/concurrency/public API changes are involved.
• Choose Strict when any high-risk signal exists: security/auth, concurrency/lifecycle, HTTP/API contract changes, persistence/schema changes, exported signature changes, or broad refactors (typically >15 files).
• Use Standard for everything else.

Lite (fast triage)

• Review focus: confirmed defects with high confidence, avoid speculative architecture commentary.
• Minimum execution:
  • Run at least one static tool (golangci-lint preferred, else staticcheck/go vet).
  • Run go test for impacted package(s).
  • Run go test -race only if concurrency risk is present; if skipped, state reason.
• Baseline/Suppression/SLA gates still apply.
• Finding volume: soft target ≤ 5 findings (severity-tiered; see Workflow step 10).

Standard (default balanced review)

• Full workflow in this skill applies as written.
• Expected execution when feasible:
  • golangci-lint run (config-aware fallback strategy),
  • impacted-package tests,
  • go test -race when concurrency/shared state risk exists.
• Finding volume: soft target ≤ 10 findings (severity-tiered; see Workflow step 10).

Strict (release/security gate)

• Perform deep impact-radius expansion and compatibility checks.
• Minimum execution when feasible:
  • golangci-lint run + direct staticcheck/go vet when not explicitly covered by config.
  • go test ./...
  • go test -race ./...
• For unresolved findings, provide explicit SLA and, if deferred, complete risk acceptance entry.
• Finding volume: soft target ≤ 15 findings (severity-tiered; see Workflow step 10).

Mandatory Review Gates

1) Execution Integrity Gate

Never claim verification was executed unless it actually ran.

• If go test or go test -race is not run, you must output:
  • Not run in this environment
  • reason
  • exact commands to run
• Do not imply pass/fail for commands you did not execute.

2) Baseline Comparison Gate

When prior review context exists (previous PR review comments, prior findings, or known issue list), classify each finding as:

• new
• regressed
• unchanged
• resolved If no baseline is available, state: Baseline not found.

3) False-Positive Suppression Gate

Before reporting a finding, check whether the risk is already blocked by:

• upstream guard/middleware/policy
• non-user-controlled input path
• framework/runtime safe guarantees If blocked, do not report as a finding. Put it in a short Suppressed items section with rationale.

4) Risk Acceptance and SLA Gate

For unresolved findings, include:

• recommended SLA by severity
• optional risk acceptance entry when immediate fix is not chosen Default SLA guidance:
• High: fix or strong mitigation in <= 3 business days
• Medium: <= 14 calendar days
• Low: next planned iteration Risk acceptance entry fields:
• finding ID
• owner
• justification
• compensating controls
• expiry/review date

5) Go Version Gate

Before recommending version-specific features, check the project's minimum Go version:

• Read go.mod for the go directive (e.g., go 1.21).
• Do NOT recommend features unavailable at the project's Go version as findings.
• If go.mod is not found or not readable, state Go version: unknown and annotate version-specific recommendations with their minimum required version. Version-gated features (non-exhaustive):

6) Generated Code Exclusion Gate

Exclude auto-generated files from review findings:

• Files matching: *.pb.go, *_gen.go, wire_gen.go, *_string.go, *_enumer.go
• Files matching generated mock patterns: mock_*.go (from mockgen), *_mock.go
• Files containing the standard header: // Code generated .* DO NOT EDIT Rules:
• If a generated file is in the diff, note it in Execution Status as Excluded (generated) with the file name.
• Review the generator configuration or template only if the user specifically requests it.

7) Reference Loading Gate

When code under review matches trigger patterns (see Appendix), the corresponding reference file MUST be loaded before evaluating that category.

• If a trigger pattern fires but the reference was not loaded, pause evaluation and load it.
• This is mandatory, not advisory. Reviewing concurrency patterns without loading go-concurrency-patterns.md will miss nuanced patterns and produce lower-quality findings.
• Record which references were loaded in the Execution Status section.

8) Change Origin Classification Gate

When reviewing a PR or diff, classify each finding's origin relative to the current change:

• introduced: The defective code was added or modified in this PR/diff. The author owns this finding.
• pre-existing: The defect exists in code that was NOT changed in this PR/diff. This is historical technical debt, not the author's fault.
• uncertain: Origin cannot be determined (e.g., full-file review without diff context, or ambiguous refactoring). Classification method:
• Check whether the finding's location falls within diff hunks (added or modified lines). If yes → introduced.
• If the finding is on an unchanged line within a diff file, or in a file not in the diff (reached via impact-radius expansion) → pre-existing.
• If no diff context is available (e.g., reviewing a file or package directly without a PR) → uncertain. Actionability by origin:

Workflow

0. Select review mode (Lite|Standard|Strict) and record mode selection rationale.

   • Check go.mod for the project Go version. Record as Go version: X.Y.
   • If go.mod is not accessible, record Go version: unknown.

1. Define scope.

• Confirm files, package, or diff under review.
• If scope is unclear, state assumptions explicitly.
• Apply Generated Code Exclusion Gate: identify and exclude generated files from findings scope. List excluded files in Execution Status.

2. Diff analysis (when reviewing PR/diff).

• Extract changed file list and diff hunks.
• Identify impact radius:
  • Interface changes → search all implementors and callers
  • Exported function signature changes → search cross-package callers
  • Struct field changes → search all construction and usage sites
• Add impact-radius files to review scope (even if not in the diff).
• Diff-boundary rule: For impact-radius files NOT in the diff, review ONLY the specific functions and code paths affected by the change. Do not audit the entire file for unrelated pre-existing issues. Pre-existing defects that are High severity (security, data integrity, crash) SHOULD be reported as findings with Origin: pre-existing and Action: follow-up issue. Medium-severity pre-existing defects in unchanged code MUST be listed in Residual Risk / Testing Gaps with a one-line summary so they are not silently lost. Low-severity pre-existing issues may be omitted.
• For refactoring changes (large-scale moves/renames), focus on behavior preservation verification rather than line-by-line review.

3. Gather evidence.

• Read changed files and relevant tests first.
• Prefer reviewing real diff context when available.
• Apply Reference Loading Gate: load reference files matching trigger patterns before evaluating.

4. Run static analysis (when feasible).

• Discover project lint config: check .golangci.yml / .golangci.yaml first; respect project settings.
• Tool priority with fallback: golangci-lint run → staticcheck ./... → go vet ./...
  • If golangci-lint is available, inspect .golangci* config for enabled linters.
  • Only mark go vet / staticcheck as covered when they are explicitly enabled by config.
  • If coverage is unclear (or linter disabled), run the missing tool directly.
  • If golangci-lint is not available, try staticcheck, then fall back to go vet.
• Include tool output in Execution Status section.
• If no tools are installed, state Not available and continue.
• Dedup rule: same location + same issue = report once. Prefer tool output as Evidence when available.
• Mode-specific tool minimum: see §Execution Modes.

5. Run targeted verification when feasible.

• go test for impacted package(s)
• go test -race when concurrency/shared state risk exists
• If execution is not feasible, apply Execution Integrity Gate.
• Mode-specific test minimum: see §Execution Modes.

6. Evaluate defect-first. Use the checklist below. Skip categories that are not applicable to the change under review. For detailed patterns and code examples, see linked reference files. Security (High) → references/go-security-patterns.md

• SQL/command/path injection in database/sql, os/exec, filepath
• Hardcoded secrets, insecure TLS (InsecureSkipVerify), weak crypto
• unsafe package usage without justification
• Sensitive data leaked in logs or error messages
• AuthN/AuthZ: JWT validation (algorithm pinning, claims verification), IDOR (ownership checks on resource access), middleware ordering (auth before handler), session management
• SSRF: user-controlled URL fetch without host allowlist or private-IP blocking
• XSS: text/template or fmt.Fprintf used for HTML output, template.HTML() on user input
• Rate limiting: missing on authentication/sensitive endpoints
• CORS: reflected Origin, wildcard with credentials
• HTTP security headers: missing X-Content-Type-Options, X-Frame-Options, Strict-Transport-Security, Content-Security-Policy
• Timing attacks: == comparison on secrets/tokens instead of crypto/subtle.ConstantTimeCompare
• Input validation: unbounded request body (missing http.MaxBytesReader), unchecked integer inputs used for allocation Error Handling (High) → references/go-error-and-quality.md
• Ignored errors (_ discard on error-returning calls)
• Missing error wrapping context (%w) — inspect every return err path
• panic used for recoverable errors
• Direct equality instead of errors.Is / errors.As Concurrency & Lifecycle (High) → references/go-concurrency-patterns.md
• Goroutine leak (no cancellation path)
• Race conditions on shared state (maps, slices, vars)
• Mutex misuse (missing defer Unlock, lock copying)
• Unbuffered channel deadlock
• Missing errgroup for coordinated goroutine error handling
• context.Context not propagated; context.Value abuse
• sync.Pool / sync.Once misuse
• Goroutine missing defer recover() — unrecovered panic crashes entire process
• Goroutines created per loop element without concurrency limit (semaphore / errgroup.SetLimit / worker pool) Test Quality (High) → references/go-test-quality.md
• Tests cover changed behavior and failure modes
• Table-driven test pattern with meaningful subtest names
• t.Helper() on test helpers
• Assertion completeness (not just "no error")
• Boundary/edge cases (nil, 0, 1, empty, max)
• Mock/stub scope minimized API & HTTP (High, when applicable) → references/go-api-http-checklist.md
• For server handlers, avoid requiring explicit r.Body.Close(); focus on bounded reads and error handling.
• For outbound HTTP clients, require resp.Body.Close() on all paths.
• HTTP status codes semantically correct
• Response Content-Type set explicitly
• Middleware ordering (auth before handler)
• http.Server graceful shutdown
• API backward compatibility preserved
• gin/echo/chi: middleware registration order, route conflict detection, c.Request.Context() propagation
• gRPC: interceptor chain order, metadata propagation, deadline propagation, stream lifecycle
• wire/fx: dependency cycle detection, provider signature consistency Database & Persistence (High, when applicable) → references/go-database-patterns.md
• sql.Rows not closed (resource leak)
• Transaction rollback pattern (defer tx.Rollback() + commit override)
• Connection pool misconfiguration (missing SetMaxOpenConns, SetConnMaxLifetime)
• N+1 queries (loop with individual queries instead of batch)
• Missing context propagation (db.Query instead of db.QueryContext)
• sql.ErrNoRows mishandled as server error
• Null handling without sql.Null* types Code Quality (Medium) → references/go-error-and-quality.md
• Function >50 lines, nesting >4 levels
• Naked returns in long functions
• Mutable package-level variables
• Interface pollution (unused abstractions)
• Early return pattern (no else after error return)
• Nil interface trap (typed nil returned as interface)
• Inconsistent pointer vs value receivers
• Shadowed error variables
• Pointer slice []*T elements not nil-guarded before field access or method call Performance (Medium) → references/go-performance-patterns.md
• String concatenation in loops (use strings.Builder)
• Slice/map without pre-allocation when size known
• sync.Pool misuse or missed opportunity for hot-path allocations
• Unnecessary allocations in hot paths
• Struct field alignment (use fieldalignment tool)
• Substring memory retention (use strings.Clone)
• Lock scope too wide; mutex where atomic suffices
• Missing buffered I/O (bufio) for frequent small writes/reads
• http.DefaultClient without timeout or transport tuning
• regexp.Compile in hot path instead of package-level compile-once Modern Go & Best Practices (Medium) → references/go-modern-practices.md
• Generics vs interface choice appropriateness
• any overuse where type constraint is possible
• slog for structured logging (Go 1.21+)
• Typed atomic operations (atomic.Int64 etc., Go 1.19+)
• context.WithCancelCause / context.WithoutCancel (Go 1.20+/1.21+)
• Goroutine lifecycle: recover panics, leak prevention, concurrency control
• Channel buffer selection semantics
• Error message format (lowercase, no punctuation)
• context.Background() vs context.TODO() usage
• Package naming (short, lowercase, no underscore)
• Godoc on exported symbols Dependency & Module (Low)
• go.sum synchronized
• Stale replace directives
• Unnecessary dependencies
• Deprecated standard library functions

7. Apply False-Positive Suppression Gate.

8. Apply Baseline Comparison Gate.

9. Apply Change Origin Classification Gate.

• For each finding, determine whether the defective code was introduced in this PR/diff or existed before.
• Use diff hunks as the primary signal: finding location inside a changed hunk → introduced; outside → pre-existing.
• When in doubt, use git blame or file history to confirm.
• Attach Origin and Action to each finding.

10. Consolidate, prioritize, and report findings.

• Merge rule: When the same conceptual issue (e.g., "missing error wrapping") appears at ≥ 3 locations, report ONE finding with a location list, not N separate findings. Merged findings share the same Origin only if all locations have the same origin; otherwise list origins per location.
• Volume cap — severity-tiered strategy:
  • Soft targets by mode: Lite ≤ 5, Standard ≤ 10, Strict ≤ 15.
  • Phase 1 — High: Report ALL High-severity findings regardless of the soft target. High findings are never dropped by volume cap.
  • Phase 2 — Medium: Fill remaining slots (soft target minus High count) with Medium-severity findings, prioritizing introduced over pre-existing.
  • Phase 3 — Low: If slots remain, include Low-severity findings.
  • Overflow: If total candidates exceed the soft target after Phase 1, move the lowest-severity candidates that did not make the cut to Residual Risk / Testing Gaps with a one-line summary each, and note N additional lower-priority issues moved to Residual Risk in Summary.
  • Example: Standard mode, 4 High + 8 Medium found → report 4 High + 6 Medium as findings, move 2 Medium to Residual Risk.
• Sort order: introduced before pre-existing within the same severity level. High → Medium → Low.
• Every finding must include policy mapping, file references, Origin, and Action.
• Apply Go Version Gate: remove or downgrade findings that recommend features above the project's Go version.

11. Add risk acceptance/SLA recommendations.

Severity Rubric

• High: functional breakage, data loss/corruption, security risk, race/deadlock, production instability
• Medium: maintainability or reliability risk without immediate failure
• Low: clarity, readability, or non-critical consistency gap

Evidence Rules

• Do not report speculative findings as confirmed defects.
• Every finding must include:
  • exact location (path:line)
  • concrete impact
  • why current behavior is risky/incorrect
  • actionable fix direction
• Clearly label inference vs directly observed behavior.

Anti-examples (DO NOT report these)

See references/go-review-anti-examples.md (always loaded — see Appendix). Before suppressing a finding using an anti-example, you MUST quote specific code evidence satisfying the anti-example's stated precondition. Category match alone is not sufficient — if you cannot cite evidence, the finding must be reported.

Output Format (Required)

Review Mode

• Lite|Standard|Strict
• mode selection rationale (1-2 lines)

Findings

List findings first, ordered by severity.

[High|Medium|Low] Short Title

• ID: REV-001
• Origin: introduced|pre-existing|uncertain
• Baseline: new|regressed|unchanged (or N/A if baseline missing)
• Principle: constitution.md clause (or N/A with reason)
• Location: path:line (or location list for merged findings)
• Impact: user/business/runtime impact
• Evidence: concrete observed behavior
• Recommendation: specific and minimal fix direction
• Action: must-fix | follow-up issue (aligned with Origin actionability table)

Suppressed Items

Only include items filtered by the suppression gate.

[Suppressed] Short Title

• Reason: upstream guard / non-user-controlled input / framework safety
• Location: path:line
• Residual risk: short note

Execution Status

• Go version: X.Y (from go.mod) or unknown
• Excluded (generated): list of generated files excluded, or None
• References loaded: list of reference files loaded for this review
• go vet: PASS|FAIL|Not available|Covered by golangci-lint config
• staticcheck: PASS|FAIL|Not available|Covered by golangci-lint config
• golangci-lint: PASS|FAIL|Not available
• go test: PASS|FAIL|Not run in this environment
• go test -race: PASS|FAIL|Not run in this environment
• If not run, include reason and exact commands.

Risk Acceptance / SLA

• SLA recommendation by severity in this review
• Optional risk acceptance entries for deferred fixes:
  • finding ID
  • owner
  • compensating control
  • expiry/review date

Open Questions

Only include blockers that materially affect confidence.

Residual Risk / Testing Gaps

This section captures items that are valuable context but do not belong in Findings:

1. Verification gaps: tools or tests that were not run, and the reason.
2. Volume-cap overflow: findings that were evaluated and confirmed but displaced by the severity-tiered volume cap (see Workflow step 10). List each with a one-line summary (severity | origin | location | short description) so no validated issue is silently dropped.
3. Pre-existing issues (non-High): Low/Medium-severity pre-existing defects found in impact-radius files (not in the diff) go here with a one-line summary each. High-severity pre-existing issues may be promoted to Findings with Origin: pre-existing.
4. Areas not covered: parts of the change whose risk could not be assessed (e.g., missing test fixtures, untraceable dynamic dispatch).

Summary

1-3 lines only, after findings. Include origin breakdown: X introduced / Y pre-existing / Z uncertain. If findings were capped by volume limit, note: N additional lower-priority issues moved to Residual Risk.

Example Output Reference

When you need to verify report formatting or check what a complete review looks like: → Load references/example-output.md for a full example covering Review Mode, Findings with all required fields (ID, Origin, Baseline, Principle, Location, Impact, Evidence, Recommendation, Action), and Summary.

No-Finding Case

If no issues are found:

• Explicitly say: No actionable findings found.
• Still provide:
  • Review Mode
  • Execution Status
  • baseline status (resolved list if available, otherwise Baseline not found)
  • residual risks/testing gaps

Review Discipline

• Prefer small, precise, defensible findings over generic advice.
• Findings are the primary deliverable.
• Recommend tests whenever behavior changes are proposed.
• Execute ALL checklist categories regardless of how many High findings have already been identified; the presence of multiple High findings does not reduce the obligation to complete remaining categories; finish the full checklist scan before consolidating findings.

Skill Maintenance

Run regression checks for this skill with:

bash "<path-to-skill>/scripts/run_regression.sh"

Appendix: Reference Loading Triggers

This is a mandatory gate (see §7 Reference Loading Gate). Use the table below as a compact trigger map; detailed patterns live in the reference files themselves.