review-changes

name

review-changes

description

Review the working-tree changes (vs origin/main) for correctness, concurrency, architecture, SwiftUI, SwiftData/CloudKit, testing, and project-specific issues — following .github/CODE_REVIEW.md — and return a severity-graded report. Scales the machinery to the diff size: a single code-reviewer agent for a small change, or a fan-out-and-verify Workflow for a large/multi-unit one. Produces findings; it does not apply fixes (the caller does).

Review Changes

Produce the pre-PR review of the current branch's changes against main, following the canonical .github/CODE_REVIEW.md. This skill scales the review to the size of the change — a single agent when the diff is small, a parallel fan-out with adversarial verification when it's large — and returns a severity-graded report. It does not edit code; the caller (e.g. /deliver's code-review phase, or you) applies the fixes.

Principles

1. One source of truth. Every reviewer — single or fanned-out — follows .github/CODE_REVIEW.md: the severity rubric, the review dimensions, and the mandatory adversarial re-evaluation. That file is an index that points at docs/*.md (SWIFT/SWIFTUI/SWIFTDATA/ARCHITECTURE/TMDB_MAPPING) as the real source of truth — reviewers read the docs, not a condensed copy. Don't invent criteria.
2. Scale to the change. Don't pay for a fan-out Workflow on a 60-line diff; don't review a 12-file multi-feature change through one over-stretched context.
3. Findings only. Return the report; the caller fixes (test-first) and may re-invoke to converge.
4. Adversarial verification cuts noise. On the large path, every Critical/High finding is independently refuted before it's reported — the strongest lever against false positives.

0. Gate — skip if there are no Swift changes

Code review exists to review Swift. If the diff touches no Swift source, there is nothing to review — return immediately, don't spawn any reviewer.

git diff --name-only origin/main...HEAD | grep -qE '\.swift$' || echo "no-swift"

If that prints no-swift (no *.swift files changed — e.g. a docs-only, config-only, .xcstrings-only, or .claude/ change), stop here and report "No Swift changes — code review skipped." Do not run §1–§3. (Snapshot reference images and .xcstrings accompany Swift changes; a non-Swift-only change is rare — note it and let the caller decide.)

1. Scope the change

git diff --stat origin/main...HEAD

Judge size (heuristic, not a rigid count):

• Small / single-unit — one cohesive area (a view model + its tests + a domain model + a mapper, or a use case + its tests), a handful of source files, roughly under ~200 changed lines → §2a.
• Large / multi-unit — a new feature spanning view + view model + use cases + adapters + tests, several new models/use-cases, or a broad multi-package diff → §2b.

When unsure, prefer small — escalate to the fan-out only when the diff is genuinely broad.

2a. Small change → single agent

Spawn the code-reviewer agent on git diff origin/main...HEAD. It follows .github/CODE_REVIEW.md and reads docs/*.md itself, including its own adversarial self-pass, and returns the full severity-graded report. Pass that report back to the caller. Done.

2b. Large change → fan-out + verify Workflow

Invoke the Workflow tool with the script below and args: { base: "origin/main" }. It fans out one reviewer per dimension (each following the spec and reading the relevant docs/*.md, focused on a single lens), dedups overlapping findings across dimensions, adversarially verifies every Critical/High finding with an independent skeptic (dropping any that don't survive), and returns the reconciled findings by severity plus droppedByVerification and dimensionsCovered.

export const meta = {
  name: 'review-changes-fanout',
  description: 'Fan-out multi-dimension code review with adversarial verification',
  phases: [
    { title: 'Find', detail: 'one reviewer per dimension, in parallel' },
    { title: 'Verify', detail: 'adversarial skeptic per Critical/High finding' },
  ],
  model: 'opus',
}

const BASE = (args && args.base) || 'origin/main'
const SPEC = '.github/CODE_REVIEW.md'

const DIMENSIONS = [
  { key: 'correctness', focus: 'correctness & safety — logic bugs, behavioural regressions, force unwraps / try! outside test helpers, and input validation at system boundaries' },
  { key: 'concurrency', focus: 'Swift 6.2 strict concurrency — async/await, actor isolation, Sendable correctness, @Observable view models are @MainActor (no missing @MainActor, no blanket @MainActor either), no DispatchQueue.* / Task.sleep(nanoseconds:), justified @preconcurrency / @unchecked Sendable' },
  { key: 'architecture', focus: 'architecture — Clean Architecture layer boundaries (Domain has no deps; Application/Infrastructure depend on Domain; Composition wires); MVVM rules (ViewState .initial→.loading→.ready/.error lifecycle, per-feature *Dependencies struct injected via init, @MainActor *Navigating protocol, view owns the VM via @State); concrete Popcorn{Context}Factory + ViewModelFactory / *Router wiring consistency; cross-context provider protocols; @Model never escaping Infrastructure' },
  { key: 'swiftui', focus: 'SwiftUI — foregroundStyle (not foregroundColor), clipShape(.rect(cornerRadius:)) (not cornerRadius), Tab API, NavigationStack + navigationDestination(for:); NO hard-coded spacing (use .spacing4/8/12/16); no ObservableObject/AnyView/hard-coded font sizes; localization bundle:.module + SCREAMING_SNAKE_CASE keys; accessibility labels/traits' },
  { key: 'swiftdata', focus: 'SwiftData / CloudKit — no @Attribute(.unique) on CloudKit-synced models; properties & relationships optional-or-defaulted in synced stores; @Model never exposed outside Infrastructure; a new VersionedSchema version + MigrationStage added when CloudKit @Model classes change' },
  { key: 'testing', focus: 'tests at ALL layers — adapter mappers, use cases, AND view models; Swift Testing with try #require (not force-unwrap); error paths and edge cases (boundaries / empty / nil), not just the happy path; feature-flag tests updated for both enabled and disabled paths; new unit-test targets registered in TestPlans/PopcornUnitTests.xctestplan (snapshot targets in PopcornSnapshotTests.xctestplan)' },
  { key: 'project-specific', focus: 'project-specific rules — TMDb mapping compliance (docs/TMDB_MAPPING.md 4-layer pipeline, naming, nil handling); remote TMDb calls use language: nil (NOT "en"); a Statsig gate exists / is created for any new FeatureFlag (gate ID = FeatureFlag.id, snake_case); no hard-coded locale/region/language values' },
]

const FINDING_SCHEMA = {
  type: 'object',
  additionalProperties: false,
  properties: {
    dimension: { type: 'string' },
    findings: {
      type: 'array',
      items: {
        type: 'object',
        additionalProperties: false,
        properties: {
          severity: { type: 'string', enum: ['critical', 'high', 'medium', 'low'] },
          file: { type: 'string' },
          line: { type: 'string', description: 'line or range, e.g. "42" or "42-50"; "" if N/A' },
          claim: { type: 'string', description: 'one-line statement of the issue' },
          why: { type: 'string', description: 'why it matters' },
          fix: { type: 'string', description: 'concrete fix' },
        },
        required: ['severity', 'file', 'line', 'claim', 'why', 'fix'],
      },
    },
  },
  required: ['dimension', 'findings'],
}

const VERDICT_SCHEMA = {
  type: 'object',
  additionalProperties: false,
  properties: {
    real: { type: 'boolean' },
    reasoning: { type: 'string' },
  },
  required: ['real', 'reasoning'],
}

const reviewPrompt = (d) =>
  `Review the changes on this branch through ONE lens only: ${d.focus}.\n\n` +
  `Get the diff yourself with \`git diff ${BASE}...HEAD\`, and read the surrounding source as needed. ` +
  `Follow the project review guidelines in ${SPEC} — the severity rubric, the review dimensions, and the MANDATORY adversarial re-evaluation (drop any finding that doesn't survive) — and read the relevant docs/*.md (SWIFT, SWIFTUI, SWIFTDATA, ARCHITECTURE, TMDB_MAPPING) it points to as the source of truth. ` +
  `Stay strictly within your lens; other reviewers cover the rest, so don't duplicate them. ` +
  `Report only findings that survive your adversarial pass, each with file, line, what's wrong, why it matters, and a concrete fix. If your lens is clean, return an empty findings array.`

phase('Find')
const perDim = await parallel(DIMENSIONS.map((d) => () =>
  agent(reviewPrompt(d), {
    label: `review:${d.key}`, phase: 'Find',
    model: 'opus', effort: 'high', agentType: 'code-reviewer', schema: FINDING_SCHEMA,
  }).then((r) => (r && r.findings ? r.findings.map((f) => ({ ...f, dimension: d.key })) : []))
))

const all = perDim.filter(Boolean).flat()

// Dedup overlapping findings across dimensions (same file:line + severity + gist).
const seen = new Set()
const deduped = all.filter((f) => {
  const key = `${f.file}:${f.line || ''}:${f.severity}:${(f.claim || '').slice(0, 40)}`
  if (seen.has(key)) return false
  seen.add(key)
  return true
})

// Critical/High get adversarially verified; Medium/Low pass through as advisory.
const blocking = deduped.filter((f) => f.severity === 'critical' || f.severity === 'high')
const advisory = deduped.filter((f) => f.severity === 'medium' || f.severity === 'low')

phase('Verify')
const verified = await parallel(blocking.map((f) => () =>
  agent(
    `A reviewer claims a ${f.severity} issue at ${f.file}:${f.line || '?'} —\n"${f.claim}"\nWhy they say it matters: ${f.why}\n\n` +
    `Try to REFUTE it. Read the actual code (\`git diff ${BASE}...HEAD\`, then open the file) and decide whether the issue is REAL and in scope for THIS change, per ${SPEC} and the docs/*.md it points to. ` +
    `Default to real=false if it is theoretical, already handled elsewhere, out of scope, or a misreading of the diff. Be strict.`,
    {
      label: `verify:${f.file}`, phase: 'Verify',
      model: 'opus', effort: 'high', agentType: 'code-reviewer', schema: VERDICT_SCHEMA,
    }
  ).then((v) => (v && v.real ? f : null))
))

const confirmed = verified.filter(Boolean)

return {
  critical: confirmed.filter((f) => f.severity === 'critical'),
  high: confirmed.filter((f) => f.severity === 'high'),
  medium: advisory.filter((f) => f.severity === 'medium'),
  low: advisory.filter((f) => f.severity === 'low'),
  droppedByVerification: blocking.length - confirmed.length,
  dimensionsCovered: DIMENSIONS.map((d) => d.key),
}

To iterate on the script, edit the file path the Workflow tool returns and re-invoke with { scriptPath } rather than resending it.

3. Output

Return the report in the .github/CODE_REVIEW.md shape — Strengths, Issues grouped Critical / High / Medium / Low (each with file:line, what's wrong, why, fix), and an Assessment (Ready to merge? + reason). On the large path, also note the dimensions covered (dimensionsCovered) and how many Critical/High findings were dropped by adversarial verification (droppedByVerification) — that number is a feature, not a gap. The caller decides what blocks and applies the fixes.

Arguments: $ARGUMENTS