gstack-review: Multi-Perspective Code Review

Review Framework

When asked to review code, follow this three-perspective framework. Run all steps systematically.

Step 0: Detect What to Review

Priority order for review scope:

1. Uncommitted changes (git diff HEAD) — if working directory is dirty
2. Specific files mentioned by user
3. Branch diff vs main — if on a feature branch
4. Recent commits — if no clear scope

# Check working tree status
git status --short

# Get uncommitted changes
git diff HEAD

# Get committed changes on current branch vs origin/main
BRANCH=$(git branch --show-current 2>/dev/null || git rev-parse --abbrev-ref HEAD 2>/dev/null)
MAIN_BRANCH=$(git main-branch 2>/dev/null || echo "main")
git log ${MAIN_BRANCH}..${BRANCH} --oneline 2>/dev/null | head -20
git diff ${MAIN_BRANCH}..${BRANCH} 2>/dev/null | head -500

# List changed files
git diff --name-only ${MAIN_BRANCH}..${BRANCH} 2>/dev/null

Step 1: Gather Context

Before reviewing, collect:

# Project type and language
ls *.json *.toml *.yaml *.gradle *.xml Makefile package.json 2>/dev/null | head -5
cat package.json 2>/dev/null | grep '"name"\|"scripts"' | head -5

# Run tests (silently, capture exit code)
TEST_OUTPUT=$(npm test 2>&1 || pytest 2>&1 || cargo test 2>&1 || true)
TEST_EXIT=$?

# Type check / lint
LINT_OUTPUT=$(npm run lint 2>&1 || npx tsc --noEmit 2>&1 || true)

# Build check
BUILD_OUTPUT=$(npm run build 2>&1 || cargo build 2>&1 || true)
BUILD_EXIT=$?

Step 2: Read Changed Files

For each changed file, read the full content to understand context. Don't just look at the diff — read the surrounding code.

For each file in the diff:
  1. Read the full file (not just changed lines — context matters)
  2. Identify what the code actually does vs. what the diff claims
  3. Note any files that are only binary/generated (skip detailed review)

Step 3: CEO / Product Perspective

Ask: Does this code serve the business and users?

Review from a product-thinking perspective:

• Correctness: Does this actually solve the stated problem?
• Complexity: Is this 10x simpler than the previous approach? Or did we add an abstraction layer that solves nothing?
• Product signal: Is this a feature users asked for, or engineer-invented complexity?
• Scope creep: Did the diff grow beyond its original purpose?
• Tech vs business: Is this engineering for its own sake, or does it genuinely ship value?

Key question: "If I had to explain this change to the CEO in 30 seconds, would they be excited or confused?"

Step 4: Engineering Perspective

Ask: Is this code sound, maintainable, and safe?

• Architecture: Does this fit the existing patterns? Or did we invent a new framework within a framework?
• Error handling: Are all failure modes handled? (network, disk, invalid input, timeouts)
• Resource management: Connections closed? Memory leaked? Background tasks cancelled?
• Dependencies: Did we add a new heavy dependency when a stdlib call would suffice?
• Security: SQL injection, XSS, auth bypass, secrets in code, overly permissive CORS?
• Performance: N+1 queries? Unindexed queries in loops? Memory-allocating operations in hot paths?
• API design: Are interfaces clean and composable, or did we leak internal state?

Code quality signals:

• Functions under 30 lines? Exceptions are allowed but need strong justification.
• Meaningful variable/function names? No temp2_final_v3 patterns.
• Comments that explain why, not what? (the code shows what)
• Tests that test behavior, not implementation?

Step 5: QA / Testing Perspective

Ask: Would this pass a senior engineer's gut check for correctness?

• Test coverage: Are the changed paths actually tested? Not just "tested" but validated?
• Edge cases: Empty input, max length, null bytes, unicode edge cases, concurrent access
• Happy path: Does the main user flow actually work end-to-end?
• Failure modes: What breaks when this code is wrong? Can a user detect it?
• Smoke tests: Does it at least import/parse/load without crashing?
• Test quality: Are tests asserting on behavior or mocking everything and asserting on internals?

Step 6: Assemble the Review

Present a structured review with three clearly labeled sections.

Review Output Template

# Code Review: [BRANCH_NAME] — [DATE]
═══════════════════════════════════════════════

## Summary
[One paragraph: what changed and why. If this were a commit message, is it a good one?]

**Files changed:** N files | **Lines:** +N -N
**Tests:** [PASS/FAIL/NONE] | **Build:** [PASS/FAIL/NONE] | **Lint:** [PASS/FAIL/NONE]

---

## 🏛️ CEO / Product Review
[Bulleted findings. Flag concerns in 🔴, praise good decisions in ✅]

**Verdict:** [Clear statement — ship it, rework it, or discuss with the team]

---

## ⚙️ Engineering Review
[Bulleted findings. Group by category: Architecture, Security, Performance, Code Quality]

**Verdict:** [Clear statement]

---

## 🧪 QA / Testing Review
[Bulleted findings. Group by: Coverage, Edge Cases, Correctness]

**Verdict:** [Clear statement]

---

## Action Items
- [ ] [Priority] [Specific actionable item — who should fix it and how]
- [ ] [Priority] ...

**Overall:** ✅ APPROVED TO SHIP / ⚠️ REVISIONS NEEDED / ❌ BLOCKED
═══════════════════════════════════════════════

Review Principles

1. Be direct. Don't hedge. "This is wrong" is more useful than "this might be worth considering."
2. Distinguish severity. A missing test on a utility function ≠ a SQL injection vulnerability.
3. Context matters. Code that looks wrong in isolation might be the right solution for the system.
4. ** Praise good work.** If the code is clean, simple, and well-tested, say so. Reinforce the pattern.
5. Actionable over academic. "Consider using a WeakMap" is less useful than "Replace new Map() with new WeakMap() on line 42 to avoid memory leaks in the closure."
6. No bikeshedding. Don't flag style preferences that a linter wouldn't flag. Focus on what matters.

When to Escalate (Don't Review Alone)

Escalate to human review for:

• 🔴 Security vulnerabilities (auth bypass, injection, data exposure)
• 🔴 Breaking changes to external APIs or database schemas
• 🔴 Complex concurrency or distributed systems changes without expert review
• 🔴 Changes to payment, billing, or access control logic
• 🔴 Anything that would require a rollback plan

Inspired by Garry Tan's gstack (github.com/garrytan/gstack) — ported for OpenClaw.