review

name

review

description

Three-axis review of the branch diff — Standards (this repo's documented standards + public API surface), Spec (the originating Linear issue / PR), and Correctness (runtime bugs + the SDK threat model — PII, native leaks, buffers, serialization, concurrency). Runs the axes as parallel sub-agents and reports them side by side. Use when reviewing a branch, a PR, or work-in-progress changes before opening or merging, or "review since X".

Review the diff between HEAD and a base, on three axes:

• Standards — does the diff conform to this repo's documented standards?
• Spec — does it faithfully implement the originating issue / PR?
• Correctness — will it actually work, and is it safe?

Run the axes as parallel sub-agents so none pollutes another's context, then aggregate. Report them side by side, and never rerank across axes — keeping them separate is the whole point (see Why separate axes).

This is the every-PR pass. It complements security-review (deeper security audit) and span-convention-review (tracing-span conventions) — invoke those for depth. It is not a debugger: when a bug is already known and reproducing, use diagnosing-bugs instead.

1. Pin the base

Use whatever base the user named — a commit, branch, tag, or HEAD~N. If they named none, default to the merge-base with the default branch (git merge-base HEAD origin/main). Capture once:

• git diff <base>...HEAD (three-dot, so it compares against the merge-base)
• git log <base>..HEAD --oneline

Confirm the ref resolves (git rev-parse <base>) and the diff is non-empty. A bad ref or empty diff fails here — not inside parallel sub-agents.

2. Identify the spec source

Look for the originating spec, in order:

1. Linear issue references in commit messages or the PR (fetch the issue via the Linear tools).
2. The PR description (motivation / what it does).
3. A path the user passed.

If none is found, ask. If there genuinely is no spec, the Spec sub-agent skips and reports "no spec available".

3. Spawn the sub-agents in parallel

Send one message with three Agent tool calls (general-purpose subagent for each). Give each the diff command and the commit list, plus its brief:

Standards sub-agent:

Read .agents/skills/code-guidelines/SKILL.md and .agents/skills/test-guidelines/SKILL.md, then review the diff against them. Top priority — public API surface: for any change to a barrel file (lib/sentry.dart, lib/sentry_flutter.dart, or an integration package's barrel), classify each exported symbol as added / removed / signature-changed / behavior-changed. Flag every removal or incompatible change as a breaking change (semver-major) and check it carries a deprecation path (per code-guidelines). Then report, per file/hunk, every place the diff violates a documented standard — cite the rule. For areas the docs don't cover, apply the smell baseline in .agents/skills/review/references/smell-baseline.md as judgement calls. Distinguish hard violations from judgement calls; a documented standard overrides the baseline. Skip anything dart analyze / dart format / lints enforce. Under 400 words.

Spec sub-agent (skip and note if no spec):

Report: (a) requirements the spec asked for that are missing or partial; (b) behavior in the diff that wasn't asked for (scope creep); (c) requirements that look implemented but wrong. Quote the spec line for each finding. Under 400 words.

Correctness sub-agent:

Find bugs and unsafe code the diff introduces — not style, which Standards owns. Check, per file/hunk:

• Runtime bugs — unawaited / fire-and-forget futures, uncancelled stream subscriptions, late used before init, null/empty edge cases, swallowed errors, missing cleanup in close() / dispose(), races / TOCTOU.
• SDK threat model — PII collected without gating on options.sendDefaultPii; sensitive data leaking into breadcrumbs, logs, or event payloads; native memory not released (JNI local refs, malloc) or a native exception that could crash the host app; unbounded buffers / queues / caches (memory growth); serialization correctness (toJson/fromJson round-trip, null handling); isolate / thread safety; rate-limiter or retry correctness. For each finding give file:line, a one-line severity (critical/high/medium/low), why it's real (not already handled, no covering test), and a concrete fix. Skip anything the analyzer or existing tests already catch. Under 400 words.

4. Aggregate

Present the reports under ## Standards, ## Spec, and ## Correctness headings, verbatim or lightly cleaned. Do not merge or rerank findings across axes.

End with a one-line summary per axis: total findings, and the worst issue within that axis. Don't pick a single winner across axes — that's the reranking the separation exists to prevent.

Why separate axes

A change can pass on one axis and fail on another, and a merged list lets one mask the others:

• Follows every standard but implements the wrong thing → Standards pass, Spec fail.
• Does what the issue asked but breaks conventions → Spec pass, Standards fail.
• Clean and on-spec but leaks a native ref or drops a sendDefaultPii check → Standards & Spec pass, Correctness fail.

Reporting them separately stops any one axis from hiding another.