REVIEW Stage

The REVIEW stage performs code review and quality checks.

Purpose

The Reviewer agent:

1. Runs static analysis (Ruff + mypy)
2. Reviews the code diff
3. Checks against review checklist
4. Auto-fixes mechanical issues
5. Approves or requests changes

Process

┌─────────────────────────────────────────────────────────────┐
│                      REVIEW STAGE                            │
├─────────────────────────────────────────────────────────────┤
│                                                              │
│   1. Static Analysis                                         │
│      ├─ Run Ruff (linting)                                  │
│      ├─ Run mypy (type checking)                            │
│      └─ Format results for review                           │
│                                                              │
│   2. Get Diff                                                │
│      ├─ git diff base...HEAD                                │
│      └─ Compare against base commit                          │
│                                                              │
│   3. Pass 1: CRITICAL Issues                                 │
│      ├─ SQL injection                                        │
│      ├─ Race conditions                                      │
│      ├─ LLM trust boundaries                                 │
│      ├─ Enum completeness                                    │
│      ├─ Silent failures                                      │
│      └─ Data loss risks                                      │
│                                                              │
│   4. Auto-Fix Mechanical Issues                              │
│      ├─ Parameterized queries                                │
│      ├─ exist_ok=True                                       │
│      └─ Commit fixes                                         │
│                                                              │
│   5. Pass 2: INFORMATIONAL Issues                            │
│      ├─ Side effects                                         │
│      ├─ Magic numbers                                        │
│      ├─ Dead code                                            │
│      ├─ Test coverage                                        │
│      └─ Observability                                        │
│                                                              │
│   6. Decision                                                │
│      ├─ APPROVED → review-approved                          │
│      └─ CHANGES → review-changes-requested                  │
│                                                              │
└─────────────────────────────────────────────────────────────┘

Static Analysis

Before the Reviewer starts, Goblin runs:

Ruff (Linting)

ruff check --output-format json .

Catches:

• Unused imports
• Undefined variables
• Style violations
• Common errors

mypy (Type Checking)

mypy --json-output .

Catches:

• Type mismatches
• Missing type annotations
• Incompatible assignments

Results Injection

Static analysis results are injected into the Reviewer's prompt:

## Static Analysis Results

**Summary**: 2 error(s), 3 warning(s)

### `app/models.py`
- Line 10: [F401] Unused import 'os'
- Line 25: [E501] Line too long (120 > 100)

---

**Your focus areas** (tools cannot check these):
- Logic correctness and edge cases
- Breaking changes to existing functionality
- Security implications

Review Checklist

The Reviewer follows a comprehensive checklist:

CRITICAL Issues (Block Merge)

Category	What to Check
SQL Injection	String interpolation, unparameterized queries
Race Conditions	Check-then-act, TOCTOU, missing transactions
LLM Trust	LLM output in shell/SQL, missing validation
Enum Completeness	All values handled in switches
Silent Failures	Bare except, swallowed errors
Data Loss	Destructive ops without confirmation

INFORMATIONAL Issues (Improve Quality)

Category	What to Check
Side Effects	Hidden mutations, impure functions
Magic Numbers	Hard-coded values, repeated literals
Dead Code	Unreachable code, unused imports
Test Gaps	Missing tests for new paths
Performance	N+1 queries, inefficient algorithms
Observability	Missing logging, insufficient context

Auto-Fix Examples

SQL Injection

# BEFORE
cursor.execute(f"DELETE FROM users WHERE id={user_id}")

# AUTO-FIX
cursor.execute("DELETE FROM users WHERE id=?", (user_id,))

Race Condition

# BEFORE
if not os.path.exists(path):
    os.makedirs(path)

# AUTO-FIX
os.makedirs(path, exist_ok=True)

Review Output

## Pre-Landing Review: ENG-123

### Summary
2 issues found (1 critical, 1 informational)

### Status: CHANGES REQUESTED

---

## CRITICAL Issues (Pass 1) - MUST FIX

### 1. [SQL INJECTION] goblin/storage/db.py:45
**Problem:** String interpolation in DELETE query
**Risk:** Attacker can delete arbitrary data
**Fix Applied:** ✅ Converted to parameterized query

---

## INFORMATIONAL Issues (Pass 2)

### 1. [TEST GAP] goblin/core/pipeline.py:156
**Missing:** Test for timeout error path
**Suggestion:** Add test_pipeline_handles_timeout()

Signal Files

Outcome	Signal File
Approved	.goblin/review-approved
Changes Requested	.goblin/review-changes-requested

review-changes-requested Content

{
  "issues": [
    {
      "type": "critical",
      "category": "sql_injection",
      "file": "db.py",
      "line": 45,
      "fixed": true
    }
  ],
  "summary": "Fixed SQL injection, need test coverage"
}

Configuration

# Enable/disable static analysis
goblin pipeline config -p PROJECT --config static_analysis_enabled=true

# Enable mypy
goblin pipeline config -p PROJECT --config static_analysis_mypy=true

Next Steps

• TEST Stage - Next stage
• Review Checklist - Full checklist
• Static Analysis - Analysis details