Go Logic Review

Purpose

Audit Go code for business logic correctness. Core question: "Does the code do what it's supposed to do?"

Key distinction from other 6 vertical skills: they use pattern matching (see SQL → check injection, see goroutine → check race). This skill uses semantic understanding — understand the code's intent, then compare with its implementation.

This skill relies primarily on AI's general reasoning ability, not heavy reference files. The checklist provides the review framework; AI provides the reasoning.

This skill does NOT cover: security patterns, concurrency patterns, performance patterns, code style, test quality, or error handling patterns — those belong to sibling skills.

When To Use

• Any code change that modifies behavior
• Code contains conditional branches (if/else, switch)
• Code contains data transformation or processing
• Code contains state management or state transitions
• Default dispatch: always run (any code change can introduce logic errors)

When NOT To Use

• Pure refactoring with no behavior change
• Config-only changes
• Security vulnerability patterns → go-security-review
• Concurrency patterns → go-concurrency-review
• Code style → go-quality-review

Mandatory Gates

1) Context Understanding Gate (unique to this skill)

Before evaluating correctness, understand the INTENT:

• Read function name, comments, docstring
• Read caller context — who calls this function, what do they expect?
• Read related tests — they document expected behavior
• Read commit message / PR description if available

If intent is ambiguous after these steps, flag as "unclear intent — needs clarification" rather than guessing. Do not report uncertain intent as a confirmed defect.

2) Anti-Example Suppression Gate

MUST cite evidence of intent mismatch. Category match alone insufficient.

Embedded anti-examples:

• "Function name doesn't match behavior" — when you cannot verify the expected behavior from available context (don't guess business rules you don't know).
• "Off-by-one in pagination" — when the code follows the framework's pagination convention (0-based vs 1-based varies by framework). Verify convention before flagging.
• "Missing state transition validation" — when the state machine is intentionally permissive by design (e.g., admin override paths).
• "Unused function parameter" — this is a quality/style issue (go-quality-review), not a logic issue. Only flag here if the unused parameter indicates a logic bug (function ignores input it should use).
• "Return value could be nil" — when callers already handle nil (check all callers before flagging).

3) Generated Code Exclusion Gate

Exclude: *.pb.go, *_gen.go, mock_*.go.

Workflow

1. Define scope — files/diff under review. Apply Generated Code Exclusion Gate.
2. Understand intent — read function signatures, comments, callers, tests (Context Understanding Gate). This step is a prerequisite — do not skip.
3. Trace data flow — map inputs through transformations to outputs. For each function: what goes in? What comes out? Does the transformation match the intent?
4. Evaluate ALL 10 checklist items — for each: "does the implementation match the intent?"
5. Classify findings — confirmed (clear evidence of mismatch) vs needs-clarification (ambiguous intent) → format output.

Logic Checklist (10 Items)

All 10 items are semantic-only — no grep patterns are applicable. Logic review relies on AI reasoning to understand code intent vs implementation. This skill does not use the Grep-Gated Execution Protocol.

#	Item	What to Check
1	Happy path correctness	Function's actual behavior matches its name, comments, caller expectations? Example: GetTopN() but no LIMIT applied
2	Boundary conditions	nil input, empty collection, single element, zero value, MaxInt/MinInt. Example: average(items) divides by len(items) without zero check
3	Off-by-one	Loop < vs <=, slice [start:end] (end exclusive), pagination offset/limit. Example: items[0:count] when count can equal len(items)+1
4	Conditional logic	> vs >=, && vs `
5	State consistency	State transitions complete? Illegal paths possible? Modified state persisted? Example: order "pending" → "completed" skipping "processing"
6	Data flow integrity	Input fully consumed? Intermediate results correctly passed? Example: filter returns filtered list but caller uses original unfiltered list
7	Resource lifecycle	Files/connections/transactions closed on ALL paths? Note: overlaps with go-error-review — here focus on logic (missing close as logic gap), there on error handling pattern
8	Return value contract	Return values meet caller's implicit assumptions? Example: caller assumes non-nil slice, function returns nil on empty
9	Idempotency and reentrancy	Operations marked retriable actually idempotent? Example: "retry-safe" endpoint creates duplicate records
10	Timing assumptions	Code assumes "A before B" — always guaranteed? Example: cache populated before first read, but init is async

Severity Rubric

High — Logic error producing incorrect results, data corruption, or silent failure in production.

Medium — Logic concern under specific edge cases or conditions.

Evidence Rules

• For each finding: explain what code DOES vs what it SHOULD do
• Intent evidence: cite function name, comment, caller context, test expectations, PR description
• Ambiguity rule: if intent is truly ambiguous, report as "potential issue — needs clarification" with Action: needs-clarification, NOT as confirmed defect
• Merge rule: same logical issue at ≥3 locations → one finding with location list

Output Format

Findings

[High|Medium] Short Title

• ID: LOGIC-NNN
• Location: path:line
• What it does: Actual behavior of the code
• What it should do: Expected behavior based on intent signals
• Evidence: Why the two differ (off-by-one, missing condition, wrong comparison)
• Recommendation: Specific fix
• Action: must-fix | needs-clarification

Summary

1-2 lines. Count by severity.

Example Output

### Findings

#### [High] GetTopN Returns All Results — Missing LIMIT
- **ID:** LOGIC-001
- **Location:** `internal/repo/product.go:34`
- **What it does:** Queries `SELECT * FROM products ORDER BY sales DESC` — returns ALL products
- **What it should do:** Return top N. Signature: `GetTopN(ctx, n int)`; caller at recommendation.go:12 passes n=10
- **Evidence:** Parameter `n` accepted but never used in query. ORDER BY suggests top-N intent but no LIMIT clause.
- **Recommendation:** Add `LIMIT $1`: `SELECT * FROM products ORDER BY sales DESC LIMIT $1`
- **Action:** must-fix

#### [High] Division by Zero on Empty Input
- **ID:** LOGIC-002
- **Location:** `internal/stats/aggregate.go:22`
- **What it does:** `total / len(items)` — panics when items empty
- **What it should do:** Return 0 or error. Comment: "returns average of items"
- **Evidence:** No length check at L22. Caller at report.go:45 passes user-filtered list that can be empty.
- **Recommendation:** Add guard: `if len(items) == 0 { return 0, nil }`
- **Action:** must-fix

#### [Medium] State Transition May Skip Validation
- **ID:** LOGIC-003
- **Location:** `internal/order/state.go:56`
- **What it does:** Allows "pending" → "shipped" directly
- **What it should do:** Unclear — no state machine doc. Tests only cover happy path (pending → confirmed → shipped).
- **Evidence:** `validTransitions` map includes `"pending": {"confirmed", "shipped", "cancelled"}` — "shipped" without "confirmed" may be intentional (express?) or bug
- **Recommendation:** Clarify with team: is pending → shipped valid? If not, remove from map.
- **Action:** needs-clarification

### Summary
2 High (missing LIMIT, division by zero), 1 Medium needs clarification (state transition).

No-Finding Case

If no issues found: state No logic findings identified. Note the intent sources consulted (callers, tests, comments).

Load References Selectively

This skill relies primarily on AI reasoning, not heavy reference files.

Reference	Load When
references/go-review-anti-examples.md	Always (for suppression discipline)

Review Discipline

• Logic correctness only — not security patterns, concurrency patterns, performance, style, tests, or error handling patterns
• Understand intent BEFORE evaluating — read callers, tests, comments first
• For each function: "If I were the caller, would I get what I expect?"
• Execute ALL 10 checklist items
• When in doubt about intent: flag for clarification, don't guess