go-logic-review

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.

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.

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