review

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.