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¶
- Define scope — files/diff under review. Apply Generated Code Exclusion Gate.
- Understand intent — read function signatures, comments, callers, tests (Context Understanding Gate). This step is a prerequisite — do not skip.
- Trace data flow — map inputs through transformations to outputs. For each function: what goes in? What comes out? Does the transformation match the intent?
- Evaluate ALL 10 checklist items — for each: "does the implementation match the intent?"
- 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 ||, negation correctness. Example: if !isAdmin || !isOwner should be && (De Morgan's) |
| 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