final-template

Final Review Template & Synthesis Guide

> **Output file**: `rounds/round-{n}/final.md` > **Manifest**: See `references/session-files.md` for authoritative file names

This guide describes how to synthesize all findings into a unified final review. Save output to `rounds/round-{n}/final.md`.

Philosophy: Model Real Engineering Teams

This synthesis process is designed to mirror how high-performing engineering teams (Google, Stripe, etc.) conduct code review, following principles articulated by Martin Fowler and the continuous delivery community:

1. **Any reviewer can block** — A single engineer identifying a critical issue (security vulnerability, data integrity risk, correctness bug) can block a merge. This is non-negotiable.

2. **No feedback is lost** — Every comment from every reviewer surfaces in the final output, fully attributed. Just like PR comments, individual perspectives are preserved—not averaged away.

3. **Suggestions are suggestions** — Non-blocking feedback (style preferences, refactoring ideas, minor improvements) is presented for consideration but doesn't prevent merge.

4. **Tech Lead synthesizes, doesn't override** — The Tech Lead aggregates and presents all feedback with a recommendation, but doesn't suppress minority opinions or "outvote" blockers.

5. **Trust and autonomy** — Authors are trusted to address feedback appropriately. Reviews are collaborative conversations, not gatekeeping.

Purpose

• **Preserve all feedback** — Every finding from every reviewer appears, attributed
• **Identify blockers** — Surface anything that should prevent merge
• **Categorize should-fix items** — Issues that aren't blocking but should be addressed
• **Present suggestions** — Lower-priority improvements for author consideration
• **Assess requirements** — Evaluate against provided requirements (if any)
• **Recommend action** — Clear verdict with rationale

Synthesis Process

Step 1: Gather All Feedback

Collect without filtering (see `references/session-files.md` for file names):

• All individual reviews from current round (`rounds/round-{n}/reviews/{type}-{n}.md`)
• Discourse results (`rounds/round-{n}/discourse.md`) if available
• Requirements context (`requirements.md`) if provided
• Tech Lead's original analysis from `context.md`

**Critical**: Do not discard or "deduplicate away" any reviewer feedback at this stage.

Step 2: Identify Blockers

A finding is a **blocker** if ANY of the following are true:

| Blocker Criteria | Examples | |------------------|----------| | **Security vulnerability** | SQL injection, auth bypass, secrets exposure | | **Data integrity risk** | Race conditions causing data loss, silent failures | | **Correctness bug** | Logic errors that produce wrong results | | **Breaking change without migration** | API contract violations, schema changes without rollback | | **Compliance violation** | GDPR, HIPAA, PCI-DSS requirements not met |

**Any single reviewer can flag a blocker.** This is not subject to consensus—one engineer seeing a security hole is sufficient to block.

Step 3: Categorize Non-Blocking Findings

All non-blocking feedback is categorized into **Should Fix** or **Suggestions**, then preserved and attributed.

**Should Fix** — Issues that aren't blocking but should be addressed before or shortly after merge:

| Should Fix Criteria | Examples | |---------------------|----------| | **Code quality issues** | Missing error handling, dead code, untested critical paths | | **Potential bugs** | Silent fallthrough, unvalidated input at boundaries, race conditions (non-data-loss) | | **Important refactors** | DRY violations with real maintenance cost, tight coupling between modules | | **Missing validation** | Input boundaries not enforced, missing null checks on external data | | **Functional gaps** | Feature partially implemented, edge case not handled |

**Suggestions** — Low-priority improvements for author consideration:

| Suggestion Criteria | Examples | |---------------------|----------| | **Style preferences** | Naming, formatting, early returns vs nested ifs | | **Minor refactors** | Extract small helper, reorder parameters, simplify expression | | **Documentation** | Add JSDoc, clarify comment, update README | | **Testing ideas** | Additional edge cases, snapshot tests, performance benchmarks | | **Informational** | Alternative approaches, FYI notes, future considerations |

## Should Fix

### 1. {Title}

**Flagged by**: @principal-1, @quality-1
**Location**: `path/to/file.ts:42-50`

{Description of the issue and why it should be fixed.}

### 2. {Title}

**Flagged by**: @quality-2
**Location**: `path/to/other-file.ts:15`

{Description.}

---

## Suggestions

### Code Quality
- "Consider extracting this into a separate function for testability" — @principal-1
- "The variable naming could be more descriptive" — @quality-1

### Performance
- "This could be optimized with memoization" — @principal-2

### Style
- "Prefer `const` over `let` here" — @quality-2

### Testing
- "Edge case for empty input not covered" — @testing-1

**No feedback is lost.** Even if only one reviewer mentions something, it surfaces.

Step 4: Note Consensus and Dissent

When multiple reviewers comment on the same area:

### Finding: Error handling in auth flow

**Reviewers**: @principal-1, @principal-2, @quality-2

@principal-1: "Missing try-catch around the OAuth callback"
@principal-2: "Agreed — this will crash on token refresh failure"
@quality-2: "The error handling exists but doesn't propagate to the UI"

**Consensus**: All agree error handling needs improvement
**Dissent**: None

When reviewers disagree:

### Finding: Caching strategy

**Reviewers**: @principal-1, @security-1

@principal-1: "Should add Redis caching for performance"
@security-1: "Caching user data introduces staleness risks for permissions"

**Consensus**: None — valid tradeoff
**Tech Lead note**: Present both perspectives