review

Code Review Skill

Perform a comprehensive review of code changes. Supports two modes:

• Local mode (default): Reviews uncommitted changes + current branch's diff from master
• PR mode: Reviews a specific pull request by URL or number

The Job

Determine Review Mode

Parse the arguments to determine the mode:

• /review or /review local → Local mode
• /review <pr_url> or /review <pr_number> → PR mode

If no argument is provided or the argument is local, use local mode.

Working Directory Detection

Determine the brave source directory based on where the skill is invoked:

# Check current working directory
CURRENT_DIR=$(pwd)

• If running from within src/brave (path contains src/brave): Use the current repo directory as the brave source
  • BRAVE_SRC="." (or the detected src/brave root)
  • CHROMIUM_SRC is ../../ relative to src/brave
• If running from brave-core-tools (path contains brave-core-tools): Use ../src/brave
  • BRAVE_SRC="../src/brave"
  • CHROMIUM_SRC="../src"
• Otherwise: Try to detect by looking for characteristic files (e.g., brave-core markers) and fall back to asking the user

Always resolve these paths and use them consistently throughout the review.

Local Mode

When reviewing local changes, gather the diff from two sources:

Step L1: Determine the Base Branch

The base branch is what this branch's changes should be compared against. Do NOT assume master — the branch may depend on another feature branch.

Detect the base branch in this order:

1. Check for an existing PR: If a PR exists for this branch, use its base branch:

   CURRENT_BRANCH=$(git -C $BRAVE_SRC branch --show-current)
   PR_BASE=$(gh pr view "$CURRENT_BRANCH" --repo brave/brave-core \
     --json baseRefName --jq '.baseRefName' 2>/dev/null) || true
   

2. Check the upstream tracking branch: If no PR exists, check what the branch tracks:

   TRACKING=$(git -C $BRAVE_SRC rev-parse --abbrev-ref \
     "$CURRENT_BRANCH@{upstream}" 2>/dev/null) || true
   # Strip the remote prefix (e.g., "origin/branch-A" -> "branch-A")
   

3. Fall back to master: If neither method yields a result, use master.

Step L2: Gather Local Changes

# 1. Get the merge base with the detected base branch
MERGE_BASE=$(git -C $BRAVE_SRC merge-base HEAD $BASE_BRANCH)

# 2. Get all committed changes on this branch since diverging from base
git -C $BRAVE_SRC diff $MERGE_BASE..HEAD

# 3. Get uncommitted changes (staged + unstaged)
git -C $BRAVE_SRC diff HEAD

# 4. Get list of changed files for context
git -C $BRAVE_SRC diff --name-only $MERGE_BASE..HEAD
git -C $BRAVE_SRC diff --name-only HEAD

Combine these diffs to form the complete set of changes to review. The combined diff represents what would be in a PR against $BASE_BRANCH if one were created right now.

Report the base branch at the start of the review so it's clear what the changes are compared against (e.g., "Reviewing against base branch: branch-A").

Step L3: Gather Context

1. Check the branch name for hints about what the change does:

   git -C $BRAVE_SRC branch --show-current
   

2. Check recent commit messages on this branch for context:

   git -C $BRAVE_SRC log $MERGE_BASE..HEAD --oneline
   

3. Read the modified files in full to understand the surrounding code context

Then proceed to the Common Analysis Steps below (Step 3 onward), using the gathered diff instead of a PR diff.

Note: For local reviews, skip Steps 1-2 (PR-specific steps) and the filtering scripts step (Step 4), since there is no PR or GitHub data to filter.

PR Mode

When reviewing a PR, follow Steps 1-3 below, then continue with the Common Analysis Steps.

Step 1: Parse PR URL and Gather Context

Extract PR information from the provided URL:

# Example: https://github.com/brave/brave-core/pull/12345
PR_REPO="brave/brave-core"  # or extract from URL
PR_NUMBER="12345"  # extract from URL

Get PR details:

gh pr view $PR_NUMBER --repo $PR_REPO --json title,body,state,headRefName,author,files

Step 2: Research Previous Fix Attempts and Prove Differentiation (PR Mode Only)

CRITICAL: Before evaluating the current fix, understand what has been tried before. If previous attempts exist, the current fix MUST prove it is materially different or the review is an AUTOMATIC FAIL.

Where to search: Previous fix attempts live as pull requests in the target repository (typically brave/brave-core). Search by issue number AND by test name/keywords, since not all PRs reference the issue directly:

# Extract issue number from PR body
ISSUE_NUMBER="<extracted from PR body>"

