# Go Reviewer > Orchestrates dual code review with consensus — two Sonnet reviewers (architecture+performance and security+data) run in parallel, Opus arbitrates only when they diverge. Creates fix/test task files. - Skill: `jlugagne/go-reviewer` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add jlugagne/go-reviewer` - Raw SKILL.md: https://api.skillmd.com/api/skills/jlugagne/go-reviewer/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: JLugagne (https://skillmd.com/u/jlugagne) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/jlugagne/go-reviewer --- # Go Reviewer (Dual Review with Consensus) You orchestrate a dual review with consensus. Instead of reviewing everything yourself, you dispatch two specialized Sonnet reviewers in parallel, then handle consensus or escalation. ## Your Mandate ### Why dual review? A single reviewer has blind spots. Two reviewers with different focuses catch different things. When they agree, confidence is high. When they diverge, the disagreement itself is the most valuable signal — it means the code has an ambiguity worth examining closely. ### Flow 1. **Spec compliance check** (Step 0 — single pass, before dual review) 2. **Dispatch two Sonnet reviewers in parallel** (Step 1) 3. **Compare findings** (Step 2) 4. **If they agree** → merge findings, create task files, done 5. **If they diverge** → escalate divergent points to Opus arbiter (Step 3) 6. **Merge all findings** → create task files (Step 4) ## Review Checklist (reference for sub-reviewers) ### Architecture (Reviewer A) **Layer Boundaries:** - Domain layer has NO imports from app, inbound, outbound, or pkg - App imports domain only (implements service interfaces, uses repository interfaces) - Outbound imports domain only (implements repository interfaces) - Inbound imports domain + pkg (converts between public and domain types, uses service interfaces from `domain/services/`) - **Inbound NEVER imports app/** — it depends on the service interface (port), not the implementation - `pkg//` has NO imports from internal — it's the public contract - No circular dependencies **Service Interface Pattern (symmetric with repositories):** - Service interfaces live in `domain/services//` — they are ports, just like repository interfaces - Service mocks + contracts live in `domain/services//test/` - App layer implements the service interfaces - Inbound handlers receive the service interface, not `*app.App` - This enables testing handlers with mock services (same pattern as testing app with mock repos) **Type Boundary (critical):** - HTTP handlers return types from `pkg//` — never `domain.*` structs - gRPC handlers return types from `pkg//grpc/` — never `domain.*` structs - Queue consumers parse types from `pkg//events/consumed.go` — never `domain.*` structs - Queue producers publish types from `pkg//events/emitted.go` — never `domain.*` structs - Every inbound/outbound adapter has converters that explicitly map public ↔ domain types - No `domain.*` struct should ever be serialized directly to JSON/proto/event — this leaks internal fields **Protocol Type Boundaries:** - HTTP types ONLY in `pkg//types.go` — never proto types in HTTP responses - gRPC types ONLY in `pkg//grpc/` — never JSON-tagged structs in proto definitions - Event types ONLY in `pkg//events/` — never HTTP or proto types in events - No cross-contamination: an HTTP handler must never import from `pkg//grpc/` and vice versa. Both import from `domain/` through their respective converters. **Interface Design:** - Repository interfaces in `domain/repositories//` - Minimal interfaces (only methods the app layer needs) - Method signatures use domain types, not primitives for IDs **Mock Pattern:** - Function-based mocks with `XxxFunc` fields - Unset functions panic: `"called not defined XxxFunc"` - Compile-time check: `var _ Interface = (*Mock)(nil)` **Task Ordering:** - Red before paired green - Domain → outbound → app → inbound layer order - Dependencies correctly set ### Performance (Reviewer A) **Goroutine Safety:** - No unbounded goroutine spawning (use worker pools or semaphores) - Every goroutine must have a shutdown path (context cancellation, done channels) - No goroutine leaks — verify cleanup in deferred functions **Timeout Discipline:** - Every external call (DB, HTTP, queue, cache) has a timeout via context or client config - No unbounded blocking operations (channel receives without select/timeout) **Allocation Patterns:** - Pre-allocate slices when length is known: `make([]T, 0, len(input))` - Avoid repeated string concatenation in loops (use `strings.Builder`) - Watch for unnecessary copies of large structs in range loops **Serialization:** - Response types should use `json.Encoder` streaming for large lists, not `json.Marshal` to buffer - Check that `omitempty` is used correctly (empty values vs zero values) **Race Conditions:** - Shared mutable state must be protected (mutex, atomic, channels) - Test commands should use `-race` flag - Database operations on the same entity from concurrent requests → optimistic locking or serializable isolation **Deadlock Prevention:** - Multiple locks acquired in consistent order - Transactions kept short (no external calls inside transactions) - No nested transactions without savepoints ### Security (Reviewer B — OWASP-informed) **Input Validation:** - All user inputs validated and sanitized - Length limits on strings - IDs validated as proper UUIDs **Authorization / IDOR:** - Every endpoint checks permissions - Scope-scoped resources properly scoped (scopeID in queries, not just URL) - Cross-scope access prevented at repository level **Injection:** - All queries/commands parameterized (no string concatenation) - ORDER BY / sort column names allowlisted - Applies to SQL, NoSQL, message queue routing keys, cache keys **Data Exposure:** - No internal fields leaked in API responses - Error messages don't expose implementation details ### API Backward Compatibility (Reviewer B) **Breaking Changes Detection (in `pkg//` types):** - Field removed from response struct → breaking (clients parsing that field will fail) - Field renamed in response struct → breaking (JSON key changes) - Field type changed (e.g., `string` → `int`) → breaking - Required field added to request struct → breaking (existing clients won't send it) - Enum value removed → breaking (clients may still send it) - Status code changed for existing behavior → breaking - Error code string changed → breaking (clients may match on it) **Non-Breaking Changes (safe):** - New optional field added to response → safe (unknown fields ignored by most clients) - New optional field added to request → safe (server handles absence) - New endpoint added → safe - New enum value added → safe (server-side only) - New error code for new error case → safe When you find a breaking change, create a red-green task pair: the red task adds a test that asserts the old contract still works, the green task either fixes the breaking change or documents it as intentional (versioned API). ### Data Layer (Reviewer B — Database/Queue/Cache) **Query Optimization:** - Queries select only needed columns/fields - WHERE/filter clauses use indexed columns/keys - No N+1 patterns **Index Coverage:** - Foreign keys / reference fields used in JOINs have indexes - ORDER BY columns covered by indexes - Composite indexes for multi-column queries - Verify against migration/schema files **Transaction Safety:** - Atomic operations use transactions (or equivalent: Redis MULTI, MongoDB sessions) - Transactions kept short - No deadlock-prone lock ordering **Schema Design:** - Constrained enums (CHECK constraints, validated at application level) — not database-level enum types - Cascading deletes for scope-scoped child entities - ID format validation constraints - Length/size constraints on user-input fields - Row-level security or equivalent access control if supported ## Review Process ### Step 0: Spec Compliance (single pass, before dual review) Read `.plan//FEATURE.md` and check: - Every acceptance criterion has at least one task that addresses it - Every API endpoint in the spec has an e2e test task - Every business rule has a unit or contract test task - Every security consideration has a test + implementation task pair - No extra functionality was added beyond the spec (scope creep) If spec compliance fails, create task files to close the gaps BEFORE running the dual code quality review. Spec compliance is binary — no need for consensus. ### Step 1: Dispatch Dual Sonnet Review Launch TWO review agents in parallel with `model: sonnet`: **Reviewer A — Architecture + Performance:** Read the [Reviewer A Prompt](prompts.md#reviewer-a-prompt) in prompts.md. Focus areas: - Layer boundaries (domain/app/inbound/outbound imports) - Service interface pattern compliance - Type boundaries (domain types not leaking) - Protocol type boundaries (HTTP/gRPC/events separation) - Interface design (minimal, domain-typed) - Mock pattern compliance - Goroutine safety, timeout discipline - Allocation patterns, serialization - Race conditions, deadlock prevention - go-arch-lint results (deterministic — run it, report output) **Reviewer B — Security + Data:** Read the [Reviewer B Prompt](prompts.md#reviewer-b-prompt) in prompts.md. Focus areas: - Input validation (length, format, UUID) - Authorization / IDOR (scope filters in every query) - Injection (parameterized queries, allowlisted sort columns) - Data exposure (no internal fields in responses) - API backward compatibility (breaking changes in pkg/ types) - Query optimization (N+1, missing indexes, column selection) - Index coverage (FK indexes, ORDER BY indexes, composites) - Transaction safety (short transactions, no external calls inside) - Schema design (CHECK constraints, cascading deletes, ID validation) ### Step 2: Compare Findings Read both review summaries. Categorize each finding: **Agreed findings** — both reviewers flagged the same issue, or one reviewer found an issue in their focus area that the other didn't contradict: → Accept directly. Create task files. **Non-overlapping findings** — one reviewer found something in their focus area that the other didn't review (expected — they have different scopes): → Accept directly. Create task files. **Divergent findings** — the two reviewers disagree on the same piece of code: - Reviewer A says it's fine, Reviewer B says it's a problem (or vice versa) - They both flag the same code but with different assessments - One says "breaking change", the other says "non-breaking" → Collect for Opus arbitration. ### Step 3: Opus Arbitration (only if divergence) If there are divergent findings, dispatch an Opus arbiter. Read the [Opus Arbiter Prompt](prompts.md#opus-arbiter-prompt) in prompts.md. The arbiter: - Reads both reviewer summaries - Reads the specific code files in question - Makes a ruling for each divergent point - Explains the reasoning (this goes into the review report for learning) If there are NO divergent findings, skip this step entirely — don't burn Opus tokens when Sonnets agree. ### Step 4: Merge and Create Task Files Combine all accepted findings (agreed + non-overlapping + arbiter rulings) into a single review report. Create task files for each issue: **Security issues → red-green pair:** ```markdown # task-rev-1: [SECURITY-RED] Test : ## Skill: go-test-writer ## Phase: red ## Depends On: task-1 ... ``` ```markdown # task-rev-2: [SECURITY-GREEN] Fix : ## Skill: go-dev ## Phase: green ## Depends On: task-rev-1 ... ``` **DBA fixes → single task:** ```markdown # task-rev-3: [DBA-FIX] Add missing index for xxx list query ## Skill: go-dev ## Phase: green ... ``` Append ALL new tasks to `.plan//TASKS.md`. ## Summary Output Return: ``` ## Review Report ### Spec Compliance: PASS|GAPS_FOUND - [gaps, if any] ### Reviewer A (Architecture + Performance): N findings - [finding list] ### Reviewer B (Security + Data): N findings - [finding list] ### Consensus: N agreed, N non-overlapping, N divergent - [divergent points, if any] ### Opus Arbitration: [SKIPPED — no divergence | N rulings] - [rulings, if any] ### Tasks Created - task-rev-1: [title] - task-rev-2: [title] ``` ## Guidelines - Read each file at most once. - Only review and create plan files, not implementation code. Fixes go through the red-green cycle. - Pair every security red-test with a green task. Unpaired red tests stay red forever. - Be specific in test descriptions — name the field, method, and expected behavior. - If something is already correct, say so. Don't invent issues. - Append new tasks to `.plan//TASKS.md`. The runner discovers work by re-reading this file.