Code Review

Code Review

Multi-reviewer audit of the current branch's changes. The orchestrator computes the diff, classifies changed files by Go top-level package, and spawns subagents that apply the team's go-* rules and xrpl-standards specs. Findings come back as JSON, are deduped + capped, and rendered grouped by file.

This skill does not replace /review or /security-review. It's a rule-based audit keyed to this repo's codified standards.

Pipeline

1. Resolve scope

Default: this branch's own first-parent, non-merge commits vs main. Not git diff main...HEAD — that semantic includes commits brought in by merging other feature branches into this one (e.g., chained-PR dependencies not yet on the base), causing reviewers to flag work that doesn't belong to this PR.

Computing the PR's own scope:

# This branch's own commits (chronological)
git log --first-parent main..HEAD --no-merges --reverse --format=%H

# This branch's own file scope
git log --first-parent main..HEAD --no-merges --name-only --format= | sort -u

# This branch's own cumulative diff — feed this to subagents instead of `git diff main...HEAD`
for sha in $(git log --first-parent main..HEAD --no-merges --reverse --format=%H); do
  git show --format= "$sha"
done

Detect feature-merges — merge commits in main..HEAD whose second parent is not an ancestor of main:

git log --merges main..HEAD --format=%H | while read sha; do
  p2=$(git rev-parse "$sha^2")
  if ! git merge-base --is-ancestor "$p2" main; then
    echo "$sha $(git log -1 --format=%s "$sha")"
  fi
done

If any feature-merges are detected, prepend a banner to the report: ⚠️ N merged-in branch(es) excluded from review (not yet on <base>): <subjects>. Pass --include-merged-branches to include.

When --include-merged-branches is set, fall back to the broader git diff main...HEAD semantics (the PR plus any merged-in feature branches).

Prompt the user (AskUserQuestion) only on ambiguous cases:

If the user passed flags or a positional package, those resolve the ambiguity — don't re-prompt.

2. Classify changed files

A package = top-level dir under repo root: address-codec/, binary-codec/, keypairs/, xrpl/, pkg/. Anything else (examples/, root-level files) → other.

Build {package: [files]}.

3. Decide single-pass vs fan-out

• Single-pass when added+removed lines < 200 AND only one package touched: run the go-mistakes rubric inline (no subagent), skip to step 6.
• Fan-out otherwise.

4. Spawn subagents (in parallel — single message, multiple Agent calls, subagent_type: "general-purpose")

Go-mistakes — one per changed package. Use prompts/go-mistakes.md as the prompt with substitutions:

• {{PACKAGE}} → package name
• {{CHANGED_FILES}} → newline-separated file list
• {{BASE}}, {{INCLUDE_UNCOMMITTED}}, {{INCLUDE_UNTRACKED}}

XRPL-domain — at most one, conditional. Spawn if any changed file matches:

• xrpl/transaction/**, xrpl/ledger-entry-types/**, xrpl/queries/**
• binary-codec/types/**, binary-codec/definitions/**

Use prompts/xrpl-domain.md with the same substitutions but {{CHANGED_FILES}} is the XRPL-relevant slice across packages.

If --only=go → skip XRPL subagent. If --only=xrpl → skip go-mistakes subagents.

5. Collect output

Each subagent returns prose followed by one ```json fenced block on the last lines (schema below).

Extract the last ```json block. On parse failure, retry once: "Your previous output was not valid JSON. Parser error: <err>. Reply with ONLY the JSON array, no prose."

If the retry also fails, fall back: include the subagent's raw text as a single concern-level finding with source: "<reviewer>/unparseable". Never abort the run on one bad subagent.

6. Cross-cutting synthesis

After all subagents return, the orchestrator does its own pass on the merged JSON + the diff:

1. Dedup exact (file, line, message) matches — keep one, merge source fields.
2. Import boundary check. Per CLAUDE.md's layering, low-level packages (address-codec/, keypairs/, binary-codec/, pkg/) must NOT import from xrpl/. Flag violations as blocker / cross-cutting/layering.
3. Shared-package consumers. If a low-level package's exported symbol changed, git grep for callers in xrpl/. If a caller wasn't also updated in this diff, flag concern / cross-cutting/consumer.
4. CHANGELOG check. If any .go file under the four real packages changed, verify CHANGELOG.md was modified in this diff. If not, emit concern / cross-cutting/changelog.

7. Render

Apply in order:

1. --severity filter (blocker → blocker only; concern → blocker+concern; nit → all).
2. --only filter (drops findings whose source doesn't match; cross-cutting findings always pass).
3. Per-file per-source nit cap = 5. Excess collapsed to +N more nits.
4. Global cap = 20. Blockers and concerns never drop. If blockers + concerns > 20, drop nits entirely.
5. Group by file (path order). Within a file, sort by severity (blocker → concern → nit) then line number.

Print the report. No follow-up actions, no auto-fix.

Subagent JSON schema

[
  {
    "file": "binary-codec/types/signers.go",
    "line": 142,
    "severity": "blocker",
    "source": "go-errors/error-wrapping",
    "message": "Wraps with %v, loses error chain.",
    "suggestion": "Use %w."
  }
]

• file — repo-relative, forward slashes.
• line — 1-based; use 1 for file-level findings.
• severity — "blocker" (must fix), "concern" (likely problem), "nit" (minor).
• source — citation. go-<skill>/<rule-filename-without-md>, xrpl-domain/XLS-NN or xrpl-domain/<category>, general/<category>, cross-cutting/<category>.
• message — what's wrong and why. Reference identifiers by name.
• suggestion — optional, concrete fix.

Output format

Header line: Branch: <current> vs <base> and severity counts. Then per-file sections. Markers: 🔴 blocker, 🟡 concern, 🔵 nit.

Banners (prepend if applicable):

• Dirty tree without --include-uncommitted: "⚠️ N uncommitted file(s) not in this review. Run with --include-uncommitted to include."
• Untracked .go files without --include-untracked: "⚠️ N untracked .go file(s) not reviewed: ..."
• Feature-merges excluded (default first-parent scope): "⚠️ N merged-in branch(es) excluded from review (not yet on ): . Pass --include-merged-branches to include."
• Subagent unparseable: "⚠️ returned unparseable output."

If zero findings after filtering: print one line — ✅ No issues found. The changes look good.

Don't add filler praise, "files reviewed" lists, or per-reviewer section headers (the [<source>] tag attributes each finding).