# Search PRs in the target repo by issue number and test name
gh api search/issues --method GET \
  -f q="repo:brave/brave-core is:pr $ISSUE_NUMBER OR <test-name>" \
  --jq '.items[] | {number, title, state, html_url, user: .user.login}'

For each previous attempt found:

# Get the diff to understand what was tried
gh pr diff <pr-number> --repo brave/brave-core

# Get review comments to understand why it failed/was rejected
gh pr view <pr-number> --repo brave/brave-core --json reviews,comments

Document findings:

• What approaches were tried before?
• Why did they fail or get rejected?
• Are there patterns in the failures?

Differentiation Requirement (AUTOMATIC FAIL if not met)

When previous fix attempts exist, you MUST compare the current PR's diff against each previous attempt's diff and answer:

1. Is the approach materially different? Compare the actual code changes, not just the PR description. Look at:

   • Are the same files being modified?
   • Are the same lines/functions being changed?
   • Is the same strategy being applied (e.g., both add a wait, both add a null check, both reorder operations)?

2. If the approach IS different, explain HOW:

   • "Previous PR #1234 added a RunUntilIdle() call. This PR instead uses TestFuture to synchronize on the specific callback."
   • "Previous PR #1234 disabled the test. This PR fixes the underlying race condition by adding an observer."

3. If the approach is the same or substantially similar → AUTOMATIC FAIL:

   • Same files modified with same type of change
   • Same strategy (e.g., both add timing delays, both add the same kind of guard)
   • Same root cause explanation with no new evidence
   • Cosmetically different but functionally identical (e.g., different wait duration, different variable name for the same fix)

The burden of proof is on the current fix. If you cannot clearly articulate why this fix is different from previous failed attempts, the review MUST FAIL with the reason: "Fix is not materially different from previous attempt(s) #XXXX."

Common Analysis Steps

The following steps apply to both local and PR mode reviews.

Step 3: Fetch Diff and Classify Changed Files

For PR mode, fetch the full diff once and save it for subagent use:

PR_DIFF=$(gh pr diff $PR_NUMBER --repo $PR_REPO)

For local mode, the diff was already gathered in Step L2. Combine the committed + uncommitted diffs into PR_DIFF.

Extract the file list from the diff:

echo "$PR_DIFF" | grep '^diff --git' | sed 's|.*b/||'

File classification is handled automatically by the discovery script in Step 6.1 — no manual classification needed.

Step 4: Fetch GitHub Data (PR Mode Only)

For the associated issue (if any):

gh issue view $ISSUE_NUMBER --repo brave/brave-browser --json title,body,comments

For PR reviews and comments:

gh api repos/$PR_REPO/pulls/$PR_NUMBER/reviews --paginate
gh api repos/$PR_REPO/pulls/$PR_NUMBER/comments --paginate
gh api repos/$PR_REPO/issues/$PR_NUMBER/comments --paginate

Step 5: Analyze the Proposed Changes

Analyze the code in context:

1. Read the modified files from $BRAVE_SRC/:

   # For each changed file, read the full file to understand context
   # Example: If the diff modifies browser/ai_chat/ai_chat_tab_helper.cc
   # Read: $BRAVE_SRC/browser/ai_chat/ai_chat_tab_helper.cc
   

2. Read related files to understand the module:

   • Header files (.h) for the modified implementation files
   • Other files in the same directory
   • Test files that exercise the modified code

3. For chromium_src overrides, also read the upstream file:

   # If modifying $BRAVE_SRC/chromium_src/chrome/browser/foo.cc
   # Also read $CHROMIUM_SRC/chrome/browser/foo.cc to understand what's being overridden
   

Questions to answer:

1. What files are changed?
2. What is the nature of the change?
   • Is it a code fix, test fix, or both?
   • Is it adding a filter/disable (potential workaround)?
3. Does the change match the problem description?
4. Is the change minimal and focused?
5. Does the fix make sense given the surrounding code?

Step 6: Check Against Best Practices (Chunked Subagent Review)

IMPORTANT: The main context does NOT load best practices docs directly. Each review is performed by subagents — one per chunk of ~3 rules — running in parallel. Large best-practice documents are split into evenly-sized chunks by a preprocessing script, so each subagent handles a focused set of rules. This ensures every rule is systematically checked rather than relying on a single pass to hold many rules in mind.

ZERO-TOLERANCE RULE: You MUST launch a subagent for EVERY chunk from EVERY discovered document. No exceptions. No filtering. No "focusing on key areas." No commentary about the number of chunks. Just launch them all.

CRITICAL — NO SHORTCUTS FOR LARGE DIFFS: Regardless of diff size (even 100KB+), you MUST pass the complete, untruncated diff to every subagent and review ALL changed files. Do NOT skip files, truncate the

Content truncated.