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.

Paths

This skill runs from the src/brave (brave-core) directory.

• Chromium source (src/) is at ../ relative to this repo

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:

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

branch:

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

• Check the upstream tracking branch: If no PR exists, check what the

branch tracks:

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

• 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=$(gitmerge-base HEAD $BASE_BRANCH)

# 2. Get all committed changes on this branch since diverging from base
gitdiff $MERGE_BASE..HEAD

# 3. Get uncommitted changes (staged + unstaged)
gitdiff HEAD

# 4. Get list of changed files for context
gitdiff --name-only $MERGE_BASE..HEAD
gitdiff --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

• Check the branch name for hints about what the change does:

   gitbranch --show-current

• Check recent commit messages on this branch for context:

   gitlog $MERGE_BASE..HEAD --oneline

• 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:

• 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)?

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

• 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:

• Read the modified files from the repo:

   # 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: src/brave/browser/ai_chat/ai_chat_tab_helper.cc

• 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

• For chromium_src overrides, also read the upstream file:

   # If modifying src/brave/chromium_src/chrome/browser/foo.cc
   # Also read ../chrome/browser/foo.cc to understand what's being overridden

Questions to answer:

• What files are changed?
• What is the nature of the change?
• Is it a code fix, test fix, or both?
• Is it adding a filter/disable (potential workaround)?
• Does the change match the problem description?
• Is the change minimal and focused?
• 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 diff, selectively

review "key chunks", or take any other shortcut based on diff size. The chunked

subagent architecture is specifically designed to handle large diffs — each

subagent only checks ~3 rules, so the diff size is not a constraint. If the diff

is large, that means MORE subagents are needed, not fewer.

CRITICAL — LAUNCH ALL DISCOVERED DOCS: You MUST launch subagents for ALL

documents returned by the discovery script — not just the ones you consider

"most relevant." The discovery script already filters out inapplicable

categories (e.g., iOS docs when no iOS files changed). If a document appears in

the discovery output, it MUST get a subagent. Do NOT second-guess the discovery

script or selectively skip documents. The only filtering happens in the script;

you do not apply additional filtering.

CRITICAL — NO COMMENTARY ABOUT CHUNK COUNT: Do NOT output any text

commenting on the number of chunks, expressing concern about the volume of work,

or announcing that you will "focus on key areas" or "launch focused subagents

for the most relevant areas." Just launch ALL subagents silently. The number of

chunks is irrelevant — launch them all in parallel without editorializing. Any

message like "Given the large number of chunks (N total), I'll focus on..." is

WRONG and violates this skill's requirements. You must launch every single chunk

subagent regardless of how many there are.

Step 6.1: Discover Applicable Docs, Chunk, and Launch Subagents

For each applicable best-practice document, run the chunking script to split it

into groups of ~3 rules, then launch one subagent per chunk. **Use multiple

Agent tool calls in a single message** so they run in parallel. Pass the

PR_DIFF content (fetched in Step 3) directly in each subagent's prompt so they

don't need to fetch it again.

Discovery step: First, determine which best-practice docs apply to this

change. The discovery script auto-detects applicable documents based on changed

file types — no hardcoded document list needed:

# Extract changed file paths from the diff
CHANGED_FILES=$(echo "$PR_DIFF" | grep '^diff --git' | sed 's|.*b/||')

# Discover applicable docs (pass changed files via stdin)
echo "$CHANGED_FILES" | python3 ./.claude/skills/review/discover-bp-docs.py \
  ./docs/best-practices/ --changed-files-stdin

The script outputs JSON with each applicable doc's path and category. Documents

are matched to file types by naming convention (e.g., testing-*.md applies

when test files are changed, android.md when .java/.kt files are changed).

Documents that don't match any specific category (like architecture.md,

documentation.md) are always included.

Chunking step: For each discovered document, run:

python3 ./.claude/skills/review/chunk-best-practices.py <doc_path>

