Review PRs for Best Practices

Scan recent open PRs in the configured PR repository for violations of documented best practices.

• Interactive mode (default): drafts comments and asks for user approval before posting.
• Auto mode (auto argument): posts all violations automatically without approval. Designed for cron/headless use.
• full: review every file in each PR. Without it, a PR the bot reviewed before is reviewed only in the files whose changes differ since then. A PR where none differ (a rebase, a merge of the base branch) is not reviewed again. When the bot is a requested reviewer on it, an unchanged patch is never a reason to approve: the request is answered without a model only for a patch the bot itself approved. That means the per-file hashes (of each file's diff against the merge base, ignoring blob ids and hunk offsets) equal the ones recorded when the bot's approval was posted (_approved_files in the cache), the bot's own latest review on the PR, read from GitHub, is that approval, and no bot comment is open. prepare-review.py then writes no prompts and sets needs_model: false in the manifest; review-session.sh starts no agent and runs select-candidates.py and collect-results.py, which approve again. Any other case, including a failed lookup, is read whole by the detectors as usual. A PR with no stored hashes is reviewed whole.

IMPORTANT: This skill only reviews PRs against existing best practices. It must NEVER create, modify, or add new best practice rules or documentation during a review run. A profile with review.proposeBestPractices lets a review describe a rule the project lacks, which collect-results.py files as an issue (below); the docs are never touched.

---

Architecture: File-Based Pipeline

The review pipeline minimizes LLM token usage by pushing all heavy data through files, not context:

1. prepare-review.py (zero LLM tokens) — fetches PRs, diffs, comments; works out which files changed since the last review; writes one detect prompt per rule chunk, plus one for bugs, to a temp work directory (more than one per check when a large diff is split into parts); outputs a tiny JSON pointer to the work dir
2. Detect subagents (review-prs-detect, Sonnet) — each reads its prompt from a file, checks the diff against its rules, and writes candidate findings to a JSON file. The rule-checking ones work from the diff. The bug pass reads the PR-head worktree with Read, Grep and Glob, following callers and callees the way a maintainer would, and the project-guidance pass reads the files its guidance names (below).
3. select-candidates.py (zero LLM tokens) — drops the candidates post-review.py would drop anyway (no rule link, a rule id that does not exist, duplicates, lines already commented on, everything past twice the per-PR cap) and writes one validate prompt per PR that has any left, or that has earlier bot comments still unresolved
4. Validate subagents (review-prs-validate, Opus) — one per PR; reads the candidates against the PR's source tree and writes the ones that hold up. It also rules on each of the bot's earlier unresolved comments: addressed or not, with a reason
5. collect-results.py (zero LLM tokens) — reads the validated results, feeds them to post-review.py which handles prioritization, dedup, posting, approval, cache updates and notifications

Project review guidance. A project profile (projects/<profile>/profile.json) may carry a review key, documented in projects/README.md. Its guidance text is read by one extra detect subagent per PR (chunk_id project, launched like the others from subagent_prompts) and by the PR's validator. prepare-review.py and select-candidates.py add it to the prompts, so the main session does nothing differently. The bot posts inline comments only: no review body, no summary, no recommendation, and never a remark about its own permissions or about a thread it could not resolve. review.verdict and review.requestChanges only choose the review event (REQUEST_CHANGES versus COMMENT). A profile without the key reviews as before.

Which best-practice docs a PR is checked against. Each doc in the target repo's best-practices/ declares it in an <!-- applicability: CONDITION --> comment in its first 10 lines: always, a file-type condition (has_cpp_files, has_frontend_files, … — see discover-best-practices.py), or paths:ui/,crates/ui-bridge/, which runs the doc only when a changed file is under one of those directories and shows it only those files.

Rule links on every finding. The bug pass and the project-guidance pass write findings with no rule_link, so a finding that breaks a documented rule would show none. With review.linkRules, select-candidates.py gives each validator an index of the rules that PR was checked against (URL, heading, start of the rule), and the validator adds cited_rule_link to an unlinked candidate that breaks one. It is a separate field on purpose: rule_link is a dedup key in post-review.py (two findings with the same one in a file collapse), and a link the validator merely cited says nothing of the kind. collect-results.py keeps a cited link only when it names a rule in the project's docs, takes the link text from the docs, and post-review.py appends it after dedup.

Proposed best practices. With review.proposeBestPractices, the validator may add one entry to proposals for a rule the review showed the project lacks and no check could decide. After posting, collect-results.py --auto files it as an issue in project.issueRepository, titled Best practice: <rule>, through file-proposals.py: at most one per PR and three per run, nothing filed when it matches an existing rule heading or an issue that already proposes it (or came from the same PR), every field cleaned of mentions and foreign links, and a failure reported and never raised. Without --auto the proposals are only printed. Only a PR that has a validator can draw one: a PR with no candidates and no open threads is clean and gets none.

The main LLM session only orchestrates: run scripts, read a small manifest, launch subagents with tiny prompts, run the collector. It never sees diffs, rule text, or violation details.

---

The Job

When invoked with /review-prs [days|page<N>|#<PR>] [open|closed|all] [auto] [reviewer-priority] [full]:

Scheduled runs: /review-prs --work-dir <dir>

The schedules start the skill through scripts/review-session.sh, which has already run Step 1 and will run Step 6 itself once this session ends. Invoked with --work-dir <dir>, do Steps 2 to 5 only, with {work_dir} set to <dir>, and stop after the validators return. Do not run prepare-review.py or collect-results.py.

This session has no GitHub token and runs under a fixed set of permission rules (scripts/review-session-settings.py): reads in the bot directory and the work directory, writes only in the work directory, subagents, and one Bash command, which must be written exactly as Step 4 shows it, from the bot directory:

python3 .claude/skills/review-prs/select-candidates.py --work-dir "<dir>"

Read the manifest with the Read tool, not a shell command. A denied tool call is the rules working, not something to work around: carry on with what is allowed, and say what was denied in your final message.

Step 1: Prepare (zero LLM tokens)

Run the prepare script with all arguments (auto → --auto, reviewer-priority → --reviewer-priority, full → --full):

BOT_DIR="<absolute path to brave-dev-loop directory>"
python3 $BOT_DIR/.claude/skills/review-prs/prepare-review.py [days|page<N>|#<PR>] [open|closed|all] [--auto] [--reviewer-priority] [--full]

The script's stdout is a tiny JSON with work_dir and manifest paths. Progress and cost summary go to stderr.

Parse the stdout JSON to get work_dir.

Step 2: Read manifest

Read the manifest file at {work_dir}/manifest.json. It contains:

• auto_mode: whether to post without approval
• bot_username: the bot's GitHub username
• pr_repo: the target PR repository
• target_repo_path: absolute path to the local target repo checkout
• fetch_summary: stats on how many PRs were fetched/filtered/skipped
• progress_lines: pre-formatted progress messages — print these to stdout for cron logs
• prs: array of PRs to review, each containing:
  • number, title, headRefOid, author, hasApproval
  • files_reviewed of files_total: how many of the PR's files this run reviews
  • subagent_prompts: array of entries with prompt_file and results_file paths (NOT prompt text)
  • summary_prompt: null, or one more entry with prompt_file and results_file for the subagent that describes the PR. Present only for a project whose profile sets review.verdict
• cached_prs: PRs not reviewed this run — already reviewed at this commit, or no file changed since the last review (handled by the prepare script — just log results)
• errors: per-PR errors encountered during preparation

Print the progress_lines. Log any errors.

For each cached PR, log:

• If approved is true: APPROVE: [PR #N](url) (title) - all threads resolved, approved
• If thread_resolution.unresolved_bot_threads > 0: CACHED: [PR #N](url) (title) - N threads still unresolved (a cached PR only has threads resolved by a person's reply; the validator rules on the rest when a file changes)

If no PRs to review (empty prs array), skip to Step 6.

Step 3: Launch the detect subagents

prepare-review.py already fetched each PR's head commit and created an isolated git worktree for it, which the validators read in Step 5. No checkout work needed here.

A worktree is a full checkout of the target repo, so a run needs tens of gigabytes. They are created under /var/tmp/review-prs — override with REVIEW_PRS_WORK_DIR, pointing it at a filesystem with room rather than a tmpfs. In auto mode a PR whose worktree could not be created is dropped from the run with a worktree entry in errors, because reviewing it would read the default branch while judging a diff from the PR head. Report those PRs as skipped.

For every PR in prs, for every entry in that PR's subagent_prompts, launch a subagent with subagent_type: "review-prs-detect" and this prompt:

Read your instructions from: {prompt_file}

If the review-prs-detect agent type is not available, use subagent_type: "general-purpose" with model: "sonnet" and the same prompt.

Every prompt file fits in one Read call. When a check's diff does not fit alongside its rules, prepare-review.py writes the diff into page files that each fit one read, lists them in the prompt (and in the entry's read_files), and gives each subagent at most four pages, splitting a larger diff into entries with part and total_parts. Each part is a separate entry, launched like any other. The subagent reads the pages itself; do not read them for it.

A PR's summary_prompt, when it is not null, gets a subagent in the same way, with the same subagent_type. It is not one of the detect prompts: select-candidates.py never reads it, and a PR whose summary subagent failed is still reviewed, with no description section.

Launch ALL detect subagents across ALL PRs in a single message so they run concurrently.

CRITICAL: Launch ALL subagents — no exceptions. The prepare script already filtered documents by file type. Every entry in subagent_prompts MUST get a subagent. Do NOT skip any.

Wait for all of them to return.

CRITICAL: NEVER post reviews, comments, or approvals to GitHub yourself. Do NOT use gh api, gh pr review, gh pr comment, or any GitHub API calls to post anything on any PR. All posting is handled exclusively by collect-results.py → post-review.py in Step 6. If you post reviews directly, it creates duplicates.

Step 4: Select candidates (zero LLM tokens)

python3 $BOT_DIR/.claude/skills/review-prs/select-candidates.py --work-dir "$WORK_DIR"

Its stdout is a JSON object whose validators array has one entry per PR with candidates left, each with a prompt_file. A PR with none is clean and needs no validator.

Step 5: Launch the validate subagents

For every entry in validators, launch a subagent with subagent_type: "review-prs-validate" and this prompt:

Read your instructions from: {prompt_file}

If the review-prs-validate agent type is not available, use subagent_type: "general-purpose" with model: "opus" and the same prompt.

Launch them all in a single message and wait for all of them to return. If validators is empty, skip to Step 6.

Step 6: Collect and post (zero LLM tokens)

Run the collector script — it reads the validators' result files and runs post-review.py:

python3 $BOT_DIR/.claude/skills/review-prs/collect-results.py --work-dir "$WORK_DIR" [--auto]

Pass --auto if auto_mode is true.

The script handles everything. Reviews are inline comments only, and never carry a body. A re-review resolves the threads the validator found addressed; a thread it did not rule on, or that GitHub will not resolve, stays open silently. With nothing new and nothing open the bot approves (an approval has no body), and re-approves when it is a requested reviewer; with an earlier comment still open it posts nothing, unless the bot is a requested reviewer: then it answers each open comment with an inline "Not fixed." on its line, as REQUEST_CHANGES under a verdict profile, so the request is answered. Each re-request answers again, so one with no new push stacks another "Not fixed." on the same line. A comment that is no longer on a line in the diff is dropped, and if none is left the PR is skipped, not reported as answered. With a profile review.verdict or review.requestChanges, a review that must be fixed before merging (a finding above nit severity) is submitted as REQUEST_CHANGES, and one with only nits as COMMENT; if GitHub refuses a request for changes without a body, the same inline comments are posted as a COMMENT review. A PR the bot opened always gets COMMENT, and a clean one gets nothing. Collecting violations from result files, prioritization/capping (25 per PR), rule link validation, deduplication, posting inline reviews, approval for clean PRs, cache updates, Signal notification, and the final summary block. A PR whose detect subagents all failed, or whose validator wrote nothing, is left out: nothing is posted and it is not cached, so the next run reviews it again.

For interactive mode (no --auto): before running collect-results.py, read each validator's results from {work_dir}/pr_{number}/validated.json, present each violation to the user for approval, write only approved violations back to that file, then run collect-results.py.

Read the script's stderr — it contains the summary. Print it for cron logs.

Summary of what consumes LLM tokens

Phase	Token cost	Who does it
Fetch PRs, diffs, comments	Zero	prepare-review.py
Build subagent prompts (files)	Zero	prepare-review.py, select-candidates.py
Resolve threads, check approval	Zero	prepare-review.py
Read manifest, launch subagents	~50 tokens per subagent	Main LLM session (Sonnet on the schedules)
Rule-checking the diff	Subagent tokens	Detect subagents (Sonnet), one per rule chunk plus one for bugs
Checking candidates against the source	Subagent tokens	Validate subagents (Opus), one per PR with candidates
Collect results, post, approve	Zero	collect-results.py + post-review.py

---

PR Link Format

When displaying PR numbers, ALWAYS use a full markdown link: [PR #<number>](https://github.com/$PR_REPO/pull/<number>). NEVER use bare #<number> — the TUI auto-links them against the wrong repository.

---

Closed/Merged PR Workflow

When reviewing closed or merged PRs and a violation is found:

1. Present the finding to the user (draft comment + ask for approval)
2. If approved, try to post inline review comments. If the API fails, fall back to:

   gh pr comment --repo $PR_REPO {number} --body "[file:line] comment text"
   

3. Create a follow-up issue in $PR_REPO to track the fix:

   gh issue create --repo $PR_REPO --title "Fix: <brief description>" --body "Found during post-merge review of PR #<NUMBER>. <description>"
   

   Where the finding is something a person can look at, show it rather than describe it: paste the screen as it renders today in a fenced block. For a full-screen terminal program its output is not its screen, so capture a raw run and replay it with scripts/terminal-screenshot.py — see pr-descriptions.md. Whoever fixes this has not seen the screen you found it on.
4. Reference the new issue back in the PR comment.