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, scope, and the mandatory adversarial re-evaluation. 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-model 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 is no reviewable code

Code review exists to review code — Swift source, plus the committed scripts under .claude/workflows/ (decision infrastructure) and Scripts/ (enforcement infrastructure: check-defaulted-witnesses.py runs from make lint and as its own CI Lint step, so a bug there silently weakens a gate — it shipped as a false green once already, passing on an empty scan). If the diff touches none of them, there is nothing to review — return immediately, don't spawn any reviewer.

git diff --name-only origin/main...HEAD \
  | grep -qE '\.swift$|^\.claude/workflows/|^Scripts/' \
  || echo "no-reviewable-code"

If that prints no-reviewable-code (e.g. a docs-only, config-only, or skills-prose change), stop here and report "No reviewable code — code review skipped." Do not run §1–§3. (JSON fixtures under Tests/**/Resources/ accompany Swift changes; a fixture-only change with no .swift is rare — note it and let the caller decide.)

Override — force-review in the arguments skips this gate entirely. The caller has decided the change carries risk the file extensions don't show. /deliver passes it for a reflexive delivery — one that rewrites the skills, agents or review spec the pipeline itself runs, where the diff is markdown but the blast radius is the pipeline (#407 shipped three defects exactly there). This override lifts the gate; it does not choose a path — §1 still decides, and a caller-stated full weight still takes §2b (Risk overrides size), however prose-shaped the diff. Whichever path runs, aim its brief at the change's own subject matter: which rule changed, whether anything still depends on the old wording, and whether the new rule is enforced or merely stated. Without this the gate and the caller contradict each other and the skip silently wins.

If the only reviewable change is a script (.claude/workflows/ or Scripts/), take the §2a single-reviewer path with a script-focused brief in place of the Swift lens: input guards and executable throws, schema shapes, tally / dead-agent handling, and prompt-injection surface (what untrusted text reaches an agent prompt). For a Scripts/ checker add the question that matters most: can it pass while measuring nothing? — an empty scan, a typo'd path, a count compared instead of an explicit set. .github/CODE_REVIEW.md's severity rubric and adversarial re-evaluation still apply; its Swift-specific checks don't.

1. Scope the change

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

Judge size (heuristic, not a rigid count):

• Small / single-unit — one cohesive area (a method + its tests + a model + docs), a handful of source files, roughly under ~200 changed lines → §2a.
• Large / multi-unit — several new models/methods/services, or a broad diff → §2b.

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