This outputs JSON with one or more chunks per document. Each chunk contains:

• doc: the source document filename
• chunk_index / total_chunks: position within the document
• rule_count: number of ## rules in this chunk
• headings: list of rule heading texts (for the audit trail)
• content: the full text to pass to the subagent (includes the doc header +

the chunk's rules)

Small documents (<=5 rules) produce 1 chunk. Large documents are split evenly

(e.g., 25 rules → 5 chunks of 5). Launch one subagent per chunk.

Step 6.2: Subagent Prompt

Each subagent prompt MUST include:

• The chunk content — embed the content field from the chunking script

output directly in the prompt. The subagent does NOT read any files — all

rules are provided inline. Include it like:

   Here are the best practice rules to check:

<chunk content>

• The diff content — include the complete, untruncated diff text

directly in the prompt. Never omit, summarize, or truncate any portion of the

diff regardless of its size. The subagent MUST NOT call gh pr diff or

git diff — the diff is already provided. Embed it in the prompt like:

   Here is the diff to review:

<PR_DIFF content>

• The review rules (copied into the subagent prompt):
• Only flag violations in ADDED lines (+ lines), not existing code
• If you notice a violation in surrounding context lines (lines without +

prefix) or in unchanged code visible in the diff, do NOT comment on it or

suggest fixing it -- unless the changes directly affect or break that

surrounding code

• Also flag bugs introduced by the change (e.g., missing string separators,

duplicate DEPS entries, code inside wrong #if guard)

• Check surrounding context before making claims. When a violation

involves dependencies, includes, or patterns, read the full file context

(e.g., the BUILD.gn deps list, existing includes in the file) to verify

your claim is accurate. Do NOT claim a PR "adds a dependency" or

"introduces a pattern" if it already existed before the PR.

• Only comment on things the author introduced. If a dependency, pattern,

or architectural issue already existed before this PR, do not flag it —

even if it violates a best practice. The author is not responsible for

pre-existing issues. Focus exclusively on what the changes add or modify.

• Do not suggest renaming imported symbols defined outside the change.

When a + line imports or calls a function/class/variable from another

module, and that symbol's definition is NOT in a file changed by the diff,

do not comment on the symbol's naming. Only flag naming issues on symbols

that are defined or renamed within the changed files.

• Security-sensitive areas (wallet, crypto, sync, credentials) deserve extra

scrutiny — type mismatches, truncation, and correctness issues should use

stronger language

• Do NOT flag: existing code not being changed, template functions defined in

headers, simple inline getters in headers, style preferences not in the

documented best practices, include/import ordering (this is handled by

formatting tools and linters)

• Every claim must be verified in the best practices source document. Do

NOT make claims based on general knowledge or assumptions about what

"should" be a best practice. If the best practices docs do not contain a

rule about something, do NOT flag it as a violation — even if you believe

it to be true. Hallucinated rules erode trust and waste developer time.

When in doubt, do not comment.

• Comment style: short (1-3 sentences), targeted, acknowledge context. Use

"nit:" for genuinely minor/stylistic issues. Substantive issues (test

reliability, correctness, banned APIs) should be direct without "nit:"

prefix

• Best practice link requirement — each rule in the best practices docs has

a stable ID anchor (e.g., <a id="CS-001"></a>) on the line before the

heading. For each violation, the subagent MUST include a direct link using

that ID. The link format is:

   https://github.com/brave/brave-core/tree/master/docs/best-practices/<doc>.md#<ID>

**CRITICAL: The rule_link fragment MUST be an exact <a id="..."> value

from the rules provided in the chunk.** Do NOT invent IDs, guess ID numbers,

or construct anchors from heading text. If no <a id> tag exists for the

rule, omit the rule_link field entirely.

• The systematic audit requirement (Step 6.3 below)
• Required output format (Step 6.4 below)

Step 6.3: Systematic Audit Requirement

**CRITICAL — this is what prevents the subagent from stopping after finding a

few violations.**

The subagent MUST work through its chunk heading by heading, checking every

## rule against the diff. It must output an audit trail listing EVERY ##

heading in the chunk with a verdict:

AUDIT:
PASS: Always Include What You Use (IWYU)
PASS: Use Positive Form for Booleans and Methods
N/A: Consistent Naming Across Layers
FAIL: Don't Use rapidjson
PASS: Use CHECK for Impossible Conditions
... (one entry per ## heading in the chunk)

Verdicts:

• PASS: Checked the diff — no violation found
• N/A: Rule doesn't apply to the types of changes in this diff
• FAIL: Violation found — must have a corresponding entry in VIOLATIONS

This forces the model to explicitly consider every rule rather than satisficing

after a few findings.

Step 6.4: Required Subagent Output Format

Each subagent MUST return this structured format:

DOCUMENT: <document name> (chunk <chunk_index+1>/<total_chunks>)

AUDIT:
PASS: <rule heading>
N/A: <rule heading>
FAIL: <rule heading>
... (one line per ## heading in the chunk)

VIOLATIONS:
- file: <path>, line: <line_number>, severity: <"high"|"medium"|"low">, rule: "<rule heading>", rule_link: <full GitHub URL to the rule heading>, issue: <brief description>, draft_comment: <1-3 sentence comment>
- ...
NO_VIOLATIONS (if none found)

Severity guide:
- **high**: Correctness bugs, use-after-free, security issues, banned APIs, test reliability problems (e.g., RunUntilIdle)
- **medium**: Substantive best practice violations (wrong container type, missing error handling, architectural issues)
- **low**: Nits, style preferences, missing docs, naming suggestions, minor cleanup

Step 6.5: Aggregate and Validate Results

After ALL chunk subagents return:

• Aggregate violations from all chunk subagents into a single list. Sort by

severity: high → medium → low.

• Validate rule links — for each violation with a rule_link, extract the

fragment ID and validate it exists in the target doc:

   python3 ./script/manage-bp-ids.py --check-link <ID> --doc <doc>.md

If the ID is invalid, strip the link from the comment text. Violations

missing rule_link that are not genuine bug/correctness/security findings

should be dropped.

• Deep-dive validation — before including in the report, validate every

remaining violation by reading the actual source code:

• Read the actual source file at and around the flagged line using the

Read tool (not the diff) to see the full file context

• Verify the claim is true. If the violation says "this should use X

instead of Y", confirm X is actually available, appropriate, and consistent

with the rest of the file/module

• Deprecation claims require header verification. Read the actual header

file to confirm deprecation. Do NOT rely on training data

• Check surrounding context for justification. Look for comments, TODOs,

or patterns that explain the code

• Drop false positives. If reading the source reveals the violation is

incorrect, drop it

• Present violations one at a time and offer to fix each. After validation,

walk through the remaining violations sequentially. For each violation,

present it to the user and ask whether they want it fixed:

   **Violation 1/N** (severity: high)
   **File**: path/to/file.cc:42
   **Rule**: <rule heading>
   **Issue**: <description>
   **Suggested fix**: <what the fix would look like>

   Fix this violation? (yes/no/skip)

• yes: Apply the fix immediately using the Edit tool. Read the file first

if not already loaded, make the targeted change, then confirm what was

changed before moving to the next violation.

• no or skip: Leave the code as-is and move to the next violation.
• If the user says "fix all" or "yes to all", apply all remaining violations

without further prompting.

• If the user says "stop" or "no to all", skip all remaining violations and

proceed to the report.

Fix guidelines:

• Fixes must be minimal and targeted — only change what the violation

requires

• Do NOT refactor surrounding code or make "while you're here" improvements
• If a fix is ambiguous or could be done multiple ways, explain the options

and ask the user which approach they prefer before editing

• If a violation cannot be auto-fixed (e.g., requires architectural redesign

or new tests), say so and move on

• Include all violations (fixed and unfixed) in the review report under the

Best Practices section. Mark each as (fixed) or (unfixed) so the user

knows what remains.

Step 7: Validate Root Cause Analysis

Read the PR body and any issue analysis carefully.

Check for RED FLAGS indicating insufficient root cause analysis:

Vague/Uncertain Language (FAIL if unexplained)

• "should" - e.g., "This should fix the issue"
• "might" - e.g., "This might be causing the problem"
• "possibly" - e.g., "This is possibly a race condition"
• "probably" - e.g., "The test probably fails because..."
• "seems" - e.g., "It seems like the timing is off"
• "appears" - e.g., "The issue appears to be..."
• "could be" - e.g., "This could be the root cause"
• "may" - e.g., "The callback may not be completing"

These words are acceptable ONLY if followed by concrete investigation:

• BAD: "This should fix the race condition"
• GOOD: "The race condition occurs because X happens before Y. Adding a wait for

signal Z ensures proper ordering."

Questions to Ask

• Can you explain WHY the test fails? (Not just symptoms, but cause)
• Can you explain HOW the fix addresses the root cause? (Mechanism, not

hope)

• Is there a clear causal chain? (A causes B, fix C breaks the chain)
• Why does this fail in Brave specifically? (What Brave-specific factors

contribute - e.g., different UI elements, additional features, different

timing characteristics, etc.)

AI Slop Detection

Watch for generic explanations that could apply to any bug:

• "Improved error handling"
• "Fixed timing issues"
• "Better synchronization"
• "Enhanced stability"

Demand specifics:

• WHAT timing issue? Between which operations?
• WHAT synchronization was missing? What signal is now used?
• WHERE was the race condition? What two things were racing?

Brave-Specific Context Required

If a test fails in Brave but passes in Chrome (or is flaky in Brave but stable

in Chrome), the root cause analysis MUST explain what Brave-specific factors

contribute:

• Different UI elements (Brave Shields, sidebar, wallet button, etc.)
• Additional toolbar items or browser chrome that affects layout/sizing
• Brave-specific features that change timing or execution order
• Different default settings or feature flags
• Additional observers or hooks that Brave adds

Without this explanation, the analysis is incomplete even if the general

mechanism is understood.

Step 8: Additional Best Practices Checks (Non-Chunked)

These checks are performed directly by the main context (not subagents) because

they require PR-level reasoning rather than per-rule checking:

Timing-Based "Fixes" (AUTOMATIC FAIL)

If the fix works by altering execution timing rather than adding proper

synchronization:

BANNED patterns:

• Adding sleep/delay calls
• Adding logging that changes timing
• Reordering code without synchronization explanation
• Adding RunUntilIdle() (explicitly forbidden by Chromium)
• Adding arbitrary waits without condition checks

ACCEPTABLE patterns:

• base::test::RunUntil() with a proper condition
• TestFuture for callback synchronization
• Observer patterns with explicit quit conditions
• MutationObserver for DOM changes (event-driven)

Nested Run Loop Issues (AUTOMATIC FAIL for macOS)

• EvalJs() or ExecJs() inside RunUntil() lambdas
• This causes DCHECK failures on macOS arm64

Test Disables

If the fix is disabling a test:

• Is there thorough documentation of why?
• Were other approaches tried first?
• Is this a Chromium test (upstream) or Brave test?
• If Chromium test, is it also disabled upstream?

CRITICAL: Use the most specific filter file possible.

Filter files follow the pattern: {test_suite}-{platform}-{variant}.filter

Available specificity levels (prefer most specific):

• browser_tests-windows-asan.filter - Platform + sanitizer specific (MOST

SPECIFIC)

• browser_tests-windows.filter - Platform specific
• browser_tests.filter - All platforms (LEAST SPECIFIC - avoid if possible)

Before accepting a test disable, verify:

• Which CI jobs reported the failure? Check issue labels (bot/platform/_,

bot/arch/_) and CI job names

• Is the root cause platform-specific? (e.g., Windows-only APIs,

macOS-specific behavior)

• Is the root cause build-type-specific? (e.g., ASAN/MSAN/UBSAN, OFFICIAL

vs non-OFFICIAL)

• Does a more specific filter file exist or should one be created?

Examples:

• Test fails only on Windows ASAN → use browser_tests-windows-asan.filter
• Test fails only on Linux → use browser_tests-linux.filter
• Test fails on all platforms due to Brave-specific code → use

browser_tests.filter

• Is the test flaky upstream? (Chromium tests only) — This check only

applies to upstream Chromium tests (defined in src/ but NOT in

src/brave/). Brave-specific tests won't appear in the Chromium database.

For Chromium tests, check the LUCI Analysis database:

   python3 ./script/check-upstream-flake.py "<TestName>"

The script queries analysis.api.luci.app and returns a verdict:

• Known upstream flake (>=5% flake rate): Supports the disable. Verify

the PR comment mentions upstream flakiness.

• Occasional upstream failures (1-5%): Weakly supports the disable. PR

should document this.

• Stable upstream (<1%): The test is reliable in Chromium — the root

cause is likely Brave-specific. The PR must explain what Brave-specific

factors cause the failure. A disable without this explanation is a **red

flag**.

• Not found / Insufficient data: Cannot determine from upstream data.

Manual analysis needed.

Use --days 60 for a wider lookback window if the default 30 days has

insufficient data.

Red flags (overly broad disables):

• Adding to browser_tests.filter when failure is only reported on one platform
• Adding to general filter when failure is only on sanitizer builds

(ASAN/MSAN/UBSAN)

• No investigation of which CI configurations actually fail

Intermittent/Flaky Test Analysis

For flaky tests, the root cause analysis must explain **why the failure is

intermittent** - not just why it fails, but why it doesn't fail every time:

Questions to answer:

• What variable condition causes the test to sometimes pass and sometimes fail?
• Is it timing-dependent? (e.g., race between two async operations)
• Is it resource-dependent? (e.g., system load, memory pressure)
• Is it order-dependent? (e.g., test isolation issues, shared state)
• Is it platform-specific? (e.g., only flaky on certain OS/architecture)

Examples of good intermittency explanations:

• "The test is flaky because the viewport resize animation may or may not

complete before the screenshot is captured, depending on system load"

• "The race window is small (~15ms) so the test only fails when thread

scheduling happens to interleave the operations in a specific order"

• "On slower CI machines, the async callback completes before the size check; on

faster machines, it doesn't"

Red flags (incomplete analysis):

• "The test is flaky" (without explaining the variable condition)
• "Sometimes passes, sometimes fails" (just restating the symptom)
• "Timing-dependent" (without explaining what timing varies)

Step 9: Assess Fix Confidence

Rate confidence level:

HIGH Confidence (likely to work)

• Clear root cause identified and explained
• Fix directly addresses the root cause
• Change is minimal and focused
• Similar patterns exist in codebase
• Tests verify the fix

MEDIUM Confidence (may work, needs verification)

• Root cause identified but explanation has minor gaps
• Fix seems reasonable but relies on assumptions
• Could benefit from additional tests

LOW Confidence (likely to fail or regress)

• Root cause not clearly identified
• Fix is a workaround, not a solution
• Uses timing-based approaches
• Overly complex for the problem
• Changes unrelated code
• Fix is not materially different from a previous failed attempt

Step 10: Generate Review Report

CRITICAL: Avoid Redundancy

• Each piece of information should appear ONCE in the report
• Do NOT repeat the same issue in multiple sections
• The verdict reasoning should be a brief reference, not a restatement of

everything above

CRITICAL: Fill Informational Gaps Yourself

• If the PR is missing context that you CAN research (e.g., "why does this flake

in Brave but not upstream?"), DO THE RESEARCH and provide the answer in your

analysis

• Only list something as an "issue requiring iteration" if it requires action

from the PR author that you cannot provide

• The confidence level should reflect the state AFTER you've provided any

missing context - if you filled the gaps, confidence should be higher

CRITICAL: No Vague Language in YOUR Analysis

• The same vague language rules (Step 7) apply to YOUR review output, not just

the PR's analysis

• If you write "appears to", "seems to", "might be", etc. in your analysis, you

have NOT completed the review

• You must either:
• Investigate further until you can make a definitive statement, OR
• Flag it as requiring investigation in the "Issues Requiring Author

Action" section

• Example of what NOT to do: "The channel detection appears to return STABLE" -

this is incomplete

• Example of what TO do: Either trace the exact code path to confirm what value

is returned, OR list "Determine exact channel value returned in CI

environment" as an issue requiring investigation

PR Mode Report Format

# PR Review: #<number> - <title>

## Summary

<2-3 sentences: what this PR does, the root cause, and whether the fix is
appropriate>

## Context

- **Issue**: #<number or "N/A">
- **Previous attempts**: <Brief list or "None found">
- **Differentiation**: <How this fix differs from previous attempts, or "N/A -
  no previous attempts">

## Analysis

### Root Cause

<Summarize the PR's explanation. If incomplete, research and provide the missing
context yourself rather than flagging it as an issue.>

### Brave-Specific Factors (if applicable)

<If this fails in Brave but not upstream, research and explain why. Provide this
context yourself.>

### Fix Evaluation

<Does the fix address the root cause? Any best practices violations?>

### Best Practices (Chunked Subagent Results)

<Summarize findings from the chunked best practices review. List any validated
violations with file, line, severity, and the specific rule violated. Include
rule links where available.>

## Issues Requiring Author Action

<ONLY list issues that genuinely require the PR author to take action. Do NOT
include:

- Informational gaps you filled in the Analysis section
- Context you researched and provided above
- Minor suggestions>

If no issues: "None - PR is ready for review."

## Verdict: PASS / FAIL (assessed AFTER accounting for any context you provided above)

**Confidence**: HIGH / MEDIUM / LOW

<1-2 sentence reasoning>

Local Mode Report Format

# Local Review: <branch-name>

## Summary

<2-3 sentences: what these changes do and whether the approach is sound>

## Changes Overview

- **Branch**: <branch-name>
- **Base branch**: <base-branch> (how it was detected: PR / tracking / default
  master)
- **Files changed**: <count>
- **Commits on branch**: <count> (+ uncommitted changes if any)

## Analysis

### Change Evaluation

<What do the changes accomplish? Is the approach correct? Any logic errors?>

### Best Practices (Chunked Subagent Results)

<Summarize findings from the chunked best practices review. List any validated
violations with file, line, severity, and the specific rule violated. Include
rule links where available.>

## Issues Found

<List significant issues that should be addressed before creating a PR. Do NOT
include:

- Style preferences
- Minor naming suggestions
- Optional refactoring ideas>

If no issues: "None - changes look ready for PR."

## Verdict: PASS / FAIL

**Confidence**: HIGH / MEDIUM / LOW

<1-2 sentence reasoning>

Important Guidelines

Only Report Significant Issues

DO report:

• Logic errors or bugs in the fix
• Missing synchronization or race conditions
• Violations of documented best practices
• Incomplete root cause analysis
• High-risk changes without adequate testing
• Potential regressions

DO NOT report:

• Style preferences
• Minor naming suggestions
• Optional refactoring ideas
• "While you're here..." improvements
• Anything that doesn't warrant a round-trip iteration

Be Specific and Actionable

• BAD: "The root cause analysis is weak"
• GOOD: "The PR says 'This should fix the timing issue' but doesn't explain what

timing issue exists or why this change fixes it. Specifically, what two

operations are racing and how does the new wait prevent that race?"

Read the Source Code

• Always read the actual files from the repo to understand context
• Don't just look at the diff in isolation
• Check related files, headers, and tests
• For chromium_src changes, also read the upstream Chromium file
• **Before each review comment, verify your claims by reading the relevant

source code in src/brave/ and src/.** Do not make assertions about APIs,

patterns, deprecations, or behavior without first confirming them in the

actual codebase. Look at how the API/pattern is used elsewhere, check header

files for documentation, and read upstream Chromium code when relevant. Every

comment you make should be grounded in what the code actually says, not

assumptions.

Local by Default

• DO NOT post comments, approve, or request changes on GitHub **unless the

user explicitly asks**

• DO NOT merge or close the PR
• This is an analysis tool for the reviewer's eyes only

Posting to GitHub

If the user asks you to post the review as a comment on GitHub, **always prefix

the review body** with:

I generated this review about the changes, sharing here. It should be used for

informational purposes only and not as proof of review.

This disclaimer must appear at the very beginning of the review body (as plain

text, not as a blockquote).

Post as inline code comments when possible. When the review identifies

specific issues tied to files and lines, post them as inline review comments on

the actual code rather than as a single general comment. Use the GitHub review

API to submit a single review with:

• Review body: The summary, verdict, and any general observations (with the

disclaimer prefix above)

• Inline comments: Each specific issue placed on its file and line

gh api repos/brave/brave-core/pulls/{number}/reviews \
  --method POST \
  --input - <<'EOF'
{
  "event": "COMMENT",
  "body": "I generated this review about the changes, sharing here. It should be used for informational purposes only and not as proof of review.\n\n## Summary\n...\n\n## Verdict: PASS/FAIL\n...",
  "comments": [
    {
      "path": "path/to/file.cc",
      "line": 42,
      "side": "RIGHT",
      "body": "specific issue description for this line"
    }
  ]
}
EOF

Key details:

• side: "RIGHT" targets the new version of the file (changed lines)
• line is the line number in the new file
• All inline comments are batched into one review (one notification to the

author)

• If an issue can't be tied to a specific line in the diff, include it in the

review body instead

• If the inline API call fails for a comment (line outside diff range), fall

back to including that issue in the review body

Example Usage

Review local changes (default - no argument needed):

/review
/review local

Review a specific PR by URL:

/review https://github.com/brave/brave-core/pull/12345

Review a PR by number (assumes brave/brave-core):

/review 12345

Checklist Before Completing Review

Both Modes

• [ ] Confirmed running from brave-core directory
• [ ] Analyzed the diff (local changes or PR diff)
• [ ] Ran discovery script to find applicable best practices documents
• [ ] Ran chunking script on each discovered document
• [ ] Launched parallel subagents for all chunks
• [ ] Each subagent produced a full AUDIT trail (one verdict per ## heading)
• [ ] Aggregated and validated all violations from subagents
• [ ] Deep-dive validated each violation by reading actual source files
• [ ] Read the actual source files in src/brave/ to understand context
• [ ] Validated root cause analysis quality (or assessed code quality for local)
• [ ] Checked timing-based fixes, nested run loops, and test disables (Step 8)
• [ ] Assessed fix confidence level
• [ ] Only reported important issues
• [ ] Provided clear pass/fail verdict with reasoning

PR Mode Only

• [ ] Parsed PR URL and gathered context
• [ ] Extracted associated GitHub issue (if any)
• [ ] Researched previous fix attempts
• [ ] If previous attempts exist: proved current fix is materially different (or

FAILED the review)

• [ ] Only posted to GitHub if user explicitly requested (with disclaimer

prefix)

• [ ] For test disables: checked upstream flakiness via check-upstream-flake.py

Local Mode Only

• [ ] Detected correct base branch (PR base > tracking branch > master)
• [ ] Gathered uncommitted changes (staged + unstaged)
• [ ] Gathered branch changes since diverging from base branch
• [ ] Checked branch name and commit messages for context