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/<context>/ 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/<entity>/ — they are ports, just like repository interfaces
• Service mocks + contracts live in domain/services/<entity>/<entity>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/<context>/ — never domain.* structs
• gRPC handlers return types from pkg/<context>/grpc/ — never domain.* structs
• Queue consumers parse types from pkg/<context>/events/consumed.go — never domain.* structs
• Queue producers publish types from pkg/<context>/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/<context>/types.go — never proto types in HTTP responses
• gRPC types ONLY in pkg/<context>/grpc/ — never JSON-tagged structs in proto definitions
• Event types ONLY in pkg/<context>/events/ — never HTTP or proto types in events
• No cross-contamination: an HTTP handler must never import from pkg/<context>/grpc/ and vice versa. Both import from domain/ through their respective converters.

Interface Design:

• Repository interfaces in domain/repositories/<entity>/
• 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/<context>/ 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-slug>/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 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 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 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:

# task-rev-1: [SECURITY-RED] Test <Entity>: <vulnerability class>
## Skill: go-test-writer
## Phase: red
## Depends On: task-1
...

# task-rev-2: [SECURITY-GREEN] Fix <Entity>: <vulnerability class>
## Skill: go-dev
## Phase: green
## Depends On: task-rev-1
...

DBA fixes → single task:

# task-rev-3: [DBA-FIX] Add missing index for xxx list query
## Skill: go-dev
## Phase: green
...

Append ALL new tasks to .plan/<feature-slug>/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/<feature-slug>/TASKS.md. The runner discovers work by re-reading this file.