Risk overrides size. Take §2b however small the diff is when either the caller states the change is full weight, or the diff touches a risky surface — concurrency (actors, Sendable, locks, Task), networking / HTTPClient, Decodable / CodingKeys, or new public API. Size decides how much there is to read; the risk surface decides how many lenses have to read it, and a cohesive one-file concurrency change is the shape whose defects most reliably survive a single pass (#401, #433, #461 each had one found late, by a second lens or a second platform). A caller that passes full has already made this judgement from the plan — do not re-derive it from --stat and quietly overrule it.

2a. Small change → single agent

Spawn the code-reviewer agent on git diff origin/main...HEAD, telling it not to build or run tests (see Reviewers don't build below). It follows .github/CODE_REVIEW.md including its own adversarial self-pass, and returns the full severity-graded report. Pass that report back to the caller. Done.

Reviewers don't build

No reviewer — single or fanned-out — may run make, swift build or swift test. Reviewing is a reading task: the reviewer has the diff and the source, and the caller has already run every gate before invoking this skill.

This is not a style preference. Every target shares one SwiftPM scratch directory per worktree, so N parallel reviewers each starting a build is N processes contending on one .build/.lock — and if any of them builds docs, the SWIFTCI_DOCC manifest flip invalidates the others' build plans, so they redo each other's work in a cycle rather than merely queueing. Observed once as ~10 zsh pipelines pinned at 100% long enough that the user killed them (see knowledge/gotchas.md → Docs builds need their own scratch path).

If a finding genuinely cannot be settled without executing something, say so in the finding and let the caller run it — serially, once.

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, focused on a single lens), dedups, adversarially verifies every Critical/High finding with an independent skeptic (dropping any that don't survive), and returns the reconciled findings by severity.

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!, and input validation at system boundaries' },
  { key: 'concurrency', focus: 'Swift 6 concurrency — async/await, actor isolation, Sendable conformance, no blanket @MainActor, and justified @preconcurrency / @unchecked Sendable' },
  { key: 'architecture', focus: 'architecture — protocol + TMDb-prefixed implementation, service-layer boundaries, new API exposed on TMDbClient with the service constructed in its private init (TMDbFactory vends shared plumbing only — services are not registered there), required model conformances, AND sibling-convention conformance: a newly-added member of an existing family (service method, model, guarded method) must match its siblings — same input validation, same error case, same conformance set / decode strategy — or the divergence is flagged' },
  { key: 'testing', focus: 'tests — both unit and integration present, fixtures exercise EVERY decoder branch, edge cases (boundaries / empty / nil), request-pattern correctness, #require over force-unwrap, AND test-suite convention conformance: a new @Suite matches its siblings (same tags / construction pattern) or the divergence is flagged' },
  { key: 'api-docs', focus: 'model<->API alignment (verify properties/optionality/types/CodingKeys via mcp__tmdb__* and the OpenAPI spec) and public-API docs + DocC catalog + README sync' },
]

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. ` +
  `DO NOT BUILD OR RUN TESTS — no \`make\`, no \`swift build\`, no \`swift test\`. You are one of several reviewers running in parallel against one shared .build directory; a build here collides with theirs and with the caller's. The caller has already run every gate. Review by reading the diff and the source. ` +
  `Follow the project review guidelines in ${SPEC} — the severity rubric, in/out scope, and the MANDATORY adversarial re-evaluation (drop any finding that doesn't survive). ` +
  `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')
// A dead agent resolves to `null` THROUGH `.then` — it does not reject — so
// `ok` must be derived here, not in a `.catch` (which would never fire). Without
// this, a died reviewer is indistinguishable from a lens that found nothing, and
// the run reports full coverage while missing a dimension entirely.
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) => ({
    key: d.key,
    ok: Boolean(r && Array.isArray(r.findings)),
    findings: r && r.findings ? r.findings.map((f) => ({ ...f, dimension: d.key })) : [],
  }))
))

// `parallel()` maps a thrown thunk to `null`, so treat that as a dead dimension
// too rather than letting it vanish from the tally.
const dimResults = DIMENSIONS.map((d, i) => perDim[i] || { key: d.key, ok: false, findings: [] })
const covered = dimResults.filter((r) => r.ok).map((r) => r.key)
const missing = dimResults.filter((r) => !r.ok).map((r) => r.key)
if (missing.length) {
  log(`WARNING: ${missing.length}/${DIMENSIONS.length} dimensions did not report (${missing.join(', ')}) — this review is PARTIAL`)
}

const all = dimResults.flatMap((r) => r.findings)

// 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}. ` +
    `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: covered,
  dimensionsMissing: missing,
  partial: missing.length > 0,
}

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

If the fan-out is interrupted or dies part-way, don't discard the run. Each completed agent's return value is recorded as a {"type":"result",…} line in journal.jsonl inside the run's transcript directory (the Workflow tool reports the path), so the findings that landed are recoverable — reduce those and report as partial, naming what is missing. Never re-present a salvaged run as complete.

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 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.

Findings you are not going to fix get filed, not dropped. A Medium or Low that is real but out of scope for this change dies in the report unless someone writes it down — and this skill produces findings precisely because the caller will not act on all of them. File those per .github/ISSUE_FILING.md, and cite the issue number beside the finding, so the report shows what was captured rather than what was noticed. Do not file Critical/High findings — those block, and filing one reads as handling it.

On the large path, coverage is mandatory in the report: list dimensionsCovered, and when partial is true name every dimension in dimensionsMissing and say plainly that the review is incomplete. A large-path report without a coverage line is malformed — treat it as untrustworthy rather than clean. A missing dimension means nobody looked, which reads identically to nothing was found unless you say so; that is the False green family (knowledge/gotchas.md), the defect this repo hits most. (§2a's single-reviewer path has no dimensions, so this does not apply there.)

Arguments: $ARGUMENTS